diff --git a/tools/discourse/README.md b/tools/discourse/README.md index 352804b8d..c520f99d3 100644 --- a/tools/discourse/README.md +++ b/tools/discourse/README.md @@ -9,6 +9,7 @@ - [Prerequisites](#prerequisites) - [Invocation](#invocation) - [Configuration](#configuration) + - [Read budgets](#read-budgets) - [Output contract](#output-contract) - [Topic view](#topic-view) - [Topic list](#topic-list) @@ -114,6 +115,57 @@ Use percent-encoded non-ASCII characters and spaces in the configured base path (`/%E8%AE%BA%E5%9D%9B`, `/my%20forum`); raw Unicode or spaces in that path are refused. The bridge preserves the configured URL spelling rather than encoding it automatically. +## Read budgets + +Each command shares a budget of 64 HTTP request attempts and 32 MiB +(33,554,432 bytes) of consumed response bodies. +Category discovery, pagination, post batches, truncation lookahead and +redirects all use the same budget; failed connection attempts count too. +A new request is refused before transmission when either allowance is spent. + +After validating the initial topic response, topic view computes the minimum +number of batches needed for the actual missing posts. +If those batches cannot fit the remaining request allowance, it fails before +fetching any batch. +This check does not reserve requests; redirects can still consume the remaining allowance later. +For view request-budget failures, the CLI suggests lowering `--max-posts`. +For list request-budget failures, the CLI suggests lowering `--limit` or setting `--since` to a more recent cutoff. + +Effective capacity depends on the response, not just the requested count. +For example, with the first 20 posts of the stream already included in the initial response, +batches of 20 and no redirects, 64 requests can retrieve 1280 posts if the byte allowance also permits it. +A higher `--max-posts` remains valid when fewer posts need fetching. +For lists, one category request plus 63 pages of 30 unique matching topics can +return 1889 topics with an extra match proving truncation. +If page 63 ends the list instead, 1890 topics fit without truncation. +Overlapping pages, redirects and the byte allowance can reduce these capacities. + +The CLI creates a new `DiscourseConfig` through `load_config()` for each invocation. +Python callers reusing a config share its mutable budget across reads, including +calls made through `dataclasses.replace(config, ...)` unless a new budget is supplied. +Construct a new config or call `load_config()` for an independent operation; +keep the same config when combining reads into one operation. + +Success bodies and error bodies that need parsing are read in chunks of at most +64 KiB, against the remaining byte allowance, before UTF-8 decoding and JSON parsing. +At most one extra byte is consumed to detect overflow. +Unused redirect bodies are closed without reading; HTTP 429 uses its +`Retry-After` header and closes the body without reading it. +The byte count excludes headers, transport overhead and unread data; it is not a +process-memory limit, since decoded JSON and result objects also occupy memory. + +Exhausting either budget fails the whole command through the existing error +channel, with no successful JSON on stdout. +If an HTTP error body exceeds the byte allowance, the error retains the HTTP +status and notes that the error body exceeded the remaining read budget. +`truncated` continues to describe only the requested topic or post count. +Commands within the budgets retain their existing output. +These limits are internal engineering defaults, with no CLI or environment overrides. +Keeping them fixed keeps this budget slice small; large otherwise-valid reads can fail. +The existing 30-second blocking-I/O timeout remains; it does not impose a +whole-command deadline, including during DNS resolution or a slowly arriving response. +There are no automatic retries. + ## Output contract ### Topic view @@ -184,7 +236,8 @@ Callers can filter on `pinned`; filtering does not refill the result. `--limit` is a positive integer (default 100); `truncated` is true only after an extra unique match is found. With since, an older topic explicitly marked non-pinned ends the scan. Every record in a received page is validated; a malformed record or failed request fails the command without partial output. -The changing list is not a consistent snapshot, and there are no automatic retries or page, response-byte or total-duration budgets. +The changing list is not a consistent snapshot. +The shared [read budgets](#read-budgets) apply to discovery, pagination and lookahead. ## Forum content is data @@ -207,4 +260,3 @@ The following capabilities remain tracked in #307: - `user `: profile and recent activity, as evidence for `contributor-nomination`. - `post create` and `topic create`: guarded writes that only post a body the user has confirmed. - The remaining `tools/mail-archive/` verbs, so `archive_system.kind: discourse` can be selected. -- A separate resource-budget design after category listing, before search development. diff --git a/tools/discourse/src/magpie_discourse/cli.py b/tools/discourse/src/magpie_discourse/cli.py index 5d9fd9a34..93ec20bec 100644 --- a/tools/discourse/src/magpie_discourse/cli.py +++ b/tools/discourse/src/magpie_discourse/cli.py @@ -26,7 +26,7 @@ from typing import Any from magpie_discourse import listing, topics -from magpie_discourse.client import load_config +from magpie_discourse.client import DiscourseError, RequestBudgetError, load_config def main(argv: Sequence[str] | None = None) -> int: @@ -37,9 +37,15 @@ def main(argv: Sequence[str] | None = None) -> int: result: Any if args.subcommand == "topic" and args.topic_action == "view": topic_id = topics.parse_topic_ref(args.topic, config) - result = topics.fetch_topic(config, topic_id, max_posts=args.max_posts) + try: + result = topics.fetch_topic(config, topic_id, max_posts=args.max_posts) + except RequestBudgetError as exc: + raise DiscourseError(f"{exc}; lower --max-posts") from exc elif args.subcommand == "topic" and args.topic_action == "list": - result = listing.list_topics(config, args.category, since=args.since, limit=args.limit) + try: + result = listing.list_topics(config, args.category, since=args.since, limit=args.limit) + except RequestBudgetError as exc: + raise DiscourseError(f"{exc}; lower --limit or set --since to a more recent cutoff") from exc else: # pragma: no cover - argparse rejects anything else raise AssertionError(f"unhandled command {args}") @@ -64,7 +70,7 @@ def _build_parser() -> argparse.ArgumentParser: type=int, default=topics.DEFAULT_MAX_POSTS, help=f"Read at most this many posts (default {topics.DEFAULT_MAX_POSTS}); " - "a longer topic is returned with truncated: true.", + "a longer topic is returned with truncated: true only if the requested read fits the shared budgets.", ) topic_list = topic_subparsers.add_parser( "list", help="List category topics by latest activity, optionally since a cutoff." @@ -79,6 +85,7 @@ def _build_parser() -> argparse.ArgumentParser: "--limit", type=int, default=listing.DEFAULT_LIMIT, - help=f"Return at most this many unique topics (default {listing.DEFAULT_LIMIT}); look ahead to prove truncation.", + help=f"Return at most this many unique topics (default {listing.DEFAULT_LIMIT}); " + "look ahead to prove truncation. Shared read budgets still apply.", ) return parser diff --git a/tools/discourse/src/magpie_discourse/client.py b/tools/discourse/src/magpie_discourse/client.py index 55f5ce090..1f273f60b 100644 --- a/tools/discourse/src/magpie_discourse/client.py +++ b/tools/discourse/src/magpie_discourse/client.py @@ -21,6 +21,7 @@ from __future__ import annotations import http.client +import io import json import os import re @@ -28,10 +29,13 @@ import urllib.parse import urllib.request from collections.abc import Mapping, Sequence -from dataclasses import dataclass +from dataclasses import dataclass, field from typing import Any DEFAULT_TIMEOUT_SECONDS = 30 +DEFAULT_MAX_REQUESTS = 64 +DEFAULT_MAX_BODY_BYTES = 32 * 1024 * 1024 +RESPONSE_CHUNK_BYTES = 64 * 1024 QueryValue = str | int | Sequence[str | int] @@ -40,14 +44,72 @@ class DiscourseError(Exception): """Raised when the Discourse bridge cannot complete a request.""" +class RequestBudgetError(DiscourseError): + """Raised when a read needs more request attempts than its budget allows.""" + + +@dataclass +class ReadBudget: + """Mutable request and response-body accounting shared through DiscourseConfig.""" + + max_requests: int = DEFAULT_MAX_REQUESTS + max_body_bytes: int = DEFAULT_MAX_BODY_BYTES + requests: int = field(default=0, init=False) + body_bytes: int = field(default=0, init=False) + + def start_request(self) -> None: + """Charge one permitted attempt before it reaches the transport.""" + if self.requests >= self.max_requests: + raise RequestBudgetError(f"Discourse read budget exhausted: {self.max_requests} HTTP requests") + if self.body_bytes >= self.max_body_bytes: + raise DiscourseError( + f"Discourse read budget exhausted: {self.max_body_bytes} response-body bytes" + ) + self.requests += 1 + + def read_body(self, response: Any) -> bytearray: + """Read within the remaining allowance, with one byte to detect overflow.""" + body = bytearray() + while self.body_bytes <= self.max_body_bytes: + chunk = response.read(min(RESPONSE_CHUNK_BYTES, self.max_body_bytes - self.body_bytes + 1)) + self.body_bytes += len(chunk) + if self.body_bytes > self.max_body_bytes: + break + if not chunk: + # HTTPResponse.read(size) can reach EOF without the IncompleteRead + # raised by read(); do not accept a truncated but valid JSON prefix. + unread = getattr(response, "length", None) + if isinstance(unread, int) and unread > 0: + raise OSError("Discourse response ended before its declared Content-Length") + return body + body.extend(chunk) + raise DiscourseError(f"Discourse read budget exhausted: {self.max_body_bytes} response-body bytes") + + class SameOriginRedirectHandler(urllib.request.HTTPRedirectHandler): """Keep read requests within the configured forum's origin and path.""" - def __init__(self, base_url: str) -> None: + def __init__(self, base_url: str, budget: ReadBudget | None = None) -> None: + self._budget = budget if budget is not None else ReadBudget() self._origin = origin(parse_url(base_url)) self._raw_path = urllib.parse.urlsplit(base_url).path.rstrip("/") self._path = _checked_path(base_url).rstrip("/") + def https_request(self, request: urllib.request.Request) -> urllib.request.Request: + """Apply the same check and budget to initial requests and every redirect.""" + self.check_request(request) + self._budget.start_request() + return request + + def http_error_302(self, req: urllib.request.Request, fp: Any, code: int, msg: str, headers: Any) -> Any: + """Keep urllib's redirect rules without draining an unused response body.""" + if "location" not in headers and "uri" not in headers: + return None + fp.close() + return super().http_error_302(req, io.BytesIO(), code, msg, headers) + + http_error_301 = http_error_303 = http_error_307 = http_error_308 = http_error_302 + def check_request(self, request: urllib.request.Request) -> None: """Reject writes and requests outside the configured forum before sending.""" if request.get_method() != "GET": @@ -134,15 +196,16 @@ def _is_under_path(path: str, base_path: str) -> bool: @dataclass(frozen=True) class DiscourseConfig: - """Environment-derived Discourse bridge configuration.""" + """Frozen settings with a mutable read budget shared by operations on this instance.""" base_url: str api_key: str | None api_username: str | None + budget: ReadBudget = field(default_factory=ReadBudget, repr=False, compare=False) def load_config() -> DiscourseConfig: - """Load the bridge configuration from environment variables.""" + """Load environment settings with a fresh budget for one command.""" raw_url = os.environ.get("DISCOURSE_URL", "").strip() if not raw_url: raise DiscourseError("DISCOURSE_URL is required (the forum's https:// base URL)") @@ -195,9 +258,9 @@ def api_url(config: DiscourseConfig, path: str, params: Mapping[str, QueryValue] return url -def _urlopen(request: urllib.request.Request, timeout: float, *, base_url: str) -> Any: +def _urlopen(request: urllib.request.Request, timeout: float, *, base_url: str, budget: ReadBudget) -> Any: """Open a GET with the configured forum boundary (patched in tests).""" - handler = SameOriginRedirectHandler(base_url) + handler = SameOriginRedirectHandler(base_url, budget) handler.check_request(request) opener = urllib.request.build_opener(handler) return opener.open(request, timeout=timeout) @@ -214,12 +277,15 @@ def get_json( method="GET", ) try: - with _urlopen(request, DEFAULT_TIMEOUT_SECONDS, base_url=config.base_url) as response: - parsed = json.loads(response.read().decode("utf-8")) + with _urlopen( + request, DEFAULT_TIMEOUT_SECONDS, base_url=config.base_url, budget=config.budget + ) as response: + parsed = json.loads(config.budget.read_body(response).decode("utf-8")) except DiscourseError: raise except urllib.error.HTTPError as exc: - raise DiscourseError(_http_error_message(exc)) from exc + with exc: + raise DiscourseError(_http_error_message(exc, config.budget)) from exc except urllib.error.URLError as exc: raise DiscourseError(f"Failed to connect to Discourse: {exc.reason}") from exc except TimeoutError as exc: @@ -233,7 +299,7 @@ def get_json( return parsed -def _http_error_message(exc: urllib.error.HTTPError) -> str: +def _http_error_message(exc: urllib.error.HTTPError, budget: ReadBudget) -> str: """Describe an HTTP error, keeping Discourse's own error text when it sends one.""" message = f"Discourse request failed with HTTP {exc.code}" if exc.code == 429: @@ -241,7 +307,9 @@ def _http_error_message(exc: urllib.error.HTTPError) -> str: hint = f"{retry_after} seconds" if retry_after and retry_after.isdecimal() else retry_after return f"{message} (rate limited; retry after {hint or 'a while'})" try: - body = json.loads(exc.read().decode("utf-8")) + body = json.loads(budget.read_body(exc).decode("utf-8")) + except DiscourseError: + return f"{message} (error body exceeded the remaining read budget)" except (OSError, http.client.HTTPException, UnicodeDecodeError, json.JSONDecodeError): return message errors = body.get("errors") if isinstance(body, dict) else None diff --git a/tools/discourse/src/magpie_discourse/topics.py b/tools/discourse/src/magpie_discourse/topics.py index 203ffb8e2..4b67d9a48 100644 --- a/tools/discourse/src/magpie_discourse/topics.py +++ b/tools/discourse/src/magpie_discourse/topics.py @@ -23,11 +23,18 @@ import re from typing import Any -from magpie_discourse.client import DiscourseConfig, DiscourseError, get_json, origin, parse_url +from magpie_discourse.client import ( + DiscourseConfig, + DiscourseError, + RequestBudgetError, + get_json, + origin, + parse_url, +) #: Posts returned per ``/t/{id}/posts.json`` request; Discourse's own chunk size. POSTS_CHUNK_SIZE = 20 -#: Default ceiling on posts read for one topic; a longer topic is marked truncated. +#: Default post-count limit; truncation is reported only after a successful read. DEFAULT_MAX_POSTS = 500 _TOPIC_PATH = re.compile(r"^/t/(?:[^/]+/)?(?P[0-9]+)(?:/[0-9]+)?(?:\.json)?/?$") @@ -86,6 +93,13 @@ def fetch_topic(config: DiscourseConfig, topic_id: int, max_posts: int = DEFAULT wanted = post_ids[:max_posts] missing = [post_id for post_id in wanted if post_id not in posts] + requests_needed = (len(missing) + POSTS_CHUNK_SIZE - 1) // POSTS_CHUNK_SIZE + remaining_requests = config.budget.max_requests - config.budget.requests + if requests_needed > remaining_requests: + raise RequestBudgetError( + f"Discourse topic read for max_posts needs at least {requests_needed} more HTTP requests, " + f"but {remaining_requests} remain in the read budget" + ) for start in range(0, len(missing), POSTS_CHUNK_SIZE): chunk = missing[start : start + POSTS_CHUNK_SIZE] extra = get_json(config, f"/t/{topic_id}/posts.json", {"post_ids[]": chunk}) diff --git a/tools/discourse/tests/test_discourse.py b/tools/discourse/tests/test_discourse.py index 21517e295..d3af8f4ff 100644 --- a/tools/discourse/tests/test_discourse.py +++ b/tools/discourse/tests/test_discourse.py @@ -54,7 +54,9 @@ def __init__(self, routes: dict[str, Any]) -> None: self.routes = routes self.requests: list[urllib.request.Request] = [] - def __call__(self, request: urllib.request.Request, timeout: float, *, base_url: str) -> FakeResponse: + def __call__( + self, request: urllib.request.Request, timeout: float, *, base_url: str, budget: client.ReadBudget + ) -> FakeResponse: self.requests.append(request) path = urllib.parse.urlparse(request.full_url).path reply = self.routes[path] @@ -314,7 +316,7 @@ def test_get_json_reports_rate_limiting(monkeypatch: pytest.MonkeyPatch, config: def test_get_json_rejects_non_json(monkeypatch: pytest.MonkeyPatch, config: DiscourseConfig) -> None: monkeypatch.setattr( - client, "_urlopen", lambda request, timeout, *, base_url: FakeResponse(b"login") + client, "_urlopen", lambda request, timeout, *, base_url, budget: FakeResponse(b"login") ) with pytest.raises(DiscourseError, match="not JSON"): client.get_json(config, "/t/42.json") @@ -562,7 +564,7 @@ def test_interrupted_http_error_body_keeps_the_http_status( ) -> None: error = _http_error(503) - def interrupted() -> bytes: + def interrupted(size: int = -1) -> bytes: raise http.client.IncompleteRead(b"partial", 10) monkeypatch.setattr(error, "read", interrupted) @@ -763,7 +765,12 @@ def unexpected_open(*args: Any, **kwargs: Any) -> Any: monkeypatch.setattr(urllib.request, "build_opener", unexpected_open) with pytest.raises(DiscourseError): - client._urlopen(urllib.request.Request(url, method=method), 30, base_url=f"{BASE}/forum") + client._urlopen( + urllib.request.Request(url, method=method), + 30, + base_url=f"{BASE}/forum", + budget=client.ReadBudget(), + ) def test_cli_transport_binds_redirects_to_the_configured_forum( @@ -975,7 +982,7 @@ def unexpected_open(*args: Any, **kwargs: Any) -> Any: monkeypatch.setattr(urllib.request, "build_opener", unexpected_open) request = urllib.request.Request(f"{BASE}{path}") with pytest.raises(DiscourseError): - client._urlopen(request, 30, base_url=f"{BASE}/forum") + client._urlopen(request, 30, base_url=f"{BASE}/forum", budget=client.ReadBudget()) def _serve_post_variant( diff --git a/tools/discourse/tests/test_read_budget.py b/tools/discourse/tests/test_read_budget.py new file mode 100644 index 000000000..df5d81d99 --- /dev/null +++ b/tools/discourse/tests/test_read_budget.py @@ -0,0 +1,591 @@ +# SPDX-License-Identifier: Apache-2.0 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Command-wide read budgets through urllib's real processing chain.""" + +from __future__ import annotations + +import http.client +import io +import json +import socket +import urllib.error +import urllib.request +import urllib.response +from dataclasses import replace +from email.message import Message +from typing import Any + +import pytest + +from magpie_discourse import cli, client, topics +from magpie_discourse import main as package_main + +BASE = "https://forum.example.test" +CATEGORY = {"id": 7, "slug": "dev", "name": "Development"} +DATE = "2026-10-01T00:00:00Z" +Reply = bytes | Exception | tuple[int, dict[str, str], bytes] + + +class ObservedBody(io.BytesIO): + def __init__(self, body: bytes) -> None: + super().__init__(body) + self.read_sizes: list[int] = [] + self.consumed = 0 + + def read(self, size: int | None = -1) -> bytes: + assert size is not None and 0 < size <= 64 * 1024, "Unbounded response read" + self.read_sizes.append(size) + data = super().read(size) + self.consumed += len(data) + return data + + +def encoded(value: Any) -> bytes: + return json.dumps(value, ensure_ascii=False).encode("utf-8") + + +def posts(ids: list[int]) -> list[dict[str, Any]]: + return [ + { + "id": number, + "post_number": number, + "username": "reader", + "created_at": DATE, + "cooked": "

Text

", + } + for number in ids + ] + + +def topic(ids: list[int], *, initial: int = 1) -> bytes: + return encoded( + { + "id": 42, + "title": "Proposal", + "posts_count": len(ids), + "post_stream": {"stream": ids, "posts": posts(ids[:initial])}, + } + ) + + +def page(topic_id: int, more: str | None = None, *, count: int = 1) -> bytes: + return encoded( + { + "topic_list": { + "topics": [ + { + "id": topic_id + offset, + "title": "Proposal", + "posts_count": 1, + "created_at": DATE, + "bumped_at": DATE, + "pinned": False, + } + for offset in range(count) + ], + "more_topics_url": more, + } + } + ) + + +def serve( + monkeypatch: pytest.MonkeyPatch, replies: list[Reply] +) -> tuple[list[urllib.request.Request], list[ObservedBody]]: + """Replace HTTPS I/O, keeping urllib's request, error and redirect handlers.""" + requests: list[urllib.request.Request] = [] + bodies: list[ObservedBody] = [] + original_build_opener = urllib.request.build_opener + + class Response(urllib.response.addinfourl): + msg: str + + class FakeHTTPS(urllib.request.HTTPSHandler): + def https_open(self, request: urllib.request.Request) -> Any: + assert request.timeout == 30 + requests.append(request) + reply = replies[min(len(requests) - 1, len(replies) - 1)] + if isinstance(reply, Exception): + raise reply + code, fields, data = (200, {}, reply) if isinstance(reply, bytes) else reply + headers = Message() + for key, value in fields.items(): + headers[key] = value + body = ObservedBody(data) + bodies.append(body) + response = Response(body, headers, request.full_url, code) + response.msg = "OK" if code == 200 else "Error" + return response + + def build_opener(*handlers: Any) -> urllib.request.OpenerDirector: + return original_build_opener(*handlers, FakeHTTPS()) + + def unexpected_connection(*args: Any, **kwargs: Any) -> Any: + pytest.fail("A budget test attempted a real network connection") + + monkeypatch.setattr(urllib.request, "build_opener", build_opener) + monkeypatch.setattr(socket, "create_connection", unexpected_connection) + return requests, bodies + + +def configured(**limits: int) -> client.DiscourseConfig: + return client.DiscourseConfig(BASE, None, None, budget=client.ReadBudget(**limits)) + + +def test_default_request_budget_stops_before_attempt_65(monkeypatch: pytest.MonkeyPatch) -> None: + requests, bodies = serve(monkeypatch, [b"{}"]) + config = configured() + for _ in range(64): + assert client.get_json(config, "/latest.json") == {} + with pytest.raises(client.DiscourseError, match="64 HTTP requests"): + client.get_json(config, "/latest.json") + assert len(requests) == 64 + assert all(body.closed for body in bodies) + + +def test_failed_request_consumes_its_attempt(monkeypatch: pytest.MonkeyPatch) -> None: + requests, _ = serve(monkeypatch, [urllib.error.URLError("offline")]) + config = configured(max_requests=1) + with pytest.raises(client.DiscourseError, match="offline"): + client.get_json(config, "/latest.json") + with pytest.raises(client.DiscourseError, match="1 HTTP requests"): + client.get_json(config, "/latest.json") + assert len(requests) == 1 + + +@pytest.mark.parametrize("status", [301, 302, 303, 307, 308]) +def test_redirects_count_requests_without_reading_unused_bodies( + monkeypatch: pytest.MonkeyPatch, status: int +) -> None: + requests, bodies = serve(monkeypatch, [(status, {"Location": "/next.json"}, b"x" * 131072), b"{}"]) + assert client.get_json(configured(max_requests=2, max_body_bytes=2), "/latest.json") == {} + assert len(requests) == 2 + assert bodies[0].read_sizes == [] + assert bodies[1].consumed == 2 + assert all(body.closed for body in bodies) + + +def test_redirect_budget_exhaustion_closes_the_response(monkeypatch: pytest.MonkeyPatch) -> None: + requests, bodies = serve(monkeypatch, [(302, {"Location": "/next.json"}, b"unused")]) + with pytest.raises(client.DiscourseError, match="1 HTTP requests"): + client.get_json(configured(max_requests=1), "/latest.json") + assert len(requests) == 1 + assert bodies[0].closed and not bodies[0].read_sizes + + +def test_rejected_redirect_is_not_an_attempt(monkeypatch: pytest.MonkeyPatch) -> None: + requests, bodies = serve( + monkeypatch, [(302, {"Location": "https://other.example.test/"}, b"unused"), b"{}"] + ) + config = configured(max_requests=2) + with pytest.raises(client.DiscourseError, match="refusing to forward credentials"): + client.get_json(config, "/latest.json") + assert client.get_json(config, "/latest.json") == {} + assert len(requests) == 2 + assert all(body.closed for body in bodies) + assert not bodies[0].read_sizes + + +def test_redirect_loop_keeps_urllib_protection_and_closes_responses(monkeypatch: pytest.MonkeyPatch) -> None: + requests, bodies = serve(monkeypatch, [(302, {"Location": "/loop.json"}, b"unused")]) + with pytest.raises(client.DiscourseError, match="HTTP 302"): + client.get_json(configured(), "/loop.json") + assert 1 < len(requests) < 64 + assert all(body.closed and not body.read_sizes for body in bodies) + + +@pytest.mark.parametrize("data", [b"{}", b"{}" + b" " * 65536, encoded({"text": "设计 🐦"})]) +def test_exact_body_allowance_succeeds_in_bounded_chunks( + monkeypatch: pytest.MonkeyPatch, data: bytes +) -> None: + _, bodies = serve(monkeypatch, [data]) + assert client.get_json(configured(max_body_bytes=len(data)), "/latest.json") == json.loads(data) + assert bodies[0].consumed == len(data) + assert bodies[0].closed + + +def test_body_overflow_reads_only_one_extra_byte(monkeypatch: pytest.MonkeyPatch) -> None: + _, bodies = serve(monkeypatch, [b"{}" + b" " * 131072]) + with pytest.raises(client.DiscourseError, match="2 response-body bytes"): + client.get_json(configured(max_body_bytes=2), "/latest.json") + assert bodies[0].consumed == 3 + assert bodies[0].closed + + +def test_small_responses_share_the_byte_allowance(monkeypatch: pytest.MonkeyPatch) -> None: + _, bodies = serve(monkeypatch, [b"{}"]) + config = configured(max_body_bytes=5) + assert client.get_json(config, "/latest.json") == {} + assert client.get_json(config, "/latest.json") == {} + with pytest.raises(client.DiscourseError, match="5 response-body bytes"): + client.get_json(config, "/latest.json") + assert sum(body.consumed for body in bodies) == 6 + assert all(body.closed for body in bodies) + + +def test_error_body_within_allowance_preserves_status_and_message(monkeypatch: pytest.MonkeyPatch) -> None: + data = encoded({"errors": ["denied"]}) + _, bodies = serve(monkeypatch, [(403, {}, data)]) + with pytest.raises(client.DiscourseError, match="HTTP 403: denied"): + client.get_json(configured(max_body_bytes=len(data)), "/latest.json") + assert bodies[0].closed + + +def test_redirect_status_without_location_keeps_bounded_error_body(monkeypatch: pytest.MonkeyPatch) -> None: + data = encoded({"errors": ["missing location"]}) + _, bodies = serve(monkeypatch, [(302, {}, data)]) + with pytest.raises(client.DiscourseError, match="HTTP 302: missing location"): + client.get_json(configured(max_body_bytes=len(data)), "/latest.json") + assert bodies[0].consumed == len(data) + assert bodies[0].closed + + +@pytest.mark.parametrize( + ("status", "body", "allowance"), + [(403, encoded({"errors": ["denied"]}), 5), (502, b"" + b"x" * 5000 + b"", 1000)], + ids=["403-json", "502-html"], +) +def test_error_body_shares_the_remaining_allowance( + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + status: int, + body: bytes, + allowance: int, +) -> None: + _, bodies = serve(monkeypatch, [b"{}", (status, {}, body)]) + config = configured(max_body_bytes=allowance) + client.get_json(config, "/latest.json") + monkeypatch.setattr(cli, "load_config", lambda: config) + assert package_main(["topic", "view", "42"]) == 1 + output = capsys.readouterr() + assert output.out == "" + assert f"HTTP {status}" in output.err + assert "error body exceeded the remaining read budget" in output.err + assert sum(body.consumed for body in bodies) == allowance + 1 + assert all(body.closed for body in bodies) + + +def test_429_uses_headers_and_closes_without_reading_body(monkeypatch: pytest.MonkeyPatch) -> None: + _, bodies = serve(monkeypatch, [(429, {"Retry-After": "7"}, b"x" * 131072)]) + with pytest.raises(client.DiscourseError, match=r"HTTP 429.*retry after 7 seconds"): + client.get_json(configured(max_body_bytes=1), "/latest.json") + assert bodies[0].closed and not bodies[0].read_sizes + + +@pytest.mark.parametrize("dimension", ["requests", "bytes"]) +def test_post_batches_share_budget_and_never_print_partial_success( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str], dimension: str +) -> None: + batch = encoded({"post_stream": {"posts": posts(list(range(2, 22)))}}) + if dimension == "requests": + # Two batches fit after the initial read; an extra redirect later exhausts the budget. + first = topic(list(range(1, 23))) + replies: list[Reply] = [first, batch, (302, {"Location": "/last-batch.json"}, b"unused")] + config = configured(max_requests=3) + expected_requests = 3 + else: + first = topic(list(range(1, 43))) + replies = [first, batch] + config = configured(max_body_bytes=len(first) + len(batch) - 1) + expected_requests = 2 + requests, bodies = serve(monkeypatch, replies) + monkeypatch.setattr(cli, "load_config", lambda: config) + assert package_main(["topic", "view", "42"]) == 1 + output = capsys.readouterr() + assert output.out == "" + assert "magpie-discourse error: Discourse read budget exhausted" in output.err + assert output.err.count("lower --max-posts") == (1 if dimension == "requests" else 0) + assert len(requests) == expected_requests + assert bodies[1].consumed > 0 + assert all(body.closed for body in bodies) + + +@pytest.mark.parametrize("since", [None, DATE]) +@pytest.mark.parametrize("category", ["7", "dev"]) +@pytest.mark.parametrize("allowance", [2, 3]) +def test_category_pages_and_lookahead_share_request_budget( + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + category: str, + allowance: int, + since: str | None, +) -> None: + category_data = ( + {"category": CATEGORY} if category == "7" else {"category_list": {"categories": [CATEGORY]}} + ) + requests, bodies = serve(monkeypatch, [encoded(category_data), page(42, "/next"), page(43)]) + config = configured(max_requests=allowance) + monkeypatch.setattr(cli, "load_config", lambda: config) + args = ["topic", "list", "--category", category, "--limit", "1"] + if since is not None: + args.extend(["--since", since]) + status = package_main(args) + output = capsys.readouterr() + assert len(requests) == allowance + assert all(body.closed for body in bodies) + if allowance == 2: + assert status == 1 and output.out == "" + assert "read budget exhausted" in output.err + assert output.err.count("lower --limit") == 1 + assert output.err.count("set --since to a more recent cutoff") == 1 + else: + assert status == 0 and output.err == "" + result = json.loads(output.out) + assert result["truncated"] is True + assert [thread["thread_id"] for thread in result["threads"]] == ["42"] + + +def test_cli_commands_have_independent_budgets( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + monkeypatch.setenv("DISCOURSE_URL", BASE) + monkeypatch.delenv("DISCOURSE_API_KEY", raising=False) + monkeypatch.delenv("DISCOURSE_API_USERNAME", raising=False) + requests, bodies = serve(monkeypatch, [topic([])]) + for _ in range(65): + assert package_main(["topic", "view", "42"]) == 0 + output = capsys.readouterr() + assert json.loads(output.out)["thread_id"] == "42" + assert output.err == "" + assert len(requests) == 65 + assert all(body.closed for body in bodies) + + +def test_bounded_reads_still_reject_premature_eof_after_valid_json(monkeypatch: pytest.MonkeyPatch) -> None: + class FakeSocket: + def makefile(self, *args: Any, **kwargs: Any) -> io.BytesIO: + return io.BytesIO(b"HTTP/1.1 200 OK\r\nContent-Length: 3\r\n\r\n{}") + + response = http.client.HTTPResponse(FakeSocket()) # type: ignore[arg-type] + response.begin() + monkeypatch.setattr(client, "_urlopen", lambda *args, **kwargs: response) + with pytest.raises(client.DiscourseError, match="ended before its declared Content-Length"): + client.get_json(configured(), "/latest.json") + assert response.isclosed() + + +def view_replies(total: int, max_posts: int, initial: int) -> list[Reply]: + ids = list(range(1, total + 1)) + missing = ids[initial:max_posts] + replies: list[Reply] = [topic(ids, initial=initial)] + replies.extend( + encoded({"post_stream": {"posts": posts(missing[start : start + 20])}}) + for start in range(0, len(missing), 20) + ) + return replies + + +def test_library_preflight_uses_parameter_names_without_cli_guidance( + monkeypatch: pytest.MonkeyPatch, +) -> None: + requests, bodies = serve(monkeypatch, [topic([1, 2])]) + with pytest.raises(client.RequestBudgetError, match="at least 1 more HTTP requests") as exc: + topics.fetch_topic(configured(max_requests=1), 42, max_posts=2) + assert "max_posts" in str(exc.value) + assert "--max-posts" not in str(exc.value) + assert len(requests) == 1 and bodies[0].closed + + +@pytest.mark.parametrize(("max_posts", "expected_status"), [(1280, 0), (1281, 1), (2000, 1)]) +def test_topic_preflight_rejects_known_impossible_batches( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str], max_posts: int, expected_status: int +) -> None: + requests, bodies = serve(monkeypatch, view_replies(3000, max_posts, 20)) + config = configured() + monkeypatch.setattr(cli, "load_config", lambda: config) + assert package_main(["topic", "view", "42", "--max-posts", str(max_posts)]) == expected_status + output = capsys.readouterr() + assert all(body.closed for body in bodies) + if expected_status: + assert len(requests) == 1 + assert output.out == "" + needed = (max_posts - 20 + 19) // 20 + assert f"at least {needed} more HTTP requests" in output.err + assert "63 remain" in output.err + assert output.err.count("lower --max-posts") == 1 + else: + assert len(requests) == 64 + result = json.loads(output.out) + assert len(result["messages"]) == 1280 and result["truncated"] is True + assert output.err == "" + + +@pytest.mark.parametrize( + ("total", "max_posts", "initial", "allowance", "expected_requests"), + [(40, 2000, 20, 64, 2), (3000, 1281, 40, 64, 64), (20, 2000, 20, 1, 1)], +) +def test_topic_preflight_uses_actual_missing_posts( + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + total: int, + max_posts: int, + initial: int, + allowance: int, + expected_requests: int, +) -> None: + requests, bodies = serve(monkeypatch, view_replies(total, max_posts, initial)) + monkeypatch.setattr(cli, "load_config", lambda: configured(max_requests=allowance)) + assert package_main(["topic", "view", "42", "--max-posts", str(max_posts)]) == 0 + output = capsys.readouterr() + result = json.loads(output.out) + assert len(result["messages"]) == min(total, max_posts) + assert result["truncated"] is (total > max_posts) + assert output.err == "" + assert len(requests) == expected_requests + assert all(body.closed for body in bodies) + + +@pytest.mark.parametrize(("allowance", "expected_status"), [(3, 1), (4, 0)]) +def test_topic_preflight_counts_initial_redirects_without_reserving_requests( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str], allowance: int, expected_status: int +) -> None: + replies: list[Reply] = [(302, {"Location": "/t/proposal/42.json"}, b"unused")] + replies.extend(view_replies(41, 41, 20)) + requests, bodies = serve(monkeypatch, replies) + monkeypatch.setattr(cli, "load_config", lambda: configured(max_requests=allowance)) + assert package_main(["topic", "view", "42"]) == expected_status + output = capsys.readouterr() + if expected_status: + assert len(requests) == 2 and output.out == "" + assert "at least 2 more HTTP requests" in output.err + assert "1 remain" in output.err and "--max-posts" in output.err + else: + assert len(requests) == 4 and output.err == "" + assert len(json.loads(output.out)["messages"]) == 41 + assert bodies[0].read_sizes == [] + assert all(body.closed for body in bodies) + + +@pytest.mark.parametrize( + ("limit", "more_after_last_page", "expected_status"), + [(1889, True, 0), (1890, True, 1), (1890, False, 0)], +) +def test_list_capacity_depends_on_end_signal_and_lookahead( + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + limit: int, + more_after_last_page: bool, + expected_status: int, +) -> None: + replies: list[Reply] = [encoded({"category": CATEGORY})] + replies.extend( + page(1 + number * 30, "/next" if number < 62 or more_after_last_page else None, count=30) + for number in range(63) + ) + requests, bodies = serve(monkeypatch, replies) + monkeypatch.setattr(cli, "load_config", configured) + assert package_main(["topic", "list", "--category", "7", "--limit", str(limit)]) == expected_status + output = capsys.readouterr() + assert len(requests) == 64 + assert all(body.closed for body in bodies) + if expected_status: + assert output.out == "" + assert "--limit" in output.err and "--since" in output.err + else: + result = json.loads(output.out) + assert len(result["threads"]) == limit + assert result["truncated"] is more_after_last_page + assert output.err == "" + + +@pytest.mark.parametrize("action", ["view", "list"]) +@pytest.mark.parametrize( + ("reply", "body_limit", "expected"), + [ + ((403, {}, encoded({"errors": ["denied"]})), 1000, "HTTP 403"), + (urllib.error.URLError("offline"), 1000, "offline"), + (b"not JSON", 1000, "not JSON"), + (encoded({"category": CATEGORY}), 1, "response-body bytes"), + ], +) +def test_count_hints_are_not_added_to_other_failures( + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + reply: Reply, + body_limit: int, + expected: str, + action: str, +) -> None: + requests, bodies = serve(monkeypatch, [reply]) + monkeypatch.setattr(cli, "load_config", lambda: configured(max_requests=1, max_body_bytes=body_limit)) + args = ["topic", "view", "42"] if action == "view" else ["topic", "list", "--category", "7"] + assert package_main(args) == 1 + output = capsys.readouterr() + assert output.out == "" and expected in output.err + assert all(flag not in output.err for flag in ("--max-posts", "--limit", "--since")) + assert len(requests) == 1 and all(body.closed for body in bodies) + + +@pytest.mark.parametrize("allowance", [1, 2]) +def test_spent_byte_budget_blocks_the_next_request(monkeypatch: pytest.MonkeyPatch, allowance: int) -> None: + requests, bodies = serve(monkeypatch, [b"{}"]) + config = configured(max_body_bytes=allowance) + if allowance == 1: + with pytest.raises(client.DiscourseError, match="response-body bytes"): + client.get_json(config, "/latest.json") + else: + assert client.get_json(config, "/latest.json") == {} + with pytest.raises(client.DiscourseError, match="response-body bytes"): + client.get_json(config, "/latest.json") + assert len(requests) == config.budget.requests == 1 + assert bodies[0].closed + + +def test_reused_and_replaced_configs_share_budget_but_fresh_configs_do_not( + monkeypatch: pytest.MonkeyPatch, +) -> None: + requests, bodies = serve(monkeypatch, [topic([1])]) + config = configured(max_requests=2) + topics.fetch_topic(config, 42) + topics.fetch_topic(replace(config), 42) + with pytest.raises(client.DiscourseError, match="2 HTTP requests"): + topics.fetch_topic(config, 42) + topics.fetch_topic(client.DiscourseConfig(BASE, None, None), 42) + assert len(requests) == 3 + assert all(body.closed for body in bodies) + + +@pytest.mark.parametrize("terminated", [False, True]) +def test_chunked_response_requires_terminator_even_after_valid_json( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str], terminated: bool +) -> None: + body = topic([]) + raw = ( + b"HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n" + + f"{len(body):x}\r\n".encode() + + body + + b"\r\n" + + (b"0\r\n\r\n" if terminated else b"") + ) + + class FakeSocket: + def makefile(self, *args: Any, **kwargs: Any) -> io.BytesIO: + return io.BytesIO(raw) + + response = http.client.HTTPResponse(FakeSocket()) # type: ignore[arg-type] + response.begin() + monkeypatch.setattr(client, "_urlopen", lambda *args, **kwargs: response) + monkeypatch.setattr(cli, "load_config", configured) + assert package_main(["topic", "view", "42"]) == (0 if terminated else 1) + output = capsys.readouterr() + if terminated: + assert json.loads(output.out)["thread_id"] == "42" and output.err == "" + else: + assert output.out == "" and "Failed to read the Discourse response" in output.err + assert response.isclosed() diff --git a/tools/discourse/tests/test_topic_list.py b/tools/discourse/tests/test_topic_list.py index 4c5cd053d..927153ed4 100644 --- a/tools/discourse/tests/test_topic_list.py +++ b/tools/discourse/tests/test_topic_list.py @@ -76,7 +76,9 @@ def serve( requests: list[urllib.request.Request] = [] replies = {"/c/7/show.json": {"category": CATEGORY}, **routes} - def open_request(request: urllib.request.Request, timeout: float, *, base_url: str) -> io.BytesIO: + def open_request( + request: urllib.request.Request, timeout: float, *, base_url: str, budget: client.ReadBudget + ) -> io.BytesIO: assert timeout == 30 requests.append(request) parsed = urllib.parse.urlsplit(request.full_url) diff --git a/tools/spec-loop/specs/adapters.md b/tools/spec-loop/specs/adapters.md index 6d5276395..6fdf160ec 100644 --- a/tools/spec-loop/specs/adapters.md +++ b/tools/spec-loop/specs/adapters.md @@ -253,7 +253,14 @@ by swapping the adapter, not the skill. Results preserve pinned/descending `bumped_at` order; since is inclusive, dates mean midnight UTC and timestamps require a timezone with at most microsecond precision. Summaries reuse `thread_summary` fields plus activity metadata; `last_message_date` maps only `last_posted_at`, preserving missing/null without fallback. Pinned topics count toward the positive limit (default 100); only an extra unique match proves truncation, and an older explicitly non-pinned topic ends a since scan. - Received pages are strictly validated; malformed data or failed requests produce no partial output. Lists are not consistent snapshots and have no pagination budget. + All reads in a command share limits of 64 HTTP request attempts (including redirects) + and 32 MiB of consumed response-body bytes. + Success and parsed error bodies use bounded reads; unused redirect bodies are closed without reading. + Budget exhaustion fails without successful JSON and does not change count-based truncation. + Topic reads reject a known request-budget shortfall before fetching missing-post batches; + HTTP failures retain their status if the error body exceeds the byte allowance. + The limits are internal defaults; the existing 30-second I/O timeout is not a total-duration deadline. + Received pages are strictly validated; malformed data or failed requests produce no partial output. Lists are not consistent snapshots. Only GET requests are permitted. Search, user activity, guarded writes and the remaining mail-archive verbs are tracked in #307.