fix(desktop): track one unscoped stream pin per session - #77826
Open
DojoGenesis wants to merge 1 commit into
Open
fix(desktop): track one unscoped stream pin per session#77826DojoGenesis wants to merge 1 commit into
DojoGenesis wants to merge 1 commit into
Conversation
`unscopedStreamSessionId` was a single shared slot. When a second chat started a turn while the first was still streaming, its `message.start` overwrote the pin, and every later unscoped event from the first stream resolved to the second chat — grafting one conversation's deltas, tool events and reasoning onto another's transcript (NousResearch#46194 / NousResearch#62823). Replace the slot with one pin per concurrent stream. `message.start` adds a pin instead of replacing it; a stream end retires only its own pin, so chats still streaming keep theirs. With several streams live an unscoped event has no field naming its owner, so it is attributed to the focused chat when that chat is itself mid-stream, and dropped otherwise. Dropping is the conservative half: the store keeps the correct rows, so the transcript recovers on refetch, whereas guessing is what painted A's output onto B. Single-stream and no-stream routing are unchanged — tightening the no-pin fallback for late events is NousResearch#70376's subject, not this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Open
1 task
SummarySixteen PRs address or reference this Desktop issue complex across pin hydration, transcript rendering, polling, cold-start restoration, window/profile isolation, queue affinity, submit coherence, and concurrent stream attribution. The diffs target distinct causes, with several closed or merged PRs retained as superseded designs or reference implementations rather than active candidates. Related pull requests
Duplicates#66270 and #67049 implement the same compact new-session remembered-state guards, with #67049 adding the requested hook-boundary regression and avoiding #66270's unrelated escaping change; #69815 overlaps that family but targets full peer windows. #70610 is superseded by #70986, #56444 by #66001, and #68181 competes with the safer #60607 design for the same #60541 fallback. Suggested consolidationKeep #77826 open with a salvage path centered on its per-session stream pins, conservative ambiguous-event drop, and concurrency regressions; also keep #58332 for its distinct polling optimization, #67049 for the hook-tested compact-window guards, and #69815 only for its distinct Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I59305(["issue #59305 (closed)"])
I62823(["issue #62823 (open)"])
P77826["PR #77826 (open)"]
P77826 -.->|partial| I59305
P77826 -.->|partial| I62823
class I59305 closed
class I62823 open
class P77826 open
class P77826 target
click I59305 "https://github.com/NousResearch/hermes-agent/issues/59305"
click I62823 "https://github.com/NousResearch/hermes-agent/issues/62823"
click P77826 "https://github.com/NousResearch/hermes-agent/pull/77826"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 16 pull requests and 21 issues in this complex. Each diff was read against this issue; Assessment working set: 207 kB of PR diffs, 89 kB of issue/PR text, 53 kB of discussion (62 comments), 66 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
alt-glitch
added
type/bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
unscopedStreamSessionIdis a single shared slot shared by every concurrent stream flowing throughhandleGatewayEvent. When a second chat starts a turn while the first is still streaming, itsmessage.startoverwrites the pin, and every later unscoped event from the first stream resolves to the second chat — grafting one conversation's deltas, tool events and reasoning onto another's transcript.I verified the race against unmodified
mainby running the sequence through the current resolver:The existing test
routes a new unscoped stream start to the currently active sessionencodes that overwrite as expected behaviour, which is why the race has stayed latent.This replaces the single slot with one pin per concurrent stream.
message.startadds a pin instead of replacing one; a stream end retires only its own pin, so chats still streaming keep theirs.When several streams are live, an unscoped event carries no field naming its owner. It is attributed to the focused chat when that chat is itself mid-stream, and dropped otherwise. Dropping is the deliberately conservative half: the store keeps the correct rows (a transcript recovers on refetch), whereas guessing is precisely what paints one chat's output onto another.
Credit: the diagnosis is entirely @johncrash64's — the root-cause analysis in #62823 identified the shared ref, the mechanism, and the per-session map as the fix. This PR implements that proposal with regression coverage.
Relationship to #70376
Deliberately orthogonal, not a competing duplicate. #70376 tightens the pin-cleared path (no pin → don't fall back to the focused chat). This PR fixes the pin-overwritten path, which #70376 does not reach: once B has clobbered the pin to
session-b, the value is truthy, no drop fires, and A's delta still lands on B.Single-stream and no-stream routing are intentionally left exactly as they are today, so the two changes compose rather than conflict.
Related Issue
Refs #46194
Refs #62823
Not marked
Fixes— both issues track several distinct symptoms. This closes the concrete renderer misattribution race, not the whole complex.Type of Change
Changes Made
apps/desktop/src/lib/gateway-events.ts—unscopedStreamSessionId: null | string→unscopedStreamSessionIds: readonly string[]on both the route input and result. AddedwithStreamPin/withoutStreamPinhelpers andresolveUnscopedStreamOwner, which returnsnullwhen ownership is genuinely ambiguous so the caller drops rather than guesses.apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts— the backing ref becomesuseRef<readonly string[]>([]). Single call site; no other consumer of the resolver exists.apps/desktop/src/lib/gateway-events.test.ts— six new cases, including the A→B overwrite regression, ambiguous-attribution both ways, and an explicit guard that single-stream / no-stream routing is unchanged.One existing assertion changed
routes a new unscoped stream start to the currently active sessionpreviously assertednextUnscopedStreamSessionId: 'session-b'— the clobber. It now asserts['session-a', 'session-b']. The test's intent (a new start routes to the active session) is preserved; only the pin bookkeeping changed. Flagging it explicitly since rewriting an existing assertion deserves scrutiny.How to Test
To see the bug on unmodified
main:git show main:apps/desktop/src/lib/gateway-events.tsinto a scratch module.message.start(A) →message.start(B) →message.deltawithexplicitSessionId: '', threadingnextUnscopedStreamSessionIdbetween calls.session-b. On this branch the equivalent sequence keeps both pins and routes A's events to A.In the app: run two chats with long prompts streaming concurrently, switch between them, and force an unscoped event (reconnect the gateway mid-stream, or trigger a global error). Before this change, deltas from one chat briefly paint onto the other's transcript.
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — N/A, and not run. This change is TypeScript-only underapps/desktop. I ran the desktop suite instead: 387 files / 3366 tests pass, plustsc --noEmit,eslint, andprettier --checkall clean.Documentation & Housekeeping
docs/, docstrings) — behaviour is documented in the JSDoc onGatewayEventSessionRouteInputandresolveUnscopedStreamOwnerScreenshots / Logs