fix(http): bound legacy response headers - #1879
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review. 📝 WalkthroughWalkthroughLegacy pinned HTTP requests now disable the first-byte timer when ChangesPinned HTTP timeout and cancellation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to 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)
✅ Passed checks (4 passed)
✨ 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 |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Maintainer CI approval requested for exact head
Both runs are currently |
Summary
idleTimeoutMscalls while retaining the existing socket inactivity timeout.idleTimeoutMs: 0as the documented disabled-timeout behavior.Exact base:
02da6cc9099db02873d6a38894af9b73d11f9f18Exact head:
3d06485eefc2cebdc9388361c254accd58cbb109Why
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:httpimplementation, whosesocketevent is synthetic and not proof that TCP/TLS completed. The explicitconnectTimeoutMs/firstByteTimeoutMs/inactivityTimeoutMspath is unchanged.Compatibility and safety
PinnedHttpError.code === "first_byte_timeout"and still retain request/response inactivity handling.0to the Node-compatible request/response timers, so disabled behavior is preserved.req.end()and preserves the original abort reason.Verification
bun run typecheckandbun run privacy:scan: pass on both runtimes.git diff --check: pass.3c4f63ee-0922-4015-aa99-304da5451ee9: 0 findings, complete changed-source coverage.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Summary by CodeRabbit
Bug Fixes
Tests