Fix liveness probe, --json output, and remaining unsettled state

Follow-up review of the pairing path and a second pass over seeding: - `isPersistedServerRuntimeStateLive` reported non-positive pids as live. `process.kill` reads 0 and -1 as signal-group targets rather than process lookups, so neither throws and both were caught by the "it didn't throw, so it's running" branch. A truncated or hand-edited file recording either would be believed forever — the dev candidate wins the devUrl preference, so it also outranks a genuinely live server elsewhere and mints tokens into a database nothing reads. Rejected in the schema and re-checked at the call site; the errno cast is now a guard, since an out-of-range pid throws a TypeError with no `code`. - `pairing url --json` emitted warnings on stdout ahead of the payload. The probe reads both state directories, so a corrupt file in the one *not* in use broke JSON for any consumer piping it; the quiet-logs layer was only applied around the issuing step that runs after. Verified with a crafted fixture: stdout is now valid JSON. - Turns copied in the `pending` state were left unsettled. Same defect as the `running` case fixed previously — a user message accepted before its provider turn started, which only that thread's next message would ever clear. - The source was read across a dozen implicit transactions while the installed app may be writing to it, so a project could vanish between the query that selected it and the copy that read its row. One deferred read transaction pins every query to a single snapshot. - `--from` and `--to` resolving to the same directory is now refused. Not for the data loss it appears to be (the source reads a pre-transaction snapshot, so the rows come back — verified in both journal modes), but because the event log, the cursors, and everything outside the caps do not.

Theo Browne committed 3 months ago

Commit 8a76bb81 parent 373e7918 files · +167 −26Browse files

Unified · Split

8 files

apps/server/src/cli/auth.ts +10 −1
apps/server/src/serverRuntimeState.test.ts +24 −0
apps/server/src/serverRuntimeState.ts +21 −2
scripts/dev-seed.ts +23 −2
scripts/lib/dev-seed.test.ts +12 −7
scripts/lib/dev-seed.ts +48 −11
scripts/lib/server-runtime-probe.test.ts +14 −0
scripts/lib/server-runtime-probe.ts +15 −3

apps/server/src/cli/auth.ts+10−1

@@ -210,7 +210,16 @@ const pairingUrlCommand = Command.make("url", { · 209 unmodified lines
210	? flags	210	? flags
211	: { ...flags, baseDir: Option.some(worktreeBaseDir) };	211	: { ...flags, baseDir: Option.some(worktreeBaseDir) };
212	const config = yield* resolveCliAuthConfig(resolvedFlags, logLevel);	212	const config = yield* resolveCliAuthConfig(resolvedFlags, logLevel);
213	const live = yield* findLiveServerRuntimeState(config);	213	// Quieted here rather than only around the issuing layer below: probing
		214	// reads both state directories, and a corrupt file in the one not in
		215	// use logs a Warn to stdout — ahead of the payload, breaking `--json`
		216	// for any consumer piping it. The warning is about a file the command
		217	// then ignores, so under --json it is noise either way.
		218	const live = yield* findLiveServerRuntimeState(config).pipe(
		219	Effect.provide(
		220	Layer.succeed(References.MinimumLogLevel, flags.json ? "Error" : config.logLevel),
		221	),
		222	);
214		223
215	if (Option.isNone(live)) {	224	if (Option.isNone(live)) {
216	return yield* new NoRunningServerError({ baseDir: config.baseDir });	225	return yield* new NoRunningServerError({ baseDir: config.baseDir });

apps/server/src/serverRuntimeState.test.ts+24−0

@@ -70,6 +70,30 @@ describe("serverRuntimeState", () => { · 69 unmodified lines
70	);	70	);
71	}),	71	}),
72	);	72	);
		73
		74	// `process.kill` reads non-positive pids as signal-group targets rather
		75	// than process lookups — 0 is the caller's own group, -1 every permitted
		76	// process — so neither throws, and a probe that only catches would call
		77	// both "live" no matter what wrote the file.
		78	for (const pid of [0, -1]) {
		79	it.effect(`reports pid ${String(pid)} as dead rather than signalling a group`, () =>
		80	Effect.gen(function* () {
		81	assert.isFalse(
		82	yield* ServerRuntimeState.isPersistedServerRuntimeStateLive(stateForPid(pid)),
		83	);
		84	}),
		85	);
		86	}
		87
		88	// Out of range for a pid: process.kill throws a TypeError with no errno,
		89	// which the EPERM check must not assume the shape of.
		90	it.effect("reports an out-of-range pid as dead", () =>
		91	Effect.gen(function* () {
		92	assert.isFalse(
		93	yield* ServerRuntimeState.isPersistedServerRuntimeStateLive(stateForPid(2 ** 40)),
		94	);
		95	}),
		96	);
