Skip to content

fix(telemetry): flush only at shutdown - #5294

Merged
Hmbown merged 1 commit into
mainfrom
codex/v095-telemetry-optout-fix
Aug 8, 2026
Merged

fix(telemetry): flush only at shutdown#5294
Hmbown merged 1 commit into
mainfrom
codex/v095-telemetry-optout-fix

Conversation

@Hmbown

@Hmbown Hmbown commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Summary

  • remove the impossible startup telemetry drain that could send current-session events before a mid-session opt-out
  • make shutdown the only structural flush point while preserving the bounded deadline and consent re-check
  • strengthen the process contract to prove an armed session sends nothing before shutdown, then prove an external opt-out suppresses the final batch
  • align the client and ingest documentation with the one-flush behavior

Root cause

buffer::arm() deliberately truncates every pre-existing telemetry buffer. The startup predicate therefore could not recover a prior session; it only observed install_or_upgrade and session_start events freshly queued by the current process. Under CI scheduling, that background task posted them before the test's external opt-out write.

This is the exact failure from protected exact-main CI run 31257458192, Ubuntu job 93102972086.

Verification

  • codewhale-telemetry unit tests: 45/45
  • exact mid-session process regression: 5/5 repeated runs
  • full TUI integration target: 263/263
  • strict workspace all-target/all-feature Clippy
  • source-structure budget: 685,101 <= 685,165
  • runtime-contract budget: 55/55 exact
  • dead-code budget: 453 exact
  • formatting, diff, reference scan, and co-author gate

No-Issue: this is an immediate v0.9.5 privacy/release-gate failure discovered only by the protected exact-main proof after PR #5292 merged.

Remove the impossible startup-recovery path: arming deliberately truncates every pre-consent buffer, so the startup predicate could only race with events queued by the current process and send them before a mid-session opt-out.

Make the single shutdown flush structural by removing the non-final actor message and public flush API. Strengthen the process-level contract to prove an armed session sends nothing before shutdown, while preserving the shutdown consent re-check and bounded deadline.

Verified with the telemetry unit suite, five focused process-level race runs, the full 263-test integration target, strict workspace all-target/all-feature Clippy, source/runtime/dead-code budgets, formatting, and diff checks.
@cursor

cursor Bot commented Aug 8, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 2m 5s —— View job


Review: fix(telemetry): flush only at shutdown

  • Gather context (diff, related code)
  • Understand the request (review this PR per repo review guidelines)
  • Review correctness/panics
  • Review provider/model/route safety implications
  • Review reuse/simplification
  • Review test coverage
  • Review security

Summary

This PR removes the "startup drain" flush path (Message::Flush, Handle::flush, flush_blocking, startup_drain_due, STARTUP_DRAIN_INTERVAL_HOURS) so telemetry now has exactly one flush point: shutdown. It's a clean, well-contained removal — no dangling references anywhere in the tree (verified with rg for flush_blocking|startup_drain_due|STARTUP_DRAIN_INTERVAL_HOURS|Message::Flush), and the accompanying doc updates (docs/TELEMETRY.md, telemetry-ingest/README.md, telemetry-ingest/wrangler.jsonc) are consistent with the new single-flush behavior. Not a v0.8.65 provider/model routing change, so no concerns there.

The rewritten mid_session_opt_out_stops_the_shutdown_flush test (crates/tui/tests/integration/telemetry_contract.rs:749-789) is a genuine improvement: it now positively asserts zero batches are sent before shutdown (previously it only waited for the buffer file to exist, which didn't actually prove the startup drain hadn't fired). This is the right test for the behavior being removed.

Minor: TelemetryState::last_flush is now dead state

crates/telemetry/src/actor.rs:174 still writes state.last_flush = Some(envelope::now_rfc3339()) on every flush, but the only reader of that field (startup_drain_due in the old lib.rs) was deleted in this PR. grep -rn "last_flush" across the crate turns up only the write site (actor.rs:174) and the field definition (envelope.rs:49) — nothing reads it back anymore.

It's not a bug (the write is harmless), but it's now pure write-only bookkeeping: an extra disk write on every flush and a field/doc-comment (envelope.rs:44-46 — "so a permanently offline machine tries at most once per interval") that no longer describes real behavior, since there's no longer any interval-based rate limiting to justify. Worth either removing last_flush (and the state.json write) entirely, or leaving a short comment noting it's now purely informational for humans reading state.json, since the docs table (docs/TELEMETRY.md:80) still describes it as "the last flush attempt" without qualifying that nothing consumes it.

Fix this →

No correctness bugs, no test-coverage gaps, no security concerns found in this diff.

@Hmbown
Hmbown merged commit a790140 into main Aug 8, 2026
27 of 28 checks passed
@Hmbown
Hmbown deleted the codex/v095-telemetry-optout-fix branch August 8, 2026 13:40
Sign up for free to 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.

1 participant