Skip to content

Add ability to use env variables in config files - #88

Merged
allada merged 1 commit into
masterfrom
add-environmental-variable-lookup-to-config
Jun 17, 2022
Merged

Add ability to use env variables in config files#88
allada merged 1 commit into
masterfrom
add-environmental-variable-lookup-to-config

Conversation

@allada

@allada allada commented Jun 17, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

This will make it much easier to use the same config with slightly
different configs.

Also adds a few useful debug tips on errors.

This change is Reviewable

@allada
Add ability to use env variables in config files
0b777c6 
This will make it much easier to use the same config with slightly
different configs.

Also adds a few useful debug tips on errors.
allada
allada commented Jun 17, 2022
View reviewed changes

@allada allada left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @allada)

@allada
allada merged commit d54b38e into master Jun 17, 2022
@allada
allada deleted the add-environmental-variable-lookup-to-config branch June 17, 2022 18:22
rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
@rejuvenile @claude
TraceMachina#88 Phase 1: BatchReadCoalescer module + unit tests
9d73acd 
Server-side proxy-read opportunistic coalescer for small blobs.
Mirrors the SmallBlobDispatcher in spirit (per-target queue, zero-window
coalesce via tokio::task::yield_now, single-flight dedup), adapted for
the read direction (oneshot reply per slot, per-digest result fanout).

Wire protocol decision: REUSE REAPI BatchReadBlobs against the worker's
CAS endpoint. The worker's cas_server::inner_batch_read_blobs already
handles this RPC (no proto change). Justified over a custom proto in the
module doc.

Threshold: SMALL_BLOB_THRESHOLD = 16 KiB, shared with small_blob_dispatcher.
Eligibility helper rejects partial reads + above-threshold blobs.

Coalesce window: 0ms (one tokio::task::yield_now()), matching the
existing grpc_store batch_flush_loop pattern.

Tests + mutation results (all PASS, all mutations RED-FAILED):
- is_eligible_rejects_above_threshold
- is_eligible_rejects_nonzero_offset
- is_eligible_accepts_full_read_with_permissive_length
- five_concurrent_requests_become_one_batch (asserts 5 reqs -> 1 batch)
- same_digest_concurrent_requests_dedup (asserts dedup works,
  mutated `is_first = true` -> RED-FAILED with specific assertion msg)
- partial_failure_response_surfaces_per_digest_errors (mutated
  per-digest result distribution -> RED-FAILED with specific msg)
- above_threshold_blob_is_not_eligible_for_batch

Wire-up to WorkerProxyStore + production-composition test follow in
Phase 2.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
@rejuvenile @claude
TraceMachina#88 Phase 2: wire BatchReadCoalescer into WorkerProxyStore
7a3b9f6 
Adds an opt-in fast path that routes eligible small-blob reads through
the BatchReadCoalescer instead of issuing per-blob ByteStream Read RPCs.

WorkerProxyStore changes:
- New `batch_small_blob_reads: AtomicBool` (default OFF) +
  `batch_read_coalescer: OnceLock<Arc<BatchReadCoalescer>>` field.
- `enable_batch_small_blob_reads()` / `disable_batch_small_blob_reads()`
  / `batch_small_blob_reads_enabled()` setters/inspector mirroring the
  existing `enable_race_peers` / `enable_locality_in_has` pattern.
- `init_batch_read_coalescer(self_arc)` bootstraps the coalescer with a
  Weak<Self> capture so its BatchFn can lazily look up per-endpoint
  GrpcStore connections. Idempotent.
- `try_batched_read_from_endpoint(endpoint, digest, writer)`: helper
  that submits to the coalescer, then forwards bytes to the caller's
  writer (send + send_eof) and tees to the inner store via a
  fire-and-forget tokio::spawn. On Err, the writer is left untouched
  (per-peer-attempt convention preserved — caller falls through to
  the streaming get_part_and_cache path).
- BatchFn (production): obtains GrpcStore via downcast_ref, builds a
  REAPI BatchReadBlobsRequest, parses per-digest responses (with the
  same length-mismatch corruption guard used by the worker-side
  validate_batch_read_responses).
- BatchFn (test injection): when the connected store is NOT a
  GrpcStore (e.g. a MemoryStore in unit tests), falls back to per-blob
  `get_part_unchunked` so tests can exercise the coalescer end-to-end
  without a real gRPC server.
