diff --git a/.github/scripts/interrupt-install.sh b/.github/scripts/interrupt-install.sh index e805b11d90..c6cfe9b0f8 100755 --- a/.github/scripts/interrupt-install.sh +++ b/.github/scripts/interrupt-install.sh @@ -86,14 +86,31 @@ if [ "$killed" = "true" ]; then kill -0 "$PID" 2>/dev/null || break sleep 1 done - if kill -0 "$PID" 2>/dev/null; then - echo "[interrupt] group survived SIGTERM; SIGKILL" - kill -KILL -- -"$PID" 2>/dev/null || kill -KILL "$PID" 2>/dev/null || true - fi + # Unconditional, and to the GROUP. The leader can exit on SIGTERM while a uv or + # python descendant ignores it or is mid-shutdown; `kill -0 "$PID"` then reported + # the leader gone, this escalation was skipped, and `wait` reaped only the leader, + # leaving that descendant free to finish the dependency pass while the probe ran. + # Signalling an already-empty group is a no-op. + echo "[interrupt] SIGKILL to process group -$PID" + kill -KILL -- -"$PID" 2>/dev/null || kill -KILL "$PID" 2>/dev/null || true fi wait "$PID" 2>/dev/null rc=$? + +# Only after the reap: an unreaped leader is still a member of its own group, so +# polling the group before `wait` would report it alive forever. Do not let the +# probe start while an installer process is still running. +if [ "$killed" = "true" ]; then + for _ in $(seq 1 "$KILL_GRACE"); do + kill -0 -- -"$PID" 2>/dev/null || break + kill -KILL -- -"$PID" 2>/dev/null || true + sleep 1 + done + if kill -0 -- -"$PID" 2>/dev/null; then + echo "::warning::processes from installer group -$PID outlived SIGKILL" + fi +fi echo "[interrupt] installer exit=$rc reason=$reason killed=$killed" echo "[interrupt] last log lines:" tail -15 "$LOG" || true diff --git a/.github/scripts/interrupted_install_probe.py b/.github/scripts/interrupted_install_probe.py index dd5a9c2254..e7a27c98b9 100644 --- a/.github/scripts/interrupted_install_probe.py +++ b/.github/scripts/interrupted_install_probe.py @@ -135,9 +135,16 @@ def main(argv: list[str]) -> int: popen_kw["start_new_session"] = True else: popen_kw["creationflags"] = getattr(subprocess, "CREATE_NEW_PROCESS_GROUP", 0) + # Straight to the artefact file, never a PIPE: nothing reads that pipe until after + # the polling loop, so a backend whose imports emit more than the OS pipe buffer + # (64 KiB on Linux and macOS, a single page by default on Windows) blocks on write + # BEFORE it binds the port. backend_ok, which this whole verdict pivots on, would + # then be false for a perfectly good install. + blog_path = out / "backend.log" + blog_fh = blog_path.open("w", encoding = "utf-8", errors = "replace") proc = subprocess.Popen( [binp, "studio", "--api-only", "-H", "127.0.0.1", "-p", str(port)], - stdout = subprocess.PIPE, + stdout = blog_fh, stderr = subprocess.STDOUT, text = True, **popen_kw, @@ -180,12 +187,8 @@ def main(argv: list[str]) -> int: proc.kill() reap() - try: - blog = proc.communicate(timeout = 30)[0] or "" - except subprocess.TimeoutExpired: - proc.kill() - blog = proc.communicate()[0] or "" - (out / "backend.log").write_text(blog, encoding = "utf-8", errors = "replace") + blog_fh.close() + blog = blog_path.read_text(encoding = "utf-8", errors = "replace") say("backend_ok", backend_ok) missing = "" diff --git a/.github/workflows/interrupted-install-ci.yml b/.github/workflows/interrupted-install-ci.yml index 319cf4ba4e..358141f456 100644 --- a/.github/workflows/interrupted-install-ci.yml +++ b/.github/workflows/interrupted-install-ci.yml @@ -37,6 +37,11 @@ on: # without a single leg running. - 'unsloth_cli/_studio_deps.py' - 'studio/install_manifest.py' + # The requirement files are the phases. studio.txt is where structlog is + # declared, the package whose absence IS the reported false-ready bug, and + # the single-env files drive the later steps, so moving a package between + # them changes what every interrupted state looks like. + - 'studio/backend/requirements/**' # `*` never matches `/`, and it is a literal `-install` that follows, so # `interrupt*-install*` matches interrupt-install.sh / .ps1 but NOT the # underscored probe. List the probe explicitly rather than rely on a glob.