Repository navigation
fix(cloud): bound and yield the relay's global cell-inventory lock - #18521
Conversation
Mirrors stablyai/orca-cloud#471 (squash c3354e8), byte-identical under cloud/. The relay's cell-inventory lock (SELECT ... FROM relay_cells FOR UPDATE over all 23 rows) is one global critical section shared by the assignment hot path and every director sweep; with the pool's 1s lock_timeout a blocked waiter held a pooled client for a full second, producing ~690 55P03 retries per 5 minutes in production. Request paths now bound the wait at 500ms with a SET LOCAL that is restored to the pool default before the next statement; director-only sweeps take the lock NOWAIT and skip the tick; sweep timers are jittered; hold time is exported as additive runtime-metrics fields so the bound can be tuned.
📝 WalkthroughWalkthroughThe relay now uses explicit cell-inventory lock modes for request and sweep paths. PostgreSQL and SQLite record measured inventory-lock hold durations and expose aggregate metrics. Background sweeps skip contended inventory locks and emit summarized warnings. Assignment cleanup and regional rehome intervals use positive jitter. Tests cover lock routing, timeout restoration, hold metrics, sweep behavior, schedule jitter, and incident evaluation for exhausted PostgreSQL retries. Merge Risk: 🔵 Low · up to Contended activity-expiry cleanup ticks can appear idle rather than busy, reducing operational visibility into deferred cleanup. Add the existing sweep contention summary instrumentation before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description gives detailed context, explains the changes and rationale, documents rollout requirements, and reports extensive verification. It does not follow the template headings and does not provide a concrete linked issue, but the core required information is present.
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 72089bd7-378a-41be-a164-4263ff5bb171
📒 Files selected for processing (13)
cloud/apps/relay-ops/src/incident-monitor.test.tscloud/apps/relay-ops/src/incident-monitor.tscloud/apps/relay/src/assignment-store.tscloud/apps/relay/src/cell-inventory-hold-samples.tscloud/apps/relay/src/cell-inventory-lock-census.test.tscloud/apps/relay/src/cell-inventory-lock-contention.test.tscloud/apps/relay/src/database.tscloud/apps/relay/src/index.tscloud/apps/relay/src/regional-rehome-store.test.tscloud/apps/relay/src/regional-rehome-worker.tscloud/apps/relay/src/relay-observability.tscloud/apps/relay/src/relay-sweep-schedule.test.tscloud/apps/relay/src/relay-sweep-schedule.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| const lease = activityLeaseById(activityLeases, text(candidate, 'activity_id')) | ||
| if (!lease || integer(lease, 'expires_at') > now) return false | ||
| await this.lockCellInventory(transaction, true) | ||
| await this.lockCellInventory(transaction, 'nowait') |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Track cell-inventory contention in both activity-expiry sweeps.
lockCellInventory does not log database_lock_unavailable. Increment a dedicated counter when each sweep skips because this lock is busy, then call warnSweepCellInventoryBusy after processing. In releaseExpiredActivity, keep assignment-row lock failures separate from cell-inventory failures.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Bounded, per-transaction cell-inventory wait —
RelayLockOptions.lockTimeoutMsissuesSET LOCAL lock_timeoutbefore the locked select and restoresPOSTGRES_LOCK_TIMEOUT_MSin afinally, binding the request path at 500ms instead of the pool's 1s whileSET LOCALstill cannot leak into later statements in the transaction. - Three-way lock-mode classification — every
lockCellInventory/lockGeneralCellInventorysite now statesrequest/nowait/pool-default;assignthreads the caller's mode soevacuateDeadCellsre-enters placement unbounded. - Sweeps yield instead of queueing — six sweep sites take the inventory
NOWAIT, skip the tick, and emit at most oneorca_relay_sweep_cell_inventory_busysummary per tick, keepingdatabase_lock_unavailableoff the retry counters. - Hold-time telemetry — a bounded reservoir (
CellInventoryHoldSamples) reportscellInventoryHoldMsMax/P95/Holdsto runtime metrics so the 500ms bound can be tuned against real data. - Sweep-schedule jitter —
jitteredSweepIntervalMslengthens the 30s cleanup and 1s rehome periods per process so co-booted directors stop piling onto the lock on the same second. - Source-ratchet census —
cell-inventory-lock-census.test.tsscrapes every call site from source and derives sweep-reachability from the call graph, failing on any new, reclassified, or bounded sweep-reachable site.
I traced the classification by hand against the store: every sweep-reachable site (assign/assignOnce/assignStickyOnce, completeEvacuation, leastLoadedCell, reconcileReservationAccounting, and the rehome sweeps) lands on nowait or pool-default, never the bounded wait, so the relayPostgresRetryExhausted = 0 gate cannot be tripped by a sweep under contention. The SET LOCAL restore-on-error path, the invalid_lock_timeout validation, and the NOWAIT-catch accounting (per-candidate inventoryBusy, the candidatesTotal - candidatesFinished report, and the swallowed-but-cleared pendingRegionalRehomeDisableLog) are all consistent. The new tests assert exact behavior (SQL statement order, lock-mode choices, warning payloads) rather than tautologies.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
… a measured bar (#18569) * fix(cloud): recalibrate the relay monitor's exhausted-retry freeze to a measured bar The pre-drain dry-run froze at minute one on relayPostgresRetryExhausted: 0 in every run since #18521 reached the director, blocking the cell roll that carries the same fix. #18521 made contended request-path waiters fail fast (500 ms) instead of succeeding slowly, so exhaustion is now a steady contention rate: 236/236 five-minute windows non-zero over 23 h; post-#18521 p50 42 / p90 147 / max 220 fleet-wide; the 2026-08-23 incident peaked at 467. 300 clears every measured healthy window and stays under the incident shape. /v1/assign 503 share was unchanged by #18521 (13.9% vs 12.3%). * test(cloud): pin the exhausted-retry freeze boundary at exactly 300 * docs(cloud): reword relay comments that still described the zero exhausted-retry bar
…a measured bar (#18580) The global relay_cells FOR UPDATE lock made successful retries a steady-state rate: fleet-wide p50 430 / p90 924 / p99 1320 / max 1504 per five minutes over the last 24 h, 55% of windows over the 300 bar, only 22% of 15-minute gates clean. Three read-only dry-runs on 2026-09-04 froze on it, blocking the same-cap roll that carries #18521 and the beginProof crash guard to the 23 cells. 2000 clears every measured healthy gate; the exhausted-retry, director concurrency, and pool bars keep the incident discriminator role.
…tablyai#18521) Mirrors stablyai/orca-cloud#471 (squash c3354e8), byte-identical under cloud/. The relay's cell-inventory lock (SELECT ... FROM relay_cells FOR UPDATE over all 23 rows) is one global critical section shared by the assignment hot path and every director sweep; with the pool's 1s lock_timeout a blocked waiter held a pooled client for a full second, producing ~690 55P03 retries per 5 minutes in production. Request paths now bound the wait at 500ms with a SET LOCAL that is restored to the pool default before the next statement; director-only sweeps take the lock NOWAIT and skip the tick; sweep timers are jittered; hold time is exported as additive runtime-metrics fields so the bound can be tuned.
… a measured bar (stablyai#18569) * fix(cloud): recalibrate the relay monitor's exhausted-retry freeze to a measured bar The pre-drain dry-run froze at minute one on relayPostgresRetryExhausted: 0 in every run since stablyai#18521 reached the director, blocking the cell roll that carries the same fix. stablyai#18521 made contended request-path waiters fail fast (500 ms) instead of succeeding slowly, so exhaustion is now a steady contention rate: 236/236 five-minute windows non-zero over 23 h; post-stablyai#18521 p50 42 / p90 147 / max 220 fleet-wide; the 2026-08-23 incident peaked at 467. 300 clears every measured healthy window and stays under the incident shape. /v1/assign 503 share was unchanged by stablyai#18521 (13.9% vs 12.3%). * test(cloud): pin the exhausted-retry freeze boundary at exactly 300 * docs(cloud): reword relay comments that still described the zero exhausted-retry bar
…a measured bar (stablyai#18580) The global relay_cells FOR UPDATE lock made successful retries a steady-state rate: fleet-wide p50 430 / p90 924 / p99 1320 / max 1504 per five minutes over the last 24 h, 55% of windows over the 300 bar, only 22% of 15-minute gates clean. Three read-only dry-runs on 2026-09-04 froze on it, blocking the same-cap roll that carries stablyai#18521 and the beginProof crash guard to the 23 cells. 2000 clears every measured healthy gate; the exhausted-retry, director concurrency, and pool bars keep the incident discriminator role.
Mirrors stablyai/orca-cloud#471 (byte-identical under
cloud/; verified per file against private main). Private PR body follows.The contention
lockCellInventoryrunsSELECT * FROM relay_cells ORDER BY cell_id ASC FOR UPDATE:one global lock over a 23-row table, taken from 31 call sites including the
assignment hot path and the director sweeps. Five director instances plus 23
cells serialise on it against a single Cloud SQL primary.
The pool sets
lock_timeout = 1000ms, so a blocked waiter also holds a pooledclient for a full second. Lock contention becomes pool exhaustion.
orca_relay_postgres_transaction_retryorca_relay_postgres_transaction_exhaustedcell-inventoryphaseZero deadlocks with all exhaustion in one phase is the signature of a queue, not
a cycle.
Bounded wait, scoped to one statement
RelayLockOptionsgainslockTimeoutMs. The Postgres transaction path issuesSET LOCAL lock_timeoutimmediately before the locked select and restoresPOSTGRES_LOCK_TIMEOUT_MSin afinally, on the error path too.The restore is load-bearing.
SET LOCALlasts to COMMIT, so without it the boundwould govern every later locked statement in the transaction and misattribute
their
55P03s.rebalanceDormant,assignOnce,assignStickyOnceandactivateControlall take further locks after the inventory.The bound is
CELL_INVENTORY_LOCK_TIMEOUT_MS = 500. Worst-case request queueingbecomes about 1.5s across the wrapper's three attempts, down from about 3s. An
invalid bound throws rather than reaching SQL.
SET LOCALis skipped for NOWAIT,skipped in autocommit where it cannot survive, and absent on SQLite.
Three-way classification, enforced by a census
The same lock is taken by live requests and by background sweeps, and the right
failure differs.
failIfUnavailableis replaced by an explicit mode, so each ofthe 31 sites states its intent:
requestnowaitpool-defaultlock_timeoutassign, per entry pointpool-defaultexists because a sweep that fails sooner turns ordinary contentioninto a terminal transaction failure, and one of those freezes the incident gate.
evacuateDeadCellsre-enters placement, soassignthreads the caller's mode:clients keep the bound, that sweep does not.
cell-inventory-lock-census.test.tsscrapes every call site from source, derivessweep reachability by walking the call graph from the ten cleanup steps plus
claimRegionalRehomeandrecordRegionalRehomeDispatchFailure, and fails if asite is new, reclassified, unclassified, or sweep-reachable on the bounded wait.
Sweeps yield instead of queueing
Six sweep sites take the inventory NOWAIT: regional rehome claim, fleet safety,
candidate start, completion, expiry abort, and expired-evacuation abort. Each
skips quietly and logs at most one
orca_relay_sweep_cell_inventory_busysummaryper tick, reporting how many candidates the tick abandoned. Contention is never a
sweep failure and never quarantines a rehome attempt. A NOWAIT skip raises
database_lock_unavailable, which is not a retryable55P03, so sweeps also stopspending the transaction retry budget.
Director sweep periods are now jittered per process. Instances boot from one
rollout, so their 30s cleanup and 1s rehome ticks land on the same wall-clock
second. The offset only ever lengthens a period.
Hold-time measurement
500ms is a first value, not a measurement, and nothing measured the hold before.
The lock is held to COMMIT and the assignment path runs many statements after
taking it. Both dialects now time each inventory lock from acquisition to COMMIT
and report
cellInventoryHoldMsMax,cellInventoryHoldMsP95andcellInventoryHoldsas additive runtime-metrics fields, so the bound can be tunedafter the first director deploy.
Monitor
relayPostgresRetries: 300is unchanged; it is correctly detecting thisregression.
relayPostgresRetryExhaustedstays at 0, with a comment namingexactly which callers can still reach the retry wrapper. Splitting the counter by
the phase label PR #423 put on the log payload would need a labelled log-based
metric, which this signal does not carry, and that is a terraform change this PR
is out of scope for.
Rollout
This ships inside the relay image, so it needs a director blue/green and a cell
roll to take full effect. The pieces are safe partially deployed: the bound is
per-transaction, and the NOWAIT sweeps only ever do less work than before.
Verification
pnpm --filter @orca-cloud/relay test— 462 passed, 80 skippedpnpm --filter @orca-cloud/relay-ops test— 76 passedtypecheck, which is also this repo's lint, clean for both packages--no-file-parallelism: these suites are flaky onorigin/mainitself, so both sides were measured. Warmed, three consecutive full runs each:
origin/main10, 8, 10 failures of 510; this branch 10, 8, 8 of 540. Samefailure count on 30 more tests, same clusters, differing sets run to run.
of the pending rehome disable log above its rethrow, observable only through a
later tick reading private state.