fix: six independent correctness fixes (C/C++ APIs, record, ROS2 bridge, tensor-pool, type registry)

[phil-opp]

Six independent bug fixes, one commit each, so they can be reviewed (or split out) commit by commit. Each commit message explains the full failure mode.

Changes

1. node-api-cxx — send_arrow_output leaves the caller's Arrow structs alone on early failure (apis/c++/node/src/lib.rs, docs/api-cxx.md)
   The caller's ArrowArray/ArrowSchema were moved out and zeroed before the output id was validated and the lock taken. On an early error the caller either dereferenced a NULL release, or, on retry, hit an assert! in arrow's import that aborts across the cxx bridge. The checks (valid id, struct not already released, lock acquired) now run before anything is moved. The metadata is moved instead of cloned.

2. C APIs — tolerate NULL handles; reject non-UTF-8 operator output ids (apis/c/node, apis/rust/operator/types)
   free_dora_context, free_dora_event and dora_next_event dereferenced NULL. init_dora_context_from_env is documented to return NULL on failure, so a failed init crashed any cleanup that frees unconditionally. NULL is now a no-op for the free functions (like free(NULL)) and end-of-stream for dora_next_event.
   dora_send_operator_output used char_p_ref::to_str (from_utf8_unchecked), so a non-UTF-8 id caused UB. The id is now validated and rejected with an error.

3. record — entry offsets use the monotonic clock (libraries/recording, binaries/record-node, binaries/cli/src/command/record.rs)
   Offsets were wall-clock deltas, so an NTP/GPS step during capture broke replay pacing: a forward step meant waits of decades, a backward step meant an unpaced burst. A shared dora_recording::offset_nanos now measures Instant time and both recorders use it. The header still records the wall-clock start.

4. ros2-bridge-node — no hang on Stop with ≥2 DDS subscriptions (binaries/ros2-bridge-node/src/main.rs)
   Each subscription ran on its own blocking thread, and all threads were joined after the event loop. On a quiet topic the join never returned, so the daemon had to force-kill the bridge at the grace deadline. The threads are gone: the streams are merged with select_all on the caller's executor, the same way run_single_subscriber does it.

5. tensor-pool — spawn-time orphan sweep covers machine-qualified segment names (libraries/extensions/tensor-pool, binaries/daemon/src/dataflow_lifecycle.rs)
   When the daemon has a machine id, segments are named dora_pool_{machine_id}_{dataflow_id}_…. The sweep only matched dora_pool_{dataflow_id}_, so /dev/shm segments left by crashed nodes leaked in any deployment with a machine id. The daemon's machine id is now passed through and both prefixes are matched. Another machine's segments still never match.

6. core — a top-level types/std.yml can't claim the reserved std/ namespace (libraries/core/src/types.rs)
   The reservation checked the file's directory, but a top-level std.yml produces std/<key> URNs, which could silently override built-in types. The check now runs on the derived URN prefix.

Known follow-up (not in this PR): the pool segment-name format is written in one place and parsed by hand in two (cleanup_orphans, is_machine_segment). A shared parse_pool_shmem_name in the core tensor-pool crate would stop the two sweeps from drifting apart again.

Validation

Class: B

• cargo fmt --all -- --check: ✅
• cargo clippy --all --all-targets (Python crates excluded) -D warnings: ✅
• cargo test --all (standard exclusions, --no-fail-fast): 2490 passed, 21 failed. All 21 failures are environmental: the test container can't open IPv6 sockets (Can not create a new TCP listener bound to tcp/[::]:0 … Address family not supported by protocol). They are all Zenoh-session tests in dora-core (topics::tests::unreachable_peer_teardown_bounded), dora-daemon (endpoint_exchange, reconnect/finish tests), dora-node-api (zenoh_schema_cache) and dora-ros2-bridge (rmw_zenoh). unreachable_peer_teardown_bounded fails identically on unmodified origin/main in the same container. I expect CI to be green on these.
• cargo check --examples: ✅
• New regression tests:
  • send_arrow_output_error_keeps_caller_ownership, send_arrow_output_rejects_released_structs (fix 1)
  • null_context_and_event_are_tolerated, dora_send_operator_output_rejects_non_utf8_id (fix 2)
  • offset_nanos_is_monotonic_elapsed_time (fix 3)
  • load_user_types_top_level_std_file_rejected (fix 6)
• No new regression test for fixes 4 and 5. Fix 4 depends on a live DDS subscription and there is no ROS distro here. Fix 5 is covered only by the existing cleanup_orphans_* tests, which still pass; none of them creates a machine-qualified segment.
• Not run: make qa-fast as a whole (its fmt/clippy halves were run individually).

🤖 Generated with Claude Code

https://claude.ai/code/session_01BpkxYmUYGvJ3bfGkkzqBhw

[trunk-io]

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

[phil-opp]

🤖 Automated review (Claude Code). This review is fully automated, with no human in the loop. Treat it as advisory only.

Reviewed head 4111973 (6 commits). Each of the six fixes addresses a real bug on main. I found no correctness regressions, and breaking-changes.sh --fast passes. I found the following issues:

1. This should be split.

• The six fixes are fully independent: no two commits touch the same file, and together they cover six subsystems. Commit 8f18694 itself bundles two unrelated fixes, the C node API NULL handling and the operator-types UTF-8 check.
• Mixing contract changes on frozen surfaces (the C++ API) with internal and exempt crates makes the accept/revert decision all-or-nothing.
• Suggested split:
  • the C/C++ node-API contract fixes, reviewed against the 1.0 guarantee on their own;
  • record + ROS2 bridge;
  • tensor-pool + core types.

2. send_arrow_output ownership contract change on the 1.0-frozen dora-node-api-cxx (b18059d).

• On main, the structs are moved out before validation. A retry after a bad-id error, or any call with already-released structs, therefore panics inside arrow's FFI import and aborts across the cxx bridge. The fix is right.
• The contract changes, though. After an early error (bad output id, poisoned lock), the caller now owns the structs. Existing C++ code that ignores the error and does nothing was fine on main and now leaks the Arrow buffers.
• It's documented in docs/api-cxx.md and the example. Because it changes a frozen surface, it should get a changelog / release-note entry rather than sit inside a bundle.

3. Two fixes aren't guarded by a real regression test.

