fix(sync): restore foreground catch-up backpressure recovery - #1934
fix(sync): restore foreground catch-up backpressure recovery#1934branarakic wants to merge 2 commits into
Conversation
|
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, |
There was a problem hiding this comment.
🟡 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 { |
There was a problem hiding this comment.
🟡 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, |
There was a problem hiding this comment.
🟡 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 }, |
There was a problem hiding this comment.
🟡 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.
|
Closing — this PR's work is already on Evidence (verified 2026-08-06 against
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. |
Summary
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 buildCanary acceptance
After merge to testnet-canary, exercise an explicit foreground catch-up while the sync scheduler is saturated and confirm: