fix: skip expired snapshots and fallback to fresh sandbox#412
Merged
sweetmantech merged 6 commits intotestfrom Apr 7, 2026
Merged
fix: skip expired snapshots and fallback to fresh sandbox#412sweetmantech merged 6 commits intotestfrom
sweetmantech merged 6 commits intotestfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors sandbox creation functions to introduce centralized snapshot validation via a new Changes
Sequence DiagramsequenceDiagram
participant Caller
participant createSandboxFromSnapshot
participant getValidSnapshotId
participant Database
participant VercelAPI as Vercel API
Caller->>createSandboxFromSnapshot: createSandboxFromSnapshot(accountId)
createSandboxFromSnapshot->>getValidSnapshotId: getValidSnapshotId(accountId)
getValidSnapshotId->>Database: selectAccountSnapshots(accountId)
Database-->>getValidSnapshotId: snapshots[]
Note over getValidSnapshotId: Validate snapshot<br/>expiration
getValidSnapshotId-->>createSandboxFromSnapshot: snapshotId or undefined
alt snapshotId exists
createSandboxFromSnapshot->>VercelAPI: createSandbox({source: {type: "snapshot", snapshotId}})
alt Success
VercelAPI-->>createSandboxFromSnapshot: sandbox
Note over createSandboxFromSnapshot: fromSnapshot = true
else Failure (expired/invalid)
VercelAPI-->>createSandboxFromSnapshot: Error
createSandboxFromSnapshot->>VercelAPI: createSandbox({})
VercelAPI-->>createSandboxFromSnapshot: fresh sandbox
Note over createSandboxFromSnapshot: fromSnapshot = false<br/>(fallback)
end
else No valid snapshot
createSandboxFromSnapshot->>VercelAPI: createSandbox({})
VercelAPI-->>createSandboxFromSnapshot: fresh sandbox
Note over createSandboxFromSnapshot: fromSnapshot = false
end
createSandboxFromSnapshot->>Database: Insert/update sandbox record
createSandboxFromSnapshot-->>Caller: {sandbox, fromSnapshot}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ 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 |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
sweetmantech
commented
Apr 7, 2026
Contributor
Author
sweetmantech
left a comment
sweetmantech
left a comment
There was a problem hiding this comment.
Missing unit tests for tdd test driven development.
Comment on lines
+24
to
+44
| const isExpired = snapshot?.expires_at && new Date(snapshot.expires_at) < new Date(); | ||
| const snapshotId = !isExpired ? snapshot?.snapshot_id : undefined; | ||
|
|
||
| let sandbox: Sandbox; | ||
| let fromSnapshot = false; | ||
|
|
||
| if (snapshotId) { | ||
| try { | ||
| const result = await createSandbox({ | ||
| source: { type: "snapshot", snapshotId }, | ||
| }); | ||
| sandbox = result.sandbox; | ||
| fromSnapshot = true; | ||
| } catch { | ||
| const result = await createSandbox({}); | ||
| sandbox = result.sandbox; | ||
| } | ||
| } else { | ||
| const result = await createSandbox({}); | ||
| sandbox = result.sandbox; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
SRP
- new lib for snapshotID logic
lib/sandbox/processCreateSandbox.ts
Outdated
| * @param accountId - The account to look up | ||
| * @returns The snapshot ID if it exists and has not expired | ||
| */ | ||
| async function getValidSnapshotId(accountId: string): Promise<string | undefined> { |
Contributor
Author
There was a problem hiding this comment.
SRP - new lib file for getValidSnapshotId
lib/sandbox/processCreateSandbox.ts
Outdated
Comment on lines
+46
to
+59
| if (snapshotId) { | ||
| try { | ||
| const createResult = await createSandbox({ | ||
| source: { type: "snapshot", snapshotId }, | ||
| }); | ||
| result = createResult.response; | ||
| } catch { | ||
| const freshResult = await createSandbox({}); | ||
| result = freshResult.response; | ||
| } | ||
| } else { | ||
| const freshResult = await createSandbox({}); | ||
| result = freshResult.response; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
DRY
- actual: createSandbox({}); defined twice.
- required: clean logic so createSandbox({}); is only called once.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 files
Confidence score: 3/5
- There is some merge risk because
lib/sandbox/createSandboxFromSnapshot.tsintroduces new expired-snapshot and try/catch fallback behavior without test coverage, which increases regression risk in sandbox creation flows. - The most severe issue is the untested logic around
expires_athandling and fallback behavior inlib/sandbox/createSandboxFromSnapshot.ts; if it behaves unexpectedly, users may get incorrect sandbox restore behavior. - Both
lib/sandbox/createSandboxFromSnapshot.tsandlib/sandbox/processCreateSandbox.tscurrently swallow snapshot creation exceptions without logging, which reduces production observability and can slow incident diagnosis. - Pay close attention to
lib/sandbox/createSandboxFromSnapshot.ts,lib/sandbox/processCreateSandbox.ts- missing tests and missing error logs around fallback paths can hide failures.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/sandbox/createSandboxFromSnapshot.ts">
<violation number="1" location="lib/sandbox/createSandboxFromSnapshot.ts:24">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**
Neither of the two new behaviors introduced by this bug fix is covered by tests: (1) expired-snapshot skipping (`expires_at` check) and (2) the try/catch fallback to a fresh sandbox. The test file already exists and the assertions are straightforward to add — e.g., a snapshot with a past `expires_at` should call `createSandbox({})`, and a rejecting `mockCreateSandbox` on a valid snapshot should still yield a fresh sandbox. Without these, the exact regression this PR fixes can silently reappear.</violation>
<violation number="2" location="lib/sandbox/createSandboxFromSnapshot.ts:37">
P2: The `catch` block silently swallows the snapshot creation error. Log the error before falling back so failures are observable in production.</violation>
</file>
<file name="lib/sandbox/processCreateSandbox.ts">
<violation number="1" location="lib/sandbox/processCreateSandbox.ts:52">
P2: Log the error before falling back to a fresh sandbox. Silently swallowing the exception removes any signal about *why* snapshot creation failed, making production incidents harder to diagnose.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant Service as Sandbox Domain Service
participant DB as Database
participant Provider as External Sandbox API
Note over Service,Provider: Includes processCreateSandbox & createSandboxFromSnapshot
Client->>Service: Request Sandbox Creation
Service->>DB: selectAccountSnapshots(accountId)
DB-->>Service: Snapshot list (with expires_at)
Service->>Service: CHANGED: Check expiration status
alt Valid Snapshot exists
Service->>Provider: createSandbox(snapshotId)
alt Success
Provider-->>Service: Sandbox instance
else NEW: Snapshot Restore Fails (e.g. 400 Error)
Service->>Service: Catch error
Service->>Provider: NEW: createSandbox() (Fallback to Fresh)
Provider-->>Service: Sandbox instance
end
else NEW: Snapshot expired or none found
Service->>Provider: createSandbox() (Fresh)
Provider-->>Service: Sandbox instance
end
Service->>DB: insertAccountSandbox(accountId, sandboxId)
DB-->>Service: Saved
Service-->>Client: Sandbox Response (runId, sandboxId)
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
sweetmantech
force-pushed
the
fix/snapshot-expiry-fallback
branch
from
ee25fd2 to
a451cff
Compare
sweetmantech
commented
Apr 7, 2026
lib/sandbox/processCreateSandbox.ts
Outdated
| * @param snapshotId - Optional snapshot ID to restore from | ||
| * @returns The sandbox creation response | ||
| */ | ||
| async function createSandboxWithFallback(snapshotId: string | undefined) { |
Contributor
Author
There was a problem hiding this comment.
SRP
- new lib file for createSandboxWithFallback
sweetmantech
commented
Apr 7, 2026
Comment on lines
+26
to
+40
| if (snapshotId) { | ||
| try { | ||
| sandbox = (await createSandbox({ source: { type: "snapshot", snapshotId } })).sandbox; | ||
| fromSnapshot = true; | ||
| } catch (error) { | ||
| console.error( | ||
| "Snapshot sandbox creation failed, falling back to fresh sandbox:", | ||
| error, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| if (!fromSnapshot) { | ||
| sandbox = (await createSandbox({})).sandbox; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
DRY - use the shared lib/sandbox/createSandboxWithFallback.ts
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/sandbox/createSandboxWithFallback.ts">
<violation number="1" location="lib/sandbox/createSandboxWithFallback.ts:10">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**
Bug-fix PR with no automated tests for the new fallback behavior. The two code paths (snapshot restore succeeds vs. fails-and-falls-back) are straightforward to test by mocking `createSandbox` to throw on the snapshot call. A regression test here would prevent this exact class of bug from silently reappearing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
![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
🧹 Nitpick comments (2)
lib/sandbox/processCreateSandbox.ts (1)
37-70: TrimprocessCreateSandboxback to the core flow.It still handles snapshot lookup, sandbox creation, persistence, and prompt dispatch in one 34-line function. Extracting the prompt block into a small helper would keep the primary path under the repo’s 20-line guideline and isolate the non-fatal error path for tests.
As per coding guidelines, "Flag functions longer than 20 lines" and "Keep functions small and focused".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sandbox/processCreateSandbox.ts` around lines 37 - 70, The function processCreateSandbox is too long and should delegate the optional prompt dispatch to a small helper; extract the prompt handling block (the if (prompt) try/catch that calls triggerPromptSandbox and sets runId) into a new async helper function (e.g., dispatchPromptIfPresent or triggerPromptForSandbox) that accepts { prompt, sandboxId, accountId } and returns the runId or undefined, preserve the try/catch there and log errors, then call that helper from processCreateSandbox and merge its returned runId into the result so the main function stays focused on snapshot lookup, createSandboxWithFallback, and insertAccountSandbox and reduces below the 20-line guideline.lib/sandbox/createSandboxFromSnapshot.ts (1)
27-32: Emit a warning before falling back.The fallback is the right behavior, but this catch makes repeated snapshot-restore failures invisible. A warn/metric with
accountIdandsnapshotIdwould let you spot widespread expiry or restore regressions without failing the request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sandbox/createSandboxFromSnapshot.ts` around lines 27 - 32, The catch block swallowing snapshot restore errors should emit a warning with context before falling back; modify the catch in createSandboxFromSnapshot (around the createSandbox({ source: { type: "snapshot", snapshotId } }) call) to log a warning including snapshotId and accountId (and the caught error message/stack) via the existing logger/processLogger so repeated snapshot-restore failures are visible and can be instrumented/metricized; keep the fallback behavior but ensure the warning includes clear context (e.g., "snapshot restore failed, falling back to fresh") plus snapshotId and accountId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/sandbox/getValidSnapshotId.ts`:
- Around line 14-15: In getValidSnapshotId, treat snapshots with expires_at
equal to now or with unparsable timestamps as invalid by parsing
snapshot.expires_at once into a Date (or timestamp), checking for NaN (e.g.,
isNaN(date.getTime()) or Number.isNaN(parsed)), and using a <= comparison
against new Date() (or Date.now()) instead of <; when the parsed date is NaN or
parsed <= now, return undefined so the restore path never receives expired or
invalid snapshots.
---
Nitpick comments:
In `@lib/sandbox/createSandboxFromSnapshot.ts`:
- Around line 27-32: The catch block swallowing snapshot restore errors should
emit a warning with context before falling back; modify the catch in
createSandboxFromSnapshot (around the createSandbox({ source: { type:
"snapshot", snapshotId } }) call) to log a warning including snapshotId and
accountId (and the caught error message/stack) via the existing
logger/processLogger so repeated snapshot-restore failures are visible and can
be instrumented/metricized; keep the fallback behavior but ensure the warning
includes clear context (e.g., "snapshot restore failed, falling back to fresh")
plus snapshotId and accountId.
In `@lib/sandbox/processCreateSandbox.ts`:
- Around line 37-70: The function processCreateSandbox is too long and should
delegate the optional prompt dispatch to a small helper; extract the prompt
handling block (the if (prompt) try/catch that calls triggerPromptSandbox and
sets runId) into a new async helper function (e.g., dispatchPromptIfPresent or
triggerPromptForSandbox) that accepts { prompt, sandboxId, accountId } and
returns the runId or undefined, preserve the try/catch there and log errors,
then call that helper from processCreateSandbox and merge its returned runId
into the result so the main function stays focused on snapshot lookup,
createSandboxWithFallback, and insertAccountSandbox and reduces below the
20-line guideline.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b5df283-b2a4-4da6-8678-d874ddbb93d9
⛔ Files ignored due to path filters (3)
lib/sandbox/__tests__/createSandboxFromSnapshot.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/sandbox/__tests__/createSandboxPostHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/sandbox/__tests__/getValidSnapshotId.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (3)
lib/sandbox/createSandboxFromSnapshot.tslib/sandbox/getValidSnapshotId.tslib/sandbox/processCreateSandbox.ts
Contributor
Author
Preview Deployment Test ResultsAll three test cases pass on the preview deployment:
Previously these accounts returned |
Accounts with expired Vercel snapshots caused Sandbox.create() to throw
"Status code 400 is not ok" with no recovery path.
Changes:
- Extract getValidSnapshotId to its own lib file (SRP)
- Both processCreateSandbox and createSandboxFromSnapshot use it
- Add fallback: if snapshot creation fails, create a fresh sandbox
- DRY: processCreateSandbox uses createSandboxWithFallback helper
so createSandbox({}) is only called once
- Add tests for getValidSnapshotId (5 tests)
- Add snapshot expiry and fallback tests to existing test files
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Single createSandbox({}) call path instead of duplicating it in
both the catch block and the else branch.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…r logging - Extract createSandboxWithFallback to lib/sandbox/createSandboxWithFallback.ts (SRP) - processCreateSandbox and createSandboxFromSnapshot use it / follow same pattern - Log snapshot creation errors before falling back to fresh sandbox - Add 4 unit tests for createSandboxWithFallback Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…allback - createSandboxFromSnapshot now delegates to createSandboxWithFallback instead of duplicating snapshot/fresh fallback logic - createSandboxWithFallback returns full SandboxCreateResult + fromSnapshot - Update tests to match new delegation pattern Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…apshot processCreateSandbox now delegates sandbox creation to createSandboxFromSnapshot instead of calling getValidSnapshotId, createSandboxWithFallback, and insertAccountSandbox directly. This eliminates the duplicated snapshot lookup + fallback + DB insert logic between the two functions. processCreateSandbox adds only the prompt-triggering behavior on top. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use <= instead of < so snapshots expiring exactly now are treated as expired. Add NaN check so unparsable expires_at values don't slip through as valid. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
sweetmantech
force-pushed
the
fix/snapshot-expiry-fallback
branch
from
bb1a73a to
936b048
Compare
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
expires_atbefore attempting snapshot-based sandbox creationprocessCreateSandbox(POST /api/sandboxes handler) andcreateSandboxFromSnapshot(shared domain logic)Context
Accounts with expired Vercel snapshots (sidney@recoupable.com, sidney+1@recoupable.com) caused
Sandbox.create()to throw"Status code 400 is not ok"with no recovery path, returning a 400 error to the client.Test plan
🤖 Generated with Claude Code
Summary by cubic
Skip expired Vercel snapshots and fall back to a fresh sandbox when restore fails, routing all sandbox creation through one path. Prevents 400s and keeps POST
/api/sandboxesreliable.Bug Fixes
expires_atin the past, equal to now, or invalid as expired.Refactors
getValidSnapshotIdandcreateSandboxWithFallbackwith error logging.processCreateSandbox->createSandboxFromSnapshot->createSandboxWithFallback; DB insert usessandbox.sandboxId.Written for commit 936b048. Summary will update on new commits.
Summary by CodeRabbit