fix(spur-net): cache OCI layers under the resolved image directory

[maybeharshit]

Motivation

Non-root image imports fall back from /var/spool/spur/images to ~/.spur/images, but the OCI layer cache remained hardcoded under the system directory. Cache writes consequently failed silently, causing every fresh import to download all layers again.

Fixes #345.

Technical Details

• Derive the default layer cache from the resolved image output directory using <output_dir>/.layers.
• Preserve SPUR_IMAGE_CACHE as an explicit override.
• Continue image imports when cache creation or writes fail.
• Emit structured warnings when tracing is configured and visible stderr warnings otherwise.
• Add unit coverage for default cache resolution and environment overrides.

Test Plan

• Run the spur-net unit tests.
• Run Clippy across the workspace.
• Run the complete workspace test suite.
• Validate rootless imports in an Ubuntu 24.04 LXD VM using an unprivileged user and a deterministic local OCI registry.
• Verify default fallback, cache reuse, explicit override, and unwritable-cache behavior.

Test Result

• All 36 spur-net tests passed.
• Workspace Clippy passed without warnings.
• Full workspace test suite passed.
• LXD validation confirmed:
  • layers were cached under ~/.spur/images/.layers;
  • a second import reused the layer without another registry request;
  • SPUR_IMAGE_CACHE remained effective;
  • cache creation and write failures produced warnings without failing the import.

Submission Checklist

• Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

[yansun1996]

This PR is on top of PR #500 , wait for the merge of the previous PR then proceed with this one.

[yansun1996]

Hi @maybeharshit , we've merged your previous PR #500 , please rebase this PR on top of latest upstream main branch.

[yansun1996]

Thanks for this — deriving the layer cache from the resolved output_dir is the right fix for #345, and degrading gracefully instead of silently swallowing cache failures is a nice touch. A few suggestions before merge, mostly an edge case and test coverage — details inline.

[yansun1996]

var_os(...).map(PathBuf::from) treats SPUR_IMAGE_CACHE="" as an explicit empty override, so layer_cache_dir returns "", create_dir_all("") fails, and the cache is disabled — which reintroduces the "re-download every layer" symptom this PR fixes. The sibling env resolver in spur-cli/src/image.rs guards this (if !dir.is_empty()); would it be worth mirroring that here?

std::env::var_os("SPUR_IMAGE_CACHE")
    .filter(|v| !v.is_empty())
    .map(PathBuf::from),

[maybeharshit]

Good catch — fixed in 1d48fa3. Rather than filtering at the call site, the guard now lives inside layer_cache_dir itself, so every caller (including the tests) goes through the same rule:

match override_dir.filter(|dir| !dir.is_empty()) {
    Some(dir) => PathBuf::from(dir),
    None => output_dir.join(".layers"),
}

An empty SPUR_IMAGE_CACHE is now treated as unset and falls back to <output_dir>/.layers, matching the SPUR_IMAGE_DIR handling in spur-cli/src/image.rs.

[yansun1996]

This tracing::enabled!(WARN) → warn! else eprintln! block is duplicated verbatim at the layer-write site below — worth pulling into a small shared helper.

One subtlety with the gate: tracing::enabled!(Level::WARN) is also false when a subscriber is installed but WARN is filtered out. In that case warn! is suppressed and the eprintln! branch never runs, so the warning is lost in exactly the situation the fallback was meant to cover. Always calling warn! (and letting the CLI install a subscriber) may be simpler and avoids that gap.

[maybeharshit]

Agreed on both points — the tracing::enabled! gate is gone entirely as of 1d48fa3.

Cache handling now sits behind a small LayerCache type that calls warn! unconditionally at both the create and write sites, so there is no duplicated block and no way for a subscriber with WARN filtered out to swallow the message without the eprintln! fallback firing.

To make sure those warnings actually reach users, spur-cli now installs a stderr subscriber in main, defaulting to warn and honouring RUST_LOG for more verbosity. Previously the CLI had no subscriber at all, which is what the eprintln! fallback was compensating for.

[yansun1996]

These two tests exercise only the pure layer_cache_dir helper, so they'd still pass if the core of the fix (the env read in pull_image, the create_dir_all failure handling, and the Option gating on the layer read/write) were reverted. Could the actual degradation path get coverage? Extracting a small synchronous cache read/write seam would let you assert the disable-on-unwritable and round-trip behaviors with tempfile, no network needed. At minimum, an empty-SPUR_IMAGE_CACHE case here would lock in the guard suggested above.

[maybeharshit]

You were right that the original tests would have survived a revert of the actual fix. 1d48fa3 extracts the synchronous read/write seam you suggested — a LayerCache type with open/read_layer/write_layer — so the degradation paths are now covered directly with tempfile, no network needed:

• layer_cache_round_trips_layers — write then read back through the cache.
• layer_cache_disabled_when_directory_cannot_be_created — open on an unwritable parent degrades to a disabled cache, and reads/writes become no-ops instead of errors.
• layer_cache_write_failure_leaves_pull_usable — a failing write does not propagate, so the pull continues.
• layer_cache_ignores_empty_environment_override — locks in the empty-value guard from the thread above.

All 40 spur-net tests pass, along with workspace clippy and the full test suite.

[maybeharshit]

@yansun1996 thanks for the review — all three points are addressed in 1d48fa3, with details in the inline threads. Summary:

• Empty SPUR_IMAGE_CACHE: the guard now lives inside layer_cache_dir (override_dir.filter(|dir| !dir.is_empty())), so an empty value is treated as unset and falls back to <output_dir>/.layers.
• Duplicated warning block: removed along with the tracing::enabled! gate. Cache handling moved behind a LayerCache type that calls warn! unconditionally, and spur-cli now installs a stderr subscriber (default warn, RUST_LOG-aware) so the warnings actually reach users — which was the real gap the eprintln! fallback was papering over.
• Test coverage: the create/read/write seam is now unit testable, so the degradation paths are exercised directly with tempfile rather than only the pure helper — round-trip, disable-on-unwritable-directory, write-failure, and the empty-override case.

Validated with cargo clippy --workspace --exclude spur-ffi --all-targets --locked (clean) and the full cargo test --locked suite. Could you take another look when you get a chance?

[codecov-commenter]

Codecov Report

❌ Patch coverage is 72.38095% with 29 lines in your changes missing coverage. Please review.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #507      +/-   ##
==========================================
+ Coverage   74.23%   74.35%   +0.12%     
==========================================
  Files         165      165              
  Lines       57595    59429    +1834     
==========================================
+ Hits        42755    44186    +1431     
- Misses      14840    15243     +403     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[yansun1996]

LGTM