[FIX] Wrap set_user_organization in transaction.atomic + worktree skill safety#1954
Open
chandrasekharan-zipstack wants to merge 2 commits into
Open
[FIX] Wrap set_user_organization in transaction.atomic + worktree skill safety#1954chandrasekharan-zipstack wants to merge 2 commits into
chandrasekharan-zipstack wants to merge 2 commits into
Conversation
The new-org branch creates the org row, then calls frictionless onboarding and the initial platform key. Failures mid-flow leave an orphan org with no adapters or key, and subsequent logins skip onboarding entirely (gated on new_organization). Atomic ensures the org rolls back on any failure so retries get a clean fresh-org path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Walkthrough
ChangesDatabase Transaction Wrapper for Organization Setup
Worktree Command Documentation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/account_v2/authentication_controller.py (1)
167-241: ⚡ Quick winDefer cache/session mutations until transaction commit.
The method correctly uses
@transaction.atomicat line 167, but side effects on cache/session (lines 231-241) execute before the transaction commits. If commit fails after these mutations run, the session/cache state becomes inconsistent with the database. Wrap these side effects intransaction.on_commit()to ensure they execute only after successful commit.Proposed patch
current_organization_id = UserSessionUtils.get_organization_id(request) - if current_organization_id: - OrganizationMemberService.remove_user_membership_in_organization_cache( - user_id=user.user_id, - organization_id=current_organization_id, - ) - UserSessionUtils.set_organization_id(request, organization_id) - UserSessionUtils.set_organization_member_role(request, organization_member) - OrganizationMemberService.set_user_membership_in_organization_cache( - user_id=user.user_id, organization_id=organization_id - ) + + def _finalize_org_context() -> None: + if current_organization_id: + OrganizationMemberService.remove_user_membership_in_organization_cache( + user_id=user.user_id, + organization_id=current_organization_id, + ) + UserSessionUtils.set_organization_id(request, organization_id) + UserSessionUtils.set_organization_member_role( + request, organization_member + ) + OrganizationMemberService.set_user_membership_in_organization_cache( + user_id=user.user_id, organization_id=organization_id + ) + + transaction.on_commit(_finalize_org_context) return response🤖 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 `@backend/account_v2/authentication_controller.py` around lines 167 - 241, The cache/session mutations at the end of set_user_organization (calls to OrganizationMemberService.remove_user_membership_in_organization_cache, UserSessionUtils.set_organization_id, UserSessionUtils.set_organization_member_role, and OrganizationMemberService.set_user_membership_in_organization_cache) must be deferred until the DB transaction commits; wrap those side-effecting calls in a transaction.on_commit(lambda: ...) closure inside set_user_organization so they execute only after successful commit, capturing user.user_id, organization_id, organization_member and request in the closure.
🤖 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.
Nitpick comments:
In `@backend/account_v2/authentication_controller.py`:
- Around line 167-241: The cache/session mutations at the end of
set_user_organization (calls to
OrganizationMemberService.remove_user_membership_in_organization_cache,
UserSessionUtils.set_organization_id,
UserSessionUtils.set_organization_member_role, and
OrganizationMemberService.set_user_membership_in_organization_cache) must be
deferred until the DB transaction commits; wrap those side-effecting calls in a
transaction.on_commit(lambda: ...) closure inside set_user_organization so they
execute only after successful commit, capturing user.user_id, organization_id,
organization_member and request in the closure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b332483-d81e-45d9-b771-de255d2e30f7
📒 Files selected for processing (1)
backend/account_v2/authentication_controller.py
Without --no-track, a later `git push -u origin <branch>` can be reported by the server as also fast-forwarding main, landing commits on main.
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
Contributor
|
| Filename | Overview |
|---|---|
| backend/account_v2/authentication_controller.py | Added @transaction.atomic to set_user_organization; the fix is correct in intent, but create_tenant_user swallows an IntegrityError without a savepoint, which poisons the outer transaction and turns a race-condition edge case into a guaranteed 500. |
| backend/account_v2/authentication_helper.py | create_initial_platform_key has a bare except Exception that silently swallows all failures; non-DB exceptions from generate_platform_key will not propagate to the new atomic boundary, so the org row is committed even when platform-key creation silently fails. |
| .claude/skills/worktree/SKILL.md | Added --no-track flag to git worktree add with a clear explanatory comment; change is correct and prevents accidental commits landing on main via upstream tracking. |
Comments Outside Diff (2)
-
backend/account_v2/authentication_controller.py, line 454-493 (link)IntegrityErrorswallowed insidecreate_tenant_userpoisons the outer transactioncreate_tenant_usercatchesIntegrityErroron line 492 without re-raising it. In PostgreSQL + Django, any database exception setsconnection.needs_rollback = Trueeven when the exception is caught in application code. With the new@transaction.atomicwrappingset_user_organization, any subsequent DB operation after this swallowedIntegrityErrorwill immediately raiseTransactionManagementError: An error occurred in the current transaction. You can't execute queries until the end of the atomic block.That error propagates out and does trigger a rollback — but it means a valid "duplicate tenant user" race-condition is now a guaranteed 500 instead of the previous silent continuation. The fix is to wraptenant_user.save()in a nestedwith transaction.atomic():savepoint to isolate the error.Prompt To Fix With AI
This is a comment left during a code review. Path: backend/account_v2/authentication_controller.py Line: 454-493 Comment: **`IntegrityError` swallowed inside `create_tenant_user` poisons the outer transaction** `create_tenant_user` catches `IntegrityError` on line 492 without re-raising it. In PostgreSQL + Django, any database exception sets `connection.needs_rollback = True` even when the exception is caught in application code. With the new `@transaction.atomic` wrapping `set_user_organization`, any subsequent DB operation after this swallowed `IntegrityError` will immediately raise `TransactionManagementError: An error occurred in the current transaction. You can't execute queries until the end of the atomic block.` That error propagates out and does trigger a rollback — but it means a valid "duplicate tenant user" race-condition is now a guaranteed 500 instead of the previous silent continuation. The fix is to wrap `tenant_user.save()` in a nested `with transaction.atomic():` savepoint to isolate the error. How can I resolve this? If you propose a fix, please make it concise.
-
backend/account_v2/authentication_helper.py, line 76-87 (link)create_initial_platform_keysilently swallows all exceptions, defeating the rollback for its failure caseThe
except Exceptionblock on line 83 catches everything and only logs — no exception propagates. For non-DB exceptions fromgenerate_platform_key(e.g., an external-API or config error), this catch prevents any exception from reaching the atomic boundary, so the org and tenant member rows are committed even though the platform key was never created. If the intent is for a failed platform-key creation to cancel the entire onboarding, this method should let the exception propagate rather than swallowing it.Prompt To Fix With AI
This is a comment left during a code review. Path: backend/account_v2/authentication_helper.py Line: 76-87 Comment: **`create_initial_platform_key` silently swallows all exceptions, defeating the rollback for its failure case** The `except Exception` block on line 83 catches everything and only logs — no exception propagates. For non-DB exceptions from `generate_platform_key` (e.g., an external-API or config error), this catch prevents any exception from reaching the atomic boundary, so the org and tenant member rows are committed even though the platform key was never created. If the intent is for a failed platform-key creation to cancel the entire onboarding, this method should let the exception propagate rather than swallowing it. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
backend/account_v2/authentication_controller.py:454-493
**`IntegrityError` swallowed inside `create_tenant_user` poisons the outer transaction**
`create_tenant_user` catches `IntegrityError` on line 492 without re-raising it. In PostgreSQL + Django, any database exception sets `connection.needs_rollback = True` even when the exception is caught in application code. With the new `@transaction.atomic` wrapping `set_user_organization`, any subsequent DB operation after this swallowed `IntegrityError` will immediately raise `TransactionManagementError: An error occurred in the current transaction. You can't execute queries until the end of the atomic block.` That error propagates out and does trigger a rollback — but it means a valid "duplicate tenant user" race-condition is now a guaranteed 500 instead of the previous silent continuation. The fix is to wrap `tenant_user.save()` in a nested `with transaction.atomic():` savepoint to isolate the error.
### Issue 2 of 2
backend/account_v2/authentication_helper.py:76-87
**`create_initial_platform_key` silently swallows all exceptions, defeating the rollback for its failure case**
The `except Exception` block on line 83 catches everything and only logs — no exception propagates. For non-DB exceptions from `generate_platform_key` (e.g., an external-API or config error), this catch prevents any exception from reaching the atomic boundary, so the org and tenant member rows are committed even though the platform key was never created. If the intent is for a failed platform-key creation to cancel the entire onboarding, this method should let the exception propagate rather than swallowing it.
Reviews (1): Last reviewed commit: "[MISC] Worktree skill — use --no-track t..." | Re-trigger Greptile
chandrasekharan-zipstack
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two small reliability fixes:
AuthenticationController.set_user_organizationis now wrapped in@transaction.atomicso the new-org branch is a single all-or-nothing unit of work.git worktree add --no-trackto prevent the new branch from inheritingorigin/mainas its upstream — a subtle footgun where a latergit push -u origin <branch>can be reported by the server as fast-forwardingmain.Why — atomic wrap
set_user_organizationcreates an organization row and a tenant member, then calls into pluggable hooks. Each step can fail for unrelated reasons. Without a transaction, a failure mid-flow leaves the organization row persisted with no follow-up bookkeeping completed.That partial state is self-perpetuating: the next login finds the organization already exists, takes the existing-org branch, and never re-runs the hooks. The user ends up logged in but in a workspace missing the state those hooks would have created — no automatic recovery path.
Wrapping the method in atomic means a failure rolls back the org row along with the tenant member, so a retry sees a clean fresh-org path.
Note: this only addresses DB state. External resources created inside the pluggable hooks are out of scope here.
Why — worktree skill --no-track
When
git worktree add -b <branch> $PATH origin/mainruns without--no-track, the new branch's upstream is set toorigin/main. A subsequentgit push -u origin <branch>is then susceptible to being interpreted by the server as also fast-forwardingmain, depending on the remote's push config.--no-trackremoves the implicit link without any other behaviour change.Test plan
account_v2_organizationfor the attempted org.🤖 Generated with Claude Code