Skip to content

Run visibility: in-progress runs on benchmark page, running indicators, stale-run sweep - #410

Draft
ashwin-pc wants to merge 5 commits into
opensearch-project:mainfrom
ashwin-pc:run-visibility
Draft

Run visibility: in-progress runs on benchmark page, running indicators, stale-run sweep#410
ashwin-pc wants to merge 5 commits into
opensearch-project:mainfrom
ashwin-pc:run-visibility

Conversation

@ashwin-pc

@ashwin-pc ashwin-pc commented Aug 25, 2026

Copy link
Copy Markdown
Member

Fixes #405, Fixes #406, Fixes #408 — three run-visibility gaps reported from real file-mode usage (a user started a run from a benchmark page and concluded it was lost).

Changes

Screenshots

View Desktop Mobile
Runs list: Running badge + progress; swept orphan shown finalized
Benchmark detail: in-progress run visible
Stale orphan finalized after restart

Verified against a copy of real file-mode data with seeded running + orphaned runs; production build green; zero console errors on both viewports.

@github-actions

github-actions Bot commented Aug 25, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 19aad6c)

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

Possible Issue

The effectiveLayoutMode logic forces 'tabs' on mobile but the layout toggle is hidden (layoutToggle = !isMobile ? ... : null). If a user sets 'split' on desktop then switches to mobile, the preference is preserved but overridden at render time. Switching back to desktop restores 'split', which is correct. However, if the user explicitly toggles to 'tabs' on desktop, then views on mobile (forced to 'tabs'), then returns to desktop, they see 'tabs' as expected. The issue: the toggle buttons call setLayoutMode(...), which updates the persisted preference. On mobile, the toggle is hidden, so the user cannot change the preference, but the effective mode is always 'tabs'. This is intentional per the comment. No concrete bug is evident—the behavior matches the stated design (preserve desktop preference, force tabs on mobile). Confidence is low that this is a defect rather than a design choice.

 
// A horizontal split leaves each pane unusably narrow on phones. Preserve
// the user's desktop preference, but render the Runs-first tab layout at the
// mobile breakpoint used by the rest of the app.
const effectiveLayoutMode = isMobile ? 'tabs' : layoutMode;
Race Condition

The polling effect depends on visibleInProgressRuns.length to adjust the interval (3s if any in-progress runs, else 5s). If loadInProgressRuns() completes and clears visibleInProgressRuns between the dependency check and the setInterval callback, the interval remains at 3s even though no runs are in progress. The next poll will see zero runs and the effect will re-run, correcting the interval. The window is narrow (a run completing between the dependency read and the interval setup), and the impact is minor (slightly faster polling until the next effect cycle). This is unlikely to cause user-visible issues but could be avoided by computing the interval inside the callback or using a ref to track the current set of in-progress runs.

 
useEffect(() => {
  const shouldPoll = isRunning || hasPendingEvaluations || hasServerInProgressRuns;
  if (pollIntervalRef.current) { clearInterval(pollIntervalRef.current); pollIntervalRef.current = null; }
  if (shouldPoll) {
    const interval = isRunning || visibleInProgressRuns.length > 0 ? POLL_INTERVAL_MS : 5000;
    pollIntervalRef.current = setInterval(() => {
      loadBenchmark();
      loadInProgressRuns();
    }, interval);
  }
  return () => { if (pollIntervalRef.current) { clearInterval(pollIntervalRef.current); pollIntervalRef.current = null; } };
}, [isRunning, hasPendingEvaluations, hasServerInProgressRuns, visibleInProgressRuns.length, loadBenchmark, loadInProgressRuns]);

@github-actions

github-actions Bot commented Aug 25, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Code Suggestions ✨

Latest suggestions up to 19aad6c

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Continue pagination after errors

The pagination loop breaks on the first error, potentially leaving orphaned runs
unprocessed. Consider continuing to the next page after logging the error, or
implementing a retry mechanism to ensure all pages are attempted during recovery.

