Repository navigation
Fix inefficient upload of stderr/stdout in workers - #218
Merged
Merged
Conversation
There was a serial operation of uploading stderr and stdout then uploading the files. This PR makes it so they get uploaded in parallel. fixes: #212
allada
commented
Jul 21, 2023
allada
left a comment
allada
left a comment
Contributor
Author
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @allada)
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…rage tests Closes the deferred test gaps from `.claude/reviews/212-phase2.1-fs-primitives/testing-czar.md`: M2 (per-blob serialization claim untested in TraceMachina#212 Phase 2.1): - chunked_filesystem_serializes_concurrent_writes_to_same_blob: 5 concurrent same-blob writes at 5 non-overlapping offsets through the per-blob async mutex; asserts byte-exact final file as the serialization correctness signal. Mutation-verified by swapping the per-blob `entry.file.lock().await` to `try_lock().expect(...)`: test fails with `MUTATION: per-blob mutex must be uncontended` panic. - chunked_filesystem_parallelizes_concurrent_writes_across_blobs: 5 concurrent cross-blob writes; wall-clock ceiling = 3x single-write baseline + 50ms (vs. 5x serialized lower bound). Mutation-verified by inserting a global `tokio::sync::Mutex` + 50ms sleep around the per-call critical section: test fails with the bespoke "cross-blob writes must parallelize" panic at ~258ms vs 212ms ceiling. M3 (over-action lifecycle gaps): - discard_chunked_on_never_written_blob_is_noop_ok: discard on a digest that was never opened is Ok(()) and does NOT touch other digests' partials (cross-blob isolation). Mutation-verified by replacing the early `return Ok(())` with `return Err(NotFound)`: test fails with the "must return Ok (idempotent contract)" panic. - commit_chunked_after_successful_commit_returns_err: double-commit surfaces an Err so the driver can distinguish double-fire from happy-path. Today returns NotFound (in-flight entry removed by stage-2 finalize); FIXME(TraceMachina#218) flag for spec to pin the variant. Mutation-verified by changing `entry.ok_or_else(NotFound)` to a silent `return Ok(())` for absent entries: test fails with the bespoke "silent re-commit would mask driver double-fire bugs" panic. - commit_chunked_zero_byte_blob_with_no_writes: pins current behavior (returns NotFound because no `open_or_create_partial` happened); documents the FIXME(TraceMachina#218) gap that the spec read as "must succeed via empty-file rename". Phase 2.3 driver wiring must resolve in one of two documented ways. Mutation-verified by changing the source's `Code::NotFound` to `Code::Internal`: test fails with the bespoke FIXME(TraceMachina#218) panic naming the alternative. Each test wraps awaits under `tokio::time::timeout(few seconds)` with a specific bespoke panic message (CLAUDE.md "Asymmetric Contract Coverage" + "Test in production composition" rules). Tests run against a real `FilesystemStore` (`production composition`), exercising the content_path/temp_path layout end-to-end. Stability: 5/5 runs pass. Out of scope (filed for follow-up): length-aware rename-recovery (spec Q7=(c) "if length == declared_size, RENAME-TO-FINAL; else GC") depends on Phase 2.3 driver wiring; tracked separately. 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
…t (Resolution B) Resolves the FIXME at `chunked_filesystem_serialization_test.rs:: commit_chunked_zero_byte_blob_with_no_writes`. Per the TraceMachina#212 Phase 2.1 M3 spec ("zero-byte commit must succeed via empty-file rename"), but under the current Phase 2.1 primitive a true zero-byte blob has no `write_chunk_at_offset` call (the writer correctly short-circuits on empty input), so no `open_or_create_partial` ever fires and `commit_chunked_to_holding`'s map lookup returns NotFound. **Resolution B**: at the top of `commit_chunked_to_holding`, if `expected_size == 0` AND no in-flight partial exists in the map, short-circuit to a direct empty-file `create` at `holding_path` via `tokio::task::spawn_blocking` (sync FS I/O off the tokio worker per CLAUDE.md). Stage 2 (`finalize_holding`) then renames `.holding` → canonical and chmods to 0o555 unchanged. The fast-path is gated on BOTH conditions: - `expected_size == 0` — true zero-byte blob - `!has_entry` — no prior `open_or_create_partial` So zero-byte blobs that happened to open a partial (via an explicit `open_or_create_partial` call) still flow through the existing length-check branch unchanged: `actual_len == 0 == expected_size` → rename succeeds. Localized to the commit primitive — no changes to `write_chunk_at_offset` and no driver-level shortcut. Resolution A from the test's prior FIXME comment was rejected because it would have required a sentinel write or wrapper at every caller. `std::fs::File::create` (NOT `create_new`) intentionally chosen so duplicate same-process commits remain idempotent — `prune_holding_ partials` GCs stale holding files at startup, and the file is empty, atomic, and identical-content under any commit. Test: `commit_chunked_zero_byte_blob_with_no_writes` — flipped from "either branch acceptable" to "must succeed via direct empty-file rename". Mutation step: replaced the fast-path block with `if false` → test FAILED in 0.16s with bespoke message `"zero-byte commit (no prior writes) must succeed via direct empty-file rename per spec — actually returned: Err(NotFound)"`. Restored fast-path → 5/5 runs at ~0.21-0.23s pass. 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
Addresses findings from the 4-reviewer pass on the 5-commit salvage bundle (TraceMachina#247, TraceMachina#244, TraceMachina#246, FIXME(TraceMachina#218), TraceMachina#247-cancellation-safety): **testing-czar MAJOR-1** — `27498447`'s cancellation-safety property was structurally untested. Adds `finalize_holding_uses_background_spawn_for_cancellation_safety_247`, a structural test that scans `filesystem_store.rs::finalize_holding` for `background_spawn!`. Behavioral tests for this property are fundamentally racy (the rename→insert window is microseconds; abort either wins the race against the rename or loses to the inline insert — see test doc-comment for details). The structural test catches the specific regression direction the reviewer named ("simplifying the spawn back to `.await`") with zero timing dependency. Mutation step verified: replace `background_spawn!` with `tokio::spawn` in the source → test red-fails with bespoke `TraceMachina#247 cancellation-safety` message. **testing-czar MINOR-2** — FIXME(TraceMachina#218) zero-byte test bypassed the `has_with_results` index-visibility contract (used `tokio::fs::read` + `tokio::fs::metadata` on the kernel-view). Adds the missing post-`finalize_holding` `has_with_results` assertion. CLAUDE.md "Index-visibility contract" rule explicitly forbids the substitution this test was making. **testing-czar MINOR-4** — adds `chunked_commit_visible_through_existence_cache_store_247` which wraps the FilesystemStore in `ExistenceCacheStore` to lock in the production-composition contract. Today this is functionally equivalent to the FilesystemStore-only test (because ExistenceCacheStore does not cache negatives), but the equivalence is fragile to a future "negative caching" change. **testing-czar MINOR-1** — adds `forward_racer_does_not_abort_racer_task_on_bind_buffered_ok_244` positive-control test for TraceMachina#244. Doc-comment is honest about the mutation-step limitation: hoisting `handle.abort()` outside the `if let Err` arm does NOT trip the test, because the racer task has already completed (its tx-drop is what triggered bind_buffered's EOF) by the time the abort lands — `abort()` on a completed task is a no-op. The test still has value as positive coverage of the peer-winner success path. **testing-czar MINOR-3** — DEFERRED. The dropped TraceMachina#246 mid-stream send-Err test would require a coordination mechanism (`tokio::sync::Notify`) rather than just a buffer-overflow body to reliably fire `verify_store.rs:188`'s `tx_guard.fail` arm. Tracker to follow. **code-reviewer M1** — drops precise line numbers in `writer_termination_245_drop_fallback_test.rs` doc-comments (`compression_store.rs:334` etc.) in favor of identifier-based references (`compression_store.rs CompressionStore::update write_fut WriteHalfGuard wrap`). Survives future doc-only line drift. **perf-optimizer MINOR-3** — cross-references the `write_chunk_at_ offset` empty-chunk no-op (`chunked_filesystem.rs:423`) to the FIXME(TraceMachina#218) zero-byte fast-path so a future change to that early- return doesn't silently flip the fast-path's `!has_entry` predicate. **Verified post-fix:** - 5/5 affected test binaries pass (3 in TraceMachina#247-test, 2 in TraceMachina#244-test, 5 in TraceMachina#218-test, 6 in verify_store_245-test, 2 in writer_term-test — 18 tests, all green). - MAJOR-1 mutation step (replace `background_spawn!` → `tokio::spawn` in finalize_holding) red-fails the new structural test with the bespoke `TraceMachina#247 cancellation-safety` message. **NIT-1 + NIT-2 deferred** — pure NITs; defer to a follow-up cleanup or never. No behavior risk. 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
…mitigation) Today's TraceMachina#253 production OOM (19:45:39 PDT, anon-rss 83.6 GB / 80 GB cap) reproduces TraceMachina#197 (2026-04-27) exactly: log-spam-driven allocator pressure with mimalloc PAGE_FULL_RETAIN=8 pinning freed pages. Pre-OOM rate was ~1073 lines/sec sustained, dominated by diagnostic info! calls added during recent TraceMachina#171, TraceMachina#212, TraceMachina#218 instrumentation passes. Demote those calls to debug! so they remain available via RUST_LOG=debug but stop producing per-chunk allocator pressure in release builds. Demotions (info! -> debug!, structured fields and messages preserved): nativelink-service/src/bytestream_server.rs 1290 inner_read awaiting consume_fut (per-chunk, hottest) 1303 inner_read consume returned empty (EOF) (per-stream-end) 1318 inner_read consume returned chunk (per-chunk) nativelink-store/src/worker_proxy_store.rs 1884 WorkerProxyStore::get_part_sequential awaiting inner.get_part 1906 WorkerProxyStore::get_part_sequential inner.get_part returned nativelink-store/src/fast_slow_store.rs 1852 populate run_producer entry 3234 FastSlowStore::update background slow write spawned nativelink-scheduler/src/api_worker_scheduler.rs 2916 broadcast_blobs_in_stable_storage_chunked dispatching These are diagnostic logs added to investigate specific bugs (TraceMachina#171 inner-miss flow, populate join3 ordering, BIS chunked dispatch); the underlying bugs are now fixed but the info-level emission was never demoted, re-running the same OOM cascade after every deploy. The behavior path is unchanged - only the tracing level moves. 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
…a#262) During the 2026-05-05 RSS-leak investigation a Bazel fetch-storm drove RSS to a 27 GB plateau. Six info! log sites dominated the burst-time output (cumulatively ~2300 events / 30 s) and compounded with mimalloc page retention. Each is leftover diagnostic from TraceMachina#212/TraceMachina#218/TraceMachina#252 chunked-streaming work. Same shape as TraceMachina#255 / TraceMachina#186 / TraceMachina#253. Demoted sites: - bytestream_server.rs:385 "ByteStream::read: CAS read completed" 525 events / 30 s. Just bumped to info! today via e59b776 (LoggingReadStream consolidation). The 17 lines/sec sustained rate is too high for info! even though it carries throughput/duration — the CLAUDE.md rule "info! for transfer completions with throughput" was written for whole-RPC summaries, not per-stream-chunk reads. - bytestream_server.rs:1007 "ByteStream::inner_read entry" 525 events / 30 s. Per-call entry log; debug! is the right level. - bytestream_server.rs:1369 "inner_read get_part_fut resolved (still awaiting consume)" 520 events / 30 s. Per-event branch trace. Sibling at :1332 (`after consume_err`) left at info! — fires only on the rarer error path. - bytestream_server.rs:2010 "ByteStream::write: skipped, blob already exists" 377 events / 30 s. The WorkerProxyStore::has() write-skip optimization log; TraceMachina#133 already filed to demote it. - streaming_blob.rs:325 "streaming blob writer sent eof, notify_waiters firing" 293 events / 30 s. Per-blob lifecycle event; the wakeup propagates to readers without needing a separate log. - existence_cache_store.rs:147,154,156 eviction-callback trio (~258 events / 30 s each). Per-eviction mechanical follow-ups. Wedge-digest gates at :150 left at info! — they're explicitly filtered to a single digest under investigation. - fast_slow_store.rs:1939 "populate sending streaming_writer.send_error after head failure" 241 events / 30 s. Per-failed-RPC mechanical follow-up. The summary line at :1927 ("populate head_result Err") left at info! — one line per failure is the right cadence. streaming_blob.rs's `info` import removed (now unused). All four files already had `debug` in scope. Behavior-only change: no flow-control, concurrency, or storage semantics modified. Build verified --release --features quic,pprof,chunked_fast_slow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
There was a serial operation of uploading stderr and stdout then uploading the files. This PR makes it so they get uploaded in parallel.
fixes: #212
This change is