Skip to content

Commit b971b2c

Browse files
fix(billing): address DeepSeek gate findings on cron lock
- HIGH: restore (errors || desyncErrors) condition in dunningSweep exit code (regression introduced when wrapping with lock — would make K8s alerts silent) - MEDIUM: wrap releaseLock in try/catch within finally to preserve original work error if both throw - LOW: guard acquireLock against invalid ttlMs (must be positive finite) - NIT: drop redundant setDefaultsOnInsert (all fields explicitly $set) Addresses gate iteration 1 BLOCK.
1 parent d3264b8 commit b971b2c

5 files changed

Lines changed: 46 additions & 14 deletions

File tree

lib/distributedLock.js

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,13 +50,16 @@ export const CronLock = mongoose.models.CronLock ?? mongoose.model('CronLock', L
5050
* @returns {Promise<boolean>}
5151
*/
5252
export async function acquireLock({ name, ttlMs, holder }) {
53+
if (!Number.isFinite(ttlMs) || ttlMs <= 0) {
54+
throw new Error(`acquireLock: ttlMs must be a positive number, received ${ttlMs}`);
55+
}
5356
const now = new Date();
5457
const lockedUntil = new Date(now.getTime() + ttlMs);
5558
try {
5659
const result = await CronLock.findOneAndUpdate(
5760
{ _id: name, lockedUntil: { $lt: now } },
5861
{ $set: { lockedAt: now, lockedUntil, holder } },
59-
{ upsert: true, returnDocument: 'after', setDefaultsOnInsert: true },
62+
{ upsert: true, returnDocument: 'after' },
6063
);
6164
return result?.holder === holder;
6265
} catch (err) {

lib/tests/distributedLock.unit.tests.js

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,20 @@ describe('distributedLock — acquireLock:', () => {
8383
await expect(acquireLock({ name: 'job-d', ttlMs: 60_000, holder: 'pod-1' })).rejects.toThrow('network timeout');
8484
});
8585

86+
test.each([
87+
[0, 'zero'],
88+
[-1, 'negative'],
89+
[Number.NaN, 'NaN'],
90+
[Infinity, 'Infinity'],
91+
[undefined, 'undefined'],
92+
[null, 'null'],
93+
])('throws when ttlMs is %s (%s)', async (ttlMs) => {
94+
await expect(acquireLock({ name: 'job-guard', ttlMs, holder: 'pod-1' })).rejects.toThrow(
95+
'acquireLock: ttlMs must be a positive number',
96+
);
97+
expect(mockFindOneAndUpdate).not.toHaveBeenCalled();
98+
});
99+
86100
test('lockedUntil is set to now + ttlMs', async () => {
87101
const before = Date.now();
88102
mockFindOneAndUpdate.mockResolvedValue({ holder: 'pod-1' });

modules/billing/crons/billing.dunningSweep.js

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -109,13 +109,18 @@ try {
109109
}
110110

111111
logger.info('[cron.dunningSweep] complete', { processed, errors, desyncErrors, durationMs: Date.now() - startMs });
112-
process.exitCode = errors > 0 ? 1 : 0;
112+
process.exitCode = errors > 0 || desyncErrors > 0 ? 1 : 0;
113113
} finally {
114114
// releaseLock failure is non-fatal: lock auto-expires on TTL.
115-
// If this throws, the outer catch logs "failed" and sets exitCode=1
116-
// (misleading — the cron's actual work succeeded). Operators can grep
117-
// for "failed to release lock" to distinguish.
118-
await releaseLock({ name: LOCK_NAME, holder: lockHolder });
115+
// Log separately to preserve any original work error.
116+
try {
117+
await releaseLock({ name: LOCK_NAME, holder: lockHolder });
118+
} catch (releaseErr) {
119+
logger.error(
120+
{ err: releaseErr, cron: LOCK_NAME },
121+
'[cron.dunningSweep] failed to release lock — will auto-expire on TTL',
122+
);
123+
}
119124
}
120125
}
121126
} catch (err) {

modules/billing/crons/billing.extrasExpiration.js

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,15 @@ try {
8282
process.exitCode = errors > 0 ? 1 : 0;
8383
} finally {
8484
// releaseLock failure is non-fatal: lock auto-expires on TTL.
85-
// If this throws, the outer catch logs "failed" and sets exitCode=1
86-
// (misleading — the cron's actual work succeeded). Operators can grep
87-
// for "failed to release lock" to distinguish.
88-
await releaseLock({ name: LOCK_NAME, holder: lockHolder });
85+
// Log separately to preserve any original work error.
86+
try {
87+
await releaseLock({ name: LOCK_NAME, holder: lockHolder });
88+
} catch (releaseErr) {
89+
logger.error(
90+
{ err: releaseErr, cron: LOCK_NAME },
91+
'[cron.extrasExpiration] failed to release lock — will auto-expire on TTL',
92+
);
93+
}
8994
}
9095
}
9196
} catch (err) {

modules/billing/crons/billing.weeklyReset.js

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,15 @@ try {
6262
process.exitCode = result.errors > 0 ? 1 : 0;
6363
} finally {
6464
// releaseLock failure is non-fatal: lock auto-expires on TTL.
65-
// If this throws, the outer catch logs "failed" and sets exitCode=1
66-
// (misleading — the cron's actual work succeeded). Operators can grep
67-
// for "failed to release lock" to distinguish.
68-
await releaseLock({ name: LOCK_NAME, holder: lockHolder });
65+
// Log separately to preserve any original work error.
66+
try {
67+
await releaseLock({ name: LOCK_NAME, holder: lockHolder });
68+
} catch (releaseErr) {
69+
logger.error(
70+
{ err: releaseErr, cron: LOCK_NAME },
71+
'[cron.weeklyReset] failed to release lock — will auto-expire on TTL',
72+
);
73+
}
6974
}
7075
}
7176
} catch (err) {

0 commit comments

Comments
 (0)