Skip to content

Phase 4: compute seam, run-state machine, fail-safe tick, chain shim - #27

Merged
renmengye merged 4 commits into
mainfrom
feat/phase4-loop
Aug 6, 2026
Merged

renmengye merged 4 commits into
mainfrom
feat/phase4-loop

Conversation

@renmengye

Copy link
Copy Markdown
Member

The loop's plumbing, matching the merged wake-delivery and placement designs.

  • compute: Slurm submit/status/cancel behind an injectable runner; submit_after for afterany wake jobs (refuses to clobber an existing dependency); the design's load-bearing distinction — SlurmQueryError (outage, defer) vs GONE (successful empty query) — enforced in types.
  • runstate: atomic run records (six endings validated; waiting runs must carry a deadline — no silently immortal runs), forward-compatible loads (a revert must not blind the sweep), expiring leases with O_EXCL acquire, holder handoff, mtime fallback for corrupt leases, and tombstone-rename reaping so two concurrent reapers can't double-deliver.
  • tick: heartbeat + pause sentinel; the five-layer sweep with real grace (clock starts at first terminal sighting, so the afterany job gets its full window on any-length experiments), lease-first ordering (a live wake defers even the stuck verdict), legacy-record repair, and a dry-run mode — which is what python -m autoresearch.tick runs until the phase-5 dispatcher exists, so the live loop cannot mutate state it can't follow through on.
  • chain shim: successor top-up to depth 2 (singleton + absolute begin grid, sbatch retry/backoff) FIRST so nothing below can break the chain; deploy pull via GIT_ASKPASS (the PAT never enters argv — /proc is world-readable on shared nodes); every deploy step best-effort.

Pre-merge adversarial pass: 8 confirmed findings, all fixed with regression tests — the worst three each broke the 'no run silently disappears' invariant on their own (grace≈0 destroying healthy runs via the placeholder dispatcher, corrupt leases stranding runs forever, deadline=0 disabling the floor). 213 tests.

🤖 Generated with Claude Code

renmengye and others added 2 commits August 6, 2026 09:48
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y-run CLI, mtime lease fallback, tombstone reap, deadline floor validation, forward-compat records, lease-first ordering

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 6, 2026 •

Copy link
Copy Markdown

