Repository navigation
Conversation
bd2245f to
172d65f
Compare
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 2 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| let generations: Vec<Generation> = | ||
| GENERATIONS.lock().unwrap_or_else(|e| e.into_inner()).drain(..).collect(); | ||
| generations | ||
| .into_iter() | ||
| .filter_map(|g| Some((g.shared.upgrade()?, g.commands))) |
There was a problem hiding this comment.
🔴 Repeated opt-out bypasses pending purge
When disable() runs twice before the first purge finishes, the second call returns success without awaiting it. GENERATIONS was drained by the first call, so cached batches and exporters can remain active after the second returns.
Learn more
The opt-out has a synchronous phase that revokes all generations and an asynchronous phase that clears storage and stops exporters. The first disable() moves all entries out of GENERATIONS before its returned future is polled. A second disable() therefore returns a future with no work, even when the first future is pending. Awaiting that second future does not establish that the purge has finished.
Example: Call let first = disable() while a disk-backed pipeline has cached records; call disable().await before polling first. The second call returns true with the cache still on disk, rather than waiting for its deletion.
Recommended fix: Track the active purge as shared lifecycle state. Make subsequent disable() futures join its completion and return its final result; ensure this also works when a prior future is dropped.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 5e17e85: global::disable now takes a snapshot of the live generations instead of draining the list. A second call made while the first is still purging also revokes, purges and awaits each one.
172d65f to
aa2add6
Compare
`global::install` makes one pipeline the process's and starts the platform's instruments on it; `scope`, `emit`, `log`, `device_event`, `set_device_state` and `stats` reach it from anywhere and no-op until it is installed. `global::disable` is the opt-out: before it returns the pipeline is removed, its instruments stopped and every generation revoked, so nothing is captured, cached or sent again and later installs are refused; the future it returns deletes everything still held.
One test per row of the device contract: crashes and kills at every storage step, full or vanished storage, offline and constrained networks, background and suspension, clock changes, device pressure, the opt-out at every point of a capture and upload, and the lifecycle of the process pipeline and its instruments.
…order `global::disable` now takes a snapshot of the live generations instead of draining the list, so every call revokes, purges and awaits each one: a second call made while the first is still purging resolves only once the cache is deleted. `install` and `uninstall` stop the outgoing instruments while their pipeline is still the installed one, so a last record from `stop` reaches that pipeline instead of the new one or nowhere. The device tests build on Windows again: `chmod` is Unix-only, like its callers. The awaited opt-out holds every live generation before the installed handle goes, so it cancels and awaits an exporter whose last handle that was, running or draining. It revokes through the admission barrier, so no request starts once it returned, and a generation already gone keeps its storage directory on record until it is known to be empty or missing, so the opt-out deletes what it left on disk. A directory that cannot be read or looked up stays on record, and the opt-out reports the erase incomplete, so a later one deletes it. A pipeline built around a caller-provided cache keeps no `storage_dir`, so the opt-out never deletes files in the directory that this cache ignores. A pipeline whose `storage_dir` could not be opened at start keeps it on record, and after the live purges the opt-out erases the storage directory of every generation, running or gone: the batches that an earlier launch left there go too. A refused erase is logged. A configure that the opt-out refuses erases its `storage_dir` too, so an earlier launch's batches go even when this launch could not open that directory. The `storage_dir` docs say that the cache owns the directory and deletes other files in it. Purges racing each other count each record once. A refused install deletes its cache outside the lifecycle lock. The install slot is named `INSTALLED`, `shared()` is gone in favor of a crate-private `current()`, and `uninstall` is private. The opt-out wait uses the shared grace and the config's timeout. The instrument hooks and `flush` have docs, the tests prove the pipeline is freed after shutdown, and use small helpers for tracks and cache-backed pipelines. A changeset records the stage.
aa2add6 to
5e17e85
Compare
Summary
global, the process-wide pipeline, and the opt-out. From here #1396 adds onlyglobal::uninstalland aSeverityconversion tolivekit-telemetry, besides the README andSPEC.md. Every link below points at the stage that adds the test (the UniFFI ones at #1396).Changes
global: one pipeline per process, installed once and reachable from anywhere, with the platform's instruments started on itglobal::disable: the opt-out, in effect before it returnsdevice_tests.rs: the device contract, one test per rowdead_codeexpectation removedglobal::disableresolves only once the cache is deleted, also while the first call is still purginginstallanduninstallstop the outgoing instruments while their pipeline is still installed, so a last record fromstopreaches itwith_cachepipeline never records thestorage_dirit ignoresDevice contract: every failure scenario → behaviour → max lost → why → test (32 rows)
What can go wrong on a device, and the most it can cost. OTel specifies no client persistence or device policy, so every row is custom. U = records not yet committed (≤ one flush interval, ≤ 2 048 queued) + open spans (≤ 256) + open RTC windows; B = one batch (≤ 512 records); E = records evicted or expired, always counted. N = records committed to the cache and not yet sent (≤ 4 MiB compressed, ≤ 512 batches, ≤ 24 h old). A batch is committed when the cache's
pushreturns (filefsynced, renamed, directoryfsynced on Unix); crash points are driven deterministically by test-only fault hooks around every write, fsync, rename, delete and publish.Max lost: 0 = nothing; 0 until limits = nothing until the cache's bounds (4 MiB, 512 files, 24 h) evict, then E; U, B, E, N as defined above. Every row is custom (no OTel spec covers client persistence or device policy); Why names the platform behaviour, spec or reason behind the row.
background_transition_flushes_while_foreground_waits_for_the_tickentering_the_background_flushes_even_with_the_allowance_spenta_crash_loses_only_what_was_not_yet_cachedbatch_is_written_before_upload_and_replayed_on_next_startpre_connect_records_replay_after_a_restarta_crash_between_the_answer_and_the_delete_resends_the_batch_oncea_batch_in_flight_at_a_crash_is_sent_again_once.tmp+fsync+ rename; a torn.tmpremoved at openrename(2)is atomic;fsync(2)first, so a crash never exposes a torn filea_write_killed_half_way_leaves_no_partial_batchfile_cache_evicts_oldest_beyond_max_bytes_and_drops_stray_tmpa_split_crashed_at_every_step_loses_and_duplicates_nothinga_split_failing_after_its_commit_point_is_kept_and_purgeablea_second_cache_on_the_directory_never_breaks_a_split_in_progressa_stuck_journal_is_retried_after_a_write_not_every_listinga_crash_while_binding_still_replays_to_the_rooms_projectcorruptcorrupt_and_stray_cache_files_are_dropped_and_countedan_inaccessible_batch_is_kept_not_counted_corrupta_failed_delete_after_acceptance_is_not_sent_againan_accepted_batch_awaiting_its_delete_is_not_rebound_and_resentmax_cache_bytes, oldest evicted and counted), still uploaded;cache.write_errors; a split that cannot commit keeps the parentENOSPCis ordinary on phones, iOS may purge Caches while the app runs, a failedfsync(2)leaves durability unknowna_full_disk_keeps_batches_in_memory_and_says_soa_failed_directory_sync_keeps_the_batch_in_memory_not_half_on_diskdurability_failures_are_errors_not_silent_successesthrottledduring a server pause)cache_eviction_is_counted_as_a_dropfile_cache_caps_the_number_of_batchesa_hold_longer_than_the_cache_reports_what_it_costbatches_older_than_a_day_are_dropped_and_counted_at_startbatches_expire_while_the_app_runsa_clock_jump_does_not_expire_a_fresh_backlogdevice_holds_record_without_attemptinga_device_change_during_a_pass_stops_its_remaining_requestsa_soft_hold_turning_hard_does_not_spindevice_holds_record_without_attemptingthe_soft_hold_cap_is_scheduled_and_splits_count_against_itdevice_state_emits_change_events_and_stretches_cadencecpu_limitation_counter_drives_cadence_pressurerelief_brings_the_pending_tick_forwardunknown_thermal_and_low_power_are_silent_and_neutraluploads_hold_while_connecting_but_never_beyond_the_captyped_spans_hold_uploads_while_connecting_and_export_when_endeddeadlines_are_served_while_a_request_is_outexport_timeout_ms, then cancels the request on the wire and stops; the rest stays committed; the device state pushed at install always ships byflush+shutdownexport_timeout_msthe_initial_device_state_always_ships_by_flush_and_shutdownshutdown_during_a_slow_backlog_is_bounded_and_joins_the_exportershutdown_drains_every_finished_span_batchshutdown_leaves_a_session_summarymaxQueueSize; drop-oldest keeps the freshest contextin_memory_bounds_count_every_dropqueue_overflow_is_counted_by_reasonflood_guard_caps_events_but_not_stats_windowsnotifications_within_an_interval_never_exceed_one_allowancethe_allowance_applies_only_while_a_room_is_in_a_calllivekit*targets, neverlivekit_telemetry*) wherever the platform callslog_forward_bootstrap(Swift's defaultOSLoggerwithffi: truedoes), whatever level it passes — that level filters only the console forwarding; platforms must not feed forwarded Rust entries totelemetry_log; any compact JWT (three base64url segments decoding to JSON, whatever the header's encoding, also inside punctuation such as(...<jwt>)) masked as<jwt>, bearer andtokencredential values (token=,"token": "…", quoted or spaced) as<redacted>; console entry unchangedonly_the_cores_own_warnings_and_errors_are_copied(added in #1396)the_platforms_level_filters_the_console_not_the_telemetry_copy(added in #1396)tokens_in_copied_records_are_masked(added in #1396)the_console_entry_is_what_it_always_was(added in #1396) (lands in #1396)Retry-After/RetryInfopause is held in memory only: after a relaunch each project gets one request (once its token is handed over) before a new 429/503 pauses it againa_failing_server_never_costs_a_batchtelemetry_disable()returns (no scope, no capture), the purge then runs on the core's runtime (a repeated opt-out joins the one already running);telemetry_is_disabled()istrueprocess-wide before the call returns, so a platform with per-isolate state checks it before eachgetStatsor submit; lifecycle serialized with instruments; every generation revoked under the locks that commit data, its exporter cancelled and awaited (draining ones too), queue/spans/windows/cache purged; then thestorage_dirof every generation, live or gone, erased, so an earlier launch's batches are deleted even when this launch could not open that directory and cached in memory; a later configure is refused: it clears its own cache, erases itsstorage_dir(also one it could not open) and starts no exporter, and a directory that stays unreadable waits for the next configure; awith_cachepipeline's cache is purged while the pipeline lives, the caller owns it afterwards, and thestorage_dirthat pipeline ignores is never touched; only actual deletions counted; a purge whose retry deletes everything reports success; incomplete deletes logged, and the opt-out reports themthe_opt_out_is_in_effect_before_its_purge_runsan_opt_out_counts_the_halves_of_an_unpublished_splitopting_out_stops_collection_and_purges_everythingopting_out_during_an_upload_resurrects_nothinga_capture_racing_the_opt_out_leaves_nothing_behindan_instrument_never_starts_after_the_opt_out_stopped_itthe_opt_out_cancels_a_draining_generation_before_returningthe_opt_out_withdraws_a_replaced_generations_pulled_request(added in #1396)a_repeated_opt_out_waits_for_the_first_purge(added in #1396)after_the_opt_out_nothing_is_capturedan_incomplete_purge_is_reportedpurges_count_only_actual_deletions_oncethe_opt_out_deletes_an_earlier_launchs_batches_after_a_fallback_starta_refused_configure_deletes_an_earlier_launchs_batchesthe_opt_out_leaves_the_storage_dir_of_a_caller_provided_cachethe_opt_out_keeps_an_unreadable_gone_disk_until_it_is_eraseda_purge_whose_retry_deletes_everything_reports_successclearing_an_unreadable_directory_failsthe_process_pipeline_no_ops_until_installed(lands in #1396)next()ortry_next(); unknown ids ignoredtry_next_serves_without_waiting_and_never_a_stale_request(added in #1396)a_suspended_host_is_never_served_stale_requests(added in #1396)a_cancelled_request_is_never_served(added in #1396)finishing_discards_what_is_queued(added in #1396) (lands in #1396)invalidcustom_data_is_room_scoped_validated_and_snapshottedrtc_windows_carry_the_attributes_of_their_readingsa_window_closed_before_a_change_keeps_its_snapshot_whenever_it_is_queueda_project_change_splits_the_rtc_windows_by_projecttimed_outon the core's clock, whoever looks first, kept through disconnectthe_subscribe_span_is_owned_by_the_corelate_first_media_is_timed_out_whoever_looks_firsta_subscribe_past_its_deadline_stays_timed_out_through_disconnectinvalid, detached spans included)a_pending_subscribe_never_keeps_the_pipeline_aliveper_track_state_is_retired_with_the_trackthe_same_track_in_two_sessions_never_mergesretained_span_state_is_boundedfinal_limits_hold_after_decoration_and_caller_strings_are_boundedUninstall or an OS cache purge is beyond the SDK's reach (N + U). Defaults, polling (fast after a subscribe or a publish) and explicit flush (drains without the per-pass budget, within holds and pauses):
defaults_export_and_window_once_a_minutethe_core_paces_stats_pollingpublishing_polls_fast_until_the_first_outbound_readingdevice_holds_uploads_and_a_backlog_replays_within_the_budget.Verification
At
5e17e859, from a clean checkout (CI's test workflow runs only for PRs intomain, so these were run locally; there is no clippy job in CI):cargo fmt --checkcargo clippy -p livekit-telemetry --all-targets -- -D warnings, also with--all-featuresand as acargo hack --feature-powerset --depth 2runcargo test -p livekit-telemetry: 275 unit, 2 docRUSTDOCFLAGS="-D warnings" cargo doc -p livekit-telemetry --no-deps