Rework task runners - #1226
Draft
mxsrc wants to merge 19 commits into
Draft
Conversation
mxsrc
force-pushed
the
rework-task-runners
branch
from
August 7, 2026 15:37
20dfeb2 to
69f2330
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
mxsrc
force-pushed
the
rework-task-runners
branch
5 times, most recently
from
August 8, 2026 15:21
8c69ae5 to
358ae48
Compare
This is to make failures visible and avoid any chance for latently lingering broken connections which may occur in FDB. This relies on the deployment mechanism to a) monitor the failures, and b) restart the task runner.
This is strictly only necessary for task runners that run concurrently. It guards against incidental duplicate execution during rollouts, and prepares tasks to be parallelizable.
The executor does not drive execution of a specific task but simply periodically evaluates backup policies. This matches the pattern that existing services have, so the executor is reclassified.
Without this, a task that fails once and succeeds later finishes DONE while still carrying the previous attempt's failure message, because _succeed only fills in "completed" when the result is empty.
The hand-written loop, lease claim, cancel/max-retry handling and status transitions all move to the driver; what remains is the backup call itself. A failed create_backup previously left the task RUNNING and re-ran it every cycle without consuming a retry, so its max_retry ceiling never applied. It is now a TaskRetry, which suspends the task, consumes a retry and backs off like every other runner's failure path.
The loop, lease, cancel/max-retry handling and every task write move to the driver; the handler keeps only the resume decision and signals through TaskDefer (node offline, sibling task on the node, cluster not fully online), TaskAbort (node gone, compression not needed) and TaskRetry (resume failed). This also fixes the not-all-nodes-online branch, whose `continue` resumed the enclosing node scan rather than skipping the task, so a single offline node suspended the task and then issued the RPC anyway. Runners on the driver no longer carry a retry counter, so they drop out of the retry-ceiling harness's source-discovered parametrization. They are now listed explicitly instead, guarded by a test that they really did hand the counter over.
The loop, lease, cancel/max-retry handling and the _finalize state machine move to the driver. Every branch _finalize was called with failure was a suspend-and-retry, so they become TaskRetry; the success path keeps only the replication-state update and the timestamps. The failure path's fixed 3s sleep is dropped in favour of the driver's per-task backoff, which no longer stalls the other tasks in the cycle. Drops the stale comment claiming a missing source node proceeds with a best-effort ANA flip — the code has always retried instead.
Both task families it serves move to the driver's contract. The sync-op handler comes back from tasks_controller, where it only lived because the runner used to execute its loop at import time; with the loop behind serve(SPEC) it belongs next to the runner it serves. The sync-delete failure paths (RPC exception, delete rejected) suspended the task without ever incrementing retry, so its declared max_retry=10 could never bind and a permanently failing delete re-issued forever. They are TaskRetry now, which makes the ceiling effective. The primary's del-sync lock is released through the driver's new on_finish hook, so it is also freed on the terminal paths the handler never sees — cancellation and the retry ceiling — instead of only on the two the old inline code happened to cover.
Both keep their domain step machines; what goes is the surrounding lifecycle. The backup runner's three families (backup, restore, merge) become void handlers over a shared dispatch, with the node lookup and transfer-state poll factored out since all three did them identically. _terminate_task becomes the driver's on_finish hook. It was previously reached only from the timeout and the retry ceiling, so a cancelled backup task left its backup PENDING forever and a cancelled merge left the old backup stuck in MERGING; a restore that gave up on a missing node left its lvol RESTORING. All of those now finalize. Each branch was already guarded on the resource still being in flight, so a successful task passes through untouched. Cluster expand loses its inner per-task loop, which re-ran a single expansion back-to-back with its own backoff and starved every other task in the cycle. The driver polls it once per cycle and backs off the same way.
The hand-rolled thread pool, in-flight sets and per-worker retry loop map directly onto the driver's concurrency, exclusion_key and backoff, so all of that goes; the node_addr guard that keeps two tasks off the same host survives as the exclusion key. The reboot-aware branch, which used to infer "this attempt should not count" by comparing task.retry before and after and rolling it back, becomes a plain TaskDefer. An add_node that raises is now treated the same as one that reports failure — it consumed no retry before, so a permanently crashing add could never reach its ceiling.
The driver wrote task state with task.write_to_db() on the copy it fetched before running the handler. A handler runs for minutes, and the row changes underneath it: set_node_status(ONLINE) cancels restart tasks via cancel_pending_node_restart_tasks, an operator cancels a task, another host's heartbeat stamps the lease. Writing the whole stale object back reverted all of it — an un-canceled task and a reclaimed lease are the two lost updates behind the 2026-07-29 double restart, and the reason upstream converted the restart runner's own writes to CAS. Every transition now runs as a mutator against the current row, refusing a row another actor has finished (and, for non-terminal transitions, one it has canceled), and reporting whether it won. Retry increments land on the fresh count instead of a stale one. on_finish only runs for the caller that won the terminal transition, so a resource is not released twice. Dispatch gets the same treatment: serialized execution now submits to the pool and waits rather than running inline. The inline path registered nothing in the inflight map, which is exactly the split that let a dispatch-mode flip re-enter a task still running and force-shut an already-recovered node. Serialization becomes a per-task predicate, since node restart chooses its mode from live cluster state rather than a fixed concurrency. Tests assert on the committed row via a store that models both write paths, so each of these reproduces as a failure against the previous implementation.
Both cancellation paths wrote the whole task object from a copy read before they decided to cancel — cancel_pending_node_restart_tasks off a bulk get_job_tasks scan, so stale by construction. The runner driving that task writes the same row meanwhile, and the write put all of it back: the owner lease cleared (handing the task to the next runner host that polls, which runs it again), a finished task reverted to running, retry and handler progress rolled back. This is the same lost update as the 2026-07-29 double restart, arriving from the canceller's side rather than the runner's, so it is fixed the same way: a mutator against the current row. cancel_task now sets only the flag and leaves the task where the runner had got to, which reads it on its next pass. It no longer emits a second task-canceled event for an already-canceled task. cancel_pending_node_restart_tasks re-checks on the fresh row and leaves a task that reached its own outcome in between alone, rather than overwriting that outcome with "canceled: node back online".
The last full-object cancellation write: node shutdown flagged the node's migration tasks canceled from a bulk get_job_tasks scan, wiping the owner lease of any the migration runner was driving — the same double-execution vector just fixed in the other two paths. It moves behind tasks_controller.cancel_node_tasks so all three cancellers share one CAS. This one declines a task that finished between the scan and the write, where an operator's explicit cancel_task stays deliberately unconditional.
Both are needed by the restart runner and generalize beyond it. checkpoint() lets a handler record progress the moment an expensive or destructive step succeeds, rather than when the handler returns: restart must not repeat its cleanup shutdown after a crash between the shutdown and the restart. It doubles as the cancellation probe such a handler needs before its next destructive step. on_cycle covers upkeep a runner owns that is attached to no task — restart's watchdog for nodes stranded in a transitional state with no task owning them. The CAS commit moves to a module-level function so both the driver's transitions and checkpoint() share it.
The runner that motivated the driver's CAS writes and single dispatch path now uses them: _task_finish/_task_update, the thread pool, both inflight maps and the per-task backoff schedule are all the driver's, which is most of the 200 lines this removes. Its parallel-vs-serialized choice becomes the spec's serialize predicate, and the condition now has one definition — the dispatch loop and the handler's peer-exclusion pre-check computed it separately, and had they ever disagreed a task fanned out in parallel would have immediately deferred on the peers it was dispatched alongside. The give-up side effects move to on_finish, keyed off the task's own state rather than off having just written it: parking the node OFFLINE and re-queueing when the ceiling terminates a node task, exhausting a device's retries when its task is canceled. Side effects the handler can still reach stay in the handler. Two behaviours needed re-expressing rather than moving: - "stop, a restart is already in flight" was inferred from the task still being NEW/SUSPENDED, which distinguishes nothing now that the driver sets RUNNING before calling. It records the fact directly instead, which also covers an attempt that issued the restart and then died — that RESTARTING is ours to finish, not to defer to. - The cleanup shutdown's once-flag is written through the driver's checkpoint, keeping it persisted the moment the shutdown succeeds rather than when the handler returns. Shutdown and restart raising stay defers, as before: neither consumed a retry, and restart's give-up has side effects that a changed verdict would start triggering where it previously could not.
The three runners drive the same data-plane operation and differ only in what they migrate and when they may start, so the shared half moves to migration_task_common: the status poll, the start-and-record step, the cluster/expansion gate, the settle wait and the recovery gate. utils.handle_task_result and tasks_controller.defer_task_for_expansion were the last two places outside a runner that wrote task state; both are gone. The vocabulary gained TaskProgress for this family. An in-progress poll cannot suspend the task: get_active_node_mig_task keys the family's mutual exclusion on a sibling being RUNNING, so a suspended migration would let a second one start on the same node. Three things needed re-expressing rather than moving: - "has this task started its migration" was read off task.status being RUNNING, which the driver now sets before every call. It reads the marker the start step writes, which is what the status stood in for. - The failed-migration runner tagged its device by asking whether any task for it was still open — correct only because handle_task_result had already written this one DONE. It moves to on_finish, where that is true by construction rather than by accident. - The master-task roll-up becomes per-cycle upkeep. It reads every sub-task of a master anyway, so running it once a cycle covers status changes that finish no sub-task, and drops the redundant recomputation each sibling used to trigger.
Both are mechanical translations of their suspend points into the handler vocabulary, and neither had a retry counter to hand over — every gate in them deferred. Node removal's "incomplete, retry later" pass becomes TaskProgress. It is progress, not failure: the removal waits on device failure-migration that can take hours, and treating each poll as a retry would have earned it an exponential backoff it never had. Port allow keeps its documented opt-out — no IN_ACTIVATION eligibility gate, because activation needs exactly these ports open. Two of its paths returned bare, leaving the task untouched for the loop to revisit; under the driver a bare return means success, so both are now explicit defers. Its abort-on-half-open-hublvol path keeps the abort side effect in the handler and raises TaskAbort for the outcome. Adds an import-smoke over all thirteen migrated runners that also asserts no two of them claim the same task function name — the lease guards against a second host, not against a second spec.
mxsrc
force-pushed
the
rework-task-runners
branch
from
August 8, 2026 15:38
358ae48 to
ef9388d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rework all 13
tasks_runner_*services onto a single shared driver (task_runner_base.py), replacing each hand-rolledwhile True-loop with aRunnerSpecwhose handler does only domain work and signals outcomes viaTaskDefer/TaskRetry/TaskAbort/TaskProgressinstead of touching task state directly. This standardizes DB-error-to-exit, max_retry semantics, task-lease application, and retry/backoff across runners that had drifted apart via copy-paste and independent incident fixes, and generalizes the concurrency + per-key exclusion modelnode_add/restartalready had to every runner.All task-row writes now go through compare-and-set (db.atomic_update) rather than full-object
write_to_db(), closing the lost-update class of bug behind the 2026-07-29 double-restart incident.tasks_runner_backup_merge.pyis reclassified asbackup_merge_service.py(a plain periodic service, not task-based);lvol_migrationandbatch_migrationare deliberately left on their own loops for a follow-up PR since they're under active, fast-moving upstream development. Migrating each runner surfaced and fixed a few latent bugs (e.g. retryable waits that were silently consuming retry budget), called out per-commit. Verified by introducing new driver-level tests (test_task_runner_base.py,test_runner_specs.py) pinning the exception-vocabulary contract.