From 754b1d4ae0cfb58358dc585adde2b23f704022bb Mon Sep 17 00:00:00 2001 From: Alex Castillo Date: Fri, 13 Nov 2026 03:11:38 -0400 Subject: [PATCH] fix(firestore): retry commits when the transaction has expired Firestore returns InvalidArgument, not Aborted, once a transaction id expires. Retry that message the way the Node and Java clients do, and leave every other InvalidArgument as a hard failure. --- .../cloud/firestore_v1/async_transaction.py | 10 +- .../cloud/firestore_v1/base_transaction.py | 18 ++++ .../google/cloud/firestore_v1/transaction.py | 10 +- .../tests/unit/v1/test_async_transaction.py | 91 +++++++++++++++++++ .../tests/unit/v1/test_base_transaction.py | 15 +++ .../tests/unit/v1/test_transaction.py | 91 +++++++++++++++++++ 6 files changed, 227 insertions(+), 8 deletions(-) diff --git a/packages/google-cloud-firestore/google/cloud/firestore_v1/async_transaction.py b/packages/google-cloud-firestore/google/cloud/firestore_v1/async_transaction.py index 4971c1ea023e..d47b94c76555 100644 --- a/packages/google-cloud-firestore/google/cloud/firestore_v1/async_transaction.py +++ b/packages/google-cloud-firestore/google/cloud/firestore_v1/async_transaction.py @@ -44,6 +44,7 @@ MAX_ATTEMPTS, BaseTransaction, _BaseTransactional, + _is_retryable_commit_error, ) # Types needed only for Type Hints @@ -329,9 +330,6 @@ async def __call__( ``max_attempts``. """ self._reset() - retryable_exceptions = ( - (exceptions.Aborted) if not transaction._read_only else () - ) last_exc = None try: @@ -340,7 +338,11 @@ async def __call__( try: await transaction._commit() return result - except retryable_exceptions as exc: + except (exceptions.Aborted, exceptions.InvalidArgument) as exc: + # Read-only transactions are never retried. InvalidArgument + # restarts the transaction only when the id has expired. + if transaction._read_only or not _is_retryable_commit_error(exc): + raise last_exc = exc # Retry attempts that result in retryable exceptions # Subsequent requests will use the failed transaction ID as part of diff --git a/packages/google-cloud-firestore/google/cloud/firestore_v1/base_transaction.py b/packages/google-cloud-firestore/google/cloud/firestore_v1/base_transaction.py index dc290b2e94d8..fd16147643af 100644 --- a/packages/google-cloud-firestore/google/cloud/firestore_v1/base_transaction.py +++ b/packages/google-cloud-firestore/google/cloud/firestore_v1/base_transaction.py @@ -26,6 +26,7 @@ Union, ) +from google.api_core import exceptions from google.api_core import retry as retries from google.cloud.firestore_v1 import types @@ -50,6 +51,23 @@ _WRITE_READ_ONLY: str = "Cannot perform write operation in read-only transaction." _EXCEED_ATTEMPTS_TEMPLATE: str = "Failed to commit transaction in {:d} attempts." _CANT_RETRY_READ_ONLY: str = "Only read-write transactions can be retried." +# Firestore reports an expired transaction id as InvalidArgument. That status +# is not generally retryable; this substring is, matching the Node and Java +# clients (error.message / getMessage() contains "transaction has expired"). +_EXPIRED_TRANSACTION_MESSAGE = "transaction has expired" + + +def _is_retryable_commit_error(exc: BaseException) -> bool: + """Return whether a commit failure should restart the transaction. + + ``Aborted`` is always retryable. ``InvalidArgument`` is retryable only + when the message says the transaction has expired. + """ + if isinstance(exc, exceptions.Aborted): + return True + if isinstance(exc, exceptions.InvalidArgument): + return _EXPIRED_TRANSACTION_MESSAGE in str(exc) + return False class BaseTransaction(object): diff --git a/packages/google-cloud-firestore/google/cloud/firestore_v1/transaction.py b/packages/google-cloud-firestore/google/cloud/firestore_v1/transaction.py index 1d2cde079b48..0aafeb17b802 100644 --- a/packages/google-cloud-firestore/google/cloud/firestore_v1/transaction.py +++ b/packages/google-cloud-firestore/google/cloud/firestore_v1/transaction.py @@ -31,6 +31,7 @@ MAX_ATTEMPTS, BaseTransaction, _BaseTransactional, + _is_retryable_commit_error, ) from google.cloud.firestore_v1.document import DocumentReference from google.cloud.firestore_v1.query import Query @@ -305,9 +306,6 @@ def __call__(self, transaction: Transaction, *args, **kwargs): ``max_attempts``. """ self._reset() - retryable_exceptions = ( - (exceptions.Aborted) if not transaction._read_only else () - ) last_exc = None try: @@ -316,7 +314,11 @@ def __call__(self, transaction: Transaction, *args, **kwargs): try: transaction._commit() return result - except retryable_exceptions as exc: + except (exceptions.Aborted, exceptions.InvalidArgument) as exc: + # Read-only transactions are never retried. InvalidArgument + # restarts the transaction only when the id has expired. + if transaction._read_only or not _is_retryable_commit_error(exc): + raise last_exc = exc # Retry attempts that result in retryable exceptions # Subsequent requests will use the failed transaction ID as part of diff --git a/packages/google-cloud-firestore/tests/unit/v1/test_async_transaction.py b/packages/google-cloud-firestore/tests/unit/v1/test_async_transaction.py index a09dd1c5b9ef..88affb9333a6 100644 --- a/packages/google-cloud-firestore/tests/unit/v1/test_async_transaction.py +++ b/packages/google-cloud-firestore/tests/unit/v1/test_async_transaction.py @@ -668,6 +668,97 @@ async def test_asynctransactional___call__success_second_attempt(): assert firestore_api.commit.mock_calls == [commit_call, commit_call] +@pytest.mark.asyncio +async def test_asynctransactional___call__success_second_attempt_expired_transaction(): + from google.api_core import exceptions + + from google.cloud.firestore_v1.types import common, firestore, write + + to_wrap = AsyncMock(return_value=mock.sentinel.result, spec=[]) + wrapped = _make_async_transactional(to_wrap) + + txn_id = b"whole-enchilada" + transaction = _make_transaction(txn_id) + + # An expired transaction id is InvalidArgument, not Aborted. + exc = exceptions.InvalidArgument( + "The referenced transaction has expired or is no longer valid." + ) + firestore_api = transaction._client._firestore_api + firestore_api.commit.side_effect = [ + exc, + firestore.CommitResponse(write_results=[write.WriteResult()]), + ] + + result = await wrapped(transaction, "a", b="c") + assert result is mock.sentinel.result + + assert transaction._id is None + assert wrapped.current_id == txn_id + assert wrapped.retry_id == txn_id + + wrapped_call = mock.call(transaction, "a", b="c") + assert to_wrap.mock_calls == [wrapped_call, wrapped_call] + db_str = transaction._client._database_string + options_ = common.TransactionOptions( + read_write=common.TransactionOptions.ReadWrite(retry_transaction=txn_id) + ) + expected_calls = [ + mock.call( + request={"database": db_str, "options": None}, + metadata=transaction._client._rpc_metadata, + ), + mock.call( + request={"database": db_str, "options": options_}, + metadata=transaction._client._rpc_metadata, + ), + ] + assert firestore_api.begin_transaction.mock_calls == expected_calls + firestore_api.rollback.assert_not_called() + commit_call = mock.call( + request={"database": db_str, "writes": [], "transaction": txn_id}, + metadata=transaction._client._rpc_metadata, + ) + assert firestore_api.commit.mock_calls == [commit_call, commit_call] + + +@pytest.mark.asyncio +async def test_asynctransactional___call__invalid_argument_not_expired_does_not_retry(): + from google.api_core import exceptions + + to_wrap = AsyncMock(return_value=mock.sentinel.result, spec=[]) + wrapped = _make_async_transactional(to_wrap) + + txn_id = b"not-expired" + transaction = _make_transaction(txn_id) + + exc = exceptions.InvalidArgument("other") + firestore_api = transaction._client._firestore_api + firestore_api.commit.side_effect = exc + + with pytest.raises(exceptions.InvalidArgument) as exc_info: + await wrapped(transaction, "a", b="c") + + assert exc_info.value is exc + to_wrap.assert_called_once_with(transaction, "a", b="c") + firestore_api.commit.assert_called_once_with( + request={ + "database": transaction._client._database_string, + "writes": [], + "transaction": txn_id, + }, + metadata=transaction._client._rpc_metadata, + ) + firestore_api.rollback.assert_called_once_with( + request={ + "database": transaction._client._database_string, + "transaction": txn_id, + }, + metadata=transaction._client._rpc_metadata, + ) + firestore_api.begin_transaction.assert_called_once() + + @pytest.mark.parametrize("max_attempts", [1, 5]) @pytest.mark.asyncio async def test_asynctransactional___call__failure_max_attempts(max_attempts): diff --git a/packages/google-cloud-firestore/tests/unit/v1/test_base_transaction.py b/packages/google-cloud-firestore/tests/unit/v1/test_base_transaction.py index db5dbd92a83f..5da33d1fedfa 100644 --- a/packages/google-cloud-firestore/tests/unit/v1/test_base_transaction.py +++ b/packages/google-cloud-firestore/tests/unit/v1/test_base_transaction.py @@ -115,3 +115,18 @@ def test__basetransactional_reset(): assert wrapped.current_id is None assert wrapped.retry_id is None + + +def test_is_retryable_commit_error(): + from google.api_core import exceptions + + from google.cloud.firestore_v1.base_transaction import _is_retryable_commit_error + + assert _is_retryable_commit_error(exceptions.Aborted("Contention.")) + assert _is_retryable_commit_error( + exceptions.InvalidArgument( + "The referenced transaction has expired or is no longer valid." + ) + ) + assert not _is_retryable_commit_error(exceptions.InvalidArgument("other")) + assert not _is_retryable_commit_error(ValueError("transaction has expired")) diff --git a/packages/google-cloud-firestore/tests/unit/v1/test_transaction.py b/packages/google-cloud-firestore/tests/unit/v1/test_transaction.py index 7a93615b3ea4..08b406c25b2c 100644 --- a/packages/google-cloud-firestore/tests/unit/v1/test_transaction.py +++ b/packages/google-cloud-firestore/tests/unit/v1/test_transaction.py @@ -687,6 +687,97 @@ def test__transactional___call__success_second_attempt(database): assert firestore_api.commit.mock_calls == [commit_call, commit_call] +@pytest.mark.parametrize("database", [None, "somedb"]) +def test__transactional___call__success_second_attempt_expired_transaction(database): + from google.api_core import exceptions + + from google.cloud.firestore_v1.types import common, firestore, write + + to_wrap = mock.Mock(return_value=mock.sentinel.result, spec=[]) + wrapped = _make__transactional(to_wrap) + + txn_id = b"whole-enchilada" + transaction = _make_transaction_pb(txn_id, database=database) + + # An expired transaction id is InvalidArgument, not Aborted. + exc = exceptions.InvalidArgument( + "The referenced transaction has expired or is no longer valid." + ) + firestore_api = transaction._client._firestore_api + firestore_api.commit.side_effect = [ + exc, + firestore.CommitResponse(write_results=[write.WriteResult()]), + ] + + result = wrapped(transaction, "a", b="c") + assert result is mock.sentinel.result + + assert transaction._id is None + assert wrapped.current_id == txn_id + assert wrapped.retry_id == txn_id + + wrapped_call = mock.call(transaction, "a", b="c") + assert to_wrap.mock_calls == [wrapped_call, wrapped_call] + db_str = transaction._client._database_string + options_ = common.TransactionOptions( + read_write=common.TransactionOptions.ReadWrite(retry_transaction=txn_id) + ) + expected_calls = [ + mock.call( + request={"database": db_str, "options": None}, + metadata=transaction._client._rpc_metadata, + ), + mock.call( + request={"database": db_str, "options": options_}, + metadata=transaction._client._rpc_metadata, + ), + ] + assert firestore_api.begin_transaction.mock_calls == expected_calls + firestore_api.rollback.assert_not_called() + commit_call = mock.call( + request={"database": db_str, "writes": [], "transaction": txn_id}, + metadata=transaction._client._rpc_metadata, + ) + assert firestore_api.commit.mock_calls == [commit_call, commit_call] + + +@pytest.mark.parametrize("database", [None, "somedb"]) +def test__transactional___call__invalid_argument_not_expired_does_not_retry(database): + from google.api_core import exceptions + + to_wrap = mock.Mock(return_value=mock.sentinel.result, spec=[]) + wrapped = _make__transactional(to_wrap) + + txn_id = b"not-expired" + transaction = _make_transaction_pb(txn_id, database=database) + + exc = exceptions.InvalidArgument("other") + firestore_api = transaction._client._firestore_api + firestore_api.commit.side_effect = exc + + with pytest.raises(exceptions.InvalidArgument) as exc_info: + wrapped(transaction, "a", b="c") + + assert exc_info.value is exc + to_wrap.assert_called_once_with(transaction, "a", b="c") + firestore_api.commit.assert_called_once_with( + request={ + "database": transaction._client._database_string, + "writes": [], + "transaction": txn_id, + }, + metadata=transaction._client._rpc_metadata, + ) + firestore_api.rollback.assert_called_once_with( + request={ + "database": transaction._client._database_string, + "transaction": txn_id, + }, + metadata=transaction._client._rpc_metadata, + ) + firestore_api.begin_transaction.assert_called_once() + + @pytest.mark.parametrize("database", [None, "somedb"]) @pytest.mark.parametrize("max_attempts", [1, 5]) def test_transactional___call__failure_max_attempts(database, max_attempts):