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

[saadqbal]

Fixes the only failing assertion in Offboard teardown (k3d), red on develop
since c246912 and the last blocker on the develop → staging promotion mirror.

Ticket: backend#2314.

It is not the revoke, and not a fail-open env default

The job's log leads with a prod URL and a refused connection, which reads like the
test targeting production. It isn't:

• The test is already offline by design — it routes all egress through a dead
  proxy (127.0.0.1:1) on purpose, and asserts the CLI's best-effort revoke
  wording. That assertion passes. So do the Helm uninstall, the cluster
  delete, the foreign-tb guard and the self-remove.
• The prod host comes from the test's own fixture ("current_env": "prod" in
  writeConfig), not from api.BaseURL's unknown-env default. $CLIENT_ENV is
  ignored once current_env is set, so setting it in the job would have changed
  nothing.
• No request ever left the runner.

The one failing line is the config-dir wipe:

delete_e2e_test.go:212: config dir /tmp/TestE2E_DeleteTeardown.../003
should be wiped, stat err = <nil>

…while the same run's captured output shows ✔ Removed local tracebloc data and config.. Both are true: the offboard removes the tree and verifies it gone, and
something puts it back afterwards.

What actually happens

main.go emits the command-outcome event after the command tree returns, and
the telemetry spool lives at <config.Dir()>/telemetry/pending-<env>.jsonl —
inside the tree the offboard just deleted. Two defects combine:

1. writeSpool called MkdirAllbefore its len(events) == 0 early return,
   so it created the directory even with nothing to write, on its way to deleting
   a file inside it. This is why the tree came back on the delivered path too
   — an online user's offboard leaves an empty ~/.tracebloc/telemetry/.
2. Nothing told the exit-path write that this invocation had deliberately removed
   local state, so on the undelivered path it wrote a real event record 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.

So every tracebloc delete left something behind — an empty dir online, and a
dir holding an undeliverable telemetry record offline, after the user asked for
local data and config to be removed. Privacy/hygiene rather than a credential
leak (the record carries no token, argv or paths by design), but the offboard's
promise was not kept.

The fix

removeHostDataDir returns the directory it removed; the offboard records it, and
writeSpool drops any write landing inside it.

It records the path, not a boolean, and that mattered twice: an unscoped
"telemetry is off now" flag silences writes the offboard never touched, and is
permanently sticky in a test binary — delete's own unit tests drive the real
offboard, and three unrelated spool tests failed exactly that way while this was
being written. A path is naturally test-isolated and answers the narrower
question.

Network delivery 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 same two fixes also cover the installer-spool drain, which reaches
writeSpool through clearInstallerRecords and defaults to the same tree.

Verification

