fix(server): first-turn session-busy guard, composer draft preservation, test events isolation - #29
Merged
Merged
Conversation
…ft across state pushes, isolate test audit logs First-turn session-busy guard hole: `handleSend` claimed the run with `beginRun(cid, cs.mcodeSessionId)` before the turn existed, so on a session's first turn the claim was registered with `sid: null` and `runsBySid` never guarded the engine session. A second window that had already learned the new sid (sidebar switch / restore after draft promotion) sent to the same brand-new session and got a 200 instead of a 409, then lost its prompt to the engine's "Session already has an active Turn". - state-bus.js: add `updateRunSid(cid, sid)` — backfills or re-points the live run's engine-session claim. It is idempotent, never steals another cid's claim, releases a stale claim only when this cid still owns it (the same ownership rule `endRun` applies), and creates no claim without a live run. All checks are synchronous, so a racing `beginRun` cannot double-register. - mcode-acp.js: call `updateRunSid` the moment the turn's sid is determined (next to `bindDraftToMcodeSid`), covering the first-turn backfill and the load-failure fallback re-point. Composer wipe on state push: page.tsx swaps the composer between the home and chat tree positions the moment the first conversation line lands — a remount that threw away the `useState`-held text, attachment chips, and the send-error banner (a failed 409 looked like a silent vanish). Move the draft into a module-scope store (`webapp/lib/composer-draft.ts`) consumed via `useSyncExternalStore`, so text, attachments, and the error banner survive any remount; only the composer's own success/submit writes clear them. Test audit isolation (chore): unit suites were appending junk to the operator's real `~/.mcode-webui/events.ndjson`. Beyond the known alert.info flood in test/lib/alerts.check.mjs, five more files audit-wrote through the real lib/events.js: test/routes/alerts.check.mjs (alert.*), test/lib/authorize.check.mjs (auth.* decisions), test/routes/sessions-search.check.mjs (auth.* per search), test/lib/mcode-session-delete.test.js (session.delete* for fixture sids). All now redirect `MCODE_WEBUI_EVENTS_PATH` to a per-run tmp dir (lazy per-append resolution; production behavior untouched) and verify zero bytes land in the real log. Tests: state-bus unit coverage for updateRunSid (backfill, race, idempotence, re-point, no ghost claims) in send-run-guard.test.js; a route-level contract check (chat-first-turn-session-guard.check.mjs) driving the real chat.js → runMcodeAcp chain against a fake ACP transport — first-turn 409 session-busy, multi-turn regression, and cross-cid parallel sessions; composer-draft contract tests. Gates: pnpm typecheck 0 errors; webui:typecheck 0 errors; full webui suite 1343 tests / 0 failures (node --test --test-force-exit; one lingering-handle singleton in mcode-acp-note.test.js predates this change — verified on a clean tree); test:webapp 198/198; pnpm build green; source inventory regenerated (3 new files) and check:source green.
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
Three fixes from verified reproducitions (session-isolation/01):
beginRun(cid, sid)registeredsid=nullduring a session's first turn (cs.mcodeSessionIdnot yet assigned), sorunsBySidnever guarded it: a second window sending to the same brand-new session got HTTP 200, the engine rejected it ("Session already has an active Turn") and the message was silently dropped. Fix:updateRunSid(cid, sid)backfills the run registry when the sid is first bound (idempotent, ownership-guarded, synchronous — no double-registration race;endRunsemantics preserved).useSyncExternalStore; typed text and error banners survive pushes/remounts and clear on successful send.~/.mcode-webui/events.ndjson— 5 files write the production audit log during test runs. All now redirectMCODE_WEBUI_EVENTS_PATHto per-test tmp paths (production behavior unchanged).Acceptance (independent agent)
PASS-WITH-CONCERNS — all 5 ticket criteria met. Live two-window verification: window B's send during window A's turn returns
409 {"reason":"session-busy"}with a red composer banner, typed text survives the state-push remount; after A completes, B continues the same engine session. events.ndjson byte-identical before/after test runs (zero-delta, 3×). Route tests construct the true first-turn shape (beginRun sid=null → updateRunSid → B send → 409) — 22/22 ×3 deterministic. Regression: single-window multi-turn and cross-cid parallel on different sessions clean.Gates: typecheck 0 · full webui suite 1343/0 fail · test:webapp 198/198 · build ✓ · check:source ✓ (inventory +3). Note:
router-auth-gate.check.mjsshows environment-dependent flakiness observed both failing and passing on clean trees; not related to this change.Full
pnpm verifydeferred to CI.