[DO NOT MERGE] Temporary CAS updates and fixes. #2301
Open
amankrx wants to merge 20 commits intoTraceMachina:mainfrom
Open
[DO NOT MERGE] Temporary CAS updates and fixes. #2301amankrx wants to merge 20 commits intoTraceMachina:mainfrom
amankrx wants to merge 20 commits intoTraceMachina:mainfrom
Conversation
Collaborator
Author
|
/build-image nativelink-worker-init |
Collaborator
Author
|
/build-image |
|
Image built and pushed! |
|
Image built and pushed! |
Collaborator
Author
|
/build-image nativelink-worker-init |
Collaborator
Author
|
/build-image |
|
Image built and pushed! |
|
Image built and pushed! |
Collaborator
Author
|
/build-image nativelink-worker-init |
Collaborator
Author
|
/build-image |
|
Image built and pushed! |
|
Image built and pushed! |
Collaborator
Author
|
/build-image nativelink-worker-init |
Collaborator
Author
|
/build-image |
|
Image built and pushed! |
|
Image built and pushed! |
Collaborator
Author
|
/build-image nativelink-worker-init |
Collaborator
Author
|
/build-image |
|
Image built and pushed! |
Collaborator
Author
|
/build-image nativelink-worker-init |
Collaborator
Author
|
/build-image |
|
Image built and pushed! |
|
Image built and pushed! |
The populating-digests dedup is a net loss for multi-GB blobs: the leader's pull from the slow store reliably exceeds LEADER_WAIT_TIMEOUT, every concurrent follower falls through to its own slow-store read anyway, and populating the fast tier with the huge blob evicts a large number of smaller, more-useful entries. Add a configurable size threshold (FastSlowSpec::bypass_dedup_threshold_bytes, default 256 MiB when unset) at and above which get_part streams straight from the slow store and skips the populate. Below the threshold the existing leader/follower path is unchanged. Tests: blobs at and above the threshold drive one slow-store call per concurrent reader and leave the fast tier empty; blobs below the threshold collapse to a single slow-store call and populate fast.
The default StoreDriver::check_health performs a full update_oneshot +
has + get_part_unchunked roundtrip. Under any meaningful slow-store
load (e.g. concurrent multi-GB blob reads saturating the network) that
roundtrip queues behind production traffic and easily exceeds the
per-indicator budget, causing /status to return 503 even when the
pod is otherwise functional. Kubelet treats those as probe failures
and eventually restarts the pod, dropping every connected client.
Override check_health on both registered indicators with a probe that
shares no path with production traffic:
* GcsStore: a single object_exists() call against a fixed
never-existing path. Metadata roundtrip only — independent of
body-transfer bandwidth and the upload buffer pool.
* FilesystemStore: a stat() of the configured content_path. A
single syscall, microseconds on a healthy mount; bounded with a
timeout so a hung NFS / EBS mount cannot wedge the indicator.
Both paths are bounded by a 2 s ceiling so they remain well inside
the HealthServer per-indicator budget.
Collaborator
Author
|
/build-image nativelink-worker-init |
Collaborator
Author
|
/build-image |
|
Image built and pushed! |
|
Image built and pushed! |
The previous 2-second PING ceiling was tight enough that a routine
Redis BGSAVE fork would push every RedisStore indicator over the line
simultaneously: on an 11 GB production master under load we observe
fork-induced pauses around 3 seconds, and with three RedisStore
indicators (AC, CAS, scheduler) all PING-ing through the same
connection pool, all three return Failed in lockstep — surfacing as a
503 on /status and a kubelet probe-failure event even though the
Redis service is otherwise healthy.
Verified by capturing /status response bodies during a flap window:
[{"namespace": ".../SCHEDULER_STORE/RedisStore",
"status": {"Failed": {"message":
"RedisStore::check_health: PING exceeded 2 s timeout"}}},
{"namespace": ".../CAS_REDIS_STORE/RedisStore",
"status": {"Failed": {"message":
"RedisStore::check_health: PING exceeded 2 s timeout"}}},
{"namespace": ".../AC_REDIS_STORE/RedisStore",
"status": {"Failed": {"message":
"RedisStore::check_health: PING exceeded 2 s timeout"}}}]
The HealthServer's per-indicator wrapper budget is 5 s
(DEFAULT_HEALTH_CHECK_TIMEOUT_SECONDS), so 4 s leaves a small
safety margin while comfortably absorbing the BGSAVE worst case we
have observed in practice.
Collaborator
Author
|
/build-image nativelink-worker-init |
Collaborator
Author
|
/build-image |
|
Image built and pushed! |
|
Image built and pushed! |
Production worker pods were OOMKilled (exit 137) after the ConnectionManager's `available_connections: usize` counter underflowed to ~u64::MAX while `waiting_connections` climbed unbounded. The manual decrement-on-issue / increment-on-Dropped accounting balances on paper, but a leak path was occasionally missing a `Dropped` delivery during tonic transport errors and task aborts. Switch to `Arc<Semaphore>` with `OwnedSemaphorePermit` on the Connection. RAII makes leakage structurally impossible: every Drop path (panic, abort, dropped oneshot receiver, transport error) releases the permit exactly once. Adds 3 integration tests covering the request/acquire/release cycle, an aborted-caller-future cleanup scenario, and the MAX_CONCURRENT ceiling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`RedisSubscription::Drop` previously dropped the `watch::Receiver` *before* taking the `subscribed_keys` write lock, then decided whether to remove the publisher entry based on `receiver_count() == 0`. Two concurrent drops on subscriptions sharing a publisher (e.g. multiple `WaitExecution` clients on the same operation_id) could both decrement their counts before either took the lock, then race for it: the loser saw the entry already removed and emitted a spurious "Key … was not found in subscribed keys" error. Worse, if a fresh `subscribe(same_key)` interleaved between the two drops, the second drop could remove the freshly-inserted publisher and silently strand its subscribers. Acquire the write lock *first*, evaluate "count == 1 with my receiver still alive", remove the entry under the lock if so, then drop the receiver. The lock now serialises both the count read and the map mutation, closing both race windows. Demote the absence log from `error!` to `warn!`: with the fix, that path now indicates a genuine unexpected mutation outside the lock, not the race noise. Adds 4 regression tests covering single-drop silence, drop-one-of-two preserving the publisher, 200-iteration concurrent-drop race, and resubscribe-after-drop creating a fresh publisher. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Brings in upstream changes since the last sync, including: - feb6a15 Bound CAS leader-wait + per-blob batch deadline (TraceMachina#2298) - 43ab01d Add expiry to completed redis actions (TraceMachina#2315) - 6cdcf8e fix RBE CI for hermetic LLVM (TraceMachina#2314) - f5846df Migrate to hermetic llvm (TraceMachina#2312) Conflicts resolved (all kept the local superset where the local change extended an upstream one): - nativelink-store/src/fast_slow_store.rs: kept HEAD's `huge_blob_dedup_bypasses` / `fast_store_stale_map_falls_through` metrics and the `DEFAULT_BYPASS_DEDUP_THRESHOLD_BYTES` const alongside upstream's `LEADER_WAIT_TIMEOUT` / `leader_wait_timeouts` - nativelink-store/src/filesystem_store.rs: kept HEAD's path_type=Temp bookkeeping inside the ENOENT branch on top of upstream's debug-demote of the rename failure - nativelink-store/src/redis_store.rs: kept HEAD's 4s PING_TIMEOUT and richer doc comment - nativelink-store/tests/{fast_slow_store_test,redis_store_test}.rs: concatenated both branches' independent test additions; merged `use` lists; updated `test_search_by_index_skips_int_from_cursor_read` to expect the local FT.AGGREGATE TIMEOUT clause; added `bypass_dedup_threshold_bytes: 0` to upstream's new FastSlowSpec literal so it satisfies the field added locally. All 30 test binaries across nativelink-store and nativelink-util pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
/build-image nativelink-worker-init |
Collaborator
Author
|
/build-image |
|
Image built and pushed! |
|
Image built and pushed! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is a combination of PRs that I have added here, such that I can get a docker image out of this and maintain a track of PRs to be merged.
Fixes # (issue)
Type of change
Please delete options that aren't relevant.
not work as expected)
How Has This Been Tested?
Please also list any relevant details for your test configuration
Checklist
bazel test //...passes locallygit amendsee some docsThis change is