Skip to content

feat(node-cxx): memory-pool transport with a CUDA-IPC-free path for integrated GPUs - #3106

Open
caothu2k1 wants to merge 34 commits into
dora-rs:mainfrom
caothu2k1:feat/cxx-memory-pool-2686
Open

feat(node-cxx): memory-pool transport with a CUDA-IPC-free path for integrated GPUs#3106
caothu2k1 wants to merge 34 commits into
dora-rs:mainfrom
caothu2k1:feat/cxx-memory-pool-2686

Conversation

@caothu2k1

Copy link
Copy Markdown
Contributor

Closes the remaining half of #2686. The Service/Action half landed in #2853.

The problem

apis/python/node/src/lib.rs:2308 fails registration when a CUDA receiver's IPC
export fails:

if receiver_is_cuda && !ipc_written {
    eyre::bail!("register_memory_pool: failed to set up GPU pool buffer / IPC handle …");
}

On an integrated GPU cudaIpcGetMemHandle is unsupported, so that branch always
fires. Today no language binding can use a CUDA receiver on such a device
the memory-pool transport is unavailable there entirely, not merely slow.

What this adds

A third transport, unified. Full shared-memory data region, ipc_flag = 0;
the receiver takes a device alias of the same pages with
cudaHostRegister(..., cudaHostRegisterMapped) + cudaHostGetDevicePointer. It
needs no protocol change because it reuses the existing segment layout, and it
works on every platform — on a discrete GPU it is merely slower than ipc.
auto resolves to unified when the receiver is CUDA and shmem otherwise, so
it can never resolve to a mode that fails at registration.
DORA_MEMORY_POOL_TRANSPORT overrides auto and nothing else.

The DORADMA data plane gains a definition in dora-memory-pool. The header
layout, the seqlock and the segment lifecycle previously existed only as
hand-written byte offsets inside the Python binding. This adds a tested
implementation the C++ binding builds on — it does not consolidate: the
Python binding still carries its own hand-written copy, and making it the single
definition is follow-up work.

The C++ binding gains the pool surface: register_memory_pool,
write_memory_pool, read_memory_pool, free_memory_pool, plus the pointers a
producer needs to fill a pool in place instead of paying a second copy.

Two optional header-only C++ helpers. dora/memory_pool.hpp provides
PoolWriteGuard and PoolReadGuard; dora/cuda_pool.hpp provides the mapping
helper. No Rust crate links CUDA — the language side owns it, matching how the
Python binding already works.

Design notes a reviewer may want

Invariants live in the types, not in doc comments. FFI is where Rust's
enforcement stops, so several review rounds moved guarantees across that line:

  • Payload access is pool_payload(&mut ptr, &mut len) -> bool, an out-param
    predicate returning false for an ipc pool. An earlier shape exported a
    pointer and a length that did not bound each other: on an ipc pool the
    offset points one byte past the mapping while the declared size reports
    megabytes, so memcpy(host_ptr(), src, size()) — the natural C++ line — wrote
    megabytes out of bounds. There is no declared-size accessor on the C++
    surface at all.
  • No generation token crosses the bridge. It lives in the segment, so a
    fabricated value cannot publish a torn frame as complete.
  • begin_read returns an opaque OpeningSample that is deliberately not
    PartialEq, and the closing comparison lives inside read_completed. Closing
    a read with a second opening sample loads the same value with the wrong fence,
    so it passes every test while admitting torn frames on a weakly-ordered CPU —
    the type system is the only thing that can catch it.
  • PoolWriteGuard is the documented write path. A caller that returns early or
    throws between begin_write and end_write leaves the generation odd
    permanently and the pool unreadable to every consumer; there is no Drop on
    the Rust side to recover it.
  • dtype and shape are advisory metadata, never a bound. An unrecognized
    dtype is assumed one byte, so a shape-derived product under-estimates.
    payload_len is the only bound.

Seqlock ordering. A reader needs two different edges — gen load → payload loads when opening, payload loads → gen load when closing. An acquire load
gives only the first. read_completed therefore pairs an acquire fence before
a relaxed load. Accesses go through AtomicU64::from_ptr rather than
read_volatile plus a bystander fence, so the code is correct under Rust's
memory model rather than only under inspection of the emitted barriers; the
shared-memory representation is unchanged.

One deliberate divergence to flag, since the extracted code is meant to match
what it was extracted from: the writer fences before the generation store,
while seqlock_end in the Python binding (apis/python/node/src/lib.rs:1070)
has its only fence(Release) after it. The pre-store fence is the one that
orders the payload writes ahead of the publish, so without it a weakly-ordered
target can publish an even generation whose payload is not yet visible. The
Rust version is stronger on purpose. The Python path is untouched here — this
change does not modify that binding — but it is worth a separate look.

Verification

Unit — 85 tests in dora-memory-pool, 53 in dora-node-api-cxx. Every
DORADMA constant is pinned against the Python binding's own values. Each check
was mutation-tested: the guard was deleted, the targeted test confirmed red for
the right reason, the guard restored. Where a mutation could not be killed it is
recorded in a comment rather than left implied.

In CIexamples/c++-memory-pool runs the whole path over a live daemon on
the CPU: a 16-slot ring filled in place through the guard, both read paths, an
abandoned write cycle rejected and then recovered, and a freed pool going
permanently unreadable while a second pool still reads. Registered in
tests/example-smoke.rs, scripts/smoke-all.sh and the nightly examples job.
The runner fails if any dora_pool_* segment survives.

Its run.rs passes -l dora_node_api_cxx before the system libraries, unlike
the neighbouring C++ example. That order is required: GNU ld resolves an archive
only against what follows it, so the other order leaves dlopen/dlsym
undefined on any glibc older than 2.34. It happens to link on ubuntu-latest
because libdl is folded into libc there.

On device — a Jetson (JetPack 5, CUDA 11.4), integrated GPU:

[cuda-receiver] transport=unified ipc_present=false integrated_gpu=true
[cuda-receiver] registered 16896 bytes of `frames`: host=0xffff… device=0x2031…, payload at +512
[cuda-receiver] 21 frames verified through the device alias, 1 torn, 1 unavailable

A kernel reads bytes the host wrote, through the device alias, with no
intervening copy — the only two cudaMemcpy calls move a 12-byte verdict, never
the payload. The mapping is registered once at startup, before any frame exists,
and 21 frames of changing content then arrive through that same never-refreshed
alias, so it cannot be a snapshot taken at registration time. Substituting the
host pointer for the device alias faults rather than passing, because the two
are genuinely different addresses on this hardware.

Cross-language — a pool registered by the untouched Python binding is
read by a C++ node, on the CPU path and from a kernel through the device alias.
Note the precise claim: the two are not byte-identical. A Python CPU pool
writes no transport key, writes pinned_type: "cpu" where a C++ unified
pool writes "cuda", uses json.dumps spacing so json_len differs, and lands
at write_gen = 2 where create leaves 0. They interoperate because the
consumer reads every one of those fields out of the header instead of
assuming them — which is the real argument, and a better one.

Not in this PR

  • The Python binding is unchanged. A Python sender with
    receiver_device="cuda" still fails on an integrated GPU. Follow-up: teach
    its auto-select about unified, and move its data plane onto the shared
    crate.
  • The C++ binding does not produce ipc pools — that would require CUDA
    inside the binding. It reads one and hands out the handle.
  • The pool's daemon parameters are still hand-mirrored.
    MemoryPoolMetadata lives in dora-memory-pool, but its MetadataParameters
    conversion is private to dora-daemon, so the binding transcribes nine keys
    and pins them with a test against a copy of the daemon's reader. Moving that
    conversion next to the type would delete the transcription at the cost of a
    dora-message dependency.
  • The CUDA example and the Python-interop CUDA dataflow are manual, hardware-
    gated artifacts; only the CPU paths run in CI.

🤖 Generated with Claude Code

caothu2k1 and others added 30 commits August 6, 2026 11:26
The layout lived only in the Python binding as hand-written offsets. Move it
into dora-memory-pool so a second language binding cannot drift from it, with
a test that pins every constant to what Python writes today.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n's CUDA-pool shape

A Python CUDA-receiver pool allocates a header-only segment (total_size =
data_offset) with ipc_flag=1 and a JSON `size` naming the full tensor byte
count that actually lives in the IPC-imported GPU buffer, not in this
segment. Defaulting an absent `transport` key to "shmem" mislabeled every
such pool, and validating `size` against the segment unconditionally would
reject every one of them once a later gate checks the value.

- transport: parse_metadata_json leaves it empty when absent (it can't see
  ipc_flag); parse_header resolves it to "ipc"/"shmem" from ipc_flag.
- size: the data-region bounds check moves into parse_header, gated on
  !ipc_present, mirroring the Python read path's own gate and saturating
  arithmetic.
- pinned_type absent now defaults to "cuda" (mirrors Python's
  effective_as_cuda polarity, not "cpu"); present-but-wrong-type is an
  error, not a silent default, for both pinned_type and shape.
- shape rejects any non-integer element instead of silently dropping it
  (filter_map turned [2,"x",3] into a plausible-looking [2,3]).
- data_offset < HEADER_SIZE is now rejected explicitly instead of
  surfacing as a confusing utf-8/EOF error further down.
- write_header zeroes the reserved range and JSON padding instead of
  assuming buf arrives pre-zeroed.
- ParsedHeader.ipc_handle is now Option<[u8; 64]>, Some only when
  ipc_present, so a zeroed non-IPC handle can't be mistaken for a real one.
- data_offset_for uses saturating arithmetic against a hostile json_len.

Adds coverage for every degradation/rejection path above, plus a
byte-level assertion on write_header's output (the existing constants
test only pins offsets against literals in the same file, so an offset
typo could round-trip through write_header/parse_header undetected).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…transport

The transport/ipc_flag resolution only filled in an *absent* transport key
from ipc_flag; a *present* key was trusted verbatim. ipc_flag and transport
are both attacker-controlled bytes in the same world-writable segment, and
ipc_flag=1 skips the payload-size bounds check — so a present
transport:"shmem" paired with ipc_flag=1 could describe an 80 MB payload
over a 512-byte header-only mapping and still parse Ok, which is exactly
the guarantee the size check exists to give. Now a present transport must
agree with ipc_flag (transport == "ipc" iff ipc_flag == 1) or parse_header
rejects the header; "unified" with ipc_flag=0 is not a contradiction and
keeps its normal size check.

Also: tightened parse_rejects_data_offset_before_the_header_ends to assert
the specific "less than HEADER_SIZE" message — the previous assertion
matched a substring ("data_offset") that the json_len-fit-check's message
also contains, so the test passed against both the fixed and the unfixed
implementation. And extended the write_header byte-level test to a
pre-poisoned (0xEE) buffer covering the ipc_flag and IPC-handle byte
ranges, so a deleted zeroing call or an ipc-field offset drift shows up as
a non-zero byte instead of being indistinguishable from an already-zeroed
Vec.

Every new/changed check was mutation-tested by hand: temporarily deleted
the transport/ipc_flag contradiction check (both directions went red), the
data_offset < HEADER_SIZE check (the tightened test went red), and each of
the ipc_flag/ipc_handle/reserved/padding zeroing lines in write_header
(each went red under the extended byte-level test) — then restored.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same protocol the Python binding writes inline at header offset 96, including
recovery from an odd generation left by a writer that died mid-write.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…emantics

Code review caught two real defects in the seqlock primitives:

- read_gen used a single acquire load/fence, which only gives the
  gen-load -> payload-loads edge. Readers need a second, opposite edge for
  the closing sample (payload-loads -> gen-load), which an acquire load
  cannot provide since it orders what follows it, not what precedes it.
  Split into begin_read (acquire load) and end_read (acquire fence before
  a relaxed load) so each function documents and provides the edge it
  actually needs.

- end_write's failure path stored an even generation ("payload complete")
  over a payload an in-place write had already destroyed. There is no
  previous frame to roll back to, so a failed write must leave the
  generation odd, not publish a false "complete" over garbage.

Also moves off read_volatile/write_volatile + bystander fences onto
AtomicU64::from_ptr, since a fence has no synchronizes-with relation
without an atomic operation to attach to. begin_write's release fence is
now unconditional so the odd-generation recovery path is covered too.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reader-edge fix replaced `read_gen_sees_the_current_value` with tests that
only check parity and inequality, dropping the one assertion that a read
function returns what is actually at the location. That coverage matters as
soon as a caller computes the pointer itself: `PoolSegment` will reach the
counter through `shmem.as_ptr().add(96)`, where an offset or cast mistake is
the most likely error and a value-pinning assertion is what catches it.

Assert exact generations on both samples instead. Also correct the comment on
`begin_write`'s fence: on the recovery path it writes nothing, so the edge it
provides is against the load, not against a store.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lace

