Skip to content

Commit f716fa6

Browse files
icecrasher321claude
andcommitted
fix(pi): size the sandbox to the run's own execution timeout
The Pi sandbox lifetime was a global constant while the execution timeout is per-plan and per-mode, varying 18x (a free sync run gets 5 minutes, an async run 90). Every Pi sandbox asked E2B for the same sub-hour ceiling, so a five-minute run whose web process died left a sandbox billing for an hour. PI_SANDBOX_LIFETIME_MS could not close the gap: its floor is 31 minutes. resolvePiRunLifetimeMs lowers the provider ceiling to whatever the run's own deadline leaves, read from the signal that enforces it. Untimed runs and Daytona are unchanged, so no path gets a longer lifetime than before. The turn cap had to move with it. PI_TIMEOUT_MS reserved the clone and both finalize budgets out of the ceiling as a module constant; leaving it there while shrinking the lifetime would re-open the exact bug its docs describe — the sandbox dying first, taking the agent's finished work with it unpushed. It is now resolvePiTimeoutMs(lifetimeMs), and each backend resolves the lifetime once and feeds both, so the two cannot disagree. Two things this surfaced: Babysit had to read context.signal, not the cancellation signal it uses everywhere else. createCancellationSignal returns a fresh controller that only forwards aborts, so the deadline lookup answers "unknown" through it and would have silently left the longest-lived mode on the ceiling. Covered by a test that fails against the wrong signal. The E2B adapter tested lifetimeMs for truthiness, so a run resolving to zero would have had the key dropped and been handed the SDK's five-minute default - longer than it asked for, on the run least entitled to it. Options precede the callback in withPiSandbox so that adding one did not re-indent every caller's sandbox body. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 93b78fb commit f716fa6

13 files changed

Lines changed: 297 additions & 70 deletions

apps/sim/executor/handlers/pi/babysit-backend.test.ts

Lines changed: 55 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ vi.mock('@/executor/handlers/pi/babysit-github', async (importOriginal) => {
5959
}
6060
})
6161

