From a7085d97181c3ff4ccf757e6efc9a6f6565025e6 Mon Sep 17 00:00:00 2001 From: dtzp555 Date: Sun, 24 May 2026 11:17:09 +1000 Subject: [PATCH] =?UTF-8?q?fix(server):=20D14=20=E2=80=94=20defer=20res.wr?= =?UTF-8?q?iteHead=20until=20first=20chunk=20in=20real-streaming=20branch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- server.mjs | 21 ++++++++++----- test-features.mjs | 66 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 7 deletions(-) diff --git a/server.mjs b/server.mjs index 37e868d..7d6a05f 100644 --- a/server.mjs +++ b/server.mjs @@ -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; diff --git a/test-features.mjs b/test-features.mjs index 20250ad..020ed90 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -4501,6 +4501,72 @@ describe('Streaming cache-miss real-time (Suite 15)', () => { } }); + it('15d: streaming early error → 502 JSON not 200 empty body (D14 defect fix)', async () => { + // Authority: D14 defect fix — deferred writeHead in real-streaming path. + // Pre-D14: writeHead(200) fired unconditionally before spawn, so a throw + // before first chunk produced HTTP 200 + empty SSE body (silent loss). + // Post-D14: writeHead is deferred to just before the first res.write; + // a throw before any chunk fires sendError(502, ...) identical to the + // buffered path. This test would FAIL on pre-D14 code. + + // Inject a mock anthropic provider whose spawn throws immediately (no yields). + const { loadedProviders: lp15d, __clearCache: cc15d } = await import('./server.mjs'); + const savedAnthropicProvider = lp15d.get('anthropic'); + + const earlyErrorMessage = 'Simulated provider failure before first chunk'; + const throwingProvider = { + ...savedAnthropicProvider, + spawn: async function* () { + throw new ProviderError(earlyErrorMessage, 'SPAWN_FAILED'); + }, + }; + lp15d.set('anthropic', throwingProvider); + cc15d(); + + try { + const r = await fetch({ + port: port15, + method: 'POST', + path: '/v1/chat/completions', + body: { + model: 'claude-sonnet-4-6', + messages: [{ role: 'user', content: 'streaming early error test' }], + stream: true, + }, + }); + + // Must be 502, NOT 200 (the pre-D14 silent-loss behaviour) + assert.equal(r.status, 502, `Expected 502, got ${r.status}. Body: ${r.body.slice(0, 400)}`); + + // Must be JSON error body, NOT SSE + assert.ok( + (r.headers['content-type'] ?? '').includes('application/json'), + `Expected application/json, got: ${r.headers['content-type']}` + ); + + // Body must be parseable as {error: {message, type}} + let parsed; + try { + parsed = JSON.parse(r.body); + } catch { + assert.fail(`Response body is not valid JSON: ${r.body.slice(0, 400)}`); + } + assert.ok(parsed.error, 'Response must have an error field'); + assert.equal(parsed.error.type, 'provider_error', `Expected type provider_error, got ${parsed.error.type}`); + assert.ok( + parsed.error.message.includes(earlyErrorMessage), + `Expected error message to contain "${earlyErrorMessage}", got: ${parsed.error.message}` + ); + } finally { + // Restore the original anthropic provider + if (savedAnthropicProvider !== undefined) { + lp15d.set('anthropic', savedAnthropicProvider); + } else { + lp15d.delete('anthropic'); + } + } + }); + }); // ── Suite 16: Spawn timeout (P1.3) ───────────────────────────────────────────