Skip to content

Full Bare Metal Support#112

Open
JustinKovacich wants to merge 134 commits into
mainfrom
feature/phase20_cleanup
Open

Full Bare Metal Support#112
JustinKovacich wants to merge 134 commits into
mainfrom
feature/phase20_cleanup

Conversation

@JustinKovacich
Copy link
Copy Markdown
Contributor

Summary

Closes the consolidated phase 18→20h punch list — 3 critical / 18 high
/ 22 medium / ~30 low items pulled from the adversarial line-by-line
review (2026-04-29) plus Copilot reviews on PRs #97#111. Each commit
is self-contained and tagged by severity for commit-by-commit review.

Commit map

  1. 5ad28ee — workspace clippy E0152 + embassy-net adapter soundness (CRIT-1/2/3, HIGH-4/5/6/21, MED-27/38)
  2. 878122e_alloc cfg gate (HIGH-7), OfferedEndpoint visibility (HIGH-8)
  3. 416b989select_biased! arm-flip fairness (HIGH-9), CI alloc-symbol audit holes (HIGH-10)
  4. 573346f — vsomeip conformance hardening (HIGH-11/12/13/14/17, MED-29)
  5. c5885ba — size_probe correctness (HIGH-15/16), sd_state half-public + doc-link rot (HIGH-20), static mut doc-example UB (HIGH-18), tokio Spawner doubled tasks (HIGH-19), MED-30/47, intra-doc fixes
  6. 62dfac3 — server constructor validation (MED-22 cluster), Phantom marker (MED-35), per-package pedantic clippy gates (MED-37), embassy-net loopback test honesty pass (MED-28)
  7. 7b0aa61 — CHANGELOG [Unreleased] + final verification + corrected vsomeip TX assertion

Load-bearing fixes

  • CRIT-2/3 (embassy-net adapter soundness): dropped a bogus
    'pool lifetime + an identity-only transmute<&SocketPool, &'static>;
    marked EmbassyNetFactory: !Send + !Sync via PhantomData<*const ()>
    because embassy-net's Stack interior RefCell isn't safe to drive
    bind() on from multiple threads.
  • HIGH-7 (alloc cfg gate): tied extern crate alloc and the
    Arc<T>: SharedHandle<T> impl to a single internal _alloc feature
    implied by server / embassy_channels / std. Previous
    cfg(any(feature = "embassy_channels", feature = "server")) was right
    by accident and silently omitted std-only flavours.
  • HIGH-9 (select_biased! fairness): three event-loop sites used
    select_biased! (select! needs std, dropped in 18d) but comments
    still claimed select!-style fairness. Server + socket_manager 2-arm
    selects now flip arm priority each iteration to approximate that
    fairness without pulling std.
  • HIGH-13/14 (vsomeip conformance): TX test now captures TWO
    announcements and asserts exact TTL (3 s default), session-ID
    monotonicity, reboot-flag behaviour, and (entries, options) == (1, 1).
    RX test now verifies vsomeip's OfferService carries the expected
    IPv4 endpoint option — a parser regression dropping options would
    have passed the old entry-only check.
  • HIGH-19 (tokio Spawner doubled tasks): TokioSpawner::spawn used
    to spawn TWO tokio tasks per call (the work future + a JoinHandle
    watcher for panic logging) — that's UNICAST_SOCKETS_CAP extra tasks
    per Client. Folded into PanicLoggingFut via
    std::panic::catch_unwind. One task per spawn.

Test plan

  • cargo fmt --all --check
  • cargo clippy --workspace --all-features -D warnings -D clippy::pedantic
  • cargo clippy --no-default-features -D warnings -D clippy::pedantic
  • cargo clippy -p simple-someip --no-default-features --features {client,bare_metal | server,bare_metal | client,server,bare_metal} -D warnings -D clippy::pedantic (new gates added in this PR)
  • thumbv7em-none-eabihf cross-build matrix; client+bare_metal rlib has 0 alloc-symbol references
  • cargo test --no-default-features (4 doc tests; previously failed due to test_support cfg-mismatch with the trait surface — fixed in 878122e)
  • cargo test --features client-tokio,server-tokio --tests --test-threads=1 — 478 lib + 11 client_server + 1 bare_metal_e2e + 3 ignored vsomeip
  • cargo test -p simple-someip-embassy-net --tests 3/3
  • SIMPLE_SOMEIP_TEST_INTERFACE=127.0.0.1 cargo test … tx_announcement_loop_emits_wire_format_offer -- --ignored passes on a multicast-enabled lo
  • cargo doc --no-deps partial-feature subsets {client | server,bare_metal | --all-features} — zero warnings
  • cargo build --release --target thumbv7em-none-eabihf for size_probe (now its own standalone workspace)

Known caveats

  • Parallel tests/client_server.rs flakiness is pre-existing on
    main
    (verified by checking out origin/main and running the same
    test set). Tests share SD multicast port 30490 and unicast ports
    across tests; CI uses cargo llvm-cov nextest which serializes by
    default, so this is dormant in CI. Not introduced by this branch;
    fix would be ephemeral ports per test as a follow-up.
  • MED items deferred: cluster A's "explicit unit tests for
    new_with_handles / SocketPool::claim/release/Drop" — these are
    exercised indirectly via the embassy-net loopback integration suite.
    Adding standalone unit tests is bare-metal plan v3 phase 21+ work.
  • MED-37 partial: per-package pedantic clippy added for the
    bare-metal triad on simple-someip; the embassy-net-adapter feature
    combos still funnel through --workspace --all-features.

Source reviews

JustinKovacich and others added 30 commits April 24, 2026 18:40
Covers the send path end-to-end (SOME/IP envelope, SD flags, OfferService
entry fields, and the IPv4 endpoint option) plus session-id advancement
and wrap-through-zero exercised via send_offer_service itself, and a
smoke test for Server::start_announcing. All `#[ignore]`d pending the
loopback MULTICAST-flag fix on this branch; without that fix, hosts
drop the multicast packet silently and the tests time out on recv.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Rustfmt on stable wraps the let-else continue blocks and the
assert_offer_matches signature differently than I hand-wrote.
Let cargo fmt normalize the style.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Per AUTOSAR SOME/IP-SD, the reboot bit on emitted SD messages must
flip from RecentlyRebooted to Continuous once the session counter
wraps past 0xFFFF. SdStateManager already owns the counter, so it
also tracks wrap (new has_wrapped: AtomicBool latched exactly on
the 0xFFFF -> 0x0001 transition) and exposes reboot_flag() as the
single source of truth.

The four SD emission paths — SdStateManager::send_offer_service,
Server::send_unicast_offer, send_subscribe_ack_from_view, and
send_subscribe_nack_from_view — all now consume the tracked flag
instead of hardcoding Flags::new(true, true).

Coverage: four new non-ignored unit tests cover the state machine
(fresh, sub-wrap, exactly-on-wrap, monotonic-after-wrap);
assert_offer_matches takes an expected RebootFlag; the existing
wrap multicast test now asserts first-emit=RecentlyRebooted and
second-emit=Continuous across the boundary.

Responds to PR #75 feedback on src/server/sd_state.rs:90.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Round-2 Copilot review caught a real correctness bug in the prior
reboot-flag refactor: the four SD emission paths read
`reboot_flag()` BEFORE advancing the session counter, so the
message whose session_id crosses 0xFFFF -> 0x0001 (where
`has_wrapped` actually latches) still advertises
`RebootFlag::RecentlyRebooted`. The flip only lands on the NEXT
emission — violating AUTOSAR SOME/IP-SD semantics that say the
wrap message itself should carry `Continuous`.

Reordered in all four sites: call `next_session_id()` first so
`has_wrapped` latches, then read `reboot_flag()` for this specific
message. Sites:

- `SdStateManager::send_offer_service` (sd_state.rs)
- `Server::send_unicast_offer` (mod.rs)
- `Server::send_subscribe_ack_from_view` (mod.rs)
- `Server::send_subscribe_nack_from_view` (mod.rs)

Added short comments at each site pointing at the canonical
ordering note on `send_offer_service`.

Also reworded the multicast-loopback `#[ignore]` comment block and
per-test message to remove the stale branch-name reference
(`feature/firmware_someip_conversion`) — the underlying dependency
is the `lo` MULTICAST flag, not a branch-specific fix. New wording
says "skipped on hosts whose `lo` lacks the MULTICAST flag" with
the `ip link show lo` diagnostic pointer.

