fix(tui): route async delegation results across the compression chain - #57586
fix(tui): route async delegation results across the compression chain#57586Bartok9 wants to merge 2 commits into
Conversation
Closes NousResearch#57576 All desktop TUI sessions share one process-wide completion_queue. Each session poller uses _notification_event_belongs_elsewhere() to skip events it does not own, comparing evt["session_key"] to session["session_key"]. Context compression rotates session["session_key"] to a new continuation id (_reanchor_session_key_after_compression). A background delegation dispatched BEFORE compression recorded the old key, so after the launching session compresses it no longer recognizes its own completion event. The event looks orphaned and whichever poller dequeues first grabs it — delivering the result to the wrong conversation (and the dispatcher never gets its result). Fix: preserve rotated-out keys in a per-session session_key_chain and match ownership against the chain (_session_owns_key) instead of only the current key. The dispatching session reclaims its event; unrelated sessions still defer to the true owner. Tests: routing now follows the compression chain; chain recording dedupes and preserves order. Verified both fail without the fix.
|
Thanks for the fix — the I tested the PR locally with the targeted regression tests: uv run --extra dev pytest tests/test_tui_gateway_server.py -k 'notification_event_routing or record_session_key_history' -q -o 'addopts='
# 3 passed, 302 deselectedI also simulated the incident shape we observed:
With this PR, the launcher handles the event and the unrelated session defers, so this would likely prevent the observed cross-session delivery while the launching session is still live. One remaining edge case seems worth covering before closing the whole bug class: if an Suggested additional behavior/test:
So I think this PR likely fixes the reported #57576 manifestation, but adding the explicit-owner/no-live-owner regression would make the routing invariant much stronger: a background delegation result with an explicit owner must never be delivered to a session that does not own that key. |
…ring Per review on NousResearch#57586: an async_delegation completion carries an explicit owner session_key. If no live session owns that key (directly or via its session_key_chain), the previous fallback let the current poller convert it into a synthetic user prompt in an arbitrary unrelated conversation. Add _async_delegation_event_is_orphaned() and guard both the main poller loop and the shutdown drain: explicit-owner + no-live-owner delegations are dropped (logged) rather than injected elsewhere. Genuine global/system events (empty session_key) and non-delegation events are unaffected. Strengthens the routing invariant: a background delegation result with an explicit owner must never be delivered to a session that does not own that key.
|
Great catch — that's exactly the remaining hole. Pushed Added
Deliberately scoped narrowly so we don't regress the good cases:
New regression: uv run --extra dev pytest tests/test_tui_gateway_server.py -k 'notification_event_routing or record_session_key_history or async_delegation_orphan' -q -o 'addopts='
# 4 passed, 302 deselected |
|
Closing — superseded by PR #60863 (salvage of #59767 + hardening), which includes the compression-chain resolution your PR pioneered: the ownership check now resolves an event's session_key through resolve_resume_session_id to the live continuation before treating it as orphaned, plus an origin-session return address stamped at dispatch and fail-closed handling for unowned payloads. Your diagnosis of the rotated-key orphan mechanism was correct and is credited in the salvage lineage. Thanks! |
|
Thanks @teknium1 — glad the rotated-key orphan diagnosis and the compression-chain resolution carried forward into #60863. The origin-session return address stamped at dispatch plus fail-closed handling for unowned payloads is a stronger invariant than my drop-on-no-live-owner approach, since it can still route a late continuation correctly instead of discarding it. Happy to see it land in the salvage. 👍 |
Summary
Motivation
Closes #57576.
All desktop TUI sessions share one process-wide
completion_queue. Each session's poller calls_notification_event_belongs_elsewhere()to skip events it doesn't own, comparingevt["session_key"]againstsession["session_key"].Context compression rotates
session["session_key"]to a new continuation id (_reanchor_session_key_after_compression). A background delegation dispatched before compression recorded the old key. Once the launching session compresses, it no longer recognizes its own completion event — the event looks orphaned, and whichever poller dequeues first grabs it. The result is delivered to an unrelated conversation, and the dispatching session never receives its result.Fix
session_key_chainwhen compression re-anchors the key (_record_session_key_history)._session_owns_key()helper, used in both branches of_notification_event_belongs_elsewhere()(current-session check + live-owner scan).This is Option A from the issue (track the compression chain). The dispatching session reclaims its event; unrelated live sessions still defer to the true owner.
Verification
python3 -m pytest tests/test_tui_gateway_server.py -k "notification_event_routing or record_session_key_history"— 3 passedtest_notification_event_routing_follows_compression_chain— a completion dispatched under a pre-compression key routes to the (now-rotated) launching session, and an unrelated session defers.test_record_session_key_history_dedupes_and_preserves_ordermainwithout the fix.test_browser_manage_connect_default_local_reports_launch_hint,test_persist_model_switch_*) are unrelated env issues (No module named 'ruamel') and fail identically onmain.