Repository navigation
fix(firestore): retry commits when the transaction has expired - #19066
pcbeingused333 wants to merge 1 commit into
Conversation
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.
There was a problem hiding this comment.
Code Review
This pull request adds support for retrying transactions when they fail due to an expired transaction ID, which Firestore reports as an InvalidArgument exception. This is implemented by introducing a helper function _is_retryable_commit_error that checks for Aborted exceptions or InvalidArgument exceptions containing the substring 'transaction has expired', and integrating this check into both synchronous and asynchronous transaction commit loops. Comprehensive unit tests are also added. The review feedback recommends making the substring check case-insensitive to ensure robustness against potential casing variations in the backend error message.
| if isinstance(exc, exceptions.InvalidArgument): | ||
| return _EXPIRED_TRANSACTION_MESSAGE in str(exc) |
There was a problem hiding this comment.
To make the check robust against potential casing variations in the backend error message (e.g., "Transaction has expired"), it is safer to perform a case-insensitive substring search by converting the exception string to lowercase.
| if isinstance(exc, exceptions.InvalidArgument): | |
| return _EXPIRED_TRANSACTION_MESSAGE in str(exc) | |
| if isinstance(exc, exceptions.InvalidArgument): | |
| return _EXPIRED_TRANSACTION_MESSAGE in str(exc).lower() |
Fixes #19063
Root cause.
_Transactional.__call__and_AsyncTransactional.__call__retry a commit only when it raisesAborted. Once a Firestore transaction id expires, the backend returnsInvalidArgumentwith the message "The referenced transaction has expired or is no longer valid." That error falls through to rollback, so the transaction fails instead of restarting. The Node and Java clients retry this specific message.Fix. A commit that raises
Aborted, orInvalidArgumentwhose text contains "transaction has expired", now restarts the transaction with the same retry id already used forAborted. Any otherInvalidArgumentis still raised immediately and the transaction is rolled back. Read-only transactions are not retried. Backoff and the attempt limit are unchanged. The check is_is_retryable_commit_error, shared by the sync and async wrappers.Test. The existing second-attempt success case is repeated with the first commit raising
InvalidArgument("The referenced transaction has expired or is no longer valid."): two commits, no rollback, and the wrapped callable runs twice. A second case raisesInvalidArgument("other")and asserts one commit plus rollback. The same pair covers the async wrapper.This PR was prepared with the help of an AI assistant; the changes were tested as described above.