Coverage: the existing ignore-gated wrap test
`send_offer_service_wraps_session_id_through_zero_on_send` already
asserts the pre-wrap/post-wrap flag transition on-the-wire; with
the ordering fix it now passes in environments that run ignored
tests (would have FAILED before this commit — which is why the
bug slipped past the first round). The non-ignored state-machine
tests (`reboot_flag_flips_to_continuous_exactly_on_wrap` et al.)
are unaffected and still green.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot round-3 flagged the `#[ignore]` reason on
start_announcing_emits_first_offer_within_timeout for still
carrying a branch-specific phrase ("re-enable after lo fix on
this branch"), which becomes stale once merged. Replaced with
a durable prerequisite description:

  requires loopback multicast support (MULTICAST on lo)

Matches the companion rewording in a638a4b on the sd_state.rs
multicast-loopback harness comment block.

Addresses Copilot comment 3132878961.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- inner.rs: on request_queue overflow, reject the ControlMessage by
  sending Err(Error::Capacity("request_queue")) on every oneshot it
  carries (via new ControlMessage::reject_with_capacity helper) rather
  than silently dropping it — the drop previously cancelled the oneshot
  and panicked callers awaiting with .unwrap().
- inner.rs: on pending_responses.insert saturation, destructure the
  returned (request_id, response) and send
  Err(Error::Capacity("pending_responses")) on the response sender;
  previously the sender was dropped, panicking PendingResponse::response.
- mod.rs: update the send_to_service saturation doc to match the new
  explicit-error behavior.
- mod.rs: fix the module-header footprint doc — SESSION_CAP lives in
  session.rs, not inner.rs.
- session.rs: gate the saturation warn! behind a one-shot saturation_warned
  latch; previously the warning fired on every check() against every new
  key once the map was full, which meant log-spam at the packet rate.
- error.rs: drop #[non_exhaustive] on client::Error; downstream crates
  relying on exhaustive matches shouldn't silently break before a planned
  breaking release.

New tests: reject_with_capacity_notifies_every_sender covers every
ControlMessage variant (including SendToService's dual senders);
capacity_overflow_warns_only_on_first_hit locks in the saturation latch.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Commit cb1d0d1 added explicit capacity-error delivery when
pending_responses.insert overflows — without the explicit Err send,
the dropped Sender would cause PendingResponse::response().await to
panic on RecvError instead of surfacing a clean Result.

The recovery logic was inline in the SendToService run-loop arm,
which made it hard to exercise without driving 64 live sockets. Lift
it to a small `track_or_reject_pending_response` helper on Inner so
the SendToService arm delegates, and the branch is testable against
the exposed `pending_responses` map directly.

Added two tests:
- `track_or_reject_pending_response_inserts_when_room_available`:
  happy path — entry lands in the map, the sender is still live
  (receiver stays pending).
- `track_or_reject_pending_response_rejects_on_saturation`: fill
  map to PENDING_RESPONSES_CAP, invoke the helper with one more
  request, assert the map is unchanged and the caller's receiver
  resolves to Err(Error::Capacity("pending_responses")) — which is
  the invariant cb1d0d1 guarantees.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Round-2 Copilot feedback on PR #76 caught 5 resolvable issues (6th
rejected — see below):

- src/client/inner.rs: the three `push_front(...).ok()` re-enqueue
  sites (SetInterface x2, SendSD, Subscribe) silently dropped the
  returned Err. Swapped each to `if let Err(rejected) = ... { ...
  rejected.reject_with_capacity("request_queue"); }`, matching the
  primary `push_back` overflow arm. These branches are defensive —
  by construction the slot we just popped is free — but a future
  refactor that changes queue usage would otherwise reintroduce the
  silent-drop-of-oneshot-senders regression cb1d0d1 was specifically
  written to prevent.

- src/server/subscription_manager.rs: `list.push(...).ok()` on a
  freshly-allocated `SubscribersList` (first subscriber in a new
  event group) swapped to `.expect(...)` with a message naming the
  `SUBSCRIBERS_PER_GROUP >= 1` invariant. Tripwires a future cap-0
  regression at test time instead of silently losing the only
  subscriber.

- src/client/session.rs: reword the `SessionTracker` doc comment's
  reference to non-existent "module docs" — point directly at the
  `SESSION_CAP` constant instead.

Rejected: src/client/error.rs `Capacity` variant as a breaking
exhaustive-match break. This is consistent with the earlier
decision recorded on #75/#80 to accept the breaking change rather
than carry deprecated shims or wrap everything in `Error::Io` —
the current release is a breaking version bump by design, and
hiding the variant in `Io` would lose the lowercase-snake_case tag
semantics the new error was specifically designed to carry.

No production behavior changes — just defensive tripwires, docs.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address two clippy::pedantic warnings introduced by this branch's
own commits:

- missing_panics_doc on SubscriptionManager::subscribe — the
  heapless::Vec::push.expect on the first-insert path can only
  trip if SUBSCRIBERS_PER_GROUP is set to zero; document that.
- cast_possible_truncation on 'i as u32' in the saturation test
  for pending_responses — use u32::try_from with an expect that
  documents why the 64-cap fits.
Copilot round-3 on PR #76 (comment 3138669839) caught a real gap
in `track_or_reject_pending_response`: the helper only handled
`Err((key, value))` (map-full + new-key), but `heapless::IndexMap::
insert` on an existing key returns `Ok(Some(old_sender))` and the
old sender was silently dropped. If `request_id` is ever reused
while an older pending entry is still live — the documented
example is `session_counter` wrap-around — the caller awaiting
the original request would see a `RecvError` on channel
cancellation, which `PendingResponse::response()` turns into a
panic.

Reworked the `if let Err(...)` into a full `match` with all
three arms:

- `Ok(None)`            — normal insert, nothing to do.
- `Ok(Some(displaced))` — `warn!` that the slot was replaced,
                          complete the displaced sender with
                          `Err(Error::Capacity("pending_responses"))`
                          so the original caller gets a clean
                          `Result`, not a `RecvError` panic.
- `Err((_req_id, r))`   — existing saturation path (unchanged).

Also updated the doc comment on `track_or_reject_pending_response`
to describe the displacement contract.

Coverage: new test
`track_or_reject_pending_response_completes_displaced_sender`
explicitly inserts the same key twice and asserts the first
receiver resolves to `Err(Error::Capacity("pending_responses"))`
before the second sender still sits pending in the map.

Addresses Copilot comment 3138669839.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The old docstring explained why client::Error is not #[non_exhaustive]
but didn't acknowledge that adding variants to a non-#[non_exhaustive]
enum is itself a breaking change for downstream exhaustive matches.
Rewrite the note to call that out explicitly and flag future
#[non_exhaustive] as a planned breaking-release change, and record the
new Capacity variant under the Unreleased Added section.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SubscriptionManager::subscribe previously returned () and logged a
warn! on capacity failure, which let handle_sd_message send
SubscribeAck to a client whose subscription had silently been dropped
— the client would believe it was subscribed but never receive any
events.

- subscribe now returns Result<(), SubscribeError> with
  SubscribersPerGroupFull / EventGroupsFull variants; existing
  capacity/refresh semantics are unchanged.
- handle_sd_message inspects the result and emits SubscribeNack with a
  reason derived from the SubscribeError variant on rejection.
- EventPublisher::register_subscriber surfaces the same Result so
  external SD dispatchers can take the same corrective action.
- SubscribeError is re-exported from server::mod.
- Tests updated to consume the Result; subscription_manager overflow
  tests now assert the specific error variant.

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

- session::check: expand the doc comment with an explicit "Capacity
  behavior" section — the saturation path does not update stored state
  for new keys, which contradicts the old "Always updates the stored
  state" wording. Describe the actual behavior (normal path updates,
  new-key inserts under saturation are silently dropped and return
  Initial repeatedly) so callers don't rely on a strict guarantee the
  implementation doesn't hold.
- session::check saturation warn!: include sender + transport in the
  log line so diagnosing which peer lost reboot-detection state no
  longer requires out-of-band correlation.
- SubscriptionManager::subscribe dedup branch: rename the log from
  "Refreshed existing subscriber" to "already subscribed; skipping"
  and add a comment making it explicit that no per-subscriber state
  is modified. The old wording implied a refresh (TTL bump, etc) the
  code never performed.
- inner.rs test comment: tokio::sync::oneshot doesn't drop the sender
  when the receiver is dropped — it flips send() to a failure mode.
  Rewrite the comment to describe the actual stash purpose (keep
  channels open for later observation).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- server::run NACK arm: stop allocating a String with format!() held
  across the SubscribeNack await; log the SubscribeError at warn!
  separately and pass a static "subscription rejected" reason string
  to send_subscribe_nack_from_view. Cleaner and avoids the borrow-
  across-await style issue Copilot flagged.
- EventPublisher::register_subscriber: add # Errors section describing
  the two SubscribeError variants, that the subscriber is NOT
  registered on Err, and that external dispatchers should NACK on Err
  just like the server's own run() loop does.
- CHANGELOG: add server::SubscribeError to Added, and list the
  breaking signature changes on SubscriptionManager::subscribe and
  EventPublisher::register_subscriber under Changed.

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

- CHANGELOG.md: drop the two "Changed" bullets about `std` becoming the
  default feature and `thiserror`/`tracing` moving to
  `default-features = false`. Those landed in 0.6.0/0.6.1 (commit
  f161980, already on main) and are not changes made in this PR.
- server/mod.rs: when a SubscribeError rejects a subscription, match on
  the variant and pass a specific `&'static str` reason to
  `send_subscribe_nack_from_view` (`"subscribers_per_group_full"` /
  `"event_groups_full"`) instead of the generic
  `"subscription rejected"`. The NACK log line now reflects the real
  cause, and the static-str choice avoids any String-across-await
  allocation.
- server/subscription_manager.rs: rewrite the `subscribe` docs to say
  the duplicate path is idempotent / deduplicated rather than implying
  TTL-refresh semantics that don't exist today. Flag the future-TTL
  extension point explicitly so the doc and the log wording stay in
  sync if that behavior is added later.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Commit 343da67 added a `required_size() > UDP_BUFFER_SIZE` pre-check
to `EventPublisher::publish_event` but left the new branch uncovered.
Regression guard added as `publish_event_pre_encode_exceeds_udp_buffer_returns_capacity_error`:
registers a subscriber (the pre-check sits after the `subscribers.is_empty()`
early return, so the test needs one or else hits the false-positive
Ok(0) path), constructs a 1501-byte fixture (16-byte header + 1485-byte
payload, one over the cap), calls publish_event, asserts
Err(Error::Capacity("udp_buffer")). Mirrors the fixture pattern from
`send_raw_message_exceeding_udp_buffer_returns_capacity_error` on the
client side.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- src/lib.rs: UDP_BUFFER_SIZE doc now enumerates exactly which send
  paths honor this cap (SocketManager::send, publish_event,
  publish_raw_event) and explicitly calls out the SD announcement /
  SubscribeAck / SubscribeNack paths that still use heap Vec buffers
  as a known gap planned for the bare-metal no_alloc refactor.
- src/server/event_publisher.rs: reworded "stack buffer sized to MTU"
  comments at the two buffer-allocation sites — the buffer lives in
  the async future's state, not literally on the stack, and the cap
  is a UDP payload limit, not an Ethernet MTU. New wording points at
  the UDP_BUFFER_SIZE docs for the distinction.

The two `use std::vec` comments (event_publisher.rs:335,
socket_manager.rs:362) were verified to be false positives: removing
the imports breaks the lib-test build with 4 errors about `vec!`
macro not in scope. Same no_std mechanics as the prior #75-1
resolution — reply posted on the comment threads.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three doc-only Copilot round-3 nits on PR #77:

- src/client/mod.rs: the per-`SocketManager` memory-footprint blurb
  implied the second `[u8; UDP_BUFFER_SIZE]` is always allocated.
  In fact `socket_loop_future` only allocates that scratch buffer
  when the send path actually needs E2E protection (the destination
  key is in the `E2ERegistry`); plain sends pay only ~1.5 KiB.
  Reworded the always-live vs peak budget so the ~24 KiB number is
  no longer presented as the steady-state cost.

- src/client/socket_manager.rs: the E2E-overflow test comment said
  "8 over MTU", but `UDP_BUFFER_SIZE` is documented as a UDP payload
  cap, not an Ethernet-MTU-safe size. Reworded to "8 bytes over
  UDP_BUFFER_SIZE".

- src/lib.rs: the `UDP_BUFFER_SIZE` doc referenced bare
  `Error::Capacity("udp_buffer")`, which is ambiguous at the crate
  root (no `crate::Error` exists). Qualified to
  `client::Error::Capacity(...)` / `server::Error::Capacity(...)`.

Addresses Copilot comments 3138697909, 3138698042, 3138698156.

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

Adds a unit test that registers a Profile4 E2E profile for the message
key and publishes a message whose raw encoded size fits UDP_BUFFER_SIZE
(1496 bytes) but whose protected size does not (1508 bytes, after the
12-byte Profile4 header). Asserts that publish_event returns
Error::Capacity("udp_buffer") — exercising the post-protect guard that
was previously only covered on the client send path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- publish_event: log now says "E2E-protected datagram (... header +
  protected payload)" so the 16+protected_len value is identified as
  the full SOME/IP datagram size, not the payload.
- test fixture comment: "8 over MTU" → "8 bytes over UDP_BUFFER_SIZE"
  for terminology consistency with the rest of the PR.

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

`header_len + payload.len()` used unchecked `usize` addition. On a
system with large enough `payload.len()` the sum can wrap, silently
bypassing the `> UDP_BUFFER_SIZE` guard and corrupting the slice
operations that follow. Switch to `checked_add` and treat overflow the
same as exceeding `UDP_BUFFER_SIZE` — return
`Error::Capacity("udp_buffer")`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Client/server "E2E-protected payload" logs now say
  "E2E-protected datagram (header + protected payload)" since the
  logged value is the full SOME/IP datagram size, not the payload.
- publish_raw_event checked_add-fail log now describes the real
  condition (usize overflow) and includes the input lengths, instead
  of falsely pointing at UDP_BUFFER_SIZE.
- Oversize fixtures in socket_manager + event_publisher tests are now
  sized from UDP_BUFFER_SIZE (and PROFILE4_HEADER_SIZE implicitly for
  the E2E case) instead of hardcoded 1480/1485. Fixtures stay valid
  if the cap is retuned.
- client/mod.rs memory-footprint doc no longer hardcodes "(1500
  bytes)" or "~1.5 KiB" as load-bearing numbers; the scaling is now
  expressed in terms of UDP_BUFFER_SIZE with the current-default
  numbers as a parenthetical reference.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The module-level "# `Send` and multithreaded executors" section showed
a HRTB bound on `<T as TransportSocketSendFut<'a>>::Fut: Send` as the
way consumers should bind `Send`. No such trait exists in this crate
— with RPITIT the returned future type is anonymous and cannot be
named, and introducing a GAT-style escape hatch would pollute the
trait for the common single-threaded case.

Replaced with the reviewer-preferred pattern: wrap the call in an
`async move` block and require `T: Send + 'static` on the captured
state. A tokio-backed implementation whose underlying `UdpSocket` is
already `Send + Sync` produces `Send` futures automatically via
async-block capture inference, so no trait-level bound is required.
Implementations holding `!Send` state fail the `T: Send` bound at
the `tokio::spawn` call site, which is the actionable location.

Docs-only change; `cargo test --doc` passes on the new ignore-fenced
example.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
JustinKovacich and others added 15 commits April 29, 2026 09:51
Adds `SdStateHandle` + `WrappableSdStateHandle` traits in
`src/server/sd_state.rs` and threads them through `Server` as a
new `Hsd` type parameter (default `Arc<SdStateManager>`). Mirrors
the pattern established by 19f's `SocketHandle` /
`WrappableSocketHandle`. Same shape, same Send/Sync defaults
(neither bound at trait level — caller adds at use sites).

Two impls ship:
- `Arc<SdStateManager>: SdStateHandle + WrappableSdStateHandle`
  (the existing default; preserves std-side behavior).
- `&'static SdStateManager: SdStateHandle` (no-alloc; user
  declares `static SD_STATE: SdStateManager = SdStateManager::new();`
  and supplies `&SD_STATE` via a future `Server::new_with_handles`
  constructor).

`SdStateManager` itself becomes `pub` and `SdStateManager::new()`
becomes `pub const fn` so the static-storage pattern compiles.
The internal methods (`next_session_id_with_reboot_flag`,
`reboot_flag`, `send_offer_service`) stay `pub(super)` —
consumers shouldn't call them directly; they go through Server.

Server's existing `new_with_deps` / `new_passive_with_deps`
constructors require `Hsd: WrappableSdStateHandle` because they
build the manager internally via `SdStateManager::new()` then
`Hsd::wrap(...)`. The future `Server::new_with_handles` will
take `Hsd: SdStateHandle` directly (no `wrap` step), enabling
the no-alloc path with `&'static SdStateManager`.

`announcement_loop`'s method-level `where` clause picks up the
new `Hsd: Send + Sync` bound, mirroring the existing `H: Send +
Sync` and `F: Send + Sync` bounds. The `_local` variant has no
such requirement and works for any `Hsd: SdStateHandle`.

Type-signature width: Server now reads `Server<R, S, F, Tm,
H = Arc<F::Socket>, Hsd = Arc<SdStateManager>>`. Both
defaults preserve every existing call site — `Server<R, S, F, Tm>`
and `Server<R, S, F, Tm, Arc<SomeSocket>>` both still resolve
correctly. No churn in `tests/` or `examples/`.

Clears 20-pre alloc audit's category-E.2 finding. Combined with
the 4f9d36e recv-buffer split, two of the four "no-alloc Server"
remediation items are done.

What this leaves:
- E.1: `Arc<EventPublisher<R, S, H>>` field on Server. Same
  shape via an `EventPublisherHandle` trait — next branch.
- D: `Server::new_with_handles` constructor that takes pre-built
  `H` + `Hsd` (and the future `Hep` for E.1) directly, skipping
  the `wrap` step. Lands after E.1 so the constructor's
  parameter list is final.

Gates green:
- cargo fmt --check
- cargo clippy --tests (2 pre-existing warnings, unrelated)
- cargo build --workspace --all-targets
- cargo build --no-default-features --features client,server,bare_metal
- cargo build -p simple-someip-embassy-net --target thumbv7em-none-eabihf
- cargo test --features client-tokio,server-tokio --test client_server -- --test-threads=1 (11/11)
- cargo test --features client,server,bare_metal --test bare_metal_e2e (2/2)
- cargo test -p simple-someip-embassy-net --test loopback (3/3)

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

Mirrors phase 20b's `SdStateHandle` work for the second of the
two production-path Arc usages flagged by the 20-pre alloc
audit (finding E.1). Adds:

- `EventPublisherHandle<R, S, H>: Clone + 'static` trait in
  `src/server/event_publisher.rs`. Method:
  `fn publisher(&self) -> &EventPublisher<R, S, H>`.
- `WrappableEventPublisherHandle<R, S, H>: EventPublisherHandle<...>`
  extension trait with `fn wrap(EventPublisher<R, S, H>) -> Self`.
- `Arc<EventPublisher<R, S, H>>: EventPublisherHandle + WrappableEventPublisherHandle`
  (alloc-using std-side default; preserves existing behavior).
- `&'static EventPublisher<R, S, H>: EventPublisherHandle`
  (no-alloc bare-metal path; user declares the static, supplies
  the reference into a future `Server::new_with_handles`).

Server gains a third type parameter `Hep = Arc<EventPublisher<R,
S, H>>` (default), threaded through the struct decl and all three
impl blocks. Field `publisher: Arc<EventPublisher<R, S, H>>`
becomes `publisher: Hep`. `Arc::new(EventPublisher::new(...))`
in constructors becomes `Hep::wrap(EventPublisher::new(...))`.
`Server::publisher()` accessor return type changes from
`Arc<EventPublisher<R, S, H>>` to `Hep` — non-breaking for std
users who pick up the default; bare-metal users get their chosen
handle type back.

Existing call sites pick up the `Hep` default; no churn in
`tests/`, `examples/`, or any caller. All three Arc impls
(SocketHandle, SdStateHandle, EventPublisherHandle) follow the
same pattern: `Arc<T>` for std (alloc), `&'static T` for
bare-metal (no-alloc), `Wrappable*` extension for the inline-
construction path.

Three of four remediation items in the 20-pre alloc audit are
now done: the recv buffer (20a), `Arc<SdStateManager>` (20b),
and `Arc<EventPublisher>` (20c). The last item — combining
all three handle types into a single `Server::new_with_handles`
constructor that accepts pre-built handles directly without
the `wrap` step — lands in 20d.

Type-signature width: Server now reads `Server<R, S, F, Tm,
H = Arc<F::Socket>, Hsd = Arc<SdStateManager>, Hep = Arc<EventPublisher<R, S, H>>>`.
Three defaults preserve every existing call site. After 20d, a
bare-metal caller will spell out all three explicitly via the
new constructor, and a std caller will keep accepting all three
defaults via `Server::new_with_deps`.

Gates green:
- cargo fmt --check
- cargo clippy --tests (2 pre-existing warnings, unrelated)
- cargo build --workspace --all-targets
- cargo build --no-default-features --features client,server,bare_metal
- cargo build -p simple-someip-embassy-net --target thumbv7em-none-eabihf
- cargo test --features client-tokio,server-tokio --test client_server -- --test-threads=1 (11/11)
- cargo test --features client,server,bare_metal --test bare_metal_e2e (2/2)
- cargo test -p simple-someip-embassy-net --test loopback (3/3)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the no-alloc-friendly counterparts to `Server::new_with_deps`
and `Server::new_passive_with_deps`. Caller supplies all four
storage handles (`H` for sockets, `Hsd` for SD state, `Hep` for
EventPublisher) pre-built; Server stores them directly without
calling `factory.bind(...)` internally and without invoking any
`Wrappable*Handle::wrap` step.

This is the constructor path bare-metal-no-alloc consumers need:
they own their UDP transport (lwIP, vendor IP stack, etc.), bind
sockets externally, and wrap them via `StaticSocketHandle` /
`&'static SdStateManager` / `&'static EventPublisher<...>` —
materializing the static storage themselves at boot.

Surface additions:

- `pub struct ServerHandles<F, Tm, R, S, H, Hsd, Hep>` — bundle
  of factory + timer + e2e_registry + subscriptions + the four
  pre-built handles. Mirrors `ServerDeps` for the same caller
  ergonomics.
- `Server::new_with_handles(deps, config) -> Result<Self, Error>`
  — constructs an active server (announcement loop runnable, run
  loop runnable). Back-fills `config.local_port` from
  `unicast_socket.local_addr()` so SD offers advertise the bound
  port.
- `Server::new_passive_with_handles(deps, config) -> Result<Self, Error>`
  — same shape, marks `is_passive = true`.
- Re-exported via `simple_someip::ServerHandles`.

Both constructors live in the existing `impl@521` block whose
bounds (`H: SocketHandle`, `Hsd: SdStateHandle`, `Hep:
EventPublisherHandle` — all without `Wrappable*`) match what the
no-alloc path requires.

Both are synchronous (`pub fn`, not `pub async fn`) — no
`factory.bind()` to await. Std users who prefer the async-
ergonomic path keep using the existing `new_with_deps` /
`new_passive_with_deps`.

Combined with phases 20a-c, this completes the four-item
"no-alloc Server completion" remediation surfaced by the 20-pre
alloc audit:

- 20a: `run_with_buffers` — caller-provided recv buffer (clears
  audit category-D recv-buffer item).
- 20b: `SdStateHandle` — drops `Arc<SdStateManager>` (clears
  audit E.2).
- 20c: `EventPublisherHandle` — drops `Arc<EventPublisher>`
  (clears audit E.1).
- 20d: this commit — `new_with_handles` + `new_passive_with_handles`
  (clears audit category-D socket-Arc item by exposing the
  pre-built-handle path).

A consumer building TC4D firmware with `default-features =
false, features = ["client", "server", "bare_metal"]` and
banning `extern crate alloc` can now construct a Server via
`Server::new_with_handles(...)` using `&'static`-backed
handles, drive it via `run_with_buffers(&mut [u8; N], &mut [u8;
N])` over `static`-declared receive buffers, and emit SD via
`announcement_loop_local`. Zero `alloc::*` surfaces in any
production code path under that feature combo.

What this leaves: an actual no-alloc bare-metal example /
integration test against `simple-someip-embassy-net` (or a
future `simple-someip-lwip` adapter) using these constructors.
That's a separate "validation" commit — 20d ships the API; the
witness comes when the lwip adapter exists.

Gates green:
- cargo fmt --check
- cargo clippy --tests (2 pre-existing warnings, unrelated)
- cargo build --workspace --all-targets
- cargo build --no-default-features --features client,server,bare_metal
- cargo build -p simple-someip-embassy-net --target thumbv7em-none-eabihf
- cargo test --features client-tokio,server-tokio --test client_server -- --test-threads=1 (11/11)
- cargo test --features client,server,bare_metal --test bare_metal_e2e (2/2)
- cargo test -p simple-someip-embassy-net --test loopback (3/3)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collapses the three near-identical handle traits introduced in
19f / 20b / 20c into a single generic trait pair:

    pub trait SharedHandle<T: 'static>: Clone + 'static {
        fn get(&self) -> &T;
    }
    pub trait WrappableSharedHandle<T: 'static>: SharedHandle<T> {
        fn wrap(value: T) -> Self;
    }

Two blanket impls cover the alloc and no-alloc paths:

    impl<T: 'static> SharedHandle<T> for &'static T { ... }
    impl<T: 'static> SharedHandle<T> for Arc<T> { ... }   // alloc-gated
    impl<T: 'static> WrappableSharedHandle<T> for Arc<T> { ... }   // alloc-gated

Deleted (each was a slot-specific copy of this same shape):
- `SocketHandle` / `WrappableSocketHandle` (transport.rs).
- `SdStateHandle` / `WrappableSdStateHandle` (server/sd_state.rs).
- `EventPublisherHandle<R, S, H>` /
  `WrappableEventPublisherHandle<R, S, H>` (server/event_publisher.rs).
- `StaticSocketHandle<T>` (the `&'static T` blanket impl
  subsumes its only purpose: carrying the `'static` lifetime).

Net trait count: 6 + 1 wrapper struct → 2 traits. ~60% less
boilerplate across the three former trait modules.

`Server`'s three handle bounds become uniform:
- `H: SharedHandle<F::Socket>`
- `Hsd: SharedHandle<SdStateManager>`
- `Hep: SharedHandle<EventPublisher<R, S, H, F::Socket>>`

`EventPublisher` gains an explicit `T: TransportSocket`
parameter — the price of carrying `T` as a generic on
`SharedHandle<T>` rather than as an associated type. The
struct grows a `PhantomData<T>` field so the type parameter is
well-formed without affecting drop-check.

Method calls collapse to one name everywhere:
- `H::socket()` / `Hsd::sd_state()` / `Hep::publisher()` →
  `.get()` (consistent across all three slot types).

Default type-param expressions adjust to spell the `T`:
- `Hep = Arc<EventPublisher<R, S, H, <F as TransportFactory>::Socket>>`.

Test type-aliases gain the new `T` slot:
- `tests/client_server.rs::TestEventPublisher`
- `event_publisher.rs::TestEventPublisher` (internal)
- The `AlwaysFailSocket` regression-test type alias.

Pure rename / shape-change patch — no behavior change. The
runtime behavior of every public API call is identical to
20d's; this is type-system tidying motivated by the adversarial
review.

Adversarial-review observations addressed:
- "Three near-identical handle traits is a code smell" — fixed.
- "We didn't generalize this into a single Shared<T> trait" —
  done.

Trade-off accepted: `EventPublisher` signature widens from
`<R, S, H>` to `<R, S, H, T>`. The cost is one extra type
parameter at the EventPublisher layer; the win is removing six
trait definitions and one wrapper struct.

Gates green:
- cargo fmt --check
- cargo clippy --tests (2 pre-existing warnings, unrelated)
- cargo build --workspace --all-targets
- cargo build --no-default-features --features client,server,bare_metal
- cargo build -p simple-someip-embassy-net --target thumbv7em-none-eabihf
- cargo test --features client-tokio,server-tokio --test client_server -- --test-threads=1 (11/11)
- cargo test --features client,server,bare_metal --test bare_metal_e2e (2/2)
- cargo test -p simple-someip-embassy-net --test loopback (3/3)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
First test in the simple-someip crate that catches **protocol
non-compliance** bugs against an external SOME/IP-SD reference
(the COVESA vsomeip implementation), rather than running our
own impl on both sides of the wire and only catching internal-
consistency issues.

Scope: single SD `OfferService` reception. simple-someip's
`Client::bind_discovery()` listens for vsomeip's announcement of
a known service+instance pair, asserts the SD entry surfaces on
the update stream within a 30 s timeout. That single signal is
the load-bearing wire-conformance check we have zero of today.
Subsequent phases will layer Subscribe/Ack roundtrips,
request/response, E2E protect/check, etc. against the same
reference.

`#[ignore]`'d by default. The test depends on an external
vsomeip Docker container being up — see the test file's
module-level docs for the docker setup, the JSON config to
mount, and the env-var (`SIMPLE_SOMEIP_TEST_INTERFACE`) to
point at the test's listening interface. Phase 20g will wire
this into CI via TestContainers-rs (or similar) once the
manual setup is proven.

Why ignored not a CI step yet:

Per FW team confirmation, vsomeip-in-docker on CI is
approved-in-principle but not yet stood up. Shipping the test
infrastructure first lets the firmware team pick up the test
locally for debugging during the codec-MVP integration; CI
automation lands as 20g.

Cleanup folded in: clippy warnings surfaced under broader
feature combos (`--features client-tokio,server-tokio`) by
prior phases:

- `event_publisher.rs:57` doc-markdown `PhantomData` now
  backticked.
- `event_publisher.rs:670/732` `clippy::type_complexity`
  `#[allow(...)]`'d on the test type aliases (with reason
  string explaining why).
- `server/mod.rs:929` doc-markdown `TokioSocket` now
  backticked.
