-
Notifications
You must be signed in to change notification settings - Fork 56
fix(orchestrator): add workflow-queue watchdog to recover orphaned auto-start prompts #1234
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
base: main
Are you sure you want to change the base?
Changes from all commits
4b7df3d
1b46c5a
f31a0d9
83588b4
5f5d40d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,7 @@ func (r *sqliteRepository) initSchema() error { | |
| queued_by TEXT NOT NULL DEFAULT '' | ||
| ); | ||
| CREATE INDEX IF NOT EXISTS idx_queued_messages_session_position ON queued_messages(session_id, position); | ||
| CREATE INDEX IF NOT EXISTS idx_queued_messages_queued_by_at ON queued_messages(queued_by, queued_at); | ||
|
|
||
| CREATE TABLE IF NOT EXISTS pending_moves ( | ||
| id TEXT PRIMARY KEY, | ||
|
|
@@ -208,6 +209,35 @@ func (r *sqliteRepository) ListBySession(ctx context.Context, sessionID string) | |
| return out, rows.Err() | ||
| } | ||
|
|
||
| func (r *sqliteRepository) ListStaleByQueuedBy(ctx context.Context, queuedBy string, olderThan time.Time, limit int) ([]QueuedMessage, error) { | ||
| query := ` | ||
| SELECT id, session_id, task_id, position, content, model, plan_mode, | ||
| attachments_json, metadata_json, queued_at, queued_by | ||
| FROM queued_messages | ||
| WHERE queued_by = ? AND queued_at < ? | ||
| ORDER BY queued_at ASC` | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing index for This query runs a full table scan on WHERE queued_by = ? AND queued_at < ? ORDER BY queued_at ASC [LIMIT 500]The only existing index is Adding a composite index in CREATE INDEX IF NOT EXISTS idx_queued_messages_queued_by_at
ON queued_messages(queued_by, queued_at);This is a single line in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 5f5d40d: added |
||
| args := []any{queuedBy, olderThan} | ||
| if limit > 0 { | ||
| query += ` LIMIT ?` | ||
| args = append(args, limit) | ||
| } | ||
| rows, err := r.ro.QueryxContext(ctx, r.ro.Rebind(query), args...) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("list stale queued: %w", err) | ||
| } | ||
| defer func() { _ = rows.Close() }() | ||
|
|
||
| var out []QueuedMessage | ||
| for rows.Next() { | ||
| msg, err := scanQueuedRow(rows) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| out = append(out, *msg) | ||
| } | ||
| return out, rows.Err() | ||
| } | ||
|
|
||
| func (r *sqliteRepository) CountBySession(ctx context.Context, sessionID string) (int, error) { | ||
| var n int | ||
| err := r.ro.GetContext(ctx, &n, r.ro.Rebind(`SELECT COUNT(*) FROM queued_messages WHERE session_id = ?`), sessionID) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| package messagequeue | ||
|
|
||
| import ( | ||
| "testing" | ||
| "time" | ||
| ) | ||
|
|
||
| // SetQueuedAtForTesting overwrites the queued_at timestamp of every entry on | ||
| // a session in the underlying memory repository. Test-only helper: lets | ||
| // external-package tests seed a stale queue entry without exposing the | ||
| // repository or adding production code paths that mutate queued_at. | ||
| // | ||
| // No-op when the service is not backed by the in-memory repository. Callers | ||
| // must guarantee the repo is a memoryRepository (NewServiceMemory). | ||
| func (s *Service) SetQueuedAtForTesting(_ testing.TB, sessionID string, at time.Time) { | ||
| mem, ok := s.repo.(*memoryRepository) | ||
| if !ok { | ||
| return | ||
| } | ||
| mem.mu.Lock() | ||
| defer mem.mu.Unlock() | ||
| for _, m := range mem.entries[sessionID] { | ||
| m.QueuedAt = at | ||
| } | ||
| } |
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.
N+1 deletes + log noise for large workflow queues
dropOrphanedWorkflowQueuefetches all entries withGetStatus, then loops and callsRemoveEntryonce per workflow entry. Two consequences:RemoveEntrycall emits anINFO"queued entry removed" log line. For a session with many stale entries (unlikely given the cap of 10, but possible aftermaxPerSessionis raised), this creates log noise.Nindividual deletes instead of oneDELETE WHERE session_id = ? AND queued_by = ?. Not a performance concern at the current cap, but the approach doesn't match the atomicity guarantees the rest of the queue code is careful about — concurrentTakeHeadbetween theGetStatuscall and aDeleteByIDcould cause a benignErrEntryNotFoundon a drained entry.Both are handled correctly (
ErrEntryNotFoundis swallowed with a debug log), but aDeleteBySessionAndQueuedBy(ctx, sessionID, queuedBy string) (int, error)addition to the repository interface would eliminate the noise and make the intent explicit.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.
Acknowledged the concern. At the current cap of 10 entries the N+1 isn't a performance problem, and adding a
DELETE WHERE session_id = ? AND queued_by = ?method would be new repo API surface. Leaving as-is for this iteration with a note for a follow-up. The log-per-drop noise is already bounded by the cap, and the benignErrEntryNotFoundrace on concurrentTakeHeadis already handled by theDebug-level log.