server/services/evaluationRunRecoveryOnBoot.ts [61-77]

 for (let page = 0; page < maxPages; page++) {
   try {
     const result = await storage.evaluationRuns.list({
       status: 'running',
       from: page * pageSize,
       size: pageSize,
       sort: 'createdAt',
       order: 'asc',
     });
     runningRuns.push(...result.items);
     if (result.items.length < pageSize) break;
   } catch (err: any) {
     stat.errors++;
     console.warn(`[evaluationRunRecovery] evaluationRuns.list failed at page=${page}: ${err?.message || err}`);
-    break;
+    // Continue to next page instead of breaking
+    continue;
   }
 }
Suggestion importance[1-10]: 6

__

Why: Valid observation that break on error stops pagination, potentially leaving orphaned runs unprocessed. Changing to continue would attempt remaining pages. However, the current break behavior may be intentional to avoid cascading failures when storage is degraded. The suggestion improves resilience but needs consideration of whether partial recovery is better than stopping on first failure.

Low
Tighten version filter logic

The version filter logic treats undefined benchmarkVersion as matching any filter,
which could show runs from different versions when a specific version is selected.
Consider filtering out runs with undefined versions when a specific version is
selected to maintain version isolation.

components/evals3/BenchmarkRunsPage.tsx [300-306]

 const visibleInProgressRuns = useMemo(() => {
   const embeddedIds = new Set((benchmark?.runs || []).map(run => run.id));
-  return inProgressRuns.filter(run =>
-    !embeddedIds.has(run.id) &&
-    (runVersionFilter === 'all' || run.benchmarkVersion === undefined || run.benchmarkVersion === runVersionFilter)
-  );
+  return inProgressRuns.filter(run => {
+    if (embeddedIds.has(run.id)) return false;
+    if (runVersionFilter === 'all') return true;
+    return run.benchmarkVersion === runVersionFilter;
+  });
 }, [benchmark?.runs, inProgressRuns, runVersionFilter]);
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies that undefined benchmarkVersion is treated as matching any filter. However, the PR's logic appears intentional—allowing unversioned runs to appear in all filters maintains backward compatibility. The suggested change would hide unversioned runs when filtering by specific versions, which may not be the desired behavior.

Low
Fix progress denominator consistency

When snapshotCount is zero but results exist, the function returns results.length as
total, which may include pending/running results. This creates inconsistency where
completed counts only terminal results but total includes all result entries,
potentially showing progress like "2 of 4" when only 2 test cases exist.

