Repository navigation
fix: keep the terminal class reference per Node environment - #23
Conversation
TerminalWrap kept its class in one process-wide static Napi::FunctionReference. Every environment that loads the addon (the main thread and each worker_threads Worker) ran TerminalWrap::Init and overwrote it. createTerminal in one thread then used another thread's reference: V8 aborted with "Cannot create a handle without a HandleScope", or the process segfaulted once that worker had exited. Store the reference in the environment's instance data instead, so each environment creates terminals from its own class. Node deletes it when the environment shuts down. test/workers.test.ts runs three worker scenarios in child processes. All three crashed before this change and pass after it. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ Change-Id: I9b923f58ec18c7f246bf37a0c1b3ab0856a550f3 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security 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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Each load of the addon gets its own napi_env, so the instance-data slot is always empty when TerminalWrap::Init runs: assert that instead of deleting a previous reference (which ran before the new reference existed). Two process.dlopen calls of the addon in one env still work, in the main thread and in a Worker. test/workers.test.ts no longer requires an empty stderr (NODE_OPTIONS can print there) and shows it as the failure message. Each child run is bounded at 20 s and each test at 30 s. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ Change-Id: Id7d371f6bbbaa47fd6f2b3cf7d411a695d3d3d27 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
What breaks today
Any process that loads this package in more than one Node environment crashes. That includes the main thread plus a
worker_threadsWorker, or two Workers. The crash happens on the nextcreateTerminalin an environment whose class reference was overwritten:FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope(SIGABRT).Cause
TerminalWrapkept its class in one process-widestatic Napi::FunctionReference constructor. Node-API addons are loaded once per environment, so each environment ranTerminalWrap::Initand overwrote the static:Initstores the main thread's reference.constructor = Napi::Persistent(func)deletes the main thread's reference through the main thread'snapi_env, from the worker thread, and stores the worker's reference.createTerminal.constructor.Newopens a handle scope in the worker's environment and crashes.Fix
TerminalWrap::Initnow stores the reference in the environment's instance data (env.SetInstanceData), andTerminalWrap::Createreads it withenv.GetInstanceData. Each environment creates terminals from its own class, and Node deletes the reference when that environment shuts down. This is the pattern that the node-addon-apiObjectWrapdocs use.Tests
test/workers.test.tsruns three scenarios, each in a child process so that a crash cannot kill the test runner:Before the fix all three failed (SIGABRT, SIGSEGV, SIGABRT). After the fix all three pass.
Validation (local, linux-x64, Node v26.12.1, zig 0.15.2)
npm run build:native,npm run build,npm run typecheck,npm test(9/9 passed),npm run smoke: all exit 0.test/workers.test.tsrun 10 more times: 10/10 passed.verify-libghostty-vt-nodeskill (top PR of this stack). That includes the worker-thread journey, which crashed before this fix.Found by the TLA+ model in the next PR of this stack (
verify-native-lifecycle).Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high