fix(server): D14 — defer res.writeHead until first chunk in real-streaming branch

cold-audit catch from 2026-05-23

Cold-audit Finding 1 (P2 correctness). Pre-D14 the D10 real-streaming
branch called `res.writeHead(200, ...)` unconditionally before any
provider chunk arrived. If `streamPlugin.spawn(...)` threw or yielded
an error chunk before the first content chunk, the catch block found
`firstChunkEmitted === false` AND `res.headersSent === true` (because
writeHead had already fired), so the `!res.headersSent` branch was
dead code and the path fell through to bare `res.end()`. Result:
HTTP 200 + Content-Type: text/event-stream + zero bytes of body + no
`[DONE]` terminator. The client saw a "successful" empty response.

Compare the buffered path: identical upstream failure returns
HTTP 502 + Content-Type: application/json + `{error: {message, type}}`
body via `sendError(res, 502, ...)`. Inconsistent client-visible
outcomes across paths, with the streaming variant being silently lossy.

Changes (server.mjs +14/-7, test-features.mjs +66):

1. Removed the unconditional pre-loop `res.writeHead(200, ...)` block.
   Replaced with a D14 explanatory comment block.

2. Added a deferred writeHead inside the for-await loop, just before
   the first `res.write`, guarded by `if (!res.headersSent)`:
   ```
   if (!res.headersSent) {
     res.writeHead(200, { Content-Type, Cache-Control, Connection,
                          X-Accel-Buffering, ...streamHeaders });
   }
   streamedChunks.push(irChunk);
   res.write(irChunkToOpenAISSE(...));
   firstChunkEmitted = true;
   ```
   After the first iteration the guard becomes a no-op for subsequent
   chunks. Order inside the loop body is fully synchronous (no await
   between writeHead, write, and the flag assignment), so the
   `firstChunkEmitted` and `res.headersSent` predicates become
   equivalent in steady state.

3. The catch block was NOT touched. Its existing branching was correct
   but the `!res.headersSent → sendError(502)` arm was dead code
   pre-D14; post-D14 it becomes live. This makes the streaming path's
   pre-first-chunk error behavior identical to the buffered path's.

Tests: 291 → 292 (+1):

- Test 15d "streaming early error → 502 JSON not 200 empty body":
  Injects a mock anthropic provider via the `loadedProviders` test
  seam; mock's `spawn` is an async generator that throws ProviderError
  immediately (no yields). Asserts r.status === 502, Content-Type
  includes application/json, body parses to `{error: {message, type}}`
  with type === 'provider_error', and message contains the original
  error string. This test would FAIL on pre-D14 code (would have seen
  200 + empty SSE body).

Scope explicitly excludes:

- Finding #5 (streaming bypasses D4 singleflight) — deferred to D14.5
  with ADR 0005 § D4 amendment pinning the tee-vs-buffer-replay design
  choice. Verified no inflight map, no singleflight machinery, no
  Promise tracking added.

- No ADR change. ADR 0004 § Fallback safety first-chunk rule already
  governs the post-first-chunk behavior (truncated `res.end()`, no
  fallback, no in-band error); D14 preserves that semantics exactly.

Authority:
- OpenAI Chat Completions streaming spec: a failed completion before
  the first byte should be an HTTP error response, not a 200 with empty
  body
  https://platform.openai.com/docs/api-reference/chat/streaming
- ALIGNMENT.md Rule 2(b): no invented response shapes (200 with empty
  body when error occurred is an implicit invention since OpenAI spec
  does not define this as a success shape)
- ADR 0004 § Fallback safety — first-chunk rule (preserved unchanged)
  https://github.com/dtzp555-max/olp/blob/main/docs/adr/0004-fallback-engine.md

Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus, independent
of drafter): APPROVE. Reviewer stashed the diff to trace pre-D14
control flow manually and confirmed the defect existed exactly as
described (writeHead unconditional → catch with firstChunkEmitted=false
and res.headersSent=true → bare res.end() → silent 200 + empty body).
Verified order inside loop body is correct, headers passed to writeHead
are unchanged, sendError signature matches, stop-as-first-chunk edge
case handled correctly, no singleflight introduced, hygiene clean.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
2026-05-24 11:17:09 +10:00
co-authored by Claude Opus 4.7
parent f34b6905eb
commit a7085d9718
2 changed files with 80 additions and 7 deletions
+14 -7
View File
@@ -443,13 +443,9 @@ async function handleChatCompletions(req, res) {
fallbackHops: 0,
});
res.writeHead(200, {
'Content-Type': 'text/event-stream',
'Cache-Control': 'no-cache',
Connection: 'keep-alive',
'X-Accel-Buffering': 'no',
...streamHeaders,
});
// D14: writeHead is deferred until just before the first res.write so that
// pre-first-chunk errors can still produce a JSON 502 (matching the buffered
// path). Calling writeHead unconditionally here was the D14 defect.
const streamedChunks = [];
let firstChunkEmitted = false;
@@ -471,6 +467,17 @@ async function handleChatCompletions(req, res) {
throw new ProviderError(irChunk.error ?? 'Provider emitted error chunk', 'SPAWN_FAILED');
}
// Defer writeHead until the moment we are about to emit the first byte.
// After this point firstChunkEmitted===true ↔ res.headersSent===true.
if (!res.headersSent) {
res.writeHead(200, {
'Content-Type': 'text/event-stream',
'Cache-Control': 'no-cache',
Connection: 'keep-alive',
'X-Accel-Buffering': 'no',
...streamHeaders,
});
}
streamedChunks.push(irChunk);
res.write(irChunkToOpenAISSE(irChunk, requestId, ir.model));
firstChunkEmitted = true;