UN-3465 [FIX] Wrap set_user_organization in transaction.atomic#1954
Open
chandrasekharan-zipstack wants to merge 3 commits into
Open
UN-3465 [FIX] Wrap set_user_organization in transaction.atomic#1954chandrasekharan-zipstack wants to merge 3 commits into
chandrasekharan-zipstack wants to merge 3 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>
This comment has been minimized.
This comment has been minimized.
![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.
This comment has been minimized.
This comment has been minimized.
chandrasekharan-zipstack
changed the title
Contributor
Author
|
Thanks for the reviews. Investigated each angle before deciding: Greptile — Greptile — bare Both will be addressed in a follow-up that either propagates the exception properly or wraps the inner blocks in savepoints. Out of scope for this PR — that change has independent semantic implications worth reviewing on its own. CodeRabbit — Sonar — B Maintainability: cyclomatic complexity of |
Preserves the traceback when the OAuth callback hits the safety-net catch. Behaviour unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
chandrasekharan-zipstack
changed the title
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.
What
AuthenticationController.set_user_organizationin@transaction.atomicso the new-org branch (org row + tenant member + frictionless onboarding hook + initial platform key) is a single all-or-nothing unit of work./worktreeskill to usegit worktree add --no-trackso the new branch does not inheritorigin/mainas its upstream.Why
set_user_organizationcreates an organization row and a tenant member, then calls into pluggable hooks (frictionless onboarding, initial platform key). 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.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.How
from django.db import transactionand@transaction.atomictoset_user_organizationinbackend/account_v2/authentication_controller.py.--no-trackto thegit worktree addinvocation in.claude/skills/worktree/SKILL.mdwith an inline comment explaining the rationale.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
create_tenant_usercatchesIntegrityErrorwithout a savepoint. The duplicate-user race path was already broken (caller does not null-check the return —Noneflowed intoset_organization_member_roleand crashed downstream). Under atomic, the same race produces aTransactionManagementErroron commit instead of anAttributeErrorlater. Both are 500s; net behaviour change is negligible.create_initial_platform_keyhas a bareexcept Exceptionthat has always silently swallowed errors, includingDuplicateData. Under atomic, an underlying DB error insidegenerate_platform_keywill surface asTransactionManagementErroron commit instead of a silently broken org. Again, both pre/post are failure modes — the atomic version at least rolls back the org row.Database Migrations
Env Config
Relevant Docs
transaction.atomicdocsgit worktree add --no-trackRelated Issues or PRs
create_tenant_userIntegrityError handling andcreate_initial_platform_keybare except (savepoint or propagate).Dependencies Versions
Notes on Testing
account_v2_organizationfor the attempted org and a retry after fixing the upstream cause succeeds cleanly.git push -u.Screenshots
Checklist
I have read and understood the Contribution Guidelines.