Skip to content

Commit b4da6f0

Browse files
fix(harness): keep decoding inside the read budget, and free what a failed start held
Round 3 security review of #20 at 0075138: - M1: a request keeps its reader until its body has been decoded and checked against the input schema, and calls are admitted before they are decoded and decoded one at a time. 600 sockets each sending 1 MiB of empty objects took the process to 5.9 GiB; they now peak at 333 MiB, the read budget plus one document. A payload the schema refuses is dropped before the next is decoded. - M2: a handler thread that fails to start releases the slot held for it, so later calls are not refused and shutdown does not wait on it. - N1: the failure code is checked again where the header is set. - N2: the harness exits with its code directly, so parked or writing connection threads cannot turn shutdown into an abort. - N3: a request beginning on a connection closed to make room is refused unread rather than run with nowhere to answer. - N4: a body over the whole read budget is refused 413, whatever the declared input bound. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 0075138 commit b4da6f0

3 files changed

Lines changed: 367 additions & 47 deletions

File tree

‎docs/protocol.md‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,10 @@ own: none is a value a placement resolved.
6767
| reading one request, head and body together (`REQUEST_READ_SECONDS`) | 10 s wall clock, from its first byte | `408` once the body has begun; a head that has not arrived is dropped unanswered |
6868
| the request line and headers (`MAX_HEAD_BYTES`) | 32 KiB | `431` |
6969
| requests being read at once (`READ_MEMORY_BYTES` ÷ (32 KiB + `max-input-bytes`), at most 1,024) | 256 MiB of heads and bodies: 248 readers at the default 1 MiB input | `503`, before any of the request is read |
70+
| one request's body, whatever the declaration (`READ_MEMORY_BYTES` − 32 KiB) | just under 256 MiB | `413`: a declared `max-input-bytes` larger than the whole read budget is held to the budget |
71+
| documents being decoded | one at a time | waits its turn, still holding its reader |
7072
| calls whose handler is running (the declared `concurrency`) | `HARNESS_CONCURRENCY`, 32, when the declaration states 0 | `503` |
71-
| open connections (readers + concurrency + `IDLE_CONNECTIONS`, 64) | — | the connection idle the longest is closed to make room; a new one is never refused |
73+
| open connections (readers + concurrency + `IDLE_CONNECTIONS`, 64) | — | the connection idle the longest is closed to make room; a new one is never refused, and a request that begins on a connection in the instant it is closed is refused unread rather than run with nowhere to answer |
7274
| an idle kept-alive connection (`IDLE_SECONDS`) | 120 s, advertised as `Keep-Alive: timeout=120` | closed |
7375
| one write of an answer (`WRITE_STALL_SECONDS`) | 10 s | the connection is dropped |
7476

@@ -81,6 +83,17 @@ call. A slot is held until the handler really returns, not until the call is
8183
answered: a call answered `504` at its deadline keeps its slot while its handler
8284
runs on, so the declared concurrency bounds author code running.
8385

86+
A request keeps its reader until its body has been decoded and checked against
87+
the input schema: a decoded document can be many times the bytes it came from —
88+
an array of empty objects is about twenty — so the readers' bound is a bound on
89+
decoding only while decoding sits inside it. The order is: read the body, take a
90+
concurrency slot (or answer `503` without decoding anything), decode and check
91+
the payload one call at a time, release the reader, run the handler. A payload
92+
the schema refuses is dropped before the next is decoded, so beyond the read
93+
budget the process holds one document being decoded and, under each running
94+
call's slot, a payload its contract admits — the envelope the declaration's
95+
`max-input-bytes` and `concurrency` state.
96+
8497
**Concurrency is not yet one number.** Core's declaration says `0` means the
8598
facility default and defines none, so the harness admits 32 when the declaration
8699
is silent. The durable-work runtime decides how many calls it dispatches from
@@ -143,7 +156,9 @@ The code becomes a response header, so it must match
143156
`^[a-zA-Z0-9][a-zA-Z0-9_.-]{0,63}$` — the grammar the durable-work runtime reads
144157
it with, which is narrower than core's 1 to 128 characters. A code outside it
145158
raises `ValueError` where it is constructed, and the handler's call then answers
146-
as any untyped error does: unproven.
159+
as any untyped error does: unproven. The code is read-only, and checked again
160+
where the header is set: a code changed after construction answers `500` with
161+
no failure code.
147162

