Keep Oxide loaded on Windows after Node worker exit - #20471
Conversation
Confidence Score: 5/5The PR appears safe to merge, with no actionable correctness, security, or repository-rule issues identified. The retention hook is scoped to Windows, uses the documented module-address pinning mechanism, reports initialization failure explicitly, and is covered by a worker integration scenario that exercises the affected lifecycle. Reviews (1): Last reviewed commit: "fix(oxide): retain Windows addon after w..." | Re-trigger Greptile |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughAdded a Windows-only NAPI export that pins the native Oxide module for the process lifetime and reports OS errors when pinning fails. Updated the worker integration test to scan HTML files, verify the Fixed issue severity: Priority: ➖ Normal — Schedule the Windows native-library retention fix because it addresses intermittent access-violation crashes after Node workers using Oxide exit. Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This change keeps the Windows Oxide native library mapped after worker exit, preventing unloaded-code crashes while preserving normal worker scanning and process exit behavior. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
Summary
Fixes #20470.
Scanning with Oxide only inside a Node worker can crash the Windows process with
0xC0000005when the worker exits. The captured original build failure executed an address in the unloaded Oxide addon. A standalone reproduction is available at https://github.com/bompus/oxide-worker-unload-repro.During Windows Node-API module registration, retain the containing library with
GetModuleHandleExW(FROM_ADDRESS | PIN). This prevents worker teardown from unmapping native code while Rayon threads may still use it. A failed pin returns an initialization error. Registration runs after library loading, rather than from a DLL load-time constructor. Non-Windows builds exclude the hook, and no dependency or JavaScript export is added.Tradeoff for maintainer review: the native library intentionally remains mapped until process termination, including in long-lived Node processes. This is a targeted retention fix, not a thread-pool shutdown implementation. The dump proves unloaded-code execution; it does not identify the precise native thread/function responsible. Related thread-pool lifetime discussion: rayon-rs/rayon#1101 and rayon-rs/rayon#1242.
Extend the existing worker integration test to scan content, check worker success, and keep the parent alive briefly. On Windows, also assert that the native library is still present in
process.report.getReport().sharedObjects. This assertion detects unloading even when the access violation does not reproduce on that particular run.Test plan
Validated locally on Windows 11 Pro build 26200, Node 24.13.1 x64, Rust 1.95.0, with ESET enabled and no parent-retention preload:
cargo build --locked --release -p tailwind-oxidesucceeded for both upstreamf723e83and this patch. Each DLL was copied to a separate.nodefile for comparison.start.jsandworker.jsfixture bodies from the changed integration test against those binaries: unmodified failed the library-retention assertion 20/20; patched passed 20/20, including expected output. These fixture runs were direct Node executions, not the full packaged Vitest integration harness.rustfmt --check --edition 2021 --config-path crates/node/rustfmt.toml crates/node/src/lib.rspassed. Prettier 3.9.6 checked the changed TypeScript file with the repository's basic formatting settings; the embedded-template/import plugins were not run.cargo test --lockedpassed the preceding unit suites but failed eight scanner symlink tests (63 scanner tests passed).cargo test --locked -p tailwindcss-oxide --test scanneron an untouched checkout atf723e83reproduced the same eight failures. No scanner code is changed here.Existing scanner failures on both checkouts: