Skip to content

fix(desktop): preserve active stream across hydration - #71877

Open
yinkev wants to merge 1 commit into
NousResearch:mainfrom
yinkev:fix/desktop-hydration-stream-race
Open

fix(desktop): preserve active stream across hydration#71877
yinkev wants to merge 1 commit into
NousResearch:mainfrom
yinkev:fix/desktop-hydration-stream-race

Conversation

@yinkev

@yinkev yinkev commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Prevents a lagging Desktop transcript hydration from splitting one live assistant turn into two bubbles.

The post-turn hydrator can finish after the user has already started the next turn. It replaced the renderer's message array with the older stored snapshot while leaving the active streamId unchanged. The next message.delta could no longer find that id and created a second assistant row, producing the intermittent byte-for-byte duplicate described in #38319 and #71857.

This PR closes the race at both seams:

  1. Stored-session hydration now reconciles through the existing live-tail contract, preserving the newest optimistic user row and pending assistant before merging renderer-only assistant errors.
  2. If another hydration/reconnect source still leaves an established streamId orphaned, the next delta rebinds the newest pending assistant to that id instead of appending a second bubble.

The fallback is deliberately narrow. It runs only when a prior stream id exists and a non-hidden pending assistant is present. A genuinely new stream still creates a new assistant message, and completed historical assistants are never reused by text equality.

Related Issue

Fixes #38319
Fixes #71857

Related but not duplicated by #68478: that broader session-reliability PR explicitly leaves the streaming-versus-hydration duplicate-response race out of scope.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • add reconcileStoredSessionMessages() as the shared stored-history reconciliation seam;
  • preserve the current live turn in both post-turn hydration and active messaging transcript refresh;
  • recover an orphaned streamId by rebinding the newest pending assistant;
  • add an exact stream regression for the hydrated-pending-row → next-delta sequence;
  • add a utility regression proving a lagging stored projection keeps the optimistic user and pending assistant tail.

How to Test

cd apps/desktop

../../node_modules/.bin/vitest run --project ui \
  src/app/session/hooks/use-message-stream/hydration-stream-race.test.tsx \
  src/app/session/hooks/use-session-actions/utils.test.ts

npm run typecheck

Then run ESLint on the five changed TypeScript files and git diff --check.

Expected focused result: 40 passed.

The stream regression was run against the pre-fix behavior first and failed with two assistant rows instead of one. The utility regression likewise failed because the shared reconciliation seam did not exist.

Checklist

Code

Documentation & Housekeeping

  • Relevant documentation — N/A; no user-facing configuration or workflow changed
  • cli-config.yaml.example — N/A; no config keys changed
  • CONTRIBUTING.md / AGENTS.md — N/A; no contributor workflow or architecture changed
  • Cross-platform impact considered; the state reconciliation is renderer-platform-neutral
  • Tool descriptions/schemas — N/A; no model-facing tool behavior changed

Verification

Focused Desktop tests: 40 passed
Desktop typecheck:      passed (renderer, Electron, E2E tsconfigs)
ESLint:                 passed
Diff hygiene:           passed

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) area/streaming Streaming responses: gateway delivery, provider wire sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 26, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused race fix. The premise remains present on current main: apps/desktop/src/app/contrib/wiring.tsx:321-326 replaces the stored transcript through preserveLocalAssistantErrors() only, while apps/desktop/src/app/session/hooks/use-message-stream/index.ts:101-116 appends a new assistant row whenever the retained streamId no longer exists in the hydrated array.

The proposed reconciliation composes the existing live-tail contract in apps/desktop/src/app/session/hooks/use-session-actions/utils.ts:342-437, and the orphaned-stream fallback is narrow to an established stream plus a visible pending assistant. The two changed hydration callers cover the post-turn and active-messaging refresh paths. The PR tests exercise both the stored-tail reconciliation and the subsequent delta/rebind sequence.

The targeted hunks remain applicable against current main despite unrelated Desktop movement since the PR base.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026

yinkev commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Author-status audit — corrected 2026-08-05: GitHub now reports this head as mergeable, current-head CI is green, and there are no unresolved inline review threads. The earlier conflict classification was a stale mergeability result, not a branch defect. Restoring the PR to ready for review. No branch rewrite is required.

@yinkev
yinkev marked this pull request as draft August 5, 2026 07:32
@yinkev
yinkev marked this pull request as ready for review August 5, 2026 11:25
@frizikk

frizikk commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Independent current-main validation on Linux (CachyOS), 2026-08-08:

  • merged exact PR head b63a1fcdd2d8fa249ba0dc2f2b022312a35821c3 without conflicts onto current main b9aa9289a8083f2e9d248ad6837b2938f5ee92d7 in an isolated worktree;
  • focused regressions: 84/84 passed (hydration-stream-race.test.tsx + use-session-actions/utils.test.ts);
  • Desktop TypeScript typecheck passed;
  • ESLint passed on all five changed files;
  • git diff --check passed.

The merged tree was discarded after verification and the worktree returned clean to b9aa9289. This confirms the targeted hydration fix remains applicable and green on today's upstream base. Maintainer review/merge would close the still-present transcript-clobber race.

— 🤖 Bot comment · LLM: gpt-5.6-sol · not written by a human

@alt-glitch alt-glitch added P2 Medium — degraded but workaround exists and removed sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages P3 Low — cosmetic, nice to have labels Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/streaming Streaming responses: gateway delivery, provider wire comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

Duplicate messages in desktop app — hydration race condition with active stream [Bug]: Desktop — identical assistant final answer delivered twice

4 participants