From c8d39bc803b41b352f445707443b73304dcf46a5 Mon Sep 17 00:00:00 2001 From: balaji Date: Tue, 11 Aug 2026 21:57:57 -0700 Subject: [PATCH 1/3] fix(nvsnap): size L2 PVCs from the capture, not a 10 GiB floor defaultL2Size measures rootfs/cachedir captures and multiplies by 1.2, then max()es against a 10 GiB floor -- so the measurement was discarded for every capture under ~8.5 GiB. Observed on dev2: a 269 MB cachedir capture provisioned a 10 GiB PVC, roughly 20 GiB raw on the RAID-10 VPG, for a 37x over-allocation. The L2 StorageClasses are Retain, so it is permanent and it is per capture hash. That is the same defect this function was written to remove, one order of magnitude down. Its own comment complains that a 14 GB capture asked for 96 GiB on a Retain StorageClass; the fix for that left a 10 GiB floor doing the same thing to small captures. Floor drops to 2 GiB. It exists to stop a degenerate manifest provisioning a sub-GiB volume, not to act as a default size. The 1.2x multiplier already covers filesystem overhead (block rounding, inodes, metadata), and the L2 StorageClasses set allowVolumeExpansion, so sizing tight is recoverable. The existing floor test caught the change, as intended. Replaced it with one that pins both ends: the dev2 269 MB capture lands on the floor, and a 5 GB capture sizes from the measurement rather than being inflated to it. Co-Authored-By: Balaji Ganesan --- .../nvsnap/internal/agent/l2_integration.go | 10 +++++++- .../internal/agent/l2_integration_test.go | 25 ++++++++++++++----- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/compute-plane-services/nvsnap/internal/agent/l2_integration.go b/src/compute-plane-services/nvsnap/internal/agent/l2_integration.go index d71f0af6d0..526bdebd94 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/l2_integration.go +++ b/src/compute-plane-services/nvsnap/internal/agent/l2_integration.go @@ -237,8 +237,16 @@ func defaultL2Size(_ string, m checkpointstore.Manifest) int64 { // capture fell through to the vRAM estimate below: a 14 GB capture // asked for 96 GiB (80 GiB H100 default x 1.2). On a Retain // StorageClass that over-allocation is permanent and per-capture. + // The floor is a guard against a degenerate manifest, not a default: it + // used to be 10 GiB, which discarded the measured size for every capture + // under ~8.5 GiB. A 269 MB cachedir capture provisioned 10 GiB (~20 GiB + // raw on a RAID-10 VPG), permanently, because these StorageClasses are + // Retain -- the same over-allocation this function was written to remove, + // one order of magnitude down. The 1.2x multiplier already covers fs + // overhead, and the L2 StorageClasses set allowVolumeExpansion, so + // sizing tight costs nothing. if (m.CaptureMethod == "rootfs" || m.CaptureMethod == "cachedir") && m.TotalSizeBytes > 0 { - const floor = 10 * oneGiB + const floor = 2 * oneGiB return max(m.TotalSizeBytes*12/10, floor) } diff --git a/src/compute-plane-services/nvsnap/internal/agent/l2_integration_test.go b/src/compute-plane-services/nvsnap/internal/agent/l2_integration_test.go index 9c5b2eb072..516251a941 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/l2_integration_test.go +++ b/src/compute-plane-services/nvsnap/internal/agent/l2_integration_test.go @@ -104,13 +104,26 @@ func TestDefaultL2Size_RootfsUsesTotalSize(t *testing.T) { } } -// Small rootfs captures get a 10 GiB floor so fs overhead on a tiny tree -// doesn't provision a sub-GiB PVC that fails to format/fit. -func TestDefaultL2Size_RootfsFloor(t *testing.T) { +// The floor guards against a degenerate manifest; it is not a default size. +// It was 10 GiB, which meant the measured size was discarded for anything +// under ~8.5 GiB -- a 269 MB cachedir capture provisioned 10 GiB, permanently, +// because the L2 StorageClasses are Retain. Sizes above the floor must come +// from the measurement. +func TestDefaultL2Size_FloorOnlyAppliesBelowIt(t *testing.T) { const oneGiB = int64(1) << 30 - m := checkpointstore.Manifest{CaptureMethod: "rootfs", TotalSizeBytes: 2 * oneGiB} - if got := defaultL2Size("hash", m); got != 10*oneGiB { - t.Errorf("2 GB rootfs tree → %d GiB, want 10 GiB floor", got/oneGiB) + + // Well under the floor: a 269 MB cachedir capture (the dev2 case). + tiny := checkpointstore.Manifest{CaptureMethod: "cachedir", TotalSizeBytes: 269 * 1000 * 1000} + if got := defaultL2Size("hash", tiny); got != 2*oneGiB { + t.Errorf("269 MB cachedir capture → %d GiB, want the 2 GiB floor", got/oneGiB) + } + + // Above the floor: measurement wins, floor must not inflate it. + m := checkpointstore.Manifest{CaptureMethod: "rootfs", TotalSizeBytes: 5 * oneGiB} + want := 5 * oneGiB * 12 / 10 + if got := defaultL2Size("hash", m); got != want { + t.Errorf("5 GB rootfs tree → %d GiB, want %d GiB (measured x1.2, not the floor)", + got/oneGiB, want/oneGiB) } } From dc9fe2134ed310d77ec79b1311d23c09e886b8d7 Mon Sep 17 00:00:00 2001 From: balaji Date: Wed, 12 Aug 2026 09:08:23 -0700 Subject: [PATCH 2/3] fix(nvsnap): record the main container's entrypoint; stop orphaning dumps on partial delete Two capture/restore correctness bugs found while validating the NVCA-driven cachedir path on a live cluster, plus the L2 sizing fix from #785. nvsnap#788 -- restore exec'd the wrong container's entrypoint. The capture orchestrator recorded the entrypoint from ResolvePodPID, which returns the lowest non-sandbox PID in the POD. That is documented as "sufficient for upperdir resolution" and it is: every container in a pod resolves the same overlay. It is wrong for the entrypoint, where the container identity is the whole question -- it returns whichever container started first. NVCF function pods run a `utils` sidecar that comes up while the main container is still pulling its image, so the sidecar always won the lowest-PID race: podCreated 04:05:36 / utils 04:05:51 / inference 04:08:48 Capture recorded the sidecar's argv, and restore exec'd it inside the main container: recorded source entrypoint ... entry_argv="[/usr/bin/app --config /etc/app/config.yaml]" nvsnap-rootfs-restore: exec [/usr/bin/app ...]: no such file or directory /usr/bin/app does not exist in that image; the real entrypoint is a /bin/sh -c uvicorn wrapper. The pod CrashLoopBackOff'd while capture reported success and the checkpoint was genuinely valid -- it presents as a workload bug. Adds ResolveContainerPID, which matches the cgroup against one container ID, and plumbs the main container's ID through CaptureRequest. Falls back to the pod PID when the ID is unknown or its process has exited, so single-container pods behave exactly as before. ResolvePodPID is unchanged and still used for upperdir/mountinfo. nvsnap#736 -- DELETE dropped the catalog row even when a tier failed. The catalog delete ran unconditionally after the tier cascade and set AnySuccess itself, so a cascade where the agent 401'd under --auth-mode=required still returned 204 No Content. The row is the only pointer to the on-disk dump, so the bytes were orphaned with nothing left to retry or GC them, and the caller was told it succeeded. The rows are now retained whenever a tier delete failed, tracked on an explicit CatalogRetained flag rather than by appending to Errors (that list is the set of things that actually failed, and callers count it -- an existing test caught this). The handler returns 500 with the summary instead of 204. Delete is idempotent, so retrying once the failing tier recovers is safe. nvsnap#785 -- L2 PVC floor 10 GiB -> 2 GiB, so the measured capture size is not discarded below ~8.5 GiB. A 269 MB cachedir capture was provisioning 10 GiB (~20 GiB raw on RAID-10), permanently, on a Retain StorageClass. Tests: ResolveContainerPID picks the named container over the lower-PID sidecar and skips pause; cascade delete retains the row on tier failure and still deletes it on a clean run; L2 sizing pins both sides of the floor. Not yet validated end to end -- the dev2 cluster was lost before the fixed build could be deployed. #788 and #736 are covered by unit tests only. Co-Authored-By: Balaji Ganesan --- .../internal/rootfsonly/orchestrator.go | 39 ++++++++++- .../nvsnap/internal/rootfsonly/pidresolver.go | 55 ++++++++++++++++ .../internal/rootfsonly/pidresolver_test.go | 66 +++++++++++++++++++ .../nvsnap/internal/rootfsonly/watcher.go | 43 ++++++++++-- .../internal/server/delete_checkpoint_test.go | 58 ++++++++++++++++ .../nvsnap/internal/server/server.go | 40 ++++++++++- 6 files changed, 290 insertions(+), 11 deletions(-) diff --git a/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go b/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go index 99a444cbba..a714841177 100644 --- a/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go +++ b/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go @@ -53,6 +53,23 @@ type CaptureRequest struct { // and image we use. Most nvsnap workloads are single-container at 0. MainContainer int + // MainContainerID is the runtime container ID of Spec.Containers + // [MainContainer], as reported in pod.status (with the runtime scheme + // stripped). Used to resolve that container's PID specifically when + // recording the entrypoint to replay at restore. + // + // Pod-scoped PID resolution is not good enough here: it returns the + // lowest non-sandbox PID in the pod, which is whichever container + // started first. NVCF function pods run a `utils` sidecar that starts + // while the main container is still pulling its image, so the sidecar + // always won and capture recorded ITS argv -- restore then exec'd a + // binary that does not exist in the main container (nvsnap#788). + // + // Empty when the status lookup found no running main container; the + // orchestrator then falls back to the pod-scoped PID rather than + // recording nothing. + MainContainerID string + // HashInput is the canonical content-addressed identity (image // digest + model id + engine compat flags + driver major). The agent // composes this from the pod spec; the orchestrator hashes it. @@ -261,9 +278,27 @@ func (c *Capturer) Capture(ctx context.Context, req CaptureRequest) (checkpoints if procRoot == "" { procRoot = mountinfo.DefaultProcRoot() } - entryArgv := readEntryArgv(procRoot, pid) + // Read the argv from the MAIN container's PID, not the pod's. `pid` + // above is the lowest non-sandbox PID in the pod -- whichever container + // started first -- which on a multi-container pod is the sidecar, not + // the workload (nvsnap#788). Fall back to the pod PID when the main + // container ID is unknown or its process has already gone, so a + // single-container pod behaves exactly as before. + entryPID := pid + if req.MainContainerID != "" { + if cpid, cerr := c.PIDResolver.ResolveContainerPID(req.MainContainerID); cerr == nil { + entryPID = cpid + } else { + log.WithError(cerr).WithField("main_container_id", req.MainContainerID). + Warn("could not resolve main container PID; falling back to the pod PID for entrypoint recording") + } + } + entryArgv := readEntryArgv(procRoot, entryPID) if len(entryArgv) > 0 { - log.WithField("entry_argv", entryArgv).Info("recorded source entrypoint for whole-rootfs restore") + log.WithFields(map[string]interface{}{ + "entry_argv": entryArgv, + "entry_pid": entryPID, + }).Info("recorded source entrypoint for whole-rootfs restore") } else { log.Warn("could not read source /proc//cmdline; restore will rely on the pod's command/args") } diff --git a/src/compute-plane-services/nvsnap/internal/rootfsonly/pidresolver.go b/src/compute-plane-services/nvsnap/internal/rootfsonly/pidresolver.go index cf52d07a60..a0730cd0aa 100644 --- a/src/compute-plane-services/nvsnap/internal/rootfsonly/pidresolver.go +++ b/src/compute-plane-services/nvsnap/internal/rootfsonly/pidresolver.go @@ -134,6 +134,61 @@ func (r *PIDResolver) ResolvePodPID(podUID string) (int, error) { return lowestPID, nil } +// ResolveContainerPID returns the lowest non-sandbox PID whose cgroup names +// containerID, i.e. a PID inside that ONE container rather than anywhere in +// the pod. containerID is the runtime ID from pod.status with the scheme +// stripped ("containerd://" -> ""). +// +// ResolvePodPID is deliberately pod-scoped and documents itself as +// "sufficient for upperdir resolution" -- true, because every container in a +// pod resolves the same overlay. It is wrong for anything where the container +// identity is the question. Recording the entrypoint is exactly that: the +// pod-scoped PID is whichever container started first, so on an NVCF function +// pod the `utils` sidecar beat the main container (still pulling its image) +// and capture recorded the sidecar's argv (nvsnap#788). +// +// Returns ErrPodNotRunning when no live PID matches, so callers can fall back. +func (r *PIDResolver) ResolveContainerPID(containerID string) (int, error) { + if containerID == "" { + return 0, errors.New("rootfsonly: ResolveContainerPID: empty containerID") + } + + entries, err := os.ReadDir(r.ProcRoot) + if err != nil { + return 0, fmt.Errorf("read proc root %q: %w", r.ProcRoot, err) + } + + lowestPID := -1 + for _, e := range entries { + if !e.IsDir() { + continue + } + pid, err := strconv.Atoi(e.Name()) + if err != nil { + continue // not a PID directory + } + match, err := pidMatchesAny(filepath.Join(r.ProcRoot, e.Name(), "cgroup"), []string{containerID}) + if err != nil { + // Same rationale as ResolvePodPID: /proc entries vanish when a + // process exits mid-walk; skip rather than fail the resolve. + continue + } + if !match { + continue + } + if isSandboxPID(filepath.Join(r.ProcRoot, e.Name(), "comm")) { + continue + } + if lowestPID == -1 || pid < lowestPID { + lowestPID = pid + } + } + if lowestPID == -1 { + return 0, ErrPodNotRunning + } + return lowestPID, nil +} + // isSandboxPID reads /proc//comm and reports whether the comm name // matches a known K8s sandbox/runtime process (pause, containerd-shim). // Errors (process exited mid-walk, etc.) are treated as "not sandbox" so diff --git a/src/compute-plane-services/nvsnap/internal/rootfsonly/pidresolver_test.go b/src/compute-plane-services/nvsnap/internal/rootfsonly/pidresolver_test.go index 7a9203a173..9f66c3d977 100644 --- a/src/compute-plane-services/nvsnap/internal/rootfsonly/pidresolver_test.go +++ b/src/compute-plane-services/nvsnap/internal/rootfsonly/pidresolver_test.go @@ -239,3 +239,69 @@ func TestNewPIDResolver_DefaultsToProc(t *testing.T) { t.Fatalf("ProcRoot = %q, want /proc or /host/proc", r.ProcRoot) } } + +// The bug in nvsnap#788: capture read the entrypoint from the pod-scoped PID, +// which is whichever container started first. On an NVCF function pod the +// `utils` sidecar starts while the main container is still pulling its image, +// so the sidecar always had the lower PID and capture recorded ITS argv. +// Restore then exec'd a binary that does not exist in the main container. +// +// Models that exact layout: sidecar PID 100, main container PID 900, same pod. +func TestResolveContainerPID_PicksTheNamedContainerNotTheLowestPodPID(t *testing.T) { + const ( + podUID = "0b4b630f-72b9-4a3b-884a-d630844351c7" + sidecar = "1111111111111111111111111111111111111111111111111111111111111111" + mainCtr = "2222222222222222222222222222222222222222222222222222222222222222" + cgFmt = "0::/kubepods.slice/kubepods-besteffort.slice/pod%s.slice/cri-containerd-%s.scope" + ) + uidUnderscored := strings.ReplaceAll(podUID, "-", "_") + root := fakeProc(t, map[int]string{ + 100: fmt.Sprintf(cgFmt, uidUnderscored, sidecar), // starts first -> lowest PID + 900: fmt.Sprintf(cgFmt, uidUnderscored, mainCtr), // main container + }) + r := &PIDResolver{ProcRoot: root} + + // Pod-scoped resolution returns the sidecar. This is correct for its + // documented purpose (upperdir) and wrong for the entrypoint. + if got, err := r.ResolvePodPID(podUID); err != nil || got != 100 { + t.Fatalf("ResolvePodPID = %d, %v; want 100 (documents the pre-fix behavior)", got, err) + } + + // Container-scoped resolution must return the main container's PID. + got, err := r.ResolveContainerPID(mainCtr) + if err != nil { + t.Fatalf("ResolveContainerPID: %v", err) + } + if got != 900 { + t.Errorf("ResolveContainerPID = %d, want 900 (the main container, not the lowest pod PID)", got) + } +} + +func TestResolveContainerPID_SkipsSandbox(t *testing.T) { + const ctr = "3333333333333333333333333333333333333333333333333333333333333333" + cg := "0::/kubepods.slice/pod_x.slice/cri-containerd-" + ctr + ".scope" + root := fakeProcWithComm(t, + map[int]string{10: cg, 42: cg}, + map[int]string{10: "pause"}, // sandbox must not win despite lower PID + ) + r := &PIDResolver{ProcRoot: root} + got, err := r.ResolveContainerPID(ctr) + if err != nil { + t.Fatalf("ResolveContainerPID: %v", err) + } + if got != 42 { + t.Errorf("ResolveContainerPID = %d, want 42 (pause at PID 10 must be skipped)", got) + } +} + +// Unknown/exited container must be a clean miss so the orchestrator can fall +// back to the pod PID rather than recording nothing. +func TestResolveContainerPID_NotFoundAndEmpty(t *testing.T) { + r := &PIDResolver{ProcRoot: fakeProc(t, map[int]string{7: "0::/kubepods.slice/other.scope"})} + if _, err := r.ResolveContainerPID("deadbeef"); !errors.Is(err, ErrPodNotRunning) { + t.Errorf("unknown container: err = %v, want ErrPodNotRunning", err) + } + if _, err := r.ResolveContainerPID(""); err == nil { + t.Error("empty containerID: want an error") + } +} diff --git a/src/compute-plane-services/nvsnap/internal/rootfsonly/watcher.go b/src/compute-plane-services/nvsnap/internal/rootfsonly/watcher.go index 9b05540861..ecf0f25ecc 100644 --- a/src/compute-plane-services/nvsnap/internal/rootfsonly/watcher.go +++ b/src/compute-plane-services/nvsnap/internal/rootfsonly/watcher.go @@ -21,6 +21,7 @@ import ( "context" "errors" "fmt" + "strings" "sync" "time" @@ -221,12 +222,13 @@ func (w *Watcher) runCapture(ctx context.Context, pod *corev1.Pod) { hashInput := w.Composer.Compose(pod, 0) req := CaptureRequest{ - PodUID: string(pod.UID), - Namespace: pod.Namespace, - Name: pod.Name, - Spec: &pod.Spec, - MainContainer: 0, - HashInput: hashInput, + PodUID: string(pod.UID), + Namespace: pod.Namespace, + Name: pod.Name, + Spec: &pod.Spec, + MainContainer: 0, + MainContainerID: mainContainerID(pod, 0), + HashInput: hashInput, } // Resolve GPU metadata from the capture node's labels (best-effort; // empty on lookup failure). GPUType/DriverVersion are display-only; @@ -352,3 +354,32 @@ func gpuInfoFromNodeLabels(labels map[string]string) (gpuType, driverVersion, co } return gpuType, driverVersion, computeCapability } + +// mainContainerID returns the runtime container ID of spec.Containers[idx] +// from pod.status, with the runtime scheme stripped ("containerd://" -> +// ""). Empty when the container has no status yet or is not running. +// +// The capture orchestrator uses this to read the entrypoint from the main +// container's own PID. Pod-scoped PID resolution returns whichever container +// started first, which on an NVCF function pod is the `utils` sidecar -- +// capture then recorded the sidecar's argv and restore exec'd a binary that +// does not exist in the main container (nvsnap#788). +func mainContainerID(pod *corev1.Pod, idx int) string { + if pod == nil || idx < 0 || idx >= len(pod.Spec.Containers) { + return "" + } + name := pod.Spec.Containers[idx].Name + for i := range pod.Status.ContainerStatuses { + cs := &pod.Status.ContainerStatuses[i] + if cs.Name != name || cs.ContainerID == "" { + continue + } + // "containerd://<64-hex>" / "docker://" -> "". The cgroup + // path carries the bare ID, so match on that. + if i := strings.Index(cs.ContainerID, "://"); i >= 0 { + return cs.ContainerID[i+3:] + } + return cs.ContainerID + } + return "" +} diff --git a/src/compute-plane-services/nvsnap/internal/server/delete_checkpoint_test.go b/src/compute-plane-services/nvsnap/internal/server/delete_checkpoint_test.go index 2b89cd6d97..a843a1fc3b 100644 --- a/src/compute-plane-services/nvsnap/internal/server/delete_checkpoint_test.go +++ b/src/compute-plane-services/nvsnap/internal/server/delete_checkpoint_test.go @@ -455,3 +455,61 @@ func TestCascadeDelete_NoCRDDeleteForRowsWithoutNamespace(t *testing.T) { t.Errorf("namespace-less row should not generate errors; got %v", res.Errors) } } + +// nvsnap#736: the catalog row is the only pointer to the on-disk dump. When a +// tier delete fails (agent 401 under --auth-mode=required, agent down, +// blobstore 5xx), dropping the row orphans those bytes -- nothing references +// them, so nothing retries or GCs them. Worse, the catalog delete itself set +// AnySuccess, so the handler returned 204 No Content while the dump survived. +// +// The row must be retained on any tier failure so the delete stays retryable. +func TestCascadeDelete_RetainsCatalogRowWhenATierFails(t *testing.T) { + s := newTestServerWithCatalog(t) + fb, fbSrv := newFakeBlobstore() + fb.failHash["server-broken"] = http.StatusInternalServerError + defer fbSrv.Close() + s.config.BlobstoreURL = fbSrv.URL + + row := &db.Checkpoint{ID: "ck-1", Hash: "abc123abc123abc123", CheckpointPath: "/var/lib/nvsnap/checkpoints/server-broken"} + if err := s.catalog.UpsertCheckpoint(row); err != nil { + t.Fatalf("seed catalog: %v", err) + } + + res := s.cascadeDeleteCheckpoint(context.Background(), "ck-1", "server-broken", row) + + if len(res.Errors) == 0 { + t.Fatal("expected a tier error from the failing blobstore") + } + if res.CatalogRows != 0 { + t.Errorf("CatalogRows = %d, want 0 — the row must survive a failed tier delete", res.CatalogRows) + } + // The row itself must still be readable, or the dump is unreachable. + if got, err := s.catalog.GetCheckpoint("ck-1"); err != nil || got == nil { + t.Errorf("catalog row was deleted despite a tier failure (get: %v) — dump is now orphaned", err) + } + if res.Status() != "partial" { + t.Errorf("Status() = %q, want partial", res.Status()) + } +} + +// The clean path must still delete the row, or nothing is ever reclaimed. +func TestCascadeDelete_DeletesCatalogRowWhenAllTiersSucceed(t *testing.T) { + s := newTestServerWithCatalog(t) + _, fbSrv := newFakeBlobstore() + defer fbSrv.Close() + s.config.BlobstoreURL = fbSrv.URL + + row := &db.Checkpoint{ID: "ck-ok", Hash: "def456def456def456", CheckpointPath: "/var/lib/nvsnap/checkpoints/agent-ok"} + if err := s.catalog.UpsertCheckpoint(row); err != nil { + t.Fatalf("seed catalog: %v", err) + } + + res := s.cascadeDeleteCheckpoint(context.Background(), "ck-ok", "agent-ok", row) + + if len(res.Errors) != 0 { + t.Fatalf("clean cascade should have no errors; got %v", res.Errors) + } + if res.CatalogRows == 0 { + t.Error("CatalogRows = 0 — a clean cascade must delete the row") + } +} diff --git a/src/compute-plane-services/nvsnap/internal/server/server.go b/src/compute-plane-services/nvsnap/internal/server/server.go index 36959c7eea..ea0a5fd429 100644 --- a/src/compute-plane-services/nvsnap/internal/server/server.go +++ b/src/compute-plane-services/nvsnap/internal/server/server.go @@ -1009,6 +1009,16 @@ func (s *Server) deleteCheckpoint(w http.ResponseWriter, r *http.Request) { Status: result.Status(), Message: result.Summary(), }) + // A partial cascade must not report 204. The catalog row is retained in + // that case (see cascadeDeleteCheckpoint), so the checkpoint still + // exists and the caller has to retry -- telling them "No Content" hides + // an orphaned dump behind an apparent success (nvsnap#736). The delete + // is idempotent, so retrying after the failing tier recovers is safe. + if result.CatalogRetained { + s.writeError(w, http.StatusInternalServerError, + "checkpoint partially deleted; catalog row retained for retry: "+result.Summary()) + return + } w.WriteHeader(http.StatusNoContent) } @@ -1065,6 +1075,12 @@ type cascadeDeleteResult struct { CaptureCMs int // count of rootfs-capture-manifest ConfigMaps deleted (per hash) Errors []string AnySuccess bool + + // CatalogRetained is set when the catalog rows were deliberately kept + // because a tier delete failed. The row is the only pointer to the + // on-disk dump, so dropping it on a partial cascade orphans those bytes + // (nvsnap#736). Callers must not report success when this is set. + CatalogRetained bool } // Status returns "success" when every attempted sub-delete succeeded @@ -1172,9 +1188,27 @@ func (s *Server) cascadeDeleteCheckpoint(ctx context.Context, id, agentID string s.deleteCaptureManifestCM(ctx, row.Hash, &result) } - // Catalog rows — DeleteByHash takes everything sharing this hash - // in one statement. Falls back to single-id delete when hash is - // unknown. + // Catalog rows LAST, and only when the tiers above actually cleaned up. + // + // The row is the only pointer to the on-disk dump. Deleting it while a + // tier delete failed (agent 401 under --auth-mode=required, agent down, + // blobstore unreachable) orphans those bytes: nothing references them, + // so nothing will ever retry or GC them. Keeping the row on failure + // makes the delete retryable and keeps the dump discoverable. + // + // This was the worst part of nvsnap#736 -- the catalog delete also set + // AnySuccess, so a cascade where every tier failed still returned 204 No + // Content. The caller was told the checkpoint was gone while the bytes + // stayed on disk. + if len(result.Errors) > 0 { + // Not appended to Errors: that list is the set of things that + // actually failed, and callers count it. + result.CatalogRetained = true + return result + } + + // DeleteByHash takes everything sharing this hash in one statement. + // Falls back to single-id delete when hash is unknown. if row != nil && row.Hash != "" { n, err := s.catalog.DeleteByHash(row.Hash) if err != nil { From 5e032e2b5767794f6bf094f66e4b96f5a9a2b275 Mon Sep 17 00:00:00 2001 From: balaji Date: Fri, 14 Aug 2026 06:27:03 -0700 Subject: [PATCH 3/3] fix(nvsnap): close the delete-status gaps, refresh the pod before capture Review follow-ups on #785. Three are real defects, two are hygiene. Delete handlers could still report success over a retained catalog row, which is the nvsnap#736 failure mode this PR set out to close: - A cascade that failed on EVERY tier leaves AnySuccess false, the same as a checkpoint that never existed, so the id endpoint answered 404 -- telling the caller the dump was gone while it sat on disk. A genuine 404 now also requires CatalogRetained false. - deleteCheckpointByHash never consulted CatalogRetained at all, so a partial cascade returned 204 with the row still there. It runs the same cascade, so it owes the same answer. - A DeleteByHash/DeleteCheckpoint error appended to Errors but left CatalogRetained false. With any other tier already successful the id endpoint returned 204 over a row the catalog had just failed to delete. runCapture holds a DeepCopy taken before WarmupDelay (60s default), and derived MainContainerID from it. A main-container restart in that window mints a new runtime ID; the stale one matches no cgroup, ResolveContainerPID misses, and the orchestrator falls back to the pod PID -- the sidecar on a multi-container pod, reopening the nvsnap#788 bug this PR fixes. Re-read the pod first, extracted as refreshPodForCapture so it is testable. A failed read falls back to the copy we hold (best-effort); a changed UID abandons the capture, since the pod we scheduled is gone and its replacement gets its own capture from its own Ready event. Entrypoint argv went to the service log in full. Inference entrypoints carry --api-key, HF tokens and signed model URLs, so log argc instead. The argv still goes to the manifest, which is what restore replays. Also correct the sizing comment (the parent already matched cachedir in the measured-size branch; the new floor is 5x smaller than the old, not an order of magnitude) and drop non-ASCII from the test diagnostics this PR added, per the subtree's ASCII rule. Every new test was confirmed to fail without its fix. The first attempt at the catalog-delete-error test passed with the fix reverted -- closing the DB also failed the sibling lookup, so the pre-delete early return set the flag instead. Reworked to seed a row with an empty Hash, which gates out every other catalog call in the cascade and leaves DeleteCheckpoint as the only one that can fail. go build ./... and go test -race ./internal/... green. Co-Authored-By: Balaji Ganesan --- .../nvsnap/internal/agent/l2_integration.go | 26 ++--- .../internal/agent/l2_integration_test.go | 4 +- .../internal/rootfsonly/orchestrator.go | 6 +- .../nvsnap/internal/rootfsonly/watcher.go | 36 ++++++ .../internal/rootfsonly/watcher_test.go | 73 ++++++++++++ .../internal/server/delete_checkpoint_test.go | 109 +++++++++++++++++- .../nvsnap/internal/server/server.go | 19 ++- 7 files changed, 252 insertions(+), 21 deletions(-) diff --git a/src/compute-plane-services/nvsnap/internal/agent/l2_integration.go b/src/compute-plane-services/nvsnap/internal/agent/l2_integration.go index 526bdebd94..3a6291ef13 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/l2_integration.go +++ b/src/compute-plane-services/nvsnap/internal/agent/l2_integration.go @@ -232,19 +232,19 @@ func defaultL2Size(_ string, m checkpointstore.Manifest) int64 { const oneGiB int64 = 1 << 30 // Rootfs and cachedir paths both measure what they captured, so size - // from that rather than guessing. cachedir was added after this - // function and did not match the rootfs-only check, so every cachedir - // capture fell through to the vRAM estimate below: a 14 GB capture - // asked for 96 GiB (80 GiB H100 default x 1.2). On a Retain - // StorageClass that over-allocation is permanent and per-capture. - // The floor is a guard against a degenerate manifest, not a default: it - // used to be 10 GiB, which discarded the measured size for every capture - // under ~8.5 GiB. A 269 MB cachedir capture provisioned 10 GiB (~20 GiB - // raw on a RAID-10 VPG), permanently, because these StorageClasses are - // Retain -- the same over-allocation this function was written to remove, - // one order of magnitude down. The 1.2x multiplier already covers fs - // overhead, and the L2 StorageClasses set allowVolumeExpansion, so - // sizing tight costs nothing. + // from that rather than guessing. Sizing from the vRAM estimate below + // asked 96 GiB (80 GiB H100 default x 1.2) for a 14 GB capture, and on + // a Retain StorageClass that over-allocation is permanent and + // per-capture. + // + // The floor is a guard against a degenerate manifest, not a default. It + // was 10 GiB, which discarded the measured size for every capture under + // ~8.5 GiB: a 269 MB cachedir capture still provisioned 10 GiB (~20 GiB + // raw on a RAID-10 VPG), permanently. Same over-allocation this branch + // exists to avoid, just 5x smaller than the vRAM estimate instead of + // 350x. The 1.2x multiplier already covers fs overhead and the L2 + // StorageClasses set allowVolumeExpansion, so sizing tight costs + // nothing. if (m.CaptureMethod == "rootfs" || m.CaptureMethod == "cachedir") && m.TotalSizeBytes > 0 { const floor = 2 * oneGiB return max(m.TotalSizeBytes*12/10, floor) diff --git a/src/compute-plane-services/nvsnap/internal/agent/l2_integration_test.go b/src/compute-plane-services/nvsnap/internal/agent/l2_integration_test.go index 516251a941..3cca705ad6 100644 --- a/src/compute-plane-services/nvsnap/internal/agent/l2_integration_test.go +++ b/src/compute-plane-services/nvsnap/internal/agent/l2_integration_test.go @@ -115,14 +115,14 @@ func TestDefaultL2Size_FloorOnlyAppliesBelowIt(t *testing.T) { // Well under the floor: a 269 MB cachedir capture (the dev2 case). tiny := checkpointstore.Manifest{CaptureMethod: "cachedir", TotalSizeBytes: 269 * 1000 * 1000} if got := defaultL2Size("hash", tiny); got != 2*oneGiB { - t.Errorf("269 MB cachedir capture → %d GiB, want the 2 GiB floor", got/oneGiB) + t.Errorf("269 MB cachedir capture -> %d GiB, want the 2 GiB floor", got/oneGiB) } // Above the floor: measurement wins, floor must not inflate it. m := checkpointstore.Manifest{CaptureMethod: "rootfs", TotalSizeBytes: 5 * oneGiB} want := 5 * oneGiB * 12 / 10 if got := defaultL2Size("hash", m); got != want { - t.Errorf("5 GB rootfs tree → %d GiB, want %d GiB (measured x1.2, not the floor)", + t.Errorf("5 GiB rootfs tree -> %d GiB, want %d GiB (measured x1.2, not the floor)", got/oneGiB, want/oneGiB) } } diff --git a/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go b/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go index a714841177..427afc20b9 100644 --- a/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go +++ b/src/compute-plane-services/nvsnap/internal/rootfsonly/orchestrator.go @@ -295,8 +295,12 @@ func (c *Capturer) Capture(ctx context.Context, req CaptureRequest) (checkpoints } entryArgv := readEntryArgv(procRoot, entryPID) if len(entryArgv) > 0 { + // argc, never argv: inference entrypoints routinely carry + // --api-key / HF tokens / signed model URLs, and this log ships to + // the cluster log stack. The argv still goes into the manifest, + // which is what restore replays, but that is not a service log. log.WithFields(map[string]interface{}{ - "entry_argv": entryArgv, + "entry_argc": len(entryArgv), "entry_pid": entryPID, }).Info("recorded source entrypoint for whole-rootfs restore") } else { diff --git a/src/compute-plane-services/nvsnap/internal/rootfsonly/watcher.go b/src/compute-plane-services/nvsnap/internal/rootfsonly/watcher.go index ecf0f25ecc..72b76e8204 100644 --- a/src/compute-plane-services/nvsnap/internal/rootfsonly/watcher.go +++ b/src/compute-plane-services/nvsnap/internal/rootfsonly/watcher.go @@ -192,6 +192,37 @@ func podGPURequest(pod *corev1.Pod) int64 { return total } +// refreshPodForCapture re-reads the Pod immediately before a capture and +// returns the object to capture from, plus whether to proceed at all. +// +// runCapture holds a DeepCopy taken before WarmupDelay (60s by default). +// Container IDs are the field that goes stale in that window: a +// main-container restart mints a new runtime ID, the old one matches no +// cgroup, so ResolveContainerPID misses and the orchestrator falls back to +// the pod PID -- the sidecar on a multi-container pod, which is exactly the +// bug nvsnap#788 fixed. Re-reading keeps MainContainerID naming what is +// actually running. +// +// A read failure is not fatal: fall back to the copy we already have, since +// a stale ID costs only that pre-#788 fallback. A UID change is fatal -- +// same name, different pod means ours was deleted and recreated during the +// delay, and capturing now would stamp the live pod's bytes with the dead +// pod's identity. The replacement gets its own capture from its own Ready +// event. +func (w *Watcher) refreshPodForCapture(ctx context.Context, pod *corev1.Pod, log *logrus.Entry) (*corev1.Pod, bool) { + fresh, err := w.KubeClient.CoreV1().Pods(pod.Namespace).Get(ctx, pod.Name, metav1.GetOptions{}) + if err != nil { + log.WithError(err).Warn("pod re-read before capture failed; using the pre-warmup copy") + return pod, true + } + if fresh.UID != pod.UID { + log.WithField("fresh_pod_uid", string(fresh.UID)). + Warn("pod was replaced during the warmup delay; abandoning this capture") + return nil, false + } + return fresh, true +} + // runCapture sleeps WarmupDelay, acquires a concurrency slot, and runs Capture. // Failures are logged but don't tear down the watcher; transient errors are // retried by re-firing on subsequent Pod Update events (we clear captured @@ -220,6 +251,11 @@ func (w *Watcher) runCapture(ctx context.Context, pod *corev1.Pod) { captureCtx, cancel := context.WithTimeout(ctx, 30*time.Minute) defer cancel() + pod, ok := w.refreshPodForCapture(captureCtx, pod, log) + if !ok { + return + } + hashInput := w.Composer.Compose(pod, 0) req := CaptureRequest{ PodUID: string(pod.UID), diff --git a/src/compute-plane-services/nvsnap/internal/rootfsonly/watcher_test.go b/src/compute-plane-services/nvsnap/internal/rootfsonly/watcher_test.go index bfb16adea4..f345dd3944 100644 --- a/src/compute-plane-services/nvsnap/internal/rootfsonly/watcher_test.go +++ b/src/compute-plane-services/nvsnap/internal/rootfsonly/watcher_test.go @@ -341,3 +341,76 @@ func TestGPUInfoFromNodeLabels(t *testing.T) { t.Errorf("empty labels should yield empty: %q %q %q", gt, dv, cc) } } + +// withContainerID stamps a running container status so mainContainerID has +// a runtime ID to return. +func withContainerID(pod *corev1.Pod, id string) *corev1.Pod { + pod.Status.ContainerStatuses = []corev1.ContainerStatus{{ + Name: pod.Spec.Containers[0].Name, + ContainerID: "containerd://" + id, + }} + return pod +} + +// runCapture holds a DeepCopy taken before WarmupDelay. If the main container +// restarts during that wait, the copy names a container ID that no longer +// exists; ResolveContainerPID then misses and the orchestrator falls back to +// the pod PID, which on a multi-container pod is the sidecar -- reopening +// nvsnap#788. The re-read must hand back the ID that is live now. +func TestRefreshPodForCapture_PicksUpRestartedContainerID(t *testing.T) { + env := newWatcherEnv(t) + w := env.watcher() + + stale := withContainerID( + fakePod("uid-restart", "vllm-8b", map[string]string{DefaultCaptureLabel: "true"}, true), + "oldcontainerid0000") + // What the API server holds after the restart: same pod, new runtime ID. + live := withContainerID(stale.DeepCopy(), "newcontainerid1111") + w.KubeClient = fake.NewSimpleClientset(live) + + got, ok := w.refreshPodForCapture(context.Background(), stale, w.logger().WithField("t", "x")) + if !ok { + t.Fatal("a live pod with the same UID must be captured, not abandoned") + } + if id := mainContainerID(got, 0); id != "newcontainerid1111" { + t.Errorf("MainContainerID = %q, want the post-restart ID; a stale ID resolves to "+ + "no cgroup and falls back to the sidecar PID (nvsnap#788)", id) + } +} + +// Same name, different UID: the pod we scheduled was deleted and replaced +// during the delay. Capturing would label the new pod's bytes with the dead +// pod's identity, so the capture is dropped; the replacement brings its own +// Ready event. +func TestRefreshPodForCapture_AbandonsWhenPodReplaced(t *testing.T) { + env := newWatcherEnv(t) + w := env.watcher() + + stale := fakePod("uid-old", "vllm-8b", map[string]string{DefaultCaptureLabel: "true"}, true) + replacement := fakePod("uid-new", "vllm-8b", map[string]string{DefaultCaptureLabel: "true"}, true) + w.KubeClient = fake.NewSimpleClientset(replacement) + + if _, ok := w.refreshPodForCapture(context.Background(), stale, w.logger().WithField("t", "x")); ok { + t.Error("a pod replaced during the warmup delay must not be captured under the old identity") + } +} + +// The re-read is an accuracy improvement, not a new dependency: if the API +// read fails, capture proceeds on the copy we already have rather than +// dropping the capture entirely. +func TestRefreshPodForCapture_FallsBackWhenReadFails(t *testing.T) { + env := newWatcherEnv(t) + w := env.watcher() // empty clientset -> NotFound + + stale := withContainerID( + fakePod("uid-1", "vllm-8b", map[string]string{DefaultCaptureLabel: "true"}, true), + "onlycontainerid000") + + got, ok := w.refreshPodForCapture(context.Background(), stale, w.logger().WithField("t", "x")) + if !ok { + t.Fatal("a failed re-read must not abandon the capture") + } + if got != stale { + t.Error("failed re-read should fall back to the pre-warmup copy") + } +} diff --git a/src/compute-plane-services/nvsnap/internal/server/delete_checkpoint_test.go b/src/compute-plane-services/nvsnap/internal/server/delete_checkpoint_test.go index a843a1fc3b..125351b415 100644 --- a/src/compute-plane-services/nvsnap/internal/server/delete_checkpoint_test.go +++ b/src/compute-plane-services/nvsnap/internal/server/delete_checkpoint_test.go @@ -321,6 +321,109 @@ func TestDeleteCheckpoint_HTTP_204WhenCatalogRowExists(t *testing.T) { } } +// A cascade where every tier fails leaves AnySuccess false, same as a +// checkpoint that genuinely does not exist. Answering 404 for both told the +// caller the dump was gone while it was still on disk -- the nvsnap#736 +// failure mode wearing a different status code. The retained catalog row is +// what distinguishes them. +func TestDeleteCheckpoint_HTTP_500WhenEveryTierFails(t *testing.T) { + s := newTestServerWithCatalog(t) + fb, fbSrv := newFakeBlobstore() + fb.failHash["agent-dead"] = http.StatusInternalServerError + defer fbSrv.Close() + s.config.BlobstoreURL = fbSrv.URL + + if err := s.catalog.UpsertCheckpoint(&db.Checkpoint{ + ID: "ck-allfail", + Hash: "aaa111aaa111aaa111", + CheckpointPath: "/var/lib/nvsnap/checkpoints/agent-dead", + CreatedAt: time.Now().UTC(), + }); err != nil { + t.Fatalf("seed: %v", err) + } + + req := httptest.NewRequest(http.MethodDelete, "/api/v1/checkpoints/ck-allfail", http.NoBody) + rr := httptest.NewRecorder() + s.router.ServeHTTP(rr, req) + + if rr.Code != http.StatusInternalServerError { + t.Errorf("got %d, want 500; body=%s", rr.Code, rr.Body.String()) + } + if _, err := s.catalog.GetCheckpoint("ck-allfail"); err != nil { + t.Errorf("catalog row must survive so the delete can be retried; get: %v", err) + } +} + +// The by-hash endpoint runs the same cascade and so owes the same answer. +// It only consulted AnySuccess, so a partial cascade reported 204 while the +// row it deliberately kept was still there. +func TestDeleteCheckpointByHash_HTTP_500OnPartialCascade(t *testing.T) { + s := newTestServerWithCatalog(t) + fb, fbSrv := newFakeBlobstore() + fb.failHash["agent-dead"] = http.StatusInternalServerError + defer fbSrv.Close() + s.config.BlobstoreURL = fbSrv.URL + + const hash = "bbb222bbb222bbb222" + if err := s.catalog.UpsertCheckpoint(&db.Checkpoint{ + ID: "ck-hash", + Hash: hash, + Namespace: "ns1", + PodName: "p1", + CheckpointPath: "/var/lib/nvsnap/checkpoints/agent-dead", + CreatedAt: time.Now().UTC(), + }); err != nil { + t.Fatalf("seed: %v", err) + } + + req := httptest.NewRequest(http.MethodDelete, "/api/v1/checkpoints/by-hash/"+hash, http.NoBody) + rr := httptest.NewRecorder() + s.router.ServeHTTP(rr, req) + + if rr.Code != http.StatusInternalServerError { + t.Errorf("got %d, want 500; body=%s", rr.Code, rr.Body.String()) + } + if rows, err := s.catalog.ListByHash(hash); err != nil || len(rows) == 0 { + t.Errorf("catalog row must survive a partial by-hash cascade (rows=%d, err=%v)", len(rows), err) + } +} + +// A catalog delete that itself errors is the one tier whose failure the +// cascade cannot route around: it appended to Errors but left +// CatalogRetained false, so a run where some other tier had already +// succeeded still answered 204 with the row in place. +func TestCascadeDelete_CatalogDeleteErrorSetsRetained(t *testing.T) { + s := newTestServerWithCatalog(t) + s.config.BlobstoreURL = "" + + // An empty Hash is what isolates this: every other catalog touch in the + // cascade (sibling lookup, L2 PVCs, snapshots, leases, capture CM) is + // gated on row.Hash != "", so DeleteCheckpoint is the only catalog call + // that runs. Closing the DB therefore fails that call and nothing else, + // leaving result.Errors empty until it -- which means the pre-delete + // "errors already happened" early return cannot be what sets the flag. + row := &db.Checkpoint{ + ID: "ck-catalog-err", + CreatedAt: time.Now().UTC(), + } + if err := s.catalog.UpsertCheckpoint(row); err != nil { + t.Fatalf("seed: %v", err) + } + if err := s.catalog.Close(); err != nil { + t.Fatalf("close catalog: %v", err) + } + + res := s.cascadeDeleteCheckpoint(context.Background(), row.ID, "agent-x", row) + + if len(res.Errors) != 1 || !strings.Contains(res.Errors[0], "DeleteCheckpoint") { + t.Fatalf("want exactly one error, from the catalog delete; got %v", res.Errors) + } + if !res.CatalogRetained { + t.Error("CatalogRetained = false; a failed catalog delete leaves the row, " + + "so callers must not be told the checkpoint is gone") + } +} + // ---------------- helpers ---------------- func splitHostPort(s string) (host, port string, ok bool) { @@ -481,11 +584,11 @@ func TestCascadeDelete_RetainsCatalogRowWhenATierFails(t *testing.T) { t.Fatal("expected a tier error from the failing blobstore") } if res.CatalogRows != 0 { - t.Errorf("CatalogRows = %d, want 0 — the row must survive a failed tier delete", res.CatalogRows) + t.Errorf("CatalogRows = %d, want 0; the row must survive a failed tier delete", res.CatalogRows) } // The row itself must still be readable, or the dump is unreachable. if got, err := s.catalog.GetCheckpoint("ck-1"); err != nil || got == nil { - t.Errorf("catalog row was deleted despite a tier failure (get: %v) — dump is now orphaned", err) + t.Errorf("catalog row was deleted despite a tier failure (get: %v); dump is now orphaned", err) } if res.Status() != "partial" { t.Errorf("Status() = %q, want partial", res.Status()) @@ -510,6 +613,6 @@ func TestCascadeDelete_DeletesCatalogRowWhenAllTiersSucceed(t *testing.T) { t.Fatalf("clean cascade should have no errors; got %v", res.Errors) } if res.CatalogRows == 0 { - t.Error("CatalogRows = 0 — a clean cascade must delete the row") + t.Error("CatalogRows = 0; a clean cascade must delete the row") } } diff --git a/src/compute-plane-services/nvsnap/internal/server/server.go b/src/compute-plane-services/nvsnap/internal/server/server.go index ea0a5fd429..0b45d37a46 100644 --- a/src/compute-plane-services/nvsnap/internal/server/server.go +++ b/src/compute-plane-services/nvsnap/internal/server/server.go @@ -996,7 +996,11 @@ func (s *Server) deleteCheckpoint(w http.ResponseWriter, r *http.Request) { result := s.cascadeDeleteCheckpoint(ctx, id, agentID, row) - if !result.AnySuccess { + // Genuine 404: nothing matched anywhere AND nothing failed. A cascade + // that failed on every tier also leaves AnySuccess false, but there the + // checkpoint still exists -- answering 404 would claim it is gone. + // CatalogRetained separates the two. + if !result.AnySuccess && !result.CatalogRetained { s.writeError(w, http.StatusNotFound, "checkpoint not found in any tier (agent, peers, blobstore, catalog)") return } @@ -1053,10 +1057,17 @@ func (s *Server) deleteCheckpointByHash(w http.ResponseWriter, r *http.Request) Status: result.Status(), Message: result.Summary(), }) - if !result.AnySuccess { + if !result.AnySuccess && !result.CatalogRetained { s.writeError(w, http.StatusNotFound, "no artifacts found for hash "+hash) return } + // Same contract as deleteCheckpoint: a retained catalog row means the + // checkpoint is still there, so this is not a 204 (nvsnap#736). + if result.CatalogRetained { + s.writeError(w, http.StatusInternalServerError, + "checkpoint partially deleted; catalog row retained for retry: "+result.Summary()) + return + } w.WriteHeader(http.StatusNoContent) } @@ -1212,6 +1223,9 @@ func (s *Server) cascadeDeleteCheckpoint(ctx context.Context, id, agentID string if row != nil && row.Hash != "" { n, err := s.catalog.DeleteByHash(row.Hash) if err != nil { + // The row survived the delete just as deliberately as in the + // branch above, so callers must hear about it the same way. + result.CatalogRetained = true result.Errors = append(result.Errors, fmt.Sprintf("catalog DeleteByHash(%s): %v", row.Hash[:12], err)) } else { @@ -1222,6 +1236,7 @@ func (s *Server) cascadeDeleteCheckpoint(ctx context.Context, id, agentID string } } else if row != nil { if err := s.catalog.DeleteCheckpoint(id); err != nil { + result.CatalogRetained = true result.Errors = append(result.Errors, fmt.Sprintf("catalog DeleteCheckpoint(%s): %v", id, err)) } else {