Code review · P3 Task 4, the projector¶
review-code on autopilot/P3-t04, base 7a18b4c, 2026-10-30. The step is P1 Task 4. It adds:
spectio_server.projector:ingestandapply_head_report(ingest.py), theProjectionof one run and launch (runs.py), and the Nextflow mapping (nextflow.py);spectio_server.ids.new_id, ULID row IDs with a type prefix;- migration
0002:tasks.name,launches.culprit_attempt_id, the indexix_tasks_run_keyand the unique attempt keyuq_attempts_key; - in
spectio.runner.types:HeadReport,HeadStateandFailure; inspectio.runner.spool:try_read_spool; packages/spectio-server/tests/test_projector_nextflow.py, 34 tests.
The reviewers were code-reviewer and security-reviewer, independent and in parallel. Format: README.md. What the projector does is owned by api §7, §7.1, §7.2 and §8.2; the task key by synthesis §2ter.
| Round | Head or range | code-reviewer | security-reviewer |
|---|---|---|---|
| 1 | 14f70b5 (the step) |
changes (0 blockers, 2 majors, 5 minors) | changes (0 blockers, 1 major, 3 minors) |
| 2 | fe49daf (the round-1 fixes) |
changes (0 blockers, 2 majors, 4 minors) | approve (0 blockers, 0 majors, 4 minors) |
| 3 | cec069d (the round-2 fixes) |
approve (0 blockers, 0 majors, 4 minors) | approve (0 blockers, 0 majors, 2 minors) |
| 4 | cec069d..08ee6fb (the post-approval fix) |
changes (0 blockers, 1 major, 1 minor) | approve (0 blockers, 0 majors, 1 minor) |
| 5 | the fix of round 4's major, 0ce1726 |
approve (0 blockers, 0 majors, 4 minors) | approve (no findings) |
Minors include the simplify: findings, as in the earlier records.
Rounds. Two fix rounds (fe49daf, cec069d), then both reviewers approved in round 3: within review-code step 4. After approval one minor that was a defect, CR-P3T04-07 (r3), was fixed in 08ee6fb and its range reviewed (round 4). That fix regressed: round 4's major CR-P3T04-08 (r4). It was fixed in 0ce1726 and reviewed again (round 5), which approved. Round 5 follows the p3-task workflow as changed on autopilot/integration in 85a5a84 ("fix and re-review a regression from post-approval fixes instead of blocking"). The text of review-code step 4 still says "one more review" after approval. Recorded in inbox #78.
IDs repeat across rounds. This file did not exist while the rounds ran, so each round numbered its findings from -03 (code) or -02 (security) again. Below, each ID carries its round: CR-P3T04-03 (r2) is not CR-P3T04-03 (r1). The commit messages and code comments use the bare IDs; the round of each is given in the Status column.
Test-first. The worker's record: each fix began with a failing test. Test command, passing at 318e30c (34 tests in this file, 186 in the workspace):
uv run pytest packages/spectio-server/tests/test_projector_nextflow.py -v
CODE step=P3-t04 review=approve b/M/m=0/5/18 security=approve b/M/m=0/1/10 rounds=5 simplify=changed -13/+12 tests=pass
The counts are per reviewer over the five rounds, each finding counted in the round that raised it. The same problem raised again in a later round counts again: try_read_spool counting every read was raised five times (CR-05 in rounds 1, 2 and 3; SEC-03 in rounds 1 and 2; SEC-02 in round 3). code-simplifier ran after round 5: commits 8756892, dc87f35 and 318e30c, −13/+12. The 186 tests pass after it.
Measured by the worker on a scratch benchmark (commit cec069d), before and after the round-2 indexes: one onFlowComplete on a 20k-task run, 24.75 s → 0.02 s (0.23 s with all 20k attempts open); 2,000 task events over 100k task rows, 38 s → 2.4 s. The "before" figures are code-reviewer's, round 2. Not re-run by doc-writer; test_projector_queries_use_indexes checks the query plans instead.
Findings¶
The finding texts are summarised from the round summaries and the fix commits' messages. A pair such as CR-P3T04-05 (r1) / SEC-P3T04-03 (r1) is one problem found by both reviewers.
| Id | Severity | Finding | Status |
|---|---|---|---|
| CR-P3T04-01 (r1) | major | HeadReport.state was an open string, and the projector relied on two magic values in it: running started a run, and succeeded or failed in launches.head_state meant "the engine already ended this launch". A launcher that named its own state succeeded would make the next onFlowComplete look like a duplicate. |
fixed in fe49daf: HeadState is a Literal of the launcher's states (starting, running, exited, unknown); onFlowComplete dedupes on the terminal run state alone. The vocabulary is now in api §7 launches |
| CR-P3T04-02 (r1) | major | The task key is process + tag. All untagged instances of one process collapsed into one task; their attempts collided on (task, launch, attempt 1) and task_counts undercounted. |
fixed in fe49daf: an untagged task is keyed on process + Nextflow's task name, in the new tasks.name (migration 0002) (test_untagged_instances_of_one_process_are_distinct_tasks); assumed, inbox #77; written into synthesis §2ter and api §7 by this step's docs |
| CR-P3T04-03 (r1) | minor | failure_class == "tool_error" was both the class and the marker of the culprit; a RETRY failure got no class; the culprit tie-break ordered by random-within-a-millisecond ULIDs, where api §8.2 says seq. |
partly fixed: the culprit is kept in launches.culprit_attempt_id, set in seq order (0ce1726), so a later class change cannot lose it. Open: a failure with errorAction RETRY or IGNORE still gets no class until the classification of schedulers §3.5 (P1 Tasks 28–29); api §7.2 now says so |
| CR-P3T04-04 (r1) | minor | The rows the projector writes (tasks, attempts, events) have created_by NULL, while the store's own contract says Spectio's rows name the system principal. |
open, follow-up FU-P3-03 (Progress page): no system principal row exists yet |
| CR-P3T04-05 (r1) / SEC-P3T04-03 (r1) | minor | try_read_spool drops read_spool's max_bytes; it counts spool_refused and logs a warning on every read while .spectio stays refused; its test is in the server package. |
open, P1 Task 9 (the runner loop, its caller); raised again as CR-05 (r2), CR-05 (r3), SEC-03 (r2) and SEC-02 (r3) |
| CR-P3T04-06 (r1) | minor | simplify: Projection.move returns a bool that no caller reads. |
fixed by code-simplifier in 8756892 |
| CR-P3T04-07 (r1) | minor | Docs: (a) P1 Task 4's interface line names tasks_total and counts, which the schema does not have; (b) "summary.tasks.total is the sum when tasks_expected is unset" is an inference the spec should own; (c) the 25.10.9 toolerr fixture shows trace.status ABORTED, evidence for an item of api §7.2. |
(a) open, next /apply-findings (inbox #78); (b) and (c) fixed by this step's docs, in api §7 summary and §7.2 |
| SEC-P3T04-01 (r1) | major | A spool event with a malformed string trace.exit raised inside ingest; the whole transaction rolled back and the cursor was never stored, so the run's projection was stuck for good (§15.3 req. 6). |
fixed in fe49daf: a string exit is read only when it is ASCII digits within int64, else null; each event is logged and projected in its own savepoint, and one that raises is counted projection_error (test_req6_malformed_trace_exit_dropped, test_projection_error_drops_one_event_only) |
| SEC-P3T04-02 (r1) | minor | apply_head_report stores state, failure.class_ and failure.message unchecked and uncapped; runs.failure is JSON with no CHECK. |
open, raised again as SEC-04 (r2); P1 Tasks 9 to 11, which build head reports |
| SEC-P3T04-04 (r1) | minor | A Nextflow run becomes succeeded from the spool's onFlowComplete alone; a later head report with a non-zero exit contradicts nothing. |
open, accepted for 0.0.1: only the user's own processes write the spool. To revisit with the network ingest of api §14 |
| CR-P3T04-03 (r2) | major | The projector's queries scaled with the whole index: no index served the task lookup, the attempt lookup or sync_tasks. Measured: one onFlowComplete on a 20k-task run held the writer for 24.75 s. |
fixed in cec069d: ix_tasks_run_key and uq_attempts_key; end_launch re-reads only the tasks it closed (RETURNING), 500 IDs at a time (test_projector_queries_use_indexes, test_end_launch_syncs_only_the_tasks_it_changed, test_attempt_key_is_unique) |
| CR-P3T04-04 (r2) | major | The per-event except Exception wrote no log line, and it also caught database errors, so a disk-full error dropped the event and moved the cursor past it. |
fixed in cec069d: log.exception with the event ID; OperationalError propagates, so the batch rolls back and the cursor stays (test_storage_error_rolls_back_the_batch) |
| CR-P3T04-05 (r2) | minor | try_read_spool counts and warns on every read (as CR-05 (r1)). |
open, P1 Task 9 |
| CR-P3T04-06 (r2) | minor | simplify: test_req6_malformed_trace_exit_dropped is misnamed; nothing is dropped. |
open: code-simplifier may not rename tests; a rename such as test_req6_malformed_trace_exit_is_null is left to the next change of this file |
| CR-P3T04-07 (r2) | minor | tasks.name and the untagged key are not in api §7 or synthesis §2ter, though the plan's Process says the owning spec changes in the same PR. |
fixed by this step's docs: api §7 tasks, synthesis §2ter |
| CR-P3T04-08 (r2) | minor | _failure picked its culprit at random when two attempts in one batch carried TERMINATE (same created_at, IDs not monotonic within a millisecond). |
fixed in 08ee6fb, then reworked in 0ce1726 (CR-P3T04-08 (r4)) |
| SEC-P3T04-02 (r2) | minor | When a projection raises, the rollback also removes the event's own log row: a validated event leaves the append-only log, and the cursor moves past it. | open, assumed acceptable for 0.0.1: the line stays in the spool file, which is the source the index is rebuilt from (Task 11). cec069d now writes a log line with the traceback. Inbox #78 |
| SEC-P3T04-03 (r2) | minor | try_read_spool floods (as SEC-03 (r1)); _private_dir puts the path into its PermissionError with str, not repr, so control characters reach the log. |
open, P1 Task 9 |
| SEC-P3T04-04 (r2) | minor | apply_head_report checks no HeadReport value at runtime (as SEC-02 (r1)). |
open, P1 Tasks 9 to 11 |
| SEC-P3T04-05 (r2) | minor | onFlowCreate.sessionId and .nextflow are stored with no format check, and the first value wins; later, resume passes them to -resume and NXF_VER. |
open, follow-up before resume (not in 0.0.1); dev-env §15.3 req. 16 checks the session ID at use |
| CR-P3T04-05 (r3) | minor | try_read_spool counts reads, not dropped events (as CR-05 (r1)). |
open, P1 Task 9 |
| CR-P3T04-06 (r3) | minor | The plan's Task 4 Interfaces line still names tasks_total and counts; Task 18 builds summary from them. |
open, next /apply-findings (inbox #78); the mapping is in the Progress page and api §7 summary |
| CR-P3T04-07 (r3) | minor | The culprit tie inside one batch (as CR-08 (r2)). | fixed in 08ee6fb, reworked in 0ce1726 |
| CR-P3T04-08 (r3) | minor | simplify: Projection.move's unused bool, and _IN_CHUNK = 500 defined twice. |
fixed by code-simplifier in 8756892 and dc87f35 |
| SEC-P3T04-02 (r3) | minor | try_read_spool logs the refused path raw, once per poll, and counts every poll (as SEC-03 (r2)). |
open, P1 Task 9 |
| SEC-P3T04-03 (r3) | minor | log.exception logs a SQLAlchemy StatementError with its SQL and bound parameters, which are the event payload; the engine is created without hide_parameters. |
open, follow-up for the store (open_db) |
| CR-P3T04-08 (r4) | major | 08ee6fb turned attempts.failure_class into a first-failure flag: a second TERMINATE failure got class null instead of tool_error (schedulers §3.5), and the lookup by class would raise MultipleResultsFound once other failures are tool_error, dropping onFlowComplete. |
fixed in 0ce1726: every TERMINATE failure is tool_error; the launch's first is kept in launches.culprit_attempt_id, set only while null; _failure joins on it (test_first_terminate_in_a_batch_is_the_culprit checks both classes) |
| CR-P3T04-09 (r4) | minor | No test covers a culprit across batches, nor the class of the second TERMINATE attempt. | partly fixed in 0ce1726: the test checks both classes. Open: no cross-batch test |
| SEC-P3T04-02 (r4) | minor | Only application code kept "at most one tool_error per launch"; .one_or_none() would raise if it broke. |
fixed in 0ce1726: _culprit is gone; the join on culprit_attempt_id matches one row at most |
| CR-P3T04-09 (r5) | minor | launches.culprit_attempt_id and the first-TERMINATE rule are not in api §7. |
fixed by this step's docs: api §7 runs.failure and launches |
| CR-P3T04-10 (r5) | minor | launches.culprit_attempt_id is the only *_id column without a ForeignKey; a dangling ID would quietly give engine_fault. |
open, follow-up: migration 0002 is not released, so the key can be added before 0.0.1 |
| CR-P3T04-11 (r5) | minor | The obligation "the caller must not crash on read_spool's PermissionError or OSError" cannot apply to Task 4, whose ingest takes a ready batch; nothing carries it to Tasks 9 and 11. |
fixed by this step's docs: the Progress page's deferred table gives it to P1 Tasks 9 and 11, with try_read_spool as the call to use |
| CR-P3T04-12 (r5) | minor | simplify: the TERMINATE test is written twice in _task. |
fixed by code-simplifier in 318e30c |
Closed from Task 3's review (record): none. SEC-P3T3-09 (the caller checks launch_dir) stays with P1 Task 9; try_read_spool is the call it will use.