Repository navigation
fix(cron): surface acknowledged worker failures and release stale claims - #129748
unsupportedpastels merged 4 commits into
Conversation
…at never happened (NousResearch#128509) A restart-safe cron worker that dies AFTER adopting its attempt was reported as "Scheduler restarted after this execution's owner exited" and treated as success. recover_interrupted_executions() only knows that an owner is gone, so that is all it can write -- but the waiter holding the worker's Popen knows the owner was that worker and knows its exit status. Worse, the sweep leaves the row terminal, so the waiter returned True, run_one_job short-circuited, and mark_job_run never ran: the job's fire claim outlived its lost execution and refused the next manual fire with "Job is already being fired" for the whole 300s lease, with an empty last_error. - executions.terminalize_dead_owner(): terminalize ONE attempt with an observed cause. Refuses a live owner, this process, and a handoff inside the adoption grace, so it can never steal a running worker's attempt; returns False so the caller keeps the generic sweep. - The waiter records that cause and RAISES, routing the existing post-handoff bookkeeping: mark_job_run records the run and retires the claim, no dispatch incident and no false crash notice. Also folds the worker's captured stderr into the error, which the pre-ack path already did (NousResearch#112729) and this path dropped. (cherry picked from commit d3f3571)
…tics Extend Finn763's dead-owner recovery with owner-fenced failure delivery based on PR NousResearch#120391 and retain stderr until the parent consumes an escaping worker traceback. Preserve unknown side-effect state and do not retry automatically. Cover both shared entry paths, an exception through the payload lifecycle, abrupt death, and concurrent recovery. Co-authored-by: Denis Dresvyanskiy <dresvyanskiydenis@gmail.com>
arkheioncorp
left a comment
There was a problem hiding this comment.
Revisão: PR #129748
Verdict: APPROVE
Análise
Resolve #128509: um worker externo que morre após adoção deixava o execution como "unknown" com causa genérica "Scheduler restarted...", e o run_one_job retornava True (pulando mark_job_run), bloqueando o fire_claim para o próximo fire manual.
Mudanças principais:
terminalize_dead_owner()emcron/executions.py: terminaliza um execution para "unknown" com causa real observada pelo waiter, com guards contra live workers e handoff adoption grace.cron/scheduler_worker_failure.py(new):record_unknown_worker_outcome()eexternal_worker_exited_reason()para visibilidade de worker failure.- Modificações em
_wait_for_external_cron_worker_body(): agora usaterminalize_dead_owner()antes de fallback pararecover_interrupted_executions(). run_one_job(): trata post-handoff failures viarecord_unknown_worker_outcome().- Testes novos em
test_restart_safe_worker_death_evidence.pycobrindo múltiplos cenários de morte de worker.
Security scan
Nenhum hardcoded secret, injection, eval/exec, ou deserialization encontrado. Código de scheduler interno.
Qualidade
- A lógica está correta: o waiter observa a morte do worker e registra a causa real.
- Os guards em
terminalize_dead_owner()(live worker check, handoff grace) são essenciais e bem implementados. - Testes muito completos cobrindo hard exit, payload exception, concurrent recovery, timeout variants.
Sugestões (non-blocking)
Nenhuma.
Aprovado.
unsupportedpastels
left a comment
There was a problem hiding this comment.
Self-review (COMMENT; own PR). Independent read-only review approved exact head 166ea75 on base 7239625; exact-head CI: run 36793140949 green, including All required checks pass, Python tests, ruff/ty lints, Windows/macOS OS-specific tests and contributor attribution; also cron suite 1,422 passed / 3 skipped on a local trial merge with current main 6633626. Evidence: repro across main and 11 candidate PRs, RED checks against main and #128616 production, cron suite 1,422 passed / 3 skipped (baseline 1,411 / 3). Authorship: Finn763's commit preserved via cherry-pick -x; Denis Dresvyanskiy credited by Co-authored-by trailer.
Summary
Fixes #129254
Surface post-acknowledgement worker failures: record the worker's exit cause, preserve its stderr diagnostic, retire the matching fire claim, and invoke the job's existing failure-notification lane.
The execution ledger deliberately remains
unknown: an acknowledged worker may already have performed side effects. The job records an error. This does not automatically retry or claim to identify/prevent the reporter's initiating startup crash.Root cause
After a worker adopts its execution and dies, the parent recovers its row to terminal
unknown, then returns without recording the job outcome. The fire claim and empty job error survive. Separately, the payload cleanup unlinks stderr before Python prints an escaping exception, so an outer traceback wrapper cannot preserve that diagnostic.Fix
cron/scheduler_worker_failure.py; normal worker completion and unrelated waiter-error semantics remain unchanged.Prior work and credit
d3f3571f8c6f4d2f188e90151d8f8393f049a573with-x, retaining the original author. Its exit-evidence/claim fix is the base; this adds failure delivery and closes its payload-exception stderr gap.Co-authored-by: Denis Dresvyanskiy <dresvyanskiydenis@gmail.com>. This version holds the fire fence over the side effect and adds worker diagnostics.e322e76, records a worker-side traceback and releases the owner-fenced claim whenrun_one_jobraises. This PR reaches the same claim/cause outcome from the parent, so it also covers a hard worker exit and sends the job's failure notice. No code copied. One deliberate difference: the ledger staysunknownrather thanfailed, becauserun_one_jobmay already have had side effects before it raised.Test plan
Canonical runner, with
HERMES_PYTHON=/home/mark/.hermes/hermes-agent/venv/bin/python:scripts/run_tests.sh tests/cron/ --file-retries 0 -j 8on base7239625ae1b: baseline 1,411 passed / 3 skipped; this branch 1,422 passed / 3 skipped; no failures.e322e76and fix(cron): relaunch with dependencies, never mid-handoff, and keep the dead worker's stderr #126355); 4/4 pass on this branch. This is controlled fault injection, not reproduction of the reporter's unknown crash cause.python3 scripts/check-windows-footguns.py --all: passed.python -m ruff check cron/scheduler.py cron/executions.py cron/scheduler_worker_failure.py tests/cron/test_restart_safe_worker.py tests/cron/test_restart_safe_worker_death_evidence.py: passed.git diff --check origin/main..HEAD: passed; contributor author-email mapping check passed.Scope and risk
No real Telegram delivery, native Windows/macOS runtime run, or hosted CI is claimed yet. Notification tests exercise the existing delivery boundary without sending messages.
This covers the surviving parent observing its worker's death, as in the report. If the gateway itself has already died, it cannot project or notify the failure here; broader periodic recovery remains separate. A failed worker may leave its stderr capture if no parent survives. No interpreter-selection, lazy-install policy, schema, or automatic retry changes. Rollback is reverting the four implementation commits.