fix(#581): adopt the radio's RESP_CODE_SENT ACK hash for send correlation

The radio is authoritative for the expected-ACK hash it reports, but the
client looked that hash up in a map keyed by a hash it recomputed locally.
When the two disagreed the lookup missed silently (debugPrint only), the
8000ms watchdog from #395 marked the message failed, and the genuine ACK
later matched nothing because every downstream map is populated only on the
match path. Delivery worked the whole time.

Captured on a Wadamesh HV4 TFT in #449: five DMs, all shown as errors, two
with a confirmed ACK whose hash equalled the radio's RESP_CODE_SENT value
exactly. Wadamesh builds the payload differently, so the client cannot
predict its digest, and Offband is expected to work against stock MeshCore,
Wadamesh and other forks.

RESP_CODE_SENT is the reply to our own CMD_SEND_TXT_MSG, so adopt the radio's
value when exactly one send awaits confirmation. Zero or several candidates
keep the previous behaviour rather than guessing. Channel sends draw the same
frame, so adoption is refused while one is outstanding.

The lookup miss is now a warn on debugLogService instead of a bare
debugPrint (SAFELANE section 6).

A send-order correlation queue was developed alongside this and has been
split out: its ordering premise needs a transport send mutex that does not
exist yet, and three review rounds each found a fresh defect in it. This
commit deliberately carries only the adoption fallback, which is what the
owner's hardware test validated.

Refs #449

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pull/680/head
Strycher 1 month ago committed by Benjamin Wiechel
parent bd43642d72
commit edffd10555

@ -7110,8 +7110,15 @@ class MeshCoreConnector extends ChangeNotifier {
} }
final retryService = _retryService; final retryService = _retryService;
// Channel sends draw this same frame. While one is awaiting its own
// RESP_CODE_SENT the frame is ambiguous, so the retry service must not
// adopt an unpredicted hash into a direct message (#581).
if (retryService != null && if (retryService != null &&
retryService.updateMessageFromSent(ackHash, timeoutMs)) { retryService.updateMessageFromSent(
ackHash,
timeoutMs,
allowUnpredictedAdoption: _pendingChannelSentQueue.isEmpty,
)) {
return; return;
} }

@ -391,7 +391,74 @@ class MessageRetryService extends ChangeNotifier {
_onMessageResolved(messageId, contact.publicKeyHex); _onMessageResolved(messageId, contact.publicKeyHex);
} }
bool updateMessageFromSent(int ackHash, int timeoutMs) { /// Message ids whose send reached the radio but whose RESP_CODE_SENT has not
/// arrived yet.
List<String> get _sendsAwaitingConfirmation => _sentConfirmationTimers.keys
.where((id) => _pendingMessages[id]?.status == MessageStatus.pending)
.toList();
/// The radio is authoritative for the expected-ACK hash it reports in
/// RESP_CODE_SENT. Firmware forks build the message payload differently, so
/// the hash recomputed locally can disagree while the send itself is fine.
/// When that happened the message stayed pending, the 8s watchdog from #395
/// marked an already-delivered DM failed, and the genuine ACK later matched
/// nothing because every downstream map is only populated on the match path.
/// Captured on a Wadamesh radio in #449.
///
/// This frame is the reply to our own CMD_SEND_TXT_MSG, so adopt the radio's
/// value when exactly one send is awaiting confirmation. With zero or several
/// candidates the correlation would be a guess, so keep the old behaviour.
///
/// [allowed] is false when a channel message is also awaiting its
/// RESP_CODE_SENT. Channel sends share this frame, so adopting there could
/// attach a channel message's confirmation to an unrelated direct message.
String? _adoptUnpredictedSentHash(
String ackHashHex,
RetryServiceConfig config, {
required bool allowed,
}) {
if (!allowed) {
config.debugLogService?.warn(
'RESP_CODE_SENT: ACK hash $ackHashHex matches no pending message and a '
'channel send is also awaiting confirmation, not adopting',
tag: 'AckHash',
);
return null;
}
final awaiting = _sendsAwaitingConfirmation;
if (awaiting.length != 1) {
config.debugLogService?.warn(
'RESP_CODE_SENT: ACK hash $ackHashHex matches no pending message and '
'${awaiting.length} sends are awaiting confirmation, ignoring',
tag: 'AckHash',
);
return null;
}
final messageId = awaiting.first;
final message = _pendingMessages[messageId];
final text = message?.text ?? '';
final shortText = text.length > 20 ? '${text.substring(0, 20)}...' : text;
config.debugLogService?.warn(
'RESP_CODE_SENT: ACK hash $ackHashHex is not the hash we predicted, '
'adopting the radio value for "$shortText" (the radio is authoritative, '
'see #449)',
tag: 'AckHash',
);
// Drop the stale prediction so it cannot mis-match a later reply.
_expectedHashToMessageId.removeWhere((_, id) => id == messageId);
return messageId;
}
/// [allowUnpredictedAdoption] must be false when a channel message is also
/// awaiting its RESP_CODE_SENT, because that frame could belong to either.
bool updateMessageFromSent(
int ackHash,
int timeoutMs, {
bool allowUnpredictedAdoption = true,
}) {
final config = _config; final config = _config;
if (config == null) return false; if (config == null) return false;
@ -424,8 +491,15 @@ class MessageRetryService extends ChangeNotifier {
} }
if (messageId == null || contact == null) { if (messageId == null || contact == null) {
debugPrint('No pending message found for ACK hash: $ackHashHex'); final adopted = _adoptUnpredictedSentHash(
return false; ackHashHex,
config,
allowed: allowUnpredictedAdoption,
);
if (adopted == null) return false;
messageId = adopted;
contact = _pendingContacts[adopted];
if (contact == null) return false;
} }
final message = _pendingMessages[messageId]!; final message = _pendingMessages[messageId]!;

@ -709,4 +709,238 @@ void main() {
}, },
); );
}); });
group('RESP_CODE_SENT correlation uses the radio hash (#449/#581)', () {
// The radio is authoritative for the expected-ACK hash. Firmware forks
// (Wadamesh on the HV4 TFT, in the #449 capture) build the payload
// differently, so the client's locally recomputed hash can disagree while
// delivery works perfectly. Correlation must not depend on that guess.
const int radioHash = 0xDEADBEEF; // deliberately not the client's value
test('a RESP_CODE_SENT hash the client did not predict still marks the '
'message sent, and its ACK still marks it delivered', () async {
final retryService = MessageRetryService();
final contact = _makeContact(
publicKey: recipientKey,
pathLength: 2,
path: const [0x10, 0x20],
);
final updates = <Message>[];
retryService.initialize(
RetryServiceConfig(
sendMessage: (_, _, _, _) async {},
addMessage: (_, _) {},
updateMessage: updates.add,
getSelfPublicKey: () => fixedKey,
),
);
await retryService.sendMessageWithRetry(
contact: contact,
text: 'Weird that I am getting errors though.',
);
await Future<void>.delayed(const Duration(milliseconds: 20));
final matched = retryService.updateMessageFromSent(radioHash, 4884);
expect(
matched,
isTrue,
reason:
'the radio replied to our own CMD_SEND_TXT_MSG, so it must be '
'correlated even though the hash is not the one we predicted',
);
expect(
updates.last.status,
equals(MessageStatus.sent),
reason:
'an unpredicted hash must not leave the message pending, '
'because the 8s watchdog would then mark a delivered DM failed',
);
// The real ACK carries the radio's hash, not ours.
retryService.handleAckReceived(radioHash, 7354);
expect(
updates.last.status,
equals(MessageStatus.delivered),
reason: 'the ACK must resolve against the radio-supplied hash',
);
retryService.dispose();
});
test('an unpredicted hash is not adopted while two sends are awaiting '
'confirmation, because the correlation would be a guess', () async {
final retryService = MessageRetryService();
final contactA = _makeContact(publicKey: recipientKey, pathLength: 2);
final contactB = _makeContact(publicKey: _makeKey(0x55), pathLength: 2);
final updates = <Message>[];
retryService.initialize(
RetryServiceConfig(
sendMessage: (_, _, _, _) async {},
addMessage: (_, _) {},
updateMessage: updates.add,
getSelfPublicKey: () => fixedKey,
),
);
// Per-contact queues, so two contacts means two in flight at once.
await retryService.sendMessageWithRetry(contact: contactA, text: 'a');
await retryService.sendMessageWithRetry(contact: contactB, text: 'b');
await Future<void>.delayed(const Duration(milliseconds: 20));
expect(
retryService.updateMessageFromSent(radioHash, 4884),
isFalse,
reason:
'with two candidates the owner would rather keep the old '
'behaviour than attach the confirmation to the wrong message',
);
retryService.dispose();
});
test('an unpredicted hash is not adopted while a channel send is also '
'awaiting confirmation, because that frame could be either', () async {
final retryService = MessageRetryService();
final contact = _makeContact(publicKey: recipientKey, pathLength: 2);
final updates = <Message>[];
retryService.initialize(
RetryServiceConfig(
sendMessage: (_, _, _, _) async {},
addMessage: (_, _) {},
updateMessage: updates.add,
getSelfPublicKey: () => fixedKey,
),
);
await retryService.sendMessageWithRetry(contact: contact, text: 'dm');
await Future<void>.delayed(const Duration(milliseconds: 20));
// The connector passes false while _pendingChannelSentQueue is not
// empty. Adopting here would attach a channel message's confirmation
// to this DM and lose both.
expect(
retryService.updateMessageFromSent(
radioHash,
4884,
allowUnpredictedAdoption: false,
),
isFalse,
);
expect(
updates.every((m) => m.status != MessageStatus.sent),
isTrue,
reason: 'the DM must not be marked sent off an ambiguous frame',
);
retryService.dispose();
});
test(
'an unpredicted hash with nothing awaiting confirmation is ignored',
() {
final retryService = MessageRetryService();
retryService.initialize(
RetryServiceConfig(
sendMessage: (_, _, _, _) async {},
addMessage: (_, _) {},
updateMessage: (_) {},
getSelfPublicKey: () => fixedKey,
),
);
expect(
retryService.updateMessageFromSent(radioHash, 4884),
isFalse,
reason:
'with no send in flight the frame must fall through to the '
'channel handler, not be swallowed',
);
retryService.dispose();
},
);
test(
'a late stray frame is not adopted once the message has resolved',
() async {
final retryService = MessageRetryService();
final contact = _makeContact(publicKey: recipientKey, pathLength: 2);
final updates = <Message>[];
retryService.initialize(
RetryServiceConfig(
sendMessage: (_, _, _, _) async {},
addMessage: (_, _) {},
updateMessage: updates.add,
getSelfPublicKey: () => fixedKey,
),
);
await retryService.sendMessageWithRetry(contact: contact, text: 'one');
await Future<void>.delayed(const Duration(milliseconds: 20));
expect(retryService.updateMessageFromSent(radioHash, 4884), isTrue);
retryService.handleAckReceived(radioHash, 5000);
expect(updates.last.status, equals(MessageStatus.delivered));
// A second, unrelated unpredicted frame arrives afterwards. Nothing is
// awaiting confirmation now, so it must not be attached to anything.
expect(
retryService.updateMessageFromSent(0xFEEDFACE, 4884),
isFalse,
reason: 'a stray frame must not be adopted by a resolved message',
);
retryService.dispose();
},
);
test(
'a hash the client did predict still matches on the fast path',
() async {
final retryService = MessageRetryService();
final contact = _makeContact(publicKey: recipientKey, pathLength: 2);
final updates = <Message>[];
int? sentTs;
int? sentAttempt;
retryService.initialize(
RetryServiceConfig(
sendMessage: (_, _, attempt, ts) async {
sentAttempt = attempt;
sentTs = ts;
},
addMessage: (_, _) {},
updateMessage: updates.add,
getSelfPublicKey: () => fixedKey,
),
);
await retryService.sendMessageWithRetry(contact: contact, text: 'Yep.');
await Future<void>.delayed(const Duration(milliseconds: 20));
final predicted = _manualAckHash(
sentTs!,
sentAttempt! & 0x03,
'Yep.',
fixedKey,
);
expect(retryService.updateMessageFromSent(predicted, 3252), isTrue);
expect(updates.last.status, equals(MessageStatus.sent));
retryService.handleAckReceived(predicted, 1200);
expect(updates.last.status, equals(MessageStatus.delivered));
retryService.dispose();
},
);
});
} }

Loading…
Cancel
Save

Powered by TurnKey Linux.