feat: bound concurrent postings with an optional semaphore - #776
feat: bound concurrent postings with an optional semaphore#776nicolasburtey wants to merge 1 commit into
Conversation
Add CalaLedgerConfig::max_concurrent_postings. When set, CalaLedger acquires a permit from a tokio Semaphore before posting a transaction, so excess posters queue in-process instead of piling up on the balance-poster advisory locks (find_for_update) inside Postgres, where every waiter pins a connection, an open transaction and an MVCC snapshot. Both entry points are gated exactly once: post_transaction acquires before opening its DbOp and delegates to an ungated inner fn, while post_transaction_in_op acquires per call. A caller composing several posts into one op therefore never holds more than one permit and cannot self-deadlock. The acquisition runs in its own cala_ledger.posting_permit.acquire span so queue wait is visible in traces. Default is None, preserving the previous unbounded behavior.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e8a3b37. Configure here.
| // than one permit and cannot self-deadlock. | ||
| let _permit = self.acquire_posting_permit().await; | ||
| self.post_transaction_in_op_ungated(db, tx_id, tx_template_code, params) | ||
| .await |
There was a problem hiding this comment.
Composed posts can deadlock
High Severity
The posting_permits semaphore is acquired after a database connection is taken and released before the transaction commits. This timing mismatch means the semaphore doesn't prevent connections from being held by waiting tasks and can lead to deadlocks where tasks holding permits block on advisory locks held by other tasks waiting for permits.
Reviewed by Cursor Bugbot for commit e8a3b37. Configure here.
| let account_sets = AccountSets::new(&pool, &publisher, &accounts, &balances, &clock); | ||
| let posting_permits = config | ||
| .max_concurrent_postings | ||
| .map(|n| std::sync::Arc::new(tokio::sync::Semaphore::new(n))); |
There was a problem hiding this comment.
Zero bound hangs all postings
Medium Severity
Configuring max_concurrent_postings to 0 creates a semaphore with zero permits without validation. This causes acquire_posting_permit to block indefinitely, silently freezing all transaction posting instead of failing fast. This can happen if 0 is mistaken for unbounded concurrency.
Reviewed by Cursor Bugbot for commit e8a3b37. Configure here.
| tx_template_code: &str, | ||
| params: impl Into<Params> + std::fmt::Debug, | ||
| ) -> Result<Transaction, LedgerError> { | ||
| let _permit = self.acquire_posting_permit().await; |
There was a problem hiding this comment.
Pool and permit can deadlock
High Severity
The new posting_permits can lead to a circular-wait deadlock. post_transaction acquires a permit before a database connection, while post_transaction_in_op (when used with begin_operation) acquires a connection first. This inverted resource acquisition order, under a bounded semaphore and saturated connection pool, causes posting operations to stall or time out.
Reviewed by Cursor Bugbot for commit e8a3b37. Configure here.
📊 Performance ReportCommit: e8a3b37 Cala Performance Benchmark Results (non-representative)Criterion Benchmark Results (single-threaded)
Load Testing Results (parallel-execution)
Note: Performance results may vary based on system resources and database state. Last updated by commit e8a3b37 |
…ies to account sets (closes #802) (#813) * feat(account): expose eventually_consistent on NewAccount + rollup EC plain accounts (closes #802) Let a *plain* account opt into eventually-consistent balance maintenance, so posting to it takes no synchronous balance lock and no inline balance write — its balance is maintained by the streaming rollup job (#811), exactly as EC *set* balances already are. This unblocks moving product omnibus accounts off the poster lock (~95% of DB time; cala #776). - account: add public NewAccountBuilder::balance_rollup(BalanceRollup) setter mapping to the existing eventually_consistent flag; default Synchronous, so the change is additive (mirrors NewAccountSet). - balance: fold EC-leaf deltas in the streaming rollup applier. New BalanceRepo::fetch_ec_leaf_accounts classifies which posted accounts are EC plain leaves (eventually_consistent AND not backing a set, via EXISTS on cala_account_sets — there is no is_account_set column). Snapshots::from_ec_entries and the effective-balance applier now fold each entry into the EC leaf itself in addition to its EC ancestor sets. - ledger: #802 guard. Reject direct entries to an EC set-backing account in post_transaction_in_op (the sole entry-creation path) with the new LedgerError::EntriesTargetEventuallyConsistentAccountSet — such a set's balance is derived from members, so a direct entry would be folded nowhere and silently vanish. Scoped to set-backing accounts; entries to EC plain leaves are the whole point and remain allowed. Locked-EC status handling is intentionally out of scope (deferred; tracked in the perf-review roadmap). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(entry)!: forbid entries to any account set via composite FK, not a post-time query Replace the post-time round-trip guard (which only covered EC set-backing accounts and cost an extra SELECT per posting) with a structural guarantee baked into the schema: - Materialize cala_accounts.is_account_set as a real column (populated from NewAccount at create; already present on the entity as AccountConfig). - Add a constant-FALSE discriminator on cala_entries and a composite FK to cala_accounts(id, is_account_set), so an entry can only reference a non-set account. Posting to any account-set backing account -- EC or synchronous -- now fails at the DB with no extra query on the hot path. Folded into the setup migration in place (breaking: requires a fresh schema). - Surface the FK violation as the typed LedgerError::EntryTargetsAccountSet. - Drop fetch_ec_set_backing_accounts / ec_set_backing_accounts_in_op and the ledger pre-check; simplify fetch_ec_leaf_accounts to is_account_set = FALSE. Broadens #802: a direct entry to a set-backing account was previously only rejected for EC sets (and silently folded for synchronous sets); it is now rejected for all sets -- an account set's balance is always derived from its members. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(migrations): roll obix outbox partitioning into cala_obix_setup (obix 0.6.0) Fold the standalone range-partition migration (#809) into cala_obix_setup so the outbox is created in the obix 0.6.0 partitioned layout directly, and align the file with the canonical obix setup: - cala_persistent_outbox_events is RANGE-partitioned by `sequence` (PK on sequence, BIGSERIAL id-less), with an initial 2M-width _p0 partition and a DEFAULT backstop; the obix partition maintainer extends it ahead of head. Drops the now-redundant seen_at column and the standalone partition migration. - Adopt the security-hardened ephemeral notification: the pg_notify payload carries only {event_type, recorded_at}; listeners fetch the payload from the table with their own credentials (LISTEN/NOTIFY is unauthenticated). Breaking: consolidated into the setup migration, so it applies on a fresh schema (matches this branch's fresh-schema stance). Query end-state is unchanged, so no .sqlx regeneration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(deps): bump es-entity 0.12.0, job 0.7.0, obix 0.7.0 Coordinated upgrade to the latest GaloyMoney crate releases. es-entity 0.12 regenerates its derived SQL (now normalized via sqlparser), so the offline .sqlx query cache is rebuilt accordingly; obix 0.7.0 is the runtime matching the partitioned outbox + hardened ephemeral-notify layout already in cala_obix_setup. No cala source changes were required. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(entry): keep the plain account_id FK so a missing account isn't misreported as an account set Cursor Bugbot (PR #813): dropping the original single-column cala_entries.account_id -> cala_accounts(id) FK meant a *nonexistent* account_id also violated the composite set-guard FK, and is_entry_account_set_violation mapped that to EntryTargetsAccountSet -- so posting to a missing account was reported as "posting to an account set". Restore the plain existence FK, declared *before* the set guard so its RI trigger fires first: a missing account trips cala_entries_account_id_fkey (a generic FK error), while an account that exists but is a set trips cala_entries_account_not_account_set_fkey (EntryTargetsAccountSet). Verified in Postgres: missing -> plain FK, set -> composite FK, plain -> ok. Adds a regression test posting to a never-created account and asserting the error is not EntryTargetsAccountSet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(account_set): membership guard also gates on posted entries so EC leaves can't bypass it Cursor Bugbot (PR #813, High): allowing plain accounts to be EventuallyConsistent broke the account-set membership invariant. Attach/detach gated only on cala_balance_history, but an EC leaf writes no history until the streaming rollup runs -- so a leaf could post, then still join or leave a set, and the rollup (resolving mappings at apply time) would fold those entries into the wrong sets (or skip a needed unfold), corrupting set balances. Fix: the guard (member_has_balance_history_in_op and its batch variant) now returns true when the member has a cala_balance_history row OR a posted cala_entries row. An EC leaf writes its entries synchronously in the posting transaction, under the same SHARED member lock that fences the guard's EXCLUSIVE lock -- so checking entries closes the window and brings EC leaves to parity with synchronous accounts (which write both history and entries). Account sets carry no entries (the #802 FK forbids it), so the history check still covers set members. Adds a regression test: post to an EC leaf (rollup not running, so no history), then attaching it to a set must fail with MemberHasBalanceHistory. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>


Why
Under sustained posting load (lana stress test, 2 loans/s target), the balance-poster lock prelude in
BalanceRepo::find_for_updatewas the #1 DB-time consumer by far: ~95% of all DB time, windowed mean 683ms at hour 1 rising to 976ms at hour 4, with 100% buffer hits — i.e. pure advisory-lock wait. Every hot shared account (omnibus, top-of-chart sets) serializes postings onpg_advisory_xact_lock(journal|account|currency), and when arrival rate exceeds the serialized service rate, the queue forms inside Postgres, where each waiter pins:That pileup is what turned a throughput ceiling into a stability incident (liveness-probe kill ~20 min into the soak).
What
New
CalaLedgerConfig::max_concurrent_postings: Option<usize>(defaultNone= unchanged behavior). When set,CalaLedgerholds atokio::sync::Semaphoreand every posting acquires a permit first, moving the queue in-process: waiters hold no connection, no snapshot, and their wait shows up in traces under thecala_ledger.posting_permit.acquirespan.Gating covers both entry points exactly once:
post_transactionacquires before creating itsDbOp(waiters don't even hold a connection), then delegates to an ungated inner fn.post_transaction_in_opacquires per call; a caller composing several posts into one op acquires/releases one permit at a time, so no self-deadlock (covered by the new test withmax_concurrent_postings(1)).Explicitly not claimed: this does not raise the serialization ceiling of a hot account (that's the
eventually_consistentescape hatch + shorter hold times). It keeps the system stable at the ceiling instead of collapsing below it, and makes saturation observable.Test
transaction_post_with_bounded_concurrency: limit 1, single post, two sequential posts in one composed op, then 4 concurrent posters; asserts final balance. Would deadlock if any path double-acquired.cala-ledgersuite: 95/95 pass.Follow-ups (not in this PR)
LANA__LEDGER__MAX_CONCURRENT_POSTINGS(or similar) through lana-app once a cala release includes this.find_for_updatelock query to drop from ~1s to ~raw hold time, no liveness events, achieved loans/s flat across the 4h soak.Note
Medium Risk
Touches the core posting path and can throttle throughput when the limit is set too low, but default is off and behavior is opt-in with explicit backpressure intent.
Overview
Adds
CalaLedgerConfig::max_concurrent_postings(Option<usize>, defaultNonefor unchanged behavior). When set,CalaLedgerkeeps atokio::sync::Semaphoreand acquires a permit before posting work touches the DB.post_transactionwaits for a permit before opening aDbOp, then runs the existing logic via a newpost_transaction_in_op_ungatedhelper so the outer path does not double-acquire.post_transaction_in_opacquires one permit per call (release between sequential posts in a composed op) to avoid self-deadlock.Excess posters queue in-process instead of stacking on Postgres advisory-lock wait with pinned connections and transactions; permit wait is visible under
cala_ledger.posting_permit.acquire.Adds
transaction_post_with_bounded_concurrency(max_concurrent_postings(1): standalone post, two posts in one op, four concurrent posters, balance assertion).Reviewed by Cursor Bugbot for commit e8a3b37. Bugbot is set up for automated code reviews on this repo. Configure here.