fix(restore): keep a restore's source, refuse orphan targets, take over stale rollback marks

cmd/controller/main.go

@@ -280,6 +280,7 @@ func main() {
 	}
 	if err := (&controller.AutoSnapshotRunnable{
 		Client: mgr.GetClient(),
+		Store:  st,
 	}).RegisterWithManager(mgr); err != nil {
 		setupLog.Error(err, "Failed to register runnable", "runnable", "auto-snapshot")
 		os.Exit(1)

docs/cli-parity-known-deltas.md

@@ -64,6 +64,7 @@ Row IDs match the command-catalogue indexes used by `cli-parity-refresh.sh` (see
 
 | 07 | `rd l --resource-definitions <rd>` (Layers column on a placed RD) | WIRE_SHAPE | permanent | Same `stampRDLayerDataFromStack` behaviour as row 81 — the flag-qualified `rd l --resource-definitions <rd>` catalogue cell (harness index 07) the bare `rd l` whitelist string does not literally cover. BS re-synthesises `layer_data: [{"type":"DRBD"},{"type":"STORAGE"}]` from `Spec.LayerStack` on every RD read, so the python CLI's Layers column renders `DRBD,STORAGE` even for a fresh `parity-rd` with only a volume-definition; upstream 1.33.2 leaves the column blank until DRBD layer data is actually allocated. BLOCKSTOR_SUPERSET, operator-friendly, and linstor-csi / piraeus-operator do not gate on RD `layer_data`. See row 81 for the full rationale and the L1/L3 pins (`stampRDLayerDataFromStack`, `tests/contract/normalize_test.go::TestNormalizeRDLayerDataDropped`). |
 | 33 | `s d <rd> <nonexistent-snap>` (idempotent snapshot delete) | WIRE_SHAPE | permanent | BLOCKSTOR idempotent-delete envelope. Deleting a snapshot definition that does not exist returns `SUCCESS: snapshot already absent: <snap>` (the desired-state delete is satisfied); upstream LINSTOR returns `WARNING: Snapshot definition <snap> of resource <rd> not found`. Both exit 0. Deliberate: a CSI `DeleteSnapshot` / operator retry MUST be idempotent, so "already gone" is a success, not a warning. Mirrors the resource-delete idempotency in row 42. Pinned by the snapshot-delete REST handler's already-absent path (`pkg/rest/snapshots.go`). |
+| 93 | `s d <rd> <snap>` while a definition is still being restored from it | BEHAVIOR | permanent | BLOCKSTOR_STRICTER. Upstream deletes the snapshot. blockstor keeps a snapshot a definition still restores from: a definition whose restore is finished has its marker taken off and the delete goes on, and one whose restore is still running makes the delete answer a retryable 409 naming it, so its next replica does not come up blank. A delete also marks the snapshot first, so a restore that races it withdraws instead of answering over a snapshot that is gone. Pinned by the unit tests in `pkg/store` and `pkg/rest`; the L6 cell `cli-matrix/snap-d-restore-source.sh` and the L7 replay `replay/snap-d-restore-source.yaml` cover it on a stand and have not run there yet. |
 | 42 | `r d <rd> <nonexistent-node>` (idempotent resource delete) | WIRE_SHAPE | permanent | BLOCKSTOR idempotent-delete envelope. Deleting a resource placement on a node that holds none returns `SUCCESS: resource already absent: <node> on <rd>`; upstream LINSTOR returns `WARNING: Node: <rd>, Resource: <node> not found`. Both exit 0. Deliberate: a CSI `DeleteVolume` / operator retry on an already-removed placement MUST be idempotent. Same family as the snapshot-delete idempotency in row 33. Pinned by the resource-delete REST handler's already-absent path (`pkg/rest/resources.go`). |
 | 40 | `n c <node> <ip> --node-type Satellite` (node-create success envelope) | WIRE_SHAPE | permanent | BLOCKSTOR envelope shape on satellite node registration. BS emits `node created: <node>` + a `SUCCESS: No active connection to satellite '<node>'` line; upstream emits `New node '<node>' registered.` (with a UUID detail) + a `WARNING: No active connection to satellite '<node>'` line whose Details explain the controller will (re-)establish the connection. Both register the node and exit 0; the operator-visible outcome (node exists, awaiting satellite handshake) is identical. The "no active connection" notice is INFO/SUCCESS-class in BS vs WARNING-class upstream, and BS does not surface the volatile node UUID inline. Envelope-shape only; no behavioural divergence. |
 | 16 | `ps l` (physical-storage list — `size` field) | WIRE_SHAPE | 2026-12-31 | BS `/v1/physical-storage` omits the per-device `size` key that python-linstor's `show_physical_storage` reads unconditionally (`linstor/responses.py` `devices.size`), so `linstor ps l` raises `KeyError: 'size'` and exits 2 against BS where upstream renders the table and exits 0. `ps l` is a hardware-discovery convenience surface; linstor-csi / piraeus-operator never call it, and the device-pool creation path (`ps create-device-pool`, cli-matrix `ps-cdp-*`) is unaffected. Tracked as a missing wire field to populate on the physical-storage DTO; whitelisted until then. |

internal/cli/rollback_budget_test.go

@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: Apache-2.0
+
+/*
+Copyright 2026 Cozystack contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cli
+
+import (
+	"context"
+	"errors"
+	"testing"
+	"time"
+
+	apiv1 "github.com/cozystack/blockstor/pkg/api/v1"
+	"github.com/cozystack/blockstor/pkg/store"
+)
+
+type deadlineRecordingRDs struct {
+	store.ResourceDefinitionStore
+
+	deadlines *[]time.Duration
+}
+
+func (d deadlineRecordingRDs) PatchResourceDefinitionSpec(
+	ctx context.Context, name string, mutate func(*apiv1.ResourceDefinition) error,
+) error {
+	if deadline, ok := ctx.Deadline(); ok {
+		*d.deadlines = append(*d.deadlines, time.Until(deadline))
+	} else {
+		*d.deadlines = append(*d.deadlines, -1)
+	}
+
+	return d.ResourceDefinitionStore.PatchResourceDefinitionSpec(ctx, name, mutate) //nolint:wrapcheck // test double
+}
+
+type deadlineRecordingStore struct {
+	store.Store
+
+	deadlines *[]time.Duration
+}
+
+func (s deadlineRecordingStore) ResourceDefinitions() store.ResourceDefinitionStore {
+	return deadlineRecordingRDs{ResourceDefinitionStore: s.Store.ResourceDefinitions(), deadlines: s.deadlines}
+}
+
+var errRestorePlacement = errors.New("placement failed")
+
+// A CLI rollback writes the same in-progress mark a REST one does, and a
+// retry takes a mark over once it is stale on the grounds that no rollback
+// can still be running. The CLI command has no deadline of its own, so the
+// rollback must carry one, shorter than the staleness bound.
+func TestCLIRollbackRunsOnABudget(t *testing.T) {
+	t.Parallel()
+
+	ctx := t.Context()
+	backend := store.NewInMemory()
+
+	if err := backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-budget"}); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+
+	var deadlines []time.Duration
+
+	run := &runContext{
+		Store: deadlineRecordingStore{Store: backend, deadlines: &deadlines},
+		Flags: &flagSet{Values: map[string]string{}},
+	}
+
+	_ = rollbackRestore(context.Background(), run, "pvc-budget", nil, errRestorePlacement)
+
+	if len(deadlines) == 0 {
+		t.Fatal("the rollback wrote no mark")
+	}
+
+	for _, left := range deadlines {
+		if left < 0 || left > store.CLIRollbackBudget {
+			t.Fatalf("a rollback write ran with %s left, want a deadline within %s", left, store.CLIRollbackBudget)
+		}
+	}
+}

internal/cli/snapshot.go

@@ -351,7 +351,7 @@ func snapshotRestoreResource(ctx context.Context, run *runContext) error {
 	}
 
 	// The snapshot was read before the definition existed; see
-	// store.ReapClonedSnapshot.
+	// store.ReapClonedSnapshot and store.DeleteSnapshot.
 	err = store.RestoreSourceWithdrawn(ctx, run.Store, snap.ResourceName, snap.Name)
 	if err != nil {
 		return rollbackRestore(ctx, run, def.Name, planned, err)
@@ -508,7 +508,7 @@ func restoreIntoExistingTarget(
 // placeOverMarkedTarget places the planned replicas on a target that carries
 // this restore's marker. The snapshot was read before the marker went on, or
 // before this run found it there, so it is read back first; see
-// store.ReapClonedSnapshot.
+// store.ReapClonedSnapshot and store.DeleteSnapshot.
 func placeOverMarkedTarget(
 	ctx context.Context, run *runContext, rdName string, snap *apiv1.Snapshot, planned []apiv1.Resource,
 ) error {
@@ -778,6 +778,14 @@ func volumeAlreadyRestored(ctx context.Context, run *runContext, rdName string,
 func rollbackRestore(
 	ctx context.Context, run *runContext, rdName string, planned []apiv1.Resource, cause error,
 ) error {
+	// The rollback runs on a budget of its own, like the REST door's. A retry
+	// takes over an in-progress mark older than store.StaleRollbackAfter on
+	// the grounds that nobody is behind it; a CLI rollback without a
+	// deadline could still be, and could then take apart a definition the
+	// retry had answered for.
+	ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), store.CLIRollbackBudget)
+	defer cancel()
+
 	err := store.HoldRollback(ctx, run.Store, rdName, true)
 	if errors.Is(err, store.ErrRollbackGone) {
 		return cause
@@ -1097,13 +1105,21 @@ func unwindVolumes(ctx context.Context, run *runContext, rdName string, added []
 func planRestoredReplicas(
 	ctx context.Context, run *runContext, srcRD, rdName string, snap *apiv1.Snapshot,
 ) ([]apiv1.Resource, error) {
+	// The same refusal the REST door makes, before anything is written: see
+	// store.RefuseReplicasWithoutDefinition. A target that already has its
+	// definition passes it.
+	err := store.RefuseReplicasWithoutDefinition(ctx, run.Store, rdName)
+	if err != nil {
+		return nil, err //nolint:wrapcheck // names the definition and the replicas itself
+	}
+
 	nodes := run.Flags.Nodes
 
 	// Named nodes have to actually hold the snapshot. Restoring onto one
 	// that does not produces a replica with nothing behind it: the objects
 	// are created, the satellite finds no snapshot to receive, and the
 	// command reports success over an empty volume.
-	err := validate.RestoreNodesHoldSnapshot(nodes, snap.Nodes)
+	err = validate.RestoreNodesHoldSnapshot(nodes, snap.Nodes)
 	if err != nil {
 		//nolint:wrapcheck // a semantic refusal; see checkVolumeSize
 		return nil, err

internal/cli/snapshot_delete_race_test.go

@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: Apache-2.0
+
+/*
+Copyright 2026 Cozystack contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cli_test
+
+import (
+	"context"
+	"errors"
+	"strings"
+	"testing"
+	"time"
+
+	apiv1 "github.com/cozystack/blockstor/pkg/api/v1"
+	"github.com/cozystack/blockstor/pkg/store"
+)
+
+// A delete of a snapshot that is not there is the no-op it always was, even
+// with a definition whose marker names it and whose restore can never finish.
+func TestSnapshotDeleteOfAMissingSnapshotIgnoresAHangingMarker(t *testing.T) {
+	t.Parallel()
+
+	app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) {
+		seedSnapshotSource(ctx, backend)
+		_ = backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{
+			Name: "pvc-hang", Props: map[string]string{store.RestoreFromSnapshotProp: "pvc-x:snap-gone"},
+		})
+	})
+
+	if got := app.Run(t.Context(), []string{"s", "d", "pvc-x", "snap-gone"}); got != 0 {
+		t.Fatalf("delete of a missing snapshot exit = %d (stderr: %s)", got, errBuf.String())
+	}
+}
+
+// The CLI restore reads its snapshot back once the marked definition stands,
+// the way the REST door does, and withdraws from one a delete has marked: the
+// delete listed dependents before the definition existed and will not see it.
+func TestSnapshotRestoreWithdrawsFromASnapshotBeingDeleted(t *testing.T) {
+	t.Parallel()
+
+	app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) {
+		seedSnapshotSource(ctx, backend)
+
+		snap, _ := backend.Snapshots().Get(ctx, "pvc-x", "snap-1")
+		snap.Props = map[string]string{store.SnapshotDeletingProp: "tok@" + time.Now().UTC().Format(time.RFC3339)}
+		_ = backend.Snapshots().Update(ctx, &snap)
+	})
+
+	if got := app.Run(t.Context(), restoreResourceArgv("pvc-wd")); got == 0 {
+		t.Fatal("restore from a snapshot being deleted succeeded")
+	}
+
+	if !strings.Contains(errBuf.String(), "retry once it finishes") {
+		t.Errorf("refusal %q does not say to retry once the delete finishes", errBuf.String())
+	}
+
+	if _, err := appStore(t, app).ResourceDefinitions().Get(t.Context(), "pvc-wd"); !errors.Is(err, store.ErrNotFound) {
+		t.Errorf("the withdrawn restore left its definition behind: %v", err)
+	}
+}
+
+// A prepared target the restore marked reads the snapshot back before it
+// places anything.
+func TestSnapshotRestoreIntoAPreparedTargetWithdrawsFromASnapshotBeingDeleted(t *testing.T) {
+	t.Parallel()
+
+	app, _, errBuf := newApp(t, seedSnapshotSource)
+
+	for _, argv := range [][]string{
+		{"rd", "c", "pvc-prep-wd"},
+		{"s", "vd", "restore", "--from-resource", "pvc-x", "--from-snapshot", "snap-1", "--to-resource", "pvc-prep-wd"},
+	} {
+		if got := app.Run(t.Context(), argv); got != 0 {
+			t.Fatalf("%v exit = %d (stderr: %s)", argv, got, errBuf.String())
+		}
+	}
+
+	st := appStore(t, app)
+
+	snap, err := st.Snapshots().Get(t.Context(), "pvc-x", "snap-1")
+	if err != nil {
+		t.Fatalf("read the snapshot: %v", err)
+	}
+
+	snap.Props = map[string]string{store.SnapshotDeletingProp: "tok@" + time.Now().UTC().Format(time.RFC3339)}
+	if err := st.Snapshots().Update(t.Context(), &snap); err != nil {
+		t.Fatalf("mark the snapshot: %v", err)
+	}
+
+	if got := app.Run(t.Context(), restoreResourceArgv("pvc-prep-wd")); got == 0 {
+		t.Fatal("restore into a prepared target from a snapshot being deleted succeeded")
+	}
+
+	replicas, err := st.Resources().ListByDefinition(t.Context(), "pvc-prep-wd")
+	if err != nil || len(replicas) != 0 {
+		t.Errorf("replicas placed after the refusal = %d (%v), want none", len(replicas), err)
+	}
+}

internal/cli/snapshot_delete_restore_source_test.go

@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: Apache-2.0
+
+/*
+Copyright 2026 Cozystack contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cli
+
+import (
+	"errors"
+	"testing"
+
+	apiv1 "github.com/cozystack/blockstor/pkg/api/v1"
+	"github.com/cozystack/blockstor/pkg/store"
+)
+
+// The CLI deletes snapshots through the store directly, so it runs the same
+// release the REST door does: a snapshot an unfinished restore reads from
+// stays, with the reason.
+func TestSnapshotDeleteRefusesWhileARestoreReadsFromIt(t *testing.T) {
+	t.Parallel()
+
+	ctx := t.Context()
+	st := store.NewInMemory()
+
+	if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "src-cli"}); err != nil {
+		t.Fatalf("seed source: %v", err)
+	}
+
+	if err := st.Snapshots().Create(ctx, &apiv1.Snapshot{Name: "snap-cli", ResourceName: "src-cli"}); err != nil {
+		t.Fatalf("seed snapshot: %v", err)
+	}
+
+	if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{
+		Name:  "dst-cli",
+		Props: map[string]string{store.RestoreFromSnapshotProp: "src-cli:snap-cli"},
+	}); err != nil {
+		t.Fatalf("seed restored definition: %v", err)
+	}
+
+	run := &runContext{
+		Store: st,
+		Flags: &flagSet{Values: map[string]string{}, Positionals: []string{"src-cli", "snap-cli"}},
+	}
+
+	err := snapshotDelete(ctx, run)
+	if !errors.Is(err, store.ErrRestoreSourceInUse) {
+		t.Fatalf("snapshot delete under an unfinished restore = %v, want ErrRestoreSourceInUse", err)
+	}
+
+	if _, err := st.Snapshots().Get(ctx, "src-cli", "snap-cli"); err != nil {
+		t.Fatalf("the refused delete took the snapshot: %v", err)
+	}
+}