components/evals3/RunningRunIndicator.tsx [20-28]

 export function getRunningRunProgress(run: ProgressRun): { completed: number; total: number } {
   const results = Object.values(run.results || {});
   const completed = results.filter(result => result.status !== 'pending' && result.status !== 'running').length;
   const snapshotCount = run.testCaseSnapshots?.length ?? 0;
-  return {
-    completed,
-    total: snapshotCount > 0 ? snapshotCount : results.length,
-  };
+  const total = snapshotCount > 0 ? snapshotCount : completed;
+  return { completed, total };
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion misunderstands the fallback logic. When snapshotCount is zero (legacy runs without snapshots), using results.length as the denominator is correct—it represents the total number of test cases that were executed. The suggested change (total = completed) would show misleading progress like "2 of 2" when there are actually 4 results (2 completed, 2 pending), breaking the progress indicator for legacy runs.

Low

Previous suggestions

Suggestions up to commit 8b1f882
CategorySuggestion                                                                                                                                    Impact
General
Improve error resilience in pagination

The pagination loop breaks on the first error, potentially leaving orphaned runs
unrecovered if an intermediate page fails. A transient error on page 2 of 5 would
skip pages 3-5 entirely. Consider continuing to the next page after logging the
error, or implementing retry logic for failed pages.

server/services/evaluationRunRecoveryOnBoot.ts [61-77]

 for (let page = 0; page < maxPages; page++) {
   try {
     const result = await storage.evaluationRuns.list({
       status: 'running',
       from: page * pageSize,
       size: pageSize,
       sort: 'createdAt',
       order: 'asc',
     });
     runningRuns.push(...result.items);
     if (result.items.length < pageSize) break;
   } catch (err: any) {
     stat.errors++;
     console.warn(`[evaluationRunRecovery] evaluationRuns.list failed at page=${page}: ${err?.message || err}`);
-    break;
+    // Continue to next page instead of breaking to maximize recovery coverage
+    continue;
   }
 }
Suggestion importance[1-10]: 6

__

Why: Valid suggestion to continue pagination after errors rather than breaking, which would improve recovery coverage. However, the impact is moderate since the recovery is already designed to be idempotent and can be re-run. The continue statement in the improved code is also redundant since it's at the end of the loop.

Low
Fix version filter logic inconsistency

The visibleInProgressRuns filter treats undefined benchmark versions as matching any
filter, which can show unversioned runs when filtering to a specific version. This
creates inconsistent filtering behavior where unversioned runs appear in
version-specific views. Consider filtering out runs with undefined benchmarkVersion
when a specific version is selected.

components/evals3/BenchmarkRunsPage.tsx [300-306]

 const visibleInProgressRuns = useMemo(() => {
   const embeddedIds = new Set((benchmark?.runs || []).map(run => run.id));
   return inProgressRuns.filter(run =>
     !embeddedIds.has(run.id) &&
-    (runVersionFilter === 'all' || run.benchmarkVersion === undefined || run.benchmarkVersion === runVersionFilter)
+    (runVersionFilter === 'all' || run.benchmarkVersion === runVersionFilter)
   );
 }, [benchmark?.runs, inProgressRuns, runVersionFilter]);
Suggestion importance[1-10]: 3

__

Why: The suggestion identifies a potential inconsistency where undefined benchmarkVersion is treated as matching any filter. However, the improved code removes the undefined check entirely, which could break legitimate use cases where runs without a version should be visible in "all" view. The current logic may be intentional for backward compatibility.

Low
Add missing effect dependencies

The polling effect depends on visibleInProgressRuns.length which is derived from
inProgressRuns and runVersionFilter, but these dependencies are not included in the
effect's dependency array. This can cause stale closures where the interval uses
outdated filter values. Add the missing dependencies or restructure to avoid closure
issues.

components/evals3/BenchmarkRunsPage.tsx [347-358]

 useEffect(() => {
   const shouldPoll = isRunning || hasPendingEvaluations || hasServerInProgressRuns;
   if (pollIntervalRef.current) { clearInterval(pollIntervalRef.current); pollIntervalRef.current = null; }
   if (shouldPoll) {
     const interval = isRunning || visibleInProgressRuns.length > 0 ? POLL_INTERVAL_MS : 5000;
     pollIntervalRef.current = setInterval(() => {
       loadBenchmark();
       loadInProgressRuns();
     }, interval);
   }
   return () => { if (pollIntervalRef.current) { clearInterval(pollIntervalRef.current); pollIntervalRef.current = null; } };
-}, [isRunning, hasPendingEvaluations, hasServerInProgressRuns, visibleInProgressRuns.length, loadBenchmark, loadInProgressRuns]);
+}, [isRunning, hasPendingEvaluations, hasServerInProgressRuns, visibleInProgressRuns.length, loadBenchmark, loadInProgressRuns, inProgressRuns, runVersionFilter]);
Suggestion importance[1-10]: 2

__

Why: The suggestion misunderstands React hooks. visibleInProgressRuns.length is already in the dependency array, and adding inProgressRuns and runVersionFilter would be redundant since visibleInProgressRuns is a useMemo that already depends on these values. The current implementation correctly captures the necessary dependencies through the memoized value.

Low
Suggestions up to commit db2de19
CategorySuggestion                                                                                                                                    Impact
General
Continue pagination after page errors

The pagination loop breaks on the first error, potentially leaving orphaned runs
unrecovered if an intermediate page fails. Consider continuing to the next page
after logging the error, or implementing retry logic to ensure all pages are
processed despite transient failures.

server/services/evaluationRunRecoveryOnBoot.ts [61-77]

 for (let page = 0; page < maxPages; page++) {
   try {
     const result = await storage.evaluationRuns.list({
       status: 'running',
       from: page * pageSize,
       size: pageSize,
       sort: 'createdAt',
       order: 'asc',
     });
     runningRuns.push(...result.items);
     if (result.items.length < pageSize) break;
   } catch (err: any) {
     stat.errors++;
     console.warn(`[evaluationRunRecovery] evaluationRuns.list failed at page=${page}: ${err?.message || err}`);
-    break;
+    // Continue to next page instead of breaking to maximize recovery coverage
+    continue;
   }
 }
Suggestion importance[1-10]: 6

__

Why: Valid concern about breaking on first error potentially leaving orphaned runs unrecovered. However, the break behavior may be intentional to avoid cascading failures when storage is fundamentally unavailable. The suggestion to continue would improve recovery coverage in transient failure scenarios, making it a reasonable improvement with moderate impact on reliability.

Low
Handle undefined benchmark versions explicitly

The version filter logic treats undefined benchmark versions as matching any filter,
which can show unversioned runs in version-specific views. Consider explicitly
handling the undefined case by either excluding such runs when a specific version is
selected or documenting this behavior as intentional.

components/evals3/BenchmarkRunsPage.tsx [300-306]

 const visibleInProgressRuns = useMemo(() => {
   const embeddedIds = new Set((benchmark?.runs || []).map(run => run.id));
-  return inProgressRuns.filter(run =>
-    !embeddedIds.has(run.id) &&
-    (runVersionFilter === 'all' || run.benchmarkVersion === undefined || run.benchmarkVersion === runVersionFilter)
-  );
+  return inProgressRuns.filter(run => {
+    if (embeddedIds.has(run.id)) return false;
+    if (runVersionFilter === 'all') return true;
+    // Exclude unversioned runs when filtering by specific version
+    return run.benchmarkVersion !== undefined && run.benchmarkVersion === runVersionFilter;
+  });
 }, [benchmark?.runs, inProgressRuns, runVersionFilter]);
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies that undefined versions match any filter, but this appears intentional based on the comment "unified evaluation-run documents" and the fact that runs may legitimately lack a benchmarkVersion. The proposed change would hide unversioned runs when filtering by version, which may not be the desired behavior. The suggestion is valid but the impact is moderate since the current behavior seems deliberate.

Low
Merge refreshed data instead of replacing

The polling effect replaces the entire evalRuns state every 3 seconds, which can
cause UI flicker and lose user scroll position. Consider merging the refreshed data
with existing runs instead of replacing the entire array, or only update runs that
have changed status.

components/evals3/EvalRunsPage.tsx [206-219]

 const hasRunningEvaluationRuns = evalRuns.some(run => run.status === 'running');
 useEffect(() => {
   if (!hasRunningEvaluationRuns) return;
   let cancelled = false;
   const refresh = async () => {
     try {
       const response = await listEvaluationRuns({ size: 500 });
-      if (!cancelled) setEvalRuns(response.evaluationRuns);
+      if (!cancelled) {
+        setEvalRuns(prev => {
+          const updated = new Map(response.evaluationRuns.map(r => [r.id, r]));
+          return prev.map(run => updated.get(run.id) || run);
+        });
+      }
     } catch (err) {
       console.error('Failed to refresh running evaluation-runs:', err);
     }
   };
   const interval = window.setInterval(refresh, 3000);
   return () => { cancelled = true; window.clearInterval(interval); };
 }, [hasRunningEvaluationRuns]);
Suggestion importance[1-10]: 3

__

Why: The suggestion addresses UI flicker concerns, but the proposed solution has issues: it only updates existing runs and doesn't add new ones from the response, which defeats the purpose of polling for new running runs. The current full replacement is simpler and ensures the list stays synchronized with the server state. The impact is low since the polling only occurs when there are running runs.

Low
Suggestions up to commit 7585d5d
CategorySuggestion                                                                                                                                    Impact
General
Improve recovery resilience on pagination errors

