fix(gateway): deliver kanban/delegate wake-ups to api_server sessions via self-post - #64998
fix(gateway): deliver kanban/delegate wake-ups to api_server sessions via self-post#64998ianks wants to merge 1 commit into
Conversation
a7af209 to
6e9c04b
Compare
Wake-ups for kanban notifications and background delegation completions were injected via handle_message() using a build_session_key()-derived key, which can never match the raw X-Hermes-Session-Id key that api_server sessions run under — so the wake landed in a session nobody was reading. On top of that, ApiServerAdapter.send() reports failure without raising, and that was treated as a successful delivery, so the notify cursor advanced past events that were permanently lost; and background delegation was forced synchronous on api_server since there was no way to wake the session afterward. Fix: route wake-ups for non-push adapters through a self-post to /v1/chat/completions with the original session id, treat non-raising send failures as failures (rewind instead of advancing the cursor), and re-enable background delegation whenever a session id is available to wake. The origin session id is captured from the request-scoped api_server chat_id binding rather than HERMES_SESSION_ID: constructing a child agent calls set_current_session_id() with the subagent's internal id, clobbering that variable right before dispatch would read it and misrouting the wake into the subagent's own session. Related: NousResearch#56580, NousResearch#64609, NousResearch#53027, NousResearch#63169, NousResearch#56531, NousResearch#50319, NousResearch#64113
6e9c04b to
8544865
Compare
|
Amended: live e2e testing found that the dispatch-time origin capture (HERMES_SESSION_ID) was clobberable — constructing a child agent calls set_current_session_id() with the subagent's internal id right before the dispatch code read it, so the completion wake self-posted into the subagent's own session. The origin is now captured from the request-scoped api_server chat_id binding (which only set_session_vars writes, so it survives child construction), centralized in a helper used by both the delegation dispatch and the kanban task session stamp, and covered by tests that reproduce the clobber. |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved with Note
Looks Good
- Fix(gateway): deliver kanban/delegate wake-ups to api_server sessions via self-post
- 979 additions, 7 deletions — substantial fix
- No security issues detected
Note
- Some debug/print patterns in diff checked — appear in existing code context, not new artifacts
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the raw-session routing problem; current main does force API-server background delegation synchronous (tools/delegate_tool.py:2795-2825), so the premise is real.
Problems
gateway/kanban_watchers.py:513-608advances the cursor beforedeliver_wake()at line 588. A failed/exhausted self-post is caught after the advance, permanently losing the API-server event rather than retrying it./v1/runscreatessession_idatgateway/platforms/api_server.py:4419, but binds onlysession_keyat line 4539. The newtools/async_delegation.py:_current_origin_session_id()readschat_id, so this route still falls back to synchronous execution.- The new
origin_session_idis in-memory only (tools/async_delegation.py:526,728). Durable dispatch persistence and abandoned-delegation recovery omit it (tools/async_delegation.py:134-150,229-260), leaving recovered API completions unroutable.
Suggested changes
- Make the kanban self-post part of the retryable delivery before cursor advancement.
- Bind the
/v1/runsraw session ID and add route coverage. - Persist and restore
origin_session_idwith durable delegation records.
This is an automated hermes-sweeper review.
| # that never matches the raw | ||
| # X-Hermes-Session-Id session real turns | ||
| # run under (wrong-session wake bug). | ||
| await deliver_wake( |
There was a problem hiding this comment.
This wake runs only after the cursor has advanced at lines 513-519, and the exception is swallowed at lines 598-608. A 429/connection failure therefore consumes the API-server notification permanently. Please make wake acceptance retryable before advancing the cursor, or rewind when it fails.
| "model": model, | ||
| "session_key": session_key, | ||
| "origin_ui_session_id": origin_ui_session_id, | ||
| "origin_session_id": origin_session_id, |
There was a problem hiding this comment.
This field is not included in _persist_dispatch()'s durable task_payload, and recover_abandoned_delegations() does not reconstruct it. A restarted gateway will recover the delegation without its raw API-session return address and cannot self-post the completion.
…post On non-push adapters (api_server) the wake self-post IS the delivery, but the cursor advanced before the self-post ran and a failed/exhausted post was swallowed by the best-effort except — permanently losing the event. Reorder the else-branch: for non-push adapters run the self-post FIRST and only advance the cursor once it succeeds. A failure rewinds the pre-send claim (same guarantee as the existing SendResult(success=False) path) so the next tick retries, with the same MAX_SEND_FAILURES drop threshold. Push-capable adapters keep the pre-existing advance-then-best-effort-wake behavior. Follow-up to #64998 (sweeper review F1).
/v1/runs bound only session_key at its _bind_api_server_session call, so tools.async_delegation._current_origin_session_id() — which reads the request-scoped HERMES_SESSION_CHAT_ID — returned "" on that route and runs-originated background delegations stayed forced-sync with no wake target. Bind chat_id/session_id the same way the other agent-entry routes do via _run_agent(). Follow-up to #64998 (sweeper review F2).
origin_session_id (the api_server wake self-post target) lived only in the in-memory record: durable dispatch persistence and abandoned- delegation recovery omitted it, leaving completions recovered after a process restart unroutable to api_server sessions. Persist it in the async_delegations table (CREATE TABLE + ALTER TABLE migration for legacy DBs), restore it on recovery, and expose it via get_durable_delegation. Also adds the contributors mapping for ianks (PR #64998 author). Follow-up to #64998 (sweeper review F3).
|
Merged via PR #70171 — your commit was cherry-picked onto current main with your authorship preserved in git log (f50c3d9). The self-post mechanism and your test suite landed intact; we layered the three fixes from the July 16 review on top as separate commits (cursor advance only after a successful wake post, chat_id binding on /v1/runs, durable origin_session_id persistence). Excellent work on this one — it closes a whole feature class that was silently broken for API-server users. Thanks! |
…post On non-push adapters (api_server) the wake self-post IS the delivery, but the cursor advanced before the self-post ran and a failed/exhausted post was swallowed by the best-effort except — permanently losing the event. Reorder the else-branch: for non-push adapters run the self-post FIRST and only advance the cursor once it succeeds. A failure rewinds the pre-send claim (same guarantee as the existing SendResult(success=False) path) so the next tick retries, with the same MAX_SEND_FAILURES drop threshold. Push-capable adapters keep the pre-existing advance-then-best-effort-wake behavior. Follow-up to NousResearch#64998 (sweeper review F1).
/v1/runs bound only session_key at its _bind_api_server_session call, so tools.async_delegation._current_origin_session_id() — which reads the request-scoped HERMES_SESSION_CHAT_ID — returned "" on that route and runs-originated background delegations stayed forced-sync with no wake target. Bind chat_id/session_id the same way the other agent-entry routes do via _run_agent(). Follow-up to NousResearch#64998 (sweeper review F2).
origin_session_id (the api_server wake self-post target) lived only in the in-memory record: durable dispatch persistence and abandoned- delegation recovery omitted it, leaving completions recovered after a process restart unroutable to api_server sessions. Persist it in the async_delegations table (CREATE TABLE + ALTER TABLE migration for legacy DBs), restore it on recovery, and expose it via get_durable_delegation. Also adds the contributors mapping for ianks (PR NousResearch#64998 author). Follow-up to NousResearch#64998 (sweeper review F3).
What does this PR do?
Sessions originated via the stateless
api_serverplatform never receiveddelegate_task(background=true)completions or kanban notification wake-ups. Three root causes:gateway/kanban_watchers.pyandgateway/run.py::_inject_watch_notificationwoke sessions viahandle_message()with abuild_session_key()-derived key, which never matches the rawX-Hermes-Session-Idkey api_server sessions actually run under.ApiServerAdapter.send()returnsSendResult(success=False)without raising; this was treated as delivered, so the notify cursor advanced past permanently-lost events.tools/delegate_tool.pyforced background delegations to run synchronously on api_server, since there was no way to wake the session afterward.The fix (new
gateway/wake.py) routes wake-ups for non-push adapters through a self-post to/v1/chat/completionswith the original session id — the same entry point real turns use to resume a session. Non-raising send failures now rewind the cursor instead of advancing it, and background delegation dispatches async whenever a session id is available to wake, falling back to sync only for session-id-less one-shots.Related Issue
Root cause for #56580, #64609, #53027, #63169; complements #56531, #50319, and #64113 (which fixes the push-adapter creator-wake path — no conflict).
Type of Change
How to Test
Targeted suites (
tests/gateway/test_wake_delivery.py,test_kanban_notifier_apiserver_wake.py,test_kanban_notifier.py,test_background_process_notifications.py,tests/tools/test_delegate_apiserver_background.py,test_delegate.py) pass — 209 tests. Fulltests/gateway tests/toolsregression: 17011 passed; the 84 failures are pre-existing/environmental (pass in isolation, none touch files changed here).Checklist
Out of Scope
The
chat_type="group"hardcoding in the push-adapter creator-wake path (#56580) is left to #64113. A downstream deployment-specific text-notify fallback was deliberately not generalized here.