From 0211bdf2c0f5911a8db04403692bc5a7cadf5b90 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 31 Aug 2026 19:26:01 -0400 Subject: [PATCH] Address review round: backoff fairness, watchdog integrity, silent failures --- esphome/components/api/api_connection.cpp | 3 + .../api/api_outgoing_connection.cpp | 66 ++++++++++++++++--- .../components/api/api_outgoing_connection.h | 5 ++ esphome/components/api/api_server.cpp | 21 +++++- esphome/components/api/api_server.h | 3 + .../api/test_outgoing_connection.py | 32 ++++++++- .../api/test_outgoing_connection_ipv6.yaml | 20 ++++++ 7 files changed, 137 insertions(+), 13 deletions(-) create mode 100644 tests/component_tests/api/test_outgoing_connection_ipv6.yaml diff --git a/esphome/components/api/api_connection.cpp b/esphome/components/api/api_connection.cpp index 0452400b50..11c87ef056 100644 --- a/esphome/components/api/api_connection.cpp +++ b/esphome/components/api/api_connection.cpp @@ -1784,6 +1784,9 @@ void APIConnection::complete_authentication_() { zwave_proxy::global_zwave_proxy->api_connection_authenticated(this); } #endif +#ifdef USE_API_OUTGOING_CONNECTION + this->parent_->on_client_authenticated(); +#endif } bool APIConnection::send_hello_response_(const HelloRequest &msg) { diff --git a/esphome/components/api/api_outgoing_connection.cpp b/esphome/components/api/api_outgoing_connection.cpp index 8b2463a6f2..42075d380c 100644 --- a/esphome/components/api/api_outgoing_connection.cpp +++ b/esphome/components/api/api_outgoing_connection.cpp @@ -35,7 +35,8 @@ void OutgoingConnectionManager::loop(APIServer *server) { return; // on_target_client() already reset the dial state } if (this->dialed_conn_ != nullptr) { - // Dialed peer still connected but unproven; its own timeouts free the slot + // A live dialed session (flagged or not, e.g. a host: peer) is the + // target; a silent one dies on the handshake timeout return; } const uint32_t now = App.get_loop_component_start_time(); @@ -58,12 +59,33 @@ void OutgoingConnectionManager::loop(APIServer *server) { } void OutgoingConnectionManager::try_dial_(APIServer *server, uint32_t now) { + if (!network::is_connected()) { + // Flips within seconds of boot; recheck fast so a deep sleep wake + // window is not spent waiting + this->schedule_wait_(now, NETWORK_RETRY_MS); + return; + } const char *host = this->target_host_(); - if (host == nullptr || !network::is_connected() || server->at_client_limit_() || !server->noise_ctx_.has_psk()) { - // Not a dial failure; retry soon without escalating the backoff + if (host == nullptr || server->at_client_limit_() || !server->noise_ctx_.has_psk()) { + ESP_LOGD(TAG, "Not dialing: %s", + host == nullptr ? "no target" + : server->at_client_limit_() ? "max connections" + : "no key"); + // Not a dial failure; retry without escalating the backoff this->schedule_wait_(now, PRECONDITION_RETRY_MS); return; } +#if !defined(USE_NETWORK_IPV6) && !defined(API_OUTGOING_CONNECTION_HOST) + if (strchr(host, ':') != nullptr) { + // Remembered by an earlier IPv6 build; set_sockaddr() would silently + // turn it into 255.255.255.255 here + ESP_LOGW(TAG, "Clearing unusable IPv6 target %s", host); + this->saved_.host[0] = '\0'; + this->target_pref_.save(&this->saved_); + this->schedule_wait_(now, PRECONDITION_RETRY_MS); + return; + } +#endif struct sockaddr_storage addr; socklen_t addr_len = socket::set_sockaddr((struct sockaddr *) &addr, sizeof(addr), host, API_OUTGOING_CONNECTION_PORT); @@ -86,6 +108,7 @@ void OutgoingConnectionManager::try_dial_(APIServer *server, uint32_t now) { if (this->dialed_conn_ == nullptr) { this->schedule_retry_(now); } else { + this->dial_handoff_ts_ = now; this->schedule_wait_(now, PRECONDITION_RETRY_MS); } return; @@ -138,7 +161,12 @@ void OutgoingConnectionManager::poll_connect_(APIServer *server, uint32_t now) { } int error = 0; socklen_t len = sizeof(error); - if (this->dial_socket_->getsockopt(SOL_SOCKET, SO_ERROR, &error, &len) != 0 || error != 0) { + if (this->dial_socket_->getsockopt(SOL_SOCKET, SO_ERROR, &error, &len) != 0) { + ESP_LOGW(TAG, "Connect status check failed: errno %d", errno); + this->schedule_retry_(now); + return; + } + if (error != 0) { ESP_LOGW(TAG, "Connect failed: %d", error); this->schedule_retry_(now); return; @@ -148,6 +176,7 @@ void OutgoingConnectionManager::poll_connect_(APIServer *server, uint32_t now) { this->schedule_retry_(now); return; } + this->dial_handoff_ts_ = now; // Hold unescalated until the peer proves itself or dies unproven this->schedule_wait_(now, PRECONDITION_RETRY_MS); } @@ -163,23 +192,40 @@ void OutgoingConnectionManager::schedule_retry_(uint32_t now) { } void OutgoingConnectionManager::on_client_removed(APIConnection *conn) { - if (conn == this->dialed_conn_) { - this->dialed_conn_ = nullptr; - this->schedule_retry_(App.get_loop_component_start_time()); + if (conn != this->dialed_conn_) { + return; + } + this->dialed_conn_ = nullptr; + const uint32_t now = App.get_loop_component_start_time(); + if (now - this->dial_handoff_ts_ >= DIAL_PROVEN_MS) { + // Outlived the handshake timeout, so it authenticated: a working peer + // (e.g. a host: target that never sends the flag) disconnected normally + this->backoff_ = BACKOFF_MIN_MS; + this->schedule_wait_(now, API_OUTGOING_CONNECTION_DELAY); + } else { + this->schedule_retry_(now); } } void OutgoingConnectionManager::on_target_client(APIConnection *conn) { // The target is connected; stop any dial in flight and reset the backoff. + // A dialed connection stays tracked unless it is this one: an inbound + // target must not orphan a still-open dial. this->dial_socket_.reset(); - this->dialed_conn_ = nullptr; + if (conn == this->dialed_conn_) { + this->dialed_conn_ = nullptr; + } this->state_ = DialState::DIAL_STATE_IDLE; this->backoff_ = BACKOFF_MIN_MS; #ifndef API_OUTGOING_CONNECTION_HOST SavedOutgoingTarget target{}; conn->get_peername_to(target.host); - if (target.host[0] == '\0' || strcmp(target.host, this->saved_.host) == 0) { - return; // unknown peer or unchanged; avoid flash wear + if (target.host[0] == '\0') { + ESP_LOGW(TAG, "Could not read peer address; not remembering target"); + return; + } + if (strcmp(target.host, this->saved_.host) == 0) { + return; // unchanged; avoid flash wear } if (!this->target_pref_.save(&target) || !global_preferences->sync()) { // Keep the old value so the save is retried on the next flagged hello diff --git a/esphome/components/api/api_outgoing_connection.h b/esphome/components/api/api_outgoing_connection.h index 2e0227a661..83f6cdc847 100644 --- a/esphome/components/api/api_outgoing_connection.h +++ b/esphome/components/api/api_outgoing_connection.h @@ -54,6 +54,10 @@ class OutgoingConnectionManager { static constexpr uint32_t BACKOFF_MAX_MS = 300000; static constexpr uint32_t CONNECT_TIMEOUT_MS = 10000; static constexpr uint32_t CONNECT_POLL_INTERVAL_MS = 250; + static constexpr uint32_t NETWORK_RETRY_MS = 500; + // Longer than the 60s handshake timeout: a dialed session still alive past + // this authenticated, so its death is a normal disconnect, not a bad dial + static constexpr uint32_t DIAL_PROVEN_MS = 65000; void try_dial_(APIServer *server, uint32_t now); void poll_connect_(APIServer *server, uint32_t now); @@ -94,6 +98,7 @@ class OutgoingConnectionManager { #endif uint32_t state_ts_{0}; uint32_t last_poll_{0}; + uint32_t dial_handoff_ts_{0}; // Byte-aligned types last #ifndef API_OUTGOING_CONNECTION_HOST diff --git a/esphome/components/api/api_server.cpp b/esphome/components/api/api_server.cpp index 7049e94b4e..d5b46e6640 100644 --- a/esphome/components/api/api_server.cpp +++ b/esphome/components/api/api_server.cpp @@ -217,6 +217,8 @@ void APIServer::remove_client_(uint8_t client_index) { this->outgoing_target_count_--; } this->outgoing_conn_.on_client_removed(client.get()); + // Read before the swap-and-reset below destroys the connection + const bool was_authenticated = client->is_authenticated(); #endif // Close socket now (was deferred from on_fatal_error to allow getpeername) @@ -239,7 +241,12 @@ void APIServer::remove_client_(uint8_t client_index) { // (suppressed while provisioning is pending - see loop()). if (this->api_connection_count_ == 0 && this->reboot_timeout_ != 0 && !this->provisioning_pending_()) { this->status_set_warning(LOG_STR("waiting for client connection")); - this->last_connected_ = App.get_loop_component_start_time(); +#ifdef USE_API_OUTGOING_CONNECTION + // An unauthenticated session (e.g. a dial to a host that accepts TCP but + // never speaks the API) must not keep resetting the reboot watchdog + if (was_authenticated) +#endif + this->last_connected_ = App.get_loop_component_start_time(); } #ifdef USE_API_CLIENT_DISCONNECTED_TRIGGER @@ -295,10 +302,20 @@ APIConnection *APIServer::add_outgoing_client_(std::unique_ptr s } auto *conn = new APIConnection(std::move(sock), this); conn->mark_outgoing(); - this->add_client_(conn); + // Unlike add_client_, no watchdog refresh: a dial that never authenticates + // must not keep petting the no-client reboot timeout + this->clients_[this->api_connection_count_++].reset(conn); + conn->start(); return conn; } +void APIServer::on_client_authenticated() { + if (this->reboot_timeout_ != 0 && !this->provisioning_pending_()) { + this->status_clear_warning(); + this->last_connected_ = App.get_loop_component_start_time(); + } +} + void APIServer::on_outgoing_target_client(APIConnection *conn) { this->outgoing_target_count_++; this->outgoing_conn_.on_target_client(conn); diff --git a/esphome/components/api/api_server.h b/esphome/components/api/api_server.h index e61645758c..2a7f9bef59 100644 --- a/esphome/components/api/api_server.h +++ b/esphome/components/api/api_server.h @@ -85,6 +85,9 @@ class APIServer final : public Component, #ifdef USE_API_OUTGOING_CONNECTION // Called by APIConnection when a client declares itself a dial-back target in its hello void on_outgoing_target_client(APIConnection *conn); + // Called by APIConnection on authentication so dialed sessions feed the + // reboot watchdog only once they are real + void on_client_authenticated(); #endif void handle_disconnect(APIConnection *conn); diff --git a/tests/component_tests/api/test_outgoing_connection.py b/tests/component_tests/api/test_outgoing_connection.py index c21f81161e..8936681e5d 100644 --- a/tests/component_tests/api/test_outgoing_connection.py +++ b/tests/component_tests/api/test_outgoing_connection.py @@ -5,7 +5,7 @@ from pathlib import Path import pytest -from esphome.components.api import CONFIG_SCHEMA +from esphome.components.api import CONFIG_SCHEMA, _validate_outgoing_host_ipv6 from esphome.components.esp32 import KEY_BOARD, KEY_VARIANT, VARIANT_ESP32 import esphome.config_validation as cv from esphome.const import PlatformFramework @@ -74,3 +74,33 @@ def test_outgoing_connection_rejects_hostnames( set_core_config(PlatformFramework.ESP32_IDF, platform_data=ESP32_PLATFORM_DATA) with pytest.raises(cv.Invalid, match="not a valid IP address"): CONFIG_SCHEMA(_api_config({"host": "homeassistant.local"})) + + +def test_outgoing_connection_ipv6_host_requires_ipv6( + set_core_config: SetCoreConfigCallable, +) -> None: + set_core_config(PlatformFramework.ESP32_IDF, platform_data=ESP32_PLATFORM_DATA) + config = CONFIG_SCHEMA(_api_config({"host": "fd00::1"})) + with pytest.raises(cv.Invalid, match="IPv6 is not"): + _validate_outgoing_host_ipv6(config) + + +def test_outgoing_connection_ipv6_host_passes_with_ipv6_enabled( + set_core_config: SetCoreConfigCallable, +) -> None: + set_core_config( + PlatformFramework.ESP32_IDF, + platform_data=ESP32_PLATFORM_DATA, + full_config={"network": {"enable_ipv6": True}}, + ) + config = CONFIG_SCHEMA(_api_config({"host": "fd00::1"})) + assert _validate_outgoing_host_ipv6(config) is config + + +def test_outgoing_connection_ipv6_host_with_ipv6( + generate_main: Callable[[str | Path], str], +) -> None: + generate_main("tests/component_tests/api/test_outgoing_connection_ipv6.yaml") + + defines = {define.name: define.value for define in CORE.defines} + assert str(defines["API_OUTGOING_CONNECTION_HOST"]) == '"fd00::1"' diff --git a/tests/component_tests/api/test_outgoing_connection_ipv6.yaml b/tests/component_tests/api/test_outgoing_connection_ipv6.yaml new file mode 100644 index 0000000000..4a1027a15c --- /dev/null +++ b/tests/component_tests/api/test_outgoing_connection_ipv6.yaml @@ -0,0 +1,20 @@ +esphome: + name: test + +esp32: + board: esp32dev + +wifi: + ssid: SomeNetwork + password: SomePassword + +network: + enable_ipv6: true + +logger: + +api: + encryption: + key: bOFFzzvfpg5DB94DuBGLXD/hMnhpDKgP9UQyBulwWVU= + outgoing_connection: + host: fd00::1