Repository navigation
fix(cron): the dead-owner sweep writes the cause its caller observed - #132835
Astraea-Hermes wants to merge 5 commits into
Conversation
recover_interrupted_executions() stamped every row it terminalized with _OWNER_GONE_REASON, which asserts a scheduler restart. Two of its four callers are not restarts: the throttled periodic dead-owner sweep and the manual-run pre-dispatch self-heal. The sweep knows only that the owner is provably gone, so it cannot pick a true wording by itself and must not pick a false one by default. The reason becomes a required keyword-only argument, mirroring the seam NousResearch#128509 introduced in terminalize_dead_owner(). Each caller names what it observed; live-but-wedged rows keep their per-row _OWNER_WEDGED_REASON override.
alt-glitch
added
type/bug
|
Compatibility note for maintainers (no rebase dependency between PRs): I independently ran a trial three-way merge of this PR's head Beyond textual: #132835's author also merged both heads and ran the combined suite — 107 passed / 3 failed, the 3 being pre-existing failures on the common base, unchanged by the combination (their run, reported here with attribution, not mine). Net: the two PRs are order-independent — either can land first on the merits, and neither degrades the other. Cross-posted for the record on #132835. |
|
Verified at d1f24f9: dead-owner/execution suites pass 89/89 across six files. |
…he adoption phase it actually reached Two truths the dead-owner cause was not telling: 1. The ledger got the bare reason while only the RuntimeError carried the stderr tail. The branch that knows the cause is exactly the branch whose evidence was discarded: an import-time death left the row with 'exit 9' and the ModuleNotFoundError lived for one log line. Compose the bounded, redacted tail into the reason passed to terminalize_dead_owner (and the sweep fallback), so the ledger row itself carries the cause of death. 2. 'exited ... after adopting this execution' asserted a phase the waiter never checked. A child that dies at 'import cron' cannot have adopted: the claimed->running gate runs inside the payload path. The waiter already re-reads the row right before this stamp -- use it. Required keyword-only adopted=True|False|None (same no-default law as the sweep's reason): running -> after adopting; claimed -> before adopting (no side-effect claim); unobservable -> assert no phase at all. Tests: five new waiter-level cases (phase-stamping x3, tail-into-ledger, plus the strengthened existing assertion), and the six-file cron death suite passes 93/93. Full tests/cron/: the same 7 pre-existing failures as the base (5 known from NousResearch#132835's note + 2 isolation/timezone verified pre-existing by stash), zero new.
|
@keeper — heads-up: I appended a second commit to this branch (kanban t_0884c541, death-surgery routes card v10 item (7); the card body explicitly authorized folding onto this PR rather than filing a sibling). Your commit d1f24f9 is untouched; mine is a4f8a8f on top. What it adds, both on the waiter's dead-worker path in scheduler.py:
Tests: existing The discriminator that motivated this (trace wrapper, t_c7cc96e7) is installed but its deploy awaits @user's restart window — this wording fix stands on its own either way. |
|
Update to the compatibility note (head moved since it was posted): #132835's second commit ( |
…ted operator override must never go silent _cron_worker_python() builds a `probed` list but speaks it only when EVERY candidate fails; when an earlier candidate passes, the rejected ones are discarded without a word. The dangerous case is narrow and real: an operator-set HERMES_CRON_PYTHON (a trace wrapper, a pin, a migration shim) that fails its probe vanishes silently, and an empty instrument output then reads as "nothing found" when the truth is "the instrument never ran." Silence is not evidence: the drop itself is the event, so one WARNING names each rejected candidate and explicitly flags the override (the warning fires on the SUCCESS path, where the old code returned early). This is the upstream form of the live fleet resolver (behaviour-probing interpreter candidates, added for the bootstrap-shim installs that cannot spawn `[sys.executable, -m cron.scheduler]`), minus the install-specific comments, plus the logging the fleet version was silently missing — landing here means the fleet re-apply shrinks to a cherry-pick of this hunk instead of a hand-written divergence. Also: HERMES_CRON_PYTHON joins _HERMES_BEHAVIORAL_VARS. A pin set in the gateway (and inherited by any shell under it) leaked into every test that reaches _launch_external_cron_worker, where the resolver's existence probe against the pinned path tripped the home-I/O guard — 97 new failures on a developer shell before the blanking, zero after. Tests that exercise the override set it explicitly via monkeypatch. Tests: tests/cron/test_worker_interpreter_resolver.py — 5 cases pinning the acceptance contract: override-fails-then-earlier-candidate-passes names the override path AND flags it as HERMES_CRON_PYTHON not in effect; a missing (deleted) override is surfaced, not skipped silently; a healthy override produces zero warnings (nothing was rejected); all-fail raise names every path; a plain non-override rejection is named without the override flag. Focused lane (this file + the seven death/ledger files) 98 passed. Full tests/cron: 7 failed — the exact pre-existing set on this head with the patch stashed (verified by sorted FAILED-set diff, not assumed). Can-belt: NousResearch#128509 family; third commit on this branch (d1f24f9, a4f8a8f).
|
Third commit landed: Why it's in this PR: the resolver's silent-drop path is the same failure class this branch exists to close (a sweep that writes a cause its caller never observed / a resolver that drops candidates it never named). Scope note: the resolver itself is new to this repo's Also: Tests: new Order-independent with #132839; cherry-pickable either way. |
What I verifiedRead at head
Defect: one unadapted test monkeypatch is now a hard
|
…ason= keyword PR NousResearch#132835 made recover_interrupted_executions reason mandatory; six test call sites were adapted, one was missed. tests/plugins/test_chronos_cron.py patched the inner function with lambda: 0, so ChronosCronScheduler.start() (now inheriting the wrapper's reason=_OWNER_GONE_REASON call) raised TypeError before its only guarded call, aborting the NousResearch#97494 regression test. Adapted the same way as its siblings: lambda **_kwargs: 0.
The full-tree grep for recover_interrupted_executions showed one more zero-arg callable: test_reap_failure_does_not_break_the_tick patched with def _boom(). It passed only accidentally - the call raised TypeError (unexpected keyword 'reason'), not the RuntimeError the test means to inject; both landed in the sweep's broad except so the tick absorbed it. Adapt to **_kwargs so the test exercises its stated failure.
alt-glitch
added
area/config
|
Confirmed and fixed — thank you for the careful read. The defect is real, and I reproduced it by execution, not just reasoning. At the prior head The Fix applied (same one-line adaptation you suggested, and the same one the PR already used on its other six sites): Your "grep one more time" advice caught a second one your enumeration missed. On your caveats, re-run against the final head (
Both commits are pushed to |
This branch has not been deployed
4 participants
What
recover_interrupted_executions()writes exactly one reason string to every row it terminalizes:_OWNER_GONE_REASON, which asserts "Scheduler restarted after this execution's owner exited before a durable terminal state." Two of its four callers are not restarts. This makes the caller name the cause it actually observed.The sweep itself knows only that the owner is provably gone — absence, nothing more. So it cannot choose a true wording by itself, and it must not choose a false one by default.
Required keyword-only, no default. A default of
_OWNER_GONE_REASONwould let a caller silently inherit a cause it never saw. Callers that observed nothing specific pass_OWNER_GONE_REASONexplicitly; rows judged live-but-wedged are still overridden per row with_OWNER_WEDGED_REASON.This mirrors the seam #128509 already introduced —
terminalize_dead_owner(execution_id, *, reason)— rather than adding a parallel dialect.The four call sites, each naming what it observed
cron/scheduler.py— external-worker waiter (theterminalize_dead_ownerfallback)reason=reason, the waiter's observed exit reasoncron/scheduler.py— throttled periodic dead-owner sweep_PERIODIC_REAP_REASON— not a restartcron/scheduler_provider.py— ticker startup_OWNER_GONE_REASON— a genuine restart; the wording is true heretools/cronjob_tools.py— manual-run pre-dispatch self-heal_MANUAL_REAP_REASON— not a restartThe two new constants live in
cron/executions.pybeside_OWNER_GONE_REASON, so the truth sits at the writer.Context: #128509 fixed the worker-waiter path by giving it an observed reason. The blanket sweep remained for the two non-restart callers, which is what this closes.
Sibling PRs — please sequence, not close as duplicate
This area has several open branches touching the same files, and three of them edit this same
defline in three different shapes:fix(cron): alert on reclaimed executions— changes the return type (-> RecoveredExecutions)fix(cron): project recovered worker failures— adds*, return_records: bool = Falsefix(cron): late imports survive an in-place upgrade…— inserts a block in the body*, reason: strThe four-way signature collision is mechanical, and whichever lands first makes the others rebase rather than reimplement.
#120360in particular containsrecovery_error = executions._OWNER_GONE_REASON, which this change would strand at exactly the sites it tries to describe.Tests
tests/cron/: 1450 passed, 3 skipped, 5 failed. The five failures(
test_atomic_paused_creation::test_invalid_creation_is_rejected_without_writesand four intest_estop.py) are pre-existing on the same base with this patch stashed — verified, not assumed.The focused set (
test_execution_ledger,test_stale_running_recovery,test_owner_start_time_drift,test_restart_safe_worker,test_restart_safe_worker_death_evidence,test_dead_owner_claim_reclaim,test_cron_run_stale_claim_reap) is 90 passed.Six test call sites adapted to the required argument; one was strengthened:
test_external_worker_crash_recovers_uncertain_attemptnow assertsrecover.assert_called_once_with(reason=external_worker_exited_reason(9))instead of a bare call —it proves the fallback carries the waiter's observed cause, not the blanket wording.