fix(cron): preserve completed jobs after claim loss

[KoNit-K]

What does this PR do?

Fixes a cron bookkeeping race where a long-running job could finish with a complete response, then be changed to Interrupted by shutdown before terminal completion. because a previously failed fire-claim heartbeat left its cancellation event set. The scheduler now revalidates the durable fire claim before treating that signal as terminal. A current owner records the completed result; a genuinely replaced or unavailable owner remains fenced out. Explicit transport cancellation remains an interruption even when the fire claim is still healthy.

Related Issue

Fixes #113357

Type of Change

• Bug fix
• Regression coverage
• No API or configuration changes

Changes Made

• cron/scheduler.py — distinguish the scheduler fire-claim heartbeat event from an independent transport cancellation, then revalidate the current fire owner before converting a completed run into an interruption.
• cron/scheduler.py — keep an already-signaled ownership loss fail-closed when the revalidation cannot reach the store, while retaining the existing best-effort behavior for an initial validation error.
• tests/cron/test_shutdown_interrupt.py — cover a completed response after heartbeat loss, a genuinely lost owner with an incomplete response, and a healthy claim paired with transport cancellation.

How to Test

Run the focused cron shutdown suite:

HERMES_PYTHON=/Users/blockkonit./Dev/hermes/hermes-agent/.venv/bin/python scripts/run_tests.sh tests/cron/test_shutdown_interrupt.py

Result: 31 passed, 0 failed. Also run:

/Users/blockkonit./Dev/hermes/hermes-agent/.venv/bin/ruff check cron/scheduler.py tests/cron/test_shutdown_interrupt.py

Result: all checks passed.

Evidence

BEFORE RED: the new completed-response regression failed on the prior implementation: the pending heartbeat event made _FireOwnership.lost() return immediately, so output was never saved and terminal bookkeeping took the generic interruption path (1 failed, 1 control passed).

AFTER GREEN: the full focused file completed with 31 passed, 0 failed. The completed-response case saves the full report, writes a successful owner-fenced terminal status, and finishes the execution successfully after ownership revalidation.

CONTROL: an incomplete response whose owner no longer validates saves no output and does not call mark_job_run; its execution is recorded as a discarded stale result. A separate control confirms that a transport cancellation remains interrupted even when the claim heartbeat validates.

The change classifier enables Python, Python production, scan, Docker, and Nix lanes for this production Python change. Local Python tests and Ruff were run above. Docker publish is not a local PR verification step, and Nix is unavailable on this macOS worker; those lanes remain CI follow-up.

Checklist

• The issue is linked with Fixes #113357.
• The fix is limited to cron claim-loss bookkeeping.
• Completed results require a fresh fire-claim validation before terminal success is recorded.
• Stale or unvalidated ownership remains fenced out.
• Transport cancellation retains its interruption behavior.
• Regression coverage reproduces the completed long-running response path.
• Control coverage protects the incomplete stale-owner path.
• scripts/run_tests.sh tests/cron/test_shutdown_interrupt.py passes with 31 tests.
• Ruff passes for both changed Python files.
• No lockfile, generated artifact, or unrelated file is included.

[KeyArgo]

Independently verified against current main (4e9d3c713a3), which is also this PR's merge base. Everything below is from running your diff on a pristine checkout, not from reading it.

RED on base — your test file applied verbatim to 4e9d3c713a3:

tests/cron/test_shutdown_interrupt.py::TestFireClaimLossAfterCompletedRun::test_completed_response_is_preserved_when_ownership_revalidates
    assert saved == [True]
E   assert [] == [True]
1 failed, 2 passed, 28 deselected

The completed-response case fails exactly where you describe: no output saved, and the terminal bookkeeping takes the interruption path. The two control cases pass on base by construction (nothing written, nothing marked), which is what makes them controls.

GREEN with the fix — same tree plus the cron/scheduler.py hunk: 31 passed in 2.13s for the file. Both controls stay green, so the completed path is preserved without relaxing the incomplete-owner case.

The ordering assumption holds at the only production construction site. The split your helpers make is positional (_events[0] vs _events[1:]), and _run_one_job_body builds _CombinedCancelEvent(lost_ownership, cancel_event) at cron/scheduler.py:2502, so index 0 is always the heartbeat's lost_ownership event. Worth flagging for later: the constructor filters Nones but does not tag its sources, so a third cancellation source would silently join other_cancel_is_set() — fine today, fragile if that call site is ever reshaped.

Overlap map. This is adjacent to, but distinct from, the open #113283 (fix(cron): a delivered run keeps its ok status when the fire-claim sample misses after delivery, for #105861), which splits the post-delivery _fire_claim_ownership_lost() guard in the run wrapper. Your change is one level earlier — the fire_claim_lost.is_set() short-circuit at the top of _FireOwnership.lost() (cron/scheduler.py:2604-2606), which never revalidates against the store at all. The two do not duplicate one another; #108912 is the older ledger-side attempt at the same distinction. Adding Related: links would help whoever picks the carrier.

What stays open. The change makes the bookkeeping honest for completed runs; it does not establish why the heartbeat saw a loss on a 90-100s run with one gateway, a 300s TTL and a 180s grace. If the claim really is being re-owned or cleared mid-run by the worker-scope lifecycle, runs that are fenced during a side effect will still be recorded as lost — correctly, but invisibly. As a follow-up, the heartbeat's failure branch (cron/scheduler.py:2401-2410) logs only that ownership was lost; logging the owner the store actually reported next to the expected one would make the next occurrence diagnosable from the log alone.

[ehz0ah]

Blocking: the completed-run regression does not exercise the claim-loss branch reported by #113357. The heartbeat sets the loss event after heartbeat_fire_claim returns false because the stored claim is missing or owned by another runner. The new test instead sets the event inside run_job and makes every durable revalidation return true. It proves recovery from a stale signal, but not recovery from the reported owner mismatch. I ran the same completed-response path on this exact head with heartbeat_result=False. It saved no output, did not call mark_job_run, and finished with Fire claim ownership lost; stale result was discarded. The reported completed run therefore still ends as a false failure while the stored claim remains missing or replaced. Please add a regression for that path and fix the lifecycle operation that clears or re-owns the live claim. If this PR is intentionally limited to a transient heartbeat signal that later revalidates, narrow the issue link and do not close #113357 as fixed.

[kshitijk4poor]

Closing as merged via salvage: @teknium1 landed this in #114837 (eafed27cf0, "a completed run keeps its result when a fire-claim heartbeat sample misses", crediting this PR), on top of 132a181d80. #105861 is closed as fixed. Thank you.