_should_run_now has no definition in src/ in any commit in this repo's
history — checked with `git log --all -S` across the whole tree, not just
the current worktree. Twelve assertion sites across six tests called it,
so these have never passed and never protected anything.
The behaviour they describe is real: cron-wildcard matching of minute,
hour, day_of_month, month and day_of_week. It lives inside the WHERE
clause of get_tasks_for_minute, not as a Python predicate, so there was
nothing to rename them onto.
KNOWN GAP, stated rather than left implied: that matching is now covered
by no test at all. Testing it means either asserting against the SQL and
params a mocked cursor receives, or extracting the predicate out of the
query — the second changes what decides, every minute, which scheduled
work runs, and is not a refactor to do casually. Deleting was chosen over
rewriting because a test that has never run is not coverage, and leaving
it in place claimed some.
Co-Authored-By: Claude <noreply@anthropic.com>
1. `executor.max_concurrent` has never existed. Concurrency is capped by the
module-level MAX_CONCURRENT_TASKS constant via asyncio.Semaphore(
MAX_CONCURRENT_TASKS) in TaskExecutor.__init__ — confirmed with git log -p
across this file's whole history (three commits), the name has always
been the module constant, never an instance attribute.
test_executor_initialization now asserts MAX_CONCURRENT_TASKS == 5 and the
semaphore's initial count, instead of a name the class never had.
test_concurrent_task_limit asserted `mock_execute.call_count <=
executor.max_concurrent`, which — separately from the AttributeError — was
asserting the wrong observable: process_minute() awaits the full batch via
asyncio.gather before returning, so by the time the assertion runs all 10
scheduled tasks have executed; the semaphore bounds how many run
concurrently mid-flight, not the eventual call_count. Reworded to assert
all scheduled tasks still run (call_count == len(tasks)); a concurrency-
in-flight assertion would need a task that can be observed mid-execution,
which the AsyncMock stand-in does not provide.
2. _run_executor (src/tasks/executor.py) loads the executor module with the
__import__ builtin directly (`__import__(module_path, fromlist=
['execute'])`), not importlib.import_module — this repo's own CLAUDE.md
documents it as "the thing that will mislead you" about this module.
importlib is never imported there, so patch('src.tasks.executor.importlib.
import_module') failed at patch setup, before the three
test_execute_task_* bodies ran at all. Switched to patch('builtins.
__import__', side_effect=...) with a routing function that falls through
to the real import for anything other than the target module — verified
the call-recording shape empirically first (call('name', fromlist=[...])).
Source is unchanged in both cases; both are test-only defects present since
this file's initial commit.
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>
jpmschweitzerandClaude