Let fast-slow has() trust the fast store when configured - #2728
Conversation
## What and why
`FastSlowStore::has_with_results` asks the slow store about every key
and never the fast store, so a blob that exists only in the fast tier
reads as missing and the client re-uploads it to durable storage. On a
`fast_slow { redis, gcs }` CAS/AC this means every `FindMissingBlobs`
and every action-result completeness check fans out one metadata GET per
key to GCS even when the fast tier already holds the blob. Under a
warm-cache lookup storm those GETs queue on the GCS client's connection
semaphore and overrun the CAS server's 30 s per-blob deadline, which
Buck2 does not retry.
This adds `FastSlowSpec::trust_fast_store_for_has` (default `false`,
behavior identical to today). When `true`, the fast store answers first
and only the keys it does not hold go to the slow store. The in-flight
slow-write merge and the `NoopDownloads` shortcut are unchanged. The
name says what an operator opts into: a fast-store hit is trusted as
proof of existence, which is exactly what the default refuses to do.
The doc comment records the durability trade-off and the deployments
where the flag is safe.
## How was this verified?
New tests in `nativelink-store/tests/fast_slow_store_test.rs`, one per
documented invariant, using a counting store to prove which tier each
existence check reached:
- `has_by_default_checks_slow_store_only`: fast-only blob reads as
missing, fast store asked 0 times, slow store asked once.
- `has_trusting_fast_store_hit_skips_slow_store`: fast hit reports the
size and the slow store is asked 0 times.
- `has_trusting_fast_store_miss_falls_through_to_slow_store`: a mixed
batch keeps the fast hit, fills the slow hit into the right slot,
leaves the true miss `None`, and asks the slow store exactly once.
- `has_trusting_fast_store_still_sees_in_flight_slow_writes`: with the
flag on, a fast miss still blocks on and sees an in-flight slow write.
The two flag-on tests that exercise the new branch fail when the branch
is disabled. All pre-existing `fast_slow_store_test` tests still pass.
Measured on a production deployment (NativeLink v1.6.3, Redis fast tier,
GCS slow tier, warm-cache lookup storm): together with two GCS-client
changes, `GetActionResult` p99 went from 30.0 s to about 1 s and GCS
GETs per minute dropped roughly 3-6x; this flag removed the per-key
metadata GET fan-out that the other two changes could not.
## Risk
Default `false` leaves every existing deployment unchanged. With the
flag on, a blob present only in an evicting fast tier is reported
present and is never re-uploaded; if the fast tier evicts it before it
reaches the slow tier, the blob is lost and later reads fail with
`NotFound`. Only enable it when the fast tier does not evict, or when
another path makes the slow tier authoritative. Fast-store `has`
failures now propagate from `has_with_results` when the flag is on,
where before only slow-store failures did. The re-upload invariant this
relaxes is the one TraceMachina#1999 and TraceMachina#2072 (`LazyExistenceOnSync`) build on and
TraceMachina#1978 touches, so a review from that area is welcome.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
MarcusSorealheis
left a comment
MarcusSorealheis
left a comment
There was a problem hiding this comment.
The opt-in flag is a useful way to reduce slow-tier metadata requests, and keeping the default false preserves the existing existence policy. Unlike the GCS upload optimization in #2729, this changes what a positive existence result guarantees. Please tighten the operator guidance and tests below before merging. This is a source review; I have not run additional tests or performance measurements.
This change is already backend-independent because it lives in FastSlowStore. The reported latency/request improvements are from several changes together; retain that qualification, and evaluate cold-cache latency too because misses now query fast and then slow serially.
| /// When `false`, existence checks consult the `slow` store only. A blob | ||
| /// held solely by the `fast` tier then reads as missing, so the client | ||
| /// re-uploads it and the blob reaches durable storage. Setting this to | ||
| /// `true` gives up that guarantee in exchange for far fewer `slow` store | ||
| /// requests: if the `fast` tier evicts a blob that never reached the | ||
| /// `slow` tier, the blob is gone. Only enable this when the `fast` tier | ||
| /// does not evict, or when another path makes the `slow` tier | ||
| /// authoritative. |
There was a problem hiding this comment.
“The fast tier does not evict” is not sufficient safety guidance for this flag. A non-evicting memory cache can disappear on restart, and a persistent node-local cache can answer FindMissingBlobs on replica A while the subsequent read goes to replica B, whose fast tier and shared slow tier both lack the blob. No eviction is needed for that failure. “Another path makes the slow tier authoritative” also needs to specify what actually guarantees persistence and visibility; this flag itself schedules no replication.
Suggested replacement:
| /// When `false`, existence checks consult the `slow` store only. A blob | |
| /// held solely by the `fast` tier then reads as missing, so the client | |
| /// re-uploads it and the blob reaches durable storage. Setting this to | |
| /// `true` gives up that guarantee in exchange for far fewer `slow` store | |
| /// requests: if the `fast` tier evicts a blob that never reached the | |
| /// `slow` tier, the blob is gone. Only enable this when the `fast` tier | |
| /// does not evict, or when another path makes the `slow` tier | |
| /// authoritative. | |
| /// By default, existence checks consult the slow store, except when the | |
| /// slow store advertises that downloads are a no-op. A fast-only blob | |
| /// is normally reported missing so clients can upload it to the slow | |
| /// tier. Enabling this option accepts fast-tier existence without | |
| /// confirming slow-tier persistence or waiting for an in-flight slow | |
| /// write when the fast tier already reports a hit. | |
| /// | |
| /// This option does not replicate fast-only blobs. Eviction, expiry, | |
| /// restart, or loss of the fast tier can make a reported blob unavailable. | |
| /// A different replica may also be unable to read a node-local fast hit. | |
| /// Disable this option when slow-tier persistence is required. Otherwise, | |
| /// ensure the fast tier meets the deployment's retention and reader | |
| /// visibility requirements, or accept the resulting missing-blob risk. | |
| /// Fast-store lookup errors propagate rather than falling back to slow. |
Please also regenerate the configuration reference using gen:config-reference, and update web/apps/docs/content/docs/how-to/stores/compose-stores.mdx plus the narrative reference/nativelink-config/store-overview.mdx. The latter's existing fast/slow durability warning should distinguish default slow-tier existence checks from this opt-in policy. Add an explicit warning next to an opt-in example, covering shared versus node-local fast tiers and that “no eviction” does not mean persistent storage. Keep exhaustive field details in the generated reference, per AGENTS.md.
| "Fast store must be consulted exactly once" | ||
| ); | ||
| assert_eq!( | ||
| slow.has_calls.load(Ordering::SeqCst), |
There was a problem hiding this comment.
Please have HasCountingStore record the keys passed to has_with_results, and assert that the slow call contains exactly the slow-hit and true-miss keys, excluding the fast-hit key. The current call-count assertion would also pass if the implementation queried the slow tier with the entire batch and then preserved fast results. That would regress the main request-volume benefit while all these assertions stayed green.
An interleaved batch with duplicate keys would also exercise reconstruction of the result slots, including distinct sizes returned by the tiers.
| // Check with the slow store first. | ||
| self.slow_store.has_with_results(key, results).await?; | ||
| if self.trust_fast_store_for_has { | ||
| self.fast_store.has_with_results(key, results).await?; |
There was a problem hiding this comment.
Please add failure/concurrency tests for the policy introduced here:
- A fast-store lookup error propagates and the slow store is not queried, even if it holds the blob. This behavior is mentioned in the PR description but should also be part of the configuration documentation and test contract.
- A fast miss followed by a slow-store lookup error propagates that error.
- A fast hit while a slow write is gated returns immediately without waiting for that write. Then fail the slow write and verify the fast-only result remains visible under the opt-in policy.
The new in-flight-write test uses a Noop fast tier, so it verifies only the fast-miss path. The fast-hit case deliberately skips the in-flight wait because its result slot is already Some; explicitly testing that distinction would prevent the claim that the merge is “unchanged” from being read as a persistence guarantee for all hits. If that early visibility is not intended, the in-flight merge needs to take precedence over trusted fast hits.
MarcusSorealheis
left a comment
MarcusSorealheis
left a comment
There was a problem hiding this comment.
As this PR stands, it cannot be merged. It changes the behavior of the existence check so it needs a lot of consideration and analysis.
@modernmedici can you try to schedule a call?
3 participants
What and why
FastSlowStore::has_with_resultsasks the slow store about every keyand never the fast store, so a blob that exists only in the fast tier
reads as missing and the client re-uploads it to durable storage. On a
fast_slow { redis, gcs }CAS/AC this means everyFindMissingBlobsand every action-result completeness check fans out one metadata GET per
key to GCS even when the fast tier already holds the blob. Under a
warm-cache lookup storm those GETs queue on the GCS client's connection
semaphore and overrun the CAS server's 30 s per-blob deadline, which
Buck2 does not retry.
This adds
FastSlowSpec::trust_fast_store_for_has(defaultfalse,behavior identical to today). When
true, the fast store answers firstand only the keys it does not hold go to the slow store. The in-flight
slow-write merge and the
NoopDownloadsshortcut are unchanged. Thename says what an operator opts into: a fast-store hit is trusted as
proof of existence, which is exactly what the default refuses to do.
The doc comment records the durability trade-off and the deployments
where the flag is safe.
How was this verified?
New tests in
nativelink-store/tests/fast_slow_store_test.rs, one perdocumented invariant, using a counting store to prove which tier each
existence check reached:
has_by_default_checks_slow_store_only: fast-only blob reads asmissing, fast store asked 0 times, slow store asked once.
has_trusting_fast_store_hit_skips_slow_store: fast hit reports thesize and the slow store is asked 0 times.
has_trusting_fast_store_miss_falls_through_to_slow_store: a mixedbatch keeps the fast hit, fills the slow hit into the right slot,
leaves the true miss
None, and asks the slow store exactly once.has_trusting_fast_store_still_sees_in_flight_slow_writes: with theflag on, a fast miss still blocks on and sees an in-flight slow write.
The two flag-on tests that exercise the new branch fail when the branch
is disabled. All pre-existing
fast_slow_store_testtests still pass.Measured on a production deployment (NativeLink v1.6.3, Redis fast tier,
GCS slow tier, warm-cache lookup storm): together with two GCS-client
changes,
GetActionResultp99 went from 30.0 s to about 1 s and GCSGETs per minute dropped roughly 3-6x; this flag removed the per-key
metadata GET fan-out that the other two changes could not.
Risk
Default
falseleaves every existing deployment unchanged. With theflag on, a blob present only in an evicting fast tier is reported
present and is never re-uploaded; if the fast tier evicts it before it
reaches the slow tier, the blob is lost and later reads fail with
NotFound. Only enable it when the fast tier does not evict, or whenanother path makes the slow tier authoritative. Fast-store
hasfailures now propagate from
has_with_resultswhen the flag is on,where before only slow-store failures did. The re-upload invariant this
relaxes is the one #1999 and #2072 (
LazyExistenceOnSync) build on and#1978 touches, so a review from that area is welcome.
This change is