Review a batch invoice settlement job

Review exercise · TypeScript

Review brief

Advanced45 min

A billing service is adding a scheduled settlement worker. Invoice credits can arrive while a batch is running, the payment provider offers idempotent charge requests, and interrupted work must resume without duplicate charges or skipped invoices. Review the change's external-side-effect and recovery semantics.

Behavioral contract

The provider treats one idempotency key as one logical charge: keys must be unique across invoices and stable for one invoice attempt across every retry until its accepted charge is durably recorded.
If the provider accepts a charge but local persistence fails, a retry must reconcile with the same key and must not create another charge.
The due-list total is only a snapshot. Credits may arrive during the run, and `getPayableTotal` returns the authoritative amount that must be read immediately before charging.
Invoices are ordered by ID. The checkpoint may advance only through a contiguous prefix whose accepted payments or terminal declines are durably recorded; it must stop before a transient failure.
A provider `declined` error is terminal and must not retry. `temporarily-unavailable` is retryable, and unknown or local persistence failures must remain recoverable.
`withBatchLock` holds a database transaction and exclusive checkpoint lock throughout its callback. Provider network requests must run outside that lock and outside any database transaction.
The provider rate limit permits sequential charging, and only one scheduled worker is expected to own a batch at a time.

Review instructions

1Review the changed code against the provider, checkpoint, amount, and failure contracts.
2Trace partial progress across worker restarts and distinguish external acceptance from local persistence.
3Prioritize no more than seven findings, including operational or test feedback only when it protects the stated behavior.

Learning objectives

• Design stable idempotency around external side effects
• Review resumable batch checkpoints
• Classify failures according to recovery behavior

src/invoice-settlement-types.tsTypeScript

@@ -0,0 +1,51 @@

1+export interface DueInvoice {

2+ id: string;

3+ accountId: string;

4+ totalCents: number;

5+}

7+export interface ChargeReceipt {

8+ chargeId: string;

9+ acceptedAt: Date;

10+}

11+

12+export interface PaymentProvider {

13+ charge(request: {

14+ accountId: string;

15+ amountCents: number;

16+ idempotencyKey: string;

17+ }): Promise<ChargeReceipt>;

18+}

19+

20+export class PaymentProviderError extends Error {

21+ constructor(

22+ message: string,

23+ readonly kind: "declined" | "temporarily-unavailable",

24+ ) {

25+ super(message);

26+ this.name = "PaymentProviderError";

27+ }

28+}

29+

30+export interface SettlementTransaction {

31+ loadCheckpoint(): Promise<string | null>;

32+ listDueAfter(checkpoint: string | null, limit: number): Promise<DueInvoice[]>;

33+ getPayableTotal(invoiceId: string): Promise<number>;

34+ markPaid(invoiceId: string, receipt: ChargeReceipt): Promise<void>;

35+ recordFailure(invoiceId: string, message: string, retryable: boolean): Promise<void>;

36+ saveCheckpoint(invoiceId: string): Promise<void>;

37+}

38+

39+export interface SettlementDependencies {

40+ ids: { next(): string };

41+ payments: PaymentProvider;

42+ repository: {

43+ withBatchLock<T>(work: (transaction: SettlementTransaction) => Promise<T>): Promise<T>;

44+ };

45+}

46+

47+export interface SettlementResult {

48+ attempted: number;

49+ paid: number;

50+ failed: number;

51+}

Reference solution

The findings below are ordered by priority.

Findings

1. A retry after accepted payment uses a new identity and can charge twice

Location: src/invoice-settlement-job.ts:12,27-42

The idempotency key is generated once at the start of each worker invocation. If the provider accepts a charge and markPaid then fails, the invocation exits without a durable paid record. The next run generates another key, so the provider cannot recognize the retry and may accept a second charge for the same invoice.

Persist a stable settlement-attempt identity before calling the provider and reuse it until the accepted charge is durably reconciled. A retry after an ambiguous persistence failure must send the same invoice-specific key.

2. Distinct invoices share one provider idempotency key

Location: src/invoice-settlement-job.ts:12,23-31

Every invoice in the batch sends batchIdempotencyKey. The provider defines an idempotency key as one logical charge, so later invoices can replay the first receipt or be rejected as conflicting rather than receiving their own charge.

Use a different stable attempt key for each invoice. Stability across retries and uniqueness across invoices are both required.

3. The checkpoint skips invoices whose payment failed

Location: src/invoice-settlement-job.ts:32-37

The catch path records the failure and then advances the ordered checkpoint past that invoice. Because later runs query only invoices after the checkpoint, a transient provider failure is marked retryable but can no longer be selected for recovery.

Advance only across a contiguous prefix of invoices with a durably recorded accepted payment or terminal decline. Stop before a transient failure so the next run resumes from it.

4. Terminal declines and transient provider failures get the same retry decision

Location: src/invoice-settlement-types.ts:20-27 and src/invoice-settlement-job.ts:32-37

The provider exposes declined and temporarily-unavailable, but the catch path ignores that kind and records retryable: true for every error. Terminal declines will be retried, while transient failures need an explicitly recoverable state.

Classify PaymentProviderError.kind: persist declines as terminal outcomes and temporary unavailability as retryable. Preserve or rethrow unknown failures rather than silently assigning them the same recovery policy.

5. The worker charges the stale total from the batch snapshot

Location: src/invoice-settlement-job.ts:15-16,27-30 and src/invoice-settlement-types.ts:30-36

Credits can change an invoice after the due list is loaded. The worker sends invoice.totalCents and never calls the provided getPayableTotal, so it can charge more than the authoritative balance at provider-call time.

Reload the payable total immediately before creating the charge request, after any durable attempt claim, and use that authoritative amount for the attempt.

6. Provider network calls run while the database batch lock is held

Location: src/invoice-settlement-job.ts:14-46

withBatchLock holds its database transaction and exclusive checkpoint lock for the whole callback, including every provider request. Slow provider latency therefore keeps database resources locked and can block unrelated credit or settlement work.

Keep database transactions short: claim durable work and attempt identities, commit, perform provider calls outside the lock, then open a new transaction to reconcile receipts and advance progress safely.

7. The tests model only an all-success run

Location: src/invoice-settlement-job.test.ts:6-44

The provider, persistence methods, and checkpoint all resolve immediately. The assertions check counts but not provider arguments, so the suite misses distinct and stable keys, provider-success/persistence-failure recovery, stale totals, failure classification, and checkpoint behavior after partial failure.

Add interruption and restart tests with captured charge arguments, a credit applied after listing, each provider failure kind, and a persistence failure after an accepted charge.

Reasonable non-findings

A batch size of 100 is a reasonable operational choice; the contract does not prescribe a different limit.
Processing provider calls sequentially is acceptable under the stated provider rate limit. The database lock duration, not the absence of parallelism, is the concern.
The worker's duration and completion telemetry does not affect settlement correctness and handles thrown failures without swallowing them.