fix: chat-route tests off the network, and a write-after-end crash the flake was hiding

🟢 open · #405 · open-legal-products/mike ← amal66/mike · opened 8d ago by amal66 · +135-1 across 4 files · ↗ on GitHub

From the PR description

Red repro then green fix

Summary

The chat and project-chat integration suites mock the LLM stream but not the chat-title generator, so every title-generating test makes a real outbound HTTPS call to the LLM provider with the test's fake API key. The test's fate rides on that socket: a fast 401 is swallowed by the route's catch (pass, ~300ms); a slow response or an AI SDK retry loop blows vitest's 20s budget (CI red). This is the flake that failed PR #294's backend job on 2026-08-31 - the identical commits passed on rerun, and on main's own CI 15 minutes earlier.

Fixing the flake exposed a second, worse bug the flake had been hiding: when the title generator succeeds (which it never could in tests, and normally does in production) after a short stream has already ended, the late chat_title write lands on an ended response. Node reports that as an async 'error' event, not a throw - no .catch in the route can reach it - so a completely successful request produces an uncaught exception.

What changed

  • backend/src/lib/chat/routeStreaming.ts - openAssistantSse's write() now drops lines once finish() has run or the response has ended underneath it. This is the one shared seam every SSE chat route writes through.
  • backend/src/lib/chat/__tests__/routeStreaming.test.ts - new; pins the two dropped-write cases (proven red against the unguarded helper) and the asymmetry that makes the guard necessary: the abort signal fires only on early client close, never on the route's own finish().
  • backend/src/__tests__/integration/{chat,projectChat}.routes.test.ts - two layers each:
    1. lib/chatTitle mocked to resolve a fixed title, so chat_title SSE assertions flow through the success path instead of depending on how fast a live provider rejects a fake key.
    2. A global-fetch tripwire that rejects instantly with the attempted URL in the message, so the next unmocked network path fails in milliseconds with a name instead of a 20-second mystery timeout.

Why

Unit/integration suites must be deterministic; a test whose outcome depends on api.openai.com's latency from a CI runner is a coin with a heavy but not certain bias. And the masked crash is a real production defect: any deployment where a title model resolves slower than a short chat stream can hit an uncaught ERR_STREAM_WRITE_AFTER_END.

Base-case replication - see it on main

  1. The flake mechanism. In backend/src/__tests__/integration/projectChat.routes.test.ts, add after the imports: vi.stubGlobal("fetch", vi.fn(() => new Promise(() => {}))); (a provider whose socket never answers - what a slow CI network looks like). Run npx vitest run src/__tests__/integration/projectChat.routes.test.ts -t "uses the shared last-selected model" → the exact CI failure, every time: timed out in 20000ms. Without the stub it passes only because the provider rejects the fake key quickly.
  2. The masked crash. On main, mock lib/chatTitle to succeed (as this PR does) and run npx vitest run src/__tests__/integration/chat.routes.test.ts → vitest reports an Unhandled Error: ERR_STREAM_WRITE_AFTER_END from routeStreaming.ts:73 via routes/chat.ts:763, surfaced by the "surfaces an empty upstream completion as a visible retry error" test.

PR replication - see it fixed

  • npx vitest run src/lib/chat/__tests__/routeStreaming.test.ts on the first commit with routeStreaming.ts stashed → 2 failed; unstashed → 4 passed.
  • Both integration suites, ten consecutive runs: 47/47 every time, tests portion 235-343ms (the project-chat file alone took 20.4s in the failing CI run). The tripwire makes any future regression loud: an unmocked network call now fails instantly with its URL.

Tradeoffs / design decisions (flagged)

  • The write guard silently drops late lines rather than erroring. Deliberate: by the time a late write happens the client is gone or the stream is complete; there is nothing correct to do with the line. The title is still persisted to the DB before the write, so nothing is lost - the next page load shows it.
  • The tripwire is per-file, not global. Only the two chat-route suites get it here; a repo-wide setup-file version would be a larger policy change and could break suites that legitimately stub network differently. Happy to promote it to vitest.config setup in a follow-up if you want the policy everywhere.
  • git checkout ordering: the crash fix is the first commit so every commit in this PR is green under bisect (the test-isolation commit is what exposes the crash).

Testing performed

  • Backend: npx vitest run full suite - 840 passed, 25 skipped, 0 failed, 0 unhandled errors (main shows 1 unhandled error with the title mock applied). npx tsc --noEmit clean. git diff --check clean.
  • Regression proof: stash/red → pop/green for the new helper test; deterministic 20s-timeout repro for the flake (recorded in the GIF above at an 8s budget for brevity - same mechanism).

Relation to PR #294

Unblocks its backend CI noise (its 2026-08-31 red was this flake) but is independent of the queue work - this is main's own test suite and main's own latent crash. #294 picks it up on its next rebase.

🤖 Generated with Claude Code

https://claude.ai/code/session_019d3xaR1WKdri4Ezie4wKL4

Our analysis

Stabilize chat title streaming tests — read the full analysis →

Think the analysis missed something the PR description covers?

Capture this PR into my fork

Download a Markdown prompt that tells Claude how to port every commit in this PR into your working tree. Run it via claude -p < capture-pull-405.md from inside the repo you want the changes in.

⬇ Download capture-pull-405.md