backup: clean up BACKUP-LOCK on compaction failure

pkg/backup/backup_job.go

@@ -51,6 +51,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/stats"
+	"github.com/cockroachdb/cockroach/pkg/util/besteffort"
 	bulkutil "github.com/cockroachdb/cockroach/pkg/util/bulk"
 	"github.com/cockroachdb/cockroach/pkg/util/ctxgroup"
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
@@ -1960,6 +1961,20 @@ func (b *backupResumer) OnFailOrCancel(
 	details := b.job.Details().(jobspb.BackupDetails)
 
 	b.deleteCheckpoint(ctx, cfg, p.User())
+
+	// For compaction jobs, clean up the BACKUP-LOCK file from the backup
+	// destination to unblock subsequent compaction attempts that may target
+	// the same location. A failed compaction does not produce a valid backup at
+	// the destination, so the lock serves no purpose and only blocks future
+	// compactions.
+	if details.Compact && details.URI != "" {
+		besteffort.Warning(ctx, "delete-compaction-backup-lock", func(ctx context.Context) error {
+			return backupinfo.DeleteBackupLock(
+				ctx, cfg, details.URI, b.job.ID(), p.User(),
+			)
+		})
+	}
+
 	if err := cfg.InternalDB.Txn(ctx, func(ctx context.Context, txn isql.Txn) error {
 		pts := cfg.ProtectedTimestampProvider.WithTxn(txn)
 		return releaseProtectedTimestamp(ctx, pts, details.ProtectedTimestampRecord)

pkg/backup/backupinfo/manifest_handling.go

@@ -541,6 +541,37 @@ func WriteBackupLock(
 	return cloud.WriteFile(ctx, defaultStore, lockFileName, bytes.NewReader([]byte("lock")))
 }
 
+// DeleteBackupLock removes the backup lock file for the given jobID from the
+// default backup destination. This is used to clean up lock files from failed
+// compaction jobs so that subsequent compaction attempts to the same destination
+// are not blocked.
+func DeleteBackupLock(
+	ctx context.Context,
+	execCfg *sql.ExecutorConfig,
+	defaultURI string,
+	jobID jobspb.JobID,
+	user username.SQLUsername,
+) error {
+	ctx, sp := tracing.ChildSpan(ctx, "backupinfo.DeleteBackupLock")
+	defer sp.Finish()
+
+	defaultStore, err := execCfg.DistSQLSrv.ExternalStorageFromURI(ctx, defaultURI, user)
+	if err != nil {
+		return err
+	}
+	defer defaultStore.Close()
+
+	lockFileName := fmt.Sprintf("%s%s", BackupLockFilePrefix, strconv.FormatInt(int64(jobID), 10))
+	if err := defaultStore.Delete(ctx, lockFileName); err != nil {
+		// If the lock file does not exist, there is nothing to clean up.
+		if errors.Is(err, cloud.ErrFileDoesNotExist) {
+			return nil
+		}
+		return err
+	}
+	return nil
+}
+
 // WriteMetadataWithExternalSSTs writes a "slim" version of manifest to
 // `exportStore`. This version has the alloc heavy `Files`, `Descriptors`, and
 // `DescriptorChanges` repeated fields nil'ed out, and written to an

pkg/backup/testdata/backup-restore/compaction-failed-lock-cleanup

@@ -0,0 +1,76 @@
+# Test that when a compaction job is cancelled/fails, its BACKUP-LOCK file is
+# cleaned up, allowing subsequent compactions to proceed without being blocked.
+# See: https://github.com/cockroachdb/cockroach/issues/172889
+
+reset test-nodelocal
+----
+
+new-cluster name=s1 disable-tenant
+----
+
+# 1. Setup: create a table and take a full backup followed by two incrementals,
+# saving timestamps to use as compaction start/end boundaries.
+exec-sql
+CREATE DATABASE orig;
+USE orig;
+CREATE TABLE foo (i INT PRIMARY KEY, s STRING);
+INSERT INTO foo VALUES (1, 'a'), (2, 'b');
+----
+
+save-cluster-ts tag=start
+----
+
+backup aost=start
+BACKUP INTO 'nodelocal://1/test-root/' AS OF SYSTEM TIME start;
+----
+
+exec-sql
+INSERT INTO orig.foo VALUES (3, 'c');
+----
+
+backup
+BACKUP INTO LATEST IN 'nodelocal://1/test-root/';
+----
+
+exec-sql
+INSERT INTO orig.foo VALUES (4, 'd');
+----
+
+save-cluster-ts tag=end
+----
+
+backup aost=end
+BACKUP INTO LATEST IN 'nodelocal://1/test-root/' AS OF SYSTEM TIME end;
+----
+
+let $backup_path
+SHOW BACKUPS IN 'nodelocal://1/test-root/';
+----
+
+# 2. Pause compaction after the BACKUP-LOCK is written so we can cancel it.
+exec-sql
+SET CLUSTER SETTING jobs.debug.pausepoints = 'backup_compaction.after.details_has_checkpoint';
+----
+
+compact expect-pausepoint start=start end=end tag=comp1
+SELECT crdb_internal.backup_compaction(0, 'BACKUP INTO LATEST IN ''nodelocal://1/test-root/''', '$backup_path', start, end);
+----
+job paused at pausepoint
+
+# 3. Cancel the compaction job to trigger OnFailOrCancel cleanup.
+job cancel=comp1
+----
+
+# 4. Clear pausepoints so the next compaction can run to completion.
+exec-sql
+SET CLUSTER SETTING jobs.debug.pausepoints = '';
+----
+
+# 5. Run a new compaction over the same range. If the stale BACKUP-LOCK from the
+# cancelled job was not cleaned up, this would fail with FileAlreadyExists.
+compact start=start end=end tag=comp2
+SELECT crdb_internal.backup_compaction(0, 'BACKUP INTO LATEST IN ''nodelocal://1/test-root/''', '$backup_path', start, end);
+----
+
+job tag=comp2 wait-for-state=succeeded
+----