docker: close five failure paths the review found

Notebook sync, in-place publish. entrypoint.sh runs sync_notebooks and then
execs the container command, so the detached refresh child is still copying
while JupyterLab serves the same tree. cp -a writes through the destination
inode, so a reader can catch half-written JSON and a save made after the
recorded-hash check is destroyed and then recorded as pristine. Publish through
a same-dir dot-prefixed temp plus an atomic rename, and re-read the hash once
the staging copy is complete (the earlier check sits before middle_unchanged, a
python subprocess, so the window was most of the loop). A single-file bind mount
cannot be renamed over, so that path falls back to the previous copy.

Notebook sync, first boot. A pre-existing file whose bytes already match the
baked template fell through to cp -a, which is --preserve=all: as root that
stamps root:root, the baked mode and the build mtime onto a bind-mounted host
file and locks its owner out of editing it. Record it as managed instead. The
hash is identical, so the state file is byte-for-byte what the copy wrote.

unsloth-studio-update. The post-update import check only warned, then the
default restart replaced a process that was serving fine with one known not to
import. supervisord retries startretries times, lands in FATAL and never leaves
it on its own, so the container serves nothing until someone execs in. Keep the
running service and exit non-zero with the remedy.

unsloth-llama-update --check. resolve_latest swallows every failure into an
empty string, which fell into the "up to date" branch and exited 0, so the
command reported a state it could not observe. Report UNKNOWN and fail.

unsloth-llama-update rollback. The in-place restore iterates the backup's
entries, so a file the new release introduced survives it and the restored tree
is mixed-version; ggml dlopens every libggml-*.so next to the binaries. Clear
the install dir before restoring, gated on the drain having completed, because
before that an entry there can still be the only copy of an old file.

docker-publish ref freeze. git ls-remote exits 0 whether or not a ref matched,
so a non-zero exit means the remote was never reached. That exit was lost twice
over: first element of a pipeline, and a run step with no explicit shell runs
under bash -e without pipefail. The step exited 0 and published ref=main, which
the amd64, arm64 and Studio builds each resolve again, so one multi-arch tag
could carry different revisions. Fail the prepare job instead, keeping the
passthrough for the reachable-but-no-match case it was written for.

Jupyter output select. lastPointerOutput was only replaced by another
pointer-down, but J/K/arrow cell navigation fires none, so Ctrl/Cmd+A on a later
cell selected the previously clicked output and suppressed notebook:select-all;
after a re-run the node is detached and the chord did nothing at all. Revalidate
the remembered output (still in the document, still in the active cell) before
using it as the fallback.

Tests: four static guards in test_docker_nb_sync_race.py, a new behavioural
test_docker_update_helpers.py driving both helpers with stub pip, supervisorctl
and mv, a new test_docker_publish_ref_freeze.py that executes each resolver step
under bash -e with a failing ls-remote, and a source check in
validate_studio_features.py. Each fails against the code before this change; the
interrupted-drain case also fails against the unconditional form of the rollback
fix.
This commit is contained in:
Daniel Han 2026-07-27 14:02:16 +00:00
commit 837b09122e
9 changed files with 558 additions and 13 deletions

View file

