fix(spanner): guard rollback when aborted commit cleared session handle#14218
fix(spanner): guard rollback when aborted commit cleared session handle#14218
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a regression test to prevent a panic in ReadWriteTransaction.rollback. The new test case accurately simulates the scenario where an aborted commit, followed by a failed retry, could lead to a rollback attempt on a transaction with a nil session handle, which previously caused a panic. The test is well-structured and effectively covers the bug fix. I have provided one suggestion to improve the conciseness and readability of the panic check within the new test.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a potential panic in ReadWriteTransaction.rollback by adding a nil-check for the session handle. The panic could occur if a commit was aborted, the session handle was cleared, and a rollback was subsequently attempted. The change correctly prevents the nil pointer dereference by snapshotting the session handle under a lock before using it. A new test is also included, which effectively reproduces the specific conditions leading to the panic and confirms the fix. The changes are well-implemented and I have no further suggestions.
PR created by the Librarian CLI to initialize a release. Merging this PR will auto trigger a release. Librarian Version: v0.8.4 Language Image: us-central1-docker.pkg.dev/cloud-sdk-librarian-prod/images-prod/librarian-go@sha256:ac1efa3ad3c6d99efed878535b3a0fe63d0cce6701c335f03811d8b49004d652 <details><summary>spanner: v1.89.0</summary> ## [v1.89.0](spanner/v1.88.0...spanner/v1.89.0) (2026-03-26) ### Features * Add E2E fallback to the spanner client. (#13518) ([16af6a1](16af6a1c)) * include cache updates and routing hint into BeginTransaction and Commit request/response respectively (PiperOrigin-RevId: 878019893) ([9c80b8b](9c80b8b4)) * add SI, adapt, split point related proto (PiperOrigin-RevId: 871366927) ([d3eb851](d3eb851d)) * Add gRPC A66/A94 metrics (#13825) ([d695802](d695802a)) * support Scan from string to NullUUID (#14128) ([d897b6d](d897b6db)) ### Bug Fixes * replace multiplexed session request loop with shared in-flight creation (#14215) ([3e3bd2d](3e3bd2d3)) * guard rollback when aborted commit cleared session handle (#14218) ([6315105](63151055)) ### Documentation * A comment for field `routing_hint` in messages `.google.spanner.v1.ResultSet` and `.google.spanner.v1.PartialResultSet` are changed (PiperOrigin-RevId: 878019893) ([9c80b8b](9c80b8b4)) * A comment in message `.google.spanner.v1.TransactionOptions.ReadWrite.ReadLockMode` is changed (PiperOrigin-RevId: 878019893) ([9c80b8b](9c80b8b4)) * A comment for message `ListCloudInstancesAction` is changed (PiperOrigin-RevId: 871366927) ([d3eb851](d3eb851d)) * A comment for field `execution_options` in message `.google.spanner.executor.v1.StartTransactionAction` is changed (PiperOrigin-RevId: 871366927) ([d3eb851](d3eb851d)) * A comment for message `TransactionExecutionOptions` is changed (PiperOrigin-RevId: 871366927) ([d3eb851](d3eb851d)) </details>
Fixes: #14025
Fix a panic in
ReadWriteTransaction.rollbackwhen a statement-based transaction commit is aborted, commit cleanup clearst.sh, and a later rollback is triggered after retry reset fails.Root cause
ReadWriteStmtBasedTransaction.CommitWithReturnResprecycles the session handle and setst.sh = nileven when commit returnsABORTED.If a caller then tries to retry with a canceled context,
ResetForRetryfails before a new transaction is created.Cleanup paths can still call
Rollbackon the original transaction.ReadWriteTransaction.rollbackdereferencedt.shwithout checking whether the handle itself was nil, which caused a panic.Change
t.shinsideReadWriteTransaction.rollbackgetID,getClient, andgetMetadataThis keeps rollback a no-op when the session handle has already been cleaned up, which matches the intended behavior for an already-aborted transaction.