fix: harden startup and Slurm timer supervision - #13
Merged
Conversation
…r the legacy warn file
Three defects found reviewing the timer work, two of them in the new
confined path.
*Staging truncated a script a running job was still executing.* Both
launch paths wrote the timer with `cat >` straight onto its final path.
A second launch that computes the same path -- one mirror on two targets
sharing $HOME (the reuse-probe is keyed per target), or two mirror names
that sanitize alike -- truncated the file under the first job's bash,
which reads its script from an open fd at a byte offset and therefore
just stops, mid-script, with no diagnostic anywhere. Verified: the
running script printed its first line and never reached its tail. Both
paths now write a temp file and `mv -f` it into place, so a running job
keeps reading the intact inode it already holds.
The salloc path additionally staged every mirror to one shared
`slurm-timer.sh`, so two concurrent unconfined mirrors overwrote each
other's script outright; it now uses the same per-mirror name the
confined path and the state files already use.
*A timer that failed to start was invisible.* The batch body ran
`nohup <timer> > /dev/null 2>&1 &` with no check, and nohup's own
failure went to /dev/null while the outer rc stayed 0 -- so a noexec
$HOME or a partially staged script produced a job with no deadline
watchdog and an empty job log, i.e. exactly the bug this timer was
added to fix. Both paths now guard on `-x` and report; the batch body
leaves nohup's stderr on the job log so an exec failure surfaces too.
*The legacy warn file was never cleared.* Making $WARN_FILE per mirror
left the un-suffixed `slurm-deadline.warn` written but no longer part of
the startup `rm -f`, so a previous job's "allocation may have ended"
survived on the legacy path into a healthy new session -- read by
exactly the older prompts that path is kept for.
Tests: the three shape assertions the changes invalidated are updated
rather than re-baselined, and each new assertion was checked by
introducing the bug it targets. Also closes two gaps found by mutating
the suite: rendering both bash helpers as empty strings shipped a script
that never warns and never snapshots while the suite stayed green (bash
-n does not flag a call to an undefined function), and both `scancel`
guards used `startswith("scancel")`, which misses `then scancel ...`,
`&& /usr/bin/scancel ...`, `$(scancel ...)` and `timeout 5 scancel ...`
-- every plausible way it would come back. 683 passed; mypy unchanged
(57 pre-existing).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Ty45bJv1jADvUbyLaW4RD
This was referenced Sep 10, 2026
The 30/15/5-minute chain fires the highest *unfired* threshold and marked
only that one, so once a lower threshold tripped first the next poll fell
through to a *less* urgent branch. A job starting with 3 minutes left
produced:
SLURM: ~3 min left (job 42). Commit and save NOW.
SLURM: ~2 min left (job 42). Start wrapping up.
SLURM: ~1 min left (job 42).
Urgency running backwards as the deadline approached, and since this PR
made each warning also snapshot, three full snapshot cycles -- a
`git add -A` over the whole tree plus a push -- in three consecutive
minutes, the cost the module docstring warns about. (An earlier
version of this message said the sweep was "on NFS". It is not: the
shared mirror is Lustre, and since the local-disk tiering work the
sweep runs against the node-local clone, so it is the push, not the
sweep, that reaches shared storage.)
Any skipped poll did the same (31 -> 14 minutes fired the 15-minute
warning, then the 30-minute notice a minute later).
This is inherited from the pre-refactor inline script, where it only
reached salloc sessions; it matters now because confined jobs -- which
had no watchdog at all before this branch -- run the same chain, and
short debug allocations and re-attaching to a nearly-expired job both
start inside a threshold.
Fix: firing a threshold touches every coarser marker too, so the chain
can only escalate.
The three tests added with it are the first to drive the monitoring loop
rather than assert on the rendered string: they stub squeue/tmux/sleep on
PATH and read back the messages a human would have seen. All three fail
against the previous chain (verified before the fix went in), covering a
normal countdown, a job that starts inside a threshold, and a skipped
poll. 686 passed; mypy unchanged (57 pre-existing).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Ty45bJv1jADvUbyLaW4RD
…e truth about their lifecycle
*One empty squeue read retired the watchdog.* The loop treated empty
output as "the job left the queue" and broke. squeue prints nothing on
a controller RPC timeout too -- routine on a busy scheduler -- so a
single blip ended the watchdog on a job with hours left and told the
human the allocation had ended. Verified against the rendered script:
10h remaining, one simulated timeout, watchdog gone.
cli._slurm_job_state already draws this distinction carefully ("An
ssh/squeue failure is NOT evidence the job is dead") 400 lines away; the
timer contradicted it. It now needs three consecutive empty reads
before believing the job is gone. A job that really ended is noticed a
couple of minutes later, which costs nothing -- the message is
informational, and every deadline warning has already fired by then.
*The lifecycle hint was false under sbatch.* Both "agent session gone"
messages said the job was kept alive and offered `sucoder release` /
`scancel`. That is true for salloc, where the user owns the allocation,
and wrong for confined jobs: the batch body's keeper loop polls the same
tmux session, so the job COMPLETEs with it. A confined user was being
sent after a job that had already ended. The hint is now chosen by
launch mode, which the builder already knows (`job_id is None` means the
id is read from $SLURM_JOB_ID at run time, i.e. sbatch).
689 passed; mypy unchanged (57 pre-existing). Both fixes were checked by
reintroducing the bug and confirming the new tests fail.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Ty45bJv1jADvUbyLaW4RD
…it checked was broken Found by mutating the suite: each of these regressions shipped green against all 680 tests. *`--force` was never exercised.* Every push in every snapshot test creates the WIP ref for the first time, where `--force` does nothing, so dropping it passed. It is load-bearing in reality: the ref is built with `commit-tree -p HEAD`, so once the agent rebases, resets, or switches branch the new snapshot is not a descendant of the old one and an unforced push is rejected -- silently, every step of snapshot_wip being best-effort -- in precisely the situation the snapshotter exists for. The new test diverges HEAD between two snapshots and asserts the ref advanced (and that the two really are unrelated). *The cadence assertion could not fail.* `_confined_manager` built its SlurmConfig without `wip_snapshot_minutes`, so it got the dataclass default 10 -- identical to build_timer_script's own default. Asserting "SNAPSHOT_MINUTES=10" therefore could not distinguish a value read from config from one hardcoded in the builder, and hardcoding it passed. The fixture now sets 7. *The snapshot directory was only checked non-empty.* The fixture stubs the remote path, so the exact value was available all along; asserting merely that SNAPSHOT_DIR was set let "snapshot the cache dir instead of the mirror" pass -- which would save none of the agent's work. 690 passed; mypy unchanged (57 pre-existing). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Ty45bJv1jADvUbyLaW4RD
ligon
force-pushed
the
fix/slurm-timer-hardening
branch
from
September 10, 2026 06:25
6f21c39 to
2c24354
Compare
Preserve the readiness handshake and allocation locks from 258accb while carrying forward the warning-loop fixes. Scheduler failures remain unknown state. Co-Authored-By: GPT-6 <noreply@openai.com>
Preserve 258accb's allocation-scoped supervision, readiness handshake, immutable SSH script names, and startup-safety changes. Retain PR 13's warning ordering, compatibility-file clearing, lifecycle hints, atomic confined staging, and regression coverage. Use mktemp for shared-HOME staging rather than relying on node-local PID uniqueness. Treat failed squeue calls as unknown state: discard partial output, reset the successful-empty-query streak, and keep periodic snapshots running. Only three consecutive successful empty observations retire the watchdog. This follows .coder/ledger.md sections 3-5. Validation: 759 tests passed with python -m pytest -q on Python 3.12. Four new scheduler regression cases failed before the fix. A real-file staging test verifies that an existing reader retains the old inode. GitNexus detect-changes against main reports the expected timer scope. Cluster smoke testing and issue 15's batch Git availability remain open. Co-Authored-By: GPT-6 <noreply@openai.com>
Verified readiness and startup machinery is preserved, scheduler failures remain unknown state, and 759 tests pass. Co-Authored-By: GPT-6 <noreply@openai.com>
ligon
changed the title
Archive the pending harness bootstrap unchanged. Document the tested revision, native smoke checks, issue 15 batch-environment probe, and required evidence. Shell snippets pass bash -n; no code changes. Co-Authored-By: GPT-6 <noreply@openai.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Slurm sessions can lose their deadline watchdog through script replacement,
silent startup failure, unsupported shared-filesystem locking, or scheduler
errors. This change stages timers atomically, supervises startup with an
explicit readiness handshake, and reuses one watchdog per target/node/allocation
without process-name killing.
Watchdog locks, owner/readiness records, threshold markers, and logs now live
in a private, owner-checked mode-700 directory under
/tmp/sucoder-<uid>.On the cluster, NFS home rejects
flockwith “No locks available”; node-local/tmpsupports it. Unsafe pre-existing runtime directories are rejected withoutmodifying them. Staged scripts and compatibility warnings remain on shared
storage, and working-tree snapshots are pushed to durable repositories.
The warning loop escalates through 30/15/5 minutes without later emitting
less urgent warnings. Failed
squeuecalls are unknown state: their partialoutput is discarded and periodic WIP snapshots continue. The watchdog exits
after three consecutive successful empty queries, or when the tmux session
ends. New watchdogs clear stale compatibility warnings, and lifecycle messages
distinguish sbatch from salloc.
Remote mirror bootstrap also preserves existing files and refs when initial
publication encounters ambiguous repository state or a transport failure.
These startup and supervision changes include local main commit
258accb.Validation:
pytest -q).with the fix. Additional tests reject symlink, public, and non-directory
runtime roots without modifying them.
interrupted empty observations, and WIP snapshots during an outage.
readiness failures, atomic staging with existing readers and quoted paths,
and snapshot/index preservation.
n0036.savio4, using existing SuCoderallocation
38661192throughcarleton-htc-ln: startup, two owner reuses,allocation cgroup membership, tracked/untracked snapshot contents pushed to
an NFS-backed repository, unchanged working index, and watchdog exit after
ending its disposable tmux session. The existing allocation was not cancelled.
the timer template, its tests, README, and validation handoff.
Remaining validation: fresh sbatch environment and Git availability (issue #15),
normal launcher submission end to end, native threshold timing,
/localclonepreparation, and unconfined SSH launch. The successful
srunsmoke test doesnot settle the fresh-batch environment question. Deferred items remain #14
(snapshot object retention), #15 (Git availability/diagnostics), and #16
(colliding compatibility warning paths and WIP refs).
Cluster evidence and remaining procedure are in the
READY handoff.
Design constraints and earlier integration verification are in
.coder/ledger.md.