From 2a58b63e15e3cd92a53b4fc3a30de9cec08b93f7 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 22:50:09 +0300 Subject: [PATCH] =?UTF-8?q?fix(ai-chat):=20=D1=80=D0=B5-=D1=80=D0=B5=D0=B2?= =?UTF-8?q?=D1=8C=D1=8E=20#491=20=E2=80=94=20=D0=B4=D1=83=D0=B1=D0=BB?= =?UTF-8?q?=D1=8C=20=D0=BD=D0=B0=20getRun-fail=20+=20epoch=20RUN=5FFACT=20?= =?UTF-8?q?+=20doc=20(#491)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ре-ревью нашло регрессию класса #137/#161 на пути отказа getRun при tail-only re-seed: - НАХОДКА 1 (MEDIUM/HIGH): в onFinish-disconnect ветка .catch (отказ getRun) на локальном дропе входила в reconnect-ladder БЕЗ ре-сида и БЕЗ фильтра «живой» частичной строки. anchorRef оставался устаревшим (mount-инициализатор), живая частичная строка со шагами N..M-1 — последней; через ~1с реконнект строил ?anchor=&n=N_mount, и при живом ране с покрытием от N_mount (flaky-сеть: SSE и getRun упали, сеть поднялась за 1с) сервер отдавал кадры ≥N_mount → SDK дописывал их к строке, где они УЖЕ есть → дублирование (клиентского дедупа реплея против parts нет). Фикс: восстановлена структурная гарантия удалённого resumeStream- фильтра — на отказе getRun (и на no-persisted-row, и на no-cid) живая частичная строка удаляется из стора по id + anchorRef=null → реконнект реплеит со start в ЧИСТЫЙ стор (полная пересборка) либо 204→poll. Нет пути, где attach tail-applies на строку с уже присутствующими шагами. Тест: getRun-reject на локальном дисконнекте → живая строка отфильтрована + URL без параметров (mutation-verify: без фикса тест краснеет — фильтр не срабатывает). - НАХОДКА 2 (LOW): RUN_FACT в enterReconnect теперь epoch-штампуется (epoch: stampEpoch), как везде (postRun): getRun-rtt расширяет окно onFinish→dispatch, конкурентный SEND_LOCAL во время rtt теперь дропает устаревший RUN_FACT по I1, а не перетирает runFact.runId нового хода. - НАХОДКА 3 (LOW, doc): run-fsm.spec.md обновлён — stripRef/strippedRowRef → anchorRef {id, stepsPersisted}, tail-only + re-seed-from-persist. FSM run-fsm.ts не тронут; инварианты #488 (epoch/honor-in-stopping/ownership-reset/ disconnect-first/render-gate) сохранены. Клиент ai-chat vitest 399 зелёный, tsc 0 ai-chat-ошибок; сервер delta(6, реально исполняется)/registry/step-marker/attach + integration attach — зелёное. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai-chat/components/chat-thread.test.tsx | 37 ++++++++++++++++ .../ai-chat/components/chat-thread.tsx | 44 +++++++++++++++---- .../features/ai-chat/state/run-fsm.spec.md | 10 +++-- 3 files changed, 78 insertions(+), 13 deletions(-) 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.