Skip to content

Fix VM reconcile fetch starvation - #1870

Open
lupuszr wants to merge 1 commit into
testnet-canaryfrom
codex/fix-vm-reconcile-starvation
Open

Fix VM reconcile fetch starvation#1870
lupuszr wants to merge 1 commit into
testnet-canaryfrom
codex/fix-vm-reconcile-starvation

Conversation

@lupuszr

@lupuszr lupuszr commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • enforce a single active catch-up fetch per admitted VM-reconciliation pass
  • retain the existing inter-pass cooldown and peer-rotation behavior
  • add a regression test where the first fetch outlives the 60-second cooldown

Root cause

The active-fetch safeguard was timestamp-only. If one fetch took longer than the cooldown, the same reconciliation pass could fetch again for the next unresolved ordinal. A stale graph could therefore monopolize the single dispatcher worker and leave live/manual reconciliation for unrelated graphs queued behind it.

Impact

One graph can now perform at most one network catch-up fetch per admitted pass. The pass then returns control to the dispatcher, allowing queued live catch-up work to run without increasing reconciliation concurrency or changing watermark semantics.

Validation

  • pnpm --filter @origintrail-official/dkg-agent exec vitest run test/chain-reconciler.test.ts test/core-fills-gap.test.ts (83 tests passed)
  • pnpm --filter @origintrail-official/dkg-agent run build

@lupuszr
lupuszr marked this pull request as ready for review July 21, 2026 07:15
shouldRunVmReconcileActiveFetch(this: DKGAgent, localCgId: string): boolean {
const now = Date.now();
this.pruneVmReconcileState(now);
if (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: The new per-pass fetch budget is modeled as ambient agent-wide state

What's wrong
This adds a second, implicit dimension to shouldRunVmReconcileActiveFetch without expressing it in the API. The method still accepts only localCgId, but its result now depends on whether some outer caller remembered to mark a pass as active. That makes the already large SWM host class harder to reason about and turns a local fetch-budget rule into scattered mutable lifecycle bookkeeping.

Example
A future caller that routes reconciliation work without going through executeVmReconcileForCg, or that adds another active-fetch path, now has to remember the hidden protocol: add localCgId to vmReconcileActivePasses, clear vmReconcileFetchUsedInPass, call shouldRunVmReconcileActiveFetch, then delete both sets in finally. Missing any part changes the fetch budget semantics even though the method signature still looks like a simple per-CG cooldown check.

Suggested direction
Make the pass budget explicit at the reconciliation boundary instead of spreading it across base-class sets and a global helper. A cleaner shape would create a fetch-admission gate for one executeVmReconcileForCg call, pass it through createVmReconcileDeps/reconcileChainOrdinal, and let that gate own both the one-per-pass flag and the existing cooldown timestamp. That removes the activePasses/fetchUsedInPass bookkeeping from DKGAgentBase and keeps the invariant next to the pass that owns it.

For Agents
Look at executeVmReconcileForCg, createVmReconcileDeps, reconcileChainOrdinal, and shouldRunVmReconcileActiveFetch. Preserve the current behavior of one active fetch per admitted reconciliation pass plus the existing inter-pass cooldown. Refactor the fetch admission state into a pass-local gate or a small VmReconcileFetchGate object owned by the reconcile pass/deps, then prove the existing damping tests and the new long-fetch test still pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants