feat(protocol): improve delivery by persisting inbound pending-decryption queue - #438
Open
mizanisoffline wants to merge 13 commits into
Open
feat(protocol): improve delivery by persisting inbound pending-decryption queue#438mizanisoffline wants to merge 13 commits into
mizanisoffline wants to merge 13 commits into
Conversation
Pending-queue evictions only surfaced a decryption-failure event for media chunks; text frames were metrics-only. With a relay that pushes ciphertext without store-and-forward there is no second copy, so a silent text eviction was a lost message the app could not see. Every eviction now emits PendingQueueDropped with a content-appropriate reason.
Frames parked because their MLS session was not ready lived only in memory, so an app restart during a slow handshake lost them, and with a relay that pushes ciphertext without store-and-forward there was no second copy to ask for. Each admitted frame is now written under its message id to a new sealed protocol-state category (pending_decrypt_entries), deleted when it is drained, pruned, overflow-dropped or discarded, and restored on initialize_mls oldest-first without being re-persisted. A record older than seven days on disk is dropped and reported as PendingQueueDropped (expired_persisted). The in-memory TTL default moves from 30 minutes to 24 hours, mirrored in the UniFFI default, the three React Native bridge fallbacks, their tests, and the docs.
The exact-mode seen set lived only in memory, so after a restart the relay-socket copy of a message the app had already consumed from a push injection was not recognised as a duplicate: it went to a ratchet whose generation was already spent and surfaced as a decryption failure, which the app counts toward a split-brain breaker that tears down healthy sessions. The deduplicator can now export and import its ids; the protocol writes them under dedup_seen_ids in batches (32 changes or 5 s on the process tick, flushed on stop and drop) and restores them next to the Lamport clock. Defaults move to 2000 ids and 24 h retention, which also means the Nostr replay overlap is now absorbed rather than re-processed; the drift guard and its three notes are updated accordingly.
…rue } The relay has no store-and-forward: when the recipient has no live socket it pushes the ciphertext and still answers MessageSent, so both bridges resolved the frame as accepted and left it awaiting an ACK from a peer who may never receive the push. Both InternetManagers now read the new pushed flag next to resolveOnRelayAccepted and, when set, fail that one id into the core as recipient_unreachable: relay_pushed (the prefix the core parks a DM on, without draining the recipient's other in-flight frames), watch the recipient, and feed an offline presence, mirroring the DeliveryError path. A core test pins the classification and a UniFFI source guard pins both call sites.
…until re-driven The behaviour already holds: presence_watch_peers lists the recipient of a DM parked on a recipient_unreachable verdict, and a presence-online answer re-drives the message with a fresh pending ACK, which takes the recipient back off the list and clears the park counter. Pinned so the relay-pushed park added alongside it keeps relying on it.
OutboxEntry.reseal was memory-only, so a resend after a restart replayed the ciphertext sealed at send time; after a re-key in between (a healed desync, a reinstalled recipient) those bytes are sealed to a dead epoch and the retry loop burned its budget delivering nothing. The provenance now rides in the persisted outbox record, sealed under the credential-store record key like the pre-session pending queue that already holds the same plaintext, and erased with the entry on ACK or expiry. A legacy record without the field restores with None and replays verbatim as before.
message_received, file_received and message_decryption_failed were dropped whenever the JS gate was shut: no subscription, or no live React instance while the app was backgrounded. By then the core has already ACKed the message, dedup-marked its id and dropped its queued copy, so the sender never resends and the drop was a lost message. Each bridge now holds those three event types in a 256-entry buffer keyed type:message_id (a second StickyEventDispatcher on Android, a new InboundEventBuffer on iOS) and flushes on subscribe and on foreground, with a session generation so a message held for a torn-down account never surfaces in the next one. The TypeScript layer holds the same types in a FIFO across the JS-to-app gap and replays them on the first matching on(). A Rust source guard pins the set across all three layers.
Reverts fefe86c. Persisting OutboxReseal wrote every sent-but-unACKed encrypted DM's plaintext to disk for the outbox lifetime, which ADR 0007 rules out by name: re-seal provenance is memory-only, and persisting it "so resends survive a restart" is listed as the change that would undo the decision. The outbox state machine (invariant S3, provenance rule 3) and the session-lifecycle residual state the same trade. A resend after a restart plus a re-key therefore replays verbatim again and settles as an honest failure, the documented cost of not keeping plaintext at rest.
… to plain DMs
Four gaps in the delivery work on this branch.
A peer-requested session reset drained the inbound pending-decryption
queue in memory but left the persisted records, so the next launch
restored frames sealed to the deleted session and drained them into the
replacement one as spurious decrypt failures. It now goes through
discard_pending_decryption_for_peer, which deletes the records too.
release_replay_protection unmarked the envelope id on the deduplicator
directly, which the persisted seen set never counted. A record written
while the group entry was buffered kept the id, the next launch restored
it, and the sender's redelivery was swallowed and re-ACKed as delivered
for the 24 h retention window. It now uses unmark_seen_persisted.
MessageSent { pushed: true } reached the core as a recipient_unreachable
tail, and the core prefix-matches that before the DM park: a pushed
connection request fast-failed as ConnectionRequestUndeliverable and a
pushed Welcome moved to Failed, both for frames the push may have
delivered, and every pushed DM emitted MessageUndeliverable. The bridges
now send a dedicated relay_pushed token, which parks plain DMs only and
emits nothing app-facing; connection requests and Welcomes stay on their
ordinary path. The timed probe stays: the relay answers a repeat push of
the same (recipient, message_id) with DeliveryError, so probing does not
re-notify, and the doc that said a successful push returns no verdict is
corrected.
The inbound pending-decryption walk added a fourth prune pool while the
docs and the pinning test still said the launch ceiling was three pools.
It also drew with advisory reservations and could spend only half its
pool, and the seen-set restore deleted a corrupt record outside every
pool. The two inbound walks now share one private pool through
refusing_private, and the docs and the ceiling test say four.
Updated the deduplicator's method for marking a message as seen to better reflect its purpose. The new method, mark_seen_local, indicates that the message ID is only tracked for the current process and will not be exported in the persisted seen set. This change enhances code readability and aligns with the intended functionality of preventing relayed echoes of outgoing messages from being persisted. Additionally, updated relevant comments and tests to reflect this change.
update_dedup_config rebuilt the deduplicator from scratch, which forgot every id it held. While the seen set lived in memory only that was a silent restart; now that the set is persisted and restored at launch it was a loss: the React Native layer applies reliability.dedup through this method on every start(), right after the restore, so the restored ids were discarded and the next batch write overwrote the record with the near-empty set. The socket copy of a message already consumed from a push injection then reached a spent ratchet generation after all, the exact failure the persisted set exists to prevent. Deduplicator::reconfigure rebuilds under the new configuration and carries the exact-mode entries across, re-bounded by the new retention window and cap (newest kept) with each entry's exportable flag intact, so a mark_seen_local id stays out of the export. A change of mode carries nothing. update_dedup_config uses it and counts the change so the next batch write re-states the record under the new bounds. Claude-Session: https://claude.ai/code/session_014wvbMz72ffaijASDcw5iri
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.
Summary
Closes the ways a message could be silently lost between a sender's send and the recipient app's handler, now that the relay pushes ciphertext without store-and-forward (so there is never a second copy to ask for).
Persistence (core)
pending_decrypt_entries) on admission, deleted when drained/evicted/discarded, and restored oninitialize_mlsoldest-first. A record older than 7 days on disk is dropped and reported asPendingQueueDropped { expired_persisted }. Previously a restart during a slow handshake lost the frame.dedup_seen_ids, written in batches (32 changes or 5 s on the process tick, flushed on stop/drop) and restored beside the Lamport clock. Fixes the relay-socket copy of a push-injected message reaching a spent ratchet generation after a restart and surfacing as a decryption failure (which apps count toward a split-brain breaker).update_dedup_confignow carries the set across viaDeduplicator::reconfigureinstead of rebuilding from empty; the sender's own outgoing ids aremark_seen_localand never exported.PendingQueueDroppedis emitted for text messages, not only media chunks. Text evictions were metrics-only.Relay-pushed DMs (core + bridges)
MessageSent { pushed: true }the bridges report the id to the core as a dedicatedrelay_pushedtoken. The core parks a plain DM only: drops the pending ACK, keeps the outbox entry, schedules the reachability probe and presence-watches the recipient, but emits noMessageUndeliverable(the push may have delivered it). Connection requests and Welcomes stay on their ordinary path. The outbox entry carries a persistedrelay_pushedmark so a laterDeliveryErrorprobe verdict for that frame re-parks silently rather than being read as an ordinary unreachable.Held inbound events (bridges)
message_received,file_receivedandmessage_decryption_failedwere dropped whenever JS had no listener or no live React instance. By then the core has ACKed and dedup-marked the message, so the drop was a lost message. Each bridge now holds up to 256 of them keyedtype:message_id(KotlinStickyEventDispatchersecond instance, new SwiftInboundEventBuffer), flushed on subscribe and on foreground and scoped to the session generation. The TypeScript layer does the same across the JS-to-listener gap. A Rust source guard pins the set across all three layers.Fixes found while doing the above
release_replay_protectionnow reaches the persisted seen set (unmark_seen_persisted), so a dropped group entry's redelivery is not swallowed and re-ACKed for the whole retention window.Defaults changed (behavioural, documented in
CHANGELOG.mdanddocs/configuration.md)pendingQueue.pendingTtlMsreliability.dedup.maxTrackedMessagesreliability.dedup.retentionTimeSecsThe Nostr replay overlap (1 h 5 min) now fits inside the dedup retention window and is absorbed rather than re-processed.
Related issues
None.
Type of change
feat— new featurefix— bug fixrefactor— code change that neither fixes a bug nor adds a featuretest— adding or correcting testsdocs— documentation onlyChecklist
cargo fmt --all -- --checkpassescargo clippy --workspace -- -D warningspassescargo test --workspacepasses (Test jobs on macOS/Ubuntu/Windows, Android, iOS, Python and embedded-core were still pending on CI when this description was written)cargo-denyis satisfied (no new license/advisory violations)<type>(<scope>): <subject>)CHANGELOG.mdupdated where relevantunsafein core crates (only theoffline-protocol-uniffiFFI boundary may use it, with SAFETY comments)Breaking changes
None to the public API, wire format or config schema. Three config defaults changed (table above); apps that set these explicitly are unaffected. Two new sealed protocol-state categories (
pending_decrypt_entries,dedup_seen_ids) are written; a legacy store without them restores as before.Notes for reviewers
relay_pushedscoping: an earlier iteration reported the push as arecipient_unreachabletail, which fast-failed pushed connection requests and moved pushed Welcomes toFailed. The dedicated token exists so those stay on their ordinary path; the pinning tests inprotocol/tests/mod.rscover the three frame kinds.mark_seen_localids (our own outgoing echoes) are flagged non-exportable and survivereconfigurewith that flag intact.refusing_private; the launch ceiling test moved from three to four pools. Worth a look if you ownprotocol/storage.rs.docs/bridges/README.md, C-section on subscribe timing). Enrolling a periodic event there would replay stale state; folding it into the one-shot map would keep one message of many.protocol/storage.rs(+527),reliability/deduplicator.rs(+342),uniffi/src/lib.rs(+250, source guards), and the two bridgeInternetManagers.