Carry an offboard observation's frames through a shared-memory ring - #736
Carry an offboard observation's frames through a shared-memory ring#736v-positronic wants to merge 16 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82bd98b64c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| def channel_path(socket_path: str) -> str: | ||
| """The descriptor socket that belongs to the session socket at ``socket_path``.""" | ||
| return socket_path + SOCKET_SUFFIX |
There was a problem hiding this comment.
Rule primitive-type violated:
channel_path, FrameWriter, and FrameChannel represent filesystem socket paths as str, forcing callers to keep re-deriving that domain. Accept and retain Path values throughout these new APIs, converting parsed URL or CLI strings once at the boundary and using os.fspath() only where the socket API requires it.
AGENTS.md reference: AGENTS.md:L14-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declined, and left open for the driver.
The offboard package types a socket path as str end to end: InferenceClient.uds, PolicyServer.uds, and uvicorn.Config(uds=...). Taking Path here alone would put a Path/str seam in the middle of one package, and every value would be converted twice on the way through. The rule's own carve-out fits: this module passes the path untouched to socket.connect and socket.bind and derives nothing from it.
Converting the package's socket paths once, where the URL is parsed, is a change to the unix:// carrier in #735 rather than to the ring.
| # One bad handover must not take the channel down. | ||
| except Exception: | ||
| logger.exception('A frame ring handover failed') |
There was a problem hiding this comment.
Rule swallowed-error violated:
_accept_forever catches every Exception and continues, so unexpected programming errors in _take_ring only leave clients timing out while the channel stays alive. Catch and log the specific malformed-handover and socket exceptions that are safe to isolate, and let unexpected failures terminate the channel.
AGENTS.md reference: AGENTS.md:L14-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declined, and left open for the driver.
The premise does not hold: a failed handover closes the connection, so the client's recv returns empty at once and _hand_over raises RuntimeError. Nothing times out, and the failure reaches the caller with a message.
The catch is around one peer's handover, and the peer is what this module cannot trust. A programming error there raises with logger.exception, which prints the traceback, and the client sees it on the same call. Letting it kill the accept thread would take every later session's ring with it, which is a larger failure than the one it reports.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c40f2dc89
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
3c40f2d to
56d5481
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56d5481aa7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37b70dc11c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39865e5b62
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
39865e5 to
a14f242
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4000708ee
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95da341716
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3ef45a0d5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| header = serialization.deserialise(message) | ||
| ring = MappedRing(fds[0], header[_SLOTS], header[_SLOT_BYTES]) |
There was a problem hiding this comment.
Rule hidden-dependency violated:
MappedRing trusts the client to apply the required seals without checking the received descriptor. A peer that has opened a session can therefore hand over an unsealed memfd, wait for the acknowledgement, and truncate or punch a hole in it while the policy reads the mapping, potentially delivering SIGBUS and terminating the server process. Check F_GET_SEALS for all required seals before mapping or acknowledging the descriptor, and reject ordinary or insufficiently sealed files.
AGENTS.md reference: AGENTS.md:L14-L22
Useful? React with 👍 / 👎.
| except OSError as refused: | ||
| logger.warning('No frame ring: this host refuses memfd_create (%s)', refused) |
There was a problem hiding this comment.
Rule swallowed-error violated:
_ring_is_supported() still converts memfd creation and sealing failures into the slower fallback while logging only at WARNING; fresh evidence beyond the earlier resolved thread is that the final implementation retains logger.warning on both paths even though this rule requires an intentionally swallowed optional-feature failure to be logged at ERROR. Raise both messages to error level while retaining the fallback.
AGENTS.md reference: AGENTS.md:L14-L22
Useful? React with 👍 / 👎.
… ring A server on a Unix socket runs on the same host as its client, so the images do not have to travel in the message. The server declares `frame_ring` in its ready handshake with the session's id. A `unix://` client then creates a ring in a sealed `memfd`, hands the descriptor over an `AF_UNIX` socket beside the session socket, writes each image into a slot, and sends a reference in its place. The server maps the ring read-only and builds a numpy view over the bytes. The seals — `F_SEAL_SHRINK`, `F_SEAL_GROW` and `F_SEAL_FUTURE_WRITE` — refuse every write, resize and hole punch the server could make, so a server that runs untrusted code reads the frames and cannot change them. A ring holds four slots, and each slot carries a sequence number before its payload and one after it, so a reader that meets another write refuses that observation. A server that declares no ring, a client over TCP, and a server that JPEG-encodes its images all keep the message path they have today. Ticket: Positronic-Robotics/internal#1247 #open
Each definition sits with the code that reads it: the seals with `FrameRing`, the observation walk and the handover timeout with `FrameWriter`. The benchmark reads the handshake key through `offboard_keys`. `FrameWriter` takes the slot count `FrameRing` already defaults to. Three comments state what their own code holds rather than what another component does. Ticket: Positronic-Robotics/internal#1247 #open
The grip channel and an action's schedule slot are names the rig and the server agree on, so a test and the benchmark read them from the one constant rather than spelling them again. Ticket: Positronic-Robotics/internal#1247 #open
Each docstring states three sentences, each comment one line, and the flag's help points at the contract rather than restating it. Ticket: Positronic-Robotics/internal#1247 #open
The suffix rule and the slot count are what another implementation must know, so the README states them and the module names only the local constraint. Ticket: Positronic-Robotics/internal#1247 #open
The module's prose keeps what a reader of the code needs and drops what the README says about the wire: the sequence numbers, the writable-then-sealed order, the handover order, the read-only view. Ticket: Positronic-Robotics/internal#1247 #open
…ides A handover that fails leaves the ring it was made for open, so `_ring_for` closes it and re-raises. The predicate's docstring drops the sentence about what its callers do. Ticket: Positronic-Robotics/internal#1247 #open
Closing the listening socket does not interrupt a thread already blocked in `accept`, so `close` knocked on nothing, waited out its join and left the thread holding the socket. It now dials the socket itself, and the loop returns as soon as it sees that the channel is closing. Ticket: Positronic-Robotics/internal#1247 #open
`SUPPORTED` said a ring was available wherever `memfd_create` exists, so Linux before 5.1 declared one and failed at the first inference on the seal it has not. It now creates a memfd and seals it, once, and reports what happened. A server whose session socket path plus `.frames` is longer than a Unix address declares no ring either, and says so. Ticket: Positronic-Robotics/internal#1247 #open
The server binds every Unix socket it serves, so it claims the handover path the same way and hands the descriptor to the channel. `claim_socket_path` takes the socket type, because a probe of another type reads a stale SEQPACKET path as live. Ticket: Positronic-Robotics/internal#1247 #open
A session that grows its frames kept every mapping it had ever been handed. It keeps the newest: a view already built holds its own mapping alive, which is what the growth test asserts. An accepted handover now has a deadline, so a peer that sends no descriptor cannot hold the one accept thread. The support probe covers the `memfd_create` a seccomp policy may refuse, and the companion path is measured in the bytes the filesystem takes. Ticket: Positronic-Robotics/internal#1247 #open
A host that refuses `memfd_create` or the seals now logs which of the two it refused, so the slower path is a line in the log rather than silence. A ring whose truncate, map or seal raises closes its descriptor before the error leaves the constructor. Ticket: Positronic-Robotics/internal#1247 #open
The probe answers whether this host supports a ring, so its name says that. `close` joins for the deadline a handover holds the thread for, which is longer than the five seconds it waited. Ticket: Positronic-Robotics/internal#1247 #open
d3ef45a to
3d25949
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d2594996d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| slot carries a sequence number before its payload and one after it; a reader that finds either one different | ||
| from the reference refuses that observation rather than serving other pixels under it. | ||
| - **A larger frame grows the ring.** The client creates a bigger one and hands it over before it sends any | ||
| reference to it. The server keeps every mapping it was handed, so a view it built earlier stays readable. |
There was a problem hiding this comment.
Rule stale-doc violated:
The frame-ring contract says the server keeps every mapping, but FrameChannel._take_ring now retains only the newest MappedRing; an older mapping survives only when a NumPy view still references its mmap. Update this bullet to describe that ownership model so readers do not expect the channel itself to retain every historical mapping.
AGENTS.md reference: AGENTS.md:L14-L22
Useful? React with 👍 / 👎.
1 participant
A server on a Unix socket runs on the same host as its client, so its frames do not have to travel in
the message. Three raw hd720 frames are 8.3 MB, and the
websocketslarge-message path costshundreds of milliseconds for one such message.
The server declares
frame_ringin its ready handshake, with the session's id. Aunix://clientthen creates a ring in a sealed
memfd, hands the descriptor over, writes each image into a slot,and sends a reference in its place. The server maps the ring read-only and builds a numpy view. A
server that declares no ring, and every client over TCP, keep the message path.
Stacked on #735. Its commits show in this diff until it merges.
The descriptor
The handover socket is an
AF_UNIXSOCK_SEQPACKETsocket. Its path is the session socket's pathplus
.frames. Each side builds that path from the socket path it already holds, so a bind mountthat gives the two processes different names for one directory still opens one socket.
The server binds every Unix socket it serves, so
claim_socket_pathclaims this one too and handsthe socket to the channel. That call takes the socket type, because a probe of another type reads a
stale
SOCK_SEQPACKETpath as live. The client sends the descriptor withSCM_RIGHTS, names thesession id, and waits for the server to map the ring, so no reference reaches the wire first. An
accepted handover has a deadline, so a peer that sends no descriptor cannot hold the accept thread.
The seals
The client maps the ring writable, applies
F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_FUTURE_WRITE, andonly then hands the descriptor over. The server cannot map it writable, write it, resize it, or punch
a hole in it. A test asserts the four refusals.
F_SEAL_WRITEis not usable here, because it refuseswhile the writer holds its own mapping.
SUPPORTEDcreates a memfd and applies the same seals, once, at import. A host that refuses eitherstep declares no ring and logs which step it refused; macOS and Linux before 5.1 are such hosts. A
server also declares no ring where the companion path is longer in bytes than
sockaddr_uncarries.The slots
A ring holds four slots. One round trip is in flight at a time, so the writer returns to a slot four
inferences later. Each slot carries a sequence number before its payload and one after it, and a
reader that finds either number different from the reference raises.
A larger frame grows the ring. The client hands a new ring over before it sends any reference to it,
and the channel keeps one ring per session. A view already built keeps its own mapping alive, so
nothing unmaps under it.
Numbers
posi-vm, 3 frames of 720x1280x3 (8.3 MB), a stub server that reads every pixel, 50 calls, through
positronic/offboard/tests/bench_frame_ring.py:For scale on the same box, msgpack packs those 8.3 MB in 1.0 ms and a raw Unix-socket write plus read
is 2.2 ms. The rig measured 200 to 224 ms on the message path. The rig is the target, and it has
measured no ring yet.
Untested
main, so the green rollup on thisstacked pull request is two skipped jobs. The macOS skip is unverified.
Open for the owner
Two Codex findings are declined and their threads stay open. One asks for
Pathin place ofstrfor the socket paths; the package types them
strfrom the URL touvicorn, so this module alonewould add a seam. One asks to narrow the accept loop's exception boundary; a failed handover closes
the connection, so the client raises at once.
Ticket: Positronic-Robotics/internal#1247. Design: Positronic-Robotics/internal#1242.