fix(api): stop retrying pause on already-killing leftovers

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Jakub Dobry <dobrac@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
GitOrigin-RevId: df365d410f5f648ce8b39d1f1b40449998bc5ea7

Author: 3 people

packages/api/internal/orchestrator/evictor/evict.go

@@ -158,12 +158,14 @@ func (e *Evictor) refreshConcurrencyLimit(ctx context.Context) {
 
 func (e *Evictor) evictSandbox(ctx context.Context, sbx sandbox.Sandbox) {
 	action := sandbox.StateActionKill
-	if sbx.AutoPause {
+	if sbx.AutoPause && canTake(sbx.State, sandbox.StateActionPause) {
 		action = sandbox.StateActionPause
 		pause.LogInitiated(ctx, sbx.SandboxID, sbx.TeamID.String(), pause.ReasonTimeout, sbx.AutoPauseFilesystemOnly)
 	}
 
-	opts := sandbox.RemoveOpts{Action: action, Eviction: true}
+	// The action was chosen from a scanned record. Pin the removal to that
+	// execution so a resume landing in between is refused, not acted on.
+	opts := sandbox.RemoveOpts{Action: action, Eviction: true, ExpectExecutionID: sbx.ExecutionID}
 	switch action {
 	case sandbox.StateActionKill:
 		opts.Reason = sandbox.KillReasonTimeout
@@ -184,12 +186,14 @@ func (e *Evictor) evictSandbox(ctx context.Context, sbx sandbox.Sandbox) {
 			switch {
 			case isNotEvictableError(err):
 				pause.LogSkipped(ctx, sbx.SandboxID, sbx.TeamID.String(), pause.ReasonTimeout, pause.SkipReasonNotEvictable, opts.FilesystemOnly)
-			case errors.Is(err, sandbox.ErrNotFound):
+			case isGone(err):
 				pause.LogSkipped(ctx, sbx.SandboxID, sbx.TeamID.String(), pause.ReasonTimeout, pause.SkipReasonNotFound, opts.FilesystemOnly)
+			case isStaleDecision(err, sbx.State):
+				pause.LogSkipped(ctx, sbx.SandboxID, sbx.TeamID.String(), pause.ReasonTimeout, pause.SkipReasonStateChanged, opts.FilesystemOnly)
 			default:
 				pause.LogFailure(ctx, sbx.SandboxID, sbx.TeamID.String(), pause.ReasonTimeout, opts.FilesystemOnly, err)
 			}
-		} else if !isKnownEvictionError(err) {
+		} else if !isKnownEvictionError(err, sbx.State) {
 			logger.L().Debug(ctx, "Evicting sandbox failed",
 				zap.Error(err),
 				logger.WithSandboxID(sbx.SandboxID),
@@ -211,10 +215,29 @@ func (e *Evictor) evictSandbox(ctx context.Context, sbx sandbox.Sandbox) {
 	}
 }
 
+func canTake(state sandbox.State, action sandbox.StateAction) bool {
+	return state == action.TargetState || sandbox.AllowedTransitions[state][action.TargetState]
+}
+
 func isNotEvictableError(err error) bool {
 	return errors.Is(err, sandbox.ErrEvictionInProgress) || errors.Is(err, sandbox.ErrEvictionNotNeeded)
 }
 
-func isKnownEvictionError(err error) bool {
-	return isNotEvictableError(err) || errors.Is(err, sandbox.ErrNotFound)
+// isGone reports the scanned sandbox is no longer there: removed, or replaced
+// by a new execution under the same ID.
+func isGone(err error) bool {
+	return errors.Is(err, sandbox.ErrNotFound) || errors.Is(err, sandbox.ErrExecutionMismatch)
+}
+
+// isStaleDecision reports a refusal explained by the sandbox moving between
+// the expired-set read and StartRemoving. A refusal from the very state the
+// action was chosen for is a real failure and stays one.
+func isStaleDecision(err error, observed sandbox.State) bool {
+	var transErr *sandbox.InvalidStateTransitionError
+
+	return errors.As(err, &transErr) && transErr.CurrentState != observed
+}
+
+func isKnownEvictionError(err error, observed sandbox.State) bool {
+	return isNotEvictableError(err) || isGone(err) || isStaleDecision(err, observed)
 }

packages/api/internal/orchestrator/evictor/evict_test.go

@@ -30,7 +30,7 @@ func TestEvictSandbox_ReasonByAction(t *testing.T) {
 	counter, err := telemetry.GetCounter(noop.NewMeterProvider().Meter("github.com/e2b-dev/infra/packages/api/internal/orchestrator/evictor"), telemetry.ApiEvictorFsOnlyAutoPause)
 	require.NoError(t, err)
 
-	runOn := func(autoPause, autoPauseFilesystemOnly bool, fcVersion string) sandbox.RemoveOpts {
+	runOn := func(state sandbox.State, autoPause, autoPauseFilesystemOnly bool, fcVersion string) sandbox.RemoveOpts {
 		var got sandbox.RemoveOpts
 		called := false
 		e := &Evictor{
@@ -52,6 +52,8 @@ func TestEvictSandbox_ReasonByAction(t *testing.T) {
 		e.evictSandbox(t.Context(), sandbox.Sandbox{
 			SandboxID:               "sbx",
 			TeamID:                  uuid.New(),
+			ExecutionID:             "exec-1",
+			State:                   state,
 			AutoPause:               autoPause,
 			AutoPauseFilesystemOnly: autoPauseFilesystemOnly,
 			FirecrackerVersion:      fcVersion,
@@ -63,7 +65,7 @@ func TestEvictSandbox_ReasonByAction(t *testing.T) {
 		return got
 	}
 	run := func(autoPause, autoPauseFilesystemOnly bool) sandbox.RemoveOpts {
-		return runOn(autoPause, autoPauseFilesystemOnly, "v1.14-0.2.0")
+		return runOn(sandbox.StateRunning, autoPause, autoPauseFilesystemOnly, "v1.14-0.2.0")
 	}
 
 	t.Run("kill carries timeout reason", func(t *testing.T) {
@@ -76,6 +78,13 @@ func TestEvictSandbox_ReasonByAction(t *testing.T) {
 		assert.Equal(t, sandbox.KillReasonTimeout, got.Reason)
 	})
 
+	t.Run("removal is pinned to the scanned execution", func(t *testing.T) {
+		t.Parallel()
+
+		assert.Equal(t, "exec-1", run(false, false).ExpectExecutionID)
+		assert.Equal(t, "exec-1", run(true, false).ExpectExecutionID)
+	})
+
 	t.Run("kill ignores the auto-pause snapshot kind", func(t *testing.T) {
 		t.Parallel()
 
@@ -120,7 +129,7 @@ func TestEvictSandbox_ReasonByAction(t *testing.T) {
 	t.Run("filesystem-only auto-pause is honored on a legacy release", func(t *testing.T) {
 		t.Parallel()
 
-		got := runOn(true, true, "v1.14.1_431f1fc")
+		got := runOn(sandbox.StateRunning, true, true, "v1.14.1_431f1fc")
 
 		assert.Equal(t, sandbox.StateActionPause, got.Action)
 		assert.True(t, got.FilesystemOnly)
@@ -129,9 +138,88 @@ func TestEvictSandbox_ReasonByAction(t *testing.T) {
 	t.Run("filesystem-only auto-pause is honored on an unparsable version", func(t *testing.T) {
 		t.Parallel()
 
-		got := runOn(true, true, "")
+		got := runOn(sandbox.StateRunning, true, true, "")
 
 		assert.Equal(t, sandbox.StateActionPause, got.Action)
 		assert.True(t, got.FilesystemOnly)
 	})
+
+	t.Run("auto-pause leftover in killing is killed not paused", func(t *testing.T) {
+		t.Parallel()
+
+		got := runOn(sandbox.StateKilling, true, true, "v1.14-0.2.0")
+
+		assert.Equal(t, sandbox.StateActionKill, got.Action)
+		assert.True(t, got.Eviction)
+		assert.Equal(t, sandbox.KillReasonTimeout, got.Reason)
+		assert.False(t, got.FilesystemOnly)
+	})
+
+	t.Run("auto-pause leftover that can still pause is paused", func(t *testing.T) {
+		t.Parallel()
+
+		for _, state := range []sandbox.State{sandbox.StatePausing, sandbox.StateSnapshotting} {
+			t.Run(string(state), func(t *testing.T) {
+				t.Parallel()
+
+				got := runOn(state, true, true, "v1.14-0.2.0")
+
+				assert.Equal(t, sandbox.StateActionPause, got.Action)
+				assert.True(t, got.FilesystemOnly)
+			})
+		}
+	})
+}
+
+func TestCanTake(t *testing.T) {
+	t.Parallel()
+
+	assert.True(t, canTake(sandbox.StateRunning, sandbox.StateActionPause))
+	assert.True(t, canTake(sandbox.StatePausing, sandbox.StateActionPause))
+	assert.True(t, canTake(sandbox.StateSnapshotting, sandbox.StateActionPause))
+	assert.False(t, canTake(sandbox.StateKilling, sandbox.StateActionPause))
+	assert.True(t, canTake(sandbox.StateKilling, sandbox.StateActionKill))
+}
+
+func TestIsStaleDecision(t *testing.T) {
+	t.Parallel()
+
+	t.Run("state moved since the scan", func(t *testing.T) {
+		t.Parallel()
+
+		err := &sandbox.InvalidStateTransitionError{
+			CurrentState: sandbox.StateKilling,
+			TargetState:  sandbox.StatePausing,
+		}
+		assert.True(t, isStaleDecision(err, sandbox.StateRunning))
+		assert.True(t, isKnownEvictionError(err, sandbox.StateRunning))
+	})
+
+	t.Run("refusal from the scanned state stays a failure", func(t *testing.T) {
+		t.Parallel()
+
+		// An unknown state is refused from the same state it was scanned in.
+		// Nothing moved; the record is broken.
+		err := &sandbox.InvalidStateTransitionError{
+			CurrentState: "",
+			TargetState:  sandbox.StateKilling,
+		}
+		assert.False(t, isStaleDecision(err, ""))
+		assert.False(t, isKnownEvictionError(err, ""))
+	})
+
+	t.Run("other errors are not stale decisions", func(t *testing.T) {
+		t.Parallel()
+
+		assert.False(t, isStaleDecision(sandbox.ErrNotFound, sandbox.StateRunning))
+	})
+}
+
+func TestIsGone(t *testing.T) {
+	t.Parallel()
+
+	assert.True(t, isGone(sandbox.ErrNotFound))
+	assert.True(t, isGone(sandbox.ErrExecutionMismatch))
+	assert.True(t, isKnownEvictionError(sandbox.ErrExecutionMismatch, sandbox.StateRunning))
+	assert.False(t, isGone(sandbox.ErrEvictionNotNeeded))
 }

packages/api/internal/pause/log.go

@@ -21,6 +21,9 @@ const (
 	SkipReasonAlreadyPaused SkipReason = "already_paused"
 	SkipReasonNotEvictable  SkipReason = "not_evictable"
 	SkipReasonNotFound      SkipReason = "not_found"
+	// SkipReasonStateChanged: the sandbox moved, between the expiry scan and
+	// the removal, into a state pause cannot start from.
+	SkipReasonStateChanged SkipReason = "state_changed"
 )
 
 // fsOnly rides on every pause event so the snapshot kind can be joined to the

packages/api/internal/sandbox/storage/redis/expiration_index_test.go

@@ -254,6 +254,27 @@ func TestExpiredItems_ReturnsExpiredRunningSandbox(t *testing.T) {
 	require.Equal(t, sbx.ExecutionID, items[0].ExecutionID)
 }
 
+func TestExpiredItems_ResidualKillingAfterStaleCutoff(t *testing.T) {
+	t.Parallel()
+
+	storage, _ := setupTestStorage(t)
+
+	teamID := uuid.New()
+	stale := makeIndexedSandbox(teamID, "sbx-stale-killing", uuid.NewString(), time.Now().Add(-time.Hour), time.Now().Add(-sandboxtypes.StaleCutoff-time.Minute))
+	stale.State = sandboxtypes.StateKilling
+	require.NoError(t, storage.Add(t.Context(), stale))
+
+	young := makeIndexedSandbox(teamID, "sbx-young-killing", uuid.NewString(), time.Now().Add(-time.Hour), time.Now().Add(-time.Minute))
+	young.State = sandboxtypes.StateKilling
+	require.NoError(t, storage.Add(t.Context(), young))
+
+	items, err := storage.ExpiredItems(t.Context())
+	require.NoError(t, err)
+	require.Len(t, items, 1)
+	require.Equal(t, stale.SandboxID, items[0].SandboxID)
+	require.Equal(t, sandboxtypes.StateKilling, items[0].State)
+}
+
 // TestHeal_RestoresMissingMember reproduces the production incident: a
 // sandbox present in team storage but missing from the global expiration
 // index is invisible to the evictor and would live forever.