Skip to content

Promote #1895 to main (catchup backpressure retry) — release-bound for v10.0.9 - #1896

Closed
branarakic wants to merge 3 commits into
mainfrom
release/promote-1895-to-main
Closed

Promote #1895 to main (catchup backpressure retry) — release-bound for v10.0.9#1896
branarakic wants to merge 3 commits into
mainfrom
release/promote-1895-to-main

Conversation

@branarakic

Copy link
Copy Markdown
Contributor

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

undefined,
stats?.priority === undefined ? undefined : { priority: stats.priority },
);
const durable = await (

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: 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. */

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: 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

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: 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 },

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: 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.

@branarakic

Copy link
Copy Markdown
Contributor Author

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 51dd0a534, before #1895 — so nothing below affects the shipped release. But one finding is confirmed and worth fixing before this promotes to main.

🔴 CONFIRMED: background callers silently lose the SWM plane when the durable plane defers

runCatchupOverPeers (dkg-agent-lifecycle.ts:5492) now skips the SWM plane whenever durable.deferredBackpressure > 0:

if (includeSharedMemory && (durable.deferredBackpressure ?? 0) === 0) { ... }

…but the compensating bounded retry only runs when stats?.retryDeferredBackpressure is set (5482/5499), which only the CLI catch-up runner passes. Three background callers pass neither priority nor retry flag and were verified to reach this gate:

  • VM-reconcile active fetch — dkg-agent-swm-host.ts:3944
  • post-approval curator rounds — dkg-agent-lifecycle.ts:5782
  • broadcast fallback — dkg-agent-lifecycle.ts:5861

Pre-PR (origin/main:5439-5441) all three attempted SWM unconditionally after the durable pass. Post-PR they skip it with no retry, so under sustained backpressure their SWM sync simply does not happen.

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 (planSharedMemorySyncContextGraphs ~4834), and per-CG admission wraps the whole durable pass before any fetch (4123-4140) — so a deferred VM plane says nothing about whether the SWM plane could have proceeded. The skip is a real behavior loss, not a redundant guard.

Suggested fix: make the retry unconditional (default retryDeferredBackpressure on), or gate the skip on the same condition as the retry so a caller can never get skip-without-retry.

✅ REFUTED: priority-split single-flight keys allow duplicate SWM streams

True 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 fetchSyncPages wrapper, which coalesces by syncPageFetchCoalescingKey (4571-4700) and does not include priority. No duplicate stream results.

Minor (not verified, noted for the author)

  • WorkerCatchupRunner.close() awaits worker.terminate() without rejecting pendingRuns and registers no exit handler — an in-flight run() never settles at shutdown (catchup-runner.ts:243/208).
  • The retry bound covers only busy-rejected admission; when the global queue has room the foreground attempt queues and awaits admission with no timeout, bypassing the backoff entirely (backpressure.ts:270-287).
  • The replaced CLI worker test dropped the assertion that a persistently-deferred SWM plane surfaces deferredBackpressure=1 and blocks finalizeCatchup — that assertion pinned exactly the behavior the confirmed finding is about.

Two further concurrency claims about the changelog lane are still being verified; I'll follow up here with those verdicts.

🤖 Generated with Claude Code

@branarakic

Copy link
Copy Markdown
Contributor Author

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.

@branarakic

Copy link
Copy Markdown
Contributor Author

Closing — this PR's code is already on main, so merging it would change nothing.

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

  • git cherry origin/main reports no new patches — the promote is already reconciled; only the merge commit remains. Main has since moved to 10.0.12, while this branch carries the 10.0.9 bump.
  • git merge-tree --write-tree origin/main 33c7e0f22 returns main's exact tree (d1812f293) — a merge would be a no-op.

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