Skip to content

fix(api): skip writing back an unchanged preferred namespace on login - #7385

Merged
otavio merged 1 commit into
masterfrom
fix/api/login-preferred-namespace-race
Oct 8, 2026
Merged

otavio merged 1 commit into
masterfrom
fix/api/login-preferred-namespace-race

Conversation

@luizhf42

@luizhf42 luizhf42 commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Summary

Stacked on #7378 so the cloud e2e job runs with the billing test already fixed. The two changes don't depend on each other.

AuthLocalUser reads the user's preferred namespace and then writes it back, changed or not. If the namespace is deleted between the read and the write, ON DELETE SET NULL clears the column, the write puts the deleted ID back, and the foreign key turns login into a 500.

 AuthLocalUser
   ns := NamespaceGetPreferred(user)          # reads namespace X
   ...                                        # namespace X deleted here, column set to NULL
-  UserUpdatePreferredNamespace(user, X)      # FK violation, login answers 500
+  if user.PreferredNamespace != tenantID     # X == X, nothing to write
+    UserUpdatePreferredNamespace(user, tenantID)

CreateUserToken already does this check.

Evidence

  • Before: the cloud e2e job on test(ui): accept a final invoice Stripe already finalized #7378 failed in billing.spec.ts:377 "a namespace with an active subscription is deleted only once it is canceled". The test polls login right after deleting the namespace, and the server logged:
    POST /api/login 500 ... violates foreign key constraint "users_preferred_namespace_id_fkey"
  • After: four TestService_AuthLocalUser cases no longer expect a write of the unchanged value, so a write back fails them. Against the old auth.go, "succeeds to authenticate without a namespace" fails. With the fix, all of server/api/services passes.

The race is still there when the value changes: a user with no preferred namespace whose login picks a namespace being deleted at that moment. That window is much narrower, and nothing has hit it.

Merge Danger

Door: two-way

Blast Radius: login

Login skips a write that stored the value already in the column. The token and its claims are unchanged.

@luizhf42
luizhf42 requested a review from a team as a code owner October 7, 2026 20:30
@luizhf42
luizhf42 added this pull request to stack #7386 October 7, 2026 20:30
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Claude finished @luizhf42's task in 1m 15s —— View job


Code Review Complete

I reviewed 2 files for Standards (repo rules, Go conventions, tests, code smells) and Correctness (security and runtime defects). I found no new issues, and the code looks good as it is. The Spec axis did not run because the PR references no issue. I did all of the review myself rather than splitting it across the four sub-agents the procedure calls for, because the diff is 22 lines.

What I checked:

  • The fix works. The full-row UserUpdate can't put the deleted namespace back. entity.UserPreferences.PreferredNamespace carries bun:"...,skipupdate" (server/api/store/pg/entity/user.go:36), so UserUpdatePreferredNamespace was the only write that could, and the new guard at server/api/services/auth.go:600 skips it when the value hasn't changed. The guard also matches the one in CreateUserToken (auth.go:672).
  • The tests still cover both cases. The "empty preferred namespace" case (auth_test.go:2022) still expects a write. The unchanged-value cases now set no expectation for UserUpdatePreferredNamespace, so an extra write back would hit a mock with no matching expectation and fail the test.
  • Cloud is unaffected. No exported symbol changed.

@luizhf42 luizhf42 self-assigned this Oct 7, 2026
@luizhf42
luizhf42 requested a review from a team as a code owner October 7, 2026 20:43
@luizhf42
luizhf42 force-pushed the fix/api/login-preferred-namespace-race branch from 45f25e9 to 9818dd4 Compare October 7, 2026 20:45
@luizhf42
luizhf42 force-pushed the fix/api/login-preferred-namespace-race branch from 9818dd4 to 737e129 Compare October 7, 2026 21:06
@gustavosbarreto

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Claude finished @gustavosbarreto's task in 1m 10s —— View job


Code Review Complete

Reviewed 2 files for Standards (repo rules, Go conventions, tests, code smells) and Correctness (security and runtime defects). I found no new issues, and the code looks good as it is. The Spec axis did not run because the PR and its commit reference no issue.

@gustavosbarreto
gustavosbarreto force-pushed the fix/api/login-preferred-namespace-race branch from 737e129 to d5c186e Compare October 7, 2026 21:37
Base automatically changed from test/ui/billing-final-invoice-paid to master October 8, 2026 10:43
@luizhf42
luizhf42 force-pushed the fix/api/login-preferred-namespace-race branch from d5c186e to ed9c26b Compare October 8, 2026 11:35
`AuthLocalUser` reads the user's preferred namespace and then writes it
back, changed or not. When the namespace is deleted between the read
and the write, `ON DELETE SET NULL` clears the column, and the write
puts the deleted namespace's ID back. The foreign key rejects it and
login answers 500. The cloud e2e job hit this when a test polled login
right after deleting a namespace:

  violates foreign key constraint "users_preferred_namespace_id_fkey"

Login now writes the preferred namespace only when it differs from the
one it read, as `CreateUserToken` already does. The service tests that
expected a write of an unchanged value no longer expect one, so a write
back of the same namespace fails them.
@luizhf42
luizhf42 force-pushed the fix/api/login-preferred-namespace-race branch from ed9c26b to e5dff2b Compare October 8, 2026 14:19
@otavio
otavio merged commit 3e7616e into master Oct 8, 2026
28 checks passed
@otavio
otavio deleted the fix/api/login-preferred-namespace-race branch October 8, 2026 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants