Repository navigation
Bound negentropy history and alert on stalled durable progress - #66
Conversation
erskingardner
left a comment
There was a problem hiding this comment.
| Review metadata | Value |
|---|---|
| Reviewed at (UTC) | 2026-10-10T19:05:37Z |
| Commit reviewed | f1284c01676dbf82fa2c54420c3b7ea36f6cde49 |
| Model | claude-opus-5-5 |
| Reasoning level | Not exposed by runtime |
| Recommended action | Resolve serious concerns before merge |
I found no correctness bugs in the schema 9 migration, the ID allocator, or the compaction transaction. Rollback, deferred-FK tree deletion, receipt and exclusion pinning all look right, and cleanup correctly bypasses the admission budget. My concerns are about one design choice and about scope:
- Sweep-level pinning weakens the retention bound. A single permanently
blockeddense second pins every sweep that revisits it. That's about 43k rows per relay before the planner's root cap stops the relay. Per-tree pinning already protects lineage and proofs. - The Kuma push path may be unnecessary. Archive admission already uses a Kuma HTTP monitor on the same metrics port. A second HTTP monitor would replace the script, the service/timer, the credential handling and the tests (~175 lines).
- The docs are spread out.
negentropy_retention_activation.mdis a session log that repeats the readiness doc. One readiness bullet now describes as future work what this PR already does. - Smaller: the
persistent_failureprobe scans all ofjobson every tick; an indexed form is suggested inline.RetentionProgressis computed but never surfaced.
CI: rustfmt, clippy, doctests, ops tests and test shards 1/3 and 3/3 are green. Shard 2/3 was still running when I posted this.
| // unresolved root/child must retain its full detailed lineage. | ||
| let open: bool = tx.query_row( | ||
| "SELECT EXISTS(SELECT 1 FROM planner_relays WHERE sequence=?1) | ||
| OR EXISTS(SELECT 1 FROM jobs WHERE sweep=?2 AND state!='complete')", |
There was a problem hiding this comment.
Pinning the whole sweep when any job in it is unresolved turns one stuck window into ~1,344+ retained rows. A dense one-second blocked leaf is permanent. Every later sweep revisits that window while it stays inside the 14-day horizon, so each of those sweeps is pinned as well. That continues until the planner's 32-unresolved-roots-per-relay cap stops the relay: roughly 32 × 1,344 ≈ 43k rows per relay against the shared 100k max_jobs. Two or three relays in this state exhaust capacity, even though every other window completed.
The per-tree checks below already protect split lineage, receipts, exclusion proofs and policy identity. The current-sweep check on the line above already covers planner appends. Compacting completed sibling roots doesn't touch the unresolved tree, and it doesn't claim coverage for the gap, because only compacted roots set bitmap bits. I suggest dropping this OR EXISTS(... sweep=?2 AND state!='complete') clause and the sweep_states index that exists only to serve it. If there's a concrete hazard that sweep-level pinning prevents and per-tree pinning doesn't, please document it.
| pending = Some(last_success.unwrap_or(activated)); | ||
| } | ||
| let persistent_failure = self.db.query_row( | ||
| "SELECT EXISTS(SELECT 1 FROM jobs WHERE state='blocked' |
There was a problem hiding this comment.
In the normal case (nothing failing) this EXISTS scans the whole jobs table. EXPLAIN QUERY PLAN shows SCAN jobs because the OR/!= terms can't use an index. It runs on the owner thread every 5 s tick, across up to 100k rows. The PR asserts indexed plans for its other new queries, but this one isn't checked.
If you list the unresolved states explicitly, SQLite uses a MULTI-INDEX OR over due_jobs(state,…) (I checked this against the same index definitions):
SELECT EXISTS(SELECT 1 FROM jobs WHERE state='blocked'
OR (state IN ('queued','leased','awaiting_durability','retry_wait') AND attempt>=3))It's worth adding this query to allocator_overflow_does_not_mutate_jobs_and_retention_uses_indexes too.
| Command::Lifecycle { reply } => { | ||
| let result = (|| { | ||
| let now = clock(); | ||
| ledger.retain_completed(now)?; |
There was a problem hiding this comment.
The RetentionProgress result is thrown away here, and nothing else in the runtime reads it. That means the new pub struct and its pub use in jobs.rs only serve tests. The activation doc says the trial must report retained-job growth, but nothing exports retention activity or the retained-row count, even though progress_observation already computes that count. Either emit it (for example a negentropy_retention_removed_jobs_total counter and a retained-jobs gauge), or have retain_completed return a plain u32 and drop the struct.
| `/usr/local/libexec/pensieve-negentropy-kuma.py` and the matching service/timer only | ||
| after the release gates. Store the new push URL, mode 0600 root-owned, in | ||
| `/etc/pensieve/negentropy-kuma.url`; systemd passes it with `LoadCredential`. | ||
| Create a **separate** Kuma push monitor with a 180-second missing-heartbeat deadline |
There was a problem hiding this comment.
Archive admission is monitored by a Kuma HTTP monitor that polls the same metrics port (docs/archive_failure_recovery.md, ops/RUNBOOK.md). /health/negentropy is served by that same listener. A second HTTP monitor that accepts only 200 gives the same alerting without any of these: negentropy-kuma.py, the service/timer units, the root-owned credential file, the status.ipf.dev host pin, or test_negentropy_kuma.py (~175 lines total). An HTTP monitor also fails on its own when the host or process dies, with no heartbeat logic. As written, the oneshot also exits 1 on every unhealthy sample, so systemd marks the unit failed every 30 s during an incident.
If Kuma really can't reach port 9091 (meaning monitor 45 isn't an HTTP monitor after all), please say so here. Otherwise I'd drop the push path.
| monitor; the explicit disabled response is not HTTP success. Keep Archive Admission | ||
| monitor 45 and API monitor 10 unchanged. | ||
|
|
||
| - Add durable completion time and last-success/failure observations. Alert on |
There was a problem hiding this comment.
This bullet is now stale. It still lists durable completion time, last-success observations and the stalled/backpressure alerts as future work, but this PR adds all three. Please update or remove it so the gate list matches what's actually left.
| @@ -0,0 +1,75 @@ | |||
| # Retention and progress alerts implementation ledger | |||
|
|
|||
| Base: merged master `a43de7f9e59e60772f94013ceb313de9431e5f63`. | |||
There was a problem hiding this comment.
This file reads like a working log rather than durable documentation. It has a base SHA, one-off preflight numbers ("approximately 259 GiB free … 116 GiB available memory") and process steps ("exact-head Claude and Grok self-review"). The retention and health policy it lays out is already in negentropy_continuous_readiness.md, and again in code comments and the PR body. That puts the same bounds in three places that will drift apart. I suggest deleting this file and moving any gate that's still needed into the readiness doc or the PR description.
erskingardner
left a comment
There was a problem hiding this comment.
| Review metadata | Value |
|---|---|
| Reviewed at (UTC) | 2026-10-10T19:12:04Z |
| Commit reviewed | f1284c01676dbf82fa2c54420c3b7ea36f6cde49 |
| Model | Grok 4.7 |
| Reasoning level | Not exposed by runtime |
| Recommended action | Fix blocking issues before merge |
Schema 9 is transactional, split parents do become complete, job ids are not reused, proof-pinned trees stay put, and CI on this head is green. The retention predicate and the progress sample each break the invariant they are there to enforce.
A removed allowlist entry keeps its last sweep id on a disabled planner_relays row, and compaction treats that id as a live sweep, so that completed history is never reclaimed. The 15-minute stale threshold is also measured from a sample taken before unbounded replay and the 9-minute session, so a healthy tick can report owner_observation_stale.
| // A current planner sweep can still append roots. A closed sweep with any | ||
| // unresolved root/child must retain its full detailed lineage. | ||
| let open: bool = tx.query_row( | ||
| "SELECT EXISTS(SELECT 1 FROM planner_relays WHERE sequence=?1) |
There was a problem hiding this comment.
This treats every stored planner_relays.sequence as a live sweep. configure_planner disables a removed relay with enabled=0 and leaves sequence unchanged, and plan_rolling only advances enabled=1 rows. After an allowlist change, the removed relay's last rolling-v1:<sequence> therefore stays open forever, even when every job in that sweep is complete. compact_tree keeps returning 0, the cursor just walks past those roots, and that completed detail counts toward the job cap indefinitely.
progress_observation already restricts the planner-due check to enabled=1. This predicate needs the same filter:
EXISTS(SELECT 1 FROM planner_relays WHERE enabled=1 AND sequence=?1)
OR EXISTS(SELECT 1 FROM jobs WHERE sweep=?2 AND state!='complete')Unfinished jobs in the sweep still pin it via the second clause. Please add a regression that completes a rolling sweep, disables that relay, and asserts the roots compact.
| gauge!("negentropy_isolated_recovery_checked").set(recovered.checked as f64); | ||
| let mut observation = parent.lifecycle().await?; | ||
| observation.capacity_exhausted |= !data_has_headroom(&config.ledger)?; | ||
| pensieve_core::reconciliation_health::reconciliation_health().observe(observation); |
There was a problem hiding this comment.
This sample is published before the rest of the tick. The same arm then runs replay_next with no time bound and can serve for SESSION_TIME (540s). The next observe happens only on the following tick, so the gap is replay plus session, not the session alone. OBSERVATION_TIMEOUT is 900s, which leaves about six minutes for replay; a slower segment makes /health/negentropy return owner_observation_stale while this owner is still inside the tick.
Publish again after replay_next returns (immediately before serve is enough to restore the "timeout exceeds one session" bound). The canary and disk-pause continues already re-enter the loop promptly; this fall-through path does not.
|
Addressed this round's exact-head Claude and Grok findings in
The Kuma push implementation is retained intentionally. Read-only inspection of the deployed Kuma database confirmed monitor 45 (Pensieve Archive Admission) is type push, interval 180 seconds; monitor 10 (Pensieve API) is type http. The private reconciliation endpoint should not be exposed just to add a direct monitor. This distinction is now documented. No credentials were printed or committed, and neither existing monitor was changed. Validation: all 10 focused retention tests and final |
erskingardner
left a comment
There was a problem hiding this comment.
| Review metadata | Value |
|---|---|
| Reviewed at (UTC) | 2026-10-10T19:21:36Z |
| Commit reviewed | 23d398767c06b311e85c425a60699783caf27996 |
| Model | claude-opus-5-5 |
| Reasoning level | Not exposed by runtime |
| Recommended action | Resolve serious concerns before merge |
The follow-up fixes both earlier blocking findings. Pinning now happens per tree, disabled planners no longer hold history, and the observation is refreshed after replay and planning. The persistent-failure probe uses due_jobs, and the retention counters are exported. I checked the ID allocator again. Only enqueue_window and split_window insert jobs, and both go through next_id, so IDs can't be reused. Compaction stays atomic and bounded.
One serious concern remains, in the alert semantics:
- A blocked job keeps
/health/negentropyat 503 for good. A one-secondblockedleaf is terminal, and nothing in code or docs resolves or acknowledges it. After the first dense second on Primal, the endpoint returnsunresolved_failuresuntil someone edits the ledger by hand. The Kuma push only sendsHTTP 503, so from then on the monitor can't tell a real stall from that old block. It stops being a progress alarm. Details are inline.
Non-blocking:
- The active-sweep pin no longer has a reason to exist. The reasoning that lets disabled cursors drop it applies to enabled cursors too.
fresh_post_replay_observation_covers_a_full_worker_sessiononly tests the 900 s threshold arithmetic. It doesn't exercise the runtime ordering it was added to cover.- The new readiness paragraph says monitor 45 is a push monitor.
ops/RUNBOOK.md:40anddocs/archive_failure_recovery.md:79still tell operators to set up an HTTP monitor. Please make them agree, or keep deployment-specific monitor IDs out of the repo docs.
CI at this head: rustfmt, clippy, doctests and the ops tests pass. Test shards 1–3 were still running when I posted.
| pending = Some(last_success.unwrap_or(activated)); | ||
| } | ||
| let persistent_failure = self.db.query_row( | ||
| "SELECT EXISTS(SELECT 1 FROM jobs WHERE state='blocked' |
There was a problem hiding this comment.
blocked is terminal. split_window sets it for a dense one-second window, and nothing moves a job out of it: there's no unblock or acknowledge path in code, and no operator procedure in the docs. So the first dense second on a busy relay makes this EXISTS true for the life of the ledger, and /health/negentropy returns unresolved_failures 503 permanently.
After that the Kuma monitor is red all the time. Operators either pause it or learn to ignore it, and either way a real durable_progress_stalled or owner_observation_stale goes unnoticed. That is the failure this endpoint exists to catch.
Suggestions, any one would do:
- Keep the health verdict about progress (stale, stalled, capacity, retries ≥ 3 that are still unresolved), and expose blocked windows as a gauge or counter (
negentropy_blocked_jobs) with its own alert. - Alert only on blocked jobs created after a persisted acknowledgement high-water mark, and document how to advance it.
- At minimum, document that a blocked window ends the bounded trial and say what the operator does next.
| code = error.code | ||
| except Exception: | ||
| return False, "negentropy progress unavailable" | ||
| return code == 200, f"negentropy progress HTTP {code}" |
There was a problem hiding this comment.
Only the status code reaches Kuma, so disabled, runtime_failed, owner_observation_stale, durable_progress_stalled, unresolved_failures and capacity_exhausted all show up as negentropy progress HTTP 503. The body is a fixed set of non-sensitive strings from reconciliation_health.rs on localhost. You could read up to about 64 bytes and forward the reason only when it is in an allowlist of those strings, which keeps the no-arbitrary-body guarantee. Without that, the on-call person has to SSH in to learn which of six conditions fired.
| { | ||
| return Ok(0); | ||
| } | ||
| // Only active planners pin their current sweep. Disabled cursors resume at |
There was a problem hiding this comment.
Non-blocking. The reason given for disabled cursors applies to enabled ones as well. plan_rolling advances only from planner_relays.next, and only the planner writes rolling-v1:* (enqueue rejects the namespace). Deleting a completed root from the current sweep therefore can't cause it to be re-planned or trip the overlap check. With that, this query and its comment can go, and the active sweep's 1,344 or more completed roots per relay become eligible after 24 h like everything else. If there's a reason I'm missing, such as "closed sweep" reporting semantics, please say so in the comment.
| } | ||
|
|
||
| #[test] | ||
| fn fresh_post_replay_observation_covers_a_full_worker_session() { |
There was a problem hiding this comment.
Nit. This only checks that a sample 540 s old is fresh and one 1,140 s old is stale, which stale_owner_failure_and_pressure_are_never_healthy already shows. It doesn't touch runtime.rs, so moving the second observe back before replay_next wouldn't make it fail. Either drop it or test the ordering in drive. Don't present it as the watchdog regression.
erskingardner
left a comment
There was a problem hiding this comment.
| Review metadata | Value |
|---|---|
| Reviewed at (UTC) | 2026-10-10T19:28:05Z |
| Commit reviewed | 23d398767c06b311e85c425a60699783caf27996 |
| Model | Grok 4.7 |
| Reasoning level | Not exposed by runtime |
| Recommended action | Fix blocking issues before merge |
The follow-up addresses the earlier sweep pin, disabled-planner retention, failure-query plan, post-replay sample, metrics, and duplicate doc. Schema 9 rollback, whole-tree deletion, and the job-id high-water mark look right, and CI on this head is green.
Opening a schema 8 ledger still stores creation time 0 for every existing incomplete job. /health/negentropy then reports durable_progress_stalled until some later completion. The fresh-ledger trial does not hit this; the migration in this PR does.
| Ok((r.get(0)?, r.get(1)?)) | ||
| })?; | ||
| let mut pending: Option<i64> = self.db.query_row( | ||
| "SELECT min(recorded_at) FROM jobs WHERE state!='complete'", |
There was a problem hiding this comment.
Schema 9 adds recorded_at INTEGER NOT NULL DEFAULT 0, and job_recorded fills it only on insert. Every incomplete row that already existed in schema 8 therefore reads as recorded at the Unix epoch.
min(recorded_at) becomes pending_since, and last_success is still NULL, so ReconciliationHealth::status returns durable_progress_stalled on the first sample. A job that was queued immediately before the upgrade looks more than an hour overdue. max(pending_since, last_success) hides that only after some later completion.
This is the same constraint as leaving completed_at NULL: the migration does not know the time, so it cannot store the epoch. Ignore recorded_at = 0 in the minimum. If incomplete rows exist and none of them have a positive recorded_at, set pending_since to activated_at rather than reporting idle. A schema-8 fixture with one incomplete job should stay inside the deadline at activated_at, and should still stall an hour later if nothing completes.
| code = error.code | ||
| except Exception: | ||
| return False, "negentropy progress unavailable" | ||
| return code == 200, f"negentropy progress HTTP {code}" |
There was a problem hiding this comment.
Non-blocking. probe keeps only the status code, and both the success path and HTTPError drop the body. Every push is then negentropy progress HTTP 503, whether the runtime is stalled, out of capacity, stale, or disabled. Read a few dozen bytes and append the body when it is one of the fixed reason tokens.
|
Addressed this round's reviews of
Final |
erskingardner
left a comment
There was a problem hiding this comment.
| Review metadata | Value |
|---|---|
| Reviewed at (UTC) | 2026-10-10T19:41:13Z |
| Commit reviewed | 337816330f0a99e1e09767d480ebcb9ef98c4e5b |
| Model | claude-opus-5-5 |
| Reasoning level | Not exposed by runtime |
| Recommended action | Merge |
This head resolves the open findings from the earlier rounds:
- Legacy pending rows:
recorded_at=0stays unknown. The one-time grace period now starts at the persistedactivated_at, so reopening the ledger can't reset it. - Blocked jobs: the readiness doc now says plainly that any blocked job ends the bounded trial. The fault stays red, and there is no automatic acknowledgement.
- Kuma probe: it reads at most 65 bytes and forwards only allowlisted reasons. The HTTP-error path is covered too.
- Active-sweep pinning: removed. It was redundant, because the persisted planner cursor already stops compacted roots from being recreated.
I re-checked the ID allocator. enqueue_window and split_window are still the only inserts into jobs. I also re-checked the FK surface: only attempts, failure_reports, receipts and policy_exclusions reference jobs or attempts, and compaction deletes or pins every one of them. The 24h retention bound fits the runtime's throughput. With a 5 s tick and at most one lease per tick, the ledger keeps at most about 17k roots a day. Two retention turns per tick keep up with that, even counting the cursor's turns on roots that are too recent to compact.
Non-blocking, inline:
- Proof-pinned trees can't be seen. They are bounded by the size of the exclusion cache (100,000 entries), not by 24h, and that cap is above the planner's job limit.
skipped_roots_totalcounts roots that are merely too recent together with pinned ones, so the trial can't measure this growth. - Duplicated reserve formula. The split-reserve formula is copied from the planner with a bare
2048.
CI at this head: rustfmt, clippy, doctests, ops tests, and test shards 1/3 and 3/3 pass. Shard 2/3 was still running when I posted this.
Posted as a comment because GitHub does not allow approving your own pull request; the recommendation is Merge.
| tx.commit()?; | ||
| Ok(RetentionProgress { | ||
| removed_jobs: removed, | ||
| retained_candidate: removed == 0, |
There was a problem hiding this comment.
retained_candidate is true for every root that is too recent to compact, which covers most of the ~17k roots a day can retain. So negentropy_retention_skipped_roots_total goes up about twice per tick even when nothing is wrong. That noise hides the one case that actually grows: trees pinned by a policy_exclusions owner (line 238).
Those trees unpin only when the exclusion cache evicts their entry. Eviction starts only once the cache reaches capacity(max_bytes), which is 1 GiB / 8 KiB clamped to 100,000 entries. That is more than the planner's 97,952-row job limit. If excluded giftwraps are sparse, around one new excluded event per window, pinned trees can fill the ledger before anything is evicted. It fails visibly as capacity_exhausted, but the real retention bound depends on how dense the exclusions are, not on the 24h window.
Suggestion: return the skip reason (too recent, proof-pinned, or oversized) and export a separate pinned-roots counter or gauge. Then the single-relay trial measures this growth directly.
| .db | ||
| .query_row("SELECT count(*) FROM jobs", [], |r| r.get(0))?; | ||
| metrics::gauge!("negentropy_retained_jobs").set(f64::from(retained)); | ||
| let reserve = 2048.min(self.limits.max_jobs / 10); |
There was a problem hiding this comment.
This copies the planner's SPLIT_JOB_RESERVE.min(self.limits.max_jobs / 10) (planner.rs:126) but with a bare 2048. If one side changes, the health check and planner backpressure will disagree about when capacity is exhausted. Make SPLIT_JOB_RESERVE pub(super), or add a shared planned_limit(limits) helper, and use it in both places.
erskingardner
left a comment
There was a problem hiding this comment.
| Review metadata | Value |
|---|---|
| Reviewed at (UTC) | 2026-10-10T19:46:24Z |
| Commit reviewed | 337816330f0a99e1e09767d480ebcb9ef98c4e5b |
| Model | Grok 4.7 |
| Reasoning level | Not exposed by runtime |
| Recommended action | Fix blocking issues before merge |
Schema 9 stays transactional, completed roots compact without pinning sibling gaps, and job ids are not reused. The post-replay sample, legacy grace baseline, and allowlisted Kuma reasons address the earlier findings. CI on this head is green.
One blocking issue remains: a successful third attempt is reported as unresolved_failures for the archive seal wait, and the activation contract treats that status as the end of the trial.
| } | ||
| let persistent_failure = self.db.query_row( | ||
| "SELECT EXISTS(SELECT 1 FROM jobs WHERE state='blocked' | ||
| OR (state IN ('queued','leased','awaiting_durability','retry_wait') AND attempt>=3))", |
There was a problem hiding this comment.
attempt increments when the lease is taken, and record_protocol_done leaves that value in place when it moves the job to awaiting_durability. A window that fails twice and then succeeds is therefore awaiting_durability with attempt=3 until the segment seals. The default seal cadence is 300 seconds, and this predicate is sampled on every owner tick.
/health/negentropy turns that into unresolved_failures. The readiness contract says a persistent failure ends the bounded trial: stop the worker and leave the alert red. A successful third try does that before archive maintenance can mark the job complete. leased has the same shape for a third attempt that is still running.
Treat blocked as the terminal failure, and treat retry_wait as persistent only after the attempt has actually failed. Leave awaiting_durability to the one-hour completion deadline. Please narrow this predicate and the matching due_jobs plan assertion together.
|
Addressed this review round in a55162d: persistent-failure detection now selects blocked jobs or retry_wait after at least three failed attempts, not an active third lease or protocol-success archive wait. The new regression uses real lease/retry/protocol-done transitions, reopens the ledger, checks third-failure alerting and successful-third-attempt grace, and proves the one-hour durability deadline still fails closed without inventing completion. The matching query-plan assertion and runbook were updated. All 12 retention tests and mandatory full just precommit passed, including 17 ops tests. Claude recommended Merge on 3378163; Grok requested this fix, so the updated head still needs a fresh merge recommendation. The two non-blocking Claude suggestions remain follow-up items. No production changes or activation were performed. |
erskingardner
left a comment
There was a problem hiding this comment.
| Review metadata | Value |
|---|---|
| Reviewed at (UTC) | 2026-10-10T19:58:30Z |
| Commit reviewed | a55162d351a5762a461ea636ebf0a6ece991b452 |
| Model | claude-opus-5-5 |
| Reasoning level | Not exposed by runtime |
| Recommended action | Merge |
This head fixes Grok's last blocking finding with the smallest possible change. Narrowing the persistent-failure predicate to blocked OR (retry_wait AND attempt>=3) means a successful third attempt waiting in awaiting_durability no longer reports unresolved_failures. That wait is still bound by the one-hour durable-completion deadline. The new test covers both outcomes across a reopen through real lease, retry and protocol-done transitions, and the query-plan assertion still shows due_jobs.
I checked the rest of the state machine against the new predicate:
lease_nextleases straight fromretry_wait, and nothing writesqueuedback after the first insert. Droppingqueuedfrom the predicate therefore loses nothing.retry_durabilitysendsawaiting_durabilitytoretry_waitwithout changingattempt. An archive failure after a third protocol success still alerts.- In normal operation the runtime refreshes the observation before
lease_next_fairand not again during the session. So a fourth attempt runs under the red observation taken while the job was still inretry_wait, and the alert doesn't flap.
Non-blocking, inline: there is one restart window where this ordering doesn't hold. A leased fourth-or-later attempt can show as healthy for up to LEASE_SECS (600 s). The fix is one clause.
The two non-blocking items from my review of 3378163 still apply as follow-ups: proof-pinned growth can't be measured, and the split-reserve formula is duplicated.
CI at this head: rustfmt, clippy, doctests, ops tests and all three test shards pass.
Posted as a comment because GitHub does not allow approving your own pull request. The recommendation is Merge.
| } | ||
| let persistent_failure = self.db.query_row( | ||
| "SELECT EXISTS(SELECT 1 FROM jobs WHERE state='blocked' | ||
| OR (state='retry_wait' AND attempt>=3))", |
There was a problem hiding this comment.
Non-blocking: once three attempts have failed, the predicate only stays red because of when the runtime samples. The observation is taken before lease_next_fair, so a fourth attempt normally runs under a red sample.
Restart breaks that. Say pensieve-ingest restarts while attempt 4 or later holds an unexpired lease. expire() leaves the lease alone. The first post-restart observation then sees state='leased' and reports progress_within_deadline as long as last_success is recent. Kuma gets up pushes for up to 600 s before the lease expires back into retry_wait. The readiness doc says startup must not refresh a grace clock, and that faults stay red until recovery actually succeeds. This contradicts both.
One clause fixes it, and due_jobs should still serve it:
SELECT EXISTS(SELECT 1 FROM jobs WHERE state='blocked'
OR (state='retry_wait' AND attempt>=3)
OR (state='leased' AND attempt>3))In third_attempt_is_not_a_failure_until_it_actually_fails, also lease attempt 4 in the !succeeds branch and assert unresolved_failures, so the fault is shown to stay latched through the next attempt.
erskingardner
left a comment
There was a problem hiding this comment.
| Review metadata | Value |
|---|---|
| Reviewed at (UTC) | 2026-10-10T20:01:28Z |
| Commit reviewed | a55162d351a5762a461ea636ebf0a6ece991b452 |
| Model | Grok 4.7 |
| Reasoning level | Not exposed by runtime |
| Recommended action | Merge |
The follow-up fixes the false unresolved_failures on a successful third attempt. Persistent failure is now only a blocked job, or retry_wait after at least three failed attempts. An in-flight lease and protocol success waiting on archive durability stay on the one-hour completion clock. The new regression covers the third failure, protocol success, and the requirement that protocol success leaves last_success unset until the archive seal. CI on this head is green.
Two earlier non-blocking notes can stay follow-ups: skipped_roots_total still counts roots that are only too recent, and the split reserve 2048 is still copied beside SPLIT_JOB_RESERVE.
Posted as a comment because GitHub does not allow approving your own pull request; the recommendation is Merge.
Problem
Continuous negentropy would eventually fill its job ledger with successful history, while archive health alone could stay green even if reconciliation stopped making progress. Operators need bounded bookkeeping and a separate progress alarm before leaving the worker running.
Fix
Compact old, fully completed rolling work into capped coverage summaries while preserving unfinished gaps, split-tree integrity, and exclusion proofs. Persist completion timing and non-reusable job IDs, and expose reconciliation health independently of ingestion health. Add a credential-safe Uptime Kuma probe and document a single-relay bounded activation rather than silently enabling continuous operation.
Safety boundaries
Verification
just precommit(format, strict workspace clippy, workspace tests, operational tooling tests).