From 3d7b6a72469ca6e28807daa9500f90488fb229a0 Mon Sep 17 00:00:00 2001 From: Ethan Ligon & Sue Coder Date: Wed, 9 Sep 2026 19:08:08 +0000 Subject: [PATCH 1/9] fix(slurm): stage the timer atomically, never start it silently, clear the legacy warn file Three defects found reviewing the timer work, two of them in the new confined path. *Staging truncated a script a running job was still executing.* Both launch paths wrote the timer with `cat >` straight onto its final path. A second launch that computes the same path -- one mirror on two targets sharing $HOME (the reuse-probe is keyed per target), or two mirror names that sanitize alike -- truncated the file under the first job's bash, which reads its script from an open fd at a byte offset and therefore just stops, mid-script, with no diagnostic anywhere. Verified: the running script printed its first line and never reached its tail. Both paths now write a temp file and `mv -f` it into place, so a running job keeps reading the intact inode it already holds. The salloc path additionally staged every mirror to one shared `slurm-timer.sh`, so two concurrent unconfined mirrors overwrote each other's script outright; it now uses the same per-mirror name the confined path and the state files already use. *A timer that failed to start was invisible.* The batch body ran `nohup > /dev/null 2>&1 &` with no check, and nohup's own failure went to /dev/null while the outer rc stayed 0 -- so a noexec $HOME or a partially staged script produced a job with no deadline watchdog and an empty job log, i.e. exactly the bug this timer was added to fix. Both paths now guard on `-x` and report; the batch body leaves nohup's stderr on the job log so an exec failure surfaces too. *The legacy warn file was never cleared.* Making $WARN_FILE per mirror left the un-suffixed `slurm-deadline.warn` written but no longer part of the startup `rm -f`, so a previous job's "allocation may have ended" survived on the legacy path into a healthy new session -- read by exactly the older prompts that path is kept for. Tests: the three shape assertions the changes invalidated are updated rather than re-baselined, and each new assertion was checked by introducing the bug it targets. Also closes two gaps found by mutating the suite: rendering both bash helpers as empty strings shipped a script that never warns and never snapshots while the suite stayed green (bash -n does not flag a call to an undefined function), and both `scancel` guards used `startswith("scancel")`, which misses `then scancel ...`, `&& /usr/bin/scancel ...`, `$(scancel ...)` and `timeout 5 scancel ...` -- every plausible way it would come back. 683 passed; mypy unchanged (57 pre-existing). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018Ty45bJv1jADvUbyLaW4RD --- sucoder/cli.py | 22 ++++++++++--- sucoder/mirror.py | 27 ++++++++++++++-- sucoder/slurm_timer.py | 9 ++++-- tests/test_batch_script.py | 15 +++++++-- tests/test_cli.py | 55 ++++++++++++++++++++++++++++++-- tests/test_mirror.py | 14 ++++++-- tests/test_slurm_timer_script.py | 47 +++++++++++++++++++++++++-- 7 files changed, 169 insertions(+), 20 deletions(-) diff --git a/sucoder/cli.py b/sucoder/cli.py index 3aef78b..eea5e32 100644 --- a/sucoder/cli.py +++ b/sucoder/cli.py @@ -1195,12 +1195,19 @@ def _start_slurm_timer( node = session.compute_node q_script = shlex.quote(script_name) + # Temp file + atomic rename, never ``cat >`` onto the live path. The + # pkill below retires the previous timer only *after* this write, so a + # relaunch would otherwise truncate a script the old timer's bash is + # still reading -- which does not restart it, it stops it at whatever + # byte offset it had reached. ``mv -f`` swaps the directory entry, + # leaving the running process's open inode intact. write_result = _sp.run( ["ssh", *ssh_opts, node, - 'mkdir -p "$HOME/.cache/sucoder" && ' - 'chmod 700 "$HOME/.cache/sucoder" 2>/dev/null || true; ' - f'cat > "$HOME/.cache/sucoder/"{q_script} && ' - f'chmod 700 "$HOME/.cache/sucoder/"{q_script}'], + 'd="$HOME/.cache/sucoder"; ' + 'mkdir -p "$d" && chmod 700 "$d" 2>/dev/null || true; ' + f't="$d/"{q_script}".tmp.$$"; ' + 'umask 077 && cat > "$t" && chmod 700 "$t" && ' + f'mv -f "$t" "$d/"{q_script}'], input=timer_script, capture_output=True, text=True, check=False, ) if write_result.returncode != 0: @@ -1213,10 +1220,15 @@ def _start_slurm_timer( # they pile up, each snapshotting. The [s] bracket keeps pkill from # matching the shell that runs it. q_pattern = shlex.quote(f"[s]lurm-timer-{token}.sh") + # ``nohup ... &`` returns 0 whether or not the script actually started, + # so the rc below only proves ssh worked; ``-x`` is what catches a + # missing or non-executable timer. run_result = _sp.run( ["ssh", *ssh_opts, node, f'pkill -u "$USER" -f {q_pattern} 2>/dev/null; ' - f'nohup "$HOME/.cache/sucoder/"{q_script} > /dev/null 2>&1 &'], + f'p="$HOME/.cache/sucoder/"{q_script}; ' + '[ -x "$p" ] || { echo "timer not startable: $p" >&2; exit 1; }; ' + 'nohup "$p" > /dev/null 2>&1 &'], capture_output=True, text=True, check=False, ) if run_result.returncode == 0: diff --git a/sucoder/mirror.py b/sucoder/mirror.py index ccf8c9f..8adde84 100644 --- a/sucoder/mirror.py +++ b/sucoder/mirror.py @@ -2409,7 +2409,18 @@ def _build_batch_script( " exit 1\n" "fi\n" + ( - f"nohup {shlex.quote(timer_path)} > /dev/null 2>&1 &\n" + # A timer that fails to start reintroduces exactly the + # bug it exists to fix -- a job with no deadline + # watchdog -- so this must never be silent. ``-x`` + # catches a missing or non-executable script, and + # nohup's stderr is left attached to the job log so an + # exec failure on a ``noexec`` $HOME is visible too. + f"if [ -x {shlex.quote(timer_path)} ]; then\n" + f" nohup {shlex.quote(timer_path)} > /dev/null &\n" + "else\n" + " echo \"SUCODER: deadline timer not startable:\" " + f"{shlex.quote(timer_path)} >&2\n" + "fi\n" if timer_path else "" ) + f"while tmux -L {q_sock} has-session -t {q_sess} 2>/dev/null; do\n" @@ -2859,11 +2870,21 @@ def _launch_confined( snapshot_dir=mirror_path, snapshot_minutes=slurm.wip_snapshot_minutes, ) + # Staged via a temp file and an atomic rename, never ``cat >`` + # onto the live path. A second launch computing the same + # timer_path (one mirror on two targets sharing $HOME -- the + # reuse-probe is keyed per target -- or two names that sanitize + # alike) would otherwise truncate a script the running job is + # still executing, and bash, reading from its open fd at a byte + # offset, silently stops: the first job loses its watchdog with + # no diagnostic anywhere. ``mv -f`` swaps the directory entry, + # so that job keeps reading the intact inode it already holds. self.executor.run_agent( [ "sh", "-c", - f"umask 077 && cat > {shlex.quote(timer_path)} " - f"&& chmod 700 {shlex.quote(timer_path)}", + 'umask 077 && t="$1.tmp.$$" && cat > "$t" ' + '&& chmod 700 "$t" && mv -f "$t" "$1"', + "sucoder-slurm-timer", timer_path, ], input=timer_script, check=True, capture_output=True, ) diff --git a/sucoder/slurm_timer.py b/sucoder/slurm_timer.py index 4d50ea4..6e37ff5 100644 --- a/sucoder/slurm_timer.py +++ b/sucoder/slurm_timer.py @@ -93,7 +93,12 @@ # State files are per mirror: several confined mirrors share one $HOME, # and a second timer's startup ``rm -f`` must not clear the first's # markers. The un-suffixed ``slurm-deadline.warn`` is still written for -# prompts that poll the legacy path. +# prompts that poll the legacy path; it is cleared at startup like the +# rest, or a warning from a previous job ("allocation may have ended") +# survives into a healthy new session. It is deliberately NOT per +# mirror -- that is what the legacy path means -- so with several +# confined mirrors it is last-writer-wins; the suffixed file is the +# one to poll. _TEMPLATE = r'''#!/bin/bash # sucoder SLURM deadline timer + WIP snapshotter (generated; do not edit). set -u @@ -111,7 +116,7 @@ WARN5="$STATE_DIR/.slurm-warn-5-$MIRROR_TOKEN" WARN15="$STATE_DIR/.slurm-warn-15-$MIRROR_TOKEN" WARN30="$STATE_DIR/.slurm-warn-30-$MIRROR_TOKEN" -rm -f "$WARN5" "$WARN15" "$WARN30" "$WARN_FILE" +rm -f "$WARN5" "$WARN15" "$WARN30" "$WARN_FILE" "$LEGACY_WARN_FILE" if [ -z "$JOB" ]; then echo "sucoder timer: no SLURM job id (not inside a job?); exiting." > "$WARN_FILE" diff --git a/tests/test_batch_script.py b/tests/test_batch_script.py index d9f0d5d..07e9c8e 100644 --- a/tests/test_batch_script.py +++ b/tests/test_batch_script.py @@ -123,11 +123,18 @@ def test_timer_started_after_session_check_before_keeper(): s = MirrorManager._build_batch_script( **_BASE, timer_path="/global/home/users/ligon/.cache/sucoder/slurm-timer-K-Aggregators.sh", ) - nohup = "nohup /global/home/users/ligon/.cache/sucoder/slurm-timer-K-Aggregators.sh > /dev/null 2>&1 &\n" + nohup = "nohup /global/home/users/ligon/.cache/sucoder/slurm-timer-K-Aggregators.sh > /dev/null &\n" assert nohup in s rc_check = s.index("SUCODER: tmux new-session failed") keeper = s.index("while tmux -L sucoder-K-Aggregators has-session") assert rc_check < s.index(nohup) < keeper + # The start is guarded and reports failure: a timer that never runs + # is the bug this whole script exists to fix, so it must not be + # silent. stderr stays on the job log (no ``2>&1``) so an exec + # failure on a noexec $HOME is visible. + assert f"if [ -x /global/home/users/ligon/.cache/sucoder/slurm-timer-K-Aggregators.sh ]; then" in s + assert "SUCODER: deadline timer not startable:" in s + assert "> /dev/null 2>&1 &" not in s def test_timer_omitted_when_no_path(): @@ -137,7 +144,11 @@ def test_timer_omitted_when_no_path(): @_bash_only def test_timer_path_is_quoted_and_script_parses(tmp_path): s = MirrorManager._build_batch_script(**_BASE, timer_path="/p q/t.sh") - assert "nohup '/p q/t.sh' > /dev/null 2>&1 &" in s + assert "nohup '/p q/t.sh' > /dev/null &" in s + assert "if [ -x '/p q/t.sh' ]; then" in s + # The diagnostic carries the path quoted too, or a path with a + # space would split into two words in the message. + assert "not startable:\" '/p q/t.sh' >&2" in s assert _bash_n(s).returncode == 0 diff --git a/tests/test_cli.py b/tests/test_cli.py index a6e438c..9a17f81 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -840,8 +840,11 @@ def capture(**kw): for raw in rendered[0].splitlines(): stripped = raw.strip() # Strings may *mention* scancel ("Run `scancel N` to free ..."), - # but no line may execute it. - assert not stripped.startswith("scancel"), ( + # but no line may execute it. Stripping quoted strings and + # comments leaves only shell code; ``startswith`` missed every + # realistic reintroduction (``then scancel``, ``$(scancel ...)``). + code = re.sub(r"'[^']*'|\"[^\"]*\"", "", stripped).split("#", 1)[0] + assert "scancel" not in code, ( "the deadline timer still emits a `scancel` shell command: " f"{stripped!r}. The user owns the SLURM lifecycle; use " "`sucoder release` for explicit cancel." @@ -849,6 +852,46 @@ def capture(**kw): assert "JOB=7\n" in rendered[0] +def test_salloc_timer_is_per_mirror_and_staged_atomically(monkeypatch): + """The unconfined timer gets a per-mirror filename and an atomic + rename, matching the confined path. + + A single shared ``slurm-timer.sh`` written with ``cat >`` had two + failure modes: two concurrent mirrors overwrote each other's script, + and a relaunch truncated a file the previous job's bash was still + reading -- which does not restart it, it silently stops it at + whatever byte offset it had reached. ``mv -f`` swaps the directory + entry instead, leaving the running job's open inode intact. + """ + calls = [] + + def record(argv, *a, **kw): + calls.append(argv) + return subprocess.CompletedProcess(argv, 0, "", "") + + monkeypatch.setattr(subprocess, "run", record) + session = SimpleNamespace(slurm_job_id=7, mirror_name="K Agg", compute_node="n0") + control = SimpleNamespace(ssh_options=lambda **kw: []) + cli._start_slurm_timer(session, control, control, mock.Mock()) + + write, start = calls[0][-1], calls[1][-1] + # Sanitized, per mirror -- never the old shared name. + assert "slurm-timer-K_Agg.sh" in write + assert "slurm-timer-K_Agg.sh" in start + for cmd in (write, start): + assert '"$HOME/.cache/sucoder/slurm-timer.sh"' not in cmd + # Staged to a temp file, chmod'd, then renamed over the destination. + assert '.tmp.$$' in write + assert 'cat > "$t"' in write + assert 'chmod 700 "$t"' in write + assert 'mv -f "$t"' in write + assert write.index('cat > "$t"') < write.index('mv -f "$t"') + # Starting it is guarded: ``nohup ... &`` exits 0 even when the script + # is missing, so the ssh rc alone proves nothing. + assert '[ -x "$p" ]' in start + assert "timer not startable" in start + + def _slurm_config(tmp_path: Path, *, with_session_jobid: bool = False) -> Path: """Write a config with a SLURM-backed target and (optionally) a saved RemoteSession for the sample mirror.""" @@ -2327,7 +2370,13 @@ def fake_run(argv, *a, **k): write_cmd, start_cmd = ssh_cmds[-2][-1], ssh_cmds[-1][-1] assert "slurm-timer-sample.sh" in write_cmd assert "pkill -u \"$USER\" -f '[s]lurm-timer-sample.sh'" in start_cmd - assert 'nohup "$HOME/.cache/sucoder/"slurm-timer-sample.sh' in start_cmd + # The path goes via $p so it can be guarded before starting: ``nohup + # ... &`` exits 0 even when the script is missing, so the ssh rc alone + # would not notice a timer that never ran. + assert 'p="$HOME/.cache/sucoder/"slurm-timer-sample.sh' in start_cmd + assert '[ -x "$p" ]' in start_cmd + assert 'nohup "$p" > /dev/null 2>&1 &' in start_cmd + assert start_cmd.index("pkill") < start_cmd.index("nohup") def test_build_executor_confined_no_local_disk_override_wins(tmp_path, monkeypatch): diff --git a/tests/test_mirror.py b/tests/test_mirror.py index 09806f1..0919493 100644 --- a/tests/test_mirror.py +++ b/tests/test_mirror.py @@ -1720,11 +1720,19 @@ def test_launch_confined_stages_and_starts_deadline_timer(tmp_path, monkeypatch) writes = [c for c in calls if c["args"][0] == "sh" and c["input"]] assert len(writes) == 2, "batch script then timer script must both be staged" batch, timer = writes[0], writes[1] - timer_path = [t for t in timer["args"][2].split() if "slurm-timer-" in t][0] + # The destination is passed as an argv positional, and the staging + # script writes a temp file and renames it into place. ``cat >`` + # straight onto the live path would truncate a script a running + # job is still reading, silently killing that job's watchdog. + timer_path = timer["args"][4] assert timer_path.endswith("/.cache/sucoder/slurm-timer-sample.sh") - assert "chmod 700" in timer["args"][2] + stage = timer["args"][2] + assert "chmod 700" in stage + assert 'mv -f "$t" "$1"' in stage + assert "cat > \"$t\"" in stage + assert timer_path not in stage, "destination must not be interpolated" # The batch body starts exactly that file, after the session check. - assert f"nohup {timer_path} > /dev/null 2>&1 &" in batch["input"] + assert f"nohup {timer_path} > /dev/null &" in batch["input"] # Confined specifics threaded through: runtime job id, dedicated socket, # the mirror as snapshot dir, the configured cadence. assert 'JOB="${SLURM_JOB_ID:-}"' in timer["input"] diff --git a/tests/test_slurm_timer_script.py b/tests/test_slurm_timer_script.py index ae5ba45..c6cf0e4 100644 --- a/tests/test_slurm_timer_script.py +++ b/tests/test_slurm_timer_script.py @@ -8,6 +8,7 @@ from __future__ import annotations import os +import re import shutil import subprocess from pathlib import Path @@ -35,7 +36,6 @@ def _bash_n(script: str, tmp_path: Path) -> subprocess.CompletedProcess: # -- rendering ---------------------------------------------------------------- def test_no_unresolved_tokens_in_either_mode(): - import re for script in (_render(job_id=123), _render(tmux_socket="s", snapshot_dir="/d")): assert not re.search(r"@[A-Z_]+@", script), script @@ -64,6 +64,20 @@ def test_confined_mode_reads_job_id_at_runtime_and_threads_socket(): assert s.count('"${TMUX_BIN[@]}"') >= 4 +def test_both_bash_helpers_reach_the_rendered_script(): + """The one thing ``build_timer_script`` uniquely does is assemble the + two helpers into the script. Tested elsewhere only as standalone + constants, so rendering them as empty strings left the suite green + while shipping a script that warns never and snapshots never -- + ``bash -n`` does not flag a call to an undefined function.""" + for script in (_render(job_id=1), _render(tmux_socket="s", snapshot_dir="/d")): + assert "left_to_mins() {" in script + assert "snapshot_wip() {" in script + # ...and they are defined before the loop that calls them. + assert script.index("left_to_mins() {") < script.index("mins=$(left_to_mins") + assert script.index("snapshot_wip() {") < script.index(" snapshot_wip\n") + + def test_state_files_are_per_mirror_and_legacy_warn_kept(): s = _render(mirror_token="alpha") assert 'WARN_FILE="$STATE_DIR/slurm-deadline-$MIRROR_TOKEN.warn"' in s @@ -73,6 +87,20 @@ def test_state_files_are_per_mirror_and_legacy_warn_kept(): assert f'WARN{n}="$STATE_DIR/.slurm-warn-{n}-$MIRROR_TOKEN"' in s +def test_startup_clears_the_legacy_warn_file_too(): + """Startup clears every warn file it may later write, the legacy one + included. Clearing only the per-mirror file let a previous job's + \"allocation may have ended\" survive on the legacy path into a + healthy new session -- read by exactly the older prompts that path + is kept for.""" + s = _render(mirror_token="alpha") + rm = next(ln for ln in s.splitlines() if ln.startswith("rm -f ")) + for var in ("$WARN5", "$WARN15", "$WARN30", "$WARN_FILE", "$LEGACY_WARN_FILE"): + assert f'"{var}"' in rm, f"{var} not cleared at startup: {rm}" + # Cleared before any warning could be written. + assert s.index(rm) < s.index("warn() {") + + def test_user_values_are_shell_quoted(): s = _render(mirror_token="x y", tmux_session="s;rm -rf /", tmux_socket="a b", snapshot_dir="/p q") @@ -94,12 +122,27 @@ def test_negative_snapshot_minutes_rejected(): _render(snapshot_minutes=-1) +def _runs_scancel(line: str) -> bool: + """True if ``line`` would *execute* scancel. + + The timer legitimately names scancel inside warning messages ("Run + 'scancel 7' to free the allocation"), so a guard cannot just look for + the word. Stripping quoted strings and comments leaves only shell + code, and any scancel surviving that is a real command -- which + ``startswith("scancel")`` was not: it missed ``then scancel ...``, + ``&& /usr/bin/scancel ...``, ``$(scancel ...)`` and ``timeout 5 + scancel ...``, i.e. every plausible way it would come back. + """ + code = re.sub(r"'[^']*'|\"[^\"]*\"", "", line).split("#", 1)[0] + return "scancel" in code + + def test_never_emits_a_bare_scancel(): """The user owns the SLURM lifecycle (``sucoder release``); the timer may *mention* scancel in a warning string but never run it.""" for script in (_render(job_id=1), _render(tmux_socket="s")): for raw in script.splitlines(): - assert not raw.strip().startswith("scancel"), raw + assert not _runs_scancel(raw), raw @_bash From fb0beb092836f43e45de0e1fef3f2b63370e812b Mon Sep 17 00:00:00 2001 From: Ethan Ligon & Sue Coder Date: Wed, 9 Sep 2026 19:09:50 +0000 Subject: [PATCH 2/9] fix(slurm): a fired deadline threshold marks the coarser ones spent The 30/15/5-minute chain fires the highest *unfired* threshold and marked only that one, so once a lower threshold tripped first the next poll fell through to a *less* urgent branch. A job starting with 3 minutes left produced: SLURM: ~3 min left (job 42). Commit and save NOW. SLURM: ~2 min left (job 42). Start wrapping up. SLURM: ~1 min left (job 42). Urgency running backwards as the deadline approached, and since this PR made each warning also snapshot, three full snapshot cycles -- a `git add -A` over the whole tree plus a push -- in three consecutive minutes, the cost the module docstring warns about. (An earlier version of this message said the sweep was "on NFS". It is not: the shared mirror is Lustre, and since the local-disk tiering work the sweep runs against the node-local clone, so it is the push, not the sweep, that reaches shared storage.) Any skipped poll did the same (31 -> 14 minutes fired the 15-minute warning, then the 30-minute notice a minute later). This is inherited from the pre-refactor inline script, where it only reached salloc sessions; it matters now because confined jobs -- which had no watchdog at all before this branch -- run the same chain, and short debug allocations and re-attaching to a nearly-expired job both start inside a threshold. Fix: firing a threshold touches every coarser marker too, so the chain can only escalate. The three tests added with it are the first to drive the monitoring loop rather than assert on the rendered string: they stub squeue/tmux/sleep on PATH and read back the messages a human would have seen. All three fail against the previous chain (verified before the fix went in), covering a normal countdown, a job that starts inside a threshold, and a skipped poll. 686 passed; mypy unchanged (57 pre-existing). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018Ty45bJv1jADvUbyLaW4RD --- sucoder/slurm_timer.py | 10 +++- tests/test_slurm_timer_script.py | 90 ++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/sucoder/slurm_timer.py b/sucoder/slurm_timer.py index 6e37ff5..ad0b751 100644 --- a/sucoder/slurm_timer.py +++ b/sucoder/slurm_timer.py @@ -169,14 +169,20 @@ break fi + # A threshold that fires also marks every COARSER one spent. Marking + # only the one that fired let the chain fall through to a less urgent + # branch on the next poll, so a job that started with 3 minutes left + # warned "Commit and save NOW", then "Start wrapping up", then the + # bare 30-minute notice -- urgency running backwards, one `git add -A` + # sweep per spurious warning. Same for any skipped poll (31 -> 14). mins=$(left_to_mins "$left") if [ "$mins" -le 5 ] && [ ! -f "$WARN5" ]; then warn "SLURM: ~${mins} min left (job $JOB). Commit and save NOW." - touch "$WARN5" + touch "$WARN5" "$WARN15" "$WARN30" snapshot_wip elif [ "$mins" -le 15 ] && [ ! -f "$WARN15" ]; then warn "SLURM: ~${mins} min left (job $JOB). Start wrapping up." - touch "$WARN15" + touch "$WARN15" "$WARN30" snapshot_wip elif [ "$mins" -le 30 ] && [ ! -f "$WARN30" ]; then warn "SLURM: ~${mins} min left (job $JOB)." diff --git a/tests/test_slurm_timer_script.py b/tests/test_slurm_timer_script.py index c6cf0e4..bd74d94 100644 --- a/tests/test_slurm_timer_script.py +++ b/tests/test_slurm_timer_script.py @@ -247,3 +247,93 @@ def test_snapshot_without_origin_is_a_noop(tmp_path): def test_snapshot_missing_or_non_git_dir_is_a_noop(tmp_path): assert _snapshot(tmp_path / "nope").returncode == 0 assert _snapshot(tmp_path).returncode == 0 + + +# -- the warning threshold chain, driven under bash --------------------------- + +def _drive(tmp_path: Path, time_left: list, **render) -> list: + """Run the rendered timer against stubbed squeue/tmux/sleep. + + ``time_left`` is fed to successive ``squeue -o %L`` polls; once it is + exhausted the job reads as gone and the loop ends. Returns the + messages the human would have seen, in order. + """ + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + counter, log = tmp_path / "n", tmp_path / "msgs" + (bin_dir / "squeue").write_text( + '#!/bin/bash\n' + f'n=$(cat {counter} 2>/dev/null || echo 0); n=$((n+1)); echo $n > {counter}\n' + 'vals=(' + " ".join(f'"{v}"' for v in time_left) + ')\n' + 'if [ "$n" -le "${#vals[@]}" ]; then echo "${vals[$((n-1))]}"; fi\n' + ) + # has-session always succeeds; only display-message is recorded. + (bin_dir / "tmux").write_text( + '#!/bin/bash\n' + 'case "$1" in\n' + f' display-message) echo "${{@: -1}}" >> {log} ;;\n' + ' has-session) exit 0 ;;\n' + 'esac\n' + 'exit 0\n' + ) + (bin_dir / "sleep").write_text("#!/bin/bash\nexit 0\n") + for f in bin_dir.iterdir(): + f.chmod(0o755) + + script = tmp_path / "timer.sh" + script.write_text(_render(job_id=42, **render)) + env = dict(os.environ, PATH=f"{bin_dir}:{os.environ['PATH']}", + HOME=str(tmp_path / "home")) + (tmp_path / "home").mkdir() + subprocess.run(["bash", str(script)], env=env, timeout=60, + capture_output=True, text=True) + if not log.exists(): + return [] + return [ln for ln in log.read_text().splitlines() if ln.strip()] + + +@_bash +def test_warnings_escalate_and_never_repeat(tmp_path): + """Each threshold fires once, in increasing urgency, and a job that + starts inside a threshold does not walk back down the ladder. + + The chain fires the highest *unfired* threshold, so marking only the + one that fired let later polls fall through to the *less* urgent + branches: a job with 3 minutes left warned "Commit and save NOW", + then "Start wrapping up" at 2 minutes, then the bare 30-minute + notice at 1 minute -- urgency running backwards as the deadline + approached, with a ``git add -A`` sweep behind each spurious + warning. A threshold firing must therefore also mark every coarser + one as spent. + """ + # A long job passes each threshold in turn: one warning each, escalating. + msgs = _drive(tmp_path, ["2:00:00", "40:00", "25:00", "12:00", "4:00"]) + deadline = [m for m in msgs if "min left" in m] + assert len(deadline) == 3, deadline + assert "Start wrapping up" not in deadline[0] + assert "Commit and save NOW" not in deadline[0] + assert "Start wrapping up" in deadline[1] + assert "Commit and save NOW" in deadline[2] + + +@_bash +def test_short_job_warns_once_at_its_true_urgency(tmp_path): + """A job that starts with 3 minutes left gets the 5-minute warning and + nothing else -- not a de-escalating sequence down to the 30-minute + notice.""" + msgs = _drive(tmp_path, ["3:00", "2:00", "1:00"]) + deadline = [m for m in msgs if "min left" in m] + assert len(deadline) == 1, f"expected one warning, got {deadline}" + assert "Commit and save NOW" in deadline[0] + + +@_bash +def test_skipped_poll_does_not_walk_back_down(tmp_path): + """A poll gap that jumps 31 -> 14 minutes fires the 15-minute warning, + then escalates to the 5-minute one; it must not emit the 30-minute + notice afterwards.""" + msgs = _drive(tmp_path, ["31:00", "14:00", "13:00", "4:00"]) + deadline = [m for m in msgs if "min left" in m] + assert len(deadline) == 2, deadline + assert "Start wrapping up" in deadline[0] + assert "Commit and save NOW" in deadline[1] From 550c23b0ed9b9440b051cb925431302631510068 Mon Sep 17 00:00:00 2001 From: Ethan Ligon & Sue Coder Date: Wed, 9 Sep 2026 19:13:29 +0000 Subject: [PATCH 3/9] fix(slurm): survive a transient squeue failure; tell confined jobs the truth about their lifecycle *One empty squeue read retired the watchdog.* The loop treated empty output as "the job left the queue" and broke. squeue prints nothing on a controller RPC timeout too -- routine on a busy scheduler -- so a single blip ended the watchdog on a job with hours left and told the human the allocation had ended. Verified against the rendered script: 10h remaining, one simulated timeout, watchdog gone. cli._slurm_job_state already draws this distinction carefully ("An ssh/squeue failure is NOT evidence the job is dead") 400 lines away; the timer contradicted it. It now needs three consecutive empty reads before believing the job is gone. A job that really ended is noticed a couple of minutes later, which costs nothing -- the message is informational, and every deadline warning has already fired by then. *The lifecycle hint was false under sbatch.* Both "agent session gone" messages said the job was kept alive and offered `sucoder release` / `scancel`. That is true for salloc, where the user owns the allocation, and wrong for confined jobs: the batch body's keeper loop polls the same tmux session, so the job COMPLETEs with it. A confined user was being sent after a job that had already ended. The hint is now chosen by launch mode, which the builder already knows (`job_id is None` means the id is read from $SLURM_JOB_ID at run time, i.e. sbatch). 689 passed; mypy unchanged (57 pre-existing). Both fixes were checked by reintroducing the bug and confirming the new tests fail. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018Ty45bJv1jADvUbyLaW4RD --- sucoder/slurm_timer.py | 42 +++++++++++++++++++++++++--- tests/test_slurm_timer_script.py | 48 ++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 4 deletions(-) diff --git a/sucoder/slurm_timer.py b/sucoder/slurm_timer.py index ad0b751..81665d3 100644 --- a/sucoder/slurm_timer.py +++ b/sucoder/slurm_timer.py @@ -147,7 +147,7 @@ if [ "$TMUX_READY" -eq 0 ]; then # The user owns the SLURM lifecycle (see `sucoder release`): leave the # allocation alone even though the agent never appeared. - echo "Timed out waiting for tmux session $TMUX_SESSION; SLURM job $JOB kept alive. Run 'sucoder release' or 'scancel $JOB' to free the allocation." > "$WARN_FILE" + echo "Timed out waiting for tmux session $TMUX_SESSION; @LIFECYCLE@" > "$WARN_FILE" exit 1 fi @@ -156,16 +156,33 @@ "${TMUX_BIN[@]}" set-option -t "$TMUX_SESSION" display-time 15000 2>/dev/null || true elapsed=0 +missed=0 while true; do left=$(squeue --job "$JOB" --noheader -o "%L" 2>/dev/null) if [ -z "$left" ]; then - warn "SLURM job $JOB is no longer queued -- allocation may have ended." - break + # Empty output means the job left the queue -- but squeue prints + # nothing on a scheduler RPC timeout too, which is routine on a + # busy controller and is NOT evidence the job is dead (the same + # distinction cli._slurm_job_state is careful about). Breaking on + # the first empty read let one transient failure retire the + # watchdog on a job with hours left, silently removing the very + # warnings this script exists to give. Believe it only after + # several consecutive misses; a job that really ended is noticed + # a few minutes later, which costs nothing. + missed=$((missed + 1)) + if [ "$missed" -ge 3 ]; then + warn "SLURM job $JOB is no longer queued -- allocation may have ended." + break + fi + sleep 60 + elapsed=$((elapsed + 1)) + continue fi + missed=0 # Agent gone: record it but do NOT scancel (see above). if ! "${TMUX_BIN[@]}" has-session -t "$TMUX_SESSION" 2>/dev/null; then - echo "Agent tmux session is gone; SLURM job $JOB kept alive. Run 'sucoder release' or 'scancel $JOB' to free the allocation." > "$WARN_FILE" + echo "Agent tmux session is gone; @LIFECYCLE@" > "$WARN_FILE" break fi @@ -240,6 +257,22 @@ def build_timer_script( ) tmux_cmd = "tmux" if tmux_socket is None else f"tmux -L {shlex.quote(tmux_socket)}" job_ref = '"${SLURM_JOB_ID:-}"' if job_id is None else shlex.quote(str(job_id)) + # What happens to the allocation when the agent's session goes away is + # the opposite in the two modes, and telling a confined user to run + # `scancel` on a job that already completed is worse than saying + # nothing. Under sbatch the batch body's keeper loop polls the same + # session, so the job ends with it; under salloc the user owns the + # allocation and it survives. + if job_id is None: + lifecycle = ( + "SLURM job $JOB ends with it (the batch body exits when the " + "session does)." + ) + else: + lifecycle = ( + "SLURM job $JOB kept alive. Run 'sucoder release' or " + "'scancel $JOB' to free the allocation." + ) return ( _TEMPLATE .replace("@MIRROR_TOKEN@", shlex.quote(mirror_token)) @@ -248,6 +281,7 @@ def build_timer_script( .replace("@SNAPSHOT_DIR@", snapshot_word) .replace("@SNAPSHOT_MINUTES@", str(int(snapshot_minutes))) .replace("@JOB_REF@", job_ref) + .replace("@LIFECYCLE@", lifecycle) .replace("@LEFT_TO_MINS@", TIME_LEFT_TO_MINS_SH) .replace("@SNAPSHOT_WIP@", WIP_SNAPSHOT_SH) ) diff --git a/tests/test_slurm_timer_script.py b/tests/test_slurm_timer_script.py index bd74d94..6dafea2 100644 --- a/tests/test_slurm_timer_script.py +++ b/tests/test_slurm_timer_script.py @@ -337,3 +337,51 @@ def test_skipped_poll_does_not_walk_back_down(tmp_path): assert len(deadline) == 2, deadline assert "Start wrapping up" in deadline[0] assert "Commit and save NOW" in deadline[1] + + +@_bash +def test_transient_squeue_failure_does_not_retire_the_watchdog(tmp_path): + """One empty squeue read must not end the loop. + + squeue prints nothing both when the job is gone and when the + controller RPC times out, so breaking on the first empty read let a + single transient failure remove every remaining deadline warning + from a job with hours left -- the exact failure this script exists + to prevent. + """ + # 10h left, one transient blank, then the countdown resumes and + # every threshold still fires. + msgs = _drive(tmp_path, ["10:00:00", "", "25:00", "12:00", "4:00"]) + deadline = [m for m in msgs if "min left" in m] + assert len(deadline) == 3, deadline + assert "Commit and save NOW" in deadline[-1] + + +@_bash +def test_sustained_squeue_silence_still_reports_the_job_gone(tmp_path): + """Tolerating blips must not mean never noticing a finished job.""" + msgs = _drive(tmp_path, ["10:00:00"]) + assert any("no longer queued" in m for m in msgs), msgs + + +def test_lifecycle_hint_matches_the_launch_mode(): + """Under sbatch the batch body's keeper loop exits with the tmux + session, so the job ends with it; telling that user to `scancel` a + job that already completed sends them after a ghost. Under salloc + the allocation really does survive.""" + def hints(script): + # Only the two warning messages, not the surrounding comments. + return [l for l in script.splitlines() + if l.strip().startswith("echo ") and "$WARN_FILE" in l + and ("kept alive" in l or "ends with it" in l)] + + confined = hints(_render(tmux_socket="s")) # job id read at run time + unconfined = hints(_render(job_id=42)) + # Both places say it: the startup timeout and the session-gone exit. + assert len(confined) == 2 and len(unconfined) == 2 + for line in confined: + assert "ends with it" in line + assert "kept alive" not in line and "sucoder release" not in line + for line in unconfined: + assert "kept alive" in line and "sucoder release" in line + assert "ends with it" not in line From 2c2435435f96bb01535cf139f2c3294951e35e00 Mon Sep 17 00:00:00 2001 From: Ethan Ligon & Sue Coder Date: Wed, 9 Sep 2026 21:29:32 +0000 Subject: [PATCH 4/9] test(slurm): close three gaps where the suite passed while the thing it checked was broken Found by mutating the suite: each of these regressions shipped green against all 680 tests. *`--force` was never exercised.* Every push in every snapshot test creates the WIP ref for the first time, where `--force` does nothing, so dropping it passed. It is load-bearing in reality: the ref is built with `commit-tree -p HEAD`, so once the agent rebases, resets, or switches branch the new snapshot is not a descendant of the old one and an unforced push is rejected -- silently, every step of snapshot_wip being best-effort -- in precisely the situation the snapshotter exists for. The new test diverges HEAD between two snapshots and asserts the ref advanced (and that the two really are unrelated). *The cadence assertion could not fail.* `_confined_manager` built its SlurmConfig without `wip_snapshot_minutes`, so it got the dataclass default 10 -- identical to build_timer_script's own default. Asserting "SNAPSHOT_MINUTES=10" therefore could not distinguish a value read from config from one hardcoded in the builder, and hardcoding it passed. The fixture now sets 7. *The snapshot directory was only checked non-empty.* The fixture stubs the remote path, so the exact value was available all along; asserting merely that SNAPSHOT_DIR was set let "snapshot the cache dir instead of the mirror" pass -- which would save none of the agent's work. 690 passed; mypy unchanged (57 pre-existing). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018Ty45bJv1jADvUbyLaW4RD --- tests/test_mirror.py | 10 ++++++-- tests/test_slurm_timer_script.py | 43 ++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/tests/test_mirror.py b/tests/test_mirror.py index 0919493..b93bac7 100644 --- a/tests/test_mirror.py +++ b/tests/test_mirror.py @@ -1290,6 +1290,10 @@ def _confined_manager(tmp_path, monkeypatch, *, target_name=None): partition="savio4_htc", account="co_carleton", qos="carleton_htc4_normal", cpus_per_task=4, mem="16G", confined=True, + # Deliberately NOT the dataclass default (10): with the + # default, asserting "SNAPSHOT_MINUTES=10" cannot tell a + # value read from config from one hardcoded in the builder. + wip_snapshot_minutes=7, ), ) monkeypatch.setattr( @@ -1738,8 +1742,10 @@ def test_launch_confined_stages_and_starts_deadline_timer(tmp_path, monkeypatch) assert 'JOB="${SLURM_JOB_ID:-}"' in timer["input"] assert "TMUX_BIN=(tmux -L sucoder-sample)" in timer["input"] assert "MIRROR_TOKEN=sample" in timer["input"] - assert "SNAPSHOT_MINUTES=10" in timer["input"] - assert "SNAPSHOT_DIR=" in timer["input"] and "SNAPSHOT_DIR=''" not in timer["input"] + assert "SNAPSHOT_MINUTES=7" in timer["input"], "cadence must come from config" + # The exact tree, not merely "non-empty": snapshotting the wrong + # directory would save none of the agent's work. + assert "SNAPSHOT_DIR=/global/home/users/coder/mirrors/sample\n" in timer["input"] # sbatch is submitted only after both files are staged. sbatch_idx = next(i for i, c in enumerate(calls) if c["args"][0] == "sbatch") assert all(calls.index(w) < sbatch_idx for w in writes) diff --git a/tests/test_slurm_timer_script.py b/tests/test_slurm_timer_script.py index 6dafea2..d3b159c 100644 --- a/tests/test_slurm_timer_script.py +++ b/tests/test_slurm_timer_script.py @@ -385,3 +385,46 @@ def hints(script): for line in unconfined: assert "kept alive" in line and "sucoder release" in line assert "ends with it" not in line + + +@_bash +@_git +def test_snapshot_force_updates_a_diverged_wip_ref(repo_pair): + """The second snapshot of a tree whose HEAD has diverged must land. + + Every other push here creates the ref for the first time, so + ``--force`` never does any work and dropping it left the suite + green. It is load-bearing in reality: the WIP ref is built with + ``commit-tree -p HEAD``, so once the agent rebases, resets, or + switches branch, the new snapshot is not a descendant of the old one + and an unforced push is rejected -- silently, every step being + best-effort -- in exactly the scenario the snapshotter exists for. + """ + origin, work = repo_pair + ref = "refs/sucoder/wip/mirror" + show = lambda: subprocess.run( + ["git", "show-ref", "-s", ref], cwd=origin, + capture_output=True, text=True).stdout.strip() + + (work / "a.txt").write_text("first\n") + assert _snapshot(work).returncode == 0 + first = show() + assert first, "first snapshot did not create the ref" + + # Diverge: commit onto HEAD, so the next WIP commit's parent is no + # longer an ancestor of the ref just pushed. + _git_run(work, "add", "-A") + _git_run(work, "-c", "user.email=a@b", "-c", "user.name=a", + "commit", "-q", "-m", "work") + (work / "a.txt").write_text("second\n") + + assert _snapshot(work).returncode == 0 + second = show() + assert second and second != first, ( + f"diverged snapshot did not update the ref ({first} -> {second}); " + "an unforced push would be rejected here" + ) + # The new snapshot really is not a descendant of the old one. + assert subprocess.run( + ["git", "merge-base", "--is-ancestor", first, second], + cwd=origin).returncode != 0 From 258accbc429d032f1d336b6efe0feb878e9baae7 Mon Sep 17 00:00:00 2001 From: Ethan Ligon Date: Fri, 11 Sep 2026 23:49:12 +0000 Subject: [PATCH 5/9] Fix collaborate startup safety and SLURM timer supervision --- README.org | 31 ++++- docs/startup-fixes.org | 80 +++++++++++ sucoder/cli.py | 62 +++++---- sucoder/mirror.py | 136 +++++++++---------- sucoder/remote_bootstrap.py | 39 ++++++ sucoder/slurm_timer.py | 18 ++- sucoder/timer_lifecycle.py | 71 ++++++++++ tests/test_batch_script.py | 4 +- tests/test_cli.py | 6 +- tests/test_mirror.py | 4 +- tests/test_remote.py | 215 ++++++------------------------ tests/test_slurm_timer_script.py | 2 +- tests/test_startup_regressions.py | 163 ++++++++++++++++++++++ tests/test_timer_lifecycle.py | 161 ++++++++++++++++++++++ 14 files changed, 707 insertions(+), 285 deletions(-) create mode 100644 docs/startup-fixes.org create mode 100644 sucoder/remote_bootstrap.py create mode 100644 sucoder/timer_lifecycle.py create mode 100644 tests/test_startup_regressions.py create mode 100644 tests/test_timer_lifecycle.py diff --git a/README.org b/README.org index 40bc453..55b2657 100644 --- a/README.org +++ b/README.org @@ -229,9 +229,11 @@ sucoder status project prompt when the two histories have diverged. - sync :: an alias for =push=, kept for compatibility. -=agents-clone= is for creating the mirror, not for routine pushing: on a -remote mirror it will rebuild the repository from scratch if it decides -the remote copy is a half-initialised husk. +=agents-clone= creates the mirror. Remote startup initializes absent or +empty directories and recovers valid repositories in place. It never deletes +a directory because a probe failed or a repository has no default branch. +Unreadable, invalid, symlinked, or non-repository nonempty paths stop startup +with a diagnostic; inspect them before retrying. ** Mirror safety: the pull must succeed before the push =push= (and =agents-clone=) send to the mirror with =git push --all @@ -243,6 +245,10 @@ push is refused rather than allowed to overwrite unretrieved commits. An empty or half-initialised mirror is *not* treated as unreadable, so first-time bootstrap is unaffected: sucoder asks the mirror whether it holds any commits instead of guessing from the error text. +Any ref (including feature branches, tags, and WIP refs) counts as content, +even with an unborn HEAD. A repository initialized by this invocation skips +the initial fetch and receives a non-forcing first push. The +=--allow-unverified-mirror= override does not bypass initialization checks. - --allow-unverified-mirror :: Push anyway, discarding any unpulled mirror commits. Use when the mirror is known to be expendable. @@ -783,11 +789,24 @@ is the right behaviour for whole-node partitions. Every SLURM-backed session (=salloc= or =confined= =sbatch=) starts a small watchdog on the compute node. It warns at 30, 15, and 5 minutes before the allocation's =--time= via =tmux display-message= and by -writing =$HOME/.cache/sucoder/slurm-deadline-.warn= (the -un-suffixed =slurm-deadline.warn= is also written for prompts that -poll the older path). It never cancels the job; that stays with +writing a warning under +=$HOME/.cache/sucoder/timers//-/= (the +un-suffixed =slurm-deadline.warn= and per-mirror +=slurm-deadline-.warn= are also written in the cache root for +older prompts; these compatibility copies show the last writer). +It never cancels the job; that stays with =sucoder release=. +The scope is a digest of the mirror and target names. The timer uses Linux +=/proc= and =flock= to verify/reuse a live watchdog for that allocation; +repeated startup does not restart it or reset warning state. Startup reports +whether it started or reused a timer. Failures explicitly report that +deadline warnings and periodic snapshots are unavailable; diagnostics remain +in =timer.log= beside its =owner= and =status= files. Timers from the older +implementation are not killed by a broad process-name match; they exit with +their old allocation. Shared-filesystem lock behavior should be verified +when bringing up a new target. + The same script snapshots the mirror's dirty working tree (tracked and untracked files, not ignored ones) to =refs/sucoder/wip/= on the mirror's =origin= at each warning and every diff --git a/docs/startup-fixes.org b/docs/startup-fixes.org new file mode 100644 index 0000000..eee0b31 --- /dev/null +++ b/docs/startup-fixes.org @@ -0,0 +1,80 @@ +#+title: Collaborate startup fixes — implementation and validation + +* Remote initialization and reconciliation + +Remote bootstrap no longer removes a mirror directory. A small Bash protocol +initializes absent or empty directories, recognizes the intended working tree, +and refuses symlinks, broken repositories, arbitrary nonempty directories, and +failed/ambiguous probes. Existing empty repositories recover in place, keeping +their configuration. An unborn HEAD does not hide feature branches, tags, or +WIP refs from the content check. + +A repository initialized by this invocation skips the initial reconciliation +fetch and gets a non-forcing first push, without a subsequent hard reset. +Existing repositories retain reconciliation and the explicit overwrite policy. +Failed working-tree status checks now stop the push, even with the unverified +mirror override. Missing-ref errors accompanied by a generic disconnect tail +are repository errors, not reasons to retry through the DTN. + +Red-team regression cases include SSH errors, timeouts, invalid probe replies, +broken/non-repository directories, ancestor Git discovery, symlinks, unborn +HEAD with a feature branch, interrupted initialization, status failure, and a +concurrent commit arriving before initial publication. Existing files/refs and +configuration are checked after failure; tests do not merely assert commands. + +* Timer lifecycle + +Both launch modes use the watchdog's =--ensure= handshake. Startup and lifetime +locks are scoped by target, mirror, node, and allocation. A healthy owner is +reused; a stale owner record is checked against Linux process birth time. +There is no process-name matching or PID-based termination. Scripts are staged +under distinct names so a running script is not truncated. Readiness and +failure diagnostics are explicit, and the background process detaches stdin +and startup lock descriptors. + +Primary warnings and logs are allocation-scoped. The old cache-root warning +paths remain compatibility copies; simultaneous writers can replace those +copies, so the scoped files are authoritative. Pre-upgrade timers are not +forcibly retired and can coexist until their old allocation ends. + +Red-team tests execute real Bash/flock processes: concurrent starters, restart +with stale metadata, target/allocation separation, missing executables, SSH +timeout handling, and both confined/unconfined script variants. A real Git +snapshot test checks warning delivery, tracked/untracked work preservation, +and that the agent's staged index is unchanged. + +* Shared skills + +The YAML corrections are already upstream in closed issue +[[https://github.com/ligon/sucoder-skills/issues/1][sucoder-skills #1]]. The local consuming checkout at =d6462b7= still carried +the old descriptions. Filed [[https://github.com/ligon/sucoder-skills/issues/3][sucoder-skills #3]] for non-destructive deployment, +revision verification on local/Savio consumers, and validation coverage. +No shared skill checkout was modified by this implementation. A locally built +Savio catalog containing an error is not evidence of Savio's own checkout +revision; the follow-up explicitly calls for checking both. + +* Validation + +- =.venv/bin/python -m pytest -q=: 744 passed. Existing Typer completion + deprecation warnings remain. +- Local CLI smoke: =collaborate= against a disposable repository with an + explicit stand-in command exited 0 and emitted =HARNESS_STARTED=. Used the + current account with =--no-agent-sudo=, an isolated Git config, and + =--no-agent-remote=; this does not validate cross-account sudo policy. +- Savio smoke: ran under existing allocation =38714904= on =n0170.savio3=. + Temporary directories on shared HOME exercised create/reuse and preservation + of non-repository files. Both timer variants reported =STARTED monitoring= + followed by =REUSED monitoring=. Temporary test files were removed. +- The Savio smoke used stand-in tmux/squeue responses and did not launch an + LLM harness, attach to an agent session, or submit a new allocation. Actual + sbatch submission/attachment remains covered by the existing automated tests, + not a fresh live confined session in this pass. +- GitNexus CLI impact and change analysis identify shared startup, Git + synchronization, and confined launch flows (aggregate CRITICAL risk). + Its bounded process graph is supplementary to the tests and direct review. + +The changes introduce no new user configuration. Timer supervision requires +Bash, flock, and Linux /proc; flock behavior was exercised on Savio's shared +filesystem. Old cached scripts/logs are retained for diagnosis, not swept by +startup. This pass does not redesign synchronization between arbitrary +concurrent writers to an already-established mirror. diff --git a/sucoder/cli.py b/sucoder/cli.py index 3aef78b..d46bd74 100644 --- a/sucoder/cli.py +++ b/sucoder/cli.py @@ -3,6 +3,7 @@ from __future__ import annotations import getpass +import hashlib import json import os import re @@ -46,7 +47,7 @@ from .executor import CommandError, CommandExecutor from .logging_utils import setup_logger from .local_tier import work_path -from .slurm_timer import TIME_LEFT_TO_MINS_SH, build_timer_script +from .slurm_timer import TIME_LEFT_TO_MINS_SH, build_timer_script, timer_identity from .mirror import ( _sanitize_session_token, MirrorError, @@ -1183,8 +1184,11 @@ def _start_slurm_timer( tmux_session=tmux_session, job_id=job_id, snapshot_dir=snapshot_dir, + timer_scope=timer_identity(session.mirror_name, getattr(session, "target_name", None)), ) - script_name = f"slurm-timer-{token}.sh" + # Immutable script names: staging never truncates a running shell's file. + digest = hashlib.sha256(timer_script.encode()).hexdigest()[:24] + script_name = f"slurm-timer-{digest}.sh" # Write the script to the compute node via stdin, then run it. # The script lives in the user's runtime cache rather than /tmp for @@ -1195,36 +1199,46 @@ def _start_slurm_timer( node = session.compute_node q_script = shlex.quote(script_name) - write_result = _sp.run( + + def timer_run(*args, **kwargs): + # Timer diagnostics must not abort an otherwise usable session. + try: + return _sp.run(*args, **kwargs) + except (_sp.TimeoutExpired, OSError) as exc: + return _sp.CompletedProcess(args[0], -1, "", str(exc)) + + write_result = timer_run( ["ssh", *ssh_opts, node, - 'mkdir -p "$HOME/.cache/sucoder" && ' - 'chmod 700 "$HOME/.cache/sucoder" 2>/dev/null || true; ' - f'cat > "$HOME/.cache/sucoder/"{q_script} && ' - f'chmod 700 "$HOME/.cache/sucoder/"{q_script}'], - input=timer_script, capture_output=True, text=True, check=False, + 'umask 077; mkdir -p "$HOME/.cache/sucoder" && ' + 'tmp=$(mktemp "$HOME/.cache/sucoder/.timer.XXXXXX") && ' + 'cat > "$tmp" && chmod 700 "$tmp" && ' + f'mv "$tmp" "$HOME/.cache/sucoder/"{q_script}'], + input=timer_script, capture_output=True, text=True, check=False, timeout=30, ) if write_result.returncode != 0: - logger.warning("Failed to write SLURM timer script: %s", - write_result.stderr.strip()) + logger.warning("Failed to write SLURM timer on %s for job %s (exit %s): %s. " + "Deadline warnings and periodic snapshots are unavailable.", + node, job_id, write_result.returncode, + write_result.stderr.strip() or "no stderr") return - # Every _build_executor for this target (attach, pull, status, ...) - # lands here, so retire the previous timer for this mirror first or - # they pile up, each snapshotting. The [s] bracket keeps pkill from - # matching the shell that runs it. - q_pattern = shlex.quote(f"[s]lurm-timer-{token}.sh") - run_result = _sp.run( + # The script ensures one live watchdog using allocation-scoped locks. + # No process name matching, signaling stale PIDs, or routine restarts. + run_result = timer_run( ["ssh", *ssh_opts, node, - f'pkill -u "$USER" -f {q_pattern} 2>/dev/null; ' - f'nohup "$HOME/.cache/sucoder/"{q_script} > /dev/null 2>&1 &'], - capture_output=True, text=True, check=False, + f'bash "$HOME/.cache/sucoder/"{q_script} --ensure'], + capture_output=True, text=True, check=False, timeout=30, ) - if run_result.returncode == 0: - logger.info("SLURM deadline timer started on %s for job %d", - node, job_id) + if run_result.returncode == 0 and run_result.stdout.strip().startswith( + ("SUCODER_TIMER_STARTED ", "SUCODER_TIMER_REUSED ") + ): + logger.info("SLURM deadline timer ready on %s for job %d: %s", + node, job_id, run_result.stdout.strip()) else: - logger.warning("Failed to start SLURM timer: %s", - run_result.stderr.strip()) + logger.warning("Failed to start SLURM timer on %s for job %s (exit %s): %s. " + "Deadline warnings and periodic snapshots are unavailable.", + node, job_id, run_result.returncode, + run_result.stderr.strip() or "no valid readiness response") def _prompt_yes_no(message: str) -> bool: diff --git a/sucoder/mirror.py b/sucoder/mirror.py index ccf8c9f..61c8fe1 100644 --- a/sucoder/mirror.py +++ b/sucoder/mirror.py @@ -19,6 +19,8 @@ import yaml +from .remote_bootstrap import INITIALIZE_MIRROR_SH + class SkillMetadata(NamedTuple): """What a skill file's frontmatter yielded. @@ -74,7 +76,7 @@ def _frontmatter_problem(metadata: Optional[SkillMetadata]) -> Optional[str]: ) from .skills_version import validate_skills_version from .local_tier import build_prepare_script, cache_exports_sh, work_path, work_path_shell -from .slurm_timer import build_timer_script +from .slurm_timer import build_timer_script, timer_identity from .workspace_prefs import WorkspacePrefs @@ -891,6 +893,11 @@ def _is_transport_failure(result: "CommandResult") -> bool: if result.returncode in (-1, 255): return True stderr = (result.stderr or "").lower() + # Git can append "remote end hung up" to a definitive missing-ref + # response. That is not a connection fault and won't improve on DTN. + if any(line.startswith("fatal: couldn't find remote ref ") + for line in stderr.splitlines()): + return False # The shared transient set, plus ``could not resolve hostname``: # for git transport a DNS miss is worth failing over to another # node (unlike a ControlMaster bring-up, where it won't self-heal). @@ -904,6 +911,8 @@ def _short_git_error(result: "CommandResult") -> str: last = "" for line in (result.stderr or "").splitlines(): stripped = line.strip() + if stripped.lower().startswith("fatal: couldn't find remote ref "): + return stripped if stripped and not stripped.lower().startswith(skip): last = stripped return last or f"rc={result.returncode}" @@ -1443,6 +1452,12 @@ def _ensure_remote_worktree_clean( check=False, cwd=remote_path, ) + if result.returncode != 0: + raise MirrorError( + f"Could not inspect remote working tree {remote_path} " + f"(exit {result.returncode}): {result.stderr.strip() or 'no stderr'}. " + "Refusing to push over unverified work." + ) dirty = (result.stdout or "").strip() if not dirty: return True @@ -1566,7 +1581,7 @@ def _exists(name: str) -> bool: cwd=remote_path, ) - def _sync_remote(self, ctx: MirrorContext) -> None: + def _sync_remote(self, ctx: MirrorContext, *, force: bool = True) -> None: """Push local canonical commits to the remote mirror. Pushes over the login node (the reliable, session-capable @@ -1584,7 +1599,7 @@ def _sync_remote(self, ctx: MirrorContext) -> None: ) try: self.executor.run_human( - ["git", "push", url, "--all", "--force"], + ["git", "push", url, "--all"] + (["--force"] if force else []), check=True, cwd=str(ctx.canonical_path), env=env, @@ -1621,22 +1636,27 @@ def _remote_repo_has_content( ) -> bool: """Return ``True`` if the remote git repo has real content. - A mirror that exists on disk but has neither a HEAD commit nor - the *base* branch is a husk left by a previously failed bootstrap - (``git init`` ran, but no push ever landed). Fetching from such - a repo fails with "couldn't find remote ref " and pushing - into it is fragile, so callers rebuild it from scratch rather - than sync into it. + An unborn HEAD is not sufficient evidence of emptiness: feature + branches, tags and WIP refs also count. Empty repositories can be + populated in place; they are never deleted by this probe. Raises ``MirrorError`` when the question cannot be answered, which is not the same as answering "no" -- see :meth:`_rev_exists`. """ if self._rev_exists(run, remote_path, "HEAD"): return True - # HEAD may be an unborn symbolic ref pointing at a branch that - # does exist (e.g. a non-default checkout); verify the base - # branch directly before declaring the repo empty. - return self._rev_exists(run, remote_path, f"refs/heads/{base}") + # An unborn HEAD can coexist with feature branches, tags, or WIP + # refs. None of these may be mistaken for a disposable empty repo. + refs = run( + ["git", "for-each-ref", "--format=%(refname)", "--count=1"], + check=False, cwd=remote_path, + ) + if refs.returncode != 0: + raise MirrorError( + f"Could not enumerate refs in {remote_path} " + f"(exit {refs.returncode}): {refs.stderr.strip()}" + ) + return bool(refs.stdout.strip()) @staticmethod def _rev_exists(run: Callable, repo_path: str, ref: str) -> bool: @@ -1706,59 +1726,28 @@ def ensure_remote_clone( base = self._resolve_base_branch(ctx) - # Check if remote mirror is a valid git repo. - check = run( - ["git", "rev-parse", "--git-dir"], + # The remote script initializes only absent/empty directories. A + # failed probe never licenses deletion, even with the overwrite flag. + probe = run( + ["bash", "-c", INITIALIZE_MIRROR_SH, "sucoder-init", abs_remote_path, base], check=False, - cwd=abs_remote_path, ) - repo_exists = check.returncode == 0 - # A repo can exist on disk yet be a husk from a previously failed - # bootstrap: `git init` ran but no push ever landed, so there are - # no commits and no base branch. That is exactly the state that - # produced the "couldn't find remote ref main" fetch failure - # followed by a wedged push. Treat such a husk as broken and - # rebuild it rather than syncing into it. - repo_usable = repo_exists and self._remote_repo_has_content( - run, abs_remote_path, base, - ) - if repo_exists and not repo_usable: - self.logger.warning( - "Remote mirror at %s exists but is empty/half-initialised " - "(no commits, no '%s' branch) — rebuilding it from scratch", - remote_path, base, + marker = probe.stdout.strip().splitlines()[-1:] if probe.stdout else [] + fresh = marker == ["SUCODER_MIRROR_CREATED"] + if not self.executor.dry_run and ( + probe.returncode != 0 or marker not in ( + ["SUCODER_MIRROR_CREATED"], ["SUCODER_MIRROR_EXISTING"], ) - - if repo_usable: - self.logger.info("Remote mirror already exists at %s", remote_path) - else: - # Clean up a missing/broken/half-initialised directory before - # a fresh init. Safe even when the repo merely existed-but- - # empty: a husk has no commits, so there is nothing to lose. - run( - ["rm", "-rf", abs_remote_path], - check=False, - ) - self.logger.info("Initialising remote mirror at %s", remote_path) - # Create with restrictive permissions: mirror roots on shared - # filesystems are visible to every user on the cluster. - run( - ["bash", "-c", - f"umask 077 && mkdir -p {shlex.quote(abs_remote_path)}"], - check=True, - ) - # Lock down the parent mirrors/ directory too (if we created it). - mirrors_parent = abs_remote_path.rsplit("/", 1)[0] - if mirrors_parent: - run( - ["chmod", "700", mirrors_parent], - check=False, # may not own the parent - ) - run( - ["git", "init", "-b", base], - check=True, - cwd=abs_remote_path, + ): + raise MirrorError( + f"Could not safely initialize remote mirror {abs_remote_path} " + f"(exit {probe.returncode}): {probe.stderr.strip() or 'invalid probe response'}. " + "Existing files left untouched." ) + self.logger.info( + "%s remote mirror at %s", + "Initialized" if fresh else "Using existing", remote_path, + ) # Always ensure the config is correct (may have been missed # by a failed earlier init). @@ -1768,11 +1757,9 @@ def ensure_remote_clone( cwd=abs_remote_path, ) - # Pull any agent commits before overwriting the mirror. A mirror - # we just re-initialised above probes as empty, so bootstrap is - # unaffected; this only bites when a mirror with commits could not - # be read, which is precisely when the push below must not run. - if not self._pull_from_remote(ctx) and not allow_unverified_mirror: + # Reconcile existing repositories before overwriting any branches. + # This invocation's fresh init has nothing to fetch yet. + if not fresh and not self._pull_from_remote(ctx) and not allow_unverified_mirror: raise MirrorError(self._unverified_mirror_message(ctx)) # The remote mirror uses receive.denyCurrentBranch=updateInstead, @@ -1788,7 +1775,11 @@ def ensure_remote_clone( # is trying to preserve. return False - # Push canonical content to the remote via tunnel. + # Initial publication must not overwrite a concurrent writer. Git's + # updateInstead populates the unborn checked-out branch on receipt. + if fresh: + self._sync_remote(ctx, force=False) + return True self._sync_remote(ctx) # Ensure HEAD points to the correct branch so that @@ -2355,7 +2346,7 @@ def _build_batch_script( session ends, the keeper exits, and the job frees. ``timer_path`` (a staged ``slurm_timer.build_timer_script`` output) - is started with ``nohup`` *after* the session is confirmed and + is started/reused through its ``--ensure`` handshake after the session is confirmed and before the keeper loop, so it runs inside the job cgroup and dies with the job. Without it a confined job has no deadline watchdog at all: ``cli._start_slurm_timer`` only runs on the salloc path. @@ -2409,7 +2400,8 @@ def _build_batch_script( " exit 1\n" "fi\n" + ( - f"nohup {shlex.quote(timer_path)} > /dev/null 2>&1 &\n" + f"bash {shlex.quote(timer_path)} --ensure || " + "echo 'SUCODER: timer failed; deadline warnings and periodic snapshots unavailable' >&2\n" if timer_path else "" ) + f"while tmux -L {q_sock} has-session -t {q_sess} 2>/dev/null; do\n" @@ -2816,7 +2808,7 @@ def _launch_confined( log_path = f"{cache_dir}/job-{safe}-%j.out" mirror_path = self._resolve_remote_path(ctx) - timer_path = f"{cache_dir}/slurm-timer-{safe}.sh" + timer_path = f"{cache_dir}/slurm-timer-{safe}-{secrets.token_hex(8)}.sh" # Local-disk tiering (docs/local-disk-tiering.org): the executor # carries the resolved root (config + --local-disk override); the # agent then works in a clone under /job$SLURM_JOB_ID. @@ -2850,6 +2842,7 @@ def _launch_confined( tmux_socket=socket, snapshot_dir_shell=work_path_shell(local_disk_root, safe), snapshot_minutes=slurm.wip_snapshot_minutes, + timer_scope=timer_identity(ctx.settings.name, self.target_name), ) else: timer_script = build_timer_script( @@ -2858,6 +2851,7 @@ def _launch_confined( tmux_socket=socket, snapshot_dir=mirror_path, snapshot_minutes=slurm.wip_snapshot_minutes, + timer_scope=timer_identity(ctx.settings.name, self.target_name), ) self.executor.run_agent( [ diff --git a/sucoder/remote_bootstrap.py b/sucoder/remote_bootstrap.py new file mode 100644 index 0000000..7049300 --- /dev/null +++ b/sucoder/remote_bootstrap.py @@ -0,0 +1,39 @@ +"""Non-destructive, remotely executed mirror initialization. + +The protocol is deliberately small: only a successful command ending in one +of the two markers authorizes the caller to continue. SSH failures, shell +noise, inaccessible paths and broken repositories are not absence proofs. +""" + +INITIALIZE_MIRROR_SH = r'''set -eu +umask 077 +path=$1 +base=$2 +fail() { echo "sucoder: $*; existing files left untouched" >&2; exit 1; } +case "$path" in /*) ;; *) fail "mirror path must be absolute" ;; esac +[ "$path" != / ] || fail "refusing root as a mirror" +[ ! -L "$path" ] || fail "mirror path is a symlink: $path" +if [ ! -e "$path" ]; then + mkdir -p -- "$(dirname -- "$path")" + # Exclusive creation: never remove a directory that won this race. + mkdir -- "$path" || fail "could not exclusively create $path; retry" +fi +[ -d "$path" ] || fail "mirror is not a directory: $path" +cd -- "$path" || fail "cannot enter $path" +if [ -e .git ] || [ -L .git ]; then + [ ! -L .git ] || fail "mirror .git is a symlink" + top=$(git rev-parse --show-toplevel) || fail "invalid repository at $path" + [ "$top" = "$(pwd -P)" ] || fail "Git resolved a different working tree" + echo SUCODER_MIRROR_EXISTING +else + # Include hidden files, and never mistake ancestor Git discovery for a + # repository at this path. Bare repos and arbitrary directories are kept. + shopt -s nullglob dotglob + entries=(*) + [ ${#entries[@]} -eq 0 ] || fail "nonempty directory has no .git: $path" + mkdir .sucoder-init.lock || fail "another initialization is in progress" + trap 'rmdir .sucoder-init.lock' EXIT + git init -b "$base" >&2 || fail "git initialization failed" + echo SUCODER_MIRROR_CREATED +fi +''' diff --git a/sucoder/slurm_timer.py b/sucoder/slurm_timer.py index 4d50ea4..4e55d52 100644 --- a/sucoder/slurm_timer.py +++ b/sucoder/slurm_timer.py @@ -27,8 +27,16 @@ from __future__ import annotations import shlex +import hashlib from typing import Optional +from .timer_lifecycle import TIMER_LIFECYCLE_SH + + +def timer_identity(mirror_name: str, target_name: Optional[str]) -> str: + """Avoid collisions between sanitized names and targets sharing HOME.""" + return hashlib.sha256(repr((mirror_name, target_name)).encode()).hexdigest()[:24] + # Converts SLURM ``squeue -o %L`` time-left into whole minutes. ``%L`` # renders as ``D-HH:MM:SS`` once a day or more remains, ``HH:MM:SS`` under # a day, ``MM:SS`` under an hour; a job with no limit prints ``UNLIMITED``. @@ -106,8 +114,11 @@ SNAPSHOT_DIR=@SNAPSHOT_DIR@ SNAPSHOT_MINUTES=@SNAPSHOT_MINUTES@ JOB=@JOB_REF@ +TIMER_SCOPE=@TIMER_SCOPE@ +@TIMER_LIFECYCLE@ WARN_FILE="$STATE_DIR/slurm-deadline-$MIRROR_TOKEN.warn" -LEGACY_WARN_FILE="$STATE_DIR/slurm-deadline.warn" +LEGACY_WARN_FILE="$CACHE_DIR/slurm-deadline.warn" +MIRROR_WARN_FILE="$CACHE_DIR/slurm-deadline-$MIRROR_TOKEN.warn" WARN5="$STATE_DIR/.slurm-warn-5-$MIRROR_TOKEN" WARN15="$STATE_DIR/.slurm-warn-15-$MIRROR_TOKEN" WARN30="$STATE_DIR/.slurm-warn-30-$MIRROR_TOKEN" @@ -125,6 +136,7 @@ warn() { echo "$1" > "$WARN_FILE" echo "$1" > "$LEGACY_WARN_FILE" + echo "$1" > "$MIRROR_WARN_FILE" "${TMUX_BIN[@]}" display-message -t "$TMUX_SESSION" "$1" 2>/dev/null } @@ -149,6 +161,7 @@ # Make each warning linger on the status line so a full-screen agent TUI # does not redraw over it before the human notices. "${TMUX_BIN[@]}" set-option -t "$TMUX_SESSION" display-time 15000 2>/dev/null || true +echo monitoring > "$STATE_DIR/status" elapsed=0 while true; do @@ -197,6 +210,7 @@ def build_timer_script( snapshot_dir: Optional[str] = None, snapshot_dir_shell: Optional[str] = None, snapshot_minutes: int = 10, + timer_scope: Optional[str] = None, ) -> str: """Render the timer script. @@ -231,6 +245,8 @@ def build_timer_script( job_ref = '"${SLURM_JOB_ID:-}"' if job_id is None else shlex.quote(str(job_id)) return ( _TEMPLATE + .replace("@TIMER_SCOPE@", shlex.quote(timer_scope or timer_identity(mirror_token, None))) + .replace("@TIMER_LIFECYCLE@", TIMER_LIFECYCLE_SH) .replace("@MIRROR_TOKEN@", shlex.quote(mirror_token)) .replace("@TMUX_SESSION@", shlex.quote(tmux_session)) .replace("@TMUX_CMD@", tmux_cmd) diff --git a/sucoder/timer_lifecycle.py b/sucoder/timer_lifecycle.py new file mode 100644 index 0000000..8f7e4eb --- /dev/null +++ b/sucoder/timer_lifecycle.py @@ -0,0 +1,71 @@ +"""Bash timer supervision without process-name matching or PID-based killing.""" + +TIMER_LIFECYCLE_SH = r''' +CACHE_DIR="$STATE_DIR" +node=$(hostname) || exit 1 +[[ "$node" =~ ^[a-zA-Z0-9._-]+$ && "$JOB" =~ ^[0-9]+$ ]] || { + echo "sucoder timer: invalid node or allocation identity" >&2; exit 1; +} +STATE_DIR="$CACHE_DIR/timers/$TIMER_SCOPE/$node-$JOB" +mkdir -p "$STATE_DIR" || exit 1 +chmod 700 "$STATE_DIR" || exit 1 +command -v flock >/dev/null || { echo "sucoder timer: flock unavailable" >&2; exit 1; } + +process_start() { + local stat + stat=$(cat "/proc/$1/stat" 2>/dev/null) || return 1 + # Strip comm (which can contain spaces/parentheses); starttime is the + # twentieth field after it. No PID is ever used to signal a process. + stat=${stat##*) } + local fields=($stat) + printf '%s\n' "${fields[19]}" +} +live_timer() { + local pid stamp + read -r pid stamp < "$STATE_DIR/owner" || return 1 + [[ "$pid" =~ ^[0-9]+$ && -n "$stamp" ]] || return 1 + [ "$(process_start "$pid")" = "$stamp" ] +} + +if [ "${1:-}" = --ensure ]; then + # Serialize starters. The watchdog closes this fd so it cannot retain + # the startup lock for its lifetime. Locks are scoped by node and job. + exec 8>"$STATE_DIR/start.lock" + flock -w 10 8 || { echo "sucoder timer: startup lock busy" >&2; exit 1; } + exec 9>"$STATE_DIR/run.lock" + if ! flock -n 9; then + if live_timer && [ -s "$STATE_DIR/status" ]; then + echo "SUCODER_TIMER_REUSED $(cat "$STATE_DIR/status")" + exit 0 + fi + echo "sucoder timer: lock held but owner is not healthy; retry" >&2 + exit 1 + fi + flock -u 9 + rm -f "$STATE_DIR/owner" "$STATE_DIR/status" + nohup bash "$0" --run >"$STATE_DIR/timer.log" 2>&1 8>&- 9>&- & + child=$! + for attempt in {1..50}; do + if live_timer 2>/dev/null && [ -s "$STATE_DIR/status" ]; then + echo "SUCODER_TIMER_STARTED $(cat "$STATE_DIR/status")" + exit 0 + fi + kill -0 "$child" 2>/dev/null || break + sleep 0.1 + done + echo "sucoder timer: startup failed; see $STATE_DIR/timer.log" >&2 + tail -n 5 "$STATE_DIR/timer.log" >&2 + exit 1 +fi + +exec 9>"$STATE_DIR/run.lock" +flock -n 9 || exit 0 +for executable in tmux squeue; do + command -v "$executable" >/dev/null || { + echo "sucoder timer: $executable unavailable" >&2; exit 1; + } +done +printf '%s %s\n' "$$" "$(process_start "$$")" > "$STATE_DIR/owner" +trap 'rm -f "$STATE_DIR/owner" "$STATE_DIR/status"' EXIT +echo waiting-for-tmux > "$STATE_DIR/status" +''' diff --git a/tests/test_batch_script.py b/tests/test_batch_script.py index d9f0d5d..0c0a027 100644 --- a/tests/test_batch_script.py +++ b/tests/test_batch_script.py @@ -123,7 +123,7 @@ def test_timer_started_after_session_check_before_keeper(): s = MirrorManager._build_batch_script( **_BASE, timer_path="/global/home/users/ligon/.cache/sucoder/slurm-timer-K-Aggregators.sh", ) - nohup = "nohup /global/home/users/ligon/.cache/sucoder/slurm-timer-K-Aggregators.sh > /dev/null 2>&1 &\n" + nohup = "bash /global/home/users/ligon/.cache/sucoder/slurm-timer-K-Aggregators.sh --ensure" assert nohup in s rc_check = s.index("SUCODER: tmux new-session failed") keeper = s.index("while tmux -L sucoder-K-Aggregators has-session") @@ -137,7 +137,7 @@ def test_timer_omitted_when_no_path(): @_bash_only def test_timer_path_is_quoted_and_script_parses(tmp_path): s = MirrorManager._build_batch_script(**_BASE, timer_path="/p q/t.sh") - assert "nohup '/p q/t.sh' > /dev/null 2>&1 &" in s + assert "bash '/p q/t.sh' --ensure" in s assert _bash_n(s).returncode == 0 diff --git a/tests/test_cli.py b/tests/test_cli.py index a6e438c..3be50bf 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2325,9 +2325,9 @@ def fake_run(argv, *a, **k): assert "SNAPSHOT_DIR=/local/job7/mirrors/sample\n" in rendered[0] write_cmd, start_cmd = ssh_cmds[-2][-1], ssh_cmds[-1][-1] - assert "slurm-timer-sample.sh" in write_cmd - assert "pkill -u \"$USER\" -f '[s]lurm-timer-sample.sh'" in start_cmd - assert 'nohup "$HOME/.cache/sucoder/"slurm-timer-sample.sh' in start_cmd + assert "mktemp" in write_cmd and "mv" in write_cmd + assert "slurm-timer-" in start_cmd and "--ensure" in start_cmd + assert "pkill" not in start_cmd def test_build_executor_confined_no_local_disk_override_wins(tmp_path, monkeypatch): diff --git a/tests/test_mirror.py b/tests/test_mirror.py index 09806f1..4f6e818 100644 --- a/tests/test_mirror.py +++ b/tests/test_mirror.py @@ -1721,10 +1721,10 @@ def test_launch_confined_stages_and_starts_deadline_timer(tmp_path, monkeypatch) assert len(writes) == 2, "batch script then timer script must both be staged" batch, timer = writes[0], writes[1] timer_path = [t for t in timer["args"][2].split() if "slurm-timer-" in t][0] - assert timer_path.endswith("/.cache/sucoder/slurm-timer-sample.sh") + assert "/.cache/sucoder/slurm-timer-sample-" in timer_path assert "chmod 700" in timer["args"][2] # The batch body starts exactly that file, after the session check. - assert f"nohup {timer_path} > /dev/null 2>&1 &" in batch["input"] + assert f"bash {timer_path} --ensure" in batch["input"] # Confined specifics threaded through: runtime job id, dedicated socket, # the mirror as snapshot dir, the configured cadence. assert 'JOB="${SLURM_JOB_ID:-}"' in timer["input"] diff --git a/tests/test_remote.py b/tests/test_remote.py index 9e467e9..a151a48 100644 --- a/tests/test_remote.py +++ b/tests/test_remote.py @@ -1,3 +1,4 @@ +import subprocess """Tests for remote execution: config parsing, session, tunnel, and RemoteExecutor.""" import os @@ -1385,149 +1386,42 @@ def fake_run_agent(args, **kwargs): assert "ControlPath" in env["GIT_SSH_COMMAND"] -def test_ensure_remote_clone_mirror_exists_skips_init( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - """When the remote mirror already exists, ensure_remote_clone skips git init.""" - from sucoder.executor import CommandResult - - manager = _build_remote_manager(tmp_path) - ctx = manager.context_for("rproj") +def test_ensure_remote_clone_mirror_exists_skips_init(tmp_path, monkeypatch): + manager, ctx, path = _local_bootstrap_manager(tmp_path, monkeypatch) + subprocess.run(["git", "clone", str(ctx.canonical_path), str(path)], check=True, capture_output=True) + subprocess.run(["git", "-C", str(path), "config", "review.keep", "yes"], check=True) + assert manager.ensure_remote_clone(ctx) + value = subprocess.check_output(["git", "-C", str(path), "config", "review.keep"], text=True) + assert value.strip() == "yes" - agent_calls: list = [] - - def fake_run_agent(args, **kwargs): - agent_calls.append(list(args)) - # All calls succeed → mirror exists and is valid - return CommandResult(list(args), list(args), "", "", 0) - - monkeypatch.setattr(manager.executor, "run_agent", fake_run_agent) - # The mirror exists and reads fine; the pull verdict is not what - # this test is about, and the real one would hit the network. - monkeypatch.setattr(manager, "_pull_from_remote", lambda ctx: True) - # Mock _sync_remote since we don't want actual sync - sync_called = [] - monkeypatch.setattr(manager, "_sync_remote", lambda ctx: sync_called.append(True)) +def test_ensure_remote_clone_pushes_to_genuinely_empty_mirror(tmp_path, monkeypatch): + manager, ctx, path = _local_bootstrap_manager(tmp_path, monkeypatch) + monkeypatch.setattr(manager, "_pull_from_remote", lambda ctx: pytest.fail("fresh repo fetched")) + assert manager.ensure_remote_clone(ctx) + assert (path / "README.md").read_text() == "hi\n" - manager.ensure_remote_clone(ctx) - # Should have rev-parse check and config fixup, but NOT git init - all_cmds = [" ".join(str(a) for a in c) for c in agent_calls] - assert any("rev-parse" in cmd for cmd in all_cmds) - assert not any("git init" in cmd for cmd in all_cmds) - # Sync should still be called - assert sync_called - - -def test_ensure_remote_clone_pushes_to_genuinely_empty_mirror( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - """First-time bootstrap is not blocked by the unverified-mirror veto. - - Runs the *real* _pull_from_remote against a remote that has no repo - at all: the fetch fails, but the content probe confirms there are no - commits, so the push must proceed without --allow-unverified-mirror. - Guards the false positive that would break every fresh setup. - """ - from sucoder.executor import CommandResult +def test_ensure_remote_clone_refuses_push_over_unverified_mirror(tmp_path, monkeypatch): from sucoder.mirror import MirrorError - - manager = _build_remote_manager(tmp_path) - ctx = manager.context_for("rproj") - - def fake_remote(args, **kwargs): - s = " ".join(str(a) for a in args) - if "echo" in s: - return CommandResult(list(args), list(args), "/home/ligon\n", "", 0) - # No repo, hence no HEAD and no base branch: an empty mirror. - if "rev-parse" in s: - return CommandResult(list(args), list(args), "", "fatal", 1) - return CommandResult(list(args), list(args), "", "", 0) - - monkeypatch.setattr(manager.executor, "run_agent", fake_remote) - if hasattr(manager.executor, "run_on_login_node"): - monkeypatch.setattr(manager.executor, "run_on_login_node", fake_remote) - # The fetch fails the way an empty remote's does. - monkeypatch.setattr( - manager.executor, "run_human", - lambda args, **kw: _transport_result( - args, 128, "fatal: couldn't find remote ref main"), - ) - - pushed: list = [] - monkeypatch.setattr(manager, "_sync_remote", lambda ctx: pushed.append(True)) - - manager.ensure_remote_clone(ctx) # must NOT raise - - assert pushed == [True], "bootstrap over an empty mirror was blocked" - - -def test_ensure_remote_clone_refuses_push_over_unverified_mirror( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - """Bootstrap honours the pull veto; the flag is the escape hatch.""" - from sucoder.executor import CommandResult - from sucoder.mirror import MirrorError - - manager = _build_remote_manager(tmp_path) - ctx = manager.context_for("rproj") - - monkeypatch.setattr( - manager.executor, "run_agent", - lambda args, **kw: CommandResult(list(args), list(args), "", "", 0), - ) - # The mirror could not be read and may hold commits. + manager, ctx, path = _local_bootstrap_manager(tmp_path, monkeypatch) + subprocess.run(["git", "clone", str(ctx.canonical_path), str(path)], check=True, capture_output=True) monkeypatch.setattr(manager, "_pull_from_remote", lambda ctx: False) - - pushed: list = [] + pushed = [] monkeypatch.setattr(manager, "_sync_remote", lambda ctx: pushed.append(True)) - with pytest.raises(MirrorError, match="Refusing to push"): manager.ensure_remote_clone(ctx) - assert not pushed, "bootstrap force-pushed over an unverified mirror" - + assert not pushed manager.ensure_remote_clone(ctx, allow_unverified_mirror=True) assert pushed == [True] -def test_ensure_remote_clone_mirror_not_exists_inits_and_syncs( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - """When the remote mirror does NOT exist, ensure_remote_clone inits then syncs.""" - from sucoder.executor import CommandResult - - manager = _build_remote_manager(tmp_path) - ctx = manager.context_for("rproj") - - agent_calls: list = [] - call_counter = [0] - - def fake_run_agent(args, **kwargs): - agent_calls.append({"args": list(args), "kwargs": kwargs}) - call_counter[0] += 1 - # First call is rev-parse → fail; also $HOME query needs to work - args_str = " ".join(str(a) for a in args) - if "rev-parse" in args_str and call_counter[0] <= 2: - return CommandResult(list(args), list(args), "", "", 1) - if "echo" in args_str: - return CommandResult(list(args), list(args), "/home/testuser\n", "", 0) - return CommandResult(list(args), list(args), "", "", 0) - - monkeypatch.setattr(manager.executor, "run_agent", fake_run_agent) - # Freshly re-inited mirror; see sibling test for the pull verdict. - monkeypatch.setattr(manager, "_pull_from_remote", lambda ctx: True) - - sync_called = [] - monkeypatch.setattr(manager, "_sync_remote", lambda ctx: sync_called.append(True)) - - manager.ensure_remote_clone(ctx) - - # Should have rev-parse, rm, mkdir, git init, and git config calls - all_cmds = [" ".join(str(a) for a in c["args"]) for c in agent_calls] - assert any("rev-parse" in cmd for cmd in all_cmds) - assert any("init" in cmd for cmd in all_cmds) - assert sync_called +def test_ensure_remote_clone_mirror_not_exists_inits_and_syncs(tmp_path, monkeypatch): + manager, ctx, path = _local_bootstrap_manager(tmp_path, monkeypatch) + assert not path.exists() + assert manager.ensure_remote_clone(ctx) + assert (path / ".git").is_dir() + assert (path / "README.md").exists() def _remote_exec_with_scaffolding(tmp_path: Path): @@ -1615,51 +1509,14 @@ def test_remote_git_env_debug_preserves_verbosity( assert "ConnectTimeout=10" in cmd -def test_ensure_remote_clone_rebuilds_empty_mirror( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - """A husk repo (exists, but no commits/base branch) is rebuilt. - - Regression guard for the PlayPen/DTN failure: a remote mirror that - was `git init`'d by a prior failed bootstrap but never received a - push has no `main` ref. ensure_remote_clone must rebuild it rather - than sync into the half-dead repo. - """ - from sucoder.executor import CommandResult - - manager = _build_remote_manager(tmp_path) - ctx = manager.context_for("rproj") - - agent_calls: list = [] - - def fake_run_agent(args, **kwargs): - agent_calls.append(list(args)) - s = " ".join(str(a) for a in args) - if "echo" in s: - return CommandResult(list(args), list(args), "/home/ligon\n", "", 0) - if "rev-parse" in s and "--git-dir" in s: - # Repo exists on disk. - return CommandResult(list(args), list(args), ".git\n", "", 0) - if "rev-parse" in s and ("HEAD" in s or "refs/heads/" in s): - # No commits, no base branch → husk. - return CommandResult(list(args), list(args), "", "fatal", 1) - return CommandResult(list(args), list(args), "", "", 0) - - monkeypatch.setattr(manager.executor, "run_agent", fake_run_agent) - # Isolate the rebuild decision: don't touch the network. True = - # "mirror read successfully"; the verdict itself is covered by - # test_ensure_remote_clone_refuses_push_over_unverified_mirror. - monkeypatch.setattr(manager, "_pull_from_remote", lambda ctx: True) - sync_called: list = [] - monkeypatch.setattr(manager, "_sync_remote", lambda ctx: sync_called.append(True)) - - manager.ensure_remote_clone(ctx) - - cmds = [" ".join(str(a) for a in c) for c in agent_calls] - # Husk detected → wiped and re-initialised before syncing. - assert any("rm -rf" in c for c in cmds) - assert any("git init" in c for c in cmds) - assert sync_called +def test_ensure_remote_clone_rebuilds_empty_mirror(tmp_path, monkeypatch): + """A previous failed bootstrap is now recovered in place, never wiped.""" + manager, ctx, path = _local_bootstrap_manager(tmp_path, monkeypatch) + path.mkdir() + subprocess.run(["git", "-C", str(path), "init", "-b", "main"], check=True, capture_output=True) + subprocess.run(["git", "-C", str(path), "config", "review.keep", "yes"], check=True) + assert manager.ensure_remote_clone(ctx) + assert subprocess.check_output(["git", "-C", str(path), "config", "review.keep"], text=True).strip() == "yes" def test_git_transports_login_first_then_dtn( @@ -2386,3 +2243,11 @@ def test_forward_x11_explicit_defaults_off() -> None: """The explicit marker is independent of forward_x11 itself.""" executor = _make_remote_executor(forward_x11=True) assert executor.forward_x11_explicit is False + + +def _local_bootstrap_manager(tmp_path, monkeypatch): + manager = _build_remote_manager(tmp_path) + path = tmp_path / "remote-bootstrap" + monkeypatch.setattr(manager, "_resolve_remote_path", lambda ctx: str(path)) + monkeypatch.setattr(manager, "_git_transports", lambda ctx: [("local-test", str(path), None)]) + return manager, manager.context_for("rproj"), path diff --git a/tests/test_slurm_timer_script.py b/tests/test_slurm_timer_script.py index ae5ba45..40c171d 100644 --- a/tests/test_slurm_timer_script.py +++ b/tests/test_slurm_timer_script.py @@ -67,7 +67,7 @@ def test_confined_mode_reads_job_id_at_runtime_and_threads_socket(): def test_state_files_are_per_mirror_and_legacy_warn_kept(): s = _render(mirror_token="alpha") assert 'WARN_FILE="$STATE_DIR/slurm-deadline-$MIRROR_TOKEN.warn"' in s - assert 'LEGACY_WARN_FILE="$STATE_DIR/slurm-deadline.warn"' in s + assert 'LEGACY_WARN_FILE="$CACHE_DIR/slurm-deadline.warn"' in s assert "MIRROR_TOKEN=alpha\n" in s for n in (5, 15, 30): assert f'WARN{n}="$STATE_DIR/.slurm-warn-{n}-$MIRROR_TOKEN"' in s diff --git a/tests/test_startup_regressions.py b/tests/test_startup_regressions.py new file mode 100644 index 0000000..7a6194c --- /dev/null +++ b/tests/test_startup_regressions.py @@ -0,0 +1,163 @@ +"""Exercise startup against real files/processes, not success-only mocks.""" +import subprocess +from pathlib import Path + +import pytest + +from sucoder.executor import CommandResult +from sucoder.mirror import MirrorError, MirrorManager +from sucoder.remote_bootstrap import INITIALIZE_MIRROR_SH +from tests.test_remote import _build_remote_manager + + +def git(path, *args): + return subprocess.run(["git", "-C", str(path), *args], check=True, + capture_output=True, text=True).stdout.strip() + + +@pytest.fixture +def remote(tmp_path, monkeypatch): + manager = _build_remote_manager(tmp_path) + path = tmp_path / "remote" + monkeypatch.setattr(manager, "_resolve_remote_path", lambda ctx: str(path)) + monkeypatch.setattr(manager, "_git_transports", lambda ctx: [("test", str(path), None)]) + return manager, manager.context_for("rproj"), path + + +def test_first_publication_has_no_fetch_or_force(remote, monkeypatch): + manager, ctx, path = remote + calls = [] + original = manager.executor.run_human + + def record(args, **kwargs): + calls.append(args) + return original(args, **kwargs) + + monkeypatch.setattr(manager.executor, "run_human", record) + assert manager.ensure_remote_clone(ctx) + assert (path / "README.md").read_text() == "hi\n" + assert not any(c[:2] == ["git", "fetch"] for c in calls) + assert not any("--force" in c or c[:2] == ["git", "reset"] for c in calls) + + +@pytest.mark.parametrize("rc,stdout", [(255, ""), (128, ""), (0, "unexpected"), + (1, "SUCODER_MIRROR_CREATED")]) +def test_failed_initialization_cannot_authorize_mutation(remote, monkeypatch, rc, stdout): + manager, ctx, path = remote + path.mkdir() + (path / "precious").write_text("keep") + calls = [] + + def failed(args, **kwargs): + calls.append(args) + return CommandResult(args, args, stdout, "probe failed", rc) + + monkeypatch.setattr(manager.executor, "run_agent", failed) + with pytest.raises(MirrorError, match="safely initialize"): + manager.ensure_remote_clone(ctx, allow_unverified_mirror=True) + assert len(calls) == 1 + assert (path / "precious").read_text() == "keep" + + +@pytest.mark.parametrize("kind", ["files", "broken", "ancestor", "symlink", "bare"]) +def test_existing_unknown_directories_survive(remote, kind): + manager, ctx, path = remote + path.mkdir() + if kind == "broken": + (path / ".git").mkdir() + elif kind == "ancestor": + git(path.parent, "init", "-b", "main") + elif kind == "symlink": + target = path.with_name("target") + path.rename(target) + path.symlink_to(target, target_is_directory=True) + elif kind == "bare": + git(path, "init", "--bare") + (path / "precious").write_text("keep") + with pytest.raises(MirrorError): + manager.ensure_remote_clone(ctx) + assert (path / "precious").read_text() == "keep" + + +def test_empty_repository_is_recovered_without_deletion(remote): + manager, ctx, path = remote + path.mkdir() + git(path, "init", "-b", "main") + git(path, "config", "review.preserve", "yes") + assert manager.ensure_remote_clone(ctx) + assert git(path, "config", "review.preserve") == "yes" + assert (path / "README.md").exists() + + +def test_unborn_head_with_feature_branch_is_not_empty(remote): + manager, ctx, path = remote + subprocess.run(["git", "clone", str(ctx.canonical_path), str(path)], check=True, + capture_output=True) + git(path, "branch", "-m", "feature") + git(path, "symbolic-ref", "HEAD", "refs/heads/unborn") + before = git(path, "rev-parse", "feature") + with pytest.raises(MirrorError, match="Refusing to push"): + manager.ensure_remote_clone(ctx) + assert git(path, "rev-parse", "feature") == before + assert (path / "README.md").exists() + + +def test_missing_ref_with_disconnect_tail_is_not_transport_failure(): + result = CommandResult([], [], "", "shell startup noise\n" + "fatal: couldn't find remote ref main\n" + "fatal: the remote end hung up unexpectedly\n", 128) + assert not MirrorManager._is_transport_failure(result) + assert MirrorManager._short_git_error(result) == "fatal: couldn't find remote ref main" + + +def test_failed_status_cannot_authorize_push(remote, monkeypatch): + manager, ctx, path = remote + path.mkdir() + git(path, "init", "-b", "main") + (path / "precious").write_text("keep") + original = manager.executor.run_agent + + def fail_status(args, **kwargs): + if args[:2] == ["git", "status"]: + return CommandResult(args, args, "", "connection closed", 255) + return original(args, **kwargs) + + monkeypatch.setattr(manager.executor, "run_agent", fail_status) + monkeypatch.setattr(manager, "_sync_remote", lambda *a, **kw: pytest.fail("unsafe push")) + with pytest.raises(MirrorError, match="Could not inspect remote working tree"): + manager.ensure_remote_clone(ctx, allow_unverified_mirror=True) + assert (path / "precious").read_text() == "keep" + + +def test_timeout_cannot_authorize_initialization(remote, monkeypatch): + from sucoder.executor import CommandError + manager, ctx, path = remote + + def timeout(args, **kwargs): + raise CommandError("timeout", CommandResult(args, args, "", "timed out", -1)) + + monkeypatch.setattr(manager.executor, "run_agent", timeout) + with pytest.raises(CommandError): + manager.ensure_remote_clone(ctx) + assert not path.exists() + + +def test_fresh_push_cannot_overwrite_concurrent_commit(remote, monkeypatch): + manager, ctx, path = remote + original = manager._sync_remote + remote_commit = [] + + def concurrent_writer(ctx, **kwargs): + git(path, "config", "user.name", "Concurrent writer") + git(path, "config", "user.email", "test@example.com") + (path / "precious").write_text("keep") + git(path, "add", "precious") + git(path, "commit", "-m", "concurrent work") + remote_commit.append(git(path, "rev-parse", "HEAD")) + return original(ctx, **kwargs) + + monkeypatch.setattr(manager, "_sync_remote", concurrent_writer) + with pytest.raises(MirrorError, match="Failed to push"): + manager.ensure_remote_clone(ctx) + assert git(path, "rev-parse", "HEAD") == remote_commit[0] + assert (path / "precious").read_text() == "keep" diff --git a/tests/test_timer_lifecycle.py b/tests/test_timer_lifecycle.py new file mode 100644 index 0000000..aca59e0 --- /dev/null +++ b/tests/test_timer_lifecycle.py @@ -0,0 +1,161 @@ +"""Run the generated watchdog and its starter under real bash/flock.""" +import os +import shutil +import signal +import subprocess +import time +from types import SimpleNamespace + +import pytest + +from sucoder.slurm_timer import build_timer_script, timer_identity + + +@pytest.fixture +def timers(tmp_path): + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + for name in ("bash", "flock", "hostname", "mkdir", "chmod", "cat", "rm", + "nohup", "sleep", "tail", "seq", "touch", "git", "mktemp", "date"): + executable = shutil.which(name) + if not executable: + pytest.skip(f"requires {name}") + (bin_dir / name).symlink_to(executable) + for name, body in (("tmux", "exit 0"), ("squeue", "echo 02:00:00")): + path = bin_dir / name + path.write_text("#!/bin/sh\n" + body + "\n") + path.chmod(0o700) + env = dict(os.environ, HOME=str(tmp_path), PATH=str(bin_dir)) + clients = [] + + def start(target="savio", job=12, confined=False, snapshot_dir=None): + scope = timer_identity("example", target) + script = tmp_path / f"timer-{scope}-{job}.sh" + if not script.exists(): + script.write_text(build_timer_script( + mirror_token="example", tmux_session="sucoder-example", + job_id=None if confined else job, + timer_scope=scope, tmux_socket="test" if confined else None, + snapshot_dir=str(snapshot_dir) if snapshot_dir else None, + )) + child_env = dict(env, SLURM_JOB_ID=str(job)) + child = subprocess.Popen(["bash", str(script), "--ensure"], env=child_env, + start_new_session=True, stdout=subprocess.PIPE, + stderr=subprocess.PIPE, text=True) + clients.append(child) + return child + + yield SimpleNamespace(start=start, root=tmp_path, bin=bin_dir, env=env) + # Only groups created by these tests; never pkill by name or a saved PID. + for child in clients: + try: + os.killpg(child.pid, signal.SIGTERM) + except ProcessLookupError: + pass + child.wait(timeout=5) + + +def finish(child): + out, err = child.communicate(timeout=15) + assert child.returncode == 0, err + return out + + +@pytest.mark.parametrize("confined", [False, True]) +def test_timer_survives_starter_and_reuses_owner(timers, confined): + assert "STARTED" in finish(timers.start(confined=confined)) + owner = next(timers.root.glob(".cache/sucoder/timers/*/*/owner")) + identity = owner.read_text() + assert "REUSED" in finish(timers.start(confined=confined)) + assert owner.read_text() == identity + os.kill(int(identity.split()[0]), 0) + + +def test_concurrent_starters_create_one_timer(timers): + a, b = timers.start(), timers.start() + outputs = [finish(a), finish(b)] + assert sum("STARTED" in out for out in outputs) == 1 + assert sum("REUSED" in out for out in outputs) == 1 + assert len(list(timers.root.glob(".cache/sucoder/timers/*/*/owner"))) == 1 + + +def test_target_and_allocation_have_distinct_timers(timers): + for target, job in [("savio", 12), ("savio-htc", 12), ("savio", 13)]: + assert "STARTED" in finish(timers.start(target, job)) + owners = list(timers.root.glob(".cache/sucoder/timers/*/*/owner")) + assert len({p.read_text() for p in owners}) == 3 + assert timer_identity("a/b", None) != timer_identity("a_b", None) + + +def test_dead_owner_metadata_does_not_signal_unrelated_process(timers): + node = subprocess.check_output(["hostname"], text=True).strip() + state = timers.root / ".cache/sucoder/timers" / timer_identity("example", "savio") / f"{node}-12" + state.mkdir(parents=True) + # The current test runner PID is intentionally paired with a wrong birth + # time. No lock exists, so a fresh timer must replace this stale record. + (state / "owner").write_text(f"{os.getpid()} 0\n") + (state / "status").write_text("monitoring\n") + assert "STARTED" in finish(timers.start()) + assert (state / "owner").read_text().split()[0] != str(os.getpid()) + + +def test_missing_executable_reports_failure(timers): + (timers.bin / "squeue").unlink() + child = timers.start() + out, err = child.communicate(timeout=15) + assert child.returncode != 0 + assert "squeue unavailable" in err + assert "STARTED" not in out + + +def test_watchdog_warns_and_snapshots_without_changing_index(timers): + origin, work = timers.root / "origin", timers.root / "work" + + def git(*args): + return subprocess.check_output(["git", *args], text=True, stderr=subprocess.DEVNULL).strip() + + git("init", "--bare", str(origin)) + git("init", "-b", "main", str(work)) + git("-C", str(work), "config", "user.name", "Test") + git("-C", str(work), "config", "user.email", "test@example.com") + (work / "tracked").write_text("base") + git("-C", str(work), "add", ".") + git("-C", str(work), "commit", "-m", "base") + git("-C", str(work), "remote", "add", "origin", str(origin)) + git("-C", str(work), "push", "origin", "main") + (work / "tracked").write_text("staged") + git("-C", str(work), "add", ".") + (work / "tracked").write_text("working") + (work / "untracked").write_text("new") + before = git("-C", str(work), "write-tree") + (timers.bin / "squeue").write_text("#!/bin/sh\necho 00:04:00\n") + assert "STARTED" in finish(timers.start(snapshot_dir=work)) + deadline = time.monotonic() + 5 + ref = "refs/sucoder/wip/example" + while time.monotonic() < deadline: + check = subprocess.run(["git", "-C", str(origin), "rev-parse", "--verify", ref], + capture_output=True) + if check.returncode == 0: + break + time.sleep(0.02) + assert check.returncode == 0 + assert git("-C", str(origin), "show", f"{ref}:tracked") == "working" + assert git("-C", str(origin), "show", f"{ref}:untracked") == "new" + assert git("-C", str(work), "write-tree") == before + warning = next(timers.root.glob(".cache/sucoder/timers/*/*/slurm-deadline-*.warn")) + assert "Commit and save NOW" in warning.read_text() + + +def test_timer_ssh_timeout_is_advisory(monkeypatch, caplog): + import logging + from sucoder import cli + + def timeout(args, **kwargs): + raise subprocess.TimeoutExpired(args, 30) + + monkeypatch.setattr(subprocess, "run", timeout) + session = SimpleNamespace(mirror_name="test", slurm_job_id=1, compute_node="node") + control = SimpleNamespace(ssh_options=lambda **kw: []) + cli._start_slurm_timer(session, control, control, logging.getLogger("timer-test")) + assert "exit -1" in caplog.text + assert "snapshots are unavailable" in caplog.text From 5af62bd32e6657e018329b6b0c5c21af2c30dde4 Mon Sep 17 00:00:00 2001 From: Sue the Coder Date: Sat, 12 Sep 2026 08:52:02 +0000 Subject: [PATCH 6/9] ledger: anchor PR 13 integration on existing timer supervision Preserve the readiness handshake and allocation locks from 258accb while carrying forward the warning-loop fixes. Scheduler failures remain unknown state. Co-Authored-By: GPT-6 --- .coder/ledger.md | 58 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 .coder/ledger.md diff --git a/.coder/ledger.md b/.coder/ledger.md new file mode 100644 index 0000000..7c66815 --- /dev/null +++ b/.coder/ledger.md @@ -0,0 +1,58 @@ +# Prior-Art Ledger - Slurm timer integration + +Search tier: GitNexus query/context/impact plus source and test inspection. +Baseline: PR #13 (`2c24354`) and local main (`258accb`). + +## 1. Task + +Integrate PR #13's deadline warnings and regression coverage with main's +timer supervision. Keep monitoring through scheduler errors. + +## 2. Existing machinery + +| Machinery | Source at baseline | Tests | Decision | +|---|---|---|---| +| SSH timer staging and readiness | `sucoder/cli.py:1129` on main | `tests/test_timer_lifecycle.py::test_timer_ssh_timeout_is_advisory` | Reuse main | +| Allocation-scoped locks and readiness handshake | `sucoder/timer_lifecycle.py:3` on main | `test_timer_survives_starter_and_reuses_owner`, `test_concurrent_starters_create_one_timer`, `test_missing_executable_reports_failure` | Reuse main | +| Confined timer launch | `sucoder/mirror.py:2391` on main | `tests/test_batch_script.py`, `tests/test_mirror.py::test_launch_confined_stages_and_starts_deadline_timer` | Keep `bash --ensure`; extend atomic staging | +| Shared warning loop and builder | `sucoder/slurm_timer.py:102` on PR #13 | `tests/test_slurm_timer_script.py` warning-chain tests | Extend scheduler error handling | +| Snapshot implementation | `sucoder/slurm_timer.py:65` on PR #13 | Real repository-pair tests and `test_watchdog_warns_and_snapshots_without_changing_index` | Reuse | +| Scheduler observation contract | `sucoder/cli.py:765` on PR #13 | Slurm state-query tests in `tests/test_cli.py` | Reuse distinction between errors and empty successful queries | + +## 3. Definitions and conventions + +- Scheduler errors are unknown state: "An ssh/squeue failure is NOT evidence + the job is dead" (`sucoder/cli.py:765`, PR #13). +- Successful empty queries indicate disappearance; retain PR #13's three + consecutive observations before ending the watchdog. +- `snapshot_minutes` is a periodic cadence; zero disables periodic snapshots, + while threshold snapshots remain enabled (`build_timer_script` docstring). +- `--ensure` reports `SUCODER_TIMER_STARTED` or `SUCODER_TIMER_REUSED`; a + successful SSH return alone does not establish readiness. + +## 4. Invariants + +- Preserve main's target/node/allocation locks, immutable SSH script names, + readiness diagnostics, and advisory timer failures. Do not restore `pkill`. +- Never cancel the user's allocation from the timer. +- Preserve the agent's real Git index and existing snapshot implementation. +- Warning urgency only increases; skipped thresholds must not fire later. +- Scheduler failures must not stop monitoring or suppress periodic snapshots. +- Keep unrelated startup safety fixes from `258accb` intact. + +## 5. Reuse decisions + +- Reuse main's supervision rather than implementing another startup check. +- Extend the existing warning loop and bash test driver to distinguish failed + queries from successful empty queries, including recovery and snapshot tests. +- Retain PR #13's confined atomic staging, warning order, lifecycle wording, + and mutation-derived regression tests; adapt assertions to supervision. +- Keep compatibility warning files cleared when a new watchdog starts. + +## 6. Open questions + +No implementation decision is blocked. Cluster smoke testing and issue #15's +batch-environment Git availability need a real allocation. Issues #14 and #16 +remain outside this integration. + +Prepared by Sue (2026-09-12). From fe3845f9bcdccc87c813444f534e92cc6c6f0983 Mon Sep 17 00:00:00 2001 From: Sue the Coder Date: Sat, 12 Sep 2026 08:57:35 +0000 Subject: [PATCH 7/9] ledger: record timer integration verification Verified readiness and startup machinery is preserved, scheduler failures remain unknown state, and 759 tests pass. Co-Authored-By: GPT-6 --- .coder/ledger.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.coder/ledger.md b/.coder/ledger.md index 7c66815..354e3f5 100644 --- a/.coder/ledger.md +++ b/.coder/ledger.md @@ -49,6 +49,17 @@ timer supervision. Keep monitoring through scheduler errors. and mutation-derived regression tests; adapt assertions to supervision. - Keep compatibility warning files cleared when a new watchdog starts. +Verification: OK against sections 3-5. `python -m pytest -q` passed all 759 +tests on Python 3.12. Four scheduler-error regression cases failed before the +loop fix. A real-file staging test checks the open inode and quoted paths. +The SSH supervisor, lifecycle module, remote bootstrap, and their regression +tests are byte-for-byte unchanged from `258accb`. GitNexus's comparison with +main reports the expected confined-launch and warning-loop scope. + +Reproduce from the repository root with `pytest -q`; the focused checks are +`pytest -q tests/test_slurm_timer_script.py tests/test_timer_lifecycle.py +tests/test_batch_script.py tests/test_cli.py tests/test_mirror.py`. + ## 6. Open questions No implementation decision is blocked. Cluster smoke testing and issue #15's From 81dd48d93ed47853d3a2d0907ca5b2efc2ff3966 Mon Sep 17 00:00:00 2001 From: Sue the Coder Date: Sat, 12 Sep 2026 09:31:09 +0000 Subject: [PATCH 8/9] docs: hand off PR 13 cluster validation and batch Git probe Archive the pending harness bootstrap unchanged. Document the tested revision, native smoke checks, issue 15 batch-environment probe, and required evidence. Shell snippets pass bash -n; no code changes. Co-Authored-By: GPT-6 --- .sucoder/handoff.org | 298 ++++++++---------- .../2026-08-25-savio-harness-bootstrap.org | 191 +++++++++++ 2 files changed, 324 insertions(+), 165 deletions(-) create mode 100644 .sucoder/handoffs/2026-08-25-savio-harness-bootstrap.org diff --git a/.sucoder/handoff.org b/.sucoder/handoff.org index 83e9134..267ff7e 100644 --- a/.sucoder/handoff.org +++ b/.sucoder/handoff.org @@ -1,191 +1,159 @@ -#+title: Savio user-level harness bootstrap -#+date: 2026-08-25 +#+title: PR 13 cluster validation and issue 15 +#+date: 2026-09-12 #+status: READY +#+author: Sue +#+property: header-args :eval never-export :tangle no :results output :exports code -* Mission +* Mission and baseline -Install and validate SuCoder's supported harnesses in the Savio account -=ligon= without root access. Work only in user-owned locations under -=$HOME=, preserve existing configuration, and leave a reproducible report in -this file. +Validate [[https://github.com/ligon/sucoder/pull/13][PR 13]] on a real Slurm +allocation and settle [[https://github.com/ligon/sucoder/issues/15][issue 15's]] +batch-environment Git availability. This is validation, not harness +installation. The earlier unfinished task is preserved at +[[file:handoffs/2026-08-25-savio-harness-bootstrap.org][the archived bootstrap handoff]]. -This is an empirical bootstrap pass. Do not redesign SuCoder unless a real -cluster-side incompatibility requires it; record such an incompatibility for a -follow-up instead. +Code baseline: =fe3845f9bcdccc87c813444f534e92cc6c6f0983= on +=fix/slurm-timer-hardening=. Fetch the current PR head, which also contains +this handoff, and record its SHA. Do not assume GitHub main includes it. +Read =AGENTS.md= and =.coder/ledger.md= before making changes. -* Important architecture +Both launch paths use =bash --ensure=, allocation-scoped locks, and +readiness responses. Failed scheduler queries leave monitoring and periodic +snapshots running. Three consecutive successful empty queries establish +disappearance. Preserve =258accb='s supervision and startup safety. -- Remote SuCoder sessions intentionally run as the SSH user =ligon=. There is - no remote =coder= account and no remote sudo boundary. SSH/SLURM and the - remote mirror provide the isolation boundary. -- SuCoder itself runs on the laptop. It does not need to be installed on - Savio. Only harness binaries and their non-secret user configuration belong - on the cluster. -- Do *not* copy =~/.sucoder/config.yaml=, the password store, GPG keys, or an - OpenRouter API key to Savio. The laptop-side SuCoder process reads the key - from =pass= and stages it transiently into the remote harness environment. -- Never print an API key, inspect its value, put it in argv, or write it to a - cluster configuration file. -- =$HOME= is shared across Savio login and compute nodes. Install there, not - in a node-local directory. +* Already verified -* Entry checks and stop conditions +- All 759 tests passed locally on Python 3.12. +- Python 3.9, Python 3.11, and docs CI passed at =fe3845f= (run =34684599185=). +- Real bash/flock tests cover concurrent starters, owner reuse, startup + failure, snapshots, and preservation of the agent's Git index. +- Scheduler stubs cover repeated errors, partial output on failure, recovery, + and snapshots during outages. No real controller outage was induced. +- A real-file test checks atomic staging, existing readers, and quoted paths. -First read =AGENTS.md= and this entire handoff. Then collect: +These do not establish Slurm cgroups, shared-filesystem locking, or executable +availability in the actual batch environment. + +* Entry checks + +SuCoder runs on the launcher host; the remote session runs as the SSH user, +normally =ligon=. Verify the launcher uses the PR's Python package before +testing generated scripts. A PR checkout only on the cluster does not update +the launcher. Record these facts without dumping credentials or environment: #+begin_src sh -date +git status --short --branch +git rev-parse HEAD +date -u hostname -f +id -un printf 'SLURM_JOB_ID=%s\n' "${SLURM_JOB_ID:-}" nproc -free -h -squeue --me -o '%i %P %T %L %D %C %m' -bash -lc 'printf "HOME=%s\nPATH=%s\n" "$HOME" "$PATH"' +cat /proc/self/cgroup #+end_src -If =SLURM_JOB_ID= is empty, this session is on a login node. Do not run the -installers there. Report that the human should relaunch with =-T savio-htc= -or =-T carleton-htc= and stop. - -Before changing anything, inventory the executable path and version of: -=claude=, =codex=, =aider=, =opencode=, =goose=, =kimi=, =gitnexus=, =uv=, -=node=, and =npm=. Also record =npm config get prefix= and the effective uv -tool directory. - -Known state from a login-shell probe on 2026-08-25: - -- =$HOME= is =/global/home/users/ligon=. -- =claude= 2.1.245 is present at =~/.local/bin/claude= and is the only - installed harness. -- =node= 20.20.2 and npm 10.8.2 are under =~/.nvm=; that npm prefix is - user-writable. -- =uv= 0.11.29 is at =~/.local/bin/uv=. -- =gitnexus= 1.6.3 is installed under the current nvm Node prefix. -- =~/.sucoder/skills= exists and resolves to the shared skills checkout. -- The current environment sets =UV_TOOL_DIR=/tmp/uv-tools-ligon=. This is - unsuitable: =/tmp= may be node-local or disposable. Do not install Aider - there. Find where this setting comes from and propose the smallest safe - correction. Until corrected, override it explicitly with a persistent - tool root under =$HOME=. - -* Installation policy - -- Install the latest *stable* releases from official upstream sources. Do not - install beta/nightly builds. -- Prefer isolated or native distributions. Do not install Python tools into - system Python 3.6 or a project environment. -- Keep executable shims in =~/.local/bin= or the existing user-owned nvm - prefix. A fresh =bash -lc= must resolve every harness without manual setup. -- Download installer scripts from official locations, inspect their relevant - install-path behavior before execution, and do not use sudo. -- Preserve existing config files. Before editing one, make a timestamped, - mode-preserving backup beside it. Never commit home-directory config or - backups to this repository. -- Do not delete stale installations during this pass. Report shadowed or - duplicate copies and recommend cleanup separately. - -Install these harnesses: - -1. *Aider*: use uv's isolated tool installation with Python 3.12 and - =aider-chat@latest=. Explicitly set a persistent =UV_TOOL_DIR= under - =~/.local/share/uv= and =UV_TOOL_BIN_DIR=$HOME/.local/bin= for the command. -2. *Codex*: install the stable =@openai/codex= package using the existing - user-owned npm prefix, unless the current official standalone installer is - materially better on this host. -3. *OpenCode*: install stable OpenCode 1 (=opencode-ai=, executable - =opencode=). Do not substitute the OpenCode 2 beta executable - =opencode2=; SuCoder's current profile targets =opencode=. -4. *Goose*: install the CLI, not the desktop application, using the official - =aaif-goose/goose= stable installer with configuration disabled during - install. Its executable should land in =~/.local/bin=. -5. *Kimi*: install current stable *Kimi Code CLI* from MoonshotAI's official - installer. Prefer its current native/single-binary distribution rather - than adding a Node-version dependency merely for installation. Confirm - that =kimi --help= still provides the flags SuCoder uses: =--agent-file=, - =--auto=, =--add-dir=, and model/provider support. - -Inspect the existing Claude and GitNexus installations. Update them only via -their native/official update path when a newer stable release is actually -available. Do not replace a working install merely to make all tools use the -same package manager. - -Official starting points (verify them at execution time): - -- Aider: https://aider.chat/docs/install.html -- Codex: https://github.com/openai/codex -- OpenCode: https://opencode.ai/docs/ -- Goose: https://github.com/aaif-goose/goose -- Kimi Code: https://github.com/MoonshotAI/kimi-code - -* Provider and harness configuration - -Configure only non-secret provider metadata on Savio. The target model for -smoke testing is =openrouter/moonshotai/kimi-k3=. - -- Aider and OpenCode should consume =OPENROUTER_API_KEY= from the launch - environment and accept the provider-prefixed model name. -- Native Kimi is adapted by SuCoder through temporary =KIMI_MODEL_*= variables; - do not persist the key in Kimi's config. -- Goose natively recognizes =OPENROUTER_API_KEY=, but it may also require - non-secret provider/model selection (=openrouter= and - =moonshotai/kimi-k3=). Configure that metadata without a key. If SuCoder's - generic =--model openrouter/...= spelling does not match Goose's CLI, record - the exact error as a local SuCoder adapter gap rather than storing a secret - to work around it. -- Codex uses custom model-provider configuration for OpenAI-compatible - endpoints. Add a key-free OpenRouter provider stanza only if current Codex - supports sourcing its key from =OPENROUTER_API_KEY=. If SuCoder's model - prefix needs translation for Codex, record the exact required mapping rather - than embedding credentials. -- Leave Claude's existing authentication and configuration intact. - -* Verification - -After installation, start a fresh login shell and record, for every harness: - -1. =command -v= result; -2. version; -3. whether the path is under =$HOME= and visible on both the allocated compute - node and a login shell; -4. whether its SuCoder-required flags are present; -5. whether native Agent Skills, shell execution, file editing, MCP, and - subagents are available as expected. - -Do not claim provider success from =--version=. Perform a minimal live -OpenRouter smoke test only when the launch environment already contains the -transient key; do not ask the human to paste it into the cluster. Keep API use -small. If this bootstrap Claude session has no OpenRouter environment, leave -the live tests for the laptop-side relaunch commands below. - -The human will validate each completed harness from the laptop with commands -of this form: +Without an allocation, stop before tests/workloads and request a relaunch with +=-T savio-htc= or =-T carleton-htc=. Use an installed harness. Do not install +harnesses or copy launcher configuration, keys, or password stores for this task. + +* Confined launch smoke test + +Use a disposable mirror and the normal launcher path with a short allocation +on the configured shared partition. Record partition, account, QoS, CPU count, +time limit, and local-disk setting. Do not snapshot an active research mirror +as a test fixture. + +1. Confirm the batch body runs =bash --ensure= before the keeper loop. + Record the staged timer path/digest. Check job output for + =SUCODER_TIMER_STARTED= and absence of startup failure diagnostics. +2. Inspect the matching directory under + =~/.cache/sucoder/timers//-/=. Record =owner=, =status=, + and sanitized =timer.log=. Status should reach =monitoring=. +3. On that allocated node, in the same job environment, run + =bash --ensure= twice. Expect + =SUCODER_TIMER_REUSED= with the same owner PID/start-time pair. + From a login node, use the confined job's =srun --overlap= route. +4. Compare =/proc//cgroup= with the job's cgroup. The watchdog must + belong to the allocation and survive detaching the client. +5. Enable local-disk tiering for the disposable mirror. Verify the timer's + snapshot directory is its node-local clone and origin is the shared mirror. + Create small tracked and untracked text changes; record =git write-tree= + before and after a snapshot. Verify =refs/sucoder/wip/= on + the shared origin contains both changes and the agent's index is unchanged. +6. Let the short allocation enter a warning threshold. Check warning files + and WIP refs. Starting below five minutes should yield one urgent warning, + with no subsequent less urgent notices. +7. End only the disposable tmux session. Under sbatch the keeper should exit + and the allocation complete. Exiting just the agent can leave a fallback + shell alive; that is not the same as ending tmux. Do not kill unrelated + sessions or use process-name matching for cleanup. + +If an unconfined target is already available, check SSH readiness and reuse +there too. Otherwise mark it untested; do not reserve a whole node solely for +this checklist. + +* Issue 15: actual batch environment + +An interactive =bash -lc= or =srun= inheriting an already repaired PATH does +not settle this issue. Run this payload directly in a fresh sbatch body, +before any login-shell wrapper or module setup. Use the normal launcher's +submission environment/export policy and configured account/partition/QoS, +one task, and a short time limit. Record the exact submission command. +Do not use =env -i= to manufacture a failure. #+begin_src sh -sucoder -T savio-htc collaborate SuCoder --harness aider --model openrouter/moonshotai/kimi-k3 -sucoder -T savio-htc collaborate SuCoder --harness opencode --model openrouter/moonshotai/kimi-k3 -sucoder -T savio-htc collaborate SuCoder --harness goose --model openrouter/moonshotai/kimi-k3 -sucoder -T savio-htc collaborate SuCoder --harness kimi --model openrouter/moonshotai/kimi-k3 +#!/bin/bash +date -u +hostname -f +printf 'SLURM_JOB_ID=%s\n' "${SLURM_JOB_ID:-}" +for executable in git bash flock nohup tmux squeue; do + if command -v "$executable"; then + printf 'FOUND %s\n' "$executable" + else + printf 'MISSING %s\n' "$executable" + fi +done +if command -v git >/dev/null 2>&1; then + git --version +fi #+end_src -Test Codex separately after confirming its OpenRouter provider mapping. +If Git is missing, preserve the failed lookup and compare a separate +login-shell probe. Identify the existing module/PATH setup supplying Git. +Propose the smallest timer-environment fix and a once-per-watchdog diagnostic +with targeted tests. Follow impact-analysis rules before implementation; do +not silently edit shell startup files or persist secrets. -* Deliverable +If Git is present, record its path/version and a successful snapshot. This +establishes this target's behavior, not every site's. The missing-Git diagnostic +remains a separate hardening question in issue 15. + +* Regression checks and boundaries -Replace the =Outcome= section below with: +Run from the PR checkout root inside the allocation: + +#+begin_src sh +pytest -q tests/test_slurm_timer_script.py tests/test_timer_lifecycle.py tests/test_batch_script.py +#+end_src + +These tests stub scheduler responses; distinguish them from native validation. +Never disrupt the scheduler or other jobs to simulate outages. Issues +[[https://github.com/ligon/sucoder/issues/14][14]] (object retention) and +[[https://github.com/ligon/sucoder/issues/16][16]] (compatibility warning/WIP-ref +collisions) remain outside this task. Do not prune or change permissions. + +* Deliverable -- a table of harness, exact version, resolved path, install/update method, and - smoke-test result; -- any persistent shell/config changes, with backup paths; -- unresolved SuCoder adapter gaps, including exact reproduction commands and - sanitized errors; -- the exact idempotent commands that should become a future target-bootstrap - script; -- a recommendation about the bad =UV_TOOL_DIR=/tmp/uv-tools-ligon= setting. +Replace Outcome with tested SHA, target, node, job ID, timestamp, submission +options, exact commands, and pass/fail/untested results for each step. Include +sanitized logs, Git path/version, snapshot ref/tree IDs, and environment +failures with reproduction commands. -Do not record secrets or secret-derived output. Commit only repository files -that document or implement the bootstrap; never commit files copied from -=$HOME=. +Commit the report on the PR branch and summarize results on PR 13 and issue +15. Mark this handoff COMPLETE when the report distinguishes all remaining +untested items. Sign as Sue. No cluster execution is claimed yet. * Outcome -Pending remote execution. +Pending an agent with cluster access. Prepared by Sue, 2026-09-12. diff --git a/.sucoder/handoffs/2026-08-25-savio-harness-bootstrap.org b/.sucoder/handoffs/2026-08-25-savio-harness-bootstrap.org new file mode 100644 index 0000000..83e9134 --- /dev/null +++ b/.sucoder/handoffs/2026-08-25-savio-harness-bootstrap.org @@ -0,0 +1,191 @@ +#+title: Savio user-level harness bootstrap +#+date: 2026-08-25 +#+status: READY + +* Mission + +Install and validate SuCoder's supported harnesses in the Savio account +=ligon= without root access. Work only in user-owned locations under +=$HOME=, preserve existing configuration, and leave a reproducible report in +this file. + +This is an empirical bootstrap pass. Do not redesign SuCoder unless a real +cluster-side incompatibility requires it; record such an incompatibility for a +follow-up instead. + +* Important architecture + +- Remote SuCoder sessions intentionally run as the SSH user =ligon=. There is + no remote =coder= account and no remote sudo boundary. SSH/SLURM and the + remote mirror provide the isolation boundary. +- SuCoder itself runs on the laptop. It does not need to be installed on + Savio. Only harness binaries and their non-secret user configuration belong + on the cluster. +- Do *not* copy =~/.sucoder/config.yaml=, the password store, GPG keys, or an + OpenRouter API key to Savio. The laptop-side SuCoder process reads the key + from =pass= and stages it transiently into the remote harness environment. +- Never print an API key, inspect its value, put it in argv, or write it to a + cluster configuration file. +- =$HOME= is shared across Savio login and compute nodes. Install there, not + in a node-local directory. + +* Entry checks and stop conditions + +First read =AGENTS.md= and this entire handoff. Then collect: + +#+begin_src sh +date +hostname -f +printf 'SLURM_JOB_ID=%s\n' "${SLURM_JOB_ID:-}" +nproc +free -h +squeue --me -o '%i %P %T %L %D %C %m' +bash -lc 'printf "HOME=%s\nPATH=%s\n" "$HOME" "$PATH"' +#+end_src + +If =SLURM_JOB_ID= is empty, this session is on a login node. Do not run the +installers there. Report that the human should relaunch with =-T savio-htc= +or =-T carleton-htc= and stop. + +Before changing anything, inventory the executable path and version of: +=claude=, =codex=, =aider=, =opencode=, =goose=, =kimi=, =gitnexus=, =uv=, +=node=, and =npm=. Also record =npm config get prefix= and the effective uv +tool directory. + +Known state from a login-shell probe on 2026-08-25: + +- =$HOME= is =/global/home/users/ligon=. +- =claude= 2.1.245 is present at =~/.local/bin/claude= and is the only + installed harness. +- =node= 20.20.2 and npm 10.8.2 are under =~/.nvm=; that npm prefix is + user-writable. +- =uv= 0.11.29 is at =~/.local/bin/uv=. +- =gitnexus= 1.6.3 is installed under the current nvm Node prefix. +- =~/.sucoder/skills= exists and resolves to the shared skills checkout. +- The current environment sets =UV_TOOL_DIR=/tmp/uv-tools-ligon=. This is + unsuitable: =/tmp= may be node-local or disposable. Do not install Aider + there. Find where this setting comes from and propose the smallest safe + correction. Until corrected, override it explicitly with a persistent + tool root under =$HOME=. + +* Installation policy + +- Install the latest *stable* releases from official upstream sources. Do not + install beta/nightly builds. +- Prefer isolated or native distributions. Do not install Python tools into + system Python 3.6 or a project environment. +- Keep executable shims in =~/.local/bin= or the existing user-owned nvm + prefix. A fresh =bash -lc= must resolve every harness without manual setup. +- Download installer scripts from official locations, inspect their relevant + install-path behavior before execution, and do not use sudo. +- Preserve existing config files. Before editing one, make a timestamped, + mode-preserving backup beside it. Never commit home-directory config or + backups to this repository. +- Do not delete stale installations during this pass. Report shadowed or + duplicate copies and recommend cleanup separately. + +Install these harnesses: + +1. *Aider*: use uv's isolated tool installation with Python 3.12 and + =aider-chat@latest=. Explicitly set a persistent =UV_TOOL_DIR= under + =~/.local/share/uv= and =UV_TOOL_BIN_DIR=$HOME/.local/bin= for the command. +2. *Codex*: install the stable =@openai/codex= package using the existing + user-owned npm prefix, unless the current official standalone installer is + materially better on this host. +3. *OpenCode*: install stable OpenCode 1 (=opencode-ai=, executable + =opencode=). Do not substitute the OpenCode 2 beta executable + =opencode2=; SuCoder's current profile targets =opencode=. +4. *Goose*: install the CLI, not the desktop application, using the official + =aaif-goose/goose= stable installer with configuration disabled during + install. Its executable should land in =~/.local/bin=. +5. *Kimi*: install current stable *Kimi Code CLI* from MoonshotAI's official + installer. Prefer its current native/single-binary distribution rather + than adding a Node-version dependency merely for installation. Confirm + that =kimi --help= still provides the flags SuCoder uses: =--agent-file=, + =--auto=, =--add-dir=, and model/provider support. + +Inspect the existing Claude and GitNexus installations. Update them only via +their native/official update path when a newer stable release is actually +available. Do not replace a working install merely to make all tools use the +same package manager. + +Official starting points (verify them at execution time): + +- Aider: https://aider.chat/docs/install.html +- Codex: https://github.com/openai/codex +- OpenCode: https://opencode.ai/docs/ +- Goose: https://github.com/aaif-goose/goose +- Kimi Code: https://github.com/MoonshotAI/kimi-code + +* Provider and harness configuration + +Configure only non-secret provider metadata on Savio. The target model for +smoke testing is =openrouter/moonshotai/kimi-k3=. + +- Aider and OpenCode should consume =OPENROUTER_API_KEY= from the launch + environment and accept the provider-prefixed model name. +- Native Kimi is adapted by SuCoder through temporary =KIMI_MODEL_*= variables; + do not persist the key in Kimi's config. +- Goose natively recognizes =OPENROUTER_API_KEY=, but it may also require + non-secret provider/model selection (=openrouter= and + =moonshotai/kimi-k3=). Configure that metadata without a key. If SuCoder's + generic =--model openrouter/...= spelling does not match Goose's CLI, record + the exact error as a local SuCoder adapter gap rather than storing a secret + to work around it. +- Codex uses custom model-provider configuration for OpenAI-compatible + endpoints. Add a key-free OpenRouter provider stanza only if current Codex + supports sourcing its key from =OPENROUTER_API_KEY=. If SuCoder's model + prefix needs translation for Codex, record the exact required mapping rather + than embedding credentials. +- Leave Claude's existing authentication and configuration intact. + +* Verification + +After installation, start a fresh login shell and record, for every harness: + +1. =command -v= result; +2. version; +3. whether the path is under =$HOME= and visible on both the allocated compute + node and a login shell; +4. whether its SuCoder-required flags are present; +5. whether native Agent Skills, shell execution, file editing, MCP, and + subagents are available as expected. + +Do not claim provider success from =--version=. Perform a minimal live +OpenRouter smoke test only when the launch environment already contains the +transient key; do not ask the human to paste it into the cluster. Keep API use +small. If this bootstrap Claude session has no OpenRouter environment, leave +the live tests for the laptop-side relaunch commands below. + +The human will validate each completed harness from the laptop with commands +of this form: + +#+begin_src sh +sucoder -T savio-htc collaborate SuCoder --harness aider --model openrouter/moonshotai/kimi-k3 +sucoder -T savio-htc collaborate SuCoder --harness opencode --model openrouter/moonshotai/kimi-k3 +sucoder -T savio-htc collaborate SuCoder --harness goose --model openrouter/moonshotai/kimi-k3 +sucoder -T savio-htc collaborate SuCoder --harness kimi --model openrouter/moonshotai/kimi-k3 +#+end_src + +Test Codex separately after confirming its OpenRouter provider mapping. + +* Deliverable + +Replace the =Outcome= section below with: + +- a table of harness, exact version, resolved path, install/update method, and + smoke-test result; +- any persistent shell/config changes, with backup paths; +- unresolved SuCoder adapter gaps, including exact reproduction commands and + sanitized errors; +- the exact idempotent commands that should become a future target-bootstrap + script; +- a recommendation about the bad =UV_TOOL_DIR=/tmp/uv-tools-ligon= setting. + +Do not record secrets or secret-derived output. Commit only repository files +that document or implement the bootstrap; never commit files copied from +=$HOME=. + +* Outcome + +Pending remote execution. From 6b9c22182e9daed2b8f9f5efdaffbf53e1973a0f Mon Sep 17 00:00:00 2001 From: Ethan Ligon Date: Sat, 12 Sep 2026 10:14:30 +0000 Subject: [PATCH 9/9] fix: keep Slurm watchdog runtime state on node-local storage --- .sucoder/handoff.org | 36 +++++++++++++++++-- README.org | 15 +++++--- sucoder/timer_lifecycle.py | 13 ++++++- tests/test_timer_lifecycle.py | 68 +++++++++++++++++++++++++++++++---- 4 files changed, 118 insertions(+), 14 deletions(-) diff --git a/.sucoder/handoff.org b/.sucoder/handoff.org index 267ff7e..055cb23 100644 --- a/.sucoder/handoff.org +++ b/.sucoder/handoff.org @@ -68,7 +68,7 @@ as a test fixture. Record the staged timer path/digest. Check job output for =SUCODER_TIMER_STARTED= and absence of startup failure diagnostics. 2. Inspect the matching directory under - =~/.cache/sucoder/timers//-/=. Record =owner=, =status=, + =/tmp/sucoder-/timers//-/=. Record =owner=, =status=, and sanitized =timer.log=. Status should reach =monitoring=. 3. On that allocated node, in the same job environment, run =bash --ensure= twice. Expect @@ -156,4 +156,36 @@ untested items. Sign as Sue. No cluster execution is claimed yet. * Outcome -Pending an agent with cluster access. Prepared by Sue, 2026-09-12. +Partial validation on 2026-09-12, via =ssh carleton-htc-ln= and +=srun --jobid=38661192 --overlap --ntasks=1 --cpus-per-task=1 bash -s=. +Node =n0036.savio4=, account =co_carleton=, QoS +=carleton_htc4_normal=, partition =savio4_htc=; reused the existing allocation. + +PR baseline =81dd48d93ed47853d3a2d0907ca5b2efc2ff3966= failed startup: +=flock: 8: No locks available=. Independent probes returned 71 on NFS +HOME and 0 on node-local /tmp. The fix on =fix/pr13-node-local-locks= +moves runtime state to a private, owner-checked mode-700 /tmp directory, +without changing shared script staging or compatibility warning paths. + +With the fixed generated script, native startup and two reuse calls passed. +The watchdog and test shell both belonged to +=/system.slice/slurmstepd.scope/job_38661192/step_4/user/task_0=. +Git =2.43.7= at =/usr/bin/git= pushed tracked and untracked changes from +=/tmp/sucoder-pr13-native.Og6uY7/work= to the durable NFS fixture +=/global/home/users/ligon/.cache/sucoder/pr13-snapshot.gLzDQe=. +The real index was unchanged. After ending only the disposable tmux session, +the watchdog exited and removed its owner/status records (WATCHDOG_EXIT_PASS). +Snapshot ref +=refs/sucoder/wip/pr13-review-smoke= resolved to +=a923fb24baeb0d55797e617235aaafead715e7c1=, tree +=95ee90e4f9f9225f47a84b6fa37499492e15409b=. + +Local validation: 763 tests pass on Python 3.11. The NFS-lock regression +fails against the original PR template and passes with the fix. Additional +cases reject symlink, public, and non-directory runtime roots without +modifying them. + +Still untested: fresh sbatch environment (issue 15), end-to-end normal +launcher submission, native threshold timing, /local clone preparation, +and unconfined SSH launch. This srun probe does not settle issue 15. +The existing allocation was not cancelled. Full handoff remains READY. diff --git a/README.org b/README.org index 55b2657..9bed282 100644 --- a/README.org +++ b/README.org @@ -790,9 +790,9 @@ Every SLURM-backed session (=salloc= or =confined= =sbatch=) starts a small watchdog on the compute node. It warns at 30, 15, and 5 minutes before the allocation's =--time= via =tmux display-message= and by writing a warning under -=$HOME/.cache/sucoder/timers//-/= (the +=/tmp/sucoder-/timers//-/= (the un-suffixed =slurm-deadline.warn= and per-mirror -=slurm-deadline-.warn= are also written in the cache root for +=slurm-deadline-.warn= are also written in =$HOME/.cache/sucoder/= for older prompts; these compatibility copies show the last writer). It never cancels the job; that stays with =sucoder release=. @@ -804,8 +804,15 @@ whether it started or reused a timer. Failures explicitly report that deadline warnings and periodic snapshots are unavailable; diagnostics remain in =timer.log= beside its =owner= and =status= files. Timers from the older implementation are not killed by a broad process-name match; they exit with -their old allocation. Shared-filesystem lock behavior should be verified -when bringing up a new target. +their old allocation. + +Locks, owner/readiness records, threshold markers, and timer logs live in a +private, owner-verified mode-700 directory on node-local =/tmp=. This transient +state only needs to survive for the watchdog lifetime. =TMPDIR= is deliberately +not used: it may point at shared storage. Staged scripts and compatibility +warnings stay on NFS; NFS need not support =flock=. Working clones may use +=/local/job/=, with snapshots pushed to durable storage. Lustre is not +used for the timer's frequent small-file operations. The same script snapshots the mirror's dirty working tree (tracked and untracked files, not ignored ones) to =refs/sucoder/wip/= on diff --git a/sucoder/timer_lifecycle.py b/sucoder/timer_lifecycle.py index 8f7e4eb..c948d43 100644 --- a/sucoder/timer_lifecycle.py +++ b/sucoder/timer_lifecycle.py @@ -6,7 +6,18 @@ [[ "$node" =~ ^[a-zA-Z0-9._-]+$ && "$JOB" =~ ^[0-9]+$ ]] || { echo "sucoder timer: invalid node or allocation identity" >&2; exit 1; } -STATE_DIR="$CACHE_DIR/timers/$TIMER_SCOPE/$node-$JOB" +# Locks and liveness records are node-local, just like /proc. Shared HOME +# may be NFS without flock support; TMPDIR may also point at shared storage. +# Never follow or chmod a pre-created path in world-writable /tmp. +RUNTIME_DIR="/tmp/sucoder-$UID" +umask 077 +mkdir -m 700 "$RUNTIME_DIR" 2>/dev/null || true +if [ -L "$RUNTIME_DIR" ] || [ ! -d "$RUNTIME_DIR" ] || + [ ! -O "$RUNTIME_DIR" ] || [ "$(stat -c %a "$RUNTIME_DIR")" != 700 ]; then + echo "sucoder timer: unsafe runtime directory: $RUNTIME_DIR" >&2 + exit 1 +fi +STATE_DIR="$RUNTIME_DIR/timers/$TIMER_SCOPE/$node-$JOB" mkdir -p "$STATE_DIR" || exit 1 chmod 700 "$STATE_DIR" || exit 1 command -v flock >/dev/null || { echo "sucoder timer: flock unavailable" >&2; exit 1; } diff --git a/tests/test_timer_lifecycle.py b/tests/test_timer_lifecycle.py index aca59e0..cde14a0 100644 --- a/tests/test_timer_lifecycle.py +++ b/tests/test_timer_lifecycle.py @@ -2,6 +2,7 @@ import os import shutil import signal +import shlex import subprocess import time from types import SimpleNamespace @@ -12,11 +13,16 @@ @pytest.fixture -def timers(tmp_path): +def timers(tmp_path, monkeypatch): + from sucoder import slurm_timer + monkeypatch.setattr(slurm_timer, "TIMER_LIFECYCLE_SH", + slurm_timer.TIMER_LIFECYCLE_SH.replace( + 'RUNTIME_DIR="/tmp/sucoder-$UID"', + "RUNTIME_DIR=" + shlex.quote(str(tmp_path / "runtime")))) bin_dir = tmp_path / "bin" bin_dir.mkdir() for name in ("bash", "flock", "hostname", "mkdir", "chmod", "cat", "rm", - "nohup", "sleep", "tail", "seq", "touch", "git", "mktemp", "date"): + "nohup", "sleep", "tail", "seq", "touch", "git", "mktemp", "date", "stat"): executable = shutil.which(name) if not executable: pytest.skip(f"requires {name}") @@ -64,7 +70,7 @@ def finish(child): @pytest.mark.parametrize("confined", [False, True]) def test_timer_survives_starter_and_reuses_owner(timers, confined): assert "STARTED" in finish(timers.start(confined=confined)) - owner = next(timers.root.glob(".cache/sucoder/timers/*/*/owner")) + owner = next(timers.root.glob("runtime/timers/*/*/owner")) identity = owner.read_text() assert "REUSED" in finish(timers.start(confined=confined)) assert owner.read_text() == identity @@ -76,20 +82,21 @@ def test_concurrent_starters_create_one_timer(timers): outputs = [finish(a), finish(b)] assert sum("STARTED" in out for out in outputs) == 1 assert sum("REUSED" in out for out in outputs) == 1 - assert len(list(timers.root.glob(".cache/sucoder/timers/*/*/owner"))) == 1 + assert len(list(timers.root.glob("runtime/timers/*/*/owner"))) == 1 def test_target_and_allocation_have_distinct_timers(timers): for target, job in [("savio", 12), ("savio-htc", 12), ("savio", 13)]: assert "STARTED" in finish(timers.start(target, job)) - owners = list(timers.root.glob(".cache/sucoder/timers/*/*/owner")) + owners = list(timers.root.glob("runtime/timers/*/*/owner")) assert len({p.read_text() for p in owners}) == 3 assert timer_identity("a/b", None) != timer_identity("a_b", None) def test_dead_owner_metadata_does_not_signal_unrelated_process(timers): node = subprocess.check_output(["hostname"], text=True).strip() - state = timers.root / ".cache/sucoder/timers" / timer_identity("example", "savio") / f"{node}-12" + (timers.root / "runtime").mkdir(mode=0o700) + state = timers.root / "runtime/timers" / timer_identity("example", "savio") / f"{node}-12" state.mkdir(parents=True) # The current test runner PID is intentionally paired with a wrong birth # time. No lock exists, so a fresh timer must replace this stale record. @@ -142,7 +149,7 @@ def git(*args): assert git("-C", str(origin), "show", f"{ref}:tracked") == "working" assert git("-C", str(origin), "show", f"{ref}:untracked") == "new" assert git("-C", str(work), "write-tree") == before - warning = next(timers.root.glob(".cache/sucoder/timers/*/*/slurm-deadline-*.warn")) + warning = next(timers.root.glob("runtime/timers/*/*/slurm-deadline-*.warn")) assert "Commit and save NOW" in warning.read_text() @@ -159,3 +166,50 @@ def timeout(args, **kwargs): cli._start_slurm_timer(session, control, control, logging.getLogger("timer-test")) assert "exit -1" in caplog.text assert "snapshots are unavailable" in caplog.text + + +def test_locks_work_when_home_does_not_support_flock(timers): + """Model the cluster: flock on HOME fails, node-local locks work.""" + real_flock = shutil.which("flock") + (timers.bin / "flock").unlink() + (timers.bin / "flock").write_text( + '#!/bin/bash\n' + 'for fd in 8 9; do\n' + ' path=$(/usr/bin/readlink /proc/$$/fd/$fd 2>/dev/null)\n' + ' case "$path" in "$HOME"/.cache/*)\n' + ' echo "No locks available" >&2; exit 71 ;; esac\n' + 'done\n' + f'exec {shlex.quote(real_flock)} "$@"\n' + ) + (timers.bin / "flock").chmod(0o700) + assert "STARTED" in finish(timers.start()) + assert "REUSED" in finish(timers.start()) + runtime = timers.root / "runtime" + assert runtime.stat().st_mode & 0o777 == 0o700 + assert list(runtime.glob("timers/*/*/run.lock")) + assert not list((timers.root / ".cache").rglob("*.lock")) + + +@pytest.mark.parametrize("kind", ["symlink", "public", "file"]) +def test_unsafe_runtime_directory_is_rejected(timers, kind): + runtime = timers.root / "runtime" + if kind == "symlink": + destination = timers.root / "destination" + destination.mkdir(mode=0o755) + runtime.symlink_to(destination, target_is_directory=True) + elif kind == "public": + runtime.mkdir(mode=0o755) + else: + runtime.write_text("keep") + child = timers.start() + out, err = child.communicate(timeout=15) + assert child.returncode != 0 + assert "unsafe runtime directory" in err + assert "STARTED" not in out + if kind == "symlink": + assert destination.stat().st_mode & 0o777 == 0o755 + assert not list(destination.iterdir()) + elif kind == "public": + assert runtime.stat().st_mode & 0o777 == 0o755 + else: + assert runtime.read_text() == "keep"