Skip to content

fix(http): bound legacy response headers - #1879

Open
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/pin-legacy-first-byte-deadline
Open

fix(http): bound legacy response headers#1879
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/pin-legacy-first-byte-deadline

Conversation

@luvs01

@luvs01 luvs01 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add an absolute request-to-response-headers deadline for legacy positive idleTimeoutMs calls while retaining the existing socket inactivity timeout.
  • Preserve legacy idleTimeoutMs: 0 as the documented disabled-timeout behavior.
  • Close the AbortSignal window between the initial cancellation check and listener installation.

Exact base: 02da6cc9099db02873d6a38894af9b73d11f9f18
Exact head: 3d06485eefc2cebdc9388361c254accd58cbb109

Why

ClientRequest.setTimeout() is reset by socket activity. A peer can therefore keep an incomplete HTTP response header alive by dripping bytes, holding a pinned provider or image-download connection beyond the intended legacy timeout.

The legacy deadline starts when the request is created so it also works with Bun 1.3.14's fetch-backed node:http implementation, whose socket event is synthetic and not proof that TCP/TLS completed. The explicit connectTimeoutMs / firstByteTimeoutMs / inactivityTimeoutMs path is unchanged.

Compatibility and safety

  • Positive legacy timeouts now fail incomplete headers with PinnedHttpError.code === "first_byte_timeout" and still retain request/response inactivity handling.
  • Legacy zero skips the owned first-byte timer and continues to pass 0 to the Node-compatible request/response timers, so disabled behavior is preserved.
  • Host, SNI, certificate verification, pinned DNS lookup, headers, credentials, redirects, and body byte limits are unchanged.
  • A cancellation that lands during listener installation is rechecked before req.end() and preserves the original abort reason.

Verification

  • Bun 1.3.14 focused transport tests: 12 pass, 0 fail, 27 assertions.
  • Bun 1.4.0-canary.1 focused transport tests: 12 pass, 0 fail, 27 assertions.
  • bun run typecheck and bun run privacy:scan: pass on both runtimes.
  • git diff --check: pass.
  • Independent correctness and test-contract reviews: CLEAN, no P0-P2 findings.
  • Codex Security diff scan 3c4f63ee-0922-4015-aa99-304da5451ee9: 0 findings, complete changed-source coverage.
  • Full repository CI is not claimed; maintained exact-head CI remains required.

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 HTTP request timeout behavior when idle timeouts are disabled.
    • Ensured first-byte timeouts start consistently across request modes.
    • Improved handling of requests that are already canceled or aborted.
  • Tests

    • Added coverage for response-header timeouts, disabled timers, and cancellation behavior.

@coderabbitai

coderabbitai Bot commented Aug 17, 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: b4ae6835-6c96-434b-942e-e43cdeabf415

📥 Commits

Reviewing files that changed from the base of the PR and between 02da6cc and 3d06485.

📒 Files selected for processing (2)
  • src/lib/pinned-http.ts
  • tests/pinned-http.test.ts

Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Legacy pinned HTTP requests now disable the first-byte timer when idleTimeoutMs is 0, start the legacy timer after request creation, and handle abort signals during listener installation. New TCP tests cover timeout and cancellation behavior.

Changes

Pinned HTTP timeout and cancellation

Layer / File(s) Summary
Legacy first-byte timeout behavior
src/lib/pinned-http.ts, tests/pinned-http.test.ts
The legacy path detects idleTimeoutMs: 0, skips the first-byte timer when disabled, and starts the timer after request creation. Tests cover partial headers, disabled timers, and delayed response bodies.
Abort listener lifecycle
src/lib/pinned-http.ts, tests/pinned-http.test.ts
The request registers the abort listener after event wiring, handles signals aborted during registration, and avoids sending a settled request. Tests verify cancellation and reason propagation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: 🟡 Moderate · up to 3d064

The PR adds a legacy HTTP response-header deadline and closes a cancellation race, but it is not merge-ready because required exact-head CI has not been claimed and the readiness checklist is still incomplete. Keep it unmerged until those checks pass and the remaining checklist items are completed.

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: bounding legacy HTTP response-header timeouts.
✨ 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.

@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Aug 17, 2026
@github-actions

github-actions Bot commented Aug 17, 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

luvs01 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

luvs01 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Maintainer CI approval requested for exact head 3d06485eefc2cebdc9388361c254accd58cbb109.

Both runs are currently completed/action_required with zero jobs because this is a fork approval gate, not a test failure. Focused transport tests passed on Bun 1.3.14 and Bun 1.4.0-canary.1; typecheck and privacy scan passed on both; CodeRabbit completed the exact-head review with no actionable comments. The PR remains Draft pending maintained CI.

@github-actions
github-actions Bot marked this pull request as ready for review August 17, 2026 03:52
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.

1 participant