Extract shared welcome accept and catch-up helpers#495
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new welcome acceptance and catch-up flow in the pika-marmot-runtime, adding AcceptedWelcome struct and accept_welcome_and_catch_up function that supports optional message backfilling. The sidecar daemon integrates this new runtime helper, replacing direct welcome acceptance with subscription and backfill capabilities. Supporting test coverage is added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Daemon as Pikachat Daemon
participant Marmot as Pika-Marmot Runtime
participant MDK
participant Relay as Relay (Optional)
participant Storage
Client->>Daemon: Receive giftwrapped welcome event
Daemon->>Marmot: accept_welcome_and_catch_up(mdk, client, relay_urls, welcome, after_accept)
activate Marmot
Marmot->>MDK: Extract welcome & create group
MDK->>Storage: Store MLS group state
Storage-->>MDK: ✓ Group created with mls_group_id
MDK-->>Marmot: IngestedWelcome
alt Relay URLs provided
Marmot->>Relay: Query for backlog messages (limit: 2048)
Relay-->>Marmot: Historical messages from relay
Marmot->>Storage: Ingest backlog messages
Storage-->>Marmot: ✓ Messages stored
else No Relay URLs
Note over Marmot: Skip backfill, proceed with hook
end
Marmot->>Marmot: Construct AcceptedWelcome with all fields
deactivate Marmot
Marmot->>Daemon: Return AcceptedWelcome(wrapper_event_id, welcome_event_id, nostr_group_id_hex, mls_group_id, group_name, ingested_messages)
Daemon->>Daemon: after_accept hook executes
Daemon->>Client: Subscribe to group messages (nostr_group_id_hex)
Client-->>Daemon: ✓ Subscription active
Daemon->>Daemon: Emit GroupJoined, WelcomeReceived events
Daemon->>Daemon: Track and publish member counts
Daemon-->>Client: Group accepted and synced
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| mdk.accept_welcome(welcome).context("accept welcome")?; | ||
| after_accept(&accepted).await?; | ||
|
|
||
| if !relay_urls.is_empty() { | ||
| accepted.ingested_messages = ingest_group_backlog( | ||
| mdk, | ||
| client, | ||
| relay_urls, | ||
| &accepted.nostr_group_id_hex, | ||
| seen, | ||
| limit, | ||
| ) | ||
| .await | ||
| .context("ingest accepted welcome backlog")?; |
There was a problem hiding this comment.
🔴 Subscription leak and misleading error on backlog fetch failure after successful welcome accept
In accept_welcome_and_catch_up (crates/pika-marmot-runtime/src/lib.rs:206-207), mdk.accept_welcome() is called and succeeds before after_accept and ingest_group_backlog. If the backlog fetch at line 210-219 fails (e.g., network timeout), the ? propagates the error, causing the entire function to return Err. In the daemon caller (crates/pikachat-sidecar/src/daemon.rs:2618), this triggers the error branch which sends an mdk_error reply to the client — but the welcome was already irrevocably accepted in MLS state. This causes two problems: (1) the client is told the accept failed and may retry (which will then fail because the welcome is already consumed), and (2) the relay subscription created in the after_accept hook (stored in the subscribed_group mutex at line 2555) is never .take()-ed and inserted into group_subs (line 2574 is only reached on Ok), so the subscription leaks — it remains active on the relay but is untracked for cleanup.
Prompt for agents
In crates/pika-marmot-runtime/src/lib.rs, the accept_welcome_and_catch_up function at lines 206-219 should not propagate errors from ingest_group_backlog as hard failures since the welcome has already been irrevocably accepted at line 206. The backlog fetch error should be logged/warned but the function should still return Ok(accepted) with an empty ingested_messages list, or alternatively the AcceptedWelcome struct should include an optional backlog error field. Additionally, in crates/pikachat-sidecar/src/daemon.rs lines 2618-2620, the error branch should check the subscribed_group mutex and clean up (unsubscribe or add to group_subs) any subscription that was created in the after_accept hook before reporting the error.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
rust/src/core/mod.rs (1)
3907-3929:⚠️ Potential issue | 🟠 MajorRoute core welcome handling through the shared runtime helper.
This branch still does inline
process_welcome/accept_welcome, so core does not pick up the shared accept+catch-up flow this PR introduced. That leaves core/sidecar lifecycle drift in place and can still miss pre-subscription backlog for newly joined groups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/core/mod.rs` around lines 3907 - 3929, Core currently calls sess.mdk.accept_welcome(&welcome) inline, bypassing the shared runtime helper that runs accept+catch-up; replace this inline accept_welcome usage with a call into the shared "process_welcome" runtime helper (the new accept+catch-up wrapper), passing the same session/context and welcome so core uses the shared accept+catch-up flow; preserve the existing error handling (tracing::error and self.toast) and keep the subsequent key-package rotation (referenced_key_package_event_id, delete_event_best_effort, ensure_key_package_published_best_effort) and refresh_all_from_storage calls.
🧹 Nitpick comments (2)
rust/src/core/mod.rs (1)
8048-8145: Don’t lock this test to eager acceptance timing.The new test asserts that a welcome must leave no pending entry immediately on receipt, which hard-codes today’s core-specific eager flow. If core is migrated to the shared helper next, this characterization test will fail even if the real invariant still holds. Prefer asserting “no duplicate active group / no ratchet reset” without depending on immediate pending-state emptiness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/core/mod.rs` around lines 8048 - 8145, Test gift_wrap_received_accepts_immediately_and_skips_duplicate_pending_state is locked to an “eager accept” timing by asserting get_pending_welcomes(...) is empty immediately after the first handle_internal call; remove that strict assertion and instead only assert the invariant that no duplicate active group is created and no ratchet/reset occurs on re-delivery. Concretely: delete the assert block that checks get_pending_welcomes(...).is_empty() right after the first InternalEvent::GiftWrapReceived, keep the check that groups.len() == 1 after first delivery, and retain the subsequent re-delivery assertions (groups.len() still == 1 and no duplicate active group). If you can access a ratchet/state identifier on the group (via the mdk API), add an assertion that its identifier/state did not change across deliveries; otherwise do not assert immediate pending-list emptiness.crates/pika-marmot-runtime/src/lib.rs (1)
206-219: Partial completion on backlog failure may complicate retry logic.If
after_acceptoringest_group_backlogfails, the welcome has already been accepted (line 206), but the function returns an error and the caller does not receiveAcceptedWelcome. A subsequent retry by the caller would fail since the welcome is no longer pending in MDK.If this is intentional (callers should handle the error and query for the accepted group separately), consider documenting it in the docstring. Otherwise, consider returning
AcceptedWelcomeeven when backfill fails so the caller knows the acceptance succeeded:💡 Alternative: non-fatal backfill approach
if !relay_urls.is_empty() { - accepted.ingested_messages = ingest_group_backlog( + // Backfill failure shouldn't prevent returning the accepted welcome. + if let Ok(msgs) = ingest_group_backlog( mdk, client, relay_urls, &accepted.nostr_group_id_hex, seen, limit, ) - .await - .context("ingest accepted welcome backlog")?; + .await + { + accepted.ingested_messages = msgs; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pika-marmot-runtime/src/lib.rs` around lines 206 - 219, The current flow accepts the welcome via mdk.accept_welcome(welcome) then calls after_accept(&accepted).await and ingest_group_backlog(...).await, but if those post-accept steps fail the function returns an error and the caller never receives the AcceptedWelcome; change the function so acceptance is final and the AcceptedWelcome is returned even when after_accept or ingest_group_backlog fail: catch errors from after_accept and ingest_group_backlog (wrap their .await calls in match or .await.map_err handling), log or attach the error context to the returned AcceptedWelcome (e.g., set a field on accepted or return a tuple/struct containing AcceptedWelcome plus backfill error info) instead of propagating the error, ensuring mdk.accept_welcome(welcome) success always results in returning AcceptedWelcome to callers so retries don’t re-accept the same welcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@rust/src/core/mod.rs`:
- Around line 3907-3929: Core currently calls sess.mdk.accept_welcome(&welcome)
inline, bypassing the shared runtime helper that runs accept+catch-up; replace
this inline accept_welcome usage with a call into the shared "process_welcome"
runtime helper (the new accept+catch-up wrapper), passing the same
session/context and welcome so core uses the shared accept+catch-up flow;
preserve the existing error handling (tracing::error and self.toast) and keep
the subsequent key-package rotation (referenced_key_package_event_id,
delete_event_best_effort, ensure_key_package_published_best_effort) and
refresh_all_from_storage calls.
---
Nitpick comments:
In `@crates/pika-marmot-runtime/src/lib.rs`:
- Around line 206-219: The current flow accepts the welcome via
mdk.accept_welcome(welcome) then calls after_accept(&accepted).await and
ingest_group_backlog(...).await, but if those post-accept steps fail the
function returns an error and the caller never receives the AcceptedWelcome;
change the function so acceptance is final and the AcceptedWelcome is returned
even when after_accept or ingest_group_backlog fail: catch errors from
after_accept and ingest_group_backlog (wrap their .await calls in match or
.await.map_err handling), log or attach the error context to the returned
AcceptedWelcome (e.g., set a field on accepted or return a tuple/struct
containing AcceptedWelcome plus backfill error info) instead of propagating the
error, ensuring mdk.accept_welcome(welcome) success always results in returning
AcceptedWelcome to callers so retries don’t re-accept the same welcome.
In `@rust/src/core/mod.rs`:
- Around line 8048-8145: Test
gift_wrap_received_accepts_immediately_and_skips_duplicate_pending_state is
locked to an “eager accept” timing by asserting get_pending_welcomes(...) is
empty immediately after the first handle_internal call; remove that strict
assertion and instead only assert the invariant that no duplicate active group
is created and no ratchet/reset occurs on re-delivery. Concretely: delete the
assert block that checks get_pending_welcomes(...).is_empty() right after the
first InternalEvent::GiftWrapReceived, keep the check that groups.len() == 1
after first delivery, and retain the subsequent re-delivery assertions
(groups.len() still == 1 and no duplicate active group). If you can access a
ratchet/state identifier on the group (via the mdk API), add an assertion that
its identifier/state did not change across deliveries; otherwise do not assert
immediate pending-list emptiness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09611c12-f6a0-4218-b88b-3f7ea3c5c8c8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
cli/src/main.rscrates/pika-marmot-runtime/src/lib.rscrates/pikachat-sidecar/Cargo.tomlcrates/pikachat-sidecar/src/daemon.rsrust/src/core/mod.rs
1 participant
Summary
pika-marmot-runtimefor reusepika-marmot-runtime, reducing duplication between sidecar daemon and coreNet: ~590 lines added, ~50 removed — consolidating welcome/key-package handling into the shared runtime crate.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests