Skip to content

Prevent stale running model evals#21

Merged
dzarlax merged 1 commit into
mainfrom
codex/fix-stale-model-evals
Jun 16, 2026
Merged

Prevent stale running model evals#21
dzarlax merged 1 commit into
mainfrom
codex/fix-stale-model-evals

Conversation

@dzarlax

@dzarlax dzarlax commented Jun 16, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Collaborator

Summary

  • run model evals and final status persistence on backend-owned contexts instead of the htmx request context
  • normalize stale running eval records into timeout errors so the UI does not hang forever
  • add regression coverage for stale running eval status

Tests

  • go test -count=1 ./internal/adminapi
  • go test -count=1 ./...

Summary by CodeRabbit

  • Bug Fixes
    • Model evaluations stuck in a running state beyond a configured timeout threshold are now automatically marked as failed with an error status.

@coderabbitai

coderabbitai Bot commented Jun 16, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

The PR hardens model eval execution by decoupling handleModelEval from request context cancellation (using fixed-budget background contexts) and by adding stale-running-eval detection: evals stuck in "running" beyond modelEvalTimeout + 15s are normalized to "error" status on load, with a matching test.

Changes

Model Eval Hardening

Layer / File(s) Summary
Request context isolation in handleModelEval
internal/adminapi/model_evals.go		 Each status persistence call and runModelEval invocation now uses its own context.WithTimeout(context.Background(), …) instead of deriving from the HTTP request context, constraining each operation to a fixed time budget independent of request cancellation.
Stale eval detection, normalization, and test
internal/adminapi/model_evals.go, internal/adminapi/adminapi_test.go		 Adds modelEvalStaleAfter (modelEvalTimeout + 15s), extends normalizeSingleModelEval to convert stale "running" evals to "error" with updated timestamps and Failed=1, adds isStaleModelEval helper, and validates the behavior with TestModelEvalNormalizesStaleRunningStatus.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 A rabbit once checked on an eval at rest,
Still "running" for hours — clearly distressed!
With a timeout plus fifteen, it measured the lag,
Then flipped it to "error" and planted a flag.
No request can cancel what Background protects,
The warren runs clean with no stale side effects! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'Prevent stale running model evals' directly and clearly summarizes the main change in the changeset: preventing stale running model evaluations from hanging indefinitely by normalizing them to timeout errors.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-stale-model-evals

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@dzarlax dzarlax merged commit 09679ad into main Jun 16, 2026
5 of 6 checks passed
@dzarlax dzarlax deleted the codex/fix-stale-model-evals branch June 16, 2026 17:09
chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed Jun 16, 2026
View reviewed changes

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 64d40b392c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/adminapi/model_evals.go
Comment on lines +267 to +271
now := time.Now().Format(time.RFC3339)
status.Status = "error"
status.FinishedAt = now
status.UpdatedAt = now
status.CheckedAt = now

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the original stale timeout time

When a stale running eval is read, this stamps CheckedAt/UpdatedAt with time.Now(), but loadModelEvals only normalizes in memory and does not write the converted record back. For any stale record left after a restart or aborted handler, every /evals refresh reassigns a fresh timestamp, so buildModelOpsData keeps sorting that old timeout above genuinely recent evals and the UI reports it as having just finished. Use a deterministic timeout time derived from StartedAt or persist the normalized status once.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@dzarlax