-
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 4 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 |
|---|---|---|
|
|
@@ -1539,6 +1539,108 @@ func (s *Service) scheduleAutoResumeForWorkflowQueue(ctx context.Context, sessio | |
| go s.tryEnsureExecution(context.WithoutCancel(ctx), sessionID) | ||
| } | ||
|
|
||
| // maybeRecoverOrphanedWorkflowQueue is the per-session decision point of the | ||
| // workflow-queue watchdog (workflow_queue_watchdog.go). For a session that | ||
| // the sweep has flagged because it holds a stale workflow auto-start prompt, | ||
| // this function decides whether to take a recovery action and what: | ||
| // | ||
| // - terminal session → drop the stale workflow entries (the agent is | ||
| // gone for good; leaving them in the queue would never drain). | ||
| // - in-flight turn (activeTurns / running / starting / cancel / reset) → | ||
| // skip; the normal lifecycle will drain. | ||
| // - live agent process → skip; handleAgentReady will drain on next turn end. | ||
| // - otherwise → fire tryEnsureExecution to drive the same auto-resume | ||
| // cascade scheduleAutoResumeForWorkflowQueue uses inline (#1163). | ||
| // | ||
| // queuedAt is the queued_at of the oldest matched entry, used only for the | ||
| // recovery log line. | ||
| func (s *Service) maybeRecoverOrphanedWorkflowQueue(ctx context.Context, sessionID string, queuedAt time.Time) { | ||
| session, err := s.repo.GetTaskSession(ctx, sessionID) | ||
| if err != nil { | ||
| // Transient DB read failure — leave the queue alone and let the | ||
| // next tick retry. Don't conflate this with "session truly gone". | ||
| s.logger.Debug("workflow queue watchdog: skip session load failed", | ||
| zap.String("session_id", sessionID), | ||
| zap.Error(err)) | ||
| return | ||
| } | ||
| if session == nil { | ||
| s.dropOrphanedWorkflowQueue(ctx, sessionID, "session_missing") | ||
| return | ||
| } | ||
| if isTerminalSessionState(session.State) { | ||
| s.dropOrphanedWorkflowQueue(ctx, sessionID, "terminal_session") | ||
| return | ||
| } | ||
| if _, hasActiveTurn := s.activeTurns.Load(sessionID); hasActiveTurn { | ||
| return | ||
| } | ||
| if s.isSessionResetInProgress(sessionID) || s.isCancelInFlight(sessionID) { | ||
| return | ||
| } | ||
| if s.executor == nil || s.agentManager == nil { | ||
| return | ||
| } | ||
| // Skip when the agent process is genuinely alive — handleAgentReady (or | ||
| // the inline drain on the next turn end) will pick up the queue. The | ||
| // agentManager probe is authoritative; the executor's in-memory store | ||
| // can lag a dying process. | ||
| if s.agentManager.IsAgentRunningForSession(ctx, sessionID) { | ||
| return | ||
| } | ||
| ageSec := int(time.Since(queuedAt) / time.Second) | ||
| s.logger.Warn("workflow queue watchdog: recovering orphaned session", | ||
| zap.String("session_id", sessionID), | ||
| zap.String("task_id", session.TaskID), | ||
| zap.String("session_state", string(session.State)), | ||
| zap.Int("queue_age_s", ageSec)) | ||
| // Bypass scheduleAutoResumeForWorkflowQueue's GetExecutionBySession gate: | ||
| // IsAgentRunningForSession just told us the process is dead, but the | ||
| // in-memory store may still hold a stale record. Drive tryEnsureExecution | ||
| // directly so the resume happens even when the store is out of sync. | ||
| // | ||
| // Escalation risk: tryEnsureExecution → ensureSessionRunning → ResumeSession | ||
| // will mark the session FAILED via updateTaskSessionState if the resume | ||
| // itself errors (disk full, ErrNoAgentProfileID, container won't start, | ||
| // etc.). The watchdog inherits this from the inline auto-resume path — | ||
| // a stuck-but-otherwise-recoverable session can flip to terminal if its | ||
| // underlying executor environment is broken. That's the right escalation | ||
| // (the orphan can never drain anyway) but worth flagging for future | ||
| // maintainers: the watchdog is "best-effort recover OR fail loudly". | ||
| go s.tryEnsureExecution(context.WithoutCancel(ctx), sessionID) | ||
| } | ||
|
|
||
| // dropOrphanedWorkflowQueue removes every workflow-tagged queued entry for | ||
| // a session whose agent is gone for good. User and agent entries are left | ||
| // alone — only the workflow auto-start prompts the watchdog owns are dropped. | ||
| func (s *Service) dropOrphanedWorkflowQueue(ctx context.Context, sessionID, reason string) { | ||
| if s.messageQueue == nil { | ||
| return | ||
| } | ||
| status := s.messageQueue.GetStatus(ctx, sessionID) | ||
|
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. N+1 deletes + log noise for large workflow queues
Both are handled correctly (
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. Acknowledged the concern. At the current cap of 10 entries the N+1 isn't a performance problem, and adding a |
||
| dropped := 0 | ||
| for _, entry := range status.Entries { | ||
| if entry.QueuedBy != messagequeue.QueuedByWorkflow { | ||
| continue | ||
| } | ||
| if err := s.messageQueue.RemoveEntry(ctx, sessionID, entry.ID); err != nil { | ||
| s.logger.Debug("workflow queue watchdog: drop entry failed", | ||
| zap.String("session_id", sessionID), | ||
| zap.String("entry_id", entry.ID), | ||
| zap.Error(err)) | ||
| continue | ||
| } | ||
| dropped++ | ||
| } | ||
| if dropped > 0 { | ||
| s.logger.Warn("workflow queue watchdog: dropped orphaned workflow entries", | ||
| zap.String("session_id", sessionID), | ||
| zap.String("reason", reason), | ||
| zap.Int("dropped", dropped)) | ||
| s.publishQueueStatusEvent(ctx, sessionID) | ||
| } | ||
| } | ||
|
|
||
| // flipStaleRunningToWaiting flips the session to WAITING_FOR_INPUT when its | ||
| // state claims RUNNING/STARTING but the orchestrator's authoritative | ||
| // activeTurns map shows no in-flight turn. This catches the manual-move race | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -208,6 +208,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,24 @@ | ||
| package messagequeue | ||
|
|
||
| import ( | ||
| "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(sessionID string, at time.Time) { | ||
|
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. Test helper compiles into production binary This file has a plain The reason it can't be a //go:build testingIf adding a build tag is too invasive, at minimum rename to signal intent, e.g.
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 |
||
| mem, ok := s.repo.(*memoryRepository) | ||
| if !ok { | ||
| return | ||
| } | ||
| mem.mu.Lock() | ||
| defer mem.mu.Unlock() | ||
| for _, m := range mem.entries[sessionID] { | ||
| m.QueuedAt = at | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.