148163
Everything else leaves the effect **unproven**, because the harness cannot know
149164
which side of its effect a handler stopped on. A caller that read a bare status
@@ -156,7 +171,7 @@ as proof would stop looking for a receipt that exists.
156171
| raised anything else | `500`, no failure code | unproven |
157172
| was still running at the deadline | `504`, no failure code | unproven |
158173
| returned an answer the contract refuses | `500`, no failure code | unproven |
159-
| was never reached (payload, identity, effect id, framing) | `400`/`408`/`411`/`431`, no failure code | unproven |
174+
| was never reached (payload, identity, effect id, framing) | `400`/`408`/`411`/`413`/`431`, no failure code | unproven |
160175
| was never reached (over a bound, or stopping) | `503`, no failure code | unproven |
161176

162177
The statuses are for humans and proxies: core classifies on the failure-code
@@ -185,7 +200,9 @@ already open, which answer `503` with `Connection: close` — and gives the call
185200
in flight `SHUTDOWN_GRACE_SECONDS` (30 s) to finish — including a handler whose
186201
call was already answered at its deadline. Cutting them off would turn every one
187202
of them into an unproven effect at once. The harness exits `0` as soon as the
188-
last handler has returned; an idle connection holds nothing. If the grace ends
203+
last handler has returned; an idle connection holds nothing. The process ends
204+
with that code directly rather than through interpreter shutdown, which can
205+
abort while connection threads are still parked or writing a diagnostic. If the grace ends
189206
with handlers still running, they are cut off and the harness exits `1`, saying
190207
how many effects it left unproven.
191208

‎pkg/harness/codefly_runnable/runner.py‎

Lines changed: 117 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
from .protocol import (
3636
ADDRESS_VARIABLE,
3737
FAILURE_CODE_HEADER,
38+
FAILURE_CODE_PATTERN,
3839
HARNESS_CONCURRENCY,
3940
IDLE_CONNECTIONS,
4041
IDLE_SECONDS,
@@ -194,8 +195,22 @@ def stop(*_: object) -> None:
194195
"their effects are unproven",
195196
file=sys.stderr,
196197
)
197-
return 1
198-
return 0
198+
_exit(1 if remaining else 0)
199+
return 1 if remaining else 0
200+
201+
202+
def _exit(code: int) -> None:
203+
"""End the process with this code, whatever its connection threads are doing.
204+
205+
Interpreter finalization with daemon threads still parked on connections —
206+
or holding the lock of a stream they are writing a diagnostic to — can
207+
abort instead of exiting, and a placement would read a crash where the code
208+
says what happened to the calls. Streams get a bounded moment to flush.
209+
"""
210+
flushing = threading.Thread(target=lambda: (sys.stdout.flush(), sys.stderr.flush()), daemon=True)
211+
flushing.start()
212+
flushing.join(2.0)
213+
os._exit(code)
199214

200215

