Repository navigation
Conversation
pblazej
added
the
internal
9ad007b to
cf773fd
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cf773fd to
b969370
Compare
`BatchCache` holds encoded batches between the exporter and the transport, oldest first: `MemoryCache`, or `FileCache` in `storage_dir`, where a batch is committed once its file is fsynced, renamed and the directory fsynced. Both evict the oldest batches past their size and count bounds, report what they evicted, and replace a batch by its two halves in one transaction. `SpillCache` keeps a batch the disk refused in memory, counted as a cache write error, and takes no write once the app opted out.
b969370 to
a06da21
Compare
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| if self.remove(id).is_ok() || matches!(self.path(id, BATCH_EXT).try_exists(), Ok(false)) | ||
| { |
There was a problem hiding this comment.
🟡 Unsynced evictions count as permanent losses
When directory sync fails after an unlink, prune reports the batch as evicted because its file is absent. A crash can restore the unsynced file, so the exporter counts a batch as lost before replaying it.
Learn more
The cache treats a directory sync as the durability boundary for both writes and deletes. remove can return an error after unlinking a file if that sync fails. A missing path then proves only that the unlink is visible now, not that it survives a crash. Reporting the ID to the exporter as evicted records a loss that can be contradicted when the file returns on restart.
Example: With a 10-byte cap, pushing batch B evicts batch A. Removing A unlinks its file, but directory sync fails; try_exists returns false and A is reported as dropped. A power loss before another successful directory sync can bring A back for upload.
Recommended fix: Distinguish an unlink that succeeded from a durable deletion. Retry the directory sync when possible, or defer final eviction reporting until durability is confirmed; preserve the failed ID for recovery or subsequent accounting.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Counting the visible removal is deliberate, even when the directory sync after it fails, and an_eviction_whose_directory_sync_fails_still_counts pins it. If a power loss brings the file back, the next launch either sends it or evicts it and counts it again, so the loss count can be one batch too high, but no record is sent twice. The cache.rs docs now say that the counter does not promise a crash-safe delete (1371f6c).
…ectory `FileCache::prune` now keeps the newest batch, as `MemoryCache` does, so a batch larger than the cap still gets its upload attempt. An eviction whose directory sync fails still counts the batch once its file is gone, so the batch just committed is no longer evicted with it. `remove` and `clear` treat a directory that the OS purged as empty, so an opt-out reports success. A push now holds the process-wide directory lock (`DIR_LOCK`, was `SPLITS`) while it writes, so a second cache opening on the directory cannot sweep away its `.tmp`. A multi-member rebinding `replace` is rejected before anything is written, because recovery could not tell it apart from a finished one. `clear` also deletes a `.tmp` that a failed write could not remove, under the directory lock, and reports a delete that fails. The file cache's diagnostics wait until the spill gate is released, so a host logger never runs under it. The spill warning fires once per failure episode instead of once per pipeline, and it and the split warning are logged after their locks. `clear` lets a stuck journal be retried. Push and replace share one after-write step that logs a prune error. The file extension and journal separator constants are named and documented, the module-wide dead-code expectation narrows to `SpillCache`, and the docs state the real id format and failure behavior. A changeset records the stage. The `BatchCache` docs list the characters an id holds, all of them valid in a file name on every platform. The `MemoryCache` docs name the newest-batch exception to its byte bound. The `BatchCache` and `prune` docs say that an eviction counts once its file is gone, even when the directory sync after the unlink failed: the count follows the visible removal and does not promise a delete that survives a crash.
a06da21 to
1371f6c
Compare
Summary
The write-ahead cache between the exporter and the transport.
Changes
BatchCachewithMemoryCacheandFileCache(storage_dir)SpillCache: keeps a batch the disk refused in memory, and takes no write after the opt-outFileCachekeeps the newest batch, asMemoryCachedoes, so a batch larger than the cap still gets its upload attemptremoveandcleartreat a directory that the OS purged as empty, so an opt-out succeeds;clearalso deletes a stray.tmpand reports a delete that fails.tmpreplacewith more than one member is rejected before anything is writtenVerification
At
1371f6c6, 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: 77 unit, 0 docRUSTDOCFLAGS="-D warnings" cargo doc -p livekit-telemetry --no-depsreports only the unresolved link toTelemetry::statsinstats.rs, whichmain(6473ec17) has too; feat(telemetry): add the pipeline, sessions, spans and exporter #1483 adds the item