Skip to content

Commit 50a7a80

Browse files
fix(billing): address CodeRabbit findings on PR #3688
- Move lib/distributedLock.js → lib/services/distributedLock.js (aligns with existing convention for service-level primitives) - Move lib/tests/distributedLock.unit.tests.js → lib/services/tests/ - Update import paths in 3 billing crons + integration tests - Flip logger.error arg order in dunningSweep + extrasExpiration finally (match file convention: (message, meta) not (meta, message)) - Same fix applied to weeklyReset (same pattern found, not flagged by CR) - Update modules/billing/crons/README.md → reference '## 6 — Cron lock stuck' (exact match with RUNBOOKS section header) Heartbeat/renewLock suggestion intentionally NOT added — see PR reply.
1 parent b971b2c commit 50a7a80

7 files changed

Lines changed: 20 additions & 20 deletions

File tree

lib/tests/distributedLock.unit.tests.js renamed to lib/services/tests/distributedLock.unit.tests.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
import { jest, describe, test, beforeEach, afterEach, expect } from '@jest/globals';
55

66
/**
7-
* Unit tests for lib/distributedLock.js
7+
* Unit tests for lib/services/distributedLock.js
88
*
99
* All Mongoose interactions are mocked — no real DB connection required.
1010
* Tests verify the acquire / release contract and contention handling.

modules/billing/crons/README.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ All scripts check `config.billing.meterMode` at startup. Downstream projects mus
121121

122122
## Concurrency control
123123

124-
All billing crons acquire a distributed lock (`lib/distributedLock.js`) before
124+
All billing crons acquire a distributed lock (`lib/services/distributedLock.js`) before
125125
mutating state. The lock auto-expires after TTL (5–15 min depending on cron)
126126
so that pod crashes don't permanently block scheduling.
127127

@@ -135,5 +135,5 @@ Lock names and TTLs:
135135

136136
If you see `lock held by another pod, skipping` in logs, that is expected when
137137
two pods race after a K8s `concurrencyPolicy` bypass (e.g. pod crash after
138-
jitter but before finalize). See the runbook entry `## Cron lock stuck` in
138+
jitter but before finalize). See the runbook entry `## 6 — Cron lock stuck` in
139139
`modules/billing/RUNBOOKS.md` for manual resolution.

modules/billing/crons/billing.dunningSweep.js

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ const [
3434
import('../../../lib/services/logger.js'),
3535
import('../lib/billing.cron-utils.js'),
3636
import('../lib/billing.constants.js'),
37-
import('../../../lib/distributedLock.js'),
37+
import('../../../lib/services/distributedLock.js'),
3838
]);
3939

4040
if (!config?.billing?.meterMode) {
@@ -116,10 +116,10 @@ try {
116116
try {
117117
await releaseLock({ name: LOCK_NAME, holder: lockHolder });
118118
} catch (releaseErr) {
119-
logger.error(
120-
{ err: releaseErr, cron: LOCK_NAME },
121-
'[cron.dunningSweep] failed to release lock — will auto-expire on TTL',
122-
);
119+
logger.error('[cron.dunningSweep] failed to release lock — will auto-expire on TTL', {
120+
err: releaseErr,
121+
cron: LOCK_NAME,
122+
});
123123
}
124124
}
125125
}

modules/billing/crons/billing.extrasExpiration.js

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ const [
2929
import('../../../lib/services/logger.js'),
3030
import('../lib/billing.cron-utils.js'),
3131
import('../lib/billing.constants.js'),
32-
import('../../../lib/distributedLock.js'),
32+
import('../../../lib/services/distributedLock.js'),
3333
]);
3434

3535
if (!config?.billing?.meterMode) {
@@ -86,10 +86,10 @@ try {
8686
try {
8787
await releaseLock({ name: LOCK_NAME, holder: lockHolder });
8888
} catch (releaseErr) {
89-
logger.error(
90-
{ err: releaseErr, cron: LOCK_NAME },
91-
'[cron.extrasExpiration] failed to release lock — will auto-expire on TTL',
92-
);
89+
logger.error('[cron.extrasExpiration] failed to release lock — will auto-expire on TTL', {
90+
err: releaseErr,
91+
cron: LOCK_NAME,
92+
});
9393
}
9494
}
9595
}

modules/billing/crons/billing.weeklyReset.js

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ const [
2828
import('../../../lib/services/logger.js'),
2929
import('../lib/billing.cron-utils.js'),
3030
import('../lib/billing.constants.js'),
31-
import('../../../lib/distributedLock.js'),
31+
import('../../../lib/services/distributedLock.js'),
3232
]);
3333

3434
if (!config?.billing?.meterMode) {
@@ -66,10 +66,10 @@ try {
6666
try {
6767
await releaseLock({ name: LOCK_NAME, holder: lockHolder });
6868
} catch (releaseErr) {
69-
logger.error(
70-
{ err: releaseErr, cron: LOCK_NAME },
71-
'[cron.weeklyReset] failed to release lock — will auto-expire on TTL',
72-
);
69+
logger.error('[cron.weeklyReset] failed to release lock — will auto-expire on TTL', {
70+
err: releaseErr,
71+
cron: LOCK_NAME,
72+
});
7373
}
7474
}
7575
}

modules/billing/tests/billing.cron-lock.integration.tests.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,10 @@
44
import { describe, beforeAll, beforeEach, afterAll, test, expect } from '@jest/globals';
55

66
import mongooseService from '../../../lib/services/mongoose.js';
7-
import { acquireLock, releaseLock, CronLock } from '../../../lib/distributedLock.js';
7+
import { acquireLock, releaseLock, CronLock } from '../../../lib/services/distributedLock.js';
88

99
/**
10-
* Integration tests for lib/distributedLock.js — real MongoDB.
10+
* Integration tests for lib/services/distributedLock.js — real MongoDB.
1111
*
1212
* Verifies the acquire / release / contention / expiry contract end-to-end.
1313
* The cron scripts themselves are top-level-await CLI entry points that cannot

0 commit comments

Comments
 (0)