Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/ci-cd.yml
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ jobs:
apps/api/src/services/cli-conversation.test.ts
apps/api/src/services/dataset-search-fitness.test.ts
apps/api/src/services/cli-runtime-recovery.test.ts
apps/api/src/services/cli-runtime-liveness.test.ts
apps/api/src/services/cli-run-selection.test.ts
apps/api/src/services/pdf-service.test.ts
apps/api/src/services/pdf-attachments.test.ts
apps/api/src/services/vendor-evidence-lines.test.ts
Expand All @@ -119,6 +121,8 @@ jobs:
packages/mcp-server/src/api-client.test.ts
packages/mcp-server/src/modifier-percent.test.ts
packages/mcp-server/src/summary-row-contract.test.ts
packages/mcp-server/src/tools/source-refs.test.ts
packages/mcp-server/src/tools/evidence-gate-contract.test.ts
apps/api/src/revision-rate-schedule-scoping.test.ts
apps/api/src/answer-resumes-run.test.ts
apps/api/src/quote-status-source.test.ts
Expand Down
15 changes: 14 additions & 1 deletion apps/api/src/answer-resumes-run.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,20 @@ test("answering a question resumes a run that already stopped", () => {
// Without this the answer sat in run history with nobody polling for it.
const body = answerHandler();
assert.match(body, /startResumedSession\(request, \{/, "must start a resumed session");
assert.match(body, /if \(session\) \{/, "must not resume when a live session is already polling");
});

test("answering a question does not resume on top of a live agent", () => {
// Deciding this from the in-memory handle was the bug: an agent we had lost
// track of looked stopped, so we resumed over it, the runtime refused the
// second writer on the thread, and the user saw a failure -- while the
// original run had the answer and went on to finish. Liveness must be probed.
// Behaviour is covered in services/cli-runtime-liveness.test.ts.
const body = answerHandler();
const resumeIndex = body.indexOf("startResumedSession(request, {");
const guardIndex = body.indexOf("liveAgent.live");
assert.notEqual(guardIndex, -1, "must probe for a live agent");
assert.ok(guardIndex < resumeIndex, "the probe must gate the resume");
assert.match(body, /probeLiveAgent\(projectId, resolveProjectDir\(projectId\)\)/);
});

test("the resumed agent is told the question and the answer", () => {
Expand Down
31 changes: 16 additions & 15 deletions apps/api/src/resume-session-guard.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,18 @@ import assert from "node:assert/strict";
import test from "node:test";
import { readFileSync } from "node:fs";

/**
* Route-level wiring for the "one live agent per project" rule.
*
* The rule itself is exercised for real in
* `services/cli-runtime-liveness.test.ts`. It has to be: this file used to
* assert that `resumeSession` contained `session.status === "running"`, which
* stayed true for three weeks while the guard did nothing in production -- the
* session registry it consulted had been emptied by an unrelated timer, so the
* check never fired and resumes kept colliding with running agents. Matching
* source text cannot tell a working guard from an inert one.
*/

const runtime = readFileSync(new URL("./services/cli-runtime.ts", import.meta.url), "utf8");
const routes = readFileSync(new URL("./routes/cli-routes.ts", import.meta.url), "utf8");

Expand All @@ -13,27 +25,16 @@ function functionBody(source: string, signature: string) {
return next === -1 ? rest : rest.slice(0, next);
}

test("resume refuses to start on top of a running session", () => {
// The real incident: a resume was issued 4 minutes into a session that kept
// running for another 90 seconds. Codex refused the second writer --
// "thread-store conflict: thread <id> already has an active writer" -- and the
// new run died. spawnSession already guarded this; resume did not.
test("the resume guard probes the process rather than trusting the registry", () => {
const body = functionBody(runtime, "export async function resumeSession(");
assert.match(body, /session\.status === "running"/, "must check the live session's status");
const guardIndex = body.indexOf('session.status === "running"');
const sessionIdIndex = body.indexOf("let sessionId");
const guardIndex = body.indexOf("probeLiveAgent(");
assert.notEqual(guardIndex, -1, "must probe for a live agent");
assert.ok(
guardIndex < sessionIdIndex,
guardIndex < body.indexOf("let sessionId"),
"the guard must run before resolving a session id to resume",
);
});

test("the guard reports a conflict, not a generic failure", () => {
const body = functionBody(runtime, "export async function resumeSession(");
assert.match(body, /statusCode: 409/, "409 so callers can distinguish it from a crash");
assert.match(body, /Stop it before resuming/, "tells the user what to do");
});

test("spawn and resume agree that one live session per project is the rule", () => {
const spawn = functionBody(runtime, "export async function spawnSession(");
assert.match(spawn, /existing\.status === "running"/, "spawn already had this guard");
Expand Down
15 changes: 12 additions & 3 deletions apps/api/src/routes/cli-routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import type { FastifyInstance, FastifyRequest } from "fastify";
import { detectCli, checkCliAuth, spawnSession, stopSession, resumeSession, getSession, listSessions, listCliModels, type AgentChatMode, type AgentRuntime } from "../services/cli-runtime.js";
import { detectCli, checkCliAuth, spawnSession, stopSession, resumeSession, getSession, probeLiveAgent, listSessions, listCliModels, type AgentChatMode, type AgentRuntime } from "../services/cli-runtime.js";
import {
startLoginSession,
attachLoginSession,
Expand All @@ -25,6 +25,7 @@ import { writeAgentLibrarySnapshot } from "../services/agent-library-snapshot.js
import { stripBlankCredentialEnv } from "../services/agent-host/env-sanitize.js";
import { getAgentRuntimeHost } from "../services/agent-host/index.js";
import { buildModeConversationContext } from "../services/cli-conversation.js";
import { selectLatestRun } from "../services/cli-run-selection.js";
import {
resolveAgentProviderKeys,
resolveRuntimeProviderKey,
Expand Down Expand Up @@ -2057,7 +2058,7 @@ ${message}`;
};
}

const latestRun = runs[runs.length - 1];
const latestRun = selectLatestRun(runs);
const latestRunEvents = ((latestRun?.output as any)?.events || []) as Array<{
type?: string;
timestamp?: string;
Expand Down Expand Up @@ -2551,7 +2552,15 @@ Merge tables that span multiple pages. Skip non-data pages.
// already stopped there is nobody polling, so the answer would sit in
// history forever. Questions have no deadline by design — the answer can
// arrive the next day — so restart the run and hand it the answer.
if (session) {
//
// "Still running" has to be decided by probing the process, not by whether
// this API process still holds the handle. Trusting the handle alone meant
// a running agent that we had lost track of looked stopped: we resumed on
// top of it, the runtime refused the second writer on the same thread, and
// the user saw a failure even though the answer had been delivered and the
// original run went on to finish normally.
const liveAgent = await probeLiveAgent(projectId, resolveProjectDir(projectId));
if (liveAgent.live) {
return { ok: true, message: "Answer delivered to agent", resumed: false };
}

Expand Down
86 changes: 86 additions & 0 deletions apps/api/src/services/cli-run-selection.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import assert from "node:assert/strict";
import test from "node:test";

import { isOpenRun, isStillbornRun, selectLatestRun } from "./cli-run-selection.js";

const statusEvent = (status: string) => ({ type: "status", data: { status } });

/** A run that started and is still going. */
const openRun = (id: string) => ({
id,
status: "running",
output: { events: [statusEvent("running"), { type: "tool_call", data: {} }] },
});

/** A run that started and finished. */
const finishedRun = (id: string, status = "completed") => ({
id,
status,
output: { events: [statusEvent("running"), statusEvent(status)] },
});

/**
* A resume that collided with a live agent: it failed without the CLI ever
* reporting "running", because the process never got that far.
*/
const stillbornRun = (id: string) => ({
id,
status: "failed",
output: {
events: [
{ type: "error", data: { message: "thread-store conflict: thread 01a0 already has an active writer" } },
{ type: "message", data: { role: "assistant", content: "Intake failed (exit code 1)." } },
statusEvent("failed"),
],
},
});

test("classifies a collided resume as stillborn and a working run as open", () => {
assert.equal(isStillbornRun(stillbornRun("r2")), true);
assert.equal(isStillbornRun(finishedRun("r1", "failed")), false, "a run that started then failed is a real failure");
assert.equal(isOpenRun(openRun("r1")), true);
assert.equal(isOpenRun(finishedRun("r1")), false);
});

test("a failed resume does not mask the run that is still working", () => {
// The incident: answering a question resumed on top of a live agent, the
// runtime refused the second writer, and that sub-second failure became the
// newest run -- so the whole project reported "failed" while the estimate was
// still being built.
const live = openRun("run-live");
const latest = selectLatestRun([finishedRun("run-0"), live, stillbornRun("run-collision")]);
assert.equal(latest, live, "status must follow the run that is actually running");
});

test("repeated failed resumes still do not mask it", () => {
// Every Resume click appended another stillborn run, so a single-step lookback
// would have gone straight back to reporting a failure.
const live = openRun("run-live");
const latest = selectLatestRun([
live,
stillbornRun("c1"),
stillbornRun("c2"),
stillbornRun("c3"),
stillbornRun("c4"),
]);
assert.equal(latest, live);
});

test("a genuine start-up failure is still reported", () => {
// Nothing else is running, so this failure is the truth about the project and
// must not be hidden behind the previous run's success.
const collision = stillbornRun("run-failed-start");
const latest = selectLatestRun([finishedRun("run-0"), collision]);
assert.equal(latest, collision);
});

test("a failure after the live run ends is reported once it is the truth", () => {
const latest = selectLatestRun([finishedRun("run-0"), stillbornRun("c1")]);
assert.equal((latest as { id: string }).id, "c1");
});

test("the newest run wins in the ordinary case", () => {
const newest = openRun("run-2");
assert.equal(selectLatestRun([finishedRun("run-1"), newest]), newest);
assert.equal(selectLatestRun([]), undefined);
});
63 changes: 63 additions & 0 deletions apps/api/src/services/cli-run-selection.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/**
* Choosing which AiRun represents a project's current agent state.
*
* "Newest run wins" is right almost always, and wrong in one case that matters:
* a resume issued against an agent that is still working dies in a few hundred
* milliseconds (the runtime refuses a second writer on the same thread). That
* stillborn run is the newest, so it became the run the status endpoint
* reported — turning a healthy, still-running estimate into a visible failure.
* Each retry appended another one, so every attempt to recover re-applied the
* mask. These helpers skip that tail while a real run is still open.
*/

export interface SelectableRun {
status?: string;
output?: unknown;
}

const TERMINAL_RUN_STATUSES = new Set(["completed", "failed", "stopped"]);

function statusEventValues(run: SelectableRun | undefined): string[] {
const events = ((run?.output as { events?: unknown })?.events || []) as Array<{
type?: string;
data?: { status?: unknown };
}>;
if (!Array.isArray(events)) return [];
return events
.filter((event) => event?.type === "status")
.map((event) => String(event?.data?.status ?? ""));
}

/**
* A run whose process never came up: marked failed, and it never once reported
* "running". A CLI that starts at all emits a running status before anything
* else, so its absence is a reliable "this never began".
*/
export function isStillbornRun(run: SelectableRun | undefined): boolean {
if (run?.status !== "failed") return false;
return !statusEventValues(run).includes("running");
}

/** A run the DB still has open, with no terminal status event in its transcript. */
export function isOpenRun(run: SelectableRun | undefined): boolean {
if (run?.status !== "running") return false;
return !statusEventValues(run).some((status) => TERMINAL_RUN_STATUSES.has(status));
}

/**
* Pick the run that represents the project's current state.
*
* Falls back to the newest run whenever the last run that actually started has
* finished, so a genuine start-up failure with nothing else running still
* surfaces to the user rather than silently reporting the previous run.
*/
export function selectLatestRun<T extends SelectableRun>(runs: T[]): T | undefined {
if (runs.length === 0) return undefined;
const newest = runs[runs.length - 1];
if (!isStillbornRun(newest)) return newest;

let index = runs.length - 1;
while (index >= 0 && isStillbornRun(runs[index])) index -= 1;
const lastStarted = index >= 0 ? runs[index] : undefined;
return lastStarted && isOpenRun(lastStarted) ? lastStarted : newest;
}
Loading
Loading