fix/rpu-unique
needs attentionc145f78 · fullPR #321reviewed 2026-07-17 19:03 UTC0H · 2M · 5L · 6I- Purpose
- Enforce the agreements-model invariant — at most one active chapter per RPU (service point) — at the database level, closing the concurrent double-activation race.
- Goal
- Add `uq_utility_contracts_one_active_per_rpu` partial unique index and fix the shell write ordering to comply with the non-deferrable index.
- Sub-goals
- SG-1: Partial unique index (migration 0061) + Drizzle schema declaration
- SG-2: Demote-first / reactivate-second ordering in the ReactivateContract shell path
- SG-3: Narrow `insertContractRaceSafe` adoption to compound-key 23505 only; propagate one-active 23505
- SG-4: Map one-active-per-RPU 23505 to retryable `ContractVersionConflict` (409) at the shell boundary
- SG-5: Remove `decideEnsureContractForTariff` / `ensureContractForTariff` (zero callers, would have violated the index)
- SG-6: Test coverage — 5 new integration tests covering race/OCC scenarios; 2 timeout bumps for staging cross-region latency
- What
- Adds a non-deferrable partial unique index on `utility_contracts(contract_number) WHERE status='active' AND deleted_at IS NULL`. The ReactivateContract branch previously ran activate-new → demote-old, which would have 23505'd itself. Reordered to demote-first / reactivate-second (mirrors T&R's terminate-then-insert). `insertContractRaceSafe` now propagates one-active 23505 rather than masking it. A new `activeChapterRace` error constructor maps the race to a retryable 409.
- Why
- A concurrent double-activation was previously a silent fork of the RPU's present (two active chapters). The DB backstop turns it into a hard 23505 detected and reported as a retryable 409. The write-ordering fix was required to prevent the shell from 23505'ing itself on the reactivation swap.
- Areas
- packages/database+17−0domains/utility/src/utility-contract+331−280domains/cross-domain/src/__tests__+2−2.claude/rules+14−8
- Blast
- 363 adds / 290 dels across 11 files (excl. generated snapshot); domains/utility shells + decisions + errors + tests, packages/database schema + migration, docs.
Findings · 14
correctness3
Demote-first ordering is correct and sound
domains/utility/src/utility-contract/utility-contract.shells.ts:782
The non-deferrable partial unique index requires that two active rows for the same RPU never coexist, even intra-transaction. Demoting before reactivating is the only valid order, mirrors TerminateAndReplace's terminate-then-insert ordering, and is correctly implemented.
`insertContractRaceSafe` correctly scopes adoption to COMPOUND_KEY_CONSTRAINT
domains/utility/src/utility-contract/utility-contract.shells.ts:90
Adopting on a compound-key race is safe (same chapter, concurrent inserts). Propagating a one-active-per-RPU 23505 is correct (different chapter activated concurrently — re-fetching by compound key would return null and mask the conflict). Propagation chain to `activeChapterRace` at the shell boundary is complete.
Outbox demotion event correctly uses pre-update snapshot for `old` status
domains/utility/src/utility-contract/utility-contract.shells.ts:807
`{ old: d.demoteContract.status, new: 'terminated' }` — `d.demoteContract` is the pre-fetch decision snapshot, so `old` is accurate. The post-update row is captured in `demoted` and used only for stable-ID operations (site links, subscriptions). Semantics are correct throughout.
security2
RPU contract number exposed in client-facing 409 error message
domains/utility/src/utility-contract/utility-contract.errors.ts:164
The `activeChapterRace` message embeds `contractNumber` (the RPU, e.g. "123456789") directly in the `message` field and `publicId`. RPU IDs are not credentials (they appear on CFE bills), but verify that the error mapper in `apps/platform` does not forward the raw `message` string to the API response body for untrusted clients.
Removal of `ensureContractForTariff` is a net security improvement
domains/utility/src/utility-contract/utility-contract.shells.ts
The deleted shell permitted creating a second active contract for the same RPU, which the new DB-level constraint now structurally prevents. The combination of DB index + shell removal closes a data-integrity gap that could previously be hit by concurrent callers.
conventions3
activeChapterRace reuses ContractVersionConflictError with sentinel 0s — weakens discriminated-union contract
domains/utility/src/utility-contract/utility-contract.errors.ts:164
`ContractErrors.activeChapterRace` sets `expectedVersion: 0` and `actualVersion: 0` as sentinels with the same `_tag: 'ContractVersionConflict'` as a real OCC miss. Callers that switch on `_tag` cannot distinguish a row-level OCC miss from an RPU-level activation race — they must string-match the message. ADR-016 requires the discriminant to be sufficient. A dedicated `_tag: 'ContractActiveChapterRace'` (new error type, ~10 lines) would restore the invariant and give `publicId` a meaningful type (`rpu: string` instead of abusing the `publicId` field for an RPU string). The PR's own rationale ("no single row owns the conflict") argues for a new type, not for reusing and misrepresenting the existing one.
Removed exports are fully cleaned up
domains/utility/src/utility-contract/index.ts
`decideEnsureContractForTariff`, `ensureContractForTariff`, and all associated types are removed from the barrel and have zero callers remaining in the workspace. A tombstone comment in `decisions.ts` explains the removal rationale. Cleanup is complete.
SQL partial index predicate is injection-safe
packages/database/src/schema/utility-contracts.ts:108
`sql\`${table.status} = 'active' AND ${table.deletedAt} IS NULL\`` uses Drizzle column references, not user input. The emitted DDL is safe. The literal `'active'` is a hardcoded string in the migration, not user-supplied.
tests2
TerminateAndReplace step-3a OCC path not tested at shell/integration level
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts
The shell's T&R branch has two writes: (3a) `updateWithVersion` terminating the old contract, then (3b) `insertContractRaceSafe` inserting the replacement. Tests cover the 3b race (insert trips `uq_utility_contracts_one_active_per_rpu`), but there is no test for 3a losing OCC (the `updateWithVersion` on the old contract returning null because a concurrent writer bumped its version). The shell correctly throws `EnsureContractAbort(statusWriteError(oldContract))` in that case, but transaction atomicity on that specific failure path is unverified. The analogous test exists for the `ReactivateContract` branch (spy on `findActiveByContractNumber`).
All three shell entry points covered for the one-active-per-RPU race
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts
Fresh-create, TerminateAndReplace INSERT, and ReactivateContract UPDATE are all covered by the new mock-based tests. Both OCC orderings for the reactivation swap (demote-loses-OCC and reactivate-loses-OCC-after-demotion) are tested with correct atomicity assertions.
improvement4
Outbox reason string `superseded_by_reactivation_of_${d.contract.publicId}` is ambiguous
domains/utility/src/utility-contract/utility-contract.shells.ts:808
The demotion outbox event for the ReactivateContract path uses `superseded_by_reactivation_of_${d.contract.publicId}`, where `d.contract` is the TERMINATED row being reactivated. To a future engineer reading the outbox, it reads as "this active row was superseded by the reactivation of [some terminated chapter's ID]" — the reverse of the intuitive "the thing in the reason string is what replaced me." Consider `replaced_by_reactivation_of_terminated_chapter_${d.contract.publicId}` or splitting into `reason: 'superseded_by_reactivation'` + `reactivatedChapterId: d.contract.publicId`.
`mkOneActiveViolation` only tests the top-level error shape, not the `.cause?.code` probe branch
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:101
`isUniqueViolation()` in `utility-contract.shells.ts` falls back to `.cause?.code` / `.cause?.constraint_name` (some postgres-js versions wrap the PGError in a `cause`). `mkOneActiveViolation()` sets `code` and `constraint_name` directly on the Error but does not set a `.cause` chain. A second variant — `Object.assign(new Error(...), { cause: { code: '23505', constraint_name: ONE_ACTIVE_PER_RPU_CONSTRAINT } })` — would cover the other branch and guard against a driver upgrade that moves to cause-only.
DB backstop test re-implements `isUniqueViolation` extraction logic instead of sharing it
domains/utility/src/utility-contract/__tests__/ensure-contract-evidence.integration.test.ts:591
The test manually extracts `code` and `constraint_name` from the thrown error using the same dual-path (top-level vs `.cause`) logic already in `isUniqueViolation`. If the shell's probe logic changes (new driver nesting), they will drift silently. A file-local `isConstraintViolation(e, name)` helper used in both spots would keep them in sync.
`COMPOUND_KEY_CONSTRAINT` / `ONE_ACTIVE_PER_RPU_CONSTRAINT` lack `@internal` annotation
domains/utility/src/utility-contract/utility-contract.shells.ts:48
Both constants are exported from the shells module (for test visibility) but omitted from the barrel `index.ts`. Without an `@internal` annotation, future contributors may barrel-export them, broadening the public API surface unintentionally. Add `/** @internal — test-only; import directly from utility-contract.shells */` to each.