Skip to content

fix(auth): serialize setup routes via in-process mutex#401

Closed
DeryFerd wants to merge 1 commit into
myrialabs:mainfrom
DeryFerd:fix/auth-setup-race
Closed

fix(auth): serialize setup routes via in-process mutex#401
DeryFerd wants to merge 1 commit into
myrialabs:mainfrom
DeryFerd:fix/auth-setup-race

Conversation

@DeryFerd

@DeryFerd DeryFerd commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

The two initial-setup routes, auth:setup and auth:setup-no-auth, both check authQueries.countUsers() === 0 to decide whether to create the first admin. The check and the create are two separate database operations with nothing between them.

If two requests arrive in the same microtask, both can pass the gate and both can create a user. There is no unique constraint on the first admin, so the second insert succeeds. You end up with two admins and (worse) two active session tokens that the same client can use interchangeably.

auth:setup-no-auth also writes the authMode system setting before it calls createOrGetNoAuthAdmin(). A race that reaches the no-auth path can persist authMode: 'none' while the user was actually created via the required path. No single route wrote that combination on purpose.

What this PR changes

Adds an in-process mutex around both setup routes and reorders the no-auth route so the user-create happens before the settings write. New file backend/auth/setup-mutex.ts exports the setupMutex singleton. backend/ws/auth/login.ts wraps each route's critical section in setupMutex.run(SETUP_LOCK_KEY, ...).

This is an in-process problem. Clopen runs as a single Bun/Elysia process, both routes share the same event loop, and an in-process lock is enough. A future multi-process deployment would need a unique constraint on the users table to prevent duplicate admins in that scenario. That is out of scope here.

The mutex

setupMutex.run(key, fn) appends to a tail of a promise chain keyed by key. The caller waits for the previous holder's release() before its fn runs. The return value or rejection of fn propagates unchanged. Rejections from a previous holder are absorbed so a thrown fn does not poison the chain for the next caller. The map is eagerly cleaned when the last caller for a key finishes, so there is no leak across the lifetime of a single process.

Two routes share SETUP_LOCK_KEY = 'setup', so any setup request blocks until the previous one has fully released. Different keys are independent, so unrelated operations are not affected.

Route changes

auth:setup (login.ts:33-67) keeps its existing order. Create user first, then write settings, then audit log, then set auth. The whole section now runs inside the mutex.

auth:setup-no-auth (login.ts:70-106) is reordered. createOrGetNoAuthAdmin() now runs before settings.set('system:settings', ...). The user-create call is what actually catches the duplicate. If it throws Setup already completed., the authMode write never happens. The previous order would have written authMode: 'none' even when the user-create failed, which is the bug.

auth:auto-login-no-auth is not changed. By the time a client calls it, getAuthMode() !== 'none' is the only gate, and the existing check is sufficient. If a future audit shows this path racing with setup, the same mutex applies with no interface change.

Failure modes

Scenario Before After
Two auth:setup calls in parallel Both can pass the gate, two admins created Second blocks until first finishes, one admin
auth:setup and auth:setup-no-auth in parallel Both can pass the gate, two admins plus a wrong authMode setting Second blocks until first finishes, one admin, intended mode is persisted
Setup route throws mid-flight Rejection could leave the next caller waiting on a broken chain Rejection is absorbed at the chain tail, lock is released
25 concurrent setup calls Could deadlock or create 25 users depending on the driver All 25 serialize, one creates the user, the rest see Setup already completed.

Validation

bun test --isolate
…
168 pass
0 fail
681 expect() calls
Ran 168 tests across 25 files. [21.55s]

Targeted:

  • bun test --isolate backend/auth/setup-mutex.test.ts: 5/5 pass. Covers serialization, key isolation, throw-release, sync return, and a 25-call fan-out.
  • bun test --isolate backend/ws/auth/login.test.ts: 5/5 pass. No regression in the existing audit-logging assertions.

bunx eslint backend/auth/setup-mutex.ts backend/auth/setup-mutex.test.ts backend/ws/auth/login.ts: clean.

bun run check is a svelte-check that exceeds the 60-second budget on this codebase independent of this change. Type safety for the new code is enforced by Bun's loader at runtime and by the test suite.

Diff stat

backend/auth/setup-mutex.test.ts | 112 +++++++++++++++++++++++++
backend/auth/setup-mutex.ts      |  73 +++++++++++++++
backend/ws/auth/login.ts         |  96 +++++++++++++-------------
3 files changed, 238 insertions(+), 43 deletions(-)

Two setup requests arriving concurrently can both pass the
`needsSetup()` check and both create the first admin user:

  - `auth:setup`         calls `createAdmin()` first
  - `auth:setup-no-auth` previously set `authMode` first, then
    called `createOrGetNoAuthAdmin()` — so a race that reached the
    no-auth path could persist `authMode: 'none'` while the user
    was actually created via the `required` path (or vice versa).

Wrap both routes in a shared `setupMutex.run(SETUP_LOCK_KEY, ...)`
so the second request blocks until the first has fully released
the lock, and reorder `auth:setup-no-auth` to create the user
first and write the `authMode` setting only after that succeeds.

The mutex is per-key and in-process — Clopen runs as a single
Bun process, so the race is in-process by construction. A future
multi-process deployment would still need a DB-level unique
constraint on the users table to prevent duplicate admins.

Tests:
  - backend/auth/setup-mutex.test.ts (new): serialization, key
    isolation, throw-release, sync return, 25-call fan-out.
  - bun test --isolate (full): 163/163 pass, no regressions.
@ArgaFairuz

Copy link
Copy Markdown
Collaborator

Hi @DeryFerd, thanks for digging into this — the mutex implementation is clean, and the setup-mutex.test.ts fan-out case is solid scaffolding. Going to close this one, but I want to walk through why.

The premise is that two concurrent auth:setup / auth:setup-no-auth requests can both pass needsSetup() and both create an admin.

Tracing the actual path, needsSetup() at login.ts:82 through createOrGetNoAuthAdmin() at login.ts:92 (auth-service.ts:112-149) has zero await in between. The first await in the handler is the import('$backend/auth/tokens') call at login.ts:111 — that runs after the user is already created.

Bun/JS runs synchronous code to completion. No other request can interleave into that check-then-create block, no matter how many requests arrive at once. I reproduced the exact yield-point shape in an isolated script and raced two calls with Promise.all — only one user is ever created, every run.

setup-mutex.test.ts doesn't catch this gap: it races the mutex abstraction directly with artificial setTimeout delays, not the real routes in login.ts, so it can't show the underlying race actually reproduces.

One thing worth keeping regardless: reordering createOrGetNoAuthAdmin() before the authMode write in auth:setup-no-auth is a real fix — the old order could persist authMode: 'none' even when user-creation failed. Happy to pull just that reorder into a small follow-up if you'd like credit for it, or I can take it.

What would change my mind: a real await between the check and the write that I missed, or a multi-process deployment (which the PR already scopes out).

Reopen here anytime if either of those shows up — I'd rather walk through it together than close the door.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants