Conversation
|
Thanks for tracing this through the current multiplex path. The steady-state fix matches the defect on current main: Problems
Suggested changes
Automated hermes-sweeper review. |
|
Confirmed and fixed in 011180e7b. The path you pointed at holds. The recovery pass at One correction to the impact, in the direction of worse. The consequence is not that later profiles miss their first tick; it is that no profile ticks at all. The recovery pass runs before the Changes:
Left alone on purpose: the single-profile path ( PR body updated to cover both halves. |
011180e to
47cfaad
Compare
Under multiplex_profiles, _start_multiplex() runs the per-profile tick loop inside one try/except for the whole cycle, while each profile's own block is try/finally with no except. cron.scheduler.tick() has no catch-all of its own (its body in cron/scheduler.py is try/finally), so anything it raises unwinds out of the loop. Two consequences: 1. Every profile ordered after the raising one is skipped for that cycle. A transient error self-heals next cycle; a persistent per-profile failure does not. The jobs.json ownership class of problem fixed on the owner side in 722bf5d is exactly such a failure, and it starves the profiles after it indefinitely with no signal that they were never reached. 2. The heartbeat pass that follows writes one cycle-wide `ok` into every profile's store, so `hermes cron status` reports profiles as failing that ticked cleanly — and record_ticker_error(), added so the reason is visible (NousResearch#68483), files the unrelated profile's error against them. Changes: - cron/scheduler_provider.py — each profile's tick gets its own try/except BaseException (BaseException for the same reason the single-profile path catches it, NousResearch#32612: a SystemExit out of a provider SDK must not kill the ticker thread). Each cycle collects one (profile, ok, error) outcome and the heartbeat pass records each profile's own result instead of a shared flag. - tests/cron/test_scheduler_provider.py — two regression tests: test_multiplex_tick_error_does_not_stop_other_profiles and test_multiplex_heartbeat_success_is_per_profile. Unchanged on purpose: the single-profile path (profile_homes unset) is untouched, and a paused can_dispatch cycle keeps its current meaning — nothing attempted, recorded as clean — matching the single-profile path. Verified on Python 3.13.14: tests/cron/test_scheduler_provider.py is 22 passed with this change; reverting only the source hunk and keeping the tests gives 2 failed, which is what makes them regression tests rather than decoration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to the tick-loop isolation in this branch, closing the same gap in the startup pass of the same function. Per review on NousResearch#74888. _start_multiplex() recovers interrupted attempts for every profile before entering the tick loop, and that pass is try/finally with no except. recover_interrupted() lands in recover_interrupted_executions(), which opens a transaction against that profile's own ledger; neither it nor _transaction() has a catch-all, so an unreadable or locked store raises straight out of start(). The blast radius is wider than in the tick loop, because this pass runs BEFORE the loop is entered. The gateway runs start() as a bare thread target (gateway/run.py::start_gateway) with nothing above it but threading's excepthook, which logs the traceback and neither restarts the thread nor stops the process. So one bad ledger kills the ticker before a single cycle runs and EVERY profile stops firing — including the ones whose recovery succeeded, and including the failing profile's own jobs, which may well be tickable — until the gateway is restarted. Nothing in `hermes cron status` attributes the silence to the profile that caused it. Changes: - cron/scheduler_provider.py — the per-profile recovery block gets its own try/except BaseException (BaseException for the same reason as the tick loop, NousResearch#32612). The failure is logged and recorded via record_ticker_error() into that profile's own store, and the loop continues to the next profile. The error-recording is itself guarded: the store is exactly what may be unreachable, and letting the report of a failure raise would reintroduce the failure it reports. The profile stays in profile_homes — a ledger that cannot be recovered may still hold tickable jobs, and if it cannot be ticked either, the per-profile handler in the loop records that on its own terms. - tests/cron/test_scheduler_provider.py — test_multiplex_startup_recovery_error_does_not_stop_other_profiles: three profiles, the first raises sqlite3.OperationalError from recovery; all three must still be ticked. - the docstring's isolation guarantee now covers startup as well as steady state, matching what the code delivers. Unchanged on purpose: the single-profile path (profile_homes unset) still lets a recovery failure propagate. There is no other profile to starve there, and the process owns exactly one store, so failing loudly stays the right contract. Verified on Python 3.13.14: tests/cron/test_scheduler_provider.py is 23 passed with this change; reverting only the source hunk and keeping the tests gives 1 failed, with the thread dying on the unhandled sqlite3.OperationalError and no profile ticked at all — the predicted failure mode observed directly. tests/cron/ is 349 passed / 19 skipped, plus 8 failures that reproduce identically on an untouched baseline (POSIX mode bits and ~ expansion on Windows) and are unrelated to this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
47cfaad to
59ee25c
Compare
One profile's broken cron store no longer takes the whole multiplex ticker down with it: - startup recovery loop: a per-profile exception (e.g. an unreadable executions.db raising sqlite3.DatabaseError) was uncaught and killed the ticker thread before its first tick — no profile ever fired. - tick loop: only CronTickYielded was caught per profile; any other exception escaped to the cycle-wide handler, skipping every remaining profile that cycle and marking all of them failed. Both loops now catch per profile, record the failure into THAT profile's ticker_last_error (`hermes cron status`), and keep ticking the siblings. The existing CronTickYielded/_profile_errors semantics and the #87644 EMFILE reclaim/backoff are preserved (backoff is applied once per cycle from the worst per-profile failure). Salvaged from PR #70747 (@Cyber-Yichen); the recovery test's real sqlite3.OperationalError shape is from PR #74888 (@OYLFLMH). Same class also reported in PR #74952 (@webtecnica). Co-authored-by: OYLFLMH <95945448+OYLFLMH@users.noreply.github.com> Co-authored-by: webtecnica <75556242+webtecnica@users.noreply.github.com>
One profile's broken cron store no longer takes the whole multiplex ticker down with it: - startup recovery loop: a per-profile exception (e.g. an unreadable executions.db raising sqlite3.DatabaseError) was uncaught and killed the ticker thread before its first tick — no profile ever fired. - tick loop: only CronTickYielded was caught per profile; any other exception escaped to the cycle-wide handler, skipping every remaining profile that cycle and marking all of them failed. Both loops now catch per profile, record the failure into THAT profile's ticker_last_error (`hermes cron status`), and keep ticking the siblings. The existing CronTickYielded/_profile_errors semantics and the #87644 EMFILE reclaim/backoff are preserved (backoff is applied once per cycle from the worst per-profile failure). Salvaged from PR #70747 (@Cyber-Yichen); the recovery test's real sqlite3.OperationalError shape is from PR #74888 (@OYLFLMH). Same class also reported in PR #74952 (@webtecnica). Co-authored-by: OYLFLMH <95945448+OYLFLMH@users.noreply.github.com> Co-authored-by: webtecnica <75556242+webtecnica@users.noreply.github.com>
One profile's broken cron store no longer takes the whole multiplex ticker down with it: - startup recovery loop: a per-profile exception (e.g. an unreadable executions.db raising sqlite3.DatabaseError) was uncaught and killed the ticker thread before its first tick — no profile ever fired. - tick loop: only CronTickYielded was caught per profile; any other exception escaped to the cycle-wide handler, skipping every remaining profile that cycle and marking all of them failed. Both loops now catch per profile, record the failure into THAT profile's ticker_last_error (`hermes cron status`), and keep ticking the siblings. The existing CronTickYielded/_profile_errors semantics and the #87644 EMFILE reclaim/backoff are preserved (backoff is applied once per cycle from the worst per-profile failure). Salvaged from PR #70747 (@Cyber-Yichen); the recovery test's real sqlite3.OperationalError shape is from PR #74888 (@OYLFLMH). Same class also reported in PR #74952 (@webtecnica). Co-authored-by: OYLFLMH <95945448+OYLFLMH@users.noreply.github.com> Co-authored-by: webtecnica <75556242+webtecnica@users.noreply.github.com>
One profile's broken cron store no longer takes the whole multiplex ticker down with it: - startup recovery loop: a per-profile exception (e.g. an unreadable executions.db raising sqlite3.DatabaseError) was uncaught and killed the ticker thread before its first tick — no profile ever fired. - tick loop: only CronTickYielded was caught per profile; any other exception escaped to the cycle-wide handler, skipping every remaining profile that cycle and marking all of them failed. Both loops now catch per profile, record the failure into THAT profile's ticker_last_error (`hermes cron status`), and keep ticking the siblings. The existing CronTickYielded/_profile_errors semantics and the #87644 EMFILE reclaim/backoff are preserved (backoff is applied once per cycle from the worst per-profile failure). Salvaged from PR #70747 (@Cyber-Yichen); the recovery test's real sqlite3.OperationalError shape is from PR #74888 (@OYLFLMH). Same class also reported in PR #74952 (@webtecnica). Co-authored-by: OYLFLMH <95945448+OYLFLMH@users.noreply.github.com> Co-authored-by: webtecnica <75556242+webtecnica@users.noreply.github.com>
One profile's broken cron store no longer takes the whole multiplex ticker down with it: - startup recovery loop: a per-profile exception (e.g. an unreadable executions.db raising sqlite3.DatabaseError) was uncaught and killed the ticker thread before its first tick — no profile ever fired. - tick loop: only CronTickYielded was caught per profile; any other exception escaped to the cycle-wide handler, skipping every remaining profile that cycle and marking all of them failed. Both loops now catch per profile, record the failure into THAT profile's ticker_last_error (`hermes cron status`), and keep ticking the siblings. The existing CronTickYielded/_profile_errors semantics and the #87644 EMFILE reclaim/backoff are preserved (backoff is applied once per cycle from the worst per-profile failure). Salvaged from PR #70747 (@Cyber-Yichen); the recovery test's real sqlite3.OperationalError shape is from PR #74888 (@OYLFLMH). Same class also reported in PR #74952 (@webtecnica). Co-authored-by: OYLFLMH <95945448+OYLFLMH@users.noreply.github.com> Co-authored-by: webtecnica <75556242+webtecnica@users.noreply.github.com>
One profile's broken cron store no longer takes the whole multiplex ticker down with it: - startup recovery loop: a per-profile exception (e.g. an unreadable executions.db raising sqlite3.DatabaseError) was uncaught and killed the ticker thread before its first tick — no profile ever fired. - tick loop: only CronTickYielded was caught per profile; any other exception escaped to the cycle-wide handler, skipping every remaining profile that cycle and marking all of them failed. Both loops now catch per profile, record the failure into THAT profile's ticker_last_error (`hermes cron status`), and keep ticking the siblings. The existing CronTickYielded/_profile_errors semantics and the #87644 EMFILE reclaim/backoff are preserved (backoff is applied once per cycle from the worst per-profile failure). Salvaged from PR #70747 (@Cyber-Yichen); the recovery test's real sqlite3.OperationalError shape is from PR #74888 (@OYLFLMH). Same class also reported in PR #74952 (@webtecnica). Co-authored-by: OYLFLMH <95945448+OYLFLMH@users.noreply.github.com> Co-authored-by: webtecnica <75556242+webtecnica@users.noreply.github.com>
One profile's broken cron store no longer takes the whole multiplex ticker down with it: - startup recovery loop: a per-profile exception (e.g. an unreadable executions.db raising sqlite3.DatabaseError) was uncaught and killed the ticker thread before its first tick — no profile ever fired. - tick loop: only CronTickYielded was caught per profile; any other exception escaped to the cycle-wide handler, skipping every remaining profile that cycle and marking all of them failed. Both loops now catch per profile, record the failure into THAT profile's ticker_last_error (`hermes cron status`), and keep ticking the siblings. The existing CronTickYielded/_profile_errors semantics and the #87644 EMFILE reclaim/backoff are preserved (backoff is applied once per cycle from the worst per-profile failure). Salvaged from PR #70747 (@Cyber-Yichen); the recovery test's real sqlite3.OperationalError shape is from PR #74888 (@OYLFLMH). Same class also reported in PR #74952 (@webtecnica). Co-authored-by: OYLFLMH <95945448+OYLFLMH@users.noreply.github.com> Co-authored-by: webtecnica <75556242+webtecnica@users.noreply.github.com>
|
Thanks for this PR. Merged via #101245 (eed481b) on current main — multiplex cron ticks isolate per profile, fire the bound port, deliver satellites fail-closed. #101245 won as the consolidated fix because it covers the whole multiplex-profile bug class in one change (with tests) rather than the single symptom addressed here; this PR is superseded by it. If anything from your original change is still missing on main >= eed481b, please open a fresh PR/issue against main and tag it. Thanks again. |
…ch#74878) One profile's broken cron store no longer takes the whole multiplex ticker down with it: - startup recovery loop: a per-profile exception (e.g. an unreadable executions.db raising sqlite3.DatabaseError) was uncaught and killed the ticker thread before its first tick — no profile ever fired. - tick loop: only CronTickYielded was caught per profile; any other exception escaped to the cycle-wide handler, skipping every remaining profile that cycle and marking all of them failed. Both loops now catch per profile, record the failure into THAT profile's ticker_last_error (`hermes cron status`), and keep ticking the siblings. The existing CronTickYielded/_profile_errors semantics and the NousResearch#87644 EMFILE reclaim/backoff are preserved (backoff is applied once per cycle from the worst per-profile failure). Salvaged from PR NousResearch#70747 (@Cyber-Yichen); the recovery test's real sqlite3.OperationalError shape is from PR NousResearch#74888 (@OYLFLMH). Same class also reported in PR NousResearch#74952 (@webtecnica). Co-authored-by: OYLFLMH <95945448+OYLFLMH@users.noreply.github.com> Co-authored-by: webtecnica <75556242+webtecnica@users.noreply.github.com>
Problem
Under
multiplex_profiles,InProcessCronScheduler._start_multiplex()ticks every served profile's cron store (#69377). Two per-profile blocks in that function aretry/finallywith noexcept, so one profile's failure escapes into the others.1. The tick loop
The per-profile loop sits inside a single
try/except BaseExceptionfor the whole cycle, while each profile's own block istry/finallywith noexcept:cron.scheduler.tick()has no catch-all of its own — its body incron/scheduler.pyistry/finally— so anything raised inside it propagates to this caller. Two consequences:Profiles ordered after the raising one are skipped for that cycle. A transient error self-heals on the next cycle; a persistent per-profile failure does not. The
jobs.jsonownership class of problem fixed on the owner side in722bf5d51is exactly such a failure, and it starves every profile after it indefinitely — with no signal that they were never reached.The heartbeat pass writes one cycle-wide
okinto every profile's store.hermes cron statusthen reports profiles as failing that ticked cleanly, andrecord_ticker_error()— added so the reason is visible (cron: CLI run as root silently locks out the uid-1000 ticker (jobs.json rewritten root:600); zombie ticker never surfaced #68483) — files the unrelated profile's error message against them.2. The startup recovery pass
The same gap exists in the pass that runs before the loop is entered:
recover_interrupted()lands inrecover_interrupted_executions(), which opens a transaction against that profile's own ledger; neither it nor_transaction()has a catch-all, so an unreadable or locked store raises straight out ofstart().The blast radius here is wider than in the tick loop, precisely because this pass runs before the loop. The gateway runs
start()as a bare thread target (gateway/run.py::start_gateway) with nothing above it butthreading.excepthook, which logs the traceback and neither restarts the thread nor stops the process. One bad ledger therefore kills the ticker before a single cycle runs, and every profile stops firing — including the ones whose recovery succeeded, and including the failing profile's own jobs, which may well still be tickable — until the gateway is restarted. Nothing inhermes cron statusattributes the silence to the profile that caused it.Solution
cron/scheduler_provider.py:try/except BaseException.BaseExceptionfor the same reason the single-profile path catches it ([Bug]: Cron ticker dies silently — no error log, no watchdog, misleading status #32612): aSystemExitout of a misbehaving provider SDK must not kill the ticker thread.(profile, ok, error)outcome, and the heartbeat pass records each profile's own result rather than a shared flag.record_ticker_error()into that profile's own store, continue to the next profile. That error-recording is itself guarded — the store is exactly what may be unreachable, and letting the report of a failure raise would reintroduce the failure it reports.profile_homes. A ledger that cannot be recovered may still hold tickable jobs, and if it cannot be ticked either, the per-profile tick handler records that on its own terms.Unchanged on purpose:
profile_homesunset) is untouched, including its recovery call. There is no other profile to starve there and the process owns exactly one store, so failing loudly stays the right contract;can_dispatchcycle keeps its current meaning — nothing attempted, recorded as clean — matching the single-profile path.Tests
Three regression tests in
tests/cron/test_scheduler_provider.py:test_multiplex_tick_error_does_not_stop_other_profiles— three profiles, the first raises every cycle; the other two must still be ticked.test_multiplex_heartbeat_success_is_per_profile— a profile that ticked cleanly must not be recorded as failing because a different profile raised.test_multiplex_startup_recovery_error_does_not_stop_other_profiles— three profiles, the first raisessqlite3.OperationalErrorfrom recovery; all three must still be ticked.All three fail on current
mainand pass with this change. Verified by reverting only the source file and keeping the tests:On the unfixed code the recovery test fails the way the analysis predicts rather than by a bare assertion: the ticker thread dies on the unhandled
sqlite3.OperationalError— pytest reports it as an unhandled thread exception — and no profile is ticked at all.Run locally on Python 3.13.14. The 20 pre-existing tests in that file pass unchanged in both runs.
Context
This is the one gap that survived from #54913, which I closed as obsolete after #69377 and
a61183b56landed the same cross-profile design.The startup-recovery half was raised in review on this PR and is fixed in 011180e7b.
Note on scope: I do not run a multiplex production deployment. The failure modes are derived from reading
_start_multiplex(),tick(),recover_interrupted_executions()and the gateway's thread setup rather than from an observed incident, and are validated by the three tests above. Feedback from multiplex users is welcome.