fix(selfhost): stop the queue drain from claiming new work during shutdown; add shutdown headroom to compose (#9007) - #9098
Merged
Conversation
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9098 +/- ##
==========================================
- Coverage 93.88% 92.66% -1.23%
==========================================
Files 810 810
Lines 80600 80603 +3
Branches 24454 24455 +1
==========================================
- Hits 75675 74689 -986
- Misses 3560 4840 +1280
+ Partials 1365 1074 -291
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…tdown; add shutdown headroom to compose (#9007) pump()'s `while (await processOne())` loop was blind to shutdown state: once a pump claimed the first due job it kept claiming every subsequent due job for as long as more existed, entirely ignoring a concurrent stop(). Graceful shutdown was therefore bounded by "drain the whole backlog," not "finish what's already in flight" -- and combined with Docker's 10s default stop_grace_period, every deploy during active review traffic sent SIGKILL before that drain completed, severing whatever pass was in flight and leaving its Redis locks/DB state exactly where they stood. This is the root cause behind the "PR looks stuck" incident class (orphaned ai-review-lock, stranded disposition, etc.). Introduces a `shuttingDown` flag distinct from `running`: `running` also gates pre-start()/direct pump()-drain() usage (binding.send()'s fire-and-forget kickOne(), and drain()'s own direct pump() call both legitimately run a pump loop before start() has ever been called, or after stop()) -- gating the drain loop on `running` itself breaks that path outright (verified: it makes every pre-start() drain()/send() a no-op). `shuttingDown` instead means only "stop() has been called and hasn't been superseded by a later start()": false until stop() sets it, reset by the next start(). Applied identically to both pg-queue.ts and sqlite-queue.ts. Also adds `stop_grace_period: 300s` to the app service in docker-compose.yml so the now-correct drain actually gets to run before Docker escalates to SIGKILL. Tests: two regressions per backend -- (1) stop() lets only the in-flight job finish rather than draining a preloaded 5-job backlog, reproducing the bug directly; (2) a pump kicked via send()/drain() before start() is ever called is unaffected by shuttingDown, guarding the exact mistake made and caught while fixing (1).
Contributor
|
Important 🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪 🔍 LoopOver is reviewing…AI analysis is in progress. This comment will update when the review is complete. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟪 Reviewing |
JSONbored
force-pushed
the
fix/9007-graceful-shutdown
branch
from
July 26, 2026 17:39
2627c9d to
1bc3974
Compare
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
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.
Problem
Every "PR looks stuck" incident this session (orphaned ai-review-lock, a disposition stranded between publish and execute, etc.) traced back to one root cause: the process cannot shut down cleanly, so a deploy during active review traffic SIGKILLs a pass mid-flight.
Two compounding defects:
pump()'swhile (await processOne())loop (bothsrc/selfhost/pg-queue.tsandsrc/selfhost/sqlite-queue.ts) never checked shutdown state — once a pump claimed the first due job it kept claiming every subsequent due job for as long as more existed, entirely blind to a concurrentstop(). Graceful shutdown was therefore bounded by "drain the whole backlog", not "finish what's already in flight".docker-compose.ymlhad nostop_grace_periodfor the app service — Docker's 10s default is far shorter than a review pass (AI calls + GitHub round-trips routinely take tens of seconds to minutes).Combined:
docker compose restart/stop/a redeploy during active traffic sends SIGKILL before the (buggy, unbounded) drain ever completes, severing whatever pass is in flight and leaving its Redis locks/DB state exactly where it stood.Fix
A
shuttingDownflag, deliberately distinct fromrunning. The obvious first attempt — gating the drain loop onrunningitself — is wrong and I caught it via a full-file test run before pushing:runningalso gates two other legitimate call shapes —binding.send()'s fire-and-forgetkickOne(), anddrain()'s own directpump()call — both of which run a pump loop beforestart()has ever been called (runningstartsfalseand isn't set until the firststart()). Gating onrunningmakes every pre-start()drain()/send()a silent no-op, which broke ~150 pre-existing tests relying on exactly that pattern in each file.shuttingDowninstead means only "stop()has been called and hasn't been superseded by a laterstart()":falseuntilstop()sets it (before its own drain-wait), reset by the nextstart(). Applied identically to both queue backends.Also adds
stop_grace_period: 300sto theloopoverservice so the now-correct drain actually gets time to run before Docker escalates.Tests
Two regressions per backend (4 total), plus an invariant guarding the exact mistake caught while building the fix:
concurrency:1,start(), let the lone pump claim job 1,stop()mid-consume — must return once job 1 finishes, not after 2-5 also drain.running-vs-shuttingDownmistake: a pump kicked viasend()/drain()beforestart()is ever called must be unaffected.Validation
npx vitest run test/unit/selfhost-pg-queue.test.ts test/unit/selfhost-sqlite-queue.test.ts— 280/280 passed (both full files, not just the new tests).npx vitest run test/unit/selfhost-compose-db-health.test.ts— 4/4 passed (existing compose structural checks).docker compose config --quiet— valid.tsc --noEmit --incremental false— clean.main(pre-existing repo-wide state), so left unreformatted rather than dragging in an unrelated tree-wide diff.Closes #9007