Skip to content

fix(sync): restore foreground catch-up backpressure recovery - #1934

Closed
branarakic wants to merge 2 commits into
mainfrom
codex/foreground-catchup-backpressure-1895
Closed

fix(sync): restore foreground catch-up backpressure recovery#1934
branarakic wants to merge 2 commits into
mainfrom
codex/foreground-catchup-backpressure-1895

Conversation

@branarakic

Copy link
Copy Markdown
Contributor

Summary

  • restores the foreground catch-up backpressure behavior originally introduced by fix(sync): retry foreground catchup under backpressure #1895 after it was removed from the exact canary candidate in test(canary): stage exact 10.0.10 Day-One sync candidate #1903
  • replaces raw priority and retry switches with a typed foreground/background catch-up policy shared by the in-agent and worker-backed CLI paths
  • gives explicit foreground catch-up elevated scheduler priority and bounded local-deferral retries at 100 ms, 250 ms, and 500 ms
  • completes durable VM catch-up before starting SWM, and retries SWM alone when only that plane is deferred
  • keeps automatic background catch-up best-effort with no waiting or priority override
  • includes mode and priority in single-flight identities so foreground work cannot silently join a background operation

This forward-port supersedes #1896, whose older branch now conflicts with current main.

Why

#1895 was removed from testnet-canary to make the #1903 tree exactly match the isolated release candidate. That was a test-candidate construction decision, not evidence that the foreground recovery behavior was incompatible. Without this behavior, a user-triggered subscribe/catch-up can report a local scheduler deferral immediately, and dependent SWM work can start before durable metadata has actually settled.

Validation

  • pnpm --filter @origintrail-official/dkg... run build
  • agent focused tests: 25 passed
  • CLI worker catch-up tests: 12 passed
  • full agent unit suite: 1,288 passed
  • broader CLI unit run: 1,730 passed and 48 skipped; five unrelated live-daemon suites could not start because the shared Hardhat context file was not present

Canary acceptance

After merge to testnet-canary, exercise an explicit foreground catch-up while the sync scheduler is saturated and confirm:

  1. local backpressure is retried within the bounded budget;
  2. durable VM sync settles before SWM starts;
  3. the request survives a transient deferral without returning an immediate false failure;
  4. persistent pressure remains bounded and reports deferred without starting dependent SWM.

@branarakic

Copy link
Copy Markdown
Contributor Author

Validation update: the follow-up commit e5e1120 preserves the pre-existing background catch-up call shape while retaining foreground priority/retry behavior. The exact previously failing agent shard 5 now passes, and the complete PR check set finished with no failures or pending checks. The implementation was also merged through canary PR #1937 (merge fb46af2) and auto-deployed successfully to all four testnet beacon nodes; all four reported healthy status, cleared updater locks, normal peer connectivity, zero admission work in flight, and no recent fatal/uncaught startup errors. Main merge remains intentionally gated on the required human review.

'syncDurable',
peerId,
request.contextGraphId,
priority,

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 worker RPC contract is becoming more brittle with another positional argument

What's wrong
This change deepens an already loose boundary between the worker and parent runner. Priority is feature-significant scheduling state, but it is carried as an anonymous optional tuple element through unknown[] and any, so the implementation now depends on call-site ordering rather than an explicit contract. That makes future catch-up options harder to add safely and keeps the orchestration harder to scan.

Example
The worker now sends invoke('syncDurable', peerId, contextGraphId, priority), while the parent decodes it with args as [string, string, number | undefined]. Adding the next sync option means changing positional tuple knowledge in both files again, with no named contract showing which arguments are valid for each method.

Suggested direction
Define a CatchupInvoke union such as { method: 'syncDurable'; peerId; contextGraphId; priority? } | { method: 'syncSharedMemory'; ... } and have both sides switch on that typed payload. That would turn the priority addition into a named field instead of another cast-dependent tuple slot.

For Agents
Look at packages/cli/src/catchup-runner-worker-impl.ts and packages/cli/src/catchup-runner.ts. Preserve the worker/parent behavior, but replace the string-plus-unknown[] invoke protocol for these catch-up methods with a discriminated typed message or named payload objects. Add a small test proving priority still reaches durable and SWM calls through the worker path.

