Close the §4.6 currency-lock race: shared account-row locks across money-writing handlers (#162)

[mforce]

Closes #162 — the follow-up PR #159 deferred: the §4.6 currency lock now actually serializes against every money write.

Design decision (the issue's open point)

FOR SHARE on the account row, not an advisory lock or a dedicated lock row:

• Money writes take shared locks — they never block each other, so write concurrency is untouched. The only path that pays is the admin-only currency change (FOR UPDATE), which is exactly the path that should pay.
• The "hot row" cost is per-write multixact bookkeeping — noise at this deployment's write rate. If a future load test disagrees, the repository methods are the single swap point for pg_advisory_xact_lock_shared (call sites unchanged).
• It locks the actual guarded resource, visible in the SQL — no hash-collision surface, no invisible cross-feature key space.
• Deadlock analysis: money writers' shared locks are mutually compatible; the only exclusive taker (the settings handler) locks nothing but the account row — no cycle exists regardless of where the shared lock sits in each handler's lock order.

What changed

• IAccountRepository.GetCurrentSharedLockedAsync / GetCurrentLockedAsync: raw SQL with the tenant WHERE inside — composing a locking clause with the global query filter would wrap it in a subquery over ALL accounts and lock every tenant's row.
• Five handlers grow a transaction spanning currency read + insert (CreateExpense, CreateSalesOrder, CreateProduct, UpdateProduct, CreateInventoryItem); two already-transactional ones swap to the locked read (RecordPurchase, UpdateInventoryItem). UpdateProduct additionally fetches the grade mapping before mutating the tracked entity, so no rollback path leaves a dirty entity for the request's later idempotency-record save to flush.
• UpdateFarmSettingsHandler: FOR UPDATE first inside the transaction. Holding it means no writer is mid-flight and none can start, so one probe is authoritative — the second probe and its CurrencyLandedMidFlight error are deleted (no external references; the domain's Account.CurrencyLocked refusal is unchanged). Lock failures (40001/40P01) surface via the existing global PostgresException → 409 mapping — nothing new needed.
• RecordPurchase's no-explicit-cost branch deliberately keeps the item's default cost without a lock: that currency was snapshotted at item creation, not read fresh off the account.

Deliberately NOT in the protocol

RecordPaymentHandler (the issue's checklist lists it): it never reads Account.DefaultCurrencyCode — it stamps order.TotalAmount.CurrencyCode off the order row it already holds FOR UPDATE, and any existing order already refuses the currency change outright. Locking the account row there would guard nothing.

Tests

• CurrencyLockRaceTests (new): both interleavings driven against the real handlers, with one raw transaction holding the opposing lock. Blocking is observed via pg_blocking_pids (row-lock waits park on the holder's transactionid, so the naive pg_locks relation probe misses them) — no timing guesses.
  • Writer first → the settings handler parks, then refuses with Account.CurrencyLocked; farm stays USD, row stays USD.
  • Change first → one theory case per stamping handler (7): each parks, then stamps the NEW currency. Per-handler cases are the point: a handler missing its lock passes every functional suite. Mutant-verified — reverting CreateProduct's lock fails exactly the product-create case, 6/7 still green.
• CurrencyLockSerializationTests kept untouched, as the issue instructs — executable evidence that SERIALIZABLE does not close this.
• TDD: both race directions watched RED against the pre-lock code (settings change raced past the writer; expense stamped the stale currency).

Verification

• .NET: 778 green ×2 (425 integration; 9 new)
• Build: 0 warnings / 0 errors
• No web changes; no user-visible behavior change (the 409 contract is the same one Farm settings — profile (name/logo) + locale/timezone/currency editing + §4.6 currency-change guard (API + SPA) #123 shipped) — GLOSSARY/Help untouched.

[mforce]

Review round outcome (codex + pi + 2 Claude agents)

12 findings across 4 reviewers → 5 accepted (fixed in df97f1e), 7 rejected with evidence. The Claude review agent returned READY with zero blocking findings after independently verifying the six hard questions (probe visibility after the exclusive lock, codebase-wide deadlock orderings, missed stamping paths, rollback/audit semantics, raw-SQL scoping, test isolation).

Accepted → fixed

#	Finding (reviewer)	Fix
1	The per-handler theory only proves the locked READ blocks; a handler whose lock evaporates on autocommit (locked read in its own transaction, insert separate) passes every case (codex #1, important)	New writer-first test with real handlers on both sides: a raw FOR UPDATE fence parks the expense handler at its read and the settings change behind the same row; on release, PG's FIFO queue grants the writer first and the change must wait out the writer's whole transaction, then refuse. Empirically mutant-verified: the exact evaporating-lock mutant fails this test 5/5.
2	Blocking probe could false-positive on unrelated blockers (codex #2 ≈ pi #2)	Probe now keys on the holder's backend pid via pg_blocking_pids(pid) @> ARRAY[holderPid]. Note pi's suggested fix (ILIKE '%accountId%') cannot work: the handlers' lock queries are parameterized, so the GUID never appears in pg_stat_activity.query.
3	Raw currency-change fixture didn't bump Version like the real mutation (codex #3)	"Version" = "Version" + 1 added.
4	"Only the currency change conflicts" is too narrow — FOR SHARE also conflicts with plain account UPDATEs (FOR NO KEY UPDATE), so a rename save can briefly wait behind money writes (codex #4)	Documented accurately in IAccountRepository; accepted behavior (rare, admin-only, transaction-length waits).
5	The discarded GetCurrentLockedAsync result + "tracked" doc wording could mislead (cavecrew #1's kernel, pi #1's kernel, Claude-agent advisory)	Comments added at both sites: the call exists for the lock; EF identity resolution returns the SAME instance without refreshing values, so capturing the result changes nothing; Version's concurrency token (AccountConfiguration.cs:39) is what guards stale writes (→ DbUpdateConcurrencyException → global 409).

Rejected — with evidence

• "🔴 GetCurrentLockedAsync result discarded → stale snapshot bug" (cavecrew Scaffold .NET solution #1 as stated): not a bug. The suggested fix (capture the result) is a no-op — identity resolution hands back the same tracked instance with unchanged values. Staleness in a settings-vs-settings race degrades to the concurrency-token 409, exactly as before this PR. The Claude agent traced the same path independently and scored it non-blocking.
• "DiscardChanges rollback hazard" (cavecrew Fix egg-farm spec gaps: hen-day math, egg unit conversion, regrade path #2): that line is the mitigation, shipped and reviewed in Farm settings: editable localization block with the §4.6 currency lock (#123) #159; DbContext is request-scoped and State = Unchanged prevents any later flush.
• "GetCurrentLockedAsync doc says tracked but query is AsNoTracking" (pi Scaffold .NET solution #1): factually wrong — AsNoTracking is on the shared variant only; the exclusive one is tracked. (The misleading-comment kernel was addressed in accepted [A1] Seed default farm/house/user + startup migrate + flock CRUD #5.)
• "Document the lock-acquisition order" / advisory-lock alternative (pi Bump to v4.4: README, CHANGELOG, and spec version line #3, INFO): the handler comments already carry the no-cycle rationale; the Claude agent verified no cycle across every lock pair in the codebase (item→account, order→lots, order-only, settings account-only).
• 429/Serializable re-attempt class — nothing new; CurrencyLockSerializationTests kept untouched per the issue's instruction.

Verification after fixes

• .NET: 779 green ×2 (426 integration; 14 currency-lock tests: 4 kept SERIALIZABLE-evidence + writer-first raw + writer-first real-handlers + 7-handler theory + change-first)
• Mutant probes: dropping one handler's FOR SHARE fails exactly its theory case; the evaporating-lock mutant fails the new writer-first test 5/5
• Build: 0 warnings / 0 errors