diff --git a/.coder/ledger.md b/.coder/ledger.md new file mode 100644 index 0000000..354e3f5 --- /dev/null +++ b/.coder/ledger.md @@ -0,0 +1,69 @@ +# 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. + +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 +batch-environment Git availability need a real allocation. Issues #14 and #16 +remain outside this integration. + +Prepared by Sue (2026-09-12). diff --git a/.sucoder/handoff.org b/.sucoder/handoff.org index 83e9134..055cb23 100644 --- a/.sucoder/handoff.org +++ b/.sucoder/handoff.org @@ -1,191 +1,191 @@ -#+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 + =/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 + =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. +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/.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. diff --git a/README.org b/README.org index 40bc453..9bed282 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,31 @@ 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 +=/tmp/sucoder-/timers//-/= (the +un-suffixed =slurm-deadline.warn= and per-mirror +=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=. +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. + +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 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..ee5db0e 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,12 +2851,17 @@ 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), ) + # Publish a complete script without truncating an open inode, as on + # the SSH launch path (ledger 4). mktemp is unique across nodes that + # share HOME, where a PID suffix alone could collide. self.executor.run_agent( [ "sh", "-c", - f"umask 077 && cat > {shlex.quote(timer_path)} " - f"&& chmod 700 {shlex.quote(timer_path)}", + 'umask 077 && t=$(mktemp "$1.tmp.XXXXXX") && 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/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..7b43808 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``. @@ -93,7 +101,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 @@ -106,12 +119,15 @@ 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" -rm -f "$WARN5" "$WARN15" "$WARN30" "$WARN_FILE" +rm -f "$WARN5" "$WARN15" "$WARN30" "$WARN_FILE" "$LEGACY_WARN_FILE" "$MIRROR_WARN_FILE" if [ -z "$JOB" ]; then echo "sucoder timer: no SLURM job id (not inside a job?); exiting." > "$WARN_FILE" @@ -125,6 +141,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 } @@ -142,36 +159,58 @@ 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 # 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 +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 + if ! left=$(squeue --job "$JOB" --noheader -o "%L" 2>/dev/null); then + # Failed queries are unknown state, not evidence the job ended + # (ledger 3). Discard partial output and interrupt the empty-query + # streak. Still check tmux and take periodic snapshots below. + left="" + missed=0 + elif [ -z "$left" ]; then + # Only consecutive successful empty queries establish disappearance. + 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 + else + missed=0 fi # 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 + # 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)." @@ -197,6 +236,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. @@ -229,14 +269,33 @@ 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("@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) .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/sucoder/timer_lifecycle.py b/sucoder/timer_lifecycle.py new file mode 100644 index 0000000..c948d43 --- /dev/null +++ b/sucoder/timer_lifecycle.py @@ -0,0 +1,82 @@ +"""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; +} +# 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; } + +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..ec839da 100644 --- a/tests/test_batch_script.py +++ b/tests/test_batch_script.py @@ -123,11 +123,14 @@ 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") assert rc_check < s.index(nohup) < keeper + # The supervisor checks readiness; a failure remains visible in the job log. + assert "--ensure || echo 'SUCODER: timer failed;" in s + assert "> /dev/null 2>&1 &" not in s def test_timer_omitted_when_no_path(): @@ -137,7 +140,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..f850281 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,35 @@ def capture(**kw): assert "JOB=7\n" in rendered[0] +def test_salloc_timer_is_per_mirror_and_staged_atomically(monkeypatch): + """Preserve main's atomic staging and readiness handshake (ledger 4).""" + 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] + # The script name hashes its contents, including mirror/allocation identity. + script = re.search(r"slurm-timer-[0-9a-f]{24}\.sh", write).group() + assert script 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 'mktemp' in write + assert 'cat > "$tmp"' in write + assert 'chmod 700 "$tmp"' in write + assert 'mv "$tmp"' in write + assert write.index('cat > "$tmp"') < write.index('mv "$tmp"') + assert '--ensure' in start + assert 'pkill' not in start and 'nohup' not 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.""" @@ -2325,9 +2357,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..cf1e6b7 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( @@ -1720,23 +1724,57 @@ 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] - assert timer_path.endswith("/.cache/sucoder/slurm-timer-sample.sh") - assert "chmod 700" in timer["args"][2] + # 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 "/.cache/sucoder/slurm-timer-sample-" in timer_path + 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"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"] 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) +def test_confined_timer_staging_keeps_open_script_intact(tmp_path, monkeypatch): + """An existing reader keeps its inode when the staged script is replaced.""" + manager, ctx = _confined_manager(tmp_path, monkeypatch) + calls = [] + manager.executor.run_agent = _confined_responder(calls, sbatch_out="17") + manager._launch_confined( + ctx, ["claude"], remote_prelude_text=None, + prelude_sentinel="__X__", env=None, detached=True, + ) + timer = next(c for c in calls if c["args"][0] == "sh" + and c["input"] and "# sucoder SLURM deadline timer" in c["input"]) + path = tmp_path / "timer with 'quotes'.sh" + command = [*timer["args"][:4], str(path)] + old = "#!/bin/bash\necho old\n" + new = "#!/bin/bash\necho new\n" + subprocess.run(command, input=old, text=True, check=True, capture_output=True) + with path.open() as reader: + subprocess.run(command, input=new, text=True, check=True, capture_output=True) + assert reader.read() == old + assert path.read_text() == new + assert path.stat().st_mode & 0o777 == 0o700 + assert not list(tmp_path.glob("*.tmp.*")) + + def test_launch_confined_local_tier_stages_prepare_script(tmp_path, monkeypatch): """With a local-disk root on the executor the confined launch stages a third file (the prepare script), the batch body runs it and cds into 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..d876218 100644 --- a/tests/test_slurm_timer_script.py +++ b/tests/test_slurm_timer_script.py @@ -8,6 +8,8 @@ from __future__ import annotations import os +import re +import shlex import shutil import subprocess from pathlib import Path @@ -35,7 +37,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,15 +65,43 @@ 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 - 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 +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", "$MIRROR_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 +123,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 @@ -204,3 +248,226 @@ 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. None represents + a failed query; (rc, output) also permits partial output on failure. + 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" + polls = [(1, "") if v is None else v if isinstance(v, tuple) else (0, v) + for v in time_left] + (bin_dir / "squeue").write_text( + '#!/bin/bash\n' + f'n=$(cat {shlex.quote(str(counter))} 2>/dev/null || echo 0); ' + f'n=$((n+1)); echo $n > {shlex.quote(str(counter))}\n' + 'vals=(' + " ".join(shlex.quote(v) for _, v in polls) + ')\n' + 'codes=(' + " ".join(str(rc) for rc, _ in polls) + ')\n' + 'if [ "$n" -le "${#vals[@]}" ]; then\n' + ' echo "${vals[$((n-1))]}"; exit "${codes[$((n-1))]}"\n' + '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}}" >> {shlex.quote(str(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() + result = subprocess.run(["bash", str(script)], env=env, timeout=10, + capture_output=True, text=True) + assert result.returncode == 0, result.stderr + 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] + + +@_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 + + +@_bash +@pytest.mark.parametrize("failures", [[None] * 4, [(1, "1:00")] * 4]) +def test_scheduler_errors_keep_monitoring_until_recovery(tmp_path, failures): + # Errors are unknown state, even when squeue emits partial output (ledger 3). + msgs = _drive(tmp_path, ["2:00:00", *failures, "25:00", "12:00", "4:00"]) + deadline = [m for m in msgs if "min left" in m] + assert len(deadline) == 3, msgs + assert "Commit and save NOW" in deadline[-1] + assert int((tmp_path / "n").read_text()) == 11 + + +@_bash +def test_failed_query_interrupts_consecutive_empty_observations(tmp_path): + msgs = _drive(tmp_path, ["", "", None, "", "", "4:00"]) + assert any("Commit and save NOW" in m for m in msgs), msgs + assert int((tmp_path / "n").read_text()) == 9 + + +@_bash +@_git +def test_scheduler_outage_does_not_suspend_periodic_snapshots(repo_pair, tmp_path): + origin, work = repo_pair + (work / "a.txt").write_text("recover this during the outage\n") + # All cadence ticks occur during failed queries; recovery is outside a + # threshold and outside the cadence, so only the outage can save this tree. + _drive(tmp_path, [None, None, None, "2:00:00"], + snapshot_dir=str(work), snapshot_minutes=2) + saved = subprocess.run( + ["git", "show", "refs/sucoder/wip/K-Aggregators:a.txt"], cwd=origin, + capture_output=True, text=True, check=True, + ) + assert saved.stdout == "recover this during the outage\n" + + +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 + + +@_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 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..cde14a0 --- /dev/null +++ b/tests/test_timer_lifecycle.py @@ -0,0 +1,215 @@ +"""Run the generated watchdog and its starter under real bash/flock.""" +import os +import shutil +import signal +import shlex +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, 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", "stat"): + 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("runtime/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("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("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() + (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. + (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("runtime/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 + + +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"