fix(desktop): composer queue — full cross-window hardening (#57516 review + 4 adversarial rounds) - #1
Merged
furancis merged 5 commits intoJul 26, 2026
Conversation
…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
furancis
changed the title
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
2 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?
Addresses teknium1's review on NousResearch#57516 (review 4705098405) and then iterates: after the initial fix, four adversarial multi-agent review rounds (line-scan, removed-behavior, cross-file, concurrency-adversary, test-quality, efficiency/altitude finders; every candidate judged by a 3-lens refute-biased verifier panel) were run against the change, and every confirmed finding was fixed and regression-tested. Findings per round: 10 → 16 → 10 → 3 → converged. This PR targets
fix/multiwindow-composer-queue, so merging it updates the upstream PR.The two reviewed defects, fixed:
mutateState/mutateSession): it reloads the freshest visible state and applies its operation, never a caller-computed snapshot.withSessionDrainClaim); the winner picks inside the claim from a fresh, tombstone-filtered read.Hardening the review rounds drove (each verified + pinned by tests):
markQueuedPromptSent/isQueuedPromptSentElsewhere): storage-independent second layer — a window whose removals can't persist (quota) can't cause healthy windows to re-submit what it already sent.MAX_AUTO_DRAIN_ATTEMPTSand the stuck-queue toast actually reachable; every manual drain path re-kicks a bounded auto-drain check so no wake-up is lost.useSessionSlice's re-render bail-out) and make repeated reads parse-free; structural attachment comparison stops a full-map write per arrow-step in the queue editor.Accepted limits (documented in code): a window crashing after gateway-accept but before its removal persists can still duplicate — closing that needs an idempotency key at the gateway (good follow-up issue); same-instant unlocked mutation interleaves can lose (never duplicate) an update; the drain claim is held across the submit round trip by design, bounded by the gateway request timeout.
Related Issue
Fixes the review feedback on NousResearch#57516 (follow-up to NousResearch#46732).
Type of Change
Changes Made
apps/desktop/src/store/composer-queue.ts: operation-based mutation core, tombstone sidecar, sent-entry locks, persist-failure fallback, sanitization, parse caches, waiting/timeout claim modes, release waiter, chained-safe async migrateapps/desktop/src/app/chat/composer/hooks/use-composer-queue.ts: claim-wrapped drain with in-claim fresh pick + busy re-check (session-scoped), outcome-driven retry/backoff/waiter liveness, waiting manual sends with toast, migration chaining, unmount safetyapps/desktop/src/store/composer-queue-test-utils.ts(new): shared fake Web Locks manager (exclusivity, FIFO waits,ifAvailable, abort,query()), storage-state helpers that also heal module state between testsapps/desktop/src/store/composer-queue.test.ts+apps/desktop/src/app/chat/composer/hooks/use-composer-queue.test.tsx: 63 tests covering every guarantee above; the strongest claims are mutation-verified (tests fail when their fix is reverted)apps/desktop/src/i18n/{types,en,ja,zh,zh-hant}.ts:queueBusyElsewherestringsHow to Test
npm run install:desktop, then inapps/desktop:npx vitest run --environment jsdom src/store/composer-queue.test.ts src/app/chat/composer/hooks/use-composer-queue.test.tsx— 63/63, verified over 3 consecutive runsnpm run typecheckandnpm run linton the touched files — cleannpm run build(tsc -b + vite build + postbuild asserts) — passesChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass (N/A — desktop renderer change; the vitest suites for the touched files pass)Documentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A🤖 Generated with Claude Code
https://claude.ai/code/session_01XZRUAaXcjP6yq2YMRctTXU