fix(hardlink): resolve an ambiguous out/ toward unsharing, and pin the linked-output scenario on a shape that exhibits it

Two review findings on #16. The `out/` discriminator keyed on Cargo's record of a build-script execution, which Cargo writes only AFTER the script exits successfully. A build script that populates OUT_DIR and then fails leaves a unit with no record at all, so its OUT_DIR read as a compile unit's artifact directory and stayed shared — a regression against the old `-name build` selection, which real-copied that state by construction. Reproduced on cargo 1.93.1 stable. An `out` directory now stays shared only when two independent signals agree: it holds an `.rlib`/`.rmeta` of its own, and its unit carries no execution record. Either one missing real-copies it. The cost is unchanged to the byte — the newly-unshared directories hold only executables and `*.d`, both already privately owned by the file rules. The live linked-test-binary scenario was built on the lib+bin probe crate, whose test binaries relink to a fresh inode — a shape gitdan-actions#17 records as measured safe. Both halves passed green against the unfixed selection on dep-info mutations the previous scenario already covers. It now builds a bin-only crate with a unit test, reads only executables, and skips loudly with a warning rather than passing quietly if the toolchain does not exhibit the rewrite at all. Also: drop a clause asserting the linker writes in place "whenever the path has no other hard link", which this change's own evidence denies; move the load-bearing comment block back above `unshare_mutable_paths`; correct a superseded 99.998% figure; and record both cost rows in the README rather than only the flattering whole-tree one.
2026-08-27 14:39:29 -05:00
parent e5b26a9368
commit 0553a6b956
3 changed files with 187 additions and 73 deletions
@@ -317,6 +317,73 @@ _unshare_files() {
    xargs -0 -r -n 64 bash -c 'rc=0; for f; do cp -p -- "$f" "$f.unshare.$$" && mv -f -- "$f.unshare.$$" "$f" || rc=1; done; exit $rc' _
 }

+# True when a directory holds a compiled library artifact of its own.
+#
+# The glob is left unquoted and unmatched-glob-safe on purpose: with nullglob
+# off an unmatched pattern stays literal and the `-e` test fails, which is the
+# answer wanted.
+_holds_compiled_artifact() {
+  local f
+  for f in "$1"/*.rlib "$1"/*.rmeta; do
+    [ -e "$f" ] && return 0
+  done
+  return 1
+}
+
+# The directories `unshare_mutable_paths` replaces, under either layout.
+#
+# All four names are pruned, so nothing selected here can contain anything else
+# selected here and the caller never unshares a subtree twice.
+#
+# `out` is the one that needs deciding rather than naming, and it is the whole
+# difficulty of layout v2: a compile unit's rlib and a build script's OUT_DIR
+# are both a directory called `out`, one directory apart, and they need
+# opposite treatment.
+#
+# AMBIGUITY RESOLVES TOWARD UNSHARING, and that direction is the rule rather
+# than a default: over-unsharing costs bytes, under-unsharing costs corruption.
+# So an `out` directory is left shared only when TWO independent signals agree
+# it is a compile unit's artifact directory, and either one missing is enough
+# to real-copy it:
+#
+#   1. it holds an `.rlib`/`.rmeta` of its own — the artifact whose sharing is
+#      the entire point of the clone; and
+#   2. its unit directory has no record of a build-script execution beside it
+#      (`run/` under layout v2, a loose `root-output` under v1).
+#
+# Signal 2 alone was the first cut of this and it is NOT sufficient, because
+# Cargo writes `root-output` only AFTER the script exits successfully. A build
+# script that populates `OUT_DIR` and then FAILS leaves a unit with no record
+# at all, which reads as "compile unit" — and the old `-name build` selection
+# covered that state by real-copying `build/` wholesale, so trusting signal 2
+# alone was a regression against it. Reproduced on cargo 1.93.1 stable: the
+# clone's build wrote through the shared inode into the source's OUT_DIR.
+# Signal 1 closes it, because a failed build script's OUT_DIR holds no rlib.
+#
+# The residual is a build script that writes a file NAMED `*.rlib`/`*.rmeta`
+# into `OUT_DIR` and has never once succeeded. Nothing bounds that away; it is
+# simply far narrower than what it replaces.
+#
+# Requiring signal 1 also means a bin, test or build-script COMPILE unit's
+# `out` is real-copied rather than shared — at no cost in bytes, since
+# everything in one is an executable or a `*.d`, and both are privately owned
+# by the file rules below either way.
+_mutable_dirs() {
+  local root="$1" d unit
+  while IFS= read -r d; do
+    if [ "${d##*/}" = out ]; then
+      unit="${d%/out}"
+      if ! [ -d "$unit/run" ] && ! [ -e "$unit/root-output" ] \
+         && _holds_compiled_artifact "$d"; then
+        continue
+      fi
+    fi
+    printf '%s\n' "$d"
+  done < <(find "$root" -type d \
+    \( -name .fingerprint -o -name fingerprint -o -name run -o -name out \) \
+    -prune -print 2>/dev/null)
+}
+
 # THE load-bearing function of this whole design.
 #
 # A hardlink clone is only safe if every write the clone's build performs
