Skip to content

PR 3: #125 server buffer extraction — caller-sized send buffers (run 7696→3528 B, publish ~3320→320 B)#133

Closed
JustinKovacich wants to merge 8 commits into
feature/pr2_125_client_async_statefrom
feature/pr3_125_server_buffers
Closed

PR 3: #125 server buffer extraction — caller-sized send buffers (run 7696→3528 B, publish ~3320→320 B)#133
JustinKovacich wants to merge 8 commits into
feature/pr2_125_client_async_statefrom
feature/pr3_125_server_buffers

Conversation

@JustinKovacich

Copy link
Copy Markdown
Contributor

PR 3 — #125 server buffer extraction + final numbers

Final PR in the #125 memory-reduction stack (PR 0 measurement #127 → PR 1 #124-follow-ups #129 → PR 2 client #132this). Stacked on #132 — review/merge that first; this PR's diff is only its own 7 commits. Closes issue #125 (server half).

What this does

Moves the server's 6 future-resident [u8; UDP_BUFFER_SIZE] send-path buffers out of the run/publish futures into caller-provided scratch — matching the server's existing caller-buffer receive model (run_with_buffers), not a pool. The server runs one combined future (run_combined) + an app publish path with no dynamic socket spawning, so claim/release is unnecessary; fixed caller scratch is the right fit.

Results (bare-metal future sizes)

Future Before After
server run (run_combined) 7696 B 3528 B
server publish (publish_event) ~3320 B 320 B

Combined with PR 2 (client socket-loop 2224 → 776 B), issue #125's arena-exhaustion failure mode is addressed end to end: per-socket and per-send buffers are now consumer-sized in .bss, not parked in TaskStorage.

Mechanism

  • The 3 SD send helpers + SdStateManager::send_offer_service take &mut [u8]; encode/E2E bounds key off buf.len().
  • Server::run_with_buffers gains two distinct send-scratch params (recv-path + announce-path — they run concurrently under select, and the four &mut [u8] params make sharing a compile error, so non-aliasing is type-enforced).
  • EventPublisher::publish_event_with_buffers / publish_raw_event_with_buffers take caller scratch.
  • New public Server::announce_only_with_buffer for bare-metal supplementary (announce-only) servers.
  • tokio paths are unchanged: Server::run, Server::new, EventPublisher::publish_event/publish_raw_event, and announce_only_future allocate the scratch internally (_alloc-gated), so existing callers compile untouched. client,bare_metal stays alloc-free (nm-verified).

Behavioral changes (only these; wire format untouched)

  • Over-capacity scratch is rejected with Error::Capacity("udp_buffer") (previously unreachable because the buffers were full-size).
  • Fixed an E2E-protect out-of-bounds panic on the publish path — the post-protect guard checked UDP_BUFFER_SIZE instead of the actual buffer length, so a caller-sized buffer smaller than 1500 B would OOB-panic the publish on an E2E-protected event. Same bug class PR 2 fixed on the client send path; regression-tested here.

Breaking (free pre-0.8.0 publish)

run_with_buffers, publish_event_with_buffers/publish_raw_event_with_buffers, and the new announce_only_with_buffer are the bare-metal signatures that changed; the tokio convenience methods are unchanged. CHANGELOG updated.

Verification

  • bare_metal_e2e 9/9 (incl. server-send-undersized, E2E-publish-undersized, raw-publish-undersized regressions + the two new witnesses).
  • cargo nextest (client-tokio,server-tokio) 521; cargo test --no-default-features clean.
  • cargo clippy workspace --all-features + --no-default-features clean.
  • cargo build --target thumbv7em-none-eabihf for client,bare_metal (alloc-free) / server,bare_metal / client,server,bare_metal.
  • cargo doc (client / server,bare_metal / all-features, -D warnings) + cargo test --doc clean.

Reviewed per-task and end-to-end (adversarial whole-branch pass, which caught the residual announce_only_future buffer — now closed).

🤖 Generated with Claude Code

JustinKovacich and others added 7 commits June 17, 2026 14:28
…inal numbers)

6 future-resident server send-path buffers (3 SD helpers, send_offer_service,
publish_event x2 incl. E2E protected, publish_raw_event) move out of the run/
publish futures into caller-provided scratch — matching the server's existing
caller-buffer receive model (run_with_buffers), NOT the client's pool (the
server runs one combined future, no dynamic socket spawning). Run path gets two
send-scratch buffers (recv + announce can be mid-send concurrently under select);
tokio allocates internally so the public API is unchanged; E2E bounds checks key
off buf.len() (the PR 2 lesson). Closes #125 with final client+server numbers.
Stacked on PR 2.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#125 PR3 T1)

