fix: treat occupied-but-valueless Z-Wave slots as unreadable, not empty - #1408
Merged
Conversation
When a lock reports userIdStatus occupied while withholding the code value, node-zwave-js's User Code CC adapter drops the credential from the unified read rather than returning it with no value. The slot then projected to empty(), telling the sync layer the write definitively did not land -- so it rewrote, failed to confirm again, and the circuit breaker suspended a slot whose PIN works on the keypad. Only the OPTIMISTIC write path could loop this way: a CONFIRMED write pops the pending entry and marks the slot verified, so the _last_set_pin escape hatch in calculate_in_sync tolerates an empty read-back. An OPTIMISTIC write stays pending and unverified, and calculate_in_sync returns False at the is_verified gate before ever reaching that hatch. Take userIdStatus as the occupancy authority and the credential as the value, so an occupied slot with no credential is unreadable rather than empty. Applied in the slot projection rather than async_get_users: the user list also drives the write paths (delete-owner resolution and async_set_user's adoption scan), and an occupancy inferred from the value database must not redirect which user a write targets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 23adf956e0cc
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1408 +/- ##
=======================================
Coverage 98.95% 98.95%
=======================================
Files 53 53
Lines 6598 6611 +13
Branches 470 470
=======================================
+ Hits 6529 6542 +13
Misses 69 69
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
Closes a parity gap between node-zwave-js's fresh and cached User Code CC reads, which LCM's slot projection turns into a wrong answer.
Three paths in
AccessControl.tsanswer "is this slot occupied?", and the cached one differs from the other two:getCredential(fresh, single slot,:960) rejects onlyAvailable/StatusNotAvailable, then returnsdata: result.userCode— including"".#getAllCredentials_UC(fresh, bulk branch,:1884) does the same, pushingdata: entry.userCoderegardless of whether a value came back.#getCredentialCached_UC(:2081) addsif (data == undefined) return undefined, collapsing "slot is empty" and "slot is occupied, value unknown" into one return value.LCM reads through the cached path (
get_all_credentials_cached), and_project_users_to_slotsstarts every managed slot atSlotCredential.empty()and only overlays credentials. So an occupied-but-valueless slot projects toempty()— LCM's strongest possible claim, "the lock definitively has no code here" — on the weakest possible evidence, an omission.That matters because
empty()is load-bearing for sync. A slot with an outstandingOPTIMISTICwrite that reads back empty stays pending and unverified, andcalculate_in_syncreturnsFalseat theis_verifiedgate before reaching the_last_set_pinescape hatch that would otherwise tolerate it. The result is a write/verify loop that ends at the circuit breaker. (ACONFIRMEDwrite cannot loop this way — it pops the pending entry and marks the slot verified.)This PR takes
userIdStatusas the occupancy authority and the credential as the value, so an occupied slot with no credential projects tounreadable()._pin_statealready mapped a withheld value tounreadablecorrectly — the credential simply never reached it.Deliberately applied in the slot projection (
async_get_usercodes) rather than inasync_get_users: the user list also drives the write paths (delete-owner resolution andasync_set_user's adoption scan). An earlier revision put the fallback inasync_get_usersand the synthesized owner was picked up by the legacy-adoption scan, silently turning a user CREATE into an UPDATE. Read-repair belongs to the read.Behavioral notes, each covered by a test:
AVAILABLE, so clears still confirm.emptyentries are upgraded; a readable credential always outranks the fallback, so a healthy lock is untouched.in_useis compared againstTrueexplicitly —Nonemeans the value database holds no status for the slot, which is absence of evidence, not evidence of occupancy.Two existing projection tests moved to slots 3/4, which the Schlage fixture reports
AVAILABLE; they previously asserted "empty" on slots the fixture reportsENABLED, which is the state this PR corrects.Type of change
Additional information
This is deliberately not marked as fixing #1397. It was found while investigating that issue and it removes a real failure mode, but whether it is the cause there is unconfirmed. Reaching the state it fixes requires
userIdStatusoccupied while the cacheduserCodeis absent, and tracing node-zwave-jsmasterdid not turn up a normal path that produces it:persistUserCodewrites status and code together or removes both,#persistCachedUserCodewrites both,setUseron a User Code CC lock throws rather than create a codeless user, andsecret: trueonuserCodeonly suppresses logging rather than excluding the value from the disk cache. If that state is unreachable, the reporter's slot 5 is readingAvailableinstead, in which case this change does not fire there. Confirming needs theuserIdStatusvalue from Z-Wave JS device diagnostics.🤖 Generated with Claude Code