Conversation
|
We require contributors to sign our Contributor License Agreement, and we don't have @ada on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check' |
AdaAibaby
requested review from
ValentaTomas,
dobrac and
jakubno
as code owners
September 29, 2026 06:42
A pause whose snapshot failed after the guest was suspended (e.g. a rootfs-diff fsync EIO, or a memfd ENOMEM under host memory/hugepage pressure) destroyed the sandbox: Server.Pause armed an unconditional deferred stop before snapshotting, so the error path tore down the already-suspended VM, and the API had already removed the routing entry and store record. The sandbox was left unrecoverable with no snapshot (e2b-dev#3658). Behind PauseRefusalRestoreFlag (off by default, so today's destroy-on-failure behaviour is unchanged), keep the sandbox recoverable: - sandbox: decouple the resume-on-error cleanup from WithMaintainSandbox via a new WithResumeOnFailure() option. It arms the same resume-in-place cleanup (health checks restarted, guest clock re-synced) that the in-place checkpoint uses, but only on failure - a successful pause still suspends and leaves the VM for the caller to stop. - Server.Pause: pass resumeOnFailure when the flag is on, and arm the deferred stop only after the snapshot succeeds. On snapshot error the guest has been resumed in place, so re-register it in the live map (MarkRunning) and return FailedPrecondition with a "sandbox preserved" marker. ErrSandboxLost (the resume itself failed) falls through to the destroy path and returns Internal unchanged. - api: classify that FailedPrecondition+marker as ErrPausePreservedSandbox (pause_instance.go) and map it in DeleteInstance to the existing restoreRefusedPause path, restoring the store record and route instead of removing them. Tests: TestSnapshotInstance_PreservedSandboxIsClassified and _PlainFailedPreconditionNotPreserved cover the API classification (a plain FailedPrecondition is not misclassified). All three packages build; the full internal/orchestrator suite passes. End-to-end validation with the flag enabled against real Firecracker (inject a snapshot fault, assert the sandbox survives and stays routable) requires a KVM host and is a follow-up. Addresses the pause half of e2b-dev#3658 (checkpoint half: e2b-dev#3666). Signed-off-by: adababys <8872412+adababys@users.noreply.github.com>
AdaAibaby
force-pushed
the
fix/pause-preserve-sandbox-on-snapshot-failure
branch
from
September 29, 2026 06:52
d483184 to
e7940da
Compare
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
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
Fixes the pause half of #3658: a failed pause could destroy the sandbox with no recoverable snapshot.
When a pause's snapshot fails after the guest has been suspended — e.g. a rootfs-diff
fsyncreturningEIO, ormmap memfd: cannot allocate memoryunder host memory/hugepage pressure —Server.Pausearmed an unconditional deferred stop before snapshotting, so the error path tore down the already-suspended VM. Combined with the API having already removed the routing entry and store record, the sandbox was left unrecoverable with no snapshot.Change (behind
PauseRefusalRestoreFlag, off by default)With the flag off, behaviour is unchanged (destroy-on-failure). With it on, a failed pause keeps the sandbox recoverable:
sandbox: decouple the resume-on-error cleanup fromWithMaintainSandboxvia a newWithResumeOnFailure()option. It arms the same resume-in-place cleanup (health checks restarted, guest clock re-synced) that the in-place checkpoint already uses — but only on failure. A successful pause still suspends and leaves the VM for the caller to stop. (The top-level cleanup only runs on the error path, so success semantics are untouched.)Server.Pause: passresumeOnFailurewhen the flag is on, and arm the deferred stop only after the snapshot succeeds. On snapshot error the guest was resumed in place, so re-register it in the live map (MarkRunning) and returnFailedPreconditionwith a stablesandbox preserved after snapshot failuremarker.ErrSandboxLost(the resume itself failed) falls through to the destroy path and returnsInternalunchanged.api: classify thatFailedPrecondition+marker asErrPausePreservedSandbox(pause_instance.go) and map it inDeleteInstanceto the existingrestoreRefusedPausepath — restoring the store record and route instead of removing them, gated by the same flag.Testing
TestSnapshotInstance_PreservedSandboxIsClassified— aFailedPrecondition+marker is classified asErrPausePreservedSandbox.TestSnapshotInstance_PlainFailedPreconditionNotPreserved— a plainFailedPrecondition(e.g. envd-version) is not misclassified.pkg/sandbox,pkg/server,api/internal/orchestrator) build; the fullinternal/orchestratorsuite passes with no regressions.Not yet done (follow-up)
End-to-end validation with the flag enabled against real Firecracker — inject a snapshot fault (fsync EIO / memfd ENOMEM), assert the sandbox survives and stays routable — has not been run. It requires a KVM host. The flag defaults off, so this is inert in production as merged; that integration run should happen before flipping the flag on.
Related: checkpoint half of #3658 is #3666.