perf(orchestrator): a feedback round does not make the user wait for training

[bbertucc]

Summary

The training a feedback round triggers — classify the correction, propose a prompt change, run the candidate against the agent's regression fixtures, then run the current prompt against them again — ran before the session was marked ready_for_review.

None of it can change the document. That was written, linted and persisted several lines earlier. So a user who submitted feedback waited through a classify call, a train call and two passes over the fixtures for work about a future document — and every upload behind them waited too, since the run holds its max_concurrent_runs slot until it returns.

It now runs after the status is set. That is the principle the contribution step immediately below it already states and follows: never block the result.

The containment is a bug fix, not a precaution. A provider error anywhere in training marked the session failed — over a document whose output.html was on disk and whose Reader had signed it off. The new e2e step proves it: with the ordering reverted and nothing else changed, that step ends run failed: openrouter 500: e2e: training call refused.

run_complete stays after the training rather than moving up with the status. It is the run's own terminal marker, and src/diagnostics.ts measures a finished run's duration up to it — writing it earlier would report a run as shorter than the time it actually held the machine.

What the widened window exposes, closed rather than noted

Moving the status flip ahead of minutes of work widens a window that already existed (runContribution has always run past the status): a client acting on ready_for_review can close the session or start a second run, and the run queue's cap is global rather than per session, so neither waits. Three consequences, all fixed here:

• GET /diagnostics could no longer see a hung training call. in_flight and in_flight_count were gated on the session status, and training now runs while the session reads ready_for_review — so a run stuck on a provider in there reported nothing in flight while still holding its slot and delaying every upload behind it, which is the one question that field exists to answer. The gate is now the window itself: ready_for_review and no terminal line written yet. Not simply "no terminal line" — failStaleSessions rewrites the status of a run whose process died and cannot append to its own log, and that run's last open call would then hang forever in the report. The existing test for exactly that caught my first attempt.
• The shared lesson bank is now written atomically. It is keyed by agent file rather than by session, and max_concurrent_runs allows more than one run at a time, so a page being extracted in one run can read the bank while another records a lesson to it. A plain writeFileSync is not atomic and loadExamples answers a partial read with [] — that page would be extracted with no lessons at all, accessibility-policy ones included, and nothing would say so. This one predates the ordering change and is reachable today with two concurrent sessions; the widened window is what made it worth closing now.
• runContribution is contained the way the training above it is. Its own two failure paths were already caught, but what is left outside them is still an fs read, and the outer catch would answer it by writing failed over a delivered document — the same wrong answer the training used to give.

The POST /close race the comment described is also corrected to what it is: forward-looking rather than live, since nothing seeds a session-built page.md today, so the only writer of that tmp path never runs for the agent this trains.

Testing

• npm run typecheck
• node --test "test/*.test.ts" — 606 passing, 3 skipped, 0 failing
• ./test/e2e.sh — 94 assertions pass, none fail
• New e2e step 9h, on a session of its own, with a POST /__fail-training control on the mock provider that fails the TASK: classify call with a 500: the round is accepted, the session still reaches ready_for_review, the document is still served (200), the round's own run_complete arrives after that (the session was ready while training was still in flight — which is the point), the failure is in the run log as feedback_training_failed, and run_complete follows the training rather than preceding it.
• Mutation-checked: reverting only src/pipeline/orchestrator.ts makes step 9h fail with run failed: openrouter 500 — so the step tests the change rather than passing alongside it. (The first draft of this step was vacuous — feedback that leaves the body unchanged makes both training functions return early, so nothing ran; it now uses feedback that actually changes the document.)
• test/diagnostics.test.ts gains the post-delivery case: a call still open while the session reads ready_for_review and no terminal line is written is reported in flight, and the same log with run_complete reports nothing. The existing "a finished run is not hung" case still passes, which is what bounds the new gate.
• test/memory.test.ts gains an atomicity case: after eight recorded lessons the bank on disk parses, holds all eight, and no temporary file is left beside it.

Checklist

• Focused change; matches surrounding code style
• No unjustified new runtime dependencies
• I agree to license my contribution under AGPL-3.0-or-later

🤖 Generated with Claude Code

https://claude.ai/code/session_012w9QjtBNYLnyKgREdG1i37

[claude]

All six checks pass, and the change itself holds up: output.html, the lint JSON and final.json are all written above the moved updateSession, so nothing the training does can reach the delivered document; the try/catch fixes a real bug rather than papering over one; keeping run_complete below the training matches src/diagnostics.ts:203 measuring to the first terminal event; and e2e 9h is non-vacuous (feedback that actually changes the body, plus the stated mutation check). No blocking issues.

Non-blocking notes

1. Same-session overlapping runs are newly reachable, and the new comment names only the /close half of it. src/pipeline/orchestrator.ts:826 flips the status before what can be minutes of work, so the CAS at src/routes/sessions.ts:487

if (!store.claimSession(s.session_id, "ready_for_review", { status: "queued", phase: "extraction", error: null })) {

now succeeds while run 1 is still inside runPipeline. RunQueue's cap is global, not per session (src/util/queue.ts:47, max_concurrent_runs: 2 in config.example.yaml:281), so run 2 does not wait for the slot — it starts alongside run 1's training. Two consequences worth naming beside the /close one:

• saveExamples is a read-modify-write with a plain writeFileSync (src/pipeline/memory.ts:112). Run 2's examplesForPrompt (src/pipeline/extraction.ts:919) can read the bank mid-write, and loadExamples's catch { return []; } (src/pipeline/memory.ts:105) turns that into "this page got no lessons" — silently, including the a11y_policy ones.
• if run 1's runContribution throws outside its own two try blocks (src/pipeline/contribute.ts:118 — loadAgent is an fs read), the outer catch at src/pipeline/orchestrator.ts:894 writes status: "failed" over a session run 2 has already delivered, or over closed. That shape pre-dates this PR, but the window went from milliseconds to a whole training round.

Reached by a client that POSTs a second /feedback (or /close) the instant the poll flips — cheap for a scripted client, unlikely for a human, hence a note.

2. The race the comment does state is not reachable for page.md today. sessionBuilt is set only by src/agents/loader.ts:68 (an agent found in tmp/<id>/agents), and the only writer of that directory is src/pipeline/feedback.ts:685, which writes target.file only when target.sessionBuilt is already true. Nothing seeds a session-built page.md, so no POST /close can currently pull one out from under in-place training. Fine as forward-looking caution — worth knowing it documents a future exposure, not a live one.

3. GET /diagnostics can no longer see a hung training call — on every feedback round, not just an interleaved one. in_flight and in_flight_count are both gated on running = status === "running" || status === "queued" (src/diagnostics.ts:197, :231, :241), and the classify/train calls now run while the session reads ready_for_review. So a run stuck waiting on the provider inside training reports in_flight: null, in_flight_count: 0 while still holding its max_concurrent_runs slot and delaying every upload behind it — which is exactly the "is it hung?" question that file's header says in_flight exists to answer. A phase marker for the post-delivery work, or relaxing that gate to "has an unmatched model_call_start", would restore it.

Accessibility impact: none on any delivered document — the HTML is written, linted and persisted before anything this PR moves; the only path from here to output is the latent torn read of the a11y-lesson bank in note 1.

[claude]

All six checks pass. Three of the four things I noted on 1e5d244 are addressed; the fourth (note 3, in_flight during the widened window) is not, because the gate that was added for it cannot fire.

Blocking

src/diagnostics.ts:243 — the new active gate is never true on a feedback round, so in_flight is still null in exactly the window this PR widened, and the test added for it cannot fail.

const active = running || (ctx.status === "ready_for_review" && !terminal);

terminal is events.find(...) over the whole session log (src/diagnostics.ts:203), and that log is one append-only file per session (src/store/paths.ts:38, src/store/runlog.ts:14) — e2e 9h reads every run from it and asserts run_complete count >= 2.

A feedback round is only accepted when the session is already ready_for_review (src/routes/sessions.ts:467), which run 1 reached by writing the status and then run_complete (src/pipeline/orchestrator.ts:361 — run 1 has no args.feedback, so no training sits between the two). So by the time run 2 is inside the training block, run 1's run_complete is in the log, terminal is truthy, and active === running === false: in_flight: null, in_flight_count: 0, identical to before the change.

The only state where active differs from running is a first run's sub-millisecond gap between updateSession and log.event("run_complete") — where no training runs.

Input that reaches it: GET /v1/sessions/{id}/diagnostics during any feedback round's training window. The run holds its max_concurrent_runs slot on a possibly-hung provider call and the field that exists to say so reports idle.

test/diagnostics.test.ts:96 passes only because its fixture log is a single run_start with no prior run_complete — a state a feedback round cannot produce — so it stays green whatever this gate does.

Scoping the terminal lookup to the current run (from the last run_start index onward) fixes both. The same first-match assumption also makes endRef measure a multi-round session up to run 1's run_complete, which is pre-existing but the same root cause — and worth fixing in the same line, since the new comment at :230-242 and at orchestrator.ts:353-359 both rest on run_complete being this run's terminal line.

Withdrawn from my review of 1e5d244

Note 1's first bullet — "run 2's examplesForPrompt can read the bank mid-write" — was wrong for a single Iris process: loadExamples and saveExamples are both synchronous fs calls on one JS thread, so two concurrent runs in the same process cannot interleave a read with a write. Withdrawn.

Non-blocking notes

1. src/pipeline/memory.ts:118-126 is correct, but the race it names isn't the one it fixes. Per the withdrawal above, "a page being extracted in one run can read this file while another run is recording a lesson to it" cannot happen in one process. The real exposure is two Iris processes sharing data_dir; there the rename does remove the torn read, but leaves the lost update — loadExamples → mutate → saveExamples is still read-modify-write, so B's rename discards A's just-recorded lesson entirely rather than half of it. Relatedly, renameSync over an open destination can fail with EPERM on Windows (Node uses MoveFileEx, not ReplaceFile as the comment says), and that throw is now swallowed by the new training catch.

2. Neither new event surfaces in GET /diagnostics. errors[] keys on run_failed || ok === false (src/diagnostics.ts:324), so feedback_training_failed and contribution_failed are absent from it. In e2e 9h the provider 500 still shows through its own model_call, but the non-model throw the new catch at orchestrator.ts:379-382 exists for (the loadAgent fs read) leaves diagnostics looking clean and is visible only in the raw ndjson.

3. No test covers the runContribution catch (orchestrator.ts:379-382); the training catch has e2e 9h.

Fixed since 1e5d244

Note 1's second bullet (runContribution is now contained, so a throw there can no longer write failed over a delivered session), note 1's naming complaint (the comment now states the /feedback half of the window), and note 2 (the session-built-agent race is labelled forward-looking).

Accessibility impact: none on any delivered document — the HTML is written, linted and persisted before everything this PR moves or contains; the atomic lesson-bank write only affects which lessons a future run's prompt carries, and only across processes.

[claude]

All six checks pass. The blocking finding from a2db9c5 — the active gate being dead because terminal was the first run's — is fixed: src/diagnostics.ts:213-215 scopes the lookup to the slice from the last run_start, and the two new fixtures in test/diagnostics.test.ts (a feedback round is read against its OWN terminal line / an earlier round's abandoned call…) are the two-round shapes that a single-run fixture could not produce, so the gate can now fail if it regresses. Scoping openCalls to currentRun as well is right, and balanced — the pre-run_start scope call has both its start and its end outside the slice. errors now carrying feedback_training_failed / contribution_failed closes note 2 from that review, and the runContribution catch closes note 3.

Non-blocking notes

1. The failStaleSessions half of the new gate does not cover the window this PR creates, so the phantom hang the comment guards against is reachable in it. src/diagnostics.ts:229

const active = running || (ctx.status === "ready_for_review" && !terminal);

The rationale above it (:222-228) is that a dead process is caught by its status being rewritten. But failStaleSessions only rewrites running/queued:

UPDATE sessions SET status = 'failed', error = 'interrupted (server restarted)', ...
 WHERE status IN ('running','queued')

(src/store/db.ts:606-609)

A process killed inside the post-delivery training window — the window this PR widens from a moment to a classify call, a train call and two fixture passes — leaves status = 'ready_for_review' with the current run's run_start in the log and no run_complete. On the next boot failStaleSessions skips that row, so active is true permanently: GET /v1/sessions/{id}/diagnostics reports the abandoned feedback call as in_flight with waiting_ms growing without bound, and elapsed_ms climbs off nowIso forever (:230, :442). Latent in the sense that it needs a restart/OOM during a feedback round's training rather than ordinary traffic, but it is the same false answer the gate was written to avoid, just entering by the status door instead of the log door. Either extend failStaleSessions to ready_for_review rows whose run never terminated, or bound the window with an event of its own (a post_delivery marker written before the training and closed after it) so the gate does not rest on a status nobody rewrites.

