Merge pull request #15 from curlewlabs-com/fix/audit-followup-and-local-mutex-bump

fix: re-audit follow-ups and local-mutex bump

Author: jeffwall-curlewlabs

.github/workflows/ci.yml

@@ -10,7 +10,7 @@ jobs:
   shellcheck:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v6
+      - uses: actions/checkout@v6.0.2
       - name: Shellcheck
         run: shellcheck lib/cache-restore.sh lib/cache-save.sh
 
@@ -23,7 +23,7 @@ jobs:
   actionlint:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v6
+      - uses: actions/checkout@v6.0.2
       - name: Run actionlint
         run: |
           bash <(curl -fsSL https://raw.githubusercontent.com/rhysd/actionlint/main/scripts/download-actionlint.bash)
@@ -32,7 +32,7 @@ jobs:
   integration-test:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v6
+      - uses: actions/checkout@v6.0.2
 
       - name: Restore against non-existent cache dir (cold start)
         id: restore-cold
@@ -346,6 +346,9 @@ jobs:
             || (echo "Existing-entry guard failed: expected 'first', got '$content'" && exit 1)
           echo "Sequential idempotent save: OK"
 
+      # The "v2:" prefix in these marker assertions is the canonical marker
+      # version from lib/cache-restore.sh (MARKER_VERSION). If that constant
+      # is ever bumped, the literals below must be bumped in lockstep.
       - name: Restore writes v2 marker
         run: |
           mkdir -p /tmp/test-marker-src
@@ -425,3 +428,66 @@ jobs:
           [ "$(cat /tmp/local-cache/entries/concurrent-restore-key/data.txt)" = "shared" ] \
             || (echo "Modifying A corrupted cache entry" && exit 1)
           echo "Concurrent restore isolation: OK"
+
+      # A second prefix restore against an already-populated target must hit
+      # the marker-skip branch in cache-restore.sh (is_current on the
+      # is_exact="false" path). Existing tests cover exact-hit-skip and
+      # prefix-match-first-time, but not this second-call-is-skipped case —
+      # which is the steady-state behavior for the README's restore → install
+      # → save pattern when the caller's exact key is slightly newer than the
+      # fallback. If this branch regresses, the restore silently starts doing
+      # full rsync work on every invocation instead of constant-time work.
+      - name: Second prefix restore against populated target is a marker-skip
+        run: |
+          mkdir -p /tmp/test-prefix-marker-src
+          echo "v1-content" > /tmp/test-prefix-marker-src/data.txt
+          sh lib/cache-save.sh /tmp/test-prefix-marker-src prefix-marker-v1 /tmp/local-cache
+
+          # First prefix restore populates the target with marker "v2:prefix-marker-v1".
+          # The caller's exact key (v2) doesn't exist; restore-keys "prefix-marker-"
+          # resolves to the only matching entry (v1).
+          sh lib/cache-restore.sh /tmp/test-prefix-marker-dst prefix-marker-v2 /tmp/local-cache "prefix-marker-"
+          marker=$(cat /tmp/test-prefix-marker-dst/.local-cache-restore)
+          [ "$marker" = "v2:prefix-marker-v1" ] \
+            || (echo "Expected marker 'v2:prefix-marker-v1' after first prefix restore, got '$marker'" && exit 1)
+
+          # Modify the restored content — if the second restore skips via
+          # marker-match, this modification survives (no rsync overwrite).
+          # Same pattern as the "Second restore with same key is skipped" test,
+          # but on the prefix-resolution branch.
+          echo "modified-between-restores" > /tmp/test-prefix-marker-dst/data.txt
+          sh lib/cache-restore.sh /tmp/test-prefix-marker-dst prefix-marker-v2 /tmp/local-cache "prefix-marker-"
+          content=$(cat /tmp/test-prefix-marker-dst/data.txt)
+          [ "$content" = "modified-between-restores" ] \
+            || (echo "Second prefix restore did not hit marker-skip: expected 'modified-between-restores', got '$content'" && exit 1)
+          echo "Prefix restore marker-skip: OK"
+
+      # When a caller runs the README's canonical pattern (restore → install
+      # → save on the same path) and the restore was a prefix hit, the target
+      # ends up with a .local-cache-restore marker pointing at the matched
+      # entry. The save step must exclude that marker from the new entry on
+      # disk, or else every cache entry that was ever populated via a prefix
+      # hit permanently carries a stale marker for the entry it was cloned
+      # from. Harmless at runtime but pollutes cache entries with stale
+      # metadata.
+      - name: Save after prefix restore excludes restore marker
+        run: |
+          mkdir -p /tmp/test-save-exclude-src
+          echo "source" > /tmp/test-save-exclude-src/data.txt
+          sh lib/cache-save.sh /tmp/test-save-exclude-src save-exclude-v1 /tmp/local-cache
+
+          # Prefix restore leaves the marker file in the target.
+          sh lib/cache-restore.sh /tmp/test-save-exclude-dst save-exclude-v2 /tmp/local-cache "save-exclude-"
+          [ -f /tmp/test-save-exclude-dst/.local-cache-restore ] \
+            || (echo "Expected marker in target after prefix restore" && exit 1)
+
+          # Save under the caller's exact key. The new entry must not contain
+          # the stale marker from the prefix hit.
+          sh lib/cache-save.sh /tmp/test-save-exclude-dst save-exclude-v2 /tmp/local-cache
+          [ -d /tmp/local-cache/entries/save-exclude-v2 ] \
+            || (echo "Expected new entry save-exclude-v2" && exit 1)
+          [ ! -f /tmp/local-cache/entries/save-exclude-v2/.local-cache-restore ] \
+            || (echo "rsync did not exclude marker — new entry contains stale marker from prefix source" && exit 1)
+          [ -f /tmp/local-cache/entries/save-exclude-v2/data.txt ] \
+            || (echo "Expected data.txt in new entry" && exit 1)
+          echo "Save excludes restore marker: OK"

AGENTS.md

@@ -7,7 +7,7 @@ A GitHub composite action that provides local-disk caching for self-hosted runne
 ## Rules
 
 - Shell scripts in `lib/` must use `#!/bin/sh` and pass `shellcheck` with no warnings.
-- No external dependencies beyond `rsync`, `sh`, and standard POSIX utilities. The one published-action dependency is [`curlewlabs-com/local-mutex`](https://github.com/curlewlabs-com/local-mutex), which `save/action.yml` uses to serialize per-key concurrent writers via the kernel's `lockf`/`flock` primitive. Pin local-mutex to a specific patch version (`@v1.0.0`, not `@v1`) so that updates land as reviewable PRs rather than silently following whatever the upstream major tag points at.
+- No external dependencies beyond `rsync`, `sh`, and standard POSIX utilities. The one published-action dependency is [`curlewlabs-com/local-mutex`](https://github.com/curlewlabs-com/local-mutex), which `save/action.yml` uses to serialize per-key concurrent writers via the kernel's `lockf`/`flock` primitive. Pin local-mutex to a specific patch version (e.g. `@v1.0.1`, not `@v1`) so that updates land as reviewable PRs rather than silently following whatever the upstream major tag points at.
 - The action interface (`action.yml`, `save/action.yml`) must remain compatible with `actions/cache` inputs/outputs (`path`, `key`, `restore-keys`, `cache-hit`, `cache-matched-key`).
 - Every change ships with a test in `.github/workflows/ci.yml`.
 - Tag releases as `v2`, `v3`, etc. (major only). Use floating major tags.

README.md

@@ -16,7 +16,7 @@ With `local-cache`, the artifact lives on the machine's local disk. On the first
 
 Cache entries are stored as plain directories under `cache-dir/entries/<key>/`. On restore, `rsync -a` copies the entry to the target path. A marker file (`.local-cache-restore`) in the target records which key was last restored:
 
-- **Marker matches current key** → restore is skipped entirely (constant-time work)
+- **Marker matches the matched entry** → restore is skipped entirely (constant-time work). For prefix matches, "matched entry" is the resolved on-disk entry name, not the caller's `key` input.
 - **Marker missing or different key** → target is cleaned and re-synced from cache
 - **No marker (v1 upgrade)** → treated as stale, cleaned and re-synced
 
@@ -158,7 +158,7 @@ Use `local-cache` when you cannot control where a tool installs itself. The Flut
 
 ## Limitations
 
-- **No TTL or eviction.** Cache entries accumulate until manually deleted. For artifacts that change infrequently (e.g. Flutter SDK, updated monthly) this is fine. Clean up with `rm -rf cache-dir/entries/`.
+- **No TTL or eviction.** Cache entries accumulate until manually deleted. For artifacts that change infrequently (e.g. Flutter SDK, updated monthly) this is fine. Clean up with `rm -rf cache-dir/entries/*`.
 - **Each restore is a full copy.** When the marker doesn't match (version bump, first v2 restore), the full artifact is copied from cache to target. For a 1.8 GB Flutter SDK this takes a few seconds on SSD — trivial compared to the network download it replaces.
 - **macOS Spotlight indexing.** On macOS runners, restoring large cache entries (e.g. the Flutter SDK) can trigger `mds` / `mds_stores` to re-index the restored files, causing CPU spikes. Exclude the runner's root directory (or at minimum the `cache-dir`) from Spotlight indexing via System Settings > Spotlight > Privacy, or programmatically with `mdutil -i off /path/to/runner`.
 - **Windows Defender on WSL2.** If your runners run inside WSL2 and you notice CPU spikes from `MsMpEng.exe` after cache restores, Windows Defender may be scanning files written to the WSL2 filesystem. Add the WSL2 distribution's directory to the Defender exclusion list in Windows Security settings.

lib/cache-restore.sh

@@ -20,6 +20,13 @@
 set -e
 
 MARKER_NAME=".local-cache-restore"
+# Marker format version. Bumped when the on-disk layout of either the marker
+# file or the cache entries changes in a backward-incompatible way (e.g. v1
+# used hard links; v2 uses full rsync copies). The marker content is
+# "${MARKER_VERSION}:<entry name>", and a mismatch triggers a clean re-sync.
+# Referenced by literal in .github/workflows/ci.yml marker tests — keep those
+# literals in sync if you bump this.
+MARKER_VERSION="v2"
 
 path_to_cache="$1"
 cache_key="$2"
@@ -59,7 +66,7 @@ append_summary() {
 # previous v2 restore.  Returns 0 (true) if the marker matches.
 is_current() {
     marker="${path_to_cache}/${MARKER_NAME}"
-    [ -f "$marker" ] && [ "$(cat "$marker")" = "v2:$1" ]
+    [ -f "$marker" ] && [ "$(cat "$marker")" = "${MARKER_VERSION}:$1" ]
 }
 
 do_restore() {
@@ -93,7 +100,7 @@ do_restore() {
     rsync -a "$entry_path/" "$path_to_cache/"
 
     # Write the v2 marker so future restores with the same key skip.
-    printf 'v2:%s' "$matched_key" > "${path_to_cache}/${MARKER_NAME}"
+    printf '%s:%s' "$MARKER_VERSION" "$matched_key" > "${path_to_cache}/${MARKER_NAME}"
 
     elapsed=$(( $(date +%s) - start_time ))
     size=$(du -sh "$path_to_cache" 2>/dev/null | cut -f1 || printf '?')
@@ -119,8 +126,13 @@ case "$safe_key" in
     .|..) printf '::error::cache-restore: key must not be "." or ".."\n'; exit 1 ;;
 esac
 
-entry_count=$(find "${entries_dir}/" -maxdepth 1 -mindepth 1 -type d 2>/dev/null | wc -l | tr -d ' ' || printf '0')
-printf '::debug::Checking local cache — key: %s, entries: %s\n' "$cache_key" "$entry_count"
+# Only count entries when debug logging is actually on — a production restore
+# that hits the marker-skip happy path must be constant-time work, and the
+# entries dir can hold enough directories that `find | wc -l` is a real cost.
+if [ "${RUNNER_DEBUG:-}" = "1" ]; then
+    entry_count=$(find "${entries_dir}/" -maxdepth 1 -mindepth 1 -type d 2>/dev/null | wc -l | tr -d ' ' || printf '0')
+    printf '::debug::Checking local cache — key: %s, entries: %s\n' "$cache_key" "$entry_count"
+fi
 
 if [ -d "${entries_dir}/${safe_key}" ]; then
     do_restore "${entries_dir}/${safe_key}" "$cache_key" "true"

lib/cache-save.sh

@@ -20,6 +20,9 @@
 # fully-written new one — never a partial directory.
 set -e
 
+# SYNC: must match lib/cache-restore.sh:MARKER_NAME exactly.
+MARKER_NAME=".local-cache-restore"
+
 path_to_cache="$1"
 cache_key="$2"
 cache_dir="$3"
@@ -77,7 +80,10 @@ cleanup_tmp() {
 trap cleanup_tmp EXIT INT TERM
 
 printf '::debug::Saving to local cache: %s\n' "$cache_key"
-rsync -a "${path_to_cache}/" "${tmp_entry}/"
+# Exclude the restore marker so a prefix-hit restore followed by save on the
+# same path (restore → install → save, the canonical README pattern) doesn't
+# carry the previous entry's name into the new entry on disk.
+rsync -a --exclude="${MARKER_NAME}" "${path_to_cache}/" "${tmp_entry}/"
 mv "$tmp_entry" "${entries_dir}/${safe_key}"
 
 elapsed=$(( $(date +%s) - start_time ))

save/action.yml

@@ -1,4 +1,4 @@
-name: 'Local Cache (Save)'
+name: 'Runner Local Cache (Save)'
 description: >-
   Save a directory to the local disk cache on self-hosted runners.
   Call this after your install step, conditional on a cache miss from
@@ -32,7 +32,7 @@ runs:
     # machine reboot — which is why local-mutex needs no PID tracking or
     # stale-lock recovery code.
     - name: Save cache
-      uses: curlewlabs-com/local-mutex@v1.0.0
+      uses: curlewlabs-com/local-mutex@v1.0.1
       env:
         INPUT_PATH: ${{ inputs.path }}
         INPUT_KEY: ${{ inputs.key }}