@@ -403,7 +470,7 @@ _unshare_files() {
 #      included. Bracketed locally: cargo 1.97.1 and 1.98.0-nightly write v1,
 #      1.100.0-nightly writes v2.
 #
-# Until 2026-08-27 the selection below was `-name .fingerprint -o -name build`,
+# Until 2026-08-27 the selection was `-name .fingerprint -o -name build`,
 # which under a v2 Cargo matched nothing on its first clause and the entire
 # tree on its second, because the artifacts moved under `build/` too. The guard
 # held by accident and the saving did not: on one scratch crate (serde +
@@ -423,33 +490,6 @@ _unshare_files() {
 # and garbage-collects old sessions by unlinking directory entries — neither
 # of which mutates a shared inode. CI should still set CARGO_INCREMENTAL=0,
 # for size rather than correctness.
-# The directories `unshare_mutable_paths` replaces, under either layout.
-#
-# All four names are pruned, so nothing selected here can contain anything else
-# selected here and the caller never unshares a subtree twice.
-#
-# `out` is the one that needs deciding rather than naming, and it is the whole
-# difficulty of layout v2: a compile unit's rlib and a build script's OUT_DIR
-# are both a directory called `out`, one directory apart, and they need
-# opposite treatment. The discriminator is structural — Cargo records a build
-# script's execution beside its OUT_DIR and nowhere else, as `run/root-output`
-# under v2 and as a loose `root-output` under v1 — so a unit directory holding
-# one of those is a RUN unit and its `out` is the OUT_DIR. A unit directory
-# without one is a compile unit, and its `out` holds the artifact whose sharing
-# is the entire point of the clone.
-_mutable_dirs() {
-  local root="$1" d unit
-  while IFS= read -r d; do
-    if [ "${d##*/}" = out ]; then
-      unit="${d%/out}"
-      [ -d "$unit/run" ] || [ -e "$unit/root-output" ] || continue
-    fi
-    printf '%s\n' "$d"
-  done < <(find "$root" -type d \
-    \( -name .fingerprint -o -name fingerprint -o -name run -o -name out \) \
-    -prune -print 2>/dev/null)
-}
-
 unshare_mutable_paths() {
  local root="$1" d
  [ -d "$root" ] || return 0
@@ -479,12 +519,14 @@ unshare_mutable_paths() {
  }
  # Linked outputs. Unlike an rlib or an rmeta — which rustc writes to a
  # temporary and renames into place — an executable or shared object is
-  # written by the LINKER, and the linker writes THROUGH an existing inode
-  # whenever the path it is given has no other hard link. Measured 2026-08-27
-  # on cargo 1.98.0-nightly (a335d47ff, layout v1) and 1.100.0-nightly
-  # (e8cb624d5, layout v2), mold and the default linker alike: a `cargo test`
-  # binary in a `cp -al` clone rewrote the SOURCE's copy of itself in place,
-  # under both layouts.
+  # written by the LINKER, and the linker writes THROUGH an existing inode.
+  # Measured 2026-08-27 on cargo 1.93.1 stable, 1.96.0-nightly, 1.98.0-nightly
+  # (layout v1) and 1.100.0-nightly (e8cb624d5, layout v2), mold and the
+  # default linker alike: a `cargo test --no-run` binary in a `cp -al` clone
+  # rewrote the SOURCE's copy of itself in place, under both layouts. Hardlink
+  # count at link time is NOT what decides it — measured 2 in both a rewritten
+  # and an intact case, and two test binaries of one crate at `nlink == 1`
+  # behaved differently from each other. See gitdan-actions#17.
  #
  # The executable bit is the discriminator because it is the linker's own
  # output that is at risk, not the directory it happens to land in — `.rlib`,
@@ -124,7 +124,8 @@ assert_partition() {

  # The cost model, asserted rather than assumed. Selecting too much is not a
  # correctness bug, which is exactly why nothing caught layout v2 taking the
-  # selection from a fifth of the tree to 99.998% of it (gitdan-actions#14):
+  # selection from 39.3% of one scratch crate's tree to 99.996% of it
+  # (gitdan-actions#14):
  # a hardlink clone that real-copies everything is a `cp -a` with extra steps.
  # The bound is loose on purpose. It is not a budget — the honest figure moves
  # with how much of a tree is linker output, and these fixtures are mostly
@@ -186,6 +187,12 @@ mkfile "$v2/debug/build/probe/da96cf45111f80dd/out/gen.txt" 'generated from 24 b
 for f in invoked.timestamp root-output stdout stderr; do
  mkfile "$v2/debug/build/probe/da96cf45111f80dd/run/$f" "$f"
 done
+# A build script that wrote into OUT_DIR and then FAILED: Cargo records the
+# run only on success, so this unit has `out/` populated and no `run/` at all.
+# Reading "no execution record" as "compile unit" left this shared, which is
+# the one state the old `-name build` selection covered and the first cut of
+# this one did not.
+mkfile "$v2/debug/build/probe/f00ded00f00ded00/out/gen.txt" 'half-written'
 mkfile "$v2/debug/incremental/probe-abc/s-xyz/dep-graph.bin" "$artifact_bytes"

 assert_partition "layout v2" "$v2" "$(cat <<'SPEC'
@@ -205,6 +212,7 @@ private|debug/build/probe/da96cf45111f80dd/run/root-output
 private|debug/build/probe/da96cf45111f80dd/run/stdout
 private|debug/build/probe/da96cf45111f80dd/run/stderr
 private|debug/build/probe/da96cf45111f80dd/run/invoked.timestamp
+private|debug/build/probe/f00ded00f00ded00/out/gen.txt
 shared|debug/libprobe.rlib
 shared|debug/build/probe/1bf5493368dce3cd/out/libprobe-1bf5493368dce3cd.rlib
 shared|debug/build/probe/1bf5493368dce3cd/out/libprobe-1bf5493368dce3cd.rmeta
@@ -243,6 +251,8 @@ for f in invoked.timestamp output root-output stderr; do
  mkfile "$v1/debug/build/probe-da96cf45111f80dd/$f" "$f"
 done
 mkfile "$v1/debug/build/probe-da96cf45111f80dd/out/gen.txt" 'generated from 24 bytes'
+# The same never-succeeded build script under layout v1.
+mkfile "$v1/debug/build/probe-f00ded00f00ded00/out/gen.txt" 'half-written'
 mkfile "$v1/debug/incremental/probe-abc/s-xyz/dep-graph.bin" "$artifact_bytes"

 assert_partition "layout v1" "$v1" "$(cat <<'SPEC'
@@ -261,6 +271,7 @@ private|debug/build/probe-da96cf45111f80dd/output
 private|debug/build/probe-da96cf45111f80dd/root-output
 private|debug/build/probe-da96cf45111f80dd/stderr
 private|debug/build/probe-da96cf45111f80dd/out/gen.txt
+private|debug/build/probe-f00ded00f00ded00/out/gen.txt
 shared|debug/libprobe.rlib
 shared|debug/deps/libprobe-1bf5493368dce3cd.rlib
 shared|debug/deps/libprobe-1bf5493368dce3cd.rmeta
@@ -567,44 +578,84 @@ echo "=== a linked TEST binary, which nothing uplifts and nothing replaces ==="
 # The one artifact family that is NOT safe to share, and the reason
 # `unshare_mutable_paths` privately owns every executable. rustc writes an
 # rlib to a temporary and renames it in; the LINKER writes an executable
-# through whatever inode is already at the path. Cargo re-creates the path
-# first when it also has to uplift the result — a bin target's
-# `deps/<bin>-<hash>` has a hardlink twin at `<profile>/<bin>` — which is why
-# bin targets look safe. A test binary has no twin, nothing re-creates it, and
-# the write lands on the source's inode.
+# through whatever inode is already at the path.
 #
-# Measured 2026-08-27 on cargo 1.98.0-nightly (a335d47ff, layout v1) and
-# 1.100.0-nightly (e8cb624d5, layout v2): under BOTH layouts a `cargo test
-# --no-run` inside a raw `cp -al` clone rewrote the SOURCE's own test binary.
+# THE CRATE SHAPE IS LOAD-BEARING AND WAS WRONG ONCE. An earlier cut of this
+# scenario reused the lib+bin probe crate above, whose test binaries relink to
+# a FRESH inode — a shape gitdan-actions#17 records as measured safe. Both
+# halves then passed green against the unfixed selection, on the strength of
+# dep-info mutations the previous scenario already covers, and the scenario
+# pinned nothing. A bin-only crate with a unit test does exhibit the rewrite,
+# on cargo 1.93.1 stable and on 1.98.0-nightly and 1.100.0-nightly, so that is
+# what this builds. `nlink` does not predict which is which — two test binaries
+# of one crate, both at 1, behaved differently — so the shape is chosen by
+# measurement rather than derived.
+mkbincrate() {
+  local dir="$1" marker="$2"
+  mkdir -p "$dir/src"
+  cat > "$dir/Cargo.toml" <<'TOML'
+[package]
+name = "binprobe"
+version = "0.1.0"
+edition = "2021"
+[workspace]
+TOML
+  cat > "$dir/src/main.rs" <<RS
+fn main() { println!("${marker}"); }
+#[cfg(test)]
+mod t { #[test] fn a() { assert_eq!("${marker}".len() > 0, true); } }
+RS
+}
+
+bin_dir="$scratch/binprobe"
+mkbincrate "$bin_dir" MARKER_AAAA
+cd "$bin_dir"
 base_exe="$scratch/base-exe"; clone_exe_ctl="$scratch/clone-exe-ctl"; clone_exe="$scratch/clone-exe"
-printf '%s\n' "$CONTENT_A" > src/lib.rs
-CARGO_TARGET_DIR="$base_exe" "${CARGO_BIN[@]}" test --no-run -q > /dev/null 2>&1
-before=$(snapshot_tree "$base_exe")
+
+# Only the executables are read here. The families the other scenarios cover
+# would satisfy a "something changed" assertion on their own, which is exactly
+# how the earlier cut of this passed while pinning nothing.
+source_exe_digest() {
+  (cd "$1" && find . -type f -executable -print0 | sort -z | xargs -0 -r sha1sum) 2>/dev/null
+}
+
+CARGO_TARGET_DIR="$base_exe" "${CARGO_BIN[@]}" test --no-run -q > /dev/null 2>&1 \
+  || fail "the bin-only probe crate failed to build"
+before=$(source_exe_digest "$base_exe")
 cp -al "$base_exe" "$clone_exe_ctl"
 strip_cargo_locks "$clone_exe_ctl"
-printf '%s\n' "$CONTENT_B" > src/lib.rs
+mkbincrate "$bin_dir" MARKER_BBBB
 CARGO_TARGET_DIR="$clone_exe_ctl" "${CARGO_BIN[@]}" test --no-run -q > /dev/null 2>&1
-exe_ctl_mutated=$(mutated_paths "$before" "$(snapshot_tree "$base_exe")")
-[ -n "$exe_ctl_mutated" ] \
-  || fail "control: a raw cp -al clone of a test build no longer mutates the source — the test can no longer tell fixed from broken"
-ok "raw cp -al clone of a test build mutates the source ($(printf '%s\n' "$exe_ctl_mutated" | wc -l) paths)"
-printf '%s\n' "$exe_ctl_mutated" | sed 's/^/       /'
+exe_ctl_mutated=$(mutated_paths "$before" "$(source_exe_digest "$base_exe")")

-# Rebuild the base from CONTENT_A so it is warm and consistent again, then do
-# the same thing through the real clone.
-printf '%s\n' "$CONTENT_A" > src/lib.rs
-CARGO_TARGET_DIR="$base_exe" "${CARGO_BIN[@]}" test --no-run -q > /dev/null 2>&1
-before=$(snapshot_tree "$base_exe")
-hardlink_clone_into "$base_exe" "$clone_exe" "selftest-exe" \
-  || fail "hardlink_clone_into refused the destination"
-printf '%s\n' "$CONTENT_B" > src/lib.rs
-CARGO_TARGET_DIR="$clone_exe" "${CARGO_BIN[@]}" test --no-run -q > /dev/null 2>&1
-exe_mutated=$(mutated_paths "$before" "$(snapshot_tree "$base_exe")")
-if [ -n "$exe_mutated" ]; then
-  printf '%s\n' "$exe_mutated" | sed 's/^/       /' >&2
-  fail "a test build in the clone mutated the source through a shared inode"
+# THREE OUTCOMES, as the freshness probe above has, and for the same reason: a
+# scenario that cannot tell "the fix works" from "the hazard never fired" is
+# not a gate. If this Cargo does not rewrite the source's test binary, the
+# assertion below would pass for a toolchain reason rather than a code one, so
+# it is skipped LOUDLY instead of passing quietly.
+if [ -z "$exe_ctl_mutated" ]; then
+  echo "::warning::hardlink-clone-selftest: this toolchain did not rewrite the source's test binary through a raw cp -al clone, so the linked-output scenario proves nothing here and was SKIPPED. That is a statement about this Cargo, not about unshare_mutable_paths."
+else
+  ok "control: a raw cp -al clone rewrites the source's own linked test binary"
+  printf '%s\n' "$exe_ctl_mutated" | sed 's/^/       /'
+
+  # Rebuild the base from the original marker so it is warm and consistent
+  # again, then do the same thing through the real clone.
+  mkbincrate "$bin_dir" MARKER_AAAA
+  CARGO_TARGET_DIR="$base_exe" "${CARGO_BIN[@]}" test --no-run -q > /dev/null 2>&1
+  before=$(snapshot_tree "$base_exe")
+  hardlink_clone_into "$base_exe" "$clone_exe" "selftest-exe" \
+    || fail "hardlink_clone_into refused the destination"
+  mkbincrate "$bin_dir" MARKER_BBBB
+  CARGO_TARGET_DIR="$clone_exe" "${CARGO_BIN[@]}" test --no-run -q > /dev/null 2>&1
+  exe_mutated=$(mutated_paths "$before" "$(snapshot_tree "$base_exe")")
+  if [ -n "$exe_mutated" ]; then
+    printf '%s\n' "$exe_mutated" | sed 's/^/       /' >&2
+    fail "a test build in the clone mutated the source through a shared inode"
+  fi
+  ok "no file in the source changed after a full test build in the clone"
 fi
-ok "no file in the source changed after a full test build in the clone"
+cd "$crate_dir"

 echo
 if [ "$CHECKSUM_MODE" = "on" ]; then