fix(#1060): drop erased records on load, and pace the restore writes

Adversarial review (gemini-2.5-pro) on the push candidate raised five
findings. Two were real and are fixed here; the other three are justified
in the PR body.

Storage is an untrusted source too. The guards already on this branch stop
an erased (all-0xFF) record being adopted from the WIRE, but a build from
before they existed could have persisted one, and loadCachedChannels
applied no filter. _cachedChannels is a fallback identity source for
_findChannelByIndex and is re-saved by the unread paths, so such a record
comes back as the channel for live traffic and outlives every radio. It is
dropped at the load boundary, which protects every consumer at once. It
carries nothing recoverable: the name is U+FFFD and the PSK is all-ones.

restoreChannelsToRadio sent its writes in an unpaced loop. On BLE sendFrame
prefers write-without-response, so its await returns when the frame reaches
the OS and not when the radio has taken it: a multi-channel restore could
overrun the radio and lose channels with no error raised anywhere. The USB
branch of sendFrame already spaces its own frames for exactly this reason.
Same 10ms gap between consecutive channel writes.

The restore result is documented as written and NOT confirmed. Counting
confirmations after the re-sync was tried and reverted: getChannels returns
once the first request is away rather than once the sync has finished, so
it would have read a list the radio had not answered for and reported zero
every time. The erased-slot banner is re-derived on every sync and stays up
if writes did not land, so it remains the authoritative signal.

Both new guards proved against planted defects rather than assumed:
removing the load filter turns 2 tests red, and removing the pacing yields
"Expected: a value greater than or equal to <20>, Actual: <0>".

flutter analyze clean, 1896 tests pass, 1 skipped.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
feat/1068-radio-channel-notify
diffwireauto 7 days ago
parent 16a98e5085
commit bd0744e472

@ -619,6 +619,11 @@ class MeshCoreConnector extends ChangeNotifier {
List<Channel> _previousChannelsCache = [];
static const int _maxChannelSyncRetries = 3;
/// Gap between consecutive channel writes in [restoreChannelsToRadio], so a
/// multi-channel restore cannot outrun the radio. Matches the spacing the USB
/// branch of [sendFrame] already applies per frame (#1060).
static const Duration _channelRestoreFrameGap = Duration(milliseconds: 10);
// #1062: fail-closed channel sync. A slot the device never answered for is
// UNKNOWN: tracked here, surfaced to the UI, and never backfilled from a
// previous list (that backfill is how a stale slot map misrouted
@ -2542,7 +2547,24 @@ class MeshCoreConnector extends ChangeNotifier {
}
Future<void> loadCachedChannels() async {
_cachedChannels = await _channelStore.loadChannels();
// #1060: a build from before `isErased` existed could persist an erased
// (all-0xFF) record over the good list, so storage itself may hold records
// that are not channel identities. Drop them at the boundary rather than
// at each use: [_cachedChannels] is a fallback identity source for
// [_findChannelByIndex] and is re-saved by the unread paths, so one that
// slips in here comes back as the channel for live traffic and outlives
// every radio. They carry nothing recoverable (name is U+FFFD, PSK is
// all-ones), so dropping them loses no information.
final loaded = await _channelStore.loadChannels();
final usable = loaded.where((c) => !c.isUnusable).toList();
if (usable.length != loaded.length) {
_appDebugLogService?.warn(
'Dropped ${loaded.length - usable.length} unusable channel record(s) '
'from storage on load; a pre-#1060 build had persisted erased slots',
tag: 'ChannelSync',
);
}
_cachedChannels = usable;
_recalculateCachedChannelsUnreadTotal();
}
@ -4465,6 +4487,12 @@ class MeshCoreConnector extends ChangeNotifier {
void setContactSettingsKeyForTest(String radioKeyHex) =>
_contactSettingsStore.setPublicKeyHex = radioKeyHex;
/// Scopes the channel store without a connect handshake, so the persisted
/// load path can be exercised directly. (#1060)
@visibleForTesting
void setChannelStoreKeyForTest(String radioKeyHex) =>
_channelStore.setPublicKeyHex = radioKeyHex;
/// Feeds a device frame through the normal dispatch, so parse paths can be
/// exercised without a radio. (#578)
@visibleForTesting
@ -7124,7 +7152,8 @@ class MeshCoreConnector extends ChangeNotifier {
/// clears history: the identities being restored are the ones that own it.
///
/// Returns the number of channels written, or -1 when there was nothing to
/// restore or the link is down.
/// restore or the link is down. Written is not the same as confirmed: the
/// erased-slot banner is what tells the user the radio took them.
Future<int> restoreChannelsToRadio() async {
if (!isConnected) return -1;
final toRestore = _cachedChannels.where((c) => !c.isUnusable).toList();
@ -7136,19 +7165,34 @@ class MeshCoreConnector extends ChangeNotifier {
tag: 'ChannelSync',
);
var written = 0;
var sent = 0;
for (final channel in toRestore) {
if (!isConnected) break;
await sendFrame(
buildSetChannelFrame(channel.index, channel.name, channel.psk),
);
written++;
sent++;
// Space the writes. On BLE [sendFrame] prefers write-without-response,
// which returns once the frame reaches the OS and not once the radio has
// taken it, so an unpaced loop can overrun the radio and lose channels
// with no error anywhere. The USB branch of [sendFrame] already spaces
// its own frames for exactly this reason; BLE has no such pause.
if (sent < toRestore.length) {
await Future<void>.delayed(_channelRestoreFrameGap);
}
}
// The radio now holds these again, so a fresh sync should confirm them and
// the erased-slot block clears itself on the next pass.
//
// This deliberately does NOT report how many the radio confirmed:
// [getChannels] returns once the first request is away, not once the sync
// has finished, so counting confirmations here would read a list the radio
// has not answered for yet and report 0 every time. The authoritative
// feedback is the erased-slot banner, which is re-derived on every sync and
// therefore stays up if some writes did not land.
await getChannels(force: true);
return written;
return sent;
}
Future<void> deleteChannel(int index) async {

@ -887,7 +887,7 @@
}
}
},
"channels_restoreToRadioDone": "Restored {count} channel(s) to the radio.",
"channels_restoreToRadioDone": "Wrote {count} channel(s) back to the radio. If the warning above stays, some did not take.",
"@channels_restoreToRadioDone": {
"placeholders": {
"count": {

@ -2908,6 +2908,12 @@ abstract class AppLocalizations {
/// **'Could not restore the channel list: nothing cached to restore, or the radio is not connected.'**
String get channels_restoreToRadioFailed;
/// No description provided for @channels_restoreToRadioPartial.
///
/// In en, this message translates to:
/// **'Only {written} of {total} channel(s) came back after the restore. The radio did not take the rest. Try the restore again.'**
String channels_restoreToRadioPartial(int written, int total);
/// No description provided for @channels_slotIdentityChanged.
///
/// In en, this message translates to:

@ -1640,6 +1640,11 @@ class AppLocalizationsBg extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1634,6 +1634,11 @@ class AppLocalizationsDe extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1616,6 +1616,11 @@ class AppLocalizationsEn extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1635,6 +1635,11 @@ class AppLocalizationsEs extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1640,6 +1640,11 @@ class AppLocalizationsFr extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1641,6 +1641,11 @@ class AppLocalizationsHu extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1636,6 +1636,11 @@ class AppLocalizationsIt extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1584,6 +1584,11 @@ class AppLocalizationsJa extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1579,6 +1579,11 @@ class AppLocalizationsKo extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1627,6 +1627,11 @@ class AppLocalizationsNl extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1646,6 +1646,11 @@ class AppLocalizationsPl extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1636,6 +1636,11 @@ class AppLocalizationsPt extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1637,6 +1637,11 @@ class AppLocalizationsRu extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1627,6 +1627,11 @@ class AppLocalizationsSk extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1624,6 +1624,11 @@ class AppLocalizationsSl extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1618,6 +1618,11 @@ class AppLocalizationsSv extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1632,6 +1632,11 @@ class AppLocalizationsUk extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -1565,6 +1565,11 @@ class AppLocalizationsZh extends AppLocalizations {
String get channels_restoreToRadioFailed =>
'Could not restore the channel list: nothing cached to restore, or the radio is not connected.';
@override
String channels_restoreToRadioPartial(int written, int total) {
return 'Only $written of $total channel(s) came back after the restore. The radio did not take the rest. Try the restore again.';
}
@override
String channels_slotIdentityChanged(String changes) {
return 'Channel layout changed on the radio: $changes';

@ -15,6 +15,7 @@ import 'package:flutter_test/flutter_test.dart';
import 'package:meshcore_open/connector/meshcore_connector.dart';
import 'package:meshcore_open/connector/meshcore_protocol.dart';
import 'package:meshcore_open/models/channel.dart';
import 'package:meshcore_open/storage/channel_store.dart';
import 'package:meshcore_open/storage/drift/blob_store.dart';
import 'package:meshcore_open/storage/drift/offband_database.dart';
import 'package:meshcore_open/storage/prefs_manager.dart';
@ -244,5 +245,94 @@ void main() {
expect(sent.where((f) => f[0] == cmdSetChannel), isEmpty);
},
);
// Adversarial review finding: on BLE sendFrame prefers write-without-
// response, so its await returns when the frame reaches the OS and not
// when the radio has taken it. An unpaced loop can therefore overrun the
// radio and drop channels with no error raised anywhere. The USB branch
// of sendFrame already spaces its frames; this loop must too.
test(
'consecutive channel writes are spaced, not fired in a tight loop',
() async {
c.seedCachedChannelsForTest([
_channel(0, 'Public', 10),
_channel(1, 'wardriving', 40),
_channel(2, 'test', 70),
]);
sent.clear();
final sw = Stopwatch()..start();
final written = await c.restoreChannelsToRadio();
sw.stop();
expect(written, 3);
// Two gaps between three writes. A lower bound, so this asserts the
// pacing exists without pinning the exact duration.
expect(
sw.elapsedMilliseconds,
greaterThanOrEqualTo(20),
reason: 'three writes with no spacing would return almost instantly',
);
},
);
});
// Adversarial review finding: the guards added here stop an erased record
// being adopted from the WIRE, but a build from before they existed could
// already have persisted one. Storage is therefore an untrusted source too,
// and _cachedChannels is a fallback identity source for _findChannelByIndex
// as well as being re-saved by the unread paths, so one that slips in comes
// back as the channel for live traffic and outlives every radio.
group('erased records already sitting in storage', () {
const radioKey = 'aabbccddee';
test('are dropped on load, not adopted from the cache', () async {
final store = ChannelStore()..setPublicKeyHex = radioKey;
await store.saveChannels([
_channel(0, 'Public', 10),
Channel.fromFrame(_erasedFrame(1))!,
_channel(2, 'wardriving', 40),
]);
// The store itself is faithful: it persisted exactly what it was given,
// which is how the bad record got there in the first place.
expect((await store.loadChannels()).length, 3);
c.setChannelStoreKeyForTest(radioKey);
await c.loadCachedChannels();
expect(c.cachedChannels.map((ch) => ch.name), ['Public', 'wardriving']);
expect(c.cachedChannels.any((ch) => ch.isUnusable), isFalse);
});
test('a clean stored list is loaded untouched', () async {
final store = ChannelStore()..setPublicKeyHex = radioKey;
await store.saveChannels([
_channel(0, 'Public', 10),
_channel(1, 'wardriving', 40),
]);
c.setChannelStoreKeyForTest(radioKey);
await c.loadCachedChannels();
expect(c.cachedChannels.length, 2);
expect(c.cachedChannels.map((ch) => ch.name), ['Public', 'wardriving']);
});
test('an all-erased stored list leaves nothing to restore', () async {
final store = ChannelStore()..setPublicKeyHex = radioKey;
await store.saveChannels([
Channel.fromFrame(_erasedFrame(0))!,
Channel.fromFrame(_erasedFrame(1))!,
]);
c.setChannelStoreKeyForTest(radioKey);
await c.loadCachedChannels();
expect(c.cachedChannels, isEmpty);
// Nothing usable cached means the restore is refused rather than
// writing erased records back onto the radio.
expect(await c.restoreChannelsToRadio(), -1);
expect(sent.where((f) => f[0] == cmdSetChannel), isEmpty);
});
});
}

@ -141,6 +141,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -503,6 +504,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -865,6 +867,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -1227,6 +1230,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -1589,6 +1593,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -1951,6 +1956,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -2313,6 +2319,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -2675,6 +2682,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -3037,6 +3045,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -3399,6 +3408,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -3761,6 +3771,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -4123,6 +4134,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -4485,6 +4497,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -4847,6 +4860,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -5209,6 +5223,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -5571,6 +5586,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",
@ -5933,6 +5949,7 @@
"channels_restoreToRadioConfirm",
"channels_restoreToRadioDone",
"channels_restoreToRadioFailed",
"channels_restoreToRadioPartial",
"channels_slotIdentityChanged",
"common_dismiss",
"channels_notifications",

Loading…
Cancel
Save

Powered by TurnKey Linux.