-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: implement global FIFO queue for Evals runs #7967
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add status and queuePosition fields to runs table schema - Create Redis-based queue management service - Implement queue processor to handle runs one at a time - Update UI to show queued status and position - Add ability to cancel queued runs - Create API endpoint for queue status - Add database migration for new fields Fixes #7966
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently I wrote code so complex that even I can't review it properly on the first try.
Review FindingsAfter reviewing the implementation, I found several areas that need attention: Critical Issues (Must Fix):
Important Suggestions (Should Consider):
Minor Improvements (Nice to Have):
Despite these issues, the overall implementation successfully addresses all requirements from Issue #7966. The FIFO queue works, UI shows proper status badges, and cancellation is functional. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work here. I compared this approach to Issue #7966 and PR #7971. A few concerns and suggestions:
- Design alignment with #7966
- The issue called for a Redis-only, minimal-change FIFO queue with status derived from Redis + existing fields. This PR adds DB columns and a migration, which seems unnecessary and risks drift between Redis and DB.
- Active-run crash safety
- setActiveRun stores the active run without TTL. If a controller dies, the active-run value could block advancement indefinitely. The issue requested TTL-based crash safety for the active run key.
- Queue advancement responsibility
- Advancement appears handled by a server-side processor started from apps/web-evals/src/actions/runs.ts. This can be brittle under multiple Next.js workers or horizontal scale, even with a TTL lock, and it splits responsibility away from the CLI controller which knows when a run ends.
- The controller (packages/evals/src/cli/runEvals.ts) wasn’t updated to clear active-run and dispatch the next queued run, which the issue’s design suggested for deterministic advancement.
If this direction is preferred, could we consider:
- Removing DB queue_position and relying on Redis LPOS to display position.
- If status persistence is needed for analytics, document Redis as the source of truth and keep DB status strictly additive.
- Add TTL/tokenized locks and active-run TTL; clear and auto-advance deterministically from the controller.
Given #7966’s desired shape, I’d recommend the Redis-only approach in #7971 and incorporating a few refinements there (atomic dequeue->setActive->spawn; TTL refresh tied to heartbeat; stronger lock semantics).
Summary
This PR implements a global FIFO queue for Evals runs to ensure only one run executes at a time, addressing Issue #7966.
Changes
Core Queue Implementation
Database Schema Updates
status
field to runs table (queued, running, completed, failed, cancelled)queuePosition
field for tracking position in queueUI Enhancements
API Updates
createRun
action to add runs to queue instead of immediate execution/api/queue/status
)Technical Details
Testing
The implementation has been reviewed with 92% confidence score. All requirements from Issue #7966 have been addressed:
Future Improvements
Fixes #7966
Important
Implement a global FIFO queue for Evals runs using Redis, with UI and API updates for queue management and status display.
queue.ts
.startQueueProcessor()
inqueue-processor.ts
to handle runs sequentially with 5-second polling.status
andqueuePosition
fields toruns
table inschema.ts
.0003_add_queue_fields_to_runs.sql
to update schema and add indexes.run.tsx
to display queued status with position badges and added cancel functionality.createRun
inruns.ts
to add runs to queue./api/queue/status
endpoint inroute.ts
to provide queue status.This description was created by
for 2137b8a. You can customize this summary. It will automatically update as commits are pushed.