fix(agent): relieve repeated sync pressure - #2053
Conversation
| // Use the exact same memory-only canonicalizer as the suppression gate. | ||
| // Curator resolution may connect a missing peer, but it must not substitute | ||
| // a different ranking algorithm and invalidate an otherwise stable roster. | ||
| const orderedPeerIds = this.vmReconcileObservedCandidatePeerIds(localCgId); |
There was a problem hiding this comment.
🔴 Bug: Resolved curator peers can be dropped from the exact-recovery proof roster
What's wrong
The code still resolves and connects curator peers, but the added canonical roster ignores those resolved IDs unless one also happens to be the single preferred peer or a known core. Because the roster is capped before the exact fetch loop, an authoritative curator can be excluded from both fetching and clean-absence evidence. That can cause the node to back off a missing ordinal after asking unrelated peers instead of asking the curator most likely to have the asset.
Example
If a wallet-scoped CG resolves registry curator peerD, preferredSyncPeers is empty, no peers are known cores, and connected peers sort as [peerA, peerB, peerC, peerD], the new roster is capped to [peerA, peerB, peerC]. Exact recovery can collect clean absence from those three and enter backoff even though the resolved curator peerD was just dialed and may have the missing KA.
Suggested direction
Carry curatorPeerIds into the roster used for orderedPeerIds or otherwise rank resolved curators ahead of generic connected peers before applying the three-peer cap, while keeping the intended stable rotation behavior.
For Agents
In recoverVmReconcileBatch, preserve the resolved curator peer IDs when building the capped exact-recovery roster. Add a scenario where the registry curator is not preferredSyncPeers, not a known core, and would be outside the first three canonical connected peers; the test should prove exact recovery attempts that curator before clean-absence backoff can be recorded.
There was a problem hiding this comment.
Fixed in 3151cd5. Exact recovery now caches the resolved structural curator IDs per CG and ranks connected cached curators ahead of ordinary peers before applying the three-peer cap. The pre-network suppression pass reuses that memory-only roster on later sweeps, so it adds no discovery/chain/dial work. Added a regression with three ordinary peers and a fourth non-preferred/non-core curator that alone returns the asset; the first physical request now goes to the curator.
There was a problem hiding this comment.
🔴 Bug: Exact-recovery backoff is lost when peers disconnect before the next sweep
What's wrong
The new rotation state is meant to damp repeated exact recovery after every candidate has cleanly reported absence. However, the pre-network suppression pass uses only currently connected peers. When that list is empty, it deletes the stored rotation record, including completed backoff evidence. That makes backoff depend on sockets staying open and can re-trigger network recovery every sweep after ordinary disconnects.
Example
A slot has already completed a clean-absence rotation and is in backoff until nextRetryAt. If all peers disconnect before the next sweep, observedCandidatePeerIds is []; line 3655 deletes the backoff record, the method proceeds to resolve/dial the curator, installs a fresh collecting record, and sends another exact fetch immediately instead of suppressing until nextRetryAt.
Suggested direction
Do not treat an empty transient connection view as an authoritative roster change for an existing backoff record. Keep the record and suppress while now < nextRetryAt, unless the target fingerprint changes or a non-empty authoritative roster invalidates it.
For Agents
In dkg-agent-swm-host.ts, preserve active backoff records when the observed pre-dial roster is empty, or move suppression to a point where cached/resolved curator identity can be considered. Add a test where a clean-absence backoff exists, getConnections() is empty before curator dialing, and recovery remains suppressed until nextRetryAt.
There was a problem hiding this comment.
🟡 Issue: Backoff can suppress the curator lookup that would make recovery succeed
What's wrong
A clean-absence backoff gathered while curator discovery was unavailable can be replayed before the next curator lookup. That makes ordinary peers' empty responses temporarily block the code from discovering and querying the actual curator once discovery recovers, delaying VM repair unnecessarily.
Example
For a structural CG with no cached curator peers: pass 1 has resolveCuratorPeerIdsForCg return lookupFailed: true, two ordinary connected peers return fresh empty exact responses, and the slot enters backoff. On pass 2 the registry is healthy and would resolve the curator holding the KA, but initiallyEligible.length === 0 returns before curator resolution, so recovery waits for nextRetryAt instead of trying the now-authoritative curator.
Suggested direction
Only honor exact-absence backoff before curator resolution when the stored candidate roster is known to include the authoritative/cached curator set, or defer suppression until after a fresh curator lookup when the previous cycle was built during lookup failure.
Confidence note
This depends on the intended authority of clean-absence evidence from non-curator peers, but the changed code explicitly treats failed curator lookup as non-authoritative elsewhere, so the backoff should not prevent the next curator lookup.
For Agents
In recoverVmReconcileBatch, distinguish rotation records collected without an authoritative/cached curator roster from records collected against the authoritative roster. Preserve the existing shutdown/stale-target guards, but keep failed-lookup/no-cache cycles fail-open or force curator resolution before honoring their suppression. Add a test where first curator discovery fails with only ordinary clean-absent peers, then the next pass resolves a curator and fetches immediately despite the prior ordinary absence.
There was a problem hiding this comment.
🔴 Bug: Disconnected cached curators can make ordinary-peer absence look authoritative
What's wrong
The backoff proof can be built from ordinary connected peers while being marked as curator-confirmed solely because a stale cached curator list exists. That can delay or block VM recovery from the actual curator after a transient dial/discovery failure.
Example
Cached curator peer C exists from a prior successful lookup, but ensurePeerConnected(C) fails during this sweep. Ordinary peer A is connected, so orderedPeerIds is [A]; line 4278 still marks the roster confirmed because the cache is non-empty. If A returns clean-absent, the slot enters backoff, and the next sweep skips before retrying discovery or dialing C even if C is now available.
Suggested direction
Compute confirmation from the candidate roster used for proof collection. If curator discovery failed and no cached curator peer is present in the connected candidate set, keep the rotation unconfirmed so clean-absence evidence cannot suppress the next discovery attempt.
For Agents
In recoverVmReconcileBatch, make curatorRosterConfirmed depend on the actual proof roster, not just the existence of cached curator ids. Preserve suppression only when discovery succeeded or at least one cached authoritative curator is included in orderedPeerIds; add a test where cached curator connection fails, an ordinary peer returns clean-absent, and the next pass still retries curator discovery/dialing.
| }; | ||
| } | ||
|
|
||
| function classifyExactDurableFetch(params: { |
There was a problem hiding this comment.
🟡 Issue: Decompose exact-fetch classification instead of pushing durable-sync.ts past 1k lines
What's wrong
The new proof-specific classification logic is added directly to an already oversized orchestration file, crossing the 1000-line threshold without a strong structural reason. It also leaves identical merge semantics duplicated in lifecycle code, which makes this harder to maintain than a single canonical exact-disposition boundary.
Example
The durable sync requester now owns exact payload filtering, exact disposition classification, disposition merging, fetch orchestration, verification, checkpointing, and storage in one file. A reader changing clean-absence semantics has to edit this file and separately remember the lifecycle-level merge helper.
Suggested direction
Create a small exact-durable-disposition helper module for filterExactAssetDurablePayload, classifyExactDurableFetch, and mergeExactDurableFetchDisposition. Keep the main durable sync loop focused on orchestration and have lifecycle reuse the same exported merge function.
For Agents
Extract exact asset filtering/classification/merge helpers from sync/requester/durable-sync.ts into a focused module, then import the shared merge helper in dkg-agent-lifecycle.ts. Preserve runDurableSync returning only the public result and runDurableSyncDetailed returning the optional disposition.
There was a problem hiding this comment.
Addressed in 3151cd5. Added sync/requester/exact-durable-fetch.ts as the canonical home for filtering, classification, disposition typing, and merge semantics. durable-sync.ts imports/re-exports the compatibility surface, and lifecycle now imports the same merge helper instead of duplicating it.
There was a problem hiding this comment.
🟡 Issue: Fold exact-fetch disposition into the durable-sync accumulator
What's wrong
The implementation bolts exact-fetch disposition onto durable sync as a separate mutable side channel instead of extending the existing aggregation model. That increases orchestration complexity and makes the detailed result depend on scattered updates in every branch that can complete, defer, skip, abort, or fail a context graph.
Example
The durable-sync path now has to remember both markDurableTerminalBoundary(accumulator, ...) and exactFetchDispositions[...] = settledExactDisposition(); lifecycle-level deferral/cancellation paths similarly have to call markExactFetchIncomplete(). Any future early return or new mark path has two summaries to keep in sync.
Suggested direction
Add exact disposition to the accumulator shape, or introduce a small DetailedDurableSyncAccumulator with a single merge function. Then runOrderedContextGraphSyncs and runDurableSyncWithBudget can merge one result object instead of maintaining a second mutable side channel.
For Agents
Look at runDurableSyncWithBudget, runLegacyDurableSyncDetailed, and the durable accumulator helpers. Preserve the public runDurableSync projection and exact coalescing behavior, but move exact disposition into the same accumulator/merge model used for durable sync completion. Existing multi-CG exact disposition tests should still pass.
| private readonly hooks: PriorityAdmissionQueueHooks<Payload>; | ||
| private readonly now: () => number; | ||
| private nextSequence = 0; | ||
| private agedTurnOwed = false; |
There was a problem hiding this comment.
🟡 Issue: Make fairness debt an explicit scheduler policy instead of scattered queue state
What's wrong
The new single boolean agedTurnOwed is a hidden state machine whose transitions are split between selection, pumping, acquisition, removal, timeout, and abort paths. That makes the scheduler harder to reason about and increases the chance that the next queue feature bypasses one of the debt invariants.
Example
To understand why a low-priority aged entry starts next, a reader has to combine selectNext() producing createsDebt/servesDebt, pump() mutating agedTurnOwed after start(), and queueChanged()/reconcileAgedTurnOwed() clearing the flag on cancellation, timeout, or removal.
Suggested direction
Collapse the aging/debt rule into a pure selector or small policy object, for example selectNextAdmission(queue, now, debt) returning the selected index plus the next debt state. That would keep debt creation, debt service, and stale-debt clearing in one place instead of distributing the invariant across queue operations.
Confidence note
The current behavior may be well covered, but the concern is structural: the new debt rule is spread across state, selection, pump, and queue mutation rather than isolated as one policy.
For Agents
Look at PriorityAdmissionQueue selection and debt handling. Preserve numeric priority, one-overtake debt, cancellation/timeout cleanup, handoff behavior, and start-failure behavior; the existing scheduler tests should continue to prove those cases.
There was a problem hiding this comment.
Validated as a structural concern. The current change keeps the debt state local to PriorityAdmissionQueue and now also makes start failure rollback explicit and tested, but I am deferring a pure-selector extraction to a dedicated no-behavior-change follow-up. Combining that refactor with the scheduler semantic change would make this liveness PR harder to audit without changing its runtime result.
There was a problem hiding this comment.
🟡 Issue: Make fairness debt an explicit scheduling policy instead of hidden queue side state
What's wrong
The new fairness behavior is implemented as a mutable side-channel flag that several queue operations must remember to maintain. This works against the queue’s already subtle admission lifecycle and makes future changes to cancellation, timeout, displacement, or start failure harder to reason about.
Example
Adding another path that removes or rejects a queued entry would have to know to call the right reconciliation hook, or the queue can retain/clear fairness debt inconsistently. That coupling is not visible from the remove/reject operation itself.
Suggested direction
Move the aged-debt transition logic into a small pure helper that takes queued entries and current debt and returns the selected entry plus next debt state. That keeps queue mechanics, capacity side effects, and fairness policy from being interleaved.
Confidence note
This is a maintainability concern from the changed structure; the existing tests may cover the current behavior, but the debt model remains implicit in queue internals.
For Agents
Look at packages/agent/src/sync/priority-admission-queue.ts. Preserve the one-overtake fairness behavior, but extract selection/debt transitions into a pure scheduler policy or represent debt as an explicit owed aged entry/sequence. Tests in sync-backpressure.test.ts should continue proving overtakes, cancellation, timeout, handoff, and start-failure behavior.
There was a problem hiding this comment.
🟡 Issue: Fairness debt is modeled as an implicit queue-wide flag
What's wrong
The scheduler now has hidden cross-call state that is only valid if every queue mutation path remembers to call the right reconciliation hook. That makes the admission queue harder to reason about and raises the cost of future changes to displacement, handoff, timeout, or start-failure behavior.
Example
When a high-priority entry overtakes an aged low-priority entry, agedTurnOwed becomes true. Later cancellation, timeout, start failure, or temporary unrunnability relies on separate calls to reconcileAgedTurnOwed() from multiple paths to keep that global bit meaningful.
Suggested direction
Make the fairness debt an explicit scheduling concept instead of a bare boolean updated opportunistically. A small policy object or pure selector that owns debt creation, repayment, and invalidation would reduce the number of places that must remember to reconcile this state.
Confidence note
This is a design-risk finding from the diff structure. The current behavior may be intentional, but the maintainability issue is that a single boolean encodes fairness debt without naming the owed entry or policy state explicitly.
For Agents
Look in packages/agent/src/sync/priority-admission-queue.ts around agedTurnOwed, selectNext, reconcileAgedTurnOwed, remove, and queued-start failure handling. Preserve the current scheduling semantics, but refactor the fairness state into an explicit scheduler policy/helper that returns {entry, decisionEffects} or tracks the owed aged sequence/owner rather than a bare queue-wide boolean. Prove with existing fairness tests plus cancellation, timeout, start-failure, and handoff tests.
There was a problem hiding this comment.
🟡 Issue: Model scheduler fairness debt explicitly instead of as a bare boolean
What's wrong
The new fairness behavior is subtle, but the state model is too lossy. A single boolean hides which queue entry earned the debt and why, so the scheduler now depends on scattered reconciliation calls to keep the flag meaningful.
Example
If one aged low-priority entry is overtaken, then cancelled, displaced, or becomes unrunnable, the boolean debt can later be consumed by a different aged entry. That may be intended, but the code does not encode that policy explicitly, so the invariant is hard to reason about from the type/model.
Suggested direction
Use a small typed state object for the fairness debt or fold the rule into one comparator that can explain which entry is protected and why. That would make cancellation, timeout, displacement, handoff, and temporary unrunnability easier to verify without relying on incidental boolean resets.
Confidence note
This is a maintainability concern about the scheduler model, not a claim that the current tests fail.
For Agents
Replace agedTurnOwed: boolean with an explicit debt model, such as a sequence/owner/priority snapshot or a documented overtake counter, and make selectNext consume that model directly. Preserve the intended one-overtake behavior and queue/displacement semantics with the existing scheduler tests.
There was a problem hiding this comment.
🟡 Issue: Give priority-aging debt a single owner instead of a hidden queue-wide flag
What's wrong
The queue now has an implicit second state machine layered on top of the queued entries. Because agedTurnOwed is a boolean with cleanup calls scattered through the class, the invariant is not locally obvious and future queue changes will have to remember to reconcile it in every path that can remove or start an entry.
Example
When a high-priority entry overtakes an aged one, selectNext() returns createsDebt, pump() flips agedTurnOwed, later queue removal or timeout calls queueChanged() to reconcile it, and the next selectNext() may serve the oldest aged entry. The policy is spread across selection, mutation, and cleanup instead of being owned by one scheduler model.
Suggested direction
Represent fairness debt explicitly, ideally as part of a scheduler policy with selectNext and afterStart/afterRemove transitions, or track the owed aged sequence rather than a bare boolean. That would let selection and debt cleanup live in one place.
Confidence note
This is a maintainability finding, not a claim that the current fairness behavior is wrong.
For Agents
In packages/agent/src/sync/priority-admission-queue.ts, pull the aging/fairness policy into a small explicit scheduler state object or pure selection helper. Preserve raw-priority ordering, one-overtake debt, cancellation/timeout cleanup, and displacement protection. Existing sync-backpressure tests should continue proving the same ordering.
There was a problem hiding this comment.
🟡 Issue: Make priority aging debt an explicit scheduler policy instead of a hidden flag
What's wrong
The shared scheduler now has implicit temporal state whose lifecycle is scattered across queue admission, pumping, timeout/cancel cleanup, and victim selection. That makes the generic queue harder to evolve because fairness behavior is no longer localized to a single selection policy.
Example
To understand one scheduling decision, a reader has to simulate selectNext() plus the previous pump() result plus whether remove() or a timeout cleared the debt. The debt is not represented as a typed scheduling decision or tied to the entry/sequence that created it.
Suggested direction
Replace the bare boolean with a typed fairness state and centralize all updates around the scheduling decision. A pure selectNext(queue, capacity, fairnessState) style helper would make the generic queue much easier to maintain.
For Agents
Refactor PriorityAdmissionQueue so fairness policy is explicit and centralized. Preserve the new one-overtake behavior, but make selection a small policy object or pure function that returns both the selected entry and the next fairness state; then mutate queue state in one place after successful start.
There was a problem hiding this comment.
🟡 Issue: Model scheduler fairness as an explicit policy, not scattered mutable debt
What's wrong
The new fairness behavior is implemented as a hidden queue-wide boolean that is reconciled opportunistically from acquire, selectNext, remove, and pump. This is exactly the kind of stateful special-case branching that makes shared scheduling code fragile: the invariant is not represented as a clear model, it is maintained by scattered calls in several control paths.
Example
To know why the next item starts, a reader has to inspect reconcileAgedTurnOwed(), then selectNext(), then the post-start mutation in pump(). A direct selector like selectNext({ debt }) -> { entry, nextDebt, reason } would make the transition explicit and testable without hidden side effects.
Suggested direction
Collapse aged-turn handling into a dedicated scheduler policy object or pure selector result. The queue should own mechanics; the policy should own priority-vs-aging decisions. That would make the fairness invariant legible and reduce the chance that future queue operations forget to reconcile the hidden debt bit.
For Agents
Refactor PriorityAdmissionQueue so admission selection is a small pure policy function that receives queued entries, current time, and current fairness state, then returns the selected index plus next fairness state. Keep the same scheduling behavior and metrics outcomes, but remove selectNext/reconcileAgedTurnOwed side effects from multiple call sites.
| ); | ||
| } | ||
|
|
||
| async syncExactKnowledgeAssetsFromPeerDetailed(this: DKGAgent, |
There was a problem hiding this comment.
🟡 Issue: Collapse the duplicated public/detailed exact-sync wrappers
What's wrong
The new detailed API adds a second wrapper that manually mirrors the existing exact-sync setup. This is thin indirection with drift risk: the two methods express the same operation but now have to stay synchronized by convention.
Example
Both wrappers call requireExactAssetUals, create a sync operation context, pass [contextGraphId], set exactAssetUals, stopOnBackoffWorthyFailure: true, priority: 1_000, and source: 'vm-recovery'. A future timeout/source/priority change has to be mirrored in both methods.
Suggested direction
Make the detailed exact-sync path the single implementation and let the public method be a projection over it. That removes the duplicated option assembly while keeping the new disposition available to VM recovery.
For Agents
In dkg-agent-lifecycle.ts, introduce one private/shared exact-sync helper that returns the detailed result. Have syncExactKnowledgeAssetsFromPeer project .result and syncExactKnowledgeAssetsFromPeerDetailed return the detailed shape. Preserve single-flight coalescing and the public method's result shape.
There was a problem hiding this comment.
Addressed in 3151cd5. syncExactKnowledgeAssetsFromPeerDetailed is now the single option-assembly and physical implementation, while the public method only projects .result. Reverse-order public+detailed callers also share the canonical exact-set single-flight identity.
There was a problem hiding this comment.
🟡 Issue: Collapse the parallel detailed durable-sync lifecycle APIs
What's wrong
The PR threads one extra exact-fetch disposition by duplicating multiple lifecycle methods. That adds indirection and synchronization burden without deleting complexity; it also makes exact-recovery details leak through general durable-sync orchestration layers.
Example
A future change to durable sync options now has to stay aligned across runLegacyDurableSync, runLegacyDurableSyncDetailed, runLegacyDurableSyncForContextGraph, runLegacyDurableSyncForContextGraphDetailed, and the exact-sync pair, even though the public/detailed difference is just whether the exact disposition is exposed.
Suggested direction
Use one canonical internal durable-sync execution path and one small projection wrapper for legacy callers. Avoid maintaining parallel lifecycle methods whose signatures and option plumbing must remain identical.
Confidence note
This is a maintainability finding; the current behavior may be intentional, but the new surface area is noticeably larger than the extra datum being returned.
For Agents
Review packages/agent/src/dkg-agent-lifecycle.ts around the durable sync wrappers. Preserve public return types and single-flight behavior, but make the detailed physical run the single internal path. Then expose projection only at the public boundary, or report exact disposition through a narrow exact-recovery observer instead of cloning lifecycle methods.
There was a problem hiding this comment.
🟡 Issue: Avoid mirroring the durable-sync API just to carry exact-fetch disposition
What's wrong
The PR adds a second, nearly identical durable-sync call chain whose main purpose is to expose one extra VM-recovery datum. That doubles the lifecycle surface area readers have to follow and creates long-term drift risk: every future durable-sync change now has to be threaded through both normal and detailed variants.
Example
Adding any future durable-sync option or callback now requires keeping the public method, detailed method, per-context public method, per-context detailed method, requester runDurableSync, and requester runDurableSyncDetailed in sync, even though the new disposition is only meaningful for exact-asset recovery.
Suggested direction
Prefer one canonical internal return shape with a projection at the public boundary, or keep the detailed return scoped to the exact-asset helper instead of creating parallel Detailed variants through the whole legacy durable-sync stack.
For Agents
Look in dkg-agent-lifecycle.ts and sync/requester/durable-sync.ts. Preserve the public result shape, but collapse to one canonical internal durable-sync result or isolate the exact-asset path behind a single runExactKnowledgeAssetSync helper that returns { result, disposition }. Add/adjust tests that prove public callers still receive only DurableSyncResult and exact recovery still receives disposition.
There was a problem hiding this comment.
🟡 Issue: Fold exact disposition into the durable-sync accumulator instead of a closure side channel
What's wrong
The detailed exact-fetch outcome is now bolted onto the legacy durable orchestration as mutable outer state. That preserves behavior, but structurally it splits one operation summary across two aggregation mechanisms, which makes future changes to durable-sync result shape or exact-response classification easy to wire inconsistently.
Example
A deferred exact sync updates the durable summary through markDeferred(summary) and updates exact disposition through the captured markExactFetchIncomplete() closure. Those two summaries travel through different control paths even though they describe the same physical run.
Suggested direction
Make the detailed result the value that runOrderedContextGraphSyncs merges. A small createDetailedDurableSyncAccumulator/mergeDetailedDurableSyncAccumulatorInto would keep exact disposition, terminal-boundary marking, skipped/deferred handling, and final projection in one model.
For Agents
In dkg-agent-lifecycle.ts, introduce a composite detailed accumulator or extend the ordered sync accumulator so result and exactFetchDisposition are merged together. Preserve public runLegacyDurableSync as a projection of .result, preserve single-flight behavior, and cover multi-CG exact aggregation/deferred/skipped cases with the existing lifecycle tests.
| if (!isTargetCurrent() || targets.length === 0) return noRecovery(); | ||
|
|
||
| const expectedOnChainCgId = onChainCgId.toString(); | ||
| const currentTargets = targets.filter((target) => |
There was a problem hiding this comment.
🟡 Issue: Stale recovery-target filtering lacks a regression test
What's wrong
The PR adds localCgId and onChainCgId to recovery targets and uses them to discard targets that do not belong to the current batch. That is a safety boundary for exact VM recovery, but the tests only exercise correctly matched targets, so a regression in this filtering would not be caught.
Example
A regression test could call recoverVmReconcileBatch(currentCg, 1n, [targetFromOtherCgOrOnChainId], ...) with syncExactKnowledgeAssetsFromPeerDetailed throwing if called, then assert no fetch, no attempted ordinals, and no rotation state for the stale target. A mixed batch should assert only the matching target is fetched.
Suggested direction
Cover the new target identity contract at the batch boundary so a future change cannot accidentally fetch or suppress recovery for the wrong Context Graph slot.
Confidence note
The added recovery tests all build matching targets through vmRecoveryTarget; I did not find a case that passes stale or mixed-CG targets into recoverVmReconcileBatch.
For Agents
Add a focused test in packages/agent/test/core-fills-gap.test.ts around recoverVmReconcileBatch: pass mismatched localCgId/onChainCgId recovery targets and prove they are ignored without creating rotation state or consuming exact-fetch budget; also cover a mixed stale/current batch if continuation behavior matters.
There was a problem hiding this comment.
Addressed in 3151cd5. Added a batch-boundary regression with both a mismatched local CG target and a mismatched on-chain CG target. It proves no exact fetch, no attempted ordinal, and no rotation state is created for stale work.
There was a problem hiding this comment.
🟡 Issue: Empty-roster reset lacks a regression test
What's wrong
This branch is meant to prevent partial clean-absence evidence from surviving across a period where no candidate peers are observed. That is a meaningful suppression/backoff safety property, but the tests added in this PR keep at least one candidate present and would not fail if this reset were removed.
Example
Failing-test sketch: create a rotation record for [peerA, peerB], credit peerA as clean-absent, call prepareVmReconcileRotationTarget(target, [], now + 1), then call prepareVmReconcileRotationTarget(target, [peerA, peerB], now + 2) and assert the returned record has empty attemptedPeerIds and cleanAbsentPeerIds.
Suggested direction
Cover the empty-candidate branch directly, including the subsequent reconnect behavior, so stale absence evidence cannot survive a full peer-disconnect interval unnoticed.
For Agents
Add a focused test in packages/agent/test/core-fills-gap.test.ts near the rotation identity tests. It should prove that an empty observed candidate roster deletes partial absence evidence and that a later reconnect starts a fresh collection cycle.
There was a problem hiding this comment.
🟡 Issue: Terminal reconcile cleanup is not directly covered
What's wrong
The PR adds slot cleanup to direct terminal reconciliation paths, but the visible coverage mostly exercises cleanup through recoverVmReconcileBatch, which independently deletes the rotation slot after any non-pending outcome. That means a regression removing these reconcileChainOrdinal cleanup calls could pass while leaving stale process-local suppression state in inline reconciliation paths.
Example
Seed vmReconcileRotationState for a target slot, make reconcileChainOrdinal hit the recentReconciledUals fast path or return already-confirmed from handleChainReconciledKC, and assert that the slot is removed. Without the new clear call, the state would remain installed.
Suggested direction
Add direct regression coverage for reconcileChainOrdinal terminal paths that proves stale rotation/backoff evidence is cleared outside recoverVmReconcileBatch.
Confidence note
The batch recovery tests assert cleanup after terminal outcomes, but recoverVmReconcileBatch also deletes the slot itself, so they do not isolate these newly added reconcileChainOrdinal cleanup calls.
For Agents
Add focused reconcileChainOrdinal tests in packages/agent/test/core-fills-gap.test.ts that pre-populate a rotation record for the target slot and verify direct terminal paths clear it: recent cache hit, promoted, already-confirmed, and stale-target.
| | { status: 'skip' }; | ||
|
|
||
| export interface OrdinalRecoveryTarget { | ||
| localCgId: string; |
There was a problem hiding this comment.
🟡 Issue: Do not leak exact-recovery rotation identity into the pure chain reconciler target
What's wrong
The PR widens the generic reconciler contract with agent-local identity data. That couples the pure watermark engine to one recovery implementation and makes future callers/tests pay for fields they do not conceptually own.
Example
The tests now need a vmRecoveryTarget helper that fabricates localCgId, onChainCgId, and merkleRoot for every target even when the scenario only cares about ordinal scheduling.
Suggested direction
Keep OrdinalRecoveryTarget minimal or make the recovery payload owned by the VM recovery layer. The pure reconciler should not carry fields whose only purpose is process-local rotation/backoff bookkeeping.
For Agents
Keep chain-reconciler.ts focused on ordinal reconciliation. Move exact-recovery identity into an agent-local target type or construct a VmExactRecoveryTarget at the recoverVmReconcileBatch boundary from the batch context plus the pending outcome. Preserve the existing recovery callback behavior, and verify the chain reconciler tests no longer need VM rotation-specific fields.
There was a problem hiding this comment.
🟡 Issue: Avoid duplicating batch identity on every recovery target
What's wrong
The new target shape creates two sources of truth for the context graph identity: the method parameters and the per-target fields. That weakens the type boundary and pushes stale-target defense into incidental control flow rather than making the invariant clear at construction time.
Example
recoverVmReconcileBatch('current', 1n, [{ localCgId: 'other', onChainCgId: '1', ... }], ...) is now a representable state and is silently filtered out. The type permits contradictory batch identity, so the implementation has to defend against that contradiction in multiple places.
Suggested direction
Replace the duplicated fields with a single explicit batch/slot model so contradictory target identity is impossible or isolated at one boundary. That should remove the ad-hoc filtering and reduce repeated equality checks in the recovery loop.
For Agents
Start with OrdinalRecoveryTarget in packages/agent/src/chain-reconciler.ts and its construction in reconcileChainOrdinal. Preserve stale in-flight response protection, but model graph identity once: either pass a typed RecoveryBatch/RecoverySlot object through the flow, or keep targets context-free and derive the slot key from the batch boundary. Add focused tests for mismatched/stale target handling after the boundary is explicit.
| && phase.nextOffset === 0 | ||
| && phase.quads.length === 0 | ||
| ); | ||
| if (freshEmptyPhase(params.metaResult) && freshEmptyPhase(params.dataResult)) { |
There was a problem hiding this comment.
🟡 Issue: Data-phase freshness is not covered for clean-absence classification
What's wrong
The new exact-absence proof depends on both fetched phases being fresh and empty, but the added tests only prove the metadata-side guard. Because this disposition feeds VM recovery backoff, a future regression could classify a stale or resumed data phase as clean-absent without a test failing.
Example
A regression that changed this to check only freshEmptyPhase(params.metaResult) would still pass the current table. Add cases like runExact({ data: { responderSessionStartedFresh: false } }) and runExact({ data: { resumedFromOffset: 4, nextOffset: 4 } }), both expecting incomplete.
Suggested direction
Mirror the existing metadata-only negative cases for the data phase so the clean-absence proof is verified for both halves of the physical fetch.
For Agents
Look in packages/agent/test/sync-requester-progress.test.ts under exact durable fetch disposition. Add negative cases that make only the data phase stale/resumed while metadata remains a fresh empty response, and assert the detailed disposition remains incomplete.
| libp2p: { getConnections: () => [{ remotePeer: connectedPeer }] }, | ||
| }; | ||
| (internals as any).vmReconcileCuratorPeersByCg.set(localCgId, [cachedPeer]); | ||
| (internals as any).resolveCuratorPeerIdsForCg = async () => ({ |
There was a problem hiding this comment.
🔴 Bug: Curator discovery failure test stubs away the new failure detection
What's wrong
The PR adds lookupFailed as the signal that separates a discovery outage from a successful empty curator resolution, and recoverVmReconcileBatch uses that distinction to retain or clear cached authoritative curators. The added test asserts the caller behavior with a stubbed lookupFailed value, but it does not verify that real discovery exceptions produce that value, which is the fragile part of the change.
Example
A regression that changed resolveCuratorPeerIdsForCg to return { peerIds: [], lookupFailed: false } when discovery.findAgents throws would clear vmReconcileCuratorPeersByCg in recoverVmReconcileBatch, but the current retention test would still pass because it bypasses resolveCuratorPeerIdsForCg entirely.
Suggested direction
Exercise the actual discovery.findAgents failure path instead of precomputing lookupFailed in a resolveCuratorPeerIdsForCg stub.
Confidence note
The caller branch is covered, but I did not find a test that exercises the new resolver behavior through a throwing discovery registry.
For Agents
Add a focused regression test using the real resolveCuratorPeerIdsForCg path: set up a structural wallet-scoped CG, seed vmReconcileCuratorPeersByCg, make discovery.findAgents throw, then run recoverVmReconcileBatch and assert the cached curator is retained and used. Consider a companion successful-empty discovery case to prove the cache is cleared only on a real successful lookup.
| export interface PriorityAdmissionQueueHooks<Payload> { | ||
| canRun: (entry: PriorityAdmissionEntry<Payload>) => boolean; | ||
| onStart: (entry: PriorityAdmissionEntry<Payload>) => PriorityAdmissionRelease; | ||
| /** Undo capacity claimed by onStart when it throws before returning its release. */ |
There was a problem hiding this comment.
🟡 Issue: Make start admission rollback atomic instead of a caller-mirrored hook
What's wrong
This hook makes the generic queue harder to reason about because rollback correctness depends on each caller manually mirroring side effects from onStart. That is brittle API design, especially in a scheduler that is supposed to centralize admission invariants.
Example
The queue contract now says onStart may mutate capacity, may throw before returning a release, and the caller must remember to provide a separate rollback that undoes whatever part of onStart happened. That is a split transaction encoded by convention.
Suggested direction
Replace the separate rollback hook with an atomic start contract. The cleaner design is one owner for capacity state, not paired callbacks that must remain behaviorally inverse.
For Agents
Look at PriorityAdmissionQueueHooks and start() in packages/agent/src/sync/priority-admission-queue.ts, plus the global backpressure hook in packages/agent/src/sync/backpressure.ts. Preserve rollback-on-start-failure behavior, but refactor the API so capacity claim and release are owned atomically, e.g. by making the queue own inflight accounting or by requiring onStart to return a claim object before any fallible work.
There was a problem hiding this comment.
🟡 Issue: Do not make callers compensate for half-started admissions
What's wrong
onStartFailureRollback leaks the scheduler's failure semantics into every caller that mutates capacity during onStart. That makes the generic queue less self-contained and turns admission atomicity into an optional convention rather than an invariant enforced by the abstraction.
Example
A future queue consumer can mutate its own capacity in onStart, throw before returning a release, and forget to implement onStartFailureRollback; the generic queue will still look reusable, but its atomic start contract will silently depend on caller discipline.
Suggested direction
Make the queue's start path atomic by construction instead of exposing a compensating hook. The queue should either own the capacity counter or require start hooks to be side-effect-free until the release is available.
Confidence note
I am judging this as an abstraction-boundary issue rather than asserting a current behavior bug; it depends on whether this queue is intended to stay generic for more callers.
For Agents
In priority-admission-queue.ts, redesign start admission so the queue owns the atomic transition. Options include splitting capacity reservation from side-effectful start, making onStart return a transaction/release before external work can throw, or moving the inflight accounting into the queue payload/policy. Preserve the new tests for start failure rollback.
| }); | ||
|
|
||
| it('runs the oldest aged entry before newer elevated work', async () => { | ||
| it('bounds an aged lower-priority entry behind one raw-priority overtake', async () => { |
There was a problem hiding this comment.
🟡 Issue: Do not push sync-backpressure.test.ts past 1k lines
What's wrong
This PR takes a test file from under 1k lines to well over 1k lines. The added scenarios are valuable, but keeping them in the already broad backpressure suite makes the scheduler behavior harder to scan and maintain.
Example
The block beginning with bounds an aged lower-priority entry behind one raw-priority overtake adds many local now, running, enabled, starts, and PriorityAdmissionQueue setups that repeat the same scheduler harness in slightly different shapes.
Suggested direction
Move the new PriorityAdmissionQueue fairness/debt coverage into a dedicated test file with a reusable harness. Keep production-helper tests in the existing backpressure suite.
For Agents
Split the new scheduler fairness tests out of packages/agent/test/sync-backpressure.test.ts into a focused priority-admission-queue test file, and extract a small queue harness/factory for clock, running count, release capture, and starts. Preserve the scenarios; just decompose the file before it crosses the 1k-line boundary.
There was a problem hiding this comment.
🟡 Issue: Split the new state-machine tests instead of growing mega specs
What's wrong
The PR pushes sync-backpressure.test.ts from below 1k lines to well over 1k lines and appends a large amount of direct queue policy coverage to a global backpressure test file. The same sprawl pattern appears in core-fills-gap.test.ts, which becomes a 4k-line catch-all for exact VM recovery state-machine coverage. This makes the test suite harder to navigate and hides the ownership boundary of the new abstractions.
Example
The tests beginning at sync-backpressure.test.ts:475 instantiate PriorityAdmissionQueue directly and assert low-level scheduling policy. Those belong with the queue, not mixed into the global withGlobalSyncBackpressure coverage. The exact recovery tests similarly exercise rotation internals through (internals as any) in a 4002-line fixture file.
Suggested direction
Create focused test files around the units this PR actually changes: one for PriorityAdmissionQueue policy/debt behavior and one for exact VM recovery rotation. Keep only global backpressure integration cases in sync-backpressure.test.ts and only core-fill integration cases in core-fills-gap.test.ts.
For Agents
Split the new queue scheduler cases into priority-admission-queue.test.ts and move the exact VM rotation scenarios into a dedicated VM reconcile/exact recovery spec. Preserve all assertions, but extract shared fixture builders so the production changes are covered by focused tests rather than expanding unrelated mega-files.
There was a problem hiding this comment.
🟡 Issue: Decompose the backpressure test file before it grows past a healthy size
What's wrong
This PR turns the backpressure test file into a large mixed-level suite. The file-size jump makes it harder to find the production-helper tests, while the new direct queue tests deserve their own focused home. Even though this is test code, the structure matters because these scheduler rules are subtle and will be maintained as executable documentation.
Example
Many new cases create the same local now, running, enabled, starts, and new PriorityAdmissionQueue setup, then assert queue ordering directly. That is a separate scheduler spec living inside a higher-level backpressure test file.
Suggested direction
Split the low-level queue policy cases from the global backpressure integration cases. A dedicated queue test file plus a small fixture builder would cut repetition and keep both suites easier to scan.
Confidence note
The diff context strongly suggests this file crossed the 1k-line threshold in this PR, but confirm against the exact base if needed.
For Agents
Move direct PriorityAdmissionQueue scheduler-policy tests into packages/agent/test/priority-admission-queue.test.ts or a nested scheduler suite with shared fixture helpers. Leave sync-backpressure.test.ts focused on the production withGlobalSyncBackpressure wiring and metrics behavior.
| // cancelling midway could strand a partially applied VM transition. | ||
| // Clear exact-absence rotations first so a late in-flight response cannot | ||
| // restore process-local suppression while the dispatcher drains. | ||
| this.closeVmReconcileRotationState(); |
There was a problem hiding this comment.
🟡 Issue: Shutdown rotation cleanup is not verified through stop()
What's wrong
The change relies on DKGAgent.stop() closing process-local exact-recovery rotation state before the dispatcher drains. Current tests validate the helper behavior directly, but they do not prove that the agent shutdown path invokes it, leaving a regression gap for late exact responses mutating suppression state during shutdown.
Example
Start recoverVmReconcileBatch with syncExactKnowledgeAssetsFromPeerDetailed blocked, call agent.stop(), then release the blocked fetch. Expected: vmReconcileRotationState remains empty and the late response cannot enter backoff.
Suggested direction
Cover the actual stop() path, not only the helper method, because the new safety depends on this wiring being present.
Confidence note
I found tests that call the cleanup helper directly, but not a test that exercises this new production stop() wiring.
For Agents
Add a lifecycle regression test in a shutdown/core-fill test file that drives DKGAgent.stop() during an in-flight exact VM recovery, then asserts vmReconcileRotationClosed is set and no rotation record is recreated after the pending fetch settles.
There was a problem hiding this comment.
🟡 Issue: Shutdown rotation cleanup is not verified through agent.stop()
What's wrong
The diff’s shutdown safety depends on DKGAgent.stop() calling closeVmReconcileRotationState() before dispatcher drain. The added tests exercise the close helper directly, so they would not catch a regression where the production stop path omits or reorders this call, leaving a late exact-recovery response able to restore process-local suppression during shutdown.
Example
Failing-test sketch: start or simulate an in-flight exact VM recovery that has installed a rotation record, call agent.stop(), then release the pending exact request with disposition='clean-absent'. The test should assert that vmReconcileRotationClosed is true and vmReconcileRotationState remains empty. If the new stop() call were removed or moved too late, the current helper-level tests would still pass.
Suggested direction
Add a behavior-level stop() test that seeds or creates rotation state, triggers stop while recovery work is pending, then verifies the state is cleared/closed and late settlement cannot recreate suppression.
For Agents
Add a lifecycle regression around DKGAgent.stop, likely near the VM reconcile tests or stop lifecycle tests. Preserve the existing helper behavior, but prove the public stop path invokes it before late exact-recovery settlement can mutate rotation state.
| @@ -2367,6 +2597,1004 @@ describe('Phase D — reconcile gate + core-fill telemetry', () => { | |||
| expect(second.outcomes.get(0)).toEqual({ status: 'reconciled', blockNumber: 100 }); | |||
There was a problem hiding this comment.
🟡 Issue: Split the new exact-recovery rotation tests behind the same extracted boundary
What's wrong
The tests are reinforcing the production sprawl: they add a large private-internals harness to an already very large integration file. That makes the behavior harder to maintain because small state-machine changes will require editing a broad integration suite instead of a focused unit-level boundary.
Example
The new rotation cases around line 2597 stub vmReconcileRotationNow, mutate private maps, override internals, and call private helpers directly. That is a sign the production rotation logic wants its own module with a public test harness.
Suggested direction
Create a dedicated rotation-state test suite and a small fixture API. This will make the test coverage easier to scan and reduce the amount of private-agent setup each case needs.
For Agents
After extracting the rotation state machine, move these exact VM rotation cases into a focused test file for that module. Keep only integration-level recovery behavior in core-fills-gap.test.ts. Preserve the same scenarios, but drive them through typed fixtures instead of broad (internals as any) mutation.
There was a problem hiding this comment.
🟡 Issue: Decompose the newly expanded test suites before they become unmaintainable
What's wrong
The PR adds substantial scenario coverage by appending to already broad test files. That makes the behavior harder to scan, slows future review, and obscures the natural boundaries between VM rotation policy, host recovery orchestration, and generic scheduler fairness.
Example
A future change to VM exact-recovery rotation now has to navigate a 4k-line mixed fixture file, while scheduler fairness scenarios are packed into a broad global backpressure suite. These are cohesive enough to be their own suites.
Suggested direction
Move the new exact-recovery rotation tests and scheduler fairness tests into focused files with local fixtures. Keep core-fills-gap.test.ts for the higher-level core-fill behavior, and keep global backpressure tests separate from low-level queue fairness mechanics.
For Agents
Split the newly added VM rotation scenarios into a focused file such as vm-reconcile-rotation.test.ts with a shared harness, and split queue fairness/debt cases into a priority-admission-queue-fairness.test.ts suite. Preserve the same assertions; this is decomposition, not coverage reduction.
There was a problem hiding this comment.
🟡 Issue: Split the exact-recovery rotation tests out of the 4k-line core-fills spec
What's wrong
The PR adds a large new suite into an already oversized catch-all test file. Even if the cases are useful, this makes the test surface harder to navigate and encourages more copy-pasted internal-agent setup instead of a reusable, domain-specific harness.
Example
The new curator/rotation tests repeatedly build an agent, cast to AgentInternals, stub node.libp2p, resolveCuratorPeerIdsForCg, ensurePeerConnected, selectCatchupPeers, waitForSyncProtocol, syncExactKnowledgeAssetsFromPeerDetailed, and reconcileChainOrdinal. That setup is now duplicated across dozens of cases in a 4k-line spec.
Suggested direction
Create a focused vm-reconcile-rotation.test.ts or similar and centralize the repeated agent/peer stubbing behind a small fixture builder. Keep core-fills-gap.test.ts for the broader core-fill behavior it already owned.
For Agents
Split the exact VM recovery rotation coverage out of core-fills-gap.test.ts into a focused spec, and extract a local harness for peer rosters, curator resolution, exact fetch disposition, and reconcile outcomes. Preserve the same test scenarios; the change should only reduce fixture duplication and file size.
There was a problem hiding this comment.
🟡 Issue: Split the exact-recovery rotation tests out of the oversized core-fill suite
What's wrong
The test coverage for this change is structurally coupled to private implementation details and concentrated in an already very large file. That makes the test suite harder to scan and also reinforces the production-code smell: there is no clear boundary for the rotation state machine, so tests have to manipulate the whole agent object directly.
Example
The block starting with exact VM recovery cases sets up node.libp2p, preferredSyncPeers, resolveCuratorPeerIdsForCg, ensurePeerConnected, waitForSyncProtocol, syncExactKnowledgeAssetsFromPeerDetailed, and reconcileChainOrdinal inline in many tests. That setup is longer than the behavior each test is trying to express.
Suggested direction
Create a dedicated rotation test harness that exposes domain-level operations such as withPeers, nextDisposition, runRecovery, and rotationRecord. That would make the tests describe the state machine instead of reassembling the agent internals in every scenario.
For Agents
Move the exact VM recovery rotation cases into a focused test file, for example vm-reconcile-rotation.test.ts, and introduce a small harness for peer rosters, recovery targets, dispositions, and cooldown control. Preserve the same assertions, but stop growing the already broad core-fill suite.
| } | ||
|
|
||
| closeVmReconcileRotationState(this: DKGAgent): void { | ||
| this.vmReconcileRotationClosed = true; |
There was a problem hiding this comment.
🔴 Bug: Restarting an agent leaves exact VM recovery permanently closed
What's wrong
The new shutdown flag is one-way. stop() calls closeVmReconcileRotationState(), which sets vmReconcileRotationClosed to true, but start() never flips it back. Since the recovery path treats that flag as a hard suppressor, reusing an agent instance after stop/start disables exact-recovery fetches for the rest of the process.
Example
Call await agent.start(); await agent.stop(); await agent.start(); then run VM reconcile with pending recovery targets. prepareVmReconcileRotationTarget() sees vmReconcileRotationClosed === true and returns suppressed/noRecovery, so exact asset recovery never fetches again on that agent instance.
Suggested direction
Reopen the process-local rotation state during start/reinitialization, or make the closed flag tied to a stop generation so stale responses from the old run stay inert without blocking the next run.
For Agents
Check DKGAgent start/stop and VM reconcile rotation state. Preserve the late-response guard during stop, but reset or generation-scope vmReconcileRotationClosed on start; add a restart test proving recoverVmReconcileBatch can prepare and fetch after stop/start.
| return 'clean-absent'; | ||
| } | ||
|
|
||
| function stripLiteral(raw: string): string { |
There was a problem hiding this comment.
🟡 Issue: Reuse the canonical RDF literal helper instead of adding another regex parser
What's wrong
This adds avoidable parsing duplication in a boundary that already depends on exact RDF metadata shape. Duplicated literal handling tends to drift, and the local regex makes the new exact-fetch helper less trustworthy and harder to maintain.
Example
stripLiteral here is another bespoke literal parser in a proof-sensitive sync path. Even if it matches today’s expected metadata shape, future escaping or typed-literal handling has to be kept consistent by hand across multiple local implementations.
Suggested direction
Delete the local stripLiteral and use a shared parser such as parseRdfLiteralTerm/decodeRdfLiteralBody from the RDF utility package, or a single agent-level helper if that is the intended boundary.
For Agents
Replace the local regex helper with the canonical RDF literal parser/helper used elsewhere, or move one sync-safe helper into a shared module that both durable sync and exact durable fetch can import. Preserve descriptor filtering behavior for plain, typed, and raw IRI-style values.
| snapshotRef, | ||
| sinceBatchId, | ||
| assetUals, | ||
| maxAcceptedBytes: exactAccumulationLimits?.maxBytes, |
There was a problem hiding this comment.
🟡 Issue: Exact recovery limit wiring is not covered by an integration test
What's wrong
The PR adds the important production wiring that makes exact VM recovery bounded against legacy responders, but the tests only verify the lower-level guard when limits are supplied manually or stub the guard as already failed. That leaves the actual production path unverified: a regression that stops passing these limits would keep the new tests green while exact recovery loses the bound.
Example
Delete maxAcceptedBytes/maxAcceptedQuads from this fetchSyncPages call: the low-level accumulation tests would still pass because they call fetchSyncPages directly with explicit limits. Add a lifecycle-level test that invokes the exact recovery fetch path with assetUals and asserts the underlying fetch receives the calculated byte and quad ceilings.
Suggested direction
Add a test at the lifecycle wrapper boundary, either by spying/stubbing the requester call or by driving an exact recovery fetch through fetchSyncPage, so removing this wiring would fail.
For Agents
Look in packages/agent/src/dkg-agent-lifecycle.ts around fetchSyncPage and the existing sync fetch coalescing/lifecycle tests. Preserve ordinary full-sync calls with no limits, and add an exact-asset call that proves assetUals wires exactSyncPhaseAccumulationLimits into fetchSyncPages.
There was a problem hiding this comment.
🟡 Issue: Exact recovery accumulation limits lack a production-wiring test
What's wrong
The PR adds a security/resource-boundary for exact VM recovery, but the tests only prove the low-level page fetcher enforces limits when called with explicit max values. They do not prove exact recovery actually supplies those max values on the production path.
Example
A regression that drops maxAcceptedBytes/maxAcceptedQuads from the fetchSyncPages call at the lifecycle boundary would let VM exact recovery accept an oversized legacy full-CG response in production, while sync-exact-accumulation.test.ts would remain green because it bypasses this adapter and supplies the limits itself.
Suggested direction
Cover the lifecycle binding, not only the low-level limiter. A focused test can reuse the sync-fetch-coalescing agent helper or mock the requester boundary to capture/trigger the maxAcceptedBytes and maxAcceptedQuads behavior.
For Agents
Add a lifecycle-level exact-fetch test around DKGAgent.fetchSyncPages or syncExactKnowledgeAssetsFromPeerDetailed that passes assetUals and returns a response over the exact per-asset byte/quads limit, proving the production adapter rejects it. Also assert a full sync without assetUals is not capped by those exact limits.
| peerIds: string[]; | ||
| curatorIsLocal: boolean; | ||
| legacyTripleResolved: boolean; | ||
| lookupFailed?: boolean; |
There was a problem hiding this comment.
🟡 Issue: Make curator resolution state explicit instead of adding an optional mode flag
What's wrong
The optional lookupFailed flag turns a previously simple resolver contract into a loosely-shaped mode object. That is a maintainability smell because the absence of the field has meaning, false has meaning, and empty peerIds still has multiple meanings depending on other flags. The VM recovery cache policy is also now encoded inside a resolver whose documentation is still framed around the write path.
Example
The return shape can now mean at least four different states with overlapping fields: local curator, resolved remote peers, authoritative empty, and lookup unavailable. Because lookupFailed is optional, callers must infer meaning from combinations like peerIds: [], curatorIsLocal: false, legacyTripleResolved: false, and missing versus false lookupFailed.
Suggested direction
Use a typed discriminated union or a dedicated recovery resolver that returns an explicit unavailable state. That makes each caller handle the states it actually understands and keeps recovery-specific cache policy from leaking into a shared write-path resolver.
Confidence note
This is a boundary/design finding rather than a behavioral claim; it assumes resolveCuratorPeerIdsForCg remains a shared resolver for write/SWM paths, as its existing comment says.
For Agents
Replace the ad-hoc optional flag with a discriminated result, for example status: 'local' | 'resolved' | 'empty' | 'unavailable', or move the cached-roster/unavailable distinction into a VM-recovery-specific resolver. Preserve existing write-path semantics: empty authoritative resolution should still be unconfirmed, while unavailable recovery resolution may reuse the cached roster.
otReviewAgent
left a comment
otReviewAgent
left a comment
There was a problem hiding this comment.
Operational Notice: Review Agent could not complete this review.
Business logic reviewer failed: retry_exhausted
2 participants
Summary
found | clean-absent | incompletedisposition through the existing exact-request single-flight, rotate each missing VM slot/fingerprint across a bounded canonical peer roster, and enter exponential backoff only after every current candidate independently proves clean absence. Suppressed passes perform no discovery, dial, protocol probe, or decision-only store/chain work.Related
Diagrams
Global sync admission
Before:
sequenceDiagram participant Foreground participant Queue participant Background participant Worker Background->>Queue: Enqueue older reconnect/reconcile work Foreground->>Queue: Enqueue priority 2000 catch-up Worker-->>Queue: Release a global slot Queue->>Worker: Start aged background by effective priority Queue-->>Foreground: Remain queued or be displacedAfter:
sequenceDiagram participant Foreground participant Queue participant Background participant Worker Background->>Queue: Enqueue lower-priority reconnect/reconcile work Foreground->>Queue: Enqueue priority 2000 catch-up Worker-->>Queue: Release a global slot Queue->>Worker: Start highest raw priority when no debt is owed Queue-->>Foreground: Foreground starts at the bounded release Worker-->>Queue: Release the next slot Queue->>Worker: Serve one retained aged debt without idling capacityMissing VM asset recovery
Before:
sequenceDiagram participant Sweep participant Recovery participant Roster participant Peer participant ChainStore Sweep->>ChainStore: Reconcile missing ordinal Sweep->>Recovery: Recover missing asset Recovery->>Roster: Resolve and prepare peers each sweep Recovery->>Peer: Exact fetch Peer-->>Recovery: Empty or incomplete aggregate result Recovery->>ChainStore: Reconcile again Sweep->>Recovery: Retry without slot-specific absence proofAfter:
sequenceDiagram participant Sweep participant Recovery participant Roster participant Peer participant ChainStore Sweep->>ChainStore: Reconcile missing ordinal and fingerprint Sweep->>Recovery: Check bounded slot record alt Backoff proof is current Recovery-->>Sweep: Suppress before roster, dial, or admission work else Fresh candidate evidence required Recovery->>Roster: Resolve bounded canonical candidates Recovery->>Peer: Fresh exact fetch from an uncredited peer Peer-->>Recovery: Found, clean-absent, or incomplete Recovery->>ChainStore: Authenticate and reconcile current binding Recovery-->>Sweep: Back off only after a complete clean-absence rotation endGraph-scoped shutdown ownership
Before:
sequenceDiagram participant DurableSync participant Agent participant Chain participant Store participant Daemon DurableSync->>Agent: Store verified graph-scoped asset Agent->>Chain: Authenticate asset and binding Daemon->>Agent: Stop node Agent-->>Daemon: VM reconcile work retired Daemon->>Store: Close backing stores Chain-->>Agent: Authentication completes late Agent->>Store: Persist binding or materialize after teardownAfter:
sequenceDiagram participant DurableSync participant Agent participant Chain participant Store participant Daemon DurableSync->>Agent: Store verified graph-scoped asset Agent->>Agent: Register deferred physical store promise Agent->>Chain: Authenticate captured binding generation Daemon->>Agent: Stop node and close admission Agent->>Agent: Drain all admitted graph-scoped work Chain-->>Agent: Authentication completes Agent->>Store: Strictly persist and atomically materialize if current Store-->>Agent: Physical work settles Agent-->>Daemon: Retirement complete or typed quarantine Daemon->>Store: Close only after retirementFiles changed
packages/agent/src/sync/priority-admission-queue.ts,packages/agent/src/sync/backpressure.tspackages/agent/src/sync/requester/*,packages/agent/src/sync/exact-assets.tspackages/agent/src/dkg-agent-swm-host.ts,packages/agent/src/vm-reconcile-service.tspackages/agent/src/dkg-agent*.ts,packages/agent/src/context-graph-*.tspackages/agent/src/discovery.ts,packages/agent/src/p2p/*packages/publisher/src/metadata.tspackages/cli/src/daemon/lifecycle.ts,packages/cli/src/daemon/teardown.tspackages/agent/test/*,packages/publisher/test/*,packages/cli/test/*Test plan
pnpm --filter @origintrail-official/dkg-agent exec tsc --noEmitpnpm --filter @origintrail-official/dkg exec tsc --noEmitpnpm --filter @origintrail-official/dkg-publisher exec tsc --noEmit4bd402de89af4b73a200cf120796c3617474edbegit diff --checkand clean final candidate worktreetestnet-canary: require full VM convergence, clean-absence/backoff suppression, curator-arrival freshness, foreground fairness, and normalized API/Oxigraph resource ceilings before promotion beyond canary.