Skip to content

Commit 4e21e64

Browse files
icecrasher321claude
andcommitted
fix(sandboxes): key the build trigger by attempt, not by spec
Cursor Bugbot. The Trigger.dev idempotency key was the content address alone, so a second attempt at the same spec was deduped against the first: the SDK returns the finished run instead of starting one, and the row that `ensureSandboxImage` just flipped to `pending` sits there with no worker. Nothing can re-claim a `pending` row until it goes stale, so a retry inside the 5-minute TTL did nothing for the next half hour. That silently disabled every repair path — save-to-retry, which the docs name explicitly, and both the resolution and create-time rebuilds. The key's own comment already said it exists "to collapse concurrent saves of the same spec into one build, not to suppress a retry after one failed". The conditional update above it is what actually collapses concurrent saves: only one caller gets a row back, so only one ever reaches the trigger. Keying by the claim's `updatedAt` keeps that property and makes each genuine attempt distinct, while a duplicate delivery of one attempt still collapses. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent af63079 commit 4e21e64

2 files changed

Lines changed: 56 additions & 9 deletions

File tree

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ import {
6767
ensureSandboxImage,
6868
FAILED_BUILD_RETRY_COOLDOWN_MS,
6969
releaseSandboxImage,
70+
sandboxBuildIdempotencyKey,
7071
} from '@/lib/execution/remote-sandbox/image-registry'
7172

7273
const READY_IMAGE = {
@@ -404,3 +405,34 @@ describe('ensureSandboxImage failed-build cooldown', () => {
404405
expect(branch).not.toContain('failed')
405406
})
406407
})
408+
409+
/**
410+
* The claim already collapses concurrent saves, so the trigger key only has to
411+
* distinguish attempts. Keyed by spec alone it suppressed the next legitimate one
412+
* instead — Trigger.dev returns the finished run, the row stays `pending` with no
413+
* worker, and nothing can re-claim it until it goes stale.
414+
*/
415+
describe('sandboxBuildIdempotencyKey', () => {
416+
it('differs between two attempts at the same spec', () => {
417+
const first = sandboxBuildIdempotencyKey('e2b', 'hash-1', new Date(1_000))
418+
const second = sandboxBuildIdempotencyKey('e2b', 'hash-1', new Date(2_000))
419+
420+
expect(first).not.toBe(second)
421+
})
422+
423+
it('still collapses a duplicate delivery of one attempt', () => {
424+
const attemptAt = new Date(1_000)
425+
426+
expect(sandboxBuildIdempotencyKey('e2b', 'hash-1', attemptAt)).toBe(
427+
sandboxBuildIdempotencyKey('e2b', 'hash-1', attemptAt)
428+
)
429+
})
430+
431+
it('keeps providers apart for the same content address', () => {
432+
const attemptAt = new Date(1_000)
433+
434+
expect(sandboxBuildIdempotencyKey('e2b', 'hash-1', attemptAt)).not.toBe(
435+
sandboxBuildIdempotencyKey('daytona', 'hash-1', attemptAt)
436+
)
437+
})
438+
})

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

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,12 @@ export interface SandboxImageBuildPayload {
4343
* so two workspaces declaring identical dependencies share the key as well as
4444
* the resulting image.
4545
*/
46-
export function sandboxBuildIdempotencyKey(provider: string, specHash: string): string {
47-
return `sandbox-image-${provider}-${specHash}`
46+
export function sandboxBuildIdempotencyKey(
47+
provider: string,
48+
specHash: string,
49+
attemptAt: Date
50+
): string {
51+
return `sandbox-image-${provider}-${specHash}-${attemptAt.getTime()}`
4852
}
4953

5054
/**
@@ -151,16 +155,23 @@ export async function ensureSandboxImage(
151155
)
152156
),
153157
})
154-
.returning({ id: sandboxImage.id, status: sandboxImage.status })
158+
.returning({
159+
id: sandboxImage.id,
160+
status: sandboxImage.status,
161+
updatedAt: sandboxImage.updatedAt,
162+
})
155163

156164
// No row returned means the conflict target matched but `setWhere` rejected the
157165
// update — an existing `ready`, `pending`, or `building` row. Nothing to do.
158166
if (inserted.length === 0) return
159167

160-
await enqueueSandboxImageBuild({ provider: provider.id, specHash })
168+
await enqueueSandboxImageBuild({ provider: provider.id, specHash }, inserted[0].updatedAt)
161169
}
162170

163-
async function enqueueSandboxImageBuild(payload: SandboxImageBuildPayload): Promise<void> {
171+
async function enqueueSandboxImageBuild(
172+
payload: SandboxImageBuildPayload,
173+
attemptAt: Date
174+
): Promise<void> {
164175
if (!isTriggerDevEnabled) {
165176
runDetached('sandbox-image-build', () => runSandboxImageBuild(payload))
166177
return
@@ -172,10 +183,14 @@ async function enqueueSandboxImageBuild(payload: SandboxImageBuildPayload): Prom
172183
import('@/lib/core/async-jobs/region'),
173184
])
174185
await tasks.trigger<typeof sandboxImageBuildTask>('sandbox-image-build', payload, {
175-
idempotencyKey: sandboxBuildIdempotencyKey(payload.provider, payload.specHash),
176-
// Short TTL on purpose: the key exists to collapse concurrent saves of the
177-
// same spec into one build, not to suppress a retry after one failed. The
178-
// default 30-day window would make a transient failure permanent.
186+
// Keyed by the claim, not by the spec alone. Concurrent saves are already
187+
// collapsed by the conditional update above — only one caller gets a row back,
188+
// so only one reaches here. A spec-only key would instead suppress the *next*
189+
// legitimate attempt: Trigger.dev returns the finished run rather than starting
190+
// one, leaving the row `pending` with no worker and unclaimable until it goes
191+
// stale. That is half an hour of a save-to-retry doing nothing.
192+
idempotencyKey: sandboxBuildIdempotencyKey(payload.provider, payload.specHash, attemptAt),
193+
// Still short, so even a duplicate delivery of one attempt cannot linger.
179194
idempotencyKeyTTL: '5m',
180195
tags: [`sandboxSpec:${payload.specHash}`],
181196
region: await resolveTriggerRegion(),

0 commit comments

Comments
 (0)