fix(retention): apply CodeRabbit review fixes

- Apply post-review fixes to the retention cycle.
- Order chain members topologically (base first, each child after its
  parent via ParentID), with Created only as a tie-breaker. Ordering by
  Created alone could place a child ahead of its base on tied or skewed
  timestamps, and the leaf-inward delete then removes the base before a
  surviving incremental — orphaning it. Stragglers from broken/cyclic
  links are appended, never dropped.
- Stop deleting a chain's members at the first failure (return, not
  continue): once a leaf delete fails, deleting its ancestors would
  orphan it, so abort the chain and leave a complete shorter prefix.
  Other chains still proceed.
- Make chain ordering deterministic: break equal newest() timestamps
  by Root so keep_last / GFS selection is reproducible (map iteration
  is randomized; a planner must not be). Same comparator used in Plan.
- Reject negative retention.max_age in config validation: ParseDuration
  accepts "-1h", which would push the cutoff into the future and make
  the whole catalog deletable.
- Reject negative CLI flag overrides before planning, mirroring the
  config-load validation that flags otherwise bypass.
- Label a partially deleted chain "partially pruned", not "pruned".
- Fix the command name in README and docs/RETENTION.md: it is
  "siphon dumps prune", not "siphon prune".
- Add tests: topological member order on tied/skewed timestamps,
  deterministic chain order on tied timestamps, and that a chain's
  deletion stops (base untouched) when the leaf delete fails.

Author: nixrajput

README.md

@@ -181,7 +181,7 @@ storage:
 
 Credentials are resolved from the standard AWS chain (env vars, `~/.aws`, instance role) — never stored in the config file. See [docs/STORAGE.md](docs/STORAGE.md) for details.
 
-A `retention:` block (default + optional per-profile override) drives `siphon prune`, which deletes old backups as whole chains so an incremental is never orphaned:
+A `retention:` block (default + optional per-profile override) drives `siphon dumps prune`, which deletes old backups as whole chains so an incremental is never orphaned:
 
 ```yaml
 defaults:
@@ -191,7 +191,7 @@ defaults:
     gfs: { daily: 7, weekly: 4, monthly: 6 }
 ```
 
-`siphon prune` is dry-run by default; pass `--apply` to delete. Flags override the configured policy per run. See [docs/RETENTION.md](docs/RETENTION.md) for details.
+`siphon dumps prune` is dry-run by default; pass `--apply` to delete. Flags override the configured policy per run. See [docs/RETENTION.md](docs/RETENTION.md) for details.
 
 ## Architecture
 

docs/RETENTION.md

@@ -1,6 +1,6 @@
 # Retention & pruning
 
-`siphon prune` applies a retention policy to the dump catalog, deleting old
+`siphon dumps prune` applies a retention policy to the dump catalog, deleting old
 backups while guaranteeing it never orphans an incremental from its base. It
 works against any storage backend (local or S3), since it prunes through the
 same `Store.Delete` the catalog already uses.
@@ -70,13 +70,13 @@ duration) fails fast at config load.
 
 ```bash
 # Dry-run (default): show which chains the policy would prune, for one profile.
-siphon prune --profile prod
+siphon dumps prune --profile prod
 
 # Override the configured policy for this run, then actually delete.
-siphon prune --profile prod --keep-last 14 --apply
+siphon dumps prune --profile prod --keep-last 14 --apply
 
 # Pure flag-driven policy (no config), GFS only.
-siphon prune --gfs-daily 7 --gfs-weekly 4 --gfs-monthly 6 --apply
+siphon dumps prune --gfs-daily 7 --gfs-weekly 4 --gfs-monthly 6 --apply
 ```
 
 `--apply` performs deletions; without it, prune only prints the plan. Flags

internal/app/prune.go

@@ -78,16 +78,19 @@ func Prune(ctx context.Context, d Deps, opt PruneOpts) (*PruneResult, error) {
 }
 
 // applyChainDeletion deletes a pruned chain's members leaf-inward (incrementals
