Skip to content

fix(cron): late imports survive an in-place upgrade; a released claim records its outcome first - #121052

Open
djmango wants to merge 1 commit into
NousResearch:mainfrom
djmango:fix/cron-module-skew-and-durable-claim
Open

djmango wants to merge 1 commit into
NousResearch:mainfrom
djmango:fix/cron-module-skew-and-durable-claim

Conversation

@djmango

@djmango djmango commented Sep 24, 2026

Copy link
Copy Markdown

Root cause (proven)

Two cron executions were recorded failed with an import error while their workers kept running to completion:

execution job status/error output written
(17:00:14) b20a472b7e80 failed — cannot import name 'safe_strftime' from 'hermes_time' 18:19:17
(17:07:15) f98d49b4524e failed — same 18:57:34

A long-running process had hermes_time cached in sys.modules from the revision that predated safe_strftime, then imported a consumer module for the first time after the tree was upgraded in place at 18:12:46 (bulk mtime batch over 2862 .py files). That late import needed a new name from an already-cached module:

ImportError: cannot import name 'safe_strftime' from 'hermes_time' (<tree>/hermes_time.py)

Evidence, in order:

  • hermes_time at the running revision vs the tree that landed mid-run: safe_strftime/_repair_surrogates are the only names added (git show <old>:hermes_time.py vs git show <new>:hermes_time.py → diff shows exactly those two).
  • Reproduced deterministically — delete the name from the cached module, then import a consumer:
    $ python -c "import hermes_time; delattr(hermes_time,'safe_strftime'); from hermes_time import safe_strftime"
    ImportError: cannot import name 'safe_strftime' from 'hermes_time' (...)
    
  • The repo already documents this exact hazard class and carries a fix + test for a sibling (cron/ledger.py, tests/cron/test_upgrade_module_skew.py): "a lazily imported module after an upgrade must not need new names from a module the daemon already has cached".

Changes

  1. hermes_time_format.py (new leaf module) holds safe_strftime + _repair_surrogates. It imports nothing from hermes_time, so a consumer first imported after an upgrade loads it from disk. hermes_time re-exports both names for existing callers, and the 12 consumers that import safe_strftime now import it from the leaf module.
  2. Durable terminal state before claim release (cron/scheduler.py, cron/executions.py): sweep_stale_inflight used to mutate the in-flight guard in its classification loop and only then write the release record — and for an age release of a finite-repeat job it wrote nothing durable. Now the outcome is written before the claim is dropped: cron.executions.mark_claim_unknown() terminalises the claim's still-open ledger row (unknown + the #115692 reason), then the release happens, re-checking the claim's start stamp so a fresh fire in the gap survives. If the write fails the claim is retained (fail closed) so the next sweep retries instead of losing the attempt.

Tests

$ PYTHONPATH=<runtime site-packages> uvx --python <runtime python> \
    pytest tests/cron/test_stale_claim_durable_terminal.py tests/cron/test_upgrade_module_skew.py -q
12 passed

$ ... pytest tests/cron/test_inflight_stale_guard.py tests/cron/test_recurring_wedge_selfheal.py \
    tests/cron/test_stale_running_recovery.py tests/cron/test_dead_owner_claim_reclaim.py \
    tests/cron/test_cron_multiplex_inflight_release.py tests/cron/test_stale_module_leaf_imports.py \
    tests/cron/test_timezone.py tests/agent/test_system_prompt.py \
    tests/cron/test_upgrade_module_skew.py tests/cron/test_stale_claim_durable_terminal.py -q
134 passed

The new tests are RED on the parent commit (6 failed, 6 passed), reproducing the production signature verbatim:

E  ImportError: cannot import name 'safe_strftime' from 'hermes_time' (.../hermes_time.py)

Uncertain

  • Which of the 12 consumers actually raised in production cannot be named from the DB row (the traceback was collapsed into the row's error), but the mechanism is identical for all of them and the fix covers every module-level consumer — the only two names added to hermes_time in that upgrade are the two that moved.
  • The wedge rows' precise trigger (hung futex behind a route/proxy flip, per #115692) is unchanged by this PR: a live-but-wedged worker is still not killed. What is now guaranteed is that its claim always has a durable outcome on disk before it is released.

…rd a released claim's outcome first

Two cron executions (b20a472b7e80 at 17:00, f98d49b4524e at 17:07) were marked failed with
"cannot import name 'safe_strftime' from 'hermes_time'" while both workers kept running to
completion (output written 18:19 / 18:57). A long-running process had hermes_time cached from
the revision that predated safe_strftime, then imported a consumer for the first time after the
tree was upgraded in place at 18:12, so the import needed a NEW name from a module it already
had cached. Move safe_strftime/_repair_surrogates into their own stdlib-only leaf module
(hermes_time re-exports them for existing callers) so a post-upgrade import loads them from
disk instead of from the stale cached module.

Second: sweep_stale_inflight dropped the in-flight claim in its classification loop and only
afterwards wrote the release record; for an "age" release of a finite-repeat job it wrote no
durable record at all. A released claim could therefore leave no answer to "did side effects
run?" (NousResearch#115692). Write the durable outcome BEFORE releasing — terminalising the claim's
still-open ledger row via cron.executions.mark_claim_unknown — then release, re-checking the
claim's start stamp so a fresh fire in the gap survives; if the write fails the claim is
retained so the next sweep retries instead of losing the attempt.

Tests: tests/cron/test_upgrade_module_skew.py reproduces the exact ImportError against a
pre-upgrade hermes_time (and pins the failure signature), tests/cron/test_stale_claim_durable_terminal.py
covers write-before-release ordering, fail-closed retention, and the durable ledger terminal state.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery comp/cron Cron scheduler and job management area/install-update Installer, updater, packaging, wheels, doctor sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Sep 24, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated follow-up for reference; not a maintainer.

The module-skew half looks right: hermes_time_format.py really is a stdlib-only leaf, every consumer is repointed (exhaustive grep), and test_upgrade_module_skew.py genuinely simulates a pre-upgrade cached hermes_time. The cron half has a blocker.

1. mark_claim_unknown terminalises the newest open row, not the claim being released (cron/executions.py:407-424). The select is ORDER BY claimed_at DESC, id DESC and the filter is a lower bound only, claimed_at >= claimed_after. claimed_after is _running_since[key], stamped by try_register_running_job (cron/scheduler.py:780-788), so a newer attempt's row satisfies it and — being first — wins. Reproduced on real cron/executions.py, base 7135182569f7b3773e517717798bca03d5d91ebf vs head 31aba735b0ef0ac60aec68dc66a4f5d34485e9eb, two create_execution rows for one job:

row base head
A — the claim being released claimed claimed (no outcome recorded)
B — a newer live attempt claimed unknown

Exactly inverted. create_execution (:193-201) is a bare INSERT with no uniqueness guard, and head runs phase 2 outside _running_lock with the claim still held (:1063-1069) — a window the PR documents at :1076-1077. The re-check at :1078 guards only the in-memory guard; the durable write at :1069 is already committed.

Chain to a double dispatch: unknown ∈ _TERMINAL_STATES (:33), so the next sweep takes ledger-terminal at cron/scheduler.py:1050-1055 and releases a still-running claim. Please confirm — I could not execute cron/scheduler.py standalone.

2. unknown is written with no owner-liveness proof, contradicting the module docstring at cron/executions.py:3-5. recover_interrupted_executions gates every write on process_id != _PROCESS_ID, _owner_is_live(), the stale bound and a handoff grace window, with a compare-and-set on pid/process_started_at (:338-372). mark_claim_unknown checks none of them. Please confirm this is intended.

Minor: stale is bound at cron/scheduler.py:1015 then re-bound at :1062; the new test only passes a future claimed_after, leaving this ordering untested.

Unverified / please confirm: all sweep_stale_inflight runtime behaviour (point 1's last paragraph, point 2's intent). Run to verify: point 1's table. Read from post-image: every file:line above.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants