refactor: StringKey dedup interner + host_test_stubs! macro (036 W5+W6)

aimdb-core/src/record_id.rs

@@ -50,20 +50,43 @@
 //! let producer = db.producer::<Temperature>(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<T> = std::sync::Mutex<T>;
+#[cfg(not(feature = "std"))]
+type Mutex<T> = spin::Mutex<T>;
+
+/// 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<T>(m: &Mutex<T>) -> std::sync::MutexGuard<'_, T> {
+    m.lock().unwrap()
+}
+#[cfg(not(feature = "std"))]
+fn lock<T>(m: &Mutex<T>) -> 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<BTreeSet<&'static str>> = 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<str>) -> 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();

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.

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<dyn RuntimeOps>` 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;

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<dyn RuntimeOps>` (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<dyn RuntimeOps>` (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) }`.

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<dyn RuntimeOps>` (issue #131), whose vtable references
-# the defmt-backed `log` path.
 defmt = { workspace = true }
 tokio = { version = "1", features = [
     "rt-multi-thread",

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<dyn RuntimeOps>` (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<dyn RuntimeOps>` (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) }`.