feat(runs): checkpoint-resume for interrupted evaluation runs + concurrent-safe run updates

[goyamegh]

Summary

Interrupted evaluation runs (server crash / deploy / cancellation) can now be resumed in place: POST /api/storage/evaluation-runs/:id/resume re-executes ONLY the test cases without a persisted report, preserving completed reports byte-for-byte. The per-test-case reports already persisted in storage act as the checkpoint — nothing new to store, nothing to lose.

Stacked on #413 — merge that first. This branch is based on it (the useTraces polling gate is required so non-trace agents in the new integration test don't take the 10-minute trace-poll detour). Only the last 4 commits are new here.

What's included

1. Resume endpoint + UI — POST /api/storage/evaluation-runs/:id/resume (SSE, mirrors the create route); the run detail page shows a Resume (N left) button for interrupted runs with unfinished test cases. Same run id — history and stats stay coherent, resumedAt is stamped.
2. Orphan EvaluationRun recovery on boot (evaluationRunRecoveryOnBoot.ts, sister of benchmarkRunRecoveryOnBoot.ts) — stale running run docs are marked failed-with-note and become resumable. Previously they were stuck running forever with no way forward.
3. Liveness heartbeat — the executing server stamps heartbeatAt every 60s. Recovery and resume gate on heartbeat staleness (EVALUATION_RUN_STALE_AFTER_MS, default 1h), so multiple agent-health servers sharing one storage cluster can never orphan-kill or double-execute each other's active runs.
4. Concurrent-safe run-doc updates (OpenSearch) — found by verification: any run with concurrency > 1 could abort within seconds on version_conflict_engine_exception. updateResult now sets retry_on_conflict, and update() is a partial doc-merge via the _update API instead of a read-modify-write full reindex (which could clobber concurrent per-test-case results).

Verification (real 84-case benchmark, shared OpenSearch cluster)

• 84-case run at concurrency 4 → kill -9 the server at 60/84 → run frozen running, heartbeat stopped
• resume rejected with 409 while heartbeat fresh; accepted once stale → re-executed only the 24 unfinished
• all 60 pre-crash reports verified untouched (report ids + timestamps identical); final stats 84/84, total covers the full run
• second resume attempt → 400 Nothing to resume
• cancel-then-resume flow exercised end-to-end through the UI button

Tests

• Unit: tests/unit/server/routes/storage/evaluationRunResume.test.ts (resumable-id semantics, liveness/staleness) — 9 cases
• Integration: tests/integration/server/routes/evaluationRunResume.integration.test.ts (seed interrupted run → resume via API → preserved vs re-executed reports, full-size stats, 400/404 paths) — runs against a live backend with the built-in demo agent, no AWS needed
• e2e: tests/e2e/resume-run.spec.ts (button contract: visible with correct remaining count on interrupted runs, hidden when nothing to resume, POSTs to /resume)

CHANGELOG updated under ## [Unreleased].

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit ca3ff17)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Race Condition The resume claim token check (lines 531-538) is vulnerable to a time-of-check-to-time-of-use race. After verifying resumeToken matches, the code proceeds with execution using the stale run object fetched before the claim (line 446). If another process overwrites the run between the claim verification and execution start, the stale run.results will be used, potentially re-executing already-completed test cases or missing newly-added results. This can cause duplicate work or lost progress when two servers race to resume after the claim window. const claimed = await storage.evaluationRuns.getById(id); if ((claimed as any)?.resumeToken !== resumeToken) { sendSSE(res, 'error', { error: 'Another server claimed this run for resume at the same time — aborting this attempt', runId: id, }); res.end(); return; } run.status = 'running'; run.results = results; run.resumedAt = now; run.heartbeatAt = now; delete run.error; Possible Deadlock The write serialization queue (lines 838-844) chains promises with .then(op, op) where both success and error paths run the same operation. If op() throws synchronously (before returning a promise), the error handler op will be invoked, which will throw again, and this repeats indefinitely on the same call stack until stack overflow. This occurs when the file system operation fails synchronously (e.g., permission denied on writeJsonFile). private serialized<T>(id: string, op: () => Promise<T>): Promise<T> { const prev = this.writeQueues.get(id) ?? Promise.resolve(); const next = prev.then(op, op); this.writeQueues.set(id, next.catch(() => {})); return next; } Premature State Update The resume handler (lines 140-149) sets resuming to false after only 1 second, regardless of whether the resume operation actually completed. If the SSE stream takes longer than 1 second to emit the 'started' event (network latency, server processing delay), the button re-enables while the resume is still in flight, allowing a user to click Resume again and trigger a second concurrent resume attempt. The 409 guard on the server only blocks same-process double-resumes; a second client-side click after the 1-second timeout can race the first resume's completion. const handleResume = async () => { if (!runId) return; setResuming(true); setError(null); resumeEvaluationRun(runId, () => {}) .catch((err: any) => setError(err.message)) .finally(() => loadRun()); // Give the server a moment to flip status to running, then refresh setTimeout(() => { loadRun(); setResuming(false); }, 1000); }; Unhandled Promise The heartbeat interval callback (line 179) calls storage.evaluationRuns.update() without awaiting or chaining its promise. If the update fails (e.g., network partition, storage unavailable), the error is caught and logged, but the promise rejection is not consumed by the caller. In Node.js with --unhandled-rejections=strict, this can terminate the process. Even without strict mode, the unhandled rejection pollutes logs and may mask the true failure mode when debugging why a run's heartbeat stopped updating. const timer = setInterval(() => { storage.evaluationRuns.update(runId, { heartbeatAt: new Date().toISOString() }) .catch((err: any) => console.warn(`[StorageAPI] Run heartbeat failed for ${runId}: ${err?.message || err}`)); }, RUN_HEARTBEAT_INTERVAL_MS);

