fix(runtime): host Node-API 9/10 addons and report classes as functions in napi_typeof - #10569
proggeramlug wants to merge 3 commits into
Conversation
… as functions in napi_typeof The Node-API host rejected every addon declaring Node-API 9 or 10 (argon2 0.45, better-sqlite3 13, sharp 0.35) and exported none of the version 9/10 entry points. It now implements the stable surface through version 10: node_api_symbol_for, node_api_create_syntax_error/node_api_throw_syntax_error, node_api_get_module_file_name, node_api_create_property_key_*, node_api_create_external_string_* and node_api_create_buffer_from_arraybuffer. Node's per-module facts (declared version, module file URL) are attributed to the addon whose code runs, and exceptions left pending by async-work completions, TSFN callbacks and finalizers follow Node's uncaught-exception policy instead of leaking into the next call. napi_typeof kept its own tag ladder, which read an INT32-tagged class ref as a number and a class object as an object. It now classifies through the typeof operator's classifier, napi_create_int32/uint32 produce plain doubles so a small integer can no longer read as a class, the number getters reject class refs, and ToObject returns a class ref unchanged.
Every successful Node-API call rebuilt its napi_ok error-info message as a fresh CString, one allocation per call. The status bookkeeping now reuses the stored message while the status and message are unchanged. The native-callback trampoline switches the module attribution inside the environment borrows it already takes instead of borrowing twice more.
proggeramlug
added
the
package-audit
|
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 (22)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe Node-API host now supports version 10, Node-API 9/10 entry points, per-addon module attribution, module-specific callback exception handling, corrected value classification, and expanded runtime and end-to-end validation. ChangesNode-API host versioning and module registration
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Addon
participant NodeAPIHost
participant ModuleRecord
participant Callback
Addon->>NodeAPIHost: load and register addon
NodeAPIHost->>ModuleRecord: record API version and file URL
NodeAPIHost->>Addon: run registration with active module
Addon->>NodeAPIHost: create callback or async operation
NodeAPIHost->>Callback: invoke under originating module
Callback-->>NodeAPIHost: return or raise exception
NodeAPIHost->>ModuleRecord: apply module-specific exception policy
Merge Risk: ⚪ Minimal · up to The Node-API version 10 changes appear merge-ready, with no actionable current-head risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 46.02% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 113 functions across 18 files. (4 skipped: 4 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 |
|
On the red I tried regenerating the catalogs locally ( Filed as #10573 (pin the generator version, regenerate once, consider |
|
Landed via merge train #10610 (v0.5.1594). All source commits preserve authorship; merged main matches the validated train exactly. |
1 participant
Summary
This PR fixes two bugs in the Node-API host (
perry.nativeAddons, #8523). Both block the real argon2, better-sqlite3 and sharp addons from replacing Perry's Rust rewrites.napi_get_versionreports 10.napi_typeofreports compiled class constructors asnapi_number(JStypeofsays "function"), so addons reject classes passed as callbacks (better-sqlite3initialize(SqliteError, …)) #10461:napi_typeofreported compiled class constructors asnapi_number, and class-expression values asnapi_object.Root cause
#10456
initialize_addon(crates/perry-runtime/src/node_api_host/loader.rs:373-381at 7661bc0) rejected anynode_api_module_get_api_version_v1()result aboveNAPI_VERSION = 8(node_api_host/mod.rs:58).#10461
napi_typeof(node_api_host/values.rs:411-447) duplicated its own tag ladder:is_number() || is_int32()came before anything else, so an INT32-tagged class ref (INT32_TAG | class_id) was a number.closure::is_closure_ptrcounted as a function, so a class object (OBJECT_TYPE_CLASS) was an object.typeofoperator's classifier (builtins/arithmetic.rsclassify_value_typeof) already handles both shapes.napi_create_int32/napi_create_uint32returnedINT32_TAG | n, and class ids are small and sequential, so an addon's small integer could already read as a class. In the baseline, JS-sideString(addon.returnsThree())printed[class (anonymous)]andtotal += addon.typeofLoop(...)concatenated function source text; the perf fixture below shows both.Fix
Version 10 surface. Every stable v9 and v10 declaration in Node 26.5.1's
js_native_api.h/node_api.his now exported and implemented, and listed insymbols.txt:node_api_symbol_forSymbol.forregistrynode_api_create_syntax_error,node_api_throw_syntax_errorcodeis installed when givennode_api_get_module_file_namefile://URL of the addon's canonical sidecar pathnode_api_create_property_key_{latin1,utf8,utf16}node_api_create_external_string_{latin1,utf16}copied = trueand the finalizer runs before returning. This is Node's ownNewExternalStringpath when V8 cannot adopt storage.node_api_create_buffer_from_arraybuffernapi_invalid_arg; an out-of-range window throwsERR_OUT_OF_RANGE, like Node 26.Version gate (
node_api_host/modules.rseffective_module_version), using Node's rules:NAPI_VERSION_EXPERIMENTALare rejected before the initializer runs. Experimental entry points are not exported.Version-dependent behavior (audit of Node's
module_api_versionchecks). Node gives each module its ownnapi_env; Perry shares one per agent. Each host-initiated entry into addon code now runs with its module marked active: the initializer, function callbacks, async-work completion, TSFNcall_js/finalize, and object/instance-data finalizers. Each record captures the module when it is created.node_api_get_module_file_nameanswers per addonCallbackIntoModuleuncaught-exception policy<true>). TSFN callbacks report it for version ≥10; modules below 10 getDEP0168and the exception is dropped (Node's<false>). During shutdown it is dropped.napi_create_referenceaccepts any value type (v10 rule)NAPI_PREAMBLEreturnsnapi_cannot_run_jswhen JS can't runnapi_get_value_string_*NAPI_VERSION_EXPERIMENTAL, which is rejecteddocs/src/internals/node-api-host.mdnow documents the version-10 support level, module attribution, and the deviations above.napi_typeofclassify_value_typeof, nowpub(crate). It keeps only thenullandexternalarms that Node-API reports itself.napi_create_int32/uint32create plain doubles.napi_number_expectedfor a class ref.js_object_coercereturns a class ref unchanged, sonapi_coerce_to_objectandObject(C)return the class instead of a boxed id.Per-call overhead (second commit), which pays for the classifier and more:
Env::set_statusrebuilt thenapi_okmessageCStringon every successful call, one allocation per call. It now reuses the stored message while status and message are unchanged.with_env_mutborrows it already takes.Tests
Runtime unit tests (
node_api_host/v10_tests.rs, plus one intests.rs):napi_typeoffor every value kind: undefined, null, boolean, number, string, symbol, bigint, object, array, external, native function, closure, bound function, INT32 class ref, class object.napi_create_int32/uint32stay numbers when a class id with the same value is registered; getters reject class refs; ToObject returns the class.node_api_symbol_forand the syntax-error pair.node_api_create_buffer_from_arraybuffer: shared storage,ERR_OUT_OF_RANGEincluding offset overflow,napi_invalid_arg.Proof the tests can fail. With the old
napi_typeofladder restored and exception settling disabled, 4 of the 9 v10 tests fail with the expected messages:left: Number, right: Function,NumberExpected, and 0 uncaught errors.e2e gate (
crates/perry/tests/node_api_host_e2e.rsnode_api_10_addons_match_node):fixtures/node_api_host/addon_v10.cis built three times: as a version 10 addon, a version 8 addon and a version 11 addon. It declares its own prototypes, so no Node headers are needed.addon_v10_main.jsexercises typeof for 17 value kinds including 4 class shapes, int32 creation,napi_new_instance, every v9/v10 entry point, per-module file names, and async-work/TSFN exception delivery for both versions (uncaughtExceptionandDEP0168).addon_v10_expected.txtbyte for byte; the transcript is Node 26.5.1's output. Whennodeprovides Node-API ≥10, Node must reproduce the transcript too. The version 11 addon must be rejected.undefined symbol: node_api_throw_syntax_error. With the fix, Perry's output is identical to Node's.A gap test was not used: this needs a compiled native addon plus
node_modules, which the gap harness cannot provide.Validation
RUST_TEST_THREADS=1 cargo test --release -p perry-runtime --features node-api-host --lib node_api_hostRUST_TEST_THREADS=1 cargo test --release -p perry-runtime --tests --no-fail-fast(default features)android_tls_pool1 passed. The failure isgc::tests::heap_generation::a_free_or_move_outside_every_scope_is_caught_in_debug_builds, which is pre-existing: its assertion is#[cfg(debug_assertions)], so it cannot pass under--release, and this PR does not touch it.cargo test --release -p perry --test node_api_host_e2ereal_node_api_addon_resolves_from_host_and_authenticates_sidecar, is a host-executable size-budget assert (614,400 B) that this PR does not touch; it is already over budget on baseline 7661bc0 (measured delta 2,125,592 B there vs 2,133,784 B on this branch, +8 KB from the new v9/v10 entry points — both far past the 614,400 B budget)../scripts/run_lint_gates.sh(full, incl. compile tier)main(unrelated to this change; left as-is per repo policy).PERRY_SKIP_BUILD=1 ./scripts/run_gap_tests.sh)GAP_EXIT=0; 813 passed / 6 known failures — identical to the 7661bc0 baseline run, no new failures.Performance
Instruction counts are
perf stat -e instructions:u, 3 runs each, 7661bc0 vs this PR, both in default auto-optimize mode on perrybuilder. The addon runsnapi_typeof100k times in a C loop, called 10× from JS: 1Mnapi_typeofcalls per row. This is the issue's repro scaled into a loop. The "JS→native" row is 1M JS calls into an addon function, which exercises the callback trampoline.napi_typeof(class)×1Mnapi_typeof({})×1Mnapi_typeof(5)×1Mnapi_typeof("text")×1Mnapi_typeof(closure)×1Mnapi_typeof(napi_create_int32(3))×1MFor context, on a loaded shared host:
In the baseline arm,
+=on thenapi_create_int32results concatenated function source text; that is the ambiguity this PR removes.Package checks (informational)
Both run in default auto-optimize mode against the npm prebuilds (
linux-x64, no build from source).argon2.cjsthat requiresprebuilds/linux-x64/argon2.glibc.nodeby a static path, because node-gyp-build's computed require is Runtimerequire()of a.nodepath never reaches the Node-API loader: computed addon paths (node-gyp-build, bindings, node-pty) fail with "Cannot find module", even when the file is in the sidecar #10457. Output is identical to Node: argon2d/argon2i/argon2idhash+verify(correct and wrong password), raw hash hex, a random-salt default hash + verify, and the short-salt error.better-sqlite3/linux-x64): loads and gets pastinitialize(SqliteError, …).execandprepare().run()match Node ({ changes: 1, lastInsertRowid: 1 }). The next blocker is row reads:rowFactorybuilds rows withFunction(...), which throws "new Function() cannot run in an ahead-of-time compiled binary" (Function(...),Function.apply(...)andFunction.call(...)with a runtime body compile to a stub that always throws, while the notice says "→ runtime interpreter" #10422).Not verified
#[cfg(unix)], and only Linux x64 was run.napi_cannot_run_jsduring environment teardown is not modeled (documented).Fixes #10456
Fixes #10461
Summary by CodeRabbit