fix(stream): preserve formatted SSE terminal failures by vinhali · Pull Request #6 · Observability360/OmniRoute

vinhali · 2026-09-08T00:16:39Z

Fixes #5

Root cause

createStreamFailureAborter (open-sse/utils/streamFailureBoundary.ts) and emitClaudeEmptyStreamErrorAndAbort (open-sse/utils/stream.ts) both enqueue() a formatted, sanitized terminal SSE payload (a translated response.failed event, or a Claude event: error frame) and then immediately call controller.error().

Per the WHATWG Streams spec, ReadableStreamDefaultController.error() resets the stream's internal queue — confirmed via a direct TransformStream reproduction:

enqueue() then error(): the reader rejects, the queued chunk is never delivered.
enqueue() then terminate(): the reader resolves with the queued chunk, then EOF.

So the just-enqueued, already-sanitized payload was silently discarded before any reader could consume it. A consumer reading the transform's own readable directly (as tests/unit/kiro-tool-call-validation.test.ts does) got a raw internal Error instead of the formatted failure text — this is exactly the shard-1 failure blocking PR #4.

Behavior before / after

Before: enqueue(formattedFailure) → controller.error(new Error(msg)) → reader rejects, formatted payload lost.
After: enqueue(formattedFailure) → controller.terminate() → reader resolves with the formatted payload, then EOF. The writable side is still errored either way (no further upstream writes succeed) — only the readable-side outcome changes.

Why this doesn't weaken sanitization

No production sanitization, redaction, or SAFE_PUBLIC_ERROR_IDENTIFIERS logic was touched — this only changes how an already-sanitized, already-formatted payload reaches the reader once it's been built. Every doesNotMatch/secret-redaction assertion in the touched tests is kept as-is.

Callers verified before making this change

streamHandler.ts's outer disconnect-aware wrapper (createDisconnectAwareStream): reads the inner transform's own reader. The newly-delivered response.failed/event: error frame reaches streamReadiness.ts's frameHasStructuredStreamError() (already recognizes both shapes), so resolveSilentCloseOutcome() correctly stands down — no synthetic duplicate error/truncation frame gets appended once the formatted failure is actually delivered.
PENDING_REQUEST_CLEARED_MARKER / isPendingRequestClearedError in streamHandler.ts: already unconditionally skips the outer onError failover/cooldown callback for these marked errors even under the old (rejecting) contract — removing the marker (no longer meaningful once nothing rejects) changes nothing observable there.
open-sse/services/combo/validateQuality.ts's Claude empty-content-block retry — the actual "combo layer can retry" mechanism referenced by the historical #3685 test comment: reads the raw upstream provider response directly, entirely independent of this transform stream. Unaffected either way.
Two other controller.error() call sites in stream.ts (idle timeout, empty-choices-stream rejection) do not enqueue anything beforehand — left untouched, since error() is correct there (nothing queued to lose).

Test changes

All mechanical: assert.rejects(...) → await the resolved, formatted text and assert.match against it.

tests/unit/kiro-tool-call-validation.test.ts — the originally-failing regression (Responses response.failed). Also tolerates the writer's own close() racing the abort's write-side termination (terminate(), like the old error(), already errors the writable as part of handling that write) — same "WritableStream is closed" class this codebase already treats as expected elsewhere (isClientClosedPipelineError).
tests/unit/claude-empty-stream-error-3685.test.ts — Claude passthrough event: error, including the pending-request-counter regression guard (still passes: clearPendingRequestFromStream() runs before terminate()).
tests/unit/stream-onfailure-callback-logging.test.ts — OpenAI passthrough response.failed, both the onFailure-throws and onFailure-returns-normally variants.
tests/unit/stream-utils.test.ts — 5 tests: Responses rate-limit (translate mode), Claude passthrough/translate empty-stream, Responses usage-limit (passthrough), pending-request-counter (passthrough).

Verified locally

kiro-tool-call-validation.test.ts: 6/6 passing (was 5/6, the target test now passing)
claude-empty-stream-error-3685.test.ts: 5/5 passing (was 1/5 before test updates)
Scoped tsc typecheck: no new regressions from this change (two unrelated pre-existing findings noted below)
git diff --check: clean

