fix(unifi): best-effort email + per-user resilient apply (CODE_ADMIN_EMAIL_EXIST crash) - #32
Conversation
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) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesPer-contact failure isolation and best-effort email writes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Coverage OverviewLanguages: Python Python / code-coverage/pytestThe overall coverage remains at 93%, unchanged from the Updated |
There was a problem hiding this comment.
Pull request overview
This PR hardens the UniFi Access reconciliation loop against persistent email-collision errors and ensures a single bad user record cannot crash an entire sync cycle.
Changes:
- Extend
UnifiClientErrorto carry the UniFi envelopecode, enabling targeted recovery logic. - Add best-effort email writes via
_request_user_write, retrying withoutuser_emailon*_EMAIL_EXISTconflicts. - Isolate per-contact apply failures, continuing the cycle while collecting failures and raising a summary error at the end; add tests and update architecture docs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/door_sync/unifi/client.py |
Adds per-user failure isolation, best-effort email syncing with conflict retry, and propagates UniFi error codes via UnifiClientError. |
tests/test_unifi_client.py |
Adds regression tests for email-conflict retry behavior and for per-user isolation + summary failure reporting. |
docs/architecture.md |
Documents the new per-user isolation behavior and the best-effort email sync semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_unifi_client.py (1)
1559-1566: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the retried create payload keeps the required non-email fields.
This test only proves
user_emailis removed on retry. If the retry body accidentally droppedemployee_numberor the split name fields too, the mock would still return success and this would pass. Please decode the second POST body and assert it still contains the expected create payload minususer_email.Suggested change
assert len(posts) == 2 assert "user_email" in _json.loads(posts[0].content) - assert "user_email" not in _json.loads(posts[1].content) + retry_body = _json.loads(posts[1].content) + assert "user_email" not in retry_body + assert retry_body == { + "first_name": "Jane", + "last_name": "Doe", + "employee_number": "42", + } assert any("email already registered" in r.message for r in caplog.records)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_unifi_client.py` around lines 1559 - 1566, The retry assertion in the create-user test only checks that user_email is removed, so it can miss regressions where other required fields are lost. Update the test around the POST capture in test_unifi_client.py to decode the second request body and assert it still includes the expected create payload fields, especially employee_number and the split name fields, while only omitting user_email. Use the existing httpx_mock.get_requests() POST filtering and _json.loads on the second request body to compare the full payload shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/door_sync/unifi/client.py`:
- Around line 428-436: The per-contact failure logging in _record_apply_failure
currently records only the error message and loses the traceback context. Update
the logger.error call in UnifiClient._record_apply_failure to pass exc_info=exc
so the original stack is preserved even outside the active except block, while
keeping the existing concise _apply_failures summary append unchanged.
In `@tests/test_unifi_client.py`:
- Line 1253: The tests creating UnifiClient instances via make_client are
leaving clients open, which can leak sockets/FDs. Update the affected test
blocks to use a context manager (for example, with make_client(...) as client:)
or ensure client.close() runs in a finally block. Apply this to each affected
test that currently assigns client directly, so the cleanup is explicit and not
dependent on fixture teardown.
---
Nitpick comments:
In `@tests/test_unifi_client.py`:
- Around line 1559-1566: The retry assertion in the create-user test only checks
that user_email is removed, so it can miss regressions where other required
fields are lost. Update the test around the POST capture in test_unifi_client.py
to decode the second request body and assert it still includes the expected
create payload fields, especially employee_number and the split name fields,
while only omitting user_email. Use the existing httpx_mock.get_requests() POST
filtering and _json.loads on the second request body to compare the full payload
shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4169e847-bf08-4a56-9499-51b5bd065d85
📒 Files selected for processing (6)
docs/architecture.mddocs/architecture/conventions.rstdocs/architecture/reconciliation.rstdocs/superpowers/specs/2026-06-17-sync-member-email-design.mdsrc/door_sync/unifi/client.pytests/test_unifi_client.py
…d summary) 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) <noreply@anthropic.com>
The crash
A live sync crashed with
CODE_ADMIN_EMAIL_EXIST: Email address is already registered:UniFi Access requires emails to be globally unique across users AND admins. Several staff are both UniFi admins and CiviCRM members, so writing a member's CiviCRM email (already registered to their admin account) is rejected. Nothing caught it, so the exception propagated out of
apply()and crashed the whole reconcile cycle — and because the collision is persistent, every cycle re-crashed, leaving sync stuck in a crash/alert loop (and the bundled name/card update for that user never applied).Fix (both behaviors confirmed with the maintainer)
1. Email is best-effort.
UnifiClientErrornow carries the envelopecode. A new_request_user_writehelper — used by all three email-writing paths (create, reactivate, credential-update) — catches an*_EMAIL_EXISTcode, retries the write withoutuser_email, and logs a warning. Name/card/policy still apply; the member keeps door access; only the conflicting email isn't synced. These are not counted as failures, so they don't alert (expected for admin-members).2. Per-user resilient apply. Each contact's apply step is wrapped: a
UnifiClientErroris logged + recorded and the cycle continues with the rest, thenapply()raises one summary error so the orchestrator still alerts on genuine failures. One bad record never blocks the others. (The batch card pre-import stays fail-fast as a shared prerequisite.) Extracted_apply_one_credentialto keep the loop readable.Tests
user_emailand the rest applies + warns)._is_email_conflictunit test (matches ADMIN/USEREMAIL_EXIST, nothing else).apply()raises a summary naming contact 1.Verification
uv run pytest→ 347 passeduv run pyrefly check→ 0 errorsuv run ruff check ./ruff format --check .→ cleansphinx-build -W→ cleanNote / possible follow-up
For an admin-member whose email never syncs, the reconciler keeps seeing the email as "differing" and will attempt (then best-effort-skip) it every cycle, emitting the warning each time. Harmless but recurring. Suppressing that re-diff would need state ("email is unsyncable for this contact") — happy to do it as a follow-up if the recurring warnings are noisy.
🤖 Generated with Claude Code
Summary by CodeRabbit