Commit Graph
3 Commits
Author SHA1 Message Date
jpmschweitzerandClaude 538bfe5944 feat(health-report): name the task that produced each check_history row
`source` names the code that wrote a row. It cannot name the schedule that
invoked it, and two tasks may share one executor -- so a row could not answer
the question a health record mostly exists to answer: which job broke?

Concretely, 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 wrote

  source: scheduler/config_backup_executor
  status: critical
  error:  Backup file was not created

which is byte-for-byte what a nightly backup failure would have written. The
row was true and unattributable, and the reflex it invited -- delete the
inconvenient row -- was correctly refused. Attribution is the actual fix: the
record stays intact and starts saying who it is about.

Carried in a ContextVar rather than an argument. Executors are invoked as
execute(config, settings) and there are ten of them, several dormant -- existing
only as a string in a database row and becoming live the moment someone inserts
a task naming them. A signature change would leave those broken in a way nothing
imports, greps or tests would reveal. Injecting the name into `config` was the
other option and is worse: `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 the ContextVar safe, both verified in the deployed runtime
rather than reasoned about:

  - asyncio.to_thread propagates the context, so reporting still sees the task
    after T-74 moved executor bodies into worker threads. Had it not, every row
    from a real executor would have quietly lost its task while unit tests kept
    passing -- so there is a test that specifically goes through report_async.
  - Each asyncio Task gets its own copy, so the five concurrent executions
    MAX_CONCURRENT_TASKS permits cannot read each other's value. The isolation
    test yields mid-execution to force interleaving; without that it would pass
    even against a shared global.

A plain await does NOT get its own copy and leaks the value to the caller, which
the runtime check showed. Both real entry points go through create_task, but
task_scope resets via token rather than depending on that.

The field is omitted, not nulled, when there is no task: report() is callable
from a script, and a null would claim a task existed with no name.

Mutation-checked: removing the scope from the engine fails both isolation tests.

Co-Authored-By: Claude <noreply@anthropic.com>
2026-08-11 12:22:47 +02:00
jpmschweitzerandClaude e45de4fec7 fix(orchestrator): record the terminal states that were never written
Two defects with one shape: an execution reaches a terminal condition and the
orchestrator fails to write it down, so the system's own record disagrees with
what happened. Neither produced an error. Both produced silence.

T-2 -- a restart mid-task unscheduled that task forever.

get_tasks_for_minute excludes any task holding a task_executions row with
status='running'. The row is written before the executor runs and updated
after, so a process dying in between left it 'running' permanently, and the
task was then excluded from every future minute with no error, no alarm and no
log line. It did not fail; it went quiet.

test_example_task had held such a row since 2025-12-07 -- 5916 hours. It is
disabled, so nothing was broken by that instance; the mechanism is the point,
and the exposure is daily, because Watchtower restarts this container at 4 AM
while the config backup starts at 03:05 and runs ~21 minutes.

Startup now reconciles them, where the reasoning is sound by construction: this
process has just begun, so nothing it can see is genuinely running.

Marked 'orphaned', not 'failed'. When the process dies mid-task the work may
well have completed -- a backup that finished and never got to update its row
is indistinguishable from one that died halfway -- and 'failed' would assert an
outcome nobody observed. Same error as the health-report diagnostic fixed in
18be804: naming a cause you did not witness.

Not extended to a duration-based sweep. While this process lives, execute_task's
finally clause always closes the row, so a stale row implies a dead owner. A
time-based rule would have to tell a slow task from a dead one, and getting that
wrong closes the record of a task still working.

T-3 -- the 'timeout' status was unreachable.

execute_task has an `except asyncio.TimeoutError` branch that records
status='timeout'. It could never run: _run_executor wrapped the awaited call in
`except Exception`, and since 3.11 asyncio.TimeoutError IS the builtin
TimeoutError (OSError -> Exception), so the broad handler caught it first and
converted it to an ordinary error tuple. Confirmed in the deployed runtime and
against the history -- 18,785 executions since 2025-12-07, of which 'timeout'
rows: zero. Every timeout in eight months was filed as a generic failure,
erasing the distinction between "too slow for its window" and "broken".

A narrower except after a broader one is dead code, and no linter is configured
here to say so.

One trap in fixing it: while the branch was unreachable a timeout travelled the
normal path, which DOES update scheduled_tasks. Making the branch reachable
without that write would have traded a wrong status for a stale one, so
_update_task_outcome now mirrors terminal outcomes onto the parent row.

The timeout message also states 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.

Both fixes are mutation-checked: removing the startup call fails the ordering
test, removing the narrow except clause fails the propagation test. Suite goes
118 -> 126 passing with the same 36 pre-existing failures.

Co-Authored-By: Claude <noreply@anthropic.com>
2026-08-11 12:09:35 +02:00
jpmschweitzerandClaude 55fe0f9083 chore: adopt the workspace agent-config baseline
Commits a .claude/settings.json rather than leaving permissions to
per-developer local state, and initialises a pql vault for this repo's
tickets and internal decisions.

Every git deny rule appears in both the `git <verb>` and `git * <verb>`
forms. Only the second catches `git -C <path>`, and without it the whole
deny list is decorative -- it looks like a policy and stops nothing.

The allow list carries pql's absolute path alongside the bare name.
pql is installed to ~/.local/bin, which is on the login PATH but not the
one a non-interactive shell gets, so the bare-name rules match nothing on
their own and every call would prompt anyway.

.gitignore now covers .claude/settings.local.json, which is machine-local
and must never be shared. `pql init` contributed the .pql/* rules with an
exception for the changelog, which is the replication log of record and
has to be committed for tickets to travel with a clone.

Co-Authored-By: Claude <noreply@anthropic.com>
2026-08-09 03:14:59 +02:00