Skip to content

Commit 20f445e

Browse files
committed
fix(client): refuse concurrent operations while a commit is in flight
Two tasks committing the same handle could both pass the open-check; the server applies one and rejects the other for the consumed handle, and if that rejection arrived last it overwrote `committed` with `aborted`, telling the caller nothing was applied and inviting a duplicate retry. The handle now transitions to a `committing` state BEFORE awaiting the RPC, so a concurrent commit or statement is refused with a clear error before it sends anything; the state settles to the real outcome when the first commit resolves. An interruption at the synchronous boundary treats a parked `committing` like `open` and records indeterminate. Regression tests (seen failing before the fix): a second concurrent commit and a statement during a commit are both refused with exactly one RPC sent and the true outcome preserved.
1 parent 15391cc commit 20f445e

2 files changed

Lines changed: 65 additions & 5 deletions

File tree

coordinode/coordinode/client.py

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,12 @@ def is_open(self) -> bool:
314314
def _require_open(self, action: str) -> None:
315315
if self._state == "open":
316316
return
317+
if self._state == "committing":
318+
raise RuntimeError(
319+
f"Cannot {action} this transaction: its commit is already in flight. "
320+
"Concurrent operations on one transaction handle would race the "
321+
"commit's outcome; await it instead."
322+
)
317323
if self._state == "aborted":
318324
raise RuntimeError(
319325
f"Cannot {action} this transaction: an earlier failure closed it on the "
@@ -467,6 +473,11 @@ async def commit(self) -> int:
467473
)
468474

469475
self._require_open("commit")
476+
# Transition BEFORE the await: a concurrent operation on this handle
477+
# would otherwise pass its own open-check while the commit is in
478+
# flight, and the loser's "unknown transaction" rejection would
479+
# overwrite the real outcome (committed) with a claimed abort.
480+
self._state = "committing"
470481
try:
471482
resp = await self._client._cypher_stub.CommitTransaction(
472483
CommitTransactionRequest(transaction_id=self._id), timeout=self._client._timeout
@@ -1371,11 +1382,12 @@ def commit(self) -> int:
13711382
except BaseException:
13721383
# An interruption at the loop boundary (Ctrl-C, SystemExit) never
13731384
# reaches the async handlers, so without this the handle would
1374-
# read "open" while the server may already have applied the
1375-
# writes — inviting the duplicate retry the indeterminate state
1376-
# exists to prevent. An outcome the inner handler already decided
1377-
# (aborted, indeterminate, committed) is kept.
1378-
if self._inner._state == "open":
1385+
# read "open" (or stay parked mid-"committing") while the server
1386+
# may already have applied the writes — inviting the duplicate
1387+
# retry the indeterminate state exists to prevent. An outcome the
1388+
# inner handler already decided (aborted, indeterminate,
1389+
# committed) is kept.
1390+
if self._inner._state in ("open", "committing"):
13791391
self._inner._state = "indeterminate"
13801392
raise
13811393

tests/unit/test_transactions.py

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1206,3 +1206,51 @@ async def slow_rollback(req, timeout=None):
12061206
assert cleanup_done.is_set(), "cancelling close() killed the cleanup"
12071207

12081208
asyncio.run(_inner())
1209+
1210+
1211+
class TestConcurrentCommitSerialization:
1212+
"""Two tasks committing the same handle must not race the state machine:
1213+
the second must be refused BEFORE it sends anything, or the loser's
1214+
"unknown transaction" rejection would overwrite `committed` with
1215+
`aborted` and invite a duplicate retry."""
1216+
1217+
def test_a_second_concurrent_commit_is_refused_not_raced(self):
1218+
from unittest.mock import AsyncMock
1219+
1220+
async def _inner() -> None:
1221+
async def slow_commit(req, timeout=None):
1222+
await asyncio.sleep(0.05)
1223+
return cypher_pb2.CommitTransactionResponse(applied_index=7)
1224+
1225+
client = _async_client(CommitTransaction=AsyncMock(side_effect=slow_commit))
1226+
tx = await client.begin_transaction()
1227+
first = asyncio.create_task(tx.commit())
1228+
await asyncio.sleep(0.01) # first commit is now awaiting the RPC
1229+
with pytest.raises(RuntimeError, match="in flight"):
1230+
await tx.commit()
1231+
assert await first == 7
1232+
assert client._cypher_stub.CommitTransaction.await_count == 1
1233+
# The handle records the real outcome, untouched by the refusal.
1234+
with pytest.raises(RuntimeError, match="already committed"):
1235+
await tx.cypher("RETURN 1")
1236+
1237+
asyncio.run(_inner())
1238+
1239+
def test_a_statement_during_a_commit_is_refused(self):
1240+
from unittest.mock import AsyncMock
1241+
1242+
async def _inner() -> None:
1243+
async def slow_commit(req, timeout=None):
1244+
await asyncio.sleep(0.05)
1245+
return cypher_pb2.CommitTransactionResponse(applied_index=7)
1246+
1247+
client = _async_client(CommitTransaction=AsyncMock(side_effect=slow_commit))
1248+
tx = await client.begin_transaction()
1249+
first = asyncio.create_task(tx.commit())
1250+
await asyncio.sleep(0.01)
1251+
with pytest.raises(RuntimeError, match="in flight"):
1252+
await tx.cypher("CREATE (:Late)")
1253+
assert await first == 7
1254+
assert client._cypher_stub.ExecuteCypher.await_count == 0
1255+
1256+
asyncio.run(_inner())

0 commit comments

Comments
 (0)