From 4ada108d15ec3afa7dfb900a062144dbf6e4a1df Mon Sep 17 00:00:00 2001 From: FENG <223868679+MakerFlyFly@users.noreply.github.com> Date: Sat, 31 Oct 2026 03:48:59 +0000 Subject: [PATCH 1/3] feat(tools/discourse): bound read requests and response bodies Topic and category reads can accumulate requests and response bodies even when their output counts are capped. Share a command-level allowance of 64 HTTP attempts and 32 MiB of consumed response bodies across discovery, pagination, post batches and redirects. Fail incomplete reads without successful JSON and preserve count-based truncation. Read success and error bodies in bounded chunks, close unused redirect bodies without draining them, and preserve urllib routing and credential boundaries. Keep the existing I/O timeout without claiming a total deadline. Refs: https://github.com/apache/magpie/issues/307 Generated-by: Codex (GPT-6) --- tools/discourse/README.md | 30 +- .../discourse/src/magpie_discourse/client.py | 78 +++- tools/discourse/tests/test_discourse.py | 8 +- tools/discourse/tests/test_read_budget.py | 346 ++++++++++++++++++ tools/discourse/tests/test_topic_list.py | 4 +- tools/spec-loop/specs/adapters.md | 5 + 6 files changed, 456 insertions(+), 15 deletions(-) create mode 100644 tools/discourse/tests/test_read_budget.py diff --git a/tools/discourse/README.md b/tools/discourse/README.md index 352804b8d..57dfb254b 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,31 @@ 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. +The next request is refused before transmission when the request allowance is spent. + +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. +`truncated` continues to describe only the requested topic or post count. +Commands within the budgets retain their existing output. +These limits are internal defaults, with no CLI or environment overrides. +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 +210,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 +234,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/client.py b/tools/discourse/src/magpie_discourse/client.py index 55f5ce090..5dfb1a795 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,64 @@ class DiscourseError(Exception): """Raised when the Discourse bridge cannot complete a request.""" +@dataclass +class ReadBudget: + """Request attempts and consumed response-body bytes for one command.""" + + 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 DiscourseError(f"Discourse read budget exhausted: {self.max_requests} HTTP requests") + 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": @@ -139,6 +193,7 @@ class DiscourseConfig: 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: @@ -195,9 +250,11 @@ 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 | None = None +) -> 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 +271,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 +293,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 +301,7 @@ 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 (OSError, http.client.HTTPException, UnicodeDecodeError, json.JSONDecodeError): return message errors = body.get("errors") if isinstance(body, dict) else None diff --git a/tools/discourse/tests/test_discourse.py b/tools/discourse/tests/test_discourse.py index 21517e295..9426b7596 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) diff --git a/tools/discourse/tests/test_read_budget.py b/tools/discourse/tests/test_read_budget.py new file mode 100644 index 000000000..41e6affb5 --- /dev/null +++ b/tools/discourse/tests/test_read_budget.py @@ -0,0 +1,346 @@ +# 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 email.message import Message +from typing import Any + +import pytest + +from magpie_discourse import cli, client +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]) -> bytes: + return encoded( + { + "id": 42, + "title": "Proposal", + "posts_count": len(ids), + "post_stream": {"stream": ids, "posts": posts(ids[:1])}, + } + ) + + +def page(topic_id: int, more: str | None = None) -> bytes: + return encoded( + { + "topic_list": { + "topics": [ + { + "id": topic_id, + "title": "Proposal", + "posts_count": 1, + "created_at": DATE, + "bumped_at": DATE, + "pinned": False, + } + ], + "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 + + +def test_error_body_shares_the_remaining_allowance(monkeypatch: pytest.MonkeyPatch) -> None: + _, bodies = serve(monkeypatch, [b"{}", (403, {}, encoded({"errors": ["denied"]}))]) + config = configured(max_body_bytes=5) + 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_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: + first = topic(list(range(1, 43))) + batch = encoded({"post_stream": {"posts": posts(list(range(2, 22)))}}) + requests, bodies = serve(monkeypatch, [first, batch]) + config = ( + configured(max_requests=2) + if dimension == "requests" + else configured(max_body_bytes=len(first) + len(batch) - 1) + ) + 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 len(requests) == 2 + assert all(body.closed for body in bodies) + + +@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 +) -> 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) + status = package_main(["topic", "list", "--category", category, "--limit", "1"]) + 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 + 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() 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..d9b37622d 100644 --- a/tools/spec-loop/specs/adapters.md +++ b/tools/spec-loop/specs/adapters.md @@ -253,6 +253,11 @@ 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. + 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. + 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 and have no pagination budget. Only GET requests are permitted. Search, user activity, guarded writes and the remaining mail-archive verbs are From f8eb46e77ddb44463e3b75d8d95716da3103b3cc Mon Sep 17 00:00:00 2001 From: FENG <223868679+MakerFlyFly@users.noreply.github.com> Date: Sat, 31 Oct 2026 05:34:58 +0000 Subject: [PATCH 2/3] fix(tools/discourse): fail early and retain HTTP status Once the topic stream is known, reject a minimum batch count that cannot fit the remaining request budget before fetching any missing posts. Keep runtime accounting for redirects and report list-specific count hints only for request-budget failures. Retain HTTP status codes when an error body exhausts the byte allowance, and refuse a new request after the byte allowance is spent. Require an explicit budget in the transport helper and document config lifetime, conditional capacities and the existing timeout boundary. Refs: https://github.com/apache/magpie/issues/307 Generated-by: Codex (GPT-6) --- tools/discourse/README.md | 29 +- tools/discourse/src/magpie_discourse/cli.py | 12 +- .../discourse/src/magpie_discourse/client.py | 20 +- .../discourse/src/magpie_discourse/topics.py | 18 +- tools/discourse/tests/test_discourse.py | 9 +- tools/discourse/tests/test_read_budget.py | 259 ++++++++++++++++-- tools/spec-loop/specs/adapters.md | 4 +- 7 files changed, 315 insertions(+), 36 deletions(-) diff --git a/tools/discourse/README.md b/tools/discourse/README.md index 57dfb254b..2c0a65a6f 100644 --- a/tools/discourse/README.md +++ b/tools/discourse/README.md @@ -121,7 +121,29 @@ 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. -The next request is refused before transmission when the request allowance is spent. +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 and suggests lowering `--max-posts`. +This check does not reserve requests; redirects can still consume the remaining allowance later. +For list request-budget failures, the error suggests a lower `--limit` or a newer `--since`. + +Effective capacity depends on the response, not just the requested count. +For example, with 20 requested posts 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. @@ -133,9 +155,12 @@ 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 defaults, with no CLI or environment overrides. +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. diff --git a/tools/discourse/src/magpie_discourse/cli.py b/tools/discourse/src/magpie_discourse/cli.py index 5d9fd9a34..e84b831e7 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: @@ -39,7 +39,10 @@ def main(argv: Sequence[str] | None = None) -> int: topic_id = topics.parse_topic_ref(args.topic, config) result = topics.fetch_topic(config, topic_id, max_posts=args.max_posts) 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 use a newer --since") from exc else: # pragma: no cover - argparse rejects anything else raise AssertionError(f"unhandled command {args}") @@ -64,7 +67,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 +82,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 5dfb1a795..f226ee5c3 100644 --- a/tools/discourse/src/magpie_discourse/client.py +++ b/tools/discourse/src/magpie_discourse/client.py @@ -44,6 +44,10 @@ 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: """Request attempts and consumed response-body bytes for one command.""" @@ -56,7 +60,11 @@ class ReadBudget: def start_request(self) -> None: """Charge one permitted attempt before it reaches the transport.""" if self.requests >= self.max_requests: - raise DiscourseError(f"Discourse read budget exhausted: {self.max_requests} HTTP 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: @@ -188,7 +196,7 @@ 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 @@ -197,7 +205,7 @@ class DiscourseConfig: 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)") @@ -250,9 +258,7 @@ def api_url(config: DiscourseConfig, path: str, params: Mapping[str, QueryValue] return url -def _urlopen( - request: urllib.request.Request, timeout: float, *, base_url: str, budget: ReadBudget | None = None -) -> 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, budget) handler.check_request(request) @@ -302,6 +308,8 @@ def _http_error_message(exc: urllib.error.HTTPError, budget: ReadBudget) -> str: return f"{message} (rate limited; retry after {hint or 'a while'})" try: 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..7557ee15f 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 needs at least {requests_needed} more HTTP requests, " + f"but {remaining_requests} remain in the read budget; lower --max-posts" + ) 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 9426b7596..d3af8f4ff 100644 --- a/tools/discourse/tests/test_discourse.py +++ b/tools/discourse/tests/test_discourse.py @@ -765,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( @@ -977,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 index 41e6affb5..4e9f1a451 100644 --- a/tools/discourse/tests/test_read_budget.py +++ b/tools/discourse/tests/test_read_budget.py @@ -27,12 +27,13 @@ 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 +from magpie_discourse import cli, client, topics from magpie_discourse import main as package_main BASE = "https://forum.example.test" @@ -72,30 +73,31 @@ def posts(ids: list[int]) -> list[dict[str, Any]]: ] -def topic(ids: list[int]) -> bytes: +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[:1])}, + "post_stream": {"stream": ids, "posts": posts(ids[:initial])}, } ) -def page(topic_id: int, more: str | None = None) -> bytes: +def page(topic_id: int, more: str | None = None, *, count: int = 1) -> bytes: return encoded( { "topic_list": { "topics": [ { - "id": topic_id, + "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, } @@ -254,13 +256,28 @@ def test_redirect_status_without_location_keeps_bounded_error_body(monkeypatch: assert bodies[0].closed -def test_error_body_shares_the_remaining_allowance(monkeypatch: pytest.MonkeyPatch) -> None: - _, bodies = serve(monkeypatch, [b"{}", (403, {}, encoded({"errors": ["denied"]}))]) - config = configured(max_body_bytes=5) +@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") - 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 + 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) @@ -275,20 +292,26 @@ def test_429_uses_headers_and_closes_without_reading_body(monkeypatch: pytest.Mo def test_post_batches_share_budget_and_never_print_partial_success( monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str], dimension: str ) -> None: - first = topic(list(range(1, 43))) batch = encoded({"post_stream": {"posts": posts(list(range(2, 22)))}}) - requests, bodies = serve(monkeypatch, [first, batch]) - config = ( - configured(max_requests=2) - if dimension == "requests" - else configured(max_body_bytes=len(first) + len(batch) - 1) - ) + 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 len(requests) == 2 + assert len(requests) == expected_requests + assert bodies[1].consumed > 0 assert all(body.closed for body in bodies) @@ -310,6 +333,7 @@ def test_category_pages_and_lookahead_share_request_budget( if allowance == 2: assert status == 1 and output.out == "" assert "read budget exhausted" in output.err + assert "--limit" in output.err and "--since" in output.err else: assert status == 0 and output.err == "" result = json.loads(output.out) @@ -344,3 +368,200 @@ def makefile(self, *args: Any, **kwargs: Any) -> io.BytesIO: 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 + + +@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 "lower --max-posts" in output.err + 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( + ("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_list_count_hint_is_not_added_to_other_failures( + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + reply: Reply, + body_limit: int, + expected: str, +) -> None: + requests, bodies = serve(monkeypatch, [reply]) + monkeypatch.setattr(cli, "load_config", lambda: configured(max_requests=1, max_body_bytes=body_limit)) + assert package_main(["topic", "list", "--category", "7"]) == 1 + output = capsys.readouterr() + assert output.out == "" and expected in output.err + assert "--limit" not in output.err and "--since" not in output.err + 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/spec-loop/specs/adapters.md b/tools/spec-loop/specs/adapters.md index d9b37622d..6fdf160ec 100644 --- a/tools/spec-loop/specs/adapters.md +++ b/tools/spec-loop/specs/adapters.md @@ -257,8 +257,10 @@ by swapping the adapter, not the skill. 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 and have no pagination budget. + 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. From ae938cbec969c37e7bf6489f7672e0193b18ab2f Mon Sep 17 00:00:00 2001 From: FENG <223868679+MakerFlyFly@users.noreply.github.com> Date: Sat, 31 Oct 2026 07:23:52 +0000 Subject: [PATCH 3/3] fix(tools/discourse): keep budget guidance at the CLI boundary Keep topic-library errors focused on max_posts and the request allowance. Add command-line count guidance only in the view/list CLI handlers for request-budget failures, without attaching hints to unrelated errors. Clarify since guidance, shared budget accounting and the stream-prefix condition in the capacity example. Preserve all budget limits and read semantics. Refs: https://github.com/apache/magpie/issues/307 Generated-by: Codex (GPT-6) --- tools/discourse/README.md | 9 +++-- tools/discourse/src/magpie_discourse/cli.py | 7 +++- .../discourse/src/magpie_discourse/client.py | 2 +- .../discourse/src/magpie_discourse/topics.py | 4 +- tools/discourse/tests/test_read_budget.py | 38 +++++++++++++++---- 5 files changed, 44 insertions(+), 16 deletions(-) diff --git a/tools/discourse/README.md b/tools/discourse/README.md index 2c0a65a6f..c520f99d3 100644 --- a/tools/discourse/README.md +++ b/tools/discourse/README.md @@ -126,13 +126,14 @@ 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 and suggests lowering `--max-posts`. +fetching any batch. This check does not reserve requests; redirects can still consume the remaining allowance later. -For list request-budget failures, the error suggests a lower `--limit` or a newer `--since`. +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 20 requested posts in the initial response, batches of 20 and -no redirects, 64 requests can retrieve 1280 posts if the byte allowance also permits it. +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. diff --git a/tools/discourse/src/magpie_discourse/cli.py b/tools/discourse/src/magpie_discourse/cli.py index e84b831e7..93ec20bec 100644 --- a/tools/discourse/src/magpie_discourse/cli.py +++ b/tools/discourse/src/magpie_discourse/cli.py @@ -37,12 +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": 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 use a newer --since") from 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}") diff --git a/tools/discourse/src/magpie_discourse/client.py b/tools/discourse/src/magpie_discourse/client.py index f226ee5c3..1f273f60b 100644 --- a/tools/discourse/src/magpie_discourse/client.py +++ b/tools/discourse/src/magpie_discourse/client.py @@ -50,7 +50,7 @@ class RequestBudgetError(DiscourseError): @dataclass class ReadBudget: - """Request attempts and consumed response-body bytes for one command.""" + """Mutable request and response-body accounting shared through DiscourseConfig.""" max_requests: int = DEFAULT_MAX_REQUESTS max_body_bytes: int = DEFAULT_MAX_BODY_BYTES diff --git a/tools/discourse/src/magpie_discourse/topics.py b/tools/discourse/src/magpie_discourse/topics.py index 7557ee15f..4b67d9a48 100644 --- a/tools/discourse/src/magpie_discourse/topics.py +++ b/tools/discourse/src/magpie_discourse/topics.py @@ -97,8 +97,8 @@ def fetch_topic(config: DiscourseConfig, topic_id: int, max_posts: int = DEFAULT remaining_requests = config.budget.max_requests - config.budget.requests if requests_needed > remaining_requests: raise RequestBudgetError( - f"Discourse topic needs at least {requests_needed} more HTTP requests, " - f"but {remaining_requests} remain in the read budget; lower --max-posts" + 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] diff --git a/tools/discourse/tests/test_read_budget.py b/tools/discourse/tests/test_read_budget.py index 4e9f1a451..df5d81d99 100644 --- a/tools/discourse/tests/test_read_budget.py +++ b/tools/discourse/tests/test_read_budget.py @@ -310,15 +310,21 @@ def test_post_batches_share_budget_and_never_print_partial_success( 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 + 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]}} @@ -326,14 +332,18 @@ def test_category_pages_and_lookahead_share_request_budget( requests, bodies = serve(monkeypatch, [encoded(category_data), page(42, "/next"), page(43)]) config = configured(max_requests=allowance) monkeypatch.setattr(cli, "load_config", lambda: config) - status = package_main(["topic", "list", "--category", category, "--limit", "1"]) + 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 "--limit" in output.err and "--since" 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) @@ -381,6 +391,17 @@ def view_replies(total: int, max_posts: int, initial: int) -> list[Reply]: 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 @@ -397,7 +418,7 @@ def test_topic_preflight_rejects_known_impossible_batches( needed = (max_posts - 20 + 19) // 20 assert f"at least {needed} more HTTP requests" in output.err assert "63 remain" in output.err - assert "lower --max-posts" in output.err + assert output.err.count("lower --max-posts") == 1 else: assert len(requests) == 64 result = json.loads(output.out) @@ -483,6 +504,7 @@ def test_list_capacity_depends_on_end_signal_and_lookahead( assert output.err == "" +@pytest.mark.parametrize("action", ["view", "list"]) @pytest.mark.parametrize( ("reply", "body_limit", "expected"), [ @@ -492,19 +514,21 @@ def test_list_capacity_depends_on_end_signal_and_lookahead( (encoded({"category": CATEGORY}), 1, "response-body bytes"), ], ) -def test_list_count_hint_is_not_added_to_other_failures( +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)) - assert package_main(["topic", "list", "--category", "7"]) == 1 + 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 "--limit" not in output.err and "--since" not 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)