jpmschweitzer/scheduler
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 5 Wiki Activity

fix(api): refuse to delete a task's history by accident, with 409 and ?purge

Browse Source 
DELETE /tasks/{name} issued a bare DELETE against scheduled_tasks. Any task
that had ever run owns rows in task_executions, so the foreign key rejected it
and the caller got:

  psycopg2.errors.ForeignKeyViolation: update or delete on table
  "scheduled_tasks" violates foreign key constraint
  "task_executions_task_id_fkey" on table "task_executions"

surfaced as a bare 500 with nothing naming history as the obstacle. It read as
the service being broken rather than the request being refusable, and since
every task that has ever fired has history, the endpoint effectively worked
only for tasks that had never run. Found while removing a temporary probe task,
which then had to be deleted with hand-written SQL across two tables.

Refusing rather than cascading, because the 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. Defaulting to the destructive
reading of an ambiguous request is how audit trails disappear quietly.

The 409 carries what the caller needs to act -- how many records are at stake,
the flag that proceeds anyway, and PUT enabled=false, which is usually what was
actually wanted: it stops the task running and keeps the record. A bare
"conflict" would be little better than the 500 it replaces.

Purge deletes history and task in one transaction. Split across two, a failure
between them leaves the audit trail gone and the task alive -- the worst of both.

Mutation-checked: removing the guard fails the refusal tests. A test also pins
that a refused delete issues no DELETE at all, and that ?purge=true on a missing
task is still 404 rather than a success.
This commit is contained in:
jpmschweitzer
2026-08-11 12:29:18 +02:00
parent 4911c94e48
commit c34db66f51
5 changed files with 216 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.pql/changelog/ticket_history/2026-08.sql
+17
View File
@@ -131,3 +131,20 @@ 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;
.pql/changelog/ticket_idmap/2026-08.sql
+1
View File
@@ -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;
.pql/changelog/tickets/2026-08.sql
+33
View File
@@ -158,3 +158,36 @@ 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;