Skip to content

fix(cron): an ownership loss is named as one, not as a shutdown that never happened - #116463

Open
kleros109 wants to merge 1 commit into
NousResearch:mainfrom
kleros109:persist/cron-ownership-loss-label
Open

kleros109 wants to merge 1 commit into
NousResearch:mainfrom
kleros109:persist/cron-ownership-loss-label

Conversation

@kleros109

Copy link
Copy Markdown

What does this PR do?

_record_fire_ownership_lost() writes "Interrupted by shutdown before terminal completion." into last_error and the executions ledger. That wording is now wrong for every path that reaches it.

Before #105861 (132a181d80, merged Sep 17) the guarded condition could also fire on an unconfirmed renewal, so a shutdown-flavoured message at least had a plausible subject. After it, the function is reached only from store-serialized evidence — a refused side-effect fence, a real transport cancel, or a terminal write the store refused. None of those is a shutdown. The gateway-shutdown wording belongs to _finish_interrupted_run, which is now its only genuinely shutdown-scoped writer.

At the same time the codebase already uses the honest phrasing one branch over: _finish_completed_run's CAS-refused path writes exactly "Fire claim ownership lost before terminal completion.". So one failure shape was being recorded under two different labels, and the misleading one is the one that lands in last_error and the ledger.

Practical cost (Sep 2026): 28 executions across 14 jobs carried the shutdown string while their sessions all ended cron_complete and their output was delivered, so an operator reading last_error was sent looking at gateway restarts for a condition that is not a restart, and the failure streak/streak nudge accumulated on jobs that never failed.

Changes Made

  • cron/scheduler.py: _OWNERSHIP_LOST_INTERRUPTED → "Fire claim ownership lost before terminal completion.", aligning it with the string _finish_completed_run already writes; docstring states that an ownership loss is its own failure shape and that shutdown wording belongs to _finish_interrupted_run alone.
  • website/docs/developer-guide/cron-internals.md: the fire-claim section quoted the old literal verbatim; updated to the new string and to the accurate set of paths that reach it.
  • tests/cron/test_fire_claim_outcome_recording.py (new): pins both directions.

How to Test

scripts/run_tests.sh tests/cron/test_fire_claim_outcome_recording.py
# 2 passed

scripts/run_tests.sh tests/cron/
# 133 files, 1508 passed, 0 failed, 1 skipped

The new file uses a real jobs.json + a real execution ledger in temp dirs (no mocks for the bookkeeping):

  1. test_delivered_run_is_recorded_from_its_own_outcome — a renewal that cannot be confirmed during a long delivery must not turn an already-delivered run into a recorded failure: execution completed, delivery_outcome: delivered, last_status: ok, no failure streak.
  2. test_refused_side_effect_fence_is_still_recorded_as_interrupted — the protection stands: a refused fence does not deliver, the execution is failed, and the recorded error names the fire claim and does not say "shutdown". This is the assertion that is red before this PR (AssertionError: Interrupted by shutdown before terminal completion.).

Duplicate check — read this before merging

Searched open and merged PRs for "fire claim" / ownership-loss ledger wording. #108912 ("fix(cron): distinguish fire-claim ownership loss from shutdown in ledger", open since 2026-09-12) works the same area with a broader, different approach. Disclosure, not concealment:

This PR is the remaining narrow slice: one constant, so an ownership loss is never labelled a shutdown. If maintainers prefer the #108912 shape, close this one — the underlying defect is what matters. Happy to fold it in instead if that PR is revived.

Related Issue

Fixes # (no issue filed; found by auditing recorded cron failures against their sessions)

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✅ Tests (adding or improving test coverage)
  • 📝 Documentation update

Checklist

  • I've read the Contributing Guide
  • Conventional Commits (fix(cron):)
  • Searched open and merged PRs — overlap with fix(cron): distinguish fire-claim ownership loss from shutdown in ledger #108912 disclosed above
  • Only changes related to this fix
  • scripts/run_tests.sh green (full tests/cron/ area)
  • Tests added; the refused-fence assertion is red on base
  • Tested on Linux (7.2.5-4-omarchy, Python 3.11)
  • Documentation updated in the same change (cron-internals.md quoted the literal)
  • No config keys touched

…never happened

_record_fire_ownership_lost wrote "Interrupted by shutdown before terminal
completion." into last_error and the executions ledger. Before 132a181 the
disjunction it guarded could also fire on an unconfirmed renewal, so the
shutdown wording at least had a plausible subject. After 132a181 the function
is reached only from store-serialized evidence -- the refused side-effect fence,
a transport cancel, or a terminal write the store refused -- none of which is a
shutdown. An operator reading last_error was sent looking at gateway restarts
for a condition that is not a restart (Sep 2026: 28 executions across 14 jobs
carried it while their sessions ended cron_complete).

The message now names the fire claim. The gateway-shutdown wording stays with
_finish_interrupted_run, its only genuinely shutdown-scoped writer, so the two
failure shapes are no longer conflated. The cron-internals doc quoted the old
literal verbatim and is updated in the same change.

Tests: tests/cron/test_fire_claim_outcome_recording.py pins both directions --
a delivered run whose renewal could not be confirmed is recorded from its own
outcome (completed / delivered / no streak), and a refused side-effect fence is
still recorded as interrupted, naming the fire claim and never a shutdown.
@kyssta-exe

Copy link
Copy Markdown

Review — fix(cron): ownership loss label

Summary: This renames the misleading _OWNERSHIP_LOST_INTERRUPTED ledger/last_error string from shutdown wording to "Fire claim ownership lost before terminal completion," matching the label _finish_completed_run already uses. The new test pins both directions (delivered runs keep their own outcome; refused fences still record failure), and the internals doc is updated to match. Narrow, well-scoped, and honest about the #108912 overlap.

What changed

  • cron/scheduler.py: one constant's message + clarifying docstring on _record_fire_ownership_lost.
  • website/docs/developer-guide/cron-internals.md: fire-claim section updated to the new literal and the accurate path set.
  • tests/cron/test_fire_claim_outcome_recording.py (new): real jobs.json + ledger, no mocks for bookkeeping; red-before assertion on the old string.

Strengths

  • The red-before/green-after test design is exactly right for a label bug — test_refused_side_effect_fence_is_still_recorded_as_interrupted fails on base with the old literal.
  • The first test guards the scarier regression (delivered run rewritten as failure + failure-streak pollution), which is the actual operator pain cited (28 executions / 14 jobs).
  • Disclosing fix(cron): distinguish fire-claim ownership loss from shutdown in ledger #108912's conflict and its landed-superseded status in the body saves maintainers triage time.

Findings

  • cron/scheduler.py:2592 — the identifier _OWNERSHIP_LOST_INTERRUPTED still carries the old shutdown vocabulary even though the message no longer does. Non-blocking, but a rename to e.g. _OWNERSHIP_LOST would stop the next reader from re-inferring shutdown semantics.
  • Coordination note (not a code issue): fix(cron): distinguish fire-claim ownership loss from shutdown in ledger #108912 is CONFLICTING in the same area — whoever lands second needs a rebase, and this PR's one-constant shape should rebase trivially.

Verdict: Looks good to merge.

Reviewed using Hermes-Agent

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

comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants