Skip to content

fix(server): bound Responses body-read timeout - #1675

Closed
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/bound-responses-body-timeout
Closed

luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/bound-responses-body-timeout

Conversation

@luvs01

@luvs01 luvs01 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Keep Bun's inbound request timeout active while a native /v1/responses body is still arriving, and disable it only after the complete body is accepted for dispatch.
  • Preserve the existing long-running Responses behavior after ingestion while making the timeout transition one-shot across combo children and policy-fallback retries.
  • Classify a client-aborted pending body as the existing 499 client_cancelled response instead of a generic malformed-request response.
  • Add deterministic regressions for partial-body gating, abort handling, combo failover, and policy fallback.

Verification

  • Base: dev at c6688c79ff58ca4f4a6502f6e6a4228124b45047; exact head: e26cbb415b70d2ee372d10103a6585024e557673.
  • Bun 1.3.14: bun test --isolate tests/server-auth.test.ts — 78 pass.
  • Bun 1.3.14: bun test --isolate tests/routing-policy-fallback.test.ts — 5 pass.
  • Bun 1.4.0-canary.1: focused body/abort/combo tests — 3 pass; policy-fallback tests — 5 pass.
  • bun run typecheck passed on both runtimes.
  • bun run privacy:scan and git diff --check passed.
  • The Bun 1.3.14 full repository suite was not green: Windows effective-account lookup returned an empty value and caused unrelated catalog/history/config test cascades, then Bun crashed with an internal index-out-of-bounds assertion after 1,286 seconds. No changed-scope test failed.
  • A Codex Security diff scan found no reportable security finding; its callback follow-ups were addressed. Final independent code and test reviews found no remaining P0-P2 issue.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. No configuration or user-facing command was added; comments and tests describe the corrected request lifecycle.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. Independent security and correctness reviews found no remaining P0-P2 issue.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes
    • Improved request handling so body acceptance is reported only after the complete request is read.
    • Aborted requests now return the appropriate client-cancelled response without being marked as accepted.
    • Prevented duplicate request acceptance during retries and fallback processing.
  • Tests
    • Added coverage for aborted requests, delayed body acceptance, and multi-attempt failover scenarios.

@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added bug Something isn't working review-ready labels Aug 14, 2026
@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers: @lidge-jun @Ingwannu @Wibias

@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f65e5760-b206-4afa-8262-3fc3b5173518

📥 Commits

Reviewing files that changed from the base of the PR and between c6688c7 and e26cbb4.

📒 Files selected for processing (5)
  • src/server/index.ts
  • src/server/responses/core.ts
  • src/server/responses/policy-fallback.ts
  • tests/routing-policy-fallback.test.ts
  • tests/server-auth.test.ts

📝 Walkthrough

Walkthrough

The Responses handler now reports request-body acceptance after complete parsing. Aborted bodies return HTTP 499 without acceptance. Combo and policy-fallback executions deduplicate acceptance notifications.

Changes

Responses body acceptance

Layer / File(s) Summary
Body read contract and dispatch
src/server/responses/core.ts
HandleResponsesOptions accepts onRequestBodyRead. The callback runs after successful body parsing and validation. Aborted body reads return HTTP 499. Synthetic combo requests do not repeat the callback.
Route timeout integration
src/server/index.ts
The Responses POST route no longer disables the timeout at entry. It disables the timeout through onRequestBodyRead after the body is accepted.
Retry notification deduplication and validation
src/server/responses/policy-fallback.ts, tests/routing-policy-fallback.test.ts, tests/server-auth.test.ts
Policy fallback shares callback-wrapped core options across initial and retry executions. Tests cover incomplete bodies, aborted bodies, combo failover, and single notification across retries.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: ⚪ Minimal · up to e26cb

The PR bounds request-body reads and preserves cancellation handling without a supplied indication of a concrete current-head defect; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ResponsesRoute
  participant handleResponses
  participant PolicyFallback
  participant UpstreamTargets

  Client->>ResponsesRoute: POST request body
  ResponsesRoute->>handleResponses: provide onRequestBodyRead
  handleResponses->>handleResponses: read and validate complete body
  handleResponses-->>ResponsesRoute: invoke body-read callback
  ResponsesRoute->>ResponsesRoute: disable request timeout
  handleResponses->>PolicyFallback: dispatch request
  PolicyFallback->>UpstreamTargets: execute initial attempt
  PolicyFallback->>UpstreamTargets: execute fallback retry
  PolicyFallback-->>ResponsesRoute: return final response
Loading

Possibly related PRs

Suggested reviewers: wibias, ingwannu, lidge-jun

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(server): bound Responses body-read timeout' directly corresponds to the main change: bounding the body-read timeout for /v1/responses requests by keeping Bun's inbound request timeout active until the complete body is accepted.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Comment @coderabbitai help to get the list of available commands.

@lidge-jun lidge-jun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Repository bug audit · 2026-08-14]

The lifecycle change is coherent: keep Bun's inbound timeout active until the complete request body is accepted, make the transition one-shot across combo/policy retries, and classify an aborted pending body as the existing 499 cancellation path. The focused tests cover partial-body, abort, combo, and fallback behavior.

This PR is currently not mergeable against dev. Rebase, resolve the server/request-path overlap without moving the callback earlier than successful body parsing, then rerun server-auth, routing-policy-fallback, typecheck, and exact-head Cross-platform CI.

@lidge-jun

Copy link
Copy Markdown
Owner

Cherry-picked onto dev as part of the bug resolution campaign (commit-and-merge loop). Changes verified with typecheck and focused tests.

@lidge-jun lidge-jun closed this Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants