fix(bin): forward signals from pty wrapper to inner CLI

The bin/pty wrapper used spawnSync, which (a) blocks the wrapper's event
loop so any signal handlers we'd register would never run, and
(b) leaves no path for SIGTERM/SIGINT/etc. to reach the cli.js child.

Under systemd with KillMode=process, the wrapper is the unit's MainPID.
systemd SIGTERMs the wrapper, the wrapper dies, but the inner cli.js
supervisor — which has its own SIGTERM handler that calls
Supervisor.stop() and releases supervisor.lock — never sees the signal.
It gets reparented and keeps running. Every subsequent unit invocation
then exits with 'another supervisor is already running' because the
orphan still holds the lock. Reproduced on a NixOS host where the
restart counter climbed past 500 with the original supervisor still
alive after an hour.

The fix switches to spawn() and forwards SIGTERM/SIGINT/SIGHUP/SIGQUIT/
SIGUSR1/SIGUSR2 to the child, then waits for the child's exit event
before propagating its status. Normal exit-code passthrough is
unchanged. Signal-death is mapped to 128+signum, matching shell
convention.

Tests in tests/wrapper-signal-forwarding.test.ts spawn the wrapper
running 'supervisor start' against an isolated PTY_SESSION_DIR, SIGTERM
the wrapper, and assert the inner cli.js dies and supervisor.lock is
released. Verified the test fails against the old spawnSync wrapper
(inner pid survives, lock leaks).

schickling-assistant 2 months ago (May 3, 2026, 6:03 AM +0200) f635fe6b 0dc1c8f5

+130 -3

2 changed files

Expand all

bin

pty

tests

wrapper-signal-forwarding.test.ts

+33 -3

bin/pty

··· 1 1 #!/usr/bin/env node 2 2 import { existsSync } from 'node:fs'; 3 - import { spawnSync } from 'node:child_process'; 3 + import { spawn } from 'node:child_process'; 4 4 import { fileURLToPath } from 'node:url'; 5 5 import { dirname, join } from 'node:path'; 6 + import { constants as osConstants } from 'node:os'; 6 7 7 8 const __dirname = dirname(fileURLToPath(import.meta.url)); 8 9 const cli = join(__dirname, '..', 'dist', 'cli.js'); ··· 12 13 process.exit(1); 13 14 } 14 15 15 - const result = spawnSync(process.execPath, [cli, ...process.argv.slice(2)], { 16 + // `spawn` (not `spawnSync`) so the wrapper stays event-loop-live and can 17 + // react to signals it receives. Without this, signals delivered to the 18 + // wrapper (e.g. systemd's SIGTERM under KillMode=process) terminated the 19 + // wrapper without ever reaching the child supervisor, leaving an orphaned 20 + // supervisor that kept holding `supervisor.lock` and made every subsequent 21 + // service restart fail with "another supervisor is already running". 22 + const child = spawn(process.execPath, [cli, ...process.argv.slice(2)], { 16 23 stdio: 'inherit', 17 24 env: process.env, 18 25 }); 19 26 20 - process.exit(result.status ?? 1); 27 + const FORWARDED_SIGNALS = ['SIGTERM', 'SIGINT', 'SIGHUP', 'SIGQUIT', 'SIGUSR1', 'SIGUSR2']; 28 + for (const sig of FORWARDED_SIGNALS) { 29 + process.on(sig, () => { 30 + // Forward and let the child decide when to exit. The wrapper's own 31 + // exit is driven by the child's 'exit' event so the child has time 32 + // to flush before we propagate its status. 33 + try { child.kill(sig); } catch {} 34 + }); 35 + } 36 + 37 + child.on('exit', (code, signal) => { 38 + if (signal) { 39 + // Mirror shell convention (128 + signum) so callers can distinguish 40 + // signal death from a nonzero exit code. 41 + const signum = osConstants.signals[signal] ?? 0; 42 + process.exit(128 + signum); 43 + } 44 + process.exit(code ?? 1); 45 + }); 46 + 47 + child.on('error', (err) => { 48 + console.error(`pty: failed to spawn cli: ${err.message}`); 49 + process.exit(1); 50 + });

+97

tests/wrapper-signal-forwarding.test.ts

