Fail Codex resume requests explicitly (#947)
This commit is contained in:
@@ -35,6 +35,7 @@ interface CollaborationModeRecord {
|
|||||||
}
|
}
|
||||||
|
|
||||||
interface CodexSessionTestAccess {
|
interface CodexSessionTestAccess {
|
||||||
|
ensureThreadLoaded(): Promise<void>;
|
||||||
handleToolApprovalRequest(params: unknown): Promise<unknown>;
|
handleToolApprovalRequest(params: unknown): Promise<unknown>;
|
||||||
handleNotification(method: string, params: unknown): void;
|
handleNotification(method: string, params: unknown): void;
|
||||||
loadPersistedHistory(): Promise<void>;
|
loadPersistedHistory(): Promise<void>;
|
||||||
@@ -378,6 +379,80 @@ describe("Codex app-server provider", () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test("resumeSession does not replace a persisted Codex thread when app-server resume fails", async () => {
|
||||||
|
const threadRequests: string[] = [];
|
||||||
|
const appServer = createFakeCodexAppServer({
|
||||||
|
"thread/loaded/list": () => {
|
||||||
|
threadRequests.push("thread/loaded/list");
|
||||||
|
return { data: [] };
|
||||||
|
},
|
||||||
|
"thread/resume": () => {
|
||||||
|
threadRequests.push("thread/resume");
|
||||||
|
return Promise.reject(new Error("no rollout found for thread id archived-thread-id"));
|
||||||
|
},
|
||||||
|
"thread/start": () => {
|
||||||
|
threadRequests.push("thread/start");
|
||||||
|
return { thread: { id: "replacement-empty-thread-id" } };
|
||||||
|
},
|
||||||
|
"thread/read": () => {
|
||||||
|
threadRequests.push("thread/read");
|
||||||
|
return { thread: { turns: [] } };
|
||||||
|
},
|
||||||
|
getUserSavedConfig: () => {
|
||||||
|
threadRequests.push("getUserSavedConfig");
|
||||||
|
return { config: {} };
|
||||||
|
},
|
||||||
|
"config/read": () => {
|
||||||
|
threadRequests.push("config/read");
|
||||||
|
return { config: {} };
|
||||||
|
},
|
||||||
|
"model/list": () => {
|
||||||
|
threadRequests.push("model/list");
|
||||||
|
return {
|
||||||
|
data: [{ id: "gpt-5.4", isDefault: true, defaultReasoningEffort: "medium" }],
|
||||||
|
};
|
||||||
|
},
|
||||||
|
});
|
||||||
|
const provider = new CodexAppServerAgentClient(createTestLogger());
|
||||||
|
castInternals<{ goalsEnabledPromise: Promise<boolean> | null }>(provider).goalsEnabledPromise =
|
||||||
|
Promise.resolve(false);
|
||||||
|
castInternals<{ spawnAppServer: () => Promise<ChildProcessWithoutNullStreams> }>(
|
||||||
|
provider,
|
||||||
|
).spawnAppServer = async () => appServer.child;
|
||||||
|
|
||||||
|
const outcome = await Promise.race([
|
||||||
|
provider
|
||||||
|
.resumeSession({
|
||||||
|
sessionId: "archived-thread-id",
|
||||||
|
metadata: {
|
||||||
|
cwd: "/tmp/codex-question-test",
|
||||||
|
modeId: "auto",
|
||||||
|
model: "gpt-5.4",
|
||||||
|
},
|
||||||
|
})
|
||||||
|
.then(
|
||||||
|
() => "resolved" as const,
|
||||||
|
(error) => {
|
||||||
|
expect(error).toBeInstanceOf(Error);
|
||||||
|
expect((error as Error).message).toContain(
|
||||||
|
"no rollout found for thread id archived-thread-id",
|
||||||
|
);
|
||||||
|
return "rejected" as const;
|
||||||
|
},
|
||||||
|
),
|
||||||
|
new Promise<"timed_out">((resolve) => setTimeout(() => resolve("timed_out"), 500)),
|
||||||
|
]);
|
||||||
|
|
||||||
|
if (outcome === "timed_out") {
|
||||||
|
appServer.child.kill("SIGTERM");
|
||||||
|
throw new Error(`resumeSession timed out; thread requests: ${threadRequests.join(", ")}`);
|
||||||
|
}
|
||||||
|
|
||||||
|
expect(threadRequests).toEqual(["thread/loaded/list", "thread/resume"]);
|
||||||
|
expect(outcome).toBe("rejected");
|
||||||
|
appServer.assertNoErrors();
|
||||||
|
});
|
||||||
|
|
||||||
test("lists repo skills using WorkspaceGitService repo-root resolution", async () => {
|
test("lists repo skills using WorkspaceGitService repo-root resolution", async () => {
|
||||||
const tempDir = await mkdtemp(path.join(tmpdir(), "codex-skills-"));
|
const tempDir = await mkdtemp(path.join(tmpdir(), "codex-skills-"));
|
||||||
const cwd = path.join(tempDir, "repo", "packages", "app");
|
const cwd = path.join(tempDir, "repo", "packages", "app");
|
||||||
@@ -1134,6 +1209,37 @@ describe("Codex app-server provider", () => {
|
|||||||
]);
|
]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test("does not replace a persisted Codex thread when app-server resume fails", async () => {
|
||||||
|
const session = createSession({ thinkingOptionId: "medium" });
|
||||||
|
session.currentThreadId = "archived-thread-id";
|
||||||
|
const requests: Array<{ method: string; params: unknown }> = [];
|
||||||
|
session.client = {
|
||||||
|
request: vi.fn(async (method: string, params: unknown) => {
|
||||||
|
requests.push({ method, params });
|
||||||
|
if (method === "thread/loaded/list") {
|
||||||
|
return { data: [] };
|
||||||
|
}
|
||||||
|
if (method === "thread/resume") {
|
||||||
|
throw new Error("no rollout found for thread id archived-thread-id");
|
||||||
|
}
|
||||||
|
if (method === "thread/start") {
|
||||||
|
return { thread: { id: "replacement-empty-thread-id" } };
|
||||||
|
}
|
||||||
|
return {};
|
||||||
|
}),
|
||||||
|
};
|
||||||
|
|
||||||
|
await expect(asInternals(session).ensureThreadLoaded()).rejects.toThrow(
|
||||||
|
"no rollout found for thread id archived-thread-id",
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(session.currentThreadId).toBe("archived-thread-id");
|
||||||
|
expect(requests).toEqual([
|
||||||
|
{ method: "thread/loaded/list", params: {} },
|
||||||
|
{ method: "thread/resume", params: { threadId: "archived-thread-id" } },
|
||||||
|
]);
|
||||||
|
});
|
||||||
|
|
||||||
test("appends blank-line spacing to /goal status messages", async () => {
|
test("appends blank-line spacing to /goal status messages", async () => {
|
||||||
const requests: Array<{ method: string; params: unknown }> = [];
|
const requests: Array<{ method: string; params: unknown }> = [];
|
||||||
const session = createSession({}, { goalsEnabled: true });
|
const session = createSession({}, { goalsEnabled: true });
|
||||||
|
|||||||
@@ -2987,9 +2987,10 @@ class CodexAppServerAgentSession implements AgentSession {
|
|||||||
}
|
}
|
||||||
await this.client.request("thread/resume", params);
|
await this.client.request("thread/resume", params);
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
this.logger.warn({ error }, "Failed to resume Codex thread, starting new thread");
|
const threadId = this.currentThreadId;
|
||||||
this.currentThreadId = null;
|
const message = error instanceof Error ? error.message : String(error);
|
||||||
await this.ensureThread();
|
this.logger.warn({ error, threadId }, "Failed to resume persisted Codex thread");
|
||||||
|
throw new Error(`Failed to resume Codex thread ${threadId}: ${message}`, { cause: error });
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -151,7 +151,7 @@ function waitForSignal<T>(
|
|||||||
|
|
||||||
class NonPersistentReloadSession implements AgentSession {
|
class NonPersistentReloadSession implements AgentSession {
|
||||||
readonly provider = "claude" as const;
|
readonly provider = "claude" as const;
|
||||||
readonly id = null;
|
readonly id: string | null;
|
||||||
readonly capabilities = {
|
readonly capabilities = {
|
||||||
supportsStreaming: false,
|
supportsStreaming: false,
|
||||||
supportsSessionPersistence: true,
|
supportsSessionPersistence: true,
|
||||||
@@ -161,7 +161,12 @@ class NonPersistentReloadSession implements AgentSession {
|
|||||||
supportsToolInvocations: false,
|
supportsToolInvocations: false,
|
||||||
} as const;
|
} as const;
|
||||||
|
|
||||||
constructor(private readonly onClose: () => void) {}
|
constructor(
|
||||||
|
private readonly onClose: () => void,
|
||||||
|
id: string | null = null,
|
||||||
|
) {
|
||||||
|
this.id = id;
|
||||||
|
}
|
||||||
|
|
||||||
async run(): Promise<AgentRunResult> {
|
async run(): Promise<AgentRunResult> {
|
||||||
return {
|
return {
|
||||||
@@ -259,6 +264,37 @@ class NonPersistentReloadClient implements AgentClient {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
class FailingResumeSession extends NonPersistentReloadSession {
|
||||||
|
constructor(onClose: () => void) {
|
||||||
|
super(onClose, "failing-resume-session");
|
||||||
|
}
|
||||||
|
|
||||||
|
describePersistence(): AgentPersistenceHandle | null {
|
||||||
|
return {
|
||||||
|
provider: "claude",
|
||||||
|
sessionId: this.id,
|
||||||
|
metadata: { cwd: process.cwd() },
|
||||||
|
};
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
class FailingResumeClient extends NonPersistentReloadClient {
|
||||||
|
async createSession(_config: AgentSessionConfig): Promise<AgentSession> {
|
||||||
|
this.createSessionCalls += 1;
|
||||||
|
return new FailingResumeSession(() => {
|
||||||
|
this.closeCalls += 1;
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
async resumeSession(
|
||||||
|
_handle: AgentPersistenceHandle,
|
||||||
|
_overrides?: Partial<AgentSessionConfig>,
|
||||||
|
): Promise<AgentSession> {
|
||||||
|
this.resumeSessionCalls += 1;
|
||||||
|
throw new Error("resume exploded");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
function resolveSpeechConfig() {
|
function resolveSpeechConfig() {
|
||||||
if (hasLocalSpeech) {
|
if (hasLocalSpeech) {
|
||||||
return {
|
return {
|
||||||
@@ -482,6 +518,36 @@ test("refresh_agent rebuilds a live agent even when it has no persistence handle
|
|||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test("refresh_agent rejects when persisted session resume fails", async () => {
|
||||||
|
const cwd = tmpCwd();
|
||||||
|
const client = new FailingResumeClient();
|
||||||
|
const localCtx = await createDaemonTestContext({
|
||||||
|
agentClients: {
|
||||||
|
claude: client,
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
|
try {
|
||||||
|
const created = await localCtx.client.createAgent({
|
||||||
|
config: {
|
||||||
|
provider: "claude",
|
||||||
|
cwd,
|
||||||
|
},
|
||||||
|
});
|
||||||
|
await localCtx.client.archiveAgent(created.id);
|
||||||
|
|
||||||
|
await expect(localCtx.client.refreshAgent(created.id)).rejects.toMatchObject({
|
||||||
|
name: "DaemonRpcError",
|
||||||
|
code: "agent_refresh_failed",
|
||||||
|
requestType: "refresh_agent_request",
|
||||||
|
});
|
||||||
|
expect(client.resumeSessionCalls).toBe(1);
|
||||||
|
} finally {
|
||||||
|
await localCtx.cleanup();
|
||||||
|
rmSync(cwd, { recursive: true, force: true });
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
test("resume_agent auto-unarchives archived agents", async () => {
|
test("resume_agent auto-unarchives archived agents", async () => {
|
||||||
const cwd = tmpCwd();
|
const cwd = tmpCwd();
|
||||||
try {
|
try {
|
||||||
|
|||||||
@@ -3085,6 +3085,17 @@ export class Session {
|
|||||||
const { handle, overrides, requestId } = msg;
|
const { handle, overrides, requestId } = msg;
|
||||||
if (!handle) {
|
if (!handle) {
|
||||||
this.sessionLogger.warn("Resume request missing persistence handle");
|
this.sessionLogger.warn("Resume request missing persistence handle");
|
||||||
|
if (requestId) {
|
||||||
|
this.emit({
|
||||||
|
type: "rpc_error",
|
||||||
|
payload: {
|
||||||
|
requestId,
|
||||||
|
requestType: msg.type,
|
||||||
|
error: "Unable to resume agent: missing persistence handle",
|
||||||
|
code: "agent_resume_failed",
|
||||||
|
},
|
||||||
|
});
|
||||||
|
}
|
||||||
this.emit({
|
this.emit({
|
||||||
type: "activity_log",
|
type: "activity_log",
|
||||||
payload: {
|
payload: {
|
||||||
@@ -3121,14 +3132,26 @@ export class Session {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
|
const message = getErrorMessage(error);
|
||||||
this.sessionLogger.error({ err: error }, "Failed to resume agent");
|
this.sessionLogger.error({ err: error }, "Failed to resume agent");
|
||||||
|
if (requestId) {
|
||||||
|
this.emit({
|
||||||
|
type: "rpc_error",
|
||||||
|
payload: {
|
||||||
|
requestId,
|
||||||
|
requestType: msg.type,
|
||||||
|
error: message,
|
||||||
|
code: "agent_resume_failed",
|
||||||
|
},
|
||||||
|
});
|
||||||
|
}
|
||||||
this.emit({
|
this.emit({
|
||||||
type: "activity_log",
|
type: "activity_log",
|
||||||
payload: {
|
payload: {
|
||||||
id: uuidv4(),
|
id: uuidv4(),
|
||||||
timestamp: new Date(),
|
timestamp: new Date(),
|
||||||
type: "error",
|
type: "error",
|
||||||
content: `Failed to resume agent: ${getErrorMessage(error)}`,
|
content: `Failed to resume agent: ${message}`,
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
@@ -3249,14 +3272,26 @@ export class Session {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
|
const message = getErrorMessage(error);
|
||||||
this.sessionLogger.error({ err: error, agentId }, `Failed to refresh agent ${agentId}`);
|
this.sessionLogger.error({ err: error, agentId }, `Failed to refresh agent ${agentId}`);
|
||||||
|
if (requestId) {
|
||||||
|
this.emit({
|
||||||
|
type: "rpc_error",
|
||||||
|
payload: {
|
||||||
|
requestId,
|
||||||
|
requestType: msg.type,
|
||||||
|
error: message,
|
||||||
|
code: "agent_refresh_failed",
|
||||||
|
},
|
||||||
|
});
|
||||||
|
}
|
||||||
this.emit({
|
this.emit({
|
||||||
type: "activity_log",
|
type: "activity_log",
|
||||||
payload: {
|
payload: {
|
||||||
id: uuidv4(),
|
id: uuidv4(),
|
||||||
timestamp: new Date(),
|
timestamp: new Date(),
|
||||||
type: "error",
|
type: "error",
|
||||||
content: `Failed to refresh agent: ${getErrorMessage(error)}`,
|
content: `Failed to refresh agent: ${message}`,
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user