GitSync: a server-side addition was silently deleted one sync cycle later by rbuergi · Pull Request #675 · Systemorph/MeshWeaver

rbuergi · 2026-07-27T18:49:27Z

TwoWaySyncTest.TwoWay_Update_KeepsServerAddition_UnlessForced was on the flake list. It is not a flake — GitSync silently deletes a server-side addition one sync cycle later.

Mechanism

Two-way sync protects a server-side change two ways: it refuses to overwrite it (upsert conflict) and it refuses to prune it (the repo has no copy — a server-side addition). Only the first path incremented StaticRepoImportResult.Preserved; the prune path filtered the node out of toPrune and reported nothing.

GitHubSyncService.ReimportAtCommit uses exactly that number to decide whether to advance the sync baseline LastSyncedAt, and its comment states the invariant: a preserving import leaves the mesh ahead of the repo, so advancing the baseline would move it past the very node two-way just protected. A kept-not-pruned node reported Preserved = 0, so the baseline advanced past it and the protection (node.LastModified > LastSyncedAt) silently disarmed. The next import that actually runs prunes the node.

Why it looked like a flake: the test usually passes only because the second update short-circuits on the unchanged content fingerprint (Skipped, so prune never runs). That short-circuit reads an eventually-consistent query — the importer itself documents a stale miss as "wasteful, not wrong". Under CI load the read misses, the second update really imports, prune runs against the advanced baseline, and the node is gone (run 30250682456, shard 5, WaitForNode timeout at TwoWaySyncTest.cs:51). A cache-ish short-circuit was concealing a data-loss bug.

Evidence

Pre-fix, instrumented, the failing test's own sequence:

baseline=…30.4931640  serverOnly.LastModified=…30.5879100   ← protected (T1 > T0)
update1 outcome=Imported preserved=0 baseline=…30.8564380   ← baseline jumped PAST T1
update2 outcome=Skipped  → ServerOnly survives (passes by luck)

With the short-circuit disabled (activity marker dropped — i.e. the CI query miss), pre-fix: update2 outcome=Imported → ServerOnly absent = True, reproducing the CI failure exactly. Post-fix, 3/3: update1 preserved=1, baseline unchanged; update2 preserved=2; ServerOnly absent = False.

New test TwoWay_ServerAddition_SurvivesEveryLaterRepoPush makes every update import a genuinely new commit so prune runs for real each time, and asserts the invariant (Preserved > 0, LastSyncedAt unchanged) rather than only the symptom. Unfixed it fails 3/3 in ~0.7 s; fixed it passes.

GitSync.Test 142/142 ×3, Graph.Test 633/633, Content.Test 249 + 1 skipped, full solution -c Release -p:CIRun=true -warnaserror clean.

What this does not establish: the original test passes locally both before and after — locally the short-circuit always engages, so it never reproduced untouched. Green runs of it prove nothing; the load-bearing evidence is the deterministic new test plus the marker-drop emulation.

⚠️ Behaviour change to weigh before merging

Freezing the baseline does refuse some later repo edits. Measured: with a server addition pending, a repo edit to Welcome applies (v2), but a second edit (v3) is preserved instead of applied — because the import itself re-stamped Welcome.LastModified past the frozen horizon, so it looks "server-newer" though no user touched it.

This false-conflict is inherent to the existing horizon design (it already occurred for upsert-preserved nodes); this change makes it reachable more often. It is not data loss — v3 stays in git and the import activity says ↩ Kept … — but a subsequent commit-back mirrors mesh state and would revert v3 in the repo, visible in the diff.

Clean follow-up: record per-node import write-times in the manifest so "server-newer" means "changed since the import wrote it" rather than "changed since a global timestamp".

Found, not fixed — same defect, second live route

The fingerprint short-circuit returns Preserved = 0, so a no-op "Update to latest" records last sync and advances the horizon past pending uncommitted changes although it verified nothing (pre-fix probe: update2 outcome=Skipped, LastSyncedAt moved …30.8564380 → …30.8765430). The git-diff scope skip has the same shape for a server-edited node whose repo file didn't change. Both can still lose a server addition on a later push.

The honest fix is to split the two fields — LastSyncCommitSha is safe to advance, LastSyncedAt (the conflict horizon) is not — but that changes the meaning of a persisted field and affects "are we up to date?" (a repo commit touching no node files would otherwise leave the space permanently "behind"). Left as a maintainer decision rather than bundled into this fix.

No What's New entry — silent data-loss fix; no user-visible behaviour change when sync is working.

🤖 Generated with Claude Code

…n cannot skip past them Two-way sync protects a server-side change two ways: it refuses to OVERWRITE it (upsert conflict) and it refuses to PRUNE it (the repo has no copy — a server-side ADDITION). Only the first incremented StaticRepoImportResult.Preserved. The prune path filtered the node out of toPrune and reported nothing. GitHubSyncService.ReimportAtCommit uses exactly that number to decide whether to advance the sync baseline LastSyncedAt, with a comment stating the invariant: a preserving import leaves the mesh ahead of the repo, so advancing the baseline would move it past the very node two-way just protected. Because a kept-not-pruned node reported Preserved = 0, the baseline advanced past it and the protection (node.LastModified > LastSyncedAt) silently disarmed. The NEXT import that actually runs prunes the node — the user's content is deleted. This surfaced as a CI flake (TwoWaySyncTest.TwoWay_Update_KeepsServerAddition_ UnlessForced, run 30250682456 shard 5, WaitForNode timeout at :51). The test usually passes only because the second update short-circuits on the unchanged content fingerprint (Skipped, so no prune runs). That short-circuit reads an eventually consistent query — the importer itself documents a stale miss as "wasteful, not wrong". Under CI load the read misses, the second update really imports, prune runs against the advanced baseline, and the node is gone. So the flake is a real data-loss bug whose usual concealment is a cache-ish short-circuit. Fix: kept-not-pruned nodes count as Preserved and get a "↩ Kept …" line in the import activity, exactly like kept-not-overwritten ones. Only GitSync constructs a two-way policy and Preserved has exactly one consumer, so git-first, boot and InstanceSync imports are bit-for-bit unchanged. Evidence. Pre-fix, instrumented, the failing test's own sequence: baseline=…30.4931640 serverOnly.LastModified=…30.5879100 protected (T1 > T0) update1 outcome=Imported preserved=0 baseline=…30.8564380 baseline jumped PAST T1 update2 outcome=Skipped -> ServerOnly survives (passes by luck) With the short-circuit disabled (activity marker dropped = the CI query miss), pre-fix: update2 outcome=Imported -> ServerOnly absent = True, i.e. the CI failure reproduced exactly. Post-fix 3/3: update1 preserved=1 with the baseline unchanged, update2 preserved=2, ServerOnly absent = False. New test TwoWay_ServerAddition_SurvivesEveryLaterRepoPush makes every update import a genuinely new commit so prune runs for real each time, and asserts the invariant directly (Preserved > 0, LastSyncedAt unchanged) rather than only the symptom. Unfixed it fails 3/3 in ~0.7s; fixed it passes. GitSync.Test 142/142 ×3, Graph.Test 633/633, Content.Test 249+1 skipped, full solution Release/-warnaserror clean. What this does NOT establish: the original test passes locally both before and after — locally the short-circuit always engages, so it never reproduced untouched. Green runs of it prove nothing; the load-bearing evidence is the deterministic new test plus the marker-drop emulation. Behaviour change to weigh: freezing the baseline does refuse some later repo edits. Measured — with a server addition pending, a repo edit to Welcome applies (v2), but a second edit (v3) is preserved instead of applied, because the import itself re-stamped Welcome.LastModified past the frozen horizon so it looks "server-newer" though no user touched it. This false-conflict is inherent to the existing horizon design (it already occurred for upsert-preserved nodes); this change makes it reachable more often. It is not data loss — v3 stays in git and the activity says "↩ Kept …" — but a subsequent commit-back mirrors mesh state and would revert v3 in the repo, visible in the diff. Clean follow-up: record per-node import write-times so "server-newer" means "changed since the import wrote it" rather than "changed since a global timestamp". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Fixes a two-way GitSync data-loss scenario where a server-side addition could be silently deleted on a later sync cycle because the prune-side “kept” path wasn’t counted as Preserved, allowing the sync baseline (LastSyncedAt) to advance past the protected node.

Changes:

Count “kept-not-pruned” nodes as Preserved in StaticRepoImporter and include them in the import summary/result.
Add a deterministic regression test that forces real imports across new commits to ensure prune executes and the baseline invariant holds.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
test/MeshWeaver.GitSync.Test/TwoWaySyncTest.cs	Adds a deterministic two-way sync regression test covering server-side additions across multiple repo commits.
src/MeshWeaver.Graph/StaticRepoImporter.cs	Counts prune-side preserved nodes in `Preserved` and surfaces them in activity/log summary and the import result.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

                    if (policy is { PreserveServerNewer: true, Force: false, Since: { } pruneSince })
+                    {
+                        keptFromPrune = toPrune.Where(n => n.LastModified > pruneSince).ToArray();
                        toPrune = toPrune.Where(n => n.LastModified <= pruneSince).ToArray();


+                            logger?.LogInformation(
+                                "[StaticRepoImport] {Partition}: two-way — keeping server-added {Path} (not pruned).",
+                                source.Partition, kept.Path);
+                            NodeTypeCompilationActivity.AppendLog(hub, activityPath,
+                                $"↩ Kept {kept.Path} (added on the server — commit to sync it back).", logger!);
+                        }


github-actions · 2026-07-27T19:04:57Z