··· 1 + // Verifies that bin/pty forwards SIGTERM/SIGINT to the inner cli.js child. 2 + // Without forwarding, systemd's KillMode=process leaves the inner supervisor 3 + // orphaned and still holding supervisor.lock — the new unit invocation then 4 + // fails with "another supervisor is already running". 5 + 6 + import { describe, it, expect, afterEach } from "vitest"; 7 + import * as fs from "node:fs"; 8 + import * as os from "node:os"; 9 + import * as path from "node:path"; 10 + import { fileURLToPath } from "node:url"; 11 + import { spawn } from "node:child_process"; 12 + 13 + const __dirname = path.dirname(fileURLToPath(import.meta.url)); 14 + const wrapperPath = path.join(__dirname, "..", "bin", "pty"); 15 + const nodeBin = process.execPath; 16 + 17 + const sessionDirs: string[] = []; 18 + const trackedPids: number[] = []; 19 + afterEach(() => { 20 + // Belt-and-braces: if a test failed mid-way, kill any inner cli.js it left 21 + // behind so the next run isn't poisoned by an orphan holding the lock. 22 + for (const pid of trackedPids) { 23 + try { process.kill(pid, "SIGKILL"); } catch {} 24 + } 25 + trackedPids.length = 0; 26 + for (const d of sessionDirs) { 27 + try { fs.rmSync(d, { recursive: true, force: true }); } catch {} 28 + } 29 + sessionDirs.length = 0; 30 + }); 31 + 32 + function makeSessionDir(): string { 33 + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "pty-wrap-")); 34 + sessionDirs.push(dir); 35 + return dir; 36 + } 37 + 38 + /** Wait until predicate returns true or `timeoutMs` elapses. */ 39 + async function waitFor(predicate: () => boolean, timeoutMs: number): Promise<boolean> { 40 + const start = Date.now(); 41 + while (Date.now() - start < timeoutMs) { 42 + if (predicate()) return true; 43 + await new Promise((r) => setTimeout(r, 25)); 44 + } 45 + return predicate(); 46 + } 47 + 48 + function isAlive(pid: number): boolean { 49 + try { process.kill(pid, 0); return true; } catch { return false; } 50 + } 51 + 52 + describe("bin/pty signal forwarding", () => { 53 + it("propagates SIGTERM to the inner cli.js so the supervisor releases its lock", async () => { 54 + const sessionDir = makeSessionDir(); 55 + 56 + // `supervisor start` is the canonical long-lived command and the one the 57 + // dev3 regression hit. Its SIGTERM handler calls Supervisor.stop() which 58 + // releases supervisor.lock — so a clean shutdown leaves no lock file. 59 + const wrapper = spawn(nodeBin, [wrapperPath, "supervisor", "start"], { 60 + env: { ...process.env, PTY_SESSION_DIR: sessionDir }, 61 + stdio: ["ignore", "pipe", "pipe"], 62 + }); 63 + 64 + let stdout = ""; 65 + let stderr = ""; 66 + wrapper.stdout?.on("data", (d: Buffer) => { stdout += d.toString(); }); 67 + wrapper.stderr?.on("data", (d: Buffer) => { stderr += d.toString(); }); 68 + 69 + let exitCode: number | null = null; 70 + let exitSignal: NodeJS.Signals | null = null; 71 + wrapper.on("exit", (c, s) => { exitCode = c; exitSignal = s; }); 72 + 73 + // Wait for the supervisor to fully start (it writes supervisor.pid). 74 + const pidPath = path.join(sessionDir, "supervisor", "supervisor.pid"); 75 + const lockPath = path.join(sessionDir, "supervisor.lock"); 76 + const ready = await waitFor(() => fs.existsSync(pidPath) && fs.existsSync(lockPath), 5000); 77 + expect(ready, `supervisor never started; stdout=${stdout} stderr=${stderr}`).toBe(true); 78 + 79 + const innerPid = parseInt(fs.readFileSync(pidPath, "utf-8").trim(), 10); 80 + trackedPids.push(innerPid); 81 + expect(innerPid).toBeGreaterThan(0); 82 + expect(innerPid).not.toBe(wrapper.pid); 83 + expect(isAlive(innerPid)).toBe(true); 84 + 85 + // The actual test: SIGTERM the wrapper, expect the inner cli.js to also die. 86 + wrapper.kill("SIGTERM"); 87 + 88 + const wrapperExited = await waitFor(() => exitCode !== null || exitSignal !== null, 5000); 89 + expect(wrapperExited, "wrapper did not exit after SIGTERM").toBe(true); 90 + 91 + const innerDied = await waitFor(() => !isAlive(innerPid), 5000); 92 + expect(innerDied, `inner cli.js (pid ${innerPid}) survived wrapper SIGTERM — signal not forwarded`).toBe(true); 93 + 94 + // Clean shutdown should release the lock; a SIGKILLed supervisor would leave it. 95 + expect(fs.existsSync(lockPath), "supervisor.lock not released — child shutdown was not graceful").toBe(false); 96 + }, 20000); 97 + });

Configure Feed

Configure Feed