Fix flaky partial-request shutdown browser tests

[mchwang]

Summary

Fixes the flaky partial-request shutdown tests in test/browser/review.spec.ts, and tightens the in-flight question edit test. Only the test harness changes. No product code changes.

Root cause (reproduced)

drains an in-flight question request before closing its agent manager (and blocks a partially received merge request when shutdown starts, which uses the same pattern) wrote one byte of the body, slept 10 ms, then called app.close(). Nothing proved the server had admitted the request by then. The server runs in the same Node process as the test. If the event loop stalls for more than 10 ms (GC, CPU contention), the timer fires before the server parses the request headers. server.close() then closes the connection, either because it is still idle (read ECONNRESET) or still waiting in the listen backlog (connect ECONNRESET). afterEach then calls close() on the server that was already closed, which adds Server is not running.. That is the exact signature in the report.

destroys a partial request body after the shutdown drain had the same gap. It used the socket write callback as its "admitted" signal, so it could pass without ever exercising the bounded drain.

Fix

• New helper requestAdmitted(server) resolves on the server's request event. By the time it resolves, the handler has registered the request and is reading its body.
• The drain and partial-merge tests now wait for admission, start app.close(), and only then send the rest of the body. The required interleaving (request partially received → shutdown starts → request completes) is now ordered by events, not timers. Promise.race([admitted, completed]) makes a client error surface right away instead of hanging.
• The partial-body test waits for real server admission before measuring the drain bound.
• preserves edits made while a question submission is in flight now waits until the submission is intercepted before editing, asserts the submitted body (text: 'Submitted question'), and checks the durable note list, not just the UI.

Assertions are unchanged or stricter. Nothing is skipped, retried, or given a longer timeout.

Evidence

Environment: Node 26.7.0, Playwright 1.63.0, Chrome Headless Shell 153.0.8010.12 (build 1243), 4 cores. Baseline = main @ af8f3c2. Intermediate checkpoint = fix commit f18e627 (the table rows below). Validated head = 3016d07 (the fix merged with main @ 5e97aa3).

Run	Baseline af8f3c2	This branch
Targeted tests, --repeat-each=10, idle	40/40 passed	50/50 passed (adds the partial-body test)
Targeted tests, --repeat-each=10, 12 CPU-burning processes on 4 cores	4/30 failed, all in the drain test: read ECONNRESET ×3, connect ECONNRESET ×1, each followed by Server is not running.	50/50 passed
Drain + partial-merge tests with a 30 ms event-loop stall injected during the wait (scratch copy)	10/10 failed (connect ECONNRESET + Server is not running.)	20/20 passed with the same stall before the admission wait
Full playwright test	–	53/53 passed at f18e627
npm run typecheck	–	clean

Validated head 3016d07: npm run typecheck clean; targeted tests --repeat-each=10 50/50 passed; full playwright test 58/58 passed. That is the current test count after the merge from main. The 47 in the report and the 53 at f18e627 are older counts. CI on this head: test job (typecheck, unit tests, full browser suite) passed; PR is mergeable.

Each assertion was checked to still fail with the feature broken (each mutation applied alone, then reverted):

Mutation	Test	Result
Close the question agent manager before draining HTTP	drain test	fails (calls expectation)
Remove the post-body stopping && merge 503 check	partial-merge test	fails (status)
Don't destroy partial bodies after the drain grace	partial-body test	fails
Clear the submitted draft unconditionally	both in-flight edit tests	fail (toHaveValue)
Drop rememberDraft() after the action response	both in-flight edit tests	fail (toHaveValue)

Not reproduced

I could not reproduce the toBeVisible / toContainText timeouts in the two preserves edits … in flight tests in any configuration: idle, heavy CPU stress, CDP CPU throttling ×20, the Node side stalled 75–94 % of the time, or right after a forced drain failure (no knock-on effect). Reading app.js and server.ts found no race in the app. Busy-gating, generation checks and draft compare-and-swap all hold. The only timing assumption in the test was editing before the submission was proven in flight, which this PR removes. If those two tests fail again, please attach the trace (trace: retain-on-failure is already on) so the concrete interleaving can be turned into a regression.

Review-lesson audit

• Round 1, Copilot on 3016d07: no findings, approval recommended. Nothing to classify and no AGENTS.md update needed.
• Unresolved threads: none. Deferred follow-ups: none. If the two in-flight edit tests fail again, the trace from that run can become a follow-up issue.

🤖 Generated with Claude Code

https://claude.ai/code/session_013owjkhQZhekCae7KP5rC3P

[Copilot]

Copilot review overview

🟢 Approval recommended

No unresolved review comments remain, and all reviewers assessed it as ready.

Review effort: Lite
Findings: None

What changed in this PR

Fixes flaky partial-request shutdown browser tests and strengthens in-flight edit assertions in the test harness.

Changes:

• Synchronizes tests on actual request admission.
• Orders shutdown and body-completion interleavings deterministically.
• Verifies submitted payloads and durable note persistence.

File	Description
test/​browser/​review.spec.ts	Stabilizes shutdown tests and strengthens in-flight editing coverage.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.