Inbox held-transaction batch send: true multi-row insert (SQL Server + PostgreSQL) (#167)#169
Merged
Conversation
… (sync + async) Forks both SQL Server batch command handlers on RelationalSendMessageCommandBatch.ExternalTransaction: the inbox held-transaction path reuses the caller's SqlConnection/SqlTransaction via the existing ProcessChunk/ProcessChunkAsync with no Open/Begin/Commit/Rollback/Dispose/Close and no catch wrapper (failures propagate; caller owns rollback). Fork sits after GuardNoScheduledJobs so scheduled-job batches are still rejected. Standalone/outbox path unchanged. Refs #167. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (sync + async) Mirrors the SQL Server fork for PostgreSQL: forks both batch handlers on RelationalSendMessageCommandBatch.ExternalTransaction, reusing the caller's NpgsqlConnection/NpgsqlTransaction via the existing ProcessChunk/ProcessChunkAsync (unnest RETURNING + ascending-sort id recovery, _getTime already in scope) with no Open/Begin/Commit/Rollback/Dispose/Close and no catch wrapper. Fork sits after GuardNoScheduledJobs. Standalone/outbox path unchanged. Refs #167. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SqlServerRelationalProducerQueue now injects the sync/async batch handlers and dispatches one RelationalSendMessageCommandBatch (carrying the caller transaction) for the held-transaction batch overrides, replacing the per-message SendOne loop. Exceptions propagate (no per-message swallow). Build+dispatch factored into an internal DispatchBatch/DispatchBatchAsync seam so dispatch behavior is unit-testable (SqlTransaction is sealed/unmockable, so the cast guard stays covered by the non-SqlTransaction tests; handler commit/rollback behavior is integration-tested). No DI init change (SimpleInjector auto-resolves; SkipRetry bypasses the retry decorator). Refs #167. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mirrors the SQL Server producer rewire for PostgreSQL: injects the sync/async batch handlers, dispatches one RelationalSendMessageCommandBatch via internal DispatchBatch/ DispatchBatchAsync seam (exceptions propagate, no SendOne loop), keeps SendOne for single-message overrides. 13 unit tests pass. No DI init change. Refs #167. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…angelog New SqlServerInboxBatchSendTests (held-transaction queue, enableHoldTransaction:true): commit -> queue+business rows visible; rollback -> none visible; caller connection still open and usable after send; forced failure (MetaData table dropped) throws rather than swallowing; one distinct positive id per message. Changelog 0.9.42 entry for #167. Integration tests run on Jenkins / local connectionstring.txt. Refs #167. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
New PostgreSqlInboxBatchSendTests mirroring the SQL Server inbox suite (commit/rollback visibility, connection still usable, forced failure throws, one distinct id per message), plus a multi-chunk test (BatchSize=2, 5 messages -> 3 chunks) asserting the unnest/RETURNING ascending-sort id recovery yields strictly increasing ids in caller input order across chunks inside one external transaction. Runs on Jenkins / local connectionstring.txt. Refs #167. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Producers: restore empty-list short-circuit after validate/guard in both batch overrides (SQL Server + PostgreSQL) so an empty batch skips dispatch (Wave 2 review). - Integration tests: forced-failure test rolls back in a finally (rollback error can't mask the assertion), comments clarify the failure originates at the meta insert (no pre-check), and a note explains the synchronous-rollback queue-count poll (Wave 3 review). Refs #167. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Update docs/outbox-pattern.md batch sub-section: the caller-transaction batch overload (SQL Server + PostgreSQL) now performs a true multi-row insert and throws on failure (catch + rollback) instead of reporting per-message HasErrors. Note other transports throw InvalidOperationException (SQLite single-writer); standalone Send(List<T>) still uses HasErrors. Addresses documenter HIGH gap. Refs #167. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds held-transaction batch dispatch for SQL Server and PostgreSQL inbox sends. The producer queues now send batch commands through external-transaction handlers, and the tests and docs were updated to cover commit, rollback, failure, and result semantics. ChangesPostgreSQL held-transaction batch send
SQL Server held-transaction batch send
Documentation update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Source/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxBatchSendTests.cs (1)
250-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the Interlocked disposal guard for
ProducerScope.
ProducerScopeis a newIDisposable; add the same thread-safe disposal guard expected for disposable types.As per coding guidelines, “Use
Interlocked-based disposal patterns for thread-safe disposal throughout the codebase.”Proposed fix
private sealed class ProducerScope : System.IDisposable { + private int _disposed; + public QueueContainer<PostgreSqlMessageQueueInit> Creator { get; init; } public IProducerQueue<FakeMessage> Producer { get; init; } public IRelationalProducerQueue<FakeMessage> RelationalProducer { get; init; } public void Dispose() { + if (System.Threading.Interlocked.Exchange(ref _disposed, 1) == 1) + return; + Producer?.Dispose(); Creator?.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.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxBatchSendTests.cs` around lines 250 - 262, ProducerScope currently disposes its dependencies without a thread-safe guard, so update its Dispose implementation to follow the codebase’s Interlocked-based disposable pattern. Add a disposal state field to ProducerScope and use an Interlocked exchange/check in Dispose so Creator and Producer are released only once even under concurrent calls. Keep the fix localized to the ProducerScope nested class in PostgreSqlInboxBatchSendTests and mirror the same disposal style used elsewhere for disposable helpers.Source: Coding guidelines
Source/DotNetWorkQueue.Transport.SqlServer/Basic/CommandHandler/SendMessageCommandBatchHandlerAsync.cs (1)
145-160: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMirror the sync handler’s
MessageDatacontract check here.
ProcessChunkAsyncstill assumes every batch item already carries non-nullMessageData. The producer currently normalizes that upstream, but this new async branch has no fail-fast check of its own, so any direct dispatch or future bypass will crash later in the chunk loop instead of at the boundary.Suggested guard
private async Task<QueueOutputMessages> HandleExternalTransactionAsync(RelationalSendMessageCommandBatch rel) { + if (rel.Messages.Any(m => m.MessageData == null)) + { + throw new DotNetWorkQueueException( + "Batch messages must have non-null MessageData before handler dispatch."); + } + var sqlTransaction = (SqlTransaction)rel.ExternalTransaction; var sqlConn = (SqlConnection)sqlTransaction.Connection;Based on learnings: "In the batch send implementations for the PostgreSQL, SQL Server, and SQLite transports,
m.MessageDatamust be non-null when the internal batch handler runs... add an explicit guard (throw) or at least a debug assertion..."🤖 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/CommandHandler/SendMessageCommandBatchHandlerAsync.cs` around lines 145 - 160, Add the same fail-fast MessageData non-null contract check used by the sync batch handler before entering ProcessChunkAsync in HandleExternalTransactionAsync. This async SQL Server path should validate rel.Messages items (or the batch chunk) and throw or assert immediately if any message has null MessageData, rather than letting the loop fail later. Use the existing HandleExternalTransactionAsync and ProcessChunkAsync flow as the place to mirror the sync handler’s guard.Source: Learnings
Source/DotNetWorkQueue.Transport.SqlServer/Basic/CommandHandler/SendMessageCommandBatchHandler.cs (1)
152-167: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFail fast if a batch reaches this handler with null
MessageData.This new path feeds
rel.Messagesstraight intoProcessChunk, which later dereferencesm.MessageDatafor expiration, delay, metadata/status writes, and correlation IDs. The producer currently normalizes nulls before dispatch, but a direct handler call or future refactor would turn that into a late runtime failure here. A small guard/debug assertion at the handoff point would make the contract explicit.Suggested guard
private QueueOutputMessages HandleExternalTransaction(RelationalSendMessageCommandBatch rel) { + if (rel.Messages.Any(m => m.MessageData == null)) + { + throw new DotNetWorkQueueException( + "Batch messages must have non-null MessageData before handler dispatch."); + } + var sqlTransaction = (SqlTransaction)rel.ExternalTransaction; var sqlConn = (SqlConnection)sqlTransaction.Connection;Based on learnings: "In the batch send implementations for the PostgreSQL, SQL Server, and SQLite transports,
m.MessageDatamust be non-null when the internal batch handler runs... add an explicit guard (throw) or at least a debug assertion..."🤖 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/CommandHandler/SendMessageCommandBatchHandler.cs` around lines 152 - 167, Add an explicit fail-fast guard in SendMessageCommandBatchHandler.HandleExternalTransaction before the call to ProcessChunk, because rel.Messages is passed through without revalidating MessageData and later code assumes it is non-null. Check the message batch (or each message in rel.Messages) for null MessageData and throw a clear exception or add a debug assertion so the contract is enforced at this handoff. Use the HandleExternalTransaction and ProcessChunk symbols to place the guard at the internal batch entry point.Source: Learnings
🤖 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 `@docs/outbox-pattern.md`:
- Around line 165-169: The batch-path description overstates the implementation
by implying everything is inserted in a single batched operation; update the
wording in the outbox-pattern docs near SendAsync(batch, transaction) to clarify
that only the body rows are inserted as a multi-row batch inside the
caller-owned transaction, while metadata/status rows are still written per
message afterward. Keep the fail-fast/rollback guidance, but avoid saying “one
insert per chunk, not one round trip per message” for the whole path.
---
Nitpick comments:
In
`@Source/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxBatchSendTests.cs`:
- Around line 250-262: ProducerScope currently disposes its dependencies without
a thread-safe guard, so update its Dispose implementation to follow the
codebase’s Interlocked-based disposable pattern. Add a disposal state field to
ProducerScope and use an Interlocked exchange/check in Dispose so Creator and
Producer are released only once even under concurrent calls. Keep the fix
localized to the ProducerScope nested class in PostgreSqlInboxBatchSendTests and
mirror the same disposal style used elsewhere for disposable helpers.
In
`@Source/DotNetWorkQueue.Transport.SqlServer/Basic/CommandHandler/SendMessageCommandBatchHandler.cs`:
- Around line 152-167: Add an explicit fail-fast guard in
SendMessageCommandBatchHandler.HandleExternalTransaction before the call to
ProcessChunk, because rel.Messages is passed through without revalidating
MessageData and later code assumes it is non-null. Check the message batch (or
each message in rel.Messages) for null MessageData and throw a clear exception
or add a debug assertion so the contract is enforced at this handoff. Use the
HandleExternalTransaction and ProcessChunk symbols to place the guard at the
internal batch entry point.
In
`@Source/DotNetWorkQueue.Transport.SqlServer/Basic/CommandHandler/SendMessageCommandBatchHandlerAsync.cs`:
- Around line 145-160: Add the same fail-fast MessageData non-null contract
check used by the sync batch handler before entering ProcessChunkAsync in
HandleExternalTransactionAsync. This async SQL Server path should validate
rel.Messages items (or the batch chunk) and throw or assert immediately if any
message has null MessageData, rather than letting the loop fail later. Use the
existing HandleExternalTransactionAsync and ProcessChunkAsync flow as the place
to mirror the sync handler’s guard.
🪄 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: 9644fc41-1f04-47b2-b40f-88c38e8540e2
📒 Files selected for processing (11)
Source/DotNetWorkQueue.Transport.PostgreSQL.Integration.Tests/Inbox/PostgreSqlInboxBatchSendTests.csSource/DotNetWorkQueue.Transport.PostgreSQL.Tests/Basic/PostgreSqlRelationalProducerQueueTests.csSource/DotNetWorkQueue.Transport.PostgreSQL/Basic/CommandHandler/SendMessageCommandBatchHandler.csSource/DotNetWorkQueue.Transport.PostgreSQL/Basic/CommandHandler/SendMessageCommandBatchHandlerAsync.csSource/DotNetWorkQueue.Transport.PostgreSQL/Basic/PostgreSqlRelationalProducerQueue.csSource/DotNetWorkQueue.Transport.SqlServer.IntegrationTests/Inbox/SqlServerInboxBatchSendTests.csSource/DotNetWorkQueue.Transport.SqlServer.Tests/Basic/SqlServerRelationalProducerQueueTests.csSource/DotNetWorkQueue.Transport.SqlServer/Basic/CommandHandler/SendMessageCommandBatchHandler.csSource/DotNetWorkQueue.Transport.SqlServer/Basic/CommandHandler/SendMessageCommandBatchHandlerAsync.csSource/DotNetWorkQueue.Transport.SqlServer/Basic/SqlServerRelationalProducerQueue.csdocs/outbox-pattern.md
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #169 +/- ##
==========================================
+ Coverage 87.42% 87.48% +0.05%
==========================================
Files 1023 1023
Lines 33793 33863 +70
Branches 2856 2864 +8
==========================================
+ Hits 29544 29625 +81
+ Misses 3373 3365 -8
+ Partials 876 873 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- docs/outbox-pattern.md: clarify only the body insert is multi-row; per-message meta/status rows are still written one per message (prior wording overstated it). - Integration test ProducerScope (SQL Server + PostgreSQL): add the Interlocked-based thread-safe disposal guard per repo convention. Refs #167. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Follow-up to #162. Gives the inbox held-transaction batch send path
(
Send(List<...>, DbTransaction)withEnableHoldTransactionUntilMessageCommitted) atrue multi-row insert inside the caller's transaction, for SQL Server and
PostgreSQL. Previously this path looped single-row
SendOneper message; now itdispatches a single
RelationalSendMessageCommandBatchto the batch handler, whichreuses the caller's connection/transaction.
Closes #167.
What changed
RelationalSendMessageCommandBatch.ExternalTransaction. When set,HandleExternalTransaction[Async]reuses the caller's
SqlConnection/SqlTransaction(resp.NpgsqlConnection/NpgsqlTransaction)via the existing
ProcessChunk/ProcessChunkAsync— noOpen/BeginTransaction/Commit/Rollback/Dispose/Closeand no catch wrapper. The fork sits afterGuardNoScheduledJobs, so scheduled-jobbatches are still rejected on this path. The standalone/outbox batch path is unchanged.
SqlServerRelationalProducerQueue,PostgreSqlRelationalProducerQueue): theSendWithExternalTransactionBatch[Async]overrides validate once at the boundary, then dispatch asingle
RelationalSendMessageCommandBatchinstead of loopingSendOne. Build-and-dispatch isfactored into an
internal DispatchBatch/DispatchBatchAsyncseam (testability — see below).SendOne/SendOneAsyncremain for the single-message overrides.Behavior change
On the held-transaction batch path only, a failure now throws (the caller owns the
transaction's all-or-nothing semantics and the rollback) instead of being swallowed into per-message
results. The standalone
Send(List<...>)path is unchanged (still reportsHasErrors).Scope / non-goals
batch non-viable (see Add outbox pattern support for SQLite (inbox not workable — single-writer lock) #149).
Send(batch, transaction)throwsInvalidOperationExceptionontransports that don't override it (the base default), i.e. everything except SQL Server + PostgreSQL.
producer ctor params, and
RelationalSendMessageCommandBatch.SkipRetrykeeps the retry decorator outof the caller's transaction.
Testing
SendOneloop), thecommand carries the caller's transaction, the producer never commits/rolls back the caller's
transaction, async dispatch, exception propagation, and a non-Sql/Npgsql transaction throws without
dispatching. (
SqlTransaction/NpgsqlTransactionare sealed/unmockable, so dispatch is testedthrough the
internal DispatchBatchseam; the cast guard is covered by the non-Sql/Npgsql tests.)connectionstring.txt): newSqlServerInboxBatchSendTestsand
PostgreSqlInboxBatchSendTests— commit → rows visible, rollback → rows absent, caller connectionstill open & usable after the call, forced failure throws (MetaData table dropped → meta insert fails),
one distinct positive id per message, and (PG) a multi-chunk batch (
BatchSize=2, 5 messages → 3chunks) asserting recovered ids are strictly increasing in caller input order across chunks within one
external transaction.
Changelog
0.9.42entry added.🤖 Generated with Claude Code
Summary by CodeRabbit
Send; rollback prevents both inbox and related business rows from appearing.