Code review · P3 Task 29, the failure summary and resources per job¶
review-code on autopilot/P3-t29, base fdcf43b, 2026-11-01. The step is P1 Task 29, the second task of milestone M8 (P2 §8). It calls sacct, which loads a database other users share (req. 15). It adds:
packages/spectio/src/spectio/runner/sacct.py: checked job IDs, the stored tool, the argument list and environment, the 10 s kill, parsing field by field,SacctLimiter;packages/spectio-server/src/spectio_server/failure.py: the Snakemake message parser, resources from the trace,job_infoand benchmark files,Accounting(the cache, its thread, the retries),build_failures,task_resources;- the read API:
failures,failures_totalon the run detail;attempt_n,resourceson task items; migration0007(attempts.scheduler);spectio initstores thesacctpath; - tests:
packages/spectio/tests/test_sacct.py(11 tests, 14 cases),packages/spectio-server/tests/api/test_failure.py(23 tests, 25 cases),web/src/api.test.ts.
The reviewers were code-reviewer and security-reviewer, independent and in parallel. Format: README.md. The rules are owned by dev-env §15.3 req. 15, architecture §13.7, api §5, §7 and §14.1; the notes of 2026-11-01 in each are assumed under inbox #95.
| Round | Head | code-reviewer | security-reviewer |
|---|---|---|---|
| 1 | 65ce213 (the step) |
changes (1 major, 8 minors, 3 simplify) | approve (5 minors) |
| 2 | 72de702 (only ended rows kept, asked again on lag) |
approve (4 minors, 2 simplify) | approve (6 minors) |
| 3 | d070fd6 (--user and WorkDir, tries pruned, deep cache read as empty) |
approve (4 minors, 1 simplify) | approve (2 minors) |
| simplify | 598522f |
code-simplifier: changed, −11 +4 (three commits) |
The reports were relayed by the coordinator in summary. Finding IDs restart in each round, so the same ID can name different findings; this record writes the round after each ID.
Test-first. The worker reports that each fix was written as a test first and seen to fail. doc-writer re-checked the round-3 fix (d070fd6): with failure.py and sacct.py of 72de702 put back, 6 of the 7 selected tests fail, among them test_cache_file_too_deep with RecursionError and the lag tests on assert {'8812401': 3} == {}; with HEAD restored all 7 pass. The seventh, test_sacct_other_workdir_not_kept, also passes on 72de702, because that parser drops a 9-field line whole. A mutation shows it does test the match: with _answers() at HEAD ignoring WorkDir, it fails on 'available' == 'unavailable' (P3-M8 Evidence).
Gate at 598522f, from the worktree, re-run by doc-writer:
uv run --locked pytest packages/spectio/tests/test_sacct.py \
packages/spectio-server/tests/api/test_failure.py -v # 39 passed
uv run --locked pytest -q # 650 passed, 17 deselected
uv run --locked ruff check . # All checks passed!
uv run --locked ruff format --check . # 105 files already formatted
uv run --locked mypy packages # Success: no issues found in 104 source files
uv run --locked lint-imports # Contracts: 4 kept, 0 broken.
uv lock --check # Resolved 55 packages
cd web && npx vitest run src/api.test.ts # 13 passed
P1 Task 29 Step 4 on slurm-mini is not run: Task 22 is not built.
CODE step=P3-t29 review=approve b/M/m=0/1/16 security=approve b/M/m=0/0/13 rounds=3 simplify=changed -11/+4 tests=pass
The counts add up the three rounds per reviewer. The six simplify findings (CR-P3T29-10 to -12 r1, -06 and -07 r2, -09 r3) are not counted.
Findings¶
| Id | Severity | Finding | Status |
|---|---|---|---|
| CR-P3T29-01 (r1) | major | Any sacct answer was cached for good, null and non-terminal rows included: a failure opened while slurmdbd lags showed "unavailable" or partial values for the life of the run. |
fixed in 72de702: only a row in an ended state is kept; otherwise asked again on the next view, at most 3 calls, then null. test_sacct_lag_asked_again (2 cases), test_sacct_lag_capped |
| CR-P3T29-02 (r1) | minor | A Nextflow task on executor 'local' has a PID as native_id; sacct is asked for it and can return another job's facts. |
fixed in d070fd6, with SEC-P3T29-01 (r1) |
| CR-P3T29-03 (r1) | minor | Snakemake failures never queue sacct: their max_rss_bytes and elapsed_s appear only if the tasks page was opened first, and accounting reads "unavailable" with accounting on. |
open: as specified (architecture §13.7: no sacct for Snakemake failures), but the "SLURM accounting unavailable" wording is wrong for them. For P1 Task 32's screen and FU-P3-32 |
| CR-P3T29-04 (r1) | minor | _scheduler replaces the trace's req_mem_bytes and time_limit_s with sacct's even when sacct parsed them as null (UNLIMITED, an odd ReqMem). |
open: failure.py _scheduler still sets them from the row at 598522f. FU-P3-32 |
| CR-P3T29-05 (r1) | minor | A deeply nested sacct.json raises RecursionError: GET /runs/{id} answers 500, every time. |
fixed in d070fd6, with SEC-P3T29-02 (r2) |
| CR-P3T29-06 (r1) | minor | No test checks that message is set on the culprit attempt and null on the others. |
open: no test asserts message at 598522f. FU-P3-32 |
| CR-P3T29-07 (r1) | minor | The RunDetail.failures comment says "culprit first"; the order is by creation time. |
fixed in d070fd6 (web/src/api.ts:111) |
| CR-P3T29-08 (r1) | minor | The plugin version "2.8.0" in the cited source has no source. | no change: architecture §13.7 names the source as snakemake-executor-plugin-slurm v2.8.0 (91f51af, __init__.py, "written slurm.py below"), and the code's line range 1546-1606 is the one §13.7 lists |
| CR-P3T29-09 (r1) | minor | Each tasks-page request re-reads the benchmark file of every ended Snakemake task on the page, up to 200 opens per request and poll. | open: FU-P3-32 |
| CR-P3T29-10 (r1) | simplify | conn.execute(query).all()[: limit + 1] slices what _page already limits. |
fixed in 4ad0f1a |
| CR-P3T29-11 (r1) | simplify | The try/except ValueError in _number cannot trigger. |
fixed in 9076c54 |
| CR-P3T29-12 (r1) | simplify | test_req15_limiter takes the unused fakebin fixture. |
open: left by code-simplifier (a test of kept code); test_sacct.py:205. FU-P3-32 |
| SEC-P3T29-01 (r1) | minor | Nothing ties a sacct row to the run's own job: a local PID or a reused SLURM ID gets another job's state, reason and memory, labelled sacct, possibly another user's. |
fixed in d070fd6: --user=<euid>, WorkDir asked and matched against the expected directory. test_sacct_other_workdir_not_kept, test_req15_argv_and_id_check |
| SEC-P3T29-02 (r1) | minor | check_tool stats the stored path and Popen runs it later; parent directories are not checked; SPECTIO_SLURM_SACCT lets the environment pick the binary. |
open (again as SEC-P3T29-03 r2). FU-P3-32 |
| SEC-P3T29-03 (r1) | minor | GET /runs/{id} and POST :cancel return log_tail without Cache-Control: no-store, which req. 13 and api §5 ask for the same bytes on the files route. |
open (again as SEC-P3T29-04 r2): no no-store in runs_read.py or cancel.py at 598522f. FU-P3-32, before any shared deployment |
| SEC-P3T29-04 (r1) | minor | sacct.json is written without a size limit but read only up to 8 MiB (about 48k rows): a large run writes a cache it cannot read back. |
open: FU-P3-32 |
| SEC-P3T29-05 (r1) | minor | A cached reason is checked as printable ASCII (^[ -~]*$), wider than the live parser's set: an edited sacct.json can put < > " ' \`` intoscheduler.reason. | **open**:_CachedRow.reasonunchanged at598522f`. FU-P3-32 |
|
| CR-P3T29-02 (r2) | minor | The TERMINAL list is presented as from sacct(1) but was not checked; the spec gives it no source. |
fixed by the docs commit: the req. 15 note cites sacct(1) JOB STATE CODES (Slurm 26.05 page, read 2026-11-01); the nine kept states are those whose description says the job ended |
| CR-P3T29-03 (r2) | minor | The api §5 note does not list "not ended within 3 calls" as a cause of unavailable; inbox #95 rule 3 lacks it. |
fixed by the docs commit: api §5 note and #95 rule 3 |
| CR-P3T29-04 (r2) | minor | _tries is never pruned. |
fixed in d070fd6 (keyed by run and job, removed when final); one leak left, CR-P3T29-07 (r3) |
| CR-P3T29-05 (r2) | minor | The RunDetail.failures comment says "culprit first". |
fixed in d070fd6 |
| CR-P3T29-06 (r2) | simplify | The redundant slice (as CR-P3T29-10 r1). | fixed in 4ad0f1a |
| CR-P3T29-07 (r2) | simplify | The unreachable except (as CR-P3T29-11 r1). |
fixed in 9076c54 |
| SEC-P3T29-01 (r2) | minor | A Nextflow native_id is sent to sacct whatever executor ran the task, with no --user; another job's terminal row is cached and shown. |
fixed in d070fd6 |
| SEC-P3T29-02 (r2) | minor | A deeply nested .spectio/sacct.json makes every GET /runs/{id} answer 500 (json.loads raises RecursionError, not caught). |
fixed in d070fd6: caught, logged, read as empty. test_cache_file_too_deep |
| SEC-P3T29-03 (r2) | minor | check_tool checks only the file, follows links, and stat and exec are not atomic: a group-writable directory above sacct lets another account swap it. |
open: FU-P3-32 |
| SEC-P3T29-04 (r2) | minor | log_tail in the run detail has neither no-store nor Referrer-Policy: no-referrer. |
open: as SEC-P3T29-03 (r1) |
| SEC-P3T29-05 (r2) | minor | Accounting._tries grows for every job that ever lagged. |
fixed in d070fd6, as CR-P3T29-04 (r2) |
| SEC-P3T29-06 (r2) | minor | Each GET /runs/{id} runs one pool job of up to 10 log tails; a few polling tabs can fill the routes' pool (4 + 16), so the files routes answer 503. | open: a self-inflicted load, not cross-user. FU-P3-32 |
| CR-P3T29-06 (r3) | minor | WorkDir is compared after normpath only, and a mismatch is not logged: under a symlinked runs root every genuine row is dropped and the job ends unavailable with no reason given. |
open: inbox #95 rule 8 (options a, b, c; (a) assumed); logging in FU-P3-32 |
| CR-P3T29-07 (r3) | minor | When a later call fails, the job goes to _failed but its _tries entry from earlier lag tries stays. |
open: probe below. FU-P3-32 |
| CR-P3T29-08 (r3) | minor | want() still queues a job whose expected directory is unknown (a native_id with no work_dir): 3 calls, then null for good. |
open: probe below. FU-P3-32 |
| CR-P3T29-09 (r3) | simplify | The guard on os.geteuid() cannot fire; joined is used once. |
fixed in 598522f |
| CR-P3T29-10 (r3) | minor | Architecture §13.7 and inbox #95 do not mention WorkDir, --user or the match. |
fixed by the docs commit: the req. 15 note "the job's own row" owns the call; §13.7's note links to it; #95 rule 8 |
| SEC-P3T29-03 (r3) | minor | WorkDir is the first free-text field asked of sacct, in line-based output with no escaping: a WorkDir holding a newline (or another str.splitlines separator) adds a forged line that replaces another job's row in the same call, with that job's expected WorkDir. Likelihood low (the pipeline's own job must set such a directory; --user limits rows to the server user), impact low (display only). |
open: probe below. FU-P3-32, before any shared deployment |
| SEC-P3T29-04 (r3) | minor | No owning spec records the new call contract (--user, WorkDir, the match) or the assumptions it rests on. |
fixed by the docs commit, as CR-P3T29-10 (r3); each assumption is marked [to be validated] there and on P3-M8 |
Probes re-run by doc-writer on 598522f (.venv/bin/python -I, the package's own functions; the call is replaced by a fake for the last two):
argv: ['--parsable2', '--noheader', '-j', '222,333', '--user=0', '--format=JobID,State,ExitCode,Reason,MaxRSS,Elapsed,ReqMem,Timelimit,WorkDir']
222 -> COMPLETED 64 GiB
after lag then failed call: _tries = {('r1', '444'): 1} _failed = {'r1': {'444'}}
where=None: calls = 3 rows = {'555': None}
- The first line is the round-3 call, as root here (
--user=0). - The second is SEC-P3T29-03 (r3): the real line of job 222 says
OUT_OF_MEMORY, a WorkDir of job 333 holding\n222|COMPLETED|…|64G|…replaces it. - The third is CR-P3T29-07 (r3), the fourth CR-P3T29-08 (r3).
Fixes by round¶
| Commit | Findings |
|---|---|
72de702 fix(server): keep only ended sacct rows, ask again on lag (CR-P3T29-01) |
CR-P3T29-01 (r1) |
d070fd6 fix(server): sacct rows checked by user and WorkDir, tries pruned, deep cache read as empty |
SEC-P3T29-01 (r1, r2), CR-P3T29-02 (r1), SEC-P3T29-02 (r2), CR-P3T29-05 (r1), CR-P3T29-04 and SEC-P3T29-05 (r2), CR-P3T29-07 (r1), CR-P3T29-05 (r2) |
4ad0f1a refactor(P3-t29): simplify task page read, drop slice already done by _page |
CR-P3T29-10 (r1), CR-P3T29-06 (r2) |
9076c54 refactor(P3-t29): simplify _number, drop except that the regex makes unreachable |
CR-P3T29-11 (r1), CR-P3T29-07 (r2) |
598522f refactor(P3-t29): simplify sacct_argv, drop impossible uid guard and one-use local |
CR-P3T29-09 (r3) |
| the docs commit | CR-P3T29-02, -03 (r2); CR-P3T29-10, SEC-P3T29-04 (r3) |
code-simplifier reran the step's check command before and after its changes (pytest 650 passed, 17 deselected; ruff, ruff format, mypy, lint-imports, uv lock --check): the same results.
For the Progress page¶
The Task 29 section is written: P3-M8, with its findings and open questions.