A name that misses the dora_pool_ prefix is one the daemon will never unlink,
so the segment leaks in /dev/shm for the life of the machine.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…id charset to match NodeId

Measured directly with shm_open (glibc 2.31, aarch64): 253 bytes succeeds, 254
and 255 both fail EINVAL. The 255 cap silently let 254-255 byte names through
to shm_open, trading the clear error this check exists to produce for a bare
EINVAL. Renamed off NAME_MAX since it was never that libc constant.

NodeId (libraries/message/src/id.rs) permits dots everywhere except a leading
position, so a node id like camera.left parsed and spawned fine and then
failed at pool registration with an error the user could not act on. Widened
the charset to match NodeId's, and added an explicit `..` rejection since
NodeId only bans a leading dot -- once `.` is allowed, `..` is no longer
unreachable by construction, and the daemon's own contains("..") guard in
free_shared_memory must stay satisfiable.

Also wired free_shared_memory and cleanup_orphans in lib.rs off the
SEGMENT_PREFIX constant instead of a repeated "dora_pool_" literal, and added
boundary tests pinned to the constant (mutating > to >= turns the
exact-limit case red).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Widening the charset to accept `.` made this test stop covering the `..` rule:
every case also contains `/`, which the charset rejects on its own, so deleting
the `..` check leaves it green. Rename it to what it does cover and point at
the test that guards the rule it no longer does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Create/open a DORADMA segment, expose the page-aligned base a CUDA receiver
must register plus the 256-byte-aligned payload start, and guard writes with
the seqlock. Adds the `unified` transport: full data region with ipc_flag 0,
which is the only shape that works on an integrated GPU where
cudaIpcGetMemHandle is unsupported.

Departures from the drafted design, all forced by the committed modules or by
`-D warnings`:

- `ParsedHeader::ipc_handle` is an `Option`, so `ipc_handle()` forwards it
  rather than rebuilding it from `ipc_present`.
- `Transport`'s string decode is a `FromStr` impl; an inherent `from_str`
  trips clippy::should_implement_trait.
- `PoolSegment` gets a hand-written `Debug` (`Shmem` has none), without the
  mapping address, which is meaningless in another process.
- `write()` refuses an ipc-backed pool. `parse_header` deliberately skips the
  payload-size check when ipc_flag is set, because a Python CUDA pool is a
  header-only segment declaring the size of a tensor in device memory — so on
  such a segment `size` does not bound the mapping and a write bounded by it
  runs off the end. Covered by a test that segfaults without the guard.
- `create()` unlinks on the header-write error path: `set_owner(false)` means
  dropping the handle would strand a segment that O_EXCL can never reuse.
- Tests are Linux-only (macOS caps shm names at 31 bytes; neither macOS nor
  Windows has the /dev/shm path unlink removes) and clean up from `Drop`, so
  an assertion failure cannot poison later runs with a stale name.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Quality review found the same shape of defect eight times: an invariant this
crate enforces internally but only *documents* at the edge a C++ node will
call across. Enforcement has to survive the FFI boundary, so it moves into
types and return values before Task 5 freezes the surface.

Payload access. `size()` read like a bound and was not one: on an
IPC-backed pool `data_offset == segment_bytes`, so the payload pointer was
one past the end while the size was multi-megabyte, and `memcpy(ptr, src,
size)` is the natural line on the other side. Renamed to `declared_size()`,
and the only way to a payload pointer is now `payload() -> Option<(u64,
usize)>`, which returns `None` for `Ipc` and otherwise guarantees
`ptr + len <= shm_base() + segment_bytes()`. Chose the `Option` over a bare
`payload_len()` accessor because it makes the guard unskippable rather than
merely available; `payload_len()` remains for callers that want the number,
and cannot produce a pointer on its own.

The write cycle. `begin_write` refuses an IPC pool exactly as `write` did —
the bracket form is what a zero-copy node uses, so guarding only `write`
guarded the path nobody takes. It no longer hands the caller a generation
token either: across the bridge one wrong integer would publish an even
generation over a frame still being overwritten, the single failure a
seqlock cannot detect afterwards. The baseline lives in `pending_write`, so
`begin_write() -> Result<(), String>` also catches a doubled call and
`end_write(ok)` is a no-op without one, both of which were previously
invisible. `write` now requires `data.len() == payload_len()`: a partial
write left the previous frame's tail in place and published it as part of
the new one. Variable-length payloads go through `payload()` under the
bracket and carry their length in the caller's own message; adding a
per-frame length field to the wire format was considered and declined.

