Conversation
There was a problem hiding this comment.
Blocking:
{x}{y}in one MQTT level drops capturex, and the sharedspansbuffer then leaks stale values from earlier routes (grammar.rs / router.rs).- Backend parity still breaks for partially overlapping filters: embedded (MQTT 5) ingests twice what native (3.1.1) does, reproduced on Mosquitto (router.rs
subscriptions). inbound_keys.droppedcounts per route, not per message.
Design: the key table can be exhausted permanently by any publisher, since keys are assigned before deserialization and never freed.
Nits: literal {/} in topics is an undocumented breaking change and inbound/outbound are asymmetric; KeyId has no record identity; stale doc reference aimdb-websocket-connector/src/client/builder.rs:12 still mentions db.collect_inbound_routes.
| } | ||
| PatternPart::Capture { multi, .. } => { | ||
| if let Some(level) = raw.last_mut() { | ||
| level.capture = Some((captures, *multi)); |
There was a problem hiding this comment.
Bug: two captures in one level silently lose the first one.
a/{x}{y} (and {x}{y..}) compiles: the second capture overwrites level.capture, the level text stays empty, so it becomes Single(Some(1)) and the filter is a/+. Capture 0 (x) is never written by matches.
Combined with the router reusing one Spans array across routes (see the comment in router.rs), m.get("x") then returns whatever an earlier, non-matching route left there. Repro through AimDb + inbound_router("mqtt", &MqttGrammar):
reg.link_from("mqtt://s/{a}/nomatch").with_match_deserializer(..).finish(); // tried first
reg.link_from("mqtt://s/{x}{y}").with_match_deserializer(/* reads m.get("x") */).finish();
// route("s/ab") -> x == "ab" (route 1's span for {a}, which then failed on level 2)If x is the keyed capture, the wrong value gets keyed.
Suggested fix: reject when a level already has a capture, e.g.
PatternPart::Capture { multi, .. } => {
if let Some(level) = raw.last_mut() {
if level.capture.is_some() {
return Err(format!("'{topic}': a capture must be a whole level, not part of one"));
}
level.capture = Some((captures, *multi));
}
captures += 1;
}Generated by Claude Code
|
|
||
| // Linear search through all routes | ||
| // Note: Multiple routes may match the same resource_id (different types) | ||
| let mut spans: Spans = [(0, 0); MAX_CAPTURES]; |
There was a problem hiding this comment.
One spans buffer is shared across every route in the loop, and TopicFilter::matches writes spans as it goes, so a route that fails part-way leaves partial spans behind. Any grammar that doesn't write every capture on a successful match (the MqttGrammar {x}{y} case above, or a third-party TopicGrammar) then leaks stale values into TopicMatch::get and into key assignment.
Resetting before each attempt is cheap and makes this robust against any grammar (still zero-alloc):
for route in &self.routes {
spans = [(0, 0); MAX_CAPTURES];
if route.matches(resource_id, &mut spans) { … }
}Alternatively, document on TopicFilter::matches that a successful match must write every capture.
Generated by Claude Code
| Router::new(self.routes) | ||
| /// Filters to subscribe: [`resource_ids`](Self::resource_ids) without | ||
| /// the filters another one covers. | ||
| pub fn subscriptions(&self) -> Vec<Arc<str>> { |
There was a problem hiding this comment.
Backend parity still breaks on partially overlapping filters (reproduced against real Mosquitto 2).
Covering only removes filters that are fully covered. For r/{x}/c and r/b/{y} on one record, neither covers the other, so both are subscribed. One PUBLISH to r/b/c:
| backend | {x} link calls |
{y} link calls |
values in record |
|---|---|---|---|
| native (rumqttc, 3.1.1) | 1 | 1 | 2 |
| embedded (mountain-mqtt, MQTT 5) | 2 | 2 | 4 |
(Test: both backends against a local mosquitto, publishes via mosquitto_pub -q 1. The fully-covered exact-link case from the PR's parity test does hold on the real broker: 1 call each on both backends.)
So the 055 §3.1 parity problem remains for partial overlaps. Options: MQTT 5 subscription identifiers on the embedded backend (drop the PUBLISH unless it carries the lowest matching id), or document it as a known difference. Separately, even with one delivery the record receives the message once per matching link. That may be intended, but worth a line in the docs.
Generated by Claude Code
| matched = true; | ||
| match (route.ingest)(ctx, payload) { | ||
| let Some(key) = route.key(resource_id, &spans) else { | ||
| log_debug!("Key table full, dropped message on '{}'", resource_id); |
There was a problem hiding this comment.
dropped counts once per matching keyed route, not per message. With two keyed links on one record whose patterns both match (t/{d} and t/{dev}, capacity 1), one message on a new value reports inbound_keys.dropped == 2 in list_records(). The field doc says "Messages turned away because the table was full", so it either needs counting once per route() call per table, or the doc should say "per link".
Generated by Claude Code
|
|
||
| /// The key for `name`, assigned if new. `None` when the table is full; | ||
| /// the caller drops the message and it is counted. | ||
| pub(crate) fn key(&self, name: &str) -> Option<KeyId> { |
There was a problem hiding this comment.
Design: any publisher can permanently exhaust the key table. The key is assigned before the deserializer runs (Router::route → route.key() → ingest), and keys are never freed. With .key("device", 2), two junk publishes to sensors/junk1/temp and sensors/junk2/temp with unparseable payloads fill the table, and the real sensors/kitchen/temp is then dropped for the rest of the process's life.
On a shared broker that's an easy DoS for anyone with publish rights under the prefix. Some options:
- assign provisionally and only commit the key if ingest returns
Ok(the deserializer would see a tentative key); - an allowlist or validator hook on
.key(); - at minimum, document it next to
.key()and in 055, and surfacedroppedprominently.
Generated by Claude Code
| // Mutual exclusion with local producers (.source()/.transform()) is | ||
| // validated once, in build(), where the record key is known. | ||
| // The `{…}` syntax; the connector checks the rest. | ||
| let pattern = crate::TopicPattern::parse(url.resource_id()).map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
(nit) Behavior change for topics containing literal braces. { and } are legal MQTT topic characters (and legal for KNX, WS and the other connectors). Before this PR, link_from("mqtt://a/{b}") subscribed the literal topic a/{b}. Now it's a capture subscribed as a/+, and a/x} or a/{{b}} fail the build, with no escape syntax.
Outbound is also asymmetric: link_to (line ~908) only rejects when parsing succeeds with captures, so link_to("mqtt://a/x}") is accepted as a literal, while the same topic inbound is rejected.
Maybe worth a CHANGELOG line, and either an escape (e.g. {{ → {) or consistent handling in both directions.
Generated by Claude Code
|
|
||
| /// A capture value's key, assigned the first time the value is seen. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] | ||
| pub struct KeyId(NonZeroU16); |
There was a problem hiding this comment.
(nit) A KeyId carries no record identity, so a key from record A passed to inbound_key_name("B", key) quietly resolves to B's value with the same index. The same happens across databases. That's fine for a dense u16, but a sentence on KeyId ("only meaningful for the record whose link produced it") would help.
Generated by Claude Code
Description
Implements design 055: one inbound link with a topic pattern feeds many topics into one record.
How it is split. Core owns the
{name}/{name..}syntax, capture numbering, keys and routing. Each connector owns its wildcard rules via aTopicGrammar, which compiles a pattern into a matcher and decides which filters cover others. This revises the spike's shape, where core held one matcher for every protocol (055 §3.3).aimdb-core
TopicPattern, and theTopicGrammar/TopicFiltertraits.ExactGrammaris for connectors without wildcards.with_match_deserializerreceives aTopicMatchborrowed from the router's stack (topic(),get(name),key()) and a borrowed&RuntimeContext..key(name, capacity): one key table per record, shared by its keyed links. The table grows as values arrive. When it is full, the message is dropped and counted.inbound_key_nameresolves a key.RecordMetadata::inbound_keysreports captures, capacity, assigned and dropped.AimDb::inbound_router(scheme, grammar)compiles every link on a scheme, including patterns a topic resolver returns. It reports every link it cannot compile at once.Router::subscriptions()lists the filters to subscribe, leaving out those another filter covers.aimdb-mqtt-connector
MqttGrammarimplements MQTT 3.1.1 §4.7:+/{name}match one level,#/{name..}the rest (last only), and a leading wildcard does not match a$…topic.+/#topic now matches. Before, it subscribed but never delivered.Breaking: one inbound path
collect_inbound_routes,RouterBuilder,Routeand the publicRouter::new. ARouternow only comes frominbound_router.IngestFnreceives theTopicMatch.pump_source(db, router, src)andpump_client(db, scheme, router, handle)take the router, so a connector subscribes and routes with the same one.InboundConnectorLinkgainskey,RecordMetadatagainsinbound_keys, and both become#[non_exhaustive].ExactGrammar: a{…}link on them now fails the build instead of being skipped.Out of scope (055 §2)
with_qosdoc no longer claims otherwise.Allocations (
b0_alloc_connector, 64 routes)The existing rows are identical to the committed baseline.
Tests
MqttGrammar: the §4.7 examples, captures at the first, middle and last level,{rest..}matching zero levels, and covering, including$topics.inbound_routererrors (grammar rejection, invalid resolver pattern, keyed capture missing after resolution), shared keys, and key metadata.parity/+/in; each record receives the message exactly once; the capture and key reach the deserializer.Related Issue
Checklist
make check): targeted clippy and test runs per affected crate and feature leg passed locally; the full matrix runs in CI.