Repository navigation
perf: cache project lookups and deduplicate logins - #934
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: d8ff06f069
ℹ️ 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".
| self._start_request( | ||
| pending, | ||
| lambda: login_to_state(app_url=app_url, api_key=api_key, org_name=org_name), | ||
| timeout=_LOGIN_TIMEOUT_SECONDS, | ||
| label="login", | ||
| previous=previous, | ||
| on_success=self.copy_state, |
There was a problem hiding this comment.
When a cold BraintrustState receives concurrent non-forced logins with different API keys or organizations, both calls can pass the logged_in recheck before the first request finishes. The second worker then waits for the first and calls login_to_state directly, overwriting the first caller's state without the credential-mismatch error that the previous serialized implementation produced; the first caller can consequently resolve its logger or project against the second caller's organization. Revalidate the active credentials after previous completes, or reject a conflicting pending key unless force_login=True.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f5f4cc3a0
ℹ️ 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".
| if previous is not None: | ||
| try: | ||
| previous.result() |
There was a problem hiding this comment.
When multiple forced logins are queued behind a stalled login, each worker waits for previous.result() before starting its own timeout timer. Consequently, the second caller can block for roughly 60 seconds and later callers for N * 30 seconds, despite each login being advertised as bounded to 30 seconds. Start the deadline before the queue wait, or pass the remaining timeout into the actual login attempt.
Useful? React with 👍 / 👎.
| label="login", | ||
| previous=previous, | ||
| on_success=self.copy_state, | ||
| on_discard=lambda state: state._client.close() if state._client is not None else None, |
There was a problem hiding this comment.
When a real-key login completes after its future timed out or was canceled by a reset, this callback closes only the generated _client. However, login_to_state also creates a separate long-lived _api_conn and installs it in a background logger whose bound finalizer is retained by atexit, so the legacy session and its connection pool remain alive until process shutdown. Repeated stalled or reset logins can therefore accumulate resources; discard cleanup should close the temporary state's legacy connections as well.
Useful? React with 👍 / 👎.
…als (#2623) the bot found this while working on braintrustdata/braintrust-sdk-python#934, and I think it's legit. - Revalidate active login credentials after a queued non-forced login waits for its predecessor. - Reuse matching completed logins and reject conflicting credentials with `forceLogin: true` guidance.
5f5f4cc to
fe39be0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe39be094f
ℹ️ 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".
| timer.cancel() | ||
|
|
||
| ctx = contextvars.copy_context() | ||
| threading.Thread(target=lambda: ctx.run(run), name=f"braintrust-{label}", daemon=True).start() |
There was a problem hiding this comment.
When an adapter or transport call hangs permanently, the timer completes the future and removes its login/cache entry, but this daemon worker remains blocked inside compute(). Each subsequent retry therefore starts another native thread while the previous one is still alive, so the stalled-adapter scenario this timeout is intended to recover from can exhaust process threads after repeated retries. Run these requests through a bounded worker pool or otherwise retain/cancel the outstanding computation before permitting another worker.
Useful? React with 👍 / 👎.
python version of braintrustdata/braintrust-sdk-javascript#2523.
here is ai slop explanation:
Loggers and imported span parents currently resolve the same project independently. Share project metadata within each Braintrust state using a 1,000-entry LRU cache with a fixed 15-minute TTL starting when a lookup succeeds. Concurrent callers share the same request and receive independent metadata copies; failed lookups allow later retries.
Deduplicate matching concurrent logins while preserving fresh requests for forced logins. Bound login and project lookups to 30 seconds, cancel pending and queued logins on reset, and ignore late results. Clear project caches when login context or the HTTP adapter changes while keeping resolved logger IDs stable. Project-group registration and the generated API client remain supported.