Fix ServerResponse.call subclass construction and socket reads - #11729
proggeramlug wants to merge 3 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change preserves explicit ChangesServerResponse call and apply
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change preserves explicit receivers for ServerResponse.call and .apply and fixes the reported socket and connection behavior. Regression tests cover the main cases. Full Fastify benchmark parity is not yet confirmed, but no concrete merge-blocking risk was found. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The fix restores expected response behavior, but newly working subclass construction can retain response objects after they are no longer needed. Repeated creation may exhaust memory in long-running applications. Whether an externally reachable application exercises this path remains unproven. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 69.23% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 7 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
http.ServerResponse.call(this, req)was folded into a free namespace function call that discardedthis. After preserving the receiver, inline-cache reads still bypassed its native-handle alias, leavingsocketandconnectionundefined and breaking Fastify'slight-my-request.Preserve
ServerResponse.call/applyfor the existing runtime construction hook. Route property reads on native-backed receivers through the alias getter, invalidate pre-construction caches using the existing exotic-read classification, and root the receiver across construction. Other namespace free functions retain their direct-call lowering.Fixes #11725. No version bump.
Validation:
.call/.applycases and checking socket identity,connection.once, and headers.Fastify smoke limitation:
fastify/inject.ts 3 0no longer throws the reportedconnection.onceexception, but exits after the benchmark header without printing a checksum. Node 26.5.1 prints6c0b6520. Full Fastify benchmark parity is therefore not established by this PR; the reported ServerResponse subclass behavior is covered by the passing execution regressions.Summary by CodeRabbit
http.ServerResponse.call()and.apply()so explicitly supplied receivers are preserved across supported import styles and invocation forms.