fix(restore): buffer live events during restore to prevent state corruption - #639
fix(restore): buffer live events during restore to prevent state corruption#639BraCR10 wants to merge 13 commits into
Conversation
During the restore window, incoming events were dropped after being marked as seen for dedup, permanently losing genuinely live state changes (e.g. a counterparty payment arriving mid-restore). Buffer them instead and replay once restore ends, on both the success and error paths, clearing the dedup entry first so the replay isn't self-blocked. Also moves the restore-buffer check ahead of the session-match check in _onData, closing a gap where an event for a session not yet recreated mid-restore was still dropped instead of buffered. Synthetic restore snapshot messages now anchor their ordering timestamp to a single restore-start time instead of the daemon's per-order created_at (which reflects order creation, not current status, and can be null per protocol) or per-message "now", so live events during the window sort correctly relative to the snapshot. Closes #584. Supersedes the stale #589, whose reviewed buffer-and- replay design (credit: Catrya) this reincorporates against current main, extended to cover both v1 (gift wrap) and v2 (NIP-44) transport.
|
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 updates session recovery with shared timestamp anchoring, live-event buffering and replay, deferred fiat-sent reconciliation, corrected freshness handling, testing hooks, and architecture documentation. ChangesSession Recovery Fixes
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
Sequence Diagram(s)sequenceDiagram
participant RestoreService
participant MostroService
participant EventStorage
participant OrderNotifier
RestoreService->>MostroService: enable restore mode
MostroService->>EventStorage: reserve and buffer live event
RestoreService->>MostroService: complete restore
MostroService->>EventStorage: delete dedup reservation
MostroService->>MostroService: replay buffered events
RestoreService->>EventStorage: reconcile fiatSent history
RestoreService->>OrderNotifier: update restored order state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/services/mostro_service.dart (1)
237-247: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIsolate per-event failures during replay.
_onDatawraps only its decrypt/process block in try/catch;eventStore.deleteItemand the reservation lines run outside it. An exception on any single event aborts the loop, and since_restoreBufferwas already cleared (Line 240) the remaining buffered events are lost with no retry. Guarding each replay keeps one bad event from silently dropping the rest.♻️ Guard each replayed event
for (final event in events) { - await eventStore.deleteItem(event.id!); - await _onData(event); + try { + await eventStore.deleteItem(event.id!); + await _onData(event); + } catch (e, stack) { + logger.e('Restore: failed to replay buffered event ${event.id}', + error: e, stackTrace: stack); + } }🤖 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 `@lib/services/mostro_service.dart` around lines 237 - 247, The replay logic in _flushRestoreBuffer() is aborting the whole batch when a single event fails, because _restoreBuffer is cleared before processing and the per-event delete/replay steps are not isolated. Update _flushRestoreBuffer() in MostroService to handle each event independently, wrapping the deleteItem and _onData(event) work for each item in its own try/catch so one failure does not stop the remaining buffered events. Use the existing _flushRestoreBuffer, _onData, and eventStorageProvider symbols to locate the replay path.
🤖 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 `@docs/architecture/SESSION_RECOVERY_ARCHITECTURE.md`:
- Around line 611-622: The fenced pseudocode block in
SESSION_RECOVERY_ARCHITECTURE.md is missing a language specifier, causing the
docs lint MD040 warning. Update that fenced block to include an explicit
language tag such as dart or text, keeping the existing _onData(event)
pseudocode content unchanged.
---
Nitpick comments:
In `@lib/services/mostro_service.dart`:
- Around line 237-247: The replay logic in _flushRestoreBuffer() is aborting the
whole batch when a single event fails, because _restoreBuffer is cleared before
processing and the per-event delete/replay steps are not isolated. Update
_flushRestoreBuffer() in MostroService to handle each event independently,
wrapping the deleteItem and _onData(event) work for each item in its own
try/catch so one failure does not stop the remaining buffered events. Use the
existing _flushRestoreBuffer, _onData, and eventStorageProvider symbols to
locate the replay path.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: 3acaad7e-abad-4b09-97d1-742eb5a2fa84
📒 Files selected for processing (4)
docs/architecture/SESSION_RECOVERY_ARCHITECTURE.mdintegration_test/test_helpers.dartlib/features/restore/restore_manager.dartlib/services/mostro_service.dart
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
_flushRestoreBuffer() cleared the buffer before processing and looped over events with no per-event error handling, so a single failure (e.g. deleteItem) would abort the whole replay, silently dropping any remaining buffered events for that restore. Wrap each event's deleteItem + _onData call in its own try/catch so one failure is logged and skipped without stopping the rest of the batch.
BraCR10
changed the title
|
While reviewing MostroP2P/mostro#754, found that this PR's buffer mechanism also might help to close #586. @codaMW already dug into #586 and found the real cause. The AddInvoice was arriving before the app had even finished recreating the session for that order, so MostroService could not match it to anything, dropped it with a warning, and never stored it at all. Instead of trying to match a session the moment an event shows up, it now checks first whether restore is still running. If it is, the event just gets buffered, no session match attempted yet. Once restore is fully done, all sessions already exist and the flag is already false, so every buffered event gets replayed the same way live events normally are, the match succeeds, it gets stored, and the notifier updates the state like it should. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 023dbe71ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex flagged a race on PR #639: RestoreService checked local message history for fiatSent/fiatSentOk before the restore buffer had a chance to flush a same-order backfilled event, permanently misclassifying cooperatively-canceled/disputed orders as no-fiat-sent. A seller in that state loses the Release action even though the buyer already paid. Fix: track affected orders during the restore loop instead of guessing inline, then reconcile once in a finally block after MostroService's buffer is provably drained via a new public flushRestoreBuffer().
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d2f390b49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
message.timestamp is already milliseconds everywhere it's set in this codebase (mostro_service.dart, mostro_storage.dart, restore_manager.dart all use millisecondsSinceEpoch). This code treated it as seconds and multiplied by 1000 again, pushing Dispute.createdAt to roughly year 58488 for restored disputes. Pre-existing since 973b017 (~9 months old), unrelated to the restore buffer/fiat-sent work in this branch. Real-world impact was low (createdAt is never rendered as text, only used to sort disputes when more than one exists) but surfaced again in review on this branch's restore path, so fixing it here.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbdb6b43ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1. _restoreStartTime used raw millisecond precision, which could occasionally sort newer than a live event created in the same Nostr second (Nostr created_at is second-precision, never exact). Floored the anchor to just before the current second so it can never outrank a same-second live event. 2. _reconcileFiatSent replayed the tracked synthetic snapshot after the buffer flush, unconditionally overwriting anything newer the flush had just applied for the same order. An earlier version of this fix guarded the replay by comparing stored timestamps, but that left setFiatWasSent() running unconditionally while skipping the action remap it depends on — a live cooperativeCancelInitiated* event flushed before reconciliation would still get stuck on the NoFiat action variant. Replaced with a direct, idempotent correction (OrderNotifier.upgradeCooperativeCancelToFiatSent) that only touches the action field when it's still a stale NoFiat variant, regardless of what produced it. Documented both as D5/D6 in SESSION_RECOVERY_ARCHITECTURE.md.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 541f0e4df1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
updateWith() re-synced Dispute.createdAt to match the wrapping message's timestamp whenever they differed, not just when createdAt was missing. For restored disputes this clobbers the real creation time (sourced from orderDetail.createdAt when the synthetic message is built) with _restoreStartTime, the restore ordering anchor — making restored disputes sort as if freshly created. A dispute's creation time is fixed and both message-construction paths (restore and live) already resolve it properly before this code runs, so it should only ever fill a genuinely missing value, never overwrite an existing one.
_onData reserves an event's dedup entry before checking for a matching session, then silently drops the event if none exists — without undoing that reservation. If restore() aborts before recreating every session (the finally block flushes the buffer regardless of outcome), any buffered event for a not-yet-recreated session gets dropped here during replay, but its dedup entry survives. A later retry, even after the session exists, would then see the event as already processed and silently skip it — permanent data loss. Delete the dedup entry before returning so a genuinely unprocessed event can still be picked up later.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3feeda7712
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Strict review
The core design (buffer before the session-match check, flush on the true → false transition, single restore-start anchor) is sound, and the reasoning in SESSION_RECOVERY_ARCHITECTURE.md is genuinely good. I traced the whole path — _clearAll() → temp subscription → restore() → incremental saveSession() → SubscriptionManager re-subscribe (the orders filter has no since, so the relay re-delivers the full history for every restored trade key) → buffer → flush → OrderNotifier.sync() — and the mechanism holds together.
Two things block for me, both because they reintroduce or extend #584-class loss outside the window the PR reasons about.
1. BLOCKING — an interrupted restore permanently loses every buffered event
_onData persists the dedup reservation before buffering:
await eventStore.putItem(event.id!, {...}); // persistent (sembast)
if (ref.read(isRestoringProvider)) {
_restoreBuffer[event.id!] = event; // in-memory only
return;
}If the process dies mid-restore — user kills the app, OOM, or mostroServiceProvider is disposed (node switch) — _restoreBuffer evaporates while the eventStore entries survive. On the next launch the relay re-delivers those exact events (no since in buildOrdersFilter), hasItem(id) returns true, and they are dropped forever. That is precisely the mechanism the PR description calls out: "dedup marked them seen but never persisted them, so they were lost for good." Restore is a multi-second, multi-round-trip operation, so this is not a theoretical window.
The fix is the same one already applied for the session-less path in 3feeda7 — release the reservation when buffering, and let the replay re-reserve it:
if (ref.read(isRestoringProvider)) {
_restoreBuffer[event.id!] = event;
await eventStore.deleteItem(event.id!); // the in-memory map already dedups by id
return;
}No downside: a duplicate delivery from a second relay during restore re-enters, overwrites the same map key, and deletes the entry again. _flushRestoreBuffer already re-reserves via _onData. This also makes dispose() — which currently discards _restoreBuffer silently, without flushing or clearing — harmless instead of lossy.
2. BLOCKING — msg.timestamp ??= <protocol time> silently re-defines the 60-second freshness gate
This line changes the meaning of MostroMessage.timestamp app-wide, not just for replayed events. Before this PR the field came out of _onData as null and was stamped by MostroStorage.addMessage:
// lib/data/repositories/mostro_storage.dart:20
message.timestamp ??= DateTime.now().millisecondsSinceEpoch; // arrival timeSo timestamp meant "when we received it". It now means "when Mostro sent it". That is the right call for ordering — but AbstractMostroNotifier.subscribe() uses the same field as a liveness gate:
// abstract_mostro_notifier.dart:114
if (msg.timestamp != null &&
msg.timestamp! > DateTime.now().subtract(const Duration(seconds: 60)).millisecondsSinceEpoch) {
unawaited(handleEvent(msg, ...)); // notification, navigation, side effects
} else { /* only disputeInitiated* survives */ }Failure scenario: the user is offline or backgrounded for five minutes; on reconnect the relay delivers a pay-invoice / fiat-sent / released gift wrap created three minutes ago. Before: timestamp = arrival = now → gate passes → notification and navigation fire. After: timestamp = three minutes ago → gate fails → state updates silently, no notification, no navigation. Relay backlog and offline reconnects are the normal case for a mobile Nostr client, so this suppresses real user-facing notifications.
The gate is a pre-existing wart, but this PR is what breaks it, and it needs a deliberate decision — either keep a separate receivedAt for liveness and use protocol time only for ordering, or convert the gate to compare against app-session start. As written it changes behavior far outside "restore" with nothing testing it.
Same field, same class of concern, in a money path: reconcileBondCancelAction() computes nowMs - latestCanceledTimestamp against the bond grace window, and bond_payout_helpers sorts by it. Behavior probably improves there (real cancel time beats arrival time), but it is an unflagged blast radius.
MEDIUM
3. Zero tests. The PR adds five @visibleForTesting hooks — onDataForTesting, flushRestoreBufferForTesting, restoreBufferForTesting, reconcileFiatSentForTesting, floorToPreviousSecondForTesting — and grep -rn ForTesting test/ returns nothing. They are used only by the FakeMostroService stubs in integration_test/test_helpers.dart. That is dead API surface on production classes, and the PR rewrites the ordering/dedup/timestamp semantics of the app's core message-ingress path with no unit coverage. At minimum: (a) event is buffered while isRestoring; (b) buffering happens even when no session matches; (c) flush clears the dedup entry so the replay is not swallowed; (d) a replayed v1 event keeps its inner rumor's created_at; (e) _reconcileFiatSent upgrades cooperativeCancelNoFiatByPeer when a fiatSent lands in the flush.
4. The doc describes logic that is not implemented. D6 in SESSION_RECOVERY_ARCHITECTURE.md states: "The tracked snapshot is only replayed if no stored message for that order is newer, so a buffered live update flushed on the same pass is never overwritten by the stale snapshot." _reconcileFiatSent() does no timestamp comparison and replays no snapshot — it calls setFiatWasSent() + upgradeCooperativeCancelToFiatSent(), which is safe only because the upgrade is guarded on the current action. The code is fine; the paragraph is fiction, and the doc is what the next reviewer will trust.
LOW
5. _activeFlush can hand back a stale future. flushRestoreBuffer() joins the in-flight flush, but _flushRestoreBuffer() snapshots _restoreBuffer.values once at the top, so anything buffered after that snapshot is not drained by the future the caller awaits. _operationInProgress currently prevents overlapping restores, so this is latent — but a while (_restoreBuffer.isNotEmpty) loop makes it structurally correct rather than accidentally correct.
6. deleteItem on session-less events costs a write cycle per redelivery. Every reconnect re-delivers events for trade keys that will never match, and each one now does putItem → deleteItem → logger.w. Correct, but checking the session before reserving would avoid the churn.
7. The buffer is unbounded and logs one logger.i per event. Because the orders filter has no since, the mid-restore re-subscribe re-delivers the entire history for every restored trade key — all of it lands in _restoreBuffer and emits an info line each. Worth logger.d plus a size counter. The naming and comments ("live events") also undersell what actually goes in there: it is the full re-delivered history plus live events.
flutter analyze is clean on the branch (the only failures in my env are missing generated mocks — build_runner breaks against analyzer 8.4.1 locally, unrelated to this PR), and CI is green.
Happy to re-review as soon as (1) and (2) are addressed; the rest can be follow-ups if you want to keep this moving.
…n gate msg.timestamp now holds protocol send time (needed for ordering), which silently broke the 60s live-notification/navigation freshness gate for any message delivered late (offline reconnect, relay backlog). Adds a receivedAt field stamped with true client arrival time, persisted write-once via MostroStorage's existing dedup guard but excluded from toJson() so it never reaches the outbound wire path. Also fixes the restore buffer to preserve the original buffering instant across redeliveries instead of re-stamping it at flush time. Addresses grunch's BLOCKING review comment on PR #639.
Both live-notification/navigation gate checks in AbstractMostroNotifier compared against msg.timestamp, which now holds protocol send time instead of arrival time. Swapped both to receivedAt so a message delivered late (offline reconnect) but genuinely new to this client still triggers a notification.
Adds the test coverage grunch's review requested for the restore-buffer work: buffering during isRestoring (with and without a matching session), dedup clearing on flush so replay isn't swallowed, the v1 inner-rumor timestamp anchor, and _reconcileFiatSent upgrading a stale cooperativeCancelNoFiatByPeer once a confirming fiat-sent message lands in the flush. Also adds the core regression coverage for this session's receivedAt fix: the freshness gate must key off receivedAt (not protocol timestamp) so a message with an old send time but a fresh arrival time still notifies, MostroStorage's write-once guard must not let a redelivery overwrite the original receivedAt, the restore buffer must preserve the first-buffered receivedAtMs across redeliveries, and the client's explicit receivedAt assignment must always win over any wire-supplied value. test/mocks.dart gains a generated mock for EventStorage so the ported buffer tests can assert dedup-clearing order with verifyInOrder, matching the mocking style already used for the sibling storage mocks.
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)
lib/data/repositories/mostro_storage.dart (1)
14-24: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake
addMessageatomic
lib/data/repositories/mostro_storage.dart:14-24— Concurrent calls can both passhasItem(id)before eitherput, so the later write can replace the firstreceivedAt. Wrap the existence check and insert in one transaction, and add a concurrent regression test.🤖 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 `@lib/data/repositories/mostro_storage.dart` around lines 14 - 24, The addMessage method is vulnerable to concurrent writes because hasItem and put are separate operations. Make the existence check and insert atomic within the transaction mechanism provided by the storage layer, preserving first-write-wins behavior for receivedAt, and add a regression test that invokes concurrent addMessage calls for the same key and verifies only the first record is retained.
🤖 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 `@lib/data/repositories/mostro_storage.dart`:
- Around line 14-24: The addMessage method is vulnerable to concurrent writes
because hasItem and put are separate operations. Make the existence check and
insert atomic within the transaction mechanism provided by the storage layer,
preserving first-write-wins behavior for receivedAt, and add a regression test
that invokes concurrent addMessage calls for the same key and verifies only the
first record is retained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8b47c07-4383-4201-9855-a26ecc0fa72a
📒 Files selected for processing (11)
integration_test/test_helpers.dartlib/data/models/mostro_message.dartlib/data/repositories/mostro_storage.dartlib/features/order/models/order_state.dartlib/features/order/notifiers/abstract_mostro_notifier.dartlib/services/mostro_service.darttest/data/repositories/mostro_storage_test.darttest/features/order/notifiers/abstract_mostro_notifier_freshness_gate_test.darttest/features/restore/restore_manager_reconciliation_test.darttest/mocks.darttest/services/mostro_service_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- integration_test/test_helpers.dart
- lib/services/mostro_service.dart
hasItem and put were separate awaited calls, leaving a window where two concurrent addMessage calls for the same key (e.g. the same event redelivered by a second relay) could both pass the existence check before either write landed, breaking the first-write-wins guarantee receivedAt depends on. Wraps both inside a single db.transaction so the check and write can't be interleaved by another call.
|
Hello @grunch, both blocking issues you flagged are fixed: Blocking 1: Applied the same dedup-release pattern you pointed out, the Blocking 2: Added a separate Also added the test coverage you asked for on the buffer/dedup/reconciliation behavior, plus regression tests for the new Thanks for the thorough review — both findings were real and worth catching before merge. |
"Message freshness check failed" logged at warning level read like an error to a reviewer, when skipping a notification for a non-live (e.g. restore-reconstructed) message is the expected, common case. Reworded to "Notification skipped/eligible" and dropped to info level.
2 participants
Closes #584.
Re-implementation of #589, which never merged — it had a reviewed buffer-and-replay fix but fell too far behind main (Transport v2 touched the same files) to rebase cleanly. Same design, redone against current main.
Changes
created_at. That field is order-creation time, not current status, and can be null.Observations
Found an unrelated bug while testing on device: ChatRoomNotifier/ChatRoomsNotifier/DisputeChatNotifier crash with "used after dispose" during restore's
_clearAll(). Predates this branch, reproduces on main too. Tracking separately.Summary by CodeRabbit
receivedAt) to better handle reconnects.