fix(kimi-web): stop the prompt queue from ghost-sending stale attachments - #1833
Conversation
…ents Sending while a turn was running queues prompts locally. A failed flush left entries stuck, and every later session open silently re-submitted them with their old file attachments. Gate the drain on locally witnessed turns, re-drive stuck entries FIFO from real events with a failure budget, restore merged entries on steer failure, persist the queue per session so a refresh loses nothing, and converge queues across tabs via storage events.
🦋 Changeset detectedLatest commit: 9c56b1d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42ca9151e6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| rawState.queuedBySession = { ...rawState.queuedBySession, [sid]: rest }; | ||
| void submitPromptInternal(sid, next.text, next.attachments).then((ok) => { |
There was a problem hiding this comment.
When two tabs hold the same persisted queue and receive the same turn-ended event before either storage event is handled, both execute this removal and immediately start submitPromptInternal; the Vue watcher and cross-tab storage notification run too late to prevent the second request. The daemon can therefore accept the same prompt and attachments twice, or the rejected tab can restore them for a later retry, recreating the ghost-send problem. Claim the entry atomically or establish a single flushing tab before submitting it.
Useful? React with 👍 / 👎.
| function syncQueuedPromptsFromStorage(): void { | ||
| rawState.queuedBySession = loadQueuedPrompts(); |
There was a problem hiding this comment.
If two tabs enqueue different prompts close together, each persists a whole-record snapshot, and this unconditional replacement adopts only whichever snapshot is currently in localStorage; the other tab's unique queued prompt is silently discarded. Storage events do not serialize read-modify-write operations, so queue entries need stable identities and merge/conflict handling rather than last-writer-wins replacement.
Useful? React with 👍 / 👎.
| const failures = (queueFlushFailures.get(sid) ?? 0) + 1; | ||
| if (failures >= MAX_QUEUE_FLUSH_FAILURES) { | ||
| queueFlushFailures.delete(sid); | ||
| return; |
There was a problem hiding this comment.
The failure count is keyed only by session, not by the entry being retried. If one head fails twice and the user then removes or reorders it through unqueue/reorderQueue, the next entry is dropped on its first failure because it inherits the count of two. Track failures per entry or clear the session count whenever user queue operations replace the head.
Useful? React with 👍 / 👎.
Whole-record adoption could silently discard a prompt another tab enqueued concurrently. Queue entries now carry a stable id and enqueue timestamp; adoption union-merges snapshots by id, a shared TTL'd removal set stops flushed/discarded entries from resurrecting (and being flushed twice), an in-flight marker covers the submit window, and manual reorders re-stamp timestamps so they survive merges. The flush failure budget is also tracked per entry instead of per session, so removing or reordering the head no longer hands its strikes to the next entry.
|
Addressed both review comments in 932e228:
Regression coverage for all of the above is in test/workspace-state.test.ts (union merge, no-resurrect, in-flight window, budget reset, reorder stability). Full kimi-web suite (641 tests) + typecheck + lint green. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 932e228f7b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (next.id !== undefined) { | ||
| const submitting = queueInFlightSubmits.get(sid) ?? new Set<string>(); | ||
| submitting.add(next.id); | ||
| queueInFlightSubmits.set(sid, submitting); | ||
| } | ||
| void submitPromptInternal(sid, next.text, next.attachments).then((ok) => { |
There was a problem hiding this comment.
When two tabs have adopted the same queued entry and both observe the same main-turn end, each independently reaches this block and submits the entry because queueInFlightSubmits is only local to each tab. The server accepts concurrent submissions as distinct pending prompts (PromptService.enqueue in packages/agent-core-v2/src/agent/prompt/promptService.ts:84-108), so the tombstone written after either request resolves is too late to prevent duplicate messages, attachments, agent work, and token usage.
Useful? React with 👍 / 👎.
| for (const entry of [...(a[sid] ?? []), ...(b[sid] ?? [])]) { | ||
| if (entry.id !== undefined) { | ||
| if (seenIds.has(entry.id) || gone[entry.id] !== undefined || submitting?.has(entry.id)) { | ||
| continue; |
There was a problem hiding this comment.
When both tabs already contain the same IDs and one tab reorders the queue, this loop always processes the local entries first and discards the storage entries carrying the newer enqueuedAt values. The first tab consequently writes its old order back, while the tab that performed the reorder likewise retains and rewrites its own order; their storage handlers can therefore ping-pong writes indefinitely instead of converging, causing continuous browser work and an unstable queue order.
Useful? React with 👍 / 👎.
| const current = rawState.queuedBySession[sid] ?? []; | ||
| rawState.queuedBySession = { | ||
| ...rawState.queuedBySession, | ||
| [sid]: [next, ...current], | ||
| }; |
There was a problem hiding this comment.
If the user archives a session while this queued submission is pending and the request subsequently rejects—for example because of a concurrent network failure—forgetSession has already discarded the session queue, but this callback unconditionally recreates it. The persistence watch then stores the stale prompt for the archived session, allowing it to reappear after restoration or cross-tab synchronization; guard the callback against forgotten sessions or tombstone/cancel the popped in-flight entry during teardown.
Useful? React with 👍 / 👎.
… forget guard (review round 2) Turn-end events reach every open tab and the server accepts concurrent submissions as distinct prompts, so two tabs holding the same adopted entry could both submit it. Entries now record their owner tab and only the owner flushes (ownerless legacy entries flush anywhere); an idle send adopts entries left behind by closed tabs so a stranded queue can still drain. Cross-tab merges now keep the newer enqueuedAt copy per id so a manual reorder converges instead of ping-ponging writes, and the flush failure callback no longer resurrects a queue whose session was forgotten while the submit was pending.
|
Round 2 addressed in 56889eb:
4 new regression tests (owner gate, idle adopt, merge convergence, forget guard). Suite: 645 tests passing, typecheck + lint clean. |
There was a problem hiding this comment.
💡 Codex Review
kimi-code/apps/kimi-web/src/composables/client/useWorkspaceState.ts
Lines 1920 to 1926 in 56889eb
Respect queue ownership when steering
In a multi-tab session, this queue can contain entries owned by another tab after storage synchronization, but steerPrompt captures, clears, and submits every entry without checking or claiming ownerTabId. If the owner tab also steers or processes the turn end before the resulting tombstone storage event arrives, both tabs submit the same queued text and attachments. Steering should exclude foreign entries or acquire the same coordinated ownership required by normal queue flushing.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| [sid]: (rawState.queuedBySession[sid] ?? []).map((entry) => | ||
| entry.ownerTabId !== undefined && entry.ownerTabId !== TAB_ID | ||
| ? { ...entry, ownerTabId: TAB_ID } | ||
| : entry, | ||
| ), |
There was a problem hiding this comment.
When two tabs share a session, tab A can already be flushing its owned head after a turn ends while tab B sees the session as idle and sends a new message. This loop unconditionally transfers A's entries to B and the following flushQueueHead submits the same head before A's request has produced a tombstone; the localStorage write offers no atomic claim or acknowledgement to prevent that race. This can duplicate the queued prompt and its attachments, so foreign entries should only be adopted through a coordinated claim or after establishing that their owner is gone.
Useful? React with 👍 / 👎.
| const cutoff = Date.now() - QUEUE_GONE_TTL_MS; | ||
| const out: Record<string, number> = {}; | ||
| for (const [id, at] of Object.entries(gone)) { | ||
| if (at >= cutoff) out[id] = at; | ||
| } |
There was a problem hiding this comment.
If a tab retaining a queued entry is frozen for more than 30 minutes while another tab flushes or discards that entry, the delayed storage synchronization will prune its tombstone here before performing the union merge. The frozen tab's stale in-memory copy is then resurrected and persisted again, and its original owner can later submit it a second time. Because there is no acknowledgement proving every tab has observed a removal, expiring tombstones solely by elapsed time reintroduces the stale-attachment ghost-send this change is intended to prevent.
Useful? React with 👍 / 👎.
…nimal fix Cross-tab queue sync over localStorage is a distributed-systems problem (claim/lease, conflict merge, ownership) that keeps generating review findings far beyond this PR's scope. Remove the persistence/hydration/ adoption machinery wholesale; keep the bug fix proper: gated queue drain, event-driven FIFO retry with a per-entry failure budget, steer queue restore, and the forgotten-session flush guard. Durable queued prompts will be designed together with the server-side prompt queue.
|
On the steer-ownership P2 and the cross-tab findings overall: the queue no longer crosses tabs at all. After two rounds of increasingly exotic localStorage multi-tab races, we've cut the persistence/adoption machinery from this PR (9781b50) — durable queued prompts will be designed together with the server-side prompt queue instead of on top of localStorage. What remains is the minimal bug fix itself: gated queue drain, event-driven FIFO retry with a per-entry failure budget (the earlier budget P2 stays fixed), steer queue restore, and the forgotten-session flush guard (the earlier forget P2 stays fixed). The steer-ownership scenario no longer exists because foreign-owned entries no longer exist. Final diff is 4 files, ~250 lines net (mostly tests and comments). Suite: 632 tests passing, typecheck + lint clean. This should be mergeable now. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a576efbea5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const restoreQueue = (): void => { | ||
| if (queue.length === 0) return; | ||
| const current = rawState.queuedBySession[sid] ?? []; | ||
| rawState.queuedBySession = { ...rawState.queuedBySession, [sid]: [...queue, ...current] }; |
There was a problem hiding this comment.
When a steer POST is accepted by the daemon but its response is lost or cannot be parsed, submitPrompt rejects even though the merged prompt is already queued server-side. Unconditionally prepending the captured entries here causes those prompts and attachments to be submitted again during a later drain, recreating the duplicate/ghost-attachment behavior this change is intended to prevent. Restore only after a definitive server rejection, or reconcile ambiguous network failures using the prompt event/request identity.
Useful? React with 👍 / 👎.
| if (count >= MAX_QUEUE_FLUSH_FAILURES) { | ||
| queueFlushFailures.delete(sid); | ||
| return; |
There was a problem hiding this comment.
When the third attempt drops the head while later entries are queued, this returns without submitting the new head. Because the failed submission produced no turn, there is no subsequent turn-end event to drive another flush, so prompts behind it—including the new prompt whose send action triggered this attempt—remain stuck until the user sends yet another message. Advance the queue when dropping the exhausted entry, or otherwise schedule a driver for the remaining head.
Useful? React with 👍 / 👎.
…after drop Two review findings: (1) restoring merged queue entries after ANY steer failure could re-submit prompts the daemon had already accepted when the failure was a lost response — submits now report ok/rejected/uncertain and restores + flush re-queues happen only on definitive daemon rejections, while ambiguous failures drop the entry (the failure toast still tells the user); (2) dropping an exhausted queue head no longer strands the entries behind it — the new head is submitted immediately, carrying its own failure budget.
|
Round 3 addressed in 9c56b1d, with one principle applied to both: never re-submit anything the daemon might already hold.
New regression tests cover drop-then-advance, ambiguous-flush drop, definitive-vs-ambiguous steer restore (running and idle paths). Suite: 635 tests, typecheck + lint green. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…ents (MoonshotAI#1833) * fix(kimi-web): stop the prompt queue from ghost-sending stale attachments Sending while a turn was running queues prompts locally. A failed flush left entries stuck, and every later session open silently re-submitted them with their old file attachments. Gate the drain on locally witnessed turns, re-drive stuck entries FIFO from real events with a failure budget, restore merged entries on steer failure, persist the queue per session so a refresh loses nothing, and converge queues across tabs via storage events. * fix(kimi-web): merge cross-tab queue updates by entry id (review P1/P2) Whole-record adoption could silently discard a prompt another tab enqueued concurrently. Queue entries now carry a stable id and enqueue timestamp; adoption union-merges snapshots by id, a shared TTL'd removal set stops flushed/discarded entries from resurrecting (and being flushed twice), an in-flight marker covers the submit window, and manual reorders re-stamp timestamps so they survive merges. The flush failure budget is also tracked per entry instead of per session, so removing or reordering the head no longer hands its strikes to the next entry. * fix(kimi-web): single-flusher queue entries, merge order convergence, forget guard (review round 2) Turn-end events reach every open tab and the server accepts concurrent submissions as distinct prompts, so two tabs holding the same adopted entry could both submit it. Entries now record their owner tab and only the owner flushes (ownerless legacy entries flush anywhere); an idle send adopts entries left behind by closed tabs so a stranded queue can still drain. Cross-tab merges now keep the newer enqueuedAt copy per id so a manual reorder converges instead of ping-ponging writes, and the flush failure callback no longer resurrects a queue whose session was forgotten while the submit was pending. * refactor(kimi-web): drop the cross-tab queue persistence, keep the minimal fix Cross-tab queue sync over localStorage is a distributed-systems problem (claim/lease, conflict merge, ownership) that keeps generating review findings far beyond this PR's scope. Remove the persistence/hydration/ adoption machinery wholesale; keep the bug fix proper: gated queue drain, event-driven FIFO retry with a per-entry failure budget, steer queue restore, and the forgotten-session flush guard. Durable queued prompts will be designed together with the server-side prompt queue. * chore: align the changeset wording with the reduced scope * fix(kimi-web): never duplicate on ambiguous submit failures; advance after drop Two review findings: (1) restoring merged queue entries after ANY steer failure could re-submit prompts the daemon had already accepted when the failure was a lost response — submits now report ok/rejected/uncertain and restores + flush re-queues happen only on definitive daemon rejections, while ambiguous failures drop the entry (the failure toast still tells the user); (2) dropping an exhausted queue head no longer strands the entries behind it — the new head is submitted immediately, carrying its own failure budget.
…ents (MoonshotAI#1833) * fix(kimi-web): stop the prompt queue from ghost-sending stale attachments Sending while a turn was running queues prompts locally. A failed flush left entries stuck, and every later session open silently re-submitted them with their old file attachments. Gate the drain on locally witnessed turns, re-drive stuck entries FIFO from real events with a failure budget, restore merged entries on steer failure, persist the queue per session so a refresh loses nothing, and converge queues across tabs via storage events. * fix(kimi-web): merge cross-tab queue updates by entry id (review P1/P2) Whole-record adoption could silently discard a prompt another tab enqueued concurrently. Queue entries now carry a stable id and enqueue timestamp; adoption union-merges snapshots by id, a shared TTL'd removal set stops flushed/discarded entries from resurrecting (and being flushed twice), an in-flight marker covers the submit window, and manual reorders re-stamp timestamps so they survive merges. The flush failure budget is also tracked per entry instead of per session, so removing or reordering the head no longer hands its strikes to the next entry. * fix(kimi-web): single-flusher queue entries, merge order convergence, forget guard (review round 2) Turn-end events reach every open tab and the server accepts concurrent submissions as distinct prompts, so two tabs holding the same adopted entry could both submit it. Entries now record their owner tab and only the owner flushes (ownerless legacy entries flush anywhere); an idle send adopts entries left behind by closed tabs so a stranded queue can still drain. Cross-tab merges now keep the newer enqueuedAt copy per id so a manual reorder converges instead of ping-ponging writes, and the flush failure callback no longer resurrects a queue whose session was forgotten while the submit was pending. * refactor(kimi-web): drop the cross-tab queue persistence, keep the minimal fix Cross-tab queue sync over localStorage is a distributed-systems problem (claim/lease, conflict merge, ownership) that keeps generating review findings far beyond this PR's scope. Remove the persistence/hydration/ adoption machinery wholesale; keep the bug fix proper: gated queue drain, event-driven FIFO retry with a per-entry failure budget, steer queue restore, and the forgotten-session flush guard. Durable queued prompts will be designed together with the server-side prompt queue. * chore: align the changeset wording with the reduced scope * fix(kimi-web): never duplicate on ambiguous submit failures; advance after drop Two review findings: (1) restoring merged queue entries after ANY steer failure could re-submit prompts the daemon had already accepted when the failure was a lost response — submits now report ok/rejected/uncertain and restores + flush re-queues happen only on definitive daemon rejections, while ambiguous failures drop the entry (the failure toast still tells the user); (2) dropping an exhausted queue head no longer strands the entries behind it — the new head is submitted immediately, carrying its own failure budget.
…ents (MoonshotAI#1833) * fix(kimi-web): stop the prompt queue from ghost-sending stale attachments Sending while a turn was running queues prompts locally. A failed flush left entries stuck, and every later session open silently re-submitted them with their old file attachments. Gate the drain on locally witnessed turns, re-drive stuck entries FIFO from real events with a failure budget, restore merged entries on steer failure, persist the queue per session so a refresh loses nothing, and converge queues across tabs via storage events. * fix(kimi-web): merge cross-tab queue updates by entry id (review P1/P2) Whole-record adoption could silently discard a prompt another tab enqueued concurrently. Queue entries now carry a stable id and enqueue timestamp; adoption union-merges snapshots by id, a shared TTL'd removal set stops flushed/discarded entries from resurrecting (and being flushed twice), an in-flight marker covers the submit window, and manual reorders re-stamp timestamps so they survive merges. The flush failure budget is also tracked per entry instead of per session, so removing or reordering the head no longer hands its strikes to the next entry. * fix(kimi-web): single-flusher queue entries, merge order convergence, forget guard (review round 2) Turn-end events reach every open tab and the server accepts concurrent submissions as distinct prompts, so two tabs holding the same adopted entry could both submit it. Entries now record their owner tab and only the owner flushes (ownerless legacy entries flush anywhere); an idle send adopts entries left behind by closed tabs so a stranded queue can still drain. Cross-tab merges now keep the newer enqueuedAt copy per id so a manual reorder converges instead of ping-ponging writes, and the flush failure callback no longer resurrects a queue whose session was forgotten while the submit was pending. * refactor(kimi-web): drop the cross-tab queue persistence, keep the minimal fix Cross-tab queue sync over localStorage is a distributed-systems problem (claim/lease, conflict merge, ownership) that keeps generating review findings far beyond this PR's scope. Remove the persistence/hydration/ adoption machinery wholesale; keep the bug fix proper: gated queue drain, event-driven FIFO retry with a per-entry failure budget, steer queue restore, and the forgotten-session flush guard. Durable queued prompts will be designed together with the server-side prompt queue. * chore: align the changeset wording with the reduced scope * fix(kimi-web): never duplicate on ambiguous submit failures; advance after drop Two review findings: (1) restoring merged queue entries after ANY steer failure could re-submit prompts the daemon had already accepted when the failure was a lost response — submits now report ok/rejected/uncertain and restores + flush re-queues happen only on definitive daemon rejections, while ambiguous failures drop the entry (the failure toast still tells the user); (2) dropping an exhausted queue head no longer strands the entries behind it — the new head is submitted immediately, carrying its own failure budget.
1 participant
Related Issue
No linked issue — this comes from a user feedback report (local deployment on macOS); the problem is described below.
Problem
In the web UI, messages sent while a turn is running are held in a local browser queue (
queuedBySession) and flushed when the turn ends. When a flush failed — typically right after the user aborted a turn, while the daemon was still busy — the entry went back to the queue head with nothing left to drive another attempt, so it stayed stuck together with its recorded file ids.Every later session open then ran the queue drain unconditionally (
handleSessionSnapshot→finishPromptLocal), silently re-submitting the stuck entries. Users saw exactly that: the third file they sent arrived together with the first two, and simply reopening a session submitted a pile of previously uploaded files out of nowhere — the agent then re-read every old file and burned tokens on them.What changed
finishPromptLocal): merely re-opening a session can no longer submit queued prompts. This is the direct fix for the ghost-attachment resend.steerPromptrestores the merged queue entries when its submit fails instead of silently losing them.Note: an earlier iteration of this PR also persisted the queue across page refreshes with cross-tab merge/ownership machinery. Review rightly flagged that as a distributed-systems problem beyond this fix's scope, so it was removed again — durable queued prompts will be designed together with the server-side prompt queue instead.
Tests: regression cases cover the reopen-no-drain gate, witnessed-turn drain, FIFO flush, failure re-queue + per-entry drop budget, forgotten-session guard, and steer restore (running/idle paths). Full kimi-web suite (632 tests),
vue-tsc, and oxlint are green.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.