feat: handle bucket conflicts for restore

[devanbenz]

This PR adds server side handling of new --on-conflict parameters: error, skip, and replace.

error has no changes and functions precisely as before. This is the default sent from influx-cli.

skip just skips buckets that already exist.

replace will write out a temporary bucket and then swap the tmp bucket with the targeted one to replace. The old targeted bucket will be deleted.

[Copilot]

Pull request overview

Adds configurable bucket-conflict handling to restore metadata requests.

Changes:

• Supports error, skip, and replace conflict modes.
• Adds handler tests covering each mode and restore failures.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
http/restore_service.go	Implements conflict handling.
http/restore_service_test.go	Tests conflict-mode behavior.

Suppressed comments (1)

http/restore_service.go:287

• replace irreversibly drops the existing bucket and all of its stored data before the replacement metadata and shard data are restored. Any later failure—including CreateBucket, RestoreBucket, client cancellation, or a subsequent shard upload—leaves the user with no usable old bucket; the new test explicitly codifies this for RestoreBucket failure. Stage/validate the replacement before removing the current bucket, and only switch over once the restore has completed successfully (or provide rollback).

				h.Logger.Info("Restore: bucket already exists, replacing",
					zap.String("bucket", b.BucketName), zap.String("bucket_id", existing.ID.String()))
				if err := h.BucketService.DeleteBucket(ctx, existing.ID); err != nil {

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 12 changed files in this pull request and generated 4 comments.

Files not reviewed (1)

• mock/backup_service.go: Generated file

Suppressed comments (1)

http/restore_service.go:334

• If this update fails, the endpoint still returns 200 and the CLI completes the restore, while tenant bucket metadata retains the old description/retention and storage metadata already contains the backup values. This silently leaves two metadata stores inconsistent. Make this update part of an atomic/compensated replacement workflow, or otherwise surface a completion failure after shard uploads can proceed.

		}); err != nil {
			h.Logger.Warn("Failed to update replaced bucket's metadata to match the backup",
				zap.String("bucket_id", target.ID.String()), zap.Error(err))

[Copilot]

Pull request overview

Copilot reviewed 10 out of 12 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• mock/backup_service.go: Generated file

Suppressed comments (2)

tsdb/store.go:1185

• Restoring each shard with a fresh epoch tracker loses pending writes and delete guards that were registered before the batch claim. After an early failure restores these shards, later deletion operations no longer wait for those in-flight operations. Keep the existing tracker while a shard is restorable, and delete it only for shards that were actually closed.

		for id, sh := range restorable {
			s.shards[id] = sh
			s.epochs[id] = newEpochTracker()

storage/engine.go:527

• This deletes the existing bucket's shards before the restore has completed. The CLI receives these mappings and uploads each shard afterward, stopping on the first local-read or HTTP error; such a failure therefore leaves the live bucket irreversibly empty or partially restored. Preserve the old shards until the client explicitly finalizes all successful uploads, or stage the new metadata/shards and atomically swap them during finalization.

	// Delete data for the replaced shards, only after the commit
	if len(replacedShardIDs) > 0 {
		if err := e.tsdbStore.DeleteShardsByID(replacedShardIDs); err != nil {

[Copilot]

Pull request overview

Copilot reviewed 10 out of 12 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• mock/backup_service.go: Generated file

Suppressed comments (2)

storage/engine.go:529

• This permanently deletes the existing bucket's shards before the client has uploaded any restored shard data. The pinned CLI uploads shards only after this metadata request returns, so a network error or failed shard upload leaves the bucket empty or partially restored with no way to recover its previous contents. Replacement needs a staging/commit-or-abort protocol that retains the old metadata and shards until every upload succeeds.

	// Delete data for the replaced shards, only after the commit
	if len(replacedShardIDs) > 0 {
		if err := e.tsdbStore.DeleteShardsByID(replacedShardIDs); err != nil {
			e.logger.Warn("Failed to delete replaced shards during restore",
				zap.Uint64s("shard_ids", replacedShardIDs), zap.Error(err))

http/restore_service.go:292

• An intentional skip returns an empty mapping, but the pinned CLI treats every missing shard mapping as unexpected and logs WARN: Server didn't map ID ... once per shard. Thus a successful --on-conflict skip produces potentially many misleading warnings. Return an explicit skipped outcome, or update the CLI to suppress missing-mapping warnings when skip was requested.

	if outcome == restoredBucketSkipped {
		// Skipped: respond with no shard mappings so the client has nothing to
		// upload for this bucket, leaving the existing data untouched.
		h.api.Respond(w, r, http.StatusOK, influxdb.RestoredBucketMappings{
			ID:            target.ID,
			Name:          target.Name,
			ShardMappings: []influxdb.RestoredShardMapping{},

[Copilot]

Pull request overview

Copilot reviewed 10 out of 12 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• mock/backup_service.go: Generated file

Suppressed comments (1)

storage/engine.go:530

• A failed old-shard deletion is only logged after the restored metadata has already replaced the sole record of those shard IDs. The request then succeeds, and a retry can discover only the new shard IDs, so the old shard files become permanently orphaned and replace can leak all prior bucket data on disk. Preserve failed IDs in a durable cleanup mechanism or make the metadata/deletion operation recoverable instead of dropping this error.

		if err := e.tsdbStore.DeleteShardsByID(replacedShardIDs); err != nil {
			e.logger.Warn("Failed to delete replaced shards during restore",
				zap.Uint64s("shard_ids", replacedShardIDs), zap.Error(err))
		}

[Copilot]

Pull request overview

Copilot reviewed 12 out of 14 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• mock/backup_service.go: Generated file

Suppressed comments (1)

tsdb/store.go:1250

• The waiters above guard only the doomed shards, so writes to surviving shards continue while their series-ID sets are sampled. If a series present in a doomed shard is added to a survivor after this Diff but before DeleteSeriesID, the write reuses the existing shared series ID and this deletion then permanently tombstones an ID referenced by the survivor. Synchronize database-wide writes/shard creation across the survivor snapshot and series-file deletion (or use an equivalent atomic reference check).

			ss.Diff(index.SeriesIDSet())

[Copilot]

Pull request overview

Copilot reviewed 12 out of 14 changed files in this pull request and generated 3 comments.

Files not reviewed (1)

• mock/backup_service.go: Generated file

[jstirnaman]

Once this feature is released, merge influxdata/docs-v2#7699 to publish the param in the spec hosted at docs.influxdata.com

[Copilot]

🟡 Changes recommended

Shard deletion races and incomplete persisted recovery can corrupt series state or leave staged data permanently untracked.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (1)

• mock/backup_service.go: Generated file

Suppressed comments (1)

tsdb/store.go:1115

• Removing these shards from restorable before close/file removal makes later physical failures non-retryable. If CloseAndRemoveMetrics or either RemoveAll fails, the deferred cleanup clears pendingShardDeletes but does not put the shard back in s.shards; the next DeleteShardsByID call therefore skips the ID and reports success. The replace finalizer can then clear its manifest while shard files or a running shard remain. Preserve retryable deletion state (including paths) until physical removal succeeds.

		// The series file no longer references what these shards owned, so
		// they can no longer be put back.
		for _, sh := range doomed {
			delete(restorable, sh.id)

• Files reviewed: 14/16 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Crash recovery, deletion retries, and concurrent upload cleanup contain unresolved correctness issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (1)

• mock/backup_service.go: Generated file

Suppressed comments (1)

tsdb/store.go:1163

• These shards become permanently untracked before the close and RemoveAll calls below have succeeded. If either operation fails, the deferred cleanup clears pendingShardDeletes without restoring the shard; a retry then treats the ID as absent and returns success, leaving its files behind. Keep failed removals in retryable path/state (or only discard that state after both directories are removed).

		for _, sh := range doomed {
			delete(restorable, sh.id)
		}

• Files reviewed: 15/17 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Staged-upload races and non-retryable shard deletion failures can discard restored data or leave orphaned files.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (1)

• mock/backup_service.go: Generated file

Suppressed comments (1)

storage/engine.go:730

• Removing these mappings before marking the replace dropped lets a concurrent RestoreShard observe no staged entry and bypass both the dropped check and the upload guard. Existing uploads are not waited on either because this function never takes st.uploads; callers can consequently delete shard files during an upload and the upload may report success for discarded data. Keep the mapping visible while marking and draining uploads, and remove it only in coordination with the caller's shard cleanup.

		delete(e.stagedReplaces, id)
		for _, sid := range st.shardIDs {
			delete(e.stagedShards, sid)
		}

• Files reviewed: 15/17 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Empty-restore crash recovery and concurrent bucket deletion can leave replacement state inconsistent or orphaned.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (1)

• mock/backup_service.go: Generated file

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

storage/engine.go:936

• An empty replacement is misclassified as committed after a crash. Before finalizeStagedReplace runs, the initial manifest has neither ShardIDs nor ReplacedShardIDs; if the process exits in that window, this branch sets committed = true, applies the bucket update, and removes the recovery record even though the metadata swap never happened, leaving the old bucket contents in place. Persist an explicit commit phase (or the pre-swap shard set) rather than inferring an empty replacement's state from two empty lists.

• Files reviewed: 15/17 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Staged replacement cancellation and crash recovery can race with uploads or lose durable cleanup state.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (1)

• mock/backup_service.go: Generated file

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

storage/engine.go:979

• Failed shard-cleanup debt is retained only in the local remaining manifest, not in any engine map. Later in the same startup, SetBucketService calls applyPendingBucketUpdates, whose writeStagedManifestLocked rebuilds the file solely from stagedReplaces and pendingBucketUpdates; after a successful update it therefore erases these ReplacedShardIDs, so the failed deletion is never retried on another startup. Keep outstanding shard IDs in persistent in-memory state that every manifest rewrite includes until deletion succeeds.

storage/engine.go:750

• This does not actually wait for an in-flight shard upload: RestoreShard holds st.uploads.RLock(), but this path only takes st.mu, after which callers immediately delete the staged shard files. A bucket delete or superseding restore can therefore remove a shard while tsdbStore.RestoreShard is writing it. Take the exclusive uploads lock before marking the entry dropped; using uploads-before-mu ordering also avoids deadlocking a last upload that enters finalization while holding the read lock.

	// Taken after stagedMu: a commit holds st.mu while briefly taking stagedMu.
	for _, st := range dropped {
		st.mu.Lock()
		st.dropped = true
		st.mu.Unlock()
	}

• Files reviewed: 15/17 changed files
• Comments generated: 1
• Review effort level: Balanced