Skip to content

Integration test should work for Mac OS. - #334

Merged
MarcusSorealheis merged 1 commit into
TraceMachina:mainfrom
MarcusSorealheis:fix-integration-for-macOS
Oct 21, 2023
Merged

MarcusSorealheis merged 1 commit into
TraceMachina:mainfrom
MarcusSorealheis:fix-integration-for-macOS

Conversation

@MarcusSorealheis

@MarcusSorealheis MarcusSorealheis commented Oct 21, 2023 •

Copy link
Copy Markdown
Member

Description

Minor issues related to MacOS for the integration tests.

Fixes # #316

Not technically an issue. It's blocking an in-progress PR.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please also list any relevant details for your test configuration

Checklist

  • Updated documentation if needed
  • Tests added/amended
  • bazel test //... passes locally
  • PR is contained in a single commit, using git amend see some docs

This change is Reviewable

@allada allada left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @MarcusSorealheis)


run_integration_tests.sh line 19 at r1 (raw file):

if [[ $EUID -eq 0 ]]; then
  echo "This script shoudl not be run as root."

Misspelled.


run_integration_tests.sh line 49 at r1 (raw file):

done

echo "What operating system OSTYPE: $OSTYPE"

This ENV is not set on all OSs.


run_integration_tests.sh line 59 at r1 (raw file):

  TEST_PATTERNS=("*")
fi

Needless new line.


run_integration_tests.sh line 73 at r1 (raw file):

sudo docker-compose rm --stop -f

echo "What operating system OSTYPE: $OSTYPE"

nit: ditto


run_integration_tests.sh line 77 at r1 (raw file):

export TMPDIR=$HOME/.cache/turbo-cache/
sudo mkdir -p "$TMPDIR"

don't mkdir with sudo, it causes issues.


run_integration_tests.sh line 79 at r1 (raw file):

sudo mkdir -p "$TMPDIR"

if [[ "$OSTYPE" == "linux-gnu"* ]]; then

use ${OSTYPE:-linux-gnu}


run_integration_tests.sh line 82 at r1 (raw file):

  export CACHE_DIR=$(mktemp -d --tmpdir="$TMPDIR" --suffix="-turbo-cache-integration-test")
elif [[ "$OSTYPE" == "darwin"* ]]; then
  # Create a temporary directory using mktemp with a random template, then add a suffix

nit: end in a period.


run_integration_tests.sh line 83 at r1 (raw file):

elif [[ "$OSTYPE" == "darwin"* ]]; then
  # Create a temporary directory using mktemp with a random template, then add a suffix
  export CACHE_DIR=$(mktemp -d "${TMPDIR}turbo-cache-integration-test")

nit: add slash between ${TMPDIR} and the constant.


run_integration_tests.sh line 84 at r1 (raw file):

  # Create a temporary directory using mktemp with a random template, then add a suffix
  export CACHE_DIR=$(mktemp -d "${TMPDIR}turbo-cache-integration-test")
  echo "CACHE_DIR for macOS is: $CACHE_DIR"  # <-- Added print statement here

nit: remove this.


run_integration_tests.sh line 85 at r1 (raw file):

  export CACHE_DIR=$(mktemp -d "${TMPDIR}turbo-cache-integration-test")
  echo "CACHE_DIR for macOS is: $CACHE_DIR"  # <-- Added print statement here
  # exit 1

nit: remove this.


run_integration_tests.sh line 88 at r1 (raw file):

else
  # Unknown OS
  echo "Unknown operating system. Exiting."

nit: Lets instead invert this and assume linux if it's not mac.


run_integration_tests.sh line 106 at r1 (raw file):

    # Cleanup.
    echo "Cleaning up cache directories TURBOC_CACHE_DIR: $TURBO_CACHE_DIR"
    

nit: Needless spaces.

@MarcusSorealheis
MarcusSorealheis force-pushed the fix-integration-for-macOS branch 2 times, most recently from c6980b0 to 6b37ecf Compare October 21, 2023 16:31

@MarcusSorealheis MarcusSorealheis left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 10 unresolved discussions (waiting on @allada)


run_integration_tests.sh line 73 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: ditto

Removed.


run_integration_tests.sh line 77 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

don't mkdir with sudo, it causes issues.

Done


run_integration_tests.sh line 79 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

use ${OSTYPE:-linux-gnu}

Done.


run_integration_tests.sh line 83 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: add slash between ${TMPDIR} and the constant.

$TEMP_DIR has a trailing slash.


run_integration_tests.sh line 88 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: Lets instead invert this and assume linux if it's not mac.

what about Windows? Some customers use Windows.

@MarcusSorealheis MarcusSorealheis left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 10 unresolved discussions (waiting on @allada)


run_integration_tests.sh line 79 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

Done.

looks like it should be linux-gnu as I had before

@MarcusSorealheis MarcusSorealheis left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 10 unresolved discussions (waiting on @allada)


run_integration_tests.sh line 79 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

looks like it should be linux-gnu as I had before

https://github.com/TraceMachina/turbo-cache/actions/runs/6598502029/job/17926538604

@MarcusSorealheis MarcusSorealheis left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 10 unresolved discussions (waiting on @allada)


run_integration_tests.sh line 79 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

https://github.com/TraceMachina/turbo-cache/actions/runs/6598502029/job/17926538604

Should I write a regex for either or?

@MarcusSorealheis
MarcusSorealheis force-pushed the fix-integration-for-macOS branch 6 times, most recently from 5418163 to 105f34a Compare October 21, 2023 18:08
@MarcusSorealheis

Copy link
Copy Markdown
Member Author

@allada ball in your court but curious about your thoughts on $OS_TYPE.

@MarcusSorealheis MarcusSorealheis left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

My overall opinion is that this works but I do not want to put too much effort into it given it will go away in the near term.

Reviewable status: 1 of 2 files reviewed, 9 unresolved discussions (waiting on @allada)


run_integration_tests.sh line 19 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Misspelled.

Done.


run_integration_tests.sh line 59 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Needless new line.

Done.

@allada allada left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @MarcusSorealheis)


run_integration_tests.sh line 79 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

Should I write a regex for either or?

The command I gave you just defaults OSTYPE to linux-gnu if it is not set.

It should look like this:

if [[ "${OSTYPE:-linux-gnu}" == "linux-gnu"* ]]; then

run_integration_tests.sh line 88 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

what about Windows? Some customers use Windows.

This test doesn't work on windows anyway.

@MarcusSorealheis
MarcusSorealheis force-pushed the fix-integration-for-macOS branch from 68f6947 to 051f999 Compare October 21, 2023 20:21

@MarcusSorealheis MarcusSorealheis left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @allada)


run_integration_tests.sh line 49 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

This ENV is not set on all OSs.

ahh. I see what you meant. This script would fail on those platforms.


run_integration_tests.sh line 79 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

The command I gave you just defaults OSTYPE to linux-gnu if it is not set.

It should look like this:

if [[ "${OSTYPE:-linux-gnu}" == "linux-gnu"* ]]; then

that wasn't clear. I get default parameter, though. I thought you only specify the string.

@MarcusSorealheis MarcusSorealheis left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @allada)


run_integration_tests.sh line 79 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

that wasn't clear. I get default parameter, though. I thought you only specify the string.

I think this is fine now

@allada allada left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r7.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @MarcusSorealheis)


run_integration_tests.sh line 79 at r7 (raw file):

  export CACHE_DIR=$(mktemp -d "${TMPDIR}turbo-cache-integration-test")
else
  # Unknown OS

nit: needless comment.


run_integration_tests.sh line 100 at r7 (raw file):

    echo "Checking for existince of the TURBO_CACHE_DIR"
    if [ -d "$TURBO_CACHE_DIR" ]; then
      sudo find "$TURBO_CACHE_DIR" -delete # add for linux

nit: Needless comment.

@MarcusSorealheis
MarcusSorealheis force-pushed the fix-integration-for-macOS branch from 051f999 to 56afc81 Compare October 21, 2023 21:45

@allada allada left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @MarcusSorealheis)


run_integration_tests.sh line 79 at r8 (raw file):

else
  echo "Unable to detect operating system. Assuming the Linux/WSL syntax will work."
  export CACHE_DIR=$(mktemp -d --tmpdir="$TMPDIR" --suffix="-turbo-cache-integration-test")

Since we are doing the same thing in:
if [[ "${OSTYPE:-linux-gnu}" == "linux-gnu"* ]]; then

Can we instead just check darwin and then assume linux otherwise and get rid of this else clause?

@MarcusSorealheis MarcusSorealheis left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @allada)


run_integration_tests.sh line 79 at r8 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Since we are doing the same thing in:
if [[ "${OSTYPE:-linux-gnu}" == "linux-gnu"* ]]; then

Can we instead just check darwin and then assume linux otherwise and get rid of this else clause?

sure

@MarcusSorealheis
MarcusSorealheis force-pushed the fix-integration-for-macOS branch from 56afc81 to 1faf0ca Compare October 21, 2023 21:53

@allada allada left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @MarcusSorealheis)

rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
…ow writes (TraceMachina#334 fix-A/B)

Two fixes for the production cascade observed today (2026-05-08, 67 GB
RSS at 1h15m, 2743 errors/min, 723 unique 16KB-1MB digests).

**Fix A — typed BackpressureSignal preservation.** When the fast tier
(MemoryStore) early-rejects with the wire-stable
ResourceExhausted+BackpressureSignal::MemoryStoreAtCapacity, it drops
fast_rx, the data-stream future's send fails with channel-closed, the
closure wraps that as generic Code::Internal, and the pre-existing match
returned the Internal BEFORE checking fast_res — masking the typed
signal. Bazel does not honor backoff hints on Internal, so retries are
immediate, in_flight grows, MemoryStore at cap rejects more, cascade.

Fix: after the join!, check fast_res for Code::ResourceExhausted carrying
a BackpressureSignal detail; if present, surface fast_res to the caller
before evaluating data_res. Strict heuristic matches Code AND detail
(option a in the task brief) — bare ResourceExhausted from a future
fast-tier store kind does not match. Sibling guard applied to the
chunked-dispatcher path (latent today; fires if MemoryStore ever
early-rejects on that path).

**Fix B — aggregate slow-write byte cap.** in_flight_slow_writes pinned
every payload until the background slow-write terminated; under slow-tier
wedge that grew unbounded. New FastSlowSpec.slow_writes_in_flight_max_bytes
(default 8 GiB) caps it via a typed
BackpressureSignal::SlowWritesAtCapacity rejection (new proto variant in
the same wire-stable enum). Counter increments after insert under the
in_flight_slow_writes mutex, decrements before remove under the same
mutex; check is best-effort snapshot before the spawn. Zero = uncapped
(historic behavior bit-identical for opt-out callers).

Tests in fast_slow_store_334_backpressure_preservation_test.rs cover
under-action + over-action + interaction paths per CLAUDE.md asymmetric
contract coverage.

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
…-precondition race

Without `tokio::task::yield_now()` in the test producer, send_fut
completes synchronously before update_fut polls — data_stream_fut wins
the send-vs-fast-rx-drop race, fast_res Err arrives via the regular
`fast_res?` path that already preserves the typed signal, and the test
PASSES even when Fix A is disabled (mutation step gives a false green).

The yield reliably suspends the producer so the inner FastSlowStore
poll order matches production: data_stream_fut hits
`reader.recv().await` (suspends, no data yet), fast_store_fut polls
and runs MemoryStore's ExactSize early-reject (returns Err, drops
fast_rx), data_stream_fut wakes when send_fut's first send arrives,
calls (*fast_guard).send().await, fails with channel-closed, returns
the wrapped Code::Internal error. Now mutation step Fix A red-fails
with the canonical production error message.

