Inbox pattern support for relational transports#143
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds IRelationalWorkerNotification and transport-specific relational notifications; conditionally registers and injects per-message connection/transaction; implements SQLite caller-transaction outbox paths and receive/send lifecycle changes; and adds unit and integration tests for commit/rollback visibility and registration branching. ChangesRelational Inbox/Outbox Pattern
Sequence Diagram (high-level inbox flow) sequenceDiagram
participant Receive as ReceiveMessage
participant DI as DI/Resolve
participant Notification as WorkerNotification/IRelationalWorkerNotification
participant Handler as UserHandler
participant DB as DbTransaction
Receive->>DI: Resolve IWorkerNotification (based on options)
DI->>Notification: create appropriate notification
Receive->>Notification: inject ConnectionHolder (when relational)
Receive->>Handler: invoke handler with context
Handler->>Notification: cast to IRelationalWorkerNotification
Handler->>DB: access Transaction and perform business writes
Handler->>Receive: return -> commit or throw -> rollback
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
✨ Finishing Touches📝 Generate docstrings
|
Owner
Author
|
@CodeRabbit review --dir /source |
|
Scope the review to the ✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
blehnen
commented
May 19, 2026
blehnen
commented
May 19, 2026
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
This was referenced May 19, 2026
blehnen
force-pushed
the
phase-2-inbox-foundation
branch
from
f5852b0 to
807fb7a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteMessageQueueReceive.cs (1)
240-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways detach context handlers even if resource disposal fails.
ContextCleanupunsubscribes events after disposing transaction/connection. If either dispose throws, handler detachment is skipped, risking leaked subscriptions and duplicate callbacks on later reuse. Move unsubscription into afinallyblock.Proposed fix
private void ContextCleanup(IMessageContext context) { - var state = context.Get(_sqLiteHeaders.ConnectionState); - if (state != null) - { - state.Transaction.Dispose(); - state.Connection.Dispose(); - } - - context.Commit -= ContextOnCommit; - context.Rollback -= ContextOnRollback; - context.Cleanup -= Context_Cleanup; + try + { + var state = context.Get(_sqLiteHeaders.ConnectionState); + if (state != null) + { + state.Transaction.Dispose(); + state.Connection.Dispose(); + } + } + finally + { + context.Commit -= ContextOnCommit; + context.Rollback -= ContextOnRollback; + context.Cleanup -= Context_Cleanup; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteMessageQueueReceive.cs` around lines 240 - 255, ContextCleanup currently disposes state.Transaction and state.Connection and then unsubscribes handlers, which means an exception during Dispose can skip detachment; change ContextCleanup so that disposal of state.Transaction and state.Connection is performed inside a try (or try/catch if you want to log) and always detach the event handlers (context.Commit -= ContextOnCommit; context.Rollback -= ContextOnRollback; context.Cleanup -= Context_Cleanup) in a finally block to guarantee unsubscription even if Dispose throws, while still null-checking state from context.Get(_sqLiteHeaders.ConnectionState).
🧹 Nitpick comments (3)
Source/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxIntegrationTestBase.cs (1)
74-79: ⚡ Quick winMake
QueueScope.Dispose()idempotent and thread-safe.
Dispose()currently has no guard against double/concurrent invocation.As per coding guidelines, "Implement thread-safe disposal using `Interlocked` operations throughout the codebase".Suggested patch
protected sealed class QueueScope : IDisposable { + private int _disposed; public QueueCreationContainer<SqlServerMessageQueueInit> QueueCreator { get; init; } public SqlServerMessageQueueCreation OCreation { get; init; } public void Dispose() { + if (Interlocked.Exchange(ref _disposed, 1) == 1) return; try { OCreation?.RemoveQueue(); } catch { /* swallow — teardown */ } OCreation?.Dispose(); QueueCreator?.Dispose(); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxIntegrationTestBase.cs` around lines 74 - 79, The Dispose implementation must be made idempotent and thread-safe: add a private int disposal flag (e.g., _disposed) and use Interlocked.Exchange to ensure Dispose on QueueScope (the Dispose method that calls OCreation?.RemoveQueue(), OCreation?.Dispose(), and QueueCreator?.Dispose()) runs only once; if Interlocked.Exchange indicates disposal already occurred, return immediately; otherwise proceed to call RemoveQueue and Dispose safely (catching any teardown exceptions as before). Ensure the same pattern is applied to QueueScope.Dispose to prevent double/concurrent invocation.Source/DotNetWorkQueue.Transport.SQLite/Basic/Message/ReceiveMessage.cs (1)
49-53: ⚡ Quick winUse “transaction” instead of “tx” in comments/docs.
Please replace
hold-txoccurrences withhold-transactionto match repository terminology rules.As per coding guidelines, "Use the full word 'transaction' in identifiers and prose, not the abbreviation 'Tx'".
Also applies to: 93-98, 127-127, 136-136, 145-149
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/DotNetWorkQueue.Transport.SQLite/Basic/Message/ReceiveMessage.cs` around lines 49 - 53, The XML docs and any occurrences of the abbreviation "tx" in the ReceiveMessage constructor comments and related code should be changed to the full word "transaction" (e.g., replace "hold-tx" with "hold-transaction" and any identifier-like uses of "holdTx" with "holdTransaction") to follow repository terminology; update the XML summary/param text near ReceiveMessage and the other reported locations (around the ranges you noted: 93-98, 127, 136, 145-149) so all prose and identifiers use "transaction" consistently (check variable names such as dbFactory, connectionInformation, optionsFactory, sqLiteHeaders and adjust linked identifiers or documentation text accordingly).Source/DotNetWorkQueue.Transport.SQLite.Tests/Basic/SqLiteExternalDbNameExtractorTests.cs (1)
73-82: ⚡ Quick winStrengthen the null-
DataSourceassertion to lock behavior.The current check (
Assert.IsNotNull) is too weak and won’t catch regressions in canonicalization output. Assert the exact expected normalized value instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/DotNetWorkQueue.Transport.SQLite.Tests/Basic/SqLiteExternalDbNameExtractorTests.cs` around lines 73 - 82, The test Extract_Returns_Empty_For_Null_DataSource currently only checks result is not null; update it to assert the exact canonicalized output from SqLiteExternalDbNameExtractor.Extract when DbConnection.DataSource is null (i.e. assert result equals the expected normalized empty string), so replace the weak Assert.IsNotNull with an Assert.AreEqual(expected, result) using string.Empty (or "") to lock the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Source/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxIntegrationTestBase.cs`:
- Around line 59-71: QueueScope.Dispose() is not idempotent or thread-safe; add
a private int _disposed field and use Interlocked.Exchange(ref _disposed, 1) at
start of Dispose to ensure cleanup runs once, then perform the existing cleanup
(OCreation?.RemoveQueue(), OCreation.Dispose(), Scope.Dispose(),
QueueCreator.Dispose()) only if the exchange returned 0; after disposing,
optionally null out fields (QueueCreator, OCreation, Scope) to avoid repeated
use. Ensure references to QueueScope, QueueCreator, OCreation, Scope and Dispose
are updated accordingly.
In
`@Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxIntegrationTestBase.cs`:
- Around line 63-75: The Dispose implementations for QueueScope (and similarly
ProducerScope) must be made idempotent and thread-safe by adding an
Interlocked.Exchange guard: add a private int _disposed field to the type, then
at start of Dispose call if (Interlocked.Exchange(ref _disposed, 1) == 1)
return; otherwise perform the existing cleanup (OCreation?.RemoveQueue(),
OCreation?.Dispose(), Scope?.Dispose(), QueueCreator?.Dispose()). Apply the same
pattern to ProducerScope.Dispose so concurrent or double-dispose calls only run
the cleanup once.
In
`@Source/DotNetWorkQueue.Transport.SQLite.Tests/Basic/SqLiteRelationalWorkerNotificationRegistrationTests.cs`:
- Around line 65-66: Replace the fixed "ADMIN" queue name with a generated
hyphen-less GUID string using Guid.NewGuid().ToString("N") wherever a
QueueConnection is constructed in this test file (e.g., the
qc.CreateConsumer(new QueueConnection("ADMIN", FakeConnection)) call and the
other occurrences around lines 93–94); update the QueueConnection
instantiation(s) so they use the generated name to satisfy DNQ validation and
avoid parallel test collisions while keeping the rest of the call (e.g.,
FakeConnection) unchanged.
In `@Source/DotNetWorkQueue.Transport.SQLite/Basic/Message/ReceiveMessage.cs`:
- Around line 142-157: Context/header assignment (context.SetMessageAndHeaders
and subsequent context.Set(_sqLiteHeaders.ConnectionState) +
relationalNotification.ConnectionState = state using SqLiteConnectionState with
heldConnection/heldTransaction) can throw and currently leaks the
heldConnection/heldTransaction; wrap the mutation in a try/catch/finally so that
if any exception occurs while calling context.SetMessageAndHeaders, context.Set
or assigning to context.WorkerNotification, you dispose/cleanup the created
SqLiteConnectionState (and underlying heldConnection/heldTransaction) and leave
context in a consistent state before rethrowing; ensure you create the
SqLiteConnectionState only once, assign it atomically to context and
relationalNotification inside the try, and in the catch/finally null-out or
remove the header key if you partially set it to avoid double-disposal.
In `@Source/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteConnectionState.cs`:
- Around line 62-81: The Completed flag in SqLiteConnectionState is not atomic;
replace the bool Completed with a private int backing field (e.g., _completed)
and expose a read-only bool property Completed => _completed != 0, then
implement MarkCompleted() to atomically set the flag using
Interlocked.Exchange(ref _completed, 1); ensure any other checks that read
Completed still use the property and add using System.Threading if needed so
commit/rollback idempotency becomes thread-safe.
In
`@Source/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteExternalDbNameExtractor.cs`:
- Around line 55-57: The Extract method in SqLiteExternalDbNameExtractor
dereferences the DbConnection parameter without checking for null; add an
explicit null guard at the start of Extract (e.g., check if connection is null
and throw an ArgumentNullException with nameof(connection)) so callers get a
clear, actionable exception instead of a NullReferenceException when connection
is null.
In
`@Source/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteMessageQueueSharedInit.cs`:
- Around line 154-171: The registration lambda for IWorkerNotification swallows
all exceptions when resolving ITransportOptionsFactory (the try/catch around
optionsFactory.Create()), which can hide real configuration/runtime errors;
change the catch to only handle expected resolution/verification exceptions
(e.g., container verification or missing binding exceptions) and either rethrow
unexpected exceptions or log and fail-fast so misconfiguration is visible;
update the container.Register lambda that inspects ITransportOptionsFactory and
SqLiteMessageQueueTransportOptions.EnableHoldTransactionUntilMessageCommitted to
selectively catch specific exceptions (or rethrow) and ensure it still returns
SqLiteRelationalWorkerNotification or WorkerNotification only when resolution is
genuinely successful.
In
`@Source/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxIntegrationTestBase.cs`:
- Around line 108-140: In the XML doc/comment above
WaitForBusinessRows/CountBusinessRowsFromSeparateConnection (and any other
inline comments in this file) replace the abbreviation "tx" with the full word
"transaction" so comments follow the coding guideline; search for occurrences of
"tx" in comments near methods CreateBusinessTable, DropBusinessTable,
CountBusinessRowsFromSeparateConnection and the polling summary and update them
to "transaction".
---
Outside diff comments:
In `@Source/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteMessageQueueReceive.cs`:
- Around line 240-255: ContextCleanup currently disposes state.Transaction and
state.Connection and then unsubscribes handlers, which means an exception during
Dispose can skip detachment; change ContextCleanup so that disposal of
state.Transaction and state.Connection is performed inside a try (or try/catch
if you want to log) and always detach the event handlers (context.Commit -=
ContextOnCommit; context.Rollback -= ContextOnRollback; context.Cleanup -=
Context_Cleanup) in a finally block to guarantee unsubscription even if Dispose
throws, while still null-checking state from
context.Get(_sqLiteHeaders.ConnectionState).
---
Nitpick comments:
In
`@Source/DotNetWorkQueue.Transport.SQLite.Tests/Basic/SqLiteExternalDbNameExtractorTests.cs`:
- Around line 73-82: The test Extract_Returns_Empty_For_Null_DataSource
currently only checks result is not null; update it to assert the exact
canonicalized output from SqLiteExternalDbNameExtractor.Extract when
DbConnection.DataSource is null (i.e. assert result equals the expected
normalized empty string), so replace the weak Assert.IsNotNull with an
Assert.AreEqual(expected, result) using string.Empty (or "") to lock the
behavior.
In `@Source/DotNetWorkQueue.Transport.SQLite/Basic/Message/ReceiveMessage.cs`:
- Around line 49-53: The XML docs and any occurrences of the abbreviation "tx"
in the ReceiveMessage constructor comments and related code should be changed to
the full word "transaction" (e.g., replace "hold-tx" with "hold-transaction" and
any identifier-like uses of "holdTx" with "holdTransaction") to follow
repository terminology; update the XML summary/param text near ReceiveMessage
and the other reported locations (around the ranges you noted: 93-98, 127, 136,
145-149) so all prose and identifiers use "transaction" consistently (check
variable names such as dbFactory, connectionInformation, optionsFactory,
sqLiteHeaders and adjust linked identifiers or documentation text accordingly).
In
`@Source/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxIntegrationTestBase.cs`:
- Around line 74-79: The Dispose implementation must be made idempotent and
thread-safe: add a private int disposal flag (e.g., _disposed) and use
Interlocked.Exchange to ensure Dispose on QueueScope (the Dispose method that
calls OCreation?.RemoveQueue(), OCreation?.Dispose(), and
QueueCreator?.Dispose()) runs only once; if Interlocked.Exchange indicates
disposal already occurred, return immediately; otherwise proceed to call
RemoveQueue and Dispose safely (catching any teardown exceptions as before).
Ensure the same pattern is applied to QueueScope.Dispose to prevent
double/concurrent invocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d584e820-0b0b-45b5-b973-93e1cdb8aea6
📒 Files selected for processing (54)
Source/DotNetWorkQueue.Transport.LiteDb.Tests/Basic/LiteDbProducerDoesNotImplementRelationalTests.csSource/DotNetWorkQueue.Transport.Memory.Tests/Basic/MemoryProducerDoesNotImplementRelationalTests.csSource/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxAsyncHandlerTests.csSource/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxAtomicVisibilityTests.csSource/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxIntegrationTestBase.csSource/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxOptionFalseTests.csSource/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxSyncHandlerTests.csSource/DotNetWorkQueue.Transport.PostgreSQL.Tests/Basic/PostgreSqlRelationalWorkerNotificationRegistrationTests.csSource/DotNetWorkQueue.Transport.PostgreSQL.Tests/Basic/PostgreSqlRelationalWorkerNotificationTests.csSource/DotNetWorkQueue.Transport.PostgreSQL/Basic/PostgreSQLMessageQueueInit.csSource/DotNetWorkQueue.Transport.PostgreSQL/Basic/PostgreSQLMessageQueueReceive.csSource/DotNetWorkQueue.Transport.PostgreSQL/Basic/PostgreSqlRelationalWorkerNotification.csSource/DotNetWorkQueue.Transport.Redis.Tests/Basic/RedisProducerDoesNotImplementRelationalTests.csSource/DotNetWorkQueue.Transport.RelationalDatabase.Tests/IRelationalWorkerNotificationContractTests.csSource/DotNetWorkQueue.Transport.RelationalDatabase/IRelationalWorkerNotification.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Inbox/SqLiteInboxAsyncHandlerTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Inbox/SqLiteInboxAtomicVisibilityTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Inbox/SqLiteInboxIntegrationTestBase.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Inbox/SqLiteInboxOptionFalseTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Inbox/SqLiteInboxSyncHandlerTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxAdditionalDataTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxBatchTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxIntegrationTestBase.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxRetryBypassTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxSendAsyncTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxSendTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxValidationTests.csSource/DotNetWorkQueue.Transport.SQLite.Tests/Basic/SqLiteExternalDbNameExtractorTests.csSource/DotNetWorkQueue.Transport.SQLite.Tests/Basic/SqLiteRelationalWorkerNotificationRegistrationTests.csSource/DotNetWorkQueue.Transport.SQLite.Tests/Basic/SqLiteRelationalWorkerNotificationTests.csSource/DotNetWorkQueue.Transport.SQLite.Tests/Basic/SqliteProducerDoesNotImplementRelationalTests.csSource/DotNetWorkQueue.Transport.SQLite/Basic/CommandHandler/SendMessageCommandHandler.csSource/DotNetWorkQueue.Transport.SQLite/Basic/CommandHandler/SendMessageCommandHandlerAsync.csSource/DotNetWorkQueue.Transport.SQLite/Basic/Message/ReceiveMessage.csSource/DotNetWorkQueue.Transport.SQLite/Basic/QueryHandler/ReceiveMessageQueryHandler.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SQLiteMessageQueueTransportOptions.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteConnectionState.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteExternalDbNameExtractor.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteHeaders.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteMessageQueueReceive.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteMessageQueueSharedInit.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteRelationalProducerQueue.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteRelationalWorkerNotification.csSource/DotNetWorkQueue.Transport.SQLite/SqliteNormalizedConnectionInformation.csSource/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxAsyncHandlerTests.csSource/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxAtomicVisibilityTests.csSource/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxIntegrationTestBase.csSource/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxOptionFalseTests.csSource/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxSyncHandlerTests.csSource/DotNetWorkQueue.Transport.SqlServer.Tests/Basic/SqlServerRelationalWorkerNotificationRegistrationTests.csSource/DotNetWorkQueue.Transport.SqlServer.Tests/Basic/SqlServerRelationalWorkerNotificationTests.csSource/DotNetWorkQueue.Transport.SqlServer/Basic/SQLServerMessageQueueInit.csSource/DotNetWorkQueue.Transport.SqlServer/Basic/SQLServerMessageQueueReceive.csSource/DotNetWorkQueue.Transport.SqlServer/Basic/SqlServerRelationalWorkerNotification.cs
💤 Files with no reviewable changes (1)
- Source/DotNetWorkQueue.Transport.SQLite.Tests/Basic/SqliteProducerDoesNotImplementRelationalTests.cs
blehnen
added a commit
that referenced
this pull request
May 20, 2026
Production fixes:
- SQLite ContextCleanup: detach handlers in finally so Dispose throw can't
leak event subscriptions onto subsequent receives.
- SQLite ReceiveMessage: wrap context.SetMessageAndHeaders / Set / notification
injection in try/catch — release held connection + transaction on throw to
prevent resource leak when cleanup delegates never get a chance to fire.
- SqLiteConnectionState: replace bool Completed with Interlocked-managed int
flag; MarkCompleted returns true on first call so commit/rollback paths use
an atomic CAS gate instead of check-then-act.
- SqLiteExternalDbNameExtractor: ArgumentNullException guard on connection
parameter.
Test fixes:
- SqLiteRelationalWorkerNotificationRegistrationTests: replace hard-coded
"ADMIN" queue name with "q" + Guid.NewGuid().ToString("N") so parallel
runs do not collide on a shared queue identity.
- SqLiteExternalDbNameExtractorTests: strengthen the null-DataSource case
to Assert.AreEqual(string.Empty) (was IsNotNull). Add a null-arg test
covering the new ArgumentNullException guard.
- {SqlServer,PostgreSql,SQLite}{Inbox,Outbox}IntegrationTestBase.QueueScope
+ SqLiteOutbox ProducerScope: Interlocked dispose guard so a duplicate
Dispose() (test teardown + using-block) no longer double-removes the
queue or disposes already-disposed containers.
Terminology:
- Tx -> transaction across SQLite ReceiveMessage docs, ReceiveMessageQueryHandler
+ SqLiteMessageQueueReceive comments, SqLiteMessageQueueSharedInit producer-
wiring comments, and SqlServerInboxIntegrationTestBase comments. Repo
convention is the full word (CLAUDE.md "No Tx abbreviation" lesson).
Catch-all clarification (not narrowing): The optionsFactory.Create() try/catch
in the IWorkerNotification registration lambda stays broad. Narrowing to
SimpleInjector.ActivationException broke QueueCreatorTests on SqlServer + PG
because the downstream SqlException is what those tests assert on, and the
broader catch is what lets that exception surface from its real source rather
than getting wrapped by a resolution-time failure here. Comment now documents
the contract explicitly so the next reader does not retry the narrowing.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Public interface in Transport.RelationalDatabase namespace that extends IWorkerNotification with a single read-only DbTransaction property. Enables the transactional inbox pattern via capability-cast. Full XML documentation on interface and member. Zero new ADO.NET provider deps. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five MSTest reflection-based assertions locking the public shape of IRelationalWorkerNotification: visibility, IWorkerNotification inheritance, DbTransaction property type, read-only accessor, and declared-member count. Pure MSTest assertions — no FluentAssertions, NSubstitute, or AutoFixture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduces the SqlServer inbox-pattern implementation of IRelationalWorkerNotification. Subclasses WorkerNotification, exposes ConnectionHolder (post-construction injection seam) and delegates Transaction to ConnectionHolder?.Transaction with null-safe upcast. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…on hold-transaction option Inserts three Register calls in SqlServerMessageQueueInit.RegisterImplementations after the outbox-pattern block. Both concrete notification classes are pre-registered as transient so the factory delegate can resolve either. The IWorkerNotification binding branches on EnableHoldTransactionUntilMessageCommitted: true returns SqlServerRelationalWorkerNotification (implements IRelationalWorkerNotification), false returns plain WorkerNotification. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…options load failure Adopt the IBaseTransportOptions try/catch pattern (line ~110 of the same file): at container.Verify() / early-resolution time the options factory may throw a SqlException trying to read persisted options before a real DB is reachable. Without the guard, an ActivationException wraps the SqlException and existing QueueCreatorTests that expected the bare SqlException at consumer/producer create time started failing (6 tests). Fall back to `EnableHoldTransactionUntilMessageCommitted = false` on options-load failure — the safe non-relational path. The capability-cast contract is preserved: real production callers that mean to use the inbox path will have options loaded before notification resolution, and the relational impl will be selected. Also drop the now-redundant `container.Register<WorkerNotification>(LifeStyles.Transient)` self-registration: WorkerNotification is already registered by the core (ComponentRegistration.cs:217) and is auto-resolvable as a concrete type via container.GetInstance<WorkerNotification>() without a separate self-registration in our override block. Removing it avoids a duplicate-registration footgun if AllowOverridingRegistrations ever changes. Source/DotNetWorkQueue.Transport.SqlServer.Tests now 156/156 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cation from receive path Injects the per-message ConnectionHolder into SqlServerRelationalWorkerNotification inside GetConnectionAndSetOnContext so user handlers can access the active dequeue transaction via the IRelationalWorkerNotification capability cast. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… tests Six NSubstitute-mocked tests covering the new class shape: 1. Constructor_Passes_Args_To_Base 2. Transaction_Returns_Null_When_ConnectionHolder_Not_Set 3. Transaction_Returns_Null_When_ConnectionHolder_Transaction_Is_Null 4. Transaction_Returns_Underlying_Transaction_When_Set 5. Cast_To_IRelationalWorkerNotification_Succeeds 6. Plain_WorkerNotification_Does_Not_Implement_IRelationalWorkerNotification Test 4 documents an NSubstitute limitation: SqlTransaction is sealed, so the mock cannot proxy it; the test asserts the holder-assignment path runs without throwing, relying on Test 3 to prove the null-transaction delegation. MSTest 3.x assertions throughout (Assert.IsTrue/IsFalse/AreSame/IsNull). Mock IConnectionHolder<SqlConnection, SqlTransaction, SqlCommand> via interface; mock TransportConfigurationReceive's 3 ctor deps (IConnectionInformation, IQueueDelayFactory, IRetryDelayFactory) via NSubstitute. Test count delta: 156 -> 162. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…elational notification Two tests proving PROJECT.md §Success Criteria #2 — the capability cast follows the EnableHoldTransactionUntilMessageCommitted option: - Resolves_Relational_When_HoldTransaction_Enabled -> cast succeeds - Resolves_NonRelational_When_HoldTransaction_Disabled -> cast fails Test seam: QueueContainer<SqlServerMessageQueueInit>(registerService, setOptions) overrides ITransportOptionsFactory with an NSubstitute-mocked factory returning a stub SqlServerMessageQueueTransportOptions with the desired option value, then calls container.GetInstance<IWorkerNotification>() inside the setOptions callback. The CreateConsumer call afterward may throw on the fake connection string during downstream resolution; the smoke only needs the IWorkerNotification cast from inside the setOptions callback to verify the factory delegate branched correctly. Test count delta: 162 -> 164 (156 baseline + 6 contract + 2 smoke). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mitation) Rename Transaction_Returns_Underlying_Transaction_When_Set -> ConnectionHolder_PropertySet_Does_Not_Throw to match what the assertion actually proves. Test name previously implied non-null-return coverage but the assertion fell back to property-set-succeeded due to SqlTransaction being sealed (NSubstitute cannot proxy it). Also tighten the test body: drop the unused DbTransaction substitute and assert reference-equality on the round-tripped ConnectionHolder. Full non-null-transaction coverage lands in Phase 7 SqlServer integration tests where a real SqlTransaction is available. Per SIMPLIFICATION-3 L1 finding. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PG mirror of Phase 3's SqlServerRelationalWorkerNotification with Npgsql substituted for SqlClient. Internal class subclassing WorkerNotification + implementing IRelationalWorkerNotification. Settable ConnectionHolder property for receive-path post-construction injection (PLAN-2.1 sets it via pattern-match). Transaction delegates to ConnectionHolder?.Transaction; returns null when option is false and the receive path skips the pattern-match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on hold-transaction option (PG) Factory-delegate registration in PostgreSQLMessageQueueInit.RegisterImplementations between the outbox RegisterConditional block (line 74) and the //**all general registrations (line 76). The IWorkerNotification binding branches on PostgreSqlMessageQueueTransportOptions.EnableHoldTransactionUntilMessageCommitted: option=true returns PostgreSqlRelationalWorkerNotification (which implements IRelationalWorkerNotification, allowing the user handler to cast and join the library transaction); option=false returns plain WorkerNotification (capability cast cleanly fails per PROJECT.md SS Functional New Public API). try/catch fallback to false on options-load failure baked in from the outset (Phase 3 lesson 1 — mirrors IBaseTransportOptions precedent at the same file line ~99). No regression in 143 existing PG unit tests. Added `using DotNetWorkQueue.Queue;` for WorkerNotification base type — Phase 3 SqlServer init had this already; PG was missing it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cation from receive path (PG) PG mirror of Phase 3 PLAN-2.1. Inside GetConnectionAndSetOnContext, after the existing context.Set / Commit-Rollback-Cleanup wiring and just before `return connection;`, add an `is PostgreSqlRelationalWorkerNotification` pattern-match that injects the per-message ConnectionHolder. When EnableHoldTransactionUntilMessageCommitted=true, the factory delegate in PostgreSQLMessageQueueInit returns the relational class; the pattern-match succeeds and the setter fires. When option=false, the factory returns plain WorkerNotification; the pattern-match fails cleanly (no-op). 143/143 existing PG tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t tests
Six NSubstitute-mocked tests mirroring Phase 3 PLAN-2.2 Task 1 with Npgsql substituted:
1. Constructor_Passes_Args_To_Base
2. Transaction_Returns_Null_When_ConnectionHolder_Not_Set
3. Transaction_Returns_Null_When_ConnectionHolder_Transaction_Is_Null
4. ConnectionHolder_PropertySet_Does_Not_Throw (named correctly from outset
per Phase 3 lesson 4 — NpgsqlTransaction is sealed; NSubstitute can't proxy it;
full non-null-return coverage deferred to Phase 7 PG integration tests)
5. Cast_To_IRelationalWorkerNotification_Succeeds
6. Plain_WorkerNotification_Does_Not_Implement_IRelationalWorkerNotification
Test count delta: 143 -> 149.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…elational notification (PG) Two tests proving PROJECT.md SS Success Criteria #2 — the capability cast follows the EnableHoldTransactionUntilMessageCommitted option on the PG transport: - Resolves_Relational_When_HoldTransaction_Enabled -> cast succeeds - Resolves_NonRelational_When_HoldTransaction_Disabled -> cast fails Test seam: QueueContainer<PostgreSqlMessageQueueInit>(registerService, setOptions) overrides ITransportOptionsFactory with an NSubstitute-mocked factory returning a stub PostgreSqlMessageQueueTransportOptions with the desired option value, then calls container.GetInstance<IWorkerNotification>() inside the setOptions callback. In-flight fix: filename uses `PostgreSQL` (all-caps) but the type is `PostgreSqlMessageQueueInit` (lowercase q) per the existing PG type-naming convention. Test count delta: 149 -> 151. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r queue + HandleExternalTransaction forks
Phase 5 PLAN-2.2 — full SQLite outbox sweep mirroring SqlServer/PG outbox milestone
patterns with SQLite-specific symmetric path canonicalization.
New types:
- SqLiteExternalDbNameExtractor (Basic/, internal-ish): IExternalDbNameExtractor.
Returns conn.DataSource with :memory: short-circuit + Path.GetFullPath +
ToUpperInvariant (symmetric normalization per spike SS3).
- SqliteNormalizedConnectionInformation (root assembly, public): wraps
SqliteConnectionInformation; overrides Container property with identical
canonicalization. Both sides of the ExternalTransactionValidator's
StringComparison.Ordinal compare get matching upper-cased canonical paths;
achieves OrdinalIgnoreCase semantics under the strict comparator.
- SqLiteRelationalProducerQueue<T> (Basic/, public sealed): mirrors
SqlServerRelationalProducerQueue<T> with SQLite types. Validates caller's
transaction at the producer surface; dispatches RelationalSendMessageCommand
to the existing SQLite send handlers (sync, async, batch).
Modifications:
- SqLiteMessageQueueSharedInit:
- Registers IExternalDbNameExtractor + ExternalTransactionValidator + 3x
RegisterConditional(IProducerQueue<>, IRelationalProducerQueue<>,
RelationalProducerQueue<>) -> SqLiteRelationalProducerQueue<>.
- Swaps IConnectionInformation registration to use the new normalized wrapper.
- SendMessageCommandHandler.Handle: forks to HandleExternalTransaction when
command.ExternalTransaction != null. Fork reuses caller's IDbConnection +
IDbTransaction; never Commits, Rolls Back, Disposes, or Closes them.
- SendMessageCommandHandlerAsync.HandleAsync: same fork pattern with async
primitives (await _readerAsync.ExecuteNonQueryAsync etc.).
Test housekeeping:
- Deleted SqliteProducerDoesNotImplementRelationalTests.cs — asserted SQLite
did NOT have outbox surface, which Phase 5 explicitly reverses per CONTEXT-5
SS3a. PLAN-3.2 adds positive-path coverage to replace it.
Verification:
- Release build clean (0 errors, 14 NU1902 pre-existing).
- SQLite tests 141/141 (was 142; -1 for the deprecated negative-path test).
- No (SqliteConnection)/(SqliteTransaction) sealed casts — handlers operate
on IDbConnection/IDbTransaction interfaces.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tilMessageCommitted backing
Phase 5 PLAN-3.1 inbox unit tests (6 contract + 2 option-driven SimpleInjector smoke).
Two architectural fixes surfaced by writing the smoke tests:
1. SqLiteRelationalWorkerNotification refactored to match Phase 3/4 SqlServer/PG
pattern: removed IMessageContext + SqLiteHeaders ctor deps; added settable
ConnectionState property. The receive path (ReceiveMessage.GetMessage) now
pattern-matches `is SqLiteRelationalWorkerNotification rn` and sets
rn.ConnectionState alongside the context.Set call (Phase 3 lesson 3 pattern).
Original design (ctor-injected IMessageContext) broke at container.Verify
time because IMessageContext is per-message scoped and not resolvable
standalone during container build.
2. SQLiteMessageQueueTransportOptions.EnableHoldTransactionUntilMessageCommitted
property had getter hardcoded `false` and setter as no-op. Refactored to
back with private bool field. RESEARCH SS2 verdict missed this — the
"unread option" finding was correct but the deeper issue was that even
if read, the value was always false. PLAN-1.1 / PLAN-2.1's wiring would
have been functionally dead without this fix. Caught by smoke tests
(Resolves_Relational_When_HoldTransaction_Enabled expected option=true
to resolve relational; failed because getter returned false regardless).
Test additions:
- SqLiteRelationalWorkerNotificationTests.cs (6 tests, mirrors Phase 3/4
contract tests with SqLiteConnectionState substitution + Phase 3 lesson 4
naming for the sealed-type limitation).
- SqLiteRelationalWorkerNotificationRegistrationTests.cs (2 option-driven
SimpleInjector smoke tests; uses concrete StubOptionsFactory instead of
NSubstitute for ITransportOptionsFactory to avoid the Phase 5 stub
cast-failure rabbit hole).
149/149 SQLite tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 5 PLAN-3.2 outbox unit tests (4 extractor tests covering spike SS3 semantics).
- Extract_Returns_MemoryLiteral_For_MemorySource — :memory: short-circuit
- Extract_Returns_Canonicalized_UpperCased_Path_For_File_Source — Path.GetFullPath
+ ToUpperInvariant
- Extract_Memory_Literal_Comparison_Is_Case_Sensitive — :Memory: (capital M) is NOT
the SQLite keyword
- Extract_Returns_Empty_For_Null_DataSource — null DataSource graceful handling
Bug surfaced + fixed: Path.GetFullPath("") throws ArgumentException. Both the
extractor and the SqliteNormalizedConnectionInformation wrapper now return
string.Empty for null-or-empty DataSource (handled before Path.GetFullPath).
Symmetric normalization preserved.
Note on PLAN-3.2 scope: the original plan also included HandleExternalTransaction
fork tests (PROJECT.md SS Success Criteria #8 "zero Commit/Rollback/Dispose/Close
on caller tx"). Those tests require mocking 4-5 dependencies and the fork bodies
are already verified by structural mirror to the SqlServer/PG equivalents (which
have integration test coverage). The fork-test scope is deferred to Phase 7
integration tests which exercise the real-DB caller-tx path end-to-end.
153/153 SQLite tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…IRelationalWorkerNotification Extends the existing outbox-milestone negative-path tests (one per non-relational transport) with a second [TestMethod] asserting the inbox-pattern IRelationalWorkerNotification interface is NOT implemented on Memory/Redis/LiteDb. - Memory: MemoryProducerDoesNotImplementRelationalTests.cs (+1 test method) - Redis: RedisProducerDoesNotImplementRelationalTests.cs (+1 test method) - LiteDb: LiteDbProducerDoesNotImplementRelationalTests.cs (+1 test method) Each new test: - Type-system check: IsAssignableFrom returns false for core WorkerNotification - Assembly scan: no type in the transport assembly implements IRelationalWorkerNotification PROJECT.md SS Success Criteria #3 satisfied: capability cast cleanly fails on non-relational transports. Pattern: extended existing files rather than creating new ones; cohesive "negative-path coverage" stays in one file per transport. All 6 new tests pass (2 each × 3 transports). Source-level invariant grep gate also clean: zero references to IRelationalWorkerNotification or IRelationalProducerQueue in the 3 transport source dirs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 inbox integration tests against live SQL Server (sync + async × commit + rollback + atomic visibility + option-false negative path). Capability cast pattern: handler casts IWorkerNotification to IRelationalWorkerNotification and writes business row on the active dequeue transaction; library commits or rolls back both atomically. Files: - SqlServerInboxIntegrationTestBase.cs (queue + business-table helpers, cross-connection assertion polling, ActivityListener registration) - SqlServerInboxSyncHandlerTests.cs (2 sync tests) - SqlServerInboxAsyncHandlerTests.cs (2 async tests) - SqlServerInboxAtomicVisibilityTests.cs (2 cross-connection visibility tests) - SqlServerInboxOptionFalseTests.cs (2 option=false negative-path tests) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 inbox integration tests against live PostgreSQL (sync + async × commit + rollback + atomic visibility + option-false negative path). PG mirror of the SqlServer suite with Npgsql substituted and PG identifier casing applied (unquoted identifiers fold to lowercase). Files: - PostgreSqlInboxIntegrationTestBase.cs - PostgreSqlInboxSyncHandlerTests.cs (2 sync tests) - PostgreSqlInboxAsyncHandlerTests.cs (2 async tests) - PostgreSqlInboxAtomicVisibilityTests.cs (2 cross-connection visibility tests) - PostgreSqlInboxOptionFalseTests.cs (2 option=false negative-path tests) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 inbox integration tests against live SQLite (sync + async × commit + rollback + atomic visibility + option-false negative path). First real-DB exercise of the Phase 5 SQLite hold-transaction implementation. Per-test file-backed SQLite database via IntegrationConnectionInfo with WAL journaling — separate verification reader connections do not block the inbox transaction's write lock. Uses System.Data.SQLite (matching the repo's existing SQLite consumer integration tests). Files: - SqLiteInboxIntegrationTestBase.cs - SqLiteInboxSyncHandlerTests.cs (2 sync tests) - SqLiteInboxAsyncHandlerTests.cs (2 async tests) - SqLiteInboxAtomicVisibilityTests.cs (2 cross-connection visibility tests) - SqLiteInboxOptionFalseTests.cs (2 option=false negative-path tests) - .shipyard/phases/7/results/SUMMARY-1.3.md (Phase 8 doc-input stub) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
12 outbox integration tests against live SQLite: - Send (sync + async × commit + rollback) = 4 tests - Batch (sync + async × commit + rollback) = 4 tests - AdditionalMessageData round-trip (priority + correlation ID) = 1 test - Validation (cross-DB file-path mismatch + closed connection) = 2 tests - Retry-bypass (committed-transaction → single-attempt failure) = 1 test Each Send + Batch + AdditionalData test asserts PROJECT.md §SC #8 via the AssertCallerResourcesUnmutated behavioral pin: caller's connection is still ConnectionState.Open and caller's transaction.Connection is non-null after Send returns, and the subsequent transaction.Commit() in each commit-path test succeeds (which proves the producer did not Commit/Rollback/Dispose/Close). Per-test file-backed SQLite via IntegrationConnectionInfo with WAL journaling. System.Data.SQLite (matching repo conventions). Files: - SqLiteOutboxIntegrationTestBase.cs - SqLiteOutboxSendTests.cs (2 tests) - SqLiteOutboxSendAsyncTests.cs (2 tests) - SqLiteOutboxBatchTests.cs (4 tests) - SqLiteOutboxAdditionalDataTests.cs (1 test) - SqLiteOutboxValidationTests.cs (2 tests) - SqLiteOutboxRetryBypassTests.cs (1 test) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…uirement) Queue validation rejects EnableStatus=true when EnableHoldTransactionUntilMessageCommitted=true with the diagnostic "[EnableStatus] must be false when using transactions. The status table may still be used." The inbox bases were copied from the outbox bases, which validly set EnableStatus=true because the outbox path runs with hold-tx OFF. The 6 hold-tx inbox tests per transport failed at queue-creation time with the above error; the 2 option-false tests passed because their hold-tx flag is false. Fix: set EnableStatus=false unconditionally in all three inbox bases. Status tracking is not required for any inbox-test assertion; consistency in both modes is simpler than branching on the enableHoldTransaction parameter. Files: - Source/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxIntegrationTestBase.cs - Source/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxIntegrationTestBase.cs - Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Inbox/SqLiteInboxIntegrationTestBase.cs Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #146 added .shipyard/ to .gitignore and removed all tracked files, but one stray phase-7 SUMMARY slipped through. Drop it from tracking; the gitignore rule keeps it out of future commits. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Production fixes:
- SQLite ContextCleanup: detach handlers in finally so Dispose throw can't
leak event subscriptions onto subsequent receives.
- SQLite ReceiveMessage: wrap context.SetMessageAndHeaders / Set / notification
injection in try/catch — release held connection + transaction on throw to
prevent resource leak when cleanup delegates never get a chance to fire.
- SqLiteConnectionState: replace bool Completed with Interlocked-managed int
flag; MarkCompleted returns true on first call so commit/rollback paths use
an atomic CAS gate instead of check-then-act.
- SqLiteExternalDbNameExtractor: ArgumentNullException guard on connection
parameter.
Test fixes:
- SqLiteRelationalWorkerNotificationRegistrationTests: replace hard-coded
"ADMIN" queue name with "q" + Guid.NewGuid().ToString("N") so parallel
runs do not collide on a shared queue identity.
- SqLiteExternalDbNameExtractorTests: strengthen the null-DataSource case
to Assert.AreEqual(string.Empty) (was IsNotNull). Add a null-arg test
covering the new ArgumentNullException guard.
- {SqlServer,PostgreSql,SQLite}{Inbox,Outbox}IntegrationTestBase.QueueScope
+ SqLiteOutbox ProducerScope: Interlocked dispose guard so a duplicate
Dispose() (test teardown + using-block) no longer double-removes the
queue or disposes already-disposed containers.
Terminology:
- Tx -> transaction across SQLite ReceiveMessage docs, ReceiveMessageQueryHandler
+ SqLiteMessageQueueReceive comments, SqLiteMessageQueueSharedInit producer-
wiring comments, and SqlServerInboxIntegrationTestBase comments. Repo
convention is the full word (CLAUDE.md "No Tx abbreviation" lesson).
Catch-all clarification (not narrowing): The optionsFactory.Create() try/catch
in the IWorkerNotification registration lambda stays broad. Narrowing to
SimpleInjector.ActivationException broke QueueCreatorTests on SqlServer + PG
because the downstream SqlException is what those tests assert on, and the
broader catch is what lets that exception surface from its real source rather
than getting wrapped by a resolution-time failure here. Comment now documents
the contract explicitly so the next reader does not retry the narrowing.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… transient IWorkerNotification resolution Phase 5 SQLite inbox tests fail under live DB with NullReferenceException in InsertBusinessRowOnInboxTransaction: relational.Transaction is null even though the IRelationalWorkerNotification cast succeeds in the user handler. Root cause: IWorkerNotification is registered LifeStyles.Transient and the factory delegate (SqLiteMessageQueueSharedInit.cs:154) calls container.GetInstance<SqLiteRelationalWorkerNotification>() per invocation, also Transient — so each MessageContext can construct a different SqLiteRelationalWorkerNotification instance than the one observed at the receive-path pattern-match. Setting ConnectionState on the receive-path instance has no observable effect on the handler's instance, and Transaction returns null. SqlServer/PostgreSQL avoid this issue because their typed IConnectionHolder abstraction is stored on the IMessageContext via a singleton header key and the notification reads it back via the holder reference. SQLite uses the un-typed IDbConnection/IDbTransaction interfaces throughout, so introducing a parallel typed holder would have been an unnecessary abstraction. Fix: back SqLiteRelationalWorkerNotification.ConnectionState (and Transaction) with a static AsyncLocal<SqLiteConnectionState>. Every instance reads/writes the SAME per-async-flow slot, so the instance-identity gap disappears. The receive path now calls SqLiteRelationalWorkerNotification.SetCurrent(state) directly (no pattern-match needed); Context_Cleanup calls ClearCurrent so the next message handled by the same worker thread does not observe stale state. Test isolation: SqLiteRelationalWorkerNotificationTests gets [TestInitialize] + [TestCleanup] hooks calling ClearCurrent() so MSTest's thread-pool reuse of execution context cannot leak state between [TestMethod]s. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
blehnen
force-pushed
the
phase-2-inbox-foundation
branch
from
15f63b5 to
676f715
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
Source/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxAsyncHandlerTests.cs (1)
133-142: ⚡ Quick winSignal
handlerInvokedin afinallyblock for rollback handler.Right now, unexpected failures before Line 140 can cause a 30s timeout instead of surfacing the real exception.
♻️ Proposed change
consumer.Start<FakeMessage>((message, workerNotification) => { - if (workerNotification is IRelationalWorkerNotification relational) - { - castSucceeded = true; - InsertBusinessRowOnInboxTransaction(relational.Transaction, businessTable, 1, "rollback-async"); - } - handlerInvoked.Set(); - throw new InvalidOperationException("intentional throw — should roll back inbox transaction"); + try + { + if (workerNotification is IRelationalWorkerNotification relational) + { + castSucceeded = true; + InsertBusinessRowOnInboxTransaction(relational.Transaction, businessTable, 1, "rollback-async"); + } + throw new InvalidOperationException("intentional throw — should roll back inbox transaction"); + } + finally + { + handlerInvoked.Set(); + } }, null);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxAsyncHandlerTests.cs` around lines 133 - 142, The handler passed to consumer.Start<FakeMessage> may exit early on exceptions and never call handlerInvoked.Set(), causing long timeouts; update the anonymous handler so handlerInvoked.Set() is executed from a finally block (i.e., wrap the current body in try { ... } finally { handlerInvoked.Set(); }) while preserving the existing cast to IRelationalWorkerNotification, the call to InsertBusinessRowOnInboxTransaction(relational.Transaction, businessTable, 1, "rollback-async") and the intentional throw of InvalidOperationException so the test still verifies rollback behavior.Source/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxAtomicVisibilityTests.cs (1)
65-70: ⚡ Quick winUse
try/finallyforhandlerInvoked.Set()in both handlers.Both handlers can timeout for 30s if an exception happens before
Set(), which makes failures harder to diagnose.♻️ Proposed change
consumer.Start<FakeMessage>((message, workerNotification) => { - var relational = (IRelationalWorkerNotification)workerNotification; - InsertBusinessRowOnInboxTransaction(relational.Transaction, businessTable, 42, "atomic-commit"); - handlerInvoked.Set(); + try + { + var relational = (IRelationalWorkerNotification)workerNotification; + InsertBusinessRowOnInboxTransaction(relational.Transaction, businessTable, 42, "atomic-commit"); + } + finally + { + handlerInvoked.Set(); + } }, null); ... consumer.Start<FakeMessage>((message, workerNotification) => { - var relational = (IRelationalWorkerNotification)workerNotification; - InsertBusinessRowOnInboxTransaction(relational.Transaction, businessTable, 99, "atomic-rollback"); - handlerInvoked.Set(); - throw new InvalidOperationException("intentional throw — should roll back inbox transaction"); + try + { + var relational = (IRelationalWorkerNotification)workerNotification; + InsertBusinessRowOnInboxTransaction(relational.Transaction, businessTable, 99, "atomic-rollback"); + throw new InvalidOperationException("intentional throw — should roll back inbox transaction"); + } + finally + { + handlerInvoked.Set(); + } }, null);Also applies to: 120-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxAtomicVisibilityTests.cs` around lines 65 - 70, The two consumer handlers registered via consumer.Start<FakeMessage>(...) currently call handlerInvoked.Set() at the end of the body and can hang if an exception occurs; wrap each handler body in a try/finally and move handlerInvoked.Set() into the finally block so it always runs (for the handler that calls InsertBusinessRowOnInboxTransaction(relational.Transaction, businessTable, 42, "atomic-commit") and the other handler around lines 120-126), preserving existing logic in the try and leaving finally only to call handlerInvoked.Set().Source/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxSyncHandlerTests.cs (1)
131-140: ⚡ Quick winGuarantee rollback handler signaling with
finally.An exception before Line 138 can cause a timeout-based failure rather than the underlying error.
♻️ Proposed change
consumer.Start<FakeMessage>((message, workerNotification) => { - if (workerNotification is IRelationalWorkerNotification relational) - { - castSucceeded = true; - InsertBusinessRowOnInboxTransaction(relational.Transaction, businessTable, 1, "rollback"); - } - handlerInvoked.Set(); - throw new InvalidOperationException("intentional throw — should roll back inbox transaction"); + try + { + if (workerNotification is IRelationalWorkerNotification relational) + { + castSucceeded = true; + InsertBusinessRowOnInboxTransaction(relational.Transaction, businessTable, 1, "rollback"); + } + throw new InvalidOperationException("intentional throw — should roll back inbox transaction"); + } + finally + { + handlerInvoked.Set(); + } }, null);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxSyncHandlerTests.cs` around lines 131 - 140, The test's consumer.Start<FakeMessage> callback can throw before handlerInvoked.Set() is reached, causing spurious timeouts; to fix it, restructure the anonymous callback so that workerNotification casting, InsertBusinessRowOnInboxTransaction(relational.Transaction, businessTable, 1, "rollback") and the InvalidOperationException remain but ensure handlerInvoked.Set() is called from a finally block (after the cast/insert/throw) so the handler always signals completion; locate the lambda passed to consumer.Start<FakeMessage> and move the handlerInvoked.Set() into a finally section while preserving castSucceeded assignment when relational is IRelationalWorkerNotification.Source/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteMessageQueueSharedInit.cs (1)
160-177: 💤 Low valueThe broad exception swallowing is documented but worth a brief log.
The catch-all at line 170 is intentional per the inline documentation (early container verification can throw various exceptions), and the fallback is safe. However, silently defaulting to
WorkerNotificationcould make debugging configuration issues harder. Consider adding a debug-level log when the catch fires so operators can trace why inbox capability was disabled.💡 Optional: Add debug logging in catch block
catch { + // Optional: log for observability when falling back + // _log?.LogDebug("Options resolution failed during IWorkerNotification factory; falling back to non-relational WorkerNotification"); holdTransaction = false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteMessageQueueSharedInit.cs` around lines 160 - 177, Change the empty catch in the container.Register block that decides IWorkerNotification to capture the exception (e.g., catch(Exception ex)) and emit a debug-level log containing the exception and a brief message explaining the fallback to WorkerNotification; keep the fallback behavior intact (use SqLiteRelationalWorkerNotification when EnableHoldTransactionUntilMessageCommitted on SqLiteMessageQueueTransportOptions is true, otherwise WorkerNotification). Locate the registration around container.Register<IWorkerNotification> which calls ITransportOptionsFactory.Create(), and add the debug log inside the catch before setting holdTransaction = false so operators can trace why the inbox capability was disabled.Source/DotNetWorkQueue.Transport.SqlServer/Basic/SQLServerMessageQueueReceive.cs (1)
186-189: ⚡ Quick winClear
ConnectionHolderduring cleanup to avoid stale transaction state.After cleanup disposes the per-message connection, the relational notification still holds the old holder reference. Reset it to
nullin cleanup to prevent accidental post-cleanup access to disposed state.♻️ Proposed fix
private void ContextCleanup(IMessageContext context, IConnectionHolder<SqlConnection, SqlTransaction, SqlCommand> connectionHolder) { if (!_configuration.Options().EnableHoldTransactionUntilMessageCommitted) { context.Commit -= ContextOnCommit; context.Rollback -= ContextOnRollback; } else { context.Commit -= ContextOnCommitTransaction; context.Rollback -= ContextOnRollbackTransaction; } context.Cleanup -= Context_Cleanup; + if (context.WorkerNotification is SqlServerRelationalWorkerNotification relationalNotification) + { + relationalNotification.ConnectionHolder = null; + } _disposeConnection(connectionHolder); }Also applies to: 273-275
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/DotNetWorkQueue.Transport.SqlServer/Basic/SQLServerMessageQueueReceive.cs` around lines 186 - 189, In SQLServerMessageQueueReceive, ensure you clear the per-message relational notification's ConnectionHolder during cleanup: when checking context.WorkerNotification for a SqlServerRelationalWorkerNotification (the same pattern used at the earlier block), set relationalNotification.ConnectionHolder = null as part of disposal to avoid leaving a stale reference to the disposed connection; apply the same change to the other cleanup location that mirrors lines 273-275 so both cleanup paths null out ConnectionHolder.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Source/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxRetryBypassTests.cs`:
- Around line 74-75: Replace the loose null check on the caught exception in
SqLiteOutboxRetryBypassTests (the Assert.IsNotNull(caught, "Caller-transaction
Send must throw on completed transaction.");) with a transaction-specific
assertion: assert the exception is the expected type (e.g.,
InvalidOperationException or a TransactionException used by the transport) and
that its Message contains a transaction/completed-transaction indicator (e.g.,
contains "transaction" or "completed"). Use Assert.IsInstanceOfType(or
Assert.IsInstanceOf) on the caught exception and Assert.IsTrue on
caught.Message.Contains(...) so the test only passes for the intended
completed-transaction failure mode.
In
`@Source/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteRelationalProducerQueue.cs`:
- Around line 136-146: The failure path recreates AdditionalMessageData twice
causing mismatched CorrelationId; in the foreach over messages in
SqLiteRelationalProducerQueue (the loop that calls SendOne), capture
m.MessageData ?? new AdditionalMessageData() into a local variable (e.g.,
msgData) before calling SendOne and use that same msgData inside the catch when
constructing the QueueOutputMessage via _sentMessageFactory.Create(...,
msgData.CorrelationId); do the same change for the other similar block (lines
160-171) so the same IAdditionalMessageData instance is reused for both the
SendOne call and the error QueueOutputMessage.
---
Nitpick comments:
In
`@Source/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxAsyncHandlerTests.cs`:
- Around line 133-142: The handler passed to consumer.Start<FakeMessage> may
exit early on exceptions and never call handlerInvoked.Set(), causing long
timeouts; update the anonymous handler so handlerInvoked.Set() is executed from
a finally block (i.e., wrap the current body in try { ... } finally {
handlerInvoked.Set(); }) while preserving the existing cast to
IRelationalWorkerNotification, the call to
InsertBusinessRowOnInboxTransaction(relational.Transaction, businessTable, 1,
"rollback-async") and the intentional throw of InvalidOperationException so the
test still verifies rollback behavior.
In
`@Source/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxAtomicVisibilityTests.cs`:
- Around line 65-70: The two consumer handlers registered via
consumer.Start<FakeMessage>(...) currently call handlerInvoked.Set() at the end
of the body and can hang if an exception occurs; wrap each handler body in a
try/finally and move handlerInvoked.Set() into the finally block so it always
runs (for the handler that calls
InsertBusinessRowOnInboxTransaction(relational.Transaction, businessTable, 42,
"atomic-commit") and the other handler around lines 120-126), preserving
existing logic in the try and leaving finally only to call handlerInvoked.Set().
In
`@Source/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxSyncHandlerTests.cs`:
- Around line 131-140: The test's consumer.Start<FakeMessage> callback can throw
before handlerInvoked.Set() is reached, causing spurious timeouts; to fix it,
restructure the anonymous callback so that workerNotification casting,
InsertBusinessRowOnInboxTransaction(relational.Transaction, businessTable, 1,
"rollback") and the InvalidOperationException remain but ensure
handlerInvoked.Set() is called from a finally block (after the
cast/insert/throw) so the handler always signals completion; locate the lambda
passed to consumer.Start<FakeMessage> and move the handlerInvoked.Set() into a
finally section while preserving castSucceeded assignment when relational is
IRelationalWorkerNotification.
In
`@Source/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteMessageQueueSharedInit.cs`:
- Around line 160-177: Change the empty catch in the container.Register block
that decides IWorkerNotification to capture the exception (e.g., catch(Exception
ex)) and emit a debug-level log containing the exception and a brief message
explaining the fallback to WorkerNotification; keep the fallback behavior intact
(use SqLiteRelationalWorkerNotification when
EnableHoldTransactionUntilMessageCommitted on SqLiteMessageQueueTransportOptions
is true, otherwise WorkerNotification). Locate the registration around
container.Register<IWorkerNotification> which calls
ITransportOptionsFactory.Create(), and add the debug log inside the catch before
setting holdTransaction = false so operators can trace why the inbox capability
was disabled.
In
`@Source/DotNetWorkQueue.Transport.SqlServer/Basic/SQLServerMessageQueueReceive.cs`:
- Around line 186-189: In SQLServerMessageQueueReceive, ensure you clear the
per-message relational notification's ConnectionHolder during cleanup: when
checking context.WorkerNotification for a SqlServerRelationalWorkerNotification
(the same pattern used at the earlier block), set
relationalNotification.ConnectionHolder = null as part of disposal to avoid
leaving a stale reference to the disposed connection; apply the same change to
the other cleanup location that mirrors lines 273-275 so both cleanup paths null
out ConnectionHolder.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f607759-5e39-4460-9257-a7cf29905e7b
📒 Files selected for processing (54)
Source/DotNetWorkQueue.Transport.LiteDb.Tests/Basic/LiteDbProducerDoesNotImplementRelationalTests.csSource/DotNetWorkQueue.Transport.Memory.Tests/Basic/MemoryProducerDoesNotImplementRelationalTests.csSource/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxAsyncHandlerTests.csSource/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxAtomicVisibilityTests.csSource/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxIntegrationTestBase.csSource/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxOptionFalseTests.csSource/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxSyncHandlerTests.csSource/DotNetWorkQueue.Transport.PostgreSQL.Tests/Basic/PostgreSqlRelationalWorkerNotificationRegistrationTests.csSource/DotNetWorkQueue.Transport.PostgreSQL.Tests/Basic/PostgreSqlRelationalWorkerNotificationTests.csSource/DotNetWorkQueue.Transport.PostgreSQL/Basic/PostgreSQLMessageQueueInit.csSource/DotNetWorkQueue.Transport.PostgreSQL/Basic/PostgreSQLMessageQueueReceive.csSource/DotNetWorkQueue.Transport.PostgreSQL/Basic/PostgreSqlRelationalWorkerNotification.csSource/DotNetWorkQueue.Transport.Redis.Tests/Basic/RedisProducerDoesNotImplementRelationalTests.csSource/DotNetWorkQueue.Transport.RelationalDatabase.Tests/IRelationalWorkerNotificationContractTests.csSource/DotNetWorkQueue.Transport.RelationalDatabase/IRelationalWorkerNotification.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Inbox/SqLiteInboxAsyncHandlerTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Inbox/SqLiteInboxAtomicVisibilityTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Inbox/SqLiteInboxIntegrationTestBase.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Inbox/SqLiteInboxOptionFalseTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Inbox/SqLiteInboxSyncHandlerTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxAdditionalDataTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxBatchTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxIntegrationTestBase.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxRetryBypassTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxSendAsyncTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxSendTests.csSource/DotNetWorkQueue.Transport.SQLite.Integration.Tests/Outbox/SqLiteOutboxValidationTests.csSource/DotNetWorkQueue.Transport.SQLite.Tests/Basic/SqLiteExternalDbNameExtractorTests.csSource/DotNetWorkQueue.Transport.SQLite.Tests/Basic/SqLiteRelationalWorkerNotificationRegistrationTests.csSource/DotNetWorkQueue.Transport.SQLite.Tests/Basic/SqLiteRelationalWorkerNotificationTests.csSource/DotNetWorkQueue.Transport.SQLite.Tests/Basic/SqliteProducerDoesNotImplementRelationalTests.csSource/DotNetWorkQueue.Transport.SQLite/Basic/CommandHandler/SendMessageCommandHandler.csSource/DotNetWorkQueue.Transport.SQLite/Basic/CommandHandler/SendMessageCommandHandlerAsync.csSource/DotNetWorkQueue.Transport.SQLite/Basic/Message/ReceiveMessage.csSource/DotNetWorkQueue.Transport.SQLite/Basic/QueryHandler/ReceiveMessageQueryHandler.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SQLiteMessageQueueTransportOptions.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteConnectionState.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteExternalDbNameExtractor.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteHeaders.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteMessageQueueReceive.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteMessageQueueSharedInit.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteRelationalProducerQueue.csSource/DotNetWorkQueue.Transport.SQLite/Basic/SqLiteRelationalWorkerNotification.csSource/DotNetWorkQueue.Transport.SQLite/SqliteNormalizedConnectionInformation.csSource/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxAsyncHandlerTests.csSource/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxAtomicVisibilityTests.csSource/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxIntegrationTestBase.csSource/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxOptionFalseTests.csSource/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxSyncHandlerTests.csSource/DotNetWorkQueue.Transport.SqlServer.Tests/Basic/SqlServerRelationalWorkerNotificationRegistrationTests.csSource/DotNetWorkQueue.Transport.SqlServer.Tests/Basic/SqlServerRelationalWorkerNotificationTests.csSource/DotNetWorkQueue.Transport.SqlServer/Basic/SQLServerMessageQueueInit.csSource/DotNetWorkQueue.Transport.SqlServer/Basic/SQLServerMessageQueueReceive.csSource/DotNetWorkQueue.Transport.SqlServer/Basic/SqlServerRelationalWorkerNotification.cs
💤 Files with no reviewable changes (1)
- Source/DotNetWorkQueue.Transport.SQLite.Tests/Basic/SqliteProducerDoesNotImplementRelationalTests.cs
…ead in receive Jenkins exposed two distinct issues on the inbox/outbox milestone's first Linux CI run: 1. IntegrationConnectionInfo built its file path with a literal "\\" separator (`localPath + "\\" + filename`). On Windows that produces a valid path; on Linux the "\" is a literal filename character. Result: the outbox-pattern ExternalTransactionValidator's path comparison diverges — the queue side parses Server from the connection string (preserves "\" verbatim, GetFullPath keeps it as-is, ends with ".db3") while the caller-side conn.DataSource after SQLiteConnection.Open() strips the directory and extension, then Path.GetFullPath re-prefixes the cwd, producing a duplicated path segment that can never match the queue side. Replace "\\" with Path.Combine() — separator now matches the OS and both sides canonicalize to identical absolute paths. 2. SqLiteMessageQueueReceive.ReceiveMessage (inner-class) cached the transport options via Lazy<T>(optionsFactory.Create). Lazy caches the FIRST .Value call; if that first call happens before the queue's options have been persisted to the DB (e.g., during container.Verify resolution graph walk), the Lazy is stuck on a default options instance — EnableHoldTransactionUntilMessageCommitted=false — for the lifetime of the ReceiveMessage instance. The IWorkerNotification registration lambda calls optionsFactory.Create() per-resolution and sees the persisted true value, so the consumer's WorkerNotification IS the relational variant — but the inner receive path's stale Lazy skips heldConnection/heldTransaction creation, so SqLiteRelationalWorkerNotification.SetCurrent is never called and the handler observes null Transaction. Drop the Lazy wrapper and call optionsFactory.Create() per-receive. The factory itself caches its first DB-backed read (SqLiteMessageQueueTransportOptionsFactory._options), so this is a cheap dictionary-lookup hot path — and it guarantees the receive path observes the same option value the registration lambda sees. Both fixes apply to the SQLite transport only. No production-API surface change; the inner ReceiveMessage ctor's signature is unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ath comparison Run 13 reduced the outbox-test failures from "path mismatch with duplicated NET10.0/" to just ".DB3 extension present on one side but not the other" — the Path.Combine fix in 465dfa3 resolved the separator issue. The remaining divergence is System.Data.SQLite's SQLiteConnection.DataSource property returning the bare file name without extension after Open() on Linux while the queue side's raw connection-string Server value keeps ".db3". Fix: drop the file extension on BOTH sides of the validator (SqLiteExternalDbNameExtractor reads from conn.DataSource; SqliteNormalizedConnectionInformation.Container reads from the connection-string Server) before applying Path.GetFullPath + ToUpperInvariant. Same canonicalization on both inputs → comparator sees identical strings regardless of whether the platform's SQLite provider preserves the extension on DataSource. Pattern continues to match the "string-comparator drift" lesson in CLAUDE.md from the outbox milestone PR #138: both sides apply identical normalization, comparator stays Ordinal. Unit test Extract_Returns_Canonicalized_UpperCased_Path_For_File_Source updated to expect the extension-stripped form (and the case-sensitive memory-literal variant likewise). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…qlServer Run 14 confirmed SqlServer + PostgreSQL inbox tests pass (same pattern) while SQLite alone fails. The static AsyncLocal change (676f715) was solving a non-bug — the prior session's "two instances exist" diagnosis was theoretical and never verified on Jenkins. PG and SqlServer both use property-injection on the per-instance ConnectionHolder and work on the SmartThreadPool worker threads; the AsyncLocal layer added noise without fixing anything. Revert SqLiteRelationalWorkerNotification to a plain instance property (ConnectionState) and the receive path back to pattern-match-and-set on context.WorkerNotification. The instance identity through the message lifecycle (MessageContext.ctor calls WorkerNotificationFactory.Create() once, then context.WorkerNotification returns the same backing field) is identical on all three transports — works on SqlServer/PG, will work on SQLite. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
SQLite was originally in scope alongside SqlServer + PostgreSQL for the relational-inbox + outbox milestone (PR #143). SqlServer and PostgreSQL integration tests pass cleanly on Jenkins (Linux). SQLite-specific failures persisted across 14 CI runs and three attempted fixes (Lazy-removal, AsyncLocal storage, property-injection); root cause was not isolated. To unblock the milestone, drop all SQLite-specific code + tests for the inbox/outbox pattern. SqlServer + PostgreSQL implementation, the shared IRelationalWorkerNotification interface, the per-transport negative tests (Memory/Redis/LiteDb proving the cast cleanly fails), and the cross-cutting contract tests all stay. Restored to master: - SqLiteMessageQueueReceive.cs, SqLiteMessageQueueSharedInit.cs (no inbox wiring, no SqLiteHeaders / SqLiteRelationalWorkerNotification refs) - ReceiveMessage.cs (no hold-transaction connection creation, no context.WorkerNotification pattern-match-and-set) - ReceiveMessageQueryHandler.cs (no caller-supplied lifecycle branch) - SQLiteMessageQueueTransportOptions.cs (no EnableHoldTransactionUntilMessageCommitted property) - SendMessageCommandHandler.cs + SendMessageCommandHandlerAsync.cs (no external-transaction Send overloads) - IntegrationConnectionInfo (no Path.Combine — master uses literal "\\" which is harmless on the legacy SQLite tests that pass on master) - SqliteProducerDoesNotImplementRelationalTests.cs (the negative test proving the SqLite producer doesn't implement IRelationalProducerQueue in the absence of outbox support — was deleted on this branch when outbox was added; restored) Deleted (all SQLite phase-5+ files): - SqLiteConnectionState.cs, SqLiteExternalDbNameExtractor.cs, SqLiteHeaders.cs, SqLiteRelationalProducerQueue.cs, SqLiteRelationalWorkerNotification.cs, SqliteNormalizedConnectionInformation.cs - All Inbox/ + Outbox/ integration test files - SQLite unit tests for the dropped types Verification: SqLite 142 / SqlServer 164 / PostgreSQL 151 unit tests pass. Solution builds clean. See issue #149 for resumption work on SQLite inbox/outbox. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
blehnen
changed the title
…viable PR #143 + issue #149 surfaced the structural reason hold-transaction inbox can't work on SQLite: BEGIN EXCLUSIVE-on-write blocks the worker's next dequeue while a prior message's transaction is still held. The four debugging avenues attempted in PR #143 (lazy-options removal, property injection, static AsyncLocal, factory-delegate wiring) were chasing behavioral symptoms of a structural lock contention problem. Capture the lesson so future sessions don't re-attempt them and re-target SQLite work to outbox-only (issue #149). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #143 +/- ##
==========================================
+ Coverage 89.76% 89.81% +0.05%
==========================================
Files 1000 1002 +2
Lines 29651 29703 +52
Branches 2401 2405 +4
==========================================
+ Hits 26615 26678 +63
Misses 2365 2365
+ Partials 671 660 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This was referenced May 21, 2026
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds inbox-pattern support to the SqlServer and PostgreSQL transports via the same
IRelationalWorkerNotificationshape that PROJECT.md §Functional locked in.User handler now writes business data on the active dequeue transaction via a single capability cast:
Non-relational transports (Memory, Redis, LiteDb) never implement the interface — the cast cleanly pattern-matches false and the handler can degrade gracefully or surface a discoverable error (Phase 6 negative-path coverage). The interface is only registered when
EnableHoldTransactionUntilMessageCommitted = true.SQLite inbox + outbox was originally in this milestone but is deferred to issue #149. SqlServer + PostgreSQL pass cleanly on Jenkins; SQLite had Linux-specific failures that did not isolate to a root cause within the milestone's scope. SQLite's existing tests remain untouched on this branch.
What's included
IRelationalWorkerNotificationinterface inTransport.RelationalDatabase(foundation).SqlServerRelationalWorkerNotification+ receive-path injection + factory-delegate DI).PostgreSqlRelationalWorkerNotification).ActivityListeneron[ClassInitialize]so trace decorators execute their full code paths (CLAUDE.md trace-decorator coverage lesson).PROJECT.md success criteria #4–#7 closed at the integration level for the SqlServer + PostgreSQL transports.
Why now / motivation
IRelationalProducerQueue<T>on the producer side. The inbox is the consumer-side counterpart; together they give applications full atomic dequeue + business-write + business-read + enqueue semantics on a single dequeue transaction.What's deliberately NOT in this PR
docs/inbox-pattern.md. Holds until SQLite is implemented so the user-facing docs can cover all three relational transports.Test plan
Transport.RelationalDatabase,Transport.SqlServer,Transport.PostgreSQL(Phases 2–4).Transport.Memory.Tests/Transport.Redis.Tests/Transport.LiteDb.Tests(Phase 6).Source/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/(8 new tests).Source/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/(8 new tests).🤖 Generated with Claude Code