• make check — vet, full suite, file-budget, check-style, check-tool-pins all
  green. fmt-check flags only untracked .claude/worktrees/** debris from
  another local session; both formatters are clean on every file in this diff.
• go test ./... green; go test -race green on the telemetry + delete tests.
• actionlint clean on the workflow change.
• Load-bearing proof: reverting each half independently turns the matching new
  test red —
  TestWriteSpoolDoesNotCreateTheDirWhenThereIsNothingToWrite for the MkdirAll
  ordering, TestAWipedHostStateStopsTheSpoolComingBack and
  TestOffboardLeavesNothingBehindOnTheNoTokenPath for the recorded path.
• Not run locally: the k3d black-box e2e itself. It refuses to touch a
  pre-existing cluster named tracebloc and this machine has one, so it skips by
  design. CI is the proof — the paths probe matches internal/cli/delete*.go, so
  the job runs on this PR.

Second commit

The paths probe gated the offboard suite on internal/cli/delete*.go but not the
telemetry transport, so this bug could recur invisibly. Added
internal/cli/telemetry*.go — the same argument the filter's comment already
records for internal/ui after #367.

Follow-ups, deliberately not in this PR

api.BaseURL's unknown/empty → prod default is already tracked as backend#2171
and is a behaviour change with a wider blast radius, so it stays separate. While
confirming it I found the "kept in lock-step" claim in its docstring is already
false, and I've added the detail to #2171: the CLI and installer both fail open
to prod, but client-runtime defaults to dev and controller.pyrefuses an
unknown CLIENT_ENV outright (SystemExit(1)), while its own
resource_monitor.py falls back to prod — the three components do not agree, and
client-runtime disagrees with itself.

---

Note

Medium Risk
Touches offboard hygiene and the telemetry spool path: a dropped outcome event is intentional, but a latch/path-match bug could silence unrelated spool writes or leave local state behind.

Overview
Stops tracebloc delete from resurrecting ~/.tracebloc after it has already been wiped. Command-outcome telemetry runs after the command returns, and the spool lives inside that tree, so a later writeSpool undid the offboard (empty dir on successful delivery, undeliverable record when there is no token).

The offboard now records the removed path; spool writes under it are dropped. Empty-spool writes no longer MkdirAll before deleting a missing file. The e2e paths filter also includes internal/cli/telemetry*.go so this class of regression runs the teardown suite.

^{Reviewed by Cursor Bugbot for commit e08f2de. Bugbot is set up for automated code reviews on this repo. Configure here.}

[LukasWodka]

Verified the whole chain against develop and the branch. Every claim holds, and the design reasoning is better than the diff size suggests.

Defect 1 confirmed on develop — writeSpool really did MkdirAll on its way to deleting a file inside the directory it had just created:

dir:=filepath.Dir(path)
iferr:=os.MkdirAll(dir, 0o700); err!=nil { // <- runs firstreturnerr
}
iflen(events) ==0 {
err:=os.Remove(path) // <- then removes inside it

So the "empty ~/.tracebloc/telemetry/ after an online offboard" is real and not a theory. The reorder is correct, and the comment at the new len(events) == 0 branch says exactly why the ordering matters rather than leaving the next person to rediscover it.

Defect 2 confirmed — main.go emits after the command tree returns and the spool lives inside <config.Dir()>, so the wiped path is the one the offboard always takes: no token left, so deliver() spools every time.

The two design choices are the part worth keeping. Both are argued in the code, and both arguments are right:

"Does the dir exist?" is the wrong question: it doesn't, and writeSpool's job is to create it.

That's the crux. A presence check can't distinguish a fresh install from a wipe, and only the offboard knows which — so the offboard has to say. A latch is the right shape, not a shortcut.

IT HOLDS THE WIPED DIRECTORY, NOT JUST A BOOLEAN … three unrelated spool tests failed exactly that way while this was being written.

I checked the premise: delete_test.go:86 does drive the real offboard (removeHostDataDir's verify-before-success), so a process-global boolean latched in one test really would stay latched for every test after it. Reporting that you hit it, rather than just asserting the design was better, is what makes the reasoning trustworthy.

And the pitfall I went looking for is already handled. A path-scoped latch invites the classic prefix bug — suppressing writes to /tmp/foo-other because /tmp/foo was wiped. insideWipedHostDir uses filepath.Rel and tests the .. prefix on the relative result rather than strings.HasPrefix on the raw path:

rel, err:=filepath.Rel(root, path)
...returnrel=="."||!strings.HasPrefix(rel, "..")

with the comment noting Rel returns a ..-prefixed path and no error for anything outside root. And there's a test for it — telemetry_transport_test.go:298, "A sibling directory sharing a name PREFIX is the case a strings.HasPrefix…". That's the one place I expected to find a real defect and instead found the case named and pinned.

Two smaller things I'd have flagged if they were missing and am noting because they're right:

• The insideWipedHostDir early return doesn't attempt the os.Remove either — "Nothing to remove either: the file went with the directory." A guard that skipped the write but still tried the delete would have been a subtler version of the same bug.
• Scope honesty: "Network delivery is untouched — an online offboard still reports its outcome. Only the on-disk fallback is suppressed." Losing a telemetry record is the cheaper side of that trade and you say so rather than presenting it as free.

Not approving yet — Test and Cursor Bugbot are still pending. I'll approve next pass once green; nothing here needs anything from you.

[LukasWodka]

Approving — green on e08f2de2, the same commit my comment was on. 27 pass, 1 skipping, MERGEABLE, zero unresolved threads.

The check that matters most is green: Offboard teardown (k3d) = pass. That's the job this PR exists to fix, red on develop since c246912, so the fix is demonstrated by the thing it was failing rather than only argued in the description.

Everything I verified stands, and the two design choices are the reason I'd own this:

• MkdirAll really did run before the empty-events return on develop — I read it there. So the "empty ~/.tracebloc/telemetry/ after an online offboard" was real on the delivered path too, not just the spooling one.
• A latch rather than a presence check is correct for the stated reason: the directory is supposed to be absent and writeSpool's job is to create it, so "does it exist?" cannot distinguish a fresh install from a wipe. Only the offboard knows.
• A path rather than a boolean, and I confirmed the premise — delete_test.go:86 drives the real offboard, so a process-global flag latched in one test really would leak into every test after it. You hit that and said so.
• The sibling-prefix case is handled and pinned.filepath.Rel with the .. test on the relative result, plus telemetry_transport_test.go:298 naming exactly that hazard. That was the one place I went looking for a defect.

Nothing outstanding from me.

[LukasWodka]

/fr-pass