Skip to content

UnitOfWork<TContext> cleanup masks the causal exception, and CurrentTransaction can return an unusable handle #485

Description

@brenpike

Two related defects in UnitOfWork<TContext>, both surfaced during the #480 outbox commit-point work and both deliberately left out of that PR. Neither was introduced by it — both are structurally identical on master.

1. Terminal cleanup on the failure path can mask the real cause

UnitOfWork.cs (the ExecuteAsync catch):

catch (Exception ex)
{
    await RollbackAsync(scope, ct);
    _logger.LogError(ex, "Error occurred during unit of work");
    throw;
}

RollbackAsync is unguarded. If it throws — a cancelled ct, or a transaction SQL Server has already aborted — the rollback exception replaces the operation's exception and the real cause of the failure is lost before the throw; is reached.

This is the same exception-masking class the #480 PR fixed for the outbox's concurrency compensation, on a different surface. That fix wrapped the whole best-effort compensation body and logged the failure so the causal exception always propagates. The same shape applies here.

Bounded impact: diagnostic only. There is no data-integrity consequence — the transaction is doomed on either path. Reachable only when the rollback itself fails, which requires an already-broken connection, an already-aborted transaction, or a cancelled token.

2. CurrentTransaction can hand back a handle that throws on use

UnitOfWork.cs:

public IPersistanceTransaction CurrentTransaction => PersistanceTransaction.Create(_context.Database.CurrentTransaction);

PersistanceTransaction.Create accepts a null IDbContextTransaction. TransactionId degrades gracefully to Guid.Empty, but CommitAsync and RollbackAsync would raise a NullReferenceException on that handle.

This surface is public: CurrentTransaction is part of IUnitOfWork, and BrokeredMessageOutbox<TContext> re-exposes it. The #480 PR's ADR amendment explicitly records that this handle stays commit-capable by design — a consumer commits on its own behalf through it — so the null case is worth closing rather than leaving to chance.

Bounded impact: requires a consumer to read CurrentTransaction when no transaction is active and then call a terminal method on the result.

Why these are one issue

Both are cleanup/handle-lifetime defects on the same type, both are diagnostic-or-misuse rather than correctness-of-committed-data, and both are cheap to close together with one set of characterization tests. Splitting them would pay the test-harness setup twice.

Links

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions