Close the NuGet isolation review findings

CHANGELOG.md

@@ -18,7 +18,7 @@ All notable changes to this project are documented here. The format follows [Kee
 
 ### Changed
 
-- For contributors: `tests/run.sh` and the new `tests/manager.sh` restore into a packages folder and an HTTP cache of their own under `dist/nuget-runs`, removed when the run ends, so a package built by a test cannot reach the machine's global-packages folder. Third-party packages a run downloads from an `https` feed move into a shared fallback folder (`dist/nuget-shared`, or `MSBUILDKIT_TESTS_NUGET_SHARED_DIR`) that later runs read; kit packages never enter it, and the machine's folder is refused there and compared before and after each run. See [CONTRIBUTING.md](./CONTRIBUTING.md#nuget-packages-during-a-run). The kit itself is unchanged.
+- For contributors: `tests/run.sh` and the new `tests/manager.sh` restore into a packages folder and an HTTP cache of their own under `dist/nuget-runs`, removed when the run ends, so a package built by a test cannot reach the machine's global-packages folder. Third-party packages a run downloads from an `https` feed move into a shared fallback folder (`dist/nuget-shared`, or `MSBUILDKIT_TESTS_NUGET_SHARED_DIR`) that later runs read; kit packages never enter it, a folder that already holds one is refused, and the machine's folder is refused there (also behind a link) and compared before and after each run. See [CONTRIBUTING.md](./CONTRIBUTING.md#nuget-packages-during-a-run). The kit itself is unchanged.
 - The documentation moved from the README into [docs/](./docs/README.md), one page per topic in reading order, with a [property reference](./docs/reference/properties.md) and a [code reference](./docs/reference/codes.md) that cover everything the kit sets, reads and reports. Corrected along the way: most packaging defaults apply to every project, not only packable ones; a Roslyn project imports its role's props itself; an update rewrites more than `.toolkit/msbuild/`; any tag build is a release build.
 
 ### Fixed

CONTRIBUTING.md

@@ -31,7 +31,7 @@ It builds and tests the tool from `manager/`, so its `global.json` selects Micro
 
 ### NuGet packages during a run
 
-Both scripts restore into folders of their own and share third-party packages between runs, so a package built by a test never reaches the machine's global-packages folder (`dotnet nuget locals global-packages --list`), where any other build on the machine would resolve it instead of the published one.
+Both scripts restore into folders of their own and share third-party packages between runs. A run never restores from or writes to the machine's global-packages folder (`dotnet nuget locals global-packages --list`), so a package built by a test cannot reach it, where any other build on the machine would resolve it instead of the published one.
 
 | Folder | What | Lifetime |
 | --- | --- | --- |
@@ -40,10 +40,11 @@ Both scripts restore into folders of their own and share third-party packages be
 | `dist/nuget-shared` | the shared fallback folder (`NUGET_FALLBACK_PACKAGES`): third-party packages earlier runs downloaded | kept; delete it to start cold |
 
 - **Harvest.** When a run ends, each package it downloaded from an `https` feed moves into the shared folder, staged first and published with one rename, so parallel runs and a killed run cannot leave a half-written package. A package from a local folder, a loopback feed or plain `http` never moves there, and neither does a **kit package**: an id that starts with one of `DragoAnt.MSBuildKit`, `DragoAnt.Fixture.`, `DragoAnt.Samples.`. So a restore never gets a shared copy in place of a fresh local build.
-- **Variables.** `MSBUILDKIT_TESTS_NUGET_SHARED_DIR` names another shared folder, for example one that several checkouts use; the scripts refuse the machine's global-packages folder there. `MSBUILDKIT_TESTS_KIT_PACKAGE_PREFIXES` replaces the kit prefixes (`;`-separated).
+- **Variables.** `MSBUILDKIT_TESTS_NUGET_SHARED_DIR` names another shared folder, for example one that several checkouts use; the scripts refuse the machine's global-packages folder there, also behind a link or another spelling, and refuse a folder that already holds a kit package. `MSBUILDKIT_TESTS_KIT_PACKAGE_PREFIXES` replaces the kit prefixes (`;`-separated); a prefix matches the id itself and every id that continues it after a dot, in any case.
 - **Clearing.** `rm -rf dist/nuget-shared` (or your own folder); the next run downloads again. A run killed outright leaves its `dist/nuget-runs` folder behind, and the next run removes it: only folders that carry the run marker file and whose process is gone.
 - `--no-nuget-fallback` neither reads nor fills the shared folder. A caller's `NUGET_PACKAGES`, `NUGET_HTTP_CACHE_PATH` or `NUGET_FALLBACK_PACKAGES` is used as it is; with your own `NUGET_PACKAGES` nothing is harvested.
-- **Guard.** Each run records the kit packages in the machine's folder with their hashes when it starts, and fails at its end if that list changed or the folder holds a package the run built. `tests/nuget-isolation.test.sh` checks the rules on hand-made folders, with a stand-in for the machine's folder.
+- **Guard.** The check only reads the machine's folder: each run lists the kit packages there with their hashes when it starts, and fails at its end if that list changed (added, changed or removed) or the folder holds a package the run built. It cannot see a third-party package that was changed or deleted there, nor a package packed outside the run's output directory whose id has no kit prefix.
+- A package that cannot be moved into the shared folder, and a run folder that cannot be removed, are reported in a `NOTE` line and do not fail the run; the next run removes the folder. The scripts stop no process and clear no machine-wide cache. `tests/nuget-isolation.test.sh` checks the rules on hand-made folders, with a stand-in for the machine's folder.
 - A test reads restored packages through `ni_packages_dir`; a test file that names a packages folder itself fails the run. A scenario that needs a folder of its own sets `NUGET_PACKAGES` for that command: the variable outranks `globalPackagesFolder` in a `nuget.config`.
 - Do not wrap the scripts in `timeout`: it ends a child `dotnet` process in the middle of a restore.
 

tests/nuget-isolation.sh

@@ -13,14 +13,17 @@ ni_default_prefixes='DragoAnt.MSBuildKit;DragoAnt.Fixture.;DragoAnt.Samples.'
 
 ni_native() { if command -v cygpath > /dev/null 2>&1; then cygpath -m "$1"; else printf '%s\n' "$1"; fi; }
 
-# ni_norm <path>: absolute, forward slashes, no trailing slash, lower case where the file system ignores case.
+ni_long() { if command -v cygpath > /dev/null 2>&1; then cygpath -m -l "$1"; else printf '%s\n' "$1"; fi; }
+
+# ni_norm <path>: absolute with every link and short name resolved, forward slashes, no trailing slash,
+# lower case where the file system ignores case.
 ni_norm() {
   ni_p=$(printf '%s' "$1" | tr '\\' '/')
   if command -v cygpath > /dev/null 2>&1; then ni_p=$(cygpath -u "$ni_p"); fi
   case "$ni_p" in /*) ;; *) ni_p="$PWD/$ni_p" ;; esac
   ni_rest=""
   while [ ! -d "$ni_p" ]; do ni_rest="/${ni_p##*/}$ni_rest"; ni_p=${ni_p%/*}; [ -n "$ni_p" ] || ni_p=/; done
-  ni_p=$(cd "$ni_p" && pwd -P); ni_p=$(ni_native "${ni_p%/}$ni_rest" | sed 's:/\.\{0,1\}$::; s:/\./:/:g; s:/*$::')
+  ni_p=$(cd "$ni_p" && pwd -P); ni_p=$(ni_long "${ni_p%/}$ni_rest" | sed 's:/\.\{0,1\}$::; s:/\./:/:g; s:/*$::')
   case "$(uname -s)" in
     MINGW*|MSYS*|CYGWIN*|Darwin) printf '%s\n' "$ni_p" | tr '[:upper:]' '[:lower:]' ;;
     *) printf '%s\n' "${ni_p:-/}" ;;
@@ -42,11 +45,22 @@ ni_check_shared() {
   return 1
 }
 
-# Reads package folder names (ids) on stdin; prints those that start (kit) or do not start (other) with a kit prefix.
+# ni_check_shared_content <shared>: fails when the folder holds a kit package, which a restore would
+# take in place of the one the run packs.
+ni_check_shared_content() {
+  [ -d "$1" ] || return 0
+  ni_stale=$(ls -1 "$1" | ni_filter_ids kit | tr '\n' ' ')
+  [ -n "$ni_stale" ] || return 0
+  echo "nuget isolation: the shared folder $1 holds kit packages: ${ni_stale}- remove them, or the folder" >&2
+  return 1
+}
+
+# The one kit-package test. Reads package ids on stdin; prints the kit packages (kit) or the rest (other).
+# An id is a kit package when it equals a prefix or continues it after a dot, in any case.
 ni_filter_ids() {
   awk -v want="$1" -v list="$(printf '%s' "${MSBUILDKIT_TESTS_KIT_PACKAGE_PREFIXES-$ni_default_prefixes}" | tr ';,' '  ')" '
-    BEGIN { n = split(tolower(list), p, " ") }
-    { kit = 0; id = tolower($0); for (i = 1; i <= n; i++) if (index(id, p[i]) == 1) kit = 1
+    BEGIN { n = split(tolower(list), p, " "); for (i = 1; i <= n; i++) sub(/[.]$/, "", p[i]) }
+    { kit = 0; id = tolower($0); for (i = 1; i <= n; i++) if (id == p[i] || index(id, p[i] ".") == 1) kit = 1
       if ((want == "kit") == kit) print }'
 }
 
@@ -61,19 +75,39 @@ ni_sweep() {
   for ni_d in "$1"/*/ "$1"/.[!.]*/; do
     [ -f "$ni_d$ni_marker" ] || continue
     ni_pid=$(sed -n 's/^pid=//p' "$ni_d$ni_marker")
+    case "$ni_pid" in ""|*[!0-9]*) continue ;; esac
     if [ "$ni_pid" != "$$" ] && ! kill -0 "$ni_pid" 2> /dev/null; then rm -rf "$ni_d"; fi
   done
 }
 
+# ni_mark <dir>: creates <dir> as this run's own; it gets its name only once the marker is inside.
+ni_mark() {
+  mkdir -p "${1%/*}"; ni_new=$(mktemp -d "$1.new.XXXXXX")
+  printf 'pid=%s\n' "$$" > "$ni_new/$ni_marker"
+  mv "$ni_new" "$1"
+}
+
+ni_rm() { rm -rf "$1"; }
+ni_retry_pause() { sleep 1; }
+
+# ni_remove <dir>: three tries; a folder that stays keeps its marker, so the next run's sweep takes it.
+ni_remove() {
+  for ni_try in 1 2 3; do
+    if ni_rm "$1" 2> /dev/null && [ ! -e "$1" ]; then return 0; fi
+    ni_retry_pause
+  done
+  echo "NOTE  could not remove $1; the next run removes it" >&2
+}
+
 ni_take_tree() { mv "$1" "$2"; }
 
 # ni_harvest <packages-dir> <shared-dir>: moves every package <packages-dir> downloaded from an https feed
 # that is not a kit package and not yet shared into <shared-dir>; prints how many. Each package is
 # staged first and published with one rename, so a reader or a parallel run sees it whole or not at all.
 ni_harvest() {
   ni_count=0
-  mkdir -p "$2/.staging"; ni_stage=$(mktemp -d "$2/.staging/$$.XXXXXX")
-  printf 'pid=%s\n' "$$" > "$ni_stage/$ni_marker"
+  mkdir -p "$2/.staging"; ni_stage=$(mktemp -u "$2/.staging/$$.XXXXXX")
+  ni_mark "$ni_stage"
   ni_others=" $(ls -1 "$1" 2> /dev/null | ni_filter_ids other | tr '\n' ' ') "
   for ni_meta in "$1"/*/*/.nupkg.metadata; do [ ! -f "$ni_meta" ] || printf '%s\n' "$ni_meta"; done > "$ni_stage/metadata"
   awk '{ file = $0; source = ""
@@ -91,9 +125,11 @@ ni_harvest() {
     mkdir -p "$ni_stage/$ni_id" "$2/$ni_id"
     if ni_take_tree "$ni_vdir" "$ni_stage/$ni_id/$ni_ver" && mv "$ni_stage/$ni_id/$ni_ver" "$2/$ni_id/" 2> /dev/null; then
       ni_count=$((ni_count+1))
+    elif [ ! -e "$2/$ni_id/$ni_ver" ]; then
+      echo "NOTE  could not save $ni_id $ni_ver into the shared folder; a later run downloads it again" >&2
     fi
   done < "$ni_stage/sources"
-  rm -rf "$ni_stage"
+  ni_remove "$ni_stage"
   echo "$ni_count"
 }
 
@@ -105,11 +141,12 @@ ni_begin() {
   if [ "$2" = fallback ] && [ -z "${NUGET_FALLBACK_PACKAGES:-}" ]; then
     ni_shared=$(ni_shared_dir)
     ni_check_shared "$ni_shared" "$ni_machine" || exit 1
+    ni_check_shared_content "$ni_shared" || exit 1
   fi
   ni_runs="$here/dist/nuget-runs"
   ni_sweep "$ni_runs"
   ni_run="$ni_runs/$1.$$.$(date +%s)"; ni_own=""
-  mkdir -p "$ni_run"; printf 'pid=%s\n' "$$" > "$ni_run/$ni_marker"
+  ni_mark "$ni_run"
   trap ni_end EXIT
   trap 'exit 129' HUP; trap 'exit 130' INT; trap 'exit 143' TERM
   ni_snapshot "$ni_machine" > "$ni_run/machine-before"
@@ -134,7 +171,7 @@ ni_end() {
     ni_moved=$(ni_harvest "$ni_own" "$ni_shared") || ni_moved="?"
     echo "NOTE  $ni_moved downloaded package(s) moved into the shared folder $ni_shared"
   fi
-  [ -z "${ni_run:-}" ] || rm -rf "$ni_run" || echo "NOTE  could not remove $ni_run"
+  [ -z "${ni_run:-}" ] || ni_remove "$ni_run"
   ni_own=""; ni_shared=""; ni_run=""
 }
 

tests/nuget-isolation.test.sh

@@ -108,7 +108,7 @@ kill "$live" 2> /dev/null || true
   || bad "sweep left: $(ls -A "$root" | tr '\n' ' ')"
 
 # --- the run lifecycle, with a stub for the machine's folder --------------------------------------
-repo="$t/repo"; machine="$t/machine"; mkdir -p "$repo/dist" "$machine/somepkg/1.0.0"
+repo="$t/repo"; machine="$t/machine"; machine_long="$t/machine with a longer name"; mkdir -p "$repo/dist" "$machine/somepkg/1.0.0" "$machine_long"
 printf 'machine\n' > "$machine/somepkg/1.0.0/file"
 machine_before=$(listing "$machine")
 cat > "$t/runner.sh" <<'EOF'
@@ -201,5 +201,74 @@ for want in "added dragoant.fixture.cacheprobe/1.0.0 (the run packed it)" "chang
 done
 grep -q newtonsoft "$t/guard.red" && bad "the guard reported a third-party package" || pass "the guard ignores third-party packages"
 
+# --- a folder another run is still creating --------------------------------------------------------
+root="$t/half"; mkdir -p "$root/no-marker" "$root/empty-marker" "$root/odd-marker"
+: > "$root/empty-marker/$ni_marker"; printf 'pid=soon\n' > "$root/odd-marker/$ni_marker"
+ni_sweep "$root"
+[ -d "$root/no-marker" ] && [ -d "$root/empty-marker" ] && [ -d "$root/odd-marker" ] \
+  && pass "the sweep leaves a folder whose marker is missing or not yet written" || bad "the sweep removed a half-created folder: left $(ls -A "$root" | tr '\n' ' ')"
+ni_mark "$root/fresh"
+[ "$(cat "$root/fresh/$ni_marker")" = "pid=$$" ] && [ "$(ls -A "$root/fresh")" = "$ni_marker" ] \
+  && pass "a run's folder appears with its marker already written" || bad "ni_mark left: $(ls -A "$root/fresh" 2> /dev/null | tr '\n' ' ')"
+
+# --- the machine's folder behind a link ----------------------------------------------------------
+link="$t/machine-link"; linked_parent="$t/parent-link"
+case "$(uname -s)" in
+  MINGW*|MSYS*|CYGWIN*)
+    cmd //c mklink //J "$(cygpath -w "$link")" "$(cygpath -w "$machine")" > /dev/null
+    cmd //c mklink //J "$(cygpath -w "$linked_parent")" "$(cygpath -w "$t")" > /dev/null ;;
+  *) ln -s "$machine" "$link"; ln -s "$t" "$linked_parent" ;;
+esac
+refused=0
+for candidate in "$link" "$linked_parent/machine" "$linked_parent/machine-link/"; do
+  ni_check_shared "$candidate" "$machine" 2> /dev/null && bad "a link to the machine's folder was accepted: $candidate" || refused=$((refused+1))
+done
+case "$(uname -s)" in MINGW*|MSYS*|CYGWIN*)
+  short=$(cygpath -m -s "$machine_long" 2> /dev/null || true)
+  if [ -n "$short" ] && [ "$short" != "$(cygpath -m "$machine_long")" ]; then
+    ni_check_shared "$short" "$machine_long" 2> /dev/null && bad "the short (8.3) name of the machine's folder was accepted: $short" || pass "the short (8.3) name of the machine's folder is refused"
+  fi ;;
+esac
+[ "$refused" = 3 ] && pass "a link to the machine's folder, in any path segment, is refused" || bad "refused $refused of 3 links to the machine's folder"
+for l in "$link" "$linked_parent"; do rm "$l" 2> /dev/null || rmdir "$l"; done
+
+# --- one kit-package predicate, exact on id boundaries --------------------------------------------
+kit=$(printf '%s\n' dragoant.msbuildkit DragoAnt.MSBuildKit.Manager dragoant.msbuildkitfoo dragoant.fixture.cacheprobe dragoant.fixtures newtonsoft.json | ni_filter_ids kit | tr '\n' ' ')
+[ "$kit" = "dragoant.msbuildkit DragoAnt.MSBuildKit.Manager dragoant.fixture.cacheprobe " ] \
+  && pass "a kit prefix matches the id itself and ids under it, in any case, and no longer id" || bad "kit ids: '$kit'"
+other=$(printf '%s\n' dragoant.msbuildkit dragoant.msbuildkitfoo newtonsoft.json | ni_filter_ids other | tr '\n' ' ')
+[ "$other" = "dragoant.msbuildkitfoo newtonsoft.json " ] && pass "every id is a kit package or not, never both" || bad "other ids: '$other'"
+
+# --- a shared folder that already holds a kit package ---------------------------------------------
+shared="$t/stale/shared"
+mkpkg "$shared" newtonsoft.json 13.0.3 "$nuget_org"; mkdir -p "$shared/.staging"
+ni_check_shared_content "$shared" 2> /dev/null && pass "a shared folder of third-party packages is accepted" || bad "a clean shared folder was refused"
+mkpkg "$shared" dragoant.msbuildkit.manager 0.1.0 "$nuget_org"
+if ni_check_shared_content "$shared" 2> "$t/stale.log"; then bad "a shared folder holding a kit package was accepted"
+else grep -q "dragoant.msbuildkit.manager" "$t/stale.log" && pass "a shared folder holding a kit package is refused by name" || bad "refusal without the package: $(cat "$t/stale.log")"; fi
+(
+  here="$repo"
+  ni_machine_folder() { printf '%s\n' "$machine"; }
+  MSBUILDKIT_TESTS_NUGET_SHARED_DIR="$shared"
+  ni_begin stale fallback
+) > "$t/stale-run.log" 2>&1 && bad "a run started on a shared folder holding a kit package" || pass "a run refuses a shared folder holding a kit package"
+
+# --- a package that cannot be saved ---------------------------------------------------------------
+src="$t/w/run"; shared="$t/w/shared"
+mkpkg "$src" newtonsoft.json 13.0.3 "$nuget_org"; mkpkg "$src" xunit.v3 4.0.1 "$nuget_org"
+status=0
+moved=$(ni_take_tree() { case "$1" in */newtonsoft.json/*) return 1 ;; *) mv "$1" "$2" ;; esac; }; ni_harvest "$src" "$shared" 2> "$t/w.err") || status=$?
+[ "$status" = 0 ] && [ "$moved" = 1 ] && grep -q "newtonsoft.json 13.0.3" "$t/w.err" && ! grep -q xunit "$t/w.err" \
+  && pass "a package that cannot be saved is named in a warning and the harvest goes on" || bad "failed save: exit $status, moved $moved, said '$(cat "$t/w.err")'"
+
+# --- a folder that cannot be removed --------------------------------------------------------------
+mkdir -p "$t/stuck"
+status=0
+tries=$(ni_rm() { echo try; return 1; }; ni_retry_pause() { :; }; ni_remove "$t/stuck" 2> "$t/stuck.err" | grep -c try) || status=$?
+[ "$tries" = 3 ] && grep -q "$t/stuck" "$t/stuck.err" && pass "a folder that cannot be removed is retried, reported and left to the next run's sweep" \
+  || bad "stuck folder: $tries tries, said '$(cat "$t/stuck.err")'"
+if grep -nE 'build-server|taskkill|pkill|killall|locals[^|]*--clear|locals[^|]* -c( |$)' "$here/tests/nuget-isolation.sh" > "$t/wide"; then bad "nuget-isolation.sh acts on the whole machine: $(cat "$t/wide")"
+else pass "nuget-isolation.sh stops no process and clears no machine cache"; fi
+
 echo
 if [ "$failures" -eq 0 ]; then echo "nuget isolation unit checks: all passed"; else echo "nuget isolation unit checks: $failures failure(s)"; exit 1; fi