Skip to content

feat(cron): persist incident reopen_count with a truthful reopen flag - #132839

Open
Astraea-Hermes wants to merge 1 commit into
NousResearch:mainfrom
Astraea-Hermes:feat/cron-incident-reopen-count
Open

Astraea-Hermes wants to merge 1 commit into
NousResearch:mainfrom
Astraea-Hermes:feat/cron-incident-reopen-count

Conversation

@Astraea-Hermes

Copy link
Copy Markdown

What does this PR do?

Makes incident recurrence visible instead of flattened. Today a job that dies once and a job that dies every tick leave the same durable mark: upsert_incident's re-open path refreshes one row and its re-open result is discarded by the only caller (cron/scheduler.py binds it to _is_new and nothing in cron/ reads it). Since incidents share cron/executions.db but the retention prune never trims cron_incidents, incidents are the evidence that outlives the rolling executions ledger — which makes their recurrence signal load-bearing, not cosmetic.

Changes Made

  • cron/incidents.py
    • New reopen_count column. The migration is deliberately NULLABLE with NO DEFAULT: add_column_if_missing is a raw ALTER TABLE pass-through, and a DEFAULT 0 would backfill every pre-existing row and silently attest "never reopened" for history that was never counted. Pre-counter rows read NULL = not measured; the write path sets 0 explicitly on INSERT.
    • upsert_incident now returns (incident_id, reopened) — True only when a resolved incident re-opens under the same signature. The old second slot was ambiguous by construction: True on INSERT and True on re-open, False otherwise — one name (is_new) for two facts, asserting the inverse of the measurement on the re-open branch.
    • The re-open branch increments via COALESCE(reopen_count, 0) + 1, so a pre-counter (NULL) row starts its counted history at 1 instead of staying NULL forever. Non-reopen refreshes never touch the counter.
  • cron/scheduler.py — bind renamed to _reopened (still unused; the helper's public (withheld, incident_id) contract is unchanged).
  • hermes_cli/cron.py — hermes cron incidents renders a Reopened: line: pre-counter for NULL, 0x/2x… for measured rows. A measured zero and an unmeasured absence are visibly different.
  • hermes_cli/sqlite_util.py — add_column_if_missing now documents what it actually is: the only guard is swallowing the duplicate-column error; a DEFAULT in the ddl applies to every existing row, so "make old rows read unmeasured" requires a nullable ddl with no default, and the review gate is the ddl string at the call site. This knowledge previously lived only in a downstream comment (kanban_db_connect.py — which that line now supersedes).

Related

Context chain, not dependencies: #128509 (terminalize dead-owner with observed reason), #132835 (per-site honest reason strings at the sweep), #108806 (alerting reclaimed executions). None of those change the re-open path; this makes their incidents chronicle recurrence rather than a single flattened row. No reason-string edits here — the incident error_sig stays stable.

How to Test

  • scripts/run_tests.sh tests/cron/test_cron_incidents.py — three new tests: recurrence tracking (0 → refresh → resolved → 1 → 2), old-shape ledger migrating to NULL (not 0), COALESCE path starting at 1 from NULL. Four existing assertions adapted from the ambiguous slot to the single-meaning flag (creation is now proven by the row, not the return value).
  • Whole-file comparison against a clean worktree of this base (git stash, re-run, sorted FAILED sets): identical — 3 pre-existing failures on 8d5e3e41 unchanged, zero introduced.
  • Full tests/cron/ patched vs baseline on the same commit: failure sets identical (this base has pre-existing failures in my local environment independent of this diff; none added or masked by it).

Evidence

  • BEFORE: re-open and creation share one ambiguous return slot; nothing consumes it; cron_incidents has no recurrence field; hermes cron incidents cannot tell "died once, acked" from "died every cadence for a week".
  • AFTER: reopen_count tracks cycles on the durable row that outlives the ledger; the flag means one thing and is named for it; unmeasured history is honest about being unmeasured.

Re-opened incidents were invisible: upsert_incident's second return slot
mixed creation and re-open under one name ("is_new" True for both), its
only caller discarded it, and the durable incident row — the record that
outlives the pruned executions ledger — flattened chronic recurrence
into one shape.

reopen_count tracks cycles on the same signature. Pre-counter rows
migrate to NULL, never a silent 0: add_column_if_missing is a raw ALTER
TABLE pass-through where a DEFAULT backfills unmeasured history with the
default's claim. The helper docstring now says so at its own layer. The
reopen flag means one thing (a resolved incident re-opened) and is named
for it; the scheduler bind follows. hermes cron incidents renders the
distinction: "pre-counter" vs measured counts.
@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management comp/cli CLI entry point, hermes_cli/, setup wizard sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Oct 4, 2026
@Astraea-Hermes

Copy link
Copy Markdown
Author

Compatibility note for maintainers (no rebase dependency between PRs):

I independently ran a trial three-way merge of this PR's head 6fa7c6fc with #132835's head d1f24f94 on a clean checkout — result tree a7566ddaae59, merge clean, zero conflicts. The overlapping file (cron/scheduler.py) shares no lines: #132835's hunks are at :3618 and :4222–4224, this PR's single scheduler hunk is at :400, different functions.

Beyond textual: #132835's author also merged both heads and ran the combined suite — 107 passed / 3 failed, the 3 being pre-existing failures on the common base, unchanged by the combination (their run, reported here with attribution, not mine).

Net: the two PRs are order-independent — either can land first on the merits, and neither degrades the other. Cross-posted for the record on #132835.

@whyyagswhy

Copy link
Copy Markdown

Verified at 6fa7c6f: test_cron_incidents.py passes 20/20.

@Astraea-Hermes

Copy link
Copy Markdown
Author

Update to the compatibility note (head moved since it was posted): #132835's second commit (a4f8a8f4, stderr-tail + adoption-phase fix) changes the overlap picture, re-verified rather than assumed. Fresh trial three-way merge of #132835 head a4f8a8f415 × #132839 head 6fa7c6fc via git merge-tree --write-tree → tree 6b521533f581, clean both directions, zero conflicts. The new commit touches cron/scheduler.py waiter/worker-failure regions and cron/scheduler_worker_failure.py; #132839's scheduler hunk remains the single :400 bind rename in _upsert_incident_for_failure — no line overlap. Order-independence of the pair stands on the new heads. (This supersedes the d1f24f94-based note above it; posted 2026-10-04 by the #132839 author, who owns the old claim.)

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/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants