Skip to content

Commit d63cfd4

Browse files
icecrasher321claude
andcommitted
fix(sandboxes): reclaim a ready row whose image was deleted underneath it
Greptile found the hole the previous commit left, and it is the case that made the claim in that commit's message wrong: this one is permanent, not transient. If a re-adopted hash reaches `ready` before the in-flight provider delete lands — plausible, since E2B layer caching can rebuild an identical spec in seconds — the row looks healthy while its imageRef points at nothing. Resolution repairs a row that is missing or failed, never one claiming to be ready, so nothing recovers it. The sandbox stays broken until someone re-saves it by hand. `rebuildIfReadopted` called `ensureSandboxImage` with no options, whose conflict guard reclaims only a failed or stale in-flight row, so it silently did nothing in exactly that case. The release path now passes `imageKnownGone`, which widens the re-claim to any settled row rather than only a failed one. It is the one caller that knows the image is gone regardless of what the row says. An in-flight build is still left alone: it either recreates the template it was building or fails into the normal repair path, and resetting it would only add a duplicate build. The three ways a settled row may be re-claimed now sit in one `settledRebuildBranch` helper — any settled row when the image is known gone, a failed one after the cooldown for an automatic caller, a failed one immediately for a person — because inlining the third case is what hid the gap. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 70ea74b commit d63cfd4

2 files changed

Lines changed: 59 additions & 13 deletions

File tree

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,4 +346,21 @@ describe('ensureSandboxImage failed-build cooldown', () => {
346346
const [failedBranch] = read() as unknown[]
347347
expect(hasTimeBound(failedBranch)).toBe(false)
348348
})
349+
350+
/**
351+
* A row that reached `ready` before the delete landed is the permanent case:
352+
* resolution repairs a missing or `failed` row, never one claiming to be ready,
353+
* so its dead `imageRef` would survive until someone re-saved the sandbox.
354+
*/
355+
it('reclaims a ready row when the image is known to be gone', async () => {
356+
const read = captureSetWhere()
357+
358+
await ensureSandboxImage(SPEC, 'hash-1', { imageKnownGone: true })
359+
360+
const branch = predicateText((read() as unknown[])[0])
361+
expect(branch).toContain('status')
362+
// Excludes only in-flight statuses, so `ready` and `failed` both qualify.
363+
expect(branch).toContain('pending')
364+
expect(branch).not.toContain('failed')
365+
})
349366
})

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

Lines changed: 42 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,38 @@ export interface EnsureSandboxImageOptions {
6666
* run of a scheduled workflow forever.
6767
*/
6868
minFailureAgeMs?: number
69+
/**
70+
* Reclaim a `ready` row as well as a failed one.
71+
*
72+
* Only the release path sets this, and only once it has deleted an image out
73+
* from under a hash something re-adopted. That row looks healthy while its
74+
* `imageRef` points at nothing, and resolution cannot tell: it repairs a row
75+
* that is missing or `failed`, never one claiming to be ready, so without this
76+
* the sandbox stays broken until someone re-saves it by hand.
77+
*
78+
* An in-flight build is still left alone. It either recreates the template it
79+
* was already building or fails into the normal repair path, and resetting it
80+
* would only add a duplicate build.
81+
*/
82+
imageKnownGone?: boolean
83+
}
84+
85+
/**
86+
* Which settled row a save may re-claim: any of them when the image is known to be
87+
* gone, otherwise a failed one — immediately for a person, after the cooldown for
88+
* an automatic caller.
89+
*/
90+
function settledRebuildBranch(options: EnsureSandboxImageOptions): SQL | undefined {
91+
if (options.imageKnownGone) {
92+
return notInArray(sandboxImage.status, ['pending', 'building'])
93+
}
94+
if (options.minFailureAgeMs) {
95+
return and(
96+
eq(sandboxImage.status, 'failed'),
97+
lt(sandboxImage.updatedAt, new Date(Date.now() - options.minFailureAgeMs))
98+
)
99+
}
100+
return eq(sandboxImage.status, 'failed')
69101
}
70102

71103
/**
@@ -112,12 +144,7 @@ export async function ensureSandboxImage(
112144
// row forever, and no later save could revive it because the content address
113145
// never changes.
114146
setWhere: or(
115-
options.minFailureAgeMs
116-
? and(
117-
eq(sandboxImage.status, 'failed'),
118-
lt(sandboxImage.updatedAt, new Date(Date.now() - options.minFailureAgeMs))
119-
)
120-
: eq(sandboxImage.status, 'failed'),
147+
settledRebuildBranch(options),
121148
and(
122149
inArray(sandboxImage.status, ['pending', 'building']),
123150
lt(sandboxImage.updatedAt, new Date(Date.now() - STALE_BUILD_MS))
@@ -338,12 +365,14 @@ type ImageClaimOutcome = 'released' | 'skipped' | 'failed'
338365
* removes. The window is inherent: the registry row and the provider template are
339366
* two systems with no shared transaction, so it can be made small but not zero.
340367
*
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.
368+
* What is avoidable is the adopter being left broken. Its row is new and
369+
* healthy-looking, so nothing else would notice — and a row that reached `ready`
370+
* before the delete landed is worse than slow, it is permanent: resolution repairs
371+
* a row that is missing or `failed`, never one claiming to be ready, so its
372+
* `imageRef` would point at nothing until someone re-saved the sandbox by hand.
373+
* `imageKnownGone` is what lets this reclaim that row. A build still in flight is
374+
* left alone, since it either recreates the template or fails into the normal
375+
* repair path.
347376
*/
348377
async function rebuildIfReadopted(
349378
providerId: string,
@@ -358,7 +387,7 @@ async function rebuildIfReadopted(
358387
if (!readopted) return
359388

360389
logger.warn('Sandbox image was re-adopted mid-delete; rebuilding it now', { specHash })
361-
await ensureSandboxImage(spec, specHash)
390+
await ensureSandboxImage(spec, specHash, { imageKnownGone: true })
362391
}
363392

364393
/**

0 commit comments

Comments
 (0)