Skip to content

Commit dd7aa1c

Browse files
Bill LeoutsakosBill Leoutsakos
authored andcommitted
fix(billing): harden sandbox usage failure recovery
1 parent 96449b0 commit dd7aa1c

9 files changed

Lines changed: 345 additions & 46 deletions

File tree

apps/sim/lib/billing/sandbox-usage-outbox.test.ts

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,13 @@ function costTotalUpdate(index: number): { text: string; params: unknown[] } {
9595
}
9696
}
9797

98+
function executedSqlIndex(substring: string): number {
99+
return dbChainMockFns.execute.mock.calls.findIndex(([query]) => {
100+
const strings = (query as { strings?: readonly string[] } | null)?.strings
101+
return Array.isArray(strings) && strings.some((value) => value.includes(substring))
102+
})
103+
}
104+
98105
afterAll(resetDbChainMock)
99106

100107
describe('sandbox usage outbox finalizer', () => {
@@ -126,7 +133,9 @@ describe('sandbox usage outbox finalizer', () => {
126133
],
127134
})
128135
)
129-
expect(dbChainMockFns.execute).toHaveBeenCalledOnce()
136+
expect(dbChainMockFns.execute).toHaveBeenCalledTimes(2)
137+
expect(executedSqlIndex('lock_timeout')).toBe(0)
138+
expect(executedSqlIndex('pg_advisory_xact_lock')).toBe(1)
130139
expect(dbChainMockFns.update).toHaveBeenCalled()
131140
expect(costTotalUpdate(0)).toEqual({
132141
text: 'GREATEST(COALESCE(, 0) + ::numeric, ::numeric)',
@@ -160,6 +169,18 @@ describe('sandbox usage outbox finalizer', () => {
160169
expect(mockRecordUsage).not.toHaveBeenCalled()
161170
})
162171

172+
it('propagates advisory lock timeouts so the generic outbox retries', async () => {
173+
const lockTimeout = Object.assign(new Error('canceling statement due to lock timeout'), {
174+
code: '55P03',
175+
})
176+
dbChainMockFns.execute.mockResolvedValueOnce([]).mockRejectedValueOnce(lockTimeout)
177+
178+
await expect(runHandler(payload())).rejects.toBe(lockTimeout)
179+
expect(executedSqlIndex('lock_timeout')).toBe(0)
180+
expect(executedSqlIndex('pg_advisory_xact_lock')).toBe(1)
181+
expect(mockRecordUsage).not.toHaveBeenCalled()
182+
})
183+
163184
it('uses the same ledger event key when finalization is replayed', async () => {
164185
mockRecordUsage.mockResolvedValueOnce(0.00046).mockResolvedValueOnce(0)
165186
queueTableRows(usageLog, [{ cost: '0.02' }])

apps/sim/lib/billing/sandbox-usage-outbox.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import {
1919
import { checkAndBillPayerOverageThreshold } from '@/lib/billing/threshold-billing'
2020
import { isBillingEnabled } from '@/lib/core/config/env-flags'
2121
import {
22-
enqueueOutboxEvent,
22+
enqueueOutboxEventIfAbsent,
2323
type OutboxEventContext,
2424
type OutboxHandlerRegistry,
2525
patchAndReleasePendingOutboxEvent,
@@ -36,6 +36,7 @@ const logger = createLogger('SandboxUsageOutbox')
3636

3737
export const SANDBOX_USAGE_OUTBOX_EVENT_TYPE = 'sandbox.usage.finalize'
3838
const CRASH_RECOVERY_GRACE_MS = 60_000
39+
const SANDBOX_USAGE_LOCK_TIMEOUT_MS = 10_000
3940

4041
export type SandboxUsageCleanupStatus = 'active' | 'terminated' | 'pending_reconciliation'
4142

@@ -199,7 +200,7 @@ export async function beginSandboxUsage(params: BeginSandboxUsageParams): Promis
199200
pricing: createSandboxPricingSnapshot(params.provider),
200201
}
201202

202-
await enqueueOutboxEvent(db, SANDBOX_USAGE_OUTBOX_EVENT_TYPE, payload, {
203+
await enqueueOutboxEventIfAbsent(db, SANDBOX_USAGE_OUTBOX_EVENT_TYPE, payload, {
203204
id: eventId,
204205
availableAt: new Date(params.providerExpiresAt.getTime() + CRASH_RECOVERY_GRACE_MS),
205206
})
@@ -276,6 +277,9 @@ async function finalizeSandboxUsage(
276277
const billingContext = toBillingContext(payload.billingAttribution)
277278

278279
await db.transaction(async (tx) => {
280+
await tx.execute(
281+
sql`select set_config('lock_timeout', ${`${SANDBOX_USAGE_LOCK_TIMEOUT_MS}ms`}, true)`
282+
)
279283
await tx.execute(sql`select pg_advisory_xact_lock(hashtextextended(${payload.executionId}, 0))`)
280284
const insertedCost = await recordUsage({
281285
userId: payload.billingAttribution.actorUserId,

apps/sim/lib/core/outbox/service.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import {
2828
deferOutboxHandler,
2929
enqueueOrReschedulePendingOutboxEvent,
3030
enqueueOutboxEvent,
31+
enqueueOutboxEventIfAbsent,
3132
enqueueOutboxEvents,
3233
outboxEventHasSourceOperationId,
3334
outboxPayloadHasSourceOperationId,
@@ -98,6 +99,19 @@ describe('enqueueOutboxEvent', () => {
9899
)
99100
})
100101

102+
it('ensures a stable event exists without failing on a duplicate ID', async () => {
103+
const id = await enqueueOutboxEventIfAbsent(
104+
dbChainMock.db,
105+
'test.event',
106+
{ foo: 'bar' },
107+
{ id: 'stable-event-id' }
108+
)
109+
110+
expect(id).toBe('stable-event-id')
111+
expect(dbChainMockFns.values.mock.calls[0][0]).toMatchObject({ id: 'stable-event-id' })
112+
expect(dbChainMockFns.onConflictDoNothing).toHaveBeenCalledWith({ target: outboxEvent.id })
113+
})
114+
101115
it('inserts a bounded event batch in one statement', async () => {
102116
const ids = await enqueueOutboxEvents(dbChainMock.db, 'test.event', [
103117
{ sequence: 0 },

apps/sim/lib/core/outbox/service.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,34 @@ export async function enqueueOutboxEvent<T>(
174174
return id
175175
}
176176

177+
/**
178+
* Ensures an outbox event with a caller-owned stable ID exists.
179+
*
180+
* This is reserved for recovery after an insert returned an indeterminate
181+
* result: PostgreSQL may have committed the first insert before the client saw
182+
* a connection failure, so retrying must collapse on the event ID instead of
183+
* surfacing a duplicate-key error.
184+
*/
185+
export async function enqueueOutboxEventIfAbsent<T>(
186+
executor: Pick<typeof db, 'insert'>,
187+
eventType: string,
188+
payload: T,
189+
options: EnqueueOptions & { id: string }
190+
): Promise<string> {
191+
await executor
192+
.insert(outboxEvent)
193+
.values({
194+
id: options.id,
195+
eventType,
196+
payload: payload as never,
197+
maxAttempts: options.maxAttempts ?? DEFAULT_MAX_ATTEMPTS,
198+
availableAt: options.availableAt ?? new Date(),
199+
})
200+
.onConflictDoNothing({ target: outboxEvent.id })
201+
logger.info('Ensured outbox event exists', { id: options.id, eventType })
202+
return options.id
203+
}
204+
177205
export async function enqueueOutboxEvents<T>(
178206
executor: Pick<typeof db, 'insert'>,
179207
eventType: string,

apps/sim/lib/execution/non-retryable-error.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ export class SandboxUsagePersistenceError extends NonRetryableExecutionError {
3232

3333
constructor(provider: string, options?: ErrorOptions) {
3434
super(
35-
`${provider} created a Function sandbox, but Sim could not persist its usage record. The sandbox was stopped before user code ran.`,
35+
`${provider} created a Function sandbox, but Sim could not persist its usage record. User code was not run, and Sim initiated sandbox cleanup before returning.`,
3636
options
3737
)
3838
this.name = 'SandboxUsagePersistenceError'

apps/sim/lib/execution/remote-sandbox/conformance.test.ts

Lines changed: 98 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,16 @@ import type { SandboxUsageContext } from '@/lib/execution/remote-sandbox/types'
153153
type Provider = 'e2b' | 'daytona'
154154
const PROVIDERS: Provider[] = ['e2b', 'daytona']
155155

156+
function deferred<T>() {
157+
let resolve!: (value: T | PromiseLike<T>) => void
158+
let reject!: (reason?: unknown) => void
159+
const promise = new Promise<T>((resolvePromise, rejectPromise) => {
160+
resolve = resolvePromise
161+
reject = rejectPromise
162+
})
163+
return { promise, resolve, reject }
164+
}
165+
156166
const usageContext: SandboxUsageContext = {
157167
workspaceId: 'ws-1',
158168
workflowId: 'wf-1',
@@ -405,7 +415,7 @@ describe.each(PROVIDERS)('sandbox conformance [%s]', (provider) => {
405415
)
406416
})
407417

408-
it('kills without running user code when initial usage persistence fails', async () => {
418+
it('kills and recovers usage without running user code when initial persistence fails', async () => {
409419
mockBeginSandboxUsage.mockRejectedValueOnce(new Error('database unavailable'))
410420

411421
await expect(
@@ -424,7 +434,94 @@ describe.each(PROVIDERS)('sandbox conformance [%s]', (provider) => {
424434
expect(
425435
provider === 'e2b' ? mockE2BCommandsRun : mockExecuteSessionCommand
426436
).not.toHaveBeenCalled()
437+
expect(mockBeginSandboxUsage).toHaveBeenCalledTimes(2)
438+
expect(mockReleaseAndProcessSandboxUsage).toHaveBeenCalledWith(
439+
'sandbox-usage-event',
440+
expect.objectContaining({
441+
outcome: 'infrastructure_error',
442+
cleanupStatus: 'terminated',
443+
})
444+
)
445+
})
446+
447+
it('preserves usage persistence failure when cancellation races admission', async () => {
448+
const controller = new AbortController()
449+
const persistence = deferred<string>()
450+
mockBeginSandboxUsage.mockImplementationOnce(() => persistence.promise)
451+
452+
const execution = executeInSandbox({
453+
code: 'x',
454+
language: CodeLanguage.Python,
455+
timeoutMs: 1000,
456+
signal: controller.signal,
457+
usageContext,
458+
})
459+
const rejection = expect(execution).rejects.toMatchObject({
460+
name: 'SandboxUsagePersistenceError',
461+
retryable: false,
462+
})
463+
464+
await vi.waitFor(() => expect(mockBeginSandboxUsage).toHaveBeenCalledOnce())
465+
controller.abort(new DOMException('cancelled', 'AbortError'))
466+
persistence.reject(new Error('database unavailable'))
467+
468+
await rejection
469+
expect(
470+
provider === 'e2b' ? mockE2BCommandsRun : mockExecuteSessionCommand
471+
).not.toHaveBeenCalled()
472+
})
473+
474+
it('durably releases cleanup when persistence and live teardown initially fail', async () => {
475+
mockBeginSandboxUsage.mockRejectedValueOnce(new Error('database unavailable'))
476+
const teardown = provider === 'e2b' ? mockE2BKill : mockDelete
477+
teardown.mockRejectedValue(new Error('provider unavailable'))
478+
479+
await expect(
480+
executeInSandbox({
481+
code: 'x',
482+
language: CodeLanguage.Python,
483+
timeoutMs: 1000,
484+
usageContext,
485+
})
486+
).rejects.toMatchObject({ name: 'SandboxUsagePersistenceError', retryable: false })
487+
488+
expect(teardown).toHaveBeenCalledTimes(2)
489+
expect(mockBeginSandboxUsage).toHaveBeenCalledTimes(2)
490+
expect(mockReleaseAndProcessSandboxUsage).toHaveBeenCalledWith(
491+
'sandbox-usage-event',
492+
expect.objectContaining({
493+
outcome: 'infrastructure_error',
494+
cleanupStatus: 'pending_reconciliation',
495+
})
496+
)
497+
expect(
498+
provider === 'e2b' ? mockE2BCommandsRun : mockExecuteSessionCommand
499+
).not.toHaveBeenCalled()
500+
})
501+
502+
it('falls back to termination by ID when persistence recovery also fails', async () => {
503+
mockBeginSandboxUsage.mockRejectedValue(new Error('database unavailable'))
504+
const teardown = provider === 'e2b' ? mockE2BKill : mockDelete
505+
teardown.mockRejectedValue(new Error('provider unavailable'))
506+
507+
await expect(
508+
executeInSandbox({
509+
code: 'x',
510+
language: CodeLanguage.Python,
511+
timeoutMs: 1000,
512+
usageContext,
513+
})
514+
).rejects.toMatchObject({ name: 'SandboxUsagePersistenceError', retryable: false })
515+
516+
if (provider === 'e2b') {
517+
expect(mockE2BStaticKill).toHaveBeenCalledWith('sb_1', { apiKey: 'test-key' })
518+
} else {
519+
expect(mockDaytonaGet).toHaveBeenCalledWith('sb_1')
520+
}
427521
expect(mockReleaseAndProcessSandboxUsage).not.toHaveBeenCalled()
522+
expect(
523+
provider === 'e2b' ? mockE2BCommandsRun : mockExecuteSessionCommand
524+
).not.toHaveBeenCalled()
428525
})
429526

430527
it('releases returned user errors as billable usage', async () => {

0 commit comments

Comments
 (0)