Skip to content

Commit 6ddbfae

Browse files
authored
Fix Modal cleanup after app settings change (#3568)
## Human comments ## What was wrong After Modal creates a sandbox but saving its resource checkpoint fails, cleanup has only the allocation key. Reconciliation looked in the currently configured app, so changing App Name could make cleanup report success while leaving the original sandbox running. This affects the cleanup path introduced in #3274. ## What changed Find sandboxes across apps in the current Modal environment using the existing `bbMachineKey` tag, terminate every match, and verify removal by sandbox ID. Enumeration and termination failures remain retryable. The change stays inside the Modal plugin and requires no new stored fields, migration, core changes, or protocol changes. ## How you verified - The cross-app regression failed before the fix and passes afterward. Tests cover duplicate matching allocations, unrelated sandboxes, repeated cleanup, and enumeration/termination failures followed by retry. - `pnpm exec turbo run test typecheck prepare:bundled lint --filter=bb-plugin-environment-modal-sandbox` passed, including 52 tests. - A local probe exercised the actual cleanup route with migrated SQLite and an injected checkpoint failure. An installed-SDK probe verified tag filtering without an app filter and pagination. Both used simulated external compute; no live Modal API calls were made. > AGENT GENERATED
1 parent f0e353d commit 6ddbfae

5 files changed

Lines changed: 150 additions & 8 deletions

File tree

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

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

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

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -286,11 +286,17 @@ export function createModalSandboxBackend(
286286
async reconcileCleanup(context: SandboxOperationContext & { key: string }) {
287287
const resolved = await requireSettings();
288288
context.signal.throwIfAborted();
289-
const sandbox = await clientFor(resolved).fromName(
290-
resolved.appName,
291-
context.key,
292-
);
293-
await sandbox?.terminate();
289+
const client = clientFor(resolved);
290+
for await (const sandbox of client.listByKey(context.key)) {
291+
context.signal.throwIfAborted();
292+
await sandbox.terminate();
293+
if ((await client.fromId(sandbox.sandboxId)) !== null) {
294+
throw new Error(
295+
"The Modal allocation is still present; retry cleanup.",
296+
);
297+
}
298+
}
299+
context.signal.throwIfAborted();
294300
},
295301
async suspend(
296302
context: SandboxLifecycleContext<ModalMachineResource>,

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ const vendor = vi.hoisted(() => ({
88
exec: vi.fn(),
99
poll: vi.fn(),
1010
list: vi.fn(),
11+
tagged: vi.fn(),
1112
}));
1213
vi.mock("modal", () => ({
1314
NotFoundError: class extends Error {},
@@ -18,6 +19,7 @@ vi.mock("modal", () => ({
1819
return "main";
1920
}
2021
sandboxes = {
22+
list: vendor.tagged,
2123
fromId: async () => ({
2224
sandboxId: "sandbox-1",
2325
exec: vendor.exec,
@@ -59,6 +61,7 @@ function processResult() {
5961

6062
beforeEach(() => {
6163
vendor.exec.mockReset();
64+
vendor.tagged.mockReset();
6265
vendor.poll.mockReset().mockResolvedValue(null);
6366
});
6467

@@ -226,3 +229,30 @@ it("bounds debug output while draining streams and preserving command failure",
226229
stderr: "abcd\n[output truncated]",
227230
});
228231
});
232+
233+
it("lists every tagged sandbox across apps and propagates enumeration failures", async () => {
234+
const client = createModalSandboxClient({
235+
tokenId: "id",
236+
tokenSecret: "secret",
237+
});
238+
const terminate = vi.fn(async () => {});
239+
vendor.tagged.mockImplementation(async function* () {
240+
yield { sandboxId: "first-app-sandbox", terminate };
241+
yield { sandboxId: "second-app-sandbox", terminate };
242+
throw new Error("next page failed");
243+
});
244+
const ids: string[] = [];
245+
await expect(
246+
(async () => {
247+
for await (const sandbox of client.listByKey("owned-key")) {
248+
ids.push(sandbox.sandboxId);
249+
await sandbox.terminate();
250+
}
251+
})(),
252+
).rejects.toThrow("next page failed");
253+
expect(ids).toEqual(["first-app-sandbox", "second-app-sandbox"]);
254+
expect(terminate).toHaveBeenCalledTimes(2);
255+
expect(vendor.tagged).toHaveBeenCalledExactlyOnceWith({
256+
tags: { bbMachineKey: "owned-key" },
257+
});
258+
});

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ export interface ModalSandboxClient {
7070
deleteSnapshot(imageId: string): Promise<void>;
7171
fromId(sandboxId: string): Promise<ModalSandboxHandle | null>;
7272
fromName(appName: string, name: string): Promise<ModalSandboxHandle | null>;
73+
listByKey(key: string): AsyncIterable<ModalSandboxHandle>;
7374
}
7475

7576
export interface ModalCredentials {
@@ -241,6 +242,13 @@ export const createModalSandboxClient: ModalSandboxClientFactory = (
241242
throw error;
242243
}
243244
},
245+
async *listByKey(key) {
246+
for await (const sandbox of client.sandboxes.list({
247+
tags: { bbMachineKey: key },
248+
})) {
249+
yield wrapSandbox(sandbox);
250+
}
251+
},
244252
async fromName(appName, name) {
245253
try {
246254
return wrapSandbox(await client.sandboxes.fromName(appName, name));

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

Lines changed: 96 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ function host(status: Host["status"]): Host {
5555
interface FakeSandboxState {
5656
id: string;
5757
name: string;
58+
appName: string;
59+
tags: Record<string, string>;
5860
connected: boolean;
5961
terminated: boolean;
6062
}
@@ -118,6 +120,8 @@ function createBackend(
118120
const state = {
119121
id: `sandbox-${nextSandbox}`,
120122
name: request.name,
123+
appName: request.appName,
124+
tags: request.tags,
121125
connected: false,
122126
terminated: false,
123127
} satisfies FakeSandboxState;
@@ -130,12 +134,21 @@ function createBackend(
130134
);
131135
return state === undefined ? null : handle(state);
132136
},
133-
async fromName(_appName, name) {
137+
async fromName(appName, name) {
134138
const state = states.find(
135-
(candidate) => candidate.name === name && !candidate.terminated,
139+
(candidate) =>
140+
candidate.appName === appName &&
141+
candidate.name === name &&
142+
!candidate.terminated,
136143
);
137144
return state === undefined ? null : handle(state);
138145
},
146+
async *listByKey(key) {
147+
for (const state of states) {
148+
if (!state.terminated && state.tags.bbMachineKey === key)
149+
yield handle(state);
150+
}
151+
},
139152
async deleteSnapshot(imageId) {
140153
deletedSnapshots.push(imageId);
141154
},
@@ -215,6 +228,7 @@ async function setup(
215228
create: (request) => backend.backend.create(request),
216229
fromId: (id) => backend.backend.fromId(id),
217230
fromName: (appName, name) => backend.backend.fromName(appName, name),
231+
listByKey: (key) => backend.backend.listByKey(key),
218232
}),
219233
now: () => Date.now(),
220234
sleep: async () => {},
@@ -471,6 +485,84 @@ describe("Modal machine provider", () => {
471485
expect(test.backend.creates).toHaveLength(1);
472486
});
473487

488+
it("cleans matching allocations across apps after checkpoint failure without touching unrelated compute", async () => {
489+
const test = await setup({ ...SETTINGS, appName: "original" });
490+
try {
491+
const context = createContext();
492+
context.checkpoint = async () => {
493+
throw new Error("checkpoint refused");
494+
};
495+
expect(await test.provider.create(context)).toMatchObject({
496+
status: "failed",
497+
});
498+
await test.harness.behavior.setSettings({ appName: "changed" });
499+
expect(await test.provider.create(context)).toMatchObject({
500+
status: "failed",
501+
});
502+
test.backend.states.push({
503+
id: "unrelated",
504+
name: context.key,
505+
appName: "other",
506+
tags: {},
507+
connected: false,
508+
terminated: false,
509+
});
510+
expect(await test.provider.reconcileCleanup(context)).toEqual({
511+
status: "removed",
512+
});
513+
expect(test.backend.states.map((state) => state.terminated)).toEqual([
514+
true,
515+
true,
516+
false,
517+
]);
518+
expect(await test.provider.reconcileCleanup(context)).toEqual({
519+
status: "removed",
520+
});
521+
expect(test.backend.creates).toHaveLength(2);
522+
expect(test.bootstrap).not.toHaveBeenCalled();
523+
} finally {
524+
await test.harness.lifecycle.dispose();
525+
}
526+
});
527+
528+
it.each(["enumeration", "termination", "still running"])(
529+
"keeps tagged cleanup retryable after %s failure",
530+
async (failure) => {
531+
const test = await setup(SETTINGS, {
532+
crashAfterTerminateOnce: failure === "termination",
533+
});
534+
try {
535+
const context = createContext();
536+
await test.provider.create(context);
537+
const list = test.backend.backend.listByKey;
538+
const lookup = vi.spyOn(test.backend.backend, "listByKey");
539+
if (failure === "enumeration")
540+
lookup.mockImplementationOnce(async function* (key) {
541+
yield* list(key);
542+
throw new Error("enumeration failed");
543+
});
544+
if (failure === "still running")
545+
lookup.mockImplementationOnce(async function* (key) {
546+
for await (const sandbox of list(key))
547+
yield { ...sandbox, terminate: async () => {} };
548+
});
549+
expect(await test.provider.reconcileCleanup(context)).toMatchObject({
550+
status: "failed",
551+
});
552+
expect(await test.provider.reconcileCleanup(context)).toEqual({
553+
status: "removed",
554+
});
555+
expect(await test.provider.reconcileCleanup(context)).toEqual({
556+
status: "removed",
557+
});
558+
expect(test.backend.states[0]?.terminated).toBe(true);
559+
expect(test.backend.creates).toHaveLength(1);
560+
} finally {
561+
await test.harness.lifecycle.dispose();
562+
}
563+
},
564+
);
565+
474566
it("preserves resources across provider suspend, resume, and remove callbacks", async () => {
475567
const harness = await setup({
476568
...SETTINGS,
@@ -736,6 +828,8 @@ it("reconciles uncertain named allocations without creating or bootstrapping", a
736828
test.backend.states.push({
737829
id: "uncertain",
738830
name: request.key,
831+
appName: "bb-sandboxes",
832+
tags: { bbMachineKey: request.key },
739833
connected: false,
740834
terminated: false,
741835
});

0 commit comments

Comments
 (0)