From e7940da9cf0c18d45dcb61b3429c3c85557937f7 Mon Sep 17 00:00:00 2001 From: adababys <8872412+adababys@users.noreply.github.com> Date: Tue, 29 Sep 2026 14:51:58 +0800 Subject: [PATCH] fix(pause): preserve sandbox when snapshot fails, behind a flag 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/infra#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/infra#3658 (checkpoint half: #3666). Signed-off-by: adababys <8872412+adababys@users.noreply.github.com> --- .../internal/orchestrator/delete_instance.go | 39 ++++++++++++++ packages/api/internal/orchestrator/errors.go | 5 ++ .../internal/orchestrator/pause_instance.go | 12 +++++ .../pause_instance_failure_test.go | 35 +++++++++++++ packages/orchestrator/pkg/sandbox/sandbox.go | 36 ++++++++++--- packages/orchestrator/pkg/server/sandboxes.go | 52 +++++++++++++++++-- 6 files changed, 168 insertions(+), 11 deletions(-) diff --git a/packages/api/internal/orchestrator/delete_instance.go b/packages/api/internal/orchestrator/delete_instance.go index e44537c2eb..a4505572a0 100644 --- a/packages/api/internal/orchestrator/delete_instance.go +++ b/packages/api/internal/orchestrator/delete_instance.go @@ -182,6 +182,45 @@ func (o *Orchestrator) RemoveSandbox(ctx context.Context, teamID uuid.UUID, sand return PauseQueueExhaustedError{} } + // The node's snapshot failed but it resumed the sandbox in place + // (e2b-dev/infra#3658). The VM is alive and back in the node's live map, + // so restore the store record and route — exactly like a retryable + // refusal — instead of removing them and orphaning a healthy sandbox. + // Gated by the same restoreOnRefusal flag the node used to decide to + // preserve; if it is off the node would have taken the destroy path and + // never returned this error. + if errors.Is(err, ErrPausePreservedSandbox) { + if restoreOnRefusal { + outcome := o.restoreRefusedPause(context.WithoutCancel(ctx), transition) + o.recordRefusalRestore(ctx, outcome, opts.Eviction) + switch outcome { + case restoreOutcomeRestored: + preserveRecord = true + err = sandbox.ErrTransitionRestored + case restoreOutcomeSuperseded: + preserveRecord = true + err = sandbox.ErrTransitionRestored + + return fmt.Errorf("%w: %w", ErrSandboxNotFound, sandbox.ErrExecutionMismatch) + } + } + + logger.L().Info(ctx, "Pause snapshot failed but the node preserved the sandbox", + logger.WithSandboxID(sbx.SandboxID), + zap.Bool("restored", preserveRecord), + ) + + if !preserveRecord { + // The sandbox is alive on the node but we could not restore its + // record/route, so it would be an unrouteable orphan: kill it. + o.killRefusedSandbox(ctx, sbx) + + return ErrSandboxOperationFailed + } + + return nil + } + if errors.Is(err, ErrRefusedRouteLost) { // The record is going and the route is already gone: kill the VM // now rather than leaving it to the orphan reconciler. diff --git a/packages/api/internal/orchestrator/errors.go b/packages/api/internal/orchestrator/errors.go index 5b8dcd712d..7e7634bb7f 100644 --- a/packages/api/internal/orchestrator/errors.go +++ b/packages/api/internal/orchestrator/errors.go @@ -9,4 +9,9 @@ var ( // ErrRefusedRouteLost: the node refused the pause retryably but the edge // could not put the sandbox's route back, so the sandbox cannot be kept. ErrRefusedRouteLost = errors.New("pause refused by the node and the edge could not restore its route") + // ErrPausePreservedSandbox: the node's snapshot failed but it resumed the + // sandbox in place instead of destroying it (e2b-dev/infra#3658). Handled + // like a retryable refusal: the store record and route are restored so the + // still-healthy sandbox stays usable, rather than removed. + ErrPausePreservedSandbox = errors.New("pause snapshot failed but the node preserved the sandbox") ) diff --git a/packages/api/internal/orchestrator/pause_instance.go b/packages/api/internal/orchestrator/pause_instance.go index 479d9647c5..7f9a08f625 100644 --- a/packages/api/internal/orchestrator/pause_instance.go +++ b/packages/api/internal/orchestrator/pause_instance.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "strings" "github.com/gogo/status" "github.com/google/uuid" @@ -118,6 +119,17 @@ func snapshotInstance(ctx context.Context, node *nodemanager.Node, sbx sandbox.S return ErrRefusedRouteLost } + // The node's snapshot failed but it resumed the sandbox in place instead of + // destroying it (e2b-dev/infra#3658). The orchestrator signals this with + // FailedPrecondition and a stable "sandbox preserved" marker in the message. + // Classify it so DeleteInstance restores the record + route rather than + // removing them, keeping the still-healthy sandbox usable. + if st.Code() == codes.FailedPrecondition && strings.Contains(st.Message(), "sandbox preserved after snapshot failure") { + logger.L().Warn(ctx, "Pause snapshot failed but the node preserved the sandbox", logger.WithSandboxID(sbx.SandboxID), zap.String("node_message", st.Message())) + + return ErrPausePreservedSandbox + } + return fmt.Errorf("failed to pause sandbox '%s': %w", sbx.SandboxID, err) } diff --git a/packages/api/internal/orchestrator/pause_instance_failure_test.go b/packages/api/internal/orchestrator/pause_instance_failure_test.go index a43e8026a8..b10e749834 100644 --- a/packages/api/internal/orchestrator/pause_instance_failure_test.go +++ b/packages/api/internal/orchestrator/pause_instance_failure_test.go @@ -128,3 +128,38 @@ func TestPauseSandbox_FailsBuildWhenPauseQueueExhausted(t *testing.T) { assert.Equal(t, string(types.BuildStatusFailed), buildStatus) assert.True(t, hasFinishedAt, "a terminal build must record finished_at") } + +// A FailedPrecondition carrying the "sandbox preserved after snapshot failure" +// marker (emitted by the node when WithResumeOnFailure resumed the VM instead +// of destroying it, e2b-dev/infra#3658) must be classified as +// ErrPausePreservedSandbox so DeleteInstance restores the record + route. +func TestSnapshotInstance_PreservedSandboxIsClassified(t *testing.T) { + t.Parallel() + + node := nodemanager.NewTestNode("node-preserved", api.NodeStatusReady, 0, 8) + node.SetSandboxClient(&pauseFailingSandboxClient{ + err: status.Error(codes.FailedPrecondition, "sandbox preserved after snapshot failure for 'sbx-x': fsync: input/output error"), + }) + + sbx := sandbox.Sandbox{SandboxID: "sbx-x", ClusterID: consts.LocalClusterID} + + err := snapshotInstance(t.Context(), node, sbx, "tmpl", "build", false, true) + require.ErrorIs(t, err, ErrPausePreservedSandbox) +} + +// A plain FailedPrecondition WITHOUT the preserved marker (e.g. an envd-version +// precondition) must NOT be misclassified as a preserved sandbox. +func TestSnapshotInstance_PlainFailedPreconditionNotPreserved(t *testing.T) { + t.Parallel() + + node := nodemanager.NewTestNode("node-plain-fp", api.NodeStatusReady, 0, 8) + node.SetSandboxClient(&pauseFailingSandboxClient{ + err: status.Error(codes.FailedPrecondition, "envd version too old"), + }) + + sbx := sandbox.Sandbox{SandboxID: "sbx-y", ClusterID: consts.LocalClusterID} + + err := snapshotInstance(t.Context(), node, sbx, "tmpl", "build", false, true) + require.Error(t, err) + require.NotErrorIs(t, err, ErrPausePreservedSandbox) +} diff --git a/packages/orchestrator/pkg/sandbox/sandbox.go b/packages/orchestrator/pkg/sandbox/sandbox.go index 55819035fc..8c33008efe 100644 --- a/packages/orchestrator/pkg/sandbox/sandbox.go +++ b/packages/orchestrator/pkg/sandbox/sandbox.go @@ -1915,6 +1915,7 @@ type pauseOptions struct { filesystemSnapshot bool deferRootfsExport bool maintainSandbox bool + resumeOnFailure bool } type PauseOption func(*pauseOptions) @@ -1928,6 +1929,22 @@ func WithMaintainSandbox() PauseOption { return func(o *pauseOptions) { o.maintainSandbox = true } } +// WithResumeOnFailure keeps the sandbox recoverable across a FAILED pause: if +// the snapshot fails after the guest has been suspended, the VM is resumed in +// place (health checks restarted, guest clock re-synced) instead of being left +// frozen for the caller to tear down, so a transient snapshot error (e.g. a +// rootfs-diff fsync EIO) no longer destroys an otherwise-healthy sandbox (see +// e2b-dev/infra#3658). +// +// Unlike WithMaintainSandbox, this does NOT resume on success: a successful +// pause still suspends the guest and leaves it for the caller to stop. It only +// arms the same resume-on-error cleanup that the in-place checkpoint uses, for +// the destroy path. The two compose: maintainSandbox implies resume on every +// outcome; resumeOnFailure alone resumes only on failure. +func WithResumeOnFailure() PauseOption { + return func(o *pauseOptions) { o.resumeOnFailure = true } +} + // WithFilesystemSnapshot makes the pause produce a filesystem-only snapshot: // guest memory is not snapshotted, only the filesystem (rootfs) is persisted. // Resuming such a snapshot reboots the guest instead of restoring memory state. @@ -2130,8 +2147,13 @@ func (s *Sandbox) Pause( // assignment). memExportDeferred := false var freezeStart time.Time - resumeOnError := pauseOpts.maintainSandbox - if pauseOpts.maintainSandbox { + // resumeOnError arms the resume-in-place cleanup below for BOTH the in-place + // checkpoint (maintainSandbox: resume on every outcome) and the recoverable + // destroy path (resumeOnFailure: resume only when the pause fails). The + // cleanup runs only on the error path (see the top-level deferred cleanup.Run + // guarded by e != nil), so a successful pause never resumes here regardless. + resumeOnError := pauseOpts.maintainSandbox || pauseOpts.resumeOnFailure + if resumeOnError { cleanup.Add(ctx, func(ctx context.Context) error { if !resumeOnError { return nil @@ -2186,7 +2208,7 @@ func (s *Sandbox) Pause( } freezeStart = time.Now() - if pauseOpts.maintainSandbox { + if resumeOnError { // The pause PATCH is the one state flip whose failure is AMBIGUOUS: a // request-ctx cancellation (client disconnect) can kill the round-trip // after FC already applied it. So it runs immune to request @@ -2194,6 +2216,9 @@ func (s *Sandbox) Pause( // cleanup above resumes on EVERY outcome (see the pre-arm rule at its // registration). pauseLanded — the metric/clock gate — is set only on // a successful return, the one case the guest is KNOWN to have frozen. + // Both the in-place checkpoint and the recoverable destroy path + // (resumeOnFailure) need this: each arms a resume that must be able to + // unfreeze the guest even if the caller's context died. pauseCtx, cancelPause := context.WithTimeout(context.WithoutCancel(ctx), inPlaceStateFlipTimeout) err := s.process.Pause(pauseCtx) cancelPause() @@ -2202,9 +2227,8 @@ func (s *Sandbox) Pause( } pauseLanded = true } else { - // Destroy path: no resume cleanup exists (resumeOnError is false), so - // the ambiguity above has no consumer; keep the plain request-scoped - // call. + // Plain destroy path with no resume cleanup: the ambiguity above has no + // consumer, so keep the plain request-scoped call. if err := s.process.Pause(ctx); err != nil { return nil, fmt.Errorf("failed to pause VM: %w", err) } diff --git a/packages/orchestrator/pkg/server/sandboxes.go b/packages/orchestrator/pkg/server/sandboxes.go index 57ef5387ac..ce8a0e9826 100644 --- a/packages/orchestrator/pkg/server/sandboxes.go +++ b/packages/orchestrator/pkg/server/sandboxes.go @@ -997,8 +997,15 @@ func (s *Server) Pause(ctx context.Context, in *orchestrator.SandboxPauseRequest // guest and can close the sandbox, which would read as a crash. sbx.SetStopReason(sandbox.StopReasonPaused) - // Stop the old sandbox in background after we're done - defer s.stopSandboxAsync(context.WithoutCancel(ctx), sbx) + // When enabled, a snapshot that fails AFTER the guest was suspended (e.g. a + // rootfs-diff fsync EIO or a memfd ENOMEM) resumes the VM in place instead + // of leaving it frozen for the deferred stop to tear down, so a transient + // snapshot error no longer destroys an otherwise-healthy sandbox + // (e2b-dev/infra#3658). Gated by the same flag as pause-refusal restore: + // both keep the sandbox recoverable when a pause could not be persisted, and + // the API path that restores the store record + route already keys off it. + // Off by default preserves today's destroy-on-failure behaviour. + preserveOnFailure := s.featureFlags.BoolFlag(ctx, featureflags.PauseRefusalRestoreFlag) // Defer the rootfs reflink off the pause critical path when enabled: pause is a // suspend, so nothing reads the diff until a later resume (which waits on the @@ -1006,13 +1013,43 @@ func (s *Server) Pause(ctx context.Context, in *orchestrator.SandboxPauseRequest deferRootfsExport := s.featureFlags.BoolFlag(ctx, featureflags.DeferRootfsExportFlag) // Fire and forget - upload completes in the background - res, err := s.snapshotAndCacheSandbox(ctx, sbx, in.GetBuildId(), map[string]string{storage.ObjectMetadataTemplateID: in.GetTemplateId()}, storage.ObjectOriginPause, in.GetFilesystemOnly(), deferRootfsExport, false) + res, err := s.snapshotAndCacheSandbox(ctx, sbx, in.GetBuildId(), map[string]string{storage.ObjectMetadataTemplateID: in.GetTemplateId()}, storage.ObjectOriginPause, in.GetFilesystemOnly(), deferRootfsExport, false, preserveOnFailure) if err != nil { telemetry.ReportCriticalError(ctx, "error snapshotting sandbox", err, telemetry.WithSandboxID(in.GetSandboxId())) + // With preserveOnFailure, sbx.Pause (WithResumeOnFailure) resumed the + // guest in place on any error short of ErrSandboxLost, so the VM is + // still alive. Put it back in the live map — MarkStopping removed it + // before the snapshot — and tell the API the sandbox was preserved via + // FailedPrecondition, so it restores the store record and route instead + // of removing them. ErrSandboxLost means the resume itself failed and + // Pause already tore the VM down: fall through to the destroy path. + if preserveOnFailure && !errors.Is(err, sandbox.ErrSandboxLost) { + if markErr := s.sandboxFactory.Sandboxes.MarkRunning(ctx, sbx); markErr != nil { + // Could not re-register the resumed VM; it would be an + // unrouteable orphan. Stop it and report the original error. + sbxlogger.E(sbx).Error(ctx, "failed to restore resumed sandbox to live map after snapshot failure", zap.Error(markErr)) + sbx.SetStopReason(sandbox.StopReasonKilled) + s.stopSandboxAsync(context.WithoutCancel(ctx), sbx) + + return nil, status.Errorf(codes.Internal, "error snapshotting sandbox '%s': %s", in.GetSandboxId(), err) + } + + return nil, status.Errorf(codes.FailedPrecondition, "sandbox preserved after snapshot failure for '%s': %s", in.GetSandboxId(), err) + } + + // Default (flag off) or the resume itself failed (ErrSandboxLost): the + // VM is frozen or already gone, so stop it as before. + s.stopSandboxAsync(context.WithoutCancel(ctx), sbx) + return nil, status.Errorf(codes.Internal, "error snapshotting sandbox '%s': %s", in.GetSandboxId(), err) } + // Snapshot succeeded: stop the old sandbox in background after we're done. + // Armed here rather than before the snapshot so a preserved failure above + // does not also stop the sandbox it just resumed. + defer s.stopSandboxAsync(context.WithoutCancel(ctx), sbx) + s.uploadSnapshotAsync(ctx, sbx, res) // Best-effort: the local snapshot is now in the cache and the remote upload @@ -1283,7 +1320,8 @@ func (s *Server) checkpointInPlace(ctx context.Context, sbx *sandbox.Sandbox, in storage.ObjectOriginSnapshotTemplate, false, // filesystemOnly: full-memory checkpoint (fs-only in-place is a follow-up) deferRootfsExport, - true, // maintainSandbox: resume in place + true, // maintainSandbox: resume in place + false, // resumeOnFailure: maintainSandbox already resumes on every outcome ) if err != nil { telemetry.ReportCriticalError(ctx, "error snapshotting sandbox for checkpoint", err, telemetry.WithSandboxID(in.GetSandboxId())) @@ -1368,7 +1406,7 @@ func (s *Server) checkpointResumeFresh(ctx context.Context, sbx *sandbox.Sandbox // Checkpoint resumes a fresh sandbox from the new build immediately, so the // diff must be materialized synchronously — never defer the rootfs export // here, and never maintain the paused sandbox. - res, err := s.snapshotAndCacheSandbox(ctx, sbx, in.GetBuildId(), in.GetMetadata(), storage.ObjectOriginSnapshotTemplate, false, false, false) + res, err := s.snapshotAndCacheSandbox(ctx, sbx, in.GetBuildId(), in.GetMetadata(), storage.ObjectOriginSnapshotTemplate, false, false, false, false) if err != nil { telemetry.ReportCriticalError(ctx, "error snapshotting sandbox for checkpoint", err, telemetry.WithSandboxID(in.GetSandboxId())) @@ -1552,6 +1590,7 @@ func (s *Server) snapshotAndCacheSandbox( filesystemOnly bool, deferRootfsExport bool, maintainSandbox bool, + resumeOnFailure bool, ) (*snapshotResult, error) { meta, err := sbx.Template.Metadata() if err != nil { @@ -1574,6 +1613,9 @@ func (s *Server) snapshotAndCacheSandbox( if maintainSandbox { pauseOpts = append(pauseOpts, sandbox.WithMaintainSandbox()) } + if resumeOnFailure { + pauseOpts = append(pauseOpts, sandbox.WithResumeOnFailure()) + } snapshot, err := sbx.Pause(ctx, meta, sandbox.SnapshotUseCasePause, pauseOpts...) if err != nil {