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".
BraCR10
left a comment
BraCR10
left a comment
There was a problem hiding this comment.
Ready for review
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.
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Strict review — round 2
Both blockers from the previous round are genuinely fixed: the isRestoring branch now releases the dedup reservation right after buffering (mostro_service.dart:166), and the two freshness gates in AbstractMostroNotifier compare receivedAt instead of timestamp. The addMessage transaction, the per-event try/catch in the flush, and the reconciliation-after-flush are all correct. Test coverage went from zero to real tests that exercise the production _onData with signed kind-14 / kind-1059 events — good.
Two new HIGH findings, both regressions caused by the fixes themselves (not pre-existing), plus a few smaller items. Verified against the branch head (5f2789f); flutter analyze is clean on lib/.
1. HIGH — same-second messages now tie in _getDefaultSort() and the notifier can lock onto the older one
mostro_service.dart:237-239:
msg.timestamp ??= innerCreatedAt?.millisecondsSinceEpoch ?? event.createdAt?.millisecondsSinceEpoch;Before this PR timestamp left _onData as null and MostroStorage.addMessage stamped it with arrival-time milliseconds — effectively unique per message. After this PR it is Nostr created_at, which is second-precision. MostroStorage._getDefaultSort() sorts by timestamp only, and sembast breaks ties by record key (SembastFinder.compareThenKey → compareKey(record1.key, record2.key)), i.e. by the rumor/event id — a random hex string.
AbstractMostroNotifier.subscribe() drives state from watchLatestMessage() (limit: 1). So whenever Mostro emits two messages for the same order within one second — hold-invoice-payment-settled + released, released + purchase-completed/rate, an action followed by a cant-do, etc. — which of the two becomes "latest" is decided by a random id comparison. If the older one wins, the newer message is stored but never emitted to the notifier, and the order sits on the wrong action/status until some later message arrives. This affects every live trade, not just restore.
The author dismissed the Codex P1 on this as "not from this PR"; it is — _onData never set timestamp before this branch.
Fix (small): add receivedAt as a secondary sort key so protocol time orders across seconds and arrival time orders within a second. receivedAt is already threaded through the flush with the original buffering instant, so replayed events keep their arrival order too.
List<SortOrder> _getDefaultSort() => [
SortOrder('timestamp', false, true),
SortOrder('receivedAt', false, true),
];Apply the same to getAllMessagesForOrderId() (it builds its own SortOrder). Synthetic restore messages have receivedAt == null; with nullLast: true they sort after live events in a tie, which is the right outcome. Add a regression test: two messages, same id, same timestamp, different receivedAt → watchLatestMessage emits the later receivedAt.
Related, lower impact: abstract_mostro_notifier.dart:173 builds the processed-event key as '${id}_${action}_${timestamp}'; two distinct messages with the same action in the same second now collide and the second handleEvent is skipped. Worth switching to receivedAt or the storage key.
2. HIGH — restored disputes now keep a wrong createdAt (≈ Jan 1970) and sort last in the dispute list
order_state.dart:166 changed from "always re-stamp Dispute.createdAt from the message timestamp" to "only fill when null". Correct intent — but it exposes a pre-existing unit bug that the old code was accidentally masking:
restore_manager.dart:779-780 (unchanged in this PR):
createdAt: orderDetail.createdAt != null
? DateTime.fromMillisecondsSinceEpoch(orderDetail.createdAt!)OrderDetail.createdAt is Mostro's created_at, which is Unix seconds (orders_response.dart:74, and Order.fromJson treats it the same way). Interpreting seconds as milliseconds yields a date a few days after the epoch.
Before this PR that value was immediately overwritten in updateWith with message.timestamp * 1000 where message.timestamp was orderDetail.createdAt (seconds) — i.e. the double-conversion happened to produce the right date for restored disputes. Now createdAt is non-null, so the guard keeps the 1970 value. disputeRepository.getUserDisputes() reads disputes from orderState.dispute, and userDisputeDataProvider sorts by createdAt, so every restored dispute shows a 1970 date and sinks to the bottom of the list after a restore.
Fix: convert at the source, in the restore loop — DateTime.fromMillisecondsSinceEpoch(orderDetail.createdAt! * 1000) (or route through the existing seconds/ms heuristic in Dispute._extractTimestamp). Add a test that a restored dispute's createdAt equals the order's created_at in seconds.
MEDIUM
3. SESSION_RECOVERY_ARCHITECTURE.md still describes code that does not exist — the point I raised last round was not addressed. D6 still says "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 nothing; it calls setFiatWasSent() + upgradeCooperativeCancelToFiatSent(). Please replace the sentence with what the code does. Also stale after this round: D2 says the buffer is Map<String, NostrEvent> (it is a record with receivedAtMs), D4 says the flush must clear the dedup entry "because _onData reserved that id when the event was first buffered" (buffering now releases it itself), and the pseudocode block omits both the deleteItem on buffering and the receivedAt stamping — the two things this round added. The doc is what the next reviewer will trust; it should match.
4. Tests do not pin the crash-safety fix. The reservation release on buffering was blocker #1 and there is no test that asserts it. 'redelivering the same event id while restoring preserves the first-buffered receivedAtMs' looks like it covers it, but it passes even if the reservation is not released: the second onDataForTesting would return early at hasItem, the buffer entry would stay untouched, and secondStamp == firstStamp still holds. Add after the first call:
expect(await realEventStorage.hasItem(event.id!), isFalse,
reason: 'buffering must release the dedup reservation');Also missing: the flushRestoreBuffer() single-flight (_activeFlush) and the ref.listen trigger have no test — all flush tests call flushRestoreBufferForTesting(), which bypasses both.
5. _flushRestoreBuffer still deletes the reservation before each replay (mostro_service.dart:283), which is now dead logic that can clobber a valid reservation. Since buffering already releases the reservation, this delete only matters in one case: the relay redelivers event E after the flush snapshot but before the loop reaches E. Then E is processed live and reserved, and the loop deletes that reservation and processes E a second time (addMessage is write-once so no duplicate row, but _maybeLinkChildOrder and the decrypt run twice). Drop the delete and let _onData's own hasItem check handle it.
LOW
6. receivedAt is parsed from the wire in MostroMessage.fromJson (mostro_message.dart:55). The invariant "a peer-supplied value never survives" is enforced by the overwrite in _onData:246, and there is a test for it, but it would be structural rather than procedural if the field were read only in MostroStorage.fromDbMap / the storage query paths. background_notification_service.dart:329 also builds MostroMessage.fromJson from the wire without overwriting; harmless today because it never reads receivedAt, but it is a second entry point to keep in mind.
7. Unaddressed from last round, still valid: _activeFlush returns a stale future if events land after the snapshot (a while (_restoreBuffer.isNotEmpty) loop fixes it structurally); logger.i per buffered event will spam during the full-history redelivery that restore triggers (logger.d + a count); FakeMostroService in integration_test/test_helpers.dart now has to stub four @visibleForTesting members, which is a smell that the test seam is on the wrong side of the interface.
8. mostro_storage_test.dart opens the same in-memory DB name in every setUp ('mostro_storage_test.db'); databaseFactoryMemory keeps it alive for the process, so tests share state and only pass because they use distinct keys. Use a counter or newDatabaseFactoryMemory().
Requesting changes for #1 and #2 — both are user-visible regressions on every trade / every restored dispute, and both are ~5-line fixes plus a test. #3 and #4 I would also want before merge since they are the paper trail for the two blockers from last round. Happy to re-review quickly.
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.