2. Two concurrent runs on one session re-blind in_flight for the second one. POST /feedback can claim the session during run 1's training window (src/routes/sessions.ts:487) and the queue cap is global, so with max_concurrent_runs: 2 run 2's run_start is appended before run 1's run_complete. lastIndexOf("run_start") then picks run 2, and run 1's trailing run_complete sits inside currentRun — so terminal is truthy and active === running === false throughout run 2's own training window, which is the state the new gate exists for. Same client behaviour as the overlapping-run window you already document at orchestrator.ts:1499-1517; noting it because the log is only append-ordered per process, not per run, and the new per-run slicing assumes it is.

3. docs/API.md:634 now understates errors. "errors lists failed calls" — with src/diagnostics.ts:352-359 it also lists feedback_training_failed and contribution_failed, which are the two entries that are not calls and are the reason the filter was widened.

Accessibility impact: none on any delivered document — output.html, the lint JSON and final.json are all written above the moved updateSession, and the atomic lesson-bank write only changes which lessons a future run's prompt carries (and only across processes).

[claude]

All six checks pass. Since e4e0019 only docs/API.md, src/diagnostics.ts and test/diagnostics.test.ts changed, and all three notes from that review are addressed:

• The failStaleSessions hole (note 1) is bounded. MAX_PLAUSIBLE_CALL_MS (src/diagnostics.ts:197) is derived correctly — src/providers/openrouter.ts:28,32 is 15 min × 3 attempts and src/providers/bedrock.ts:44 is 15 min, and nothing in src/ feeds timeouts.maxTotalMs from config, so an hour is genuinely above what a caller can produce today.
• The interleaved-round blinding (note 2) is fixed by counting rather than reading the slice (src/diagnostics.ts:231-233), and rounds are counted, so an interleaved second round is not read as finished is the two-run_start-before-one-run_complete shape a slice fixture cannot express.
• docs/API.md:632-635 now says what errors actually carries.

Non-blocking notes

1. The residual half of note 1: unfinished is counted over the whole file, but abandoned is scoped to currentRun, so a round with no terminal line still pins active on when the slice has no open call to bound.

const abandoned = oldestOpenAt !== null && ms(oldestOpenAt, nowIso) > MAX_PLAUSIBLE_CALL_MS;
const active = running || (ctx.status === "ready_for_review" && unfinished && !abandoned);

(src/diagnostics.ts:282-283, with openCalls/oldestOpenAt built from currentRun at :248,259)

Two shapes where oldestOpenAt is null and so abandoned can never fire:

• the process is killed in the post-delivery window between model calls — after learnFromFeedback returns, during a fixture read, or in runContribution's loadAgent fs read (src/pipeline/orchestrator.ts:372);
• the process is killed mid-call, and a later POST /feedback round then runs and completes — /feedback is accepted because the status is still ready_for_review, and lastIndexOf("run_start") puts the orphaned start outside currentRun.

Either way roundsStarted > roundsEnded holds for the life of the row and no sweep touches a ready_for_review status, so active stays true. in_flight/in_flight_count are correctly null/0 (nothing is open in the current run), but endRef = nowIso (:284) means elapsed_ms counts from the session's first event to now without bound (:471), concurrency_factor decays toward 0 (:489), and the last phase's entry in phase_durations_ms grows forever (:375) — an idle, delivered session reading as one that has been working for days. Latent: it needs a process death inside a feedback round's post-delivery window, and the field the gate was written for is now right. Bounding endRef by lastEventAt when the current run has no open call, or writing a post_delivery marker that closes, would cover both shapes.

Accessibility impact: none on any delivered document — this commit touches only the diagnostics summary, its tests and the doc that describes it; output.html, the lint JSON and final.json are still written above the moved updateSession.

[claude]

All six checks pass. Since bb8ee50 only src/diagnostics.ts and test/diagnostics.test.ts changed, and the single note from that review is addressed.

