fix(api): don't kill sandboxes when snapshot requests are cancelled mid-flight#3302
fix(api): don't kill sandboxes when snapshot requests are cancelled mid-flight#3302mishushakov wants to merge 1 commit into
Conversation
…id-flight Checkpoint, snapshot-template, and remove/pause operations ran on the client-cancellable request context past their commit point, so a client disconnect (e.g. mid-fork) aborted the snapshot on the node and the sandbox was killed — for pause, without a persisted snapshot. Detach from caller cancellation once the state transition is committed so a started snapshot always runs to completion, and mark the snapshot build failed when the post-checkpoint status update fails instead of leaving it dangling in Snapshotting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit ccb6d3e. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request detaches orchestrator operations (checkpointing, snapshotting, and sandbox removal) from the caller's cancellation context once committed, ensuring they run to completion. It also adds logic to fail snapshot builds if status updates fail. The review feedback highlights a critical compilation error in the failSnapshotBuild helper method and recommends wrapping the detached contexts with safety timeouts to prevent the orchestrator from hanging indefinitely if a node or database becomes unresponsive.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // The sandbox itself resumed fine (finish(nil) restores it to Running), | ||
| // but without a success status the snapshot is unusable: mark the build | ||
| // failed so it doesn't dangle in Snapshotting forever. | ||
| o.failSnapshotBuild(ctx, upsertResult.BuildID, err) |
There was a problem hiding this comment.
The helper method failSnapshotBuild (defined in snapshot_template.go at line 143) contains a compilation error at line 146: FinishedAt: new(time.Now()). In Go, new() takes a type argument, not a value expression. This will cause the build to fail.
Please fix failSnapshotBuild in snapshot_template.go by passing a pointer to a local time.Time variable instead:
func (o *Orchestrator) failSnapshotBuild(ctx context.Context, buildID uuid.UUID, cause error) {
now := time.Now()
err := o.sqlcDB.UpdateEnvBuildStatus(ctx, queries.UpdateEnvBuildStatusParams{
Status: types.BuildStatusFailed,
FinishedAt: &now,
Reason: types.BuildReason{Message: cause.Error()},
BuildID: buildID,
})
if err != nil {
telemetry.ReportError(ctx, "error failing build", err)
}
}| // Past this point the checkpoint is committed: the sandbox will be paused | ||
| // on its node, and cancelling the checkpoint mid-snapshot kills it. Detach | ||
| // from the caller's cancellation (e.g. client disconnect mid-fork) so the | ||
| // checkpoint always runs to completion once started. | ||
| ctx = context.WithoutCancel(ctx) |
There was a problem hiding this comment.
Detaching the context from the caller's cancellation using context.WithoutCancel also strips any associated deadlines or timeouts. Without a safety timeout, the subsequent gRPC and database calls could hang indefinitely if the node or network becomes unresponsive. It is highly recommended to wrap the detached context with a reasonable safety timeout.
| // Past this point the checkpoint is committed: the sandbox will be paused | |
| // on its node, and cancelling the checkpoint mid-snapshot kills it. Detach | |
| // from the caller's cancellation (e.g. client disconnect mid-fork) so the | |
| // checkpoint always runs to completion once started. | |
| ctx = context.WithoutCancel(ctx) | |
| // Past this point the checkpoint is committed: the sandbox will be paused | |
| // on its node, and cancelling the checkpoint mid-snapshot kills it. Detach | |
| // from the caller's cancellation (e.g. client disconnect mid-fork) so the | |
| // checkpoint always runs to completion once started. | |
| // Use a safety timeout to prevent the orchestrator from hanging indefinitely if the node is unresponsive. | |
| ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Minute) | |
| defer cancel() |
| // Past this point the snapshot is committed: the sandbox will be paused | ||
| // on its node, and cancelling the checkpoint mid-snapshot kills it. Detach | ||
| // from the caller's cancellation (e.g. client disconnect) so the snapshot | ||
| // always runs to completion once started. | ||
| ctx = context.WithoutCancel(ctx) |
There was a problem hiding this comment.
Detaching the context from the caller's cancellation using context.WithoutCancel also strips any associated deadlines or timeouts. Without a safety timeout, the subsequent gRPC and database calls could hang indefinitely if the node or network becomes unresponsive. It is highly recommended to wrap the detached context with a reasonable safety timeout.
| // Past this point the snapshot is committed: the sandbox will be paused | |
| // on its node, and cancelling the checkpoint mid-snapshot kills it. Detach | |
| // from the caller's cancellation (e.g. client disconnect) so the snapshot | |
| // always runs to completion once started. | |
| ctx = context.WithoutCancel(ctx) | |
| // Past this point the snapshot is committed: the sandbox will be paused | |
| // on its node, and cancelling the checkpoint mid-snapshot kills it. Detach | |
| // from the caller's cancellation (e.g. client disconnect) so the snapshot | |
| // always runs to completion once started. | |
| // Use a safety timeout to prevent the orchestrator from hanging indefinitely if the node is unresponsive. | |
| ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Minute) | |
| defer cancel() |
| // Once we start the removal process, we want to make sure it gets removed from the store | ||
| defer o.sandboxStore.Remove(context.WithoutCancel(ctx), teamID, sandboxID) | ||
| defer o.sandboxStore.Remove(ctx, teamID, sandboxID) | ||
| err = o.removeSandboxFromNode(ctx, sbx, opts.Action, opts.Reason, opts.FilesystemOnly) |
There was a problem hiding this comment.
Since ctx has been detached from caller cancellation and has no timeout, calling removeSandboxFromNode could block indefinitely if the node is unresponsive. To prevent this, wrap the context passed to removeSandboxFromNode with a safety timeout. This keeps the other cleanup operations (like store removal and analytics) safe from early cancellation while ensuring the node call doesn't hang forever.
nodeCtx, cancel := context.WithTimeout(ctx, 3*time.Minute)
defer cancel()
err = o.removeSandboxFromNode(nodeCtx, sbx, opts.Action, opts.Reason, opts.FilesystemOnly)![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this changes cancellation semantics on the sandbox pause/kill/snapshot commit paths (removing deadlines as well as cancellation once committed), so a human should weigh in given how critical this code is to sandbox lifecycle correctness.
Extended reasoning...
Overview
The PR moves context.WithoutCancel(ctx) earlier in CheckpointSandbox, CreateSnapshotTemplate, and RemoveSandbox so that once a sandbox's state transition is committed (pause/kill/snapshot has started), the remaining work — snapshot upload, node RPCs, DB status updates, store cleanup — is detached from the caller's cancellation. It also adds a failSnapshotBuild call when the post-checkpoint UpdateEnvBuildStatus write fails, so a build doesn't dangle in Snapshotting forever.
Security risks
None identified. No new external input handling, auth, or crypto is touched. The change is purely about context propagation and error handling within already-authenticated internal orchestration calls.
Level of scrutiny
This is production-critical orchestration code governing sandbox pause/kill/snapshot lifecycle — a wrong cancellation change here can leak running sandboxes, leave builds stuck, or (in the other direction) cause requests to hang since context.WithoutCancel also strips any deadline from the parent context, not just cancellation. I verified the call sites (throttledUpsertSnapshot's semaphore acquire, node.GetClient/GetSandboxDeleteCtx) don't impose their own timeout, so any deadline carried on the original request context is now gone for the committed portion of these flows. That's consistent with the PR's stated intent (let committed operations run to completion), but it's a real behavioral trade-off — a hung downstream node call after commit no longer times out via the caller's context — that deserves a human sign-off rather than an automatic approval.
Other factors
The diff is small, well-commented, and the description accurately matches the code. The bug-hunting pass found no defects, and I additionally checked the deferred cleanup ordering in RemoveSandbox (store removal → analytics → finish) and confirmed it's unchanged from before the PR. Given the criticality of the code path, I'm deferring rather than auto-approving.
1 participant
Description
CheckpointSandbox(fork),CreateSnapshotTemplate, andRemoveSandbox(pause/kill) ran on the client-cancellable request context past their commit point. A client disconnect — e.g. a fork request dropped mid-flight — cancelled the in-flight snapshot RPC, and the node killed the sandbox; for pause this meant losing the sandbox without a persisted snapshot.This detaches each operation from caller cancellation (
context.WithoutCancel) once its state transition is committed, so a started snapshot always runs to completion and the sandbox survives a disconnect. It also marks the snapshot build failed when the post-checkpoint status update fails (previously it dangled inSnapshottingforever) and removes the now-redundant per-callWithoutCancelworkarounds.Timeouts that fire before the pause starts (queue acquire, precondition checks) still safely restore the sandbox to Running.
🤖 Generated with Claude Code