- Inserted batched fast path inside `try_read_from_endpoints` AND
  `try_read_from_worker`. Eligibility checks: flag on, original
  caller offset/length unchanged (no resume-from-mid-stream
  interference), `BatchReadCoalescer::is_eligible(digest, offset,
  length)` true. On batched-Err, eviction policy mirrors the
  streaming path (NotFound / DataLoss only); fall through to
  get_part_and_cache.
- New helper `code_from_grpc_status(i32) -> Code` mapping
  google.rpc.Status codes to nativelink_error::Code.

src/bin/nativelink.rs:
- Call `proxy_arc.init_batch_read_coalescer()` in the WorkerProxyStore
  wrapping loop so the coalescer is ready when the operator flips
  the kill-switch.

Existing 42 worker_proxy_store_test cases continue to pass (default
flag is OFF, so the new path is dormant in those tests).

Production-composition test (per CLAUDE.md feedback_writer_termination_class)
follows in Phase 3.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
@rejuvenile @claude
TraceMachina#88 Phase 3: production-composition tests for batched pro…
a47775f 
…xy reads

Adds nativelink-store/tests/worker_proxy_batch_read_test.rs:
6 tests wrapping VerifyStore -> WorkerProxyStore (the production
cas_STORE chain) and asserting the writer-termination contract is
preserved on the batched fast path.

Per CLAUDE.md feedback_writer_termination_class_2026_04_25 + the
asymmetric-coverage rule (under-action AND over-action both tested):

UNDER-action (writer not terminated when it should be):
- batched_path_terminates_writer_on_ok_under_verify_store
  Mutation: comment out `writer.send_eof()` in
  try_batched_read_from_endpoint -> RED-FAILED with the assertion msg
  "Sender dropped before sending EOF" (verified).

OVER-action (writer terminated when wrapper still wants it):
- batched_path_does_not_terminate_writer_on_err_under_verify_store
  Mutation: add `writer.send_error(e.clone())` to the Err path of
  try_batched_read_from_endpoint -> RED-FAILED with
  "Tried to send while stream is closed" surfacing through the
  streaming fallback to peer B (verified).

Eligibility coverage:
- above_threshold_blob_takes_streaming_path_under_verify_store
  (32 KiB blob, flag ON, must take streaming path)

Concurrency coverage:
- five_concurrent_small_reads_coalesce_into_one_batch_under_verify_store
  (5 same-endpoint distinct digests; correctness check, not a
  precise batch-count assertion — that's owned by the unit test
  in the coalescer module against a controlled BatchFn fake)
- three_concurrent_same_digest_reads_succeed_under_verify_store
  (single-flight dedup smoke under VerifyStore)
- delayed_peer_batched_read_holds_writer_open_under_verify_store
  (DelayedPeerStore fixture with 150ms delay; verifies the writer
  stays open across the async boundary)

Every test wraps `outer.get_part_unchunked` in a 5s
`tokio::time::timeout` per the deadlock-detector rule.

All 6 tests PASS post-restore. All other nativelink-store tests
continue to pass (failpoints test suite needs `--features failpoints`,
unchanged from baseline; one filesystem_store_test is a pre-existing
flake unrelated to this diff).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
@rejuvenile @claude
batch coalescer fixup: B1-B4 blockers + M1/M2 majors from review
b39f009 
Address 4 BLOCKERS + 3 MAJORS from the post-rebase reviewer pass on TraceMachina#88
(batch_read_small_blobs racing wastes bandwidth):

