Make inbound/convergence processing incremental, bounded, non-blocking, single-source (#736)#768
Merged
Merged
Conversation
…ff-critical-path idempotency fsync Part of the #736 inbound/convergence boundary-contracts campaign (cheap correctness / non-blocking wins). - #637: drain take_pending_convergence_groups() after the deferred-startup replay loop so buffered convergence groups are scheduled before steady state instead of stranded until the next unrelated command/event. - #695: replace the per-event full transport resync with a routes-dirty flag + a single coalesced resync after the batch drains (both drain_pending_session_events and observe_account_device_effects). - #760: collect kind-448 push-gossip ids into a HashSet and retain once after the loop (O(n^2) -> O(n)). - #691: move SendIdempotencyStore::persist_to_disk off the async send hot path via spawn_blocking (at-least-once durability; inline fallback outside a runtime). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… drop pre-auth O(groups) scan (#740) Part of the #736 campaign (boundary contract 4: transport routing). Unknown transport_group_ids previously forced a list_groups() + per-group MlsGroup::load scan BEFORE payload authentication — attacker-paced CPU/IO. Add an authoritative in-memory transport_group_id -> GroupId index, populated at hydrate_stable_groups_from_storage and at group create/join (transport_group_id is immutable per group), so resolution is O(1) and a miss returns the direct id with no scan. (engine.rs also introduces the #636 seen_message_ids_hex_cache field used later in distributed_convergence.rs.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sor; bound lag replay (#630 #609 #747) Part of the #736 campaign (boundary contracts 1 & 2: storage ordering + runtime recovery). - #630: introduce AppEventReplayCursor = (recorded_at, message_id_hex, insert_order) in storage-sqlite with a matching APP_EVENT_REPLAY_ORDER_ASC/_DESC SQL fragment, so the recovery query order, the subscription watermark, and recovery_row_is_pre_subscription suppression are ONE definition and cannot drift. The local insert_order is the third field because the cursor is a per-client recovery cut-point (not the cross-client display order) and it is load-bearing for unscoped/all-groups recovery where message_id_hex is unique only per group. insert_order is surfaced on StoredAppMessageRecord / AppMessageRecord (not FFI). The frozen legacy migration reader keeps its own COALESCE order. - #609/#747: bound agent-connector replay_missed_inbound to the newest DELIVERED_INBOUND_CURSOR_CAPACITY rows instead of limit:None (full history). Tests: storage app_messages order == cursor comparator (incl. unscoped cross-group insert_order case); runtime same-second + insert_order suppression. (marmot-app/lib.rs also adds display_name_from_directory_entry used later by #639; account_projection.rs also adds the #762 batched component load.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ction cleanups (#636 #698 #752 #704 #750 #639 #761 #762) Part of the #736 campaign (boundary contracts 1 & 4 + incremental recompute). - #636: BoundedIdSet generation counter + engine seen_message_ids_hex_cache, so the convergence hex snapshot is re-encoded once per change, not per (up to 16) pass. - #698/#752: transport-nostr-adapter by_transport_group index + CanonicalEndpoint (cached parsed RelayUrl), rebuilt at activate/sync_groups/deactivate; routes_for is O(matching groups) with no double-parse and a constant-time miss. - #704: unread_summary_tx derives count + first-unread + mention_count from ONE ordered scan. - #750: chat_list_projection_meta marker (migration 0022) skips the per-group mention recompute on warm-up once counts are reconciled. - #639: notification_user fetches the directory entry once (display_name derived via display_name_from_directory_entry) instead of a duplicate lookup. - #761: public_directory_users batched into two queries (was 2N+1). - #762: account group components loaded in one bucketed query (was N+1). - Timeline display order consolidated into TIMELINE_ORDER_BY_ASC/_DESC, kept distinct from the raw-event replay cursor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…reated_at (#630 package E) Part of the #736 campaign (cross-client recorded_at consistency). The outer kind-445 envelope was signed with no custom_created_at, so it defaulted to wrap-time now(): receivers all agreed (shared envelope) but the sender's own copy used the inner event created_at, so a same-second pair could sort differently in the sender's view. Stamp the outer created_at with the inner app event's created_at (already carried in GroupMessageMetadata::Application) so the sender and every receiver record the same recorded_at. PROTOCOL-VISIBLE: changes the outer kind-445 event id (created_at is in the id preimage) and makes identical content broadcast to multiple groups share a timestamp (a minor, accepted correlation trade-off). message_id_hex is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ntracts (#736) Name the canonical derived-state owners (storage ordering surfaces, runtime recovery, convergence scheduling, transport routing, critical sections) in the app-runtime overview, keep the raw-event replay cursor distinct from the materialized-timeline display order, and note recorded_at is cross-client-stable once package E binds the outer created_at. Cross-link from distributed-convergence.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR bounds inbound replay, moves idempotency persistence off the async hot path, threads ChangesAgent-connector inbound replay and idempotency persistence
Replay cursor and insert order across storage and runtime
cgka-engine transport routing index and convergence caching
Client route refresh and buffered convergence scheduling
Display name resolution and notification caching
Chat-list mention counts and batched storage queries
Nostr transport adapter routing index and outer timestamp binding
Architecture documentation
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Ready to review this PR? Stage has broken it down into 9 individual chapters for you: 
Chapters generated by Stage for commit 2ccf265 on Jul 1, 2026 9:34pm UTC. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cgka-engine/src/message_processor/ingest.rs (1)
1354-1381: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winReject duplicate
transport_group_ids when populating the index
HashMap::insertsilently overwrites on collision, so a second group using the same 32-byte routing id will rebind inbound traffic to the newerGroupIdand make the earlier group stop resolving. Add a duplicate check or fail the insert on conflict instead of letting the last writer win.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cgka-engine/src/message_processor/ingest.rs` around lines 1354 - 1381, The transport_group_id_index population currently overwrites existing entries on collision, which can rebind inbound traffic to the wrong GroupId. Update the insertion path in the Engine::transport_group_id_index setup (and any helper that feeds it) to detect duplicate transport_group_id values and reject or preserve the first mapping instead of using HashMap::insert unconditionally. Make the conflict handling explicit so group_id_for_transport_group_id continues to resolve to the intended group without silent rebinding.
🧹 Nitpick comments (4)
crates/storage-sqlite/src/chat_list.rs (1)
686-706: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared
TIMELINE_ORDER_BY_ASCconstant instead of a duplicate literal.This scan's
ORDER BY timeline_at ASC, message_id_hex ASCis exactlytimeline.rs's newTIMELINE_ORDER_BY_ASC, introduced in this same PR specifically so non-aliasedORDER BYsites overmessage_timelineshare one definition. Using the constant here avoids the two drifting apart later.♻️ Proposed fix
let scan_sql = format!( "SELECT message_id_hex, plaintext, tags_json FROM message_timeline WHERE group_id_hex = ?1 AND kind = ?2 AND deleted = 0 AND invalidation_status IS NULL AND sender != ?3 AND {where_sql} - ORDER BY timeline_at ASC, message_id_hex ASC" + {}", + TIMELINE_ORDER_BY_ASC, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/storage-sqlite/src/chat_list.rs` around lines 686 - 706, Replace the duplicated ORDER BY literal in the unread scan inside chat_list.rs with the shared TIMELINE_ORDER_BY_ASC constant from timeline.rs. The scan built in the code path around scan_sql should reference that constant instead of spelling out timeline_at ASC, message_id_hex ASC directly, so all non-aliased message_timeline ordering stays centralized and consistent.crates/transport-nostr-adapter/src/lib.rs (2)
806-825: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding direct unit coverage for index invalidation.
The only supplied downstream test exercises
activatewith a single account/group. There's no test in the provided context asserting thatby_transport_groupentries are removed afterdeactivate, or correctly replaced aftersync_groupschanges a group'stransport_group_id/endpoints, or that two accounts sharing the sametransport_group_idboth resolve correctly. Given this index is now the sole gate for inboundGroupMessagedelivery, targeted tests here would guard against future regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/transport-nostr-adapter/src/lib.rs` around lines 806 - 825, Add targeted unit tests around the by_transport_group index in the routing state code, focusing on rebuild_transport_group_index and the mutation paths that call it. Cover that deactivate removes stale group entries, sync_groups replaces an updated group’s transport_group_id/endpoints correctly, and two accounts sharing the same transport_group_id are both retained and resolvable. Use the existing activate/deactivate/sync_groups flow and assert against by_transport_group so index invalidation is directly verified.
727-748: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate endpoint-matching logic between
CanonicalEndpoint::matchesandendpoints_match.
CanonicalEndpoint::matches(lines 739-747) reimplements the same byte-equal-then-parsed-RelayUrl-equal semantics as the free functionendpoints_match(lines 792-803), which is still used for theWelcomerouting arm. Two independent implementations of the same matching rule can silently drift if one is changed without the other.Consider having
CanonicalEndpoint::matchesdelegate toendpoints_matchfor the fallback comparison (or vice versa), so there is a single source of truth for endpoint equality semantics.Also applies to: 792-803
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/transport-nostr-adapter/src/lib.rs` around lines 727 - 748, `CanonicalEndpoint::matches` duplicates the endpoint equality rules already implemented in `endpoints_match`, so make the matching logic single-sourced by delegating one to the other. Update `CanonicalEndpoint::matches` to reuse `endpoints_match` (or refactor `endpoints_match` to call the method) while preserving the existing byte-equality fast path and parsed `RelayUrl` fallback, so the `Welcome` routing arm and `CanonicalEndpoint` stay consistent.crates/cgka-engine/src/group_lifecycle.rs (1)
226-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated transport-id indexing block into a shared helper.
This exact best-effort block is duplicated in
do_create_group,do_join_welcome, andEngine::hydrate_one_stored_group(engine.rs, lines 806-817). Consolidating into oneEnginemethod would remove the triplication and give a single place to evolve the indexing contract (e.g. collision handling, cleanup) later.♻️ Proposed helper
impl<S: StorageProvider> Engine<S> { /// Best-effort: index `group_id` under its transport routing id for O(1) /// inbound resolution (see `Engine::transport_group_id_index`). A /// routing-read failure only forfeits the fast path. fn index_transport_group_id(&mut self, group_id: &GroupId, mls_group: &openmls::group::MlsGroup) { if let Ok(transport_group_id) = crate::app_components::transport_group_id_of_group(mls_group) { self.transport_group_id_index .insert(transport_group_id, group_id.clone()); } } }Also applies to: 544-551
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cgka-engine/src/group_lifecycle.rs` around lines 226 - 235, The transport-group-id indexing logic is duplicated across group creation, welcome join, and hydration, so extract the repeated best-effort block into a shared Engine helper. Add a single method on Engine (for example, index_transport_group_id) that takes group_id and mls_group, calls transport_group_id_of_group, and inserts into transport_group_id_index on success. Then replace the inline blocks in do_create_group, do_join_welcome, and Engine::hydrate_one_stored_group with calls to that helper so the indexing contract lives in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/agent-connector/src/inbound.rs`:
- Around line 239-251: The bounded replay in `InboundStream` is not sufficient
when `Lagged(dropped)` is larger than `DELIVERED_INBOUND_CURSOR_CAPACITY`,
because only the newest suffix is replayed and older missed messages remain
omitted. Update the lag-recovery path in `inbound.rs` (around the
`AppMessageQuery`/replay handling) to detect when the dropped count exceeds the
available replay window and emit `resync_required` instead of completing the
replay normally. Use the existing `Lagged` handling and
`DELIVERED_INBOUND_CURSOR_CAPACITY` check to decide when the window is
exhausted, and keep the normal replay path unchanged for smaller gaps.
In `@crates/cgka-engine/src/engine.rs`:
- Around line 183-205: `transport_group_id_index` is populated for inbound
routing but is never cleared when a group is removed, so stale mappings can
survive after `StorageProvider::delete_group`. Update the group deletion path in
`Engine` to remove the corresponding `transport_group_id_index` entry whenever a
group is deleted, using the same `GroupId`/transport-id lookup used by
`hydrate_one_stored_group`, `do_create_group`, and `do_join_welcome` to keep
routing resolution in sync with storage.
In `@docs/marmot-architecture/overview/marmot-app-runtime.md`:
- Around line 76-84: The storage surface name is wrong in this section:
`AppEventReplayCursor` is used by `SqliteAccountStorage::app_messages`, not
`app_events`, so update the wording to reference the implementation’s actual
table/surface name. Keep the rest of the explanation about replay ordering and
`insert_order` unchanged, but make sure the terms around `AppEventReplayCursor`
and `SqliteAccountStorage::app_messages` are aligned so readers don’t confuse it
with the timeline surface.
---
Outside diff comments:
In `@crates/cgka-engine/src/message_processor/ingest.rs`:
- Around line 1354-1381: The transport_group_id_index population currently
overwrites existing entries on collision, which can rebind inbound traffic to
the wrong GroupId. Update the insertion path in the
Engine::transport_group_id_index setup (and any helper that feeds it) to detect
duplicate transport_group_id values and reject or preserve the first mapping
instead of using HashMap::insert unconditionally. Make the conflict handling
explicit so group_id_for_transport_group_id continues to resolve to the intended
group without silent rebinding.
---
Nitpick comments:
In `@crates/cgka-engine/src/group_lifecycle.rs`:
- Around line 226-235: The transport-group-id indexing logic is duplicated
across group creation, welcome join, and hydration, so extract the repeated
best-effort block into a shared Engine helper. Add a single method on Engine
(for example, index_transport_group_id) that takes group_id and mls_group, calls
transport_group_id_of_group, and inserts into transport_group_id_index on
success. Then replace the inline blocks in do_create_group, do_join_welcome, and
Engine::hydrate_one_stored_group with calls to that helper so the indexing
contract lives in one place.
In `@crates/storage-sqlite/src/chat_list.rs`:
- Around line 686-706: Replace the duplicated ORDER BY literal in the unread
scan inside chat_list.rs with the shared TIMELINE_ORDER_BY_ASC constant from
timeline.rs. The scan built in the code path around scan_sql should reference
that constant instead of spelling out timeline_at ASC, message_id_hex ASC
directly, so all non-aliased message_timeline ordering stays centralized and
consistent.
In `@crates/transport-nostr-adapter/src/lib.rs`:
- Around line 806-825: Add targeted unit tests around the by_transport_group
index in the routing state code, focusing on rebuild_transport_group_index and
the mutation paths that call it. Cover that deactivate removes stale group
entries, sync_groups replaces an updated group’s transport_group_id/endpoints
correctly, and two accounts sharing the same transport_group_id are both
retained and resolvable. Use the existing activate/deactivate/sync_groups flow
and assert against by_transport_group so index invalidation is directly
verified.
- Around line 727-748: `CanonicalEndpoint::matches` duplicates the endpoint
equality rules already implemented in `endpoints_match`, so make the matching
logic single-sourced by delegating one to the other. Update
`CanonicalEndpoint::matches` to reuse `endpoints_match` (or refactor
`endpoints_match` to call the method) while preserving the existing
byte-equality fast path and parsed `RelayUrl` fallback, so the `Welcome` routing
arm and `CanonicalEndpoint` stay consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1068a11-db34-4122-8df4-8d8bf1cb0cc9
📒 Files selected for processing (32)
crates/agent-connector/src/inbound.rscrates/agent-connector/src/stream_session.rscrates/agent-connector/src/tests.rscrates/cgka-engine/src/bounded_id_set.rscrates/cgka-engine/src/distributed_convergence.rscrates/cgka-engine/src/engine.rscrates/cgka-engine/src/group_lifecycle.rscrates/cgka-engine/src/message_processor/ingest.rscrates/cli/src/lib.rscrates/marmot-app/src/client/sync.rscrates/marmot-app/src/conversions.rscrates/marmot-app/src/lib.rscrates/marmot-app/src/notifications.rscrates/marmot-app/src/projection.rscrates/marmot-app/src/runtime/account_worker.rscrates/marmot-app/src/runtime/subscriptions.rscrates/marmot-app/src/runtime/tests.rscrates/marmot-uniffi/src/conversions/media.rscrates/marmot-uniffi/src/conversions/mod.rscrates/storage-sqlite/src/account_projection.rscrates/storage-sqlite/src/account_projection/tests.rscrates/storage-sqlite/src/chat_list.rscrates/storage-sqlite/src/lib.rscrates/storage-sqlite/src/migrations.rscrates/storage-sqlite/src/migrations/0022_chat_list_projection_version.rscrates/storage-sqlite/src/shared.rscrates/storage-sqlite/src/timeline.rscrates/traits/src/peeler.rscrates/transport-nostr-adapter/src/lib.rscrates/transport-nostr-peeler/src/peeler.rsdocs/marmot-architecture/distributed-convergence.mddocs/marmot-architecture/overview/marmot-app-runtime.md
…ison-5d6e39 # Conflicts: # crates/storage-sqlite/src/lib.rs # crates/storage-sqlite/src/migrations.rs
- inbound: when broadcast lag exceeds the bounded replay window (DELIVERED_INBOUND_CURSOR_CAPACITY), emit resync_required instead of a necessarily-partial newest-window replay, so older missed messages are not silently omitted (CodeRabbit Major, data integrity). - engine: strengthen the transport_group_id_index invariant doc — the engine has no group-deletion path today and a stale entry is self-correcting, but a future deleter/routing-rotation site MUST update the index (CodeRabbit). - docs: point the raw-event replay cursor at the `app_events` table queried via SqliteAccountStorage::app_messages (CodeRabbit). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/storage-sqlite/src/migrations.rs (1)
516-541: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a schema-level test for migration 0023 too.
A dedicated test was added for migration 0022 (
chat_list_self_membership_migration_adds_member_defaulted_column), following the pattern used for other migrations in this file (0018, 0019). No equivalent test was added here for migration 0023'schat_list_projection_metatable creation/seed row, even though it's a new table with a singleton-row invariant (CHECK (id = 1)) worth regression-testing directly at this layer, independent of the downstreamchat_list.rsconsumer tests.♻️ Suggested test to add alongside the 0022 test
+ #[test] + fn chat_list_projection_version_migration_seeds_default_row() { + let store = SqliteAccountStorage::in_memory().unwrap(); + let conn = store.lock().unwrap(); + let version: i64 = conn + .query_row( + "SELECT mention_counts_version FROM chat_list_projection_meta WHERE id = 1", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(version, 0); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/storage-sqlite/src/migrations.rs` around lines 516 - 541, Add a schema-level regression test for migration 0023 alongside chat_list_self_membership_migration_adds_member_defaulted_column, using the same run/MIGRATIONS pattern. The new test should verify that chat_list_projection_meta is created, the seed row exists after running migrations, and the singleton-row invariant enforced by CHECK (id = 1) is preserved. Reference the migration setup helpers in migrations.rs and the chat_list_projection_meta table/migration 0023 when adding the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/storage-sqlite/src/migrations.rs`:
- Around line 516-541: Add a schema-level regression test for migration 0023
alongside chat_list_self_membership_migration_adds_member_defaulted_column,
using the same run/MIGRATIONS pattern. The new test should verify that
chat_list_projection_meta is created, the seed row exists after running
migrations, and the singleton-row invariant enforced by CHECK (id = 1) is
preserved. Reference the migration setup helpers in migrations.rs and the
chat_list_projection_meta table/migration 0023 when adding the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b67fa56-55bd-4254-9169-c7be68132b1f
📒 Files selected for processing (14)
crates/agent-connector/src/inbound.rscrates/cgka-engine/src/engine.rscrates/marmot-app/src/client/sync.rscrates/marmot-app/src/conversions.rscrates/marmot-app/src/lib.rscrates/marmot-app/src/runtime/tests.rscrates/marmot-uniffi/src/conversions/mod.rscrates/storage-sqlite/src/account_projection.rscrates/storage-sqlite/src/account_projection/tests.rscrates/storage-sqlite/src/chat_list.rscrates/storage-sqlite/src/lib.rscrates/storage-sqlite/src/migrations.rscrates/storage-sqlite/src/migrations/0023_chat_list_projection_version.rsdocs/marmot-architecture/overview/marmot-app-runtime.md
💤 Files with no reviewable changes (1)
- crates/storage-sqlite/src/migrations/0023_chat_list_projection_version.rs
✅ Files skipped from review due to trivial changes (1)
- docs/marmot-architecture/overview/marmot-app-runtime.md
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/marmot-uniffi/src/conversions/mod.rs
- crates/marmot-app/src/conversions.rs
- crates/storage-sqlite/src/lib.rs
- crates/storage-sqlite/src/account_projection/tests.rs
- crates/marmot-app/src/runtime/tests.rs
- crates/cgka-engine/src/engine.rs
- crates/marmot-app/src/client/sync.rs
- crates/marmot-app/src/lib.rs
- crates/storage-sqlite/src/chat_list.rs
- crates/storage-sqlite/src/account_projection.rs
…versarial review) Two P1 findings: the transport routing index and the app-side subscription refresh could not handle an in-place Nostr routing-component rotation, which the protocol allows (local UpdateAppComponents and remote admin commits both reach routing via the generic app-component update path). Engine (Finding 1 — resolver went stale, a regression vs the old live-scan): - Add Engine::reindex_transport_group_id, called at every commit-apply site (do_confirm_published, remote-commit ingest, convergence apply) to additively re-insert the group's CURRENT transport_group_id. The prior id is left mapped so inbound messages still addressed to the pre-rotation route during the overlap window keep resolving (the map is intentionally many-to-one, matching the spec's "publish to the prior routing address" model). Runs on commit application only, not per app message, so the extra MlsGroup::load is cheap. - Regression test: after a rotation X->Y is applied, a later commit addressed to the NEW nostr_group_id resolves to the group instead of a phantom direct id. App (Finding 2 — pre-existing; the app never resubscribed on a routing change): - AppTransportRouting::add_group now replaces by group_id (was early-return on a duplicate) and reports whether the route set changed. - refresh_group_routes returns whether any route was added/modified; the receive path runs it once per batch and resyncs when the membership count OR a route changed (a routing/relay rotation emits no GroupStateChanged event, so the count-only trigger missed it). - Unit test: add_group replaces (not duplicates) a group's route on rotation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
Author
Follow-up: end-to-end
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cgka-engine/src/engine.rs (1)
637-707: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMove
reindex_transport_group_idsohydrate_stable_groups_from_storagekeeps its doc comment
crates/cgka-engine/src/engine.rs: the long crash-recovery doc block attaches toreindex_transport_group_id, leavinghydrate_stable_groups_from_storageundocumented. Move the helper elsewhere or give it its own doc comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cgka-engine/src/engine.rs` around lines 637 - 707, The crash-recovery doc comment is currently attached to reindex_transport_group_id instead of hydrate_stable_groups_from_storage, so move the helper out of the way or add a separate doc comment to reindex_transport_group_id and keep the existing recovery documentation with hydrate_stable_groups_from_storage. Use the function names hydrate_stable_groups_from_storage and reindex_transport_group_id to place the comment on the intended API without changing behavior.
🧹 Nitpick comments (1)
crates/cgka-engine/src/distributed_convergence.rs (1)
385-395: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGate the reindex call on
selected_tip.is_some()to avoid a redundantMlsGroup::loadon every no-op convergence pass.
reindex_transport_group_id(which does its ownMlsGroup::load) runs unconditionally right afterapply_openmls_canonicalization_result, even when this pass settles with nothing new applied (result.selected_tipisNone, i.e. theStablebranch below at line 443). Per the engine.rs doc forreindex_transport_group_id, the cost is justified because it "only fires on commit application, not per app message" — but a convergence drain calls this function up to 16 times per cycle, and most of those passes settle with no new commit. Moving the call inside theif let Some(selected_tip) = result.selected_tipblock keeps the reindex tied to an actual applied commit, matching the semantics documented at the call site, and avoids the extra deserialization on already-settled passes.♻️ Proposed fix
let observations = apply_openmls_canonicalization_result( &self.storage, group_id, &result, max_retained_anchor_rewind, )?; - // `#740` rotation: a routing-component update commit applied through - // convergence may have changed this group's nostr_group_id; additively - // refresh the transport-id index so it resolves on the inbound path - // (prior id retained for the overlap window). - self.reindex_transport_group_id(group_id); let origin_commit_id = single_accepted_commit_id(&result); let origin_commit_actor = single_accepted_commit_actor(&observations, origin_commit_id.as_ref()); if let Some(selected_tip) = result.selected_tip { let selected_tip = EpochId(selected_tip); self.epoch_manager .set_stable(group_id.clone(), selected_tip); + // `#740` rotation: a routing-component update commit applied through + // convergence may have changed this group's nostr_group_id; additively + // refresh the transport-id index so it resolves on the inbound path + // (prior id retained for the overlap window). + self.reindex_transport_group_id(group_id);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cgka-engine/src/distributed_convergence.rs` around lines 385 - 395, The unconditional reindex in distributed_convergence::apply_openmls_canonicalization_result should only run when a new commit was actually applied. Move the self.reindex_transport_group_id(group_id) call into the existing if let Some(selected_tip) = result.selected_tip branch, so it is gated on selected_tip being present and skipped for Stable/no-op convergence passes. Use the surrounding apply_openmls_canonicalization_result and result.selected_tip logic to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/marmot-app/src/lib.rs`:
- Around line 2788-2808: The add_group path in TransportGroupState currently
overwrites an existing TransportGroupSubscription immediately, which causes the
runtime sync in the app to drop the old route before the overlap window can
finish. Update add_group (and the related group_routes sync logic) so a
transport rotation keeps the prior subscription active alongside the new one
until the overlap window closes, instead of replacing it in place. Use the
existing group_id and TransportGroupSubscription handling in add_group as the
entry point for preserving both routes during the transition.
---
Outside diff comments:
In `@crates/cgka-engine/src/engine.rs`:
- Around line 637-707: The crash-recovery doc comment is currently attached to
reindex_transport_group_id instead of hydrate_stable_groups_from_storage, so
move the helper out of the way or add a separate doc comment to
reindex_transport_group_id and keep the existing recovery documentation with
hydrate_stable_groups_from_storage. Use the function names
hydrate_stable_groups_from_storage and reindex_transport_group_id to place the
comment on the intended API without changing behavior.
---
Nitpick comments:
In `@crates/cgka-engine/src/distributed_convergence.rs`:
- Around line 385-395: The unconditional reindex in
distributed_convergence::apply_openmls_canonicalization_result should only run
when a new commit was actually applied. Move the
self.reindex_transport_group_id(group_id) call into the existing if let
Some(selected_tip) = result.selected_tip branch, so it is gated on selected_tip
being present and skipped for Stable/no-op convergence passes. Use the
surrounding apply_openmls_canonicalization_result and result.selected_tip logic
to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a212800e-9265-4934-abc3-d8223783070d
📒 Files selected for processing (9)
crates/cgka-engine/src/distributed_convergence.rscrates/cgka-engine/src/engine.rscrates/cgka-engine/src/message_processor/ingest.rscrates/cgka-engine/src/publish.rscrates/cgka-engine/tests/update_group_data.rscrates/marmot-app/src/client/mod.rscrates/marmot-app/src/client/sync.rscrates/marmot-app/src/lib.rscrates/marmot-app/src/tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/marmot-app/src/client/sync.rs
Comment on lines
+2788
to
2808
| /// Insert or replace the route for a group, returning whether the route set | ||
| /// changed. Replaces by `group_id` (rather than early-returning on a | ||
| /// duplicate) so an in-place `nostr_group_id` / relay rotation on an existing | ||
| /// group actually switches the subscription (Finding 2). Returns `false` | ||
| /// when the group's subscription is already present and identical. | ||
| fn add_group(&self, group: TransportGroupSubscription) -> bool { | ||
| let mut state = self.write(); | ||
| if state | ||
| if let Some(existing) = state | ||
| .group_routes | ||
| .iter() | ||
| .any(|existing| existing.group_id == group.group_id) | ||
| .iter_mut() | ||
| .find(|existing| existing.group_id == group.group_id) | ||
| { | ||
| return; | ||
| if *existing == group { | ||
| return false; | ||
| } | ||
| *existing = group; | ||
| return true; | ||
| } | ||
| state.group_routes.push(group); | ||
| true | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: check TransportGroupSubscription derives, and how group_routes /
# transport_group_id feed relay subscription filters in the adapter/runtime.
rg -n -B3 'struct TransportGroupSubscription' crates/traits/src/transport_adapter.rs
rg -n -C5 'group_routes' crates/marmot-app/src/runtime/*.rs crates/marmot-app/src/client/*.rs
rg -n -C5 'transport_group_id' crates/transport-nostr-adapter/src/lib.rsRepository: marmot-protocol/darkmatter
Length of output: 10466
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the app-side route refresh and runtime sync path.
sed -n '1938,1995p' crates/marmot-app/src/client/mod.rs
printf '\n---\n'
sed -n '1,220p' crates/marmot-app/src/client/sync.rs
# Inspect the adapter state built from group routes and how subscriptions are derived.
sed -n '740,835p' crates/transport-nostr-adapter/src/lib.rs
printf '\n---\n'
sed -n '970,1005p' crates/transport-nostr-adapter/src/lib.rs
# Inspect the engine overlap-window logic mentioned in the comment.
rg -n -C6 'reindex_transport_group_id|overlap window|transport_group_id' crates -g '*.rs'Repository: marmot-protocol/darkmatter
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow the engine-side rotation behavior.
rg -n -C8 'reindex_transport_group_id|overlap window' crates -g '*.rs'Repository: marmot-protocol/darkmatter
Length of output: 12257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the app-side route refresh and runtime sync path.
sed -n '1938,1995p' crates/marmot-app/src/client/mod.rs
printf '\n---\n'
sed -n '1,220p' crates/marmot-app/src/client/sync.rs
# Inspect the adapter state built from group routes and how subscriptions are derived.
sed -n '740,835p' crates/transport-nostr-adapter/src/lib.rs
printf '\n---\n'
sed -n '970,1005p' crates/transport-nostr-adapter/src/lib.rs
# Inspect the engine overlap-window logic mentioned in the comment.
rg -n -C8 'reindex_transport_group_id|overlap window' crates -g '*.rs'Repository: marmot-protocol/darkmatter
Length of output: 29927
Preserve the old route through transport rotation
add_group replaces the stored TransportGroupSubscription immediately, and the app/runtime sync path rebuilds relay subscriptions from the current group_routes set. That unsubscribes the old transport_group_id/endpoints before the engine’s overlap window can resolve late traffic, so publishes to the prior route can be dropped. Keep the previous route around until the overlap window closes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/marmot-app/src/lib.rs` around lines 2788 - 2808, The add_group path in
TransportGroupState currently overwrites an existing TransportGroupSubscription
immediately, which causes the runtime sync in the app to drop the old route
before the overlap window can finish. Update add_group (and the related
group_routes sync logic) so a transport rotation keeps the prior subscription
active alongside the new one until the overlap window closes, instead of
replacing it in place. Use the existing group_id and TransportGroupSubscription
handling in add_group as the entry point for preserving both routes during the
transition.
Completes the two partials flagged in review so their scope is fully covered: #698 / #752 — the group-message routing arm already used the cached-canonical `by_transport_group` index, but the Welcome/inbox arm still called `endpoints_match`, re-parsing both stored inbox endpoints AND the inbound endpoint on every event. Store inbox endpoints as pre-parsed `CanonicalEndpoint`s (built once at activate) and match via `CanonicalEndpoint::matches` with the inbound endpoint parsed once per event, mirroring the group arm. The now-unused `endpoints_match` free function is removed (its rationale moved onto `CanonicalEndpoint::matches`); the existing `welcome_event_routes_despite_trailing_slash_mismatch` regression test still guards normalization. #761 — `public_directory_users` collapsed the 2N+1 into two batched queries but still loaded the entire (network-populated) directory cache. Add a defensive `PUBLIC_DIRECTORY_USERS_MAX` (10_000) cap: the user query is `LIMIT`-bounded and the follows query is scoped to the same capped set via a matching subquery LIMIT, so neither materializes the whole cache. The cap sits well above the app-layer search reachability (`USER_DIRECTORY_SEARCH_MAX_VISITED` == 8192) so it never truncates realistic results, and a privacy-safe warn fires if it is ever hit (no silent cap). Extracted a `public_directory_users_capped(max)` helper so the bound is unit-testable without inserting 10k rows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/storage-sqlite/src/shared.rs (1)
340-376: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTruncation-cap check can false-positive when the count exactly equals
max.Both queries are already scoped with
LIMIT ?1(cap), sorecords.len()can never exceedmax— it can only equalmaxeither because truncation genuinely occurred, or because the directory happens to contain exactlymaxrows with nothing dropped.records.len() >= max(line 367) can't distinguish these two cases, so the warn can fire even when no truncation happened.🐛 Proposed fix: over-fetch by one to detect truncation precisely
- let cap = i64::try_from(max).unwrap_or(i64::MAX); + let cap = i64::try_from(max).unwrap_or(i64::MAX); + let probe_cap = cap.saturating_add(1); @@ let mut stmt = self .conn_ref(&conn) .prepare( "SELECT account_id_hex, npub, profile_json, relay_lists_json, key_package_json, event_id_hex, event_kind, event_created_at FROM directory_users ORDER BY account_id_hex LIMIT ?1", ) .storage()?; - let records = stmt - .query_map(params![cap], |row| { + let mut records = stmt + .query_map(params![probe_cap], |row| { Ok(PublicDirectoryUserRecord { account_id_hex: row.get(0)?, npub: row.get(1)?, profile_json: row.get(2)?, relay_lists_json: row.get(3)?, key_package_json: row.get(4)?, event_id_hex: row.get(5)?, event_kind: optional_i64_to_u64(row.get::<_, Option<i64>>(6)?), event_created_at: optional_i64_to_u64(row.get::<_, Option<i64>>(7)?), follows: Vec::new(), }) }) .storage()? .collect::<Result<Vec<_>, _>>() .storage()?; - if records.len() >= max { + if records.len() > max { + records.truncate(max); tracing::warn!( target: "storage_sqlite::shared", method = "public_directory_users", cap = max, "public directory listing hit the defensive cap; results truncated", ); }Note the follows query stays scoped to
cap(notprobe_cap) so it still matches the final truncated set of returned users.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/storage-sqlite/src/shared.rs` around lines 340 - 376, The truncation warning in public_directory_users can false-positive when the result count exactly matches the limit, because records.len() is only compared against max after querying with LIMIT cap. Update the listing logic in shared.rs to over-fetch by one using a probe cap in the query path around the stmt/query_map collection, then use that extra row to detect whether truncation really occurred before emitting the tracing::warn. Keep the follows lookup scoped to the final cap so it still matches the returned truncated set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/storage-sqlite/src/shared.rs`:
- Around line 340-376: The truncation warning in public_directory_users can
false-positive when the result count exactly matches the limit, because
records.len() is only compared against max after querying with LIMIT cap. Update
the listing logic in shared.rs to over-fetch by one using a probe cap in the
query path around the stmt/query_map collection, then use that extra row to
detect whether truncation really occurred before emitting the tracing::warn.
Keep the follows lookup scoped to the final cap so it still matches the returned
truncated set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 614ece42-d01b-4b10-a08e-3a9c85b20ff6
📒 Files selected for processing (2)
crates/storage-sqlite/src/shared.rscrates/transport-nostr-adapter/src/lib.rs
…(#639) Introduce a per-batch `NotificationResolver` that memoizes the notification- building lookups that repeat across the events drained in one `collect_notifications_after_wake` pass: account notification settings (keyed by account_label), the group record (keyed by account_label + group_id_hex), and directory-derived users (keyed by account_id_hex, shared by the receiver and every sender). Each backing lookup re-opens the per-account SQLCipher / directory caches and fans across accounts, so a busy multi-message batch previously re-paid that per message. - The resolver is created once and shared across the wake-drain loop; the live per-event notification stream keeps a throwaway resolver per event (via the unchanged `notification_update_from_event` wrapper), so it never serves stale settings/group/user across events. - Cached values are message-agnostic: `user` stores the raw directory result and the per-message sender-display-name fallback is applied to the returned clone in `notification_user_from_message`, never written back into the cache. - `reaction_target` stays per-message (keyed by a distinct reacted-to id). Completes the previously-partial #639 (the duplicate sender lookup was already folded). Adversarially reviewed for transparency, fallback isolation, staleness, and cross-account keying. Test: sender-display-name fallback is per-message, not cached. Closes #639 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/marmot-app/src/notifications.rs (1)
988-1013: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInvalidate cached group data on
GroupStateUpdated.Line 1009 still drops
GroupStateUpdatedwithout touchingresolver.groups. In a drained batch likeMessageReceived(group A) -> GroupStateUpdated(group A) -> MessageReceived(group A), the second notification will keep using the stale cachedAppGroupRecordinstead of the updated group state. Clear that cache entry before returningOk(None).Suggested fix
- MarmotAppEvent::GroupStateUpdated { .. } - | MarmotAppEvent::ProjectionUpdated(_) + MarmotAppEvent::GroupStateUpdated { + account_label, + group_id, + .. + } => { + resolver.groups.remove(&( + account_label.to_owned(), + hex::encode(group_id.as_slice()), + )); + Ok(None) + } + MarmotAppEvent::ProjectionUpdated(_) | MarmotAppEvent::AgentStreamStarted(_) | MarmotAppEvent::GroupEvent(_) | MarmotAppEvent::AccountError(_) => Ok(None),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/marmot-app/src/notifications.rs` around lines 988 - 1013, Invalidate the cached group entry when handling MarmotAppEvent::GroupStateUpdated in notification_update_from_event_cached, since the current match arm returns Ok(None) without touching resolver.groups and can leave stale AppGroupRecord data in later MessageReceived handling. Update that arm to remove/clear the affected group cache entry from NotificationResolver before returning, using the existing resolver.groups cache path and the group identifier available from the event payload or nearby cache lookup logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/marmot-app/src/notifications.rs`:
- Around line 988-1013: Invalidate the cached group entry when handling
MarmotAppEvent::GroupStateUpdated in notification_update_from_event_cached,
since the current match arm returns Ok(None) without touching resolver.groups
and can leave stale AppGroupRecord data in later MessageReceived handling.
Update that arm to remove/clear the affected group cache entry from
NotificationResolver before returning, using the existing resolver.groups cache
path and the group identifier available from the event payload or nearby cache
lookup logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a59b03d6-9fed-4264-9e91-8a0ec26fd2fd
📒 Files selected for processing (3)
crates/marmot-app/src/notifications.rscrates/marmot-app/src/notifications/tests.rscrates/marmot-app/src/runtime/mod.rs
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
1 participant
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.
Implements the bulk of tracking issue marmot-protocol/mdk#380 as five boundary contracts around the receive/convergence path (not one
InboundPipeline— the engine's live MLS state stays in the engine). Full workspace test suite +just fast-cigreen.Boundary contracts
storage-sqlite): a raw-event replay cursorAppEventReplayCursor = (recorded_at, message_id_hex, insert_order)+ shared SQL fragment, kept distinct from the materialized-timeline display order(timeline_at, message_id_hex). The replay cursor is a per-client recovery cut-point (localinsert_orderis correct + load-bearing for unscoped/all-groups recovery); it must never be applied to timeline pagination.marmot-app): the lag watermark +recovery_row_is_pre_subscriptionsuppression are the same cursor the recovery query orders by. Lag replay reads a bounded window; when the drop count exceeds the window it emitsresync_requiredrather than a partial replay.marmot-appworker): pending convergence groups drained at every loop entry, including deferred startup.cgka-engine+transport-nostr-adapter): in-memorytransport_group_idindexes built from authoritative state; no unauthenticated peer can force an O(groups) pre-auth scan. The engine index self-heals onnostr_group_idrotation (additive reindex at every commit-apply site, prior id retained for the overlap window), the app resubscribes on an in-place routing/relay change, and both the group and Welcome/inbox arms match against pre-canonicalized endpoints (parsed once).agent-connector): relay I/O / resync / idempotencyfsyncmoved off the per-event/per-command path.Plus per-batch memoization of notification-building lookups (#639) and a defensive bound on the public-directory listing (#761).
Corrections found while grounding the issues in code
!app_event_exists_txfull-rebuild gate is gone on currentmaster; a re-delivered whitelisted CHAT already takes the incremental path. Revalidated; not claimed by this PR.The peeler now binds the outer kind-445
created_atto the inner app event'screated_atso senders and receivers record the samerecorded_at. This changes the outer transport event id and makes identical content broadcast to multiple groups share a timestamp (a minor, accepted correlation trade-off).message_id_hexis unaffected.Scope: closed vs deferred
Fully addressed (closed by this PR):
Closes marmot-protocol/mdk#439
Closes marmot-protocol/mdk#444
Closes marmot-protocol/darkmatter#704
Closes marmot-protocol/mdk#415
Closes marmot-protocol/mdk#417
Closes marmot-protocol/mdk#435
Closes marmot-protocol/mdk#408
Closes marmot-protocol/mdk#406
Closes marmot-protocol/mdk#404
Closes marmot-protocol/mdk#436
Closes marmot-protocol/mdk#402
Closes marmot-protocol/mdk#414
Closes marmot-protocol/mdk#405
Closes marmot-protocol/mdk#403
Closes marmot-protocol/mdk#434
Deferred (tracked separately; not touched):
max_rewind_commits=5; the cheap companion Convergence re-hex-serializes the entireseen_message_idsset into a freshBTreeSet<String>on every pass mdk#436 ships here.workersmutex across relay I/O) — highest blast radius (CLI daemon concurrency model).Umbrella marmot-protocol/mdk#380 stays OPEN. Every in-scope child above is closed; the only remaining children are the two deliberately-deferred items (#635, marmot-protocol/mdk#438), so marmot-protocol/mdk#380 should track those rather than auto-close with this PR.
Test coverage / honest caveats
seen_message_idsset into a freshBTreeSet<String>on every pass mdk#436, package E (outer==innercreated_at),nostr_group_idrotation (engine inbound re-resolves the new id; appadd_groupreplaces not duplicates), storage-sqlite:public_directory_usersis an unbounded 2N+1 query that loads the whole directory cache into memory mdk#403 defensive cap (uncapped returns all; capped bounds + scopes follows), N+1 directory/group/profile lookups per inbound message on the receive hot path mdk#434 (sender-display-name fallback is per-message, not cached — adversarially reviewed for transparency/isolation/staleness/keying). transport-nostr-adapter:routes_for/endpoints_matchre-parses both relay URLs (doubleRelayUrl::parse) per stored endpoint, per inbound event, on the slow path mdk#414 inbox normalization is guarded by the existingwelcome_event_routes_despite_trailing_slash_mismatchregression (a parse-once perf refactor that preserves matching). Adapter/storage/engine/cli/uniffi suites green.marmot-apphas no mockTransportAdapterand those seams have no test hook, so they rely on the passingrelay_runtimeintegration suite + by-construction correctness. A counting mock-adapter harness would be a follow-up.🤖 Generated with Claude Code
Summary by CodeRabbit