Skip to content

Fix concurrent same-key PUT and PATCH upserts - #3787

Open
aaronburtle wants to merge 3 commits into
mainfrom
dev/aaronburtle/concurrent-same-key-put-patch-bug
Open

Fix concurrent same-key PUT and PATCH upserts#3787
aaronburtle wants to merge 3 commits into
mainfrom
dev/aaronburtle/concurrent-same-key-put-patch-bug

Conversation

@aaronburtle

@aaronburtle aaronburtle commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Why make this change?

Closes #3743

What is this change?

  • Serialize insert-capable upserts before checking whether the target row exists. This prevents concurrent PUT or PATCH requests for the same initially missing key from both attempting an insert.
    • SQL Server uses UPDLOCK and HOLDLOCK on the existence check.
    • PostgreSQL uses transaction-scoped advisory locks with a conservative hybrid strategy:
      • Non-null, non-array smallint, integer, bigint, and UUID primary keys use a key-scoped lock derived from the source and complete ordered primary key. Upserts for different keys can therefore proceed concurrently.
      • Text, fixed-width character, nullable, array, custom, unknown, and mixed composite keys fall back to a source-scoped advisory lock. This preserves correctness when PostgreSQL considers differently represented values equal, including trailing-space and collation-sensitive comparisons.
      • Lock acquisition remains a separate statement so the subsequent existence check receives a fresh READ COMMITTED snapshot after waiting.
      • The implementation continues using hashtextextended and does not introduce a PostgreSQL 14 dependency.
    • Data Warehouse SQL uses TABLOCKX and HOLDLOCK because configured logical keys are not necessarily enforced by unique constraints.
    • Update-only fallback behavior remains unchanged.
  • Adds regression coverage for:
    • Concurrent PUT and PATCH requests.
    • Composite primary keys.
    • PostgreSQL integer and UUID key-scoped locks.
    • PostgreSQL string-key source fallback.
    • PostgreSQL fixed-width keys with different representations that compare equal.
    • PostgreSQL advisory-lock result-set handling.
    • Update-only fallback paths.
  • Relevant documentation:

How was this tested?

  • Integration Tests
    • Concurrent PUT and PATCH integration tests were added for SQL Server and PostgreSQL.
    • A PostgreSQL regression test verifies that differently padded character(n) keys that compare equal produce only one row.
  • Unit Tests
    • All six relational upsert query-builder tests passed.
    • The focused PostgreSQL advisory-lock result-set test passed.
    • The full solution builds successfully.
    • Formatting verification, editor diagnostics, and git diff --check passed.

Sample Request(s)

  • Send multiple concurrent PUT requests to the same initially missing composite key:
PUT /api/commodities/categoryid/0/pieceid/9000 HTTP/1.1
Content-Type: application/json

{
  "categoryName": "SciFi",
  "piecesAvailable": 1,
  "piecesRequired": 1
}
  • The same scenario is supported with PATCH:
PATCH /api/commodities/categoryid/0/pieceid/9100 HTTP/1.1
Content-Type: application/json

{
  "categoryName": "SciFi",
  "piecesAvailable": 2,
  "piecesRequired": 2
}
  • When concurrent requests target the same missing key, exactly one request creates the row and returns 201 Created; the remaining requests update that row and return 200 OK. Only one row remains for the key.
  • No CLI changes are included.

Copilot AI 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.

Pull request overview

This PR addresses a TOCTOU race in relational upsert (PUT/PATCH) flows where concurrent requests for the same initially-missing key could both choose the INSERT path, leading to duplicate-key failures (SQL Server/PostgreSQL) or duplicate logical rows (DWSQL). It adds provider-specific serialization around the “row exists?” decision and introduces regression tests to validate same-key concurrency behavior and PostgreSQL result-set handling.

Changes:

  • SQL Server & DWSQL: add locking table hints on the existence check for insert-capable upserts (UPDLOCK,HOLDLOCK for MSSQL; TABLOCKX,HOLDLOCK for DWSQL), without affecting update-only fallback behavior.
  • PostgreSQL: add a transaction-scoped advisory lock derived from source + full PK before the existence check, and update the executor to skip the lock statement’s result set.
  • Add unit + integration regression coverage for concurrent PUT/PATCH upserts, composite PK ordering, and PostgreSQL multi-result-set handling.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/Core/Resolvers/MsSqlQueryBuilder.cs Adds locking hints to serialize insert-capable upsert existence checks.
src/Core/Resolvers/DWSqlQueryBuilder.cs Adds exclusive table locking on DWSQL existence checks for insert-capable upserts.
src/Core/Resolvers/PostgresQueryBuilder.cs Emits an advisory-lock statement before the existence check for insert-capable upserts.
src/Core/Resolvers/PostgreSqlExecutor.cs Skips the advisory-lock result set before interpreting existing upsert result sets.
src/Service.Tests/UnitTests/RelationalUpsertConcurrencyQueryBuilderTests.cs Unit tests asserting provider-specific serialization primitives are emitted (and not emitted for update-only fallback).
src/Service.Tests/UnitTests/PostgreSqlQueryExecutorUnitTests.cs Unit test verifying the executor correctly skips the advisory-lock result set.
src/Service.Tests/SqlTests/RestApiTests/UpsertConcurrencyTestBase.cs Shared integration test logic for concurrent same-key PUT/PATCH upserts against missing composite keys.
src/Service.Tests/SqlTests/RestApiTests/MsSqlUpsertConcurrencyTests.cs SQL Server integration coverage for concurrent same-key PUT/PATCH upserts.
src/Service.Tests/SqlTests/RestApiTests/PostgreSqlUpsertConcurrencyTests.cs PostgreSQL integration coverage for concurrent same-key PUT/PATCH upserts.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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

Labels

bug Something isn't working

Projects

Status: Review In Progress

Development

Successfully merging this pull request may close these issues.

[Bug]: Concurrent same-key PUT/PATCH upserts can fail or create duplicate logical rows

4 participants