Skip to content

fix(gateway): retarget non-compression completions to active session (#96850) - #107812

Closed
salch-cred wants to merge 1 commit into
NousResearch:mainfrom
salch-cred:fix/96850-background-task-session-routing
Closed

salch-cred wants to merge 1 commit into
NousResearch:mainfrom
salch-cred:fix/96850-background-task-session-routing

Conversation

@salch-cred

Copy link
Copy Markdown

Fixes #96850

Summary of Changes

  • Updated _resolve_async_delegation_session\ in \gateway/run_notifications.py\ so that non-compression background completions deliver directly to \session_entry\ (the chat route's current active session) rather than calling \switch_session\ back to a stale spawning session ID.
  • Added test coverage in \ ests/gateway/test_async_delegation_session_binding.py\ verifying retargeting to the chat's current active session without forcing a route switch to a stale session ID.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Sep 11, 2026

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I walked this from the exact current head through the synthetic-completion caller, the session-store contract, the regression file, #96850, and the earlier ownership fixes. There is one blocking ownership regression in the current shape.

P1 — the completion's dispatch-time owner is being replaced by the chat route's current session.

gateway_session_id is not an incidental/stale coordinate here. It is the durable parent session_id captured when the async delegation was dispatched. _hmwa_resolve_session() intentionally resolves the chat's current session_entry first and then calls _resolve_async_delegation_session(session_entry, pinned_session_id) precisely so the completion can be checked against that captured return address. On the live/non-compression mismatch path, this PR now discards the captured owner and returns the already-resolved current route.

That is the same defect class #57498 described: a late background delegation result follows whichever session is currently active for the routing peer instead of the session that spawned it. The merged repair #60871 made parent_session_id/gateway_session_id survive dispatch → completion → gateway routing specifically to stop that cross-session contamination, preserving the original #57535 work by nankingjing. This hunk reverses that invariant for the exact case where both sessions are still live.

There is a particularly useful self-check in the touched test file: its module contract still says (1) completions are pinned to the spawning session and (2) an ended spawning session is never rerouted to the peer's current session. The changed test now asserts the opposite for a live spawning session. The test is therefore proving the new implementation, but not the established ownership contract.

#96850 also does not provide evidence for this inversion. Its requested behavior is that long-running output return to the exact originating chat session. The existing issue analysis separates the plain terminal-output/UI-owner rail (#73351, origin_ui_session_id) from the still-unreproduced symptom where a new conversation is created and the original appears frozen. Retargeting an async-delegation completion to whatever session is current neither reproduces that residual nor preserves the requested originating-session identity.

The compression exception is different and should stay different: compression is a proved continuation lineage, so following an ended parent to its verified live compression tip is legitimate. An arbitrary other live session under the same routing key has no such lineage proof. In the authority model, the current route is a candidate coordinate, not authority to receive a completion that carries a more-qualified dispatch-time owner.

Required closure: keep the captured live parent session as the completion owner. If the real problem is that switch_session() mutates the peer's foreground route/focus while delivering a late result, solve that effect boundary separately: target the captured durable session without granting the current route ownership, or fail/defer closed if no safe delivery primitive exists. Do not move the completion payload into an unrelated live session as the workaround.

Please add a real two-live-session witness: dispatch in A; move the routing peer to distinct live B without ending A; let the delegation complete; prove B receives zero completion content and A remains the owner (or the completion is explicitly deferred/dropped by a documented policy). Keep /new as the ended-parent negative control and compression rotation as the verified-continuation positive control. For Fixes #96850, also reproduce the actual reported long-running terminal/webhook sequence so the fix is tied to the rail that creates the contextless-session/freeze symptom rather than assuming this async-delegation resolver is it.

Interlocks / merge order: #73351 (ayushnangia) is complementary spawn-time UI/live terminal-output ownership and should not be collapsed into this durable async-delegation rail. #106227 is also directly coupled: its open Telegram topic-binding repair currently treats the non-compression switch_session() here as a real owner move and rebinds (chat_id, thread_id) to that exact owning session; removing the owner move invalidates one of its four tested switch surfaces. The two changes need one coherent ownership contract, not opposite assumptions. #107247 is complementary source-profile lookup authority around completion preflight, not a substitute for session ownership. Closed-unmerged #64530 is useful historical compression-lineage design evidence, but not a live carrier and should not be credited as landed work.

At review time this carrier is 1 commit ahead / 4 behind live main@45a6101f36576367359c171cd5820ee76a3d047b. Exact-head CI 34550156072, Docker 34550155289, and Nix 34550155311 all settled action_required with zero jobs executed; the head has zero check-runs and zero commit statuses. So the surviving carrier is 0/1 commits proven hosted-green. That is separate from the code blocker, but it also means this object is not yet release-proven.

The instinct to stop a late completion from yanking foreground routing around is reasonable, and keeping the compression path narrow is good. The fix needs to preserve the stronger thing the existing system already knows: who commissioned the work. Once that identity survives the final delivery boundary, the session-focus problem can be solved without reopening the original cross-session leak.

pinned_session_id, session_entry.session_id,
)
return switched
return session_entry

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — this drops the dispatch-time owner and substitutes the ambient/current route. pinned_session_id is the durable parent session captured when the delegation was spawned; session_entry is whichever session the routing peer resolves to now. Returning the latter on a live mismatch recreates #57498/#60871's cross-session contamination: dispatch in live A, move the peer to distinct live B, completion arrives → B receives A's result. Preserve A as the qualified owner (or fail/defer closed); if switch_session() itself is the unwanted foreground-route mutation, introduce a delivery path that can target A without transferring B's route rather than changing ownership. The modified regression should prove B receives zero completion content.

@holny holny left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flips the non-compression case from rebinding the routing key to the owning session to delivering into the chat's current session_entry — a real semantic change against the contract documented at run_notifications.py:182-186 ("never let a late completion override an unrelated /new or restored route"), and the renamed test asserts the opposite of the old one. Could you say how the two reconcile? If #96850 intends a late non-compression completion to land in the user's live session, the docstring guard should be updated in the same PR — and I'd want to confirm the ownership guards still fail closed when the pinned session is unrelated to the current route (else a stale delegation result gets injected after /new).

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks @salch-cred. This was resolved by #114765, which covers the same fix; #114765 — fix(gateway): async completion no longer re-pins a route after /stop or /new won the race (#113690, salvage #113692, #113716) — is now merged on main at 3e408dcc2a74, closing issues #113690. Closing this PR in favour of the landed change; if you see a case it does not cover, please open a fresh issue with the repro and tag it.

@teknium1 teknium1 closed this Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Background task output routes to a newly spawned contextless session and freezes the originating session

5 participants