Also remove the stale MUTATION STEP comment from fast_slow_store.rs
that survived a mid-development edit cycle.

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#334 fix-AB fixup)

Reviewer findings on the Fix A+B bundle:

1. **perf BLOCKER + security MEDIUM**: `get_part`'s defensive size-mismatch
   guard at `:4856-4861` removed from `in_flight_slow_writes` without
   decrementing `in_flight_slow_writes_bytes`. Each activation permanently
   shrinks the effective cap; over a long-lived process the cap drifts to 0
   and rejects every admission with `SlowWritesAtCapacity`. Mirror the
   canonical `:4218-4222` decrement pattern.

2. **dsr MAJOR + red-team blind spot**: cap-rejection at `update:4023` and
   `update_oneshot:4330` previously left the digest invisible to
   `failed_slow_writes` drain, so the server-side `failed_writes_drain` →
   `UploadMissingBlobs` recovery loop could not pick it up. If the upstream
   client retry budget exhausted, bytes aged out of MemoryStore (PIN_TIMEOUT
   = 120s) with no recovery. Now mirrors the chunked dispatcher's symmetric
   error site at `:1229-1232`: insert into `failed_slow_writes` + re-pin
   BEFORE returning the typed error.

3. **perf MAJOR + code-reviewer H2**: doc-comments at `:529-538` and
   `:4053-4060` (post-Fix-B) misdescribed the lock/counter ordering. The
   actual semantics are eventually-consistent: insert under lock, fetch_add
   OUTSIDE lock, remove under lock, fetch_sub under lock. Rewrite to
   accurately describe the asymmetry and the `tokio::spawn`-ordering
   invariant that prevents underflow.

4. **code-reviewer H3**: `slow_writes_in_flight_max_bytes` was an
   `AtomicU64` but is set once at construction with no setter. Convert to
   plain `u64`.
rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
…TraceMachina#334 fix-AB fixup)

Reviewer findings:

1. **code-reviewer H1 + dsr M2 + testing-czar**: new variant
   `BackpressureSignal::SlowWritesAtCapacity` was not added to the existing
   classifier-iteration tests
   (`looks_like_dead_channel_respects_backpressure_signal`,
   `new_fixup_variants_roundtrip_and_classify`). A future change to either
   helper could silently regress for the new variant. Add to both
   iteration lists.

2. **red-team blind spot**: `SLOW_WRITES_AT_CAPACITY_RETRY_MS = 100` had
   no rationale comment for the 4× difference vs
   `MEMORY_STORE_BACKPRESSURE_RETRY_MS = 25`. MemoryStore eviction
   resolves on single-digit-ms timescale; slow-tier ack on hundreds-of-ms.
   Document.

3. **red-team TraceMachina#3 + dsr M4**: 8 GiB server cap default needed an explicit
   per-host rationale and worst-case overshoot bound. Document both.

4. **code-reviewer C1 + dsr M4**: worker `slow_writes_in_flight_max_bytes:
   0` lacked any explanatory comment — looked like a copy-paste oversight.
   Per parent decision: keep `: 0` on worker (worker's slow tier is
   GrpcStore→server, bounded by upstream h2 windowing + server-side cap).
   The 2026-05-08 OOM was on buildcache, not on workers. Add explicit
   comment justifying the choice at both worker FSS construction sites.
rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
… fixup)

Reviewer findings (code-reviewer C2): the new
`slow_writes_in_flight_max_bytes: 0` field was inserted at 12-space
indent across ~37 test files where surrounding fields use 8 spaces.
Run rustfmt on every file in the branch's diff to fix; the bulk of
the line churn is rustfmt's reformatting of other pre-existing
non-conformant style in the same files, which CI's `cargo fmt`
gate would have rejected anyway.
rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
…raceMachina#334 fix-AB fixup)

Reviewer findings:

1. **testing-czar BLOCK-1 + dsr (Fix A over-action)**: existing test family
   covered Fix A under-action only. Added
   `fix_a_does_not_over_match_bare_resource_exhausted` — a new
   `BareResourceExhaustedFastStore` returns `Code::ResourceExhausted` with
   NO `BackpressureSignal` detail; the Fix A heuristic
   (`Code::ResourceExhausted && error_has_backpressure_signal(e)`) MUST
   NOT match. Mutation-step: dropping the `error_has_backpressure_signal`
   clause from the predicate red-fails the test with the bespoke
   "Fix A predicate over-matched" message.

2. **testing-czar BLOCK-2 (Fix B failure-path counter decrement)**: covers
   the failure arm at `fast_slow_store.rs:4196-4221`. Added a
   `set_fail_after_release` flag to `GatedSlowStore` so the spawned
   slow-write returns `Err(Internal)` after the gate releases. Asserts
   `in_flight_slow_write_bytes()` drains to zero on failure (not just
   success). Mutation-step: commenting out the `fetch_sub` in the
   failure-arm closure red-fails with the bespoke "counter leaked on
   failure path — admission cap will permanently reject after first
   slow-write failure" message.

3. **testing-czar BLOCK-3 (`update_oneshot` Fix B sibling)**: parallel
   shape to `fix_b_slow_writes_in_flight_byte_cap_emits_typed_signal`
   but exercises `Store::update_oneshot(...)` directly. Asserts the
   cap-check at `:4359` fires + counter mutates correctly at
   `:4338-4339`/`:4459-4471`. Catches a regression in the oneshot
   path that the streaming `update` test would miss.

4. **testing-czar BLOCK-4 (production composition wrap)**: changed
   `fix_a_small_blob_preserves_backpressure_signal_through_fast_slow_store`
   to wrap `FastSlowStore` in `VerifyStore` and `ExistenceCacheStore`
   (matching the production `cas_STORE` chain). The bug being fixed is
   cross-component signal preservation; testing only at the FSS unit
   boundary would miss a regression where a wrapper layer demotes the
   typed signal to generic Internal across its own join boundary.

5. **testing-czar MAJOR-1 (mutation-step bespoke message reachable)**:
   reordered the asserts in the Fix A under-action test so the bespoke
   `assert!` ("typed BackpressureSignal must be preserved when
   MemoryStore early-rejects") fires BEFORE the helper's generic
   `assert_eq!(err.code, ResourceExhausted, ...)` panic. Without this,
   the documented mutation-step red-fail message would never appear in
   CI output.

All 7 tests in the file pass + the 2 classifier tests
(`looks_like_dead_channel_respects_backpressure_signal`,
`new_fixup_variants_roundtrip_and_classify`) pass with the new
`SlowWritesAtCapacity` variant.
rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
…yStore pin overrides (TraceMachina#334 Fix C)

Server-side cas_FAST_SLOW_STORE writes go through MemoryStore (48 GB
fast tier). Pre-fix MemoryStore::pin_delegation = Leaf made
pin_digests a documented no-op via the trait default's Leaf arm; the
post-write pin held to bridge the BlobsInStableStorage ack window
silently evaporated, so an LRU eviction during the ack window broke
the >=2-replica durability invariant.

Changes:
- Add unpin_digests trait method (mirrors pin_digests routing via the
  PinDelegation enum so existing wrapper chain composes for free).
- Override pin_digests / pin_digests_with_results / unpin_digests on
  MemoryStore (delegate to MokaEvictingMap pin_keys / unpin_key,
  mirroring FilesystemStore).
- Override unpin_digests on FilesystemStore as the trait-method form
  of the existing public unpin_digest (singular) so the fan-out from
  FastSlowStore reaches the slow tier too.
- Replace MemoryStore::pin_delegation doc-comment ("no-op") with a
  Fix-C-aware comment explaining why Leaf + overrides is the correct
  semantics ("no inner-store delegation needed", not "no pinning").

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#334 Fix C)

After broadcast_blobs_in_stable_storage_chunked tells workers a CAS
digest is durably mirrored, release the server-side fast-tier pin
acquired by FastSlowStore::update at write time. Without this unpin
every write accumulates a permanent pin entry in MemoryStore's
25%-of-cap pin budget; once the budget fills (~12 GB at the 48 GB
production cap) MokaEvictingMap::pin_keys warn-and-skips every
subsequent pin and silently re-opens the durability gap Fix C closes.

Capture per-store CAS drains alongside the existing flat broadcast
list so unpin can be scoped to the originating store (avoids racing
with a pin acquired by a fresh write of the same digest on a
different store). AC stores intentionally not unpinned: AC pin lives
in ac_pin_registry, swept separately above by
remove_digests_for_endpoint_batch.

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
…x C pin/unpin

Two tests covering both directions of the pin contract on
MemoryStore (asymmetric coverage per CLAUDE.md):

- memory_store_pin_survives_eviction_pressure (under-action):
  asserts pin_digests on the cas_FAST_SLOW_STORE chain
  (ExistenceCacheStore → VerifyStore → FastSlowStore { fast: Memory,
  slow: Filesystem }) actually keeps blob A alive across sibling-write
  eviction pressure that drives MemoryStore past cap. Pre-fix: trait
  default's Leaf arm no-op'd the pin and A was evicted.

- memory_store_unpin_releases_for_eviction (over-action):
  after unpin_digests on the same chain, the pin must be released so
  A becomes the LRU victim. Pre-fix: no unpin_digests trait method
  existed so the BIS broadcast loop had no path to release pins; an
  accumulating pin set would fill the 25%-of-cap pin budget and
  silently re-open the durability gap Fix C closes.

Both tests use 5 s tokio::time::timeout deadlock detectors and
bespoke .expect() messages naming the contract violation. Mutation
verified: removing pin_digests body red-fails the under-action with
"MemoryStore-pin no-op shipped" message; removing unpin_digests
body red-fails the over-action with "pin leaks past BIS-ack" message.

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
…raceMachina#334 Fix C sibling)

RefStore overrides pin_digests to forward to the resolved target
store; symmetric override for unpin_digests so the BIS broadcast
loop can release pins through any RefStore-mediated chain. Without
this the trait default's Leaf arm would be a no-op for RefStore
and the unpin would never reach the actual pinning leaf.

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
…dsr MAJOR-1)

Per dsr's MAJOR-1 review of Fix C: the original BIS-broadcast unpin
loop only released CAS fast-tier pins. The doc-comment claimed "AC
backend is FilesystemStore-only in production" which is factually
wrong against the production config — `AC_BACKEND_CACHED` is
`FastSlowStore { fast: MemoryStore(4 GB), slow: ref(REDIS_AC_STORE) }`.
Every AC write pins on the AC MemoryStore at fast_slow_store.rs:4103,
but Fix C never unpinned, leaking the AC pin budget symmetrically to
the CAS leak Fix C closed. After ~1 GB of AC writes (4 GB cap × 25%
pin cap) `pin_keys: pin cap exceeded` would flood and AC pins would
silently fail, re-opening the same durability gap on AC.

Symmetric fix: build per-AC-store drains alongside the existing
per-CAS-store drains, and call `unpin_digests` on each originating
AC store post-broadcast. Routes through the same `pin_delegation`
chain (AcProxyStore → CompletenessCheckingStore → FastSlowStore)
that `pin_digests` traversed at write time.

Doc-comment updated to reflect the correct AC composition.
rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
…x C AC unpin

Symmetric to the existing memory_store_pin_durability_test for CAS:
exercises the AC chain (AcProxyStore → CompletenessCheckingStore →
FastSlowStore { fast: MemoryStore, slow: MemoryStore }) for both
under-action (pin survives eviction pressure) and over-action (unpin
releases for eviction) contracts.

Mutation step verified: setting AcProxyStore::pin_delegation to Leaf
red-fails ac_unpin_releases_for_eviction with the bespoke
"AC pin leaks past BIS-ack — entry A survived sibling-pressure
eviction even after unpin_digests through the AC chain..." message.
This proves the test guards the new AC unpin loop's chain-routing
contract end-to-end.

The under-action test (ac_pin_survives_eviction_pressure) covers
the symmetric AC-side of the existing CAS pin override; this test
will additionally guard against any future regression in
AcProxyStore / CompletenessCheckingStore pin_delegation forwarding.
rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
 Fix C eviction extension)

