Repository navigation
fix(telemetry): keep the counts off when another server on the same state directory writes - #2408
Conversation
…tate directory writes Every telemetry write saves the whole telemetry.json as it was read. A second server on the same state directory (an installed app beside a source checkout) that read the switch on a moment before it was turned off wrote it back on with its visit or count, and the next active message went out. The switch turned off now lives in a file of its own, telemetry-off, which only the switch writes or removes; the counts are on only when it is absent and telemetry.json says on. A switch an older build turned off is copied into it at start. Each active message checks it before it goes, so days still to send stop once another server turns the counts off. Found by the reviewer bench (pre-existing at 73d1826).
…ld's off at any read Review of the opt-out fix (Codex Terra, omo): - An off from one server overlapping an on from another could end with telemetry.json off and no telemetry-off, so a late write turned it back on. Off now marks before and after it writes telemetry.json, and on removes the mark only once telemetry.json says on. - An older build's off was copied into telemetry-off only at start; when that write failed it never was. Every read of telemetry.json now copies it. - Off stops this server's own sends before it reads or writes anything (a read may write the mark), so a mark that cannot be written no longer leaves them going. The two-server test helper writes its lines synchronously, so a buffered line cannot be held back by the child's blocking read. The changelog says the fix holds between servers of this version or later.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 8 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ight before each send Merge-pass review of #2408: the telemetry-off lookup used existsSync, which reads an inaccessible state directory (EACCES) or a dangling telemetry-off link as no mark, so a server with the counts on in memory still sent its install event. The lookup (offMarked) now fails closed: only a name that is not there, in a state directory this process can read, is no mark; anything at the name or any failure to tell is off. A first run makes its state directory before looking. The mark is now checked last, right before each fetch: after the active message's own state write, and in report() before the install or update event. report() now clears its in-flight send once that send is stored, as flushActive does: an off check that returned before the first await left it standing, so an owed event waited for a restart (Codex Terra). A state directory that cannot be made now reads as off in Settings, as well as sending nothing.
Merge-pass review @ c8f8c74: fixed in d7d3ea3
One older expectation changed on purpose: a state directory that cannot be made still sends nothing, and now also reads off in Settings instead of on. Validation: |
|
Merging d7d3ea3. Privacy fix (follow-up to #2392): telemetry 'off' now lives in its own telemetry-off file that only the switch writes, so another server process on the same state directory can no longer turn counts back on with a late visit or count write; an older build's off is copied into it on read; the lookup fails closed (only ENOENT in a readable state directory means no mark; EACCES, ENOTDIR, a dangling link or any other error read as off), and the mark is checked right before each send. Strict privacy review by the merge pass, two rounds. Round 1 reproduced a HIGH (an unreadable state directory or a dangling telemetry-off link was read as 'no mark', so a cached on still sent); fixed, and the exact repros now send nothing (unreadable directory 0, dangling link 0; a readable ENOENT control still sends when on). Also checked: two processes with stale writes stay off, migration honours an older build's off, a corrupt mark stays off, a denied first-run mkdir stays off, re-enabling revives no off-time counts, a held-back event no longer waits for a restart. Known limit, accepted: no cross-server lock, so the moment between the last check and the fetch call itself is not atomic. Downgrading to an older build after another process wrote enabled=true can send again (noted in the PR). 60 tests + typecheck pass; Worker/shared/deployed.json untouched. |
Found by the reviewer bench (claim #1 in its report). It was there before #2392, at 73d1826; #2392's usage counts added another writer that can trigger it. To fix before 0.4.21.
Bug
Every telemetry write in
server/telemetry.tsreads all oftelemetry.json(refresh()) and later writes all of it back (persist()). Two Ovis servers can share one state directory, for example an installed app and a source checkout. If one of them reads the switch as on just before the user turns it off in the other, its next visit or count writesenabled: trueback. The opt-out is undone and the next daily message goes out.Change
telemetry-off, besidetelemetry.json. Only the switch writes or removes it, and no visit or count touches it.telemetry-offis absent andtelemetry.jsonsays on.telemetry.jsonand again after it, so an on from another server between the two writes can't leave off unmarked.telemetry.jsonsays on.enabled: falsewritten by an older build is copied into the mark whenevertelemetry.jsonis read. A read that can't write it still reads off and tries again next time.offMarked. Only a name that is not there, in a state directory this process can read, counts as no mark. A dangling link,EACCES,ENOTDIRor any other error reads off, so nothing is sent. A first run makes its state directory before looking, so a fresh install still starts on. A state directory that cannot be made now also reads off in Settings; before, it sent nothing but showed on.report()before the install or update event. Days still waiting stop once another server turns the counts off.report()clears its in-flight send after the send is stored, as the daily-message path does, so an event held back by that last check is sent once the counts are on again.shared/telemetry.tsand the Worker are unchanged, so nothing needs deploying.Tests (fail first)
Five new tests in
server/telemetry.test.ts("another server on the same state directory") fail on main and pass here. The first two run a real second server process: it reads the state, then pauses on itsnowclock, which Telemetry calls between reading and writing. The two processes hand off over stdin and stdout, with no sleeps or polling.A sixth test checks that turning the counts on from either server turns them on.
Known limits
Reviews
omo-native-code-reviewer on 9913b08: APPROVE, LOWs only.
Codex Terra 6.1 fast xhigh, three rounds:
EPERM, reproduced there without any telemetry code), so it could not run the two-process tests; they run here and in check fast.Merge pass @ c8f8c74: BLOCK on a reproduced HIGH: the mark failed open (an inaccessible state directory or a dangling link read as no mark, and
report()sent before a write failed). There was also a MEDIUM: move the check right before each send. Both are fixed in d7d3ea3.Codex Terra on d7d3ea3: a LOW (an event held back by the last check stayed stuck until restart), fixed; then APPROVE with no findings. Its probes: modes
000,100,400andENOTDIRread off;500,700and a first run read on; dangling and self-referencing links read off.Validation
bun run check faston d7d3ea3: ok (8794 pass, 0 fail, 35 skip; typecheck and build ok).bun test server/telemetry.test.ts: 60 pass, 0 fail.server/api.contract.test.ts -t telemetry: 4 pass.server/conventions.test.tsandscripts/changelog-fold.test.ts: pass.server/telemetry.ts, and the two round-2 tests fail on 9913b08.