fix(billing): address code-quality findings on cron lock

- Move distributedLock unit test to canonical lib location (or document deviation)
- Add releaseLock throw-path test + non-fatal comment in 3 crons' finally
- Harmonize log style in skip-on-contention path
- Rename + document integration test (in-process concurrency clarification)
- Add findOne verification step in RUNBOOKS before deleteOne

Addresses code-quality review I1, I2, M1, M3, M4.

Author: PierreBrisorgueil

…ices/tests/distributedLock.unit.tests.js lib/tests/distributedLock.unit.tests.jslib/services/tests/distributedLock.unit.tests.js renamed to lib/tests/distributedLock.unit.tests.js lib/services/tests/distributedLock.unit.tests.js renamed to lib/tests/distributedLock.unit.tests.js

@@ -36,7 +36,7 @@ describe('distributedLock — acquireLock:', () => {
       },
     }));
 
-    ({ acquireLock } = await import('../../distributedLock.js'));
+    ({ acquireLock } = await import('../distributedLock.js'));
   });
 
   afterEach(() => {
@@ -121,7 +121,7 @@ describe('distributedLock — releaseLock:', () => {
       },
     }));
 
-    ({ releaseLock } = await import('../../distributedLock.js'));
+    ({ releaseLock } = await import('../distributedLock.js'));
   });
 
   afterEach(() => {
@@ -137,4 +137,10 @@ describe('distributedLock — releaseLock:', () => {
   test('does not throw when deleteOne resolves', async () => {
     await expect(releaseLock({ name: 'job-b', holder: 'pod-2' })).resolves.toBeUndefined();
   });
+
+  test('propagates deleteOne errors to the caller', async () => {
+    const dbErr = new Error('network timeout');
+    mockDeleteOne.mockRejectedValue(dbErr);
+    await expect(releaseLock({ name: 'job-c', holder: 'pod-1' })).rejects.toThrow('network timeout');
+  });
 });

modules/billing/RUNBOOKS.md

@@ -188,6 +188,16 @@ Operational runbooks for the billing module. Each runbook references real endpoi
 
 **If urgent — drop the stale lock manually:**
 
+**Before drop:** verify the holder and TTL window first to avoid kicking a running cron.
+
+```js
+db.cron_locks.findOne({ _id: "billing.weeklyReset" })
+// If lockedUntil is in the past → safe to drop.
+// If in the future → the lock is genuinely held; wait for TTL unless the holder pod is confirmed dead.
+```
+
+Then drop:
+
 ```js
 // weeklyReset
 db.cron_locks.deleteOne({ _id: "billing.weeklyReset" })

modules/billing/crons/billing.dunningSweep.js

@@ -56,7 +56,7 @@ try {
   lockHolder = `${process.env.HOSTNAME ?? 'unknown'}:${randomUUID()}`;
   const acquired = await acquireLock({ name: LOCK_NAME, ttlMs: LOCK_TTL_MS, holder: lockHolder });
   if (!acquired) {
-    logger.info({ cron: LOCK_NAME }, 'lock held by another pod, skipping');
+    logger.info('[cron.dunningSweep] lock held by another pod, skipping');
     process.exitCode = 0;
   } else {
     try {
@@ -111,6 +111,10 @@ try {
       logger.info('[cron.dunningSweep] complete', { processed, errors, desyncErrors, durationMs: Date.now() - startMs });
       process.exitCode = errors > 0 ? 1 : 0;
     } finally {
+      // releaseLock failure is non-fatal: lock auto-expires on TTL.
+      // If this throws, the outer catch logs "failed" and sets exitCode=1
+      // (misleading — the cron's actual work succeeded). Operators can grep
+      // for "failed to release lock" to distinguish.
       await releaseLock({ name: LOCK_NAME, holder: lockHolder });
     }
   }

modules/billing/crons/billing.extrasExpiration.js

@@ -51,7 +51,7 @@ try {
   lockHolder = `${process.env.HOSTNAME ?? 'unknown'}:${randomUUID()}`;
   const acquired = await acquireLock({ name: LOCK_NAME, ttlMs: LOCK_TTL_MS, holder: lockHolder });
   if (!acquired) {
-    logger.info({ cron: LOCK_NAME }, 'lock held by another pod, skipping');
+    logger.info('[cron.extrasExpiration] lock held by another pod, skipping');
     process.exitCode = 0;
   } else {
     try {
@@ -81,6 +81,10 @@ try {
       logger.info('[cron.extrasExpiration] complete', { processed, errors, durationMs: Date.now() - startMs });
       process.exitCode = errors > 0 ? 1 : 0;
     } finally {
+      // releaseLock failure is non-fatal: lock auto-expires on TTL.
+      // If this throws, the outer catch logs "failed" and sets exitCode=1
+      // (misleading — the cron's actual work succeeded). Operators can grep
+      // for "failed to release lock" to distinguish.
       await releaseLock({ name: LOCK_NAME, holder: lockHolder });
     }
   }

modules/billing/crons/billing.weeklyReset.js

@@ -51,7 +51,7 @@ try {
   lockHolder = `${process.env.HOSTNAME ?? 'unknown'}:${randomUUID()}`;
   const acquired = await acquireLock({ name: LOCK_NAME, ttlMs: LOCK_TTL_MS, holder: lockHolder });
   if (!acquired) {
-    logger.info({ cron: LOCK_NAME }, 'lock held by another pod, skipping');
+    logger.info('[cron.weeklyReset] lock held by another pod, skipping');
     process.exitCode = 0;
   } else {
     try {
@@ -61,6 +61,10 @@ try {
       logger.info('[cron.weeklyReset] complete', { processed: result.processed, errors: result.errors, durationMs: Date.now() - startMs });
       process.exitCode = result.errors > 0 ? 1 : 0;
     } finally {
+      // releaseLock failure is non-fatal: lock auto-expires on TTL.
+      // If this throws, the outer catch logs "failed" and sets exitCode=1
+      // (misleading — the cron's actual work succeeded). Operators can grep
+      // for "failed to release lock" to distinguish.
       await releaseLock({ name: LOCK_NAME, holder: lockHolder });
     }
   }

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

@@ -71,7 +71,9 @@ describe('distributedLock integration — acquire / release contract:', () => {
     expect(gone).toBeNull();
   });
 
-  test('concurrent acquires: only one pod wins (Promise.all race)', async () => {
+  test('only one of N concurrent in-process acquires wins via E11000', async () => {
+    // Node event loop: 3 Promise.all fires hit Mongo concurrently (not serialized by await).
+    // Real inter-pod race uses the same E11000 path; this exercises the same mechanism.
     const results = await Promise.all([
       acquireLock({ name: 'billing.dunningSweep', ttlMs: 60_000, holder: 'pod-A' }),
       acquireLock({ name: 'billing.dunningSweep', ttlMs: 60_000, holder: 'pod-B' }),