fix(examples): respect raw streaming proxy backpressure - #2663
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Castiron custom code✅ No new custom-code files detected. 32 mixed files remain; 0 existing customizations changed. Compared 32 existing customizations unchanged
A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 34067337990 --repo openai/openai-node \
--name castiron-custom-code-34067337990-1 --dir /tmp/castiron-custom-code-34067337990-1
git apply --stat /tmp/castiron-custom-code-34067337990-1/custom-code.patch
cat /tmp/castiron-custom-code-34067337990-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 7b5d1209090f82f56b4971976147b5c2a859452e 912c68a18b5a05a33a073f21e9bfe3f00ceb2f82
python3 scripts/castiron/custom_code_report.py report \
--base 7b5d1209090f82f56b4971976147b5c2a859452e \
--head 912c68a18b5a05a33a073f21e9bfe3f00ceb2f82 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-912c68a18b5a
cat /tmp/castiron-custom-code-912c68a18b5a/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed exact head 88c9ff8.\n\nThe raw streaming example now correctly stops pulling upstream chunks after res.write() reports backpressure, resumes on drain, and cancels that wait through the existing client-disconnect signal. The abort filtering is appropriately strict: it suppresses only the exact Node AbortError/ABORT_ERR tied to this disconnect signal and preserves genuine write, drain, and upstream failures, including same-tick error/disconnect races. Listener cleanup remains covered on every tested path.\n\nVerified locally at the exact head:\n- focused disconnect/backpressure/security regression suite: 47/47 passed\n- oxlint passed for both changed files\n- oxfmt check passed\n- git diff --check passed\n\nNo unresolved review threads and no in-scope blocking findings.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed exact head 88c9ff8 against freshly fetched main/base 7b5d120. One maintainability issue below should be addressed before approval: the test file grows from 835 to 1,063 lines while duplicating its HTTP lifecycle fixture. The raw example grows from 215 to 242 lines.
The production boundary uses the appropriate Node primitive, events.once. Its narrow abort/cause check preserves error-first failures even when disconnect follows in the same tick; reusing the SDK's broad isAbortError or introducing competing wait promises would weaken this. I found no additional in-scope correctness/security defect. Express behavior, auth/TLS/origin/request handling, SDK runtime and payload limits are unchanged.
Independently executed:
- Canonical four focused suites: 81/81 on Node 22.23.2 and 24.20.0.
- Eight actual-route event/write probes plus two built-SDK localhost HTTP probes on each version. Upstream consumption stayed paused at 21 chunks; maximum response queue was 262,310 bytes; resume delivered all 16 MiB; disconnect canceled upstream with listener cleanup. Base failed the same queue assertion at about 11 MB buffered. These are fixture observations, not limits.
- CJS/ESM build, changed-file pinned lint/format checks, and diff whitespace check passed.
Validation limits: initial localhost checks hit the environment proxy; a process-local localhost exemption fixed them. Type-checking remains blocked by missing example dependencies (express, dotenv, @azure/identity). I did not rerun the author's 7,811-test suite, 18-check harness, exact Node 22.0.0/26 matrix, or platform integrations. The HTTP probes invoke the actual captured route with real ServerResponse and built SDK, not a full Express/TLS deployment.
Move raw backpressure cases into a focused suite and give the example and HTTP harness one owner. Configure upstream content count and terminal event independently while preserving all 47 cases and their assertions.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Re-reviewed all four changed files at 912c68a following author response 5127024766 to my review 5126983318.
The P2 is addressed: both HTTP suites now share one setup/teardown owner, and explicit content/count/terminal-event inputs replace the combined fixture modes. The disconnect suite is 434 lines (835 on base; 1,063 in the first candidate), the focused backpressure suite 198, and the shared harness 474. The raw example remains 242 lines. The split preserves the test descriptions and assertion counts while removing duplicated orchestration. No remaining actionable maintainability, correctness, or security findings.
The production drain/cancel boundary is unchanged from the reviewed implementation. Fresh checks confirm backpressure pauses upstream consumption, matching Node aborts are suppressed, genuine write/error-first failures remain visible, and listeners are cleaned up. Express, auth/TLS/origin/request handling, SDK runtime, and payload limits remain unchanged.
Fresh validation: canonical five focused suites passed 81/81 on Node 22.23.2 and 24.20.0; eight independent route/event probes and two real localhost/built-SDK probes passed on each. During the pause, consumption stayed at 21 chunks and the response queue peaked at 262,310 bytes; resume delivered 16 MiB and disconnect canceled upstream. CJS/ESM build, four-file pinned lint/format checks, and diff whitespace check passed.
Limits: tsc remains blocked by missing example dependencies (express, dotenv, @azure/identity). I did not rerun the author's full 7,811-test suite, 18-check harness, exact Node 22.0.0/26 matrix, or platform integrations. The synthetic HTTP probes exercise the actual captured route and built SDK, not a full Express/TLS deployment.
3 participants
Summary
ServerResponse.write(false)in the raw streaming proxy example by waiting for Node'sdrainevent before reading another upstream chunk.Only the raw example and its handwritten tests change. The Express streaming-helper example, authentication, TLS, origin checks, request handling, SDK runtime, and payload limits are unchanged.
Reproduction
Send a synthetic 16 MiB streamed response through the actual raw-example route and built SDK to a paused local HTTP client. On main, the route consumes all 65 chunks while the client remains paused and queues roughly 14.9 MB in the response.
With this change, the same check pauses upstream consumption, peaks at 262,282 buffered bytes for 256 KiB chunks, and either delivers the complete 16 MiB after the client resumes or cancels the upstream request when the client disconnects. It leaves no drain, close, or error listeners behind. These are observed fixture results, not a new payload cap.
Validation
git diff --checkpass.The generated resource suite and remaining platform integrations are left to CI.