feat(protocol): drop the telemetry queue when the portal toggle is off - #437
Merged
Conversation
The uploader treated every 401 and 403 alike: keep the refused batch, halt until the next enable_telemetry, keep queuing. Right for a bad key, which the developer fixes and re-enables through. Wrong for the application's telemetry toggle in the developer portal: once it came back on, a relaunch resent up to six days of the off period, the backfill the toggle promises not to make. The ingest now answers that refusal with the error code telemetry_disabled. On a 403 carrying it the uploader drops the refused batch and everything queued behind it, halts, and reports it to the pipe, which discards the ring at every later cycle instead of cutting batches until the next enable_telemetry. The flag is read on the cycle, never on the emit path. Every other 401 or 403, and a 403 from an ingest that predates the code, keeps the queue as before.
The telemetry_disabled scenario ends by building a fresh pipe over the same sealed store and asserting that nothing is resent. That is a negative assertion, and it passes just as happily when the fresh pipe never adopted the store and quietly ran in memory. An empty in-memory queue has nothing to backfill either. So the day someone breaks release-then-adopt, this test keeps saying "no backfill" for entirely the wrong reason. Assert the fresh pipe is durable before trusting the count. The bad-key sibling never needed this, because its assertion is positive: the refused batch shows up again, which it can only do from the store.
Classifying a 403 parsed the body, cloned the error code into a fresh String, and compared that against a constant. The String existed only to be compared and thrown away. Ask the question directly instead: does the body name this code. Same parse, and junk or a missing field still lands on the ordinary halt. It only runs on a 403, so this is tidiness, not a hot-path win, and I won't pretend otherwise.
The telemetry_disabled change updated the uploader's answer table and added an errors row, and left three other places describing the old world. The stats table listed four sources for dropped, the privacy page named three things that clear the queue, and the threat model said persisted batches are kept for a later enable. All three are wrong for an app whose portal toggle is off. Say it in each place. dropped also counts what was queued or collected after the ingest reported the toggle off, and the queue is cleared when that happens rather than held for the toggle coming back. The privacy page is the one integrators quote to their users, so it is not the place to be out of date. While at it, document the one sharp edge. The ingest caches a rejection for 30 seconds, so a launch just after the toggle is turned back on can still be refused, and that process then keeps nothing until its next enableTelemetry. Before this change the next launch would have backfilled it. Now it doesn't, which is the whole point, but a developer checking the dashboard right after flipping the switch deserves to know why that launch went missing.
The telemetry_disabled halt promised that nothing from the off period is left on disk for a later pipe to resend. It kept that promise only for a pipe that was already durable when the refusal arrived. It turns out the engine never starts one that way. enable_telemetry starts every pipe in memory and hands the protocol-state store over for the worker to adopt, which on the mobile bindings means after initialize_mls. A flush that reaches the ingest first takes the halt with an empty in-memory queue. Adoption then loads whatever an earlier launch persisted, and nothing touches it again: the cycle has stopped cutting batches, and the halted uploader returns before it reads the queue. A re-enable that waits for the old pipe to let go of the queue gets there the same way. So the next enable_telemetry, with the toggle back on, sends the off period. Which is exactly the backfill this change exists to prevent. Sweep the store at adoption when the flag is set. Adoption is the one way a batch can still enter the store after the refusal, so the check belongs there rather than in the uploader, which both a battery-deferred cycle and the worker's adoption-only wake skip. The swept batches count as dropped, in events, like the rest of the halt. The new scenario runs three launches over one store and fails with the sweep removed.
stop() runs the same cycle as any other flush, so a toggle-off refusal on the final flush already drops the queue. Nothing pinned it, though, and a batch left on disk there would be resent by the next enable_telemetry like any other. Pin it: a sealed pipe whose only send is the final flush, then a fresh pipe over the same store with nothing to resend.
The telemetry_disabled change taught the Rust doc and docs/telemetry.md that dropped also counts what the pipe discards once the ingest reports the portal toggle off. The same sentence is hand-copied into four more places: the UDL, the TypeScript type and method docs, and the Python wrapper. All four still listed the old sources. Say it in all four. The UDL edit is a plain comment, which uniffi-bindgen ignores: regenerating all three bindings from it changes no generated file, so there is nothing to regenerate here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
While an application's telemetry toggle is off in the developer portal, the ingest now answers
403with the error codetelemetry_disabledinstead of theforbiddenit shares with a revoked key or a key for another app (companion ingest PR: https://github.com/Offline-Protocol/telemetry/pull/37).The pipe treated every 401/403 the same way: keep the refused batch, halt until the next
enableTelemetry, keep queuing. That is right for a bad key, which the developer fixes and re-enables through. It is wrong for the toggle: once it came back on, a relaunch resent up to six days of the off period, which is exactly the backfill the toggle promises not to make.Now the uploader reads the 403 body's
errorcode. Ontelemetry_disabledit drops the refused batch and everything queued behind it, halts, and reports it to the pipe, which discards the ring at every later cycle instead of cutting batches, and empties any durable queue it adopts afterwards, until the nextenableTelemetry. Every other 401/403, and a 403 from an ingest that predates the code, keeps the queue exactly as before.Related issues
Refs Offline-Protocol/telemetry#36 (the toggle-gap note in its description). Companion ingest change: https://github.com/Offline-Protocol/telemetry/pull/37.
Type of change
feat— new featureChecklist
cargo fmt --all -- --checkpassescargo clippy --workspace -- -D warningspasses (CI's exact invocation, run locally)cargo test --workspacepasses — rancargo test --workspace --liblocally (2,728 passed across 13 suites, including the pipe suite's 106); doctests are left to CIcargo-denyis satisfied — not run; no dependency change<type>(<scope>): <subject>)CHANGELOG.mdupdated where relevant (docs/telemetry.md, its errors table,UPGRADING.md§20, the unreleased changelog section)unsafein core cratesBreaking changes
None. Two observable differences, both additive:
telemetryStats().droppedgains one more source (events discarded while the toggle is off), andlastErrorreadsingest responded 403 (telemetry_disabled)for this case. Theingest responded 401/403strings for every other refusal are unchanged.Notes for reviewers
PipeShared::server_disabledis read on the flush cycle, never on the emit path, so an emit still costs one atomic load. The ring keeps filling between cycles and is thrown away at the next one; nothing is persisted or sent, and the discarded events are counted indropped.enableTelemetrystarts the pipe in memory, and the worker adopts the durable queue onceinitialize_mlshands the store over, so a refusal can arrive before adoption. Adoption then loads whatever an earlier launch persisted. The halted uploader never reads the queue again, so without a sweep those batches would sit on disk until the nextenableTelemetryresent them. Adoption empties the queue it loads while the flag is set. It is the one way a batch can enter the store after the refusal.telemetry_disabledthat long after the toggle comes back on. A launch inside that window halts and discards until its nextenableTelemetry, and unlike a bad-key halt nothing is recovered at the launch after.docs/telemetry.mdstates this in its errors table. Shortening that window is an ingest change, not an SDK one.forbiddenall stay the ordinary halt, so deploy order between the SDK and the ingest does not matter. Pinned inonly_a_403_carrying_the_telemetry_disabled_code_disables.CapturingClientnow answers a non-2xx with the ingest's error shape, witherror_codesettable. Every existing scenario only looked at the status, so nothing else moved.