Skip to content

Commit 70ea74b

Browse files
icecrasher321claude
andcommitted
fix(sandboxes): rebuild a hash adopted while its image was being deleted
Greptile's third pass on this path, and a case the previous two did not cover: the adopter starting a *fresh build* rather than inheriting a ready row. Claiming removes the registry row, so between that and the provider delete finishing, a workspace can declare the same package list, get a new row, and start a build under the same content-derived imageRef — which the in-flight delete then removes. The window itself is inherent. The registry row and the provider template are two systems with no shared transaction, so it can be narrowed but not closed. A Redis lock would not close it either: acquireLock returns true when Redis is absent, so it cannot be a correctness guarantee for self-hosted. Holding a Postgres advisory lock would, but only by pinning a pooled connection for the length of a provider call, which is a worse trade. What was avoidable is the adopter finding out the slow way. Its row is new and healthy-looking, so nothing noticed: resolution only repairs a row that is missing or failed, and a failed one waits out the retry cooldown first. The release path now re-checks after the delete and re-enqueues, so the rebuild starts immediately instead of one failed run plus a cooldown later. A build already in flight is left to the conflict guard, since it may still outlive the delete. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent d9bcc57 commit 70ea74b

2 files changed

Lines changed: 90 additions & 1 deletion

File tree

apps/sim/lib/execution/remote-sandbox/image-registry.test.ts

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ import {
7272
const READY_IMAGE = {
7373
id: 'img-1',
7474
status: 'ready',
75+
spec: { language: 'python', dependencies: ['pandas'] },
7576
imageRef: 'sim-sbx-abc',
7677
buildId: 'build-1',
7778
providerImageId: 'tmpl-1',
@@ -93,6 +94,25 @@ function predicateText(predicate: unknown): string {
9394
return String(predicate)
9495
}
9596

97+
/**
98+
* True once a build upsert has been issued. Distinguished from the restore path by
99+
* the conflict clause: a rebuild is `onConflictDoUpdate`, a restore is
100+
* `onConflictDoNothing`.
101+
*/
102+
function captureUpsert(): () => boolean {
103+
let seen = false
104+
mockInsert.mockReturnValue({
105+
values: () => ({
106+
onConflictDoUpdate: () => {
107+
seen = true
108+
return { returning: () => Promise.resolve([]) }
109+
},
110+
onConflictDoNothing: () => Promise.resolve(),
111+
}),
112+
})
113+
return () => seen
114+
}
115+
96116
/** Captures the conditional-delete predicate and what the claim resolves to. */
97117
function stubClaim(rows: unknown[]): () => unknown {
98118
let captured: unknown
@@ -176,6 +196,36 @@ describe('releaseSandboxImage', () => {
176196
expect(mockInsert).toHaveBeenCalledTimes(1)
177197
})
178198

199+
/**
200+
* The adopter's row is new and healthy-looking, so nothing else would notice the
201+
* template went out from under it — resolution only repairs a missing or
202+
* `failed` row, and a `failed` one waits out the cooldown first.
203+
*/
204+
it('rebuilds a hash re-adopted while the provider delete was in flight', async () => {
205+
stubClaim([READY_IMAGE])
206+
mockSelect.mockReturnValue({
207+
from: () => ({ where: () => ({ limit: () => Promise.resolve([{ id: 'img-new' }]) }) }),
208+
})
209+
const enqueued = captureUpsert()
210+
211+
await releaseSandboxImage('hash-1')
212+
213+
expect(mockDeleteImage).toHaveBeenCalledTimes(1)
214+
expect(enqueued()).toBe(true)
215+
})
216+
217+
it('does not rebuild when nothing re-adopted the hash', async () => {
218+
stubClaim([READY_IMAGE])
219+
mockSelect.mockReturnValue({
220+
from: () => ({ where: () => ({ limit: () => Promise.resolve([]) }) }),
221+
})
222+
const enqueued = captureUpsert()
223+
224+
await releaseSandboxImage('hash-1')
225+
226+
expect(enqueued()).toBe(false)
227+
})
228+
179229
it('skips the provider when the claimed row never had an image', async () => {
180230
stubClaim([{ ...READY_IMAGE, imageRef: null }])
181231

@@ -195,9 +245,15 @@ describe('releaseSandboxImage', () => {
195245
describe('cleanupSandboxImages', () => {
196246
const CANDIDATE = { id: 'img-1', specHash: 'hash-1', imageRef: 'sim-sbx-abc' }
197247

198-
/** One candidate row from the sweep's nomination query. */
248+
/**
249+
* Nomination query only — later selects (the re-adoption check) resolve empty, so
250+
* a candidate is not mistaken for its own adopter.
251+
*/
199252
function stubCandidates(rows: unknown[]) {
200253
mockSelect.mockReturnValue({
254+
from: () => ({ where: () => ({ limit: () => Promise.resolve([]) }) }),
255+
})
256+
mockSelect.mockReturnValueOnce({
201257
from: () => ({ where: () => ({ limit: () => Promise.resolve(rows) }) }),
202258
})
203259
}

apps/sim/lib/execution/remote-sandbox/image-registry.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,38 @@ export async function releaseSandboxImage(specHash: string): Promise<void> {
329329

330330
type ImageClaimOutcome = 'released' | 'skipped' | 'failed'
331331

332+
/**
333+
* Rebuilds a hash that was adopted while its image was being deleted.
334+
*
335+
* Claiming removes the row, so between that and the provider call finishing a
336+
* workspace can declare the same package list, get a fresh row, and start a build
337+
* under the same content-derived `imageRef` — which the in-flight delete then
338+
* removes. The window is inherent: the registry row and the provider template are
339+
* two systems with no shared transaction, so it can be made small but not zero.
340+
*
341+
* What is avoidable is the adopter finding out the slow way. Its row is new and
342+
* healthy-looking, so nothing else would notice: resolution only repairs a row
343+
* that is missing or `failed`, and a `failed` one waits out
344+
* {@link FAILED_BUILD_RETRY_COOLDOWN_MS} first. Re-enqueueing here converts that
345+
* into a rebuild starting immediately. A build already in flight is left alone by
346+
* the conflict guard, since it may still outlive the delete.
347+
*/
348+
async function rebuildIfReadopted(
349+
providerId: string,
350+
specHash: string,
351+
spec: SandboxSpec
352+
): Promise<void> {
353+
const [readopted] = await db
354+
.select({ id: sandboxImage.id })
355+
.from(sandboxImage)
356+
.where(and(eq(sandboxImage.provider, providerId), eq(sandboxImage.specHash, specHash)))
357+
.limit(1)
358+
if (!readopted) return
359+
360+
logger.warn('Sandbox image was re-adopted mid-delete; rebuilding it now', { specHash })
361+
await ensureSandboxImage(spec, specHash)
362+
}
363+
332364
/**
333365
* Takes ownership of a spec hash's registry row and deletes the image behind it.
334366
*
@@ -383,6 +415,7 @@ async function claimAndDeleteImage(
383415
buildId: claimed.buildId ?? '',
384416
providerImageId: claimed.providerImageId ?? undefined,
385417
})
418+
await rebuildIfReadopted(providerId, specHash, claimed.spec as SandboxSpec)
386419
return 'released'
387420
} catch (error) {
388421
await db

0 commit comments

Comments
 (0)