Switch to existing collections to heapless, hide other non-embedded f…#76
Closed
JustinKovacich wants to merge 12 commits into
Closed
Switch to existing collections to heapless, hide other non-embedded f…#76JustinKovacich wants to merge 12 commits into
JustinKovacich wants to merge 12 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR moves several client/server runtime data structures from unbounded std::collections to fixed-capacity heapless collections to better support constrained/bare-metal use cases, and documents the resulting saturation behavior.
Changes:
- Replace server subscription storage with
heapless::FnvIndexMap+heapless::Vecand add capacity overflow tests. - Replace client session tracking, pending response tracking, unicast socket tracking, and request queue buffering with fixed-capacity
heaplessstructures and add overflow/capacity tests. - Expand client documentation around memory footprint and saturation, and add
Error::Capacity(plus#[non_exhaustive]).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/subscription_manager.rs | Switch subscriptions map/list to fixed-capacity heapless collections; add capacity overflow tests. |
| src/client/session.rs | Replace session state HashMap with bounded FnvIndexMap; add capacity overflow test and docs. |
| src/client/mod.rs | Add memory footprint + saturation behavior documentation for the client API. |
| src/client/inner.rs | Replace queue/maps with heapless Deque/FnvIndexMap; add capacity checks and tests. |
| src/client/error.rs | Add Error::Capacity and mark the error enum #[non_exhaustive]. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JustinKovacich
added
documentation
This was referenced Apr 22, 2026
JustinKovacich
added a commit
that referenced
this pull request
Apr 23, 2026
- inner.rs: on request_queue overflow, reject the ControlMessage by
sending Err(Error::Capacity("request_queue")) on every oneshot it
carries (via new ControlMessage::reject_with_capacity helper) rather
than silently dropping it — the drop previously cancelled the oneshot
and panicked callers awaiting with .unwrap().
- inner.rs: on pending_responses.insert saturation, destructure the
returned (request_id, response) and send
Err(Error::Capacity("pending_responses")) on the response sender;
previously the sender was dropped, panicking PendingResponse::response.
- mod.rs: update the send_to_service saturation doc to match the new
explicit-error behavior.
- mod.rs: fix the module-header footprint doc — SESSION_CAP lives in
session.rs, not inner.rs.
- session.rs: gate the saturation warn! behind a one-shot saturation_warned
latch; previously the warning fired on every check() against every new
key once the map was full, which meant log-spam at the packet rate.
- error.rs: drop #[non_exhaustive] on client::Error; downstream crates
relying on exhaustive matches shouldn't silently break before a planned
breaking release.
New tests: reject_with_capacity_notifies_every_sender covers every
ControlMessage variant (including SendToService's dual senders);
capacity_overflow_warns_only_on_first_hit locks in the saturation latch.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JustinKovacich
force-pushed
the
feature/move_vec_behind_feature_gate
branch
from
303f070 to
eb29f43
Compare
JustinKovacich
added a commit
that referenced
this pull request
Apr 23, 2026
Round-2 Copilot feedback on PR #76 caught 5 resolvable issues (6th rejected — see below): - src/client/inner.rs: the three `push_front(...).ok()` re-enqueue sites (SetInterface x2, SendSD, Subscribe) silently dropped the returned Err. Swapped each to `if let Err(rejected) = ... { ... rejected.reject_with_capacity("request_queue"); }`, matching the primary `push_back` overflow arm. These branches are defensive — by construction the slot we just popped is free — but a future refactor that changes queue usage would otherwise reintroduce the silent-drop-of-oneshot-senders regression cb1d0d1 was specifically written to prevent. - src/server/subscription_manager.rs: `list.push(...).ok()` on a freshly-allocated `SubscribersList` (first subscriber in a new event group) swapped to `.expect(...)` with a message naming the `SUBSCRIBERS_PER_GROUP >= 1` invariant. Tripwires a future cap-0 regression at test time instead of silently losing the only subscriber. - src/client/session.rs: reword the `SessionTracker` doc comment's reference to non-existent "module docs" — point directly at the `SESSION_CAP` constant instead. Rejected: src/client/error.rs `Capacity` variant as a breaking exhaustive-match break. This is consistent with the earlier decision recorded on #75/#80 to accept the breaking change rather than carry deprecated shims or wrap everything in `Error::Io` — the current release is a breaking version bump by design, and hiding the variant in `Io` would lose the lowercase-snake_case tag semantics the new error was specifically designed to carry. No production behavior changes — just defensive tripwires, docs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/client/session.rs:90
SessionTracker::check’s doc comment says it “Always updates the stored state after the check”, but when the fixed-capacity map is saturated and a new key is dropped, the state is not stored/updated for that key. Please adjust the docs to reflect the saturation behavior (or adjust the implementation if you intend the invariant to hold).
impl SessionTracker {
/// Check the session ID and reboot flag for a specific service instance
/// and return a verdict. Always updates the stored state after the check.
///
/// Call this once per service entry in an SD message (not once per message),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JustinKovacich
added a commit
that referenced
this pull request
Apr 24, 2026
Copilot round-3 on PR #76 (comment 3138669839) caught a real gap in `track_or_reject_pending_response`: the helper only handled `Err((key, value))` (map-full + new-key), but `heapless::IndexMap:: insert` on an existing key returns `Ok(Some(old_sender))` and the old sender was silently dropped. If `request_id` is ever reused while an older pending entry is still live — the documented example is `session_counter` wrap-around — the caller awaiting the original request would see a `RecvError` on channel cancellation, which `PendingResponse::response()` turns into a panic. Reworked the `if let Err(...)` into a full `match` with all three arms: - `Ok(None)` — normal insert, nothing to do. - `Ok(Some(displaced))` — `warn!` that the slot was replaced, complete the displaced sender with `Err(Error::Capacity("pending_responses"))` so the original caller gets a clean `Result`, not a `RecvError` panic. - `Err((_req_id, r))` — existing saturation path (unchanged). Also updated the doc comment on `track_or_reject_pending_response` to describe the displacement contract. Coverage: new test `track_or_reject_pending_response_completes_displaced_sender` explicitly inserts the same key twice and asserts the first receiver resolves to `Err(Error::Capacity("pending_responses"))` before the second sender still sits pending in the map. Addresses Copilot comment 3138669839. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JustinKovacich
added a commit
that referenced
this pull request
Apr 24, 2026
…ubscribe docs - CHANGELOG.md: drop the two "Changed" bullets about `std` becoming the default feature and `thiserror`/`tracing` moving to `default-features = false`. Those landed in 0.6.0/0.6.1 (commit f161980, already on main) and are not changes made in this PR. - server/mod.rs: when a SubscribeError rejects a subscription, match on the variant and pass a specific `&'static str` reason to `send_subscribe_nack_from_view` (`"subscribers_per_group_full"` / `"event_groups_full"`) instead of the generic `"subscription rejected"`. The NACK log line now reflects the real cause, and the static-str choice avoids any String-across-await allocation. - server/subscription_manager.rs: rewrite the `subscribe` docs to say the duplicate path is idempotent / deduplicated rather than implying TTL-refresh semantics that don't exist today. Flag the future-TTL extension point explicitly so the doc and the log wording stay in sync if that behavior is added later. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JustinKovacich
force-pushed
the
feature/firmware_someip_conversion
branch
from
b5927b7 to
7f975cc
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…eatures behind gates
- inner.rs: on request_queue overflow, reject the ControlMessage by
sending Err(Error::Capacity("request_queue")) on every oneshot it
carries (via new ControlMessage::reject_with_capacity helper) rather
than silently dropping it — the drop previously cancelled the oneshot
and panicked callers awaiting with .unwrap().
- inner.rs: on pending_responses.insert saturation, destructure the
returned (request_id, response) and send
Err(Error::Capacity("pending_responses")) on the response sender;
previously the sender was dropped, panicking PendingResponse::response.
- mod.rs: update the send_to_service saturation doc to match the new
explicit-error behavior.
- mod.rs: fix the module-header footprint doc — SESSION_CAP lives in
session.rs, not inner.rs.
- session.rs: gate the saturation warn! behind a one-shot saturation_warned
latch; previously the warning fired on every check() against every new
key once the map was full, which meant log-spam at the packet rate.
- error.rs: drop #[non_exhaustive] on client::Error; downstream crates
relying on exhaustive matches shouldn't silently break before a planned
breaking release.
New tests: reject_with_capacity_notifies_every_sender covers every
ControlMessage variant (including SendToService's dual senders);
capacity_overflow_warns_only_on_first_hit locks in the saturation latch.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Commit cb1d0d1 added explicit capacity-error delivery when pending_responses.insert overflows — without the explicit Err send, the dropped Sender would cause PendingResponse::response().await to panic on RecvError instead of surfacing a clean Result. The recovery logic was inline in the SendToService run-loop arm, which made it hard to exercise without driving 64 live sockets. Lift it to a small `track_or_reject_pending_response` helper on Inner so the SendToService arm delegates, and the branch is testable against the exposed `pending_responses` map directly. Added two tests: - `track_or_reject_pending_response_inserts_when_room_available`: happy path — entry lands in the map, the sender is still live (receiver stays pending). - `track_or_reject_pending_response_rejects_on_saturation`: fill map to PENDING_RESPONSES_CAP, invoke the helper with one more request, assert the map is unchanged and the caller's receiver resolves to Err(Error::Capacity("pending_responses")) — which is the invariant cb1d0d1 guarantees. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Round-2 Copilot feedback on PR #76 caught 5 resolvable issues (6th rejected — see below): - src/client/inner.rs: the three `push_front(...).ok()` re-enqueue sites (SetInterface x2, SendSD, Subscribe) silently dropped the returned Err. Swapped each to `if let Err(rejected) = ... { ... rejected.reject_with_capacity("request_queue"); }`, matching the primary `push_back` overflow arm. These branches are defensive — by construction the slot we just popped is free — but a future refactor that changes queue usage would otherwise reintroduce the silent-drop-of-oneshot-senders regression cb1d0d1 was specifically written to prevent. - src/server/subscription_manager.rs: `list.push(...).ok()` on a freshly-allocated `SubscribersList` (first subscriber in a new event group) swapped to `.expect(...)` with a message naming the `SUBSCRIBERS_PER_GROUP >= 1` invariant. Tripwires a future cap-0 regression at test time instead of silently losing the only subscriber. - src/client/session.rs: reword the `SessionTracker` doc comment's reference to non-existent "module docs" — point directly at the `SESSION_CAP` constant instead. Rejected: src/client/error.rs `Capacity` variant as a breaking exhaustive-match break. This is consistent with the earlier decision recorded on #75/#80 to accept the breaking change rather than carry deprecated shims or wrap everything in `Error::Io` — the current release is a breaking version bump by design, and hiding the variant in `Io` would lose the lowercase-snake_case tag semantics the new error was specifically designed to carry. No production behavior changes — just defensive tripwires, docs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address two clippy::pedantic warnings introduced by this branch's own commits: - missing_panics_doc on SubscriptionManager::subscribe — the heapless::Vec::push.expect on the first-insert path can only trip if SUBSCRIBERS_PER_GROUP is set to zero; document that. - cast_possible_truncation on 'i as u32' in the saturation test for pending_responses — use u32::try_from with an expect that documents why the 64-cap fits.
Copilot round-3 on PR #76 (comment 3138669839) caught a real gap in `track_or_reject_pending_response`: the helper only handled `Err((key, value))` (map-full + new-key), but `heapless::IndexMap:: insert` on an existing key returns `Ok(Some(old_sender))` and the old sender was silently dropped. If `request_id` is ever reused while an older pending entry is still live — the documented example is `session_counter` wrap-around — the caller awaiting the original request would see a `RecvError` on channel cancellation, which `PendingResponse::response()` turns into a panic. Reworked the `if let Err(...)` into a full `match` with all three arms: - `Ok(None)` — normal insert, nothing to do. - `Ok(Some(displaced))` — `warn!` that the slot was replaced, complete the displaced sender with `Err(Error::Capacity("pending_responses"))` so the original caller gets a clean `Result`, not a `RecvError` panic. - `Err((_req_id, r))` — existing saturation path (unchanged). Also updated the doc comment on `track_or_reject_pending_response` to describe the displacement contract. Coverage: new test `track_or_reject_pending_response_completes_displaced_sender` explicitly inserts the same key twice and asserts the first receiver resolves to `Err(Error::Capacity("pending_responses"))` before the second sender still sits pending in the map. Addresses Copilot comment 3138669839. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The old docstring explained why client::Error is not #[non_exhaustive] but didn't acknowledge that adding variants to a non-#[non_exhaustive] enum is itself a breaking change for downstream exhaustive matches. Rewrite the note to call that out explicitly and flag future #[non_exhaustive] as a planned breaking-release change, and record the new Capacity variant under the Unreleased Added section. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SubscriptionManager::subscribe previously returned () and logged a warn! on capacity failure, which let handle_sd_message send SubscribeAck to a client whose subscription had silently been dropped — the client would believe it was subscribed but never receive any events. - subscribe now returns Result<(), SubscribeError> with SubscribersPerGroupFull / EventGroupsFull variants; existing capacity/refresh semantics are unchanged. - handle_sd_message inspects the result and emits SubscribeNack with a reason derived from the SubscribeError variant on rejection. - EventPublisher::register_subscriber surfaces the same Result so external SD dispatchers can take the same corrective action. - SubscribeError is re-exported from server::mod. - Tests updated to consume the Result; subscription_manager overflow tests now assert the specific error variant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ording - session::check: expand the doc comment with an explicit "Capacity behavior" section — the saturation path does not update stored state for new keys, which contradicts the old "Always updates the stored state" wording. Describe the actual behavior (normal path updates, new-key inserts under saturation are silently dropped and return Initial repeatedly) so callers don't rely on a strict guarantee the implementation doesn't hold. - session::check saturation warn!: include sender + transport in the log line so diagnosing which peer lost reboot-detection state no longer requires out-of-band correlation. - SubscriptionManager::subscribe dedup branch: rename the log from "Refreshed existing subscriber" to "already subscribed; skipping" and add a comment making it explicit that no per-subscriber state is modified. The old wording implied a refresh (TTL bump, etc) the code never performed. - inner.rs test comment: tokio::sync::oneshot doesn't drop the sender when the receiver is dropped — it flips send() to a failure mode. Rewrite the comment to describe the actual stash purpose (keep channels open for later observation). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- server::run NACK arm: stop allocating a String with format!() held across the SubscribeNack await; log the SubscribeError at warn! separately and pass a static "subscription rejected" reason string to send_subscribe_nack_from_view. Cleaner and avoids the borrow- across-await style issue Copilot flagged. - EventPublisher::register_subscriber: add # Errors section describing the two SubscribeError variants, that the subscriber is NOT registered on Err, and that external dispatchers should NACK on Err just like the server's own run() loop does. - CHANGELOG: add server::SubscribeError to Added, and list the breaking signature changes on SubscriptionManager::subscribe and EventPublisher::register_subscriber under Changed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ubscribe docs - CHANGELOG.md: drop the two "Changed" bullets about `std` becoming the default feature and `thiserror`/`tracing` moving to `default-features = false`. Those landed in 0.6.0/0.6.1 (commit f161980, already on main) and are not changes made in this PR. - server/mod.rs: when a SubscribeError rejects a subscription, match on the variant and pass a specific `&'static str` reason to `send_subscribe_nack_from_view` (`"subscribers_per_group_full"` / `"event_groups_full"`) instead of the generic `"subscription rejected"`. The NACK log line now reflects the real cause, and the static-str choice avoids any String-across-await allocation. - server/subscription_manager.rs: rewrite the `subscribe` docs to say the duplicate path is idempotent / deduplicated rather than implying TTL-refresh semantics that don't exist today. Flag the future-TTL extension point explicitly so the doc and the log wording stay in sync if that behavior is added later. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
JustinKovacich
force-pushed
the
feature/move_vec_behind_feature_gate
branch
from
505bf8f to
bceae77
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+100
to
+104
| /// continue to return [`SessionVerdict::Initial`] until an existing | ||
| /// key is evicted or capacity is raised. A single `warn!` fires the | ||
| /// first time saturation is hit (further saturation drops are | ||
| /// suppressed to avoid log spam at the packet rate). For existing | ||
| /// keys under saturation the update still succeeds, because |
There was a problem hiding this comment.
The doc comment says new sender keys will keep returning Initial until an existing key is "evicted", but SessionTracker never evicts/removes entries (and there is no eviction policy documented/implemented). Please reword this to reflect actual behavior (e.g., until capacity is increased or the tracker is reset/cleared), so callers don't assume entries will eventually be evicted automatically.
Suggested change
| /// continue to return [`SessionVerdict::Initial`] until an existing | |
| /// key is evicted or capacity is raised. A single `warn!` fires the | |
| /// first time saturation is hit (further saturation drops are | |
| /// suppressed to avoid log spam at the packet rate). For existing | |
| /// keys under saturation the update still succeeds, because | |
| /// continue to return [`SessionVerdict::Initial`] unless capacity is | |
| /// increased or the tracker state is explicitly reset/cleared. A single | |
| /// `warn!` fires the first time saturation is hit (further saturation | |
| /// drops are suppressed to avoid log spam at the packet rate). For | |
| /// existing keys under saturation the update still succeeds, because |
Contributor
Author
|
Closing without merge to declutter the stack: this phase's changes are carried in full by the consolidated lineage under PR #114 (phase 21), which the next development stack builds on. Branch is retained. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…eatures behind gates
This is a chained PR. Prev: #75. Next: #77