Scheduler rewrite made BaseTask.on_success/on_error dead hooks: subclass terminal bookkeeping silently no-ops #120

New issue

Open

opened 2026-09-08 01:15:33 +00:00 by mara · 0 comments

mara commented

2026-09-08 01:15:33 +00:00

Collaborator

Summary

Commit 0b62cccb ("fix: isolate scheduler run contexts", PR #111) made BaseTask.on_success, BaseTask.on_error (and the PlannedTask pass-through overrides at helpers/task_scheduler.py:596-602) dead code: _execute_task now calls _finish_run(...) directly on all terminal paths and nothing calls on_success/on_error anymore (verified by grep across helpers/, tools/, api/, extensions/, plugins/ at main bba5dcba).

Consequences

on_error's last_result=f"ERROR: {error}" / on_success's last_result=result formatting is now duplicated in _finish_run with different wording ("ERROR: …" vs f"{label}: {result}"), so any external caller or subclass relying on those hooks silently no-ops.
Subclasses that override on_success/on_error for custom bookkeeping (documented extension points on BaseTask) are never invoked on the new terminal path — only on_run/on_finish still fire.
The old state-consistency self-heals ("Fixing task state consistency…" reload+update blocks) were removed without replacement: _finish_run's guarded update_task_by_uuid covers the main path, but on_finish can still clobber state via update_task(self.uuid, updated_at=...) without guards (benign today, fragile tomorrow).

Location

helpers/task_scheduler.py:342-366 (on_error/on_success — now uncalled), :596-602 (PlannedTask overrides), :947-982 (_finish_run — the actual terminal path), :1180-1195 (error path calls _finish_run only).
Reviewed at main bba5dcba; regression introduced in 0b62cccb (2026-09-06).

Repro

Subclass AdHocTask with async def on_success(self, result): log.append(result).
Create + run the task via TaskScheduler.run_task_by_uuid.
Run succeeds; log stays empty because _execute_task → _finish_run never touches the hook.

Suggested fix

Either (a) call current_task.on_success(result) / await current_task.on_error(...) from the corresponding terminal branches of _execute_task (and let _finish_run remain the single state writer, demoting the hook bodies to no-ops to avoid double-writes), or (b) delete on_success/on_error outright and document _finish_run as the only terminal hook — but then migrate the PlannedTask overrides and any out-of-tree subclass guidance in the DOX.

## Summary Commit `0b62cccb` ("fix: isolate scheduler run contexts", PR #111) made `BaseTask.on_success`, `BaseTask.on_error` (and the `PlannedTask` pass-through overrides at `helpers/task_scheduler.py:596-602`) **dead code**: `_execute_task` now calls `_finish_run(...)` directly on all terminal paths and nothing calls `on_success`/`on_error` anymore (verified by grep across `helpers/`, `tools/`, `api/`, `extensions/`, `plugins/` at main `bba5dcba`). ## Consequences 1. `on_error`'s `last_result=f"ERROR: {error}"` / `on_success`'s `last_result=result` formatting is now duplicated in `_finish_run` with *different* wording (`"ERROR: …"` vs `f"{label}: {result}"`), so any external caller or subclass relying on those hooks silently no-ops. 2. Subclasses that override `on_success`/`on_error` for custom bookkeeping (documented extension points on `BaseTask`) are never invoked on the new terminal path — only `on_run`/`on_finish` still fire. 3. The old state-consistency self-heals ("Fixing task state consistency…" reload+update blocks) were removed without replacement: `_finish_run`'s guarded `update_task_by_uuid` covers the main path, but `on_finish` can still clobber state via `update_task(self.uuid, updated_at=...)` without guards (benign today, fragile tomorrow). ## Location - `helpers/task_scheduler.py:342-366` (`on_error`/`on_success` — now uncalled), `:596-602` (`PlannedTask` overrides), `:947-982` (`_finish_run` — the actual terminal path), `:1180-1195` (error path calls `_finish_run` only). - Reviewed at main `bba5dcba`; regression introduced in `0b62cccb` (2026-09-06). ## Repro 1. Subclass `AdHocTask` with `async def on_success(self, result): log.append(result)`. 2. Create + run the task via `TaskScheduler.run_task_by_uuid`. 3. Run succeeds; `log` stays empty because `_execute_task` → `_finish_run` never touches the hook. ## Suggested fix Either (a) call `current_task.on_success(result)` / `await current_task.on_error(...)` from the corresponding terminal branches of `_execute_task` (and let `_finish_run` remain the single state writer, demoting the hook bodies to no-ops to avoid double-writes), or (b) delete `on_success`/`on_error` outright and document `_finish_run` as the only terminal hook — but then migrate the `PlannedTask` overrides and any out-of-tree subclass guidance in the DOX.

mara added the

bug

label

2026-09-08 01:15:33 +00:00