diff --git a/.changeset/stale-ui-shutdown.md b/.changeset/stale-ui-shutdown.md new file mode 100644 index 00000000..27ea3f63 --- /dev/null +++ b/.changeset/stale-ui-shutdown.md @@ -0,0 +1,14 @@ +--- +'@inkeep/open-knowledge': patch +--- + +fix: tear down the `ok ui` sibling when `ok start` exits via a signal + +`ok start` spawns a detached `ok ui` process to serve the editor shell. On +`Ctrl+C` (SIGINT/SIGTERM) the CLI destroyed the collab server but left that UI +child running until its 12-hour safety timer expired, holding its port so the +next `ok start` bound a different one. The signal path now runs the same guarded +UI teardown as idle-shutdown — SIGTERM, wait out a grace window, then escalate +to SIGKILL — scoped by `spawnedUiPid` to the sibling this process actually +spawned, so a lock holder we did not spawn (a desktop shell, another session) is +left alone. diff --git a/.github/scripts/bridge-public-pr-to-monorepo.mjs b/.github/scripts/bridge-public-pr-to-monorepo.mjs index 3ff3b175..1c3958f6 100644 --- a/.github/scripts/bridge-public-pr-to-monorepo.mjs +++ b/.github/scripts/bridge-public-pr-to-monorepo.mjs @@ -11,7 +11,7 @@ import { applyClaGate } from './cla-gate.mjs'; // Sibling bridge copies: // - public/agents/.github/scripts/bridge-public-pr-to-monorepo.mjs // - public/agents-optional-local-dev/.github/scripts/bridge-public-pr-to-monorepo.mjs -// - public/mermaid-wysiwyg/.github/scripts/bridge-public-pr-to-monorepo.mjs +// - public/visimer/.github/scripts/bridge-public-pr-to-monorepo.mjs // The Open Knowledge copy additionally imports a co-located `cla-gate.mjs` for // contributor-CLA enforcement — an OK-only divergence. That module ships to the // same repo via Copybara, so the import resolves on the mirror; the "no shared diff --git a/packages/cli/src/commands/start.test.ts b/packages/cli/src/commands/start.test.ts index 0d5c6b50..28ae6961 100644 --- a/packages/cli/src/commands/start.test.ts +++ b/packages/cli/src/commands/start.test.ts @@ -26,6 +26,7 @@ import { shouldConnectToExistingServer, spawnOkUi, startCommand, + teardownUiSibling, tryDescribeLockCollision, type UiSpawnDecision, withEphemeralTempDirReap, @@ -346,6 +347,78 @@ describe('buildIdleShutdownHandler', () => { }); }); +describe('teardownUiSibling (signal-driven ok start teardown)', () => { + test('SIGTERMs the owned sibling and returns once it exits within grace', async () => { + const events: string[] = []; + let alive = true; + await teardownUiSibling({ + readUiLock: () => ({ pid: 1234, port: 3000 }), + spawnedUiPid: () => 1234, + isAlive: () => alive, + killPid: (pid, sig) => { + events.push(`kill:${pid}:${sig}`); + if (sig === 'SIGTERM') alive = false; + }, + sigtermGraceMs: 100, + sigtermPollIntervalMs: 5, + sleep: async () => {}, + reason: 'shutdown', + }); + expect(events).toEqual(['kill:1234:SIGTERM']); + }); + + test('escalates to SIGKILL when the owned sibling ignores SIGTERM', async () => { + const events: string[] = []; + await teardownUiSibling({ + readUiLock: () => ({ pid: 1234, port: 3000 }), + spawnedUiPid: () => 1234, + isAlive: () => true, + killPid: (pid, sig) => events.push(`kill:${pid}:${sig}`), + sigtermGraceMs: 20, + sigtermPollIntervalMs: 5, + sleep: async () => {}, + reason: 'shutdown', + }); + expect(events).toEqual(['kill:1234:SIGTERM', 'kill:1234:SIGKILL']); + }); + + // The regression guard: on the signal path too, a live ui.lock holder that is + // NOT the pid we spawned must be left untouched. This is the exact incident + // (a stale/idle server SIGTERMing a live desktop-spawned or other-session + // server that merely holds ui.lock) the spawnedUiPid guard exists to prevent. + test('leaves a live ui.lock holder alone when it is not the spawned sibling', async () => { + const events: string[] = []; + const infos: object[] = []; + await teardownUiSibling({ + readUiLock: () => ({ pid: 9999, port: 3000 }), + spawnedUiPid: () => 1234, + isAlive: () => true, + killPid: (pid, sig) => events.push(`kill:${pid}:${sig}`), + log: { info: (obj) => infos.push(obj), warn: () => {}, error: () => {} }, + reason: 'shutdown', + }); + expect(events).toEqual([]); + expect(infos.find((entry) => (entry as { pid?: number }).pid === 9999)).toBeDefined(); + }); + + // Defense-in-depth: even if a null ownPid reaches teardownUiSibling (the + // signal handler's `!== null` guard means it normally can't), a live lock + // holder routes through the ownership-guard "leave it alone" branch — a null + // ownPid never equals a real pid — so nothing is killed. + test('leaves a live lock holder alone when ownPid is null (routes through the ownership guard)', async () => { + const events: string[] = []; + await teardownUiSibling({ + readUiLock: () => ({ pid: 52425, port: 64430 }), + spawnedUiPid: () => null, + isAlive: () => true, + killPid: (pid, sig) => events.push(`kill:${pid}:${sig}`), + log: { info: () => {}, warn: () => {}, error: () => {} }, + reason: 'shutdown', + }); + expect(events).toEqual([]); + }); +}); + describe('spawnOkUi', () => { let tmpDir: string; let lockDir: string; diff --git a/packages/cli/src/commands/start.ts b/packages/cli/src/commands/start.ts index 9ac635b5..0693c29f 100644 --- a/packages/cli/src/commands/start.ts +++ b/packages/cli/src/commands/start.ts @@ -426,24 +426,6 @@ interface BuildIdleShutdownHandlerInput { const DEFAULT_SIGTERM_GRACE_MS = SHARED_DEFAULT_SIGTERM_GRACE_MS; const DEFAULT_SIGTERM_POLL_MS = SHARED_DEFAULT_SIGTERM_POLL_MS; -/** - * Build the idle-shutdown `onShutdown` closure. On fire: - * (1) look up `ui.lock`; SIGTERM the sibling if it's still alive AND it is - * the pid this process spawned (`spawnedUiPid`) — a live holder we did - * not spawn is left alone (see the field's docstring for the incident - * class this prevents); - * (2) poll its liveness up to `sigtermGraceMs` (default 10s); - * (3) if still alive after the grace window, escalate to SIGKILL; - * (4) await `destroy()`, which releases `server.lock` as its final step. - * - * Escalation matters because a hung `ok ui` (stuck in a GC pause or a - * downstream fetch in `/api/config`) would otherwise block idle-shutdown - * indefinitely. Escalation is logged at WARN so the operator sees that a - * non-standard path ran. - * - * Extracted so tests can exercise each branch (no UI, live UI, stale UI, - * SIGTERM-takes, SIGKILL-escalation) without standing up Hocuspocus. - */ /** * Wrap an idle-shutdown handler so that, after the server is destroyed, the * ephemeral session's throwaway temp projectDir is removed. Without this an @@ -555,56 +537,81 @@ export function withIdleShutdownProcessExit( }; } -export function buildIdleShutdownHandler( - input: BuildIdleShutdownHandlerInput, -): () => Promise { +/** + * Signal the auto-spawned `ok ui` sibling to exit: SIGTERM, poll its liveness + * up to `sigtermGraceMs`, then escalate to SIGKILL if it's wedged. Shared by + * idle-shutdown and the signal-driven `ok start` teardown so BOTH honor the + * same ownership guard — a live lock holder whose pid is not `spawnedUiPid()` + * is left alone (see that field's docstring for the incident class this + * prevents). `reason` prefixes the log lines to identify the calling path + * (`idle-shutdown` | `shutdown`; defaults to `teardown` when omitted). + * Best-effort throughout: a failed lookup/kill is logged, never thrown, so the + * caller's own teardown (destroy / server.lock release) always proceeds. + */ +export async function teardownUiSibling( + input: Omit & { reason?: string }, +): Promise { const graceMs = input.sigtermGraceMs ?? DEFAULT_SIGTERM_GRACE_MS; const pollMs = input.sigtermPollIntervalMs ?? DEFAULT_SIGTERM_POLL_MS; const sleep = input.sleep ?? ((ms: number) => wait(ms)); + const reason = input.reason ?? 'teardown'; - return async () => { - try { - const lock = input.readUiLock(); - const ownPid = input.spawnedUiPid(); - if (lock && input.isAlive(lock.pid) && lock.pid !== ownPid) { - // The lock holder is alive but is NOT the sibling we spawned — a - // desktop-spawned server advertising its shell, or another session's - // UI. It is not ours to kill; it has its own lifecycle (idle-shutdown - // or the `ok ui` 12h safety net). - input.log?.info( - { pid: lock.pid, port: lock.port, spawnedUiPid: ownPid }, - 'idle-shutdown: ui.lock holder is not our spawned sibling — leaving it alone', - ); - } else if (lock && input.isAlive(lock.pid)) { - try { - input.killPid(lock.pid, 'SIGTERM'); - input.log?.info({ pid: lock.pid, port: lock.port }, 'idle-shutdown: SIGTERM UI sibling'); - // Wait up to graceMs for the UI process to exit under SIGTERM. - const deadline = Date.now() + graceMs; - while (Date.now() < deadline) { - if (!input.isAlive(lock.pid)) break; - await sleep(pollMs); - } - if (input.isAlive(lock.pid)) { - // Grace expired — escalate to SIGKILL. Operators see this at WARN. - try { - input.killPid(lock.pid, 'SIGKILL'); - input.log?.warn( - { pid: lock.pid, graceMs }, - 'idle-shutdown: SIGTERM grace expired — escalated to SIGKILL', - ); - } catch (err) { - input.log?.error({ pid: lock.pid, err }, 'idle-shutdown: SIGKILL failed'); - } + try { + const lock = input.readUiLock(); + const ownPid = input.spawnedUiPid(); + if (lock && input.isAlive(lock.pid) && lock.pid !== ownPid) { + // The lock holder is alive but is NOT the sibling we spawned — a + // desktop-spawned server advertising its shell, or another session's + // UI. It is not ours to kill; it has its own lifecycle (idle-shutdown + // or the `ok ui` 12h safety net). + input.log?.info( + { pid: lock.pid, port: lock.port, spawnedUiPid: ownPid }, + `${reason}: ui.lock holder is not our spawned sibling — leaving it alone`, + ); + } else if (lock && input.isAlive(lock.pid)) { + try { + input.killPid(lock.pid, 'SIGTERM'); + input.log?.info({ pid: lock.pid, port: lock.port }, `${reason}: SIGTERM UI sibling`); + // Wait up to graceMs for the UI process to exit under SIGTERM. + const deadline = Date.now() + graceMs; + while (Date.now() < deadline) { + if (!input.isAlive(lock.pid)) break; + await sleep(pollMs); + } + if (input.isAlive(lock.pid)) { + // Grace expired — escalate to SIGKILL. Operators see this at WARN. + try { + input.killPid(lock.pid, 'SIGKILL'); + input.log?.warn( + { pid: lock.pid, graceMs }, + `${reason}: SIGTERM grace expired — escalated to SIGKILL`, + ); + } catch (err) { + input.log?.error({ pid: lock.pid, err }, `${reason}: SIGKILL failed`); } - } catch (err) { - input.log?.warn({ pid: lock.pid, err }, 'idle-shutdown: failed to SIGTERM UI sibling'); } + } catch (err) { + input.log?.warn({ pid: lock.pid, err }, `${reason}: failed to SIGTERM UI sibling`); } - } catch (err) { - input.log?.warn({ err }, 'idle-shutdown: UI lookup failed; proceeding with destroy'); } - await input.destroy(); + } catch (err) { + input.log?.warn({ err }, `${reason}: UI lookup failed; proceeding`); + } +} + +/** + * The idle-shutdown `onShutdown` closure: tear down the UI sibling (guarded by + * `spawnedUiPid`), then `destroy()` — which releases `server.lock` as its final + * step. A thin adapter over `teardownUiSibling`; the escalation logic lives + * there. + */ +export function buildIdleShutdownHandler( + input: BuildIdleShutdownHandlerInput, +): () => Promise { + const { destroy, ...teardown } = input; + return async () => { + await teardownUiSibling({ ...teardown, reason: 'idle-shutdown' }); + await destroy(); }; } @@ -768,6 +775,14 @@ export interface BootedStartServer { degraded: readonly string[]; /** What we decided about the UI sibling at boot — for tests + status output. */ uiSpawnDecision: UiSpawnDecision; + /** + * Pid of the `ok ui` child THIS process spawned, or null when none was + * spawned (sibling reused, auto-spawn skipped, or desktop single-origin). + * The signal-driven teardown in `runStartCommand` passes this to + * `teardownUiSibling` so it only signals the sibling we own — the same + * ownership guard the idle-shutdown closure applies via `spawnedUiPid`. + */ + spawnedUiPid: number | null; /** * The port `ok ui` is actually serving on, resolved end-to-end: * - `action: 'skip'` (sibling already alive) → `uiSpawnDecision.port` @@ -1126,6 +1141,7 @@ export async function bootStartServer(opts: BootStartServerOptions): Promise readUiLock(booted.lockDir), + spawnedUiPid: () => booted.spawnedUiPid, + isAlive: isProcessAlive, + killPid: (pid, sig) => process.kill(pid, sig), + log: getLogger('start'), + reason: 'shutdown', + }); + } catch (err) { + console.error( + `${error('ui sibling teardown failed:')} ${err instanceof Error ? (err.stack ?? err.message) : String(err)}`, + ); + } + } try { await booted.destroy(); } catch (err) {