Skip to content

Commit 728e3dc

Browse files
authored
Merge pull request #152 from blehnen/revert-sqlite-outbox-149
Revert SQLite outbox pattern support (#151)
2 parents 33aa6e9 + cbd78ef commit 728e3dc

22 files changed

Lines changed: 93 additions & 2435 deletions

CLAUDE.md

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -179,12 +179,7 @@ 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.
183182
- **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.
188183

189184
## Code Quality
190185
- Prefer correct, complete implementations over minimal ones.

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

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,8 @@ 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.
4743
/// </summary>
48-
public class ExternalTransactionValidator
44+
public sealed class ExternalTransactionValidator
4945
{
5046
private readonly IExternalDbNameExtractor _extractor;
5147
private readonly IConnectionInformation _connectionInfo;
@@ -74,9 +70,7 @@ public ExternalTransactionValidator(IExternalDbNameExtractor extractor,
7470
/// <exception cref="ArgumentNullException">Transaction is null.</exception>
7571
/// <exception cref="InvalidOperationException">Connection is null, not open, or
7672
/// points to a different database than the queue's configured container.</exception>
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)
73+
public void Validate(DbTransaction transaction)
8074
{
8175
if (transaction == null)
8276
throw new ArgumentNullException(nameof(transaction));

Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqliteOutboxAdditionalDataTests.cs

Lines changed: 0 additions & 147 deletions
This file was deleted.

0 commit comments

Comments
 (0)