fix(streaming): prevent Darwin rewrite stalls - #947
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe change extends eager SSE relay selection from Windows-only payload rewrites to Windows and Darwin. It updates stream-policy documentation and adds coverage for platform selection, large payloads, cancellation cleanup, and completion events. ChangesCross-platform rewrite relay
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant responsesCore
participant requiresEagerRewriteRelay
participant relayEager
Client->>responsesCore: Request passthrough SSE with payload rewrite
responsesCore->>requiresEagerRewriteRelay: Check platform and rewrite requirement
requiresEagerRewriteRelay-->>responsesCore: Select eager relay on Windows or Darwin
responsesCore->>relayEager: Rewrite and stream with bounded buffering
relayEager-->>Client: Deliver rewritten SSE events
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/relay-eager.test.ts`:
- Around line 273-280: Replace the fixed settle(60) wait in the reader.cancel
test with a promise resolved by hooks.onDone, awaiting that promise before
assertions; retain a separate timeout that fails if relay completion hangs. Keep
the existing abort, budget, cancellation, completion, and synthetic-record
assertions unchanged.
- Around line 258-266: Ensure the test scope around relaySseEagerBounded
disposes the caller-owned budget by adding budget.dispose() in a finally block.
Use the existing budget created by createTranslatorBudget and preserve cleanup
even when the relay assertions or execution fail.
In `@tests/responses-image-gen-repair.test.ts`:
- Around line 347-350: Update the large-payload assertion in the response test
to verify that clientBody contains largeEcho, rather than only comparing their
lengths. Keep the existing isEagerRelaySseResponse assertion unchanged and
preserve the test’s large-payload scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 44347c50-36ae-4a17-9fe3-c404609fe8ca
📒 Files selected for processing (14)
docs-site/src/content/docs/ja/troubleshooting/windows-memory.mddocs-site/src/content/docs/ko/troubleshooting/windows-memory.mddocs-site/src/content/docs/ru/troubleshooting/windows-memory.mddocs-site/src/content/docs/troubleshooting/windows-memory.mddocs-site/src/content/docs/zh-cn/troubleshooting/windows-memory.mdsrc/lib/bun-stream-caps.tssrc/server/index.tssrc/server/relay-eager.tssrc/server/responses/core.tsstructure/04_transports-and-sidecars.mdtests/bun-stream-caps.test.tstests/passthrough-abort.test.tstests/relay-eager.test.tstests/responses-image-gen-repair.test.ts
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Maintainer action required: GitHub is holding the fork workflows at
Please approve and run both workflows. The exact head |
2c18223 to
015ca8f
Compare
|
@codex review |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The underlying Darwin rewrite-stall fix is still valuable, but I will not approve the held fork workflows on this old head.
Current dev still keeps Darwin's eager selection explicit-only for ordinary no-rewrite traffic, while this PR addresses a different observed failure: client-facing payload rewrite over the tee + JS-pull chain. Reusing the bounded single-reader relay for rewrite-required Darwin traffic remains the right architectural direction, and this diff does not modify workflows, dependency manifests, lockfiles, or install scripts.
However, head 015ca8fc is now 473 commits behind dev, the PR is draft and conflicting, and the shared relay/inspection/terminal paths have changed substantially since its base (including the current SSE-bound work). The old action_required runs are no longer evidence for the tree we could merge today. Please rebase onto current dev, resolve the relay policy against the current createSseInspector/eager-relay invariants, rerun the focused Darwin/Windows rewrite tests locally, and tick the current review-readiness checklist before requesting workflow approval again. Do not merge dev into the contributor branch.
I am leaving it open because the bug and proposed boundary are meaningful; it is not mergeable or workflow-approvable in its present state.
|
Thank you — your predicate is landing as #1142 (closing #1127), credited to you via You identified the right condition: Why I rebuilt rather than cherry-picked, so this is not mysterious: the branch conflicts with current I also kept the change narrower than the original: Darwin Verified natively on macOS: red-then-green confirmed, 70 pass / 0 fail on the affected suites, 101 pass after rebasing onto the Copilot phase, full suite 9,513 pass / 0 fail. Leaving this PR open for you. |
Reimplement the explicit Darwin config-eager predicate from PR lidge-jun#947 for issue lidge-jun#1127, and apply client rewrites with their budget inside the selected single-reader relay. This was rebuilt instead of cherry-picked because PR lidge-jun#947 conflicts with the current stacked transport code and its fixed settle waits did not deterministically prove relay cleanup. Co-authored-by: biao <16611004+WZBbiao@users.noreply.github.com>
|
Closing — your predicate is on You identified the right condition: Why it was rebuilt rather than cherry-picked, so this is not mysterious: the branch conflicted with I also kept it narrower than the original: Darwin Closing since the fix shipped, not because the work was rejected — the diagnosis was yours. Verified natively on macOS: red-then-green confirmed; 9,593 pass / 0 fail on the merged tree; CI green on the exact |
3 participants
Summary
Root cause and safety
Codex Desktop requests that advertise the
image_gennamespace require OpenCodex to restore client-facing tool payloads. On Darwin, theReadableStream.tee()plus JS-pull rewrite chain can stall before forwarding its first large SSE event. The upstream can return a complete stream while the WebSocket bridge sends zero frames, leaving Codex in Thinking until its 300-second retry.This change reuses the relay already required for Windows rewrite traffic. Raw upstream bytes still feed inspection, request-log metadata, and continuation state before the client-only transform. The incomplete rewrite frame remains charged to the turn translator budget, and cancellation/error teardown releases that charge.
streamModecannot pin the known-bad rewrite chain, while ordinary streams remain configurable exactly as before.Upstream integration
The branch is rebased on
e44d234f, which already contains the terminal-event closure carried by #953 from #942. The eager relay therefore usesinspector.terminalSeen()and closes atresponse.completedeven when no outcome callback is configured; this PR adds the Darwin rewrite-path selection without replacing or weakening that behavior.Verification
bun test tests/bun-stream-caps.test.ts tests/passthrough-abort.test.ts tests/relay-eager.test.ts tests/responses-image-gen-repair.test.ts- 80 passed, 0 failed on final head015ca8fc.bun run prepushfrom a detached/tmpworktree on final head015ca8fc- 8,215 passed, 8 skipped, 0 failed; typecheck, dashboard lint, full tests, privacy scan, and React Doctor all passed.bun scripts/darwin-eager-abort-stress.ts --seed 260801 --per-class 67 --deadline-ms 240000-PASS-WITH-CAVEAT: 67/67 before-first-byte and 67/67 mid-frame aborts completed without crash or hang; the real socket probe reported all 67 backpressure cases unreachable. Deterministic relay tests cover queue-full pause, cancel wakeup, abort wakeup, and budget release.turn.completedunder both pinnedlegacy-teeand installedautoconfigurations; no five-minute reconnect or duplicate upstream request occurred.Checklist
Summary by CodeRabbit
Bug Fixes
Documentation