fix(sync): bound rootless snapshot progress and preserve VM lifecycle pointers - #1751
Conversation
| * by returning `null`, so callers cannot advance a cursor on attacker-shaped | ||
| * metadata or a mixed legacy/V2 snapshot. | ||
| */ | ||
| export function planBoundedGraphScopedDurableBatch( |
There was a problem hiding this comment.
🟡 Issue: Bounded snapshot planning is coupled to integrity verification and duplicates responder ordering
What's wrong
This adds a second authority for exact-graph ordering and safe cursor boundaries in the wrong layer. The code works by mirroring responder internals, but that hidden coupling makes the sync protocol harder to maintain and easier to drift. It also pushes more specialized requester orchestration into an already oversized integrity module.
Example
The requester now defines compareUnicodeCodePoints and sorts descriptors independently, while the responder sorts graph plan entries with its own compareCodePoint. A future responder ordering tweak would require a second, non-obvious update in the verifier module.
Suggested direction
Extract a shared pure exact-graph manifest/order module, and keep durable-integrity.ts focused on metadata parsing and integrity selection. The bounded planner can consume explicit parsed graph descriptors instead of reimplementing responder assumptions inside the verifier.
For Agents
Look at packages/agent/src/sync/durable-integrity.ts around the bounded planner and packages/agent/src/sync/responder/graph-plan.ts for the existing ordering contract. Preserve bounded rootless progress behavior, but move shared exact-graph ordering/prefix planning behind one typed helper used by requester and responder. Tests should prove requester/responder sort parity plus unchanged timed-out/resumed checkpoint offsets.
There was a problem hiding this comment.
🟡 Issue: Do not duplicate the responder's graph ordering rule
What's wrong
The new planner relies on exactly the same graph ordering as the responder but reimplements the comparator locally. That creates a hidden coupling: the bounded cursor logic only stays valid while two separate functions remain identical.
Example
If responder ordering changes in compareCodePoint, the requester-side bounded planner will keep using the cloned comparator unless someone remembers to update both places. This planner's cursor safety depends on matching that ordering exactly.
Suggested direction
Make the graph ordering comparator a shared canonical utility used by both the responder plan and requester bounded planner.
Confidence note
This assumes importing directly from the responder module is not the intended boundary; if that boundary is wrong, the comparator should still move to a shared sync ordering utility rather than be duplicated.
For Agents
Move the code-point comparator to a shared module under packages/agent/src/sync/ or reuse an existing shared utility, then update both durable-integrity.ts and sync/responder/graph-plan.ts to import it. Keep behavior identical and add/keep a test with non-ASCII graph names if one exists for ordering-sensitive paths.
| const fetchDurationMs = Date.now() - fetchStartedAt; | ||
| const isSystemContextGraph = (Object.values(SYSTEM_CONTEXT_GRAPHS) as string[]).includes(pid); | ||
|
|
||
| let effectiveDataResult = dataResult; |
There was a problem hiding this comment.
🟡 Issue: The bounded branch creates split data-result state inside the main sync loop
What's wrong
The main durable sync loop now has a small hidden state machine spread across local variables. That makes the loop harder to reason about because different downstream operations must choose between raw and projected data. The structure is especially brittle in a long function that already handles fetch, verify, store, checkpoints, summaries, and callbacks.
Example
In the same iteration, verification reads dataForVerification, summary bytes read raw dataResult, checkpointing reads effectiveDataResult, and full-snapshot notification gates on effectiveDataResult. A later edit to this function has to know which version is correct for each use site.
Suggested direction
Collapse the three mutable variables into a single typed plan object returned by a helper. Give the raw fetch result and checkpoint/effective result distinct field names so the rest of the loop cannot accidentally consume the wrong one.
For Agents
In packages/agent/src/sync/requester/durable-sync.ts around the bounded-progress branch, introduce a typed DurableBatchPlan or prepareDurableVerificationInput helper with explicit raw-result, checkpoint-result, verification-quads, and verification-mode fields. Preserve existing summary counters, logging, checkpointing, timeout/backoff behavior, and sinceBatchId behavior.
There was a problem hiding this comment.
🟡 Issue: Make bounded durable projection a first-class phase object
What's wrong
The bounded-progress feature is implemented by mutating a shadow copy of the fetched data result and manually swapping it into selected downstream paths. That adds incidental branching and makes the sync loop harder to reason about because there is no explicit boundary for "raw fetch result" versus "safe page to verify and checkpoint."
Example
A future edit near recordPhaseOutcome or timeout handling can easily use dataResult instead of effectiveDataResult, which would silently checkpoint or classify the raw timed-out page instead of the bounded safe page. The current code already has to thread the distinction through several call sites after line 244.
Suggested direction
Collapse the raw/effective page split behind a small typed abstraction before verification starts, so the rest of the sync loop has one obvious source for checkpoint and timeout state.
For Agents
In packages/agent/src/sync/requester/durable-sync.ts, replace the ad-hoc mutable projection with a named preparation step, for example prepareDurableDataPhase(...), that returns explicit fields for raw metrics, verification input, checkpoint page, and verification mode. Then downstream code should consume that prepared object instead of choosing between raw/effective variables. Preserve bounded-progress behavior and existing summary/checkpoint semantics.
| if (!hasAuthoritativeMeta) { | ||
| await this.refreshMetaFromCurator(contextGraphId, curatorMetaRefreshOptions); | ||
| hasAuthoritativeMeta = await this.hasConfirmedMetaState(contextGraphId) | ||
| let includeSharedMemory = true; |
There was a problem hiding this comment.
🟡 Issue: Curator retry policy is embedded as another state machine in the lifecycle file
What's wrong
This change adds more control-flow weight to an already sprawling lifecycle module. The behavior is policy-heavy, but it is encoded as ad-hoc nested branching inside the outer method, which makes future changes to post-approval sync harder to scan and reason about.
Example
The connected-curator branch now carries includeSharedMemory, total counters, hasAuthoritativeMeta, madeVerifiedProgress, denial handling, the max-round cap, and two fallback log paths in one nested loop before returning to the outer broadcast fallback.
Suggested direction
Move the curator-direct retry policy into a dedicated helper that returns { succeeded, totals, fallbackReason } or similar. Let runImmediatePostApprovalSync read as orchestration: authenticate curator, attempt direct bootstrap, then choose whether to broadcast fallback.
For Agents
Extract the new loop around runImmediatePostApprovalSync into a focused helper such as runCuratorDirectPostApprovalCatchup. Preserve the exact sequence: initial curator meta refresh, first catchup with SWM, disable SWM after SWM progress, retry only while verified progress plus authoritative metadata are present, and fall back on denied/no-progress/no-meta/cap. Existing post-approval tests should assert the helper outcome and call sequence.
There was a problem hiding this comment.
🟡 Issue: Move the curator retry state machine out of the lifecycle method
What's wrong
This change grows an already busy lifecycle path with a multi-condition retry state machine. The behavior may be right, but the structure makes future changes risky because SWM readiness, durable progress, metadata proof, retry termination, and fallback logging are all interleaved in one branch.
Example
The loop now has to coordinate these concepts at once: call runCatchupOverPeers, accumulate totals, decide whether SWM is done, refresh/confirm metadata, decide success, decide fallback, and decide retry. That makes the method harder to scan and harder to change without accidentally altering one of those policies.
Suggested direction
Create a small helper or policy object for curator-targeted post-approval catchup, so the lifecycle method reads as orchestration and the retry/fallback rules are isolated and named.
For Agents
Extract the curator-direct retry policy from runImmediatePostApprovalSync in packages/agent/src/dkg-agent-lifecycle.ts into a focused helper that returns a discriminated result such as success/fallback with totals and reason. Preserve the existing behavior: initial curator meta refresh, retry while verified progress is made, keep SWM enabled until clean completion, fallback on denied/no-progress/missing-meta/cap. Keep the existing post-approval tests proving the same call sequences.
| // is independently verifiable. Keep them in scope on every bounded round; | ||
| // this is idempotent and avoids an offset ambiguity for zero-width entries. | ||
| for (const descriptor of descriptors) { | ||
| if (descriptor.publicTripleCount === 0) completeGraphs.add(descriptor.assertionGraph); |
There was a problem hiding this comment.
🟡 Issue: Zero-public bounded snapshot behavior is untested
What's wrong
The new bounded planner has special handling for zero-public V2 assets so private-only metadata remains in scope during partial rootless progress. The added tests cover positive-size graphs, boundary replay, resumed suffix completion, and mixed metadata, but not this new zero-width branch. That leaves a data-integrity/readiness path for private-only assets without direct validation.
Example
A timed-out rootless snapshot contains one complete public graph plus metadata for a fully private KA with publicTripleCount=0. The planner should include the private-only assertion graph in changedDataGraphs, runDurableSync should persist its verified metadata, and the data checkpoint should still advance only by the public rows. A regression that removed the zero-public inclusion would currently still pass the added tests.
Suggested direction
Cover the zero-public branch with a regression test that goes through the bounded changelogPage mode, not only the existing fullSnapshot/private-only verifier tests.
For Agents
Add a bounded-progress test in packages/agent/test/rootless-durable-bounded-progress.test.ts with a private-only graph-scoped metadata fixture. Verify planBoundedGraphScopedDurableBatch includes its assertion graph in changedDataGraphs and a runDurableSync path persists the private-only metadata / reports verifiedPrivateOnlyResponses without changing the safe row offset.
There was a problem hiding this comment.
🔴 Bug: Bounded durable checkpoint trusts graph counts without proving row order
What's wrong
The new bounded-progress path is supposed to advance the cursor only at a graph-aligned prefix. It currently proves that the batch contains enough rows for earlier graphs, but not that those rows appeared before the partial graph in the responder stream. A malformed or malicious peer can make the requester persist verified rows and advance a cursor that was not actually observed at that stream boundary, which can skip or duplicate rows on resume and undermine the safety property this code is relying on.
Example
For descriptors ordered as A(2 rows), B(2 rows), a peer can return rows [B1, A1, A2] with rawNextOffset=3. The planner counts A as complete and B as partial, filters/stores A1+A2, and checkpoints offset 2. But the first two received rows were not the A boundary, so the checkpoint is not proven to be a safe boundary in the responder stream.
Suggested direction
Track the expected graph while walking dataQuads in order, not just aggregate counts. Only return a bounded batch when the received rows are a contiguous prefix of the graph-ordered manifest.
Confidence note
This depends on treating sync peers as untrusted; the standard responder emits graph-ordered rows, but the requester is accepting peer-supplied pages before verifying that order.
For Agents
In packages/agent/src/sync/durable-integrity.ts, make planBoundedGraphScopedDurableBatch validate the actual dataQuads sequence against descriptor order from resumedFromOffset: complete graphs must appear contiguously and the final graph may be the only partial one. Preserve the existing fail-closed behavior and add a test with out-of-order graph rows that returns null instead of checkpointing.
|
CI triage: the red jobs reproduce unchanged on #1752, whose runtime diff is zero (harness shell only). Failures are the existing post-rootless full-matrix debt: legacy subset/lifecycle expectations, node-ui named-graph seed fixtures that now hit the intentional atomic-KA guard, and existing private-only/chain-alias cases. The changed focused suites, agent build, and the full live 50+50 autoApprove/SWM/VM/restart/resource gate pass. Proceeding with the time-boxed certified fix; genuinely relevant baseline failures remain in the release follow-up audit. |
2 participants
Summary
dkg:reservedUalcontrol and fail closed on conflicting graph claimsTestnet evidence
Validated as a direct node-2 hotfix before opening this PR. The full private-CG gate passed with autoApprove, 50 SWM KAs + 50 VM KAs, 1,000 triples per KA, late-join recovery, restart recovery, exact per-KA digest/lifecycle verification, and fleet crash/OOM/resource auditing. Result: 100/100 KAs and 100,000/100,000 triples recovered both before and after restart; 50/50 VM publications finalized; health gate passed.
Verification
pnpm exec vitest run --config vitest.unit.config.ts test/rootless-durable-bounded-progress.test.ts test/sync-control-metadata-admission.test.ts(13/13)pnpm exec vitest run --config vitest.config.ts test/context-graph-discovery.test.ts(83/83)pnpm --filter @origintrail-official/dkg-agent build