fix(gateway): gateway sessions lack activity watchdog - agent loop stalls silently with no detection and notification - #73031
Conversation
207a69b to
85c763a
Compare
|
@kshitijk4poor thanks again for the salvage/refactor work on the #72817 re-land path. Could you take another look when you have a chance? A few follow-ups landed on top of the restored watchdog set:
Happy to address any further review notes. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for restoring the watchdog and activity work. The current-main premise remains valid: active sessions queue follow-ups in gateway/platforms/base.py:5712-5724, while current startup has no session-stall watcher (gateway/run.py:10991-11073).
Problems
agent/conversation_compression.py:458-464does not honor the advertised total compression ceiling afterbegin_commit(): it spins until the fence lock is available and then calls unboundedfuture.result(). The commit fence covers SessionDB mutation such asarchive_and_compact()atagent/conversation_compression.py:2349andpublish_compression_child()at line 2412, so a hang there can still stall the turn indefinitely. The new test explicitly accepts an over-ceiling commit wait attests/agent/test_compress_context_progress_timeout.py:125-149.
Suggested changes
- Bound or otherwise make the commit phase recoverable, or narrow the documented/configured timeout guarantee and add a test for a commit-phase hang.
This is an automated hermes-sweeper review.
…out (NousResearch#72424) Three mechanisms to detect and notify when gateway sessions stall silently: 1. Mid-turn activity heartbeats stamped to SessionDB so hermes sessions list and hermes status show progress during long turns without new message rows. 2. Stall watchdog: when a busy session has pending inbound and the shared activity clock is idle past agent.session_stall_timeout (default 300), log a WARNING and notify the user once to try /new. Notify-only; does not kill the turn. 3. Compaction timeout: fenceless compress_context callers get a progress-aware host budget (compression.context_timeout_seconds default 120 idle, compression.context_total_ceiling_seconds default 600 ceiling). On timeout, cancel via commit fence, skip compaction without dropping messages, and continue the turn. Closes NousResearch#72016 (slices 1-3; slice 4 cumulative SSE stream-retry deadline remains a follow-up). Cherry-picked from PR NousResearch#72424 by @fangliquanflq.
Three code-reuse fixes applied during salvage: 1. Reuse _relative_time from hermes_cli/main.py instead of duplicating the relative-time formatting logic in hermes_cli/status.py. 2. Extract _stamp_hygiene_compression_provenance helper in gateway/run.py to deduplicate the two nearly-identical try/except blocks that stamp compression timeout/abort provenance in the hygiene path. 3. Add ContextCompressor.record_timeout_failure() method and use it from the in-agent compress_context timeout callback instead of re-implementing the (60, 300, 900) cooldown ladder inline. The existing summary-LLM exception handler already has this ladder — now both paths share one method.
…text compress_context now runs on a daemon pool worker thread (via run_compress_context_with_progress_timeout). The session id rotation updates hermes_logging._session_context (a threading.local) on the WORKER thread, not the caller thread. After the wrapper returns, propagate self.session_id back to the caller's logging context so subsequent log lines carry the rotated id (NousResearch#34089). Fixes CI failure in test_compression_logging_session_context.
Host progress timeout leaves compress_context running on a daemon worker while the live turn continues. Latch heartbeat silence on fence cancel or terminal timeout/cooldown provenance so a later UNKNOWN stamp cannot re-arm agent.compression and poison stall clocks.
Once begin_commit() wins, SessionDB mutation cannot be fence-cancelled; document that context_total_ceiling_seconds covers the summary phase only and pin the hang-wait contract in tests.
f82e688 to
7571298
Compare
Keep relay turn teardown when clearing activity labels after a turn exits.
Resolve gateway/run.py conflicts: keep session stall watchdog and hygiene activity provenance stamps; use DB-backed hygiene cooldown from main (NousResearch#74136).
…bility probe The stubbed-compressor fixture still let the one-time compression-model feasibility probe run inside the first _compress_context call. On machines with real credentials configured, that probe resolves a live auxiliary provider (credential pool seeding, Copilot token exchange over HTTPS), which nondeterministically exceeds the 2s event-timing budget in test_fence_cancelled_compression_leaves_lock_reacquirable (reproduced on PR #73031's own head). Mark the probe done in the shared fixture: these tests exercise locking/fencing/rotation, never aux feasibility. Suite runtime drops from ~90s to ~4s.
|
Closing as superseded by #76354 — @teknium1's clean re-land of the session activity watchdog onto current main, with the commit-phase ceiling fix and heartbeat write discipline composed with recent SessionDB merges. Your original work was solid (Teknium noted the revert was a process correction); #76354 carries it forward. Thanks! |
…bility probe The stubbed-compressor fixture still let the one-time compression-model feasibility probe run inside the first _compress_context call. On machines with real credentials configured, that probe resolves a live auxiliary provider (credential pool seeding, Copilot token exchange over HTTPS), which nondeterministically exceeds the 2s event-timing budget in test_fence_cancelled_compression_leaves_lock_reacquirable (reproduced on PR #73031's own head). Mark the probe done in the shared fixture: these tests exercise locking/fencing/rotation, never aux feasibility. Suite runtime drops from ~90s to ~4s.
…bility probe The stubbed-compressor fixture still let the one-time compression-model feasibility probe run inside the first _compress_context call. On machines with real credentials configured, that probe resolves a live auxiliary provider (credential pool seeding, Copilot token exchange over HTTPS), which nondeterministically exceeds the 2s event-timing budget in test_fence_cancelled_compression_leaves_lock_reacquirable (reproduced on PR #73031's own head). Mark the probe done in the shared fixture: these tests exercise locking/fencing/rotation, never aux feasibility. Suite runtime drops from ~90s to ~4s.
|
Superseded by PR #76354 (#76354), now merged — your full commit sequence was cherry-picked as its base with your authorship preserved via rebase-merge (the #72858 revert was process-only, and your work survived it intact). On top of your base, the merged PR carries two adversarial review rounds' worth of hardening (bounded commit-phase waits, worker transcript isolation, holder-qualified lease cancellation, bounded pool admission, watchdog send bounds) plus live-test verification of the heartbeat cadence, stall notify, and compression timeout behavior. Thanks for building the watchdog and for persisting through the revert — the activity/heartbeat design was validated as-is by both reviewers. Closing this re-land as superseded. |
…bility probe The stubbed-compressor fixture still let the one-time compression-model feasibility probe run inside the first _compress_context call. On machines with real credentials configured, that probe resolves a live auxiliary provider (credential pool seeding, Copilot token exchange over HTTPS), which nondeterministically exceeds the 2s event-timing budget in test_fence_cancelled_compression_leaves_lock_reacquirable (reproduced on PR NousResearch#73031's own head). Mark the probe done in the shared fixture: these tests exercise locking/fencing/rotation, never aux feasibility. Suite runtime drops from ~90s to ~4s.
…bility probe The stubbed-compressor fixture still let the one-time compression-model feasibility probe run inside the first _compress_context call. On machines with real credentials configured, that probe resolves a live auxiliary provider (credential pool seeding, Copilot token exchange over HTTPS), which nondeterministically exceeds the 2s event-timing budget in test_fence_cancelled_compression_leaves_lock_reacquirable (reproduced on PR NousResearch#73031's own head). Mark the probe done in the shared fixture: these tests exercise locking/fencing/rotation, never aux feasibility. Suite runtime drops from ~90s to ~4s.
What does this PR do?
Re-lands the session activity watchdog work after #72858 reverted #72817 (salvage of #72424).
Maintainer note on the revert: the original work is solid; the revert was a process correction because #72817 landed before a full review. This PR restores the same change set and is intended to go through the normal review cycle before merge.
Cherry-picks 3 commits from #72817 (skips the redundant noreply contributor mapping;
fangliquan@oppo.comis already mapped):_relative_time, hygiene provenance helper,record_timeout_failure()Behavior (unchanged from #72817 / #72424):
hermes sessions list/hermes statusshow progress during long turnsagent.session_stall_timeout(default 300) -> WARNING + one-shot/newnotify (notify-only)compress_contextcallers (compression.context_timeout_seconds/compression.context_total_ceiling_seconds)Does not include #72016 item 4 (cumulative SSE stream-retry deadline); that remains a follow-up.
Related Issue
Related to #72016 (closed when #72817 merged; code was later reverted by #72858)
Supersedes / re-opens the review path for #72424 and #72817
Type of Change
Changes Made
How to Test
hermes sessions list/hermes statusadvancelast_activewithout a new message row/newnotification; no spam on later ticksChecklist
Code
scripts/run_tests.shon relevant tests and they passDocumentation & Housekeeping
cli-config.yaml.exampleif I added/changed config keysCONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows - N/A