Skip to content

fix(mobile): preserve channel group edits and sync sorting #2829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tellaho wants to merge 2 commits into from
Closed

fix(mobile): preserve channel group edits and sync sorting #2829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c397f5
fix(mobile): protect unpublished channel-section edits and sync chann…
Jul 25, 2026
83271da
fix(mobile): retry channel-sort startup sync when relay rate-limits c…
Jul 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 73 additions & 7 deletions mobile/lib/features/channels/channel_sections/channel_sections_manager.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,17 @@ class ChannelSectionsManager {
void Function()? _unsubscribe;
bool _disposed = false;

/// Unix seconds of the oldest unpublished local edit; 0 when clean.
/// Persisted so unpublished edits survive manager teardown — the provider
/// rebuilds this manager on every relay-session status flip, which would
/// otherwise drop the pending debounce and let a stale remote blob clobber
/// newer local state on the next fetch.
int _dirtySince;

/// Bumped on every local edit so a publish only clears the dirty flag when
/// no new edit landed while the publish was in flight.
int _editGeneration = 0;

ChannelSectionsManager({
required this.pubkey,
required SharedPreferences prefs,
Expand All @@ -62,10 +73,14 @@ class ChannelSectionsManager {
_signedEventRelay = signedEventRelay,
_remoteEnabled = remoteEnabled,
_onChanged = onChanged,
_store = ChannelSectionsStorage(prefs).read(pubkey);
_store = ChannelSectionsStorage(prefs).read(pubkey),
_dirtySince = ChannelSectionsStorage(prefs).readDirtySince(pubkey);

ChannelSectionStore get store => _store;

@visibleForTesting
bool get isDirty => _dirtySince > 0;

Future<void> initialize() async {
if (_disposed) return;

Expand All @@ -74,8 +89,19 @@ class ChannelSectionsManager {
return;
}

await _fetchAndMerge();
final foundRemote = await _fetchAndMerge();
await _startLiveSubscription();

// Publish-on-reconnect: unpublished local edits (persisted dirty flag)
// survive teardown/rebuild and get flushed once we're connected again.
// Seed-publish: if the relay confirmed it has no blob at all but local
// state exists, push local up so other clients can sync. `foundRemote`
// is null when the fetch failed — never seed-publish on a failed fetch.
if (_dirtySince > 0 ||
(foundRemote == false &&
(_store.sections.isNotEmpty || _store.assignments.isNotEmpty))) {
markDirty();
}
_onChanged();
}

Expand Down Expand Up @@ -193,16 +219,27 @@ class ChannelSectionsManager {
}

void markDirty() {
if (!_remoteEnabled || _disposed) return;
if (_disposed) return;
// Record dirtiness even while offline (remote disabled) — the flag is
// persisted, so the reconnect-time manager re-publishes instead of
// letting the stale remote blob clobber offline edits.
if (_dirtySince == 0) {
_dirtySince = currentUnixSeconds();
_storage.writeDirtySince(pubkey, _dirtySince);
}
_editGeneration++;
if (!_remoteEnabled) return;
_publishDebounce?.cancel();
_publishDebounce = Timer(const Duration(seconds: 5), () {
_publishDebounce = null;
unawaited(_publish());
});
}

Future<void> _fetchAndMerge() async {
if (_relaySession == null) return;
/// Returns whether the relay reported a `channel-sections` blob, or null
/// when the fetch failed (offline / relay error).
Future<bool?> _fetchAndMerge() async {
if (_relaySession == null) return null;
try {
final events = await _relaySession.fetchHistory(
NostrFilter(
Expand All @@ -217,8 +254,12 @@ class ChannelSectionsManager {
_mergeEvents(events);
_persist();
if (!_disposed) _onChanged();
return events.any(
(e) => e.pubkey == pubkey && e.getTagValue('d') == 'channel-sections',
);
} catch (_) {
// Local state remains usable when relay is unavailable.
return null;
}
}

Expand Down Expand Up @@ -269,6 +310,13 @@ class ChannelSectionsManager {
if (isNewer) {
_lastRemoteCreatedAt = event.createdAt;

@wpfleger96 wpfleger96 Jul 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CRITICAL — dirty-state protection silently and permanently discards legitimate remote edits, then overwrites them. Flagged independently by all three reviewers.

The ordering is the defect. When isNewer is true, this advances _lastRemoteCreatedAt and _lastRemoteEventId to the incoming event, and then line 319 bails with if (_dirtySince > 0) return; — dropping incoming entirely. Walk a real sequence:

  1. Mobile is dirty — one unpublished edit, say a renamed group.
  2. On desktop the user creates a new group. Desktop publishes at T+10.
  3. Mobile's _publish calls _fetchAndMerge (line 363), pulls the desktop event, sets _lastRemoteCreatedAt = T+10 and _lastRemoteEventId = <desktop id>, then returns here without storing it.
  4. Mobile publishes with createdAt = max(currentUnixSeconds(), _lastRemoteCreatedAt + 1) (line 378) — deliberately sorting above desktop's event — carrying a blob that never contained desktop's new group.
  5. Desktop adopts it by whole-blob LWW. The desktop-created group is now gone on both clients.

It is also unrecoverable, not merely deferred. Because _lastRemoteEventId was advanced in step 3, a later redelivery of that same desktop event fails the isNewer test forever — even after the dirty flag clears.

The comment at 313-318 says "the pending publish then reconciles the relay." It doesn't reconcile; it overwrites. That's the crux: a local dirty veto on the read combined with an unconditional win on the write is guaranteed clobber, not conflict resolution. The PR's goal is preventing edit loss on mobile, and this achieves it by relocating the identical bug onto desktop. Whole-blob LWW plus a one-sided veto can't be made correct.

Severity is amplified by the blob shape: channel-sections is one whole-blob record, so "one dropped event" means every section and assignment the other client created in it.

Fix (preferred) — bail before touching the cursor, and stash the dropped state for reconciliation:

if (isNewer) {
  if (_dirtySince > 0) {
    _deferredRemote = incoming;   // reconcile after the pending publish lands
    return;                       // cursor NOT advanced
  }
  _lastRemoteCreatedAt = event.createdAt;
  _lastRemoteEventId = event.id;
  _store = incoming;
  _persist();
}

Then field-merge _deferredRemote into _store before publishing, so both edits survive.

Fix (minimum, if a real merge is out of scope for this PR) — move the bail above the two assignments so the event stays re-adoptable once clean, and don't let the publish createdAt leapfrog an unmerged remote event. That leaves a window where the two clients disagree, but nothing is destroyed.

Note this applies identically at channel_sort_manager.dart:286-290, which is the same code — see the duplication note in the review summary.

_lastRemoteEventId = event.id;
// Dirty-state protection: never let a remote blob overwrite
// unpublished local edits. Whole-blob LWW would otherwise adopt a
// stale remote store fetched right after a teardown/rebuild and
// silently drop the just-made edit. We still advance
// _lastRemoteCreatedAt above so our eventual publish sorts after the
// remote event; the pending publish then reconciles the relay.
if (_dirtySince > 0) return;
_store = incoming;
_persist();
}
Expand Down Expand Up @@ -311,11 +359,18 @@ class ChannelSectionsManager {
return;
}

// Read-before-write: merge remote state before publishing
final generationAtStart = _editGeneration;

// Read-before-write: advance _lastRemoteCreatedAt past any remote blob so
// our event sorts after it. Dirty-state protection in _mergeEvent keeps
// the fetched blob from clobbering the unpublished local store.
await _fetchAndMerge();

// No-op suppression: skip if nothing changed
if (_isIdenticalToLastPublished()) return;
if (_isIdenticalToLastPublished()) {

@wpfleger96 wpfleger96 Jul 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR — _isIdenticalToLastPublished can report equality for two different assignment maps.

(Anchored at the call site; the defect is in the method body at lines 348-351, unchanged by this PR but newly load-bearing now that this branch clears the persisted dirty flag.)

The loop iterates only _store.assignments.keys, so a key present in last but absent from _store is never examined — the lookup for a key not in _store is simply never performed. Equal lengths are checked at line 338, so the only hole is an equal-size key swap: one channel unassigned and a different one assigned within the same debounce window. assignChannel + unassignChannel on different channels between publishes does exactly that.

The consequence is a suppressed publish, so the relay keeps the old assignment map and the local change is silently not synced — and with the dirty flag cleared on that path (line 371), it isn't retried either.

Low likelihood, real consequence. Fix: check containment in both directions.

if (last.assignments.length != _store.assignments.length) return false;
for (final entry in _store.assignments.entries) {
  if (!last.assignments.containsKey(entry.key)) return false;
  if (last.assignments[entry.key] != entry.value) return false;
}

The positional comparison of sections above is fine by contrast — a reordered list compares unequal, which only costs a redundant publish.

_clearDirty(generationAtStart);

@wpfleger96 wpfleger96 Jul 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR — clearing the dirty flag on the no-op-suppression path is safe today only by accident.

_isIdenticalToLastPublished() returns false when _lastPublishedStore == null (line 336), so neither a fresh manager nor one rebuilt from the persisted dirty flag can reach this line — the dirty flag can't be cleared without a publish having happened in this instance. That's the saving grace, and it holds.

But it's incidental rather than stated. If _lastPublishedStore ever gets seeded from persistence — a natural follow-up, since everything else here is now durable — this line starts clearing the persisted dirty flag for a store the relay never received.

One reviewer initially rated this CRITICAL on the grounds that _lastPublishedStore can drift from relay truth. It can, but only as a consequence of the CRITICAL at line 311 (a remote blob dropped while dirty), so it resolves when that does rather than needing an independent fix.

Fix: either add a comment making the _lastPublishedStore == null dependency explicit, or don't clear dirty in the no-op branch at all and let the next real publish clear it — a redundant publish of an unchanged blob is much cheaper than a lost edit.

Same code at channel_sort_manager.dart:331.

return;
}

try {
final payload = jsonEncode(_store.toJson());

@wpfleger96 wpfleger96 Jul 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMPORTANT — one future-dated remote event permanently wedges publishing for this d-tag.

(Anchored here; the line in question is final createdAt = max(currentUnixSeconds(), _lastRemoteCreatedAt + 1); just below, which this PR does not itself modify but now makes durable-state-dependent.)

createdAt = max(currentUnixSeconds(), _lastRemoteCreatedAt + 1) inherits whatever _lastRemoteCreatedAt was set to from a remote event, and _mergeEvent applies no plausibility check before advancing it. The relay rejects any event more than ±900s from server time (crates/buzz-relay/src/handlers/ingest.rs:1506-1513).

So a single blob published with created_at = now + 20min — a desktop with a skewed clock is sufficient, no malice required — puts _lastRemoteCreatedAt 20 minutes ahead. Every subsequent publish then computes a createdAt the relay refuses, throws, leaves the dirty flag set (line 397), and gets retried on the next initialize(). The state is permanent: nothing lowers _lastRemoteCreatedAt, so section changes stop syncing from this device indefinitely, and the only visible symptom is a debugPrint.

This codebase already solved this. read_state_manager.dart:526-527 has:

bool _isPlausibleCreatedAt(int createdAt) =>
    createdAt <= currentUnixSeconds() + readStateMaxClockDriftSeconds;

guarding its own cursor advance at :236 and :307, with readStateMaxClockDriftSeconds = 300 in read_state_time.dart:25 — comfortably inside the relay's 900s. These two new managers just don't call it.

Fix: reuse the existing helper — refuse to advance _lastRemoteCreatedAt for an event failing _isPlausibleCreatedAt, and clamp the publish createdAt to currentUnixSeconds() + readStateMaxClockDriftSeconds. read_state_manager.dart also has _isPermanentReadStateRemoteError (:529) to stop retrying rejections that will never succeed; worth mirroring, since neither of these managers distinguishes a transient failure from a permanent one.

Same code at channel_sort_manager.dart:338.

Expand All @@ -337,11 +392,22 @@ class ChannelSectionsManager {
sections: List.of(_store.sections),
assignments: Map.of(_store.assignments),
);
_clearDirty(generationAtStart);
} catch (error) {
debugPrint('[ChannelSectionsManager] publish failed: $error');
// Dirty flag stays set; the next initialize() re-schedules the publish.
}
}

/// Clears the persisted dirty flag unless a new edit landed while the
/// publish that succeeded was in flight.
void _clearDirty(int generationAtStart) {
if (_editGeneration != generationAtStart) return;
if (_dirtySince == 0) return;

@wpfleger96 wpfleger96 Jul 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMPORTANT — the dirty flag is persisted per-pubkey but _editGeneration is in-memory per-instance, so one manager can clear another's dirty state and strand an unpublished edit.

_clearDirty guards with _editGeneration != generationAtStart, which correctly catches an edit landing during an in-flight publish within one instance. It cannot see edits made on a different instance — and instances are routinely concurrent here, because channel_sections_provider.dart:73-79 registers ref.onDispose(() => manager.dispose()) with the default flushPending: true, which fires unawaited(_publish(allowDisposed: true)) (line 142) on a manager that is being torn down while its replacement is already being constructed.

The race:

  1. Manager A is dirty with a publish in flight (generationAtStart captured, _editGeneration == 3).
  2. Provider rebuild — relay status flip — tears A down. A's flush keeps running.
  3. Manager B is constructed, reads _dirtySince > 0 from prefs, and accepts a new local edit. B's own _editGeneration starts at 0 and is unrelated to A's.
  4. A's publish succeeds and calls _clearDirty(3). A's counter still equals 3, so the guard passes and A writes dirty = 0 for the pubkey.
  5. B's edit is now unpublished but no longer marked dirty. It won't survive a restart, and — worse — the next remote blob will overwrite it, since the dirty veto at line 319 is what was protecting it.

The persisted flag is shared state; the guard protecting it is not. Fix: make the persisted value the thing you compare against — store a monotonic edit token alongside dirty-since and have _clearDirty re-read it, clearing only when it still equals the value observed at publish start (compare-and-clear). A cheaper variant: have _clearDirty re-read _storage.readDirtySince(pubkey) and skip clearing when it differs from the _dirtySince this instance captured before publishing.

Also note the flush is only attempted when hadPending is true (line 137) — a manager that is dirty with no live debounce timer is torn down with no flush at all, relying entirely on the persisted flag that this race can clear.

Same code at channel_sort_manager.dart:361-366.

_dirtySince = 0;
_storage.writeDirtySince(pubkey, 0);
}

void _persist() {
_storage.write(pubkey, _store);
}
Expand Down
17 changes: 17 additions & 0 deletions mobile/lib/features/channels/channel_sections/channel_sections_storage.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import 'package:shared_preferences/shared_preferences.dart';

String channelSectionsKey(String pubkey) => 'buzz.channel-sections.v1:$pubkey';

String channelSectionsDirtySinceKey(String pubkey) =>
'buzz.channel-sections.dirty-since.v1:$pubkey';

class ChannelSection {
final String id;
final String name;
Expand Down Expand Up @@ -113,4 +116,18 @@ class ChannelSectionsStorage {
void write(String pubkey, ChannelSectionStore store) {
_prefs.setString(channelSectionsKey(pubkey), jsonEncode(store.toJson()));
}

/// Unix seconds of the oldest unpublished local edit, or 0 when local state
/// has been fully published. Persisted so unpublished edits survive manager
/// teardown (relay status flips rebuild the provider) and app restarts.
int readDirtySince(String pubkey) =>
_prefs.getInt(channelSectionsDirtySinceKey(pubkey)) ?? 0;

void writeDirtySince(String pubkey, int dirtySince) {
if (dirtySince <= 0) {
_prefs.remove(channelSectionsDirtySinceKey(pubkey));
} else {
_prefs.setInt(channelSectionsDirtySinceKey(pubkey), dirtySince);
}
}
}
Loading
Loading