From 9597d9ca5253c444518f726762fda0ce57ea4c9d Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 14 Aug 2026 20:55:23 -0700 Subject: [PATCH] fix(mqtt): do not send an NTP request to a name that failed to resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `, 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. --- src/helpers/bridges/MQTTBridge.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index 7afb2e28..1ce0f1bb 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -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);