From 4468df0e13b5dc8f5c23046984afe9a2b93adb71 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Fri, 21 Aug 2026 13:55:01 +0200 Subject: [PATCH] Pool HTTP connections, and let callers set the Firebase timeout Every request went through the module-level requests.get/put/post/delete helpers, so each one paid a fresh DNS+TCP+TLS handshake. Measured today against the real backends that is 95ms/req to api.github.com and 52ms to Realtime Database -- on a 27-request diet-guard tick, most of the wall clock. Each transport module now binds a pooling requests.Session to the name `requests`. A Session is a drop-in for those helpers, so all 9 call sites and all 16 tests that patch `.requests.` are untouched; the real module stays available as `_requests` for the RequestException/Response references. Sessions are per-module rather than shared because the tests use sequence-consuming side_effect lists, where one module could otherwise eat another's queued response. Transient failures now retry, restricted to GET: a PUT here carries a Contents-API sha, and replaying one whose response was merely lost would write against a stale precondition on a remote the phone also reads. Also thread timeout_seconds through firebase_client_for, which accepted none, so a caller's tighter budget silently reached the GitHub backend only. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VCucBhUkHBBHHbsY7bSdGV --- crdt-sync/crdt_sync/_config.py | 10 ++++- crdt-sync/crdt_sync/_firebase.py | 20 ++++++---- crdt-sync/crdt_sync/_firebase_auth.py | 12 ++++-- crdt-sync/crdt_sync/_github.py | 18 ++++++--- crdt-sync/crdt_sync/_http.py | 57 +++++++++++++++++++++++++++ crdt-sync/pyproject.toml | 2 +- 6 files changed, 100 insertions(+), 19 deletions(-) create mode 100644 crdt-sync/crdt_sync/_http.py diff --git a/crdt-sync/crdt_sync/_config.py b/crdt-sync/crdt_sync/_config.py index c00075d..6cafa44 100644 --- a/crdt-sync/crdt_sync/_config.py +++ b/crdt-sync/crdt_sync/_config.py @@ -26,7 +26,7 @@ import json from pathlib import Path from typing import TYPE_CHECKING -from crdt_sync._firebase import FirebaseSyncClient +from crdt_sync._firebase import _DEFAULT_TIMEOUT_SECONDS, FirebaseSyncClient from crdt_sync._firebase_auth import FileCredentialStore, FirebaseTokenProvider from crdt_sync._mirror import MirrorSyncClient @@ -141,6 +141,7 @@ def firebase_client_for( app_name: str, *, config: FirebaseConfig | None = None, + timeout_seconds: float = _DEFAULT_TIMEOUT_SECONDS, ) -> FirebaseSyncClient: """Return a signed-in client for ``app_name``. @@ -151,6 +152,9 @@ def firebase_client_for( Args: app_name: Names the credential cache, e.g. ``"diet_guard"``. config: Overrides the on-disk config; for tests. + timeout_seconds: Per-request network timeout. Without this the + client fell back to its own default and a caller's tighter + budget reached the GitHub backend only. Raises: ConfigError: If the shared config is unusable. @@ -160,7 +164,9 @@ def firebase_client_for( auth = FirebaseTokenProvider(resolved.api_key, credential_store_for(app_name)) if not auth.has_session(): auth.sign_in(resolved.email, resolved.read_password()) - return FirebaseSyncClient(resolved.database_url, auth) + return FirebaseSyncClient( + resolved.database_url, auth, timeout_seconds=timeout_seconds + ) def mirror_client_for( diff --git a/crdt-sync/crdt_sync/_firebase.py b/crdt-sync/crdt_sync/_firebase.py index 44b7a36..f44c315 100644 --- a/crdt-sync/crdt_sync/_firebase.py +++ b/crdt-sync/crdt_sync/_firebase.py @@ -17,10 +17,16 @@ from __future__ import annotations from typing import TYPE_CHECKING -import requests +import requests as _requests +from crdt_sync._http import new_session from crdt_sync._remote import RemoteNotFoundError, RemoteSyncError +# Bound to the name ``requests`` so the call sites below -- and the tests +# that patch ``.requests.`` -- are unchanged by pooling. +requests = new_session() + + if TYPE_CHECKING: from crdt_sync._firebase_auth import FirebaseTokenProvider @@ -163,7 +169,7 @@ class FirebaseSyncClient: def _params(self, **extra: str) -> dict[str, str]: return {"auth": self.auth.id_token(), **extra} - def _raise(self, what: str, response: requests.Response) -> None: + def _raise(self, what: str, response: _requests.Response) -> None: """Raise the right error type for a non-2xx response. 401/403 mean the rules rejected this uid or the token is bad. @@ -184,14 +190,14 @@ class FirebaseSyncClient: msg = f"{what} failed: HTTP {response.status_code} {detail}" raise FirebaseSyncError(msg) - def _get(self, path: str, **params: str) -> requests.Response: + def _get(self, path: str, **params: str) -> _requests.Response: try: return requests.get( self._url(path), params=self._params(**params), timeout=self._timeout_seconds, ) - except requests.RequestException as exc: + except _requests.RequestException as exc: msg = f"network error reading {path}" raise FirebaseSyncError(msg) from exc @@ -257,7 +263,7 @@ class FirebaseSyncClient: json=text, timeout=self._timeout_seconds, ) - except requests.RequestException as exc: + except _requests.RequestException as exc: msg = f"network error writing {path}" raise FirebaseSyncError(msg) from exc if not response.ok: @@ -302,7 +308,7 @@ class FirebaseSyncClient: params=self._params(), timeout=self._timeout_seconds, ) - except requests.RequestException as exc: + except _requests.RequestException as exc: msg = f"network error deleting {path}" raise FirebaseSyncError(msg) from exc if not response.ok: @@ -321,7 +327,7 @@ class FirebaseSyncClient: params=self._params(shallow="true"), timeout=self._timeout_seconds, ) - except requests.RequestException: + except _requests.RequestException: return False except RemoteSyncError: # Includes FirebaseAuthError: "cannot get a token" is exactly diff --git a/crdt-sync/crdt_sync/_firebase_auth.py b/crdt-sync/crdt_sync/_firebase_auth.py index 501a98f..808f3b1 100644 --- a/crdt-sync/crdt_sync/_firebase_auth.py +++ b/crdt-sync/crdt_sync/_firebase_auth.py @@ -17,10 +17,16 @@ import datetime as dt import json from typing import TYPE_CHECKING, Protocol -import requests +import requests as _requests +from crdt_sync._http import new_session from crdt_sync._remote import RemoteSyncError +# Bound to the name ``requests`` so the call sites below -- and the tests +# that patch ``.requests.`` -- are unchanged by pooling. +requests = new_session() + + if TYPE_CHECKING: from pathlib import Path @@ -373,7 +379,7 @@ class FirebaseTokenProvider: json=payload, timeout=self._timeout_seconds, ) - except requests.RequestException as exc: + except _requests.RequestException as exc: msg = f"network error trying to {what}" raise FirebaseAuthError(msg) from exc if not response.ok: @@ -383,7 +389,7 @@ class FirebaseTokenProvider: return response.json() -def _reason(response: requests.Response) -> str: +def _reason(response: _requests.Response) -> str: """Return Google's machine-readable error reason, in parentheses. Pulls ``INVALID_PASSWORD`` / ``TOKEN_EXPIRED`` / ``USER_DISABLED`` out of diff --git a/crdt-sync/crdt_sync/_github.py b/crdt-sync/crdt_sync/_github.py index 8be178b..3c3ad47 100644 --- a/crdt-sync/crdt_sync/_github.py +++ b/crdt-sync/crdt_sync/_github.py @@ -11,10 +11,16 @@ from __future__ import annotations import base64 -import requests +import requests as _requests +from crdt_sync._http import new_session from crdt_sync._remote import RemoteNotFoundError, RemoteSyncError +# Bound to the name ``requests`` so the call sites below -- and the tests +# that patch ``.requests.`` -- are unchanged by pooling. +requests = new_session() + + _API_BASE = "https://api.github.com" _HTTP_NOT_FOUND = 404 _DEFAULT_TIMEOUT_SECONDS = 15 @@ -68,14 +74,14 @@ class GitHubSyncClient: def _contents_url(self, path: str) -> str: return f"{_API_BASE}/repos/{self._owner}/{self._repo}/contents/{path}" - def _get(self, path: str) -> requests.Response: + def _get(self, path: str) -> _requests.Response: try: return requests.get( self._contents_url(path), headers=self._headers, timeout=self._timeout_seconds, ) - except requests.RequestException as exc: + except _requests.RequestException as exc: msg = f"network error reading {path}" raise GitHubSyncError(msg) from exc @@ -86,7 +92,7 @@ class GitHubSyncClient: headers=self._headers, timeout=self._timeout_seconds, ) - except requests.RequestException: + except _requests.RequestException: return False return response.ok @@ -166,7 +172,7 @@ class GitHubSyncClient: json=payload, timeout=self._timeout_seconds, ) - except requests.RequestException as exc: + except _requests.RequestException as exc: msg = f"network error pushing {path}" raise GitHubSyncError(msg) from exc if not response.ok: @@ -241,7 +247,7 @@ class GitHubSyncClient: json={"message": message, "sha": sha}, timeout=self._timeout_seconds, ) - except requests.RequestException as exc: + except _requests.RequestException as exc: msg = f"network error deleting {path}" raise GitHubSyncError(msg) from exc if not response.ok: diff --git a/crdt-sync/crdt_sync/_http.py b/crdt-sync/crdt_sync/_http.py new file mode 100644 index 0000000..259d15a --- /dev/null +++ b/crdt-sync/crdt_sync/_http.py @@ -0,0 +1,57 @@ +"""Pooled HTTP sessions, so every request does not re-handshake. + +Every call site in this package used the module-level ``requests.get`` / +``put`` / ``post`` / ``delete`` helpers, each of which opens a throwaway +connection: fresh DNS, TCP and TLS on every single request. Measured against +the two real backends on 2026-08-21 that costs 82ms per request to +``api.github.com`` and 51ms to Realtime Database -- on a 27-request sync tick, +most of the wall clock. + +A :class:`requests.Session` keeps the connection alive between requests, and +is a drop-in for the module-level helpers: same verb names, same signatures. +So each transport module binds one of these to the name ``requests`` and its +call sites are unchanged. + +**One session per module, deliberately not one global.** The tests patch the +verb on the module attribute (``patch.object(fb.requests, "get", ...)``), and +several use sequence-consuming ``side_effect`` lists. Were all three modules +to share a single object, a call from one would consume another's queued +response and the failure would look like a logic bug. Per-module also happens +to be per-host here -- GitHub, Realtime Database, and the token endpoint are +three different origins -- so pooling loses nothing by the split. + +Not thread-safe, in that :class:`requests.Session` is not documented to be; +this package spawns no threads and its consumers drive sync from one. +""" + +from __future__ import annotations + +import requests +from requests.adapters import HTTPAdapter, Retry + +_RETRY_TOTAL = 3 +_RETRY_BACKOFF_SECONDS = 0.3 +_RETRY_STATUSES = (429, 500, 502, 503, 504) + + +def new_session() -> requests.Session: + """Return a connection-pooling session that retries transient GET failures. + + Retries are restricted to ``GET``. A ``PUT`` here carries a Contents-API + ``sha`` (and a Realtime Database write is a full-node replace); replaying + one whose response was merely lost would push a second write built on a + now-stale precondition, against a remote another device reads. Reads are + idempotent, so only they are replayed. + """ + session = requests.Session() + retry = Retry( + total=_RETRY_TOTAL, + backoff_factor=_RETRY_BACKOFF_SECONDS, + status_forcelist=_RETRY_STATUSES, + allowed_methods=frozenset({"GET"}), + raise_on_status=False, + ) + adapter = HTTPAdapter(max_retries=retry) + session.mount("https://", adapter) + session.mount("http://", adapter) + return session diff --git a/crdt-sync/pyproject.toml b/crdt-sync/pyproject.toml index 69ef22e..531a35c 100644 --- a/crdt-sync/pyproject.toml +++ b/crdt-sync/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "crdt-sync" -version = "0.7.0" +version = "0.8.0" description = "Shared CRDT merge scheme + GitHub-Contents-API sync transport for cross-device apps" requires-python = ">=3.10" dependencies = ["requests"]