Skip to content

Keep Oxide loaded on Windows after Node worker exit - #20471

Open
bompus wants to merge 1 commit into
tailwindlabs:mainfrom
bompus:codex/oxide-windows-unload
Open

Keep Oxide loaded on Windows after Node worker exit#20471
bompus wants to merge 1 commit into
tailwindlabs:mainfrom
bompus:codex/oxide-windows-unload

Conversation

@bompus

@bompus bompus commented Sep 8, 2026

Copy link
Copy Markdown

Summary

Fixes #20470.

Scanning with Oxide only inside a Node worker can crash the Windows process with 0xC0000005 when 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-oxide succeeded for both upstream f723e83 and this patch. Each DLL was copied to a separate .node file for comparison.
  • Original isolated worker reproduction, 20 sequential pairs with reversed arm order on alternate pairs: unmodified 18 passes / 2 access violations; patched 20 passes / 0 access violations.
  • Executed the exact start.js and worker.js fixture 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.rs passed. 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 --locked passed the preceding unit suites but failed eight scanner symlink tests (63 scanner tests passed). cargo test --locked -p tailwindcss-oxide --test scanner on an untouched checkout at f723e83 reproduced the same eight failures. No scanner code is changed here.
  • Full JavaScript/integration suites and non-Windows execution remain unverified locally. Opening as a draft for the retention-policy review and CI validation. [ci-all]

Existing scanner failures on both checkouts:

test_changes_to_the_canonical_path_of_a_symlinked_file_are_detected
test_files_behind_symlinks_are_tracked_with_their_canonical_paths
test_glob_with_symlinks
test_globs_with_recursive_symlinks
test_new_files_behind_symlinks_are_detected_at_their_canonical_path
test_partial_globs_with_symlinks
test_symlinked_sources_outside_the_scanned_tree_can_be_ignored
test_symlinked_sources_within_the_scanned_tree_can_be_ignored

@greptile-apps

greptile-apps Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

The 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

@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 68102a74-f56e-4a97-af28-32f67899681c

📥 Commits

Reviewing files that changed from the base of the PR and between f723e83 and 64a8dcd.

📒 Files selected for processing (2)
  • crates/node/src/lib.rs
  • integrations/oxide/workers.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

Added 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 flex candidate, assert successful worker exit, confirm the native module remains mapped on Windows, and verify parent-process survival.

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 64a8d

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)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: retaining the Oxide native library on Windows after a Node worker exits.
Description check ✅ Passed The description directly explains the Windows access violation, the library-retention implementation, its tradeoff, and the regression test changes.
Linked Issues check ✅ Passed The changes satisfy issue #20470 by pinning the Windows native library during Node-API registration, returning an initialization error if pinning fails, excluding non-Windows builds, and adding worker…
Out of Scope Changes check ✅ Passed The native hook and worker integration test changes are directly related to issue #20470. No unrelated code, dependency, or JavaScript export changes are included.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Oxide 4.3.3: Windows access violation after scanning in a Node worker and unloading the addon

1 participant