// deep-importing the compiled `dist/` module.
export { mapWithConcurrency } from './map-with-concurrency.js';
export { CATCHUP_MAX_CONCURRENT_PEER_SYNCS } from './sync/catchup-concurrency.js';
export {

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 catch-up policy helper leaks internal orchestration onto the public agent surface

What's wrong
This export block turns a low-level orchestration helper, its generic callback context, and its testability hooks into public package API. That makes the retry strategy and priority plumbing harder to change later, because consumers can start depending on details that are really implementation policy between the agent and CLI worker.

Example
External callers can now import runCatchupPlanesWithPolicy and supply their own retryDelaysMs / wait, even though those knobs appear to be deterministic test seams and the only production consumer is the CLI catch-up worker.

Suggested direction
Either keep this helper behind an internal subpath/module used by the CLI worker, or expose a narrower production-facing API that does not include wait, retryDelaysMs, and the generic plane context types. Public exports should reflect stable concepts like CatchupMode, not the current retry-loop implementation details.

Confidence note
The CLI worker does need a way to share this helper with the agent package, so this may need a small internal export strategy rather than simply making the function private.

For Agents
Review packages/agent/src/index.ts and packages/agent/src/sync/catchup-policy.ts. Preserve the shared foreground/background behavior for agent and CLI worker paths, but avoid publishing test seams and low-level plane orchestration as general agent API. Verify the CLI worker still imports the shared implementation cleanly after the boundary change.

);
return this.runCatchupOverPeers(contextGraphId, includeSharedMemory, peers, {
totalPeers: orderedPeers.length,
mode,

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: Foreground mode is not verified through the public catch-up API

What's wrong
The changed behavior introduces a public mode option, but the current agent-path regression test bypasses the code that reads and forwards that option. This leaves the user-facing foreground catch-up path under-verified.

Example
If mode were accidentally omitted from the object passed to runCatchupOverPeers, syncContextGraphFromConnectedPeers(..., { includeSharedMemory: true, mode: 'foreground' }) would still run as background catch-up, but the new private-helper test would continue to pass.

Suggested direction
Cover the public API propagation path, not only runCatchupOverPeers directly.

For Agents
Add or adjust a test to call agent.syncContextGraphFromConnectedPeers('coalesced-cg', { includeSharedMemory: true, mode: 'foreground' }) with one connected peer and stubbed durable/SWM sync methods. Prove the first durable deferral is retried with FOREGROUND_CATCHUP_SYNC_PRIORITY before SWM starts, while preserving existing background behavior.

undefined,
undefined,
undefined,
priority === undefined ? undefined : { priority },

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: Worker bridge priority forwarding lacks coverage

What's wrong
The worker-backed catch-up path now depends on a new argument crossing the worker-to-agent boundary. Tests cover the worker emitting that argument, but not the host consuming it, so the production runner could silently lose foreground admission priority while tests remain green.

Example
A regression that changed line 560 to pass undefined, or dropped the third argument before calling syncSharedMemoryFromPeerDetailed, would still satisfy the worker-impl tests because those tests stop at the mocked invoke boundary.

Suggested direction
Add validation at the parent-runner boundary so the worker-emitted priority is proven to reach the agent APIs.

For Agents
Add a focused test for the worker host bridge, or extract the invoke dispatch into a testable helper. Simulate syncDurable and syncSharedMemory invoke messages with FOREGROUND_CATCHUP_SYNC_PRIORITY and assert the mocked agent receives { priority: FOREGROUND_CATCHUP_SYNC_PRIORITY } in the correct options position.

@branarakic

Copy link
Copy Markdown
Contributor Author

Closing — this PR's work is already on main, landed under different commit(s). Merging it now would regress main, because main has since evolved past the version on this branch.

Evidence (verified 2026-08-06 against main @ 0277d82c6):

  • The fix landed as 737f125b7 "fix(sync): retry foreground catchup under backpressure" (ancestor of main). git cherry origin/main marks this PR's fix commit 5f7c4c94f as already upstream (patch-id equivalent).
  • One commit remains non-upstream, e5e11209a "preserve background catchup call shape" — a behaviourally-neutral refactor of the runCatchupPlanesWithPolicy call in packages/agent/src/dkg-agent-lifecycle.ts (passing args positionally vs. omitting them; identical under JS default-parameter semantics).
  • Merging it would regress main: main's current call site also threads a source field ({ ...(priority === undefined ? {} : { priority }), source }) added after this branch. This PR's older shape drops source.
  • Note on evidence grade: unlike the others in this sweep, the raw patches are not byte-identical — equivalence here is by git cherry patch-id plus direct inspection of the residual.

Adjudicated independently and then re-checked by a second reviewer instructed to refute the claim; it survived. Closed as part of a sweep of open PRs whose code had already landed. Please reopen if this is wrong.

@branarakic branarakic closed this Aug 6, 2026
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