feat(slurm): adopt a mirror's live job instead of allocating a second - #25
Merged
Merged
Conversation
Issue 19's option 3, and the leak the existing reuse probe could not see. _launch_confined already refused to resubmit while a job was live, but it asked the session record -- ~/.sucoder/sessions/<mirror>--<target>.yaml, which holds ONE job id, and which RemoteSession.load returns blank for when the file is missing or unreadable. A record that was clobbered by a later launch, lost, or written under a different -T spelling therefore read as "no job at all", and the launch went straight to sbatch over a job that was still running. That is the configuration issue 19 was filed about, arrived at from the other direction. Slurm has known the answer all along: every confined launch carries --job-name=sucoder-<token>. So when the record finds nothing, ask the scheduler. A live job for this mirror on this target is adopted -- attached to, and written back into the record, so attach/release/renew can reach it again -- rather than allocated beside. Three deliberate non-blocks: A job on a DIFFERENT target does not stop the launch. One mirror on two targets is a deliberate configuration, not a mistake; it is reported and the new job is allocated. Targets are told apart by partition/account/qos via sessions_report.match_target, the same signature `sucoder sessions` groups by, so a target pinning none of the three claims nothing rather than everything. A failed squeue does not stop it either. This is a safety net over the record-keyed probe, not a gate, and that probe already refuses to resubmit on an unknown answer for a job it knows about. An array element like `4242_3` is not adopted: int() reads underscores as digit separators, so it would have become job 42423. salloc launches now carry the same --job-name. Nothing reuses by name on that path yet -- it reuses via the record and adopts by node -- but without it an allocation is invisible to `sucoder sessions`, which filters on the `sucoder-` prefix, and there is nothing for a name-keyed probe to match on later. The two reuse paths share _reuse_confined_job so they cannot drift. All three changes are mutation-checked: disabling the name probe, adopting regardless of target, and dropping the salloc name each fail a test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lb2GYSyS7vteb34ziwHQ4c
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
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.
Issue #19's option 3 — and the leak the existing reuse probe could not see.
What was wrong
_launch_confinedalready refused to resubmit while a job was live. But it asked the session record —~/.sucoder/sessions/<mirror>--<target>.yaml, which holds one job id, and whichRemoteSession.loadreturns blank for when the file is missing or unreadable.So a record clobbered by a later launch, lost, or written under a different
-Tspelling read as "no job at all", and the launch went straight tosbatch— over a job that was still running. That is the configuration #19 was filed about, arrived at from the other direction: not two deliberate launches racing, but one launch that could no longer see the first.Slurm has known the answer all along. Every confined launch carries
--job-name=sucoder-<token>.What it does now
When the record finds nothing, ask the scheduler:
squeue --me --name=sucoder-<token>. A live job for this mirror on this target is adopted — attached to, and written back into the record, soattach,releaseandrenewcan reach it again — instead of allocated beside.That last part matters as much as the refusal: it heals the orphan rather than just declining to make another one.
Three deliberate non-blocks
A job on a different target does not stop the launch. One mirror on two targets is a deliberate configuration, not a mistake; it is reported and the new job allocated. Targets are told apart by partition/account/qos via
sessions_report.match_target— the same signaturesucoder sessionsgroups by, so a target pinning none of the three claims nothing rather than everything.A failed
squeuedoes not stop it either. This is a safety net over the record-keyed probe, not a gate, and that probe already refuses to resubmit on an unknown answer for a job it knows about. Making this one fail closed would turn a controller blip into "cannot launch".An array element like
4242_3is not adopted.int()reads underscores as digit separators, so it would have become job42423— the same trap that bitparse_squeuein #23.salloc jobs get a name
salloc --no-shellcarried no--job-nameat all. Nothing reuses by name on that path yet — it reuses via the record and adopts by node — but without the name an allocation is invisible tosucoder sessions, which filters on thesucoder-prefix, and there is nothing for a name-keyed probe to match on later. Sanitized identically toconfined_tmux_target, or the two paths disagree about what a mirror is called.Testing
886 passed(from 880 on main). The two reuse paths now share_reuse_confined_jobso they cannot drift.All three changes are mutation-checked — disabling the name probe, adopting regardless of target, and dropping the salloc name each fail a test. The
_confined_responderstub asserts the argv shape of both squeue forms, so a drift in either command fails loudly rather than silently mis-routing to a green pass.tests/test_direct_remote.py::test_direct_collaborate_rejects_nodefails in my environment before and after (a chmod-based readability check); CI is green on it.Not here
The unconfined path still reuses via the record and adopts by node, and can still double-allocate if the record is lost. Naming the jobs is the prerequisite for fixing that, and it is in this PR; the fix itself is not, because
_ensure_slurm_nodehas a documented persist-before-query window that deserves its own change rather than a rider on this one.🤖 Generated with Claude Code
https://claude.ai/code/session_01Lb2GYSyS7vteb34ziwHQ4c