Skip to content

feat(telemetry): the CLI now drains the installer's spool too (backend#2217) - #545

Merged
LukasWodka merged 1 commit into
developfrom
feat/2217-installer-spool-drain
Aug 21, 2026
Merged

feat(telemetry): the CLI now drains the installer's spool too (backend#2217)#545
LukasWodka merged 1 commit into
developfrom
feat/2217-installer-spool-drain

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Completes backend#2217. The CLI half landed in #542; this is the installer half.

Built as option (b), because the ticket's assumption doesn't hold

#2217 says "convert at the seam and POST", which presumes the installer can authenticate. It cannot. The ingest endpoint requires Token/Bearer under IsAuthenticatedEdge; the installer holds TRACEBLOC_CLIENT_ID/TRACEBLOC_CLIENT_PASSWORD — a provisioning pair with no exchange for a token — and never reads the CLI's config (git grep -E '\.tracebloc/config\.json|TRACEBLOC_CONFIG_DIR' -- scripts/ → zero hits).

Having the installer read ~/.tracebloc/config.json also delivers nothing for the failures that matter most: validate_config and early_data_dir_guard run before provisioning, so no token exists on disk when those events are written — and those are precisely what the $TMPDIR fallback was built to preserve. Shipping that and calling it "the installer now delivers telemetry" would be false for the highest-value cases.

So the CLI carries them. It already owns the token and, since #542, the spool, drain loop and OTLP mapping. This adds one file to read and leaves the installer with no credential handling at all — consistent with that file's posture.

The unpredictable path didn't need the index file I expected

I flagged on the ticket that _telemetry_fallback_spool's mktemp .../tracebloc-telemetry-XXXXXX would need a shared index or a deterministic name. Neither: the name is unpredictable but the pattern is fixed, so a glob over $TMPDIR / $HOME / /tmp finds them. The installer needs no change and there is no shared state to keep in step. Both candidate shapes are covered — the predictable $HOST_DATA_DIR/telemetry/pending.jsonl and the globbed fallbacks — and the same directory reached twice via $TMPDIR and $HOME is de-duplicated, since a duplicate would send one install outcome twice in a batch.

The part to review: per-record environment filtering

Our spool is partitioned by env in the filename; the installer's is not. Its records carry whatever CLIENT_ENV that run used, so forwarding blind would post a prod-labelled install failure to whichever backend this invocation points at — the same label-versus-destination leak as #542's second Bugbot finding, arriving by a different route.

So a record ships only when its own deployment.environment matches this run's. The rest stay put for a later invocation against that env. A record with no environment is never forwarded — the contract omits an attribute rather than sending it empty, so absent means the emitter couldn't resolve one — but it is never dropped either, because it is still evidence.

Their files are touched only on a path that consumed them, never on delivery failure and never when unauthenticated. A file we took nothing from isn't rewritten at all.

Test plan

make check green (vet, full suite, fmt, file-budget, style, tool-pins). make lint clean (errcheck, ineffassign, misspell, staticcheck). 16 mutations across the three suites, all killed.

The nine new ones, each asserting its anchor applied:

mutationtest
env filter removed — forwards every environment..._OnlyTakesThisEnvironment
records with no environment are forwarded..._LeavesRecordsWithNoEnvironment
drain cap ignored..._RespectsTheDrainCap
the mktemp fallback glob is dropped..._FindsBothShapes
the glob widens to * and catches unrelated files..._FindsBothShapes
duplicate-directory guard removed..._DoesNotDuplicateOneDirectory
a file we took nothing from is rewritten anyway..._SkipsAFileWithNothingForUs
installer files cleared on a failed delivery..._LeavesTheInstallerSpoolAloneOnFailure
installer records not carried at all..._CarriesTheInstallersRecords

One of them earned its keep by finding a vacuous test of mine.TestInstallerRecordsSkipsAFileWithNothingForUs originally compared the file's bytes before and after — and a rewrite of unchanged records produces identical bytes, so it passed under the very mutation it existed to catch. It now asserts the real contract (the file never enters remainder), and the mutation kills it. Worth naming because "assert the mutation anchor applied" would not have caught this one; only running the mutation did.

Two mutations also had to be rewritten because they broke the build rather than the test — an inconclusive mutation and a real kill look identical in a log unless you check.

Not in scope

The Windows installer emits nothing at all to drain — install-k8s.ps1 has zero telemetry references. Filed separately as backend#2268.

🤖 Generated with Claude Code


Note

Medium Risk
Touches the CLI telemetry drain path and rewrites another component’s spool files on the exit of every command. Environment filtering and consume-only-on-success are the main correctness risks (wrong-backend leak or lost install evidence).

Overview
The CLI now delivers installer telemetry that previously had no route: deliver piggybacks matching installer spool records onto the existing authenticated ingest POST.

It locates both $HOST_DATA_DIR/telemetry/pending.jsonl and tracebloc-telemetry-* fallbacks under $TMPDIR/$HOME//tmp, then forwards only records whose deployment.environment matches this run (capped at 10, after the CLI’s own events). Foreign-env and unlabeled records stay on disk; installer files are rewritten or deleted only after a successful consume, never on failure or when unsigned-in.

Reviewed by Cursor Bugbot for commit 8a92647. Bugbot is set up for automated code reviews on this repo. Configure here.

…d#2217)
Completes backend#2217. The CLI half landed in #542; this is the installer half,
built as option (b) rather than (a).
WHY NOT (a), THE ROUTE THE TICKET ASSUMED. #2217 says "convert at the seam and
POST", which presumes the installer can authenticate. It cannot: the ingest
endpoint needs `Token`/`Bearer` under `IsAuthenticatedEdge`, and the installer
holds `TRACEBLOC_CLIENT_ID`/`TRACEBLOC_CLIENT_PASSWORD` -- a provisioning pair
with no exchange for a token -- and never reads the CLI's config (zero hits across
`scripts/`). Having it read `~/.tracebloc/config.json` would also deliver NOTHING
for the failures that matter most: `validate_config` and `early_data_dir_guard`
run before provisioning, so no token exists on disk when those events are written,
and those are exactly what the installer's `$TMPDIR` fallback was built to
preserve.
So the CLI carries them. It already owns the token and, since #542, the spool,
drain loop and OTLP mapping -- this adds one more file to read and leaves the
installer with no credential handling at all.
THE UNPREDICTABLE FALLBACK PATH TURNED OUT NOT TO NEED AN INDEX FILE. I had
expected to need one; `_telemetry_fallback_spool` uses
`mktemp .../tracebloc-telemetry-XXXXXX`, so the NAME is unpredictable but the
PATTERN is fixed. A glob over $TMPDIR / $HOME / /tmp finds them, the installer
needs no change, and there is no shared state to keep in step.
EVERY RECORD IS FILTERED BY ITS OWN ENVIRONMENT, and this is the part to review.
Our spool is partitioned by env in the FILENAME; the installer's is not, and its
records carry whatever CLIENT_ENV that run used. Forwarding blind would post a
prod-labelled install failure to whichever backend this invocation points at --
the same leak #542's second finding was about. A record ships only when its
`deployment.environment` matches this run's; the rest stay for a later invocation
against that env. A record with NO environment is never forwarded (the contract
omits rather than empties, so absent means unresolvable) but is also never
dropped: it is still evidence.
Their files are touched only on a path that CONSUMED them, and never on failure
or when unauthenticated. A file we took nothing from is not rewritten at all.
Verified: `make check` green, `make lint` clean, 16 mutations across the three
suites all killed. One of the new nine earned its keep: it proved
`TestInstallerRecordsSkipsAFileWithNothingForUs` VACUOUS -- it compared file
bytes, and a rewrite of unchanged records produces identical bytes, so it passed
under the mutation it existed to catch. It now asserts the real contract (the file
never enters `remainder`) and the mutation kills it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodkaLukasWodka self-assigned this Aug 20, 2026

@saqlainsyed007saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the two correctness-sensitive parts and the design is right — I'll approve once CI is green.

  • Per-record env filtering (the wrong-backend risk).installerRecords forwards a record only when rec.Resource["deployment.environment"] == env, and everything else — other envs, over-cap, and crucially no-environment records — stays in remainder for a later invocation. That's the same label-vs-destination invariant #542's per-env spool established, applied per-record here since the installer's spool isn't env-partitioned in the filename. Treating an absent env as "leave in place, never drop" is the right call: the contract omits the attribute rather than sending it empty, so absent means unresolved, not global.
  • Consume-only-on-success (the lost-evidence risk).clearInstallerRecords rewrites each source file with only the remainder, and the mutation table's LeavesTheInstallerSpoolAloneOnFailure (files cleared on a failed delivery reddens) is exactly the guard I'd want — an install failure's telemetry must not be discarded undelivered. writeSpool removing the file when keep is empty is correct.
  • The glob-not-index approach is clean: fixed mktemp pattern over $HOST_DATA_DIR/$TMPDIR/$HOME//tmp, de-duped so the same dir reached via $TMPDIR and $HOME doesn't double-send.

Nice catch on your own vacuous test — SkipsAFileWithNothingForUs comparing bytes before/after would pass under the mutation it existed to catch (an unchanged rewrite is byte-identical); asserting the file never enters remainder is the real contract. That's the house bar working.

Also agree with building it as option (b): the installer genuinely can't authenticate (provisioning pair, no token, never reads the CLI config), and the pre-provisioning validate_config/early_data_dir_guard events — the highest-value ones — have no token on disk anyway, so routing them through the CLI that already owns the token and (since #542) the spool is the honest shape.

CI is still running — I'll confirm the full gate and approve once it's green.

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

@saqlainsyed007CI is green: 25 passed, 0 failed, 0 pending (3 skipped). Full gate across all platforms including the kind e2e and the shell job.

Thanks for checking the two correctness-sensitive parts specifically rather than the diff as a whole — those were the ones I'd have wanted a second pair of eyes on, and your reading of the absent-environment case is exactly the reasoning in the code: the contract omits rather than empties, so absent means unresolved, not global, and dropping it would discard evidence we simply can't route yet.

Ready when you are.

@saqlainsyed007saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. CI is green on the same head I reviewed (8a926470), so the verification from last pass stands:

  • Per-record env filtering forwards a record only when its deployment.environment matches this run's, leaving other-env / over-cap / no-env records in remainder — the #542 label-vs-destination invariant applied per-record, since the installer's spool isn't env-partitioned by filename.
  • Consume-only-on-success: clearInstallerRecords rewrites each source file with only the remainder, guarded by the LeavesTheInstallerSpoolAloneOnFailure mutation so an install failure's telemetry can't be discarded undelivered — that test now passes in green CI.
  • The glob-not-index drain (fixed mktemp pattern over $HOST_DATA_DIR/$TMPDIR/$HOME//tmp, de-duped) and the option-(b) routing (installer can't authenticate; its pre-provisioning events have no token on disk; the CLI already owns token + spool) are the right calls.

No open threads, no conflict. This closes #2217 end-to-end — clean work, and the self-caught vacuous byte-compare test is the house bar doing its job.

@LukasWodka
LukasWodka merged commit 858ad3b into developAug 21, 2026
28 checks passed
@LukasWodka
LukasWodka deleted the feat/2217-installer-spool-drain branch August 21, 2026 06:37
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

/fr-pass

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LukasWodka@saqlainsyed007