201216
class _Input(socket.SocketIO):
@@ -269,6 +284,11 @@ def __init__(self, host: str, port: int, handler, *, readers: int, connections:
269284
super().__init__((host, port), handler)
270285

271286
def process_request(self, request, client_address) -> None:
287+
self.register(request)
288+
super().process_request(request, client_address)
289+
290+
def register(self, request) -> None:
291+
"""Count a new connection in, idle, closing the one idle the longest if it is at the bound."""
272292
with self._lock:
273293
if len(self._open) >= self._connections:
274294
oldest = next((connection for connection, idle in self._open.items() if idle), None)
@@ -279,14 +299,26 @@ def process_request(self, request, client_address) -> None:
279299
except OSError:
280300
pass
281301
self._open[request] = True
282-
super().process_request(request, client_address)
283302

284-
def idle(self, request, idle: bool) -> None:
285-
"""Mark a connection idle (moved to the back of the eviction order) or busy."""
303+
def idle(self, request) -> None:
304+
"""Mark a connection idle again, at the back of the eviction order."""
286305
with self._lock:
287306
if request in self._open:
288307
del self._open[request]
289-
self._open[request] = idle
308+
self._open[request] = True
309+
310+
def claim(self, request) -> bool:
311+
"""Mark a connection busy, unless it was already closed to make room.
312+
313+
A connection is closed to make room only while it is idle, so a
314+
request that began on it in that same instant is refused before it
315+
is read, rather than run with no connection to answer on.
316+
"""
317+
with self._lock:
318+
if request not in self._open:
319+
return False
320+
self._open[request] = False
321+
return True
290322

291323
def shutdown_request(self, request) -> None:
292324
with self._lock:
@@ -296,6 +328,11 @@ def shutdown_request(self, request) -> None:
296328

297329
def _server(address: str, contract: Contract, calls: _Calls, handle, receipt_of) -> ThreadingHTTPServer:
298330
host, port = _listen(address)
331+
decoding = threading.Lock()
332+
readers = max(1, min(MAX_READERS, READ_MEMORY_BYTES // (MAX_HEAD_BYTES + contract.max_input_bytes)))
333+
# The largest body one reader may hold. A declaration whose input bound is
334+
# larger than the whole budget is held to the budget instead.
335+
readable = READ_MEMORY_BYTES - MAX_HEAD_BYTES
299336

300337
class Handler(BaseHTTPRequestHandler):
301338
protocol_version = "HTTP/1.1"
@@ -313,7 +350,6 @@ def setup(self) -> None:
313350

314351
def handle_one_request(self) -> None:
315352
"""Read one request under the harness's bounds, then serve it."""
316-
self.server.idle(self.connection, True)
317353
self._input.deadline = None
318354
try:
319355
begun = self._body.peek(1)
@@ -324,7 +360,9 @@ def handle_one_request(self) -> None:
324360
# connection was closed to make room for a new one.
325361
self.close_connection = True
326362
return
327-
self.server.idle(self.connection, False)
363+
if not self.server.claim(self.connection):
364+
self.close_connection = True
365+
return
328366
self._prepare()
329367
if not self.server.readers.acquire(blocking=False):
330368
# Refused before a byte of it is read, so no peer can make the
@@ -342,6 +380,9 @@ def handle_one_request(self) -> None:
342380
finally:
343381
self.rfile = self._body
344382
self._done_reading()
383+
# Back of the eviction order: the connection idle the longest
384+
# is the one whose last request ended first.
385+
self.server.idle(self.connection)
345386

346387
def _prepare(self) -> None:
347388
"""Make an answer possible before a request line was parsed."""
@@ -375,19 +416,29 @@ def do_POST(self) -> None: # noqa: N802 — BaseHTTPRequestHandler's spelling
375416
if serve is None:
376417
self._answer(_Answer(404, code="unimplemented", message=f"{self.path} is not served"))
377418
return
378-
call = self._call()
379-
if isinstance(call, _Answer):
380-
self._answer(call)
419+
body = self._read()
420+
if isinstance(body, _Answer):
421+
self._answer(body)
381422
return
382423
# A slot is taken only once the whole request has arrived, so a
383-
# request still arriving never holds one.
424+
# request still arriving never holds one — and before the body is
425+
# decoded, so a call refused for want of one is never decoded.
384426
slot = calls.admit()
385427
if isinstance(slot, str):
428+
self._done_reading()
386429
self._answer(_Answer(503, code="unavailable", message=slot))
387430
return
388431
try:
389432
try:
390-
answer = serve(call, slot)
433+
try:
434+
call = self._decode(body)
435+
finally:
436+
# The request stays under the readers' memory bound
437+
# until it is decoded; from here the decoded call is
438+
# held under its concurrency slot instead.
439+
body = None
440+
self._done_reading()
441+
answer = call if isinstance(call, _Answer) else serve(call, slot)
391442
except Exception as err: # noqa: BLE001 — a defect in the framing still answers
392443
answer = _Answer(500, code="internal", message=_describe(err))
393444
self._answer(answer)
@@ -424,31 +475,55 @@ def _framing(self) -> int | _Answer:
424475
if declared > contract.max_input_bytes:
425476
return _Answer(400, code="invalid_argument", message=(
426477
f"input is {declared} bytes, over the declared {contract.max_input_bytes} byte bound"))
478+
if declared > readable:
479+
return _Answer(413, code="resource_exhausted", message=(
480+
f"input is {declared} bytes, over the {readable} bytes one request may hold in memory"))
427481
return declared
428482

429-
def _call(self) -> Call | _Answer:
430-
"""Read exactly one call off the connection, or refuse it."""
483+
def _read(self) -> bytes | _Answer:
484+
"""Read exactly one call's body off the connection, or refuse it.
485+
486+
The request keeps its reader until it has been decoded: the bytes
487+
read here count against the readers' memory bound.
488+
"""
489+
declared = self._framing()
490+
if isinstance(declared, _Answer):
491+
return declared
431492
try:
432-
declared = self._framing()
433-
if isinstance(declared, _Answer):
434-
return declared
435-
try:
436-
body = self._body.read(declared)
437-
except TimeoutError:
438-
return _Answer(408, code="deadline_exceeded",
439-
message=f"the request was not read within {REQUEST_READ_SECONDS:g}s")
440-
finally:
441-
self._done_reading()
493+
body = self._body.read(declared)
494+
except TimeoutError:
495+
return _Answer(408, code="deadline_exceeded",
496+
message=f"the request was not read within {REQUEST_READ_SECONDS:g}s")
442497
if len(body) != declared:
443498
return _Answer(400, code="invalid_argument", message="the body ended before its Content-Length")
444-
try:
445-
return read_call(self.headers, body, contract)
446-
except ProtocolError as err:
447-
return _Answer(400, code="invalid_argument", message=str(err))
499+
return body
500+
501+
def _decode(self, body: bytes) -> Call | _Answer:
502+
"""Decode one call and check it against the input schema, one call at a time.
503+
504+
A decoded document can be many times the bytes it came from — an
505+
array of empty objects is about twenty — so concurrent decoding
506+
would put memory beyond any bound on what is read. A document the
507+
schema refuses is dropped before the next is decoded, so only a
508+
call the contract admits outlives this, under its concurrency
509+
slot. Decoding and checking are CPU-bound under the interpreter
510+
lock, so doing them one call at a time costs no throughput.
511+
"""
512+
with decoding:
513+
try:
514+
call = read_call(self.headers, body, contract)
515+
contract.input.validate(call.payload, "input")
516+
except (ProtocolError, SchemaError) as err:
517+
return _Answer(400, code="invalid_argument", message=str(err))
518+
return call
448519

449520
def _answer(self, answer: _Answer) -> None:
450521
# Anything but a success may have left request bytes unread, and a
451522
# stopping harness keeps no connection for a call it will refuse.
523+
if answer.failure is not None and not _asserts(answer.failure):
524+
# Checked again where it is sent: a code changed after it was
525+
# constructed asserts nothing, and never reaches a header.
526+
answer = _Answer(500, code="internal", message="the handler's failure code changed after it was checked")
452527
close = answer.status != 200 or calls.stopping
453528
self.connection.settimeout(WRITE_STALL_SECONDS)
454529
if answer.failure is not None:
@@ -481,10 +556,14 @@ def log_request(self, *_: object) -> None:
481556
def log_message(self, *_: object) -> None:
482557
"""Diagnostics are what the harness prints deliberately, not a request log."""
483558

484-
readers = max(1, min(MAX_READERS, READ_MEMORY_BYTES // (MAX_HEAD_BYTES + contract.max_input_bytes)))
485559
return _Server(host, port, Handler, readers=readers, connections=readers + calls.limit + IDLE_CONNECTIONS)
486560

487561

562+
def _asserts(failure: HandlerFailure) -> bool:
563+
"""Whether a failure's code is still one the grammar admits."""
564+
return isinstance(failure.code, str) and FAILURE_CODE_PATTERN.fullmatch(failure.code) is not None
565+
566+
488567
def _listen(address: str) -> tuple[str, int]:
489568
"""Split the allocated host:port.
490569
@@ -499,12 +578,7 @@ def _listen(address: str) -> tuple[str, int]:
499578

500579

501580
def _invoke(contract: Contract, handle, call: Call, slot: _Slot) -> _Answer:
502-
"""Answer one call: validate the payload, run the handler, validate what it returned."""
503-
try:
504-
contract.input.validate(call.payload, "input")
505-
except SchemaError as err:
506-
return _Answer(400, code="invalid_argument", message=str(err))
507-
581+
"""Answer one call whose payload the contract admits: run the handler, validate what it returned."""
508582
outcome = _run(handle, _context(contract, call), call, slot)
509583
if outcome.failure is not None or outcome.status != 200:
510584
return outcome
@@ -519,16 +593,11 @@ def _invoke(contract: Contract, handle, call: Call, slot: _Slot) -> _Answer:
519593

520594

521595
def _lookup(contract: Contract, receipt_of, call: Call, slot: _Slot) -> _Answer:
522-
"""Answer a recovery: what did this effect commit?
596+
"""Answer a recovery whose payload the contract admits: what did this effect commit?
523597
524598
Absent answers 404 and is never an error — absent is "not yet known", and a
525599
caller reads it as inconclusive rather than as permission to invoke again.
526600
"""
527-
try:
528-
contract.input.validate(call.payload, "input")
529-
except SchemaError as err:
530-
return _Answer(400, code="invalid_argument", message=str(err))
531-
532601
outcome = _run(receipt_of, _context(contract, call), call, slot)
533602
if outcome.failure is not None or outcome.status != 200:
534603
return outcome
@@ -565,7 +634,12 @@ def invoke() -> None:
565634

566635
worker = threading.Thread(target=invoke, daemon=True)
567636
slot.hold()
568-
worker.start()
637+
try:
638+
worker.start()
639+
except BaseException:
640+
# The thread that would have released this hold never ran.
641+
slot.release()
642+
raise
569643
worker.join(call.budget)
570644
if worker.is_alive():
571645
return _Answer(504, code="deadline_exceeded", message="the deadline passed")

0 commit comments

Comments
 (0)