Four fixes found bringing a real Hono/drizzle service up under Perry (#8930, #8962, #8968, #9289) - #9311
Closed
proggeramlug wants to merge 4 commits into
Closed
Four fixes found bringing a real Hono/drizzle service up under Perry (#8930, #8962, #8968, #9289)#9311proggeramlug wants to merge 4 commits into
proggeramlug wants to merge 4 commits into
Conversation
…twice (#8962) `import { Hono } from "hono"; new Hono()` compiled and linked, then threw `TypeError: Cannot initialize private elements twice on the same object` during construction. It reduces to two files and no inheritance at all: // base.ts export class BaseX { #m(): number { return 1; } call(): number { return this.#m(); } } // main.ts import { BaseX } from "./base"; new BaseX().call(); The importing module sees the class only as the metadata-only stub `compile_module` synthesizes for an import (`codegen/mod.rs`, "Build a stub Class with the minimum fields the codegen needs"). A stub is a name table: it carries member names so dispatch symbols resolve, and carries no bodies, no initializers and no constructor. Everything construction actually *does* is baked into the defining module's standalone `<prefix>__<class>_constructor` instead — `codegen/method.rs` says so where it emits them, "At the `new ImportedClass(...)` call site, `lower_new` applies initializers against the imported class stub — which has none". That premise held for FIELDS, because the stub flattens every field to `is_private: false` with `init: None`: the worst `apply_field_initializers_ recursive` could do at the `new` site was write `undefined` into a slot the real constructor overwrote moments later. It did not hold for the private BRAND. The stub copies private METHOD and accessor names verbatim, and `has_private_instance_brand` is defined purely over `#`-prefixed member names, so a stub answered `true` and the `new` site emitted `js_private_brand_add` on top of the one the defining module's constructor emits. Installing a class's brand twice on one object is the error PrivateMethodOrAccessorAdd requires, so the runtime threw — correctly, at the second install. Fix: `apply_field_initializers_recursive` skips the private-element decision for a chain entry that is an imported stub. The duplicate check itself is untouched: exactly one `js_private_brand_add` survives, in the defining module's constructor (verified with objdump — the importing module's object now has none, the defining module's still has one). Reached both spellings: the class constructed directly (`new BaseX()`), and the class reached as an ANCESTOR through the `AncestorsOnly` walk, where the leaf is a local subclass. hono hits the second — `class Hono extends HonoBase` with `#path`, `#notFoundHandler`, `#clone`, `#addRoute`, `#dispatch` on the base. Only classes with a private method or accessor were affected; a private field alone never was, since the stub does not mark fields private. Tests: `crates/perry/tests/issue_8962_imported_class_private_brand.rs`. Every case calls the private member after constructing, so a fix that dropped the second install without leaving the first standing fails them too — the brand check throws when no brand is present. Two guard cases pin the boundaries: same-module construction still installs the brand at the `new` site, and a genuine double initialization (a base ctor returning an object the derived class already branded) still throws. Verified: `new Hono()` runs (routing, `route()`, `basePath()`, `fetch`); `cargo test -p perry --bin perry` 1049/1049; `cargo test -p perry-hir -p perry-codegen` all green; mb24's `packages/db/src/migrate.ts` still compiles. Claude-Session: https://claude.ai/code/session_0145yUtx1jiWHf66QEZh6DzY
…dlib (#8930) A `bundled-streams` build died with `undefined reference to <futures_channel::mpsc::SenderTask>::notify` out of `libperry_ext_http.a`, even though the `libperry_stdlib.a` on the same command line exports that symbol from its own `futures_channel` member. The mechanism is archive order, but not the obvious one. The wrapper archives already appear twice — once before perry-stdlib, once after — so at a glance every reference has somewhere to go. What the #8930 link map shows is that the FIRST listing pulls nothing at all: the user objects reference only `js_*` symbols that perry-stdlib provides, so nothing in the wrapper is undefined yet when `ld` walks it. All 92 wrapper members that end up in the executable are pulled from the SECOND listing, on references that stdlib's own members had just opened (`js_node_http_*`, …) — and the references those members carry back INTO stdlib have nowhere to go, because GNU `ld` scans each archive once and never revisits one it has passed. That was harmless while each wrapper bundled its own copy of everything it closed over. It becomes a hard error the moment `strip_bundled_shared_deps_from_well_known_lib` drops a bundled member because stdlib provides it — a correct decision by the archive index, but one that only holds if `ld` can still get back to stdlib. That is what `bundled-streams` changes: it is the feature (enabled by a `fs/promises` or `stream/web` import, per `stdlib_features::module_to_features`) that pulls futures_channel into perry-stdlib's own graph, so stdlib starts bundling a name-matching `futures_channel-*.rcgu.o`, so the wrapper's copy becomes eligible to drop. A stdlib built without it carries no futures_channel member at all, the wrapper keeps its copy, and the link stays self-contained. #8939 tightened the pruning rule to the name-matched stdlib member's own exports; here that member does export the symbol, so the rule fires correctly and the link still fails. The bug is on the link line, not in the pruning. Wrap the perry archive block in `-Wl,--start-group` / `-Wl,--end-group` on ELF targets so `ld` re-scans it to a fixed point — the guarantee a mutually recursive archive set needs. Repeating one archive (the codebase's usual "archive twice" trick) only covers a one-step cycle, and this graph is LTO-partitioned across hundreds of codegen units on both sides. Members are still pulled left to right, so the wrappers-before-stdlib and localized-runtime-last first-definition-wins ordering is unaffected; Mach-O `ld64` resolves archives to a fixed point already (and rejects the flag) and `lld-link` / MSVC have no group concept, so both are left alone. Verified against both reported repros in mb24 — `apps/landing` (hono plus a compiled `@skelpo/cms-client`) and `apps/api` (hono + ws + mysql2, four wrapper archives, no `perry.compilePackages` at all). Both fail before and link after; replaying either final link line with only the two group flags removed reproduces the exact undefined reference. A case that already linked (`packages/db/src/migrate.ts`) produces a byte-identical executable with and without the flags. The link/strip-dedup unit tests pass (68), as do the two archive-ordering integration tests — `issue_5920_wrapper_bundled_runtime_async_starvation` (the two-runtime-copies hazard) and `issue_6715_native_wrapper_precedence`. `native_link_cache` fails identically on the unpatched parent commit.
Lower the read half of private compound assignments through the class-mangled storage key and the normal private brand guard. Parse dynamic HeadersInit values in Response constructors and carry string-body metadata explicitly through the Response ABI so default content types survive without mutable side-channel state.
|
Important Review skippedToo many files! This PR contains 768 files, which is 468 over the limit of 300. To get a review, reduce the PR to 300 files or fewer by splitting it into smaller PRs or changing its base branch. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (768)
You can disable this status message by setting the 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 |
proggeramlug
added a commit
that referenced
this pull request
Aug 31, 2026
#9311) (#9314) * fix(link): group the ELF archive block so ld can resolve back into stdlib (#8930) A `bundled-streams` build died with `undefined reference to <futures_channel::mpsc::SenderTask>::notify` out of `libperry_ext_http.a`, even though the `libperry_stdlib.a` on the same command line exports that symbol from its own `futures_channel` member. The mechanism is archive order, but not the obvious one. The wrapper archives already appear twice — once before perry-stdlib, once after — so at a glance every reference has somewhere to go. What the #8930 link map shows is that the FIRST listing pulls nothing at all: the user objects reference only `js_*` symbols that perry-stdlib provides, so nothing in the wrapper is undefined yet when `ld` walks it. All 92 wrapper members that end up in the executable are pulled from the SECOND listing, on references that stdlib's own members had just opened (`js_node_http_*`, …) — and the references those members carry back INTO stdlib have nowhere to go, because GNU `ld` scans each archive once and never revisits one it has passed. That was harmless while each wrapper bundled its own copy of everything it closed over. It becomes a hard error the moment `strip_bundled_shared_deps_from_well_known_lib` drops a bundled member because stdlib provides it — a correct decision by the archive index, but one that only holds if `ld` can still get back to stdlib. That is what `bundled-streams` changes: it is the feature (enabled by a `fs/promises` or `stream/web` import, per `stdlib_features::module_to_features`) that pulls futures_channel into perry-stdlib's own graph, so stdlib starts bundling a name-matching `futures_channel-*.rcgu.o`, so the wrapper's copy becomes eligible to drop. A stdlib built without it carries no futures_channel member at all, the wrapper keeps its copy, and the link stays self-contained. #8939 tightened the pruning rule to the name-matched stdlib member's own exports; here that member does export the symbol, so the rule fires correctly and the link still fails. The bug is on the link line, not in the pruning. Wrap the perry archive block in `-Wl,--start-group` / `-Wl,--end-group` on ELF targets so `ld` re-scans it to a fixed point — the guarantee a mutually recursive archive set needs. Repeating one archive (the codebase's usual "archive twice" trick) only covers a one-step cycle, and this graph is LTO-partitioned across hundreds of codegen units on both sides. Members are still pulled left to right, so the wrappers-before-stdlib and localized-runtime-last first-definition-wins ordering is unaffected; Mach-O `ld64` resolves archives to a fixed point already (and rejects the flag) and `lld-link` / MSVC have no group concept, so both are left alone. Verified against both reported repros in mb24 — `apps/landing` (hono plus a compiled `@skelpo/cms-client`) and `apps/api` (hono + ws + mysql2, four wrapper archives, no `perry.compilePackages` at all). Both fail before and link after; replaying either final link line with only the two group flags removed reproduces the exact undefined reference. A case that already linked (`packages/db/src/migrate.ts`) produces a byte-identical executable with and without the flags. The link/strip-dedup unit tests pass (68), as do the two archive-ordering integration tests — `issue_5920_wrapper_bundled_runtime_async_starvation` (the two-runtime-copies hazard) and `issue_6715_native_wrapper_precedence`. `native_link_cache` fails identically on the unpatched parent commit. * fix(crypto): honor scrypt cost parameters (#9289) * refactor(link): split the HarmonyOS native-object scan out of build_and_run.rs build_and_run.rs was at 1994 lines; #8930's archive-group call takes it over the 2000-line gate. The HarmonyOS block needs only cmd/target/format, so it moves to a sibling module unchanged. --------- Co-authored-by: Claude <ralph3@skelpo.com> Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Contributor
Author
|
Reviewed all four. Two are already fixed on #8962 (imported class private brand) — already on #8968 (private compound reads / Response headers) — already on #8930 (ELF archive grouping) and #9289 (scrypt cost parameters) — merged via #9314, with your original commits and authorship preserved. I verified the scrypt fix directly rather than taking the table on trust: the parity fixture runs identically under the pinned Node 26.5.1 oracle and the compiled binary, all four sections including the One mechanical note: Closing this, since everything in it has now either landed or been superseded. If you want the broader |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four independent fixes, each with regression tests, found while taking a real TypeScript service (Hono + drizzle + mysql2 + zod + Stripe) from
perry compileto a running binary. Every one was discovered by something failing at runtime, not by reading code.Each commit stands alone and can be reviewed — or reverted — separately.
980b9f267— #8962: an imported class installs its private brand twice+334 / 3 files (284 of them tests)
Minimal trigger, much smaller than the hono case suggested — no inheritance involved:
Two conditions, both necessary: the class declares a private method or accessor (a private field alone never triggers it), and it is constructed from a different module than the one declaring it. Every single-module shape works, which is why this cannot be reproduced by hand in one file.
Cause: the imported-class stub flattens fields to
is_private: falsebut copies private method names verbatim (it needs them to resolve dispatch symbols), andhas_private_instance_brand()is defined purely over#-prefixed method/accessor names. So the stub answerstrue, and the importing module emitsjs_private_brand_addat itsnewon top of the one the defining module's constructor already emits. Harmless for fields — anundefinedwrite the real constructor overwrites — and fatal for the non-idempotent brand.Fix:
Class::is_imported_stub(), and one.filter()in the private-element lookup. Two guard tests pin the boundaries: same-module construction still installs at thenewsite, and a genuine double-init still throws.ace855bd6— #8930: ELF archive block needs grouping+136 / 3 files
The obvious diagnosis is wrong. The wrappers are already emitted twice, before and after stdlib, so on paper every reference has somewhere to go. The
ldmap settles it: 364 stdlib members are pulled, then 92 ext members — the first ext listing pulls nothing at all, because the user objects reference onlyjs_*symbols that stdlib provides. Every ext member arrives from the second listing, on references stdlib had just opened, and the references those members carry back into stdlib have nowhere to go, becauseldscans each archive once.Fix: bracket the archive block in
--start-group/--end-group, gated to ELF targets.Chosen over the codebase's usual "emit the archive twice" trick (which
8837fb7fbpreferred for the GTK4 case, and which also fixes this — I tested it): a repeat only covers a one-step cycle, and this graph is LTO-partitioned across hundreds of CGUs in both directions, so the fixed-point semantics is the actual guarantee.Verified as a no-op where linking already worked: a binary built with and without the group flags is byte-identical (
cmpclean, same 232 dynamic symbols), and adds no measurable link time.362a1a743— #8968: private compound reads, and Response headers+633 / −46, 19 files
Two defects that presented as one symptom: an unmatched hono route returned 200 with an empty body and a custom
app.notFound()handler never ran.A compound or logical assignment to a private member read the wrong slot.
lower_assign_target_to_expraddressed the field by its source spelling (#x) while every other private access uses the mangled storage key. So the read missed, silently:hono's
Contextmemoizes asget res() { return this.#res ||= new Response(null, …) }, so every read ofc.resdiscarded the finalized response — including the 404 the not-found handler had just produced. A matched route never readsc.res(the single-handler fast path returns the handler's response directly), which is exactly why only the miss path looked wrong.c.text()/c.json()/c.html()also lost theirContent-Typeentirely, while a rawnew Response(body, { headers })kept it — soResponse/Headerswere fine and the Context path was not. Through@hono/node-serverthe missing content type became a missing body:c.json()endpoints answeredContent-Length: 0.Fix also replaced a thread-local
Cellside channel (which could be clobbered by nested constructors, or hold stale state if initialization threw) with an explicit constructor ABI argument.328d14814— #9289:crypto.scryptignores every cost parameter+405 / −73, 7 files · the security-relevant one
crypto.scrypt(password, salt, keylen, options)discarded the entireoptionsobject and always computed with node's defaults.05f47c22…33e39503…33e39503…33e39503…e5581239…33e39503…cd4b3b2d…33e39503…One constant digest for every input — node's N=2¹⁴/r=8/p=1 result. The KDF was correct; the options were dropped.
Why this is worse than a wrong answer: it is silent, and it survives an audit. A password hasher records its parameters beside the digest (
scrypt$131072$8$1$…), so the database claims a cost that was never paid, and aneedsRehashcheck keyed on those parameters never fires. A caller asking for the OWASP baseline N=2¹⁷ got N=2¹⁴ — an 8× weaker work factor. It also breaks cross-runtime verification: a hash made under node cannot be verified by the same code compiled with Perry, which is how it was found (an admin account created by a node provisioning script could not log in to the compiled API —node verify → true, binary says no).Fix honours
N/r/p/keylen/maxmemwith node's aliases, enforces OpenSSL's exact128 * r * (N + p + 2)boundary, and — the important part — throws instead of substituting defaults. All eight rows now match node byte-for-byte, including themaxmemRangeError.Verification
Every fix was verified independently of the agent that wrote it, against real application code rather than only its own tests.
cargo test --release -p perry --bin perry: 1,052 passed-p perry-codegen -p perry-hir -p perry-stdlib: greencargo fmt --check, file-size check,git diff --check: cleanKnown pre-existing failure, unrelated:
native_link_cache::native_compile_skips_link_on_identical_second_build— confirmed failing on the unpatched parent commit by stashing.Not included
A fifth bug is still open and unfixed here: #9310 — every mysql2 prepared-statement parameter binds as NULL, which is silent data loss. That one is being worked separately; it is not addressed by this branch.