Repository navigation
fix(sse): publish the stream-failure handlers before the pipeline pumps (#17290) - #17329
diegosouzapw wants to merge 2 commits into
Conversation
…ps (#17290) Since #14725 split handleChatCore into leaves, the streaming tail returned onPipelineStreamError / onClientDisconnectFinalize through `carry`, i.e. only after runStreamingTail had resolved. chatCore's stream controller reads both through getters at event time, so between "pipeline assembled and pumping" and "tail returned" the getter answered null: an upstream failure pumped through the pipeline during the tail's remaining awaits found no handler, and the request was never finalized as a stream failure (no onStreamFailure for the caller, no stream_pipeline_error record). Fast machines rarely hit the window; hosted CI runners hit it on 16 of 21 unit-shard attempts on 2026-10-20 through tests/unit/perplexity-web-stream-error-boundary.test.ts. chatCore now owns a small holder shared by reference with the tail; the tail publishes both handlers into it the moment it creates them, before assembleStreamingPipeline starts pumping, restoring the pre-#14725 closure timing. The carry write-back stays as a no-op safety net for early-return paths. Regression test: tests/unit/chatcore/late-bound-stream-handlers.test.ts pins the holder wiring on both sides, the publish-before-assemble order, and that a pipeline error raised once the handler is installed reaches it.
CI Coverage Report
Coverage artifact was not available for this run. |
…e-late-bound-handler
|
Closing in favour of #17226 by @pacocartones, which was opened first (2026-10-20 01:36Z) and fixes the same race the same way: the streaming tail registers Two things learned since this PR was opened, both recorded on #17290:
The evidence collected on this branch stays useful for #17226: the fixture passed on both hosted runs here ( |
Summary
Root cause of the "flaky"
tests/unit/perplexity-web-stream-error-boundary.test.ts(#17290), which failed 16 of 21 unit-shard attempts across the release/v3.8.53 PRs on 2026-10-20 and held three PRs after two re-runs each.It is not a flaky test — it is a real race introduced by the chatCore decomposition in #14725 (2026-10-06):
runStreamingTailcreatesonPipelineStreamError/onClientDisconnectFinalizeand hands them back to chatCore throughcarry, i.e. only after the tail has resolved.() => onPipelineStreamError), so from "pipeline assembled and pumping" (assembleStreamingPipeline) until "tail returned" the getter answersnull.emitRequestGamificationEvent,runPluginOnResponseHook, theResponseconstruction) finds no handler:handleErrorfalls through toclearPendingRequest, nothing callshandleStreamFailure, so the request is never finalized as a stream failure — noonStreamFailurefor the caller (combo fallback), nostream_pipeline_errorusage record.Fast machines rarely hit the window; hosted CI runners hit it most of the time. Before #14725 the handler lived in the same closure as the getter, so it was visible the moment it was assigned.
Fix
open-sse/handlers/chatCore.ts: the twoletbindings become a small holder object (lateBoundStreamHandlers) that the stream controller's getters read and that is passed to the tail as a dep.open-sse/handlers/chatCore/streamingTail.ts: publishes both handlers into the holder right after creating them — beforeassembleStreamingPipelinestarts pumping. Thecarrywrite-back in chatCore stays as a no-op safety net for early-return paths.No behaviour change on the happy path; the failure path simply finalizes again, as it did before #14725.
Tests
tests/unit/chatcore/late-bound-stream-handlers.test.ts: pins the holder wiring on both sides, asserts the publish happens beforeassembleStreamingPipeline(in the tail, and drivescreateChatStreamControllerto prove a pipeline error raised once the handler is installed reaches it.tests/unit/chatcore/{streaming-tail,leaf-deps-contract}.test.tsstill pass (10/10 locally).check:open-sse-typecheck0 errors; eslint/prettier clean.the downstream pipeline must finalize the stream failure); this PR's own unit shard is the passing half.Follow-ups