- `sd_state.rs` `Default` impl added on `SdStateManager`
  (clippy::pedantic; bare-metal callers should still prefer
  the explicit `const` `new()` for `static` initializers).

Default-features `cargo clippy --tests` had only 2 pre-existing
warnings before this commit and still has only 2 after; no new
warnings on the canonical CI gate. The broader-feature warnings
were a 20e-introduced side-effect.

Gates green:
- cargo fmt --check
- cargo clippy --tests (default features; 2 pre-existing
  warnings unrelated to this work)
- cargo build --workspace --all-targets
- cargo build --no-default-features --features client,server,bare_metal
- cargo build -p simple-someip-embassy-net --target thumbv7em-none-eabihf
- cargo test --features client-tokio,server-tokio --test client_server -- --test-threads=1 (11/11)
- cargo test --features client,server,bare_metal --test bare_metal_e2e (2/2)
- cargo test -p simple-someip-embassy-net --test loopback (3/3)
- cargo test --features client-tokio,server-tokio --test vsomeip_sd_compat (1 ignored as expected)

What this leaves for 20g:
- `tests/data/vsomeip-offerer/Dockerfile` building vsomeip from
  source.
- TestContainers-rs (or equivalent) integration so `cargo test
  --features client-tokio,server-tokio --test vsomeip_sd_compat
  -- --ignored` works in a CI runner with Docker available.
- vsomeip version pin matching whatever the FW team selects for
  production validation.
- Subsequent conformance tests: Subscribe/Ack, request/response,
  E2E roundtrips.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a `tools/size_probe/` workspace member that mirrors halo PR
#4429's `rust_simple_someip` C-callable FFI surface (header
encode/decode + E2E Profile 4/5 round-trips) and builds as a
`staticlib` for `thumbv7em-none-eabihf`. Used during phase
20-pre to estimate simple-someip's flash footprint.

Build + measure:

    cargo build -p size_probe --release --target thumbv7em-none-eabihf
    llvm-size target/thumbv7em-none-eabihf/release/libsize_probe.a

(rustup toolchain ships `llvm-size` under
`~/.rustup/toolchains/.../bin/`).

Why a probe instead of measuring simple-someip's rlib directly:
rlibs include compiler metadata that bloats them ~60×. A
staticlib with `extern "C"` entry points lets post-link
dead-code elimination strip everything an actual FFI consumer
wouldn't reach, giving a closer-to-real-world flash number.

First measurement (default release profile, no `opt-level=z`,
no LTO at the probe level): ~12 KB of simple-someip-specific
text + 14 KB of transitive dep code (heapless, thiserror,
tracing). Compiler-rt builtins and `core::fmt` chains aren't
simple-someip-unique — they're amortized firmware-wide — and
were excluded from the per-component breakdown.

NOT a production crate. Pure measurement tool. Includes a
panic-on-alloc stub `GlobalAlloc` to satisfy the link-target
requirement on builds where some transitive dep pulls
`extern crate alloc` even though the codec FFI surface itself
is alloc-free.

Why thumbv7em-none-eabihf and not the actual TC4D target:
halo's TriCore build pipeline uses an in-house LLVM-IR-to-
TriCore proxy + a private Docker image we don't have local
access to. cortex-m4f is the closest upstream-Rust-supported
target with similar code-density characteristics; gives a
defensible bracket for the real TC4D flash cost (likely within
±50% on the proxy toolchain).

Future use: when the Option-A stateful FFI surface lands,
re-add equivalent `extern "C"` shims for the new entry points
(`rust_handle_udp_rx`, `rust_tick`, etc.) and re-measure.
Lets us track the flash-cost delta from codec-only → full
state machines as that work progresses.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Makes phase 20f's `tests/vsomeip_sd_compat.rs` actually
runnable. Adds `tests/data/vsomeip-offerer/`:

- `Dockerfile` — multi-stage Ubuntu 22.04 base. Stage 1 builds
  vsomeip 3.4.10 (the LumPDK / EnVision pinned version per
  `LumPDK/packages/thirdparty/vsomeip/vsomeip.MODULE.bazel`)
  from upstream tarball, plus our minimal C++ offerer. Stage 2
  is a slim runtime image with just libvsomeip3 + the offerer
  binary + entrypoint. ~463 MB final image.
- `offerer.cpp` — ~85 LOC. Calls `application->offer_service(0x1234,
  0x0001, 1, 0)` and idles while vsomeip's SD subsystem emits
  cyclic OfferService broadcasts.
- `offerer.json` — vsomeip configuration. Standard SD multicast
  `224.0.23.0:30490` per spec defaults; cyclic_offer_delay=1000ms;
  ttl=5s. `unicast` is templated at container start (see below).
- `entrypoint.sh` — substitutes `VSOMEIP_UNICAST` env var into the
  JSON before exec'ing the offerer. Bails loudly if the env var
  isn't set. The substitution exists because `unicast: 127.0.0.1`
  doesn't work on Linux — `lo` lacks the `MULTICAST` flag by
  default, so SD multicast never actually leaves the host. Caller
  must pick a real interface IP via `ip route get 224.0.23.0`.
- `CMakeLists.txt` — builds offerer against `find_package(vsomeip3)`.
- `README.md` — full build + run + test invocation flow with the
  multicast-on-lo gotcha documented.

Test file (`tests/vsomeip_sd_compat.rs`) module docs updated to
match the new harness shape. The `#[ignore]`'d test itself is
unchanged from 20f.

Verified end-to-end on 2026-04-29:

    docker build --network=host -t vsomeip-offerer tests/data/vsomeip-offerer/
    docker run --rm -d --name vsomeip-offerer --network host \
        -e VSOMEIP_UNICAST=172.20.21.206 vsomeip-offerer
    SIMPLE_SOMEIP_TEST_INTERFACE=172.20.21.206 \
        cargo test --features client-tokio,server-tokio \
        --test vsomeip_sd_compat -- --ignored --nocapture
    # client_sees_vsomeip_offer_service ... ok in 0.59s

This is the FIRST wire-level conformance signal in the project.
Every prior test ran simple-someip on both sides of the wire and
couldn't catch protocol non-compliance against an external
reference. Today: simple-someip's Client successfully decoded a
real vsomeip-emitted SD `OfferService` entry — service ID,
instance ID, TTL, major/minor version, source address all
matched the spec.

What this proves:
- vsomeip 3.4.10 builds + runs from upstream source in our docker
- simple-someip's SD-receive code path is wire-conformant against
  vsomeip's SD-emit path for OfferService entries (one rung)

What this does NOT prove (worth being explicit about):
- Anything on TC4D — all of this is x86_64 Linux + native upstream
  Rust + tokio. No proxy LLVM-IR-TriCore exercise.
- Bidirectional wire compatibility — we only tested vsomeip ->
  simple-someip. The reverse (simple-someip emits SD that vsomeip
  parses) is the next test (phase 20h).
- Other SD entry types — FindService, SubscribeEventGroup,
  SubscribeAck, SubscribeNack are all separate code paths.
- Anything stateful — request/response correlation, subscription
  state, event publishing, E2E protect/check on real payloads.
- The lwip transport story — vsomeip uses its own UDP socket; nothing
  about Halo's planned lwip integration was tested.
- The Option-A FFI shape — doesn't exist yet. This test went
  through simple-someip's existing tokio/`Client` API, which
  Halo won't use in production.

CI integration deferred. The test stays `#[ignore]`'d by default;
flipping it on `cargo test` would fail until a CI runner has
docker + the harness available. That's the next phase (20i?)
once we have the full conformance test set built out.

What this leaves:
- 20h: bidirectional SD test (simple-someip emits, vsomeip
  subscribes; proves TX wire format).
- 20i+: SubscribeEventGroup roundtrip, request/response, E2E
  conformance.
- Eventual CI: TestContainers-rs (or equivalent) to bring up
  this docker on every PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds two TX-direction tests covering simple-someip's SD emit path:

`tx_announcement_loop_emits_wire_format_offer` (no docker, CI-gated):
drives Server::announcement_loop and captures the emitted bytes on a
second multicast socket joined to the SD group. Asserts every field
of the SOME/IP envelope (service/method/message-type/protocol+iface
versions, return code), the SD flags (unicast set), the OfferService
entry body (service+instance+major+minor+TTL>0), and the IPv4
endpoint option (interface, port, UDP). Catches silent regressions
in the emit path without requiring vsomeip up.

`vsomeip_sees_simple_someip_offer_service` (full cross-impl, optional):
keeps the existing docker-based subscriber test for cross-impl
validation when run on a second host. Module docs now record the
same-host caveat we hit: vsomeip's routing-host architecture binds
both endpoints to 0.0.0.0:30490 with SO_REUSEPORT, and same-host
multicast delivery between two such instances is non-deterministic
(reproduced with vsomeip-offerer → vsomeip-subscriber on the same
box). The docker test should be run on a second host sharing the
multicast-capable network.

CI: new step in the `test` job flips MULTICAST on `lo` and runs the
no-docker test with `--ignored --exact` so only that test runs (the
docker-dependent ignored tests stay skipped).

Both vsomeip JSON configs aligned to multicast 239.255.0.255 to
match simple-someip's hardcoded MULTICAST_IP. The subscriber.cpp /
subscriber.json / entrypoint.sh role dispatcher round out the docker
image so both offerer and subscriber roles ship in one container.

Follow-up: simple-someip's SD MULTICAST_IP is hardcoded to the
Luminar-internal 239.255.0.255; making it configurable would let us
run vsomeip with its spec-default 224.0.23.0 for stricter
conformance.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the four highest-severity items from the consolidated phase
18→20h punch list:

CRIT-1: `tools/size_probe` excluded from `[workspace]` and given
its own empty `[workspace]` table so `cargo clippy --workspace
--all-features` no longer trips E0152 against the probe's
`#[panic_handler]` / `#[global_allocator]`. Probe still builds via
`cd tools/size_probe && cargo build --release --target …`.

