Code review · P3 Task 9, head script, local launcher and runner loop¶
review-code on autopilot/P3-t09, base ba1a9db, 2026-10-30. The step is P1 Task 9. It adds:
packages/spectio/src/spectio/runner/headscript.py: the head script.spectio/head.sh, its engine arguments and environment, andwrite_launch_files(params,spectio.config, the bridges);packages/spectio/src/spectio/runner/engines.py:check_engine(missing or too-old engine), the bridges directory, and the idempotent unpack of nf-spectio;packages/spectio/src/spectio/runner/local.py:LocalLauncher(setsid bash head.sh, adopt, poll, cancel with the PID check of dev-env §15.3 req. 11);packages/spectio/src/spectio/runner/loop.py:RunnerLoop(leaselaunchandcancel, read spools, poll heads);packages/spectio/src/spectio/runner/rundir.py: run directories walked one private component at a time (req. 10), and the run manifest;runner/types.py(LaunchSpec,LaunchRef,Command,RunnerSink,Launcher),runner/spool.py,loginshell.py(the process-group kill of FU-P3-13),cli/init.pyandcli/doctor.py(the unpack);packages/spectio/tests/test_headscript.py,test_local_launcher.py, and the engines test in.github/workflows/pipelines-synthetic.yml.
The reviewers were code-reviewer and security-reviewer, independent and in parallel. Format: README.md. The head and its files are owned by schedulers §3.1.1; the run directory and the signal rules by dev-env §15.3; the plugin copy by architecture §13.8.
| Round | Head or range | code-reviewer | security-reviewer |
|---|---|---|---|
| 1 | e3e1306 (the step) |
changes (1 blocker, 1 major, 6 minors) | changes (0 blockers, 1 major, 5 minors) |
| 2 | 850d70a (umask, cancel before start) |
changes (1 blocker, 0 majors, 3 minors) | approve (0 blockers, 0 majors, 4 minors) |
| 3 | 4674d03 (cancelled launch ends at once) |
approve (0 blockers, 0 majors, 1 minor) | changes (0 blockers, 2 majors, 3 minors) |
| 4 | 574b25a (stale SIGKILL, plugins directory) |
changes (0 blockers, 3 majors, 5 minors) | changes (0 blockers, 1 major, 4 minors) |
| 5 | a70154d (fixes of round 4) |
changes (1 blocker; minors R5-02 to R5-04) | changes (1 major; minors R5-02 to R5-06) |
| 6 | 20f6823 (fixes of round 5) |
approve (0 blockers, 0 majors, 3 minors) | approve (0 blockers, 0 majors, 2 minors) |
| 7 | polish of round 6 (CR-P3T09-R6-01 to -03) | not reviewed | not reviewed |
IDs repeat across rounds. Each round numbered its findings again, so each ID below carries its round: CR-P3T09-04 (r1) is not CR-P3T09-04 (r4). The commit messages use the bare IDs of the round they answer. Round 5 named its findings -R5-NN. Its full reports were relayed by the coordinator in summary; the counts beyond the blocker and the major are not in this record.
Test-first. For the round-5 fixes, every new or changed test was run against the code before the fix and failed for the reason it names:
- the Snakemake tests:
'--cores' is not in list,'--jobs' is not in list,--default-resourcespresent with a profile, PYTHONPATH…/pylibinstead of…/pylib:/x, andDID NOT RAISE ValueErrorfor the three bad run IDs and launch numbers; test_plugins_dir_is_absolute:~/pkept literally;test_unpack_refuses_an_untrusted_copy:DID NOT RAISE PermissionError(symlink, group-writable) and a rawNotADirectoryError(file);test_doctor_relative_plugins_dir: doctor crashed with ValueError;test_reused_pid_keeps_two_launches_apart: both launches got the same native ID;test_never_signals_a_group_the_head_does_not_lead: reportedrunningonce native IDs carried the start time, before the group check was added;test_loop_launches_reads_spool_and_ends:_dirsstill held the ended launch;test_refused_adopt_logged_once: three log records for three ticks.
For the round-6 fixes, the same: test_unpack_is_idempotent and test_unpack_private_under_umask_002 failed under umask 002 (the copy refused as drwxrwxr-x), test_unpack_refuses_a_symlink_in_the_bridge did not raise, test_doctor_refuses_an_untrusted_copy printed ok, test_login_timeout_bounded_by_a_detached_child hung until the 300 s timeout, and test_redelivered_launch_after_a_cancel_with_no_native_id timed out with the head still running.
Gate at afe0aac, from the worktree:
uv run --locked pytest -q # 297 passed, 15 deselected
uv run --locked ruff check . # All checks passed
uv run --locked ruff format --check . # 47 files already formatted
uv run --locked mypy packages # no issues in 46 source files
uv run --locked lint-imports # 4 contracts kept, 0 broken
uv lock --check # OK
Gate at bf27893: pytest 302 passed, 15 deselected; ruff, ruff format, mypy, lint-imports and uv lock --check clean. Under umask 002, test_headscript.py (27 passed), test_local_launcher.py and test_doctor.py pass. test_spool_reader.py (Task 3, not in this diff) fails under umask 002: its fixtures write spool files with the umask's mode, which the reader refuses; that is the umask-002 CI job of SEC-P3T09-R5-06.
CODE step=P3-t09 review=approve b/M/m=3/4/18+ security=approve b/M/m=0/5/18+ rounds=6 simplify=changed -10/+6 tests=pass
The counts are per reviewer over rounds 1 to 4, each finding counted in the round that raised it, plus round 5's blocker and major and round 6's minors; round 5's minors are not counted here.
Findings¶
The finding texts are summarised from the reviewers' reports. A pair such as CR-P3T09-06 (r1) / CR-P3T09-05 (r4) is one topic seen twice.
| Id | Severity | Finding | Status |
|---|---|---|---|
| CR-P3T09-01 (r1) / SEC-P3T09-01 (r1) | blocker / major | Under umask 002 the head script wrote head.pid, head.exit and head.out group-writable; read_small refuses them, so the local mode never reported a head and cancel never signalled. |
fixed in 850d70a: umask 077 in the outer script, the inherited umask given back to the engine (test_head_files_private_whatever_the_umask, umask 002 and 000) |
| CR-P3T09-02 (r1) | major | A cancel before the head started was acked as failed, and the launch that followed still ran the whole pipeline. | fixed in 850d70a: .spectio/cancel is written and acked; _launch starts no head when it is there; a head started in the same lease checks it before the engine runs (test_cancel_before_the_head_starts, test_cancel_with_a_stale_ref_after_the_launch) |
| CR-P3T09-03 (r1) | minor | A local Snakemake launch passes no --cores; whether it runs was [to be validated]. |
fixed in 924408c, as CR-P3T09-04 (r4) |
| CR-P3T09-04 (r1) | minor | Every Snakemake run ends end_grace_s (60 s) after its head exits, though its exit code is known and the spool was already read in that tick. |
open, follow-up: the grace still applies to every ended report without a failure (loop.py _report) |
| CR-P3T09-05 (r1) | minor | An unreadable head file was logged at every poll. | fixed in 850d70a: once per head (test_unreadable_head_logged_once) |
| CR-P3T09-06 (r1) | minor | PYTHONPATH replaced the login profile's. | fixed in 924408c, as CR-P3T09-05 (r4) |
| CR-P3T09-07 (r1) | minor | run_id unquoted in a comment line of head.sh. |
fixed in 924408c, as CR-P3T09-10 (r4) |
| CR-P3T09-08 (r1) | minor | _dirs, _procs, _kill_at never pruned. |
fixed in afe0aac, as CR-P3T09-07 (r4) |
| SEC-P3T09-02 (r1) | minor | Launcher state keyed by bare PID: after PID reuse, a cancel of A reaches B. | fixed in 12fd506, as SEC-P3T09-04 (r4) |
| SEC-P3T09-03 (r1) | minor | An existing nf-spectio copy is trusted without lstat, owner or mode checks. | fixed in 231e602, as SEC-P3T09-06 (r4) |
| SEC-P3T09-04 (r1) | minor | The runner unpacks nf-spectio under its own NXF_HOME, the head reads the login profile's. | fixed in 574b25a, as SEC-P3T09-03 (r3) |
| SEC-P3T09-05 (r1) | minor | run_id and launch_n unchecked in head.sh's comment. |
fixed in 924408c, as SEC-P3T09-07 (r4) |
| SEC-P3T09-06 (r1) | minor | run_login is unbounded after a timeout: communicate() without a timeout waits for a setsid descendant that holds the pipes, and check_engine runs inside the runner tick. |
fixed in fa5838e, as SEC-P3T09-R5-04 |
| CR-P3T09-03 (r2) | blocker | A launch cancelled before its head started was never reported with the default 60 s grace. | fixed in 4674d03: the report carries a head_lost failure and skips the grace (test_cancel_before_the_head_starts_skips_the_spool_grace) |
| CR-P3T09-04 (r2) | minor | A redelivered launch whose head already runs, leased after a cancel with no native ID, is adopted but never signalled. | fixed in e449e51: the started branch cancels the adopted head when the marker is there (test_redelivered_launch_after_a_cancel_with_no_native_id) |
| CR-P3T09-05 (r2) | minor | Launcher state never pruned (with _warned). |
fixed in afe0aac, as CR-P3T09-07 (r4) |
| CR-P3T09-06 (r2) | minor | .spectio/cancel, like the other head files, is per run, not per launch. |
open, hand-off to Task 30, as CR-P3T09-11 (r4) |
| SEC-P3T09-02 (r2) | minor | run_id in head.sh unchecked; also, LaunchPayload.location accepts NUL and control characters. |
run_id: fixed in 924408c. location: open, Task 17, as SEC-P3T09-R5-05 |
| SEC-P3T09-03 (r2) | minor | run_login unbounded after a timeout. |
fixed in fa5838e, as SEC-P3T09-R5-04 |
| SEC-P3T09-04 (r2) | minor | Existing nf-spectio copy trusted; the plugins parent is made under the inherited umask. | the copy: fixed in 231e602. The parent: made 0700 when new, in fa5838e; an existing one's owner and mode: open, Task 21, as SEC-P3T09-R5-02 |
| SEC-P3T09-05 (r2) | minor | .spectio/cancel names no launch and is never removed: a Resume would end at once. |
open, hand-off to Task 30, as CR-P3T09-11 (r4) |
| CR-P3T09-04 (r3) | minor | A launch cancelled before its head starts ends cancelled only if the run is already cancelling when the report arrives (the projector's order); Task 17's endpoint must make it so. |
open, for the Progress page and Task 17's tests |
| SEC-P3T09-02 (r3) | major | A stale SIGKILL deadline, keyed by bare PID, kills a later head with the same PID. | fixed in 574b25a: _kill_at stores the HeadPid SIGTERM went to (test_stale_kill_never_reaches_a_later_head); rest: SEC-P3T09-04 (r4) |
| SEC-P3T09-03 (r3) | major | nf-spectio unpacked with the server's NXF_HOME; the engine looks with the login shell's. | fixed in 574b25a: NXF_PLUGINS_DIR exported after the login profile (test_engine_finds_the_unpacked_nf_spectio); rest: SEC-P3T09-05 (r4) |
| SEC-P3T09-04 (r3) | minor | run_login unbounded after a timeout (a regression of the FU-P3-13 fix). |
fixed in fa5838e, as SEC-P3T09-R5-04 |
| SEC-P3T09-05 (r3) | minor | Existing nf-spectio copy trusted; optionally compare a digest. | fixed in 231e602 (owner and mode); the digest: open, Task 21, as SEC-P3T09-R5-02 |
| SEC-P3T09-06 (r3) | minor | run_id in head.sh unchecked. |
fixed in 924408c |
| CR-P3T09-04 (r4) | major | Snakemake 9 refuses to run without --cores (local) or --jobs (SLURM); verified on 9.27.1 by the reviewer (its api.py:588-610). |
fixed in 924408c: with no workflow profile and neither option in extra_args, --cores all (local) or --jobs 100 (SLURM_JOBS) (test_snakemake_concurrency_without_a_profile, one case per executor) |
| CR-P3T09-05 (r4) | major | PYTHONPATH=<pylib> after the login profile replaces a module's PYTHONPATH. |
fixed in 924408c: export PYTHONPATH=<pylib>${PYTHONPATH:+:$PYTHONPATH} (test_pythonpath_prepended_to_the_login_profiles) |
| CR-P3T09-06 (r4) | major | --default-resources on the command line replaces the workflow profile's whole default-resources (verified on 9.27.1: a profile's runtime: 5 is lost). |
fixed in 924408c: passed only when no workflow profile is given (test_workflow_profile_keeps_its_default_resources); the limit is below |
| CR-P3T09-07 (r4) | minor | _dirs, _procs, _warned grow by one entry per launch. |
fixed in afe0aac: Launcher.forget(ids), called by RunnerLoop._forget (test_loop_launches_reads_spool_and_ends) |
| CR-P3T09-08 (r4) | minor | Launcher state and watched keyed by PID: after reuse, A is never polled again. |
fixed in 12fd506, with SEC-P3T09-04 (r4) |
| CR-P3T09-09 (r4) | minor | A refused adopt is logged with a traceback at every tick. | fixed in afe0aac: once per launch, then at debug level (test_refused_adopt_logged_once) |
| CR-P3T09-10 (r4) | minor | run_id unquoted in head.sh's comment. |
fixed in 924408c: RUN_ID.fullmatch and launch_n >= 1 (test_head_script_refuses_a_bad_run_id) |
| CR-P3T09-11 (r4) | minor | head.pid, head.exit, cancel and launch-failed.json do not depend on launch_n; a second launch of the same run (Resume) would be adopted, cancelled or failed. |
open, hand-off to Task 30; no code change in Task 9 |
| SEC-P3T09-04 (r4) | major | The local native ID is a bare PID: after reuse, a cancel of A SIGTERMs B's group and the reports mix. | fixed in 12fd506: native ID <pid>.<start time>, the start read from /proc/<pid>/stat right after Popen; _report and _signal require head.pid's PID and start to match (test_reused_pid_keeps_two_launches_apart) |
| SEC-P3T09-05 (r4) | minor | nf_plugin_copy keeps a relative path or a literal ~. |
fixed in 231e602: expanduser, and ValueError on a relative directory (test_plugins_dir_is_absolute, test_doctor_relative_plugins_dir) |
| SEC-P3T09-06 (r4) | minor | An existing nf-spectio copy is accepted without lstat, owner or mode checks. | fixed in 231e602: a real directory, owned by the user, not group- or other-writable, else PermissionError naming it (test_unpack_refuses_an_untrusted_copy: symlink, group-writable, file). The plugins directory itself is not checked: open, Task 21, as SEC-P3T09-R5-02 |
| SEC-P3T09-07 (r4) | minor | run_id unchecked in head.sh. |
fixed in 924408c |
| SEC-P3T09-08 (r4) | minor | _signal does not require pgid == pid. |
fixed in 12fd506: such a head.pid is never signalled and is reported unknown (test_never_signals_a_group_the_head_does_not_lead) |
| CR-P3T09-R5-01 / SEC-P3T09-R5-01 | blocker / major | copytree keeps the bridge's modes: under umask 002 the copy is 0775 and the next launch refuses it (test_unpack_is_idempotent fails under umask 002). |
fixed in fa5838e: group and other write bits cleared on the copy before the rename, a symlink in the bundled tree refused, a new plugins directory made 0700 (test_unpack_private_under_umask_002, test_unpack_refuses_a_symlink_in_the_bridge) |
| CR-P3T09-R5-02 | minor | spectio doctor checked the copy with is_dir(), not the launcher's check. |
fixed in fa5838e: doctor calls existing_copy and prints its refusal as fail (test_doctor_refuses_an_untrusted_copy) |
| CR-P3T09-R5-03 | minor | A launch must leave active_launches only after the runner's ended report, or forget drops a SIGKILL still due for a head that ignored SIGTERM. |
open, Task 17 (the server side of RunnerSink) |
| CR-P3T09-R5-04 | minor | --profile in extra_args is not taken as setting --cores or --jobs, so the head adds its own beside a profile given that way. |
open, known limit: give the profile through the launch's profiles |
| SEC-P3T09-R5-02 | minor | A manifest or digest check of the nf-spectio copy, and the owner and mode of an existing plugins directory. | open, Task 21 (the wheel that bundles nf-spectio) |
| SEC-P3T09-R5-03 | minor | The tmux launcher needs its own per-process identity, as the local native ID <pid>.<start>. |
open, Task 10 |
| SEC-P3T09-R5-04 | minor | run_login is unbounded after a timeout (SEC-P3T09-06 (r1)). |
fixed in fa5838e: communicate(timeout=5), then the pipes closed, wait(timeout=5) and 124 (test_login_timeout_bounded_by_a_detached_child) |
| SEC-P3T09-R5-05 | minor | LaunchPayload.location accepts NUL and control characters. |
open, Task 17 (the launch endpoint) |
| CR-P3T09-R6-01 | minor | The continuation lines of run_login's docstring are mis-indented. |
fixed in the round-6 polish commit |
| CR-P3T09-R6-02 | minor | The SEC-P3T09-04 (r2) row of this record had no Status cell. |
fixed in the round-6 polish commit |
| CR-P3T09-R6-03 | minor | This record said FU-P3-13's wait was open, and described CR-P3T09-R5-03 inexactly. | fixed in the round-6 polish commit |
| SEC-P3T09-R6-01 | minor | The temporary copy is writable by group or others (under umask 002) until _make_private runs, before the rename. |
open, Task 21: compare the digest after _make_private, or create the temporary directory at 0700 first |
| SEC-P3T09-R6-02 | minor | A stale per-run .spectio/cancel now also SIGTERMs a running head when its launch is delivered again (e449e51). |
open, Task 30: scope the marker by launch_n |
| SEC-P3T09-R5-06 | minor | No CI job runs the tests under umask 002, which hid R5-01. | open, dev-env follow-up; under umask 002 test_spool_reader.py fails today (its fixtures, see the gate above) |
Closed from earlier reviews: SEC-P3T3-09 (rest), CR-P3T04-11 and CR-P3T04-05 (confirmed by both reviewers in rounds 1 to 4), and FU-P3-13 (the process-group kill of run_login; its unbounded wait fixed in fa5838e (SEC-P3T09-R5-04)).
Round 5 fixes¶
On autopilot/P3-t09, after 574b25a:
| Commit | Findings |
|---|---|
924408c fix(runner): Snakemake concurrency, profile resources and PYTHONPATH; check the run ID |
CR-P3T09-04 (r4), CR-P3T09-05 (r4), CR-P3T09-06 (r4), CR-P3T09-10 (r4), SEC-P3T09-07 (r4) |
231e602 fix(runner): an absolute plugins directory and a private nf-spectio copy |
SEC-P3T09-05 (r4), SEC-P3T09-06 (r4) |
8a806b2 test(runner): type the bad run ID and launch number cases for mypy |
(the test of 924408c) |
12fd506 fix(runner): a local native ID unique per process; signal only a group the head leads |
SEC-P3T09-04 (r4), CR-P3T09-08 (r4), SEC-P3T09-08 (r4) |
afe0aac fix(runner): forget heads of ended launches; log a refused adopt once |
CR-P3T09-07 (r4), CR-P3T09-09 (r4) |
Round 6 fixes¶
On autopilot/P3-t09, after a70154d:
| Commit | Findings |
|---|---|
fa5838e fix(runner): a private nf-spectio copy under any umask; doctor's check; bounded run_login |
CR-P3T09-R5-01, SEC-P3T09-R5-01, CR-P3T09-R5-02, SEC-P3T09-R5-04 (SEC-P3T09-06 (r1)) |
e449e51 fix(runner): a launch delivered again after a cancel with no native ID is cancelled |
CR-P3T09-04 (r2) |
bf27893 test(runner): two fixtures that depended on the umask |
(umask 002 runs of test_local_launcher.py and test_doctor.py) |
For the Progress page¶
The Task 9 section of P3-M2 is written: P3-M2 · Task 9, with its findings and open questions. It carries:
- Assumption (CR-P3T09-04 (r4)). Snakemake 9 needs
--cores(local) or--jobs(SLURM). When the launch has no workflow profile andengines.extra_args.snakemakeholds none of--cores,--jobs,-c,-j, the head passes--cores allon the local executor and--jobs 100on SLURM. 100 is Spectio's own cap (SLURM_JOBSinheadscript.py), twice thejobs: 50oftests/pipelines/synthetic/snakemake/profiles/slurm/config.yaml; whether sites' per-user limits (MaxSubmitJobs) want less is [to be validated]. With a profile, the profile sets them. - Limit (CR-P3T09-06 (r4)). With a workflow profile,
[slurm] partitionandaccountfromspectio.tomlare not passed to Snakemake:--default-resourceson the command line would replace the profile's wholedefault-resources. The profile must name them. Merging the two (reading the profile's YAML) was left out: profiles come asconfig.yamlor versioned files, in list or mapping form. - Native ID (SEC-P3T09-04 (r4)). The local launcher's native ID is
<pid>.<start time in clock ticks>, not a bare PID. Task 10 (tmux) chooses its own. - Interface.
RunnerLooptakessettingsandend_grace_s, which the plan's signature does not have;Launchergainsforget(ids). - Dependency on Task 7.
--logger spectiois rendered as the plan says; the tests use stub engines. - Hand-off to Task 30 (CR-P3T09-11 (r4), CR-P3T09-06 (r2), SEC-P3T09-05 (r2)).
head.pid,head.exit,cancelandlaunch-failed.jsonare per run, not per launch: scope them by launch, or clear them when a new launch starts. - Hand-off to Task 17 (CR-P3T09-04 (r3)). A run cancelled before its head starts ends
cancelledonly if the cancel endpoint moves it tocancellingbefore it enqueues the command; carry an end-to-end case into Task 17's tests. - Known limit (CR-P3T09-R5-04). A
--profileinengines.extra_args.snakemakeis not taken as setting--coresor--jobs; give the profile through the launch's profiles. - Follow-ups, each with its owner:
- a manifest or digest check of the nf-spectio copy, and an existing plugins directory's owner and mode: Task 21 (SEC-P3T09-R5-02);
- a per-process identity for tmux heads: Task 10 (SEC-P3T09-R5-03);
- control characters in a launch's
location: Task 17 (SEC-P3T09-R5-05); - a CI job that runs the tests under umask 002: dev-env follow-up (SEC-P3T09-R5-06);
- a launch leaves
active_launchesonly after the runner's ended report: Task 17 (CR-P3T09-R5-03); - Snakemake launches wait the 60 s spool grace although their exit code is known: follow-up (CR-P3T09-04 (r1));
- the relaunch head files: Task 30 (above);
- the temporary nf-spectio copy is writable before
_make_private: Task 21 compares the digest after it, or creates the temporary directory at 0700 first (SEC-P3T09-R6-01); - a stale per-run
.spectio/cancelalso SIGTERMs a running head whose launch is delivered again: Task 30 scopes the marker bylaunch_n(SEC-P3T09-R6-02).