fix(orchestrator): address review on host control server transport (#468)
prd-number-check / require-numbered-prds (pull_request) Failing after 12s
test / integration-docker (pull_request) Successful in 19s
tracker-policy-pr / check-pr (pull_request) Successful in 9s
test / unit (pull_request) Failing after 52s
lint / lint (push) Successful in 1m1s
test / coverage (pull_request) Has been skipped
prd-number-check / require-numbered-prds (pull_request) Failing after 12s
test / integration-docker (pull_request) Successful in 19s
tracker-policy-pr / check-pr (pull_request) Successful in 9s
test / unit (pull_request) Failing after 52s
lint / lint (push) Successful in 1m1s
test / coverage (pull_request) Has been skipped
Codex review on #496: - **High — ambiguous delivery no longer orphans a launched bottle.** A timeout / dropped response from the host controller is now the ambiguous BrokerUnavailableError (distinct from the definite BrokerAuthError / BrokerClientError). OrchestratorCore.launch_bottle keeps the registry row on the ambiguous case instead of deregistering — deregistering would orphan a running container with no record (reconcile reaps rows, never containers). The row is left for reconcile to reap iff the bottle is not actually live. Definite failures still roll back, so a real failure leaves no orphan row. - **Medium — the privileged endpoint bounds request bodies.** The host server rejects an oversized Content-Length with 413 before reading it, and sets a per-request socket timeout, so a caller that can merely reach the socket (no signed token) can't exhaust memory or a handler thread. Tests: ambiguous-keep vs definite-rollback in the launch path; the BrokerUnavailableError/BrokerClientError split in BrokerClient; the 413 body cap + handler error paths (driven in-thread, since daemon request threads lose coverage); and the __main__ entrypoint broker selection. Diff-coverage 98%; pyright clean; pylint 9.88. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -57,6 +57,16 @@ BROKER_SECRET_ENV = "BOT_BOTTLE_BROKER_SECRET"
|
||||
# (8099) — a separate privileged component listening on its own socket.
|
||||
DEFAULT_PORT = 8091
|
||||
|
||||
# Cap on the request body. A signed broker request is tiny, so rejecting anything
|
||||
# larger *before reading it* keeps a caller that can merely reach the socket (no
|
||||
# signed token needed) from exhausting memory or a handler thread with a huge
|
||||
# Content-Length — the signed token, not mere reachability, is the authority.
|
||||
MAX_BODY_BYTES = 64 * 1024
|
||||
|
||||
# Per-request socket timeout, bounding how long a stalled / slow-loris caller can
|
||||
# hold a handler thread on this privileged listener.
|
||||
REQUEST_TIMEOUT_SECONDS = 15
|
||||
|
||||
|
||||
def _parse_json_object(body: bytes) -> Json:
|
||||
"""Parse a JSON object body. Raises ValueError for non-objects / bad JSON."""
|
||||
@@ -130,6 +140,10 @@ def dispatch( # pylint: disable=too-many-return-statements
|
||||
class Handler(http.server.BaseHTTPRequestHandler):
|
||||
"""Thin stdlib adapter: read the body, call `dispatch`, write JSON."""
|
||||
|
||||
# Socket timeout per request (applied by StreamRequestHandler.setup) so a
|
||||
# stalled caller can't pin a handler thread on this privileged listener.
|
||||
timeout = REQUEST_TIMEOUT_SECONDS
|
||||
|
||||
# Quiet by default; opt back into stdlib access logging with
|
||||
# BOT_BOTTLE_HOST_CONTROLLER_DEBUG (the controller has its own logging).
|
||||
def log_message(self, format: str, *args: typing.Any) -> None: # noqa: A002
|
||||
@@ -137,12 +151,21 @@ class Handler(http.server.BaseHTTPRequestHandler):
|
||||
super().log_message(format, *args)
|
||||
|
||||
def _serve(self, method: str) -> None:
|
||||
"""Read the request body, dispatch it, and write the JSON reply. A
|
||||
dispatch that raises (it shouldn't — dispatch is total) still returns a
|
||||
500 rather than dropping the connection."""
|
||||
"""Read the request body (bounded), dispatch it, and write the JSON
|
||||
reply. A dispatch that raises (it shouldn't — dispatch is total) still
|
||||
returns a 500 rather than dropping the connection."""
|
||||
server = self.server
|
||||
assert isinstance(server, HostControlServer)
|
||||
length = int(self.headers.get("Content-Length") or 0)
|
||||
try:
|
||||
length = int(self.headers.get("Content-Length") or 0)
|
||||
except ValueError:
|
||||
self._reply(400, {"error": "invalid Content-Length"})
|
||||
return
|
||||
if length < 0 or length > MAX_BODY_BYTES:
|
||||
# Reject before reading: nothing legitimate is this big, so an
|
||||
# oversized declared length is a bug or a resource-exhaustion attempt.
|
||||
self._reply(413, {"error": "request body too large"})
|
||||
return
|
||||
body = self.rfile.read(length) if length > 0 else b""
|
||||
try:
|
||||
status, payload = dispatch(server.broker, method, self.path, body)
|
||||
@@ -150,6 +173,10 @@ class Handler(http.server.BaseHTTPRequestHandler):
|
||||
sys.stderr.write(f"host controller: {method} {self.path} failed: {e!r}\n")
|
||||
sys.stderr.flush()
|
||||
status, payload = 500, {"error": f"internal error: {e}"}
|
||||
self._reply(status, payload)
|
||||
|
||||
def _reply(self, status: int, payload: typing.Mapping[str, object]) -> None:
|
||||
"""Write one JSON response with an explicit Content-Length."""
|
||||
data = json.dumps(payload).encode()
|
||||
self.send_response(status)
|
||||
self.send_header("Content-Type", "application/json")
|
||||
|
||||
Reference in New Issue
Block a user