Repository navigation
crates/rcache: split reader/writer, use compare-and-swap on index - #155
Conversation
The previous design did an unsynchronized read-modify-write on index.shisha: writers would fetch the index at run start, accumulate new (spec_hash, sha256) entries during their build, and overwrite the file on commit with their fetched snapshot + their entries. With buildbot default max_concurrent_builds=2, two writers committing in overlapping windows would each clobber the other — the later writer wrote a stale snapshot that lacked the earlier writer's entries, even though the earlier writer had already committed them. The blob objects themselves stayed in GCS, but the index pointing at them got lost, making downstream consumers see cache-misses for artifacts that were physically present. This is what bit yesterday's openssl 3.6.2 bump: PR #99's run committed openssl + its rebuilt transitives to the index, then a smaller PR run that started concurrently committed its single entry on top of the pre-99 snapshot, dropping all the openssl-keyed entries. Users hit NotFound on every openssl consumer and paid full local-rebuild cost. Fix bifurcates the API: - RemoteCache<B> is now read-only. The local-cache fast path (5min TTL on index.shisha) stays — readers benefit from it. - RemoteCacheWriter is GCS-only, always fresh-fetches the index at construction, records the generation it observed, and on finish_uploads writes with set_if_generation_match(generation). On 412 (someone else committed in between), refetches the current index, re-merges our pending entries onto the fresh base, and retries with the new generation. Bounded to 8 retries with exponential backoff (200ms->6.4s, ~25s total cap) plus jitter to break synchronized retry storms. Idempotency falls out for free: BTreeMap::extend with the same (spec_hash, sha256) is a no-op, so concurrent writers committing the same entry both succeed without producing inconsistent state. Bifurcating by type (rather than just adding a generation field to the existing RemoteCache) is the safer factoring — it makes it structurally impossible to write through a stale-from-local-cache index. A writer with no real generation and if_generation_match=0 would silently overwrite an existing index whose absence we never verified. Test coverage for remote_writer (14 unit tests): - Retry policy table: 412 backoff schedule across all attempt indices (200ms -> 6.4s capped), MAX_RETRIES exhaustion, non-412 HTTP errors, and no-status-code transport errors — all bubble up rather than silently succeed. - Merge invariant: empty pending, empty fetched, idempotent on repeated entries, pending overrides fetched. - Race simulation: directly exercises merge_for_commit to model yesterday's lost-update scenario (W1+W2 race over base v0; all three entries survive). Plus a 10-writer chained contention variant. Both fail under the old index.extend(uploaded) design. Two writers updated to the new type: - crates/minimal/src/cmd_upload_cache.rs - (downstream) build-servers/crates/buildbot/src/orchestrate.rs — separate PR. Reader call sites unchanged. RemoteCache::new_with_gcs_bucket is preserved for read-only GCS use (e.g. res-server). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| const MAX_INDEX_WRITE_RETRIES: u32 = 8; | ||
|
|
||
| /// Initial backoff between retries; doubled each attempt up to a cap. | ||
| const INITIAL_RETRY_BACKOFF_MS: u64 = 100; |
There was a problem hiding this comment.
With a default max-age of 300, our retries need to at least span that. Start at 2s?
There was a problem hiding this comment.
Done in febadd2. Bumped initial backoff from 100ms to 2s, so the schedule across 8 retries is now 4s, 8s, 16s, 32s, 64s, then capped at 128s for the last three (about 508s total). That spans the 300s cache window with margin. Tests updated to match.
| /// GCS generation it was loaded from. Returns `(default, 0)` if the index | ||
| /// doesn't exist yet — `0` doubles as the "create-if-not-exists" precondition | ||
| /// value for subsequent `if_generation_match` writes. | ||
| async fn fetch_index( |
There was a problem hiding this comment.
Can we use the same code for fetching the index for RemoteCache as here for RemoteCacheWriter? also, theres no need to fetch it again if you need a RemoteIndex as part of the construction of a RemoteCacheWriter: We should keep track of the generation in RemoteCache too.
There was a problem hiding this comment.
Done in febadd2, both parts.
For shared fetch, extracted fetch_gcs_index as pub(crate) in remote_writer.rs and added a generation() method on the FetchResponse trait. Default impl returns None, the GCS impl returns Some(response.object().generation), and Reqwest stays default. The existing RemoteCache::new picks up the value through the trait so non-GCS readers are unchanged.
For tracking generation in RemoteCache, added a gcs_generation: Option<i64> field. Populated from index_resp.generation() on fresh fetches, set to None on the local-cache fast path (since we never asked GCS what generation we have). Then added RemoteCache<Storage>::into_writer that reuses the index plus generation when Some, falls back to fetch_gcs_index when None. The local-cache fallback still hits GCS once because the cached file does not carry a generation, but that only triggers for a reader that already accepted the 5min stale window for reads.
…iter)
Two changes in response to Tom's review:
1. Bump INITIAL_RETRY_BACKOFF_MS from 100ms to 2000ms so the retry
schedule spans the index file's Cache-Control max-age of 300s. If
any HTTP cache layer between us and GCS serves a stale generation
during refetch, short backoffs would just burn retries against the
same stale data. With 2s initial, the schedule becomes
4s/8s/16s/32s/64s/128s (capped)/128s/128s, totalling ~508s which
comfortably exceeds the 5-min cache window. Test expectations
updated accordingly.
2. Share the index-fetch path between RemoteCache and RemoteCacheWriter,
and let the reader remember the GCS generation so that converting a
reader into a writer avoids a redundant fetch.
Specifically:
- Add fn generation() -> Option<i64> to the FetchResponse trait in
common::fetchers, with a default impl returning None. Override in
the GCS impl to read ObjectHighlights.generation off the response.
Plain-HTTPS (Reqwest) readers keep the default None.
- RemoteCache gains a gcs_generation field, populated from
index_resp.generation() on fresh fetches. Local-cache-load path
sets None (we never asked GCS what generation we have).
- Add RemoteCache<Storage>::into_writer(self, ot) which reuses the
already-fetched index and generation when possible, and falls
back to refetching via the shared fetch_gcs_index helper when
the reader was loaded from the local cache.
- Rename the writer's fetch_index to fetch_gcs_index, make it
pub(crate), and add RemoteCacheWriter::from_parts so
into_writer can construct a writer directly from the reader's
state without a round-trip.
All 16 existing tests still pass. fmt + clippy clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Up to you if you want to do another round or just get it in as-is, i think ideally we could flesh out the tests a bit more but we can always do that later.
| // --- jitter_ms --- | ||
|
|
||
| #[test] | ||
| fn jitter_in_range() { | ||
| for _ in 0..20 { | ||
| let j = jitter_ms(100); | ||
| assert!(j < 100, "jitter {j} should be in [0, 100)"); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn jitter_zero_does_not_panic() { | ||
| // The .max(1) guard prevents modulo-by-zero. Result is 0. | ||
| assert_eq!(jitter_ms(0), 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
The jitter_ms function is 5 lines, these arent really useful
https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html
There was a problem hiding this comment.
Deleted in d7fa248. The article makes the case clearly, both jitter_* tests removed.
| ); | ||
| } | ||
|
|
||
| // --- merge_for_commit + RemoteIndex behavior --- |
There was a problem hiding this comment.
The code under test is effectively BTreeMap::extend - which has these semantics, so these tests arent really testing anything.
A useful test would be something that faked the GCS API and imitated these conditions, because then the code under test would be much more substantial, and using the public API would be more stable to change than the simple internal methods being used here.
There was a problem hiding this comment.
Removed all four merge_* tests and both race_simulation_* tests in d7fa248. They were exercising BTreeMap::extend semantics rather than the writer logic.
Your point about needing mocks for the real coverage is right, and the new code in this PR is small enough (store an int, add the if_generation_match arg) that the mock-GCS test infrastructure would be disproportionate. The plan is to lean on the buildbot integration once #155 plus the build-servers PR land, since concurrent PRs naturally
8000
hit the contention path against staging.
Also consolidated the decide_retry_after_failure tests from 6 down to 3 covering the distinct branches (exponential backoff with cap, MAX_RETRIES bound, non-412 errors skip retries). Kept those because the policy table is the contract for retry behavior, but happy to drop them too if you think they fall on the change-detector side.
Per Tom's review on PR #155 (and the linked Google Testing on the Toilet article), the new tests added for the writer were mostly change detectors that exercised stdlib behavior rather than the writer's logic. Removed: - merge_with_empty_pending_clones_fetched - merge_with_empty_fetched_uses_pending - merge_idempotent_with_repeated_pending - merge_pending_overrides_fetched_for_same_spec_hash All four were testing BTreeMap::extend semantics through the internal merge_for_commit helper. - race_simulation_loses_no_entries - race_simulation_many_writers_all_survive These looked correctness-flavored but were also exercising BTreeMap::extend and not the actual retry-loop logic in finish_uploads. - jitter_in_range - jitter_zero_does_not_panic The jitter_ms function is 5 lines; the tests just restate the modulo operator. Consolidated the retry policy tests from 6 to 3 covering the distinct branches of decide_retry_after_failure: the exponential backoff schedule (with cap), the MAX_INDEX_WRITE_RETRIES bound, and the rule that non-412 responses (including transport errors with no status code) skip the retry path. The substantive new code in this PR is small (store an int, add the if_generation_match arg). The CAS retry loop's correctness under real contention will be exercised through the buildbot integration once #155 plus the build-servers PR land, since concurrent PRs naturally hit the contention path against staging. Building dedicated mock-GCS test infrastructure for code this size would be disproportionate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Tom's review on PR #155, the existing tests were change detectors exercising `BTreeMap::extend` rather than the writer's logic. The google-cloud-storage crate exposes a `stub::Storage` trait specifically for this kind of mocking, so the writer is now generic over it and tests inject a `MockStorage` via `Storage::from_stub`. Refactor: - `RemoteCacheWriter<S: stub::Storage = stub::DefaultStorage>` is now generic over the GCS backend stub. The default keeps the public API unchanged for production callers. - `from_parts` and `fetch_gcs_index` are correspondingly generic. - `RemoteCacheWriter::new` stays on the default-storage variant only, since fresh GCS construction has no test use. Tests (4 mock-based, replacing the 3 retry-policy unit tests; total remote_writer test count is 4): - finish_uploads_no_contention_writes_once: single write attempt observes the recorded generation in the precondition. - finish_uploads_retries_on_412_then_succeeds: 412 triggers refetch and a second write with the new generation. Verified by asserting `if_generation_match` on each write request. - finish_uploads_exhausts_retries_and_surfaces_error: 412 forever produces MAX_RETRIES + 1 write attempts, MAX_RETRIES refetches, then surfaces a `Backend(412)` error. - finish_uploads_does_not_retry_on_non_412: 500 propagates after one attempt with no refetch. Tests use `tokio::test(start_paused = true)` so the multi-second backoff sleeps auto-advance and the suite still finishes in 0.00s. Dev-deps added: `mockall = "0.13"` and `tokio` with `test-util`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Pushed b925927. Went the Tests assert on Replaces the 3 retry-policy tests from the previous fixup since the new behavior tests cover the same ground through the public API. |
Summary
Splits
rcache::RemoteCacheinto a read-only type and a new GCS-onlyRemoteCacheWriterthat usesif_generation_matchcompare-and-swap onindex.shisha. Fixes the lost-update race that caused the openssl 3.6.2 incident — concurrent writers were silently dropping each other's index entries.What was happening
finish_uploadsdid unsynchronized read-modify-write: each writer fetched the index at run start, accumulated(spec_hash, sha256)entries during the build, then overwrote the file withtheir snapshot + their entrieson commit. Withmax_concurrent_builds=2, two writers committing in overlapping windows would each clobber the other:The blob objects stayed in GCS — only the index entries pointing at them got lost. Yesterday: PR #99 (openssl 3.6.2) committed openssl + ~25 rebuilt transitives; a smaller concurrent run committed on top of the pre-99 snapshot, dropping all the openssl-keyed entries. Users hit
NotFoundon every consumer and paid full local-rebuild cost.The fix
Bifurcation, not just a generation field. Reason: keeping local-cache + writer in the same type is a footgun — a writer holding a cached-from-disk index has no real generation to use, and writing with
if_generation_match=0would silently overwrite if the index didn't exist OR if we got lucky on timing. Two types make the constraint type-safe.RemoteCache<B>— reader. Local-cache fast path (5min TTL) preserved. Used bymctx,res-server.RemoteCacheWriter— GCS-only. Always fresh-fetches the index at construction, records the generation it observed. Onfinish_uploads:set_if_generation_match(generation).Idempotency falls out for free:
BTreeMap::extendwith the same(spec_hash, sha256)is a no-op, so concurrent writers committing the same entry both succeed without producing inconsistent state.Test plan
14 new unit tests in
remote_writer::tests, all passing:Retry policy (6) — exhaustive table:
Nonewas being mapped toDone)Merge invariant (4):
spec_hashRace simulation (2) — directly model the lost-update bug:
Both race tests would FAIL under the old
index.extend(uploaded)design.Jitter (2): in-range + zero-edge-case.
cargo clippy --workspace --all-targetsclean;cargo fmtclean.Callers updated
crates/minimal/src/cmd_upload_cache.rs— switched toRemoteCacheWritervia newContext::remote_cache_writer().gominimal/build-serverscrates/buildbot/sr 8000 c/orchestrate.rs— separate PR after this merges + the rev pin gets bumped.Reader call sites unchanged.
RemoteCache::new_with_gcs_bucketpreserved for read-only GCS use.Out of scope (follow-ups)