feat(agent): batch exact missing VM asset reconciliation - #1871
Conversation
| isTargetCurrent?: () => boolean; | ||
| }, | ||
| ) => { | ||
| permits.push(options.acquireActiveFetchPermit?.() ?? true); |
There was a problem hiding this comment.
🔴 Bug: Batch-fetch budget test stubs away the production behavior it claims to verify
What's wrong
The new host-level test verifies that a shared permit function is passed, but it does not verify that the changed active-fetch logic actually honors the permit or the peer-attempt cap. That gives false confidence for the user-facing resource-bound behavior this PR is adding.
Example
A regression that removed const batchAllowsFetch = options.acquireActiveFetchPermit?.() ?? true from reconcileChainOrdinal would still pass this test, because the stub consumes the permit itself and no syncContextGraphFromConnectedPeers calls are asserted.
Suggested direction
Exercise the real ordinal worker’s active-fetch path instead of calling the injected permit from a stub.
For Agents
In core-fills-gap.test.ts, add a focused test that invokes the real reconcileChainOrdinal on two or more no-SWM ordinals through runVmReconcileForCg, stubs only syncContextGraphFromConnectedPeers/finalizer inputs, and proves only one real active fetch occurs plus maxPeerAttempts: 1 prevents multi-peer retry expansion.
| watermark: before, | ||
| reconciled: 0, | ||
| pending: 0, | ||
| processed: 0, |
There was a problem hiding this comment.
🟡 Issue: No-work early return can hide a stale target
What's wrong
The new stale-target mechanism only runs inside the ordinal loop. When the captured watermark is already at or ahead of the captured head, the function returns without consulting isTargetCurrent, so a local-CG rebind that happens during the head read is reported as non-stale.
Example
If a pass captures on-chain CG 10 with watermark 5, then discovery rebinds the same local CG to on-chain CG 11 while getKCCount(10) is in flight, and the old chain returns head 5, this branch reports current/staleTarget: false. The new binding may have head > 0, but the caller will not enqueue the immediate follow-up pass for it.
Suggested direction
Treat a stale binding as stale even when the captured watermark already covers the captured head, so the host can schedule a pass for the repaired binding instead of returning a misleading current result.
Confidence note
This depends on a rebinding racing with the head-count read, but the new stale-target contract is explicitly meant to handle this class of race.
For Agents
In reconcileContextGraph, re-check deps.isTargetCurrent(localCgId, onChainCgId) after the head read and before the before >= head early return. Preserve the no-work fast path only when the captured target is still current, and add a test where the target changes before a head-equals-watermark return proves staleTarget: true and hasMore: false.
There was a problem hiding this comment.
🔴 Bug: Recovery results can be applied after the captured chain binding goes stale
What's wrong
The new stale-target guard is intended to stop an active pass when the local-CG to on-chain-CG binding changes, but the long-running batch recovery await is not bracketed by a post-await freshness check. That leaves a window where recovered outcomes from the old binding can advance or persist state for a reused local CG.
Example
A pass starts for local CG cg bound to on-chain id 5; ordinal 0 is pending and exact recovery waits on a peer. During that await, discovery replaces cg with on-chain id 9 and a new cursor. If recovery returns ordinal 0 as reconciled, staleTarget is still false, so the pass can record completion and persist a watermark for the current cg, potentially skipping ordinal 0 for id 9.
Suggested direction
Treat a stale target detected after recovery the same as stale target detected around ordinal work: set staleTarget, skip applying outcomes, reset the scan cursor, and let the caller queue a fresh pass.
Confidence note
Production recovery does receive an isTargetCurrent closure, but the reconciler itself does not re-check the captured binding after the awaited recovery call, so this still needs a code-level guard at the orchestration boundary.
For Agents
In packages/agent/src/chain-reconciler.ts, re-check deps.isTargetCurrent(localCgId, onChainCgId) immediately after recoverPendingOrdinals resolves and before merging/applying recovered outcomes. Preserve the no-watermark-move behavior when stale, and add a test where the target flips during recovery and recovered outcomes are ignored.
| } | ||
|
|
||
| if (options.isTargetCurrent && !options.isTargetCurrent()) { | ||
| return { status: 'skip' }; |
There was a problem hiding this comment.
🟡 Issue: Stale-target cancellation can leave a false active-fetch cooldown
What's wrong
The stale-target return bypasses the existing no-swm switch cleanup. Because the cooldown is keyed only by local CG id, a cooldown acquired by an abandoned binding throttles the fresh binding that the reconciler immediately queues next.
Example
A pass for old on-chain CG A reaches a missing SWM, calls shouldRunVmReconcileActiveFetch(localCgId), then the local CG is rebound to on-chain CG B before the first peer pull. The pass returns skip as stale, queues the next slice, but the new slice for B sees the per-CG cooldown and skips the one active fetch it needs for up to DKG_VM_RECONCILE_INTERVAL_MS.
Suggested direction
Make stale cancellation undo cooldown state that was created for the abandoned binding, or move the cooldown acquisition closer to the actual fetch after a final target check.
For Agents
In reconcileChainOrdinal, clear or avoid recording vmReconcileFetchCooldownAt when stale-target cancellation happens before a usable fetch for the current binding. A focused test should make isTargetCurrent flip after the cooldown check and assert the immediately queued next pass is not blocked by the old target's cooldown.
| * retries any gaps. This is deliberately not persisted: completed ordinals | ||
| * remain protected by `ahead`, and replay after restart is safe. | ||
| */ | ||
| scanOrdinal: number; |
There was a problem hiding this comment.
🟡 Issue: Keep fair-scan progress out of the cursor model
What's wrong
CursorState used to be a tight model for durable reconciliation evidence: watermark plus ahead. scanOrdinal is a scheduling/fairness concern, not completion evidence, and it makes every cursor consumer inherit a second notion of progress. That weakens the abstraction and explains the new reset branches in the reconciler.
Example
A reader now has to distinguish watermark as the next durable ordinal from scanOrdinal as the next fair-scan ordinal. That forces reconcileContextGraph to repair combinations like scanOrdinal past the head or behind the watermark even though those states are not part of cursor correctness.
Suggested direction
Extract slice selection/progress into a dedicated ReconcileScanState or pure selectReconcileSlice(...) helper rather than extending CursorState. The code-judo move is to make the cursor keep one meaning again and make bounded scanning a separate policy.
For Agents
Keep watermark/ahead behavior unchanged. Move bounded-slice progress into a small reconcile-scan state or slice-selector helper owned by the scheduler/reconciler layer, then have cursor helpers remain about durable completion only. Tests should still prove bounded passes resume at later ordinals and eventually reset to retry gaps.
| onChainCgId: bigint, | ||
| ordinal: number, | ||
| headBlock: number | undefined, | ||
| options: VmReconcileOrdinalOptions = {}, |
There was a problem hiding this comment.
🟡 Issue: Replace the optional callback bag with an explicit reconcile pass context
What's wrong
The options bag is carrying pass-level orchestration through a per-ordinal API. Because every field is optional, the real contract is implicit: some callers get bounded fetches and stale-target guards, others get legacy behavior. The result is branchy, cast-prone-feeling control flow in an already large method, with policy checks scattered through the body.
Example
The bounded batch path passes acquireActiveFetchPermit, maxPeerAttempts, and isTargetCurrent; the legacy path omits them. The callee now contains default-preserving branches for both modes plus repeated target-current probes around slow operations.
Suggested direction
Introduce a small VmReconcilePassContext/ActiveFetchPolicy with named methods and invariants, or move the active-fetch retry into a helper that owns the budget and staleness checks. That would let the ordinal method read as a direct ordinal reconciliation flow instead of a legacy path plus bounded-batch overlays.
For Agents
Look at createVmReconcileDeps and reconcileChainOrdinal. Preserve the active-fetch budget, peer rotation cap, and stale-target stop behavior, but package them as an explicit pass/session object or split the active-fetch retry into a helper that accepts a typed policy. Tests should still prove one fetch permit is shared across a batch and stale targets stop the pass.
There was a problem hiding this comment.
🟡 Issue: reconcileChainOrdinal now has hidden modes and unused recovery-budget hooks
What's wrong
The added options turn a previously direct operation into a flag-driven multi-mode function. Some of the new hooks are dead configuration surface today, and the active-fetch/deferred-fetch behavior is now controlled by booleans rather than by a clear abstraction. That makes future changes harder because readers must mentally execute option combinations to know what the method does.
Example
reconcileChainOrdinal(..., { deferActiveFetch: true }) now means “probe and return a recovery target,” while the same method without that flag means “run active fetch and possibly promote.” That hidden mode split makes the method name less truthful and leaves unused budget hooks in the API surface.
Suggested direction
Prefer two explicit operations, for example inspectChainOrdinalForRecovery and reconcileChainOrdinalWithFetch, or pass a small named policy object whose variants are exhaustive. Avoid landing knobs like acquireActiveFetchPermit, maxPeerAttempts, and VM_RECONCILE_FETCHES_PER_BATCH until the caller actually uses them.
For Agents
In packages/agent/src/dkg-agent-swm-host.ts, separate the probe path from the active-fetch path or introduce an explicit reconcile policy/strategy. Delete unused budget hooks and the unused env config unless they are actually connected to the batch recovery caller. Preserve existing normal reconcile behavior and the new deferred batch behavior with focused tests for each path.
There was a problem hiding this comment.
🟡 Issue: Replace mode flags in reconcileChainOrdinal with separate scan and recovery phases
What's wrong
The new options turn a single busy method into a multi-mode state machine. Optional booleans and callbacks are now controlling core behavior deep inside the ordinal reconciler, which makes invariants such as when active fetch is allowed or when stale targets abort much less obvious.
Example
The batch scan calls reconcileChainOrdinal(..., { isTargetCurrent, deferActiveFetch: true }), while the legacy path relies on default options. That means one function now implements both “inspect and return a recovery target” and “actively fetch and retry,” selected by optional flags.
Suggested direction
Make the scan path a direct helper that returns either a completion or an OrdinalRecoveryTarget, then let a separate recovery policy perform fetch/retry. The current optional flags can disappear instead of becoming permanent branch controls.
For Agents
Split the ordinal flow into clearer phases: resolve on-chain ordinal input, evaluate local VM/SWM state, and execute a recovery policy. Preserve existing return statuses and active-fetch behavior. Add coverage that the batch scan produces recovery targets without running active fetch, while the legacy path still fetches and retries.
| pending: number; | ||
| processed: number; | ||
| /** True when another bounded slice should be queued immediately. */ | ||
| hasMore: boolean; |
There was a problem hiding this comment.
🟡 Issue: Model reconcile continuation as a scheduler decision, not result booleans
What's wrong
The reconciler result now mixes telemetry (processed, pending) with control instructions for the host scheduler. hasMore and staleTarget are not the same kind of outcome, and making callers combine them creates another cross-layer special case rather than a clear continuation protocol.
Example
hasMore means continue the same bounded sweep behind other queued CGs; staleTarget means the captured local/on-chain binding changed and the target should be rediscovered. Those are different scheduling reasons, but the caller only sees two booleans and collapses them into the same live nudge.
Suggested direction
Replace the pair of booleans with a single typed continuation value, or have the dispatcher own bounded-slice continuation directly. At minimum, avoid making executeVmReconcileForCg interpret unrelated result flags and re-emit both as a live trigger.
Confidence note
This is a design concern from the diff shape; the current behavior may be intentional, but the continuation model should be made explicit if so.
For Agents
Keep the response telemetry fields if needed, but move continuation ownership to the dispatcher/reconcile orchestration boundary. Preserve queued next-slice behavior for large graphs and refresh behavior after stale targets. Add focused tests around the resulting continuation source/ordering.
| // Queue one trailing slice while this key is still active. The dispatcher | ||
| // places it behind already-waiting live CGs, so a large graph makes steady | ||
| // progress without monopolising the only VM worker. | ||
| if (result.hasMore || result.staleTarget) { |
There was a problem hiding this comment.
🟡 Issue: Follow-up slice scheduling is not verified at the host boundary
What's wrong
The PR’s fairness behavior depends on this host glue: bounded slices only make progress beyond the first batch if the host requeues another pass. Current tests validate the lower-level pieces separately, so a broken or missing bridge would not be caught.
Example
If lines 2751-2752 were deleted, the added reconciler slicing tests would still pass because they call the next slice directly; the dispatcher test would also still pass because it manually self-schedules. A graph with 25 missing ordinals and batch size 10 would stop after the first host pass until a later external trigger.
Suggested direction
Add an integration-style test that proves result.hasMore and/or result.staleTarget from the reconciler causes the host to enqueue the next pass.
Confidence note
This is based on the diff and repository test search: the pure reconciler and dispatcher are tested independently, but I did not find a host-level assertion that executeVmReconcileForCg queues the follow-up when the reconciler returns hasMore or staleTarget.
For Agents
Add a host-level test around runVmReconcileForCg/executeVmReconcileForCg with head larger than VM_RECONCILE_BATCH_SIZE, a stubbed ordinal reconciler, and waitForIdle; assert that the host schedules the trailing slice(s) and that queued work for another CG can run between them. Include a stale-binding variant if practical.
There was a problem hiding this comment.
🟡 Issue: The trailing-slice requeue path is only tested in isolated pieces
What's wrong
The PR verifies the reconciler can return hasMore and verifies the dispatcher can handle a self-scheduled job, but it does not verify this new branch actually wires those behaviors together in the agent. If this requeue call were removed or miswired, large context graphs would process only one bounded slice per trigger and rely on the later periodic sweep for progress.
Example
A test could set VM_RECONCILE_BATCH_SIZE to 2, make the chain head 5, stub reconcileChainOrdinal to record ordinals, call runVmReconcileForCg, wait for the dispatcher to become idle, and expect ordinals [0,1,2,3,4] across queued slices. Removing this branch should make that test stop after [0,1].
Suggested direction
Add an agent-level test that proves hasMore from the bounded reconciler causes executeVmReconcileForCg to schedule and complete the next slice.
Confidence note
I found unit coverage for reconcileContextGraph.hasMore and for dispatcher self-scheduling separately, but not for the agent glue that connects them here.
For Agents
Add an integration-style test around runVmReconcileForCg/ensureVmReconcileDispatcher that forces hasMore from the reconciler and asserts the trailing live slice is actually enqueued and drained. Consider a second stale-target case only if practical.
| ? Number.POSITIVE_INFINITY | ||
| : Math.max(1, Math.floor(configuredLimit)); | ||
| const scanStart = Math.max(state.watermark, state.scanOrdinal); | ||
| let candidates = outstandingBefore.filter((ordinal) => ordinal >= scanStart); |
There was a problem hiding this comment.
🔴 Bug: Moving heads can starve the earliest pending ordinal
What's wrong
The new fair-scan cursor skips outstanding ordinals below scanOrdinal until a pass observes no later candidates. On an actively growing context graph, that condition may never occur, so an earlier transient gap can stop the durable watermark from advancing indefinitely even after the gap becomes fixable.
Example
With maxOrdinalsPerPass = 10, suppose ordinal 0 is temporarily pending and scanOrdinal advances while ordinals 1..999 are processed. If new KCs keep being appended so each pass still has candidates >= scanOrdinal, the reset at lines 166-170 never runs. Ordinal 0 is not retried even after its data becomes available, so the contiguous watermark remains stuck at 0 while the agent keeps reconciling newer ordinals.
Suggested direction
Include the current watermark gap periodically or in every bounded pass when it is still outstanding, or reset/age the scan cursor independently of whether newer candidates exist, so continuous registrations cannot indefinitely postpone the gap that controls the watermark.
For Agents
In reconcileContextGraph, preserve bounded/fair scanning but ensure missing ordinals at the contiguous watermark are retried under a moving head. Add a test where an early ordinal is pending, later becomes reconcilable, and getKCCount grows between slices; the watermark should still advance without requiring the scanner to catch a quiet head.
| const outcomes = new Map<number, OrdinalOutcome>(); | ||
| let nextOrdinalIndex = 0; | ||
|
|
||
| const runOrdinalWorker = async (): Promise<void> => { |
There was a problem hiding this comment.
🟡 Issue: Use the canonical concurrency helper instead of embedding a worker pool in the reconciler
What's wrong
This diff mixes fair-scan/watermark logic with low-level worker-pool mechanics. That makes an already stateful reconciliation function harder to scan and duplicates an existing project abstraction for exactly this kind of ordered bounded concurrency.
Example
The slice can be expressed as ordered work over ordinals with the existing helper, returning { ordinal, outcome } entries and then applying successful outcomes in array order. That removes the local worker loop, mutable index, and result map from the domain reconciler.
Suggested direction
Move generic concurrency mechanics out of reconcileContextGraph. The reconciler should read as: choose slice, run bounded ordinal attempts, apply ordered outcomes. If mapWithConcurrency cannot quite model stale cancellation, extend/extract a shared bounded mapper with a stop predicate rather than keeping bespoke scheduler state in this domain function.
For Agents
In packages/agent/src/chain-reconciler.ts, replace the hand-rolled worker pool around runOrdinalWorker with the canonical mapWithConcurrency or extract a small helper if early-stop semantics need to be explicit. Preserve bounded concurrency, ordered cursor updates, stale-target stopping, and the existing processed/reconciled counters; keep the chain-reconciler tests proving deterministic ordered watermark updates.
There was a problem hiding this comment.
🟡 Issue: The reconciler slice grew into selection, concurrency, recovery, and cursor application all at once
What's wrong
This file is intended to be the thin, testable reconcile engine, but the diff concentrates several new responsibilities into the core function. The resulting control flow is correct-looking but dense: cursor fairness, concurrency, stale target cancellation, network recovery, and deterministic application are interleaved rather than modeled as separate steps.
Example
The new block from slice selection through recovery requires tracking outstandingBefore, scanOrdinal, candidates, hasUnvisitedCandidates, staleTarget, outcomes, processed, recoveryTargets, and final cursor reset rules in one function.
Suggested direction
Decompose the pass into plan, execute, and apply phases. The code-judo move here is to make each phase own one invariant so the main reconciler reads as the high-level algorithm again.
Confidence note
This is a structural maintainability concern rather than a behavior defect; the current tests may pass as-is, but the function has become noticeably harder to audit.
For Agents
Refactor packages/agent/src/chain-reconciler.ts without changing behavior. Suggested split: a pure selectOrdinalSlice(state, head, max) helper, a runOrdinalBatch helper for concurrency/staleness/recovery, and an applyOrdinalOutcomes helper for deterministic cursor updates. Keep tests around bounded slices, stale targets, and recovery reuse.
There was a problem hiding this comment.
🟡 Issue: Split bounded-slice orchestration from cursor mutation
What's wrong
The reconciler used to be a straightforward sweep reducer; this PR turns it into a mixed scheduler, worker pool, recovery coordinator, and cursor mutator. The behavior may be right, but the structure makes the watermark and scan-cursor invariants much harder to audit.
Example
To reason about when scanOrdinal advances or resets, a reader has to trace candidate filtering, worker early exits, recovery map merging, ordered outcome application, and the final hasMore calculation in one long function.
Suggested direction
Refactor toward small units such as selectOrdinalSlice, runOrdinalWorkers, recoverPendingOutcomes, and applyOrdinalOutcomes. That would keep concurrency/recovery orchestration separate from the cursor invariant.
For Agents
Work in chain-reconciler.ts. Extract pure helpers for selecting the slice, running workers, recovering pending outcomes, and applying ordered cursor mutations. Preserve deterministic ordinal-ordered cursor advancement and the existing bounded-slice/stale-target/recovery tests.
| } | ||
|
|
||
| createVmReconcileDeps(this: DKGAgent, localCgId: string): ChainReconcilerDeps { | ||
| const capturedSub = this.subscribedContextGraphs.get(localCgId); |
There was a problem hiding this comment.
🟡 Issue: Build reconcile deps from the resolved target instead of recapturing mutable maps
What's wrong
The new stale-target guard is structurally coupled to mutable maps but is created separately from the target object the pass actually uses. Readers now have to prove that target.cursor and capturedCursor, and target.sub and capturedSub, are the same objects before they can reason about the pass boundary.
Example
A simpler shape is createVmReconcileDeps(localCgId, target) where the stale guard compares the current subscription/cursor to the exact target already being reconciled. That keeps target ownership in one model instead of re-discovering it in the dependency factory.
Suggested direction
Make the resolved VmReconcileTarget the single pass context. That can own the captured subscription, cursor, on-chain id, target guard, and per-batch fetch budget, so the dependency factory does not silently depend on re-reading mutable global maps in the same state as resolveVmReconcileTarget.
For Agents
In packages/agent/src/dkg-agent-swm-host.ts, thread VmReconcileTarget into createVmReconcileDeps and derive the target-current guard from that object. Preserve the behavior that a repaired/replaced binding stops the active pass and queues a fresh slice.
| let outcome = await fh.handleChainReconciledKC(reconcileInput, ctx); | ||
| if (outcome === 'no-swm') { | ||
| swmState = await this.collectVmReconcileSwmCandidateState(localCgId); | ||
| if (outcome === 'no-swm' || outcome === 'verified-vm-metadata-pending') { |
There was a problem hiding this comment.
🟡 Issue: Model active-fetch recovery outcomes instead of repeating string branches
What's wrong
The PR adds a second recoverable finalization state by bolting that string into multiple conditionals inside a long method. The behavior may be right, but the structure makes the recovery policy implicit and easy to update inconsistently the next time another outcome needs similar treatment.
Example
Instead of scattering literal outcome checks, model this once with helpers such as shouldActiveFetchForReconcileOutcome(outcome) and needsNegativeCacheSnapshot(outcome), or return a structured finalization outcome with those properties.
Suggested direction
Introduce a small outcome-policy helper or typed outcome metadata so the active-fetch loop is driven by named intent rather than duplicated string unions. This would keep the large ordinal reconciler from accumulating more special-case branching as finalization outcomes grow.
For Agents
In reconcileChainOrdinal, centralize the finalization-outcome policy before the active-fetch block. Preserve the current behavior: both no-swm and verified-vm-metadata-pending may trigger fetch/retry; only no-swm records SWM candidate state for negative caching.
| }); | ||
| let maxAttempts = 1; | ||
| for (let attempt = 0; attempt < maxAttempts && outcome === 'no-swm'; attempt += 1) { | ||
| const fixedMaxAttempts = options.maxPeerAttempts === undefined |
There was a problem hiding this comment.
🟡 Issue: The per-batch peer-attempt cap is not verified against the real fetch loop
What's wrong
The change adds a resource-safety cap for peer rotations, but the added coverage stubs away the code that is supposed to honor the cap. That leaves the main operational risk unverified: a large batch could still fan out across every connected peer inside the one permitted active fetch.
Example
A regression that ignored options.maxPeerAttempts and kept expanding maxAttempts to totalPeers would still pass shares one active payload-fetch permit, because that test never runs the real fetch loop. With 33 connected peers and { maxPeerAttempts: 1 }, the expected behavior is one syncContextGraphFromConnectedPeers call, not 33.
Suggested direction
Cover the real maxPeerAttempts behavior rather than only checking that the option is forwarded into a stubbed ordinal reconciler.
For Agents
Add a focused reconcileChainOrdinal test in packages/agent/test/core-fills-gap.test.ts that calls the real method with maxPeerAttempts: 1, stubs multiple connected peers and no successful materialization, and asserts the active fetch loop stops after one peer. Keep the existing no-options tests proving legacy full peer rotation still works.
There was a problem hiding this comment.
🟡 Issue: Deferred metadata-pending recovery is not covered
What's wrong
The new production path defers active fetches during the parallel scan, so metadata-pending KAs rely on the recovery target flow. Current tests cover metadata-pending only in the old inline fetch mode, leaving the changed behavior unverified.
Example
A regression that only returns recovery for no-swm in deferred mode would still pass the current metadata-pending test, because that test exercises the inline fetch path. A focused test could make handleChainReconciledKC return verified-vm-metadata-pending, call the deferred reconcile path, and assert the recovery target is batched and revalidated after exact sync.
Suggested direction
Add a regression test for the batched exact-recovery path using a verified-vm-metadata-pending outcome, not just no-swm or the legacy inline fetch path.
For Agents
Add coverage in packages/agent/test/core-fills-gap.test.ts or chain-reconciler.test.ts for verified-vm-metadata-pending under deferActiveFetch: true. Preserve the existing inline metadata-pending behavior, but prove the production VM pass sends that reason through recoverVmReconcileBatch/exact recovery and can advance after revalidation.
branarakic
changed the title
| ).map((peer) => peer.toString()); | ||
| const peerIds = [...new Set([ | ||
| ...curatorPeerIds.filter((peerId) => connectedPeerIds.has(peerId)), | ||
| ...orderedConnectedPeerIds, |
There was a problem hiding this comment.
🔴 Bug: Exact VM recovery can contact connected peers that failed network admission
What's wrong
The exact recovery path bypasses the established recovery admission filter for non-curator connected peers. That can send signed exact-asset sync requests, including the private CG id and requested UALs, to peers the node has rejected or not admitted.
Example
A private CG has one missing KA, the curator is offline, and a connected peer has already been rejected by network admission but still advertises PROTOCOL_SYNC. The new exact recovery path can include that peer from orderedConnectedPeerIds and call syncExactKnowledgeAssetsFromPeer, sending the CG id and exact KA UAL to a peer the rest of sync recovery would skip.
Suggested direction
Apply the same requester-side peer admission gate to orderedConnectedPeerIds before exact recovery fetches, or restrict fallback peers to already accepted peers.
Confidence note
This assumes messenger.sendToPeer does not independently enforce networkAdmissionCoordinator; the surrounding sync paths explicitly call ensurePeerAdmittedForRecovery, which suggests admission is expected at the caller.
For Agents
In recoverVmReconcileBatch, filter fallback connected peers through the same admission check used by syncContextGraphFromConnectedPeers before adding them to peerIds; preserve curator-first ordering and prove rejected connected peers are not contacted while accepted peers still are.
| */ | ||
| totalTimeoutMs?: number; | ||
| /** Internal VM-recovery filter; only these locally-missing KAs are stored. */ | ||
| exactAssetUals?: string[]; |
There was a problem hiding this comment.
🟡 Issue: Exact-asset sync is threaded through the generic sync stack as a nullable side channel
What's wrong
This change makes exact-KA recovery a cross-cutting mode hidden behind assetUals?: string[]. The behavior may be correct, but the structure pushes one feature flag through many unrelated layers and forces each layer to remember how to key, serialize, filter, and page it. That is brittle architecture: the invariant is global, but the implementation is scattered.
Example
A future sync selection mode would need the same edits across lifecycle, requester, auth envelope, checkpointing, responder, and graph-plan code. That is a sign the selection concept is missing from the model rather than belonging as another nullable array parameter.
Suggested direction
Make the selection first-class, e.g. SyncSelection = { kind: 'full' } | { kind: 'since'; batchId } | { kind: 'exactAssets'; uals }, with canonical helpers for identity, envelope fields, checkpoint keys, and responder plans. That would remove most of the repeated optional-parameter plumbing and make the next mode much cheaper to reason about.
For Agents
Look at packages/agent/src/dkg-agent-lifecycle.ts, sync/requester/page-fetch.ts, sync/requester/durable-sync.ts, sync/responder/sync-handler.ts, and sync/responder/graph-plan.ts. Preserve exact-KA filtering behavior, checkpoint isolation, and rolling-upgrade filtering, but introduce a single typed sync-selection object or a dedicated exact-asset sync path that owns request serialization, cache/checkpoint keys, and responder planning. Add/keep tests proving full, since, recovery, and exact selections do not share cursors or responder sessions.
There was a problem hiding this comment.
🟡 Issue: Exact-asset request identity is missing coalescing coverage
What's wrong
The PR makes exactAssetUals part of in-flight request identity so offsets and responses cannot cross between different VM recovery batches. Existing coalescing tests enumerate other identity fields, but not the newly added asset filter, so a regression that drops this key component would likely pass.
Example
Start two concurrent page fetches for the same peer/CG/phase with assetUals [UAL 7] and [UAL 8]. The expected behavior is two physical sends. Without assetUals in the coalescing key, the second caller can receive the first batch's page result.
Suggested direction
Add regression tests that different exact-asset batches do not coalesce or single-flight together.
For Agents
Extend packages/agent/test/sync-fetch-coalescing.test.ts with an assetUals case in the direct fetch identity table, and add a direct durable sync single-flight case using different exactAssetUals options to prove they do not share one run.
| * enters the wire request. After every peer response we re-run local chain | ||
| * verification and remove completed KAs before considering another peer. | ||
| */ | ||
| async recoverVmReconcileBatch(this: DKGAgent, |
There was a problem hiding this comment.
🟡 Issue: Batch recovery is embedded as a large orchestration method in the host class
What's wrong
This is feature-specific orchestration landing in one of the largest files in the agent package. It increases local sprawl and couples VM recovery details directly to the host class instead of giving the new recovery concept a clear ownership boundary.
Example
The function starts by resolving curator peers, then mutates connection state, then builds a peer list, then emits per-target telemetry, then runs durable sync, then re-runs ordinal reconciliation. Each of those steps has a separate reason to change, but they now live in one method on the host class.
Suggested direction
Move this into a dedicated recovery component with small helpers for selectRecoveryPeers, fetchExactAssetsFromPeer, and recheckRecoveredTargets. That would shrink the host class and make the recovery algorithm readable without scanning unrelated host-mode state.
For Agents
Extract the new batch-recovery workflow from packages/agent/src/dkg-agent-swm-host.ts into a focused module such as vm-reconcile-recovery.ts. Keep behavior the same, but inject peer/connect/sync/reconcile dependencies, make peer planning a small helper, and leave SwmHostModeMethods as wiring. Tests should still cover curator targeting and batched recovery.
There was a problem hiding this comment.
🟡 Issue: Move exact VM recovery out of the already oversized SWM host class
What's wrong
This grows an already sprawling agent host file with a complete recovery workflow. The implementation may work, but structurally it pushes more feature-specific orchestration into a class that is already carrying too many responsibilities, making future VM reconcile changes harder to isolate and review.
Example
The new method owns at least four concepts in one place: resolving candidate peers, connecting/selecting peers, issuing exact durable sync, and reconciling remaining ordinals. A reader changing peer selection now has to reason inside the SWM host class instead of a focused VM recovery module.
Suggested direction
Introduce a small recovery orchestrator with explicit dependencies for peer resolution, connection, exact sync, and ordinal recheck. That would delete a large block from the host class and make the new behavior easier to reason about.
For Agents
Extract recoverVmReconcileBatch into a focused VM exact-recovery helper/module near the reconcile/sync code. Preserve peer ordering, exact UAL batching, and post-fetch reconcileChainOrdinal behavior. Keep dkg-agent-swm-host.ts as wiring only, with tests proving the same peer order and recovery outcomes.
There was a problem hiding this comment.
🟡 Issue: Move batch recovery out of the monolithic host class
What's wrong
This adds another feature-sized orchestration block to a 4.8k-line class and couples VM reconciliation policy directly to host networking internals. Even if behavior is correct, the implementation increases the amount of agent state a maintainer must understand before safely changing recovery.
Example
Changing the peer cap, cooldown rule, or revalidation behavior now requires editing the same loop that performs network fetches and emits replication telemetry. The new tests also stub many agent internals through any, which is a symptom that this behavior lacks a clean boundary.
Suggested direction
Keep the host class as a thin adapter and move the batch-recovery orchestration into a focused module such as vm-reconcile-recovery.ts. A pure planner plus an executor would make this policy testable without stubbing the full agent object.
For Agents
Start at recoverVmReconcileBatch and createVmReconcileDeps. Extract a VM reconcile recovery coordinator or planner module with injected dependencies for peer ordering/connection, exact sync, cooldown, logging, and ordinal revalidation. Preserve peer ordering, cooldown clearing, and revalidation semantics; move tests toward the extracted coordinator with fake deps.
There was a problem hiding this comment.
🟡 Issue: Exact-KA recovery should not live as another large orchestration block in the SWM host class
What's wrong
This deepens the existing “agent class owns everything” problem. The code is not just implementation detail; it is a new recovery subsystem, but it is embedded in a massive class and coupled directly to peer discovery, admission, sync, telemetry, and cursor revalidation. Future changes to any one of those policies will have to reason through the whole method.
Example
Within this one method, peer selection happens around lines 3687-3723, admission at 3736, protocol slicing at 3744-3746, network fetch at 3761-3765, revalidation at 3777-3792, and cooldown cleanup at 3800-3803.
Suggested direction
Extract a small recovery coordinator, plus pure helpers for peer ordering and batch slicing. That would delete a large chunk of mixed-responsibility code from the 4.8k-line host class and make the recovery policy testable without constructing agent internals.
For Agents
Look at packages/agent/src/dkg-agent-swm-host.ts. Preserve the exact recovery behavior, but move the batch-recovery orchestration into a focused VM reconcile recovery module or sync requester helper with injected dependencies. Keep the agent method as a thin adapter. Existing batch recovery tests should continue to prove peer ordering, cap slicing, admission gating, cooldown behavior, and revalidation.
|
|
||
| let effectiveMetaResult = metaResult; | ||
| let dataResult = rawDataResult; | ||
| if (exactAssetUals !== undefined) { |
There was a problem hiding this comment.
🟡 Issue: Exact-asset requester filtering is only unit-tested, not verified in the durable sync flow
What's wrong
The new VM recovery path depends on runDurableSync both requesting exact assets from upgraded peers and filtering full-CG responses from old peers before verification. The current tests validate the helper in isolation, but they would not catch a broken call-site or a future change that filters after verification instead of before it.
Example
A regression that stops passing exactAssetUals to the META/DATA fetch calls, or verifies rawDataResult/metaResult instead of the filtered effective results, would still pass the added unit tests. A focused test could runDurableSync with exactAssetUalsFor returning [wantedUal], have fetchSyncPages return both wanted and unwanted KA quads, then assert the fetch calls receive the asset list and processDurableBatchInWorker sees only the wanted metadata/data.
Suggested direction
Cover the full requester path that threads exactAssetUalsFor into fetchSyncPages and applies the rolling-upgrade filter before verification/storage.
For Agents
Add a requester-level regression test around runDurableSync in packages/agent/test/durable-sync-since-threading.test.ts or a nearby durable sync test. Exercise exactAssetUalsFor, capture the positional fetchSyncPages args for both meta and data, return a mixed old-responder full-CG payload, and assert verification/storage inputs are narrowed to the requested KA.
There was a problem hiding this comment.
🟡 Issue: Model durable sync selection explicitly instead of threading independent optional filters
What's wrong
Exact-asset sync is currently bolted onto durable sync as another optional side channel. The result is scattered branching and string-key composition across requester, responder, checkpoint, and graph-plan layers, so the real invariant is not represented in one place.
Example
A future caller can accidentally combine sinceBatchIdFor and exactAssetUalsFor; the data selection, session key, and verification mode are then decided by different optional fields in different files. That is a boundary smell even if today’s VM caller only uses exact assets without since.
Suggested direction
Centralize selection normalization and key generation behind a typed model. That would collapse the repeated exact/full/since conditionals and make invalid mixed modes impossible at the call boundary.
For Agents
Introduce a discriminated sync selection model, for example { kind: 'full' } | { kind: 'since', sinceBatchId } | { kind: 'exactAssets', assetUals }, and pass that through request building, checkpoint/session key generation, requester filtering, and responder planning. Preserve wire compatibility by encoding the same fields at the boundary.
There was a problem hiding this comment.
🟡 Issue: Exact-asset durable sync is not tested through the requester path
What's wrong
The new VM repair behavior depends on runDurableSync threading exactAssetUalsFor into both page fetches and filtering old-responder full-CG payloads before verification/storage. The added tests exercise the helper filter directly, so they would not fail if this integration were removed or miswired.
Example
A regression that still fetches pages but forgets to call filterExactAssetDurablePayload before processDurableBatchInWorker would keep the direct filter unit test green while an old responder's full-CG response is verified/stored as part of exact VM recovery. A focused test could return metadata/data for UAL 7 and UAL 8 from fetchSyncPages with exactAssetUalsFor => [UAL 7], then assert the worker and store only see UAL 7's quads.
Suggested direction
Cover the integrated exact-asset requester flow, not only the helper filter and responder read functions.
For Agents
Add a runDurableSync integration test, likely in packages/agent/test/durable-sync-since-threading.test.ts, wiring exactAssetUalsFor and a fetchSyncPages stub that records the assetUals argument and returns extra old-responder quads. Prove meta/data fetches receive the exact filter and that verification/storage receive only the requested KA.
There was a problem hiding this comment.
🟡 Issue: Requester exact-asset filtering is only tested as a standalone helper
What's wrong
The rolling-upgrade guarantee says old responders may return the whole CG, but the requester must filter to the requested assets before verification or storage. The new test validates the helper in isolation, not that runDurableSync actually invokes it and threads assetUals through both fetch phases.
Example
A failing-test sketch: configure runDurableSync with exactAssetUalsFor: () => [wanted], have fetchSyncPages return meta/data for wanted plus existing, then assert both fetch calls receive [wanted] and processDurableBatchInWorker sees only the filtered wanted quads.
Suggested direction
Add an integration-style unit test for the runDurableSync exact-asset branch.
For Agents
Extend packages/agent/test/durable-sync-since-threading.test.ts or add a focused requester test that drives runDurableSync with exactAssetUalsFor. Preserve normal full/since sync behavior, and prove exact sync forwards the filter to both phases and filters old-responder full-CG payloads before verification/storage.
| let syncSessionId: string | undefined; | ||
| let assetUals: string[] | undefined; | ||
| let tail = parts.length; | ||
| if (tail >= 2 && parts[tail - 2] === 'assets') { |
There was a problem hiding this comment.
🟡 Issue: Exact-asset wire parsing lacks regression coverage
What's wrong
The responder only sees the exact-asset filter if the newly-added parser code preserves it. Existing added tests cover encoding and normalization separately, but not the wire decode path that connects those pieces. If this parser regressed, exact recovery against upgraded peers could fall back to full responses while the current tests still pass.
Example
Build or hand-write mfacts|0|100|session|s1|since|42|assets|${encodeURIComponent(JSON.stringify([ual]))} and parse it through parseSyncRequest/parsePipeDelimitedSyncRequest; the test should assert assetUals is [ual]. A malformed present filter such as |assets|%7B%7D should parse to [] so the responder serves nothing rather than treating it as undefined/full sync.
Suggested direction
Test the parser/handler boundary, not just the envelope builder and normalize helper, so a dropped or misordered assets token cannot silently expand an exact recovery request into a full-CG response.
For Agents
Add wire-level parser coverage in the CG resolve or sync envelope tests. Exercise authenticated JSON and public pipe-delimited requests, including invalid-present asset filters, and verify the parsed SyncRequestEnvelope preserves valid filters and fails closed to an empty list for malformed filters.
There was a problem hiding this comment.
🟡 Issue: Exact-asset wire parsing is not verified end to end
What's wrong
The new exact-KA behavior depends on parseSyncRequest preserving and fail-closing assetUals before registerSyncHandler can narrow responses. The added tests exercise encoding, normalization, and page readers separately, but not the production wire parsing path that connects them.
Example
A regression that drops the |assets| parser branch, parses it in the wrong token order, or stops normalizing parsed.assetUals would still leave the new encoder and page-reader tests green while upgraded responders receive assetUals === undefined and serve the full Context Graph.
Suggested direction
Cover the actual parse-to-responder path, not only the encoder and direct graph-plan helpers.
For Agents
Add parser or handler-level tests around ContextGraphResolveMethods.parseSyncRequest and the sync handler: JSON envelopes with valid and malformed assetUals, and pipe-delimited requests containing |session|...|since|...|assets|.... Prove valid filters reach the durable meta/data readers and present-but-invalid filters become an empty response.
| } | ||
|
|
||
| try { | ||
| const result = await this.syncExactKnowledgeAssetsFromPeer( |
There was a problem hiding this comment.
🟡 Issue: Configured reconcile batches above ten make exact recovery fail closed
What's wrong
The reconcile batch size can be increased above the exact-sync asset limit, but recovery sends the whole set in one exact request. Once more than ten unique missing UALs are collected, the request validator rejects the batch, so the new recovery path silently degrades to repeated pending retries under that supported configuration.
Example
With DKG_VM_RECONCILE_BATCH_SIZE=20 and 11 missing ordinals in one slice, requestedUals has 11 entries. syncExactKnowledgeAssetsFromPeer calls requireExactAssetUals, which throws Exact VM sync requires 1-10 valid KA UALs; the catch logs the failure and the batch never fetches any of those KAs.
Suggested direction
Keep the public batch-size knob consistent with the exact-sync protocol cap, either by bounding the configured batch size or splitting recovery into multiple exact requests of at most 10 UALs.
For Agents
Cap VM_RECONCILE_BATCH_SIZE to MAX_EXACT_SYNC_ASSETS or chunk requestedUals in recoverVmReconcileBatch before calling syncExactKnowledgeAssetsFromPeer. Add a test with a configured reconcile batch larger than 10 proving all missing assets are recovered in valid exact-sync chunks.
|
Review: well-engineered — approve with two requested changes (a config clamp and a damping question), verified against the branch head rather than the PR text. What I verified holds
Requested changes
Smaller notes
Tests are genuinely strong — slice/concurrency/stale-target coverage on the pure reconciler, fail-closed filter tests, a real-store responder test proving only the requested descriptor + data graph is served, and both new suites are correctly added to the explicit vitest include list. |
…ive recovery Two review fixes for the batched exact VM reconciliation path: - recoverVmReconcileBatch now requests at most MAX_EXACT_SYNC_ASSETS UALs per peer. A scan batch configured above the protocol cap (DKG_VM_RECONCILE_BATCH_SIZE > 10) previously produced a filter that requireExactAssetUals rejects, so every exact fetch threw and recovery was silently dead. Over-cap targets stay in `remaining` for a later peer or pass; fetch telemetry and revalidation (which costs chain reads per ordinal) are restricted to the requested slice. - The batch fetch is now gated on the per-CG active-fetch cooldown. The batched path deliberately skips the per-UAL negative cache (consulting it primes connections to every discovered agent), which left no damper at all: a CG with permanently unavailable KAs ran a 3-peer, priority-1000 exact sync on every reconcile pass. An unproductive batch now costs one bounded fetch per sweep interval. Mirroring the inline path, progress or an unreachable network clears the cooldown, so a draining backlog still proceeds slice after slice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed 1. Wire cap ( 2. Damping. The batch fetch is now gated on the existing per-CG
Three new tests in Correction to my review: the note that Validation: agent The smaller review notes (the |
| }; | ||
|
|
||
| const workerCount = Math.min(ordinalConcurrency, ordinals.length); | ||
| await Promise.all(Array.from({ length: workerCount }, () => runOrdinalWorker())); |
There was a problem hiding this comment.
🔴 Bug: Concurrent ordinal failures can leave reconcile work running after the pass has failed
What's wrong
The new concurrent worker pool changes the lifecycle contract of a reconcile pass. A single rejected ordinal now makes Promise.all reject immediately, while other in-flight ordinal workers continue with side effects outside the caller's awaited lifecycle. That can break per-CG serialization, make retries overlap with unfinished finalization/fetch work, and produce misleading failure/idle state.
Example
With maxOrdinalConcurrency = 2, let ordinal 0 throw immediately and ordinal 1 block inside reconcileOrdinal before later promoting a KA. reconcileContextGraph() rejects on ordinal 0 and releases the dispatcher, but ordinal 1 can still promote after the caller has started a retry/manual pass for the same CG.
Suggested direction
Use an all-settled style barrier or catch errors inside each worker, set a shared stop flag, and only rethrow after every started worker has completed. Preserve the existing behavior that a failed pass does not advance the cursor unless the code intentionally records partial successes after all workers are settled.
For Agents
In packages/agent/src/chain-reconciler.ts, keep the bounded parallelism but make worker failures coordinated. Capture the first error, stop assigning new ordinals, wait for all already-started workers to settle, then rethrow before recovery/cursor updates. Add a test where one concurrent ordinal rejects while another is still pending, proving the function does not resolve/reject until the second worker has stopped or settled and no overlapping pass can begin.
| @@ -161,14 +162,16 @@ interface FetchSyncPagesParams { | |||
| * members-only `isMemberRecoveryAuthorized`). Default false ⇒ normal sync. | |||
There was a problem hiding this comment.
🟡 Issue: Model exact-asset selection instead of threading another optional positional argument
What's wrong
The PR adds exact-asset sync by pushing a loose optional array through already-wide APIs and then re-deriving its identity at each layer. That makes the sync path harder to extend and audit because every future mode now has to remember every checkpoint, cache, responder-session, coalescing, and filtering site.
Example
A caller requesting exact assets must rely on the same loose assetUals array changing the wire request, requester checkpoint, responder session id, row cache key, and old-responder filtering path. That is one concept spread across several unrelated branches.
Suggested direction
Introduce a small typed selection object, for example SyncSelection = { kind: 'full' } | { kind: 'since'; sinceBatchId: string } | { kind: 'exact'; assetUals: string[]; key: string }, and derive all checkpoint/session/cache/single-flight identities from it in one place.
For Agents
Look at sync/exact-assets.ts, sync/requester/page-fetch.ts, sync/requester/durable-sync.ts, sync/responder/sync-handler.ts, and sync/responder/graph-plan.ts. Preserve fail-closed normalization and wire compatibility, but introduce a normalized sync selection model with a canonical key and tests proving full/since/exact selections stay isolated.
There was a problem hiding this comment.
🟡 Issue: Exact-asset sync is being threaded through an already brittle positional API
What's wrong
The PR adds another optional mode by extending long positional function signatures instead of cleaning the boundary. This makes the sync path harder to scan and easier to misuse because argument meaning depends on position rather than names, especially where multiple optional booleans and filters sit next to each other.
Example
runLegacyDurableSyncForContextGraph calls this.fetchSyncPages(..., snapshotRef, sinceBatchId, undefined, undefined, onVerifiedFullSnapshot !== undefined, exactAssetUals), which is a sign that the API boundary has outgrown positional parameters.
Suggested direction
Introduce a named parameter object for page fetch and request building before adding more sync modes. This would remove the placeholder arguments, make call sites self-documenting, and give future sync selectors a canonical home.
For Agents
Look at packages/agent/src/sync/requester/page-fetch.ts, packages/agent/src/dkg-agent-lifecycle.ts, and packages/agent/src/dkg-agent-cg-resolve.ts. Preserve all request fields and checkpoint/coalescing behavior, but replace the long optional positional chains with a typed options object, e.g. SyncPageRequest / BuildSyncRequestInput. Tests that exercise since, recovery, forceFreshSession, and asset filters should prove the object form threads the same values.
lupuszr
left a comment
lupuszr
left a comment
There was a problem hiding this comment.
Requesting changes for three correctness and network-boundary blockers confirmed on head ea5a5fb29e13b3848008865cafb5fa11ecfb5710:
-
Recovery outcomes are merged after the long
recoverPendingOrdinalsawait without re-checking the captured local-CG to on-chain-CG binding. A rebind can therefore let results from the old target advance or persist the new target cursor. Existing thread: #1871 (comment) -
Exact VM recovery appends every currently connected peer after the curator candidates without filtering or re-admitting them through
NetworkAdmissionCoordinator. This can send sync requests to unknown or rejected-network peers. Existing thread: #1871 (comment) -
The ordinal worker pool uses fail-fast
Promise.all. If one worker rejects, sibling workers keep running after the reconcile pass and scheduler lane have failed, so network/store side effects can overlap the retry. Existing thread: #1871 (comment)
The assetUals and seal concern itself is not a blocker: this exact path is seeded from on-chain registration, responders intersect it with confirmed VM descriptors, and requesters verify chain root, version, CG binding, and receipt before materialization.
Please fix the three races and boundary violations above and add controlled regression tests before merge.
…t-sync test coverage Review blockers (lupuszr): - Re-check the captured local-CG -> on-chain-CG binding immediately after recoverPendingOrdinals resolves. Recovery is the longest await in a reconcile pass; a rebind landing during it could let outcomes recovered under the old binding advance or persist cursor state for the rebound CG. Staleness detected there is now treated exactly like staleness during ordinal work: outcomes discarded, no watermark move, scan cursor reset. - Gate every exact-recovery peer through ensurePeerAdmittedForRecovery before sending. Curator hints and getConnections() both predate the network-identity probe, so a merely-connected peer is not necessarily admitted; the batch path could previously send authenticated exact requests to unverified or rejected-network peers. - Contain ordinal-worker failures. The pool used fail-fast Promise.all, so one rejecting ordinal left sibling workers running past the pass's lifetime, overlapping their network/store side effects with the caller's retry. The first error now stops dispatch across all workers, in-flight ordinals drain, and only then does the pass reject. Coverage (review-bot asks): - runDurableSync integration: exactAssetUalsFor reaches both fetch phases and an old-responder full-CG payload is filtered before the verification worker sees it. - Page-fetch coalescing + durable single-flight identity now covered for assetUals (different batches never share a run; identical batches do). - parseSyncRequest wire tests for both formats: valid filters survive, present-but-invalid fail closed to [], absent stays undefined. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed 1. Post-recovery stale-binding race (#1871 (comment)) — 2. Admission boundary on exact-recovery peers (#1871 (comment)) — every peer in the batch loop is now gated through the existing 3. Fail-fast worker pool (#1871 (comment)) — worker bodies now trap their own errors: the first failure stops dispatch across all workers ( Also added the three test-coverage items from the review bot's sweep, since they guard exactly these seams:
Not taken, deliberately: the bot's structural refactors (extracting Validation: agent |
| }): Promise<SyncRow[]> { | ||
| if (params.assetUals !== undefined) { | ||
| if (params.assetUals.length === 0) return []; | ||
| const manifest = await readGraphScopedVmManifest( |
There was a problem hiding this comment.
🔴 Bug: Exact-asset recovery still scans the full VM manifest
What's wrong
This defeats the new exact recovery contract for large Context Graphs. A request for 1-10 assets can fail because unrelated assets push the full manifest over the responder snapshot budget, leaving the VM reconcile cursor pending even though the peer could have served the requested assets.
Example
A Context Graph with 70,000 confirmed V2 KAs receives an exact recovery request for one UAL. Instead of querying that UAL directly, the responder tries to build the whole manifest, hits the 64,000-row snapshot cap, and the exact recovery fails even though the requested batch is only one asset.
Suggested direction
Pass the exact UAL set into manifest loading and bound both the marker/descriptor queries to that set before applying the normal descriptor validation.
For Agents
In packages/agent/src/sync/responder/graph-plan.ts, update the exact branches of readDurableMetaPage and readDurableDataPage so they validate and load only the requested UALs, likely via a filtered manifest reader using VALUES ?ual { ... }. Preserve fail-closed behavior for empty/invalid filters, and add a test with more unrelated KAs than SYNC_RESPONDER_SNAPSHOT_BUILD_MAX_ROWS proving one requested UAL still syncs.
| * malformed value becomes an empty filter, which is fail-closed: a bad | ||
| * narrowing hint must never silently expand into a full-CG response. | ||
| */ | ||
| export function normalizeExactAssetUals(value: unknown): string[] | undefined { |
There was a problem hiding this comment.
🟡 Issue: The exact-asset filter needs a typed selection model instead of raw array sentinel values
What's wrong
The new abstraction still exposes low-level sentinel values as the contract. Because undefined, [], and non-empty arrays all mean different things, every layer has to remember the same invariant and branch on raw shape. That is exactly the kind of optionality churn that makes this sync path harder to evolve safely.
Example
parseSyncRequest stores assetUals: normalizeExactAssetUals(...), readDurableMetaPage treats [] as fail-closed no rows, buildSyncRequest rejects empty arrays through requireExactAssetUals, and sync-handler rebuilds assetUals === undefined ? 'full' : exactAssetFilterKey(assetUals) again.
Suggested direction
Make the selection state explicit once, then pass that model through the sync stack. That should collapse repeated undefined/empty/non-empty branches and make the fail-closed invariant local to the parser instead of being rediscovered at every layer.
Confidence note
This is a maintainability concern rather than a behavioral claim; the current tests may cover the intended undefined versus empty-array behavior.
For Agents
Look at packages/agent/src/sync/exact-assets.ts and the changed call sites in dkg-agent-cg-resolve.ts, sync-handler.ts, graph-plan.ts, and page-fetch.ts. Preserve fail-closed parsing, but introduce a discriminated selection type such as { kind: 'full' } | { kind: 'exact'; uals: NonEmptyArray<string> } | { kind: 'empty' }, with parser, serializer, and cache-key helpers on that type.
| params.signal, | ||
| ); | ||
| const requested = new Set(params.assetUals); | ||
| const confirmedUals = manifest.confirmedEntries |
There was a problem hiding this comment.
🟡 Issue: Exact-asset serving lacks a regression for requested tentative assets
What's wrong
The exact filter is a new serving path with a security-sensitive contract: it must narrow to confirmed VM assets only. The current added test does not exercise the case where the requested UAL is present but unconfirmed, so it would not catch a regression that leaks tentative workspace metadata or data through this branch.
Example
Seed a requested KA descriptor with dkg:status "tentative" and its VM graph data, then call readDurableMetaPage and readDurableDataPage with assetUals: [tentative.ual]. Expected: both pages are empty. A regression that used all complete manifest entries, or skipped the confirmed filter, should fail that test.
Suggested direction
Add a negative exact-asset responder test where the requested UAL exists locally but is not confirmed, and assert no descriptor or data rows are served.
For Agents
Extend packages/agent/test/exact-asset-responder.test.ts with a requested tentative asset. Keep the confirmed requested-asset test, and add assertions that exact meta/data serving returns nothing for a requested unconfirmed KA, ideally through the same exact branch and optionally through registerSyncHandler wiring.
|
Step-1 canary composition is up as #1876 (this change included): current testnet-canary + #1871 + #1852, gated green on the 6-node devnet sync suite (verify-fixes 8/10 excepted-only; proof 33/35 and 32/34, watermark-only failures — full numbers in #1876). Merging #1876 supersedes #1852. Big-CG pair #1868+#1842 follows as step 2. |
3 participants
Summary
Safety invariant
Already-complete KAs never enter the exact request. Exact filters have isolated checkpoint, session, and singleflight keys. Malformed filters fail closed, and all returned data still passes the existing authorization and Merkle verification path.
Why
VM reconciliation previously waited on SWM-oriented recovery and could refresh a whole CG once per ordinal. It also primed connections by dialing every discovered agent. A CG with an on-chain VM gap could therefore make little visible progress despite the missing UALs being known.
Configuration
DKG_VM_RECONCILE_BATCH_SIZE(default 10)DKG_VM_RECONCILE_ORDINAL_CONCURRENCY(default 5)DKG_VM_RECONCILE_CONCURRENCY(default 2)Validation
pnpm buildexact-batch:9; ordinal 15 was already complete and was excluded12D3KooWENez...9cxFnZh9and displaced a generic background sync after about four secondsRolling upgrade note
The requester-side correctness and store filter work immediately. The network-size and latency improvement requires the serving curator/host to run this PR; an older responder can still do an expensive whole-CG read before the requester narrows its response.