From ac508968fbda5717db80a497ede8b82b0f2a5c95 Mon Sep 17 00:00:00 2001 From: Ryan Morash Date: Mon, 29 Jun 2026 17:06:13 -0400 Subject: [PATCH 1/3] fix(unifi): best-effort email + per-user resilient apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live sync crashed with CODE_ADMIN_EMAIL_EXIST: UniFi requires globally unique emails across users AND admins, and several staff are both UniFi admins and CiviCRM members. Writing a member's email that already belongs to an admin was rejected, and nothing caught it — so the whole reconcile cycle crashed, every cycle (the collision is persistent), leaving sync stuck in a crash/alert loop. - Email best-effort: UnifiClientError now carries the envelope `code`. _request_user_write (used by create, reactivate, and credential-update) retries the write without `user_email` on an *_EMAIL_EXIST code and warns; the rest of the record still applies. Not treated as a failure. - Per-user resilience: each contact's apply step is wrapped; a UnifiClientError is logged + recorded and the cycle continues with the rest, then apply() raises one summary so the orchestrator still alerts. (Batch card pre-import stays fail-fast.) Extracted _apply_one_credential. Docs: architecture.md §7 documents both behaviors. 347 passed; pyrefly 0 errors; ruff + sphinx -W clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/architecture.md | 4 + src/door_sync/unifi/client.py | 248 ++++++++++++++++++++++++---------- tests/test_unifi_client.py | 210 ++++++++++++++++++++++++++++ 3 files changed, 394 insertions(+), 68 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 6c111c4..6450ab3 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -248,6 +248,10 @@ config.load() 4. **Compute diff** — see §8. 5. **Safety check** — see §9. 6. **Apply diff** — `unifi.apply(diff)` iterates the diff sets serially with a 50-100ms inter-call delay. Each action retried per design guide §8 (exponential backoff, honor `Retry-After` on 429). Failures partway through are tolerable: idempotency means the next cycle resumes correctly. + + **Per-user isolation:** a single contact's `UnifiClientError` is recorded and the cycle continues with the remaining contacts; `apply()` then raises one summary error so the orchestrator alerts. One bad record never blocks the rest. (The batch card pre-import is a shared prerequisite and still fails fast.) + + **Email is best-effort:** UniFi requires globally-unique emails across users *and* admins. When a member's CiviCRM email is already registered to another account — commonly a staff member who is also a UniFi admin — the write is retried without the `user_email` field (so name/card/policy still apply) and a warning is logged. The member keeps door access; only the conflicting email isn't synced. This is *not* counted as a per-user failure, so it doesn't alert. 7. **Log + persist state** — write audit entries; write last-success timestamp atomically (write to temp, fsync, rename). --- diff --git a/src/door_sync/unifi/client.py b/src/door_sync/unifi/client.py index 094c619..f92bef7 100644 --- a/src/door_sync/unifi/client.py +++ b/src/door_sync/unifi/client.py @@ -41,7 +41,16 @@ class UnifiClientError(Exception): - """Raised on non-recoverable UniFi Access API failure.""" + """Raised on non-recoverable UniFi Access API failure. + + Carries the UniFi envelope error `code` (e.g. "CODE_ADMIN_EMAIL_EXIST") + when one is available, so callers can branch on specific conditions without + parsing the message string. + """ + + def __init__(self, message: str, *, code: str | None = None) -> None: + super().__init__(message) + self.code = code class UnifiClient: @@ -105,6 +114,9 @@ def __init__( self._unifi_user_id_by_contact: dict[int, str] = {} self._nfc_cards_by_contact: dict[int, list[dict[str, Any]]] = {} self._nfc_token_map: dict[int, str] | None = None + # Per-cycle record of contacts whose apply step failed (reset each + # apply()); a non-empty list at the end of apply() raises a summary. + self._apply_failures: list[str] = [] # Reverse of the token map: a card's `token` (the only stable card # identifier the /users endpoint exposes) -> its card number. Populated # alongside `_nfc_token_map` so reads can resolve a user's card. @@ -192,7 +204,7 @@ def _unwrap(self, response: httpx.Response) -> Any: code = payload.get("code") if code != "SUCCESS": msg = payload.get("msg", "") - raise UnifiClientError(f"{code}: {msg}") + raise UnifiClientError(f"{code}: {msg}", code=code if isinstance(code, str) else None) return payload.get("data") def _with_retries(self, action: Callable[[], httpx.Response]) -> httpx.Response: @@ -395,14 +407,72 @@ def apply(self, diff: Diff) -> None: self._populate_token_map_for_dry_run(diff) self._log_dry_run_actions(diff) return + # Per-user failures are isolated: a single contact's UnifiClientError is + # recorded and the cycle continues with the rest, then a summary is + # raised so the orchestrator alerts. _preimport is a shared prerequisite + # (batch card import) and is intentionally left to fail fast. + self._apply_failures = [] self._preimport_unknown_cards(diff) self._apply_deactivate(diff) self._apply_update_credential(diff) self._apply_update_policy(diff) self._apply_add(diff) + if self._apply_failures: + raise UnifiClientError( + f"{len(self._apply_failures)} user update(s) failed this cycle: " + + "; ".join(self._apply_failures) + ) _INTER_CALL_DELAY_SECONDS = 0.075 + def _record_apply_failure(self, contact_id: int, exc: UnifiClientError) -> None: + """Log and record a per-user apply failure so the cycle can continue. + + Args: + contact_id: The contact whose update failed. + exc: The API error raised for that contact. + """ + logger.error("apply failed for contact=%d: %s", contact_id, exc) + self._apply_failures.append(f"contact={contact_id}: {exc}") + + def _request_user_write( + self, method: str, path: str, body: dict[str, Any], *, contact_id: int + ) -> Any: + """Create/update a user, treating `user_email` as best-effort. + + UniFi requires globally-unique emails (across users and admins). When a + member's email is already registered to another account — e.g. they are + also a UniFi admin — the write is rejected with an EMAIL_EXIST code. The + email simply cannot be synced for that member, but the rest of the + record (name, employee number) should still apply, so on that specific + error the `user_email` field is dropped and the write retried. If + nothing remains to write, the call is skipped. + + Args: + method: HTTP method ("POST" or "PUT"). + path: User endpoint path. + body: Request body, possibly containing "user_email". + contact_id: Contact being written, for the warning log. + + Returns: + The API `data` payload from the successful (possibly retried) write, + or None if the retry had nothing left to send. + """ + try: + return self._request(method, path, json=body) + except UnifiClientError as exc: + if "user_email" not in body or not _is_email_conflict(exc.code): + raise + logger.warning( + "contact %d: email already registered to another UniFi account; " + "syncing the record without email", + contact_id, + ) + retry_body = {k: v for k, v in body.items() if k != "user_email"} + if not retry_body: + return None + return self._request(method, path, json=retry_body) + def _apply_update_credential(self, diff: Diff) -> None: """Apply credential changes (display name and/or NFC card) from a diff. @@ -421,48 +491,65 @@ def _apply_update_credential(self, diff: Diff) -> None: ) continue - user_fields: dict[str, Any] = {} - if resolved.display_name != unifi_user.display_name: - first, last = _split_name(resolved.display_name) - user_fields["first_name"] = first - user_fields["last_name"] = last - if _email_differs_ci(resolved.email, unifi_user.email): - # Empty string clears the email in UniFi; a value sets it. - user_fields["user_email"] = resolved.email or "" - if user_fields: + try: + self._apply_one_credential(resolved, unifi_user, user_id) + except UnifiClientError as exc: + self._record_apply_failure(resolved.contact_id, exc) + continue + + def _apply_one_credential( + self, resolved: ResolvedMember, unifi_user: UnifiUser, user_id: str + ) -> None: + """Apply one contact's name/email/card credential change. + + Args: + resolved: Desired member state from CiviCRM. + unifi_user: Current UniFi state for the same contact. + user_id: UniFi user identifier. + """ + user_fields: dict[str, Any] = {} + if resolved.display_name != unifi_user.display_name: + first, last = _split_name(resolved.display_name) + user_fields["first_name"] = first + user_fields["last_name"] = last + if _email_differs_ci(resolved.email, unifi_user.email): + # Empty string clears the email in UniFi; a value sets it. + user_fields["user_email"] = resolved.email or "" + if user_fields: + self._request_user_write( + "PUT", + f"/api/v1/developer/users/{user_id}", + user_fields, + contact_id=resolved.contact_id, + ) + time.sleep(self._INTER_CALL_DELAY_SECONDS) + + if resolved.card_id != unifi_user.card_id: + # Delete old card(s) on the user. + for old_card in self._nfc_cards_by_contact.get(resolved.contact_id, []): + old_token = str(old_card.get("token", "")) + if not old_token: + continue self._request( - "PUT", - f"/api/v1/developer/users/{user_id}", - json=user_fields, + "DELETE", + f"/api/v1/developer/users/{user_id}/nfc_cards/delete", + json={"token": old_token}, ) time.sleep(self._INTER_CALL_DELAY_SECONDS) - - if resolved.card_id != unifi_user.card_id: - # Delete old card(s) on the user. - for old_card in self._nfc_cards_by_contact.get(resolved.contact_id, []): - old_token = str(old_card.get("token", "")) - if not old_token: - continue - self._request( - "DELETE", - f"/api/v1/developer/users/{user_id}/nfc_cards/delete", - json={"token": old_token}, - ) - time.sleep(self._INTER_CALL_DELAY_SECONDS) - # Bind new card if specified. - if resolved.card_id is not None: - new_token = self._ensure_nfc_token_map().get(resolved.card_id) - if new_token is None: - raise UnifiClientError( - f"no token for card_id={_redact(resolved.card_id)} " - f"after import (contact={resolved.contact_id})" - ) - self._request( - "PUT", - f"/api/v1/developer/users/{user_id}/nfc_cards", - json={"token": new_token, "force_add": False}, + # Bind new card if specified. + if resolved.card_id is not None: + new_token = self._ensure_nfc_token_map().get(resolved.card_id) + if new_token is None: + raise UnifiClientError( + f"no token for card_id={_redact(resolved.card_id)} " + f"after import (contact={resolved.contact_id})" ) - time.sleep(self._INTER_CALL_DELAY_SECONDS) + self._request( + "PUT", + f"/api/v1/developer/users/{user_id}/nfc_cards", + json={"token": new_token, "force_add": False}, + ) + time.sleep(self._INTER_CALL_DELAY_SECONDS) def _apply_update_policy(self, diff: Diff) -> None: for resolved, _unifi_user in diff.to_update_policy: @@ -483,11 +570,15 @@ def _apply_update_policy(self, diff: Diff) -> None: # users is intentionally omitted: this endpoint sets per-user # assignments, so including the global ID would convert it into a # manual per-user mapping. The global policy auto-applies on its own. - self._request( - "PUT", - f"/api/v1/developer/users/{user_id}/access_policies", - json={"access_policy_ids": [resolved.target_policy]}, - ) + try: + self._request( + "PUT", + f"/api/v1/developer/users/{user_id}/access_policies", + json={"access_policy_ids": [resolved.target_policy]}, + ) + except UnifiClientError as exc: + self._record_apply_failure(resolved.contact_id, exc) + continue time.sleep(self._INTER_CALL_DELAY_SECONDS) def _apply_deactivate(self, diff: Diff) -> None: @@ -499,11 +590,15 @@ def _apply_deactivate(self, diff: Diff) -> None: unifi_user.contact_id, ) continue - self._request( - "PUT", - f"/api/v1/developer/users/{user_id}", - json={"status": "DEACTIVATED"}, - ) + try: + self._request( + "PUT", + f"/api/v1/developer/users/{user_id}", + json={"status": "DEACTIVATED"}, + ) + except UnifiClientError as exc: + self._record_apply_failure(unifi_user.contact_id, exc) + continue time.sleep(self._INTER_CALL_DELAY_SECONDS) def _log_dry_run_actions(self, diff: Diff) -> None: @@ -687,21 +782,25 @@ def _apply_add(self, diff: Diff) -> None: diff: The diff containing members to add. """ for resolved in diff.to_add: - existing_user_id = self._unifi_user_id_by_contact.get(resolved.contact_id) - first, last = _split_name(resolved.display_name) - if existing_user_id is not None: - # Reactivate path: prepare credentials/policy first, then activate. - self._prepare_reactivation(resolved, existing_user_id, first, last) - self._bind_card_if_set(existing_user_id, resolved) - self._assign_policy_if_set(existing_user_id, resolved) - self._activate_user(existing_user_id) - else: - # True create - user_id = self._create_user(resolved, first, last) - self._unifi_user_id_by_contact[resolved.contact_id] = user_id - # Common tail for newly created users. - self._bind_card_if_set(user_id, resolved) - self._assign_policy_if_set(user_id, resolved) + try: + existing_user_id = self._unifi_user_id_by_contact.get(resolved.contact_id) + first, last = _split_name(resolved.display_name) + if existing_user_id is not None: + # Reactivate path: prepare credentials/policy first, then activate. + self._prepare_reactivation(resolved, existing_user_id, first, last) + self._bind_card_if_set(existing_user_id, resolved) + self._assign_policy_if_set(existing_user_id, resolved) + self._activate_user(existing_user_id) + else: + # True create + user_id = self._create_user(resolved, first, last) + self._unifi_user_id_by_contact[resolved.contact_id] = user_id + # Common tail for newly created users. + self._bind_card_if_set(user_id, resolved) + self._assign_policy_if_set(user_id, resolved) + except UnifiClientError as exc: + self._record_apply_failure(resolved.contact_id, exc) + continue def _prepare_reactivation( self, @@ -732,10 +831,11 @@ def _prepare_reactivation( "employee_number": str(resolved.contact_id), "user_email": resolved.email or "", } - self._request( + self._request_user_write( "PUT", f"/api/v1/developer/users/{user_id}", - json=body, + body, + contact_id=resolved.contact_id, ) time.sleep(self._INTER_CALL_DELAY_SECONDS) # Delete any old cards that differ from the new card_id. @@ -790,7 +890,9 @@ def _create_user(self, resolved: ResolvedMember, first: str, last: str) -> str: } if resolved.email is not None: body["user_email"] = resolved.email - data = self._request("POST", "/api/v1/developer/users", json=body) + data = self._request_user_write( + "POST", "/api/v1/developer/users", body, contact_id=resolved.contact_id + ) time.sleep(self._INTER_CALL_DELAY_SECONDS) if not isinstance(data, dict) or "id" not in data: raise UnifiClientError(f"POST /users returned no id for contact={resolved.contact_id}") @@ -884,6 +986,16 @@ def _parse_nfc_id(nfc_id: str, expected_facility_code: int) -> int | None: return cn +def _is_email_conflict(code: str | None) -> bool: + """True if a UniFi error code signals an already-registered email. + + UniFi Access requires globally-unique emails across users and admins; a + collision surfaces as CODE_ADMIN_EMAIL_EXIST (or a USER variant). Matching + the ``EMAIL_EXIST`` suffix covers both without hard-coding each one. + """ + return code is not None and code.endswith("EMAIL_EXIST") + + def _parse_sync_alias(alias: str) -> int | None: """Recover the card number door-sync encoded in an import alias. diff --git a/tests/test_unifi_client.py b/tests/test_unifi_client.py index 328774d..167ac33 100644 --- a/tests/test_unifi_client.py +++ b/tests/test_unifi_client.py @@ -20,6 +20,7 @@ UnifiClient, UnifiClientError, _compute_nfc_id, + _is_email_conflict, _parse_nfc_id, _parse_sync_alias, _redact, @@ -92,6 +93,19 @@ def test_parse_sync_alias_rejects_non_digit_suffix() -> None: assert _parse_sync_alias("sync-১") is None # U+09E7 Bengali digit one +# --- email-conflict detection --- + + +def test_is_email_conflict_matches_email_exist_codes() -> None: + """Matches the admin/user EMAIL_EXIST variants; nothing else.""" + assert _is_email_conflict("CODE_ADMIN_EMAIL_EXIST") is True + assert _is_email_conflict("CODE_USER_EMAIL_EXIST") is True + assert _is_email_conflict("CODE_AUTH_FAILED") is False + assert _is_email_conflict("CODE_RESOURCE_NOT_FOUND") is False + assert _is_email_conflict(None) is False + assert _is_email_conflict("") is False + + # --- Name splitting --- @@ -1226,6 +1240,139 @@ def test_apply_update_credential_name_only( assert user_nfc_calls == [] +def test_apply_update_credential_email_conflict_retries_without_email( + httpx_mock: HTTPXMock, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, + make_client: Callable[..., UnifiClient], +) -> None: + """A member email already registered to another UniFi account (admin) must + not crash the cycle: retry the write without user_email so name still + applies, and warn. (CODE_ADMIN_EMAIL_EXIST).""" + monkeypatch.setattr("door_sync.unifi.client.time.sleep", lambda _: None) + client = make_client(managed_policy_ids={"pol-1"}) + + httpx_mock.add_response( + method="GET", + url="https://192.0.2.1:12445/api/v1/developer/credentials/nfc_cards/tokens?page_num=1&page_size=100", + json=_card_tokens_page([(1234, "tok-1234")]), + ) + httpx_mock.add_response( + method="GET", + url="https://192.0.2.1:12445/api/v1/developer/users?page_num=1&page_size=100&expand[]=access_policy", + json=_users_page( + [ + _user_row( + contact_id=42, + user_id="uuid-42", + first_name="Old", + last_name="Name", + nfc_token="tok-1234", + user_email="old@example.com", + ) + ] + ), + ) + fetched = client.fetch_users() + + # First profile PUT → email-conflict; retry (no email) → success. + httpx_mock.add_response( + method="PUT", + url="https://192.0.2.1:12445/api/v1/developer/users/uuid-42", + json={ + "code": "CODE_ADMIN_EMAIL_EXIST", + "msg": "Email address is already registered. Choose a different one.", + "data": None, + }, + ) + httpx_mock.add_response( + method="PUT", + url="https://192.0.2.1:12445/api/v1/developer/users/uuid-42", + json={"code": "SUCCESS", "msg": "success", "data": None}, + ) + + resolved = ResolvedMember( + contact_id=42, + display_name="New Name", + card_id=1234, # unchanged → no card calls + target_policy="pol-1", + resolution="tier", + email="staff@example.com", # collides with an admin account + ) + with caplog.at_level(logging.WARNING, logger="door_sync.unifi.client"): + client.apply(_diff(to_update_credential=((resolved, fetched[0]),))) + + profile_puts = [ + r + for r in httpx_mock.get_requests() + if r.method == "PUT" and r.url.path == "/api/v1/developer/users/uuid-42" + ] + assert len(profile_puts) == 2 + assert _json.loads(profile_puts[0].content)["user_email"] == "staff@example.com" + retry_body = _json.loads(profile_puts[1].content) + assert "user_email" not in retry_body + assert retry_body == {"first_name": "New", "last_name": "Name"} + assert any("email already registered" in r.message for r in caplog.records) + + +def test_apply_isolates_per_user_failure_and_summarizes( + httpx_mock: HTTPXMock, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, + make_client: Callable[..., UnifiClient], +) -> None: + """One user's failure must not stop the rest: contact 1's policy PUT fails + (non-email), contact 2's still runs, and apply() raises a summary so the + cycle is alerted.""" + monkeypatch.setattr("door_sync.unifi.client.time.sleep", lambda _: None) + client = make_client() + + httpx_mock.add_response( + method="GET", + url="https://192.0.2.1:12445/api/v1/developer/users?page_num=1&page_size=100&expand[]=access_policy", + json=_users_page( + [ + _user_row(contact_id=1, user_id="u1"), + _user_row(contact_id=2, user_id="u2"), + ] + ), + ) + fetched = client.fetch_users() + + httpx_mock.add_response( + method="PUT", + url="https://192.0.2.1:12445/api/v1/developer/users/u1/access_policies", + json={"code": "CODE_RESOURCE_NOT_FOUND", "msg": "policy not found", "data": None}, + ) + httpx_mock.add_response( + method="PUT", + url="https://192.0.2.1:12445/api/v1/developer/users/u2/access_policies", + json={"code": "SUCCESS", "msg": "success", "data": None}, + ) + + r1 = _resolved(contact_id=1, target_policy="pol-x") + r2 = _resolved(contact_id=2, target_policy="pol-y") + diff = _diff(to_update_policy=((r1, fetched[0]), (r2, fetched[1]))) + + with caplog.at_level(logging.ERROR, logger="door_sync.unifi.client"): + with pytest.raises(UnifiClientError) as exc_info: + client.apply(diff) + + # Contact 2 was still attempted despite contact 1 failing. + policy_put_paths = { + r.url.path + for r in httpx_mock.get_requests() + if r.method == "PUT" and r.url.path.endswith("/access_policies") + } + assert policy_put_paths == { + "/api/v1/developer/users/u1/access_policies", + "/api/v1/developer/users/u2/access_policies", + } + # The summary names the failed contact, and it was logged. + assert "contact=1" in str(exc_info.value) + assert any("apply failed for contact=1" in r.message for r in caplog.records) + + def test_apply_update_policy_replaces( httpx_mock: HTTPXMock, monkeypatch: pytest.MonkeyPatch, make_client: Callable[..., UnifiClient] ) -> None: @@ -1357,6 +1504,69 @@ def test_apply_create_new_user_path( assert body == {"first_name": "Jane", "last_name": "Doe", "employee_number": "42"} +def test_apply_create_email_conflict_retries_without_email( + httpx_mock: HTTPXMock, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, + make_client: Callable[..., UnifiClient], +) -> None: + """Creating a new member who is also a UniFi admin: the POST with + user_email is rejected (EMAIL_EXIST), so it retries without email and the + user is still created (no card/policy here to keep it focused).""" + monkeypatch.setattr("door_sync.unifi.client.time.sleep", lambda _: None) + client = make_client() + + httpx_mock.add_response( + method="GET", + url="https://192.0.2.1:12445/api/v1/developer/users?page_num=1&page_size=100&expand[]=access_policy", + json=_users_page([], total=0), + ) + client.fetch_users() + + # _preimport_unknown_cards loads the token map even with no card to import. + httpx_mock.add_response( + method="GET", + url="https://192.0.2.1:12445/api/v1/developer/credentials/nfc_cards/tokens?page_num=1&page_size=100", + json=_cards_page([]), + ) + # First create POST → email conflict; retry without email → success. + httpx_mock.add_response( + method="POST", + url="https://192.0.2.1:12445/api/v1/developer/users", + json={"code": "CODE_ADMIN_EMAIL_EXIST", "msg": "already registered", "data": None}, + ) + httpx_mock.add_response( + method="POST", + url="https://192.0.2.1:12445/api/v1/developer/users", + json={ + "code": "SUCCESS", + "msg": "success", + "data": {"id": "uuid-new", "first_name": "Jane", "last_name": "Doe"}, + }, + ) + + resolved = ResolvedMember( + contact_id=42, + display_name="Jane Doe", + card_id=None, + target_policy=None, + resolution="tier", + email="staff@example.com", + ) + with caplog.at_level(logging.WARNING, logger="door_sync.unifi.client"): + client.apply(_diff(to_add=(resolved,))) + + posts = [ + r + for r in httpx_mock.get_requests() + if r.method == "POST" and r.url.path == "/api/v1/developer/users" + ] + assert len(posts) == 2 + assert "user_email" in _json.loads(posts[0].content) + assert "user_email" not in _json.loads(posts[1].content) + assert any("email already registered" in r.message for r in caplog.records) + + def test_apply_reactivate_inactive_user_path( httpx_mock: HTTPXMock, monkeypatch: pytest.MonkeyPatch, make_client: Callable[..., UnifiClient] ) -> None: From e2637033dc2ede77588ea189ed54c650167bc568 Mon Sep 17 00:00:00 2001 From: Ryan Morash Date: Mon, 29 Jun 2026 17:12:15 -0400 Subject: [PATCH 2/3] docs: reflect best-effort email + per-user resilient apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Beyond the architecture.md §7 note already in this PR, update the other docs that described the old apply/error behavior: - conventions.rst "Error Handling": note the two apply() refinements (per-user isolation + best-effort email) under the Clients layer. - reconciliation.rst: clarify the "exceptions propagate" property — apply() isolates a single contact's failure and raises a summary; email is best-effort. - 2026-06-17-sync-member-email-design.md: dated correction that email writes are best-effort (UniFi's global email-uniqueness constraint, the CODE_ADMIN_EMAIL_EXIST case, and the retry-without-email behavior). Docs build clean under sphinx-build -W. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/architecture/conventions.rst | 11 ++++++++++- docs/architecture/reconciliation.rst | 6 +++++- .../specs/2026-06-17-sync-member-email-design.md | 13 ++++++++++++- 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/docs/architecture/conventions.rst b/docs/architecture/conventions.rst index 2e4bae8..d71b410 100644 --- a/docs/architecture/conventions.rst +++ b/docs/architecture/conventions.rst @@ -78,7 +78,16 @@ exceptions on data issues. They return sentinel values — for example, orchestrator decide how to handle them. **Clients** (``civicrm.client``, ``unifi.client``) raise after exhausting -retries. Client exceptions propagate through the orchestrator to the scheduler. +retries. Client exceptions propagate through the orchestrator to the scheduler, +with two refinements inside ``unifi.apply()``: + +- **Per-user isolation.** A single contact's ``UnifiClientError`` is logged, + recorded, and skipped so the remaining contacts still apply; ``apply()`` then + raises one summary error at the end so the failure is still surfaced. +- **Best-effort email.** UniFi requires globally-unique emails across users and + admins, so an email already registered to another account is dropped from the + write (the rest of the record still applies) and warned — not treated as a + failure. **The scheduler** catches per-cycle exceptions, logs them, writes a crash audit record, and continues to the next cycle. diff --git a/docs/architecture/reconciliation.rst b/docs/architecture/reconciliation.rst index 792669e..631beff 100644 --- a/docs/architecture/reconciliation.rst +++ b/docs/architecture/reconciliation.rst @@ -178,6 +178,10 @@ Key design properties: - **Clients are per-cycle.** They're cheap to construct and this gives clean isolation between cycles, avoiding stale HTTP sessions. - **Exceptions propagate.** The orchestrator does not catch — the scheduler's - per-cycle ``try/except`` handles crashes. + per-cycle ``try/except`` handles crashes. Within ``apply()`` itself, a single + contact's failure is isolated (logged and skipped) so the rest of the cycle + still applies; a summary error is then raised so the failure still surfaces. + Email writes are best-effort: an address already registered to another UniFi + account is dropped and warned rather than failing the contact. - **One function, many callers.** The same ``reconcile()`` is called by the daemon loop, the ``--once`` CLI mode, and (in the future) the webhook handler. diff --git a/docs/superpowers/specs/2026-06-17-sync-member-email-design.md b/docs/superpowers/specs/2026-06-17-sync-member-email-design.md index 3262369..86fb76b 100644 --- a/docs/superpowers/specs/2026-06-17-sync-member-email-design.md +++ b/docs/superpowers/specs/2026-06-17-sync-member-email-design.md @@ -7,7 +7,18 @@ provisioned and kept in sync on the UniFi Access user record. --- -## 1. Motivation +> **Correction (2026-06-29) — email writes are best-effort.** +> +> This spec describes writing `user_email` on create/reactivate/update (§3.5) +> but did not account for UniFi's constraint that **emails are globally unique +> across users *and* admins**. In production this surfaced as a +> `CODE_ADMIN_EMAIL_EXIST` crash for members who are also UniFi admins. The +> implemented behavior: a write rejected with an `*_EMAIL_EXIST` code is retried +> **without** the `user_email` field (so name/card/policy still apply) and a +> warning is logged; the email simply isn't synced for that member, and it is +> not treated as a cycle failure. Per-user failures during `apply()` are also +> isolated now (one bad contact no longer halts the rest). See +> `docs/architecture.md` §7 and `architecture/conventions.rst` "Error Handling". UniFi Access uses a user's email **functionally**: it delivers mobile credential invites and PIN codes to that address. Today the reconciler syncs From b1340e61c55c0e8e5d25d6673ba5b50dadaf7674 Mon Sep 17 00:00:00 2001 From: Ryan Morash Date: Mon, 29 Jun 2026 17:23:48 -0400 Subject: [PATCH 3/3] fix(unifi): harden resilient-apply per review (delay, exc_info, capped summary) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #32 review: - _request_user_write: sleep the inter-call delay before the email-conflict retry so a burst of EMAIL_EXIST conflicts doesn't fire back-to-back requests (Copilot). - _record_apply_failure: log with exc_info=exc to preserve the traceback, per the project's crash-logging convention (CodeRabbit). - Cap the apply-failure summary at 10 detailed entries + "...and N more" via _format_apply_failure_summary, so a widespread failure can't produce an unbounded error/alert message (Copilot). Add unit tests. (Declined the suggestion to wrap test clients in `with` — the make_client fixture already closes every client at teardown, matching all 67 existing usages.) 349 passed; pyrefly 0 errors; ruff clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/door_sync/unifi/client.py | 36 ++++++++++++++++++++++++++++++----- tests/test_unifi_client.py | 20 +++++++++++++++++++ 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/src/door_sync/unifi/client.py b/src/door_sync/unifi/client.py index f92bef7..2edcd6f 100644 --- a/src/door_sync/unifi/client.py +++ b/src/door_sync/unifi/client.py @@ -30,6 +30,9 @@ _MAX_ATTEMPTS = 3 _MAX_PAGES = 1_000 _PAGE_SIZE = 100 +# Cap on per-contact detail lines in an apply()-failure summary, so a +# widespread failure doesn't produce an unbounded error/alert message. +_MAX_FAILURE_DETAIL = 10 # Alias door-sync stamps on every card it imports, encoding the card number. # Neither read endpoint returns the Wiegand `nfc_id`: /users gives each card's # `token` (plus a display `id`), and the card list (/credentials/nfc_cards/ @@ -418,10 +421,7 @@ def apply(self, diff: Diff) -> None: self._apply_update_policy(diff) self._apply_add(diff) if self._apply_failures: - raise UnifiClientError( - f"{len(self._apply_failures)} user update(s) failed this cycle: " - + "; ".join(self._apply_failures) - ) + raise UnifiClientError(_format_apply_failure_summary(self._apply_failures)) _INTER_CALL_DELAY_SECONDS = 0.075 @@ -432,7 +432,7 @@ def _record_apply_failure(self, contact_id: int, exc: UnifiClientError) -> None: contact_id: The contact whose update failed. exc: The API error raised for that contact. """ - logger.error("apply failed for contact=%d: %s", contact_id, exc) + logger.error("apply failed for contact=%d: %s", contact_id, exc, exc_info=exc) self._apply_failures.append(f"contact={contact_id}: {exc}") def _request_user_write( @@ -471,6 +471,9 @@ def _request_user_write( retry_body = {k: v for k, v in body.items() if k != "user_email"} if not retry_body: return None + # Keep the retry on the same inter-call pacing as other writes so a + # burst of EMAIL_EXIST conflicts doesn't fire back-to-back requests. + time.sleep(self._INTER_CALL_DELAY_SECONDS) return self._request(method, path, json=retry_body) def _apply_update_credential(self, diff: Diff) -> None: @@ -986,6 +989,29 @@ def _parse_nfc_id(nfc_id: str, expected_facility_code: int) -> int | None: return cn +def _format_apply_failure_summary( + failures: list[str], *, max_detail: int = _MAX_FAILURE_DETAIL +) -> str: + """Build the summary message for per-contact apply failures. + + Caps the detail at `max_detail` entries and appends "...and N more" so the + message (and any alert derived from it) stays bounded under a widespread + failure. + + Args: + failures: Per-contact failure detail strings. + max_detail: Maximum number of detail entries to include verbatim. + + Returns: + A single-line summary suitable for a UnifiClientError message. + """ + n = len(failures) + detail = "; ".join(failures[:max_detail]) + if n > max_detail: + detail += f"; ...and {n - max_detail} more" + return f"{n} user update(s) failed this cycle: {detail}" + + def _is_email_conflict(code: str | None) -> bool: """True if a UniFi error code signals an already-registered email. diff --git a/tests/test_unifi_client.py b/tests/test_unifi_client.py index 167ac33..8a5539d 100644 --- a/tests/test_unifi_client.py +++ b/tests/test_unifi_client.py @@ -20,6 +20,7 @@ UnifiClient, UnifiClientError, _compute_nfc_id, + _format_apply_failure_summary, _is_email_conflict, _parse_nfc_id, _parse_sync_alias, @@ -106,6 +107,25 @@ def test_is_email_conflict_matches_email_exist_codes() -> None: assert _is_email_conflict("") is False +# --- apply-failure summary --- + + +def test_format_apply_failure_summary_caps_detail() -> None: + """A widespread failure caps the detailed entries with '...and N more' so + the summary (and any alert built from it) stays bounded.""" + fails = [f"contact={i}: err" for i in range(1, 13)] # 12 failures + msg = _format_apply_failure_summary(fails, max_detail=10) + assert msg.startswith("12 user update(s) failed this cycle: ") + assert msg.count("contact=") == 10 # only 10 detailed + assert "...and 2 more" in msg + + +def test_format_apply_failure_summary_no_truncation_when_under_cap() -> None: + msg = _format_apply_failure_summary(["contact=1: boom"], max_detail=10) + assert msg == "1 user update(s) failed this cycle: contact=1: boom" + assert "more" not in msg + + # --- Name splitting ---