fix(transport): contain callback exceptions at C ABI boundary (#67)

[tetsuh]

Closes #67

Scope

• Define safe handling for empty and throwing user callbacks in Get and Queryable callback thunks.
• Catch exceptions at the outermost C callback boundary and terminate the affected callback operation safely.
• Preserve owned-resource cleanup on normal and exceptional paths.
• Add regression coverage for callback error containment where testable.

Changes

src/transport/zenoh_transport.cpp

• OnGetReply (Get reply closure): wrap the complete C callback thunk in try/catch(...). On throw, stop further replies for this Get (c->stop = true) so the closure drains without re-entering a throwing sink. A ZenohOwned<z_owned_slice_t> RAII owner releases a successfully created slice on both normal and exceptional paths.
• Queryable closure thunk: extract OnQueryable as a named callback and wrap the complete thunk in try/catch(...). Atomic callback_state->active deactivation runs on normal, exceptional, and inactive-queryable paths.
• DeclareQueryable: reject empty callbacks at declaration time (return empty handle) instead of registering a closure that would throw std::bad_function_call on every query.

tests/integration/transport_test.cpp

• QueryableCallbackExceptionIsContained: a throwing queryable callback does not crash the process; the session remains usable (round-trips a second working queryable afterward).
• GetSinkExceptionIsContained: confirms the throwing Get result sink is invoked, contains its exception, and verifies the session remains usable afterward.
• EmptyQueryableCallbackIsSafe: an empty callback yields a safe empty handle.

AC Verification

1. No zenoh-c callback lets a C++ exception cross the C ABI boundary. — Both OnGetReply and OnQueryable wrap their complete callback thunks in try/catch(...).
2. An empty callback has defined, safe behavior. — DeclareQueryable returns an empty handle for an empty callback; the thunk also guards if (state->callback).
3. Existing transport round-trip and no-queryable tests remain green. — 97/97 tests pass including QueryableRoundTrip, GetWithoutQueryableDoesNotInvokeSink, PutAndDeleteReturnOk.

RED Phase

Confirmed before the fix (current main):

• QueryableCallbackExceptionIsContained: Timeout — the thrown exception escaped the zenoh-c closure, disrupting the internal runtime thread so the session stopped responding.
• GetSinkExceptionIsContained: Failed — C++ exception with description "sink boom" thrown in the test body (escaped the C ABI boundary into the test runner).
• EmptyQueryableCallbackIsSafe: passed (declaration did not crash even before the guard).

After the fix all three pass and the full suite is green (97/97).

Notes

• The contained exception is swallowed silently because zenoh-c callbacks have no error/abort channel. Stopping further replies for the affected Get is the safest recoverable behavior.
• std::bad_function_call from an empty callback is now impossible because empty callbacks are rejected at DeclareQueryable time; the if (state->callback) guard in the thunk is a defensive second layer.

[coderabbitai]

📝 Walkthrough

Walkthrough

Zenoh transport callback handlers now contain exceptions from Get sinks and Queryable callbacks, reject empty Queryable callbacks, add atomic query activity gating, and include integration tests covering safe failure behavior and continued session operation.

Changes

Callback safety

Layer / File(s)	Summary
Harden Zenoh callback boundaries src/transport/zenoh_transport.cpp	Get reply decoding and sink invocation are exception-contained; queryable callbacks use atomic activity state, empty callbacks return an empty handle, and query callback failures disable further replies.
Validate callback safety tests/integration/transport_test.cpp	Integration tests cover throwing Queryable callbacks, throwing Get sinks, empty callbacks, and subsequent transport usability.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Zenoh as zenoh-c
  participant Transport as OnGetReply / OnQueryable
  participant Callback as User callback
  participant Query as TransportQuery
  Zenoh->>Transport: Invoke callback thunk
  Transport->>Callback: Deliver reply or query
  Callback-->>Transport: Return or throw
  Transport->>Query: Update stop or active state
  Query->>Zenoh: Reply only while active

Loading

Possibly related PRs

• tetsuh/sitos#58: Introduced the ZenohTransport Get and Queryable callback paths modified here.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR implements the linked issue goals: exception containment, safe empty callbacks, cleanup preservation, and regression tests.
Out of Scope Changes check	✅ Passed	The code and tests stay focused on callback exception containment and related safety checks, with no clear unrelated changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check	✅ Passed	The title clearly states the main change: containing callback exceptions at the C ABI boundary.
Description check	✅ Passed	The description is mostly complete and covers the main template sections, though it omits explicit requirement IDs and the ADR checkbox.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/67-transport-callback-exception-containment

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/transport/zenoh_transport.cpp`:
- Around line 197-208: Move the exception guard in OnGetReply and the queryable
closure to encompass all callback operations, including string and encoding
setup, make_shared, and key-expression conversion. Ensure any exception escaping
those preparation steps is caught at the outermost zenoh C callback boundary and
sets the existing stop flag without propagating.

In `@tests/integration/transport_test.cpp`:
- Around line 124-141: Update the test around transport_->Get to wait for the
queryable/callback processing to complete before asserting result and
sink_called. Add an explicit completion signal, synchronize access to
sink_called when callbacks may run on another thread, and ensure the test still
verifies Get succeeds without invoking the sink after the throwing queryable.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a1b622ea-0e7e-4553-87bb-a59b9a5a9c08

📥 Commits

Reviewing files that changed from the base of the PR and between 56add3b and 0719ea2.

📒 Files selected for processing (2)

• src/transport/zenoh_transport.cpp
• tests/integration/transport_test.cpp

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
5 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud