Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
5dbc2c38a4 | ||
|
|
4e68767771 | ||
|
|
7b12ce8f0a | ||
|
|
c34db66f51 |
@@ -131,3 +131,76 @@ The timeout message also records that the work may still be running: after T-74
|
||||
|
||||
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;
|
||||
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', '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.', '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.
|
||||
|
||||
FIXED in v1.8.0 (c34db66). 409 with a message that can be acted on; ?purge=true proceeds.
|
||||
|
||||
Verified against the live service with a throwaway task that had one execution row:
|
||||
|
||||
DELETE /tasks/t1_delete_probe2 -> HTTP 409
|
||||
Task ''t1_delete_probe2'' has 1 execution record(s). Deleting it would discard
|
||||
that history. Re-send with ?purge=true to delete the task and its history
|
||||
together, or PUT enabled=false to stop it running while keeping the record.
|
||||
task still present afterwards: 1 (the refusal deleted nothing)
|
||||
|
||||
DELETE /tasks/t1_delete_probe2?purge=true -> HTTP 200
|
||||
{"message":"...deleted successfully","executions_purged":1}
|
||||
tasks: 24, probe execution rows: 0
|
||||
|
||||
REFUSE RATHER THAN CASCADE, chosen for asymmetry of recovery: a task definition can be recreated from the API in one call, its execution history cannot be recreated at all. Defaulting to the destructive reading of an ambiguous request is how audit trails disappear quietly.
|
||||
|
||||
The message carries the three things a caller needs — how much history is at stake, the flag that proceeds, and PUT enabled=false, which is usually what was actually wanted since it stops the task running and keeps the record. A bare "conflict" would be little better than the 500 it replaced.
|
||||
|
||||
History and task are deleted in ONE transaction. Split across two, a failure between them leaves the audit trail gone and the task alive: the worst of both outcomes.
|
||||
|
||||
Mutation-checked: removing the guard fails the refusal tests. Separate tests pin that a refused delete issues no DELETE at all, and that ?purge=true against a missing task is still 404 rather than a success.
|
||||
|
||||
NOTE: the commit for this work is missing its Co-Authored-By trailer — I wrote the message file without it. Already pushed, and fixing it would require rewriting published history on main, so it stands as-is.', NULL, '2026-08-11 10:31:46', '2026-08-11 10:31:46.465', '2026-08-11 10:31:46.465', NULL, 'efc9df5b52ac92d63e5ae5a60c2bdef8', 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 ('06FZ0FHPBBXXKBRWGR3FJZRK58', 'status', 'backlog', 'done', NULL, '2026-08-11 10:31:46', '2026-08-11 10:31:46.598', '2026-08-11 10:31:46.598', NULL, '7bad13e92e65eb912519a498a882faee', 2) ON CONFLICT(hash) DO NOTHING;
|
||||
|
||||
@@ -1,3 +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;
|
||||
|
||||
@@ -158,3 +158,116 @@ A trap surfaced while fixing it, and it is the interesting part. While the branc
|
||||
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;
|
||||
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.
|
||||
|
||||
FIXED in v1.8.0 (c34db66). 409 with a message that can be acted on; ?purge=true proceeds.
|
||||
|
||||
Verified against the live service with a throwaway task that had one execution row:
|
||||
|
||||
DELETE /tasks/t1_delete_probe2 -> HTTP 409
|
||||
Task ''t1_delete_probe2'' has 1 execution record(s). Deleting it would discard
|
||||
that history. Re-send with ?purge=true to delete the task and its history
|
||||
together, or PUT enabled=false to stop it running while keeping the record.
|
||||
task still present afterwards: 1 (the refusal deleted nothing)
|
||||
|
||||
DELETE /tasks/t1_delete_probe2?purge=true -> HTTP 200
|
||||
{"message":"...deleted successfully","executions_purged":1}
|
||||
tasks: 24, probe execution rows: 0
|
||||
|
||||
REFUSE RATHER THAN CASCADE, chosen for asymmetry of recovery: a task definition can be recreated from the API in one call, its execution history cannot be recreated at all. Defaulting to the destructive reading of an ambiguous request is how audit trails disappear quietly.
|
||||
|
||||
The message carries the three things a caller needs — how much history is at stake, the flag that proceeds, and PUT enabled=false, which is usually what was actually wanted since it stops the task running and keeps the record. A bare "conflict" would be little better than the 500 it replaced.
|
||||
|
||||
History and task are deleted in ONE transaction. Split across two, a failure between them leaves the audit trail gone and the task alive: the worst of both outcomes.
|
||||
|
||||
Mutation-checked: removing the guard fails the refusal tests. Separate tests pin that a refused delete issues no DELETE at all, and that ?purge=true against a missing task is still 404 rather than a success.
|
||||
|
||||
NOTE: the commit for this work is missing its Co-Authored-By trailer — I wrote the message file without it. Already pushed, and fixing it would require rewriting published history on main, so it stands as-is.', 'backlog', 'high', NULL, NULL, NULL, '2026-08-11 09:53:56.826', '2026-08-11 10:31:46.465', NULL, '9299b6bccd53cdc77e2551e2de94a10e', 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.
|
||||
|
||||
FIXED in v1.8.0 (c34db66). 409 with a message that can be acted on; ?purge=true proceeds.
|
||||
|
||||
Verified against the live service with a throwaway task that had one execution row:
|
||||
|
||||
DELETE /tasks/t1_delete_probe2 -> HTTP 409
|
||||
Task ''t1_delete_probe2'' has 1 execution record(s). Deleting it would discard
|
||||
that history. Re-send with ?purge=true to delete the task and its history
|
||||
together, or PUT enabled=false to stop it running while keeping the record.
|
||||
task still present afterwards: 1 (the refusal deleted nothing)
|
||||
|
||||
DELETE /tasks/t1_delete_probe2?purge=true -> HTTP 200
|
||||
{"message":"...deleted successfully","executions_purged":1}
|
||||
tasks: 24, probe execution rows: 0
|
||||
|
||||
REFUSE RATHER THAN CASCADE, chosen for asymmetry of recovery: a task definition can be recreated from the API in one call, its execution history cannot be recreated at all. Defaulting to the destructive reading of an ambiguous request is how audit trails disappear quietly.
|
||||
|
||||
The message carries the three things a caller needs — how much history is at stake, the flag that proceeds, and PUT enabled=false, which is usually what was actually wanted since it stops the task running and keeps the record. A bare "conflict" would be little better than the 500 it replaced.
|
||||
|
||||
History and task are deleted in ONE transaction. Split across two, a failure between them leaves the audit trail gone and the task alive: the worst of both outcomes.
|
||||
|
||||
Mutation-checked: removing the guard fails the refusal tests. Separate tests pin that a refused delete issues no DELETE at all, and that ?purge=true against a missing task is still 404 rather than a success.
|
||||
|
||||
NOTE: the commit for this work is missing its Co-Authored-By trailer — I wrote the message file without it. Already pushed, and fixing it would require rewriting published history on main, so it stands as-is.', 'done', 'high', NULL, NULL, NULL, '2026-08-11 09:53:56.826', '2026-08-11 10:31:46.597', NULL, 'e020f77c66ce6631f6d05ee63ec0c2b8', 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,23 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
## [1.9.0] - 2026-08-11
|
||||
|
||||
### Changed
|
||||
- `check_history` rows carry `summary` at the top level, beside `status`, matching the other
|
||||
writer of that table. It was nested under `metrics`, so a reader had to know which producer
|
||||
wrote a row to find its substance. Old rows keep the nested spelling.
|
||||
|
||||
## [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
|
||||
|
||||
@@ -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.7.0` (verified 2026-08-11). Human
|
||||
`http://localhost:8090/openapi.json` — 10 paths, `version: 1.9.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.7.0"
|
||||
version = "1.9.0"
|
||||
description = "System-wide maintenance orchestration - backups, doc mirroring, cleanup, task automation"
|
||||
readme = "README.md"
|
||||
requires-python = ">=3.12"
|
||||
|
||||
@@ -202,6 +202,7 @@ async def execute(config: dict, settings: Settings) -> str:
|
||||
domain="backup",
|
||||
status=health_report.CRITICAL,
|
||||
source="scheduler/config_backup_executor",
|
||||
summary=f"backup failed: {str(exc)[:300]}",
|
||||
metrics={"job": "scheduler/config_backup_executor", "error": str(exc)[:400]},
|
||||
)
|
||||
raise
|
||||
@@ -210,6 +211,7 @@ async def execute(config: dict, settings: Settings) -> str:
|
||||
domain="backup",
|
||||
status=health_report.OK,
|
||||
source="scheduler/config_backup_executor",
|
||||
metrics={"job": "scheduler/config_backup_executor", "summary": output[:400]},
|
||||
summary=output[:400],
|
||||
metrics={"job": "scheduler/config_backup_executor"},
|
||||
)
|
||||
return output
|
||||
|
||||
@@ -16,10 +16,14 @@ Recorded as D-33 in the workspace vault: `check_history` is the central health
|
||||
record and any self-maintained service may push a row describing its own
|
||||
outcome. sysmon polls only the things that cannot report themselves.
|
||||
|
||||
Three consequences that are load-bearing here:
|
||||
The properties that are load-bearing here — a count is not given, because
|
||||
this list has grown twice and a stale number is worse than none:
|
||||
|
||||
- `source` names the producer, because the table now has several writers and a
|
||||
row must say which one wrote it.
|
||||
- `summary` sits at the top level, beside `status`, because that is where the
|
||||
other writer puts it. One spelling per fact, or a reader has to know which
|
||||
producer wrote a row before it can find out what the row says.
|
||||
- `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
|
||||
@@ -59,6 +63,7 @@ def report(
|
||||
domain: str,
|
||||
status: str,
|
||||
source: str,
|
||||
summary: str,
|
||||
metrics: Optional[Dict[str, Any]] = None,
|
||||
) -> bool:
|
||||
"""Write one row to check_history. Returns whether it landed.
|
||||
@@ -77,6 +82,14 @@ def report(
|
||||
"source": source,
|
||||
"domain": domain,
|
||||
"status": status,
|
||||
# Top level, beside status — the same place sysmon-go writes it. It lived
|
||||
# under metrics until 2026-08-11, so the two writers of this shared table
|
||||
# disagreed about where the substance of a row was, and any query written
|
||||
# the obvious way found one and missed the other. That is the T-36 shape
|
||||
# exactly: per-domain queries returned nothing because the data was
|
||||
# nested somewhere else. D-33 made this table a contract between
|
||||
# producers; a contract needs one spelling.
|
||||
"summary": summary,
|
||||
"metrics": metrics,
|
||||
}
|
||||
# Which scheduled task produced this. `source` names the code; two tasks can
|
||||
@@ -144,6 +157,7 @@ async def report_async(
|
||||
domain: str,
|
||||
status: str,
|
||||
source: str,
|
||||
summary: str,
|
||||
metrics: Optional[Dict[str, Any]] = None,
|
||||
) -> bool:
|
||||
"""`report` for callers on the event loop. Prefer this one inside executors.
|
||||
@@ -155,7 +169,7 @@ async def report_async(
|
||||
`/health` from that loop, so the cost of a slow report is the whole service
|
||||
appearing down (T-74).
|
||||
"""
|
||||
return await asyncio.to_thread(report, settings, domain, status, source, metrics)
|
||||
return await asyncio.to_thread(report, settings, domain, status, source, summary, metrics)
|
||||
|
||||
|
||||
def _host() -> str:
|
||||
|
||||
@@ -158,6 +158,7 @@ async def execute(config: dict, settings: Settings) -> str:
|
||||
domain="backup",
|
||||
status=health_report.CRITICAL,
|
||||
source="scheduler/portainer_backup_executor",
|
||||
summary=f"backup failed: {str(exc)[:300]}",
|
||||
metrics={"job": "scheduler/portainer_backup_executor", "error": str(exc)[:400]},
|
||||
)
|
||||
raise
|
||||
@@ -166,6 +167,7 @@ async def execute(config: dict, settings: Settings) -> str:
|
||||
domain="backup",
|
||||
status=health_report.OK,
|
||||
source="scheduler/portainer_backup_executor",
|
||||
metrics={"job": "scheduler/portainer_backup_executor", "summary": output[:400]},
|
||||
summary=output[:400],
|
||||
metrics={"job": "scheduler/portainer_backup_executor"},
|
||||
)
|
||||
return output
|
||||
|
||||
+53
-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
|
||||
@@ -353,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(
|
||||
|
||||
@@ -7,9 +7,10 @@ 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.
|
||||
task they are, so the name travels in a ContextVar. These tests pin what makes
|
||||
that safe: it reaches the reporter, it survives the worker thread T-74
|
||||
introduced, and concurrent executions cannot read each other's. They also pin
|
||||
where the summary lives, since two producers write this table.
|
||||
"""
|
||||
import asyncio
|
||||
import json
|
||||
@@ -44,7 +45,8 @@ def captured_row(monkeypatch):
|
||||
def _report(settings, **kw):
|
||||
health_report.report(
|
||||
settings, domain="backup", status=health_report.OK,
|
||||
source="scheduler/config_backup_executor", metrics={}, **kw
|
||||
source="scheduler/config_backup_executor",
|
||||
summary="backed up 3 sources", metrics={}, **kw
|
||||
)
|
||||
|
||||
|
||||
@@ -89,7 +91,8 @@ class TestAttribution:
|
||||
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={},
|
||||
source="scheduler/portainer_backup_executor",
|
||||
summary="backed up Portainer", metrics={},
|
||||
)
|
||||
assert captured_row['result']['task'] == "backup_portainer_daily"
|
||||
|
||||
@@ -152,3 +155,35 @@ class TestAttributionIsolation:
|
||||
)
|
||||
|
||||
assert seen == {"slow_one": "slow_one", "fast_one": "fast_one"}
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestSummaryPlacement:
|
||||
"""The two writers of check_history must agree where the substance lives.
|
||||
|
||||
sysmon-go writes `summary` at the top level, beside `status`. This module
|
||||
wrote it under `metrics` until 2026-08-11, so a reader had to know which
|
||||
producer wrote a row before it could find out what the row said — and a
|
||||
query written the obvious way silently found half the data. That is the T-36
|
||||
failure exactly, where per-domain queries returned nothing because the value
|
||||
was nested somewhere else.
|
||||
"""
|
||||
|
||||
def test_summary_is_top_level(self, test_settings: Settings, captured_row):
|
||||
_report(test_settings)
|
||||
r = captured_row['result']
|
||||
assert r['summary'] == "backed up 3 sources"
|
||||
assert 'summary' not in r['metrics'], "summary must not also live under metrics"
|
||||
|
||||
def test_summary_is_required(self, test_settings: Settings, captured_row):
|
||||
"""Omitting it is an error at the call, not a silently empty column.
|
||||
|
||||
sysmon-go enforces this through Domain.Run's signature; a parameter with
|
||||
no default is the equivalent here. A row whose substance is missing looks
|
||||
exactly like a row whose check found nothing to say.
|
||||
"""
|
||||
with pytest.raises(TypeError):
|
||||
health_report.report(
|
||||
test_settings, domain="backup", status=health_report.OK,
|
||||
source="scheduler/x", metrics={},
|
||||
)
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user