fix(datastore): detach rollback context from transaction cancellation#14602
Conversation
Background (How Datastore Concurrency Works)When a client updates records inside a Datastore transaction, the Datastore backend places an invisible transaction lock on those records. This prevents other transactions from making concurrent/conflicting changes. These locks must be released as quickly as possible to avoid "database traffic jams":
The BugWhen users set a transaction timeout (e.g., using a 5-second context limit) and a Commit is slow or hangs:
The SolutionThis PR fixes the bug by ensuring the Rollback RPC always goes out, even if the parent context is cancelled or expired:
|
There was a problem hiding this comment.
Code Review
This pull request updates the rollbackWithRetry method in datastore/transaction.go to use a detached context with a 5-second timeout, ensuring that rollback RPCs are executed even if the original context is cancelled or timed out. A corresponding test case, TestRunInTransaction_ContextCanceled, has been added to verify that rollbacks occur correctly under context cancellation. I have no feedback to provide.
| origCtx := t.ctx | ||
| t.ctx = rollbackCtx | ||
| defer func() { | ||
| t.ctx = origCtx |
There was a problem hiding this comment.
I understand the logic for why the rollbackCtx is needed, but why do we have to reset t.ctx back to the original context?
There was a problem hiding this comment.
- Avoid Side-Effects:
rollbackCtxis cancelled immediately afterrollbackWithRetryreturns (viadefer cancel()). If we don't restoret.ctx, theTransactionstruct would be left holding a reference to a cancelled context. - Clearer Errors on Misuse: If the transaction is somehow reused after a failed rollback (where state might not be marked as expired), restoring
origCtxensures subsequent operations run under the user's original context (which might still be active) rather than failing immediately with a confusingcontext cancelederror. - Memory Management: It prevents keeping the temporary
rollbackCtxand its wrapper in memory if theTransactionobject is held elsewhere.
PR created by the Librarian CLI to initialize a release. Merging this PR will auto trigger a release. Librarian Version: v0.13.2-0.20260518181009-04b8e642ea4c Language Image: us-central1-docker.pkg.dev/cloud-sdk-librarian-prod/images-prod/librarian-go@sha256:b04b076f5eedbb5546bd6fc1404969dd3698c8b19c0f34ae815a84ae735a606a <details><summary>datastore: v1.24.0</summary> ## [v1.24.0](datastore/v1.23.0...datastore/v1.24.0) (2026-05-19) ### Bug Fixes * detach rollback context from transaction cancellation (#14602) ([0a8d10d](0a8d10d1)) * fix context leak in Iterator and Transaction spans (#14478) ([78de20d](78de20da)) * add retries to emulator (#14591) ([d944830](d9448309)) </details>
Fix a lock contention and high-latency issue in RunInTransaction caused by failing to send Rollback commands when a context timer expires
or is canceled.
Fixes: b/346474631