Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
7b12ce8f0a | ||
|
|
c34db66f51 | ||
|
|
4911c94e48 | ||
|
|
538bfe5944 | ||
|
|
1c861e2fe1 | ||
|
|
e45de4fec7 |
@@ -0,0 +1,150 @@
|
||||
INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0FHPBBXXKBRWGR3FJZRK58', 'description', NULL, 'DELETE /tasks/{task_name} returns 500 whenever the task has at least one row in task_executions:
|
||||
|
||||
psycopg2.errors.ForeignKeyViolation: update or delete on table "scheduled_tasks"
|
||||
violates foreign key constraint "task_executions_task_id_fkey" on table "task_executions"
|
||||
DETAIL: Key (id)=(46) is still referenced from table "task_executions".
|
||||
|
||||
src/main.py:355. Since every task that has ever fired has execution history, the endpoint works only for tasks that have never run — which is close to none of them. Found on 2026-08-11 while cleaning up a temporary probe task created for T-74; it had to be removed with hand-written SQL against two tables, which is not something the API should require.
|
||||
|
||||
The caller gets a bare "Internal Server Error" with no indication that history is the obstacle, so it reads as the service being broken rather than the request being refusable.
|
||||
|
||||
Deciding what delete should MEAN is the actual work here, and it should not be guessed:
|
||||
- cascade — drop the execution history with the task. Simple, and silently destroys the audit trail for a task someone deletes by mistake.
|
||||
- soft delete — mark it deleted and keep the history. Keeps the audit trail, adds a state every query then has to filter on.
|
||||
- refuse with 409 and a real message ("task has N executions; pass ?purge=true"). Explicit, and makes the destructive variant a deliberate act.
|
||||
|
||||
The third is the smallest correct change and matches how the rest of this system treats destructive operations. Whichever is chosen, a 500 on a foreseeable, well-defined condition is the part that is simply wrong.', NULL, '2026-08-11 09:53:56', '2026-08-11 09:53:56.969', '2026-08-11 09:53:56.969', NULL, '8b2dfce5342a7cd89bbe6d04c03157f1', 2) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQAR5KXM3PTG0QT3R1PV8', 'description', NULL, 'get_tasks_for_minute (src/tasks/executor.py:70) excludes any task holding a task_executions row with status=''running'':
|
||||
|
||||
AND id NOT IN (SELECT task_id FROM task_executions WHERE status = ''running'')
|
||||
|
||||
Nothing ever reconciles that row. It is written before the executor runs and updated after, so a process that dies in between leaves it ''running'' permanently — and the task is then excluded from every future minute, forever, with no error, no alarm and no log line. The task simply stops existing as far as the scheduler is concerned.
|
||||
|
||||
This is not hypothetical. test_example_task has held a ''running'' row since 2025-12-07 — 5916 hours. It happens to be disabled, so nothing is broken today; the mechanism is what matters, not this instance.
|
||||
|
||||
The exposure is real and daily: Watchtower restarts this container every morning at 4 AM. Any task still running at that moment is permanently unscheduled by it. The config backup runs 03:05 and takes ~21 minutes, which is not far off.
|
||||
|
||||
The failure mode is the one this system keeps producing: it looks like nothing. A backup that stops running forever produces no failure — it produces silence, and silence reads as health.
|
||||
|
||||
FIX: reconcile at startup. Any row left ''running'' when the process starts cannot be running, because the process that owned it is gone. Mark those ''orphaned'' (or ''failed'' with a message naming the restart) during app startup, before the scheduler begins its first minute. Consider also a stale-row guard for rows older than the task''s timeout_seconds, which covers a killed worker without a restart.
|
||||
|
||||
The audit trail is preserved either way — this is a status correction, not a deletion.', NULL, '2026-08-11 09:59:05', '2026-08-11 09:59:05.288', '2026-08-11 09:59:05.288', NULL, '1b14bc4af4acc6df0dc2d21675edf31f', 2) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQBTCRCTXK6EJF0T8CS2C', 'description', NULL, 'execute_task has an `except asyncio.TimeoutError` handler (src/tasks/executor.py:179) that records status=''timeout''. It can never run.
|
||||
|
||||
_run_executor wraps the awaited call in `except Exception` (line 219) and returns the error as a value. Since Python 3.11 asyncio.TimeoutError IS the builtin TimeoutError, which inherits OSError -> Exception, so the broad handler catches it first and converts it into an ordinary (None, error) tuple. execute_task then sees a non-None error and files status=''failed''.
|
||||
|
||||
Verified in the deployed runtime:
|
||||
asyncio.TimeoutError is TimeoutError: True
|
||||
MRO: TimeoutError -> OSError -> Exception -> BaseException
|
||||
|
||||
And confirmed against 8 months of history — 18,785 executions, and the count of status=''timeout'' rows is zero:
|
||||
success 16690 | failed 2093 | running 2 | timeout 0
|
||||
|
||||
So every timeout since 2025-12-07 has been recorded as a generic failure carrying a traceback. Operationally that erases the distinction that matters most when a job misbehaves: "this job is too slow for its window" and "this job is broken" need different responses, and right now they look identical in the history.
|
||||
|
||||
FIX: catch asyncio.TimeoutError explicitly in _run_executor, ahead of the broad handler, and let it propagate (or return a marker execute_task can distinguish). Note the ordering is the whole bug — a narrower except after a broader one is dead code, and there is no linter configured here to say so.
|
||||
|
||||
Related: a thread started by asyncio.to_thread cannot be cancelled, so after T-74 a timeout frees the loop while the work continues to completion. Whatever ''timeout'' comes to mean should say so rather than implying the work stopped.', NULL, '2026-08-11 09:59:05', '2026-08-11 09:59:05.545', '2026-08-11 09:59:05.545', NULL, 'af08842306d6326036b1865b8cbf7c58', 2) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQAR5KXM3PTG0QT3R1PV8', 'description', 'get_tasks_for_minute (src/tasks/executor.py:70) excludes any task holding a task_executions row with status=''running'':
|
||||
|
||||
AND id NOT IN (SELECT task_id FROM task_executions WHERE status = ''running'')
|
||||
|
||||
Nothing ever reconciles that row. It is written before the executor runs and updated after, so a process that dies in between leaves it ''running'' permanently — and the task is then excluded from every future minute, forever, with no error, no alarm and no log line. The task simply stops existing as far as the scheduler is concerned.
|
||||
|
||||
This is not hypothetical. test_example_task has held a ''running'' row since 2025-12-07 — 5916 hours. It happens to be disabled, so nothing is broken today; the mechanism is what matters, not this instance.
|
||||
|
||||
The exposure is real and daily: Watchtower restarts this container every morning at 4 AM. Any task still running at that moment is permanently unscheduled by it. The config backup runs 03:05 and takes ~21 minutes, which is not far off.
|
||||
|
||||
The failure mode is the one this system keeps producing: it looks like nothing. A backup that stops running forever produces no failure — it produces silence, and silence reads as health.
|
||||
|
||||
FIX: reconcile at startup. Any row left ''running'' when the process starts cannot be running, because the process that owned it is gone. Mark those ''orphaned'' (or ''failed'' with a message naming the restart) during app startup, before the scheduler begins its first minute. Consider also a stale-row guard for rows older than the task''s timeout_seconds, which covers a killed worker without a restart.
|
||||
|
||||
The audit trail is preserved either way — this is a status correction, not a deletion.', 'get_tasks_for_minute (src/tasks/executor.py:70) excludes any task holding a task_executions row with status=''running'':
|
||||
|
||||
AND id NOT IN (SELECT task_id FROM task_executions WHERE status = ''running'')
|
||||
|
||||
Nothing ever reconciles that row. It is written before the executor runs and updated after, so a process that dies in between leaves it ''running'' permanently — and the task is then excluded from every future minute, forever, with no error, no alarm and no log line. The task simply stops existing as far as the scheduler is concerned.
|
||||
|
||||
This is not hypothetical. test_example_task has held a ''running'' row since 2025-12-07 — 5916 hours. It happens to be disabled, so nothing is broken today; the mechanism is what matters, not this instance.
|
||||
|
||||
The exposure is real and daily: Watchtower restarts this container every morning at 4 AM. Any task still running at that moment is permanently unscheduled by it. The config backup runs 03:05 and takes ~21 minutes, which is not far off.
|
||||
|
||||
The failure mode is the one this system keeps producing: it looks like nothing. A backup that stops running forever produces no failure — it produces silence, and silence reads as health.
|
||||
|
||||
FIX: reconcile at startup. Any row left ''running'' when the process starts cannot be running, because the process that owned it is gone. Mark those ''orphaned'' (or ''failed'' with a message naming the restart) during app startup, before the scheduler begins its first minute. Consider also a stale-row guard for rows older than the task''s timeout_seconds, which covers a killed worker without a restart.
|
||||
|
||||
The audit trail is preserved either way — this is a status correction, not a deletion.
|
||||
|
||||
FIXED in v1.6.0 (e45de4f). TaskExecutor.reconcile_orphaned_executions() runs in the lifespan before the scheduler starts.
|
||||
|
||||
Proven on the real system rather than in a test. The row that had been ''running'' since 2025-12-07 22:44 was released at startup:
|
||||
|
||||
WARNING - Orphaned execution recovered: test_example_task was left ''running''
|
||||
since 2025-12-07 22:44:00.046695. That task had been excluded from
|
||||
scheduling until now.
|
||||
|
||||
Rows still ''running'' afterwards: 0. Status is ''orphaned'', error names the restart.
|
||||
|
||||
''orphaned'' rather than ''failed'' because the outcome is genuinely unknown — a task that completed and never got to update its row looks exactly like one that died halfway, and ''failed'' would assert something nobody observed.
|
||||
|
||||
Ordering is part of the fix and is mutation-checked: reconciliation must complete before the first minute is processed, or the first tick still sees the stale rows. Removing the startup call fails the test.
|
||||
|
||||
Not extended to a duration-based sweep, deliberately. While the process lives, execute_task''s finally always closes the row out, so a stale row implies a dead owner; a time-based rule would have to distinguish a slow task from a dead one and would eventually close the record of a task that is still working.', NULL, '2026-08-11 10:11:39', '2026-08-11 10:11:39.058', '2026-08-11 10:11:39.058', NULL, '0cec030639664cf075a113be75218e8d', 2) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQAR5KXM3PTG0QT3R1PV8', 'status', 'backlog', 'done', NULL, '2026-08-11 10:11:39', '2026-08-11 10:11:39.178', '2026-08-11 10:11:39.178', NULL, 'cd26a078365d91d44e6d284e80da39f5', 2) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQBTCRCTXK6EJF0T8CS2C', 'description', 'execute_task has an `except asyncio.TimeoutError` handler (src/tasks/executor.py:179) that records status=''timeout''. It can never run.
|
||||
|
||||
_run_executor wraps the awaited call in `except Exception` (line 219) and returns the error as a value. Since Python 3.11 asyncio.TimeoutError IS the builtin TimeoutError, which inherits OSError -> Exception, so the broad handler catches it first and converts it into an ordinary (None, error) tuple. execute_task then sees a non-None error and files status=''failed''.
|
||||
|
||||
Verified in the deployed runtime:
|
||||
asyncio.TimeoutError is TimeoutError: True
|
||||
MRO: TimeoutError -> OSError -> Exception -> BaseException
|
||||
|
||||
And confirmed against 8 months of history — 18,785 executions, and the count of status=''timeout'' rows is zero:
|
||||
success 16690 | failed 2093 | running 2 | timeout 0
|
||||
|
||||
So every timeout since 2025-12-07 has been recorded as a generic failure carrying a traceback. Operationally that erases the distinction that matters most when a job misbehaves: "this job is too slow for its window" and "this job is broken" need different responses, and right now they look identical in the history.
|
||||
|
||||
FIX: catch asyncio.TimeoutError explicitly in _run_executor, ahead of the broad handler, and let it propagate (or return a marker execute_task can distinguish). Note the ordering is the whole bug — a narrower except after a broader one is dead code, and there is no linter configured here to say so.
|
||||
|
||||
Related: a thread started by asyncio.to_thread cannot be cancelled, so after T-74 a timeout frees the loop while the work continues to completion. Whatever ''timeout'' comes to mean should say so rather than implying the work stopped.', 'execute_task has an `except asyncio.TimeoutError` handler (src/tasks/executor.py:179) that records status=''timeout''. It can never run.
|
||||
|
||||
_run_executor wraps the awaited call in `except Exception` (line 219) and returns the error as a value. Since Python 3.11 asyncio.TimeoutError IS the builtin TimeoutError, which inherits OSError -> Exception, so the broad handler catches it first and converts it into an ordinary (None, error) tuple. execute_task then sees a non-None error and files status=''failed''.
|
||||
|
||||
Verified in the deployed runtime:
|
||||
asyncio.TimeoutError is TimeoutError: True
|
||||
MRO: TimeoutError -> OSError -> Exception -> BaseException
|
||||
|
||||
And confirmed against 8 months of history — 18,785 executions, and the count of status=''timeout'' rows is zero:
|
||||
success 16690 | failed 2093 | running 2 | timeout 0
|
||||
|
||||
So every timeout since 2025-12-07 has been recorded as a generic failure carrying a traceback. Operationally that erases the distinction that matters most when a job misbehaves: "this job is too slow for its window" and "this job is broken" need different responses, and right now they look identical in the history.
|
||||
|
||||
FIX: catch asyncio.TimeoutError explicitly in _run_executor, ahead of the broad handler, and let it propagate (or return a marker execute_task can distinguish). Note the ordering is the whole bug — a narrower except after a broader one is dead code, and there is no linter configured here to say so.
|
||||
|
||||
Related: a thread started by asyncio.to_thread cannot be cancelled, so after T-74 a timeout frees the loop while the work continues to completion. Whatever ''timeout'' comes to mean should say so rather than implying the work stopped.
|
||||
|
||||
FIXED in v1.6.0 (e45de4f). _run_executor now catches asyncio.TimeoutError ahead of the broad handler and re-raises, so execute_task''s timeout branch is reachable for the first time.
|
||||
|
||||
A trap surfaced while fixing it, and it is the interesting part. While the branch was unreachable, a timeout travelled the NORMAL path — which does update scheduled_tasks. Making the branch reachable without adding that write would have swapped a wrong status (''failed'') for a stale one (last_status showing the previous run''s outcome indefinitely). _update_task_outcome now mirrors terminal outcomes onto the parent row. A fix that repairs the reported symptom while breaking something adjacent is worse than the bug.
|
||||
|
||||
The timeout message also records that the work may still be running: after T-74 executors are handed to asyncio.to_thread, and a thread cannot be cancelled, so wait_for frees the loop while the work continues to completion. Saying "timed out" without that would imply the work stopped.
|
||||
|
||||
Mutation-checked: removing the narrow except clause fails the propagation test. A third test pins that ordinary exceptions are still returned rather than raised, so the narrow clause cannot start swallowing anything else.', NULL, '2026-08-11 10:11:39', '2026-08-11 10:11:39.291', '2026-08-11 10:11:39.291', NULL, '8bdb93fdcfbc234e18f3d034ddf25277', 2) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQBTCRCTXK6EJF0T8CS2C', 'status', 'backlog', 'done', NULL, '2026-08-11 10:11:39', '2026-08-11 10:11:39.421', '2026-08-11 10:11:39.421', NULL, '7bc24ec03ed40ccacd71d7c7269d05af', 2) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0PNFBDB743QJQ9MF38MPJ8', 'description', NULL, 'DONE in v1.7.0 (538bfe5). Verified on a real row:
|
||||
|
||||
12:24:27 ok task=backup_portainer_daily source=scheduler/portainer_backup_executor
|
||||
|
||||
WHY, and why not the other fix. A row written by a failing probe on 2026-08-11 was byte-for-byte what a nightly backup failure would have written — same source, same domain, nothing naming the task. The reflex was to delete the inconvenient row; that was refused as audit tampering and the refusal was right twice over, because the row is TRUE (a run did fail) and the defect is that it cannot say which run. Attribution keeps the record intact and makes it answer the question.
|
||||
|
||||
MECHANISM. A ContextVar (src/task_context.py) set by the engine around executor invocation, read by health_report. Not an argument: executors are invoked as execute(config, settings), there are ten, and several are dormant — they exist only as a string in a database row and become live the moment someone inserts a task naming them. A signature change would leave those broken in a way no import, grep or test would reveal. Not injected into `config` either: config is what a human wrote in the task definition and an executor may legitimately reject unknown keys.
|
||||
|
||||
TWO PROPERTIES VERIFIED IN THE DEPLOYED RUNTIME rather than assumed:
|
||||
- asyncio.to_thread propagates context, so attribution survives the worker thread T-74 introduced. Had it not, every row from a real executor would have lost its task while unit tests kept passing — hence a test that goes through report_async specifically.
|
||||
- Each asyncio Task gets its own copy, so five concurrent executions cannot read each other''s. The isolation test yields mid-execution to force interleaving; without that it would pass against a shared global.
|
||||
- A plain await does NOT get its own copy and leaks to the caller. Both entry points use create_task, but task_scope resets by token rather than relying on it.
|
||||
|
||||
Omitted rather than nulled when absent — report() is callable from a script, and a null would claim a task existed with no name.
|
||||
|
||||
The one pre-existing critical row still carries no task, which now identifies it: it is the only backup row without the field, so it is legible as pre-attribution rather than ambiguous.', NULL, '2026-08-11 10:25:02', '2026-08-11 10:25:02.952', '2026-08-11 10:25:02.952', NULL, 'ede9b41ea9849e08202b7aae16e9a54c', 2) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0PNFBDB743QJQ9MF38MPJ8', 'status', 'backlog', 'done', NULL, '2026-08-11 10:25:03', '2026-08-11 10:25:03.085', '2026-08-11 10:25:03.085', NULL, 'b45fb340bf78c6c7e4287df001d01456', 2) ON CONFLICT(hash) DO NOTHING;
|
||||
@@ -0,0 +1,4 @@
|
||||
INSERT INTO ticket_idmap (record_id, ticket_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0FHPBBXXKBRWGR3FJZRK58', 'T-1', '2026-08-11 09:53:56.835', '2026-08-11 09:53:56.835', NULL, '3d980945e785a6bc7ca8fcaa8250e22b', 2) ON CONFLICT(record_id) DO UPDATE SET ticket_id=excluded.ticket_id, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= ticket_idmap.updated_at;
|
||||
INSERT INTO ticket_idmap (record_id, ticket_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQAR5KXM3PTG0QT3R1PV8', 'T-2', '2026-08-11 09:59:05.153', '2026-08-11 09:59:05.153', NULL, 'c8655ec9601ba93fe822395329e62262', 2) ON CONFLICT(record_id) DO UPDATE SET ticket_id=excluded.ticket_id, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= ticket_idmap.updated_at;
|
||||
INSERT INTO ticket_idmap (record_id, ticket_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQBTCRCTXK6EJF0T8CS2C', 'T-3', '2026-08-11 09:59:05.427', '2026-08-11 09:59:05.427', NULL, '3bce28c3d803d3f5036cfbb1ac11969c', 2) ON CONFLICT(record_id) DO UPDATE SET ticket_id=excluded.ticket_id, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= ticket_idmap.updated_at;
|
||||
INSERT INTO ticket_idmap (record_id, ticket_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0PNFBDB743QJQ9MF38MPJ8', 'T-4', '2026-08-11 10:25:02.811', '2026-08-11 10:25:02.811', NULL, 'eb8c6e4088b39b797541c1be0ee313fd', 2) ON CONFLICT(record_id) DO UPDATE SET ticket_id=excluded.ticket_id, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= ticket_idmap.updated_at;
|
||||
@@ -0,0 +1,193 @@
|
||||
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0FHPBBXXKBRWGR3FJZRK58', 'bug', NULL, 'DELETE /tasks/{name} 500s for any task that has ever run', NULL, 'backlog', 'high', NULL, NULL, NULL, '2026-08-11 09:53:56.826', '2026-08-11 09:53:56.826', NULL, '86310746d98f53b722c7336b0bab980a', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at;
|
||||
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0FHPBBXXKBRWGR3FJZRK58', 'bug', NULL, 'DELETE /tasks/{name} 500s for any task that has ever run', 'DELETE /tasks/{task_name} returns 500 whenever the task has at least one row in task_executions:
|
||||
|
||||
psycopg2.errors.ForeignKeyViolation: update or delete on table "scheduled_tasks"
|
||||
violates foreign key constraint "task_executions_task_id_fkey" on table "task_executions"
|
||||
DETAIL: Key (id)=(46) is still referenced from table "task_executions".
|
||||
|
||||
src/main.py:355. Since every task that has ever fired has execution history, the endpoint works only for tasks that have never run — which is close to none of them. Found on 2026-08-11 while cleaning up a temporary probe task created for T-74; it had to be removed with hand-written SQL against two tables, which is not something the API should require.
|
||||
|
||||
The caller gets a bare "Internal Server Error" with no indication that history is the obstacle, so it reads as the service being broken rather than the request being refusable.
|
||||
|
||||
Deciding what delete should MEAN is the actual work here, and it should not be guessed:
|
||||
- cascade — drop the execution history with the task. Simple, and silently destroys the audit trail for a task someone deletes by mistake.
|
||||
- soft delete — mark it deleted and keep the history. Keeps the audit trail, adds a state every query then has to filter on.
|
||||
- refuse with 409 and a real message ("task has N executions; pass ?purge=true"). Explicit, and makes the destructive variant a deliberate act.
|
||||
|
||||
The third is the smallest correct change and matches how the rest of this system treats destructive operations. Whichever is chosen, a 500 on a foreseeable, well-defined condition is the part that is simply wrong.', 'backlog', 'high', NULL, NULL, NULL, '2026-08-11 09:53:56.826', '2026-08-11 09:53:56.969', NULL, '248bb53ca2bae0f12fabd9d88a1ca171', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at;
|
||||
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQAR5KXM3PTG0QT3R1PV8', 'bug', NULL, 'A restart mid-task unschedules that task forever, silently', NULL, 'backlog', 'high', NULL, NULL, NULL, '2026-08-11 09:59:05.153', '2026-08-11 09:59:05.153', NULL, '0bda80455ba51ea4cb0cb91e302ac216', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at;
|
||||
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQAR5KXM3PTG0QT3R1PV8', 'bug', NULL, 'A restart mid-task unschedules that task forever, silently', 'get_tasks_for_minute (src/tasks/executor.py:70) excludes any task holding a task_executions row with status=''running'':
|
||||
|
||||
AND id NOT IN (SELECT task_id FROM task_executions WHERE status = ''running'')
|
||||
|
||||
Nothing ever reconciles that row. It is written before the executor runs and updated after, so a process that dies in between leaves it ''running'' permanently — and the task is then excluded from every future minute, forever, with no error, no alarm and no log line. The task simply stops existing as far as the scheduler is concerned.
|
||||
|
||||
This is not hypothetical. test_example_task has held a ''running'' row since 2025-12-07 — 5916 hours. It happens to be disabled, so nothing is broken today; the mechanism is what matters, not this instance.
|
||||
|
||||
The exposure is real and daily: Watchtower restarts this container every morning at 4 AM. Any task still running at that moment is permanently unscheduled by it. The config backup runs 03:05 and takes ~21 minutes, which is not far off.
|
||||
|
||||
The failure mode is the one this system keeps producing: it looks like nothing. A backup that stops running forever produces no failure — it produces silence, and silence reads as health.
|
||||
|
||||
FIX: reconcile at startup. Any row left ''running'' when the process starts cannot be running, because the process that owned it is gone. Mark those ''orphaned'' (or ''failed'' with a message naming the restart) during app startup, before the scheduler begins its first minute. Consider also a stale-row guard for rows older than the task''s timeout_seconds, which covers a killed worker without a restart.
|
||||
|
||||
The audit trail is preserved either way — this is a status correction, not a deletion.', 'backlog', 'high', NULL, NULL, NULL, '2026-08-11 09:59:05.153', '2026-08-11 09:59:05.288', NULL, '23a72bff9ffc3592c796741df8a7232e', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at;
|
||||
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQBTCRCTXK6EJF0T8CS2C', 'bug', NULL, 'The ''timeout'' execution status is unreachable; timeouts are filed as generic failures', NULL, 'backlog', 'medium', NULL, NULL, NULL, '2026-08-11 09:59:05.427', '2026-08-11 09:59:05.427', NULL, '31a32d6dce87263657892d48552d1f0a', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at;
|
||||
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQBTCRCTXK6EJF0T8CS2C', 'bug', NULL, 'The ''timeout'' execution status is unreachable; timeouts are filed as generic failures', 'execute_task has an `except asyncio.TimeoutError` handler (src/tasks/executor.py:179) that records status=''timeout''. It can never run.
|
||||
|
||||
_run_executor wraps the awaited call in `except Exception` (line 219) and returns the error as a value. Since Python 3.11 asyncio.TimeoutError IS the builtin TimeoutError, which inherits OSError -> Exception, so the broad handler catches it first and converts it into an ordinary (None, error) tuple. execute_task then sees a non-None error and files status=''failed''.
|
||||
|
||||
Verified in the deployed runtime:
|
||||
asyncio.TimeoutError is TimeoutError: True
|
||||
MRO: TimeoutError -> OSError -> Exception -> BaseException
|
||||
|
||||
And confirmed against 8 months of history — 18,785 executions, and the count of status=''timeout'' rows is zero:
|
||||
success 16690 | failed 2093 | running 2 | timeout 0
|
||||
|
||||
So every timeout since 2025-12-07 has been recorded as a generic failure carrying a traceback. Operationally that erases the distinction that matters most when a job misbehaves: "this job is too slow for its window" and "this job is broken" need different responses, and right now they look identical in the history.
|
||||
|
||||
FIX: catch asyncio.TimeoutError explicitly in _run_executor, ahead of the broad handler, and let it propagate (or return a marker execute_task can distinguish). Note the ordering is the whole bug — a narrower except after a broader one is dead code, and there is no linter configured here to say so.
|
||||
|
||||
Related: a thread started by asyncio.to_thread cannot be cancelled, so after T-74 a timeout frees the loop while the work continues to completion. Whatever ''timeout'' comes to mean should say so rather than implying the work stopped.', 'backlog', 'medium', NULL, NULL, NULL, '2026-08-11 09:59:05.427', '2026-08-11 09:59:05.545', NULL, 'ae0db866086e38b681b0ea32837df277', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at;
|
||||
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQAR5KXM3PTG0QT3R1PV8', 'bug', NULL, 'A restart mid-task unschedules that task forever, silently', 'get_tasks_for_minute (src/tasks/executor.py:70) excludes any task holding a task_executions row with status=''running'':
|
||||
|
||||
AND id NOT IN (SELECT task_id FROM task_executions WHERE status = ''running'')
|
||||
|
||||
Nothing ever reconciles that row. It is written before the executor runs and updated after, so a process that dies in between leaves it ''running'' permanently — and the task is then excluded from every future minute, forever, with no error, no alarm and no log line. The task simply stops existing as far as the scheduler is concerned.
|
||||
|
||||
This is not hypothetical. test_example_task has held a ''running'' row since 2025-12-07 — 5916 hours. It happens to be disabled, so nothing is broken today; the mechanism is what matters, not this instance.
|
||||
|
||||
The exposure is real and daily: Watchtower restarts this container every morning at 4 AM. Any task still running at that moment is permanently unscheduled by it. The config backup runs 03:05 and takes ~21 minutes, which is not far off.
|
||||
|
||||
The failure mode is the one this system keeps producing: it looks like nothing. A backup that stops running forever produces no failure — it produces silence, and silence reads as health.
|
||||
|
||||
FIX: reconcile at startup. Any row left ''running'' when the process starts cannot be running, because the process that owned it is gone. Mark those ''orphaned'' (or ''failed'' with a message naming the restart) during app startup, before the scheduler begins its first minute. Consider also a stale-row guard for rows older than the task''s timeout_seconds, which covers a killed worker without a restart.
|
||||
|
||||
The audit trail is preserved either way — this is a status correction, not a deletion.
|
||||
|
||||
FIXED in v1.6.0 (e45de4f). TaskExecutor.reconcile_orphaned_executions() runs in the lifespan before the scheduler starts.
|
||||
|
||||
Proven on the real system rather than in a test. The row that had been ''running'' since 2025-12-07 22:44 was released at startup:
|
||||
|
||||
WARNING - Orphaned execution recovered: test_example_task was left ''running''
|
||||
since 2025-12-07 22:44:00.046695. That task had been excluded from
|
||||
scheduling until now.
|
||||
|
||||
Rows still ''running'' afterwards: 0. Status is ''orphaned'', error names the restart.
|
||||
|
||||
''orphaned'' rather than ''failed'' because the outcome is genuinely unknown — a task that completed and never got to update its row looks exactly like one that died halfway, and ''failed'' would assert something nobody observed.
|
||||
|
||||
Ordering is part of the fix and is mutation-checked: reconciliation must complete before the first minute is processed, or the first tick still sees the stale rows. Removing the startup call fails the test.
|
||||
|
||||
Not extended to a duration-based sweep, deliberately. While the process lives, execute_task''s finally always closes the row out, so a stale row implies a dead owner; a time-based rule would have to distinguish a slow task from a dead one and would eventually close the record of a task that is still working.', 'backlog', 'high', NULL, NULL, NULL, '2026-08-11 09:59:05.153', '2026-08-11 10:11:39.058', NULL, '8d88d4684f803319131765d97772e64d', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at;
|
||||
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQAR5KXM3PTG0QT3R1PV8', 'bug', NULL, 'A restart mid-task unschedules that task forever, silently', 'get_tasks_for_minute (src/tasks/executor.py:70) excludes any task holding a task_executions row with status=''running'':
|
||||
|
||||
AND id NOT IN (SELECT task_id FROM task_executions WHERE status = ''running'')
|
||||
|
||||
Nothing ever reconciles that row. It is written before the executor runs and updated after, so a process that dies in between leaves it ''running'' permanently — and the task is then excluded from every future minute, forever, with no error, no alarm and no log line. The task simply stops existing as far as the scheduler is concerned.
|
||||
|
||||
This is not hypothetical. test_example_task has held a ''running'' row since 2025-12-07 — 5916 hours. It happens to be disabled, so nothing is broken today; the mechanism is what matters, not this instance.
|
||||
|
||||
The exposure is real and daily: Watchtower restarts this container every morning at 4 AM. Any task still running at that moment is permanently unscheduled by it. The config backup runs 03:05 and takes ~21 minutes, which is not far off.
|
||||
|
||||
The failure mode is the one this system keeps producing: it looks like nothing. A backup that stops running forever produces no failure — it produces silence, and silence reads as health.
|
||||
|
||||
FIX: reconcile at startup. Any row left ''running'' when the process starts cannot be running, because the process that owned it is gone. Mark those ''orphaned'' (or ''failed'' with a message naming the restart) during app startup, before the scheduler begins its first minute. Consider also a stale-row guard for rows older than the task''s timeout_seconds, which covers a killed worker without a restart.
|
||||
|
||||
The audit trail is preserved either way — this is a status correction, not a deletion.
|
||||
|
||||
FIXED in v1.6.0 (e45de4f). TaskExecutor.reconcile_orphaned_executions() runs in the lifespan before the scheduler starts.
|
||||
|
||||
Proven on the real system rather than in a test. The row that had been ''running'' since 2025-12-07 22:44 was released at startup:
|
||||
|
||||
WARNING - Orphaned execution recovered: test_example_task was left ''running''
|
||||
since 2025-12-07 22:44:00.046695. That task had been excluded from
|
||||
scheduling until now.
|
||||
|
||||
Rows still ''running'' afterwards: 0. Status is ''orphaned'', error names the restart.
|
||||
|
||||
''orphaned'' rather than ''failed'' because the outcome is genuinely unknown — a task that completed and never got to update its row looks exactly like one that died halfway, and ''failed'' would assert something nobody observed.
|
||||
|
||||
Ordering is part of the fix and is mutation-checked: reconciliation must complete before the first minute is processed, or the first tick still sees the stale rows. Removing the startup call fails the test.
|
||||
|
||||
Not extended to a duration-based sweep, deliberately. While the process lives, execute_task''s finally always closes the row out, so a stale row implies a dead owner; a time-based rule would have to distinguish a slow task from a dead one and would eventually close the record of a task that is still working.', 'done', 'high', NULL, NULL, NULL, '2026-08-11 09:59:05.153', '2026-08-11 10:11:39.177', NULL, '99284d5f4a53a35637ee10c7b6b66c6e', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at;
|
||||
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQBTCRCTXK6EJF0T8CS2C', 'bug', NULL, 'The ''timeout'' execution status is unreachable; timeouts are filed as generic failures', 'execute_task has an `except asyncio.TimeoutError` handler (src/tasks/executor.py:179) that records status=''timeout''. It can never run.
|
||||
|
||||
_run_executor wraps the awaited call in `except Exception` (line 219) and returns the error as a value. Since Python 3.11 asyncio.TimeoutError IS the builtin TimeoutError, which inherits OSError -> Exception, so the broad handler catches it first and converts it into an ordinary (None, error) tuple. execute_task then sees a non-None error and files status=''failed''.
|
||||
|
||||
Verified in the deployed runtime:
|
||||
asyncio.TimeoutError is TimeoutError: True
|
||||
MRO: TimeoutError -> OSError -> Exception -> BaseException
|
||||
|
||||
And confirmed against 8 months of history — 18,785 executions, and the count of status=''timeout'' rows is zero:
|
||||
success 16690 | failed 2093 | running 2 | timeout 0
|
||||
|
||||
So every timeout since 2025-12-07 has been recorded as a generic failure carrying a traceback. Operationally that erases the distinction that matters most when a job misbehaves: "this job is too slow for its window" and "this job is broken" need different responses, and right now they look identical in the history.
|
||||
|
||||
FIX: catch asyncio.TimeoutError explicitly in _run_executor, ahead of the broad handler, and let it propagate (or return a marker execute_task can distinguish). Note the ordering is the whole bug — a narrower except after a broader one is dead code, and there is no linter configured here to say so.
|
||||
|
||||
Related: a thread started by asyncio.to_thread cannot be cancelled, so after T-74 a timeout frees the loop while the work continues to completion. Whatever ''timeout'' comes to mean should say so rather than implying the work stopped.
|
||||
|
||||
FIXED in v1.6.0 (e45de4f). _run_executor now catches asyncio.TimeoutError ahead of the broad handler and re-raises, so execute_task''s timeout branch is reachable for the first time.
|
||||
|
||||
A trap surfaced while fixing it, and it is the interesting part. While the branch was unreachable, a timeout travelled the NORMAL path — which does update scheduled_tasks. Making the branch reachable without adding that write would have swapped a wrong status (''failed'') for a stale one (last_status showing the previous run''s outcome indefinitely). _update_task_outcome now mirrors terminal outcomes onto the parent row. A fix that repairs the reported symptom while breaking something adjacent is worse than the bug.
|
||||
|
||||
The timeout message also records that the work may still be running: after T-74 executors are handed to asyncio.to_thread, and a thread cannot be cancelled, so wait_for frees the loop while the work continues to completion. Saying "timed out" without that would imply the work stopped.
|
||||
|
||||
Mutation-checked: removing the narrow except clause fails the propagation test. A third test pins that ordinary exceptions are still returned rather than raised, so the narrow clause cannot start swallowing anything else.', 'backlog', 'medium', NULL, NULL, NULL, '2026-08-11 09:59:05.427', '2026-08-11 10:11:39.291', NULL, 'b7379fa662b40c6996f17c36b64010c0', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at;
|
||||
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0GQBTCRCTXK6EJF0T8CS2C', 'bug', NULL, 'The ''timeout'' execution status is unreachable; timeouts are filed as generic failures', 'execute_task has an `except asyncio.TimeoutError` handler (src/tasks/executor.py:179) that records status=''timeout''. It can never run.
|
||||
|
||||
_run_executor wraps the awaited call in `except Exception` (line 219) and returns the error as a value. Since Python 3.11 asyncio.TimeoutError IS the builtin TimeoutError, which inherits OSError -> Exception, so the broad handler catches it first and converts it into an ordinary (None, error) tuple. execute_task then sees a non-None error and files status=''failed''.
|
||||
|
||||
Verified in the deployed runtime:
|
||||
asyncio.TimeoutError is TimeoutError: True
|
||||
MRO: TimeoutError -> OSError -> Exception -> BaseException
|
||||
|
||||
And confirmed against 8 months of history — 18,785 executions, and the count of status=''timeout'' rows is zero:
|
||||
success 16690 | failed 2093 | running 2 | timeout 0
|
||||
|
||||
So every timeout since 2025-12-07 has been recorded as a generic failure carrying a traceback. Operationally that erases the distinction that matters most when a job misbehaves: "this job is too slow for its window" and "this job is broken" need different responses, and right now they look identical in the history.
|
||||
|
||||
FIX: catch asyncio.TimeoutError explicitly in _run_executor, ahead of the broad handler, and let it propagate (or return a marker execute_task can distinguish). Note the ordering is the whole bug — a narrower except after a broader one is dead code, and there is no linter configured here to say so.
|
||||
|
||||
Related: a thread started by asyncio.to_thread cannot be cancelled, so after T-74 a timeout frees the loop while the work continues to completion. Whatever ''timeout'' comes to mean should say so rather than implying the work stopped.
|
||||
|
||||
FIXED in v1.6.0 (e45de4f). _run_executor now catches asyncio.TimeoutError ahead of the broad handler and re-raises, so execute_task''s timeout branch is reachable for the first time.
|
||||
|
||||
A trap surfaced while fixing it, and it is the interesting part. While the branch was unreachable, a timeout travelled the NORMAL path — which does update scheduled_tasks. Making the branch reachable without adding that write would have swapped a wrong status (''failed'') for a stale one (last_status showing the previous run''s outcome indefinitely). _update_task_outcome now mirrors terminal outcomes onto the parent row. A fix that repairs the reported symptom while breaking something adjacent is worse than the bug.
|
||||
|
||||
The timeout message also records that the work may still be running: after T-74 executors are handed to asyncio.to_thread, and a thread cannot be cancelled, so wait_for frees the loop while the work continues to completion. Saying "timed out" without that would imply the work stopped.
|
||||
|
||||
Mutation-checked: removing the narrow except clause fails the propagation test. A third test pins that ordinary exceptions are still returned rather than raised, so the narrow clause cannot start swallowing anything else.', 'done', 'medium', NULL, NULL, NULL, '2026-08-11 09:59:05.427', '2026-08-11 10:11:39.420', NULL, '333edb040cfa995cf26f45b6fa2cd2f9', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at;
|
||||
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0PNFBDB743QJQ9MF38MPJ8', 'task', NULL, 'check_history rows name the task that produced them', NULL, 'backlog', 'medium', NULL, NULL, NULL, '2026-08-11 10:25:02.811', '2026-08-11 10:25:02.811', NULL, 'b099197d1cbdea7e682a054dc17788ab', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at;
|
||||
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0PNFBDB743QJQ9MF38MPJ8', 'task', NULL, 'check_history rows name the task that produced them', 'DONE in v1.7.0 (538bfe5). Verified on a real row:
|
||||
|
||||
12:24:27 ok task=backup_portainer_daily source=scheduler/portainer_backup_executor
|
||||
|
||||
WHY, and why not the other fix. A row written by a failing probe on 2026-08-11 was byte-for-byte what a nightly backup failure would have written — same source, same domain, nothing naming the task. The reflex was to delete the inconvenient row; that was refused as audit tampering and the refusal was right twice over, because the row is TRUE (a run did fail) and the defect is that it cannot say which run. Attribution keeps the record intact and makes it answer the question.
|
||||
|
||||
MECHANISM. A ContextVar (src/task_context.py) set by the engine around executor invocation, read by health_report. Not an argument: executors are invoked as execute(config, settings), there are ten, and several are dormant — they exist only as a string in a database row and become live the moment someone inserts a task naming them. A signature change would leave those broken in a way no import, grep or test would reveal. Not injected into `config` either: config is what a human wrote in the task definition and an executor may legitimately reject unknown keys.
|
||||
|
||||
TWO PROPERTIES VERIFIED IN THE DEPLOYED RUNTIME rather than assumed:
|
||||
- asyncio.to_thread propagates context, so attribution survives the worker thread T-74 introduced. Had it not, every row from a real executor would have lost its task while unit tests kept passing — hence a test that goes through report_async specifically.
|
||||
- Each asyncio Task gets its own copy, so five concurrent executions cannot read each other''s. The isolation test yields mid-execution to force interleaving; without that it would pass against a shared global.
|
||||
- A plain await does NOT get its own copy and leaks to the caller. Both entry points use create_task, but task_scope resets by token rather than relying on it.
|
||||
|
||||
Omitted rather than nulled when absent — report() is callable from a script, and a null would claim a task existed with no name.
|
||||
|
||||
The one pre-existing critical row still carries no task, which now identifies it: it is the only backup row without the field, so it is legible as pre-attribution rather than ambiguous.', 'backlog', 'medium', NULL, NULL, NULL, '2026-08-11 10:25:02.811', '2026-08-11 10:25:02.951', NULL, '5b7bbb05cf6aedfbd0a6a49a356f6e6f', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at;
|
||||
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FZ0PNFBDB743QJQ9MF38MPJ8', 'task', NULL, 'check_history rows name the task that produced them', 'DONE in v1.7.0 (538bfe5). Verified on a real row:
|
||||
|
||||
12:24:27 ok task=backup_portainer_daily source=scheduler/portainer_backup_executor
|
||||
|
||||
WHY, and why not the other fix. A row written by a failing probe on 2026-08-11 was byte-for-byte what a nightly backup failure would have written — same source, same domain, nothing naming the task. The reflex was to delete the inconvenient row; that was refused as audit tampering and the refusal was right twice over, because the row is TRUE (a run did fail) and the defect is that it cannot say which run. Attribution keeps the record intact and makes it answer the question.
|
||||
|
||||
MECHANISM. A ContextVar (src/task_context.py) set by the engine around executor invocation, read by health_report. Not an argument: executors are invoked as execute(config, settings), there are ten, and several are dormant — they exist only as a string in a database row and become live the moment someone inserts a task naming them. A signature change would leave those broken in a way no import, grep or test would reveal. Not injected into `config` either: config is what a human wrote in the task definition and an executor may legitimately reject unknown keys.
|
||||
|
||||
TWO PROPERTIES VERIFIED IN THE DEPLOYED RUNTIME rather than assumed:
|
||||
- asyncio.to_thread propagates context, so attribution survives the worker thread T-74 introduced. Had it not, every row from a real executor would have lost its task while unit tests kept passing — hence a test that goes through report_async specifically.
|
||||
- Each asyncio Task gets its own copy, so five concurrent executions cannot read each other''s. The isolation test yields mid-execution to force interleaving; without that it would pass against a shared global.
|
||||
- A plain await does NOT get its own copy and leaks to the caller. Both entry points use create_task, but task_scope resets by token rather than relying on it.
|
||||
|
||||
Omitted rather than nulled when absent — report() is callable from a script, and a null would claim a task existed with no name.
|
||||
|
||||
The one pre-existing critical row still carries no task, which now identifies it: it is the only backup row without the field, so it is legible as pre-attribution rather than ambiguous.', 'done', 'medium', NULL, NULL, NULL, '2026-08-11 10:25:02.811', '2026-08-11 10:25:03.084', NULL, '63f6b48e753891a75d47dd0b3d7b5a4c', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at;
|
||||
@@ -6,6 +6,35 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
## [1.8.0] - 2026-08-11
|
||||
|
||||
### Fixed
|
||||
- Deleting a task that has run returns 409 instead of a bare 500. It failed on a foreign key
|
||||
against its own execution history, which the error never mentioned.
|
||||
|
||||
### Added
|
||||
- `DELETE /tasks/{name}?purge=true` removes a task together with its execution history, in one
|
||||
transaction. The response reports `executions_purged`.
|
||||
|
||||
## [1.7.0] - 2026-08-11
|
||||
|
||||
### Added
|
||||
- `check_history` rows name the scheduled task that produced them. `source` names the code,
|
||||
which cannot distinguish two tasks sharing one executor — so a failure could not be
|
||||
attributed to the job that caused it. Omitted when there is no task.
|
||||
|
||||
## [1.6.0] - 2026-08-11
|
||||
|
||||
### Fixed
|
||||
- A restart mid-task no longer unschedules that task forever. An execution row left
|
||||
`running` excluded its task from every future minute, silently; startup now releases them.
|
||||
- Timeouts are recorded as `timeout` instead of a generic failure. The status existed but was
|
||||
unreachable, so all 18,785 executions since December contain zero of them.
|
||||
|
||||
### Added
|
||||
- `orphaned` execution status — an execution whose process died, whose outcome is unknown.
|
||||
Distinct from `failed`, which asserts the work did not succeed.
|
||||
|
||||
## [1.5.1] - 2026-08-11
|
||||
|
||||
### Fixed
|
||||
|
||||
@@ -9,7 +9,7 @@ It is the homelab's cron. Recurring work belongs here rather than in a systemd t
|
||||
|
||||
## Live contract
|
||||
|
||||
`http://localhost:8090/openapi.json` — 10 paths, `version: 1.4.0` (verified 2026-08-09). Human
|
||||
`http://localhost:8090/openapi.json` — 10 paths, `version: 1.8.0` (verified 2026-08-11). Human
|
||||
docs at `/docs`. Generated from running code, so read it instead of inferring routes.
|
||||
|
||||
**Every route except `/health` requires `Authorization: Bearer $SCHEDULER_API_KEY`.** An
|
||||
|
||||
+1
-1
@@ -1,6 +1,6 @@
|
||||
[project]
|
||||
name = "the-scheduler"
|
||||
version = "1.5.1"
|
||||
version = "1.8.0"
|
||||
description = "System-wide maintenance orchestration - backups, doc mirroring, cleanup, task automation"
|
||||
readme = "README.md"
|
||||
requires-python = ">=3.12"
|
||||
|
||||
@@ -20,6 +20,11 @@ Three consequences that are load-bearing here:
|
||||
|
||||
- `source` names the producer, because the table now has several writers and a
|
||||
row must say which one wrote it.
|
||||
- `task` names the schedule that invoked it, which `source` cannot: two tasks
|
||||
may share one executor. On 2026-08-11 two did, and their rows were identical
|
||||
apart from their contents — a failure could not be attributed to either. It
|
||||
comes from a ContextVar the engine sets (`src/task_context.py`), so executors
|
||||
need no signature change and dormant ones stay valid.
|
||||
- `domain` is a shared namespace. Two producers claiming one name would
|
||||
interleave silently.
|
||||
- **A reporting failure must never fail the task.** Backing up successfully and
|
||||
@@ -36,6 +41,8 @@ from typing import Any, Dict, Optional
|
||||
|
||||
import psycopg2
|
||||
|
||||
from src.task_context import current_task_name
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
# The database holding check_history. Not the scheduler's own database — this is
|
||||
@@ -61,6 +68,7 @@ def report(
|
||||
"""
|
||||
metrics = metrics or {}
|
||||
now = datetime.now(timezone.utc)
|
||||
task = current_task_name()
|
||||
result = {
|
||||
# The envelope the table has carried since the shell era. A reader of a
|
||||
# year of history should not have to know which producer wrote a row in
|
||||
@@ -71,6 +79,14 @@ def report(
|
||||
"status": status,
|
||||
"metrics": metrics,
|
||||
}
|
||||
# Which scheduled task produced this. `source` names the code; two tasks can
|
||||
# share one executor, and on 2026-08-11 two did — a 425 MB probe and the 5 GB
|
||||
# nightly backup wrote rows that were identical apart from their contents, so
|
||||
# a failure could not be attributed to either. Omitted rather than nulled
|
||||
# when absent, per the convention that a missing field means "not
|
||||
# applicable": report() is also callable from a script with no task around it.
|
||||
if task:
|
||||
result["task"] = task
|
||||
|
||||
try:
|
||||
conn = psycopg2.connect(
|
||||
|
||||
+60
-12
@@ -7,7 +7,7 @@ Architecture: Hybrid APScheduler + DB-based priority system
|
||||
- Job queries DB for tasks scheduled in that minute
|
||||
- Executes up to 5 tasks concurrently based on priority
|
||||
"""
|
||||
from fastapi import FastAPI, HTTPException, Depends, Header
|
||||
from fastapi import FastAPI, HTTPException, Depends, Header, Query
|
||||
from apscheduler.schedulers.asyncio import AsyncIOScheduler
|
||||
from apscheduler.jobstores.sqlalchemy import SQLAlchemyJobStore
|
||||
from apscheduler.triggers.cron import CronTrigger
|
||||
@@ -65,6 +65,13 @@ async def lifespan(app: FastAPI):
|
||||
task_executor = TaskExecutor(settings)
|
||||
logger.info("Task executor initialized (max 5 concurrent tasks)")
|
||||
|
||||
# Before the first minute is processed, release any task still held by an
|
||||
# execution row belonging to an instance that no longer exists. A 'running'
|
||||
# row excludes its task from scheduling permanently, so skipping this leaves
|
||||
# tasks silently unschedulable across every restart. Blocking briefly is fine
|
||||
# here — the app serves no requests until lifespan yields.
|
||||
task_executor.reconcile_orphaned_executions()
|
||||
|
||||
# Initialize APScheduler with minimal configuration
|
||||
# No jobstore needed - we only have one in-memory job
|
||||
scheduler = AsyncIOScheduler(
|
||||
@@ -346,26 +353,67 @@ async def update_task(
|
||||
@app.delete("/tasks/{task_name}")
|
||||
async def delete_task(
|
||||
task_name: str,
|
||||
purge: bool = Query(
|
||||
False,
|
||||
description="Also delete this task's execution history. Required when the "
|
||||
"task has ever run, and destroys its audit trail."
|
||||
),
|
||||
api_key: str = Depends(verify_api_key),
|
||||
executor: TaskExecutor = Depends(get_task_executor)
|
||||
):
|
||||
"""Delete a scheduled task."""
|
||||
"""Delete a scheduled task.
|
||||
|
||||
A task that has ever run owns rows in task_executions, and those rows are
|
||||
the audit trail — when it ran, how long it took, what it returned. Deleting
|
||||
the task alone violates task_executions_task_id_fkey, which surfaced as a
|
||||
bare 500 with no indication that history was the obstacle, so it read as the
|
||||
service being broken rather than the request being refusable. Since every
|
||||
task that has ever fired has history, the endpoint effectively worked only
|
||||
for tasks that had never run.
|
||||
|
||||
Refusing with 409 rather than cascading by default, because the two outcomes
|
||||
are not equally recoverable: a task definition can be recreated from the API
|
||||
in one call, its execution history cannot be recreated at all. The caller
|
||||
who wants both gone says so.
|
||||
|
||||
Disabling is usually what was actually wanted — it stops the task running and
|
||||
keeps the record — so the refusal names that too.
|
||||
"""
|
||||
with executor.get_db_connection() as conn:
|
||||
with conn.cursor() as cur:
|
||||
cur.execute("""
|
||||
DELETE FROM scheduled_tasks
|
||||
WHERE task_name = %s
|
||||
RETURNING task_name
|
||||
""", (task_name,))
|
||||
|
||||
deleted = cur.fetchone()
|
||||
if not deleted:
|
||||
cur.execute("SELECT id FROM scheduled_tasks WHERE task_name = %s", (task_name,))
|
||||
row = cur.fetchone()
|
||||
if not row:
|
||||
raise HTTPException(404, f"Task '{task_name}' not found")
|
||||
task_id = row[0]
|
||||
|
||||
cur.execute("SELECT COUNT(*) FROM task_executions WHERE task_id = %s", (task_id,))
|
||||
executions = cur.fetchone()[0]
|
||||
|
||||
if executions and not purge:
|
||||
raise HTTPException(
|
||||
409,
|
||||
f"Task '{task_name}' has {executions} execution record(s). "
|
||||
f"Deleting it would discard that history. Re-send with "
|
||||
f"?purge=true to delete the task and its history together, or "
|
||||
f"PUT enabled=false to stop it running while keeping the record."
|
||||
)
|
||||
|
||||
# One transaction: a purge that removed the history and then failed to
|
||||
# remove the task would leave the audit trail gone and the task alive.
|
||||
if executions:
|
||||
cur.execute("DELETE FROM task_executions WHERE task_id = %s", (task_id,))
|
||||
cur.execute("DELETE FROM scheduled_tasks WHERE id = %s", (task_id,))
|
||||
conn.commit()
|
||||
|
||||
logger.info(f"Deleted task: {task_name}")
|
||||
return {"message": f"Task '{task_name}' deleted successfully"}
|
||||
if executions:
|
||||
logger.warning(f"Deleted task {task_name} and purged {executions} execution record(s)")
|
||||
else:
|
||||
logger.info(f"Deleted task: {task_name}")
|
||||
return {
|
||||
"message": f"Task '{task_name}' deleted successfully",
|
||||
"executions_purged": executions,
|
||||
}
|
||||
|
||||
@app.post("/tasks/{task_name}/trigger")
|
||||
async def trigger_task(
|
||||
|
||||
@@ -0,0 +1,59 @@
|
||||
"""Which scheduled task is currently executing.
|
||||
|
||||
Executors are invoked as `execute(config, settings)` and are never told which
|
||||
task they are. That is fine until one of them writes to a shared record: on
|
||||
2026-08-11 a probe task and the nightly backup both used
|
||||
config_backup_executor, and the rows they wrote into `check_history` were
|
||||
indistinguishable — same `source`, same `domain`. A failure from a 425 MB test
|
||||
archive was impossible to tell from a failure of the 5 GB nightly job, so the
|
||||
health record could not answer "which one broke?".
|
||||
|
||||
A ContextVar rather than a parameter, because the alternatives are worse here:
|
||||
|
||||
- Threading a `task` argument through `execute(config, settings, task)` is a
|
||||
signature change across every executor, including the dormant ones that
|
||||
exist only as a string in a database row and would break the moment
|
||||
somebody enabled them.
|
||||
- Injecting the name into `config` corrupts the thing executors validate.
|
||||
`config` is what a human wrote in the task definition, and an executor is
|
||||
entitled to reject keys it does not recognise.
|
||||
|
||||
Two properties make this safe, both verified in the deployed runtime rather
|
||||
than assumed:
|
||||
|
||||
- `asyncio.to_thread` propagates the context, so a reporter still sees the
|
||||
task after T-74 moved executor bodies into worker threads.
|
||||
- Each asyncio Task gets its own copy, so the five concurrent executions
|
||||
allowed by MAX_CONCURRENT_TASKS cannot read each other's value.
|
||||
|
||||
A plain `await` of a coroutine does NOT get its own copy and would leak the
|
||||
value back to the caller, so `task_scope` resets it rather than relying on the
|
||||
call always arriving via create_task.
|
||||
"""
|
||||
from contextlib import contextmanager
|
||||
from contextvars import ContextVar
|
||||
from typing import Optional
|
||||
|
||||
_current_task_name: ContextVar[Optional[str]] = ContextVar(
|
||||
"current_task_name", default=None
|
||||
)
|
||||
|
||||
|
||||
@contextmanager
|
||||
def task_scope(task_name: str):
|
||||
"""Name the executing task for the duration of the block."""
|
||||
token = _current_task_name.set(task_name)
|
||||
try:
|
||||
yield
|
||||
finally:
|
||||
_current_task_name.reset(token)
|
||||
|
||||
|
||||
def current_task_name() -> Optional[str]:
|
||||
"""The executing task's name, or None outside an execution.
|
||||
|
||||
None is a real answer, not an error: a reporter may be called from a script
|
||||
or a test with no task around it. Callers omit the field rather than
|
||||
inventing one.
|
||||
"""
|
||||
return _current_task_name.get()
|
||||
+122
-4
@@ -11,6 +11,7 @@ from psycopg2.extras import RealDictCursor
|
||||
import traceback
|
||||
|
||||
from src.config import Settings
|
||||
from src.task_context import task_scope
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@@ -115,6 +116,70 @@ class TaskExecutor:
|
||||
|
||||
return True
|
||||
|
||||
def reconcile_orphaned_executions(self) -> int:
|
||||
"""Close out execution rows left 'running' by a process that is gone.
|
||||
|
||||
get_tasks_for_minute excludes any task holding a 'running' row. That row
|
||||
is written before the executor runs and updated after, so a process that
|
||||
dies in between leaves it 'running' forever — and the task is then
|
||||
excluded from every future minute, permanently, with no error and no log
|
||||
line. It does not fail; it goes quiet, and quiet reads as healthy.
|
||||
|
||||
Live exposure rather than theory: Watchtower restarts this container at
|
||||
4 AM daily, and the config backup starts at 03:05 and runs ~21 minutes.
|
||||
A row from 2025-12-07 sat 'running' for eight months before anyone
|
||||
looked.
|
||||
|
||||
Called at startup, where the reasoning is sound by construction: this
|
||||
process has just begun, so nothing it can see is genuinely running, and
|
||||
any such row belongs to an instance that no longer exists.
|
||||
|
||||
Marked 'orphaned', not 'failed'. When the process dies mid-task the work
|
||||
may well have finished — a backup that completed and never got to update
|
||||
its row is indistinguishable from one that died halfway. 'failed' would
|
||||
assert an outcome nobody observed. 'orphaned' says only what is known:
|
||||
we lost track of it.
|
||||
|
||||
Deliberately not extended to a time-based sweep of long-running rows.
|
||||
While this process lives, execute_task's finally clause always closes the
|
||||
row out, so a stale row implies a dead owner. A duration-based rule would
|
||||
have to tell a slow task from a dead one, and getting that wrong closes
|
||||
the record of a task that is still working.
|
||||
"""
|
||||
try:
|
||||
with self.get_db_connection() as conn:
|
||||
with conn.cursor() as cur:
|
||||
cur.execute("""
|
||||
UPDATE task_executions
|
||||
SET status = 'orphaned',
|
||||
completed_at = %s,
|
||||
error = 'Scheduler restarted while this execution was '
|
||||
'running; its outcome is unknown.'
|
||||
WHERE status = 'running'
|
||||
RETURNING task_name, started_at
|
||||
""", (datetime.now(timezone.utc),))
|
||||
orphans = cur.fetchall()
|
||||
conn.commit()
|
||||
except Exception as e:
|
||||
# Never fatal. A scheduler that refuses to start because it could not
|
||||
# tidy up is worse than one carrying a stale row.
|
||||
logger.error(f"Could not reconcile orphaned executions: {e}")
|
||||
return 0
|
||||
|
||||
for task_name, started_at in orphans:
|
||||
logger.warning(
|
||||
f"Orphaned execution recovered: {task_name} was left 'running' "
|
||||
f"since {started_at}. That task had been excluded from scheduling "
|
||||
f"until now."
|
||||
)
|
||||
if orphans:
|
||||
logger.warning(
|
||||
f"{len(orphans)} task(s) were unschedulable and are now released."
|
||||
)
|
||||
else:
|
||||
logger.info("No orphaned executions to reconcile")
|
||||
return len(orphans)
|
||||
|
||||
async def execute_task(self, task: Dict[str, Any]):
|
||||
"""
|
||||
Execute a single task with timeout and error handling.
|
||||
@@ -145,8 +210,11 @@ class TaskExecutor:
|
||||
execution_id = cur.fetchone()[0]
|
||||
conn.commit()
|
||||
|
||||
# Load and execute the task
|
||||
output, error = await self._run_executor(executor_name, task, timeout)
|
||||
# Load and execute the task. The scope names it for anything the
|
||||
# executor writes to a shared record — without it, two tasks sharing
|
||||
# one executor produce rows nobody can tell apart.
|
||||
with task_scope(task_name):
|
||||
output, error = await self._run_executor(executor_name, task, timeout)
|
||||
|
||||
completed_at = datetime.now(timezone.utc)
|
||||
duration = int((completed_at - started_at).total_seconds())
|
||||
@@ -178,8 +246,17 @@ class TaskExecutor:
|
||||
|
||||
except asyncio.TimeoutError:
|
||||
logger.error(f"Task {task_name} timed out after {timeout}s")
|
||||
self._update_execution_status(execution_id, 'timeout',
|
||||
error=f"Task exceeded timeout of {timeout}s")
|
||||
self._update_execution_status(
|
||||
execution_id, 'timeout',
|
||||
error=f"Task exceeded timeout of {timeout}s. The underlying work may "
|
||||
f"still be running — executors that use asyncio.to_thread hand "
|
||||
f"the work to a thread, and a thread cannot be cancelled.")
|
||||
# scheduled_tasks has to be written here as well. On the normal path
|
||||
# it is updated alongside the execution row, and while this branch was
|
||||
# unreachable a timeout travelled that path as a 'failed' result — so
|
||||
# last_status did stay current. Making the branch reachable without
|
||||
# this call would swap one wrong status for a stale one.
|
||||
self._update_task_outcome(task_id, 'timeout', started_at)
|
||||
|
||||
except Exception as e:
|
||||
logger.error(f"Task {task_name} failed with exception: {e}")
|
||||
@@ -214,11 +291,52 @@ class TaskExecutor:
|
||||
|
||||
return result, None
|
||||
|
||||
except asyncio.TimeoutError:
|
||||
# This clause must precede `except Exception`, and that ordering is
|
||||
# the entire bug it fixes. Since Python 3.11 asyncio.TimeoutError IS
|
||||
# the builtin TimeoutError, which inherits OSError -> Exception, so
|
||||
# the broad handler below used to catch it first and convert it into
|
||||
# an ordinary (None, error) tuple. execute_task then filed it as a
|
||||
# generic 'failed', and its own `except asyncio.TimeoutError` branch
|
||||
# was unreachable: zero 'timeout' rows across 18,785 executions and
|
||||
# eight months of history.
|
||||
#
|
||||
# Re-raised rather than returned, because the distinction is the
|
||||
# point: "too slow for its window" and "broken" call for different
|
||||
# responses and were indistinguishable in the record.
|
||||
raise
|
||||
|
||||
except ModuleNotFoundError:
|
||||
return None, f"Executor module not found: {executor_name}"
|
||||
except Exception as e:
|
||||
return None, f"Executor error: {str(e)}\n{traceback.format_exc()}"
|
||||
|
||||
def _update_task_outcome(self, task_id: int, status: str, started_at: datetime):
|
||||
"""Mirror a terminal outcome onto scheduled_tasks.
|
||||
|
||||
The happy path writes task_executions and scheduled_tasks in one
|
||||
transaction. The error branches historically wrote only the former, which
|
||||
did not show while every timeout was being funnelled through the happy
|
||||
path as a 'failed'. Once a branch bypasses that path it has to keep
|
||||
last_run/last_status current itself, or the task list quietly reports the
|
||||
previous run's outcome as though it were the latest.
|
||||
"""
|
||||
try:
|
||||
completed_at = datetime.now(timezone.utc)
|
||||
with self.get_db_connection() as conn:
|
||||
with conn.cursor() as cur:
|
||||
cur.execute("""
|
||||
UPDATE scheduled_tasks
|
||||
SET last_run = %s, last_status = %s,
|
||||
last_duration_seconds = %s, updated_at = %s
|
||||
WHERE id = %s
|
||||
""", (completed_at, status,
|
||||
int((completed_at - started_at).total_seconds()),
|
||||
completed_at, task_id))
|
||||
conn.commit()
|
||||
except Exception as e:
|
||||
logger.error(f"Failed to update task outcome for {task_id}: {e}")
|
||||
|
||||
def _update_execution_status(self, execution_id: int, status: str, error: str = None):
|
||||
"""Update execution record with final status."""
|
||||
if execution_id is None:
|
||||
|
||||
@@ -0,0 +1,154 @@
|
||||
"""A check_history row must say which task produced it.
|
||||
|
||||
On 2026-08-11 a 425 MB probe task and the 5 GB nightly backup both ran through
|
||||
config_backup_executor. The probe failed, and the row it wrote was
|
||||
indistinguishable from a nightly-backup failure — same `source`, same `domain`,
|
||||
nothing naming the task. The health record could not answer which job broke,
|
||||
which is most of what a health record is for.
|
||||
|
||||
Executors are called as `execute(config, settings)` and are never told which
|
||||
task they are, so the name travels in a ContextVar. These tests pin the three
|
||||
properties that makes safe: it reaches the reporter, it survives the worker
|
||||
thread T-74 introduced, and concurrent executions cannot read each other's.
|
||||
"""
|
||||
import asyncio
|
||||
import json
|
||||
|
||||
import pytest
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
from src.config import Settings
|
||||
from src.executors import health_report
|
||||
from src.task_context import current_task_name, task_scope
|
||||
from src.tasks.executor import TaskExecutor
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def captured_row(monkeypatch):
|
||||
"""Capture the JSON payload report() would insert, without a database."""
|
||||
box = {}
|
||||
conn, cur = MagicMock(), MagicMock()
|
||||
|
||||
def execute(sql, params):
|
||||
box['result'] = json.loads(params[4])
|
||||
|
||||
cur.execute.side_effect = execute
|
||||
conn.cursor.return_value.__enter__ = MagicMock(return_value=cur)
|
||||
conn.cursor.return_value.__exit__ = MagicMock(return_value=None)
|
||||
conn.__enter__ = MagicMock(return_value=conn)
|
||||
conn.__exit__ = MagicMock(return_value=None)
|
||||
monkeypatch.setattr(health_report.psycopg2, 'connect', lambda **kw: conn)
|
||||
return box
|
||||
|
||||
|
||||
def _report(settings, **kw):
|
||||
health_report.report(
|
||||
settings, domain="backup", status=health_report.OK,
|
||||
source="scheduler/config_backup_executor", metrics={}, **kw
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestAttribution:
|
||||
|
||||
def test_the_row_names_the_task(self, test_settings: Settings, captured_row):
|
||||
with task_scope("backup_docker_configs_daily"):
|
||||
_report(test_settings)
|
||||
assert captured_row['result']['task'] == "backup_docker_configs_daily"
|
||||
|
||||
def test_source_still_names_the_code(self, test_settings: Settings, captured_row):
|
||||
"""Task and source answer different questions; both are needed.
|
||||
|
||||
`source` says which code wrote the row, `task` says which schedule
|
||||
invoked it. Replacing one with the other loses a distinction.
|
||||
"""
|
||||
with task_scope("t74_loop_probe"):
|
||||
_report(test_settings)
|
||||
r = captured_row['result']
|
||||
assert r['source'] == "scheduler/config_backup_executor"
|
||||
assert r['task'] == "t74_loop_probe"
|
||||
|
||||
def test_the_field_is_omitted_rather_than_nulled(self, test_settings: Settings, captured_row):
|
||||
"""report() is callable from a script with no task around it.
|
||||
|
||||
A null would claim there was a task and it had no name. Absence says the
|
||||
question does not apply.
|
||||
"""
|
||||
_report(test_settings)
|
||||
assert 'task' not in captured_row['result']
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_attribution_survives_the_worker_thread(
|
||||
self, test_settings: Settings, captured_row
|
||||
):
|
||||
"""T-74 moved reporting into asyncio.to_thread. Attribution has to follow.
|
||||
|
||||
If the context did not propagate, every row written by a real executor
|
||||
would silently lose its task while the tests above still passed.
|
||||
"""
|
||||
with task_scope("backup_portainer_daily"):
|
||||
await health_report.report_async(
|
||||
test_settings, domain="backup", status=health_report.OK,
|
||||
source="scheduler/portainer_backup_executor", metrics={},
|
||||
)
|
||||
assert captured_row['result']['task'] == "backup_portainer_daily"
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestAttributionIsolation:
|
||||
"""MAX_CONCURRENT_TASKS is 5, so mixing values between them is a live risk."""
|
||||
|
||||
def _executor_with_db(self, test_settings):
|
||||
executor = TaskExecutor(test_settings)
|
||||
conn, cur = MagicMock(), MagicMock()
|
||||
cur.fetchone.return_value = [1]
|
||||
conn.cursor.return_value.__enter__ = MagicMock(return_value=cur)
|
||||
conn.cursor.return_value.__exit__ = MagicMock(return_value=None)
|
||||
conn.__enter__ = MagicMock(return_value=conn)
|
||||
conn.__exit__ = MagicMock(return_value=None)
|
||||
return executor, patch.object(executor, 'get_db_connection', return_value=conn)
|
||||
|
||||
def _task(self, name):
|
||||
return {'id': hash(name) % 1000, 'task_name': name, 'executor': 'x',
|
||||
'service': 'scheduler', 'priority': 5, 'timeout_seconds': 60}
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_the_engine_names_the_task_and_clears_it_after(
|
||||
self, test_settings: Settings
|
||||
):
|
||||
executor, db = self._executor_with_db(test_settings)
|
||||
seen = {}
|
||||
|
||||
async def fake_run(name, task, timeout):
|
||||
seen['during'] = current_task_name()
|
||||
return "done", None
|
||||
|
||||
with db, patch.object(executor, '_run_executor', new=fake_run):
|
||||
await executor.execute_task(self._task("nightly_backup"))
|
||||
|
||||
assert seen['during'] == "nightly_backup", "executor ran unattributed"
|
||||
assert current_task_name() is None, "attribution leaked past the execution"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_concurrent_executions_do_not_see_each_other(
|
||||
self, test_settings: Settings
|
||||
):
|
||||
executor, db = self._executor_with_db(test_settings)
|
||||
seen = {}
|
||||
|
||||
async def fake_run(name, task, timeout):
|
||||
who = task['task_name']
|
||||
# Yield mid-flight so the executions genuinely interleave; without
|
||||
# this they would run to completion one at a time and the test would
|
||||
# pass even with a shared global.
|
||||
await asyncio.sleep(0.01 if who == "slow_one" else 0)
|
||||
seen[who] = current_task_name()
|
||||
return "done", None
|
||||
|
||||
with db, patch.object(executor, '_run_executor', new=fake_run):
|
||||
await asyncio.gather(
|
||||
executor.execute_task(self._task("slow_one")),
|
||||
executor.execute_task(self._task("fast_one")),
|
||||
)
|
||||
|
||||
assert seen == {"slow_one": "slow_one", "fast_one": "fast_one"}
|
||||
@@ -0,0 +1,112 @@
|
||||
"""Deleting a task must not destroy its history by accident, or 500 by surprise.
|
||||
|
||||
DELETE /tasks/{name} used to issue a bare DELETE against scheduled_tasks. Any
|
||||
task that had ever run owned rows in task_executions, so the foreign key
|
||||
rejected it and the caller got "Internal Server Error" with nothing pointing at
|
||||
history as the obstacle — it read as the service being broken rather than the
|
||||
request being refusable. Since every task that has ever fired has history, the
|
||||
endpoint effectively worked only for tasks that never ran.
|
||||
|
||||
It now refuses with 409 and takes ?purge=true to mean it. The asymmetry is the
|
||||
argument: a task definition can be recreated from the API in one call, its
|
||||
execution history cannot be recreated at all.
|
||||
"""
|
||||
import pytest
|
||||
from fastapi.testclient import TestClient
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
from src.main import app, get_task_executor
|
||||
|
||||
|
||||
def _fake_executor(task_row, execution_count=0):
|
||||
"""A stand-in whose cursor answers the endpoint's two lookups in order."""
|
||||
ex = MagicMock()
|
||||
conn, cur = MagicMock(), MagicMock()
|
||||
cur.fetchone.side_effect = (
|
||||
[task_row, (execution_count,)] if task_row is not None else [None]
|
||||
)
|
||||
conn.cursor.return_value.__enter__ = MagicMock(return_value=cur)
|
||||
conn.cursor.return_value.__exit__ = MagicMock(return_value=None)
|
||||
conn.__enter__ = MagicMock(return_value=conn)
|
||||
conn.__exit__ = MagicMock(return_value=None)
|
||||
ex.get_db_connection.return_value = conn
|
||||
return ex, cur
|
||||
|
||||
|
||||
def _statements(cur):
|
||||
return [c[0][0] for c in cur.execute.call_args_list]
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def override():
|
||||
made = {}
|
||||
|
||||
def _install(task_row, execution_count=0):
|
||||
ex, cur = _fake_executor(task_row, execution_count)
|
||||
app.dependency_overrides[get_task_executor] = lambda: ex
|
||||
made['cur'] = cur
|
||||
return cur
|
||||
|
||||
yield _install
|
||||
app.dependency_overrides.pop(get_task_executor, None)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestDeleteTask:
|
||||
|
||||
def test_history_blocks_deletion_with_409(self, client: TestClient, auth_headers, override):
|
||||
cur = override((46,), execution_count=12)
|
||||
r = client.delete("/tasks/some_task", headers=auth_headers)
|
||||
|
||||
assert r.status_code == 409
|
||||
detail = r.json()["detail"]
|
||||
# The message has to carry the facts the caller needs to act: how much
|
||||
# history is at stake, the flag that proceeds, and the option they
|
||||
# probably actually wanted. A bare "conflict" would be no better than
|
||||
# the 500 it replaces.
|
||||
assert "12 execution record" in detail
|
||||
assert "purge=true" in detail
|
||||
assert "enabled=false" in detail
|
||||
|
||||
def test_a_refused_delete_deletes_nothing(self, client: TestClient, auth_headers, override):
|
||||
cur = override((46,), execution_count=12)
|
||||
client.delete("/tasks/some_task", headers=auth_headers)
|
||||
assert not any("DELETE" in s.upper() for s in _statements(cur))
|
||||
|
||||
def test_purge_removes_history_then_the_task(self, client: TestClient, auth_headers, override):
|
||||
cur = override((46,), execution_count=12)
|
||||
r = client.delete("/tasks/some_task?purge=true", headers=auth_headers)
|
||||
|
||||
assert r.status_code == 200
|
||||
assert r.json()["executions_purged"] == 12
|
||||
deletes = [s for s in _statements(cur) if "DELETE" in s.upper()]
|
||||
assert len(deletes) == 2
|
||||
# History first: the foreign key points that way, and the reverse order
|
||||
# is the failure this endpoint started with.
|
||||
assert "task_executions" in deletes[0]
|
||||
assert "scheduled_tasks" in deletes[1]
|
||||
|
||||
def test_a_task_that_never_ran_deletes_without_the_flag(
|
||||
self, client: TestClient, auth_headers, override
|
||||
):
|
||||
cur = override((46,), execution_count=0)
|
||||
r = client.delete("/tasks/fresh_task", headers=auth_headers)
|
||||
|
||||
assert r.status_code == 200
|
||||
assert r.json()["executions_purged"] == 0
|
||||
deletes = [s for s in _statements(cur) if "DELETE" in s.upper()]
|
||||
assert len(deletes) == 1, "nothing to purge, so history must not be touched"
|
||||
assert "scheduled_tasks" in deletes[0]
|
||||
|
||||
def test_unknown_task_is_404_not_409(self, client: TestClient, auth_headers, override):
|
||||
override(None)
|
||||
r = client.delete("/tasks/nope", headers=auth_headers)
|
||||
assert r.status_code == 404
|
||||
|
||||
def test_purge_on_an_unknown_task_is_still_404(
|
||||
self, client: TestClient, auth_headers, override
|
||||
):
|
||||
"""The flag must not turn a missing task into a success."""
|
||||
override(None)
|
||||
r = client.delete("/tasks/nope?purge=true", headers=auth_headers)
|
||||
assert r.status_code == 404
|
||||
@@ -305,3 +305,181 @@ class TestTaskExecutor:
|
||||
|
||||
# Should only execute max_concurrent (5) tasks
|
||||
assert mock_execute.call_count <= executor.max_concurrent
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestOrphanReconciliation:
|
||||
"""T-2. A 'running' row excludes its task from scheduling forever.
|
||||
|
||||
get_tasks_for_minute filters out any task holding one, and nothing ever
|
||||
closed those rows, so a process that died between writing the row and
|
||||
updating it left its task permanently unschedulable — silently. A row from
|
||||
2025-12-07 sat that way for eight months. Watchtower restarts this container
|
||||
nightly, so the exposure was daily.
|
||||
"""
|
||||
|
||||
def _mock_conn(self, executor, rows):
|
||||
conn, cur = MagicMock(), MagicMock()
|
||||
cur.fetchall.return_value = rows
|
||||
conn.cursor.return_value.__enter__ = MagicMock(return_value=cur)
|
||||
conn.cursor.return_value.__exit__ = MagicMock(return_value=None)
|
||||
conn.__enter__ = MagicMock(return_value=conn)
|
||||
conn.__exit__ = MagicMock(return_value=None)
|
||||
patcher = patch.object(executor, 'get_db_connection', return_value=conn)
|
||||
patcher.start()
|
||||
return cur, patcher
|
||||
|
||||
def test_running_rows_are_released(self, test_settings: Settings):
|
||||
executor = TaskExecutor(test_settings)
|
||||
cur, p = self._mock_conn(executor, [("backup_docker_configs_daily", datetime(2026, 8, 11))])
|
||||
try:
|
||||
assert executor.reconcile_orphaned_executions() == 1
|
||||
sql = cur.execute.call_args[0][0]
|
||||
assert "UPDATE task_executions" in sql
|
||||
# It must target exactly the rows get_tasks_for_minute excludes, and
|
||||
# move them to a status it does not exclude. If these two ever drift
|
||||
# apart the bug returns in silence.
|
||||
assert "WHERE status = 'running'" in sql
|
||||
assert "status = 'orphaned'" in sql
|
||||
finally:
|
||||
p.stop()
|
||||
|
||||
def test_clean_startup_reports_nothing(self, test_settings: Settings):
|
||||
executor = TaskExecutor(test_settings)
|
||||
cur, p = self._mock_conn(executor, [])
|
||||
try:
|
||||
assert executor.reconcile_orphaned_executions() == 0
|
||||
finally:
|
||||
p.stop()
|
||||
|
||||
def test_a_database_failure_does_not_stop_startup(self, test_settings: Settings):
|
||||
"""Refusing to boot because cleanup failed is worse than a stale row."""
|
||||
executor = TaskExecutor(test_settings)
|
||||
with patch.object(executor, 'get_db_connection', side_effect=Exception("db down")):
|
||||
assert executor.reconcile_orphaned_executions() == 0 # no raise
|
||||
|
||||
def test_orphaned_is_not_failed(self, test_settings: Settings):
|
||||
"""The outcome is unknown, not known-bad.
|
||||
|
||||
A backup that finished and never got to update its row looks identical to
|
||||
one that died halfway. Recording 'failed' asserts something nobody
|
||||
observed.
|
||||
"""
|
||||
executor = TaskExecutor(test_settings)
|
||||
cur, p = self._mock_conn(executor, [("t", datetime(2026, 1, 1))])
|
||||
try:
|
||||
executor.reconcile_orphaned_executions()
|
||||
sql = cur.execute.call_args[0][0]
|
||||
assert "'failed'" not in sql
|
||||
finally:
|
||||
p.stop()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_startup_reconciles_before_the_scheduler_starts(
|
||||
self, test_settings: Settings, monkeypatch
|
||||
):
|
||||
"""Order matters: reconcile must finish before the first minute is processed.
|
||||
|
||||
Run the other way round and the first tick still sees the stale rows.
|
||||
"""
|
||||
from src import main
|
||||
|
||||
order = []
|
||||
monkeypatch.setattr(main, 'get_settings', lambda: test_settings)
|
||||
monkeypatch.setattr(
|
||||
TaskExecutor, 'reconcile_orphaned_executions',
|
||||
lambda self: (order.append('reconcile'), 0)[1],
|
||||
)
|
||||
|
||||
class FakeScheduler:
|
||||
def add_job(self, **kw): order.append('add_job')
|
||||
def start(self): order.append('start')
|
||||
def shutdown(self, wait=True): order.append('shutdown')
|
||||
|
||||
monkeypatch.setattr(main, 'AsyncIOScheduler', lambda **kw: FakeScheduler())
|
||||
|
||||
async with main.lifespan(None):
|
||||
pass
|
||||
|
||||
assert 'reconcile' in order, "startup never reconciled orphaned executions"
|
||||
assert order.index('reconcile') < order.index('start')
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestTimeoutIsDistinguishable:
|
||||
"""T-3. The 'timeout' status existed in the code and had never been written.
|
||||
|
||||
_run_executor's `except Exception` sat above execute_task's
|
||||
`except asyncio.TimeoutError`, and since 3.11 asyncio.TimeoutError IS the
|
||||
builtin TimeoutError (OSError -> Exception), so the broad handler always won.
|
||||
Eight months, 18,785 executions, zero timeout rows — every one filed as a
|
||||
generic failure, erasing the difference between "too slow for its window" and
|
||||
"broken".
|
||||
"""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_timeout_propagates_instead_of_becoming_an_error_tuple(
|
||||
self, test_settings: Settings, monkeypatch
|
||||
):
|
||||
import sys, types, asyncio as aio
|
||||
|
||||
mod = types.ModuleType("src.executors.slow_probe")
|
||||
|
||||
async def execute(config, settings):
|
||||
await aio.sleep(5)
|
||||
|
||||
mod.execute = execute
|
||||
monkeypatch.setitem(sys.modules, "src.executors.slow_probe", mod)
|
||||
|
||||
executor = TaskExecutor(test_settings)
|
||||
with pytest.raises(aio.TimeoutError):
|
||||
await executor._run_executor("slow_probe", {"config": {}}, timeout=0.05)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_timed_out_task_is_recorded_as_timeout(self, test_settings: Settings):
|
||||
import asyncio as aio
|
||||
|
||||
executor = TaskExecutor(test_settings)
|
||||
task = {'id': 7, 'task_name': 'slow', 'executor': 'slow_probe',
|
||||
'service': 'scheduler', 'priority': 5, 'timeout_seconds': 1}
|
||||
|
||||
conn, cur = MagicMock(), MagicMock()
|
||||
cur.fetchone.return_value = [123]
|
||||
conn.cursor.return_value.__enter__ = MagicMock(return_value=cur)
|
||||
conn.cursor.return_value.__exit__ = MagicMock(return_value=None)
|
||||
conn.__enter__ = MagicMock(return_value=conn)
|
||||
conn.__exit__ = MagicMock(return_value=None)
|
||||
|
||||
with patch.object(executor, 'get_db_connection', return_value=conn), \
|
||||
patch.object(executor, '_run_executor',
|
||||
new=AsyncMock(side_effect=aio.TimeoutError())), \
|
||||
patch.object(executor, '_update_execution_status') as upd_exec, \
|
||||
patch.object(executor, '_update_task_outcome') as upd_task:
|
||||
await executor.execute_task(task)
|
||||
|
||||
assert upd_exec.call_args[0][1] == 'timeout', "execution row must say timeout"
|
||||
# The trap: while the timeout branch was unreachable a timeout travelled
|
||||
# the normal path, which DOES update scheduled_tasks. Making the branch
|
||||
# reachable without this call would swap a wrong status for a stale one.
|
||||
assert upd_task.called, "scheduled_tasks left stale after a timeout"
|
||||
assert upd_task.call_args[0][1] == 'timeout'
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ordinary_errors_are_still_returned_not_raised(
|
||||
self, test_settings: Settings, monkeypatch
|
||||
):
|
||||
"""The narrow clause must not swallow anything else on its way past."""
|
||||
import sys, types
|
||||
|
||||
mod = types.ModuleType("src.executors.boom_probe")
|
||||
|
||||
async def execute(config, settings):
|
||||
raise ValueError("kaboom")
|
||||
|
||||
mod.execute = execute
|
||||
monkeypatch.setitem(sys.modules, "src.executors.boom_probe", mod)
|
||||
|
||||
executor = TaskExecutor(test_settings)
|
||||
output, error = await executor._run_executor("boom_probe", {"config": {}}, timeout=5)
|
||||
assert output is None
|
||||
assert "kaboom" in error
|
||||
|
||||
Reference in New Issue
Block a user