Skip to content

Commit 55554e5

Browse files
Priya RamanPaperclip-Paperclip
andcommitted
fix(server): record the orphan cleanup row before the compensating teardown
When a conditional lease insert rejected a foreign-company binding, the acquire tore the sandbox down first and recorded the durable pending_cleanup row only after the teardown. If the teardown ran long and the database dropped during it, the durable write failed and the orphan lease id stayed only in the in-process buffer, which a restart loses. Record the durable pending_cleanup row first, while the rejection proves the database reachable, then run the teardown. A successful teardown releases the row to the terminal expired state. If the durable write and the teardown both fail, the acquire buffers the orphan and throws SandboxOrphanCleanupWriteError, as before. A failed release is safe: a later sweep runs the idempotent teardown on the already-gone sandbox and releases the row. Co-authored-by: Paperclip <noreply@paperclip.ing>
1 parent 07cb9d7 commit 55554e5

2 files changed

Lines changed: 341 additions & 123 deletions

File tree

server/src/__tests__/environment-runtime.test.ts

Lines changed: 170 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -633,12 +633,17 @@ describeEmbeddedPostgres("environmentRuntimeService", () => {
633633
details: { code: "environment_company_mismatch" },
634634
});
635635

636-
// The rejected insert leaves no lease row.
636+
// The acquire records the durable pending-cleanup row before the teardown,
637+
// so the row lands while the database is proven reachable. The successful
638+
// teardown then releases the row to the terminal `expired` state, so no
639+
// active or pending_cleanup row remains for the orphan.
637640
const leaseRows = await db
638641
.select()
639642
.from(environmentLeases)
640643
.where(eq(environmentLeases.environmentId, environment.id));
641-
expect(leaseRows).toHaveLength(0);
644+
expect(leaseRows).toHaveLength(1);
645+
expect(leaseRows[0]?.status).toBe("expired");
646+
expect(leaseRows[0]?.cleanupStatus).toBe("success");
642647

643648
// The acquire already provisioned the remote sandbox, so it releases the
644649
// sandbox on the rejection. Without this teardown the rejected insert leaks
@@ -755,12 +760,17 @@ describeEmbeddedPostgres("environmentRuntimeService", () => {
755760
details: { code: "environment_company_mismatch" },
756761
});
757762

758-
// The rejected insert leaves no lease row.
763+
// The acquire records the durable pending-cleanup row before the teardown,
764+
// so the row lands while the database is proven reachable. The successful
765+
// teardown then releases the row to the terminal `expired` state, so no
766+
// active or pending_cleanup row remains for the orphan.
759767
const leaseRows = await db
760768
.select()
761769
.from(environmentLeases)
762770
.where(eq(environmentLeases.environmentId, environment.id));
763-
expect(leaseRows).toHaveLength(0);
771+
expect(leaseRows).toHaveLength(1);
772+
expect(leaseRows[0]?.status).toBe("expired");
773+
expect(leaseRows[0]?.cleanupStatus).toBe("success");
764774

765775
// The acquire provisioned the remote plugin sandbox, so it destroys the
766776
// sandbox on the rejection. Without this teardown the rejected insert leaks a
@@ -832,6 +842,162 @@ describeEmbeddedPostgres("environmentRuntimeService", () => {
832842
}
833843
});
834844

845+
it("records the durable pending-cleanup row before it runs the inline teardown", async () => {
846+
const { companyId, environment, runId } = await seedEnvironment({
847+
driver: "sandbox",
848+
name: "Foreign-bound Fake Sandbox Write First",
849+
config: { provider: "fake", image: "ubuntu:24.04", reuseLease: false },
850+
});
851+
const otherCompanyId = randomUUID();
852+
await db.insert(companies).values({
853+
id: otherCompanyId,
854+
name: "Other Co Write First",
855+
issuePrefix: "OWF",
856+
status: "active",
857+
createdAt: new Date(),
858+
updatedAt: new Date(),
859+
});
860+
await db.insert(builtInManagedResources).values({
861+
companyId: otherCompanyId,
862+
bundleKey: "managed-environment",
863+
resourceKind: "environment",
864+
resourceKey: "managed-sandbox",
865+
resourceId: environment.id,
866+
stockVersion: "1",
867+
stockHash: "hash",
868+
});
869+
870+
// The teardown reads the lease table when it runs. A durable pending_cleanup
871+
// row must already exist, which proves the acquire records the row before the
872+
// teardown. This ordering closes the window the finding describes: the durable
873+
// write lands while the database is proven reachable, before the teardown RPC
874+
// that a database outage could otherwise interrupt.
875+
let pendingCleanupRowsAtTeardown = -1;
876+
const destroySpy = vi
877+
.spyOn(sandboxProviderRuntime, "destroySandboxProviderLease")
878+
.mockImplementation(async () => {
879+
const rows = await db
880+
.select()
881+
.from(environmentLeases)
882+
.where(eq(environmentLeases.environmentId, environment.id));
883+
pendingCleanupRowsAtTeardown = rows.filter((row) => row.status === "pending_cleanup").length;
884+
});
885+
try {
886+
await expect(
887+
runtime.acquireRunLease({
888+
companyId,
889+
environment,
890+
issueId: null,
891+
heartbeatRunId: runId,
892+
persistedExecutionWorkspace: null,
893+
assertCompanyBinding: true,
894+
}),
895+
).rejects.toMatchObject({ status: 403, details: { code: "environment_company_mismatch" } });
896+
897+
// The durable row already tracked the orphan when the teardown ran.
898+
expect(pendingCleanupRowsAtTeardown).toBe(1);
899+
expect(destroySpy).toHaveBeenCalledTimes(1);
900+
901+
// The teardown succeeded, so the acquire released the row to the terminal
902+
// `expired` state and left no pending_cleanup row.
903+
const rows = await db
904+
.select()
905+
.from(environmentLeases)
906+
.where(eq(environmentLeases.environmentId, environment.id));
907+
expect(rows).toHaveLength(1);
908+
expect(rows[0]?.status).toBe("expired");
909+
expect(rows[0]?.cleanupStatus).toBe("success");
910+
} finally {
911+
destroySpy.mockRestore();
912+
}
913+
});
914+
915+
it("leaves no orphan and no error when the durable write fails but the teardown succeeds", async () => {
916+
const { companyId, environment, runId } = await seedEnvironment({
917+
driver: "sandbox",
918+
name: "Foreign-bound Fake Sandbox Write Fail Teardown Ok",
919+
config: { provider: "fake", image: "ubuntu:24.04", reuseLease: false },
920+
});
921+
const otherCompanyId = randomUUID();
922+
await db.insert(companies).values({
923+
id: otherCompanyId,
924+
name: "Other Co Write Fail",
925+
issuePrefix: "OWK",
926+
status: "active",
927+
createdAt: new Date(),
928+
updatedAt: new Date(),
929+
});
930+
await db.insert(builtInManagedResources).values({
931+
companyId: otherCompanyId,
932+
bundleKey: "managed-environment",
933+
resourceKind: "environment",
934+
resourceKey: "managed-sandbox",
935+
resourceId: environment.id,
936+
stockVersion: "1",
937+
stockHash: "hash",
938+
});
939+
940+
// Force every durable pending-cleanup write to fail, but let the teardown
941+
// succeed. A successful teardown removes the orphan, so the acquire needs no
942+
// durable row and raises no orphan-cleanup write error. The write-first order
943+
// never turns a clean teardown into a failure.
944+
const realEnvironmentService = environmentsModule.environmentService;
945+
const factorySpy = vi
946+
.spyOn(environmentsModule, "environmentService")
947+
.mockImplementation((database: Parameters<typeof realEnvironmentService>[0]) => {
948+
const real = realEnvironmentService(database);
949+
return {
950+
...real,
951+
insertPendingCleanupLease: () =>
952+
Promise.reject(new Error("pending-cleanup write failed; database down")),
953+
};
954+
});
955+
const destroySpy = vi
956+
.spyOn(sandboxProviderRuntime, "destroySandboxProviderLease")
957+
.mockResolvedValue(undefined as never);
958+
try {
959+
// Set the retry backoff to zero, so the failed write retries never slow the
960+
// test.
961+
const runtime = environmentRuntimeService(db, { pendingCleanupWriteBackoffMs: 0 });
962+
const rejection = await runtime
963+
.acquireRunLease({
964+
companyId,
965+
environment,
966+
issueId: null,
967+
heartbeatRunId: runId,
968+
persistedExecutionWorkspace: null,
969+
assertCompanyBinding: true,
970+
})
971+
.then(
972+
() => {
973+
throw new Error("acquireRunLease resolved but must reject");
974+
},
975+
(error: unknown) => error,
976+
);
977+
978+
// The rejection is the original company mismatch, not an orphan-cleanup
979+
// write error, because the teardown removed the orphan.
980+
expect(rejection).not.toBeInstanceOf(SandboxOrphanCleanupWriteError);
981+
expect(rejection).toMatchObject({ status: 403, details: { code: "environment_company_mismatch" } });
982+
expect(destroySpy).toHaveBeenCalledTimes(1);
983+
984+
// No orphan remains and no durable row was needed, so the lease table is
985+
// empty for the environment.
986+
const rows = await db
987+
.select()
988+
.from(environmentLeases)
989+
.where(eq(environmentLeases.environmentId, environment.id));
990+
expect(rows).toHaveLength(0);
991+
992+
// The buffer holds no orphan, so a flush recovers nothing.
993+
const flushed = await runtime.flushDeferredOrphanCleanups();
994+
expect(flushed).toEqual({ recovered: 0, pending: 0 });
995+
} finally {
996+
destroySpy.mockRestore();
997+
factorySpy.mockRestore();
998+
}
999+
});
1000+
8351001
it("records a durable pending-cleanup lease when the plugin teardown fails after a foreign-company rejection", async () => {
8361002
const pluginId = randomUUID();
8371003
const { companyId, environment: baseEnvironment, runId } = await seedEnvironment();

0 commit comments

Comments
 (0)