diff --git a/apps/client/src/features/ai-chat/components/chat-thread.test.tsx b/apps/client/src/features/ai-chat/components/chat-thread.test.tsx index 5fbcebaa..47e047cd 100644 --- a/apps/client/src/features/ai-chat/components/chat-thread.test.tsx +++ b/apps/client/src/features/ai-chat/components/chat-thread.test.tsx @@ -1089,6 +1089,43 @@ describe("ChatThread — live reconnect + stalled", () => { ); }); + it("#491 regression (#137/#161 dup): getRun REJECT on a live disconnect drops the live partial + nulls the anchor", async () => { + // The re-seed source (getRun) FAILS — a flaky-network blip (SSE + getRun both + // fail, network recovers in ~1s). The OLD .catch just re-entered the ladder with + // NO re-seed and NO filter, so the reconnect could tail-apply the registry's + // frames onto the live partial that ALREADY has those steps -> duplicated text. + renderLive(); + h.state.getRun.mockReset(); + h.state.getRun.mockRejectedValue(new Error("network")); + await disconnect(); // live partial = liveMsg (id "a2") + expect(h.state.getRun).toHaveBeenCalledWith("c1"); + // THE GUARANTEE: on the getRun failure the live partial (a2) is FILTERED from the + // store, so the reconnect can never tail-apply already-present steps onto it. + // MUTATION-VERIFY: revert the .catch fix (enterReconnect only, no filter) and no + // setMessages call removes a2 -> this reddens. + const removedLivePartial = ( + h.state.setMessages as unknown as { + mock: { calls: [unknown][] }; + } + ).mock.calls.some(([updater]) => { + if (typeof updater !== "function") return false; + const out = (updater as (p: { id: string }[]) => { id: string }[])([ + { id: "a2" }, + { id: "u1" }, + ]); + return !out.some((m) => m.id === "a2"); + }); + expect(removedLivePartial).toBe(true); + expect(screen.getByText(/reconnecting/i)).toBeTruthy(); + advanceToAttempt(1); + expect(h.state.resumeStream).toHaveBeenCalledTimes(1); + // Anchor was nulled -> replay-from-start (no params) / 204 -> poll; never a stale + // ?anchor=&n= over the live partial. + expect(h.state.transport!.prepareReconnectToStreamRequest!().api).toBe( + "/api/ai-chat/runs/c1/stream", + ); + }); + it("#488 (browser QA): the reconnect banner is SHOWN, not masked by the residual useChat error", async () => { // The drop sets useChat `error` (real SDK), and the terminal errorView describes // it ("Lost connection to the server"). The FSM phase-gate must let the diff --git a/apps/client/src/features/ai-chat/components/chat-thread.tsx b/apps/client/src/features/ai-chat/components/chat-thread.tsx index 01e703ce..6eb7fdaa 100644 --- a/apps/client/src/features/ai-chat/components/chat-thread.tsx +++ b/apps/client/src/features/ai-chat/components/chat-thread.tsx @@ -720,13 +720,33 @@ export default function ChatThread({ const runId = extractRunId(message ?? undefined) ?? "pending"; const enterReconnect = (fact: string): void => { if (!mountedRef.current) return; - dispatch({ type: "RUN_FACT", runFact: { runId: fact } }); + // Epoch-stamp the run-fact too (I1): the getRun rtt widens the + // onFinish->dispatch window, so a concurrent SEND_LOCAL during it must be + // able to drop this stale RUN_FACT (else it clobbers the new turn's + // runFact.runId). Consistent with the postRun RUN_FACT stamp. + dispatch({ type: "RUN_FACT", runFact: { runId: fact }, epoch: stampEpoch }); dispatch({ type: "FINISH_DISCONNECT", hasVisibleContent: msgHasVisible, epoch: stampEpoch, }); }; + // Restore the STRUCTURAL guarantee that the live partial is never the + // tail-apply base: drop the live partial from the store by id and null the + // anchor, so the reconnect replays from step 0 into a CLEAN store (a full + // rebuild) or, past any rotation, 204s -> degraded poll. Used on BOTH the + // no-persisted-row and getRun-FAILURE paths — after this there is no path + // where the attach tail-applies frames onto a row that already has them + // (the #137/#161 duplication class). + const dropLivePartialAndReplayFromStart = (): void => { + if (message?.role === "assistant" && typeof message.id === "string") { + const liveId = message.id; + setMessagesRef.current?.((prev) => + prev.filter((m) => m.id !== liveId), + ); + } + anchorRef.current = null; + }; if (cid) { void getRun(cid) .then((res) => { @@ -738,23 +758,29 @@ export default function ChatThread({ stepsPersisted: stepsPersistedOf(persisted), }; // Replace the live partial with the persisted row IN PLACE by id — - // the re-seed from persist. + // the re-seed from persist. The attach's tail (steps >= N) then + // appends to a store holding EXACTLY steps 0..N-1: no duplication. setMessages((prev) => mergeById(prev, rowToUiMessage(persisted))); } else { - // No persisted assistant row (pre-first-frame break): no anchor; the - // reconnect attaches with no params and replays from start. - anchorRef.current = null; + // No persisted assistant row (pre-first-frame break): drop the live + // partial + replay from start (no anchor/n) so nothing is duplicated. + dropLivePartialAndReplayFromStart(); } enterReconnect(res.run?.id ?? runId); }) .catch(() => { - // Persist read failed: keep whatever anchor we had and still enter the - // ladder so a live detached run is not stranded (the attach 204s -> - // degraded poll if it cannot cover). + if (!mountedRef.current) return; + // Persist read FAILED: we cannot re-seed from fresh persist, and a + // stale mount-time anchor over the live partial would tail-apply + // already-present steps -> duplication (a flaky-network blip: + // SSE + getRun both fail, network recovers in ~1s, the registry still + // covers from the mount frontier). Restore the removed-filter guarantee + // instead: drop the live partial + replay from start / 204 -> poll. + dropLivePartialAndReplayFromStart(); enterReconnect(runId); }); } else { - anchorRef.current = null; + dropLivePartialAndReplayFromStart(); enterReconnect(runId); } setStopNotice(null); diff --git a/apps/client/src/features/ai-chat/state/run-fsm.spec.md b/apps/client/src/features/ai-chat/state/run-fsm.spec.md index 4f70f744..c40f3fbf 100644 --- a/apps/client/src/features/ai-chat/state/run-fsm.spec.md +++ b/apps/client/src/features/ai-chat/state/run-fsm.spec.md @@ -121,8 +121,7 @@ holds. **Pending column: empty.** | 11 | `stopPendingRef` | **FSM phase `stopping`** | the deferred stop fires from the chat-id adoption effect while `stopping` | | 12 | `mountedRef` | **retained (React liveness)** | orthogonal to run-lifecycle; gates imperative onFinish side-effects post-unmount. Epoch (I1) handles stale COMMAND-outcomes; DISPOSE bumps it | | 13 | `attemptResumeRef` | **FSM `ATTACH_START` + run-fact** | mount arms attach ONLY on a confirmed active run (commit 4b: streaming-tail status, or POST /run for a user tail) | -| 14 | `stripRef` | **data** (attachStrategy) | strip+replay detail; the `resumeStream` effect reads it | -| 15 | `strippedRowRef` | **data** (attachStrategy) | the anchor row | +| 14–15 | `anchorRef {id, stepsPersisted}` | **data** (attachStrategy) | #491 tail-only: replaced `stripRef`/`strippedRowRef`. The PERSISTED assistant row that pins the run (server invariant 6) + its step frontier N; feeds `?anchor=&n=`. No strip — the seed keeps every row; entering reconnecting re-seeds from persist | | 16 | `attachAbortRef` | **effect-owned controller** | aborted by the `abortAttach` effect in cleanup (I5) | | 17–25 | `chatIdRef`, `openPageRef`, `getEditorSelectionRef`, `roleIdRef`, `stableIdRef`, `queuedRef`, `sendMessageRef`, `statusRef`, `lastForwardedChatIdRef` | **data** (identity/send mirrors) | unchanged — not lifecycle flags | | NEW | `pendingSupersedeRef` | **data** (send-plumbing) | the runId injected into the next `POST /stream {supersede}`; the single replacement for the 3 DELETED one-shots (#8/#9/#10) — net −2 refs | @@ -178,6 +177,9 @@ Pessimism rule: a stale-but-positive fact PERMITS entering recovery (attach); th /run) are effect-owned and aborted in cleanup (`abortAttach` on `DISPOSE`), not render-phase refs. A client abort of an already-sent POST does not cancel the server action, so disarming on unmount is safe. -- **attachStrategy** (strip+replay today) is behind the `resumeStream` effect; the - resume-stack iteration (#491) swaps it to tail-only WITHOUT touching the FSM. +- **attachStrategy** is behind the `resumeStream` effect; #491 swapped it to + tail-only (`?anchor=&n=`, `anchorRef` data) WITHOUT touching the FSM. Entering + reconnecting always re-seeds from persist; on a getRun failure the live partial + is dropped + replay-from-start so it is never the tail-apply base (no #137/#161 + duplication). - **Queue** stays a data structure; flush/interrupt decisions are transitions.