Skip to content

test(spanner): unflake testCreateSessionDeadlineExceededWithNoSessionCreateWaitTime - #14866

Open
sakthivelmanii wants to merge 3 commits into
mainfrom
unflake-testcreatesessiondeadlineexceededwithnosessioncr
Open

sakthivelmanii wants to merge 3 commits into
mainfrom
unflake-testcreatesessiondeadlineexceededwithnosessioncr

Conversation

@sakthivelmanii

Copy link
Copy Markdown
Contributor

Problem

  • In MultiplexedSessionDatabaseClient, the initial future was exceptionally completed with setException(t) before multiplexedSessionReference was updated to the retry future, letting waiters observe the stale error.

Solution

  • Reorder the logic in onSessionCreateFailure to instantiate and install the retry future into multiplexedSessionReference before exceptionally completing the current attempt.
  • Add testRetryOfInitialSessionIsInstalledBeforeFailureIsPropagated to prevent regressions.

…CreateWaitTime

The initial future was completed with setException(t) before multiplexedSessionReference was updated to the retry future. Waiters waking up and polling the reference encountered the stale completed-with-error future. This fix installs the retry future before completing the initial future exceptionally to eliminate the race.
@sakthivelmanii
sakthivelmanii enabled auto-merge (squash) October 30, 2026 10:16
@sakthivelmanii
sakthivelmanii requested a review from a team as a code owner October 30, 2026 10:16
@sakthivelmanii

Copy link
Copy Markdown
Contributor Author

/gemini review

@sakthivelmanii
sakthivelmanii requested a review from a team as a code owner October 30, 2026 10:16

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

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.

Code Review

This pull request modifies the multiplexed session creation failure handling in MultiplexedSessionDatabaseClient to install the retry future before propagating the failure to existing waiters, preventing subsequent transactions from failing prematurely. A unit test has been added to verify this behavior. The reviewer recommends wrapping the asyncCreateMultiplexedSession call in a try-catch block to exceptionally complete the newly installed future if a synchronous exception is thrown, preventing potential deadlocks.

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

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.

Code Review

This pull request ensures that when a multiplexed session creation fails, the future for the retry attempt is installed before the failure exception is propagated to existing waiters. This prevents new transactions initiated immediately after the failure from failing with the previous retryable error. A corresponding unit test has been added to verify this behavior. The reviewer suggested a refactoring to simplify the conditional logic and avoid duplicating the exception-setting call.

@sakthivelmanii

Copy link
Copy Markdown
Contributor Author

/gemini review

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

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.

Code Review

This pull request modifies the session creation failure handling in MultiplexedSessionDatabaseClient to install the retry future before propagating the exception, ensuring subsequent transactions wait for the retry instead of failing immediately. A corresponding unit test has also been added. The review feedback recommends using compareAndSet instead of set when updating the multiplexed session reference to prevent potential race conditions, and wrapping the test client in a try-finally block to guarantee proper resource cleanup in case of test failures.

@sakthivelmanii

Copy link
Copy Markdown
Contributor Author

/gemini review

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

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.

Code Review

This pull request updates MultiplexedSessionDatabaseClient to install a retry future for multiplexed session creation before propagating the failure of the current attempt, preventing subsequent transactions from failing with the previous error. A corresponding unit test has been added to verify this behavior. The review feedback points out a critical issue where a synchronous exception thrown during the retry initiation could leave the new future uncompleted indefinitely, and suggests wrapping the call in a try-catch block to safely propagate the exception.

@sakthivelmanii

Copy link
Copy Markdown
Contributor Author

/gemini review

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

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.

Code Review

This pull request updates the multiplexed session creation retry logic in MultiplexedSessionDatabaseClient to install the retry future before propagating the failure of the current attempt, preventing subsequent transactions from failing with the retryable error. A new unit test was added to verify this behavior. The review feedback suggests wrapping the asynchronous session creation call in a try-catch block to handle potential synchronous exceptions, ensuring the newly installed future is completed exceptionally rather than hanging indefinitely.

@sakthivelmanii

Copy link
Copy Markdown
Contributor Author

/gemini review

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

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.

Code Review

This pull request modifies MultiplexedSessionDatabaseClient to ensure that when a multiplexed session creation fails, the retry future is installed before the failure is propagated to waiters. This prevents subsequent transactions from failing with the transient error of the previous attempt, forcing them to wait for the retry instead. A corresponding unit test has been added to verify this behavior. There are no review comments to address, and I have no additional feedback to provide.

This branch has not been deployed

No deployments
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.

1 participant