From ff8a790f6c89a64b69af203317652839d063b6a2 Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Fri, 2 Oct 2026 14:18:28 +0200 Subject: [PATCH] Sync: take the server having no such item as a delete done, not a rejection nobody can clear --- src/DodoSSH.Client.Sync/SyncEngine.cs | 15 +++++ .../UnpushedDeleteTests.cs | 61 ++++++++++++++++++- 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/src/DodoSSH.Client.Sync/SyncEngine.cs b/src/DodoSSH.Client.Sync/SyncEngine.cs index 3e162f5..7283f4e 100644 --- a/src/DodoSSH.Client.Sync/SyncEngine.cs +++ b/src/DodoSSH.Client.Sync/SyncEngine.cs @@ -539,6 +539,17 @@ public sealed class SyncEngine return resealed; } + /// Whether a refused operation is a delete of an item the server never had. + /// + /// A delete queued over a create that never reached the server carries no expected version, and the + /// server's only reason to refuse one of those is that it has no such item — which is the state the delete + /// asked for, so it is done. It used to be parked as a rejection: a "conflict" about an item nobody else + /// had ever seen, which the user could neither resolve nor dismiss. A create that did land is no exception + /// to this — the server has the item then, and applies the delete. + /// + private static bool DeletesSomethingNeverCreated(PendingOperation operation) => + operation is { Operation: SyncOperation.Delete, ExpectedVersion: null }; + /// Whether this answer warrants another push round. private async Task HandleAsync( Guid vaultId, @@ -581,6 +592,10 @@ public sealed class SyncEngine cancellationToken).ConfigureAwait(false); return false; + case SyncOperationStatus.Invalid when DeletesSomethingNeverCreated(operation): + await outbox.CompleteAsync(operation, cancellationToken).ConfigureAwait(false); + return false; + case SyncOperationStatus.Invalid: await RejectAsync( vaultId, diff --git a/tests/DodoSSH.Client.Sync.Tests/UnpushedDeleteTests.cs b/tests/DodoSSH.Client.Sync.Tests/UnpushedDeleteTests.cs index 6c06e7b..6d8f7b2 100644 --- a/tests/DodoSSH.Client.Sync.Tests/UnpushedDeleteTests.cs +++ b/tests/DodoSSH.Client.Sync.Tests/UnpushedDeleteTests.cs @@ -147,7 +147,8 @@ public sealed class UnpushedDeleteTests // The ambiguous case, and the reason the attempt count is part of the test rather than just the // expected version. A create that was dispatched may have been applied by a server whose answer never // arrived. Dropping it locally would strand a row nobody can see and nobody can delete; a tombstone - // that turns out to be unnecessary is refused once and can be dealt with. + // that turns out to be unnecessary is refused once, and the refusal is taken as the delete being done — + // see the two tests after this one. using var harness = await CreateAsync(); var laptop = harness.First; @@ -166,6 +167,64 @@ public sealed class UnpushedDeleteTests tombstone.Operation.ShouldBe(SyncOperation.Delete); } + /// + /// The rest of that story, when the create never did land. The tombstone carries no expected version, and + /// the server refuses it because it has no such item — which is what the delete wanted. It used to be + /// parked as a rejection the user could neither resolve nor dismiss, with a pending count stuck above zero. + /// + [Fact] + public async Task ATombstoneForACreateThatNeverLanded_IsDoneWhenTheServerSaysItHasNoSuchItem() + { + using var harness = await CreateAsync(); + var laptop = harness.First; + + var entityId = await laptop.CreateAsync(Host("in-flight")); + var queued = (await laptop.Outbox.ListAllAsync(VaultId, Token)).ShouldHaveSingleItem(); + await laptop.Outbox.MarkDispatchedAsync(queued.Sequence, Token); + + await laptop.DeleteAsync(entityId); + + var report = await laptop.SyncAsync(); + + report.Parked.ShouldBe(0, "the server not having the item is the delete succeeding"); + (await laptop.Outbox.ListAllAsync(VaultId, Token)).ShouldBeEmpty(); + harness.Server.RowCount.ShouldBe(0); + } + + /// + /// And when it did land, the shortcut above must not be what answers. The pull brings the item back first, + /// at a version the tombstone never knew about, and the engine's ordinary rule for that applies: the delete + /// gives way, the item stays, and the user is told — nothing parked, nothing lost, nothing stuck in the + /// outbox. Deleting it again is then an ordinary delete of an item the server holds. + /// + [Fact] + public async Task ATombstoneForACreateThatDidLand_GivesWayToIt_RatherThanBeingTakenAsDone() + { + using var harness = await CreateAsync(); + var laptop = harness.First; + + var entityId = await laptop.CreateAsync(Host("in-flight")); + + // The create reaches the server with its answer lost: the server holds the item while the laptop's own + // row still says "sent, unanswered". + var queued = (await laptop.Outbox.ListAllAsync(VaultId, Token)).ShouldHaveSingleItem(); + await laptop.Outbox.MarkDispatchedAsync(queued.Sequence, Token); + harness.Server.ExternalUpsert(entityId, queued.Payload.ShouldNotBeNull(), queued.Fields); + + await laptop.DeleteAsync(entityId); + var report = await laptop.SyncAsync(); + + report.DeletesAbandoned.ShouldBe(1, "the user is told the item came back"); + report.Parked.ShouldBe(0); + (await laptop.Outbox.ListAllAsync(VaultId, Token)).ShouldBeEmpty(); + harness.Server.RowCount.ShouldBe(1); + + await laptop.DeleteAsync(entityId); + await laptop.SyncAsync(); + + harness.Server.RowCount.ShouldBe(0, "deleting it again reaches the server"); + } + /// /// The path most likely to meet this in practice, which is why it gets its own test rather than trusting /// the generic repository: trust is pinned by connecting and withdrawn from the host editor, so trusting a