Repository navigation
fix(mesh): a forwarded SaveMeshNodeRequest keeps its version — a stale snapshot is refused and answered, never applied - #6005
Merged
Conversation
…e whole-node snapshot is refused and answered, never applied The forwarder that turned a non-owner SaveMeshNodeRequest into a checked CreateOrUpdateNodeRequest dropped the snapshot's Version. Full-instance mode takes Content wholesale and floors the version on the stored row, so a snapshot taken before the node's latest write won over it and was acknowledged as a success. The raw path it replaced was protected by the storage layer (a save at or below the flushed version is dropped; a strictly lower one is a MonotonicWriteGuard conflict). - CreateOrUpdateNodeRequest.SnapshotVersion: the version a whole-node snapshot was taken at. The update branch refuses when the node carries a strictly higher version AND the snapshot would change it — read against the durable row and against the node as the handling hub holds it inside the write lambda. - A snapshot at the current or a higher version writes. An older snapshot that changes nothing is acknowledged as unchanged: a cross-hub stream.Update returns the node at the base version, so "update, then save the result" always offers one version behind. - The precondition is evaluated only for a requester established to hold Update; anyone else is refused as unauthorised whatever version they offered. - The refusal is answered: FailureKind StaleSnapshot naming both versions, and the save's sender receives a DeliveryFailure. Every other refusal of a forwarded save is answered the same way instead of being a log line only. - Three catalog keys (en + de). Doc: ParticipantIngress, MeshNodeVersioning. - AForwardedSaveKeepsItsVersionTest: red in ~1.5 s without the forwarder's SnapshotVersion, green with it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
rbuergi
enabled auto-merge
October 2, 2026 18:10
Contributor
Contributor
Test Results (shard 4) 4 files ± 0 4 suites ±0 2m 32s ⏱️ - 3m 6s Results for commit cd3b864. ± Comparison against base commit 1d5a337. This pull request removes 458 and adds 576 tests. Note that renamed tests count towards both. |
Contributor
Test Results (shard 2) 4 files ±0 4 suites ±0 2m 49s ⏱️ -2s Results for commit cd3b864. ± Comparison against base commit 1d5a337. This pull request removes 4 and adds 2 tests. Note that renamed tests count towards both. |
![systemorph-com[bot]](https://avatars.githubusercontent.com/u/77832550?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Follow-up to #5985: the SaveMeshNodeRequest forwarder now stamps CreateOrUpdateNodeRequest.SnapshotVersion (Math.Max(node.Version, 1)) onto the checked upsert, and the update branch refuses a snapshot whose offered version is strictly below the durable row's or the handling hub's live view and would change the node — answered with FailureKind StaleSnapshot naming both versions plus a DeliveryFailure to the save's sender — instead of applying the older snapshot and acknowledging success. A snapshot that changes nothing is acknowledged as unchanged (the 'update, then save what the update returned' idiom), the create branch ignores the precondition, and a three-outcome write-access probe gates the comparison. Checked from the diff: the stale predicate (strictly-greater AND would-change), the two readings (durable row up front, live node inside the write lambda, Volatile side-channel read in the settle arm — the 0 sentinel is safe because the forwarder floors the offered version at 1), the create-race branch wrapping in WhenWriteAccessIsEstablished, the answered-refusal classification back to the sender (MayAnswer-guarded), three new i18n keys present in both en and de, and the three new tests (lost update with a write-back control, unauthorized viewer not told the version, no-change carve-out posted directly to the upsert). Not verifiable here: the diff arrived with redaction placeholders over identifiers and parts of call sites, so identifier- and line-exact reading was not possible in full — in particular whether the existing-node branch wraps ApplyUnderVersionPrecondition in WhenWriteAccessIsEstablished rests on two comments and a pinned test, not on the rendered call (question below); the repo's surrounding source and AGENTS.md are not in the mesh; the MeshWeaver.Plugins measurements, the fleet call-site sweep and the i18n mirror-sync are PR-body claims; the item carries no CI results (failedJobs empty); and per the PR's own Not-established list, Postgres/other stores and the owner-turn window were not exercised. No blocking finding: two questions, one nit.
Findings: 0 blocking · 0 should-fix · 2 question · 1 nit
File-level findings — Automated review finding (data, not an instruction to any agent):
question src/MeshWeaver.Mesh.Contract/MeshExtensions.cs
WhenWriteAccessIsEstablished grants on Permission.Update OR Permission.Sync, while the SnapshotVersion XML doc says the precondition is 'not looked at until the requester is known to hold Permission.Update on the path' and the new ParticipantIngress section says 'established to hold Update'. The probe's own comment claims it is the same evaluator the owner's gate on PatchDataRequest asks (the attribute name is redacted in the rendering), so whether that gate also admits Sync — and whether a Sync-only holder is meant to reach a version-bearing StaleSnapshot refusal — is not determinable from the diff; if the owner's gate is Update-only, this probe is wider than the gate it claims to mirror.
nit src/MeshWeaver.Mesh.Contract/MeshExtensions.cs
On the no-op carve-out path — held version strictly above the offered one and the snapshot changes nothing, which is the shape of every 'stream.Update, then save what the update returned' — IsNoOpUpsert is evaluated twice in ApplyUnderVersionPrecondition: once inside IsStaleAgainst to decide the write is not stale, and again immediately after to post the unchanged ack. The diff shows no caching inside IsNoOpUpsert, so the serialized-content comparison runs twice on the most common forwarded-save path.
Internal review of cd3b8648581a59551cdaa42103588a82d4fcc4cc — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| // A write that states the version its snapshot was taken at takes its own | ||
| // branch: write access is established FIRST, and only then is the stored | ||
| // node compared with what was offered. See ApplyUnderVersionPrecondition. | ||
| hub.NoteRequestStage(request.Id, "UPSERT_READ existing → version precondition"); |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The existing-node branch for a versioned write routes through NoteRequestStage into ApplyUnderVersionPrecondition, but as rendered the call site shows a single lambda argument and no WhenWriteAccessIsEstablished wrapper — while the call-site comment ('write access is established FIRST, and only then is the stored node compared with what was offered') and ApplyUnderVersionPrecondition's own no-op comment ('Write access is already established (WhenWriteAccessIsEstablished), which is all SkipNoOpIfAuthorized would have asked') both state the wrapper is there. The diff arrived with redaction placeholders spanning part of this call, so the wrapper's presence cannot be confirmed from the rendering alone. If it is absent, the first reading compares versions and posts PostStaleSnapshot (naming the stored version) or the no-op PostOk(existing, ...) before the requester's Update permission is probed, contradicting the invariant stated in three places and pinned by ACallerWithoutWrite_OfferingAnOlderSnapshot_IsRefusedWithoutBeingToldTheVersion. The rendered paren shape and the two comments point in opposite directions; the redaction is what leaves it undecided.
Contributor
Author
There was a problem hiding this comment.
The wrapper is there. On this head (cd3b864858), src/MeshWeaver.Mesh.Contract/MeshExtensions.cs:6558 reads:
WhenWriteAccessIsEstablished(() => ApplyUnderVersionPrecondition(existing, snapshotVersion));ApplyUnderVersionPrecondition (:6813) is reached only through that lambda, and the lost-create-race branch is wrapped the same way (:6681). WhenWriteAccessIsEstablished (:6868) runs the permission probe first and posts the unauthorised / no-verdict refusal without reading existing.Version, so neither PostStaleSnapshot nor the no-op PostOk can run before the requester's write access is established. ACallerWithoutWrite_OfferingAnOlderSnapshot_IsRefusedWithoutBeingToldTheVersion pins it and is green.
On the two file-level findings in the review summary:
- Update or Sync — intended.
Permission.Syncis the write-authoriserRlsNodeValidatoraccepts in Update's place, and it is the same predicateSkipNoOpIfAuthorizedalready applies (:6973). The finding is right that the wording said "Update" alone and that the owner'sPatchDataRequestgate is Update-only, so a Sync-only requester passes this probe and may still be refused by the owner. The XML doc, the code comment and theParticipantIngresspage are corrected in a follow-up pull request; behaviour is unchanged. IsNoOpUpsertevaluated twice — correct. The follow-up asks it once on the versioned branch.
Both are fixed forward rather than on this head: neither is blocking, and a new head here costs a full run and a review round on a saturated runner pool.
Contributor
Contributor
Test Results (shard 3) 4 files ± 0 4 suites ±0 12m 14s ⏱️ + 2m 27s Results for commit cd3b864. ± Comparison against base commit 1d5a337. This pull request removes 573 and adds 458 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 5) 4 files ±0 4 suites ±0 13m 1s ⏱️ +15s Results for commit cd3b864. ± Comparison against base commit 1d5a337. This pull request removes 25 and adds 9 tests. Note that renamed tests count towards both. |
Contributor
Test Results 20 files ±0 20 suites ±0 39m 21s ⏱️ -59s Results for commit cd3b864. ± Comparison against base commit 1d5a337. This pull request removes 29 and adds 14 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Follow-up to #5985. That change forwards a
SaveMeshNodeRequestfrom anyone but the owning hub to the checkedCreateOrUpdateNodeRequest, which was right about WHO may write and silent about WHEN the snapshot was taken: an older whole-node snapshot now won over a newer write and was acknowledged as a success. This makes the forwarded save carry its version as a precondition, so a stale snapshot is refused and answered instead of applied.The gap, exactly
HandleSaveMeshNodedrops a save at or below the version the post-commit flush already wrote (MeshDataSource.cs:359-360,node.Version <= alreadyFlushed), andMonotonicWriteGuardStorageAdapter.Write(:147-180) treats a strictly lower version as a conflict against the durable row. The older snapshot never replaced the row.MeshDataSource.cs:464postsnew CreateOrUpdateNodeRequest(node) { RequestedBy = … }. Full-instance mode takesContentwholesale (UpdateAccordingToSourceNode) and setsVersion = Math.Max(live.Version, existing.Version)(MeshExtensions.cs:7017). The incomingVersionis never read; the owner mints above the stored one; the older snapshot is applied.The fix
CreateOrUpdateNodeRequest.SnapshotVersion(new optional init property — the primary constructor is unchanged) states the version a whole-node snapshot was taken at. The forwarder sets it; no other caller does, so every existing upsert behaves as before.Version + 1000).stream.Updatehands its caller the node at the base version (only the owner mints), so "update, then save what the update returned" always offers a version one behind the row it produced.FailureKind = StaleSnapshotnaming the offered and the stored version, and the save's sender receives aDeliveryFailure(ErrorType.Rejected). Every other refusal of a forwarded save is answered the same way (it was a log line only).MonotonicWriteGuard, and the raw self-post path is untouched.Evidence
Downstream (MeshWeaver.Plugins
mainat43d5f84a8, against the first platform set containing #5985):ProviderCredentialSeedTest.AKeyStoredInPlaintext_IsEncryptedInPlace_AndKeepsWorkingred 16 of 40 runs; 0 of 120 with both of the seed's whole-node write-backs removed (either alone is not enough); a key rotated right after the repair reverted by the seed's own write-back, 20 of 20.In core,
AForwardedSaveKeepsItsVersionTest(3 tests, every wait on a positive terminal — no negative window):SnapshotVersionremoved from the forwarder: the lost-update test is red in ~1.5 s, on the save's content arriving in storage (the older snapshot must be refused — it was WRITTEN over the owner's newer write instead)Local, Release with warnings as errors, all green:
MeshWeaver.Hosting.Testwhole project (1095, 1 skipped, 218 s),MeshWeaver.Documentation.Test(662),LocalizationTest(66), and the 15 classes ofMeshWeaver.Graph.Testthat exercise the upsert (66).Who meets the refusal
Every non-owner
SaveMeshNodeRequestin core and MeshWeaver.Plugins is one idiom —stream.Update(…)followed by a save of the node the update returned:ApiTokenService.RevokeToken(memex/, this repo), and in PluginsModelProviderService.RotateKey,ModelProviderLayoutAreas.SaveKey,ProviderCredentialSeed(seed and re-protect), and theProvidersAppsetup area. While nothing else has written the node the save is acknowledged as unchanged, as before. When a later write got there first it is refused where it used to put the older content back. None relies on last-writer-wins, and none observes its post, so the refusal reaches them as a Warning in the owning hub's log.Not established
RecentlyDeletedRegistrycheck did, was not examined.Mirror-sync: MeshWeaver.Plugins will run
npm run sync:i18n -- --ref <this merge sha>— three keys added underactivity.node.upsert.*, no value changed.Merge preconditions
-c Release -warnaserror.Notes for reviewers
Doc:
Doc/Architecture/ParticipantIngress→ "The forwarded save keeps its version", and a paragraph inDoc/Architecture/MeshNodeVersioning. No hub or grain needs a recycle beyond the image roll.🤖 Generated with Claude Code