fix(runners,tasks): critical runner dispatch and finalization bugs#4016
fix(runners,tasks): critical runner dispatch and finalization bugs#4016cursor[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>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![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 findings.
Reviewed all added/modified code in this PR (job_pool.go, running_job.go, TaskPool.go, TaskRunner.go, runner_reconciler.go and tests). Prior automation review threads: none found on this PR.
Scope reviewed
- Runner-side
finalizeAfterRunterminal-status handling - Task-pool requeue/finalize ordering (
onTaskStopbeforeEnqueue) - HA reconciler races (
failTaskRunnerLost,requeueTaskRunnerOffline,finalizeRemoteTaskLocked) - Non-HA
End != nilguard infinalizeRemoteTaskLocked
Why no findings
These changes are internal task-lifecycle/state-machine fixes. Attacker-controlled input does not reach the modified sinks:
- Reconciler
reasonstrings are hard-coded operational messages, not user input. - Runner progress still enforces runner-token auth and
RunnerIDownership before accepting status/log updates. TryFinalize/ClaimAndDequeueguards remain in place; the PR tightens ordering to prevent duplicate dispatch and stale pool leaks rather than weakening checks.finalizeAfterRunpreserves an emergency-stopped terminal state instead of overwriting it — a correctness fix that improves integrity of stop handling.- Moving the
End != nilshort-circuit outside the HA-only block only skips duplicatefinishRun/autorun when completion is already persisted; it does not bypass authorization.
No injection, authn/authz bypass, secret leakage, SSRF, XSS, path traversal, or unsafe deserialization risks were identified in the diff.
Automated security review
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: 73aa1b2960
ℹ️ 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 path now releases the running/claim state before enqueueing, but it still emits EventTypeRequeued, whose handler in handleQueue calls p.onTaskStop again. If another node, or a pending queue tick, ClaimAndDequeues the just-enqueued task before this node processes the requeue event, that second cleanup deletes the fresh claim/running entry for the new owner, making the task look orphaned and risking duplicate cleanup or dispatch. Since cleanup is already synchronous here, the follow-up event needs to only suppress the immediate retry, not call onTaskStop again.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes three critical correctness bugs in the remote runner / task pool path that remain unfixed on
develop. Recent commits (CSRF middleware, secure session cookies, dynamic playbook picker) were reviewed and do not introduce new critical issues.Bugs and impact
1. Duplicate dispatch on all-runners-busy requeue
When every runner is at capacity,
ErrAllRunnersBusyrequeued tasks by enqueueing them while they still sat in the running set, then sendingEventTypeRequeuedfrom a defer. A concurrent queue tick couldClaimAndDequeuethe task in that window and start a second dispatch on the sameTaskRunner, causing duplicate remote jobs.2. Stopped → failed on runner after
terminated_jobsWhen the server tells a runner to emergency-stop a job via
terminated_jobs,applyTerminatedJobssets status tostopped. Ifjob.Run()then unwinds with an error, the error path overwrotestoppedwithfailed, misreporting user-stopped tasks.3. HA pool state leak in reconciler finalization
When
failTaskRunnerLostwon the finalize lock but the DB already had a terminal status, it returned without completing finalization. Pool/Redis state (running set, claims) and autorun children could leak. Related requeue paths also enqueued before releasing running/active bookkeeping.Root cause
Race between in-memory pool bookkeeping and queue/reconciler paths; missing terminal-status guard on runner client error path; incomplete finalization when reconciler and runner report race.
Fix
onTaskStopbefore enqueueing on all requeue paths (ErrAllRunnersBusy, offline runner, undispatched task).finalizeAfterRunon the runner client that respects an already-finished status.failTaskRunnerLostwhen status is already terminal; harden offline requeue with DB re-check and rollback on persist failure.End != nilcleanup outside HA-only guard infinalizeRemoteTaskLocked.Validation
go test ./services/tasks/... ./services/runners/...— all pass