Skip to content

Bound the SQS handler drain on shutdown + de-flake the drain test - #3899

Draft
jayjanssen wants to merge 1 commit into
cashapp:masterfrom
jayjanssen:jayj/sqs-shutdown-hardening
Draft

jayjanssen wants to merge 1 commit into
cashapp:masterfrom
jayjanssen:jayj/sqs-shutdown-hardening

Conversation

@jayjanssen

@jayjanssen jayjanssen commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #3895 (now merged) — three small things on top of it. FYI @mateuszmrozewski since this builds directly on your merge; 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 as shutdown_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 #3895'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 #3895's 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 = 1001 when 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 use stopAsync().awaitTerminated() instead.

The real-SQS run also flagged retrying works as timing-tight against real AWS (redelivery vs a 10s latch) — pre-existing on master, so I left it alone here.

🤖 Generated with Claude Code

…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant