diff --git a/internal/cli/telemetry_installer_spool.go b/internal/cli/telemetry_installer_spool.go index 3d6f7e0..ff245e1 100644 --- a/internal/cli/telemetry_installer_spool.go +++ b/internal/cli/telemetry_installer_spool.go @@ -56,6 +56,31 @@ const ( // on the resource map rather than the record's own attributes. const resourceEnvironment = "deployment.environment" +// installerFallbackDirVars names every environment variable an installer twin may +// pick its pre-log fallback directory from, in the order the twins try them. +// +// TRANSCRIBED FROM THE PRODUCERS, and the residual is stated rather than hidden: +// they live in `tracebloc/client`, so nothing in this repo's CI can prove this +// list still agrees with them. Keeping them in step is a review rule until the +// two repos share a fixture. +// +// scripts/lib/telemetry.sh _telemetry_fallback_dir +// $TMPDIR -> $HOME -> /tmp +// scripts/lib/telemetry.ps1 Get-TelemetryFallbackSpool +// $USERPROFILE -> $HOME -> [IO.Path]::GetTempPath(), i.e. $TMP / $TEMP +// +// THE WINDOWS HALF WAS MISSING, and that was the whole defect (backend#2377). +// Windows does not set `TMPDIR` and usually does not set `HOME` — `USERPROFILE` +// is its home variable — so the search reduced to `/tmp`, a path that does not +// exist there. Every Windows pre-log install failure was written and never +// collected, which is exactly the class the fallback exists for: `validate_config` +// and `early_data_dir_guard` run before there is a log or a data dir, so the +// fallback file is their only record. +// +// The glob is unaffected: `tracebloc-telemetry-*` already matches the twin's +// `tracebloc-telemetry-.jsonl` as well as bash's suffix-less mktemp name. +var installerFallbackDirVars = []string{"TMPDIR", "HOME", "USERPROFILE", "TEMP", "TMP"} + // installerSpoolFiles returns every file that may hold installer records. // // Ordered predictable-first so a run with both delivers the data-dir spool before @@ -77,18 +102,29 @@ func installerSpoolFiles(getenv func(string) string) []string { out = append(out, filepath.Join(base, "telemetry", "pending.jsonl")) } - // 2. The pre-log fallback files. `_telemetry_fallback_dir` picks $TMPDIR, else - // $HOME, else /tmp — and disqualifies $TMPDIR when the installer is running - // from inside it. From here we cannot tell which it chose, so all three are - // candidates; a glob that matches nothing costs one syscall. + // 2. The pre-log fallback files. Each twin picks ONE directory out of its own + // chain (installerFallbackDirVars records both chains), and bash also + // disqualifies $TMPDIR when the installer is running from inside it. From + // here we cannot tell which it chose, so every candidate is searched; a + // glob that matches nothing costs one syscall. + candidates := make([]string, 0, len(installerFallbackDirVars)+1) + for _, name := range installerFallbackDirVars { + candidates = append(candidates, strings.TrimSpace(getenv(name))) + } + // bash's last resort, which is a literal rather than a variable. + candidates = append(candidates, "/tmp") + seen := map[string]bool{} - for _, dir := range []string{ - strings.TrimSpace(getenv("TMPDIR")), - strings.TrimSpace(getenv("HOME")), - "/tmp", - } { - dir = strings.TrimRight(dir, "/") - if dir == "" || seen[dir] { + for _, dir := range candidates { + if dir == "" { + continue + } + // Clean, not TrimRight("/"): it normalises a trailing separator on BOTH + // platforms, so `C:\Users\me\` and `C:\Users\me` dedupe as one directory. + // Two names for one directory would forward the same install outcome + // twice in a single batch. + dir = filepath.Clean(dir) + if seen[dir] { continue } seen[dir] = true diff --git a/internal/cli/telemetry_installer_spool_test.go b/internal/cli/telemetry_installer_spool_test.go index 9dedcac..2141549 100644 --- a/internal/cli/telemetry_installer_spool_test.go +++ b/internal/cli/telemetry_installer_spool_test.go @@ -128,6 +128,113 @@ func TestInstallerSpoolFilesDoesNotDuplicateOneDirectory(t *testing.T) { } } +// TestInstallerSpoolFilesFindsTheFallbackOnEveryHomeVariable pins backend#2377. +// +// THE NAMES BELOW ARE WRITTEN DOWN INDEPENDENTLY of installerFallbackDirVars, on +// purpose. A test that iterates the production list and feeds it back in is +// self-consistent and therefore blind: mistype `USERPROFILE` there and the test +// plants the same typo, finds the file, and goes green while Windows stays +// undrainable. These five literals come from the two producers +// (`client/scripts/lib/telemetry.sh` `_telemetry_fallback_dir` and +// `client/scripts/lib/telemetry.ps1` `Get-TelemetryFallbackSpool`), read there +// and copied here, so a drift between the list and the world is a failing test +// rather than agreement with itself. +// +// The Windows three are the regression: before #2377 the search was TMPDIR/HOME +// //tmp, and Windows sets neither of the first two. +func TestInstallerSpoolFilesFindsTheFallbackOnEveryHomeVariable(t *testing.T) { + for _, envVar := range []string{"TMPDIR", "HOME", "USERPROFILE", "TEMP", "TMP"} { + t.Run(envVar, func(t *testing.T) { + dir := t.TempDir() + // The ps1 twin's exact spelling, suffix included — the bash twin's + // mktemp name has no suffix, and the glob must cover both. + spool := filepath.Join(dir, "tracebloc-telemetry-3f9c1a.jsonl") + writeInstallerSpool(t, spool, installerEvent("prod", "win-run", 1)) + + // ONLY this variable is set. Nothing else may stand in for it, which + // is what makes the assertion about this variable and not about the + // environment as a whole. + getenv := func(k string) string { + if k == envVar { + return dir + } + return "" + } + + var found bool + for _, f := range installerSpoolFiles(getenv) { + if f == spool { + found = true + } + } + if !found { + t.Errorf("a fallback spool reachable only through $%s was not found; "+ + "an install failure written before the log exists is undeliverable", + envVar) + } + }) + } +} + +// TestInstallerFallbackDirVarsAreAllSearched is the totality half: every name the +// production list DECLARES must actually be read by installerSpoolFiles. +// +// It cannot see a name that is missing from the list — that is what the test +// above is for — but it does catch the opposite failure, a name added to the +// vocabulary and never wired into the loop, which would look like coverage while +// searching nothing. +func TestInstallerFallbackDirVarsAreAllSearched(t *testing.T) { + if len(installerFallbackDirVars) == 0 { + t.Fatal("installerFallbackDirVars is empty; the loop below would assert nothing") + } + for _, envVar := range installerFallbackDirVars { + dir := t.TempDir() + spool := filepath.Join(dir, "tracebloc-telemetry-Qq7") + writeInstallerSpool(t, spool, installerEvent("prod", "a", 1)) + getenv := func(k string) string { + if k == envVar { + return dir + } + return "" + } + var found bool + for _, f := range installerSpoolFiles(getenv) { + if f == spool { + found = true + } + } + if !found { + t.Errorf("$%s is declared in installerFallbackDirVars but is never searched", envVar) + } + } +} + +// A trailing separator must not turn one directory into two candidates. The +// pre-#2377 code trimmed only "/", so this also guards the filepath.Clean that +// replaced it. +func TestInstallerSpoolFilesDedupesATrailingSeparator(t *testing.T) { + dir := t.TempDir() + writeInstallerSpool(t, filepath.Join(dir, "tracebloc-telemetry-Ss2"), installerEvent("prod", "a", 1)) + getenv := func(k string) string { + switch k { + case "USERPROFILE": + return dir + case "TEMP": + return dir + string(filepath.Separator) + } + return "" + } + count := 0 + for _, f := range installerSpoolFiles(getenv) { + if strings.Contains(f, "tracebloc-telemetry-Ss2") { + count++ + } + } + if count != 1 { + t.Errorf("the same fallback file appears %d times; one install outcome would be sent twice", count) + } +} + // ─────────────────────────────────────────────── filtering by environment func TestInstallerRecordsOnlyTakesThisEnvironment(t *testing.T) {