Skip to content

Commit 73669b3

Browse files
t3dotggclaude
andcommitted
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. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
1 parent 331fc71 commit 73669b3

8 files changed

Lines changed: 167 additions & 26 deletions

File tree

‎apps/server/src/cli/auth.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,16 @@ const pairingUrlCommand = Command.make("url", {
210210
? flags
211211
: { ...flags, baseDir: Option.some(worktreeBaseDir) };
212212
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+
);
214223

215224
if (Option.isNone(live)) {
216225
return yield* new NoRunningServerError({ baseDir: config.baseDir });

‎apps/server/src/serverRuntimeState.test.ts‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,30 @@ describe("serverRuntimeState", () => {
7070
);
7171
}),
7272
);
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+
);
7397
});
7498

7599
it.effect("treats a missing runtime state file as absent", () =>

‎apps/server/src/serverRuntimeState.ts‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,11 @@ import { formatHostForUrl, isWildcardHost } from "./startupAccess.ts";
1010

1111
export const PersistedServerRuntimeState = Schema.Struct({
1212
version: Schema.Literal(1),
13-
pid: Schema.Int,
13+
/**
14+
* Positive: `process.kill` reads 0 and negatives as signal-group targets, so
15+
* a liveness probe on one reports "live" for a process that never existed.
16+
*/
17+
pid: Schema.Int.check(Schema.isGreaterThan(0)),
1418
host: Schema.optional(Schema.String),
1519
port: Schema.Int,
1620
origin: Schema.String,
@@ -107,6 +111,13 @@ export const clearPersistedServerRuntimeState = (path: string) =>
107111
);
108112
});
109113

