Sync 2 Inkeep OSS Changes

.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.

.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

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;

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<void> {
+/**
+ * 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<BuildIdleShutdownHandlerInput, 'destroy'> & { reason?: string },
+): Promise<void> {
   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<void> {
+  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<Boo
     ready: booted.ready,
     degraded: booted.degraded,
     uiSpawnDecision,
+    spawnedUiPid,
     resolvedUiPort,
   };
 }
@@ -1379,6 +1395,34 @@ export async function runStartCommand(config: Config, opts: StartCommandOptions)
     for (const line of details) {
       console.log(dim(`  ${line}`));
     }
+    // Tear down the detached `ok ui` sibling BEFORE destroy releases
+    // server.lock — same order as idle-shutdown. Without this, Ctrl+C left the
+    // UI child running until its 12h safety timer, holding its port. Two
+    // distinct guards: the outer `!== null` skips the whole block (and its
+    // import) when we spawned no sibling; `teardownUiSibling`'s internal
+    // `spawnedUiPid` comparison confirms the CURRENT lock holder is still the
+    // pid we spawned, so a holder we didn't spawn (desktop shell, another
+    // session) is left alone. Wrapped so a failure here never bypasses
+    // destroy() — the best-effort teardown contract idle-shutdown also honors.
+    if (booted.spawnedUiPid !== null) {
+      try {
+        const { getLogger, isProcessAlive, readUiLock } = await import(
+          '@inkeep/open-knowledge-server'
+        );
+        await teardownUiSibling({
+          readUiLock: () => 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) {