fix: fail jobs stuck running past 90min; recover re-queued completions

[guohai]

Fixes a job that hangs at running forever when its agent dies/zombies, and a completion whose result is silently dropped after the job was re-queued (the reported eval-jobs/25762).

Root causes

1. Stuck "running" forever. The reaper only re-queues a running job when its agent hasn't heartbeated in 5 min. But the agent row is shared per token — a superseded old process (still "running" the job) stops updating last_seen_at, while a new process on the same token keeps it fresh. So the reaper sees a healthy agent and never reclaims the job. There was no job-level timeout.
2. Result dropped on re-run. Re-queuing a job clears eval_agent_id (→ NULL). /complete then rejected any completion where job.eval_agent_id !== agentId (403) and discarded the results — so a valid finish arriving after a re-queue was lost.

Fixes

• Global 90-min job timeout (MAX_JOB_RUN_MINUTES = 90, module-level constant in server/index.ts). The reaper now fails any job running longer than that, regardless of heartbeat — terminal, with a clear "exceeded maximum running time" error, so it stops instead of running forever. Chosen behavior: fail (not re-queue).
• /complete no longer drops valid results. The lease is already fenced against superseded duplicates; now a completion from the current lease holder is accepted when the job is unassigned (re-queued) or assigned to this agent — rejected only if actively assigned to a different agent. Also idempotent: a duplicate completion returns "already completed" with no second result.

Verification

• A job forced to "running" for 91 min → failed by the reaper with the message + completed_at set.
• A re-queued job (eval_agent_id NULL) completed by the current agent → 200, result saved (was 403).
• Duplicate completion → idempotent (integration test added, region sa to avoid the local na daemon).
• npm run check clean; suite failures unchanged (pre-existing/environmental).

Generated with SMT smt@agora.io

[github-actions]

🤖 Claude Code Review

Reviewed the diff on the merge ref (server/index.ts, server/routes.ts, server/storage.ts, plus tests). The core design is sound — the lease-fencing + "accept when evalAgentId is null-or-mine, reject only when assigned to a different agent" logic correctly recovers a legitimate completion that arrives after a re-queue, and started_at is reset to NOW() on every claim and NULL on release, so the 90-min hard timeout can't fire prematurely on a re-run. Two real issues:

1. Idempotency guard is read-then-write, not atomic — duplicate results possible (server/routes.ts:2813)
The comment promises "a duplicate completion (agent retry) must not create a second result," but the guard is a non-transactional read (getEvalJob → check status) followed by a separate write (createEvalResult + completeEvalJob). Two concurrent completions from the same agent (a retry fired while the first request is still in-flight — exactly the retry scenario this targets) can both read status !== "completed", both pass, and both insert. There is no unique constraint on eval_results.eval_job_id (shared/schema.ts:326), so nothing at the DB layer stops the duplicate. To actually be idempotent this needs a conditional UPDATE ... WHERE status <> 'completed' that returns whether it won the transition (and gate result creation on that), or a unique index on eval_job_id.

