refactor(native): split bridge modules and merge worker teardown fix#255
Merged
RtlZeroMemory merged 7 commits intomainfrom Mar 6, 2026
Merged
refactor(native): split bridge modules and merge worker teardown fix#255RtlZeroMemory merged 7 commits intomainfrom
RtlZeroMemory merged 7 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughComprehensive refactor of native bindings introducing modular architecture with dedicated modules for FFI definitions, configuration validation, debugging API, and thread-safe engine registry. Adds module pinning for lifetime safety, restructures engine lifecycle management, and exposes new debug and capability query functions to JavaScript. Changes
Sequence DiagramsequenceDiagram
participant JS as JavaScript
participant NAPI as N-API Binding
participant REG as Engine Registry
participant CFG as Config Validator
participant DBG as Debug Module
participant FFI as C FFI Layer
rect rgba(100, 200, 255, 0.5)
Note over JS,FFI: Engine Creation Flow
JS->>NAPI: engineCreate(config)
NAPI->>CFG: apply_create_cfg_strict(config)
CFG->>CFG: validate_known_keys + extract fields
CFG->>FFI: zr_engine_config_default + apply values
NAPI->>FFI: engine_create(config)
FFI-->>NAPI: *mut zr_engine_t
NAPI->>REG: register_engine(engine_ptr)
REG->>REG: allocate engine_id, create EngineSlot
REG-->>NAPI: engine_id
NAPI-->>JS: engine_id
end
rect rgba(200, 100, 255, 0.5)
Note over JS,FFI: Debug Query Flow
JS->>NAPI: engineDebugQuery(engine_id, query)
NAPI->>REG: get_engine_guard(engine_id)
REG->>REG: validate ownership, increment active_calls
REG-->>NAPI: EngineGuard
NAPI->>DBG: apply_debug_query(query)
DBG->>DBG: parse_debug_query fields
NAPI->>FFI: engine_debug_query(config)
FFI-->>NAPI: zr_debug_query_result_t
NAPI->>DBG: convert to DebugQueryResult (BigInt conversion)
DBG-->>NAPI: DebugQueryResult
NAPI-->>JS: DebugQueryResult
Note over REG: EngineGuard drops, active_calls--
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/node/src/backend/nodeBackend.ts (1)
356-369:⚠️ Potential issue | 🟠 MajorAuto mode can now pick an unrunnable worker path in headless environments.
With
hasAnyTty/nativeShimModuleno longer considered here,executionMode: "auto"frequently resolves to"worker"and then fails at startup viaassertWorkerEnvironmentSupported, turning mode selection into a deferred runtime error.🔧 Proposed fix
export function selectNodeBackendExecutionMode( input: NodeBackendExecutionModeSelectionInput, ): NodeBackendExecutionModeSelection { - const { requestedExecutionMode, fpsCap } = input; + const { requestedExecutionMode, fpsCap, hasAnyTty, nativeShimModule } = input; const resolvedExecutionMode: "worker" | "inline" = requestedExecutionMode === "inline" ? "inline" : requestedExecutionMode === "worker" ? "worker" : fpsCap <= 30 ? "inline" : "worker"; + + if ( + requestedExecutionMode === "auto" && + resolvedExecutionMode === "worker" && + nativeShimModule === undefined && + !hasAnyTty + ) { + return { + resolvedExecutionMode, + selectedExecutionMode: "inline", + fallbackReason: "worker-requires-tty", + }; + } + return { resolvedExecutionMode, selectedExecutionMode: resolvedExecutionMode, fallbackReason: null, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/node/src/backend/nodeBackend.ts` around lines 356 - 369, When resolving executionMode for the "auto" path, the current logic can pick "worker" even in headless environments; change the resolution so that when requestedExecutionMode is neither "inline" nor "worker" you only choose "worker" if the runtime supports workers (check the same runtime flags used by assertWorkerEnvironmentSupported, e.g., hasAnyTty and nativeShimModule or an existing isWorkerSupported helper); otherwise pick "inline" and set fallbackReason accordingly. Update the block that computes resolvedExecutionMode/selectedExecutionMode (the variables requestedExecutionMode, fpsCap, resolvedExecutionMode) to include this environment support check so auto never defers to an unrunnable worker path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/native/src/config.rs`:
- Around line 235-237: The code currently masks a parsed u32 into a u8
(js_u32(...) -> dst.requested_color_mode = (v & 0xFF) as u8) which silently
accepts out-of-range values; change this to validate the parsed value before
assignment: after obtaining v from js_u32 for
"requestedColorMode"/"requested_color_mode", check that v <= u8::MAX and return
a proper parse/validation error if it exceeds that range, otherwise cast to u8
and assign to dst.requested_color_mode; reference the js_u32 call and
dst.requested_color_mode field to locate where to add the range check and error
return.
- Around line 185-195: The js_obj function currently calls coerce_to_object
which boxes primitives; change it to reject non-object JS values by checking
v.get_type()?==ValueType::Object and only then calling v.coerce_to_object(); if
the type is not Object, skip/continue (so primitives and other types are not
accepted as valid objects for strict parsing of limits/plat). Ensure this check
is applied in the js_obj function before attempting coerce_to_object for both
primary and alias lookup paths.
In `@packages/native/src/debug.rs`:
- Around line 74-80: The Number-to-u64 conversion in the ValueType::Number
branch currently accepts fractional values and unsafe JS integers; update the
logic in the ValueType::Number handling (around value.coerce_to_number(),
number.get_double(), and the float → u64 return) to first verify the double is
an exact integer (no fractional part) and within the JavaScript safe integer
range (<= 9007199254740991 and >= 0). If the value is out of the safe range,
reject here and require using the BigInt path (e.g., handle ValueType::BigInt or
coerce_to_bigint()) instead; if the double is non-finite, negative, fractional,
or > JS_SAFE_INTEGER, return Err(()) rather than truncating, otherwise safely
cast to u64.
In `@packages/native/src/lib.rs`:
- Around line 386-404: enginePostUserEvent currently calls
ffi::engine_post_user_event without the is_owner_thread() guard other mutating
entry points use; either restore the same thread-owner enforcement or add
explicit documentation why cross-thread calls are safe. Update
engine_post_user_event/enginePostUserEvent to call is_owner_thread() (same
pattern used by engineSubmitDrawlist, enginePresent, engineSetConfig) and return
the same error code when not owner, or add a clear doc comment on
engine_post_user_event and the FFI declaration (ffi::engine_post_user_event)
explaining thread-safety guarantees and why get_engine_guard plus calling from
other threads is safe, referencing these symbols so the rationale is
discoverable.
In `@packages/native/src/registry.rs`:
- Around line 39-50: The wait_for_idle() method uses active_calls_mu,
active_calls_cv and the active_calls AtomicUsize, but notifications are
currently sent without holding active_calls_mu which can cause a lost-wakeup;
update the Drop implementation that decrements active_calls so it first locks
active_calls_mu (using the same Mutex used by wait_for_idle), decrement
active_calls while holding that guard, then call
active_calls_cv.notify_all()/notify_one() while still holding the guard (or
immediately after releasing if your CV requires), ensuring the decrement and
notify are performed under the same mutex to satisfy the condition-variable
contract.
---
Outside diff comments:
In `@packages/node/src/backend/nodeBackend.ts`:
- Around line 356-369: When resolving executionMode for the "auto" path, the
current logic can pick "worker" even in headless environments; change the
resolution so that when requestedExecutionMode is neither "inline" nor "worker"
you only choose "worker" if the runtime supports workers (check the same runtime
flags used by assertWorkerEnvironmentSupported, e.g., hasAnyTty and
nativeShimModule or an existing isWorkerSupported helper); otherwise pick
"inline" and set fallbackReason accordingly. Update the block that computes
resolvedExecutionMode/selectedExecutionMode (the variables
requestedExecutionMode, fpsCap, resolvedExecutionMode) to include this
environment support check so auto never defers to an unrunnable worker path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3805425a-5400-4f05-9915-255499b049b1
📒 Files selected for processing (14)
packages/native/build.rspackages/native/index.d.tspackages/native/loader.cjspackages/native/scripts/smoke-worker.mjspackages/native/scripts/smoke.mjspackages/native/src/config.rspackages/native/src/debug.rspackages/native/src/ffi.rspackages/native/src/lib.rspackages/native/src/registry.rspackages/native/src/tests.rspackages/node/src/__tests__/config_guards.test.tspackages/node/src/__tests__/worker_integration.test.tspackages/node/src/backend/nodeBackend.ts
💤 Files with no reviewable changes (1)
- packages/native/loader.cjs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/native/src/config.rs (1)
238-240:⚠️ Potential issue | 🟠 MajorDo not silently truncate
requestedColorMode.Line 239 masks with
& 0xFF, which converts out-of-range inputs into unrelated modes. This should reject invalid ranges instead.💡 Proposed fix
fn apply_plat(dst: &mut ffi::plat_config_t, obj: &JsObject) -> ParseResult<()> { if let Some(v) = js_u32(obj, "requestedColorMode", "requested_color_mode")? { - dst.requested_color_mode = (v & 0xFF) as u8; + if v > u8::MAX as u32 { + return Err(()); + } + dst.requested_color_mode = v as u8; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/native/src/config.rs` around lines 238 - 240, The code is silently truncating out-of-range `requestedColorMode` values by masking with `& 0xFF` instead of rejecting them; replace the mask with an explicit range check after reading `v` (from `js_u32(obj, "requestedColorMode", "requested_color_mode")?`) and return an error if `v` > u8::MAX, otherwise assign `dst.requested_color_mode = v as u8`. Ensure the function returns a descriptive error (not silently adjusting the value) so callers know the input was invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/native/scripts/smoke.mjs`:
- Around line 75-97: The assertWorkerLoadExitCleanly function can hang because
worker.once("exit", ...) is added only after the message promise resolves;
register the exit listener earlier or capture exit code inside the initial
Promise that waits for the "message" so the exit event can't be missed. Update
the Promise that creates loadResult (in assertWorkerLoadExitCleanly) to also
listen for "exit" (and store/resolve the exit code or reject if non-zero) or
attach the worker.once("exit", ...) before awaiting loadResult, and ensure all
listeners (onExit, onError, onMessage) are removed consistently when any of them
fires.
In `@packages/native/src/config.rs`:
- Around line 137-152: The js_u32 function currently allows non-number types to
be coerced (e.g., strings/bools/null); update js_u32 to mirror the strict
pattern used by js_u8_bool and js_obj by first checking v.get_type() and
rejecting any type other than ValueType::Undefined or ValueType::Number (i.e.,
continue on Undefined, return Err(()) on any other type), then safely call
v.coerce_to_number() / get_double() and validate the finite, non-negative,
integer, <= u32::MAX constraints before returning Some(f as u32); keep the
existing alias/primary loop and error mapping behavior.
---
Duplicate comments:
In `@packages/native/src/config.rs`:
- Around line 238-240: The code is silently truncating out-of-range
`requestedColorMode` values by masking with `& 0xFF` instead of rejecting them;
replace the mask with an explicit range check after reading `v` (from
`js_u32(obj, "requestedColorMode", "requested_color_mode")?`) and return an
error if `v` > u8::MAX, otherwise assign `dst.requested_color_mode = v as u8`.
Ensure the function returns a descriptive error (not silently adjusting the
value) so callers know the input was invalid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66b4c434-1f65-40df-82cb-be5ad0094a62
📒 Files selected for processing (4)
packages/native/scripts/smoke.mjspackages/native/src/config.rspackages/native/src/lib.rspackages/native/src/registry.rs
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.
Summary
packages/native/src/lib.rsValidation
npm run buildnpm run lintcargo testnpm run build:nativenpm run test:native:smokenode scripts/run-tests.mjs --filter "config_guards|worker_integration"Summary by CodeRabbit
New Features
Refactor
Tests