fix(restore): return from a failed restore without waiting for its cleanup

A failed or canceled native restore cleared its ongoing marker under a
fresh one-minute deadline, while the restore itself could still hold the
object lock that the cleanup waits for. The request then stalled for
about a minute (60 s in TestNativeRestoreWithoutOuterLock and
TestRestoreRecoversStrandedMarker).

Try the cleanup for 100 ms and leave the rest to the bounded background
retries, which already exist for the pools path and now also serve the
single-set path. Each retry still takes the object lock afresh and clears
only the marker of its own attempt. The CI restore race step drops from
about 260 s to 37 s.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>

CHANGELOG.md

@@ -48,6 +48,8 @@ Native restore:
   than one pool no longer hang, stay in progress or change the restored version
   ([#289](https://github.com/pgsty/silo/issues/289)), and a failed ordinary
   multipart completion stays usable ([#327](https://github.com/pgsty/silo/issues/327)).
+  A failed or canceled restore returns at once and clears its marker in the
+  background, instead of waiting up to a minute for the object lock.
 
 Locking, conditional writes and storage:
 

cmd/restore-attempt.go

@@ -43,6 +43,11 @@ const (
 	// restoreTakeoverWait bounds how long a POST waits for the restore lock
 	// of its version. A running restore holds it for its whole run.
 	restoreTakeoverWait = 2 * time.Second
+	// restoreResetAttemptTimeout bounds the synchronous cleanup attempt made
+	// after a restore fails. A caller whose context has ended must not wait for
+	// another object operation to release the restore lock; the recovery loop
+	// below retries the cleanup after this attempt gives up.
+	restoreResetAttemptTimeout = 100 * time.Millisecond
 	// restoreCleanupTimeout bounds each status reset and convergence pass.
 	restoreCleanupTimeout = time.Minute
 )
@@ -639,6 +644,10 @@ func (z *erasureServerPools) recoverRestoreStatus(bucket, object string, opts Ob
 func (z *erasureServerPools) resetRestoreStatus(bucket, object string, opts ObjectOptions) error {
 	ctx, cancel := context.WithTimeout(GlobalContext, restoreCleanupTimeout)
 	defer cancel()
+	return z.resetRestoreStatusWithContext(ctx, bucket, object, opts)
+}
+
+func (z *erasureServerPools) resetRestoreStatusWithContext(ctx context.Context, bucket, object string, opts ObjectOptions) error {
 	lk := z.restoreLock(bucket, object)
 	lkctx, err := lk.GetLock(ctx, globalOperationTimeout)
 	if err != nil {

cmd/restore-native-isolation_test.go

@@ -455,7 +455,13 @@ func TestRestoreRecoversStrandedMarker(t *testing.T) {
 						t.Fatal(lerr)
 					}
 					ctx, cancel := context.WithTimeout(t.Context(), 100*time.Millisecond)
+					start := time.Now()
 					err = r.z.RestoreTransitionedObject(ctx, r.bucket, r.object, nativeRestoreOptions(r, oi.VersionID))
+					// The canceled caller must not wait for the minute-long
+					// background cleanup deadline before releasing its lock.
+					if elapsed := time.Since(start); elapsed > 5*time.Second {
+						t.Errorf("canceled restore blocked on marker cleanup for %s", elapsed)
+					}
 					cancel()
 					lk.Unlock(lkctx)
 				} else {

cmd/restore-native_test.go

@@ -263,7 +263,13 @@ func TestNativeRestoreWithoutOuterLock(t *testing.T) {
 			before := nativeRestoreSnapshot(t, r)
 			ctx, cancel := context.WithTimeout(t.Context(), 200*time.Millisecond)
 			defer cancel()
+			start := time.Now()
 			err = er.RestoreTransitionedObject(ctx, r.bucket, r.object, nativeRestoreOptions(r, oi.VersionID))
+			// Failure cleanup must return while this caller still holds the
+			// object lock, leaving marker recovery to the background retry.
+			if elapsed := time.Since(start); elapsed > 5*time.Second {
+				t.Errorf("restore blocked on failure cleanup for %s", elapsed)
+			}
 			if err == nil {
 				t.Error("restore wrote through another caller's lock")
 			}

cmd/restore-stage.go

@@ -793,22 +793,47 @@ func (er erasureObjects) RestoreTransitionedObject(ctx context.Context, bucket,
 		return lkctx, func() { lk.Unlock(lkctx) }, nil
 	}
 	clearMarker := func() {
-		ctx, cancel := context.WithTimeout(GlobalContext, restoreCleanupTimeout)
-		defer cancel()
-		lkctx, unlock, err := lock(ctx)
-		if err != nil {
-			storageLogIf(ctx, fmt.Errorf("Unable to clear the restore status of %s/%s(%s): %w", bucket, object, opts.VersionID, err))
+		tryClear := func(ctx context.Context) error {
+			lkctx, unlock, err := lock(ctx)
+			if err != nil {
+				return err
+			}
+			defer unlock()
+			versionID := opts.VersionID
+			if a := opts.Transition.attempt; a != nil {
+				versionID = a.VersionID
+			}
+			err = er.clearRestoreStatus(lkctx.Context(), bucket, object, versionID, opts.Transition.attempt)
+			if isErrObjectNotFound(err) || isErrVersionNotFound(err) || isErrMethodNotAllowed(err) {
+				return nil
+			}
+			return err
+		}
+
+		ctx, cancel := context.WithTimeout(GlobalContext, restoreResetAttemptTimeout)
+		err := tryClear(ctx)
+		cancel()
+		if err == nil {
 			return
 		}
-		defer unlock()
-		versionID := opts.VersionID
-		if a := opts.Transition.attempt; a != nil {
-			versionID = a.VersionID
-		}
-		if err := er.clearRestoreStatus(lkctx.Context(), bucket, object, versionID, opts.Transition.attempt); err != nil &&
-			!isErrObjectNotFound(err) && !isErrVersionNotFound(err) && !isErrMethodNotAllowed(err) {
-			storageLogIf(ctx, fmt.Errorf("Unable to clear the restore status of %s/%s(%s): %w", bucket, object, versionID, err))
-		}
+
+		go func() {
+			var lastErr error
+			for _, delay := range restoreRecoveryDelays {
+				select {
+				case <-GlobalContext.Done():
+					return
+				case <-time.After(delay):
+				}
+				ctx, cancel := context.WithTimeout(GlobalContext, restoreCleanupTimeout)
+				lastErr = tryClear(ctx)
+				cancel()
+				if lastErr == nil {
+					return
+				}
+			}
+			storageLogIf(GlobalContext, fmt.Errorf("Unable to clear the restore status of %s/%s(%s): %w", bucket, object, opts.VersionID, lastErr))
+		}()
 	}
 	return er.restoreVersion(ctx, bucket, object, opts, restoreHooks{lock: lock, clear: clearMarker})
 }
@@ -835,7 +860,10 @@ func (z *erasureServerPools) RestoreTransitionedObject(ctx context.Context, buck
 	opts.SkipDecommissioned = true
 	opts.NoLock = true
 	clearMarker := func() {
-		if err := z.resetRestoreStatus(bucket, object, opts); err != nil {
+		ctx, cancel := context.WithTimeout(GlobalContext, restoreResetAttemptTimeout)
+		err := z.resetRestoreStatusWithContext(ctx, bucket, object, opts)
+		cancel()
+		if err != nil {
 			z.recoverRestoreStatus(bucket, object, opts)
 		}
 	}