Skip to content

fix(node): record zenoh-delivered inputs in the write_events_to log - #3103

Draft
phil-opp wants to merge 2 commits into
mainfrom
claude/dreamy-bardeen-tkzcup-record-zenoh-input
Draft

fix(node): record zenoh-delivered inputs in the write_events_to log#3103
phil-opp wants to merge 2 commits into
mainfrom
claude/dreamy-bardeen-tkzcup-record-zenoh-input

Conversation

@phil-opp

@phil-opp phil-opp commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Issue

EventStream::record_event (apis/rust/node/src/event_stream/mod.rs) serializes events into the optional write_events_to recording. It only handled the daemon/TCP path input variant:

NodeEvent::Input { id, metadata, data } => { /* full JSON */ }
...
_ => None,          // <- EventItem::ZenohInput lands here

EventItem has two input-bearing variants — NodeEvent::Input (daemon path) and ZenohInput (the direct zenoh data plane added in #2366). Both are converted to a user-visible Event::Input by convert_event_item, i.e. they are the same class of event to the user. But ZenohInput fell through the outer _ => None, so it was never recorded.

Because the zenoh data plane bypasses the daemon entirely, the daemon cannot record these inputs either. So for any dataflow whose node-to-node inputs take the direct zenoh path (the optimized common case), the write_events_to JSON silently loses those inputs — while still reporting recording_status: clean. Replay tools and integration-test oracles get an incomplete recording that looks complete. No error is raised, so the #1857 poisoning machinery doesn't catch it — the events are simply dropped.

Fix

  • Add a ZenohInput arm to record_event that serializes the already-decoded ArrayData and tags it "type": "Input", mirroring the NodeEvent::Input arm.
  • Refactor the JSON encoding out of convert_output_to_json into shared json_header + append_arrow_array_json helpers, and add a convert_arrow_input_to_json entry point that takes an already-decoded ArrayRef (the daemon path decodes a DataMessage first; the zenoh path is already decoded). Sharing the encoder keeps the two recording paths from drifting.

Validation

  • Added zenoh_input_serializes_into_recording_json, asserting a ZenohInput's array serializes into the same id / data / data_type recording shape with all elements present.
  • cargo +1.97.1 fmt -p dora-node-api -- --check — clean.
  • cargo +1.97.1 clippy -p dora-node-api -- -D warnings — clean.
  • cargo +1.97.1 test -p dora-node-api --lib zenoh_input_serializes — passes.

⚠️ This is a machine-generated pull request authored by Claude (Claude Code). A human should review before merging.

🤖 Generated with Claude Code


Generated by Claude Code

EventStream::record_event only serialized daemon-path
`NodeEvent::Input` events; `EventItem::ZenohInput` fell through the
outer `_ => None` arm and was never pushed to the recording buffer.
Both variants surface to the user as `Event::Input`, but the zenoh data
plane bypasses the daemon, so neither the recorder nor the daemon
captured those inputs — a dataflow whose node-to-node inputs take the
direct zenoh path (the optimized common case) produced a
`write_events_to` recording that silently omitted them while still
reporting `recording_status: clean`.

Add a `ZenohInput` arm that serializes the already-decoded array through
a new `convert_arrow_input_to_json` entry point. The JSON encoding is
refactored out of `convert_output_to_json` into shared `json_header` +
`append_arrow_array_json` helpers so the daemon-path and zenoh-path
recordings can never drift. Adds a unit test for the zenoh serialization.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1Fmp8pELuTiPGTStomkZj
@trunk-io

trunk-io Bot commented Aug 9, 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

A zenoh-delivered input can carry a remote HLC timestamp earlier than
this node's start_timestamp. json_header computed the offset with
Timestamp::get_diff_duration, an unguarded NTP64 (u64) subtraction that
underflows in that case — a debug panic (which unwinds past add_event's
Err guard and kills the event loop) or a release wraparound to a garbage
time_offset_secs. Clamp to a zero offset when the input predates start.
The daemon path only records locally-timestamped inputs (always >=
start), so it is unaffected. Adds a regression test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1Fmp8pELuTiPGTStomkZj

Copy link
Copy Markdown
Collaborator Author

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

No issues found.

I verified the fix against the code:

  • EventItem::ZenohInput previously fell through the outer _ => None in record_event, so zenoh-data-plane inputs were never written to write_events_to. The new arm mirrors the daemon-path NodeEvent::Input arm exactly — same convert_*_to_json encoder, same "type": "Input" tag, same id/data/data_type shape — and since ZenohInput.data is an ArrayData, make_array(data.clone()) is the right way to lift it to an ArrayRef. These items reach record_event via receiver.recv()add_event, so the arm is genuinely exercised.
  • The underflow guard is a real fix: uhlc 0.9's Timestamp::get_diff_duration is an unguarded NTP64 (u64) subtraction, so a zenoh input whose remote HLC timestamp predates start_timestamp would debug-panic (past add_event's Err-only guard) or wrap in release. The input_ts.get_time() >= start_timestamp.get_time() check clamps to Duration::ZERO and is a safe no-op on the daemon path.
  • Both tests are meaningful: zenoh_input_serializes_into_recording_json asserts the full element/data/data_type shape, and zenoh_input_with_earlier_timestamp_does_not_underflow constructs a strictly-earlier input_ts and asserts a 0.0 offset — it would panic without the clamp.

Generated by Claude Code

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