From 430346e14bd7183e900cc82b7af0693257221f6f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Padez?= <andre.padez@gmail.com>
Date: Sun, 16 Aug 2026 17:41:36 +0000
Subject: [PATCH] a restarted job outlives the server that restarted it
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The other half of the same report. The pill came back after a refresh, and then a
`pm2 restart officer` killed the dev server outright — because `restartBackgroundTask`
spawned it as a plain child of the platform. A job the AGENT starts is a child of the
`officer-claude-code` sidecar, a PM2 peer, so those were never affected. The tray
therefore showed two chips that looked identical and behaved differently, and the
restart button's own help text promised the opposite of what happened.

Measured against a throwaway PM2 app rather than reasoned about, because the two
plausible causes call for different fixes — a signal to the process GROUP, or a walk
of the process TREE:

    plain `sh -c`           killed
    `setsid sh -c`          killed     <- a new session is NOT enough
    `setsid --fork sh -c`   survived

PM2 walks the tree by ppid, so a new session leaves the job a child and it is found
anyway. `--fork` is what matters: setsid forks and its parent exits at once, so the
job is reparented to init before any restart can enumerate it.

── What that costs, and how it is paid ──

The handle `Bun.spawn` returns is now the setsid parent, which exits immediately.
Waiting on it reported every job as finished the moment it started, so exit detection
moved to `watchForExit`: poll who holds the output file, and when nobody does, read
the status the wrapper recorded. That is the same liveness question Stop and Pause
already ask, so it adds no second notion of "running" — and a paused job still holds
its file, so SIGSTOP correctly reads as running rather than as an exit.

Two details that would each have been a bug:

  - A GRACE PERIOD. Between the fork and the job opening its output there is a moment
    with no holders; polling into it announces a completion for a job that never ran.
  - The command is a POSITIONAL PARAMETER to the wrapper, not interpolated into it.
    Interpolation would let a command containing quotes terminate the wrapper's own
    quoting. Tested with `echo "quoted 'both' ways"`.

A missing status file is reported as a clean finish, not a failure: that is what Stop
looks like, and "you stopped it" is nearer the truth than "it crashed".

Also corrected the button text. It said background jobs survive "because they are
their own PM2 processes" — true of agent-started jobs, false of restarted ones, and
now the wrong reason for both. They survive because they are outside this process's
tree.

── Verified ──

End-to-end through `buildHonoApp`, on a synthetic task:

    POST /restart -> 200
    holders: pid 906186 (ppid 1, session leader), pid 906188 (ppid 906186)
             probe process was 906173 — in neither chain
    quotes survived the wrapper
    durable events: task:started -> task:notification (completed), exit file "0"

My own assertion in that probe was wrong and the output is what corrected it: it
demanded ppid 1 of EVERY holder, but only the session root is reparented — its own
children hang off it, which is the point.

Not in `bun test`: nothing in the suite touches Postgres or spawns processes.

tsgo clean, 919 pass / 7 fail unchanged. Needs a server restart.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 .../ServerSettings/MaintenanceSection.tsx     |   2 +-
 src/servers/api/chat/task-restart.ts          | 130 +++++++++++++++---
 2 files changed, 113 insertions(+), 19 deletions(-)

diff --git a/src/apps/officer-web/Screens/Dashboard/Settings/ServerSettings/MaintenanceSection.tsx b/src/apps/officer-web/Screens/Dashboard/Settings/ServerSettings/MaintenanceSection.tsx
index 471b5fb9..0d6f88e2 100644
--- a/src/apps/officer-web/Screens/Dashboard/Settings/ServerSettings/MaintenanceSection.tsx
+++ b/src/apps/officer-web/Screens/Dashboard/Settings/ServerSettings/MaintenanceSection.tsx
@@ -113,7 +113,7 @@ export const MaintenanceSection = () => {
           </p>
           <p className="mt-1 text-xs text-muted-foreground">
             Costs every open connection: websockets drop and in-memory chat sessions are lost. Background jobs survive —
-            they are their own PM2 processes.
+            they run outside this process&rsquo;s tree.
           </p>
         </div>
         <Button
diff --git a/src/servers/api/chat/task-restart.ts b/src/servers/api/chat/task-restart.ts
index f3971ca1..9d4f20ac 100644
--- a/src/servers/api/chat/task-restart.ts
+++ b/src/servers/api/chat/task-restart.ts
@@ -28,8 +28,56 @@ import { findTaskOutput, pidsHoldingFile, type ChatIdentity } from './claude-ses
  * which pids hold it open. A new file would have meant teaching both about a second location.
  */
 
-/** In-flight restarts, so a second click stops the first rather than racing it. */
-const restarted = new Map<string, { proc: Bun.Subprocess }>();
+/**
+ * In-flight restarts, so a second click stops the first rather than racing it.
+ *
+ * A timer rather than a subprocess handle, because the process is deliberately no longer ours — see
+ * `detachedArgv`.
+ */
+const restarted = new Map<string, { watch: ReturnType<typeof setInterval> }>();
+
+/** How long to let a job open its output file before an empty holder list is read as "it ended". */
+const WATCH_GRACE_MS = 2500;
+const WATCH_INTERVAL_MS = 5000;
+
+/**
+ * The argv that survives `pm2 restart officer`.
+ *
+ * A restarted job used to be spawned as a plain child of the platform, and it died with the platform. That
+ * was not a footnote: the tray showed two chips that looked identical — one the agent started, one this
+ * module restarted — and only the second evaporated on a server restart, while the button's own help text
+ * promised that background jobs survive.
+ *
+ * Measured rather than reasoned about, against a throwaway PM2 app. Three spawn shapes, one `pm2 restart`:
+ *
+ *   plain `sh -c`              killed
+ *   `setsid sh -c`             killed      <- a new SESSION is not enough
+ *   `setsid --fork sh -c`      survived
+ *
+ * PM2 walks the process tree by ppid, so changing session leaves the job a child and it is found anyway.
+ * `--fork` is the part that matters: setsid forks and its parent exits at once, so the job is reparented
+ * to init before any restart can enumerate it. It is not officer's descendant, so there is nothing to walk.
+ *
+ * The cost is paid in `watchForExit`: the handle `Bun.spawn` returns belongs to the setsid parent, which
+ * exits immediately, so the process's real ending has to be noticed some other way.
+ *
+ * The wrapper passes the command as a POSITIONAL PARAMETER rather than interpolating it into the script,
+ * so a command containing quotes stays one argument and cannot terminate the wrapper's own quoting. `$2`
+ * is where the exit status is recorded — the only place it survives, now that nothing is waiting on the
+ * process.
+ */
+const WRAPPER = 'rm -f "$2"; eval "$1"; echo $? > "$2"';
+
+const detachedArgv = (command: string, exitFile: string): string[] => [
+  'setsid',
+  '--fork',
+  'sh',
+  '-c',
+  WRAPPER,
+  'sh',
+  command,
+  exitFile,
+];
 
 export type RestartResult = { ok: boolean; command?: string; reason?: string };
 
@@ -143,15 +191,12 @@ export async function restartBackgroundTask(
     return { ok: false, reason: 'cannot tell which directory this job ran in — refusing to guess' };
   }
 
-  // Ours first, then anything else holding the file. A second click must replace the first restart, not
-  // leave two dev servers fighting over a port.
+  // A second click must replace the first restart, not leave two dev servers fighting over a port. Only
+  // the watcher is ours to cancel — the process itself is detached, and is stopped by the kill loop below
+  // like any other holder of the file.
   const previous = restarted.get(taskId);
   if (previous) {
-    try {
-      previous.proc.kill();
-    } catch {
-      /* already dead */
-    }
+    clearInterval(previous.watch);
     restarted.delete(taskId);
   }
 
@@ -179,23 +224,72 @@ export async function restartBackgroundTask(
   // to see. `kill -CONT` brought it back untouched.
   //
   // The child gets its own duplicate of the descriptor at spawn, so closing here costs it nothing.
+  const exitFile = `${output}.exit`;
   const fd = openSync(output, 'a');
-  const argv = who.osUser ? runAsArgv(who.osUser, ['sh', '-c', command]) : ['sh', '-c', command];
+  const inner = detachedArgv(command, exitFile);
+  const argv = who.osUser ? runAsArgv(who.osUser, inner) : inner;
   const proc = Bun.spawn(argv, { cwd, stdout: fd, stderr: fd, stdin: 'ignore' });
   closeSync(fd);
-  restarted.set(taskId, { proc });
 
-  void proc.exited.then((code) => {
-    // Only report the exit of the process we are still tracking — a later restart owns the task by then.
-    if (restarted.get(taskId)?.proc === proc) {
-      restarted.delete(taskId);
-      onExit(code ?? 0);
-    }
-  });
+  // This resolves immediately and says nothing about the job: it is setsid reporting that it forked. A
+  // non-zero status is still worth catching, because it means nothing was started at all — and reporting
+  // `ok` there would leave a chip running forever with no process behind it.
+  const spawnStatus = await proc.exited;
+  if (spawnStatus !== 0) {
+    return { ok: false, reason: `could not start the job — setsid exited ${spawnStatus}` };
+  }
 
+  restarted.set(taskId, { watch: watchForExit(who, taskId, output, exitFile, onExit) });
   return { ok: true, command };
 }
 
+/**
+ * Notice a detached job ending, since nothing is waiting on it any more.
+ *
+ * The question "is it still running" is answered the same way Stop and Pause answer it — who holds the
+ * output file open — so this adds no new notion of liveness, it reuses the one the rest of the module is
+ * already built on. A paused job still holds its file, so `SIGSTOP` reads as running, which is correct.
+ *
+ * The grace period is the only subtle part: between the fork and the job opening its output there is a
+ * moment with no holders, and polling into it would announce a completion for a job that had not started.
+ */
+function watchForExit(
+  who: ChatIdentity,
+  taskId: string,
+  output: string,
+  exitFile: string,
+  onExit: (code: number) => void,
+): ReturnType<typeof setInterval> {
+  const startedAt = Date.now();
+  const timer: ReturnType<typeof setInterval> = setInterval(() => {
+    void (async () => {
+      if (Date.now() - startedAt < WATCH_GRACE_MS) return;
+      if ((await pidsHoldingFile(who, output)).length > 0) return;
+
+      // A later restart owns the task by now; its watcher will do the reporting.
+      if (restarted.get(taskId)?.watch !== timer) {
+        clearInterval(timer);
+        return;
+      }
+      clearInterval(timer);
+      restarted.delete(taskId);
+
+      // Read as the owning account: for a member this file sits in a directory the platform's own uid
+      // cannot even traverse.
+      let code = 0;
+      try {
+        code = Number.parseInt(readTextAs(who.osUser, exitFile).trim(), 10);
+      } catch {
+        // No status file: the job was killed before its wrapper could record one, which is what Stop looks
+        // like. Reported as a clean finish rather than a failure — the pill has to clear either way, and
+        // "you stopped it" is nearer the truth than "it crashed".
+      }
+      onExit(Number.isInteger(code) ? code : 0);
+    })();
+  }, WATCH_INTERVAL_MS);
+  return timer;
+}
+
 /** Whether the platform (rather than the harness) is currently supervising this task. */
 export const isRestarted = (taskId: string): boolean => restarted.has(taskId);