@ -0,0 +1,132 @@
# SPDX-License-Identifier: AGPL-3.0-only
# Copyright 2026-Present the Unsloth team. See /studio/LICENSE.AGPL-3.0
"""The docker publish workflow must never forward an unfrozen ref.
`prepare` resolves unsloth, unsloth-zoo and notebooks to ONE commit each so the
amd64 leg, the arm64 leg and the Studio build all bake identical source; that is
the whole reason the job exists. Each resolver was
SHA="$(git ls-remote <repo> "$REF" | awk 'NR==1{print $1}')"
[ -n "$SHA" ] || SHA="$REF"
`git ls-remote` exits 0 whether or not a ref matched, so a non-zero exit means
the remote was never reached. That exit was lost twice over: it is the first
element of a pipeline, and a `run:` step with no explicit `shell:` runs under
`bash -e` WITHOUT pipefail, so the step exited 0 and published `ref=main`. Each
build then resolved `main` independently, and a branch advance between them
would ship one multi-arch tag containing different revisions. The stable-tag
gates key off the inputs, not off whether resolution worked, so `:latest` would
still be moved onto it.
Static plus behavioural: the resolver `run:` blocks are executed under `bash -e`
with a `git` stub. No docker, no network.
"""
from __future__ import annotations
import os
import re
import shutil
import subprocess
from pathlib import Path
import pytest
import yaml
REPO_ROOT = Path(__file__).resolve().parents[2]
WORKFLOW = REPO_ROOT / ".github" / "workflows" / "docker-publish.yml"
RESOLVER_STEPS = ("unsloth_ref", "zoo_ref", "notebooks")
pytestmark = pytest.mark.skipif(
shutil.which("bash") is None, reason = "needs bash",
)
@pytest.fixture(scope = "module")
def steps() -> dict:
assert WORKFLOW.is_file(), f"missing {WORKFLOW}"
doc = yaml.safe_load(WORKFLOW.read_text(encoding = "utf-8"))
found = {}
for step in doc["jobs"]["prepare"]["steps"]:
if step.get("id") in RESOLVER_STEPS:
found[step["id"]] = step["run"]
missing = set(RESOLVER_STEPS) - set(found)
assert not missing, f"resolver steps missing from the prepare job: {missing}"
return found
def test_the_workflow_never_pins_a_shell_so_bash_e_has_no_pipefail(steps: dict):
# If someone later adds `shell: bash` the runner switches to
# `bash --noprofile --norc -eo pipefail`, which would make the guards below
# redundant rather than wrong -- but until then they are the only protection.
doc = yaml.safe_load(WORKFLOW.read_text(encoding = "utf-8"))
assert "shell" not in doc.get("defaults", {}).get("run", {}), (
"this test models the default `bash -e` shell; update it if a default "
"shell with pipefail is introduced"
)
@pytest.mark.parametrize("step_id", RESOLVER_STEPS)
def test_an_unreachable_remote_fails_the_step(steps: dict, step_id: str, tmp_path: Path):
script = _expand(steps[step_id])
res = _run_with_failing_ls_remote(script, tmp_path)
assert res.returncode != 0, (
"a transport failure must fail the prepare job, not fall through to the "
f"mutable ref:\nstdout={res.stdout}\nstderr={res.stderr}"
)
@pytest.mark.parametrize("step_id", RESOLVER_STEPS)
def test_an_unreachable_remote_never_emits_a_mutable_ref(
steps: dict, step_id: str, tmp_path: Path,
):
script = _expand(steps[step_id])
res = _run_with_failing_ls_remote(script, tmp_path)
emitted = (tmp_path / "github_output").read_text(encoding = "utf-8") \
if (tmp_path / "github_output").exists() else ""
for line in emitted.splitlines():
key, _, value = line.partition("=")
assert re.fullmatch(r"[0-9a-f]{40}", value), (
f"{step_id} published {key}={value!r}, which the three builds each "
"resolve again, so they can bake different revisions"
)
assert res.returncode != 0
def _expand(run: str) -> str:
"""Replace the `${{ ... }}` expressions with the empty string the default
(push to main, no dispatch inputs) trigger produces."""
return re.sub(r"\$\{\{[^}]*\}\}", "", run)
def _run_with_failing_ls_remote(script: str, tmp_path: Path):
bin_dir = tmp_path / "bin"
bin_dir.mkdir(parents = True, exist_ok = True)
stub = bin_dir / "git"
stub.write_text(
"#!/usr/bin/env bash\n"
'if [ "$1" = "ls-remote" ]; then\n'
' echo "fatal: unable to access: Could not resolve host" >&2\n'
" exit 128\n"
"fi\n"
"exit 0\n",
encoding = "utf-8",
)
stub.chmod(0o755)
out = tmp_path / "github_output"
out.write_text("", encoding = "utf-8")
env = dict(os.environ)
env["PATH"] = f"{bin_dir}{os.pathsep}" + env["PATH"]
env["GITHUB_OUTPUT"] = str(out)
# Whatever the expansions above blanked out; the resolvers default to "main".
for name in ("INPUT_REF", "TAG_REF", "PUSH_SHA"):
env[name] = ""
path = tmp_path / "step.sh"
path.write_text(script, encoding = "utf-8")
# Exactly how the runner invokes a `run:` step with no explicit `shell:`.
return subprocess.run(
["bash", "-e", str(path)],
capture_output = True, text = True, env = env, timeout = 60,
)