fix: eliminate process.env race condition in GoogleLlm and AiSdkLlm#658
fix: eliminate process.env race condition in GoogleLlm and AiSdkLlm#658brianmhunt wants to merge 3 commits intoIQAIcom:mainfrom
Conversation
GoogleLlm, AiSdkLlm, and GeminiContextCacheManager lazily read process.env to build Google clients. In a multi-tenant server, concurrent requests needing different backends (Vertex AI vs API key) race on the shared mutable process.env. Add GoogleLlmConfig to allow explicit, request-scoped configuration (apiKey, vertexai/project/location, or a pre-built client) that bypasses env vars entirely. Env fallback is preserved when no config is provided, so the change is fully backward compatible. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: a800cc3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@brianmhunt is attempting to deploy a commit to the IQ Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello, 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 resolves a critical race condition in multi-tenant server environments where Highlights
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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the AiSdkLlm and GoogleLlm classes to introduce explicit configuration options, primarily to address potential race conditions in multi-tenant server environments. This is achieved by allowing pre-built GoogleGenAI clients or specific API configurations to be injected directly rather than relying solely on environment variables. Key changes include adding AiSdkLlmOptions and GoogleLlmConfig interfaces, updating constructors to accept these options, and centralizing client building logic in GoogleLlm to prioritize explicit configurations. All internal private fields have been converted to use the # private class field syntax. Corresponding test files have been updated with new test cases to validate these explicit configuration options and ensure backward compatibility and race condition safety. I have no feedback to provide.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83c11e3f42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (this.#config?.vertexai === true) { | ||
| this.#apiBackend = GoogleLLMVariant.VERTEX_AI; | ||
| } else if (this.#config) { | ||
| this.#apiBackend = GoogleLLMVariant.GEMINI_API; |
There was a problem hiding this comment.
apiBackend treats any config with vertexai: true as VERTEX_AI, but #buildClient only creates a Vertex client when both project and location are present. With inputs like { vertexai: true, apiKey: "..." }, the instance ends up using a Gemini API-key client while still reporting VERTEX_AI, so preprocessRequest skips Gemini-specific sanitization (labels/displayName) and requests can fail against the API-key backend.
Useful? React with 👍 / 👎.
| // 3. Env fallback (existing behaviour) | ||
| const useVertexAI = process.env.GOOGLE_GENAI_USE_VERTEXAI === "true"; | ||
| const apiKey = process.env.GOOGLE_API_KEY; | ||
| const project = process.env.GOOGLE_CLOUD_PROJECT; | ||
| const location = process.env.GOOGLE_CLOUD_LOCATION; |
There was a problem hiding this comment.
When a GoogleLlmConfig object is provided but missing required fields, #buildClient falls through to environment-variable selection. That silently reintroduces the shared-process.env behavior this change is intended to avoid (for example, { vertexai: true, project: "p" } can unexpectedly pick env credentials/backend instead of failing fast), which can route a request to the wrong backend in multi-tenant usage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Interesting, I see codex picked them. Yep, I think these are significant
Address review feedback: - When GoogleLlmConfig is provided but incomplete (e.g. vertexai without project/location), throw instead of silently falling through to process.env (which would reintroduce the race condition) - Align apiBackend getter with #buildClient: require vertexai + project + location to report VERTEX_AI, not just vertexai flag Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
hi @brianmhunt , thank you for contributing to ADK-TS. 🎉 your pr will be reviewed soon by one of our developers. |
MarvelNwachukwu
left a comment
MarvelNwachukwu
left a comment
There was a problem hiding this comment.
Hey @brianmhunt, two things came up.
- Pre-built Vertex clients get misclassified
If someone passes { client: vertexClient } without also setting vertexai: true + project + location, the apiBackend getter falls through to GEMINI_API. The #buildClient() path does the right thing and returns the injected client, but apiBackend has no idea it's actually a Vertex client.
That's not just cosmetic. preprocessRequest() checks apiBackend to decide whether to strip labels and display names, and liveApiVersion uses it to pick between v1alpha and v1beta1. So you end up with a Vertex client getting the Gemini API treatment, which will quietly break things.
Probably the simplest fix is adding a backend field to GoogleLlmConfig so callers can declare what their pre-built client actually is. Or have the apiBackend getter refuse to guess when it sees { client: ... } without an explicit backend.
- The AiSdkLlm cache manager test is a false positive
The test at ai-sdk.test.ts:340 consumes the generator and then asserts GeminiContextCacheManager was constructed with the right args. But generateContentAsyncImpl has a catch block (line305) that swallows non-rate-limit errors and yields an error response instead of throwing. So if the mock isn't wired up right and handleContextCaching throws a TypeError, the test still passes because the generator just quietly yields an error and moves on. The assertion on the constructor call succeeds, but the code path it's meant to verify never actually ran.
Should either assert that the yielded response isn't an error, or check that handleContextCaching was actually called successfully.
One more thing: the changeset should be patch, not minor.
3 participants
Summary
GoogleLlmConfiginterface toGoogleLlmallowing explicit, request-scoped configuration (apiKey,vertexai/project/location, or a pre-builtGoogleGenAIclient) that bypassesprocess.enventirelyAiSdkLlmOptionswithgoogleGenaiClienttoAiSdkLlm, passed through toGeminiContextCacheManagerfor the same purposeprivate _fieldto JS#privatefields and extract shared#buildClient()method to DRY client construction acrossapiClient,liveApiClientMotivation
GoogleLlm,AiSdkLlm, andGeminiContextCacheManagerall lazily readprocess.envat request time to construct Google clients. In a multi-tenant server, concurrent requests needing different Google backends (Vertex AI vs API key) race on the shared mutableprocess.env, making request-scoped provider selection unsafe.All changes are fully backward compatible — env fallback is preserved when no config is provided.
Test plan
apiKeyconfig bypasses envvertexaiconfig bypasses envclientinjection used directly (for bothapiClientandliveApiClient)apiBackendresolves from config when provided, env when notGoogleLlminstances with different configs each resolve their own client independentlyAiSdkLlmgoogleGenaiClientoption passed through toGeminiContextCacheManager🤖 Generated with Claude Code