fix(sync): promote discovery after partial bootstrap - #2049
Conversation
d12e95f to
1d04c6c
Compare
| return finishSyncAccounting(); | ||
| } | ||
| if (accounting.backoffWorthyFailure) { | ||
| if (phase.afterResult) await runNonTransportStep(phase.afterResult); |
There was a problem hiding this comment.
🔴 Bug: Bootstrap discovery now runs after any backoff-worthy aggregate result
What's wrong
The code assumes a backoff-worthy bootstrap result means authoritative discovery records may already be durably committed, but the aggregate result does not prove the Agents/Ontology portion completed. Because discovery reads whatever is currently in the store, a timeout during the ontology graph itself can promote a partial registry snapshot into subscription/admission state.
Example
Bootstrap requests [Agents, Ontology, configured-cg]. If Ontology times out after committing only an early page containing rdf:type/name but before the page containing accessPolicy/registration binding, the aggregate result is backoff-worthy. The new hook still runs discovery, so a core node can record that context graph with incomplete metadata, for example as core-hosting-only instead of automatic-public/private. Expected behavior is to promote discovery only when the ontology discovery records being scanned are known complete enough, or to wait for the retry.
Suggested direction
Gate afterResult on phase-specific completion evidence, split the bootstrap request so Ontology completion is known before later graphs can stop the fanout, or make the durable result report which context graph completed so discovery only runs for a completed ontology prefix.
Confidence note
This depends on whether the durable requester can expose per-context-graph completion for the bootstrap batch; the surrounding requester code shows it can stop after a timeout in any requested graph while still returning already inserted triples.
For Agents
Look at packages/agent/src/sync/on-connect/sync-on-connect.ts and the durable sync result shape from packages/agent/src/sync/requester/durable-sync.ts. Preserve the intended case where Agents/Ontology completed before a later graph timed out, but avoid running discovery when the timeout/backoff occurred before or during Ontology completion. Add a focused test where the bootstrap aggregate has inserted triples plus timedOutPhases from an incomplete ontology phase and prove discovery is not promoted until safe.
| * prefix may already contain authoritative discovery records even when a | ||
| * later graph in the same phase timed out. | ||
| */ | ||
| afterResult?: () => Promise<void>; |
There was a problem hiding this comment.
🟡 Issue: Avoid hiding bootstrap discovery behind a generic phase hook
What's wrong
This turns a very specific lifecycle requirement into a generic optional callback on the shared phase model. The result is more indirect and more fragile: the runner now has duplicated hook calls in multiple branches, while the type gives readers no clear ownership boundary for why this callback exists or which phases may use it.
Example
A future fanout stop branch added to runSyncPhase now has to remember to call phase.afterResult before finishSyncAccounting(). Nothing in the type expresses that this hook is really a bootstrap durable post-accounting step, and shared phases can also provide it even though the documented invariant is durable-prefix discovery.
Suggested direction
Make the bootstrap post-accounting behavior explicit instead of adding a broad afterResult escape hatch. A cleaner shape would compute the phase stop reason once, run a single phase epilogue that knows about durable bootstrap discovery, and then handle the stop outcome, eliminating the three duplicated optional-callback calls.
For Agents
Look in runSyncOnConnectWithScopePlan and keep the current ordering: bootstrap discovery must happen after durable transport accounting and before final peer-sync accounting when the bootstrap phase stops. Refactor the phase runner so the stop decision is computed once, run a named post-accounting epilogue once, then return/log/finish from one place. Add/keep a focused test that proves bootstrap discovery occurs before the peer outcome callback on backoff-worthy pressure.
| `Synced ${accounting.insertedTriples} ${phase.label} ${dataLabel} triples from peer ${shortPeer}`, | ||
| ); | ||
| if (accounting.deferredByBackpressure) { | ||
| if (phase.afterResult) await runNonTransportStep(phase.afterResult); |
There was a problem hiding this comment.
🟡 Issue: Deferred-backpressure discovery promotion is not verified
What's wrong
The new behavior is intended to run the bootstrap afterResult hook even when sync fanout stops early. Tests now cover the backoff-worthy timeout branch, but not the separate admission-deferral branch added here. That leaves the deferral half of the behavior unverified, even though it is the path that returns deferred-backpressure and is explicitly changed by this line.
Example
A regression that removes if (phase.afterResult) await runNonTransportStep(phase.afterResult); from the deferred-backpressure branch would still leave the updated tests green. A focused test could return { insertedTriples: 1, insertedDataTriples: 1, completedPhases: 1, checkpointAdvances: 1, deferredBackpressure: 1 } from bootstrap sync and assert discoverContextGraphsFromStore.calls is [[]] before the function returns deferred-backpressure.
Suggested direction
Add a regression assertion for the deferredByBackpressure early-return path, parallel to the new backoff-worthy pressure test.
For Agents
In packages/agent/test/sync-on-connect-retry.test.ts, update one of the deferred-backpressure cases or add a new bootstrap deferred case to use a recorder for discoverContextGraphsFromStore. Preserve the existing assertions that shared-memory fanout does not continue and peer accounting remains correct, and add the assertion proving committed discovery is promoted before returning on local admission deferral.
User impact
A cold Core can now turn public Context Graph definitions that were already durably received during a partial system bootstrap into bounded automatic VM and SWM coverage. A timeout later in the same bootstrap batch no longer leaves those public graphs invisible until an unrelated future discovery event.
The pressure stop remains intact: the node records the committed discovery state, stops the pressured peer fanout, and lets the existing coverage-epoch reconciler schedule a fresh bounded public round. Edge selection rules, private-CG exclusion, batch size, and concurrency are unchanged.
Live failure reproduced
The anchored M1 testnet run placed all five fresh graph definitions and on-chain bindings in the cold Core store. A large
agentstransfer later returned a verified prefix with a backoff-worthy timeout. The peer round exited beforediscoverContextGraphsFromStore, leavingtrackedContextGraphs: 0and producing no public Core VM/SWM work.Before
After
Implementation
Validation
pnpm exec vitest run packages/agent/test/sync-on-connect-churn.test.ts packages/agent/test/sync-on-connect-retry.test.ts --reporter=dot: 77 passedpnpm --filter @origintrail-official/dkg-agent build: TypeScript, type tests, package-root test passedpnpm test:m1:rfc64-selective-coverage:unit: 82 passedgit diff --check: passedStack
Base: #2048 (
codex/rfc64-m1-live-gate-timeout)This is the next independently reviewable M1 PR and is not merged into
testnet-canaryormain.