From 4aa7039266d96613583fff03051f2eca8375b779 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 29 Oct 2026 13:04:19 +0000 Subject: [PATCH 1/2] fix: keep the terminal class reference per Node environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- native/terminal.cc | 15 ++++-- native/terminal.hh | 2 - test/workers.test.ts | 112 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 7 deletions(-) create mode 100644 test/workers.test.ts diff --git a/native/terminal.cc b/native/terminal.cc index fc68458..3a049ce 100644 --- a/native/terminal.cc +++ b/native/terminal.cc @@ -42,8 +42,6 @@ void ThrowResult(Napi::Env env, const char* operation, GhosttyResult result) { } // namespace -Napi::FunctionReference TerminalWrap::constructor; - void TerminalWrap::Init(Napi::Env env, Napi::Object exports) { Napi::Function func = DefineClass( env, @@ -58,13 +56,20 @@ void TerminalWrap::Init(Napi::Env env, Napi::Object exports) { InstanceMethod("dispose", &TerminalWrap::Dispose), }); - constructor = Napi::Persistent(func); - constructor.SuppressDestruct(); + // Each Node environment (the main thread and every worker_threads Worker) + // runs Init and gets its own class. Keep the class reference in this + // environment's instance data, never in a static: a reference belongs to + // the environment that created it. Node deletes the instance data when the + // environment shuts down. + delete env.GetInstanceData(); + env.SetInstanceData(new Napi::FunctionReference(Napi::Persistent(func))); exports.Set("createTerminal", Napi::Function::New(env, CreateTerminal)); } Napi::Object TerminalWrap::Create(Napi::Env env, const Napi::Object& options) { - return constructor.New({options}); + Napi::FunctionReference* constructor = env.GetInstanceData(); + assert(constructor != nullptr && "TerminalWrap::Init must run in this environment first"); + return constructor->New({options}); } TerminalWrap::TerminalWrap(const Napi::CallbackInfo& info) diff --git a/native/terminal.hh b/native/terminal.hh index bb74a3f..bdf8606 100644 --- a/native/terminal.hh +++ b/native/terminal.hh @@ -20,8 +20,6 @@ class TerminalWrap final : public Napi::ObjectWrap { ~TerminalWrap() override; private: - static Napi::FunctionReference constructor; - void DisposeNative(); GhosttyTerminal RequireTerminal(Napi::Env env); Napi::Value Feed(const Napi::CallbackInfo& info); diff --git a/test/workers.test.ts b/test/workers.test.ts new file mode 100644 index 0000000..7b37a7c --- /dev/null +++ b/test/workers.test.ts @@ -0,0 +1,112 @@ +import { spawnSync } from "node:child_process"; +import { dirname, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; +import { describe, expect, it } from "vitest"; +import { nativeSupport } from "./native-support.js"; + +const support = nativeSupport(); +const describeIfNative = support.available ? describe : describe.skip; +const root = resolve(dirname(fileURLToPath(import.meta.url)), ".."); + +// Each scenario runs in a child process: a broken addon aborts or segfaults the +// whole process, which would otherwise kill the test runner. +const prelude = ` +const { Worker } = require("node:worker_threads"); +const { once } = require("node:events"); +const addon = require("node-gyp-build")(process.env.ADDON_ROOT); +function use(label) { + const t = addon.createTerminal({ cols: 10, rows: 1 }); + t.feed(label); + const text = t.getVisibleText(); + t.dispose(); + if (text !== label) throw new Error("expected " + label + ", got " + JSON.stringify(text)); +} +const workerSource = \` + const { parentPort } = require("node:worker_threads"); + const addon = require("node-gyp-build")(process.env.ADDON_ROOT); + function use(label) { + const t = addon.createTerminal({ cols: 10, rows: 1 }); + t.feed(label); + const text = t.getVisibleText(); + t.dispose(); + if (text !== label) throw new Error("expected " + label + ", got " + JSON.stringify(text)); + } + use("w-load"); + parentPort.postMessage("loaded"); + parentPort.on("message", (msg) => { + if (msg === "exit") { parentPort.close(); return; } + use(msg); + parentPort.postMessage("used " + msg); + }); +\`; +async function startWorker() { + const w = new Worker(workerSource, { eval: true }); + const [msg] = await once(w, "message"); + if (msg !== "loaded") throw new Error("worker did not load: " + msg); + return w; +} +async function workerUses(w, label) { + w.postMessage(label); + const [msg] = await once(w, "message"); + if (msg !== "used " + label) throw new Error("worker failed: " + msg); +} +async function stopWorker(w) { + w.postMessage("exit"); + await once(w, "exit"); +} +`; + +function runScenario(body: string) { + const script = `${prelude}\n(async () => { ${body}\n console.log("done"); })().catch((err) => { console.error(err); process.exit(1); });`; + return spawnSync(process.execPath, ["-e", script], { + cwd: root, + encoding: "utf8", + env: { ...process.env, ADDON_ROOT: root }, + timeout: 30_000, + }); +} + +function expectSurvived(result: ReturnType) { + expect({ signal: result.signal, status: result.status, stderr: result.stderr }).toEqual({ + signal: null, + status: 0, + stderr: "", + }); + expect(result.stdout.trim()).toBe("done"); +} + +describeIfNative("worker_threads", () => { + it("creates terminals in the main thread after a worker loads the addon", () => { + expectSurvived( + runScenario(` + use("main-1"); + const w = await startWorker(); + use("main-2"); + await workerUses(w, "w-2"); + await stopWorker(w);`), + ); + }); + + it("creates terminals in the main thread after a worker that loaded the addon exits", () => { + expectSurvived( + runScenario(` + use("main-1"); + const w = await startWorker(); + await stopWorker(w); + use("main-2");`), + ); + }); + + it("creates terminals in each of two workers", () => { + expectSurvived( + runScenario(` + const w1 = await startWorker(); + const w2 = await startWorker(); + await workerUses(w1, "w1-2"); + await workerUses(w2, "w2-2"); + await stopWorker(w1); + await workerUses(w2, "w2-3"); + await stopWorker(w2);`), + ); + }); +}); From 0d255369dd8233e3ed1bce0575ab98b2bbd98d07 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 29 Oct 2026 13:20:20 +0000 Subject: [PATCH 2/2] fix: assert a fresh instance-data slot, bound the worker test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- native/terminal.cc | 14 +++++++------- test/workers.test.ts | 25 +++++++++++++++---------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/native/terminal.cc b/native/terminal.cc index 3a049ce..f32a491 100644 --- a/native/terminal.cc +++ b/native/terminal.cc @@ -56,19 +56,19 @@ void TerminalWrap::Init(Napi::Env env, Napi::Object exports) { InstanceMethod("dispose", &TerminalWrap::Dispose), }); - // Each Node environment (the main thread and every worker_threads Worker) - // runs Init and gets its own class. Keep the class reference in this - // environment's instance data, never in a static: a reference belongs to - // the environment that created it. Node deletes the instance data when the - // environment shuts down. - delete env.GetInstanceData(); + // Node runs Init once for each load of the addon: once in the main thread + // and once in every worker_threads Worker that loads it. Each load has its + // own napi_env, so keep the class reference in that env's instance data, + // never in a static: a reference belongs to the env that created it. Node + // deletes the instance data when the env shuts down. + assert(env.GetInstanceData() == nullptr); env.SetInstanceData(new Napi::FunctionReference(Napi::Persistent(func))); exports.Set("createTerminal", Napi::Function::New(env, CreateTerminal)); } Napi::Object TerminalWrap::Create(Napi::Env env, const Napi::Object& options) { Napi::FunctionReference* constructor = env.GetInstanceData(); - assert(constructor != nullptr && "TerminalWrap::Init must run in this environment first"); + assert(constructor != nullptr && "TerminalWrap::Init runs before createTerminal is exported"); return constructor->New({options}); } diff --git a/test/workers.test.ts b/test/workers.test.ts index 7b37a7c..87ffa9c 100644 --- a/test/workers.test.ts +++ b/test/workers.test.ts @@ -56,23 +56,28 @@ async function stopWorker(w) { } `; +// The child gets a bounded run, and the test waits longer than that, so a +// hung child fails with its own output instead of a Vitest timeout. +const CHILD_TIMEOUT_MS = 20_000; +const TEST_TIMEOUT_MS = 30_000; + function runScenario(body: string) { const script = `${prelude}\n(async () => { ${body}\n console.log("done"); })().catch((err) => { console.error(err); process.exit(1); });`; return spawnSync(process.execPath, ["-e", script], { cwd: root, encoding: "utf8", env: { ...process.env, ADDON_ROOT: root }, - timeout: 30_000, + timeout: CHILD_TIMEOUT_MS, }); } function expectSurvived(result: ReturnType) { - expect({ signal: result.signal, status: result.status, stderr: result.stderr }).toEqual({ - signal: null, - status: 0, - stderr: "", - }); - expect(result.stdout.trim()).toBe("done"); + // stderr is not asserted (NODE_OPTIONS such as --inspect can print to it), + // but it is the failure message: it holds the V8 fatal error or the stack. + expect( + { signal: result.signal, status: result.status, stdout: result.stdout.trim() }, + result.stderr, + ).toEqual({ signal: null, status: 0, stdout: "done" }); } describeIfNative("worker_threads", () => { @@ -85,7 +90,7 @@ describeIfNative("worker_threads", () => { await workerUses(w, "w-2"); await stopWorker(w);`), ); - }); + }, TEST_TIMEOUT_MS); it("creates terminals in the main thread after a worker that loaded the addon exits", () => { expectSurvived( @@ -95,7 +100,7 @@ describeIfNative("worker_threads", () => { await stopWorker(w); use("main-2");`), ); - }); + }, TEST_TIMEOUT_MS); it("creates terminals in each of two workers", () => { expectSurvived( @@ -108,5 +113,5 @@ describeIfNative("worker_threads", () => { await workerUses(w2, "w2-3"); await stopWorker(w2);`), ); - }); + }, TEST_TIMEOUT_MS); });