Scope the Slurm epilog cleanup to the job that ended

[100milliongold]

Closes part of #1407.

What the issue asked, and what was already there

run-parts.sh already gates every *-lastuserjob-* script on the user having no other job on this node, and already refuses to treat a failed squeue lookup as "no other jobs". The ordinary concurrent-job case the issue worries about was covered. These commits close the gaps that remained; the two scoping items that need a rewrite (40 by cgroup, 50 by enroot path) are left for follow-ups, as the issue asks.

1. Suspended and configuring jobs were invisible to the gate

The gate asked squeue for RUNNING alone. A job that is SUSPENDED — scontrol suspend, or preemption with SuspendTime — still owns its processes, its files under /tmp and /dev/shm, and its enroot directories. So does a STOPPED job. A CONFIGURING or RESIZING job holds an allocation on the node and is about to. None of them appear under -t running, so 40-lastuserjob-processes would killall -9 them and 42-lastuserjob-cleanup would rm -fr their files.

The state list is now running,suspended,stopped,configuring,resizing.

COMPLETING is deliberately absent. The epilog runs while its own job is in that state, so including it would make every job find itself, leave last_user_job at 0, and switch the cleanup off entirely. The job's own id is filtered out as well — cheap, and it keeps a future state addition from silently doing that.

State names verified against squeue --helpstate on Slurm 26.05.1.

2. The file cleanup reached files the job did not own

42-lastuserjob-cleanup walked /tmp and /dev/shm and removed everything owned by the job's user. Three ways that hit bystanders:

The localusers.backup exemption was missing. 40-lastuserjob-processes spares those accounts so an operator keeps their login session when a job of theirs ends (#1404). Deleting every file they own undoes that from the other side: the session survives but loses the editor swap files, sockets and scratch it is holding open. The same exemption now applies, with the same treatment of a failed lookup — grep exiting >1 means the list could not be read, which is not evidence that the user is absent from it.

find's exit status was discarded by ||:. When the directory service is unavailable, find -user exits non-zero and prints nothing — and a partial listing would still have been piped into rm -fr. The account is resolved once up front and the step is skipped when it cannot be: an unresolvable owner is not evidence that nothing is owned. The listing is materialised before deletion for the same reason, since piping straight into xargs hides find's status behind xargs'.

-xdev was missing. The walk descended into anything mounted underneath — a job_container/tmpfs private /tmp, a user's sshfs, a bind mount of shared storage — and deleted files belonging to another node's still-running job.

3. Tests

The slurm role is excluded from the molecule CI because it needs systemd services a container cannot run, so these scripts had no automated cover. What they do is destructive and the failure mode is silent: a cleanup that reaps a bystander looks exactly like one that worked.

The harness renders the Jinja templates and runs the result. A trimmed copy would test something the cluster never runs, and the templates cannot even be parsed by bash -n as they sit in the tree.

Twelve cases pin the guards that decide whether to reap. Run on Rocky Linux 9.8:

this branch                12 passed, 0 failed
roles/ from master          5 passed, 7 failed

The seven that fail before the change:

case	pre-fix behaviour
a SUSPENDED job blocks the cleanup	cleanup ran
a CONFIGURING job blocks the cleanup	cleanup ran
the job's own id does not block its cleanup	cleanup skipped
a user listed in localusers.backup is spared	find ran against them
an unreadable localusers.backup stops the cleanup	proceeded anyway
an unresolvable owner stops the cleanup	proceeded anyway
-xdev keeps the walk off a submount	the file on the submount was deleted

One note on the harness itself: the squeue stub filters on the -t argument rather than answering the same way whatever was asked. An earlier version ignored it, and the pre-fix script passed the SUSPENDED case too — the test proved nothing until the stub honoured the filter.

Requires python3 with jinja2, which CI already installs alongside ansible. The mount-boundary case needs root and an unprivileged account, and skips itself when either is missing.

Not in this PR

Per the issue, kept separate:

• 40-lastuserjob-processes scoped by cgroup. ProctrackType=proctrack/cgroup makes the job's cgroup the authoritative scope, but the processes this script exists to catch are the ones that escaped it. Narrowing to the cgroup alone would quietly drop that purpose, so it needs a per-PID check against other jobs' cgroups and user.slice — and real-hardware verification before it can land.
• 50-lastuserjob-all-enroot-dirs scoped by job. The default paths are per-user, not per-job (/run/enroot/user-$(id -u), /tmp/enroot-data/user-$(id -u)). Sites that move enroot_data_path onto shared storage get a cross-node hazard the per-node gate cannot see. Fixing it properly means changing the defaults in config.example, which is a behaviour change that deserves its own review.

🤖 Generated with Claude Code

[dholt]

The added job-state and failed-lookup guards are useful, and the process/cgroup and per-user Enroot redesigns can remain separate. Two issues in this PR's own changed paths still need correction:

1. 42-lastuserjob-cleanup:51-63: find -xdev limits the walk, not the later rm -fr. A selected user-owned parent directory containing a nested mount is passed to recursive rm, which descends into that mount. A user-owned mounted root can itself be selected, and same-filesystem bind mounts are not excluded by -xdev at all. The current mount test keeps both directories root-owned, so it avoids these cases. Please enforce the mount boundary at deletion as well as enumeration. One conservative option is to skip roots with nested mounts; retaining cleanup around mounts requires explicitly excluding mountpoints and avoiding recursive deletion of selected parents. rm --one-file-system alone does not cover every case above. Add owned-parent, owned-mount-root and same-device bind-mount regressions in an isolated environment.
2. tests/slurm-epilog/run-tests.sh:173-212: setup_cleanup redirects the exemption file but leaves real /tmp and /dev/shm as cleanup roots. If the guard being tested regresses and alice exists, the test can delete that account's real scratch before reporting failure. Redirect both roots into the fixture before every execution, including negative controls/master comparisons, and reject any destructive target outside it. Test safety cannot depend on the production guard passing.

The exemption test also needs a deterministic resolvable identity and assertions on actual walk/delete attempts. With the exemption deliberately bypassed and Alice absent, the contributed suite still reports 11 passed, 0 failed. With a resolvable synthetic Alice and an outer recording stub, the same mutant attempts both real cleanup roots. These were inert reproductions; no host cleanup or mounts were performed.

CI also flagged the new renderer's implicit Jinja autoescape setting. It renders shell rather than HTML, so address that with a documented, narrowly scoped scanner disposition without changing shell-rendering semantics. The lint download timeout was retried separately; it is not a source-code finding.

[100milliongold]

Both findings reproduced. 6ed42458, bfa09315 and 031fce1a address them.

1. The mount boundary now holds at deletion

-xdev bounds the walk and not the rm -fr that follows, and all three shapes
you named get past it:

• a user-owned directory holding a mount is still selected, and the recursive
  delete descends into the mount the walk refused to enter;
• a user-owned mount root is selected in its own right, so the delete empties
  the mounted filesystem;
• a bind mount from the same filesystem carries the same device number, so it is
  not a boundary to -xdev at all.

The boundary is now taken from the kernel's mount table rather than inferred
from device numbers. Every mount under the cleanup root is pruned from the walk,
and a selected directory that still has one beneath it is left alone with a log
line naming the mount. Its siblings are still deleted, so cleanup continues
around a mount instead of stopping at the first one — which is the behaviour you
described as requiring explicit mountpoint exclusion rather than skipping the
root outright.

awk's exit status is checked the way find's already was: an empty mount list
reads as "nothing is mounted here", which would put the recursive delete back
exactly where it was. An unreadable /proc/self/mountinfo skips the root rather
than deleting across a boundary it cannot see.

One defect I introduced and caught while doing this: ${#mounts[@]} opens a
Jinja comment in a .j2-rendered file, so the template stopped rendering with
Missing end of comment tag. bash -n cannot see it because the file is a
template; rendering does. The emptiness test goes through the joined elements
instead.

2. Test isolation

As described. Only the mount case redirected the cleanup roots; every other case
ran the script with /tmp and /dev/shm still pointing at the real
directories, so a regression in the guard under test would have deleted a real
account's scratch and only then reported the failure.

setup_cleanup now builds fixture roots and redirects both for every case,
including the negative controls, and then verifies the redirect rather than
assuming it. If a template edit moves that line so the sed misses, setup
refuses to hand back the script instead of returning one aimed at /tmp.

The exemption case is no longer inert. It used alice, which resolves on almost
no host, so the script exited at the cannot resolve guard before reaching the
deletion — your 11-passed-0-failed observation. It now uses the account running
the tests, which resolves by construction, and asserts that a file that account
owns is still present rather than only that a log line is absent. A positive
control covers the other side: an unlisted, resolvable owner must actually be
cleaned up, or every guard case would pass for a script that deletes nothing.

Three mount-boundary cases are added, one per shape above.

Evidence

All runs in a privileged Ubuntu 24.04 container as root, which is what supplies
mount --bind and a readable /proc/self/mountinfo.

tree under test	result
this branch	16 passed, 0 failed
previous revision of the epilog, current tests	the three mount cases fail
exemption check deleted	a user listed in localusers.backup is spared fails
for dir in ... line edited so the redirect misses	harness refuses before any case runs

The mutant failure message names the fault rather than reporting a generic
mismatch, e.g. the guard did not stop the deletion: the fixture file is gone.

No host cleanup was performed outside the container's own fixture directories,
and every mount made by the suite is unmounted by it.

A separate harness fault these cases exposed

The scripts log with logger under set -e. On a host with no syslog socket —
any CI container — logger exits non-zero and takes the script down at whatever
line it had reached, so "the guard refused to delete" and "the script never got
there" were the same observation. That is the one distinction these tests exist
to make. A stub on PATH keeps the message on stderr without the exit status.
Four failures already present at the previous head go away with it; I ran that
revision separately to confirm they were pre-existing rather than introduced
here.

3. CodeQL autoescape

tests/slurm-epilog/render.py now states the setting instead of relying on the
default, with the query named and the reasoning next to it: the output is a
shell script written to a local file for bash -n and for the harness to
execute, never a web response; HTML escaping would corrupt it rather than
protect it (&& becomes &&); the substituted values are the fixed
fixture constants in the module. Shell-rendering semantics are unchanged.

Two things I could not do. Dismissing the alert in the Security tab needs write
access to this repository. And I could not confirm from GitHub's own
documentation whether code scanning honours in-source suppression comments such
as # lgtm[py/jinja2/autoescape-false], so I did not add a marker whose effect I
had not verified. If a suppression comment is the disposition you want, say
which form this repository uses and I will add it.

[dholt]

The fixture-root redirection and non-vacuous exemption test are fixed, and the new mount table protects the ordinary parent/root/bind-mount shapes. Two pathname details still bypass the new deletion boundary:

1. 42-lastuserjob-cleanup:98: shell quoting does not make find -path literal. A mount named mnt[1] or containing a backslash is treated as a glob pattern; the actual mount is not pruned, and its root/payload can reach rm -fr. Escape backslash and glob metacharacters in a separate pattern variable, retaining the decoded pathname for literal comparisons. A mount-root equality guard alone would not protect already-enumerated bind-mount descendants.
2. 42-lastuserjob-cleanup:94: command substitution removes trailing newlines after decoding mountinfo's octal escapes, changing a valid mount pathname. Use printf -v mount_point '%b' "$field" so every decoded byte is retained.

The attached suggestions are the small corrections exercised by the independent reviewer. With actual rendered scripts, real GNU find and synthetic mount records, bracket/backslash/trailing-newline cases selected protected paths for deletion, while plain/space/tab/internal-newline controls worked. The two corrections protected the failing cases and retained sibling cleanup. A separate parent sandbox independently reproduced all three failing pathname cases; no real mounts or host cleanup were used.

Please add these pathname regressions, including */? patterns and unrelated siblings. Also check the cleanup exit status in mount_case and require positive sibling cleanup in the owned-mount-root case: its current assertion reports success even when a replacement cleanup command exits17.

The existing shell-renderer CodeQL alert is already dismissed with the documented false-positive rationale; no unverified suppression marker is needed. The deferred process/cgroup/Enroot redesign remains outside this review.

[dholt]

Suggested change

	mount_point=$(printf '%b' "$field")
	printf -v mount_point '%b' "$field"

[100milliongold]

Applied in 2fe2843. Dropping the command substitution also stops $( ) from eating trailing newlines in a mount point, which the previous form would have.

[dholt]

Suggested change

	prune+=( -path "$mount_point" -prune -o )
	pattern=${mount_point//\\/\\\\}
	pattern=${pattern//\*/\\*}
	pattern=${pattern//\?/\\?}
	pattern=${pattern//\[/\\[}
	prune+=( -path "$pattern" -prune -o )

[100milliongold]

Applied in 2fe2843, with the backslash pass first so the escapes added after it are not doubled again.

I checked this against real find rather than reasoning about it. With a mount point named a*b sitting next to axb and ayb:

pattern	files left after the prune
unescaped $t/a*b	0
escaped	2

So the unescaped form pruned all three directories, and the epilog then skipped exactly the files it exists to remove. The escaped form prunes only the mount point.

The repository's own tests/slurm-epilog/run-tests.sh does not cover this: its mount-boundary cases skip unless run as root with mount and /proc/self/mountinfo, so they do not run on a developer machine. 11 of 12 pass here before and after the change, and the one failure is the same with and without it (macOS has no /proc/self/mountinfo).

[dholt]

Approving at 2fe2843b. Both remaining pathname findings are fixed exactly as suggested: -path patterns now escape backslash first, then *, ? and [, and printf -v keeps every decoded byte of the mount point, including trailing newlines.

Independently re-checked against the rendered template with real GNU find and synthetic mount records, without any real mounts: bracket, backslash, trailing-newline, *, ?, tab, space and internal-newline mount names are all protected, and ordinary siblings plus glob-lookalike siblings (data1 next to data[1], datax1 next to data*1) are still cleaned. The same fixture against the previous head reproduces both defects (protected payload deleted for bracket/backslash/trailing-newline; lookalike siblings wrongly skipped for */?/[). The contributed suite passes 12/12 in an unprivileged sandbox; the four root-only mount cases were not run here.

Not blocking this merge, tracked separately as a follow-up: mount_case still ignores the cleanup exit status and assert_owned_mount_root only checks payload presence, so a cleanup that exits early would still pass that case; the metacharacter pathname cases also have no regression tests yet. Happy to take that as a small test-only follow-up PR if you want to do it.

Thanks for the careful, well-documented revisions on this one.