Conversation
jat255
added
py
Contributor
|
Preview root: https://posit-dev.github.io/commons/pr-351/ Python site preview: https://posit-dev.github.io/commons/pr-351/py/ Built from the latest commit on this branch. The R links in it point at the published R site, which no pull request rebuilds. |
The wire between the execution driver and its worker: one JSON object per line, with values crossing by copy. Frames go as Arrow IPC and arrive as whichever library sent them; scalars go as JSON; anything that can only live in the process holding it crosses as its repr, the way a REPL would show it. No pickle in either direction, which is why `multiprocessing` and `ProcessPoolExecutor` were both rejected. `pickle.loads` runs the opcodes it is handed, and in this direction those opcodes were written by whatever the worker just ran. A test asserts the property directly rather than a comment claiming it: encoding never asks a value how to pickle itself. The module imports standalone, without `commons`, because the worker runs under `-I` with only its own directory on the path and has no business holding the agent's dependencies. A subprocess test pins that. Nothing consumes this yet; the worker and the driver are separate work.
Everything the driver reads was written by a process running model-written code, so the shape of a line is never a given. Three ways a bad one escaped as something other than a `ProtocolError`: A fixed field of the wrong type constructed a message anyway. An id arriving as an object then failed in the driver that keys its in-flight call on it, a long way from the line that carried it. A line that was not UTF-8 raised `UnicodeDecodeError` out of `json.loads`, which the JSON-specific handler did not cover. A frame with a column Arrow cannot hold raised out of the encoder, against the codec's own contract that a value which cannot cross falls back to its repr. One unconvertible column now costs the value rather than the call.
Arrow reports a refusal across several exception types, and only `ArrowInvalid` and `ArrowTypeError` are `ValueError` and `TypeError`. A pandas column of complex numbers raises `ArrowNotImplementedError`, which escaped both the encoder's fallback and the decoder's error boundary. Encoding now answers `None` for a frame Arrow will not carry, since what the caller does with that is fall back rather than fail, and decoding turns the same family into a `ProtocolError`. `ArrowIOError` is named separately because it does not descend from `ArrowException`.
- cap decoded Arrow frames at FRAME_BYTES_LIMIT so a compressed IPC body cannot exhaust the driver - raise a latched ChannelError on stream overrun, not a recoverable ProtocolError, so the channel is never resynchronised onto bytes the far end positioned - make encode_value total: numpy scalars cross as JSON, a Series as a one-column frame, duplicate columns deduplicated, and whatever is left as a repr that cannot itself raise - shrink oversized messages (clip output, then values to reprs) rather than emit a line no reader can consume - refuse ImportError and RecursionError as ProtocolError on decode, and abbreviate rejected lines in error messages - replace the field registries with explicit match statements
- isinstance checks in encode_value are model-controlled (`__class__` is assignable), so classification now sits inside the fallback boundary - clip Error.message alongside the traceback, so a giant exception string cannot cost the error response - document why _read_capped's cap fires after one batch is materialized: the worker had to hold that batch to compress it
The cap fired only after pyarrow had allocated a batch, and a crafted stream can claim any uncompressed length without holding the bytes. Walk the IPC framing and flatbuffer metadata first and charge each batch its declared size — compression prefixes, declared buffer lengths, a pointer per row for null-only batches — refusing over-cap streams before pyarrow sees them. _read_capped stays as the runtime backstop.
A lying count would churn tuple allocations until the reads ran out of message. Validate it against the metadata it lives in, and walk entries without materializing a list.
A null column carries no Arrow buffer, so a frame of nothing but null columns was kilobytes on the wire, reported zero `nbytes`, and cost the driver a pointer per cell once pandas held it: 20,000 x 30,000 is 1.2 MB of stream and 4.8 GB of frame, which both caps waved through. Nesting one is cheaper still, at a few hundred bytes for a row of `list<null>` holding as many nulls as an offset can count. The pre-flight now reads the schema, which is the only part of a stream that says which fields are free, and charges each null field by the length its own node declares. The charge is pandas's alone: Arrow holds a null column as a length and polars keeps it that way, so a frame bound for either is no dearer than its buffers, and charging it anyway would refuse what this module is willing to encode. A dictionary is one node of indices however deep the type behind it, and its values reach pandas by index rather than a pointer each, so the walk stops there. The runtime backstop walks into nested nulls rather than weighing top-level columns, so the two agree instead of sharing a blind spot. Frame reconstruction moves behind its own catch. pandas and polars are handed a table the far end chose the bytes and the library name for separately, and their refusals are ordinary exceptions of their own making, which escaped the boundary as themselves. Also: refuse a node of negative length, and a compressed buffer too short to hold its length prefix, both of which paid a batch back for what it declared; compare a frame against the line limit before base64 and with room for the message around it, so a frame that cannot fit falls back at once instead of being encoded and thrown away; and cover the refusal branches, the repr clip, and the scalars that stay reprs on purpose, each pinned by disabling the code it covers.
jat255
force-pushed
the
jat255/m6-0qnc-protocol
branch
from
September 14, 2026 16:20
a9d74d7 to
0610fbf
Compare
jat255
commented
Sep 14, 2026
|
|
||
| # The largest a frame may be after Arrow decompression. IPC bodies can be | ||
| # compressed, so a line well under STREAM_LIMIT can decode to far more. | ||
| # 1 GiB covers any real frame while bounding what a hostile worker can make |
Collaborator
Author
There was a problem hiding this comment.
The worker lifecycle task has to pass this to create_subprocess_exec, since the 64 KiB default fails on the first frame-shaped result.
jat255
commented
Sep 14, 2026
| if data is not None and len(data) <= _FRAME_WIRE_LIMIT: | ||
| return { | ||
| "encoding": "arrow", | ||
| "library": library, |
Collaborator
Author
There was a problem hiding this comment.
Judgment call worth a second opinion: one column Arrow cannot hold costs the value rather than the call, on the grounds that the model may not have wanted it.
jat255
commented
Sep 14, 2026
| try: | ||
| _check_ipc_size(data, max_frame_bytes, cells) | ||
| reader = pyarrow.ipc.open_stream(pyarrow.py_buffer(data)) | ||
| table = _read_capped(reader, pyarrow, max_frame_bytes, cells) |
Collaborator
Author
There was a problem hiding this comment.
Judgment call worth a second opinion: only pandas is charged for null cells, because Arrow and polars hold a null column as a length (measured at 200M rows, ~0 bytes), and charging them would refuse frames this module itself encodes.
Refactor docstrings and comments throughout the protocol module for better clarity, consistency, and emphasis on security considerations. Rephrase passive constructions, clarify error handling, and improve explanations of limits and untrusted input handling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds the wire between the code-execution driver and its worker: one JSON object per line, with values crossing by copy as Arrow IPC for frames and JSON for scalars. A frame that would cost the driver more than the channel allows is refused before it decompresses. Nothing consumes it yet, since the worker and the driver are separate units of work.
Pickling is forbidden in either direction, because it is inherently insecure. Loading a pickle runs whatever opcodes it is handed, and on the way back those opcodes were written by whatever the worker just ran.
Design decisions and review notes (agent-written)
Arrow rides base64 inside the JSON line. The alternative was a second binary channel, but the
exec-shaped backend offers stdin, stdout and stderr and nothing else, and a container-hosted backend may not pass extra file descriptors. One channel is what keeps a Connect backend a drop-in.Frames arrive as whichever library sent them. The payload names the library and the far end rebuilds with it. The driver and the worker share an interpreter and site-packages, so it is always available.
commonsaccepts pandas and polars everywhere else without preferring one, and the transport should not quietly impose a choice on the model.A value that cannot cross becomes an
OpaqueValuecarrying its repr. An open connection or a fitted model has no copy. Refusing outright would fail a whole call over a value the model may not want, and a repr is what a REPL shows. Frames are not exempt: a column Arrow cannot hold costs the value rather than the call.The frame cap counts what pandas will hold, not what Arrow sends. A null column writes no buffer at all, so a wide frame of them, or a single row of
list<null>, is kilobytes on the wire and a pointer per cell once pandas has it. The schema is the only part of a stream that says which fields write nothing. The pre-flight reads it, then charges each such field by the length its own node declares. Only pandas pays. Arrow holds a null column as a length and polars keeps it that way. Charging a frame bound for either would refuse what this module is willing to encode.Nothing crosses the frame boundary as its own exception. The far end chooses the bytes and the library name separately, so a table Arrow accepts can still be one pandas or polars will not hold. A duplicate column name is the everyday case for polars. Those refusals arrive as
ProtocolError, like any other malformed payload.JSON nesting depth is bounded by the protocol, not the interpreter. Before 3.14,
json.dumpsandjson.loadsrefused deep nesting withRecursionError; the rewritten parser and encoder are iterative and never will, while everything a value meets after the codec —repr, re-serialization, display — still recurses._JSON_DEPTH_LIMITkeeps the codec the single chokepoint: a value nested past it crosses as its repr, and a line nested past it is refused. The check is a stack of iterators, so its memory tracks depth rather than breadth — a wide hostile line cannot exhaust the driver between parse and refusal.STREAM_LIMITis exported and has to be used.asynciogives a stream 64 KiB by default, which any real frame exceeds, so the worker lifecycle task has to pass it tocreate_subprocess_exec. Worth naming now rather than meeting as a mysterious failure on the first result that matters.The module imports standalone, without
commons. The worker runs under-Iwith only its own directory on the path, and has no business holding the agent's dependencies. A subprocess test pins this using the environment allowlist from #251.No
tests/shared/fixture: the R package usescallr's own transport, so this protocol is not a cross-language contract.Every commit was reviewed by roborev, and every finding is fixed or dismissed with the reasoning recorded on the job. Three are worth a reviewer's attention:
ArrowNotImplementedError, which is not aValueErrorand escaped the fallback.tolist()bypassed the JSON depth check.nbytes, which Arrow reports as zero for a null column however many cells it holds.Two findings were dismissed with evidence on the job, 608 and 737. Each refusal the cap adds has a test that fails when the code it covers is disabled.