Repository navigation
Conversation
- GrpcStore::new is now sync (returns Result, not a future) in shard_ring test - experimental_remote_cache_compression widened to Option<bool> - ByteStream::from_body_0_4 -> from_body_1_x (aws-smithy http-body 1.x) - add slow_store_write_back field to FastSlowSpec test literals - drop unused tracing_test import; remove duplicate default_true; destructure new ServicesConfig fields in single_use validation Co-Authored-By: Claude Opus 4.8 <[email protected]>
…live/inner-lock decoupling Adds `heartbeat_stamped_before_inner_lock_under_contention`: holds `inner` to simulate a contended/starved lock, then delivers a keepalive on another task. With the fix the registry (the eviction sweep's independent liveness source) is stamped on receipt -- while the lock is still held; without it `refresh_worker` is parked on the lock and never stamps, so a punctual keepalive fails to refresh liveness and a live worker is falsely evicted as Stale. Pairs with the monotone regression already on this branch to cover both halves of the fix. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…kup drop-guard clobber (N8)
Adds `concurrent_same_key_survives_completed_leader_guard`, which reproduces
the drop-guard clobber deterministically: a `PausableAcStore` parks the second
same-key request inside its cache check (its in-flight map entry still present)
while the leader completes and its spawn's drop-guard fires. Without the
`ScopeGuard::into_inner` defuse the second request is orphaned and surfaces the
production symptom "tx hung up in CacheLookupScheduler::add_action"; with the
defuse it survives and completes.
Also adds `MockActionScheduler::{receive_add_action, respond_add_action}` — a
receive-without-respond rendezvous that holds a leader parked mid-`add_action`
so another request can interleave deterministically.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
…date update read the upload stream with while let Ok(chunk) = reader.recv().await, which treats a stream Err identically to the empty-chunk EOF. An upload that fails partway (client disconnect, or an upstream store erroring mid-copy) was upserted as whatever bytes arrived and returned Ok(partial_len) -- a truncated blob stored under the full digest key, served as success on every later read. Propagate the recv error instead, and (belt-and-suspenders) reject a completed read whose size disagrees with UploadSizeInfo::ExactSize. Regression update_rejects_truncated_stream_instead_of_committing_it drops the writer mid-stream without send_eof and asserts update returns Err and the digest is absent; fails without the fix (commits the truncation as Ok). Co-authored-by: Claude Opus 4.8 <[email protected]>
A mid-stream retry in `GrpcStore::get_part` advanced `read_offset` by the forwarded byte count but left `read_limit` unchanged. `read_limit` is a REAPI ByteStream count measured from `read_offset`, so the resume re-requested the original window from the advanced offset and over-read by exactly the bytes already delivered (an L-byte bounded read could yield up to L+f bytes). Unbounded reads (`read_limit == 0`) were unaffected and must stay so. Decrement `read_limit` by the forwarded length on resume, guarded so unbounded stays unbounded. Regression asserts the resume ReadRequest's read_limit shrinks by the forwarded count (fails without the fix: 10 vs 7). Co-Authored-By: Claude Opus 4.8 <[email protected]>
…part
S3/ONTAP/GCS get_part built bytes={offset}-{offset+length}, but HTTP byte
ranges are inclusive on both ends, so a bounded read ending before the object
end over-read one byte (a torn read on content-addressed partial reads). Use
offset+length-1, guard zero-length reads, and fix the GCS mock (which sliced
exclusively and masked the bug). r2_store wraps s3_store, so S3/R2/ONTAP + GCS
are all covered.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Cherry-pick of b7r6/mongo-update-data-upsert-defeats-version-cas, re-grafted onto 1.7.3: upsert(current_version == 0) so a genuine create still upserts but a stale-version write misses cleanly. Test kept alongside the truncated-stream regression test (both land at the same point). Co-Authored-By: Claude Opus 4.8 <[email protected]> Co-Authored-By: b7r6 <[email protected]>
…-fast - Rewrite has_does_not_consult_fast_store... -> fast-first: the fast store IS consulted first; a fast-hit is answered without the slow probe, a fast-miss falls through to the slow tier. - Add has_fast_miss_fails_fast_when_slow_tier_hangs: a hung slow tier must degrade a fast-miss to missing within SLOW_PROBE_DEADLINE, not block on the slow store full RPC timeout. Co-Authored-By: Claude Opus 4.8 <[email protected]>
- running_actions_manager_test: add slow_store_write_back to FastSlowSpec literals - fast_slow deadline test: HangingSlowStore must be a braces struct (MetricsComponent derive rejects unit structs) Co-Authored-By: Claude Opus 4.8 <[email protected]>
- fetch_server_test: FetchConfig gained fork oci field -> oci: None - oci_bridge_integration_test: OciFetchConfig gained self_registry -> None Co-Authored-By: Claude Opus 4.8 <[email protected]>
The prior tests asserted the slow-only has() behavior (fast-only blobs reported absent; a concurrent has() blocking on the gated slow write). Under fast-first a fast-resident blob is reported present and a concurrent has() may resolve as soon as the write-through populates the fast tier. Updated the inverted assertions (ignore_value_in_fast_store -> fast_store_only_value_is_reported_by_has, mixed_key_sources fast-only key, the in-flight observer tests). Tidied core::time::Duration qualification and unused mut. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Upstream 1.7.3 redesigned FilesystemStore::get_part around a single-shot read with a remove_if(Arc::ptr_eq) self-heal, and added tests that pin that contract (stale_read_does_not_remove_replacement_upload et al). The fork had an older bounded-retry loop that re-fetched the entry and read the replacement, which fails those upstream tests. Taking upstream's version; the fork's map/disk-divergence test still passes against it. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…ents Two confirmed bugs left the RedisStore keyspace/pattern subscriptions recorded as active while never actually established on the live connection. Both come with a deterministic repro test that fails on the old code and passes on the fix. 1. psubscribe lost across a concurrent reconnect (StandardRedisManager). `psubscribe` used to fetch a connection, then insert the pattern into the tracked `subscriptions` set. If a concurrent `reconnect` snapshotted+replayed that set on the new connection in between, the pattern was PSUBSCRIBEd only on the old, about-to-be-demoted connection and recorded as active — so every PMessage for it was dropped until an unrelated later reconnect replayed the now-complete set. Fix: order registration against the reconnect snapshot. `psubscribe` now inserts the pattern BEFORE fetching a connection, and `reconnect` recomputes the missing delta and subscribes it while holding the `connection_manager` write lock just before publishing the new handle. A racing `psubscribe` then either lands in the replayed snapshot or blocks on the write lock and subscribes on the already-published live connection — no window where a pattern is "active" but subscribed only on a stale connection. 2. transient init error permanently disabled remove callbacks. `register_remove_callback` returned Ok(()) and deferred the CONFIG GET + PSUBSCRIBE "__key*__:*" to a background one-shot `OnceCell::get_or_try_init`. A single transient Redis error (e.g. -LOADING during a restart/failover) was only logged; the OnceCell stayed empty and nothing retried, so eviction/expiry callbacks were silently dead forever (ExistenceCacheStore never invalidated → stale `exists` hits). Fix: retry the init with capped backoff until it succeeds, and demote the advisory notify-keyspace-events CONFIG GET check to non-fatal (log misconfig, subscribe anyway) so a transient read failure can no longer permanently disable the subscription. Both repro tests fail on the unfixed code and pass on the fix (verified both directions on the build fleet). Full `cargo test -p nativelink-store` green, clippy clean, nightly rustfmt applied. Provenance: adversarial review fleet. Co-Authored-By: Claude Fable 5 <[email protected]>
… (C20/N6 drop residual) The RAII PauseGuard's Drop decremented the pause count (fixing N6's permanently-wedged pause) but discarded the pending-removal vector that `release()` returns when the last holder goes away. On the evict-before-cancel ordering — evict cached A while upload B holds the sole pause, then cancel B — A's queued invalidation was dropped on the floor and the existence cache kept reporting A forever even though the inner store had evicted it. Drop now drains those queued removals. Since Drop cannot await and the removal is async, it spawns a detached background task (via a weak self-reference) to run drain_pending_removals. New regression `cancelled_update_drains_removals_queued_before_cancel` uses the evict-*before*-cancel ordering and fails on the prior Drop (removals discarded); the existing N6 test `cancelled_update_does_not_leave_callbacks_paused` (evict-after-cancel) is kept. Also closes the C13 has-path residual: inner_has_with_results now pauses remove callbacks around the inner `has` + the positive-result insert, so an eviction delivered mid-`has` lands after the insert instead of being a no-op before it. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Astra finding N1 (P1): the worker forgot a kill received before async action
registration finished. `create_and_add_action` fetches the Action, waits for
prior cleanup, and makes the action directory across await points before
inserting the op into `running_actions`. A `KillOperationRequest` arriving in
that window hit `kill_operation`, found nothing in `running_actions`, returned
an error, and dropped the kill — the action then started executing unkilled.
Fix: add a `pending_kills` set. `kill_operation` now, when the op is not yet
resident, records it as pending (returning Ok) instead of erroring.
`create_and_add_action`, under the same `running_actions` lock that guards the
insert, checks `pending_kills` for the op and, if present, applies the kill to
the freshly created action so the request stays effective through the
registration race. Lock ordering is fixed as running_actions before
pending_kills.
Regression test: tests/running_actions_manager_test.rs
kill_before_registration_stays_effective. It kills the op BEFORE
create_and_add_action registers it (modelling the in-transit race), then runs
the action and asserts exit_code 9 (SIGKILLed). WITHOUT THE FIX the
pre-registration kill returns Err ("Failed to get running action") and the
kill is dropped; the test fails at that point (verified by stashing the fix:
"A kill for a not-yet-resident op must be recorded, not rejected"). WITH THE
FIX the kill is parked and applied at registration, and the action runs killed.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Astra finding N2 (P1): a same-worker retry overwrites a live reservation. `Worker::run_action` unconditionally re-ran `reduce_platform_properties` and overwrote the op-keyed `running_action_infos` entry. When an op whose earlier attempt was still resident (e.g. an execution-deadline requeue that left the ledger entry in place) was dispatched again to the same worker, the platform budget was deducted a second time while `complete_action` only refunds the surviving entry — leaking the first reservation's capacity permanently. Fix: reject-if-exists under the dispatch lock. `run_action` now refuses a dispatch for an op the worker already holds (Code::AlreadyExists) before any budget deduction or insert. Regression test: tests/worker_retry_reservation_test.rs. It dispatches an op (reserving 4 of 10 mem), then re-dispatches the same op to the same worker and asserts the budget is still 6. WITHOUT THE FIX the second dispatch deducts again and the budget reads 2 (test fails: found 2, expected 6); with the fix the redundant dispatch is refused and the budget stays 6. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…nstruction The matching-engine task is spawned inside `Arc::new_cyclic`, during which `weak_inner.upgrade()` returns `None` because the strong count is still zero. The task treats a failed upgrade as scheduler shutdown, so if `task_change_notify` already held a permit (e.g. a Redis pubsub event arriving before `SimpleScheduler::new`), its first poll completed instantly, consumed the permit, saw `None`, and returned permanently — a scheduler that never matches anything. Gate the task's loop on a oneshot fired unconditionally right after `Arc::new_cyclic` returns, so the task can neither consume a pre-stored notify permit nor observe the partially constructed scheduler. Includes a deterministic in-crate repro (adversarial review fleet, branch repro/me-constructor-race, d44e3cb) that pre-stores a notify permit and widens the in-closure window via a test-only timing seam; it fails without the fix and passes with it. Co-Authored-By: Claude Fable 5 <[email protected]>
`wait_for_no_open_files` polls a process-global open-handle counter, so it drains handles from every test sharing the binary. Under asan the previous 1s ceiling was too tight and `upgraded_layout_is_not_migrated_again_on_restart` intermittently failed with "Timed out waiting all files to close" even though the handles did close. Raise the cap to 10s; the loop still returns the instant the count hits zero, so healthy runs are unaffected. This is a test-only timing fix; it does not touch the eviction change this PR exists for. Co-Authored-By: Claude Opus 4.8 <[email protected]>
… writes
Client keep-alive stamps in the memory awaited-action db (the
ActionEvent::ClientKeepAlive path in handle_action_events, and the
initial stamp in try_subscribe) mutate the AwaitedAction in place via
send_if_modified WITHOUT bumping AwaitedActionVersion. Any concurrent
read-modify-write cycle (SimpleSchedulerStateManager borrow() ->
update_awaited_action) that snapshotted the action at version N before
the stamp still passes the version CAS, and send_replace writes the
whole stale snapshot back -- a classic lost update that silently moves
last_client_keepalive_timestamp backwards. The Code::Aborted retry
defense never fires because the version matches. Consequence: the
client-timeout sweep force-fails operations whose clients are alive and
polling ("no more clients listening" DeadlineExceeded), including
operations a client subscribed to moments earlier.
Fix shape: treat the keep-alive stamp as data outside the versioned
snapshot rather than routing it through the versioned update path.
update_awaited_action now merges the stamp forward (monotone max of old
and new) before send_replace, so an accepted stale write can never
revert it. Chosen over version-bumping keep-alives because (1) a bump
would spuriously abort legitimate in-flight worker updates and wake
every changed() subscriber ~every CLIENT_KEEPALIVE_DURATION per client,
(2) it needs no new per-operation state, and (3) it leaves the
AwaitedAction wire/record format and the store/redis backend untouched.
Two deterministic repro tests included; both fail without the one-line
merge and pass with it (verified both directions, 5x each).
Provenance: adversarial review fleet.
Co-Authored-By: Claude Fable 5 <[email protected]>
…l timestamp In OperationSubscriber::changed (USE_SEPARATE_CLIENT_KEEPALIVE_KEY branch), a failed ck_* store write still advanced the local last_known_keepalive_ts and then parked for a full CLIENT_KEEPALIVE_DURATION. Peer replicas judge client liveness solely from the store, so a couple of consecutive transient store failures (Redis blip) left the store's keepalive stamp aging past client_action_timeout and a peer's sweep retired the live client's action as abandoned. Now the local timestamp only advances on a successful write; on failure the sleep becomes a bounded CLIENT_KEEPALIVE_RETRY_DELAY (1s) so the write is retried promptly without spinning when the store is down hard. Repro test fails before the fix (attempts = 1 across 3x keepalive periods) and passes after. Co-Authored-By: Claude Fable 5 <[email protected]>
`try_subscribe` stopped at the first exact unique-qualifier match, even when that match was a completed record. The unique-qualifier index retains completed records alongside any live successor, and the backend does not order results by state, so a retained completed record could be returned ahead of a joinable live one — forcing an unnecessary recreate and a new operation instead of joining the live successor. Scan every exact match and prefer a joinable (neither finished nor abandoned) candidate, keeping the first exact match only as a fallback. This is the selection half of finding N7. The atomic qualifier->live-op claim (two concurrent same-qualifier submissions can still both create an operation) needs a store primitive that does not exist today (a compare-and-set on the shared qualifier key); that is tracked separately. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…n, not timestamp kill_revoked_operations snapshots (worker, operation) pairs under the scheduler mutex, then confirms revocation against the state manager lock-free. If the operation is declined/requeued and re-dispatched to the same worker inside that window, the final locked re-guard used to pass on the fresh entry and the kill landed on the new, legitimate dispatch. An earlier version of this fix carried the entry's dispatched_at timestamp as the dispatch's identity, but a timestamp is not an identity: dispatched_at has second resolution (duration_since(UNIX_EPOCH).as_secs()), so a requeue-and-redispatch inside one wall-clock second carries the same dispatched_at and the stale kill still lands. Review caught the collision; the regression test now pins both dispatches to the same second and fails on the timestamp scheme. Each dispatch now carries a dispatch_generation: a scheduler-wide u64 incremented under the scheduler mutex at send time, stored on the running_action_infos entry. The sweep snapshots the generation and worker_notify_kill_operation only sends the kill if the entry under the lock still carries it; a re-dispatch always gets a strictly greater generation, so a stale kill can never match. The counter is scheduler-wide rather than per-worker: a per-worker counter would restart when a worker re-registers under the same id, letting a stale snapshot from the old incarnation match a fresh dispatch on the new one. dispatched_at stays as a timestamp for the acknowledgement timeout math. Also registers tests/kill_revoked_redispatch_race_test.rs in the Bazel rust_test_suite, which had not picked it up. Provenance: adversarial review fleet; incompleteness found by a follow-up review of TraceMachina#2838. Co-Authored-By: Claude Fable 5 <[email protected]>
1.7.3 WorkerUpdate::RunAction carries a dispatch_generation (the 4th tuple element the kill_revoked generation fix relies on); the N2 same-worker-redispatch test predated it. Co-Authored-By: Claude Opus 4.8 <[email protected]>
|
Some of the pull request description still needs filling in:
Edit the description and this check re-runs on its own. The sections exist because they are the parts a reviewer cannot get from the diff: why the change is needed, how you know it works, what breaks if it is wrong, and which AI tools helped ("None" is a complete answer). |
Owner
Author
|
CI-spike measurement/experiment scratch — closing to keep the queue clean. Superseded by the single clean deliverable #25; branch retained if ever needed. |
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
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.
Draft for reading the migration diff in GitHub's viewer (fork-internal, do not merge).
Base = straylight fork infra on upstream v1.7.3 (pre-fixes). Head = migrate-1.7.3 (@ 0ac9413).
Scope: 18 of 22 verified fixes applied. FOUR held for human reconciliation because they conflict with the TL1-keepalive / redis-read paths: worker-replacement-evicts, n3-placement-version, eviction-rechecks-heartbeat, redis-read-integrity. Those four touch scheduler-timing paths, so any CI-speed/reliability compose benchmark on this branch is a LOWER BOUND on the full compose effect.
Source of record: straylight forge git.s4.gl/straylight/straylight-nativelink migrate-1.7.3.