fix(cron): avoid self-contention in fire-claim heartbeat

[codeshipsingh]

Summary

• Fix recurring false ownership-loss cancellations caused by self-contention between delivery-side fire fence lock and heartbeat renewal.
• Add process-wide fence-held depth tracking.
• In heartbeat renewal, when this process already holds the fence (or fence acquisition fails), return owner-read path instead of failing closed.
• Keep delivery outcome persisted when ownership is lost during save/deliver paths.

Why

• Long delivery phases could exceed lock timeout and cause heartbeat renewal to fail on our own lock, triggering erroneous run cancellation.

Tests

• Added tests/cron/test_fire_fence_heartbeat_contention.py (2 invariant tests).
• Verified:
  • scripts/run_tests.sh tests/cron/test_fire_fence_heartbeat_contention.py tests/cron/test_script_claim_heartbeat.py -q

Notes

• Known unrelated existing test issue remains outside this PR scope: tests/cron/test_file_permissions.py::test_ensure_hermes_home_sets_0700.

[alt-glitch]

This was generated by AI during triage.

Related: this is the fifth open fix for the fence-contention-read-as-ownership-loss bug alongside #95432, #97565, #100965 and #102627. This PR uses process-wide fence-held depth tracking; the others use a tri-state heartbeat result. Flagging so a maintainer can pick one approach.

[ricanwarfare]

Independent verification on a production install — this approach is correct, and it fixes the real failure

Not a duplicate: I came at this from a live incident and built an independent regression suite
before finding the PR queue. Tested this branch in a scratch worktree against my suite. Summary:
the mechanism here is right, and it fixes the failure that the tri-state PRs fix only partially.
Two findings worth a maintainer's attention.

What I verified

Reproduced on a production gateway (Linux, single gateway process, ~37 active jobs) with a real
failure the tri-state approach does not address. Fetched this branch into a clean worktree and ran
an independent regression suite (5 tests) against it:

• ✅ contention does not cancel a healthy run
• ✅ contention does not interrupt the run body's cancel event
• ✅ genuine ownership loss still returns False (takeover still detected)
• ✅ happy path still refreshes
• ✅ non-heartbeat callers keep the fail-closed False contract
• ⚠️ one assertion mismatch: heartbeat_fire_claim returns True (renewed via the unfenced CAS)
  where my suite expected a distinct "unknown" state (None)

That last one is a design difference, not a defect. Renewing through the same-owner CAS without the
fence is sound: fire_claim is written under the jobs lock, a takeover needs the fence, and the CAS
refuses a changed owner. Returning True also keeps the claim's TTL fresh across a delivery longer
than FIRE_CLAIM_TTL_SECONDS — which the None-returning tri-state PRs (#95432, #97565, #100965,
#102627) do not do, because they route None to the grace path without renewing. For a delivery
that outlives the TTL, this approach is strictly more correct than the tri-state ones.

Finding 1 — the tri-state PRs return a value that breaks their own callers

Worth flagging for whoever picks the winner. The tri-state PRs change the signature to
Optional[bool] and return None on fence contention, but _fire_job_lock still yields a plain
False, so every other caller of heartbeat_fire_claim receives None unless each call site is
individually updated. Two call sites in cron/scheduler.py are easy to miss:

• the initial pre-execution ownership check — if owns_fire_claim is False: becomes dead, so a
  contended startup falls through and starts the run on unverified ownership
• _record_fire_ownership_lost — if fire_owner is not None and heartbeat_fire_claim(...) treats
  None as falsy, silently taking the "stale result was discarded" branch

I hit exactly this in my own first cut. It is why I split the helper rather than changing the return
type in place. If a tri-state PR is chosen, those call sites need explicit is None handling.

Finding 2 — the terminal-status mislabeling is still present in this branch

This is the part I could not get anyone else's PR to cover, and it is the operator-visible half of
the bug. On this branch, _record_fire_ownership_lost still does:

if fire_owner is not None and heartbeat_fire_claim(job_id, expected_owner=fire_owner):
    mark_job_run(job_id, False, _OWNERSHIP_LOST_INTERRUPTED, expected_fire_owner=fire_owner)

The branch does improve the string (to "Interrupted before terminal completion after fire-claim
renewal failed (no gateway shutdown was recorded).") and does preserve delivery_outcome on the
execution row — both good. But mark_job_run(..., False, ...) still overwrites a successful,
already-delivered run's last_status with an error. Since this branch fixes the contention
trigger, that path becomes much rarer — but it remains reachable via a genuine loss during delivery,
and when it fires the operator still sees a false failure for a run whose output was sent.

#105882 addresses precisely this (_delivery_phase_interrupted), and I believe the ideal
outcome is this branch's contention mechanism combined with #105882's post-delivery exemption —
after _save_compose_deliver completes without _FireClaimLostDuringSideEffect, the claim is
bookkeeping only and should not adjudicate a completed delivery.

Production evidence for the record

The real incident, for the maintainers' timeline:

• 8badc15ed104 (Morning News Briefing): output written (6,558 chars, complete), delivery recorded
  delivered in deliveries.db, gateway log shows Cron output preserved for chunking adapter (6537 chars) at 07:04:06 — and last_status=error, "Interrupted by shutdown before terminal completion." No shutdown occurred; the next SIGTERM was 6.5 hours later.
• Delivery held the fence 07:03:07 → 07:04:07 = 60s against _JOBS_LOCK_TIMEOUT_SECONDS = 30.0.
  Deterministic, not flaky.
• Same signature on 6d1817eadd57 (50s hold). Nine Timed out waiting for local fire fence events
  in the error logs since Sep 3; 63 of the last 400 deliveries took ≥30s, so the precondition is
  routine.

Corroborates #100401 (which already narrowed the trigger to slow delivery rather than long runtime —
correct; my 60s measurement is an independent confirmation of that 40s bot-chat measurement).

Suggested resolution

1. Pick this branch's mechanism (or equivalently, add fence-held depth tracking) for the contention
   fix, so the TTL stays fresh through long deliveries.
2. Fold in fix(cron): keep a delivered run's success through a transient post-delivery claim blip (#105861) #105882's post-delivery exemption so a completed delivery can never be re-adjudicated as
   an interruption.
3. Add explicit is None handling at the two call sites above if a tri-state variant wins instead.

Happy to rebase my regression suite onto whichever branch is selected if useful — the 5 tests above
are self-contained and assert behaviour, not implementation.

[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).

Thanks @codeshipsingh, and @ricanwarfare for the independent production verification — the diagnosis was correct. The landed fix removes the fence from the heartbeat outright, so the process-wide held-fence map and the _OWNERSHIP_LOST_INTERRUPTED message change are not needed. Closing with credit.