fix: Prevent corrupting restored order state on relaunch#589
Conversation
…restore During account restoration, live direct messages (DMs) were being written to local storage. This could interfere with the restoration process, potentially leading to an inconsistent or corrupted state where restored data is overwritten by new events, causing issues upon app relaunch. This change prevents new DMs from being saved while a restoration is active, ensuring the integrity of the restored account data.
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesRestore Guard for Message Persistence
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/services/mostro_service.dart (1)
115-118:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLive events arriving during restore are permanently dropped from
mostroStorageThe event ID is committed to
eventStoreunconditionally at lines 115–118, before the restore guard at lines 166–169. WhenisRestoringProvideristrue, the guard returns early without writing tomostroStorage. After restore completes andisRestoringProvidertransitions tofalse, theeventStoreretains those cached IDs indefinitely. If the relay replays or resends those events,eventStore.hasItem(event.id!)at line 112 returnstrueand the handler exits before reaching the guard — the message is never stored, regardless of relay retries.Any genuinely live event (e.g. a counterparty payment confirmation arriving while restore was running) is silently and permanently excluded from
mostroStorage. The restore process does not invalidateeventStorageProvideror trigger a re-subscription withsincefilters to recover missed events.A minimal mitigation is to defer the event ID write until after the restore guard, so the event remains eligible for normal processing once
isRestoringProvidertransitions tofalse:🔧 Proposed fix
Future<void> _onData(NostrEvent event) async { final eventStore = ref.read(eventStorageProvider); if (await eventStore.hasItem(event.id!)) return; - // Reserve event ID immediately to prevent duplicate processing from multiple relays - await eventStore.putItem(event.id!, { - 'id': event.id, - 'created_at': event.createdAt!.millisecondsSinceEpoch ~/ 1000, - }); // ... (decryption, JSON parsing, restore-payload filters) ... + final isRestoring = ref.read(isRestoringProvider); + + if (isRestoring) { + logger.i('Restore in progress, skipping storage write for ${msg.action}'); + return; + } + + // Reserve event ID to prevent duplicate processing from multiple relays. + // Deferred until after restore guard to allow post-restore reprocessing. + await eventStore.putItem(event.id!, { + 'id': event.id, + 'created_at': event.createdAt!.millisecondsSinceEpoch ~/ 1000, + }); - if (ref.read(isRestoringProvider)) { - logger.i('Restore in progress, skipping storage write for ${msg.action}'); - return; - } await messageStorage.addMessage(messageKey, msg);This ensures that events received during restore are not marked as processed in
eventStore, allowing them to be correctly stored when they arrive again post-restore.🤖 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 115 - 118, The event ID is being committed to eventStore via eventStore.putItem before the isRestoringProvider guard, causing live events received during restore to be marked processed and permanently skipped; move the eventStore.putItem call so it runs only after the isRestoringProvider check and after the event has been successfully written to mostroStorage (i.e., only write the ID in the branch where you call mostroStorage write/success), ensuring eventStore.hasItem(event.id!) remains false during restore and the event can be processed/replayed later once isRestoringProvider is false.
🧹 Nitpick comments (1)
lib/services/mostro_service.dart (1)
156-169: 💤 Low valueMinor inefficiency: JSON parsing before the restore guard
MostroMessage.fromJson(result[0])at line 156 is invoked before theisRestoringProvidercheck at line 166. During high-volume restore replay, every incoming event pays the full deserialization cost even though the result is discarded. Moving the guard to just after the restore-payload filter (line 154) avoids this work.♻️ Proposed refactor
+ final isRestoring = ref.read(isRestoringProvider); + if (isRestoring) { + logger.i('Restore in progress, skipping storage write'); + return; + } + final msg = MostroMessage.fromJson(result[0]); final messageStorage = ref.read(mostroStorageProvider); final messageKey = decryptedEvent.id ?? event.id ?? 'msg_${DateTime.now().millisecondsSinceEpoch}'; - if (ref.read(isRestoringProvider)) { - logger.i('Restore in progress, skipping storage write for ${msg.action}'); - return; - }🤖 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 156 - 169, Move the isRestoringProvider guard so we skip expensive JSON deserialization when restoring: check ref.read(isRestoringProvider) (and the existing payload filter at the top of the handler) before calling MostroMessage.fromJson(result[0]). In practice, relocate the restore-check above the current MostroMessage.fromJson call and only construct msg and compute messageKey (using decryptedEvent.id/event) after the guard passes; preserve the existing logger.i('Restore in progress...') early-return behavior and keep references to ref.read(mostroStorageProvider), decryptedEvent, event, and messageKey intact.
🤖 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/services/mostro_service.dart`:
- Around line 115-118: The event ID is being committed to eventStore via
eventStore.putItem before the isRestoringProvider guard, causing live events
received during restore to be marked processed and permanently skipped; move the
eventStore.putItem call so it runs only after the isRestoringProvider check and
after the event has been successfully written to mostroStorage (i.e., only write
the ID in the branch where you call mostroStorage write/success), ensuring
eventStore.hasItem(event.id!) remains false during restore and the event can be
processed/replayed later once isRestoringProvider is false.
---
Nitpick comments:
In `@lib/services/mostro_service.dart`:
- Around line 156-169: Move the isRestoringProvider guard so we skip expensive
JSON deserialization when restoring: check ref.read(isRestoringProvider) (and
the existing payload filter at the top of the handler) before calling
MostroMessage.fromJson(result[0]). In practice, relocate the restore-check above
the current MostroMessage.fromJson call and only construct msg and compute
messageKey (using decryptedEvent.id/event) after the guard passes; preserve the
existing logger.i('Restore in progress...') early-return behavior and keep
references to ref.read(mostroStorageProvider), decryptedEvent, event, and
messageKey intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 363e3505-e6a5-4e82-ad3b-1de7259320e3
📒 Files selected for processing (1)
lib/services/mostro_service.dart
During account restoration, `MostroService` saved historical relay events with current timestamps. This caused them to be replayed after the authoritative synthetic messages (which use original creation timestamps), leading to incorrect order state after an app relaunch. The `isRestoringProvider` flag now prevents `addMessage` during restore, ensuring only the correct restored state persists.
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
I agree with the direction of the fix, but there is still a real correctness issue in the current implementation that makes this unsafe to merge as-is.
The problem is the interaction between the new restore guard and event deduplication in MostroService._onData:
- event IDs are still registered in
eventStoragebefore the restore guard returns - when
isRestoringProvideris true, the message is not written tomostroStorage - after restore finishes, that event is now considered already seen and will not be reprocessed
So any genuinely live DM that arrives during the restore window can be permanently dropped from mostroStorage. This fixes one corruption mode, but introduces another one: missing live events that happened during restore.
That tradeoff is not safe unless the restore flow explicitly guarantees that every event seen during restore is either:
- replayed again after restore, or
- intentionally buffered and persisted later, or
- guaranteed to be fully represented by the synthetic restore state.
From the code in this PR, I do not see that guarantee. Right now the implementation looks like: dedup yes, persist no, replay no. That can lose data.
Please address that gap before merge, either by moving the restore guard earlier so skipped events are not marked as processed, or by buffering/replaying skipped live events once restore completes, or by documenting and enforcing a stronger invariant that makes the drop impossible in practice.
I understand the concern. The filter exists because Nostr relays sometimes replay old events with newer timestamps, which caused |
|
Thanks for the clarification. I agree that this patch fixes the specific relaunch corruption path you were targeting, and I also agree the probability of a genuinely live DM landing inside that restore window is likely low in practice. That said, I do not think the reply fully resolves the correctness concern I raised. The current behavior is still: an event can be marked as seen in So my view is:
Because of that, I am not going to claim my original objection was wrong. If the team decides the low-probability drop during the restore window is acceptable compared with the relaunch corruption it prevents, that is a project call. But I think it should be acknowledged as such, ideally with a follow-up issue tracking either buffering, replay, or moving the dedup boundary. |
1 participant
Closes #584
Summary
MostroService._onDatawas saving relay-replayed events tomostroStoragewithDateTime.now()timestamps. Those were newer than the synthetic restore messages (which useorderDetail.createdAt). On relaunch,sync()replayed them last, overwriting the correct restored state.addMessagein_onDatawhenisRestoringProvideris true. Event IDs still register ineventStoragefor dedup.How to test
Summary by CodeRabbit