Skip to content

PR 2: #125 client async-state reduction — buffer pool extraction (socket-loop future 2224→776 B)#132

Closed
JustinKovacich wants to merge 12 commits into
feature/pr1_124_followupsfrom
feature/pr2_125_client_async_state
Closed

PR 2: #125 client async-state reduction — buffer pool extraction (socket-loop future 2224→776 B)#132
JustinKovacich wants to merge 12 commits into
feature/pr1_124_followupsfrom
feature/pr2_125_client_async_state

Conversation

@JustinKovacich

Copy link
Copy Markdown
Contributor

PR 2 — #125 client async-state reduction

Third PR in the #125 memory-reduction stack (after PR 0 measurement harness #127, PR 1 #124 follow-ups #129). Stacked on #129 — review/merge that first; this PR's diff is only its own 9 commits.

What this does

Moves the client's per-socket [u8; UDP_BUFFER_SIZE] (1500 B) receive buffer out of the spawned socket-loop future into a caller-provided pool, so the bare-metal Embassy-arena (TaskStorage) footprint drops and becomes consumer-sized.

Headline result: the bare-metal client socket-loop future shrinks 2224 B → 776 B (the 1500-byte buffer is no longer future-resident). The witness budget is tightened to 1024 B so the win is a regression tripwire.

Mechanism

  • New BufferPool<SLOTS, LEN> / BufferLease primitive (src/buffer_pool.rs): per-slot AtomicBool compare-exchange claim, &'static mut [u8] handed out, RAII release on drop. No &'static mut aliasing.
  • New BufferProvider trait (src/transport.rs) with a static-pool impl and a heap-backed TokioBufferProvider.
  • Threaded through ClientDeps (new BP generic) → BindDispatch/SpawnerDispatchSocketManager::bind_*. The socket loop receives its buffer by lease; it's released when the loop exits (socket close → loop future drops). Bare-metal consumers size the pool in .bss (e.g. 2 × 512 B); the tokio path provisions one pool per client internally — public tokio API unchanged.

Behavioral changes (only these two, per the design)

  • An inbound datagram larger than the claimed buffer is dropped with a log (not parsed from a truncated buffer).
  • The oversize-send rejection — including the E2E-protected path — keys off buf.len(), not the UDP_BUFFER_SIZE constant.

Wire format untouched.

