fix(gateway): stop hung sessions from auto-resuming on every restart - #96204
fix(gateway): stop hung sessions from auto-resuming on every restart#96204fangliquanflq wants to merge 6 commits into
Conversation
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 7f609df6487d9f713e0567e4a6796be5cdfee1a6 against live main@f45477b4a8e03600a13fc3ca14c75d277558d526, including both changed files, the full restart-resume regression surface, #96181, the original stuck-loop/resume lineage, the current no-progress-watchdog owner, the gateway/run.py decomposition owner, and exact-head CI. There were no prior reviews or PR comments when I started.
The central correction is good: counting the synthetic resume before dispatch is the right side of the SIGKILL boundary. Reusing .restart_failure_counts instead of inventing another session counter is also the right ownership decision, and preserving unrelated pending counters when one session is suspended fixes a real multi-session bookkeeping loss. The long-gap regression is useful because it explicitly removes the 300-second process-wide restart-chain breaker and proves the per-session mechanism independently. Exact-head CI/Docker/Nix are all green.
I cannot clear this head yet. There are two P1 contract problems plus one hard architecture/interlock gate.
P1 — the new durable-attempt safety gate fails open when its proof cannot be persisted
_record_startup_resume_attempt() is now the load-bearing protection for the exact failure class in #96181: the attempt must exist durably before a hard-killable resume is allowed to run. But both failure paths erase that distinction:
try:
counts = json.loads(path.read_text(...)) if path.exists() else {}
except Exception:
counts = {}
...
try:
atomic_json_write(path, counts, indent=None)
except Exception:
logger.debug(...)and _schedule_resume_pending_sessions() ignores the result and continues directly into the session-slot claim/task creation.
That creates two unsafe states:
- if the existing ledger is unreadable/corrupt, the code promotes unknown prior attempts to
0and can overwrite that evidence with a fresh count of1; - if the atomic write itself fails (permissions, disk/full-path failure, filesystem error), the resume is still dispatched with no durable attempt receipt at all.
For ordinary telemetry, best-effort persistence is reasonable. Here it defeats the feature: a SIGKILL after the fail-open dispatch leaves the next boot with exactly the missing evidence this PR is meant to fix, so the poisoned session can remain replayable indefinitely.
Required repair: make the counter advance an admission condition, not a side effect. _record_startup_resume_attempt() should return/raise a result that proves the increment was durably committed; _schedule_resume_pending_sessions() must refuse this synthetic resume when the ledger cannot be read or advanced. Do not reinterpret malformed persisted state as an empty trustworthy ledger. Preserve/quarantine it and fail closed until it can be reconciled.
Please add deterministic regressions for at least:
- an existing valid count plus
atomic_json_writefailure → no resume task/adapter dispatch and prior evidence remains authoritative; - malformed/unreadable
.restart_failure_counts→ no reset-to-zero/overwrite and no resume dispatch; - successful persistence → dispatch proceeds with the committed increment.
#18179/#17842 already established atomic bytes for this control-plane file. This PR now makes successful publication itself part of the authority contract; swallowing the publication failure is the remaining other side of that shape.
P1 interlock/closure — this does not close the live no-progress half of #96181
#96181 describes two separable failures:
- the same
resume_pendingsession is re-admitted on later boots because a SIGKILL bypasses shutdown-time counting; - one admitted startup resume can make no progress for ~3,718 seconds / ~100% CPU before the existing idle watchdog forces completion, and the issue explicitly requires a much shorter startup-resume no-progress bound.
This PR fixes (1). It deliberately leaves (2) unchanged: after _schedule_resume_pending_sessions() creates the task, the first/second/third poisoned attempts can still sit hot for roughly an hour apiece if nobody restarts the gateway. So Closes #96181 currently makes the repository graph claim the observed single-boot liveness defect is resolved when it is not.
There is already a complementary owner for that class: #95663 (Finn763), which implements the #95548 turn-liveness watchdog using an activity clock rather than lease renewal. That PR is not a duplicate of this one: #96204 owns cross-boot retry-budget durability; #95663 owns in-boot no-progress termination. #95663 still has an outstanding final commit-point race in its current review, so do not copy its unfinished implementation here.
Required graph repair: either change this PR to Partially fixes #96181 / otherwise keep #96181 open and explicitly bind its remaining no-progress requirement to #95663/#95548, or—only after #95663's race is repaired—compose that accepted liveness primitive before claiming full closure. One counter and one watchdog, each with a single owner; no duplicate liveness subsystem.
#55105 is adjacent evidence of the same zombie-resume family on Photon, but it includes adapter/sidecar-specific failure modes and should remain distinct rather than being silently closed by this generic counter change.
Hard architecture gate — these methods already have an extracted lifecycle owner
This PR adds ~60 lines of new restart/session-lifecycle authority directly back into gateway/run.py, around lines 11,370–12,486. The repo-wide decomposition ledger #78647 records gateway/run.py as KILLED via 38 shards under the standing “sharded, never reverted” rule.
More specifically, #77738 is not merely an adjacent refactor: its GatewayLifecycleMixin explicitly owns this exact cluster, including _increment_restart_failure_counts, _suspend_stuck_loop_sessions, shutdown bookkeeping, and startup auto-resume of restart-interrupted sessions. The later restart shard lineage (#79392) reinforces the same ownership boundary.
So this change should be composed through the lifecycle shard rather than regrowing the monolith. Keep the behavioral fix and its tests, but place the new attempt-recording semantics with the extracted lifecycle owner/re-export/MRO contract. This is a direct FILE-LIST/ownership collision, not a request to create a second implementation.
Provenance / merge-order
- #9941 is the merged origin of
.restart_failure_countsand the successful-turn clear contract. - #11852 (BrennerSpear) is the restart-continuity specification; merged #12301 implemented
resume_pendingwhile explicitly reusing the #9941 counter rather than creating a parallel one. - #18179 preserves johnncenae's #17842 atomic-control-file work and is the persistence foundation this PR builds on.
- #77738 owns the extracted Gateway lifecycle/restart cluster; compose this behavior there rather than adding a new run.py island.
- #95663 is complementary no-progress authority; keep the retry-budget and watchdog responsibilities separate, then interlock them through #96181.
Exact-object evidence / landing state
Exact head is green:
- CI
33054285846: https://github.com/NousResearch/hermes-agent/actions/runs/33054285846 - Docker
33054284967: https://github.com/NousResearch/hermes-agent/actions/runs/33054284967 - Nix
33054285037: https://github.com/NousResearch/hermes-agent/actions/runs/33054285037
Live main advanced after this branch point; the PR is currently 1 commit ahead / 11 commits behind merge base 93a29d110db7724f18959cb941aae3520fb587ac. I inspected that main-only delta and it is path-disjoint from both PR files, so I found no landing-edge semantic collision from those 11 commits. The lifecycle-shard and #95663 interlocks above are the material ones.
This is a useful fix and the core pre-dispatch counting idea should survive. The remaining work is to make that count authoritative when persistence is unhealthy, keep the still-open no-progress defect visible instead of closing over it, and land the behavior in the lifecycle owner that already exists. Once those are true, the SIGKILL recovery story is materially stronger rather than just better on the happy filesystem path. 🚀
andrexibiza
left a comment
There was a problem hiding this comment.
Re-review on exact head e4580eaa55c2f080c18c5533ee949463eea560fa after the persistence/topology repair.
The two substantive blockers from my prior review are closed on this object. _record_startup_resume_attempt() now treats a readable, valid ledger plus a successful atomic write as admission authority and returns false on malformed/read failure or publication failure; _schedule_resume_pending_sessions() refuses before session-slot/task creation when that authoritative increment is unavailable. The new regressions cover existing-count/write failure, malformed ledger, successful persisted dispatch, malformed sibling state, concurrent clear/admission, failed suspension persistence, and failed resume-marker clearing. SessionStore.clear_resume_pending() also restores the in-memory marker if _save() fails, so retry evidence is not consumed ahead of durable session state.
The graph claim is corrected as well: the PR now says Partially fixes #96181 and keeps #95663/#95548 as the complementary in-boot no-progress owner.
I also need to correct one claim from my prior review. I treated #77738's GatewayLifecycleMixin as though it were already repository authority. It is not: #77738 is still open and unmerged, and gateway/lifecycle_mixin.py does not exist on live main@74ec63fe18fec174299af2371febd122e6dd5c42. Current main still owns this lifecycle code in gateway/run.py, so that was not a valid present-tense architecture blocker. If #77738 lands first, this PR will need semantic composition into the extracted owner; until then that is a merge-order interlock, not grounds to reject this current source shape.
I do not have a remaining source-level correctness blocker in these three files.
Exact-head hosted evidence is not fully green, however: Docker 33061363316 and Nix 33061363271 succeeded. CI 33061368786 completed failure and its workflow API returns zero jobs. I attempted the reversible failed-run rerun and GitHub returned 403 Resource not accessible by integration, so I cannot promote this exact commit to accepted/complete while the CI authority remains red/no-job.
What does this PR do?
Prevents a restart-interrupted gateway session from being auto-resumed forever after repeated hard kills. The gateway now counts each synthetic startup resume before dispatch, so attempts survive SIGKILL and the existing stuck-session suspension threshold can retire a repeatedly hung session while the gateway continues serving new inbound messages.
Symptom
A
resume_pending=restart_interruptedsession can consume one CPU thread until the agent idle watchdog fires, then be replayed again on every later gateway restart.Impact
The affected session repeatedly consumes CPU for up to the watchdog duration and cannot recover without manual session-state surgery. The startup restore gate is already bounded for other channels, but it does not prevent the same bad session from being replayed on the next boot.
Bug Cause
Trigger:
gateway/run.py/GatewayRunner._schedule_resume_pending_sessions()schedules a fresh synthetic turn for every eligible restart-interrupted session.Causal chain:
resume_pendingsession..restart_failure_countsupdate.Why it is wrong: the global restart-loop guard only chains boots whose gaps stay within its configured maximum. A resume that hangs for about an hour exceeds the default 300-second gap, so each boot begins a new global chain while the per-session shutdown counter remains unchanged.
Working sibling / contrast: fast crash loops are stopped by the global restart-loop guard, and graceful shutdowns update the existing per-session counter. The missing case is a long startup resume terminated by SIGKILL.
Ruled out: the startup restore drain bound is not the missing guard. It releases queued inbound messages after its timeout, but intentionally leaves the slow resume turn running and does not persist a failed attempt for the next boot.
Fix
.restart_failure_countsfile and successful-turn clear path rather than adding a second session counter.Related Issue
Partially fixes #96181. This PR owns the cross-boot retry-budget durability half. The remaining in-boot no-progress timeout is tracked by #95548 and its complementary implementation in #95663.
Type of Change
Changes Made
gateway/run.py- make durable retry publication an admission condition and serialize retry-ledger state transitions.gateway/session.py- restore the in-memory resume marker when durable clearing fails.tests/gateway/test_restart_resume_pending.py- cover retry bounds and fail-closed persistence/error/concurrency paths.How to Test
Run the restart-resume gateway regression suite:
The focused suite passes 48 tests on this branch.
Checklist
Code
fix(scope):,feat(scope):, etc.)Documentation & Housekeeping
cli-config.yaml.exampleis N/A because no config keys changedCONTRIBUTING.mdandAGENTS.mdare N/A because no architecture or workflow changedScreenshots / Logs
N/A - this is gateway session-state behavior covered by the focused regression suite.