Fix Modal cleanup after app settings change

plugins/environment-modal-sandbox/providers/README.md

@@ -36,7 +36,11 @@ The backend must preserve these lifecycle invariants:
 - `create` converges on the durable allocation key and checkpoints an
   allocation before returning its executor.
 - `reconcileCleanup` removes uncertain allocations by key without creating or
-  bootstrapping anything.
+  bootstrapping anything. Modal lists sandboxes tagged with `bbMachineKey` across
+  apps in the credentials' current environment, then terminates and checks each
+  sandbox by ID. Changing App Name does not redirect cleanup. Enumeration or
+  termination failures remain retryable; credentials must still access the
+  original account/environment. No additional persisted allocation state is used.
 - `suspend`, `resume`, and `remove` are idempotent.
 - `suspend` returns a resource from which `resume` can converge; the mechanism
   remains vendor-specific.

plugins/environment-modal-sandbox/providers/modal/backend.ts

@@ -286,11 +286,17 @@ export function createModalSandboxBackend(
     async reconcileCleanup(context: SandboxOperationContext & { key: string }) {
       const resolved = await requireSettings();
       context.signal.throwIfAborted();
-      const sandbox = await clientFor(resolved).fromName(
-        resolved.appName,
-        context.key,
-      );
-      await sandbox?.terminate();
+      const client = clientFor(resolved);
+      for await (const sandbox of client.listByKey(context.key)) {
+        context.signal.throwIfAborted();
+        await sandbox.terminate();
+        if ((await client.fromId(sandbox.sandboxId)) !== null) {
+          throw new Error(
+            "The Modal allocation is still present; retry cleanup.",
+          );
+        }
+      }
+      context.signal.throwIfAborted();
     },
     async suspend(
       context: SandboxLifecycleContext<ModalMachineResource>,

plugins/environment-modal-sandbox/providers/modal/client.test.ts

@@ -8,6 +8,7 @@ const vendor = vi.hoisted(() => ({
   exec: vi.fn(),
   poll: vi.fn(),
   list: vi.fn(),
+  tagged: vi.fn(),
 }));
 vi.mock("modal", () => ({
   NotFoundError: class extends Error {},
@@ -18,6 +19,7 @@ vi.mock("modal", () => ({
       return "main";
     }
     sandboxes = {
+      list: vendor.tagged,
       fromId: async () => ({
         sandboxId: "sandbox-1",
         exec: vendor.exec,
@@ -59,6 +61,7 @@ function processResult() {
 
 beforeEach(() => {
   vendor.exec.mockReset();
+  vendor.tagged.mockReset();
   vendor.poll.mockReset().mockResolvedValue(null);
 });
 
@@ -226,3 +229,30 @@ it("bounds debug output while draining streams and preserving command failure",
     stderr: "abcd\n[output truncated]",
   });
 });
+
+it("lists every tagged sandbox across apps and propagates enumeration failures", async () => {
+  const client = createModalSandboxClient({
+    tokenId: "id",
+    tokenSecret: "secret",
+  });
+  const terminate = vi.fn(async () => {});
+  vendor.tagged.mockImplementation(async function* () {
+    yield { sandboxId: "first-app-sandbox", terminate };
+    yield { sandboxId: "second-app-sandbox", terminate };
+    throw new Error("next page failed");
+  });
+  const ids: string[] = [];
+  await expect(
+    (async () => {
+      for await (const sandbox of client.listByKey("owned-key")) {
+        ids.push(sandbox.sandboxId);
+        await sandbox.terminate();
+      }
+    })(),
+  ).rejects.toThrow("next page failed");
+  expect(ids).toEqual(["first-app-sandbox", "second-app-sandbox"]);
+  expect(terminate).toHaveBeenCalledTimes(2);
+  expect(vendor.tagged).toHaveBeenCalledExactlyOnceWith({
+    tags: { bbMachineKey: "owned-key" },
+  });
+});

plugins/environment-modal-sandbox/providers/modal/client.ts

@@ -70,6 +70,7 @@ export interface ModalSandboxClient {
   deleteSnapshot(imageId: string): Promise<void>;
   fromId(sandboxId: string): Promise<ModalSandboxHandle | null>;
   fromName(appName: string, name: string): Promise<ModalSandboxHandle | null>;
+  listByKey(key: string): AsyncIterable<ModalSandboxHandle>;
 }
 
 export interface ModalCredentials {
@@ -241,6 +242,13 @@ export const createModalSandboxClient: ModalSandboxClientFactory = (
         throw error;
       }
     },
+    async *listByKey(key) {
+      for await (const sandbox of client.sandboxes.list({
+        tags: { bbMachineKey: key },
+      })) {
+        yield wrapSandbox(sandbox);
+      }
+    },
     async fromName(appName, name) {
       try {
         return wrapSandbox(await client.sandboxes.fromName(appName, name));

plugins/environment-modal-sandbox/server.test.ts

@@ -55,6 +55,8 @@ function host(status: Host["status"]): Host {
 interface FakeSandboxState {
   id: string;
   name: string;
+  appName: string;
+  tags: Record<string, string>;
   connected: boolean;
   terminated: boolean;
 }
@@ -118,6 +120,8 @@ function createBackend(
       const state = {
         id: `sandbox-${nextSandbox}`,
         name: request.name,
+        appName: request.appName,
+        tags: request.tags,
         connected: false,
         terminated: false,
       } satisfies FakeSandboxState;
@@ -130,12 +134,21 @@ function createBackend(
       );
       return state === undefined ? null : handle(state);
     },
-    async fromName(_appName, name) {
+    async fromName(appName, name) {
       const state = states.find(
-        (candidate) => candidate.name === name && !candidate.terminated,
+        (candidate) =>
+          candidate.appName === appName &&
+          candidate.name === name &&
+          !candidate.terminated,
       );
       return state === undefined ? null : handle(state);
     },
+    async *listByKey(key) {
+      for (const state of states) {
+        if (!state.terminated && state.tags.bbMachineKey === key)
+          yield handle(state);
+      }
+    },
     async deleteSnapshot(imageId) {
       deletedSnapshots.push(imageId);
     },
@@ -215,6 +228,7 @@ async function setup(
       create: (request) => backend.backend.create(request),
       fromId: (id) => backend.backend.fromId(id),
       fromName: (appName, name) => backend.backend.fromName(appName, name),
+      listByKey: (key) => backend.backend.listByKey(key),
     }),
     now: () => Date.now(),
     sleep: async () => {},
@@ -471,6 +485,84 @@ describe("Modal machine provider", () => {
     expect(test.backend.creates).toHaveLength(1);
   });
 
+  it("cleans matching allocations across apps after checkpoint failure without touching unrelated compute", async () => {
+    const test = await setup({ ...SETTINGS, appName: "original" });
+    try {
+      const context = createContext();
+      context.checkpoint = async () => {
+        throw new Error("checkpoint refused");
+      };
+      expect(await test.provider.create(context)).toMatchObject({
+        status: "failed",
+      });
+      await test.harness.behavior.setSettings({ appName: "changed" });
+      expect(await test.provider.create(context)).toMatchObject({
+        status: "failed",
+      });
+      test.backend.states.push({
+        id: "unrelated",
+        name: context.key,
+        appName: "other",
+        tags: {},
+        connected: false,
+        terminated: false,
+      });
+      expect(await test.provider.reconcileCleanup(context)).toEqual({
+        status: "removed",
+      });
+      expect(test.backend.states.map((state) => state.terminated)).toEqual([
+        true,
+        true,
+        false,
+      ]);
+      expect(await test.provider.reconcileCleanup(context)).toEqual({
+        status: "removed",
+      });
+      expect(test.backend.creates).toHaveLength(2);
+      expect(test.bootstrap).not.toHaveBeenCalled();
+    } finally {
+      await test.harness.lifecycle.dispose();
+    }
+  });
+
+  it.each(["enumeration", "termination", "still running"])(
+    "keeps tagged cleanup retryable after %s failure",
+    async (failure) => {
+      const test = await setup(SETTINGS, {
+        crashAfterTerminateOnce: failure === "termination",
+      });
+      try {
+        const context = createContext();
+        await test.provider.create(context);
+        const list = test.backend.backend.listByKey;
+        const lookup = vi.spyOn(test.backend.backend, "listByKey");
+        if (failure === "enumeration")
+          lookup.mockImplementationOnce(async function* (key) {
+            yield* list(key);
+            throw new Error("enumeration failed");
+          });
+        if (failure === "still running")
+          lookup.mockImplementationOnce(async function* (key) {
+            for await (const sandbox of list(key))
+              yield { ...sandbox, terminate: async () => {} };
+          });
+        expect(await test.provider.reconcileCleanup(context)).toMatchObject({
+          status: "failed",
+        });
+        expect(await test.provider.reconcileCleanup(context)).toEqual({
+          status: "removed",
+        });
+        expect(await test.provider.reconcileCleanup(context)).toEqual({
+          status: "removed",
+        });
+        expect(test.backend.states[0]?.terminated).toBe(true);
+        expect(test.backend.creates).toHaveLength(1);
+      } finally {
+        await test.harness.lifecycle.dispose();
+      }
+    },
+  );
+
   it("preserves resources across provider suspend, resume, and remove callbacks", async () => {
     const harness = await setup({
       ...SETTINGS,
@@ -736,6 +828,8 @@ it("reconciles uncertain named allocations without creating or bootstrapping", a
   test.backend.states.push({
     id: "uncertain",
     name: request.key,
+    appName: "bb-sandboxes",
+    tags: { bbMachineKey: request.key },
     connected: false,
     terminated: false,
   });