Shape validation. `shape` and `dtype` were exported unvalidated, so a
producer declaring `size: 64, shape: [4096, 4096]` handed a consumer a 16 MB
view of a 64-byte buffer while passing every other check. `open` and
`create` now check `product(shape) * itemsize <= declared_size`, with the
element size taken from the vocabularies dora senders actually write (numpy
names, torch's `str(tensor.dtype)`, numpy typestr). An unrecognized dtype is
not rejected — a sender may legitimately use one we have not enumerated —
but keeps the one-byte-per-element lower bound, which is what catches the
case above. The bound is `<=`, not `==`, against the review's suggestion:
only an over-large shape is a safety problem, an over-allocated pool is what
a producer that page-aligns its size writes, and Python's own reader accepts
it (`dora/cuda.py` rejects only on `expected_bytes > size`) — so requiring
equality would cost interop and buy no safety.

Seqlock. The reader's closing load is no longer something a caller can get
wrong: `begin_read` returns an opaque `OpeningSample` (not `PartialEq`, so
comparing two by hand does not compile) and the acquire-fence load plus both
acceptance rules live in `seqlock::read_completed`. `end_read` is gone. Last
round this was guarded by a comment because no test could catch the swap; it
is now `error[E0369]`.

Also: `parse_header` takes the segment length as an argument so `open` can
hand it a slice bounded to the write-once header region — a `&[u8]` over the
payload aliases a peer's concurrent writes, which is UB whether or not
anything reads it. Every unlink goes through one guarded `naming::
unlink_segment`, so the daemon's prefix/`/`/`..` check cannot be applied in
one place and forgotten in the other. `pinned_type()` is derived from the
transport instead of read back from the unchecked JSON field. The size rule
is a pure predicate, tested without `ftruncate`ing a real gigabyte.

`PoolSegment::unlink` is removed rather than made `pub(crate)`: nothing
outside this crate needs it, and a `pub(crate)` method used only from tests
is `error: method is never used` under `-D warnings`.

Correcting last commit's claim: the unlink on `create`'s header-write error
path is unreachable today — `write_header` cannot fail once the segment
covers the header, and `parse_header` cannot reject bytes `write_header`
just produced. It is hardening against a silent, permanent failure, not a
fix for a live leak.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…apping

Four follow-ups from re-review, all narrowing what the bridge can reach.

The header-region bound was not one. `open` sized its slice with
`HEADER_SIZE + json_len` clamped to the segment length, but `json_len` is
unvalidated at that point: a corrupt value overshoots and the clamp lands on
the whole mapping, payload included — reinstating the very reference the
split was meant to prevent. A merely-large value needs no overflow at all:
json_len 4096 against a data_offset of 256 in an 8 KiB segment passes any
segment-length test and still spans the payload.

The only correct cap is `data_offset`. `doradma::header_region_len` replaces
`json_len_field`, reading both fields and validating
`HEADER_SIZE + json_len <= data_offset <= segment_len` before returning a
length that provably cannot reach the payload; `parse_header` re-checks all
of it as the backstop. Every well-formed segment satisfies the chain by
construction (`write_header` derives `data_offset` from `json_len`), pinned
by a test over a matrix of payload and shape sizes, so nothing legitimate is
newly rejected. Its rejections are worded distinctly from `parse_header`'s
equivalents so a test can tell which of the two fired — otherwise the
assertion would pass on either. A `debug_assert!` at the call site checks the
promise against the parsed `data_offset` rather than the raw one. This also
turns the previously untestable `.min(segment_bytes)` mutation into an
observable `Err`.

`data_offset()` becomes `payload_offset() -> Option<usize>` rather than being
deleted. The CUDA path needs it — register the mapping, take the device alias,
then add the offset to reach the payload on the device — but it has no caller
until that task lands, and as a bare `usize` it reproduced the hazard
`payload()` closes. As an `Option`, `shm_base() + payload_offset()` cannot be
formed on an IPC pool without unwrapping a `None`.

`seqlock` is now `pub(crate)`: its writer half publishes a generation from a
raw pointer and a caller-supplied token, which is exactly what `begin_write`
was changed to make unreachable — leaving the module public left the same
hole open one `unsafe` block away. `segment` re-exports `OpeningSample`, the
only part appearing in a public signature. Verified from outside the crate:
`seqlock` is `E0603`, `segment::OpeningSample` resolves.

Finally, the two contracts a seqlock rests on but cannot check now sit on
`begin_write`, where the bridge will see them, instead of only in the module
docs: one writer per pool process-wide (`pending_write` is per-handle, so two
handles both take the same baseline and both publish over an interleaved
payload, indistinguishable from one clean write), and `PoolSegment` is `Send`
but not `Sync`.

Also adds `float128`/`complex256` to the element-size table; their typestr
forms already parsed, only the name forms fell to the one-byte floor.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
register_memory_pool / write_memory_pool / free_memory_pool, plus the
pointers a node needs to fill the pool in place instead of paying a second
copy. The mapping base is exposed separately from the payload start because
cudaHostRegister needs a page-aligned address and the payload is only
256-byte aligned.

Three of the invariants segment.rs moved into types could be undone at this
boundary, so they are carried across it rather than documented:

- the payload pointer is an out-param predicate, and the only length that
  bounds it is pool_payload_len. pool_declared_size / pool_dtype /
  pool_shape are labelled advisory: on an ipc pool the declared size
  describes device memory that is not in the mapping, and an unrecognized
  dtype makes a shape-derived product an under-estimate.
- no generation token crosses the bridge. pool_begin_write returns a
  DoraResult and pool_end_write takes only `ok`.
- dora/memory_pool.hpp ships a header-only PoolWriteGuard, because an early
  return or a throw between the bare calls leaves the generation odd
  permanently and no Drop on the Rust side recovers it.

A failed registration unlinks the segment it created: the daemon has no
record of it, so nothing else ever would, and the name is O_EXCL.
free_memory_pool calls only the daemon — the daemon's unlink is what also
notifies every node that touched the pool.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pool_declared_size carried no information on the producer side —
PoolSegment::create allocates exactly the declared size, so it equals
pool_payload_len for every pool this binding can mint. Its only distinct
value is on a pool created elsewhere, where it describes device memory
outside the mapping; there pool_shm_base + pool_declared_size is a
pointer/length pair that does not bound itself, reachable in two noexcept
calls with no Option to unwrap. Removing it now closes that structurally
rather than by documentation. PoolSegment::declared_size stays; it is
legitimate in Rust and simply does not cross the boundary.

PoolWriteGuard nulls data() and zeroes size() when the cycle closes, in
commit() and in the destructor alike. A write through a pointer kept past
the cycle lands in the payload with no generation bump around it, so every
reader publishes it as a complete frame; a null dereference stops there.
operator new is deleted, because a heap guard outlives the scope that owns
the pool.

apis/c++/node/tests/memory_pool_compile.cpp gives the header compile
coverage inside the repo — static_asserts for the type properties plus an
early return out of a live cycle — driven by a test that shells out to the
C++ compiler with the headers build.rs just installed. Nothing else here
compiles that header: cxx_build::bridges runs codegen only.

On the Rust side, a failed registration off Linux no longer appends
"additionally failed to remove the orphaned segment": unlink_segment
reports the platform there, not the segment. The nine daemon parameter
keys are pinned by a test against a transcription of the daemon's own
reader — pool_metadata_from_params is private to dora-daemon, so a typo
was otherwise a runtime-only "missing shared_memory_name".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
read_memory_pool maps a pool by the segment name the daemon reports — never
one guessed from the pool id, which does not determine it — and returns a view
whose byte-reaching accessors are all predicates. `view_payload` and
`view_mapping` are out-param pairs that fail rather than hand out half of a
pointer/length pair, and the declared size is not exposed at all: a view, unlike
a pool this node created, can be `ipc`, where the declared size describes a
device buffer megabytes larger than the whole mapping.

Two read shapes, because a copying reader and a zero-copy reader need different
things. `view_try_read` brackets the seqlock, the copy and the re-check inside
Rust, so there is no token to mishandle; `view_begin_read` hands back an opaque
`DoraPoolRead` for a reader that consumes the frame in place. The token is a
`rust::Box` of an opaque type rather than a `uint64_t` — a plain integer could
be forged, compared with the wrong fence, or held across frames — and it carries
the serial of the view that issued it, because two views of one segment share a
generation word and a swapped token would otherwise validate a bracket that
never enclosed the read.

Views register a liveness flag, and take_freed_pools flips it for every view of
every pool named in a daemon FreeMemoryPool notification before returning the
ids. That ordering is what makes the ids actionable: the node can
cudaHostUnregister the mappings it registered while every accessor already
refuses to produce a fresh address, which Rust cannot do on its behalf, having
no idea which segments were handed to the driver.

dora/memory_pool.hpp gains dora::try_read_pool, which sizes the destination
from the pool so that a false return can only mean the retryable torn-frame
case, and dora::PoolReadGuard, which keeps the token, the pointer and the
length together for the zero-copy path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mark_freed took a slice and returned nothing, so take_freed_pools marked and
then returned the ids as two separate steps — an ordering a later edit could
reverse or drop without anything noticing, and the one ordering that matters:
the ids are only actionable because every view of those pools already refuses
to hand out an address by the time C++ sees them.

Consuming and returning the Vec makes that unskippable. take_freed_pools is now
a single expression whose only route to the ids is through the marking, and the
rule has somewhere a test can reach it, which the real path does not — nothing
in this crate can push a notification into the event stream's pending set.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ceeds

try_read_pool returned a bool whose false conflated three cases, two of which
never become true. `while (!try_read_pool(view, buf)) {}` — the obvious loop —
spins forever on an ipc view, whose payload is in device memory and will never
arrive in this mapping, and on a freed one. The Rust-side doc on view_try_read
drew the distinction; the C++ helper that is the documented preferred path did
not, which is where it bites. PoolReadGuard already got this right by throwing.

It now returns PoolReadOutcome::{Copied,Torn,Unavailable}. An enum class rather
than a documented convention: it has no conversion to bool, so the spinning
loop is ill-formed instead of merely wrong, and the compile check pins that with
a negation probe that first proves itself against bool. try_read_pool also asks
for the length before resizing, so an unavailable pool no longer empties a
vector the caller may still hold the last good frame in.

Corrects the record from the previous commit. Consuming and returning the Vec in
mark_freed does not make the marking unskippable and the ids' route through it
is not the only one: drain_freed_pools returns the same Vec<String>, so dropping
mark_freed out of the composition compiles clean and passed all 51 tests. It is
an ordering and readability improvement, not enforcement, and saying otherwise
is worse than saying nothing — the next person to touch it would trust it. The
gap is now closed by take_freed_pools_from, a local seam taking the drain as a
parameter, which a test drives with the notification only the event thread can
otherwise produce. push_freed_pool stays pub(crate) in dora-node-api: it is an
event-thread internal, and publishing it would let any node forge a free.

Also: view_shape/view_dtype warned only that a shape-derived product
under-estimates, which is backwards on the case that matters — an ipc view's
shape describes device memory absent from this mapping and overshoots the
segment by megabytes; view_ipc_present now honours the dead flag as
view_ipc_handle already did, so a freed pool cannot report a handle as present
while refusing to produce it; view_mapping documents that the ordinary
"done with this view" path needs cudaHostUnregister too, not just the
take_freed_pools path; take_freed_pools documents the thread it must be called
on, the one ordering the liveness flag cannot cover; both guards delete
placement new, which deleting the plain operator new had left open;
shared_memory_extended moves to a workspace dependency instead of being
hand-pinned in two manifests with a comment asking for manual sync.

Declined: collapsing the 32 per-function `#[allow(clippy::borrowed_box)]`
attributes into one module-scoped allow. Most predate this task, cxx resolves
bridge functions relative to the bridge's parent module so the move is not
mechanical, and the per-function comment naming cxx as the reason is the
convention the rest of the file follows.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…probe

The previous commit added `operator new(size_t, void*) = delete` to both pool
guards on the review's report that deleting the plain form left placement new
available. It does not: a class-scope `operator new` hides every global form, so
`new (buffer) Guard(...)` already found only the single-argument signature and
failed to match it. Verified against g++ on a two-struct probe — a class with no
`operator new` is placement-constructible, one with only the plain form deleted
is not.

The deletion was therefore a no-op, and worse, the static_asserts guarding it
could not fail: both the before and after states are ill-formed, so mutating the
line away left the compile check green. That is the same non-discriminating
assertion this task has been repeatedly caught by.

The declaration is gone and the comment now states the lookup rule that makes
one deletion enough. The placement probes stay, because they do pin something
real — a guard that declares no `operator new` at all, which is what forgetting
the deletion actually looks like — and they now come with a `PlacementProbe`
struct that proves the probe can return true, so a probe that silently stopped
detecting anything cannot pass. Removing the declaration from PoolReadGuard now
fires both its heap and its placement assertion.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cudaHostRegister(Mapped) + cudaHostGetDevicePointer over a pool segment,
which is the zero-copy route on an integrated GPU where CUDA IPC does not
exist. Header-only and not pulled in by dora-node-api.h, so neither the
Rust build nor a plain C++ node acquires a CUDA dependency.

Registers from the segment's mapping base (pool_shm_base/view_mapping),
not the payload start, because cudaHostRegister needs a page-aligned
address and the payload only starts 256-byte aligned. unmap_pool is safe
to call twice and on a never-mapped MappedPool; callers must unmap before
a segment is unlinked, which is what take_freed_pools is for.

Verified on this Jetson (JetPack 5, CUDA 11.4): is_integrated_gpu() reports
true, and a page-aligned host buffer registered with map_pool round-trips
a value through the device pointer via a real kernel launch.

Added cuda_pool_header_compiles alongside memory_pool_header_compiles for
compile-only coverage of the new header; it locates nvcc via NVCC/
CUDA_PATH/CUDA_HOME/PATH and skips (not fails) when none is found, so the
crate's test suite still passes with no CUDA toolchain present.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… and gate the CUDA-header skip

Review of the previous commit found four real defects, all in this
commit's footprint:

- The header's own doc example dropped pool_payload_offset's/
  view_payload_offset's return value, so a false (ipc pool, or a freed
  view) left offset at 0 and pointed a kernel at the DORADMA header
  instead of the payload — silent corruption, not a crash. Braced both
  examples and added the consumer-side one (view_mapping +
  view_payload_offset), since Task 9 has nothing else to copy from.

- unmap_pool's doc named take_freed_pools() as the only "before unlink"
  signal, but dropping the pool/view rust::Box munmaps the segment the
  same way an unlink does. Documented both release points, and recorded
  why this stays a plain function instead of an RAII guard: the correct
  release point is a take_freed_pools() tick, not a C++ scope exit.

- map_pool swallowed the cudaError_t behind a bool, and left the
  runtime's last-error slot poisoned on failure — a caller's next
  cudaGetLastError() (the standard post-kernel-launch check) would see
  map_pool's stale error instead of its own. Added an optional
  out_error out-param and clear the slot on every failure path.

- cuda_pool_header_compiles skips identically to a real pass under
  cargo test's default output capture, so silent loss of CUDA on a box
  that is supposed to have it would never go red. Added
  DORA_REQUIRE_CUDA to turn the skip into a failure on demand, without
  making the crate's tests require CUDA by default.

Minors: documented MappedPool as a handle rather than an owner (a copy
does not share the registration), documented that is_integrated_gpu's
false also covers a failed property query, noted the device_index vs.
current-device mismatch with map_pool, noted cudaHostRegister's cost is
per-pool/per-view and must not be paid per frame, and commented why
find_nvcc trusts an explicit NVCC without an is_file check.

Verified on this Jetson (JetPack 5, CUDA 11.4): the round trip still passes
with the new out_error signature, a deliberately failing map_pool
(double-registering a range) reports cudaErrorHostMemoryAlreadyMapped
through out_error while leaving cudaGetLastError() clean afterward, and
DORA_REQUIRE_CUDA correctly turns a forced no-nvcc skip into a failure.
The map_pool null/zero-guard mutation still cannot be killed on this
CUDA runtime, as before — cudaHostRegister already rejects null/zero on
its own.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two nodes in lockstep over one daemon-registered pool. The producer fills a
slot of a 16-slot ring in place through `dora::PoolWriteGuard` and publishes
only the index; the consumer maps the pool by id, gets the segment name from
the daemon, and checks every slot of the ring on every frame.

Six tasks of unit tests could not reach three things, because C++ can only
obtain a pool from a live daemon, so the example produces them deliberately:
an abandoned write guard (early return, no commit) that readers must reject
as `Torn` and that the next commit must recover from; a freed pool that reads
back `Unavailable` rather than retryably; and the whole register -> daemon ->
read path. A second, tiny pool covers `write_memory_pool`, which brackets its
own cycle and so cannot share a pool with a guard.

The CPU path needs no GPU, so it runs in the nightly examples job, Linux-gated
because the transport is a segment in /dev/shm. The runner fails the example
if any dora_pool_ segment survives the run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rn as fatal

Review follow-up on the C++ memory-pool example.

`tests/example-smoke.rs`'s audit table requires a row for every example
without a `smoke_*` test; the example had none, and nothing enumerates
`examples/`, so CI would not have caught the omission. Added, citing the
nightly step that covers it.

The two-pool rationale was wrong in four places. `write_memory_pool` really
cannot run inside a live `PoolWriteGuard`, but that is not what forces two
pools here — the guard is scoped to `fill_slot` and the banner is written
before the event loop. The real reasons are that a copy-path write to
`frames` must be the whole 16 KiB and would flatten the ring, and that
"freeing one pool leaves the other readable" needs a second live pool.
Stating the restriction as the cause reads as a stricter API rule than
exists, in a file users copy from.

`Torn` is retryable, and asserting it as fatal taught the opposite of what
`dora/memory_pool.hpp` documents. The frame path now retries once — in
lockstep it never fires — and the README says plainly that the assertions
are the harness, that a real consumer retries, and that the example
exercises no concurrency at all: the `Torn` it produces is an abandoned
cycle, not a race, and the seqlock's contended path is covered by the unit
tests, not here. Also lists which parts not to copy.

Smaller: the /dev/shm guard runs before the two-minute C++ build and names
`rm /dev/shm/dora_pool_*` as the remedy; `ring_mismatch` returns its
diagnostic instead of printing it, so a mid-read overwrite no longer emits a
misleading byte-mismatch line ahead of the real cause; `fill_slot` returns a
three-way `Fill` so a genuine bounds failure cannot pass for the deliberate
abandon; the consumer asserts a fresh view is alive, not only that a freed
one is not; and the producer checks `pool_write_in_progress` after the
abandoned guard dies, which is what that call is for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A kernel reads bytes the host wrote with no intervening copy, through the
device alias of the pool mapping: `cudaHostRegister(..., Mapped)` once per
view, `cudaHostGetDevicePointer`, then a comparison of all 16 slots against
the model, bracketed by `PoolReadGuard`. Nothing copies the payload
host->device; the only cudaMemcpy moves the kernel's verdict back.

On the Jetson's integrated GPU the pool resolves to transport=unified with
ipc_present=false, which is the acceptance criterion of dora-rs#2686. The
producer is not forked for it: dataflow-cuda.yml sets
DORA_MEMORY_POOL_TRANSPORT=unified in its env: block, which overrides the
`auto` the producer asks for and nothing else.

CudaMappedView models the release ordering cuda_pool.hpp names and nothing
enforced: cudaHostUnregister at the daemon's free notification, and again in
the destructor -- whose body runs before the rust::Box member drops and
unmaps the segment -- for the pool that is never freed.

outcome_name and segment_name_from_daemon move to nodes/checks.h rather than
being duplicated into the second receiver.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six checks, six deliberate breaks, and what each one printed. The most
informative is the device alias: on this Jetson cudaHostGetDevicePointer
hands back an address distinct from the host pointer, so substituting the
host pointer faults rather than passing by coincidence.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A CPU pool from the untouched Python binding and a C++ unified pool are
byte-identical on the wire, so a C++ receiver on an integrated GPU can map a
Python sender's segment for GPU access. Interop is a test, not a claim.

The example's own consumers could not be reused, and not for cosmetic
reasons: a Python pool id is generated rather than chosen, so it has to
travel on a topic; there is no way to abandon a write cycle through the
Python API, so the `Torn` outcome their exit gates require can never happen;
and a Python CPU pool reports transport `shmem`, which
`pool-receiver-cuda.cc` asserts against. `pool-interop-receiver.cc` is one
source compiled twice — g++ for the CPU path, nvcc for the device alias — so
the two paths cannot drift in what they claim about a Python segment.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ducer

Review follow-ups.

The README pointed at `cargo run --example cxx-memory-pool` for the producer,
which builds through `clang++ -std=c++20` -- absent from the JetPack image the
section otherwise targets. It now builds pool_sender with the same g++ as
everything else.

`transport == "unified"` asserts a label, not a behaviour: the zero-copy path
is byte-identical on a `shmem` pool, as the Python-interop run demonstrates by
reading one through the same device alias. Said so at the assertion and in the
README, since everything else in that file is careful not to overclaim.

Also: two cudaMemcpys, not one, neither touching the payload -- the count was
wrong in a file whose subject is which copies happen. Each CUDA call now
reports its own returned cudaError_t rather than re-reading the sticky slot,
which prints "no error" for a synchronous rejection. PoolReadGuard's throw is
caught and reported rather than escaping main, where terminate would skip the
cudaHostUnregister the file exists to demonstrate. And the header now makes
the stronger argument for the alias: the segment is registered before any
frame exists, so 21 frames of changing content through that same mapping rule
out a snapshot taken at registration time.

Declined: dropping <algorithm> from pool-receiver.cc. std::find_if (:70) and
std::all_of (:211) are still there; only std::count moved to checks.h, which
includes <algorithm> for itself.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ender

d85f05eb committed a line that publishes a deliberately wrong pool id, so the
CPU interop dataflow fails out of the box: the consumer looks up a pool the
daemon has never heard of. It was a reviewer's falsification probe, left in the
working tree of a checkout two agents were using at once, and swept up by an
unrelated `git add -a`.

Nothing about it was subtle to find once looked for — the marker was still in
the line. The lesson is about the checkout, not the code: a probe belongs in a
copy, never in the tree someone else is committing from.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rce rotting

The interop README replaced one overclaim with another: "differ in the
metadata JSON by that one key, and in nothing else". Dumping a live segment
of each finds four differences for the same 16 KiB payload — no `transport`
key, `pinned_type` "cpu" against "cuda", `json.dumps`'s spaced separators
against serde_json's compact ones (so `json_len` is 73 against 91), and
`write_gen` 2 after Python's register-with-copy where `PoolSegment::create`
leaves 0. None of them reach the consumer, and the reason is the better
argument: `json_len`, `data_offset`, `pinned_type` and the generation are
read out of the header, never assumed.

`doradma.rs` and `segment.rs` were the source of the belief — "exactly as
the Python binding does" and "byte-identical on the wire" — so they are
corrected too, since this branch is the first thing able to disprove them.

`run.rs` now compiles `pool_interop_receiver` with the other nodes, so the
source cannot rot when the pool surface changes, and runs the interop
dataflow when the workspace Python binding is importable, skipping loudly
otherwise. The nightly `examples` job provisions that binding on Linux so
the skip does not become permanent. The second dataflow is spawned as
`dora run` rather than a second `RunCommand::execute()`, which installs the
global tracing subscriber and so can only be called once per process.

`checks.h`'s segment-name assertion is now shared with the interop receiver
instead of copied into it, parameterised by the tail because the two
producers genuinely differ there: C++ derives its pool id and segment name
from one `naming::segment_name`, while Python formats them in two
independent `format!` calls that share only `<node>_<counter>`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Covers the three transports and how `auto` resolves, why the payload
length rather than dtype/shape is the only bound for a copy, why
`cudaHostRegister` takes the segment's mapping base and not the payload
pointer, and the two release points a CUDA node must unregister at
before its mapping disappears.

The guide section is the reference for the surface; the runnable
walkthrough stays in `examples/c++-memory-pool`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The acceptance evidence needs a reader to know it ran on an integrated GPU with
no CUDA IPC; it does not need the specific board model. Say "a Jetson" and keep
the JetPack and CUDA versions, which are what actually determine the behaviour
being demonstrated.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
caothu2k1 and others added 2 commits August 10, 2026 09:32
A review of the finished branch, rather than of each commit as it landed,
found documentation that survived earlier corrections and surface left behind
by them.

The Changelog said the DORADMA wire format "now has a single definition" while
also saying the Python binding is unchanged. Both cannot be true, and the
second is the accurate one: apis/python/node still declares its own constants
and writes and parses the header by hand. This branch adds a second definition
for the C++ binding to build on; consolidating is follow-up work, and the entry
now says so. The Python interop sender still described its segment as "the same
bytes" the C++ unified transport writes, three commits after the README and the
C++ consumer were corrected to name all four differences.

constants_match_the_python_binding promised to be a tripwire for Python drifting
and asserted this file's constants against literals in this file, so it could
never observe the thing it claimed to watch. It now reads the three constants
out of apis/python/node/src/lib.rs and fails loudly, naming the path, if that
file is unreachable — a version that skipped when the file was missing would
have the same defect it is fixing.

Producer-side pool_payload/pool_payload_offset were predicates whose false
branch that surface cannot reach: register_memory_pool rejects ipc twice over,
so a producer pool always has a payload. They are plain getters now, and
pool_ipc_present, PoolSegment::pinned_type and an unused Debug impl are gone.
The consumer side keeps its predicates, where ipc and freed pools make them
load-bearing.

PoolSegment::open now rejects a zero-size pool as create already did.
try_read_pool reads payload_len == 0 as "no payload, permanently", which was
sound only because create refuses size 0 — a foreign segment declaring 0 could
open, look live, and report Unavailable forever while the two causes the header
documents both failed to explain it.

free_memory_pool refuses while a write cycle is open. It carried the comment
explaining why but never the check; the helper existed and was tested directly,
so the rule passed its test without being wired to the path it governs.

Also: unified registers pinned_type "cuda", which sends a Python consumer down
its CUDA read path, so the override is not free of consequence for a CPU Python
receiver and the guide and README now say so; shared_memory_extended is a
workspace dependency across all four dependents rather than two; and the
example-smoke audit row cites the nightly step's real line.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The pin moved to [workspace.dependencies] so the crates that map and unlink the
same segment cannot drift onto different versions of the mapping. The daemon is
one of them and was missed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@trunk-io

trunk-io Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

Two repo lint gates I did not run before proposing the change.

The unwrap budget counted this crate's segment tests as production code and
failed at 202 against a budget of 163. The overage is the test module's own
size: scripts/qa/unwrap-budget.sh recognises test-only blocks by the literal
`#[cfg(test)]`, and the module was written `#[cfg(all(test, target_os =
"linux"))]`, which the check does not match. Stacked attributes mean the same
thing to rustc and are recognised, so the module is excluded again and the count
returns to what main already carried. This branch adds no production unwrap:
apis/c++/node/src/lib.rs counts 3 on main and 3 here, and the other four files
count 0.

The typos check reads `mis-sized` as a misspelling of `miss`/`mist`; reworded.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copy link
Copy Markdown
Collaborator

🤖 Automated review by Claude Code — fully automated review, not vetted by a human.

No issues found.

