Repository navigation
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
wasm-bindgen initializes finalization registries before its WASM instance. Replacing the current refusal with an empty registry would hide lost Rust and closure cleanup, and running callbacks from native weak-disposal hooks would enter user code during teardown.
This draft records a focused ownership proposal for vercel-labs#1678 and adds three regression checks preserving the TypeScript diagnostic and JavaScript runtime fence. It does not implement FinalizationRegistry or close the issue. Internal review only; no upstream PR is authorized.
The proposal keeps the generated module, WASM instance, targets, held values, unregister tokens, and closures inside a Node island, with a copied JSON adapter for narrow operations. It distinguishes initialization from finalization, documents strong and weak edges, token semantics, collector-safe cleanup jobs, ProgramLifetime implications, and acceptance work. It builds on the existing libnode PRs instead of duplicating them. vercel-labs#1686 was merged into the loader branch, not main; the required loader, packs, and hosted-loop work remain outstanding.
Validation on upstream main eca0cab, in an isolated worktree:
ok; the current compiler still reports SC2020 for the type, constructor, and register call.--expose-gcobservation: one collectible target-as-token callback ran after the GC call stack returned; no callback ran for the retained target, the held-value-to-target cycle, or an unregistered target. Callback count/deadline was not a pass condition. This is a Node observation, not native collector or Rust-destructor evidence.abcunder Node 24.22.0. This does not demonstrate native execution or full client compatibility.SCRIPTC_SAN=1configurations on Node 24.22.0. These frontend-only tests do not exercise ASan or a native collector.pnpm -r build,pnpm lint,pnpm format:check,pnpm test:ts7 --baselines-only, andgit diff --checkpass.The issue remains unresolved. No failing-base/passing-native-patch result exists, because this draft intentionally leaves native behavior unchanged. Full native/sanitizer suites and zero-cost validation belong to an eventual implementation; they are not claimed by this proposal. Downloaded consumer artifacts and experimental scripts are excluded.
Review base:
review-base/issue-1678-eca0cab8apoints to the verified upstream main commit. Fork main is older; using a dedicated base keeps this internal PR limited to the two issue-specific files without changing fork main.Compiled follow-up experiment, outside this PR
A separate worktree at existing upstream hosted-loop commit
eaefbdf44compiled a native ELF caller of@scriptc/node.callSyncwith AddressSanitizer and the runtime reference-count audit. It uses the branch-pinned, checksum-verified prebuilt libnode 24.22.1 and its bridge. No Node source build or FinalizationRegistry implementation was added.The native executable loaded the exact published consumer inside Node, returned SHA256 of
abc, ran the reduced registry-before-WASM and explicit-disposal experiment, and copied an invalid-byteRangeErrorback to native code. Its stdout matched the Node 24.22.0 oracle. The hosted pack is 24.22.1, as pinned by the experimental branch; this is a narrow smoke comparison, not a same-version full parity claim:The direct host run exited 0. ASan emitted its existing makecontext/swapcontext support warning; it was not suppressed. The sandboxed run could not complete LeakSanitizer's ptrace-based inspection, so the same binary was rerun on the authorized host without changing security settings or disabling leak checks. This proves a narrow compiled caller plus in-process Node island route. It does not prove native lowering of the package/WASM/registry, Rust client lifecycle correctness, Baileyrs compatibility, or a passing full native/sanitizer suite.
The concrete native-language blockers are (1) ProgramLifetime instances have no collector header and empty retain/release helpers, so last-release observer hooks cannot discover their unreachable lifetime; (2) cycle free functions run while
scr_collectingis true and dead members have been marked DOOMED, so arbitrary cleanup cannot run inline; (3) generated Rust closures use internal_wbg_cb_unrefplus their own registry, not the public.free()method on class wrappers. A.free()-only convention does not establish complete ownership for this package.The published consumer reports
getWasmAllocationSnapshot().enabled === false; its zero allocation counters are not usable as evidence that Rust resources were reclaimed. A memory-profiled consumer build or another destructor-level observation is required for a stateful ownership claim.The next shippable step is a stacked adapter/integration change on the existing Node-island stack, with an explicit JSON boundary and all resources remaining in Node. Shipping native FinalizationRegistry instead requires a separate language/collector decision, including how to handle ProgramLifetime and queued cleanup roots. This main-based draft intentionally does not import an API absent from main or duplicate the pending island infrastructure. Its scope remains partial and the issue remains open.
Tested adapter stack prepared for this PR
The working adapter and executable ownership tests are now published separately for focused review while the current head's CI finishes: 13-file comparison, head
0931187d2. This is not yet this PR's head, and no second PR was opened.The new stack passed 12/12 focused tests in both plain and sanitizer configurations, covering release/dev, the exact consumer's public ESM export, counted-WASM class/closure ownership, explicit disposal/unregister after errors, interleaved async/sync calls, and callback/handle boundary refusals. The CLI example also runs with
--libnode=beside. It remains a partial Node-owned compatibility path, not native FinalizationRegistry or complete client support.After the existing run reaches a terminal state, this PR's review base/head will move to that focused stack. No run is being cancelled to make that update.
The original CI run is now terminal with failure: 50 jobs succeeded, while plain shard 8/20 ended in failure after its test step was marked cancelled, and the aggregate gate failed. The failed job log was unavailable from GitHub, so its cancellation cause is unverified. This front sent no cancellation. Upstream vercel-labs#1696 has since advanced to
049b34c1; the adapter stack is being revalidated against that existing fix before this PR's head/base update. The current head is still the original proposal; the focused stack comparison remains available above.