fix(deployment): journal a failed stop when pending retirements cannot be loaded

An app-level cleanup failure belongs to no service, so Stop journaled a success from the per-service results and a replay of the same key returned it. It now records a failed pending.retire step, a failed outcome, and the warning, so the replay returns the same failure.

commit: 633b9ae71f2d4065b9f98556e86bc1c5633e6336
author: brice · 6d ago · 2026-10-16 19:59 UTC
parent: a2e0a06
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +70 -7

M internal/usecase/deployment/lifecycle.go +20 -7


@@ -89,9 +89,19 @@ func (s Service) stopLocked(ctx context.Context, app, opID string) (LifecycleR
89	89		// A superseded predecessor an overlap replacement left running is not in
90	90		// ACTIVE, so the loop above never saw it: a stopped app must not keep it
91	91		// running. One that cannot be retired keeps its marker and fails the stop.
92		-	failures = append(failures, s.retirePendingForStop(ctx, app, result)...)
	92	+	pendingFailed, pendingErr := s.retirePendingForStop(ctx, app, result)
	93	+	failures = append(failures, pendingFailed...)
93	94		op.Outcome = ComputeOutcome(result.Services)
94		-	op.Warnings = journalWarnings(collectCleanupWarnings(result.Services))
	95	+	if pendingErr != nil {
	96	+	// An app-level cleanup failure belongs to no service, so the
	97	+	// per-service results alone would journal a success and a replay of
	98	+	// this key would read it. It is a failed step and a failed outcome.
	99	+	op.Steps = append(op.Steps, domain.AppOperationStep{
	100	+	ID: "pending.retire", State: domain.AppStepFailed, Error: pendingErr.Error(),
	101	+	})
	102	+	op.Outcome = domain.AppOutcomeFailed
	103	+	}
	104	+	op.Warnings = journalWarnings(append(collectCleanupWarnings(result.Services), result.Warnings...))
95	105		if err := s.deps.State.SaveOperation(ctx, op); err != nil {
96	106		log.Warn().Err(err).Msg("deployment: failed to record stop outcome")
97	107		}

@@ -558,12 +568,15 @@ func (s *Service) pendingRetirements(ctx context.Context, app string) ([]domain.
558	568
559	569		// retirePendingForStop retires the pending superseded containers of a stopped
560	570		// app, recording a leftover or a failure on the service they belong to. It
561		-	// returns the services whose predecessor is not confirmed retired.
562		-	func (s Service) retirePendingForStop(ctx context.Context, app string, result LifecycleResult) []string {
	571	+	// returns the services whose predecessor is not confirmed retired, and an
	572	+	// app-level error when the markers themselves could not be loaded: nothing
	573	+	// can then be said about any predecessor, which the caller must journal as a
	574	+	// failed operation.
	575	+	func (s Service) retirePendingForStop(ctx context.Context, app string, result LifecycleResult) ([]string, error) {
563	576		pending, err := s.pendingRetirements(ctx, app)
564	577		if err != nil {
565		-	result.Warnings = append(result.Warnings, CleanupWarning{Detail: err.Error()})
566		-	return []string{"pending retirements"}
	578	+	result.Warnings = append(result.Warnings, CleanupWarning{Detail: "pending retirements unknown: " + err.Error()})
	579	+	return []string{"pending retirements"}, err
567	580		}
568	581		var failed []string
569	582		for _, marker := range pending {

@@ -579,7 +592,7 @@ func (s Service) retirePendingForStop(ctx context.Context, app string, result
579	592		}
580	593		result.Services[marker.Service] = svc
581	594		}
582		-	return failed
	595	+	return failed, nil
583	596		}
584	597
585	598		// ReconcileBoot reconciles apps intended to run after a daemon boot:

 	// A superseded predecessor an overlap replacement left running is not in
 	// ACTIVE, so the loop above never saw it: a stopped app must not keep it
 	// running. One that cannot be retired keeps its marker and fails the stop.
-	failures = append(failures, s.retirePendingForStop(ctx, app, result)...)
+	pendingFailed, pendingErr := s.retirePendingForStop(ctx, app, result)
+	failures = append(failures, pendingFailed...)
 	op.Outcome = ComputeOutcome(result.Services)
-	op.Warnings = journalWarnings(collectCleanupWarnings(result.Services))
+	if pendingErr != nil {
+		// An app-level cleanup failure belongs to no service, so the
+		// per-service results alone would journal a success and a replay of
+		// this key would read it. It is a failed step and a failed outcome.
+		op.Steps = append(op.Steps, domain.AppOperationStep{
+			ID: "pending.retire", State: domain.AppStepFailed, Error: pendingErr.Error(),
+		})
+		op.Outcome = domain.AppOutcomeFailed
+	}
+	op.Warnings = journalWarnings(append(collectCleanupWarnings(result.Services), result.Warnings...))
 	if err := s.deps.State.SaveOperation(ctx, op); err != nil {
 		log.Warn().Err(err).Msg("deployment: failed to record stop outcome")
 	}
 // retirePendingForStop retires the pending superseded containers of a stopped
 // app, recording a leftover or a failure on the service they belong to. It
-// returns the services whose predecessor is not confirmed retired.
+// returns the services whose predecessor is not confirmed retired, and an
-func (s *Service) retirePendingForStop(ctx context.Context, app string, result *LifecycleResult) []string {
+// app-level error when the markers themselves could not be loaded: nothing
+// can then be said about any predecessor, which the caller must journal as a
+// failed operation.
+func (s *Service) retirePendingForStop(ctx context.Context, app string, result *LifecycleResult) ([]string, error) {
 	pending, err := s.pendingRetirements(ctx, app)
 	if err != nil {
-		result.Warnings = append(result.Warnings, CleanupWarning{Detail: err.Error()})
+		result.Warnings = append(result.Warnings, CleanupWarning{Detail: "pending retirements unknown: " + err.Error()})
-		return []string{"pending retirements"}
+		return []string{"pending retirements"}, err
 	}
 	var failed []string
 	for _, marker := range pending {
 		}
 		result.Services[marker.Service] = svc
 	}
-	return failed
+	return failed, nil
 }
 // ReconcileBoot reconciles apps intended to run after a daemon boot:

M internal/usecase/deployment/overlap_replacement_test.go +50 -0


@@ -793,3 +793,53 @@ func TestRemove_FailsAndKeepsStateWhenAPendingPredecessorCannotBeRetired(t *test
793	793		require.NoError(t, loadErr)
794	794		assert.True(t, ok, "the incarnation is not ended")
795	795		}
	796	+
	797	+	// inhibitionsFailingState wraps the real store and fails the load of the
	798	+	// recovery inhibitions, which is how Stop learns about pending predecessors.
	799	+	type inhibitionsFailingState struct {
	800	+	*appstate.Store
	801	+	}
	802	+
	803	+	func (s *inhibitionsFailingState) LoadRecoveryInhibitions(context.Context, string) ([]domain.AppRecoveryInhibition, error) {
	804	+	return nil, assert.AnError
	805	+	}
	806	+
	807	+	// TestStop_PendingRetirementLoadFailureIsAJournaledFailure proves an app-level
	808	+	// cleanup failure (the pending predecessors cannot even be listed) is not lost
	809	+	// in the per-service results: the operation is journaled as failed, so Stop
	810	+	// errors and a replay of the same key reads the same failure instead of a
	811	+	// success.
	812	+	func TestStop_PendingRetirementLoadFailureIsAJournaledFailure(t *testing.T) {
	813	+	ctx := context.Background()
	814	+	store := newTestStore(t)
	815	+	runtime := outmocks.NewMockContainerRuntime(t)
	816	+	seedPendingRetirement(t, ctx, store, "c-old")
	817	+	// The ACTIVE container is retired successfully before the load fails.
	818	+	runtime.EXPECT().StopContainer(mock.Anything, "c-new", mock.Anything).Return(nil).Once()
	819	+	runtime.EXPECT().RemoveContainer(mock.Anything, "c-new", false).Return(nil).Once()
	820	+	svc := deployment.NewService(deployment.Deps{
	821	+	State: &inhibitionsFailingState{Store: store}, Runtime: runtime, Traffic: &storeTraffic{store: store},
	822	+	}, zerowrap.Default())
	823	+
	824	+	result, err := svc.Stop(ctx, "blog", "key-stop")
	825	+	require.Error(t, err)
	826	+	assert.Equal(t, "deployed", result.Services["web"].Result, "the ACTIVE container really was stopped")
	827	+	require.NotEmpty(t, result.Warnings, "the app-level failure is surfaced on the result")
	828	+
	829	+	op, loadErr := store.LoadOperation(ctx, "blog", "key-stop")
	830	+	require.NoError(t, loadErr)
	831	+	assert.Equal(t, domain.AppOutcomeFailed, op.Outcome, "the journal records the failure, not the per-service success")
	832	+	step, found := opStep(op, "pending.retire")
	833	+	require.True(t, found)
	834	+	assert.Equal(t, domain.AppStepFailed, step.State)
	835	+	assert.NotEmpty(t, step.Error)
	836	+	require.NotEmpty(t, op.Warnings)
	837	+	assert.Contains(t, op.Warnings[0].Detail, "pending retirements unknown")
	838	+
	839	+	// Replaying the same key returns the same failure and runs nothing again.
	840	+	_, replayErr := svc.Stop(ctx, "blog", "key-stop")
	841	+	require.Error(t, replayErr)
	842	+	assert.ErrorIs(t, replayErr, domain.ErrAppStateConflict)
	843	+	assert.Contains(t, replayErr.Error(), domain.AppOutcomeFailed)
	844	+	runtime.AssertNumberOfCalls(t, "StopContainer", 1)
	845	+	}


@@ -793,3 +793,53 @@ func TestRemove_FailsAndKeepsStateWhenAPendingPredecessorCannotBeRetired(t *test
793	require.NoError(t, loadErr)	793		require.NoError(t, loadErr)
794	assert.True(t, ok, "the incarnation is not ended")	794		assert.True(t, ok, "the incarnation is not ended")
795	}	795		}
		796	+
		797	+	// inhibitionsFailingState wraps the real store and fails the load of the
		798	+	// recovery inhibitions, which is how Stop learns about pending predecessors.
		799	+	type inhibitionsFailingState struct {
		800	+	*appstate.Store
		801	+	}
		802	+
		803	+	func (s *inhibitionsFailingState) LoadRecoveryInhibitions(context.Context, string) ([]domain.AppRecoveryInhibition, error) {
		804	+	return nil, assert.AnError
		805	+	}
		806	+
		807	+	// TestStop_PendingRetirementLoadFailureIsAJournaledFailure proves an app-level
		808	+	// cleanup failure (the pending predecessors cannot even be listed) is not lost
		809	+	// in the per-service results: the operation is journaled as failed, so Stop
		810	+	// errors and a replay of the same key reads the same failure instead of a
		811	+	// success.
		812	+	func TestStop_PendingRetirementLoadFailureIsAJournaledFailure(t *testing.T) {
		813	+	ctx := context.Background()
		814	+	store := newTestStore(t)
		815	+	runtime := outmocks.NewMockContainerRuntime(t)
		816	+	seedPendingRetirement(t, ctx, store, "c-old")
		817	+	// The ACTIVE container is retired successfully before the load fails.
		818	+	runtime.EXPECT().StopContainer(mock.Anything, "c-new", mock.Anything).Return(nil).Once()
		819	+	runtime.EXPECT().RemoveContainer(mock.Anything, "c-new", false).Return(nil).Once()
		820	+	svc := deployment.NewService(deployment.Deps{
		821	+	State: &inhibitionsFailingState{Store: store}, Runtime: runtime, Traffic: &storeTraffic{store: store},
		822	+	}, zerowrap.Default())
		823	+
		824	+	result, err := svc.Stop(ctx, "blog", "key-stop")
		825	+	require.Error(t, err)
		826	+	assert.Equal(t, "deployed", result.Services["web"].Result, "the ACTIVE container really was stopped")
		827	+	require.NotEmpty(t, result.Warnings, "the app-level failure is surfaced on the result")
		828	+
		829	+	op, loadErr := store.LoadOperation(ctx, "blog", "key-stop")
		830	+	require.NoError(t, loadErr)
		831	+	assert.Equal(t, domain.AppOutcomeFailed, op.Outcome, "the journal records the failure, not the per-service success")
		832	+	step, found := opStep(op, "pending.retire")
		833	+	require.True(t, found)
		834	+	assert.Equal(t, domain.AppStepFailed, step.State)
		835	+	assert.NotEmpty(t, step.Error)
		836	+	require.NotEmpty(t, op.Warnings)
		837	+	assert.Contains(t, op.Warnings[0].Detail, "pending retirements unknown")
		838	+
		839	+	// Replaying the same key returns the same failure and runs nothing again.
		840	+	_, replayErr := svc.Stop(ctx, "blog", "key-stop")
		841	+	require.Error(t, replayErr)
		842	+	assert.ErrorIs(t, replayErr, domain.ErrAppStateConflict)
		843	+	assert.Contains(t, replayErr.Error(), domain.AppOutcomeFailed)
		844	+	runtime.AssertNumberOfCalls(t, "StopContainer", 1)
		845	+	}