fix(client): per-transport SD session tracking via dual discovery sockets (+ Windows CI)#130
Conversation
Pins the SessionTracker contract the SD transport-attribution fix relies on: interleaved multicast/unicast session domains for the same (service,instance) must not look like a reboot, while a real counter reset within a domain must. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Validates the per-transport SD fix's core assumption: a socket bound to the MULTICAST GROUP address (joined) receives only multicast, and a socket bound to the SPECIFIC host interface IP (not joined) receives only unicast — no cross-delivery. NOTE the spike also found the naive variant fails: binding the unicast socket to INADDR_ANY double-delivers (it receives the multicast too via SO_REUSEPORT), so the unicast socket MUST bind the specific host IP. Skips on hosts with no multicast route (e.g. lo-only CI). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ANY mc + interface-IP unicast) recvmsg/PKTINFO is per-platform cmsg (no Windows/bare-metal story), so dual-socket is the portable mechanism. Bind-to-group (my first spike) is rejected on Windows, so validate the portable variant instead: keep the multicast socket on INADDR_ANY + join (unchanged, still sends) and add a more-specific interface-IP socket for unicast. 'Most-specific bind wins' diverts the sensor's unicast SD to it, so the wildcard socket sees only multicast — no bind-to-group, no send-path change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…kets Splits the sensor's multicast and unicast SD session/reboot domains onto separate SessionTracker keys, fixing the false-reboot churn (interleaved counters looked like perpetual reboots). Mechanism (validated by dual_socket_splits test, portable to Windows + bare-metal Transport trait): - multicast discovery socket: unchanged (INADDR_ANY:30490 + group join, still sends outgoing SD); its receives are now tagged TransportKind::Multicast. - NEW receive-only unicast discovery socket bound to the interface IP — by most-specific-bind-wins the kernel diverts the sensor's unicast SD here; tagged TransportKind::Unicast. The shared process_discovery() feeds both arms, so each transport's session counter is tracked independently. No send-path change. SessionTracker was already correct (keys by (sender, transport, svc, inst)); this stops feeding it a hardcoded Multicast for every datagram. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The per-transport SD fix binds a wildcard multicast socket alongside an interface-IP unicast socket and relies on "most-specific bind wins" to divert the sensor's unicast SD to the unicast socket. That divert is OS-dependent, so add a `windows-latest` job running the full `--all-features` suite (incl. the `client` socket code + `dual_socket_splits_multicast_from_unicast`) to confirm the behavior holds on Windows. Note: the divert test SKIPs (vacuous pass) on hosts with no multicast route / no egress IPv4 — watch the runner log to confirm it actually ran. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes false SenderRebooted churn by tracking SOME/IP-SD session state separately for multicast vs unicast discovery traffic. It does so by introducing a dedicated receive-only unicast SD socket (bound to the interface IP) and threading the true TransportKind into session tracking, plus adds a Windows CI job to validate cross-platform bind behavior.
Changes:
- Add
SocketManager::bind_discovery_unicast()to create an interface-IP-bound SD receive socket. - Refactor SD receive handling in the client loop into a shared
process_discovery()that keysSessionTrackerby real transport (multicast vs unicast). - Add a
windows-latestGitHub Actions job runningcargo test --all-features.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/client/socket_manager.rs |
Adds unicast SD bind helper and a kernel-behavior regression test for multicast/unicast split. |
src/client/inner.rs |
Introduces a second discovery socket and routes SD datagrams through shared processing with correct TransportKind. |
src/client/session.rs |
Adds unit tests documenting/guarding the per-transport session tracking behavior. |
.github/workflows/ci.yml |
Adds a Windows CI job to exercise the socket binding behavior on Windows runners. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…cket test `socket2::Socket::set_reuse_port` is Unix-only; the test helper called it unconditionally, breaking the Windows build (the just-added windows-latest CI job caught it). Production `bind_discovery*` already gates it `#[cfg(unix)]` and compiled fine on Windows — only the test helper was missing the gate. `set_reuse_address` (cross-platform) stays unconditional. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lity `combined_sd_subscribe_uses_its_own_options_run` sent the test datagram to the SD socket's `local_addr()`, which is `0.0.0.0:<port>` (the SD socket binds the wildcard to receive multicast). Sending unicast to `0.0.0.0` loops back on Linux but is dropped on Windows, so the server's `recv_from` timed out — the new windows-latest CI job surfaced this pre-existing Linux-only assumption. Target `127.0.0.1:<port>` explicitly so the test is cross-platform; behavior on Linux is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #130 +/- ##
==========================================
- Coverage 96.55% 96.50% -0.06%
==========================================
Files 30 30
Lines 7993 8144 +151
==========================================
+ Hits 7718 7859 +141
- Misses 275 285 +10
|
…ndows The previous commit's loopback retarget was a no-op: the SD socket binds `config.interface:30490` and the test config's interface is already `127.0.0.1`, so `local_addr()` was never the wildcard. The real cause is that every test server binds the same fixed `127.0.0.1:30490` with `SO_REUSEADDR`; under parallel execution Windows delivers the test's unicast to a different bound socket, so `recv_from` timed out. Drop the socket round-trip entirely: `message` is already in memory and the sender addr isn't asserted (the subscriber endpoint must come from the SubscribeEventGroup's `options[1]`), so parse it directly and call `handle_sd_message` with a synthetic sender. Hermetic, no shared-port dependency, identical assertions. Verified on Linux; behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Windows job's purpose is to confirm the dual-socket SD bind divert is portable; that assertion lives in the library unit tests (`dual_socket_splits_multicast_from_unicast`). Running the full suite also ran `tests/client_server.rs`, whose integration tests bind fixed SD ports and are flaky under parallel execution on every platform — they fail on `main` on the same host and flake on the ubuntu coverage job too (tracked separately, #84). Split into `--no-run` (compile everything incl. integration tests = build portability) + `--lib` (run unit tests incl. the divert = behavior portability). The integration suite still compiles on Windows but isn't run here, so the job reflects this fix's portability without unrelated cross-platform flakiness. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Bind the multicast probe socket to an ephemeral port (:0) and reuse the OS-assigned port for the unicast socket + sender target, instead of a hardcoded 51900 that could collide on a shared CI runner. - Treat probe send_to failures as an explicit, logged SKIP (env issue, not a logic regression) rather than relying on an empty drain to pass. - Fix inaccurate assertion message: the mc socket is bound to INADDR_ANY, not the group address. - Clarify bind_discovery_unicast doc: it is receive-only by usage, not by type (the returned SocketManager still carries an unused send half). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-port self-collision The client's receive-only unicast discovery socket binds interface:30490 with SO_REUSEPORT. When the integration-test server bound its SD socket on the same IP:30490, the client's own SubscribeEventgroup could be SO_REUSEPORT load-balanced to the client's own discovery socket, so the server never registered the subscriber and wait_for_subscribers failed (client_server.rs:172). This is a test-only artifact: in production a full SD-announcing server is a remote sensor on its own IP, and the co-located server in iris_someip_client is new_passive (binds an ephemeral SD port, never 30490). So the collision only arises when a full Server::new is co-located on the same IP as a client, which only the integration test did. Move the test server to 127.0.0.2 (client stays 127.0.0.1), mirroring the remote-sensor topology. The serial-sd-port nextest group is retained — multiple client_server tests each bind their server's :30490 and must stay serial vs each other. Full suite passes via nextest (423/423). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed Root cause of the coverage-job failure ( Why it's production-safe (not a real regression): the collision only occurs when a full SD-announcing Fix: move the test server to Local verification: This supersedes the earlier "pre-existing flake / serial-group / PR #84" diagnosis in the threads above — it was neither pre-existing nor a parallelism/serial-group issue; it's an intra-test self-collision introduced by |
…g fix Add `most_specific_bind_diverts_unicast_on_loopback`, a deterministic companion to `dual_socket_splits_multicast_from_unicast`. The multicast variant needs a live multicast route and SKIPs on a lo-only host (CI), leaving the "most-specific bind wins" divert — the load-bearing mechanism of the per-transport SD fix — unexercised in coverage and prone to the vacuous-skip trap. The new test proves the same divert with pure unicast on loopback (ephemeral port, never the fixed SD port), so it never skips: a datagram to the interface IP lands on the interface-bound socket, not the wildcard socket sharing the port. Unix-gated (leans on SO_REUSEPORT, the discovery socket's own #[cfg(unix)] reuse path); cross-platform compilation stays covered by the build-all CI job. Also fix the lone `SubscribeEventgroup` -> `SubscribeEventGroup` (capital G) typo in the client_server doc comment to match the spec spelling used everywhere else in the crate (Copilot review). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The companion test bound `0.0.0.0:<ephemeral>` with `SO_REUSEPORT`, which can collide with a parallel integration-test client that the OS hands the same ephemeral port: that client's `bind` (REUSEADDR only, no REUSEPORT) then fails with `EADDRINUSE` and its test fails. This surfaced as a `test_multiple_ subscribers_receive_events` failure on the coverage job. The production `bind_discovery_unicast` is already covered via the `inner.rs` discovery tests, so the marginal coverage isn't worth the cross-test hazard. The `SubscribeEventGroup` spelling fix (prior commit) stays. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The SD-port-collision comment said the client's unicast discovery socket uses `SO_REUSEPORT`, but `bind_discovery_unicast` only sets it under `#[cfg(unix)]` (Windows gets `SO_REUSEADDR` only). Reword to "address/port reuse — SO_REUSEPORT on Unix" so the comment is accurate cross-platform (Copilot review). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… update The coverage job intermittently failed `test_multiple_subscribers_receive_events` with `client1 expected Unicast, got DiscoveryUpdated(SubscribeAckEventGroup)`. Root cause: this PR's unicast SD socket now delivers the SubscribeAck to the client as a `DiscoveryUpdated` on the same update channel as events. The tests did a single 250ms pre-drain then asserted the *next* update was the `Unicast` event — racy, because under the slower coverage build the ack can arrive after the pre-drain and get consumed where the event is expected. Add a `recv_unicast` helper that drains the update stream until the `Unicast` event arrives, skipping interleaved discovery acks / reboot notices (bounded by a 5s deadline). Apply it to every test that asserts event receipt. The opportunistic pre-drains are kept (harmless) and the helper makes ordering no longer load-bearing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Forward-ports #130 (released on the 0.7.x line, never ported to the 0.8.0 spine) onto the post-#125 client architecture. The spine's discovery-loop rewrite dropped #130's dual-socket split, so 0.8.0 regressed the false-reboot fix: a sensor's interleaved multicast (~1468) and unicast (~739) SD session counters collided on one SessionTracker key and looked like perpetual reboots. Mechanism (re-applied to the generic TransportFactory / BindDispatch / Spawner surface, not the old raw-socket2 path): - `SocketManager::bind_discovery_unicast_with_transport[_local]`: a receive-only unicast SD socket bound to the *specific* interface IP on the SD port (vs the multicast socket's INADDR_ANY), no group join. "Most-specific bind wins" diverts the sensor's unicast SD here. - `BindDispatch::bind_discovery_unicast`: claims a buffer lease and binds via the Send / Local dispatch (one extra socket-loop buffer per client). - `Inner`: new `discovery_unicast_socket`, best-effort bound in `bind_discovery` (failure is non-fatal — multicast still works), shut down in `unbind_discovery`. The discovery-handling block is extracted to `Self::handle_discovery_datagram` and driven from both the multicast arm (TransportKind::Multicast) and a new unicast arm (TransportKind::Unicast). The spine's SessionTracker already keys by transport, so no engine change there. Tests: port #130's two per-transport SessionTracker regression tests, the dual_socket_splits kernel-behaviour spike, the hermetic combined-SD server test, and the client_server hardening this fix requires (server on 127.0.0.2 so the client's unicast SD socket can't steal its own SubscribeAck; `recv_unicast` drains discovery acks before asserting events). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`SO_REUSEPORT` / socket2's `set_reuse_port` is Unix-only and does not compile on Windows. The Windows CI job compiles the integration suite (`--no-run`); this spine-era test (added post-#130, which had gated its own `set_reuse_port` calls) was the lone unguarded call site — every `set_reuse_port` in `src/` is already `#[cfg(unix)]`-guarded. The test is `#[ignore]`'d and only runs on Linux CI, so the Windows build just needs it to compile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Forward-ports #130 (released on the 0.7.x line, never ported to the 0.8.0 spine) onto the post-#125 client architecture. The spine's discovery-loop rewrite dropped #130's dual-socket split, so 0.8.0 regressed the false-reboot fix: a sensor's interleaved multicast (~1468) and unicast (~739) SD session counters collided on one SessionTracker key and looked like perpetual reboots. Mechanism (re-applied to the generic TransportFactory / BindDispatch / Spawner surface, not the old raw-socket2 path): - `SocketManager::bind_discovery_unicast_with_transport[_local]`: a receive-only unicast SD socket bound to the *specific* interface IP on the SD port (vs the multicast socket's INADDR_ANY), no group join. "Most-specific bind wins" diverts the sensor's unicast SD here. - `BindDispatch::bind_discovery_unicast`: claims a buffer lease and binds via the Send / Local dispatch (one extra socket-loop buffer per client). - `Inner`: new `discovery_unicast_socket`, best-effort bound in `bind_discovery` (failure is non-fatal — multicast still works), shut down in `unbind_discovery`. The discovery-handling block is extracted to `Self::handle_discovery_datagram` and driven from both the multicast arm (TransportKind::Multicast) and a new unicast arm (TransportKind::Unicast). The spine's SessionTracker already keys by transport, so no engine change there. Tests: port #130's two per-transport SessionTracker regression tests, the dual_socket_splits kernel-behaviour spike, the hermetic combined-SD server test, and the client_server hardening this fix requires (server on 127.0.0.2 so the client's unicast SD socket can't steal its own SubscribeAck; `recv_unicast` drains discovery acks before asserting events). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`SO_REUSEPORT` / socket2's `set_reuse_port` is Unix-only and does not compile on Windows. The Windows CI job compiles the integration suite (`--no-run`); this spine-era test (added post-#130, which had gated its own `set_reuse_port` calls) was the lone unguarded call site — every `set_reuse_port` in `src/` is already `#[cfg(unix)]`-guarded. The test is `#[ignore]`'d and only runs on Linux CI, so the Windows build just needs it to compile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…criber test after #134 rebase bf21b2c made the SOME/IP server caps env-configurable but lowered the *defaults* (SUBSCRIBERS_PER_GROUP 16->1, EVENT_GROUPS_CAP 32->4, ServerConfig EVENT_GROUP_IDS_CAP 32->1, ACCEPTED_OFFERS_CAP 16->4) for every build. Those consts are shared with the std/tokio server, so plain std consumers that never inject SIMPLE_SOMEIP_MAX_* were silently capped (e.g. one subscriber per event group), and 11 lib unit tests + the multi-subscriber integration test went red. - Gate the fallback defaults by feature: bare-metal keeps the tight values (the host build injects exact sizes via the env vars — the memory win), std builds keep the historical generous defaults. Env override still applies to both via from_env_or(option_env!(...)). - Expose ServerConfig::SUBSCRIBERS_PER_GROUP_CAP so callers/tests can adapt to the build-time capacity. - from_env_or: fix clippy::pedantic (doc backticks, manual_assert, i32->usize sign-loss via is_ascii_digit) and gate on `server` (its only callers) to drop the dead_code warning on client-only / bare_metal-only builds. Test fixes (rebase artifacts from porting onto #134): - test_multiple_subscribers_receive_events / test_client_send_sd_auto_binds_discovery sent to Ipv4Addr::LOCALHOST while create_server binds SERVER_IP (127.0.0.2, the #130 dual-socket split) -> subscribes hit a dead address, 0 registered. Restore SERVER_IP. Also gate the second-subscriber assertion on SUBSCRIBERS_PER_GROUP_CAP so it passes at the bare-metal default of 1 and exercises the real fan-out under SIMPLE_SOMEIP_MAX_SUBS>=2. - inner.rs: restore the discovery_unicast_socket teardown in unbind_discovery dropped during the rebase. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…criber test after #134 rebase bf21b2c made the SOME/IP server caps env-configurable but lowered the *defaults* (SUBSCRIBERS_PER_GROUP 16->1, EVENT_GROUPS_CAP 32->4, ServerConfig EVENT_GROUP_IDS_CAP 32->1, ACCEPTED_OFFERS_CAP 16->4) for every build. Those consts are shared with the std/tokio server, so plain std consumers that never inject SIMPLE_SOMEIP_MAX_* were silently capped (e.g. one subscriber per event group), and 11 lib unit tests + the multi-subscriber integration test went red. - Gate the fallback defaults by feature: bare-metal keeps the tight values (the host build injects exact sizes via the env vars — the memory win), std builds keep the historical generous defaults. Env override still applies to both via from_env_or(option_env!(...)). - Expose ServerConfig::SUBSCRIBERS_PER_GROUP_CAP so callers/tests can adapt to the build-time capacity. - from_env_or: fix clippy::pedantic (doc backticks, manual_assert, i32->usize sign-loss via is_ascii_digit) and gate on `server` (its only callers) to drop the dead_code warning on client-only / bare_metal-only builds. Test fixes (rebase artifacts from porting onto #134): - test_multiple_subscribers_receive_events / test_client_send_sd_auto_binds_discovery sent to Ipv4Addr::LOCALHOST while create_server binds SERVER_IP (127.0.0.2, the #130 dual-socket split) -> subscribes hit a dead address, 0 registered. Restore SERVER_IP. Also gate the second-subscriber assertion on SUBSCRIBERS_PER_GROUP_CAP so it passes at the bare-metal default of 1 and exercises the real fan-out under SIMPLE_SOMEIP_MAX_SUBS>=2. - inner.rs: restore the discovery_unicast_socket teardown in unbind_discovery dropped during the rebase. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
Per-transport SD session tracking via dual discovery sockets, plus a
windows-latestCI job to confirm the bind behavior is cross-platform.Previously the client tracked a single SD session-counter domain across both multicast and unicast Service Discovery, so the two domains' counters collided and produced spurious
SenderRebootedchurn against a steadily-running sensor.This change:
INADDR_ANY:30490+ group join, still sends),TransportKindintoSessionTracker::check()via a sharedprocess_discovery(), so the two session domains are tracked independently and stop colliding.No bind-to-group (Windows-safe), no change to the send path.
Verification
Live, on real hardware (Iris
192.168.11.151, FW 0.11.0.3, via thedft/EnVision consumer with this branch patched in):tcpdumpon the SD port captured both 96 multicast offers and 31 unicast SD datagrams; the unicast SD landed on<iface>:30490(the new socket) as designed.SenderRebootedover a 60s steady-state window; stable scan profile.CI: Windows portability
The unicast divert ("most-specific bind wins") is the OS-dependent part of this design. simple_someip CI was ubuntu-only, so this PR adds a
test-windows(windows-latest) job runningcargo test --all-features— which builds theclientsocket code and runssocket_manager::dual_socket_splits_multicast_from_unicast(real binds + send/drain asserting the divert).Follow-ups (not in this PR)
0.7.1) so thedftconsumer can drop its local[patch]and pick this up.iris_someip_client'sservice_instances()override (dft side) so SD reboot detection actually runs.🤖 Generated with Claude Code