diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 8e25f5b..d0ec14b 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -171,6 +171,17 @@ underlying instant is identical. This caused a real bug (unchanged records misreported as "updated") before being standardized on naive-UTC (see `storage/models.py`'s `utcnow()`). +**Retry with backoff, but never on POST.** Every outbound HTTP call goes +through `http_client.get_session()` — a `requests.Session` with automatic +retry and exponential backoff for connection errors and 502/503/504. It +deliberately relies on urllib3's default allowed-methods policy (GET, PUT, +DELETE — not POST), since blindly retrying a failed POST risks creating a +duplicate record if the original request actually reached the server but +the response was lost. Every create-type call in this project (Pipedrive +deal/organization/note creation, calendar event creation, Graph token +requests) is a POST and intentionally does not get automatic retries for +this reason. + ## Adding a new data source See [ADDING_A_SCRAPER.md](ADDING_A_SCRAPER.md). diff --git a/pyproject.toml b/pyproject.toml index 7e41b29..76d7559 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -11,6 +11,7 @@ dependencies = [ "python-dotenv>=1.0", "requests>=2.31", "openpyxl>=3.1", + "urllib3>=2.0", ] [project.optional-dependencies] diff --git a/src/tendertracker/http_client.py b/src/tendertracker/http_client.py new file mode 100644 index 0000000..1407de6 --- /dev/null +++ b/src/tendertracker/http_client.py @@ -0,0 +1,31 @@ +import requests +from requests.adapters import HTTPAdapter +from urllib3.util.retry import Retry + + +def get_session(total_retries: int = 3, backoff_factor: float = 0.5) -> requests.Session: + """A requests.Session with automatic retry + exponential backoff for + transient failures — pulled from a pattern validated in a prior, related + system's own design (retry-with-backoff on outbound HTTP calls). + + Deliberately relies on urllib3's default allowed-methods policy: GET, + HEAD, PUT, DELETE, OPTIONS are retried; POST is not. Retrying a failed + POST blindly risks creating a duplicate record (e.g. a duplicate + Pipedrive deal) if the request actually reached the server but the + response was lost in transit — every POST in this project (create_deal, + create_event, add_note, organization creation, Graph token requests) + intentionally does NOT get automatic retries for this reason. GET/PUT + calls (data feeds, deal/event lookups and updates, file uploads) are + idempotent and safe to retry. + """ + session = requests.Session() + retry = Retry( + total=total_retries, + backoff_factor=backoff_factor, + status_forcelist=[502, 503, 504], + raise_on_status=False, + ) + adapter = HTTPAdapter(max_retries=retry) + session.mount("https://", adapter) + session.mount("http://", adapter) + return session diff --git a/src/tendertracker/integrations/calendar_sync.py b/src/tendertracker/integrations/calendar_sync.py index 718b3ea..caa1ba3 100644 --- a/src/tendertracker/integrations/calendar_sync.py +++ b/src/tendertracker/integrations/calendar_sync.py @@ -1,7 +1,6 @@ from datetime import datetime, timedelta -import requests - +from ..http_client import get_session from .graph_auth import GRAPH_BASE, get_access_token EVENT_DURATION = timedelta(hours=1) @@ -20,6 +19,7 @@ def __init__(self, tenant_id: str, client_id: str, client_secret: str, user_id: self.token = get_access_token(tenant_id, client_id, client_secret) self.user_id = user_id self.timeout = timeout + self.session = get_session() def _headers(self) -> dict: return {"Authorization": f"Bearer {self.token}", "Content-Type": "application/json"} @@ -34,7 +34,7 @@ def _event_payload(self, subject: str, start: datetime, body: str) -> dict: } def create_event(self, subject: str, start: datetime, body: str) -> dict: - response = requests.post( + response = self.session.post( f"{GRAPH_BASE}/users/{self.user_id}/events", headers=self._headers(), json=self._event_payload(subject, start, body), @@ -45,7 +45,7 @@ def create_event(self, subject: str, start: datetime, body: str) -> dict: return response.json() def update_event(self, event_id: str, subject: str, start: datetime, body: str) -> dict: - response = requests.patch( + response = self.session.patch( f"{GRAPH_BASE}/users/{self.user_id}/events/{event_id}", headers=self._headers(), json=self._event_payload(subject, start, body), @@ -56,7 +56,7 @@ def update_event(self, event_id: str, subject: str, start: datetime, body: str) return response.json() def delete_event(self, event_id: str) -> None: - response = requests.delete( + response = self.session.delete( f"{GRAPH_BASE}/users/{self.user_id}/events/{event_id}", headers=self._headers(), timeout=self.timeout ) # 404 is fine here — the event is already gone, which is the desired end state. diff --git a/src/tendertracker/integrations/graph_auth.py b/src/tendertracker/integrations/graph_auth.py index 82d8c9d..e5aa26c 100644 --- a/src/tendertracker/integrations/graph_auth.py +++ b/src/tendertracker/integrations/graph_auth.py @@ -1,4 +1,4 @@ -import requests +from ..http_client import get_session GRAPH_BASE = "https://graph.microsoft.com/v1.0" @@ -8,9 +8,15 @@ def get_access_token(tenant_id: str, client_id: str, client_secret: str) -> str: suitable for an unattended daily job. Requires an Azure AD app registration with the relevant application (not delegated) Graph permission, admin-consented. Shared by every Graph-backed integration - (OneDrive/SharePoint sync, calendar sync) — see docs/CLOUD_SYNC_SETUP.md.""" + (OneDrive/SharePoint sync, calendar sync) — see docs/CLOUD_SYNC_SETUP.md. + + This is a POST request, so it does not get the automatic retry + http_client.get_session() gives GET/PUT calls (see that module's + docstring) — safe to retry in principle (no side effects), but left + consistent with the rest of the project's POST-is-not-retried policy + rather than special-cased.""" url = f"https://login.microsoftonline.com/{tenant_id}/oauth2/v2.0/token" - response = requests.post( + response = get_session().post( url, data={ "grant_type": "client_credentials", diff --git a/src/tendertracker/integrations/onedrive_sync.py b/src/tendertracker/integrations/onedrive_sync.py index c551877..4ce188e 100644 --- a/src/tendertracker/integrations/onedrive_sync.py +++ b/src/tendertracker/integrations/onedrive_sync.py @@ -1,8 +1,7 @@ from pathlib import Path from typing import TYPE_CHECKING -import requests - +from ..http_client import get_session from .graph_auth import GRAPH_BASE, get_access_token if TYPE_CHECKING: @@ -39,7 +38,7 @@ def sync_file_to_onedrive( content = Path(local_path).read_bytes() url = f"{GRAPH_BASE}/drives/{drive_id}/root:/{remote_path}:/content" - response = requests.put( + response = get_session().put( url, headers={"Authorization": f"Bearer {token}", "Content-Type": "application/octet-stream"}, data=content, diff --git a/src/tendertracker/integrations/pipedrive.py b/src/tendertracker/integrations/pipedrive.py index e3c9382..d91e27a 100644 --- a/src/tendertracker/integrations/pipedrive.py +++ b/src/tendertracker/integrations/pipedrive.py @@ -1,4 +1,4 @@ -import requests +from ..http_client import get_session class PipedriveError(Exception): @@ -13,11 +13,12 @@ def __init__(self, api_token: str, domain: str, timeout: int = 30) -> None: self.api_token = api_token self.base_url = f"https://{domain}.pipedrive.com/api/v1" self.timeout = timeout + self.session = get_session() def _request(self, method: str, path: str, *, params: dict | None = None, json: dict | None = None) -> dict: params = dict(params or {}) params["api_token"] = self.api_token - response = requests.request(method, f"{self.base_url}{path}", params=params, json=json, timeout=self.timeout) + response = self.session.request(method, f"{self.base_url}{path}", params=params, json=json, timeout=self.timeout) if not response.ok: raise PipedriveError(f"{method} {path} failed: {response.status_code} {response.text}") data = response.json() diff --git a/src/tendertracker/scrapers/canadabuys_feed.py b/src/tendertracker/scrapers/canadabuys_feed.py index 22a63d5..9b8105f 100644 --- a/src/tendertracker/scrapers/canadabuys_feed.py +++ b/src/tendertracker/scrapers/canadabuys_feed.py @@ -3,8 +3,7 @@ import logging from typing import Iterator -import requests - +from ..http_client import get_session from .base import RawTender, Scraper logger = logging.getLogger(__name__) @@ -46,7 +45,7 @@ def __init__(self, feed_url: str = FEED_URL, timeout: int = 30, headers: dict | self.headers = headers or {"User-Agent": self.DEFAULT_USER_AGENT} def fetch(self) -> Iterator[RawTender]: - response = requests.get(self.feed_url, headers=self.headers, timeout=self.timeout) + response = get_session().get(self.feed_url, headers=self.headers, timeout=self.timeout) response.raise_for_status() # utf-8-sig strips the BOM this feed is published with. reader = csv.DictReader(io.StringIO(response.content.decode("utf-8-sig"))) diff --git a/tests/test_calendar_client.py b/tests/test_calendar_client.py index 35633a8..8ba06ed 100644 --- a/tests/test_calendar_client.py +++ b/tests/test_calendar_client.py @@ -13,13 +13,14 @@ def client(): def test_create_event_request_shape(client): - with patch("tendertracker.integrations.calendar_sync.requests.post") as mock_post: - mock_post.return_value = MagicMock(status_code=201, json=lambda: {"id": "EVT1"}) + with patch("requests.Session.request") as mock_req: + mock_req.return_value = MagicMock(status_code=201, json=lambda: {"id": "EVT1"}) event = client.create_event("Tender closing: X", datetime(2026, 9, 15, 14, 0, 0), "body text") assert event["id"] == "EVT1" - call = mock_post.call_args - assert call.args[0] == "https://graph.microsoft.com/v1.0/users/someone@example.com/events" + call = mock_req.call_args + assert call.args[0] == "POST" + assert call.args[1] == "https://graph.microsoft.com/v1.0/users/someone@example.com/events" assert call.kwargs["headers"]["Authorization"] == "Bearer FAKE_TOKEN" payload = call.kwargs["json"] assert payload["subject"] == "Tender closing: X" @@ -28,21 +29,23 @@ def test_create_event_request_shape(client): def test_update_event_request_shape(client): - with patch("tendertracker.integrations.calendar_sync.requests.patch") as mock_patch: - mock_patch.return_value = MagicMock(status_code=200, json=lambda: {"id": "EVT1"}) + with patch("requests.Session.request") as mock_req: + mock_req.return_value = MagicMock(status_code=200, json=lambda: {"id": "EVT1"}) client.update_event("EVT1", "Updated", datetime(2026, 9, 16, 10, 0, 0), "body") - call = mock_patch.call_args - assert call.args[0] == "https://graph.microsoft.com/v1.0/users/someone@example.com/events/EVT1" + call = mock_req.call_args + assert call.args[0] == "PATCH" + assert call.args[1] == "https://graph.microsoft.com/v1.0/users/someone@example.com/events/EVT1" def test_delete_event_request_shape(client): - with patch("tendertracker.integrations.calendar_sync.requests.delete") as mock_delete: - mock_delete.return_value = MagicMock(status_code=204) + with patch("requests.Session.request") as mock_req: + mock_req.return_value = MagicMock(status_code=204) client.delete_event("EVT1") - call = mock_delete.call_args - assert call.args[0] == "https://graph.microsoft.com/v1.0/users/someone@example.com/events/EVT1" + call = mock_req.call_args + assert call.args[0] == "DELETE" + assert call.args[1] == "https://graph.microsoft.com/v1.0/users/someone@example.com/events/EVT1" def test_delete_event_tolerates_404(): @@ -50,13 +53,13 @@ def test_delete_event_tolerates_404(): state — so this must not raise.""" with patch("tendertracker.integrations.calendar_sync.get_access_token", return_value="TOK"): client = CalendarClient("T", "C", "S", "someone@example.com") - with patch("tendertracker.integrations.calendar_sync.requests.delete") as mock_delete: - mock_delete.return_value = MagicMock(status_code=404) + with patch("requests.Session.request") as mock_req: + mock_req.return_value = MagicMock(status_code=404) client.delete_event("EVT1") # must not raise def test_create_event_raises_on_failure(client): - with patch("tendertracker.integrations.calendar_sync.requests.post") as mock_post: - mock_post.return_value = MagicMock(status_code=500, text="Server Error") + with patch("requests.Session.request") as mock_req: + mock_req.return_value = MagicMock(status_code=500, text="Server Error") with pytest.raises(CalendarSyncError, match="500"): client.create_event("X", datetime(2026, 1, 1), "b") diff --git a/tests/test_http_client.py b/tests/test_http_client.py new file mode 100644 index 0000000..0f0cfb2 --- /dev/null +++ b/tests/test_http_client.py @@ -0,0 +1,27 @@ +from tendertracker.http_client import get_session + + +def test_session_has_retry_adapter_mounted_for_both_schemes(): + session = get_session() + https_adapter = session.get_adapter("https://example.com") + http_adapter = session.get_adapter("http://example.com") + assert https_adapter.max_retries.total == 3 + assert http_adapter.max_retries.total == 3 + + +def test_retry_config_matches_documented_policy(): + session = get_session(total_retries=5, backoff_factor=1.0) + retry = session.get_adapter("https://example.com").max_retries + assert retry.total == 5 + assert retry.backoff_factor == 1.0 + assert set(retry.status_forcelist) == {502, 503, 504} + + +def test_post_is_not_in_the_retried_methods(): + """The whole point: POST must not auto-retry, since retrying a create + call that actually reached the server risks a duplicate record.""" + session = get_session() + retry = session.get_adapter("https://example.com").max_retries + assert "POST" not in retry.allowed_methods + assert "GET" in retry.allowed_methods + assert "PUT" in retry.allowed_methods diff --git a/tests/test_onedrive_sync.py b/tests/test_onedrive_sync.py index 59b7e93..2a38d56 100644 --- a/tests/test_onedrive_sync.py +++ b/tests/test_onedrive_sync.py @@ -31,9 +31,9 @@ def test_sync_file_to_onedrive_request_shape(tmp_path): local_file.write_bytes(b"fake-excel-bytes") with patch("tendertracker.integrations.onedrive_sync.get_access_token", return_value="FAKE_TOKEN"), patch( - "tendertracker.integrations.onedrive_sync.requests.put" - ) as mock_put: - mock_put.return_value = MagicMock( + "requests.Session.request" + ) as mock_req: + mock_req.return_value = MagicMock( status_code=201, json=lambda: {"webUrl": "https://example.sharepoint.com/fake"} ) result = sync_file_to_onedrive( @@ -46,8 +46,9 @@ def test_sync_file_to_onedrive_request_shape(tmp_path): ) assert result == {"webUrl": "https://example.sharepoint.com/fake"} - call = mock_put.call_args - assert call.args[0] == "https://graph.microsoft.com/v1.0/drives/DRIVE123/root:/TenderTracker/tenders.xlsx:/content" + call = mock_req.call_args + assert call.args[0] == "PUT" + assert call.args[1] == "https://graph.microsoft.com/v1.0/drives/DRIVE123/root:/TenderTracker/tenders.xlsx:/content" assert call.kwargs["headers"]["Authorization"] == "Bearer FAKE_TOKEN" assert call.kwargs["data"] == b"fake-excel-bytes" @@ -65,8 +66,8 @@ def test_sync_file_to_onedrive_raises_on_failed_upload(tmp_path): local_file.write_bytes(b"data") with patch("tendertracker.integrations.onedrive_sync.get_access_token", return_value="TOK"), patch( - "tendertracker.integrations.onedrive_sync.requests.put" - ) as mock_put: - mock_put.return_value = MagicMock(status_code=403, text="Forbidden") + "requests.Session.request" + ) as mock_req: + mock_req.return_value = MagicMock(status_code=403, text="Forbidden") with pytest.raises(OneDriveSyncError, match="403"): sync_file_to_onedrive(str(local_file), "D", "p", "T", "C", "S") diff --git a/tests/test_pipedrive_client.py b/tests/test_pipedrive_client.py index 96cb7e7..9480a23 100644 --- a/tests/test_pipedrive_client.py +++ b/tests/test_pipedrive_client.py @@ -11,7 +11,7 @@ def client(): def test_find_or_create_organization_returns_existing_match(client): - with patch("tendertracker.integrations.pipedrive.requests.request") as mock_req: + with patch("requests.Session.request") as mock_req: mock_req.return_value = MagicMock( ok=True, json=lambda: {"success": True, "data": {"items": [{"item": {"id": 42, "name": "Acme Co"}}]}}, @@ -32,14 +32,14 @@ def side_effect(method, url, **kwargs): return MagicMock(ok=True, json=lambda: {"success": True, "data": {"items": []}}) return MagicMock(ok=True, json=lambda: {"success": True, "data": {"id": 99}}) - with patch("tendertracker.integrations.pipedrive.requests.request", side_effect=side_effect): + with patch("requests.Session.request", side_effect=side_effect): org_id = client.find_or_create_organization("New Org") assert org_id == 99 def test_create_deal_request_shape(client): - with patch("tendertracker.integrations.pipedrive.requests.request") as mock_req: + with patch("requests.Session.request") as mock_req: mock_req.return_value = MagicMock(ok=True, json=lambda: {"success": True, "data": {"id": 7}}) deal = client.create_deal("Test Deal", org_id=99, value=1000.0, currency="CAD") @@ -51,7 +51,7 @@ def test_create_deal_request_shape(client): def test_update_deal_request_shape(client): - with patch("tendertracker.integrations.pipedrive.requests.request") as mock_req: + with patch("requests.Session.request") as mock_req: mock_req.return_value = MagicMock(ok=True, json=lambda: {"success": True, "data": {"id": 7}}) client.update_deal(7, "Updated Title", 2000.0, "CAD") @@ -61,7 +61,7 @@ def test_update_deal_request_shape(client): def test_add_note_request_shape(client): - with patch("tendertracker.integrations.pipedrive.requests.request") as mock_req: + with patch("requests.Session.request") as mock_req: mock_req.return_value = MagicMock(ok=True, json=lambda: {"success": True, "data": {}}) client.add_note(7, "some note") @@ -72,7 +72,7 @@ def test_add_note_request_shape(client): def test_get_deal_request_shape(client): - with patch("tendertracker.integrations.pipedrive.requests.request") as mock_req: + with patch("requests.Session.request") as mock_req: mock_req.return_value = MagicMock(ok=True, json=lambda: {"success": True, "data": {"id": 7, "title": "X"}}) deal = client.get_deal(7) @@ -83,14 +83,14 @@ def test_get_deal_request_shape(client): def test_non_ok_response_raises_pipedrive_error(client): - with patch("tendertracker.integrations.pipedrive.requests.request") as mock_req: + with patch("requests.Session.request") as mock_req: mock_req.return_value = MagicMock(ok=False, status_code=401, text="Unauthorized") with pytest.raises(PipedriveError, match="401"): client.create_deal("X", None, None, None) def test_success_false_raises_pipedrive_error(client): - with patch("tendertracker.integrations.pipedrive.requests.request") as mock_req: + with patch("requests.Session.request") as mock_req: mock_req.return_value = MagicMock(ok=True, json=lambda: {"success": False, "error": "bad request"}) with pytest.raises(PipedriveError, match="success=false"): client.create_deal("X", None, None, None)