fix(agent): turn liveness watchdog surfaces silent turn stalls (#95548) - #95663
fix(agent): turn liveness watchdog surfaces silent turn stalls (#95548)#95663Finn763 wants to merge 1 commit into
Conversation
alt-glitch
added
type/bug
|
@pr-comment-95663.md |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Blocking review — exact head 3ea547f2d8cd60deb32789f568489203a571ee70
Separating turn liveness from lease renewal is the right model, and the three regression lanes target the correct failure classes: silent stall, continued progress, and an interrupt-resistant wedge. I cannot clear this head yet.
1. Blocker — the stall decision can become stale before the hard cancel commits
In run_agent.py::_watch_turn_liveness, the watchdog samples _last_activity_ts, releases durable_turn_lease_activity_lock, logs and emits the warning, and only then calls _interrupt_turn(). _touch_activity() cannot participate in this function-local lock. A real progress stamp—or normal turn completion—can therefore land after the sample but before the interrupt. The watchdog will still announce an abort and can hard-cancel a turn that has resumed, contradicting the PR's no-false-positive claim.
Bind the abort to the observed activity generation/timestamp and revalidate at the commit point under synchronization shared with _touch_activity(). Add a race regression where activity resumes during the warning/callback window; that turn must continue and its lease must keep renewing.
Exact source: https://github.com/NousResearch/hermes-agent/blob/3ea547f2d8cd60deb32789f568489203a571ee70/run_agent.py
2. Blocker — this grows the run_agent.py godfile instead of extracting the policy
At this exact head, run_agent.py still contains code past line 9,200, and this PR adds 120 lines / changes 137 lines there for a new runtime-policy subsystem. The repository guide explicitly calls for multi-thousand-line clusters in run_agent.py to be extracted into focused modules and for the core to remain a narrow waist.
Move the watchdog configuration, state machine, and thread mechanics into a bounded module (for example, agent/turn_liveness.py) and leave only the smallest integration seam. Do not add another policy cluster to the godfile.
Repository guidance: https://github.com/NousResearch/hermes-agent/blob/7d6c6ae4aedfd932533b8638e1d51db07654d792/AGENTS.md
3. Blocker — the proposed user-facing configuration surface is explicitly disallowed
This PR introduces HERMES_TURN_LIVENESS_WATCHDOG_S and HERMES_TURN_LIVENESS_WATCHDOG_POLL_S as behavioral knobs. AGENTS.md explicitly rejects new non-secret HERMES_* configuration and requires behavioral settings such as timeouts and thresholds to live in config.yaml; an internal env bridge is allowed only behind that user-facing authority.
Wire this through the config schema/defaults and test precedence plus invalid values. The current raw float() parsing also turns a typo into a durable-turn startup exception, while NaN/Inf can silently disable the timeout or kill the watcher thread.
4. Blocker — there is no current-main, exact-head acceptance receipt
As reviewed, this head is 95 commits behind current main (7d6c6ae4aedfd932533b8638e1d51db07654d792) and the PR is currently non-mergeable. The exact-head CI workflow was cancelled, and its Python-test job was skipped. Docker and Nix succeeded, but neither proves this Python runtime change.
- Cancelled CI: https://github.com/NousResearch/hermes-agent/actions/runs/33002626123
- Successful Docker workflow: https://github.com/NousResearch/hermes-agent/actions/runs/33002625329
- Successful Nix workflow: https://github.com/NousResearch/hermes-agent/actions/runs/33002625379
Rebase onto current main, remove any diff already present there (including the contributor mapping as applicable), then run the focused watchdog/concurrency suites and full head-bound CI until every required check is terminal green.
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Follow-up on exact head 554d1e5ad5c2e0c7468642c3d111b041923d0cef: this object has regressed out of reviewable scope.
GitHub now reports 121 commits, 222 changed files, +21,599 / -1,707 for a PR whose body still describes a narrowly scoped turn-liveness watchdog. The submitted diff now includes unrelated Docker image-provenance work, the ACP/OpenAI bridge, a very large Desktop/session/profile surface, MCP changes, update/restart work, locales/docs, and many unrelated tests. That is not a current-main composition of the #95548 fix; it is a broad composite branch.
The exact-head workflows are green — CI 33039008755, Docker 33039008302, and Nix 33039008308 — but those runs certify this 222-file composite, not a coherent #95548 landing object. They therefore cannot close the four blockers on my prior review. In particular, I cannot infer that the activity-generation race, godfile extraction, and config-authority problems were repaired just because unrelated current work was accumulated onto the branch.
Please reconstruct this as one current-main, liveness-only object containing the intended #95548 implementation/tests plus the actual fixes for the prior blockers, then rerun exact-head CI/Docker/Nix. Until the branch is reduced back to a coherent product delta, this head is not landable or meaningfully reviewable.
554d1e5 to
241f8e4
Compare
|
Rebuilt from scratch — this head is now one current-main, liveness-only object. Apologies for the scope accident: a fork-state sync had accumulated ~120 unrelated upstream commits onto the branch (the 121-commit / 222-file composite). I reconstructed the reviewed object plus all four blockers of the prior review onto fresh main and force-pushed. New head Point-by-point on the blocking review:
Ready for re-review once CI reports on this head. |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Re-review on exact head 241f8e484f092f89ad1b9c93b3836e61d0c04eb0 after the current-main reconstruction.
Three prior blockers are closed on this object: the branch is back to a focused five-file liveness carrier, policy/configuration moved into bounded agent/turn_liveness.py, and the raw environment-knob bypass is gone. Exact-head CI 33046255725, Docker 33046255241, and Nix 33046255196 are all green.
One P1 race remains in the abort commit itself. _commit_turn_liveness_abort() revalidates the sampled (generation, activity_ts) under _liveness_activity_lock(), but then releases that lock before it acquires the durable-turn lease lock and calls interrupt(..., hard_cancel=True). _touch_activity() uses the same liveness lock, so real progress can publish a new generation in that post-validation/pre-interrupt gap and still be killed by the already-authorized abort. The new regression only resumes activity while the warning callback is blocked, i.e. before commit revalidation, so it does not exercise this remaining window.
Please add a deterministic schedule that pauses after the snapshot has passed revalidation but before the hard interrupt is committed, calls _touch_activity() there, and proves cancellation is vetoed. Then make the accepted-abort state atomic relative to _touch_activity() (or use an equivalent deadlock-safe generation claim) so progress cannot cross the final commit boundary unnoticed. Once that gap is closed, the architecture/authority and exact-head evidence on this narrow carrier otherwise look clean.
249d45c to
241f8e4
Compare
|
Thanks for the round-3 catch — the post-revalidation window was real: the commit revalidates the
|
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Re-reviewed exact response head 249d45c7bc45042f77e8bc439cec8737d230e92d against live main@6607f706732492b4ffd226e38a73a9994ac97fd1, including the full five-file diff, the new watchdog module/config contract, the exact AIAgent.interrupt() control flow, current regression tests, prior review history, exact-head workflow state, #95548, and the active interrupt/recovery neighborhood.
There is real progress here. Moving the policy into bounded agent/turn_liveness.py, making configuration authoritative, and carrying the sampled activity generation into the interrupt API are all the right direction. The previous outer race — progress after _commit_turn_liveness_abort() revalidation but before entering interrupt() — is now covered.
One P1 remains in the same generation-authority class: the generation claim is still validated before, rather than at, the destructive publication boundary.
At this head, AIAgent.interrupt(..., require_generation=G) does:
- acquire
_turn_liveness_activity_lock; - compare live
_turn_liveness_activity_generationtoG; - release that lock;
- later acquire
_interrupt_redirect_lock, publish_interrupt_requested = True, and for a hard cancel call_admit_hard_cancel(); _admit_hard_cancel()may itself block in the compression fence'scancel_before_commit(...)before the hard-cancel event is published.
That leaves a deterministic post-validation/pre-publication window. A resumed turn can publish real progress through _touch_activity() after step 3, advancing the generation to G+1, while the already-admitted interrupt still proceeds through steps 4–5 and kills the healthy turn. The generation proof has become stale before the mutation consumes it.
The new regression does not cover that window. test_watchdog_declines_abort_when_activity_resumes_after_revalidation monkeypatches agent.interrupt with a wrapper that calls _touch_activity() before delegating to the real interrupt(). The real interrupt therefore sees G+1 during its initial generation check and declines. That proves the old outer window is fixed, but not the new internal window after that check.
A deterministic regression can hit the residual schedule without timing luck: install a controllable _active_compression_commit_fence.cancel_before_commit that blocks after interrupt() has passed its internal generation check; while it is blocked, call _touch_activity() to advance the generation; release the fence; assert that neither _interrupt_requested nor the hard-cancel event is published and that lease renewal remains active. The current control flow will still publish the stop.
There is one second manifestation of the same contract in _commit_turn_liveness_abort(): if interrupt(...) raises, the exception fallback directly sets _interrupt_requested / _interrupt_message and treats the abort as published without re-consuming the generation claim. An exceptional interrupt path therefore discards the very proof this round added.
Required repair: make the sampled generation an actual cancellation claim consumed at the final mutation edge. Since this PR correctly avoids holding the activity lock across a potentially unbounded compression-commit wait, a good shape is a generation-bound abort token/CAS that _touch_activity() invalidates and that the publisher consumes only after any blocking fence, immediately before publishing the interrupt/hard-cancel state. A final recheck earlier in the call chain is not enough if another blocking boundary remains after it. The exception fallback must either consume the same claim or decline the abort fail-closed; it cannot convert inability to validate/publish through the normal path into unconditional interrupt authority.
Please add the post-internal-validation regression above plus an exception-path regression proving a stale/indeterminate claim cannot be turned into a direct flag mutation.
Graph / ownership: #95548 remains the root same-boot silent-stall incident this PR should close. #96204 is complementary cross-boot retry bounding for restart-interrupted sessions; it should not absorb this no-progress watchdog class. #24201 (qWaitCrypto) is an active direct semantic collision in AIAgent.interrupt() for concurrent-tool detach/late-result suppression, so whichever lands second needs a fresh semantic composition of the interrupt sink rather than a textual conflict resolution. Closed #72039 and #72079 (fangliquanflq) remain useful activity-clock / notify-only watchdog provenance, not merged authority to inherit silently.
The branch is currently 2 commits ahead / 26 behind its actual live-main merge base 6defe7eb6c462bb784d1f27f5afe7ca4b627fc70. The 26 current-main commits I checked are path-disjoint from the production files in this PR, so the new blocker is not main drift; nevertheless, final acceptance still needs the repaired exact head composed onto landing main.
Exact-head evidence is mixed: Docker 33059359421 and Nix 33059359443 are green, while CI 33059360360 completed failure with zero jobs returned by the workflow API. I am not assigning a test cause where GitHub exposes no job-level failure receipt. This exact commit is therefore not fully green yet.
The watchdog itself is increasingly well-shaped: single progress clock, explicit generation, bounded policy module, loud surfacing, and lease-renewal withdrawal are the right primitives. The last thing to make exact is the thing this watchdog is allowed to kill: the generation proof must survive all the way to the hammer. 🚀
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Re-review on exact head 249d45c7bc45042f77e8bc439cec8737d230e92d: the round-3 patch moves the race later, but it does not make the generation claim atomic with interrupt publication.
interrupt(require_generation=G) re-checks G under _liveness_activity_lock(), then releases that lock before it mutates _interrupt_requested / _interrupt_message, admits the hard cancel through the compression fence, signals tool workers, and propagates to children. _touch_activity() is free to publish generation G+1 anywhere in that post-check interval. The interrupt then still publishes against the stale claim.
The new regression does not hit this remaining window. Its wrapper calls _touch_activity() before entering real_interrupt(...), so the new generation check sees the bump and returns False; it proves the commit→interrupt-entry window, not the generation-check→interrupt-publication window.
Required closure is one actual commit point shared with _touch_activity(): either (a) reserve/claim the generation under the activity lock and make _touch_activity() invalidate/refuse that reservation before any interrupt state becomes visible, or (b) restructure interrupt admission so the generation re-check and first observable cancellation publication are atomic without holding the activity lock across the potentially blocking compression fence. Add a deterministic witness that pauses immediately after the require_generation comparison succeeds, publishes _touch_activity(), then resumes interrupt() and proves no interrupt flag/event/tool signal is published.
Exact-head Docker 33059354396 and Nix 33059354449 are green. CI 33059360360 terminated as failure with zero jobs returned/executed, so there is also no exact-head CI acceptance object to transfer. Source-wise, the five-file reconstruction/config extraction remains clean; this last publication race is the remaining correctness blocker.
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Re-review on exact head 249d45c7bc45042f77e8bc439cec8737d230e92d.
The new generation claim closes the window between _commit_turn_liveness_abort() and entry into AIAgent.interrupt(), but it does not make the abort atomic relative to _touch_activity() at the actual hard-cancel publication point.
interrupt(require_generation=G) checks G under _liveness_activity_lock() and then releases that lock before it acquires _pending_redirect_lock, sets _interrupt_requested, and calls _admit_hard_cancel(). _admit_hard_cancel() can itself wait on the compression commit fence. _touch_activity() needs only the liveness lock, so real progress can still land after the new generation check and before either _interrupt_requested = True or the hard-cancel event is admitted. That resumed turn is then killed by a claim that was valid only before the second check.
The new regression does not exercise this remaining window: its wrapper calls _touch_activity() before delegating to the real interrupt(), so the injected generation bump is observed by the new generation check and causes the expected False. It never pauses after that check while the real interrupt is waiting to publish.
Required repair: make the accepted abort claim indivisible with the interrupt publication, or add a final generation validation at the actual publication boundary under a lock protocol that prevents _touch_activity() from slipping between validation and publication without holding the liveness lock across an unbounded compression-fence wait. A generation-claim state/compare-and-commit primitive is fine if it preserves the existing lock-order constraints. Add a deterministic regression that pauses after the current require_generation check but before _interrupt_requested / hard-cancel admission, publishes _touch_activity(), then proves no interrupt state/event is published and lease renewal continues.
Exact-hosted evidence on this head is also not fully green: Docker 33059354396 and Nix 33059354449 succeeded, while CI 33059360360 completed failure with zero jobs exposed by the run API. That is an execution-evidence gap, not support for or against the race above.
Status: the round-3 race is narrowed but not closed; this exact head is not substantively ready yet.
|
@andrexibiza — thank you for the round-4 re-review; both P1 items are closed on head 1. Generation re-check and first observable publication are now atomic — one commit point shared with
2. Exception-path defect closed. 3. Two deterministic regressions (TDD — both RED at
4. Local test results on this head (Windows, venv
CI: the previous exact-head run ( Please re-review when you have a moment — happy to iterate further. |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Blocking re-review — exact head 34d0105a5f9cf1b32ae987a9731b5d09bbddba2f
Reviewed the current head and the current-main merge object 713ad90bbe6bc4b63fd027262ea0f486126202f9 against main 5fc308a70719a83cccdbba4c0e39c23f5a8239d5. Round 4 materially closes the earlier generation-check → compression-fence window, and the exceptional watchdog path now declines instead of manufacturing interrupt authority. One P1 race remains.
P1 — claim consumption and first interrupt publication are still separate critical sections
The linked follow-up says the generation check and first observable publication are atomic. They are not atomic on this tree.
In run_agent.py::interrupt, _consume_generation_claim() acquires _liveness_activity_lock(), verifies the reserved generation, clears _turn_liveness_abort_claim, then releases the activity lock and returns. Both publication branches subsequently execute:
if not _consume_generation_claim():
return False
self._interrupt_requested = True
self._interrupt_message = message
self._tool_interrupt_reason = tool_interrupt_reasonThat leaves this valid interleave:
- The watchdog consumes claim
G, clears the reservation, and releases the activity lock. - The recovered turn enters
_touch_activity(), acquires that same lock, and publishes generationG+1. The reservation is alreadyNone, so there is nothing left to invalidate. - The watchdog resumes and publishes
_interrupt_requested, the interrupt reason, and the hard-cancel event against the now-healthy turn.
The round-4 regression parks inside cancel_before_commit(), which is before _consume_generation_claim(). It proves the fence window is closed, but it does not exercise this consume → publication window. The race is narrower, not closed.
Keep the potentially blocking compression-fence work before the activity critical section. Then, under one acquisition of _liveness_activity_lock(), re-check/consume the claim and publish the first interrupt state before releasing the lock. The critical section only needs to contain bounded state/event publication; it does not need to hold the activity lock across the fence or child/tool propagation.
Add a deterministic regression that parks exactly after successful claim validation/consumption and before _interrupt_requested is assigned, publishes real activity on the competing thread, and proves the current tree goes red. On the repaired tree, the generation winner must be total: either progress wins and no interrupt flag/event/tool/child signal is published, or the interrupt publication commits under the lock before later activity.
Hosted evidence and commit hygiene
The new exact-head merge object is green:
- CI
33071281886: success - Docker
33071281387: success - Nix
33071281374: success
That closes the prior hosted-evidence gap; CI is not the substantive blocker in this review.
The current head itself is an empty ci: re-trigger after timing-report transient failure commit over the unchanged 0f8075e tree. When repairing the P1, drop/squash that CI-only commit and rerun the surviving real commits so the branch is every-commit green rather than carrying red predecessor heads plus an empty green trigger.
Disposition: not ready. The current matrix is green, but the destructive publication edge is still not generation-atomic.
34d0105 to
7c932e3
Compare
7c932e3 to
2991225
Compare
|
@andrexibiza — thank you for the round-6 re-review; the consume→publication window was real, and it is closed on head Round-6 P1 — atomic claim consumption + first publication: Deterministic regressions (TDD): CI-caught regression, fixed in the same commit: the first round-6 push ( Test counts on this head: |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Follow-up review — exact head 299122558.
The round-6 P1 is closed. The claimed interrupt path now performs the claim check/consumption and the first observable interrupt publication (_interrupt_requested, _interrupt_message, _tool_interrupt_reason, and the hard-cancel event) inside one _liveness_activity_lock() critical section — the same lock _touch_activity() uses to advance the generation and invalidate a reservation. That gives the activity stamp and abort commit one total order. The direct boundary witness is structurally valid: it parks release #2, proves publication is already committed before competing G+1 activity can acquire the lock on this head, and was red on 0f8075ecc. The unclaimed path remains lock-free, preserving the unrelated AIAgent stand-ins.
The current-main composed object is also real: GitHub synthesized 8e380476 from current main 0dfba37b plus this head, and all three exact-head workflows completed successfully:
So there is no remaining source-level blocker in the round-6 repair.
One acceptance blocker remains: the submitted branch is still four commits on merge base 6defe7eb6c, currently 127 commits behind main, and two surviving real commits still carry red exact-object CI receipts:
Dropping the empty 34d0105a5 trigger was correct, but it did not satisfy the prior requirement to make the surviving branch history every-commit green; a green top commit does not retroactively turn those exact objects green. The PR body is also stale where it calls 6defe7eb6c current main and reports the earlier +950/−22 object.
Please rebuild/squash the four real commits into one coherent commit on current main, update the base/diff receipt in the PR body, and rerun CI/Docker/Nix on that resulting exact head. Do not add another empty CI-only trigger commit. The alternative is to rebase onto current main and obtain green exact-object reruns for every surviving commit, but the one-commit reconstruction is cleaner here.
Disposition: not ready solely on commit-lineage/current-main acceptance. The round-6 concurrency defect itself is resolved.
Overnight Verification ✅Tested this PR in an isolated worktree against Tests: 657/658 Code change verified: New
Architecture note: The race-safety analysis is thorough (6 rounds documented in the module docstring). The key invariant: a turn that resumes while the stall is being evaluated is never hard-cancelled — the commit callback revalidates the This addresses the silent-stall pattern where turns hang forever with the lease renewing. No regressions detected. This is ready to merge. |
2991225 to
703083e
Compare
|
Round-7 feedback addressed on the rebuilt head
|
…atchdog (NousResearch#95548, NousResearch#95663) Add a turn-liveness watchdog keyed to the agent activity clock: a turn that stalls mid-flight while the durable lease keeps renewing is logged loudly, surfaced to the UI, force-interrupted, and — when the hard interrupt cannot unwind the wedge — lease renewal is stopped so stale-turn cleanup can reclaim the session. Race safety (rounds 3/4/6 of the NousResearch#95663 review, all folded into this squashed commit): - AIAgent.interrupt(require_generation=G) re-validates the generation claim at the last instant before the hammer; a stale claim abandons the abort and the turn continues. - The claim is reserved under the activity lock, invalidated by any real progress in _touch_activity(), and consumed immediately before the first observable interrupt publication; exceptional paths fail closed. - Claim consumption and the first interrupt publication are atomic inside one _liveness_activity_lock() critical section; unclaimed interrupts publish lock-free so AIAgent stand-ins without the liveness seam keep working (CI 33096454629 regression, fixed here). Deterministic race regressions (written red-first) in tests/run_agent/test_turn_liveness_watchdog.py cover the post-revalidation window, the consume-to-publication window, the exceptional path, and atomic claim consumption. Round-7 rebuild: single squashed commit on current origin/main; the former four-commit lineage (241f8e484..299122558 on merge base 6defe7e) no longer exists, so no surviving commit carries a red exact-object CI record, and no empty CI-trigger commit was added.
703083e to
45df75e
Compare
|
Round-7 (amended): after my previous round-7 message,
|
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 45df75e262b243d92cd836c84e442f223d59daf7 after the requested lineage/current-main reconstruction.
The remaining acceptance blocker from my 299122558 review is closed. This branch is now exactly one surviving commit directly on current main@10b388300a63d83857fac3ca4f8b05b64e01bc50; the five-file delta is the same focused liveness object, without the older red intermediate commits or an empty CI-trigger commit. Under the every-commit-green rule, the entire surviving branch history is therefore the current head itself.
I re-read the destructive publication edge rather than transferring the round-6 verdict blindly. The generation-bound path still reserves the observed generation under _liveness_activity_lock(), runs the potentially blocking compression-fence admission before the commit point, then consumes the claim and publishes the first observable interrupt state + hard-cancel event inside the same activity-lock critical section. _touch_activity() uses that same lock, so the progress-vs-abort winner is total at the mutation boundary. The deterministic test_interrupt_consumes_claim_and_publishes_first_state_atomically witness remains in the submitted object.
Exact-head hosted authority is fully green on this one surviving commit:
- CI
33118138956— success - Docker
33118138394— success - Nix
33118138266— success
GitHub also reports the PR mergeable, and the submitted base is the exact current main above. I found no remaining blocker from my review on this object.
Current-main revalidation ✅ (Pattern-E review pass)andrexibiza's final review cleared the last blocker against
With the blocking reviewer's acceptance + green revalidation on today's main, this looks merge-ready. It's the architectural anchor for the Pattern-E cluster (silent stalls / liveness gaps) from the 2026-08 perf triage — #92316 and #92318 (both armed) cover the startup and hygiene-compression flavors; this covers the in-turn flavor. |
|
too large to review safely This PR changes 662 production lines before tests and docs. Please split it or add a focused justification if it should stay together. Signed: GPT-5.6-luna-high in Codex |
alt-glitch
added
comp/cli
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Blocking re-review — exact head 45df75e262b243d92cd836c84e442f223d59daf7
Re-read the focused five-file object, the round-6 generation/interrupt publication repair, the watchdog’s visible-settlement path, its regression suite, live main@4209d371aa1bb8840ce8447555bdd863a1a96c38, and the latest scope objection.
The generation-bound destructive edge still reads correctly: claim consumption and first interrupt publication are total-ordered with _touch_activity() under the shared activity lock. I am not reopening that race. There is one different blocker in the layer immediately before it.
Blocker — the watchdog publishes a definitive abort outcome before the abort has authority
agent/turn_liveness.py::TurnLivenessWatchdog._watch() currently does this in order:
_surface_stall(snapshot);_commit_abort(snapshot, message);- only if the commit returns
True,_deactivate_turn()and return.
But _surface_stall() already logs:
Force-aborting the turn and stopping lease renewal
and emits the user-visible warning:
aborting it so the session can recover
The next operation is specifically allowed to veto that outcome. If activity resumed during warning delivery, _commit_abort() returns False, the watcher keeps sampling, the original turn continues, and its lease keeps renewing.
This is not a hypothetical schedule. test_watchdog_declines_abort_when_activity_resumes_during_warning deliberately resumes the turn from inside _emit_warning(), then proves the turn completes normally with no interrupt and continued lease renewal — after the definitive abort/lease-stop message has already been published. The test even asserts the pre-commit error log was emitted.
Exact source:
- https://github.com/NousResearch/hermes-agent/blob/45df75e262b243d92cd836c84e442f223d59daf7/agent/turn_liveness.py
- https://github.com/NousResearch/hermes-agent/blob/45df75e262b243d92cd836c84e442f223d59daf7/tests/run_agent/test_turn_liveness_watchdog.py
That makes the visible/operator settlement disagree with the mutation authority: a recovered, still-running turn is reported as force-aborted with lease renewal stopped. For this PR, surfacing is part of the product contract, not decorative logging.
Required repair:
- Keep the pre-commit surface observational only: stall detected / recovery attempt beginning. It must not claim that interruption or lease withdrawal has committed.
- Publish the definitive aborted/lease-stopped settlement only after
_commit_abort()succeeds and the turn is deactivated. - Extend the existing
...resumes_during_warningwitness to capture warning/log text and prove the declined path contains no committed-abort or lease-stop claim. - Add the complementary committed-path assertion proving the definitive settlement appears when interruption and lease withdrawal actually win.
Exact-current-main acceptance is stale again
GitHub’s current synthetic merge ref is 180d06712e1291d360f4472df6f7b7c2caed0b7f, built on main@9d9f44d63826b18503f44c754e48e1f4f83b3a6e. Live main is now 4209d371aa1bb8840ce8447555bdd863a1a96c38, 59 commits later. The PR’s three owned production files did not change in that interval, but the semantic neighborhood did: current main includes 835a913ffd599e52b7843334c8abb74726c6aeeb in agent/conversation_compression.py, the compression path traversed by this PR’s hard-cancel fence, plus new compaction tests. No hosted workflow run exists on merge object 180d067; the Aug 27 CI/Docker/Nix receipts certify the head and its then-current composition, not today’s landing object.
After the settlement repair, update onto landing main and require terminal-green CI, Docker, and Nix on the resulting exact object.
Scope disposition
The raw 662 production-line count is not, by itself, a reason to split this PR. The production delta is one bounded watchdog policy module, one config-authority surface, and the interrupt/lease integration required to make that policy race-safe; the remaining 903 added lines are adversarial regression coverage. Arbitrarily splitting those ownership edges would make the invariant harder to review by creating intermediate objects without the complete sample → claim → publication → lease-withdrawal chain.
The object is large but cohesive. The blocker is false settlement before commit, not raw line count.
|
Merged via #99758 (salvage) — your commits cherry-picked with authorship preserved as Your watchdog went through 8 review rounds here and survived all of them with its core design intact — the activity-clock keyed liveness, the generation-claim machinery from rounds 3/4/6, and the deterministic race witnesses were all preserved verbatim. The salvage carries three additions on top of your head:
Thanks for the persistent, precise iterations — this PR is materially better for them. |
…atchdog (#95548, #95663) Add a turn-liveness watchdog keyed to the agent activity clock: a turn that stalls mid-flight while the durable lease keeps renewing is logged loudly, surfaced to the UI, force-interrupted, and — when the hard interrupt cannot unwind the wedge — lease renewal is stopped so stale-turn cleanup can reclaim the session. Race safety (rounds 3/4/6 of the #95663 review, all folded into this squashed commit): - AIAgent.interrupt(require_generation=G) re-validates the generation claim at the last instant before the hammer; a stale claim abandons the abort and the turn continues. - The claim is reserved under the activity lock, invalidated by any real progress in _touch_activity(), and consumed immediately before the first observable interrupt publication; exceptional paths fail closed. - Claim consumption and the first interrupt publication are atomic inside one _liveness_activity_lock() critical section; unclaimed interrupts publish lock-free so AIAgent stand-ins without the liveness seam keep working (CI 33096454629 regression, fixed here). Deterministic race regressions (written red-first) in tests/run_agent/test_turn_liveness_watchdog.py cover the post-revalidation window, the consume-to-publication window, the exceptional path, and atomic claim consumption. Round-7 rebuild: single squashed commit on current origin/main; the former four-commit lineage (241f8e484..299122558 on merge base 6defe7e) no longer exists, so no surviving commit carries a red exact-object CI record, and no empty CI-trigger commit was added.
Closes the #95663 round-8 review blocker (false settlement before commit veto): the pre-commit surface (`_surface_stall`) logged "Force-aborting the turn and stopping lease renewal" and warned the user "aborting it so the session can recover" BEFORE `_commit_abort` could veto — so a turn that resumed during the warning window (or an exceptional interrupt path that declines fail-closed) was reported as force-aborted with lease stopped while it actually continued running. - Split the surface: `_surface_stall` is now observational only ("no progress for Ns; attempting recovery"), and the definitive aborted/lease-stopped settlement moves to a new `_surface_committed_abort` that runs only after `_commit_abort` succeeds and the turn lease is deactivated. - Rate-limit repeated pre-commit surfaces per observed generation: a turn whose aborts keep declining no longer re-logs an ERROR and re-warns the user every poll interval. - Add the committed-path regression test (`test_watchdog_publishes_definitive_settlement_only_after_commit`) and extend the declined-path witness (`...resumes_during_warning`) to assert no committed-abort or definitive pre-commit claim appears when the abort is vetoed. Both fail on the pre-fix tree (mutation-checked). - Document the `_interrupt_turn` lease-loss asymmetry (fires unconditionally, no generation claim — losing the lease means the process no longer owns the session). - Trim review-round archaeology from comments/docstrings (keep the WHY, drop the round numbering), and drop the dead `cancel_event` compat note from the test fence. - Document `agent.turn_liveness` in the configuration guide. On top of PR #95663 by Finn763 (cherry-picked with authorship preserved).
|
Scope disposition (for the record, since this PR was superseded by #99758): andrexibiza's round-8 review adjudicated the size objection directly — the raw 662 production-line count is one bounded watchdog policy module ( |
|
Thanks @Finn763 — the bug (#95548's silent turn wedge) is real and your final design direction (activity-clock-keyed, never lease renewal) is right, but +1,527 lines into run_agent.py/turn machinery is more surface than we can take into the most invariant-critical code in the repo, and the squash-rebuilt branch history makes the review burden worse. Closing per maintainer decision — this is a size/blast-radius call, not a correctness one. If we build a minimal version of the watchdog we'll credit your design work on the detection predicate; a resubmission as small reviewable slices (detector / interruptor / lease-stop as separate PRs) would also be welcome. |
…atchdog (NousResearch#95548, NousResearch#95663) Add a turn-liveness watchdog keyed to the agent activity clock: a turn that stalls mid-flight while the durable lease keeps renewing is logged loudly, surfaced to the UI, force-interrupted, and — when the hard interrupt cannot unwind the wedge — lease renewal is stopped so stale-turn cleanup can reclaim the session. Race safety (rounds 3/4/6 of the NousResearch#95663 review, all folded into this squashed commit): - AIAgent.interrupt(require_generation=G) re-validates the generation claim at the last instant before the hammer; a stale claim abandons the abort and the turn continues. - The claim is reserved under the activity lock, invalidated by any real progress in _touch_activity(), and consumed immediately before the first observable interrupt publication; exceptional paths fail closed. - Claim consumption and the first interrupt publication are atomic inside one _liveness_activity_lock() critical section; unclaimed interrupts publish lock-free so AIAgent stand-ins without the liveness seam keep working (CI 33096454629 regression, fixed here). Deterministic race regressions (written red-first) in tests/run_agent/test_turn_liveness_watchdog.py cover the post-revalidation window, the consume-to-publication window, the exceptional path, and atomic claim consumption. Round-7 rebuild: single squashed commit on current origin/main; the former four-commit lineage (241f8e484..299122558 on merge base 6defe7e) no longer exists, so no surviving commit carries a red exact-object CI record, and no empty CI-trigger commit was added.
Closes the NousResearch#95663 round-8 review blocker (false settlement before commit veto): the pre-commit surface (`_surface_stall`) logged "Force-aborting the turn and stopping lease renewal" and warned the user "aborting it so the session can recover" BEFORE `_commit_abort` could veto — so a turn that resumed during the warning window (or an exceptional interrupt path that declines fail-closed) was reported as force-aborted with lease stopped while it actually continued running. - Split the surface: `_surface_stall` is now observational only ("no progress for Ns; attempting recovery"), and the definitive aborted/lease-stopped settlement moves to a new `_surface_committed_abort` that runs only after `_commit_abort` succeeds and the turn lease is deactivated. - Rate-limit repeated pre-commit surfaces per observed generation: a turn whose aborts keep declining no longer re-logs an ERROR and re-warns the user every poll interval. - Add the committed-path regression test (`test_watchdog_publishes_definitive_settlement_only_after_commit`) and extend the declined-path witness (`...resumes_during_warning`) to assert no committed-abort or definitive pre-commit claim appears when the abort is vetoed. Both fail on the pre-fix tree (mutation-checked). - Document the `_interrupt_turn` lease-loss asymmetry (fires unconditionally, no generation claim — losing the lease means the process no longer owns the session). - Trim review-round archaeology from comments/docstrings (keep the WHY, drop the round numbering), and drop the dead `cancel_event` compat note from the test fence. - Document `agent.turn_liveness` in the configuration guide. On top of PR NousResearch#95663 by Finn763 (cherry-picked with authorship preserved).
6 participants
What Problem This Solves
A conversation turn can stall mid-flight (observed in #95548 between "model returned tool_calls" and tool execution, after a slow model response + a desktop WS disconnect) with no error logged, no further progress, and the durable turn lease kept renewing — so nothing force-aborts the turn and the session stays wedged until the process is killed.
This PR adds a turn-liveness watchdog that keys off the agent's activity clock (the #72039 single progress source: stamped by API waits, stream tokens, tool heartbeats and tool completions — never by lease renewal). When a turn shows no observable progress for the configured idle bound it logs the stall loudly, emits a UI-visible warning, force-interrupts the turn so it surfaces as an interrupted turn the user can retry, and — when the hard interrupt cannot unwind the wedge — stops lease renewal so durable-lease TTL expiry lets stale-turn cleanup reclaim the session.
Fixes #95548
Scope note (round-7 rebuild): the branch is one squashed commit on current
origin/main. The four earlier real commits (241f8e484→249d45c7b→0f8075ecc→299122558) built on the old merge base6defe7eb6cno longer exist as separate objects, so no surviving commit carries the red exact-object CI records (33059360360,33069621017). Net diff vs the base: 5 files, +1527/−38 — production filesagent/turn_liveness.py+292/−0,hermes_cli/config_defaults.py+14/−0,run_agent.py+318/−38 (production subtotal +624/−38); test filestests/agent/test_turn_liveness.py+139/−0 andtests/run_agent/test_turn_liveness_watchdog.py+764/−0 (tests subtotal +903/−0). No unrelated upstream commits ride along.Architecture
Bounded module —
agent/turn_liveness.pyowns the whole policy: settings resolution/validation (resolve_turn_liveness_settings), the sampled-idle state machine (TurnLivenessWatchdog), and thread mechanics.run_agent.pykeeps only the integration seam: lazy shared-lock acquisition + generation counter inside_touch_activity, a settings-resolve block next to the durable lease refresher, and commit/deactivate callbacks owning turn-lease state.Config authority — replaces the raw
HERMES_TURN_LIVENESS_WATCHDOG*env knobs (zero references remain anywhere):Defaults declared in
hermes_cli/config_defaults.py. Invalid values (non-numeric typo, NaN, Inf, non-positive poll) log a warning and fall back to documented defaults — they never crash durable-turn startup, never silently disable the timeout (NaN), and never freeze the watcher thread (Infpoll).timeout_s <= 0is the documented opt-out.Race safety (fix(agent): turn liveness watchdog surfaces silent turn stalls (#95548) #95663 review) —
_touch_activitystamps the clock under a shared per-agent lock and bumps a monotonic generation counter; the watchdog binds its abort decision to the sampled(generation, timestamp)pair and revalidates that pair at the commit point under the same lock. A turn that resumed while the stall was being logged/emitted is declined: it continues and its lease keeps renewing. The lock is deliberately released for the surface/log window and only re-taken at the commit point, so acting on the observation cannot deadlock with_interrupt_turn.Race-safety history (rounds 3/4/6 — all folded into the squashed head)
AIAgent.interrupt(require_generation=G)re-compares the generation claim against the live clock under the activity lock immediately before the hammer, and abandons the abort when the claim went stale._touch_activity(), and consumed immediately before the first observable interrupt publication; fence admission runs publication-free, and exceptional paths fail closed (a raisinginterrupt()publishes nothing)._interrupt_requested/_interrupt_message/_tool_interrupt_reasonassignment and the hard-cancel event all commit inside ONE_liveness_activity_lock()critical section — the same lock_touch_activitystamps the clock with. Unclaimed interrupts (require_generation is None) publish lock-free soAIAgentstand-ins without the liveness seam keep working (regression caught by full-suite CI33096454629, fixed on this head).Deterministic race regressions (written red-first) in
tests/run_agent/test_turn_liveness_watchdog.py:test_watchdog_declines_abort_when_activity_resumes_after_revalidationtest_watchdog_declines_abort_when_activity_resumes_inside_interrupt_publicationtest_watchdog_declines_abort_when_interrupt_publish_raisestest_interrupt_consumes_claim_and_publishes_first_state_atomicallyEvidence
All runs on the exact squashed head
45df75e26(venvpython -m pytest ... -q, isolatedHERMES_HOME):tests/agent/test_turn_liveness.py— resolver/validator suite: defaults when section missing, explicit-value precedence over defaults, numeric strings accepted,timeout_s=0opt-out, typo → warn + default without startup crash, NaN timeout → default (never silently disabled), Inf poll → default (watcher never frozen), Inf timeout fallback, non-positive poll fallback, malformed sections fallback.tests/run_agent/test_turn_liveness_watchdog.py— behavior + race suite: force-abort of a silently stalled turn, no fire while the turn still makes progress, lease-renewal stop when the interrupt cannot unwind the wedge, plus the four deterministic race regressions above.test_session_activity.py7 passed,test_sequential_tool_timeout.py5 passed,test_compression_concurrent_fork.py46 passed,test_cron_direct_api_call_62151.py4 passed,test_cron_inline_api_call_62151.py2 passed → 64 passed, zero failures.test_start_order_gate.py(the CI-caught stand-in seam) — 3 passed.py_compileclean on all touched production files.origin/main=10b388300a63d83857fac3ca4f8b05b64e01bc50, fetched immediately before the rebuild. The old merge base6defe7eb6cis no longer the base. The squashed head45df75e26sits directly on10b388300; CI/Docker/Nix rerun automatically on this exact head after push.Round-7 follow-up (2026-08-28)
Addressing the round-7 acceptance blocker (andrexibiza, review 5044827674): the four surviving real commits were built on the stale merge base
6defe7eb6cand two of them carried red exact-object CI records, so any exact-head CI receipt would remain red by history. Rebuilt as one coherent commit on current main:git rebase --onto origin/main 6defe7eb6c fix/95548-turn-liveness-watchdogreplayed all four commits with zero conflicts; the replay tree was then squashed (git reset --soft origin/main && git commit) into a single commit on top of the then-currentorigin/main(28ee6ac04).origin/mainadvanced past that base (28ee6ac04→10b388300, PR fix(compression): dedupe current-turn rows when rotation splits the session mid-turn #96636), the single commit was re-based onto the newestorigin/main(again zero conflicts) and the commit message amended to summarize the full feature plus the rounds 3/4/6 race fixes. Final head:45df75e26on10b388300— still exactly one commit.+1527/−38, same 5 files), so no code changed — only the commit lineage and base.