Skip to content

The ffi feature is built by no crate on the PR gate, and its re-entrancy detector was deleted #291

Description

@YuanYuYuan

Part of #282 — the defect class and its fix shape are stated there.

Residual, mechanism gap. Found by an adversarial review of #250.

The gap

hiroz's ffi feature is enabled by no crate in the workspace, and ci.yml never passes it.

⚠️ Corrected 2026-08-06. An earlier revision said "every FFI code path is unbuilt and untested on the PR gate". The unbuilt half is wrong: .github/workflows/test.yml triggers on pull_request and builds --features "ffi,<distro>,rmw-zenoh" --lib at :179-183. What is missing is narrower and still real:

on the PR gate ffi
compiled test.yml:179
linted (clippy -D warnings) ❌ never
tested ❌ never — and the detector was deleted

This is not theoretical — it has already let a defect through, on the very PR that found it.

Evidence: a type error reached main's gate unnoticed

#250 changed SubscriberHandle::Advanced { dispatcher } to hold an Option. There are two construction sites; the second is behind #[cfg(feature = "ffi")]. It did not compile.

Commit 68af53d8 exists solely to repair it. Its message records that ci.yml "compiled clean there" — but also that test.yml's FFI build step did catch it. So the compile error did not pass the gate; ci.yml alone would have missed it.

⚠️ The sentence that used to stand here — "a compile error passed the PR gate" — was wrong. The real cost is the row below it.

defect on #250's FFI arm caught? by what
type error (68af53d8) test.yml's build step
behavioural defect — node.rs:682 still passing DISPATCH_UNBOUNDED (12a968a5) nothing. A wrong constant is neither a compile error nor a lint, and the only test that could have caught it was deleted by 1dcf3736

That second row is this issue's whole point: building is not testing.

And the runtime detector is gone

1dcf3736 deleted crates/hiroz-tests/tests/reentrant_raw_publish.rs and reverted hiroz-tests' ffi feature. Consequence, verifiable today:

Delete let _local = crate::pubsub::LocalPublishGuard::enter(); from crates/hiroz/src/ffi/publisher.rs and nothing in the suite fails. local_only_shim sees depth 0, the raw callback runs inline on the publishing thread, and a raw callback that republishes recurses until the stack is exhausted.

The revert's stated reason was 22 pre-existing missing_safety_doc violations failing under -D warnings. That is a lint-scoping problem — a module-scoped #![allow(clippy::missing_safety_doc)] with a TODO resolves it — not a reason to drop the only detector for a re-entrancy path.

The wider shape

The guard is required on five publish paths. Only two have detectors:

path LocalPublishGuard detector
publish
async_publish
publish_serialized
publish_sample
ffi::publisher raw publish ❌ (deleted)

The module doc explicitly warns that "a fifth publish path added later must do the same" — with no mechanism that fails when it doesn't. That is the same shape as #283's motivating argument: correctness resting on someone having remembered.

Acceptance

  • ffi is built and linted on the PR gate, not only in release.yml
  • The raw-publish re-entrancy detector is restored, with the lint scoped rather than the test dropped
  • Removing LocalPublishGuard::enter() from any of the five publish paths fails at least one test — demonstrated per path, not asserted

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions