Skip to content

Commit 0a84677

Browse files
jstar0claude
andcommitted
fix: stop a clean close from disabling the retirement compensation wait
Retirement compensation decides whether to wait for the restored managed route by asking whether anything went wrong, and it asked with len(restoreErrors)==0. Appending still grows a slice when the value is nil, and two of the appends were unconditional: deletionLock.Close() and errors.Join of an open error with a resolver close. Closing cleanly therefore made the check false every time, so waitForMountAcknowledgement and the route verification behind it never ran — rollback returned the moment it had rewritten the state, before the route it had just restored was readable again. errors.Is on the returned error could not see this: the rejection is joined with the restore errors, and it matches on the rejection alone. The only test that observed it read a channel the restoring goroutine had not necessarily reached yet, so the failure was a platform-dependent coin flip rather than the certainty it actually was. Record only real failures. A unit test pins the accounting, since that is the part that silently decided the behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent d8988a7 commit 0a84677

2 files changed

Lines changed: 41 additions & 2 deletions

File tree

‎internal/cli/fs.go‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2044,14 +2044,24 @@ func newFSRollbackCommand() *cobra.Command {
20442044
}
20452045
restoreManagedRoute := func(cause error, retiredState string, retiredSnapshot string) error {
20462046
var restoreErrors []error
2047+
// Only a real failure may be recorded. `len(restoreErrors) == 0`
2048+
// is what decides whether this compensation goes on to wait for
2049+
// the restored route, and appending a nil still grows the slice:
2050+
// closing the lock cleanly used to make that check false every
2051+
// time, so the wait and the verification below never ran.
2052+
recordRestoreError := func(errs ...error) {
2053+
if joined := errors.Join(errs...); joined != nil {
2054+
restoreErrors = append(restoreErrors, joined)
2055+
}
2056+
}
20472057
deletionLock, lockErr := storage.AcquireOperationLock(store, "session-deletions")
20482058
if lockErr != nil {
20492059
return errors.Join(cause, fmt.Errorf("serialize retirement compensation with explicit deletion: %w", lockErr))
20502060
}
20512061
lockOpen := true
20522062
closeDeletionLock := func() {
20532063
if lockOpen {
2054-
restoreErrors = append(restoreErrors, deletionLock.Close())
2064+
recordRestoreError(deletionLock.Close())
20552065
lockOpen = false
20562066
}
20572067
}
@@ -2105,7 +2115,7 @@ func newFSRollbackCommand() *cobra.Command {
21052115
} else {
21062116
restored = restoredManaged.State()
21072117
restoredVisible, openErr = hashManagedSessionVisible(command.Context(), restoredManaged)
2108-
restoreErrors = append(restoreErrors, errors.Join(openErr, restoredResolver.Close()))
2118+
recordRestoreError(openErr, restoredResolver.Close())
21092119
if err := upsertManagedSessionRegistryEntryIfPresent(store, restored.SessionID, restored.Generation); err != nil {
21102120
restoreErrors = append(restoreErrors, err)
21112121
}

‎internal/cli/fs_test.go‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5241,3 +5241,32 @@ func TestEnrollmentPlanSizesAnAutomaticBatchFromMeasuredCost(t *testing.T) {
52415241
t.Fatalf("manual batch selected %d sessions, want exactly 3", len(manual.Selected))
52425242
}
52435243
}
5244+
5245+
func TestRestoreErrorAccountingIgnoresCleanCloses(t *testing.T) {
5246+
// This mirrors how the retirement compensation accumulates failures. It
5247+
// decides whether to wait for the restored route by asking whether anything
5248+
// went wrong, and a nil appended by a clean close answered that question
5249+
// wrongly: the wait and the verification behind it became unreachable.
5250+
var restoreErrors []error
5251+
record := func(errs ...error) {
5252+
if joined := errors.Join(errs...); joined != nil {
5253+
restoreErrors = append(restoreErrors, joined)
5254+
}
5255+
}
5256+
5257+
var cleanClose error
5258+
record(cleanClose)
5259+
record(nil, nil)
5260+
if len(restoreErrors) != 0 {
5261+
t.Fatalf("a clean close counted as a failure: %v", restoreErrors)
5262+
}
5263+
5264+
record(errors.New("boom"))
5265+
if len(restoreErrors) != 1 {
5266+
t.Fatalf("a real failure was not recorded: %v", restoreErrors)
5267+
}
5268+
record(nil, errors.New("second"))
5269+
if len(restoreErrors) != 2 {
5270+
t.Fatalf("a failure joined with a nil was not recorded: %v", restoreErrors)
5271+
}
5272+
}

0 commit comments

Comments
 (0)