Public Access
Stop asking the server to delete things it has never seen
Add a host on a laptop with no network, change your mind, delete it: the outbox holds a tombstone for a row the server has never heard of, the push answers Invalid, the change is parked, and the user is left looking at a rejected change for an item they already deleted and a pending count that will never reach zero. It applies to all four item types, because they all go through the one generic repository — the known-host path is only the likeliest way to meet it, since trust is pinned by connecting and withdrawn from the host editor. DeleteAsync now drops the queued create instead, when the server cannot be holding the item. A null expected version means the row is a create — including a create that has since been edited, because coalescing keeps the original expected version — so there is no server row and no mirror row, and dropping the queued change makes the item genuinely gone. The attempt count is what makes that safe rather than merely convenient. Nothing sent cannot have landed. A parked row cannot have landed either, because parking is what the pusher does when the server has refused, so the refusal is the evidence — and a parked create that the user then deletes could not be got rid of at all before this: the tombstone replacing it was parked in its turn. What is left is a create that went out and whose answer was never seen. That one still gets a tombstone, because the server may be holding the item and a local drop would strand it there for ever. A refused tombstone is recoverable; an orphan nobody can see and nobody can delete is not. Eight tests, and the interesting half is the other direction. A repository that quietly dropped tombstones would pass a suite written only around the bug and would lose data on every machine but the one that pressed the button. Which is not hypothetical, because the mutation pass found exactly that hole in the first draft of these tests. Removing the expected-version guard left every test passing: after a sync there is no queued row at all, so deleting a synced item never reaches the shortcut and proves nothing about it. The way to hold an unpushed Upsert over an item the server holds is to edit it offline, and EditingASyncedItemOfflineAndThenDeletingIt_StillQueuesATombstone is the test that was missing. Without the guard it deletes the item here, leaves it on the server, and the next pull brings it back. Three mutations, all caught now: removing the shortcut (5 tests), removing the expected-version guard (1), removing the attempt-count guard (1). The Upsert check itself is conservative rather than load-bearing — a queued Delete with no expected version is not reachable from the interface, and completing one locally would discard a tombstone that might be needed, so it stays and is not independently covered. 106 tests green in Client.Sync, 8 of them new. Zero warnings, format clean.
This commit is contained in:
@@ -210,8 +210,14 @@ internal sealed class VaultItemRepository<TSecret>(
|
||||
/// Deletes an item.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// Queued as a tombstone, never a local removal. An offline client that simply forgot the row would
|
||||
/// be unable to tell the server anything, and the item would come back on the next pull.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Unless the server has never heard of the item, which is the one case where a tombstone is not only
|
||||
/// unnecessary but wrong — see <see cref="NeverReachedTheServer" />.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
internal async Task DeleteAsync(Guid vaultId, Guid entityId, CancellationToken cancellationToken)
|
||||
{
|
||||
@@ -219,6 +225,12 @@ internal sealed class VaultItemRepository<TSecret>(
|
||||
.FindAsync(vaultId, kind.EntityType, entityId, cancellationToken)
|
||||
.ConfigureAwait(false);
|
||||
|
||||
if (pending is not null && NeverReachedTheServer(pending))
|
||||
{
|
||||
await outbox.CompleteAsync(pending.Sequence, cancellationToken).ConfigureAwait(false);
|
||||
return;
|
||||
}
|
||||
|
||||
var expectedVersion = pending is not null
|
||||
? pending.ExpectedVersion
|
||||
: await MirrorVersionAsync(vaultId, entityId, cancellationToken).ConfigureAwait(false);
|
||||
@@ -239,6 +251,32 @@ internal sealed class VaultItemRepository<TSecret>(
|
||||
cancellationToken).ConfigureAwait(false);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Whether a queued change describes an item the server cannot be holding.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// A null <c>ExpectedVersion</c> means the row is a create — including a create that has since been
|
||||
/// edited, because coalescing keeps the original expected version. So there is no server row and no
|
||||
/// mirror row, and dropping the queued change makes the item genuinely gone. Queueing a tombstone
|
||||
/// instead asks the server to delete something it has never seen, which it answers <c>Invalid</c>; the
|
||||
/// change is parked, and the user is left with a rejected item they already deleted and a pending count
|
||||
/// that never reaches zero. Add a host on a laptop with no network, change your mind, and that is the
|
||||
/// state — it applies to all four item types.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The attempt count is what makes this safe rather than merely convenient. Nothing sent cannot have
|
||||
/// landed. A parked row cannot have landed either — parking is what the pusher does when the server has
|
||||
/// refused, so the refusal is the evidence. What is left is a create that went out and whose answer was
|
||||
/// never seen: in flight, or failed in a way that might yet have been applied. That one still gets a
|
||||
/// tombstone, because the server may be holding the item and a local drop would strand it there for
|
||||
/// ever. A refused tombstone is recoverable; an orphan on the server is not.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
private static bool NeverReachedTheServer(PendingOperation pending) =>
|
||||
pending is { Operation: SyncOperation.Upsert, ExpectedVersion: null }
|
||||
&& (pending.Attempts == 0 || pending.IsParked);
|
||||
|
||||
private static void Validate(TSecret secret)
|
||||
{
|
||||
if (!secret.TryValidate(out var error))
|
||||
|
||||
Reference in New Issue
Block a user