From 6083c5434a4f39aa719a15827da3c8666e5bdd51 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Wed, 22 Jul 2026 16:36:19 +0500 Subject: [PATCH 1/2] =?UTF-8?q?fix(delete):=20verify=20the=20host-data=20w?= =?UTF-8?q?ipe=20before=20printing=20=E2=9C=94?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit removeHostDataDir did os.RemoveAll and returned nil without confirming the tree was actually gone; the caller then printed "✔ Removed local tracebloc data and config". A nil RemoveAll is not proof of absence (racing writer, mount, masked partial failure), so offboard could claim a clean slate it didn't achieve — the RFC-0003 offboard-hygiene gap. Now removeHostDataDir stats the dir after RemoveAll and treats "still present" (or an unexpected stat error) as a failure, so the caller prints the warn + manual-rm hint instead of ✔. Adds an osStat seam + a test proving delete does NOT claim success on an unverified wipe. Closes #388. Refs tracebloc/client#367, backend#1151, tracebloc/cli#366. Co-Authored-By: Claude Opus 4.8 --- internal/cli/delete.go | 15 ++++++++++++- internal/cli/delete_test.go | 45 +++++++++++++++++++++++++++++++++++-- 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/internal/cli/delete.go b/internal/cli/delete.go index a88e2131..cf27d6a5 100644 --- a/internal/cli/delete.go +++ b/internal/cli/delete.go @@ -31,6 +31,7 @@ var ( var ( osExecutable = os.Executable osRemoveAll = os.RemoveAll + osStat = os.Stat ) // deleteOpts bundles the `tracebloc delete` flags. @@ -382,7 +383,19 @@ func removeHostDataDir() error { if err != nil { return err } - return osRemoveAll(dir) + if err := osRemoveAll(dir); err != nil { + return err + } + // Verify the directory is actually gone before the caller prints "✔ Removed". + // A nil RemoveAll is not proof the tree is absent — a racing writer, a mount, + // or a masked partial failure can leave it present — and claiming a clean wipe + // we didn't achieve is exactly the offboard-hygiene gap RFC-0003 flags. + if _, statErr := osStat(dir); statErr == nil { + return fmt.Errorf("%s still present after removal", dir) + } else if !errors.Is(statErr, os.ErrNotExist) { + return fmt.Errorf("verifying removal of %s: %w", dir, statErr) + } + return nil } // hostDataDirDisplay is the data dir for a user-facing hint; falls back to the diff --git a/internal/cli/delete_test.go b/internal/cli/delete_test.go index 9b9948a0..48ec1dae 100644 --- a/internal/cli/delete_test.go +++ b/internal/cli/delete_test.go @@ -39,6 +39,7 @@ type fakeNodeboot struct { pruneErr error removedPaths []string removeErr map[string]error // path → error to return from osRemoveAll + statPresent bool // when true, osStat reports the data dir still exists after rm executable string executableErr error // Kubeconfig/context the uninstall seam was handed — so a test can prove the @@ -50,7 +51,7 @@ type fakeNodeboot struct { func (f *fakeNodeboot) install(t *testing.T) { t.Helper() origU, origT, origP := uninstallChart, teardownCluster, pruneImages - origExe, origRm := osExecutable, osRemoveAll + origExe, origRm, origStat := osExecutable, osRemoveAll, osStat uninstallChart = func(_ context.Context, ns, kubeconfig, kubeContext string) error { f.calls = append(f.calls, "uninstall:"+ns) f.uninstallKubeconfig, f.uninstallContext = kubeconfig, kubeContext @@ -78,9 +79,19 @@ func (f *fakeNodeboot) install(t *testing.T) { } return nil } + // osStat backs removeHostDataDir's verify-before-success. Default: the data dir + // is gone (ErrNotExist) so a clean wipe reports success. statPresent flips it to + // "still there" so a test can prove the CLI does NOT claim ✔ on an unverified wipe. + osStat = func(path string) (os.FileInfo, error) { + f.calls = append(f.calls, "stat:"+path) + if f.statPresent { + return nil, nil + } + return nil, os.ErrNotExist + } t.Cleanup(func() { uninstallChart, teardownCluster, pruneImages = origU, origT, origP - osExecutable, osRemoveAll = origExe, origRm + osExecutable, osRemoveAll, osStat = origExe, origRm, origStat }) } @@ -402,6 +413,36 @@ func TestDelete_WipeFails_StillClearsPointer(t *testing.T) { } } +// A nil RemoveAll is not proof the wipe happened: if the data dir is still present +// afterwards, delete must NOT print "✔ Removed" — it must warn and flag degraded, +// so offboard never claims a clean slate it didn't achieve (RFC-0003). +func TestDelete_WipeUnverified_DoesNotClaimSuccess(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + _, _ = w.Write([]byte(`[{"id":5,"first_name":"gpu-box-01","namespace":"gpu-box-01","status":0}]`)) + case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke"): + w.WriteHeader(http.StatusOK) + } + }) + setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01") + exe := filepath.Join(t.TempDir(), "tracebloc") + // osRemoveAll returns nil (no removeErr), but osStat reports the dir still present. + fn := &fakeNodeboot{executable: exe, statPresent: true} + fn.install(t) + + var out bytes.Buffer + if err := runDelete(context.Background(), ui.New(&out), nil, deleteOpts{yes: true}); err != nil { + t.Fatalf("offboard: %v", err) + } + if strings.Contains(out.String(), "Removed local tracebloc data and config") { + t.Errorf("must NOT claim a clean wipe when the dir is still present:\n%s", out.String()) + } + if !strings.Contains(out.String(), "Couldn't remove local data") { + t.Errorf("expected the unverified-wipe warning, got:\n%s", out.String()) + } +} + // When a teardown step leaves real state behind, the closing line must NOT claim a // clean offboard — it should flag that some cleanup didn't complete. func TestDelete_TeardownFailure_HonestClosing(t *testing.T) { From 6227efc8fa46c525f3b5f2418b495bb227e55be6 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Wed, 22 Jul 2026 16:38:32 +0500 Subject: [PATCH 2/2] test(delete): regenerate copy-catalog golden for the two verify strings The wipe-verify error messages are user-visible (surfaced via the "Couldn't remove local data (%v)" warn), so zz-all-strings.golden picks them up. Co-Authored-By: Claude Opus 4.8 --- internal/cli/testdata/golden/zz-all-strings.golden | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 009e7131..708a194e 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -48,6 +48,7 @@ screen. %s/%d are runtime placeholders. "%s of %s GiB" "%s of %s cores" "%s requires CLIENT_WRITE permission" +"%s still present after removal" "%s unreachable: %v" "%s · %d" "%s · Online%s" @@ -579,6 +580,7 @@ screen. %s/%d are runtime placeholders. "unavailable" "unknown command %q for %q" "values:" +"verifying removal of %s: %w" "waiting for ingestor Pod: %w" "waiting for staging-cleanup pod: %w" "waiting for teardown pod: %w"