fix(runners,tasks): prevent duplicate dispatch and preserve terminal task status#4039
fix(runners,tasks): prevent duplicate dispatch and preserve terminal task status#4039cursor[bot] wants to merge 2 commits into
Conversation
When every runner is at capacity, ErrAllRunnersBusy requeued tasks by enqueueing them while they still sat in the running set, then sending EventTypeRequeued from a defer. A periodic queue tick could ClaimAndDequeue the task in that window and start a second dispatch on the same TaskRunner. Release running/active bookkeeping before enqueueing and notify the pool synchronously, matching the reconciler requeue paths. Co-authored-by: Denis Gukov <fiftin@outlook.com>
…finalization Runner client: when applyTerminatedJobs emergency-stops a job and Run() unwinds with an error, the error path overwrote stopped with failed. Add finalizeAfterRun that respects an already-finished status. HA reconciler: when failTaskRunnerLost wins the finalize lock but the DB already has a terminal status, complete finalization instead of bailing so pool/Redis state and autorun children are not leaked. Harden offline requeue with a pre-mutation DB re-check and rollback on persist failure. Co-authored-by: Denis Gukov <fiftin@outlook.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security review
Outcome: No medium, high, or critical vulnerabilities identified in the added or modified code.
Scope reviewed: Task/runner pool concurrency fixes — finalizeAfterRun, requeue ordering (onTaskStop before enqueue), HA reconciler finalization, and the Task.End guard moved outside HA-only mode.
Prior threads: No unresolved automation review threads were present on this PR.
Analysis summary:
- Changes are internal state-machine / bookkeeping fixes; no new HTTP handlers, auth checks, SQL, or deserialization paths.
- Attacker-controlled inputs do not reach new sinks. Runner progress still requires authenticated runner identity and enforces
RunnerIDownership before accepting status updates. - Reordering
onTaskStopahead of enqueue closes a duplicate-dispatch race (task simultaneously in the running set and queue). This is a reliability fix that reduces duplicate execution risk rather than introducing one. finalizeAfterRunpreserves an already-terminal status (e.g. emergency stop viaterminated_jobs); it does not bypass runner ownership or permission checks.- Moving the
Task.End != nilearly-return into all deployments strengthens idempotent finalization and reduces duplicate autorun risk. - HA reconciler additions (DB re-check before requeue, rollback on persist failure, finalization when status is already terminal) are defensive and do not weaken authorization boundaries.
No inline findings to report.
Sent by Cursor Automation: Find vulnerabilities
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be92c89d2a
ℹ️ 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".
| t.pool.onTaskStop(t) | ||
| t.pool.state.Enqueue(t) | ||
| log.Info("Task " + strconv.Itoa(t.Task.ID) + " re-queued (waiting for available runner)") | ||
| t.pool.queueEvents <- PoolEvent{EventTypeRequeued, t} |
There was a problem hiding this comment.
In HA, this cleanup removes the old claim before the task is visible in the shared queue, but the EventTypeRequeued sent just below is still handled by handleQueue with another onTaskStop. If another node claims the requeued task in the window after Enqueue and before this node receives the event, that later cleanup can delete the new owner's claim/running record, leaving an actively running task without HA ownership and subject to duplicate/orphan recovery. Keep the cleanup in only one place, or make the requeue event skip cleanup after pre-cleaning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit be92c89. Configure here.
| // Roll back in-memory changes so the next reconcile tick retries via | ||
| // the runner-liveness path instead of mis-routing as undispatched. | ||
| tsk.Task.RunnerID = prevRunnerID | ||
| tsk.Task.Status = prevStatus |
There was a problem hiding this comment.
Rollback ignores SetStatus persistence
Medium Severity
When offline requeue clears RunnerID and calls SetStatus to waiting, saveStatus may already persist that row. If the follow-up UpdateTask fails, the new rollback restores prior in-memory RunnerID and status while the database still holds the cleared assignment—pool state and reconciler decisions can diverge until something else refreshes.
Reviewed by Cursor Bugbot for commit be92c89. Configure here.
| t.pool.onTaskStop(t) | ||
| t.pool.state.Enqueue(t) | ||
| log.Info("Task " + strconv.Itoa(t.Task.ID) + " re-queued (waiting for available runner)") | ||
| t.pool.queueEvents <- PoolEvent{EventTypeRequeued, t} |
There was a problem hiding this comment.
Requeue races queue sweep
High Severity
Requeue paths call onTaskStop, then Enqueue, then block on sending EventTypeRequeued. While handleQueue is finishing another event’s queue sweep, the task can already be in the queue without running/claim bookkeeping, so ClaimAndDequeue may start a second run() on the same TaskRunner. When the requeue event is handled, onTaskStop can strip pool state from that active dispatch.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit be92c89. Configure here.
1 participant
Summary
Critical bug inspection found three unfixed runner/task correctness bugs still present on
developafter recent commits (mostly dependency updates and CWE-620 password verification).Bugs fixed
1. Duplicate dispatch on ErrAllRunnersBusy
When every runner is at capacity, tasks were requeued by enqueueing while still in the running set. A concurrent queue tick could claim and dispatch the same task twice.
Trigger: All runners busy → task requeued → periodic queue tick claims task before
EventTypeRequeuedreleases running state.2. Stopped → failed after emergency stop
When the server told a runner to emergency-stop via
terminated_jobs, the runner'sjob.Run()error path overwrotestoppedwithfailed.Trigger: Server sends
terminated_jobswhile job is running → runner stops job →Run()returns error → status becomesfailedinstead ofstopped.3. HA pool state leak in failTaskRunnerLost
When the reconciler won the finalize lock but the DB already had a terminal status, it returned without completing finalization, leaking running/active pool state and skipping autorun children.
Trigger: HA cluster → runner reports success on node A → reconciler on node B calls
failTaskRunnerLostwith stale in-memory state → bails on finished status without cleanup.Fix
Cherry-picked validated fixes:
finalizeAfterRunon runner to respect already-finished statusfailTaskRunnerLostwhen DB is already terminalValidation
go test ./services/tasks/... ./services/runners/... ./api/runners/...— all passNot in scope
Note
High Risk
Touches core task lifecycle (queue, running set, HA finalize races, runner status reporting); bugs here cause duplicate runs or stuck/leaked tasks, though changes are targeted with new tests.
Overview
Fixes three correctness bugs in runner dispatch and task finalization.
Duplicate dispatch when all runners are busy: Requeue paths now call
onTaskStopbefore putting the task back on the queue and emitEventTypeRequeuedsynchronously fromTaskRunner.run, so the task is not still in the running set when another queue tick canClaimAndDequeueit.Emergency stop reported as failed: Runner job completion goes through new
finalizeAfterRun, which does not change status if the job is already terminal (e.g. serverterminated_jobssetstoppedwhileRununwinds with an error).HA / reconciler leaks:
failTaskRunnerLostcompletesfinalizeRemoteTaskLockedwhen the DB already has a terminal status instead of returning without cleanup.finalizeRemoteTaskLockedtreatsTask.Endset as a signal to release stale pool state even when HA is off. Offline requeue adds a last-moment DB re-check under HA and rolls back in-memory state if persist fails.Reviewed by Cursor Bugbot for commit be92c89. Configure here.