The billing job ran twice last month. An agent added a distributed lock, and four more reviewers have already been through the diff.
TypeScriptRedisBilling
BILL-2210THE TICKET
Stop the period-close job running concurrently
On 3 August the nightly close ran on two workers at once during a rolling deploy. No customer was charged twice — the idempotency keys held — but we burned through the provider's rate limit and the on-call spent four hours proving no money moved.
Put a lock around it.
Context for whoever picks this up: the nightly cron calls closeBillingPeriod(periodId) exactly once, and when that promise resolves it marks the period closed and files the revenue report. We bill about 400 accounts a period at the moment, growing somewhere around 15% month over month.
THE AUTHOR’S NOTEagent · claude-sonnet-4.5
Adds a Redis-backed mutex around closeBillingPeriod using SET NX PX with a per-run token, so only one worker can be inside the period close at a time.
The lock is released with a compare-and-delete Lua script rather than a GET followed by a DEL — the two-round-trip version has a window where our lock expires, another worker acquires it, and we delete theirs.
Waiting workers poll rather than using a blocking primitive. At one caller every 24 hours the contention is a rolling deploy, not a workload, and a poll loop is easier to reason about than a subscription.
No change to the charging path.
src/jobs/close-billing-period.ts0 flagged
src/jobs/close-billing-period.ts62 lines
Click a line to flag it. Shift-click to flag a range.
WHAT YOU ARE DOING
The review already happened. Call each comment, then flag anything in the file that nobody said and somebody should have. Both halves are scored; the second one is worth more.
Deadlock risk.acquire() sets the lock with SET NX but never sets an expiry on it. If this worker is OOM-killed or the pod is evicted mid-run, the key stays in Redis forever and every subsequent nightly close blocks on the while loop until someone deletes it by hand. This needs PX on the set, or a TTL set immediately afterwards.
This is the classic distributed-lock footgun and it should not merge without it.
The TTL is shorter than the work it protects.LOCK_TTL_MS is 30 seconds. The loop charges up to BATCH = 500 invoices and the comment on line 44 says the provider rate-limits us to 8 per second, so a full batch cannot finish in under ~62 seconds. The lock expires while the job is still running, a second worker acquires it, and now two workers are inside the section the lock exists to protect.
Either renew the lock on a heartbeat while the loop runs, or set the TTL above the worst-case runtime.
Blocker — this reintroduces the exact concurrency BILL-2210 was filed about.
P. Okaforstaff engineer · owns billingSHOULD FIX
src/jobs/close-billing-period.ts:60
release(token) is only reached on the happy path. If payments.charge or the invoice update throws — and the provider 502s often enough that we have an alert for it — the function exits with the lock still held, and every retry for the next 30 seconds sits in the poll loop.
Not fatal because of the TTL, but the whole body from the acquire down should be in a try { … } finally { await release(token) }.
Duplicate charge risk. The idempotency key is built from invoice.id, which is regenerated each time the invoice row is rewritten. On a retried run the key will differ from the first attempt, the provider will treat it as a new charge, and the customer is billed twice.
Key the charge on something stable — the period and the customer, or a dedicated chargeAttemptId column.
linthouse rules · log-volume ruleNIT
src/jobs/close-billing-period.ts:57
log.info inside the per-invoice loop emits one structured line per charge. At batch size this is up to 500 info lines per run. Consider log.debug, or aggregate and log once at the end.
WHAT NOBODY SAID
You have flagged 0 lines. Explain what is wrong with them and when it fires. If you think the review caught everything, say that instead — it is a legitimate answer and it is scored.