From c5a624fb73e1f1cb7c2b2020e48c669fac23f36b Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 9 Nov 2026 18:17:30 +0000 Subject: [PATCH 1/5] fix: give rings a readable latest value in core; drop get_record retry (#444) aimdb-mcp's get_record retried `not_found` for up to 3 s so a ring's drain cursor had time to fill. An empty single_latest record or a misspelled name also answers `not_found`, so both paid the full window on every read. The retry existed only because a ring buffer has no current-value slot, so `record.get` fell back to draining the connection's cursor. Fix that in core instead of in each adapter: - `LatestSlot` wraps every SpmcRing buffer at build time, remembers the last value pushed, and answers `peek()`. Every write goes through `RecordWriter::push`, so it sees them all. No adapter changes. - `record.get` is a single `peek()` for every buffer type. The drain-cursor fallback is gone, so a get no longer consumes values a later `record.drain` should return. - aimdb-mcp's `get_record` drops the retry and the drain client: `not_found` comes back at once. - Late-join snapshots (AimX subscribe, OutboundLatest) still skip rings: a ring is a stream of events, not a state. Cost: one clone and a short lock per push on ring records. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WGkDJ6rHvDMa63myz3jwfZ --- CHANGELOG.md | 9 ++ aimdb-client/CHANGELOG.md | 6 + aimdb-client/src/engine.rs | 11 +- aimdb-client/tests/aimx_session.rs | 46 +++--- aimdb-core/CHANGELOG.md | 13 ++ aimdb-core/src/buffer/latest.rs | 143 ++++++++++++++++++ aimdb-core/src/buffer/mod.rs | 2 + aimdb-core/src/buffer/traits.rs | 10 +- aimdb-core/src/builder.rs | 17 +-- aimdb-core/src/outbound/latest.rs | 3 +- aimdb-core/src/session/aimx/dispatch.rs | 48 +++--- aimdb-core/src/typed_api.rs | 12 +- aimdb-core/src/typed_record.rs | 41 ++++- .../tests/http_current_values.rs | 14 +- examples/remote-access-demo/src/client.rs | 36 ++--- tools/aimdb-mcp/CHANGELOG.md | 6 + tools/aimdb-mcp/src/server.rs | 4 +- tools/aimdb-mcp/src/tools/record.rs | 69 +++------ 18 files changed, 340 insertions(+), 150 deletions(-) create mode 100644 aimdb-core/src/buffer/latest.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index b5d3e28c..d1c85699 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed — `record.get` on rings and empty records (#444) + +- **Every buffer type now has a readable latest value.** Core remembers a + ring's last value, so `record.get` and `latest()` work on `SpmcRing` records + without draining a cursor, and `aimdb-mcp`'s `get_record` drops its 3 s retry. + See [aimdb-core](aimdb-core/CHANGELOG.md), + [aimdb-client](aimdb-client/CHANGELOG.md) and + [aimdb-mcp](tools/aimdb-mcp/CHANGELOG.md). + ### Removed — `examples/weather-mesh-demo` - **The in-repo weather mesh demo is gone.** The fleet behind aimdb.dev lives in diff --git a/aimdb-client/CHANGELOG.md b/aimdb-client/CHANGELOG.md index 8fcb193c..dd913b4d 100644 --- a/aimdb-client/CHANGELOG.md +++ b/aimdb-client/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **`get_record` on a ring returns its last value (#444).** It no longer moves + the connection's drain cursor, and it no longer comes back empty until the + ring produces after the first read. + ### Removed (breaking) - **Obsolete AimX message types (#419).** `protocol::{Request, Response, diff --git a/aimdb-client/src/engine.rs b/aimdb-client/src/engine.rs index 92df4643..e6bfa4bb 100644 --- a/aimdb-client/src/engine.rs +++ b/aimdb-client/src/engine.rs @@ -239,12 +239,11 @@ impl AimxConnection { /// Get a record's current value. /// - /// For a `SingleLatest`/state record this is a non-destructive read. A ring - /// (`SpmcRing`) has no canonical latest, so the server returns the most recent - /// value from this connection's drain cursor — which **advances that cursor** - /// (interleaving with [`drain_record`](Self::drain_record)) and is empty until - /// the ring produces a value after the connection first reads it. Prefer - /// [`drain_record`](Self::drain_record) for ring/history records. + /// A non-destructive read of the last value written, for every buffer type + /// (`SingleLatest`, `Mailbox` and `SpmcRing`). It never moves this + /// connection's drain cursor; use [`drain_record`](Self::drain_record) for a + /// ring's history. A server error with code `not_found` means the record does + /// not exist or holds no value yet. /// /// The value comes with the stamp of its write; a reply without `ts` is a /// [`ClientError::Protocol`]. diff --git a/aimdb-client/tests/aimx_session.rs b/aimdb-client/tests/aimx_session.rs index 0987b8a6..58967aa1 100644 --- a/aimdb-client/tests/aimx_session.rs +++ b/aimdb-client/tests/aimx_session.rs @@ -326,10 +326,10 @@ async fn exact_subscribe_sends_the_current_value_once() { } } -/// `record.get` on a ring (`SpmcRing`) record has no canonical latest, so it -/// falls back to draining the connection's cursor for the most-recent value. +/// `record.get` on a ring (`SpmcRing`) record returns the last value written, +/// on the first call, without moving the connection's drain cursor. #[tokio::test] -async fn record_get_on_ring_falls_back_to_drain() { +async fn record_get_on_ring_returns_last_value_without_draining() { let dir = tempfile::tempdir().unwrap(); let sock = dir.path().join("aimdb.sock"); @@ -348,28 +348,36 @@ async fn record_get_on_ring_falls_back_to_drain() { let conn = AimxConnection::connect(sock.to_str().unwrap()) .await .expect("connect"); - let producer = db.producer::("stream").expect("producer"); - // First get opens the cursor; a fresh broadcast reader starts at the tail, so - // it sees nothing until a value is produced afterwards. + // Nothing written yet: not_found straight away. assert!(conn.get_record("stream").await.is_err()); - // Produce after the cursor is open; get now returns the most-recent value. - let mut got = None; - for n in 1..=50u64 { - producer.produce(Reading { n }); - tokio::time::sleep(Duration::from_millis(10)).await; - if let Ok(v) = conn.get_record("stream").await { - got = Some(v); + // Values written before this connection read the record are visible. + let producer = db.producer::("stream").expect("producer"); + producer.produce(Reading { n: 1 }); + producer.produce(Reading { n: 2 }); + + let got = conn.get_record("stream").await.expect("ring get"); + assert_eq!(got.value, serde_json::json!({ "n": 2 })); + // Non-destructive: the same value again. + let again = conn.get_record("stream").await.expect("ring get again"); + assert_eq!(again.value, serde_json::json!({ "n": 2 })); + + // `record.get` opened no drain cursor: the first drain opens one at the + // tail and sees only what is written after it. + let first = conn.drain_record("stream").await.expect("drain"); + assert_eq!(first.count, 0); + producer.produce(Reading { n: 3 }); + let mut drained = Vec::new(); + for _ in 0..50 { + drained.extend(conn.drain_record("stream").await.expect("drain").values); + if !drained.is_empty() { break; } + tokio::time::sleep(Duration::from_millis(10)).await; } - let got = got.expect("ring get returns a value after producing"); - assert!( - got.value.get("n").is_some(), - "ring get yields a Reading: {}", - got.value - ); + assert_eq!(drained.len(), 1); + assert_eq!(drained[0].value, serde_json::json!({ "n": 3 })); drop(conn); } diff --git a/aimdb-core/CHANGELOG.md b/aimdb-core/CHANGELOG.md index 635c3753..74cee38b 100644 --- a/aimdb-core/CHANGELOG.md +++ b/aimdb-core/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed (wire) + +- **`record.get` and `latest()` read a ring's last value (#444).** Core wraps + every `SpmcRing` buffer in `LatestSlot` at build time, which remembers the + last value pushed and answers `peek()`. `TypedRecord::latest()`, + `AimDb::latest()` and `try_latest_as_json*` now return it instead of `None`, + on every runtime adapter, with no adapter changes. AimX `record.get` is a + single `peek()` for every buffer type: the ring fallback that drained the + connection's cursor is gone, so `record.get` no longer consumes values a + later `record.drain` should return, and `not_found` now always means "no + such record or no value yet". Cost: one clone and a short lock per push on + ring records. + ### Removed (breaking) - **Obsolete AimX message types (#419).** `remote::{Request, Response, diff --git a/aimdb-core/src/buffer/latest.rs b/aimdb-core/src/buffer/latest.rs new file mode 100644 index 00000000..bfc1117b --- /dev/null +++ b/aimdb-core/src/buffer/latest.rs @@ -0,0 +1,143 @@ +//! `LatestSlot` — gives any buffer a canonical latest value. +//! +//! A [`SpmcRing`](super::BufferCfg::SpmcRing) is a stream: runtime adapters +//! build it on a broadcast channel, which has no slot to +//! [`peek`](DynBuffer::peek). Rather than make every adapter keep one, core +//! wraps a ring's buffer in a `LatestSlot` when the database is built. The +//! wrapper remembers the last value pushed and answers `peek()` from it; +//! everything else goes straight to the adapter's buffer. +//! +//! Every write reaches the buffer through +//! [`RecordWriter::push`](super::RecordWriter), so the slot sees every value. + +use alloc::{boxed::Box, sync::Arc}; + +use super::traits::{BufferReader, DynBuffer}; + +#[cfg(feature = "observability")] +use super::BufferMetricsSnapshot; + +#[cfg(feature = "std")] +type Mutex = std::sync::Mutex; +#[cfg(not(feature = "std"))] +type Mutex = spin::Mutex; + +#[cfg(feature = "std")] +fn lock(m: &Mutex) -> std::sync::MutexGuard<'_, T> { + m.lock().unwrap_or_else(|poisoned| poisoned.into_inner()) +} +#[cfg(not(feature = "std"))] +fn lock(m: &Mutex) -> spin::MutexGuard<'_, T> { + m.lock() +} + +/// Wraps a buffer and remembers the last value pushed into it, so `peek()` +/// returns that value for buffers that have no current-value slot of their +/// own. +/// +/// The lock is held across the inner push, so with concurrent writers the +/// remembered value is always the one the inner buffer received last. +/// `push` is synchronous and non-blocking on every adapter, so this holds the +/// lock only briefly. On `no_std` the lock is a spin lock: do not push to a +/// wrapped buffer from an interrupt handler that can preempt another pusher. +pub(crate) struct LatestSlot { + inner: Arc>, + latest: Mutex>, +} + +impl LatestSlot { + pub(crate) fn new(inner: Arc>) -> Self { + Self { + inner, + latest: Mutex::new(None), + } + } +} + +impl DynBuffer for LatestSlot { + fn push(&self, value: T) { + let mut latest = lock(&self.latest); + *latest = Some(value.clone()); + self.inner.push(value); + } + + fn subscribe_boxed(&self) -> Box + Send> { + self.inner.subscribe_boxed() + } + + /// The adapter's buffer, so downcasts to a concrete buffer type still + /// work through the wrapper. + fn as_any(&self) -> &dyn core::any::Any { + self.inner.as_any() + } + + fn peek(&self) -> Option { + lock(&self.latest).clone() + } + + #[cfg(feature = "observability")] + fn metrics_snapshot(&self) -> Option { + self.inner.metrics_snapshot() + } + + #[cfg(feature = "observability")] + fn reset_metrics(&self) { + self.inner.reset_metrics(); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::DbError; + use core::task::{Context, Poll}; + + /// A buffer with no current-value slot that records what it received. + struct Stream(Mutex>); + + struct NoReader; + impl BufferReader for NoReader { + fn poll_recv(&mut self, _cx: &mut Context<'_>) -> Poll> { + Poll::Pending + } + fn try_recv(&mut self) -> Result { + Err(DbError::BufferEmpty) + } + } + + impl DynBuffer for Stream { + fn push(&self, value: u32) { + lock(&self.0).push(value); + } + fn subscribe_boxed(&self) -> Box + Send> { + Box::new(NoReader) + } + fn as_any(&self) -> &dyn core::any::Any { + self + } + } + + #[test] + fn peek_is_none_before_the_first_push() { + let slot = LatestSlot::new(Arc::new(Stream(Mutex::new(alloc::vec::Vec::new())))); + assert_eq!(slot.peek(), None); + } + + #[test] + fn peek_returns_the_last_push_and_the_inner_buffer_gets_every_push() { + let inner = Arc::new(Stream(Mutex::new(alloc::vec::Vec::new()))); + let slot = LatestSlot::new(inner.clone() as Arc>); + slot.push(1); + slot.push(2); + assert_eq!(slot.peek(), Some(2)); + // peek does not consume. + assert_eq!(slot.peek(), Some(2)); + assert_eq!(*lock(&inner.0), [1, 2]); + } + + #[test] + fn as_any_downcasts_to_the_inner_buffer() { + let slot = LatestSlot::new(Arc::new(Stream(Mutex::new(alloc::vec::Vec::new())))); + assert!(slot.as_any().downcast_ref::().is_some()); + } +} diff --git a/aimdb-core/src/buffer/mod.rs b/aimdb-core/src/buffer/mod.rs index 8c3dd87e..31ad98e5 100644 --- a/aimdb-core/src/buffer/mod.rs +++ b/aimdb-core/src/buffer/mod.rs @@ -59,6 +59,7 @@ mod cfg; #[cfg(feature = "observability")] mod counters; +mod latest; mod reader; mod traits; mod writer; @@ -76,6 +77,7 @@ pub use reader::Reader; pub use traits::{Buffer, BufferReader, DynBuffer, TryProduceError}; // Crate-private — used by Producer to push without per-call lookup +pub(crate) use latest::LatestSlot; pub(crate) use traits::WriteHandle; pub(crate) use writer::RecordWriter; diff --git a/aimdb-core/src/buffer/traits.rs b/aimdb-core/src/buffer/traits.rs index 7cb3f081..c111c39b 100644 --- a/aimdb-core/src/buffer/traits.rs +++ b/aimdb-core/src/buffer/traits.rs @@ -88,14 +88,16 @@ pub trait DynBuffer: Send + Sync { /// /// Returns `Some(T)` if the buffer holds a current value that can be read /// without affecting any consumer's position. Returns `None` if the buffer - /// type has no canonical "current value" concept (e.g., SPMC Ring) or if + /// type has no current-value slot of its own (e.g., SPMC Ring) or if /// no value has been produced yet. /// - /// This is the buffer-native point-in-time read used by AimX `record.get` - ///. Implementations must not advance any reader position. + /// Implementations must not advance any reader position. /// /// The default returns `None`, which is the correct behaviour for buffers - /// without a canonical latest value. + /// without a current-value slot. Adapters do not need to add one: core + /// wraps every `SpmcRing` buffer in a `LatestSlot` at build time, so + /// `TypedRecord::latest()` and AimX `record.get` see the last value + /// written on every buffer type. fn peek(&self) -> Option { None } diff --git a/aimdb-core/src/builder.rs b/aimdb-core/src/builder.rs index a1555eea..8c28493d 100644 --- a/aimdb-core/src/builder.rs +++ b/aimdb-core/src/builder.rs @@ -877,6 +877,7 @@ impl AimDbBuilder { let clock = runtime.new_clock(); for entry in &mut storages { entry.record.bind_clock(clock.clone()); + entry.record.seal_buffer(); } log_debug!( @@ -1160,10 +1161,9 @@ impl AimDb { /// Reads a record's current value without consuming it. /// - /// Returns `Ok(None)` before the first value, for records without a buffer, - /// or for buffers without a canonical latest value (notably - /// [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing)). Use a subscriber to - /// read a ring's stream instead. + /// Returns `Ok(None)` before the first value or for records without a + /// buffer. For an [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing) this is + /// the last value written; use a subscriber to read the ring's stream. /// /// Unknown keys and mismatched types return the same errors as /// [`produce`](Self::produce). This read needs neither serialization nor @@ -1309,11 +1309,10 @@ impl AimDb { /// /// # Returns /// `Some(JsonValue)` with the current value, or `None` if the record has no - /// value yet, no buffer, or a buffer with **no canonical latest** — i.e. - /// [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing). A ring is a stream/backlog - /// with no single "current value"; read it via a subscriber or `record.drain`, - /// not a peek (`record.get`). Use [`SingleLatest`](crate::buffer::BufferCfg::SingleLatest) - /// for state you want to read latest-value style. + /// value yet or no buffer. For an + /// [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing) the current value is the + /// last one written; read the ring's backlog via a subscriber or + /// `record.drain`. #[cfg(feature = "remote")] pub fn try_latest_as_json( &self, diff --git a/aimdb-core/src/outbound/latest.rs b/aimdb-core/src/outbound/latest.rs index 708483cf..5e4340cf 100644 --- a/aimdb-core/src/outbound/latest.rs +++ b/aimdb-core/src/outbound/latest.rs @@ -26,7 +26,8 @@ pub(crate) struct Rendered { /// Renders a link's current value. The topic is resolved first and passed to /// `accept` (`None`: the link's default topic); the value is serialized only /// if `accept` returns `true`. `None` if the record has no current value (none -/// written yet, or an `SpmcRing` buffer), `accept` rejects the topic, or the +/// written yet), is an `SpmcRing` (a stream of events, so no late-join +/// snapshot), `accept` rejects the topic, or the /// topic does not fit or serializing fails. pub(crate) type LatestFn = Arc< dyn Fn(&AimDb, &RuntimeContext, &dyn Fn(Option<&str>) -> bool) -> Option diff --git a/aimdb-core/src/session/aimx/dispatch.rs b/aimdb-core/src/session/aimx/dispatch.rs index 9697a2a8..15190163 100644 --- a/aimdb-core/src/session/aimx/dispatch.rs +++ b/aimdb-core/src/session/aimx/dispatch.rs @@ -226,7 +226,19 @@ impl AimxSession { impl futures_util::Stream + Send + 'static, > { let stream = crate::remote::stream::stream_record_updates(&self.db, key)?; - let snapshot = self.db.try_latest_as_json_bytes(key).map(|sample| { + // A ring is a stream of events, not a state: it gets no snapshot. + let is_ring = self + .db + .inner() + .resolve_str(key) + .and_then(|id| self.db.inner().storage(id)) + .is_some_and(|record| record.is_ring()); + let latest = if is_ring { + None + } else { + self.db.try_latest_as_json_bytes(key) + }; + let snapshot = latest.map(|sample| { let ts = sample.stamp(); self.pending_snapshots .push((key.to_string(), Payload::from(sample.value), ts)); @@ -319,25 +331,19 @@ impl AimxSession { /// `record.get`: the record's current value and its write stamp, /// `{"value": , "ts": }`. /// - /// A `SingleLatest`/state record exposes a non-destructive canonical latest + /// A non-destructive read of the record's latest value /// ([`try_latest_as_json_bytes`](crate::AimDb::try_latest_as_json_bytes)). - /// A ring - /// ([`SpmcRing`](crate::buffer::BufferCfg::SpmcRing)) has none, so we fall back - /// to the connection's drain cursor and return the **most recent** available - /// value. Two consequences of that fallback: it *advances the shared drain - /// cursor* (so `record.get` and `record.drain` interleave on one connection), - /// and it yields `NotFound` until the ring produces a value *after* the cursor - /// is first opened (a fresh broadcast reader starts at the tail). Use - /// `record.drain` for a ring's full backlog. - fn record_get(&mut self, name: &str) -> Result { - if let Some(sample) = self.db.try_latest_as_json_bytes(name) { - return Ok(super::value_reply(&sample.value, sample.stamp())); - } - // Ring fallback: drain to the newest currently-available value (or NotFound). - self.drain_values(name, usize::MAX)? - .pop() - .map(|sample| to_payload(&stamped_value_json(sample))) - .ok_or(RpcError::NotFound) + /// Every buffer type has one: `SingleLatest` and `Mailbox` natively, and + /// `SpmcRing` through the `LatestSlot` core + /// installs at build time. It never touches this connection's drain + /// cursor. `NotFound` means the record does not exist or holds no value + /// yet. + fn record_get(&self, name: &str) -> Result { + let sample = self + .db + .try_latest_as_json_bytes(name) + .ok_or(RpcError::NotFound)?; + Ok(super::value_reply(&sample.value, sample.stamp())) } /// `record.drain`: return everything accumulated since the previous drain @@ -359,9 +365,7 @@ impl AimxSession { } /// Lazily open (on first call) the per-record drain cursor and read up to - /// `limit` values accumulated since the previous read (oldest-first). Shared - /// by [`record.drain`](Self::record_drain) and [`record.get`](Self::record_get)'s - /// ring fallback, so both read from the same per-connection cursor. + /// `limit` values accumulated since the previous read (oldest-first). fn drain_values(&mut self, name: &str, limit: usize) -> Result>, RpcError> { if !self.drain_readers.contains_key(name) { let id = self diff --git a/aimdb-core/src/typed_api.rs b/aimdb-core/src/typed_api.rs index 50a6789c..a3b6e700 100644 --- a/aimdb-core/src/typed_api.rs +++ b/aimdb-core/src/typed_api.rs @@ -1603,12 +1603,12 @@ where move |db: &AimDb, ctx: &crate::RuntimeContext, accept: &dyn Fn(Option<&str>) -> bool| { - let sample = db - .inner() - .get_typed_record_by_key::(&record_key) - .ok()? - .latest()? - .into_stamped(); + let record = db.inner().get_typed_record_by_key::(&record_key).ok()?; + // A ring is a stream of events: no late-join snapshot. + if crate::typed_record::AnyRecord::is_ring(record) { + return None; + } + let sample = record.latest()?.into_stamped(); let topic = match &topic_writer { None => None, diff --git a/aimdb-core/src/typed_record.rs b/aimdb-core/src/typed_record.rs index b098f086..b93da498 100644 --- a/aimdb-core/src/typed_record.rs +++ b/aimdb-core/src/typed_record.rs @@ -284,6 +284,23 @@ pub trait AnyRecord: Send + Sync { /// runs (design 058 §3.3). fn bind_clock(&mut self, clock: Arc); + /// Gives the record's buffer a canonical latest value if it has none of + /// its own. + /// + /// Called by `AimDbBuilder::build()` for every record before any task + /// runs. A [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing) buffer is + /// wrapped in `LatestSlot`, so `latest()` + /// and AimX `record.get` work on rings the same way they work on + /// `SingleLatest` and `Mailbox` buffers, on every runtime adapter. + fn seal_buffer(&mut self); + + /// Whether the record's buffer is an + /// [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing): a stream of events + /// rather than a state. A ring's latest value is readable on request + /// (`latest()`, `record.get`), but late-join snapshots skip it, so a new + /// subscriber does not receive an old event as if it were current state. + fn is_ring(&self) -> bool; + /// Returns the record's JSON remote-access surface, if it has one. /// /// This accessor is the single place the `remote` cfg-gate lives @@ -1229,10 +1246,10 @@ impl TypedRecord { /// Returns the latest produced value /// /// Returns the most recent value wrapped in `RecordValue`, or `None` if no - /// value has been produced yet, the record has no buffer, or the buffer has - /// **no canonical latest** — i.e. [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing) - /// (a ring is a stream/backlog; read it via a subscriber/drain instead). - /// Non-blocking. + /// value has been produced yet or the record has no buffer. Works for every + /// buffer type: an [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing) gets its + /// latest value from the `LatestSlot` installed + /// at build time. Non-blocking. /// /// **Both std and no_std**: Direct access via `Deref`, `.get()`, `.into_inner()` /// @@ -1271,6 +1288,15 @@ impl AnyRecord for TypedRecord { self.clock = Some(clock); } + fn seal_buffer(&mut self) { + if !self.is_ring() { + return; + } + if let Some(inner) = self.buffer.take() { + self.buffer = Some(Arc::new(crate::buffer::LatestSlot::new(inner))); + } + } + fn as_any_mut(&mut self) -> &mut dyn Any { self } @@ -1334,6 +1360,13 @@ impl AnyRecord for TypedRecord { TypedRecord::has_producer(self) } + fn is_ring(&self) -> bool { + matches!( + self.buffer_cfg, + Some(crate::buffer::BufferCfg::SpmcRing { .. }) + ) + } + fn has_buffer(&self) -> bool { TypedRecord::has_buffer(self) } diff --git a/aimdb-websocket-connector/tests/http_current_values.rs b/aimdb-websocket-connector/tests/http_current_values.rs index 5543854e..99ae0a7a 100644 --- a/aimdb-websocket-connector/tests/http_current_values.rs +++ b/aimdb-websocket-connector/tests/http_current_values.rs @@ -65,8 +65,20 @@ async fn typed_latest_preserves_lookup_and_buffer_semantics() { db.latest::("second").unwrap().map(|s| s.value), Some(Reading(3)) ); - db.produce("ring", Reading(4)).unwrap(); + // A ring has no current-value slot of its own; core remembers its last + // value, so `latest` reads it without consuming anything. assert_eq!(db.latest::("ring").unwrap().map(|s| s.value), None); + let mut ring = db.subscribe::("ring").unwrap(); + db.produce("ring", Reading(4)).unwrap(); + db.produce("ring", Reading(6)).unwrap(); + for _ in 0..2 { + assert_eq!( + db.latest::("ring").unwrap().map(|s| s.value), + Some(Reading(6)) + ); + } + assert_eq!(ring.try_recv().unwrap().value, Reading(4)); + assert_eq!(ring.try_recv().unwrap().value, Reading(6)); let mut mailbox = db.subscribe::("mailbox").unwrap(); db.produce("mailbox", Reading(5)).unwrap(); assert_eq!( diff --git a/examples/remote-access-demo/src/client.rs b/examples/remote-access-demo/src/client.rs index f43d48f1..7e1ca81b 100644 --- a/examples/remote-access-demo/src/client.rs +++ b/examples/remote-access-demo/src/client.rs @@ -55,27 +55,16 @@ async fn main() -> Result<(), Box> { } println!(); - // A ring (SpmcRing) has no canonical latest, so the server falls back to - // *draining* this connection's cursor and returning the most recent value. A - // fresh cursor starts at the ring tail, so the first read is empty until a new - // value arrives — we retry over a couple of ticks. (This opens the *same* - // cursor `record.drain` uses below.) - println!("📤 record.get on Temperature (SpmcRing — drains the cursor for the latest)..."); - let mut latest = None; - for _ in 0..10 { - if let Ok(v) = conn.get_record("server::Temperature").await { - latest = Some(v); - break; - } - tokio::time::sleep(Duration::from_millis(500)).await; - } - match latest { - Some(v) => println!( - "🌡️ Most recent Temperature (via drain fallback, written {}):\n{}", + // A ring (SpmcRing) answers record.get the same way: with the last value + // written, without touching this connection's drain cursor. + println!("📤 record.get on Temperature (SpmcRing — last value written)..."); + match conn.get_record("server::Temperature").await { + Ok(v) => println!( + "🌡️ Most recent Temperature (written {}):\n{}", since_start(v.stamp()), serde_json::to_string_pretty(&v.value)? ), - None => println!("ℹ️ No reading yet — the ring cursor was still empty."), + Err(e) => println!("ℹ️ No reading yet: {e}"), } println!(); @@ -138,14 +127,11 @@ async fn main() -> Result<(), Box> { println!("🧪 Record History (record.drain)"); println!("━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━\n"); - // `record.get` above already opened this connection's Temperature cursor, so - // this drain returns only what's accrued since (they share one cursor). - println!("📤 Drain #1: history since the (already-open) cursor..."); + // The first drain opens this connection's Temperature cursor at the ring + // tail, so it returns nothing yet. + println!("📤 Drain #1: opens the cursor..."); let d1 = conn.drain_record("server::Temperature").await?; - println!( - " Values: {} (cursor shared with the record.get above)\n", - d1.count - ); + println!(" Values: {} (cursor just opened)\n", d1.count); println!("⏳ Waiting 7s for temperature readings to accumulate..."); tokio::time::sleep(Duration::from_secs(7)).await; diff --git a/tools/aimdb-mcp/CHANGELOG.md b/tools/aimdb-mcp/CHANGELOG.md index a882d070..1ba09b96 100644 --- a/tools/aimdb-mcp/CHANGELOG.md +++ b/tools/aimdb-mcp/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **`get_record` answers at once (#444).** It no longer retries `not_found` + for up to 3 s, so an empty record or a misspelled name returns immediately + instead of after 3 s. A ring returns its last value on the first call. + ### Removed (breaking) - **The architecture agent (#411).** The 16 `state.toml` tools diff --git a/tools/aimdb-mcp/src/server.rs b/tools/aimdb-mcp/src/server.rs index 937d57bb..a256d819 100644 --- a/tools/aimdb-mcp/src/server.rs +++ b/tools/aimdb-mcp/src/server.rs @@ -211,7 +211,9 @@ impl McpServer { description: "Get the current value of a specific record from an AimDB instance. \ Returns {\"value\": , \"ts\": }, where ts is \ nanoseconds since the AimDB instance started (not wall-clock time; only comparable \ - within one instance lifetime).".to_string(), + within one instance lifetime). For a ring buffer this is the last value written; \ + use drain_record for its history. Returns a not_found error at once when the \ + record does not exist or holds no value yet.".to_string(), input_schema: json!({ "type": "object", "properties": { diff --git a/tools/aimdb-mcp/src/tools/record.rs b/tools/aimdb-mcp/src/tools/record.rs index 740ce3a9..17e4b866 100644 --- a/tools/aimdb-mcp/src/tools/record.rs +++ b/tools/aimdb-mcp/src/tools/record.rs @@ -1,10 +1,9 @@ //! Record-related tools (list_records, get_record, set_record) use crate::error::{McpError, McpResult}; -use aimdb_client::{AimxConnection, ClientError}; +use aimdb_client::AimxConnection; use serde::{Deserialize, Serialize}; use serde_json::{json, Value}; -use std::time::{Duration, Instant}; use tracing::debug; /// Parameters for list_records tool @@ -162,59 +161,25 @@ pub async fn get_record(args: Option) -> McpResult { endpoint, params.record_name ); - // Reuse the *persistent* connection (the same pool `drain_record` uses) rather - // than a throwaway one. For ring buffers (`SpmcRing`, which has no canonical - // latest), the server's `record.get` falls back to *this connection's* drain - // cursor (see `aimdb_core::session::aimx::dispatch`'s `record_get`). A fresh - // connection per call opens a new cursor at the ring tail every time and always - // reads empty → `not_found`; the persistent connection lets that cursor - // accumulate. The get→drain fallback already lives server-side, so nothing is - // duplicated here — we just stop discarding the connection. - let pool = super::connection_pool() - .ok_or_else(|| McpError::Internal("Connection pool not initialized".to_string()))?; + // `record.get` is a single non-destructive read for every buffer type (rings + // included), so it needs no retry and no drain cursor: `not_found` means the + // record does not exist or holds no value yet, and is returned at once. + let client = if let Some(pool) = super::connection_pool() { + pool.get_connection(&endpoint) + .await + .map_err(McpError::Client)? + } else { + AimxConnection::connect(&endpoint) + .await + .map_err(McpError::Client)? + }; - let client_arc = pool - .get_drain_client(&endpoint) + let sample = client + .get_record(¶ms.record_name) .await .map_err(McpError::Client)?; - - let client = client_arc.lock().await; - - // A ring's drain cursor opens at the tail, so the first read is empty until a - // value is produced after it opened. Briefly retry on `not_found` so a one-shot - // `get_record` on a ring returns the latest value instead of failing. This adds - // no latency for `single_latest` records (they return immediately via the - // server's canonical-latest path) nor for an already-warm ring cursor. Note: a - // record that simply doesn't exist also reports `not_found`, so a bad name - // costs the full window before erroring. - let deadline = Instant::now() + Duration::from_secs(3); - loop { - let err = match client.get_record(¶ms.record_name).await { - Ok(sample) => { - debug!("✅ Retrieved record '{}'", params.record_name); - return Ok(stamped_json(&sample)); - } - Err(e) => e, - }; - - let cursor_warming = - matches!(&err, ClientError::ServerError { code, .. } if code == "not_found"); - - if cursor_warming && Instant::now() < deadline { - tokio::time::sleep(Duration::from_millis(250)).await; - continue; - } - - // A genuine connection/protocol error (not a warming ring cursor) means the - // persistent client is unhealthy — drop it so the next call reconnects. - if !cursor_warming { - let socket = endpoint.clone(); - let pool = pool.clone(); - tokio::spawn(async move { pool.invalidate_drain_client(&socket).await }); - } - - return Err(McpError::Client(err)); - } + debug!("✅ Retrieved record '{}'", params.record_name); + Ok(stamped_json(&sample)) } /// Set the value of a writable record From 9f4cc04ee011cdf907cbe3a290b32fb7065bb27e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 9 Nov 2026 18:35:24 +0000 Subject: [PATCH 2/5] fix(core): interrupt-safe LatestSlot on Embassy; require declared kind for raw buffers (#450) Two regressions from the previous commit, found in review: 1. On no_std, LatestSlot locked with a spin lock. An interrupt that pushed to a ring while the code it interrupted held that lock (in push or peek) would spin forever. New core feature `critical-section` makes the lock a `critical_section::Mutex`, as Embassy's own buffers use; the Embassy adapter enables it. std keeps std::sync::Mutex; other no_std builds (wasm) keep the spin lock and need no critical-section impl. 2. A ring registered through buffer_raw() has no BufferCfg, so core did not wrap it, and with the drain fallback gone record.get answered not_found forever. A remote-access record with an undeclared raw buffer now fails build() and names the fix (`.buffer_cfg(BufferCfg::SpmcRing { .. })`). A declared raw ring returns its last write. Tests cover both, plus an undeclared raw buffer without remote access, which still builds. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WGkDJ6rHvDMa63myz3jwfZ --- Cargo.lock | 1 + aimdb-core/CHANGELOG.md | 13 ++- aimdb-core/Cargo.toml | 14 +++ aimdb-core/src/buffer/latest.rs | 107 ++++++++++++++---- aimdb-core/src/typed_api.rs | 19 +++- aimdb-core/src/typed_record.rs | 17 +++ aimdb-embassy-adapter/CHANGELOG.md | 7 ++ aimdb-embassy-adapter/Cargo.toml | 3 + .../tests/remote_access_validation.rs | 77 +++++++++++++ 9 files changed, 230 insertions(+), 28 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ab1216c4..f4c01899 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -152,6 +152,7 @@ dependencies = [ "aimdb-build-info", "aimdb-derive", "async-channel", + "critical-section", "defmt 1.1.1", "futures", "futures-channel", diff --git a/aimdb-core/CHANGELOG.md b/aimdb-core/CHANGELOG.md index 74cee38b..0891719a 100644 --- a/aimdb-core/CHANGELOG.md +++ b/aimdb-core/CHANGELOG.md @@ -18,7 +18,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 connection's cursor is gone, so `record.get` no longer consumes values a later `record.drain` should return, and `not_found` now always means "no such record or no value yet". Cost: one clone and a short lock per push on - ring records. + ring records. On `no_std` the lock is a spin lock unless the new + `critical-section` feature is on, which makes it a `critical_section::Mutex` + so interrupt handlers can push to a ring; the Embassy adapter enables it. + +### Migration + +- **Declare the kind of a `buffer_raw()` buffer on remote-access records + (#450).** Core can only give a ring its readable latest value when it knows + the buffer is a ring, and `record.get` no longer drains a cursor to find one. + A record with `.with_remote_access()` and a raw buffer with no declared + `BufferCfg` now fails `build()`. Add `.buffer_cfg(BufferCfg::SpmcRing { .. })` + (or the matching kind) after `.buffer_raw(...)`, or use `buffer_with_cfg`. ### Removed (breaking) diff --git a/aimdb-core/Cargo.toml b/aimdb-core/Cargo.toml index 4fb13066..bcad3a26 100644 --- a/aimdb-core/Cargo.toml +++ b/aimdb-core/Cargo.toml @@ -65,6 +65,16 @@ log = ["dep:log"] # `critical-section` impl. observability = ["alloc", "portable-atomic/fallback", "portable-atomic/critical-section"] +# Interrupt-safe locking on no_std. Core keeps a ring's last value +# (`LatestSlot`) behind a lock. On no_std that lock is a spin lock by default, +# which deadlocks if an interrupt pushes to a ring while the code it +# interrupted holds the lock. With this feature the lock is a +# `critical_section::Mutex` instead, as in Embassy's own buffers, so pushing +# from an interrupt is safe. The final binary must provide a `critical-section` +# impl (every Embassy target does). The Embassy adapter enables it. No effect +# with `std`. +critical-section = ["dep:critical-section"] + # Testing features test-utils = ["std"] # `AimDbBuilder::start_order_seed`: permute the record task start order per @@ -122,6 +132,8 @@ log = { workspace = true, optional = true } # Synchronization primitives for no_std spin = { version = "0.9", default-features = false, features = ["mutex", "spin_mutex"] } +# Interrupt-safe lock for `LatestSlot` on no_std (feature `critical-section`). +critical-section = { workspace = true, optional = true } # Hash map for no_std (same implementation as std::collections::HashMap) hashbrown = { version = "0.15", default-features = false, features = ["default-hasher"] } @@ -134,6 +146,8 @@ tracing = { workspace = true, features = ["std"] } serde_json = { workspace = true } # For no_std testing heapless = "0.9.1" +# Host impl so `--features critical-section` tests link. +critical-section = { workspace = true, features = ["std"] } # For async testing (`sync` covers tokio::sync::mpsc in tests/session_engine.rs, # which previously leaked in from the optional [dependencies] entry) tokio = { workspace = true, features = ["macros", "rt", "time", "sync"] } diff --git a/aimdb-core/src/buffer/latest.rs b/aimdb-core/src/buffer/latest.rs index bfc1117b..b0b2a112 100644 --- a/aimdb-core/src/buffer/latest.rs +++ b/aimdb-core/src/buffer/latest.rs @@ -17,18 +17,58 @@ use super::traits::{BufferReader, DynBuffer}; #[cfg(feature = "observability")] use super::BufferMetricsSnapshot; +/// The lock around the remembered value. +/// +/// - `std`: `std::sync::Mutex`. +/// - `no_std` + feature `critical-section`: a `critical_section::Mutex`, the +/// same synchronization Embassy's own buffers use. Taking it masks +/// interrupts, so an interrupt handler can never find it held, and pushing to +/// a ring from an interrupt is safe. +/// - `no_std` without it: a spin lock. Correct where nothing preempts a holder +/// (a cooperative executor, wasm), but an interrupt that pushes while the +/// code it interrupted holds the lock would spin forever. #[cfg(feature = "std")] -type Mutex = std::sync::Mutex; -#[cfg(not(feature = "std"))] -type Mutex = spin::Mutex; +struct Slot(std::sync::Mutex>); #[cfg(feature = "std")] -fn lock(m: &Mutex) -> std::sync::MutexGuard<'_, T> { - m.lock().unwrap_or_else(|poisoned| poisoned.into_inner()) +impl Slot { + const fn new() -> Self { + Self(std::sync::Mutex::new(None)) + } + fn with(&self, f: impl FnOnce(&mut Option) -> R) -> R { + // A poisoned slot still holds a whole value: `push` only replaces it. + let mut guard = self.0.lock().unwrap_or_else(|p| p.into_inner()); + f(&mut guard) + } } -#[cfg(not(feature = "std"))] -fn lock(m: &Mutex) -> spin::MutexGuard<'_, T> { - m.lock() + +#[cfg(all(not(feature = "std"), feature = "critical-section"))] +struct Slot(critical_section::Mutex>>); + +#[cfg(all(not(feature = "std"), feature = "critical-section"))] +impl Slot { + const fn new() -> Self { + Self(critical_section::Mutex::new(core::cell::RefCell::new(None))) + } + fn with(&self, f: impl FnOnce(&mut Option) -> R) -> R { + // The critical section is not re-entered from inside `f` (it only + // clones a value or pushes to the inner buffer, which takes its own, + // nestable critical section), so the `RefCell` borrow cannot clash. + critical_section::with(|cs| f(&mut self.0.borrow_ref_mut(cs))) + } +} + +#[cfg(all(not(feature = "std"), not(feature = "critical-section")))] +struct Slot(spin::Mutex>); + +#[cfg(all(not(feature = "std"), not(feature = "critical-section")))] +impl Slot { + const fn new() -> Self { + Self(spin::Mutex::new(None)) + } + fn with(&self, f: impl FnOnce(&mut Option) -> R) -> R { + f(&mut self.0.lock()) + } } /// Wraps a buffer and remembers the last value pushed into it, so `peek()` @@ -36,29 +76,32 @@ fn lock(m: &Mutex) -> spin::MutexGuard<'_, T> { /// own. /// /// The lock is held across the inner push, so with concurrent writers the -/// remembered value is always the one the inner buffer received last. -/// `push` is synchronous and non-blocking on every adapter, so this holds the -/// lock only briefly. On `no_std` the lock is a spin lock: do not push to a -/// wrapped buffer from an interrupt handler that can preempt another pusher. +/// remembered value is always the one the inner buffer received last. Core +/// does not enforce a single producer per ring: several `Producer`s, inbound +/// connectors and `produce()` calls can all write. `push` is synchronous and +/// non-blocking on every adapter, so the lock is held only for one clone and +/// one push. See [`Slot`] for which lock is used where, and when pushing from +/// an interrupt handler is safe. pub(crate) struct LatestSlot { inner: Arc>, - latest: Mutex>, + latest: Slot, } impl LatestSlot { pub(crate) fn new(inner: Arc>) -> Self { Self { inner, - latest: Mutex::new(None), + latest: Slot::new(), } } } impl DynBuffer for LatestSlot { fn push(&self, value: T) { - let mut latest = lock(&self.latest); - *latest = Some(value.clone()); - self.inner.push(value); + self.latest.with(|latest| { + *latest = Some(value.clone()); + self.inner.push(value); + }); } fn subscribe_boxed(&self) -> Box + Send> { @@ -72,7 +115,7 @@ impl DynBuffer for LatestSlot { } fn peek(&self) -> Option { - lock(&self.latest).clone() + self.latest.with(|latest| latest.clone()) } #[cfg(feature = "observability")] @@ -93,7 +136,7 @@ mod tests { use core::task::{Context, Poll}; /// A buffer with no current-value slot that records what it received. - struct Stream(Mutex>); + struct Stream(Slot>); struct NoReader; impl BufferReader for NoReader { @@ -107,7 +150,8 @@ mod tests { impl DynBuffer for Stream { fn push(&self, value: u32) { - lock(&self.0).push(value); + self.0 + .with(|v| v.get_or_insert_with(alloc::vec::Vec::new).push(value)); } fn subscribe_boxed(&self) -> Box + Send> { Box::new(NoReader) @@ -119,25 +163,40 @@ mod tests { #[test] fn peek_is_none_before_the_first_push() { - let slot = LatestSlot::new(Arc::new(Stream(Mutex::new(alloc::vec::Vec::new())))); + let slot = LatestSlot::new(Arc::new(Stream(Slot::new()))); assert_eq!(slot.peek(), None); } #[test] fn peek_returns_the_last_push_and_the_inner_buffer_gets_every_push() { - let inner = Arc::new(Stream(Mutex::new(alloc::vec::Vec::new()))); + let inner = Arc::new(Stream(Slot::new())); let slot = LatestSlot::new(inner.clone() as Arc>); slot.push(1); slot.push(2); assert_eq!(slot.peek(), Some(2)); // peek does not consume. assert_eq!(slot.peek(), Some(2)); - assert_eq!(*lock(&inner.0), [1, 2]); + assert_eq!(inner.0.with(|v| v.clone()), Some(alloc::vec![1, 2])); + } + + /// With `critical-section`, the slot's lock is a critical section, which + /// nests: pushing or peeking from code that already masks interrupts (an + /// interrupt handler, or an inner buffer taking its own critical section) + /// does not deadlock. + #[cfg(all(not(feature = "std"), feature = "critical-section"))] + #[test] + fn push_and_peek_nest_inside_a_critical_section() { + let slot = LatestSlot::new(Arc::new(Stream(Slot::new()))); + critical_section::with(|_| { + slot.push(7); + assert_eq!(slot.peek(), Some(7)); + }); + assert_eq!(slot.peek(), Some(7)); } #[test] fn as_any_downcasts_to_the_inner_buffer() { - let slot = LatestSlot::new(Arc::new(Stream(Mutex::new(alloc::vec::Vec::new())))); + let slot = LatestSlot::new(Arc::new(Stream(Slot::new()))); assert!(slot.as_any().downcast_ref::().is_some()); } } diff --git a/aimdb-core/src/typed_api.rs b/aimdb-core/src/typed_api.rs index a3b6e700..55dcf93b 100644 --- a/aimdb-core/src/typed_api.rs +++ b/aimdb-core/src/typed_api.rs @@ -616,8 +616,18 @@ where /// - Runtime adapter implementations to provide convenient wrappers /// - Advanced use cases requiring custom buffer implementations /// - /// **Note:** For metadata tracking in std mode, call `buffer_with_cfg()` instead, - /// or call `buffer_cfg()` separately to set the configuration. + /// **Declare the buffer's kind** with [`buffer_cfg`](Self::buffer_cfg) after + /// this call (it resets any earlier declaration), or use + /// [`buffer_with_cfg`](Self::buffer_with_cfg). Core cannot see what kind of + /// buffer a raw one is, and relies on the declaration: + /// + /// - A buffer declared as [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing) + /// gets a remembered last value at build time, so `latest()` and AimX + /// `record.get` return the last write. Without the declaration a ring with + /// the default `peek()` has no readable latest value. + /// - A record with `.with_remote_access()` and an undeclared raw buffer + /// fails `build()`, rather than answering `record.get` with `not_found` + /// forever. pub fn buffer_raw( &mut self, buffer: Box>>, @@ -637,7 +647,10 @@ where self } - /// Sets the buffer configuration for metadata tracking + /// Declares the kind of the buffer set with [`buffer_raw`](Self::buffer_raw). + /// + /// Used for metadata and to decide whether core keeps a ring's last value + /// for `latest()` / `record.get`. Call it after `buffer_raw()`. pub fn buffer_cfg(&mut self, cfg: crate::buffer::BufferCfg) -> &mut Self { self.rec.set_buffer_cfg(cfg); self diff --git a/aimdb-core/src/typed_record.rs b/aimdb-core/src/typed_record.rs index b93da498..39dd2755 100644 --- a/aimdb-core/src/typed_record.rs +++ b/aimdb-core/src/typed_record.rs @@ -1320,6 +1320,23 @@ impl AnyRecord for TypedRecord { )); } + // A buffer from `buffer_raw()` hides its kind. Core gives a ring its + // readable latest value (`LatestSlot`) only when it knows the buffer is + // a ring, and `record.get` no longer falls back to draining, so an + // undeclared raw ring would answer `not_found` forever. Require the + // kind to be declared instead of guessing. + #[cfg(feature = "remote")] + if self.remote_codec.is_some() && self.has_buffer() && self.buffer_cfg.is_none() { + errors.push(crate::error::ConfigError::new( + "", + None, + "record has .with_remote_access() and a buffer from .buffer_raw() \ + with no declared kind; declare it after .buffer_raw() with \ + .buffer_cfg(BufferCfg::SpmcRing { .. }) for a ring, or the matching \ + BufferCfg otherwise, so record.get reads the right value", + )); + } + errors } diff --git a/aimdb-embassy-adapter/CHANGELOG.md b/aimdb-embassy-adapter/CHANGELOG.md index 9c772d54..e07c3593 100644 --- a/aimdb-embassy-adapter/CHANGELOG.md +++ b/aimdb-embassy-adapter/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **Rings stay interrupt-safe (#450).** The adapter enables `aimdb-core`'s + `critical-section` feature, so the last value core keeps for each ring (for + `latest()` / `record.get`) is guarded by a critical section, like the ring + itself. Pushing to a ring from an interrupt handler remains safe. + ### Removed (breaking) - **The `std` feature (#419).** It existed only so `build.rs` could reject it: diff --git a/aimdb-embassy-adapter/Cargo.toml b/aimdb-embassy-adapter/Cargo.toml index 35f7c77a..ca3453fc 100644 --- a/aimdb-embassy-adapter/Cargo.toml +++ b/aimdb-embassy-adapter/Cargo.toml @@ -55,6 +55,9 @@ observability = ["aimdb-core/observability", "embassy-runtime"] # Core AimDB types - no_std for Embassy (requires alloc for typed APIs) aimdb-core = { version = "3.0.0", path = "../aimdb-core", default-features = false, features = [ "alloc", + # Interrupt-safe lock for a ring's remembered last value (`LatestSlot`): + # rings may be pushed from interrupt handlers, as Embassy's own buffers allow. + "critical-section", ] } # Generic framed `Connection` over async UART halves (feature `connector-io`). diff --git a/aimdb-tokio-adapter/tests/remote_access_validation.rs b/aimdb-tokio-adapter/tests/remote_access_validation.rs index 454c956f..501d852d 100644 --- a/aimdb-tokio-adapter/tests/remote_access_validation.rs +++ b/aimdb-tokio-adapter/tests/remote_access_validation.rs @@ -46,3 +46,80 @@ async fn remote_access_record_with_buffer_builds() { "remote-access record with a buffer must build" ); } + +// ── Raw buffers (`buffer_raw`) ─────────────────────────────────────────────── +// +// A raw buffer hides its kind. Core keeps a ring's last value only for a buffer +// declared as `SpmcRing`, and `record.get` no longer drains a cursor to find +// one, so the kind must be declared (#444, #450). + +fn raw_ring() -> Box>> { + use aimdb_core::buffer::Buffer; + Box::new(aimdb_tokio_adapter::TokioBuffer::< + aimdb_core::Stamped, + >::new(&BufferCfg::SpmcRing { capacity: 8 })) +} + +#[tokio::test] +async fn undeclared_raw_buffer_with_remote_access_fails_build() { + let mut builder = AimDbBuilder::new().runtime(Arc::new(TokioAdapter)); + + builder.configure::("test::Config", |reg| { + reg.buffer_raw(raw_ring()).with_remote_access(); + }); + + let err = match builder.build().await { + Ok(_) => panic!("build() must reject an undeclared raw buffer on a remote-access record"), + Err(e) => e.to_string(), + }; + assert!(err.contains("buffer_cfg"), "the error names the fix: {err}"); +} + +#[tokio::test] +async fn declared_raw_ring_returns_its_last_write() { + let mut builder = AimDbBuilder::new().runtime(Arc::new(TokioAdapter)); + + builder.configure::("test::Config", |reg| { + reg.buffer_raw(raw_ring()) + .buffer_cfg(BufferCfg::SpmcRing { capacity: 8 }) + .with_remote_access(); + }); + let (db, _runner) = builder.build().await.expect("a declared raw ring builds"); + + // Nothing written yet. + assert_eq!(db.try_latest_as_json("test::Config"), None); + + let producer = db.producer::("test::Config").expect("producer"); + producer.produce(Config { threshold: 1 }); + producer.produce(Config { threshold: 2 }); + + // What `record.get` serves: the last write, readable again and again. + for _ in 0..2 { + assert_eq!( + db.try_latest_as_json("test::Config").map(|s| s.value), + Some(serde_json::json!({ "threshold": 2 })) + ); + } + assert_eq!( + db.latest::("test::Config") + .unwrap() + .map(|s| s.value), + Some(Config { threshold: 2 }) + ); +} + +#[tokio::test] +async fn undeclared_raw_buffer_without_remote_access_still_builds() { + // Only remote-access records need the declaration: `latest()` on an + // undeclared raw ring returns `None`, as it did before. + let mut builder = AimDbBuilder::new().runtime(Arc::new(TokioAdapter)); + + builder.configure::("test::Config", |reg| { + reg.buffer_raw(raw_ring()); + }); + let (db, _runner) = builder.build().await.expect("builds"); + db.producer::("test::Config") + .unwrap() + .produce(Config { threshold: 1 }); + assert_eq!(db.latest::("test::Config").unwrap(), None); +} From c307aa47828c0748bb72c8a17ef3ca6d12b74805 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 9 Nov 2026 19:06:52 +0000 Subject: [PATCH 3/5] fix(core): never hold LatestSlot's snapshot lock while foreign code runs (#450) A waker registered through Reader::poll_recv runs inside the adapter's push (tokio wakes inside send(), Embassy and wasm inside their own lock). The wrapper held its snapshot lock across that push, so a waker reading db.latest() on the same ring deadlocked on tokio and panicked on Embassy. peek() also ran T::clone under the lock, and replacing the snapshot ran T::drop under it. Split the lock in two: - the snapshot is an Arc behind a lock that only moves pointers; T::clone and T::drop run after it is released; - a writer lock keeps the snapshot ordered with the ring write. It is held across the adapter's push (no adapter offers a point between write and wake), and is re-entrant on its own thread (std) or a nesting critical section (critical-section), so a waker may also push to the same ring. The contract is written out at the top of buffer/latest.rs: where foreign code runs (wakers, clone, drop, interrupts) and the invariants that hold there. Each cell has a test: core unit tests against a fake ring that wakes synchronously, and a new test_support::assert_latest_slot_contract that the tokio, Embassy and wasm adapters run against their real rings. Each test was checked to fail on the bug it guards (old lock-across-push, no writer lock, clone under lock, non-re-entrant writer lock); deadlocks fail after 5 s instead of hanging. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WGkDJ6rHvDMa63myz3jwfZ --- aimdb-core/CHANGELOG.md | 17 +- aimdb-core/src/buffer/latest.rs | 517 ++++++++++++++++++++++---- aimdb-core/src/buffer/test_support.rs | 121 ++++++ aimdb-embassy-adapter/src/buffer.rs | 6 + aimdb-tokio-adapter/src/buffer.rs | 6 + aimdb-wasm-adapter/src/buffer.rs | 6 + 6 files changed, 605 insertions(+), 68 deletions(-) diff --git a/aimdb-core/CHANGELOG.md b/aimdb-core/CHANGELOG.md index 0770d870..4fc5f4a9 100644 --- a/aimdb-core/CHANGELOG.md +++ b/aimdb-core/CHANGELOG.md @@ -17,10 +17,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 single `peek()` for every buffer type: the ring fallback that drained the connection's cursor is gone, so `record.get` no longer consumes values a later `record.drain` should return, and `not_found` now always means "no - such record or no value yet". Cost: one clone and a short lock per push on - ring records. On `no_std` the lock is a spin lock unless the new - `critical-section` feature is on, which makes it a `critical_section::Mutex` - so interrupt handlers can push to a ring; the Embassy adapter enables it. + such record or no value yet". Cost per push on ring records: one clone, one + `Arc` allocation, and a short lock. The wrapper's contract is written out in + `buffer/latest.rs`: the snapshot matches the ring's order with concurrent + writers; no wrapper lock is held while wakers, `T::clone` or `T::drop` run, + so they may read the same ring; a waker may push to the same ring where the + adapter allows it (tokio). On `no_std` the new `critical-section` feature + makes the locks interrupt-safe; the Embassy adapter enables it. + +### Added + +- **`buffer::test_support::assert_latest_slot_contract` (#450).** Runs the + wrapper's contract against an adapter's real ring with real, synchronous + wakes. The tokio, Embassy and wasm adapters run it in their buffer suites. ### Migration diff --git a/aimdb-core/src/buffer/latest.rs b/aimdb-core/src/buffer/latest.rs index b0b2a112..bac3bd40 100644 --- a/aimdb-core/src/buffer/latest.rs +++ b/aimdb-core/src/buffer/latest.rs @@ -17,91 +17,204 @@ use super::traits::{BufferReader, DynBuffer}; #[cfg(feature = "observability")] use super::BufferMetricsSnapshot; -/// The lock around the remembered value. +// ── Contract ──────────────────────────────────────────────────────────────── +// +// Code outside core runs during `push` and `peek` at exactly these points: +// +// (W) wakers: the adapter's push wakes readers synchronously, inside the +// push call (tokio after releasing its own locks, Embassy while still +// holding its lock), so a waker registered via `Reader::poll_recv` runs +// in the middle of `LatestSlot::push`; +// (C) `T::clone`: the remembered copy, and every `peek`; +// (D) `T::drop`: the replaced snapshot, and `peek`'s temporary `Arc`; +// (I) interrupts, on Embassy: they can preempt any of the above. +// +// Invariants, each covered by a test (here or in +// `test_support::assert_latest_slot_contract`, which every adapter runs +// against its real ring): +// +// 1. Ordered: after every push has returned, `peek` returns the value the +// ring received last, also with concurrent writers. +// 2. The snapshot lock is never held while code outside core runs: not +// across (W), (C) or (D). So from any of them, `peek` on the same ring +// returns without deadlock or panic. +// 3. A waker may push to the same ring (W re-entering `push`). On std the +// writer lock is re-entrant on its own thread; with `critical-section` +// the critical section nests. (Embassy's own channel panics on a +// re-entrant publish; that is the adapter's limit, not this wrapper's.) +// 4. With `critical-section`, interrupts cannot find either lock held (I). +// +// Not supported: two threads whose wakers push into each other's rings at the +// same moment (an A→B / B→A lock order across two writer locks). Invariant 1 +// needs writers serialized across the adapter's push, and tokio and Embassy +// both wake inside it, so any lock that keeps the order is held during (W). + +/// Serializes writers of one ring across the adapter's push (invariant 1), +/// re-entrantly (invariant 3). +#[cfg(feature = "std")] +struct WriterLock { + lock: std::sync::Mutex<()>, + /// The thread holding `lock`, as [`thread_token`], or 0. + owner: core::sync::atomic::AtomicUsize, +} + +/// A non-zero id unique among live threads: the address of a thread-local. +#[cfg(feature = "std")] +fn thread_token() -> usize { + std::thread_local!(static TOKEN: u8 = const { 0 }); + TOKEN.with(|t| t as *const u8 as usize) +} + +#[cfg(feature = "std")] +impl WriterLock { + const fn new() -> Self { + Self { + lock: std::sync::Mutex::new(()), + owner: core::sync::atomic::AtomicUsize::new(0), + } + } + + fn run(&self, f: impl FnOnce() -> R) -> R { + use core::sync::atomic::Ordering::Relaxed; + let me = thread_token(); + // Only this thread ever stores `me`, and it stores 0 again before + // unlocking, so reading `me` means this thread holds the lock: a + // waker re-entering push on the thread that is pushing. + if self.owner.load(Relaxed) == me { + return f(); + } + let _guard = self.lock.lock().unwrap_or_else(|p| p.into_inner()); + self.owner.store(me, Relaxed); + struct Release<'a>(&'a core::sync::atomic::AtomicUsize); + impl Drop for Release<'_> { + fn drop(&mut self) { + self.0.store(0, Relaxed); + } + } + let _release = Release(&self.owner); + f() + } +} + +/// With `critical-section`, the critical section itself serializes writers: +/// it masks interrupts and, on multi-core targets, excludes the other cores. +/// It nests, so it is re-entrant. +#[cfg(all(not(feature = "std"), feature = "critical-section"))] +struct WriterLock; + +#[cfg(all(not(feature = "std"), feature = "critical-section"))] +impl WriterLock { + const fn new() -> Self { + Self + } + fn run(&self, f: impl FnOnce() -> R) -> R { + critical_section::with(|_| f()) + } +} + +/// no_std without `critical-section`: nothing can preempt a push (a +/// cooperative executor, wasm), so writers never overlap and there is nothing +/// to serialize. Pushing from an interrupt here is unsupported; enable +/// `critical-section`. +#[cfg(all(not(feature = "std"), not(feature = "critical-section")))] +struct WriterLock; + +#[cfg(all(not(feature = "std"), not(feature = "critical-section")))] +impl WriterLock { + const fn new() -> Self { + Self + } + fn run(&self, f: impl FnOnce() -> R) -> R { + f() + } +} + +/// The remembered value. Held as an `Arc` so the lock only ever moves or +/// copies a pointer: `T::clone` and `T::drop` run after it is released +/// (invariant 2). /// /// - `std`: `std::sync::Mutex`. -/// - `no_std` + feature `critical-section`: a `critical_section::Mutex`, the -/// same synchronization Embassy's own buffers use. Taking it masks -/// interrupts, so an interrupt handler can never find it held, and pushing to -/// a ring from an interrupt is safe. -/// - `no_std` without it: a spin lock. Correct where nothing preempts a holder -/// (a cooperative executor, wasm), but an interrupt that pushes while the -/// code it interrupted holds the lock would spin forever. +/// - `no_std` + `critical-section`: a `critical_section::Mutex` (invariant 4). +/// - `no_std` otherwise: a spin lock. #[cfg(feature = "std")] -struct Slot(std::sync::Mutex>); +struct Snapshot(std::sync::Mutex>>); #[cfg(feature = "std")] -impl Slot { +impl Snapshot { const fn new() -> Self { Self(std::sync::Mutex::new(None)) } - fn with(&self, f: impl FnOnce(&mut Option) -> R) -> R { - // A poisoned slot still holds a whole value: `push` only replaces it. + fn with(&self, f: impl FnOnce(&mut Option>) -> R) -> R { + // Only pointer moves happen under the lock, so it cannot be poisoned + // mid-update; recover anyway rather than propagate a panic. let mut guard = self.0.lock().unwrap_or_else(|p| p.into_inner()); f(&mut guard) } } #[cfg(all(not(feature = "std"), feature = "critical-section"))] -struct Slot(critical_section::Mutex>>); +struct Snapshot(critical_section::Mutex>>>); #[cfg(all(not(feature = "std"), feature = "critical-section"))] -impl Slot { +impl Snapshot { const fn new() -> Self { Self(critical_section::Mutex::new(core::cell::RefCell::new(None))) } - fn with(&self, f: impl FnOnce(&mut Option) -> R) -> R { - // The critical section is not re-entered from inside `f` (it only - // clones a value or pushes to the inner buffer, which takes its own, - // nestable critical section), so the `RefCell` borrow cannot clash. + fn with(&self, f: impl FnOnce(&mut Option>) -> R) -> R { + // `f` only moves pointers and never re-enters, so the borrow is free. critical_section::with(|cs| f(&mut self.0.borrow_ref_mut(cs))) } } #[cfg(all(not(feature = "std"), not(feature = "critical-section")))] -struct Slot(spin::Mutex>); +struct Snapshot(spin::Mutex>>); #[cfg(all(not(feature = "std"), not(feature = "critical-section")))] -impl Slot { +impl Snapshot { const fn new() -> Self { Self(spin::Mutex::new(None)) } - fn with(&self, f: impl FnOnce(&mut Option) -> R) -> R { + fn with(&self, f: impl FnOnce(&mut Option>) -> R) -> R { f(&mut self.0.lock()) } } /// Wraps a buffer and remembers the last value pushed into it, so `peek()` /// returns that value for buffers that have no current-value slot of their -/// own. +/// own. See the contract at the top of this module. /// -/// The lock is held across the inner push, so with concurrent writers the -/// remembered value is always the one the inner buffer received last. Core -/// does not enforce a single producer per ring: several `Producer`s, inbound -/// connectors and `produce()` calls can all write. `push` is synchronous and -/// non-blocking on every adapter, so the lock is held only for one clone and -/// one push. See [`Slot`] for which lock is used where, and when pushing from -/// an interrupt handler is safe. +/// Cost per push: one `T::clone` and one `Arc` allocation. pub(crate) struct LatestSlot { inner: Arc>, - latest: Slot, + writer: WriterLock, + latest: Snapshot, } impl LatestSlot { pub(crate) fn new(inner: Arc>) -> Self { Self { inner, - latest: Slot::new(), + writer: WriterLock::new(), + latest: Snapshot::new(), } } } -impl DynBuffer for LatestSlot { +impl DynBuffer for LatestSlot { fn push(&self, value: T) { - self.latest.with(|latest| { - *latest = Some(value.clone()); + // (C) outside every lock. + let snapshot = Arc::new(value.clone()); + let replaced = self.writer.run(|| { + // Snapshot first, then the ring, both under the writer lock: a + // waker woken by this push already sees this value, and no other + // writer can interleave (invariant 1). The snapshot lock is + // released before the adapter's push wakes anyone (invariant 2). + let replaced = self.latest.with(|latest| latest.replace(snapshot)); self.inner.push(value); + replaced }); + // (D) outside every lock. + drop(replaced); } fn subscribe_boxed(&self) -> Box + Send> { @@ -115,7 +228,9 @@ impl DynBuffer for LatestSlot { } fn peek(&self) -> Option { - self.latest.with(|latest| latest.clone()) + // Copy the pointer under the lock; clone and drop outside it. + let snapshot = self.latest.with(|latest| latest.clone())?; + Some(T::clone(&snapshot)) } #[cfg(feature = "observability")] @@ -131,29 +246,73 @@ impl DynBuffer for LatestSlot { #[cfg(test)] mod tests { + //! Invariants 1–4 against a fake inner buffer. The same cells run against + //! every adapter's real ring in `test_support::assert_latest_slot_contract`. use super::*; use crate::DbError; + use alloc::sync::Weak; + use alloc::vec::Vec; + use core::sync::atomic::{AtomicBool, AtomicUsize, Ordering::SeqCst}; use core::task::{Context, Poll}; - /// A buffer with no current-value slot that records what it received. - struct Stream(Slot>); + type Hook = Arc) + Send + Sync>; + type WakeHook = spin::Mutex>, Hook)>>; + #[cfg(feature = "std")] + type WriteHook = spin::Mutex>>; + + /// A ring stand-in: logs pushes in order, then runs a "wake" hook + /// synchronously, inside `push`, as tokio's and Embassy's rings do. + struct Ring { + log: spin::Mutex>, + /// Yield before logging, to widen the window between the snapshot + /// update and the ring write, so a missing writer lock shows up. + #[cfg(feature = "std")] + yield_before_write: bool, + /// Runs before the write; lets a test hold one writer inside it. + #[cfg(feature = "std")] + before_write: WriteHook, + on_wake: WakeHook, + } + + impl Ring { + fn last(&self) -> Option { + self.log.lock().last().cloned() + } + } struct NoReader; - impl BufferReader for NoReader { - fn poll_recv(&mut self, _cx: &mut Context<'_>) -> Poll> { + impl BufferReader for NoReader { + fn poll_recv(&mut self, _cx: &mut Context<'_>) -> Poll> { Poll::Pending } - fn try_recv(&mut self) -> Result { + fn try_recv(&mut self) -> Result { Err(DbError::BufferEmpty) } } - impl DynBuffer for Stream { - fn push(&self, value: u32) { - self.0 - .with(|v| v.get_or_insert_with(alloc::vec::Vec::new).push(value)); + impl DynBuffer for Ring { + fn push(&self, value: T) { + #[cfg(feature = "std")] + if self.yield_before_write { + extern crate std; + std::thread::yield_now(); + } + #[cfg(feature = "std")] + { + let hook = self.before_write.lock().clone(); + if let Some(hook) = hook { + hook(&value); + } + } + self.log.lock().push(value); + let hook = self.on_wake.lock().clone(); + if let Some((slot, hook)) = hook { + if let Some(slot) = slot.upgrade() { + hook(&slot); + } + } } - fn subscribe_boxed(&self) -> Box + Send> { + fn subscribe_boxed(&self) -> Box + Send> { Box::new(NoReader) } fn as_any(&self) -> &dyn core::any::Any { @@ -161,42 +320,272 @@ mod tests { } } + fn ring() -> (Arc>, Arc>) { + let ring = Arc::new(Ring { + log: spin::Mutex::new(Vec::new()), + #[cfg(feature = "std")] + yield_before_write: false, + #[cfg(feature = "std")] + before_write: spin::Mutex::new(None), + on_wake: spin::Mutex::new(None), + }); + let slot = Arc::new(LatestSlot::new(ring.clone() as Arc>)); + (ring, slot) + } + + fn on_wake(ring: &Ring, slot: &Arc>, hook: Hook) { + *ring.on_wake.lock() = Some((Arc::downgrade(slot), hook)); + } + + /// Runs `f`, failing with "deadlock" instead of hanging if it does not + /// return within 5 s (std); runs it directly elsewhere. + fn no_deadlock(f: impl FnOnce() + Send + 'static) { + #[cfg(feature = "std")] + { + extern crate std; + let (done, rx) = std::sync::mpsc::channel(); + let t = std::thread::spawn(move || { + f(); + let _ = done.send(()); + }); + match rx.recv_timeout(std::time::Duration::from_secs(5)) { + Ok(()) => t.join().unwrap(), + Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { + // `f` panicked: re-raise its panic. + if let Err(e) = t.join() { + std::panic::resume_unwind(e); + } + } + Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { + panic!("deadlock: no return within 5 s") + } + } + } + #[cfg(not(feature = "std"))] + f(); + } + #[test] fn peek_is_none_before_the_first_push() { - let slot = LatestSlot::new(Arc::new(Stream(Slot::new()))); + let (_, slot) = ring::(); assert_eq!(slot.peek(), None); } #[test] - fn peek_returns_the_last_push_and_the_inner_buffer_gets_every_push() { - let inner = Arc::new(Stream(Slot::new())); - let slot = LatestSlot::new(inner.clone() as Arc>); + fn peek_returns_the_last_push_and_the_ring_gets_every_push() { + let (ring, slot) = ring::(); slot.push(1); slot.push(2); assert_eq!(slot.peek(), Some(2)); - // peek does not consume. - assert_eq!(slot.peek(), Some(2)); - assert_eq!(inner.0.with(|v| v.clone()), Some(alloc::vec![1, 2])); + assert_eq!(slot.peek(), Some(2), "peek does not consume"); + assert_eq!(*ring.log.lock(), [1, 2]); + } + + #[test] + fn as_any_downcasts_to_the_inner_buffer() { + let (_, slot) = ring::(); + assert!(slot.as_any().downcast_ref::>().is_some()); + } + + /// Invariants 1 + 2 (W × peek): the waker sees the value being pushed. + #[test] + fn a_waker_can_peek_the_ring_it_was_woken_by() { + no_deadlock(|| { + let (ring, slot) = ring::(); + let seen = Arc::new(spin::Mutex::new(Vec::new())); + let log = seen.clone(); + on_wake(&ring, &slot, Arc::new(move |s| log.lock().push(s.peek()))); + slot.push(1); + slot.push(2); + assert_eq!(*seen.lock(), [Some(1), Some(2)]); + }); + } + + /// Invariant 3 (W × push same ring): ends on the ring's last value. + #[test] + fn a_waker_can_push_to_the_ring_it_was_woken_by() { + no_deadlock(|| { + let (ring, slot) = ring::(); + let once = Arc::new(AtomicBool::new(true)); + on_wake( + &ring, + &slot, + Arc::new(move |s| { + if once.swap(false, SeqCst) { + s.push(20); + } + }), + ); + slot.push(10); + assert_eq!(*ring.log.lock(), [10, 20]); + assert_eq!(slot.peek(), ring.last()); + }); + } + + /// W × push other ring: a waker of ring A pushes to ring B. + #[test] + fn a_waker_can_push_to_another_ring() { + no_deadlock(|| { + let (ring_a, slot_a) = ring::(); + let (ring_b, slot_b) = ring::(); + let target = slot_b.clone(); + on_wake(&ring_a, &slot_a, Arc::new(move |_| target.push(7))); + slot_a.push(1); + assert_eq!(slot_b.peek(), Some(7)); + assert_eq!(slot_b.peek(), ring_b.last()); + assert_eq!(slot_a.peek(), ring_a.last()); + }); + } + + /// A value whose `Clone` and `Drop` read the ring it lives in. + struct Probe { + n: u32, + slot: Arc>>>, + calls: Arc, + busy: Arc, + } + + impl Probe { + fn fire(&self) { + // Guard: the peek below clones and drops a Probe itself. + if self.busy.swap(true, SeqCst) { + return; + } + let slot = self.slot.lock().upgrade(); + if let Some(slot) = slot { + let _ = slot.peek(); + } + self.calls.fetch_add(1, SeqCst); + self.busy.store(false, SeqCst); + } } - /// With `critical-section`, the slot's lock is a critical section, which - /// nests: pushing or peeking from code that already masks interrupts (an - /// interrupt handler, or an inner buffer taking its own critical section) - /// does not deadlock. + impl Clone for Probe { + fn clone(&self) -> Self { + self.fire(); + Self { + n: self.n, + slot: self.slot.clone(), + calls: self.calls.clone(), + busy: self.busy.clone(), + } + } + } + + impl Drop for Probe { + fn drop(&mut self) { + self.fire(); + } + } + + /// Invariant 2 (C × peek, D × peek): `T::clone` and `T::drop` run + /// outside the snapshot lock, so they can read the same ring. + #[test] + fn clone_and_drop_can_peek_the_same_ring() { + no_deadlock(|| { + let (_ring, slot) = ring::(); + let cell = Arc::new(spin::Mutex::new(Arc::downgrade(&slot))); + let calls = Arc::new(AtomicUsize::new(0)); + let busy = Arc::new(AtomicBool::new(false)); + let probe = |n| Probe { + n, + slot: cell.clone(), + calls: calls.clone(), + busy: busy.clone(), + }; + slot.push(probe(1)); // clone for the snapshot + slot.push(probe(2)); // clone, and drop of the replaced snapshot + assert_eq!(slot.peek().map(|p| p.n), Some(2)); // clone + drop + assert!(calls.load(SeqCst) >= 4, "clone and drop each read the ring"); + }); + } + + /// Invariant 1 under real concurrency: after every push has returned, + /// `peek` is the value the ring received last. + #[cfg(feature = "std")] + #[test] + fn concurrent_writers_end_on_the_rings_last_value() { + extern crate std; + for _ in 0..20 { + let ring = Arc::new(Ring { + log: spin::Mutex::new(Vec::new()), + yield_before_write: true, + before_write: spin::Mutex::new(None), + on_wake: spin::Mutex::new(None), + }); + let slot = Arc::new(LatestSlot::new(ring.clone() as Arc>)); + let threads: Vec<_> = (0..8u32) + .map(|t| { + let slot = slot.clone(); + std::thread::spawn(move || { + for i in 0..500 { + slot.push(t * 1000 + i); + } + }) + }) + .collect(); + for t in threads { + t.join().unwrap(); + } + assert_eq!(ring.log.lock().len(), 4000); + assert_eq!(slot.peek(), ring.last()); + } + } + + /// Invariant 1, deterministically: writer A is held inside the ring write + /// until writer B's push returns (or 200 ms pass). Without a writer lock + /// B overtakes A, so the ring ends on A while the snapshot says B. With + /// it, B waits for A and the two agree. + #[cfg(feature = "std")] + #[test] + fn a_second_writer_cannot_overtake_one_inside_the_ring_write() { + extern crate std; + use std::sync::mpsc; + use std::time::Duration; + + let (ring, slot) = ring::(); + let (b_done, b_done_rx) = mpsc::channel::<()>(); + let (a_inside, a_inside_rx) = mpsc::channel::<()>(); + let b_done_rx = std::sync::Mutex::new(b_done_rx); + let a_inside = std::sync::Mutex::new(a_inside); + *ring.before_write.lock() = Some(Arc::new(move |v: &u32| { + if *v == 1 { + a_inside.lock().unwrap().send(()).unwrap(); + let _ = b_done_rx + .lock() + .unwrap() + .recv_timeout(Duration::from_millis(200)); + } + })); + + let a = { + let slot = slot.clone(); + std::thread::spawn(move || slot.push(1)) + }; + a_inside_rx.recv().unwrap(); + let b = { + let slot = slot.clone(); + std::thread::spawn(move || { + slot.push(2); + let _ = b_done.send(()); + }) + }; + a.join().unwrap(); + b.join().unwrap(); + assert_eq!(slot.peek(), ring.last()); + } + + /// Invariant 4: with `critical-section`, push and peek nest inside a + /// critical section that is already held (an interrupt handler, or an + /// adapter taking its own). #[cfg(all(not(feature = "std"), feature = "critical-section"))] #[test] fn push_and_peek_nest_inside_a_critical_section() { - let slot = LatestSlot::new(Arc::new(Stream(Slot::new()))); + let (ring, slot) = ring::(); critical_section::with(|_| { slot.push(7); assert_eq!(slot.peek(), Some(7)); }); - assert_eq!(slot.peek(), Some(7)); - } - - #[test] - fn as_any_downcasts_to_the_inner_buffer() { - let slot = LatestSlot::new(Arc::new(Stream(Slot::new()))); - assert!(slot.as_any().downcast_ref::().is_some()); + assert_eq!(slot.peek(), ring.last()); } } diff --git a/aimdb-core/src/buffer/test_support.rs b/aimdb-core/src/buffer/test_support.rs index d356021c..ee73b9c1 100644 --- a/aimdb-core/src/buffer/test_support.rs +++ b/aimdb-core/src/buffer/test_support.rs @@ -207,3 +207,124 @@ where "SpmcRing peek must be None (no canonical latest)" ); } + +/// Whether an adapter's ring tolerates a push from inside one of its own +/// wakes, for [`assert_latest_slot_contract`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ReentrantPush { + /// The ring wakes readers after releasing its own lock (tokio), so a + /// waker may push to the same ring. + Supported, + /// The ring wakes readers while holding its own lock (Embassy, wasm), so a + /// re-entrant push fails inside the adapter. That cell is skipped. + AdapterLimit, +} + +/// Asserts the contract of the latest-value wrapper core puts around every +/// `SpmcRing` (`LatestSlot`, see `buffer/latest.rs`) against an adapter's +/// real ring, with its real, synchronous wakes. +/// +/// `mk` must produce an `SpmcRing` whose capacity holds at least 4 values. +/// +/// Cells covered (foreign code × what it does): +/// - a waker, run inside `push`, reads `peek` on the same ring: it sees the +/// value being pushed, without deadlock or panic; +/// - a waker pushes to the same ring (when `reentrant` is +/// [`ReentrantPush::Supported`]): `peek` ends on the ring's last value; +/// - the reader receives every value in ring order, and `peek` is +/// non-destructive. +pub async fn assert_latest_slot_contract(mk: F, reentrant: ReentrantPush) +where + B: Buffer> + DynBuffer> + 'static, + F: Fn() -> B, +{ + use alloc::sync::Arc; + use alloc::task::Wake; + use alloc::vec::Vec; + use core::task::{Context, Poll, Waker}; + + use super::LatestSlot; + + type Slot = LatestSlot>; + + /// On wake: read the slot, and optionally push `push_on_wake` once. + struct Probe { + slot: Arc, + seen: spin::Mutex>>>, + push_on_wake: spin::Mutex>>, + } + impl Wake for Probe { + fn wake(self: Arc) { + self.wake_by_ref(); + } + fn wake_by_ref(self: &Arc) { + let seen = self.slot.peek(); + self.seen.lock().push(seen); + let push = self.push_on_wake.lock().take(); + if let Some(v) = push { + self.slot.push(v); + } + } + } + + let new_slot = || Arc::new(LatestSlot::new(Arc::new(mk()) as Arc>)); + + // ── A waker reads the same ring during the push that woke it ────────── + let slot = new_slot(); + let probe = Arc::new(Probe { + slot: slot.clone(), + seen: spin::Mutex::new(Vec::new()), + push_on_wake: spin::Mutex::new(None), + }); + let waker = Waker::from(probe.clone()); + let mut reader = slot.subscribe_boxed(); + assert!( + reader + .poll_recv(&mut Context::from_waker(&waker)) + .is_pending(), + "an empty ring is pending" + ); + slot.push(sample(1)); + assert_eq!( + probe.seen.lock().first().cloned(), + Some(Some(sample(1))), + "the push woke the reader synchronously, and the waker's peek saw the \ + value being pushed" + ); + assert!( + matches!( + reader.poll_recv(&mut Context::from_waker(&waker)), + Poll::Ready(Ok(ref v)) if *v == sample(1) + ), + "the reader receives the value" + ); + assert_eq!(slot.peek(), Some(sample(1)), "peek is non-destructive"); + assert_eq!(slot.peek(), Some(sample(1))); + + // ── A waker pushes to the same ring ────────────────────────────────── + if reentrant == ReentrantPush::Supported { + let slot = new_slot(); + let probe = Arc::new(Probe { + slot: slot.clone(), + seen: spin::Mutex::new(Vec::new()), + push_on_wake: spin::Mutex::new(Some(sample(3))), + }); + let waker = Waker::from(probe.clone()); + let mut reader = slot.subscribe_boxed(); + assert!(reader + .poll_recv(&mut Context::from_waker(&waker)) + .is_pending()); + // 2 enters the ring, wakes the probe, which pushes 3 from inside. + slot.push(sample(2)); + let mut received = Vec::new(); + while let Poll::Ready(Ok(v)) = reader.poll_recv(&mut Context::from_waker(&waker)) { + received.push(v); + } + assert_eq!(received, [sample(2), sample(3)], "ring order"); + assert_eq!( + slot.peek(), + received.last().cloned(), + "peek ends on the value the ring received last" + ); + } +} diff --git a/aimdb-embassy-adapter/src/buffer.rs b/aimdb-embassy-adapter/src/buffer.rs index a502e689..16c1fe45 100644 --- a/aimdb-embassy-adapter/src/buffer.rs +++ b/aimdb-embassy-adapter/src/buffer.rs @@ -872,6 +872,12 @@ mod tests { block_on(test_support::assert_single_latest_contract(B::new_watch)); block_on(test_support::assert_mailbox_contract(B::new_mailbox)); block_on(test_support::assert_spmc_ring_contract(B::new_spmc)); + // PubSub wakes subscribers inside its own lock, so a push from a waker + // fails in embassy-sync itself; that cell is the adapter's limit. + block_on(test_support::assert_latest_slot_contract( + B::new_spmc, + test_support::ReentrantPush::AdapterLimit, + )); } /// Design 058 §4.5: the write stamp costs every slot. The buffer keeps diff --git a/aimdb-tokio-adapter/src/buffer.rs b/aimdb-tokio-adapter/src/buffer.rs index 343028a0..6a968c09 100644 --- a/aimdb-tokio-adapter/src/buffer.rs +++ b/aimdb-tokio-adapter/src/buffer.rs @@ -1020,6 +1020,12 @@ mod tests { TokioBuffer::>::new(&BufferCfg::SpmcRing { capacity: 4 }) }) .await; + // broadcast wakes after releasing its own lock, so a waker may push. + test_support::assert_latest_slot_contract( + || TokioBuffer::>::new(&BufferCfg::SpmcRing { capacity: 4 }), + test_support::ReentrantPush::Supported, + ) + .await; } #[tokio::test] diff --git a/aimdb-wasm-adapter/src/buffer.rs b/aimdb-wasm-adapter/src/buffer.rs index 9e1b8997..9b8022c7 100644 --- a/aimdb-wasm-adapter/src/buffer.rs +++ b/aimdb-wasm-adapter/src/buffer.rs @@ -558,6 +558,12 @@ mod tests { block_on(test_support::assert_spmc_ring_contract(|| { WasmBuffer::>::new(&BufferCfg::SpmcRing { capacity: 4 }) })); + // Wakes run inside the buffer's `RefCell` borrow, so a push from a waker + // fails in the adapter itself; that cell is the adapter's limit. + block_on(test_support::assert_latest_slot_contract( + || WasmBuffer::>::new(&BufferCfg::SpmcRing { capacity: 4 }), + test_support::ReentrantPush::AdapterLimit, + )); } // ======================================================================== From b81eaee2d96f557ce64f4198e3ee362cd1cc34aa Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 9 Nov 2026 19:22:15 +0000 Subject: [PATCH 4/5] refactor: narrow #450 to record.get; drop LatestSlot (#444) Per review, scope this PR to the 3 s delay in #444 and to separating record.get from record.drain. LatestSlot added a heap allocation to every ring write and held a writer lock while waking readers, and it broke the connector allocation gate; it is removed, along with the critical-section feature, the buffer_raw() build error and the shared LatestSlot suite. A buffer-native snapshot, if needed, is a separate design. - aimdb-mcp: get_record drops the 3 s retry and the drain client. - Server: record.get is one non-destructive read of the buffer's own snapshot and never touches the drain cursor; the get-to-drain fallback is gone. Only record.drain advances its cursor. - Unknown and empty records answer not_found at once. A ring answers the new RpcError::Unsupported (wire code "unsupported"); read it with record.drain or record.subscribe. - aimdb-client: maps Unsupported; watch::arm treats it as "no baseline". Tests: an MCP integration test against a real Unix-socket server and a client session test check that the misses answer at once and that drain still returns every event after gets. Both fail on the old code (6 s of retries; a ring answering not_found). The connector allocation gate (b0_alloc_connector) passes. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WGkDJ6rHvDMa63myz3jwfZ --- CHANGELOG.md | 10 +- Cargo.lock | 4 +- aimdb-client/CHANGELOG.md | 7 +- aimdb-client/src/engine.rs | 19 +- aimdb-client/src/watch.rs | 9 +- aimdb-client/tests/aimx_session.rs | 79 ++- aimdb-core/CHANGELOG.md | 41 +- aimdb-core/Cargo.toml | 14 - aimdb-core/src/buffer/latest.rs | 591 ------------------ aimdb-core/src/buffer/mod.rs | 2 - aimdb-core/src/buffer/test_support.rs | 121 ---- aimdb-core/src/buffer/traits.rs | 10 +- aimdb-core/src/builder.rs | 17 +- aimdb-core/src/outbound/latest.rs | 3 +- aimdb-core/src/session/aimx/codec.rs | 3 + aimdb-core/src/session/aimx/dispatch.rs | 54 +- aimdb-core/src/session/mod.rs | 3 + aimdb-core/src/typed_api.rs | 31 +- aimdb-core/src/typed_record.rs | 58 +- aimdb-embassy-adapter/CHANGELOG.md | 7 - aimdb-embassy-adapter/Cargo.toml | 3 - aimdb-embassy-adapter/src/buffer.rs | 6 - aimdb-tokio-adapter/src/buffer.rs | 6 - .../tests/remote_access_validation.rs | 77 --- aimdb-wasm-adapter/src/buffer.rs | 6 - aimdb-wasm-adapter/src/ws_bridge.rs | 1 + .../tests/http_current_values.rs | 14 +- examples/remote-access-demo/src/client.rs | 15 +- tools/aimdb-cli/README.md | 4 +- tools/aimdb-mcp/CHANGELOG.md | 7 +- tools/aimdb-mcp/Cargo.toml | 4 + tools/aimdb-mcp/src/server.rs | 6 +- tools/aimdb-mcp/src/tools/record.rs | 6 +- tools/aimdb-mcp/tests/get_record.rs | 106 ++++ 34 files changed, 295 insertions(+), 1049 deletions(-) delete mode 100644 aimdb-core/src/buffer/latest.rs create mode 100644 tools/aimdb-mcp/tests/get_record.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index d1c85699..231d2921 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,12 +31,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Fixed — `record.get` on rings and empty records (#444) +### Fixed — `record.get` answers at once and no longer drains (#444) -- **Every buffer type now has a readable latest value.** Core remembers a - ring's last value, so `record.get` and `latest()` work on `SpmcRing` records - without draining a cursor, and `aimdb-mcp`'s `get_record` drops its 3 s retry. - See [aimdb-core](aimdb-core/CHANGELOG.md), +- **`record.get` is a single, non-destructive read.** `aimdb-mcp`'s + `get_record` drops its 3 s retry, the server drops the get-to-drain fallback, + and a ring answers `unsupported` instead of consuming events. See + [aimdb-core](aimdb-core/CHANGELOG.md), [aimdb-client](aimdb-client/CHANGELOG.md) and [aimdb-mcp](tools/aimdb-mcp/CHANGELOG.md). diff --git a/Cargo.lock b/Cargo.lock index f4c01899..18ac70c1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -152,7 +152,6 @@ dependencies = [ "aimdb-build-info", "aimdb-derive", "async-channel", - "critical-section", "defmt 1.1.1", "futures", "futures-channel", @@ -249,12 +248,15 @@ version = "0.10.0" dependencies = [ "aimdb-client", "aimdb-core", + "aimdb-tokio-adapter", + "aimdb-uds-connector", "anyhow", "chrono", "clap", "once_cell", "serde", "serde_json", + "tempfile", "thiserror 1.0.69", "tokio", "tokio-test", diff --git a/aimdb-client/CHANGELOG.md b/aimdb-client/CHANGELOG.md index dd913b4d..c79cba66 100644 --- a/aimdb-client/CHANGELOG.md +++ b/aimdb-client/CHANGELOG.md @@ -9,9 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- **`get_record` on a ring returns its last value (#444).** It no longer moves - the connection's drain cursor, and it no longer comes back empty until the - ring produces after the first read. +- **`get_record` on a ring returns `unsupported` (#444).** A ring has no + current value; use `drain_record` or `subscribe`. `get_record` no longer moves + the connection's drain cursor, so `drain_record` returns every unread event. + `watch::arm` treats `unsupported` like `not_found`: no baseline. ### Removed (breaking) diff --git a/aimdb-client/src/engine.rs b/aimdb-client/src/engine.rs index e6bfa4bb..e12f0534 100644 --- a/aimdb-client/src/engine.rs +++ b/aimdb-client/src/engine.rs @@ -239,11 +239,14 @@ impl AimxConnection { /// Get a record's current value. /// - /// A non-destructive read of the last value written, for every buffer type - /// (`SingleLatest`, `Mailbox` and `SpmcRing`). It never moves this - /// connection's drain cursor; use [`drain_record`](Self::drain_record) for a - /// ring's history. A server error with code `not_found` means the record does - /// not exist or holds no value yet. + /// A single, non-destructive read of the record's snapshot (`SingleLatest`, + /// `Mailbox`). It returns at once and never moves this connection's drain + /// cursor. Server errors: + /// + /// - `not_found`: no such record, or no value yet. + /// - `unsupported`: the record is a ring (`SpmcRing`), which has no current + /// value. Use [`drain_record`](Self::drain_record) for unread events or + /// [`subscribe`](Self::subscribe) for new ones. /// /// The value comes with the stamp of its write; a reply without `ts` is a /// [`ClientError::Protocol`]. @@ -570,6 +573,12 @@ fn rpc_err(e: RpcError) -> ClientError { ClientError::server_error("not_found", "method or record not found", None) } RpcError::Denied => ClientError::server_error("denied", "permission denied", None), + RpcError::Unsupported => ClientError::server_error( + "unsupported", + "not supported for this record (a ring has no current value: \ + use drain_record for unread events or subscribe for new ones)", + None, + ), // `Internal` today, plus any future non-exhaustive variant. _ => ClientError::server_error("internal", "engine/transport failure", None), } diff --git a/aimdb-client/src/watch.rs b/aimdb-client/src/watch.rs index 5ffbd5f5..c577aa6d 100644 --- a/aimdb-client/src/watch.rs +++ b/aimdb-client/src/watch.rs @@ -84,8 +84,13 @@ pub async fn arm(conn: &AimxConnection, record: &str) -> Result Some(current.stamp()), // No value yet (or an unknown record, whose subscribe refusal is - // picked up below). - Err(ClientError::ServerError { code, .. }) if code == "not_found" => None, + // picked up below), or a ring: it has no current value, and its + // subscription starts after the last write, so nothing old can count. + Err(ClientError::ServerError { code, .. }) + if code == "not_found" || code == "unsupported" => + { + None + } Err(e) => return Err(e), }; let stream = match stream.next().now_or_never() { diff --git a/aimdb-client/tests/aimx_session.rs b/aimdb-client/tests/aimx_session.rs index 58967aa1..6b3ec050 100644 --- a/aimdb-client/tests/aimx_session.rs +++ b/aimdb-client/tests/aimx_session.rs @@ -326,10 +326,11 @@ async fn exact_subscribe_sends_the_current_value_once() { } } -/// `record.get` on a ring (`SpmcRing`) record returns the last value written, -/// on the first call, without moving the connection's drain cursor. +/// `record.get` is one non-destructive read (#444): it answers at once for an +/// empty record, an unknown name and a ring, and never moves the connection's +/// drain cursor, so `record.drain` still returns every unread event. #[tokio::test] -async fn record_get_on_ring_returns_last_value_without_draining() { +async fn record_get_answers_at_once_and_leaves_drain_intact() { let dir = tempfile::tempdir().unwrap(); let sock = dir.path().join("aimdb.sock"); @@ -341,6 +342,9 @@ async fn record_get_on_ring_returns_last_value_without_draining() { reg.buffer(BufferCfg::SpmcRing { capacity: 16 }) .with_remote_access(); }); + builder.configure::("state", |reg| { + reg.buffer(BufferCfg::SingleLatest).with_remote_access(); + }); let (db, runner) = builder.build().await.expect("build db"); let db = Arc::new(db); tokio::spawn(runner.run()); @@ -349,35 +353,60 @@ async fn record_get_on_ring_returns_last_value_without_draining() { .await .expect("connect"); - // Nothing written yet: not_found straight away. - assert!(conn.get_record("stream").await.is_err()); - - // Values written before this connection read the record are visible. - let producer = db.producer::("stream").expect("producer"); - producer.produce(Reading { n: 1 }); - producer.produce(Reading { n: 2 }); - - let got = conn.get_record("stream").await.expect("ring get"); - assert_eq!(got.value, serde_json::json!({ "n": 2 })); - // Non-destructive: the same value again. - let again = conn.get_record("stream").await.expect("ring get again"); - assert_eq!(again.value, serde_json::json!({ "n": 2 })); - - // `record.get` opened no drain cursor: the first drain opens one at the - // tail and sees only what is written after it. - let first = conn.drain_record("stream").await.expect("drain"); - assert_eq!(first.count, 0); - producer.produce(Reading { n: 3 }); + // Each miss answers at once (the old MCP retry took 3 s per read). + let code = |r: Result<_, aimdb_client::ClientError>| match r { + Err(aimdb_client::ClientError::ServerError { code, .. }) => code, + other => panic!("expected a server error, got {other:?}"), + }; + let started = std::time::Instant::now(); + assert_eq!(code(conn.get_record("state").await), "not_found", "empty"); + assert_eq!( + code(conn.get_record("missing").await), + "not_found", + "unknown" + ); + assert_eq!(code(conn.get_record("stream").await), "unsupported", "ring"); + assert!( + started.elapsed() < Duration::from_secs(1), + "three misses took {:?}", + started.elapsed() + ); + + // Open the drain cursors, then write. + assert_eq!(conn.drain_record("stream").await.expect("drain").count, 0); + assert_eq!(conn.drain_record("state").await.expect("drain").count, 0); + let ring = db.producer::("stream").expect("producer"); + let state = db.producer::("state").expect("producer"); + for n in 1..=3 { + ring.produce(Reading { n }); + } + state.produce(Reading { n: 7 }); + + // Reads in between: the ring still refuses, the state answers, twice. + assert_eq!(code(conn.get_record("stream").await), "unsupported"); + for _ in 0..2 { + let got = conn.get_record("state").await.expect("state get"); + assert_eq!(got.value, serde_json::json!({ "n": 7 })); + } + + // Drain still sees every event the gets did not consume. let mut drained = Vec::new(); for _ in 0..50 { drained.extend(conn.drain_record("stream").await.expect("drain").values); - if !drained.is_empty() { + if drained.len() >= 3 { break; } tokio::time::sleep(Duration::from_millis(10)).await; } - assert_eq!(drained.len(), 1); - assert_eq!(drained[0].value, serde_json::json!({ "n": 3 })); + let ns: Vec<_> = drained.iter().map(|v| v.value["n"].clone()).collect(); + assert_eq!(ns, [1, 2, 3]); + let state_drain = conn.drain_record("state").await.expect("drain"); + let ns: Vec<_> = state_drain + .values + .iter() + .map(|v| v.value["n"].clone()) + .collect(); + assert_eq!(ns, [7]); drop(conn); } diff --git a/aimdb-core/CHANGELOG.md b/aimdb-core/CHANGELOG.md index 4fc5f4a9..b1504856 100644 --- a/aimdb-core/CHANGELOG.md +++ b/aimdb-core/CHANGELOG.md @@ -9,36 +9,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed (wire) -- **`record.get` and `latest()` read a ring's last value (#444).** Core wraps - every `SpmcRing` buffer in `LatestSlot` at build time, which remembers the - last value pushed and answers `peek()`. `TypedRecord::latest()`, - `AimDb::latest()` and `try_latest_as_json*` now return it instead of `None`, - on every runtime adapter, with no adapter changes. AimX `record.get` is a - single `peek()` for every buffer type: the ring fallback that drained the - connection's cursor is gone, so `record.get` no longer consumes values a - later `record.drain` should return, and `not_found` now always means "no - such record or no value yet". Cost per push on ring records: one clone, one - `Arc` allocation, and a short lock. The wrapper's contract is written out in - `buffer/latest.rs`: the snapshot matches the ring's order with concurrent - writers; no wrapper lock is held while wakers, `T::clone` or `T::drop` run, - so they may read the same ring; a waker may push to the same ring where the - adapter allows it (tokio). On `no_std` the new `critical-section` feature - makes the locks interrupt-safe; the Embassy adapter enables it. +- **`record.get` is a single, non-destructive read (#444).** It returns the + buffer's own snapshot (`SingleLatest`, `Mailbox`) and never touches the + connection's drain cursor, so only `record.drain` advances it. The fallback + that drained a ring to answer `record.get` is gone: it made a point-in-time + read wait for a future write and consumed events a later `record.drain` + should have returned. A ring (`SpmcRing`) now answers with the new error + code `unsupported`; read it with `record.drain` (unread events) or + `record.subscribe` (new ones). An empty `SingleLatest`/`Mailbox` record and + an unknown name answer `not_found` at once. A ring registered with + `buffer_raw()` and no `buffer_cfg()` answers `not_found`, since core cannot + tell it is a ring. ### Added -- **`buffer::test_support::assert_latest_slot_contract` (#450).** Runs the - wrapper's contract against an adapter's real ring with real, synchronous - wakes. The tokio, Embassy and wasm adapters run it in their buffer suites. - -### Migration - -- **Declare the kind of a `buffer_raw()` buffer on remote-access records - (#450).** Core can only give a ring its readable latest value when it knows - the buffer is a ring, and `record.get` no longer drains a cursor to find one. - A record with `.with_remote_access()` and a raw buffer with no declared - `BufferCfg` now fails `build()`. Add `.buffer_cfg(BufferCfg::SpmcRing { .. })` - (or the matching kind) after `.buffer_raw(...)`, or use `buffer_with_cfg`. +- **`RpcError::Unsupported` (#444),** wire code `"unsupported"`: the method + exists but the target cannot serve it. Older clients decode an unknown code + as `internal`. ### Removed (breaking) diff --git a/aimdb-core/Cargo.toml b/aimdb-core/Cargo.toml index bcad3a26..4fb13066 100644 --- a/aimdb-core/Cargo.toml +++ b/aimdb-core/Cargo.toml @@ -65,16 +65,6 @@ log = ["dep:log"] # `critical-section` impl. observability = ["alloc", "portable-atomic/fallback", "portable-atomic/critical-section"] -# Interrupt-safe locking on no_std. Core keeps a ring's last value -# (`LatestSlot`) behind a lock. On no_std that lock is a spin lock by default, -# which deadlocks if an interrupt pushes to a ring while the code it -# interrupted holds the lock. With this feature the lock is a -# `critical_section::Mutex` instead, as in Embassy's own buffers, so pushing -# from an interrupt is safe. The final binary must provide a `critical-section` -# impl (every Embassy target does). The Embassy adapter enables it. No effect -# with `std`. -critical-section = ["dep:critical-section"] - # Testing features test-utils = ["std"] # `AimDbBuilder::start_order_seed`: permute the record task start order per @@ -132,8 +122,6 @@ log = { workspace = true, optional = true } # Synchronization primitives for no_std spin = { version = "0.9", default-features = false, features = ["mutex", "spin_mutex"] } -# Interrupt-safe lock for `LatestSlot` on no_std (feature `critical-section`). -critical-section = { workspace = true, optional = true } # Hash map for no_std (same implementation as std::collections::HashMap) hashbrown = { version = "0.15", default-features = false, features = ["default-hasher"] } @@ -146,8 +134,6 @@ tracing = { workspace = true, features = ["std"] } serde_json = { workspace = true } # For no_std testing heapless = "0.9.1" -# Host impl so `--features critical-section` tests link. -critical-section = { workspace = true, features = ["std"] } # For async testing (`sync` covers tokio::sync::mpsc in tests/session_engine.rs, # which previously leaked in from the optional [dependencies] entry) tokio = { workspace = true, features = ["macros", "rt", "time", "sync"] } diff --git a/aimdb-core/src/buffer/latest.rs b/aimdb-core/src/buffer/latest.rs deleted file mode 100644 index bac3bd40..00000000 --- a/aimdb-core/src/buffer/latest.rs +++ /dev/null @@ -1,591 +0,0 @@ -//! `LatestSlot` — gives any buffer a canonical latest value. -//! -//! A [`SpmcRing`](super::BufferCfg::SpmcRing) is a stream: runtime adapters -//! build it on a broadcast channel, which has no slot to -//! [`peek`](DynBuffer::peek). Rather than make every adapter keep one, core -//! wraps a ring's buffer in a `LatestSlot` when the database is built. The -//! wrapper remembers the last value pushed and answers `peek()` from it; -//! everything else goes straight to the adapter's buffer. -//! -//! Every write reaches the buffer through -//! [`RecordWriter::push`](super::RecordWriter), so the slot sees every value. - -use alloc::{boxed::Box, sync::Arc}; - -use super::traits::{BufferReader, DynBuffer}; - -#[cfg(feature = "observability")] -use super::BufferMetricsSnapshot; - -// ── Contract ──────────────────────────────────────────────────────────────── -// -// Code outside core runs during `push` and `peek` at exactly these points: -// -// (W) wakers: the adapter's push wakes readers synchronously, inside the -// push call (tokio after releasing its own locks, Embassy while still -// holding its lock), so a waker registered via `Reader::poll_recv` runs -// in the middle of `LatestSlot::push`; -// (C) `T::clone`: the remembered copy, and every `peek`; -// (D) `T::drop`: the replaced snapshot, and `peek`'s temporary `Arc`; -// (I) interrupts, on Embassy: they can preempt any of the above. -// -// Invariants, each covered by a test (here or in -// `test_support::assert_latest_slot_contract`, which every adapter runs -// against its real ring): -// -// 1. Ordered: after every push has returned, `peek` returns the value the -// ring received last, also with concurrent writers. -// 2. The snapshot lock is never held while code outside core runs: not -// across (W), (C) or (D). So from any of them, `peek` on the same ring -// returns without deadlock or panic. -// 3. A waker may push to the same ring (W re-entering `push`). On std the -// writer lock is re-entrant on its own thread; with `critical-section` -// the critical section nests. (Embassy's own channel panics on a -// re-entrant publish; that is the adapter's limit, not this wrapper's.) -// 4. With `critical-section`, interrupts cannot find either lock held (I). -// -// Not supported: two threads whose wakers push into each other's rings at the -// same moment (an A→B / B→A lock order across two writer locks). Invariant 1 -// needs writers serialized across the adapter's push, and tokio and Embassy -// both wake inside it, so any lock that keeps the order is held during (W). - -/// Serializes writers of one ring across the adapter's push (invariant 1), -/// re-entrantly (invariant 3). -#[cfg(feature = "std")] -struct WriterLock { - lock: std::sync::Mutex<()>, - /// The thread holding `lock`, as [`thread_token`], or 0. - owner: core::sync::atomic::AtomicUsize, -} - -/// A non-zero id unique among live threads: the address of a thread-local. -#[cfg(feature = "std")] -fn thread_token() -> usize { - std::thread_local!(static TOKEN: u8 = const { 0 }); - TOKEN.with(|t| t as *const u8 as usize) -} - -#[cfg(feature = "std")] -impl WriterLock { - const fn new() -> Self { - Self { - lock: std::sync::Mutex::new(()), - owner: core::sync::atomic::AtomicUsize::new(0), - } - } - - fn run(&self, f: impl FnOnce() -> R) -> R { - use core::sync::atomic::Ordering::Relaxed; - let me = thread_token(); - // Only this thread ever stores `me`, and it stores 0 again before - // unlocking, so reading `me` means this thread holds the lock: a - // waker re-entering push on the thread that is pushing. - if self.owner.load(Relaxed) == me { - return f(); - } - let _guard = self.lock.lock().unwrap_or_else(|p| p.into_inner()); - self.owner.store(me, Relaxed); - struct Release<'a>(&'a core::sync::atomic::AtomicUsize); - impl Drop for Release<'_> { - fn drop(&mut self) { - self.0.store(0, Relaxed); - } - } - let _release = Release(&self.owner); - f() - } -} - -/// With `critical-section`, the critical section itself serializes writers: -/// it masks interrupts and, on multi-core targets, excludes the other cores. -/// It nests, so it is re-entrant. -#[cfg(all(not(feature = "std"), feature = "critical-section"))] -struct WriterLock; - -#[cfg(all(not(feature = "std"), feature = "critical-section"))] -impl WriterLock { - const fn new() -> Self { - Self - } - fn run(&self, f: impl FnOnce() -> R) -> R { - critical_section::with(|_| f()) - } -} - -/// no_std without `critical-section`: nothing can preempt a push (a -/// cooperative executor, wasm), so writers never overlap and there is nothing -/// to serialize. Pushing from an interrupt here is unsupported; enable -/// `critical-section`. -#[cfg(all(not(feature = "std"), not(feature = "critical-section")))] -struct WriterLock; - -#[cfg(all(not(feature = "std"), not(feature = "critical-section")))] -impl WriterLock { - const fn new() -> Self { - Self - } - fn run(&self, f: impl FnOnce() -> R) -> R { - f() - } -} - -/// The remembered value. Held as an `Arc` so the lock only ever moves or -/// copies a pointer: `T::clone` and `T::drop` run after it is released -/// (invariant 2). -/// -/// - `std`: `std::sync::Mutex`. -/// - `no_std` + `critical-section`: a `critical_section::Mutex` (invariant 4). -/// - `no_std` otherwise: a spin lock. -#[cfg(feature = "std")] -struct Snapshot(std::sync::Mutex>>); - -#[cfg(feature = "std")] -impl Snapshot { - const fn new() -> Self { - Self(std::sync::Mutex::new(None)) - } - fn with(&self, f: impl FnOnce(&mut Option>) -> R) -> R { - // Only pointer moves happen under the lock, so it cannot be poisoned - // mid-update; recover anyway rather than propagate a panic. - let mut guard = self.0.lock().unwrap_or_else(|p| p.into_inner()); - f(&mut guard) - } -} - -#[cfg(all(not(feature = "std"), feature = "critical-section"))] -struct Snapshot(critical_section::Mutex>>>); - -#[cfg(all(not(feature = "std"), feature = "critical-section"))] -impl Snapshot { - const fn new() -> Self { - Self(critical_section::Mutex::new(core::cell::RefCell::new(None))) - } - fn with(&self, f: impl FnOnce(&mut Option>) -> R) -> R { - // `f` only moves pointers and never re-enters, so the borrow is free. - critical_section::with(|cs| f(&mut self.0.borrow_ref_mut(cs))) - } -} - -#[cfg(all(not(feature = "std"), not(feature = "critical-section")))] -struct Snapshot(spin::Mutex>>); - -#[cfg(all(not(feature = "std"), not(feature = "critical-section")))] -impl Snapshot { - const fn new() -> Self { - Self(spin::Mutex::new(None)) - } - fn with(&self, f: impl FnOnce(&mut Option>) -> R) -> R { - f(&mut self.0.lock()) - } -} - -/// Wraps a buffer and remembers the last value pushed into it, so `peek()` -/// returns that value for buffers that have no current-value slot of their -/// own. See the contract at the top of this module. -/// -/// Cost per push: one `T::clone` and one `Arc` allocation. -pub(crate) struct LatestSlot { - inner: Arc>, - writer: WriterLock, - latest: Snapshot, -} - -impl LatestSlot { - pub(crate) fn new(inner: Arc>) -> Self { - Self { - inner, - writer: WriterLock::new(), - latest: Snapshot::new(), - } - } -} - -impl DynBuffer for LatestSlot { - fn push(&self, value: T) { - // (C) outside every lock. - let snapshot = Arc::new(value.clone()); - let replaced = self.writer.run(|| { - // Snapshot first, then the ring, both under the writer lock: a - // waker woken by this push already sees this value, and no other - // writer can interleave (invariant 1). The snapshot lock is - // released before the adapter's push wakes anyone (invariant 2). - let replaced = self.latest.with(|latest| latest.replace(snapshot)); - self.inner.push(value); - replaced - }); - // (D) outside every lock. - drop(replaced); - } - - fn subscribe_boxed(&self) -> Box + Send> { - self.inner.subscribe_boxed() - } - - /// The adapter's buffer, so downcasts to a concrete buffer type still - /// work through the wrapper. - fn as_any(&self) -> &dyn core::any::Any { - self.inner.as_any() - } - - fn peek(&self) -> Option { - // Copy the pointer under the lock; clone and drop outside it. - let snapshot = self.latest.with(|latest| latest.clone())?; - Some(T::clone(&snapshot)) - } - - #[cfg(feature = "observability")] - fn metrics_snapshot(&self) -> Option { - self.inner.metrics_snapshot() - } - - #[cfg(feature = "observability")] - fn reset_metrics(&self) { - self.inner.reset_metrics(); - } -} - -#[cfg(test)] -mod tests { - //! Invariants 1–4 against a fake inner buffer. The same cells run against - //! every adapter's real ring in `test_support::assert_latest_slot_contract`. - use super::*; - use crate::DbError; - use alloc::sync::Weak; - use alloc::vec::Vec; - use core::sync::atomic::{AtomicBool, AtomicUsize, Ordering::SeqCst}; - use core::task::{Context, Poll}; - - type Hook = Arc) + Send + Sync>; - type WakeHook = spin::Mutex>, Hook)>>; - #[cfg(feature = "std")] - type WriteHook = spin::Mutex>>; - - /// A ring stand-in: logs pushes in order, then runs a "wake" hook - /// synchronously, inside `push`, as tokio's and Embassy's rings do. - struct Ring { - log: spin::Mutex>, - /// Yield before logging, to widen the window between the snapshot - /// update and the ring write, so a missing writer lock shows up. - #[cfg(feature = "std")] - yield_before_write: bool, - /// Runs before the write; lets a test hold one writer inside it. - #[cfg(feature = "std")] - before_write: WriteHook, - on_wake: WakeHook, - } - - impl Ring { - fn last(&self) -> Option { - self.log.lock().last().cloned() - } - } - - struct NoReader; - impl BufferReader for NoReader { - fn poll_recv(&mut self, _cx: &mut Context<'_>) -> Poll> { - Poll::Pending - } - fn try_recv(&mut self) -> Result { - Err(DbError::BufferEmpty) - } - } - - impl DynBuffer for Ring { - fn push(&self, value: T) { - #[cfg(feature = "std")] - if self.yield_before_write { - extern crate std; - std::thread::yield_now(); - } - #[cfg(feature = "std")] - { - let hook = self.before_write.lock().clone(); - if let Some(hook) = hook { - hook(&value); - } - } - self.log.lock().push(value); - let hook = self.on_wake.lock().clone(); - if let Some((slot, hook)) = hook { - if let Some(slot) = slot.upgrade() { - hook(&slot); - } - } - } - fn subscribe_boxed(&self) -> Box + Send> { - Box::new(NoReader) - } - fn as_any(&self) -> &dyn core::any::Any { - self - } - } - - fn ring() -> (Arc>, Arc>) { - let ring = Arc::new(Ring { - log: spin::Mutex::new(Vec::new()), - #[cfg(feature = "std")] - yield_before_write: false, - #[cfg(feature = "std")] - before_write: spin::Mutex::new(None), - on_wake: spin::Mutex::new(None), - }); - let slot = Arc::new(LatestSlot::new(ring.clone() as Arc>)); - (ring, slot) - } - - fn on_wake(ring: &Ring, slot: &Arc>, hook: Hook) { - *ring.on_wake.lock() = Some((Arc::downgrade(slot), hook)); - } - - /// Runs `f`, failing with "deadlock" instead of hanging if it does not - /// return within 5 s (std); runs it directly elsewhere. - fn no_deadlock(f: impl FnOnce() + Send + 'static) { - #[cfg(feature = "std")] - { - extern crate std; - let (done, rx) = std::sync::mpsc::channel(); - let t = std::thread::spawn(move || { - f(); - let _ = done.send(()); - }); - match rx.recv_timeout(std::time::Duration::from_secs(5)) { - Ok(()) => t.join().unwrap(), - Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { - // `f` panicked: re-raise its panic. - if let Err(e) = t.join() { - std::panic::resume_unwind(e); - } - } - Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { - panic!("deadlock: no return within 5 s") - } - } - } - #[cfg(not(feature = "std"))] - f(); - } - - #[test] - fn peek_is_none_before_the_first_push() { - let (_, slot) = ring::(); - assert_eq!(slot.peek(), None); - } - - #[test] - fn peek_returns_the_last_push_and_the_ring_gets_every_push() { - let (ring, slot) = ring::(); - slot.push(1); - slot.push(2); - assert_eq!(slot.peek(), Some(2)); - assert_eq!(slot.peek(), Some(2), "peek does not consume"); - assert_eq!(*ring.log.lock(), [1, 2]); - } - - #[test] - fn as_any_downcasts_to_the_inner_buffer() { - let (_, slot) = ring::(); - assert!(slot.as_any().downcast_ref::>().is_some()); - } - - /// Invariants 1 + 2 (W × peek): the waker sees the value being pushed. - #[test] - fn a_waker_can_peek_the_ring_it_was_woken_by() { - no_deadlock(|| { - let (ring, slot) = ring::(); - let seen = Arc::new(spin::Mutex::new(Vec::new())); - let log = seen.clone(); - on_wake(&ring, &slot, Arc::new(move |s| log.lock().push(s.peek()))); - slot.push(1); - slot.push(2); - assert_eq!(*seen.lock(), [Some(1), Some(2)]); - }); - } - - /// Invariant 3 (W × push same ring): ends on the ring's last value. - #[test] - fn a_waker_can_push_to_the_ring_it_was_woken_by() { - no_deadlock(|| { - let (ring, slot) = ring::(); - let once = Arc::new(AtomicBool::new(true)); - on_wake( - &ring, - &slot, - Arc::new(move |s| { - if once.swap(false, SeqCst) { - s.push(20); - } - }), - ); - slot.push(10); - assert_eq!(*ring.log.lock(), [10, 20]); - assert_eq!(slot.peek(), ring.last()); - }); - } - - /// W × push other ring: a waker of ring A pushes to ring B. - #[test] - fn a_waker_can_push_to_another_ring() { - no_deadlock(|| { - let (ring_a, slot_a) = ring::(); - let (ring_b, slot_b) = ring::(); - let target = slot_b.clone(); - on_wake(&ring_a, &slot_a, Arc::new(move |_| target.push(7))); - slot_a.push(1); - assert_eq!(slot_b.peek(), Some(7)); - assert_eq!(slot_b.peek(), ring_b.last()); - assert_eq!(slot_a.peek(), ring_a.last()); - }); - } - - /// A value whose `Clone` and `Drop` read the ring it lives in. - struct Probe { - n: u32, - slot: Arc>>>, - calls: Arc, - busy: Arc, - } - - impl Probe { - fn fire(&self) { - // Guard: the peek below clones and drops a Probe itself. - if self.busy.swap(true, SeqCst) { - return; - } - let slot = self.slot.lock().upgrade(); - if let Some(slot) = slot { - let _ = slot.peek(); - } - self.calls.fetch_add(1, SeqCst); - self.busy.store(false, SeqCst); - } - } - - impl Clone for Probe { - fn clone(&self) -> Self { - self.fire(); - Self { - n: self.n, - slot: self.slot.clone(), - calls: self.calls.clone(), - busy: self.busy.clone(), - } - } - } - - impl Drop for Probe { - fn drop(&mut self) { - self.fire(); - } - } - - /// Invariant 2 (C × peek, D × peek): `T::clone` and `T::drop` run - /// outside the snapshot lock, so they can read the same ring. - #[test] - fn clone_and_drop_can_peek_the_same_ring() { - no_deadlock(|| { - let (_ring, slot) = ring::(); - let cell = Arc::new(spin::Mutex::new(Arc::downgrade(&slot))); - let calls = Arc::new(AtomicUsize::new(0)); - let busy = Arc::new(AtomicBool::new(false)); - let probe = |n| Probe { - n, - slot: cell.clone(), - calls: calls.clone(), - busy: busy.clone(), - }; - slot.push(probe(1)); // clone for the snapshot - slot.push(probe(2)); // clone, and drop of the replaced snapshot - assert_eq!(slot.peek().map(|p| p.n), Some(2)); // clone + drop - assert!(calls.load(SeqCst) >= 4, "clone and drop each read the ring"); - }); - } - - /// Invariant 1 under real concurrency: after every push has returned, - /// `peek` is the value the ring received last. - #[cfg(feature = "std")] - #[test] - fn concurrent_writers_end_on_the_rings_last_value() { - extern crate std; - for _ in 0..20 { - let ring = Arc::new(Ring { - log: spin::Mutex::new(Vec::new()), - yield_before_write: true, - before_write: spin::Mutex::new(None), - on_wake: spin::Mutex::new(None), - }); - let slot = Arc::new(LatestSlot::new(ring.clone() as Arc>)); - let threads: Vec<_> = (0..8u32) - .map(|t| { - let slot = slot.clone(); - std::thread::spawn(move || { - for i in 0..500 { - slot.push(t * 1000 + i); - } - }) - }) - .collect(); - for t in threads { - t.join().unwrap(); - } - assert_eq!(ring.log.lock().len(), 4000); - assert_eq!(slot.peek(), ring.last()); - } - } - - /// Invariant 1, deterministically: writer A is held inside the ring write - /// until writer B's push returns (or 200 ms pass). Without a writer lock - /// B overtakes A, so the ring ends on A while the snapshot says B. With - /// it, B waits for A and the two agree. - #[cfg(feature = "std")] - #[test] - fn a_second_writer_cannot_overtake_one_inside_the_ring_write() { - extern crate std; - use std::sync::mpsc; - use std::time::Duration; - - let (ring, slot) = ring::(); - let (b_done, b_done_rx) = mpsc::channel::<()>(); - let (a_inside, a_inside_rx) = mpsc::channel::<()>(); - let b_done_rx = std::sync::Mutex::new(b_done_rx); - let a_inside = std::sync::Mutex::new(a_inside); - *ring.before_write.lock() = Some(Arc::new(move |v: &u32| { - if *v == 1 { - a_inside.lock().unwrap().send(()).unwrap(); - let _ = b_done_rx - .lock() - .unwrap() - .recv_timeout(Duration::from_millis(200)); - } - })); - - let a = { - let slot = slot.clone(); - std::thread::spawn(move || slot.push(1)) - }; - a_inside_rx.recv().unwrap(); - let b = { - let slot = slot.clone(); - std::thread::spawn(move || { - slot.push(2); - let _ = b_done.send(()); - }) - }; - a.join().unwrap(); - b.join().unwrap(); - assert_eq!(slot.peek(), ring.last()); - } - - /// Invariant 4: with `critical-section`, push and peek nest inside a - /// critical section that is already held (an interrupt handler, or an - /// adapter taking its own). - #[cfg(all(not(feature = "std"), feature = "critical-section"))] - #[test] - fn push_and_peek_nest_inside_a_critical_section() { - let (ring, slot) = ring::(); - critical_section::with(|_| { - slot.push(7); - assert_eq!(slot.peek(), Some(7)); - }); - assert_eq!(slot.peek(), ring.last()); - } -} diff --git a/aimdb-core/src/buffer/mod.rs b/aimdb-core/src/buffer/mod.rs index 31ad98e5..8c3dd87e 100644 --- a/aimdb-core/src/buffer/mod.rs +++ b/aimdb-core/src/buffer/mod.rs @@ -59,7 +59,6 @@ mod cfg; #[cfg(feature = "observability")] mod counters; -mod latest; mod reader; mod traits; mod writer; @@ -77,7 +76,6 @@ pub use reader::Reader; pub use traits::{Buffer, BufferReader, DynBuffer, TryProduceError}; // Crate-private — used by Producer to push without per-call lookup -pub(crate) use latest::LatestSlot; pub(crate) use traits::WriteHandle; pub(crate) use writer::RecordWriter; diff --git a/aimdb-core/src/buffer/test_support.rs b/aimdb-core/src/buffer/test_support.rs index ee73b9c1..d356021c 100644 --- a/aimdb-core/src/buffer/test_support.rs +++ b/aimdb-core/src/buffer/test_support.rs @@ -207,124 +207,3 @@ where "SpmcRing peek must be None (no canonical latest)" ); } - -/// Whether an adapter's ring tolerates a push from inside one of its own -/// wakes, for [`assert_latest_slot_contract`]. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum ReentrantPush { - /// The ring wakes readers after releasing its own lock (tokio), so a - /// waker may push to the same ring. - Supported, - /// The ring wakes readers while holding its own lock (Embassy, wasm), so a - /// re-entrant push fails inside the adapter. That cell is skipped. - AdapterLimit, -} - -/// Asserts the contract of the latest-value wrapper core puts around every -/// `SpmcRing` (`LatestSlot`, see `buffer/latest.rs`) against an adapter's -/// real ring, with its real, synchronous wakes. -/// -/// `mk` must produce an `SpmcRing` whose capacity holds at least 4 values. -/// -/// Cells covered (foreign code × what it does): -/// - a waker, run inside `push`, reads `peek` on the same ring: it sees the -/// value being pushed, without deadlock or panic; -/// - a waker pushes to the same ring (when `reentrant` is -/// [`ReentrantPush::Supported`]): `peek` ends on the ring's last value; -/// - the reader receives every value in ring order, and `peek` is -/// non-destructive. -pub async fn assert_latest_slot_contract(mk: F, reentrant: ReentrantPush) -where - B: Buffer> + DynBuffer> + 'static, - F: Fn() -> B, -{ - use alloc::sync::Arc; - use alloc::task::Wake; - use alloc::vec::Vec; - use core::task::{Context, Poll, Waker}; - - use super::LatestSlot; - - type Slot = LatestSlot>; - - /// On wake: read the slot, and optionally push `push_on_wake` once. - struct Probe { - slot: Arc, - seen: spin::Mutex>>>, - push_on_wake: spin::Mutex>>, - } - impl Wake for Probe { - fn wake(self: Arc) { - self.wake_by_ref(); - } - fn wake_by_ref(self: &Arc) { - let seen = self.slot.peek(); - self.seen.lock().push(seen); - let push = self.push_on_wake.lock().take(); - if let Some(v) = push { - self.slot.push(v); - } - } - } - - let new_slot = || Arc::new(LatestSlot::new(Arc::new(mk()) as Arc>)); - - // ── A waker reads the same ring during the push that woke it ────────── - let slot = new_slot(); - let probe = Arc::new(Probe { - slot: slot.clone(), - seen: spin::Mutex::new(Vec::new()), - push_on_wake: spin::Mutex::new(None), - }); - let waker = Waker::from(probe.clone()); - let mut reader = slot.subscribe_boxed(); - assert!( - reader - .poll_recv(&mut Context::from_waker(&waker)) - .is_pending(), - "an empty ring is pending" - ); - slot.push(sample(1)); - assert_eq!( - probe.seen.lock().first().cloned(), - Some(Some(sample(1))), - "the push woke the reader synchronously, and the waker's peek saw the \ - value being pushed" - ); - assert!( - matches!( - reader.poll_recv(&mut Context::from_waker(&waker)), - Poll::Ready(Ok(ref v)) if *v == sample(1) - ), - "the reader receives the value" - ); - assert_eq!(slot.peek(), Some(sample(1)), "peek is non-destructive"); - assert_eq!(slot.peek(), Some(sample(1))); - - // ── A waker pushes to the same ring ────────────────────────────────── - if reentrant == ReentrantPush::Supported { - let slot = new_slot(); - let probe = Arc::new(Probe { - slot: slot.clone(), - seen: spin::Mutex::new(Vec::new()), - push_on_wake: spin::Mutex::new(Some(sample(3))), - }); - let waker = Waker::from(probe.clone()); - let mut reader = slot.subscribe_boxed(); - assert!(reader - .poll_recv(&mut Context::from_waker(&waker)) - .is_pending()); - // 2 enters the ring, wakes the probe, which pushes 3 from inside. - slot.push(sample(2)); - let mut received = Vec::new(); - while let Poll::Ready(Ok(v)) = reader.poll_recv(&mut Context::from_waker(&waker)) { - received.push(v); - } - assert_eq!(received, [sample(2), sample(3)], "ring order"); - assert_eq!( - slot.peek(), - received.last().cloned(), - "peek ends on the value the ring received last" - ); - } -} diff --git a/aimdb-core/src/buffer/traits.rs b/aimdb-core/src/buffer/traits.rs index c111c39b..7cb3f081 100644 --- a/aimdb-core/src/buffer/traits.rs +++ b/aimdb-core/src/buffer/traits.rs @@ -88,16 +88,14 @@ pub trait DynBuffer: Send + Sync { /// /// Returns `Some(T)` if the buffer holds a current value that can be read /// without affecting any consumer's position. Returns `None` if the buffer - /// type has no current-value slot of its own (e.g., SPMC Ring) or if + /// type has no canonical "current value" concept (e.g., SPMC Ring) or if /// no value has been produced yet. /// - /// Implementations must not advance any reader position. + /// This is the buffer-native point-in-time read used by AimX `record.get` + ///. Implementations must not advance any reader position. /// /// The default returns `None`, which is the correct behaviour for buffers - /// without a current-value slot. Adapters do not need to add one: core - /// wraps every `SpmcRing` buffer in a `LatestSlot` at build time, so - /// `TypedRecord::latest()` and AimX `record.get` see the last value - /// written on every buffer type. + /// without a canonical latest value. fn peek(&self) -> Option { None } diff --git a/aimdb-core/src/builder.rs b/aimdb-core/src/builder.rs index 8c28493d..a1555eea 100644 --- a/aimdb-core/src/builder.rs +++ b/aimdb-core/src/builder.rs @@ -877,7 +877,6 @@ impl AimDbBuilder { let clock = runtime.new_clock(); for entry in &mut storages { entry.record.bind_clock(clock.clone()); - entry.record.seal_buffer(); } log_debug!( @@ -1161,9 +1160,10 @@ impl AimDb { /// Reads a record's current value without consuming it. /// - /// Returns `Ok(None)` before the first value or for records without a - /// buffer. For an [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing) this is - /// the last value written; use a subscriber to read the ring's stream. + /// Returns `Ok(None)` before the first value, for records without a buffer, + /// or for buffers without a canonical latest value (notably + /// [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing)). Use a subscriber to + /// read a ring's stream instead. /// /// Unknown keys and mismatched types return the same errors as /// [`produce`](Self::produce). This read needs neither serialization nor @@ -1309,10 +1309,11 @@ impl AimDb { /// /// # Returns /// `Some(JsonValue)` with the current value, or `None` if the record has no - /// value yet or no buffer. For an - /// [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing) the current value is the - /// last one written; read the ring's backlog via a subscriber or - /// `record.drain`. + /// value yet, no buffer, or a buffer with **no canonical latest** — i.e. + /// [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing). A ring is a stream/backlog + /// with no single "current value"; read it via a subscriber or `record.drain`, + /// not a peek (`record.get`). Use [`SingleLatest`](crate::buffer::BufferCfg::SingleLatest) + /// for state you want to read latest-value style. #[cfg(feature = "remote")] pub fn try_latest_as_json( &self, diff --git a/aimdb-core/src/outbound/latest.rs b/aimdb-core/src/outbound/latest.rs index 5e4340cf..708483cf 100644 --- a/aimdb-core/src/outbound/latest.rs +++ b/aimdb-core/src/outbound/latest.rs @@ -26,8 +26,7 @@ pub(crate) struct Rendered { /// Renders a link's current value. The topic is resolved first and passed to /// `accept` (`None`: the link's default topic); the value is serialized only /// if `accept` returns `true`. `None` if the record has no current value (none -/// written yet), is an `SpmcRing` (a stream of events, so no late-join -/// snapshot), `accept` rejects the topic, or the +/// written yet, or an `SpmcRing` buffer), `accept` rejects the topic, or the /// topic does not fit or serializing fails. pub(crate) type LatestFn = Arc< dyn Fn(&AimDb, &RuntimeContext, &dyn Fn(Option<&str>) -> bool) -> Option diff --git a/aimdb-core/src/session/aimx/codec.rs b/aimdb-core/src/session/aimx/codec.rs index 5bcd3d47..52496400 100644 --- a/aimdb-core/src/session/aimx/codec.rs +++ b/aimdb-core/src/session/aimx/codec.rs @@ -147,6 +147,7 @@ fn payload_of(raw: Option<&RawValue>) -> Payload { fn err_code(e: &RpcError) -> &'static str { match e { RpcError::NotFound => "not_found", + RpcError::Unsupported => "unsupported", RpcError::Denied => "denied", RpcError::Internal => "internal", RpcError::VersionMismatch => "version_mismatch", @@ -158,6 +159,7 @@ fn err_code(e: &RpcError) -> &'static str { fn code_err(s: &str) -> RpcError { match s { "not_found" => RpcError::NotFound, + "unsupported" => RpcError::Unsupported, "denied" => RpcError::Denied, "version_mismatch" => RpcError::VersionMismatch, _ => RpcError::Internal, @@ -509,6 +511,7 @@ mod tests { fn reply_err_codes_roundtrip() { for err in [ RpcError::NotFound, + RpcError::Unsupported, RpcError::Denied, RpcError::Internal, RpcError::VersionMismatch, diff --git a/aimdb-core/src/session/aimx/dispatch.rs b/aimdb-core/src/session/aimx/dispatch.rs index 15190163..414a0e9c 100644 --- a/aimdb-core/src/session/aimx/dispatch.rs +++ b/aimdb-core/src/session/aimx/dispatch.rs @@ -226,19 +226,7 @@ impl AimxSession { impl futures_util::Stream + Send + 'static, > { let stream = crate::remote::stream::stream_record_updates(&self.db, key)?; - // A ring is a stream of events, not a state: it gets no snapshot. - let is_ring = self - .db - .inner() - .resolve_str(key) - .and_then(|id| self.db.inner().storage(id)) - .is_some_and(|record| record.is_ring()); - let latest = if is_ring { - None - } else { - self.db.try_latest_as_json_bytes(key) - }; - let snapshot = latest.map(|sample| { + let snapshot = self.db.try_latest_as_json_bytes(key).map(|sample| { let ts = sample.stamp(); self.pending_snapshots .push((key.to_string(), Payload::from(sample.value), ts)); @@ -331,19 +319,37 @@ impl AimxSession { /// `record.get`: the record's current value and its write stamp, /// `{"value": , "ts": }`. /// - /// A non-destructive read of the record's latest value - /// ([`try_latest_as_json_bytes`](crate::AimDb::try_latest_as_json_bytes)). - /// Every buffer type has one: `SingleLatest` and `Mailbox` natively, and - /// `SpmcRing` through the `LatestSlot` core - /// installs at build time. It never touches this connection's drain - /// cursor. `NotFound` means the record does not exist or holds no value - /// yet. + /// A single, non-destructive read of the buffer's own snapshot + /// ([`try_latest_as_json_bytes`](crate::AimDb::try_latest_as_json_bytes)): + /// it returns at once and never touches this connection's drain cursor. + /// + /// - `NotFound`: no such record, or a record with a snapshot (`SingleLatest`, + /// `Mailbox`) that holds no value yet. + /// - `Unsupported`: an [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing). A + /// ring is a stream with no snapshot; read it with `record.drain` + /// (unread events) or `record.subscribe` (future events). An application + /// that needs an inspectable current state exposes a `SingleLatest` + /// record next to the stream. + /// + /// The ring check reads the declared [`BufferCfg`](crate::buffer::BufferCfg): + /// a ring registered with `buffer_raw()` and no `buffer_cfg()` reports + /// `NotFound`, because core cannot tell it is a ring. fn record_get(&self, name: &str) -> Result { - let sample = self - .db - .try_latest_as_json_bytes(name) + if let Some(sample) = self.db.try_latest_as_json_bytes(name) { + return Ok(super::value_reply(&sample.value, sample.stamp())); + } + let inner = self.db.inner(); + let record = inner + .resolve_str(name) + .and_then(|id| inner.storage(id)) .ok_or(RpcError::NotFound)?; - Ok(super::value_reply(&sample.value, sample.stamp())) + // Only the miss path pays for this lookup. + let ring = crate::buffer::BufferCfg::SpmcRing { capacity: 0 }.name(); + if record.buffer_info().0 == ring { + Err(RpcError::Unsupported) + } else { + Err(RpcError::NotFound) + } } /// `record.drain`: return everything accumulated since the previous drain diff --git a/aimdb-core/src/session/mod.rs b/aimdb-core/src/session/mod.rs index e713bf38..3f6b0695 100644 --- a/aimdb-core/src/session/mod.rs +++ b/aimdb-core/src/session/mod.rs @@ -270,6 +270,9 @@ pub enum CodecError { pub enum RpcError { /// No such method or topic. NotFound, + /// The method exists but the target cannot serve it, e.g. `record.get` on + /// a ring buffer, which has no current value to read. + Unsupported, /// The caller lacks permission for this operation. Denied, /// The handler failed. diff --git a/aimdb-core/src/typed_api.rs b/aimdb-core/src/typed_api.rs index 55dcf93b..50a6789c 100644 --- a/aimdb-core/src/typed_api.rs +++ b/aimdb-core/src/typed_api.rs @@ -616,18 +616,8 @@ where /// - Runtime adapter implementations to provide convenient wrappers /// - Advanced use cases requiring custom buffer implementations /// - /// **Declare the buffer's kind** with [`buffer_cfg`](Self::buffer_cfg) after - /// this call (it resets any earlier declaration), or use - /// [`buffer_with_cfg`](Self::buffer_with_cfg). Core cannot see what kind of - /// buffer a raw one is, and relies on the declaration: - /// - /// - A buffer declared as [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing) - /// gets a remembered last value at build time, so `latest()` and AimX - /// `record.get` return the last write. Without the declaration a ring with - /// the default `peek()` has no readable latest value. - /// - A record with `.with_remote_access()` and an undeclared raw buffer - /// fails `build()`, rather than answering `record.get` with `not_found` - /// forever. + /// **Note:** For metadata tracking in std mode, call `buffer_with_cfg()` instead, + /// or call `buffer_cfg()` separately to set the configuration. pub fn buffer_raw( &mut self, buffer: Box>>, @@ -647,10 +637,7 @@ where self } - /// Declares the kind of the buffer set with [`buffer_raw`](Self::buffer_raw). - /// - /// Used for metadata and to decide whether core keeps a ring's last value - /// for `latest()` / `record.get`. Call it after `buffer_raw()`. + /// Sets the buffer configuration for metadata tracking pub fn buffer_cfg(&mut self, cfg: crate::buffer::BufferCfg) -> &mut Self { self.rec.set_buffer_cfg(cfg); self @@ -1616,12 +1603,12 @@ where move |db: &AimDb, ctx: &crate::RuntimeContext, accept: &dyn Fn(Option<&str>) -> bool| { - let record = db.inner().get_typed_record_by_key::(&record_key).ok()?; - // A ring is a stream of events: no late-join snapshot. - if crate::typed_record::AnyRecord::is_ring(record) { - return None; - } - let sample = record.latest()?.into_stamped(); + let sample = db + .inner() + .get_typed_record_by_key::(&record_key) + .ok()? + .latest()? + .into_stamped(); let topic = match &topic_writer { None => None, diff --git a/aimdb-core/src/typed_record.rs b/aimdb-core/src/typed_record.rs index 39dd2755..b098f086 100644 --- a/aimdb-core/src/typed_record.rs +++ b/aimdb-core/src/typed_record.rs @@ -284,23 +284,6 @@ pub trait AnyRecord: Send + Sync { /// runs (design 058 §3.3). fn bind_clock(&mut self, clock: Arc); - /// Gives the record's buffer a canonical latest value if it has none of - /// its own. - /// - /// Called by `AimDbBuilder::build()` for every record before any task - /// runs. A [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing) buffer is - /// wrapped in `LatestSlot`, so `latest()` - /// and AimX `record.get` work on rings the same way they work on - /// `SingleLatest` and `Mailbox` buffers, on every runtime adapter. - fn seal_buffer(&mut self); - - /// Whether the record's buffer is an - /// [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing): a stream of events - /// rather than a state. A ring's latest value is readable on request - /// (`latest()`, `record.get`), but late-join snapshots skip it, so a new - /// subscriber does not receive an old event as if it were current state. - fn is_ring(&self) -> bool; - /// Returns the record's JSON remote-access surface, if it has one. /// /// This accessor is the single place the `remote` cfg-gate lives @@ -1246,10 +1229,10 @@ impl TypedRecord { /// Returns the latest produced value /// /// Returns the most recent value wrapped in `RecordValue`, or `None` if no - /// value has been produced yet or the record has no buffer. Works for every - /// buffer type: an [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing) gets its - /// latest value from the `LatestSlot` installed - /// at build time. Non-blocking. + /// value has been produced yet, the record has no buffer, or the buffer has + /// **no canonical latest** — i.e. [`SpmcRing`](crate::buffer::BufferCfg::SpmcRing) + /// (a ring is a stream/backlog; read it via a subscriber/drain instead). + /// Non-blocking. /// /// **Both std and no_std**: Direct access via `Deref`, `.get()`, `.into_inner()` /// @@ -1288,15 +1271,6 @@ impl AnyRecord for TypedRecord { self.clock = Some(clock); } - fn seal_buffer(&mut self) { - if !self.is_ring() { - return; - } - if let Some(inner) = self.buffer.take() { - self.buffer = Some(Arc::new(crate::buffer::LatestSlot::new(inner))); - } - } - fn as_any_mut(&mut self) -> &mut dyn Any { self } @@ -1320,23 +1294,6 @@ impl AnyRecord for TypedRecord { )); } - // A buffer from `buffer_raw()` hides its kind. Core gives a ring its - // readable latest value (`LatestSlot`) only when it knows the buffer is - // a ring, and `record.get` no longer falls back to draining, so an - // undeclared raw ring would answer `not_found` forever. Require the - // kind to be declared instead of guessing. - #[cfg(feature = "remote")] - if self.remote_codec.is_some() && self.has_buffer() && self.buffer_cfg.is_none() { - errors.push(crate::error::ConfigError::new( - "", - None, - "record has .with_remote_access() and a buffer from .buffer_raw() \ - with no declared kind; declare it after .buffer_raw() with \ - .buffer_cfg(BufferCfg::SpmcRing { .. }) for a ring, or the matching \ - BufferCfg otherwise, so record.get reads the right value", - )); - } - errors } @@ -1377,13 +1334,6 @@ impl AnyRecord for TypedRecord { TypedRecord::has_producer(self) } - fn is_ring(&self) -> bool { - matches!( - self.buffer_cfg, - Some(crate::buffer::BufferCfg::SpmcRing { .. }) - ) - } - fn has_buffer(&self) -> bool { TypedRecord::has_buffer(self) } diff --git a/aimdb-embassy-adapter/CHANGELOG.md b/aimdb-embassy-adapter/CHANGELOG.md index e07c3593..9c772d54 100644 --- a/aimdb-embassy-adapter/CHANGELOG.md +++ b/aimdb-embassy-adapter/CHANGELOG.md @@ -7,13 +7,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Changed - -- **Rings stay interrupt-safe (#450).** The adapter enables `aimdb-core`'s - `critical-section` feature, so the last value core keeps for each ring (for - `latest()` / `record.get`) is guarded by a critical section, like the ring - itself. Pushing to a ring from an interrupt handler remains safe. - ### Removed (breaking) - **The `std` feature (#419).** It existed only so `build.rs` could reject it: diff --git a/aimdb-embassy-adapter/Cargo.toml b/aimdb-embassy-adapter/Cargo.toml index ca3453fc..35f7c77a 100644 --- a/aimdb-embassy-adapter/Cargo.toml +++ b/aimdb-embassy-adapter/Cargo.toml @@ -55,9 +55,6 @@ observability = ["aimdb-core/observability", "embassy-runtime"] # Core AimDB types - no_std for Embassy (requires alloc for typed APIs) aimdb-core = { version = "3.0.0", path = "../aimdb-core", default-features = false, features = [ "alloc", - # Interrupt-safe lock for a ring's remembered last value (`LatestSlot`): - # rings may be pushed from interrupt handlers, as Embassy's own buffers allow. - "critical-section", ] } # Generic framed `Connection` over async UART halves (feature `connector-io`). diff --git a/aimdb-embassy-adapter/src/buffer.rs b/aimdb-embassy-adapter/src/buffer.rs index 16c1fe45..a502e689 100644 --- a/aimdb-embassy-adapter/src/buffer.rs +++ b/aimdb-embassy-adapter/src/buffer.rs @@ -872,12 +872,6 @@ mod tests { block_on(test_support::assert_single_latest_contract(B::new_watch)); block_on(test_support::assert_mailbox_contract(B::new_mailbox)); block_on(test_support::assert_spmc_ring_contract(B::new_spmc)); - // PubSub wakes subscribers inside its own lock, so a push from a waker - // fails in embassy-sync itself; that cell is the adapter's limit. - block_on(test_support::assert_latest_slot_contract( - B::new_spmc, - test_support::ReentrantPush::AdapterLimit, - )); } /// Design 058 §4.5: the write stamp costs every slot. The buffer keeps diff --git a/aimdb-tokio-adapter/src/buffer.rs b/aimdb-tokio-adapter/src/buffer.rs index 6a968c09..343028a0 100644 --- a/aimdb-tokio-adapter/src/buffer.rs +++ b/aimdb-tokio-adapter/src/buffer.rs @@ -1020,12 +1020,6 @@ mod tests { TokioBuffer::>::new(&BufferCfg::SpmcRing { capacity: 4 }) }) .await; - // broadcast wakes after releasing its own lock, so a waker may push. - test_support::assert_latest_slot_contract( - || TokioBuffer::>::new(&BufferCfg::SpmcRing { capacity: 4 }), - test_support::ReentrantPush::Supported, - ) - .await; } #[tokio::test] diff --git a/aimdb-tokio-adapter/tests/remote_access_validation.rs b/aimdb-tokio-adapter/tests/remote_access_validation.rs index 501d852d..454c956f 100644 --- a/aimdb-tokio-adapter/tests/remote_access_validation.rs +++ b/aimdb-tokio-adapter/tests/remote_access_validation.rs @@ -46,80 +46,3 @@ async fn remote_access_record_with_buffer_builds() { "remote-access record with a buffer must build" ); } - -// ── Raw buffers (`buffer_raw`) ─────────────────────────────────────────────── -// -// A raw buffer hides its kind. Core keeps a ring's last value only for a buffer -// declared as `SpmcRing`, and `record.get` no longer drains a cursor to find -// one, so the kind must be declared (#444, #450). - -fn raw_ring() -> Box>> { - use aimdb_core::buffer::Buffer; - Box::new(aimdb_tokio_adapter::TokioBuffer::< - aimdb_core::Stamped, - >::new(&BufferCfg::SpmcRing { capacity: 8 })) -} - -#[tokio::test] -async fn undeclared_raw_buffer_with_remote_access_fails_build() { - let mut builder = AimDbBuilder::new().runtime(Arc::new(TokioAdapter)); - - builder.configure::("test::Config", |reg| { - reg.buffer_raw(raw_ring()).with_remote_access(); - }); - - let err = match builder.build().await { - Ok(_) => panic!("build() must reject an undeclared raw buffer on a remote-access record"), - Err(e) => e.to_string(), - }; - assert!(err.contains("buffer_cfg"), "the error names the fix: {err}"); -} - -#[tokio::test] -async fn declared_raw_ring_returns_its_last_write() { - let mut builder = AimDbBuilder::new().runtime(Arc::new(TokioAdapter)); - - builder.configure::("test::Config", |reg| { - reg.buffer_raw(raw_ring()) - .buffer_cfg(BufferCfg::SpmcRing { capacity: 8 }) - .with_remote_access(); - }); - let (db, _runner) = builder.build().await.expect("a declared raw ring builds"); - - // Nothing written yet. - assert_eq!(db.try_latest_as_json("test::Config"), None); - - let producer = db.producer::("test::Config").expect("producer"); - producer.produce(Config { threshold: 1 }); - producer.produce(Config { threshold: 2 }); - - // What `record.get` serves: the last write, readable again and again. - for _ in 0..2 { - assert_eq!( - db.try_latest_as_json("test::Config").map(|s| s.value), - Some(serde_json::json!({ "threshold": 2 })) - ); - } - assert_eq!( - db.latest::("test::Config") - .unwrap() - .map(|s| s.value), - Some(Config { threshold: 2 }) - ); -} - -#[tokio::test] -async fn undeclared_raw_buffer_without_remote_access_still_builds() { - // Only remote-access records need the declaration: `latest()` on an - // undeclared raw ring returns `None`, as it did before. - let mut builder = AimDbBuilder::new().runtime(Arc::new(TokioAdapter)); - - builder.configure::("test::Config", |reg| { - reg.buffer_raw(raw_ring()); - }); - let (db, _runner) = builder.build().await.expect("builds"); - db.producer::("test::Config") - .unwrap() - .produce(Config { threshold: 1 }); - assert_eq!(db.latest::("test::Config").unwrap(), None); -} diff --git a/aimdb-wasm-adapter/src/buffer.rs b/aimdb-wasm-adapter/src/buffer.rs index 9b8022c7..9e1b8997 100644 --- a/aimdb-wasm-adapter/src/buffer.rs +++ b/aimdb-wasm-adapter/src/buffer.rs @@ -558,12 +558,6 @@ mod tests { block_on(test_support::assert_spmc_ring_contract(|| { WasmBuffer::>::new(&BufferCfg::SpmcRing { capacity: 4 }) })); - // Wakes run inside the buffer's `RefCell` borrow, so a push from a waker - // fails in the adapter itself; that cell is the adapter's limit. - block_on(test_support::assert_latest_slot_contract( - || WasmBuffer::>::new(&BufferCfg::SpmcRing { capacity: 4 }), - test_support::ReentrantPush::AdapterLimit, - )); } // ======================================================================== diff --git a/aimdb-wasm-adapter/src/ws_bridge.rs b/aimdb-wasm-adapter/src/ws_bridge.rs index b39b4cbd..2cfb7120 100644 --- a/aimdb-wasm-adapter/src/ws_bridge.rs +++ b/aimdb-wasm-adapter/src/ws_bridge.rs @@ -840,6 +840,7 @@ impl WsBridge { fn rpc_err_str(e: &RpcError) -> &'static str { match e { RpcError::NotFound => "not_found", + RpcError::Unsupported => "unsupported (a ring has no current value)", RpcError::Denied => "denied", RpcError::VersionMismatch => { "version_mismatch (server rejected the client protocol version)" diff --git a/aimdb-websocket-connector/tests/http_current_values.rs b/aimdb-websocket-connector/tests/http_current_values.rs index 99ae0a7a..5543854e 100644 --- a/aimdb-websocket-connector/tests/http_current_values.rs +++ b/aimdb-websocket-connector/tests/http_current_values.rs @@ -65,20 +65,8 @@ async fn typed_latest_preserves_lookup_and_buffer_semantics() { db.latest::("second").unwrap().map(|s| s.value), Some(Reading(3)) ); - // A ring has no current-value slot of its own; core remembers its last - // value, so `latest` reads it without consuming anything. - assert_eq!(db.latest::("ring").unwrap().map(|s| s.value), None); - let mut ring = db.subscribe::("ring").unwrap(); db.produce("ring", Reading(4)).unwrap(); - db.produce("ring", Reading(6)).unwrap(); - for _ in 0..2 { - assert_eq!( - db.latest::("ring").unwrap().map(|s| s.value), - Some(Reading(6)) - ); - } - assert_eq!(ring.try_recv().unwrap().value, Reading(4)); - assert_eq!(ring.try_recv().unwrap().value, Reading(6)); + assert_eq!(db.latest::("ring").unwrap().map(|s| s.value), None); let mut mailbox = db.subscribe::("mailbox").unwrap(); db.produce("mailbox", Reading(5)).unwrap(); assert_eq!( diff --git a/examples/remote-access-demo/src/client.rs b/examples/remote-access-demo/src/client.rs index 7e1ca81b..b91e70c8 100644 --- a/examples/remote-access-demo/src/client.rs +++ b/examples/remote-access-demo/src/client.rs @@ -55,16 +55,13 @@ async fn main() -> Result<(), Box> { } println!(); - // A ring (SpmcRing) answers record.get the same way: with the last value - // written, without touching this connection's drain cursor. - println!("📤 record.get on Temperature (SpmcRing — last value written)..."); + // A ring (SpmcRing) is a stream with no current value, so record.get + // reports `unsupported`. Its events are read with record.drain (below) or + // record.subscribe. + println!("📤 record.get on Temperature (SpmcRing — no current value)..."); match conn.get_record("server::Temperature").await { - Ok(v) => println!( - "🌡️ Most recent Temperature (written {}):\n{}", - since_start(v.stamp()), - serde_json::to_string_pretty(&v.value)? - ), - Err(e) => println!("ℹ️ No reading yet: {e}"), + Ok(v) => println!("❌ UNEXPECTED value: {}", v.value), + Err(e) => println!("✅ Expected: {e}"), } println!(); diff --git a/tools/aimdb-cli/README.md b/tools/aimdb-cli/README.md index 32616b07..f237e73a 100644 --- a/tools/aimdb-cli/README.md +++ b/tools/aimdb-cli/README.md @@ -156,7 +156,9 @@ Options: - `-w, --writable`: Show only writable records #### `record get` -Get the current value of a specific record. +Get the current value of a specific record. A `spmc_ring` record is a stream +with no current value, so `record get` reports `unsupported` for it; use +`watch` to follow its events. ```bash aimdb record get [OPTIONS] diff --git a/tools/aimdb-mcp/CHANGELOG.md b/tools/aimdb-mcp/CHANGELOG.md index 1ba09b96..cbe7974e 100644 --- a/tools/aimdb-mcp/CHANGELOG.md +++ b/tools/aimdb-mcp/CHANGELOG.md @@ -9,9 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- **`get_record` answers at once (#444).** It no longer retries `not_found` - for up to 3 s, so an empty record or a misspelled name returns immediately - instead of after 3 s. A ring returns its last value on the first call. +- **`get_record` answers at once (#444).** It no longer retries `not_found` for + up to 3 s, so an empty record or a misspelled name returns immediately + instead of after 3 s. A ring returns `unsupported` (use `drain_record`), and + `get_record` no longer shares `drain_record`'s cursor. ### Removed (breaking) diff --git a/tools/aimdb-mcp/Cargo.toml b/tools/aimdb-mcp/Cargo.toml index 3c93fd85..e6b39e5f 100644 --- a/tools/aimdb-mcp/Cargo.toml +++ b/tools/aimdb-mcp/Cargo.toml @@ -69,3 +69,7 @@ chrono = "0.4" # `__event_capture!` for the test-only `events::capture` module. aimdb-core = { version = "3.0.0", path = "../../aimdb-core", default-features = false } tokio-test = "0.4" +# tests/get_record.rs: a real AimX server over a Unix socket. +aimdb-tokio-adapter = { version = "0.8.0", path = "../../aimdb-tokio-adapter" } +aimdb-uds-connector = { version = "0.2.0", path = "../../aimdb-uds-connector" } +tempfile = "3" diff --git a/tools/aimdb-mcp/src/server.rs b/tools/aimdb-mcp/src/server.rs index a256d819..d552bad4 100644 --- a/tools/aimdb-mcp/src/server.rs +++ b/tools/aimdb-mcp/src/server.rs @@ -211,9 +211,9 @@ impl McpServer { description: "Get the current value of a specific record from an AimDB instance. \ Returns {\"value\": , \"ts\": }, where ts is \ nanoseconds since the AimDB instance started (not wall-clock time; only comparable \ - within one instance lifetime). For a ring buffer this is the last value written; \ - use drain_record for its history. Returns a not_found error at once when the \ - record does not exist or holds no value yet.".to_string(), + within one instance lifetime). Answers at once: not_found when the record does \ + not exist or holds no value yet, unsupported for a ring buffer (spmc_ring), which \ + has no current value; use drain_record for its unread events.".to_string(), input_schema: json!({ "type": "object", "properties": { diff --git a/tools/aimdb-mcp/src/tools/record.rs b/tools/aimdb-mcp/src/tools/record.rs index 17e4b866..6692cb1e 100644 --- a/tools/aimdb-mcp/src/tools/record.rs +++ b/tools/aimdb-mcp/src/tools/record.rs @@ -161,9 +161,9 @@ pub async fn get_record(args: Option) -> McpResult { endpoint, params.record_name ); - // `record.get` is a single non-destructive read for every buffer type (rings - // included), so it needs no retry and no drain cursor: `not_found` means the - // record does not exist or holds no value yet, and is returned at once. + // `record.get` is a single non-destructive read with no drain cursor, so it + // needs no retry: `not_found` (no such record, or no value yet) and + // `unsupported` (a ring, which has no current value) come back at once. let client = if let Some(pool) = super::connection_pool() { pool.get_connection(&endpoint) .await diff --git a/tools/aimdb-mcp/tests/get_record.rs b/tools/aimdb-mcp/tests/get_record.rs new file mode 100644 index 00000000..35252b1f --- /dev/null +++ b/tools/aimdb-mcp/tests/get_record.rs @@ -0,0 +1,106 @@ +//! `get_record` answers at once and leaves `drain_record` intact (#444). +//! +//! It used to retry `not_found` for up to 3 s, so an empty record or a bad +//! name cost the whole window, and a ring was read through the drain cursor. + +use std::sync::Arc; +use std::time::{Duration, Instant}; + +use aimdb_core::buffer::BufferCfg; +use aimdb_core::remote::AimxConfig; +use aimdb_core::AimDbBuilder; +use aimdb_mcp::tools; +use aimdb_tokio_adapter::{TokioAdapter, TokioRecordRegistrarExt}; +use aimdb_uds_connector::UdsServer; +use serde::{Deserialize, Serialize}; +use serde_json::{json, Value}; + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +struct Reading { + n: u64, +} + +fn args(endpoint: &str, record: &str) -> Option { + Some(json!({ "endpoint": endpoint, "record_name": record })) +} + +#[tokio::test] +async fn get_record_answers_at_once_and_leaves_drain_intact() { + let dir = tempfile::tempdir().unwrap(); + let sock = dir.path().join("aimdb.sock"); + let endpoint = sock.to_str().unwrap().to_string(); + + let config = AimxConfig::uds_default().socket_path(&endpoint); + let mut builder = AimDbBuilder::new() + .runtime(Arc::new(TokioAdapter)) + .with_connector(UdsServer::from_config(config)); + builder.configure::("stream", |reg| { + reg.buffer(BufferCfg::SpmcRing { capacity: 16 }) + .with_remote_access(); + }); + builder.configure::("state", |reg| { + reg.buffer(BufferCfg::SingleLatest).with_remote_access(); + }); + let (db, runner) = builder.build().await.expect("build db"); + tokio::spawn(runner.run()); + tools::init_connection_pool(aimdb_mcp::connection::ConnectionPool::new()); + + // Each miss used to take ~3 s. Allow 1 s for all three together. + let started = Instant::now(); + for (record, code) in [ + ("state", "not_found"), // single_latest, no value yet (the issue) + ("missing", "not_found"), // unknown name + ("stream", "unsupported"), // a ring has no current value + ] { + let err = tools::get_record(args(&endpoint, record)) + .await + .expect_err(record) + .to_string(); + assert!(err.contains(code), "{record}: {err}"); + } + assert!( + started.elapsed() < Duration::from_secs(1), + "three misses took {:?}", + started.elapsed() + ); + + // Open the ring's drain cursor, write, and read the ring with get in + // between: drain still returns every event. + let drain = || { + tools::drain_record(Some( + json!({ "endpoint": endpoint, "record_name": "stream" }), + )) + }; + assert_eq!(drain().await.expect("drain")["count"], 0); + let producer = db.producer::("stream").expect("producer"); + for n in 1..=3 { + producer.produce(Reading { n }); + } + assert!(tools::get_record(args(&endpoint, "stream")).await.is_err()); + + let mut ns = Vec::new(); + for _ in 0..50 { + let page = drain().await.expect("drain"); + ns.extend( + page["values"] + .as_array() + .unwrap() + .iter() + .map(|v| v["value"]["n"].clone()), + ); + if ns.len() >= 3 { + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + assert_eq!(ns, [1, 2, 3]); + + // A state record with a value answers with it, at once. + db.producer::("state") + .unwrap() + .produce(Reading { n: 7 }); + let got = tools::get_record(args(&endpoint, "state")) + .await + .expect("get"); + assert_eq!(got["value"], json!({ "n": 7 })); +} From 490adabb53fc83b4bab7b487c5eb9fc82c159b65 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 9 Nov 2026 19:34:21 +0000 Subject: [PATCH 5/5] fix(mcp): get_record shares the persistent session with drain_record (#450) get_record opened a second AimX connection. Over serial that either fails (the tty is already open) or sends a new hello, which resets the board's session and discards the drain cursor drain_record reads. - get_record now uses get_drain_client(), takes the per-client lock and issues exactly one record.get. Sharing is safe: the server keeps the drain cursor apart from record.get. - Error replies (not_found, unsupported, even internal) keep the session; drain_record no longer drops its client on any error. - get_drain_client() replaces a cached client only once its engine has stopped, via the new AimxConnection::is_closed(), since internal can be an ordinary server reply. Tests: - serial_session: a SerialServer board behind a pty, with a relay that counts hellos. Open the ring's drain cursor, publish events, get_record a SingleLatest (plus not_found and unsupported misses, and a drain error), drain again: the read succeeds, the drain returns the queued events, and the board saw exactly one hello. Fails with a second connection (tty busy) and with drop-on-error (cursor lost). Runs in `make test`. - persistent_client: the client survives error replies and is replaced after its server goes away. Fails if engine state is ignored. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WGkDJ6rHvDMa63myz3jwfZ --- Cargo.lock | 2 + Makefile | 2 + aimdb-client/CHANGELOG.md | 6 + aimdb-client/src/engine.rs | 10 + tools/aimdb-mcp/CHANGELOG.md | 6 + tools/aimdb-mcp/Cargo.toml | 5 + tools/aimdb-mcp/src/connection.rs | 36 +++- tools/aimdb-mcp/src/tools/record.rs | 48 ++--- tools/aimdb-mcp/tests/persistent_client.rs | 110 ++++++++++ tools/aimdb-mcp/tests/serial_session.rs | 225 +++++++++++++++++++++ 10 files changed, 413 insertions(+), 37 deletions(-) create mode 100644 tools/aimdb-mcp/tests/persistent_client.rs create mode 100644 tools/aimdb-mcp/tests/serial_session.rs diff --git a/Cargo.lock b/Cargo.lock index 18ac70c1..0ca5788a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -248,11 +248,13 @@ version = "0.10.0" dependencies = [ "aimdb-client", "aimdb-core", + "aimdb-serial-connector", "aimdb-tokio-adapter", "aimdb-uds-connector", "anyhow", "chrono", "clap", + "libc", "once_cell", "serde", "serde_json", diff --git a/Makefile b/Makefile index 5de300a7..7f42589c 100644 --- a/Makefile +++ b/Makefile @@ -243,6 +243,8 @@ test: test-operational-events test-build-info-link cargo test --package aimdb-cli --features yaml @printf "$(YELLOW) → Testing MCP server$(NC)\n" cargo test --package aimdb-mcp + @printf "$(YELLOW) → Testing MCP server (serial: one session for get and drain)$(NC)\n" + cargo test --package aimdb-mcp --features "transport-serial" --test serial_session @printf "$(YELLOW) → Testing persistence backend$(NC)\n" cargo test --package aimdb-persistence @printf "$(YELLOW) → Testing persistence SQLite backend$(NC)\n" diff --git a/aimdb-client/CHANGELOG.md b/aimdb-client/CHANGELOG.md index c79cba66..d9725321 100644 --- a/aimdb-client/CHANGELOG.md +++ b/aimdb-client/CHANGELOG.md @@ -14,6 +14,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the connection's drain cursor, so `drain_record` returns every unread event. `watch::arm` treats `unsupported` like `not_found`: no baseline. +### Added + +- **`AimxConnection::is_closed()` (#450):** whether the connection's engine has + stopped. A caller keeping a connection open uses it to decide when to + reconnect; an `internal` error reply does not mean the connection is gone. + ### Removed (breaking) - **Obsolete AimX message types (#419).** `protocol::{Request, Response, diff --git a/aimdb-client/src/engine.rs b/aimdb-client/src/engine.rs index e12f0534..54b18389 100644 --- a/aimdb-client/src/engine.rs +++ b/aimdb-client/src/engine.rs @@ -231,6 +231,16 @@ impl AimxConnection { &self.server_info } + /// Whether this connection's engine has stopped: the transport closed or + /// failed. The engine does not reconnect, so every later call fails; a + /// caller that keeps a connection open replaces it. + /// + /// This is the reliable signal: an `internal` error can also be an + /// ordinary server reply on a healthy connection. + pub fn is_closed(&self) -> bool { + self.engine.is_finished() + } + /// List all registered records. pub async fn list_records(&self) -> ClientResult> { let reply = self.call("record.list", null_payload()).await?; diff --git a/tools/aimdb-mcp/CHANGELOG.md b/tools/aimdb-mcp/CHANGELOG.md index cbe7974e..7cb2e3a2 100644 --- a/tools/aimdb-mcp/CHANGELOG.md +++ b/tools/aimdb-mcp/CHANGELOG.md @@ -13,6 +13,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 up to 3 s, so an empty record or a misspelled name returns immediately instead of after 3 s. A ring returns `unsupported` (use `drain_record`), and `get_record` no longer shares `drain_record`'s cursor. +- **`get_record` and `drain_record` share one session per endpoint (#450).** + `get_record` opened a second connection, which over serial failed (the tty + was already open) or sent a new `hello` that reset the board's session and + discarded its drain cursor. Both now use the persistent client, one call at a + time. An error reply (`not_found`, `unsupported`, even `internal`) keeps the + session; the client is replaced only once its engine has stopped. ### Removed (breaking) diff --git a/tools/aimdb-mcp/Cargo.toml b/tools/aimdb-mcp/Cargo.toml index e6b39e5f..821b532f 100644 --- a/tools/aimdb-mcp/Cargo.toml +++ b/tools/aimdb-mcp/Cargo.toml @@ -73,3 +73,8 @@ tokio-test = "0.4" aimdb-tokio-adapter = { version = "0.8.0", path = "../../aimdb-tokio-adapter" } aimdb-uds-connector = { version = "0.2.0", path = "../../aimdb-uds-connector" } tempfile = "3" +# tests/serial_session.rs: a board (`SerialServer`) behind a pty. +aimdb-serial-connector = { version = "0.2.0", path = "../../aimdb-serial-connector", features = [ + "std", +] } +libc = "0.2" diff --git a/tools/aimdb-mcp/src/connection.rs b/tools/aimdb-mcp/src/connection.rs index 06bb6ca7..fe69a065 100644 --- a/tools/aimdb-mcp/src/connection.rs +++ b/tools/aimdb-mcp/src/connection.rs @@ -81,21 +81,37 @@ impl ConnectionPool { } } - /// Get or create a persistent drain client for a socket path. + /// Get or create the persistent client for an endpoint. /// - /// Drain clients are kept alive across calls so the server-side drain - /// reader accumulates values between invocations. The first drain call - /// on a new connection is a cold start (returns empty); subsequent calls - /// return all values accumulated since the previous drain. + /// One connection per endpoint, kept across calls and shared by + /// `get_record` and `drain_record`; callers take its lock for each call. + /// Keeping it open matters twice: the server's drain cursor lives in the + /// session, so values accumulate between drains, and on serial a second + /// connection either fails (the tty is open) or sends a new `hello` that + /// resets the board's session and discards that cursor. + /// + /// The client is replaced only once its engine has stopped + /// ([`AimxConnection::is_closed`]), never because a call returned an error: + /// `not_found`, `unsupported` and even `internal` are replies on a healthy + /// session. A client busy with a call is returned as is; if its engine has + /// stopped, the next call replaces it. pub async fn get_drain_client( &self, endpoint: &str, ) -> Result>, ClientError> { - let drain_map = self.drain_clients.lock().await; + let mut drain_map = self.drain_clients.lock().await; if let Some(client) = drain_map.get(endpoint) { - debug!("♻️ Reusing persistent drain client for {}", endpoint); - return Ok(Arc::clone(client)); + let closed = client.try_lock().is_ok_and(|c| c.is_closed()); + if !closed { + debug!("♻️ Reusing persistent client for {}", endpoint); + return Ok(Arc::clone(client)); + } + debug!( + "🔁 Persistent client for {} has stopped, reconnecting", + endpoint + ); + drain_map.remove(endpoint); } debug!("🔌 Creating persistent drain client for {}", endpoint); @@ -109,7 +125,9 @@ impl ConnectionPool { let mut drain_map = self.drain_clients.lock().await; // Double-check: another task may have inserted while we were connecting if let Some(existing) = drain_map.get(endpoint) { - return Ok(Arc::clone(existing)); + if !existing.try_lock().is_ok_and(|c| c.is_closed()) { + return Ok(Arc::clone(existing)); + } } drain_map.insert(endpoint.to_string(), Arc::clone(&shared)); Ok(shared) diff --git a/tools/aimdb-mcp/src/tools/record.rs b/tools/aimdb-mcp/src/tools/record.rs index 6692cb1e..1cb64cd0 100644 --- a/tools/aimdb-mcp/src/tools/record.rs +++ b/tools/aimdb-mcp/src/tools/record.rs @@ -161,19 +161,21 @@ pub async fn get_record(args: Option) -> McpResult { endpoint, params.record_name ); - // `record.get` is a single non-destructive read with no drain cursor, so it - // needs no retry: `not_found` (no such record, or no value yet) and - // `unsupported` (a ring, which has no current value) come back at once. - let client = if let Some(pool) = super::connection_pool() { - pool.get_connection(&endpoint) - .await - .map_err(McpError::Client)? - } else { - AimxConnection::connect(&endpoint) - .await - .map_err(McpError::Client)? - }; + // The persistent client `drain_record` uses: on serial a second connection + // fails (the tty is open) or says `hello` again, which resets the board's + // session and its drain cursors. Sharing is safe because `record.get` never + // touches the drain cursor (the server keeps them apart). + let pool = super::connection_pool() + .ok_or_else(|| McpError::Internal("Connection pool not initialized".to_string()))?; + let client_arc = pool + .get_drain_client(&endpoint) + .await + .map_err(McpError::Client)?; + let client = client_arc.lock().await; + // One `record.get`, no retry: `not_found` (no such record, or no value yet) + // and `unsupported` (a ring) come back at once and leave the session as it + // is. A stopped engine is replaced by `get_drain_client` on the next call. let sample = client .get_record(¶ms.record_name) .await @@ -255,8 +257,8 @@ pub async fn drain_record(args: Option) -> McpResult { endpoint, params.record_name ); - // Use persistent drain client so the server-side drain reader persists - // across calls, allowing values to accumulate between drains. + // The persistent client, shared with `get_record`: the server-side drain + // cursor lives in its session, so values accumulate between calls. let pool = super::connection_pool() .ok_or_else(|| McpError::Internal("Connection pool not initialized".to_string()))?; @@ -267,27 +269,17 @@ pub async fn drain_record(args: Option) -> McpResult { let client = client_arc.lock().await; - // Drain record values + // An error reply keeps the session (and its cursors); a stopped engine is + // replaced by `get_drain_client` on the next call. let response = match params.limit { Some(limit) => client .drain_record_with_limit(¶ms.record_name, limit) .await - .map_err(|e| { - // On connection error, invalidate so next call reconnects - let socket = endpoint.clone(); - let pool = pool.clone(); - tokio::spawn(async move { pool.invalidate_drain_client(&socket).await }); - McpError::Client(e) - })?, + .map_err(McpError::Client)?, None => client .drain_record(¶ms.record_name) .await - .map_err(|e| { - let socket = endpoint.clone(); - let pool = pool.clone(); - tokio::spawn(async move { pool.invalidate_drain_client(&socket).await }); - McpError::Client(e) - })?, + .map_err(McpError::Client)?, }; debug!( diff --git a/tools/aimdb-mcp/tests/persistent_client.rs b/tools/aimdb-mcp/tests/persistent_client.rs new file mode 100644 index 00000000..79ad6ff3 --- /dev/null +++ b/tools/aimdb-mcp/tests/persistent_client.rs @@ -0,0 +1,110 @@ +//! The MCP keeps one AimX client per endpoint and replaces it only when its +//! engine has stopped, never because a call returned an error. + +use std::path::{Path, PathBuf}; +use std::sync::Arc; +use std::time::Duration; + +use aimdb_core::buffer::BufferCfg; +use aimdb_core::remote::AimxConfig; +use aimdb_core::AimDbBuilder; +use aimdb_mcp::connection::ConnectionPool; +use aimdb_tokio_adapter::{TokioAdapter, TokioRecordRegistrarExt}; +use aimdb_uds_connector::UdsServer; +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Clone, Serialize, Deserialize)] +struct State { + n: u64, +} + +/// An AimX server in its own runtime on its own thread. Dropping it shuts that +/// runtime down, which drops every session, as a crashed or rebooted instance +/// would. +struct Server { + stop: Option>, + thread: Option>, +} + +impl Server { + fn start(sock: &Path) -> Self { + let sock: PathBuf = sock.to_path_buf(); + let (ready_tx, ready_rx) = std::sync::mpsc::channel(); + let (stop, stop_rx) = tokio::sync::oneshot::channel::<()>(); + let thread = std::thread::spawn(move || { + let rt = tokio::runtime::Runtime::new().unwrap(); + rt.block_on(async move { + let _ = std::fs::remove_file(&sock); + let config = AimxConfig::uds_default().socket_path(sock.to_str().unwrap()); + let mut builder = AimDbBuilder::new() + .runtime(Arc::new(TokioAdapter)) + .with_connector(UdsServer::from_config(config)); + builder.configure::("state", |reg| { + reg.buffer(BufferCfg::SingleLatest).with_remote_access(); + }); + let (db, runner) = builder.build().await.expect("build"); + db.producer::("state") + .unwrap() + .produce(State { n: 1 }); + tokio::spawn(runner.run()); + while !sock.exists() { + tokio::time::sleep(Duration::from_millis(5)).await; + } + ready_tx.send(()).unwrap(); + let _ = stop_rx.await; + drop(db); + }); + rt.shutdown_timeout(Duration::from_secs(1)); + }); + ready_rx.recv().unwrap(); + Self { + stop: Some(stop), + thread: Some(thread), + } + } +} + +impl Drop for Server { + fn drop(&mut self) { + let _ = self.stop.take().unwrap().send(()); + self.thread.take().unwrap().join().unwrap(); + } +} + +#[tokio::test] +async fn the_client_is_kept_across_error_replies_and_replaced_once_stopped() { + let dir = tempfile::tempdir().unwrap(); + let sock = dir.path().join("aimdb.sock"); + let endpoint = sock.to_str().unwrap().to_string(); + let pool = ConnectionPool::new(); + + let server = Server::start(&sock); + let first = pool.get_drain_client(&endpoint).await.expect("connect"); + + // Error replies are answers on a healthy session: the client stays. + for missing in ["missing", "also-missing"] { + assert!(first.lock().await.get_record(missing).await.is_err()); + let again = pool.get_drain_client(&endpoint).await.unwrap(); + assert!(Arc::ptr_eq(&first, &again), "kept after an error reply"); + } + assert!(!first.lock().await.is_closed()); + + // The instance goes away: the engine stops. + drop(server); + let mut stopped = false; + for _ in 0..200 { + if first.lock().await.is_closed() { + stopped = true; + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + assert!(stopped, "the engine stops when the transport closes"); + + // The instance is back: the next call replaces the stopped client. + let _server = Server::start(&sock); + let next = pool.get_drain_client(&endpoint).await.expect("reconnect"); + assert!(!Arc::ptr_eq(&first, &next), "a stopped client is replaced"); + let got = next.lock().await.get_record("state").await.expect("get"); + assert_eq!(got.value["n"], 1); +} diff --git a/tools/aimdb-mcp/tests/serial_session.rs b/tools/aimdb-mcp/tests/serial_session.rs new file mode 100644 index 00000000..80a5c68f --- /dev/null +++ b/tools/aimdb-mcp/tests/serial_session.rs @@ -0,0 +1,225 @@ +//! `get_record` and `drain_record` share one serial session (#450). +//! +//! A UART never closes, so a board learns of a new client only from a `hello`, +//! and a `hello` resets its session, drain cursors included. If `get_record` +//! opened a second connection, it would either fail (the tty is already open) +//! or say `hello` again and lose the cursor `drain_record` reads. +//! +//! The board is a `SerialServer` behind a pty; `aimdb-mcp` opens the other +//! side as `serial://…`. A relay between the pty and the board counts the +//! `hello`s the board receives. + +#![cfg(all(feature = "transport-serial", target_os = "linux"))] + +use std::fs::File; +use std::io::{Read, Write}; +use std::os::fd::{FromRawFd, OwnedFd}; +use std::sync::atomic::{AtomicUsize, Ordering::SeqCst}; +use std::sync::Arc; +use std::time::Duration; + +use aimdb_core::buffer::BufferCfg; +use aimdb_core::AimDbBuilder; +use aimdb_mcp::tools; +use aimdb_serial_connector::framing::FrameAccumulator; +use aimdb_serial_connector::SerialServer; +use aimdb_tokio_adapter::net::TokioByteStream; +use aimdb_tokio_adapter::{TokioAdapter, TokioRecordRegistrarExt}; +use serde::{Deserialize, Serialize}; +use serde_json::{json, Value}; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; + +#[derive(Debug, Clone, Serialize, Deserialize)] +struct Reading { + n: u64, +} + +/// A pty pair: the master (the board's wire) and the slave's path (the +/// host's tty). The slave fd is held open so the master never sees a +/// hang-up between host opens. +struct Pty { + master: File, + path: String, + _slave: OwnedFd, +} + +fn openpty() -> Pty { + let (mut master, mut slave) = (0, 0); + // SAFETY: `openpty` writes two fresh fds; null name/termios/winsize are + // allowed. + let rc = unsafe { + libc::openpty( + &mut master, + &mut slave, + std::ptr::null_mut(), + std::ptr::null(), + std::ptr::null(), + ) + }; + assert_eq!(rc, 0, "openpty: {}", std::io::Error::last_os_error()); + // SAFETY: both fds were just returned by `openpty` and are owned here. + let (master, slave) = unsafe { (File::from_raw_fd(master), OwnedFd::from_raw_fd(slave)) }; + let path = std::fs::read_link(format!("/proc/self/fd/{}", { + use std::os::fd::AsRawFd; + slave.as_raw_fd() + })) + .unwrap() + .to_string_lossy() + .into_owned(); + Pty { + master, + path, + _slave: slave, + } +} + +/// Relay bytes between the pty master and the board's end of a duplex, +/// counting the `hello` requests on the way to the board. Blocking pty I/O +/// runs on plain threads, so nothing left blocked can stall the runtime. +fn relay(master: File, board_line: tokio::io::DuplexStream, hellos: Arc) { + let (mut line_rd, mut line_wr) = tokio::io::split(board_line); + let mut master_wr = master.try_clone().unwrap(); + let mut master_rd = master; + + // host → board + let (to_board, mut to_board_rx) = tokio::sync::mpsc::unbounded_channel::>(); + std::thread::spawn(move || { + let mut frames = FrameAccumulator::new(); + let mut buf = [0u8; 512]; + while let Ok(n) = master_rd.read(&mut buf) { + if n == 0 { + break; + } + frames.push_bytes(&buf[..n]); + while let Some(frame) = frames.next_frame() { + let Ok(frame) = frame else { continue }; + let msg: Value = serde_json::from_slice(&frame).unwrap_or(Value::Null); + if msg["method"] == "hello" { + hellos.fetch_add(1, SeqCst); + } + } + if to_board.send(buf[..n].to_vec()).is_err() { + break; + } + } + }); + tokio::spawn(async move { + while let Some(bytes) = to_board_rx.recv().await { + if line_wr.write_all(&bytes).await.is_err() { + break; + } + } + }); + + // board → host + let (to_host, to_host_rx) = std::sync::mpsc::channel::>(); + tokio::spawn(async move { + let mut buf = [0u8; 512]; + while let Ok(n) = line_rd.read(&mut buf).await { + if n == 0 || to_host.send(buf[..n].to_vec()).is_err() { + break; + } + } + }); + std::thread::spawn(move || { + while let Ok(bytes) = to_host_rx.recv() { + if master_wr.write_all(&bytes).is_err() { + break; + } + } + }); +} + +#[tokio::test] +async fn get_record_shares_the_drain_session_over_serial() { + // The board: a ring of events and a state record, served over the line. + let (board_end, line_end) = tokio::io::duplex(64 * 1024); + let mut builder = AimDbBuilder::new() + .runtime(Arc::new(TokioAdapter)) + .with_connector(SerialServer::new(TokioByteStream(board_end))); + builder.configure::("events", |reg| { + reg.buffer(BufferCfg::SpmcRing { capacity: 16 }) + .with_remote_access(); + }); + builder.configure::("state", |reg| { + reg.buffer(BufferCfg::SingleLatest).with_remote_access(); + }); + let (board, runner) = builder.build().await.expect("build board"); + tokio::spawn(runner.run()); + board + .producer::("state") + .unwrap() + .produce(Reading { n: 42 }); + + let pty = openpty(); + let hellos = Arc::new(AtomicUsize::new(0)); + relay(pty.master, line_end, hellos.clone()); + + tools::init_connection_pool(aimdb_mcp::connection::ConnectionPool::new()); + let endpoint = format!("serial://{}?baud=115200", pty.path); + let drain = || { + tools::drain_record(Some( + json!({ "endpoint": endpoint, "record_name": "events" }), + )) + }; + let get = + |name: &str| tools::get_record(Some(json!({ "endpoint": endpoint, "record_name": name }))); + + // 1. Open the ring's drain cursor. + let first = drain().await.expect("first drain"); + assert_eq!(first["count"], 0); + + // 2. The board publishes events. + let events = board.producer::("events").unwrap(); + for n in 1..=3 { + events.produce(Reading { n }); + } + + // 3. Read other records on the same session, misses included. + let got = get("state").await.expect("get_record over serial"); + assert_eq!(got["value"], json!({ "n": 42 })); + let err = get("missing") + .await + .expect_err("unknown record") + .to_string(); + assert!(err.contains("not_found"), "{err}"); + let err = get("events").await.expect_err("a ring").to_string(); + assert!(err.contains("unsupported"), "{err}"); + // An error reply from drain keeps the session too. + let err = tools::drain_record(Some( + json!({ "endpoint": endpoint, "record_name": "missing" }), + )) + .await + .expect_err("unknown record") + .to_string(); + assert!(err.contains("not_found"), "{err}"); + // Let any deferred cleanup run before the session is used again. + tokio::time::sleep(Duration::from_millis(50)).await; + + // 4. The cursor survived: the drain returns the queued events. + let mut ns = Vec::new(); + for _ in 0..100 { + let page = drain().await.expect("drain"); + ns.extend( + page["values"] + .as_array() + .unwrap() + .iter() + .map(|v| v["value"]["n"].clone()), + ); + if ns.len() >= 3 { + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + assert_eq!(ns, [1, 2, 3]); + + // 5. One session the whole time, still usable. + let got = get("state").await.expect("get_record after the drains"); + assert_eq!(got["value"], json!({ "n": 42 })); + assert_eq!( + hellos.load(SeqCst), + 1, + "the board received exactly one hello" + ); +}