diff --git a/.changeset/21114-start-signal-forwarding.md b/.changeset/21114-start-signal-forwarding.md new file mode 100644 index 00000000000..0ef829ee59a --- /dev/null +++ b/.changeset/21114-start-signal-forwarding.md @@ -0,0 +1,31 @@ +--- +'@objectstack/cli': patch +--- + +fix(cli): `os start` forwards SIGTERM and SIGINT to its `serve` child and takes the child down when it exits + +Clause-②: no + +`os start` runs the server as a separate `serve` child process. It used to +listen for that child's exit and nothing else. A SIGTERM or SIGINT sent to the +`start` process alone (a plain `kill`, `docker stop`, a systemd stop, a CI step) +ended `start` and left `serve` running with no parent. The port stayed bound, +`/health` kept answering 200, and the next start on that port collided with it. + +`os start` now supervises its child the way `os dev` already does, through the +same mechanism: + +- SIGTERM or SIGINT to `start` is forwarded to the child. `start` waits for the + child to shut down, then exits with the child's exit code, which is 0 after a + graceful shutdown. It used to die on the signal at once (shell status 143 for + SIGTERM, 130 for SIGINT) while the child kept running. +- Whatever else ends `start`, the child is sent SIGTERM on the way out. +- A child that exits on its own still ends `start` with the child's exit code, + as before. + +One visible side effect, the same one `os dev` already has: Ctrl-C at a terminal +signals `start` and the child together, so the child now receives SIGINT twice +and logs one `Shutdown already in progress, ignoring SIGINT` warning. The +terminal prompt also returns only after the server has stopped, not before. + +No flag, port, environment variable, banner line or `os dev` behaviour changes. diff --git a/packages/cli/src/commands/start.ts b/packages/cli/src/commands/start.ts index af0f754aba2..b6d246505be 100644 --- a/packages/cli/src/commands/start.ts +++ b/packages/cli/src/commands/start.ts @@ -13,6 +13,7 @@ import { redirectStdoutToStderr } from '../utils/json-stdout.js'; import { redactConnectionUrl } from '../utils/connection-display.js'; import { databaseDriverFlag } from '../utils/database-driver-flag.js'; import { childEnvWithResolvedArtifact } from '../utils/internal-artifact-channel.js'; +import { ServeRestartCoordinator } from '../utils/dev-restart.js'; import { readEnvWithDeprecation } from '@objectstack/types'; // The ONE port contract, shared with `dev` and with the `serve` child this // command spawns (#12673). ⛔ Nothing about ports is declared in this file — @@ -445,18 +446,50 @@ export default class Start extends Command { } const binPath = process.argv[1]; - const child = spawn( - process.execPath, - [ - binPath, - 'serve', - flags.ui ? '--ui' : '--no-ui', - ...(flags.verbose ? ['--verbose'] : []), - ...(flags['log-level'] ? ['--log-level', flags['log-level']] : []), - ], - { stdio: 'inherit', env: localEnv }, - ); - child.on('exit', (code) => process.exit(code ?? 0)); + + // ── The serve child's lifecycle: `os dev`'s mechanism, not a copy (#21114) + // `start` is a supervisor, so a signal sent to ITS pid alone — a plain + // `kill`, `docker stop`, a systemd stop, a CI step — has to reach the + // child too. This command used to listen for the child's `exit` and + // nothing else: a SIGTERM to the parent ended the parent and left `serve` + // running, reparented to init, port still bound and `/health` still + // answering 200. + // + // ⭐ Supervised by `ServeRestartCoordinator`, the ONE forwarding mechanism + // `os dev` already runs, used as is — `start` never calls + // `requestRestart`, so only the parts both commands need are reached: + // `beginShutdown` forwards SIGINT / SIGTERM and lets the child's exit end + // the parent; `killChildOnParentExit` reaps the child on whatever path + // ends the parent; and a child that exits on its own still ends the + // parent with `code ?? 0`, exactly the handler this replaces. + // ⛔ No forwarding or reaping logic of this command's own: what a signal + // DOES belongs in the coordinator, where `dev` gets the same change. + // Pinned end to end, with `os dev` as the control, by + // `test/start-signal-forwarding.e2e.test.ts`. + const coordinator = new ServeRestartCoordinator({ + spawnChild: () => { + // `const child = spawn(` is the anchor + // `utils/port-contract-single-source.test.ts` reads to hold the port + // door above AHEAD of this spawn — keep the spelling. + const child = spawn( + process.execPath, + [ + binPath, + 'serve', + flags.ui ? '--ui' : '--no-ui', + ...(flags.verbose ? ['--verbose'] : []), + ...(flags['log-level'] ? ['--log-level', flags['log-level']] : []), + ], + { stdio: 'inherit', env: localEnv }, + ); + return child; + }, + exitParent: (code) => process.exit(code), + }); + process.on('SIGINT', () => coordinator.beginShutdown('SIGINT')); + process.on('SIGTERM', () => coordinator.beginShutdown('SIGTERM')); + process.on('exit', () => coordinator.killChildOnParentExit()); + coordinator.start(); } } diff --git a/packages/cli/test/start-signal-forwarding.e2e.test.ts b/packages/cli/test/start-signal-forwarding.e2e.test.ts new file mode 100644 index 00000000000..e3004ec236d --- /dev/null +++ b/packages/cli/test/start-signal-forwarding.e2e.test.ts @@ -0,0 +1,286 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A signal to `os start` takes its `serve` child down with it (#21114). + * + * ## The defect, as measured on `origin/main` before the repair + * + * `os start` is a supervisor: it spawns `os serve` as a SEPARATE process and + * then waits. It listened for the child's `exit` and nothing else, so a SIGTERM + * sent to the `start` pid alone (a plain `kill`, a CI step, a process manager) + * ended the parent and left the child running, reparented to init, with its + * port still bound and `/health` still answering 200. `os dev` never had the + * defect: its `ServeRestartCoordinator` forwards SIGINT / SIGTERM to the child + * and reaps the child when the parent exits. + * + * The repair is that `start` supervises its child through that SAME + * coordinator, so there is one forwarding mechanism in the CLI, not two. + * + * ## What is asserted + * + * The ruling's pin, literally: a SIGTERM (and a SIGINT) to the PARENT leaves no + * child process and a free port. `os dev`, booted on the same artifact and sent + * the same signal, is the control: it states that the instrument reads the + * shipped mechanism correctly, and that `dev`'s behaviour did not move. + * + * Both readings carry a positive control taken on the same boot before the + * signal: the probe sees the child ALIVE and the port BOUND. Without that, "no + * child" passes on a probe that cannot see children at all, and "port free" + * passes on a child that bound somewhere else. + * + * ⛔ The signal goes to the parent's pid ALONE, never to its process group. A + * group signal reaches the child directly and passes whether or not the parent + * forwards anything — it is the shape that HID this defect (the neighbouring + * `start-port-banner-agreement.e2e.test.ts` documents why it kills the group, + * and for its purpose it is right to). + * + * ## Spawn shape + * + * The SOURCE entry, `bin/run-dev.js`, under the tsx loader passed as an + * `--import` flag — never the `tsx` CLI, which spawns the script as a child of + * its own and relays signals to it, so the pid this file signals would be tsx's + * rather than the command's. Loaded this way the spawned pid IS the `os start` / + * `os dev` parent, and the `serve` process is its direct child. + * + * The subject is the supervisor's wiring in `src/commands/start.ts`, which the + * built entry runs byte for byte from `dist/`, so the source entry measures the + * same code without a build prerequisite. The `development` posture it pins + * re-opens `serve`'s port auto-shift; that is harmless here because the port is + * read back out of the child's own banner (`boundPortFromBanner`) rather than + * assumed from what this file asked for. The built entry was measured too, by + * hand, before and after the repair (the PR that landed this records both). + * + * Each child gets its own process group (`detached: true`) so `afterEach` can + * still SIGKILL a survivor — which is exactly what the unrepaired command + * leaves behind — instead of leaking it into the container. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { execFileSync, spawn, type ChildProcess } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve, dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { PROTOCOL_MAJOR } from '@objectstack/spec/kernel'; +import { + childEnv, + boundPortFromBanner, + portIsFree, + reservePort, +} from './helpers/serve-process.js'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const RUN_DEV_JS = resolve(HERE, '../bin/run-dev.js'); +const TSX_LOADER = resolve(HERE, '../../../node_modules/tsx/dist/loader.mjs'); + +/** The banner tail `printServerReady` ends with — the boot is fully printed. */ +const BANNER_TAIL = /Press Ctrl\+C to stop/; + +const BOOT_TIMEOUT_MS = 180_000; +/** The kernel's graceful shutdown, plus event-loop delivery of the exit. */ +const EXIT_TIMEOUT_MS = 60_000; + +/** A minimal but real compiled artifact, booted identically by both commands. */ +const ARTIFACT = JSON.stringify({ + manifest: { + id: 'com.example.signals', + name: 'signals', + version: '1.0.0', + type: 'app', + engines: { protocol: `^${PROTOCOL_MAJOR}` }, + }, + objects: [ + { name: 'sig_note', label: 'Note', fields: { name: { type: 'text', label: 'Name' } } }, + ], + views: [], + apps: [], + flows: [], + requires: [], +}); + +type Command = 'start' | 'dev'; + +/** The argv that boots `command` on `artifact` at `port`, with no Console. */ +function argvFor(command: Command, artifact: string, port: number): string[] { + // `start` declares `--ui` with `allowNo` (default on); `dev` declares it + // default-off and rejects `--no-ui`. Both therefore boot without a Console. + return command === 'start' + ? ['start', '--artifact', artifact, '--port', String(port), '--no-ui'] + : ['dev', '--artifact', artifact, '--port', String(port)]; +} + +/** + * The pids of `ppid`'s `serve` children, read from the process table — a child + * whose argv carries the `serve` command token. + * + * ⚠️ Selected by argv, not "every child", and MEASURED to need it: on a cold + * tsx transform cache the loader runs an `esbuild --service` process as a + * second child of the CLI parent. It exits on its own a moment AFTER the parent + * (alive at the parent's exit, gone 2 s later, 2 of 2 runs with + * `TSX_DISABLE_CACHE=1`), so counting it reads as an orphaned server on the + * file's first boot and on no other. The ruling's sentence is about the + * `serve` child, and so is this probe. `-ww` keeps BSD `ps` from cutting the + * argv at a terminal width. + */ +function serveChildPidsOf(ppid: number): number[] { + const table = execFileSync('ps', ['-A', '-ww', '-o', 'pid=,ppid=,args='], { + encoding: 'utf8', + env: childEnv(), + }); + return table + .split('\n') + .map((line) => line.trim().split(/\s+/)) + .filter(([pid, parent, ...args]) => + Number.isInteger(Number(pid)) && Number(parent) === ppid && args.includes('serve')) + .map(([pid]) => Number(pid)); +} + +/** Does `pid` name a live process? (`EPERM` = alive, owned by someone else.) */ +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (err) { + return (err as NodeJS.ErrnoException).code === 'EPERM'; + } +} + +/** SIGKILL a whole process group — the survivor sweep, never the measurement. */ +const killGroup = (child: ChildProcess | undefined): void => { + if (child?.pid === undefined) return; + try { process.kill(-child.pid, 'SIGKILL'); } catch { /* group already gone */ } +}; + +interface Exit { code: number | null; signal: NodeJS.Signals | null } + +interface Booted { + parent: ChildProcess; + port: number; + output: () => string; + exited: Promise; +} + +let running: ChildProcess | undefined; +let workdir: string | undefined; + +afterEach(() => { + killGroup(running); + running = undefined; + if (workdir) rmSync(workdir, { recursive: true, force: true }); + workdir = undefined; +}); + +/** Boot a real `os ` and resolve once its child printed the ready banner. */ +function boot(command: Command): Promise { + const dir = mkdtempSync(join(tmpdir(), `os-${command}-signal-`)); + workdir = dir; + const artifact = join(dir, 'objectstack.json'); + writeFileSync(artifact, ARTIFACT); + const port = reservePort(); + + return new Promise((resolveBoot, rejectBoot) => { + const parent = spawn( + process.execPath, + ['--import', TSX_LOADER, RUN_DEV_JS, ...argvFor(command, artifact, port)], + { + cwd: dir, + env: childEnv({ + NO_COLOR: '1', + OS_HOME: join(dir, 'home'), + OS_LOG_LEVEL: 'error', + }), + stdio: ['ignore', 'pipe', 'pipe'], + detached: true, + }, + ); + running = parent; + + let output = ''; + let ready = false; + const exited = new Promise((settle) => { + parent.once('exit', (code, signal) => settle({ code, signal })); + }); + + const timer = setTimeout(() => { + if (ready) return; + rejectBoot(new Error(`os ${command} never printed a complete banner.\n--- output ---\n${output}`)); + }, BOOT_TIMEOUT_MS); + + const onData = (d: unknown) => { + output += String(d); + if (!ready && BANNER_TAIL.test(output)) { + ready = true; + clearTimeout(timer); + const readback = boundPortFromBanner(output); + if (readback.state !== 'bound') { + rejectBoot(new Error(`os ${command}: banner unreadable (${readback.state}).\n--- output ---\n${output}`)); + return; + } + resolveBoot({ parent, port: readback.port, output: () => output, exited }); + } + }; + parent.stdout?.on('data', onData); + parent.stderr?.on('data', onData); + parent.on('error', (err) => { clearTimeout(timer); rejectBoot(err); }); + void exited.then(({ code, signal }) => { + if (ready) return; + clearTimeout(timer); + rejectBoot(new Error( + `os ${command} exited (${signal ?? code}) before its banner.\n--- output ---\n${output}`, + )); + }); + }); +} + +/** Signal the parent alone, then read what is left of its child and its port. */ +async function signalParent(command: Command, signal: NodeJS.Signals): Promise { + const { parent, port, output, exited } = await boot(command); + const parentPid = parent.pid!; + + // ── Positive controls, on the same boot, before the signal ───────────── + const children = serveChildPidsOf(parentPid); + expect(children, `os ${command}: the probe does not see exactly one serve child.\n${output()}`) + .toHaveLength(1); + expect(children.every(isAlive), `os ${command}: a child reads dead before the signal`).toBe(true); + expect(portIsFree(port), `os ${command}: port ${port} reads free while the server is up`).toBe(false); + + // ── The measurement: the PARENT's pid alone, never its group ─────────── + process.kill(parentPid, signal); + + let timer: ReturnType | undefined; + const fate = await Promise.race([ + exited, + new Promise<'timeout'>((settle) => { timer = setTimeout(() => settle('timeout'), EXIT_TIMEOUT_MS); }), + ]); + clearTimeout(timer); + expect(fate, `os ${command} did not exit within ${EXIT_TIMEOUT_MS}ms of ${signal}.\n${output()}`) + .not.toBe('timeout'); + + // Soft, both: the two readings are independent facts, and a regression + // should report each of them rather than stop at the first. + expect.soft( + children.filter(isAlive), + `os ${command}: ${signal} to the parent left its serve child running (orphaned, ` + + `reparented to init).\n--- output ---\n${output()}`, + ).toEqual([]); + expect.soft( + portIsFree(port), + `os ${command}: ${signal} to the parent left port ${port} bound.\n--- output ---\n${output()}`, + ).toBe(true); +} + +describe('a signal to the CLI parent takes its `serve` child down with it', () => { + for (const signal of ['SIGTERM', 'SIGINT'] as const) { + it( + `os start: ${signal} to the parent leaves no child process and a free port`, + () => signalParent('start', signal), + BOOT_TIMEOUT_MS + EXIT_TIMEOUT_MS + 30_000, + ); + + it( + `os dev (control): ${signal} to the parent leaves no child process and a free port`, + () => signalParent('dev', signal), + BOOT_TIMEOUT_MS + EXIT_TIMEOUT_MS + 30_000, + ); + } +});