fix(desktop): sync the composer queue across windows (#46732) - #57516
fix(desktop): sync the composer queue across windows (#46732)#57516furancis wants to merge 6 commits into
Conversation
) Every desktop window boots $queuedPromptsBySession from the same localStorage key, then never syncs again: no 'storage' listener, and every save() writes the window's entire private snapshot back. With several windows open this diverges immediately — - window B's save clobbers entries window A enqueued after B booted (prompts silently vanish), - window A's stale snapshot resurrects entries B already drained, and its auto-drain then re-submits them — the queued prompt 'sends from a window the user never typed in', - runDrain picks from the rendered React slice, so an entry another window drained after our last render double-submits. Fix, three small pieces: 1. composer-queue.ts listens for 'storage' events on the queue key and re-loads the atom. The event only fires in non-writing windows, so there is no self-echo. 2. writeSession()/migrateQueuedPrompts() merge over load() — the live persisted map — instead of the in-memory atom, so a save can never revert a write that landed between sync events (the event is async; this closes the same-frame race). 3. runDrain() picks the entry from getQueuedPrompts() (live store) instead of the rendered slice, so a cross-window removal observed at send time stops a double-submit. No behavior change for single-window use: load()/save() round-trip the same data, and the listener never fires without a second window. Tests: 6 new cases in composer-queue.test.ts covering adopt-on-event, no-clobber, the same-frame race, cross-window drain removal, full storage clear, and unrelated-key isolation.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the stale whole-map persistence path; current main still has that defect in apps/desktop/src/store/composer-queue.ts:47-60.
Problems
apps/desktop/src/store/composer-queue.ts:67reloads storage, but callers already computedqueuefrom their localqueueFor(sid). Two windows appending or editing the same session still replace that session array with competing stale versions. The new tests atcomposer-queue.test.ts:173-195only cover different session ids.apps/desktop/src/app/chat/composer/hooks/use-composer-queue.ts:195reads live state, but both windows can select the same entry and callonSubmitbefore either reaches removal at line 213. The lock is a renderer-local ref, while every idle renderer schedules auto-drain (use-composer-queue.ts:260-316), so this does not prevent a concurrent double submit.
Suggested changes
- Add a cross-window-safe same-session mutation/claim protocol and test two independent renderer contexts for concurrent same-session writes and drain attempts.
Automated hermes-sweeper review.
| // Merge over the freshly-persisted map, not the in-memory atom: another | ||
| // window may have written between our last sync event and now, and basing | ||
| // the save on a stale snapshot would silently revert its change. | ||
| const next = { ...load() } |
There was a problem hiding this comment.
queue was computed from queueFor(sid) before this reload. Two windows concurrently appending or editing this same sid still each replace next[sid] with a stale array, so the final write loses the other mutation. Please derive or claim the per-session mutation from authoritative current state and cover same-session concurrency.
| // open, another window may have drained (removed) this entry after our | ||
| // last render, and sending from the stale slice would double-submit the | ||
| // prompt into the session (#46732). | ||
| const entry = pickEntry(getQueuedPrompts(activeQueueSessionKey)) |
There was a problem hiding this comment.
This is not a cross-window drain claim: two renderers can both read this entry and invoke onSubmit before either reaches removal below. drainingQueueRef is local to one hook instance. Please add shared ownership/claiming before submit and a two-renderer regression test asserting exactly one submission.
…drain Address review on NousResearch#57516: the cross-window sync in NousResearch#46732 still had two same-session concurrency holes. 1. Same-session lost updates: writeSession merged the freshly persisted map but callers handed it a queue array computed from the in-memory atom, so two windows appending/editing/removing in the SAME session replaced the array with competing stale versions. Every mutator is now operation-based (mutateSession): it reloads persisted storage and applies its operation (append, filter, reorder, edit) to the freshest queue, never to the caller's snapshot. migrateQueuedPrompts gets the same treatment for both its source and target keys. 2. Cross-window double drain: the drain lock was a renderer-local ref while every idle window schedules auto-drain, so two windows could pick the same head entry and each submit it before either removed it. Draining now runs inside an exclusive per-session Web Lock (withSessionDrainClaim) arbitrated by the browser process across all windows and auto-released on window crash/close. Losers use ifAvailable to skip without blocking, the winner picks its entry INSIDE the claim from persisted storage (readPersistedQueuedPrompts, bypassing the async storage-event echo), and auto-drain re-locates entries by id so a vanished entry is a no-op, not a resubmit. runDrain now reports an outcome so only genuinely rejected sends count toward the stuck-queue retry cap; losing a race no longer burns attempts or toasts spuriously. Regression tests cover concurrent same-session writes for every mutator, claim exclusivity/fallback semantics, and a two-window auto-drain race that must submit exactly once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XZRUAaXcjP6yq2YMRctTXU
…aces, and drain stranding Follow-up to the NousResearch#57516 review fixes, addressing everything a deep self-review confirmed: - In-memory fallback restored (regression): a failed save() flips the store into atom-based operation — mutations, drain picks, and clears keep working single-window when localStorage is unwritable (which cannot sync windows anyway), recovering automatically on the next successful save. - Removal tombstones: a sidecar key of recently-removed entry ids, written BEFORE the map write and filtered from every read/write. A stale concurrent save can no longer resurrect a drained (or user- deleted) entry into a drainable state — the one interleaving the operation-based mutations could not close, since localStorage has no CAS and sync mutators cannot take async locks. - Migration waits on the source key's drain claim: a backend re-key mid-submit previously moved the in-flight entry to a key whose drain claim is a different lock, reopening the double submit single- and cross-window. Now the move happens only after the in-flight drain settles and removes its entry under the old key. - Drain liveness: losers of the claim race arm a lock-release waiter (fires even when the winner crashes — the browser frees dead windows' locks) and rejected sends schedule bounded backoff retries, making MAX_AUTO_DRAIN_ATTEMPTS and the stuck-queue toast actually reachable instead of dead code. - Manual sends survive contention: send-now WAITS (bounded, 15s) for an in-flight drain elsewhere instead of silently dropping the tap, toasts if still blocked, and no longer interrupts a live turn for an entry another window already drained. - Perf: raw-string parse cache makes repeated full-map reads free and restores the per-key referential stability useSessionSlice's re-render bail-out depends on; a shared mutateState primitive keeps the write discipline in one place (migrate included). - Tests: shared composer-queue-test-utils (waiting-capable fake lock manager), regression coverage for every fix above; 47 queue tests, no change to the pre-existing unrelated suite failures; tsc, eslint, and the full npm run build pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XZRUAaXcjP6yq2YMRctTXU
…ueue hardening A 63-agent multi-angle review with 3-lens adversarial verification of the previous two commits surfaced 16 confirmed findings; this closes all of them: - Corrupt storage can no longer brick module init: queue and tombstone payloads are sanitized per session and per entry on read. - The storage-event listener no longer clobbers in-memory entries while persistence is broken, and it writes tombstone-purged maps back so a resurrected ghost leaves storage within one event round trip instead of waiting for 'the next write'. - Tombstone TTL raised to 24h (the cap bounds size; the TTL only sheds abandoned ids) so a quiescent resurrected copy cannot outlive the tombstone hiding it; tombstones recorded while the sidecar is unwritable are kept in an in-memory overlay. - Sent entries additionally hold a per-id Web Lock (storage-independent): a window whose removals cannot persist (quota) no longer causes healthy windows to re-submit everything it sends — drains check the held claim and purge instead. - Manual send-now waits for THIS window's own in-flight drain too (the local ref no longer bounces it into a false 'busy elsewhere' toast, and the toast copy is now window-agnostic); wake-ups that land during an in-flight drain are replayed instead of swallowed. - The claim-release waiter is keyed per session (a stale waiter can no longer suppress the current session's), only counts a genuine wait-and-release as a wake-up (a rejecting lock environment cannot spin), and auto-drain re-picks the fresh head inside the claim so a promote in another window is honored at send time, with failure counts attributed to the entry actually attempted. - Runtime re-key migrations chain, so two quick re-keys cannot strand entries under a dead intermediate key. - updateQueuedPrompt's no-change guard compares attachments structurally, eliminating a full-map write per arrow-step through the queue-edit stack. - Test gaps the review proved (mutation-tested): persistFailed recovery is now pinned in both directions, the retry cap + stuck toast, the manual-send timeout toast, tombstone TTL/cap eviction, unmount safety of the liveness plumbing, and the two-re-key migration chain all have regression tests. 61 queue tests pass; tsc, eslint, and the full npm run build pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XZRUAaXcjP6yq2YMRctTXU
…y re-check, tombstone clamp Third adversarial-review round (34 agents, refute-biased 3-lens panel) over the previous wave surfaced 10 findings; all closed: - The manual-send local-lane wait now races the settle promise against its remaining budget, so a hung in-flight drain can no longer suspend the MANUAL_SEND_WAIT_MS deadline check itself. - runDrain re-checks busy INSIDE the claim (fresh via busyRef): a tap that outwaits a drain whose turn just started takes the designed promote-and-interrupt branch instead of direct-submitting into a busy session; auto-drain simply defers to the busy→false effect edge. - Thrown submits are converted to 'rejected' inside the claim so failure counts land on the entry actually attempted (the pre-claim captured head could differ); lock-manager failures map to rejected/contended by whether an entry had been picked. - Tombstone timestamps are clamped to now at sanitize time: future-dated stamps (corruption) could otherwise never expire, sort newest, occupy the whole cap, and evict every genuine tombstone. - The storage-event listener's ghost-purge write-back is 'quiet': its failure can no longer flip a purely passive window into permanent degraded in-memory mode. - Removed the swallowed-wakeup replay: every in-flight-drain outcome already produces its own follow-up signal (storage event, backoff timer, claim waiter, manual post-kick), so the flag was unreachable belt-and-braces — documented instead of pretending it was load-bearing. - Tests that mutation-testing proved toothless are now real: the migration-chain test holds the source drain claim (the unchained code fails it), the sent-lock producer path is pinned end-to-end through a real drain plus tombstone loss, and the unmount test asserts the actual clearTimeout of the captured retry handle rather than the mountedRef guard's shadow. 62 queue tests pass 3× consecutively; tsc, eslint, and the full npm run build pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XZRUAaXcjP6yq2YMRctTXU
… busy veto, clamp pin Fourth (and converging: 16 → 10 → 3 findings) adversarial-review round: - drainNextQueued (empty-Enter / Cmd+Shift+K) now schedules the same bounded re-check as sendQueuedNow when its drain does not send, so a wake-up dropped during ITS in-flight window is not lost — the removal of the swallowed-wakeup replay had left this one drain initiator without a non-'sent' follow-up signal. - The in-claim busy veto is honored only while the hook still shows the session the drain belongs to: busy is window-global, so after a mid-drain session switch it describes the NEW session and must not stop the old session's send. - The tombstone-timestamp clamp is now pinned by a mutation-verified test: 70 future-dated garbage stamps in the sidecar cannot evict genuine tombstones (the unclamped code fails it). 63 queue tests pass 3× consecutively; tsc, eslint, and the full npm run build pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XZRUAaXcjP6yq2YMRctTXU
What does this PR do?
Fixes cross-window contamination of the composer prompt queue: with several desktop windows open, a prompt queued in one window could silently vanish, resurrect after being drained, or auto-drain and submit from a window the user never typed in.
Root cause
Every desktop window boots
$queuedPromptsBySessionfrom the same localStorage key (hermes.desktop.composerQueue.v1) and then never syncs again:storageevent listener anywhere — each window keeps a private, diverging snapshot of the entire cross-session queue map;save()writes the window's whole snapshot back, so window B's next write clobbers entries window A enqueued after B booted;runDrain()picks the entry from the rendered React slice, so an entry another window already drained can double-submit;Fix (3 small pieces, no architecture change)
composer-queue.ts— listen forstorageevents on the queue key and reload the atom. The event only fires in non-writing windows, so there's no self-echo to guard against.writeSession()/migrateQueuedPrompts()— merge overload()(the live persisted map) instead of the in-memory atom, so a save can never revert a write that landed between sync events. Thestorageevent is async in real browsers; this closes the same-frame race.runDrain()— pick the entry fromgetQueuedPrompts()(live store) instead of the rendered slice, so a cross-window removal observed at send time stops a double-submit. Also drops a now-unneededqueuedPromptsdependency from the callback.Single-window behavior is unchanged:
load()/save()round-trip identical data and the listener never fires without a second window.Related Issue
Fixes #46732
Related: #46194 (queued follow-up state leaking across session switches — the storage-sync half of that report is covered here; the composer UI-state half is separate), #39086, #40394
Type of Change
How was this tested?
composer-queue.test.ts(cross-window sync (#46732)describe block):storageevent (same-frame race)event.key === null)npx vitest run src/store/composer-queue.test.ts→ 20/20 (14 existing + 6 new)npx vitest run src/app/chat/composer→ 42 pass / 1 fail — the failingAttachmentList renders empty list without errorfails identically on a cleanmaincheckout (verified via stash), pre-existing and unrelatedtsc -p . --noEmit→ cleannpx eslinton all three touched files → no issuesScreenshots (if applicable)
N/A — behavioral fix; the symptom is prompts appearing/vanishing across windows.
Checklist