fix(#2463): a PARTIAL merge refusal is still a refusal — stop acking it Success by rbuergi · Pull Request #2840 · Systemorph/MeshWeaver

rbuergi · 2026-08-30T20:42:24Z

The defect

ApplyMeshNodeMerge returns how many keys the owner's three-way merge refused. Both callers act on it only inside this branch:

if (JsonNode.DeepEquals(preMergeNode, currentNode))   // ← only when NOTHING landed
{
    if (refusedKeys > 0) AckOnce(false, Conflict);
    else                 AckOnce(true);
    return;
}

A patch where some fields applied and others were refused never reaches it — the node changed, so DeepEquals is false — and falls straight through to the success path.

So the caller is told its whole write landed when half of it is on the floor. That is the #648 acked-write-loss in its partial form, and it is the harder one to see: the node really did change, so every "did it commit?" question answers yes. The existing pin covers only the all-refused case precisely because that one leaves the node byte-identical.

It has a name in the wild

#2463, verbatim from the run: RolePlay/Scenery compiled fine (ok … 4 source(s)), the mesh phase patched four compile-outcome fields, MergeGuard refused all four as stale/reordered while another field in the same patch landed — so this path acked Success, IsDirty never converged, and the gate read a status that had never been written and called it a compile failure.

A false RED on the required check, produced by a write that reported itself applied.

The fix, and the arguable part

NACK Conflict on any refusal, and let the commit proceed. AckOnce latches on Interlocked.Exchange, so the caller learns either way.

Letting the commit stand is deliberate: the fields that did not conflict were written against a current base and are valid. Rolling them back would convert a reportable conflict into real data loss. The caller re-reads and re-applies; what already landed re-diffs to a no-op, and Conflict is one of the three provably-safe retried codes with a bounded budget, so this cannot spin.

The test pins that half explicitly — Description must still read "landed" — so a future change cannot satisfy the guard by discarding the valid fields.

Mutation-proven, both directions

With the fix reverted behind a runtime-false condition the compiler cannot fold (&& Environment.TickCount < 0), so --no-build cannot run stale binaries:

PartiallyRefusedThreeWayMerge_IsNackedConflict_AndKeepsWhatDidNotConflict [FAIL]
   Expected value to be false because part of the caller's write was refused …
   but found True

The owner really does ack Success = true. Restored: green.

Verification

MeshWeaver.Hosting.Test 269/269 (whole project — it owns ConflictRetryPolicyTest, the caller-side path this change routes more traffic into).
Conflict-path suites SupersededActivationPatchNack + StoreLevelWriteConflict + ActivityWriteSurvivesConflict: 5/5.
Release -warnaserror clean.

🚨 No timing claim

A peer rightly asked for a cost measurement, having had two fixes refuted by wall-clock tonight. I am not offering one, because the honest answer is that it would measure noise: the change adds one integer comparison to the common path — refusedKeys is already computed, and the extra ack only fires when it is non-zero. There is no new work on any path that was previously fast. The structural argument is the cost argument here, and I would rather say that than dress up a contended single-sided run as evidence.

Fixes #2463

…it Success `ApplyMeshNodeMerge` returns how many keys the three-way merge refused, and both callers NACK Conflict only when `DeepEquals(preMergeNode, currentNode)` — i.e. only when NOTHING landed. A patch where some fields applied and others were refused falls straight through to the success path. So the caller is told its whole write landed when half of it is on the floor. That is the #648 acked-write-loss in its partial form, and it is the harder one to see: the node really did change, so every "did it commit?" question answers yes. The existing pin only covers the all-refused case precisely because that one leaves the node byte-identical. It has a name in the wild. RolePlay/Scenery compiled fine; the mesh phase patched four compile-outcome fields; MergeGuard refused all four as stale/reordered while another field in the same patch landed; this path acked Success; `IsDirty` never converged; and the gate read a status that had never been written and called it a compile failure — a false RED on the required check, from a write that reported itself applied. The fix NACKs Conflict on any refusal and LETS THE COMMIT PROCEED. AckOnce latches, so the caller learns either way. The fields that did not conflict were written against a current base and are valid — rolling them back would convert a reportable conflict into real data loss. The caller re-reads and re-applies; what already landed re-diffs to a no-op, and Conflict is one of the provably-safe retried codes with a bounded budget, so this cannot spin. Mutation-proven both directions. With the fix reverted behind a runtime-false condition the compiler cannot fold: PartiallyRefusedThreeWayMerge_IsNackedConflict_AndKeepsWhatDidNotConflict [FAIL] Expected value to be false … but found True Restored: green. The test also pins that Description ("landed") survives, so a future fix cannot satisfy the guard by discarding the valid half. Verified: MeshWeaver.Hosting.Test 269/269, the conflict-path suites (SupersededActivationPatchNack, StoreLevelWriteConflict, ActivityWriteSurvivesConflict) 5/5, Release -warnaserror clean. 🚨 No timing claim. The change adds one integer comparison to the common path — `refusedKeys` is already computed and the extra ack only fires when it is non-zero — so a wall-clock A/B would measure noise for a structurally null result. That reasoning, not a measurement, is the cost argument here. Fixes #2463 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014FhP7Jg65q8cAATnFfeaTK

Copilot

Pull request overview

This PR fixes owner-side ACK semantics for cross-hub MeshNode three-way merges so that any refused keys (including partial refusals) are NACKed as Conflict rather than incorrectly ACKed as Success, preventing “acked-success write loss” where callers stop retrying refused fields.

Changes:

NACK Conflict when a three-way merge refuses any fields (even if other fields applied), instead of only when the merge resulted in no net change.
Add a deterministic regression test that asserts (a) partial refusal returns Conflict, and (b) non-conflicting fields still persist.

Reviewed changes

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

File	Description
`src/MeshWeaver.Data/DataExtensions.cs`	Adjusts PatchDataRequest handling to NACK `Conflict` on partial merge refusal in both relevant code paths.
`test/MeshWeaver.Hosting.Monolith.Test/SupersededActivationPatchNackTest.cs`	Adds a new pin test covering the partial-refusal scenario and asserting “kept non-conflicting fields” behavior.

Suppressed comments (1)

src/MeshWeaver.Data/DataExtensions.cs:1689

The Conflict response text asserts that non-conflicting fields were kept, but this AckOnce is emitted before the commit/flush completes; if a later apply/flush error occurs, that claim can be inaccurate. Consider wording that doesn’t imply successful persistence.

                            $"cross-hub write PARTIALLY refused: {refusedKeys} field(s) changed on "
                            + "the owner since the writer's base. What did not conflict was kept — "
                            + "re-read and re-apply so the refused field(s) converge."));

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+                            $"cross-hub write PARTIALLY refused: {refusedKeys} field(s) changed on "
+                            + "the owner since the writer's base. What did not conflict was kept — "
+                            + "re-read and re-apply so the refused field(s) converge."));


github-actions · 2026-08-30T20:48:42Z