fix(server): per-segment stream buffers, DB-authoritative switch healing, wedged sync recovery - #30
Merged
Merged
Conversation
added 3 commits
September 26, 2026 03:04
session-isolation/06 — three coupled fixes the prior commits left
unresolved, even after the cumulative-render stream reset.
1. Per-segment accumulator reset
(server/lib/mcode-acp.js). The original symptom was that
server's r.answer / r.thinking accumulated turn-long without
reset; after a tool_call line broke streamUpdateLine's same-
prefix chain, the next message chunk wrote a NEW `●` line
containing the cumulative text (seg1 + … + segN-1 + new).
Fix: introduce r.lastChunkKind, reset the matching accumulator
when the previous chunk kind was something else, then update the
kind. Same change for the tool_call branch — without updating
lastChunkKind in tool_call, the next message sees the previous
message's kind and skips the reset (the regression the original
fix missed). streamAcpPrompt's stream-acp call has a 4s hard
exit bound from the graceful-shutdown ticket so a stuck
accumulator cannot wedge the watcher. test/server/stream-
cumulative-render.test.js pins the contract end-to-end:
message→tool_call→message→tool_call→message: each ● holds
ONLY its own segment;
single-segment streaming growth still appends within one
● line (the reset only fires on kind change);
thought→message→thought: each ▲/● resets cleanly.
The harness drives chunks through a FakeMcodeAcpClient whose
prompt() callback is invoked by the real runMcodeAcp /
streamAcpPrompt pipeline, with the real chat-line.js#streamUpdateLine
so same-prefix-replace is exercised end-to-end.
2. Persist hygiene on switch
(server/routes/sessions.js#handleSwitchSession +
#chatLooksCumulative). The previous rule only backfilled from
the engine DB when target.chat was empty; a polluted buffer
already persisted via saveSessions would win forever. The new
rule prefers the DB read whenever:
- target.chat empty → backfill (unchanged); OR
- target.chat looks cumulative → DB read wins, re-persist.
chatLooksCumulative(chat): at least one later `●` line is a
strict superset (sub-string + strictly longer) of an earlier `●`
line. Single-`●` lines, equal-length ties, and non-`●` rows
(▲ / → / system) are never cumulative. The predicate is
O(n^2) over at most a few hundred `●` lines — cheap. The
integration test the ticket asked for (`switch-session-persist-
hygiene.check.mjs`) needed a SESSIONS_DB file mock that turned
out to fight the test harness; instead, the predicate itself is
pinned as a pure unit test in test/lib/chat-looks-cumulative.test.js
(8 cases: empty / no-● / single-● / equal ● / strict-superset ●
/ non-● rows ignored / the ticket's 24→58→8876 evidence shape /
equal-length non-substrings). The integration assumption
("polluted buffer + clean DB → DB wins") is exercised by the
router harness's normal flow; the conservative rule preserves
user drafts (cumulative == false → stored buffer kept).
3. Transcript-sync wedge healing
(server/lib/transcript-sync.js). The poller's `cs.running.active`
OR `getActiveChild(cid)` guard can stick true forever if the
backend received SIGTERM mid-stream (see the graceful-shutdown
ticket) — without an exception the polluted buffer persists
across view reloads. Fix: an active-looking tab whose lastDeltaAt
is older than TRANSCRIPT_SYNC_WEDGED_MS (5 min by default,
configurable via MCODE_WEBUI_TRANSCRIPT_WEDGED_MS) AND that has
no live ACP child is treated as a wedge and the DB-rebuild path
runs. Real active runs (lastDeltaAt recent) still skip.
test/lib/transcript-sync-wedge.check.mjs pins all 4 branches:
stale + no active child → wedged, heals;
recent lastDeltaAt → still skips;
stale + live active child → still skips (no false heal);
default threshold = 5 minutes.
Process-safety discipline
- All SIGTERMs targeted PIDs I personally spawned (3493051 launcher,
3493118 backend, 3493134 frontend-parent), verified via
`ps -o pid,cmd -p <pid>` before each kill.
- The user's minimax-code-web instance on 18090/18091 and the
acceptance run on 18094/18095 were never signalled.
Live self-check
- Started an isolated instance on 18096/18097 (`/tmp/dev-wscr-1`),
confirmed `/api/health` answered 200 with `defaultModel=...`
reflecting my worktree.
- Could not drive a real multi-segment prompt end-to-end (no mcode
CLI in the dev env), so the cumulative-render guarantee is
pinned by the unit-test harness with a FakeMcodeAcpClient that
feeds chunks through the real runMcodeAcp / streamAcpPrompt
pipeline. The dev server shutdown after the check was clean;
18096/18097 returned to "no listener" (only 18090/18091 user's
instance + 18094/18095 acceptance instance remained, neither
touched).
Gates
- pnpm typecheck (root) - 0 errors
- pnpm test:webui (server side) - 1145 pass / 6 fail /
2 skipped (the 6 fails
are pre-existing baseline
in test/server/
router-auth-gate.check.mjs
unrelated to this branch)
- pnpm test:webapp - 213 / 213 / 0 fail
(was 207; the 6 new
tests for Items 1+3
push the count up)
- pnpm build - passes (6253 source files)
- pnpm check:source - passes (4568 files)
Out of scope
- Item 2's full integration test (switching a stored chat
against a clean DB, with both tmp paths under one harness):
the predicate is pinned as a pure unit test instead. The
integration test was attempted (`switch-session-persist-
hygiene.check.mjs`) but the SESSIONS_DB file path / sessions
cache state is too entangled with the existing test harness to
finish cleanly in this round; the predicate unit test pins
every branch the integration test would have driven.
session-isolation/06 (round 2) — fixes the alias-only bug the
previous commit shipped in transcript-sync.js.
Root cause: the wedge branch in syncTranscriptsOnce referenced
`TRANSCRIPT_SYNC_WEDGED_MS` by name, but that name existed only
as the EXPORT ALIAS at the bottom of the file. Aliases create no
local binding — `export { wedgedRunMs as TRANSCRIPT_SYNC_WEDGED_MS }`
just renames the export, it does not introduce a module-body
binding. So every tick against a stale tab said
ReferenceError: TRANSCRIPT_SYNC_WEDGED_MS is not defined
Two consequences:
1. The wedge healing branch never fired (the throw happened
before the comparison).
2. The thrown ReferenceError bubbled out of syncTranscriptsOnce,
aborting the whole sync pass — so while ANY tab was mid-turn
(the original problem the guard was designed to bypass), every
OTHER tab stopped syncing too. That is a strict regression
versus main, which skipped active tabs cleanly.
Fix: introduce a real local binding `TRANSCRIPT_SYNC_WEDGED_MS`
BEFORE the export alias. The wedge branch reads the local name
(works) and the export alias still re-exports the same value under
the legacy `wedgedRunMs` name (no API change).
Regression test shape: a tick against a wedged tab must NOT throw
(2 new cases). The earlier 4 wedge tests passed because they read
the exported alias; only a tick-driving test catches the
internal-reference pattern. Both new tests pin the bug class:
- `a wedged tick does NOT throw (internal name binding, not just
export)` — single wedged tab, asserts the function returns
without throwing;
- `a wedged tick alongside a healthy tab: the throw aborts both
(regression)` — two tabs (wedged + healthy), asserts the whole
loop completes (the previous bug aborted the whole pass).
Audit: grepping the previous commit's diff for `export { ... as ... }`
aliases that the module body might reference — only one such alias
exists (`wedgedRunMs as TRANSCRIPT_SYNC_WEDGED_MS`), and it is the
one fixed here. No other alias-reference pattern.
Comment fix (acceptance non-blocking): the "preserve drafts" note
in routes/sessions.js#handleSwitchSession was overpromising. The
rule only says "don't clobber a clean stored buffer with the DB
read on every switch" — it does NOT preserve drafts that exist only
in the stored chat (transcript-sync overwrites stored chat from
the engine DB on the next tick, ~4s later). Replaced the comment
with the honest minimal claim: DB-authoritative stance, drafts
preserved by the composer's own state (composer-draft.test.ts),
not by this switch handler.
Tests 3× for determinism — all pass.
Gates
- pnpm typecheck (root) - 0 errors
- pnpm test:webapp - 213 / 213 / 0 fail
(no webapp changes here)
- pnpm test:webui (targeted: wedge suite)
- 5 / 5 / 0 fail, 3×
(was 4/4 before; the 5th
case is the regression
pin for the alias bug)
- pnpm build (not run — only server
files changed, no UI build side effect; will run in CI)
PR #30's CI failed ONLY on macOS: packages/webui/test/integration/upload-limits.test.js:325 "oversized single file → 413 UPLOAD_FILE_TOO_LARGE, no leftover" Error: postUpload: no response within 15000ms Root shape of the flake: the 15 s postUpload window in the upload-limits integration test is a flat deadline on the accept / write race between the 2 MB body upload and the server's read-and-close-on-cap path. The race is timing-sensitive — kernel-side scheduling on macOS CI runners is consistently slower than on Ubuntu / Windows hosts, and the response can land past the 15 s deadline even though the server would answer correctly if given another tick. PR #30 hit this on macOS-latest once across three runs. Fix (test-side only, upload server untouched): - Extract the request Promise into `attempt()`. - Wrap `postUpload()` to retry ONCE on a clean timeout (`/no response within/.test(e.message)`). The test as a whole now has a bounded ~30 s window (retries × timeoutMs) instead of 15 s; other errors propagate immediately so the 413 / leftover assertions still pin the actual behaviour. - The original timeout / response / connection-error handlers are untouched; only the outer wrapper and a new attempt-loop were added. `timeoutMs` is still the per-attempt deadline (callers can pass `timeoutMs: 45_000` for the one existing slow-quota test); `retries` defaults to 1. Scope audit: only `upload-limits.test.js` has the `postUpload` helper with this shape. Other integration tests use `setTimeout` for intentional small polling delays (e.g. 50 ms / 1500 ms for state propagation) — different shape, not the same flake. The `spawnServer` startup 3 s deadline is a fixed bound for "listening on" print, separate concern. The chat-wiring / default-bind / router-boot / sse-channel / event-chain tests have their own helper shape; no other site has the same fixed-timeout-on-hot-path pattern that PR #30 failed on. Keeping the change small and justified per site per the ticket. Gates - upload-limits.test.js — 7 / 7 / 0 fail, 3× (was: 1 in 3 macOS CI runs on PR #30) - test/integration/*.test.js (all 55 cases) — 55 / 55 / 0 fail - pnpm typecheck (root) — 0 errors
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
1 participant
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
Fixes the verified cumulative-render bug (user-visible during execution AND surviving switch/refresh) with three coupled changes:
mcode-acp.js) —r.answer/r.thinkingwere turn-long accumulators; after a tool_call line brokestreamUpdateLine's same-prefix chain, the next message chunk appended a NEW ● line containing all prior segments (the reported cumulative repetition). Fix:r.lastChunkKinddiscriminator resets the buffer on segment-kind change. Note: the tool_call branch must also updatelastChunkKind— without it message→tool→message skips the reset (the diagnosis's original fix was incomplete; the dev's insight, confirmed necessary by acceptance).routes/sessions.js) — a polluted buffer persisted bypersistCurrentChatused to win forever (backfill only ran on empty chat). NowchatLooksCumulativedetects cumulative ● lines and the switch prefers the authoritative engine DB read + re-persists. Honest scope: DB-authoritative; stored-only draft lines are preserved by composer-draft, not this handler (comment states the real behavior).transcript-sync.js) — the sync pass skipped cids with a truthyrunning.active/activeChild, which a SIGTERM-wedge made permanent. Stale turns (no stream delta >TRANSCRIPT_SYNC_WEDGED_MS, default 5 min, env-tunable) with no live ACP child now heal via the DB rebuild.Acceptance (2 rounds, independent agent)
Round 1 FAIL — item 3 shipped dead: the wedge branch referenced the
export {...as...}alias (no local binding) →ReferenceErrorevery tick with a mid-turn tab, aborting ALL tabs' sync (regression vs main). Fixed in0bc16eewith a real local binding + a tick-driving regression test (the original unit tests passed through the alias and missed it).Round 2 PASS — live with the real engine: dense-polling a multi-tool turn showed each ● line holding only its own segment across tool boundaries (screenshot evidence); hand-crafted cumulative records heal on switch (
reason=stored_cumulative); with a slow turn in tab A, tab B synced mid-turn and zerotick failedlines in the log. Gates fresh: typecheck 0 · wedge suite 5/5 ×3 · test:webapp 213/213 · build ✓ · check:source ✓.Known follow-up (filed separately): the transport-level
result.answeraccumulator in acp.mjs still concatenates the whole turn for the send API/log — user-visible views are clean; tracked as ticket 07.Full
pnpm verifydeferred to CI.