diff --git a/CHANGELOG.md b/CHANGELOG.md index 7818775f..3572486a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -120,6 +120,43 @@ let (_server, _handles, run) = Server::new(config).await?; tokio::spawn(run); // receive only; co-located Client drives SD ``` +#### Breaking — `NonSdRequestCallback` is now a parsed, ctx-carrying callback + +```rust +// before +pub type NonSdRequestCallback = fn(data: &[u8], source: SocketAddrV4); +// after +pub type NonSdRequestCallback = fn( + ctx: usize, + source: core::net::SocketAddrV4, + service_id: u16, + method_id: u16, + payload: &[u8], + e2e_status: u8, +); +``` + +The observer is now registered as a `(callback, ctx)` pair +(`ServerDeps::non_sd_observer: Option<(NonSdRequestCallback, usize)>`), +and `ctx` is passed back verbatim on every invocation. `recv_loop` +parses the SOME/IP header so consumers receive decoded +`(service_id, method_id, payload)` and never hand-roll parsing — the +parse stays in the audited crate per MISRA/ASIL rather than being +replicated in N C consumers. `payload` is the bytes after the 16-byte +header. `e2e_status` is `0` (unchecked) — server-side request E2E is +not applied today. `usize` (rather than a stored `*mut c_void`) keeps +`Server: Send` — and therefore `Server::run`'s declared `+ Send` bound +— with no `unsafe` in this crate; the pointer cast lives in the +consumer's callback body. FFI consumers stash their state pointer as +`usize`; pure-Rust callers pass `0`. + +##### Migration + +`non_sd_observer: Some(my_cb)` → `non_sd_observer: Some((my_cb, 0))`, +and replace the leading `ctx: usize` parameter plus manual header parsing +with the decoded arguments `(ctx, source, service_id, method_id, payload, +e2e_status)`. Registration becomes `Some((my_cb, 0))`. + #### Removed - **`Server::announcement_loop` / `Server::announcement_loop_local`** — folded into the combined run-future. The `announcement_loop_started: AtomicBool` latch that protected against two simultaneously-driven announcement futures is gone with them (single entry point makes the failure mode structurally impossible). diff --git a/docs/simple_someip/plans/2026-06-10-pr1-124-followups-design.md b/docs/simple_someip/plans/2026-06-10-pr1-124-followups-design.md new file mode 100644 index 00000000..cda70430 --- /dev/null +++ b/docs/simple_someip/plans/2026-06-10-pr1-124-followups-design.md @@ -0,0 +1,162 @@ +# PR 1 — #124 follow-ups: design + +Second PR of the #125 / phase-22 close-out stack +(`2026-06-09-phase22-125-memory-reduction-design.md`, "PR 1" section). +Closes the four review findings recorded there against PR #124, plus +one stale-doc item surfaced during PR 0. + +**Branch:** `feature/pr1_124_followups` off +`feature/pr0_measurement_harness` (PR base = the PR 0 branch). PR 1 +must stack on PR 0: changing the callback shape touches `ServerDeps` +initializers in test files PR 0 modified +(`tests/bare_metal_e2e.rs`, `tests/bare_metal_server.rs`). + +**External gate:** Feliciano gets the callback-signature heads-up +before this merges, so no further halo FFI builds on the bare `fn` +shape (stack-plan gate 2 — user action, still open as of +2026-06-10). + +## 1. `NonSdRequestCallback` gains a context argument (BREAKING) + +Decision (2026-06-10): **`ctx: usize`**, over an unsafe-Send +`*mut c_void` newtype and over a generic observer type parameter. + +Revised 2026-06-11: a parallel branch (`feat/embassy-mem-channel-cap`) +independently reshaped the callback to a library-parsed +`(service_id, method_id, payload, e2e_status)` contract. The +reconciled contract is the union of both — ctx + source + decoded +fields — keeping parse/E2E in the audited crate (MISRA/ASIL) rather +than in N C consumers. `e2e_status` is 0 (unchecked) on the server +path today; `source` is future-proofing (unused by halo and dft). + +```rust +pub type NonSdRequestCallback = fn( + ctx: usize, + source: core::net::SocketAddrV4, + service_id: u16, + method_id: u16, + payload: &[u8], + e2e_status: u8, +); +``` + +- Storage everywhere becomes `Option<(NonSdRequestCallback, usize)>` + — a plain tuple, no wrapper struct: the `Server` field, + `ServerDeps.non_sd_observer`, and + `ServerDeps::with_non_sd_observer`. `recv_loop` threads the pair + down and invokes `cb(ctx, data, src)`. +- Rationale (recorded on the type alias's doc comment): a stored + `*mut c_void` makes `Server` `!Send` and breaks `Server::run`'s + declared `+ Send` bound; `usize` is trivially `Send + Sync`, + keeps the field `Copy`, and matches the `uintptr_t` the C caller + holds anyway. halo passes `(dispatch, state_ptr as usize)`; + Rust-native users pass `(f, 0)`. +- **No `unsafe` enters this crate.** The library stores, copies, and + passes back a plain integer through a safe `fn`-pointer call — + `Server: Send` holds by construction, with no `unsafe impl` and no + soundness contract for the library to document or uphold. The + unsafe dereference (`ctx as *mut T`, then `unsafe { &*ptr }`) + happens in the consumer's callback body — halo's FFI dispatch + code, which is already unsafe territory and is the only party + that can verify the pointee's lifetime and thread-safety. Known + trade-off: `usize` carries no provenance, so the compiler can't + stop a caller passing a wrong address — but the rejected + `*mut c_void` newtype was equally untyped; only the + generic-observer design would have fixed that, at the + 8th-type-parameter cost. +- Rejected alternatives, for the record: the unsafe-Send newtype + moves an unverifiable soundness contract into this library; the + generic observer adds an 8th type parameter that ripples through + `ServerDeps`/`ServerHandles`/both cfg-switched alias families, + and halo's FFI would still write its own unsafe-Send wrapper. +- Breaking now is free: 0.8.0 is unpublished and halo is the only + consumer. CHANGELOG gets a breaking-change entry with the + before/after signature. + +## 2. Eager-`Ready` timing documented (no code change) + +Decision (2026-06-10): document, don't make lazy. +`StaticSubscriptionHandle::subscribe`/`unsubscribe` execute the +locked mutation when the future is *constructed* (inside +`core::future::ready(...)`), unlike the `Box::pin(async)` impls, +which are lazy. The only in-tree caller (`runtime.rs`) awaits +immediately, so laziness would be ~80 lines of poll boilerplate for +behavior no current caller observes. + +- Trait-level note on `SubscriptionHandle::subscribe`/`unsubscribe`: + implementations with a fully synchronous critical section may + perform the mutation at future construction; callers must not + assume construction is side-effect-free. +- Matching sentence on the `StaticSubscriptionHandle` impl block. +- The phase-22 preflight patch's hand-written `StaticSubscribeFuture` + (`.claude/phase22_item5_preflight.patch` in the main worktree) is + permanently obsolete. + +## 3. `announce_only_future` rationale (doc-only) + +The method's doc already explains the shared-socket topology +(supplementary Servers via `new_with_handles` announce on the shared +SD socket; the primary owns all inbound loops). It gains the honest +acknowledgment that this partially reintroduces the split-future +shape phase 21 removed, and why that is acceptable here: an +announce-only future never touches the recv path, so the +single-run-future invariant that motivated phase 21b (no two futures +racing on the same sockets/session counter) is preserved — the +`started` latch still guards the full run-future. + +The originally-planned MSRV check is recorded as moot: the crate is +edition 2024 (Rust ≥ 1.85); `use<>` precise capture needs only 1.82. + +## 4. Strengthen the non-SD-observer negative test + +`non_sd_observer_none_preserves_ignore_behavior` +(`tests/bare_metal_server.rs`) cannot currently fail: `record_none` +is never wired into the server, so no code path can populate +`OBSERVED_NONE` regardless of any routing regression. + +Replace with negative tests that have a live witness — register the +recording callback as a real observer, then assert it does NOT fire +for: + +- (i) an SD unicast datagram (exercises the SD-vs-non-SD routing + branch), and +- (ii) a non-SD **multicast** datagram (exercises the + unicast-vs-multicast branch — the mock needs to mark the datagram + as arriving on the SD/multicast socket; mechanics resolved in the + implementation plan). + +The `None` case shrinks to what it actually proves: the run loop +processes a non-SD unicast datagram without panicking when no +observer is registered. The positive test +(`non_sd_observer_some_receives_unicast_method_request`) is updated +for the new signature and asserts the ctx value round-trips. + +## 5. Stale `UDP_BUFFER_SIZE` rustdoc + +Verified false during PR 0 review (2026-06-10): `send_ack` +(`src/server/runtime.rs`) builds into a stack +`[0u8; crate::UDP_BUFFER_SIZE]`, and the `server,bare_metal` rlib +audits to zero allocator symbols. The Vec-to-stack conversion was +the phase-21 per-event-allocation cleanup (`7c58649`, PR #114 +stack), not #124 — the rustdoc claim was stale even before #124, +which never touched those paths. The constant's rustdoc paragraph +claiming announcement builders / `SubscribeAck`/`Nack` "still use +heap `Vec` buffers — known gap" is rewritten: all outbound SD paths +are stack-buffered and capped by `UDP_BUFFER_SIZE`. + +## Error handling + +No new fallible paths. The callback invocation remains +fire-and-forget from `recv_loop` (a misbehaving callback is the +consumer's responsibility — same contract as today, restated in the +type-alias docs). + +## Verification + +fmt, clippy `--workspace --all-features` + `--no-default-features` +(both `-D warnings -D clippy::pedantic`), full suite at +`--test-threads=1`, doc tests, the three `-Zbuild-std=core` thumb +builds, and the `nm` server audit — the latter two also enforced in +CI since PR 0. Future-size witnesses from PR 0 must stay within +budget (the tuple adds 2×usize to the run-future capture; budgets +have 25% headroom). diff --git a/docs/simple_someip/plans/2026-06-10-pr1-124-followups.md b/docs/simple_someip/plans/2026-06-10-pr1-124-followups.md new file mode 100644 index 00000000..8a42ed2f --- /dev/null +++ b/docs/simple_someip/plans/2026-06-10-pr1-124-followups.md @@ -0,0 +1,869 @@ +# PR 1 — #124 Follow-ups Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Land the four #124 review follow-ups — ctx-carrying `NonSdRequestCallback` (breaking), eager-`Ready` timing docs, `announce_only_future` rationale, live-witness negative tests — plus the stale `UDP_BUFFER_SIZE` rustdoc fix. + +**Architecture:** One breaking signature change (`fn(data, src)` → `fn(ctx: usize, data, src)`, stored as `Option<(NonSdRequestCallback, usize)>` on `ServerDeps`/`ServerHandles`/`Server` and threaded through `recv_loop`); a mock-transport split in `tests/bare_metal_server.rs` (per-socket pipes routed by `multicast_if_v4`, making `from_unicast` deterministic); the rest is documentation and CHANGELOG. + +**Tech Stack:** Rust (stable; nightly only for the final `-Zbuild-std=core` verification), cargo, GNU `nm`. + +**Spec:** `docs/simple_someip/plans/2026-06-10-pr1-124-followups-design.md` + +**Working tree:** the `simple_someip-pr0` worktree, branch `feature/pr1_124_followups` (created off `feature/pr0_measurement_harness` at the design commit). PR base = `feature/pr0_measurement_harness`. + +--- + +### Task 1: Verify preconditions + +**Files:** none (git only) + +- [ ] **Step 1: Confirm branch and clean tree** + +Run: `git branch --show-current && git status --short` +Expected: `feature/pr1_124_followups`, no output from status (clean tree; the design doc commit `da3e02c` or later is HEAD). + +- [ ] **Step 2: Confirm the baseline is green** + +Run: `cargo test --features server-tokio,client-tokio,bare_metal --tests --lib -- --test-threads=1 2>&1 | grep -E "^test result" | grep -v "0 failed" || echo ALL_GREEN` +Expected: `ALL_GREEN` + +--- + +### Task 2: ctx argument on `NonSdRequestCallback` (breaking) + +Red first: rewrite the test-side callbacks and the positive test to the NEW signature (compile failure is the failing test), then change the library, then green. + +**Files:** +- Modify: `tests/bare_metal_server.rs:365-385` (statics + record fns), `:416-492` (positive test) +- Modify: `src/server/mod.rs:645` (type alias), `:285-293` (ServerDeps field), `:409-417` (builder), `:519-521` (ServerHandles field), `:629-634` (Server field) +- Modify: `src/server/runtime.rs:443`, `:588` (params), `:543-545` (invocation) + +- [ ] **Step 1: Update the test-side statics and record fns to the new shape** + +Replace the two statics and two fns at `tests/bare_metal_server.rs:367-379` with: + +```rust +static OBSERVED_SOME: OnceLock, SocketAddrV4)>>> = OnceLock::new(); + +fn record_some(ctx: usize, data: &[u8], source: SocketAddrV4) { + let slot = OBSERVED_SOME.get_or_init(|| Mutex::new(None)); + *slot.lock().unwrap() = Some((ctx, data.to_vec(), source)); +} +``` + +(`OBSERVED_NONE` / `record_none` are deleted here; Task 4 replaces the test that used them. If the file temporarily fails to compile because the old negative test still references them, stub the references by deleting the body of `non_sd_observer_none_preserves_ignore_behavior` down to `let _ = ();` — Task 4 rewrites it entirely.) + +- [ ] **Step 2: Update the positive test to the new signature + ctx round-trip** + +In `non_sd_observer_some_receives_unicast_method_request`: + +```rust + non_sd_observer: Some((record_some as NonSdRequestCallback, 0xC0FF_EE00)), +``` + +and replace the result-destructuring + asserts at the end with: + +```rust + let (got_ctx, got_data, got_src) = OBSERVED_SOME + .get() + .unwrap() + .lock() + .unwrap() + .clone() + .expect("callback fired"); + assert_eq!( + got_ctx, 0xC0FF_EE00, + "callback must receive the registered ctx word verbatim" + ); + assert_eq!( + got_data, payload, + "callback must receive the full raw datagram bytes" + ); + assert_eq!(got_src, src, "callback must receive the original source"); +``` + +- [ ] **Step 3: Verify it fails to compile (red)** + +Run: `cargo test --no-run --features server,bare_metal --test bare_metal_server 2>&1 | tail -5` +Expected: FAIL — type mismatch (`Option` vs the tuple) — the library hasn't changed yet. + +- [ ] **Step 4: Change the type alias** + +`src/server/mod.rs:636-645` — replace the alias and its doc: + +```rust +/// Callback invoked by the server's `recv_loop` for every non-SD +/// unicast datagram received on the service's port (i.e. method +/// requests / fire-and-forget calls to the offered services). The +/// payload is the full raw datagram bytes; the caller is responsible +/// for re-parsing the SOME/IP header (and applying any E2E check) on +/// the consumer side. +/// +/// `ctx` is an opaque caller-owned context word, registered alongside +/// the callback as a `(NonSdRequestCallback, usize)` pair and passed +/// back verbatim on every invocation. It is deliberately `usize` +/// rather than `*mut c_void`: a stored raw pointer would make +/// [`Server`] `!Send` and break [`Server::run`]'s declared `+ Send` +/// bound, while `usize` is trivially `Send + Sync` and matches the +/// `uintptr_t` an FFI caller holds anyway. No `unsafe` enters this +/// crate — the cast back to a pointer (and its safety justification) +/// lives in the consumer's callback body, the only place that knows +/// the pointee's lifetime and thread-safety. Rust-native users that +/// need no context pass `0`. `fn` pointers are +/// `Copy + Send + Sync + 'static`, so the pair can be stored on the +/// `Server` and captured by the run-future without adding a new +/// generic. +pub type NonSdRequestCallback = fn(ctx: usize, data: &[u8], source: core::net::SocketAddrV4); +``` + +- [ ] **Step 5: Change the three struct fields and the builder** + +`src/server/mod.rs:287-293` (`ServerDeps` field — replace doc + type): + +```rust + /// Optional `(callback, ctx)` pair invoked from the server's receive + /// loop for every non-SD **unicast** datagram (method requests / + /// fire-and-forget calls to offered services). `None` reproduces the + /// historical "non-SD ignored" behavior. The callback receives the + /// opaque `ctx` word back verbatim, plus the full raw datagram bytes + /// and the source `SocketAddrV4`; the consumer is responsible for + /// re-parsing the SOME/IP header and any E2E check. + pub non_sd_observer: Option<(NonSdRequestCallback, usize)>, +``` + +`src/server/mod.rs:409-417` (`ServerDeps::with_non_sd_observer`): + +```rust + /// Register a `(callback, ctx)` pair invoked for every non-SD unicast + /// datagram (method requests / fire-and-forget calls to offered + /// services). The opaque `ctx` word is passed back verbatim on every + /// invocation — FFI callers stash a pointer here as `usize`; + /// pure-Rust callers that need no context pass `0`. Passing `None` + /// (the default if unset) preserves the historical "ignore non-SD" + /// behavior. + #[must_use] + pub fn with_non_sd_observer( + mut self, + observer: Option<(NonSdRequestCallback, usize)>, + ) -> Self { + self.non_sd_observer = observer; + self + } +``` + +`src/server/mod.rs:519-521` (`ServerHandles` field): + +```rust + /// Optional `(callback, ctx)` pair for non-SD unicast datagrams + /// (method requests). `None` reproduces the default "non-SD + /// ignored" behavior. + pub non_sd_observer: Option<(NonSdRequestCallback, usize)>, +``` + +`src/server/mod.rs:629-634` (`Server` field — keep the existing doc sentence about halo's HWP1 dispatch, change the type): + +```rust + non_sd_observer: Option<(NonSdRequestCallback, usize)>, +``` + +All `non_sd_observer: None` initializers and the struct-update copies (`non_sd_observer: self.non_sd_observer`, `non_sd_observer: deps_non_sd_observer`, …) compile unchanged — do not touch them. + +- [ ] **Step 6: Thread the pair through `recv_loop`** + +`src/server/runtime.rs:443` and `:588` — both parameter declarations become: + +```rust + non_sd_observer: Option<(super::NonSdRequestCallback, usize)>, +``` + +`src/server/runtime.rs:543-545` — the invocation becomes: + +```rust + if let Some((cb, ctx)) = non_sd_observer { + if let core::net::SocketAddr::V4(src_v4) = addr { + cb(ctx, data, src_v4); + } +``` + +- [ ] **Step 7: Verify green** + +Run: `cargo test --features server-tokio,client-tokio,bare_metal --tests --lib -- --test-threads=1 2>&1 | grep -E "^test result"` +Expected: all `ok`, 0 failed (the gutted negative test passes vacuously until Task 4). + +- [ ] **Step 8: Commit** + +```bash +git add src/server/mod.rs src/server/runtime.rs tests/bare_metal_server.rs +git commit -m "feat(server)!: NonSdRequestCallback gains an opaque ctx:usize argument + +Stored as (callback, ctx) on ServerDeps/ServerHandles/Server and +passed back verbatim from recv_loop. usize over *mut c_void keeps +Server: Send (run's declared + Send bound survives) with no unsafe +in this crate; the pointer cast lives in the consumer's callback. + +Co-Authored-By: Claude Fable 5 " +``` + +--- + +### Task 3: Per-socket mock pipes in `tests/bare_metal_server.rs` + +Both mock sockets currently pop the SAME `inbound` queue, so which select +arm wins (and therefore `from_unicast`) depends on the alternating +`select_biased!` bias — latent nondeterminism, and a blocker for Task 4's +multicast test. Route by `SocketOptions`: the server binds its SD socket +with `multicast_if_v4 = Some(..)` (`src/server/mod.rs:880`), the unicast +socket with plain `SocketOptions::new()`. + +**Files:** +- Modify: `tests/bare_metal_server.rs:53-77` (factory), all four test constructors + +- [ ] **Step 1: Split the factory** + +Replace `MockFactory` and its `bind`: + +```rust +#[derive(Clone)] +struct MockFactory { + /// Handed to sockets bound WITHOUT multicast options — the + /// server's unicast service socket. + unicast_pipe: Arc, + /// Handed to sockets bound WITH `multicast_if_v4` set — the + /// server's SD socket. Per-socket queues make `recv_loop`'s + /// `from_unicast` flag deterministic: with a single shared queue, + /// whichever select arm polled first stole the datagram, so + /// routing depended on the alternating `select_biased!` bias. + sd_pipe: Arc, + next_port: Arc>, +} + +impl TransportFactory for MockFactory { + type Socket = MockSocket; + type BindFuture<'a> = + core::pin::Pin> + Send + 'a>>; + fn bind<'a>(&'a self, addr: SocketAddrV4, options: &'a SocketOptions) -> Self::BindFuture<'a> { + let pipe = if options.multicast_if_v4.is_some() { + Arc::clone(&self.sd_pipe) + } else { + Arc::clone(&self.unicast_pipe) + }; + // Mock: assign port deterministically. If caller asked for 0, + // hand out an incrementing fake ephemeral port. + let port = if addr.port() == 0 { + let mut p = self.next_port.lock().unwrap(); + let next = *p + 1; + *p = next; + 40000 + next + } else { + addr.port() + }; + let local = SocketAddrV4::new(*addr.ip(), port); + Box::pin(async move { Ok(MockSocket { pipe, local }) }) + } +} +``` + +- [ ] **Step 2: Update every factory construction site** + +In all four tests (`server_constructible_without_server_tokio_feature`, +`passive_server_constructible_without_server_tokio_feature`, +`non_sd_observer_some_receives_unicast_method_request`, and the gutted +negative test), replace: + +```rust + let pipe = Arc::new(MockPipe::default()); + let factory = MockFactory { + pipe: Arc::clone(&pipe), + next_port: Arc::new(Mutex::new(0)), + }; +``` + +with: + +```rust + let unicast_pipe = Arc::new(MockPipe::default()); + let sd_pipe = Arc::new(MockPipe::default()); + let factory = MockFactory { + unicast_pipe: Arc::clone(&unicast_pipe), + sd_pipe: Arc::clone(&sd_pipe), + next_port: Arc::new(Mutex::new(0)), + }; +``` + +In the positive test, the datagram pushes change `pipe.` → `unicast_pipe.` +(both the `inbound` push and the `inbound_waker` wake), and DELETE the +now-false comment block above the push ("Queue a non-SD unicast … relying +on the `select_biased!`'s prefer-unicast tick…") — replace it with: + +```rust + // Queue a non-SD unicast method-request datagram on the unicast + // socket's own pipe; per-socket pipes make `from_unicast = true` + // deterministic. +``` + +- [ ] **Step 3: Verify green** + +Run: `cargo test --features server,bare_metal --test bare_metal_server -- --test-threads=1 2>&1 | grep -E "^test result"` +Expected: `ok`, 0 failed. + +- [ ] **Step 4: Commit** + +```bash +git add tests/bare_metal_server.rs +git commit -m "test(server): per-socket mock pipes routed by multicast_if_v4 + +Removes the shared-queue nondeterminism (from_unicast depended on +select_biased bias order) and enables deterministic SD-socket +injection for the negative observer tests. + +Co-Authored-By: Claude Fable 5 " +``` + +--- + +### Task 4: Live-witness negative observer tests + +The old `non_sd_observer_none_preserves_ignore_behavior` could not fail: +its witness callback was never registered, so no routing regression could +populate `OBSERVED_NONE`. Replace it with two tests whose observer IS +registered and must NOT fire, plus a slim no-panic `None` case. + +**Files:** +- Modify: `tests/bare_metal_server.rs` (new helper + statics, replace the negative test) + +- [ ] **Step 1: Add the SD datagram builder next to `build_method_request`** + +```rust +/// Build a minimal, well-formed SOME/IP-SD datagram: SD message id +/// (0xFFFF / 0x8100), then an SD payload with flags + reserved and +/// ZERO entries / options. Routing-wise a legitimate (if vacuous) SD +/// message — `recv_loop` must hand it to SD handling, never to the +/// non-SD observer, regardless of which socket it arrived on. +fn build_sd_message() -> Vec { + let mut buf = Vec::with_capacity(28); + buf.extend_from_slice(&0xFFFFu16.to_be_bytes()); // message_id (high): SD service + buf.extend_from_slice(&0x8100u16.to_be_bytes()); // message_id (low): SD method + buf.extend_from_slice(&20u32.to_be_bytes()); // length = header(8) + sd payload(12) + buf.extend_from_slice(&0u32.to_be_bytes()); // request_id + buf.push(1); // protocol_version + buf.push(1); // interface_version + buf.push(2); // message_type = Notification (0x02) + buf.push(0); // return_code = OK + buf.push(0x80); // SD flags: reboot + buf.extend_from_slice(&[0, 0, 0]); // reserved + buf.extend_from_slice(&0u32.to_be_bytes()); // entries array length = 0 + buf.extend_from_slice(&0u32.to_be_bytes()); // options array length = 0 + buf +} +``` + +- [ ] **Step 2: Add per-test witness statics + record fns (next to `OBSERVED_SOME`)** + +Separate statics per test — they share the process under parallel `cargo test`. + +```rust +static OBSERVED_SD_UNICAST: OnceLock, SocketAddrV4)>>> = + OnceLock::new(); +static OBSERVED_MULTICAST: OnceLock, SocketAddrV4)>>> = + OnceLock::new(); + +fn record_sd_unicast(ctx: usize, data: &[u8], source: SocketAddrV4) { + let slot = OBSERVED_SD_UNICAST.get_or_init(|| Mutex::new(None)); + *slot.lock().unwrap() = Some((ctx, data.to_vec(), source)); +} + +fn record_multicast(ctx: usize, data: &[u8], source: SocketAddrV4) { + let slot = OBSERVED_MULTICAST.get_or_init(|| Mutex::new(None)); + *slot.lock().unwrap() = Some((ctx, data.to_vec(), source)); +} +``` + +- [ ] **Step 3: Replace the gutted negative test with three tests** + +```rust +/// A registered observer must NOT fire for an SD message arriving on +/// the unicast socket — SD-formatted unicast traffic (e.g. unicast +/// FindService) routes to SD handling. Unlike the pre-PR-1 negative +/// test, the witness callback IS registered, so a routing regression +/// (SD datagrams leaking to the observer) trips the assertion. +#[tokio::test] +async fn non_sd_observer_ignores_sd_message_on_unicast_socket() { + let unicast_pipe = Arc::new(MockPipe::default()); + let sd_pipe = Arc::new(MockPipe::default()); + let factory = MockFactory { + unicast_pipe: Arc::clone(&unicast_pipe), + sd_pipe: Arc::clone(&sd_pipe), + next_port: Arc::new(Mutex::new(0)), + }; + + let e2e_handle: Arc> = Arc::new(Mutex::new(E2ERegistry::new())); + let config = ServerConfig::new(0x1234, 1) + .with_interface(Ipv4Addr::LOCALHOST) + .with_local_port(30702); + + let deps: ServerDeps>, MockSubscriptions> = + ServerDeps { + factory, + timer: MockTimer, + e2e_registry: e2e_handle, + subscriptions: MockSubscriptions::default(), + non_sd_observer: Some((record_sd_unicast as NonSdRequestCallback, 7)), + }; + + let (_server, _handles, run): ( + Server>, MockSubscriptions>, + _, + _, + ) = Server::new_with_deps(deps, config, false) + .await + .expect("Server::new_with_deps must succeed"); + let handle = tokio::spawn(run); + + let src = SocketAddrV4::new(Ipv4Addr::new(192, 0, 2, 102), 40002); + unicast_pipe + .inbound + .lock() + .unwrap() + .push_back((build_sd_message(), src)); + if let Some(w) = unicast_pipe.inbound_waker.lock().unwrap().take() { + w.wake(); + } + + // No positive completion signal exists for "was ignored" — give + // the run-future a generous processing window, then assert. + for _ in 0..50 { + tokio::task::yield_now().await; + } + tokio::time::sleep(Duration::from_millis(10)).await; + + let observed = OBSERVED_SD_UNICAST.get().and_then(|m| m.lock().unwrap().clone()); + assert!( + observed.is_none(), + "observer must NOT fire for SD messages; got {observed:?}" + ); + handle.abort(); + let _ = handle.await; +} + +/// A registered observer must NOT fire for a non-SD datagram arriving +/// on the SD/multicast socket — the observer contract is unicast-only +/// (`from_unicast == true`). +#[tokio::test] +async fn non_sd_observer_ignores_non_sd_on_multicast_socket() { + let unicast_pipe = Arc::new(MockPipe::default()); + let sd_pipe = Arc::new(MockPipe::default()); + let factory = MockFactory { + unicast_pipe: Arc::clone(&unicast_pipe), + sd_pipe: Arc::clone(&sd_pipe), + next_port: Arc::new(Mutex::new(0)), + }; + + let e2e_handle: Arc> = Arc::new(Mutex::new(E2ERegistry::new())); + let config = ServerConfig::new(0x1234, 1) + .with_interface(Ipv4Addr::LOCALHOST) + .with_local_port(30703); + + let deps: ServerDeps>, MockSubscriptions> = + ServerDeps { + factory, + timer: MockTimer, + e2e_registry: e2e_handle, + subscriptions: MockSubscriptions::default(), + non_sd_observer: Some((record_multicast as NonSdRequestCallback, 9)), + }; + + let (_server, _handles, run): ( + Server>, MockSubscriptions>, + _, + _, + ) = Server::new_with_deps(deps, config, false) + .await + .expect("Server::new_with_deps must succeed"); + let handle = tokio::spawn(run); + + let src = SocketAddrV4::new(Ipv4Addr::new(192, 0, 2, 103), 40003); + sd_pipe + .inbound + .lock() + .unwrap() + .push_back((build_method_request(0x1234, 0x0001), src)); + if let Some(w) = sd_pipe.inbound_waker.lock().unwrap().take() { + w.wake(); + } + + for _ in 0..50 { + tokio::task::yield_now().await; + } + tokio::time::sleep(Duration::from_millis(10)).await; + + let observed = OBSERVED_MULTICAST.get().and_then(|m| m.lock().unwrap().clone()); + assert!( + observed.is_none(), + "observer must NOT fire for non-unicast datagrams; got {observed:?}" + ); + handle.abort(); + let _ = handle.await; +} + +/// With `non_sd_observer: None`, a non-SD unicast datagram is processed +/// without panicking (historical "ignore" behavior). This is all the +/// `None` case can actually prove — there is no callback to witness. +#[tokio::test] +async fn non_sd_observer_none_preserves_ignore_behavior() { + let unicast_pipe = Arc::new(MockPipe::default()); + let sd_pipe = Arc::new(MockPipe::default()); + let factory = MockFactory { + unicast_pipe: Arc::clone(&unicast_pipe), + sd_pipe: Arc::clone(&sd_pipe), + next_port: Arc::new(Mutex::new(0)), + }; + + let e2e_handle: Arc> = Arc::new(Mutex::new(E2ERegistry::new())); + let config = ServerConfig::new(0x1234, 1) + .with_interface(Ipv4Addr::LOCALHOST) + .with_local_port(30701); + + let deps: ServerDeps>, MockSubscriptions> = + ServerDeps { + factory, + timer: MockTimer, + e2e_registry: e2e_handle, + subscriptions: MockSubscriptions::default(), + non_sd_observer: None, + }; + + let (_server, _handles, run): ( + Server>, MockSubscriptions>, + _, + _, + ) = Server::new_with_deps(deps, config, false) + .await + .expect("Server::new_with_deps must succeed"); + let handle = tokio::spawn(run); + + let src = SocketAddrV4::new(Ipv4Addr::new(192, 0, 2, 101), 40001); + unicast_pipe + .inbound + .lock() + .unwrap() + .push_back((build_method_request(0x1234, 0x0001), src)); + if let Some(w) = unicast_pipe.inbound_waker.lock().unwrap().take() { + w.wake(); + } + + for _ in 0..50 { + tokio::task::yield_now().await; + } + tokio::time::sleep(Duration::from_millis(10)).await; + + assert!( + !handle.is_finished(), + "run-future must keep running (no panic / no error) after \ + ignoring a non-SD datagram with no observer registered" + ); + handle.abort(); + let _ = handle.await; +} +``` + +- [ ] **Step 4: Run the new tests — verify they pass, then verify they CAN fail** + +Run: `cargo test --features server,bare_metal --test bare_metal_server -- --test-threads=1 2>&1 | grep -E "^test result"` +Expected: `ok`, 0 failed. + +Sanity-check the witness is live: temporarily change `else if from_unicast` +at `src/server/runtime.rs` to `else if true`, rerun +`cargo test --features server,bare_metal --test bare_metal_server non_sd_observer_ignores_non_sd_on_multicast -- --test-threads=1`, +expect FAIL (observer fired); **revert the temporary change** and rerun to +green before committing. + +- [ ] **Step 5: Commit** + +```bash +git add tests/bare_metal_server.rs +git commit -m "test(server): live-witness negative tests for the non-SD observer + +The old None-case test could not fail (its witness was never +registered). Replace with: registered observer must not fire for SD +messages on the unicast socket nor for non-SD datagrams on the SD +socket; the None case shrinks to its real guarantee (no panic). +Refutation-checked by inverting the from_unicast branch. + +Co-Authored-By: Claude Fable 5 " +``` + +--- + +### Task 5: Eager-`Ready` timing docs (spec item 2, doc-only) + +**Files:** +- Modify: `src/server/subscription_manager.rs:321-341` (trait method docs), `:495-502` (impl comment) + +- [ ] **Step 1: Trait method notes** + +On `SubscriptionHandle::subscribe` (after the "Idempotent…" paragraph, before `fn subscribe`): + +```rust + /// Timing note: implementations whose critical section is fully + /// synchronous (e.g. `StaticSubscriptionHandle`) may perform the + /// mutation when the future is *constructed*, deferring only the + /// result delivery to the poll. Callers must not assume that + /// constructing the returned future is free of side effects. +``` + +On `unsubscribe` (after "Remove a subscriber from an event group."): + +```rust + /// Same construction-time-mutation caveat as [`Self::subscribe`]. +``` + +- [ ] **Step 2: Impl-side sentence** + +In the comment block above `type SubscribeFuture` in +`impl SubscriptionHandle for StaticSubscriptionHandle` +(`src/server/subscription_manager.rs:496-502`), append after +"…satisfying any `Send`-checked run path.": + +```rust + // Consequence (documented on the trait): the mutation runs + // eagerly at future construction; only the result is delivered + // through the poll. The in-tree caller awaits immediately, so + // the difference from the lazy boxed impls is unobservable + // there. +``` + +- [ ] **Step 3: Verify docs build + commit** + +Run: `cargo doc --no-deps --features server,bare_metal 2>&1 | tail -2` — expect `Finished`. + +```bash +git add src/server/subscription_manager.rs +git commit -m "docs(server): document eager-at-construction timing of Ready-based subscribe + +Co-Authored-By: Claude Fable 5 " +``` + +--- + +### Task 6: `announce_only_future` rationale (spec item 3, doc-only) + +**Files:** +- Modify: `src/server/mod.rs:1286-1296` (doc comment) + +- [ ] **Step 1: Append to the doc comment** + +After "…competing for inbound datagrams." and before "The returned future +loops forever…", insert: + +```rust + /// Design note: this partially reintroduces the split-future shape + /// phase 21 removed — deliberately. An announce-only future never + /// touches the receive path, so the invariant that motivated the + /// phase-21 combined run-future (no two futures racing the same + /// sockets and SD session counter) is preserved: the [`Self::run`] + /// path is still guarded by the first-poll `started` latch, and + /// supplementary announce loops only ever *send* on the shared SD + /// socket. + /// +``` + +- [ ] **Step 2: Verify + commit** + +Run: `cargo doc --no-deps --features server,bare_metal 2>&1 | tail -2` — expect `Finished`. + +```bash +git add src/server/mod.rs +git commit -m "docs(server): record why announce_only_future may split the run shape + +Co-Authored-By: Claude Fable 5 " +``` + +--- + +### Task 7: `UDP_BUFFER_SIZE` rustdoc trues-up (spec item 5) + +**Files:** +- Modify: `src/lib.rs:145-150` + +- [ ] **Step 1: Confirm the claim is stale before editing** + +Run: `grep -rn "alloc::vec\|Vec::with_capacity\|vec!" src/server/runtime.rs src/protocol/sd/ --include="*.rs" | grep -v test` +Expected: no production-path `std`/`alloc` `Vec` hits (heapless only). `send_ack`/`send_nack` build into `[0u8; crate::UDP_BUFFER_SIZE]` (`src/server/runtime.rs`, inside `send_ack`); the `server,bare_metal` rlib `nm`-audits to zero allocator symbols (CI since PR 0). If a real heap `Vec` shows up in an outbound SD path, STOP — the doc is not stale and the spec is wrong; report back. + +- [ ] **Step 2: Rewrite the stale sentence** + +Replace (in the `UDP_BUFFER_SIZE` doc): + +```text +Paths that return early before +attempting serialization (e.g. `publish_event` when there are no +subscribers) are not affected. Other outbound SD paths (announcement +builders, `SubscribeAck` / `SubscribeNack`) currently still use +heap `Vec` buffers and are not capped by this constant — that is a +known gap, planned alongside the bare-metal `no_alloc` refactor. +``` + +with: + +```text +Paths that return early before +attempting serialization (e.g. `publish_event` when there are no +subscribers) are not affected. The remaining outbound SD paths +(`OfferService` announcements, `SubscribeAck` / `SubscribeNack`) +serialize into stack buffers of this same size — PR #124's no-alloc +server work removed the former heap `Vec` buffers, so every outbound +path is capped by this constant. +``` + +- [ ] **Step 3: Verify + commit** + +Run: `cargo doc --no-deps 2>&1 | tail -2` — expect `Finished`. + +```bash +git add src/lib.rs +git commit -m "docs: UDP_BUFFER_SIZE rustdoc — outbound SD paths are stack-buffered since #124 + +Co-Authored-By: Claude Fable 5 " +``` + +--- + +### Task 8: CHANGELOG + +**Files:** +- Modify: `CHANGELOG.md` (0.8.0 section — add a `#### Breaking` block after the existing GAT one, matching its style) + +- [ ] **Step 1: Add the entry** + +````markdown +#### Breaking — `NonSdRequestCallback` gains an opaque `ctx: usize` first argument + +```rust +// before +pub type NonSdRequestCallback = fn(data: &[u8], source: SocketAddrV4); +// after +pub type NonSdRequestCallback = fn(ctx: usize, data: &[u8], source: SocketAddrV4); +``` + +The observer is now registered as a `(callback, ctx)` pair +(`ServerDeps::non_sd_observer: Option<(NonSdRequestCallback, usize)>`), +and `ctx` is passed back verbatim on every invocation. FFI consumers +stash their state pointer as `usize`; pure-Rust callers pass `0`. +`usize` (rather than a stored `*mut c_void`) keeps `Server: Send` — +and therefore `Server::run`'s declared `+ Send` bound — with no +`unsafe` in this crate; the pointer cast lives in the consumer's +callback body. + +##### Migration + +`non_sd_observer: Some(my_cb)` → `non_sd_observer: Some((my_cb, 0))`, +and add the leading `ctx: usize` parameter to the callback. +```` + +- [ ] **Step 2: Commit** + +```bash +git add CHANGELOG.md +git commit -m "docs: changelog for the ctx-carrying NonSdRequestCallback break + +Co-Authored-By: Claude Fable 5 " +``` + +--- + +### Task 9: Spec correction, full verification, push, PR + +**Files:** +- Modify: `docs/simple_someip/plans/2026-06-10-pr1-124-followups-design.md` (one word) + +- [ ] **Step 1: Fix the builder owner in the design doc** + +The spec says `ServerConfig::with_non_sd_observer`; the builder lives on +**ServerDeps**. Replace that one occurrence with +`ServerDeps::with_non_sd_observer` and commit: + +```bash +git add docs/simple_someip/plans/2026-06-10-pr1-124-followups-design.md +git commit -m "docs: with_non_sd_observer lives on ServerDeps, not ServerConfig + +Co-Authored-By: Claude Fable 5 " +``` + +- [ ] **Step 2: Full local verification (mirrors CI)** + +```bash +cargo fmt --check +cargo clippy --workspace --all-features -- -D warnings -D clippy::pedantic +cargo clippy --no-default-features -- -D warnings -D clippy::pedantic +cargo test --features server-tokio,client-tokio,bare_metal --tests --lib -- --test-threads=1 +cargo test --doc +cargo +nightly build --no-default-features --features client,bare_metal -Zbuild-std=core --target thumbv7em-none-eabihf +cargo +nightly build --no-default-features --features server,bare_metal -Zbuild-std=core --target thumbv7em-none-eabihf +cargo +nightly build --no-default-features --features client,server,bare_metal -Zbuild-std=core --target thumbv7em-none-eabihf +cargo clean -p simple-someip --target thumbv7em-none-eabihf +cargo build --target thumbv7em-none-eabihf --no-default-features --features server,bare_metal +nm -A target/thumbv7em-none-eabihf/debug/libsimple_someip.rlib | grep -c -E '__rust_alloc|__rg_alloc' +``` + +Expected: fmt/clippy clean; tests green; doc tests green; three `Finished`; +`nm` count `0`. The PR 0 future-size witnesses run inside the test step — +the `(fn, usize)` capture adds 8 bytes to the server run-future, far +inside the 25% budget headroom; if a witness trips, STOP and investigate +rather than raising a budget. + +- [ ] **Step 3: Push and open the PR (stacked on PR 0)** + +```bash +git push -u origin feature/pr1_124_followups +gh pr create --base feature/pr0_measurement_harness \ + --title "PR 1: #124 follow-ups — ctx-carrying NonSdRequestCallback + doc trues-up (#125 stack)" \ + --body "$(cat <<'EOF' +Second PR of the #125 / phase-22 stack +(docs/simple_someip/plans/2026-06-10-pr1-124-followups-design.md). +Stacked on PR 0 (#127); retargets when that merges. + +- **BREAKING:** `NonSdRequestCallback` gains an opaque `ctx: usize` + first argument, registered as a `(callback, ctx)` pair. Keeps + `Server: Send` with zero `unsafe` in-crate; FFI casts its pointer + in the callback body. Migration in CHANGELOG. +- Negative observer tests now have a live witness (the old None-case + test could not fail); mock transport gets per-socket pipes so + `from_unicast` is deterministic. +- Doc trues-up: eager-at-construction timing of the `Ready`-based + subscribe/unsubscribe; `announce_only_future` split-shape rationale; + `UDP_BUFFER_SIZE` outbound-SD claim (stack-buffered since #124). + +**Merge gate:** Feliciano gets the callback-signature heads-up first +so no further halo FFI builds on the bare `fn` shape. + +🤖 Generated with [Claude Code](https://claude.com/claude-code) +EOF +)" +``` + +--- + +## Self-review notes (already applied) + +- Spec coverage: item 1 → Task 2; item 2 → Task 5; item 3 → Task 6; + item 4 → Tasks 3+4 (the mock split is the enabling mechanic the spec + deferred to this plan); item 5 → Task 7; CHANGELOG → Task 8; the + spec's own ServerConfig/ServerDeps naming slip → Task 9. +- The `None` initializers needing no edit (Task 2 Step 5) was verified + against all 20 `non_sd_observer:` sites — only `Some(...)` sites and + type declarations change; `examples/*` and `tests/bare_metal_e2e.rs` + all pass `None` and compile unchanged. +- Task 4's refutation step (invert `from_unicast`, watch the test fail, + revert) guards against rebuilding another vacuous test. + +--- + +## Recorded deviation (2026-06-11, post-execution) + +Task 2's callback shape was superseded after execution by the +cross-branch contract reconciliation: the final signature is the +union `fn(ctx, source, service_id, method_id, payload, e2e_status)` +(library parses; e2e_status 0 = unchecked today). See the design +doc's §1 revision note. Applied as a follow-up commit rather than +rewriting this plan's task history. diff --git a/docs/simple_someip/plans/2026-06-11-128-embassy-union-rebase.md b/docs/simple_someip/plans/2026-06-11-128-embassy-union-rebase.md new file mode 100644 index 00000000..3bd6c2b0 --- /dev/null +++ b/docs/simple_someip/plans/2026-06-11-128-embassy-union-rebase.md @@ -0,0 +1,360 @@ +# PR #128 Rebase — Union Callback Adoption Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Rebase `feat/embassy-mem-channel-cap` (PR #128, Feliciano's draft) onto the merged #124→#127→#129 spine, dropping its three #124-duplicate commits and adopting the union `NonSdRequestCallback` contract for the runtime's `DispatchFn`, then capture the fresh size baseline that becomes PR 2's "before". + +**Architecture:** A 9-commit rebase with a small, fully-enumerated conflict surface (dry-run executed 2026-06-11 — see inventory below), followed by a mechanical union-adoption pass on the new `bare_metal_runtime` files (2 compile errors, both at `runtime.rs:345`), a `DISPATCH_CTX` parallel static, and ctx/source threading through `event_rx_dispatch_future`. + +**Tech Stack:** git rebase, Rust (nightly for the runtime's `#![feature]` and `-Zbuild-std`), cargo, GNU `nm`. + +**Ownership note:** `feat/embassy-mem-channel-cap` is Feliciano's draft PR. This plan produces a **preview branch**; force-pushing his branch happens only with his explicit ack (Task 8). + +--- + +## Dry-run inventory (executed 2026-06-11, preview preserved) + +A complete dry run exists: branch `preview/embassy_union_rebase` (tip `35ba14f`) in worktree `/tmp/embassy_rebase_preview`, rebased onto the PR 1 tip `8e41b0e`. Tasks 1–2 below are **already done on that branch** — an executor can resume from it (start at Task 3) or replay from scratch using the recipes. Per-commit results: + +| #128 commit | Result on rebase | +|---|---| +| `0901274`, `45a4630`, `1a4ca83` | **Dropped by construction** (rewritten duplicates of #124's `be292bb`/`3dafd3e`/`64fcc08`; patch-ids drifted so git will NOT auto-skip them — rebase from `1a4ca83`, not from the branch base) | +| `16e5b27` pre-bound sockets | clean | +| `1124531` host rx notify | clean | +| `7623829` payload/config sizes | clean | +| `63e549f` CLIENT_SOCKET_CHANNEL_CAP | 1 trivial conflict: `src/client/socket_manager.rs` import adjacency — keep BOTH lines | +| `0f48c16` ARENA cap cuts | clean | +| `d56a691` co-offered Subscribe | 1 conflict: `src/server/runtime.rs` — new `accept_subscribe` fn inserted above `handle_sd_message`; take embassy's insertion AND the chain's backticked doc line (`` `FindService` ``) | +| `77cf725` SD codec + helpers | 5 hunks in `src/server/mod.rs` / `src/server/runtime.rs` / `tests/bare_metal_server.rs` — ALL are union-vs-4-param of the same content; **keep HEAD (the union side) in every hunk**. The commit's new files (`src/sd_codec.rs`, helper fns) apply clean | +| `3f7d7d1` run_someip | clean | +| `223bf40` reusable runtime | clean (new files) — but **semantically un-adopted**: leaves exactly 2 compile errors at `src/bare_metal_runtime/runtime.rs:345` (E0308 `Some(fn)` vs `Option<(fn, usize)>`, E0605 4-param→6-param fn cast). Tasks 3–5 fix this | + +--- + +### Task 1: Preconditions and base selection + +**Files:** none (git only) + +- [ ] **Step 1: Pick the rebase target** + +The real target is `feature/phase21_api_symmetry` AFTER the spine (#124 → #127 → #129) merges. Until then, the PR 1 tip is content-identical: `origin/feature/pr1_124_followups` (`8e41b0e`). The preserved preview was built against the PR 1 tip. If the spine has merged since, replay Task 2 against the merged phase21 instead of resuming the preview — the inventory above still applies unless #129 was amended in review (check: `git log 8e41b0e..origin/feature/phase21_api_symmetry --oneline -- src/server/` — if #129 landed with changes beyond `8e41b0e`, re-verify the `77cf725` resolutions). + +- [ ] **Step 2: Resume or replay?** + +Run: `git -C /tmp/embassy_rebase_preview log --oneline -1 2>/dev/null` +If it prints `35ba14f feat(bare-metal): reusable runtime …`, resume from the preview (skip Task 2). Otherwise replay Task 2. + +--- + +### Task 2: The rebase (replay recipe — skip if resuming the preview) + +**Files:** conflict resolutions only, per the inventory. + +- [ ] **Step 1: Create the worktree and rebase from above the duplicates** + +```bash +git worktree add /tmp/embassy_rebase_preview -b preview/embassy_union_rebase origin/feat/embassy-mem-channel-cap +cd /tmp/embassy_rebase_preview +git rebase --onto 1a4ca83 +``` + +`` = `origin/feature/pr1_124_followups` (pre-spine-merge) or `origin/feature/phase21_api_symmetry` (post-merge). Rebasing from `1a4ca83` drops the three duplicates by construction. + +- [ ] **Step 2: Resolve `63e549f` (socket_manager.rs)** + +One hunk: HEAD's `use crate::log::{…}` vs embassy's `use super::CLIENT_SOCKET_CHANNEL_CAP;` at the same insertion point. Keep both (CLIENT_SOCKET_CHANNEL_CAP line first, then the log line). `git add` + `git rebase --continue`. + +- [ ] **Step 3: Resolve `d56a691` (runtime.rs)** + +One hunk: embassy inserts `async fn accept_subscribe(…)` (a ~95-line function) above `handle_sd_message`; HEAD's side of the hunk is only the doc line `/// Handle a Service Discovery message (Subscribe / \`FindService\` etc.).`. Resolution: take the entire embassy insertion, and replace its trailing un-backticked `FindService` doc line with the backticked HEAD version. `git add` + continue. + +- [ ] **Step 4: Resolve `77cf725` (3 files, 5 hunks)** + +Every hunk is the union contract (HEAD) vs the 4-param contract (embassy) of the SAME semantic content — the union strictly supersedes (it has everything plus `ctx` + `source`). Keep the HEAD side of **every** hunk; do NOT use whole-file `checkout --ours` (it would discard the commit's cleanly-merged additions elsewhere in those files). `git add -u` + continue. + +- [ ] **Step 5: Confirm completion** + +`3f7d7d1` and `223bf40` apply clean. Expected: `Successfully rebased`. Then `cargo +nightly check --no-default-features --features bare-metal-runtime,client 2>&1 | grep -c "^error"` → exactly 2 (both at `runtime.rs:345`) — that's the Task 3–5 worklist, not a failure. + +--- + +### Task 3: Union adoption — `DispatchFn`, `DISPATCH_CTX`, trampoline + +**Files:** +- Modify: `src/bare_metal_runtime/runtime.rs` (~line 63 alias; ~line 174 statics; ~line 263 trampoline; ~line 345 registration; ~line 441 init store; the init config struct — locate fields with `grep -n "pub dispatch\|dispatch:" src/bare_metal_runtime/runtime.rs`) + +- [ ] **Step 1: Reshape the alias** (~line 63) + +```rust +/// Platform dispatch sink for inbound messages (decoded by the runtime). +/// Same shape as [`crate::server::NonSdRequestCallback`] — the union +/// contract — so a platform can use one handler for both the server's +/// non-SD observer and the runtime's notification RX path. `ctx` is the +/// opaque word registered at [`init`]; `source` is the sender; +/// `e2e_status` is real on the RX path (Profile-5 check) and `0` +/// (unchecked) on the server-request path. +pub type DispatchFn = fn( + ctx: usize, + source: core::net::SocketAddrV4, + service_id: u16, + method_id: u16, + payload: &[u8], + e2e_status: u8, +); +``` + +- [ ] **Step 2: Add the parallel ctx static** (next to `static DISPATCH`, ~line 174) + +```rust +static DISPATCH: AtomicUsize = AtomicUsize::new(0); // DispatchFn as usize +static DISPATCH_CTX: AtomicUsize = AtomicUsize::new(0); // opaque ctx word for DISPATCH +``` + +- [ ] **Step 3: Register ctx at init** + +The init config struct (the one whose fields feed `SEND_FN`/`NOW_FN`/`DISPATCH` stores at ~line 439-441) gains a field: + +```rust + /// Opaque context word passed back verbatim as the first argument of + /// every `dispatch` invocation (FFI: stash a pointer as `usize`). + pub dispatch_ctx: usize, +``` + +and beside `DISPATCH.store(...)` (~line 441): + +```rust + DISPATCH_CTX.store(config.dispatch_ctx, Ordering::Release); +``` + +**Flag for Feliciano:** if that struct is `#[repr(C)]` consumed from C, this is a C-side ABI addition — field at the END of the struct, and the C header updates with it. + +- [ ] **Step 4: Reshape the trampoline** (~line 263) + +```rust +/// Forwards a parsed inbound message to the platform dispatch callback. +/// The `_ctx` received from the caller is ignored: the runtime's real +/// ctx lives in [`DISPATCH_CTX`] (registered at [`init`], possibly +/// re-registered later), so loading it here keeps late re-registration +/// coherent — callers register/pass `0`. +fn dispatch( + _ctx: usize, + source: core::net::SocketAddrV4, + service_id: u16, + method_id: u16, + payload: &[u8], + e2e_status: u8, +) { + let raw = DISPATCH.load(Ordering::Acquire); + if raw == 0 { + return; + } + // SAFETY: stored from a valid DispatchFn in `init`. + let f: DispatchFn = unsafe { core::mem::transmute::(raw) }; + f( + DISPATCH_CTX.load(Ordering::Acquire), + source, + service_id, + method_id, + payload, + e2e_status, + ); +} +``` + +- [ ] **Step 5: Fix the registration** (~line 345) + +```rust + non_sd_observer: Some((dispatch as crate::server::NonSdRequestCallback, 0)), +``` + +(`0` because the trampoline injects `DISPATCH_CTX` itself — see Step 4 doc.) + +- [ ] **Step 6: Verify the two errors are gone** + +Run: `cargo +nightly check --no-default-features --features bare-metal-runtime,client 2>&1 | grep -E "^error" | head` +Expected: errors at `runtime.rs:345` gone; remaining errors (if any) are in `bare_metal_tasks.rs` — Task 4's worklist. + +--- + +### Task 4: Thread ctx + source through `event_rx_dispatch_future` + +**Files:** +- Modify: `src/bare_metal_tasks.rs` (~lines 95-125, plus its callers — locate with `grep -n "event_rx_dispatch_future" src/`) + +- [ ] **Step 1: Reshape the helper** + +The fn's `dispatch` parameter is currently an inline 4-param fn type. It becomes the union shape plus a pass-through `ctx`, and the receive captures the datagram's source (`ReceivedDatagram.source` is already there — only `bytes_received` was being kept): + +```rust +pub async fn event_rx_dispatch_future<'a, S, R>( + rx_socket: &'a S, + e2e: &'a R, + e2e_enabled: bool, + dispatch: crate::bare_metal_runtime::DispatchFn, + ctx: usize, + buf: &'a mut [u8], +) where + S: TransportSocket, + R: E2ERegistryHandle, +{ + loop { + let (n, source) = match rx_socket.recv_from(&mut *buf).await { + Ok(d) => (d.bytes_received, d.source), + Err(_) => continue, + }; + let Some(parsed) = parse_someip_datagram(&buf[..n]) else { + continue; + }; + let (status, body) = if e2e_enabled { + check_parsed_e2e(e2e, &parsed) + } else { + (E2ECheckStatus::Unchecked, parsed.payload) + }; + dispatch( + ctx, + source, + parsed.service_id, + parsed.method_id, + body, + e2e_status_code(status), + ); + } +} +``` + +NOTE on the `dispatch` param type: if `bare_metal_tasks` must stay decoupled from the `bare-metal-runtime` feature (check the cfg on `bare_metal_runtime`'s module declaration in lib.rs), keep an inline fn type with the same six params instead of naming `DispatchFn`. External (non-runtime) callers pass their real callback + ctx and get verbatim forwarding; the runtime passes its trampoline + `0`. + +- [ ] **Step 2: Update the callers** + +`run_someip` (in `bare_metal_tasks.rs`) and any direct caller pass the extra `ctx` argument — the runtime's composition passes `0` (trampoline injects). Locate: `grep -n "event_rx_dispatch_future(" src/`. + +- [ ] **Step 3: Sweep for leftover 4-param shapes** + +Run: `grep -rn "fn(service_id: u16, method_id: u16" src/` +Expected: zero hits (every dispatch-shaped type is now the union). + +- [ ] **Step 4: Full check** + +Run: `cargo +nightly check --no-default-features --features bare-metal-runtime,client 2>&1 | tail -2` → `Finished`. + +- [ ] **Step 5: Commit** (one commit on the preview branch for Tasks 3+4) + +```bash +git add src/bare_metal_runtime/runtime.rs src/bare_metal_tasks.rs +git commit -m "feat(bare-metal): DispatchFn adopts the union callback contract + +Same six-param shape as NonSdRequestCallback (decision 2026-06-11): +ctx + source + decoded fields + e2e_status. DISPATCH_CTX parallel +static carries the opaque word; the dispatch trampoline injects it so +late re-registration stays coherent; event_rx_dispatch_future threads +ctx/source through (e2e_status stays REAL on this path). + +Co-Authored-By: Claude Fable 5 " +``` + +--- + +### Task 5: CHANGELOG + sd_codec visibility note + +**Files:** +- Modify: `CHANGELOG.md` + +- [ ] **Step 1: Behavior notes under the 0.8.0 section** + +Append to the existing `#### Breaking — NonSdRequestCallback…` block's vicinity (match file style): + +- `PENDING_RESPONSES_CAP` 64→8 is a **global** bound (also tokio): more than 8 outstanding request-response pairs now returns `Err(Error::Capacity(…))`. Sized for the embedded target; raise the const if a host consumer genuinely needs more in flight. (`REQUEST_QUEUE_CAP` 32→4 is NOT consumer-visible: the feeding control channel was always depth 4 on both paths — verified `BoundedSender` + tokio `channel(N)`.) +- The bare-metal runtime's `DispatchFn` now matches `NonSdRequestCallback` (union shape); the init config gains `dispatch_ctx`. + +- [ ] **Step 2: sd_codec visibility — decision recorded, not changed** + +`sd_codec::parse_someip_datagram` stays at its current visibility (the runtime's RX path uses it; halo's FFI may too). Narrowing to `pub(crate)` is Feliciano's call on his own PR — leave a PR-comment question, don't change it here. + +- [ ] **Step 3: Commit** + +```bash +git add CHANGELOG.md +git commit -m "docs: changelog for ARENA cap bounds + DispatchFn union shape + +Co-Authored-By: Claude Fable 5 " +``` + +--- + +### Task 6: Full verification + +- [ ] **Step 1: fmt the conflict resolutions** + +Run: `cargo fmt` then `git diff --stat` — the Task 2 resolutions (esp. `accept_subscribe`) may need reflow; if fmt changed files, amend them into the rebase HEAD: `git add -u && git commit --amend --no-edit` is WRONG here (HEAD is the Task 5 commit) — instead commit fmt separately: `git commit -m "style: rustfmt over rebase resolutions"`. + +- [ ] **Step 2: The matrix** + +```bash +cargo fmt --check +cargo clippy --workspace --all-features -- -D warnings -D clippy::pedantic +cargo clippy --no-default-features -- -D warnings -D clippy::pedantic +cargo test --features server-tokio,client-tokio,bare_metal --tests --lib -- --test-threads=1 +cargo test --doc +cargo check -p simple-someip-embassy-net --tests +cargo +nightly build --no-default-features --features client,bare_metal -Zbuild-std=core --target thumbv7em-none-eabihf +cargo +nightly build --no-default-features --features server,bare_metal -Zbuild-std=core --target thumbv7em-none-eabihf +cargo +nightly build --no-default-features --features client,server,bare_metal -Zbuild-std=core --target thumbv7em-none-eabihf +cargo +nightly build --no-default-features --features bare-metal-runtime,client -Zbuild-std=core --target thumbv7em-none-eabihf +cargo clean -p simple-someip --target thumbv7em-none-eabihf +cargo build --target thumbv7em-none-eabihf --no-default-features --features server,bare_metal +nm -A target/thumbv7em-none-eabihf/debug/libsimple_someip.rlib | grep -c -E '__rust_alloc|__rg_alloc' +``` + +Expected: all clean; nm prints `0`. The fourth build-std line is NEW (the runtime feature under halo's constraint) — if it fails on `embassy-executor` deps, record the failure verbatim; it's a finding about #128, not about this rebase. The future-size witnesses run inside the test step and must PASS (the cap cuts shrink futures; budgets are upper bounds). If the `#128` clippy surface has pre-existing pedantic warnings the chain's gate now catches (the chain enforces `--workspace --all-features`), fix mechanically and commit as `style:`. + +- [ ] **Step 3: Probe-mirror check (`7623829` touched payload/option sizes)** + +`tools/size_probe`'s `ProbePayload` mirrors `TestPayload` (`src/protocol/sd/test_support.rs`) field-for-field. `7623829` changed `heapless_payload.rs` / `sd/options.rs` / `static_channels` — verify `TestPayload`/`TestSdHeader` themselves are untouched (`git diff 1a4ca83..HEAD -- src/protocol/sd/test_support.rs` → empty means the mirror holds). `MAX_CONFIGURATION_STRING_LENGTH` changes WILL shift captured layouts — that's expected and handled by Step 4's re-capture, not an error. + +- [ ] **Step 4: Fresh baseline — this is PR 2's "before"** + +```bash +tools/capture_type_sizes.sh +``` + +Copy the new numbers into a new committed baseline `docs/simple_someip/plans/baselines/post-128-size-baseline.md` (same format as `pr0-size-baseline.md`, with a header noting: captured on the #128-rebased tree; supersedes pr0 baseline as PR 2's "before"; the deltas vs pr0 quantify #128's cap cuts — record them, they're the first real measured win of the stack). Also re-run the host witnesses with `--nocapture` and record the `FUTURE_SIZE` lines. + +```bash +git add docs/simple_someip/plans/baselines/post-128-size-baseline.md +git commit -m "docs: post-#128 size baseline (PR 2's before; quantifies the cap cuts) + +Co-Authored-By: Claude Fable 5 " +``` + +--- + +### Task 7: Witness-budget tightening decision (optional, record either way) + +The PR 0 witness budgets are `pr0-baseline × 1.25`. After #128's cuts the real sizes drop well below those budgets, leaving slack that could mask a future regression up to the OLD budget. Either tighten the budget consts (`src/client/mod.rs`, `tests/bare_metal_e2e.rs`) to `post-128-baseline × 1.25` in this branch, or record in the new baseline doc that tightening lands with PR 2. **Default: tighten now** — it's two consts per file and the witnesses exist to be tight. + +--- + +### Task 8: Handoff (Feliciano coordination — DO NOT force-push his branch unilaterally) + +- [ ] **Step 1: Push the preview** + +```bash +git push -u origin preview/embassy_union_rebase +``` + +- [ ] **Step 2: PR comment on #128** + +Summarize: rebase preview ready; 3 duplicate commits dropped; his 9 commits survive with authorship intact (rebase preserves author); union contract adopted for `DispatchFn` + `DISPATCH_CTX` + init-config `dispatch_ctx` field (C-side ABI addition flagged); the conflict inventory + this plan's path; ask him to either `git reset --hard origin/preview/embassy_union_rebase && git push --force-with-lease` on his branch, or cherry-pick at his leisure. Include the open question: `sd_codec` visibility (keep `pub` for halo FFI, or narrow?). + +- [ ] **Step 3: Sequencing reminder** + +This branch can only MERGE after the spine (#124→#127→#129) lands in phase21 — it contains the spine. If #129 gets amended in review, re-run Task 1 Step 1's check and rebase the preview again (cheap: the inventory holds). + +--- + +## Self-review notes (already applied) + +- The dry run IS the spec-coverage check: every #128 commit is accounted for in the inventory table; the 2 compile errors are closed by Tasks 3–4; sweep step (Task 4 Step 3) catches any 4-param stragglers. +- `e2e_status` semantics differ by path and both docs say so: REAL on the RX/notification path (`check_parsed_e2e`), `0` on the server-request path — the union docs on both aliases carry the distinction. +- The trampoline-injects-ctx design (register `(dispatch, 0)`) was chosen over registering the real ctx because `DISPATCH`/`DISPATCH_CTX` support late re-registration; the server's stored copy would go stale. Documented on the trampoline. +- `event_rx_dispatch_future` gets ctx as a pass-through parameter (not a static read) because it's a public spawnable helper — non-runtime callers need verbatim forwarding. diff --git a/simple-someip-embassy-net/tests/loopback.rs b/simple-someip-embassy-net/tests/loopback.rs index bb27c108..bc9f571c 100644 --- a/simple-someip-embassy-net/tests/loopback.rs +++ b/simple-someip-embassy-net/tests/loopback.rs @@ -605,6 +605,7 @@ async fn client_receives_server_sd_announcement() { timer: LocalTimer, e2e_registry: server_e2e, subscriptions: server_subs, + non_sd_observer: None, }; // Default `H = Arc`. `Arc: @@ -728,6 +729,7 @@ async fn client_send_request_server_runloop_stable() { timer: LocalTimer, e2e_registry: server_e2e, subscriptions: server_subs, + non_sd_observer: None, }; // Explicit `Arc` `H` so the compiler diff --git a/src/lib.rs b/src/lib.rs index d49a654a..05047a61 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -143,10 +143,11 @@ extern crate alloc; /// `server::Error::Capacity("udp_buffer")`, depending on the path. /// Paths that return early before /// attempting serialization (e.g. `publish_event` when there are no -/// subscribers) are not affected. Other outbound SD paths (announcement -/// builders, `SubscribeAck` / `SubscribeNack`) currently still use -/// heap `Vec` buffers and are not capped by this constant — that is a -/// known gap, planned alongside the bare-metal `no_alloc` refactor. +/// subscribers) are not affected. The remaining outbound SD paths +/// (`OfferService` announcements, `SubscribeAck` / `SubscribeNack`) +/// serialize into stack buffers of this same size — the phase-21 +/// per-event-allocation cleanup (`7c58649`) removed the former heap +/// `Vec` buffers, so every outbound path is capped by this constant. /// /// Note that this is an application-level UDP payload limit, not an /// Ethernet-MTU-safe size: a 1500-byte UDP payload exceeds a 1500-byte diff --git a/src/server/mod.rs b/src/server/mod.rs index 2859a028..5e781b63 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -284,13 +284,14 @@ where /// `Arc>` for this; bare-metal callers /// supply their own [`SubscriptionHandle`] impl. pub subscriptions: Sub, - /// Optional callback invoked from the server's receive loop for every - /// non-SD **unicast** datagram (method requests / fire-and-forget calls - /// to offered services). `None` reproduces the historical - /// "non-SD ignored" behavior. The callback receives the full raw - /// datagram bytes and the source `SocketAddrV4`; the consumer is - /// responsible for re-parsing the SOME/IP header and any E2E check. - pub non_sd_observer: Option, + /// Optional `(callback, ctx)` pair invoked from the server's receive + /// loop for every non-SD **unicast** datagram (method requests / + /// fire-and-forget calls to offered services). `None` reproduces the + /// historical "non-SD ignored" behavior. The callback receives the + /// opaque `ctx` word back verbatim, plus the full raw datagram bytes + /// and the source `SocketAddrV4`; the consumer is responsible for + /// re-parsing the SOME/IP header and any E2E check. + pub non_sd_observer: Option<(NonSdRequestCallback, usize)>, } /// Tokio-defaulted constructor. @@ -406,12 +407,15 @@ where } } - /// Register a callback invoked for every non-SD unicast datagram - /// (method requests / fire-and-forget calls to offered services). - /// Passing `None` (the default if unset) preserves the historical - /// "ignore non-SD" behavior. + /// Register a `(callback, ctx)` pair invoked for every non-SD unicast + /// datagram (method requests / fire-and-forget calls to offered + /// services). The opaque `ctx` word is passed back verbatim on every + /// invocation — FFI callers stash a pointer here as `usize`; + /// pure-Rust callers that need no context pass `0`. Passing `None` + /// (the default if unset) preserves the historical "ignore non-SD" + /// behavior. #[must_use] - pub fn with_non_sd_observer(mut self, observer: Option) -> Self { + pub fn with_non_sd_observer(mut self, observer: Option<(NonSdRequestCallback, usize)>) -> Self { self.non_sd_observer = observer; self } @@ -516,9 +520,10 @@ where /// run-futures built from the same `Server` from racing the sockets /// and SD session counter. pub started: StartedLatch, - /// Optional callback for non-SD unicast datagrams (method requests). - /// `None` reproduces the default "non-SD ignored" behavior. - pub non_sd_observer: Option, + /// Optional `(callback, ctx)` pair for non-SD unicast datagrams + /// (method requests). `None` reproduces the default "non-SD + /// ignored" behavior. + pub non_sd_observer: Option<(NonSdRequestCallback, usize)>, } /// SOME/IP Server that can offer services and publish events. @@ -626,23 +631,46 @@ pub struct Server< /// — because the run-future captures an owned copy independent of /// `&self`'s lifetime, and both alternatives are `Clone + 'static`. started: StartedLatch, - /// Optional callback invoked for non-SD unicast datagrams received + /// Optional `(callback, ctx)` pair invoked for non-SD unicast datagrams received /// on the service's port (method requests / fire-and-forget calls). /// `None` preserves the historical "ignore non-SD" behavior; `Some` /// surfaces those datagrams to the consumer (used by halo's FFI to /// dispatch HWP1 method requests). - non_sd_observer: Option, + non_sd_observer: Option<(NonSdRequestCallback, usize)>, } /// Callback invoked by the server's `recv_loop` for every non-SD /// unicast datagram received on the service's port (i.e. method /// requests / fire-and-forget calls to the offered services). The -/// payload is the full raw datagram bytes; the caller is responsible -/// for re-parsing the SOME/IP header (and applying any E2E check) on -/// the consumer side. `fn` pointers are `Copy + Send + Sync + 'static`, -/// so they can be stored on the `Server` and captured by the -/// run-future without adding a new generic. -pub type NonSdRequestCallback = fn(data: &[u8], source: core::net::SocketAddrV4); +/// SOME/IP header is parsed in `recv_loop` and the callback receives +/// decoded fields — the consumer never parses bytes. `payload` is the +/// bytes after the 16-byte SOME/IP header. `e2e_status` is `0` +/// (unchecked) — server-side request E2E is not applied here today. +/// `source` is the sender's address, currently unused by known +/// consumers (future-proofing). +/// +/// `ctx` is an opaque caller-owned context word, registered alongside +/// the callback as a `(NonSdRequestCallback, usize)` pair and passed +/// back verbatim on every invocation. It is deliberately `usize` +/// rather than `*mut c_void`: a stored raw pointer would make +/// [`Server`] `!Send` and break [`Server::run`]'s declared `+ Send` +/// bound, while `usize` is trivially `Send + Sync` and matches the +/// `uintptr_t` an FFI caller holds anyway. No `unsafe` enters this +/// crate — the cast back to a pointer (and its safety justification) +/// lives in the consumer's callback body, the only place that knows +/// the pointee's lifetime and thread-safety. Rust-native users that +/// need no context pass `0`. `fn` pointers are +/// `Copy + Send + Sync + 'static`, so the pair can be stored on the +/// `Server` and captured by the run-future without adding a new +/// generic. +pub type NonSdRequestCallback = fn( + ctx: usize, + source: core::net::SocketAddrV4, + service_id: u16, + method_id: u16, + payload: &[u8], + e2e_status: u8, +); #[cfg(feature = "_alloc")] type StartedLatch = Arc; @@ -1292,6 +1320,15 @@ where /// `OfferService` to the same SD multicast group without /// competing for inbound datagrams. /// + /// Design note: this partially reintroduces the split-future shape + /// phase 21 removed — deliberately. An announce-only future never + /// touches the receive path, so the invariant that motivated the + /// phase-21 combined run-future (no two futures racing the same + /// sockets and SD session counter) is preserved: the [`Self::run`] + /// path is still guarded by the first-poll `started` latch, and + /// supplementary announce loops only ever *send* on the shared SD + /// socket. + /// /// The returned future loops forever (1 s tick between /// announcements); spawn it on your executor. pub fn announce_only_future<'a>( diff --git a/src/server/runtime.rs b/src/server/runtime.rs index 33314c91..9f07235b 100644 --- a/src/server/runtime.rs +++ b/src/server/runtime.rs @@ -440,7 +440,7 @@ async fn recv_loop( subscriptions: &Sub, unicast_buf: &mut [u8], sd_buf: &mut [u8], - non_sd_observer: Option, + non_sd_observer: Option<(super::NonSdRequestCallback, usize)>, ) -> Result<(), Error> where T: TransportSocket, @@ -538,11 +538,20 @@ where } else if from_unicast { // Surface non-SD unicast (method requests / fire-and-forget // calls to offered services) via the registered callback. - // The full raw datagram is forwarded; the consumer is - // responsible for re-parsing and any E2E check. - if let Some(cb) = non_sd_observer { + // The SOME/IP header is parsed here; the consumer receives + // decoded fields and never hand-rolls parsing. + if let Some((cb, ctx)) = non_sd_observer { if let core::net::SocketAddr::V4(src_v4) = addr { - cb(data, src_v4); + let id = view.header().message_id(); + // Server-side requests are not E2E-checked today. + cb( + ctx, + src_v4, + id.service_id(), + id.method_id(), + view.payload_bytes(), + 0, + ); } } else { crate::log::trace!( @@ -585,7 +594,7 @@ pub(super) async fn run_combined( is_passive: bool, unicast_buf: &mut [u8], sd_buf: &mut [u8], - non_sd_observer: Option, + non_sd_observer: Option<(super::NonSdRequestCallback, usize)>, ) -> Result<(), Error> where H: SharedHandle, diff --git a/src/server/subscription_manager.rs b/src/server/subscription_manager.rs index da395c8b..3c3b591f 100644 --- a/src/server/subscription_manager.rs +++ b/src/server/subscription_manager.rs @@ -322,6 +322,12 @@ pub trait SubscriptionHandle: Clone + 'static { /// Idempotent: if the subscriber is already present, this is a no-op /// returning `Ok(())`. Returns `Err(SubscribeError)` if a capacity /// limit would be exceeded. + /// + /// Timing note: implementations whose critical section is fully + /// synchronous (e.g. `StaticSubscriptionHandle`) may perform the + /// mutation when the future is *constructed*, deferring only the + /// result delivery to the poll. Callers must not assume that + /// constructing the returned future is free of side effects. fn subscribe( &self, service_id: u16, @@ -331,6 +337,8 @@ pub trait SubscriptionHandle: Clone + 'static { ) -> Self::SubscribeFuture<'_>; /// Remove a subscriber from an event group. + /// + /// Same construction-time-mutation caveat as [`Self::subscribe`]. fn unsubscribe( &self, service_id: u16, @@ -500,6 +508,11 @@ pub mod bare_metal_subscription_impl { // free of `alloc` (the `server` feature no longer implies // `_alloc`). `Ready` is `Send` when `T: Send`, satisfying any // `Send`-checked run path. + // Consequence (documented on the trait): the mutation runs + // eagerly at future construction; only the result is delivered + // through the poll. The in-tree caller awaits immediately, so + // the difference from the lazy boxed impls is unobservable + // there. type SubscribeFuture<'a> = core::future::Ready>; type UnsubscribeFuture<'a> = core::future::Ready<()>; diff --git a/tests/bare_metal_server.rs b/tests/bare_metal_server.rs index 25952ac5..46885e19 100644 --- a/tests/bare_metal_server.rs +++ b/tests/bare_metal_server.rs @@ -49,9 +49,20 @@ struct MockPipe { inbound_waker: Mutex>, } +/// Per-socket pipes routed by bind options: with a single shared +/// queue, whichever `select_biased!` arm polled first stole the +/// datagram, so `recv_loop`'s `from_unicast` flag depended on the +/// alternating bias — per-socket queues make routing deterministic. #[derive(Clone)] struct MockFactory { - pipe: Arc, + /// Handed to sockets bound WITHOUT multicast options — the + /// server's unicast service socket. + unicast_pipe: Arc, + /// Handed to sockets bound WITH `multicast_if_v4` set — the + /// active server's SD socket. NOTE: passive servers bind their SD + /// placeholder without multicast options, so BOTH passive sockets + /// share `unicast_pipe` and this pipe goes unused. + sd_pipe: Arc, next_port: Arc>, } @@ -59,8 +70,12 @@ impl TransportFactory for MockFactory { type Socket = MockSocket; type BindFuture<'a> = core::pin::Pin> + Send + 'a>>; - fn bind<'a>(&'a self, addr: SocketAddrV4, _options: &'a SocketOptions) -> Self::BindFuture<'a> { - let pipe = Arc::clone(&self.pipe); + fn bind<'a>(&'a self, addr: SocketAddrV4, options: &'a SocketOptions) -> Self::BindFuture<'a> { + let pipe = if options.multicast_if_v4.is_some() { + Arc::clone(&self.sd_pipe) + } else { + Arc::clone(&self.unicast_pipe) + }; // Mock: assign port deterministically. If caller asked for 0, // hand out an incrementing fake ephemeral port. let port = if addr.port() == 0 { @@ -271,9 +286,9 @@ impl SubscriptionHandle for MockSubscriptions { #[tokio::test] async fn server_constructible_without_server_tokio_feature() { - let pipe = Arc::new(MockPipe::default()); let factory = MockFactory { - pipe: Arc::clone(&pipe), + unicast_pipe: Arc::new(MockPipe::default()), + sd_pipe: Arc::new(MockPipe::default()), next_port: Arc::new(Mutex::new(0)), }; @@ -319,9 +334,9 @@ async fn server_constructible_without_server_tokio_feature() { #[tokio::test] async fn passive_server_constructible_without_server_tokio_feature() { - let pipe = Arc::new(MockPipe::default()); let factory = MockFactory { - pipe: Arc::clone(&pipe), + unicast_pipe: Arc::new(MockPipe::default()), + sd_pipe: Arc::new(MockPipe::default()), next_port: Arc::new(Mutex::new(0)), }; @@ -352,30 +367,86 @@ async fn passive_server_constructible_without_server_tokio_feature() { // ── NonSdRequestCallback witness ────────────────────────────────────── // -// Drives a non-SD unicast datagram through the server's `recv_loop` -// and verifies the registered callback receives the right bytes + source. -// The companion test confirms `None` preserves the historical -// "ignore non-SD" behavior. - -// `NonSdRequestCallback` is `fn(&[u8], SocketAddrV4)` — a plain function -// pointer, so it can't capture environment. Each test parks its -// observation in a dedicated static so the callback can write into it -// without interfering with sibling tests (cargo runs tests in parallel -// within a test binary). +// Drives datagrams through the server's `recv_loop` and checks the +// observer contract: `NonSdRequestCallback` is +// `fn(ctx: usize, source: SocketAddrV4, service_id: u16, method_id: u16, +// payload: &[u8], e2e_status: u8)` — a plain function pointer, so +// it can't capture environment. `recv_loop` parses the SOME/IP header +// and passes decoded fields; the consumer never sees raw datagram bytes. +// Each test parks its observation in a dedicated `OnceLock`-backed +// static to avoid interference under parallel `cargo test`. +// +// Positive test: registered observer fires for non-SD unicast. +// Negative tests: registered observer must NOT fire for SD messages +// (regardless of socket) or for non-SD datagrams on the SD socket. +// None test: with no observer registered, a non-SD unicast is processed +// without panicking (no callback to witness — just a no-panic guarantee). use std::sync::OnceLock; -static OBSERVED_SOME: OnceLock, SocketAddrV4)>>> = OnceLock::new(); -static OBSERVED_NONE: OnceLock, SocketAddrV4)>>> = OnceLock::new(); - -fn record_some(data: &[u8], source: SocketAddrV4) { +static OBSERVED_SOME: OnceLock, u8)>>> = + OnceLock::new(); + +fn record_some( + ctx: usize, + source: SocketAddrV4, + service_id: u16, + method_id: u16, + payload: &[u8], + e2e_status: u8, +) { let slot = OBSERVED_SOME.get_or_init(|| Mutex::new(None)); - *slot.lock().unwrap() = Some((data.to_vec(), source)); + *slot.lock().unwrap() = Some(( + ctx, + source, + service_id, + method_id, + payload.to_vec(), + e2e_status, + )); +} + +static OBSERVED_SD_UNICAST: OnceLock, u8)>>> = + OnceLock::new(); +static OBSERVED_MULTICAST: OnceLock, u8)>>> = + OnceLock::new(); + +fn record_sd_unicast( + ctx: usize, + source: SocketAddrV4, + service_id: u16, + method_id: u16, + payload: &[u8], + e2e_status: u8, +) { + let slot = OBSERVED_SD_UNICAST.get_or_init(|| Mutex::new(None)); + *slot.lock().unwrap() = Some(( + ctx, + source, + service_id, + method_id, + payload.to_vec(), + e2e_status, + )); } -fn record_none(data: &[u8], source: SocketAddrV4) { - let slot = OBSERVED_NONE.get_or_init(|| Mutex::new(None)); - *slot.lock().unwrap() = Some((data.to_vec(), source)); +fn record_multicast( + ctx: usize, + source: SocketAddrV4, + service_id: u16, + method_id: u16, + payload: &[u8], + e2e_status: u8, +) { + let slot = OBSERVED_MULTICAST.get_or_init(|| Mutex::new(None)); + *slot.lock().unwrap() = Some(( + ctx, + source, + service_id, + method_id, + payload.to_vec(), + e2e_status, + )); } /// Build a minimal SOME/IP method-request datagram (16-byte header, @@ -385,16 +456,39 @@ fn record_none(data: &[u8], source: SocketAddrV4) { /// `simple_someip::protocol::Header::encode` would be cleaner but the /// header is small enough to spell out by hand and avoids dragging /// the encoder dep into the test. -fn build_method_request(service_id: u16, method_id: u16) -> Vec { - let mut buf = Vec::with_capacity(16); +fn build_method_request(service_id: u16, method_id: u16, payload: &[u8]) -> Vec { + let mut buf = Vec::with_capacity(16 + payload.len()); buf.extend_from_slice(&service_id.to_be_bytes()); // message_id (high) buf.extend_from_slice(&method_id.to_be_bytes()); // message_id (low) - buf.extend_from_slice(&8u32.to_be_bytes()); // length = header(8) + payload(0) + buf.extend_from_slice(&(8u32 + payload.len() as u32).to_be_bytes()); // length = header(8) + payload buf.extend_from_slice(&0u32.to_be_bytes()); // request_id buf.push(1); // protocol_version buf.push(1); // interface_version buf.push(0); // message_type = Request (0x00) buf.push(0); // return_code = OK + buf.extend_from_slice(payload); + buf +} + +/// Build a minimal, well-formed SOME/IP-SD datagram: SD message id +/// (0xFFFF / 0x8100), then an SD payload with flags + reserved and +/// ZERO entries / options. Routing-wise a legitimate (if vacuous) SD +/// message — `recv_loop` must hand it to SD handling, never to the +/// non-SD observer, regardless of which socket it arrived on. +fn build_sd_message() -> Vec { + let mut buf = Vec::with_capacity(28); + buf.extend_from_slice(&0xFFFFu16.to_be_bytes()); // message_id (high): SD service + buf.extend_from_slice(&0x8100u16.to_be_bytes()); // message_id (low): SD method + buf.extend_from_slice(&20u32.to_be_bytes()); // length = header(8) + sd payload(12) + buf.extend_from_slice(&0u32.to_be_bytes()); // request_id + buf.push(1); // protocol_version + buf.push(1); // interface_version + buf.push(2); // message_type = Notification (0x02) + buf.push(0); // return_code = OK + buf.push(0x80); // SD flags: reboot + buf.extend_from_slice(&[0, 0, 0]); // reserved + buf.extend_from_slice(&0u32.to_be_bytes()); // entries array length = 0 + buf.extend_from_slice(&0u32.to_be_bytes()); // options array length = 0 buf } @@ -415,9 +509,10 @@ async fn drive_until bool>(mut check: F) { #[tokio::test] async fn non_sd_observer_some_receives_unicast_method_request() { - let pipe = Arc::new(MockPipe::default()); + let unicast_pipe = Arc::new(MockPipe::default()); let factory = MockFactory { - pipe: Arc::clone(&pipe), + unicast_pipe: Arc::clone(&unicast_pipe), + sd_pipe: Arc::new(MockPipe::default()), next_port: Arc::new(Mutex::new(0)), }; @@ -434,7 +529,7 @@ async fn non_sd_observer_some_receives_unicast_method_request() { timer: MockTimer, e2e_registry: e2e_handle, subscriptions: subs, - non_sd_observer: Some(record_some as NonSdRequestCallback), + non_sd_observer: Some((record_some as NonSdRequestCallback, 0xC0FF_EE00)), }; let (_server, _handles, run): ( @@ -447,21 +542,17 @@ async fn non_sd_observer_some_receives_unicast_method_request() { let handle = tokio::spawn(run); - // Queue a non-SD unicast method-request datagram. `from_unicast` - // distinguishes which socket received it: the first socket bound - // by `MockFactory` (port 30700) is the unicast socket; the second - // (port 30490) is the SD socket. Since the mock pipe is shared - // across both sockets, we drive the datagram into the unicast - // arm by tagging it with a non-SD message_id and relying on the - // `select_biased!`'s prefer-unicast tick to pick the unicast - // future first. - let payload = build_method_request(0x1234, 0x0001); + // Queue a non-SD unicast method-request datagram on the unicast + // socket's own pipe; per-socket pipes make `from_unicast = true` + // deterministic. + let datagram = build_method_request(0x1234, 0x0001, &[0xDE, 0xAD, 0xBE, 0xEF]); let src = SocketAddrV4::new(Ipv4Addr::new(192, 0, 2, 100), 40000); - pipe.inbound + unicast_pipe + .inbound .lock() .unwrap() - .push_back((payload.clone(), src)); - if let Some(w) = pipe.inbound_waker.lock().unwrap().take() { + .push_back((datagram.clone(), src)); + if let Some(w) = unicast_pipe.inbound_waker.lock().unwrap().take() { w.wake(); } @@ -473,7 +564,7 @@ async fn non_sd_observer_some_receives_unicast_method_request() { }) .await; - let (got_data, got_src) = OBSERVED_SOME + let (got_ctx, got_src, got_service, got_method, got_payload, got_e2e) = OBSERVED_SOME .get() .unwrap() .lock() @@ -481,47 +572,182 @@ async fn non_sd_observer_some_receives_unicast_method_request() { .clone() .expect("callback fired"); assert_eq!( - got_data, payload, - "callback must receive the full raw datagram bytes" + got_ctx, 0xC0FF_EE00, + "callback must receive the registered ctx word verbatim" ); assert_eq!(got_src, src, "callback must receive the original source"); + assert_eq!(got_service, 0x1234, "decoded service id"); + assert_eq!(got_method, 0x0001, "decoded method id"); + assert_eq!( + got_payload, + [0xDE, 0xAD, 0xBE, 0xEF], + "payload must be the bytes after the 16-byte header" + ); + assert_eq!(got_e2e, 0, "server-side requests are not E2E-checked today"); handle.abort(); let _ = handle.await; } +/// A registered observer must NOT fire for an SD message arriving on +/// the unicast socket — SD-formatted unicast traffic (e.g. unicast +/// FindService) routes to SD handling. Unlike the pre-PR-1 negative +/// test, the witness callback IS registered, so a routing regression +/// (SD datagrams leaking to the observer) trips the assertion. #[tokio::test] -async fn non_sd_observer_none_preserves_ignore_behavior() { - let pipe = Arc::new(MockPipe::default()); +async fn non_sd_observer_ignores_sd_message_on_unicast_socket() { + let unicast_pipe = Arc::new(MockPipe::default()); let factory = MockFactory { - pipe: Arc::clone(&pipe), + unicast_pipe: Arc::clone(&unicast_pipe), + sd_pipe: Arc::new(MockPipe::default()), next_port: Arc::new(Mutex::new(0)), }; let e2e_handle: Arc> = Arc::new(Mutex::new(E2ERegistry::new())); - let subs = MockSubscriptions::default(); + let config = ServerConfig::new(0x1234, 1) + .with_interface(Ipv4Addr::LOCALHOST) + .with_local_port(30702); + + let deps: ServerDeps>, MockSubscriptions> = + ServerDeps { + factory, + timer: MockTimer, + e2e_registry: e2e_handle, + subscriptions: MockSubscriptions::default(), + non_sd_observer: Some((record_sd_unicast as NonSdRequestCallback, 7)), + }; + + let (_server, _handles, run): ( + Server>, MockSubscriptions>, + _, + _, + ) = Server::new_with_deps(deps, config, false) + .await + .expect("Server::new_with_deps must succeed"); + let handle = tokio::spawn(run); + + let src = SocketAddrV4::new(Ipv4Addr::new(192, 0, 2, 102), 40002); + unicast_pipe + .inbound + .lock() + .unwrap() + .push_back((build_sd_message(), src)); + if let Some(w) = unicast_pipe.inbound_waker.lock().unwrap().take() { + w.wake(); + } + // Deterministic completion signal: wait until the run-future has + // consumed the datagram from the pipe, then yield once more. The + // observer path has no await point between dequeue and callback, + // so any leaked invocation has already happened by now. + drive_until(|| unicast_pipe.inbound.lock().unwrap().is_empty()).await; + tokio::task::yield_now().await; + assert!( + !handle.is_finished(), + "run-future must still be alive after processing the datagram" + ); + + let observed = OBSERVED_SD_UNICAST + .get() + .and_then(|m| m.lock().unwrap().clone()); + assert!( + observed.is_none(), + "observer must NOT fire for SD messages; got {observed:?}" + ); + handle.abort(); + let _ = handle.await; +} + +/// A registered observer must NOT fire for a non-SD datagram arriving +/// on the SD/multicast socket — the observer contract is unicast-only +/// (`from_unicast == true`). +#[tokio::test] +async fn non_sd_observer_ignores_non_sd_on_multicast_socket() { + let sd_pipe = Arc::new(MockPipe::default()); + let factory = MockFactory { + unicast_pipe: Arc::new(MockPipe::default()), + sd_pipe: Arc::clone(&sd_pipe), + next_port: Arc::new(Mutex::new(0)), + }; + + let e2e_handle: Arc> = Arc::new(Mutex::new(E2ERegistry::new())); let config = ServerConfig::new(0x1234, 1) .with_interface(Ipv4Addr::LOCALHOST) - .with_local_port(30701); + .with_local_port(30703); + + let deps: ServerDeps>, MockSubscriptions> = + ServerDeps { + factory, + timer: MockTimer, + e2e_registry: e2e_handle, + subscriptions: MockSubscriptions::default(), + non_sd_observer: Some((record_multicast as NonSdRequestCallback, 9)), + }; - // Same `record_none` is installed as a witness that the path the - // observer would have taken is NOT walked when the field is None. - // The pipe still receives the datagram and the recv_loop still - // parses it — but the `else if from_unicast` arm with `None` - // falls through to the trace log, never invoking the callback. - // We don't actually wire the function pointer into the server - // (deps.non_sd_observer = None), but we keep a `record_none` so a - // future regression that accidentally fires *any* observer would - // populate OBSERVED_NONE and trip the assertion below. - let _kept_alive_to_witness_no_invocation: NonSdRequestCallback = record_none; + let (_server, _handles, run): ( + Server>, MockSubscriptions>, + _, + _, + ) = Server::new_with_deps(deps, config, false) + .await + .expect("Server::new_with_deps must succeed"); + let handle = tokio::spawn(run); + + let src = SocketAddrV4::new(Ipv4Addr::new(192, 0, 2, 103), 40003); + sd_pipe + .inbound + .lock() + .unwrap() + .push_back((build_method_request(0x1234, 0x0001, &[]), src)); + if let Some(w) = sd_pipe.inbound_waker.lock().unwrap().take() { + w.wake(); + } + + // Deterministic completion signal: wait until the run-future has + // consumed the datagram from the pipe, then yield once more. The + // observer path has no await point between dequeue and callback, + // so any leaked invocation has already happened by now. + drive_until(|| sd_pipe.inbound.lock().unwrap().is_empty()).await; + tokio::task::yield_now().await; + assert!( + !handle.is_finished(), + "run-future must still be alive after processing the datagram" + ); + + let observed = OBSERVED_MULTICAST + .get() + .and_then(|m| m.lock().unwrap().clone()); + assert!( + observed.is_none(), + "observer must NOT fire for non-unicast datagrams; got {observed:?}" + ); + handle.abort(); + let _ = handle.await; +} + +/// With `non_sd_observer: None`, a non-SD unicast datagram is processed +/// without panicking (historical "ignore" behavior). This is all the +/// `None` case can actually prove — there is no callback to witness. +#[tokio::test] +async fn non_sd_observer_none_preserves_ignore_behavior() { + let unicast_pipe = Arc::new(MockPipe::default()); + let factory = MockFactory { + unicast_pipe: Arc::clone(&unicast_pipe), + sd_pipe: Arc::new(MockPipe::default()), + next_port: Arc::new(Mutex::new(0)), + }; + + let e2e_handle: Arc> = Arc::new(Mutex::new(E2ERegistry::new())); + let config = ServerConfig::new(0x1234, 1) + .with_interface(Ipv4Addr::LOCALHOST) + .with_local_port(30701); let deps: ServerDeps>, MockSubscriptions> = ServerDeps { factory, timer: MockTimer, e2e_registry: e2e_handle, - subscriptions: subs, + subscriptions: MockSubscriptions::default(), non_sd_observer: None, }; @@ -532,37 +758,30 @@ async fn non_sd_observer_none_preserves_ignore_behavior() { ) = Server::new_with_deps(deps, config, false) .await .expect("Server::new_with_deps must succeed"); - let handle = tokio::spawn(run); - let payload = build_method_request(0x1234, 0x0001); let src = SocketAddrV4::new(Ipv4Addr::new(192, 0, 2, 101), 40001); - pipe.inbound + unicast_pipe + .inbound .lock() .unwrap() - .push_back((payload.clone(), src)); - if let Some(w) = pipe.inbound_waker.lock().unwrap().take() { + .push_back((build_method_request(0x1234, 0x0001, &[]), src)); + if let Some(w) = unicast_pipe.inbound_waker.lock().unwrap().take() { w.wake(); } - // Let the run-future poll the inbound queue enough times to dequeue - // and process the datagram. Since the path doesn't invoke any - // callback, there's no positive signal — we just need to give the - // recv_loop a window to act, then confirm OBSERVED_NONE stayed empty. - for _ in 0..50 { - tokio::task::yield_now().await; - } - // Belt-and-braces: also wait a real tick so the unicast `select_biased!` - // arm cycles at least once. - tokio::time::sleep(Duration::from_millis(10)).await; + // Deterministic completion signal: wait until the run-future has + // consumed the datagram from the pipe, then yield once more. The + // observer path has no await point between dequeue and callback, + // so any leaked invocation has already happened by now. + drive_until(|| unicast_pipe.inbound.lock().unwrap().is_empty()).await; + tokio::task::yield_now().await; - let observed = OBSERVED_NONE.get().and_then(|m| m.lock().unwrap().clone()); assert!( - observed.is_none(), - "callback must NOT fire when non_sd_observer is None; got {:?}", - observed + !handle.is_finished(), + "run-future must keep running (no panic / no error) after \ + ignoring a non-SD datagram with no observer registered" ); - handle.abort(); let _ = handle.await; }