Feat/retire aimdb ws protocol - #201
Conversation
…sport on AimX The mapping-table design doc 036 A2 gated the protocol unification on (038 §3.9, 034 §3.10 rc10): AimX ↔ ws-protocol frame mapping, the gap decisions (wildcard subscribe via optional Event.topic, Snapshot routing via a new sub field, the explicit subscribed ack, the 6→3 error-code collapse, shared record.query/record.list result shapes), two scope corrections verified against the code (the aimdb-client demux fold was already done in PR #124; query passthrough already existed — only the result shape was missing), and the go/no-go. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r AimX (design 045)
The two features that justified the ws-protocol fork, added to AimX so
every transport inherits them:
- session::topic_match — the MQTT-style matcher moved in from
aimdb-ws-protocol (same semantics, same tests) plus is_wildcard.
- Subscription streams yield SubUpdate {topic: Option<Arc<str>>, data}
on both engines; Outbound::Event gains an optional topic and
Outbound::Snapshot the routing sub, so a wildcard subscription's
events and late-join snapshots name the record that fired.
Session::snapshot becomes snapshots() — one per covered record.
- AimxDispatch matches wildcard patterns against the registry once at
subscribe time (builder-frozen record set), merges matched streams
under one subscription id, and snapshots each match on open.
- AimxCodec learns the {"t":"subscribed"} ack (servers running
acks_subscribe:true — the WS connector) and gets a dedicated
roundtrip test suite; exact-topic frames are unchanged on the wire.
- ClientConfig::topic_routed_subs removed — it existed solely for the
retired ws wire; all subscriptions are id-routed.
- Shared result vocabulary: remote::QueryRecord {topic, payload, ts},
with_persistence's QueryHandlerFn returning {records, total} (sorted
by ts), and RecordMetadata gaining optional schema_type/entity.
- aimdb-client: AimxConnection::subscribe_with_topics yields
(Option<String>, Value) pairs; wildcard e2e test over the production
UDS server; query-shape test against the persistence registration.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ign 045)
The wire is now AimX — one tagged frame per WS text message, the same
envelope as UDS/serial/TCP; the delta really was just the envelope
(everything below the codec already rode the shared session engine).
- run_session drives the shared AimxCodec; the 507-line per-connection
WsCodec and its id↔topic maps are deleted, as are the multi-topic
Subscribe split and the protocol re-export shim.
- ClientManager delivers topic-tagged SubUpdates; the per-frame envelope
is applied by each connection's codec (payload bytes stay Arc-shared).
The pre-serialized Data frame, its server-side ts, and with_raw_payload
are gone.
- WsDispatch converges on the AimX vocabulary: record.query via a
plugged-in QueryHandler (now returning core QueryRecord rows) or the
Extensions QueryHandlerFn from with_persistence; record.list answers
{name, schema_type, entity} rows (TopicInfo now lives here).
Errors collapse to the 3-code set; auth stays at the HTTP 401.
- SnapshotProvider::snapshots(pattern) returns every cached value under
the pattern, so wildcard subscriptions late-join each covered record.
- Auto-subscribe seeds AimX sub frames with ids counting down from
u64::MAX (cannot collide with client-chosen ids; design 045 §3.6).
- ws-client builder rides AimxCodec (topic_routed_subs gone); e2e tests
rewritten to drive raw AimX frames, including golden wire frames and
ws↔AimX parity for ack/wildcard/snapshot/query/list.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…c (design 045)
The browser is now a first-class engine client: a web_sys::WebSocket-
backed Connection/Dialer pair (JS events funneled into the engine's
async recv; single-threaded wasm Send shims, same pattern and compile
guard as SendFuture) drives run_client + AimxCodec, so reply and
subscription correlation, reconnect backoff, keepalive, and the offline
queue exist exactly once — in core. The hand-rolled 835-line demux is
gone.
The #[wasm_bindgen] surface is preserved (write, query, listTopics,
onStatusChange, status, disconnect, connectBridge options; lateJoin is
retained for option-shape compatibility — snapshots are server-driven
under AimX). Subscription pumps mirror topic-tagged updates into local
records and re-subscribe when a stream ends; the queued subscribe
replays after the engine redials. WasmDb.discover speaks a one-shot
record.list request and resolves {name, schema_type, entity} rows.
Depends on aimdb-core's connector-session + remote features (the
engines cross-compile to wasm32); the aimdb-ws-protocol dep is gone.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…idue (design 045) With no consumers left, the fork is gone: - aimdb-ws-protocol crate deleted (353 lines) and de-registered from the workspace members, Makefile build/test/clippy/doc targets, and the publish sequence (17 → 16 crates). - aimdb-client: dead NDJSON helpers retired with the hand-rolled client in PR #124 are deleted (RequestExt/ResponseExt/serialize_message/ parse_message/EventMessage/cli_hello — grep-confirmed zero consumers); the protocol re-export keeps RecordMetadata/WelcomeMessage/Request/ Response/Event. - design 038 §2.5/§3.9 annotated as resolved by 045; root and per-crate CHANGELOGs describe the breaking release. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nents and documentation
…ancelToken, CancelHandle, WasmWsConnection, WasmWsDialer and WsBridge
…ndshake and compatibility checks
…ns and related components
- Updated comments in `Cargo.toml` files to clarify transport features. - Revised comments in `engine.rs`, `aimx_session.rs`, and various other files to remove design references and improve clarity. - Removed outdated design references in `protocol.rs`, `query.rs`, and `codec.rs`. - Enhanced clarity in `dispatch.rs`, `builder_ext.rs`, and `http.rs` by simplifying comments. - Adjusted test descriptions in `query_handler_shape.rs` and `e2e.rs` for better understanding. - Cleaned up `ws_bridge.rs`, `registry.rs`, and `session.rs` to focus on current functionality without legacy design notes. - Updated `CHANGELOG.md` to reflect changes in `record.list` response structure and removed references to obsolete types. - Improved overall consistency and readability of comments throughout the codebase.
Reconcile main's direct-JSON-byte remote path (#192/#203) with design-048's SubUpdate.skipped loss-signal design: - Subscribe path keeps design-048's SubUpdate/skipped contract (the merged Session::subscribe trait yields BoxStream<SubUpdate>); stream.rs stays on the (Value, u64) design. - dispatch.rs combines both: SubUpdate subscribe + wildcard fan-in + WI1 hello() version gate, plus main's DispatchReply / record.get fast path. Restored the futures_util::StreamExt import main had dropped. - aimdb-bench/Cargo.toml unions both bench sets (fanout_encode + remote_json); remote_json.rs updated for Outbound::Event's new `topic` field. main's direct-JSON-bytes optimization in the live subscribe path is superseded here and left as the documented follow-up (design 048, "removing the intermediate serde_json::Value"). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The subscribe path never inspects a record update between reading it and encoding it onto the wire, yet it parsed the buffer's JSON bytes into a `serde_json::Value` (`recv_json`) only to re-serialize the tree back to bytes (`to_payload`) at the dispatch boundary — a pure parse+re-serialize round-trip per update. This is the direct-JSON-bytes follow-up design 048 flagged. `stream_record_updates` now reads owned JSON bytes via `recv_json_bytes` and yields `(Payload, u64)`; the two `stream.map` sites in the AimX dispatch build `SubUpdate` directly from those bytes. The design-048 `skipped` loss signal is untouched — it still rides each `SubUpdate`. No wire or public API change. Verified with the `remote_json` subscription-event benchmark (Tree vs Direct): the direct-bytes path sustains ~2× the throughput of the former tree path, matching the 2.06–2.49× encode figure in design 048. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…DB client and CLI
… for transform join
… the client engine
… final frame flag
…prevent pending entry accumulation
…ve connection stability
…-chromedriver version compatibility
thaodt
left a comment
There was a problem hiding this comment.
I'm requesting changes for the remaining correctness blockers described inline, primarily the lost snapshot_end marker at the public API boundary and request-ID wraparound.
Separately, I noticed the quiet ws subscription lazy-pruning issue also exists on main and is not a regression of this PR, so it should be tracked in a separate follow-up issue rather than block it.
| ws.set_onerror(Some(on_error.as_ref().unchecked_ref())); | ||
| plain_callbacks.push(on_error); | ||
|
|
||
| if open_rx.await != Ok(true) { |
There was a problem hiding this comment.
The post-handshake stopped check fixes the late-adoption/status-resurrection part. I'm reopening this because disconnect() still cannot interrupt open_rx.await: while the handshake is pending, the socket is not reachable through shared.ws, so the dial future and its callbacks can remain alive until the browser eventually emits an open/close/error event.
The added regression proves that the socket is rejected after the handshake resolves, but it does not prove prompt cancellation while the handshake remains pending. Could we track and close the in-flight socket, and add a test that keeps open_rx pending and verifies that disconnect() settles the dial promptly?
4ea7bfc to
36eb670
Compare
The transform_join browser target hangs at "Loading scripts..." in the wasm-browser-tests job while the lib target passes, and it loads fine locally including pinned to one core. Ruled out by prior runs: Chrome sandbox, harness timeout (it hung the full 120 s), module size (3 MB stripped behaves like 23 MB unstripped), CPU, and session order — it fails alone as the job's first browser session. The page dies before the harness boots, so the harness's own console divs stay empty and report nothing useful. Run the target under a verbose chromedriver log, which carries Chrome's stderr, and print it. Temporary diagnostic, continue-on-error so the suite still reports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
36eb670 to
1d12efc
Compare
The transform_join browser target never loaded in CI: its generated JS glue failed to parse with SyntaxError: Identifier 'wasm_bindgen_..._convert__closures_____invoke______' has already been declared Two closure-invoke shims mangle to one JS identifier, so the module is a parse error and no test ever starts — which is why the page sat at "Loading scripts..." with empty console divs and why raising the harness timeout, shrinking the module and reordering sessions all changed nothing. It reproduces only in CI because mangling depends on the compiler: the devcontainer is on rustc 1.91.1 while the workflow floats on @stable (1.97.1). wasm-bindgen 0.2.105 was held at that version by wasm-bindgen-test 0.3.55; moving the family together picks up the upstream codegen fix. wasm-bindgen 0.2.105 -> 0.2.126, wasm-bindgen-test 0.3.55 -> 0.3.76, web-sys/js-sys 0.3.82 -> 0.3.103. Only aimdb-wasm-adapter depends on them; clippy, test-wasm and the browser suite pass locally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The verbose chromedriver capture found the cause (duplicate wasm-bindgen JS identifier, fixed by the 0.2.126 upgrade) and the suite is green, so the scaffolding comes out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@lxsaah lemme know whenever you've finished all the changes and want me review the final round I think 😸 |
…r handling - Refactor `AimxConnection::subscribe` to yield `Result<RecordUpdate, ClientError>`, allowing consumers to handle errors and delivery gaps. - Update `RecordUpdate` struct to include metadata for delivery gaps and snapshot completion. - Modify `aimdb watch` command to report gaps and handle refused subscriptions as terminal errors. - Improve documentation and examples to reflect the new subscription behavior and metadata.
…d mirroring logic
Closes the two browser-bridge threads from the PR #201 review (design 049 item D). Both are the same blind spot: the socket's lifecycle was invisible between `close_channel`/`WebSocket::new` and `onclose`. D1 — a connection dropped without an `onclose` (frame-funnel overflow, engine stop) left JS reading `"connected"` forever with `autoReconnect: false`: `Drop` detaches `onclose` before closing, so nothing reported the transition. It now reports `drop_status()` itself when it still owns the published socket; `set_status` deduplicates, so `onclose`/`disconnect()` paths stay idempotent. D2 — `disconnect()` could not interrupt a pending handshake, since the pre-open socket was not published. The dialer now publishes it before awaiting `onopen`, so `disconnect()` closes it and the dial fails promptly instead of hanging until the browser's connect timeout. A dial interrupted by a stopped bridge returns terminal `Closed` rather than `Io`, so the engine stops on that attempt instead of sleeping a reconnect backoff first to learn the same thing. `accept_dial` loses its socket install and keeps only the stopped re-check. The funnel-overflow branch moves out of the `onmessage` closure into `funnel_frame`, which is reachable in a test — the closure is only installed by a handshake that completes, and the browser lane has no WebSocket peer to complete one against. It also now checks `is_closed()` up front: a full sender stays parked, so the old code re-announced the overflow for every frame still arriving before the socket closed. Regressions (browser lane, run by CI's wasm-browser-tests job): the drop seam in both reconnect modes and after a `disconnect()` took the socket; the overflow trigger ending the frame stream; and the pending-dial interruption, at the dialer and through `WsBridge::disconnect`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@thaodt the PR is now again ready to be reviewed! Thanks again!! |
thaodt
left a comment
There was a problem hiding this comment.
The previous correctness blockers are fixed: snapshot loss is now observable through the public API, request-ID exhaustion is refused rather than wrapped, pending calls are canceled in O(1), the in-flight WASM dial is interruptible, terminal subscription errors are preserved and the browser tests are running successfully in CI.
Again im requesting changes for one confirmed migration blocker: WsBridge.maxOfflineQueue is no longer a hard bound during an initial pending dial or reconnect backoff, despite the preserved public option and design contract.
I also left:
- a security-contract question about the newly automatic persistence query and full
record.listexposure - two lower-priority WASM findings covering synchronous constructor failure and frame-byte budgeting
- a few documentation/counting corrections.
I intentionally did not charge the generic duplicate-subscription-ID issue to this PR because the retired ws codec could already trigger it by reusing the synthesized ID for a repeated topic subscription. Likewise, the unchecked serializer splice follows design 048's explicitly documented trusted-payload contract and is not being treated as a blocker here.
| max_reconnect_delay: 8_000, | ||
| max_reconnect_attempts: 0, | ||
| keepalive_interval: (config.keepalive_ms > 0).then_some(config.keepalive_ms as u64), | ||
| max_offline_queue: config.max_offline_queue, |
There was a problem hiding this comment.
run_client uses an unbounded command channel, while bound_offline_queue() runs only once before reconnect backoff. Commands accepted during an initial pending dial or during that sleep can exceed the configured cap; if the next dial succeeds, drive_connection flushes them without another trim.
I reproduced max_offline_queue = 2 returning from backoff with 102 queued commands. The retired bridge enforced this limit at enqueue time and design 047 says the JS offline-queue behavior is preserved.
Please enforce the cap continuously while offline, define whether the oldest or newest commands are dropped, and cover both a blocked initial dial and reconnect backoff.
| } | ||
| serde_json::json!(records) | ||
| } | ||
| "record.query" => { |
There was a problem hiding this comment.
Is record.query intentionally authorized only at connection level?
With this PR, a WebSocket client authenticated with empty subscribe/write permissions can still invoke the automatic with_persistence() fallback with name: "*". I verified this end-to-end on this head. Persisted records do not need .with_remote_access() and record.list now also returns every database record rather than only the records exposed through the ws connector.
This may be intentional as part of AimX convergence and Im not assuming that subscribe_patterns should automatically govern historical queries. However, the current AuthHandler API has no query/list authorization hook, so applications cannot express a per-client historical-read policy.
Could we make the intended security contract explicit and add a regression for it? If query/list access should follow record visibility, this needs filtering or a dedicated authorization hook before merge.
| return Err(TransportError::Closed); | ||
| } | ||
|
|
||
| let ws = web_sys::WebSocket::new(&url).map_err(|_| TransportError::Io)?; |
There was a problem hiding this comment.
A synchronous WebSocket::new failure no longer reaches the connectBridge caller.
The retired bridge constructed the socket in new_internal and returned a JsError when the URL was invalid. The migrated implementation returns the bridge first and later maps the constructor failure to TransportError::Io.
With autoReconnect = false, the engine exits while the observable status remains Connecting. With reconnect enabled, it repeatedly retries a permanent constructor error while still appearing to be in the initial connecting state.
Please surface this terminal failure deterministically and add a malformed-URL browser regression.
| let mut frame_tx = frame_tx.clone(); | ||
| Closure::wrap(Box::new(move |event: web_sys::MessageEvent| { | ||
| if let Some(text) = event.data().as_string() { | ||
| if funnel_frame(&mut frame_tx, text.into_bytes()) == Funneled::Overflowed { |
There was a problem hiding this comment.
FRAME_QUEUE_CAP bounds the number of frames, but not the amount of retained memory.
Each queued item is an unconstrained Vec<u8> and onmessage copies the complete JS string with into_bytes() before en-queueing it. With the default ws message ceiling allowing very large messages, 1024 queued frames can still represent an impractical amount of browser memory.
Please consider enforcing a per-frame byte limit before copying the string into the Rust queue and treating an oversized frame like queue overflow, close the connection and resubscribe from a clean state.
| fields. Core populates `entity` from the record key's final `.` segment | ||
| (the server owns the naming convention; clients must not parse topics). | ||
| `schema_type` is populated by dispatches that own a schema registry — the | ||
| WS dispatch answers `record.list` from its `StreamableRegistry`-derived |
There was a problem hiding this comment.
This still describes the retired three-field TopicInfo result, but the current impl calls db.list_records() and returns the full RecordMetadata rows.
In that shape, the record key is record_key, name is the Rust type name, not the topic name. WasmDb.discover forwards these rows to JS unchanged. The design note and WASM changelog currently suggest {name, schema_type, entity}, which can lead migrating clients to treat name as the record key.
Could we align the design, changelog and discovery example with the actual canonical RecordMetadata shape and field semantics?
| **Scope:** delete the `aimdb-ws-protocol` crate and the WS-JSON envelope it | ||
| defines, porting the WebSocket connector (`aimdb-websocket-connector`), the | ||
| browser bridge (`aimdb-wasm-adapter::ws_bridge`), and the UI's raw discovery | ||
| path onto the AimX-v2 envelope (`aimdb-core::session::aimx`). Extends AimX |
There was a problem hiding this comment.
Nitpick: The active documentation still calls this AimX v2, while PROTOCOL_VERSION and the WebSocket upgrade gate now use 3.0.
The stale wording appears in this design document and several current module/README docs (remote, AimxCodec, aimdb-client, the ws connector and the CLI). Could we update the active documentation to AimX v3 while leaving genuinely historical changelog entries unchanged?
| @printf "$(YELLOW) → Publishing aimdb-mcp (17/17)$(NC)\n" | ||
| @printf "$(YELLOW) → Publishing aimdb-mcp (16/16)$(NC)\n" | ||
| @cargo publish -p aimdb-mcp | ||
| @printf "$(GREEN)✓ All 17 crates published successfully!$(NC)\n" |
There was a problem hiding this comment.
Nitpick: The publish sequence was updated to 16/16 after removing aimdb-ws-protocol, but the final message still says All 17 crates published successfully.
|
Thanks, as I am on leave this week - I'll need some time to get back to it. |
Retire
aimdb-ws-protocol— every transport speaks AimXDeletes the WebSocket-only wire protocol and converges the entire workspace on
the AimX-v3 envelope (
aimdb-core::session::aimx). One codec, one errormapping, one client demux — the WebSocket stack and the browser bridge now ride
the same session engine as UDS/serial/TCP and
aimdb-client.Implements design 047 (
docs/design/047-retire-ws-protocol-converge-on-aimx.md).Why
The workspace shipped two wire protocols for one feature. Every capability —
snapshots, acks, writes, queries, subscribe — existed twice, with two codecs and
two error vocabularies, plus a hand-rolled ~835-line browser demux duplicating
run_client. Both protocols already rode the same session engine, so the forkwas pure duplication.
Convergence removes a 353-line protocol crate, a 507-line codec, two error
mappings, and the browser demux, against a small additive AimX extension
(wildcard subscribe + two result-shape changes) that every transport now
inherits (CLI
watch 'temp.#', MCP wildcard reads, future serial/TCP browsers).What changed
Core (additive AimX extension)
#/*matches the (builder-frozen) record set once at subscribe time, merges the
matched records' update streams under one subscription id, and emits one
snapshot per matched record on open.
topic_matchmodule — MQTT-style matching over dot-separated keys, sharedby the core wildcard path and the WS fan-out bus.
SubUpdateitem type — subscription streams now carry{ topic: Option<Arc<str>>, data, skipped }so each event names the concreterecord that fired and threads buffer-lag counts through for drop detection.
topiconevent,subonsnap, an explicitsubscribedack frame (WS only; UDS/serial/TCP stay ack-implicit).record.query→{ records: [{topic, payload, ts}], total }(canonicalQueryRecordrow);RecordMetadatagainsschema_typeandentity.hello— protocol bumped2.0 → 3.0; an incompatible orversion-less peer is refused at the handshake (
RpcError::VersionMismatch).skipped; the server pump doesseq += skipped + 1; the client demux recovers loss as aseqshortfall.WebSocket connector
framing). Dispatch converged on
record.query/record.list.(topic, payload)asArc-shared updates; theper-connection codec envelopes each event (O(1) fan-out preserved).
record.queryfalls back to the persistence-registeredQueryHandlerFnwhenno custom handler is plugged in;
record.listrows are stamped with schemanames from the connector's
StreamableRegistry.WsCodec+ id↔topic maps,split_multi_topic, the pre-serializedData-frame broadcast,with_raw_payload,ClientConfig::topic_routed_subs.Browser bridge (
aimdb-wasm-adapter::ws_bridge)run_client+ClientHandleover aweb_sys::WebSocket-backedConnection/Dialer(single-threaded wasmSend/Syncwrappers). Reply andsubscription correlation now exists exactly once, in core.
write,query,listTopics,onStatusChange, offlinequeue, and reconnect are all delegated to the engine.
Deletions
aimdb-ws-protocolcrate,WsCodec,protocol.rsshims, deadaimdb-clientre-exports.
Breaking changes
2.0 → 3.0— pre-3.x clients are refused athello. Browserclients on the old ws-protocol wire must ship the rebuilt bridge; they fail
closed against a 3.0 server.
with_raw_payload,ClientConfig::topic_routed_subs.record.queryresult shape changed from{values:[{record,value,stored_at}], count}to{records:[{topic,payload,ts}],total}.Lands as one branch in a release already flagged protocol-breaking, one commit
per work item (no stacked PRs).
Testing
make checkandmake allgreen. New/updated coverage: AimX codec roundtripsuite (frames + splice byte-for-byte),
topic_matchtests,topic_leafdual-separator, version-compatibility matrix,
query_handler_shape,UDS
handshake_version, rewritten WebSockete2e.rs(subscribe ack, wildcardfan-out, late-join), and fan-out encode benchmarks.
Known limitations (accepted)
server-chosen sub ids (counted down from
u64::MAX); arun_client-basedconsumer drops events for ids it never issued, so engine clients (including the
new bridge) subscribe explicitly. No user-visible change (the pro hub already
subscribes by exact name).
time against the builder-frozen record set; MQTT-style late registration is
deferred until core grows runtime registration.