From 5a5bfca834718685064f090a2e07fad2f09b98b8 Mon Sep 17 00:00:00 2001 From: Amine Hmida Date: Sat, 15 Aug 2026 17:46:27 +0100 Subject: [PATCH 1/3] fix(client): lock async session setup against concurrent callers --- src/simple_github/client.py | 54 +++++++++++++++++++++---------------- test/test_auth.py | 26 +++++++++++++++++- test/test_client.py | 12 +++++++++ 3 files changed, 68 insertions(+), 24 deletions(-) diff --git a/src/simple_github/client.py b/src/simple_github/client.py index 16650e7..94262ea 100644 --- a/src/simple_github/client.py +++ b/src/simple_github/client.py @@ -242,6 +242,10 @@ def execute(self, query: str, variables: RequestData = None) -> dict[str, Any]: class AsyncClient(Client): + def __init__(self, auth: "Auth"): + super().__init__(auth) + self._session_lock = asyncio.Lock() + async def __aenter__(self): return self @@ -265,32 +269,36 @@ async def _get_gql_session(self) -> ReconnectingAsyncClientSession: Returns: aiohttp.ClientSession: An AIOHTTP session object. """ - token = await self.get_token() - if token == self._prev_token: + # The lock spans `get_token()` because `AppInstallationAuth` drives an + # async generator, which two callers cannot advance at once. + async with self._session_lock: + token = await self.get_token() + + if token == self._prev_token: + assert isinstance(self._gql_session, ReconnectingAsyncClientSession) + return self._gql_session + + # Create a new session with updated token. + self._prev_token = token + if self._gql_client: + await self._gql_client.close_async() + + headers = { + "Accept": "application/vnd.github+json", + } + if token: + headers["Authorization"] = f"Bearer {token}" + + transport = AIOHTTPTransport( + url=GITHUB_GRAPHQL_ENDPOINT, headers=headers, ssl=True + ) + self._gql_client = GqlClient( + transport=transport, fetch_schema_from_transport=False + ) + self._gql_session = await self._gql_client.connect_async(reconnecting=True) assert isinstance(self._gql_session, ReconnectingAsyncClientSession) return self._gql_session - # Create a new session with updated token. - self._prev_token = token - if self._gql_client: - await self._gql_client.close_async() - - headers = { - "Accept": "application/vnd.github+json", - } - if token: - headers["Authorization"] = f"Bearer {token}" - - transport = AIOHTTPTransport( - url=GITHUB_GRAPHQL_ENDPOINT, headers=headers, ssl=True - ) - self._gql_client = GqlClient( - transport=transport, fetch_schema_from_transport=False - ) - self._gql_session = await self._gql_client.connect_async(reconnecting=True) - assert isinstance(self._gql_session, ReconnectingAsyncClientSession) - return self._gql_session - async def _get_aiohttp_session(self) -> ClientSession: session = await self._get_gql_session() assert isinstance(session.transport, AIOHTTPTransport) diff --git a/test/test_auth.py b/test/test_auth.py index 81dec85..f506de8 100644 --- a/test/test_auth.py +++ b/test/test_auth.py @@ -1,3 +1,4 @@ +import asyncio import time from unittest import mock @@ -5,7 +6,7 @@ import pytest from simple_github.auth import AppAuth, AppInstallationAuth, PublicAuth, TokenAuth -from simple_github.client import GITHUB_API_ENDPOINT +from simple_github.client import GITHUB_API_ENDPOINT, AsyncClient @pytest.mark.asyncio @@ -83,3 +84,26 @@ async def test_app_installation_auth_get_token( with mock.patch.object(time, "time", return_value=cur + 3600 - 59): new_token = await auth.get_token() assert new_token != token + + +@pytest.mark.asyncio +async def test_concurrent_app_installation_auth(aioresponses, privkey: str): + """Concurrent callers must not advance the token generator at once.""" + inst_id = 100 + owner = "mozilla" + + aioresponses.get( + f"{GITHUB_API_ENDPOINT}/app/installations", + status=200, + payload=[{"id": inst_id, "account": {"login": owner}}], + ) + aioresponses.post( + f"{GITHUB_API_ENDPOINT}/app/installations/{inst_id}/access_tokens", + status=200, + payload={"token": "111"}, + ) + aioresponses.get(f"{GITHUB_API_ENDPOINT}/octocat", payload={}, repeat=True) + + client = AsyncClient(auth=AppInstallationAuth(AppAuth(42, privkey), owner)) + await asyncio.gather(*(client.get("/octocat") for _ in range(2))) + await client.close() diff --git a/test/test_client.py b/test/test_client.py index b071509..b8cf258 100644 --- a/test/test_client.py +++ b/test/test_client.py @@ -1,3 +1,4 @@ +import asyncio from unittest import mock import pytest @@ -60,6 +61,17 @@ async def test_async_client_get_session(async_client): } +@pytest.mark.asyncio +async def test_async_client_get_session_concurrent(async_client): + """Concurrent callers all get a session, none observe a partial setup.""" + client = async_client + + sessions = await asyncio.gather(*(client._get_aiohttp_session() for _ in range(25))) + + assert isinstance(client._gql_session, ReconnectingAsyncClientSession) + assert all(s == sessions[0] for s in sessions) + + @pytest.mark.asyncio async def test_async_client_get_session_no_token(async_client): client = async_client From d07e001accbd349d18fe69ed1afce58b2765a872 Mon Sep 17 00:00:00 2001 From: Amine Hmida Date: Mon, 17 Aug 2026 11:17:05 +0100 Subject: [PATCH 2/3] fix(client): only publish async session state once it is valid --- src/simple_github/client.py | 26 ++++++++++++++++---------- test/test_client.py | 20 ++++++++++++++++++++ 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/src/simple_github/client.py b/src/simple_github/client.py index 94262ea..14a0925 100644 --- a/src/simple_github/client.py +++ b/src/simple_github/client.py @@ -278,10 +278,14 @@ async def _get_gql_session(self) -> ReconnectingAsyncClientSession: assert isinstance(self._gql_session, ReconnectingAsyncClientSession) return self._gql_session - # Create a new session with updated token. - self._prev_token = token - if self._gql_client: - await self._gql_client.close_async() + # Drop the old session before building the new one, so a failure + # below leaves nothing cached and the next caller retries. + prev_client = self._gql_client + self._prev_token = None + self._gql_client = None + self._gql_session = None + if prev_client: + await prev_client.close_async() headers = { "Accept": "application/vnd.github+json", @@ -292,12 +296,14 @@ async def _get_gql_session(self) -> ReconnectingAsyncClientSession: transport = AIOHTTPTransport( url=GITHUB_GRAPHQL_ENDPOINT, headers=headers, ssl=True ) - self._gql_client = GqlClient( - transport=transport, fetch_schema_from_transport=False - ) - self._gql_session = await self._gql_client.connect_async(reconnecting=True) - assert isinstance(self._gql_session, ReconnectingAsyncClientSession) - return self._gql_session + client = GqlClient(transport=transport, fetch_schema_from_transport=False) + session = await client.connect_async(reconnecting=True) + assert isinstance(session, ReconnectingAsyncClientSession) + + self._gql_client = client + self._gql_session = session + self._prev_token = token + return session async def _get_aiohttp_session(self) -> ClientSession: session = await self._get_gql_session() diff --git a/test/test_client.py b/test/test_client.py index b8cf258..59fa537 100644 --- a/test/test_client.py +++ b/test/test_client.py @@ -82,6 +82,26 @@ async def test_async_client_get_session_no_token(async_client): } +@pytest.mark.asyncio +async def test_async_client_recovers_from_connection_failure(async_client): + """A failed connect leaves nothing cached, so the next call retries.""" + client = async_client + + with mock.patch.object( + GqlClient, "connect_async", side_effect=OSError("no route to host") + ): + with pytest.raises(OSError): + await client._get_gql_session() + + assert client._prev_token is None + + session = await client._get_aiohttp_session() + assert dict(session._default_headers) == { + "Accept": "application/vnd.github+json", + "Authorization": f"Bearer {client.auth._token}", + } + + def test_sync_client_get_session(sync_client): client = sync_client assert client._gql_client is None From 85a44654f859800aa647eccc9159efe7cac627be Mon Sep 17 00:00:00 2001 From: Amine Hmida Date: Mon, 17 Aug 2026 11:18:46 +0100 Subject: [PATCH 3/3] fix(client): only publish sync session state once it is valid --- src/simple_github/client.py | 26 ++++++++++++++++---------- test/test_client.py | 19 +++++++++++++++++++ 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/simple_github/client.py b/src/simple_github/client.py index 14a0925..239c3e2 100644 --- a/src/simple_github/client.py +++ b/src/simple_github/client.py @@ -112,10 +112,14 @@ def _get_gql_session(self) -> SyncClientSession: assert isinstance(self._gql_session, SyncClientSession) return self._gql_session - # Create a new session with updated token. - self._prev_token = token - if self._gql_client: - self._gql_client.close_sync() + # Drop the old session before building the new one, so a failure below + # leaves nothing cached and the next caller retries. + prev_client = self._gql_client + self._prev_token = None + self._gql_client = None + self._gql_session = None + if prev_client: + prev_client.close_sync() headers = { "Accept": "application/vnd.github+json", @@ -124,12 +128,14 @@ def _get_gql_session(self) -> SyncClientSession: headers["Authorization"] = f"Bearer {token}" transport = RequestsHTTPTransport(url=GITHUB_GRAPHQL_ENDPOINT, headers=headers) - self._gql_client = GqlClient( - transport=transport, fetch_schema_from_transport=False - ) - self._gql_session = self._gql_client.connect_sync() - assert isinstance(self._gql_session, SyncClientSession) - return self._gql_session + client = GqlClient(transport=transport, fetch_schema_from_transport=False) + session = client.connect_sync() + assert isinstance(session, SyncClientSession) + + self._gql_client = client + self._gql_session = session + self._prev_token = token + return session def _get_requests_session(self) -> Session: session = self._get_gql_session() diff --git a/test/test_client.py b/test/test_client.py index 59fa537..2a4f15c 100644 --- a/test/test_client.py +++ b/test/test_client.py @@ -140,6 +140,25 @@ def test_sync_client_get_session_no_token(sync_client): } +def test_sync_client_recovers_from_connection_failure(sync_client): + """A failed connect leaves nothing cached, so the next call retries.""" + client = sync_client + + with mock.patch.object( + GqlClient, "connect_sync", side_effect=OSError("no route to host") + ): + with pytest.raises(OSError): + client._get_gql_session() + + assert client._prev_token is None + + client._get_requests_session() + assert dict(client._gql_session.transport.headers) == { + "Accept": "application/vnd.github+json", + "Authorization": f"Bearer {client.auth._token}", + } + + @pytest.mark.asyncio async def test_async_client_rest(aioresponses, async_client): client = async_client