_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>
This asserted that _redact_sensitive recurses into a dict under a
sensitive key — redacting auth.token while leaving auth.type readable.
The source replaces the whole value the moment the KEY matches, so
redacted["config"]["auth"] is a string and indexing ["token"] into it
raises TypeError. Source and test arrived in the same commit, so this
was never drift: it was a disagreement nobody settled.
Settled in favour of the source. Fine-grained redaction has to know
which sub-keys carry a secret, which is a guess about the shape of data
nobody has inspected; matching on the key cannot be wrong that way. Real
configs here are {"auth": {"type": "bearer", "token": "${SOME_KEY}"}},
and the cost of guessing wrong is a credential in a log, which no later
fix undoes. The price is readability, and it is paid deliberately.
Adds a second assertion that the secret appears nowhere in the output by
any path, which is the property actually worth protecting.
Co-Authored-By: Claude <noreply@anthropic.com>
tests/conftest.py sets os.environ["POSTGRES_HOST"] = "postgres-shared" at
module level (before `from src.main import app`), commented "Use real
postgres for integration tests". test_settings_loads_from_environment
asserted settings.postgres_host == "test-postgres", a value grep confirms
nothing in this suite has ever set — git log -p shows both the conftest
line and this assertion originate in the same single commit and neither has
changed since. Matched the assertion to the environment the suite actually
runs under.
Not a source change and not a claim that "postgres-shared" is the right
fixture value for a suite this ticket also found is not actually hermetic
where that value is concerned (see the 12 pre-existing DNS errors, tracked
separately from this fix) — only that the assertion should test what the
fixture sets, not an unset value.
Same dependency_overrides fix as the other test_*.py commits for
get_task_executor (patch() cannot reach a route's already-registered
Depends() reference — see the conftest.py commit for the full explanation
and the empirical check that established it).
Once the mock actually intercepted the DB layer, a second, independent
defect surfaced: KeyError: 0. mock_cursor.fetchone.side_effect held 3 values
sized for "create, list, delete" (one fetchone() each), but list_tasks uses
fetchall(), not fetchone(), and delete_task has grown a second fetchone()
call since this test was written — c34db66 added the 409-on-history-loss
check, which does `row = cur.fetchone()` for the task id and then a
separate `cur.fetchone()[0]` for its execution count. A real create->list->
delete flow through this endpoint set needs 1 (create) + 2 (delete) = 3
fetchone() values in that order, not one per named step. Re-sequenced so
delete_task sees a real id and a zero execution count, reaching the intended
200 rather than an unhandled KeyError.
Source unchanged; delete_task's behavior is deliberate (c34db66) and this
integration test had not been updated to track it.
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.
test_executor_successful_sync_entire_repo: _get_git_commit is patched
separately and never calls the real _run_command, so it does not consume a
slot in mock_run.side_effect. The list reserved one anyway (labelled "git
rev-parse HEAD"), which shifted "M README.md\n" one call late — git status
--porcelain saw "" (no changes) instead, so execute() took the no-changes
branch and the test asserted "Successfully synced" against "already up to
date". Removed the phantom slot. Also gave the iterdir() mock items real
string .name attributes: MagicMock(name="X") sets the mock's repr, not the
.name attribute read by execute()'s `item.name != '.git'` check, so every
item was being treated as non-.git regardless of the intended value.
test_executor_sync_specific_paths, test_executor_handles_no_changes:
mock_upstream_dir/mock_gitea_dir were built but never wired to
mock_path.return_value, so `work_dir = Path(...)` and `upstream_dir =
work_dir / "upstream"` resolved to a different, unconfigured auto-generated
MagicMock. `source.name` on that mock is itself a MagicMock, not a string,
so `', '.join(copied_paths)` raised TypeError. Wired mock_path.return_value
to a work_dir mock whose __truediv__ yields the intended upstream/gitea
mocks, matching the pattern the first test already used correctly.
All three are test-side: doc_sync_executor.py is unchanged. Confirmed via
git log -p that this file and its test have exactly one commit in this
repo's history (the initial extraction), so there is no prior passing
version to regress from — these tests appear to have never passed.
1. get_task_executor mocking: same dependency_overrides fix as the previous
commit, applied to this file's remaining patch('src.main.get_task_executor')
call sites (test_valid_api_key_allows_access, test_create_task_with_valid_data,
test_trigger_task_endpoint, test_stats_endpoint_returns_metrics,
test_executions_endpoint_returns_history, test_executions_filter_by_task_name).
2. test_create_task_missing_fields_returns_400 asserted a status this endpoint
cannot return. `task: TaskCreate` in src/main.py is a plain Pydantic request
body with no custom validation for missing fields — FastAPI's own
dependency-resolution layer rejects the request before create_task's body
runs, and that layer always answers 422, not 400. There is no code path in
this repo, at any point in its git history, that produces 400 for this
request. Renamed to test_create_task_missing_fields_returns_422 and the
assertion updated to match; source unchanged.
Kept together in one commit because both live in the same small file and were
found in the same pass, rather than risk a manual hunk split on tightly
interleaved diff context.
FastAPI resolves Depends(get_task_executor) against the function object it
captured when each route was decorated, at import time. patch('src.main.
get_task_executor') therefore never reaches an already-registered route —
confirmed empirically with a minimal FastAPI app before touching this repo's
tests. Every one of these tests fell through to the real dependency, which
called the real psycopg2.connect() and failed on postgres-shared's DNS (a
Docker-network name unreachable from a host process either way, so this
reproduced identically with and without host networking).
app.dependency_overrides is FastAPI's own supported mechanism for this, and
was already used correctly elsewhere in this suite (test_task_delete.py).
Added a shared override_task_executor fixture in conftest.py and switched
every get_task_executor patch() call site in test_api_comprehensive.py to it.
Source (src/main.py) is unchanged — Depends() is the idiomatic, correct
pattern; the tests were using an ineffective substitute for it.
check_history has two producers. sysmon-go writes `summary` at the top level
beside `status`; this module wrote it under `metrics`. 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 found one and silently missed the other.
That is the T-36 failure repeating. There, per-domain queries returned rows from
August and looked like a system that had stopped reporting, because the data was
nested under a composite row nobody had mentioned. Nothing was missing; the
query was asking the wrong shape. verify.sh had already grown a coalesce over
both spellings, which is the tell: a compatibility shim that hides a schema
disagreement rather than resolving it.
D-33 made this table a contract between producers, and a contract needs one
spelling.
Summary is now a required parameter with no default. sysmon-go enforces the same
thing through Domain.Run's signature, and the reason is identical: a row whose
substance is missing looks exactly like a row whose check found nothing to say.
Both call sites pass it; the failure path passes the exception rather than
leaving the field to the metrics blob.
Old rows keep the nested spelling and verify.sh keeps reading both, because
rewriting history to match a new convention is a worse trade than a fallback
with a reason attached.
Also drops "Three consequences" from the module docstring, which by then listed
five. A hardcoded count beside the thing it counts is the same defect as
install.sh printing "wrote 8 keys" while writing ten — this morning's bug, in
prose instead of code.
Co-Authored-By: Claude <noreply@anthropic.com>
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.
`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>
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>
The scheduler serves its own REST API from the same loop that runs executors,
and the config backup spends ~21 minutes inside tarfile and zlib. Called inline
that starves the loop for the whole window: the service was unreachable
03:05-03:25 every night, and again at 07:39 today when the job was triggered by
hand to prove the T-69 report path. asyncio.to_thread is the fix -- zlib
releases the GIL while compressing, so the loop is scheduled normally.
The outage was invisible for as long as it existed. The hourly health check
fires at :35 and the outage runs 03:05-03:25, so no sample ever landed inside
it. A fixed-phase hourly probe cannot see a 20-minute event; that is aliasing,
not bad luck, and it would have stayed hidden indefinitely.
health_report.report_async joins it: psycopg2 is a blocking driver, so
reporting from the loop held it for the connect and insert -- up to
connect_timeout seconds precisely when the database is unreachable, which is
when a report matters most. Both backup executors use it now.
Caveat worth knowing: the engine wraps executors in asyncio.wait_for and a
thread cannot be cancelled, so on timeout the task is recorded failed while the
tar runs to completion. Still strictly better than blocking everything, and the
configured 3600s is well clear of the observed 1263s.
Seven tests in this file had been red since the initial commit -- they came
over with the portainer-core extraction, patched the Path class wholesale,
asserted "backed up" against a function returning "Backup completed: ...", and
one wrapped its call in except Exception: pass with its only assertion
commented out. There is no CI test gate here, so nothing reported it. Replaced
with tests that build real archives in tmp_path and assert on their contents.
The new loop test is the one that matters and it is mutation-checked: with
to_thread reverted it counts 0 heartbeat ticks, with it ~40.
Their structural demands (_create_tar_filter, a sync _cleanup_old_backups) were
adopted because threading wanted that shape anyway. Their exclude semantics
were not. The tests assert fnmatch behaviour and the deployed config is written
against substring matching -- it excludes logs as ".log", and paths as
unanchored fragments like "ollama/models/*" against members named
"docker-data/ollama/...". Under fnmatch neither matches, and the nightly
archive would silently gain many GB of model blobs instead of shrinking. That
is now pinned by tests naming the consequence, so the "improvement" fails loudly.
Co-Authored-By: Claude <noreply@anthropic.com>
Portainer keeps every stack definition, endpoint, user and access-control
rule in a BoltDB inside the portainer_data Docker volume. That volume
sits under /var/lib/docker/volumes/, and the daily config backup covers
~/docker-data and code-server-config only — so the thing that defines all
24 stacks was the one thing not backed up.
Calls Portainer's /api/backup rather than tarring the volume. BoltDB is a
single memory-mapped file, so copying it while Portainer writes can
capture a torn page; the API serialises a consistent snapshot.
A 200 whose body is not a readable archive is treated as failure. An
archive that will not open is worse than a missing one, because it looks
like a backup until the day it is needed. Writing that check found a real
gap in it: a truncated tar.gz raises EOFError, which is neither TarError
nor OSError, so the first version of the guard let it through.
Archives contain TLS certificates and private keys and are written 0600.
Retention only ever deletes files matching the exact name this executor
writes, so an unrelated archive left in the same directory survives.
Co-Authored-By: Claude <noreply@anthropic.com>
Non-interactive equivalent of system-admin-toj's prune-docker.sh, which
prompts per stage and so cannot run from cron.
Only the stages that discard regenerable data run by default: build
cache and dangling images. Unused images and volumes are opt-in, because
docker volume prune removes volumes belonging to merely-stopped
containers rather than only orphaned ones, which on this host is a
plausible way to lose a database.
A failing stage is reported and the remaining stages still run, since a
partial reclaim beats none, but the task still ends up failed so the
error is not swallowed.
Co-Authored-By: Claude <noreply@anthropic.com>
Deletes rows past a retention window from a table on the shared Postgres
server. Written for sysmon's check_history, which grows with every
monitoring check and had no retention at all despite the docs promising
a 30-day rolling window.
Connects with the Scheduler's own credentials and overrides only the
database name, so no second set of secrets enters the stack. The target
database grants scheduler_user just SELECT and DELETE on the table, so a
bug here can drop old rows but cannot corrupt or forge history.
Table and column names cannot be bound as query parameters, so both are
validated against a strict identifier pattern before interpolation, and
a retention window below 1 day is refused rather than silently emptying
the table.
Co-Authored-By: Claude <noreply@anthropic.com>
jpmschweitzerandClaude