Measurement-gated outcomes (no change kept, by design)

  • E2E scratch buffer: the protected buffer is block-scoped and dropped before the send await, so it's transient stack, not future-resident — verified, no change.
  • Control-handler flatten: attempted; moved the run-future witness only −1.03% (it's dominated by the select_biased! sub-futures, not handler locals), so reverted per "changes that don't move the measurement are dropped."

Folded in: doc-gate hygiene

cargo doc (feature subsets) and cargo test --doc were already red on the phase-21 base (unresolved ServerDeps/ClientDeps::tokio/EventPublisher intra-doc links, dead announcement_loop links, an illegal with_announce(false) link, and a transport adapter-sketch doctest referencing the absent futures umbrella crate). Repaired (doc-comment-only) so the stack is green end-to-end.

Design divergence from the written plan (intentional)

  • Synchronization: per-slot atomics instead of the plan's critical-section (removes the dependency and is std-test-host-safe).
  • Module location: BufferPool lives at the crate root (src/buffer_pool.rs, ungated) rather than inside the bare_metal-gated static_channels, because the tokio provider needs it too; static_channels re-exports it for back-compat.

Verification

  • cargo nextest (client-tokio,server-tokio): 514 passed / 11 skipped. bare_metal_e2e: 6/6 incl. three new tests (E2E-OOB regression, inbound-oversize, buffer-claim/exhaustion).
  • cargo clippy — workspace --all-features, --no-default-features, per-feature bare_metal, client-tokio: clean.
  • cargo build --target thumbv7em-none-eabihfclient,bare_metal / server,bare_metal / client,server,bare_metal: clean. client,bare_metal no-alloc witness: pass.
  • cargo doc (client / server,bare_metal / all-features, -D warnings) and cargo test --doc: clean.
  • cargo test --no-default-features: clean.

Reviewed end-to-end (incl. adversarial whole-branch review, which caught the E2E-OOB bug fixed in the final commit).

🤖 Generated with Claude Code

JustinKovacich and others added 12 commits June 17, 2026 09:43
8 TDD tasks: BufferPool/BufferLease primitive + BufferProvider trait
(mirroring the channel-pool machinery), thread the provider through
ClientDeps -> BindDispatch -> bind_*, move per-socket buffers out of the
spawned loop futures into caller-sized pooled storage, kill the second
E2E scratch buffer, and flatten handle_control_message. Every memory
change is gated on the PR-0 future-size witnesses. Stacked on PR1.

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

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

Addresses Task 1 review: shared static could flake under parallel libtest.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…BufferPool to ungated module (#125)

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

Task 2 review: lib.rs blank-line consistency. The reviewer's 'redundant Box
import' finding was a false positive — the crate is no_std, so std-gated
modules import Box explicitly; verified client-tokio fails without it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…vider; oversize drop/reject (#125)

Threads a BufferProvider through ClientDeps -> BindDispatch -> bind_*; the
socket loop receives its buffer by lease (released on loop-future drop).
Inbound datagrams over the claimed length are dropped+logged; oversize-send
rejection keys off buf.len(). Tokio path provisions one leaked pool per client.
bare_metal socket-loop future: 2224 B -> 776 B. (Tasks 3+4 of the #125 plan.)

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

Task 3+4 review (Minor): the test asserts claim+exhaustion, not release-on-close
(RAII release is unit-covered in tests/buffer_pool.rs). Inert fn rename, no callers.

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

Socket-loop future is 776 B post-extraction (was ~2224); budget now ceil64(776*1.25)=1024,
turning the #125 win into a regression tripwire. Footprint doc rewritten for pooled,
caller-sized buffers. (Task 7 of the #125 plan.)

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

Feature-subset cargo doc and cargo test --doc were red on the phase-21 base
(ServerDeps/ClientDeps::tokio, server EventPublisher/announcement_loop dead
links, an illegal with_announce(false) link, and a transport adapter-sketch
doctest referencing the absent  umbrella crate). Doc-comment-only:
cross-feature links demoted to code spans; the doctest reshaped to compile
against the real GAT traits. Folded into PR2 so the stack is green end-to-end.

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

Final-review Issue #1: on a caller-sized buffer smaller than UDP_BUFFER_SIZE,
an E2E-protected frame that expands past buf.len() but stays under
UDP_BUFFER_SIZE passed the constant guard and then indexed buf out of bounds,
panicking the socket loop (the dft E2E path). Guard now uses buf.len();
regression test reproduces the OOB (RED) and confirms a typed Capacity error
(GREEN). Also documents the coarse send() pre-filter.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…; capacity headroom + min-size floor (#125)

Adversarial-review fixes (pool/unsafe cluster):
- BufferLease redesigned to raw NonNull ptrs + an alloc-gated Arc keepalive
  (_owner). Static (bare-metal) path: _owner=None, no allocation. Tokio path:
  TokioBufferProvider now holds Arc<BufferPool<10, UDP_BUFFER_SIZE>> and claims
  via claim_arc(), so the pool is reference-counted, not leaked per client.
- Pool sized 9->10 (UNICAST_SOCKETS_CAP + discovery + 1 release-lag slack) to
  avoid spurious Capacity errors when an evicted socket's lease frees async.
- const assert LEN >= 16 (SOME/IP header floor); footprint doc states the
  practical floor and the +1 bare-metal sizing guidance.
- Added a concurrent-claim test proving no double-claim / no aliasing under
  contention. Per-slot AtomicBool exclusivity invariant unchanged.

nm-verified: client,bare_metal stays alloc-free (Arc path cfg'd out).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…doc + test cleanup (#125)

Adversarial-review finding #1: the oversize-drop guard was hollow on tokio
(kernel-truncates, pre-existing #119) and FATAL on embassy-net (Truncated ->
Io(Other), counted toward the kill cap -> 16 oversize datagrams killed the loop).
- New IoErrorKind::Truncated variant, classified transient by is_transient_recv;
  embassy-net maps RecvError::Truncated to it, so oversize datagrams drop and the
  loop survives. Io(Other) stays fatal (no masking of real failures).
- Honest doc at the guard re: per-backend behavior (tokio MSG_TRUNC is a tracked
  follow-up); honesty comment on the ScriptSocket guard-mechanism test.
- Deleted a vacuous E2E-overflow unit test (1500-byte buffer made its check
  degenerate); authoritative coverage is bare_metal_e2e's small-buffer test.
- Soundness-review nit: BufferLease module doc no longer claims &'static mut.

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 advances the #125 memory-reduction stack by extracting the client socket-loop receive buffer out of the spawned future and into a caller-provided buffer pool, reducing Embassy task-state size and making buffer RAM consumer-sized (static .bss on bare metal; heap-backed but internal on tokio). It also updates docs/doctests and adds regression tests around pool claim/exhaustion and oversize datagram/send behavior.

Changes:

  • Introduce BufferPool/BufferLease and thread a BufferProvider through ClientDeps → bind dispatch → SocketManager so socket loops lease their buffers instead of owning [u8; UDP_BUFFER_SIZE].
  • Update client socket-loop logic to (a) drop inbound datagrams larger than the claimed buffer and (b) reject oversize sends based on buf.len() (including E2E-protected expansion).
  • Add/adjust tests and documentation, including future-size witness budget tightening.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
tools/size_probe/src/lib.rs Updates probe deps to supply a static buffer pool/provider for future-size measurement.
tests/static_channels_alloc_witness.rs Ensures client construction in alloc-witness tests provides a static buffer provider.
tests/buffer_pool.rs Adds unit tests for pool claiming, drop/release, and concurrent claim non-aliasing.
tests/bare_metal_e2e.rs Threads buffer provider into E2E harness; adds oversize/claim-exhaustion/E2E regression tests; tightens size budgets.
tests/bare_metal_client.rs Updates bare-metal client construction to include static buffer provider.
tests/bare_metal_client_local.rs Updates local-spawner bare-metal client construction to include static buffer provider.
src/transport.rs Adds IoErrorKind::Truncated and introduces BufferProvider + StaticBufferProvider.
src/tokio_transport.rs Adds TokioBufferProvider (Arc-backed pool) implementing BufferProvider.
src/static_channels/mod.rs Re-exports BufferPool/BufferLease for back-compat paths.
src/server/mod.rs Rustdoc link hygiene updates (one link currently broken; see comments).
src/lib.rs Exposes new buffer_pool module at crate root.
src/client/socket_manager.rs Socket loops now take a leased buffer; oversize send/drop logic updated; unit tests updated accordingly.
src/client/mod.rs Adds buffer-provider generic/field to ClientDeps; updates memory-footprint docs and tokio constructors.
src/client/inner.rs Carries updated dispatch generic; notes buffer lease is released via loop drop on eviction.
src/client/bind_dispatch.rs Dispatchers now hold a BufferProvider and claim a buffer per bind.
src/buffer_pool.rs New BufferPool/BufferLease implementation (atomic slot-claim + RAII release; Arc-backed claim supported under _alloc).
simple-someip-embassy-net/tests/loopback.rs Updates embassy-net loopback tests to provide a buffer pool/provider.
simple-someip-embassy-net/src/socket.rs Maps embassy-net truncation to IoErrorKind::Truncated (drop-and-continue classification).
examples/embassy_net_client/src/main.rs Updates example client deps to include a buffer provider.
examples/bare_metal_client/src/main.rs Adds static buffer pool/provider to bare-metal client example (pool sizing needs a fix; see comments).
docs/simple_someip/plans/2026-06-17-pr2-125-client-async-state-reduction.md Adds an implementation-plan document (one TokioBufferProvider snippet is now outdated; see comments).
Cargo.toml Registers the new buffer_pool test target.

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

Comment thread src/client/mod.rs
Comment on lines +343 to 348
/// `buffer_provider` is a single `TokioBufferProvider::new()`
/// constructed here exactly once. `TokioBufferProvider::new()` does
/// a `Box::leak`, so it MUST be one-per-client and never called on a
/// per-bind / hot path — this constructor is the canonical single
/// call site for the tokio path.
#[must_use]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in #133 (commit 82be01d): reworded — TokioBufferProvider is Arc-backed (the pool is freed when the last provider/lease drops, not leaked); kept one-per-client to share the pool and avoid a per-bind heap allocation. (The #125 doc/example fixes are consolidated on the stacked PR-3 branch.)

Comment thread src/client/mod.rs
Comment on lines +693 to +695
// One `TokioBufferProvider::new()` per client construction.
// It `Box::leak`s internally, so it must not be moved to a
// per-bind path; this single call covers every `bind_*`.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in #133 (commit 82be01d): same Box::leakArc-backed wording fix on this constructor comment.

Comment thread src/server/mod.rs
/// into a single future so callers cannot forget to spawn the
/// announcement side; passing
/// [`ServerConfig::with_announce(false)`] suppresses the
/// [`ServerConfig::with_announce`]`(false)` suppresses the

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in #133 (commit 82be01d): separated the ServerConfig::with_announce link from the (false) argument.

Comment on lines +881 to +884
/// Claim a single socket-loop buffer for a direct `bind_with_transport`
/// call in these unit tests. Each call leaks one small `BufferPool`
/// (acceptable in a test); production paths claim from one shared
/// provider per client.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in #133 (commit 82be01d): the test_buf comment no longer implies a leak — TokioBufferProvider is Arc-backed and freed on drop.

Comment on lines +302 to +308
// Caller-declared static buffer pool (#125): one slot per
// possible socket. On real firmware this is a `static`; here it
// is a function-local `static` for the example.
buffer_provider: {
static POOL: BufferPool<9, UDP_BUFFER_SIZE> = BufferPool::new();
StaticBufferProvider(&POOL)
},

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in #133 (commit 82be01d): bumped the example pool to BufferPool<10, _> (UNICAST_SOCKETS_CAP + discovery + release-lag slack), matching the tokio provider and the documented sizing guidance.

Comment on lines +292 to +316
/// Tokio-path buffer provider: a single leaked `BufferPool` sized at
/// `UNICAST_SOCKETS_CAP + 1` × `UDP_BUFFER_SIZE` (one per possible socket
/// plus discovery). Leaking is fine — a client process holds one for its
/// lifetime; the API hides it entirely from callers.
#[derive(Clone, Copy, Debug)]
pub struct TokioBufferProvider(&'static BufferPool<9, UDP_BUFFER_SIZE>);

impl TokioBufferProvider {
#[must_use]
pub fn new() -> Self {
Self(Box::leak(Box::new(BufferPool::new())))
}
}

impl Default for TokioBufferProvider {
fn default() -> Self {
Self::new()
}
}

impl BufferProvider for TokioBufferProvider {
fn claim(&self) -> Option<BufferLease> {
self.0.claim()
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in #133 (commit 82be01d): updated the plan snippet to the merged Arc-backed implementation (no Box::leak).

JustinKovacich added a commit that referenced this pull request Jun 17, 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
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

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/pr2_125_client_async_state branch June 29, 2026 20:40
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