With MemoryStore::emit_backpressure_enabled=true (production default
for cas_FAST_SLOW_STORE.fast), check_backpressure_gate refuses inserts
BEFORE moka can do its natural admission-driven LRU eviction. Once the
cache fills to cap, NOTHING evicts (no insert pressure, no TTL, no
explicit removes), turning the cache into write-once-evict-never until
process restart.

This new helper restores liveness by giving the gate a way to free
unpinned room before emitting backpressure. Pinned entries (the load-
bearing ones during the BIS ack window) live OUTSIDE moka's cache in
the `pinned` DashMap, so they're protected automatically — this helper
can only touch evictable entries.

Order caveat: moka exposes only arbitrary-order iter() (no public LRU
walker — access_order_q_node is pub(crate)). Aspirational "LRU" in
the method name; arbitrary order is acceptable because pin protection
is independent of iteration order, and unpinned entries are by
definition evictable. Documented in the helper rustdoc.
rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
…ix C eviction extension

Three tests covering the asymmetric contract for the gate's new
active-eviction path before emitting the typed BackpressureSignal:

1. Under-action — eviction frees room for unpinned entries:
   cap full of 4 unpinned entries, gate evicts one to admit a new
   write. Mutation step verified: commenting out evict_unpinned_lru_bytes
   in check_backpressure_gate red-fails this test with the bespoke
   "Fix C eviction extension regressed — gate refused admission with
   evictable unpinned entries available" message.

2. Over-action — pinned entries are protected: cap saturated by a
   pinned entry, gate finds nothing evictable, emits typed signal,
   pinned entry survives. Mutation step verified: making
   evict_unpinned_lru_bytes also drain the `pinned` DashMap red-fails
   this test on either the pin-survival assert or the expect_err
   assert with the bespoke "must NEVER touch keys present in `pinned`"
   message.

3. Mixed — partial eviction frees enough room: cap = 1 pinned + 3
   unpinned, gate evicts ≥1 unpinned to admit a new write while the
   pinned entry survives. Same mutation as test 2 catches durability
   regressions; same mutation as test 1 catches under-eviction.

Each assertion runs under a tokio::time::timeout(5s) deadlock detector
(eviction loop is sub-millisecond healthy) so a lock-contention
regression manifests as a red test rather than a hung CI runner.

Cap = 4 KiB, entry = 1 KiB: the smallest cap where pin_cap (25% × cap)
fits a real pinned entry AND a triggerable over-cap incoming write.
rejuvenile added a commit to rejuvenile/nativelink that referenced this pull request Aug 19, 2026
…raceMachina#334 bundle fixup TraceMachina#1)

Branch 2 (Fix C + AC + eviction extension) authored two test files —
`memory_store_pin_durability_test.rs` and `ac_pin_durability_test.rs` —
without the new `slow_writes_in_flight_max_bytes` field on `FastSlowSpec`.
After the cherry-pick merge of Fix A+B (which adds the field with
`#[serde(deny_unknown_fields)]`), these tests fail to compile with
E0063: missing field.

Both literals add `slow_writes_in_flight_max_bytes: 0,` (= uncapped, the
historic behavior bit-identical for opt-out callers). The eviction-
extension test file `memory_store_backpressure_eviction_test.rs` was
checked workspace-wide for FastSlowSpec literals; it has none, so no
change required there.

Also surfaced and fixed: pre-existing `chunked_fast_slow` gate hole on
`writer_termination_245_drop_fallback_test`. The test calls
`set_chunked_size_threshold_for_test` (gated `#[cfg(all(feature =
"chunked_fast_slow", any(test, feature = "test-utils")))]`) and
`set_bazel_chunked_dispatcher` (gated on `chunked_fast_slow`). Without
the matching `[[test]] required-features = ["chunked_fast_slow",
"test-utils"]` declaration, the test silently passes when the feature
is off and fails to compile when the feature is on. Mirrors the
existing entry for `fast_slow_str_key_skips_chunked_dispatch_test`.

Compile verified with both `cargo check -p nativelink-store --tests`
and `--features chunked_fast_slow`. Workspace-level
`cargo check --workspace --tests --features chunked_fast_slow` clean.

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
…afety mechanism (TraceMachina#334 bundle fixup TraceMachina#2)

6 reviewers (testing-czar + dsr + code-reviewer + perf + simplifier +
red-team) converged on the BIS-loop unpin doc-comment at
`src/bin/nativelink.rs:1037-1042`. The original comment claimed "per-
store accounting (drains_per_store) keeps the unpin scoped to the BIS-
acked write so we don't race a fresh pin on the same digest." This is
incorrect: `MokaEvictingMap::unpin_key` (`moka_evicting_map.rs:903-911`)
is wholesale `pinned.remove(key)` with NO per-write epoch tracking. A
concurrent re-pin between drain and unpin DOES lose its pin.

The race IS benign — but for a different reason than the original
comment claimed:
  - BIS-ack implies the SLOW TIER (FilesystemStore for CAS, Redis for
    AC) already has the bytes. The fast-tier pin is no longer load-
    bearing past this point.
  - A concurrent re-pin between drain and unpin loses its pin, but the
    bytes are recoverable via slow-tier.
  - For CAS specifically, content-addressing means a re-uploaded
    same-key blob has identical bytes; the slow tier already has them.

Rewrites both doc-comments (server BIS loop + MemoryStore::unpin_digests)
to name the actual mechanism. Adds an explicit warning that future
callers needing different per-write semantics will need epoch tracking
in `MokaEvictingMap` — do NOT assume safety in arbitrary contexts.

No code changes; doc-only.

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
…es (TraceMachina#334 bundle fixup TraceMachina#3)

Code-reviewer MA-3: production deploys with `chunked_fast_slow` enabled,
but `.github/workflows/native-cargo.yaml` never runs the
chunked_fast_slow-gated test bins. Three tests in this bundle (and ten
others pre-existing) silently skip in CI today.

Adds a CI step `Test chunked_fast_slow feature` analogous to the
existing failpoints carve-out. Combines `chunked_fast_slow` AND
`test-utils` so tests requiring both (e.g.
`fast_slow_str_key_skips_chunked_dispatch_test`,
`writer_termination_245_drop_fallback_test`,
`chunked_filesystem_serialization_test`) all run in one pass.

Also adds `[[test]] required-features = ["chunked_fast_slow"]` for
`memory_store_backpressure_eviction_test` so cargo silently skips it
from default `cargo test --all` (matches the existing pattern for
`memory_store_backpressure_emission_test`). The other two new test
files (`memory_store_pin_durability_test`, `ac_pin_durability_test`)
do not depend on chunked_fast_slow APIs and run fine without the
feature, so no required-features declaration for them.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants