fix(request): respect client backpressure when forwarding streams

[ndycode]

Summary

Implements the backpressure handling CodeRabbit suggested on #532's review (now merged): forwardStreamingResponse ignored res.write()'s return value, so a slow client caused the whole upstream stream to buffer in process memory instead of pacing the upstream reads.

Changes

• lib/request/stream-failover-runtime.ts: when a write reports a full socket buffer, the forwarder now waits for drain before the next upstream read. The waiter also settles on close/error, so a client that disconnects mid-backpressure cannot park the forwarder forever — the next reader.read() then observes the cancellation installed by the existing close handler.
• test/stream-failover-runtime.test.ts: two new deterministic tests — write ordering around drain (deliberately asserting write order, not the source's pull order: ReadableStream prefetches into its internal queue independently of the forwarder's pacing), and the disconnect-during-backpressure path.

Validation

• npm run typecheck; eslint on both files --max-warnings=0
• npx vitest run test/stream-failover-runtime.test.ts test/runtime-rotation-proxy.test.ts — 92 passed, 2 failed: the two known IPv6 ::1 bind environment failures from the documented baseline
• Re-verified after merging the post-integration main (e453111) into the branch

Risk / Rollback

The only behavioral delta is pacing: chunks and termination semantics are unchanged; fast clients never hit the wait. Revert the single fix commit.

https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB

---

Generated by Claude Code

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

implements backpressure handling in forwardStreamingResponse by checking res.write()'s return value and awaiting a new waitForDrain helper before the next upstream read; also adds a fourth test covering the error-during-backpressure path that was flagged on the previous review.

• waitForDrain settles on drain, close, or error, preventing a disconnecting client from parking the forwarder forever; the error path surfaces through the subsequent res.write() throw, which the existing catch block records correctly.
• four new vitest cases cover: write ordering around drain, multiple backpressured chunks, disconnect-during-backpressure, and socket-error-during-backpressure.
• the drain wait itself has no application-level timeout; streamStallTimeoutMs guards only reader.read(), so a slow client that holds the connection open without draining can park the forwarder and its upstream account slot indefinitely.

Confidence Score: 4/5

safe to merge; the core backpressure logic is correct and all three settlement paths are tested, but the drain wait carries no application-level ceiling

the forwarder correctly pauses upstream reads and resumes on drain/close/error, and the new error-during-backpressure test fills the gap flagged in the previous review. the one open concern is that waitForDrain has no timeout of its own — streamStallTimeoutMs only guards reader.read(), so a client that holds the TCP connection open without draining or closing will park the forwarder and the upstream API account slot for however long OS-level keepalives take to fire

lib/request/stream-failover-runtime.ts — specifically the unbounded drain wait in waitForDrain

Important Files Changed

Filename	Overview
lib/request/stream-failover-runtime.ts	adds waitForDrain helper and backpressure guard in forwardStreamingResponse; logic is correct for the drain/close/error paths but the drain wait has no timeout ceiling, leaving the upstream slot potentially parked indefinitely on a stubborn client
test/stream-failover-runtime.test.ts	adds four new backpressure scenarios including error-during-backpressure (previously flagged); coverage is good but tests use real setTimeout/setInterval rather than vi.useFakeTimers(), which is inconsistent with the file's existing timer-test pattern and can flake on slow CI

Sequence Diagram

sequenceDiagram
    participant U as Upstream API
    participant F as forwardStreamingResponse
    participant W as waitForDrain
    participant C as Client Socket

    F->>U: reader.read() guarded by streamStallTimeoutMs
    U-->>F: chunk
    F->>C: res.write(chunk)
    alt write returns true
        C-->>F: ok, continue
    else write returns false - backpressure
        F->>W: await waitForDrain(res)
        W->>C: once drain or close or error
        alt client drains
            C-->>W: drain event
            W-->>F: resolve
            F->>U: reader.read() next chunk
        else client disconnects
            C-->>W: close event
            W-->>F: resolve
            note over F: close handler cancelled reader, next read returns done
        else socket error
            C-->>W: error event
            W-->>F: resolve
            F->>C: res.write() throws ERR_STREAM_DESTROYED
            note over F: catch block records error and calls onStreamError
        end
    end

Loading

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
lib/request/stream-failover-runtime.ts:129-141
**unbounded drain wait could park upstream connections indefinitely**

