fix(streaming): relay Darwin rewrites eagerly (#1127) - #1142
Conversation
Reimplement the explicit Darwin config-eager predicate from PR #947 for issue #1127, and apply client rewrites with their budget inside the selected single-reader relay. This was rebuilt instead of cherry-picked because PR #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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6d325079f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await Promise.race([ | ||
| done, |
There was a problem hiding this comment.
The new done promise only confirms teardown after ac.abort(); it does not confirm that the fragment pushed on line 284 reached terminalBoundary.feed(). Since enqueue() resolves the pending reader.read() asynchronously, the abort can be observed first, causing the producer to exit before exercising the rewrite boundary, while both zero-byte assertions still pass. Await a deterministic inspection/processing signal before the first assertion and abort so this remains a focused regression test rather than a vacuous cleanup check.
AGENTS.md reference: AGENTS.md:L228-L230
Useful? React with 👍 / 👎.
| minFixed: string | null = MIN_FIXED_BUN_VERSION, | ||
| ): EagerRelayDecision | null { | ||
| if (needsClientRewrite || (platform !== "win32" && platform !== "darwin")) { | ||
| if (platform !== "win32" && platform !== "darwin") { |
There was a problem hiding this comment.
For darwin + needsClientRewrite + eager-relay, this selector now returns config-eager, but the module header (src/lib/bun-stream-caps.ts:9-12), the mirror comment (src/server/index.ts:343-345), and the architecture contract (structure/04_transports-and-sidecars.md:51-62) still state that Darwin eager relay is restricted to no-rewrite traffic; the architecture document explicitly requires these descriptions and the platform matrix to remain in lockstep. Update those declarations with this policy change so future maintenance and source-invariant tests do not preserve the obsolete fallback.
AGENTS.md reference: src/AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
| const inlineEagerRewrite = needsClientRewrite | ||
| && (win32EagerRewrite || eagerPath?.useEagerRelay === true); |
There was a problem hiding this comment.
On Darwin with explicit eager-relay, a client rewrite can throw while processing response.completed, for example when item-ID or Copilot retained state exceeds its translator budget. The eager producer feeds the raw chunk to inspector before rewriting it, so sawTerminal() is already true; its catch block consequently suppresses the synthetic failure, closes the client stream without the completed block or a response.failed tail, and leaves accounting recorded as completed. The prior Darwin tee path converted this rewrite error into a typed failed terminal, so base failure suppression on whether a terminal was emitted to the client rather than whether the raw inspector saw one.
AGENTS.md reference: src/AGENTS.md:L17-L19
Useful? React with 👍 / 👎.
| onClientCancel: () => options.onNativePassthroughCancel?.(), | ||
| onDone: () => unregisterTurn(turnAc), | ||
| }, win32EagerRewrite ? { rewriteBudget: translatorBudget } : undefined); | ||
| }, inlineEagerRewrite ? { rewriteBudget: translatorBudget } : undefined); |
There was a problem hiding this comment.
For Darwin with explicit eager-relay and any client rewrite, supplying the translator budget here does not bound partial SSE blocks: relaySseEagerBounded calls terminalBoundary.feed(value) before rewriteOutbound, and that boundary accumulates an incomplete event in an unmetered string. The previous Darwin tee path passed every fragment through relaySseWithBlockRewrite, whose reservations fail at the 32 MiB turn limit; after this change, a broken gateway sending an arbitrarily large or never-delimited data: event can grow proxy RSS without limit. Account this framing in the rewrite budget or impose an equivalent cap at the terminal boundary.
AGENTS.md reference: src/AGENTS.md:L15-L19
Useful? React with 👍 / 👎.
|
✅ Deterministic PR hygiene checks passed. |
1 participant
Summary
Reopens the eager-relay transport path on macOS for streams that need a client rewrite, so the macOS half of #893 actually works.
selectEagerPathrejected any caller withneedsClientRewriteset before platform logic ran at all, and its Darwin tail then admitted only aconfig-eagerdecision. A macOS client needing a rewrite therefore always fell back to tee, while the inline payload rewrite andrewriteBudgetwere wired for Win32 only. #1025 repaired the snapshots, but the transport gate closed with #928 was never reopened for macOS.An explicit
eager-relayselection on Darwin now stays eligible when a rewrite is required.Deliberately narrow:
autois not broadened — it remains tee for both rewrite and no-rewrite traffic. Only the explicit opt-in admits Darwin rewrite traffic. A bug fix should not change the default transport strategy as a side effect.isWin32EagerRewriteis unmodified and its full matrix stays green.Closes #1127.
Attribution
The predicate comes from @WZBbiao's (biao) #947, credited via
Co-authored-by. Reported by @0xWinner98 (#1127), whose report correctly identified that #1025 fixed the snapshots while the transport gate stayed closed — that separation is what made this findable.Why rebuilt rather than cherry-picked: #947 conflicts with current
devacross 14 files, and its tests assert through fixedsettle()waits, which pass for the wrong reason — a timing-based assertion can go green while the behavior is broken. The predicate is ported; the tests are rewritten around a deterministiconDonepromise with a bounded failure timeout.#947 is left open for the author. Planning unit:
devlog/_plan/260806_stacked_bug_campaign/070_phase8_darwin_eager_rewrite.md.Stack 9 of the 260806 attribution campaign, stacked on #1141.
Verification
bun test tests/bun-stream-caps.test.ts tests/relay-eager.test.ts tests/responses-snapshot-repair-server.test.ts— 70 pass, 0 failbun run typecheck— exit 0bun run privacy:scan— passedThis ran natively on macOS, so the Darwin path is genuinely exercised rather than only mocked — the one platform phase in this campaign with real local coverage.
Checklist