fix(datastore): fix context leak in Iterator and Transaction spans#14478
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors context management within the datastore package to prevent the overwriting of internal struct contexts during tracing. By utilizing local context variables for spans and passing them explicitly to methods like nextBatch, the code ensures better context isolation. Feedback was provided regarding an inconsistency in beginTransaction, where a tracing span is only conditionally created, potentially leading to missing telemetry compared to the Commit and Rollback implementations.
shollyman
left a comment
There was a problem hiding this comment.
Approving as is (disabled automerge), but please consider whether we could add a test here for either validating the contexts are unmingled or that we don't end up with a deeply nested span pattern.
Added tests |
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>
Fixes #9528
This PR fixes a context leak in the
datastorelibrary where several methods ofIteratorandTransactionwere overwriting their internal context field with a new span context on every call (e.g.,t.ctx = trace.StartSpan(t.ctx, ...)).When these methods are called repeatedly (e.g., in a loop over millions of objects calling
Cursor()orGet()), the context chain grows linearly, eventually leading to a stack overflow incontext.Done()orcontext.Value()due to deep recursion.Fix is to use a local context variable for the span instead of overwriting.