fix(payment): flush Wolverine outbox in ExecuteInTransactionAsync#26
Conversation
ExecuteInTransactionAsync opens an EF transaction, runs the caller's work
delegate, then commits. Missing: a SaveChangesAsync between work() and
Commit. Without it, Wolverine's outbox staging — which happens via
UseEntityFrameworkCoreTransactions' SaveChanges interceptor — never reaches
the wolverine.outgoing_envelopes table. The entity write commits, the event
publish silently disappears.
Concrete failure mode: PaymentRecoveryJob calls
ExecuteInTransactionAsync(async ct => { await UpdateAsync(payment, ct);
await eventPublisher.PublishAsync(failedEvent, ct); }, ct).
UpdateAsync's internal SaveChanges flushes the Payment row. PublishAsync
then stages the envelope to the tracker. work() returns. tx.CommitAsync
commits the Payment update, but no SaveChanges has run to flush the
envelope — it stays in the tracker, the transaction commits without it,
and PaymentFailedEvent is dropped. The saga stalls — Order never sees the
payment failed, never marks itself failed, customer never sees a refund
offer.
Fix is one line: `await context.SaveChangesAsync(ct)` between work() and
Commit. The teaching comment is generous because this is exactly the kind
of "looks correct, silently wrong" outbox bug that the architecture review
caught and that a junior reader needs to understand to not regress.
Discovered by: architecture-reviewer agent pass on PaymentRecoveryJob.
Follow-up not in this PR: integration test that asserts a row appears in
wolverine.outgoing_envelopes inside the same transaction as the Payment
update. Should be added to tests/PaymentService.Tests.Integration when
that slice exists.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
WalkthroughPaymentRepository's ChangesOutbox envelope transactional persistence
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PaymentService/Infrastructure/PaymentRepository.cs (1)
5-60: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse a true file-scoped namespace in this C# file.
This file still uses block-scoped layout; switch to file-scoped namespace style consistently.
♻️ Suggested update
-namespace PaymentService.Infrastructure; - -/// <summary> -/// EF Core implementation of <see cref="IPaymentRepository"/>. Both <see cref="GetByIdAsync"/> -/// and <see cref="GetByOrderIdAsync"/> are shared with the command path -/// (<c>ProcessPaymentHandler</c> uses <c>GetByOrderIdAsync</c> as the idempotency check then -/// later mutates and updates the entity), so tracking stays ON for both — see -/// <c>docs/cqrs-data-access.md</c> for the rationale. -/// </summary> -public class PaymentRepository(PaymentDbContext context) : IPaymentRepository -{ +namespace PaymentService.Infrastructure; + +/// <summary> +/// EF Core implementation of <see cref="IPaymentRepository"/>. Both <see cref="GetByIdAsync"/> +/// and <see cref="GetByOrderIdAsync"/> are shared with the command path +/// (<c>ProcessPaymentHandler</c> uses <c>GetByOrderIdAsync</c> as the idempotency check then +/// later mutates and updates the entity), so tracking stays ON for both — see +/// <c>docs/cqrs-data-access.md</c> for the rationale. +/// </summary> +public class PaymentRepository(PaymentDbContext context) : IPaymentRepository +{ ... }As per coding guidelines: "**/*.cs: File-scoped namespaces required."
🤖 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 `@PaymentService/Infrastructure/PaymentRepository.cs` around lines 5 - 60, The file currently uses a block-scoped namespace; change it to a file-scoped namespace by declaring "namespace PaymentService.Infrastructure;" with a trailing semicolon and removing the surrounding namespace { ... } braces so the top-level class PaymentRepository (and its constructor PaymentRepository(PaymentDbContext context) and methods like GetByIdAsync, GetByOrderIdAsync, AddAsync, UpdateAsync, GetStalePendingPaymentIdsAsync, ExecuteInTransactionAsync) are declared at file scope; keep all existing comments, usings and members intact and adjust indentation accordingly.
🤖 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.
Outside diff comments:
In `@PaymentService/Infrastructure/PaymentRepository.cs`:
- Around line 5-60: The file currently uses a block-scoped namespace; change it
to a file-scoped namespace by declaring "namespace
PaymentService.Infrastructure;" with a trailing semicolon and removing the
surrounding namespace { ... } braces so the top-level class PaymentRepository
(and its constructor PaymentRepository(PaymentDbContext context) and methods
like GetByIdAsync, GetByOrderIdAsync, AddAsync, UpdateAsync,
GetStalePendingPaymentIdsAsync, ExecuteInTransactionAsync) are declared at file
scope; keep all existing comments, usings and members intact and adjust
indentation accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4068bb4f-7697-4633-bf48-e0c0668775c8
📒 Files selected for processing (1)
PaymentService/Infrastructure/PaymentRepository.cs
ExecuteInTransactionAsync opens an EF transaction, runs the caller's work delegate, then commits. Missing: a SaveChangesAsync between work() and Commit. Without it, Wolverine's outbox staging — which happens via UseEntityFrameworkCoreTransactions' SaveChanges interceptor — never reaches the wolverine.outgoing_envelopes table. The entity write commits, the event publish silently disappears.
Concrete failure mode: PaymentRecoveryJob calls
ExecuteInTransactionAsync(async ct => { await UpdateAsync(payment, ct); await eventPublisher.PublishAsync(failedEvent, ct); }, ct). UpdateAsync's internal SaveChanges flushes the Payment row. PublishAsync then stages the envelope to the tracker. work() returns. tx.CommitAsync commits the Payment update, but no SaveChanges has run to flush the envelope — it stays in the tracker, the transaction commits without it, and PaymentFailedEvent is dropped. The saga stalls — Order never sees the payment failed, never marks itself failed, customer never sees a refund offer.
Fix is one line:
await context.SaveChangesAsync(ct)between work() and Commit. The teaching comment is generous because this is exactly the kind of "looks correct, silently wrong" outbox bug that the architecture review caught and that a junior reader needs to understand to not regress.Discovered by: architecture-reviewer agent pass on PaymentRecoveryJob.
Follow-up not in this PR: integration test that asserts a row appears in wolverine.outgoing_envelopes inside the same transaction as the Payment update. Should be added to tests/PaymentService.Tests.Integration when that slice exists.
What changed
How it was built
Verification
Touches (check only if applies)
See CLAUDE.mdparaphrase (run/check-rulesto audit drift)Summary by CodeRabbit