Repository navigation
fix(sandbox): align Sprites sandbox + terminal with the hibernate-and-wake model - #1674
Conversation
…-wake model
Agents lost their sandbox mid-conversation and terminals hung on "Connecting
to shell…" because the implementation fought the platform. Sprites hibernate
when idle and wake on demand; we were destroying them, hanging on cold file I/O,
never retrying the wake handshake, running the terminal service on an
unsupported Node, and flattening every control-plane failure into one opaque
reason. This aligns each with the SDK's documented usage.
C1 — realtime ran Node 22 but @fly/sprites requires Node >= 24: bump the
realtime image (builder + runner) to Node 24 and add an assertSandboxRuntime
guard on the terminal path so a wrong runtime fails loudly instead of hanging.
Fix the stale "Node 22 handles it natively" comment.
C2 — writeFile/readFile went through sprite.filesystem() (bare fetch, no
AbortSignal) and hung on a cold/hibernated VM. Bound every fs op with a timeout
and, on first failure, wake the VM via the exec/WebSocket path and retry once.
C3 — the conversation lifecycle destroyed hibernated VMs on a 15-min idle timer
(stop = deleteSprite), discarding agent state and churning into delete/recreate
races. Idle now RESUMES (the platform wakes the VM); teardown is reserved for
session-end and a 24h hard-expiry reclaim ceiling. (idleTimeoutMs → hardExpiryMs.)
C4 — no retry on the cold-start exec race: a hibernated VM's first exec can be
dropped ("closed before open") while it boots. Retry that pre-open wake signal
with backoff; never retry post-open failures (timeout/overflow/non-zero/mid-run).
H1 — resource caps were never passed to createSprite despite the comment. Thread
a provider-neutral SandboxResourceCaps (RAM/vCPU/storage) through the options
seam and map it to SpriteConfig in the driver.
H2 — terminal idle sessions were destroyed: pass persistent:true and treat the
planner's noop as reconnect-and-wake.
H3 — distinct provisioning failures were one opaque provision_failed. Classify
rate-limit/concurrent (→ rate_limited + retryAfter) and conflict (delete-recreate
race) in the driver, surface a distinct provision_rate_limited tool reason, and
log the classified kind.
Plan/thesis: tasks/sprites-hibernation-alignment.md. No changelog entry — the
code-execution surface is still admin-gated/default-OFF (nothing user-visible).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NBkG69PYNEYBYR6pF3TPRP
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NBkG69PYNEYBYR6pF3TPRP
|
Warning Review limit reached
More reviews will be available in 11 minutes and 8 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1d40f9b0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Sprites hibernate when idle and wake on demand, so an idle terminal VM | ||
| // must be resumed, not destroyed — `persistent` makes the planner return | ||
| // `noop` on idle (handled below as a reconnect) instead of `teardown`. | ||
| persistent: true, |
There was a problem hiding this comment.
When a terminal page has been idle past the 15-minute planner timeout, this now returns noop/resume instead of tearing the Sprite down. The realtime path then fetches the raw Sprite (apps/realtime/src/index.ts calls sdk.getSprite) and starts the PTY with a single sprite.spawn('bash', ...) in openPtyShell, so it bypasses the new runSpawnedWithWakeRetry that handles the hibernated-Sprite closed before open failure. In that idle-terminal reconnect scenario, the first wake WebSocket can close the shell instead of retrying; either add the same wake retry/warm-up to the PTY path before making terminals persistent, or keep reprovisioning idle terminals.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 000ff5f. The terminal PTY path now lands on an already-awake VM: terminal resume always relocks via client.get({ sandboxId, options }), and that relock's mkdir runs through runSpawnedWithWakeRetry — so the hibernated Sprite is woken (with the cold-start "closed before open" retry) before openPtyShell's bash spawn. A terminal acquire happens once per connection (not per keystroke), so the extra warm round-trip is negligible. See terminal-session-manager.ts reconnectExisting (relockOptions) + the driver get(options) path. Left open for your verification.
There was a problem hiding this comment.
Update (a24d036): kept this fix but via a cleaner mechanism than the earlier relock. The realtime terminal path now calls a dedicated ensureSpriteAwake(sprite) (exported from the Sprites driver) before openPtyShell, but only when the acquire resumed an existing session (fresh creates are already warmed by the acquire's mkdir). ensureSpriteAwake runs a no-op exec through runSpawnedWithWakeRetry, so the hibernated VM is woken — with the cold-start "closed before open" retry — before the PTY's bash spawn. So the resumed-terminal cold-start race you flagged is handled. See apps/realtime/src/index.ts (resume branch) + ensureSpriteAwake in sprites.ts. Left open for verification.
|
|
||
| // Within the window: resume. A hibernated VM wakes on the next exec, so we keep | ||
| // the session and its state rather than destroying a sleeping (near-free) VM. | ||
| return { action: 'resume', sandboxId: existingSession.sandboxId }; |
There was a problem hiding this comment.
For conversation sandboxes idle longer than the old 15-minute timeout but shorter than the new 24-hour hard expiry, this now returns resume; production tool execution then reconnects via buildRealSandboxRunDeps.reconnect, which calls client.get, and the Sprites get implementation only wraps sdk.getSprite without running applyEgressLockdown. In that multi-hour resume scenario, a sandbox keeps whatever network policy it had before, so tightening SANDBOX_EGRESS_ALLOWLIST or recovering an older Sprite will not be enforced until hard expiry/session end; route hibernation resumes through a path that reapplies the egress policy before handing the VM back.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 000ff5f. get now accepts optional create options; when present it reapplies applyEgressLockdown before hand-back (never destroying a warm session — it rejects if the relock fails, so no command runs without a confirmed policy). The planner relocks a resume once idle past a new 5-min warm window (warmWindowMs), so a tightened SANDBOX_EGRESS_ALLOWLIST/recovered VM is enforced within minutes of the next use instead of waiting for the 24h hard-expiry — while a burst of rapid sequential commands still reconnects cheaply (no per-command relock). This makes the code match the sprites.ts header's reapply-on-every-hand-back invariant. Left open for your verification.
There was a problem hiding this comment.
After reflection I've taken a different approach here (a24d036) and reverted the per-resume relock, on the view that reapplying egress on every resume is unnecessary complexity that drifts from the platform's configure-once model. Rationale: (1) a Sprite retains its network policy across hibernation (platform-persisted), so a resumed VM is not running unprotected — reapply would only propagate an allowlist change; (2) SANDBOX_EGRESS_ALLOWLIST is currently a static empty deny-all, so there is nothing to propagate today; (3) any future widening is bounded by the 24h hard-expiry reclaim; (4) a dropped first wake is already recovered by the per-op cold-start retry (runCommand + fs wake-retry). If/when the allowlist becomes dynamic, reapply-on-resume is the right move and easy to re-add (the driver's getOrCreate already reapplies on create) — I'd rather not carry the warm-window/relock plumbing speculatively. Happy to reinstate if you think the dynamic-allowlist case is near-term. Left open for your call.
…1/P2) Two review findings, both consequences of routing idle sessions through resume instead of teardown: P2 (lifecycle.ts) — resume reconnected via client.get, which never re-ran the egress lockdown, so a tightened SANDBOX_EGRESS_ALLOWLIST or a recovered older VM stayed on its stale policy until hard-expiry/session-end (the sprites.ts header already claimed reapply-on-every-hand-back). Fix: get() now accepts optional create options; when present it reapplies applyEgressLockdown before hand-back (never destroying a warm session; rejecting if the relock fails). The planner relocks a resume once it is idle past a 5-min warm window, so a burst of sequential commands still reconnects cheaply while a long-hibernated VM is refreshed. P1 (terminal PTY) — openPtyShell spawns bash directly, bypassing the cold-start wake retry, so a resumed (hibernated) terminal could hit "closed before open" and die. Fix: terminal resume now always relocks via get(options), and the relock's mkdir runs through runSpawnedWithWakeRetry — warming the VM (with the cold-start retry) before the PTY's bash spawn lands on it. Tests: +10 (warm-window relock planning, resume relock plumbing, driver get(options) reapply/no-reapply/relock-failure). lib+realtime typecheck, lint, 250 sandbox tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NBkG69PYNEYBYR6pF3TPRP
Why
Agents lost their sandbox mid-conversation, and terminals hung on "Connecting to shell…". Root cause: the implementation fought the Fly Sprites model. Sprites are persistent environments that hibernate when idle and wake automatically on demand — but we destroyed them on a timer, hung on cold-VM file I/O, never retried the documented wake handshake, ran the terminal service on a Node version the SDK forbids, and collapsed every control-plane failure into one opaque
provision_failed.This PR aligns each discrepancy with the SDK's documented usage. Plan/thesis:
tasks/sprites-hibernation-alignment.md.What changed (root cause → fix)
@fly/spritesrequires Node ≥ 24 → terminal PTY connect hungapps/realtime/Dockerfile(builder + runner) to Node 24; addassertSandboxRuntimeguard on the realtime terminal path; fix the misleading "Node 22 handles it natively" commentwriteFile/readFileusedsprite.filesystem()(barefetch, noAbortSignal) → hung on a cold/hibernated VMstop=deleteSprite), discarding agent state + churning delete/recreate racesidleTimeoutMs→hardExpiryMs)createSpritedespite the comment claiming soSandboxResourceCaps(RAM/vCPU/storage) through the options seam → mapped toSpriteConfigin the driverpersistent: true; treat the planner'snoopas reconnect-and-wakeprovision_failedrate_limited+retryAfter) and conflict (delete-recreate race) in the driver; surface a distinctprovision_rate_limitedtool reason; log the classified kindValidation
@pagespace/lib+realtimetypecheck (build) cleanbun run typecheck/ web build not run in the worktree (cost); the only web change is a signature-compatible one-liner insprites-client.ts, and the type owner (@pagespace/lib) compiles.Notes
stopstill destroys on genuine session-end and on the 24h hard-expiry reclaim — no orphan billing.🤖 Generated with Claude Code
Review follow-ups (Codex)
ensureSpriteAwake(sprite)beforeopenPtyShell, with the cold-start retry, so the bash spawn lands on an awake VM. (a24d036ad)a24d036ad)This net-removed ~69 lines vs the interim relock approach, keeping the implementation close to the Sprites docs' create→exec→hibernate→delete model.