• Record, monotonic offsets (927edb9): offset_nanos_is_monotonic_elapsed_time` only checks the new helper against its own arithmetic.
  • It "fails" on main only because the helper doesn't exist.
  • Nothing checks that record-node or dora record --proxy actually use the monotonic clock, so reverting either call site stays green.
• ROS2 bridge, Stop hang with ≥2 subscriptions (508e6c3): no test at all.
  • The bug is real: a quiet topic's subscriber thread keeps the join blocked until the grace-period kill. The select_all rewrite looks correct.
  • If a DDS-free test isn't feasible, please say so in the PR.

4. User-visible behaviour change (4111973, types).

• A project whose top-level types/std.yml defines only new keys, with no collision, built on main. It now fails dora build / dora validate.
• This is justified, since std/ is documented as reserved, but it should be in the changelog.

Tests. I ran each of these with only the non-test code reverted, and each fails:

• both cxx tests;
• null_context_and_event_are_tolerated (SIGABRT on main);
• dora_send_operator_output_rejects_non_utf8_id;
• cleanup_orphans_leaves_another_daemons_segments_alone;
• load_user_types_top_level_std_file_rejected.

All pass at the head.

---

Generated by Claude Code

[phil-opp]

🤖 Automated follow-up review (Claude Code). This review is fully automated, with no human in the loop. Treat it as advisory only.

Head 4111973 is unchanged since the last review. A fresh pass found one new issue, related to point 2 of the earlier review.

The mdBook guide still documents the old send_arrow_output ownership contract (b18059d). The PR updates docs/api-cxx.md and the example in examples/c++-arrow-dataflow, but not guide/src/languages/cpp.md, which documents the same API:

• guide/src/languages/cpp.md:155 still says "Ownership of the Arrow data transfers to Rust on success." With this PR, an early error (invalid output id, released struct, poisoned lock) leaves the structs with the caller.
• The full example at guide/src/languages/cpp.md:~490-505 only logs result.error. Under the new contract, C++ code that copies that example leaks both Arrow buffers whenever the send fails early. The docs/api-cxx.md copy of the same example got the two if (x.release) x.release(&x); lines (docs/api-cxx.md:656-658); the guide copy did not.

Suggested fix: apply the same wording change and release-on-error lines to the guide. This matters more than usual because the contract change lands on the 1.0-frozen C++ surface, and the guide is where most users will read it. (Verified by reading guide/src/languages/cpp.md at the PR head.)

---

Generated by Claude Code

[phil-opp]

🤖 Automated follow-up review (Claude Code). This review is fully automated, with no human in the loop. Treat it as advisory only.

Head 4111973 is unchanged. A fresh pass found one new point about the C++ fix (b18059d, earlier point 2). The ownership contract change on the frozen cxx bridge looks avoidable.

• On main, send_arrow_output_impl already consumes the structs on every path except null pointers. It ptr::reads them, zeroes the caller's copies, and imports them before parse_output_id / lock_node. On a bad id or a poisoned lock, the imported array and schema are dropped before the error returns, so they are released and nothing leaks.
• The abort only happens when a struct that is already released or zeroed gets imported. The typical case is a caller resending after a failed call.
• The new guard at apis/c++/node/src/lib.rs:1572 (is_released() / release.is_none()) is enough on its own to turn that abort into an error.
• It is moving parse_output_id / lock_node ahead of the import that hands the structs back to the caller on error. That is also what requires the extra release lines in docs/api-cxx.md and the example, and what makes existing "log the error and continue" C++ code leak, including the unchanged guide example.

Suggestion: keep only the released-struct guard, and leave the id/lock checks after the import as on main. The 1.0 contract ("consumed unless a pointer is null") then stays as it is. The docs, example and guide changes, and the changelog entry for this part, would no longer be needed. The existing cxx regression tests should still fail without the guard; please re-check that.

The earlier points (split the PR, the missing record/ROS2 regression tests, the types/std.yml behaviour change) still apply.

---

Generated by Claude Code

[phil-opp]

🤖 Fully automated review by Claude Code. No human checked this; it's advisory only.

I found one new issue at 4111973, in the run_multi_subscriber part (binaries/ros2-bridge-node/src/main.rs, around L1925-1975):

The hang fix moves all ROS2 decoding onto one thread, and a smaller fix that keeps decoding parallel exists.

• Before: each subscription's async_stream_seed(deserializer) stream ran under block_on_stream on its own thread. CDR→Arrow decoding for N topics ran in parallel, and the main loop only forwarded (output_id, ArrayData) to send_output.
• After: select_all(ros_streams) is merged into dora_events and polled by the single block_on_stream(merged) loop. Decoding for every topic now runs one after another, on the same thread that also handles Dora events, handle_publish_input (the publish direction) and send_output.

For a topics: bridge with several large topics (cameras, point clouds), throughput drops from up to N+1 cores to one, and a burst on one topic delays the others and the Dora→ROS direction.

The hang came only from the trailing for handle in handles { handle.join() }: a thread blocked on a quiet topic never noticed the closed channel. Removing that join loop, and letting the detached threads be torn down at process exit, fixes the Stop hang without giving up parallel decoding. If the single-thread merge is the preferred design anyway (for symmetry with run_single_subscriber), please say in the PR that losing parallel decoding is intentional. This also sits next to the earlier points that this PR should be split, and that this fix has no regression test.

---

Generated by Claude Code

[phil-opp]

🤖 Fully automated review by Claude Code. No human checked this; it's advisory only.

Head 4111973 is unchanged. A fresh pass found one new point, which adds to the earlier point about the send_arrow_output ownership contract (b18059d).

The changed ownership rule also applies, silently, to send_arrow_service_request.

• send_arrow_service_request (apis/c++/node/src/lib.rs:1348-1358) calls the same send_arrow_output_impl.
• Its bridge doc (lib.rs:319-321) says it "Consumes the Arrow C Data Interface structs … exactly like send_arrow_output does".
• With this PR, an early error (invalid output id, poisoned lock) leaves the structs with the caller there too.
• The PR updates docs/api-cxx.md and the arrow example only for send_arrow_output. Neither the bridge comment nor any doc for the service variant (apis/c++/node/README.md:218, docs/patterns.md:375) mentions the new release-on-error duty.

Failure scenario: a C++ service client calls send_arrow_service_request with a mistyped output id and only checks the returned error. On main the structs were imported and freed on that path. With this PR they are never released, so both Arrow buffers leak on every such failed call, and nothing tells the caller.

This is another reason for the earlier suggestion to keep only the released-struct guard and leave the id/lock checks after the import, so the 1.0 contract stays "consumed unless a pointer is null" for both functions. If the contract change is kept, the service variant needs the same documentation and changelog entry.

---

Generated by Claude Code

[phil-opp]

🤖 Fully automated review by Claude Code (no human checked this; advisory only). Reviewed at 4111973.

Head is unchanged. This pass checked the six fixes against the other open PRs. I found one new overlap:

#3720 already contains the metadata part of fix 1. #3720 (perf(node-api-cxx): move the metadata parameters into an Arrow send instead of cloning them) changes the same line in send_arrow_output_impl (apis/c++/node/src/lib.rs ~L1575) in exactly the same way: .as_ref().map(|m| m.parameters.clone()) becomes .map(|metadata| metadata.into_parameters()). git merge-tree merges the two cleanly only because the hunks are identical. When this PR is split (earlier point 1), please drop that part from the cxx commit b18059d. The commit would then contain only the ownership/released-struct fix.

No other PR duplicates the rest. I fetched #3693, #3678, #3717, #3690, #3681, #3718, #3645 and #3721 and checked them against this PR:

• fix(operator-api-cxx): deliver a timer tick or empty input to C++ as an empty slice #3693 is the C++ operator API (empty input as an empty slice), not the node API.
• fix(cli,record-node): pass the recorded descriptor encoded, past env expansion #3681 touches record.rs and record-node/src/main.rs, but for the base64 descriptor env var, not entry offsets.
• fix(tensor-pool): keep the cross-machine write-ack timeout past the mirror's read window #3690 and fix(tensor-pool): don't reuse a pooled direct-TCP connection the mirror closed #3717 change the tensor-pool cross-machine data plane and ack timeout, not the orphan sweep.
• fix(ros2-bridge): reject a null sequence, array or nested-message row #3678 is in ros2-bridge/arrow serialization. fix(ros2-bridge-node): keep the DDS service and action bridges alive across a failed call #3718 and fix(ros2-bridge-node): don't cancel finished goals or replace a live goal in the Zenoh action server #3645 are the service/action paths of ros2-bridge-node, not run_multi_subscriber.

Each of these merges cleanly with this branch. origin/main has not fixed any of the six since the merge base 3d3a90d, and the branch still merges cleanly into it. The earlier open points still apply: split the PR, the send_arrow_output/send_arrow_service_request ownership contract change, the missing record/ROS2 regression tests, and the std.yml changelog note.

---

Generated by Claude Code

[phil-opp]

🤖 Automated review (fully automated Claude Code review, not checked by a human; advisory only). Reviewed head 4111973 (unchanged).

One new point, from a PR opened since the last review:

#3730 is a standalone copy of fix 2 (927edb9, record offsets on the monotonic clock), and the two conflict.

• Both PRs change the same three files the same way. They take an Instant base next to start_nanos in run_record_proxy (binaries/cli/src/command/record.rs) and in binaries/record-node/src/main.rs, and both add a helper to dora-recording (libraries/recording/src/lib.rs).
• The helpers have different names and signatures. This PR adds offset_nanos(start, now); fix(record): measure entry offsets on the monotonic clock #3730 adds offset_since(start).
• git merge-tree pr-3660 pr-3730 reports content conflicts in all three files. Whichever lands second has to be redone by hand. If both were resolved by keeping both sides, dora-recording would end up with two public helpers for the same thing.
• Suggestion: drop 927edb9 from this bundle and land the fix through fix(record): measure entry offsets on the monotonic clock #3730. That also makes the split suggested earlier smaller. If you keep this version instead, please close fix(record): measure entry offsets on the monotonic clock #3730. Neither version has a test that fails when a call site goes back to the wall clock (point 3 of the first review), so the choice should not depend on test coverage.

I found this with git merge-tree and by comparing the two diffs. I did not build the merged tree.

---

Generated by Claude Code

[phil-opp]

🤖 Automated review (Claude Code) — this is a fully automated review with no human in the loop; treat it as advisory only.

Head 4111973 is unchanged. Since the last review, #3730 has merged to main (4ff9743), and this changes things for the PR:

• Fix 2 (927edb9, record offsets on the monotonic clock) is now on main, and the PR conflicts with main because of it. git merge-tree origin/main pr-3660 reports content conflicts in binaries/cli/src/command/record.rs, binaries/record-node/src/main.rs and libraries/recording/src/lib.rs. main already covers both call sites (run_record_proxy and the record node), using dora_recording::offset_since(start). If the conflicts are resolved by keeping both sides, dora-recording ends up with two public helpers for the same thing (offset_since and this PR's offset_nanos).
• Suggested fix: drop 927edb9 from the branch. I reverted just that commit locally and merged origin/main: there were no conflicts. On that tree, load_user_types_top_level_std_file_rejected (dora-core) and the cleanup_orphans tests (dora-tensor-pool) pass. I did not build the C/C++ or ROS2 parts.
• perf(node-api-cxx): move the metadata parameters into an Arrow send instead of cloning them #3720 has also merged. Its into_parameters() change to send_arrow_output_impl is identical to the hunk in b18059d, so that part is now a no-op, as the earlier review expected.

The other open points from earlier reviews still apply: split the PR, the send_arrow_output / send_arrow_service_request ownership contract change on the frozen C++ surface, the missing ROS2 regression test (and the loss of parallel decoding in run_multi_subscriber), and the types/std.yml changelog note.

---

Generated by Claude Code

[phil-opp]

Review by Claude: suggestion to drop one fix and split the rest

Thanks for collecting these fixes! Five of the six are still needed on current main and look correct. Bundled together, though, the PR is hard to land: one fix is now obsolete, and the bundle conflicts with main. Here is a suggested way forward:

1. Drop fix 3 (record offsets on the monotonic clock, 927edb9). It is already on main through fix(record): measure entry offsets on the monotonic clock #3730 (dora_recording::offset_since in libraries/recording/src/lib.rs, used by dora record and the record node). It is also the only cause of the merge conflicts in binaries/cli/src/command/record.rs, binaries/record-node/src/main.rs and libraries/recording/src/lib.rs. Without it, the remaining commits merge cleanly into main.

2. Open separate PRs for the rest, so each one can be reviewed and merged on its own. These three look ready as they are and could go first:

   • Fix 2: C API NULL handles and the non-UTF-8 operator id (8f18694). Optionally split this into its C-node part and its operator part.
   • Fix 5: machine-qualified tensor-pool orphan sweep (1d8c0e3). One correction for its PR description: the Validation section says there is no new regression test, but the diff adds three machine-qualified cases to cleanup_orphans_leaves_another_daemons_segments_alone.
   • Fix 6: types/std.yml reserved namespace (4111973). It needs a short changelog note, because a project with a top-level types/std.yml that used to build will now fail dora build / dora validate.

   These two need small changes first, as discussed in the earlier comments:

   • Fix 1: C++ send_arrow_output (b18059d). Keeping only the released-struct guard preserves the frozen 1.0 contract ("consumed unless a pointer is null"), and then no doc, guide or example changes are needed. The into_parameters() hunk is already on main via perf(node-api-cxx): move the metadata parameters into an Arrow send instead of cloning them #3720. If the ownership change is kept after all, examples/c++-arrow-dataflow/README.md is a third copy of the send snippet that would also need updating.
   • Fix 4: ROS 2 bridge Stop hang (508e6c3). Either explain why decoding all topics serially on the event-loop thread is acceptable, or use the simpler variant that only drops the trailing join loop. Both fix the hang.

3. Re-run CI after the split. The current green checks are from 2026-09-30 and don't reflect today's main.

A small note for the split PRs' descriptions: some earlier review comments call the record fix "fix 2", while this PR's body numbers it fix 3, so it's worth referring to commits by hash.

---

Generated by Claude Code