diff --git a/src/basic_memory/cli/commands/cloud/webdav.py b/src/basic_memory/cli/commands/cloud/webdav.py index e6642107f..f5caeba77 100644 --- a/src/basic_memory/cli/commands/cloud/webdav.py +++ b/src/basic_memory/cli/commands/cloud/webdav.py @@ -19,6 +19,7 @@ express per-project access. """ +import asyncio import re import xml.etree.ElementTree as ElementTree from dataclasses import dataclass @@ -45,6 +46,24 @@ # content hash and must never be compared as one. _CONTENT_HASH_PATTERN = re.compile(r"[0-9a-fA-F]{32}") +# The service meters every request on this transport, and a transfer of any real +# size will meet its own rate limit: enumerating a project costs one PROPFIND per +# directory, and the transfer that follows costs one request per file. A 429 is +# therefore an ordinary step in a healthy transfer rather than a failure, and the +# response carries a Retry-After telling us when the window resets. +# +# Retrying is what makes progress possible at all. Before this, a single 429 +# aborted the whole transfer, and since every re-run restarts the walk at the +# first directory, a project past the per-minute ceiling could never finish no +# matter how long the user waited (#2039). +_RATE_LIMIT_MAX_ATTEMPTS = 6 +# A single wait is capped so an implausible Retry-After cannot hang the CLI, and +# floored at one second so a "retry immediately" answer cannot spin. +_RATE_LIMIT_MAX_WAIT_SECONDS = 60.0 +_RATE_LIMIT_MIN_WAIT_SECONDS = 1.0 +# Used when a 429 arrives with no Retry-After, or one we cannot parse. +_RATE_LIMIT_FALLBACK_WAIT_SECONDS = 5.0 + class WebdavError(Exception): """Raised when the cloud WebDAV surface cannot be read or written.""" @@ -130,6 +149,72 @@ def etag_content_hash(etag: str | None) -> str | None: return etag.lower() +# --- Rate-limit retry --- + + +async def _sleep(seconds: float) -> None: + """Wait out a rate-limit window. + + A module-level seam so tests can observe the waits this client would take + without spending them. + """ + await asyncio.sleep(seconds) + + +def _retry_after_seconds(response: httpx.Response) -> float: + """Read Retry-After as a wait in seconds, clamped to a sane range. + + RFC 9110 allows either delay-seconds or an HTTP-date, and the value is + advisory: a header we cannot parse still tells us the window is closed, so + every unusable form falls back to a fixed wait rather than giving up. + """ + raw = response.headers.get("Retry-After") + if raw is None: + return _RATE_LIMIT_FALLBACK_WAIT_SECONDS + + candidate = raw.strip() + if candidate.isdigit(): + seconds = float(candidate) + else: + try: + retry_at = parsedate_to_datetime(candidate) + except (TypeError, ValueError, OverflowError): + return _RATE_LIMIT_FALLBACK_WAIT_SECONDS + if retry_at.tzinfo is None: + return _RATE_LIMIT_FALLBACK_WAIT_SECONDS + seconds = (retry_at - datetime.now(retry_at.tzinfo)).total_seconds() + + return min(max(seconds, _RATE_LIMIT_MIN_WAIT_SECONDS), _RATE_LIMIT_MAX_WAIT_SECONDS) + + +async def _request_with_rate_limit_retry( + client: httpx.AsyncClient, + method: str, + request_path: str, + *, + content: bytes | str | None = None, + headers: dict[str, str] | None = None, +) -> httpx.Response: + """Send one request, waiting out any rate-limit rejections. + + Every request this module sends is safe to repeat: the reads have no body, + and the one write carries the same bytes and headers each time, so a replay + is the identical request rather than a second effect. The body is passed + explicitly rather than forwarded, so a caller cannot quietly add a parameter + that makes a retry something other than the same request again. + + A 429 on the final attempt is returned rather than raised, so the caller's + own error handling reports it with the rate-limit detail attached. + """ + for remaining in range(_RATE_LIMIT_MAX_ATTEMPTS - 1, -1, -1): + response = await client.request(method, request_path, content=content, headers=headers) + if response.status_code != httpx.codes.TOO_MANY_REQUESTS or remaining == 0: + return response + await _sleep(_retry_after_seconds(response)) + + raise AssertionError("unreachable: the loop returns on its final attempt") + + async def list_project_files(client: httpx.AsyncClient, project: str) -> list[RemoteFile]: """Enumerate every file in a cloud project. @@ -176,7 +261,7 @@ async def download_file(client: httpx.AsyncClient, project: str, rel_path: str) """ request_path = webdav_path(project, rel_path) try: - response = await client.get(request_path) + response = await _request_with_rate_limit_retry(client, "GET", request_path) response.raise_for_status() except httpx.HTTPError as exc: raise WebdavError(f"Failed to download {rel_path}: {_describe(exc)}") from exc @@ -220,7 +305,9 @@ async def upload_file( headers["If-None-Match"] = "*" try: - response = await client.put(request_path, content=content, headers=headers) + response = await _request_with_rate_limit_retry( + client, "PUT", request_path, content=content, headers=headers + ) # Checked before raise_for_status: a refused precondition is the answer # this call asked for, not a failure. if create_only and response.status_code == httpx.codes.PRECONDITION_FAILED: @@ -239,7 +326,8 @@ async def _propfind(client: httpx.AsyncClient, project: str, rel_dir: str) -> li """List one collection, returning its immediate children.""" request_path = webdav_path(project, rel_dir) try: - response = await client.request( + response = await _request_with_rate_limit_retry( + client, "PROPFIND", request_path, content=_PROPFIND_BODY, @@ -384,5 +472,32 @@ def _same_path(href: str, request_path: str) -> bool: def _describe(exc: httpx.HTTPError) -> str: """Render an httpx failure as a single actionable line.""" if isinstance(exc, httpx.HTTPStatusError): - return f"HTTP {exc.response.status_code} - {exc.response.text.strip()}" + detail = f"HTTP {exc.response.status_code} - {exc.response.text.strip()}" + return f"{detail}{_rate_limit_detail(exc.response)}" return str(exc) + + +def _rate_limit_detail(response: httpx.Response) -> str: + """Append the rate-limit headers to a 429, or nothing for any other status. + + A rate-limited transfer is the one failure a user can act on, by retrying + later or by moving less at once, but only if they can see it. The body alone + does not say what the limit was or when it resets, which left the headers + reachable only by editing this file (#2039). + """ + if response.status_code != httpx.codes.TOO_MANY_REQUESTS: + return "" + + reported = [ + (label, response.headers.get(header)) + for label, header in ( + ("limit", "X-RateLimit-Limit"), + ("remaining", "X-RateLimit-Remaining"), + ("retry after", "Retry-After"), + ("resets at", "X-RateLimit-Reset"), + ) + ] + known = [f"{label} {value}" for label, value in reported if value is not None] + if not known: + return "" + return f" (rate limit: {', '.join(known)})" diff --git a/tests/cli/cloud/test_webdav_client.py b/tests/cli/cloud/test_webdav_client.py index ae698bc83..2bc3ddd7d 100644 --- a/tests/cli/cloud/test_webdav_client.py +++ b/tests/cli/cloud/test_webdav_client.py @@ -11,6 +11,7 @@ import httpx import pytest +from basic_memory.cli.commands.cloud import webdav as webdav_module from basic_memory.cli.commands.cloud.webdav import ( WebdavError, download_file, @@ -468,3 +469,189 @@ async def handler(request: httpx.Request) -> httpx.Response: async with _client(handler) as client: with pytest.raises(WebdavError, match="HTTP 412"): await upload_file(client, "research", "a.md", content=b"hi", mtime=1) + + +# --- Rate-limit retry (#2039) --- + + +@pytest.fixture +def recorded_waits(monkeypatch): + """Collect the waits this client would take instead of spending them.""" + waits: list[float] = [] + + async def fake_sleep(seconds: float) -> None: + waits.append(seconds) + + monkeypatch.setattr(webdav_module, "_sleep", fake_sleep) + return waits + + +def _rate_limited(retry_after: str | None = "4") -> httpx.Response: + headers = { + "X-RateLimit-Limit": "120", + "X-RateLimit-Remaining": "0", + "X-RateLimit-Reset": "1789045631", + } + if retry_after is not None: + headers["Retry-After"] = retry_after + return httpx.Response( + 429, + headers=headers, + json={ + "detail": { + "code": "rate_limit_exceeded", + "message": "Too many requests. Retry after the current rate-limit window resets.", + "scope": "principal", + } + }, + ) + + +@pytest.mark.asyncio +async def test_propfind_walk_survives_a_rate_limit(recorded_waits): + """A 429 mid-walk must not abort the transfer. + + This is the reported failure: one throttled PROPFIND aborted the whole + project walk, and because each re-run restarted at the first directory the + pull could never finish. + """ + attempts: list[str] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + attempts.append(request.url.path) + if len(attempts) == 1: + return _rate_limited() + return httpx.Response( + 200, + text=_multistatus( + "/webdav/research/", + _file_entry("/webdav/research/a.md", "a.md", 3), + ), + ) + + async with _client(handler) as client: + files = await list_project_files(client, "research") + + assert [file.path for file in files] == ["a.md"] + assert len(attempts) == 2 + assert recorded_waits == [4.0] + + +@pytest.mark.asyncio +async def test_download_survives_a_rate_limit(recorded_waits): + """The transfer phase is metered too, so GET needs the same handling.""" + attempts: list[str] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + attempts.append(request.method) + if len(attempts) == 1: + return _rate_limited() + return httpx.Response(200, content=b"body") + + async with _client(handler) as client: + downloaded = await download_file(client, "research", "a.md") + + assert downloaded.content == b"body" + assert recorded_waits == [4.0] + + +@pytest.mark.asyncio +async def test_upload_retry_replays_the_same_write(recorded_waits): + """A replayed PUT must be the identical request, not a second effect.""" + seen: list[tuple[bytes, str | None, str | None]] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + seen.append( + ( + request.content, + request.headers.get("X-OC-Mtime"), + request.headers.get("If-None-Match"), + ) + ) + if len(seen) == 1: + return _rate_limited() + return httpx.Response(201) + + async with _client(handler) as client: + written = await upload_file( + client, "research", "a.md", content=b"body", mtime=1789045631, create_only=True + ) + + assert written is True + assert seen[0] == seen[1] == (b"body", "1789045631", "*") + assert recorded_waits == [4.0] + + +@pytest.mark.asyncio +async def test_create_only_precondition_is_not_retried(recorded_waits): + """412 is this call's answer, not a rejection to wait out.""" + attempts: list[str] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + attempts.append(request.method) + return httpx.Response(412) + + async with _client(handler) as client: + written = await upload_file( + client, "research", "a.md", content=b"body", mtime=1, create_only=True + ) + + assert written is False + assert len(attempts) == 1 + assert recorded_waits == [] + + +@pytest.mark.asyncio +async def test_persistent_rate_limit_reports_the_headers(recorded_waits): + """A transfer that cannot get through must say what stopped it.""" + attempts: list[str] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + attempts.append(request.method) + return _rate_limited() + + async with _client(handler) as client: + with pytest.raises(WebdavError) as caught: + await list_project_files(client, "research") + + message = str(caught.value) + assert "HTTP 429" in message + assert "rate limit: limit 120, remaining 0, retry after 4, resets at 1789045631" in message + # Bounded: the attempts stop, and only the waits between them are taken. + assert len(attempts) == 6 + assert recorded_waits == [4.0] * 5 + + +@pytest.mark.asyncio +async def test_non_rate_limit_errors_are_not_retried(recorded_waits): + """Only 429 is a wait-and-repeat answer; a 404 is final.""" + attempts: list[str] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + attempts.append(request.method) + return httpx.Response(404, text="missing") + + async with _client(handler) as client: + with pytest.raises(WebdavError) as caught: + await download_file(client, "research", "a.md") + + assert len(attempts) == 1 + assert recorded_waits == [] + # No rate-limit detail is appended to a status that has none. + assert "rate limit" not in str(caught.value) + + +@pytest.mark.parametrize( + ("retry_after", "expected"), + [ + ("4", 4.0), + ("0", 1.0), # floored, so "retry immediately" cannot spin + ("9999", 60.0), # capped, so an implausible header cannot hang the CLI + (None, 5.0), # absent: the window is still closed + ("not-a-date", 5.0), # unparseable falls back rather than giving up + ("Mon, 08 Jun 2026 10:30:00 GMT", 1.0), # a past HTTP-date floors + ], +) +def test_retry_after_seconds(retry_after, expected): + response = _rate_limited(retry_after=retry_after) + assert webdav_module._retry_after_seconds(response) == expected