Two unrelated pre-existing findings surfaced by typecheck, not touched here per scope:

open-sse/executors/glm.ts TS2554 — the same highWaterMark-argument-count regression already found and fixed independently in the baseline-repair PR (fix(ci): restore release/v3.8.51 baseline health #4); this branch is based on pure release/v3.8.51, which still has it. Out of scope here (explicitly: don't touch GLM/combo).
src/shared/utils/tiktokenCounter.ts TS2307 (js-tiktoken module not found) — a local sandbox node_modules artifact from an unrelated earlier install issue, not a real code problem.

stream-utils.test.ts and stream-onfailure-callback-logging.test.ts could not be fully re-run locally end-to-end (this sandbox's Node v20.11.1 predates the project's required >=22.22.2 and has no working better-sqlite3 binding, so many DB-touching tests in those larger files fail on driver init regardless of branch content) — the 5 edited tests in each follow the exact same, already-validated pattern as the 11 tests that did run cleanly. Real CI (Linux, matching Node/tooling requirements) is the authoritative check for those two files.

Not touched

PR #3, PR #1, PR #4, routing policy, GLM/combo, quality gates, migrations, provider code. No merge performed.

🤖 Generated with Claude Code

…arding them Fixes #5 Root cause: createStreamFailureAborter (open-sse/utils/streamFailureBoundary.ts) and emitClaudeEmptyStreamErrorAndAbort (open-sse/utils/stream.ts) both enqueue a formatted, sanitized terminal SSE payload (a translated response.failed event, or a Claude event: error frame) and then immediately call controller.error(). Per the WHATWG Streams spec, ReadableStreamDefaultController.error() resets the stream's internal queue -- confirmed via direct TransformStream reproduction (enqueue+error: reader rejects, queued chunk never delivered; enqueue+terminate: reader resolves with the queued chunk, then EOF). So the just-enqueued payload was silently discarded before any reader could consume it: a consumer reading the transform's own readable directly (as tests/unit/kiro-tool-call-validation.test.ts does) got a raw internal Error instead of the formatted, sanitized failure text. Fix: both call sites now call controller.terminate() instead of controller.error(). terminate() closes the readable side normally (draining the already-enqueued payload to the reader, then EOF) while still erroring the writable side, so no further upstream writes can succeed -- the exact same writable-side effect as before, just without discarding readable-side content already handed to it. Callers verified before this change: - open-sse/utils/streamHandler.ts's outer disconnect-aware wrapper (createDisconnectAwareStream): reads the inner transform's own reader: the newly-delivered response.failed/event:error frame reaches streamReadiness.ts's frameHasStructuredStreamError() (already recognizes "response.failed" and Claude "event: error"), so resolveSilentCloseOutcome() correctly stands down (no synthetic duplicate error/truncation frame gets appended) once the formatted failure is actually delivered. - streamHandler.ts's PENDING_REQUEST_CLEARED_MARKER / isPendingRequestClearedError guard: already unconditionally skips the outer onError failover/cooldown callback for these marked errors even under the old (rejecting) contract, so removing the marker (no longer meaningful once nothing rejects) changes nothing observable there. - open-sse/services/combo/validateQuality.ts's Claude empty-content-block retry (the actual "combo layer can retry" mechanism referenced by the historical cheaper-inference#3685 test comment): reads the RAW upstream provider response directly, entirely independent of this transform stream -- unaffected either way. - Two other controller.error() call sites in stream.ts (idle timeout at line ~1249, empty-choices-stream rejection at line ~2803) do NOT enqueue anything beforehand -- left untouched, since error() is the correct call there (nothing queued to lose). kiro-tool-call-validation.test.ts also needed a small update: writer.close() after the write that triggers the abort now races the abort's own write-side termination (terminate(), like the old error(), already errors the writable as part of handling that write) -- tolerate the resulting "WritableStream is closed" the same way isClientClosedPipelineError already treats it as expected elsewhere in this codebase, rather than asserting a specific teardown order. Test updates (all mechanical: assert.rejects(...) -> await the resolved, formatted text and assert.match against it -- no security assertion weakened, every doesNotMatch/secret-redaction check kept): - tests/unit/kiro-tool-call-validation.test.ts (Responses response.failed -- the originally-failing regression) - tests/unit/claude-empty-stream-error-3685.test.ts (Claude passthrough event: error, incl. the pending-request-counter regression guard, which still passes: clearPendingRequestFromStream() runs before terminate()) - tests/unit/stream-onfailure-callback-logging.test.ts (OpenAI passthrough response.failed, onFailure-throws and onFailure-returns-normally variants) - tests/unit/stream-utils.test.ts (5 tests: Responses rate-limit translate mode, Claude passthrough/translate empty-stream, Responses usage-limit passthrough, pending-request-counter passthrough) Security/sanitization: unchanged. No production sanitization, redaction, or SAFE_PUBLIC_ERROR_IDENTIFIERS logic touched -- this only changes how an already-sanitized, already-formatted payload reaches the reader once built. Not touched: PR #3, PR #1, PR #4, routing policy, GLM/combo, quality gates, migrations, provider code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MPKHh3DGo2bJBmXGQwtvSb

…e new terminate() contract Real CI caught a whole test file my earlier grep for "assert.rejects" missed (it uses a manual try/catch around the reader loop instead, capturing the caught error into result.error) -- confirmed via the real CI failure output: AssertionError: a translated upstream error must terminate the stream actual: null, expected: true All 9 tests in this file follow the same pattern: assert.ok(result.error, "... must terminate the stream"). Since controller.terminate() (unlike the old controller.error()) doesn't reject the reader, result.error is now always null once the stream ends cleanly -- exactly the fix's intended effect. Each assertion is now assert.equal(result.error, null, "... must terminate cleanly"), and the now-meaningless assertNoHostileDetail(String(result.error)) trailer (checking a constant "null" string for secrets, once result.error can never carry the original message) is removed -- security coverage is unaffected: it was checking the same sanitized text from a second angle, and assertNoHostileDetail(result.output) / assertNoHostileDetail(convertedLog...) right above each already independently prove the delivered content is clean. Verified: 9/9 passing locally. Also confirmed via the same CI run, across all other shards, that every remaining failure (Docs Gates, Merge integrity, API Route Typecheck, and the rest of the Unit Tests shards) is the exact same pre-existing release/v3.8.51 base drift already fixed independently in the baseline-repair PR (#4) -- this branch is based on pure, unfixed release/v3.8.51 by design (per this issue's own "não partir do PR #4" instruction), so it inherits every one of those already-known, already-tracked gaps. None of it originates from this fix. Full breakdown posted on the PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MPKHh3DGo2bJBmXGQwtvSb

vinhali · 2026-09-08T00:36:15Z

Update: real CI found one more file, now fixed — full failure breakdown

Execution SHA: 60d5919 (was 427e615)

One real, genuine miss — now fixed

tests/unit/stream-passthrough-error-redaction.test.ts (9 tests) uses a manual try/catch around its own reader loop instead of assert.rejects — my earlier grep for that keyword missed it. All 9 asserted result.error was truthy ("must terminate the stream"); since controller.terminate() doesn't reject, result.error is now always null once the stream ends cleanly — exactly the intended fix. Updated all 9 to assert.equal(result.error, null, "... must terminate cleanly"), dropped the now-meaningless assertNoHostileDetail(String(result.error)) trailer (checking a constant "null" string), kept every other security assertion (assertNoHostileDetail(result.output) etc.) untouched. Verified: 9/9 passing locally.

Everything else red in this CI run: pre-existing, already fixed in PR #4, not from this change

This branch is based on pure, unfixed release/v3.8.51 by design (per this issue's own "não partir do PR #4" instruction) — so it inherits every base-owned gap PR #4 (the baseline-repair PR) already found and fixed there. Traced each one individually against the actual CI failure output:

Unit Tests shard 1 — chat-rejects-image-only-model.test.ts (missing "Route" in message), skill-doc sync drift, sanitizeErrorMessage ReDoS-threshold flakiness, tunnel-routes-error-sanitization.test.ts, zai-web-silent-empty-repro.test.ts (missing zai_stream_error allowlist entry) — all fixed in PR fix(ci): restore release/v3.8.51 baseline health #4.
Unit Tests shard 2 — Google-key test fixture (too short), model-sync-route.test.ts (missing "Route"), huggingchat-transport-error-redaction.test.ts (obsolete access_token=[REDACTED] assertion) — all fixed in PR fix(ci): restore release/v3.8.51 baseline health #4.
Unit Tests shard 3 — dashboard-request-failed-redaction.test.ts, huggingchat-transport-error-redaction.test.ts again, and handleImageGeneration (codex) — the exact TypeError: Cannot convert object to primitive value at String (<anonymous>) at saveImageErrorResult (imageGeneration.ts:2813) null-prototype crash — fixed in PR fix(ci): restore release/v3.8.51 baseline health #4 via stringifySafe.
Unit Tests shard 4 — stream-handler-public-error-boundary.fixture.ts's obsolete Authorization: [REDACTED] assertions — fixed in PR fix(ci): restore release/v3.8.51 baseline health #4.
Docs Gates / Merge integrity — the same migration-count (169 vs 170) and generated-skill-doc drift fixed in PR fix(ci): restore release/v3.8.51 baseline health #4.
API Route Typecheck — open-sse/executors/glm.ts TS2554 (the highWaterMark argument-count regression, already restored in PR fix(ci): restore release/v3.8.51 baseline health #4) — explicitly out of scope here per "não tocar GLM combo".

None of the above involves open-sse/utils/stream.ts or open-sse/utils/streamFailureBoundary.ts beyond what this PR already touches, and none of it is caused by the controller.error() → controller.terminate() change.

Net result

Real regressions from this fix: 0 (after the stream-passthrough-error-redaction.test.ts update)
Pre-existing, already fixed elsewhere (PR fix(ci): restore release/v3.8.51 baseline health #4): everything else red
The originally-failing tests/unit/kiro-tool-call-validation.test.ts — passing in real CI (confirmed in shard 1's log: ✔ Kiro stream errors become Responses response.failed events)

Next human action

Once PR #4 merges and release/v3.8.51 is updated, rebasing this branch onto the new base should clear essentially all of the remaining red (it's the same base drift, not anything specific to this fix). Not merging.

…size cap Fast Quality Gates' test-file-size check flagged a real, genuine consequence of this branch's own earlier edits: the 5 explanatory (#5) comments added to tests/unit/stream-utils.test.ts (each 3 lines) pushed the file from 2517 to 2522 lines, past its frozen cap. Condensed each to a single line -- same explanation, no content lost -- bringing it back to ~2512, under the cap. Verified via check-file-size.mjs --base-ref against the real base tip: 0 violations (was 1: tests/unit/stream-utils.test.ts). The other Fast Quality Gates failures (mutation-test-coverage, pack-policy, open-sse-typecheck/glm.ts) remain pre-existing release/v3.8.51 base drift already fixed independently in the baseline-repair PR (#4) -- unrelated to this change, per the PR's own earlier comment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MPKHh3DGo2bJBmXGQwtvSb

vinhali · 2026-09-08T00:53:34Z

Update: Fast Quality Gates — one genuine self-inflicted issue, fixed

Execution SHA: c31cdbb

Fast Quality Gates' `test-file-size` check flagged a real consequence of this branch's own earlier commit: the 5 explanatory `(#5)` comments I added to `tests/unit/stream-utils.test.ts` (3 lines each) pushed that file from 2517 to 2522 lines, past its frozen cap. Condensed each to a single line — same explanation, no content lost. Verified via `check-file-size.mjs --base-ref` against the real base tip: 0 violations (was 1).

Fast Quality Gates' other 3 failures (`mutation-test-coverage`, `pack-policy`, `open-sse-typecheck`/`glm.ts`) are confirmed pre-existing `release/v3.8.51` base drift already fixed independently in the baseline-repair PR (#4) — unrelated to this change, as noted in the previous comment.

Waiting on this new CI run. Not merging.

vinhali · 2026-09-08T01:04:43Z

Confirmed: fix verified clean, all remaining red is pre-existing/tracked in PR #4

Head SHA: c31cdbb

`test-file-size`: OK — the `stream-utils.test.ts` fix landed cleanly. Fast Quality Gates now shows only 3 gates failing: `mutation-test-coverage`, `pack-policy`, `open-sse-typecheck` (glm.ts) — all confirmed pre-existing `release/v3.8.51` drift, already fixed independently in PR fix(ci): restore release/v3.8.51 baseline health #4.
`tests/unit/kiro-tool-call-validation.test.ts` ("Kiro stream errors become Responses response.failed events") — passing in this run.
`tests/unit/stream-passthrough-error-redaction.test.ts` (all 9, incl. "translated root error frames..." and "Claude type:error...") — passing in this run.
Docs Gates, Merge integrity, API Route Typecheck, and the remaining Unit Tests shard failures are the same already-documented pre-existing base drift from earlier comments — nothing new.

The actual issue #5 fix (open-sse/utils/stream.ts, open-sse/utils/streamFailureBoundary.ts) is fully verified: correct, tested, and isolated from every pre-existing base issue. Waiting on your review to merge PR #4 first; this branch will need a rebase onto the fixed base afterward, per your own stated sequencing. Not merging.

… fix Merging PR #6's controller.terminate() fix (issue #5) alongside this branch's own highWaterMark restoration in the same file pushed open-sse/utils/stream.ts 3 lines past its frozen file-size cap (3077, already raised in an earlier commit here for the highWaterMark restore). Condensed the terminate() call's explanatory comment to a single trailing line -- same explanation, no content lost, streamFailureBoundary.ts still carries the full detailed writeup. Verified via check-file-size.mjs --base-ref: 0 violations (was 1). Verified via typecheckBaseline.mjs diff against api-typecheck-baseline.json: no new regressions -- the glm.ts TS2554 highWaterMark-argument-count error is gone (this branch's own fix), only the pre-existing local js-tiktoken node_modules sandbox artifact remains. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MPKHh3DGo2bJBmXGQwtvSb

vinhali · 2026-09-08T01:32:03Z

Superseded by #4; the validated #5 fix is now part of release/v3.8.51.

vinhali and others added 2 commits September 7, 2026 21:16

vinhali mentioned this pull request Sep 8, 2026

fix(ci): restore release/v3.8.51 baseline health #4

Merged

vinhali closed this Sep 8, 2026

vinhali mentioned this pull request Sep 8, 2026

feat(combo): O360 explicit-target-routing V1 #1

Merged

Navigation Menu

Repository navigation

fix(stream): preserve formatted SSE terminal failures - #6

fix(stream): preserve formatted SSE terminal failures#6
vinhali wants to merge 3 commits into
release/v3.8.51from
fix/sse-terminal-failure-boundary

vinhali commented Sep 8, 2026

Uh oh!

vinhali commented Sep 8, 2026

Uh oh!

vinhali commented Sep 8, 2026

Uh oh!

vinhali commented Sep 8, 2026

Uh oh!

vinhali commented Sep 8, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

vinhali commented Sep 8, 2026

Root cause

Behavior before / after

Why this doesn't weaken sanitization

Callers verified before making this change

Test changes

Verified locally

Not touched

Uh oh!

vinhali commented Sep 8, 2026

Update: real CI found one more file, now fixed — full failure breakdown

One real, genuine miss — now fixed

Everything else red in this CI run: pre-existing, already fixed in PR #4, not from this change

Net result

Next human action

Uh oh!

vinhali commented Sep 8, 2026

Update: Fast Quality Gates — one genuine self-inflicted issue, fixed

Uh oh!

vinhali commented Sep 8, 2026

Confirmed: fix verified clean, all remaining red is pre-existing/tracked in PR #4

Uh oh!

vinhali commented Sep 8, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant