Skip to content

Commit 1147eaf

Browse files
committed
fix(pi): reserve both finalize budgets in the Pi command timeout
Create PR dispatches two commands at FINALIZE_TIMEOUT_MS, not one — the commit and the push — so reserving a single budget left the push unbudgeted and the worst case overshot the sandbox lifetime by exactly that amount. Losing the sandbox during the push is the most expensive moment to lose it: the work is committed and unpushed, which is the outcome the reserve exists to prevent. The comment no longer claims more than the arithmetic delivers. What is reserved is each command's timeout ceiling rather than its measured elapsed time, so this is a budget that adds up, not a guarantee. Two other comments described Babysit verifying the config digest in the present tense, when no mode verifies it yet. Also drops the digest-line test: it asserted a string constant contains its own substrings, while cloud-backend.test.ts already pins the property that matters — the marker being the clone's last line, after the remote rewrite.
1 parent 40cb5ec commit 1147eaf

2 files changed

Lines changed: 33 additions & 29 deletions

File tree

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

Lines changed: 6 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,28 +6,19 @@ import { PI_SANDBOX_MAX_LIFETIME_MS } from '@/lib/execution/remote-sandbox/pi-li
66
import {
77
CLONE_TIMEOUT_MS,
88
FINALIZE_TIMEOUT_MS,
9-
GIT_CONFIG_DIGEST_LINE,
10-
GIT_CONFIG_DIGEST_MARKER,
119
PI_TIMEOUT_MS,
1210
} from '@/executor/handlers/pi/cloud-shared'
1311

1412
describe('PI_TIMEOUT_MS', () => {
15-
it('leaves the host room to commit and push after the agent turn ends', () => {
13+
it('reserves every command budget that brackets the agent turn', () => {
1614
// Capping at the bare sandbox lifetime would mean the sandbox always died
17-
// first, taking the agent's finished work with it unpushed.
15+
// first, taking the agent's finished work with it unpushed. Create PR runs
16+
// three bracketing commands, and the commit and the push each get the full
17+
// finalize budget — reserving only one of them leaves the push unbudgeted,
18+
// which is exactly when losing the sandbox costs the most.
1819
expect(PI_TIMEOUT_MS).toBeLessThanOrEqual(
19-
PI_SANDBOX_MAX_LIFETIME_MS - CLONE_TIMEOUT_MS - FINALIZE_TIMEOUT_MS
20+
PI_SANDBOX_MAX_LIFETIME_MS - CLONE_TIMEOUT_MS - 2 * FINALIZE_TIMEOUT_MS
2021
)
2122
expect(PI_TIMEOUT_MS).toBeGreaterThan(0)
2223
})
2324
})
24-
25-
describe('GIT_CONFIG_DIGEST_LINE', () => {
26-
it('emits the marker a host parses, over the one config scope a root agent can write', () => {
27-
expect(GIT_CONFIG_DIGEST_LINE).toContain(GIT_CONFIG_DIGEST_MARKER)
28-
expect(GIT_CONFIG_DIGEST_LINE).toContain('.git/config')
29-
// A worktree config is not always present, and its absence must not fail the clone.
30-
expect(GIT_CONFIG_DIGEST_LINE).toContain('.git/config.worktree')
31-
expect(GIT_CONFIG_DIGEST_LINE).toContain('2>/dev/null')
32-
})
33-
})

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

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,33 +19,46 @@ export const FINALIZE_TIMEOUT_MS = 10 * 60 * 1000
1919
export const MAX_DIFF_BYTES = 200_000
2020
export const PUSH_ERROR_MAX = 1000
2121

22-
/** Floor for {@link PI_TIMEOUT_MS}, so a very short configured lifetime still leaves a usable turn. */
22+
/**
23+
* Floor for {@link PI_TIMEOUT_MS}. Only reachable by configuring a sandbox
24+
* lifetime too short to hold the commands around the turn, in which case the run
25+
* was going to be reaped either way; the floor keeps the turn non-zero rather
26+
* than making it useful.
27+
*/
2328
const MIN_PI_TIMEOUT_MS = 60 * 1000
2429

2530
/**
2631
* How long one Pi CLI invocation may run. The platform's max execution timeout
27-
* is longer than the sandbox lives, so without this a hung CLI would sit there
28-
* until E2B reaped the sandbox and surface as an opaque SDK error.
32+
* outlives the sandbox, so without this a hung CLI would sit there until E2B
33+
* reaped the sandbox and surface as an opaque SDK error.
34+
*
35+
* The reserve matters as much as the cap. The sandbox clock starts at create,
36+
* and three commands bracket the agent turn: the clone before it, then the
37+
* commit and the push after it, the last two sharing
38+
* {@link FINALIZE_TIMEOUT_MS}. Capping at the bare lifetime would mean the
39+
* sandbox always died first, taking the agent's finished work with it unpushed.
2940
*
30-
* The reserve matters as much as the cap: the sandbox clock starts at create,
31-
* and the clone runs before the agent while the commit and push run after it.
32-
* Capping at the bare lifetime would mean the sandbox always died first, taking
33-
* the agent's finished work with it unpushed. Reserving both surrounding command
34-
* budgets leaves the host time to finalize whatever the agent produced.
41+
* What is reserved is each command's timeout ceiling, not its measured elapsed
42+
* time — a clone takes seconds in practice — so this is a budget that adds up,
43+
* not a guarantee that the sandbox outlives the run.
3544
*/
3645
export const PI_TIMEOUT_MS = Math.min(
3746
getMaxExecutionTimeout(),
38-
Math.max(resolvePiSandboxLifetimeMs() - CLONE_TIMEOUT_MS - FINALIZE_TIMEOUT_MS, MIN_PI_TIMEOUT_MS)
47+
Math.max(
48+
resolvePiSandboxLifetimeMs() - CLONE_TIMEOUT_MS - 2 * FINALIZE_TIMEOUT_MS,
49+
MIN_PI_TIMEOUT_MS
50+
)
3951
)
4052

4153
/**
4254
* Marker carrying a digest of the cloned repository's git config. A clone script
4355
* emits it as its *last* line, after any `git remote set-url` rewrite — a digest
4456
* taken before that rewrite mismatches at push time and every push fails.
4557
*
46-
* Every mode that clones in order to push emits it; only Babysit re-verifies it
47-
* before pushing, because verification is not a pure tightening: a run that
48-
* legitimately writes repo-local config would fail its push.
58+
* Every mode that clones in order to push emits it. No mode verifies it yet;
59+
* Babysit will, and deliberately alone, because verification is not a pure
60+
* tightening — a run that legitimately writes repo-local config would fail its
61+
* push.
4962
*/
5063
export const GIT_CONFIG_DIGEST_MARKER = '__GIT_CONFIG_DIGEST__='
5164

@@ -84,8 +97,8 @@ if git diff --quiet "$BASE_SHA" HEAD; then echo "__NO_CHANGES__=1"; else echo "_
8497
* could send the token's userinfo to another host. Repository-local config —
8598
* the scope a root agent inside the checkout can actually write — still
8699
* rewrites the push URL, and stays open until a mode compares the
87-
* {@link GIT_CONFIG_DIGEST_MARKER} digest before pushing. Babysit does; Create
88-
* PR does not, and inherits the pre-existing exposure it always had.
100+
* {@link GIT_CONFIG_DIGEST_MARKER} digest before pushing. No mode does yet;
101+
* Babysit will, and Create PR will not, keeping the exposure it always had.
89102
*
90103
* Git is invoked by absolute path so a shim planted earlier on `$PATH` is not
91104
* what runs. Both sandbox images apt-install git on Debian (see

0 commit comments

Comments
 (0)