fix: harden supervisor objective resets

pallyoung · pallyoung · commit 63ad12a008a1 · 2026-05-15T23:41:40.000Z
diff --git a/packages/server/src/__tests__/supervisor-manager.test.ts b/packages/server/src/__tests__/supervisor-manager.test.ts
@@ -578,6 +578,57 @@ describe("SupervisorManager cycle triggers", () => {
     expect(deps.sessionMgr.sendInput).not.toHaveBeenCalled();
   });
 
+  it("marks the aborted attempt as cancelled when the objective changes", async () => {
+    const supervisor = await manager.create({
+      sessionId: "sess-objective-attempt-cancelled",
+      workspaceId: "ws-1",
+      objective: "Initial objective",
+      evaluatorProviderId: "codex",
+    });
+
+    let observedSignal: AbortSignal | undefined;
+    vi.spyOn(getManagerInternals().evaluator, "evaluate").mockImplementationOnce(
+      async (_supervisor, _context, options) =>
+        await new Promise<SupervisorEvaluationResult>((_resolve, reject) => {
+          observedSignal = options?.signal;
+          options?.signal?.addEventListener(
+            "abort",
+            () => {
+              reject({
+                code: "supervisor_eval_aborted",
+                message: "Supervisor evaluator aborted",
+              });
+            },
+            { once: true }
+          );
+        })
+    );
+
+    const cycle = await manager.triggerEvaluation(supervisor.id);
+
+    await waitFor(() => {
+      expect(observedSignal).toBeDefined();
+      expect(manager.get(supervisor.id)?.state).toBe("evaluating");
+    });
+
+    const updatedPromise = manager.update(supervisor.id, {
+      objective: "New objective",
+    });
+
+    await waitFor(() => {
+      expect(observedSignal?.aborted).toBe(true);
+    });
+
+    await updatedPromise;
+
+    expect(deps.cycleAttemptRepo.listForCycle(cycle.id)).toMatchObject([
+      {
+        status: "cancelled",
+        errorReason: undefined,
+      },
+    ]);
+  });
+
   it("marks supervisor_uncertain stops as cancelled instead of completed target meta", async () => {
     const supervisor = await manager.create({
       sessionId: "sess-uncertain-stop",
@@ -711,6 +762,42 @@ describe("SupervisorManager cycle triggers", () => {
     );
   });
 
+  it("rolls back persisted supervisor changes when resetting target files fails", async () => {
+    const supervisor = await manager.create({
+      sessionId: "sess-objective-rollback",
+      workspaceId: "ws-1",
+      objective: "Initial objective",
+      evaluatorProviderId: "codex",
+    });
+
+    deps.targetStore.resetTargetFiles.mockImplementation(async () => {
+      throw new Error("disk full");
+    });
+
+    await expect(
+      manager.update(supervisor.id, {
+        objective: "New objective",
+      })
+    ).rejects.toThrow("disk full");
+
+    const updatesForSupervisor = deps.supervisorRepo.update.mock.calls.filter(
+      ([id]: [string, SupervisorUpdatePatch]) => id === supervisor.id
+    );
+
+    expect(updatesForSupervisor).toHaveLength(2);
+    expect(updatesForSupervisor[0]?.[1]).toEqual(
+      expect.objectContaining({
+        objective: "New objective",
+      })
+    );
+    expect(updatesForSupervisor[1]?.[1]).toEqual(
+      expect.objectContaining({
+        objective: "Initial objective",
+      })
+    );
+    expect(deps.supervisorRepo.findById(supervisor.id)?.objective).toBe("Initial objective");
+  });
+
   it("retries evaluator timeout up to the global retry budget", async () => {
     vi.useFakeTimers();
     deps.settingsRepo.get = vi.fn((key: string) => {
diff --git a/packages/server/src/supervisor/manager.ts b/packages/server/src/supervisor/manager.ts
@@ -448,17 +448,31 @@ export class SupervisorManager {
       updatedAt: Date.now(),
     };
 
+    const rollbackPatch = this.toSupervisorUpdatePatch(current, Date.now());
+    let updated = this.attachCycles(this.deps.supervisorRepo.update(id, nextPatch));
+
     if (objectiveChanged) {
-      await this.deps.targetStore.resetTargetFiles(workspace.path, {
-        targetId: current.targetId,
-        sessionId: current.sessionId,
-        workspaceId: current.workspaceId,
-        objective: nextObjective,
-        createdAt: nextPatch.updatedAt ?? Date.now(),
-      });
+      try {
+        await this.deps.targetStore.resetTargetFiles(workspace.path, {
+          targetId: current.targetId,
+          sessionId: current.sessionId,
+          workspaceId: current.workspaceId,
+          objective: nextObjective,
+          createdAt: nextPatch.updatedAt ?? Date.now(),
+        });
+      } catch (error) {
+        try {
+          this.deps.supervisorRepo.update(id, rollbackPatch);
+        } catch (rollbackError) {
+          this.logger.error(
+            { err: rollbackError, supervisorId: id },
+            "Failed to roll back supervisor after target reset failure"
+          );
+        }
+        throw error;
+      }
     }
 
-    const updated = this.attachCycles(this.deps.supervisorRepo.update(id, nextPatch));
     const enriched = await this.attachTargetState(updated, workspace.path);
 
     this.storeSnapshot(enriched);
@@ -782,8 +796,7 @@ export class SupervisorManager {
       return finalized.cycle;
     } catch (error: unknown) {
       if (isSupervisorEvalAborted(error)) {
-        const cancelled =
-          this.pendingPauses.has(supervisorId) || this.pendingObjectiveUpdates.has(supervisorId);
+        const cancelled = this.isCancellationRequested(supervisorId);
         const abortedCycle = this.deps.cycleRepo.update(activeCycle.id, {
           status: cancelled ? "cancelled" : "failed",
           errorReason: cancelled ? null : messageOf(error, "Supervisor evaluator aborted"),
@@ -1043,12 +1056,11 @@ export class SupervisorManager {
         };
       } catch (error) {
         if (isSupervisorEvalAborted(error)) {
+          const cancelled = this.isCancellationRequested(supervisor.id);
           this.deps.cycleAttemptRepo.update(attempt.id, {
-            status: this.pendingPauses.has(supervisor.id) ? "cancelled" : "failed",
+            status: cancelled ? "cancelled" : "failed",
             completedAt: Date.now(),
-            errorReason: this.pendingPauses.has(supervisor.id)
-              ? null
-              : messageOf(error, "Supervisor evaluator aborted"),
+            errorReason: cancelled ? null : messageOf(error, "Supervisor evaluator aborted"),
           });
           throw error;
         }
@@ -1318,6 +1330,30 @@ export class SupervisorManager {
     };
   }
 
+  private isCancellationRequested(supervisorId: string): boolean {
+    return this.pendingPauses.has(supervisorId) || this.pendingObjectiveUpdates.has(supervisorId);
+  }
+
+  private toSupervisorUpdatePatch(
+    supervisor: Supervisor,
+    updatedAt: number
+  ): Parameters<SupervisorRepo["update"]>[1] {
+    return {
+      state: supervisor.state,
+      objective: supervisor.objective,
+      evaluatorProviderId: supervisor.evaluatorProviderId,
+      evaluatorModel: supervisor.evaluatorModel ?? null,
+      maxSupervisionCount: supervisor.maxSupervisionCount,
+      completedSupervisionCount: supervisor.completedSupervisionCount,
+      scheduledAt: supervisor.scheduledAt ?? null,
+      stopReason: supervisor.stopReason ?? null,
+      lastCycleAt: supervisor.lastCycleAt ?? null,
+      lastEvaluatedTurnId: supervisor.lastEvaluatedTurnId ?? null,
+      errorReason: supervisor.errorReason ?? null,
+      updatedAt,
+    };
+  }
+
   private applyEvaluationToTargetMemory(
     memory: SupervisorTargetMemory,
     evaluation: Awaited<ReturnType<SupervisorEvaluator["evaluate"]>>,
diff --git a/packages/server/src/supervisor/target-store.atomic.test.ts b/packages/server/src/supervisor/target-store.atomic.test.ts
@@ -0,0 +1,113 @@
+import { mkdtempSync, rmSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+
+const renameState = vi.hoisted(() => ({
+  callCount: 0,
+  failOnCall: 0,
+}));
+
+vi.mock("node:fs/promises", async () => {
+  const actual = await vi.importActual<typeof import("node:fs/promises")>("node:fs/promises");
+  return {
+    ...actual,
+    rename: vi.fn(async (from: string, to: string) => {
+      renameState.callCount += 1;
+      if (renameState.failOnCall !== 0 && renameState.callCount === renameState.failOnCall) {
+        throw new Error("promote failed");
+      }
+      return actual.rename(from, to);
+    }),
+  };
+});
+
+import {
+  appendTargetCycleRecord,
+  createTargetFiles,
+  loadTargetMemory,
+  readTargetCycleRecords,
+  readTargetMeta,
+  resetTargetFiles,
+  saveTargetMemory,
+} from "./target-store.js";
+
+describe("target store atomic reset", () => {
+  let workspacePath: string;
+
+  beforeEach(() => {
+    workspacePath = mkdtempSync(join(tmpdir(), "supervisor-target-store-atomic-"));
+    renameState.callCount = 0;
+    renameState.failOnCall = 0;
+  });
+
+  afterEach(() => {
+    rmSync(workspacePath, { recursive: true, force: true });
+  });
+
+  it("restores the existing target files if promotion fails during reset", async () => {
+    await createTargetFiles(workspacePath, {
+      targetId: "tgt-1",
+      sessionId: "sess-1",
+      workspaceId: "ws-1",
+      objective: "Old objective",
+      createdAt: 1,
+    });
+
+    await saveTargetMemory(workspacePath, "tgt-1", {
+      targetId: "tgt-1",
+      planGenerated: true,
+      plan: [{ id: "step-1", title: "Old step", status: "in_progress" }],
+      activeStepId: "step-1",
+      progressSummary: "In progress",
+      lastGuidance: "Do old thing",
+      stalledCount: 1,
+      updatedAt: 2,
+    });
+
+    await appendTargetCycleRecord(workspacePath, "tgt-1", {
+      cycleId: "cycle-1",
+      targetId: "tgt-1",
+      startedAt: 1,
+      completedAt: 2,
+      result: "continue",
+      reason: "Stale progress",
+      guidance: "Do the old thing",
+      injected: true,
+      attemptCount: 1,
+    });
+
+    renameState.failOnCall = 2;
+
+    await expect(
+      resetTargetFiles(workspacePath, {
+        targetId: "tgt-1",
+        sessionId: "sess-1",
+        workspaceId: "ws-1",
+        objective: "New objective",
+        createdAt: 3,
+      })
+    ).rejects.toThrow("promote failed");
+
+    const meta = await readTargetMeta(workspacePath, "tgt-1");
+    const memory = await loadTargetMemory(workspacePath, "tgt-1");
+    const cycles = await readTargetCycleRecords(workspacePath, "tgt-1");
+
+    expect(meta.objective).toBe("Old objective");
+    expect(memory).toMatchObject({
+      targetId: "tgt-1",
+      planGenerated: true,
+      activeStepId: "step-1",
+      progressSummary: "In progress",
+      lastGuidance: "Do old thing",
+      stalledCount: 1,
+      updatedAt: 2,
+    });
+    expect(cycles).toMatchObject([
+      {
+        cycleId: "cycle-1",
+        guidance: "Do the old thing",
+      },
+    ]);
+  });
+});
diff --git a/packages/server/src/supervisor/target-store.ts b/packages/server/src/supervisor/target-store.ts