CRIT-2: Dropped the bogus `'pool` lifetime parameter on
`EmbassyNetFactory` and the `mem::transmute<&SocketPool, &'static>`
that was identity-only by accident. Factory now takes
`&'static SocketPool` directly; `&'static SocketPool` coerces
straight to `&'static dyn SlotReclaim`. Same observable behaviour
on every existing caller, less unsafe.

CRIT-3: Added `_not_thread_safe: PhantomData<*const ()>` to
`EmbassyNetFactory` so the factory is `!Send + !Sync`. embassy-net's
`Stack` uses interior `RefCell` for its socket-set bookkeeping and
is not safe to drive `bind()` on from multiple threads; this pins
the factory to a single executor task at the type level.

HIGH-4: Documented at the call site why
`RecvError::Truncated → Err(Io(Other))` is a deliberate adapter
choice rather than the trait's `truncated: true` semantics —
embassy-net 0.4 doesn't deliver any bytes on truncation and doesn't
surface the original datagram length, so we can't honor the trait
truthfully. Operator-side fix is to size `SocketPool` `RX_BUF` ≥
link MTU.

HIGH-5/6: `bind()` now honors `addr.ip()` (passes a full
`IpListenEndpoint` instead of just the port) and reads the actual
ephemeral port back from `socket.endpoint()` post-bind, so
`local_addr()` reports truth instead of the bind-time `:0`.

HIGH-21 + new shared `LINK_MTU` const: the loopback driver and
example client previously declared raw `1500` link MTUs that
silently coincided with `simple-someip`'s `UDP_BUFFER_SIZE`. Hoisted
a `pub const LINK_MTU: usize = 1500` into
`simple-someip-embassy-net` itself (with docs explaining it's the
*link-layer* cap, distinct from `UDP_BUFFER_SIZE`'s
*application*-payload cap) and switched both consumers to import it.

MED-22 (partial): `EmbassyNetBindFuture` now wraps
`core::future::Ready` instead of an ad-hoc `Option::take` that
bare-panicked on second poll; same semantics, stdlib panic message.

MED-38: Rewrote the `endpoint_to_socket_addr_v4` rationale comment;
the previous version conflated "non-exhaustive" with "no
`unreachable_patterns` attribute".

Verified: `cargo clippy --workspace --all-features` green;
`cargo test -p simple-someip-embassy-net --tests` all 3 pass;
`cargo build -p simple-someip --target thumbv7em-none-eabihf
--no-default-features --features client,bare_metal` green; size_probe
still builds standalone.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HIGH-7 + MED-36: Tied `extern crate alloc` and the
`Arc<T>: SharedHandle<T>` impl to a single internal feature
`_alloc`, implied by `server`, `embassy_channels`, and `std`. The
previous `cfg(any(feature = "embassy_channels", feature = "server"))`
was right by accident — duplicated across two locations and silently
omitted `std`-only flavours. The new gate makes the coupling
explicit so a future `client-tokio`-style consumer that legitimately
needs `Arc<T>: SharedHandle<T>` will get it without a fresh
cfg-juggling exercise.

HIGH-8: Re-exported `OfferedEndpoint` unconditionally. It was
gated on `feature = "std"` while the trait method
`PayloadWireFormat::for_each_offered_endpoint` that produces it is
unconditional, so no-std `client`-only consumers couldn't name the
type returned by a method they were expected to call.

Pre-existing bug surfaced as fallout: `cargo test
--no-default-features` was failing on `src/protocol/sd/test_support.rs`
since phase 18d removed `std` from the `client`/`server` feature set.
The trait method `new_subscription_sd_header` is unconditional; the
`TestPayload` impl was `#[cfg(feature = "std")]`. Same for
`set_reboot_flag`. Both now unconditional, with `std::net::Ipv4Addr`
swapped for the `core::net::Ipv4Addr` re-export the trait already
uses.

Verified: 13-config build matrix green; `cargo clippy --workspace
--all-features` and `cargo clippy --no-default-features` clean;
`cargo test --no-default-features` now compiles and runs (4 doc
tests pass). `client + bare_metal` rlib still has 0 alloc-symbol
references on `thumbv7em-none-eabihf`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HIGH-9: Three event loops use `select_biased!` (futures-util's
pseudo-random `select!` requires `std`, which we dropped from
client/server in 18d) but their comments still claim
`select!`-style fairness. Fixed by:

- `client/socket_manager.rs`: 2-arm send/recv. Added
  `prefer_recv_first` flip flag that toggles arm priority each
  iteration so a sustained one-sided load can't starve the
  other arm. Approximates pseudo-random fairness without `std`.
- `server/mod.rs::run_with_buffers`: 2-arm unicast/sd. Same
  flip pattern with `prefer_sd_first`.
- `client/inner.rs::run_future`: 4-arm
  control/sleep/discovery/unicast. Documented the deliberate
  top-down priority — control drives loop lifecycle, the other
  three aren't at real risk of sustained starvation in practice
  — with a forward-compat note pointing at the flip pattern if
  that ever changes.

HIGH-10: Two CI audit holes plugged in the alloc-symbol step:

1. `find target/... | head -1` was nondeterministic and could
   read stale artifacts from earlier matrix steps. Pinned to the
   exact `target/thumbv7em-none-eabihf/debug/libsimple_someip.rlib`
   path.
2. `rm -f libsimple_someip*.rlib` doesn't invalidate cargo's
   fingerprint cache, so the rebuild on the next line could no-op
   and leave the previous step's artifact in place. Replaced with
   `cargo clean -p simple-someip --target ...` which removes both
   the rlib and the fingerprint.
3. `nm 2>/dev/null` silently passed when the tool itself failed
   (missing binutils, malformed rlib). Dropped `2>/dev/null`,
   added `set -o pipefail`, kept the `|| true` only for the
   no-match case.

Verified: 478/478 lib tests pass under client-tokio,server-tokio;
all 13 build-matrix combos green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HIGH-11: subscriber.json's `clients[].unreliable` was 30509,
matching offerer.json instead of the simple-someip Server's
ADVERTISED_PORT (30500). subscriber.json is paired with the
simple-someip Server, not with offerer.json — the two configs are
independent. Fixed to 30500 with a comment pinning the
relationship.

HIGH-12: SocketOptions docs called out `SO_REUSEADDR` for the SD
port without mentioning the Linux requirement that BOTH
SO_REUSEADDR and SO_REUSEPORT be set on an SD socket sharing the
multicast port — the test was already setting both, but the
trait docs only documented one. Updated to make the requirement
explicit on both fields.

HIGH-13: TX wire-format conformance test rewritten to capture
TWO consecutive announcements and assert:
- Exact TTL (3 s default), not just `> 0`.
- Session-ID monotonicity across announcements via `request_id`.
- `RebootFlag::RecentlyRebooted` on the first announcement,
  flipping to `Continuous` on the second.
- Exactly one SD entry, exactly one SD option, with the expected
  `(first_options, second_options) == (1, 0)` count.
- IPv4 endpoint pin already covered, plus the dead
  `let _ = RebootFlag::RecentlyRebooted` import-pin
  (RebootFlag is now genuinely used).

HIGH-14: RX-direction test now verifies vsomeip's OfferService
carries an IPv4 endpoint option with `port=30509 UDP` — a parser
regression that silently dropped options would have passed the
old entry-only check.

HIGH-17: Module docs referred to multicast group `224.0.23.0`
(vsomeip spec default) while simple-someip and offerer.json both
override to `239.255.0.255`. Updated the `ip route get`
walkthrough and the failure-mode iptables hint to match the
group simple-someip actually uses, and explicitly noted the
non-spec-default in both places.

MED-29: Added `required-features = ["client-tokio", "server-tokio"]`
to vsomeip_sd_compat in Cargo.toml so `cargo test` cleanly skips
it under the wrong feature set instead of silently reporting "0
tests" while every test inside refers to types that aren't in
scope.

Verified: `cargo build --features client-tokio,server-tokio
--tests` passes; the conformance tests stay `#[ignore]`'d so CI
behavior is unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HIGH-15: `tools/size_probe`'s `someip_header_encode` validated
`MessageType` via `MessageType::try_from(byte & 0xBF)`, which
masked off bit 6 — a reserved bit pattern like `0x40` would
silently coerce to `Request` instead of erroring. Switched to
`MessageTypeField::try_from(byte)` which validates the raw byte
and only strips the TP flag (`0x20`) internally.

HIGH-16: Same function ignored the caller-supplied `length`
field and passed `payload_len = 0` to `Header::new`, producing
encoded headers with `length = 8` regardless of what the C ABI
caller asked for. Now derives `payload_len = length - 8` (the
SOME/IP `length` field covers the 8 fixed bytes after itself
plus the payload), with `checked_sub` for under-flow safety.

LOW (size_probe): `payload_len + 12` and `payload_len + 4` in
the E2E round-trip stubs would wrap on a 32-bit target with
sufficiently large input. Switched to `checked_add`. Also
renamed `PanicAllocator` to `NullAllocator` — it never panics,
returns null on alloc, and the docstring now explains the
runtime null-deref discipline rather than implying link-time
failures.

HIGH-18: Server's `run_with_buffers` doc example used
`&mut UNICAST_BUF` on `static mut` — hard error in Rust 2024
and unsound on any edition. Rewrote the example as a
`static UnsafeCell<[u8; …]>` with an `unsafe impl Sync` anchored
to the single-task-owner invariant.

HIGH-20: `SdStateManager::with_initial` and
`next_session_id_with_reboot_flag` lifted from `pub(super)` to
`pub` so external test harnesses can pre-seed counter state and
drive emission without a full Server lifecycle. The remaining
`reboot_flag()` / `next_session_id()` accessors stay
`pub(super)` + `cfg(test)` because they're deliberately racy and
only safe when no other emitter is concurrent. Doc link
`[Self::reboot_flag]` (which referred to the cfg(test) accessor
and broke under the public docs build) rewritten to point at the
production `next_session_id_with_reboot_flag` instead.