-// before the base). A per-dump failure is collected and does not abort the rest
-// of the chain or the run — a single stuck object should not block reclaiming
-// the others.
+// before the base) and STOPS at the first failure within the chain. Stopping is
+// the invariant: if a leaf delete fails (its meta may be gone but the dump still
+// catalogued), continuing to delete its ancestors would orphan that leaf —
+// exactly what the chain-aware design prevents. Aborting the chain leaves a
+// complete, still-restorable prefix. Other chains are unaffected; the failure is
+// recorded and the run continues with them.
 func applyChainDeletion(ctx context.Context, d Deps, ch dumps.Chain, oc *ChainOutcome, result *PruneResult) {
 	for i := len(ch.Members) - 1; i >= 0; i-- {
 		m := ch.Members[i]
 		if err := d.Dumps.Delete(ctx, m.ID); err != nil {
 			oc.Errors = append(oc.Errors, m.ID+": "+err.Error())
 			result.Failed++
-			continue
+			return // do not delete this member's ancestors — would orphan it
 		}
 		oc.Deleted = append(oc.Deleted, m.ID)
 		result.Reclaimed += m.SizeBytes

internal/app/prune_test.go

@@ -193,6 +193,36 @@ func TestPrune_CollectsDeletionFailures(t *testing.T) {
 	}
 }
 
+// TestPrune_StopsChainOnFirstFailure proves the orphan-prevention invariant:
+// when deleting the leaf incremental of a pruned chain fails, the prune must NOT
+// proceed to delete the base — otherwise the surviving leaf is orphaned.
+func TestPrune_StopsChainOnFirstFailure(t *testing.T) {
+	store := newRecordingStore()
+	cat := dumps.New(store)
+	old := time.Now().AddDate(0, 0, -40)
+	seedDump(t, cat, "base", "p", old, "base", "")
+	seedDump(t, cat, "inc", "p", old.Add(time.Hour), "base", "base")
+	// The leaf (inc) is deleted first (leaf-inward). Make its meta delete fail —
+	// Catalog.Delete removes meta first, so this is the first Delete call.
+	store.failKeys["inc.meta.json"] = true
+
+	res, err := Prune(context.Background(), pruneDeps(store), PruneOpts{
+		Policy: dumps.RetentionPolicy{MaxAge: time.Hour}, Apply: true,
+	})
+	if err != nil {
+		t.Fatalf("Prune: %v", err)
+	}
+	if res.Failed != 1 {
+		t.Errorf("Failed = %d, want 1", res.Failed)
+	}
+	// The base must NOT have been deleted — neither its meta nor its body.
+	for _, k := range store.deletes {
+		if strings.HasPrefix(k, "base") {
+			t.Errorf("base was deleted after leaf failure (orphans the leaf): deletes=%v", store.deletes)
+		}
+	}
+}
+
 // helpers
 
 func firstIndexWithPrefix(s []string, p string) int {

internal/cli/dumps.go

@@ -100,6 +100,13 @@ func dumpsPruneCmd() *cobra.Command {
 			if gfsMSet {
 				policy.GFS.Monthly = gfsMonthly
 			}
+			// Config-backed policy is validated at load; flag overrides bypass that,
+			// so reject negatives here too before a destructive run sees an
+			// undefined policy.
+			if policy.KeepLast < 0 || policy.MaxAge < 0 ||
+				policy.GFS.Daily < 0 || policy.GFS.Weekly < 0 || policy.GFS.Monthly < 0 {
+				return &errs.Error{Op: "prune", Code: errs.CodeUser, Hint: "retention values must be non-negative"}
+			}
 
 			deps, err := buildDeps()
 			if err != nil {
@@ -169,7 +176,13 @@ func renderPruneResult(w io.Writer, res *app.PruneResult) {
 			continue
 		}
 		prunedChains++
-		_, _ = fmt.Fprintf(w, "%s chain %s (%d dump(s), %d bytes)\n", verb, oc.Root, len(oc.DumpIDs), oc.SizeBytes)
+		// On apply, a chain with errors was only partially deleted (the delete
+		// aborted leaf-inward at the failure), so don't label it fully "pruned".
+		label := verb
+		if res.Apply && len(oc.Errors) > 0 {
+			label = "partially pruned"
+		}
+		_, _ = fmt.Fprintf(w, "%s chain %s (%d dump(s), %d bytes)\n", label, oc.Root, len(oc.DumpIDs), oc.SizeBytes)
 		for _, e := range oc.Errors {
 			_, _ = fmt.Fprintf(w, "  ! %s\n", e)
 		}

internal/config/config.go

@@ -59,9 +59,16 @@ func (r *RetentionConfig) Validate() error {
 		return errors.New("retention.gfs tiers must be >= 0")
 	}
 	if r.MaxAge != "" {
-		if _, err := time.ParseDuration(r.MaxAge); err != nil {
+		d, err := time.ParseDuration(r.MaxAge)
+		if err != nil {
 			return fmt.Errorf("retention.max_age %q is not a valid duration: %w", r.MaxAge, err)
 		}
+		// time.ParseDuration accepts signed inputs ("-1h"); a negative max-age
+		// would push the cutoff into the future and make the whole catalog
+		// eligible for deletion, so reject it.
+		if d < 0 {
+			return fmt.Errorf("retention.max_age must be >= 0, got %q", r.MaxAge)
+		}
 	}
 	return nil
 }

internal/dumps/retention.go

@@ -66,8 +66,14 @@ type RetentionPlan struct {
 // backup (BaseID == ID, or legacy empty BaseID) is a singleton chain; its
 // incrementals attach to it via their BaseID. A dump whose root is missing from
 // the set anchors its own chain rather than being dropped, so a partially
-// present catalog never loses entries silently. Within each chain, members are
-// ordered by Created (base first).
+// present catalog never loses entries silently.
+//
+// Within each chain, members are ordered by ParentID/BaseID TOPOLOGY (base
+// first, then each child after its parent), with Created only as a tie-breaker.
+// This is the contract the leaf-inward delete path relies on: deleting members
+// last-to-first must remove every descendant before its ancestor. Ordering by
+// Created alone would break that on tied or skewed timestamps — a child could
+// sort ahead of its parent and the base could be deleted while a leaf survives.
 func GroupChains(dumps []Meta) []Chain {
 	byRoot := map[string][]Meta{}
 	for _, m := range dumps {
@@ -79,18 +85,88 @@ func GroupChains(dumps []Meta) []Chain {
 	}
 	chains := make([]Chain, 0, len(byRoot))
 	for root, members := range byRoot {
-		sort.SliceStable(members, func(i, j int) bool {
-			return members[i].Created.Before(members[j].Created)
-		})
-		chains = append(chains, Chain{Root: root, Members: members})
+		chains = append(chains, Chain{Root: root, Members: orderMembers(root, members)})
 	}
-	// Deterministic order: newest chain (by newest member) first.
+	// Deterministic order: newest chain first, Root breaking ties so the plan is
+	// reproducible across runs even when timestamps collide (map iteration is
+	// randomized; a destructive planner must not be).
 	sort.SliceStable(chains, func(i, j int) bool {
-		return chains[i].newest().After(chains[j].newest())
+		ti, tj := chains[i].newest(), chains[j].newest()
+		if ti.Equal(tj) {
+			return chains[i].Root < chains[j].Root
+		}
+		return ti.After(tj)
 	})
 	return chains
 }
 
+// orderMembers returns a chain's members in topological apply order: the base
+// (root) first, then each incremental placed immediately it can be (its parent
+// already emitted). Created breaks ties among siblings and gives a stable order.
+// Any members left unplaceable by broken/cyclic ParentID links are appended in
+// Created order so nothing is dropped — a corrupt chain still surfaces all its
+// dumps for the caller to handle.
+func orderMembers(root string, members []Meta) []Meta {
+	// Stable Created order first, so ties resolve deterministically.
+	sort.SliceStable(members, func(i, j int) bool {
+		if members[i].Created.Equal(members[j].Created) {
+			return members[i].ID < members[j].ID
+		}
+		return members[i].Created.Before(members[j].Created)
+	})
+
+	byID := make(map[string]Meta, len(members))
+	for _, m := range members {
+		byID[m.ID] = m
+	}
+
+	ordered := make([]Meta, 0, len(members))
+	emitted := make(map[string]bool, len(members))
+
+	// Emit the base first if present (its ID == root, or it self-roots / is a
+	// legacy empty-BaseID full backup).
+	if base, ok := byID[root]; ok {
+		ordered = append(ordered, base)
+		emitted[base.ID] = true
+	}
+
+	// attachable reports whether m can be emitted now: its parent is already
+	// emitted, or its parent is not part of this chain (absent/self/empty), in
+	// which case it attaches at the root level. The base itself is handled above.
+	attachable := func(m Meta) bool {
+		if m.ID == root {
+			return false // already emitted (or will be appended as a straggler)
+		}
+		p := m.ParentID
+		return p == "" || p == m.ID || emitted[p] || byID[p].ID == ""
+	}
+
+	// Iterate to a fixed point; members are Created-sorted, so each pass emits the
+	// earliest now-attachable child.
+	for {
+		progressed := false
+		for _, m := range members {
+			if emitted[m.ID] || !attachable(m) {
+				continue
+			}
+			ordered = append(ordered, m)
+			emitted[m.ID] = true
+			progressed = true
+		}
+		if !progressed {
+			break
+		}
+	}
+
+	// Append any stragglers (cycles) in Created order — never drop a member.
+	for _, m := range members {
+		if !emitted[m.ID] {
+			ordered = append(ordered, m)
+		}
+	}
+	return ordered
+}
+
 // Plan decides which chains to keep vs prune under p, as of now. It is pure: no
 // I/O, no clock — now is injected — so every rule and edge case is unit-testable
 // with synthetic fixtures. An empty policy keeps everything.
@@ -99,10 +175,16 @@ func Plan(chains []Chain, p RetentionPolicy, now time.Time) RetentionPlan {
 		return RetentionPlan{Keep: append([]Chain(nil), chains...)}
 	}
 
-	// chains is already sorted newest-first by GroupChains; copy defensively.
+	// chains is already sorted newest-first by GroupChains; copy and re-sort with
+	// the SAME deterministic comparator (Root breaks ties) so callers that build
+	// a Chain slice by hand still get reproducible keep_last / GFS selection.
 	ordered := append([]Chain(nil), chains...)
 	sort.SliceStable(ordered, func(i, j int) bool {
-		return ordered[i].newest().After(ordered[j].newest())
+		ti, tj := ordered[i].newest(), ordered[j].newest()
+		if ti.Equal(tj) {
+			return ordered[i].Root < ordered[j].Root
+		}
+		return ti.After(tj)
 	})
 
 	keep := map[string]bool{} // union of every active rule's keep set

internal/dumps/retention_test.go

@@ -58,6 +58,71 @@ func TestGroupChains(t *testing.T) {
 			t.Fatalf("orphan grouping = %+v, want one chain rooted at missingbase", chains)
 		}
 	})
+
+	t.Run("members ordered by topology, not Created, on tied timestamps", func(t *testing.T) {
+		// Base and incremental share the SAME Created time (clock skew / coarse
+		// granularity). Topological order MUST still put the base first, because
+		// the leaf-inward delete path deletes last-to-first and would otherwise
+		// remove the base before its surviving child.
+		same := daysAgo(2)
+		ds := []Meta{
+			dump("inc", "p", same, "base", "base"), // listed first, same timestamp
+			dump("base", "p", same, "base", ""),
+		}
+		chains := GroupChains(ds)
+		if len(chains) != 1 {
+			t.Fatalf("got %d chains, want 1", len(chains))
+		}
+		got := chains[0].Members
+		if len(got) != 2 || got[0].ID != "base" || got[1].ID != "inc" {
+			t.Errorf("member order = [%s %s], want [base inc] (base first despite tie)", got[0].ID, got[1].ID)
+		}
+	})
+
+	t.Run("members topologically ordered when a child predates its parent", func(t *testing.T) {
+		// Pathological: the incremental's Created is BEFORE the base's (skew).
+		// Topology must still win — base first.
+		ds := []Meta{
+			dump("base", "p", daysAgo(1), "base", ""),
+			dump("inc", "p", daysAgo(5), "base", "base"), // older than its base
+		}
+		chains := GroupChains(ds)
+		got := chains[0].Members
+		if got[0].ID != "base" {
+			t.Errorf("member order = [%s ...], want base first despite older child", got[0].ID)
+		}
+	})
+}
+
+func TestGroupChains_DeterministicOnTiedChainTimestamps(t *testing.T) {
+	// Two independent chains with identical newest() timestamps must sort in a
+	// stable order (by Root) across runs — a destructive planner cannot depend on
+	// randomized map iteration. Run the grouping repeatedly and assert identical
+	// output.
+	same := daysAgo(3)
+	ds := []Meta{
+		dump("zzz", "p", same, "", ""),
+		dump("aaa", "p", same, "", ""),
+		dump("mmm", "p", same, "", ""),
+	}
+	first := roots(GroupChains(ds))
+	for i := 0; i < 20; i++ {
+		if got := roots(GroupChains(ds)); !equalStrings(got, first) {
+			t.Fatalf("non-deterministic chain order: run %d = %v, first = %v", i, got, first)
+		}
+	}
+}
+
+func equalStrings(a, b []string) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for i := range a {
+		if a[i] != b[i] {
+			return false
+		}
+	}
+	return true
 }
 
 func TestPlan_EmptyPolicyKeepsEverything(t *testing.T) {