Repository navigation
fix: record.get answers at once and no longer drains; MCP get/drain share one session (#444) - #450
Merged
Merged
Conversation
#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 <[email protected]> Claude-Session: https://claude.ai/code/session_01WGkDJ6rHvDMa63myz3jwfZ
…d 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 <[email protected]> Claude-Session: https://claude.ai/code/session_01WGkDJ6rHvDMa63myz3jwfZ
…' into claude/fervent-heisenberg-jjqj28
…uns (#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<T> 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 <[email protected]> Claude-Session: https://claude.ai/code/session_01WGkDJ6rHvDMa63myz3jwfZ
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 <[email protected]> Claude-Session: https://claude.ai/code/session_01WGkDJ6rHvDMa63myz3jwfZ
…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 <[email protected]> Claude-Session: https://claude.ai/code/session_01WGkDJ6rHvDMa63myz3jwfZ
lxsaah
changed the title
2 of 5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
aimdb-mcp'sget_recordtook about 3 s for asingle_latestrecord with no value, and for a misspelled name (#444). The cause was an MCP retry onnot_found. It existed because the server answeredrecord.geton a ring by draining the connection's cursor. That coupling had two side effects: a point-in-time read waited for a future write, and it consumed events a laterrecord.drainshould have returned.This PR removes the coupling.
getanddrainbecome separate operations that can share one session.record.get(server,aimdb-core)SingleLatest,Mailbox). It never touches the connection's drain cursor; onlyrecord.drainadvances it.SingleLatest/Mailboxrecord, getsnot_foundat once;SpmcRing) gets the newRpcError::Unsupported, wire code"unsupported". A ring is read withrecord.drain(unread events) orrecord.subscribe(future events).buffer_raw()and nobuffer_cfg()answersnot_found, because core cannot tell it is a ring.aimdb-mcpget_recorddrops the 3 s retry.get_recordanddrain_recordshare the persistent per-endpoint client (get_drain_client()). Each takes the per-client lock and sends one RPC. Over serial, a second connection either failed (the tty was already open) or sent a newhello. Thathelloreset the board's session and discarded its drain cursor.not_found,unsupported, eveninternal.drain_recordno longer drops its client on any error.AimxConnection::is_closed()), not because of a reply code.aimdb-clientUnsupportedto an error that points atdrain_recordandsubscribe.AimxConnection::is_closed().watch::armtreatsunsupportedlikenot_found: no baseline. A new ring subscription never replays an earlier value, so nothing old can count.Not in this PR
LatestSlotaround ring buffers to provide one, and it was dropped. It allocated on every ring write, held a writer lock while waking readers, and failed the connector allocation gate. An application that needs an inspectable current state can expose aSingleLatestrecord alongside its event stream. If AimDB needs the exact last ring event, that belongs inside the buffer's own design.Compatibility
RpcErroris#[non_exhaustive], so the new variant doesn't break code that matches on it. A released client that predates this decodesunsupportedasinternal. The protocol version is unchanged.Tests
tools/aimdb-mcp/tests/get_record.rsgetstill returns every event.aimdb-client/tests/aimx_session.rsSingleLatestrecord.tools/aimdb-mcp/tests/serial_session.rsSerialServerboard behind a pty, with a relay that countshellos. Open a ring's drain cursor, publish events, callget_record(a hit, plus misses and a drain error), then drain again. The read succeeds, the drain returns the queued events, and the board receives exactly onehello. Added tomake test.tools/aimdb-mcp/tests/persistent_client.rsEach test fails when its bug is put back:
not_foundget_recordopens its own connectiondrain_recorddrops its client on an error replymake bench-gate(b0_alloc_connector) passes.Related Issue
Checklist
make check). The fullmake checkwas not run. Run instead:transport-serial, the CLI, Cortex-M, and wasm32;bench-gate.🤖 Generated with Claude Code
https://claude.ai/code/session_01WGkDJ6rHvDMa63myz3jwfZ