I reviewed the data-plane crate (seqlock.rs, doradma.rs, segment.rs, naming.rs), the C++ binding FFI, and the shipped C++ headers, focusing on the parts a seqlock/FFI transport tends to get wrong:

  • Seqlock ordering — the closing read correctly uses an acquire fence before a relaxed load (not a second acquire load), begin_write fences after marking the generation odd, and the leftover-odd recovery path lands back on an even baseline. OpeningSample/DoraPoolRead are opaque across the bridge so a caller can't forge or mis-fence a token.
  • Boundsheader_region_len caps the header slice at data_offset (never at segment_len), the ipc_flag/transport contradiction is rejected before the size check can be skipped, and offset arithmetic saturates/checks. payload() returns None for ipc rather than a one-past-the-end ptr with a large length.
  • Free/liveness racemark_freed flips the flags under the registry mutex with Release/Acquire pairing before the ids reach C++, read tokens are serial-bound to their issuing view, and the mapping is deliberately not unmapped on free, so a racing read is not a use-after-free.
  • Naming — component charset + explicit .. check on construction, and validate_segment_name re-guards prefix///.. at the unlink point for names arriving from a peer.

The tests exercise real behavior (specific error substrings, adversarial headers, cross-language segment shapes) rather than tautologies. Given the size and the unsafe/FFI surface, a human pass over the memory-ordering and the C++ header lifetimes is still worthwhile before merge.


Generated by Claude Code

@phil-opp

Copy link
Copy Markdown
Collaborator

Two things worth fixing before this lands, neither blocking the design.

Data race on the seqlock generation. segment.rs:360 / doradma.rs:386 form a &[u8] over the header region (HEADER_SIZE + json_len, so >=256 bytes). That span includes byte offset 96 — the seqlock generation a peer writer stores to via AtomicU64 — and it is read with read_u64. That is exactly the race the payload path takes care to avoid, and the comment at segment.rs:346-348 ("written once, before the pool is registered") is not true for those 8 bytes. ParsedHeader.write_gen is never read anywhere in the workspace, so dropping the field removes the race for free (otherwise load it through AtomicU64::from_ptr).

free_memory_pool can strand a segment. apis/c++/node/src/lib.rs:1790 refuses when a write cycle is open, but the Box<DoraMemoryPool> is consumed regardless — so after the refusal the caller has no handle left to close the cycle or retry. Net effect of the "safe" path is a segment left in /dev/shm and in the daemon's table for the rest of the dataflow. Returning the box on refusal would make it actionable.

Minor: DORA_MEMORY_POOL_TRANSPORT="" hits the Some(other) arm and hard-fails instead of being treated as unset (lib.rs:1488); and the nightly uv venv step writes VIRTUAL_ENV to $GITHUB_ENV, which applies to every subsequent step in the examples job, not just this one.

For the record, I read the memory ordering closely and it is correct — relaxed odd-store plus fence(Release) is the right formulation, and making OpeningSample opaque and non-PartialEq genuinely prevents the close-with-a-second-begin_read bug.

Review feedback on dora-rs#3106.

ParsedHeader carried a `write_gen` field read out of the header slice with a
plain `read_u64`, while a peer writer stores to those same 8 bytes through an
AtomicU64. Nothing in the workspace read the field — its only consumer was a
test assertion — so the race bought nothing. Dropped. Every generation access
already goes through `gen_ptr()` as an atomic.

The comment justifying the header-region reference claimed the whole span is
written once before registration. True of the magic, the lengths, the IPC flag
and handle, and the JSON; not of byte 96. Nothing reads it now, but the
reference still covers it, and narrowing that needs the fixed header and the
JSON passed as two slices — every fixed field this parses lives below offset
96. Recorded as a follow-up rather than claimed as safe.

free_memory_pool refused while a write cycle was open but consumed the
Box regardless, so after the refusal the caller had no handle left to close the
cycle or retry: the segment stayed in /dev/shm and in the daemon's table for
the rest of the dataflow. The "safe" path was worse than the mistake it
guarded. It borrows now, so a refusal is actionable.

DORA_MEMORY_POOL_TRANSPORT= is set-but-empty, which std::env::var reports as
Ok(""); it hit the invalid-value arm and failed the node instead of restoring
the default. Treated as unset, matching how an empty `requested` from the C++
side already meant "no preference". The test that asserted the old behaviour
asserted the defect, and now covers the fix.

The nightly venv step exported VIRTUAL_ENV through $GITHUB_ENV, which applies
to every later step in the examples job rather than to the one that needs it.
Scoped to the step.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@caothu2k1

Copy link
Copy Markdown
Contributor Author

Thanks — all four verified at the source and fixed in 4354a25. Three are fully closed; the first is closed only in part and I'd rather say so than let it read as done.

Seqlock read race. Confirmed: parse_header decoded write_gen with a plain read_u64 out of a span a peer writer stores to atomically, and the field's only consumer in the workspace was a test assertion. Dropped the field and the read. Every generation access already went through gen_ptr() as an AtomicU64, so nothing else changed.

You were also right that the justification comment was false for those 8 bytes, and that is the part still open. Dropping the field removes the read, but the &[u8] still spans offset 96, and by the standard that comment itself sets — UB whether or not anything reads through the reference — that is not yet clean. Narrowing it properly means passing the fixed header and the JSON as two slices; conveniently every fixed field parse_header decodes lives below offset 96, so 0..96 plus the JSON region covers it with nothing left aliasing the generation word. That touches parse_header's signature and 19 test call sites, so I did not fold it into a PR you are mid-review on. The comment now states the residual plainly instead of claiming the span is write-once. Happy to do the split here or as a follow-up — your call.

Strandable free. This one was a regression I introduced, and your reading of it is exactly right: the refusal landed in a late review round, and I added the guard without following what happens after it fires. The box was consumed regardless, so the "safe" path guaranteed a leak where the unguarded path merely risked one. It borrows now, so a refusal leaves the caller the handle to close the cycle and free properly. The example's release() was the only call site.

Empty env var. Fixed. Worth noting the test that covered it asserted the defect — it listed "" among the values that must be rejected — so correcting the behaviour turned it red. It now asserts that clearing the variable restores the default.

$GITHUB_ENV. Scoped to the step that needs it.

Gates green after the change: fmt, workspace clippy -D warnings, 87 + 56 unit tests, cargo check --examples, and the CPU example end to end on a live daemon (11 copied, 10 zero-copy, 1 torn, 1 unavailable, /dev/shm clean).

And thank you for reading the ordering closely — that is the part I most wanted a second pair of eyes on, and knowing the relaxed-store-plus-release-fence formulation and the OpeningSample opacity hold up is worth more than the rest of the review combined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants