From 1a180073733aa1fcc73ab9f4e4e03fff5f8d4daa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20B=C3=BCrgi?= Date: Mon, 27 Jul 2026 19:48:58 +0100 Subject: [PATCH] GitSync: count kept-not-pruned nodes as Preserved, so the sync horizon cannot skip past them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/MeshWeaver.Graph/StaticRepoImporter.cs | 33 ++++++++-- .../MeshWeaver.GitSync.Test/TwoWaySyncTest.cs | 64 +++++++++++++++++++ 2 files changed, 92 insertions(+), 5 deletions(-) diff --git a/src/MeshWeaver.Graph/StaticRepoImporter.cs b/src/MeshWeaver.Graph/StaticRepoImporter.cs index 5e455b5c2c..5e8ce66223 100644 --- a/src/MeshWeaver.Graph/StaticRepoImporter.cs +++ b/src/MeshWeaver.Graph/StaticRepoImporter.cs @@ -17,8 +17,10 @@ namespace MeshWeaver.Graph; /// Outcome of a run. is /// the number of live nodes a two-way import kept because they were newer on the server (see -/// ) — non-zero means the mesh is now AHEAD of the repo, so the -/// caller must NOT advance its last-sync baseline until those edits are committed back. +/// ) — BOTH kinds: kept-not-overwritten (the repo's copy differs) and +/// kept-not-pruned (the repo has no copy at all — a server-side addition). Non-zero means the mesh is +/// now AHEAD of the repo, so the caller must NOT advance its last-sync baseline until those changes are +/// committed back. public sealed record StaticRepoImportResult(string Partition, string Fingerprint, string Outcome, int Count = 0, int Preserved = 0); /// @@ -858,8 +860,27 @@ bool IsClaimed(string path, MeshNode? target) => // Two-way: never prune a node CREATED/changed on the server since the last sync. It // isn't in the repo yet, but it's a local addition to be committed back — not a stale // extra to remove. A forced import prunes it (the repo state wins). + // 🚨 A node KEPT here counts as PRESERVED exactly like a kept upsert conflict — it is + // the same statement: "this local change is not in the repo yet, so the mesh is AHEAD". + // Reporting 0 here let GitHubSyncService.ReimportAtCommit advance the sync baseline + // (LastSyncedAt) past the node it had just protected, so on the NEXT import the node was + // no longer "newer on the server" and got PRUNED — the server addition silently deleted + // one cycle later, which is precisely what that baseline guard exists to prevent. + // Deterministic repro: TwoWaySyncTest.TwoWay_ServerAddition_SurvivesEveryLaterRepoPush. + var keptFromPrune = Array.Empty(); 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(); + foreach (var kept in keptFromPrune) + { + 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!); + } + } var pruned = toPrune.Count == 0 ? Observable.Return(0) @@ -900,8 +921,10 @@ bool IsClaimed(string path, MeshNode? target) => // full diagnostic log (no torn "Succeeded but the ⚠ lines didn't land yet"). var failed = count.Failed; var status = failed > 0 ? ActivityStatus.Warning : ActivityStatus.Succeeded; - // Two-way preserved local edits (kept, not overwritten) noted only when any. - var preservedNote = count.Preserved > 0 ? $", kept {count.Preserved} local edit(s)" : ""; + // Two-way preserved local changes: kept-not-overwritten (upsert conflicts) PLUS + // kept-not-pruned (server additions). Both leave the mesh ahead of the repo. + var preserved = count.Preserved + keptFromPrune.Length; + var preservedNote = preserved > 0 ? $", kept {preserved} local change(s)" : ""; var summary = failed > 0 ? $"Imported {count.Imported} node(s), {failed} FAILED (see ⚠ above){preservedNote}, pruned {prunedCount}, synced {contentCount} content file(s)." : $"Imported {count.Imported} node(s){preservedNote}, pruned {prunedCount}, synced {contentCount} content file(s)."; @@ -918,7 +941,7 @@ bool IsClaimed(string path, MeshNode? target) => "[StaticRepoImport] {Partition}: imported {Count}, failed {Failed}, pruned {Pruned}, content {Content} at {Fingerprint}.", source.Partition, count.Imported, failed, prunedCount, contentCount, fingerprint); return new StaticRepoImportResult(source.Partition, fingerprint, - failed > 0 ? "ImportedWithErrors" : "Imported", count.Imported, count.Preserved); + failed > 0 ? "ImportedWithErrors" : "Imported", count.Imported, preserved); }); }))); }); diff --git a/test/MeshWeaver.GitSync.Test/TwoWaySyncTest.cs b/test/MeshWeaver.GitSync.Test/TwoWaySyncTest.cs index 4df6e655ad..ca77528078 100644 --- a/test/MeshWeaver.GitSync.Test/TwoWaySyncTest.cs +++ b/test/MeshWeaver.GitSync.Test/TwoWaySyncTest.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Immutable; using System.Reactive.Linq; using System.Reactive.Threading.Tasks; using MeshWeaver.GitSync; @@ -55,4 +56,67 @@ await Sync.SaveConfig(a, repo, "main", subdirectory: null, Assert.True(await IsAbsent($"{a}/ServerOnly")); Assert.NotNull(await WaitForNode($"{a}/Welcome")); } + + /// + /// The same baseline invariant under REAL repo traffic — the deterministic form of the guard + /// above (which relies on the second update short-circuiting on an unchanged fingerprint). + /// Every update here imports a genuinely new commit, so prune runs for real each time: a node + /// added on the server must survive update after update until it is committed back, because + /// PRESERVING it (here: by not pruning it) leaves the mesh AHEAD of the repo and must NOT + /// advance the sync baseline. Advancing it loses the node on the NEXT push — silent data loss. + /// + [Fact(Timeout = 120000)] + public async Task TwoWay_ServerAddition_SurvivesEveryLaterRepoPush() + { + await Connect(); + var a = "GhY" + Guid.NewGuid().ToString("N")[..8]; + await CreateSpace(a, "Space Y"); + await CreateMarkdown($"{a}/Welcome", "Welcome", "# Welcome\n\nv1."); + + var repo = "https://github.com/test/space-y"; + await Sync.SaveConfig(a, repo, "main", subdirectory: null, + createBranchIfMissing: true, createRepoIfMissing: true, + direction: SyncDirection.Bidirectional, sourceId: null, twoWay: true) + .Timeout(30.Seconds()).ToTask(); + + var c1 = await Sync.SyncToGitHub(a, UserId).Timeout(60.Seconds()).ToTask(); + var baseline = await WaitForConfig(a, c => c.LastSyncedAt != null && c.LastSyncCommitSha == c1.CommitSha); + + // A user adds a node on the SERVER after that baseline — not in the repo at any commit. + await CreateMarkdown($"{a}/ServerOnly", "ServerOnly", "# ServerOnly\n\nadded on the server."); + + // Someone commits to the repo; the update imports it and KEEPS the server addition. + await PushRepoFile(repo, "Docs.md", "# Docs\n\nfrom the repo."); + var update1 = await Sync.ReimportAtCommit(a, "main", UserId).Timeout(90.Seconds()).ToTask(); + Assert.NotNull(await WaitForNode($"{a}/Docs")); + Assert.NotNull(await WaitForNode($"{a}/ServerOnly")); + + // The invariant the survival above RESTS on, asserted directly: keeping the server addition + // (by not pruning it) is a PRESERVE, and a preserving import leaves the mesh ahead of the + // repo — so the sync baseline must NOT advance past the node it just protected. Reporting + // Preserved=0 here is what silently disarmed the protection and deleted the node one update + // later; the assertions below would then fail. + Assert.True(update1.Preserved > 0, $"kept-not-pruned must count as preserved (was {update1.Preserved})"); + var afterUpdate1 = await Sync.ReadConfig(a).Timeout(30.Seconds()).ToTask(); + Assert.Equal(baseline.LastSyncedAt, afterUpdate1!.LastSyncedAt); + + // A SECOND repo commit must not delete it either — the baseline may not have moved past it. + await PushRepoFile(repo, "Docs2.md", "# Docs2\n\nfrom the repo."); + await Sync.ReimportAtCommit(a, "main", UserId).Timeout(90.Seconds()).ToTask(); + Assert.NotNull(await WaitForNode($"{a}/Docs2")); + Assert.NotNull(await WaitForNode($"{a}/ServerOnly")); + } + + /// Simulates a repo-side commit: the current tree plus one added file. + private Task PushRepoFile(string repo, string path, string content) => + Fake.Push(new GitHubPushRequest + { + RepositoryUrl = repo, + Branch = "main", + Files = Fake.Tree(repo).Append(new RepoFile(path, content)).ToImmutableList(), + CommitMessage = $"add {path}", + AuthorName = "octocat", + AuthorEmail = "octocat@users.noreply.github.com", + AccessToken = "ghp_test_token", + }).Timeout(30.Seconds()).ToTask(); }