Promote #1895 to main (catchup backpressure retry) — release-bound for v10.0.9 - #1896
Promote #1895 to main (catchup backpressure retry) — release-bound for v10.0.9#1896branarakic wants to merge 3 commits into
Conversation
fix(sync): retry foreground catchup under backpressure
| undefined, | ||
| stats?.priority === undefined ? undefined : { priority: stats.priority }, | ||
| ); | ||
| const durable = await ( |
There was a problem hiding this comment.
🟡 Issue: Centralize the foreground catch-up plane sequencing instead of duplicating it
What's wrong
The retry helper only abstracts the delay loop, leaving the higher-value policy scattered in two runners. That makes the architecture more fragile and keeps the inline and worker catch-up paths coupled by convention rather than code.
Example
A future change to the foreground catch-up policy, such as changing when SWM can start or how a deferred durable result is handled, now has to be made in both the inline lifecycle path and the worker path. The code already has comments elsewhere saying the two paths must mirror each other; this diff adds another mirrored behavior instead of making that invariant structural.
Suggested direction
Move the durable/SWM sequencing policy into one shared helper in the agent sync layer. The worker can still supply RPC-backed closures, but it should not have to know or duplicate the durable-before-SWM retry policy.
For Agents
Look at LifecycleSyncMethods.runCatchupOverPeers and runCatchup in catchup-runner-worker-impl.ts. Preserve current behavior: retry durable on local backpressure, do not start SWM while durable remains deferred, retry SWM without rerunning completed durable work, and keep per-peer failure isolation. Extract a shared runCatchupPeerSyncRound-style helper that takes durable/shared closures and empty-result handlers, then use it from both paths.
| includeSharedMemory?: boolean; | ||
| maxPeers?: number; | ||
| peerRotationKey?: string; | ||
| /** Admission override used by explicit foreground catch-up callers. */ |
There was a problem hiding this comment.
🟡 Issue: Model foreground catch-up as one admission policy, not loose flags
What's wrong
The foreground catch-up concept leaks through several layers as an optional number plus an optional boolean. Those options only make sense as a coordinated policy, but the API lets callers assemble mismatched combinations and forces readers to trace multiple flags through durable, changelog, shared-memory, and CLI worker boundaries.
Example
A new explicit catch-up caller can pass { priority: FOREGROUND_CATCHUP_SYNC_PRIORITY } without retryDeferredBackpressure, or retry deferred work without foreground priority. Both combinations are representable even though the comments describe one foreground admission mode.
Suggested direction
Replace the naked priority plus retryDeferredBackpressure plumbing with a named mode or typed SyncAdmissionPolicy that carries the intended priority, retry behavior, and single-flight identity together.
For Agents
Review DurableSyncOptions, syncSharedMemoryFromPeerDetailed options, syncContextGraphFromConnectedPeers options, and the CLI runner calls. Preserve the current foreground priority value and bounded retry behavior, but represent them as one named admission policy or mode and derive the single-flight key fields from that policy.
| stats?.priority === undefined ? undefined : { priority: stats.priority }, | ||
| ); | ||
| const durable = await ( | ||
| stats?.retryDeferredBackpressure |
There was a problem hiding this comment.
🟡 Issue: Agent catch-up retry behavior is not verified at the agent entry point
What's wrong
The changed branch is what inline/direct foreground catch-up uses, but the new tests do not call this entry point with retryDeferredBackpressure. A regression that removed this retry wrapper, ran SWM while durable was still locally deferred, or reran durable after only SWM deferred would still pass the added helper and worker-implementation tests.
Example
Failing-test sketch: stub one connected sync-capable peer, make syncFromPeerDetailed return { deferredBackpressure: 1 } and then a clean durable result, call syncContextGraphFromConnectedPeers('cg', { includeSharedMemory: true, retryDeferredBackpressure: true, priority: 2000 }), and assert durable is called twice, SWM is not called until durable clears, and the final result has deferredBackpressure: 0.
Suggested direction
Add a targeted agent-path test for the new retry flag instead of relying only on the helper unit test and the worker implementation's duplicate flow.
For Agents
Add an agent-level regression test near packages/agent/test/sync-fetch-coalescing.test.ts or the catch-up tests. Exercise syncContextGraphFromConnectedPeers or runCatchupOverPeers with retryDeferredBackpressure: true, preserve the no-rerun durable behavior when only SWM defers, and prove the aggregate result clears or reports deferred pressure correctly.
| undefined, | ||
| undefined, | ||
| undefined, | ||
| { priority: FOREGROUND_CATCHUP_SYNC_PRIORITY }, |
There was a problem hiding this comment.
🟡 Issue: CLI runner priority handoff is not covered
What's wrong
The production worker runner now attaches the foreground priority before calling the agent, but none of the added tests exercise this parent-side switch. If these options were omitted or wired to the wrong argument slot, the requester priority tests would still pass and the worker implementation tests would still pass because they stub the invoke result after this boundary.
Example
Failing-test sketch: mock node:worker_threads.Worker so it emits invoke messages for syncDurable and syncSharedMemory, pass an agent whose sync methods are vi.fn, then assert syncFromPeerDetailed receives { priority: FOREGROUND_CATCHUP_SYNC_PRIORITY } as its options argument and syncSharedMemoryFromPeerDetailed receives the same priority options.
Suggested direction
Cover the runner boundary where foreground priority is attached, because lower-level priority admission tests only prove the agent respects an option after it receives one.
For Agents
Add runner-boundary tests in packages/cli/test/catchup-runner.test.ts for WorkerCatchupRunner.invokeAgent via a mocked Worker, and a small createInlineCatchupRunner test that asserts it forwards priority: FOREGROUND_CATCHUP_SYNC_PRIORITY and retryDeferredBackpressure: true to syncContextGraphFromConnectedPeers.
|
Independent adversarial review of the #1895 diff (4 lenses, findings then verified by separate agents). This PR is not in v10.0.9 — the release was cut from 🔴 CONFIRMED: background callers silently lose the SWM plane when the durable plane defers
if (includeSharedMemory && (durable.deferredBackpressure ?? 0) === 0) { ... }…but the compensating bounded retry only runs when
Pre-PR ( The key premise was checked rather than assumed: durable deferral does not imply SWM would also fail. The SWM plan is computed from already-local metadata ( Suggested fix: make the retry unconditional (default ✅ REFUTED: priority-split single-flight keys allow duplicate SWM streamsTrue that the keys now differ so foreground/background no longer coalesce — but a deeper, priority-blind dedup layer survives: all SWM page streams pass through the agent's Minor (not verified, noted for the author)
Two further concurrency claims about the changelog lane are still being verified; I'll follow up here with those verdicts. 🤖 Generated with Claude Code |
|
Forward-port replacement is now in #1934. It restores the #1895 foreground catch-up behavior against current main and addresses the outstanding review concerns with a typed shared policy plus agent, worker, retry-order, and priority-aware single-flight coverage. I am leaving this PR open for history until #1934 is accepted. |
|
Closing — this PR's code is already on Evidence (verified 2026-08-06 against
Closed as part of a sweep of open PRs whose code had already landed. Please reopen if this is wrong. |
Brings #1895 (fix(sync): retry foreground catchup under backpressure, by @branarakic) from testnet-canary to main so it ships in v10.0.9. Merge is conflict-free; no overlap with the #1894 version bump.
Author's validation: full agent unit suite 1,257 passed; live saturated-node trial advanced durable VM progress 0 → 38/38 graphs. An independent adversarial review of the diff is running in parallel; findings (if any) will be posted here before release tagging.
🤖 Generated with Claude Code