fix(agent): autonomously retry durable finalization inbox - #2002
Conversation
Review —
|
Round 2 — re-review at
|
| Round 1 | Status |
|---|---|
| H1 immortal poison row | Fixed. finalization-recovery.ts:390-402 transitions to REJECTED once isLiveEntry && attemptCount >= liveRetryLimit. Capacity counts only live states, so the slot frees immediately, and REJECTED falls under the terminal prune. Mutating >= this.liveRetryLimit to + 1000 turns it red. |
| H2 double-apply | Genuinely fixed. A union-counter probe over {apply, replayVerified} gives maxSection: 1; removing await previous gives maxSection: 2. No re-entrancy deadlock — replayDueEntryLocked deliberately inlines matchingEntries + replayEntry rather than calling replayMatching. No map leak, unrelated keys not serialised. |
| M VERIFIED unbounded | Fixed — isLiveEntry (:471-475) covers VERIFIED, so the VERIFIED-poison case is bounded too. |
| M backoff wipe | Fixed — recordAttempt now CASE … ELSE MAX(next_attempt_at, ?), widen-only. Pinned by a new test. |
| M lifecycle wiring unpinned | Fixed, and pinned the way I hoped. finalization-recovery.test.ts:565+ builds a real DKGAgent, spies on the handler before agent.start(), asserts startWorker called once, then await agent.stop() and asserts stopWorker. It also asserts handler identity, so it can't pass vacuously — which was the trap I flagged. |
| L due-gate/reconciliation | Pinned by a new test. |
| L no telemetry | Addressed — dueEntries + oldestDueAgeMs in health, telemetry-api.ts gauges, status-route depth. |
Suite: 94 passed / 1 skipped.
HIGH — the retry budget is an attempt count burned by two unthrottled lanes, so a legitimate finalization can be permanently rejected
This one is on me. In round 1 I wrote "after N attempts, or created_at + maxLiveAgeMs". The count form was chosen, and it is the fragile half — because attemptCount is not a clock.
FINALIZATION_RECOVERY_LIVE_RETRY_LIMIT = 7*24*60 = 10080 is documented as "approximately seven days", which is only true if the worker is the sole consumer at one attempt per 60 s. It isn't. Three callers increment it and only one is time-spaced:
processLivehas no due gate and passes no delay torecordDeferred(:305,:323— I verified both call sites). Under the new widen-onlyCASE … ELSE MAX(...), a NULL delay leavesnext_attempt_atuntouched, so duplicate gossip now increments the counter for free.replayMatching(chain reconciler) likewise bypasses the due gate by design (:1592-1596).- The
processedUalsdedupe atfinalization-handler.ts:715sits inprocessFinalization, which is only reached whenprocessLivereturnsfalse— with the durable inbox configured, every duplicate reachesprocessLivefirst.
So the real budget is 10080 / (1 + duplicates-per-minute). And exhaustion is terminal for both lanes — I verified this myself:
:299—if (!this.isLiveEntry(entry)) return true;— re-gossip is swallowed as "handled" and never materialized. The comment states the intent: "duplicate gossip must not revive an entry that exhausted the autonomous retry budget.":1589—replayEntryreturns'none'for a non-live, non-SETTLED entry, so authoritative chain reconciliation refuses it too.
Reproduced with liveRetryLimit: 5 and a frozen clock (zero ms elapsed throughout):
AFTER 3 CHAIN-RECONCILIATION PASSES: { state:'RECEIVED', attemptCount:3 }
AFTER 5 DUPLICATE GOSSIP DELIVERIES: { state:'RECEIVED', attemptCount:5, applyCalls:0 }
AFTER ONE WORKER TICK, HEALTHY CHAIN: { state:'REJECTED',
lastError:'autonomous retry limit exhausted after 5 attempts',
applyCalls:0 }
AFTER RE-GOSSIP + CHAIN RECONCILIATION: { state:'REJECTED', replayOutcome:'none', applyCalls:0 }
The chain was healthy at the moment of rejection, the KA was never materialized, and nothing can revive it. Pre-PR this row self-healed via the 7-day raw prune. The realistic trigger is a KA gossiped by many peers during a multi-hour chain outage — no adversary required.
Fix: make the gate wall-clock, which is the other half of what I suggested in round 1 — reject only when now - entry.createdAt >= liveRetryWindowMs, optionally && attemptCount >= limit as a belt-and-braces bound. createdAt is already on the entry, so no schema change. Applied literally it will break the author's liveRetryLimit: 2 test, which advances a fake clock only to nextAttemptAt; that test must also advance now past the window.
MEDIUM — the new lock test does not actually pin the lock
finalization-recovery.test.ts "serializes live and autonomous materialization for the same inbox key" counts concurrency inside apply only. Once the live lane promotes the row to VERIFIED, the worker lane takes replayVerified and never enters apply, so the counter is structurally capped at 1. Mutating :487 await previous.catch(…) → void previous (lock disabled) leaves the test passing.
Fix: count apply and replayVerified in one shared counter — that variant does go red with the lock disabled, so it is a true guard for the H2 fix.
MEDIUM — degradedReason overwrite masks capacity-exhausted, the loudest incident signal
dkg-agent-base.ts:1691-1703 spreads ...health then conditionally rewrites degradedReason: 'canonical-finalization-receipt-unsupported', clobbering the store's 'capacity-exhausted'. In the incident replay, capacity-exhausted was reported at every sample — it is the single clearest signal the store emits, and this PR's own status-route fixture is exactly the masking configuration. Pre-existing, but it blunts this PR's stated purpose. Fix: make the reason additive (degradedReasons: string[]) or prefer the store's. A literal string swap breaks two toEqual assertions.
MEDIUM — oldestDueAgeMs measures age-since-receipt, not age-of-non-progress
finalization-recovery-sqlite-store.ts:596-609 uses MIN(created_at), which never moves. A row being retried correctly still ages monotonically, and with a 60 s backoff cap and a 10,080 limit a healthy-but-slow row legitimately climbs toward days. So "oldest due age is large" ≠ "stuck", which is precisely the discrimination an operator needs. Fix: expose MIN(updated_at) over the due set as well (attempts touch updated_at) — emit both, since updated_at alone loses the never-attempted case.
LOW — the new gauges silently freeze on the two paths that matter
recordDueMetrics runs only at finalization-recovery.ts:356; the early returns at :340 and :344-351 skip it. So when listDue throws (store read failing) or the chain binding is absent — exactly when you want the signal — the gauges retain their last-good value and only a log.warn remains. Move it into a finally.
Nit — the new packages/chain test commit
92e0efeca is a correct fix: two eth_getCode probes run in parallel, so asserting their server arrival order was testing an implementation accident. Swapping to set-equality is right. One small thing — the Set drops the count, so a duplicated probe (TO, TO, OTHER_TO) collapses to the same set and passes. expect(server.calls.slice(2)).toHaveLength(2) alongside it keeps the flake fix without losing that.
Merge readiness
One thing to fix. The HIGH is a genuine data-loss path for a legitimate finalization, and the remedy is small — swap the count gate for a wall-clock window. The lock-test MEDIUM is worth doing at the same time, since it currently gives false confidence in the H2 fix that is correct.
Everything else is small, and the substance of this round is good: H1 and H2 are properly fixed, the wiring is pinned end to end, and the observability additions are real. My round-1 finding about the lifecycle wiring was valid and is now closed by a test that can't pass vacuously.
Method: 2 lenses over the delta, schema-free, executing the real modules and mutation-testing each new test with proven restores. I independently verified the wiring test's start/stop assertions, the processLive no-delay call sites, both terminal-refusal paths (:299, :1589), isLiveEntry's VERIFIED coverage, the capacity/prune state sets, and the packages/chain commit.
| if (!store.isAttemptDue(entry)) return 'retry-pending'; | ||
| if ( | ||
| this.isLiveEntry(entry) | ||
| && entry.attemptCount >= this.liveRetryLimit |
There was a problem hiding this comment.
🔴 Bug: Live duplicate gossip can exhaust the autonomous retry budget
What's wrong
The new exhaustion check treats attemptCount as if it only counted autonomous worker retries. It is shared with live processing, and live duplicate gossip still calls materialization and recordDeferred even while a future retry deadline is present. That allows ordinary duplicate traffic, or a peer replaying the same message, to turn a recoverable inbox row into a terminal REJECTED row much sooner than the documented retry window.
Example
A valid finalization has a temporarily pending receipt. The worker sets nextAttemptAt and attemptCount=1. Repeated duplicate gossip before that deadline calls processLive, records more deferrals, and can drive attemptCount to 10080 in minutes or hours. When the worker next sees the entry due, it rejects it as exhausted instead of continuing the intended seven-day autonomous retry window.
Suggested direction
Track autonomous worker attempts separately from live/reconciliation deferrals, or only apply liveRetryLimit to attempts actually made by the worker. Another acceptable direction is to avoid incrementing the shared attemptCount for live duplicates that arrive before the persisted retry deadline.
For Agents
Look in FinalizationRecovery around processLive, recordDeferred, and replayDueEntryLocked. Preserve the worker's poison-entry cap, but do not let duplicate live gossip or chain reconciliation spend the autonomous retry budget. Add a test with a low liveRetryLimit where live duplicates during backoff do not cause the next due worker pass to REJECT a still-valid entry.
| merkleRoot: entry.merkleRoot, | ||
| kaId: entry.kaId, | ||
| }; | ||
| const matches = await this.matchingEntries(replayInput); |
There was a problem hiding this comment.
🟡 Issue: The due worker should not route one due row through the KA-wide reconciliation selector
What's wrong
This makes the new autonomous worker depend on the chain-cursor reconciliation abstraction even though it already has an exact due entry. That adds an avoidable query, a synthetic FinalizationRecoveryReplayInput, another filter by key, and more coupling between two different orchestration modes. It also keeps growing an already very large recovery class instead of deleting complexity around a clearer entry-level replay primitive.
Example
A batch with 16 due entries for the same KA now runs the KA-wide reconciliation selector 16 times, then discards every match except the original key each time. The resulting flow is harder to reason about than an entry-centric replay path that validates the one due row directly.
Suggested direction
Extract the canonical eligibility/read logic from matchingEntries into a smaller helper, then let autonomous due replay process the already-selected entry directly. If batch efficiency matters, group due rows by KA and share the chain reads rather than repeatedly calling the reconciliation selector.
For Agents
Look at replayDueEntryLocked, matchingEntries, and replayEntry in packages/agent/src/finalization-recovery.ts. Preserve autonomous recovery outcomes and reconciliation behavior, but split out a single-entry due replay path or shared canonical-chain snapshot helper so the worker does not synthesize a reconciliation request and re-query/filter the same inbox row. Existing autonomous recovery and reconciliation tests should still pass, with coverage proving due replay and cursor replay share the same canonical checks without looping through each other.
There was a problem hiding this comment.
🟡 Issue: Autonomous worker orchestration is being added to an already oversized recovery state machine
What's wrong
This makes the central recovery class harder to scan and reason about. The PR is not just adding a method; it is mixing lifecycle/timing policy with canonical replay and state-transition logic, which is the kind of growth that turns this file into the only place where finalization behavior can be safely changed.
Example
A single due inbox row now flows through processDueBatch -> replayDueEntryLocked -> matchingEntries -> replayEntry -> replaySettled/recover..., while the same class also records telemetry and manages retry-budget policy.
Suggested direction
Extract the autonomous due-replay runner/policy into a focused module that owns listDue, due metrics, retry budgets, and entry serialization, and have it call a narrow recovery API for state transitions/materialization. That would keep FinalizationRecovery closer to a durable state machine instead of making it also own worker orchestration.
For Agents
Look in packages/agent/src/finalization-recovery.ts around processDueBatch, replayDueEntryLocked, recordDueMetrics, withEntryLock, and the new retry options. Preserve live finalization, chain reconciliation, autonomous drain, backoff, and same-key serialization behavior. Prove the refactor with the existing autonomous drain/backoff/entry-lock tests plus a reconciliation replay test.
There was a problem hiding this comment.
🟡 Issue: The autonomous due-worker workflow is being bolted into an already oversized recovery class
What's wrong
This change substantially widens the responsibilities of a class that was already too large to scan comfortably. The new worker class is mostly a timer wrapper; the real worker behavior is embedded in FinalizationRecovery, so the codebase gains a new concept without a real ownership boundary. That makes future changes to live finalization, chain reconciliation, retry policy, and autonomous replay more likely to interfere with each other.
Example
A reader trying to understand one due worker tick now has to follow processDueBatch -> replayDueEntry -> replayDueEntryLocked -> matchingEntries -> replayEntry -> replaySettled, while also tracking live-gossip locking and metrics from the same 2k-line class.
Suggested direction
Move the new autonomous retry workflow behind a dedicated replayer/policy module instead of growing FinalizationRecovery into a scheduler, metrics emitter, lock manager, replay selector, and state machine all at once.
For Agents
Look in packages/agent/src/finalization-recovery.ts. Preserve the current live admission and replay behavior, but extract the autonomous due-entry workflow into a focused collaborator, for example FinalizationRecoveryDueReplayer, that owns processDueBatch, due metrics, retry budget/backoff policy, and per-entry serialization. Keep FinalizationRecovery as the canonical owner of durable entry transitions and materialization primitives. Existing finalization recovery tests should still pass without changing observable outcomes.
| snapshot: FinalizationRecoveryEntry, | ||
| input: FinalizationRecoveryReplayInput, | ||
| replayKey: string, | ||
| deferredRetryDelay?: number, |
There was a problem hiding this comment.
🟡 Issue: Make replay mode explicit instead of using an optional delay as a control flag
What's wrong
The optional deferredRetryDelay parameter is doing too much: it carries a delay value, selects autonomous-vs-reconciliation behavior, controls whether persisted deadlines are honored, and changes returned outcomes. Threading that nullable flag through several settled/live recovery helpers makes the boundary muddy and invites future special-case branching.
Example
deferredRetryDelay !== undefined currently means both “this is autonomous replay” and “persist backoff / return retry-pending / respect attempt deadlines”. A future settled-retry change has to know that passing undefined is not just “no delay”; it changes the replay mode.
Suggested direction
Introduce a small typed replay policy/context object or split autonomous and reconciliation replay into separate entry points. That would make deadline handling, retry outcome mapping, and persisted backoff explicit instead of relying on undefined as hidden control flow.
For Agents
Replace the optional numeric mode flag in replayMatching, replayEntry, replaySettled, recoverSettledPublisherUpgrade, and recoverSettledReorg with an explicit replay context, for example a union like { kind: 'reconciliation' } | { kind: 'autonomous'; backoffMs: number; respectDeadline: true }. Preserve existing live, reconciliation, and worker behavior, and keep tests around worker backoff plus immediate reconciliation before deadline.
There was a problem hiding this comment.
🟡 Issue: The worker mode leaks through replay as optional retry-delay plumbing
What's wrong
The new optional parameter is doing too much: it controls due-deadline checks, whether deferrals persist backoff, and how outcomes are mapped. That makes the replay path harder to modify because a future change must understand a hidden mode protocol rather than a named abstraction.
Example
deferredRetryDelay === undefined means reconciliation mode: ignore live-entry deadlines and map deferrals to none. A number means autonomous mode: honor deadlines, persist backoff, and return retry-pending. That implicit mode contract is spread across several branches.
Suggested direction
Replace the nullable delay flag with an explicit policy boundary, or split reconciliation replay from autonomous due replay so each path has direct return/outcome handling. The lower-level helpers should not need to infer caller intent from whether a number is undefined.
For Agents
Refactor packages/agent/src/finalization-recovery.ts around replayMatching, replayEntry, replaySettled, and the settled recovery helpers. Preserve the different reconciliation-vs-worker retry semantics, but model them explicitly with a small ReplayPolicy/ReplayMode object or separate entry points. Tests should cover chain reconciliation before deadline and worker deferral/backoff.
There was a problem hiding this comment.
🟡 Issue: Autonomous replay mode is threaded through the main replay path as optional flags
What's wrong
This PR adds a second replay mode into an already very large FinalizationRecovery class by using optional parameters as hidden mode switches. The result is harder to scan and easier to extend incorrectly because worker-specific backoff, reconciliation semantics, settled retry handling, and outcome mapping are now interleaved across the same helper chain.
Example
A reader now has to know that undefined means chain reconciliation/live replay semantics, while a number means autonomous worker semantics. That mode controls due gating at lines 1616-1620 and result mapping at lines 1681-1684 and 1716-1718.
Suggested direction
Introduce an explicit ReplayPolicy/ReplayTrigger model, or move autonomous replay into a small FinalizationDueReplayer that owns due gating, backoff, and retry-budget handling. Keep the core replay helpers focused on replaying one fresh entry rather than inferring caller mode from optional parameters.
For Agents
Refactor packages/agent/src/finalization-recovery.ts around processDueBatch, replayMatching, and replayEntry. Preserve behavior where chain reconciliation ignores persisted retry deadlines and the worker respects them, but represent that with an explicit replay policy/trigger or extract a focused background replayer instead of passing optional delay flags through settled/live helpers. Existing recovery tests should continue proving worker backoff and reconciliation-before-deadline behavior.
There was a problem hiding this comment.
🟡 Issue: The replay mode is encoded as an optional retry delay
What's wrong
Using undefined as a mode flag makes the replay flow harder to reason about and spreads autonomous-worker policy across settled replay, live-entry replay, and deferred recording branches. This is exactly the kind of nullable mode that tends to accumulate more special cases over time.
Example
deferredRetryDelay !== undefined currently means “this is autonomous replay, enforce persisted retry deadlines, persist backoff, and surface retry-pending.” That invariant is implicit and must be rediscovered at every call site that receives the optional number.
Suggested direction
Model the replay trigger/backoff policy explicitly, or isolate autonomous replay so the regular replay path does not need nullable mode flags threaded through several private helpers.
For Agents
Replace the optional deferredRetryDelay / persistDeferredBackoff threading in packages/agent/src/finalization-recovery.ts with an explicit replay context, such as { trigger: 'reconciliation' | 'autonomous-worker', deferredRetryDelayMs?: number }, or split the autonomous path into its own helper that computes backoff once. Preserve that chain reconciliation can ignore worker deadlines while autonomous replay respects them. Tests around replayMatching, SETTLED replay, and due worker backoff should continue to prove both modes.
There was a problem hiding this comment.
🟡 Issue: Make replay mode explicit instead of encoding it as an optional delay
What's wrong
This change adds a hidden mode flag across the replay stack. The optional number is doing too much: it is both data and a control-flow selector. That makes the recovery path harder to modify safely because every replay helper now has to understand both chain reconciliation semantics and autonomous worker semantics.
Example
replayMatching(input, { persistDeferredBackoff: true }) actually means: observe the persisted due deadline, persist exponential backoff on deferral, and return retry-pending instead of none. A reader only discovers that by following the optional number several methods down the call stack.
Suggested direction
Introduce an explicit replay policy object or split the autonomous due-replay processor from reconciliation replay. The helpers should receive a named mode with clear operations like shouldObserveDeadline, deferredDelayMs, and deferredOutcome, rather than inferring mode from whether a number is undefined.
For Agents
In packages/agent/src/finalization-recovery.ts, replace the optional backoff plumbing with an explicit replay context/policy, e.g. a discriminated ReplayTrigger for reconciliation vs autonomous. Preserve current worker and reconciliation behavior, and keep the existing deadline/backoff tests proving the two modes differ intentionally.
| `Finalization recovery worker batch failed: ` | ||
| + `${error instanceof Error ? error.message : String(error)}`, | ||
| ); | ||
| } finally { |
There was a problem hiding this comment.
🟡 Issue: Worker failure recovery is not verified
What's wrong
The autonomous inbox worker is meant to be the durable retry mechanism, but the added tests only cover successful batches and shutdown. A transient exception from the batch callback could stop future retries if the catch/finally scheduling regressed, leaving due finalization entries stuck with all current tests still green.
Example
A focused test could use vi.fn().mockRejectedValueOnce(new Error('busy')).mockResolvedValueOnce(0), start a worker with a tiny pollIntervalMs, and assert the batch function is called a second time and warn is emitted. Without the catch/finally reschedule behavior, that test would fail while the current worker tests still pass.
Suggested direction
Add a worker test that exercises the catch/finally path after a transient processDueBatch rejection and proves the retry loop continues.
For Agents
Add coverage in packages/agent/test/finalization-recovery-worker.test.ts for a rejected processDueBatch: preserve that the worker logs the failure, remains running, and schedules another poll without overlapping work. Use a short poll interval or fake timers so the test is deterministic.
Round 3 — re-review at
|
| return 0; | ||
| } | ||
| for (const entry of entries) { | ||
| const outcome = await this.replayDueEntry(entry); |
There was a problem hiding this comment.
🔴 Bug: One poison due entry can block the autonomous recovery queue
What's wrong
The new autonomous worker is supposed to drain a durable retry queue, but a single due entry that throws outside the retryable-error path aborts the entire batch. Because that row is not updated, it remains due and ordered ahead of later rows, so unrelated finalizations can be starved indefinitely.
Example
If listDue(16) returns [poisonEntry, validEntry] and materializer.apply or replayVerified throws a non-retryable error for poisonEntry, processDueBatch exits before validEntry is replayed. On the next poll the same poisonEntry is still oldest/due, so the autonomous worker can keep retrying it and never drain later valid finalizations.
Suggested direction
Handle replay failures per entry instead of letting one exception abort the whole batch; make sure the failed entry is not immediately selected forever without updated retry state.
For Agents
In packages/agent/src/finalization-recovery.ts, add a per-entry error boundary around replayDueEntry inside processDueBatch. Preserve fail-closed behavior for the bad entry, but record a deferred/backoff or terminal state when appropriate and continue processing the rest of the batch. Add a test with two due rows where the first replay throws a non-retryable error and the second still settles or is attempted.
| ); | ||
|
|
||
| await expect(recovery.processDueBatch(16)).resolves.toBe(0); | ||
| expect(health).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
🔴 Bug: Due-metrics test never observes the emitted metrics
What's wrong
The added test claims to verify metric refresh on a due-inbox read failure, but it only spies on the intermediate health read. That gives false confidence for the new telemetry contract: incorrect, missing, or zeroed metric recordings would not be caught.
Example
Removing the two gauge writes in recordDueMetrics while leaving the store.health() call intact would keep this test green, even though dkg.finalization_recovery.due_entries and oldest_due_age_ms would stop being emitted.
Suggested direction
Assert the actual metric datapoints or mocked gauge calls, not only that the health snapshot was fetched.
For Agents
Use the existing OpenTelemetry test pattern from the metrics tests: install an in-memory meter provider, call rebuildMetrics(), drive processDueBatch() through the failed listDue path with known health() values, force-flush, and assert the finalization recovery gauge datapoints. Consider also asserting finalizationRecoveryAttemptsTotal labels on a successful due batch.
| stateCounts: { RECEIVED: 1 }, | ||
| }); | ||
| await agent.stop(); | ||
| expect(stopWorker).toHaveBeenCalled(); |
There was a problem hiding this comment.
🟡 Issue: Shutdown test does not prove the recovery worker is awaited
What's wrong
The change adds an awaited shutdown boundary so no autonomous finalization retry continues while chain and graph-store dependencies are being torn down. The current test would pass if the call were made without awaiting it, so it does not verify the behavior that prevents shutdown races.
Example
A regression such as changing the production call to void this.finalizationHandler?.stopRecoveryWorker() would still satisfy this assertion, but shutdown could continue while a recovery batch is still using chain/store dependencies.
Suggested direction
Add an integration assertion that DKGAgent.stop() awaits the worker stop promise before continuing shutdown, not just that it invokes the method.
For Agents
In finalization-recovery.test.ts or an agent lifecycle test, mock stopRecoveryWorker to return a deferred promise, call agent.stop(), assert stop does not complete or dependent teardown does not proceed until the deferred promise is released, then assert it completes. Preserve the existing start/handler reuse assertions.
| clearSettledRetry(key: string, generation: number): Promise<void>; | ||
| rejectSettled(key: string, generation: number, lastError: string): Promise<boolean>; | ||
| isAttemptDue(entry: FinalizationRecoveryEntry): boolean; | ||
| /** |
There was a problem hiding this comment.
🟡 Issue: Due-queue semantics leak across the store and recovery layers
What's wrong
The durable inbox is now both a persistence store and a retry queue, but the queue contract is spread across low-level methods and comments. That makes the implementation depend on callers remembering to re-read snapshots, re-check deadlines, and rely on generation transitions in the right order. This is maintainability debt in the new worker path rather than a clean ownership boundary.
Example
Adding a new retryable state or changing due eligibility now requires keeping DUE_FINALIZATION_SQL_PREDICATE, isLiveEntry, listDue, isAttemptDue, and replayDueEntryLocked aligned by convention.
Suggested direction
Replace the loose trio of listDue/get/isAttemptDue with a higher-level due-work API, such as loadDueBatch(limit) returning fresh due entries under the store’s serialization boundary, or a small queue repository that owns the SQL predicate, freshness check, and retry clock. That would let recovery process entries without reassembling queue invariants manually.
Confidence note
This is a structural concern from the diff shape; the generation-checked transitions may make behavior safe, but the queue ownership boundary is still unnecessarily leaky.
For Agents
Look at packages/agent/src/finalization-recovery-store.ts, packages/agent/src/finalization-recovery-sqlite-store.ts, and processDueBatch/replayDueEntryLocked in finalization-recovery.ts. Preserve bounded oldest-first processing and generation-checked transitions, but move due-entry selection/freshness into a single store/repository-level operation or a dedicated due-queue abstraction.
| // write-ahead journal. Its lifecycle is independent of chain-cursor | ||
| // progress so entries received after a watermark advance are still | ||
| // reconsidered. The worker batches SQLite reads but serializes graph work. | ||
| if (this.finalizationRuntime.getRecoveryStore()) { |
There was a problem hiding this comment.
🟡 Issue: Recovery worker lifecycle is scattered across agent layers
What's wrong
This couples a handler-internal retry loop to broad agent startup, stop, and persistence teardown code. It works, but it makes future lifecycle changes brittle because every new stop/reconfigure path has to remember the same resource and ordering rule.
Example
The lifecycle of one timer-backed resource is now split across three classes: FinalizationHandler owns it, LifecycleSyncMethods starts it, and both DKGAgent plus DKGAgentBase know to stop it.
Suggested direction
Give the owner a single lifecycle method, for example FinalizationHandler.start()/close() or a runtime-owned finalization lifecycle hook, and call that from the existing agent lifecycle boundary. Avoid making generic persistence cleanup know about a handler-internal worker.
For Agents
Consolidate worker lifecycle ownership around FinalizationHandler or the finalization runtime. Preserve the shutdown ordering that stops the worker before chain/graph-store dependencies close, but expose one lifecycle boundary rather than requiring multiple agent layers to know about the worker.
| const now = this.#policy.now(); | ||
| return this.database.prepare(` | ||
| SELECT * FROM finalization_inbox_v1 | ||
| WHERE ${DUE_FINALIZATION_SQL_PREDICATE} |
There was a problem hiding this comment.
🟡 Issue: Autonomous SETTLED recovery lacks direct regression coverage
What's wrong
The PR expands the due queue beyond live entries, but the tests do not exercise the new autonomous replay path for SETTLED entries. That leaves a meaningful gap: regressions in the worker-only setup of replay input, matching, or pending publisher-upgrade handling could still pass because the current SETTLED tests enter through chain reconciliation instead.
Example
A persisted SETTLED row with publisherUpgradePending: true after restart should be picked up by processDueBatch(16), validate the trusted publisher, apply the upgraded access semantics, and clear publisherUpgradePending. Likewise, a SETTLED receipt retry whose nextAttemptAt is due should be replayed by the worker, not only by replayMatching.
Suggested direction
Add a worker-driven test for at least one due SETTLED row, preferably covering the publisher-upgrade case because it is newly admitted by publisher_upgrade_pending even without a retry timestamp.
For Agents
Look in packages/agent/test/finalization-recovery.test.ts or the handler integration tests. Seed or create a SETTLED row that is due, then call processDueBatch or startRecoveryWorker; assert the row is recovered/updated and that retry state is cleared or advanced as appropriate. Preserve the existing chain-reconciliation tests; this should specifically prove the autonomous path.
There was a problem hiding this comment.
🟡 Issue: Bounded due-batch behavior is not directly tested
What's wrong
This PR makes the finalization inbox an autonomous retry queue, and the safety story depends on bounded SQLite reads plus serialized graph work. The new tests cover due filtering and ordering, but they do not prove that a positive limit is honored, so a future regression could process an unbounded backlog while this suite stays green.
Example
Seed three due RECEIVED rows, call await store.listDue(1), and expect exactly the oldest single key. A companion processDueBatch(1) assertion could prove only one due row is processed per batch.
Suggested direction
Add a regression test with more due entries than the requested positive limit, not just the zero-limit edge case.
For Agents
Look at packages/agent/test/finalization-recovery-sqlite-store.test.ts near the new listDue coverage. Add a case where due rows exceed the requested positive limit and assert oldest-first truncation; optionally verify FinalizationRecovery.processDueBatch(1) only processes one row to preserve worker backpressure.
Round 4 — re-review at
|
Round 5 —
|
| } | ||
| const store = this.getStore(); | ||
| if (!store) return 'none'; | ||
| const entry = await store.get(snapshot.key); |
There was a problem hiding this comment.
🔴 Bug: New store methods break old recoveryStore implementations
What's wrong
The PR extends the recovery-store contract and then calls the new method unconditionally. Existing callers that inject a custom recovery store compiled or written against the previous contract will either fail TypeScript compatibility or crash at runtime in reconciliation/background replay paths.
Example
A JS integration that provided the previous recovery store shape can still pass an object at runtime. handleChainReconciledKC() now enters replayMatching(), reaches replayEntry(), and throws TypeError: store.get is not a function, aborting reconciliation instead of preserving the prior store behavior.
Suggested direction
Keep get/listDue optional for externally supplied stores or gate autonomous replay on a store capability check, so older stores degrade to the previous reconciliation-only behavior instead of throwing.
Confidence note
This matters if callers can supply their own FinalizationRecoveryStore through FinalizationHandlerOptions.recoveryStore or deep imports from dist/*; internal SQLite usage is updated.
For Agents
Look at FinalizationRecoveryStore, FinalizationHandlerOptions.recoveryStore, FinalizationRecovery.replayEntry, and worker startup. Preserve old custom-store behavior by feature-detecting the new autonomous-retry methods, splitting the autonomous worker interface from the base store interface, or explicitly gating this behind a documented major-version contract change. Add a compatibility test with an old-shape recovery store proving chain reconciliation does not throw.
| } | ||
|
|
||
| describe('graph-scoped finalization recovery admission', () => { | ||
| it('autonomously settles a durable RECEIVED entry without a chain-cursor replay', async () => { |
There was a problem hiding this comment.
🟡 Issue: Decompose the finalization recovery test file before letting it sprawl further
What's wrong
This PR appears to push an already broad test module well past the 1k-line maintainability boundary by adding a large autonomous-worker section directly into it. The result is harder to navigate and makes unrelated recovery concepts compete in one file.
Example
The first nine tests in this describe block all exercise the autonomous due-worker path, but they live in the same 1.5k-line file as parsing, admission, settled replacement, reorg, and unsupported-capability tests. A future change to worker backoff now requires scanning a broad mixed-purpose suite.
Suggested direction
Move the worker-specific tests into a dedicated file and keep this file focused on admission/replay semantics. Shared setup helpers would remove much of the repeated store/recovery construction without changing behavior.
For Agents
Split the autonomous due-worker/backoff/metrics/restart/concurrency cases from packages/agent/test/finalization-recovery.test.ts into a focused test file such as finalization-recovery-autonomous.test.ts, and extract shared store/recovery factory helpers if needed. Preserve the same assertions and vitest unit config inclusion.
Correction — my round-4/round-5 "red test" finding was wrongI reported Root cause. In my review worktree, The agent test resolves metrics.finalizationRecoveryDueEntries?.record(health.dueEntries);it silently no-opped. That produces exactly the symptom I chased: no exported datapoint, Proof: What this retracts:
What still stands, and was worth doing: the teardown fix in Also unaffected and still open as a follow-up: My mistake, concretely: a reviewer running suites out of a worktree must build the dependency closure before trusting a failure. I had already seen this exact class today — a stale |
3 participants
Summary
This PR turns the durable SQLite finalization inbox into an executable retry queue.
It follows the crash-safe admission and guarded replay foundation introduced in #1939. That work persists the raw finalization message as
RECEIVEDbefore store-heavy processing and requires canonical chain evidence before promotion. The missing piece was autonomous progress: a durable row was reconsidered only if chain reconciliation happened to revisit the same graph, UAL, and KA.This PR adds a lifecycle-owned asynchronous recovery worker that:
The worker is an asynchronous Node.js event-loop task, not an OS worker thread. It does not block startup. The per-entry mutex is intentionally not a global mutex: independent live finalizations remain governed by the existing store scheduler.
Problem
The inbox already prevented finalization messages from being lost under store pressure, but persistence alone did not guarantee progress:
RECEIVED.next_attempt_at = NULLor an expired deadline.currentwithout revisiting it.capacity-exhaustedeven after Blazegraph and the normal store queue recovered.next_attempt_atwas an eligibility gate, not a schedule.Failure sequence before this PR
sequenceDiagram participant G as Finalization gossip participant H as Finalization handler participant I as SQLite inbox participant S as Store scheduler participant R as Chain reconciler G->>H: finalization message H->>I: persist RECEIVED I-->>H: durable admission succeeds H->>S: immediate verification and materialization S-->>H: busy, deferred, or interrupted opt outcome is recorded H->>I: record attempt and optional deadline end R->>R: watermark already covers ordinal R-->>R: return current without exact replay Note over I,R: No autonomous due-row consumer Note over I: RECEIVED or VERIFIED remains live G->>I: later finalizations consume the graph quota I-->>G: capacity-exhausted at 64 live graph rowsBase implementation
listDue(limit)using the existing retention bound.Review findings addressed
Commit
5c1d430eaaddresses every actionable finding from the post-implementation review.updated_atevery minute, preventing raw TTL eviction and making the capacity leak immortal.RECEIVED,VERIFIED, orREORGEDto audited terminalREJECTED, immediately releasing live capacity. Terminal retention still bounds disk use.REJECTED, proves a replacement is admitted atmaxEntries: 1, and proves duplicate gossip cannot revive materialization.applyandreplayVerified; maximum same-key materialization concurrency is 1.VERIFIEDhad no age-eviction path and could permanently consume capacity.VERIFIEDas well asRECEIVEDandREORGED; it does not silently delete live evidence bycreated_at.VERIFIED.startRecoveryWorkerandstopRecoveryWorker, and drives actual agentstart()andstop().next_attempt_at; reconciliation remains immediate while sharing the entry lock.dueEntriesandoldestDueAgeMs. OpenTelemetry exportsdkg.finalization_recovery.due_entries,dkg.finalization_recovery.oldest_due_age_ms, anddkg.finalization_recovery.attempts_total{outcome}.listDue; builds type-check the metric surface.Round 2 review findings addressed
Commits
c98c796a5and5f23ae4e1address the new review findings against92e0efeca.apply; if the live lane promoted the row toVERIFIED, the worker could enterreplayVerifiedconcurrently without the test noticing.applyandreplayVerified.capacity-exhausted.canonical-finalization-receipt-unsupportedis now a fallback only when the store has no degraded reason.capacity-exhaustedwins.listDuefailed because metric refresh occurred only on the success path.finallywhenever a store exists. Metric snapshot failures remain contained and logged.listDueto reject and proves the health snapshot still runs.Setequality assertion verified distinct chain targets but not the number of calls.processDueBatchexception was handled in production but the worker reschedule path was not regression-tested.catch/finallystops scheduling after one rejection.deferredRetryDelayalso acts as an implicit autonomous-vs-reconciliation mode flag.oldestDueAgeMsis based oncreated_at, so it reports age since receipt rather than age since the last retry/progress.MIN(updated_at)rather than silently changing the meaning of the existing series.Retry-budget sequence after Round 2
sequenceDiagram participant G as Duplicate live gossip participant I as Durable inbox participant W as Recovery worker participant C as Live capacity loop duplicate or reconciliation traffic G->>I: serialize by entry key G->>I: record outcome and monotonic deadline Note over I: attempt count may cross 10,080 end W->>I: reload due row under the same entry lock alt attempt ceiling reached but age is under seven days I-->>W: keep row live W->>I: guarded replay and backoff else attempt ceiling and age window both expired W->>I: generation-CAS live row to REJECTED I-->>C: release live quota endRound 2 validation
Round 3 review findings addressed
Commit
697c7308aaddresses the standalone findings added against5f23ae4e1.retry-pending, and the worker continues with the remaining snapshot.SETTLED. Removing the boundary makes the batch reject immediately.store.health(), so deleting or corrupting the actual gauge writes would not fail the test.listDuepath with known health values, flushes, and asserts both exported gauge datapoints.stopRecoveryWorker()was called, but not thatDKGAgent.stop()awaited it before dependent teardown.awaitwithvoidmakes teardown advance and the test fail before the gate is released.Per-entry failure isolation
sequenceDiagram participant W as Recovery worker participant I as Durable inbox participant P as Poison entry participant V as Later valid entry W->>I: listDue limit 16 I-->>W: poison entry, then valid entry W->>P: guarded replay P-->>W: escaped error W->>I: best-effort record failure and backoff Note over W: keep processing this bounded snapshot W->>V: guarded replay V-->>W: recovered W->>I: generation-CAS to SETTLEDRound 3 validation
awaitfails the lifecycle gate.git diff --check: passed.Round 5 small review fix
The Round 4 review found one blocking defect in the new metrics regression test. The production recovery path already refreshes due-inbox metrics from a
finallyblock, but the test could both hide its real assertion and bind its instruments to the wrong OpenTelemetry provider.trypathEBUSYand replaced the useful assertion failure.tryand close it first infinally, before removing the temporary directory.metrics snapshot failedwarning occurs.No production behavior changed in this round. The test now proves the intended failure-path sequence without allowing cleanup or global telemetry state to obscure the result:
sequenceDiagram participant Test participant Recovery as FinalizationRecovery participant Store as SQLite inbox participant Meter as OTel meter provider participant Exporter Test->>Meter: disable prior provider Test->>Meter: register provider and assert success Test->>Recovery: processDueBatch(16) Recovery->>Store: listDue(16) Store-->>Recovery: throw due-read error Recovery->>Store: health() from finally Store-->>Recovery: dueEntries=7, oldestDueAgeMs=4321 Recovery->>Meter: record both gauges Recovery-->>Test: return 0 Test->>Meter: forceFlush() Meter->>Exporter: export datapoints Test->>Exporter: assert both gauge values Test->>Store: close() in finally Test->>Test: remove temporary directoryValidation on
6d20e82d1:finalization-recovery.test.ts: 25/25 passed.git diff --check: passed.Round 4 medium review
Commit
9622adc28addresses the contained test gap added after Round 3.SETTLEDrecovery lacked direct coverage.processDueBatch(16).SETTLEDrow, applies upgraded access semantics once, advances generation 0 to 1, and clearspublisherUpgradePending.Round 4 validation
SETTLEDcase uses a real close/reopen boundary and autonomousprocessDueBatch; it does not callreplayMatching.git diff --check: passed.Recovery sequence
sequenceDiagram participant L as Agent lifecycle participant W as Recovery worker participant I as SQLite inbox participant C as Chain adapter participant R as Guarded recovery path participant S as Graph store L->>W: start after inbox and runtime are ready loop while agent is running W->>I: listDue limit 16 I-->>W: oldest due snapshot loop each row, strictly serial W->>C: resolve KA context-graph binding C-->>W: current on-chain graph id W->>R: replay persisted raw finalization R->>C: resolve canonical receipt R->>R: validate placement, authority, and target R->>S: existing verified materialization alt applied or already applied R->>I: generation-CAS to SETTLED else busy, deferred, or chain evidence pending R->>I: record outcome and monotonic backoff end end alt batch contained 16 rows W->>W: schedule next batch immediately else partial or empty batch W->>W: poll after 5 seconds end end L->>W: stop W->>W: cancel timer and await active batch W-->>L: drained L->>S: continue graph-store shutdownSame-entry serialization
sequenceDiagram participant L as Live gossip participant W as Recovery worker participant M as Per-entry FIFO mutex participant I as SQLite inbox participant S as Graph store L->>M: acquire entry key W->>M: wait for same entry key L->>I: receive or reload durable row L->>S: verify and materialize L->>I: generation-CAS to SETTLED L-->>M: release M-->>W: acquire W->>I: reload after lock I-->>W: state is SETTLED W-->>M: no duplicate graph write; releaseThe reverse ordering has the same result: whichever lane acquires the key first may materialize; the second reloads current durable state rather than trusting a stale snapshot.
Bounded poison-row lifecycle
sequenceDiagram participant W as Recovery worker participant I as SQLite inbox participant C as Live capacity loop due while count or age budget remains W->>I: reload due live row W->>W: canonical validation or materialization W->>I: record failure and monotonic backoff end W->>I: reload after count ceiling and age window W->>I: generation-CAS live state to REJECTED I-->>C: row no longer counts against live quota Note over I: Terminal row remains auditable and is bounded by terminal retention C-->>W: new finalizations can be admittedThis deliberately avoids changing raw pruning to
created_at: a long but legitimate RPC or reorg outage retains durable evidence, duplicate traffic cannot burn the lifetime early, and a poison row still has a finite autonomous lifecycle once both the count and wall-clock bounds expire. Identical gossip for a terminal row is handled without graph work until retention removes that row.Batching and load bounds
At the existing global cap of 128, a fully due inbox needs at most eight 16-row selections to inspect every row. A graph at its 64-row cap spans at most four full selections. Completion time remains dominated by serial chain and graph work.
Measured recovery and concurrency evidence
RECEIVEDrows withattempt_count=0,last_error=NULL, andnext_attempt_at=NULLSETTLEDin 416 ms; health changed fromcapacity-exhaustedto ready; the 65th row changed from rejected to admittedRECEIVEDwith zero attempts after 20 secondsapplyentries in 3 of 5 runsapplyandreplayVerifiedThe 416 ms result is an in-process real-SQLite review benchmark, not a production Blazegraph latency claim.
Consistency and security invariants
This does not create an alternate or weaker finalization path. The worker calls the #1939 recovery machinery:
Lifecycle and SQLite compatibility
stop()cancels the timer and awaits the active batch.RECEIVED,VERIFIED, andREORGED, plus eligibleSETTLEDpublisher-upgrade or receipt-retry work.Validation
New coverage
Commands and results
SIGKILLbut observed exit code 13 under the loaded full run; its immediate isolated rerun passed 3/3.StreamStateErrorunhandled rejection fromtest/libp2p-network.test.tsand exited non-zero.pnpm --filter @origintrail-official/dkg-core build: passed.pnpm --filter @origintrail-official/dkg-agent build: passed (tsc, type tests, package-root test).git diff --check: passed.Risk and rollout
The worker uses conservative fixed bounds. It does not raise inbox capacity, silently delete live evidence, or bypass chain evidence. It contributes at most one serial recovery operation; same-entry live/replay work is mutexed, while independent live operations remain under the existing store scheduler.
After deployment, old eligible rows will begin moving through attempts. Rows whose chain evidence or store capacity remains unavailable keep durable evidence with monotonic deadlines. A row that exhausts both the default attempt ceiling and wall-clock window becomes audited terminal
REJECTEDand releases live capacity.Operators can alert on:
dkg.finalization_recovery.due_entries;dkg.finalization_recovery.oldest_due_age_ms;dkg.finalization_recovery.attempts_total{outcome};Scope boundary
This PR fixes autonomous progress for the durable finalization inbox. It does not change Blazegraph atomic graph replacement, store-scheduler admission, reconciliation watermark semantics, or inbox capacity limits. It does not claim that every atomic graph-replace 500/deadline has this root cause; it removes the code-level condition that can leave safely persisted finalizations permanently unconsumed after transient pressure.
Related