fix/rpu-race-err

needs attention

183452f · fullPR #323reviewed 2026-07-17 19:38 UTC0H · 1M · 3L · 1I

The branch

Purpose

Follow-up to #321 (one-active-per-RPU partial unique index) — fix an ADR-016 discriminant violation and add missing OCC test coverage.

Goal

Give the concurrent activation race its own `ContractActiveChapterRace` error type so callers can discriminate it from an optimistic-lock miss without message-string parsing, and cover the T&R step-3a OCC path with an integration test.

Sub-goals

SG-1 (M-1): Introduce dedicated ContractActiveChapterRaceError with _tag 'ContractActiveChapterRace' and rpu field; remove sentinel zeros from ContractVersionConflict reuse
SG-2 (M-2): Add T&R step-3a OCC test — stale version on findActiveByContractNumber forces 0-row termination, whole tx rolls back, no stray events or replacements
SG-3 (Lows): Driver-compatibility test for .cause-nested 23505; shared extractPgViolation helper; @internal JSDoc on test-only exported constants

The changes (whole branch)

What: New `ContractActiveChapterRaceError` type (errors.ts); updated `activeChapterRace` constructor; type added to `EnsureContractFromBillsError` union and barrel export; @internal JSDoc on COMPOUND_KEY_CONSTRAINT + ONE_ACTIVE_PER_RPU_CONSTRAINT; four new/updated integration tests.
Why: ADR-016 requires discriminants to be sufficient without message-string parsing. #321 reused ContractVersionConflict with sentinel expectedVersion:0/actualVersion:0 for the RPU-level race, which made the two error cases indistinguishable at the type level.
Areas: domains/utility/src/utility-contract/utility-contract.errors.ts+23−10domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts+107−8domains/utility/src/utility-contract/utility-contract.shells.ts+10−2domains/utility/src/index.ts+1−0
Blast: 4 files, +141/-20 lines. All changes in domains/utility. No happy-path behavior change — error-identity and test-coverage only. Consumer (bill.shells.ts) logs error._tag + .message, no exhaustive switch on EnsureContractFromBillsError in any handler (Lambda pipeline only).

no-behavior-change error-identity-fix adr-016-compliance

➖ci· No CI checks returned by gh pr checks for PR 323➖coderabbit· No .coderabbit.yaml in repo

Findings · 5

correctness1

medium

log.fail emits stale 'ContractVersionConflict' after error renamed

domains/utility/src/utility-contract/utility-contract.shells.ts:544

The catch block logs `log.fail('ContractVersionConflict', { rpu, cause: 'one_active_per_rpu_race' })` but immediately returns `ContractErrors.activeChapterRace(input.rpu)` whose `_tag` is `'ContractActiveChapterRace'`. Any observability tooling (alerts, dashboards, log queries) searching for `ContractActiveChapterRace` will find zero entries; anything searching `ContractVersionConflict` will see phantom hits. Fix: `log.fail('ContractActiveChapterRace', { rpu: input.rpu, cause: 'one_active_per_rpu_race' })`.

security1

low

RPU embedded in error message propagates to Lambda response body / SFN history

domains/utility/src/utility-contract/utility-contract.errors.ts:181

The message `Concurrent activation race for RPU ${rpu}: ...` embeds the meter identifier. It flows through `bill.shells.ts:failed[].message` into the internal `/api/internal/bills-internal/batchPersist` response and into SFN execution history. The endpoint is API-key gated and internal-only, so there is no direct exposure to end users. The RPU is also already on the structured `rpu` field, making the embedding in the human-readable message redundant. Risk is low given the internal boundary, but the duplication widens RPU's presence in logs unnecessarily.

tests1

low

Nested-cause test does not assert rpu field on the error

domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:716

The '.cause-nested' variant test verifies `_tag = 'ContractActiveChapterRace'` but does not narrow and assert `result.error.rpu`. The flat-violation test does check `rpu`. A bug that constructed the error with the wrong RPU would go undetected in this variant. Low because the flat test already pins `rpu` and both paths share the same constructor.

improvement2

low

bill.shells.ts call-site does not propagate the 409/retryable signal

domains/utility/src/bill/bill.shells.ts

The caller of `ensureContractFromBillsShell` maps every `EnsureContractFromBillsError` into `failed[]` uniformly — the retryable 409 semantics of `ContractActiveChapterRaceError` are not surfaced. This is a pre-existing design gap made newly visible by the dedicated discriminant. A follow-up that branches on `_tag === 'ContractActiveChapterRace'` (or logs it at a different severity) would complete the intent of this fix.

info

@internal JSDoc duplicated verbatim on both constraint constants

domains/utility/src/utility-contract/utility-contract.shells.ts:52

Both `COMPOUND_KEY_CONSTRAINT` and `ONE_ACTIVE_PER_RPU_CONSTRAINT` carry the identical @internal boilerplate added by this PR. Consider a single comment block or note for both, to avoid the copy drifting if the barrel policy changes.