fix(mqtt): do not send an NTP request to a name that failed to resolve
Found on hardware while validating the SNTP fix. `set mqtt.ntp bogus.invalid`
reported SUCCESS with a correct epoch, in 4 s, with no retry and without ever
reaching the SNTP fallback:
[E] hostByName(): DNS Failed for bogus.invalid
[E] beginPacket(): could not get host from dns: 11
MQTT: Time synced: 1786764354 (via bogus.invalid)
Three pieces compose it. WiFiUDP::beginPacket(const char*, port) returns 0 on a
DNS failure and leaves remote_ip/remote_port at their previous values.
NTPClient::sendNTPPacket() discards that return and calls endPacket() regardless.
endPacket() sends to whatever remote_ip still holds. So the request went to the
pool address resolved at boot, that server answered with a genuine timestamp, and
the loop recorded ntp_server_used as the name that had never been contacted.
This sits one layer above the fallback that b1ceaf01 made honest — control never
reaches it — so `set mqtt.ntp <typo>`, whose whole purpose is to fail fast, still
reported OK and the fleet kept a server name it had never spoken to.
The DNS pre-check was already here and only logged a warning. Make it decide:
skip a name that does not resolve rather than attempt a send that cannot go where
it claims. IP literals are unaffected — hostByName() returns them via
fromString() without a lookup — and the lookup already ran, so no latency is
added. Moved setPoolServerName() below it so the client is never pointed at a
server being skipped.
Residual, narrower window: our lookup succeeds and NTPClient's own
gethostbyname() then fails, which needs the entry to leave the lwIP cache between
two calls microseconds apart. Closing it properly needs the resolved IP handed to
NTPClient, and this version exposes no setPoolServerIP(); the constructor is the
only way in.
Not host-testable — NTPClient and WiFiUDP both. Verified by inspection of both
library sources plus the captured hardware trace above.
This commit is contained in:
1 parent
436bb65ae6
commit
9597d9ca52
1 file changed
+12
-2
@@ -3954,15 +3954,25 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) {
|
||||
const int kMaxNtpRetriesPerServer = 2;
|
||||
for (int s = 0; s < server_count && !ntp_ok; s++) {
|
||||
const char* server = servers[s];
|
||||
_ntp_client.setPoolServerName(server);
|
||||
|
||||
#ifdef ESP_PLATFORM
|
||||
// Authoritative, not advisory. NTPClient::sendNTPPacket() ignores what
|
||||
// beginPacket() returns, and WiFiUDP leaves remote_ip/remote_port at the previous
|
||||
// destination when a name fails to resolve — so asking an unresolvable host sends
|
||||
// the request to whichever server resolved last, and that server's genuine reply
|
||||
// gets credited to this name. Observed on d4: `set mqtt.ntp bogus.invalid` reported
|
||||
// success with a correct epoch, answered by the pool address left over from boot.
|
||||
// Skipping is what keeps the credit honest; the name that answered is the name
|
||||
// recorded.
|
||||
IPAddress resolved_ip;
|
||||
if (!WiFi.hostByName(server, resolved_ip)) {
|
||||
MQTT_DEBUG_PRINTLN("WARNING: DNS resolution failed for %s - NTP sync may fail", server);
|
||||
MQTT_DEBUG_PRINTLN("NTP: %s does not resolve — skipping, not attempting a send", server);
|
||||
continue;
|
||||
}
|
||||
#endif
|
||||
|
||||
_ntp_client.setPoolServerName(server);
|
||||
|
||||
for (int attempt = 1; attempt <= kMaxNtpRetriesPerServer && !ntp_ok; attempt++) {
|
||||
if (attempt > 1) {
|
||||
MQTT_DEBUG_PRINTLN("NTP retry %d/%d on %s...", attempt, kMaxNtpRetriesPerServer, server);
|
||||
|
||||
Reference in new issue
Block a user