Bound the handler drain + de-flake the drain test - #1
Closed
jayjanssen wants to merge 5 commits into
Closed
jayjanssen wants to merge 5 commits into
jayjanssen wants to merge 5 commits into
Conversation
Bumps the Jackson BOM from 2.21.2 to 3.2.1 and moves the five Jackson-using source files onto the `tools.jackson` packages. `jackson-annotations` is deliberately left on `com.fasterxml` — it is not renamed in 3.x and the 3.2.1 BOM pins it to 2.22, which is what lets Jackson 2 and 3 coexist on a classpath. `jackson-datatype-jsr310` is dropped; java.time support is folded into databind in 3.x. Five changes are behavioural rather than mechanical: - Mappers are immutable in 3.x, but `SecretDeserializer` and `ResourceAwareDeserializer` parse nested documents with the very mapper they are registered on. They now take a `() -> ObjectMapper` supplier that resolves once `builder.build()` returns. `SecretJacksonModule` keeps its `ObjectMapper` constructor and exposes `mapper` as a computed property, so its source shape is unchanged. - `FAIL_ON_UNKNOWN_PROPERTIES` defaults to false in 3.x. Left alone, MiskConfig's "'x' not found in Config, did you mean...?" warning would silently never fire again and config typos would be ignored. It is now explicitly enabled for the first parse attempt; the retry path used to relax the mapper in place and instead rebuilds one. - `SORT_PROPERTIES_ALPHABETICALLY` and `EnumFeature.READ/WRITE_ENUMS_USING_TO_STRING` default on in 3.x and are pinned off. Sorting would reshuffle every service's redacted config dashboard, and config enums are matched by name while `toString()` is frequently overridden for display. - `KotlinFeature.StrictNullChecks` defaults on in 3.x and is pinned off. With it on a null element of a collection nested inside a map is rejected even when that element type is declared nullable, so `Map<String, Set<String?>>` fails where a top-level `Set<String?>` is accepted. Config that loads today would stop loading. - A `Map` field with an enum key is deserialized into an `EnumMap` in 3.x (databind#1853), which iterates in enum declaration order rather than in the order the keys appear in the YAML. There is no feature flag for it, but the rewrite only fires when the declared raw type is exactly `Map` and abstract type resolution runs first, so naming `LinkedHashMap` as the implementation settles it beforehand. Iteration order of a config map is observable and this would have changed it silently. This is a breaking change for consumers. The affected ABI, confirmed by the regenerated api dumps, is limited to: the `MiskConfig.load` overloads taking `JsonNode`/`ValueDeserializerModifier`, the three `SimpleModule` subclasses in misk-config, and `BackwardsCompatibleClientsConfigConverter`. Both misk-config and misk expose Jackson via `api(...)`, so consumers relying on the transitive dependency inherit Jackson 3.
…ss (cashapp#3898) HibernateDatabaseQueryDynamicAction authorizes a caller against a query class's metadata, then resolved the entity to actually query by Kotlin simpleName (`transacter.entities().find { it.simpleName == request.entityClass }`). simpleName is not unique: two entities in the same transacter can share one (e.g. a `DbMovie` in two different packages). When a colliding entity that was never exposed to Database Query is registered first, the lookup selects it, and the follow-up binding check added in cashapp#3827 also compares simpleName — so both the authorized and the colliding entity pass it — disclosing the unauthorized entity's rows. This is an incomplete-fix bypass of cashapp#3827 (VULN-78154), which used simpleName as the identity. Carry the authoritative KClass identities server-side instead of matching by name. A new internal HibernateDatabaseQueryRegistration pairs each entity's DatabaseQueryMetadata with the exact entity/query KClasses it was built from. It is multibound alongside the existing metadata and is never serialized, so the browser-facing JSON API (DatabaseQueryMetadata) is unchanged. - Dynamic action: run against registration.entityClass; still reject a request whose entityClass names a different entity than the authorized query (VULN-78154 contract). - Static action: run registration.queryClass / registration.entityClass directly instead of re-deriving the query by simpleName. - Transacter lookup: match by KClass identity rather than simple name. - Removes the now-unused HibernateQuery multibinding. Adds a regression test that reproduces the collision (two `DbMovie` classes, the protected one mapped to `actors` and registered first) and asserts the authorized `movies` entity is queried. CWE-863 (Incorrect Authorization), CWE-285 (Improper Authorization). Reported via Block's bug bounty program (Bugcrowd), tracked internally as VULN-78266. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
jayjanssen
marked this pull request as ready for review
August 17, 2026 12:03
* Shutdown SQS subscribers gracefully * Update tests to remove long polling * Fix the AsyncSwitch tests * Cancel in-flight SQS receives on shutdown stop() only stopped issuing new receives, so a receive already parked in await() kept the poller alive until its long poll expired. doStop() joins each subscription in turn, so that delay was paid per queue. Subscriber now tracks the in-flight ReceiveMessage futures and exposes cancelInFlightReceives() to abort them. doStop() first gives the poller a grace period to wind down on its own, and only cancels if it overruns: a long poll holding messages returns immediately, and abandoning it would leave those messages invisible until their visibility timeout expired. This also lets the tests go back to long polling. Co-Authored-By: Jay Janssen <691356+jayjanssen@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * API dump --------- Co-authored-by: Jay Janssen <691356+jayjanssen@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…top() - Add shutdown_timeout_ms to SqsQueueConfig: an optional per-queue bound on how long doStop() waits for in-progress handlers before cancelling the queue's remaining work. Default null keeps the unbounded join. - De-flake `queues are drained drain`: the Approximate* queue depth attributes are exact in ElasticMQ but eventually consistent on real SQS, so poll until conservation holds instead of asserting on a single read taken right after stop. - Remove the public SqsJobConsumer.stop(): it bypasses the Guava service state machine. Tests use stopAsync().awaitTerminated(). - New integration test for the stuck-handler path, config resolution tests for shutdown_timeout_ms, and the regenerated API dump. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jayjanssen
force-pushed
the
jayj/sqs-shutdown-hardening
branch
from
August 17, 2026 14:13
4f44f48 to
f74e067
Compare
Author
|
🤖 Re-targeted to cashapp/misk after cashapp#3895 merged: cashapp#3899 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up on top of cashapp#3895's branch (cashapp#3895) — three small things, take what you like:
1. Optional bound on the handler drain (
shutdown_timeout_ms, default null = current behavior).doStop()joins the handling jobs with no timeout, so one stuck handler holds shutdown until the pod is SIGKILLed — and a SIGKILL mid-drain loses the acks for every other job that finished after the kill signal would have been graceful. This adds a per-queue opt-in bound in the same shape asshutdown_grace_period_ms: on expiry, log and cancel that queue's remaining work (its messages stay in the visibility window for redelivery, same as today's cancellation). Default null keeps this PR's behavior unchanged. New integration test covers the stuck-handler path (shutdown terminates in ~1s, message left unacked for redelivery).2. De-flaked
queues are drained drain.I ran this branch through a real-SQS harness (against a throwaway staging account — see https://github.com/squareup/jayj-Notebook/tree/main/2026-08/real-sqs-validation): 12/14, and the shutdown logic itself validated cleanly, including the grace-period escalation. But this test's conservation assert read
visible 1 + invisible 1 + handled 999 = 1001when sampled immediately after stop —ApproximateNumberOfMessages*are exact in ElasticMQ but eventually consistent on real SQS. The test now polls the depths until conservation holds (or 20s), then asserts. Same intent, immune to counter lag — and cheap insurance against slow-CI timing too.3. Removed the public
stop().It bypasses the Guava state machine: calling it on a RUNNING service skips the STOPPING transition and listener lifecycle (and depending on Guava version
notifyStopped()may throw), leaving service state inconsistent with reality. Tests usestopAsync().awaitTerminated()instead.The real-SQS run also flagged
retrying worksas timing-tight against real AWS (redelivery vs a 10s latch) — pre-existing on master, so I left it alone here.🤖 Generated with Claude Code