Audit follow-ups: keyboard, wording, performance, transfer, sync, retry and relay fixes #22

Merged
jaap-jan merged 21 commits from claude/audit-followups into main 2026-10-02 12:59:56 +00:00
2 changed files with 75 additions and 1 deletions
Showing only changes of commit ff8a790f6c - Show all commits

No files matched your search

+15
View File
@@ -539,6 +539,17 @@ public sealed class SyncEngine
return resealed;
}
/// <summary>Whether a refused operation is a delete of an item the server never had.</summary>
/// <remarks>
/// 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.
/// </remarks>
private static bool DeletesSomethingNeverCreated(PendingOperation operation) =>
operation is { Operation: SyncOperation.Delete, ExpectedVersion: null };
/// <returns>Whether this answer warrants another push round.</returns>
private async Task<bool> 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,
@@ -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);
}
/// <remarks>
/// 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.
/// </remarks>
[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);
}
/// <remarks>
/// 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.
/// </remarks>
[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");
}
/// <remarks>
/// 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