The pagination loop breaks on the first error, potentially leaving orphaned runs
unrecovered if an intermediate page fails. Consider logging the error but continuing
to the next page, or implementing retry logic to ensure all pages are attempted
during recovery.

server/services/evaluationRunRecoveryOnBoot.ts [61-77]

 for (let page = 0; page < maxPages; page++) {
   try {
     const result = await storage.evaluationRuns.list({
       status: 'running',
       from: page * pageSize,
       size: pageSize,
       sort: 'createdAt',
       order: 'asc',
     });
     runningRuns.push(...result.items);
     if (result.items.length < pageSize) break;
   } catch (err: any) {
     stat.errors++;
     console.warn(`[evaluationRunRecovery] evaluationRuns.list failed at page=${page}: ${err?.message || err}`);
-    break;
+    // Continue to next page instead of breaking to maximize recovery coverage
+    continue;
   }
 }
Suggestion importance[1-10]: 6

__

Why: Valid improvement to recovery robustness. Changing break to continue allows the recovery process to attempt remaining pages even if one fails, maximizing orphan run recovery. The error is already logged and counted, so continuing is safer than aborting the entire recovery.

Low
Fix version filter logic consistency

The version filter logic treats undefined benchmark versions as matching all
filters, which can cause runs without version metadata to appear in version-specific
views. This may confuse users expecting strict version filtering. Consider only
showing unversioned runs when runVersionFilter === 'all'.

components/evals3/BenchmarkRunsPage.tsx [300-306]

 const visibleInProgressRuns = useMemo(() => {
   const embeddedIds = new Set((benchmark?.runs || []).map(run => run.id));
   return inProgressRuns.filter(run =>
     !embeddedIds.has(run.id) &&
-    (runVersionFilter === 'all' || run.benchmarkVersion === undefined || run.benchmarkVersion === runVersionFilter)
+    (runVersionFilter === 'all' || run.benchmarkVersion === runVersionFilter)
   );
 }, [benchmark?.runs, inProgressRuns, runVersionFilter]);
Suggestion importance[1-10]: 4

__

Why: The suggestion addresses a potential UX inconsistency where undefined benchmark versions match all filters. However, the current behavior may be intentional to handle legacy runs without version metadata gracefully. The impact is moderate as it affects filtering behavior but doesn't cause functional breakage.

Low
Optimize polling interval calculation

The polling effect depends on visibleInProgressRuns.length which is derived from
inProgressRuns and runVersionFilter. If runVersionFilter changes to hide all
in-progress runs, polling continues at the fast interval unnecessarily. Use
hasServerInProgressRuns (which already accounts for visible runs) instead of
visibleInProgressRuns.length in the interval calculation.

components/evals3/BenchmarkRunsPage.tsx [347-358]

 useEffect(() => {
   const shouldPoll = isRunning || hasPendingEvaluations || hasServerInProgressRuns;
   if (pollIntervalRef.current) { clearInterval(pollIntervalRef.current); pollIntervalRef.current = null; }
   if (shouldPoll) {
-    const interval = isRunning || visibleInProgressRuns.length > 0 ? POLL_INTERVAL_MS : 5000;
+    const interval = isRunning || hasServerInProgressRuns ? POLL_INTERVAL_MS : 5000;
     pollIntervalRef.current = setInterval(() => {
       loadBenchmark();
       loadInProgressRuns();
     }, interval);
   }
   return () => { if (pollIntervalRef.current) { clearInterval(pollIntervalRef.current); pollIntervalRef.current = null; } };
-}, [isRunning, hasPendingEvaluations, hasServerInProgressRuns, visibleInProgressRuns.length, loadBenchmark, loadInProgressRuns]);
+}, [isRunning, hasPendingEvaluations, hasServerInProgressRuns, loadBenchmark, loadInProgressRuns]);
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly identifies that visibleInProgressRuns.length in the dependency array could cause unnecessary re-renders when the filter changes. However, hasServerInProgressRuns already accounts for visibleInProgressRuns.length in its calculation (line 342-344), so the interval logic would remain correct. The optimization is minor and the current implementation is not incorrect.

