Skip to content

refactor(api): IssueAsync commits, so every caller must wrap it in a transaction #195

Closed

Description

@bitbiter-dev

Split out of #172, which fixed the lost-write bug in AdminUserService and deliberately left this alone: it is interface depth, not a correctness defect, and folding an auth-flow refactor into a two-line transaction fix would have been the wrong trade.

The seam

TokenService.IssueAsync stages a refresh token and commits it (src/Anichron.API/Services/TokenService.cs:43):

tokens.Add(new RefreshToken { ... });
await unitOfWork.SaveChangesAsync(ct);          // the callee owns the save
return new AuthTokens(jwtFactory.Create(user), rawToken);

Because the callee commits, every caller that has its own pending changes must open a transaction purely to make the two saves atomic. AuthService does this twice — src/Anichron.API/Services/AuthService.cs:106-110 and :160-164:

return AuthResult.Ok(await unitOfWork.ExecuteInTransactionAsync(async () =>
{
    await unitOfWork.SaveChangesAsync(ct);          // save #1 — the caller's changes
    return await tokenService.IssueAsync(user, ct); // saves again inside
}, ct));

Two round trips and an explicit transaction, to work around a callee that commits.

Why it is worth changing

The interface currently reads "issuance also commits, so wrap me". It should read "issuance stages a token" — leaving the caller to save once, as the unit-of-work pattern intends. SaveChangesAsync is the caller's decision everywhere else in the API.

⚠️ The current shape is not a bug. The transactions are present and correct; this is about the interface making them necessary.

Sketch

  • Drop the SaveChangesAsync from IssueAsync, so it only stages.
  • Callers save once. AuthService's two sites collapse from transaction-plus-two-saves to a single SaveChangesAsync.
  • Check every other caller of IssueAsync first — any that relied on it committing will silently stop persisting the token, which is the obvious way to get this wrong.

Done when

  • IssueAsync stages without committing
  • AuthService:106 and :160 no longer need a transaction solely to pair two saves
  • A test pins that a token issued but not saved is not persisted, so the new contract is enforced rather than assumed
  • Every caller audited, not just the two in AuthService

Line numbers verified against master at the time of writing.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestneeds-triageMaintainer needs to evaluate this issue

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions