Skip to content

auth: fix lack of atomicity in ConversationService.finish (#102) - #153

Open
BKJN1 wants to merge 1 commit into
mainfrom
fix/102-conversation-finish-atomicity
Open

auth: fix lack of atomicity in ConversationService.finish (#102)#153
BKJN1 wants to merge 1 commit into
mainfrom
fix/102-conversation-finish-atomicity

Conversation

@BKJN1

@BKJN1 BKJN1 commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

closes #102

Problem

In ConversationService.finish, the conversation was deleted from the DB, then the
authorization code and the session were created via two separate repository calls with no
shared transaction. If delete succeeded but either create failed (transient DB error, rare
collision), the conversation was gone but no code/session existed — an unrecoverable
IllegalState for the user, with no way to retry.

Fix

Introduced ConversationFinalizer, a narrow interface with a single finish(...) method that
performs delete-conversation + create-authorization-code + create-session as one unit.
PostgresConversationFinalizer implements it with all three writes inside a single
xa.transactMeasured block, so they commit or roll back together — mirroring the
multi-statement transaction pattern already used in PostgresSessionRepository
(invalidateByUserId, invalidate, create).

ConversationRepository, AuthorizationCodeRepository, and SessionRepository are unchanged;
ConversationService no longer depends on the latter two directly (they were only used inside
finish).

Changes

  • New: ConversationFinalizer (trait) and PostgresConversationFinalizer (impl).
  • New: PostgresConversationFinalizerSpec — DB-backed test covering:
  • ConversationService.finish now calls conversationFinalizer.finish(...) once instead of
    deletecreatecreate. Public signature unchanged, no caller impact.
  • PostgresOAuthApp wires PostgresConversationFinalizer.live into the layer graph.
  • ConversationServiceSpec updated for the new dependency; added a test for
    finish reporting falseIllegalState.
  • No migrations changed — no schema changes required.

@augmentcode

augmentcode Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor
🤖 Augment PR Summary

Summary: This PR makes OAuth conversation completion atomic to avoid losing a completed authorization flow after a partial database failure.

Changes:

  • Adds the narrow ConversationFinalizer contract and its named completion-request record.
  • Implements the finalizer for PostgreSQL with one measured transaction.
  • Performs conversation deletion, authorization-code insertion, and session creation as one unit.
  • Extracts raw repository write helpers so their SQL can run within the shared transaction.
  • Preserves prior-session invalidation and refresh-token migration within that transaction.
  • Updates ConversationService.finish to delegate the three writes to the finalizer.
  • Wires the PostgreSQL finalizer into the OAuth application layer graph.
  • Updates unit-test dependencies and adds database-backed rollback regression coverage.
Technical Notes: A failed code or session insert rolls back the conversation deletion, allowing the authorization flow to be retried safely.

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

@BKJN1
BKJN1 force-pushed the fix/102-conversation-finish-atomicity branch 2 times, most recently from 4f6d45c to 254b6a5 Compare August 8, 2026 06:47
@BKJN1

BKJN1 commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator Author

augment review

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

@BKJN1
BKJN1 force-pushed the fix/102-conversation-finish-atomicity branch from 254b6a5 to ca59ab8 Compare August 8, 2026 09:44
@BKJN1

BKJN1 commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator Author

augment review

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

@BKJN1

BKJN1 commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator Author

@goshacodes
ready for approve

@goshacodes

Copy link
Copy Markdown
Member

Conflicts, leave as is for now, fix it later

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

auth: Lack of atomicity in ConversationService.finish leads to orphaned deletions

2 participants