fix(sqlite-outbox): address PR #151 review feedback

blehnen · blehnen · commit 348d3345a066 · 2026-05-24T18:18:08.000-05:00
CodeRabbit review on PR #151 surfaced 6 actionable findings; all addressed. PRODUCTION (Major: sealed-cast coupling per CLAUDE.md lesson): - SendMessageCommandHandler.HandleExternalTransaction and the async fork no longer downcast relCommand.ExternalTransaction to System.Data.SQLite's sealed SQLiteTransaction/SQLiteConnection. Both handlers now operate on IDbTransaction/IDbConnection only — the producer's GuardSQLiteTransaction still enforces the concrete type at the API boundary. Removing the casts: * matches the PG/SS transport discipline (sealed-cast lesson, CLAUDE.md) * makes the outbox-fork path unit-testable with NSubstitute on the IDb* interfaces (the sealed types cannot be mocked at all) TESTS (Minor + Major fixes): - SqliteOutboxIntegrationTestBase: added EnsureActivityListenerRegistered helper + ClassInitialize hooks across all 5 outbox suites, so trace decorators are exercised (CLAUDE.md trace-decorator coverage lesson; mirrors PG/SS inbox base). - QueueScope/ProducerScope Dispose() now guard with Interlocked.Exchange for idempotent double-dispose safety. - SqliteOutboxValidationTests.Validation_GuardPass_HappyPath_SingleSend now asserts result.HasError == false in addition to row visibility. - SqliteOutboxRetryBypassTests.RetryBypass_TransientError_SingleAttempt uses Assert.ThrowsExactly<InvalidOperationException> instead of catch-all + null check — false-positive resistance. - Comment cleanup: "hold-tx" → "hold transaction" (matches no-Tx-abbreviation preference and CodeRabbit ask). LESSON CAPTURE: - CLAUDE.md: new SQLite FullUri/SQLiteConnectionStringBuilder symmetry lesson anchored to commit d111b0b — documents the asymmetric-builder bug shape for future SQLite work. 171/171 SQLite unit tests pass; both prod and IT projects build clean.
diff --git a/CLAUDE.md b/CLAUDE.md
@@ -179,6 +179,7 @@ Projects target net10.0 and net8.0. Legacy conditional compilation symbols (NETF
 - **Plan code shapes can drift from the actual API surface.** Outbox PR #138 / REVIEW-1.2 caught a missing `using DotNetWorkQueue.Configuration;` in the `docs/outbox-pattern.md` tutorial code block — copy-paste would have failed CS0246 because `QueueConnection` lives in `DotNetWorkQueue.Configuration` not `DotNetWorkQueue` root. Pattern: reviewers for doc/code-example plans should TRACE the example against the current API surface (mentally compile it), not just verify it matches the plan's code shape.
 - **Phase scope reframing — RESEARCH should validate the phase isn't already done.** ROADMAP Phase 7 framed as "add XML doc comments to phases 2-4 public types." RESEARCH §1 surfaced that the builders had already added docs as they went. Phase 7 reframed to a VERIFICATION pass + csproj gate fixes (net8 `<DocumentationFile>` gap + ISSUE-032 NU1902 closure on Transport.SQLite). Pattern: when a phase title implies authoring, researcher should explicitly confirm the work was not already incidentally done before architect plans new authoring work.
 - **`<WarningsNotAsErrors>` pattern for accepted advisory carry-forward.** ISSUE-032 (OpenTelemetry NU1902 advisory) blocked the strictest `dotnet build -c Release -p:CI=true` build path on `Transport.SQLite`. Phase 7 PLAN-1.1 / commit `88ff8996` added `<WarningsNotAsErrors>NU1902</WarningsNotAsErrors>` to all 3 Release PropertyGroup blocks (`Release|net10.0`, `Release|net8.0`, `Release|AnyCPU`). The advisory still surfaces as a visible warning on every Release build (long-term remediation isn't forgotten) but the build is no longer blocked. Reusable pattern when a ship-blocking advisory is out of scope for the current milestone.
+- **`SQLiteConnectionStringBuilder.DataSource` is empty when `FullUri=` is used — both sides of any compare MUST go through the builder symmetrically.** Outbox milestone PR #151 shipped with an asymmetric validator (`Container` side via builder, connection side via `conn.DataSource`) that passed local on-disk integration but failed every shared-in-memory FullUri test on Jenkins with `'' != 'file:Iabc?mode=memory&cache=shared'`. Pre-fix: extractor read `conn.DataSource` (raw URI on opened FullUri connection); queue side read `Container` → `DbDataSource.DataSource` → `builder.DataSource` (empty). Fix (commit `d111b0b5`): added `SqLiteExternalDbNameExtractor.Canonicalize(string)` that parses via `SQLiteConnectionStringBuilder` and selects `FullUri ?? DataSource` before `Path.GetFileNameWithoutExtension`; validator now uses `Canonicalize(_connectionInfo.ConnectionString)` for `expected`. Pattern: any new "compare two SQLite database identities" code must route both sides through the same builder + selection, never one side via the builder and the other via the open connection's `DataSource` property. Same root shape as the SqlServer `ToUpperInvariant` symmetric-normalization lesson from PR #138.
 - **SQLite + hold-transaction inbox pattern: structurally incompatible.** Inbox milestone (PR #143, issue #149) attempted to add `IRelationalWorkerNotification` to the SQLite transport alongside SqlServer + PostgreSQL. Failed on Jenkins across 14 CI runs in two recurring shapes: (1) `Sync_Commit`/`Async_Commit` NRE on `relational.Transaction` despite cast succeeding, (2) `Sync_Rollback`/`Async_Rollback`/`BusinessRow_*Visibility` handler-never-invoked 30s timeouts. Root cause is NOT a code bug — SQLite uses `BEGIN EXCLUSIVE`-on-write semantics. Holding the dequeue transaction for the duration of the user handler blocks ALL other writers on the queue table, including the worker thread's own next dequeue attempt → deadlock. `EnableHoldTransactionUntilMessageCommitted` has always been a no-op on SQLite for this reason. **Inbox pattern is a permanent non-goal for SQLite; outbox is viable with a documented concurrency caveat.** Treat the existing options-property as obsolete on SqlLite (issue #149 tracks the cleanup). Don't re-attempt the four debugging directions tried in PR #143: lazy-options removal, property-injection, static `AsyncLocal<>`, factory-delegate wiring — none address the structural lock.
 - **Decorator-chain parity audit when porting features across transports.** When adding a capability (e.g., outbox `IRetrySkippable`) to a new transport, audit the FULL handler chain — producer + decorator(s) — not just the producer-level shape. SQLite outbox milestone (issue #149) Phase 1 added the producer queue + send-handler outbox branches + DI wiring, but missed updating `Source/DotNetWorkQueue.Transport.SQLite/Decorator/RetryCommandHandlerOutputDecorator{,Async}.cs` to honor `IRetrySkippable.SkipRetry`. PG and SqlServer have the guard at line 54/55; SQLite was silently retrying caller-managed outbox transactions through Polly. Integration tests caught it (Phase 3 PLAN-2.5 review); fix in commit `66f7e864`. The PG/SS-mirror discipline must extend through decorators, not stop at producers.
 - **`System.Data.SQLite` vs `Microsoft.Data.Sqlite` provider name distinction.** This transport is built on `System.Data.SQLite` (types `SQLiteConnection` / `SQLiteTransaction` — all-caps `SQLite`). `Microsoft.Data.Sqlite` uses `SqliteConnection` / `SqliteTransaction` (lowercase second letter onward). The two are NOT interchangeable. Docs/code identifiers must use the correct provider — copy-paste from Microsoft.Data.Sqlite docs/samples will compile against the wrong NuGet package and fail at runtime in `GuardSQLiteTransaction` (which uses fully-qualified `System.Data.SQLite.SQLiteTransaction`). Caught at Phase 4 docs review for `docs/outbox-pattern.md`; fix in commit `447250fc`.
diff --git a/Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqliteOutboxAdditionalDataTests.cs b/Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqliteOutboxAdditionalDataTests.cs
@@ -46,6 +46,9 @@ namespace DotNetWorkQueue.Transport.SQLite.Integration.Tests.Outbox
     [TestClass]
     public class SqliteOutboxAdditionalDataTests : SqliteOutboxIntegrationTestBase
     {
+        [ClassInitialize]
+        public static void Init(TestContext _) => EnsureActivityListenerRegistered();
+
         private const byte ExpectedPriority = 7;
 
         /// <summary>
diff --git a/Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqliteOutboxIntegrationTestBase.cs b/Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqliteOutboxIntegrationTestBase.cs
@@ -19,6 +19,8 @@
 using System;
 using System.Collections.Generic;
 using System.Data.SQLite;
+using System.Diagnostics;
+using System.Threading;
 using DotNetWorkQueue.Configuration;
 using DotNetWorkQueue.IntegrationTests.Shared;
 using DotNetWorkQueue.Messages;
@@ -53,6 +55,27 @@ namespace DotNetWorkQueue.Transport.SQLite.Integration.Tests.Outbox
     /// </remarks>
     public abstract class SqliteOutboxIntegrationTestBase
     {
+        // ---- ActivityListener (mandatory per CLAUDE.md trace-decorator lesson) ----
+        // Without an ActivityListener registered against the DotNetWorkQueue ActivitySource,
+        // ActivitySource.StartActivity() returns null and trace decorators short-circuit
+        // silently, leaving them at 0% coverage. Mirrors the PG/SS inbox base pattern.
+        private static readonly object ListenerLock = new();
+        private static ActivityListener _listener;
+
+        protected static void EnsureActivityListenerRegistered()
+        {
+            lock (ListenerLock)
+            {
+                if (_listener != null) return;
+                _listener = new ActivityListener
+                {
+                    ShouldListenTo = src => src.Name.StartsWith("DotNetWorkQueue", StringComparison.Ordinal),
+                    Sample = (ref ActivityCreationOptions<ActivityContext> _) => ActivitySamplingResult.AllData
+                };
+                ActivitySource.AddActivityListener(_listener);
+            }
+        }
+
         /// <summary>
         /// Generates a fresh queue name that satisfies DNQ's alphanumeric/underscore/dot constraint
         /// (no hyphens — <see cref="Guid.ToString(string)"/> with "N" strips them).
@@ -76,8 +99,10 @@ protected sealed class QueueScope : IDisposable
             public SqLiteMessageQueueCreation OCreation { get; init; }
             public ICreationScope Scope { get; init; }
 
+            private int _disposed;
             public void Dispose()
             {
+                if (Interlocked.Exchange(ref _disposed, 1) != 0) return;
                 try { OCreation?.RemoveQueue(); } catch { /* swallow — best-effort cleanup */ }
                 OCreation?.Dispose();
                 Scope?.Dispose();
@@ -106,7 +131,7 @@ protected QueueScope CreateQueue(QueueConnection queueConnection, bool enablePri
             oCreation.Options.EnableMessageExpiration = false;
             // EnableHoldTransactionUntilMessageCommitted is not set here — it is an explicit
             // ITransportOptions implementation on SqLiteMessageQueueTransportOptions, hardwired
-            // to false with a discarded setter (SQLite BEGIN EXCLUSIVE semantics make hold-tx
+            // to false with a discarded setter (SQLite BEGIN EXCLUSIVE semantics make hold-transaction
             // structurally non-viable; see issue #149 and the class XML doc above).
             oCreation.Options.EnablePriority = enablePriority;
 
@@ -132,8 +157,10 @@ protected sealed class ProducerScope : IDisposable
             public IProducerQueue<FakeMessage> Producer { get; init; }
             public IRelationalProducerQueue<FakeMessage> RelationalProducer { get; init; }
 
+            private int _disposed;
             public void Dispose()
             {
+                if (Interlocked.Exchange(ref _disposed, 1) != 0) return;
                 Producer?.Dispose();
                 Creator?.Dispose();
             }
diff --git a/Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqliteOutboxRetryBypassTests.cs b/Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqliteOutboxRetryBypassTests.cs
@@ -37,6 +37,9 @@ namespace DotNetWorkQueue.Transport.SQLite.Integration.Tests.Outbox
     [TestClass]
     public class SqliteOutboxRetryBypassTests : SqliteOutboxIntegrationTestBase
     {
+        [ClassInitialize]
+        public static void Init(TestContext _) => EnsureActivityListenerRegistered();
+
         /// <summary>
         /// Verifies that sending with a completed (already-committed) transaction throws on the
         /// first attempt and does not engage the Polly retry chain.
@@ -64,16 +67,11 @@ public void RetryBypass_TransientError_SingleAttempt(bool inMemory)
             var completedTransaction = conn.BeginTransaction();
             completedTransaction.Commit();
 
-            Exception thrown = null;
             var stopwatch = Stopwatch.StartNew();
-            try
-            {
-                producer.RelationalProducer.Send(new FakeMessage(), new AdditionalMessageData(), completedTransaction);
-            }
-            catch (Exception ex)
-            {
-                thrown = ex;
-            }
+            // The validator's #2 check (null Connection on completed transaction) throws
+            // InvalidOperationException; SkipRetry=true short-circuits Polly before any retry.
+            var thrown = Assert.ThrowsExactly<InvalidOperationException>(() =>
+                producer.RelationalProducer.Send(new FakeMessage(), new AdditionalMessageData(), completedTransaction));
             stopwatch.Stop();
 
             // WHY: Polly's default 3-attempt retry chain with exponential back-off adds seconds. A
diff --git a/Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqliteOutboxSendAsyncTests.cs b/Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqliteOutboxSendAsyncTests.cs
@@ -28,6 +28,9 @@ namespace DotNetWorkQueue.Transport.SQLite.Integration.Tests.Outbox
     [TestClass]
     public class SqliteOutboxSendAsyncTests : SqliteOutboxIntegrationTestBase
     {
+        [ClassInitialize]
+        public static void Init(TestContext _) => EnsureActivityListenerRegistered();
+
         [TestMethod]
         [DataRow(true)]
         [DataRow(false)]
diff --git a/Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqliteOutboxSendTests.cs b/Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqliteOutboxSendTests.cs
@@ -27,6 +27,9 @@ namespace DotNetWorkQueue.Transport.SQLite.Integration.Tests.Outbox
     [TestClass]
     public class SqliteOutboxSendTests : SqliteOutboxIntegrationTestBase
     {
+        [ClassInitialize]
+        public static void Init(TestContext _) => EnsureActivityListenerRegistered();
+
         [TestMethod]
         [DataRow(true)]
         [DataRow(false)]
diff --git a/Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqliteOutboxValidationTests.cs b/Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqliteOutboxValidationTests.cs
@@ -36,6 +36,9 @@ namespace DotNetWorkQueue.Transport.SQLite.Integration.Tests.Outbox
     [TestClass]
     public class SqliteOutboxValidationTests : SqliteOutboxIntegrationTestBase
     {
+        [ClassInitialize]
+        public static void Init(TestContext _) => EnsureActivityListenerRegistered();
+
         /// <summary>
         /// Guard: cross-database mismatch — transaction belongs to a DIFFERENT SQLite file/URI
         /// than the queue's database. Must throw before any INSERT lands in the queue table.
@@ -131,8 +134,9 @@ public void Validation_GuardPass_HappyPath_SingleSend(bool inMemory)
             conn.Open();
             using var transaction = conn.BeginTransaction();
 
-            // Assert no throw — happy path.
-            producer.RelationalProducer.Send(new FakeMessage(), new AdditionalMessageData(), transaction);
+            // Assert no throw AND no send error — happy path.
+            var result = producer.RelationalProducer.Send(new FakeMessage(), new AdditionalMessageData(), transaction);
+            Assert.IsFalse(result.HasError, result.SendingException?.ToString());
 
             transaction.Commit();
 
diff --git a/Source/DotNetWorkQueue.Transport.SQLite/Basic/CommandHandler/SendMessageCommandHandler.cs b/Source/DotNetWorkQueue.Transport.SQLite/Basic/CommandHandler/SendMessageCommandHandler.cs
@@ -227,15 +227,20 @@ public long Handle(SendMessageCommand commandSend)
 
         /// <summary>
         /// Caller-supplied-transaction fork of <see cref="Handle(SendMessageCommand)"/>. Reuses
-        /// the caller's <see cref="SQLiteTransaction"/> and its <see cref="SQLiteConnection"/>
-        /// for all queue INSERTs; never commits, rolls back, closes, or disposes the caller's
-        /// resources. Invoked from <see cref="Handle"/> when
-        /// <see cref="RelationalSendMessageCommand.ExternalTransaction"/> is non-null. The producer
-        /// surface (<c>SqliteRelationalProducerQueue&lt;T&gt;</c>) guarantees the
-        /// transaction is a <see cref="SQLiteTransaction"/> and its connection's database
-        /// matches the queue's configured database via the validator at the API boundary,
-        /// so this method performs no validation of its own.
+        /// the caller's transaction and its connection for all queue INSERTs; never commits,
+        /// rolls back, closes, or disposes the caller's resources. Invoked from
+        /// <see cref="Handle"/> when <see cref="RelationalSendMessageCommand.ExternalTransaction"/>
+        /// is non-null. The producer surface (<c>SqliteRelationalProducerQueue&lt;T&gt;</c>)
+        /// guarantees the transaction is a <c>System.Data.SQLite.SQLiteTransaction</c> and its
+        /// connection's database matches the queue's configured database via the validator at
+        /// the API boundary, so this method performs no validation of its own.
         /// </summary>
+        /// <remarks>
+        /// The handler operates on <see cref="IDbTransaction"/> / <see cref="IDbConnection"/>
+        /// abstractions only — no downcast to the sealed <c>System.Data.SQLite</c> provider
+        /// types. Same discipline applied across the PG/SS transports (CLAUDE.md sealed-cast
+        /// lesson) so the method remains unit-testable with NSubstitute on the interfaces.
+        /// </remarks>
         /// <param name="commandSend">The send-message command carrying a non-null
         /// <see cref="RelationalSendMessageCommand.ExternalTransaction"/>.</param>
         /// <returns>The newly-inserted message ID.</returns>
@@ -247,11 +252,10 @@ private long HandleExternalTransaction(SendMessageCommand commandSend)
             // Cast guard: Handle() only routes here for RelationalSendMessageCommand
             // (the `commandSend is RelationalSendMessageCommand` pattern in the fork check),
             // so this cast is always safe. The producer subclass also enforces the
-            // DbTransaction subtype via GuardSQLiteTransaction before construction.
+            // SQLiteTransaction subtype via GuardSQLiteTransaction before construction.
             var relCommand = (RelationalSendMessageCommand)commandSend;
-            // fully-qualified to avoid collision with DotNetWorkQueue.Transport.SQLite.Basic.SQLiteTransaction wrapper
-            var sqliteTransaction = (System.Data.SQLite.SQLiteTransaction)relCommand.ExternalTransaction;
-            var sqliteConn = (System.Data.SQLite.SQLiteConnection)sqliteTransaction.Connection;
+            IDbTransaction transaction = relCommand.ExternalTransaction;
+            IDbConnection connection = transaction.Connection;
 
             var expiration = TimeSpan.Zero;
             if (_messageExpirationEnabled.Value)
@@ -270,20 +274,20 @@ private long HandleExternalTransaction(SendMessageCommand commandSend)
 
             if (!(string.IsNullOrWhiteSpace(jobName) ||
                   _jobExistsHandler.Handle(new DoesJobExistQuery<IDbConnection, IDbTransaction>(
-                      jobName, scheduledTime, sqliteConn, sqliteTransaction)) == QueueStatuses.NotQueued))
+                      jobName, scheduledTime, connection, transaction)) == QueueStatuses.NotQueued))
             {
                 throw new DotNetWorkQueueException(
                     "Failed to insert record - the job has already been queued or processed");
             }
 
             long id;
-            using (var command = SendMessage.GetMainCommand(commandSend, sqliteConn, _commandCache, _headers, _serializer))
+            using (var command = SendMessage.GetMainCommand(commandSend, connection, _commandCache, _headers, _serializer))
             {
-                command.Transaction = sqliteTransaction;
+                command.Transaction = transaction;
                 command.ExecuteNonQuery();
-                using (var commandId = sqliteConn.CreateCommand())
+                using (var commandId = connection.CreateCommand())
                 {
-                    commandId.Transaction = sqliteTransaction;
+                    commandId.Transaction = transaction;
                     commandId.CommandText = "SELECT last_insert_rowid();";
                     id = Convert.ToInt64(commandId.ExecuteScalar());
                 }
@@ -296,10 +300,10 @@ private long HandleExternalTransaction(SendMessageCommand commandSend)
             }
 
             using (var commandMeta = SendMessage.CreateMetaDataRecord(commandSend.MessageData.GetDelay(), expiration,
-                sqliteConn, commandSend.MessageToSend, commandSend.MessageData, _tableNameHelper, _headers,
+                connection, commandSend.MessageToSend, commandSend.MessageData, _tableNameHelper, _headers,
                 _options.Value, _getTime))
             {
-                commandMeta.Transaction = sqliteTransaction;
+                commandMeta.Transaction = transaction;
                 var param = commandMeta.CreateParameter();
                 param.ParameterName = "@QueueID";
                 param.DbType = DbType.Int64;
@@ -310,9 +314,9 @@ private long HandleExternalTransaction(SendMessageCommand commandSend)
 
             if (_options.Value.EnableStatusTable)
             {
-                using (var commandStatus = CreateStatusRecord(sqliteConn, commandSend.MessageToSend, commandSend.MessageData))
+                using (var commandStatus = CreateStatusRecord(connection, commandSend.MessageToSend, commandSend.MessageData))
                 {
-                    commandStatus.Transaction = sqliteTransaction;
+                    commandStatus.Transaction = transaction;
                     var param = commandStatus.CreateParameter();
                     param.ParameterName = "@QueueID";
                     param.DbType = DbType.Int64;
@@ -325,7 +329,7 @@ private long HandleExternalTransaction(SendMessageCommand commandSend)
             if (!string.IsNullOrWhiteSpace(jobName))
             {
                 _sendJobStatus.Handle(new SetJobLastKnownEventCommand<IDbConnection, IDbTransaction>(
-                    jobName, eventTime, scheduledTime, sqliteConn, sqliteTransaction));
+                    jobName, eventTime, scheduledTime, connection, transaction));
             }
 
             // Caller owns lifecycle: no Commit, Rollback, Close, or Dispose performed here.
diff --git a/Source/DotNetWorkQueue.Transport.SQLite/Basic/CommandHandler/SendMessageCommandHandlerAsync.cs b/Source/DotNetWorkQueue.Transport.SQLite/Basic/CommandHandler/SendMessageCommandHandlerAsync.cs

Original file line number	Diff line number	Diff line change
`@@ -46,6 +46,9 @@ namespace DotNetWorkQueue.Transport.SQLite.Integration.Tests.Outbox`
`46`	`46`	`[TestClass]`
`47`	`47`	`public class SqliteOutboxAdditionalDataTests : SqliteOutboxIntegrationTestBase`
`48`	`48`	`{`
	`49`	`+ [ClassInitialize]`
	`50`	`+ public static void Init(TestContext _) => EnsureActivityListenerRegistered();`
	`51`	`+`
`49`	`52`	`private const byte ExpectedPriority = 7;`
`50`	`53`
`51`	`54`	`/// <summary>`