fix(localdb): preserve the poison cause when dead-lettering itself fails; pin self-referencing cascade (0.3.3 review follow-ups)

Three review follow-ups on 122940c. Still 0.3.3 — unpublished, so no further version bump. No behaviour change to any succeeding path. - The per-entry poison handler's recovery work (ROLLBACK TO + the dead-letter INSERT) was unguarded: a wedged connection, an I/O error, or a dead-letter INSERT that faults would propagate the SECOND exception and silently discard the FIRST — the one that says why the entry was poison at all. The pair is now wrapped; a recovery failure throws an AggregateException whose message names the entry (table + seq) and the recovery fault, and whose InnerException is the ORIGINAL entry fault, with the recovery fault carried alongside it at InnerExceptions[1] with its own stack. The outcome is unchanged and still fail-closed: nothing commits, and disposing the batch transaction unwinds it. OperationCanceledException is deliberately excluded from the wrap so a shutdown still surfaces as a cancellation rather than as a fault. No upstream caller catches SqliteException from the apply path (checked), so widening the escaping type is safe. Injected cheaply after all: a RAISE(ABORT) trigger on __localdb_dead_letter, stacked on the existing row-version poison trigger, produces exactly the two-fault case with no production test seam. The test asserts both causes are carried AND that the batch committed nothing — the entry that applied cleanly before the poison one is gone and the peer watermark never moved. - Register_SelfReferencingCascade_Throws pins the README's "or from itself" claim: a table that is its own cascading parent (parent_id REFERENCES nodes(id) ON DELETE CASCADE) is refused. The guard needed no special case — it reads every foreign key in the database — but the promise is now tested. - Comment on why EnterEntryAsync sits outside the try: a SAVEPOINT that failed opened nothing, so a handler ROLLBACK TO would name a savepoint that does not exist and fault in turn. Both new tests were run against 122940c first and fail there: the self-reference case throws nothing, and the stacked-fault case escapes as a bare SqliteException("dead-letter write failed") with the original cause gone. Suite: 215/215 green, build 0 warnings / 0 errors.
2026-08-20 08:49:11 -04:00
parent 122940c0e4
commit 9663279594
3 changed files with 76 additions and 2 deletions
@@ -98,6 +98,9 @@ internal sealed class LwwApplier(SqliteLocalDb db, Func<DateTimeOffset>? utcNow
            // the row died), or an upsert whose new row carries the OLD version and therefore loses
            // the next LWW comparison it should win. A per-entry savepoint makes the pair
            // all-or-nothing without widening the poison catch to the whole batch.
+            //
+            // Opened OUTSIDE the try on purpose: a SAVEPOINT that failed opened nothing, so the
+            // handler's ROLLBACK TO would name a savepoint that does not exist and fault in turn.
            await sql.EnterEntryAsync(ct);
            try
            {
@@ -117,8 +120,27 @@ internal sealed class LwwApplier(SqliteLocalDb db, Func<DateTimeOffset>? utcNow
                // statement, not the transaction, so the batch continues after dead-lettering.
                // Any other exception propagates with the savepoint still open, and disposing the
                // outer transaction unwinds the whole batch (fail-closed).
-                await sql.RollbackEntryAsync(ct);
-                await sql.DeadLetterAsync(entry, ex.Message, utcNow, ct);
+                try
+                {
+                    await sql.RollbackEntryAsync(ct);
+                    await sql.DeadLetterAsync(entry, ex.Message, utcNow, ct);
+                }
+                catch (Exception recoveryFailure) when (recoveryFailure is not OperationCanceledException)
+                {
+                    // Recovering from the poison entry itself failed — a wedged connection, an I/O
+                    // error, a dead-letter INSERT that faults. The outcome is unchanged and still
+                    // fail-closed (nothing commits; disposing the batch transaction unwinds it),
+                    // but reporting only the recovery failure would throw away the ORIGINAL cause,
+                    // which is the one that says WHY this entry is poison. Both are carried out:
+                    // the original as InnerException (AggregateException surfaces the first inner
+                    // there), the recovery failure alongside it with its own stack intact.
+                    throw new AggregateException(
+                        $"Failed to dead-letter a poison oplog entry for table '{entry.TableName}' " +
+                        $"(seq {entry.Seq}): {recoveryFailure.Message}. The entry's own fault is the " +
+                        "inner exception. The batch is not committed.",
+                        ex, recoveryFailure);
+                }
+
                deadLettered++;
            }
        }
@@ -543,6 +543,41 @@ public sealed class LwwApplierTests : IDisposable
        Assert.Equal(("LOCAL", (long?)5), await ReadOrder(db, 1));
    }

+    [Fact]
+    public async Task Apply_DeadLetterFailure_ReportsBothCausesAndCommitsNothing()
+    {
+        // Two faults stacked: the entry is poison (its ledger write aborts) AND recovering from it
+        // fails (the dead-letter INSERT aborts too). Reporting only the second would discard the
+        // one that says WHY the entry was poison, leaving an operator with "dead-letter failed"
+        // and no cause.
+        using var db = await NewOrdersDb();
+        await db.ExecuteAsync("INSERT INTO orders (id, sku, qty) VALUES (1, 'LOCAL', 5)");
+        var localHlc = await RowVersionHlc(db, Pk1);
+        await PoisonRowVersionWriteFor(db, Pk1);
+        await db.ExecuteAsync(
+            "CREATE TRIGGER poison_dl BEFORE INSERT ON __localdb_dead_letter " +
+            "BEGIN SELECT RAISE(ABORT, 'dead-letter write failed'); END");
+
+        var ex = await Assert.ThrowsAsync<AggregateException>(() => new LwwApplier(db).ApplyBatchAsync(
+        [
+            Entry(95, "{\"id\":2}", "{\"id\":2,\"sku\":\"BEFORE\",\"qty\":2}", localHlc + 1000, "remote-node"),
+            Entry(96, Pk1, null, localHlc + 1001, "remote-node", tombstone: true),
+        ], default));
+
+        // The entry's own fault first (InnerException), the recovery fault carried alongside it.
+        Assert.Contains("ledger write failed", ex.InnerException!.Message);
+        Assert.Contains("dead-letter write failed", ex.InnerExceptions[1].Message);
+        Assert.Contains("orders", ex.Message);
+        Assert.Contains("96", ex.Message);
+
+        // Fail-closed: the batch never commits, so even the entry that applied cleanly is gone.
+        Assert.Null(await ReadOrder(db, 2));
+        Assert.Equal(("LOCAL", (long?)5), await ReadOrder(db, 1));
+        var watermark = await db.QueryAsync(
+            "SELECT last_applied_remote_seq FROM __localdb_peer_state WHERE id = 1", x => x.GetInt64(0));
+        Assert.Equal(0, watermark[0]);
+    }
+
    [Fact]
    public async Task Apply_ReplaceExistingRow_DoesNotFireCaptureTriggers()
    {
@@ -141,6 +141,23 @@ public sealed class RegistrationTests : IDisposable
        Assert.Equal(3, (await Triggers(db)).Count);
    }

+    [Fact]
+    public async Task Register_SelfReferencingCascade_Throws()
+    {
+        // A table can be its own cascading parent: replacing a row would delete that row's
+        // children in the SAME table, uncaptured. The guard reads every foreign key in the
+        // database, so it does not need a special case — but the README promises this, so it is
+        // pinned.
+        using var db = Create();
+        await db.ExecuteAsync(
+            "CREATE TABLE nodes (id INTEGER PRIMARY KEY, " +
+            "parent_id INTEGER REFERENCES nodes(id) ON DELETE CASCADE, name TEXT)");
+
+        var ex = Assert.Throws<LocalDbRegistrationException>(() => db.RegisterReplicated("nodes"));
+        Assert.Contains("nodes.parent_id", ex.Message);
+        Assert.Contains("ON DELETE CASCADE", ex.Message);
+    }
+
    [Fact]
    public async Task Register_CascadeDeleteChild_IsAllowed()
    {