fix(validator): commit evaluation progress so it is visible while the run is in flight (#251)#252
Conversation
|
Two notes for reviewers: CI needs approval. Overlap with #250. These two are independent fixes in the same file and the code merges cleanly in either order — I verified with a trial merge that |
…flight
`_touch_progress()` ended in `session.flush()` and the worker claimed its job
with `session.flush()` too. A flush only writes into the worker's own
transaction, and `process_once()` holds that single transaction open across the
whole job body — up to `EVAL_TIMEOUT_SECONDS`, 2h by default. Under Postgres
READ COMMITTED the API process cannot read uncommitted rows, so:
* every parsed `[submission] item i/N` progress line was invisible until the run
had already finished, leaving `current_phase` / `current_message` /
`current_progress_*` blank on `/api/submissions/{id}` and on the public
submission page for the entire evaluation;
* `/api/jobs` reported a claimed job as `queued`, and the submission as
un-started, for hours.
Commit in both places instead. Committing the claim also makes it durable and
releases the `SELECT ... FOR UPDATE` row lock early; the committed
`queued` -> `running` transition is what keeps other workers off the row, so
`skip_locked` claiming is unaffected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
CI is green on this branch. I enabled Actions on my fork and ran this repo's unmodified
Run: https://github.com/kai392/minirouter/actions/runs/30106137472 That also settles the one risk I flagged earlier: the I also ran the state after both #250 and this PR are merged (JOURNAL conflict resolved by keeping both entries) — all three jobs green: https://github.com/kai392/minirouter/actions/runs/30106669053 The remaining red |
1 participant
Fixes #251
Root cause
_touch_progress()ended insession.flush(), and the worker claimed its job withsession.flush()too. A flush writes into the worker's own transaction — andprocess_once()opens one session and does not commit until after the job body returns, which is up toEVAL_TIMEOUT_SECONDS(7200s by default) later.Under Postgres READ COMMITTED the API process cannot read uncommitted rows, so for the whole run:
[submission] item i/Nline was recorded and then invisible —current_phase,current_message,current_progress_current,current_progress_totalstayednullon/api/submissions/{id}and on the public submission page until the evaluation had already finished;/api/jobsreported a claimed job asqueued, withclaimed_by/claimed_at/heartbeat_atunset;submission.status = "running"never surfaced.The entire
_PROGRESS_*_RE/_consume_progress_linepath exists only to drive that live progress, so it was doing nothing observable in production.Fix approach
Two one-line changes,
flush()->commit():services/eval_runner.py::_touch_progress— progress updates are only useful if another process can read them mid-run.worker.py::process_once— commit the claim before the long job body starts.Why this is safe for job claiming
Committing releases the
SELECT ... FOR UPDATErow lock earlier than before, but that lock was never what kept other workers off the row — theWHERE status == "queued"predicate is. After the commit the row is durablyrunning, so a competing worker's... WHERE status = 'queued' FOR UPDATE SKIP LOCKEDno longer matches it. In the old code the claim was not durable at all, so a worker that died mid-run silently released it back toqueued.Impact
Restores live progress reporting on the public submission page and in
/api/jobs, and stops a single evaluation from holding one Postgres transaction open for hours — the contention thatensure_schema()'slock_timeoutand its_add_column_if_missingcatalog pre-check were added to work around.Tests
New file
validator/tests/test_progress_visibility.py(new file on purpose — keeps this PR conflict-free with #250, which touchestest_eval_runner.py):test_touch_progress_is_committed_not_just_flushedtest_worker_commits_the_claim_before_the_job_body_runs— drivesprocess_once()with a stubbed evaluator and asserts a commit has already happened when the job body starts.Both fail on
mainand pass here.Risk / tradeoffs
queued; now it staysrunning. The existing exception handler still marks the jobfailedexplicitly, so only an un-catchable kill is affected.attempts/max_attempts/heartbeat_atare already onJobQueuefor a reaper to use. Note this is the opposite end of Validator worker re-queues submissions forever after uncaught evaluation errors #50, which argues the old auto-requeue is itself a bug ("retried forever, blocking the evaluation queue") — this change removes that particular symptom but deliberately does not implement the dead-lettering Validator worker re-queues submissions forever after uncaught evaluation errors #50 asks for.SYNC_EVAL_ON_SUBMIT=true(non-default), a mid-run commit means a later failure no longer rolls theEvaluationRunrow away; the attempt stays recorded instead of vanishing. That seems preferable, but flagging it._local_attempt()still does not forwardon_lineto_run_bash_stream(), so local / local-fallback runs parse no progress lines at all. Kept out to stay reviewable — happy to send it separately.Verification
The validator job needs POSIX
ptyand a Postgres service, neither available on my machine, so I ran the suite locally against SQLite with those modules stubbed:The same 3 failures occur on unmodified
main— they come from my SQLite stand-inconftestlacking the real fixture'sTRUNCATEreset, not from this change. The delta is exactly the 2 new tests.ruff checkis clean on all changed files.I also confirmed the
validator_sessionfixture stays isolated under this change: it binds the Session to a Connection that already has an open transaction, so SQLAlchemy 2.0 joins via SAVEPOINT and an innersession.commit()is still undone by the fixture's outertransaction.rollback().🤖 Generated with Claude Code