Adjacent doc-link fixes surfaced by the partial-feature
rustdoc gate:
- `SubscriptionManager::get_subscribers` referred to
  `Self::for_each_subscriber`; the method lives on the
  `SubscriptionHandle` trait. Re-pointed and additionally moved
  the cfg gate from `feature = "std"` to `feature = "_alloc"`
  with `alloc::vec::Vec` so the method is reachable in
  `embassy_channels` and pure-`server,bare_metal` builds where
  alloc is already in scope.
- `Server::publisher` referred to a removed `EventPublisherHandle`
  trait alias (collapsed into `SharedHandle` in 19f / 20e).
- `E2ERegistryHandle::register` referred to bare
  `E2ERegistryFull` instead of `crate::e2e::E2ERegistryFull`.
- `tokio_transport`'s named-future docs intra-doc-linked
  `futures::future::BoxFuture`, which doesn't resolve under
  `--all-features` (the futures crate isn't a direct dep). Made
  it a code literal.

MED-30: Server's `run_with_buffers` docstring claimed
`tracing::warn!` on backend truncation; the run loop never
inspects `ReceivedDatagram::truncated`. Rewrote to describe the
current (no-warn) behaviour and reference the bare-metal v3
backlog.

HIGH-19: `TokioSpawner::spawn` used to spawn TWO tokio tasks
per call (the work future + a JoinHandle watcher for panic
logging) — `UNICAST_SOCKETS_CAP` extra tasks per Client. Now
wraps the work future in `PanicLoggingFut`, which uses
`std::panic::catch_unwind` + `AssertUnwindSafe` to log panics
inline and resolve the future cleanly. One task per spawn.

Verified: `cargo doc --no-deps --features {client | server,bare_metal | --all-features}`
all clean (no broken intra-doc links); `cargo test --features
client-tokio,server-tokio --lib` 478/478 pass; size_probe still
builds for thumbv7em.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
MED-22: `Server::new_with_handles` and `new_passive_with_handles`
overwrote `config.local_port` unconditionally. Now back-fills only
when the caller passed `local_port = 0` and returns
`Error::InvalidUsage` when a non-zero `local_port` doesn't match
the unicast socket's bound port — matches the
back-fill-only-on-zero discipline of `new_with_deps`.

MED-35: `EventPublisher`'s `_phantom: PhantomData<T>` re-imposed
`T: Send + Sync` redundantly with `H`'s bounds. Switched to
`PhantomData<fn() -> T>` (unconditionally `Send + Sync`) so a
future `!Send T` behind a Send static-mutex handle doesn't force
`EventPublisher: !Send`.

MED-28: `client_send_request_server_runloop_stable` in the
embassy-net loopback test was vacuous — it constructed a passive
server then spawned `server.run()`, which returns
`Err(InvalidUsage)` on the first poll. Removed the no-op spawn,
rewrote the doc to honestly describe what the test verifies (the
client's send path, not a server runloop) and noted the kept-name
is for git-blame continuity with the parent reference test.

MED-37: Added per-package pedantic clippy gates for the bare-metal
feature subsets (`client+bare_metal`, `server+bare_metal`,
`client+server+bare_metal`) under `simple-someip` alone. The
existing `--workspace --all-features` pass was right by feature
unification but masked feature-specific regressions and tied
parent-crate lint health to embassy-net adapter dep storms.
Per-package gates surface a regression against the responsible
feature flag.

MED-30 (cont.): Documented `# Panics` on
`SdStateManager::next_session_id_with_reboot_flag` (lifted to
`pub` in the previous commit). The closure's `.unwrap()` is
statically infallible; doc'd as a tripwire.

Adjacent fixes surfaced by the new pedantic gates:

- `Server::run_with_buffers` was 104 lines after the
  select-arm-flip pattern duplicated the recv body twice. Factored
  the arms to return only `(datagram, from_unicast)`; the
  `(len, addr, source)` derivation lives once below the select.
  Now 82 lines, no `#[allow]` needed.
- `examples/embassy_net_client`: 4 pedantic violations
  (uninlined-format-args ×3, default_trait_access ×1). Fixed
  format-args inline; the default_trait_access on `dns_servers`
  is intentional (embassy-net's private heapless re-export type
  isn't reachable to spell out), now `#[allow]`'d with a
  one-line justification.
- `MED-47` (rolled in): `SubscriptionManager::get_subscribers`
  was gated on `feature = "std"` but only requires `alloc`.
  Switched to the new internal `_alloc` cfg + `alloc::vec::Vec`
  return type so the method is reachable in `embassy_channels`
  and pure-`server,bare_metal` builds.

Verified: `cargo clippy --workspace --all-features -D warnings
-D clippy::pedantic` clean; per-package pedantic gates clean for
all three bare-metal combos; 478/478 lib tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LOW (CHANGELOG.md): added an `[Unreleased]` section documenting
every change in this cleanup branch (Added / Changed / Fixed),
filling the gap the consolidated punchlist flagged on the 0.8.0
release line.

vsomeip TX-conformance assertion correction: the new test was
asserting `RebootFlag::Continuous` on the second announcement
under the misreading that the flag flips per-emission. Per
AUTOSAR SOME/IP-SD (and `SdStateManager`'s implementation), the
flag stays `RecentlyRebooted` until the session counter wraps
from 0xFFFF → 0x0001 — i.e. ~65535 announcements after boot. The
unit tests inside `sd_state.rs` cover the wrap transition itself;
this integration test now correctly asserts the flag is unchanged
across two ticks.

Full verification matrix (all green):
- `cargo fmt --all --check`
- `cargo clippy --workspace --all-features -D warnings -D clippy::pedantic`
- `cargo clippy --no-default-features -D warnings -D clippy::pedantic`
- `cargo clippy -p simple-someip --no-default-features --features {client,bare_metal | server,bare_metal | client,server,bare_metal} -D warnings -D clippy::pedantic`
- `cargo build` thumbv7em-none-eabihf no_std target across {bare_metal alone | server,bare_metal | client,server,bare_metal | client,bare_metal} — `client+bare_metal` rlib has 0 alloc-symbol references
- `cargo test --no-default-features` — 4 doc tests
- `cargo test --features client-tokio,server-tokio --tests --test-threads=1` —
  478 lib + 11 client_server + 1 bare_metal_e2e + 3 vsomeip_sd_compat (all
  ignored) all green. Note: `client_server` integration tests share
  the SD multicast port (30490) and unicast ports across tests, so
  parallel execution flakes; CI uses cargo-nextest which serializes.
  This is pre-existing behaviour on `main`, not introduced by this
  branch.
- `cargo test -p simple-someip-embassy-net --tests` — 3/3
- `SIMPLE_SOMEIP_TEST_INTERFACE=127.0.0.1 cargo test --features
  client-tokio,server-tokio --test vsomeip_sd_compat
  tx_announcement_loop_emits_wire_format_offer -- --ignored` — pass
  on a multicast-enabled `lo`
- `cargo doc --no-deps` partial-feature subsets {client | server,bare_metal | --all-features} — zero warnings
- `cargo build --release --target thumbv7em-none-eabihf` size_probe (standalone workspace)

Branch summary: phase 20 cleanup closes the 73-item punch list
spanning 3 critical / 18 high / 22 medium / 30 low items in 7
commits. See CHANGELOG `[Unreleased]` section for the full
changes-by-area breakdown.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 30, 2026 02:06
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

@JustinKovacich JustinKovacich changed the title Overall Bare Metal cleanup Full Bare Metal Support Apr 30, 2026
@JustinKovacich JustinKovacich changed the base branch from main to feature/phase17_cleanup April 30, 2026 02:16
@JustinKovacich JustinKovacich requested a review from Copilot April 30, 2026 02:16
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 47 out of 50 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread examples/embassy_net_client/src/main.rs Outdated
Coverage pass: cover every executable code path THIS branch
introduced. Rubric is "everything new in this branch is covered" —
not "boost overall %". Pre-existing untested branches in
client/socket_manager, client/inner, etc. are out of scope.

## New tests (+14 lib, +2 adapter, +2 doctests)

`src/server/sd_state.rs`: 5 mock-socket tests for
`SdStateManager::send_offer_service`. Previously only exercised by
`#[ignore]`'d multicast tests. New `CapturingSocket` /
`FailingSocket` `TransportSocket` impls capture send_to bytes
without touching a real network. Asserts full SOME/IP+SD envelope
shape, session-id advancement, wrap-flag transition (0xFFFE →
0xFFFF → 0x0001 with reboot flip), TTL=0 round-trip, and error
propagation when the socket fails. Lifts file from 43% → 55% line.

`src/server/mod.rs`: 7 tests for `new_with_handles` /
`new_passive_with_handles` (the MED-22 validation logic added in
this branch had zero coverage before). `build_test_handles` helper
constructs a real `ServerHandles` over `TokioTransport` with
ephemeral ports. Tests cover: back-fill on `local_port = 0`, accept
matching port, reject mismatch (both active and passive variants),
plus passive `run_with_buffers` / `announcement_loop`
short-circuits returning `InvalidUsage`. Lifts file from 80.6% →
84.4% line.

`src/tokio_transport.rs`: 2 tests for `PanicLoggingFut` (new in
this branch). Verifies (a) normal completion passes through the
catch_unwind Ok arm and (b) a panicking inner future is caught,
logged, and resolved cleanly without crashing the runtime — the
panic-Err arm. Both arms now have coverage counts.

`simple-someip-embassy-net/tests/loopback.rs`: 2 tests for the
new `EmbassyNetFactory::bind` paths.
`factory_bind_returns_address_in_use_when_pool_exhausted` covers
the pool-exhausted fallback. `factory_bind_accepts_wildcard_ip`
covers the `addr.ip().is_unspecified()` branch that translates
`0.0.0.0` → embassy-net's `addr: None` mode.

`simple-someip-embassy-net/src/factory.rs`: 2 `compile_fail`
doctests on `EmbassyNetFactory` that verify the type stays
`!Send + !Sync` at the type level. Locks in the
`PhantomData<*const ()>` marker against any future change that
would accidentally re-impose thread-safety. (Copilot incorrectly
flagged the marker as a no-op; the compile_fail doctests are now
authoritative.)

## Adjacent fixes

- **CI doc gate**: two `[Error::Io]` intra-doc links in
  `src/server/mod.rs` referenced a removed enum variant; replaced
  with `[Error::InvalidUsage]` to match the actual error type.
- **CHANGELOG line 29**: "RecentlyRebooted flipping to Continuous
  on the second" was wrong — the flag stays RecentlyRebooted until
  session-counter wrap (~65k announcements). Reworded to match
  the actual test assertion + sd_state semantics.
- **`tests/vsomeip_sd_compat.rs:646`**: same wording bug in the
  test body comment.
- **`tests/vsomeip_sd_compat.rs:775`**: "first run, none in the
  second" was ambiguous (the test ALSO has first/second
  announcements). Rewrote to clarify it's the first/second
  *options-runs* of the SD spec.
- **`Cargo.toml:17`**: `cargo build -p size_probe` no longer
  resolves now that the probe is excluded from the workspace.
  Updated the build instruction to use the standalone manifest.
- **`StaticSocketHandle` doc-rot**: 4 references to the trait alias
  collapsed into `SharedHandle<T>` in phase 19f / 20e
  (`examples/embassy_net_client/src/main.rs:24`,
  `src/server/mod.rs:141,224,373`). Rewrote to reference the
  `&'static T` shape and the blanket impl.

## Verified

- `cargo fmt --all --check`
- `cargo clippy --workspace --all-features -D warnings -D clippy::pedantic`
- `cargo clippy --no-default-features -D warnings -D clippy::pedantic`
- `cargo clippy -p simple-someip --no-default-features --features {client,bare_metal | server,bare_metal | client,server,bare_metal} -D warnings -D clippy::pedantic`
- `RUSTDOCFLAGS=-Dwarnings cargo doc --no-deps --no-default-features --features client` (the gate that blocked CI)
- `RUSTDOCFLAGS=-Dwarnings cargo doc --no-deps --no-default-features --features server,bare_metal`
- `RUSTDOCFLAGS=-Dwarnings cargo doc --no-deps --all-features`
- `cargo test --no-default-features` 4 doc tests pass
- `cargo test --features client-tokio,server-tokio --tests --test-threads=1` —
  492 lib (478 + 14 new) + 11 client_server + 1 bare_metal_e2e + 3
  ignored vsomeip — all green
- `cargo test -p simple-someip-embassy-net --tests` 5/5 (was 3, +2 new)
- `cargo test -p simple-someip-embassy-net --doc` 2/2 (the new
  compile_fail Send/Sync assertions)

Total workspace coverage: 90.32% line / 93.57% function — line
coverage up from 89.12% baseline.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@JustinKovacich JustinKovacich marked this pull request as ready for review April 30, 2026 02:52
@JustinKovacich JustinKovacich changed the base branch from feature/phase17_cleanup to main April 30, 2026 02:52
Audit of the 16 tests added in `a6e13d4` surfaced two genuinely
defective tests + several weak assertions. All fixed.

## HIGH severity

`panic_logging_fut_catches_panic_and_resolves_cleanly` and
`panic_logging_fut_passes_through_normal_completion` were
spawner-integration tests that would have passed even if
`PanicLoggingFut` did nothing — tokio's default behaviour in
`current_thread` mode already absorbs task panics, and a healthy
async block runs whether or not the wrapper is in the path. False
confidence.

Rewrote both as direct unit tests against `PanicLoggingFut::poll`
itself, polling the wrapper with a no-op `Waker`:

- normal-completion test: wraps a future that bumps an
  `AtomicUsize` poll-counter, asserts `Ready(())` AND that the
  inner future was polled exactly once. A regression that
  bypassed `inner.poll` would fail the counter check.
- catches-panic test: wraps a future that panics on first poll,
  manually polls the wrapper, asserts `Ready(())`. A regression
  that removed `catch_unwind` would unwind out of `poll` and
  abort the test (failing it).

Added a third test, `tokio_spawner_isolates_panicking_tasks_from_runtime`,
which stays at the spawner-integration layer but bounds itself
with `tokio::time::timeout(1s, ...)` and verifies the
behavioural difference end-to-end: a panicking spawned task must
not prevent a subsequently-spawned healthy task from running.
With `PanicLoggingFut` the runtime stays alive and the healthy
task completes; without it, tokio's default already handles the
panic, so this test is a smoke test rather than a strong gate —
but combined with the unit tests above it covers both layers.

## MEDIUM severity

`send_offer_service_propagates_socket_errors` used `Err(_)` which
matched any error including unrelated regressions. Narrowed to
`Err(Error::Transport(TransportError::Io(IoErrorKind::NetworkUnreachable)))`
so it specifically asserts the propagated error from
`FailingSocket::send_to` rather than catching any random failure
mode.

`new_with_handles_back_fills_local_port_on_zero`: added an
`assert_ne!(bound_port, 0, ...)` precondition so the test can't
silently pass on a degenerate kernel-allocated port of 0.

`new_with_handles_rejects_local_port_mismatch` and the passive
counterpart used a clever-but-confusing `if bound_port == 1 { 2 }
else { 1 }` to pick a "bogus port" that wouldn't match. Replaced
with `bound_port.wrapping_add(1)` — deterministic, no privileged-
port mythology, and `assert_ne!` makes the distinctness explicit.

## LOW severity

Renamed the five `*_via_mock` sd_state tests to
`send_offer_service_through_mock_*` so the prefix groups them as
a unit. Added a section comment explaining why both the mock
tests AND the `#[ignore]`'d multicast tests below remain: the
mocks cover the encoding/framing path; the multicast tests
exercise the kernel-multicast `socket.send_to` that the mocks
can't observe. Don't delete one in favour of the other.

## Mislabel correction

The `passive_server_run_with_buffers_returns_invalid_usage` and
`passive_server_announcement_loop_returns_invalid_usage` tests
were claimed in `a6e13d4`'s commit message as covering "new code
this branch added." They don't — the `is_passive` short-circuits
in those methods predate the cleanup. They're still valuable
(they pin specific `InvalidUsage` tag strings that the
`new_with_handles` constructor relies on), so they remain. This
commit message corrects the scope claim.

## Verified

- `cargo fmt --all --check`
- `cargo clippy --workspace --all-features -D warnings -D clippy::pedantic`
- `cargo clippy --no-default-features -D warnings -D clippy::pedantic`
- `RUSTDOCFLAGS=-Dwarnings cargo doc --no-deps --no-default-features --features client`
- `cargo test --features client-tokio,server-tokio --tests --test-threads=1` —
  493 lib (was 492, +1 from `tokio_spawner_isolates`) + 11 + 1 + 3 ignored
- 15 impacted tests rerun individually, all green

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 30, 2026 03:12
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

The comment on `embassy-net = "0.4"` claimed the pin avoided a
parallel-version cargo resolution by keeping embassy-sync at 0.6.
That's wrong: embassy-net 0.4.0 itself depends on embassy-sync
0.5.x, so the resolved `Cargo.lock` already carries both 0.5.0
and 0.6.2 in parallel.

Updated the comment to reflect what's actually happening: the
parallel-version split exists today, we accept the binary-size
cost because newer embassy-net releases would widen it further,
and unifying on a single embassy-sync version is a future
coordinated-bump phase across embassy-{sync,net,executor,time}.

Caught by Copilot review on PR #113.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 30, 2026

Codecov Report

❌ Patch coverage is 82.98137% with 685 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/server/sd_state.rs 56.77% 204 Missing ⚠️
src/static_channels/mod.rs 82.47% 116 Missing ⚠️
src/client/socket_manager.rs 76.76% 112 Missing ⚠️
src/client/mod.rs 80.85% 94 Missing ⚠️
src/transport.rs 85.76% 40 Missing ⚠️
src/server/event_publisher.rs 89.55% 35 Missing ⚠️
src/embassy_channels.rs 90.02% 34 Missing ⚠️
src/client/bind_dispatch.rs 50.00% 28 Missing ⚠️
src/tokio_transport.rs 94.46% 19 Missing ⚠️
src/server/subscription_manager.rs 99.36% 2 Missing ⚠️
... and 1 more

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
- Coverage   96.55%   91.59%   -4.97%     
==========================================
  Files          30       37       +7     
  Lines        7993    12486    +4493     
==========================================
+ Hits         7718    11437    +3719     
- Misses        275     1049     +774     
Files with missing lines Coverage Δ
src/client/error.rs 100.00% <100.00%> (ø)
src/client/inner.rs 93.97% <ø> (+1.97%) ⬆️
src/client/service_registry.rs 100.00% <100.00%> (ø)
src/client/session.rs 100.00% <100.00%> (ø)
src/e2e/crc.rs 96.15% <100.00%> (ø)
src/e2e/e2e_checker.rs 99.49% <100.00%> (ø)
src/e2e/e2e_protector.rs 100.00% <100.00%> (ø)
src/e2e/mod.rs 97.19% <100.00%> (ø)
src/e2e/registry.rs 100.00% <100.00%> (ø)
src/protocol/byte_order.rs 98.51% <ø> (ø)
... and 20 more

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.

3 participants