fix(api): serialize concurrent commits on one variant with a row lock - #5750
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe git commit path now supports expected-head validation under a PostgreSQL variant lock. It reports revision conflicts, lock timeouts, and missing variants through domain and HTTP exceptions. Unit and PostgreSQL race tests cover ordering, errors, and concurrent commits. ChangesChecked Commit Concurrency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant GitDAO
participant PostgreSQL
participant FastAPI
Caller->>GitDAO: commit_revision(expected_head_revision_id)
GitDAO->>PostgreSQL: set lock timeout and lock variant
GitDAO->>PostgreSQL: read current head
PostgreSQL-->>GitDAO: current head or no head
GitDAO->>GitDAO: validate expected head
GitDAO->>PostgreSQL: insert revision when heads match
GitDAO-->>FastAPI: revision or domain exception
FastAPI-->>Caller: commit result or mapped HTTP error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| "An initial revision already exists for this variant." | ||
| ) | ||
|
|
||
| if expected_head_revision_id is not None: |
There was a problem hiding this comment.
This re-read is the guarantee. A lock alone does not prevent a lost update.
Two writers can read head N, each in its own transaction. Both then pass their own base check. Both then insert. The lock makes the two inserts sequential. The lock does not find the stale base.
Only a read that happens while the writer holds the lock can see the insert of the other writer. If you move this read before the lock, the guard becomes a comparison that races.
| .where( | ||
| self.RevisionDBE.project_id == project_id, # type: ignore | ||
| self.RevisionDBE.variant_id == revision_commit.variant_id, # type: ignore | ||
| self.RevisionDBE.deleted_at.is_(None), # type: ignore |
There was a problem hiding this comment.
This filter makes archive_revision and unarchive_revision relevant to the head. Neither method takes the variant lock.
This is safe, because neither method inserts a revision. Two orders are possible. If the archive commits first, this read finds the older revision, and the caller receives a 409 that names it. If the archive commits second, the commit already used the head that it expected.
The result is a refusal that the caller can retry. The result is never a lost write. Section 5.1 of notes/dao-lock-impact.md gives the full argument.
| # -------------------------------------------------------------------------- | ||
|
|
||
| @suppress_exceptions(exclude=[InitialRevisionConflict]) | ||
| # A refusal is an ANSWER, not a failure: suppression would turn a 409, a 503, or a 404 |
There was a problem hiding this comment.
This method carries suppress_exceptions. That decorator changes an exception into a None return value. The caller then reports zero revisions and gives no reason.
Every name in this list is an answer for the client, not a failure. RevisionConflict becomes a 409. CommitLockTimeout becomes a 503. VariantNotFound becomes a 404.
If you remove a name from this list, its HTTP status becomes a 200 response with a count of zero.
| ) | ||
| # A row that is missing, or owned by another project, locks nothing and | ||
| # reports nothing. Continuing would run every guard below unprotected. | ||
| if lock_result.scalar_one_or_none() is None: |
There was a problem hiding this comment.
SELECT ... FOR UPDATE does not fail when it matches no row. It locks nothing and reports nothing.
A variant id that does not exist gives this result. A variant that belongs to a different project gives the same result. The earlier code ignored the result of the statement. The guards below then ran without a lock, and the insert completed.
This test makes a missing row an error.
| # A waiter must fail loudly rather than hold a connection forever; | ||
| # the contract requires the wait to be bounded. | ||
| await session.execute( | ||
| text("SET LOCAL lock_timeout = :timeout").bindparams( |
There was a problem hiding this comment.
This statement limits the wait for the lock. SET LOCAL applies the limit to this transaction only, so no other query inherits it.
The limit must precede the lock statement, or it has no effect. Postgres reports the expiry as SQLSTATE 55P03, and _raise_on_lock_timeout changes that code into CommitLockTimeout.
Without a limit, one blocked holder keeps a connection until the client stops. Under load, this empties the connection pool.
| ) | ||
|
|
||
| session.add(revision_dbe) | ||
| await session.commit() |
There was a problem hiding this comment.
This commit releases the variant lock. The version bookkeeping runs after this line.
_get_version, _set_version, and the version-zero normalization are therefore outside the serialized unit. This is acceptable. They operate on the row that this method just inserted, and they are not part of the guard.
A move of these calls inside the lock does not help the callers that can collide on a version number. Those callers pass no expectation, so they take no lock at all.
| # the int() cast keeps the interpolation injection-free. | ||
| await session.execute( | ||
| text( | ||
| f"SET LOCAL lock_timeout = '{int(env.postgres.commit_lock_timeout_ms)}ms'" |
There was a problem hiding this comment.
This line was found broken in live testing, after the review round. The first version used a bind parameter (SET LOCAL lock_timeout = :timeout). Postgres rejects bind parameters in SET statements. On a real database, every locked commit failed with a syntax error. The suppression layer turned that into a 500 commit_failed response. The unit tests did not catch it because the fake session never executes real SQL, and the real-database tests skip when no Postgres is reachable. The fix is an inline literal. The int() cast keeps it injection-free. A unit assertion now fails if a bind placeholder ever returns to this statement. The end-to-end proof ran against a live stack: the same commit that failed now returns 200, and a stale-base retry returns 409 with the re-anchor instruction.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bd1010f-4bf2-4d48-b575-2f2af1fd52b9
📒 Files selected for processing (9)
api/oss/src/apis/fastapi/git/exceptions.pyapi/oss/src/core/git/interfaces.pyapi/oss/src/core/git/types.pyapi/oss/src/dbs/postgres/git/dao.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/unit/git/conftest.pyapi/oss/tests/pytest/unit/git/test_commit_revision_lock.pyapi/oss/tests/pytest/unit/git/test_commit_revision_race.pydocs/design/agent-config-editing/notes/dao-lock-impact.md
6793e05 to
b6ec133
Compare
9b80aa9 to
7e94e2a
Compare
b6ec133 to
de02f74
Compare
7e94e2a to
39f2a82
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 58722e4b-1acf-4129-8551-37dbaf72bdba
📒 Files selected for processing (9)
api/oss/src/apis/fastapi/git/exceptions.pyapi/oss/src/core/git/interfaces.pyapi/oss/src/core/git/types.pyapi/oss/src/dbs/postgres/git/dao.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/unit/git/conftest.pyapi/oss/tests/pytest/unit/git/test_commit_revision_lock.pyapi/oss/tests/pytest/unit/git/test_commit_revision_race.pydocs/design/agent-config-editing/notes/dao-lock-impact.md
🚧 Files skipped from review as they are similar to previous changes (6)
- api/oss/tests/pytest/unit/git/conftest.py
- api/oss/src/utils/env.py
- api/oss/tests/pytest/unit/git/test_commit_revision_lock.py
- api/oss/src/core/git/interfaces.py
- api/oss/src/core/git/types.py
- api/oss/src/dbs/postgres/git/dao.py
de02f74 to
1ab8e68
Compare
39f2a82 to
fb6de69
Compare
fb6de69 to
9ba89e7
Compare
1ab8e68 to
08a899a
Compare
9ba89e7 to
5efea5e
Compare
39d989c to
540f3de
Compare
792e1c1 to
40dd540
Compare
… lock (S1b-lock) Extends the initial-commit-only lock condition to every commit and re-reads the head UNDER the lock, which is what makes the base check an invariant rather than advice. Adds RevisionConflict, excludes it and the embed error from exception suppression, and pins the behavior with a two-writer race test. Impact analysis for every flow that commits through this dao: docs/design/agent-config-editing/notes/dao-lock-impact.md. Deliberately minimal for owner and CTO review.
Exact head comparison including the absent-head case, deterministic head ordering, a real two-connection race test (integration-marked, fails if the lock is removed), a bounded lock wait via POSTGRES_COMMIT_LOCK_TIMEOUT_MS, the interface declaration, and the corrected impact note stating plainly what the lane does and does not buy.
…(final review F5-dao, dao bullet 3)
…r (found live: every locked commit failed 500 on real Postgres)
…l proves the SET LOCAL statement
…c race gate (CodeRabbit)
…t commit's real data (four-month-old bug; stored-row tests added)
…gs merged (companion to the nulling fix)
…ted-review blocker A, dao half)
…ag-off service cells move to the lane that passes no_change_check (they failed here: the wrapper argument does not exist yet)
540f3de to
4768cf9
Compare
40dd540 to
34cc7c8
Compare
dosubot
Bot
added
size:L
Railway Preview Environment
|
fix(api): serialize concurrent commits on one variant with a row lock
1 participant
Context
Two commits on the same workflow variant can both succeed and one can vanish. The service reads the head, applies the change, then calls the DAO, and those are three separate transactions. Two writers both read head N, both pass the base check, and both insert. The second write silently replaces the first in the history the user sees.
A comparison the caller makes before calling cannot fix this. Only a head read taken while holding a lock can see the other writer's insert.
Changes
GitDAO.commit_revisiongains two optional parameters.expected_head_revision_idis the revision the caller built on.no_change_checkis a comparison the caller hands down rather than making itself.When either is passed, the DAO bounds the wait with
SET LOCAL lock_timeout, locks the variant row withSELECT ... FOR UPDATE, and does its checking inside that lock. It re-reads the head with the samecreated_at DESC, id DESCordering every other head read uses and raisesRevisionConflictwhen it differs from the caller's expectation. Absence counts as a difference: an expectation against a variant with no head means the caller read a revision this variant does not have.The re-read is the point, and it is why this lane is not just the lock. With a lock alone, both writers still succeed:
The lock serialized the inserts. It did not catch the stale base, because B checked before it held the lock.
no_change_checkexists for the same reason, one level up. A caller that decides "this commit would store what is already stored" before calling has decided it about a head another writer can still move. The callback runs against the head as it stands under the lock, and after the conflict check, so a stale caller whose result happens to equal the NEW head gets the conflict and never a no-change answer. It raisesRevisionUnchangedcarrying the head it decided against.Two refusals stop being silent.
commit_revisioncarries@suppress_exceptions, which turns any exception into aNonereturn, and the caller then reports "nothing committed" with no reason.RevisionConflictis excluded, so a 409 reaches the client. An expiredlock_timeoutis now translated intoCommitLockTimeoutand excluded too, so a caller that waits past the timeout learns that instead of receiving a successful empty commit. Postgres reports the expiry as SQLSTATE55P03on the driver error that SQLAlchemy wraps, so the translation walks the exception chain rather than trusting one wrapper's shape.The locking select now checks that it locked a row.
SELECT ... FOR UPDATEon a row that is missing, or owned by another project, locks nothing and reports nothing. Ignoring that result ran every guard below it unprotected and inserted a revision against a variant the project does not have. It now raisesVariantNotFound.A first commit no longer loses its data
Separate defect, same method, four months old. A variant's first revision is version 0, the empty placeholder, and its fields were nulled so a reader could tell "not configured yet" from "configured empty". A first commit that CARRIES content is not that placeholder, and nulling it discarded what the caller sent: the endpoint answered 200, the stored row held NULL, and the next read reported no revision at all.
Every flow that commits twice hid this, because the second commit is version 1. It surfaced in live testing on a single-commit flow.
Before:
POSTa first revision with data, read it back, getdata: null.After: the nulling applies only when the commit carries no
data,flags,tagsormeta.The invariant, and where it stops
The invariant this lane buys is exactly one sentence: two concurrent writers cannot both insert a revision for the same variant. The guard and the insert are atomic with respect to each other, so a lost update is impossible. Three boundaries sit outside it, and each is safe for a stated reason.
notes/dao-lock-impact.mdcarries the full argument.The lock is opt-in, not a GitDAO-wide rule. It is taken only when a caller passes
initial,expected_head_revision_id, orno_change_check. Only the workflows checked-commit path passes any of them. Environments, testsets, queries,fork_variant, and ten data migrations pass none, so they take no lock and their cost is unchanged. This is deliberate: an unconditional lock would be taken thousands of times during a migration run and once per replayed revision insidefork_variant. The narrow blast radius is the reason to prefer this over a database unique constraint, which cannot be opt-in and would need a migration this work excludes.Archive and unarchive do not take the lock, even though
deleted_atdecides which revision is the head. No ordering of them can lose an update, because archiving never inserts. If an archive commits first, the checked commit's re-read sees the older revision and the caller gets a 409 naming it, which is a refusal rather than a silent write on a base that moved. If it commits second, the commit already landed on the head it expected. The worst case is a spurious refusal, and the retry loop already handles those.Version bookkeeping runs after the lock is released.
_get_version,_set_version, and the version-zero normalization run after the explicit commit insidecommit_revision. They are bookkeeping on the row just inserted, not part of the guard. Moving them inside would also not help the callers that could collide on them, because those callers take no lock at all. This one is a real ordering smell with no exposure change, and it is recorded rather than fixed.AsyncEngine.session()is task-scoped, so a nestedasync with engine.session()yields the same session, not a savepoint. Anyone extending this method should know that before moving work around. The note explains it.Tests / notes
test_commit_revision_lock.py, fake sessions and no database. It pins the decisions: an unchecked commit takes no lock and sets no timeout, an initial commit still takes the lock, the head read happens under the lock and not before, a moved head refuses and inserts nothing, an expired wait raises instead of returningNonein both the wrapped and the bare driver-error shape, every other database error keeps today's suppressed behavior, and a locked select that matched no row refuses.test_commit_revision_race.py, real Postgres with two live connections. It pins the mechanism: two writers on one head, exactly one commits, the loser is told the winner's id, the revision count rises by exactly one, and ten consecutive races never double-commit. Deletewith_for_update()andtest_exactly_one_writer_winsstarts failing.no_change_checkdistinguishes a pre-lock from a post-lock comparison directly: a second connection holds the variant lock and inserts a revision while the commit waits on it, and the candidate equals what that other writer stored. A pre-lock comparison cannot see that row and inserts a duplicate. Making the comparison skip the lock makes exactly that cell fail.test_commit_lock_scope.pyrecords which commits take the lock at all, so the claim that unchecked callers are unaffected is executed rather than read off the condition.SET LOCALcannot take a bind parameter on Postgres, so every locked commit failed with a 500 on a real database while staying green on 1911 mocked tests. Fixed with an inline literal. A unit assertion pins the statement shape and an integration cell runs the real statement against a live database, and it provably fails on the old form.POSTGRES_COMMIT_LOCK_TIMEOUT_MSdefaults to 5000.This targets
agent-config-editing-s6and is part of the agent-config-editing stack. Read the stack bottom up.