From 91a69b372b67fcd73d92d5e7d990a3334c422ae4 Mon Sep 17 00:00:00 2001 From: whoisasx Date: Mon, 18 May 2026 15:42:23 +0530 Subject: [PATCH] Fix CLI failure event review issues --- packages/cli/__tests__/commands/start.test.ts | 70 ++++++++++++++++++- packages/cli/__tests__/lib/shutdown.test.ts | 18 ++++- packages/cli/src/commands/start.ts | 49 +++++++++---- packages/cli/src/lib/shutdown.ts | 18 +++-- 4 files changed, 133 insertions(+), 22 deletions(-) diff --git a/packages/cli/__tests__/commands/start.test.ts b/packages/cli/__tests__/commands/start.test.ts index 60a247c8e..b821d3637 100644 --- a/packages/cli/__tests__/commands/start.test.ts +++ b/packages/cli/__tests__/commands/start.test.ts @@ -19,7 +19,7 @@ import { join } from "node:path"; import { tmpdir } from "node:os"; import { parse as parseYaml } from "yaml"; import { EventEmitter } from "node:events"; -import type { SessionManager } from "@aoagents/ao-core"; +import { recordActivityEvent, type SessionManager } from "@aoagents/ao-core"; // --------------------------------------------------------------------------- // Hoisted mocks @@ -153,6 +153,7 @@ vi.mock("@aoagents/ao-core", async (importOriginal) => { sweepDaemonChildren: mockSweepDaemonChildren, scanAoOrphans: mockScanAoOrphans, reapAoOrphans: mockReapAoOrphans, + recordActivityEvent: vi.fn(), }; }); @@ -336,6 +337,7 @@ beforeEach(async () => { program.exitOverride(); registerStart(program); registerStop(program); + vi.mocked(recordActivityEvent).mockClear(); vi.spyOn(console, "log").mockImplementation(() => {}); vi.spyOn(console, "error").mockImplementation(() => {}); @@ -505,6 +507,9 @@ function makeProject(overrides: Record = {}): Record> => + vi.mocked(recordActivityEvent).mock.calls.map((c) => c[0] as Record); + /** Mock process.cwd() to return a specific directory (avoids process.chdir in workers). */ function mockCwd(dir: string): void { cwdSpy = vi.spyOn(process, "cwd").mockReturnValue(dir); @@ -1571,6 +1576,19 @@ describe("start command — orchestrator session strategy display", () => { await expect(program.parseAsync(["node", "test", "start"])).rejects.toThrow("process.exit(1)"); expect(releaseStartupLock).toHaveBeenCalledTimes(1); + const startFailedEvents = recordedEvents().filter((e) => e.kind === "cli.start_failed"); + expect(startFailedEvents).toHaveLength(1); + expect(startFailedEvents[0]).toEqual( + expect.objectContaining({ + projectId: "my-app", + source: "cli", + level: "error", + data: expect.objectContaining({ + reason: "orchestrator_setup", + errorMessage: "Spawn failed", + }), + }), + ); }); it("fails and cleans up dashboard when sm.restore throws on a killed orchestrator", async () => { @@ -1651,6 +1669,56 @@ describe("start command — orchestrator session strategy display", () => { expect(written.stoppedAt).toBe("2026-04-28T10:00:00.000Z"); }); + it("attributes other-project restore failures to the owning project", async () => { + mockReadLastStop.mockResolvedValue({ + stoppedAt: "2026-04-28T10:00:00.000Z", + projectId: "my-app", + sessionIds: ["app-1"], + otherProjects: [{ projectId: "other-app", sessionIds: ["other-1"] }], + }); + + mockConfigRef.current = makeConfig({ + "my-app": makeProject(), + "other-app": makeProject({ name: "Other App", sessionPrefix: "other" }), + }); + const { findWebDir } = await import("../../src/lib/web-dir.js"); + vi.mocked(findWebDir).mockReturnValue(tmpDir); + writeFileSync(join(tmpDir, "package.json"), "{}"); + + const fakeDashboard = { on: vi.fn(), kill: vi.fn(), emit: vi.fn() }; + mockSpawn.mockReturnValue(fakeDashboard); + + mockSessionManager.restore.mockImplementation((id: string) => { + if (id === "other-1") return Promise.reject(new Error("workspace gone")); + return Promise.resolve(undefined); + }); + + await program.parseAsync(["node", "test", "start", "my-app", "--no-orchestrator"]); + + const restoreFailedEvents = recordedEvents().filter( + (e) => e.kind === "cli.restore_session_failed", + ); + expect(restoreFailedEvents).toHaveLength(1); + expect(restoreFailedEvents[0]).toEqual( + expect.objectContaining({ + projectId: "other-app", + sessionId: "other-1", + source: "cli", + level: "warn", + data: expect.objectContaining({ errorMessage: "workspace gone" }), + }), + ); + + const written = mockWriteLastStop.mock.calls[0][0]; + expect(written).toEqual( + expect.objectContaining({ + projectId: "my-app", + sessionIds: [], + otherProjects: [{ projectId: "other-app", sessionIds: ["other-1"] }], + }), + ); + }); + it("clears last-stop record when every session restored successfully", async () => { mockReadLastStop.mockResolvedValue({ stoppedAt: "2026-04-28T10:00:00.000Z", diff --git a/packages/cli/__tests__/lib/shutdown.test.ts b/packages/cli/__tests__/lib/shutdown.test.ts index de0b81cee..6ff859cc5 100644 --- a/packages/cli/__tests__/lib/shutdown.test.ts +++ b/packages/cli/__tests__/lib/shutdown.test.ts @@ -209,13 +209,25 @@ describe("shutdown handlers — activity events", () => { const events = recordedEvents(); expect(events).toContainEqual( expect.objectContaining({ - kind: "cli.shutdown_failed", + kind: "cli.last_stop_write_failed", source: "cli", level: "error", - data: expect.objectContaining({ errorMessage: "disk full" }), + data: expect.objectContaining({ + targetSessionCount: 1, + otherProjectCount: 0, + totalKilled: 1, + errorMessage: "disk full", + }), }), ); - expect(events.filter((e) => e.kind === "cli.shutdown_completed")).toHaveLength(0); + expect(events).toContainEqual( + expect.objectContaining({ + kind: "cli.shutdown_completed", + source: "cli", + projectId: "p1", + }), + ); + expect(events.filter((e) => e.kind === "cli.shutdown_failed")).toHaveLength(0); }); it("emits cli.shutdown_force_exit when the 10s timer fires", async () => { diff --git a/packages/cli/src/commands/start.ts b/packages/cli/src/commands/start.ts index e9a3cb083..d19fc066f 100644 --- a/packages/cli/src/commands/start.ts +++ b/packages/cli/src/commands/start.ts @@ -117,6 +117,17 @@ import { projectSessionUrl } from "../lib/routes.js"; // HELPERS // ============================================================================= +class CliFailureEventRecordedError extends Error { + constructor(message: string, options?: ErrorOptions) { + super(message, options); + this.name = "CliFailureEventRecordedError"; + } +} + +function isCliFailureEventRecordedError(err: unknown): boolean { + return err instanceof CliFailureEventRecordedError; +} + function readProjectBehaviorConfig(projectPath: string): LocalProjectConfig { const localConfig = loadLocalProjectConfigDetailed(projectPath); if (localConfig.kind === "loaded") { @@ -964,7 +975,7 @@ async function runStartup( if (dashboardProcess) { dashboardProcess.kill(); } - throw new Error( + throw new CliFailureEventRecordedError( `Failed to setup orchestrator: ${err instanceof Error ? err.message : String(err)}`, { cause: err }, ); @@ -992,7 +1003,7 @@ async function runStartup( if (dashboardProcess) { dashboardProcess.kill(); } - throw new Error( + throw new CliFailureEventRecordedError( `Failed to start project supervisor: ${err instanceof Error ? err.message : String(err)}`, { cause: err }, ); @@ -1006,12 +1017,21 @@ async function runStartup( if (lastStop && lastStop.sessionIds.length > 0) { const stoppedAgo = `stopped at ${new Date(lastStop.stoppedAt).toLocaleString()}`; const otherProjects = lastStop.otherProjects ?? []; + const restoreProjectBySessionId = new Map(); // Build flat list of all sessions to restore, grouped for display const allRestoreSessions: string[] = [ ...(lastStop.projectId === projectId ? lastStop.sessionIds : []), ...otherProjects.flatMap((p) => p.sessionIds), ]; + for (const sessionId of lastStop.sessionIds) { + restoreProjectBySessionId.set(sessionId, lastStop.projectId); + } + for (const otherProject of otherProjects) { + for (const sessionId of otherProject.sessionIds) { + restoreProjectBySessionId.set(sessionId, otherProject.projectId); + } + } // Display grouped by project const currentProjectSessions = lastStop.projectId === projectId ? lastStop.sessionIds : []; @@ -1072,8 +1092,9 @@ async function runStartup( restoredCount++; } catch (err) { failedSessionIds.add(sessionId); + const restoreProjectId = restoreProjectBySessionId.get(sessionId) ?? projectId; recordActivityEvent({ - projectId, + projectId: restoreProjectId, sessionId, source: "cli", kind: "cli.restore_session_failed", @@ -1743,16 +1764,18 @@ export function registerStart(program: Command): void { // Ctrl+C and `ao stop` (which sends SIGTERM) perform a full // graceful shutdown via the handler installed inside runStartup(). } catch (err) { - recordActivityEvent({ - source: "cli", - kind: "cli.start_failed", - level: "error", - summary: `ao start action failed`, - data: { - reason: "outer", - errorMessage: err instanceof Error ? err.message : String(err), - }, - }); + if (!isCliFailureEventRecordedError(err)) { + recordActivityEvent({ + source: "cli", + kind: "cli.start_failed", + level: "error", + summary: `ao start action failed`, + data: { + reason: "outer", + errorMessage: err instanceof Error ? err.message : String(err), + }, + }); + } if (err instanceof Error) { console.error(chalk.red("\nError:"), err.message); } else { diff --git a/packages/cli/src/lib/shutdown.ts b/packages/cli/src/lib/shutdown.ts index 6c215403b..8e024e4f8 100644 --- a/packages/cli/src/lib/shutdown.ts +++ b/packages/cli/src/lib/shutdown.ts @@ -119,7 +119,6 @@ export function installShutdownHandlers(ctx: ShutdownContext): void { } } - let lastStopWriteError: unknown; if (killedSessionIds.length > 0) { const targetIds = killedSessionIds.filter((id) => activeSessions.some((s) => s.id === id && s.projectId === ctx.projectId), @@ -144,15 +143,24 @@ export function installShutdownHandlers(ctx: ShutdownContext): void { otherProjects: otherProjects.length > 0 ? otherProjects : undefined, }); } catch (err) { - lastStopWriteError = err; + recordActivityEvent({ + projectId: ctx.projectId, + source: "cli", + kind: "cli.last_stop_write_failed", + level: "error", + summary: `failed to write last-stop state during shutdown`, + data: { + targetSessionCount: targetIds.length, + otherProjectCount: otherProjects.length, + totalKilled: killedSessionIds.length, + errorMessage: err instanceof Error ? err.message : String(err), + }, + }); } } await sweepDaemonChildren({ ownerPid: process.pid }); await unregister(); - if (lastStopWriteError) { - throw lastStopWriteError; - } recordActivityEvent({ projectId: ctx.projectId, source: "cli",