fix(rfc64): settle SWM only after VM commit - #2040
Conversation
d52ab96 to
8cd5a47
Compare
c73afcc to
cd49cfe
Compare
8cd5a47 to
6d75cc0
Compare
| // snapshot lock above: the finalizer acquires that same lock before | ||
| // stamping the local retirement marker. A failed insert therefore keeps | ||
| // the recovery snapshot visible and cannot deadlock settlement. | ||
| if (settleGraphScopedSnapshot) { |
There was a problem hiding this comment.
🟡 Issue: Snapshot materialization can now be wired without its required settlement step
What's wrong
This PR reintroduces two independent optional knobs for one protocol. The code comment says settlement is post-commit policy for materialized snapshots, but the type system no longer enforces that relationship, so readers and future callers must remember a hidden invariant instead of being guided by the API.
Example
A future caller or test harness can pass snapshotMaterializer and publicSnapshotStore but omit settleGraphScopedSnapshot; the sync path will still write verified snapshots and metadata, then silently skip post-commit retirement. That is exactly the half-wired mode the old cohesive materializer contract was designed to prevent.
Suggested direction
Keep the ordering outside the lock, but model materialization plus post-commit settlement as one dependency again. A small GraphScopedSnapshotCommitter/commitSettledSnapshots abstraction could own the full protocol while still exposing an internal store adapter for the lock-bound writes.
For Agents
Look at SharedMemorySyncContext, runSharedMemorySync, and createSharedMemorySnapshotMaterializer. Preserve the current ordering requirement: materialize under the per-KA lock, insert verified metadata, then settle outside the lock. Prove the contract cannot be half-wired, ideally with a type-level/harness case where materialization-enabled sync must also provide settlement.
| * verified metadata insert succeeds and outside the per-KA write lock. | ||
| */ | ||
| snapshotMaterializer?: SharedMemorySnapshotMaterializer; | ||
| settleGraphScopedSnapshot?: ( |
There was a problem hiding this comment.
🟡 Issue: The split settlement dependency is not verified against the half-configured case
What's wrong
This change moves post-commit settlement out of SharedMemorySnapshotMaterializer, but the new callback is optional and the coordinator no-ops when it is missing. The added tests validate the desired ordering only in harnesses that provide the callback, so they would not fail if a production or future caller materialized verified public SWM snapshots without retiring the finalized recovery copy.
Example
A caller supplies snapshotMaterializer and publicSnapshotStore so a verified graph-scoped snapshot is materialized, but omits settleGraphScopedSnapshot. runSharedMemorySync still inserts metadata, records phase completion, and never calls retireSyncedGraphScopedSwmIfFinalized, so the missing post-commit retirement is not caught by the current tests.
Suggested direction
Either make settlement a required dependency whenever snapshot materialization is enabled, or add a focused test proving that omitting it fails or leaves the phase incomplete instead of silently checkpointing.
Confidence note
I did not run the test suite in the read-only sandbox, but the diff and surrounding tests show the coordinator only exercises settlement when a callback is provided.
For Agents
Look at runSharedMemorySync in packages/agent/src/sync/requester/shared-memory-sync.ts and its snapshot materialization tests. Preserve the new ordering where settlement runs after verified metadata insertion and outside the KA lock, but add a regression test or contract check for the half-configured case: materializer present with settled descriptors but no settleGraphScopedSnapshot should not silently complete as successful materialization.
User impact
An Edge node no longer hides a synchronized SWM assertion until the corresponding VM transition is genuinely durable:
This closes the failure mode where a store/RPC interruption could make content disappear from the user-facing SWM view before VM recovery completed.
Before
sequenceDiagram participant Sync as Sync or reconcile participant Store as Triple store participant SWM as SWM query view Sync->>SWM: Write finalized marker early Sync->>Store: Repair VM metadata Store--xSync: Repair fails Note over SWM: Matching SWM is hidden despite incomplete VM repairAfter
sequenceDiagram participant Sync as Sync or reconcile participant Lock as Per-KA lock participant Store as Triple store participant Settle as Finalization settlement participant SWM as SWM query view Sync->>Lock: Materialize verified graph and head Lock->>Store: Commit graph and verified metadata Store-->>Sync: Commit succeeds Sync->>Settle: Revalidate finalized VM outside lock Settle->>SWM: Retire only matching current head Note over Sync,SWM: Any commit failure leaves SWM visible and retryableImplementation
SharedMemorySnapshotMaterializeras a store-only adapter;SharedMemorySnapshotCommitterthat composes materialization with post-commit settlement without lock recursion.Validation
pnpm --filter @origintrail-official/dkg-agent buildka-graph-finalization-handler.test.tsswm-public-snapshot-materialization.test.tsswm-snapshot-materializer.test.tsgit diff --checkStacked on #2039.