fix(gateway): keep /heartbeat alive across a compression session rotation - #80225
fix(gateway): keep /heartbeat alive across a compression session rotation#80225Drexuxux wants to merge 1 commit into
Conversation
…tion The heartbeat watch stores the session id that was current when /heartbeat ran. Context compression rotates the session: migrate_heartbeat_to_session copies the state onto the continuation session and marks the parent row cleared, and load_heartbeat returns None for a cleared row. So the first time a session compresses, the poller's has_heartbeat() check on the captured id reads False, which it took to mean the user cleared the heartbeat — it dropped the watch and the heartbeat never fired again. No error surfaced, and the sessions this happens to are the long-running ones a heartbeat exists for. The CLI driver is unaffected: it rebinds its manager whenever session_id changes. Follow the compression chain before concluding the heartbeat is gone, via the same get_compression_tip helper the delegation and resume paths use, and rebind the watch to the tip. The lookup only runs when the stored id comes back empty, so the steady-state poll is unchanged, and it degrades to the input id when there is no rotation, no session DB, or the query fails.
Related: #80208 is an earlier open fix for the same compression-rotation heartbeat failure, using the live SessionStore route mapping. This PR follows the SessionDB compression tip; maintainers should choose the source-of-truth approach. |
|
Adjudication note for the duplicate pair on this member — #80225 and #80208 both fix "heartbeat dies on compression rotation," with different but both real resolvers:
For a poller that can be arbitrarily stale relative to rotations, the chain-walk-plus-verify shape here is the more defensive of the two; #80208's route lookup is the cheaper first hop. If maintainers want both properties, the compose is three lines inside this PR's Context: this is the heartbeat member of the rotation-seam map on #80337; the ACP member is #80423, and the orphan-recovery member just merged as #80487. |
What
/heartbeatregisters a gateway watch keyed by the session id that was current when the command ran:Context compression then rotates the session. On rotation
agent/conversation_compression.pycallsmigrate_heartbeat_to_session(old_session_id, agent.session_id), which copies the state onto the continuation session and marks the parent rowstatus="cleared"— andload_heartbeatreturnsNonefor a cleared row.The poller still holds the id captured at
/heartbeattime, so its liveness check reads False and it concludes the user cleared the heartbeat:The watch is dropped and the heartbeat never fires again. Nothing is logged above DEBUG and no message reaches the user — the recurring instruction they set just stops, in exactly the long-running sessions a heartbeat exists for. Driving the real functions:
The CLI driver does not have this problem:
_get_heartbeat_managerrebinds its manager wheneversession_idchanges, so it picks up the continuation session on the next poll. Only the gateway caches the id.The fix
Follow the compression chain before concluding the heartbeat is gone, then rebind the watch to the tip.
get_compression_tipis the helper the delegation-binding and resume paths already use for this, and it returns the input unchanged when the session isn't a compression parent:The lookup runs only when the stored id comes back empty — a heartbeat that is simply not due never reaches it, so the steady-state poll is unchanged.
_heartbeat_session_tipdegrades to the input id when there is no rotation, no session DB is attached, or the query raises, so a DB hiccup can't turn into an exception inside the poll loop.A genuinely cleared heartbeat still unregisters: the tip is authoritative, and if the heartbeat is cleared there the watch is dropped as before.
Tests and results
New file
tests/gateway/test_heartbeat_poller_rotation.py, driving the real poll loop with a stub runner supplying its collaborators:/heartbeat clearwith no rotation — still unregistersOn
mainthe first test fails on its behavioural assertion (live heartbeat was unregistered after rotation) and the helper test fails because the method is new; the two boundary tests pass there, which is what a non-regression pin should do. All four pass with the fix.tests/gateway/plustests/hermes_cli/test_heartbeat.pyrun to 4798 passed. I ran that suite before and after the change and diffed the failure lists: 72 failures, byte-identical both ways, all pre-existing onmainand unrelated (update command/streaming, andtest_teams.py, which fails at collection onmainand is excluded from the run for that reason).