Skip to content

fix(central): optimize upsertForm version query and handle concurrent insert collision - #181

Open
Lekerr wants to merge 3 commits into
mainfrom
feat/99-upsert-form-concurrent-fix
Open

fix(central): optimize upsertForm version query and handle concurrent insert collision#181
Lekerr wants to merge 3 commits into
mainfrom
feat/99-upsert-form-concurrent-fix

Conversation

@Lekerr

@Lekerr Lekerr commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Closes #99

Problem

upsertForm had two issues:

  1. Fetched all version rows to compute MAX in Scala — unnecessary data transfer
  2. No concurrency protection: two parallel calls could read the same MAX(version), compute the same nextVersion, and both attempt an INSERT — causing an unhandled PRIMARY KEY violation

Solution

  1. Replaced the full version scan with a single aggregate query:
    SELECT COALESCE(MAX(version), 0) + 1, COUNT(*) = 0 FROM forms WHERE id = $id
  2. Switched from REPEATABLE_READ to READ_COMMITTED and added .retry() on SqlException wrapping a 23505 (unique_violation) — the entire transaction retries with fresh data

Test

Added concurrent test: 5 parallel upsertForm calls on the same id — verifies all 5 versions are created without collision

@Lekerr

Lekerr commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

augment review

@augmentcode

augmentcode Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor
🤖 Augment PR Summary

Summary: This PR optimizes PostgreSQL form upserts and adds retry handling for concurrent version creation.

Changes:

  • Replaces the full form-version scan with an aggregate query that returns the next version and empty-state flag.
  • Uses the default READ COMMITTED transaction isolation for upserts so retries can observe newly committed versions.
  • Retries transactions on PostgreSQL unique-violation (23505) failures with exponential, jittered backoff.
  • Preserves active-version switching and retention of the latest five versions inside the transaction.
  • Adds an integration test that issues five parallel upserts and verifies versions 1 through 5 are present.

🤖 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. 1 suggestion posted.

Fix All in Augment

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

case cause: PSQLException => cause.getSQLState == "23505"
case _ => false
case _ => false
} && Schedule.recurs(3)

@augmentcode augmentcode Bot Aug 15, 2026

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.

With five callers that all read the same version, one insert succeeds per round while the remaining calls collide; after the initial attempt plus these three recurrences, one of five callers can still receive the 23505 failure. This means the new five-call concurrency contract is not guaranteed, and the unsynchronized test may pass without reaching that worst-case interleaving.

Severity: medium

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

@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. 1 suggestion posted.

Fix All in Augment

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

case cause: PSQLException => cause.getSQLState == "23505"
case _ => false
case _ => false
} && Schedule.recurs(3)

@augmentcode augmentcode Bot Aug 15, 2026

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.

Schedule.recurs(3) permits only three retries after the initial attempt. If five calls concurrently read the same maximum version, only one can commit in each round, so the fifth call still collides on its third retry and fails with 23505; this does not provide the stated collision handling for the new five-call scenario (and can occur with a pool larger than the test pool).

Severity: medium

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

@Lekerr

Lekerr commented Aug 15, 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.

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.

central: PostgresFormRepository.upsertForm fetches all version rows and has an unhandled concurrent-insert collision

1 participant