73	});	97	});
74		98
75	it.effect("treats a missing runtime state file as absent", () =>	99	it.effect("treats a missing runtime state file as absent", () =>

apps/server/src/serverRuntimeState.ts+21−2

 export const PersistedServerRuntimeState = Schema.Struct({
   version: Schema.Literal(1),
-  pid: Schema.Int,
+  /**
+   * Positive: `process.kill` reads 0 and negatives as signal-group targets, so
+   * a liveness probe on one reports "live" for a process that never existed.
+   */
+  pid: Schema.Int.check(Schema.isGreaterThan(0)),
   host: Schema.optional(Schema.String),
   port: Schema.Int,
   origin: Schema.String,
     );
   });
+/**
+ * `process.kill` rejects an out-of-range pid with a TypeError rather than an
+ * errno, so the catch below cannot assume either shape.
+ */
+const isErrnoException = (cause: unknown): cause is NodeJS.ErrnoException =>
+  typeof cause === "object" && cause !== null && "code" in cause;
 /**
  * Whether the process that wrote a runtime-state file is still alive.
  *
   state: PersistedServerRuntimeState,
 ): Effect.Effect<boolean> =>
   Effect.sync(() => {
+    // `process.kill` reads non-positive pids as signal-group targets rather
+    // than process lookups: 0 signals the caller's own group and -1 every
+    // permitted process, so neither throws and both would report "live"
+    // unconditionally. A server never records one; a corrupt or truncated file
+    // can, and that file would then be believed forever.
+    if (!Number.isSafeInteger(state.pid) || state.pid <= 0) {
+      return false;
+    }
     try {
       process.kill(state.pid, 0);
       return true;
     } catch (cause) {
       // EPERM means the process exists but belongs to another user.
-      return (cause as NodeJS.ErrnoException).code === "EPERM";
+      return isErrnoException(cause) && cause.code === "EPERM";
     }
   });

scripts/dev-seed.ts+23−2

 class DevSeedTargetError extends Schema.TaggedErrorClass<DevSeedTargetError>()(
   "DevSeedTargetError",
   {
-    reason: Schema.Literals(["shared-home", "missing-target", "not-a-worktree", "server-running"]),
+    reason: Schema.Literals([
+      "shared-home",
+      "missing-target",
+      "not-a-worktree",
+      "server-running",
+      "same-source",
+    ]),
     detail: Schema.String,
   },
 ) {
           "which leaves the running server's in-memory read model describing rows that no longer exist.",
           "Stop it, run the seed, then start it again.",
         ].join("\n");
+      case "same-source":
+        return [
+          `Refusing to seed ${this.detail} from itself.`,
+          "The copy would restore what the delete removed, but the event log and the",
+          "projector cursors are cleared either way — so the directory loses its history",
+          "and keeps only whatever the thread and activity caps selected.",
+        ].join("\n");
     }
   }
 }
       // The whole point is to keep dev data off the real home; overwriting it
       // here would be the exact accident this command exists to avoid.
-      const [canonicalTarget, canonicalShared] = yield* Effect.all([
+      const [canonicalTarget, canonicalSource, canonicalShared] = yield* Effect.all([
         fileSystem.realPath(targetBaseDir).pipe(Effect.orElseSucceed(() => targetBaseDir)),
+        fileSystem.realPath(sourceBaseDir).pipe(Effect.orElseSucceed(() => sourceBaseDir)),
         fileSystem.realPath(sharedHome).pipe(Effect.orElseSucceed(() => sharedHome)),
       ]);
       if (canonicalTarget === canonicalShared) {
         return yield* new DevSeedTargetError({ reason: "shared-home", detail: targetBaseDir });
       }
+      // Seeding a directory from itself is not the data loss it looks like —
+      // the source reads a pre-transaction snapshot, so the copy puts the rows
+      // back. What does not come back is the event log and the cursors, and
+      // anything outside the caps. Nobody means to ask for that.
+      if (canonicalTarget === canonicalSource) {
+        return yield* new DevSeedTargetError({ reason: "same-source", detail: targetBaseDir });
+      }
       const targetDbPath = stateDbPath(path, targetBaseDir);
       if (!(yield* fileSystem.exists(targetDbPath))) {

scripts/lib/dev-seed.test.ts+12−7

     }
     // The newest thread is left mid-turn, as a copy taken while an agent is
     // working would be: a running turn with no completion, and a message still
-    // streaming into it.
+    // streaming into it. The one before it is left "pending" — a user message
+    // accepted but whose provider turn never started.
     const midFlight = index === threadCount - 1;
+    const queued = index === threadCount - 2;
     database
       .prepare(
         `INSERT INTO projection_turns (thread_id, turn_id, state, requested_at, completed_at, checkpoint_files_json)
       .run(
         threadId,
         `turn-${threadId}`,
-        midFlight ? "running" : "completed",
+        midFlight ? "running" : queued ? "pending" : "completed",
         at,
-        midFlight ? null : at,
+        midFlight || queued ? null : at,
       );
     database
       .prepare(`INSERT INTO projection_thread_sessions VALUES (?,'running','claude',?,'boom',?)`)
     });
   });
-  // A copied running turn has no agent left to finish it. The session override
+  // A copied turn has no agent left to finish it, whether it was already
-  // alone does not cover this: the turn is read independently, and an unsettled
+  // running or still queued behind a "pending" row. The session override alone
-  // one keeps the thread spinning and its timeline unfoldable forever.
+  // does not cover either: the turn is read independently, and an unsettled one
-  it("settles turns copied mid-flight", () => {
+  // keeps the thread spinning and its timeline unfoldable forever.
+  it("settles turns copied before they finished", () => {
     withDatabases(({ source, target }) => {
       seedDevDatabase({
         sourceDbPath: source,
         target,
         "SELECT state, completed_at FROM projection_turns",
       );
+      // The fixture puts one running turn and one pending turn in this range.
       assert.isAbove(turns.length, 0);
       for (const turn of turns) {
         assert.notEqual(turn.state, "running");
+        assert.notEqual(turn.state, "pending");
         assert.isNotNull(turn.completed_at);
       }
     });

scripts/lib/dev-seed.ts+48−11

   return { copied, skipped };
 }
+/**
+ * Turn states that describe work an agent still owes: "running" for a turn in
+ * flight, "pending" for one whose user message was accepted but whose provider
+ * turn had not started yet (`insertPendingProjectionTurn` in
+ * apps/server/src/persistence/Layers/ProjectionTurns.ts). Neither can make
+ * progress in a copy, and only the thread's own next message would clear a
+ * pending one.
+ */
+const UNSETTLED_TURN_STATES = ["running", "pending"] as const;
 /**
  * Settles turns copied mid-flight, matching what the server does to a turn
  * whose session leaves "running": `settledTurnStateForSessionStatus` in
  * status the sessions are copied with to "interrupted".
  *
  * An UPDATE rather than a copy override, because only the rows that were
- * actually running should change; the rest keep their real recorded state.
+ * actually unsettled should change; the rest keep their real recorded state.
  */
-function settleRunningTurns(
+function settleUnfinishedTurns(
   target: NodeSqlite.DatabaseSync,
   threadIds: ReadonlyArray<string>,
 ): void {
   if (threadIds.length === 0 || !hasColumn(target, "projection_turns", "state")) {
     return;
   }
-  // completed_at is what marks a turn settled alongside its state; a running
+  // completed_at is what marks a turn settled alongside its state; an
-  // turn has none, and leaving it null keeps the thread unsettled regardless.
+  // unfinished turn has none, and leaving it null keeps the thread unsettled
+  // regardless of the state written here.
   const setCompletedAt = hasColumn(target, "projection_turns", "completed_at")
     ? `, completed_at = COALESCE(completed_at, requested_at)`
     : "";
   target
     .prepare(
       `UPDATE projection_turns SET state = 'interrupted'${setCompletedAt}
-       WHERE state = 'running' AND thread_id IN (${placeholders(threadIds.length)})`,
+       WHERE state IN (${placeholders(UNSETTLED_TURN_STATES.length)})
+         AND thread_id IN (${placeholders(threadIds.length)})`,
     )
-    .run(...threadIds);
+    .run(...UNSETTLED_TURN_STATES, ...threadIds);
 }
 export function seedDevDatabase(options: DevSeedOptions): DevSeedSummary {
     );
   }
+  // The source is normally the *live* installed app's database — only the
+  // target is guarded against a running server, because reading is harmless.
+  // Reading it across a dozen separate implicit transactions is not: the app
+  // can delete a project between the query that picks it up and the copy that
+  // reads its row, leaving a copied thread whose project no longer exists (no
+  // foreign keys are declared, so nothing catches it). One deferred read
+  // transaction pins every query below to a single snapshot.
+  let sourceTransactionOpen = false;
+  try {
+    source.exec("BEGIN");
+    sourceTransactionOpen = true;
+  } catch {
+    // A concurrent writer can leave the connection unable to start one. The
+    // copy is still worth doing — it just loses snapshot isolation.
+  }
   try {
     // Threads the user actually touched most recently. Mirrors the sidebar's own
     // ordering (packages/client-runtime/src/state/threadSort.ts). Both recency
       }),
     );
     // A thread copied mid-turn has no agent to finish it. The session status is
-    // forced to "stopped" below, but the turn is read independently: a
+    // forced to "stopped" below, but the turn is read independently: an
-    // `state = 'running'` turn keeps the thread unsettled and unfoldable
+    // unfinished turn keeps the thread unsettled and unfoldable forever
-    // forever (deriveUnsettledTurnId in MessagesTimeline.logic.ts). Interrupted
+    // (deriveUnsettledTurnId in MessagesTimeline.logic.ts). Interrupted is what
-    // is what the server itself settles an abandoned turn to.
+    // the server itself settles an abandoned turn to.
-    settleRunningTurns(target, threadIds);
+    settleUnfinishedTurns(target, threadIds);
     const messages = record(
       copyRows({
         source,
       ? cause
       : new DevSeedError(`could not seed the dev database: ${String(cause)}`);
   } finally {
+    if (sourceTransactionOpen) {
+      try {
+        // Read-only, so there is nothing to commit; end it before closing so
+        // the snapshot is released explicitly rather than by teardown.
+        source.exec("ROLLBACK");
+      } catch {
+        // Already ended, which is equally fine for a read-only transaction.
+      }
+    }
     source.close();
     target.close();
   }

scripts/lib/server-runtime-probe.test.ts+14−0

@@ -86,6 +86,20 @@ describe("findRunningServerPid", () => { · 85 unmodified lines
86	}).pipe(Effect.provide(NodeServices.layer)),	86	}).pipe(Effect.provide(NodeServices.layer)),
87	);	87	);
88		88
		89	// `process.kill` reads these as signal-group targets, not process lookups, so
		90	// neither throws — a naive probe calls both "live" and blocks seeding forever
		91	// on a file no server ever wrote.
		92	for (const pid of [0, -1]) {
		93	it.effect(`ignores a state file recording pid ${String(pid)}`, () =>
		94	Effect.gen(function* () {
		95	const baseDir = yield* makeBaseDir();
		96	yield* writeRuntimeState({ baseDir, stateDir: "dev", contents: runtimeStateJson(pid) });
		97
		98	assert.isTrue(Option.isNone(yield* findRunningServerPid(baseDir)));
		99	}).pipe(Effect.provide(NodeServices.layer)),
		100	);
		101	}
		102
89	it.effect("reports nothing when no state files exist", () =>	103	it.effect("reports nothing when no state files exist", () =>
90	Effect.gen(function* () {	104	Effect.gen(function* () {
91	const baseDir = yield* makeBaseDir();	105	const baseDir = yield* makeBaseDir();

scripts/lib/server-runtime-probe.ts+15−3

 const STATE_DIRS = ["dev", "userdata"] as const;
 const RuntimeStateProbe = Schema.Struct({
-  pid: Schema.Int,
+  /** Positive, for the reason `isPidLive` explains. */
+  pid: Schema.Int.check(Schema.isGreaterThan(0)),
 });
 const decodeRuntimeStateProbe = Schema.decodeUnknownEffect(
  * case is being told to stop a server that is already stopped.
  */
 const isPidLive = (pid: number): boolean => {
+  // `process.kill` reads non-positive pids as signal-group targets rather than
+  // process lookups — 0 signals the caller's own group, -1 every permitted
+  // process — so neither throws and both would report "live" unconditionally.
+  // The schema rejects those, and this repeats the check so the invariant
+  // travels with the code that depends on it.
+  if (!Number.isSafeInteger(pid) || pid <= 0) {
+    return false;
+  }
   try {
     process.kill(pid, 0);
     return true;
   } catch (cause) {
-    // EPERM means the process exists but belongs to another user.
+    // EPERM means the process exists but belongs to another user. An
-    return (cause as NodeJS.ErrnoException).code === "EPERM";
+    // out-of-range pid throws a TypeError instead, which has no errno.
+    return typeof cause === "object" && cause !== null && "code" in cause
+      ? cause.code === "EPERM"
+      : false;
   }
 };