B1 (code-reviewer + perf-optimizer): IS_WORKER_REQUEST not propagated
across the spawned drainer's BatchFn. tokio::spawn does NOT inherit
tokio::task_local! storage; the drainer's batch_read_blobs RPC was
called WITHOUT IS_WORKER_REQUEST=true, so the receiving worker did not
enter responder mode (loop-terminator invariant: "workers NEVER chain
externally" enforced at depth 1). Fix: capture the flag at submit()
time, ferry it via PendingRead, re-establish via
IS_WORKER_REQUEST.scope(...) around the BatchFn invocation in the
drainer. Regression test
drainer_propagates_is_worker_request_across_spawn observes the flag
inside the BatchFn fake — mutation-verified.

B2 (perf-optimizer): DigestHasherFunc context lost across the spawn
for the same reason as B1. Hard-coded SHA-256 broke non-SHA-256
deployments (every batch returned InvalidArgument from the worker side
+ silent fallback). Fix: capture the hasher from OTel Context at
submit() time, pass it as a 3rd arg to BatchFn so the outgoing
BatchReadBlobsRequest.digest_function field matches the caller's
hasher. Regression test
drainer_propagates_digest_hasher_across_spawn — mutation-verified.

B3 (perf-optimizer): redirect-path try_read_from_endpoints was missing
the bytes_before_proxy == writer.get_bytes_written() guard that the
streaming-path try_read_from_worker callsite already had. Endpoint A
streaming partial-write followed by endpoint B batched whole-blob send
would silently corrupt the consumer-visible stream
(partial_A_bytes ++ full_B_bytes). Fix: capture bytes_before_proxy
once before the endpoints loop; gate the batched fast-path on
writer.get_bytes_written() == bytes_before_proxy per the streaming-
path pattern. Regression test
redirect_path_skips_batched_after_partial_write_to_writer uses a
PartialThenErrorPeer fixture for endpoint A + a healthy MemoryStore
for endpoint B; asserts batches_dispatched <= 1 (at most A's failed
attempt, NEVER B's). Mutation-verified.

B4 (testing-czar): 7 unit tests in batch_read_coalescer.rs used
#[tokio::test] instead of CLAUDE.md-mandated #[nativelink_test].
Converted all 7.

M1 (code-reviewer): submit() took Arc<str>, allocating one fresh Arc
per submission (defeats the perf benefit of coalescing). Now takes
&str; allocates Arc<str> only on the insert-miss path inside submit
when a new per-endpoint queue is created. Caller updated.

M2 (rust-crate-reviewer): replace 17-line hand-rolled
code_from_grpc_status with tonic::Code::from_i32 (re-exported as
nativelink_error::Code). Old fn returned Internal for unknowns;
from_i32 returns Unknown — both pass should_try_peers, semantically
equivalent for the caller's fallback. NOTE comment added.

M1 (rust-crate-reviewer): import order — moved opentelemetry::context
out of the nativelink_util::* block; subsequently fully removed since
the OTel Context is no longer read directly in worker_proxy_store
(batch_digest_function is now passed as an argument to BatchFn).

Mutation steps verified for B1, B2, B3 individually:
- B1: comment out IS_WORKER_REQUEST.scope wrapper in drainer →
  drainer_propagates_is_worker_request_across_spawn FAILED
- B2: replace batch_digest_function with default_digest_hasher_func()
  in BatchFn call → drainer_propagates_digest_hasher_across_spawn FAILED
- B3: remove bytes_before_proxy guard at redirect path →
  redirect_path_skips_batched_after_partial_write_to_writer FAILED

Deferred per task instructions (separate trackers):
- testing-czar M1-M4 coverage gaps (init-not-called, kill-switch
  flip mid-flight, 16 KiB boundary, partial-read offset/length)
- rust-crate L1-L8 polish (MetricsComponent, new_cyclic, comment
  cleanup)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
@rejuvenile
nativelink: enable batch_small_blob_reads (TraceMachina#88) and small…
436b844 
…_blob_mirror (TraceMachina#168)

Per operator authorization 2026-05-05, promote two production gates from
default-OFF to ON at server startup:

- TraceMachina#88 (BatchReadBlobs coalescing): call
  `WorkerProxyStore::enable_batch_small_blob_reads()` immediately after
  `init_batch_read_coalescer()` on every CAS proxy wrap. Concurrent
  same-target small-blob server→worker proxy reads now collapse into
  one BatchReadBlobs RPC per coalesce window per endpoint instead of
  N individual ByteStream Read RPCs.

- TraceMachina#168 (small-CAS peer mirror): construct `SmallBlobDispatcherConfig`
  with `small_blob_mirror_enabled: true` instead of
  `::default()` (which had it false). Promotes the dispatcher's
  `enqueue` from inert no-op to live mirror-push. The dispatcher
  remains gated on per-store pin-set registration below, so stores
  without a FastSlowStore backing are still skipped.

Pre-existing flake `test_execute_dependent_actions_with_peer_sharing`
also fails on origin/main HEAD (verified) — unrelated to this change.
rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
@rejuvenile @claude
#linkperf-3: observable win-rate for the worker server-vs-peer get_pa…
c0b1d37 
…rt race

Adds `race_peer_win_total` / `race_server_win_total` counters to WorkerProxyStore
so the worker-side server-vs-peer `get_part` race's effectiveness is readable in
production. The win/loss outcome was previously ONLY at `debug!`, which
`release_max_level_info` compiles out of the prod binary — the race ran dark.

SCOPE NOTE / design-drift (reported to dispatcher): the brainstorm's TraceMachina#3 premise —
"the LARGE branch ... does NOT race fastest-of {server, peer-A, peer-B}" — is
largely FALSE against current code. The large input path
(download_to_directory large branch -> populate_fast_store_unchecked ->
copy_slow_to_fast -> run_producer -> slow_store.get -> WorkerProxyStore::get_part)
ALREADY races the inner CAS against peers[0] with PROMPT loser cancellation
(`cancel_loser_racer`: cooperative rx-drop + 50ms LOSER_GRACE + abort fallback)
— landed in `7457436d`, enabled in prod at `local_worker.rs:4733`
(`enable_race_peers`). The `TraceMachina#88` bandwidth-doubling/loser-abort concern the
dispatch flagged is therefore ALREADY solved. The only genuine residual gaps are
(a) it races a SINGLE peer (peers[0]), not multiple peers, and (b) no
load-spreading across peers for the large class (unlike the small path's
round-robin). Both are an ARCHITECTURAL extension to an already-tuned race (the
RST-budget note at `cancel_loser_racer` documents why parallel_chunk_count was
cut 64->16 to stay under hyper's reset budget) — re-implementing/multiplying the
race needs sign-off, so this commit ships ONLY the missing observability the
dispatch explicitly asked for and flags the rest.

Invariant being violated (pre-fix): the win-rate of the production server-vs-peer
race is not observable in the release binary (debug!-only), so an operator cannot
tell whether peer-fetch is offloading the central CAS or whether the race is pure
overhead (near-zero peer wins) — and cannot size the bandwidth-doubling cost.

Mechanism that violated it: the 4 winner-naming branches in
`WorkerProxyStore::get_part`'s `tokio::select!` and the two fallback helpers only
emitted `debug!`, compiled out under release_max_level_info.

Mechanism that re-establishes it: two `CounterWithTime` fields incremented at
EVERY serving point — the 2 primary data branches + 2 zero-length branches in the
select (`get_part`), and the success points of `await_peer_after_empty_server` /
`await_server_after_empty_peer` (now `&self` methods). "Who ultimately served the
bytes" => complete, path-independent win-rate. Published via the existing
`publish!` block in `MetricsComponent::publish` (so these ARE on /metrics, unlike
the scheduler-side prefetch counters).

Composite invariants this fix interacts with: none structurally — counting is
side-effect-only on existing branches. The two helpers changed from associated
fns to `&self` methods (4 call sites in `get_part`, all updated); no signature or
behavior change beyond the `inc()`. No change to racing, loser-cancellation,
stale-positive eviction, writer-termination, ack semantics, or store-chain shape.

Test that proves the fix: test_race_peer_win_increments_peer_counter (inner empty
-> peer serves -> peer counter +1, server counter unchanged) and
test_race_server_win_increments_server_counter (peer stale-positive empty ->
server serves via fallback -> server counter +1, peer counter unchanged). Both in
production composition (race_peers=ON, INITIATOR mode). TDD fail->pass->mutate
verified: commenting all peer inc sites red-fails the peer test with "peer win
must increment race_peer_win_total (was 0, now 0)"; same for server.

Behavior changes:
- WorkerProxyStore (worker-side, race_peers=ON; prod composition wrapping the
  central-CAS GrpcStore as FastSlowStore.slow): on every raced get_part, ALSO
  increments race_peer_win_total or race_server_win_total. Prior: debug!-only.
  New: two registered counters. No runtime path/timing/ack change. Server-side
  WorkerProxyStore (race_peers=OFF) never enters the race branches, so the
  counters stay zero there — unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@allada