Skip to content

fix: Use apiKey in loadPrompt - #718

Open
Abhijeet Prasad (AbhiPrasad) wants to merge 1 commit into
mainfrom
abhi-load-prompt-api-key
Open

fix: Use apiKey in loadPrompt#718
Abhijeet Prasad (AbhiPrasad) wants to merge 1 commit into
mainfrom
abhi-load-prompt-api-key

Conversation

@AbhiPrasad

Copy link
Copy Markdown
Member

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0afb95d561

ℹ️ 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 (@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 (@codex) address that feedback".

try:
conn = _authenticated_api_conn(login_result.api_url, api_key)
finally:
client.close()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not reuse an adapter after closing its owner

When set_http_adapter() has installed a custom adapter and a loader uses a separate credential, both the temporary BraintrustClient and _authenticated_api_conn() mount the same _http_adapter instance. This finally closes the client's session, which closes its mounted adapter before the returned connection makes the prompt/parameters request; adapters that release non-recreatable resources on close() will therefore fail, and later cache eviction can also close the adapter used by global connections. Give the loader connection separate adapter ownership or avoid closing an adapter that remains shared.

Useful? React with 👍 / 👎.

Comment on lines +752 to +753
self._loader_login_cache.set(cache_namespace, lazy_state)
return lazy_state.get()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Close loader states evicted while authentication is pending

When more than 16 distinct credentials authenticate concurrently, inserting a new LazyValue can evict one whose authentication is still running. The removal callback sees get_sync() report an unresolved value and does nothing, but this subsequent get() can then finish creating a connection after the entry is no longer tracked, so no reset or later eviction will close it. Repeated multi-tenant bursts can consequently bypass the intended bound and leak sessions; pending evictions need to arrange cleanup after resolution.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant