diff --git a/aimdb-core/src/record_id.rs b/aimdb-core/src/record_id.rs index 8d2bb6d..71ccbf9 100644 --- a/aimdb-core/src/record_id.rs +++ b/aimdb-core/src/record_id.rs @@ -50,20 +50,43 @@ //! let producer = db.producer::(AppKey::TempIndoor); //! ``` -use alloc::{boxed::Box, string::ToString}; +use alloc::{boxed::Box, collections::BTreeSet, string::ToString}; #[cfg(all(debug_assertions, feature = "std"))] use core::sync::atomic::{AtomicUsize, Ordering}; -/// Counter for interned keys (debug builds only, std only) +/// Counter for distinct interned keys (debug builds only, std only) #[cfg(all(debug_assertions, feature = "std"))] static INTERNED_KEY_COUNT: AtomicUsize = AtomicUsize::new(0); -/// Maximum expected interned keys before warning. +/// Maximum expected distinct interned keys before warning. /// If exceeded, a debug assertion fires to catch potential misuse. #[cfg(all(debug_assertions, feature = "std"))] const MAX_EXPECTED_INTERNED_KEYS: usize = 1000; +#[cfg(feature = "std")] +type Mutex = std::sync::Mutex; +#[cfg(not(feature = "std"))] +type Mutex = spin::Mutex; + +/// Locks the intern table, hiding the std/spin API difference +/// (`std::sync::Mutex::lock` returns a `LockResult`; `spin` returns the guard +/// directly). A poisoned std mutex is unrecoverable here, so `.unwrap()` is +/// the correct response. Same pattern as the `TypedRecord` field mutexes. +#[cfg(feature = "std")] +fn lock(m: &Mutex) -> std::sync::MutexGuard<'_, T> { + m.lock().unwrap() +} +#[cfg(not(feature = "std"))] +fn lock(m: &Mutex) -> spin::MutexGuard<'_, T> { + m.lock() +} + +/// Dedup table for interned keys: re-interning an already-known key returns +/// the existing `'static` allocation instead of leaking a new one, bounding +/// the leak by the number of *distinct* dynamic keys. +static INTERNED_KEYS: Mutex> = Mutex::new(BTreeSet::new()); + // Re-export derive macro when feature is enabled #[cfg(feature = "derive")] pub use aimdb_derive::RecordKey; @@ -248,8 +271,9 @@ enum StringKeyInner { Static(&'static str), /// Interned runtime string (leaked into 'static lifetime) /// - /// Memory is intentionally leaked for O(1) cloning and comparison. - /// This is safe because keys are registered once at startup. + /// Memory is intentionally leaked for O(1) cloning and comparison; a + /// dedup table bounds the leak by the number of distinct dynamic keys. + /// See [`StringKey::intern`] for the contract. Interned(&'static str), } @@ -267,34 +291,47 @@ impl StringKey { /// /// Use this for dynamic names (multi-tenant, config-driven, etc.). /// - /// # Memory + /// # Memory contract /// - /// The string is leaked into `'static` lifetime. This is intentional: - /// - Keys are registered once at startup - /// - Enables O(1) Copy/Clone - /// - Typical overhead: <4KB for 100 keys + /// Each **distinct** dynamic key allocates once for the lifetime of the + /// process (the string is leaked into `'static`); re-interning a known + /// key returns the existing allocation. This is what keeps `StringKey` + /// `Copy` with O(1) comparison — and it means every distinct key stays + /// resident forever. Do **not** derive keys from unbounded input (e.g. + /// per-request or per-message IDs). Typical overhead: <4KB for 100 keys. /// /// # Panics (debug builds only) /// - /// In debug builds with `std` feature, panics if more than 1000 keys are - /// interned. This catches accidental misuse (e.g., creating keys in a loop). - /// Production builds have no limit. - #[inline] + /// In debug builds with the `std` feature, panics if more than 1000 + /// *distinct* keys are interned. This catches accidental misuse (e.g., + /// deriving keys from unbounded input). Production builds have no limit. #[must_use] pub fn intern(s: impl AsRef) -> Self { + let s = s.as_ref(); + + // Hold the lock across lookup + leak + insert so a race cannot leak + // two allocations for the same key. + let mut interned = lock(&INTERNED_KEYS); + if let Some(&existing) = interned.get(s) { + return Self(StringKeyInner::Interned(existing)); + } + #[cfg(all(debug_assertions, feature = "std"))] { let count = INTERNED_KEY_COUNT.fetch_add(1, Ordering::Relaxed); debug_assert!( count < MAX_EXPECTED_INTERNED_KEYS, - "StringKey::intern() called {} times. This exceeds the expected limit of {}. \ - Interned keys leak memory and should only be created at startup. \ + "StringKey::intern() created {} distinct keys. This exceeds the expected \ + limit of {}. Each distinct interned key leaks memory for process lifetime; \ + keys should only be created at startup, never derived from unbounded input. \ Use static string literals or enum keys for better performance.", count + 1, MAX_EXPECTED_INTERNED_KEYS ); } - let leaked: &'static str = Box::leak(s.as_ref().to_string().into_boxed_str()); + + let leaked: &'static str = Box::leak(s.to_string().into_boxed_str()); + interned.insert(leaked); Self(StringKeyInner::Interned(leaked)) } @@ -529,6 +566,19 @@ mod tests { assert_eq!(key.as_str(), "sensors.temperature"); } + #[test] + fn test_intern_dedup_returns_same_allocation() { + // Two separately built Strings with the same content must intern to + // the very same 'static allocation (leak bounded by distinct keys). + let a = StringKey::intern(["dedup", ".", "sensor"].concat()); + let b = StringKey::intern(["dedup", ".", "sensor"].concat()); + assert!(core::ptr::eq(a.as_str(), b.as_str())); + + // A different key gets its own allocation. + let c = StringKey::intern(["dedup", ".", "other"].concat()); + assert!(!core::ptr::eq(a.as_str(), c.as_str())); + } + #[test] fn test_string_key_equality() { let static_key: StringKey = "sensors.temp".into(); diff --git a/aimdb-embassy-adapter/src/buffer.rs b/aimdb-embassy-adapter/src/buffer.rs index a45e747..e70c848 100644 --- a/aimdb-embassy-adapter/src/buffer.rs +++ b/aimdb-embassy-adapter/src/buffer.rs @@ -572,41 +572,13 @@ mod tests { // ── Host-test scaffolding ──────────────────────────────────────────── // The crate links `defmt` (workspace dep) and embassy-time's // `defmt-timestamp-uptime`, but on the host neither a defmt logger nor a - // time driver exists. Provide no-op stubs so the test binary links. Run via - // the `test` Make target, or directly: + // time driver exists. `host_test_stubs!` provides no-op stubs so the test + // binary links (expanded here for the lib test binary; integration tests + // expand it themselves). Run via the `test` Make target, or directly: // cargo test -p aimdb-embassy-adapter \ // --no-default-features --features "alloc,embassy-sync,embassy-time" // (`embassy-runtime` would pull the cortex-m executor, which can't host-build.) - #[defmt::global_logger] - struct TestLogger; - - unsafe impl defmt::Logger for TestLogger { - fn acquire() {} - unsafe fn flush() {} - unsafe fn release() {} - unsafe fn write(_bytes: &[u8]) {} - } - - #[defmt::panic_handler] - fn defmt_panic() -> ! { - core::panic!("defmt panic in host test") - } - - // Trivial time driver so `_embassy_time_now` resolves on the host. The - // clock is pinned at 0; `schedule_wake` wakes immediately so an - // already-expired `Timer` (e.g. `sleep(Duration::ZERO)` in the RuntimeOps - // contract test) completes on its next poll. Non-zero sleeps would spin - // forever — host tests must not use them. - struct TestTimeDriver; - impl embassy_time_driver::Driver for TestTimeDriver { - fn now(&self) -> u64 { - 0 - } - fn schedule_wake(&self, _at: u64, waker: &core::task::Waker) { - waker.wake_by_ref(); - } - } - embassy_time_driver::time_driver_impl!(static TEST_TIME_DRIVER: TestTimeDriver = TestTimeDriver); + crate::host_test_stubs!(); // Note: Embassy tests typically run on actual embedded targets or with embassy-executor // For now, these are basic compilation tests. Integration tests would need embassy-executor. diff --git a/aimdb-embassy-adapter/src/lib.rs b/aimdb-embassy-adapter/src/lib.rs index 57c996f..1abf8ae 100644 --- a/aimdb-embassy-adapter/src/lib.rs +++ b/aimdb-embassy-adapter/src/lib.rs @@ -80,6 +80,59 @@ pub mod send_wrapper; #[cfg(all(not(feature = "std"), feature = "connectors"))] pub mod connectors; +/// Link stubs for **host** test binaries that touch the Embassy adapter +/// (035 §2.4): a no-op `#[defmt::global_logger]` + `#[defmt::panic_handler]` +/// and a pinned-at-0 embassy-time driver. +/// +/// Coercing `EmbassyAdapter` to `Arc` instantiates a vtable +/// whose `log` entry calls `defmt::*` unconditionally, so the `_defmt_*` +/// extern symbols must resolve in every host test binary that links the +/// adapter — and `#[global_logger]`/the time driver must be defined **once +/// per binary**, so a shared `#[cfg(test)]` item cannot serve integration +/// tests. This macro keeps the definition in one place; each test binary +/// expands it exactly once at top level (or once in the lib's test module). +/// +/// The invoking crate needs `defmt` and `embassy-time-driver` resolvable +/// (dev-dependencies are enough). +/// +/// The time driver pins the clock at 0 and `schedule_wake` wakes immediately, +/// so an already-expired timer (e.g. `sleep(Duration::ZERO)`) completes on its +/// next poll. Non-zero sleeps would spin forever — host tests must not use +/// them. +#[macro_export] +#[doc(hidden)] +macro_rules! host_test_stubs { + () => { + #[defmt::global_logger] + struct HostTestLogger; + + unsafe impl defmt::Logger for HostTestLogger { + fn acquire() {} + unsafe fn flush() {} + unsafe fn release() {} + unsafe fn write(_bytes: &[u8]) {} + } + + #[defmt::panic_handler] + fn defmt_panic() -> ! { + core::panic!("defmt panic in host test") + } + + struct HostTestTimeDriver; + impl embassy_time_driver::Driver for HostTestTimeDriver { + fn now(&self) -> u64 { + 0 + } + fn schedule_wake(&self, _at: u64, waker: &core::task::Waker) { + waker.wake_by_ref(); + } + } + embassy_time_driver::time_driver_impl!( + static HOST_TEST_TIME_DRIVER: HostTestTimeDriver = HostTestTimeDriver + ); + }; +} + // Error handling exports #[cfg(not(feature = "std"))] pub use error::EmbassyErrorSupport; diff --git a/aimdb-embassy-adapter/tests/session_smoke.rs b/aimdb-embassy-adapter/tests/session_smoke.rs index 3da31fa..44a3cfb 100644 --- a/aimdb-embassy-adapter/tests/session_smoke.rs +++ b/aimdb-embassy-adapter/tests/session_smoke.rs @@ -24,34 +24,10 @@ use aimdb_core::session::{ }; use aimdb_embassy_adapter::EmbassyAdapter; -// No-op defmt logger so the binary links: the engine holds the adapter as -// `Arc` (issue #131), whose vtable references the `log` path -// (`defmt` on Embassy) even though this smoke never logs. -#[defmt::global_logger] -struct TestLogger; - -unsafe impl defmt::Logger for TestLogger { - fn acquire() {} - unsafe fn flush() {} - unsafe fn release() {} - unsafe fn write(_bytes: &[u8]) {} -} - -#[defmt::panic_handler] -fn defmt_panic() -> ! { - core::panic!("defmt panic in host test") -} - -// Trivial host time driver so `embassy_time` links (the happy path never awaits -// `clock.sleep`, so `now`/`schedule_wake` are never actually exercised). -struct TestTimeDriver; -impl embassy_time_driver::Driver for TestTimeDriver { - fn now(&self) -> u64 { - 0 - } - fn schedule_wake(&self, _at: u64, _waker: &core::task::Waker) {} -} -embassy_time_driver::time_driver_impl!(static TEST_TIME_DRIVER: TestTimeDriver = TestTimeDriver); +// No-op defmt logger + host time driver so the binary links: the engine holds +// the adapter as `Arc` (issue #131), whose vtable references +// the `log` path (`defmt` on Embassy) even though this smoke never logs. +aimdb_embassy_adapter::host_test_stubs!(); /// Minimal echo wire: a `Request` is `[id:8][params]`; the loopback returns those /// bytes verbatim, which `decode_outbound` reads back as `Reply { id, Ok(params) }`. diff --git a/aimdb-serial-connector/Cargo.toml b/aimdb-serial-connector/Cargo.toml index 371e642..cce83b9 100644 --- a/aimdb-serial-connector/Cargo.toml +++ b/aimdb-serial-connector/Cargo.toml @@ -83,14 +83,14 @@ tracing = { version = "0.1", optional = true } defmt = { workspace = true, optional = true } [dev-dependencies] -# Trivial host time driver so `embassy-time` links in the embassy smoke test. -# (`aimdb-embassy-adapter` itself is the regular `embassy-runtime` optional dep — -# keeping it out of dev-deps avoids a std/no_std unification clash with the tokio -# tests, where it would otherwise compile against a std `aimdb-core`.) +# The embassy smoke test expands `aimdb_embassy_adapter::host_test_stubs!()` +# (no-op defmt logger + host time driver, 035 §2.4); the expansion references +# `defmt` and `embassy-time-driver` at the invocation site, so both must stay +# resolvable in the test binary. (`aimdb-embassy-adapter` itself is the regular +# `embassy-runtime` optional dep — keeping it out of dev-deps avoids a +# std/no_std unification clash with the tokio tests, where it would otherwise +# compile against a std `aimdb-core`.) embassy-time-driver = { path = "../_external/embassy/embassy-time-driver" } -# No-op defmt logger stub for the embassy smoke test binary: the engine holds -# the adapter as `Arc` (issue #131), whose vtable references -# the defmt-backed `log` path. defmt = { workspace = true } tokio = { version = "1", features = [ "rt-multi-thread", diff --git a/aimdb-serial-connector/tests/embassy_smoke.rs b/aimdb-serial-connector/tests/embassy_smoke.rs index f066cd6..ebfdceb 100644 --- a/aimdb-serial-connector/tests/embassy_smoke.rs +++ b/aimdb-serial-connector/tests/embassy_smoke.rs @@ -31,34 +31,10 @@ use aimdb_embassy_adapter::connectors::{EmbassyConnection, OneShotDialer}; use aimdb_embassy_adapter::EmbassyAdapter; use aimdb_serial_connector::embassy_transport::CobsFramer; -// Trivial host time driver so `embassy_time` links (the happy path never awaits -// `clock.sleep`, so the driver is never actually exercised). -// No-op defmt logger so the binary links: the engine holds the adapter as -// `Arc` (issue #131), whose vtable references the `log` path -// (`defmt` on Embassy) even though this smoke never logs. -#[defmt::global_logger] -struct TestLogger; - -unsafe impl defmt::Logger for TestLogger { - fn acquire() {} - unsafe fn flush() {} - unsafe fn release() {} - unsafe fn write(_bytes: &[u8]) {} -} - -#[defmt::panic_handler] -fn defmt_panic() -> ! { - core::panic!("defmt panic in host test") -} - -struct TestTimeDriver; -impl embassy_time_driver::Driver for TestTimeDriver { - fn now(&self) -> u64 { - 0 - } - fn schedule_wake(&self, _at: u64, _waker: &core::task::Waker) {} -} -embassy_time_driver::time_driver_impl!(static TEST_TIME_DRIVER: TestTimeDriver = TestTimeDriver); +// No-op defmt logger + host time driver so the binary links: the engine holds +// the adapter as `Arc` (issue #131), whose vtable references +// the `log` path (`defmt` on Embassy) even though this smoke never logs. +aimdb_embassy_adapter::host_test_stubs!(); /// Minimal echo wire: a `Request` is `[id:8][params]`; the loopback returns those /// bytes verbatim, which `decode_outbound` reads back as `Reply { id, Ok(params) }`.