ci: collect telemetry on every arch, and retry teardown on any FileSystemException

.github/scripts/collect-bsp-diagnostics.sh

@@ -0,0 +1,87 @@
+#!/usr/bin/env bash
+# Collect the compile server's telemetry and logs into ./bsp-diagnostics for upload.
+#
+# The socket directory sits under bleep's per-user cache dir, which `UserPaths.fromAppDirs` derives from
+# `ProjectDirectories.from("build", null, "bleep")` — a different path on every OS:
+#
+#   linux    ~/.cache/bleep/socket
+#   macos    ~/Library/Caches/build.bleep/socket
+#   windows  %LOCALAPPDATA%\bleep\cache\socket
+#
+# Globbing only the Linux one collected nothing anywhere else. That mattered less than it looks, because the
+# steps that called it existed only in the `build` job (ubuntu) — so the native-image matrix, i.e. every
+# Windows and macOS run, produced no telemetry at all. Windows is where the platform-specific failures live
+# and was the one arch we could not see into.
+#
+# The Windows root comes from `$LOCALAPPDATA` run through `cygpath`, not from `~/AppData/Local`. Two reasons,
+# both of which make the difference between collecting everything and collecting nothing:
+#
+#   - `$LOCALAPPDATA` is a native path (`C:\Users\runneradmin\AppData\Local`) and backslashes are escapes to
+#     the globber, so it has to be converted rather than interpolated. `build.yml`'s "Kill leftover JVMs"
+#     step already documents this trap; this is the same `cygpath` call.
+#   - `~` is not a safe substitute. GitHub runs every `shell: bash` block as `bash --noprofile --norc`, so
+#     none of Git Bash's startup files run and `$HOME` is whatever the runner exported — which is not
+#     guaranteed to be a POSIX path, and is not guaranteed to be the account whose LOCALAPPDATA bleep used.
+#
+# Every candidate root is echoed with what was found under it, so a run that collects nothing says which
+# paths it looked at instead of printing one unfalsifiable "nothing to summarise".
+#
+# Never fails the caller — a diagnostic that breaks the run it is diagnosing is worse than no diagnostic.
+set -uo pipefail
+shopt -s nullglob
+
+out=${1:-bsp-diagnostics}
+
+roots=(
+  "$HOME/.cache/bleep"               # linux (XDG)
+  "$HOME/Library/Caches/build.bleep" # macos
+)
+
+if [ -n "${LOCALAPPDATA:-}" ]; then
+  if command -v cygpath >/dev/null 2>&1; then
+    roots+=("$(cygpath "$LOCALAPPDATA")/bleep/cache")
+  else
+    # Only reachable if something other than Git Bash is running this on Windows. Say so rather than
+    # appending a backslash path that would silently match nothing.
+    echo "::warning::LOCALAPPDATA is set but cygpath is not on PATH — cannot resolve the Windows cache dir"
+  fi
+fi
+
+echo "compile-server socket dirs:"
+dirs=()
+for root in "${roots[@]}"; do
+  socket="$root/socket"
+  if [ -d "$socket" ]; then
+    here=("$socket"/*/)
+    echo "  $socket — ${#here[@]} server dir(s)"
+    dirs+=("${here[@]}")
+  else
+    echo "  $socket — absent"
+  fi
+done
+
+if [ ${#dirs[@]} -eq 0 ]; then
+  echo "no compile-server socket dirs under any of the above — nothing to summarise"
+  exit 0
+fi
+
+mkdir -p "$out"
+for d in "${dirs[@]}"; do
+  name=$(basename "$d")
+  mkdir -p "$out/$name"
+  for f in "$d"metrics.jsonl "$d"output "$d"output.1 "$d"output.2; do
+    [ -f "$f" ] || continue
+    cp "$f" "$out/$name/"
+    # Sizes in the log because an empty metrics.jsonl and a missing one are different failures, and the
+    # artifact listing alone does not distinguish them.
+    echo "  collected $name/$(basename "$f") ($(wc -c <"$f" | tr -d ' ') bytes)"
+  done
+done
+
+# The summary is a convenience so the common questions do not need an artifact download. The raw files are
+# already copied by this point, so a missing interpreter costs the summary, not the diagnostics.
+if command -v python3 >/dev/null 2>&1; then
+  python3 "$(dirname "$0")/summarise-bsp-metrics.py" "$out"
+else
+  echo "python3 not on PATH — skipping the inline summary; the uploaded artifact still has the raw metrics"
+fi

.github/scripts/summarise-bsp-metrics.py

@@ -75,6 +75,16 @@ def summarise(path: pathlib.Path, rows: list[dict]) -> None:
         idle = sum(1 for r in machine if r.get("used_cpu", 0) == 0 and r.get("waiting", 0) == 0)
         peak_wait = max((r.get("waiting", 0) for r in machine), default=0)
         print(f"\n  machine: {samples} samples at 15s")
+        # Recorded rather than derived: `total_memory_mb` is the *fork* budget (machine RAM minus the server's
+        # own footprint minus an OS reserve) and it is retuned as the run proceeds, so neither the machine's RAM
+        # nor the heap cap can be recovered from it. Older metrics.jsonl files predate these fields.
+        cores = machine[0].get("total_cpu")
+        phys = machine[0].get("physical_memory_mb")
+        heap = machine[0].get("server_heap_mb")
+        if phys is not None and heap is not None:
+            print(f"    machine: {cores} cores, {phys} MB RAM; server heap capped at {heap} MB")
+        budgets = [r.get("total_memory_mb", 0) for r in machine]
+        print(f"    fork budget: {min(budgets)}–{max(budgets)} MB over the run (retuned to what the machine can spare)")
         print(f"    saturated (all cpu in use):            {saturated:>4}  ({100 * saturated / samples:.0f}%)")
         print(f"    idle (nothing running, nothing queued): {idle:>4}  ({100 * idle / samples:.0f}%)")
         print(f"    STARVED (queue non-empty, cpu free):    {starved:>4}  ({100 * starved / samples:.0f}%)   <- admission refusing work it could take")

.github/workflows/build.yml

@@ -63,34 +63,30 @@ jobs:
       - name: Run tests
         env:
           CI: true
+        # Step cap, deliberately below the job's 30. A job that exceeds `timeout-minutes` is torn down with
+        # its remaining steps — including the `if: always()` ones — so a hang here would destroy exactly the
+        # telemetry that explains it. Failing the *step* instead leaves the collection below to run.
+        # Measured at 9m26s on master against ~3m30s of preceding steps, so 22 is >2x headroom and still
+        # lands inside 30 with room for the upload.
+        timeout-minutes: 22
         run: ./bleep-cli.sh --dev test
 
-      # Always, including on failure and timeout — a run that went wrong is the one whose telemetry
+      # Always, including on failure and step timeout — a run that went wrong is the one whose telemetry
       # is worth having. CI is also the only environment with a realistic shape: 4 cores and 16GB,
       # where memory bounds actually bind. A developer machine cannot reproduce that.
+      #
+      # Invoked as `bash <script>` rather than executed directly: the exec bit does not survive a Windows
+      # checkout, and this same step exists in the native-image matrix below.
       - name: Summarise compile-server telemetry
         if: always()
         shell: bash
-        run: |
-          set -uo pipefail
-          shopt -s nullglob
-          dirs=(~/.cache/bleep/socket/*/)
-          if [ ${#dirs[@]} -eq 0 ]; then echo "no compile-server socket dirs — nothing to summarise"; exit 0; fi
-          mkdir -p bsp-diagnostics
-          for d in "${dirs[@]}"; do
-            name=$(basename "$d")
-            mkdir -p "bsp-diagnostics/$name"
-            for f in "$d"metrics.jsonl "$d"output "$d"output.1 "$d"output.2; do
-              [ -f "$f" ] && cp "$f" "bsp-diagnostics/$name/"
-            done
-          done
-          python3 .github/scripts/summarise-bsp-metrics.py bsp-diagnostics
+        run: bash .github/scripts/collect-bsp-diagnostics.sh
 
       - name: Upload compile-server telemetry
         if: always()
         uses: actions/upload-artifact@v7
         with:
-          name: bsp-diagnostics
+          name: bsp-diagnostics-build
           path: bsp-diagnostics
           retention-days: 14
           if-no-files-found: ignore
@@ -125,7 +121,12 @@ jobs:
     runs-on: ${{ matrix.os }}
     # Bumped from 25 → 40: macos-latest (arm64) cancelled at 25:19 mid native-image build under the prior 25-min ceiling. Other arches finish in ~13-20min, but
     # mac arm needs the headroom for the native-image step itself before the test phase even starts.
-    timeout-minutes: 40
+    #
+    # Then 40 → 45: the test step below now has its own 20-minute cap, and this ceiling has to sit far
+    # enough above `slowest-preceding-steps + 20` that the telemetry collection at the end of the job still
+    # fits. macos-latest reaches the test step at ~17m, so 17 + 20 = 37 leaves 8 minutes for collection and
+    # upload. This is a backstop for a wedged runner, not the mechanism that catches a hanging test run.
+    timeout-minutes: 45
     # ============================================================================
     # !!! DO NOT ADD A `run_tests` FLAG OR ANY OTHER PER-ARCH TEST-SKIP MECHANISM !!!
     #
@@ -306,10 +307,38 @@ jobs:
         #
         # `selftest` reaches the two FFM surfaces in the binary unconditionally (kernel32 for the terminal, SHGetKnownFolderPath for the user directories).
         # Both need `foreign` reachability metadata; without it the native image aborts at runtime and only a real Windows user finds out (#601).
+        #
+        # Step cap, deliberately below the job's 45. Exceeding the *job* timeout tears the runner down with
+        # every remaining step, `if: always()` included — which is precisely what happened on windows-latest
+        # in this PR's own first run: the step hung for 36 minutes, the job was killed at its ceiling, and
+        # the two telemetry steps below never ran. The one arch the telemetry was written for produced
+        # nothing, for the one failure mode worth seeing. Failing the step instead keeps them.
+        #
+        # Measured healthy durations for this step: 4m45s ubuntu-arm, 6m31s ubuntu, 10m13s macos-arm,
+        # 11m31s macos-intel, 12m52s windows. 20 is >1.5x the slowest, and clears the job budget on the
+        # slowest arch (macos-arm reaches this step at ~17m).
+        timeout-minutes: 20
         run: |
           ./${{ matrix.file_name }} --dev test --no-color jvm3 --exclude-tag slow
           ./${{ matrix.file_name }} selftest
 
+      # Same telemetry as the `build` job. This matrix is the only place Windows and macOS run, so without
+      # it those arches were invisible: no metrics, no server log, nothing to read when a job hung or was
+      # killed by the job timeout.
+      - name: Summarise compile-server telemetry
+        if: always()
+        shell: bash
+        run: bash .github/scripts/collect-bsp-diagnostics.sh
+
+      - name: Upload compile-server telemetry
+        if: always()
+        uses: actions/upload-artifact@v7
+        with:
+          name: bsp-diagnostics-${{ matrix.os }}
+          path: bsp-diagnostics
+          retention-days: 14
+          if-no-files-found: ignore
+
       - name: Temporarily save package
         uses: actions/upload-artifact@v7
         with:

bleep-bsp-tests/src/scala/bleep/analysis/TestFiles.scala

@@ -2,7 +2,7 @@ package bleep.analysis
 
 import java.io.IOException
 import java.nio.file.attribute.BasicFileAttributes
-import java.nio.file.{DirectoryNotEmptyException, FileVisitResult, Files, Path, SimpleFileVisitor}
+import java.nio.file.{FileSystemException, FileVisitResult, Files, Path, SimpleFileVisitor}
 
 /** Recursively delete a file or directory tree. Top-level, so every test in `bleep.analysis` gets it without an import.
   *
@@ -15,24 +15,29 @@ import java.nio.file.{DirectoryNotEmptyException, FileVisitResult, Files, Path,
   *
   * ==Why it retries==
   *
-  * The tree may still be growing while we delete it. `Outcome.runInFreshThread` implements cancellation as `cancel()` + `Thread.interrupt()` and returns
+  * The tree is still in use while we delete it. `Outcome.runInFreshThread` implements cancellation as `cancel()` + `Thread.interrupt()` and returns
   * `ThreadOutcome.Cancelled` *without waiting for the worker to stop* — it parks the thread in `runawayThreads` instead, on the grounds that "most native
-  * compilers ignore interrupt and keep running". So a cancellation test that asserts `Cancelled` and then deletes its output directory is racing a kotlinc that
-  * is still emitting into it: the walk empties the directory, the writer refills it, and unlinking the directory then fails with `DirectoryNotEmptyException`.
-  * Seen on both windows-latest and ubuntu, which is what made these tests look flaky rather than broken.
+  * compilers ignore interrupt and keep running". So a cancellation test that asserts `Cancelled` and then deletes its output directory is racing a toolchain
+  * that is still using it. The two OSes fail differently, which is why fixing one did not fix the other:
   *
-  * Retrying the whole walk lets the runaway writer finish and the delete converge. It is bounded — if the tree will not settle, the exception is rethrown and
-  * the test fails rather than leaving cleanup silently half-done.
+  *   - ubuntu: the walk empties the directory, the runaway writer puts a file back, and unlinking the directory fails `DirectoryNotEmptyException`.
+  *   - windows: a live handle blocks the unlink outright — `FileSystemException: The process cannot access the file because it is being used by another
+  *     process`, seen on `scalajs-mid-cancel` in `TimeoutAndResourceTest`.
+  *
+  * Both are `FileSystemException`, so that is what we retry on rather than either leaf type. Retrying the whole walk lets the runaway work finish and the
+  * delete converge. It is bounded — if the tree will not settle the exception is rethrown and the test fails, rather than leaving cleanup silently half-done.
   */
 def deleteRecursively(path: Path): Unit = {
-  var remaining = 10
+  // 20 × 100ms. Generous enough for a killed node/kotlinc process to exit and drop its handles, short enough that a genuinely stuck tree still fails the test
+  // rather than hanging the suite.
+  var remaining = 20
   var deleted = false
   while (!deleted)
     try {
       deleteTreeOnce(path)
       deleted = true
     } catch {
-      case _: DirectoryNotEmptyException if remaining > 0 =>
+      case _: FileSystemException if remaining > 0 =>
         remaining -= 1
         Thread.sleep(100L)
     }

bleep-bsp/src/scala/bleep/bsp/BspMetrics.scala

@@ -237,18 +237,28 @@ object BspMetrics {
 
   /** Machine-governor state: what the daemon-wide resource governor is admitting and what is queued behind it. Emitted periodically by the daemon's reporter,
     * which is the only thing holding the governor.
+    *
+    * `total_memory_mb` is the *fork* budget, not the machine's RAM — [[bleep.MachineResources.forkMemoryBudgetMb]] subtracts the server's own footprint and an
+    * OS reserve, and [[bleep.MachineResources.retuneMemoryBudget]] then moves it up and down with what the machine can actually spare. Two consequences that
+    * repeatedly misled readers of this data: `used_memory_mb` can legitimately exceed it (retuning down while reservations are held), and the machine's RAM
+    * cannot be recovered from it by inverting the formula, because the value sampled is rarely the initial one.
+    *
+    * So `physical_memory_mb` and `server_heap_mb` are recorded outright. They are constant per process and mildly redundant on every 15s sample, which is the
+    * price of each sample being self-describing rather than something to reconstruct.
     */
   def recordMachine(
       usedCpu: Int,
       totalCpu: Int,
       usedMemoryMb: Long,
       totalMemoryMb: Long,
+      physicalMemoryMb: Long,
+      serverHeapMb: Long,
       activeCompiles: Int,
       running: Int,
       waiting: Int
   ): Unit =
     writeEvent(
-      s"""{"type":"machine","ts":${now()},"used_cpu":$usedCpu,"total_cpu":$totalCpu,"used_memory_mb":$usedMemoryMb,"total_memory_mb":$totalMemoryMb,"active_compiles":$activeCompiles,"running":$running,"waiting":$waiting}"""
+      s"""{"type":"machine","ts":${now()},"used_cpu":$usedCpu,"total_cpu":$totalCpu,"used_memory_mb":$usedMemoryMb,"total_memory_mb":$totalMemoryMb,"physical_memory_mb":$physicalMemoryMb,"server_heap_mb":$serverHeapMb,"active_compiles":$activeCompiles,"running":$running,"waiting":$waiting}"""
     )
 
   /** What the Zinc analysis cache is holding after each sweep. The largest single retainer in the server heap, so its size is the first number to look at when

bleep-bsp/src/scala/bleep/bsp/BspServerDaemon.scala

@@ -281,6 +281,11 @@ object BspServerDaemon {
     locally {
       import cats.effect.unsafe.implicits.global
       val reporter = new Thread("bleep-machine-reporter") {
+        // Constant for the life of the process, so read once rather than per sample. Both are recorded on every machine event anyway: without them the
+        // machine's RAM has to be inferred by inverting the fork-budget formula, which does not work once the budget has been retuned.
+        private val serverHeapMb: Long = Runtime.getRuntime.maxMemory() / (1024L * 1024L)
+        private val physicalMemoryMb: Long = bleep.MachineResources.physicalMemoryMb(fallbackMb = serverHeapMb * 4)
+
         override def run(): Unit =
           try
             while (!shutdownRequested.get()) {
@@ -292,6 +297,8 @@ object BspServerDaemon {
                 totalCpu = snapshot.totalCpu,
                 usedMemoryMb = snapshot.usedMemoryMb,
                 totalMemoryMb = snapshot.totalMemoryMb,
+                physicalMemoryMb = physicalMemoryMb,
+                serverHeapMb = serverHeapMb,
                 activeCompiles = snapshot.activeCompiles,
                 running = snapshot.active.size,
                 waiting = snapshot.waiting.size