fix(reticulum): PN deposit link timeouts and local PN official parity - #832
Conversation
Advance Prefer PN cascade on link-establishment timeout instead of hammering the same hash; deposit into hosted local-prop in-process; cover outbound→drain and peer-offer round-trips with tests and docs.
|
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 (6)
📝 WalkthroughWalkthroughLocal propagation now deposits messages directly into the hosted PropagationNode. The outbound driver reports hosted-PN completion, supports peer synchronization and retrieval, and advances remote cascades after link failures, timeouts, or deferrals. ChangesHosted PN delivery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LxmfOutboundDriver
participant PropagationNode
participant PNPeer
participant LXMFClient
LxmfOutboundDriver->>PropagationNode: accept local propagation deposit
PropagationNode->>PNPeer: synchronize through /offer
LXMFClient->>PropagationNode: request message through /get
PropagationNode-->>LXMFClient: return stored message
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 |
Keep Flatpak standalone pnpm archive URLs/sha256 in sync with the pin.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (4)
docs/agents/reticulum.md-19-19 (1)
19-19: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the in-process local-prop drain name.
Line 19 describes local-prop as “client
/getdraining”. The subsystem guidance distinguishes remotePropagationClient/getretrieval from local-propPropagationBridge::drain_local_inbox()replay. Update this sentence to name the local path and describe remote/getseparately.🤖 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 `@docs/agents/reticulum.md` at line 19, Update the local-prop description in the Reticulum outbound documentation to identify the in-process drain as PropagationBridge::drain_local_inbox(). Distinguish this local replay path from remote PropagationClient /get retrieval, while preserving the existing host-peer /offer synchronization description.docs/reticulum.md-41-41 (1)
41-41: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the Propagation row’s cascade summary.
Line 41 still says Manual falls back to the “local inbox”. When local hosting is enabled, the candidate is the hosted
local-propPN and the terminal status isstored_locally. The row also conflates the Auto sync target with outbound deposit order: outbound deposits try configured remotes before Discovered PNs and local-prop last. Update this summary to match Lines 35 and 314-321.🤖 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 `@docs/reticulum.md` at line 41, Update the Propagation row to describe Manual fallback as the hosted local-prop PN with terminal status stored_locally, and distinguish Auto’s best Discovered PN sync target from outbound deposit ordering. State that outbound deposits try configured remotes first, then Discovered PNs, with local-prop last, while preserving the existing mode guidance.reticulum-sidecar/src/stack/lxmf_outbound.rs-743-762 (1)
743-762: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReturn
falsewhen the message has no hash so the Link fallback still runs.The function returns
trueafter the warning. The caller then returns fromdeliver_propagatedwithout starting packed Link delivery, without requeueing, and without emitting a status. A message that has neitherhashnormessage_idis dropped silently. Every other failure path in this function returnsfalseand falls back to Link delivery.🐛 Proposed fix
let msg_hash = message.hash.or(message.message_id); - if let Some(hash) = msg_hash { - self.pending_pn_deposits - .insert(hash, (prop_hash, last_tid.or(message.transient_id))); - self.handle_delivery_result( - router, - event_tx, - DeliveryResult::Complete { - link_id: prop_hash, - msg_hash: Some(hash), - }, - ); - } else { + let Some(hash) = msg_hash else { tracing::warn!( target: "propagation-deposit", pn_hash = %prop_hex, - "local-prop in-process deposit missing message hash" + "local-prop in-process deposit missing message hash — falling back to Link" ); - } + return false; + }; + self.pending_pn_deposits + .insert(hash, (prop_hash, last_tid.or(message.transient_id))); + self.handle_delivery_result( + router, + event_tx, + DeliveryResult::Complete { + link_id: prop_hash, + msg_hash: Some(hash), + }, + ); true🤖 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/lxmf_outbound.rs` around lines 743 - 762, Update the hash-missing branch in the outbound delivery function containing the pending_pn_deposits insertion and propagation-deposit warning to return false after logging the warning, while retaining the true return for successfully hashed messages. This must allow the caller’s Link fallback, requeue, and status handling to run when both message.hash and message.message_id are absent.reticulum-sidecar/src/stack/lxmf_outbound.rs-589-595 (1)
589-595: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid blocking the outbound tick on the shared
PropagationNodemutex.
process_ticknow holds the outbound driver and router locks while readinglocal_fl oorand accepting packed entries. Other tasks can hold the samePropagationNodemutex, so anaccept_stamped_propagated_blobpath can delay every local-propagation cascade while the maintenance loop is blocked. Keep the local-node access short and avoid long work, I/O, or CPU-heavy validation while that mutex is held.🤖 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/lxmf_outbound.rs` around lines 589 - 595, Update process_tick’s local propagation cost and packed-entry acceptance flow to avoid blocking on the shared PropagationNode mutex while outbound driver/router locks are held. Read the needed local floor with a short, non-blocking access (or defer/skip when the mutex is unavailable), and ensure accept_stamped_propagated_blob and any validation or other lengthy work occur after releasing the PropagationNode guard.
🧹 Nitpick comments (1)
reticulum-sidecar/src/stack/pn_hosting_apply.rs (1)
141-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that declined candidates are not added to
router.peers.The test checks only the boolean return of
autopeer. A regression that returnsfalsebut still inserts the peer would pass. Add the negative membership assertions fordeepandcostly, matching the positive assertion at Line 177.♻️ Proposed test hardening
metadata: None, hops: Some(5), })); + assert!(!router.peers.contains_key(&deep)); // Peering cost above max declined. assert!(!router.autopeer(AutopeerCandidate { @@ metadata: None, hops: Some(1), })); + assert!(!router.peers.contains_key(&costly));🤖 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/pn_hosting_apply.rs` around lines 141 - 164, Extend the test around the declined AutopeerCandidate cases to assert that neither deep nor costly is present in router.peers after autopeer returns false, matching the existing positive membership assertion near the later accepted-candidate case.
🤖 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.
Other comments:
In `@docs/agents/reticulum.md`:
- Line 19: Update the local-prop description in the Reticulum outbound
documentation to identify the in-process drain as
PropagationBridge::drain_local_inbox(). Distinguish this local replay path from
remote PropagationClient /get retrieval, while preserving the existing host-peer
/offer synchronization description.
In `@docs/reticulum.md`:
- Line 41: Update the Propagation row to describe Manual fallback as the hosted
local-prop PN with terminal status stored_locally, and distinguish Auto’s best
Discovered PN sync target from outbound deposit ordering. State that outbound
deposits try configured remotes first, then Discovered PNs, with local-prop
last, while preserving the existing mode guidance.
In `@reticulum-sidecar/src/stack/lxmf_outbound.rs`:
- Around line 743-762: Update the hash-missing branch in the outbound delivery
function containing the pending_pn_deposits insertion and propagation-deposit
warning to return false after logging the warning, while retaining the true
return for successfully hashed messages. This must allow the caller’s Link
fallback, requeue, and status handling to run when both message.hash and
message.message_id are absent.
- Around line 589-595: Update process_tick’s local propagation cost and
packed-entry acceptance flow to avoid blocking on the shared PropagationNode
mutex while outbound driver/router locks are held. Read the needed local floor
with a short, non-blocking access (or defer/skip when the mutex is unavailable),
and ensure accept_stamped_propagated_blob and any validation or other lengthy
work occur after releasing the PropagationNode guard.
---
Nitpick comments:
In `@reticulum-sidecar/src/stack/pn_hosting_apply.rs`:
- Around line 141-164: Extend the test around the declined AutopeerCandidate
cases to assert that neither deep nor costly is present in router.peers after
autopeer returns false, matching the existing positive membership assertion near
the later accepted-candidate case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 43a16c38-9192-4553-98fd-754705f456c6
⛔ Files ignored due to path filters (1)
src/renderer/locales/en/translation.jsonis excluded by!src/renderer/locales/**
📒 Files selected for processing (9)
docs/agents/reticulum.mddocs/reticulum-sidecar-ipc.mddocs/reticulum.mddocs/troubleshooting.mdreticulum-sidecar/src/stack/live.rsreticulum-sidecar/src/stack/lxmf_outbound.rsreticulum-sidecar/src/stack/pn_cascade.rsreticulum-sidecar/src/stack/pn_hosting_apply.rsreticulum-sidecar/src/stack/propagation_bridge.rs
…lback) Clarify local drain vs remote /get; distinguish Auto sync vs deposit order; try_lock + validate outside PropagationNode; return false without hash for Link fallback; assert declined autopeers are absent from router.peers.
1 participant
Summary
Fixes flaky / broken Remote Prefer PN deposits (not limited to local-prop-only setups) by advancing the Prefer cascade on Propagated link-establishment timeouts, hosting local-prop deposits in-process, and keeping sync↔deposit Link serialization so Prefer PN sync and outbound deposits stop fighting the same Link.
Also includes chore: pnpm 11.21.0 + Flatpak archive sha sync.
Problem
Remote Prefer PN deposits were flaky or broken even when users were not on a local-prop-only configuration.
Field evidence (PN delivery / w0rmt + Joey diagnosis):
pn-9f3f189elogged repeatedlink-timeout … skip dest=9f3f189e. With local-prop off, the cascade could fail visibly instead of quietly masking the miss.pn-deadbeefsawsyncTimedOut, never got a durablelastPropagationSyncAt, and accumulated heavylinkDeliveryTimeoutson Prefer PN / Ratspeak / Direct paths.Why it looked OK sometimes:
Root causes:
has_pending_to/propagation_sync_target), producing deferral loops andsyncTimedOut./offerinventory visibility.Fix
on_propagated_link_failure: if other cascade candidates remain, mark the timed-out Prefer PN as tried and advance; only requeue the same PN when the cascade is exhausted / last-resort retry budget applies.set_local_prop_node), pack andaccept_stamped_propagated_blobin-process; emitstored_locally; skip self-Link and skip sync-busy defer for that path; pin local prop identity while serving.PN_DEPOSIT_DEFER_ADVANCE_AFTER; sync returnsPROPAGATION_SYNC_OUTBOUND_BUSYwhen deposit owns the Link.Test plan
Automated coverage added/extended for the diagnosis paths:
DeliverPropagated→ local accept →stored_locally→ drain9f3f/deadbeef)/offerinventorystatic_peersManual (as before):
link-timeout … skip dest=…on the same Prefer id until budget burnstored_locally(no self-Link timeout) and house badge semanticsPROPAGATION_SYNC_OUTBOUND_BUSY/ defer rather thansyncTimedOutloops; deposit should advance after defer windowNote: PR #831 PATH_UNKNOWN fail-fast improves sync error surfacing, but Prefer PN deposit reliability required this cascade / Link ownership fix.