fix(delete): stop the exit-path telemetry write re-creating the wiped ~/.tracebloc (backend#2314)

.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/**'

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

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
+// (<config.Dir()>/telemetry/pending-<env>.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

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) {