fix(client): raise auth exception synchronously in Client.init()#1370
fix(client): raise auth exception synchronously in Client.init()#1370xodn348 wants to merge 1 commit into
Conversation
Previously, _start_auth_task() fired authentication off in a daemon thread without joining, so any InvalidApiKeyException from fetch_auth_token was silently swallowed and Client.init() always set _initialized=True regardless of whether the key was valid. - v3.fetch_auth_token now raises InvalidApiKeyException instead of returning None when the server returns no token or the request fails outright. - _fetch_auth_async propagates exceptions rather than catching them. - _start_auth_task in synchronous contexts (no running event loop) joins the auth thread and re-raises, surfacing failures immediately to the caller. - Update test mocks to use AsyncMock for the async fetch_auth_token method, and fix the wrong patch target in test_session.py (api → client module). Closes AgentOps-AI#1336
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42e3415196
ℹ️ 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".
| self._auth_task = loop.create_task(self._fetch_auth_async(api_key)) | ||
| else: | ||
| # Create new event loop in background thread | ||
| def run_async_auth(): | ||
| asyncio.run(self._fetch_auth_async(api_key)) | ||
| return |
There was a problem hiding this comment.
Propagate auth failures when an event loop is running
When init() is called from an async context such as an ASGI startup hook or notebook, this branch only schedules _fetch_auth_async() and returns, so any InvalidApiKeyException raised by fetch_auth_token() stays on the background task while init() continues and later marks the client initialized. This means the new synchronous auth failure behavior still does not apply in running-loop environments, and invalid credentials can be silently accepted until export fails.
Useful? React with 👍 / 👎.
Summary
Fixes #1336 —
Client.init()silently succeeds with an invalid API key.Root cause:
_start_auth_task()fired authentication off in a background daemon thread without joining. Any exception raised byfetch_auth_token(e.g.InvalidApiKeyException) was swallowed inside the thread, soinit()unconditionally set_initialized = Trueand returned a session object even when the key was wrong.Changes:
agentops/client/api/versions/v3.py—fetch_auth_tokennow raisesInvalidApiKeyException(instead of returningNone) when the server returns no token or the HTTP request fails, so callers get a typed, informative error.agentops/client/client.py—_fetch_auth_asyncno longer wraps the whole body in a blanketexcept Exception: return None; exceptions propagate naturally._start_auth_taskin synchronous contexts (no running event loop) now joins the auth thread and re-raises any exception, surfacing failures immediately toinit().fetch_auth_tokenmocks fromMagicMock.return_value = dicttoAsyncMock(return_value=dict)since the method is async. Also fixed the wrong patch target intest_session.py(agentops.client.api.ApiClient→agentops.client.client.ApiClient). Addedassert not client.initializedto the invalid-key test.Before:
After:
Note on async callers: When
init()is called from inside a running event loop, the auth task is still scheduled as a backgroundasyncio.Task(blocking would deadlock the loop). This is an inherent limitation of mixing sync/async init; a future improvement could makeinit()itself async or expose anawait client.verify_auth()helper.Local test output