- Notifications
You must be signed in to change notification settings - Fork 87
Claude Agent SDK Thread Locals #653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @alanzhang25, 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 issue in the Claude Agent SDK integration where concurrent use of multiple tracer instances could lead to trace ID collisions due to a shared global thread-local storage. The changes refactor the thread-local storage mechanism to be specific to each tracer instance, ensuring proper isolation of tracing contexts. This prevents cross-contamination of tracing data, allowing multiple Claude Agent setups to operate simultaneously without interference. Highlights
π§ New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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.
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 effectively addresses a critical issue with context propagation when using multiple tracer instances by introducing tracer-specific thread-local storage. The approach of keying storage by tracer ID and storing the tracer on the client instance is sound. The changes are well-supported with both new unit tests for thread-local isolation and a new end-to-end test that simulates the multi-tracer scenario. I have a few suggestions to improve thread safety and maintainability.
| def _get_thread_local(tracer: "BaseTracer") -> threading.local: | ||
| """Get thread-local storage for a specific tracer instance.""" | ||
| tracer_id = id(tracer) | ||
| if tracer_id not in _tracer_thread_locals: | ||
| _tracer_thread_locals[tracer_id] = threading.local() | ||
| return _tracer_thread_locals[tracer_id] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function has a race condition. If two threads call _get_thread_local for the same new tracer instance concurrently, both could pass the if check, leading to one threading.local() object overwriting the other. This can cause unpredictable behavior and context loss.
To fix this, you should use a threading.Lock to ensure that the check and creation of the threading.local instance are atomic.
You'll need to define a lock at the module level, for example:
_thread_locals_lock = threading.Lock()And then use it within the function as shown in the suggestion.
| def _get_thread_local(tracer: "BaseTracer") -> threading.local: | |
| """Get thread-local storage for a specific tracer instance.""" | |
| tracer_id = id(tracer) | |
| if tracer_id not in _tracer_thread_locals: | |
| _tracer_thread_locals[tracer_id] = threading.local() | |
| return _tracer_thread_locals[tracer_id] | |
| def _get_thread_local(tracer: "BaseTracer") -> threading.local: | |
| """Get thread-local storage for a specific tracer instance.""" | |
| tracer_id = id(tracer) | |
| if tracer_id not in _tracer_thread_locals: | |
| with _thread_locals_lock: | |
| # Double-check locking pattern to prevent race conditions | |
| if tracer_id not in _tracer_thread_locals: | |
| _tracer_thread_locals[tracer_id] = threading.local() | |
| return _tracer_thread_locals[tracer_id] |
π Summary
β Checklist