backup: fix AdminUnsplit race in online restore download cleanup

pkg/backup/restore_online.go

@@ -816,18 +816,23 @@ func (r *restoreResumer) waitForDownloadToComplete(
 	return ctx.Err()
 }
 
-func unstickRestoreSpans(
-	ctx context.Context, execCfg *sql.ExecutorConfig, spans roachpb.Spans,
-) error {
+func unstickRestoreSpans(ctx context.Context, execCfg *sql.ExecutorConfig, spans roachpb.Spans) {
 	for _, sp := range spans {
-		if err := execCfg.DB.AdminUnsplit(ctx, sp.Key); err != nil {
-			return errors.Wrapf(err, "failed to unsplit %s", sp)
-		}
-		if err := execCfg.DB.AdminUnsplit(ctx, sp.EndKey); err != nil {
-			return errors.Wrapf(err, "failed to unsplit %s", sp.EndKey)
-		}
+		besteffort.Warning(ctx, "or-unsplit", func(ctx context.Context) error {
+			if err := execCfg.DB.AdminUnsplit(ctx, sp.Key); err != nil &&
+				!kvpb.IsKeyNotStartOfRange(err) {
+				return errors.Wrapf(err, "failed to unsplit %s", sp)
+			}
+			return nil
+		})
+		besteffort.Warning(ctx, "or-unsplit", func(ctx context.Context) error {
+			if err := execCfg.DB.AdminUnsplit(ctx, sp.EndKey); err != nil &&
+				!kvpb.IsKeyNotStartOfRange(err) {
+				return errors.Wrapf(err, "failed to unsplit %s", sp.EndKey)
+			}
+			return nil
+		})
 	}
-	return nil
 }
 
 func getExternalBytesOverSpans(
@@ -985,7 +990,8 @@ func (r *restoreResumer) cleanupAfterDownload(
 			log.Dev.Warningf(ctx, "failed to re-enable auto stats on table %d", id)
 		}
 	}
-	return unstickRestoreSpans(ctx, r.execCfg, details.DownloadSpans)
+	unstickRestoreSpans(ctx, r.execCfg, details.DownloadSpans)
+	return nil
 }
 
 func createImportRollbackJob(
@@ -1115,7 +1121,8 @@ func (r *restoreResumer) maybeCleanupFailedOnlineRestore(
 		return errors.Wrapf(err, "failed to get external data after excise")
 	}
 
-	return unstickRestoreSpans(ctx, p.ExecCfg(), details.DownloadSpans)
+	unstickRestoreSpans(ctx, p.ExecCfg(), details.DownloadSpans)
+	return nil
 }
 
 // getNumOnlineRestoreLinkWorkers returns the total number of workers to use for

pkg/backup/utils_test.go

@@ -612,4 +612,5 @@ func getFullBackupPaths(t *testing.T, sqlDB *sqlutils.SQLRunner, uri string) []s
 func AllowORDownloadBestEffortFailures(t *testing.T) {
 	t.Helper()
 	t.Cleanup(besteffort.TestAllowFailure("or-download-failed-log"))
+	t.Cleanup(besteffort.TestAllowFailure("or-unsplit"))
 }

pkg/kv/kvpb/errors.go

@@ -11,6 +11,7 @@ import (
 	"fmt"
 	"reflect"
 	"slices"
+	"strings"
 
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
@@ -29,6 +30,24 @@ import (
 	"github.com/gogo/protobuf/proto"
 )
 
+// ErrKeyNotStartOfRange is returned by AdminUnsplit when the target key is not
+// the start key of any range, typically because the range was merged
+// concurrently.
+//
+// NB: don't change the string here; this will cause cross-version issues since
+// this singleton is used as a marker.
+var ErrKeyNotStartOfRange = errors.New("key is not the start of a range")
+
+// IsKeyNotStartOfRange returns true if the error indicates that an AdminUnsplit
+// request targeted a key that is not the start of a range. It falls back to
+// string matching for compatibility with older nodes that produce the untyped
+// error.
+func IsKeyNotStartOfRange(err error) bool {
+	// TODO(msbutler): remove string matching fallback in 27.1.
+	return errors.Is(err, ErrKeyNotStartOfRange) ||
+		strings.Contains(err.Error(), "is not the start of a range")
+}
+
 // Printer is an interface that lets us use what's common between the
 // errors.Printer interface and redact.SafePrinter so we can write functions
 // that both SafeFormatError and SafeFormat can share.

pkg/kv/kvserver/replica_command.go

@@ -591,7 +591,10 @@ func (r *Replica) adminUnsplitWithDescriptor(
 ) (kvpb.AdminUnsplitResponse, error) {
 	var reply kvpb.AdminUnsplitResponse
 	if !bytes.Equal(desc.StartKey.AsRawKey(), args.Header().Key) {
-		return reply, errors.Errorf("key %s is not the start of a range", args.Header().Key)
+		return reply, errors.Mark(
+			errors.Newf("key %s is not the start of a range", args.Header().Key),
+			kvpb.ErrKeyNotStartOfRange,
+		)
 	}
 
 	// If the range's sticky bit is already hlc.Timestamp{}, we treat the unsplit

pkg/sql/gcjob/gc_job.go

@@ -7,7 +7,6 @@ package gcjob
 
 import (
 	"context"
-	"strings"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/jobs"
@@ -189,7 +188,7 @@ func unsplitRangesInSpan(
 			// that the sticky bit was removed and merged concurrently. DROP TABLE
 			// should not fail because of this.
 			if err := kvDB.AdminUnsplit(ctx, desc.StartKey); err != nil &&
-				!strings.Contains(err.Error(), "is not the start of a range") {
+				!kvpb.IsKeyNotStartOfRange(err) {
 				return err
 			}
 		}
@@ -222,7 +221,7 @@ func unsplitRangesInSpanForSecondaryTenant(
 			// Swallow "key is not the start of a range" errors because it would mean
 			// that the sticky bit was removed and merged concurrently. DROP TABLE
 			// should not fail because of this.
-			if strings.Contains(err.Error(), "is not the start of a range") {
+			if kvpb.IsKeyNotStartOfRange(err) {
 				continue
 			}
 			// If we are in a secondary tenant and get an auth