Skip to content

Commit cfbfbd7

Browse files
committed
fix(coding-agent): restore attach/catch-up ordering invariants and sweep snapshot atomicity
- Finding 22: async snapshot creation exposed clients before attach initialization; publish client attachment only after the snapshot resolves. origin: PR #587 - Finding 23: catch-up validity was checked only before async snapshot creation; revalidate session and client attachment after the await. origin: PR #587 - Finding 25: deterministic ambiguous selectors entered the 30-second remote-send retry loop; fail fast for Ambiguous errors. origin: PR #588 - Finding 28: active rows ignored stable file-keyed schedule pins during active-id rebinding; consult both id- and file-keyed heartbeat/cron sets. origin: PR #588 - Finding 29: passivation sampled activity before an asynchronous passive-registry walk; complete the walk before atomically sampling summary and admission markers. origin: PR #589
1 parent 2334d6e commit cfbfbd7

4 files changed

Lines changed: 220 additions & 11 deletions

File tree

packages/coding-agent/src/modes/daemon/daemon-mode.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2289,6 +2289,7 @@ export class AgentDaemon {
22892289
state: ActiveSessionState,
22902290
passiveRlmSubagents?: readonly PassiveRlmSubagent[],
22912291
): Promise<SessionPassivationSnapshot> {
2292+
const passiveDescendants = passiveRlmSubagents ?? (await this.listPassiveRlmSubagents());
22922293
const summary = summaryForActiveSession(state);
22932294
const sessionFile = state.runtime.session.sessionFile;
22942295
const jobs = this.cronStore
@@ -2299,7 +2300,6 @@ export class AgentDaemon {
22992300
job.status !== "cancelled" &&
23002301
job.status !== "completed",
23012302
);
2302-
const passiveDescendants = passiveRlmSubagents ?? (await this.listPassiveRlmSubagents());
23032303
const hasPendingAdmission =
23042304
this.agentMessageAcceptingTargets.has(state.activeSessionId) ||
23052305
this.agentMessagePreparingTargets.has(state.activeSessionId) ||
@@ -3264,20 +3264,18 @@ export class AgentDaemon {
32643264
const snapshotSignal = streamsSnapshot
32653265
? markClientSnapshotStreaming(client, state.activeSessionId)
32663266
: undefined;
3267-
state.clients.add(client);
3268-
client.attachedActiveSessionIds.add(state.activeSessionId);
32693267
let result: DaemonAttachResult;
32703268
try {
32713269
result = await this.createAttachResult(client, state, command);
32723270
} catch (error) {
3273-
state.clients.delete(client);
3274-
client.attachedActiveSessionIds.delete(state.activeSessionId);
32753271
removeDaemonClientSessionCapabilities(client, state.activeSessionId);
32763272
if (streamsSnapshot) {
32773273
finishClientSnapshotStreaming(client, state.activeSessionId);
32783274
}
32793275
throw error;
32803276
}
3277+
state.clients.add(client);
3278+
client.attachedActiveSessionIds.add(state.activeSessionId);
32813279
if (deferClientEnv && clientEnv) {
32823280
this.updateRestart?.deferredClientEnv.push({ client, state, env: clientEnv });
32833281
}
@@ -4733,7 +4731,10 @@ export class AgentDaemon {
47334731
return response.data as AgentSessionMessageReceipt;
47344732
} catch (error) {
47354733
lastError = error;
4736-
if (error instanceof Error && error.message.startsWith("Unknown active session:")) {
4734+
if (
4735+
error instanceof Error &&
4736+
(error.message.startsWith("Unknown active session:") || error.message.startsWith("Ambiguous"))
4737+
) {
47374738
throw error;
47384739
}
47394740
} finally {
@@ -5489,6 +5490,9 @@ export class AgentDaemon {
54895490
type: "attach",
54905491
activeSessionId,
54915492
});
5493+
if (this.sessions.get(activeSessionId) !== state || !state.clients.has(client)) {
5494+
continue;
5495+
}
54925496
if (
54935497
client.transport === "private-framed" &&
54945498
daemonClientCapabilitiesForSession(client, activeSessionId).has("chunked_snapshot")

packages/coding-agent/src/modes/daemon/daemon-session-list.ts

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,10 @@ export function buildSessionList(
144144
activeSession,
145145
savedSession,
146146
heartbeatSessionIds.has(activeSession.activeSessionId),
147-
registeredHeartbeatSessionIds.has(activeSession.activeSessionId),
148-
registeredCronSessionIds.has(activeSession.activeSessionId),
147+
registeredHeartbeatSessionIds.has(activeSession.activeSessionId) ||
148+
registeredHeartbeatSessionFiles.has(sessionFile),
149+
registeredCronSessionIds.has(activeSession.activeSessionId) ||
150+
registeredCronSessionFiles.has(sessionFile),
149151
),
150152
);
151153
seenActiveSessionIds.add(activeSession.activeSessionId);
@@ -162,13 +164,17 @@ export function buildSessionList(
162164

163165
for (const activeSession of activeSessions) {
164166
if (!seenActiveSessionIds.has(activeSession.activeSessionId)) {
167+
const sessionFile = activeSession.runtime.session.sessionFile;
168+
const resolvedSessionFile = sessionFile ? resolve(sessionFile) : undefined;
165169
entries.push(
166170
summaryForActiveSession(
167171
activeSession,
168172
undefined,
169173
heartbeatSessionIds.has(activeSession.activeSessionId),
170-
registeredHeartbeatSessionIds.has(activeSession.activeSessionId),
171-
registeredCronSessionIds.has(activeSession.activeSessionId),
174+
registeredHeartbeatSessionIds.has(activeSession.activeSessionId) ||
175+
(resolvedSessionFile !== undefined && registeredHeartbeatSessionFiles.has(resolvedSessionFile)),
176+
registeredCronSessionIds.has(activeSession.activeSessionId) ||
177+
(resolvedSessionFile !== undefined && registeredCronSessionFiles.has(resolvedSessionFile)),
172178
),
173179
);
174180
}

packages/coding-agent/test/daemon-mode.test.ts

Lines changed: 176 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { EventEmitter } from "node:events";
22
import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
3-
import type { Socket } from "node:net";
3+
import { createServer, type Socket } from "node:net";
44
import { tmpdir } from "node:os";
55
import { join } from "node:path";
66
import type { Api, Model } from "@earendil-works/pi-ai";
@@ -31,9 +31,13 @@ import {
3131
} from "../src/modes/daemon/daemon-mode.js";
3232
import {
3333
createDaemonCommandEnvelope,
34+
DAEMON_PROTOCOL_INFO,
35+
DAEMON_SCHEMA_REVISION,
3436
type DaemonAttachResult,
3537
type DaemonCommand,
38+
failure,
3639
} from "../src/modes/daemon/daemon-protocol.js";
40+
import { DAEMON_WORKER_SUPERVISOR_SOCKET_ENV } from "../src/modes/daemon/daemon-worker-protocol.js";
3741

3842
describe("daemon mode helpers", () => {
3943
it("preserves envelope client identity while registering prompt admission", () => {
@@ -1018,6 +1022,64 @@ describe("daemon mode helpers", () => {
10181022
expect(sendRemoteAgentSessionMessage).toHaveBeenCalledWith(source, "deleted-child", "continue", undefined);
10191023
});
10201024

1025+
it("does not retry permanent ambiguity errors from the supervisor", async () => {
1026+
const tempDir = mkdtempSync(join(tmpdir(), "pa-ambiguous-"));
1027+
const socketPath = join(tempDir, "s");
1028+
let requestCount = 0;
1029+
const server = createServer((socket) => {
1030+
socket.write(
1031+
`${JSON.stringify({
1032+
type: "daemon_hello",
1033+
socketPath,
1034+
protocol: DAEMON_PROTOCOL_INFO,
1035+
schemaRevision: DAEMON_SCHEMA_REVISION,
1036+
serverCapabilities: [],
1037+
})}\n`,
1038+
);
1039+
let buffered = "";
1040+
socket.on("data", (chunk: Buffer) => {
1041+
buffered += chunk.toString("utf8");
1042+
const newline = buffered.indexOf("\n");
1043+
if (newline < 0 || requestCount > 0) return;
1044+
const command = JSON.parse(buffered.slice(0, newline)) as { id?: string };
1045+
requestCount++;
1046+
socket.write(
1047+
`${JSON.stringify(failure(command.id, "send_message", new Error('Ambiguous session selector "duplicate"')))}\n`,
1048+
);
1049+
});
1050+
});
1051+
const previousSocketPath = process.env[DAEMON_WORKER_SUPERVISOR_SOCKET_ENV];
1052+
try {
1053+
await new Promise<void>((resolve, reject) => {
1054+
server.once("error", reject);
1055+
server.listen(socketPath, resolve);
1056+
});
1057+
process.env[DAEMON_WORKER_SUPERVISOR_SOCKET_ENV] = socketPath;
1058+
const daemon = new AgentDaemon(join(tempDir, "worker.sock"), {
1059+
defaultSessionConfig: { agentDir: tempDir, cwd: tempDir },
1060+
createRuntime: vi.fn(),
1061+
});
1062+
const source = makeState("source");
1063+
const internals = daemon as unknown as {
1064+
sendRemoteAgentSessionMessage(
1065+
fromState: ActiveSessionState,
1066+
targetSelector: string,
1067+
message: string,
1068+
): Promise<unknown>;
1069+
};
1070+
1071+
await expect(internals.sendRemoteAgentSessionMessage(source, "duplicate", "hello")).rejects.toThrow(
1072+
'Ambiguous session selector "duplicate"',
1073+
);
1074+
expect(requestCount).toBe(1);
1075+
} finally {
1076+
if (previousSocketPath === undefined) delete process.env[DAEMON_WORKER_SUPERVISOR_SOCKET_ENV];
1077+
else process.env[DAEMON_WORKER_SUPERVISOR_SOCKET_ENV] = previousSocketPath;
1078+
await new Promise<void>((resolve) => server.close(() => resolve()));
1079+
rmSync(tempDir, { recursive: true, force: true });
1080+
}
1081+
});
1082+
10211083
it("reports queued status when a direct accept races into the queue", async () => {
10221084
const daemon = new AgentDaemon("/tmp/prime-agent-test.sock", {
10231085
defaultSessionConfig: { agentDir: "/tmp/prime-agent-test-agent", cwd: "/tmp" },
@@ -3136,6 +3198,84 @@ describe("daemon mode helpers", () => {
31363198
expect(client.catchupActiveSessionIds).toEqual(new Set());
31373199
});
31383200

3201+
it("does not attach a non-chunked client until its snapshot is ready", async () => {
3202+
const daemon = new AgentDaemon("/tmp/prime-agent-test.sock", {
3203+
defaultSessionConfig: { agentDir: "/tmp/prime-agent-test-agent", cwd: "/tmp" },
3204+
createRuntime: vi.fn(),
3205+
});
3206+
const state = makeState("active");
3207+
const client = makeClient("client-1", state.activeSessionId);
3208+
client.attachedActiveSessionIds.clear();
3209+
let releaseSnapshot!: () => void;
3210+
const snapshotGate = new Promise<void>((resolve) => {
3211+
releaseSnapshot = resolve;
3212+
});
3213+
const result = {
3214+
activeSessionId: state.activeSessionId,
3215+
snapshot: { summary: {}, state: {}, messages: [] },
3216+
lastEventSequence: 0,
3217+
} as unknown as DaemonAttachResult;
3218+
const internals = daemon as unknown as {
3219+
sessions: Map<string, ActiveSessionState>;
3220+
createAttachResult: ReturnType<typeof vi.fn>;
3221+
handleCommand(client: DaemonSocketClient, command: DaemonCommand): Promise<unknown>;
3222+
};
3223+
internals.sessions.set(state.activeSessionId, state);
3224+
internals.createAttachResult = vi.fn(async () => {
3225+
await snapshotGate;
3226+
return result;
3227+
});
3228+
3229+
const attach = internals.handleCommand(client, { type: "attach", activeSessionId: state.activeSessionId });
3230+
await vi.waitFor(() => expect(internals.createAttachResult).toHaveBeenCalledOnce());
3231+
expect(state.clients).not.toContain(client);
3232+
expect(client.attachedActiveSessionIds).not.toContain(state.activeSessionId);
3233+
releaseSnapshot();
3234+
await attach;
3235+
expect(state.clients).toContain(client);
3236+
expect(client.attachedActiveSessionIds).toContain(state.activeSessionId);
3237+
});
3238+
3239+
it("drops a backpressure catch-up when the client detaches during snapshot creation", async () => {
3240+
const daemon = new AgentDaemon("/tmp/prime-agent-test.sock", {
3241+
defaultSessionConfig: { agentDir: "/tmp/prime-agent-test-agent", cwd: "/tmp" },
3242+
createRuntime: vi.fn(),
3243+
});
3244+
const state = makeState("active");
3245+
const write = vi.fn(() => true);
3246+
const client = makeClient("client-1", state.activeSessionId);
3247+
client.socket = { destroyed: false, write } as unknown as Socket;
3248+
client.catchupActiveSessionIds = new Set([state.activeSessionId]);
3249+
state.clients.add(client);
3250+
let releaseSnapshot!: () => void;
3251+
const snapshotGate = new Promise<void>((resolve) => {
3252+
releaseSnapshot = resolve;
3253+
});
3254+
const result = {
3255+
activeSessionId: state.activeSessionId,
3256+
snapshot: { summary: {}, state: {}, messages: [], lastEventSequence: 0 },
3257+
lastEventSequence: 0,
3258+
} as unknown as DaemonAttachResult;
3259+
const internals = daemon as unknown as {
3260+
sessions: Map<string, ActiveSessionState>;
3261+
createAttachResult: ReturnType<typeof vi.fn>;
3262+
drainBackpressuredClientCatchups(client: DaemonSocketClient): Promise<void>;
3263+
};
3264+
internals.sessions.set(state.activeSessionId, state);
3265+
internals.createAttachResult = vi.fn(async () => {
3266+
await snapshotGate;
3267+
return result;
3268+
});
3269+
3270+
const catchup = internals.drainBackpressuredClientCatchups(client);
3271+
await vi.waitFor(() => expect(internals.createAttachResult).toHaveBeenCalledOnce());
3272+
state.clients.delete(client);
3273+
client.attachedActiveSessionIds.delete(state.activeSessionId);
3274+
releaseSnapshot();
3275+
await catchup;
3276+
expect(write).not.toHaveBeenCalled();
3277+
});
3278+
31393279
it("marks a chunked attach as snapshotting before deferred streaming", async () => {
31403280
const tempDir = mkdtempSync(join(tmpdir(), "prime-agent-daemon-snapshot-order-"));
31413281
try {
@@ -4034,6 +4174,41 @@ describe("daemon mode helpers", () => {
40344174
}
40354175
});
40364176

4177+
it("does not passivate a child that starts streaming during the fence snapshot", async () => {
4178+
const tempDir = mkdtempSync(join(tmpdir(), "prime-agent-daemon-passivation-stream-race-"));
4179+
try {
4180+
const fixture = makePersistedRlmDaemonFixture(tempDir);
4181+
const internals = fixture.daemon as unknown as {
4182+
createRuntime(command: Extract<DaemonCommand, { type: "create" }>): Promise<ActiveSessionState>;
4183+
listPassiveRlmSubagents: ReturnType<typeof vi.fn>;
4184+
passivateIdleChildren(threshold: number, now: number, limit: number): Promise<number>;
4185+
};
4186+
const parentState = await internals.createRuntime({ type: "create", sessionPath: fixture.parentSessionFile });
4187+
const childState = await internals.createRuntime({ type: "create", sessionPath: fixture.childSessionFile });
4188+
const childSession = childState.runtime.session as unknown as {
4189+
isStreaming: boolean;
4190+
isSessionActive: boolean;
4191+
abort: ReturnType<typeof vi.fn>;
4192+
};
4193+
let passiveListCalls = 0;
4194+
internals.listPassiveRlmSubagents = vi.fn(async () => {
4195+
passiveListCalls++;
4196+
if (passiveListCalls === 2) {
4197+
childSession.isStreaming = true;
4198+
childSession.isSessionActive = true;
4199+
}
4200+
return [];
4201+
});
4202+
4203+
await expect(internals.passivateIdleChildren(90, Date.parse("2036-08-01T12:00:00Z"), 1)).resolves.toBe(0);
4204+
expect(passiveListCalls).toBe(2);
4205+
expect(childSession.abort).not.toHaveBeenCalled();
4206+
expect(parentState.runtime.session.releaseFinishedRlmChildSession).not.toHaveBeenCalled();
4207+
} finally {
4208+
rmSync(tempDir, { recursive: true, force: true });
4209+
}
4210+
});
4211+
40374212
it("limits each worker sweep and leaves non-leaf children resident", async () => {
40384213
const daemon = new AgentDaemon("/tmp/prime-agent-passivation-cap.sock", {
40394214
defaultSessionConfig: { agentDir: "/tmp", cwd: "/tmp" },

packages/coding-agent/test/daemon-session-list.test.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,30 @@ describe("buildSessionList", () => {
167167
});
168168
});
169169

170+
it("keeps file-keyed schedule pins on active rows while active ids are being rebound", () => {
171+
const sessionFile = "/tmp/child.jsonl";
172+
const [entry] = buildSessionList(
173+
[makeState({ activeSessionId: "new-active-id", sessionFile })],
174+
[makeSessionInfo({ id: "child-session", path: sessionFile })],
175+
[
176+
makeCronJob({
177+
id: "stale-heartbeat",
178+
activeSessionId: "old-active-id",
179+
sessionFile,
180+
source: "heartbeat",
181+
}),
182+
makeCronJob({
183+
id: "stale-cron",
184+
activeSessionId: "old-active-id",
185+
sessionFile,
186+
source: "cron",
187+
}),
188+
],
189+
);
190+
191+
expect(entry).toMatchObject({ hasRegisteredHeartbeat: true, hasRegisteredCronJob: true });
192+
});
193+
170194
it("reports accepted in-flight prompts as active with no queued work", () => {
171195
const oneMessage = [{ role: "user", content: "hi" }] as unknown as AgentMessage[];
172196
const summary = summaryForActiveSession(

0 commit comments

Comments
 (0)