fix(tasks): give resume the worker it never had (#1005) - #1020
Conversation
`POST /tasks/{id}/resume` called runtime.resume_run() and returned. That flips
the run to RUNNING and the task to IN_PROGRESS with nothing executing it — and
because the run is then *active*, start_task_run() refuses to start the task
again. The task is wedged until someone edits the database. Every BLOCKED task
resumed from the web UI hits this.
`start` already spawns a background thread for exactly this; resume had no
equivalent. The two endpoints drifted, so the thread body is now one helper
(_spawn_agent_worker) called by both — resume inherits the #722 fail_run /
ErrorEvent recovery for free.
resume's `execute` defaults to True, unlike start's False: web-ui/src/lib/api.ts
posts here with no query params, so any other default would leave the bug in
place for the only real caller, and a resumed run that deliberately does no work
has no use case.
`cf work resume` had the same gap and no escape at all — it never executed, and
afterwards `work start` rejects the active run, so no command could drive the
task. It now runs the agent in the foreground by default, with --no-execute to
opt out (and a message pointing at --execute when you do).
Tests assert on outcome — did a worker actually run the resumed run — not on the
endpoint returning 200, which it always did.
|
Claude finished @frankbria's task in 3m 52s —— View job Bug-hunting review complete
1 defect found (1 inline comment + summary posted):
The web-endpoint change (shared |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
GLM bug-hunting review — 1 defect
Scope: concrete defects only (logic errors, error-path/crash, data loss, races, resource leaks). Style, architecture, naming, and test coverage are out of scope — covered by the general review. No other concrete defects found. The web-endpoint extraction ( |
…view) The bot review caught the CLI path re-introducing the exact wedge this change fixes. `resume_run` has already flipped the run to RUNNING and the task to IN_PROGRESS; `execute_agent` then raises before its own try for a missing key, the outer `except ValueError` printed and exited, and the run stayed RUNNING with no worker — so `work start` refused the task and only `work stop` got you out. The web worker recovers from this via fail_run (#722); the CLI had no equivalent. It was uncovered because the CLI tests' recorder returns a fake state instead of raising, so nothing exercised the raising path. Now it does. Writing that fix also surfaced a NameError: cli/app.py defines no module logger, so the `logger.debug` I first put in the nested recovery handler would have masked the real error with its own exception. Swallowed with a comment instead, and a second test asserts the real error still reaches the user when recovery itself fails.
|
Claude finished @frankbria's task in 4m 53s —— View job GLM bug-hunting review (re-review after
|
| severity | file:line | finding |
|---|---|---|
| minor | codeframe/cli/app.py:2725 |
cf work resume exits 0 when execute_agent returns a FAILED state — work_start exits 1 on the same condition (app.py:2610–2611), and the PR body claims resume should "match cf work start --execute". Scripting cf work resume <id> && next proceeds on a failed execution. execute_agent does return AgentState(FAILED) (runtime.py:753/994), so this branch is reachable. Inline suggestion posted. |
Follow-up on the prior round's major finding. The first push's finding (cf work resume re-wedges the run when execute_agent raises up front — missing/revoked API key) is resolved by 1a3ddbb: the new except Exception → runtime.fail_run(...) recovery resets the run to FAILED (task stays retryable), mirroring the web worker's #722 path. The nested except: pass correctly swallows the only fail_run failure mode — an already-terminal run (runtime.py:403). Verified execute_agent always returns an AgentState (never None), so the post-call state.status read can't AttributeError.
No other concrete defects found. The web-endpoint extraction (_spawn_agent_worker) is a clean move of the existing start_single_task thread body; resume_run → RUNNING → worker runs the same run; up-front failures reset to FAILED via fail_run; double-resume is guarded by core (Run is not blocked). tasks_v2.py defines its module logger (line 40), and the CLI path correctly avoids one (no module logger in cli/app.py), so the commit's noted NameError does not occur.
…follow-up) (#1021) Post-merge review finding on #1020. `cf work resume` printed "Task execution failed" and exited 0, so `cf work resume <id> && next_step` proceeded as if the run had succeeded. `work start` exits 1 for anything short of COMPLETED. Matched to start exactly — BLOCKED and FAILED both exit 1. The reviewer suggested FAILED only, on the grounds that blocking again is a normal outcome of a resume; that is true, but it is equally true of `work start`, and one predictable rule across both commands beats two that differ by a case. Parametrized test pins all three outcomes.
1 participant
Closes #1005.
The bug
POST /tasks/{id}/resumecalledruntime.resume_run()and returned. That flips the run to RUNNING and the task to IN_PROGRESS — and nothing executes it.startspawns a background thread for exactly this; resume never had an equivalent.The task is then wedged: it holds an active run, so
start_task_run()rejects any attempt to start it again, and nothing is driving the run it does have. The only way out was editing the database.Every BLOCKED task resumed from the web UI hits this — stall blockers, agent-escalated blockers, and (as of #911) cost-cap blockers, which give users a routine reason to block and resume.
The CLI was worse
cf work resumehad the same gap and no escape at all. It never executed, and afterwardscf work startrejects the active run — so no command existed that would drive the task.Fix
The two endpoints drifted, so the thread body is now one helper,
_spawn_agent_worker, called by both. Resume inherits the #722fail_run/ErrorEventrecovery for free — a resumed run whose agent fails up front (missing API key, unknown provider) now resets to FAILED instead of stranding.resume'sexecutedefaults to True, unlikestart'sFalse.web-ui/src/lib/api.ts:437posts here with no query params, so any other default would leave the bug in place for the only real caller — and a resumed run that deliberately does no work has no use case.execute=falsekeeps the old flip-only behaviour for anyone who wants it.cf work resumenow runs the agent in the foreground by default, matchingcf work start --execute, with--no-executeto opt out (and a message pointing at--executewhen you do).Core is untouched:
runtime.resume_runstays headless and does not learn about threads or event publishers.Testing
Asserted on outcome — did a worker actually run the resumed run — not on the endpoint returning 200, which it always did.
test_resume_actually_runs_the_agent— a worker runs at alltest_the_worker_gets_the_resumed_run— the same run resume returned, not a fresh onetest_resume_does_not_wedge_the_task— the user-visible symptom: after a failing resume the task is retryable andstartworks againtest_execute_false_keeps_the_flip_only_behaviourtest_the_default_executes_because_the_web_ui_sends_no_paramstest_start_still_works_through_the_shared_helper— this one passes onmain, so it pins that extracting the helper does not regress start--no-executeopts outtests/ui/+tests/cli/: 1168 passed,ruffclean.