fix(metrics): resolve thread-safety race condition in MetricsInterceptor#1491
fix(metrics): resolve thread-safety race condition in MetricsInterceptor#1491sinhasubham wants to merge 1 commit intomainfrom
Conversation
Summary of ChangesHello @sinhasubham, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical thread-safety regression in the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the thread-safety race condition in MetricsInterceptor by capturing the current_metrics_tracer in a local variable within the intercept method. This is a solid fix for the described bug. The addition of the test_intercept_thread_safety is excellent, as it deterministically reproduces the race condition and verifies the fix, preventing future regressions. The existing comment regarding a potential copy-paste error in the test code has been retained as it does not conflict with any provided rules.
1d5bdcb to
fd16d8a
Compare
fd16d8a to
eb2635d
Compare
Title:
fix(metrics): resolve thread-safety race condition in MetricsInterceptor
Description: This PR fixes a thread-safety regression introduced in v3.60.0 that caused AlreadyExists: 409 errors during parallel batch operations.
The Bug: The
MetricsInterceptor was accessing the global SpannerMetricsTracerFactory.current_metrics_tracer after the gRPC call completed. In concurrent scenarios, this global variable could be overwritten by another thread starting a new request, leading to the interceptor attempting to record completion on the wrong (or uninitialized) tracer. This resulted in an AttributeError crash, which the client library would catch and retry, causing a duplicate INSERT attempt and the 409 error.
The Fix:
Thread Safety: The intercept method now captures the current_metrics_tracer into a local variable before making the gRPC call. This guarantees that record_attempt_completion uses the same tracer instance that started the attempt, regardless of what other threads do.
Testing: Added test_intercept_thread_safety which uses threading and mocks to deterministically reproduce the race condition and verify the fix.