fix(cron): fence contention no longer reads as fire-claim ownership loss

[BrunoBza]

What does this PR do?

Fixes #95307 — finite cron jobs (once / repeat-limited) delivering to bot-chat complete their agent turn and then get recorded as failed with Fire claim ownership lost; stale result was discarded., losing the result entirely. The root cause is a semantic conflation in the fire-claim ownership probe; this PR separates "definitively lost" from "could not verify" so the runner stops discarding its own successful result.

Bug cause

heartbeat_fire_claim() returned False for two different facts:

1. the claim is gone or now belongs to another owner (a definitive takeover), and
2. the per-job fire fence could not be acquired within \_JOBS_LOCK_TIMEOUT_SECONDS — pure lock contention, with no information about who owns the claim.

Those are not interchangeable. The fence that blocks the refresh is the very lock that excludes replacement owners while the legitimate runner performs a fenced side effect. A bot-chat delivery runs an entire child-agent turn (hermes chat -Q ...) while holding that fence, so heartbeat/probe calls issued during delivery contend with the runner's own fence. Reading that as ownership loss makes the runner discard its own completed result. Deliveries to local / ordinary platforms finish in well under a second and never straddle the probe window, which matches the reporter's isolation matrix exactly.

On the current main, mark_job_run(expected_fire_owner=...) already CAS-guards terminal writes and the post-run probes run before finalization — so the reporter's proposed fixes #1/#2 are largely landed upstream. What remains broken is exactly the conflated False: any caller that cannot acquire the fence is told "you lost the claim" when the truth is "I don't know".

Fix

heartbeat_fire_claim() becomes tri-state:

Result	Meaning
True	claim exists, belongs to expected_owner, refreshed
False	definitive loss: job/claim gone, or owner replaced
None	fire fence busy — ownership unknown, no evidence either way

Scheduler consumers updated accordingly:

• Pre-run validation: None closes the execution row with the existing "Fire claim ownership could not be validated before execution started." record (same treatment as a transport exception) instead of reporting a loss.
• Heartbeat loop: a None beat is skipped without consuming the exception grace budget; refreshes resume once the fence frees up. Only a definitive False sets the ownership-lost event.
• Post-run probes (\_fire_claim_ownership_lost): None is treated as not lost, and the run proceeds to its owner-fenced terminal write — mark_job_run(expected_fire_owner=...) remains the authoritative last-line guard against completing on a stolen claim.
• Shutdown-interrupt re-probes: only take the fenced "interrupted by shutdown" branch when ownership is positively confirmed (is True).

False still means loss everywhere: a real takeover keeps interrupting stale runs and suppressing stale deliveries exactly as before.

Testing

New regression suite tests/cron/test_fire_claim_contention_95307.py (behavior-contract, real store under a temp HERMES_HOME, no snapshot tests):

• tri-state store contract: busy fence ⇒ None; uncontended ⇒ True; wrong owner ⇒ False
• pre-run busy fence finishes the row as "could not be validated", body never runs
• definitive loss at startup still reports the distinct "ownership lost" record
• post-delivery bookkeeping with a busy fence completes successfully instead of discarding
• a genuine mid-run owner change is still detected as a discard (guards against over-correction)

All six fail on pristine main (three of them reproduce the exact reported ledger error string); all pass with the fix.

Neighbor suites: test_claim_job_for_fire.py, test_script_claim_heartbeat.py, test_shutdown_interrupt.py, test_run_one_job.py, test_cron_bot_chat_delivery.py, test_dead_owner_claim_reclaim.py, test_inflight_stale_guard.py, test_cron_run_stale_claim_reap_86721.py — 116 passed. Full tests/cron/: 971 passed, 1 skipped, plus one pre-existing environment failure in test_cron_created_delivery.py that fails identically on pristine main (unrelated to this change). tests/gateway/test_restart_resume_pending.py: 36 passed.

Related

• Cron fire-claim TTL (300s) expires mid-run, allowing a second concurrent execution of a long job #88507 (fire-claim flat TTL — same subsystem, opposite symptom), Cron/session recovery: fence reconciliation by exact ownership generation #88688 (fence reconciliation by ownership generation — the long-term primitive)
• The now-closed fix(cron): deliver finite bot-chat jobs without false ownership loss #95334 diagnosed the same failure mode; its approach patched around the conflation at the wrapper level, while this fix removes it at the source so every consumer benefits.

[Enough1122]

AI code review — automated review for reference; please use your judgment.

1. cron/scheduler.py (~L6704, pre-run validation branch) — A busy fence at startup now records the execution as success=False ("could not be validated"), i.e. a transiently contended moment (another process mid-delivery on a sibling execution, GC pause inside the lock budget) permanently closes the row as a failed run that never ran. Why it matters: operationally this is indistinguishable-in-dashboards noise from real failures, and for finite one-shots a scare at the wrong instant loses the run record entirely. The mid-run path deliberately treats None as proceed — startup applies the opposite (strict) policy with no retry. Suggestion: consider a small bounded retry (e.g. 3 validation attempts over a few heartbeats) before recording could not be validated, or classify the error as retriable so the scheduler requeues — the current tests pin the record-as-failure behavior, so this should be a deliberate choice, not a default.

2. cron/scheduler.py (_fire_claim_ownership_lost, status is None branch) — The mid-run lenient path's safety net is "the owner-fenced terminal write" being the authoritative guard against completing on a stolen claim — but no test exercises the terminal write itself contending with the fence (the one place where the guarded write is the only protection left). Why it matters: the whole design now leans on that single fence as the final arbiter; if it also fails-open on contention, a real takeover during a fenced delivery could still double-complete. Suggestion: one test where a definitive takeover lands while the terminal write's fence busy/timeout path is exercised, asserting single completion.

3. tests/cron/test_fire_claim_contention_95307.py — The timing constants are tight: _RUN_CLAIM_HEARTBEAT_SECONDS = 0.05, _JOBS_LOCK_TIMEOUT_SECONDS = 0.05, and test_run_completes_when_delivery_spans_heartbeat_beats relies on several heartbeat beats landing inside a time.sleep(0.3) window while a threaded lock is held. Why it matters: on a loaded CI worker a 0.3 s wall sleep is not a guarantee the heartbeat thread gets scheduled at all — this reads as a flake farm. Suggestion: drive beats explicitly where possible (event-synced like the takeover test) or widen margins (sleep 1.0s+) — slow is better than flaky.

The tri-state design itself is right: contention-as-None matches the fence's actual semantics (it's the same lock that already excludes replacement owners), the old truthiness call sites now use explicit is True / is False checks so None can never silently downgrade to "lost", and the takeover/regression tests are well-constructed. Item 1 is the only behavioral question; 2-3 are hardening.

[kshitijk4poor]

The fix for this bug landed on main via #109310 (merge 9a60a7f): heartbeat_fire_claim now refreshes under _jobs_lock only and no longer waits on the per-thread fire fence its own run holds, so a delivery/agent turn longer than the 30s fence timeout is no longer misread as ownership loss. _refresh_claim still CASes on claim["by"], so a real takeover keeps returning False (pinned by test).

Thank you for the earliest report and fix of this bug (#95307). The landed change takes the smaller route — dropping the fence from the heartbeat instead of a tri-state result — which also resolves the startup-branch concern raised in the review here (a busy fence can no longer make the pre-run heartbeat return False). Closing in favour of the merged fix; your diagnosis is credited in the PR body.