From bac7223e9e6f76b466b7ac18b5b21e918e8ffda5 Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Tue, 25 Aug 2026 11:54:17 -0400 Subject: [PATCH 1/6] test: prove daily and weekly job-credit reliability defects against real MySQL The API's test suite could not load at all: `Db`'s constructor calls `knex(config[process.env.NODE_ENV])` at import time, jest sets NODE_ENV=test, and `knexfile` defined only `development` and `production`. Seven of eleven suites died with "Cannot read property 'client' of undefined" before running an assertion, and the two member-service login tests that had been failing since the daily credit stopped being awaited were invisible behind them. Adding the `test` key takes the run from 4 executed tests to 20. That key is also the connection the new database-backed specs use. They refuse to write unless CTR_INTEGRATION_TEST_DB names the configured schema exactly, so a reachable database is deliberately not enough to arm them - the API's ordinary environment names a schema these fixtures must never touch. The six defects, all reproduced against MySQL 5.7 rather than argued from the code: - a role paying 0 CityCash and 0 XP still qualifies its holder for payroll, takes a slot in the capped batch, and gets last_weekly_role_credit stamped; - among roles paying equal CityCash the selected one is arbitrary, so a higher-XP role can lose to a lower-XP one; - the weekly payout commits wallet and ledger in one transaction and member XP and eligibility in another, so failing the second leaves money moved and the member still eligible - a retry pays again; - two overlapping cron executions both select the same member and both pay; - two logins arriving together both observe "not credited today" and both pay, and the credit is not awaited, so the token can be returned before it lands; - a daily credit and a weekly credit touching one wallet each read the balance and write back their own total, so one of the two is lost. The concurrency specs install a trigger that delays every wallet update, so the races are forced rather than hoped for; the rollback specs install a trigger that fails the member half of a payout at the database, which no mock can do. Run with --runInBand: they share the member table. --- api/spec/integration-db.ts | 132 +++++++ api/src/cron/role-credit.integration.spec.ts | 342 ++++++++++++++++++ api/src/knexfile.ts | 35 ++ .../member/daily-credit.integration.spec.ts | 252 +++++++++++++ 4 files changed, 761 insertions(+) create mode 100644 api/spec/integration-db.ts create mode 100644 api/src/cron/role-credit.integration.spec.ts create mode 100644 api/src/services/member/daily-credit.integration.spec.ts diff --git a/api/spec/integration-db.ts b/api/spec/integration-db.ts new file mode 100644 index 00000000..9e3dc128 --- /dev/null +++ b/api/spec/integration-db.ts @@ -0,0 +1,132 @@ +import dotenv from 'dotenv'; +import { Knex } from 'knex'; + +// `knexfile` loads this same file, but these helpers read DB_* directly to decide whether +// they are allowed to write, so they cannot rely on that import having happened first. +// jest's cwd is `api/`, so `../.env` is the repository root env the API itself uses. +dotenv.config({ path: '../.env' }); + +/** + * Whether a spec may create and delete fixture rows in the configured database. + * + * `DB_HOST`/`DB_DATABASE` only prove that *a* database is reachable, and the API's + * ordinary environment defines both -- pointed at a shared or production schema where + * fixture INSERTs and cleanup DELETEs must never run. Writing therefore additionally + * requires `CTR_INTEGRATION_TEST_DB` to name the configured database exactly: an + * explicit, per-environment statement that this specific schema is disposable, rather + * than an inference drawn from configuration that happens to be present. + */ +export function integrationDbAuthorized(env: NodeJS.ProcessEnv = process.env): boolean { + return Boolean( + env.DB_HOST + && env.DB_DATABASE + && env.CTR_INTEGRATION_TEST_DB + && env.CTR_INTEGRATION_TEST_DB === env.DB_DATABASE, + ); +} + +/** + * `describe` for a block that writes to the database, downgraded to `describe.skip` + * when the opt-in above has not been given. Skipped blocks report as skipped, never as + * a silent pass. + */ +export const describeWithDb = integrationDbAuthorized() ? describe : describe.skip; + +/** Marks every row this run creates, so cleanup can never reach a row it did not make. */ +export const FIXTURE_TAG = 'b1itest'; + +let sequence = 0; + +/** A name unique to this process and call, prefixed so cleanup can find it again. */ +export function fixtureName(label: string): string { + sequence += 1; + return `${FIXTURE_TAG}-${label}-${process.pid}-${sequence}`; +} + +/** Fields of a fixture role that a test actually cares about. */ +export interface RoleFixture { + name: string; + income_cc: number; + income_xp: number; +} + +/** A fixture member, with the wallet the payout paths credit. */ +export interface MemberFixture { + id: number; + walletId: number; + username: string; +} + +/** Inserts a role and returns its id. */ +export async function createRole(knex: Knex, role: RoleFixture): Promise { + const [id] = await knex('role').insert(role); + return id; +} + +/** + * Inserts a member and its wallet. + * @param knex connection to write through + * @param overrides member columns to set, most usefully the two credit timestamps + */ +export async function createMember( + knex: Knex, + overrides: Record = {}, +): Promise { + const username = fixtureName('member'); + const [walletId] = await knex('wallet').insert({ balance: 1000 }); + const [id] = await knex('member').insert({ + username, + email: `${username}@example.invalid`, + password: 'not-a-real-hash', + wallet_id: walletId, + ...overrides, + }); + return { id, walletId, username }; +} + +/** Assigns a role to a member, optionally scoped to a place. */ +export async function assignRole( + knex: Knex, + memberId: number, + roleId: number, + placeId: number = null, +): Promise { + await knex('role_assignment').insert({ + member_id: memberId, + role_id: roleId, + place_id: placeId, + }); +} + +/** A timestamp `days` days before now, for setting a credit timestamp out of its window. */ +export function daysAgo(days: number): Date { + const when = new Date(); + when.setDate(when.getDate() - days); + return when; +} + +/** + * Deletes every row created by `createMember`/`createRole` in this schema. + * + * Keyed off {@link FIXTURE_TAG} rather than off ids collected during the run, so a spec + * that died part way through still cleans up after itself on the next run. + */ +export async function cleanUpFixtures(knex: Knex): Promise { + const members = await knex('member') + .select('id', 'wallet_id') + .where('username', 'like', `${FIXTURE_TAG}-%`); + const memberIds = members.map(member => member.id); + const walletIds = members.map(member => member.wallet_id); + if (memberIds.length) { + await knex('role_assignment').whereIn('member_id', memberIds).del(); + await knex('member').whereIn('id', memberIds).del(); + } + if (walletIds.length) { + await knex('transaction').whereIn('recipient_wallet_id', walletIds).del(); + await knex('wallet').whereIn('id', walletIds).del(); + } + await knex('role_assignment') + .whereIn('role_id', knex('role').select('id').where('name', 'like', `${FIXTURE_TAG}-%`)) + .del(); + await knex('role').where('name', 'like', `${FIXTURE_TAG}-%`).del(); +} diff --git a/api/src/cron/role-credit.integration.spec.ts b/api/src/cron/role-credit.integration.spec.ts new file mode 100644 index 00000000..0b28ce8c --- /dev/null +++ b/api/src/cron/role-credit.integration.spec.ts @@ -0,0 +1,342 @@ +import { Container } from 'typedi'; + +import { Db } from '../db/db.class'; +import { RoleAssignmentService } from '../services'; +import { + assignRole, + cleanUpFixtures, + createMember, + createRole, + daysAgo, + describeWithDb, + fixtureName, + MemberFixture, +} from '@spec/integration-db'; + +/** + * Weekly job pay, exercised end to end through the cron that actually runs it, against a + * real MySQL. + * + * Everything under test here is about a database's behaviour rather than a service's: + * which rows a predicate selects, whether two writes land in one transaction, and what + * two workers racing each other leave behind. A mocked query builder can assert that a + * predicate was *written*; only a database can say what it *selects*, and no mock can + * fail a transaction half way through and show what survived. + * + * Requires the explicit disposable-database opt-in described in `spec/integration-db.ts`, + * and `--runInBand`: these specs read and write the shared `member` table. + */ +describeWithDb('weekly role credit (real database)', () => { + const db = Container.get(Db); + const knex = db.knex; + /** The cron module is the production entry point, and takes no arguments. */ + const runRoleCreditCron: () => Promise = jest.requireActual('./role-credit'); + + /** A member who is due weekly pay: paid last week, and active within the last seven days. */ + async function createDueMember(): Promise { + return createMember(knex, { + last_weekly_role_credit: daysAgo(7), + last_daily_login_credit: new Date(), + xp: 0, + }); + } + + async function balanceOf(member: MemberFixture): Promise { + const wallet = await knex('wallet').where({ id: member.walletId }).first(); + return wallet.balance; + } + + async function memberRow(member: MemberFixture) { + return knex('member').where({ id: member.id }).first(); + } + + async function weeklyLedgerRows(member: MemberFixture) { + return knex('transaction') + .where({ recipient_wallet_id: member.walletId }) + .andWhere('reason', 'like', 'weekly-role-credit%'); + } + + /** True when the member's eligibility timestamp has been moved to today. */ + async function stampedToday(member: MemberFixture): Promise { + const [rows] = await knex.raw( + 'SELECT DATE(last_weekly_role_credit) = DATE(NOW()) AS paid_today FROM member WHERE id = ?', + [member.id], + ); + return Boolean(rows[0].paid_today); + } + + beforeEach(async () => { + await cleanUpFixtures(knex); + }); + + afterEach(async () => { + await cleanUpFixtures(knex); + }); + + afterAll(async () => { + await knex.destroy(); + }); + + describe('eligibility', () => { + it('does not pay, or stamp, a member whose only role pays nothing', async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('AdminOnly'), + income_cc: 0, + income_xp: 0, + })); + + await runRoleCreditCron(); + + expect(await balanceOf(member)).toBe(1000); + expect((await memberRow(member)).xp).toBe(0); + expect(await weeklyLedgerRows(member)).toHaveLength(0); + expect(await stampedToday(member)).toBe(false); + }); + + it('pays a role that grants CityCash but no XP', async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('CcOnly'), + income_cc: 40, + income_xp: 0, + })); + + await runRoleCreditCron(); + + expect(await balanceOf(member)).toBe(1040); + expect((await memberRow(member)).xp).toBe(0); + expect(await weeklyLedgerRows(member)).toHaveLength(1); + expect(await stampedToday(member)).toBe(true); + }); + + it('pays a role that grants XP but no CityCash', async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('XpOnly'), + income_cc: 0, + income_xp: 7, + })); + + await runRoleCreditCron(); + + expect(await balanceOf(member)).toBe(1000); + expect((await memberRow(member)).xp).toBe(7); + expect(await weeklyLedgerRows(member)).toHaveLength(1); + expect(await stampedToday(member)).toBe(true); + }); + + it('leaves a member who was already paid today alone', async () => { + const member = await createMember(knex, { + last_weekly_role_credit: new Date(), + last_daily_login_credit: new Date(), + xp: 0, + }); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: 50, + income_xp: 5, + })); + + await runRoleCreditCron(); + + expect(await balanceOf(member)).toBe(1000); + expect(await weeklyLedgerRows(member)).toHaveLength(0); + }); + }); + + describe('single-pay role selection', () => { + it('pays the earning role, not the one that pays nothing', async () => { + const member = await createDueMember(); + const worker = await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: 50, + income_xp: 5, + }); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('AdminOnly'), + income_cc: 0, + income_xp: 0, + })); + await assignRole(knex, member.id, worker); + + await runRoleCreditCron(); + + const ledger = await weeklyLedgerRows(member); + expect(ledger).toHaveLength(1); + expect(ledger[0].reason).toBe(`weekly-role-credit for ${worker}`); + expect(await balanceOf(member)).toBe(1050); + expect((await memberRow(member)).xp).toBe(5); + }); + + it('breaks a CityCash tie on XP, and still pays only once', async () => { + const member = await createDueMember(); + const lowXp = await createRole(knex, { + name: fixtureName('EqualCcLowXp'), + income_cc: 60, + income_xp: 1, + }); + const highXp = await createRole(knex, { + name: fixtureName('EqualCcHighXp'), + income_cc: 60, + income_xp: 9, + }); + await assignRole(knex, member.id, lowXp); + await assignRole(knex, member.id, highXp); + + await runRoleCreditCron(); + + const ledger = await weeklyLedgerRows(member); + expect(ledger).toHaveLength(1); + expect(ledger[0].reason).toBe(`weekly-role-credit for ${highXp}`); + expect(await balanceOf(member)).toBe(1060); + expect((await memberRow(member)).xp).toBe(9); + }); + + it('pays a place-scoped assignment exactly as it pays an unscoped one', async () => { + const unscoped = await createDueMember(); + const scoped = await createDueMember(); + const role = await createRole(knex, { + name: fixtureName('PlaceScoped'), + income_cc: 50, + income_xp: 5, + }); + await assignRole(knex, unscoped.id, role, null); + await assignRole(knex, scoped.id, role, 1); + + await runRoleCreditCron(); + + expect(await balanceOf(unscoped)).toBe(1050); + expect(await balanceOf(scoped)).toBe(1050); + expect((await memberRow(scoped)).xp).toBe(5); + }); + }); + + describe('batch limit', () => { + it('fills the batch with earning members rather than with non-earning ones', async () => { + const nothing = await createRole(knex, { + name: fixtureName('AdminOnly'), + income_cc: 0, + income_xp: 0, + }); + const worker = await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: 50, + income_xp: 5, + }); + const nonEarners: MemberFixture[] = []; + for (let index = 0; index < 3; index += 1) { + const member = await createDueMember(); + await assignRole(knex, member.id, nothing); + nonEarners.push(member); + } + const earners: MemberFixture[] = []; + for (let index = 0; index < 2; index += 1) { + const member = await createDueMember(); + await assignRole(knex, member.id, worker); + earners.push(member); + } + + const batch = await Container.get(RoleAssignmentService).getMembersDueRoleCredit(2); + const selected = batch.map(row => row.member_id); + + expect(selected).toHaveLength(2); + expect(selected.sort()).toEqual(earners.map(member => member.id).sort()); + for (const member of nonEarners) { + expect(selected).not.toContain(member.id); + } + }); + }); + + describe('concurrency', () => { + /** + * Slows every wallet update down, so that two payouts starting together are + * guaranteed to overlap rather than merely likely to. Without it the race is real + * but timing-dependent, and a green run would prove nothing. + */ + beforeEach(async () => { + await knex.raw('DROP TRIGGER IF EXISTS b1_slow_wallet_update'); + await knex.raw( + 'CREATE TRIGGER b1_slow_wallet_update BEFORE UPDATE ON wallet ' + + 'FOR EACH ROW SET @b1_slow = SLEEP(0.4)', + ); + }); + + afterEach(async () => { + await knex.raw('DROP TRIGGER IF EXISTS b1_slow_wallet_update'); + }); + + it('pays exactly once when two cron executions overlap', async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: 50, + income_xp: 5, + })); + + await Promise.all([runRoleCreditCron(), runRoleCreditCron()]); + + expect(await balanceOf(member)).toBe(1050); + expect((await memberRow(member)).xp).toBe(5); + expect(await weeklyLedgerRows(member)).toHaveLength(1); + }, 30000); + }); + + describe('rollback', () => { + /** + * Fails the member half of the payout at the database, leaving the wallet and ledger + * half untouched by the injection. If the two halves commit independently, the money + * moves and the member stays eligible to be paid again. + */ + async function failMemberUpdatesFor(memberId: number): Promise { + await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + await knex.raw( + `CREATE TRIGGER b1_block_member_update BEFORE UPDATE ON member FOR EACH ROW + BEGIN + IF NEW.id = ${memberId} THEN + SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'b1 injected member update failure'; + END IF; + END`, + ); + } + + afterEach(async () => { + await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + }); + + it('leaves no money moved when the member half of the payout fails', async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: 50, + income_xp: 5, + })); + await failMemberUpdatesFor(member.id); + + await expect(runRoleCreditCron()).rejects.toThrow(); + + expect(await balanceOf(member)).toBe(1000); + expect(await weeklyLedgerRows(member)).toHaveLength(0); + expect((await memberRow(member)).xp).toBe(0); + expect(await stampedToday(member)).toBe(false); + }); + + it('does not pay twice when the run is retried after that failure', async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: 50, + income_xp: 5, + })); + await failMemberUpdatesFor(member.id); + await expect(runRoleCreditCron()).rejects.toThrow(); + + await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + await runRoleCreditCron(); + + expect(await balanceOf(member)).toBe(1050); + expect(await weeklyLedgerRows(member)).toHaveLength(1); + expect((await memberRow(member)).xp).toBe(5); + }); + }); +}); diff --git a/api/src/knexfile.ts b/api/src/knexfile.ts index 9cebb342..6b39f5f6 100644 --- a/api/src/knexfile.ts +++ b/api/src/knexfile.ts @@ -28,6 +28,41 @@ const config: { [key: string]: Knex.Config } = { directory: './../db/seed', }, }, + /** + * Used when NODE_ENV=test, which jest sets for us. + * + * Without this key `config[process.env.NODE_ENV]` is undefined, and because `Db`'s + * constructor calls `knex(...)` at import time, every suite that transitively imports a + * repository dies while loading rather than running a single assertion. The + * database-backed specs need the key for a second reason: it is the connection they + * actually talk to. Point DB_DATABASE at a disposable schema before running those -- + * see `spec/integration-db.ts`, which refuses to write without an explicit opt-in. + */ + test: { + client: 'mysql', + connection: { + host: process.env.DB_HOST, + port: Number.parseInt(process.env.DB_PORT), + user: process.env.DB_USER, + password: process.env.DB_PASS, + database: process.env.DB_DATABASE, + charset: 'utf8mb4', + }, + // min 0, unlike the other environments: a minimum of 2 opens connections nothing asks + // for, and jest then hangs at the end of a run waiting on handles nothing will close. + pool: { + min: 0, + max: 5, + }, + migrations: { + directory: '../db/migrations', + extension: 'ts', + tableName: 'migrations', + }, + seeds: { + directory: './../db/seed', + }, + }, production: { client: 'mysql', connection: { diff --git a/api/src/services/member/daily-credit.integration.spec.ts b/api/src/services/member/daily-credit.integration.spec.ts new file mode 100644 index 00000000..b6fa8c7a --- /dev/null +++ b/api/src/services/member/daily-credit.integration.spec.ts @@ -0,0 +1,252 @@ +import bcrypt from 'bcrypt'; +import { Container } from 'typedi'; + +import { Db } from '../../db/db.class'; +import { MemberService } from './member.service'; +import { + assignRole, + cleanUpFixtures, + createMember, + createRole, + daysAgo, + describeWithDb, + fixtureName, + MemberFixture, +} from '@spec/integration-db'; + +/** + * The daily login credit, against a real MySQL. + * + * The interesting questions are all about what the database ends up holding: whether two + * logins arriving together pay once or twice, whether a failure part way through leaves + * money moved, and whether a credit landing at the same moment as the weekly payroll can + * overwrite it. None of those can be answered by a mocked query builder. + * + * Requires the explicit disposable-database opt-in described in `spec/integration-db.ts`, + * and `--runInBand`: these specs read and write the shared `member` table. + */ +describeWithDb('daily login credit (real database)', () => { + const knex = Container.get(Db).knex; + const service = Container.get(MemberService); + + /** The amounts are economy policy and deliberately hard-coded: B1 must not move them. */ + const UNEMPLOYED_CC = 50; + const UNEMPLOYED_XP = 5; + const EMPLOYED_CC = 100; + const EMPLOYED_XP = 10; + + /** A member who has not been credited today, and so is due the bonus. */ + async function createDueMember(overrides: Record = {}) { + return createMember(knex, { + last_daily_login_credit: daysAgo(1), + last_weekly_role_credit: new Date(), + xp: 0, + ...overrides, + }); + } + + async function balanceOf(member: MemberFixture): Promise { + const wallet = await knex('wallet').where({ id: member.walletId }).first(); + return wallet.balance; + } + + async function memberRow(member: MemberFixture) { + return knex('member').where({ id: member.id }).first(); + } + + async function dailyLedgerRows(member: MemberFixture) { + return knex('transaction') + .where({ recipient_wallet_id: member.walletId, reason: 'daily-credit' }); + } + + /** True when the member's eligibility timestamp has been moved to today. */ + async function stampedToday(member: MemberFixture): Promise { + const [rows] = await knex.raw( + 'SELECT DATE(last_daily_login_credit) = DATE(NOW()) AS credited_today ' + + 'FROM member WHERE id = ?', + [member.id], + ); + return Boolean(rows[0].credited_today); + } + + beforeEach(async () => { + await cleanUpFixtures(knex); + }); + + afterEach(async () => { + await cleanUpFixtures(knex); + }); + + afterAll(async () => { + await knex.destroy(); + }); + + describe('amounts', () => { + it('credits an unemployed member the standard amount, once', async () => { + const member = await createDueMember(); + + await service.maybeGiveDailyCredits(member.id); + + expect(await balanceOf(member)).toBe(1000 + UNEMPLOYED_CC); + expect((await memberRow(member)).xp).toBe(UNEMPLOYED_XP); + expect(await dailyLedgerRows(member)).toHaveLength(1); + expect(await stampedToday(member)).toBe(true); + }); + + it('credits the employed amount to anyone holding a role, including one that pays nothing', + async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('AdminOnly'), + income_cc: 0, + income_xp: 0, + })); + + await service.maybeGiveDailyCredits(member.id); + + expect(await balanceOf(member)).toBe(1000 + EMPLOYED_CC); + expect((await memberRow(member)).xp).toBe(EMPLOYED_XP); + }); + + it('is a no-op for a member already credited today', async () => { + const member = await createDueMember({ last_daily_login_credit: new Date() }); + + await service.maybeGiveDailyCredits(member.id); + + expect(await balanceOf(member)).toBe(1000); + expect(await dailyLedgerRows(member)).toHaveLength(0); + }); + }); + + describe('login', () => { + const password = 'b1-correct-horse'; + + async function createLoginMember(): Promise { + return createDueMember({ password: await bcrypt.hash(password, 10) }); + } + + it('has already applied the credit by the time it returns a token', async () => { + const member = await createLoginMember(); + + const token = await service.login(member.username, password); + + expect(typeof token).toBe('string'); + expect(await balanceOf(member)).toBe(1000 + UNEMPLOYED_CC); + }); + + describe('when the credit cannot be applied', () => { + afterEach(async () => { + await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + }); + + async function failMemberUpdatesFor(memberId: number): Promise { + await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + await knex.raw( + `CREATE TRIGGER b1_block_member_update BEFORE UPDATE ON member FOR EACH ROW + BEGIN + IF NEW.id = ${memberId} THEN + SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'b1 injected member update failure'; + END IF; + END`, + ); + } + + it('still logs the member in', async () => { + const member = await createLoginMember(); + await failMemberUpdatesFor(member.id); + + const token = await service.login(member.username, password); + + expect(typeof token).toBe('string'); + expect(token.length).toBeGreaterThan(0); + }); + + it('leaves no half-applied credit behind', async () => { + const member = await createLoginMember(); + await failMemberUpdatesFor(member.id); + + await service.login(member.username, password); + // Long enough for a credit that was never awaited to land anyway. + await new Promise(resolve => setTimeout(resolve, 500)); + + expect(await balanceOf(member)).toBe(1000); + expect(await dailyLedgerRows(member)).toHaveLength(0); + expect(await stampedToday(member)).toBe(false); + }); + }); + }); + + describe('concurrency', () => { + /** See the equivalent note in `role-credit.integration.spec.ts`. */ + beforeEach(async () => { + await knex.raw('DROP TRIGGER IF EXISTS b1_slow_wallet_update'); + await knex.raw( + 'CREATE TRIGGER b1_slow_wallet_update BEFORE UPDATE ON wallet ' + + 'FOR EACH ROW SET @b1_slow = SLEEP(0.4)', + ); + }); + + afterEach(async () => { + await knex.raw('DROP TRIGGER IF EXISTS b1_slow_wallet_update'); + }); + + it('credits once when two logins arrive together', async () => { + const member = await createDueMember(); + + await Promise.all([ + service.maybeGiveDailyCredits(member.id), + service.maybeGiveDailyCredits(member.id), + ]); + + expect(await balanceOf(member)).toBe(1000 + UNEMPLOYED_CC); + expect((await memberRow(member)).xp).toBe(UNEMPLOYED_XP); + expect(await dailyLedgerRows(member)).toHaveLength(1); + }, 30000); + + it('does not lose a wallet update when the weekly payroll credits the same wallet', + async () => { + const runRoleCreditCron: () => Promise = jest.requireActual('../../cron/role-credit'); + const weeklyCc = 50; + const weeklyXp = 5; + const member = await createDueMember({ last_weekly_role_credit: daysAgo(7) }); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: weeklyCc, + income_xp: weeklyXp, + })); + + await Promise.all([ + service.maybeGiveDailyCredits(member.id), + runRoleCreditCron(), + ]); + + expect(await balanceOf(member)).toBe(1000 + EMPLOYED_CC + weeklyCc); + expect((await memberRow(member)).xp).toBe(EMPLOYED_XP + weeklyXp); + }, 30000); + }); + + describe('rollback', () => { + afterEach(async () => { + await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + }); + + it('moves no money when the member half of the credit fails', async () => { + const member = await createDueMember(); + await knex.raw( + `CREATE TRIGGER b1_block_member_update BEFORE UPDATE ON member FOR EACH ROW + BEGIN + IF NEW.id = ${member.id} THEN + SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'b1 injected member update failure'; + END IF; + END`, + ); + + await expect(service.maybeGiveDailyCredits(member.id)).rejects.toThrow(); + + expect(await balanceOf(member)).toBe(1000); + expect(await dailyLedgerRows(member)).toHaveLength(0); + expect((await memberRow(member)).xp).toBe(0); + expect(await stampedToday(member)).toBe(false); + }); + }); +}); From 9fefaa7c954a2ed1a4c52446fb7d22586ea2ff1c Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Tue, 25 Aug 2026 12:02:11 -0400 Subject: [PATCH 2/6] fix: make the daily login credit atomic and awaited The daily bonus moved four things - wallet balance, ledger row, member XP, and the timestamp that decides whether the bonus may be given again - and committed them in two independent transactions. Failing the second left the money moved with the member still eligible, so the next login paid again. Eligibility was also read before either transaction opened, so two logins arriving together both saw "not credited today" and both paid; and the wallet was credited by reading the balance and writing back read + amount, which loses a concurrent credit to the same wallet. CreditRepository now owns the payout as one transaction: lock the member row with SELECT ... FOR UPDATE, recheck eligibility against the locked row, then write wallet, ledger, XP and timestamp together. A locking read returns the latest committed row rather than the transaction's snapshot, so the second of two concurrent callers sees the first one's timestamp and does nothing. The wallet moves by `balance = balance + ?`, so the database computes the new total from whatever is committed at that moment. `login` now awaits the credit rather than leaving it in flight behind the returned token, and catches its failure: refusing someone their account over a missed bonus is the worse outcome, and since the payout is all-or-nothing there is no partial credit left to clean up. `createMemberAndLogin` and the session refresh - which could previously 500 a member out of an otherwise valid session - go through the same helper. Amounts are untouched, and so is the rule that any role assignment counts as employed for this bonus, including one that pays nothing. Only weekly payroll cares whether a role earns. TransactionRepository.createDailyCreditTransaction is removed rather than left dead: it is the read-then-write wallet update this change exists to stop. Also brings the four touched files to zero lint errors and warnings. The one exception is MemberService.getAccessLevel, still `Promise`: typing it as the `string[]` it returns makes tsc reject `accessLevel === 'admin'` in two controllers, which is always false today. That is an authority bug, out of scope here, and reported rather than fixed. --- api/src/controllers/member.controller.ts | 13 +- .../repositories/credit/credit.repository.ts | 142 ++++++++++++++++++ api/src/repositories/index.ts | 1 + .../transaction/transaction.repository.ts | 71 +++++---- .../services/member/member.service.spec.ts | 36 +++-- api/src/services/member/member.service.ts | 120 +++++++++++---- 6 files changed, 301 insertions(+), 82 deletions(-) create mode 100644 api/src/repositories/credit/credit.repository.ts diff --git a/api/src/controllers/member.controller.ts b/api/src/controllers/member.controller.ts index 29dd0e8c..9839ecdc 100644 --- a/api/src/controllers/member.controller.ts +++ b/api/src/controllers/member.controller.ts @@ -7,7 +7,6 @@ import * as badwords from 'badwords-list'; import { sendPasswordResetEmail, sendPasswordResetUnknownEmail } from '../libs'; import { MemberService, HomeService, PlaceService } from '../services'; -import { SessionInfo } from 'session-info.interface'; import {parseInt} from 'lodash'; class MemberController { @@ -98,7 +97,7 @@ class MemberController { } } - public async check3d(request: Request, response: Response): Promise { + public async check3d(request: Request, response: Response): Promise { const session = this.memberService.decryptSession(request, response); if(!session) return; try { @@ -110,7 +109,7 @@ class MemberController { } } - public async getActivePlaces(request: Request, response: Response): Promise { + public async getActivePlaces(request: Request, response: Response): Promise { const session = this.memberService.decryptSession(request, response); if(!session) return; try { @@ -313,7 +312,7 @@ class MemberController { const token = await this.memberService.getMemberToken(session.id); const { banned, banInfo } = await this.memberService.isBanned(session.id); if (!banned) { - await this.memberService.maybeGiveDailyCredits(session.id); + await this.memberService.giveDailyCreditsForLogin(session.id); const homeInfo = await this.homeService.getHome(session.id); const chatdefault = await this.memberService.getMemberChat(session.id); session.hasHome = !!homeInfo; @@ -462,7 +461,7 @@ class MemberController { } } - public async getOnlineUsers(request: Request, response: Response): Promise { + public async getOnlineUsers(request: Request, response: Response): Promise { const session = this.memberService.decryptSession(request, response); if (!session) return; try { @@ -491,7 +490,7 @@ class MemberController { } } - public async getStorage(request: Request, response: Response): Promise { + public async getStorage(request: Request, response: Response): Promise { const session = this.memberService.decryptSession(request, response); if (!session) return; const member_id = parseInt(request.body.member_id); @@ -504,7 +503,7 @@ class MemberController { } } - public async updateStorage(request: Request, response: Response): Promise { + public async updateStorage(request: Request, response: Response): Promise { const session = this.memberService.decryptSession(request, response); if(!session) return; try { diff --git a/api/src/repositories/credit/credit.repository.ts b/api/src/repositories/credit/credit.repository.ts new file mode 100644 index 00000000..fb258723 --- /dev/null +++ b/api/src/repositories/credit/credit.repository.ts @@ -0,0 +1,142 @@ +import { Knex } from 'knex'; +import { Service } from 'typedi'; + +import { Db } from '../../db/db.class'; +import { Member, Transaction, TransactionReason, Wallet } from '../../types/models'; + +/** CityCash and experience granted by a single credit. */ +export interface CreditAmount { + /** CityCash added to the member's wallet. */ + cc: number; + /** Experience points added to the member. */ + xp: number; +} + +/** What a payout attempt did. */ +export interface CreditOutcome { + /** + * `true` when this call is the one that paid. `false` means the member was not + * eligible when the row was actually held - already credited, or holding no earning + * role - and nothing was written. + */ + credited: boolean; + /** Amounts paid. Only present when `credited`. */ + amount?: CreditAmount; + /** Role the weekly payment was made for. Only present on a weekly credit. */ + roleId?: number; +} + +/** + * Whether a member has already had their daily login bonus since the beginning + * (00:00:00) of the current day. + * + * Lives here rather than on MemberService because the check that decides whether money + * moves has to run inside the transaction that moves it. MemberService keeps a method + * of the same name, delegating here, so both answer identically. + */ +export function hasReceivedDailyCreditToday(lastCredit: Date, now: Date = new Date()): boolean { + return lastCredit.getTime() >= new Date(now).setHours(0, 0, 0, 0); +} + +/** + * Repository owning the two job-credit payouts, each as a single database transaction. + * + * Both payouts move four things that have to agree: a wallet balance, a ledger row, the + * member's XP, and the eligibility timestamp that decides whether the payout may happen + * again. Splitting them across transactions is what let a failed second half leave money + * moved with the member still eligible, and reading eligibility outside the transaction + * that pays is what let two callers both decide to pay. + * + * So every method here follows the same shape: + * + * 1. lock the member row with SELECT ... FOR UPDATE; + * 2. re-evaluate eligibility against the locked row, not against anything a caller + * passed in or a batch query read earlier; + * 3. write wallet, ledger, XP and timestamp; + * 4. commit. + * + * A second caller arriving concurrently blocks at step 1, and a locking read returns the + * latest committed row rather than the transaction's snapshot, so it sees the first + * caller's timestamp at step 2 and becomes a no-op. Wallet balances are moved with + * `balance = balance + ?` rather than a read followed by a write of the total, so a + * credit landing on a wallet another transaction is also crediting cannot overwrite it. + * + * The member row is always the first row locked, so these paths cannot deadlock against + * each other. + */ +@Service() +export class CreditRepository { + constructor(private db: Db) {} + + /** + * Gives the member their daily login bonus, unless they have already had it today. + * @param memberId id of the member to credit + * @param amounts amounts to award, by whether the member holds any role + * @returns what was paid, or `credited: false` if the bonus had already been given + */ + public async giveDailyCredit( + memberId: number, + amounts: { unemployed: CreditAmount; employed: CreditAmount }, + ): Promise { + return this.db.knex.transaction(async trx => { + const member = await this.lockMember(trx, memberId); + if (!member) return { credited: false }; + if (hasReceivedDailyCreditToday(member.last_daily_login_credit)) { + return { credited: false }; + } + + // Holding any role at all is what counts as employed for the daily bonus, + // including a role that pays nothing. That is existing policy, unchanged here - + // the earning-role filter belongs to weekly payroll, not to this. + const [{ roles }] = await trx('role_assignment') + .where('member_id', memberId) + .count({ roles: '*' }); + const amount = Number(roles) > 0 ? amounts.employed : amounts.unemployed; + + await this.pay(trx, member.wallet_id, amount.cc, TransactionReason.DailyCredit); + await trx('member') + .where({ id: memberId }) + .update({ + xp: trx.raw('xp + ?', [amount.xp]), + last_daily_login_credit: new Date(), + }); + + return { credited: true, amount }; + }); + } + + /** + * Locks a member row for the life of the transaction. + * + * A locking read, so it returns the latest committed row rather than the snapshot the + * transaction started with - which is what makes the eligibility recheck that follows + * it see a concurrent payout that has already committed. + */ + private async lockMember(trx: Knex.Transaction, memberId: number): Promise { + return trx('member').where({ id: memberId }).forUpdate().first(); + } + + /** + * Moves CityCash into a wallet and records it in the ledger. + * + * `increment` emits `balance = balance + ?`, so the new balance is computed by the + * database from whatever is committed at that moment. Reading the balance and writing + * back `read + amount` instead is what let one of two concurrent credits vanish. + * + * The ledger row is written even when the amount is zero: an XP-only role still earns + * a weekly credit, and the row is the only record that it happened. + */ + private async pay( + trx: Knex.Transaction, + walletId: number, + amount: number, + reason: string, + ): Promise { + await trx('wallet').where({ id: walletId }).increment('balance', amount); + await trx('transaction').insert({ + amount, + reason, + recipient_wallet_id: walletId, + }); + } +} diff --git a/api/src/repositories/index.ts b/api/src/repositories/index.ts index 7936f133..f7799bc7 100644 --- a/api/src/repositories/index.ts +++ b/api/src/repositories/index.ts @@ -2,6 +2,7 @@ export * from './avatar/avatar.repository'; export * from './ban/ban.repository'; export * from './block/block.repository'; export * from './club-member/club-member.repository'; +export * from './credit/credit.repository'; export * from './colony/colony.repository'; export * from './home/home.repository'; export * from './home-design/home-design.repository'; diff --git a/api/src/repositories/transaction/transaction.repository.ts b/api/src/repositories/transaction/transaction.repository.ts index ad718a7d..9de38a36 100644 --- a/api/src/repositories/transaction/transaction.repository.ts +++ b/api/src/repositories/transaction/transaction.repository.ts @@ -3,36 +3,26 @@ import { Service } from 'typedi'; import { Db } from '../../db/db.class'; import { Transaction, TransactionReason, Wallet } from '../../types/models'; +/** A `count(id)` result, as knex returns it: a single row holding the total. */ +export interface TransactionCount { + count: number; +} + +/** + * A transaction as the admin listings hand it back: the stored columns, plus the two + * username fields `AdminService` resolves onto each row from the wallet ids once it has + * them. Optional because the repository never sets them itself. + */ +export interface TransactionListRow extends Transaction { + recipient_username?: { username: string }[]; + sender_username?: { username: string }[]; +} + /** Repository for creating/interacting with transaction/wallet data in the database. */ @Service() export class TransactionRepository { constructor(private db: Db) {} - /** - * Applies the given amount to the balance for the wallet with the given id, and creates - * a transaction record. - * @param walletId id of recipient wallet - * @param amount amount transacted - * @returns promise resolving in the created transaction object, or rejecting on error - */ - public async createDailyCreditTransaction( - walletId: number, - amount: number, - ): Promise { - return await this.db.knex.transaction(async trx => { - const wallet = await trx('wallet').where({ id: walletId }).first(); - await trx('wallet') - .where({ id: walletId }) - .update({ balance: wallet.balance + amount }); - const [transactionId] = await trx('transaction').insert({ - amount, - reason: TransactionReason.DailyCredit, - recipient_wallet_id: walletId, - }); - return this.find({ id: transactionId }); - }); - } - /** * Finds a transaction with the given search parameters if one exists. * @param transactionSearchParams object containing properties of a transaction for searching on @@ -257,7 +247,11 @@ export class TransactionRepository { }); } - public async getTransactions(type: string, limit: number, offset: number): Promise { + public async getTransactions( + type: string, + limit: number, + offset: number, + ): Promise { return this.db.knex .select( 'id', @@ -272,10 +266,13 @@ export class TransactionRepository { .limit(limit) .offset(offset) .orderBy('id', 'DESC'); - ; } - public async getTransactionsByWalletId(id: number, limit: number, offset: number): Promise { + public async getTransactionsByWalletId( + id: number, + limit: number, + offset: number, + ): Promise { return this.db.knex .select( 'id', @@ -291,35 +288,37 @@ export class TransactionRepository { .limit(limit) .offset(offset) .orderBy('id', 'DESC'); - ; } - public async getLatestTransactions(time: Date): Promise { + public async getLatestTransactions(time: Date): Promise { return this.db.knex .select('transaction.*') .from('transaction') .where('created_at', '>=', time) .limit(30) .orderBy('transaction.id', 'DESC'); - ; } - public async getTotal( type: string): Promise { - return this.db.knex + public async getTotal( type: string): Promise { + // knex types an untyped `count` as a dictionary of unnamed columns; the alias makes + // the shape known here in a way the builder's own types cannot express. + const rows = await this.db.knex .count('id as count') .from('transaction') .where('reason', type); + return rows; } - public async getWalletTotal( id: number): Promise { - return this.db.knex + public async getWalletTotal( id: number): Promise { + const rows = await this.db.knex .count('id as count') .from('transaction') .where('recipient_wallet_id', id) .orWhere('sender_wallet_id', id); + return rows; } - public async removeAllByWalletId(id: number): Promise { + public async removeAllByWalletId(id: number): Promise { await this.db.knex('transaction') .where('recipient_wallet_id', id) .orWhere('sender_wallet_id', id) diff --git a/api/src/services/member/member.service.spec.ts b/api/src/services/member/member.service.spec.ts index bb151a0b..bbf91abf 100644 --- a/api/src/services/member/member.service.spec.ts +++ b/api/src/services/member/member.service.spec.ts @@ -9,6 +9,7 @@ import { } from 'models'; import { AvatarRepository, + CreditRepository, MemberRepository, TransactionRepository, WalletRepository, @@ -26,6 +27,7 @@ describe('MemberService', () => { email: 'foo@foo.com', }; let avatarRepository: jest.Mocked; + let creditRepository: jest.Mocked; let memberRepository: jest.Mocked; let transactionRepository: jest.Mocked; let walletRepository: jest.Mocked; @@ -34,6 +36,8 @@ describe('MemberService', () => { beforeEach(() => { avatarRepository = createSpyObj(AvatarRepository); avatarRepository.find.mockResolvedValue(fakeAvatar as Avatar); + creditRepository = createSpyObj(CreditRepository); + creditRepository.giveDailyCredit.mockResolvedValue({ credited: true }); memberRepository = createSpyObj(MemberRepository); memberRepository.create.mockResolvedValue(fakeMember.id); memberRepository.find.mockResolvedValue(fakeMember as Member); @@ -42,6 +46,7 @@ describe('MemberService', () => { walletRepository = createSpyObj(WalletRepository); Container.reset(); Container.set(AvatarRepository, avatarRepository); + Container.set(CreditRepository, creditRepository); Container.set(MemberRepository, memberRepository); Container.set(TransactionRepository, transactionRepository); Container.set(WalletRepository, walletRepository); @@ -140,17 +145,28 @@ describe('MemberService', () => { beforeEach(async () => { await service.login(fakeMember.username, fakeMember.password); }); - it('gives daily xp to the member', () => { - expect(memberRepository.update).toHaveBeenCalledWith( - fakeMember.id, - expect.objectContaining({ xp: expect.any(Number) }), - ); + it('gives the member their daily citycash and xp', () => { + expect(creditRepository.giveDailyCredit).toHaveBeenCalledWith(fakeMember.id, { + unemployed: { + cc: MemberService.DAILY_CC_AMOUNT, + xp: MemberService.DAILY_XP_AMOUNT, + }, + employed: { + cc: MemberService.DAILY_CC_EMPLOYED_AMOUNT, + xp: MemberService.DAILY_XP_EMPLOYED_AMOUNT, + }, + }); }); - it('updates the timestamp of when the user last received login credit', () => { - expect(memberRepository.update).toHaveBeenCalledWith( - fakeMember.id, - expect.objectContaining({ last_daily_login_credit: expect.any(Date) }), - ); + }); + describe('when the daily credit cannot be given', () => { + // Refusing someone their account over a missed bonus would be the worse outcome, + // and the payout is all-or-nothing, so there is no partial credit to undo. + beforeEach(() => { + creditRepository.giveDailyCredit.mockRejectedValue(new Error('database is down')); + }); + it('still logs the member in', async () => { + const token = await service.login(fakeMember.username, fakeMember.password); + expect(token).toBe(await service.getMemberToken(fakeMember.id)); }); }); }); diff --git a/api/src/services/member/member.service.ts b/api/src/services/member/member.service.ts index 8f033d45..d3e13e51 100644 --- a/api/src/services/member/member.service.ts +++ b/api/src/services/member/member.service.ts @@ -7,6 +7,8 @@ import { Service } from 'typedi'; import { AvatarRepository, BanRepository, + CreditRepository, + hasReceivedDailyCreditToday, MapLocationRepository, MemberRepository, PlaceRepository, @@ -17,11 +19,53 @@ import { ObjectInstanceRepository, VoteRepository, } from '../../repositories'; -import { Member } from '../../types/models'; +import { Member, ObjectInstance, Place } from '../../types/models'; import { MemberInfoView, MemberAdminView } from '../../types/views'; import { SessionInfo } from 'session-info.interface'; import { Request, Response } from 'express'; +/** A role a member holds, as the member views list it. */ +interface MemberRoleSummary { + id: number; + place_id: number; + name: string; + place: string; +} + +/** Whether a member is banned, and the ban that says so. */ +interface BanStatus { + banned: boolean; + banInfo?: { end_date?: string; reason?: string }; +} + +/** A place with people in it right now, decorated with its name and headcount. */ +interface ActivePlaceSummary { + place_id: number; + name?: string; + slug?: string; + type?: number; + username?: string; + count?: number; +} + +/** + * A member who has been active recently, plus the two fields the member controller + * resolves onto each row after this returns. Optional because the service never sets + * them itself. + */ +interface OnlineMember { + id: number; + username: string; + hasHome?: boolean; + security?: boolean; +} + +/** A storage unit belonging to a member, with the number of objects it holds. */ +interface StorageUnitSummary { + id: number; + count?: number; +} + /** Service for dealing with members */ @Service() export class MemberService { @@ -42,6 +86,7 @@ export class MemberService { private avatarRepository: AvatarRepository, private banRepository: BanRepository, private memberRepository: MemberRepository, + private creditRepository: CreditRepository, private transactionRepository: TransactionRepository, private walletRepository: WalletRepository, private placeRepository: PlaceRepository, @@ -141,7 +186,7 @@ export class MemberService { username, password: hashedPassword, }); - await this.maybeGiveDailyCredits(memberId); + await this.giveDailyCreditsForLogin(memberId); return this.getMemberToken(memberId); } @@ -301,7 +346,7 @@ export class MemberService { return this.memberRepository.getPrimaryRoleName(memberId); } - public async getRoles(memberId: number): Promise { + public async getRoles(memberId: number): Promise { const roles = await this.roleAssignmentRepository.getRoleNameAndIdByMemberId(memberId); return roles; } @@ -313,8 +358,7 @@ export class MemberService { * @returns `true` if the member has received their daily login bonus today, `false` otherwise */ public hasReceivedLoginCreditToday(member: Member): boolean { - const today = new Date().setHours(0, 0, 0, 0); - return member.last_daily_login_credit.getTime() >= today; + return hasReceivedDailyCreditToday(member.last_daily_login_credit); } /** @@ -332,7 +376,7 @@ export class MemberService { * @param memberId * @return banned boolean true if banned */ - public async isBanned(memberId: number): Promise { + public async isBanned(memberId: number): Promise { let banned = false; const member = await this.memberRepository.findById(memberId); const banInfo = await this.banRepository.getBanMaxDate(memberId); @@ -360,33 +404,51 @@ export class MemberService { const validPassword = await bcrypt.compare(password, member.password); if (!validPassword) throw new Error('Incorrect login details.'); if (member.status === 0) throw new Error('banned'); - this.maybeGiveDailyCredits(member.id); + await this.giveDailyCreditsForLogin(member.id); return this.encodeMemberToken(member); } /** * Distributes daily credits (citycash, xp) to the member with the given id if they haven't * already received any today. + * + * Eligibility is rechecked inside the transaction that pays, so calling this twice at + * once credits once. Rejects if the credit could not be applied - in which case nothing + * was applied, not even partly. Callers that are logging someone in should treat that + * rejection as a lost bonus, not as a failed login. * @param memberId id of member to receive daily credits * @returns promise resolving when complete, rejecting on error */ public async maybeGiveDailyCredits(memberId: number): Promise { - const member = await this.memberRepository.findById(memberId); - if (!this.hasReceivedLoginCreditToday(member)) { - let ccIncrease = MemberService.DAILY_CC_AMOUNT; - let xpIncrease = MemberService.DAILY_XP_AMOUNT; - - const roles = await this.roleAssignmentRepository.getByMemberId(memberId); - if (roles.length > 0) { - ccIncrease = MemberService.DAILY_CC_EMPLOYED_AMOUNT; - xpIncrease = MemberService.DAILY_XP_EMPLOYED_AMOUNT; - } + await this.creditRepository.giveDailyCredit(memberId, { + unemployed: { + cc: MemberService.DAILY_CC_AMOUNT, + xp: MemberService.DAILY_XP_AMOUNT, + }, + employed: { + cc: MemberService.DAILY_CC_EMPLOYED_AMOUNT, + xp: MemberService.DAILY_XP_EMPLOYED_AMOUNT, + }, + }); + } - await this.transactionRepository.createDailyCreditTransaction(member.wallet_id, ccIncrease); - await this.memberRepository.update(memberId, { - last_daily_login_credit: new Date(), - xp: member.xp + xpIncrease, - }); + /** + * Gives daily credits as part of logging someone in, and swallows the failure if it + * cannot. + * + * Awaited, so the credit has landed before the caller gets a token: unawaited, the + * caller could read its own balance and not see it yet, and nothing kept the process + * interested in a write that outlived the response. Caught, because refusing someone + * their account over a missed bonus is the worse outcome of the two - and the payout is + * all-or-nothing, so a failure here leaves no half-applied credit to clean up. + * @param memberId id of member being logged in + * @returns promise resolving when the credit has been applied, or has failed + */ + public async giveDailyCreditsForLogin(memberId: number): Promise { + try { + await this.maybeGiveDailyCredits(memberId); + } catch (error) { + console.error(`Failed to give daily credits to member ${memberId}:`, error); } } @@ -514,7 +576,7 @@ export class MemberService { }); } - public async getActivePlaces(): Promise { + public async getActivePlaces(): Promise { const returnPlaces = []; const placeIds = []; const activeTime = new Date(Date.now() - 5 * 60000); @@ -575,13 +637,13 @@ export class MemberService { } } - public async getOnlineUsers(): Promise { + public async getOnlineUsers(): Promise { const activeTime = new Date(Date.now() - 5 * 60000); const users = await this.memberRepository.findOnlineUsers(activeTime); return users; } - public async getBackpack(username: string): Promise { + public async getBackpack(username: string): Promise { let memberId = null; let userId = null; try { @@ -595,7 +657,7 @@ export class MemberService { } } - public async getStorage(memberId: number): Promise { + public async getStorage(memberId: number): Promise { const units = []; const unit = await this.placeRepository.findStorageByUserID(memberId); for (const storage of unit) { @@ -606,17 +668,17 @@ export class MemberService { return units; } - public async getStorageById(placeId: number): Promise { + public async getStorageById(placeId: number): Promise { const unit = await this.placeRepository.findById(placeId); return unit; } - public async getMemberByWalletId(walletId: number): Promise { + public async getMemberByWalletId(walletId: number): Promise { const user = await this.memberRepository.findByWalletId(walletId); return user; } - public async removeAccount(id: number): Promise { + public async removeAccount(id: number): Promise { const user = await this.memberRepository.findById(id); await this.roleAssignmentRepository.removeAllByUserId(id); await this.banRepository.removeAllByUserId(id); From 2c65c31af2311b7e5f5fa200b967d14545d5f83e Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Tue, 25 Aug 2026 12:06:33 -0400 Subject: [PATCH 3/6] fix: pay weekly job credit once, atomically, and only to earning roles Weekly payroll had the same split-commit and stale-read shape as the daily bonus, plus an eligibility question it never asked. Eligibility. The inner join to role_assignment existed only to test "does this member hold a job", with no income filter, so any assignment qualified - including a role paying 0 CityCash and 0 XP. Admin is seeded exactly that way, so it is live today: such a member gets a zero-value weekly-role-credit ledger row, has last_weekly_role_credit stamped, and takes one of the batch's capped slots away from someone who actually earned pay. The predicate is now "pays CityCash OR pays XP", never CityCash alone - the two columns are independent, and filtering on CityCash would stop an XP-only role ever accruing its XP. It is applied to the batch query and to role selection alike, so a member cannot be selected on one role and paid for another. Payout. getMembersDueRoleCredit read each member's XP, wallet and paying role and handed them to the payout as arguments; by the time a worker used them another worker might have paid already. It now returns ids and nothing else, and CreditRepository re-reads everything under SELECT ... FOR UPDATE on the member row: eligibility rechecked with the same three conditions the batch selects on, paying role re-resolved, then wallet, ledger, XP and timestamp written in that one transaction. Two overlapping cron executions therefore pay once - the second blocks on the lock, sees the first one's timestamp, and does nothing - and a failure part way through leaves nothing behind for a retry to double. Single pay is unchanged: a citizen holding several jobs is paid for one of them, the highest-paying. Ties on CityCash now go to the role granting more XP rather than to whichever row came back first. place_id still plays no part in who gets paid, which is deliberate: that is A1's question, not this one. Salary and XP values are untouched. TransactionRepository.createWeeklyRoleCreditTransaction is removed for the same reason as its daily counterpart. --- api/src/cron/role-credit.integration.spec.ts | 3 +- api/src/cron/role-credit.ts | 11 +- .../repositories/credit/credit.repository.ts | 100 +++++++++++++ .../role-assignment.repository.ts | 137 ++++++++++-------- .../transaction/transaction.repository.ts | 19 +-- .../role-assignment.service.ts | 54 ++++--- 6 files changed, 206 insertions(+), 118 deletions(-) diff --git a/api/src/cron/role-credit.integration.spec.ts b/api/src/cron/role-credit.integration.spec.ts index 0b28ce8c..963ac6a1 100644 --- a/api/src/cron/role-credit.integration.spec.ts +++ b/api/src/cron/role-credit.integration.spec.ts @@ -237,8 +237,7 @@ describeWithDb('weekly role credit (real database)', () => { earners.push(member); } - const batch = await Container.get(RoleAssignmentService).getMembersDueRoleCredit(2); - const selected = batch.map(row => row.member_id); + const selected = await Container.get(RoleAssignmentService).getMembersDueRoleCredit(2); expect(selected).toHaveLength(2); expect(selected.sort()).toEqual(earners.map(member => member.id).sort()); diff --git a/api/src/cron/role-credit.ts b/api/src/cron/role-credit.ts index e31c055e..ca550603 100644 --- a/api/src/cron/role-credit.ts +++ b/api/src/cron/role-credit.ts @@ -6,14 +6,7 @@ module.exports = async () => { const roleAssignmentService = Container.get(RoleAssignmentService); const batch = await roleAssignmentService.getMembersDueRoleCredit(20); console.log(`CRON[role-credit]: ${ batch.length } to process...`); - for(const row of batch) { - await roleAssignmentService.giveWeeklyRoleCredit( - row.member_id, - row.xp, - row.wallet_id, - row.income_xp, - row.income_cc, - row.role_id, - ); + for(const memberId of batch) { + await roleAssignmentService.giveWeeklyRoleCredit(memberId); } }; diff --git a/api/src/repositories/credit/credit.repository.ts b/api/src/repositories/credit/credit.repository.ts index fb258723..2e6b9260 100644 --- a/api/src/repositories/credit/credit.repository.ts +++ b/api/src/repositories/credit/credit.repository.ts @@ -26,6 +26,27 @@ export interface CreditOutcome { roleId?: number; } +/** + * Restricts a role join to roles that actually earn: ones paying CityCash, XP, or both. + * + * A role paying neither is not a job. Payroll's inner join used to exist only to ask + * "does this member hold any role at all", so an Admin assignment - seeded at 0 CityCash + * and 0 XP - qualified its holder, took a slot in the capped batch, produced a + * zero-value ledger row, and got the eligibility timestamp stamped. + * + * The two income columns are independent, so the predicate is "pays CityCash OR pays XP", + * never CityCash alone: filtering on CityCash would stop an XP-only role ever accruing + * its XP. Applied to eligibility and to role selection alike, so a member cannot be + * selected on one role and paid for another. + * + * Shared with RoleAssignmentRepository, which asks the same question when it picks the + * batch. + * @param builder grouped-where builder supplied by knex + */ +export function wherePayingRole(builder: Knex.QueryBuilder): void { + builder.where('role.income_cc', '>', 0).orWhere('role.income_xp', '>', 0); +} + /** * Whether a member has already had their daily login bonus since the beginning * (00:00:00) of the current day. @@ -105,6 +126,85 @@ export class CreditRepository { }); } + /** + * Pays a member for one week of the highest-paying role they hold, unless they have + * already been paid today or hold no earning role. + * + * Takes only an id: the batch query that produced it read the member's XP, wallet and + * roles before any lock was held, and none of that is authoritative by the time the + * money moves. Everything the payout needs is re-read here, under the lock. + * @param memberId id of the member to pay + * @returns what was paid, or `credited: false` if the member was not due pay + */ + public async giveWeeklyRoleCredit(memberId: number): Promise { + return this.db.knex.transaction(async trx => { + const member = await this.lockMemberDueWeeklyCredit(trx, memberId); + if (!member || !member.due) return { credited: false }; + + // One role, not every role held: paying a citizen for a single job is the + // behaviour established when single pay landed, and is preserved here. + const role = await trx('role_assignment') + .select('role_assignment.role_id', 'role.income_cc', 'role.income_xp') + .innerJoin('role', 'role_assignment.role_id', 'role.id') + .where('role_assignment.member_id', memberId) + .where(wherePayingRole) + // Highest CityCash wins; among roles paying the same CityCash, the one granting + // more XP wins rather than whichever row the database happened to return first. + .orderBy('role.income_cc', 'desc') + .orderBy('role.income_xp', 'desc') + .first(); + if (!role) return { credited: false }; + + const amount: CreditAmount = { cc: role.income_cc, xp: role.income_xp }; + await this.pay( + trx, + member.wallet_id, + amount.cc, + `${TransactionReason.WeeklyCredit} for ${role.role_id}`, + ); + await trx('member') + .where({ id: memberId }) + .update({ + xp: trx.raw('xp + ?', [amount.xp]), + // Stamped from the database clock, because the predicate that reads it back is + // also evaluated there. A JS timestamp would open a double-pay window whenever + // the API and the database disagree about the date. + last_weekly_role_credit: trx.fn.now(), + }); + + return { credited: true, amount, roleId: role.role_id }; + }); + } + + /** + * Locks a member row and answers, in the same statement, whether they are due weekly + * pay. + * + * One statement rather than a lock followed by a separate check, so that the answer + * cannot come from a different point in time than the lock. Under REPEATABLE READ a + * plain SELECT reads the transaction's snapshot rather than the latest committed row, + * which is exactly the mistake this method exists to avoid making. + * + * The three conditions are deliberately the ones the batch query selects on, evaluated + * by the database so that "today" and "within the last seven days" mean the same thing + * in both places. + */ + private async lockMemberDueWeeklyCredit( + trx: Knex.Transaction, + memberId: number, + ): Promise<{ wallet_id: number; due: number }> { + return trx('member') + .select('wallet_id') + .select(trx.raw( + `status = 1 + AND DATE(last_weekly_role_credit) <> DATE(NOW()) + AND DATE(last_daily_login_credit) >= DATE(NOW() - INTERVAL 7 DAY) AS due`, + )) + .where({ id: memberId }) + .forUpdate() + .first(); + } + /** * Locks a member row for the life of the transaction. * diff --git a/api/src/repositories/role-assignment/role-assignment.repository.ts b/api/src/repositories/role-assignment/role-assignment.repository.ts index eac9abf0..62d6f632 100644 --- a/api/src/repositories/role-assignment/role-assignment.repository.ts +++ b/api/src/repositories/role-assignment/role-assignment.repository.ts @@ -1,15 +1,52 @@ import { Service } from 'typedi'; import { Db } from '../../db/db.class'; -import { knex } from 'knex'; import { RoleAssignment } from '../../types/models'; +import { wherePayingRole } from '../credit/credit.repository'; + +/** The donor role ids, and which of them the member should end up holding. */ +interface DonorRoleIds { + supporter: number; + advocate: number; + devotee: number; + champion: number; + donorLevel?: number; +} + +/** A member id, as the access-rights queries select it. */ +interface MemberIdRow { + member_id: number; +} + +/** A username, as the access-rights and roster queries select it. */ +interface UsernameRow { + username: string; +} + +/** A recent hire: who was given which role. */ +interface LatestAssignmentRow extends UsernameRow { + roleName: string; +} + +/** A role a member holds, with the place it is scoped to if any. */ +interface MemberRoleRow { + id: number; + place_id: number; + name: string; + place: string; +} + +/** A `count(id)` result, as knex returns it: a single row holding the total. */ +export interface AssignmentCount { + count: number; +} /** Repository for fetching/interacting with role assignment data in the database. */ @Service() export class RoleAssignmentRepository { constructor(private db: Db) {} - public async addDonor(member_id: number, roleId: any): Promise { + public async addDonor(member_id: number, roleId: DonorRoleIds): Promise { try{ await this.db.knex('role_assignment') .where('member_id', member_id) @@ -34,7 +71,7 @@ export class RoleAssignmentRepository { placeId: number, memberId: number, roleId: number, - ): Promise { + ): Promise { return this.db.knex('role_assignment') .insert( { @@ -48,15 +85,15 @@ export class RoleAssignmentRepository { public async getAccessInfoByID( placeId, ownerCode, - deputyCode): Promise<{ owner: any[]; deputies: any[] }> { - const owner: any[] = await this.db.knex + deputyCode): Promise<{ owner: MemberIdRow[]; deputies: MemberIdRow[] }> { + const owner: MemberIdRow[] = await this.db.knex .select( 'member_id', ) .from('role_assignment') .where('place_id', placeId) .where('role_id', ownerCode); - const deputies: any[] = await this.db.knex + const deputies: MemberIdRow[] = await this.db.knex .select( 'member_id', ) @@ -69,8 +106,8 @@ export class RoleAssignmentRepository { public async getAccessInfoByUsername( placeId, ownerCode, - deputyCode): Promise<{ owner: any[]; deputies: any[] }> { - const owner: any[] = await this.db.knex + deputyCode): Promise<{ owner: UsernameRow[]; deputies: UsernameRow[] }> { + const owner: UsernameRow[] = await this.db.knex .select( 'member.username', ) @@ -78,7 +115,7 @@ export class RoleAssignmentRepository { .where('role_assignment.place_id', placeId) .where('role_assignment.role_id', ownerCode) .innerJoin('member', 'role_assignment.member_id', 'member.id'); - const deputies: any[] = await this.db.knex + const deputies: UsernameRow[] = await this.db.knex .select( 'member.username', ) @@ -94,26 +131,26 @@ export class RoleAssignmentRepository { return roleResults; } - public async removeRoleAssignment(id: number): Promise { + public async removeRoleAssignment(id: number): Promise { await this.db.knex('role_assignment') .where('place_id', id) .del(); } - public async removeAllByUserId(id: number): Promise { + public async removeAllByUserId(id: number): Promise { await this.db.knex('role_assignment') .where('member_id', id) .del(); } - public async getUsernamesByRoleId(roleId: number): Promise { + public async getUsernamesByRoleId(roleId: number): Promise { return this.db.knex('role_assignment') .select('member.username') .where('role_assignment.role_id', '=', roleId) .leftJoin('member', 'role_assignment.member_id', 'member.id'); } - public async getLatest(): Promise { + public async getLatest(): Promise { return this.db.knex('role_assignment') .select('member.username', 'role.name as roleName') .leftJoin('member', 'role_assignment.member_id', 'member.id') @@ -122,7 +159,7 @@ export class RoleAssignmentRepository { .orderBy('role_assignment.id', 'desc'); } - public async getDonor(memberId: number, roleId: any): Promise { + public async getDonor(memberId: number, roleId: DonorRoleIds): Promise { return this.db.knex .select('role.name') .from('role_assignment') @@ -138,7 +175,7 @@ export class RoleAssignmentRepository { .first(); } - public async getRoleNameAndIdByMemberId(memberId: number): Promise { + public async getRoleNameAndIdByMemberId(memberId: number): Promise { return this.db.knex .distinct( 'role_assignment.role_id as id', @@ -153,61 +190,38 @@ export class RoleAssignmentRepository { } /** - * query finds all users with job who meet pay requirements - * the inner join is just there to check for holding a job - * then the for function will gather the highest paying role information per user - * the for function also packages all the information for the return - * @param limit - * @returns list of users with jobs that earned pay + * Finds members who are due weekly job pay. + * + * Returns ids and nothing else. Everything the payout needs - which role pays, what it + * pays, the member's XP and wallet - is re-read by CreditRepository inside the + * transaction that moves the money, because anything read here is already stale by the + * time a worker gets to it, and a second worker may have paid in between. + * + * The join to `role` is what makes the batch worth its size: without the earning-role + * filter, a member whose only role pays nothing qualifies, takes one of `limit` slots, + * and displaces someone who actually earned pay. + * @param limit maximum number of members to return + * @returns ids of members eligible for weekly pay, at most `limit` of them */ - public async getMembersDueRoleCredit(limit: number): Promise { - const query = await this.db.knex - .select( - 'member.id', - 'member.wallet_id', - 'member.xp', - ) + public async getMembersDueRoleCredit(limit: number): Promise { + const rows = await this.db.knex + .distinct('member.id') .from('member') .innerJoin('role_assignment', 'member.id', 'role_assignment.member_id') + .innerJoin('role', 'role_assignment.role_id', 'role.id') .where('member.status', 1) + .where(wherePayingRole) .whereRaw('DATE(member.last_weekly_role_credit) != DATE(NOW())') .whereRaw('DATE(member.last_daily_login_credit) >= DATE(NOW() - INTERVAL 7 DAY)') - .limit(limit) - .distinct('member.id'); - - const results = []; - for (const index in query) { - const member_info = query[index]; - const role_info = await this.db.knex - .select( - 'role_assignment.role_id', - 'role.income_cc', - 'role.income_xp', - ) - .from('role_assignment') - .innerJoin('role', 'role_assignment.role_id', 'role.id') - .where('role_assignment.member_id', member_info.id) - .orderBy('role.income_cc','desc') - .first(); - if (role_info) { - results[index] = { - member_id: member_info.id, - role_id: role_info.role_id, - wallet_id: member_info.wallet_id, - xp: member_info.xp, - income_cc: role_info.income_cc, - income_xp: role_info.income_xp, - }; - } - } - return results; + .limit(limit); + return rows.map(row => row.id); } public async removeIdFromAssignment( placeId: number, memberId: number, roleId: number, - ): Promise { + ): Promise { return await this.db.knex('role_assignment') .where('place_id', placeId) .where('member_id', memberId) @@ -215,9 +229,12 @@ export class RoleAssignmentRepository { .del(); } - public async countByAssigned(id: number): Promise { - return this.db.knex('role_assignment') + public async countByAssigned(id: number): Promise { + // knex types an untyped `count` as a dictionary of unnamed columns; the alias makes + // the shape known here in a way the builder's own types cannot express. + const rows = await this.db.knex('role_assignment') .count('id as count') .where('role_id', id); + return rows; } } diff --git a/api/src/repositories/transaction/transaction.repository.ts b/api/src/repositories/transaction/transaction.repository.ts index 9de38a36..b85fe328 100644 --- a/api/src/repositories/transaction/transaction.repository.ts +++ b/api/src/repositories/transaction/transaction.repository.ts @@ -79,24 +79,7 @@ export class TransactionRepository { return this.find({ id: transactionId }); }); } - public async createWeeklyRoleCreditTransaction( - walletId: number, - amount: number, - roleId: number, - ): Promise { - return await this.db.knex.transaction(async trx => { - const wallet = await trx('wallet').where({ id: walletId }).first(); - await trx('wallet') - .where({ id: walletId }) - .update({ balance: wallet.balance + amount }); - const [transactionId] = await trx('transaction').insert({ - amount, - reason: `${TransactionReason.WeeklyCredit} for ${roleId}`, - recipient_wallet_id: walletId, - }); - return this.find({ id: transactionId }); - }); - } + public async createSystemCreditTransaction( walletId: number, amount: number, diff --git a/api/src/services/role-assignment/role-assignment.service.ts b/api/src/services/role-assignment/role-assignment.service.ts index 954e62e6..d7e03f5d 100644 --- a/api/src/services/role-assignment/role-assignment.service.ts +++ b/api/src/services/role-assignment/role-assignment.service.ts @@ -2,9 +2,9 @@ import { Service } from 'typedi'; import { RoleAssignment } from '../../types/models'; import { + AssignmentCount, + CreditRepository, RoleAssignmentRepository, - MemberRepository, - TransactionRepository, } from '../../repositories'; /** Service for interacting with roles */ @@ -12,8 +12,7 @@ import { export class RoleAssignmentService { constructor( private roleAssignmentRepository: RoleAssignmentRepository, - private memberRepository: MemberRepository, - private transactionRepository: TransactionRepository, + private creditRepository: CreditRepository, ) {} public async getMembersRoles(memberId: number): Promise { @@ -22,36 +21,33 @@ export class RoleAssignmentService { } /** - * Grabs all payments due to users from database 50 at a time and - * places them in response array sorts respone into highest cc payout - * then drops all other payouts to the same user + * Finds up to `limit` members who are due weekly job pay. + * @param limit maximum number of members to return + * @returns ids of members eligible for weekly pay */ - public async getMembersDueRoleCredit(limit: number): Promise { - const response = await this.roleAssignmentRepository.getMembersDueRoleCredit(limit); - return response; + public async getMembersDueRoleCredit(limit: number): Promise { + return this.roleAssignmentRepository.getMembersDueRoleCredit(limit); } - public async giveWeeklyRoleCredit( - memberId: number, - memberXp: number, - walletId: number, - incomeXp: number, - incomeCc: number, - roleId: number, - ): Promise { - await this.transactionRepository.createWeeklyRoleCreditTransaction( - walletId, - incomeCc, - roleId, - ); - await this.memberRepository.update(memberId, { - last_weekly_role_credit: new Date(), - xp: memberXp + incomeXp, - - }); + /** + * Pays a member for one week of the highest-paying role they hold. + * + * Eligibility is rechecked, and the paying role re-resolved, inside the transaction + * that moves the money, so a member already paid by another worker is a no-op rather + * than a second payment. + * @param memberId id of the member to pay + * @returns promise resolving when the payout has been decided, rejecting on error + */ + public async giveWeeklyRoleCredit(memberId: number): Promise { + await this.creditRepository.giveWeeklyRoleCredit(memberId); } - public async countByAssigned(id: number): Promise { + /** + * Counts how many members hold the role with the given id. + * @param id id of the role to count assignments of + * @returns a single-row count, as knex returns it + */ + public async countByAssigned(id: number): Promise { return await this.roleAssignmentRepository.countByAssigned(id); } } From 6fdfad430029c41b522341f4f38141a182dce581 Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Tue, 25 Aug 2026 12:08:34 -0400 Subject: [PATCH 4/6] test: close the remaining ways a payout could still go wrong Both payouts now fail in the direction the implementation does not write first: the ledger insert is failed at the database, and the member row must come back untouched. Written the other way round, the earlier rollback specs only prove that whichever half runs second cannot commit alone. Also retries the daily credit after a failed attempt, which is where a split payout does its real damage - the weekly equivalent already covered it, and this is the same defect on the other path. --- api/src/cron/role-credit.integration.spec.ts | 34 +++++++++++++++ .../member/daily-credit.integration.spec.ts | 42 +++++++++++++++++-- 2 files changed, 73 insertions(+), 3 deletions(-) diff --git a/api/src/cron/role-credit.integration.spec.ts b/api/src/cron/role-credit.integration.spec.ts index 963ac6a1..8d5ddc42 100644 --- a/api/src/cron/role-credit.integration.spec.ts +++ b/api/src/cron/role-credit.integration.spec.ts @@ -299,8 +299,42 @@ describeWithDb('weekly role credit (real database)', () => { ); } + /** + * Fails the ledger half instead, so the two halves are proven to depend on each + * other in both directions rather than only in the one the implementation happens + * to write first. + */ + async function failLedgerWritesFor(walletId: number): Promise { + await knex.raw('DROP TRIGGER IF EXISTS b1_block_ledger_insert'); + await knex.raw( + `CREATE TRIGGER b1_block_ledger_insert BEFORE INSERT ON transaction FOR EACH ROW + BEGIN + IF NEW.recipient_wallet_id = ${walletId} THEN + SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'b1 injected ledger failure'; + END IF; + END`, + ); + } + afterEach(async () => { await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + await knex.raw('DROP TRIGGER IF EXISTS b1_block_ledger_insert'); + }); + + it('leaves the member untouched when the ledger half of the payout fails', async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: 50, + income_xp: 5, + })); + await failLedgerWritesFor(member.walletId); + + await expect(runRoleCreditCron()).rejects.toThrow(); + + expect(await balanceOf(member)).toBe(1000); + expect((await memberRow(member)).xp).toBe(0); + expect(await stampedToday(member)).toBe(false); }); it('leaves no money moved when the member half of the payout fails', async () => { diff --git a/api/src/services/member/daily-credit.integration.spec.ts b/api/src/services/member/daily-credit.integration.spec.ts index b6fa8c7a..02563dda 100644 --- a/api/src/services/member/daily-credit.integration.spec.ts +++ b/api/src/services/member/daily-credit.integration.spec.ts @@ -228,18 +228,23 @@ describeWithDb('daily login credit (real database)', () => { describe('rollback', () => { afterEach(async () => { await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + await knex.raw('DROP TRIGGER IF EXISTS b1_block_ledger_insert'); }); - it('moves no money when the member half of the credit fails', async () => { - const member = await createDueMember(); + async function failMemberUpdatesFor(memberId: number): Promise { await knex.raw( `CREATE TRIGGER b1_block_member_update BEFORE UPDATE ON member FOR EACH ROW BEGIN - IF NEW.id = ${member.id} THEN + IF NEW.id = ${memberId} THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'b1 injected member update failure'; END IF; END`, ); + } + + it('moves no money when the member half of the credit fails', async () => { + const member = await createDueMember(); + await failMemberUpdatesFor(member.id); await expect(service.maybeGiveDailyCredits(member.id)).rejects.toThrow(); @@ -248,5 +253,36 @@ describeWithDb('daily login credit (real database)', () => { expect((await memberRow(member)).xp).toBe(0); expect(await stampedToday(member)).toBe(false); }); + + it('leaves the member untouched when the ledger half of the credit fails', async () => { + const member = await createDueMember(); + await knex.raw( + `CREATE TRIGGER b1_block_ledger_insert BEFORE INSERT ON transaction FOR EACH ROW + BEGIN + IF NEW.recipient_wallet_id = ${member.walletId} THEN + SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'b1 injected ledger failure'; + END IF; + END`, + ); + + await expect(service.maybeGiveDailyCredits(member.id)).rejects.toThrow(); + + expect(await balanceOf(member)).toBe(1000); + expect((await memberRow(member)).xp).toBe(0); + expect(await stampedToday(member)).toBe(false); + }); + + it('credits exactly once when the attempt is retried after that failure', async () => { + const member = await createDueMember(); + await failMemberUpdatesFor(member.id); + await expect(service.maybeGiveDailyCredits(member.id)).rejects.toThrow(); + + await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + await service.maybeGiveDailyCredits(member.id); + + expect(await balanceOf(member)).toBe(1000 + UNEMPLOYED_CC); + expect(await dailyLedgerRows(member)).toHaveLength(1); + expect((await memberRow(member)).xp).toBe(UNEMPLOYED_XP); + }); }); }); From fe10e2229873337dcc52af74b74030e7dffdd079 Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Tue, 25 Aug 2026 18:08:40 -0400 Subject: [PATCH 5/6] fix: satisfy B1 lint gate without changing access semantics getAccessLevel() was annotated Promise, the sole ESLint warning across the files this branch touches. It resolves to a string[] of access tags, but three legacy callers compare the whole return value to a bare string, so annotating the true string[] makes tsc reject them. Type the return as LegacyAccessLevel = string[] | 'admin' | 'security' instead: narrow enough to name only the two scalars callers actually compare against, wide enough that those callers still compile. The method body is untouched and the emitted JavaScript is byte-for-byte identical across all 142 compiled files. Repairing the always-false comparisons would change who can reach admin functionality, so it is left to a separate authority lane. --- api/src/services/member/member.service.ts | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/api/src/services/member/member.service.ts b/api/src/services/member/member.service.ts index d3e13e51..10e064ef 100644 --- a/api/src/services/member/member.service.ts +++ b/api/src/services/member/member.service.ts @@ -66,6 +66,21 @@ interface StorageUnitSummary { count?: number; } +/** + * The value `MemberService.getAccessLevel()` resolves to. + * + * At runtime this is always a `string[]` of the access tags the member holds + * ('admin', 'security', 'leader'). The `'admin'` and `'security'` scalar members + * exist purely for compatibility: some legacy callers still compare the whole + * return value to a bare string (`accessLevel === 'admin'` in + * admin.controller.ts, `accessLevel === 'security'` in member.controller.ts) + * instead of testing membership. Those comparisons are an existing authority + * bug: they can never be true against the array this method actually returns. + * Repairing them changes who can reach admin functionality, so it is + * deliberately out of scope here and is tracked as separate authority work. + */ +type LegacyAccessLevel = string[] | 'admin' | 'security'; + /** Service for dealing with members */ @Service() export class MemberService { @@ -146,7 +161,7 @@ export class MemberService { }); } - public async getAccessLevel(memberId: number): Promise { + public async getAccessLevel(memberId: number): Promise { const security = await this.canAdmin(memberId); const roleAssignments = await this.roleAssignmentRepository.getByMemberId(memberId); const leader = await this.canLeader(memberId); From 9008902ee589f2732e983e629ee72de6ef2a6968 Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Tue, 25 Aug 2026 19:10:31 -0400 Subject: [PATCH 6/6] fix: correct wallet member lookup return type getMemberByWalletId was annotated Promise, but the query behind it, MemberRepository.findByWalletId, selects username alone. The rows it resolves to have never been full Member records, so the annotation promised columns that are undefined at runtime. No caller is affected today: all four sites in admin.controller.ts assign the result into a variable seeded [{username: 'System'}] and read nothing but username. The annotation would, however, have silently accepted new code reading id, email or xp off a row that does not carry them. Narrow the return to a WalletMemberSummary[] that describes what the query actually produces, rather than widening the query to match the annotation, which would change runtime behaviour. Type-only: emitted JavaScript is byte-for-byte identical across all 142 compiled files. The overstated annotation was introduced by 9fefaa7, not by the lint-gate commit. --- api/src/services/member/member.service.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/api/src/services/member/member.service.ts b/api/src/services/member/member.service.ts index 10e064ef..798ed3a5 100644 --- a/api/src/services/member/member.service.ts +++ b/api/src/services/member/member.service.ts @@ -66,6 +66,16 @@ interface StorageUnitSummary { count?: number; } +/** + * A member identified from their wallet, as `findByWalletId` actually returns them. + * + * The query behind it selects `username` alone, so a full `Member` is not available + * here however much the name suggests otherwise. + */ +interface WalletMemberSummary { + username: string; +} + /** * The value `MemberService.getAccessLevel()` resolves to. * @@ -688,7 +698,9 @@ export class MemberService { return unit; } - public async getMemberByWalletId(walletId: number): Promise { + public async getMemberByWalletId( + walletId: number, + ): Promise { const user = await this.memberRepository.findByWalletId(walletId); return user; }