62-
import { getMaxExecutionTimeout } from '@/lib/core/execution-limits'
62+
import { createTimeoutAbortController, getMaxExecutionTimeout } from '@/lib/core/execution-limits'
6363
import {
6464
resolveBabysitExecutionBudgetMs,
6565
runBabysitPiWithOptions,
@@ -283,6 +283,37 @@ describe('runBabysitPiWithOptions', () => {
283283
expect(mockWithPiSandbox).not.toHaveBeenCalled()
284284
})
285285

286+
it("creates its sandbox against the execution's deadline, not the provider ceiling", async () => {
287+
mockFetchSnapshot.mockResolvedValue(snapshot)
288+
mockFetchThreads.mockResolvedValue({
289+
actionable: [],
290+
skipped: [],
291+
totalUnresolved: 0,
292+
latestReview: null,
293+
})
294+
mockFetchChecks.mockResolvedValue(greenChecks)
295+
const { runner } = makeRunner({})
296+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
297+
298+
// Babysit wraps `context.signal` in its own cancellation controller, and the
299+
// deadline is recorded against the executor's signal alone. Resolving the
300+
// lifetime from that wrapper answers "unknown" and silently falls back to the
301+
// provider ceiling — the run still succeeds, it just over-reserves the
302+
// sandbox. This is the longest-lived Pi mode, so that regression matters most
303+
// here and is invisible without an assertion.
304+
const timeout = createTimeoutAbortController(6 * 60 * 1000)
305+
await runBabysitPiWithOptions(
306+
params(),
307+
{ onEvent: vi.fn(), signal: timeout.signal },
308+
{ roundWaitMs: 0 }
309+
)
310+
311+
const [{ lifetimeMs }] = mockWithPiSandbox.mock.calls[0]
312+
expect(lifetimeMs).toBeLessThanOrEqual(6 * 60 * 1000)
313+
expect(lifetimeMs).toBeGreaterThan(5 * 60 * 1000)
314+
timeout.cleanup()
315+
})
316+
286317
it('requests the initial review and waits without consuming a round when the PR starts clean', async () => {
287318
mockFetchSnapshot.mockResolvedValue(snapshot)
288319
mockFetchThreads.mockResolvedValue({
@@ -293,7 +324,7 @@ describe('runBabysitPiWithOptions', () => {
293324
})
294325
mockFetchChecks.mockResolvedValue(greenChecks)
295326
const { runner } = makeRunner({})
296-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
327+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
297328

298329
const result = await runBabysitPiWithOptions(params(), { onEvent: vi.fn() }, { roundWaitMs: 0 })
299330

@@ -325,7 +356,7 @@ describe('runBabysitPiWithOptions', () => {
325356
const { runner } = makeRunner({
326357
cloneResult: commandResult('', 'clone failed', 1),
327358
})
328-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
359+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
329360

330361
const result = await runBabysitPiWithOptions(params(), { onEvent: vi.fn() })
331362

@@ -348,7 +379,7 @@ describe('runBabysitPiWithOptions', () => {
348379
})
349380
mockFetchChecks.mockResolvedValue(greenChecks)
350381
const { runner } = makeRunner({})
351-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
382+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
352383

353384
const result = await runBabysitPiWithOptions(
354385
params({ executionBudgetMs: 2 * 60 * 1000 }),
@@ -375,7 +406,7 @@ describe('runBabysitPiWithOptions', () => {
375406
})
376407
mockFetchChecks.mockResolvedValue(greenChecks)
377408
const { runner } = makeRunner({})
378-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
409+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
379410

380411
const result = await runBabysitPiWithOptions(params(), { onEvent: vi.fn() }, { roundWaitMs: 0 })
381412

@@ -409,7 +440,7 @@ describe('runBabysitPiWithOptions', () => {
409440
contextRequirements: new Map(failures.map((check) => [check.key, true])),
410441
})
411442
const { runner, runCalls } = makeRunner({})
412-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
443+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
413444

414445
const result = await runBabysitPiWithOptions(params(), { onEvent: vi.fn() })
415446

@@ -489,7 +520,7 @@ describe('runBabysitPiWithOptions', () => {
489520
throw new Error(`Unexpected read ${path}`)
490521
}),
491522
}
492-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
523+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
493524

494525
const result = await runBabysitPiWithOptions(
495526
params(),
@@ -543,7 +574,7 @@ describe('runBabysitPiWithOptions', () => {
543574
})
544575
mockFetchChecks.mockResolvedValue(greenChecks)
545576
const { runner, runCalls } = makeRunner({})
546-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
577+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
547578

548579
await runBabysitPiWithOptions(params(), { onEvent: vi.fn() })
549580

@@ -605,7 +636,7 @@ describe('runBabysitPiWithOptions', () => {
605636
roundFile: JSON.stringify({ threads: [] }),
606637
diff: ['round-one-diff', 'round-two-diff'],
607638
})
608-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
639+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
609640

610641
const result = await runBabysitPiWithOptions(
611642
params({ reviewMentions: ['@review-bot'] }),
@@ -641,7 +672,7 @@ describe('runBabysitPiWithOptions', () => {
641672
const { runner, runCalls } = makeRunner({
642673
prepareStdout: `__CUMULATIVE_CHANGED__=.github/workflows/ci.yml\n__CUMULATIVE_DIFF_BYTES__=20\n__CHANGED__=.github/workflows/ci.yml\n__NEW_SHA__=${NEW_SHA}\n__NEEDS_PUSH__=1\n`,
643674
})
644-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
675+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
645676

646677
const result = await runBabysitPiWithOptions(params(), { onEvent: vi.fn() })
647678

@@ -671,7 +702,7 @@ describe('runBabysitPiWithOptions', () => {
671702
const { runner, runCalls } = makeRunner({
672703
prepareStdout: `__CUMULATIVE_CHANGED__=${unicodePath}\n__CUMULATIVE_DIFF_BYTES__=20\n__CHANGED__=${unicodePath}\n__NEW_SHA__=${NEW_SHA}\n__NEEDS_PUSH__=1\n`,
673704
})
674-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
705+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
675706

676707
const result = await runBabysitPiWithOptions(params(), { onEvent: vi.fn() })
677708

@@ -701,7 +732,7 @@ describe('runBabysitPiWithOptions', () => {
701732
const { runner, runCalls } = makeRunner({
702733
prepareStdout: `__CUMULATIVE_CHANGED__=${quotedPath}\n__CUMULATIVE_DIFF_BYTES__=20\n__CHANGED__=${quotedPath}\n__NEW_SHA__=${NEW_SHA}\n__NEEDS_PUSH__=1\n`,
703734
})
704-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
735+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
705736

706737
const result = await runBabysitPiWithOptions(params(), { onEvent: vi.fn() })
707738

@@ -721,7 +752,7 @@ describe('runBabysitPiWithOptions', () => {
721752
const { runner } = makeRunner({
722753
pushResult: commandResult('', 'rejected by remote', 1),
723754
})
724-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
755+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
725756

726757
const result = await runBabysitPiWithOptions(params(), { onEvent: vi.fn() })
727758

@@ -747,7 +778,7 @@ describe('runBabysitPiWithOptions', () => {
747778
const { runner, runCalls } = makeRunner({
748779
prepareStdout: '__NEEDS_PUSH__=1\n',
749780
})
750-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
781+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
751782

752783
const result = await runBabysitPiWithOptions(params(), { onEvent: vi.fn() })
753784

@@ -772,7 +803,7 @@ describe('runBabysitPiWithOptions', () => {
772803
})
773804
mockFetchChecks.mockResolvedValue(greenChecks)
774805
const { runner, runCalls } = makeRunner({})
775-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
806+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
776807

777808
const result = await runBabysitPiWithOptions(params(), { onEvent: vi.fn() })
778809

@@ -814,7 +845,7 @@ describe('runBabysitPiWithOptions', () => {
814845
})
815846
mockFetchChecks.mockResolvedValueOnce(pendingChecks).mockResolvedValueOnce(greenChecks)
816847
const { runner, runCalls } = makeRunner({})
817-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
848+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
818849

819850
const result = await runBabysitPiWithOptions(
820851
params({ maxRounds: 1 }),
@@ -846,7 +877,7 @@ describe('runBabysitPiWithOptions', () => {
846877
awaitingConfirmation: true,
847878
})
848879
const { runner } = makeRunner({})
849-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
880+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
850881

851882
const result = await runBabysitPiWithOptions(
852883
params(),
@@ -878,7 +909,7 @@ describe('runBabysitPiWithOptions', () => {
878909
})
879910
mockFetchChecks.mockResolvedValue(noChecksGreen)
880911
const { runner } = makeRunner({})
881-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
912+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
882913

883914
const result = await runBabysitPiWithOptions(
884915
params({ reviewMentions: ['@review-bot'] }),
@@ -910,7 +941,7 @@ describe('runBabysitPiWithOptions', () => {
910941
})
911942
mockFetchChecks.mockResolvedValue(noChecksGreen)
912943
const { runner } = makeRunner({})
913-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
944+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
914945

915946
const result = await runBabysitPiWithOptions(
916947
params(),
@@ -942,7 +973,7 @@ describe('runBabysitPiWithOptions', () => {
942973
})
943974
mockFetchChecks.mockResolvedValue(greenChecks)
944975
const { runner } = makeRunner({ roundFile: 'not json' })
945-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
976+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
946977

947978
const result = await runBabysitPiWithOptions(
948979
params(),
@@ -998,7 +1029,7 @@ describe('runBabysitPiWithOptions', () => {
9981029
failures: ['@missing-bot'],
9991030
})
10001031
const { runner } = makeRunner({})
1001-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
1032+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
10021033

10031034
const result = await runBabysitPiWithOptions(
10041035
params({ reviewMentions: ['@review-bot', '@missing-bot'] }),
@@ -1040,7 +1071,7 @@ describe('runBabysitPiWithOptions', () => {
10401071
],
10411072
}),
10421073
})
1043-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
1074+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
10441075

10451076
const result = await runBabysitPiWithOptions(params(), { onEvent: vi.fn() }, { roundWaitMs: 0 })
10461077

@@ -1076,7 +1107,7 @@ describe('runBabysitPiWithOptions', () => {
10761107
prepareStdout: '__NO_CHANGES__=1\n',
10771108
roundFile: JSON.stringify({ threads: [] }),
10781109
})
1079-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
1110+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
10801111

10811112
const result = await runBabysitPiWithOptions(params(), { onEvent: vi.fn() }, { roundWaitMs: 0 })
10821113

@@ -1126,7 +1157,7 @@ describe('runBabysitPiWithOptions', () => {
11261157
],
11271158
}),
11281159
})
1129-
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
1160+
mockWithPiSandbox.mockImplementation(async (_options, callback) => callback(runner))
11301161

11311162
const result = await runBabysitPiWithOptions(
11321163
params(),

apps/sim/executor/handlers/pi/babysit-backend.ts

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@ import { getMaxExecutionTimeout } from '@/lib/core/execution-limits'
1111
import { sleepUntilAborted } from '@/lib/data-drains/destinations/utils'
1212
import { isExecutionCancelled, isRedisCancellationEnabled } from '@/lib/execution/cancellation'
1313
import { type PiSandboxRunner, withPiSandbox } from '@/lib/execution/remote-sandbox'
14-
import { resolvePiSandboxLifetimeMs } from '@/lib/execution/remote-sandbox/pi-lifetime'
14+
import {
15+
resolvePiRunLifetimeMs,
16+
resolvePiSandboxLifetimeMs,
17+
} from '@/lib/execution/remote-sandbox/pi-lifetime'
1518
import {
1619
assertBabysitPinned,
1720
type BabysitCheck,
@@ -50,11 +53,11 @@ import {
5053
GIT_CONFIG_DIGEST_MARKER,
5154
MAX_DIFF_BYTES,
5255
MIN_PI_TIMEOUT_MS,
53-
PI_TIMEOUT_MS,
5456
PROMPT_PATH,
5557
PUSH_ERROR_MAX,
5658
REPO_DIR,
5759
raceAbort,
60+
resolvePiTimeoutMs,
5861
scrubGitSecrets,
5962
} from '@/executor/handlers/pi/cloud-shared'
6063
import { buildPiPrompt } from '@/executor/handlers/pi/context'
@@ -814,7 +817,19 @@ export async function runBabysitPiWithOptions(
814817
landed: false,
815818
}
816819

817-
return await withPiSandbox(async (runner) => {
820+
// Resolved here rather than at the top of the run: the GitHub reads above
821+
// already spent part of the execution's budget, and reading it at the moment
822+
// of creation is what keeps the sandbox from outliving the run by that much.
823+
//
824+
// Deliberately `context.signal`, not the `signal` every other call in this
825+
// function uses. The deadline is recorded against the signal the executor
826+
// created; `createCancellationSignal` returns a fresh controller that only
827+
// forwards aborts, so asking it for a deadline answers "unknown" and would
828+
// silently leave the longest-lived Pi mode on the provider ceiling.
829+
const lifetimeMs = resolvePiRunLifetimeMs(context.signal)
830+
const piTimeoutMs = resolvePiTimeoutMs(lifetimeMs)
831+
832+
return await withPiSandbox({ lifetimeMs }, async (runner) => {
818833
const clone = await raceAbort(
819834
runner.run(BABYSIT_CLONE_SCRIPT, {
820835
envs: {
@@ -969,7 +984,7 @@ export async function runBabysitPiWithOptions(
969984
const round = buildRoundPrompt(params, latestThreads!, promptChecks, diagnostics, secrets)
970985
progress.notes.push(...round.notes)
971986
const agentTimeoutMs = Math.min(
972-
PI_TIMEOUT_MS,
987+
piTimeoutMs,
973988
lifetime - (Date.now() - startedAt) - ROUND_FINALIZATION_RESERVE_MS
974989
)
975990
if (agentTimeoutMs < MIN_ROUND_BUDGET_MS) {

apps/sim/executor/handlers/pi/cloud-backend.test.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@ vi.mock('@/lib/execution/remote-sandbox', () => ({
2626
}))
2727
vi.mock('@/lib/execution/remote-sandbox/pi-lifetime', () => ({
2828
resolvePiSandboxLifetimeMs: () => 40 * 60 * 1000,
29+
// Same ceiling: these cases run without an execution deadline, where the run
30+
// lifetime is the ceiling because there is nothing shorter to narrow to.
31+
resolvePiRunLifetimeMs: () => 40 * 60 * 1000,
2932
}))
3033
vi.mock('@/executor/handlers/pi/babysit-backend', () => ({
3134
runBabysitPi: mockRunBabysit,
@@ -64,7 +67,7 @@ function baseParams(overrides: Partial<PiCloudRunParams> = {}): PiCloudRunParams
6467
describe('runCloudPi', () => {
6568
beforeEach(() => {
6669
vi.clearAllMocks()
67-
mockWithPiSandbox.mockImplementation((fn: (runner: unknown) => unknown) =>
70+
mockWithPiSandbox.mockImplementation((_options: unknown, fn: (runner: unknown) => unknown) =>
6871
fn({ run: mockRun, readFile: mockReadFile, writeFile: mockWriteFile })
6972
)
7073
mockProviderEnvVar.mockReturnValue('ANTHROPIC_API_KEY')

apps/sim/executor/handlers/pi/cloud-backend.ts

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,10 @@ import { generateShortId } from '@sim/utils/id'
2323
import { truncate } from '@sim/utils/string'
2424
import { getMaxExecutionTimeout, getRemainingExecutionMs } from '@/lib/core/execution-limits'
2525
import { withPiSandbox } from '@/lib/execution/remote-sandbox'
26-
import { resolvePiSandboxLifetimeMs } from '@/lib/execution/remote-sandbox/pi-lifetime'
26+
import {
27+
resolvePiRunLifetimeMs,
28+
resolvePiSandboxLifetimeMs,
29+
} from '@/lib/execution/remote-sandbox/pi-lifetime'
2730
import { runBabysitPi } from '@/executor/handlers/pi/babysit-backend'
2831
import type { PiBackendRun, PiCloudRunParams, PiRunResult } from '@/executor/handlers/pi/backend'
2932
import {
@@ -35,14 +38,14 @@ import {
3538
FINALIZE_TIMEOUT_MS,
3639
GIT_CONFIG_DIGEST_LINE,
3740
MAX_DIFF_BYTES,
38-
PI_TIMEOUT_MS,
3941
PREPARE_SCRIPT,
4042
PROMPT_PATH,
4143
PUSH_ERR_PATH,
4244
PUSH_ERROR_MAX,
4345
PUSH_SCRIPT,
4446
REPO_DIR,
4547
raceAbort,
48+
resolvePiTimeoutMs,
4649
scrubGitSecrets,
4750
} from '@/executor/handlers/pi/cloud-shared'
4851
import { buildPiPrompt } from '@/executor/handlers/pi/context'
@@ -263,7 +266,12 @@ export const runCloudPi: PiBackendRun<PiCloudRunParams> = async (params, context
263266
const totals = createPiTotals()
264267
const thinking = mapThinkingLevel(params.thinkingLevel) ?? 'medium'
265268

266-
const created = await withPiSandbox<CreatePrPhaseResult>(async (runner) => {
269+
// Resolved once and shared: the sandbox is created with this lifetime and the
270+
// agent turn reserves its finalize budget against the same number.
271+
const lifetimeMs = resolvePiRunLifetimeMs(context.signal)
272+
const piTimeoutMs = resolvePiTimeoutMs(lifetimeMs)
273+
274+
const created = await withPiSandbox<CreatePrPhaseResult>({ lifetimeMs }, async (runner) => {
267275
try {
268276
const clone = await raceAbort(
269277
runner.run(CLONE_SCRIPT, {
@@ -331,7 +339,7 @@ export const runCloudPi: PiBackendRun<PiCloudRunParams> = async (params, context
331339
}
332340
: {}),
333341
},
334-
timeoutMs: PI_TIMEOUT_MS,
342+
timeoutMs: piTimeoutMs,
335343
onStdout: handleChunk,
336344
}),
337345
context.signal

apps/sim/executor/handlers/pi/cloud-review-backend.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ const mockModelRuntime = {
6060
}
6161

6262
vi.mock('@/lib/execution/remote-sandbox', () => ({
63-
withPiSandbox: (fn: (runner: unknown) => unknown) =>
63+
withPiSandbox: (_options: unknown, fn: (runner: unknown) => unknown) =>
6464
fn({ run: mockRun, writeFile: mockWriteFile }),
6565
}))
6666
vi.mock('@/tools', () => ({ executeTool: mockExecuteTool }))

0 commit comments

Comments
 (0)