The 3 SD send helpers (send_unicast_offer, send_subscribe_ack/nack_from_view)
take a &mut [u8] scratch instead of a future-resident [u8; UDP_BUFFER_SIZE];
over-capacity now returns Error::Capacity("udp_buffer"). handle_sd_message
threads one reused scratch for now (Task 3 threads the real caller buffer).
+7 helper-level unit tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ers; run future 7696->3528 B (#125 PR3 T2+T3)

send_offer_service + the 3 SD helpers now write into caller-provided scratch
threaded via run_with_buffers (two distinct &mut [u8], borrowck-guaranteed
non-aliasing across the concurrent recv/announce loops). tokio run_inner
allocates them internally so Server::run is unchanged; bare-metal callers
pass their own. Dead post-encode checks -> debug_assert.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…uard on buf.len() (#125 PR3 T4)

publish_event_with_buffers / publish_raw_event_with_buffers take caller scratch
instead of future-resident [u8; UDP_BUFFER_SIZE] arrays; the E2E overflow guard
now checks msg_buf.len() (was UDP_BUFFER_SIZE) — the same OOB-panic bug class
PR 2 fixed on the client, now on the server publish path (regression test
RED->GREEN). Tokio publish_event/publish_raw_event kept as ergonomic _alloc
wrappers so the app API is unchanged. bm_server_publish_future ~3320 -> 320 B.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r extraction; fix comment typo (#125 PR3 T5)

bm_server_run_future is 3528 B post-extraction; budget = ceil64(3528*1.25)=4416
(was 9664 for the pre-PR3 7696). Also fixes a 1400->1500 UDP_BUFFER_SIZE
comment typo flagged in the Task 4 review.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Task 4's new doc comments wrote unresolvable intra-doc links surfaced by the
feature-subset cargo doc gate: [`Error::Capacity("udp_buffer")`] (the paren
isn't a valid path) -> [`Error::Capacity`]`("udp_buffer")` matching the
runtime/sd_state form; [`EventPublisher::publish_event`] and [`Vec`] -> code
spans. Doc-comment-only; cargo doc client / server,bare_metal / all-features
all clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…y + docs/CHANGELOG (#125 PR3 T6)

Whole-branch review fixes:
- announce_only_with_buffer (PUBLIC) takes caller scratch and threads it to
  announce_loop, so the supplementary-server announce future is no longer
  buffer-resident for bare-metal callers (the prior doc pointed at pub(super)
  announce_loop — non-actionable). announce_only_future kept as the _alloc
  tokio convenience that allocs internally.
- publish_event_with_buffers: undersized protected_buf (E2E BufferTooSmall)
  now maps to Capacity("udp_buffer") for contract symmetry with msg_buf.
- publish_raw_event_with_buffers: direct undersized-buffer regression test.
- Wrapper docs trued-up (buffer-length contract, not UDP_BUFFER_SIZE);
  CHANGELOG breaking-signature note. Fixed a cross-cfg doc link.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR completes the server-side portion of issue #125 by moving server send-path scratch buffers out of async future state into caller-provided buffers (with _alloc wrappers preserving the tokio/std public API), significantly reducing retained future sizes for run_combined and EventPublisher publish futures.

Changes:

  • Thread caller-provided SD send scratch through run_with_buffers/run_combined and SD helper send functions, including SdStateManager::send_offer_service.
  • Update EventPublisher publish paths to accept caller scratch buffers, with _alloc convenience wrappers preserving existing tokio call sites.
  • Add/adjust bare-metal E2E regression tests, witness budgets, changelog/docs, and update examples/cross-crate tests for the new buffer parameters.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 28 comments.

Show a summary per file
File Description
tests/bare_metal_e2e.rs Updates witness budget and adds regression/witness tests for undersized scratch behavior (publish path)
src/server/sd_state.rs Makes send_offer_service use caller scratch and updates tests accordingly
src/server/runtime.rs Makes SD send helpers accept caller scratch and threads send scratch through runtime loops; adds helper unit tests
src/server/mod.rs Extends run_with_buffers, adds announce_only_with_buffer, updates _alloc wrappers and server tests/callers
src/server/event_publisher.rs Adds *_with_buffers publish APIs and _alloc wrappers; updates capacity checks to use buf.len()
simple-someip-embassy-net/tests/loopback.rs Updates run_with_buffers call sites to pass the additional send scratch buffers
examples/embassy_net_client/src/main.rs Updates run_with_buffers call sites to pass the additional send scratch buffers
docs/simple_someip/plans/2026-06-17-pr3-125-server-buffer-extraction.md Adds implementation plan documentation for PR3
CHANGELOG.md Documents the breaking bare-metal signature changes and the new announce-only API
Comments suppressed due to low confidence (1)

src/server/mod.rs:2581

  • Same temporary-buffer-across-.await issue for send_unicast_offer: &mut [0u8; crate::UDP_BUFFER_SIZE] is a temporary borrowed across .await (because send_to().await needs the buffer). Bind it to a local variable first.
        runtime::send_unicast_offer(
            &mut [0u8; crate::UDP_BUFFER_SIZE],
            &server.config,
            server.sd_socket.get(),
            server.sd_state.get(),
            recv_addr,
        )
        .await
        .expect("send_unicast_offer failed");

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

Comment thread src/server/runtime.rs
Comment on lines +894 to +895
let result =
send_unicast_offer(&mut [0u8; 24], &config, &socket, &sd_state, target).await;
Comment thread src/server/runtime.rs
Comment on lines +912 to +913
let result =
send_unicast_offer(&mut [0u8; 8], &config, &socket, &sd_state, target).await;
Comment thread src/server/runtime.rs
Comment on lines +929 to +936
let result = send_unicast_offer(
&mut [0u8; crate::UDP_BUFFER_SIZE],
&config,
&socket,
&sd_state,
target,
)
.await;
Comment thread src/server/runtime.rs
Comment on lines +987 to +995
let result = send_subscribe_ack_from_view(
&mut [0u8; 24],
&config,
&socket,
&sd_state,
&entry_view,
subscriber,
)
.await;
Comment thread src/server/runtime.rs
Comment on lines +1015 to +1023
let result = send_subscribe_ack_from_view(
&mut [0u8; crate::UDP_BUFFER_SIZE],
&config,
&socket,
&sd_state,
&entry_view,
subscriber,
)
.await;
Comment thread src/server/mod.rs
Comment on lines 2530 to 2540
runtime::handle_sd_message(
&server.config,
server.sd_socket.get(),
server.sd_state.get(),
&server.subscriptions,
&sd_view,
addr,
&mut [0u8; crate::UDP_BUFFER_SIZE],
)
.await
.unwrap();
Comment thread src/server/mod.rs
Comment on lines 2762 to 2771
let result = runtime::handle_sd_message(
&server.config,
server.sd_socket.get(),
server.sd_state.get(),
&server.subscriptions,
&sd_view,
"127.0.0.1:12345".parse().unwrap(),
&mut [0u8; crate::UDP_BUFFER_SIZE],
)
.await;
Comment thread src/server/mod.rs
Comment on lines 2803 to 2813
runtime::handle_sd_message(
&server.config,
server.sd_socket.get(),
server.sd_state.get(),
&server.subscriptions,
&sd_view,
addr,
&mut [0u8; crate::UDP_BUFFER_SIZE],
)
.await
.unwrap();
Comment thread src/server/mod.rs
Comment on lines 3119 to 3129
runtime::handle_sd_message(
&server.config,
server.sd_socket.get(),
server.sd_state.get(),
&server.subscriptions,
&sd_view,
sender,
&mut [0u8; crate::UDP_BUFFER_SIZE],
)
.await
.unwrap();
Comment on lines 363 to 375
// Pre-build size check. Fail fast with `Error::Capacity` BEFORE
// calling `Header::new_event`, which `assert!`s on payloads
// larger than `u32::MAX as usize - 8`. The earlier
// `checked_add(header_len, payload.len())` guard below was dead
// for that reason; keeping it for defence-in-depth on platforms
// where `Header::SIZE + payload` could overflow `usize`. The
// `16` here is the SOME/IP header size in bytes.
if payload.len() > UDP_BUFFER_SIZE.saturating_sub(16) {
// larger than `u32::MAX as usize - 8`. The `16` here is the
// SOME/IP header size in bytes. Guard is keyed off `buf.len()`
// (not `UDP_BUFFER_SIZE`) — the PR-2 lesson applied here too.
if payload.len() > buf.len().saturating_sub(16) {
crate::log::error!(
"raw event payload ({} bytes) + 16-byte header exceeds UDP_BUFFER_SIZE ({}); dropping publish",
"raw event payload ({} bytes) + 16-byte header exceeds buf.len() ({}); dropping publish",
payload.len(),
UDP_BUFFER_SIZE
buf.len()
);
return Err(Error::Capacity("udp_buffer"));
}
Valid findings fixed (all landed here on the PR3 branch per request):
- #133: publish_raw_event_with_buffers now guards buf.len() < 16 explicitly,
  so an empty payload + sub-header buffer returns Capacity("udp_buffer")
  instead of a protocol I/O error (the payload>buf-16 guard saturated to
  0>0 and missed it). + regression test case.
- #132 doc accuracy: corrected stale "Box::leak"/"leaks" wording in
  ClientDeps::tokio, the new_with_* constructor, the socket_manager test_buf
  helper, and the PR2 plan snippet — TokioBufferProvider is Arc-backed
  (freed on drop), not leaked.
- #132 example: bare_metal_client buffer pool 9 -> 10 (UNICAST_SOCKETS_CAP +
  discovery + release-lag slack), matching the tokio provider + the doc
  guidance, to avoid a transient Capacity on evict-then-rebind.
- #132 doc: tidied the ServerConfig::with_announce intra-doc link.

REJECTED (false positives): the ~26 "passing &mut [0u8; N] into an async fn
and awaiting it fails to compile (temporary dropped while borrowed)" comments
on #133. These are inline `helper(&mut [0u8; N]).await` calls — the temporary
lives until the end of the statement, so the borrow is valid across the
await. The code compiles and is green (bare_metal_e2e 9/9, nextest 521,
clippy clean). Copilot misapplied the "store future then await later" rule.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@JustinKovacich

Copy link
Copy Markdown
Contributor Author

Review comments addressed (commit 82be01d)

Fixes for both this PR and #132's comments are landed here on the PR-3 branch.

Fixed (valid):

Not changed (false positives) — the ~26 "passing &mut [0u8; N] into an async fn and awaiting it fails to compile (temporary dropped while borrowed)" comments:

These are all inline helper(&mut [0u8; N]).await calls (a single statement). A temporary in a function-call argument lives until the end of the enclosing statement, and the .await completes within that same statement — so the borrow is valid for the whole await and the temporary is dropped only after the await returns. This pattern compiles; it only fails when a future is stored (let fut = helper(&mut [0u8; N]);) and awaited in a later statement, which is not the case at any of these sites (they're test call sites that bind a temporary inline).

Empirically the branch is green on exactly this code: bare_metal_e2e 9/9, cargo nextest (client-tokio,server-tokio) 521, cargo clippy --workspace --all-features, all three cargo doc feature combos, and the thumbv7em-none-eabihf builds. No change made for these.

JustinKovacich added a commit that referenced this pull request Jun 26, 2026
Valid findings fixed (all landed here on the PR3 branch per request):
- #133: publish_raw_event_with_buffers now guards buf.len() < 16 explicitly,
  so an empty payload + sub-header buffer returns Capacity("udp_buffer")
  instead of a protocol I/O error (the payload>buf-16 guard saturated to
  0>0 and missed it). + regression test case.
- #132 doc accuracy: corrected stale "Box::leak"/"leaks" wording in
  ClientDeps::tokio, the new_with_* constructor, the socket_manager test_buf
  helper, and the PR2 plan snippet — TokioBufferProvider is Arc-backed
  (freed on drop), not leaked.
- #132 example: bare_metal_client buffer pool 9 -> 10 (UNICAST_SOCKETS_CAP +
  discovery + release-lag slack), matching the tokio provider + the doc
  guidance, to avoid a transient Capacity on evict-then-rebind.
- #132 doc: tidied the ServerConfig::with_announce intra-doc link.

REJECTED (false positives): the ~26 "passing &mut [0u8; N] into an async fn
and awaiting it fails to compile (temporary dropped while borrowed)" comments
on #133. These are inline `helper(&mut [0u8; N]).await` calls — the temporary
lives until the end of the statement, so the borrow is valid across the
await. The code compiles and is green (bare_metal_e2e 9/9, nextest 521,
clippy clean). Copilot misapplied the "store future then await later" rule.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JustinKovacich added a commit that referenced this pull request Jun 26, 2026
Port-target branch off the #133 tip with a handoff for Feliciano: the
keep/drop/adapt classification of #126's 7 commits, the union
NonSdRequestCallback (already in the stack), the target server/E2E/
subscription APIs to adapt polled.rs against, the divergent-API touch
points in polled.rs, and the rationale for NOT adopting the const-generic
E2ERegistry refactor into the stack. polled.rs is a port (not a rebase)
because it sits on #126's own divergent server/e2e rework.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@JustinKovacich

Copy link
Copy Markdown
Contributor Author

Folded into main via the 0.8.0 consolidation (#138). Content verified present in main; closing and deleting the branch as part of post-0.8.0 cleanup.

@JustinKovacich
JustinKovacich deleted the feature/pr3_125_server_buffers branch June 29, 2026 20:40
@JustinKovacich
JustinKovacich restored the feature/pr3_125_server_buffers branch June 29, 2026 20:45
@JustinKovacich
JustinKovacich deleted the feature/pr3_125_server_buffers branch June 29, 2026 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants