fix(cron): keep a delivered run's success through a transient post-delivery claim blip (#105861)

[PRATHAMESH75]

Problem (#105861)

Agent-mode cron jobs hold a durable fire claim a heartbeat re-validates every 60s. After the save/compose/deliver phase, _run_one_job_body short-circuited to _record_fire_ownership_lost whenever:

if d.side_effect_ownership_lost or _fire_claim_ownership_lost():
    _record_fire_ownership_lost(job["id"], fire_owner, execution_id)
    return True

_fire_claim_ownership_lost() (fence.lost()) is a sampled flag that flips permanently on a single transient missed heartbeat tick (disk latency, a concurrent jobs.json rewrite, a brief spawn spike). So a run that had already delivered its output got last_status overwritten with Interrupted by shutdown before terminal completion. — turning a healthy job's history into error and firing false watchdog alerts on every subsequent tick, while the message had in fact been sent.

The reporter observed this 3× in one day (~12% of that day's agent runs), all independently verified delivered, zero duplicates.

Confirmed in _record_fire_ownership_lost itself: it only writes the error if ... heartbeat_fire_claim(job_id, expected_owner=fire_owner) returns True — i.e. it wrote the interruption because the claim was still held. The fence.lost() sample and the atomic heartbeat disagreed; the sample was the stale one.

Fix

The post-delivery interruption decision is now _delivery_phase_interrupted(d), True only when the claim was lost during the side effect (_save_compose_deliver raised _FireClaimLostDuringSideEffect, so delivery did not complete under a held claim). A completed delivery falls through to _finish_completed_run, whose owner-fenced mark_job_run(expected_fire_owner=...) is the authoritative check:

• claim genuinely lost at mark time → mark_job_run returns False → records the real ownership loss;
• claim still held → records the real last_status: ok.

This removes the redundant, sampled post-delivery check without weakening any guarantee:

• Duplicate-delivery is still guarded by the pre-delivery fence inside _save_compose_deliver (side_effect_fence() → raises _FireClaimLostDuringSideEffect), which is unchanged.
• Gateway shutdown is signaled by _consume_interrupted_flag (a separate per-execution flag), not this fence, so the shutdown path is unaffected.
• _fire_claim_ownership_lost() is still used for the pre-run ownership check earlier in the body.

Tests

tests/gateway/test_cron_delivered_success_survives_claim_blip.py: a completed delivery is not an interruption (the regression), a loss during the side effect still is, a normal delivery error is not an ownership interruption, the predicate takes no sampled-ownership input, and the wrapper routes through _delivery_phase_interrupted (guards against re-adding the disjunction).

Fixes #105861

[alt-glitch]

This was generated by AI during triage.

Related: this joins the open fix cluster for #100401 (same _record_fire_ownership_lost overwrite of a delivered run): #95432, #97565, #100418, #100965, #104286. This PR removes the sampled post-delivery ownership check and defers to the owner-fenced mark_job_run; #100965 instead raises an explicit unavailable-fence condition. Reviewers should compare and consolidate.

[kshitijk4poor]

Heads-up: the root cause of the 'sampled lost flag flips on a transient tick' behaviour landed on main via #109310 (merge 9a60a7f). The tick that flipped fence.lost() after a delivery was the heartbeat thread timing out on the fire fence its own run holds — not disk latency — and heartbeat_fire_claim no longer takes that fence, so False now means only a real owner change. Please re-check #105861 against current main; if a post-delivery false loss still reproduces, this PR's scheduler-side change is the right next layer, otherwise it may be redundant.

[PRATHAMESH75]

Closing this as superseded — thanks @kshitijk4poor for the heads-up, and for landing the actual root-cause fix in #109310 (9a60a7f).

I re-checked #105861 against current upstream/main. This PR guarded _run_one_job_body's post-delivery tail (now cron/scheduler.py:2943) so a transient _fire_claim_ownership_lost() reading wouldn't overwrite an already-delivered success. But that reading can no longer flip transiently: _FireOwnership.lost() re-validates through heartbeat_fire_claim, and #109310 moved heartbeat_fire_claim out from under _under_fire_fence (cron/jobs.py). The run thread holds the per-job fire fence across _save_compose_deliver, and the fence's reentrancy is per-thread, so pre-fix the heartbeat (and the synchronous lost() re-check) timed out on the run's own fence and reported a false loss. With the heartbeat now serialized only by _jobs_lock, a post-delivery False/lost result means a genuine owner change, not disk latency or a spawn spike — so honoring it at :2943 is correct, and this PR's scheduler-side layer is no longer needed.

That matches your read: the fix belongs at the ownership-check source, not as a downstream exception in the bookkeeping tail. Maintainers may want to keep #105861 open until they've confirmed the P1 symptom is gone on main, but there's nothing left for this PR to add. Closing.