Wait for the daily login credit before returning the token - #11
Open
DJAscendance wants to merge 2 commits into
Open
Wait for the daily login credit before returning the token#11DJAscendance wants to merge 2 commits into
DJAscendance wants to merge 2 commits into
Conversation
added 2 commits
July 31, 2026 20:07
`npm test` reported 8 of 12 suites as failing. None of them were failing on an
assertion -- they died while loading:
TypeError: Cannot read properties of undefined (reading 'client')
at new Db (src/db/db.class.ts:13:22)
Db's constructor runs `knex(config[process.env.NODE_ENV])` at import time, jest
sets NODE_ENV=test, and knexfile only defined `development` and `production`.
`config['test']` was undefined, so every module that transitively imported a
repository blew up before a single test ran. Four tests executed in total.
Adding the key takes that to 8 suites passing and 28 tests running.
pool.min is 0 here rather than 2: a minimum of 2 makes the pool open
connections nothing asked for, and jest then hangs waiting on handles that will
never close. Unit tests never open a socket regardless, because knex connects
lazily -- they only needed the constructor not to throw.
Connection details still come from the environment, so this is also the key a
database-backed test points at; set DB_DATABASE to a throwaway schema for that.
Two things this exposes, both pre-existing and neither caused by this change:
- src/services/member/member.service.spec.ts has 2 genuine assertion
failures. They are real; see the next commit.
- role.repository.spec.ts, wallet.service.spec.ts and club.service.spec.ts
are empty files, so jest reports "Your test suite must contain at least one
test". They are stubs that were never written, previously indistinguishable
from the import crash.
`login()` called `this.maybeGiveDailyCredits(member.id)` without awaiting it, so the credit was still in flight when the token went back to the caller. The existing spec asserts exactly this and has been failing all along -- nobody saw it, because the suite could not load at all until the previous commit. The call has been unawaited since 633b7ce (December 2022), which added two call sites in one change: one awaited, one not. This is the one that was not. What the race costs: - the caller can read its own balance and not see the credit yet; - two quick logins can both observe "not credited today" and pay twice, since the check and the write are no longer close together; - if the request finished first, nothing kept the process interested in the write, so the credit could simply be lost. Now awaited, and caught: refusing someone their account because a daily bonus failed would be a worse outcome than the missing bonus. Previously a failure here surfaced as an unhandled promise rejection and nothing else. member.service.spec.ts goes from 7/9 to 9/9. Full suite: 28/28 tests pass. Same shape of bug as the fire-and-forget reconcilePrimaryRole in fix/primary-role-atomicity -- worth a sweep for other unawaited service calls, which is not attempted here.
📝 WalkthroughWalkthroughThe pull request adds a test Knex environment for MySQL and updates member login to await daily credit distribution while preserving authentication when distribution fails. ChangesTest database configuration
Login credit handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/src/knexfile.ts`:
- Line 54: Update the knex configuration’s DB_PORT handling to parse with radix
10 and explicitly handle unset, empty, or invalid values; use the project’s
documented default of 3306 if applicable, otherwise throw a clear configuration
error instead of passing NaN.
In `@api/src/services/member/member.service.ts`:
- Around line 363-376: Make maybeGiveDailyCredits atomic and idempotent: execute
the daily-credit eligibility check, payout creation, and member timestamp update
within one database transaction, using a conditional claim or unique (memberId,
creditDate, creditType) constraint to prevent concurrent duplicate payouts and
allow safe retries after partial failures. Update the login flow’s comments to
reflect the transactional behavior, and add tests covering concurrent logins and
retrying after a failed transaction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ad6d8741-28f0-4d46-ba0b-354155e39d64
📒 Files selected for processing (2)
api/src/knexfile.tsapi/src/services/member/member.service.ts
| client: 'mysql', | ||
| connection: { | ||
| host: process.env.DB_HOST, | ||
| port: Number.parseInt(process.env.DB_PORT), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate knexfile =="
fd -a 'knexfile\.(ts|js|mjs|cjs)$' . || true
echo "== relevant knexfile =="
if [ -f api/src/knexfile.ts ]; then
nl -ba api/src/knexfile.ts | sed -n '1,120p'
fi
echo "== search DB_PORT usage and docs =="
rg -n "DB_PORT|process\.env\.DB_PORT|Number\.parseInt|parseInt\(" -S . --glob '!node_modules' --glob '!dist' --glob '!build' | head -200
echo "== parse DB_PORT behavior probe =="
node - <<'JS'
const values = [undefined, "", "3306", "abc", "0xc42"];
for (const v of values) process.env.DB_PORT = v;
for (const v of values) {
const used = process.env.DB_PORT;
console.log(JSON.stringify({env: used, Number.parseInt: Number.parseInt(used), JSON: Number.isNaN(Number.parseInt(used))}));
}
JSRepository: DJAscendance/ctr
Length of output: 1921
Handle an absent or invalid DB_PORT.
Number.parseInt(process.env.DB_PORT) returns NaN when DB_PORT is unset, empty, invalid, or hexadecimal. Default only if the project contract is port 3306; otherwise validate and throw a clear configuration error. Use radix 10 for parseInt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/src/knexfile.ts` at line 54, Update the knex configuration’s DB_PORT
handling to parse with radix 10 and explicitly handle unset, empty, or invalid
values; use the project’s documented default of 3306 if applicable, otherwise
throw a clear configuration error instead of passing NaN.
Comment on lines
+363
to
+376
| // Awaited, so the credit has actually landed before the token goes back. Unawaited, | ||
| // this raced the response: the caller could read its own balance and not see it yet, | ||
| // two quick logins could both observe "not credited today" and pay twice, and a | ||
| // request that finished first left the write in flight with nothing keeping the | ||
| // process around to finish it. | ||
| // | ||
| // Caught rather than propagated, because a failure to hand out a daily bonus is not a | ||
| // reason to refuse someone their account. Before, errors here surfaced as an | ||
| // unhandled rejection instead of anything anyone would see. | ||
| try { | ||
| await this.maybeGiveDailyCredits(member.id); | ||
| } catch (error) { | ||
| console.error(`Failed to give daily credits to member ${member.id}:`, error); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make daily credit claiming atomic and idempotent. await does not fix the duplicate-payout race.
The comment on Lines [363]-[367] is not correct for the current implementation. await only waits for the current login() call. Two concurrent calls can still enter maybeGiveDailyCredits together.
That method checks the timestamp at Line [388], then creates the wallet transaction at Line [398] and updates the member at Lines [399]-[402] in separate operations. Both calls can pass the check and create duplicate payouts. If the transaction succeeds and the member update fails, the catch at Lines [374]-[376] can also allow a later retry to pay again.
Wrap the credit claim and writes in one database transaction. Use a conditional claim or a unique (memberId, creditDate, creditType) guard. Add concurrent-login and partial-failure retry tests.
#!/bin/bash
set -euo pipefail
# Verify whether repository methods already enforce an atomic daily-credit guard.
rg -n -C 12 \
'maybeGiveDailyCredits|createDailyCreditTransaction|last_daily_login_credit|creditDate|daily.*credit' \
api --glob '*.ts'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/src/services/member/member.service.ts` around lines 363 - 376, Make
maybeGiveDailyCredits atomic and idempotent: execute the daily-credit
eligibility check, payout creation, and member timestamp update within one
database transaction, using a conditional claim or unique (memberId, creditDate,
creditType) constraint to prevent concurrent duplicate payouts and allow safe
retries after partial failures. Update the login flow’s comments to reflect the
transactional behavior, and add tests covering concurrent logins and retrying
after a failed transaction.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a race in the daily login payout that has been live since December 2022.
The bug
login()calledmaybeGiveDailyCredits(member.id)without awaiting it, so the credit was still in flight when the token went back to the caller.Three ways it bites:
Introduced by
633b7ce, which added two call sites in one change — one awaited, one not. This is the one that was not.Now awaited and caught: refusing someone their account because a daily bonus failed would be a worse outcome than the missing bonus. Previously a failure here surfaced as an unhandled promise rejection and nothing else.
Why the knexfile commit is in here
A spec asserting exactly this behaviour existed the whole time and had never once executed.
knexfile.tshad notestenvironment, soconfig[process.env.NODE_ENV]wasundefinedunder Jest and 8 of 12 suites died at import:Without that commit the fix ships unprovable, and the fix's own commit message refers back to it. They are separate commits so the split is still easy if you want the one-file change on its own.
Verification
member.service.spec.ts9/9 with the fix (was 7/9).Note the spec reaches a real database through an unmocked transaction, so it needs
ctr-db-1up; that is a pre-existing issue, not introduced here.Out of scope, but worth knowing
admin.services.ts:98firesaddIdToAssignmentunawaited — that assigns a role, and roles are income.Summary by CodeRabbit
Bug Fixes
Tests