diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 6b71166..5fb9839 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/**' diff --git a/internal/cli/delete.go b/internal/cli/delete.go index f232d88..81f3f3f 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 f3041fe..0ced415 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 34b9dbe..4bcb8ab 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) {