fix(ci): close remaining uv sync diagnostic gaps (#8249)

scripts/dev/ci_uv_sync_diag.sh

@@ -13,31 +13,51 @@ set -uo pipefail
 label="${1:-uv-sync-diag}"
 
 # Bounded tree sizing (issue #8249): `du` walks scale with cache/venv size and
-# shared-host I/O contention (measured 28s for a 60GB cache against the 30s
-# test budget, so loaded hosts exceed it). Each probe below is capped so this
-# advisory diagnostic stays fast; on timeout it reports
+# shared-host I/O contention. With GNU timeout(1), each tree-size probe below
+# is capped so this advisory diagnostic stays fast; on timeout it reports
 # `unavailable-timed-out` instead of hanging the caller. Override the cap with
 # ROBOT_SF_DIAG_DU_TIMEOUT_SECONDS (default 10). Hosts without GNU timeout(1)
-# (e.g. macOS) keep the previous unbounded behavior.
+# (e.g. macOS) keep the previous direct-`du` fallback behavior.
 diag_du_timeout="${ROBOT_SF_DIAG_DU_TIMEOUT_SECONDS:-10}"
-if ! [[ "$diag_du_timeout" =~ ^[0-9]+$ ]]; then
+if ! [[ "$diag_du_timeout" =~ ^[1-9][0-9]*$ ]]; then
   diag_du_timeout=10
 fi
 # GNU timeout(1) is absent on some hosts (e.g. stock macOS bash 3.2 runners);
-# without it keep the previous unbounded behavior. The function form (rather
-# than an arg array) stays safe under `set -u` on old bash versions.
-du_timeout_secs=""
-if command -v timeout >/dev/null 2>&1; then
-  du_timeout_secs="$diag_du_timeout"
+# without it keep the previous direct-`du` behavior. Detect GNU explicitly so
+# a non-GNU command with the same name does not receive incompatible options.
+# The function form (rather than an arg array) stays safe under `set -u` on old
+# bash versions.
+du_timeout_bin=""
+if command -v timeout >/dev/null 2>&1 &&
+  timeout --version 2>/dev/null | grep -q "GNU coreutils"; then
+  du_timeout_bin="$(command -v timeout)"
 fi
+du_timeout_kill_after_secs=2
 bounded_du() {
-  if [[ -n "${du_timeout_secs:-}" ]]; then
-    timeout "$du_timeout_secs" "$@"
+  if [[ -n "${du_timeout_bin:-}" ]]; then
+    "$du_timeout_bin" --kill-after="${du_timeout_kill_after_secs}s" "$diag_du_timeout" "$@"
   else
     "$@"
   fi
 }
 
+du_timed_out() {
+  local rc="$1"
+  [[ -n "${du_timeout_bin:-}" && ( "$rc" -eq 124 || "$rc" -eq 137 ) ]]
+}
+
+report_du_failure() {
+  local prefix="$1"
+  local rc="$2"
+  if du_timed_out "$rc"; then
+    echo "  ${prefix}_sizing_status=timed-out"
+    echo "  ${prefix}_sizing_timeout_seconds=${diag_du_timeout}"
+  else
+    echo "  ${prefix}_sizing_status=error"
+  fi
+  echo "  ${prefix}_sizing_exit_code=${rc}"
+}
+
 echo "::group::${label}"
 
 echo "uv_sync_diag runner_info"
@@ -52,11 +72,6 @@ echo "uv_sync_diag uv_info"
 if command -v uv >/dev/null 2>&1; then
     echo "  uv_version=$(uv --version 2>/dev/null || echo unknown)"
     echo "  uv_cache_dir=$(uv cache dir 2>/dev/null || echo unknown)"
-    # Best-effort cache size report; older uv versions may not have 'uv cache size'.
-    uv_cache_size="$(uv cache size 2>/dev/null || true)"
-    if [[ -n "$uv_cache_size" ]]; then
-        echo "  uv_cache_size=${uv_cache_size}"
-    fi
 else
     echo "  uv_version=not_installed"
 fi
@@ -100,15 +115,14 @@ if [[ -d "$cache_dir" ]]; then
     # tree up to a dozen times and risking preflight timeouts on large caches.
     # The captured output is then parsed in a single awk pass (pure in-memory,
     # no further disk I/O), preserving the curated key names and ordering.
-    # The walk itself is capped by bounded_du (issue #8249); exit 124 means the
-    # probe timed out under contention, which is reported, not retried.
+    # The walk itself is capped by bounded_du (issue #8249). It is the sole
+    # cache-size traversal; `uv cache size` is intentionally not called because
+    # that full-cache operation cannot be deadline controlled here.
     cache_du=""
     cache_du_rc=0
     cache_du="$(bounded_du du -h -d 1 "$cache_dir" 2>/dev/null)" || cache_du_rc=$?
-    if [[ "$cache_du_rc" -eq 124 ]]; then
-        echo "  cache_total_size=unavailable-timed-out"
-        echo "  cache_sizing_note=du exceeded ${diag_du_timeout}s under host contention"
-    else
+    if [[ "$cache_du_rc" -eq 0 ]]; then
+        echo "  cache_sizing_status=ok"
         printf '%s\n' "$cache_du" | awk -F'\t' -v dir="$cache_dir" '
             { size[$2] = $1 }
             END {
@@ -120,6 +134,13 @@ if [[ -d "$cache_dir" ]]; then
                 }
             }
         '
+    else
+        report_du_failure "cache" "$cache_du_rc"
+        if du_timed_out "$cache_du_rc"; then
+            echo "  cache_total_size=unavailable-timed-out"
+        else
+            echo "  cache_total_size=unavailable-error"
+        fi
     fi
 else
     echo "  cache_dir=${cache_dir} (does not exist)"
@@ -129,10 +150,16 @@ echo "uv_sync_diag venv_info"
 if [[ -d .venv ]]; then
     venv_du_rc=0
     venv_du="$(bounded_du du -sh .venv 2>/dev/null)" || venv_du_rc=$?
-    if [[ "$venv_du_rc" -eq 124 ]]; then
-        echo "  venv_size=unavailable-timed-out"
-    else
+    if [[ "$venv_du_rc" -eq 0 ]]; then
+        echo "  venv_sizing_status=ok"
         printf '%s\n' "$venv_du" | awk '{print "  venv_size="$1}' || true
+    else
+        report_du_failure "venv" "$venv_du_rc"
+        if du_timed_out "$venv_du_rc"; then
+            echo "  venv_size=unavailable-timed-out"
+        else
+            echo "  venv_size=unavailable-error"
+        fi
     fi
     if [[ -x .venv/bin/python ]]; then
         echo "  python_version=$(.venv/bin/python --version 2>&1 || true)"

tests/dev/test_ci_uv_sync_diag.py

@@ -5,6 +5,7 @@
 import os
 import shutil
 import subprocess
+import time
 from pathlib import Path
 
 import pytest
@@ -118,6 +119,7 @@ def test_ci_uv_sync_diag_reports_runner_and_uv_state() -> None:
     assert "uv_sync_diag cache_size" in output
     assert "uv_sync_diag venv_info" in output
     assert "::endgroup::" in output
+    assert "uv_cache_size=" not in output
     if shutil.which("uv"):
         assert "uv_version=uv " in output
 
@@ -186,26 +188,50 @@ def test_ci_uv_sync_diag_cache_sizing_is_single_pass(tmp_path: Path) -> None:
 
 
 def test_ci_uv_sync_diag_du_timeout_reports_unavailable(tmp_path: Path) -> None:
-    """Slow `du` walks under host contention must not hang the probe (issue #8249).
+    """Slow `du` walks must not hang the probe (issue #8249).
 
-    A 60GB cache measured 28s for one `du` pass against the 30s test budget, so
-    loaded hosts exceed it. With a slow-`du` shim and a 1s probe budget, the
-    script must still exit 0 quickly with timed-out markers. Completion inside
-    the outer 30s budget is itself the boundedness proof.
+    With a TERM-resistant slow-`du` shim and a 1s probe budget, both probes must
+    be killed promptly after the configured deadline and the script must still
+    exit 0 with timed-out markers. The fake uv also proves that the diagnostic
+    no longer performs the unbounded ``uv cache size`` traversal.
     """
     timeout_bin = shutil.which("timeout")
     if timeout_bin is None:
         pytest.skip("GNU timeout(1) is required for the du-budget probe")
+    timeout_version = subprocess.run(
+        [timeout_bin, "--version"], capture_output=True, text=True, check=False, timeout=5
+    )
+    if "GNU coreutils" not in timeout_version.stdout:
+        pytest.skip("GNU timeout(1) is required for the du-budget probe")
     script = _script_path()
     bash_path = shutil.which("bash")
     assert bash_path, "bash is required for this test"
 
     fake_bin = tmp_path / "bin"
     fake_bin.mkdir()
+    du_calls = tmp_path / "du_calls.log"
     slow_du = fake_bin / "du"
-    slow_du.write_text("#!/usr/bin/env bash\nsleep 30\n", encoding="utf-8")
+    slow_du.write_text(
+        f'#!/usr/bin/env bash\nprintf "%s\\n" "$*" >> "{du_calls}"\ntrap \'\' TERM\nsleep 30\n',
+        encoding="utf-8",
+    )
     slow_du.chmod(0o755)
 
+    uv_calls = tmp_path / "uv_calls.log"
+    uv_shim = fake_bin / "uv"
+    uv_shim.write_text(
+        "#!/usr/bin/env bash\n"
+        f'printf "%s\\n" "$*" >> "{uv_calls}"\n'
+        'case "$*" in\n'
+        '  "--version") printf "uv 0.11.21\\n" ;;\n'
+        '  "cache dir") printf "%s\\n" "$UV_CACHE_DIR" ;;\n'
+        '  "cache size") sleep 30 ;;\n'
+        "  *) exit 99 ;;\n"
+        "esac\n",
+        encoding="utf-8",
+    )
+    uv_shim.chmod(0o755)
+
     cache_dir = tmp_path / "uv-cache"
     cache_dir.mkdir()
     (tmp_path / ".venv" / "bin").mkdir(parents=True)
@@ -215,21 +241,171 @@ def test_ci_uv_sync_diag_du_timeout_reports_unavailable(tmp_path: Path) -> None:
     env["UV_CACHE_DIR"] = str(cache_dir)
     env["ROBOT_SF_DIAG_DU_TIMEOUT_SECONDS"] = "1"
 
+    started = time.monotonic()
     result = subprocess.run(
         [bash_path, str(script), "du-timeout-test"],
         capture_output=True,
         text=True,
         check=False,
         env=env,
         cwd=tmp_path,  # .venv dir present here, so both du probes fire
-        timeout=30,
+        timeout=10,
     )
+    elapsed = time.monotonic() - started
     assert result.returncode == 0, f"stderr: {result.stderr}"
+    assert elapsed < 8, f"diagnostic took {elapsed:.2f}s instead of stopping near the 1s probes"
     assert "cache_total_size=unavailable-timed-out" in result.stdout
+    assert "cache_sizing_status=timed-out" in result.stdout
+    assert "cache_sizing_timeout_seconds=1" in result.stdout
     assert "venv_size=unavailable-timed-out" in result.stdout
+    assert "venv_sizing_status=timed-out" in result.stdout
+    assert "venv_sizing_timeout_seconds=1" in result.stdout
+    assert "under host contention" not in result.stdout
+    uv_invocations = uv_calls.read_text(encoding="utf-8").splitlines()
+    assert "cache size" not in uv_invocations
+    assert len(du_calls.read_text(encoding="utf-8").splitlines()) == 2
     assert "::endgroup::" in result.stdout
 
 
+def test_ci_uv_sync_diag_zero_timeout_uses_default(tmp_path: Path) -> None:
+    """A zero timeout must not disable the GNU timeout boundary."""
+    script = _script_path()
+    bash_path = shutil.which("bash")
+    assert bash_path, "bash is required for this test"
+
+    fake_bin = tmp_path / "bin"
+    fake_bin.mkdir()
+    timeout_calls = tmp_path / "timeout_calls.log"
+    fake_timeout = fake_bin / "timeout"
+    fake_timeout.write_text(
+        "#!/bin/bash\n"
+        'if [[ "${1:-}" == "--version" ]]; then\n'
+        '  printf "timeout (GNU coreutils) 9.1\\n"\n'
+        "  exit 0\n"
+        "fi\n"
+        f'printf "%s\\n" "$*" >> "{timeout_calls}"\n'
+        "exit 124\n",
+        encoding="utf-8",
+    )
+    fake_timeout.chmod(0o755)
+
+    cache_dir = tmp_path / "uv-cache"
+    cache_dir.mkdir()
+    env = os.environ.copy()
+    env["PATH"] = f"{fake_bin}{os.pathsep}{env['PATH']}"
+    env["UV_CACHE_DIR"] = str(cache_dir)
+    env["ROBOT_SF_DIAG_DU_TIMEOUT_SECONDS"] = "0"
+
+    result = subprocess.run(
+        [bash_path, str(script), "zero-timeout-test"],
+        capture_output=True,
+        text=True,
+        check=False,
+        env=env,
+        cwd=tmp_path,
+        timeout=5,
+    )
+    assert result.returncode == 0, f"stderr: {result.stderr}"
+    assert "cache_sizing_status=timed-out" in result.stdout
+    assert "cache_sizing_timeout_seconds=10" in result.stdout
+    assert "cache_total_size=unavailable-timed-out" in result.stdout
+    calls = timeout_calls.read_text(encoding="utf-8").splitlines()
+    assert len(calls) == 1
+    assert "--kill-after=2s 10 du -h -d 1" in calls[0]
+    assert "--kill-after=2s 0 " not in calls[0]
+
+
+def test_ci_uv_sync_diag_reports_du_errors(tmp_path: Path) -> None:
+    """Non-timeout `du` failures must remain visible in advisory output."""
+    script = _script_path()
+    bash_path = shutil.which("bash")
+    assert bash_path, "bash is required for this test"
+
+    fake_bin = tmp_path / "bin"
+    fake_bin.mkdir()
+    failing_du = fake_bin / "du"
+    failing_du.write_text("#!/bin/bash\nexit 125\n", encoding="utf-8")
+    failing_du.chmod(0o755)
+
+    cache_dir = tmp_path / "uv-cache"
+    cache_dir.mkdir()
+    (tmp_path / ".venv" / "bin").mkdir(parents=True)
+    env = os.environ.copy()
+    env["PATH"] = f"{fake_bin}{os.pathsep}{env['PATH']}"
+    env["UV_CACHE_DIR"] = str(cache_dir)
+
+    result = subprocess.run(
+        [bash_path, str(script), "du-error-test"],
+        capture_output=True,
+        text=True,
+        check=False,
+        env=env,
+        cwd=tmp_path,
+        timeout=10,
+    )
+    assert result.returncode == 0, f"stderr: {result.stderr}"
+    assert "cache_sizing_status=error" in result.stdout
+    assert "cache_sizing_exit_code=125" in result.stdout
+    assert "cache_total_size=unavailable-error" in result.stdout
+    assert "venv_sizing_status=error" in result.stdout
+    assert "venv_sizing_exit_code=125" in result.stdout
+    assert "venv_size=unavailable-error" in result.stdout
+
+
+def test_ci_uv_sync_diag_without_gnu_timeout_preserves_du_output(tmp_path: Path) -> None:
+    """The direct-`du` fallback must preserve ordinary size markers."""
+    script = _script_path()
+    bash_path = shutil.which("bash")
+    assert bash_path, "bash is required for this test"
+
+    fake_bin = tmp_path / "bin"
+    fake_bin.mkdir()
+    for command_name in ("awk", "date", "df", "nproc", "uptime"):
+        command_path = shutil.which(command_name)
+        if command_path:
+            (fake_bin / command_name).symlink_to(command_path)
+    assert (fake_bin / "awk").exists(), "awk is required for this fallback test"
+
+    du_calls = tmp_path / "du_calls.log"
+    du_shim = fake_bin / "du"
+    du_shim.write_text(
+        "#!/bin/bash\n"
+        f'printf "%s\\n" "$*" >> "{du_calls}"\n'
+        'target="${@: -1}"\n'
+        'if [[ "$target" == ".venv" ]]; then\n'
+        '  printf "4K\\t%s\\n" "$target"\n'
+        "else\n"
+        '  printf "8K\\t%s\\n" "$target"\n'
+        "fi\n",
+        encoding="utf-8",
+    )
+    du_shim.chmod(0o755)
+
+    cache_dir = tmp_path / "uv-cache"
+    cache_dir.mkdir()
+    (tmp_path / ".venv" / "bin").mkdir(parents=True)
+    env = _clean_diag_env(tmp_path)
+    env["PATH"] = str(fake_bin)
+
+    assert shutil.which("timeout", path=env["PATH"]) is None
+    result = subprocess.run(
+        [bash_path, str(script), "no-timeout-test"],
+        capture_output=True,
+        text=True,
+        check=False,
+        env=env,
+        cwd=tmp_path,
+        timeout=10,
+    )
+    assert result.returncode == 0, f"stderr: {result.stderr}"
+    assert "cache_sizing_status=ok" in result.stdout
+    assert "cache_total_size=8K" in result.stdout
+    assert "venv_sizing_status=ok" in result.stdout
+    assert "venv_size=4K" in result.stdout
+    assert "unavailable-timed-out" not in result.stdout
+    assert len(du_calls.read_text(encoding="utf-8").splitlines()) == 2
+
+
 def test_workflow_uv_cache_is_pruned_by_setup_uv() -> None:
     """CI must use setup-uv's pruned cache without a second unbounded payload cache.