Skip to content

fix(desktop): salvage inflight-journal cluster — skip stale duplicate folds + bound journal persistence - #86590

Merged
teknium1 merged 4 commits into
mainfrom
salvage/desktop-inflight-journal
Aug 15, 2026
Merged

fix(desktop): salvage inflight-journal cluster — skip stale duplicate folds + bound journal persistence#86590
teknium1 merged 4 commits into
mainfrom
salvage/desktop-inflight-journal

Conversation

@teknium1

Copy link
Copy Markdown
Contributor

Salvages the Desktop inflight-journal duplicate-answer cluster — PR #84021 by @Nicolas-Formenton and PR #82832 by @egilewski — onto one green branch against current main, with contributor authorship preserved via cherry-pick.

Fixes #85308
Fixes #63047

What this combines

1. Bound inflight journal persistence (#63047) — from #82832, author @egilewski

The desktop journal synchronously read, parsed, cloned, and rewrote one aggregate localStorage value while streamed turns were repainting. Large tool results and multi-session state could block the renderer and leave the app completely unresponsive (~5 messages on the reporter's machine, Settings included).

  • Store bounded recovery projections under per-session keys; migrate legacy v1 data once.
  • Isolate quota and storage failures; keep the newest recoverable tail without letting oversized writes replace valid state.
  • Keep bulk tool payloads and diagnostics out of the hot path.
  • Adds a real Electron/CDP macOS-arm64 A/B repro harness (repro:short-session-hang) with native visibility control, renderer heartbeat, and Settings/composer/transcript checks, plus focused regressions.
  • Includes the follow-up test fix (spy on the active localStorage implementation, not Storage.prototype) so the regression is observable under Node 26's jsdom fallback.

2. Skip stale journal duplicate folds (#85308) — from #84021, author @Nicolas-Formenton

The inflight-turn journal can outlive the turn it recorded (reclaim, reconnect, or restart races skip the settle that clears it). On session resume the fold then re-appended journaled assistant rows to a transcript that already held the committed replies — duplicate answers in scrambled order — and carried the stale entry's streamId onto the resumed state, which kept the journal alive (persistInFlightTurnState only clears when streamId is null) and re-folded the same tail on every open.

  • journalTailAlreadyCommitted(): when no base user row matches the tail, drop the fold if every recoverable assistant row's text is already committed in the transcript (error-only rows keep the conservative append path).
  • Idle-resume rule: only a genuinely running turn (keepPending: true) keeps a live stream target; an idle resume clears streamId so the journal self-heals instead of re-folding forever.

3. Sibling-site widening (maintainer follow-up)

The assistant-tail append exit path (user row persisted, no projection row) still carried the journal's streamId on a not-running resume — the same self-perpetuating-fold bug class. Applied the same keepPending gate there and pinned it with a regression assertion.

Verification

  • npx vitest run src/lib/inflight-turn-journal.test.ts — 38/38 pass (includes both PRs' regression suites merged).
  • node --test scripts/run-short-session-hang-repro.test.mjs — 7/7 pass.
  • npx tsc --noEmit on apps/desktop — clean.
  • npx eslint on all touched JS/TS files — clean.
  • Both PR branches were based ~1.1k commits behind; cherry-picks applied onto current origin/main with one clean auto-merge and zero manual conflicts (the journal file has not moved; the use-session-state-cache.ts / use-session-actions call sites are unchanged).

Credit

Infographic

desktop-inflight-journal

@teknium1
teknium1 requested a review from a team August 15, 2026 01:51
@github-actions

github-actions Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

૮ >ﻌ< ა ci review

ran on bfd423d — fix(desktop): clear the stale stream target on the assistant

⚠️ Warnings

CI timings · View report · View job

Wall time 7m43s vs 3m24s (+127.0%). 15 job(s) slower, 19 faster, 3 unchanged.

  • Python tests / Run tests slice 11/12: -46.0s
  • Python tests / Run tests slice 2/12: -38.0s
  • Python tests / Run tests slice 3/12: +35.0s
  • JS & TS checks / apps/desktop / check:test:desktop:all: -32.0s
  • Python tests / Run tests slice 8/12: -23.0s

OSV vulnerability scan · View job

5 known vulnerabilities found in pinned dependencies.

How to fix:

Review the findings in the Security tab. Update the affected dependencies if a patched version is available.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 15, 2026
egilewski and others added 4 commits August 14, 2026 20:26
The desktop journal synchronously read, parsed, cloned, and rewrote one
aggregate localStorage value while streamed turns were repainting. Large
tool results and multi-session state could therefore block the renderer and
leave the app unresponsive, while the existing macOS diagnostic path lacked
a real native hide/restore regression check.

Store bounded recovery projections under per-session keys, migrate legacy v1
data once, isolate quota and storage failures, and preserve the newest
recoverable tail without allowing oversized writes to replace valid state.
Add a real Electron/CDP macOS-arm64 A/B harness with native visibility control,
renderer heartbeat and Settings/composer/transcript checks, plus focused
regressions. Keep bulk tool payloads, diagnostics, and the existing recovery
merge behavior out of the hot path.

Fixes #63047
The inflight journal regression test always spied on Storage.prototype,
but Node 26's jsdom setup can provide a plain in-memory localStorage fallback.
That left the test unable to observe the setItem call in CI even though the
per-session journal write was correct.

Select the native window.Storage prototype when available and otherwise spy
on the active localStorage object, preserving the assertion across both
storage implementations.

Refs #82832
The inflight-turn journal can outlive the turn it recorded (reclaim,
reconnect or restart races skip the settle that clears it). On session
resume the fold then re-appends journaled assistant rows to a transcript
that already holds the committed replies, so the conversation ends with
duplicate answers in scrambled order. The fold also carried the stale
entry's streamId onto the resumed state on an idle resume, which kept the
journal entry alive (persistInFlightTurnState only clears when streamId is
null) and re-folded the same tail on every open.

Detect text-level staleness before the append path: when every recoverable
journaled assistant row already exists as committed text in the base
transcript, treat the entry as caught up and clear it. Only keep a stream
target when the resumed session is genuinely running (keepPending), so an
idle resume self-heals instead of re-folding.
…end path too

Sibling site of the idle-resume rule from the stale-fold fix: the
assistant-tail append exit (user row persisted, no projection row) still
carried the journal's streamId onto a not-running resume, which kept the
journal entry alive (persistInFlightTurnState only clears when streamId is
null) and re-folded the same tail on every open. Apply the same
keepPending gate and pin it with a regression assertion.

Refs #85308
@teknium1
teknium1 force-pushed the salvage/desktop-inflight-journal branch from db1e360 to bfd423d Compare August 15, 2026 03:30
@teknium1
teknium1 enabled auto-merge (rebase) August 15, 2026 03:40
@teknium1
teknium1 merged commit 77248f8 into main Aug 15, 2026
55 checks passed
@teknium1
teknium1 deleted the salvage/desktop-inflight-journal branch August 15, 2026 04:10
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/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

3 participants