fix(gateway): quiesce the thread pool before closing state.db at shutdown - #101118
kshitijk4poor merged 2 commits into
Conversation
…down
`_shutdown_executor()` ran *after* the SessionDB close block in `_stop_impl`,
and it never waited. That left two ways for blocking DB work to outlive
`SessionDB.close()`:
(a) `_executor_closing` was still False during the close, so a coroutine
reaching `_run_in_executor_with_context` minted a brand-new pool and ran
more blocking DB work against handles that had just been closed;
(b) `cancel_futures` only drops work that has not started, and cancelling
`self._background_tasks` does not stop the worker thread behind a
`run_in_executor` future that is already running.
`SessionDB.close()` checkpoints the WAL and lets SQLite unlink the sidecar. A
write that lands after it silently reopens the handle (NousResearch#94736) and mints a
fresh WAL generation behind that checkpoint, so teardown checkpoints the same
file a second time from a connection the shutdown log never accounts for --
the close-time page-write damage in NousResearch#101093 and the split WAL generation in
NousResearch#101064.
The quiesce now runs before the close and waits for the running workers. The
wait is bounded by `_EXECUTOR_QUIESCE_TIMEOUT` (2s) and clamped to what is left
of the shutdown watchdog leash minus a second for the close itself, so a stuck
worker can never cost the post-close cleanup window (NousResearch#82161). Workers still
alive after the budget are logged as a warning instead of being waited on.
`_shutdown_executor()` keeps its no-argument fire-and-forget contract and now
returns the number of workers still running.
Refs NousResearch#101093
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DGpGPnvz5FFFH999i59Xeb
alt-glitch
added
type/bug
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Strong root-cause work on the gateway-owned executor leg here. Moving _executor_closing ahead of the SessionDB close is the right direction, and the new ordering regression is useful: it makes the old close:session_db -> worker_write sequence concrete instead of hand-wavy.
I reviewed exact head 49dcb59079d05fc3570d6e0e73db0722926d1e7d against its recorded base fbd40c907d75e6b5cb0037845f06228ac1aa594f, current main (bcb412cd9d0566648d44b30bc1af2590a42db7f3 at review time), the current #101093 / #101095 / #101081 / #98991 graph, and the exact-head workflows. The single commit currently has green CI, Docker, and Nix receipts:
- CI: https://github.com/NousResearch/hermes-agent/actions/runs/33613260005
- Docker: https://github.com/NousResearch/hermes-agent/actions/runs/33613257639
- Nix: https://github.com/NousResearch/hermes-agent/actions/runs/33613257627
There are still merge blockers before I think this is safe to land.
Blocker 1 — the timeout branch still performs the unsafe close
In _stop_impl_body, _shutdown_executor() can return _exec_live > 0; the code logs:
a late write may reopen state.db
and then immediately proceeds into the SessionDB close block anyway.
That is the exact branch the PR is meant to make impossible. test_shutdown_executor_reports_a_stuck_worker() actually proves the helper can return 1 after the budget expires, but there is no stop-level regression for what happens next. The implementation therefore narrows the race window; it does not close it. The docstring claim that the bounded shutdown means DB work “cannot outlive SessionDB.close()” and the diagram’s “Nothing Left To Write” edge are stronger than the mechanism.
Required fix: make _exec_live > 0 a fail-closed DB-teardown boundary, not an observability-only warning. Either:
- do not enter a checkpointing/close path while a worker that can still mutate SessionDB is live, or
- compose this with a landed write-side fence that mechanically guarantees every surviving worker can no longer touch/reopen the DB before close.
Please add the converse regression: a worker that intentionally outlives the quiesce budget must be unable to produce a post-close SessionDB write / fresh WAL generation. Merely proving that it is counted and warned is not sufficient for the corruption class.
The out-of-loop shutdown watchdog already gives this path a bounded hard-exit backstop, so this does not require turning the join into an unbounded wait.
Blocker 2 — the PR knowingly leaves the other executor owner outside the boundary, and there is already adjacent work for it
The PR body correctly calls out the missing side: session-hygiene compression uses run_in_executor(None, ...), can outlive its asyncio waiter, and is not owned by the gateway executor being quiesced here.
That is not hypothetical repository context. Open #98991 (fix(gateway): drain detached hygiene workers, head b32ab64003441eccbf11a0cfd96ce4b69311c671) already implements the complementary lifecycle accounting: it retains timed-out hygiene worker futures, includes them in drain/watchdog/scale-to-zero accounting, hard-interrupts them, and keeps them in the cooperative settle window before SessionDB teardown. It is currently stale/unmergeable, but it is the existing contributor-authored implementation of precisely the “other side” this PR lists as a limitation.
Please establish an explicit composition path instead of leaving this as a known hole: rebase/salvage the #98991 mechanism or otherwise incorporate the required boundary while preserving ayushnangia's authorship/credit. This PR should not independently rediscover that work or claim class closure while a known DB-touching executor lane remains able to outlive teardown.
Blocker 3 — #101093 has two independent auto-closers, but this PR is complementary hardening, not the whole issue contract
#101093 itself enumerates the remaining shutdown/write requirements and points to #101095 as the PR implementing them: structural-corruption quarantine, no reopen, close-time checkpoint refusal, and transcript diversion. #101081 separately owns deleted-WAL-generation refusal. Those are complementary boundaries, not duplicates of this ordering fix.
Both #101095 and this PR currently say Fixes #101093. This PR’s own Limitations section says the late-write window is only narrowed when a worker survives the 2s budget and that the default-executor hygiene lane remains uncovered. If this merges first, GitHub can auto-close the P1 while the issue’s stated remediation is still open.
Please change this PR to Refs / Related #101093, or establish an explicit canonical close + prerequisite merge order on the graph. Preserve each contributor’s ownership: #101095 (leomcamilo) is the corrupt-handle/quarantine boundary; #101081 (rainbowgore) is the deleted-WAL-generation boundary; #98991 (ayushnangia) is the detached-hygiene-worker lifecycle boundary; this PR is the gateway-owned executor ordering/quiesce boundary.
Hard architecture gate — this adds shutdown behavior to the 27k-line gateway god-file
The behavior hunks land around lines 16.6k and 27k of gateway/run.py. Current AGENTS.md explicitly names gateway/run.py as a god-file that should be extracted into focused modules/mixins, and the repository’s development gate does not accept new behavior into an oversized owner.
Please move the executor/shutdown lifecycle into a focused sub-2k owner and leave a thin orchestration seam in gateway/run.py. Open #54962 is broader platform-routing extraction rather than ownership of this exact shutdown slice, so I would treat it as architectural coordination context, not as a reason to block on that particular issue.
Merge/current-head proof
At the final read before posting, this PR still has exact head 49dcb590..., but GitHub now reports it non-mergeable against the moving main; current main is four commits beyond the recorded base. Those four landed commits do not touch gateway/run.py, so I did not find a semantic collision there, but the final corrected/rebased head needs fresh exact-object CI/Docker/Nix proof. The current green runs certify only this unchanged head.
There is good work here. The ordering fix is real and the reproduction is valuable. What needs tightening is the terminal state: shutdown cannot say “a late write may reopen state.db” and then deliberately take the close/checkpoint boundary that makes that warning dangerous. Compose the executor owners, make the live-worker branch fail closed, reconcile the issue closer, extract the lifecycle owner, and this becomes a strong safety slice rather than a best-effort narrowing. 🚀
… budget andrexibiza's review on NousResearch#101118 pointed out that the timeout branch still ran the SessionDB close/checkpoint even when _shutdown_executor() reported a live worker -- the exact sequence that produces the wrong-page-number corruption in NousResearch#101093. The close block now only runs when _exec_live == 0; a surviving worker skips it entirely and leaves the handle open for SQLite to recover from its own WAL on next open. Adds test_stuck_worker_skips_the_session_db_close to prove the converse of the existing ordering test: a worker that outlives the budget must never be raced by close(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019JujDvCo2vfpEiizidAS2U
|
Thanks for the thorough review — pushed `600dbec398` addressing it. Blocker 1 (timeout branch still closed unsafely) — fixed. You were right that the warning was observability-only. The SessionDB close/checkpoint block now only executes when `_exec_live == 0`; a surviving worker skips it entirely instead of being raced. Added `test_stuck_worker_skips_the_session_db_close`, the converse regression you asked for: forces the quiesce budget to 0, confirms `close:session_db` never lands while the worker is still alive. Blocker 2 (composition with #98991) — acknowledged, not folded in. Confirmed #98991 is open and does implement the complementary default-executor drain/accounting for the hygiene lane. Per this repo's own norm on not reimplementing another contributor's PR under a different name, I didn't port that logic here — the PR body now names #98991 explicitly (Related Issue + Limitations) instead of leaving that gap unattributed. Blocker 3 (`Fixes` vs `Refs`) — fixed. Changed to `Refs #101093` with an explicit note that #101095 stays the canonical closer. Architecture gate — pushing back, open to discussion. I reread `AGENTS.md` lines 58-70: the god-file extraction is listed as "wanted work," and the same section says expanding reach "is a goal, not a footprint concern." I don't read that as a hard precondition that blocks unrelated bug fixes from landing in `gateway/run.py` until it's split up — and re-homing the shutdown lifecycle into a new module in the same PR as a corruption fix seems like it trades a real safety fix for refactor risk. Happy to file the extraction as a tracked follow-up instead, separate from #54962. Let me know if I'm misreading the gate. Full regression suite (6 files, 44 tests) + ruff still green after the change. |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Re-review of exact head 600dbec398e92b2dcd5832767f6b38d66b6bad0c.
The functional blockers from the prior review are resolved: the timeout branch now fails closed by skipping SessionDB close/checkpoint while a gateway-owned worker remains live; the #98991 ownership boundary is explicit rather than duplicated; and this PR no longer claims sole closure of #101093. Exact-head CI, Docker, and Nix are green.
One blocking architecture issue remains. This head still adds shutdown-lifecycle responsibility directly inside gateway/run.py at the existing ~16.5K-line shutdown tail. The repository explicitly identifies gateway/run.py as a god-file to extract rather than grow, and this change adds a new constant plus roughly a full shutdown-quiesce subsystem at that seam. The fact that the behavior is safety-critical makes the ownership boundary more important, not less: executor quiesce, SessionDB-close admission, watchdog budgeting, and the fail-closed decision need one focused owner below the god-file, with gateway/run.py delegating into it.
Please extract the new shutdown-quiesce/close-admission logic into a focused sub-2K module, keep the current regression contract byte-for-byte in behavior, and re-run the exact hosted matrix. I do not see another functional blocker on this head.
… budget andrexibiza's review on NousResearch#101118 pointed out that the timeout branch still ran the SessionDB close/checkpoint even when _shutdown_executor() reported a live worker -- the exact sequence that produces the wrong-page-number corruption in NousResearch#101093. The close block now only runs when _exec_live == 0; a surviving worker skips it entirely and leaves the handle open for SQLite to recover from its own WAL on next open. Adds test_stuck_worker_skips_the_session_db_close to prove the converse of the existing ordering test: a worker that outlives the budget must never be raced by close(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019JujDvCo2vfpEiizidAS2U
…cutor join The cherry-picked fix waited on cron AND deferred workers in a blocking time.sleep loop before the executor join, and returned early before _shutdown_executor() ran. That left the executor unsealed (the NousResearch#101118 regression), stalled the async stop phase, ignored the configured cron drain budget, and double-covered deferred workers, which are executor futures already joined by _shutdown_executor(). Keep only the verified gap: cron jobs run on the scheduler's own pool, so the executor join never sees them. Check _active_cron_job_count() AFTER the executor is sealed and fail closed the same way the executor-busy branch does — no second wait, the cron drain already spent its budget. Restores the NousResearch#82161 comment terminator the cherry-pick spliced over. Refs NousResearch#102198, NousResearch#102219.
…cutor join The cherry-picked fix waited on cron AND deferred workers in a blocking time.sleep loop before the executor join, and returned early before _shutdown_executor() ran. That left the executor unsealed (the #101118 regression), stalled the async stop phase, ignored the configured cron drain budget, and double-covered deferred workers, which are executor futures already joined by _shutdown_executor(). Keep only the verified gap: cron jobs run on the scheduler's own pool, so the executor join never sees them. Check _active_cron_job_count() AFTER the executor is sealed and fail closed the same way the executor-busy branch does — no second wait, the cron drain already spent its budget. Restores the #82161 comment terminator the cherry-pick spliced over. Refs #102198, #102219.
4 participants
What does this PR do?
Gateway shutdown now quiesces its thread pool before closing the session databases, waits (bounded) for the workers that are already running, and — per review — skips the close/checkpoint entirely if a worker still survives that budget, instead of racing it.
Symptom this closes
In the #101093 field incident the shutdown checkpoint on a live
state.dbwrote 15 valid page images under the wrong page numbers (page 1 ← amessages_fts_trigram_dataleaf), turning a file whosesessions/messagesstill scanned 100% intofile is not a database.Bug cause
_shutdown_executor()was called after the SessionDB close block in_stop_impl, and it never waited:_executor_closingwas stillFalsewhile the databases were being closed, so any coroutine reaching_run_in_executor_with_contextminted a brand-new pool (_get_executorrecreates it when_executor is None) and ran more blocking DB work against handles that had just been closed.executor.shutdown(cancel_futures=True)only drops work that has not started, and cancellingself._background_tasksdoes not stop the worker thread behind arun_in_executorfuture that is already running. The task dies; the thread keeps writing.SessionDB.close()checkpoints the WAL and lets SQLite unlink the sidecar. A write landing after that reopens the handle — reproduced on this branch:so teardown mints a fresh WAL generation behind a checkpoint that already ran, and then checkpoints the same file a second time from a connection the shutdown log never accounts for. That is the close-time page-write damage in #101093 and the split WAL generation in #101064.
The fix
_executor_closingis already set (no fresh pool) and running workers finish their writes first._shutdown_executor(drain_timeout=...)joins the worker threads.ThreadPoolExecutor.shutdown()has no timeout, so the threads are joined directly.min(2s, watchdog_leash − elapsed − 1s)— clamped to the shutdown watchdog leash exactly like the cron drain floor in Gateway drain exits after 0.00s with in-flight cron job, killing it mid-run #82161, so a stuck worker can never cost the post-close cleanup window.--replaceat worst, never a raced checkpoint. Seetest_stuck_worker_skips_the_session_db_close.tests/gateway/test_compress_command.py,test_session_env.pyare unaffected); it now returns the number of workers still running.%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#8b0000', 'mainBkg': '#0a0204', 'primaryTextColor': '#ffccd5', 'primaryBorderColor': '#ff0038', 'lineColor': '#ff0038'}}}%% graph TD A[🩸 SIGTERM] --> B[⚔️ Drain Active Agents] B --> C{Executor Quiesce} C -->|Before: skipped| D[🔥 Close SessionDB] D --> E[Checkpoint WAL + Unlink Sidecar] E --> F[💀 Late Worker Write] F --> G[Silent Reopen - New WAL Generation] G --> H[Second Checkpoint - Wrong Page Numbers] C -->|After: worker quiesced| I[✅ Workers Finish Writing] I --> J[🔒 Close SessionDB - Nothing Left To Write] J --> K[Single Accounted Checkpoint] C -->|After: worker still alive| L[⚠️ Skip Close Entirely] L --> M[🛡️ Handle Left Open - No Raced Checkpoint]Related Issue
Fixes #101093 (not
Fixes— see the ownership graph below; the issue's own remediation plan names #101095 as the PR that resolves it, and this PR should not auto-close it out from under that work).Deliberately not overlapping:
leomcamilo) owns the write-side quarantine, the corrupt-handle close-checkpoint skip and the transcript divert — all insidehermes_state.py/gateway/session.py/run_agent.py. This PR touches neither file and does not changeSessionDBbehaviour at all; it removes the window in which a post-close write exists, which is orthogonal to what that handle does once it happens.rainbowgore) refuses opens/writes on an already-deleted WAL generation (detection, Linux/proc). This prevents the gateway from creating one at shutdown.recoverable: true.ayushnangia, open) owns the other executor lane this PR does not touch: session-hygiene compression dispatched viarun_in_executor(None, ...)on the loop's default executor. It retains timed-out hygiene futures and folds them into drain/watchdog/scale-to-zero accounting. That mechanism is that PR's to land — this PR does not reimplement it (see Limitations).Suggested merge order for full closure of #101093: this PR (gateway-owned executor ordering) can land independently since it touches only
gateway/run.py's shutdown tail — no file overlap with #101095, #101081, or #98991. #101095 should still be treated as the canonical closer of #101093.Type of Change
Changes Made
gateway/run.py:_EXECUTOR_QUIESCE_TIMEOUT = 2.0;_stop_impl_bodycalls_shutdown_executor(drain_timeout=...)before the SessionDB close block and logs the outcome; the old post-close call is removed;_shutdown_executortakes an optional boundeddrain_timeout, joins the workers and returns how many are still alive. Added after review: the SessionDB close/sweep block now only runs in the_exec_live == 0branch — a surviving worker skips it entirely instead of being raced.tests/gateway/test_shutdown_executor_quiesce.py: 6 tests — ordering (a running future writes before the close),_executor_closingset before the close, a stuck worker causes the close to be skipped (not raced), the default call still does not wait, a stuck worker is reported within budget, and the no-executor case.How to Test
python -m pytest tests/gateway/test_shutdown_executor_quiesce.py tests/gateway/test_compress_command.py tests/gateway/test_session_env.py tests/gateway/test_shutdown_cache_cleanup.py tests/gateway/test_restart_drain.py tests/gateway/test_bounded_adapter_teardown.py -q→ 44 passed.ruff check gateway/run.py tests/gateway/test_shutdown_executor_quiesce.py→ clean.gateway.run._EXECUTOR_QUIESCE_TIMEOUT = 0.0) on the original ordering test and it flips to['close:session_db', 'worker_write']— the close lands while a worker is still writing.test_stuck_worker_skips_the_session_db_closeasserts"close:session_db"never appears in the event log while the worker is still alive — the close is skipped, not raced.Limitations
run_in_executor(None, ...), seerun_codex_hygiene_compaction) and is abandoned on its inactivity timeout, so that worker is untouched by this PR's quiesce/skip logic and can still be mid-write when this PR's close block runs (or is skipped). fix(gateway): drain detached hygiene workers #98991 already implements the complementary drain/accounting mechanism for that lane and should land to close this specific hole — this PR does not attempt to fold that logic in, to avoid re-doingayushnangia's work under a different PR.Review feedback addressed (
andrexibiza, review #5088169464)_exec_live == 0; a surviving worker skips it. New regression testtest_stuck_worker_skips_the_session_db_closeproves the converse the review asked for.ayushnangia's open PR and stays theirs to land; this PR now names it explicitly in Related Issue/Limitations instead of leaving an unattributed gap.FixesvsRefs, dual auto-closer) — fixed. Changed toRefs #101093with an explicit merge-order note; fix(state): quarantine SessionDB handle after structural corruption (no retry, no close-time checkpoint) #101095 remains the intended closer.gateway/run.pyshutdown lifecycle before merging) — respectfully declined for this PR.AGENTS.md's god-file section (lines 65-70) lists thecli.py/run_agent.py/gateway/run.pyextraction as "wanted work" alongside "expand reach at the edges... breadth in the product is a goal, not a footprint concern" (lines 58-64) — it does not state that new behavior is refused into an oversized owner as a merge precondition. This diff is 86 lines inside an already-existing, deeply-tested shutdown sequence; re-homing that sequence into a new module in the same PR as a corruption fix would substantially raise the risk on a P1 safety fix for no correctness gain. Filing the extraction as its own follow-up (distinct from Extract Gateway Platform Routing from gateway/run.py #54962, which is broader platform-routing extraction) seems like the safer path — open to being told otherwise.Checklist
cli-config.yaml.example— N/A (no config keys)Fixes 101093