Advisory review — not an approval. Automated findings from autoresearch; a human code owner still owns this PR. Reply to any finding you disagree with, or add the opt-out label to silence future runs on this PR.

  • "Fail loudly" misconfig error is written to /dev/null (medium confidence, scripts/tick_chain.sbatch:71)
    #SBATCH --output=/dev/null (line 23) and no --error directive mean both stdout and stderr of the job go to /dev/null until the redirect at line 77. Everything emitted before that point is discarded: the echo "tick misconfigured; missing:$missing" &gt;&amp;2 + exit 1 at lines 71-72 (the whole point of deferring set -u was to make config errors loud) and the echo "sbatch retry $attempt failed; backing off" at line 63. In practice a chain that dies because e.g. AUTORESEARCH_ACCOUNT is unset, or whose successor submissions are failing, leaves no trace at all — the operator sees only a missing heartbeat much later. Emitting these to a fixed fallback location (or moving the log redirect above the validation when AUTORESEARCH_ROOT is present) would preserve the intended diagnostics.
  • Waiting run without an experiment job id has no deadline floor and is never swept (medium confidence, src/autoresearch/runstate.py:100)
    save_record only requires a deadline when record.experiment_job_id is non-empty, and _sweep_one returns early on if not record.experiment_job_id (tick.py) with the comment "not yet submitted; not the sweep's business". A run persisted as waiting before/without a successful sbatch (submission crashed between the state write and recording the job id) therefore has: no deadline, no Slurm state to poll, no wake_attempts accumulation, and hence no path to the stuck ending — it is exactly the "silently immortal run" the module claims to forbid. Nothing else in this PR ages such records out (non-waiting states such as a crashed implementing session are likewise never touched by the sweep).
  • A persistent squeue failure silently ends the chain (low confidence, scripts/tick_chain.sbatch:39)
    When squeue fails the script sets need=0 and skips the top-up, relying on the already-queued successors. That is correct for a transient outage, but for a persistent condition (squeue unavailable on the node, $USER unset in the batch environment so squeue -u "" always errors, etc.) every tick skips the top-up and the chain expires after at most two more ticks — with no message anywhere (see the /dev/null finding). Since the log line is also lost, the failure mode is a silently dead loop rather than the advertised "a crash anywhere never breaks the chain".
  • Pending-past-deadline cancel can kill a healthy queued job; nothing re-bases the deadline (low confidence, src/autoresearch/tick.py:315)
    The elif is_pending(state) and record.deadline &gt; 0 and now &gt; record.deadline branch scancels the experiment. RunRecord.deadline is documented as "submit+walltime+slack, re-based on start", but no code in this PR performs the re-basing: the sweep does nothing at all for RUNNING jobs. On a busy cluster the queue wait can exceed walltime+slack while the job is perfectly schedulable, in which case the backup layer cancels a healthy pending experiment (and then, on the following ticks, the CANCELLED state drives wake attempts toward the stuck ending). Either the deadline must be re-based when the job is first seen RUNNING/started, or the pending branch needs a separate, queue-aware threshold.
  • reap_lease can destroy a fresh lease if os.link fails with anything but FileExistsError (low confidence, src/autoresearch/runstate.py:226)
    In the CAS-restore path only FileExistsError is handled. Any other OSError from os.link (e.g. EPERM/EXDEV/ENOTSUP on filesystems or configurations without usable hard links, EIO on NFS) propagates out of reap_lease; the freshly written lease it renamed away is then neither restored nor unlinked — it survives only as .lease.json.reaped.&lt;reaper&gt;, so the run has no lease while a live wake job believes it holds one. The next tick can then deliver a duplicate wake concurrently with the live session. Catching OSError broadly (and logging) or restoring with os.replace when link is unsupported would close it.
  • Heartbeat temp file name is not writer-unique (low confidence, src/autoresearch/tick.py:96)
    write_heartbeat writes to a fixed .heartbeat.json.tmp, unlike save_record/update_lease_holder which deliberately include os.getpid() "so two concurrent writers must not interleave into the same tmp file before the atomic replace". Two ticks overlapping (a manual run alongside the chain, or a slow tick whose singleton successor starts) can interleave into the same temp file, so the watchdog can read a truncated/garbled heartbeat. Same one-line fix as elsewhere: include the pid in the temp name.
  • submit/cancel leak OSError and TimeoutExpired instead of SlurmError (low confidence, src/autoresearch/compute.py:130)
    status wraps runner invocation in except (OSError, subprocess.TimeoutExpired) and re-raises as SlurmQueryError, but submit, submit_after and cancel call self.runner(...) unguarded. With the default _subprocess_runner, a missing sbatch/scancel binary or a 60s timeout raises FileNotFoundError/TimeoutExpired out of the documented SlurmError contract, so callers written against the documented exceptions (phase-5 dispatcher) will not handle it. tick happens to catch Exception around cancel, so the impact today is limited to future callers.
  • mktemp failure leaves ASKPASS empty and the write silently fails (low confidence, scripts/tick_chain.sbatch:85)
    ASKPASS=$(mktemp 2&gt;/dev/null || echo "") &amp;&amp; [ -n "$ASKPASS" ] &amp;&amp; chmod 700 "$ASKPASS" can leave ASKPASS empty, after which line 86's printf ... &gt; "$ASKPASS" is an ambiguous redirect and git runs with GIT_ASKPASS="" — the deploy pull then fails (fetch aborts with GIT_TERMINAL_PROMPT=0) and rm -f "" errors, all reported only as "deploy: fetch failed". Also note the generated script interpolates $AUTORESEARCH_PAT_FILE unquoted into a double-quoted shell string, so a path containing " or $ produces a broken/expanding script; operator-supplied, but cheap to guard.

Review is based only on the provided diff and file contents; docs/design/architecture.md (referenced throughout) was not included, so claims about matching the wake-delivery design could not be verified. _sweep_one also contains a vestigial if True: wrapper and two unused parameters (dispatcher, holder) — noted only as diff artifacts, not defects. The dry-run terminal path deliberately reports a would-wake on first sighting, which diverges from live behaviour (live only records terminal_seen); documented in the code, so not filed as a finding.

renmengye and others added 2 commits August 6, 2026 10:12
… flags wired, pre-top-up guards, forward-compat leases

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ard, unique tmp names, attempts contract

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@renmengye
renmengye merged commit b233748 into main Aug 6, 2026
6 checks passed
@renmengye
renmengye deleted the feat/phase4-loop branch August 6, 2026 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant