diff --git a/custom_components/lock_code_manager/providers/_zwave_js_uc.py b/custom_components/lock_code_manager/providers/_zwave_js_uc.py index ffaa01cd..a0ce5a82 100644 --- a/custom_components/lock_code_manager/providers/_zwave_js_uc.py +++ b/custom_components/lock_code_manager/providers/_zwave_js_uc.py @@ -82,6 +82,21 @@ SetValueStatus.WORKING, ) +# SetValueResult statuses that are structural impossibilities for this +# lock/endpoint -- the operation cannot succeed no matter how many times +# we retry. Surface these as a real rejection. Any *other* non-OK status +# (notably ``FAIL``) is treated as transient: 3.x routed sets through the +# HA service ``zwave_js.set_lock_usercode``, which never inspected the +# status at all, so those locks worked fine on flaky firmware that +# returned ``FAIL`` intermittently. Raising on transient ``FAIL`` is +# exactly what disabled TheMegamind's slots on 4.0.3-4.0.4 (#1251). +_UC_SET_VALUE_FATAL = ( + SetValueStatus.NO_DEVICE_SUPPORT, + SetValueStatus.ENDPOINT_NOT_FOUND, + SetValueStatus.NOT_IMPLEMENTED, + SetValueStatus.INVALID_VALUE, +) + @dataclass(repr=False, eq=False) class ZWaveJSUserCodeFallbackSupport(BaseLock): @@ -333,11 +348,22 @@ async def _async_uc_set_usercode(self, code_slot: int, usercode: str) -> bool: f"set usercode slot {code_slot} failed: {err}" ) from err if result is not None and result.status not in _UC_SET_VALUE_OK: - self._set_in_progress_code_slot = None - raise CodeRejectedError( - code_slot=code_slot, - lock_entity_id=self.lock.entity_id, - reason=f"set value returned {result.status.name}", + if result.status in _UC_SET_VALUE_FATAL: + self._set_in_progress_code_slot = None + raise CodeRejectedError( + code_slot=code_slot, + lock_entity_id=self.lock.entity_id, + reason=f"set value returned {result.status.name}", + ) + # Transient non-OK (canonically ``FAIL``): match the 3.x + # behavior of the HA service we used to call -- log and + # let the optimistic push + next sync tick converge. + _LOGGER.info( + "Lock %s slot %s: set returned %s; " + "trusting optimistic push and continuing", + self.lock.entity_id, + code_slot, + result.status.name, ) await self._async_uc_verify_write(code_slot, "set") # Optimistic update: the value cache updates asynchronously via push @@ -379,9 +405,18 @@ async def _async_uc_clear_usercode(self, code_slot: int) -> bool: f"clear usercode slot {code_slot} failed: {err}" ) from err if result is not None and result.status not in _UC_SET_VALUE_OK: - raise LockOperationFailed( - f"clear usercode slot {code_slot} failed: " - f"set value returned {result.status.name}" + if result.status in _UC_SET_VALUE_FATAL: + raise LockOperationFailed( + f"clear usercode slot {code_slot} failed: " + f"set value returned {result.status.name}" + ) + # Transient non-OK: see _async_uc_set_usercode for rationale. + _LOGGER.info( + "Lock %s slot %s: clear returned %s; " + "trusting optimistic push and continuing", + self.lock.entity_id, + code_slot, + result.status.name, ) await self._async_uc_verify_write(code_slot, "clear") # Optimistic update: see _async_uc_set_usercode for rationale. diff --git a/tests/providers/zwave_js/test_uc_fallback.py b/tests/providers/zwave_js/test_uc_fallback.py index 71c5cd2e..be732957 100644 --- a/tests/providers/zwave_js/test_uc_fallback.py +++ b/tests/providers/zwave_js/test_uc_fallback.py @@ -228,11 +228,24 @@ async def test_uc_set_credential_proceeds_when_masked( mock_uc_utils["set_usercode"].assert_awaited_once() -async def test_uc_set_credential_failure_status_raises_code_rejected( +async def test_uc_set_fail_status_logs_and_continues( uc_fallback_lock: ZWaveJSLock, mock_uc_utils: dict, + caplog: pytest.LogCaptureFixture, ) -> None: - """A FAIL set-value result maps to CodeRejectedError.""" + """A transient ``FAIL`` set-value status is tolerated, not a rejection. + + 3.3.0 routed sets through the HA service ``zwave_js.set_lock_usercode``, + which never inspected ``SetValueResult.status`` -- transient ``FAIL`` + responses (the canonical case on flaky-interview Schlage UC v1 locks, + #1251) were silently swallowed and the slot stayed enabled. The + structural statuses (``NO_DEVICE_SUPPORT`` etc.) still escalate to + ``CodeRejectedError`` because they are real impossibilities. + """ + mock_coordinator = MagicMock() + mock_coordinator.data = {} + uc_fallback_lock.coordinator = mock_coordinator + mock_uc_utils["set_usercode"].return_value = SetValueResult( {"status": SetValueStatus.FAIL} ) @@ -240,7 +253,44 @@ async def test_uc_set_credential_failure_status_raises_code_rejected( credential = Credential( type=CredentialType.PIN, slot=5, state=SlotCredential.known("4321") ) - with pytest.raises(CodeRejectedError, match="FAIL"): + with caplog.at_level(logging.INFO): + result = await uc_fallback_lock.async_set_credential( + user_id=5, credential=credential, pin="4321", name=None, source="sync" + ) + + assert result is True + mock_coordinator.push_update.assert_called_once_with( + {5: SlotCredential.known("4321")} + ) + assert "set returned FAIL" in caplog.text + + +@pytest.mark.parametrize( + "status", + [ + SetValueStatus.NO_DEVICE_SUPPORT, + SetValueStatus.ENDPOINT_NOT_FOUND, + SetValueStatus.NOT_IMPLEMENTED, + SetValueStatus.INVALID_VALUE, + ], + ids=lambda s: s.name, +) +async def test_uc_set_fatal_status_raises_code_rejected( + uc_fallback_lock: ZWaveJSLock, + mock_uc_utils: dict, + status: SetValueStatus, +) -> None: + """Structural-impossibility statuses still raise ``CodeRejectedError``. + + These mean the operation cannot succeed on this device/endpoint -- + not transient -- so the slot should be rejected loudly. + """ + mock_uc_utils["set_usercode"].return_value = SetValueResult({"status": status}) + + credential = Credential( + type=CredentialType.PIN, slot=5, state=SlotCredential.known("4321") + ) + with pytest.raises(CodeRejectedError, match=status.name): await uc_fallback_lock.async_set_credential( user_id=5, credential=credential, pin="4321", name=None, source="sync" ) @@ -982,17 +1032,49 @@ async def test_uc_clear_credential_transport_error_raises_lock_disconnected( await uc_fallback_lock.async_delete_credential(ref) -async def test_uc_clear_credential_failure_status_raises_operation_failed( +async def test_uc_clear_fail_status_logs_and_continues( uc_fallback_lock: ZWaveJSLock, mock_uc_utils: dict, + caplog: pytest.LogCaptureFixture, ) -> None: - """A FAIL set-value result during clear maps to LockOperationFailed.""" + """A transient ``FAIL`` clear status is tolerated; mirrors the set path.""" + mock_coordinator = MagicMock() + mock_coordinator.data = {} + uc_fallback_lock.coordinator = mock_coordinator + mock_uc_utils["clear_usercode"].return_value = SetValueResult( {"status": SetValueStatus.FAIL} ) ref = CredentialRef(user_id=3, type=CredentialType.PIN, slot=3) - with pytest.raises(LockOperationFailed, match="FAIL"): + with caplog.at_level(logging.INFO): + result = await uc_fallback_lock.async_delete_credential(ref) + + assert result is True + mock_coordinator.push_update.assert_called_once_with({3: SlotCredential.empty()}) + assert "clear returned FAIL" in caplog.text + + +@pytest.mark.parametrize( + "status", + [ + SetValueStatus.NO_DEVICE_SUPPORT, + SetValueStatus.ENDPOINT_NOT_FOUND, + SetValueStatus.NOT_IMPLEMENTED, + SetValueStatus.INVALID_VALUE, + ], + ids=lambda s: s.name, +) +async def test_uc_clear_fatal_status_raises_operation_failed( + uc_fallback_lock: ZWaveJSLock, + mock_uc_utils: dict, + status: SetValueStatus, +) -> None: + """Structural-impossibility clear statuses still raise ``LockOperationFailed``.""" + mock_uc_utils["clear_usercode"].return_value = SetValueResult({"status": status}) + + ref = CredentialRef(user_id=3, type=CredentialType.PIN, slot=3) + with pytest.raises(LockOperationFailed, match=status.name): await uc_fallback_lock.async_delete_credential(ref)