github-mirrors/netris-nestri
1
0
Fork 0
mirror of https://github.com/nestriness/nestri.git synced 2026-09-19 17:25:19 +03:00
Code Issues Packages Projects Releases Wiki Activity

fix(core): hold the account rules when two requests arrive together

Browse Source 
Three rules here are enforced across a lookup and then a write, and each
was only as good as whatever stopped the two from interleaving. Nothing
did.

The connection cap counted with `select ... for update` over the
connections a user already had. That locks the rows it finds, and when
it finds none it locks nothing — there are no gap locks under read
committed — so several first-time links all counted zero and all
inserted. Six concurrent links against a cap of four produced six. The
count now happens under a lock on the account's own row, which is the
one thing every caller for that account is guaranteed to contend on.

Creating an account from a verified address looked the address up and
then inserted. Two tabs finishing the same sign-in both found nothing,
and the loser got the driver's constraint violation instead of the
account the winner had just made. The unique index is the thing that
actually arbitrates, so the loser now reads back what the winner wrote.
Claiming an address on an older account had the same shape and now gives
the same sentence a screen would have shown a moment earlier.

The tests run each call several times at once against a real database,
because run one at a time all three pass whether or not any of this
exists.
This commit is contained in:
Wanjohi
2026-09-05 09:32:16 +03:00
parent 2c4e9d9b0b
commit 15f8d3eb34
2 changed files with 202 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

70
packages/core/src/user/identity.test.ts
View File

@@ -221,3 +221,73 @@ describe('Identity.linkSteam', () => {
expect(resolved.userID).toBe(legacy.userID);
});
});
/**
* The same call, several times at once, against a real database.
*
* Every one of these holds a rule that is enforced across two statements — a
* lookup and then a write — which means the rule is only as good as whatever
* stops the two from interleaving. Run one at a time they all pass whether or
* not that protection exists, which is exactly why they are written this way.
*/
describe('the same thing happening twice at once', () => {
beforeEach(cleanup);
test('the cap holds when the links arrive together', async () => {
const { userID } = await Identity.fromVerifiedEmail({ email: email(8) });
track(userID);
const wanted = Identity.MAX_STEAM_ACCOUNTS + 2;
const results = await Promise.allSettled(
Array.from({ length: wanted }, (_, i) =>
Identity.linkSteam({ userId: userID, steamId: steamID(60 + i) })
)
);
expect(await Identity.listSteam(userID)).toHaveLength(Identity.MAX_STEAM_ACCOUNTS);
expect(results.filter((r) => r.status === 'fulfilled')).toHaveLength(
Identity.MAX_STEAM_ACCOUNTS
);
for (const rejected of results.filter((r) => r.status === 'rejected')) {
expect((rejected as PromiseRejectedResult).reason.code).toBe('invalid_state');
}
});
test('several sign-ins for one new address make one account', async () => {
const address = email(9);
const results = await Promise.all([
Identity.fromVerifiedEmail({ email: address }),
Identity.fromVerifiedEmail({ email: address }),
Identity.fromVerifiedEmail({ email: address })
]);
results.forEach((r) => track(r.userID));
expect(new Set(results.map((r) => r.userID)).size).toBe(1);
expect(results.filter((r) => r.created)).toHaveLength(1);
const rows = await sql`
select count(*)::int as n from "user"
where email = ${address} and time_deleted is null
`;
expect(rows[0]!.n).toBe(1);
});
test('two accounts claiming one address get an answer rather than a driver error', async () => {
const first = await legacySteamUser(70);
const second = await legacySteamUser(71);
const address = email(10);
const results = await Promise.allSettled([
Identity.claimWithEmail({ userId: first.userID, email: address }),
Identity.claimWithEmail({ userId: second.userID, email: address })
]);
const rejected = results.filter((r) => r.status === 'rejected') as PromiseRejectedResult[];
expect(rejected).toHaveLength(1);
// The point of the assertion: a sentence a screen can render, and not
// whatever text the driver puts on a constraint violation.
expect(rejected[0]!.reason.type).toBe('already_exists');
expect(rejected[0]!.reason.message).toMatch(/another account/);
});
});

179
packages/core/src/user/identity.ts
View File

@@ -6,11 +6,38 @@ import { ErrorCodes, VisibleError } from '../error.js';
import { fn } from '../fn.js';
import { Identifier } from '../id.js';
import { User } from './index.js';
import { UserTable } from './user.sql.js';
import { LinkedAccount } from './linked-account.js';
import { LinkedAccountTable } from './linked-account.sql.js';
const STEAM_ID_RE = /^\d{17}$/;
/** The partial unique index on a live account's address, named by the migration. */
const EMAIL_UNIQUE = 'user_email_unique';
/**
* Whether a failure is the database refusing a duplicate.
*
* Every read-then-write below has a window between the read and the write, and
* the index is what actually closes it. Recognising the refusal is how the
* loser of a race turns a raw driver error into the answer it was asking for —
* so the constraint is the mechanism and this is how the code hears from it.
*/
function isUniqueViolation(err: unknown, constraint: string): boolean {
// Walked rather than read off the top, because the query builder wraps what
// the driver threw: the outer error carries the SQL and the parameters, and
// the code and the constraint name are on the cause underneath it.
for (let e: unknown = err, depth = 0; e && depth < 8; depth++) {
if (typeof e !== 'object') break;
const candidate = e as { code?: unknown; constraint_name?: unknown; cause?: unknown };
if (String(candidate.code) === '23505' && candidate.constraint_name === constraint) {
return true;
}
e = candidate.cause;
}
return false;
}
/**
* Email is the root of an account, so two spellings of one address must not be
* two accounts. Case and surrounding whitespace are the two ways the same
@@ -51,27 +78,48 @@ export namespace Identity {
z.object({ email: Email, name: z.string().optional() }),
async (input) => {
const email = input.email;
return Database.transaction(async () => {
const existing = await User.fromEmail(email);
if (existing) {
// An address that was attached but never confirmed is
// confirmed now: getting here means a code was redeemed.
if (!existing.emailVerified) {
await User.setEmail({ id: existing.id, email, emailVerified: true });
}
return { userID: existing.id, created: false };
}
const userID = Identifier.ascending('user');
await User.create({
id: userID,
name: input.name?.trim() || email.split('@')[0]!,
email,
emailVerified: true,
image: null
async function attempt() {
return Database.transaction(async () => {
const existing = await User.fromEmail(email);
if (existing) {
// An address that was attached but never confirmed is
// confirmed now: getting here means a code was redeemed.
if (!existing.emailVerified) {
await User.setEmail({ id: existing.id, email, emailVerified: true });
}
return { userID: existing.id, created: false };
}
const userID = Identifier.ascending('user');
await User.create({
id: userID,
name: input.name?.trim() || email.split('@')[0]!,
email,
emailVerified: true,
image: null
});
return { userID, created: true };
});
return { userID, created: true };
});
}
try {
return await attempt();
} catch (err) {
if (!isUniqueViolation(err, EMAIL_UNIQUE)) throw err;
// Somebody else finished the same sign-in first.
//
// Two people redeeming a code for one address is one person
// with two tabs, and the answer they both want is the account
// that now exists. The lookup and the insert cannot be made one
// statement here — the row is built from an id this process
// generates — so the index arbitrates and the loser reads back
// what the winner wrote. Retried once and not in a loop: a
// second refusal means the row is gone again, which is a
// deletion racing a sign-in and not something to spin on.
return await attempt();
}
}
);
@@ -86,29 +134,43 @@ export namespace Identity {
z.object({ userId: z.string(), email: Email }),
async (input) => {
const email = input.email;
return Database.transaction(async () => {
const holder = await User.fromEmail(email);
if (holder && holder.id !== input.userId) {
throw new VisibleError(
'already_exists',
ErrorCodes.Validation.ALREADY_EXISTS,
'That email address already belongs to another account'
);
}
const updated = await User.setEmail({
id: input.userId,
email,
emailVerified: true
try {
return await Database.transaction(async () => {
const holder = await User.fromEmail(email);
if (holder && holder.id !== input.userId) {
throw new VisibleError(
'already_exists',
ErrorCodes.Validation.ALREADY_EXISTS,
'That email address already belongs to another account'
);
}
const updated = await User.setEmail({
id: input.userId,
email,
emailVerified: true
});
if (!updated) {
throw new VisibleError(
'not_found',
ErrorCodes.NotFound.RESOURCE_NOT_FOUND,
'No such account'
);
}
return updated;
});
if (!updated) {
throw new VisibleError(
'not_found',
ErrorCodes.NotFound.RESOURCE_NOT_FOUND,
'No such account'
);
}
return updated;
});
} catch (err) {
// The check above and the update below are two statements, so
// two accounts claiming one address can both find it free. The
// index refuses the second, and the person deserves the same
// sentence they would have got a moment earlier rather than a
// driver's error text.
if (!isUniqueViolation(err, EMAIL_UNIQUE)) throw err;
throw new VisibleError(
'already_exists',
ErrorCodes.Validation.ALREADY_EXISTS,
'That email address already belongs to another account'
);
}
}
);
@@ -143,8 +205,8 @@ export namespace Identity {
* Hang a Steam account off a user, up to {@link MAX_STEAM_ACCOUNTS}.
*
* The count and the insert are one transaction because they are one
* decision: read the four, then write the fifth, and two concurrent calls
* each see four.
* decision, and the transaction takes the account's own row first so that
* two callers cannot each count four and each write a fifth.
*/
export const linkSteam = fn(
z.object({
@@ -154,6 +216,30 @@ export namespace Identity {
}),
async (input) => {
return Database.transaction(async (tx) => {
// Take the account's own row first, and hold it.
//
// The cap is a count, and a count only means something if it
// is taken while nothing can change it. Locking the
// connections instead locks nothing at all when there are
// none: there are no gap locks under read committed, so
// `for update` over an empty result set is an empty set of
// locks, and several simultaneous first-time links all read
// zero and all insert. The account's own row is the one thing
// every caller for it is guaranteed to contend on, so it is
// what serializes them.
const [owner] = await tx
.select({ id: UserTable.id })
.from(UserTable)
.where(and(eq(UserTable.id, input.userId), isNull(UserTable.timeDeleted)))
.for('update');
if (!owner) {
throw new VisibleError(
'not_found',
ErrorCodes.NotFound.RESOURCE_NOT_FOUND,
'No such account'
);
}
const existing = await LinkedAccount.findByProvider({
provider: 'steam',
providerAccountId: input.steamId
@@ -172,8 +258,8 @@ export namespace Identity {
return existing.id;
}
// `for update` on the rows already there, so a second caller
// holding at the cap waits rather than counting alongside.
// Counted under the lock taken above, so what is counted is
// what is still there at the insert.
const held = await tx
.select({ id: LinkedAccountTable.id })
.from(LinkedAccountTable)
@@ -183,8 +269,7 @@ export namespace Identity {
eq(LinkedAccountTable.provider, 'steam'),
isNull(LinkedAccountTable.timeDeleted)
)
)
.for('update');
);
if (held.length >= MAX_STEAM_ACCOUNTS) {
throw new VisibleError(