Low
Suggestions up to commit 20a0153
CategorySuggestion                                                                                                                                    Impact
General
Continue pagination after page errors

The pagination loop breaks on the first error, potentially leaving many orphaned
runs unprocessed. Consider continuing to the next page after logging the error, or
implementing retry logic to ensure all pages are attempted during recovery.

server/services/evaluationRunRecoveryOnBoot.ts [61-77]

 for (let page = 0; page < maxPages; page++) {
   try {
     const result = await storage.evaluationRuns.list({
       status: 'running',
       from: page * pageSize,
       size: pageSize,
       sort: 'createdAt',
       order: 'asc',
     });
     runningRuns.push(...result.items);
     if (result.items.length < pageSize) break;
   } catch (err: any) {
     stat.errors++;
     console.warn(`[evaluationRunRecovery] evaluationRuns.list failed at page=${page}: ${err?.message || err}`);
-    break;
+    // Continue to next page instead of breaking to maximize recovery coverage
+    continue;
   }
 }
Suggestion importance[1-10]: 6

__

Why: Valid suggestion that improves recovery robustness. Continuing pagination after a single page failure would allow recovery of runs on subsequent pages. However, the improved_code shows continue which is redundant (the loop naturally continues), and persistent storage failures might indicate a systemic issue where continuing is futile. Moderate improvement to error handling.

Low
Handle undefined benchmark versions explicitly

The version filter logic treats undefined benchmark versions as matching any filter,
which can show runs from different versions together. Consider explicitly handling
undefined versions by either excluding them or mapping them to a specific version
value to prevent version mixing.

components/evals3/BenchmarkRunsPage.tsx [300-306]

 const visibleInProgressRuns = useMemo(() => {
   const embeddedIds = new Set((benchmark?.runs || []).map(run => run.id));
-  return inProgressRuns.filter(run =>
-    !embeddedIds.has(run.id) &&
-    (runVersionFilter === 'all' || run.benchmarkVersion === undefined || run.benchmarkVersion === runVersionFilter)
-  );
+  return inProgressRuns.filter(run => {
+    if (embeddedIds.has(run.id)) return false;
+    if (runVersionFilter === 'all') return true;
+    // Exclude runs without version info when filtering by specific version
+    return run.benchmarkVersion !== undefined && run.benchmarkVersion === runVersionFilter;
+  });
 }, [benchmark?.runs, inProgressRuns, runVersionFilter]);
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies that undefined versions are treated as matching any filter. However, the current behavior appears intentional (allowing unversioned runs to show in all filters), and the suggested change would hide unversioned runs when filtering by specific versions, which may not be desired. The impact is moderate as it changes filtering semantics.

Low
Merge refreshed runs instead of replacing

The polling effect replaces the entire evalRuns state on each refresh, which can
cause UI flicker and lose any local state changes. Consider merging the refreshed
running runs with the existing state instead of wholesale replacement to maintain
stability.

components/evals3/EvalRunsPage.tsx [206-219]

 useEffect(() => {
   if (!hasRunningEvaluationRuns) return;
   let cancelled = false;
   const refresh = async () => {
     try {
       const response = await listEvaluationRuns({ size: 500 });
-      if (!cancelled) setEvalRuns(response.evaluationRuns);
+      if (!cancelled) {
+        setEvalRuns(prev => {
+          const runningIds = new Set(response.evaluationRuns.map(r => r.id));
+          const nonRunning = prev.filter(r => r.status !== 'running' || !runningIds.has(r.id));
+          return [...response.evaluationRuns, ...nonRunning];
+        });
+      }
     } catch (err) {
       console.error('Failed to refresh running evaluation-runs:', err);
     }
   };
   const interval = window.setInterval(refresh, 3000);
   return () => { cancelled = true; window.clearInterval(interval); };
 }, [hasRunningEvaluationRuns]);
