Repository navigation
fix(sandbox): terminal kill_switch_off + drop resume-time relock - #1675
Conversation
…+ per-op retry
Simplify back toward the Sprites docs' model ("configure once, hibernate, wake
on demand") after review reflection. The previous commit added a warm-window +
relock + get(options) path to reapply egress on every multi-hour resume; that is
unnecessary complexity:
- A Sprite retains its network policy across hibernation (platform-persisted), so
a resume is NOT running unprotected — reapplying only propagates allowlist
*changes*, and the allowlist is currently a static empty deny-all, so there is
nothing to propagate today.
- A dropped first wake on resume is already recovered by the per-op cold-start
retry: runCommand (runSpawnedWithWakeRetry) and writeFile/readFile
(fsWithWakeRetry). So the conversation path needs neither relock nor warm.
Reverted: warmWindowMs / relock plan flag (lifecycle), get(options) on the
client seam (types + session-manager + driver), terminal relock plumbing.
Kept (load-bearing, was Codex P1 — the original "terminal stuck connecting"
symptom on a resumed VM): the terminal PTY still needs an awake VM because
openPtyShell's bash spawn isn't wrapped in the cold-start retry. Now handled by
a dedicated exported ensureSpriteAwake(sprite), called in the realtime path only
on resume (fresh creates are warmed by the acquire's mkdir).
Net −69 lines vs the relock commit. lib+realtime typecheck, lint, 247 sandbox
tests green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NBkG69PYNEYBYR6pF3TPRP
…ess.env, not full-schema env Agent chat (web) could run code but terminals were denied with "Terminal access denied: kill_switch_off". Root cause: the terminal PTY auth runs in the realtime service, and isCodeExecutionEnabled() / resolveSpritesToken() / getSandboxSessionSecret() read their values via getValidatedEnv(), which parses process.env against the FULL web schema (DATABASE_URL, CSRF_SECRET, ENCRYPTION_KEY, …). realtime is a lean Socket.IO server that doesn't carry the whole web env, so getValidatedEnv() THROWS there; the kill-switch read catches the throw and returns false → every terminal denied even with CODE_EXECUTION_ENABLED=true. (The same throw also blanked SPRITES_API_TOKEN and SANDBOX_SESSION_SECRET on the terminal path.) The agent path works because it runs in web, where the full schema validates. Fix: read these three cross-service values directly from process.env. They are a non-secret feature flag and two individual secrets — gating them on full web-env validation is the bug. Behavior in web is unchanged; realtime now sees the flag and credentials it actually has. New regression test (env-reads.test.ts) locks the decoupling by reading the flags with the rest of the env absent. 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
|
Warning Review limit reached
More reviews will be available in 38 minutes and 48 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 (3)
📝 WalkthroughWalkthroughRemoves the relock/warm-window concept from the sandbox lifecycle planner and ChangesSandbox Lifecycle and Client Refactor
Sequence Diagram(s)sequenceDiagram
participant Client
participant makeTerminalCheckAuth
participant createSpritesSandboxClient
participant ensureSpriteAwake
participant PTYSession
Client->>makeTerminalCheckAuth: terminal auth request (sandboxId)
makeTerminalCheckAuth->>createSpritesSandboxClient: get({ sandboxId })
createSpritesSandboxClient-->>makeTerminalCheckAuth: sprite (resumed=true)
makeTerminalCheckAuth->>ensureSpriteAwake: ensureSpriteAwake(sprite)
Note over ensureSpriteAwake: exec retry loop until<br/>WebSocket open or error
ensureSpriteAwake-->>makeTerminalCheckAuth: VM warmed
makeTerminalCheckAuth->>PTYSession: open PTY, audit session, charge budget
PTYSession-->>Client: PTY session established
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: e9042ec6a4
ℹ️ 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".
| // connecting. A fresh create was just warmed by the acquire's mkdir, so only | ||
| // resumes need this extra wake. | ||
| if (sandboxResult.resumed) { | ||
| await ensureSpriteAwake(sprite); |
There was a problem hiding this comment.
When ensureSpriteAwake throws after acquireCodeExecutionSlot has succeeded (for example, a resumed hibernated Sprite exhausts the wake retry or the SDK errors), this rejection bypasses the only failure branch that calls releaseSlot() and the terminal handler never receives a releaseSlot callback to clean up. Because the concurrency semaphore is in-process, the user's slot remains held and later terminal/code execution can be denied with concurrency_limit; release the slot in a catch around this post-acquire wake path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bb8f21c. In makeTerminalCheckAuth, both sdk.getSprite(...) and ensureSpriteAwake(sprite) are now wrapped in a try/catch after the slot is acquired: on any throw it calls releaseSlot(), logs 'Terminal sandbox wake failed', and returns { ok:false, reason:'provision_failed' } — so a failed resume wake no longer leaks the in-process concurrency slot. Left open for verification.
| } catch { | ||
| return ''; | ||
| } | ||
| return process.env.SANDBOX_SESSION_SECRET ?? ''; |
There was a problem hiding this comment.
This direct read is now used specifically in realtime, where the full env schema no longer runs, so a non-empty but too-short SANDBOX_SESSION_SECRET is accepted even though the schema requires at least 32 characters. acquireConversationSandbox/acquireTerminalSandbox only check that the secret is non-empty before deriving HMAC-based sandbox names, so a misconfigured weak secret enables sandbox acquisition instead of failing closed as the previous validated read did; keep the direct read but return '' unless the value meets the documented minimum.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bb8f21c. getSandboxSessionSecret now returns '' for a secret shorter than 32 chars (return secret.length >= 32 ? secret : ''), so a too-short SANDBOX_SESSION_SECRET fail-closes exactly like an absent one rather than deriving HMAC session keys from a weak secret — restoring the web schema's >=32 guard on the realtime path. Added a regression assertion in env-reads.test.ts. Left open for verification.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/realtime/src/index.ts`:
- Around line 112-120: The ensureSpriteAwake(sprite) call in the resumed sprite
path can throw an error, which would exit the function before the execution slot
can be released back to the caller. Add error handling around the
ensureSpriteAwake(sprite) call to ensure that if it throws, the error is caught
and the slot release is still properly returned or executed before rethrowing or
handling the error, preventing the concurrency slot from being permanently
allocated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88328a87-d79c-4e7a-ac36-bbe1c3bc0134
📒 Files selected for processing (11)
apps/realtime/src/index.tspackages/lib/src/services/sandbox/__tests__/env-reads.test.tspackages/lib/src/services/sandbox/__tests__/lifecycle.test.tspackages/lib/src/services/sandbox/__tests__/session-manager.test.tspackages/lib/src/services/sandbox/can-run-code.tspackages/lib/src/services/sandbox/lifecycle.tspackages/lib/src/services/sandbox/sandbox-client/__tests__/sprites.test.tspackages/lib/src/services/sandbox/sandbox-client/sprites.tspackages/lib/src/services/sandbox/sandbox-client/types.tspackages/lib/src/services/sandbox/session-manager.tspackages/lib/src/services/sandbox/terminal-session-manager.ts
…ssion secret (review) FIX A: in makeTerminalCheckAuth, the getSprite handle lookup and ensureSpriteAwake both run after the concurrency slot is acquired and can throw (SDK error, or a resumed Sprite exhausting its wake retries). Wrap both in a try/catch that releases the slot, warns, and returns provision_failed so the in-process semaphore no longer leaks a slot on wake failure. FIX B: getSandboxSessionSecret now treats a non-empty but <32-char secret as unset (returns ''), mirroring the web schema's >=32-char guard that realtime bypasses, so acquisition fail-closes rather than deriving HMAC session keys from a weak secret. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NBkG69PYNEYBYR6pF3TPRP
1 participant
Follow-up to #1674 (which merged at the interim relock version). Two changes — plus a required deploy step for terminals to actually work.
1. Terminal
kill_switch_off— code half (load-bearing, not sufficient alone)Agent chat (web) can run code, but terminals were denied with "Terminal access denied: kill_switch_off".
Root cause: the terminal PTY auth runs in the realtime service.
isCodeExecutionEnabled()/resolveSpritesToken()/getSandboxSessionSecret()resolved their values throughgetValidatedEnv(), which validatesprocess.envagainst the full web schema. Thepagespace-realtimeFly app does not carryCSRF_SECRET/ENCRYPTION_KEY(the schema requires both whenNODE_ENV != test), sogetValidatedEnv()throws there — the kill-switch read catches it and returnsfalse, denying every terminal even if the flag were set. The same throw blanksSPRITES_API_TOKEN/SANDBOX_SESSION_SECRET. Agent chat works because it runs in web, where the full schema validates.Fix: read these three cross-service values directly from
process.env, decoupled from full web-env validation. Newenv-reads.test.tslocks it.2. Drop the resume-time egress relock (the agreed simplification)
Reverts the interim relock/warm-window/
get(options)(Codex P2): policy persists across hibernation, allowlist is static deny-all, dropped wakes are recovered by the per-op cold-start retry, future widening is bounded by the 24h reclaim. Terminal P1 wake kept viaensureSpriteAwake(warms a resumed VM before the PTY). Net −69 lines.Required deploy step (separate, in PageSpace-Deploy)
Set the three vars on the realtime Fly app (token + secret must match the web app's):
(or add
CODE_EXECUTION_ENABLED="true"tofly.realtime.toml [env]+ the two secrets.) Until this is done, terminals staykill_switch_off.Validation
@pagespace/lib+realtimetypecheck, lintenv-readsregression +ensureSpriteAwake)🤖 Generated with Claude Code