fix(test): raise the native passthrough reset from a requested pull - #5128
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes add a reusable deferred SSE reset helper. The native passthrough reset test uses the helper instead of directly erroring a stream controller. ChangesSSE reset fixture
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~15 minutes Change: Other · Severity of issue fixed: Low 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
…5073) The reset fixture in tests/server/server-auth.test.ts leaked its stream controller out of start() and errored it from the test body. Whether that rejection had a consumer depended on where Bun's server-side response sink happened to be: between its reads there is no pending read request to reject, so on a loaded runner the fixture's own error escaped as an unhandled error and failed the whole file. It fired on four unrelated heads (#4989, #5024, dev at ecd3ada, and #5085). Raise it from inside pull() on a stream whose high-water mark is zero instead. shouldCallPull is then true only while a read request is outstanding, so pull() runs if and only if a consumer is waiting for the next chunk, and throwing there rejects that read request. The reset now has a consumer no matter when the test calls it. What the code under test sees is unchanged: one SSE chunk, then a mid-stream body error. Closes #5073
276cee3 to
c2b3323
Compare
리뷰 · 우선순위 78 / 80이 PR은 고침은 컨트롤러를 밖으로 빼지 않는 것입니다. 새 헬퍼 라인 - 실질적인 문제 없음. 헬퍼·호출부 모두 기존 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
추가 리뷰 · 우선순위 78 / 80이 PR이 하는 일은 이전과 같습니다. 이번 헤드는 이 헤드의 호스티드 CI도 끝났습니다. 라인 - 동작 버그 없음. 주석만 고친 푸시이고, 고친 방향이 맞습니다. 없는 보장을 지우고, 스트림이 실제로 하는 일만 남겼습니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
1 participant
Summary
tests/server/server-auth.test.tsfails intermittently on the line where the fixture deliberately resets its own upstream stream, and it fails as an unhandled error rather than an assertion. It has fired on four unrelated heads — fix(responses): gate post-header reset recovery on SSE protocol state #4989, fix(codex): let a request-owned main bearer take part in pool ordering #5024,devatecd3adae75, and contributor PR fix(codex): keep routed rows from inheriting experimental context #5085 whose change was thirteen lines in an unrelated subsystem.ReadableStreamDefaultControllerout ofstart()and calledcontroller.error()from the test body. The stream lives on the syntheticBun.serveupstream, one real HTTP hop away from the proxy, so the only thing that can ever observe thatErrorobject is Bun's own response-body sink — the object cannot cross the socket, which is why the report carries the stack of line 4264 while the proxy still delivers its synthetic failed terminal. Erroring a leaked controller happens at a moment of the test's choosing, which is not necessarily a moment when anything is reading: between the sink's reads there is no pending read request to reject, and the rejection's only subscriber is whatever the runtime attaches next. On a busy runner the unhandled-error report wins that race. A downstream read on the proxy's response does not prove the upstream sink currently has a handled read outstanding.pull()on a stream constructed withhighWaterMark: 0. The load-bearing property is that the pull algorithm's promise is always observed:CallPullIfNeededattaches its own rejection handler and routes the failure intoReadableStreamDefaultControllerError, so a throw frompull()cannot be an orphaned rejection whatever the consumer is doing.highWaterMark: 0then keeps the reset in a faithful place — the queue is never stocked ahead of demand, sopull()runs if and only if a read request is outstanding, and the stream is never errored before anything has attached to it, which is the state where the error has nowhere to go. The opening chunk is unaffected, becauseenqueue()ignores the high-water mark.tests/responses/sse-failed-tail.test.ts,tests/server/stream-aborted-marker.test.ts,tests/responses/sse-client-frame-bounds.test.tsand others all error from apull()the consumer requested. ThehighWaterMark: 0part is the addition:streamThatFailsMidStream()instream-aborted-marker.test.tsenqueues on the first pull and errors on the second, and with the default high-water mark that second pull is reached by desired size alone rather than by a waiting consumer.closeReason: "terminal",terminalStatus: "failed",streamAborted: true, one pool failure and a single dispatch.tests/helpers/deferred-reset-sse-upstream.tsbecausetests/server/server-auth.test.tssits exactly at itsfile-size-baseline.jsoncap of 4589 lines, so the explanation could not be written inline. The test file is now 4581 lines, which the ratchet reports asSHRANKand does not treat as an offender, so no baseline edit is needed. Helpers are support-only and need no test-layout entries.What this guarantees, and what it only makes less likely
It guarantees that the fixture's reset is never raised anywhere the stream machinery does not already observe it, and that the stream is never errored before a consumer has attached and asked for a chunk. That removes the window the reported failure needs.
It does not prove Bun has no other unhandled path in its server-side sink; that code is not in this repository. It is also not a claim that a read request is still pending at the instant of the throw — a consumer that cancels in between removes its own request — only that the rejection is consumed either way. The argument that such a window exists at all is the observed intermittency itself: if erroring a response-body stream were reported unconditionally, this test would have failed every run rather than four times.
I also looked for the opposite finding, that the proxy's own reset path leaves an unhandled rejection, and did not find one. The production path attaches its consumers explicitly: both tee branches install
void reader.closed.catch(...)insrc/server/inspection-tee.ts, the inspection consumer is started before the response is returned and reads continuously, client delivery catches body reads and emits a failed SSE tail insrc/server/relay.ts, and a mid-stream failure is explicitly non-replayable there.src/lib/abort.tsalready documents and guards the analogous Bun fetch-body attachment gap on the client side.Verification
~/.opencodex; the change was reviewed by static reasoning against the stream specification and against the existing fixture patterns in this suite instead.start()the stream is not yet started, so the openingenqueuecannot pull. Whenstart()settles there is no reader and desired size is-1. The sink's first read is served from the queue andCallPullIfNeededstill declines, because no request is pending and desired size is0. The second read registers its request andpull()runs. The only behavioural difference from the old fixture is when the stream errors relative to the sink's demand.*.test.tsand excludetests/helpers/; the new 63-line helper isNEW_OKfor the ratchet and the test fileSHRANK;structure:checkenumeratessrc/ownership and this adds no source area; nothing in the helper matches aprivacy:scanpattern.test 1/4..4/4andmacos 1/2..2/2are the checks that carry the file.Checklist
Closes #5073
Summary by CodeRabbit