Final-stack review (hermes-pr-review 2c) on #133283:
- The tick lock reported its outage under the unresolved profile home (_get_lock_paths), while
every save, probe and grace check uses the resolved store path. With a symlinked HERMES_HOME
(or macOS /var -> /private/var) the lock outage never cleared: writable stuck at 0, no
"recovered" notice, and a skipped one-shot retired as missed. All store_health records are now
keyed through one _key() (realpath). Pinned with a symlinked home; red without it.
- unwritable -> recovered (before delivery) -> unwritable posted nothing: the dropped first
notice left the "recovered" timer pending, so the re-entry cancelled it as if an outage notice
had gone out. A dropped "unwritable" now cancels that timer, so the re-entry announces. The
notice test runs on a manual loop clock (no wall-clock timing; it had flaked once under load)
and pins the sequence.
- cron status printed "0 due run(s) not fired" and an empty summary when the jobs could not be
read from an unwritable store; it now says the jobs could not be read.
/simplify-code quality pass on the final #133283 stack, no behaviour change:
- One null-safe is_recurring in cron/constants.py replaces scheduler_tick's new copy and
unreachable_retry's (whose .get("schedule", {}) raised on a null schedule).
- cron status / cron list share _probe_then_list_jobs (probe, print, guarded read) instead of
two near-identical blocks.
- StoreDegraded.notice_fields and probe_report build their fields through one _report_fields.
- The notice listener's pre-checks no longer repeat deliver()'s store re-check; deliver() is the
single staleness gate (still red when removed).
Greptile r2 on #133283:
- #1: probe_store built one byte string the size of jobs.json; a large store made cron status,
doctor and the idle recovery check allocate that much. It now writes the same total in 64 KiB
chunks.
- #2: the host-wide store gauges read every degraded record, so a profile that left this
gateway while its store was down kept it at writable=0 until a save that would never come.
register_ticked_homes already discards a departed home's worker pool; it now also drops that
home's store state (store_health.forget_homes). The export test pins it.
The read-only tick-lock fallback (85f06c4777) pushed _tick_admitted to CC 22 against the
code-health target of 20 (was 17 on main). Move the lock-or-degrade step and the
advance-or-drop-recurring step into _acquire_tick_lock_or_degrade and
_advance_or_drop_recurring, with a small _is_recurring predicate. Behaviour is unchanged:
the cron, monitoring and cron CLI suites pass as-is, and the read-only-lock test still
fails with the errno filter removed.
Greptile r1 on #133283:
- #3: probe_store created an empty file in cron/, while a save stages a full payload beside a
symlinked jobs.json's real target. A read-only or full target passed the probe, so status and
doctor reported a writable store, and a fixed 1 MiB free-space cutoff could flag a store
whose small save still fits. The probe now stages a jobs.json-sized payload with
mkstemp_beside (the save's own staging helper), fsyncs it, and checks the resolved target is
writable; the free-space cutoff is gone.
- #5: on a read-only cron/ without cron/output, list_jobs tries to create output and raised
before `cron status` / `cron list` printed the warning and fix. Both print the store warning
first, and a read failure on an unwritable store no longer hides it.
Greptile r1 on #133283:
- #1: on a read-only (or full / permission-denied) mount the tick lock cannot be opened for
writing, so _acquire_tick_lock raised before the degraded-store path ever ran and the tick
still failed. An unwritable-class errno (EROFS/EACCES/EPERM/ENOSPC/EDQUOT) from the lock now
degrades the store (site "lock") and returns 0 with the MCP orphan sweep; EMFILE/ENFILE and
other errors still fail the tick (#87644). Live: a real read-only HFS mount raised errno 30
on the previous head and returns 0 here.
- #2: end_recovery_window ran before the scan's save, so a store that failed again before that
save landed lost the first outage's window and a later scan retired the skipped one-shot as
missed. The window now closes only after the scan's save lands, and a re-entry before then
keeps the first outage's start.
Follow-up review of the three fixes above:
- recheck_idle cleared the outage on a passing probe alone. The probe only creates an empty
file, so EDQUOT or a read-only jobs.json in a writable dir passed it while every real save
failed, flapping the state (and its WARN pair and metric) every idle minute. A passing probe
now does a real jobs.json save; its note_writable ends the outage, a failure keeps it.
- first_report still double-counted a miss at outage start (counted before the failing save
created the record) and after recovery by another save. Replace it: the scan queues its
miss/catch-up metrics and emits them only after its save lands, which counts each persisted
fast-forward exactly once with no per-outage bookkeeping.
- The ended-outage window lived for the process lifetime, so a one-shot due during the outage
but first scanned much later still fired late. The first due scan after recovery now closes it.
Review cleanup, no behaviour change:
- warn_store_unwritable only resolved the active store before calling
store_health.note_unwritable; note_unwritable now defaults cron_dir to the active store and
every caller uses it directly (scheduler_tick no longer imports both).
- The advance-skip split no longer zips a parallel bool list twice, and reads
(schedule or {}) so a null schedule cannot raise inside the OSError handler.
- skipped_runs metric: a walrus instead of getattr around an Optional record.
- Notices: runner._served_profile_homes is declared on the reconcile mixin, so no getattr.
- cron status loads the active jobs once (skipped count + summary) instead of twice.
While the store is unwritable the due scan's fast-forward cannot persist, so every tick finds
the same miss again and called record_catch_up_occurrence (or record_cron_missed when
catch_up_missed is false) once per tick per job for the whole outage, inflating the
catch-up/missed metrics.
store_health.first_report counts each (job, due instant) once per outage; with a writable
store it always counts, as before.
The degraded record cleared only when a jobs.json save landed in the gateway. With nothing due
(the user freed space and then paused or removed the due jobs, or CLI saves from another
process), no save lands, so writable stayed 0, the "recovered" notice never went out and the
one-shot grace gate stayed open indefinitely.
An idle tick now re-probes a degraded store at most once a minute (the same throttle as the
dispatch gate) and clears it when the probe passes.
The one-shot grace gate only looked at the live degraded record, and the first jobs.json save
that lands clears it. On a real recovery that save is usually another job's heartbeat or a
create, landing before the due scan, so every one-shot the outage skipped was retired as
"missed" anyway (review probe: degraded 15 min, one-shot due 10 min ago, an unrelated
create_job, then the scan retired it).
note_writable now keeps the ended outage per store (recovered_at), and the gate fires a
one-shot due inside [since - grace, recovered_at]. A one-shot due after recovery and then
missed is still retired, as on main.
Gate finding C12 r3 #9 (quality): degraded_since only wrapped
degraded_record(...).since, and jobs.py late-imported store_health in three
places for no cycle (store_health imports cron.jobs only lazily, inside
_active_cron_dir). Remove degraded_since, call degraded_record at the
one-shot gate, and import store_health once at module level next to
_claim_owner_is_dead. A fresh interpreter imports cron.jobs,
cron.scheduler, cron.store_health (either order) and
agent.monitoring.cron_health cleanly. dispatch_blocked loses the walrus and
the split locking: probe outside the lock, then update last_probe, error
and skipped under it in a single block. Behaviour is unchanged (the existing
cron tests pass as-is).
Gate finding C12 r3 #7 (efficiency): once dispatch had been tried, each
degraded tick could make two write attempts — the due scan's failing save,
then dispatch_blocked's minute-throttled probe_store — even though the scan
had just shown the store refusing writes. note_unwritable now refreshes
last_probe on any failing site once the record has been probed, so the
re-probe only runs on ticks where nothing else tried to write. A record
entered by a scan/load failure still has last_probe None, so its first
dispatch attempt and the one-shot "failed" row are unchanged. The degrade
test now asserts zero extra probes across the 5 outage ticks (2 before).
Gate finding C12 r3 #6 (2c): disk_usage().free leaves out the blocks the
filesystem reserves for root. A gateway running as root (common in
containers) can still write into them, yet the probe reported ENOSPC and
kept the store degraded while every real save succeeded. When geteuid() is
0, free_bytes now reads statvfs f_bfree * f_frsize. Platforms without
geteuid (Windows) keep disk_usage().free. The doctor low-space row uses the
same helper and so agrees with the probe.
Probe-verified (no third test function): with a faked root, statvfs and
disk_usage, HEAD returns 0 and this commit returns f_bfree * f_frsize.
A one-shot that came due while its store was unwritable could not settle (the claim write
failed), so it stayed due. The first tick after the store recovered found it past
ONESHOT_GRACE_SECONDS (120 s) and retired it as "missed": the outage silently ate the run.
The one-shot grace gate now treats a run that was due no earlier than grace before the store
entered its degraded state as skipped, not missed, and lets it fire once. Recurring jobs
already behaved: the unpersisted fast-forward re-runs each tick and the first writable tick
fires the job once under the existing catch-up policy.
The new lifecycle test drives 5 ENOSPC ticks plus recovery through a real tick(): one
"unwritable" transition, 2 probes for 5 ticks, only the scan's save attempted after entry,
then the recurring job and the one-shot each fire once and the one-shot keeps one failed row.
warn_store_unwritable logged one WARNING per (store, errno, site) every 15 minutes, and every
60s tick still ran the advance and the one-shot claims that could only fail again. Nothing could
tell the user "this store is degraded since X, N runs skipped".
cron/store_health.py (new; jobs.py is at its size cap) keeps one in-memory record per store dir:
since, error, the sites that failed, the distinct (job, due instant) runs skipped, last probe.
warn_store_unwritable delegates to it and WARNs once on entry with the fix (free disk space /
remount read-write / fix permissions on <path>). _save_jobs_unlocked, the single jobs.json write
chokepoint, clears it on the first save that lands and logs the recovery with the skip count.
While a store is known unwritable, tick() skips advance/claim for the due jobs (recording them as
skipped) and re-probes at most once per 60 s with a temp-file create+unlink plus a free-space
floor. The heartbeat and the MCP orphan sweep keep running. The first dispatch after entry still
goes through, so a due one-shot settles its single failed row, as before.
The OSError branch of _process_due_job's fire claim is the full-disk path,
yet it closed the receipt with a bare finish_execution while the sibling
branch already used settle_unstarted_execution. On a full disk the
executions.db write fails too, so the skip raised sqlite3.OperationalError
out of the worker instead of returning False. Use the best-effort settle
there as well (fold finding 1); a parametrize row covers the ledger failing.
The except branch covers both create_execution and the steps after it
(note_cron_execution, context copy). When the receipt WAS created the log
still said "execution creation failed", pointing operators at the ledger
while the row had been written and settled. Name the step from whether
`execution` was set (fold finding 3, quality review).
_submit_with_guard persists a `claimed` receipt before the worker runs, and
run_one_job only owns the terminal writes once it starts. Two exits in
between left the row `claimed` forever (a permanently in-flight attempt
for a run that never happened): a fire claim raising anything other than
OSError in _process_due_job, and a failure in _submit_with_guard after
create_execution returned (note_cron_execution, context copy). Both now
close the receipt through a best-effort settle_unstarted_execution in
cron/executions.py (the ledger owner; scheduler.py is a capped facade); the
claim failure still re-raises so the real error surfaces.
Co-authored-by: PRELVAS94 <86016656+PRELVAS94@users.noreply.github.com>
The late import in _claim_is_live was only there because the helper moved
to scheduler_ownership; that module's top-level imports never reach
cron.jobs, so there is no cycle. A function-level cron.* import inside
jobs.py is a first-time import on the completion tail, which #133407's
generation-pinning guard (test_completion_tail_imports[jobs.py]) rejects
once both land.
load_jobs() drops non-object entries (and flattens id-keyed maps, normalizes
repeat.completed) in memory, then saves the repair. That save was the one
store write left unguarded: on a full disk / read-only mount a single junk
entry in jobs.json raised ENOSPC out of every load_jobs(), so every tick died
before reaching the guarded advance/claim paths. The repaired list is already
what callers use; log through the rate-limited warn_store_unwritable(site=
'load') and let the next load retry the persist.
The due scan's save runs first on every tick and fails whenever a recurring job is due, so a shared (store, errno) key let its 'repairs not persisted' warning silence the tick's 'skipped N recurring job(s)' for the whole outage. Key on (store, errno, site) with site in scan/advance/claim: at most three WARNINGs per store per window, and the skip is always logged. The tick test now asserts the skip warning and that the recurring job never reached its fire claim (both red with the shared key / a keep-all filter). Gate r2 Medium (all five reviewers).
Gate r1 quality (Low): the size-gate move put get/record_catch_up_occurrence
into occurrences.py, whose docstring only described scheduled identities.
Widen it to name the counter (the larger cron/markers.py consolidation is
out of scope for this fix).
Gate r1 quality + reuse (Low): drop the third blank line before tick()
(E303); rewrite the test module docstring, which still claimed a writable
store is pinned and that the dispatch is never lost, to describe what the
two kept tests assert; lift one module-level _enospc stub shared by the
full_disk fixture and the tick test instead of two copies. No behaviour
change.
Gate r1 efficiency + quality (Low): when advance_next_runs fails and every
due job is recurring, the tick returned 0 without _sweep_mcp_orphans(),
unlike the idle and dispatch paths, so stdio children of earlier crashed
jobs were never reaped for the whole store outage. The partition also ran
the same predicate twice and filtered by O(n*m) dict-equality membership.
Partition in one pass (skipped count = n - len(filtered)) and sweep before
the early return, like the idle branch. The tick test is parametrized over
a recurring-only store (the skip-all branch) and asserts one sweep: red
without the sweep, green here.
Gate r1 2c (Low): _last_store_warning was keyed by errno alone and process
global. In a multiplexed gateway every profile ticks in one process, so
profile A's ENOSPC warning silenced profile B's for 15 minutes, and the text
gave no store path, so an operator could not tell which profile was stuck.
Key the limiter by (cron_dir, errno) via _current_cron_store() (same module,
no lazy import) and put the store path in the warning. The scan test now
also warns from a second profile's store: red on the errno-only key (1
warning), green here (2, each naming its store).
Gate r1 quality (Medium): the due-scan save guard in _get_due_jobs_locked
called logger.warning directly, so while the store stayed unwritable it
logged every 60s tick (the repairs are never persisted, so needs_save stays
true) -- the spam _STORE_WARN_INTERVAL_SECONDS exists to stop. Its wording
("due jobs still dispatched") also contradicted the tick-level warning that
recurring dispatch was skipped in the same tick.
Move warn_store_unwritable and its state into cron/jobs.py next to
_write_marker (the lower layer; jobs.py never imports scheduler_tick), route
the due-scan guard through it with neutral wording ("due-scan repairs not
persisted"), and have scheduler_tick.py and scheduler.py import it from
cron.jobs (scheduler.py via its existing top-level block, not inside the
except). The helper now takes the whole consequence phrase so each caller
states what it did.
The scan test now runs two scans and asserts exactly one neutral WARNING:
red on the previous code (2 warnings), green here.
Code-health ratchet: cron/jobs.py is capped at 3495 lines (its value on
main). The next commit moves the store-unwritable warning helper down into
jobs.py (gate r1 quality finding), which would push it over. Offset that by
a pure move of the self-contained claim-owner liveness probe into the
existing ownership sibling; body is byte-identical, its only caller
(_claim_is_live) late-imports it, and nothing in tests/ patches it.
Code-health ratchet: the store-unwritable fix grew cron/jobs.py (3505 > 3495) and
cron/scheduler.py (4464 > 4457), both already over the 2000-line target. Pure moves, bodies
unchanged: _log_tick_yield_once (+ its rate-limit state; sole caller is the tick) into
scheduler_tick.py, and the catch-up occurrence counter pair into occurrences.py. Importers
(cron_health metrics, the stale-edit test) updated; no re-exports left behind.
The salvaged due-scan guard only moved the crash: on an unwritable store (ENOSPC/EROFS/EACCES)
advance_next_runs raised out of tick() for recurring jobs, and claim_job_for_fire raised in the
worker for one-shots. Running without either write would break at-most-once (a crash mid-run
re-fires), so both sites now skip that dispatch instead: recurring jobs are dropped from this tick,
an unclaimable job's execution is closed as failed. The jobs stay due and fire once writes work.
A rate-limited WARNING (once per errno per 15 minutes) lives in scheduler_tick.py, the tick's
topical sibling, rather than in the 3.5k-line jobs.py.
_get_due_jobs_locked repairs store-side problems mid-scan (half-paused records
self-disable, completed one-shots are swept, records are normalized) and then
persists them with a bare save_jobs(...) before returning the due list. Any
store-write failure there — ENOSPC on a full disk, a read-only mount, a
permissions problem — raised out of get_due_jobs(), so cron_tick() aborted and
NO job on that profile ran until a write succeeded. Live on a full /opt/data:
"Cron tick error for profile at /opt/data: [Errno 28] No space left on device"
from that exact call site, repeating every tick, with every job then reporting
"missed its scheduled time" and re-anchoring.
The repairs are already applied in memory — that is what the scan keys off — so
the persist is a side effect the next tick retries. Contain the store write and
keep dispatching; a non-store error still propagates.
(cherry picked from commit 76676fb0084f06890f76280f50387325ad2d59c7)
An explicitly-set empty allowlist meant 'nothing allowed' everywhere in
prose, but every boundary normalized it away:
- tui_gateway _load_enabled_toolsets converted an empty resolved set to
None (= no restriction), so platform_toolsets.cli: [] handed the
session every default toolset (#82010).
- cron create/update normalized enabled_toolsets=[] to null, and the
dispatch resolver treated a stored [] as absent (falsy), falling back
to platform_toolsets.cron.
Distinguish unset (None = no restriction, unchanged) from explicit
empty ([] = zero toolsets) end to end: the gateway returns [] bare (no
client-surface fold-in), cron persists and reloads [] verbatim, and
dispatch resolves [] to zero toolsets with no MCP merge. Absent keys
keep today's defaults.
Plugins can now call ctx.register_automation_blueprint(key, *, title,
description, schedule_template, prompt_template, category, slots,
deliver_default, skills, tags). The fields mirror the built-in
AutomationBlueprint, and slots are BlueprintSlot field dicts, so the call
also crosses the plugin-host boundary.
- One lookup, cron.blueprint_catalog.list_blueprints() / get_blueprint(),
merges the built-ins with the active profile's plugin blueprints at call
time. /blueprint (CLI/TUI/messengers), /api/cron/blueprints and the
instantiate route all use it. Each dashboard route resolves inside the
?profile= scope, off the event loop.
- Keys are namespaced <plugin>:<key>, so a plugin cannot shadow a built-in.
A ':' in the key, a duplicate key, an unknown placeholder, a bad slot type
or a schedule that does not parse is warned and ignored at registration.
- Catalog entries carry source/plugin. Desktop fetches the catalog for the
profile it creates jobs in, keyed per profile, and shows the plugin name
on the blueprint row and in the "Start from" select. The dashboard shows
a plugin badge.
- A created job is an ordinary cron job with the prompt and schedule already
filled in, so it keeps running if the plugin is later disabled.
- The ctx content registrars (register_skill, now joined by
register_automation_blueprint) move to hermes_cli/plugins_content.py.
The dashboard's blueprint types move to web/src/lib/automation-blueprints.ts.
Closes#132373. Requested by @enckequity.
Plugins could only see a cron run as the task_id string `cron:<job>:<execution>`,
with no way to tell a scheduled fire from a manual `hermes cron run`. A
security plugin that may only act inside a real scheduled occurrence had
nothing to check.
The scheduler now binds a frozen CronExecution (job id/name, execution id,
source, scheduled_instant, started_at, profile) in a ContextVar once the run
wins its claimed->running CAS (or a restart-safe worker adopted the row), and
resets it when the fire ends. ContextVar scoping keeps concurrent jobs and
profiles apart and follows the agent onto the threads the scheduler already
copies context into. Delegated children read None.
Also trims duplicated comments in _run_one_job_body to keep the facade within
its code-health line cap.
Closes#130722
The Home Assistant gateway platform (plugins/platforms/homeassistant) and the
homeassistant toolset (tools/homeassistant_tool.py) now ship as the official
catalog plugin `homeassistant` (NousResearch/hermes-homeassistant), with the
same platform name, toolset/tool names, config keys and env vars.
Migration (hermes_cli/left_core_migration.py, a generic "left core" table
reusing the memory-provider migration's helpers): every profile home that
used Home Assistant (HASS_TOKEN in its .env, platforms.homeassistant enabled
or holding a token, or the homeassistant toolset selected) gets the plugin
installed from the catalog at its reviewed pin by `hermes update`, and lazily
once per process at agent start / gateway start (honours
security.allow_lazy_installs). Every outcome reaches terminal, Desktop and
chat; a gateway-start outcome is handed to that home's first agent.
Core HA special cases become generic PlatformEntry seams the plugin uses:
trusted_inbound (allowlist bypass for event-bus platforms), display_tier
(built-in display defaults) and shared_env_prefixes (profile-clone ownership
rule for env shared with the plugin's tools). Cron delivery uses the existing
cron_deliver_env_var seam; HASS_TOKEN stays stripped from subprocesses via the
installed plugin's manifest secrets.
`hermes -s dup-demo` (CLI, oneshot, TUI/kanban worker) died with "Unknown
skill(s): dup-demo" and cron logged "skill not found, skipping" for a name that
exists twice. skill_view's refusal now carries load_names; preload labels the
failed identifier "Ambiguous skill name X: use one of <paths>" (the label is
computed only on the failure path) and format_missing_skills() keeps that
wording in the raised error; cron skips with the same message and lists it in
the job's skipped-skills notice.
CRONSCOPE r2 quality Lows: the dispatch-failure notice in run_one_job and
the env build in _launch_external_cron_worker each hand-rolled the same
install / try / finally / reset around _install_fire_secret_scope. Add
_fire_secret_scope() next to the helper pair and use it at both sites;
reset order is unchanged (it still delegates to _reset_fire_secret_scope).
Also collapse the five-line comment at the dispatch-failure site to one.
_run_one_job_body keeps its tokens (the reset sits in a larger finally) and
scheduler_worker_failure.py is untouched (mark_job_run shares its finally).
_run_one_job_body's except branch sends _deliver_crash_failure when anything in
the body raises, but the firing profile's secret scope was only installed after
claim_dispatch and mark_execution_running. A raise from either sent the crash
notice with no scope: under multiplex get_secret fails closed
(UnscopedSecretError, "Hermes could not read this profile's
TELEGRAM_HOME_CHANNEL") and the notice never left. Same class as the
dispatch-failure branch fixed for #131585 (and called out in #130431's body).
Install the scope first thing in the try so every exit through the except is
scoped; the outer finally already resets it. The handoff path stays outside it
(#107692), and the external-worker payload path and the post-handoff notice
already run scoped.
Extends the #130431 test with a claim_dispatch-raising case.
Refs #131585
Co-authored-by: skyeyesec333 <skyeyesec333@users.noreply.github.com>
run_one_job's pre-handoff dispatch-failure branch (#124013) calls
_deliver_crash_failure before _run_one_job_body, which is where the
firing profile's secret scope is installed for delivery. Under a
multiplexed gateway the notice then resolves the home channel through
get_secret with no scope, raises UnscopedSecretError, and never leaves.
Wrap the notice in _install_fire_secret_scope/_reset_fire_secret_scope,
the pair _run_one_job_body and record_unknown_worker_outcome already use.
(cherry picked from commit 5e0735b17267a636337818505bae485d7e208837)
The external worker now calls GatewayStartupMixin._register_config_hooks (the
helper gateway startup and each multiplexed profile already use) instead of a
copied load_config/register_from_config sequence, and calls it after
set_secret_scope rather than before. Registered outside the scope, a
hooks.outbound target with secret_env hits get_secret with no scope while
multiplexing, which raises UnscopedSecretError; the never-raise guard swallowed
it, so that profile's outbound webhooks were silently dropped again.
Both registries are idempotent per (home, event, ...) so a worker running
several payloads registers each hook once. The kept test gains a secret_env
target on a multiplexed payload to pin the ordering; the broken-config test is
dropped (register_from_config never raises on a malformed block, and the
helper's guard is already covered by the gateway path).
Refs #131764
The restart-safe external worker (python -m cron.scheduler
--external-worker-file) called discover_plugins() under the payload's
home override but never agent.shell_hooks.register_from_config() nor
agent.outbound_webhooks.register_from_config(), so hooks: blocks in the
owning profile's config.yaml silently stopped firing for cron sessions
once gateway fires were handed to the worker instead of running
in-process. Plugin hooks kept working, which made the gap invisible.
Register both right after plugin discovery + secret hydration, under the
same home override, with the gateway's never-raise shape (#102504):
consent resolves non-interactively from the owning profile's allowlist /
HERMES_ACCEPT_HOOKS / hooks_auto_accept, and a broken hooks block logs a
warning instead of taking the cron execution down.
Fixes#131764
(cherry picked from commit d1971c698a4429868e9d43d058032930ec62d7b9)
Register direct workers with a live future and fence dispatcher release and future attachment with per-registration identity tokens. Preserve profile-scoped release keys.
Port the in-memory fix from 251b99332550be4dc5a7ba265109e367842e9081, without timestamp-based durable-state guards.
Fixes#132223. Refs #88688.
(cherry picked from commit dd834c874fef04a7d56658e0d05baa3bee5decba)
All three ownership-lost call sites in _classify_delivery_outcome computed
ladder_rung=is_retry_run(job) themselves and passed job["id"]; the helper is
the only place that needs the flag, so pass the job and derive it inside.
That removes the duplicated expression (and a mis-indented continuation
line at the third call site) and drops the now-unused function-level
is_retry_run import in _classify_delivery_outcome. The conditional
ladder_rung splat is kept so mark_job_run fakes without the kwarg still
see the same call shape as before. No behaviour change.
_record_fire_ownership_lost is the last terminal write on the claimed
snapshot that still counted a ladder re-run toward repeat. When a re-run of
an unreachable-model occurrence is cancelled at the transport (dashboard
drain) while we still own the claim, it records the interruption through
mark_job_run, which bumped repeat.completed a second time for an occurrence
that had already counted -- the same bug the previous commits fixed for the
finish, crash, dispatch-failure and dead-worker tails.
The helper now takes ladder_rung and forwards it (conditional kwarg, same
idiom as the sibling paths), and its three callers in _run_one_job_body pass
is_retry_run(job). The shutdown/mark_interrupted path carries no snapshot
and is left as is.
The flag was count_run=True, and mark_job_run passed
count_run=not ladder_rung, so the same idea had two names with opposite
polarity. Use ladder_rung on _advance_after_run too. The docstring now
says when the bump is skipped, and the inline comment lists both reasons a
run is not counted again (a ladder re-run and a pre-claimed finite
one-shot). No behaviour change.
The previous commit stopped a re-run from the unreachable-model ladder from
counting toward repeat, but only on the normal _finish_completed_run path.
The other places that record a failed fire on the same claimed snapshot
still counted it: the crash/interrupt tail of _run_one_job_body, the
pre-handoff dispatch failure in run_one_job, and the recovered unknown
worker outcome in scheduler_worker_failure. A re-run that crashed or failed
to dispatch therefore still bumped repeat.completed a second time for an
occurrence that had already counted.
All three now pass ladder_rung=True when is_retry_run(job). The
worker-failure job is the same run_one_job snapshot, which carries
_scheduled_instant (stamped by the due scan and copied by
_process_due_job), so is_retry_run applies there too. Manual runs carry no
_scheduled_instant and still count.
The kept test gains a "crash" row: a re-run whose run_job raises must leave
repeat.completed at 1.
A recurring job with a finite repeat count (for example repeat=3, every
24h) lost one repeat slot for each automatic re-run after a fire that
could not reach the model. The failed occurrence counted, and then each
5/15/30-minute re-run of that occurrence counted again, so a half-hour
outage finished a repeat=3 job with zero model calls. With
cron.retry_unreachable off, the same outage costs only the one failed
occurrence.
Cause: cron/jobs.py:_advance_after_run bumps repeat.completed on every
mark_job_run, and the re-runs parked by
cron/unreachable_retry.py:plan_retry go through it like scheduled
occurrences. cron/unreachable_retry.py:will_retry also treated a re-run
on the last slot as the job's final run.
Fix: cron/scheduler.py:_finish_completed_run passes ladder_rung=True
when the dispatched instant is the parked re-run instant (new
is_retry_run, which compares the dispatch snapshot's _scheduled_instant
with the ladder state). mark_job_run then calls _advance_after_run with
count_run=False, and will_retry leaves that run out of its final-run
check the same way. The first failed occurrence still counts, and manual
runs still count.
(cherry picked from commit 27038f7064bb611811efee8c527dddecd4fcb57b)
When two cron scheduler instances run against the same jobs.json (e.g.,
a desktop Hermes InProcessCronScheduler and a NSSM-managed
HermesGateway), the cross-process file lock correctly prevents the
loser from firing jobs. However, the "Tick skipped" message was at
DEBUG level, making dual-scheduler suppression invisible in production
logs at the default INFO level.
Change to INFO so operators can see when a scheduler instance is being
suppressed by the lock — this is an expected control-flow outcome, not
diagnostic noise.
Closes#53720
