Repository navigation
fix(module): keep a concurrent verified cache fill instead of replacing it - #105
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Comment |
Tiny Sweeper reviewReviewed head: Verdict: Ready for maintainer review · Priority: none Diagnostics and supporting detailsFull summaryPull request purpose (title): fix(module): keep a concurrent verified cache fill instead of replacing it. Purpose (from the pull-request title): fix(module): keep a concurrent verified cache fill instead of replacing it. The pull request addresses a race in the GitHub release module cache where two processes missing the cache for the same release simultaneously cause the second finisher to delete the first's verified directory via `cache::commit` and replace it with an identical copy. The commit tail of `acquire_cached` is moved into a new `commit_or_reuse` helper in `crates/tinybus/src/module/github.rs` that re-checks `cache::find_verified` before committing; on a hit, staging is dropped and the cached module and digest are returned. The pre-existing post-failure fallback is retained through a shared closure, and a `ponytail:` note records the remaining check-then-rename window until a cross-process `File::lock` is available at Rust 1.89. A regression test covering the reuse path is added to `crates/tinybus/src/module/github_tests.rs`. No push delta is available. Review snapshot
Completeness: Complete What changed`acquire_cached` in `crates/tinybus/src/module/github.rs` now hands its post-staging work to a new `commit_or_reuse` function. That helper builds a `concurrent_fill` closure over `cache::find_verified` and checks it before `cache::commit`; on a verified hit it logs and returns the cached module and digest without replacing the directory. The existing race fallback after a failed commit reuses the same closure. The remaining race window is documented in a `ponytail:` comment. A new test in `crates/tinybus/src/module/github_tests.rs` exercises keeping a concurrent verified fill. Features
TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. FindingsNo active actionable findings. Before mergeNone. Agent review detailscritique
security
tests
commits
description
Evidence and run details
|
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
…ng it cache::commit removes whatever is at the cache directory before renaming the staging directory over it. When another process filled and verified the same release while this one downloaded, that removed a directory the sibling may have its module mapped from. acquire_cached now looks at the cache once more before committing and keeps a fill that already verifies.
8b0c16f to
4506d3f
Compare
Summary
cache::commitremoves whatever is at the cache directory and renames the staging directory over it.acquire_cachedonly looked at the cache again after a failed commit. So when two processes miss the cache for the same release at the same time, the one that finishes second deletes the first one's verified directory, which that process may already have its module mapped from, and replaces it with an identical copy.Fix
The commit tail of
acquire_cachedmoves intocommit_or_reuse, which checkscache::find_verifiedonce more right before committing. If the cache already verifies (a concurrent fill), the staging directory is dropped and the existing module is returned. The existing fallback after a failed commit is unchanged.A
ponytail:note records the remaining window: a sibling can still commit between that check and the rename. Closing it needs a cross-processFile::lock, which needs Rust 1.89.Validation
Regression test
a_concurrent_fill_that_verifies_is_kept_rather_than_replaced, run alone withcargo test -p tinybus --features modules --lib -- <name>:Behaviour change
When a verified fill for the same release appears while a download is in flight, the download's copy is discarded and the existing directory is used. Nothing changes when the cache is empty or fails verification.