Conversation
… Chat The sidecar never ran the LXMF client /get pull, so propagation-node inbox retrieval could not work: Sync only ran the /offer peer-replication path (PropagationSyncTask), and the "propagation-retrieve" telemetry on Sync Completes actually described peer-offer progress, not inbox retrieval. - Add propagation_download.rs: decrypt/unpack a downloaded propagated blob (dest || encrypted_data) with the local identity into an inbound LxMessage. - PropagationBridge now owns a PropagationClient; start_client_download / poll_client_download drive list -> get -> purge and return decoded messages; cancel_client_download for teardown. - live.rs::spawn_client_download_driver runs the /get download alongside the existing /offer peer sync and delivers each message through the router delivery callback, so WS lxmf_message, the recent ring, and renderer catch-up all fire unchanged. Cancel wired into cancel_propagation_sync. - local-prop Sync (previously a no-op) drains own-dest mail via PropagationBridge::drain_local_inbox (in-process node /get replay). - Telemetry: relabel peer-offer Completes as propagation-sync peer_outcome; real inbox retrieval logs propagation-retrieve (retrieve_mode=get|local with listed/downloaded/delivered). Docs updated accordingly. - Tests: decode round-trip, local-prop deposit->drain->deliver->purge loopback, foreign-recipient rejection, and source-contract asserts to prevent regress.
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesThe propagation sync flow now separates peer-offer synchronization from message retrieval. Remote Propagation retrieval
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LiveBridge
participant PropagationBridge
participant PropagationClient
participant Router
LiveBridge->>PropagationBridge: start_client_download(pn_hash)
PropagationBridge->>PropagationClient: start authenticated /get download
LiveBridge->>PropagationBridge: poll_client_download(known_identities)
PropagationBridge->>PropagationClient: poll download events
PropagationBridge-->>LiveBridge: completed messages and counts
LiveBridge->>Router: deliver decoded messages
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
reticulum-sidecar/src/stack/live.rs-3748-3759 (1)
3748-3759: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCancel the client download on
Failedbefore breaking.
start_client_downloadonly starts when the client is idle, andcancel_client_download()is the bridge path that consumes terminal snapshots back to idle. AClientDownloadPoll::Failedbreak leavesPropagationClientState::Failed; without an explicitbridge.cancel_client_download(), later retrieval can be blocked until the next sync cancel path.🤖 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 `@reticulum-sidecar/src/stack/live.rs` around lines 3748 - 3759, The ClientDownloadPoll::Failed branch in the bridge.poll_client_download flow must cancel the client download before breaking. Call bridge.cancel_client_download() after logging the failure and before exiting the match branch, ensuring the terminal failed state is consumed and returned to idle for later retrieval.
🧹 Nitpick comments (1)
reticulum-sidecar/src/stack/propagation_bridge.rs (1)
441-458: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReturn the client to Idle after a terminal Complete.
The
Completearm takes the received blobs but leaves the client inComplete. Only a latercancel_client_download(reached throughcancel_propagation_sync) resets it to Idle. If any future caller starts a download without that cancel,start_client_downloadreturns false and retrieval is silently skipped. Acknowledge the transfer here so the state machine self-heals.♻️ Proposed change
PropagationClientState::Complete => { let listed = client.available_messages().len(); let downloaded = client.received_count(); let blobs = client.take_received_messages(); + // Consume the terminal snapshot so the next download can start + // from Idle without depending on an external cancel. + let _ = client.acknowledge_transfer(); drop(client);🤖 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 `@reticulum-sidecar/src/stack/propagation_bridge.rs` around lines 441 - 458, Update the PropagationClientState::Complete branch in the client download polling logic to acknowledge or reset the client after taking the received messages, returning it to Idle before completing the poll. Preserve the existing message decoding and Complete result values while ensuring subsequent start_client_download calls can proceed without requiring cancellation.
🤖 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 `@reticulum-sidecar/src/stack/live.rs`:
- Around line 3725-3733: Replace the separate current-run check and
unconditional bridge.cancel_client_download call in the driver loop with
PropagationBridge::run_if_current, performing cancellation only inside its
lifecycle-lock-protected callback. If the run is stale, exit the task without
touching shared client state; preserve cancellation for the active run and the
existing loop-break behavior.
In `@reticulum-sidecar/src/stack/propagation_bridge.rs`:
- Around line 471-478: Update drain_local_propagation_inbox and
drain_local_inbox so the synchronous inbox drain runs via Tokio spawn_blocking
rather than directly on the async worker. In drain_local_inbox, limit the
local_node Mutex guard to listing, serving, and purging operations, then release
it before iterating over blobs and performing per-message decryption/decoding.
- Around line 502-512: Update the propagation purge flow around
decode_downloaded_propagated_blob so only successfully decoded messages are
purged. Build the purge IDs from each decoded message’s transient_id rather than
all tids, preserving the exact IDs stamped by compute_propagation_transient_id.
Log the number of skipped/failed blobs, and return the decoded message count
alongside the existing purge result.
---
Other comments:
In `@reticulum-sidecar/src/stack/live.rs`:
- Around line 3748-3759: The ClientDownloadPoll::Failed branch in the
bridge.poll_client_download flow must cancel the client download before
breaking. Call bridge.cancel_client_download() after logging the failure and
before exiting the match branch, ensuring the terminal failed state is consumed
and returned to idle for later retrieval.
---
Nitpick comments:
In `@reticulum-sidecar/src/stack/propagation_bridge.rs`:
- Around line 441-458: Update the PropagationClientState::Complete branch in the
client download polling logic to acknowledge or reset the client after taking
the received messages, returning it to Idle before completing the poll. Preserve
the existing message decoding and Complete result values while ensuring
subsequent start_client_download calls can proceed without requiring
cancellation.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 861c4911-2ecf-4ee8-9ad6-ae9f4ea1fb6f
📒 Files selected for processing (7)
docs/agents/reticulum.mddocs/reticulum.mddocs/troubleshooting.mdreticulum-sidecar/src/stack/live.rsreticulum-sidecar/src/stack/mod.rsreticulum-sidecar/src/stack/propagation_bridge.rsreticulum-sidecar/src/stack/propagation_download.rs
| pub(crate) fn drain_local_inbox(&self) -> (Vec<LxMessage>, usize) { | ||
| use rmpv::Value; | ||
|
|
||
| let our_delivery = | ||
| Destination::hash_from_name_and_identity("lxmf.delivery", Some(&self.identity.hash)); | ||
| let Ok(mut node) = self.local_node.lock() else { | ||
| return (Vec::new(), 0); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
drain_local_inbox blocks the async runtime and the PN serve path.
drain_local_propagation_inbox in live.rs awaits this function directly on a Tokio worker. The function holds the local_node std Mutex across list, serve, per-message decryption, and purge. Two consequences follow:
- The Tokio worker is blocked for the whole drain. This repository already treats messagestore work as blocking:
spawn_messagestore_loadusesspawn_blockingbecause "large local PN stores can take many seconds". - Remote
/offerand/getserving contend on the same node lock for the entire drain, including the decryption of every blob.
Move the drain onto spawn_blocking, and release the node lock before decoding the blobs.
🤖 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 `@reticulum-sidecar/src/stack/propagation_bridge.rs` around lines 471 - 478,
Update drain_local_propagation_inbox and drain_local_inbox so the synchronous
inbox drain runs via Tokio spawn_blocking rather than directly on the async
worker. In drain_local_inbox, limit the local_node Mutex guard to listing,
serving, and purging operations, then release it before iterating over blobs and
performing per-message decryption/decoding.
…drain - Client download driver: exit without touching shared client state when the sync run is stale (a newer run owns the client); cancel only via run_if_current (lifecycle-lock guarded) for the active run. - poll_client_download: acknowledge the terminal Complete snapshot so the client returns to Idle for the next start_client_download. - Driver Failed branch: consume the terminal failed state before breaking so a later sync can retry. - drain_local_inbox: scope the node lock to list/serve, release it before per-message decrypt/decode, and purge only successfully decoded transient_ids (log skipped/undecodable blobs) so failures are not silently dropped. - drain_local_propagation_inbox: run the blocking drain via spawn_blocking so it cannot stall the async runtime.
1 participant
Summary
Closes the structural gap where mesh-client Sync never retrieved store-and-forward mail from propagation nodes (PNs).
/offerpeer-replication path (PropagationSyncTask); it never ran the LXMF client/getpull (PropagationClient), so PN→inbox retrieval could not work. Thepropagation-retrievetelemetry on Sync Completes actually described peer-offer progress, not inbox retrieval./getpull (list → get → purge, Pythonrequest_messages_from_propagation_nodeparity) into Sync, delivers retrieved mail through the existing router callback, drains the local-prop inbox, and corrects the misleading telemetry — all locked with tests.What changed
Retrieval (
reticulum-sidecar/src/stack/)propagation_download.rs:decode_downloaded_propagated_blobdecrypts a downloaded entry (dest || encrypted_data) with the local identity, unpacks it, and marks it inbound/Propagated.propagation_bridge.rs: bridge now owns aPropagationClient;start_client_download/poll_client_downloaddrive list→get→purge and return decodedLxMessages;cancel_client_downloadfor teardown.live.rs:spawn_client_download_driverruns the/getdownload alongside the existing/offerpeer sync duringstart_propagation_sync, and on Complete delivers each message through the existing router delivery callback (WSlxmf_message, recent ring, and renderer catch-up all fire unchanged). Cancel wired intocancel_propagation_sync.Local-prop (
mod.rs+propagation_bridge.rs::drain_local_inbox): the former no-op now replays the in-process node's own/getlist→serve→purge against ourlxmf.deliveryhash and drains our inbox into Chat.Telemetry / docs: peer-offer Completes relabeled
propagation-sync … peer_outcome=have_all|transfer; real inbox retrieval now logspropagation-retrievewithretrieve_mode=get|localandlisted/downloaded/deliveredcounts. Updateddocs/troubleshooting.md,docs/reticulum.md,docs/agents/reticulum.md.Design note
The client
/getdownload runs in parallel with the existing/offerpeer sync (additive), so the WS progress bar and host-replication behavior are untouched. Making the progress bar reflect the download, or dropping peer-offer for plain user syncs, is a possible follow-up.Test plan
cargo test --features rns-stack— full sidecar suite (483 passed / 0 failed)cargo clippy --features rns-stack --all-targets— no new warningscargo fmt --check— cleanlisted=0) and not a failure; existing peer-host sync still worksSummary by CodeRabbit
New Features
Documentation