From 8200c908cf2dd675c0c7bc48c0339a8595a29702 Mon Sep 17 00:00:00 2001 From: Arne Luenser Date: Wed, 19 Aug 2026 20:54:13 +0200 Subject: [PATCH] fix: forward termination signals to the CLI binary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The npm launcher spawned the Ory CLI binary with spawnSync and installed no signal handlers, so a SIGINT/SIGTERM/SIGHUP delivered to the launcher ended the launcher alone and orphaned the binary. `ory tunnel` and `ory proxy` create a temporary Ory Network API key and delete it in their graceful shutdown handler; an orphaned process never runs that handler, so the key lingered until its expiry and the running proxy kept serving. Spawn the binary asynchronously and forward SIGINT, SIGTERM and SIGHUP to it, then exit with the child's status — or, when the child was killed by a signal, re-raise that signal on the launcher so callers observe the same termination as before. This restores the behaviour callers had when they ran the binary directly. Add npm/run.test.js (node:test) covering argument passthrough, exit-status propagation, signal forwarding, signal-based termination, and the missing-binary error, wired into CI as the test-launcher job. The test file is excluded from the published package by the existing `files` allowlist. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011uZabxw2Kb2u9v1ALCXqcA --- .github/workflows/ci.yaml | 10 +++ npm/run.js | 39 +++++++--- npm/run.test.js | 157 ++++++++++++++++++++++++++++++++++++++ package.json | 3 +- 4 files changed, 198 insertions(+), 11 deletions(-) create mode 100644 npm/run.test.js diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 8c054349..5be1eae0 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -11,6 +11,16 @@ on: - cron: "0 0 * * *" jobs: + test-launcher: + name: Test npm launcher + runs-on: ubuntu-latest + steps: + - uses: ory/ci/checkout@master + - uses: actions/setup-node@v4 + with: + node-version: "22" + - run: npm run test:launcher + test: name: Run tests runs-on: ubuntu-latest diff --git a/npm/run.js b/npm/run.js index abf78dbd..2a2f196b 100755 --- a/npm/run.js +++ b/npm/run.js @@ -8,7 +8,7 @@ "use strict" -var spawnSync = require("child_process").spawnSync +var spawn = require("child_process").spawn var packages = { "darwin arm64": "@ory/cli-darwin-arm64", @@ -45,13 +45,32 @@ function binaryPath() { } } -var result = spawnSync(binaryPath(), process.argv.slice(2), { - stdio: "inherit", +// Termination signals sent to the launcher are forwarded to the binary rather +// than ending the launcher alone: `ory tunnel` and `ory proxy` delete their +// temporary API key on shutdown, which an orphaned binary never gets to do. +var forwardedSignals = ["SIGINT", "SIGTERM", "SIGHUP"] + +var child = spawn(binaryPath(), process.argv.slice(2), { stdio: "inherit" }) + +forwardedSignals.forEach(function (signal) { + process.on(signal, function () { + child.kill(signal) + }) +}) + +child.on("error", function (err) { + console.error("Unable to start the Ory CLI binary: " + err.message) + process.exit(1) +}) + +child.on("exit", function (code, signal) { + if (signal) { + // Re-raise with the default disposition restored so the launcher is + // terminated by the same signal as the binary. + forwardedSignals.forEach(function (s) { + process.removeAllListeners(s) + }) + process.kill(process.pid, signal) + } + process.exit(typeof code === "number" ? code : 1) }) -if (result.error) { - throw result.error -} -if (result.signal) { - process.kill(process.pid, result.signal) -} -process.exit(typeof result.status === "number" ? result.status : 1) diff --git a/npm/run.test.js b/npm/run.test.js new file mode 100644 index 00000000..8be4efed --- /dev/null +++ b/npm/run.test.js @@ -0,0 +1,157 @@ +// Copyright © 2026 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + +"use strict" + +const { test } = require("node:test") +const assert = require("node:assert/strict") +const { spawn } = require("node:child_process") +const fs = require("node:fs") +const os = require("node:os") +const path = require("node:path") + +const platformPackage = "@ory/cli-" + process.platform + "-" + process.arch + +// The binary stand-in is a Node script: it exits with a requested status, +// echoes its arguments, waits for a signal and records which one arrived, or +// terminates itself with SIGTERM. In wait mode it gives up after ten seconds so +// that a launcher which orphans it cannot hold the test's pipes open forever. +const fakeBinary = `#!/usr/bin/env node +"use strict" +const fs = require("node:fs") +const [mode, arg] = process.argv.slice(2) +switch (mode) { + case "echo": + console.log(process.argv.slice(3).join(" ")) + break + case "exit": + process.exit(Number(arg)) + break + case "wait": + for (const signal of ["SIGINT", "SIGTERM", "SIGHUP"]) { + process.on(signal, () => { + fs.writeFileSync(arg, signal) + process.exit(0) + }) + } + fs.writeFileSync(arg, "ready") + setTimeout(() => process.exit(2), 10000) + break + case "self-kill": + process.kill(process.pid, "SIGTERM") + break +} +` + +// Lays out @ory/cli and the platform package the way npm installs them, so the +// launcher resolves the stand-in through the same require.resolve call it uses +// for the real binary. +function install(t, withBinary) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "ory-cli-launcher-")) + t.after(() => fs.rmSync(root, { recursive: true, force: true })) + + const launcherDir = path.join(root, "node_modules", "@ory", "cli", "npm") + fs.mkdirSync(launcherDir, { recursive: true }) + fs.copyFileSync( + path.join(__dirname, "run.js"), + path.join(launcherDir, "run.js"), + ) + + if (withBinary) { + const binDir = path.join(root, "node_modules", platformPackage, "bin") + fs.mkdirSync(binDir, { recursive: true }) + fs.writeFileSync(path.join(binDir, "ory"), fakeBinary, { mode: 0o755 }) + } + + return { root, launcher: path.join(launcherDir, "run.js") } +} + +function run(launcher, args) { + const child = spawn(process.execPath, [launcher, ...args], { + stdio: ["ignore", "pipe", "pipe"], + env: { + ...process.env, + // The stand-in's shebang needs the node running this test on the PATH. + PATH: path.dirname(process.execPath) + path.delimiter + process.env.PATH, + }, + }) + let stdout = "" + let stderr = "" + child.stdout.on("data", (chunk) => (stdout += chunk)) + child.stderr.on("data", (chunk) => (stderr += chunk)) + const closed = new Promise((resolve) => { + child.on("close", (code, signal) => resolve({ code, signal })) + }) + return { + child, + done: () => closed.then((result) => ({ ...result, stdout, stderr })), + } +} + +async function waitForFile(file, content) { + for (let i = 0; i < 200; i++) { + if (fs.existsSync(file) && fs.readFileSync(file, "utf8") === content) { + return + } + await new Promise((resolve) => setTimeout(resolve, 50)) + } + throw new Error("timed out waiting for " + file + " to contain " + content) +} + +const opts = { skip: process.platform === "win32" && "no shebang support" } + +test( + "passes arguments through and exits with the binary's status", + opts, + async (t) => { + const { launcher } = install(t, true) + + const echoed = await run(launcher, [ + "echo", + "tunnel", + "--port", + "4000", + ]).done() + assert.equal(echoed.stdout, "tunnel --port 4000\n") + assert.equal(echoed.code, 0) + + const failed = await run(launcher, ["exit", "3"]).done() + assert.equal(failed.code, 3) + }, +) + +test("forwards termination signals to the binary", opts, async (t) => { + for (const signal of ["SIGTERM", "SIGINT", "SIGHUP"]) { + const { root, launcher } = install(t, true) + const marker = path.join(root, "signal") + const proc = run(launcher, ["wait", marker]) + await waitForFile(marker, "ready") + + proc.child.kill(signal) + + const result = await proc.done() + assert.equal(fs.readFileSync(marker, "utf8"), signal) + assert.equal(result.code, 0) + assert.equal(result.signal, null) + } +}) + +test( + "terminates with the signal that terminated the binary", + opts, + async (t) => { + const { launcher } = install(t, true) + + const result = await run(launcher, ["self-kill"]).done() + assert.equal(result.signal, "SIGTERM") + assert.equal(result.code, null) + }, +) + +test("fails when the platform package is missing", opts, async (t) => { + const { launcher } = install(t, false) + + const result = await run(launcher, ["version"]).done() + assert.equal(result.code, 1) + assert.match(result.stderr, new RegExp(platformPackage + " is missing")) +}) diff --git a/package.json b/package.json index 3bc5f52f..48244c51 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,8 @@ "scripts": { "test": "npm run test:cypress", "test:cypress": "cypress run", - "test:cypress:dev": "cypress open" + "test:cypress:dev": "cypress open", + "test:launcher": "node --test npm/run.test.js" }, "prettier": "ory-prettier-styles", "devDependencies": {