diff --git a/plugins/environment-modal-sandbox/providers/README.md b/plugins/environment-modal-sandbox/providers/README.md index 6d5e91b2ad..8d7ef2124e 100644 --- a/plugins/environment-modal-sandbox/providers/README.md +++ b/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. diff --git a/plugins/environment-modal-sandbox/providers/modal/backend.ts b/plugins/environment-modal-sandbox/providers/modal/backend.ts index 1840b00dc4..89324b69f7 100644 --- a/plugins/environment-modal-sandbox/providers/modal/backend.ts +++ b/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, diff --git a/plugins/environment-modal-sandbox/providers/modal/client.test.ts b/plugins/environment-modal-sandbox/providers/modal/client.test.ts index fbb0c67a92..35fb1096ba 100644 --- a/plugins/environment-modal-sandbox/providers/modal/client.test.ts +++ b/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" }, + }); +}); diff --git a/plugins/environment-modal-sandbox/providers/modal/client.ts b/plugins/environment-modal-sandbox/providers/modal/client.ts index 6ba0712154..8eabc7c077 100644 --- a/plugins/environment-modal-sandbox/providers/modal/client.ts +++ b/plugins/environment-modal-sandbox/providers/modal/client.ts @@ -70,6 +70,7 @@ export interface ModalSandboxClient { deleteSnapshot(imageId: string): Promise; fromId(sandboxId: string): Promise; fromName(appName: string, name: string): Promise; + listByKey(key: string): AsyncIterable; } 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)); diff --git a/plugins/environment-modal-sandbox/server.test.ts b/plugins/environment-modal-sandbox/server.test.ts index edcc9739de..3f5ef6ee93 100644 --- a/plugins/environment-modal-sandbox/server.test.ts +++ b/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; 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, });