3 Commits

Include renames

Author	SHA1	Message	Date
agessaman	6e7f3cdb52	fix(network): bound link transitions and restore each medium's DNS ... Two defects found by an adversarial review of the merge. A link transition no longer stops a slot whose connect attempt is in flight. Stopping one means esp_mqtt_client_stop(), which waits on the SDK's API mutex and its task's stopped event with no bound; the client task notices only when it returns from whatever transport call it is in. During teardown the bridge's StopUnproven timeout contains that, but during a transition nothing does, so a routine WiFi flap could freeze the sole MQTT worker for far longer than the 2 s this path advertises, with the bridge's own stop handshake queued behind it. The attempt is left to resolve instead: unlike a reconfigure, a transition changes neither endpoint nor credentials, so an attempt that completes is credited to the broker it actually reached and drops with the old route. Failing back to Ethernet left the node using the WiFi network's DNS server. lwIP keeps one global server list and IDF 4.4 has no per-interface retention, so the medium that leased last owns DNS for every socket. Each link now remembers the resolver its own DHCP lease installed and puts it back when it is selected again; a medium that never held a lease leaves the current resolver alone. Where the two networks are on different subnets this was a silent outage: the link read as connected while every broker and NTP hostname failed to resolve, until Ethernet's own DHCP renewal happened to fix it.	2026-09-19 15:31:51 -07:00
agessaman	25f26c3058	Merge fix/mqtt-review-2026-09-09 into feat/abstract-networking ... #54 lands first, so the network abstraction is reworked on top of the per-slot client ownership model instead of alongside it. Two resolutions are semantic, not textual: - Link-transition teardown goes through the ownership API. It called softDisconnect() on every started client, which bypassed client_state and ignored the typed result. A slot that is still Starting now gets a real stop: softDisconnect() returns immediately on a client that is not yet connected, so its attempt would otherwise complete against the old route and deliver a CONNECTED event indistinguishable from the new one's (F04). On a medium switch the old route can still be briefly usable, so that is not hypothetical. A connected client keeps the cheap bounded path, and a quarantined one is left alone. - NetworkLink::applyPowerPrefs() adopts WifiPowerSavePolicy. Its local `2 ? MAX : NONE` mapping would have reintroduced F11 and read the new stored value 3 (explicit `min`) as none. It also applies the setting when the link starts already associated, which is the case the bridge used to cover. The NTP probe from #54 keeps its validation and its one-attempt-per-server bound, but resolves and gates on the selected link rather than on WiFi, so it works on an Ethernet-preferred node.	2026-09-19 15:15:07 -07:00
agessaman	b4daf8ca43	fix(mqtt): close the review's P1 and P2 findings on the F01/F04/F06 work ... Review of the branch found one merge-blocking lifecycle hole and three correctness gaps where the implementation stopped short of contracts the design had already written down. All four are real; each was confirmed against the source (two of them against hardware) before anything changed. **P1 — a quarantined client could still produce a clean bridge stop.** The cooperative teardown set `_teardown_complete` unconditionally, so a slot whose `esp_mqtt_client_stop()` had not completed — deliberately skipped by `destroySlotClients()` and marked Quarantined — still let the trampoline publish the acknowledgement. The owner then freed the queue and buffers and allowed a restart while that SDK task might still be running: exactly the ownership ambiguity StopUnproven exists to remove. The ack is now withheld unless EVERY client is proven stopped, so one unproven client leaves the whole bridge unproven. The rule lives in MQTTClientState.h (`mqttStopMayBeAcknowledged`) with host tests, alongside the state predicates moved out of the bridge. **P2 — the F04 protection did not cover a client that was still connecting.** `softDisconnect()` returns ESP_OK immediately when the client is not connected, so for a slot mid-DNS/TLS/CONNECT it cancelled nothing: the attempt ran on and its CONNECTED event arrived after the new configuration was applied, and with callbacks registered once per client and esp-mqtt events carrying no generation, nothing could tell it from the new attempt's. A reconfigure that lands on a `Starting` client now stops it, joining its SDK task, before applying the new configuration. A Connected client still takes the cheap softDisconnect path, which is where the fragmentation argument applies. One helper (`closeLiveClientForReconfigure`) so the two call sites cannot drift. **P2 — a failed renewal bounce still advanced the effective expiry.** Minting updates `token_expires_at` immediately and the renewal decision read it, so a bounce that failed looked complete: the next pass saw a fresh future expiry and never retried, and clearing `last_token_renewal` re-armed nothing. Slots now carry `applied_token_expires_at` — the expiry of the credential the CONNECTION is using — which only advances when a connect or reconnect has carried it. A failed bounce leaves it on the old credential, so the renewal stays due. **P2 — config-committed and start-accepted were conflated.** `connect()` returned one result for both, so a start that failed after the configuration had committed left `applied_config` describing the previous configuration, and the next recreate-or-reuse decision could reuse a client whose trust policy was not the one it believed. `applyConfig()` is now its own wrapper operation; `applied_config` records the commit, activation records the start. The reconnect ladder resets there too rather than in `teardownSlot()` — the old endpoint's history still applies until a replacement configuration actually commits. Two of my own bugs surfaced on hardware while testing this, both fixed here: - `recreateSlotClient()` called the full `teardownSlot()`, which cleared `broker_uri`, the just-minted token and both expiries out from under a configuration that had already been decided, so a recreate handed the SDK an empty URI and an empty token. It now stops the client and swaps the object, touching nothing else, and the apply step refuses to configure a URI that changed under it rather than passing it on. - `stopSlotClient()` quarantined on any non-OK result, but `ESP_FAIL` from `esp_mqtt_client_stop()` means "client is in invalid state", i.e. not started: there was no task to join, the safest state there is. It was observed quarantining healthy clients on hardware. The case that genuinely cannot be proven is a stop that never RETURNS, which cannot surface here at all — it hangs the task, which is what the bridge-level timeout contains. Hardware (Heltec V4, 5 live slots): a reconfigure landing on a connecting client logs `reconfigure during connect - stopping to cancel the attempt`; a broker holding the CONNACK sees the client close the socket and the disabled slot never connects; `wss`→`mqtt`→`wss` recreate cycles reconnect each way; a blackholed endpoint recovers. 499/499 native tests, four envs clean.	2026-09-09 20:03:13 -07:00