fix(api): make tenant deletion cleanup atomic - #12379
Conversation
📝 WalkthroughWalkthroughTenant deletion cleanup now runs in an atomic transaction on the admin database. A regression test verifies membership rollback, exception propagation, and prevention of asynchronous cleanup when user deletion fails. A changelog entry documents the behavior. ChangesTenant deletion transaction
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
✅ All required changelog fragments are present. |
|
✅ No Conflicts No conflict markers, and the branch merges cleanly into its base. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@api/src/backend/api/v1/views.py`:
- Line 1445: Update destroy() to register delete_tenant_task with
transaction.on_commit using MainRouter.admin_db after the tenant cleanup writes,
rather than launching it immediately. Ensure the callback is registered within
the relevant atomic flow so the task starts only after the outer admin
transaction commits.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6130d580-2c75-4334-b0b7-36e8bdaaa5e8
📒 Files selected for processing (3)
api/changelog.d/tenant-deletion-transaction.fixed.mdapi/src/backend/api/tests/test_views.pyapi/src/backend/api/v1/views.py
🔒 Container Security ScanImage: ✅ No Vulnerabilities DetectedThe container image passed all security checks. No known CVEs were found.📋 Resources:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a multi-DB transactional consistency issue in the API tenant deletion flow by ensuring the cleanup transaction is executed on the same database alias (MainRouter.admin_db) used for membership/user deletion, and adds a regression test to prevent partial cleanup on failure.
Changes:
- Bind
TenantViewSet.destroy()cleanup totransaction.atomic(using=MainRouter.admin_db)to ensure rollback applies to admin-routed writes. - Add a multi-database regression test that simulates a user deletion failure via
pre_deleteand asserts membership deletion is rolled back and no async deletion task is scheduled. - Add an API changelog fragment documenting the fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| api/src/backend/api/v1/views.py | Runs tenant deletion cleanup inside an atomic transaction on MainRouter.admin_db to make membership/user cleanup rollback correctly. |
| api/src/backend/api/tests/test_views.py | Adds a regression test covering rollback behavior when exclusive-user cleanup fails under separate DB connections. |
| api/changelog.d/tenant-deletion-transaction.fixed.md | Documents the tenant deletion transactional fix as a changelog fragment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #12379 +/- ##
=======================================
Coverage 94.55% 94.55%
=======================================
Files 271 271
Lines 42191 42211 +20
=======================================
+ Hits 39894 39914 +20
Misses 2297 2297
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
🔎 Container Security Scan (Grype)Image: ✅ Nothing BlockingNo findings at critical or high severity. Not blocking at this cutoff — medium: 19, low: 4, negligible: 1. 44 finding(s) excluded by 📋 Resources:
|
Context
Tenant deletion opened
transaction.atomic()on the default database connection while membership and user writes were routed throughadmin_db. A failure during user cleanup could therefore leave memberships partially deleted.This was reproduced on the latest OSS
masterwith separatedefaultandadmindatabase connections. Cloud has a dedicated implementation, so this PR must not be synchronized automatically.Description
MainRouter.admin_db.skip-synclabel because Cloud has a separate tailored PR.Steps to review
Run the relevant API tests:
cd api uv run pytest -q src/backend/api/tests/test_views.py::TestTenantDeletionTransactions src/backend/api/tests/test_views.py::TestTenantViewSet src/backend/tasks/tests/test_deletion.py::TestDeleteTenantExpected result:
54 passed.Run the API formatting and lint checks:
Checklist
Community Checklist
SDK/CLI
API
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Summary by CodeRabbit