Suggestion importance[1-10]: 3

__

Why: While the suggestion aims to reduce UI flicker, the proposed merge logic is flawed—it would accumulate stale running runs that have completed. The current wholesale replacement is simpler and correct, as loadData() already loads the full dataset initially. The flicker concern is minor given the 3-second polling interval.

Low
Suggestions up to commit d420e7b
CategorySuggestion                                                                                                                                    Impact
General
Continue pagination after page errors

The pagination loop breaks on the first error, potentially leaving orphaned runs
unrecovered if an intermediate page fails. Consider continuing to the next page
after logging the error, or implementing retry logic to ensure all pages are
attempted.

server/services/evaluationRunRecoveryOnBoot.ts [61-77]

 for (let page = 0; page < maxPages; page++) {
   try {
     const result = await storage.evaluationRuns.list({
       status: 'running',
       from: page * pageSize,
       size: pageSize,
       sort: 'createdAt',
       order: 'asc',
     });
     runningRuns.push(...result.items);
     if (result.items.length < pageSize) break;
   } catch (err: any) {
     stat.errors++;
     console.warn(`[evaluationRunRecovery] evaluationRuns.list failed at page=${page}: ${err?.message || err}`);
-    break;
+    continue;
   }
 }
Suggestion importance[1-10]: 7

__

Why: Valid improvement that changes break to continue to attempt all pages even if one fails. This increases the robustness of the recovery process by not abandoning remaining pages after a single error, which is important for a boot recovery mechanism.

Medium
Handle undefined benchmark versions consistently

The version filter logic treats undefined benchmarkVersion as matching any filter,
which can show unversioned runs in version-specific views. This may confuse users
expecting strict version filtering. Consider treating undefined as version 1 or
filtering it out when a specific version is selected.

components/evals3/BenchmarkRunsPage.tsx [300-306]

 const visibleInProgressRuns = useMemo(() => {
   const embeddedIds = new Set((benchmark?.runs || []).map(run => run.id));
-  return inProgressRuns.filter(run =>
-    !embeddedIds.has(run.id) &&
-    (runVersionFilter === 'all' || run.benchmarkVersion === undefined || run.benchmarkVersion === runVersionFilter)
-  );
+  return inProgressRuns.filter(run => {
+    if (embeddedIds.has(run.id)) return false;
+    if (runVersionFilter === 'all') return true;
+    const runVersion = run.benchmarkVersion ?? 1;
+    return runVersion === runVersionFilter;
+  });
 }, [benchmark?.runs, inProgressRuns, runVersionFilter]);
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that undefined benchmarkVersion is treated as matching any filter. However, the improved code assumes undefined should default to version 1, which may not be correct for all cases. The change improves consistency but requires verification of the intended behavior for unversioned runs.

Low
Ensure total reflects actual case count

When testCaseSnapshots exists but results is empty or has fewer entries, the
completed count (0) can exceed or mismatch the total (snapshotCount), showing "0 of
5 cases" initially. Ensure the total always reflects the maximum of snapshots and
result count to avoid misleading progress displays.

