Skip to content

Commit 33aa6e9

Browse files
authored
Merge pull request #151 from blehnen/sqlite-outbox-149
Add SQLite outbox pattern support (issue #149)
2 parents 26a405a + 348d334 commit 33aa6e9

22 files changed

Lines changed: 2435 additions & 93 deletions

CLAUDE.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,12 @@ Projects target net10.0 and net8.0. Legacy conditional compilation symbols (NETF
179179
- **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.
180180
- **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.
181181
- **`<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.
182+
- **`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.
182183
- **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.
184+
- **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.
185+
- **`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`.
186+
- **`SQLiteConnectionStringBuilder.DataSource` returns empty for `FullUri=file:...` connection strings.** In-memory shared-cache mode uses `FullUri=file:queue?mode=memory&cache=shared`; the builder's `.DataSource` property only populates for `Data Source=` form. `IDbDataSource.DataSource(connString)` (used by `SqliteConnectionInformation.Container`) inherits this gap. Any validator/extractor relying on `.DataSource` for in-memory mode produces empty — risk of validator-mismatch on `FullUri` connection strings. Latent in this codebase as of issue #149; not yet observed in CI.
187+
- **Team-mode Agent dispatch (`Agent(team_name: ...)`) shares the parent session's task list with spawned teammates.** Idle builders auto-claim pending tasks from other waves once their assigned task completes. SQLite outbox milestone (issue #149) Phase 1 Wave 2 hit this: wave-2 builders autonomously executed PLAN-3.1/3.2/3.3 (commits `5ba910d6`, `8a0d6969`, `644228b9`) while the user/orchestrator was discarding the pre-build uncommitted edits those plans targeted. All three commits had to be reverted. Use **plain Agent dispatch (no `team_name`)** for Shipyard builds when teams are enabled to avoid this autopilot drift. Phases 2-4 of the same milestone used Agent mode and had zero autopilot incidents.
183188

184189
## Code Quality
185190
- Prefer correct, complete implementations over minimal ones.

Source/DotNetWorkQueue.Transport.RelationalDatabase/Basic/ExternalTransactionValidator.cs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,12 @@ namespace DotNetWorkQueue.Transport.RelationalDatabase.Basic
4040
/// <see cref="InvalidOperationException"/> with both database names in the
4141
/// message.</description></item>
4242
/// </list>
43+
/// Derived classes may extend this type to apply transport-specific normalization
44+
/// (for example, path-stem extraction for file-based databases) before the
45+
/// database-name check without altering the behavior of the PostgreSQL or
46+
/// SQL Server transports that use the base class directly.
4347
/// </summary>
44-
public sealed class ExternalTransactionValidator
48+
public class ExternalTransactionValidator
4549
{
4650
private readonly IExternalDbNameExtractor _extractor;
4751
private readonly IConnectionInformation _connectionInfo;
@@ -70,7 +74,9 @@ public ExternalTransactionValidator(IExternalDbNameExtractor extractor,
7074
/// <exception cref="ArgumentNullException">Transaction is null.</exception>
7175
/// <exception cref="InvalidOperationException">Connection is null, not open, or
7276
/// points to a different database than the queue's configured container.</exception>
73-
public void Validate(DbTransaction transaction)
77+
/// <remarks>Derived classes may override this method to apply transport-specific
78+
/// normalization (such as path-stem stripping) before the database-name check.</remarks>
79+
public virtual void Validate(DbTransaction transaction)
7480
{
7581
if (transaction == null)
7682
throw new ArgumentNullException(nameof(transaction));
Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
// ---------------------------------------------------------------------
2+
//This file is part of DotNetWorkQueue
3+
//Copyright © 2015-2026 Brian Lehnen
4+
//
5+
//This library is free software; you can redistribute it and/or
6+
//modify it under the terms of the GNU Lesser General Public
7+
//License as published by the Free Software Foundation; either
8+
//version 2.1 of the License, or (at your option) any later version.
9+
//
10+
//This library is distributed in the hope that it will be useful,
11+
//but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
//MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
13+
//Lesser General Public License for more details.
14+
//
15+
//You should have received a copy of the GNU Lesser General Public
16+
//License along with this library; if not, write to the Free Software
17+
//Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
18+
// ---------------------------------------------------------------------
19+
using System;
20+
using System.Data.SQLite;
21+
using DotNetWorkQueue.Configuration;
22+
using DotNetWorkQueue.IntegrationTests.Shared;
23+
using DotNetWorkQueue.Messages;
24+
using DotNetWorkQueue.Transport.RelationalDatabase.Basic;
25+
using DotNetWorkQueue.Transport.SQLite.Basic;
26+
using Microsoft.VisualStudio.TestTools.UnitTesting;
27+
28+
namespace DotNetWorkQueue.Transport.SQLite.Integration.Tests.Outbox
29+
{
30+
/// <summary>
31+
/// Integration tests proving <see cref="AdditionalMessageData"/> survives the
32+
/// caller-transaction Send path on SQLite. Verification uses direct SQL queries against
33+
/// the queue's MetaData table rather than a live consumer dequeue, avoiding consumer
34+
/// lifecycle complexity.
35+
///
36+
/// Assertions:
37+
/// 1. Exactly 1 metadata row after commit (write confirmation).
38+
/// 2. The CorrelationID persisted in the MetaData table matches the Guid returned by
39+
/// <c>sendResult.SentMessage.CorrelationId.Id.Value</c>. Auto-assigned by
40+
/// <c>GenerateMessageHeaders.HeaderSetup</c>; the persisted value must equal what
41+
/// the producer returned to the caller.
42+
/// 3. The Priority column persists the caller-supplied value — the regression guard
43+
/// catches a silent drop of <c>IAdditionalMessageData</c> on the external-transaction
44+
/// path (ISSUE-037).
45+
/// </summary>
46+
[TestClass]
47+
public class SqliteOutboxAdditionalDataTests : SqliteOutboxIntegrationTestBase
48+
{
49+
[ClassInitialize]
50+
public static void Init(TestContext _) => EnsureActivityListenerRegistered();
51+
52+
private const byte ExpectedPriority = 7;
53+
54+
/// <summary>
55+
/// Verifies that CorrelationID and Priority both round-trip through the external-transaction
56+
/// Send path to the MetaData table. Runs for both in-memory and file-based SQLite to confirm
57+
/// the outbox contract holds regardless of connection mode.
58+
/// </summary>
59+
[TestMethod]
60+
[DataRow(true)]
61+
[DataRow(false)]
62+
public void AdditionalMessageData_RoundTrip_PreservesHeadersAndCorrelation(bool inMemory)
63+
{
64+
using var connInfo = new IntegrationConnectionInfo(inMemory);
65+
var qc = new QueueConnection(NewQueueName(), connInfo.ConnectionString);
66+
using var queue = CreateQueue(qc, enablePriority: true);
67+
using var producer = CreateRelationalProducer(qc);
68+
69+
var data = new AdditionalMessageData();
70+
// WHY: Set priority explicitly. Unlike CorrelationID (auto-assigned by
71+
// GenerateMessageHeaders.HeaderSetup), Priority has no fallback in the
72+
// producer path — asserting its persisted value catches a regression where
73+
// the producer silently drops AdditionalMessageData on the external-transaction
74+
// path (ISSUE-037).
75+
data.SetPriority(ExpectedPriority);
76+
var msg = GenerateMessage.Create<FakeMessage>();
77+
78+
IQueueOutputMessage sendResult;
79+
using var conn = new SQLiteConnection(connInfo.ConnectionString);
80+
conn.Open();
81+
using (var transaction = conn.BeginTransaction())
82+
{
83+
sendResult = producer.RelationalProducer.Send(msg, data, transaction);
84+
Assert.IsFalse(sendResult.HasError, sendResult.SendingException?.ToString());
85+
transaction.Commit();
86+
}
87+
88+
// Sanity: exactly one metadata row was committed.
89+
AssertQueueRowCount(qc, 1);
90+
91+
// Retrieve the CorrelationID that the producer returned to the caller.
92+
var returnedCorrelationGuid = (Guid)sendResult.SentMessage.CorrelationId.Id.Value;
93+
94+
// Query the MetaData table directly and assert the persisted GUID matches.
95+
AssertCorrelationIdInMetadata(qc, returnedCorrelationGuid);
96+
97+
// Priority round-trip: the regression guard described above.
98+
AssertPriorityInMetadata(qc, ExpectedPriority);
99+
}
100+
101+
/// <summary>
102+
/// Queries the queue's MetaData table for a single CorrelationID row and asserts it
103+
/// equals <paramref name="expected"/>. Opens a fresh connection to isolate the read
104+
/// from the producer's commit visibility.
105+
/// </summary>
106+
private static void AssertCorrelationIdInMetadata(QueueConnection queueConnection, Guid expected)
107+
{
108+
var info = new SqliteConnectionInformation(queueConnection, new DbDataSource());
109+
var helper = new TableNameHelper(info);
110+
// WHY: Fresh connection per assertion — isolates the read from the producer
111+
// connection and absorbs any SQLite commit-visibility edge cases.
112+
using var conn = new SQLiteConnection(queueConnection.Connection);
113+
conn.Open();
114+
using var cmd = conn.CreateCommand();
115+
cmd.CommandText = $"SELECT CorrelationID FROM {helper.MetaDataName} LIMIT 1";
116+
var raw = cmd.ExecuteScalar();
117+
// WHY: SQLite stores CorrelationID via DbType.StringFixedLength size 38
118+
// (SendMessage.cs lines 124, 182). Read-back is a string; Guid.Parse
119+
// tolerates both {B} and D formats produced by the SQLite driver.
120+
var actual = Guid.Parse((string)raw!);
121+
Assert.AreEqual(expected, actual,
122+
$"CorrelationID did not round-trip: expected {expected}, persisted {actual}.");
123+
}
124+
125+
/// <summary>
126+
/// Queries the queue's MetaData table for the Priority column and asserts the single
127+
/// persisted row equals <paramref name="expected"/>. Opens a fresh connection to isolate
128+
/// the read from the producer's commit visibility.
129+
/// </summary>
130+
private static void AssertPriorityInMetadata(QueueConnection queueConnection, byte expected)
131+
{
132+
var info = new SqliteConnectionInformation(queueConnection, new DbDataSource());
133+
var helper = new TableNameHelper(info);
134+
// WHY: Fresh connection per assertion — same isolation rationale as AssertCorrelationIdInMetadata.
135+
using var conn = new SQLiteConnection(queueConnection.Connection);
136+
conn.Open();
137+
using var cmd = conn.CreateCommand();
138+
cmd.CommandText = $"SELECT Priority FROM {helper.MetaDataName} LIMIT 1";
139+
var raw = cmd.ExecuteScalar();
140+
// WHY: SQLite INTEGER affinity surfaces from ExecuteScalar as long;
141+
// Convert.ToByte handles the narrowing safely (same idiom as PG test).
142+
var actual = Convert.ToByte(raw);
143+
Assert.AreEqual(expected, actual,
144+
$"Priority did not round-trip: expected {expected}, persisted {actual}.");
145+
}
146+
}
147+
}

0 commit comments

Comments
 (0)