Fixed: endRef no longer runs to now for a round that will never terminate. src/diagnostics.ts:296-297 requires an open call in the post-delivery window, and both shapes I named are covered — killed between calls (oldestOpenAt === null, so endRef = lastEventAt) and killed mid-call followed by a completed later round (endRef = terminal?.ts, round 2's own line). a round killed between calls does not count the clock up for ever (test/diagnostics.test.ts:171) is the fixture that fails without the change: active is still true there, so the old line measured three days.

Non-blocking notes

1. The bound now rests on an open model call, but the longest non-model step in that window is a GitHub request with no timeout — so elapsed_ms freezes for a live run that is genuinely holding its slot. src/diagnostics.ts:296

const pending = running || (active && oldestOpenAt !== null);

proposeAgentUpdatesFromFeedback files the update issue inside the training block, above run_complete (src/pipeline/orchestrator.ts:349, src/pipeline/feedback.ts:765-779): an Octokit title search, then a create-or-comment, on new Octokit({ auth: token, baseUrl: apiBase }) (src/github/issue.ts:256) with no request.timeout and no signal. None of it is a model_call, so openCalls is empty and pending is false while it runs. With a token configured — the deployed configuration for the learning path — a stalled GitHub connection leaves the run holding its max_concurrent_runs slot indefinitely while GET /v1/sessions/{id}/diagnostics reports in_flight: null, in_flight_count: 0, and now an elapsed_ms frozen at the last fixture call, a concurrency_factor computed off that frozen elapsed, and the last phase's duration fixed — an occupied queue slot reading as a finished run.

Before this commit elapsed_ms was right in that case and wrong in the dead-process case, so the direction of the trade is the better one. What is missing is that the ceiling which makes the dead case safe is not applied to the shape that has no open call to age out. MAX_PLAUSIBLE_CALL_MS works there too: unfinished && ms(lastEventAt, nowIso) <= MAX_PLAUSIBLE_CALL_MS covers both — a live gap between calls measures to now, and a killed round stops climbing an hour after its last event, with no open call required.

Latent in the sense that it needs GitHub filing to hang rather than ordinary traffic, and no delivered document is affected either way.

2. The comment at src/diagnostics.ts:285-295 states the rule but not its cost. It lists the dead-process shapes the open-call requirement handles; it does not say that a live run stalled in a non-model step in this window is measured to its last event. If the trade in note 1 is intended, one clause saying so keeps the rationale matching the code — these comments are the only place the gate's reasoning lives.

Accessibility impact: none on any delivered document — this commit touches only the diagnostics summary and its tests; output.html, the lint JSON and final.json are all written above the moved updateSession.

[claude]

All six checks pass. Since 4185633 only src/diagnostics.ts and test/diagnostics.test.ts changed, and both notes from that review are addressed.

Note 1 is fixed — the bound no longer rests on an open model call. src/diagnostics.ts:305:

const pending = running || (active && ms(lastEventAt ?? undefined, nowIso) <= MAX_PLAUSIBLE_CALL_MS);

Both directions hold: a round killed between calls stops measuring at its last event three days out, and a live run ten minutes past its last event — the GitHub-filing shape, which logs nothing while it runs — still measures to now. The added second half of a round killed between calls does not count the clock up for ever (test/diagnostics.test.ts:191-200) is the mutation-catching fixture: under the old oldestOpenAt !== null form live.elapsed_ms is 2000, not >500_000.

Note 2 is fixed — src/diagnostics.ts:300-304 now states what the trade costs.

And the residual I raised on bb8ee50 (unfinished counted file-wide while abandoned is scoped to currentRun, so active can pin on for the life of the row) no longer has a user-visible effect: active feeds only inFlight/inFlightCount — both correctly null/0 when the current run has no open call — and pending, which is now bounded by recency either way.

Non-blocking notes

1. The contribution window after run_complete is still measured as finished — the same blind spot, one step later. src/diagnostics.ts:233 sets unfinished false as soon as roundsEnded == roundsStarted, and run_complete is written at src/pipeline/orchestrator.ts:361, above runContribution (:379), which files through the same untimed Octokit client (src/github/issue.ts:256) as the filing inside the training block. So a stalled contribution leaves the run holding its max_concurrent_runs slot while GET /v1/sessions/{id}/diagnostics reports in_flight: null, in_flight_count: 0 and an elapsed_ms frozen at run_complete — an occupied queue slot reading as a finished run. Pre-existing and untouched by this PR (runContribution has always run past run_complete), and the placement of run_complete is deliberate and correctly argued for the duration measurement; noting it only because the PR reasons explicitly about that line and newly surfaces contribution_failed from the step below it, so the two post-delivery steps now differ in whether diagnostics can see them hang.

Accessibility impact: none on any delivered document — this commit touches only the diagnostics summary and its tests; output.html, the lint JSON and final.json are all written above the moved updateSession.