Skip to content

Fix ServerResponse.call subclass construction and socket reads - #11729

Open
proggeramlug wants to merge 3 commits into
mainfrom
fix/11725-server-response-call
Open

proggeramlug wants to merge 3 commits into
mainfrom
fix/11725-server-response-call

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

http.ServerResponse.call(this, req) was folded into a free namespace function call that discarded this. After preserving the receiver, inline-cache reads still bypassed its native-handle alias, leaving socket and connection undefined and breaking Fastify's light-my-request.

Preserve ServerResponse.call/apply for 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:

  • Full HIR unit suite: 550 passed, one ignored; the new lowering regression fails on the baseline and passes with the fix.
  • Compiled CommonJS and TypeScript execution regressions: two tests passed, covering six .call/.apply cases and checking socket identity, connection.once, and headers.
  • Formatting, file-size, Node-version consistency, test registration, GC root-holder, address-classification, and whitespace checks passed.
  • Runtime inline-cache unit regression passed: pre-construction cached miss, changing native state, sibling isolation, and own-property precedence.
  • Rebuilt compiler and runtime/stdlib/HTTP/net/WS static wrappers together. The compiled execution tests were built directly with Rust's test harness against that compiler.

Fastify smoke limitation: fastify/inject.ts 3 0 no longer throws the reported connection.once exception, but exits after the benchmark header without printing a checksum. Node 26.5.1 prints 6c0b6520. 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

  • Bug Fixes
    • Fixed http.ServerResponse.call() and .apply() so explicitly supplied receivers are preserved across supported import styles and invocation forms.
    • Corrected native-backed property reads after construction, including socket and header access. These reads now reflect the current receiver’s values without affecting sibling objects or overriding its own properties.

@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: a4481b51-9912-4e3c-994b-2a81d799cb0b

📥 Commits

Reviewing files that changed from the base of the PR and between 8fe7895 and 776c22c.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 06c88a04-8fb3-4e50-8343-e5dfd65ddd23

📥 Commits

Reviewing files that changed from the base of the PR and between d368f62 and 8fe7895.

📒 Files selected for processing (8)
  • changelog.d/11729-server-response-explicit-this.md
  • crates/perry-hir/src/lower/expr_call/intrinsics/apply_call.rs
  • crates/perry-hir/src/lower/expr_call/mod.rs
  • crates/perry-hir/src/lower/expr_call/server_response_call_tests.rs
  • crates/perry-runtime/src/object/field_get_set/ic_miss.rs
  • crates/perry-runtime/src/object/native_this_alias.rs
  • crates/perry-runtime/src/object/native_this_alias/tests.rs
  • crates/perry/tests/issue_11725_server_response_call.rs

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The change preserves explicit this in http.ServerResponse.call and .apply calls. The runtime roots and aliases the receiver, routes aliased property reads through native getters, and adds regression coverage for lowering and execution.

Changes

ServerResponse call and apply

Layer / File(s) Summary
Preserve the receiver during lowering
crates/perry-hir/src/lower/expr_call/intrinsics/apply_call.rs, crates/perry-hir/src/lower/expr_call/mod.rs, crates/perry-hir/src/lower/expr_call/server_response_call_tests.rs
http.ServerResponse.call/apply calls retain reflective lowering and explicit this. Tests cover HTTP import forms and verify that path.join.call keeps its intrinsic lowering.
Register aliases and route native reads
crates/perry-runtime/src/object/native_this_alias.rs, crates/perry-runtime/src/object/field_get_set/ic_miss.rs, crates/perry-runtime/src/object/native_this_alias/tests.rs
The constructor path roots the receiver before native dispatch and alias registration. Aliased receivers are marked for exotic reads, and qualifying cache misses use the native getter. Tests check updated native values, sibling receivers, and own-property precedence.
Exercise ServerResponse constructor calls
crates/perry/tests/issue_11725_server_response_call.rs, changelog.d/11729-server-response-explicit-this.md
End-to-end tests cover call/apply forms in CommonJS and TypeScript, including socket, event-listener, and header behavior. The changelog records the changes.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: claude

Merge Risk: ⚪ Minimal · up to 8fe78

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 Review

Security architecture risk: 🟡 Moderate · up to 8fe78

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

  • Medium · security · inferred: Restored namespace ServerResponse.call/apply construction broadens access to an existing permanently rooted receiver table. Distinct receivers append entries whose garbage-collection roots are not coupled to response completion or destruction. Repeated subclass construction can therefore accumulate receiver objects and increase process-level resource-exhaustion risk. Same-receiver deduplication and socket destruction do not reclaim distinct receiver roots. External request reachability is not established.
Security review details

Security Blast Radius

  • inferred — The supported availability exposure is accumulation of receiver roots in an affected long-lived runtime thread, potentially exhausting its process. An application would need to exercise the restored subclass construction repeatedly. The inspected execution tests demonstrate local construction, not an internet-facing caller, tenant boundary, or deployment-wide attack path.

Security Findings and Attack Paths

  • inferred — If untrusted workload frequency drives creation of distinct subclass receivers, restored namespace construction can feed an append-only rooted table and cause resource exhaustion. This is an inferred availability concern involving broader access to a pre-existing ownership policy, not a verified remote exploit. Recycled-handle access remains unresolved and is not asserted as a finding.

Trust Boundaries and Controls

  • observed — The construction boundary obtains the forwarding handle from the registered native constructor and binds it to a validated explicit receiver. Exact-address lookup, receiver rooting, and exotic-read classification protect identity and stale-cache behavior. These controls do not establish terminal ownership cleanup or handle-generation protection.

Resilience and Maintainability Implications

  • observed — Repeated registration for the same receiver updates its entry rather than duplicating it. Response destroy marks the response destroyed and closes an associated connection. Neither operation releases roots belonging to distinct receiver entries; native resource shutdown and JavaScript receiver ownership are separate lifecycle mechanisms in the inspected code.

Hardening Proposals

  • proposed — Define alias ownership so unreachable receivers can be reclaimed while still-reachable response objects preserve their documented post-completion behavior. Validate repeated distinct constructions through end and destroy with garbage collection, rather than removing aliases solely because a response has completed.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary fix: preserving ServerResponse.call subclass construction and restoring socket reads.
Description check ✅ Passed The description explains the cause, implementation, linked issue, validation results, and the remaining Fastify smoke-test limitation. It does not follow every template heading exactly, but it provide…
Linked Issues check ✅ Passed The changes address [#11725]. http.ServerResponse.call and .apply preserve explicit this for CommonJS and TypeScript paths. Native-backed receiver reads use the alias getter, and construction ro…
Out of Scope Changes check ✅ Passed The changes stay within [#11725]. The HIR lowering change, native alias read handling, cache invalidation, receiver rooting, and related regression tests support correct ServerResponse subclass cons…
Full details: Docstring Coverage

Explanation

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 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

@proggeramlug proggeramlug added the run-extended-tests Opt PR into compile-smoke/parity/doc-tests/drizzle-mysql-smoke label Oct 1, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-extended-tests Opt PR into compile-smoke/parity/doc-tests/drizzle-mysql-smoke

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CJS http.ServerResponse subclass (ServerResponse.call + util.inherits): assignSocket is a no-op (breaks fastify inject)

1 participant