Skip to content

fix(fastify): forward dynamic request/reply property reads in external-fastify variant builds (#5037)#5157

Merged
proggeramlug merged 2 commits into
mainfrom
fix/5037-fastify-request-props
Jun 15, 2026
Merged

fix(fastify): forward dynamic request/reply property reads in external-fastify variant builds (#5037)#5157
proggeramlug merged 2 commits into
mainfrom
fix/5037-fastify-request-props

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #5037 — a fastify request object passed into a plain helper function reads request.headers === undefined (→ fastify 500), but only in auto-optimize variant builds. The same source against the prebuilt release libs works.

Root cause

The compiler binary is identical between prebuilt and auto-optimize modes — only the linked runtime/ext static libs differ by cargo feature flags. For a program importing fastify, auto-optimize triggers the well-known-bindings flip: import 'fastify' is routed to perry-ext-fastify, bundled-fastify/http-server are stripped, and external-fastify-pump is activated (see optimized_libs.rs and well_known_bindings.toml).

The bundled FastifyContext property-dispatch arm in js_handle_property_dispatch (crates/perry-stdlib/src/common/dispatch.rs) is gated on #[cfg(feature = "http-server")], so in the flipped build it is compiled out. When a request/reply handle escapes into a user helper, its static type is erased and codegen emits a generic dynamic property read here rather than a NativeMethodCall — and there was no external-fastify dispatch arm to handle it, so the read fell through to undefined. Inline reads in the handler still worked because codegen recognised the receiver and called js_fastify_req_* directly.

perry-ext-fastify already exports the membership probe js_ext_fastify_is_context_handle precisely for this path (its doc comment claims "perry-stdlib's js_handle_property_dispatch arms call this") — but the stdlib arm was never wired up, unlike the equivalent external-zlib and external-net arms.

Fix

Add the missing external-fastify property-dispatch arm, gated on all(feature = "external-fastify-pump", not(feature = "http-server")) (the same gate the external-fastify-pump pump uses in async_bridge.rs). It probes js_ext_fastify_is_context_handle and forwards query/params/body/rawBody/text/headers/method/url/user to the same js_fastify_req_* exports the bundled arm uses. The extern symbols resolve at link time from perry-ext-fastify, mirroring the established external-zlib-pump / external-net-pump probe-and-forward arms.

Validation

  • cargo check -p perry-stdlib (default config — new arm compiled out under http-server): clean.
  • cargo check -p perry-stdlib --no-default-features --features external-fastify-pump (the flipped/auto-optimize config — new arm compiled in): clean. This config is not exercised by CI, so it was verified locally.

Note: macOS host builds don't reproduce the original bug because their auto-optimize variant set keeps the bundled fastify path (no flip); the linux worker pipeline does flip. No version bump / changelog per maintainer (folded in at squash-merge).

Summary by CodeRabbit

  • New Features
    • Improved Fastify request/reply property dispatch for the external-fastify integration, including better support for common fields like query, params, body/raw body, headers, method, URL, and user data.
  • Chores / Refactor
    • Reformatted Android linker and NDK clang executable name construction for the Windows host case, preserving the same output behavior.

…l-fastify variant builds

When the well-known flip routes `import 'fastify'` to perry-ext-fastify
(auto-optimize / `--no-default-features` builds), `bundled-fastify` and
`http-server` are stripped, so the bundled FastifyContext property-dispatch
arm in `js_handle_property_dispatch` is compiled out. A `request`/`reply`
handle that escaped into a user helper function has its static type erased,
so codegen emits a generic dynamic property read here rather than a
`NativeMethodCall` — and with no external-fastify dispatch arm wired up, the
read fell through to `undefined`. Inline reads in the handler still worked
because codegen recognised the receiver and called `js_fastify_req_*`
directly.

The request handle lives in perry-ext-fastify's perry-ffi registry, so probe
membership via the external `js_ext_fastify_is_context_handle` symbol
(resolved at link time) and forward to the same `js_fastify_req_*` exports
the bundled arm uses. Mirrors the existing external-zlib / external-net
probe-and-forward arms and the `external-fastify-pump` pump in
async_bridge.rs.

Fixes #5037
@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3e20be30-a2cd-4e8c-ae05-b43b5270c4e8

📥 Commits

Reviewing files that changed from the base of the PR and between a953a04 and 4dd0910.

📒 Files selected for processing (2)
  • crates/perry/src/commands/compile/link/mod.rs
  • crates/perry/src/commands/compile/link/platform_cmd.rs
✅ Files skipped from review due to trivial changes (2)
  • crates/perry/src/commands/compile/link/mod.rs
  • crates/perry/src/commands/compile/link/platform_cmd.rs

📝 Walkthrough

Walkthrough

A new external-fastify-pump-gated branch is added to js_handle_property_dispatch that routes request/reply property reads through external symbols to preserve object properties across function call boundaries. Two Android NDK clang path construction sites reformat their Windows conditional suffix from single-line to multi-line without functional change.

Changes

External-fastify handle property dispatch

Layer / File(s) Summary
external-fastify-pump property dispatch arm
crates/perry-stdlib/src/common/dispatch.rs
Adds 69 lines inside js_handle_property_dispatch: an external-fastify-pump (non-http-server) feature gate that probes handle membership via js_ext_fastify_is_context_handle, then dispatches each supported request/reply property name to the matching js_fastify_req_* external function, guards string-pointer returns against null by emitting undefined, and returns undefined as the fallback for unknown property names.

Android NDK clang path formatting

Layer / File(s) Summary
Multi-line Windows conditional formatting
crates/perry/src/commands/compile/link/mod.rs, crates/perry/src/commands/compile/link/platform_cmd.rs
Reformats the Windows host-tag conditional suffix (.cmd appended on Windows, empty otherwise) used in Android NDK clang executable path construction from single-line cfg! macros into multi-line if/else expressions across both linker configuration files, preserving behavior and improving readability.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • PerryTS/perry#5154: Contains matching Android NDK clang path construction reformatting changes in the same two linker configuration files for Windows host toolchain invocation.

Poem

🐇 Hop-hop through the dispatch gate,
A feature flag seals each handle's fate.
request.headers once went astray—
Across a function call, lost on the way.
Now ext-fastify probes the context right,
And every property returns to light! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Out of Scope Changes check ❓ Inconclusive The main fix to dispatch.rs directly addresses #5037, but secondary formatting changes to link/mod.rs and platform_cmd.rs appear to be pre-existing rustfmt-dirty issues rather than necessary for the core fix. Clarify whether the formatting changes to link/mod.rs and platform_cmd.rs are necessary for the PR or unrelated style cleanup that should be separated.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the primary change: adding external-fastify variant support for dynamic request/reply property reads, addressing the core issue #5037.
Description check ✅ Passed The description comprehensively covers the summary, root cause analysis, the fix, and validation results, aligning with the repository's template requirements.
Linked Issues check ✅ Passed The PR fully addresses issue #5037 by implementing the missing external-fastify property-dispatch arm to forward dynamic property reads, resolving the undefined property issue in auto-optimize variant builds.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/5037-fastify-request-props

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

`main` is currently red on the `Check formatting` lint step: the
admin-merged #5154 left the `if cfg!(target_os = "windows") { ".cmd" }
else { "" }` argument in `link/mod.rs` and `platform_cmd.rs` in a form
rustfmt rejects. Apply `cargo fmt` so this PR's lint gate can pass; no
behaviour change.
@proggeramlug

Copy link
Copy Markdown
Contributor Author

End-to-end verification (auto-optimize variant, reproduced on macOS host)

The well-known flip does trigger on this host once the prebuilt ext lib isn't short-circuited — the compiler logs the same variant as the issue:

well-known: routing `fastify` → rebuilding `perry-ext-fastify` ...
auto-optimize: rebuilding runtime+stdlib (features=async-runtime,crypto,external-fastify-pump)
auto-optimize: built .../libperry_runtime.a / libperry_stdlib.a / libperry_ext_fastify.a

Ran the issue's exact repro against that binary:

build /direct /helper
without this fix (origin/main stdlib) {"auth":"Bearer abc"} {"statusCode":500,..."Cannot read properties of undefined (reading 'authorization')"}
with this fix {"auth":"Bearer abc"} {"auth":"Bearer abc"}

The negative control rebuilt only libperry_stdlib.a (the changed crate), confirming the arm is exactly what was missing.

Note on the second commit

The style: commit just runs cargo fmt over the two #5154 Android-NDK .cmd lines that main currently leaves rustfmt-dirty — without it this PR's lint gate stays red on a pre-existing main issue unrelated to the fix. No behaviour change.

@proggeramlug proggeramlug merged commit 816798d into main Jun 15, 2026
15 checks passed
@proggeramlug proggeramlug deleted the fix/5037-fastify-request-props branch June 15, 2026 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

auto-optimize variant fastify: request properties lost across function-call boundary (request.headers undefined in helper functions)

1 participant