2. failed is not treated as terminal — late completion resurrects a failed job to completed (server/routes.ts:2812)
The guard only short-circuits on status === "completed". A job the new reaper marked failed at 90 min still has eval_agent_id set to the original agent (the reaper doesn't clear it), and its lease is still current, so a late completion from that agent passes both checks, creates a result, and flips the job failed → completed. That defeats the hard cap's purpose — a run that exceeded 90 min (the reaper's definition of "agent stopped responding") gets its stale results counted into metrics anyway. The same asymmetry applies to a job that failed via jobError. Consider treating any terminal state (completed or failed) as final in the idempotency check, or having the reaper clear eval_agent_id so the different-agent branch can't be bypassed.

Minor / non-blocking

• failTimedOutRunningJobs marks the job failed but leaves eval_agent_id set and the agent's state untouched. Harmless (a failed job won't be re-released, and markOfflineAgents handles a dead agent), but clearing the agent link would also close the gap in issue 2.
• The hard timeout bypasses the retry/maxRetries path in releaseStaleJobs and fails straight out. That reads as intended for a global cap, just noting it's a different policy than heartbeat-staleness releases.
• Test coverage is good for the happy-path idempotency case, but doesn't exercise the re-queue-then-late-completion recovery path (the actual motivation) or the different-agent rejection — worth adding since those branches carry the real risk.

[github-actions]

🤖 Codex Code Review

Findings

• High: server/routes.ts:2822 allows any valid, current eval-agent lease to complete any unassigned job. Once eval_agent_id is NULL, there is no check that this token/agent ever held the job, nor even that the agent region matches the job. A buggy or malicious agent with any token can POST results for an arbitrary pending/requeued job id and mark it completed. The fix needs a durable ownership fence for requeued jobs, or at minimum region/status checks plus a persisted prior-agent/token/lease relation.

• High: server/routes.ts:2813 only treats completed as terminal, so a late /complete can overwrite a job already failed by the new 90-minute timeout. failTimedOutRunningJobs leaves eval_agent_id set, so the same agent can pass the assignment check, insert results, and completeEvalJob at server/routes.ts:2871 changes the timed-out job from failed back to completed. This contradicts the PR’s “terminal failed” behavior; reject all non-running/non-pending terminal states before saving results.

• Medium: server/routes.ts:2814 returns "Job already completed" before verifying the completed job is assigned to this token/agent. That weakens authorization and creates a job-status/existence oracle for any eval-agent token. Move idempotency handling after the assignment/token authorization check.

• Medium: The duplicate-completion idempotency is not atomic. Two concurrent /complete retries can both read a non-completed job at server/routes.ts:2809, both insert eval results at server/routes.ts:2841, and both mark the job completed. If idempotency is required, use a transaction/row lock or a unique constraint/upsert on eval_results.eval_job_id.

No injection or credential exposure issues found in the changed code.

[guohai]

Reworked the completion path to be secure (5aeca79), addressing all four findings:

• High (any-token completes unassigned job): reverted the over-broad relaxation — a completion is accepted only when the job is assigned to this agent. The normal re-run re-claims the job (eval_agent_id set again), so legitimate finishes still record; genuinely orphaned jobs are now failed by the 90-min timeout rather than needing a loose completion path.
• High (resurrect a timed-out job): finalizeRunningJob transitions only from running, so a late completion of a job already failed by the timeout stays failed and saves no result.
• Medium (status oracle): the assignment/auth check runs before any terminal/idempotency handling.
• Medium (non-atomic idempotency): finalization is now a single conditional UPDATE … WHERE status='running' RETURNING, so exactly one concurrent completion wins — no duplicate result.

Net: the primary fix is the 90-min job timeout (stuck jobs fail with a clear message instead of hanging). The completion path is now strict, terminal-safe, and atomically idempotent. Verified: duplicate completion → "already finalized"; timed-out job completed late → stays failed, 0 results.

[github-actions]

🤖 Codex Code Review

Findings

• server/storage.ts:916 — finalizeRunningJob only gates on id + status = running, but /complete authorizes against a stale pre-read. If the job is re-queued after line server/routes.ts:2809 and then claimed by another agent before line server/routes.ts:2824, the old agent can finalize the new runner’s lease and write stale results. Include the authorized evalAgentId (and ideally an attempt/lease identifier if available) in the atomic UPDATE predicate so finalization cannot cross a requeue/reclaim boundary.

• server/index.ts:232 — the maintenance order lets stale-job release win before the new 90-minute hard timeout. A job that has run >90 minutes but whose agent is also stale will be re-queued by releaseStaleJobs and have started_at cleared before failTimedOutRunningJobs runs, so it won’t fail as intended. Run the hard timeout before stale release, or make releaseStaleJobs exclude/terminally fail jobs past the max runtime.

No security injection or credential exposure issues found in the touched code.

[github-actions]

🤖 Claude Code Review

Reviewed the full diff (server/index.ts, server/routes.ts, server/storage.ts, tests/api.test.ts) on the merge ref. The core idea — atomic running → terminal finalize + a hard job-run timeout — is sound and correctly closes the duplicate-result / resurrection races. A few issues:

Correctness

1. Result becomes permanently unrecoverable if createEvalResult fails (regression vs. stated goal). server/routes.ts:2851-2866

const finalized = await storage.finalizeRunningJob(...)   // running → completed
...
} catch (resultError) {
  await storage.completeEvalJob(parseInt(jobId), "Failed to save eval results"); // → failed
  return res.status(500).json(...);
}

Because finalizeRunningJob only matches status = 'running', once a transient DB error flips the job to failed, the agent's retry (a 500 is the kind of response an agent retries) hits the authorization check, then finalizeRunningJob returns undefined, and the handler answers 200 "already finalized" — the valid results are silently dropped forever.

The old code, being unguarded on status, could re-attempt createEvalResult on retry and recover. So the atomic-finalize refactor trades "duplicate results" for "permanently lost results on a transient failure," which runs against this PR's "recover re-queued completions" intent. Consider leaving the job in running (or resetting it) on save-failure instead of flipping to failed, so a retry can still finalize-with-result; the 90-min reaper is the backstop if it never comes back.

2. Transient "completed with no result" window (order inversion). server/routes.ts:2851 then 2846

Finalize now marks the job completed before the result row is inserted (previously the result was created first, status set last). For a latency-metrics platform, a metrics query LEFT-JOINing results against completed jobs can momentarily see a completed job with no metrics. The window is a single request long, so low severity, but it's a behavioral change worth being aware of — inserting the result before finalizing (or in the same transaction) would avoid it.

Minor

• server/index.ts:12-15 — const MAX_JOB_RUN_MINUTES = 90; is declared in the middle of the import block. It works (imports hoist), but it splits the imports awkwardly; move it below them with the other module-level constants.
• failTimedOutRunningJobs leaves eval_agent_id set and doesn't touch retry_count (unlike releaseStaleJobs). That's fine for the "zombie" case, but note a legitimately long (>90min) job gets force-failed while the healthy agent keeps running; its eventual completion is dropped as "already finalized." Acceptable given 90min is the documented cap, just confirm no real eval approaches it.

Good

• Authorization ordered before finalization with the explicit "stranger can't probe job existence" rationale — correct (404 only after token+agent are validated).
• finalizeRunningJob's WHERE status = 'running' + .returning() gives a clean single-winner semantics; the idempotency test covers it.
• The timeout SQL correctly measures per-run duration since releaseStaleJobs resets started_at to NULL on re-queue.

Net: ship-worthy after reconsidering finding #1 — the unrecoverable-on-save-failure path is the one real behavioral gap.

[guohai]

Addressed finding #1 in the latest commit: on a transient createEvalResult failure, the job is now rolled back to running (resetJobToRunning) instead of flipped to failed, so the agent's retry re-finalizes with the result rather than getting "already finalized" and losing it. The 90-min reaper remains the backstop. (Finding #2 — the single-request "completed-with-no-result" window — I'm leaving as-is per your low-severity note, since result-before-finalize would reopen the duplicate-result race the atomic finalize just closed.)

[github-actions]

🤖 Codex Code Review

Findings

• P1 server/routes.ts:2817 / server/storage.ts:737 / server/storage.ts:790 / server/storage.ts:916: The “re-queued completion” path still does not save results. Both reapers clear eval_agent_id and set the job back to pending, but /complete still requires job.evalAgentId === agentId; even if that check were relaxed, finalizeRunningJob only updates status = 'running' jobs. So a completion arriving after requeue is still rejected or treated as already finalized, and the reported dropped-result case is not covered by the new test.

• P1 server/routes.ts:2809 / server/storage.ts:916: Completion authorization and finalization are not fenced in the same atomic update. The route reads a job assigned to agent A, then finalizeRunningJob updates by only id + status = running. If the job is released/reclaimed by agent B between those two operations, A’s stale completion can finalize B’s active run and insert stale results. The finalizing update should include the expected eval_agent_id/lease-owned state, or perform authorization and status transition in one statement/transaction.

Open Question

• The added test only verifies duplicate completion while the job is still assigned to the same agent; it does not exercise the PR’s described eval_agent_id NULL requeue scenario. Was accepting completions for unassigned requeued jobs intentionally dropped from scope?

[github-actions]

🤖 Claude Code Review

The change is well-reasoned and the atomic-finalize approach is sound. A few notes, all low severity:

Logic — lost-result race on duplicate completions (low, backstop exists)

server/routes.ts:2824-2868: if two completions for the same job race (agent client-timeout + retry), request A can win finalizeRunningJob (→ completed), request B gets {message: "already finalized"} with a 200, and then A's createEvalResult fails and calls resetJobToRunning (→ running) returning 500. B already told the agent "success," so the agent won't retry, and the job hangs running with no result until the 90-min reaper fails it. Narrow window and the reaper is a correct backstop, so acceptable — just flagging that "already finalized" returning success before the winner has durably saved the result is the gap. Not worth blocking on.

Consistency — agent left idle while its job is reset to running (low)

routes.ts:2829 sets the agent idle before result creation. On the resetJobToRunning path the job goes back to running still assigned to that agent (evalAgentId intact — good, that's what lets the retry re-finalize), but the agent is now idle. If the agent fetches new work instead of retrying, it could pick up a second job while still owning this one. The dispatcher/heartbeat logic likely tolerates this, but the states are momentarily inconsistent.

Maintainability — completeEvalJob is now dead code

storage.ts:929: both call sites were replaced by finalizeRunningJob/resetJobToRunning, and no test references it. Either remove it or note why it's retained.

Minor — 404 vs 403 disclosure

routes.ts:2809-2818: splitting the old combined 403 into 404 (not found) / 403 (not yours) lets an authenticated-but-unrelated agent token distinguish whether a job ID exists. Only matters for holders of a valid agent token, so effectively negligible — mentioning only because the comment claims the ordering prevents "probing job status/existence," which the 404 slightly undercuts.

Things that are correct and worth confirming stayed correct:

• failTimedOutRunningJobs keys on started_at (set to NOW() at claim, cleared to NULL on stale-release), so the cutoff measures actual run time and released jobs are excluded. Good.
• resetJobToRunning leaves started_at untouched, so the 90-min countdown continues from the original claim — the reaper backstop still fires after a failed retry. Good.
• Finalize's WHERE status = 'running' plus evalAgentId auth check correctly fences superseded/re-queued agents (release nulls eval_agent_id, so an old agent gets 403). Good.
• No schema change, so no migration needed — the columns touched all pre-exist.

Overall: solid fix, ship-able. The duplicate-completion result-loss race is the only item I'd consider hardening (e.g. have "already finalized" verify a result exists, or reorder so the result is saved before the winner is observable), but the reaper makes it non-critical.