From b8fdc928a35665f9a7f5d5666ab7cf5538203229 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 21 Aug 2026 17:39:35 +0500 Subject: [PATCH 1/2] fix(delete): stop the exit-path telemetry write re-creating the wiped ~/.tracebloc (backend#2314) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tracebloc delete` printed "✔ Removed local tracebloc data and config." and then put the directory back before the process exited, so the offboard's central promise was not kept. main.go emits the command-outcome event AFTER the command tree returns, and the telemetry spool lives at /telemetry/pending-.jsonl — inside the tree the offboard just removed. Two separate defects combined: * writeSpool called MkdirAll BEFORE its len(events) == 0 early return, so it created the directory even when it had nothing to write and was about to delete the spool file. This is why the tree came back on the DELIVERED path too, not just offline. * Nothing told the exit-path write that this invocation had deliberately removed local state, so on the undelivered path it wrote a real event file back into the wiped tree. That is the path the offboard always takes: the wipe takes the token with it, so deliver() finds no credential and spools. removeHostDataDir now returns the directory it removed and the offboard records it, so writeSpool drops any write that lands inside it. The recorded value is the PATH, not a boolean: a bare "telemetry is off" flag silences writes the offboard never touched, and is permanently sticky inside a test binary — three unrelated spool tests failed exactly that way while this was being written. Delivery over the network is untouched: an online offboard still reports its outcome. Only the on-disk fallback is suppressed, and a dropped telemetry record is the cheaper loss against silently undoing a wipe the user asked for. Regression coverage in telemetry_transport_test.go, verified load-bearing by reverting each half independently. This is the only failing assertion in the `Offboard teardown (k3d)` e2e, red on develop since c246912. Co-Authored-By: Claude Opus 5 --- internal/cli/delete.go | 27 ++++-- internal/cli/telemetry_transport.go | 74 +++++++++++++++- internal/cli/telemetry_transport_test.go | 106 +++++++++++++++++++++++ 3 files changed, 197 insertions(+), 10 deletions(-) diff --git a/internal/cli/delete.go b/internal/cli/delete.go index f232d888..81f3f3f1 100644 --- a/internal/cli/delete.go +++ b/internal/cli/delete.go @@ -331,11 +331,19 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er if o.keepData { p.Infof("Kept local data and config (~/.tracebloc); cleared the active-client pointer — --keep-data.") } else { - if derr := removeHostDataDir(); derr != nil { + if removed, derr := removeHostDataDir(); derr != nil { degraded = true p.Warnf("Couldn't remove local data (%v) — cleared the active-client pointer; "+ "remove the data by hand: rm -rf %s", derr, hostDataDirDisplay()) } else { + // The tree is gone and verified gone. Record it BEFORE printing the + // success line, so nothing later in this process can put it back and + // make that line false — specifically main.go's command-outcome + // telemetry, which runs after this command returns and whose spool + // lives inside the directory just removed (backend#2314). Only on + // the success branch: a failed removal leaves the tree in place, and + // a spool written into a tree that still exists is correct. + markHostStateWiped(removed) p.Successf("Removed local tracebloc data and config.") } } @@ -398,24 +406,29 @@ func renderOffboardSummary(p *ui.Printer, name string, keepData bool) { // $TRACEBLOC_CONFIG_DIR when set — the same resolution config.Dir uses). It goes // through the config package so a test's temp override is honored and the real // ~/.tracebloc is never touched in tests. -func removeHostDataDir() error { +// +// Returns the directory it removed, so the caller can tell the telemetry spool +// not to re-create it on the way out (backend#2314). The path is returned rather +// than re-resolved by the caller because config.Dir() is resolved here, and two +// resolutions of the same thing is how they come to disagree. +func removeHostDataDir() (string, error) { dir, err := config.Dir() if err != nil { - return err + return "", err } if err := osRemoveAll(dir); err != nil { - return err + 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) + 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 "", fmt.Errorf("verifying removal of %s: %w", dir, statErr) } - return nil + return dir, nil } // hostDataDirDisplay is the data dir for a user-facing hint; falls back to the diff --git a/internal/cli/telemetry_transport.go b/internal/cli/telemetry_transport.go index f3041fef..0ced4157 100644 --- a/internal/cli/telemetry_transport.go +++ b/internal/cli/telemetry_transport.go @@ -41,6 +41,7 @@ import ( "os" "path/filepath" "strings" + "sync/atomic" "time" "github.com/tracebloc/cli/internal/api" @@ -165,6 +166,62 @@ func readSpool(path string) []spooledEvent { return out } +// wipedHostDir records the directory THIS process deliberately removed — the +// `tracebloc delete` offboard on its default (no `--keep-data`) path. Spool +// writes under it are dropped for the rest of the process: see writeSpool. +// +// WHY A LATCH AND NOT A CHECK. main.go emits the command-outcome event AFTER the +// command tree returns, so the offboard has already deleted ~/.tracebloc by the +// time telemetry runs — and telemetry's spool lives INSIDE that tree +// (/telemetry/pending-.jsonl). "Does the dir exist?" is the +// wrong question: it doesn't, and writeSpool's job is to create it. The question +// is whether its absence is a fresh install (spool away) or a wipe the user just +// asked for (don't), and only the offboard knows which. So the offboard says so. +// +// backend#2314: `tracebloc delete` printed "✔ Removed local tracebloc data and +// config." and then the exit-path telemetry write re-created the tree behind it — +// empty when delivery succeeded, and holding an undeliverable event record when +// it didn't (after the wipe there is no token left to deliver with, so deliver +// takes its no-token spool branch every time). An explicit wipe that the CLI +// silently undoes on the way out is a broken promise, and a dropped telemetry +// record is unambiguously the cheaper loss. +// +// IT HOLDS THE WIPED DIRECTORY, NOT JUST A BOOLEAN, and that is about blast +// radius rather than precision for its own sake. A bare "telemetry is off now" +// flag is unscoped: it silences every later writeSpool in the process, including +// one for a path the offboard never touched. It is also permanently sticky in a +// test binary — `delete`'s own unit tests drive the real offboard, so a boolean +// latched there stays latched for every test that runs after it, and three +// unrelated spool tests failed exactly that way while this was being written. +// Recording the path answers the narrower and more useful question: is THIS +// spool inside the tree we removed? +// +// atomic.Value, not a plain string: the sink runs on the exit path while nothing +// else should still be writing, but "should" is not a guarantee and the race +// detector runs in CI. +var wipedHostDir atomic.Value // string + +// markHostStateWiped records the directory this process deliberately removed, so +// the exit-path telemetry write does not resurrect it. Called by the offboard. +func markHostStateWiped(dir string) { wipedHostDir.Store(dir) } + +// insideWipedHostDir reports whether path lies within a directory this process +// deliberately removed. +func insideWipedHostDir(path string) bool { + root, _ := wipedHostDir.Load().(string) + if root == "" { + return false + } + rel, err := filepath.Rel(root, path) + if err != nil { + // Different volumes, so not inside it. + return false + } + // filepath.Rel returns a ".."-prefixed path for anything outside root, and + // no error — so the prefix test, not the error, is what decides this. + return rel == "." || !strings.HasPrefix(rel, "..") +} + // writeSpool replaces the spool with events, keeping the NEWEST // telemetrySpoolMax and dropping the oldest past it. // @@ -174,20 +231,31 @@ func readSpool(path string) []spooledEvent { // file on the exit path of every command is a new way for telemetry to hang the // product, which is the one thing it may not do. func writeSpool(path string, events []spooledEvent) error { + // The offboard removed the tree this spool lives in. Writing here would + // re-create it — see wipedHostDir (backend#2314). Nothing to remove either: + // the file went with the directory. + if insideWipedHostDir(path) { + return nil + } if len(events) > telemetrySpoolMax { events = events[len(events)-telemetrySpoolMax:] } dir := filepath.Dir(path) - if err := os.MkdirAll(dir, 0o700); err != nil { - return err - } if len(events) == 0 { + // Nothing to write, so do NOT create the directory on the way to + // deleting a file inside it — the MkdirAll used to run before this + // branch, which re-created a wiped ~/.tracebloc/telemetry/ even on the + // delivered path, where the spool is being emptied rather than filled + // (backend#2314). err := os.Remove(path) if err != nil && !os.IsNotExist(err) { return err } return nil } + if err := os.MkdirAll(dir, 0o700); err != nil { + return err + } tmp, err := os.CreateTemp(dir, "pending-*.jsonl") if err != nil { return err diff --git a/internal/cli/telemetry_transport_test.go b/internal/cli/telemetry_transport_test.go index 34b9dbe1..4bcb8ab8 100644 --- a/internal/cli/telemetry_transport_test.go +++ b/internal/cli/telemetry_transport_test.go @@ -276,6 +276,112 @@ func TestWriteSpoolRemovesTheFileWhenEmpty(t *testing.T) { } } +// ────────────────────────── the spool must not resurrect a wiped ~/.tracebloc +// +// backend#2314. `tracebloc delete` wipes ~/.tracebloc and prints "✔ Removed +// local tracebloc data and config."; main.go then emits the command-outcome +// event, whose spool lives INSIDE that tree. Both halves of the fix are asserted +// here because they cover different user paths — the empty-spool write is what a +// SUCCESSFUL delivery does (dir came back empty), and the latch is what an +// undelivered event needs (dir came back holding a record). + +// wipedHostState records dir as the offboard's removed tree for one test and +// clears it afterwards. The clear is belt-and-braces: the recorded path is a +// per-test TempDir, so it cannot match another test's spool even if it leaked. +func wipedHostState(t *testing.T, dir string) { + t.Helper() + markHostStateWiped(dir) + t.Cleanup(func() { wipedHostDir.Store("") }) +} + +func TestInsideWipedHostDirOnlyMatchesTheTreeThatWasRemoved(t *testing.T) { + // A sibling directory sharing a name PREFIX is the case a strings.HasPrefix + // check on the raw paths gets wrong: /tmp/a-cfg2 is not inside /tmp/a-cfg. + root := t.TempDir() + wipedHostState(t, filepath.Join(root, "cfg")) + + for _, tc := range []struct { + path string + want bool + }{ + {filepath.Join(root, "cfg"), true}, + {filepath.Join(root, "cfg", "telemetry", "pending-prod.jsonl"), true}, + {filepath.Join(root, "cfg2", "telemetry", "pending-prod.jsonl"), false}, + {filepath.Join(root, "other", "telemetry", "pending-prod.jsonl"), false}, + } { + if got := insideWipedHostDir(tc.path); got != tc.want { + t.Errorf("insideWipedHostDir(%q) = %v, want %v", tc.path, got, tc.want) + } + } +} + +func TestAnUnrelatedSpoolStillWritesAfterAnOffboard(t *testing.T) { + // The scoping that matters in practice: one process offboarded one tree, and + // that must not silence telemetry for a path it never touched. + wipedHostState(t, filepath.Join(t.TempDir(), "gone")) + + path := filepath.Join(t.TempDir(), "telemetry", "pending.jsonl") + if err := writeSpool(path, []spooledEvent{event("run-elsewhere", 0)}); err != nil { + t.Fatalf("writeSpool: %v", err) + } + if got := readSpool(path); len(got) != 1 { + t.Errorf("a spool outside the wiped tree must still be written; got %d records", len(got)) + } +} + +func TestWriteSpoolDoesNotCreateTheDirWhenThereIsNothingToWrite(t *testing.T) { + dir := filepath.Join(t.TempDir(), "telemetry") + path := filepath.Join(dir, "pending.jsonl") + + // The delivered path: the batch landed, so the spool is written EMPTY. It + // must not mkdir on its way to removing a file that isn't there — that is + // what re-created a just-wiped ~/.tracebloc/telemetry/ for every online + // offboard. + if err := writeSpool(path, nil); err != nil { + t.Fatalf("writeSpool(nil): %v", err) + } + if _, err := os.Stat(dir); !os.IsNotExist(err) { + t.Errorf("writing an empty spool created %s; nothing was written, so nothing should be created (stat err = %v)", dir, err) + } +} + +func TestAWipedHostStateStopsTheSpoolComingBack(t *testing.T) { + dir := filepath.Join(t.TempDir(), "telemetry") + path := filepath.Join(dir, "pending.jsonl") + wipedHostState(t, dir) + + // A REAL event, i.e. the undelivered path — the one the offboard actually + // takes, because the wipe took the token with it and deliver then has + // nothing to post with. + if err := writeSpool(path, []spooledEvent{event("run-after-offboard", 0)}); err != nil { + t.Fatalf("writeSpool after a wipe must be a silent no-op, got: %v", err) + } + if _, err := os.Stat(dir); !os.IsNotExist(err) { + t.Errorf("the offboard removed this tree; telemetry re-created %s (stat err = %v)", dir, err) + } +} + +// TestOffboardLeavesNothingBehindOnTheNoTokenPath is the end-to-end shape of the +// CI failure: the exact branch `tracebloc delete` reaches on the way out, run +// against a config dir the offboard has already removed. +func TestOffboardLeavesNothingBehindOnTheNoTokenPath(t *testing.T) { + path := withTempConfigDir(t, "prod") + cfgDir := os.Getenv("TRACEBLOC_CONFIG_DIR") + + // Stand where main.go stands: the offboard has run, the tree is gone, and + // the token went with it — so deliver takes its no-token spool branch. + if err := os.RemoveAll(cfgDir); err != nil { + t.Fatalf("simulate the offboard wipe: %v", err) + } + wipedHostState(t, cfgDir) + + deliver(path, "http://127.0.0.1:1"+telemetryIngestPath, "", "prod", event("run-offboard", 0), time.Now()) + + if _, err := os.Stat(cfgDir); !os.IsNotExist(err) { + t.Errorf("`tracebloc delete` promised the tree was removed; the exit-path telemetry write put %s back (stat err = %v)", cfgDir, err) + } +} + // --------------------------------------------------------------- delivery func TestClassifyStatus(t *testing.T) { From e08f2de25223172ccc7b1c729f87d09f319b7418 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 21 Aug 2026 17:40:57 +0500 Subject: [PATCH 2/2] ci(e2e): the offboard suite depends on internal/cli/telemetry*.go too (backend#2314) The paths probe gates `Offboard teardown (k3d)` on the black-box run's dependency surface, and the telemetry transport was missing from it. That is the same gap the filter's own comment records for internal/ui after #367: the command-outcome event is emitted from main.go AFTER the offboard returns, and its spool lives inside the ~/.tracebloc the offboard just deleted, so a telemetry change re-created the wiped tree and broke the suite's config-dir assertion without touching delete.go. Co-Authored-By: Claude Opus 5 --- .github/workflows/e2e.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 6b711663..5fb9839e 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -73,6 +73,13 @@ jobs: # broke the copy assertions without touching delete.go. The glob # internal/cli/delete*.go deliberately excludes data_delete*.go # (`tracebloc data delete` is a different command, unit-tested). + # + # internal/cli/telemetry*.go is here for the same reason as + # internal/ui, and backend#2314 is the proof: the command-outcome + # event is emitted from main.go AFTER the offboard returns, and its + # spool lives inside the ~/.tracebloc the offboard just deleted — so + # a telemetry change re-created the wiped tree and broke the teardown + # suite's config-dir assertion without touching delete.go at all. filters: | e2e: - '.github/workflows/e2e.yml' @@ -82,6 +89,7 @@ jobs: - 'cmd/**' - 'test/integration/**' - 'internal/cli/delete*.go' + - 'internal/cli/telemetry*.go' - 'internal/nodeboot/**' - 'internal/api/**' - 'internal/config/**'