`waitForDrain` only settles on `drain`, `close`, or `error` — there is no application-level timeout. a client that holds a TCP connection open (keepalives prevent the OS from closing it) while never draining will park this coroutine and its upstream API connection until TCP keepalive fires, which can be hours. `streamStallTimeoutMs` guards only `reader.read()`; nothing guards the drain wait itself. on a proxy that manages account quota, a stalled upstream slot is a real resource cost. consider passing `streamStallTimeoutMs` through to `waitForDrain` as a fallback ceiling (`Promise.race([waitForDrain(res), sleep(timeout)])` that resolves and lets the next `res.write()` surface the state of the response).

### Issue 2 of 2
test/stream-failover-runtime.test.ts:254-257
**real timers in backpressure tests conflict with this file's fake-timer convention**

all timeout-driven tests in this file (`withTimeout` suite) use `vi.useFakeTimers()` per the test/AGENTS.md convention ("no real timeouts"). the four new backpressure tests use real `setTimeout(fn, 20)` and `setInterval(fn, 15)`. on a loaded CI runner the 15ms interval could fire before `waitForDrain` registers its `once("drain")` listener (control must yield to the event loop first), making these assertions timing-sensitive. switching to `vi.useFakeTimers()` + `vi.advanceTimersByTimeAsync()` around the `forwardStreamingResponse` call would make the tests deterministic and ~20x faster.

_{Reviews (2): Last reviewed commit: "test: cover error-during-backpressure an..." | Re-trigger Greptile}

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[coderabbitai]

📝 Walkthrough

Walkthrough

stream forwarder now handles client backpressure by awaiting socket drain when res.write() returns false, preventing memory buildup. test double enhanced to simulate backpressure, with new tests covering drain-event resume and client-disconnect-mid-backpressure scenarios.

Changes

Stream backpressure flow control

Layer / File(s)	Summary
Backpressure wait helper and integration lib/request/stream-failover-runtime.ts	waitForDrain(res) helper resolves on drain, close, or error events while cleaning up listeners. Write loop at lib/request/stream-failover-runtime.ts:178 now awaits waitForDrain() when res.write() returns false instead of continuing unbuffered.
Test infrastructure and backpressure scenarios test/stream-failover-runtime.test.ts	FakeServerResponse at test/stream-failover-runtime.test.ts:47 adds backpressureWrites set and events array to track write order and simulate buffer-full conditions. Two new tests: drain-resume at test/stream-failover-runtime.test.ts:237 and client-close-during-backpressure at test/stream-failover-runtime.test.ts:264.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• ndycode/codex-multi-auth#532: introduces the original forwardStreamingResponse extraction into stream-failover-runtime.ts that this PR now adds backpressure handling to.

Suggested labels

bug

---

review notes

• the waitForDrain helper at lib/request/stream-failover-runtime.ts:125 registers three listeners (drain, close, error) and cleans them up on any of those events. confirm the listener cleanup is idempotent—if drain fires first, both close and error handlers should already be detached when those events eventually fire in the real network lifecycle.

• the write loop at lib/request/stream-failover-runtime.ts:178 calls await waitForDrain(res) only when res.write() returns false. if the client already closed, res.write() may throw instead of returning false. check whether that error path is already handled by the outer try/catch or if a new edge case exists.

• backpressure tests use setImmediate to schedule drain events, but node.js stream drains can fire synchronously or within the same tick depending on buffer state. confirm the test's timing assumptions hold under CI runners with different CPU/memory profiles.

• no windows-specific behavior tested; backpressure handling relies on res.write() return values and drain events, which differ between pipes and TCP sockets on windows. if this code runs in a windows environment (e.g., dev machines or ci agents), verify drain semantics match expectations.

• the new tests do not cover the case where multiple chunks arrive while backpressured (queue depth > 1). existing concurrent upstream reads could still push data into the response buffer faster than the client drains it, causing the second backpressure to trigger before the first drain completes. recommend a test case with at least 3 chunks and backpressure on write:0 and write:2.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	PR description omits required validation checklist items and risk/rollback section is incomplete; docs governance checklist is entirely missing.	Complete the template: check off or explain all validation steps (npm run lint, build, docs updates); fill risk level and rollback plan sections explicitly; review SECURITY.md and CONTRIBUTING.md alignment.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	title follows conventional commits format with fix type, (request) scope, lowercase imperative summary, and is 65 characters — well under the 72-char limit.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/audit-34-stream-backpressure

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch claude/audit-34-stream-backpressure

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}