114+
/**
115+
* `process.kill` rejects an out-of-range pid with a TypeError rather than an
116+
* errno, so the catch below cannot assume either shape.
117+
*/
118+
const isErrnoException = (cause: unknown): cause is NodeJS.ErrnoException =>
119+
typeof cause === "object" && cause !== null && "code" in cause;
120+
110121
/**
111122
* Whether the process that wrote a runtime-state file is still alive.
112123
*
@@ -123,12 +134,20 @@ export const isPersistedServerRuntimeStateLive = (
123134
state: PersistedServerRuntimeState,
124135
): Effect.Effect<boolean> =>
125136
Effect.sync(() => {
137+
// `process.kill` reads non-positive pids as signal-group targets rather
138+
// than process lookups: 0 signals the caller's own group and -1 every
139+
// permitted process, so neither throws and both would report "live"
140+
// unconditionally. A server never records one; a corrupt or truncated file
141+
// can, and that file would then be believed forever.
142+
if (!Number.isSafeInteger(state.pid) || state.pid <= 0) {
143+
return false;
144+
}
126145
try {
127146
process.kill(state.pid, 0);
128147
return true;
129148
} catch (cause) {
130149
// EPERM means the process exists but belongs to another user.
131-
return (cause as NodeJS.ErrnoException).code === "EPERM";
150+
return isErrnoException(cause) && cause.code === "EPERM";
132151
}
133152
});
134153

‎scripts/dev-seed.ts‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,13 @@ const MAX_THREAD_LIMIT = 32_766;
2828
class DevSeedTargetError extends Schema.TaggedErrorClass<DevSeedTargetError>()(
2929
"DevSeedTargetError",
3030
{
31-
reason: Schema.Literals(["shared-home", "missing-target", "not-a-worktree", "server-running"]),
31+
reason: Schema.Literals([
32+
"shared-home",
33+
"missing-target",
34+
"not-a-worktree",
35+
"server-running",
36+
"same-source",
37+
]),
3238
detail: Schema.String,
3339
},
3440
) {
@@ -56,6 +62,13 @@ class DevSeedTargetError extends Schema.TaggedErrorClass<DevSeedTargetError>()(
5662
"which leaves the running server's in-memory read model describing rows that no longer exist.",
5763
"Stop it, run the seed, then start it again.",
5864
].join("\n");
65+
case "same-source":
66+
return [
67+
`Refusing to seed ${this.detail} from itself.`,
68+
"The copy would restore what the delete removed, but the event log and the",
69+
"projector cursors are cleared either way — so the directory loses its history",
70+
"and keeps only whatever the thread and activity caps selected.",
71+
].join("\n");
5972
}
6073
}
6174
}
@@ -167,13 +180,21 @@ const devSeedCli = Command.make("dev-seed", {
167180

168181
// The whole point is to keep dev data off the real home; overwriting it
169182
// here would be the exact accident this command exists to avoid.
170-
const [canonicalTarget, canonicalShared] = yield* Effect.all([
183+
const [canonicalTarget, canonicalSource, canonicalShared] = yield* Effect.all([
171184
fileSystem.realPath(targetBaseDir).pipe(Effect.orElseSucceed(() => targetBaseDir)),
185+
fileSystem.realPath(sourceBaseDir).pipe(Effect.orElseSucceed(() => sourceBaseDir)),
172186
fileSystem.realPath(sharedHome).pipe(Effect.orElseSucceed(() => sharedHome)),
173187
]);
174188
if (canonicalTarget === canonicalShared) {
175189
return yield* new DevSeedTargetError({ reason: "shared-home", detail: targetBaseDir });
176190
}
191+
// Seeding a directory from itself is not the data loss it looks like —
192+
// the source reads a pre-transaction snapshot, so the copy puts the rows
193+
// back. What does not come back is the event log and the cursors, and
194+
// anything outside the caps. Nobody means to ask for that.
195+
if (canonicalTarget === canonicalSource) {
196+
return yield* new DevSeedTargetError({ reason: "same-source", detail: targetBaseDir });
197+
}
177198

178199
const targetDbPath = stateDbPath(path, targetBaseDir);
179200
if (!(yield* fileSystem.exists(targetDbPath))) {

‎scripts/lib/dev-seed.test.ts‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -101,8 +101,10 @@ function makeSource(
101101
}
102102
// The newest thread is left mid-turn, as a copy taken while an agent is
103103
// working would be: a running turn with no completion, and a message still
104-
// streaming into it.
104+
// streaming into it. The one before it is left "pending" — a user message
105+
// accepted but whose provider turn never started.
105106
const midFlight = index === threadCount - 1;
107+
const queued = index === threadCount - 2;
106108
database
107109
.prepare(
108110
`INSERT INTO projection_turns (thread_id, turn_id, state, requested_at, completed_at, checkpoint_files_json)
@@ -111,9 +113,9 @@ function makeSource(
111113
.run(
112114
threadId,
113115
`turn-${threadId}`,
114-
midFlight ? "running" : "completed",
116+
midFlight ? "running" : queued ? "pending" : "completed",
115117
at,
116-
midFlight ? null : at,
118+
midFlight || queued ? null : at,
117119
);
118120
database
119121
.prepare(`INSERT INTO projection_thread_sessions VALUES (?,'running','claude',?,'boom',?)`)
@@ -297,10 +299,11 @@ describe("seedDevDatabase", () => {
297299
});
298300
});
299301

300-
// A copied running turn has no agent left to finish it. The session override
301-
// alone does not cover this: the turn is read independently, and an unsettled
302-
// one keeps the thread spinning and its timeline unfoldable forever.
303-
it("settles turns copied mid-flight", () => {
302+
// A copied turn has no agent left to finish it, whether it was already
303+
// running or still queued behind a "pending" row. The session override alone
304+
// does not cover either: the turn is read independently, and an unsettled one
305+
// keeps the thread spinning and its timeline unfoldable forever.
306+
it("settles turns copied before they finished", () => {
304307
withDatabases(({ source, target }) => {
305308
seedDevDatabase({
306309
sourceDbPath: source,
@@ -314,9 +317,11 @@ describe("seedDevDatabase", () => {
314317
target,
315318
"SELECT state, completed_at FROM projection_turns",
316319
);
320+
// The fixture puts one running turn and one pending turn in this range.
317321
assert.isAbove(turns.length, 0);
318322
for (const turn of turns) {
319323
assert.notEqual(turn.state, "running");
324+
assert.notEqual(turn.state, "pending");
320325
assert.isNotNull(turn.completed_at);
321326
}
322327
});

‎scripts/lib/dev-seed.ts‎

Lines changed: 48 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -216,33 +216,45 @@ function copyRows(input: {
216216
return { copied, skipped };
217217
}
218218

219+
/**
220+
* Turn states that describe work an agent still owes: "running" for a turn in
221+
* flight, "pending" for one whose user message was accepted but whose provider
222+
* turn had not started yet (`insertPendingProjectionTurn` in
223+
* apps/server/src/persistence/Layers/ProjectionTurns.ts). Neither can make
224+
* progress in a copy, and only the thread's own next message would clear a
225+
* pending one.
226+
*/
227+
const UNSETTLED_TURN_STATES = ["running", "pending"] as const;
228+
219229
/**
220230
* Settles turns copied mid-flight, matching what the server does to a turn
221231
* whose session leaves "running": `settledTurnStateForSessionStatus` in
222232
* apps/server/src/orchestration/Layers/ProjectionPipeline.ts maps the "stopped"
223233
* status the sessions are copied with to "interrupted".
224234
*
225235
* An UPDATE rather than a copy override, because only the rows that were
226-
* actually running should change; the rest keep their real recorded state.
236+
* actually unsettled should change; the rest keep their real recorded state.
227237
*/
228-
function settleRunningTurns(
238+
function settleUnfinishedTurns(
229239
target: NodeSqlite.DatabaseSync,
230240
threadIds: ReadonlyArray<string>,
231241
): void {
232242
if (threadIds.length === 0 || !hasColumn(target, "projection_turns", "state")) {
233243
return;
234244
}
235-
// completed_at is what marks a turn settled alongside its state; a running
236-
// turn has none, and leaving it null keeps the thread unsettled regardless.
245+
// completed_at is what marks a turn settled alongside its state; an
246+
// unfinished turn has none, and leaving it null keeps the thread unsettled
247+
// regardless of the state written here.
237248
const setCompletedAt = hasColumn(target, "projection_turns", "completed_at")
238249
? `, completed_at = COALESCE(completed_at, requested_at)`
239250
: "";
240251
target
241252
.prepare(
242253
`UPDATE projection_turns SET state = 'interrupted'${setCompletedAt}
243-
WHERE state = 'running' AND thread_id IN (${placeholders(threadIds.length)})`,
254+
WHERE state IN (${placeholders(UNSETTLED_TURN_STATES.length)})
255+
AND thread_id IN (${placeholders(threadIds.length)})`,
244256
)
245-
.run(...threadIds);
257+
.run(...UNSETTLED_TURN_STATES, ...threadIds);
246258
}
247259

248260
export function seedDevDatabase(options: DevSeedOptions): DevSeedSummary {
@@ -267,6 +279,22 @@ export function seedDevDatabase(options: DevSeedOptions): DevSeedSummary {
267279
);
268280
}
269281

282+
// The source is normally the *live* installed app's database — only the
283+
// target is guarded against a running server, because reading is harmless.
284+
// Reading it across a dozen separate implicit transactions is not: the app
285+
// can delete a project between the query that picks it up and the copy that
286+
// reads its row, leaving a copied thread whose project no longer exists (no
287+
// foreign keys are declared, so nothing catches it). One deferred read
288+
// transaction pins every query below to a single snapshot.
289+
let sourceTransactionOpen = false;
290+
try {
291+
source.exec("BEGIN");
292+
sourceTransactionOpen = true;
293+
} catch {
294+
// A concurrent writer can leave the connection unable to start one. The
295+
// copy is still worth doing — it just loses snapshot isolation.
296+
}
297+
270298
try {
271299
// Threads the user actually touched most recently. Mirrors the sidebar's own
272300
// ordering (packages/client-runtime/src/state/threadSort.ts). Both recency
@@ -376,11 +404,11 @@ export function seedDevDatabase(options: DevSeedOptions): DevSeedSummary {
376404
}),
377405
);
378406
// A thread copied mid-turn has no agent to finish it. The session status is
379-
// forced to "stopped" below, but the turn is read independently: a
380-
// `state = 'running'` turn keeps the thread unsettled and unfoldable
381-
// forever (deriveUnsettledTurnId in MessagesTimeline.logic.ts). Interrupted
382-
// is what the server itself settles an abandoned turn to.
383-
settleRunningTurns(target, threadIds);
407+
// forced to "stopped" below, but the turn is read independently: an
408+
// unfinished turn keeps the thread unsettled and unfoldable forever
409+
// (deriveUnsettledTurnId in MessagesTimeline.logic.ts). Interrupted is what
410+
// the server itself settles an abandoned turn to.
411+
settleUnfinishedTurns(target, threadIds);
384412
const messages = record(
385413
copyRows({
386414
source,
@@ -475,6 +503,15 @@ export function seedDevDatabase(options: DevSeedOptions): DevSeedSummary {
475503
? cause
476504
: new DevSeedError(`could not seed the dev database: ${String(cause)}`);
477505
} finally {
506+
if (sourceTransactionOpen) {
507+
try {
508+
// Read-only, so there is nothing to commit; end it before closing so
509+
// the snapshot is released explicitly rather than by teardown.
510+
source.exec("ROLLBACK");
511+
} catch {
512+
// Already ended, which is equally fine for a read-only transaction.
513+
}
514+
}
478515
source.close();
479516
target.close();
480517
}

‎scripts/lib/server-runtime-probe.test.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,20 @@ describe("findRunningServerPid", () => {
8686
}).pipe(Effect.provide(NodeServices.layer)),
8787
);
8888

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+
89103
it.effect("reports nothing when no state files exist", () =>
90104
Effect.gen(function* () {
91105
const baseDir = yield* makeBaseDir();

‎scripts/lib/server-runtime-probe.ts‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,8 @@ import * as Schema from "effect/Schema";
1616
const STATE_DIRS = ["dev", "userdata"] as const;
1717

1818
const RuntimeStateProbe = Schema.Struct({
19-
pid: Schema.Int,
19+
/** Positive, for the reason `isPidLive` explains. */
20+
pid: Schema.Int.check(Schema.isGreaterThan(0)),
2021
});
2122

2223
const decodeRuntimeStateProbe = Schema.decodeUnknownEffect(
@@ -34,12 +35,23 @@ const decodeRuntimeStateProbe = Schema.decodeUnknownEffect(
3435
* case is being told to stop a server that is already stopped.
3536
*/
3637
const isPidLive = (pid: number): boolean => {
38+
// `process.kill` reads non-positive pids as signal-group targets rather than
39+
// process lookups — 0 signals the caller's own group, -1 every permitted
40+
// process — so neither throws and both would report "live" unconditionally.
41+
// The schema rejects those, and this repeats the check so the invariant
42+
// travels with the code that depends on it.
43+
if (!Number.isSafeInteger(pid) || pid <= 0) {
44+
return false;
45+
}
3746
try {
3847
process.kill(pid, 0);
3948
return true;
4049
} catch (cause) {
41-
// EPERM means the process exists but belongs to another user.
42-
return (cause as NodeJS.ErrnoException).code === "EPERM";
50+
// EPERM means the process exists but belongs to another user. An
51+
// out-of-range pid throws a TypeError instead, which has no errno.
52+
return typeof cause === "object" && cause !== null && "code" in cause
53+
? cause.code === "EPERM"
54+
: false;
4355
}
4456
};
4557

0 commit comments

Comments
 (0)