fix: dispose of teed response bodies on the 529 overload path (#354) - #355
fix: dispose of teed response bodies on the 529 overload path (#354)#355lunetics wants to merge 3 commits into
Conversation
`Response.clone()` tees the body into a second stream fed from the same source, and the tee retains everything the faster branch has already consumed. Three sites on the 529 path produced copies that nothing ever read. Two handed a clone to `provider.parseRateLimit()`, which is declared synchronous in providers/types.ts and reads only headers and status — it cannot consume a body at all, so those copies were orphaned by construction. The third passed a clone to the usage extraction in updateAccountMetadata, whose Anthropic implementation clones again internally and reads only its own inner copy, leaving the outer one untouched. The path runs once per 529 and once more per in-place retry, so an upstream overload leaves orphans in proportion to the burst: a single reset-less 529 with one retry produced four. Each orphan holds a body stream open and keeps the tee buffering for the twin that is concurrently being written to the client — the same socket the runtime then has to tear down. The two header-only calls now receive the response itself instead of a copy. The two remaining copies are genuinely needed, because their readers may consume a body, and now go through `withDisposableCopy`, which cancels whatever the reader left behind. Cancelling disturbs the body, so a copy that was fully read is left alone. The new regression test asserts the invariant rather than the call shape — no clone may survive the path with an unconsumed body — and reports four orphans without this change. Refs tombii#354.
The guard shipped with three weaknesses that an adversarial review surfaced. It patched `Response.prototype.clone` from beforeEach and restored it in afterEach; since Bun runs every test file in one process, a throwing assertion would have left that patch installed for unrelated suites. It then waited a flat 50 ms for the fire-and-forget usage reader in updateAccountMetadata, which is both slower than needed when the code is correct and forgeable on a loaded machine when it is not. And its ProxyContext stub advertised a parseRateLimit that never runs: proxy-operations resolves `getProvider(account.provider) || ctx.provider`, and importing the providers package fills the registry, so the real AnthropicProvider wins — editing that stub changes nothing, which is a trap for the next reader. The patch now lives in a recordClonesDuring() helper that installs it around the single call and removes it in finally, so the window cannot outlive the assertion. The wait polls until every recorded clone is accounted for instead of sleeping: the passing path returns in about 220 ms, the failing path still gets a full second before it judges. The stub carries a note naming the real provider as the one in charge and why that is wanted here — it supplies extractUsageInfo, whose clone is one of the orphans under test. A new assertion requires at least one clone to have been recorded, so the invariant cannot pass vacuously if the path stops running. Behaviour is unchanged: 4 orphans without the fix, 0 with it. Verified by swapping in the pre-fix files directly rather than via git stash, which had collided with a foreign stash entry holding bun.lock. Refs tombii#354.
The fix shipped with a single test case: reset-less 529 with the terminal-attempt flag. That left the paths most likely to regress unguarded — failover, a 529 carrying a reset header, plain 429, SSE streaming, multiple in-place retries, a null body, a throwing reader, and the one place that legitimately reads a body (zai's 429 branch, which has to keep working now that the kept clone is disposed of). There are now 20 cases across four files plus a shared clone-leak-harness.ts holding the fixtures, the scoped Response.prototype.clone recorder and the settle-poller. Each case asserts the invariant — no clone survives with an unconsumed, non-null body — rather than a call shape, and each states whether it can go red without the fix. A red proof by file swap (never git stash: the shared checkout carries foreign stash entries and a pop already produced a bun.lock conflict) reproduces the orphan counts 4, 3, 3, 1 and 5 on the pre-fix tree. Four cases pin invariants that already held on both trees and are labelled regression guards rather than catchers; one file can only be proven red via a scratch copy, because it imports withDisposableCopy, which does not exist pre-fix. withDisposableCopy is exported and marked @internal: reachable for direct unit tests, but outside the package surface, since neither handlers/index.ts nor the package index re-exports it. Refs tombii#354.
Greptile SummaryFixes orphaned
Confidence Score: 5/5Safe to merge; the disposal changes match header-only parseRateLimit and tee cancel-on-copy semantics without altering cooldown or client forwarding behavior. Production edits only drop unnecessary clones and cancel unread private copies after readers finish; provider parseRateLimit paths stay header-only, terminal-529 still forwards the original body, and tests pin no orphaned bodies and intact source readability.
|
| Filename | Overview |
|---|---|
| packages/proxy/src/handlers/proxy-operations.ts | Stops orphaning parseRateLimit clones on 529/retry; disposes terminal-529 rate-limit clone after processProxyResponse. |
| packages/proxy/src/handlers/response-processor.ts | Introduces withDisposableCopy and uses it for usage extraction so unread tee branches are cancelled. |
| packages/proxy/src/handlers/tests/proxy-operations-529-clone-leak.test.ts | End-to-end orphan assertions for 529 terminal/failover/retry, streaming 200, and null-body cases. |
| packages/proxy/src/handlers/tests/response-processor-usage-disposal.test.ts | Unit coverage for throwing/ignoring readers and withDisposableCopy cancel contracts. |
| packages/proxy/src/handlers/tests/clone-leak-harness.ts | Shared clone recording, settlement wait, and fixtures for the leak suite. |
Sequence Diagram
sequenceDiagram
participant Up as Upstream
participant PO as proxyWithAccount
participant RP as processProxyResponse
participant Meta as updateAccountMetadata
participant Client as forwardToClient
Up-->>PO: Response (e.g. 529)
PO->>PO: parseRateLimit(response) headers only
alt terminal 529 needs client body
PO->>PO: clone for rate-limit check
PO->>RP: processProxyResponse(clone)
RP->>Meta: fire-and-forget usage via withDisposableCopy
PO->>PO: cancel clone body if unread
PO->>Client: original response body
else failover / non-clone path
PO->>RP: processProxyResponse(response)
RP->>Meta: withDisposableCopy for usage
end
Reviews (1): Last reviewed commit: "test: cover every clone site on the 529 ..." | Re-trigger Greptile
|
Thanks for the deep dive here — the root-cause writeup on the tee semantics is exactly right, and the 20-case test suite (with honest labelling of which cases are regression guards vs. red-provable) is genuinely well done. There's a conflict with work that landed on
I'll credit this PR's analysis and test coverage in the commit that lands the equivalent fix. Appreciate you tracking this down from a live outage — #354 was a good catch. |
parseRateLimit is synchronous and header-only, so cloning the response before passing it in (initial 529 check, in-place retry check) teed the body into a stream nothing ever read. The terminal-529 rate-limit-check clone is still needed (processProxyResponse may read the body), but was never disposed; the streaming parseUsage clone in updateAccountMetadata had the same gap. Both now go through drainBody, not body.cancel() — this repo already benchmarked cancel() as a no-op leak on Bun for the neighboring extractUsageInfo clone (issue #273). Credit to @lunetics (PR #355) for tracing this to issue #354 and identifying all three sites; the disposal mechanism here differs from that PR to stay consistent with the drainBody approach #273 landed with. Refs #354.
Root Cause:
Response.clone()tees the body — the copy is a second stream fed from the same source, and the tee retains everything the faster branch already consumed. Three sites on the 529 path produced copies nothing ever read. Two handed a clone toprovider.parseRateLimit(), which is declared synchronous (packages/providers/src/types.ts:60) and reads only status and headers, so it cannot consume a body at all — those clones were orphaned by construction. The third passed a clone to the usage extraction inupdateAccountMetadata, whose Anthropic implementation clones again internally and reads only its own inner copy, leaving the outer one untouched.Minimal trigger: one reset-less 529 with one in-place retry leaves 4 orphaned body streams. The path runs once per 529 and once more per retry, so an upstream overload leaves orphans in proportion to the burst. Each orphan holds a stream open and keeps the tee buffering for the twin being written to the client concurrently — the same socket the runtime then tears down.
The Fix: The two header-only calls receive the response itself. The two remaining copies are genuinely needed, because their readers may consume a body, and now go through
withDisposableCopy, which cancels whatever the reader left behind; cancelling disturbs the body, so a fully-read copy is untouched. Byte-identical: the 429 ramp, cooldown durations, streak handling and the terminal-529 forwarding decision.zai's 429 body branch keeps working — it reads through its own separateparseRateLimitFromBodyclone, which the disposal never touches (pinned by a test).Accepted risk: this is a hardening, not a root-cause fix. The production crash it came out of (issue #354, 2026-07-29) terminated inside Bun's socket teardown (
us_socket_close, Bun 1.3.11); that defect lives in the runtime, andoven-sh/bun#31467is open against 1.3.14 with its fix PR unmerged, so no Bun version currently available resolves it. Fewer unread streams at teardown reduces exposure; it does not eliminate the crash. The three cloning sites also pre-date the version that crashed — they come from v3.5.16 and v3.5.31 and are present in the 3.5.41 build production was rolled back to — so this is a long-standing defect, not the 3.5.43/44 regression.Out of scope: the same defect class survives at sites this PR deliberately does not touch, found while writing the tests.
OpenAICompatibleProvider.extractUsageInfoorphans a body on every openai-compatible JSON response (packages/providers/src/providers/openai/provider.ts:138-146); the caller-side clones atproxy-operations.ts:780and:1074are orphaned by construction when their guard short-circuits; and the per-request retryRequestclone at:686is never disposed. Each deserves its own change — filing follow-ups.Validation: 20 test cases across four files plus a shared harness. Every case asserts the invariant (no clone survives with an unconsumed, non-null body) rather than a call shape, and each is labelled honestly. Red proof on a clean
origin/mainworktree: 4 orphans, test fails; with only the two fix files swapped in, green — same worktree, same dependencies, so the fix is the only variable. Orphan counts 4, 3, 3, 1, 5 reproduce on the pre-fix tree. Four cases pin invariants that already held and are labelled regression guards, not catchers; one file needs a scratch copy for its red proof because it importswithDisposableCopy, which does not exist pre-fix.bunx tsc --noEmitclean; 1382 tests green acrossproxy,providersandcore, no new failures. Red proofs use file swaps, nevergit stash— the shared checkout carries foreign stash entries.References: #354 · builds on the 529/429 cooldown separation in #342.