components/evals3/RunningRunIndicator.tsx [20-28]

 export function getRunningRunProgress(run: ProgressRun): { completed: number; total: number } {
   const results = Object.values(run.results || {});
   const completed = results.filter(result => result.status !== 'pending' && result.status !== 'running').length;
   const snapshotCount = run.testCaseSnapshots?.length ?? 0;
-  return {
-    completed,
-    total: snapshotCount > 0 ? snapshotCount : results.length,
-  };
+  const total = Math.max(snapshotCount, results.length);
+  return { completed, total };
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion addresses a potential edge case where results.length could exceed snapshotCount, but the current logic already prioritizes snapshotCount when it's greater than 0. Using Math.max would change behavior when snapshots exist but results are more numerous, which may not be the intended fix. The issue described ("0 of 5 cases") is expected initial state, not a bug.

Low

@codecov

codecov Bot commented Aug 25, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.32787% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.33%. Comparing base (0570de4) to head (19aad6c).

Files with missing lines Patch % Lines
components/evals3/BenchmarkRunsPage.tsx 69.23% 3 Missing and 9 partials ⚠️
components/evals3/EvalRunsPage.tsx 70.37% 3 Missing and 5 partials ⚠️
server/services/evaluationRunRecoveryOnBoot.ts 92.85% 0 Missing and 3 partials ⚠️
components/evals3/RunningRunIndicator.tsx 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #410      +/-   ##
==========================================
+ Coverage   60.18%   60.33%   +0.14%     
==========================================
  Files         373      375       +2     
  Lines       30451    30603     +152     
  Branches     8962     8991      +29     
==========================================
+ Hits        18326    18463     +137     
+ Misses      10369    10336      -33     
- Partials     1756     1804      +48
Flag Coverage Δ
e2e 39.91% <39.70%> (-0.10%) ⬇️
integration 42.17% <ø> (ø)
unit 67.01% <75.40%> (-1.12%) ⬇️

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

Files with missing lines Coverage Δ
server/app.ts 71.73% <100.00%> (+0.62%) ⬆️
server/routes/storage/evaluationRuns.ts 11.73% <100.00%> (+0.99%) ⬆️
components/evals3/RunningRunIndicator.tsx 90.90% <90.90%> (ø)
server/services/evaluationRunRecoveryOnBoot.ts 92.85% <92.85%> (ø)
components/evals3/EvalRunsPage.tsx 60.19% <70.37%> (+5.72%) ⬆️
components/evals3/BenchmarkRunsPage.tsx 52.73% <69.23%> (+5.74%) ⬆️

... and 4 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

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 20a0153

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 7585d5d

@ashwin-pc
fix: surface in-progress runs and finalize orphaned ones
8e5f938 
- Evaluation Runs list: distinct Running badge with live case progress
- Benchmark detail: in-progress runs for the benchmark shown alongside
  completed history, refreshing until completion
- Startup sweep finalizes eval-runs stuck in 'running' with no live
  execution (status failed, error notes server restart), preserving
  persisted per-case results

Fixes opensearch-project#405, Fixes opensearch-project#406, Fixes opensearch-project#408

Signed-off-by: ashwin pc <ashwinpc@amazon.com>
@ashwin-pc
test: complete evaluation run recovery coverage
828dc9b 
Signed-off-by: ashwin pc <ashwinpc@amazon.com>
@ashwin-pc
fix: run boot recovery under the CLI entry point too
db2de19 
Boot recovery (trace-poll resume, orphan benchmark/evaluation-run
finalization) lived only in server/index.ts, which the CLI never
executes — 'agent-health serve' imports app.js and listens itself, so
recovery was dead code on the primary distribution path. Extract a
shared runBootRecoverySafely() and call it post-listen from both
entries (after AH_PORT reflects the bound port, since the trace poller
makes HTTP self-calls).

Signed-off-by: ashwin pc <ashwinpc@amazon.com>
@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit db2de19

@ashwin-pc
test: isolate app setup from boot recovery routes
8b1f882 
Signed-off-by: ashwin pc <ashwinpc@amazon.com>
@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 8b1f882

@ashwin-pc
test: include run visibility in unit coverage
19aad6c 
Signed-off-by: ashwin pc <ashwinpc@amazon.com>
@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 19aad6c

@ashwin-pc ashwin-pc mentioned this pull request Aug 28, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TackAdam TackAdam Awaiting requested review from TackAdam TackAdam will be requested when the pull request is marked ready for review TackAdam is a code owner
@goyamegh goyamegh Awaiting requested review from goyamegh goyamegh will be requested when the pull request is marked ready for review goyamegh is a code owner
@ps48 ps48 Awaiting requested review from ps48 ps48 will be requested when the pull request is marked ready for review ps48 is a code owner
@jasonlhamazon jasonlhamazon Awaiting requested review from jasonlhamazon jasonlhamazon will be requested when the pull request is marked ready for review jasonlhamazon is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

1 participant

@ashwin-pc