Conversation
AdaAibaby
requested review from
ValentaTomas,
dobrac and
jakubno
as code owners
September 28, 2026 09:49
|
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' |
checkpointResumeFresh caches the snapshot successfully, then resumes a fresh replacement sandbox from it. If that replacement fails to start (e.g. "mmap memfd: cannot allocate memory" when the host hugepage pool is exhausted), the handler returned before runCheckpointUpload ran, so the already-valid snapshot was never uploaded to remote storage. The build was left marked failed with local-only bytes, and restoring it returned 404: template not found. Upload the snapshot on the resume-failure path before returning the error, so it stays restorable. This is safe because resume-fresh never defers the rootfs export and never keeps a CoW memory window (snapshotAndCacheSandbox is called with deferRootfsExport=false and maintainSandbox=false), so the snapshot bytes are fully materialized in the local cache by the time the resume is attempted. The upload is best-effort: a failure is logged and never masks the resume error. Addresses the checkpoint half of e2b-dev#3658. Signed-off-by: AdaAibaby <ada@users.noreply.github.com>
AdaAibaby
force-pushed
the
fix/checkpoint-persist-snapshot-on-resume-failure
branch
from
September 28, 2026 11:58
6dc76a3 to
0c073e3
Compare
AdaAibaby
pushed a commit
to AdaAibaby/infra
that referenced
this pull request
Sep 29, 2026
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>
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
3 participants
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 checkpoint half of #3658: on self-hosted Embed, a checkpoint whose replacement sandbox fails to start loses the snapshot.
checkpointResumeFreshsnapshots the running sandbox and caches it locally, then resumes a fresh replacement sandbox from the produced build. If that replacement fails to start — e.g.mmap memfd: cannot allocate memorywhen the host hugepage pool is exhausted (exactly the reproduction in #3658) — the handler returned beforerunCheckpointUploadran. The already-valid snapshot was never uploaded to remote storage, the build was marked failed, and restoring it returned404: template not found.Change
On the resume-failure path, upload the cached snapshot before returning the error, so it stays restorable.
This is safe because resume-fresh never defers the rootfs export and never keeps a CoW memory window —
snapshotAndCacheSandboxis called withdeferRootfsExport=falseandmaintainSandbox=false, so the snapshot bytes are fully materialized in the local cache by the time the resume is attempted. The upload is best-effort: a failure is logged and never masks the underlying resume error returned to the caller.Scope
This addresses the checkpoint scenario only. The pause scenario in #3658 (a failed rootfs-diff
fsyncduring pause) requires a separate, larger change: the API deletes the routing entry and store record before the orchestrator Pause RPC runs, so keeping the sandbox recoverable there needs an API↔orchestrator contract change (gate teardown on snapshot durability, extend the existingrestoreOnRefusalpath to snapshot failures). That will be a follow-up PR, since it touches the in-place-checkpoint lifecycle and needs KVM integration testing.Testing
pkg/serverbuilds clean with the change.ResumeSandbox), so full validation is via integration tests on a KVM host; the change is additive and only runs on an error path that today discards a valid snapshot, so it cannot regress the success path.