[codecov]

Codecov Report

❌ Patch coverage is 32.58427% with 120 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.03%. Comparing base (8e83dc0) to head (ba72497).

Files with missing lines	Patch %	Lines
server/routes/storage/evaluationRuns.ts	22.65%	98 Missing and 1 partial ⚠️
components/evals3/EvalRunDetailPage.tsx	53.33%	5 Missing and 2 partials ⚠️
server/adapters/opensearch/StorageModule.ts	0.00%	7 Missing ⚠️
services/client/evaluationRunsApi.ts	30.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #414      +/-   ##
==========================================
- Coverage   60.14%   60.03%   -0.12%     
==========================================
  Files         373      373              
  Lines       30441    30602     +161     
  Branches     9100     9136      +36     
==========================================
+ Hits        18310    18373      +63     
- Misses      10375    10476     +101     
+ Partials     1756     1753       -3     

Flag	Coverage Δ
e2e	40.06% <53.33%> (+0.16%)	⬆️
integration	42.20% <60.00%> (+0.02%)	⬆️
unit	67.79% <19.73%> (-0.34%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
server/adapters/file/StorageModule.ts	81.17% <100.00%> (+0.30%)	⬆️
types/index.ts	100.00% <ø> (ø)
components/evals3/EvalRunDetailPage.tsx	51.69% <53.33%> (+9.94%)	⬆️
server/adapters/opensearch/StorageModule.ts	58.37% <0.00%> (-0.22%)	⬇️
services/client/evaluationRunsApi.ts	79.66% <30.00%> (-4.60%)	⬇️
server/routes/storage/evaluationRuns.ts	15.84% <22.65%> (+5.10%)	⬆️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to ca3ff17

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Clean up resources on claim failure After detecting a failed claim, the code doesn't clean up the activeCancellationTokens entry or stop the heartbeat that was started earlier. This could leave stale entries in memory. Ensure proper cleanup in the failure path before returning. server/routes/storage/evaluationRuns.ts [531-539] const claimed = await storage.evaluationRuns.getById(id); if ((claimed as any)?.resumeToken !== resumeToken) { + stopHeartbeat(); + activeCancellationTokens.delete(id); sendSSE(res, 'error', { error: 'Another server claimed this run for resume at the same time — aborting this attempt', runId: id, }); res.end(); return; } Suggestion importance[1-10]: 8 __ Why: This is a genuine resource leak bug. When the claim verification fails, stopHeartbeat() and activeCancellationTokens.delete(id) are never called, leaving the heartbeat timer running and a stale entry in the map. The fix correctly adds cleanup before the early return.	Medium
	Strengthen resume token uniqueness The resume token uses process.pid which can be reused across different processes or machines in a distributed system. In containerized environments or after process restarts, PIDs can repeat, potentially causing token collisions. Consider using a more globally unique identifier like a UUID or combining the PID with a timestamp and hostname. server/routes/storage/evaluationRuns.ts [513] -const resumeToken = `${process.pid}-${Math.random().toString(36).slice(2, 10)}`; +const resumeToken = `${Date.now()}-${process.pid}-${Math.random().toString(36).slice(2, 10)}`; Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that process.pid can be reused in containerized environments. Adding a timestamp improves uniqueness, though the existing random component already provides reasonable collision resistance. This is a moderate improvement for distributed deployments.	Low
	Make conflict retry count configurable The retry_on_conflict: 10 setting may be insufficient under high concurrency scenarios with many simultaneous updates. Consider making this configurable via an environment variable or increasing the default to handle burst traffic patterns more reliably. server/adapters/opensearch/StorageModule.ts [1204-1210] +const retryCount = parseInt(process.env.OPENSEARCH_RETRY_ON_CONFLICT || '20', 10); await this.client.update({ index: this.index, id, - retry_on_conflict: 10, + retry_on_conflict: retryCount, body: { doc: updates }, refresh: 'wait_for', }); Suggestion importance[1-10]: 4 __ Why: Making retry_on_conflict configurable is a reasonable enhancement for high-concurrency scenarios. However, the current hardcoded value of 10 is already documented as addressing the concurrency issue found in testing, so this is more of an optimization than a fix.	Low
	Add null check for storage If storage is null or undefined (e.g., during initialization failures), calling storage.evaluationRuns.update will throw and crash the heartbeat. Add a null check to prevent potential runtime errors in edge cases where storage might not be properly initialized. server/routes/storage/evaluationRuns.ts [176-184] function startRunHeartbeat(storage: ReturnType<typeof getStorageModule>, runId: string): () => void { const timer = setInterval(() => { + if (!storage?.evaluationRuns) { + console.warn(`[StorageAPI] Storage not available for heartbeat on ${runId}`); + return; + } storage.evaluationRuns.update(runId, { heartbeatAt: new Date().toISOString() }) .catch((err: any) => console.warn(`[StorageAPI] Run heartbeat failed for ${runId}: ${err?.message || err}`)); }, RUN_HEARTBEAT_INTERVAL_MS); (timer as any).unref?.(); return () => clearInterval(timer); } Suggestion importance[1-10]: 3 __ Why: While defensive null checks are generally good practice, startRunHeartbeat is only called after storage has been successfully obtained via getStorageModule() in the route handlers. The scenario where storage is null at this point is extremely unlikely, making this a low-priority defensive measure.	Low

---

Previous suggestions

Suggestions up to commit ba72497

Category	Suggestion	Impact
Security	Use cryptographically secure random token The resume token generation uses Math.random() which is not cryptographically secure and could produce collisions in high-concurrency scenarios. Use crypto.randomBytes() or crypto.randomUUID() for a collision-resistant claim token that guards against cross-server resume races. server/routes/storage/evaluationRuns.ts [513] -const resumeToken = `${process.pid}-${Math.random().toString(36).slice(2, 10)}`; +const resumeToken = `${process.pid}-${require('crypto').randomBytes(8).toString('hex')}`; Suggestion importance[1-10]: 7 __ Why: Using Math.random() for the resume token could lead to collisions in high-concurrency scenarios. A cryptographically secure random source like crypto.randomBytes() would provide better collision resistance for the cross-server claim mechanism.	Medium
General	Log write queue errors The write queue cleanup strategy silently swallows errors in the queue chain via .catch(() => {}). If an operation fails, subsequent operations in the queue will still execute, but the error context is lost. Consider logging the error or implementing a circuit breaker pattern to prevent cascading failures. server/adapters/file/StorageModule.ts [839-844] private serialized<T>(id: string, op: () => Promise<T>): Promise<T> { const prev = this.writeQueues.get(id) ?? Promise.resolve(); const next = prev.then(op, op); - this.writeQueues.set(id, next.catch(() => {})); + this.writeQueues.set(id, next.catch((err) => { + console.warn(`[FileStorage] Write queue error for ${id}: ${err?.message || err}`); + })); return next; } Suggestion importance[1-10]: 6 __ Why: The .catch(() => {}) silently swallows errors in the write queue chain, making debugging difficult. Logging these errors would improve observability without changing the queue's error-handling semantics.	Low
	Stop heartbeat after consecutive failures The heartbeat timer continues indefinitely even if the storage update consistently fails (e.g., run deleted, storage unavailable). After multiple consecutive failures, the heartbeat should stop to avoid log spam and unnecessary storage load. Implement a failure counter that stops the heartbeat after a threshold. server/routes/storage/evaluationRuns.ts [176-184] function startRunHeartbeat(storage: ReturnType<typeof getStorageModule>, runId: string): () => void { + let failureCount = 0; + const maxFailures = 5; const timer = setInterval(() => { storage.evaluationRuns.update(runId, { heartbeatAt: new Date().toISOString() }) - .catch((err: any) => console.warn(`[StorageAPI] Run heartbeat failed for ${runId}: ${err?.message || err}`)); + .then(() => { failureCount = 0; }) + .catch((err: any) => { + failureCount++; + console.warn(`[StorageAPI] Run heartbeat failed for ${runId} (${failureCount}/${maxFailures}): ${err?.message || err}`); + if (failureCount >= maxFailures) clearInterval(timer); + }); }, RUN_HEARTBEAT_INTERVAL_MS); (timer as any).unref?.(); return () => clearInterval(timer); } Suggestion importance[1-10]: 6 __ Why: The heartbeat continues indefinitely even when storage updates consistently fail, causing unnecessary log spam and storage load. Implementing a failure counter to stop the heartbeat after a threshold would improve resource efficiency and reduce noise in logs.	Low
	Restore run state on failed claim After detecting a failed claim (another server won the race), the run document is left in status: 'running' with the winner's resumeToken. If the winner crashes before completing, this run becomes orphaned until the next boot recovery cycle. Reset the run to its pre-claim state on failed claim attempts to allow immediate retry. server/routes/storage/evaluationRuns.ts [531-538] const claimed = await storage.evaluationRuns.getById(id); if ((claimed as any)?.resumeToken !== resumeToken) { + await storage.evaluationRuns.update(id, { + status: run.status, + resumedAt: run.resumedAt, + heartbeatAt: run.heartbeatAt, + results: run.results, + }).catch(() => {}); sendSSE(res, 'error', { error: 'Another server claimed this run for resume at the same time — aborting this attempt', runId: id, }); res.end(); return; } Suggestion importance[1-10]: 5 __ Why: When a claim fails due to another server winning the race, the run is left in status: 'running' with the winner's token. While boot recovery will eventually handle this, restoring the pre-claim state would allow immediate retry. However, this adds complexity and the current approach is acceptable given the existing recovery mechanisms.	Low

Suggestions up to commit 24facd2

Category	Suggestion	Impact
Possible issue	Clean up run state on early return When testCases.length === 0 but resumableIds is non-empty, the code sends an error and returns early without cleaning up the claim token or heartbeat. This leaves the run in a running state with an active resumeToken, blocking future resume attempts. services/traces/tracePoller.ts [490-499] const resumableSet = new Set(resumableIds); const testCases = resolved.testCases.filter((tc) => resumableSet.has(tc.id)); if (testCases.length === 0) { + await storage.evaluationRuns.update(id, { + status: 'failed', + error: 'The run sources no longer contain the pending test cases — nothing to resume', + completedAt: new Date().toISOString(), + }); sendSSE(res, 'error', { error: 'The run sources no longer contain the pending test cases — nothing to resume', runId: id, }); res.end(); return; } Suggestion importance[1-10]: 8 __ Why: Valid issue: when testCases.length === 0 after filtering, the code returns early without reverting the run's status or clearing the resumeToken, leaving the run stuck in running state and blocking future resume attempts. The suggested fix properly finalizes the run as failed before returning.	Medium
	Ensure resume token uniqueness across servers The resume token uses process.pid which can collide across different servers in a cluster (PIDs are not globally unique). Consider including a server-unique identifier (e.g., hostname or a startup UUID) to ensure true uniqueness across the cluster. server/routes/storage/evaluationRuns.ts [513] -const resumeToken = `${process.pid}-${Math.random().toString(36).slice(2, 10)}`; +const serverUuid = process.env.SERVER_UUID || require('os').hostname(); +const resumeToken = `${serverUuid}-${process.pid}-${Math.random().toString(36).slice(2, 10)}`; Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that process.pid alone may not be globally unique across a multi-server cluster. Including a server-unique identifier (hostname or startup UUID) would strengthen the claim token's uniqueness guarantee, reducing the risk of false claim conflicts in edge cases.	Medium
General	Clear pending timer on early stop When stopping early due to a terminal report state, the code clears the completion promise but doesn't clear the timer (state.timerId). If a timer was already scheduled, it will fire later and attempt to poll again on a deleted state, potentially causing errors. services/traces/tracePoller.ts [305-318] if (currentReport && (currentReport.metricsStatus === 'ready' || currentReport.metricsStatus === 'error')) { debug('TracePoller', `Report ${reportId} is already '${currentReport.metricsStatus}' — stopping poll (no clobber)`); state.running = false; + if (state.timerId) clearTimeout(state.timerId); this.callbacks.delete(reportId); this.polls.delete(reportId); this.completionPromises.get(reportId)?.resolve(); this.completionPromises.delete(reportId); try { callbacks?.onStopped?.(); } catch { /* notification only */ } return; } Suggestion importance[1-10]: 7 __ Why: Valid observation: when the poller stops early due to a terminal report state, state.timerId is not cleared. If a timer was already scheduled, it could fire later and attempt to poll on deleted state. The suggested clearTimeout prevents this potential error path.	Medium
	Revert partial state on claim conflict After detecting a claim conflict, the code returns without reverting the partial state written by the failed claimer (e.g., reset results). This could leave the run in an inconsistent state where some results were cleared but execution never started. server/routes/storage/evaluationRuns.ts [531-539] const claimed = await storage.evaluationRuns.getById(id); if ((claimed as any)?.resumeToken !== resumeToken) { + await storage.evaluationRuns.update(id, { + status: run.status, + results: run.results, + resumeToken: undefined, + }).catch(() => {}); sendSSE(res, 'error', { error: 'Another server claimed this run for resume at the same time — aborting this attempt', runId: id, }); res.end(); return; } Suggestion importance[1-10]: 6 __ Why: The suggestion identifies a potential inconsistency: when a claim conflict is detected, the failed claimer's partial state (reset results) remains written. However, the suggested revert is best-effort (.catch(() => {})) and may not fully solve the issue. The impact is moderate since the winning claimer will overwrite the state anyway.	Low

Suggestions up to commit 68b6299

Category	Suggestion	Impact
Possible issue	Prevent memory leak in write queue The serialized method chains operations but doesn't clean up completed promises from writeQueues. Over time, this map will accumulate entries for every run ID ever processed, causing a memory leak. Add cleanup logic to remove the entry once the promise chain settles, or implement a size-based eviction policy. server/adapters/file/StorageModule.ts [837-842] private serialized<T>(id: string, op: () => Promise<T>): Promise<T> { const prev = this.writeQueues.get(id) ?? Promise.resolve(); const next = prev.then(op, op); this.writeQueues.set(id, next.catch(() => {})); + next.finally(() => { + if (this.writeQueues.get(id) === next) { + this.writeQueues.delete(id); + } + }); return next; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a memory leak where writeQueues accumulates entries indefinitely. The proposed cleanup logic is sound and prevents unbounded growth, though the impact is moderate since run IDs are finite in practice.	Medium
	Validate polling interval is positive The intervalMs fallback chain doesn't validate that the final value is positive and finite. A malicious or misconfigured agent config could set tracePolling.intervalMs to 0, negative, or Infinity, causing the polling loop to either spin infinitely or never execute. Add validation to clamp intervalMs to a safe range. services/traces/tracePoller.ts [105-122] const requestedMax = Number.isFinite(options?.maxAttempts) ? options!.maxAttempts! : Number.isFinite(cfgPolling?.maxAttempts) ? cfgPolling!.maxAttempts! : DEFAULT_MAX_ATTEMPTS; +const rawInterval = options?.intervalMs ?? cfgPolling?.intervalMs ?? DEFAULT_POLL_INTERVAL_MS; +const safeInterval = Number.isFinite(rawInterval) && rawInterval > 0 ? rawInterval : DEFAULT_POLL_INTERVAL_MS; const state: PollState = { ... maxAttempts: Math.min(requestedMax, MAX_POLL_CEILING), - intervalMs: options?.intervalMs ?? cfgPolling?.intervalMs ?? DEFAULT_POLL_INTERVAL_MS, + intervalMs: safeInterval, ... }; Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that intervalMs lacks validation and could be set to invalid values (0, negative, Infinity) via agent config, causing polling to malfunction. The proposed validation ensures a safe fallback and prevents potential infinite loops or stalled polling.	Medium
Security	Strengthen claim token uniqueness guarantee The claim token check uses process.pid which can collide across different machines in a cluster. Two servers with the same PID could generate identical tokens if Math.random() produces the same value. Use a more robust unique identifier like crypto.randomUUID() or include a machine-specific identifier to ensure global uniqueness. server/routes/storage/evaluationRuns.ts [487-513] -const resumeToken = `${process.pid}-${Math.random().toString(36).slice(2, 10)}`; +const resumeToken = `${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2, 10)}`; ... await storage.evaluationRuns.update(id, { ... resumeToken, } as Partial<EvaluationRun>); const claimed = await storage.evaluationRuns.getById(id); if ((claimed as any)?.resumeToken !== resumeToken) { sendSSE(res, 'error', { error: 'Another server claimed this run for resume at the same time — aborting this attempt', runId: id, }); res.end(); return; } Suggestion importance[1-10]: 6 __ Why: Adding Date.now() improves uniqueness by reducing collision probability when process.pid values overlap across machines. However, the existing token already includes 8 random base-36 characters (~41 bits of entropy), making collisions extremely rare. The improvement is marginal but valid.	Low
General	Guard heartbeat cleanup from throwing If executeEvaluationRun throws synchronously before the heartbeat timer's first tick, and the finally block's stopHeartbeat() call itself throws (e.g., clearInterval fails), the activeCancellationTokens.delete(runId) cleanup won't execute. Wrap the stopHeartbeat() call in a try-catch to ensure subsequent cleanup always runs. server/routes/storage/evaluationRuns.ts [312-361] const stopHeartbeat = startRunHeartbeat(storage, runId); try { // Execute the evaluation run const completedRun = await executeEvaluationRun(run, testCases, { ... }); ... } catch (error: any) { ... } finally { - stopHeartbeat(); + try { stopHeartbeat(); } catch { /* heartbeat cleanup failed, non-fatal */ } activeCancellationTokens.delete(runId); res.end(); } Suggestion importance[1-10]: 5 __ Why: The suggestion adds defensive error handling around stopHeartbeat() to ensure subsequent cleanup (activeCancellationTokens.delete) always executes. While clearInterval rarely throws in practice, the guard improves robustness at minimal cost. The impact is low since the failure scenario is unlikely.	Low

Suggestions up to commit ad41511

Category	Suggestion	Impact
General	Clean up resources on claim conflict After detecting a claim conflict, the code should clean up the cancellation token and heartbeat that were already registered. Without cleanup, these resources remain allocated even though the resume attempt was aborted. server/routes/storage/evaluationRuns.ts [438-446] const claimed = await storage.evaluationRuns.getById(id); if ((claimed as any)?.resumeToken !== resumeToken) { + stopHeartbeat(); + activeCancellationTokens.delete(id); sendSSE(res, 'error', { error: 'Another server claimed this run for resume at the same time — aborting this attempt', runId: id, }); res.end(); return; } Suggestion importance[1-10]: 9 __ Why: Critical resource leak fix. When a claim conflict is detected, stopHeartbeat() and activeCancellationTokens.delete(id) were already set up (lines 467-468) but not cleaned up on this early-exit path. Without cleanup, the heartbeat timer continues and the token remains in the registry, potentially blocking future resume attempts.	High
	Optimize check order for performance The active-in-process check should occur before the expensive liveness age calculation. If a run is active in the current process, there's no need to compute its age at all, improving performance during recovery scans. server/services/evaluationRunRecoveryOnBoot.ts [108-111] +if (isEvaluationRunActiveInThisProcess(run.id)) continue; + const ageMs = runLivenessAgeMs(run, now); if (ageMs < staleAfterMs) continue; -if (isEvaluationRunActiveInThisProcess(run.id)) continue; - Suggestion importance[1-10]: 6 __ Why: Valid micro-optimization. Checking isEvaluationRunActiveInThisProcess (a simple Map lookup) before runLivenessAgeMs (date parsing and Math.max across three fields) avoids unnecessary computation for active runs. The improvement is marginal but correct and has no downsides.	Low
Possible issue	Add hostname to resume token The resume token uses process.pid which can collide across different machines in a distributed deployment. Consider including a machine identifier (hostname or a persistent UUID) to ensure uniqueness across the cluster. server/routes/storage/evaluationRuns.ts [420] -const resumeToken = `${process.pid}-${Math.random().toString(36).slice(2, 10)}`; +const resumeToken = `${os.hostname()}-${process.pid}-${Math.random().toString(36).slice(2, 10)}`; Suggestion importance[1-10]: 7 __ Why: Valid concern for distributed deployments where process.pid can collide across machines. Adding os.hostname() improves uniqueness, though the random component already provides reasonable collision resistance. The suggestion is correct and beneficial for multi-server setups.	Medium

Suggestions up to commit c4c77e8

Category	Suggestion	Impact
Possible issue	Re-check report status before judging The clobber guard fetches the report at poll start but doesn't re-check before writing the final verdict. A race condition exists where another poller could judge the report between the initial check and the onTracesFound callback. Re-validate metricsStatus immediately before calling the callback to prevent verdict overwrites. services/traces/tracePoller.ts [304-318] const currentReport = await this.safeGetReport(reportId); if (currentReport && (currentReport.metricsStatus === 'ready' || currentReport.metricsStatus === 'error')) { ... return; } +... +// Before judging, re-check the report hasn't been settled by another path +const preJudgeReport = await this.safeGetReport(reportId); +if (preJudgeReport && (preJudgeReport.metricsStatus === 'ready' || preJudgeReport.metricsStatus === 'error')) { + state.running = false; + callbacks?.onStopped?.(); + return; +} Suggestion importance[1-10]: 7 __ Why: This identifies a legitimate race condition where another poller could judge the report between the initial check and the callback. The calculating claim at line 449 partially mitigates this, but a pre-callback re-check would strengthen the clobber guard.	Medium
General	Continue pagination after transient errors The pagination loop breaks on the first storage error, potentially leaving stale runs unrecovered if a transient error occurs mid-scan. Consider retrying failed pages or continuing to the next page after logging the error, so recovery remains resilient to intermittent storage issues. server/services/evaluationRunRecoveryOnBoot.ts [84-99] const candidates: EvaluationRun[] = []; let from = 0; for (let page = 0; page < maxPages; page++) { let runs: EvaluationRun[]; try { const result = await storage.evaluationRuns.list({ from, size: pageSize, status: 'running' }); runs = result.items; } catch (err: any) { stat.errors++; console.warn(`[evaluationRunRecovery] evaluationRuns.list failed at from=${from}: ${err?.message || err}`); - break; + // Continue to next page instead of breaking + from += pageSize; + continue; } if (!runs || runs.length === 0) break; candidates.push(...runs); if (runs.length < pageSize) break; from += pageSize; } Suggestion importance[1-10]: 6 __ Why: The suggestion improves resilience by continuing pagination after transient storage errors rather than aborting the entire scan. However, skipping failed pages could still leave some stale runs unrecovered, so the improvement is moderate.	Low
	Make conflict retry count configurable The retry_on_conflict: 10 parameter may be insufficient under high concurrency loads (e.g., concurrency > 10). Consider making this configurable via environment variable or increasing the default to handle larger concurrent workloads safely. server/adapters/opensearch/StorageModule.ts [1199-1205] +const retries = parseInt(process.env.OPENSEARCH_UPDATE_RETRIES || '20', 10); await this.client.update({ index: this.index, id, - retry_on_conflict: 10, + retry_on_conflict: retries, body: { doc: updates }, refresh: 'wait_for', }); Suggestion importance[1-10]: 5 __ Why: Making retry_on_conflict configurable is a reasonable enhancement for high-concurrency scenarios. However, the hardcoded value of 10 is already sufficient for most workloads, and the suggestion doesn't address a critical issue.	Low
Security	Use cryptographically secure random token The resume claim token uses Math.random() which is not cryptographically secure and could theoretically collide across concurrent resume attempts. Use crypto.randomBytes() or a UUID library to generate a collision-resistant token for the cross-server claim mechanism. server/routes/storage/evaluationRuns.ts [420] -const resumeToken = `${process.pid}-${Math.random().toString(36).slice(2, 10)}`; +import { randomBytes } from 'crypto'; +const resumeToken = `${process.pid}-${randomBytes(8).toString('hex')}`; Suggestion importance[1-10]: 4 __ Why: While using crypto.randomBytes() would be more secure, the collision risk with Math.random() is extremely low given the combined process.pid prefix and 8-character suffix. The suggestion is valid but offers marginal improvement for this use case.	Low

[github-actions]

Persistent review updated to latest commit e800a93

[github-actions]

Persistent review updated to latest commit c4c77e8

[goyamegh]

Adversarial review (codex / gpt-5.4) — findings & resolutions

Ran a second-opinion review from a different model family on this diff before requesting approval. Findings and what was done (all fixes in c4c77e85):

#	Sev	Finding	Resolution
1	HIGH	Stale heartbeatAt outranked fresh resumedAt right after a resume claim → just-resumed run looked orphaned (double-resume window)	✅ runLivenessAgeMs = max(heartbeat, resumed, created); claim also stamps heartbeatAt. Regression unit test added
2	HIGH	No atomic claim — two servers could resume the same orphan concurrently	✅ Claim token: write resumeToken nonce → re-read → abort via SSE error if another claimer won. (No cross-server CAS primitive in the storage interface; write-then-verify shrinks the race to ~one refresh cycle.) Same-process double resume already 409s — integration test added
3	HIGH	Partial source re-resolution flipped unresolvable test cases to eternally-pending on a "completed" run	✅ Only test cases that will actually execute are reset; missing ones keep their failed-with-note entry and are surfaced as missingCount / missingTestCaseIds on the started event
4	MED	Boot recovery used offset pagination over the shrinking status:running result set → skipped stale runs beyond page 1	✅ Two-phase: scan all pages first, mutate after
5	MED	update() doc-merge is a semantic break for the PUT upsert path (omitted nested results keys would survive)	✅ PUT route now does full-document replace via create(); internal partial updates keep doc-merge
6	MED	File backend had the same lost-update race (in-process, at await boundaries)	✅ Per-run write serialization queue in the file adapter; the double-resume integration test runs against the file backend and exercises it

Verification proof

• Full unit suite: 258 suites / 4652 passed
• Integration (against a live backend built from this branch, file storage, built-in demo agent — no credentials involved):
  • resumes only unfinished test cases, preserves completed reports, then 400s when nothing is left ✅ (5.1s)
  • 404s for an unknown run id ✅
  • 409s a second resume while the first is still executing ✅ (7.6s)
• Manual crash test on a real 84-case benchmark: kill -9 at 60/84 → heartbeat froze → resume accepted only after staleness threshold → exactly 24 re-executed, all 60 pre-crash reports byte-identical, final stats 84/84.

[github-actions]

Persistent review updated to latest commit ad41511

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 24facd2.

Path	Line	Severity	Description
server/routes/storage/evaluationRuns.ts	497	medium	The distributed resume claim token is generated with Math.random() — a cryptographically weak, predictable PRNG. Two servers racing to claim the same orphaned run have a small but non-zero chance of generating the same token, defeating the race-condition guard. Should use crypto.randomBytes() or crypto.randomUUID() for this write-then-re-read CAS mechanism.
server/routes/storage/evaluationRuns.ts	67	low	buildBenchmarkRunProjection explicitly propagates the run's `headers` field (which may contain Authorization tokens or API keys) into the benchmark-scoped embedded projection stored in benchmark.runs. Any code path that reads benchmark.runs for display or export would surface those credential values. The explicit !=(null) guard added for correctness makes this propagation unconditional when headers are present.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 1 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

Persistent review updated to latest commit 68b6299

[goyamegh]

Addendum: resumed runs no longer vanish from benchmark.runs (production bug, hit twice)

Two follow-up commits on this branch (68b62993, 24facd26), independent of #417 / #418.

Bug

POST /api/storage/evaluation-runs links a completed run into the benchmark's embedded runs array (storage.benchmarks.addRun) on its success path — that's what the benchmark detail page and the scoped comparison pool read directly, not the evaluation-run document. If a run's original create-route execution crashed before reaching that success branch, the run was never linked — and until this fix, a later POST .../resume that finished it successfully didn't link it either, since the resume completion path only updated the evaluation-run document. GET .../evaluation-runs/:id and the plain Evaluation Runs list looked completely fine (they read the evaluation-run doc directly); only benchmark-scoped views silently missed the run.

Fix

The resume completion path now builds the same BenchmarkRun projection (buildBenchmarkRunProjection) and links it via a new linkCompletedRunToBenchmark helper: upserts by run id (updateRun if already linked, addRun otherwise) so re-resuming a run already linked never duplicates it.

codex_review findings (applied)

• Blocking — a benchmark-linking failure (e.g. benchmark deleted mid-resume) rewrote the run's own status to 'failed', even though its test cases had genuinely completed and reports were already persisted. → Linking is now in its own try/catch, logged on failure, never allowed to corrupt the canonical run record. New integration test forces this exact path (benchmark id doesn't exist) and asserts the run still reports completed.
• Medium — buildBenchmarkRunProjection used truthy checks for optional fields, silently dropping a real concurrency: 0 or an intentionally empty description/evaluatorId. → Switched to explicit != null checks.
• Documented, not silently claimed solved — linkCompletedRunToBenchmark is read-then-branch-then-write, not an atomic storage upsert; two truly concurrent links of the same run id could both addRun and produce a duplicate a later updateRun can't repair. Added an explicit code comment describing the known, bounded race window (narrowed by the existing same-process/cross-process resume claim guards) plus a unit test that demonstrates it under controlled concurrency. Closing it fully needs an atomic upsert-by-id primitive in the storage adapters (OpenSearch + file) — a bigger, separate change touching both the create and resume paths uniformly, tracked as a follow-up rather than bundled into this bug fix.
• codex also flagged (medium/low, not applied — rationale): (a) the "shared helper" framing overclaims centralization since the create path (owned by unmerged fix: stabilize file-mode benchmark imports #399) doesn't use it yet — code comments/CHANGELOG were reworded to be explicit that only the resume path uses it today; (b) throwing on a missing benchmark mirrors the create route's own pre-existing convention for the same domain concern, not a new decision — changing that shared convention is a deliberate design call better made once both paths are unified, out of scope here; (c) the denormalized-projection architecture itself (embedded benchmark.runs as a hand-maintained secondary view) is a pre-existing pattern in this codebase, not something a single bug-fix PR should redesign.

Tests (red→green at every level, verified locally)

• Unit: tests/unit/server/routes/storage/evaluationRunResume.test.ts — buildBenchmarkRunProjection field coverage (including the != null fix), linkCompletedRunToBenchmark add-vs-upsert-by-id, a repeated-call regression guard against real array semantics, and the documented concurrent-race test.
• Integration: tests/integration/server/routes/evaluationRunResume.integration.test.ts — seeds a benchmark-linked run with an empty benchmark.runs (simulating the pre-addRun crash), resumes to completion, asserts it appears exactly once, resumes again and asserts still no duplicate; a second test forces the benchmark-not-found path and asserts the run still resolves completed.
• E2E: tests/e2e/resume-run-benchmark-link.spec.ts — drives a real resume against the mock demo agent through the actual UI and asserts the run renders on /benchmarks/:id/runs.
• Verified by reverting just the route change and re-running each level (exact expected failure reproduced: e.g. integration test failed with benchmark.runs length 0 instead of 1) before restoring the fix.

Verification

• npm run build:all — green.
• npm run test:unit (this branch's full suite) — 4661 passed, 5 skipped, 0 failed.
• Full test:integration suite (73 suites / 760 tests) — run against an isolated, disposable OpenSearch 2.17.0 Docker container (not the shared cluster, not ports 4000/4001) — all green.
• npm audit --audit-level=high — pre-existing high-severity findings in react-router/undici/vite, unrelated to this diff (no package.json changes), noted per team convention, not a blocker.

Note: this PR (#414) is itself stacked on #413 and not yet mergeable to origin/main standalone; this addendum doesn't change that dependency.

[github-actions]

Persistent review updated to latest commit 24facd2

[github-actions]

Persistent review updated to latest commit ba72497

[github-actions]

Persistent review updated to latest commit ca3ff17