backend: clean up Twilio caller IDs when an account is deleted

[mdmohsin7]

Closes #7640

Problem

When a user deletes their omi account, the background wipe removes their Firestore data but never tells Twilio about it. The verified outgoing caller IDs they had registered stay on our Twilio account forever.

Twilio enforces uniqueness at the account level, so any later attempt to verify the same number — by a brand-new omi user, or by the same person signing back up — fails with error 21450 → "This phone number is already registered." Remediation today is an engineer manually calling client.outgoing_caller_ids({sid}).delete() after confirming no live omi user owns the number.

Fix

• New utils.twilio_service.delete_user_caller_ids(uid) reads users/{uid}/phone_numbers, calls delete_caller_id(twilio_sid) for every entry that has one. Best-effort: per-entry Twilio errors and the phone-list fetch are caught + logged so a momentary Twilio issue cannot leave the user half-deleted.
• _background_wipe_user_data (in routers/users.py) calls it before delete_user_data(uid) — must run while the twilio_sid metadata is still in Firestore.
• 5 unit tests cover: per-entry call, skipping entries without twilio_sid, continuing past a raising delete call, no-phone-numbers no-op, and a Firestore list failure being swallowed.

Test plan

• pytest tests/unit/test_twilio_account_deletion.py -v → 5 passed
• pytest tests/unit/test_twilio_service.py -v (existing) → still passes
• black-formatted at line-length 120, skip-string-normalization
• No PII in commits / tests / PR

[greptile-apps]

Greptile Summary

This PR fixes a resource leak where deleted Omi accounts left their verified Twilio outgoing caller IDs registered at the account level, permanently blocking re-verification of those phone numbers for any future user. The fix hooks into the existing _background_wipe_user_data path to clean up Twilio before the Firestore data is removed.

• utils/twilio_service.py: New delete_user_caller_ids(uid) reads users/{uid}/phone_numbers, calls delete_caller_id for each entry that carries a twilio_sid, and returns the count of successfully deleted IDs. Both the Firestore fetch and each per-entry Twilio call are independently guarded so a Twilio or DB error cannot leave the account in a half-deleted state.
• routers/users.py: delete_user_caller_ids is called first in _background_wipe_user_data, before delete_user_data, ensuring the twilio_sid metadata is still present in Firestore at call time.
• tests/unit/test_twilio_account_deletion.py: Five unit tests covering the happy path, skip-without-sid, per-entry exception continuity, empty list, and Firestore list failure; the top-level sys.modules stub correctly models the module boundary so patch.object intercepts calls in twilio_service's namespace.

Confidence Score: 5/5

Safe to merge — the Twilio cleanup is fully best-effort and cannot block or corrupt the Firestore wipe that follows it.

The ordering guarantee (Twilio before Firestore) is correctly enforced by sequential calls in the background task. delete_user_caller_ids cannot raise to its caller under any real condition — both the Firestore list and each per-entry Twilio call are independently caught and logged. No data loss or half-deleted state is possible.

No files require special attention.

Important Files Changed

Filename	Overview
backend/utils/twilio_service.py	Adds delete_user_caller_ids with top-level import (previously flagged in-function import is now fixed); logic is correct and best-effort by design with clear comments.
backend/routers/users.py	Calls delete_user_caller_ids before delete_user_data — correct ordering to ensure Twilio SID metadata is still in Firestore; wrapped in the existing outer try/except.
backend/tests/unit/test_twilio_account_deletion.py	Five unit tests with correct patch.object targeting; covers SID iteration, skip-without-sid, per-entry exception continuity, empty list, and Firestore list failure — all assertions are accurate.
backend/test.sh	Adds the new test file to the unit test suite in the correct position.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Router as routers/users.py
    participant TwilioSvc as utils/twilio_service.py
    participant Twilio as Twilio API
    participant DB as database/phone_calls.py
    participant Firestore

    Client->>Router: DELETE /v1/users/delete-account
    Router->>Router: _background_wipe_user_data(uid) [background thread]
    Router->>TwilioSvc: delete_user_caller_ids(uid)
    TwilioSvc->>DB: get_phone_numbers(uid)
    DB->>Firestore: "users/{uid}/phone_numbers.stream()"
    Firestore-->>DB: phone number docs
    DB-->>TwilioSvc: "[{twilio_sid, ...}, ...]"
    loop For each entry with twilio_sid
        TwilioSvc->>TwilioSvc: delete_caller_id(sid)
        TwilioSvc->>Twilio: outgoing_caller_ids(sid).delete()
        Twilio-->>TwilioSvc: success / error (swallowed)
    end
    TwilioSvc-->>Router: deleted count
    Router->>Router: delete_user_data(uid)
    Note over Router,Firestore: Firestore wipe runs after Twilio cleanup

Loading

_{Reviews (2): Last reviewed commit: "backend: greptile review — top-level imp..." | Re-trigger Greptile}

[greptile-apps]

In-function import violates team rule

from database import phone_calls as phone_calls_db is placed inside the function body. The team's import rule explicitly prohibits in-function imports and requires all imports to be at the module top-level. Thirteen other utils/ modules already import from database at the top level without issues (e.g. utils/stripe.py, utils/analytics.py).

The reason this was done here is to let the test stub database.phone_calls via sys.modules before twilio_service is imported — but that same stub approach works equally well with a top-level import, since the stub is installed before twilio_service is first touched. Moving the import to the top of the file would conform to the team standard without breaking the test.

Context Used: Backend Python import rules - no in-function impor... (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[greptile-apps]

Outer except around delete_caller_id only guards _get_client() failures

delete_caller_id already swallows every exception inside its own try/except and returns False — the only way an exception can escape it and reach the except Exception as e block here is if _get_client() raises (e.g. missing env vars) before the inner try. Twilio API errors in client.outgoing_caller_ids(sid).delete() are absorbed by delete_caller_id and produce a False return, which flows through the logger.warning branch instead.

The code is functionally correct, but the test comment "An exception escaping delete_caller_id" and the PR description "per-entry Twilio errors are caught + logged" both imply Twilio 5xx errors will hit the except path — they won't. Consider adding a brief inline note clarifying that this guard is specifically for _get_client() raising, so future maintainers don't conflate the two paths.

[mdmohsin7]

@greptile-apps re-review