Follow-up to c0d974b19 (#79741). Three review findings against that commit,
none of which change the escalation behaviour it shipped.
1. The recovery decision lived inline in `_handle_message_with_agent`, a
~2000-line async method, so the only way to pin it was a test that read
`inspect.getsource(...)` and asserted on substrings. AGENTS.md bans reading
source in tests outright and names this exact situation: "if the logic lives
inline in a god-file (gateway/run.py) and extracting it feels disruptive:
that's the actual signal to do the extraction, not to regex around it."
Those tests were not merely stylistically wrong, they were actively harmful.
One asserted the substring `_new_tokens < _approx_tokens` was PRESENT -- so
it passed while the gate had the bug that substring represents, and had to be
edited when the gate was fixed. It failed on correct code and passed on
broken code, in one assertion.
Extracted `hygiene_compaction_recovered()` as a module-level pure predicate
and replaced the three source-reading tests with eight direct unit tests.
The extraction immediately earned itself: the new tests caught a `NameError`
(the predicate called `compression_made_progress` while the module bound it
under an alias) that a source-text assertion cannot see, because the symbol
is spelled correctly in the source and only fails at runtime.
2. The gate inferred "did the transcript actually get rewritten" from a numeric
side effect -- the degenerate "did not rotate or compact in place" path
(#21301) reuses the pre-compression counts -- when the booleans
`_hyg_rotated` / `_hyg_in_place` were already in scope and explicitly set
False on that path. The predicate now takes them directly, so a future edit
that re-estimates instead of reusing the old counts cannot silently defeat
the escalation.
3. `_record_hygiene_cooldown` passed no `error` to
`record_compression_failure_cooldown`, which writes `compression_failure_error`
unconditionally -- so a hygiene failure clobbered to NULL whatever reason the
in-conversation path had recorded, and readers then show the user "unknown
error" (agent/manual_compression_feedback.py, gateway/slash_commands.py). The
reason was already in hand at both call sites. Pre-existing from #74136 but
amplified by escalation: a blank reason on a 45-minute cooldown is far more
user-visible than on a 5-minute one.
Also: the ladder docstring described the compressor's absolute 60/300/900s
ladder while the constant is multipliers (1, 3, 9); the config docs still
described `hygiene_failure_cooldown_seconds` as a flat interval rather than the
first rung of a capped ladder; and `PersistentState.hygiene_failure_streak` now
documents that it is process-local by design -- keying on `session_key` is what
survives compaction rotation, which the persisted `compression_*_streak`
columns cannot express since they key on the rotating `session_id`. Making it
durable is a schema change, tracked on #79624 rather than smuggled in here.
Also replaces the file's hand-written `_Runner` stub with
`object.__new__(GatewayRunner)` (already the idiom elsewhere in the same file).
The stub reimplemented `_session_state` and `_peek_session_state`, so the tests
exercised copies that could drift from production; using the real class
immediately made one assertion stronger -- on a fresh runner `_sessions` does not
exist at all until something materialises it, so the reset provably did not even
create the map.
A review pass on this follow-up then caught that the CALL SITE was still
unbound: deleting the whole `if not _hyg_aborted: if
hygiene_compaction_recovered(...)` block left every ladder test green, because
the unit tests prove the predicate correct without proving it is wired in. The
merged commit had the same gap and its only cover was the banned source-reading
test. `test_session_hygiene_forces_in_place_compaction_with_bound_session_db`
now spies the reset on a genuine in-place compaction, so deleting the wiring
fails. Two earlier attempts at this test did NOT close the gap -- asserting on
streak VALUES passes either way, since the streak is 0 whether or not the gate
ran; only a positive spy assertion on a recovering run detects the deletion.
Same pass also corrected an overstatement: point (2) is hardening, not a live
bug. The degenerate path also sets `_new_count = _msg_count` and `_new_tokens =
_approx_tokens`, and `compression_made_progress(n, n, t, t)` is always False, so
the merged code already declined to reset there. A 200k-trial fuzz over the
reachable state space found zero behavioural disagreements between the merged
gate and this one. The guard's value is surviving a future edit that stops
reusing those counts.
Tests: 28 in tests/gateway/test_hygiene_failure_cooldown_ladder.py (8 new unit
tests for the predicate, 3 for reason forwarding, 3 source-reading tests
deleted). All 5 mutations caught -- including one that restores the hand-rolled
comparison and one that removes the rotated/in_place guard. Two mutations
initially SURVIVED and exposed vacuous tests of my own: the no-rewrite test used
counts the progress predicate already rejects, so it passed without binding the
guard at all; it now passes counts that read as progress on their own, proving
the guard is what rejects them. gateway hygiene + session-state + agent
compression-progress suites: 54 passed; ruff clean.
Refs #79624
A gateway session whose summary model keeps timing out no longer retries
compaction on the same fixed interval forever.
The in-agent compressor already escalates repeat summary timeouts
60 -> 300 -> 900s (ContextCompressor.record_timeout_failure), but that ladder
reads the in-memory _consecutive_timeout_failures counter and
bind_session_state() zeroes it (context_compressor.py:1645). Session hygiene
constructs a FRESH AIAgent for every run (gateway/run.py:16820) and re-binds
state each time, so from the gateway that streak is structurally always 0 --
only the flat hygiene_failure_cooldown_seconds (300s) could ever be recorded.
Issue #79624 reported exactly that steady state: an oversized session
(1053 messages, ~119.5k tokens) whose aux model always timed out, re-attempting
compaction every 300s across five days until the reporter deleted the session
by hand.
Track the streak on PersistentState instead, which outlives the per-run agent
and is not cleared by turn/boundary resets, so consecutive hygiene failures
climb 300 -> 900 -> 2700s and then saturate. Both failure sites (progress
timeout and aborted compression) feed it; a real compression resets it, so a
session that recovers starts from the first rung again. The ladder multiplies
the configured base, so operators who tuned
hygiene_failure_cooldown_seconds keep their first rung. Per-session, so one
wedged chat cannot penalize other conversations.
Deliberately NOT changed, since each is a maintainer policy call rather than a
defect (all three are written up on #79624):
- no durable failure-streak column, so escalation still resets on restart
- the gateway 30s / in-agent 120s / aux-client 300s-floor timeout mismatch
- no `hermes doctor` check or `hermes sessions list` marker for a session
stuck in a compression-failure cooldown
Note the reported exit(1) is NOT a crash: it is the deliberate
_signal_initiated_shutdown path (gateway/run.py:26746-26751, #5646) that lets
systemd Restart=on-failure revive the gateway after a bare SIGTERM, and it
fires on every `systemctl restart` independently of compaction. The compaction
log lines appear after the shutdown line because the gateway-owned executor is
torn down with shutdown(wait=False, cancel_futures=True) (run.py:21164), so an
in-flight turn keeps logging during teardown. Full analysis on the issue.
Post-review hardening (Phase 2c + /simplify-code found five real defects in the
first cut):
- the recovery gate hand-rolled `_new_tokens < _approx_tokens` when a canonical
predicate already existed: `compression_made_progress` (agent/turn_context.py,
#39548). They disagree on 3 of 5 cases -- the hand-rolled form misses a
row-count win when the summary keeps the token estimate flat, misses one
where the summary is slightly MORE verbose (so a genuinely recovered session
would keep escalating forever), and counts a sub-5% wobble as recovery. Now
reuses the shared predicate, promoted from `_compression_made_progress` to a
public name with the old private name kept as a back-compat alias so the
existing importer (tests/agent/test_protected_tail_pressure_61932.py) and any
patcher of that symbol keep working.
- the reset was gated on "not aborted", but the degenerate "did not rotate or
compact in place" branch (#21301) is NOT aborted and yields zero reduction,
so a session wedged there reset its streak every run and could never
escalate -- silently defeating the fix. Now gated on real progress.
- no absolute ceiling: base * 9 reaches 9h at an operator base of 3600s,
indistinguishable from "compaction switched off". Added
_HYGIENE_COOLDOWN_MAX_SECONDS = 3600, mirroring the in-file
_RECONNECT_BACKOFF_CAP precedent.
- the reset used the get-or-create accessor to write a 0 that was already 0,
materialising a _sessions entry (never evicted). Now peeks.
- the abort verdict was probed twice, leaving the reset/record mutual
exclusion implicit; a future await between the probes would have broken it
silently. Computed once into _hyg_aborted.
Tests: 19 new in tests/gateway/test_hygiene_failure_cooldown_ladder.py --
ladder escalation, saturation, the absolute cap, per-session isolation,
reset-on-recovery, custom/zero base, PersistentState scoping (a mutation moving
the field to TurnState fails), degraded runners, the progress gate, the exact
progress-predicate semantics the gate depends on, and end-to-end that the
escalated value is what reaches the state DB. All 12 mutations caught, including
ones that restore the flat cooldown (the original bug), ungate the reset, swap
the canonical predicate back for the hand-rolled comparison, remove the cap, and
share the streak globally; the harness hard-errors when a mutation cannot be
applied, since a silently no-op mutation check is worse than none -- an earlier
version of it WAS silently no-opping after a refactor. The gate's contract test
slices by AST node span rather than a fixed character count, which had already
truncated once as the block grew. gateway hygiene + session-state + the three
touched agent compression suites: 50 passed; ruff clean.
E2E with real imports demonstrates the premise rather than asserting it:
bind_session_state zeroes the in-agent counter, and the recorded deadlines go
300 -> 900 -> 2700 -> 2700 -> 2700s where they were previously a flat 300s.
Reported by @yucezerey (#79624), whose state.db column dump and
"deleting the session fixed it" datapoint made the real mechanism findable.
GatewayRunner carried ~19 separate Dict[str, ...] attributes keyed by
session_key, each with an ad-hoc lifecycle. They now live in one
`self._sessions: dict[str, SessionState]` (gateway/session_state.py) with
three lifecycle scopes and a `_session_state(key)` get-or-create accessor.
Mechanical refactor: same state, same semantics, new container.
Migration table (dict -> old decl line -> current clear path -> new home):
| legacy dict | decl | cleared by (before) | SessionState field |
|------------------------------------------|-------|--------------------------------------------------|----------------------------------------|
| _running_agents | 3505 | _release_running_agent_state; stop() .clear() | turn.agent |
| _running_agents_ts | 3506 | same | turn.started_ts |
| _active_session_leases | 3507 | same (+ lease.release()) | turn.lease |
| _busy_ack_ts | 3539 | same | turn.busy_ack_ts |
| _turn_lease_tokens ((key, gen)-keyed) | 3518 | _release_turn_lease (generation-guarded) | turn.lease_token + turn.lease_generation |
| _session_model_overrides | 3585 | _CONVERSATION_SCOPED_STATE funnel | conversation.model_override |
| _pending_one_turn_model_restores | 3586 | funnel; one-shot pop in turn finally | conversation.one_turn_restore |
| _session_reasoning_overrides | 3589 | funnel; lazy-init dict swap ~5692 (RACE) | conversation.reasoning_override |
| _session_service_tier_overrides | 3592 | funnel; lazy-init dict swap ~5738 (RACE) | conversation.service_tier_override |
| _last_resolved_model ("*" = process-wide)| 3527 | funnel | conversation.last_resolved_model |
| _queued_events | 3537 | funnel; lazy-init dict swap ~5204 (RACE) | conversation.queued_events |
| _pending_turn_sidecar_notes | 3597 | funnel; lazy-init dict swap ~19832 (RACE) | conversation.sidecar_notes |
| _session_ephemeral_pin | 3601 | agent-cache evict pop; lazy swap ~19885 (RACE) | conversation.ephemeral_pin |
| _session_vc_last | 3604 | agent-cache evict pop; lazy swap ~19864 (RACE) | conversation.vc_last |
| _pending_approvals | 3611 | boundary security funnel; stop() .clear() | persistent.approvals |
| _update_prompt_pending | 3623 | security funnel; update watcher pops | persistent.update_prompt_pending |
| _pending_native_image_paths_by_session | 3538 | one-shot consume; lazy swap ~12944 (RACE) | persistent.native_image_paths |
| _pending_messages (runner-level, str) | 3519 | _interrupt_and_clear_session pop; stop() flush | persistent.pending_command_text |
| _session_run_generation | 3540 | NEVER (monotonic, #28686) | persistent.run_generation (never reset)|
Races eliminated: every `self._X = {}` lazy-init/reset replaced the WHOLE
dict, so a writer on session A racing a lazy init triggered by session B
could lose its entry. All six such sites (_session_reasoning_overrides
~5692, _session_service_tier_overrides ~5738, _queued_events ~5204,
_pending_turn_sidecar_notes ~19832, _session_ephemeral_pin ~19885,
_session_vc_last ~19864, _pending_native_image_paths_by_session ~12944,
_turn_lease_tokens ~13644) are now per-session field writes on an existing
SessionState; a reset can no longer cross sessions structurally.
Registry successors:
- _release_running_agent_state -> state.turn.clear() (one structured reset
instead of the drifting pop-list; still pops the slot lease and calls
lease.release() first; still generation-guarded).
- _CONVERSATION_SCOPED_STATE funnel -> state.conversation.clear(); the
tuple is retained for legacy plain-dict stores not yet folded in
(_pending_model_notes) and for the public test contract.
- _turn_lease_tokens' (key, generation) tuple key -> lease_token +
lease_generation fields; release/rebind only match when the generation is
current, preserving the #28686/#64934 ownership check.
- _session_run_generation stays monotonic on persistent.run_generation and
is never cleared (conversation boundaries and turn releases don't touch it).
Compatibility adapters: tests (and a few mixin call sites) access the old
dict names directly (137 direct assignments to _running_agents alone), so
each legacy name is kept as a thin @property returning a live MutableMapping
view over the corresponding SessionState field (legacy_dict_property /
legacy_lease_token_property in session_state.py). Setter accepts a plain
dict (the `runner._X = {...}` test pattern); views support ==, in, len,
.get/.pop/.clear. The shutdown path in _stop_impl deliberately keeps
duck-typed legacy-attribute access because test fakes borrow it with plain
dicts.
Name collision noted (NOT touched, out of scope): gateway/platforms/base.py
has its own _pending_messages Dict[str, MessageEvent] (adapter-level slot);
the runner-level Dict[str, str] of the same name is what moved to
persistent.pending_command_text.
Entry leaks preserved (follow-up, no new eviction in this PR): SessionState
entries in self._sessions are never evicted, matching the old dicts — e.g.
_last_resolved_model, _session_run_generation, _session_vc_last entries for
dead sessions leaked before and their fields still occupy a SessionState now.
Verification: 123 tests/gateway files referencing the old names +
_release_running_agent_state + _CONVERSATION_SCOPED_STATE all pass (sole
failure test_feishu.py::test_websocket_sdk_accepts_channel_ua_tag is
pre-existing, stash-verified); 8 non-gateway test files touching the names
pass (266 tests); `import gateway.run` subprocess smoke OK; ruff clean;
post-migration grep shows zero non-comment `self._<oldname>` references in
run.py outside the property adapters and the duck-typed shutdown block.
kshitij
teknium1