Fix daily and weekly job credit reliability by DJAscendance · Pull Request #423 · CybertownRevival/ctr

DJAscendance · 2026-08-25T16:14:14Z

Problem

A citizen should receive each credit they earn exactly once, with their wallet, the ledger, their XP and the timestamp that gates the next payment all moving together. Neither job-credit path did that.

Weekly payroll paid people who had not earned anything. getMembersDueRoleCredit's inner join existed only to ask "does this member hold a job", with no income filter, so any role_assignment row qualified its holder — including a role paying 0 CityCash and 0 XP. Admin is seeded exactly that way, so this 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 20 slots away from someone who actually earned pay.

XP-only roles had to keep earning. income_cc and income_xp are independent columns, so filtering eligibility on CityCash alone would have silently stopped an XP-only role ever accruing its XP — one bug traded for another. No seeded role is XP-only today, which is precisely why it would have gone unnoticed.

The daily credit was fire-and-forget. login() called maybeGiveDailyCredits without awaiting it, so the token went back with the credit still in flight. The caller could read its own balance and not see it; two quick logins could both observe "not credited today"; and if the request finished first, nothing kept the process interested in the write.

Both payouts committed in halves. Wallet and ledger went in one transaction, member XP and the eligibility timestamp in another. Fail the second and the money has moved while the member is still eligible — the next attempt pays again.

Both payouts read eligibility outside the transaction that paid. Two overlapping cron executions select the same member before either marks them paid, and both pay. Two logins arriving together do the same.

Wallet updates could overwrite each other. Every credit read wallet.balance and wrote back read + amount. A daily credit and a weekly credit landing on the same wallet — an ordinary Friday morning — lose one of the two.

What changed

CreditRepository now owns both payouts, each as a single database transaction with the same shape:

lock the member row with SELECT ... FOR UPDATE;
re-evaluate eligibility against the locked row — never against anything a caller passed in or a batch query read earlier;
write wallet, ledger, XP and eligibility timestamp;
commit.

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 becomes a no-op. Weekly eligibility is evaluated inside the locking SELECT rather than by a follow-up query, so the answer cannot come from a different point in time than the lock. Wallets move by balance = balance + ?, so the database computes the new total from whatever is committed at that instant. The daily and weekly payout writers therefore compose safely with each other, and with any other writer that also mutates the balance atomically. The member row is always the first row locked on both paths, so they cannot deadlock against each other.

getMembersDueRoleCredit now returns member ids and nothing else, filtered to members holding a role that pays CityCash or XP. The per-member "which role would pay" lookup it used to do is gone: the payout re-resolves that under the lock, where it is still true. login, createMemberAndLogin and the session refresh all go through one helper that awaits the credit 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 behind. The session refresh could previously return a 500 and log a member out for the same reason.

TransactionRepository.createDailyCreditTransaction and createWeeklyRoleCreditTransaction are removed rather than left dead: they are the read-then-write wallet update this change exists to stop.

Residual risk, outside this PR. Independent QA found that pre-existing legacy TransactionRepository methods which read a wallet balance and later write an absolute calculated value can still overwrite an intervening atomic credit — the ledger row and the eligibility timestamp survive, but the credited amount is gone from the wallet. Modernizing those unrelated wallet writers is a separate platform-wide follow-up and is not part of this PR; it is tracked as Cyber-Town-Next-Gen/ctr-restoration#11.

Single pay is unchanged — a citizen holding several jobs is paid for one of them, the highest-paying — with ties on CityCash now going to the role granting more XP rather than to whichever row the database returned first.

Recovered work

This branch reconstructs the relevant behaviour onto current upstream. It does not merge beta, does not merge fork master, and cherry-picks nothing from the 29-commit fix/daily-credit-await branch or the 107-commit beta branch. Credit where it is due:

source	carried forward as
DJAscendance#11	the daily-credit lane this reconstructs; kept open as its archive
`4cc7fb2d`	the knexfile `test` environment, without which no suite could load
`cd915c79`	awaiting the daily credit at login, and catching its failure
`3b597880`	excluding non-earning roles from payroll
`7af83c74`	`income_cc > 0 OR income_xp > 0`, with XP as the CityCash tie-break
`786231a0` (squash-merged upstream as `cbcc2b0`, PR #204)	single pay: one role per citizen, the highest-paying

Out of scope

Deliberately unchanged, and verified unchanged in the diff:

salary and XP amounts — no role's income_cc/income_xp and no seed data is touched; DAILY_CC_AMOUNT (50), DAILY_XP_AMOUNT (5), DAILY_CC_EMPLOYED_AMOUNT (100) and DAILY_XP_EMPLOYED_AMOUNT (10) keep their values, and the specs assert those numbers literally so a future change has to be deliberate;
Colony Representative, City Mayor, Deputy Mayor authority — no authority check is modified; upstream Phil00 live event #418 is neither touched nor depended on;
place-scoped hiring — place_id still plays no part in who gets paid, and there is a test proving a place-scoped assignment pays exactly as an unscoped one. Whether it should is a separate question;
Access Rights, worker UI, SPA — no file outside api/ is touched;
unrelated transaction methods — only the two the payouts themselves used are removed.

Who counts as "employed" for the daily bonus is also unchanged: any role assignment qualifies, including one that pays nothing. Only weekly payroll cares whether a role earns, and there is a test pinning that distinction.

Verification

Node 14.21.3, MySQL 5.7.44 in a throwaway container on port 13311, schema ctr_b1_itest — created for this work and holding nothing else. The database-backed specs refuse to write unless CTR_INTEGRATION_TEST_DB names the configured database exactly, so a merely reachable database does not arm them; without the opt-in they register as skipped, never as a silent pass.

$ npx jest --runInBand
Test Suites: 3 failed, 10 passed, 13 total
Tests:       43 passed, 43 total

All 43 tests pass. The three failing suites are pre-existing and untouched: wallet.service.spec.ts, club.service.spec.ts and role.repository.spec.ts contain no tests at all (the last is a stray copy of a repository class), and jest reports "Your test suite must contain at least one test" as a failure. They fail identically on upstream master.

Before the fix, on unmodified upstream with only the knexfile key and the new specs added, 11 of the 23 new database-backed tests failed — one for each defect above. Recorded in before-failures.txt.

Concurrency, forced rather than hoped for: a trigger delays every wallet update, so two payouts starting together are guaranteed to overlap.

two overlapping cron executions → one credit, one ledger row, one XP increase;
two logins arriving together → same;
a daily credit racing the weekly payroll on one wallet → both land, balance exact;
8 concurrent cron workers over 6 eligible members → each paid exactly once;
10 concurrent daily credits for one member → credited once.

Re-run five times with no flakes.

Rollback, injected at the database with a trigger raising SQLSTATE 45000 — something no mock can do:

fail the member half → wallet unchanged, no ledger row, XP unchanged, member still eligible;
fail the ledger half → member row untouched, tested in both directions so neither half can commit alone;
retry after either failure → paid exactly once, on both the daily and the weekly path.

Payroll behaviour, each proved against real SQL:

role	outcome
0cc / 0xp	not eligible, not paid, not stamped, takes no batch slot
40cc / 0xp	paid
0cc / 7xp	paid
0/0 plus 50/5	pays the 50/5 role, one ledger row
60/1 vs 60/9	pays the 60/9 role
`place_id` NULL vs 1	identical

Lint. All 13 touched files are at zero errors and zero warnings (eslint --max-warnings=0). MemberService.getAccessLevel no longer returns Promise<any>: it is typed LegacyAccessLevel = string[] | 'admin' | 'security', a deliberately narrow compatibility type. The method still returns exactly the array it always returned — the change is type-only, and the emitted JavaScript is byte-for-byte identical across all 142 compiled files. The two scalar members exist solely because legacy callers compare the whole return value to a bare string: accessLevel === 'admin' in admin.controller.ts and accessLevel === 'security' in member.controller.ts. Those comparisons can never be true against the array actually returned — a live authority bug worth its own issue. Repairing them would grant access currently denied, which is not this PR's to decide, so they are left untouched and the type accommodates them.

Typecheck / build. tsc --noEmit, npm run build and npm run build:prod all exit 0, with dist/ laid out exactly as on master.

Running the database-backed specs

Point DB_* at a disposable schema, set CTR_INTEGRATION_TEST_DB to the same database name, and run npx jest --runInBand (they share the member table). Bootstrapping that schema needs one manual step, because of a pre-existing upstream defect this PR deliberately does not fix: 20260309032638_add_voting_tables.ts creates its tables and then inserts a vote_list row with place_id = 1 in the same migration. On an empty database no place exists yet, the foreign key rejects the insert, and since MySQL DDL is not transactional the three vote tables survive — so every retry dies with ER_TABLE_EXISTS_ERROR. Seeds run after migrations, so npm run db:init can never succeed on a fresh schema. Working order: migrate (fails), drop the three orphaned vote tables, seed, migrate again.

…eal 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.

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<any>`: 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.

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.

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.

getAccessLevel() was annotated Promise<any>, 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.

getMemberByWalletId was annotated Promise<Member[]>, 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.

DJAscendance · 2026-08-25T23:19:21Z

Ready for upstream review.
Independent QA is complete, including real MySQL concurrency/rollback testing, final delta QA, and an additional Ultracode review. No correctness blockers remain; the one review nit was fixed in the current head. The separate legacy wallet read-modify-write risk is tracked outside this PR.

Copilot

Pull request overview

This PR hardens the daily login credit and weekly payroll credit paths so each eligible member is credited exactly once, with wallet balance, ledger row, XP, and eligibility timestamps updated atomically and safely under concurrency.

Changes:

Introduces CreditRepository to own daily + weekly payouts as single-row-locking, all-in-one DB transactions (atomic wallet increment, ledger insert, XP, and timestamp updates).
Fixes weekly payroll eligibility batching to exclude non-earning roles and to return only member ids; payout re-resolves the paying role under lock.
Updates login/session paths to await daily credit and swallow credit failures (don’t fail login), and adds DB-backed integration specs for concurrency/rollback behavior.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
api/src/services/role-assignment/role-assignment.service.ts	Switch weekly payout execution to `CreditRepository` and tighten return types.
api/src/services/member/member.service.ts	Route daily credit through `CreditRepository`, await on login, and improve typing.
api/src/services/member/member.service.spec.ts	Update unit tests to assert daily credit call via `CreditRepository`.
api/src/services/member/daily-credit.integration.spec.ts	Add real-DB integration coverage for daily credit correctness, concurrency, and rollback.
api/src/repositories/transaction/transaction.repository.ts	Remove legacy credit transaction helpers and improve repository typing.
api/src/repositories/role-assignment/role-assignment.repository.ts	Fix weekly batch selection to only include earning roles and return member ids.
api/src/repositories/index.ts	Export new `CreditRepository`.
api/src/repositories/credit/credit.repository.ts	New transactional payout implementation (daily + weekly) with row locking.
api/src/knexfile.ts	Add `test` knex env configuration needed for Jest + integration-db specs.
api/src/cron/role-credit.ts	Cron now iterates member ids and delegates payout to the new weekly credit path.
api/src/cron/role-credit.integration.spec.ts	Add real-DB integration coverage for weekly payroll eligibility, concurrency, and rollback.
api/src/controllers/member.controller.ts	Ensure session refresh path awaits daily credit via the new helper.
api/spec/integration-db.ts	Add integration DB opt-in + fixtures helpers for safe real-DB tests.

Suppressed comments (1)

api/src/repositories/credit/credit.repository.ts:215

first() can return undefined when the member id doesn’t exist, and giveDailyCredit checks if (!member). Update the return type to include undefined so the type reflects runtime behavior.

  private async lockMember(trx: Knex.Transaction, memberId: number): Promise<Member> {

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+/** A role a member holds, as the member views list it. */
+interface MemberRoleSummary {
+  id: number;
+  place_id: number;


+/** A role a member holds, with the place it is scoped to if any. */
+interface MemberRoleRow {
+  id: number;
+  place_id: number;


+  knex: Knex,
+  memberId: number,
+  roleId: number,
+  placeId: number = null,


+  private async lockMemberDueWeeklyCredit(
+    trx: Knex.Transaction,
+    memberId: number,
+  ): Promise<{ wallet_id: number; due: number }> {


+      it('still logs the member in', async () => {
+        const token = await service.login(fakeMember.username, fakeMember.password);
+        expect(token).toBe(await service.getMemberToken(fakeMember.id));
      });


DJAscendance added 4 commits August 25, 2026 11:54

DJAscendance added this to CTR Restoration Tracker Aug 25, 2026

DJAscendance moved this to QA in CTR Restoration Tracker Aug 25, 2026

This was referenced Aug 25, 2026

City Jobs B1 — Restore reliable job pay Cyber-Town-Next-Gen/ctr-restoration#4

Open

Infrastructure — eliminate legacy wallet read-modify-write races Cyber-Town-Next-Gen/ctr-restoration#11

Open

DJAscendance added 2 commits August 25, 2026 18:08

DJAscendance marked this pull request as ready for review August 25, 2026 23:18

Copilot AI lite review requested due to automatic review settings August 25, 2026 23:18

Copilot started reviewing on behalf of DJAscendance August 25, 2026 23:19 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

Navigation Menu

Uh oh!

Fix daily and weekly job credit reliability - #423

Fix daily and weekly job credit reliability#423
DJAscendance wants to merge 6 commits into
CybertownRevival:masterfrom
DJAscendance:fix/city-job-pay-reliability

DJAscendance commented Aug 25, 2026 •

edited

Loading

Uh oh!

DJAscendance commented Aug 25, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

DJAscendance commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

What changed

Recovered work

Out of scope

Verification

Running the database-backed specs

Uh oh!

DJAscendance commented Aug 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DJAscendance commented Aug 25, 2026 •

edited

Loading