fix(proxy): cap client abort drain window#1277
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough在 response-handler 中新增内部常量 CLIENT_ABORT_DRAIN_MAX_MS = 60_000，并将 clientAbortDrainTimeoutMs 固定为该值；重组 idle/client-drain 定时器的外部作用域与启动时机，客户端断开后显式启动 idle 静默检测；新增/调整单元测试覆盖首字节挂起与活动 chunk 场景下的 60s 边界行为。 客户端drain超时上限
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request caps the client-abort drain timeout at 60 seconds by taking the minimum of the idle timeout and a maximum cap, and updates the corresponding unit tests. The reviewer noted that capping the total background drain duration by the idle timeout when the idle timeout is small is problematic, as the idle timeout is already enforced between chunks. It is recommended to decouple them and keep the background drain cap at a fixed 60 seconds to allow active streams to fully drain.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const idleTimeoutMs = | ||
| provider.streamingIdleTimeoutMs > 0 ? provider.streamingIdleTimeoutMs : Infinity; | ||
| const clientAbortDrainTimeoutMs = idleTimeoutMs === Infinity ? 60_000 : idleTimeoutMs; | ||
| const clientAbortDrainTimeoutMs = Math.min(idleTimeoutMs, CLIENT_ABORT_DRAIN_MAX_MS); |
There was a problem hiding this comment.
Conflation of Idle Timeout and Drain Duration Cap
Using Math.min(idleTimeoutMs, CLIENT_ABORT_DRAIN_MAX_MS) for clientAbortDrainTimeoutMs unnecessarily caps the total duration of the background drain by the idle timeout (inactivity limit) when the idle timeout is small (e.g., 5s or 10s).
Why this is an issue:
- Idle Timeout (
idleTimeoutMs) is the maximum allowed time between chunks (inactivity). - Drain Timeout (
clientAbortDrainTimeoutMs) is a hard cap on the total time the proxy is willing to spend draining the stream in the background to capture terminal markers. - If a provider has a small idle timeout (e.g., 5s), but the stream is highly active and transmitting chunks, the background drain will be forcefully aborted after 5s of total time, even if chunks are actively arriving. This prevents the proxy from capturing the final usage metrics and terminal markers, defeating the purpose of background draining.
Why it is safe to decouple them:
- The idle timeout is already actively enforced during the background drain by
startIdleTimer()(line 2774), which resets on every received chunk and aborts the stream if it becomes inactive foridleTimeoutMs. - Therefore, the hard cap on the background drain should always be a fixed
CLIENT_ABORT_DRAIN_MAX_MS(60s), allowing active streams to fully drain and finish successfully up to the 60s limit.
| const clientAbortDrainTimeoutMs = Math.min(idleTimeoutMs, CLIENT_ABORT_DRAIN_MAX_MS); | |
| const clientAbortDrainTimeoutMs = CLIENT_ABORT_DRAIN_MAX_MS; |
github-actions
Bot
added
bug
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This is a well-scoped, minimal fix that correctly caps the client-abort drain window at 60 seconds regardless of the provider's configured streamingIdleTimeoutMs. The logic change from a ternary to Math.min is both simpler and more correct (handles finite values > 60s, not just Infinity). The test is appropriately updated to validate the new capping behavior with an explicit 120s provider timeout.
PR Size: XS
- Lines changed: 7
- Files changed: 2
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate
- Code clarity - Good
Automated review by Claude AI
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![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: c3f2bade59
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const idleTimeoutMs = | ||
| provider.streamingIdleTimeoutMs > 0 ? provider.streamingIdleTimeoutMs : Infinity; | ||
| const clientAbortDrainTimeoutMs = idleTimeoutMs === Infinity ? 60_000 : idleTimeoutMs; | ||
| const clientAbortDrainTimeoutMs = CLIENT_ABORT_DRAIN_MAX_MS; |
There was a problem hiding this comment.
When a provider configures a short streamingIdleTimeoutMs and the downstream disconnects after response headers but before any body chunk has started the idle timer, this unconditional 60s drain window keeps the upstream stream, accumulated state, and abort listeners alive much longer than the provider's configured timeout (previously 5s would drain for 5s). Since the change is meant to cap long drain windows rather than extend short ones, this should use the smaller of the configured idle timeout and the 60s cap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Handled with a slightly different split: the total client-abort drain cap stays fixed at 60s, and client abort now also arms the existing idle watchdog. That preserves the 60s total drain window for active streams while still letting a short streamingIdleTimeoutMs release pre-body hangs before the first chunk arrives. Added coverage for both cases: active chunks with a 5s idle timeout are allowed to drain until the 60s cap, and pre-body hangs with a 5s idle timeout are aborted by the idle watchdog.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b19373f563
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // still drain buffered final usage and record the request as successful. | ||
| // Idle/response timeout paths still abort via abortController. | ||
| clearClientAbortDrainTimer(); | ||
| startIdleTimer(); |
There was a problem hiding this comment.
When the downstream disconnects after at least one chunk, an idle timer is already measuring silence since the last upstream chunk. Calling startIdleTimer() here clears that timer and starts a fresh full streamingIdleTimeoutMs window, so if the client aborts just before the provider's idle timeout would have fired and the upstream then stays silent, the upstream stream can be retained for almost another full idle window (up to the 60s drain cap) instead of being aborted on the original deadline. This regresses the configured idle bound for already-silent streams; only start a timer when none exists, or preserve the remaining deadline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/proxy/response-handler-client-abort-drain.test.ts (1)
779-783: ⚡ Quick win补一条阈值前断言，避免把“提前超时”也测成通过。
现在这个用例只验证了推进到
5_000ms后会上游 abort，但没有验证5_000ms之前不会 abort。若 idle timer 提前触发，这个测试仍然会通过，覆盖不到这次 PR 想锁住的边界行为。建议改法
- await vi.advanceTimersByTimeAsync(5_000); + await vi.advanceTimersByTimeAsync(4_999); + expect(upstreamController.signal.aborted).toBe(false); + + await vi.advanceTimersByTimeAsync(1); const tasks = asyncTasks.splice(0, asyncTasks.length); await Promise.allSettled(tasks);依据 PR 目标，这个用例应验证的是 5s 边界本身，而不只是最终会超时。
🤖 Prompt for 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. In `@tests/unit/proxy/response-handler-client-abort-drain.test.ts` around lines 779 - 783, 在断言上游在 5_000ms 时被 abort 之前，先断言在阈值之前不会被 abort：在使用 vi.advanceTimersByTimeAsync(5_000) 或替换为先 advanceTimersByTimeAsync(4_999) 前，检查 upstreamController.signal.aborted 为 false（参考测试中的 asyncTasks、vi.advanceTimersByTimeAsync 和 upstreamController.signal.aborted 标识符），然后再推进到 5_000ms 并断言为 true，以确保测试覆盖“恰在边界触发”的行为而非提前触发。
🤖 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.
Nitpick comments:
In `@tests/unit/proxy/response-handler-client-abort-drain.test.ts`:
- Around line 779-783: 在断言上游在 5_000ms 时被 abort 之前，先断言在阈值之前不会被 abort：在使用
vi.advanceTimersByTimeAsync(5_000) 或替换为先 advanceTimersByTimeAsync(4_999) 前，检查
upstreamController.signal.aborted 为 false（参考测试中的
asyncTasks、vi.advanceTimersByTimeAsync 和 upstreamController.signal.aborted
标识符），然后再推进到 5_000ms 并断言为 true，以确保测试覆盖“恰在边界触发”的行为而非提前触发。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa35d2f8-e10b-4997-b9da-597c75631259
📒 Files selected for processing (2)
src/app/v1/_lib/proxy/response-handler.tstests/unit/proxy/response-handler-client-abort-drain.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/v1/_lib/proxy/response-handler.ts
Brisbanehuang
force-pushed
the
codex/cch-client-abort-drain-cap
branch
from
bc33fce to
aca0c32
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aca0c32331
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const clientAbortDrainTimeoutMs = idleTimeoutMs === Infinity ? 60_000 : idleTimeoutMs; | ||
| const clientAbortDrainTimeoutMs = CLIENT_ABORT_DRAIN_MAX_MS; | ||
|
|
||
| // ⭐ 提升 idleTimeoutId 到外部作用域，以便客户端断开时能清除 |
There was a problem hiding this comment.
AGENTS.md for this repo says: “No Emoji in Code - Never use emoji characters in any code, comments, or string literals.” This hoisted timer block adds comments containing emoji in the changed hunk, so the patch violates the repository rule even though the logic was moved from the inner scope; please remove the emoji from the newly added/moved comments before merging.
Useful? React with 👍 / 👎.
Brisbanehuang
force-pushed
the
codex/cch-client-abort-drain-cap
branch
from
aca0c32 to
3d1e139
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d1e139b89
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!idleTimeoutId) { | ||
| startIdleTimer(); |
There was a problem hiding this comment.
When the downstream aborts before the first upstream chunk and streamingIdleTimeoutMs is finite, this new call arms the normal idle watchdog during the client-abort drain. If that watchdog fires, the existing isIdleTimeout catch branch runs before the client-abort branch and calls finalizeStream(..., false, false, "STREAM_IDLE_TIMEOUT"), so an already-disconnected client is persisted and accounted as a provider/idle failure (502 / STREAM_IDLE_TIMEOUT) instead of the intended 499 CLIENT_ABORTED path; the new pre-body test even expects the 499 behavior. Please preserve the client-aborted classification when this post-abort idle timer terminates the drain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b2aeba0. The idle-timeout catch path now preserves clientAborted when the idle watchdog fires during a post-abort drain, so those records continue through the 499 CLIENT_ABORTED finalization path while non-client-aborted idle timeouts remain STREAM_IDLE_TIMEOUT.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
Summary
This caps the background drain window after a downstream client aborts at 60 seconds, even when the provider's streaming idle timeout is configured to a larger value.
The normal streaming idle timeout still controls live client connections. This change only applies after the client has already disconnected and the proxy is doing best-effort background draining so it can still capture terminal markers / usage and finalize the request correctly.
Why
#1251 added a bounded drain fallback when
streamingIdleTimeoutMsis disabled (0/Infinity). However, when a provider sets a long idle timeout such as 120s or 600s, the client-abort drain window currently inherits that full duration. That can keep an upstream stream, accumulated response chunks, session state, and abort listeners alive long after there is no downstream client left to receive the response.A fixed 60s cap keeps the drain behavior best-effort while giving resource retention a predictable upper bound.
Trade-off
This is intentionally a behavior change for client-aborted streams. If an upstream remains silent for more than 60 seconds after the downstream client has already disconnected and only then emits final usage / terminal markers, the proxy may stop draining before capturing that final accounting data.
That trade-off is limited to already-disconnected clients. Active client streams continue to use the provider's configured
streamingIdleTimeoutMs.Validation
bun run test tests/unit/proxy/response-handler-client-abort-drain.test.tsbunx biome check src/app/v1/_lib/proxy/response-handler.ts tests/unit/proxy/response-handler-client-abort-drain.test.tsbun run typecheckGreptile Summary
This PR caps the background drain window to a fixed 60 seconds after a downstream client disconnects, regardless of the provider's configured
streamingIdleTimeoutMs. It also movesstartIdleTimer/clearIdleTimerand thechunksarray to the outer scope so they can be used from the client-abort listener, and fixes a bug where the idle-timeout finalization path incorrectly reportedSTREAM_IDLE_TIMEOUTinstead ofCLIENT_ABORTEDwhen the client had already disconnected before the idle timer fired.clientAbortDrainTimeoutMsis now alwaysCLIENT_ABORT_DRAIN_MAX_MS(60 s) instead of inheriting the provider's potentially largestreamingIdleTimeoutMs.startIdleTimer()is called immediately so a shortstreamingIdleTimeoutMscan provide earlier termination than the 60 s cap.isIdleTimeoutfinalization branch now passes the actualclientAbortedflag and uses"CLIENT_ABORTED"as the abort reason when appropriate, fixing the mislabelled accounting for client-aborted streams that happen to be terminated by the idle timer.Confidence Score: 5/5
Safe to merge. The change only affects already-disconnected clients; active streams are untouched.
The diff is targeted and well-tested. The 60 s cap only applies after client disconnect; active streams are unaffected. The clientAborted flag fix corrects a mislabel, and all four key edge cases are covered by new tests.
No files require special attention.
Important Files Changed
Reviews (6): Last reviewed commit: "fix(proxy): keep abort-drain idle as cli..." | Re-trigger Greptile