Skip to content

test(spanner): fix IndexError in async session retry deadline unit test#17140

Merged
sakthivelmanii merged 1 commit into
mainfrom
fix-async-session-retry-indexerror
May 15, 2026
Merged

test(spanner): fix IndexError in async session retry deadline unit test#17140
sakthivelmanii merged 1 commit into
mainfrom
fix-async-session-retry-indexerror

Conversation

@sakthivelmanii
Copy link
Copy Markdown
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Root Cause

In Python 3.13 (and under recent pytest-asyncio / asyncio runtimes), the asynchronous event loop and task scheduling infrastructure makes internal calls to time.time() during coroutine execution and retry delay handling.

In test_run_in_transaction_w_abort_w_retry_metadata_deadline, time.time was mocked using a helper that popped from a fixed 2-element list on every invocation (_results.pop(0)). The extra internal event loop calls exhausted _results prematurely, deterministically raising an IndexError: pop from empty list when _delay_until_retry checked the deadline.

Solution

Updated the _time mock helper in packages/google-cloud-spanner/tests/unit/_async/test_session.py to return the last timestamp repeatedly once _results has reached its final element:

def _time(_results=[1, 1.5]):
    if len(_results) > 1:
        return _results.pop(0)
    return _results[0]

@sakthivelmanii sakthivelmanii requested a review from a team as a code owner May 14, 2026 19:19
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modifies the _time mock helper in the async session unit tests to ensure that the final timestamp is returned repeatedly once the results list is exhausted, rather than raising an error on subsequent calls. The reviewer suggested further improving this helper by explicitly checking for an empty results list and raising a ProgrammingError to provide clearer fail-fast behavior and prevent potential IndexError exceptions.

Comment on lines +1805 to +1807
if len(_results) > 1:
return _results.pop(0)
return _results[0]
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.

medium

The current implementation of the _time mock helper is fragile as it assumes _results will always contain at least one element. While the default value provides this, an empty list passed explicitly would still cause an IndexError. To ensure fail-fast behavior and prevent potential issues with missing values, the function should raise a ProgrammingError if the list is empty instead of returning a default value.

Suggested change
if len(_results) > 1:
return _results.pop(0)
return _results[0]
if not _results:
raise ProgrammingError('No time results provided')
if len(_results) > 1:
return _results.pop(0)
return _results[0]
References
  1. When a function receives parameters of an unsupported type, it should raise an error (e.g., ProgrammingError) instead of silently returning empty values. This ensures fail-fast behavior and prevents potential issues with missing parameter values in database operations.

@sakthivelmanii sakthivelmanii enabled auto-merge (squash) May 15, 2026 04:27
@sakthivelmanii sakthivelmanii merged commit c375821 into main May 15, 2026
31 checks passed
@sakthivelmanii sakthivelmanii deleted the fix-async-session-retry-indexerror branch May 15, 2026 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants