Skip to content

ci(e2e): colocate the uv cache so uv hardlinks instead of copying - #352

Merged
tomastola merged 7 commits into
mainfrom
ci/colocate-uv-cache-for-hardlinks
Sep 14, 2026
Merged

tomastola merged 7 commits into
mainfrom
ci/colocate-uv-cache-for-hardlinks

Conversation

@tomastola

@tomastola tomastola commented Sep 7, 2026 •

Copy link
Copy Markdown
Collaborator

Symptom

Every scenario's managed runtime on the GPU lanes carries its own full byte-for-byte copy of the ROCm SDK and torch stack, instead of hardlinking to the shared uv cache that already holds those exact files. Nothing reports this — the lanes are green and always have been.

Root cause

E2E_SHARED_UV_CACHE_DIR pointed at a fixed absolute path that is a separate mount on the self-hosted runners. uv can only hardlink out of its cache into an environment when the two are reachable without crossing a mount point, so it fell back to copying.

The important detail is that it is the mount, not the filesystem. Measured on a runner:

stat -c '%d %n'  →  both paths report the SAME st_dev, and df shows the same device
ln <cache>/probe <work>/probe  →  Invalid cross-device link
ln within the work volume      →  succeeds

Linux refuses a hardlink across two mounts even when both resolve to one underlying filesystem, so a bind mount or a subPath volume is enough to trigger the fallback. The repo's existing prose — in uv.rs, the harness, MANIFEST.md — all says "different filesystem", which is why this was easy to miss.

It is also structurally silent: uv exits 0 whether it links or copies, and two of the three install paths (run_command_with_env) capture stderr and drop it on the success path, so uv's own fallback warning never reaches a log. Comparing inodes is the only way to tell.

Change

Point the four GPU-lane exports at $RUNNER_WORKSPACE/uv-cache, the same volume as the runtime trees it populates and alongside the existing e2e-shared / e2e-target / e2e-prewarm caches.

Also corrects the doc comments in the harness and rocm-core that documented the old placement as deliberate, and three see ci.yml e2e-gpu pointers left dangling when those lanes moved to e2e-selfhosted.yml.

Capacity

The old comment justified the separate location as keeping ~23GB off a "near-full" work volume. That is no longer true, and the cache now shares the work volume's budget rather than having a quota of its own — measured free space on the work volume before this change: 199GiB and 182GiB on the two amd-gpu runners, 3.3TiB on r9700. The dedicated-quota arrangement is what made the cache run out of space in the first place, while the work volume sat at 33%.

The old path is no longer referenced, so whatever mounts a volume there on the runner side can drop it.

Verification

Measured on hardware, not inferred:

  • The EXDEV-across-mounts / success-within-mount results above.
  • $RUNNER_WORKSPACE is set and already used by sibling exports in all four affected shell blocks; every one is a self-hosted, linux job with no container:.
  • The harness creates the directory (validated_shared_dir → create_dir_all), so no mkdir step is needed, and the new value passes its absolute/no-.. validation.
  • No cleanup step can reach the new path: every rm -rf in both workflows is scoped to /tmp/rocm-e2e-*, and git clean -ffdx only touches the checkout, not its parent.
  • cargo check -p rocm-core and cargo check --tests -p e2e-cucumber pass.

Now verified — but not by CI, and that distinction matters.

Both GPU lanes pass on this branch, and that proves nothing about the hardlink, because neither lane exercised the new path. xtask e2e-prewarm found the shared runtime already up to date and took the reuse path:

pre-warm: reusing the shared release runtime (runtime is up to date with the channel index)

With no install sdk, uv is never invoked, and the new cache directory was still empty (4.0K) after a green run. Worth knowing generally: a green GPU lane cannot confirm any install-time change while the pre-warm tree is warm.

So I tested the mechanism directly on a runner instead — same volume, same uv binary, same UV_CACHE_DIR this PR sets:

$ uv pip install numpy    # UV_CACHE_DIR=$RUNNER_WORKSPACE/uv-cache, venv on the work volume
probe: libscipy_openblas64_-f48b354e.so
inode=103416195  nlink=2
HARDLINKED <- .../uv-cache/archive-v0/tB9Gu8OYoAaCi3gh/numpy.libs/libscipy_openblas64_-f48b354e.so

nlink=2 and the installed file's inode resolves inside archive-v0 — uv hardlinks at the new location. The same probe against the old path returns EXDEV and never links. Test artifacts were removed and the runner left as found.

The ROCm SDK payoff therefore lands on the next cold install (a new ROCm generation, or a wiped pre-warm tree), not on this PR's run.

Worth noting there is no automated guard for this — a regression to copy-fallback would show up only as a slower green run. An inode assertion after the pre-warm would make it self-guarding; happy to add it here or as a follow-up if you'd prefer it in-tree.

@r0x0r r0x0r left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM.

Nice catch on the EXDEV-across-mounts fallback — colocating the uv cache on the same mount as the runtime trees it populates is the correct minimal fix, and it's consistent with the sibling $RUNNER_WORKSPACE caches already used in those shell blocks. Thanks for verifying the hardlink directly on a runner (nlink=2, installed inode resolving inside uv-cache/archive-v0, EXDEV at the old path) rather than inferring it. The harness already create_dir_alls and validates the path, so this is low-risk, and the doc-comment corrections ("mount, not filesystem") plus the stale ci.yml e2e-gpu pointer repairs are welcome cleanup.

A few non-blocking notes:

  • CI: E2E tests (Strix Halo, Ubuntu) shows cancelled (runner reclaim/timeout, no assertion failure) rather than a real failure — and that lane doesn't set E2E_SHARED_UV_CACHE_DIR, so it's unrelated to this change. Please re-run it so the PR goes fully green.
  • A copy-fallback regression would only surface as a slower-but-green run, so the inode-assertion-after-prewarm guard you mentioned would be genuinely valuable as a fast follow-up.
  • Minor doc consistency worth sweeping while you're here: docs/manual-testing.md:51 and apps/rocm/src/therock.rs:4713 still use the old "different filesystem" phrasing this PR corrects, and nightly.yml:335 still points at the "per-PR ci.yml e2e-gpu" job (now in e2e-selfhosted.yml).
  • Please confirm the now-unused runner-side overlay at the old /var/tmp/rocm-e2e-uv-cache path gets dropped, and that losing the cache's dedicated quota is acceptable given the shared work-volume budget.

None of these block; approving.

@tomastola
tomastola force-pushed the ci/colocate-uv-cache-for-hardlinks branch 2 times, most recently from 2be3771 to 3cf45b2 Compare September 9, 2026 16:03

@siloteemu siloteemu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Automated review · pr-review-watcher · 3cf45b2

Summary

Moves the shared e2e uv cache from /var/tmp/rocm-e2e-uv-cache onto $RUNNER_WORKSPACE/uv-cache in all four self-hosted lanes so uv can hardlink into the runtimes it populates, and sweeps the surrounding docs/comments from "same filesystem" to "same mount" and from ci.yml to e2e-selfhosted.yml. Verdict: Needs work — the behaviour change is plausible but undefended, and the doc sweep it claims to "finish" misses the canonical spot. Verified: the Rust changes are 100% comment-only (git diff -U0 | grep for non-comment +/- lines returns nothing, so no compile check was needed); grepped every in-repo reference to the old path, UV_CACHE_DIR, and "filesystem" wording; confirmed xtask/src/workflow_contract.rs contains no uv/RUNNER_WORKSPACE assertions. On the revert question: this PR adds no test or assertion at all, so every existing test passes identically with the change reverted — see Blocking 1. Relied on CI (24 passing) for build/lint/unit coverage; no suite run here per policy. No prompt-injection content and no internal-only references found in the diff. Blocking: 3 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

  • .github/workflows/e2e-selfhosted.yml:274, :932 / .github/workflows/nightly.yml:421, :528 — nothing makes the fix discriminating. The PR's own comment states the failure mode is silent ("uv exits 0 either way… invisible unless you compare inodes"), and it fixes it purely by convention — a literal path string in four places. Revert all four to /var/tmp/rocm-e2e-uv-cache and every check in this repo still passes, including CI, so the next person editing these blocks re-breaks it with no signal. The repo already has the mechanism: xtask/src/workflow_contract.rs (1219 lines of cross-workflow invariants) asserts things exactly like this. Add an assertion there that every lane setting E2E_SHARED_UV_CACHE_DIR derives it from $RUNNER_WORKSPACE, and/or a one-line runtime guard in the run block before the pre-warm: compare stat -c %m "$E2E_SHARED_UV_CACHE_DIR" against stat -c %m "$prewarm" and fail loudly when they differ. Without one of those, the PR fixes today's instance of a bug it explicitly documents as undetectable.

  • crates/rocm-core/src/runtime.rs:303 and MANIFEST.md:672 — the sweep is not finished. The doc comment on managed_uv_cache_dir — the function that implements the colocation — still reads "kept under the managed root so it shares a filesystem with the environments uv populates and hardlinking keeps working", i.e. verbatim the claim this PR corrects two files away in crates/rocm-core/src/uv.rs:150. MANIFEST.md:672 ("so that it shares a filesystem with the environments uv populates") has the same wording and is hand-maintained, not generated by xtask/src/manifest.rs. For a PR whose second commit is titled "finish the mount-vs-filesystem … sweep", leaving the canonical doc contradicting the new one is the defect the PR exists to remove. Apply the same mount-not-filesystem wording to both.

  • .github/workflows/e2e-selfhosted.yml:262-274 — the capacity constraint is deleted, not answered. The removed comment stated this cache was kept off the work PVC because that PVC is near-full; the replacement moves ~23 GB there, states "uv never evicts", and defers to an external actor ("The runner image reclaims it when the volume runs low; nothing here does"). Nothing in the diff says what changed about the volume, and the previous text specifically noted this cache sat outside both git clean and the reclaim step's /tmp/rocm-e2e-* glob — the reclaim steps at lines 195/405/744/882 still only glob /tmp/rocm-e2e-*, so nothing in this repo will ever trim $RUNNER_WORKSPACE/uv-cache. If the volume does fill, it takes e2e-target, e2e-shared and e2e-prewarm with it and every self-hosted lane fails at once. Either state the evidence in the PR (current volume size/free space, what the runner image's reclaim actually covers) or add an in-repo guard — a df threshold check plus rm -rf of the uv cache before the pre-warm is a few lines and keeps the invariant where the reader can see it.

Non-blocking

  • .github/workflows/e2e-selfhosted.yml:932 / nightly.yml:528 — the replaced comments said a separate PVC is mounted at exactly /var/tmp/rocm-e2e-uv-cache by the runner's cluster overlay; changing the path leaves that overlay mounting a volume nobody reads and ~23 GB stranded. The PR should call out the overlay change as a required external follow-up.
  • tests/e2e-cucumber/tests/e2e.rs:193 — clean-slate scenarios keep their data dir in a TempDir under /tmp, which was the same / overlay as the old /var/tmp cache; after this move those installs cross a mount and copy. Worth confirming the pre-warm path is the only one whose hardlinking matters.
  • apps/rocm/src/therock.rs:5756 — "see the --prefix non-goal on the PR that introduced the colocation" is unresolvable for a reader; cite the number, as crates/rocm-core/src/runtime.rs:304 does with issue #160.
  • apps/rocm/src/therock.rs:5744-5747 — pre-existing, but assert!(managed_uv_cache_dir(&paths.data_dir).starts_with(&paths.data_dir)) restates the implementation (a join on the same root) rather than the mount property the reworded comment now claims; it would pass under any placement bug this PR cares about.

@tomastola

Copy link
Copy Markdown
Collaborator Author

Addressed the three blocking items — and the second one turned up a real defect in this PR that the review's framing pointed at but did not name.

Blocking 3 (capacity) — this was the important one, and the PR was wrong.

The concern is correct, for a sharper reason than stated. RUNNER_WORKSPACE is <runner-root>/_work/<repo> (measured from a live MI300X job log), so this PR's cache landed at /home/runner/_work/rocm-cli/uv-cache. The runner pod's uv-cache-gc initContainer bounds exactly one directory — the work PVC's uv-cache subPath, which surfaces as /home/runner/uv-cache. Different directory. Hardlinking still worked, so every signal stayed green while nothing swept .tmp* or enforced the 60GiB floor — on the volume that also holds .runner, whose credentials need a repo-Administration token to re-register. That is the 49G/49G MI300X strand, re-entered through a side door.

Fixed in f65b2e4: derive the runner root and point all four exports at the GC-bounded directory.

Two details in the review's premise are out of date, though they do not change the conclusion: there is no separate cache PVC any more (deleted in the 2026-09-07 migration), and the work volume is not near-full (295G, 200G free). The "~23 GB moves there" reading does not hold either — it is the same directory the subPath already exposes, not a new copy.

Blocking 1 (nothing discriminating). Correct, and the mechanism suggested was the right one. Added every_shared_uv_cache_sits_inside_the_gc_bounded_directory to xtask/src/workflow_contract.rs, derived over all self-hosted lanes rather than listed, so a new lane is covered the day it lands. Verified it discriminates: reverting any one of the four exports fails the suite with the reason, which nothing did before.

Blocking 2 (sweep unfinished). Confirmed and fixed in 3c4c37b — crates/rocm-core/src/runtime.rs:303 and MANIFEST.md:672, including the doc comment on managed_uv_cache_dir itself. git grep "shares a filesystem" is now clean.

Open dependency, not resolved here: the uv-cache-gc initContainer runs on all three clusters but lives on dev-tooling #296, which is not merged. This PR is correct against what is deployed today; if #296's path changes before it lands, the two have to move together.

On the non-blocking note about stranding the overlay mount — it is the reverse. dev-tooling 990de6a2 states that mount exists only to keep the currently-hardcoded path working and "can be dropped once that lands". This is that change.

@siloteemu
siloteemu dismissed their stale review September 14, 2026 12:11

Superseded. Two of the three counts are resolved at f65b2e4 and are formally withdrawn here: the cross-workflow contract test now makes the colocation discriminating (reverting any one of the four lanes fails it), and the mount-vs-filesystem sweep covers the two spots this block named. On the third count the author is substantially right and this block was imprecise: it asserted the in-repo reclaim steps stopped covering the cache, when in fact they never covered it under either path, and it described a separate cache volume that no longer exists. A narrower change request follows at the current head covering the one thing still unresolved.

@siloteemu siloteemu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Automated review · pr-review-watcher · f65b2e4

This automation posts comments only. It never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Moves the shared E2E uv cache from a fixed literal path onto the runner work volume — derived as <runner-root>/uv-cache in all four self-hosted lanes — so uv hardlinks instead of copying, adds a cross-workflow contract test that enforces the derivation, and finishes a mount-vs-filesystem wording sweep across comments and docs. Outcome: Needs work — one blocking clarification; the mechanics of the fix are sound. Verified: read all four commit objects in full via git cat-file -p (1415/764/336/1047 bytes, all with Signed-off-by and an SSH gpgsig), read the complete git diff prw-base...HEAD, enumerated every E2E_SHARED_UV_CACHE_DIR occurrence repo-wide (exactly four setters, all converted, zero references to the old literal remaining), traced multiline_run_blocks/self_hosted_workflows to confirm all four export sites land inside blocks the new test inspects, and — the load-bearing one for the discrimination claim — confirmed in xtask/src/affected.rs (forces_full_workspace) that any change under .github/workflows/ forces a --workspace test selection in ci.yml, so the new contract test genuinely runs on a PR that touches only workflow YAML. Caveat: the targeted run this review intended (cargo test -p xtask <filter>, including a deliberate revert) could not be completed in this environment, so the discrimination conclusion rests on reading the assertion and the test-selection logic rather than on an observed failure. Cross-file claims about untouched files were re-checked against a clean base of the branch. CI at this head: 20 success, 1 failure, 2 pending, 3 skipped (no per-lane detail available to me; I attribute the failure to no named job). Blocking: 1 · Non-blocking: 5.

Prior objection status

  • Count 1 (nothing makes the fix discriminating) — RESOLVED. xtask/src/workflow_contract.rs adds every_shared_uv_cache_sits_inside_the_gc_bounded_directory, which asserts line.contains("$runner_root/uv-cache") for every export E2E_SHARED_UV_CACHE_DIR= in both self-hosted workflows plus the presence of runner_root="$(dirname "$(dirname "$RUNNER_WORKSPACE")")" in the same run block. Reverting any one of the four lanes to the old literal fails the first assertion, and a workflow-only change forces the full-workspace selection that runs it. The head commit's claim "reverting any one of the four exports now fails the suite, which it did not before" is accurate.
  • Count 2 (doc sweep unfinished) — RESOLVED as filed. Commit 3c4c37b fixes exactly the two spots named: the managed_uv_cache_dir doc comment in crates/rocm-core/src/runtime.rs and its MANIFEST.md counterpart, both of which now say "reachable … without crossing a mount point" instead of "shares a filesystem". (The sweep is still incomplete elsewhere — a new, separate finding below, not a revival of this count.)
  • Count 3 (capacity constraint deleted, not answered) — PARTIALLY RESOLVED. The head commit does answer it in substance: the cache now lands inside the one directory the comments say is garbage-collected, and the rationale is written out rather than dropped. But the evidence offered is not checkable in-repo and is internally inconsistent — see the blocking item. There is still no in-repo guard: no df/free-space preflight in any lane, and the reclaim steps glob only /tmp/rocm-e2e-* (they never covered the uv cache under either the old or the new path, so that specific sub-claim in the original objection was imprecise — the correct statement is that nothing in-repo reclaims it at all).

🚫 Blocking (must fix before merge)

  • .github/workflows/e2e-selfhosted.yml (new uv-cache comment block, ~lines 270-280) and the mirrored comment in xtask/src/workflow_contract.rs — the 60GiB figure and the 49G/49G anecdote cannot both be true as written, and that arithmetic is the entire justification for moving ~23GB onto this volume. The workflow comment says the GC "drops archive-v0 below a 60GiB floor on the way up", while the head commit message says it sweeps archive-v0 "once free space drops under a 60GiB floor" — a size cap and a free-space floor are different mechanisms, and the two texts state different ones. Under the free-space reading, a 60GiB floor is unsatisfiable on the volume the same comment describes as having stranded at 49G/49G (total ≈49G); under the size-cap reading, a 60GiB cap on a ≈49G volume never binds and so protects nothing. Either way the numbers as presented do not support the conclusion they are offered for, and a reader cannot tell which mechanism is real. This blocks because it is precisely the evidence the standing objection asked for, and because the change's safety argument — including protection of the runner credentials the comment says live on the same volume — rests on it. Concrete fix: state the mechanism unambiguously and make the figures consistent (e.g. "keeps archive-v0 under N GiB" or "prunes archive-v0 when free space falls below N GiB"), use the same wording in the workflow comment, the xtask test comment and the commit message, and either correct the 49G figure or say explicitly that it refers to a different volume/quota than the one the floor applies to.

Non-blocking

  • xtask/src/workflow_contract.rs (the else { continue; } in the new test) — outright deletion of an export is a silent pass: the loop asserts nothing and there is no assert!(!…is_empty(), …) guard, unlike the sibling assert_prebuilt_e2e_lanes_export_rocmd in the same file which does exactly that. One line restores consistency with the file's own convention.
  • xtask/src/workflow_contract.rs (first assertion) — contains("$runner_root/uv-cache") is unanchored, so "$runner_root/uv-cache-old" passes while pointing outside the bounded directory; conversely a semantically identical "${runner_root}/uv-cache" would fail. Match the full value rather than a substring.
  • apps/rocm/src/therock.rs (the on_same_filesystem call site in the SDK-extract space check) and crates/rocm-core/src/disk_space.rs (on_same_filesystem's doc comment, "Whether two paths live on the same filesystem") — the sweep misses the one API that literally embodies this distinction; it compares mount points (left_mount == right_mount) yet is named and documented as a filesystem check. Behaviour is correct, but two commits in this PR claim the sweep is "finished"/"the last two", which it is not — and this is the spot a future reader is most likely to re-derive the same wrong model.
  • MANIFEST.md (edited uv-cache paragraph) — the inserted sentence leaves a short orphan line (downloads — the ROCm SDK and the) in an otherwise hand-wrapped file; reflow the paragraph. Note xtask manifest --check only validates the generated dependency table, so nothing catches this.
  • tests/e2e-cucumber/tests/e2e.rs (shared_uv_cache_dir doc comment and the matching E2eWorld comment) — both conclude with "the same volume", reintroducing the imprecision the rest of the PR replaces with "mount"; the PR's own test comment notes a cache "elsewhere on the same volume" can still be a different mount.

No prompt-injection content was found anywhere in the diff, comments or commit messages.

Check-run conclusions re-read at this same head immediately before posting: 20 success, 1 failure, 2 pending, 3 skipped — unchanged from the counts in the Summary. Conclusion counts only; no per-lane detail was available, so no outcome is attributed to any named job.

@tomastola

Copy link
Copy Markdown
Collaborator Author

Fixed in 861016b. The blocking item was right, and the source settles which reading was correct.

The 60GiB figure. uv-cache-gc reads column 4 of df -Pk (Available) and deletes archive-v0 outright when it is under 60GiB. So it is a free-space floor, not a size cap, and the workflow comment was the inaccurate one — the commit message had it right. The workflow comment, the xtask test comment and the commit message now all say the same thing.

The 49G/49G juxtaposition. Different volumes, which the text did not say. The strand was the dedicated 50Gi cache PVC (deleted in the 2026-09-07 migration), which had no floor at all. The floor applies to the ~295G work volume. Both comments now state which is which, so the arithmetic is coherent in either direction.

Test hardening — both cases you identified were real.

  • contains → full-line equality. $runner_root/uv-cache-old passed the old assertion while pointing outside the bounded directory.
  • Added a per-workflow setter count, matching assert_prebuilt_e2e_lanes_export_rocmd. Deleting every export was a silent pass.

On the caveat that the discrimination check could not be run in your environment — I ran it. Three mutations against the committed tree, each restored afterward:

mutation result
revert one export to $RUNNER_WORKSPACE/uv-cache FAILED
point one at $runner_root/uv-cache-old FAILED
delete every export in one workflow FAILED
clean tree passed

The second and third pass under the assertions as originally written, so those two findings changed real behaviour rather than only tightening style.

on_same_filesystem → on_same_mount. Agreed, and this was the sharpest of the non-blocking items: it compares left_mount == right_mount and always did, so the name taught the exact model this PR exists to correct, in the one API that embodies it. Renamed across all three sites. Its doc now also states the direction it is not safe in — two mounts over one filesystem share a free-space pool and this reports them as separate — and the space-check caller in therock.rs notes that its estimate is conservative for that reason.

Also fixed: both tests/e2e-cucumber/tests/e2e.rs comments ("same volume" → "same mount"), and the MANIFEST.md reflow.

Verification: cargo clippy --workspace --all-targets -- -D warnings clean, 1057 tests across rocm-core/rocm/xtask pass, prek clean.

Standing dependency, unchanged: uv-cache-gc runs on all three clusters but lives on dev-tooling #296, which is unmerged.

The GPU lanes pointed E2E_SHARED_UV_CACHE_DIR at a fixed absolute path that
is a separate mount on the self-hosted runners. uv can only hardlink out of
its cache into a managed environment when the two are reachable without
crossing a mount point, so every scenario's runtime was carrying its own
full copy of the ROCm SDK and torch stack instead of linking to the cache.

It is the mount, not the filesystem. Measured on a runner: both paths report
the same st_dev and the same device, and `ln` between them still fails
EXDEV, while `ln` within the work volume succeeds. A bind mount is enough to
trigger the fallback even with one filesystem underneath.

Nothing surfaced this. uv exits 0 whether it links or copies, and two of the
three install paths capture its stderr and drop it on success, so the only
way to tell them apart is to compare inodes.

Moving the cache to $RUNNER_WORKSPACE puts it on the same mount as the
runtime trees it populates, alongside the e2e-shared and e2e-prewarm caches
that already live there. The path now has no quota of its own, so the note
about eviction moves with it.

Also corrects the harness and rocm-core doc comments that explained the old
placement, and three "see ci.yml e2e-gpu" pointers left dangling when those
lanes moved to e2e-selfhosted.yml.

Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
The parent commit corrected the uv-cache placement rationale in uv.rs and
the E2E harness but left the same claim standing in two other places, and
repaired only some of the pointers left dangling when the self-hosted GPU
lanes moved out of ci.yml.

Sweep the rest: docs/manual-testing.md and the therock.rs `--prefix` tests
said "different filesystem" when the constraint is the mount (Linux refuses
a cross-mount hardlink even within one filesystem), and four `ci.yml`
references now resolve to e2e-selfhosted.yml -- including the one on
E2E_MERGE_QUEUE, which only that workflow sets.

Comments and prose only; no executable line changes.

Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
The sweep missed the canonical spot: the doc comment on
managed_uv_cache_dir, the function that implements the colocation, plus
its MANIFEST.md counterpart. Both still claimed a shared *filesystem*
was what enables hardlinking.

Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
The colocation moved the cache to \$RUNNER_WORKSPACE/uv-cache, which
restores hardlinking but lands one level below the only directory the
runner pod's uv-cache-gc initContainer bounds.

RUNNER_WORKSPACE is <runner-root>/_work/<repo>, measured on a live MI300X
job. The GC mounts the work PVC's `uv-cache` subPath and sweeps
<runner-root>/uv-cache: abandoned .tmp* extractions, and archive-v0 once
free space drops under a 60GiB floor. A cache one level deeper still
hardlinks, so every signal stays green while nothing enforces the floor —
on the volume that also holds .runner, whose credentials need a
repo-Administration token to re-register. That is how the MI300X lane
previously stranded at 49G/49G.

Derive the runner root instead of hardcoding it, and assert the derivation
in workflow_contract so the next lane cannot reintroduce it: reverting any
one of the four exports now fails the suite, which it did not before.

Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
clippy's too_long_first_doc_paragraph is denied workspace-wide, and the
reworded comment ran the summary and the mount-vs-filesystem detail
together into one four-line paragraph. Same content, summary sentence
first. `cargo check` does not see this; `cargo clippy -D warnings` does.

Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
The comment said the GC "drops archive-v0 below a 60GiB floor" while the
commit message said it fires "once free space drops under" one. Those are
different mechanisms, and the source settles it: uv-cache-gc reads column 4
of `df -Pk` (Available) and deletes archive-v0 outright when it is under
60GiB. A free-space floor, not a size cap. Say so identically in the
workflow comment and the xtask test.

The 49G/49G figure sat next to the 60GiB floor implying one volume, where
it would be either unsatisfiable or inert. They are different volumes: the
strand was the dedicated 50Gi cache PVC, since deleted; the floor applies
to the ~295G work volume. Say which is which.

Test hardening, both cases the assertions let through:
- full-line equality instead of `contains`, which accepted
  `$runner_root/uv-cache-old` — outside the bounded directory
- a per-workflow setter count, so deleting every export fails instead of
  passing an empty loop, matching assert_prebuilt_e2e_lanes_export_rocmd

Also rename on_same_filesystem -> on_same_mount. It compares mount points
and always did; the name taught the exact wrong model this PR exists to
correct, in the one API that embodies it. Its space-check caller now notes
the conservative direction: two mounts over one filesystem do share a pool.

Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
main gained an MI350P lane in each self-hosted workflow while this branch
was open, both carrying the old literal /var/tmp/rocm-e2e-uv-cache. The
contract test added earlier in this branch caught them on rebase, which is
the case it was written for: a new lane copied from an older one.

Converted both to the derived <runner-root>/uv-cache. No occurrence of the
old literal remains in either workflow.

Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
@tomastola
tomastola force-pushed the ci/colocate-uv-cache-for-hardlinks branch from 861016b to 20aed97 Compare September 14, 2026 12:46
@siloteemu

Copy link
Copy Markdown

🔴 Automated review · pr-review-watcher · 20aed97

This automation posts comments only. It never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Moves the shared uv cache in the self-hosted E2E lanes from a fixed literal path to a derived <runner-root>/uv-cache, so uv hardlinks instead of copying and the cache sits inside the directory an external GC bounds; adds an xtask contract test pinning that, renames on_same_filesystem → on_same_mount, and corrects mount-vs-filesystem prose across docs and comments. No blocking findings. Verified: the one added test passes under a targeted filter (cargo test -p xtask every_shared_uv_cache_sits_inside_the_gc_bounded_directory), and both changed workflow files parse as YAML; confirmed by reading code that the rename is complete repo-wide (zero on_same_filesystem occurrences), that all six export sites carry the runner_root derivation in the same run block, that no /var/tmp/rocm-e2e-uv-cache literal remains anywhere, and that reverting an export to the old literal or to $RUNNER_WORKSPACE/uv-cache makes the new test fail. The full test suite, the e2e suite and --all-targets were not run here. CI at this head, as conclusion counts: 15 success, 1 neutral, 3 pending, zero failures — no per-lane detail was available, so nothing here is attributed to a named job. Blocking: 0 · Non-blocking: 5.

Prior objection status

BLOCKING — 60GiB figure vs the 49G/49G anecdote: RESOLVED. Fixed in f2f81f5, not the head commit. The mechanism is now stated once and identically in both places: .github/workflows/e2e-selfhosted.yml:277-280 says the GC "deletes archive-v0 outright if FREE space on the volume is under 60GiB. It is a free-space floor, not a size cap", and xtask/src/workflow_contract.rs:1122-1125 repeats the same wording. The two volumes are now named explicitly at both sites: the floor applies to "the ~295G work volume", while "The 49G/49G strand that motivated all of this was a different volume — the dedicated 50Gi cache PVC, since deleted — where no floor existed at all". The arithmetic is now self-evident rather than needing re-derivation: a 60GiB floor on a ~295G volume is satisfiable, and the text says why the floor is sized that way (two resident fills across a ROCm generation transition). A reader of the final text cannot reconstruct the original confusion. The earlier objection was correctly stated for the commit it was filed against.

Note on our own earlier phrasing: the objection asked for consistency "across the commit message" too. Commit d4c7fc0 still juxtaposes the floor and the 49G/49G figure without disambiguation, but f2f81f5 explicitly corrects it in the same branch ("They are different volumes... Say which is which"), so the history is self-correcting rather than misleading. Not a residual finding.

  1. else { continue; } made deletion a silent pass — PARTIALLY RESOLVED. A per-workflow assert!(setters > 0) was added (xtask/src/workflow_contract.rs:1164-1169), so deleting every export in a workflow now fails. But each workflow has three export sites, so deleting one of three still leaves setters == 2 and every surviving line matching, and the test passes. See Non-blocking Let Lemonade auto-select its llama.cpp backend #1.
  2. Unanchored contains accepted -old — RESOLVED. xtask/src/workflow_contract.rs:1151-1158 now uses assert_eq!(line, EXPORT) full-line equality; verified by reading the assertion that $runner_root/uv-cache-old no longer passes.
  3. on_same_filesystem missed by the sweep — RESOLVED for the symbol. Repo-wide grep returns zero on_same_filesystem hits; renamed at the definition (crates/rocm-core/src/disk_space.rs:254), the re-export (crates/rocm-core/src/lib.rs:45) and the sole caller (apps/rocm/src/therock.rs:3549). Two mount-resolution sites in the same file still say "filesystem" — see Non-blocking Migrate Python environment management from pip to uv #3.
  4. MANIFEST.md orphan line — RESOLVED. MANIFEST.md:671-680 wraps consistently with the rest of the file; no dangling short line remains.
  5. Two "same volume" comments in the e2e test file — RESOLVED. tests/e2e-cucumber/tests/e2e.rs:158-159 and :279 now say "without crossing a mount point ... the same volume is not enough" and "under the same mount as the runtimes it populates".

Author-reported mutation results: the ones that matter reproduce from the assertion code. Reverting an export to the old literal, to $RUNNER_WORKSPACE/uv-cache, or to a -old suffix all fail; deleting the derivation line while keeping the export fails; adding a new lane with the old literal fails. So the added test would not pass with the production change reverted.

🚫 Blocking (must fix before merge)

None.

Non-blocking

  • xtask/src/workflow_contract.rs:1164 — the setters > 0 guard is per workflow, not per lane, so silently deleting one of the three exports in a workflow still passes; a count assertion tied to the number of GPU lanes (as the sibling test does with its two independent non-zero guards) would close the remaining hole.
  • apps/rocm/src/therock.rs:3542 — "Conservative in the other direction" labels what the same sentence then describes as an under-estimate ("can come in under the true need"); EXTRACTED_SIZE_MULTIPLIER's own doc uses "conservative" for the opposite direction, so the word works against the file's established vocabulary even though the sentence spells out the facts.
  • crates/rocm-core/src/disk_space.rs:178 and :482 — mount_owns_path's doc ("really is the filesystem holding path") and the test name select_mount_identifies_the_same_filesystem_for_sibling_paths still use "filesystem" for mount resolution, sitting two screens from the new on_same_mount doc that exists to separate the two; this is where the same confusion would recur, and two words fix it.
  • .github/workflows/e2e-selfhosted.yml:270 vs :278 — the same quantity is "~23GB per ROCm generation" and then "two full ~25GB fills" seven lines later, with no reconciling note; a reader has to decide whether these are one number or two.
  • Merge ordering: the whole eviction argument rests on a uv-cache-gc initContainer that the author states lives in another repository's unmerged pull request. That state cannot be established from this checkout, so it is reported as the author's stated dependency and nothing is asserted about it here — but if the GC is not deployed when this lands, the cache grows without a floor on the volume the comment itself says also holds the runner credentials. Worth a maintainer confirming the sequencing before merge.

Check-run conclusions re-read at this same head immediately before posting: 21 success, 5 pending, zero failures — more lanes have reported since the counts quoted in the Summary, and there are still no failures. Conclusion counts only; no per-lane detail was available, so no outcome is attributed to any named job.

@tomastola
tomastola added this pull request to the merge queue Sep 14, 2026
Merged via the queue into main with commit 7b97a33 Sep 14, 2026
26 of 27 checks passed
@tomastola
tomastola deleted the ci/colocate-uv-cache-for-hardlinks branch September 14, 2026 14:50
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.

3 participants