Repository navigation
ci: close the two edges the shared spm- cache namespace opened - #13933
Conversation
Sharing one `spm-<hash>` key across the arm64 pools made two latent edges reachable that the pool-scoped keys had hidden. `ci-macos-compat.yml` is an OS/arch compatibility matrix: it has an x86_64 leg (`macos-15-intel`) and an older-Xcode `macos-14` leg, and it caches the same `.ci-source-packages` path. Same path means same GitHub cache version, so a bare `restore-keys: spm-` would prefix-match its Intel-produced entry. `scripts/ci/r2-cache.sh:36` namespaces this exact directory by `$RUNNER_ARCH`, so the repo already treats arch as identity-bearing here. Move the matrix to a `compat-spm-` prefix so the two namespaces are disjoint by construction rather than by luck. `perf-activation.yml` was the one consumer of the shared namespace with no poisoned-cache check. `test-e2e.yml` and `test-depot.yml` both verify Sparkle and Sentry actually materialized and wipe the directory between attempts, because resolve can report success without binary artifacts; perf-activation retried against the same directory and failed opaquely later in "Build tagged app". Port the same check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 26 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
All contributors have signed the CLA ✍️ ✅ |
|
Independent review. I verified both claims against Edge 1 is real. Same path means same cache version, and Edge 2 is real and is the one I'd have insisted on. The ordering argument in the description is the load-bearing one: edge 2 is what makes edge 1 non-damaging rather than merely improbable. Worth landing even if the prefix rename were unnecessary. One thing this does not close, and I think correctly. The bare Cost is one cold resolve per compat leg on first run after this lands, on a Guards are green, including the full — Zarathustra g1 🌱 |
41f6862 ci: integrate canonical app-host compilation paths (manaflow-ai#13854) 165b05c ci: run the CmuxMobileShell package tests serially (manaflow-ai#13935) ca53e05 test: repair the renderer gate and tmux mirror sizing fixtures (manaflow-ai#13873) a1029b2 test(ios): stop the keyboard test seam renegotiating the grid (manaflow-ai#13932) 587f661 ci: namespace the compat cache and guard perf-activation's restore (manaflow-ai#13933) # Conflicts: # .github/workflows/ci-macos-compat.yml # .github/workflows/ci-macos.yml # .github/workflows/nightly.yml # .github/workflows/perf-activation.yml # .github/workflows/test-ios.yml
Follow-up to #13925. An independent review of that PR flagged two edges around the now-shared
spm-cache namespace; this closes both. A second review of this PR cleared it with no confirmed defects.1.
perf-activation.ymlgets the poisoned-cache guardThis is the substantive half. It was the only consumer of the shared namespace without one.
test-e2e.yml:412-419andtest-depot.yml:168-175both verify Sparkle and Sentry actually materialized andrm -rfthe directory between attempts, because — quoting test-e2e's own comment — "resolve can report success without binary artifacts materialized".perf-activation.ymlretried against the same possibly-poisoned directory andexit 0d on first success, then failed opaquely later in "Build tagged app".This guard is the one that matters, because it detects a bad restore regardless of cause.
2.
ci-macos-compat.ymlmoves to acompat-spm-prefixDefensive hygiene, not a fix for an observed failure — stated plainly rather than dressed up.
ci-macos-compat.yml:17-24has an x86_64 leg (macos-15-intel) and:11-16an older-Xcodemacos-14leg. TheCache Swift packagesstep at:137-147has noif:—run_unit_tests: falsegates only the test step at:184, so all four legs populate and save this cache. They sharepath: .ci-source-packageswith the workflows that now restore with a barerestore-keys: spm-, and since the GitHub cache version derives frompath(andenableCrossOsArchivegates OS, not CPU arch),spm-can prefix-matchspm-macos-15-intel-<hash>.Two honest caveats:
ci-macos.yml:455-456has used a barespm-prefix on this path since ci: extract reusable macOS workflow #13405. What ci: let the macOS 15 and 26 pools share one Swift package cache #13925 changed is that ci-macos.yml and nightly.yml read through./.github/actions/cache-restore, whose default backend isvars.CI_CACHE_BACKEND || 'r2', andscripts/ci/r2-cache.sh:36namespaces byv1/${RUNNER_OS}-${RUNNER_ARCH}— already arch-safe. The three workflows ci: let the macOS 15 and 26 pools share one Swift package cache #13925 touched use rawactions/cache, which always hits the GitHub store with no arch namespacing. So ci: let the macOS 15 and 26 pools share one Swift package cache #13925 added the first unconditionally-exposed consumers..ci-source-packagesis not demonstrably arch-sensitive.sanitize-xcode-source-packages-cache.pyremoves onlyworkspace-state.json; what remains is checkouts plus multi-slice Sparkle/Sentry xcframeworks. A cross-arch restore may well be benign. This is insurance, not a repair.Cost is nil: no
spm-macos-15-intel-*entry exists in the cache today, anddocs/ci/workflow-inventory.md:128records this workflow at 0 runs in the last 7 days (last success 2026-08-13), well past the 7-day eviction window. It also costs compat nothing in sharing — its old restore prefix wasspm-${{ matrix.os }}-, which was never a prefix of the pool'sspm-<hash>entries either, so its arm64 legs were already isolated.Known-better end state, deliberately not done here
Putting
${{ runner.arch }}in the shared key (astest-ios.yml:443andnightly.yml:613already do, and asr2-cache.sh:36does for this exact directory) would close the whole class rather than today's one Intel producer, and would let compat's arm64 legs share the nightly-seeded pool cache. That touches five hot-path workflows and forces a one-time cold resolve on the contended pool, so it belongs in its own change. Filed as a follow-up.Verification
actionlintrc=0. Its single SC2129 finding is atperf-activation.yml:70, pre-existing onorigin/main; these edits are at:168and:180.test_ci_pull_request_caches_are_read_only.py(parsesci-macos.yml/nightly.ymlonly, so the compat prefix is outside its assertions),test_ci_manual_macos_package_cache.py, andtest_ci_self_hosted_guard.sh.spm-macos, docs, guards): no stale references.🤖 Generated with Claude Code