fix: harden the MQTT providers against review findings from #1466 - #1471
Merged
Conversation
Two independent ways the node subscription could contradict what a poll of the same slot reports. The AVAILABLE branch took the lock at its word. Some locks re-announce AVAILABLE after a code lands, which marks the slot cleared, makes sync rewrite it, and starts a loop that never settles. The zwave_js provider already guards the identical hardware signal against LCM's desired credential; the same guard now applies here. The userCode branch confirmed a code from any publication, while the poll projection requires ENABLED. A Disabled slot keeps its digits, so the same slot read in sync over push and unreadable over poll -- decided by which retained topic the broker replayed first, which is not a property of the lock. The last-seen userIdStatus per slot is now tracked and a code is only confirmed when that status is ENABLED, or when no status has been seen at all: a gateway that publishes codes and never statuses would otherwise leave every slot permanently unconfirmed, and an over-confirmed code is corrected by the next poll while a muted one has no such correction. The tracking dies with the node subscription it was observed on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The predicate raised on any two-or-more slot read where nothing came back, which bricks two kinds of lock that were working before it existed. A bridge that cannot answer a read at all -- a Zigbee2MQTT converter exposing the PIN as write-only, a node with User Code Set but not Get -- previously polled all-unreadable forever while every write landed. Under the old rule every poll raised, the connectivity breaker tripped, and the writes were suspended along with the reads that were never going to work. The raise is now gated on this instance having read something successfully at least once, so a lock that can never prove it is readable is left exactly as it was. And a #1397-class lossy mesh drops the replies of one poll together, not independently, so a two-slot poll on a node timing out half its responses comes back entirely empty a good fraction of the time. One silent poll is now absorbed and only a second consecutive one raises, which costs one poll interval of detection latency and buys not suspending a lock that is merely slow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ents The shared scan result answers a question about a gateway, and every lock behind that gateway binds from it. Two paths threw it away on evidence that was never about the gateway at all. Releasing a transport did. Every borrowed provider releases on the way out -- a config flow read, the unmanaged sweep -- and so does a connection blip, so a sweep of N locks paid N full three-second discovery windows instead of one. Instance teardown now clears only the instance's own base. A timed-out call did. Every api this provider sends a lock is a sendCommand that waits on the mesh, so a FLiRS lock missing its wake window times one out routinely; that dropped the shared binding and sent every lock on the prefix rescanning into the same command queue that was already too slow to answer. The gateway is now asked directly first -- getInfo, on the gateway-local budget, out of its own cached driver state and off the mesh -- and a gateway still answering for this lock's network keeps its binding. An answer from another network, no answer, or no response subscription to ask over all fall back to invalidating, so a gateway that really did move is still rediscovered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The repair fired for any BaseException the per-lock setup gather returned, while its text diagnoses an unsupported bridge and asks the user to remove the entity. Two wrong audiences got it. CancelledError, which asyncio.gather(return_exceptions=True) hands back as a result rather than propagating: Home Assistant shutting down mid-setup dropped the lock and left a persistent repair, blaming a bridge that was never at fault and outliving the restart that ended the condition. Cancellation is now re-raised out of the results loop. And any bug in a provider LCM does speak, which told a user with a working lock to throw it out. Unclaimed locks now raise their own UnclaimedLockError from the resolver, and only that type creates the repair -- a type rather than a message match, so the diagnosis cannot drift onto a different failure that happens to read similarly. Everything else keeps the log-only pop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The unclaimed-mqtt check ran over the whole pre-filled lock list on every options and reauth submission, so an entry configured before that check existed -- carrying an mqtt lock whose bridge no provider speaks -- could never be saved again. An unrelated PIN edit was refused, and reauth, the recovery path that entry is most likely to be in, could not complete. There was no way out but hand-editing storage. Both flows now pass the entry's current lock list and only newly added locks are checked. Grandfathering is per lock, so any other unclaimed lock added to the same entry is still refused, and the initial user step -- which has no pre-existing entry -- keeps validating everything. A pre-existing unclaimed lock is not silently accepted either: it reaches the runtime, is dropped at setup, and raises its own repair naming the entity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_async_run_provider_setup clears _setup_deferred as its first statement, and that flag is the only thing arming another attempt for a lock whose provider integration was loaded all along -- no LOADED transition is ever coming for it. So a transient LockDisconnected during the retried setup consumed the one retry: the lock stayed un-set-up and sync, gated on that, refused to write anything to it until the integration was reloaded by hand. The reconnect path now re-arms the flag when the retry fails on connectivity, which is the exact condition the retry exists for. A validation failure still does not re-arm: that lock is known-degraded with a diagnosis and its own revalidation path, and re-probing it from every connection check would put a capability read on the wire every thirty seconds to learn what it already knows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The read path confirmed whatever an Enabled slot reported, so a lock configured to withhold its user codes -- which answers with an asterisk per digit -- had those asterisks confirmed as the PIN. They never equal the configured one, so sync saw a permanent mismatch and reprogrammed the slot on every tick, forever. Same Zigbee hardware as Zigbee2MQTT, whose read path already guards this; ZHA was the one site the earlier sweep missed. Deliberately deferred: the deeper fix is to apply the mask check once at the provider boundary rather than at each read site, so a new read path cannot miss it again. That refactor spans every provider and is out of scope here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s nothing Verified upstream: zwave-js-ui does permit '/' inside mqtt.prefix. The settings UI's validPrefix rule lists it among the allowed characters, its sanitizeTopic keeps it unless a caller asks for it to be stripped, the settings POST handler validates the field not at all, and every topic is built by plain concatenation -- so a prefix of home/zwave produces real multi-level topics. That makes the state topic structurally ambiguous: in home/zwave/nodeID_20/98/0/currentMode nothing distinguishes the prefix's second segment from a NAMED gateway's location segment. The scan fallback takes the first segment, searches under home/_CLIENTS, finds an empty broker, and reports it exactly as it reports a gateway that is down -- a diagnosis the operator cannot tell apart or guess. It cannot be made right, so it is made loud. The refusal now says the prefix was taken from the state topic's first segment, that a multi-level prefix is only recoverable from the gateway's discovery availability entry, and what to enable; a debug line records the topic the guess came from. The reasoning and the upstream citation sit at _split_state_topic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The idempotence guard reads _api_response_topic and the subscribe that sets it awaits, so two callers arriving together both passed the guard and both subscribed. Whichever finished second overwrote the other's unsub, and nothing else held a reference to the loser -- so the broker kept delivering into a handler nothing could ever release, for the rest of the run. Reachable in ordinary operation: the deferred-setup retry task and an in-flight poll both re-ensure. A dedicated lock, not the base's _aio_lock, which is held for the whole of every rate-limited provider operation -- and those resolve the api base, which ensures this subscription, so sharing it would deadlock on the first read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Four independent costs, none of which bought the payer anything. The settling window after subscribing to the api response topic was slept through at subscribe time, so setup and every borrowed instance -- a config flow allocation read, the unmanaged sweep -- paid a full second for a guarantee only a publish needs. It is now a recorded deadline that the first publish waits out, crediting whatever time resolution already spent. The api response handler parsed every message on the wildcard into a dict before discovering it belonged to another client. The gateway's own user interface polls getNodes on that topic, whose response can run to megabytes. The request id only ever appears in the echo of one of our own requests, so a substring scan of the undecoded payload settles it first; anything that gets through is still parsed and correlated exactly as before. A bridged provider derives push support from discovery data that arrives on the broker's schedule, so a lock whose setup was deferred was built before that answer existed and kept the poll cadence chosen for a lock with none -- polling a pushing lock forever, one api round trip per slot every five minutes. The deferred-setup retry now re-reads the capability once, on success, and the coordinator carries it directly rather than inferring it from a nullable interval. And the mqtt resolver tested both dispatch rules against each identifier in turn, over a SET whose iteration order is arbitrary. A device carrying both shapes resolved to whichever the hash yielded first, so the same device could dispatch differently across restarts. Two passes over the whole set is what makes the documented Zigbee2MQTT-first precedence hold over the device. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… tree The block still described a flat test_zigbee2mqtt.py and per-provider files named test_<provider>.py, neither of which has existed for a while, and had never gained zha/, zigbee2mqtt/, or zwave_js_ui/. It also listed none of the cross-provider modules that sit beside them. Documented as the convention it actually is -- conftest, test_provider, test_e2e per provider, with anything further split by subject -- so the add-a-provider checklist has something to point at. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
No fix's semantics change; every mutation that was killed before is still killed, each by a uniquely identified test. R2 loses its consecutive-silent-poll counter. It existed to absorb a #1397-class burst where a two-slot poll loses both replies at once, but the coordinator's connectivity breaker already takes BACKOFF_FAILURE_THRESHOLD (3) consecutive failures before it suspends anything, and nothing reacts to a single raise before that. The counter was a second absorber in front of one that already worked, and it made an absorbed poll look like a successful one, resetting the breaker it was supposed to be feeding. The _reads_have_succeeded latch -- the part that actually protects a bridge that can never answer a read -- stays. R1b's three helpers and the status-interpreting module function collapse into the one method they were split out of. Tracking bool "was the last status Enabled" instead of the status itself makes the unknown case a dict default rather than a branch, and a value naming no status simply records nothing. R3b's reappraise helper inlines into the one caller. The reviewer's simpler alternative -- let the scan path invalidate the shared entry when a re-scan finds the gateway gone -- cannot work: a re-scan only runs when the shared entry is already None, so nothing would ever null it and a renamed gateway would be unreachable for the process lifetime. The probe stays, at a third the size. The home-id comparison stays too: a gateway answering at the same client topic for a different network is exactly the case that would write PINs into a neighbouring network. R10a's settle helper inlines at its one call site. R9's lock and R10c's transition keep their shape -- there is no smaller thing that protects -- but lose the prose. Tests: dropped the retained-order permutation grid (all four rows duplicate an explicit test), the enabled-then-code test (with "unknown admits" as the default it asserts what the no-status test already does; the ENABLED branch's unique killer is the re-enable test), the disabled-code e2e (the gate never consults the coordinator, so a real one adds nothing), the text half of the payload-type grid, and both counter tests. AGENTS.md trimmed to the correction the review asked for. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
python
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1471 +/- ##
=======================================
Coverage 99.13% 99.14%
=======================================
Files 64 64
Lines 8222 8268 +46
Branches 520 520
=======================================
+ Hits 8151 8197 +46
Misses 71 71
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
Follow-up to #1466 (merged/released): a second full-PR review found defects that per-commit review could not see — several were interactions between individually-correct fixes. This PR addresses all of them, slimmed to the minimal mechanism per finding (417 production insertions):
Correctness
zwave_jsprovider applies to the identical hardware signal (documented infinite reprogram loop) now also guards the zwave-js-ui push path, and auserCodepublication no longer confirms a slot whose last-seenuserIdStatuswas non-ENABLED (retained-message order no longer makes state nondeterministic).lock_droppedrepair fires only for a genuinely unclaimed lock (newUnclaimedLockError);CancelledErrorpropagates out of setup instead of minting a persistent misdirecting repair at shutdown; other setup errors keep the log-only path./in the prefix — verified in zwave-js-ui source) now fail loud with a diagnosable message in the scan fallback, pointing at the discovery availability entry that handles them correctly.Efficiency
Type of change
Additional information
🤖 Generated with Claude Code