[Jinwoo-H]

	Files	Added	Deleted	Net
Test	5	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​452	$\color{#cf222e}{\Huge{\mathbf{−}}}$​8	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​444
Prod	2	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​119	$\color{#cf222e}{\Huge{\mathbf{−}}}$​29	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​90

Why

postgres_retries on the director is ~100% 55P03 against one self-imposed critical
section: SELECT * FROM relay_cells ORDER BY cell_id ASC FOR UPDATE over a 23-row table,
taken by the assignment hot path and by director sweeps alike. #18521 bounded the wait to
500 ms and #18606 took per-connection paths off it, but two paths that mutate one or two
cells still took all 23:

• the sticky refresh retry. A returning host whose first NOWAIT probe of its own cell
  row loses retries by taking a cell row before the assignment row (the order that avoids
  the legacy cell-first cycle). That retry took the whole inventory, so a rebind anywhere in
  the fleet stalled every other cell's reconnects.
• reconcileReservationAccounting. It repairs exactly two cells' counters — source and
  target of a migration — and locked the other 21 to do it. It runs on nine migration paths.

Separately, statement_timeout was hardcoded, DDL ran on the same deadline-bearing pool as
requests, and 57014 was not in retryablePostgresTransactionError, so a statement
deadline reached the caller as a terminal failure rather than a retry.

Before / after locking model

path	before	after
sticky refresh retry	relay_cells ORDER BY cell_id ASC FOR UPDATE (all 23)	unlocked pin read, then FOR UPDATE on that one row
reservation reconciliation	all 23 FOR UPDATE	FOR UPDATE on {source, target}; unlocked SELECT cell_id for the existence check
leastLoadedCell	optional lockedCells, else a second fleet-wide lock	rows is required; the fallback is gone
sticky grant counter	SET reserved_requests = <snapshot + 1>	adjustCellReservationAtomically (reserved_requests + $1 with the capacity guard in the same statement)
placement	ordered inventory lock	unchanged, deliberately

Placement keeps the narrow global lock. Choosing the least-loaded cell is a genuinely
fleet-wide decision, and the existing comment records that dynamically locking only the
selected target is what allowed cross-cell cycles. Row order is still ascending cell_id
everywhere (lockCellRows keeps its ORDER BY), so the per-cell locks cannot cycle with it.

lockCellRows now takes the caller's CellInventoryLockMode instead of hardcoding the
500 ms request bound, so a sweep-reachable site does not turn ordinary contention into a
terminal 55P03. It still does not sample hold time: cellInventoryHoldMs* tracks the
fleet-wide lock these rows replace, and mixing in short single-row holds would flatter it.

Invariants preserved, and how each is tested

• Lock order (cell before assignment on the retry). postgres-transaction-recovery.test.ts
  now traces assignment, activity, cell, cell-rows, assignment, activity against a legacy
  cell-first holder. The pin read is classified in the same hook but must not appear —
  only locked statements reach it, so its absence is the proof it stays unlocked.
• A held cell row blocks only that cell. New cell-inventory-per-cell-locking-postgres.test.ts
  pins the host to the last cell id, holds that row, and NOWAIT-probes the two earlier
  rows while the refresh waits. Mutation-checked: restoring lockCellInventory there fails
  with ["percell-postgres-a", "percell-postgres-b"].
• Counters stay exact under concurrency. Six hosts across three connections acquire and
  release splices on one cell; reserved_requests lands on 18 then back to 6.
• Capacity / headroom / supersession / admission selector. Unchanged code paths; the
  full suite (591 tests) passes against real PostgreSQL, including the connection-headroom,
  control-supersession, migration-recovery and control-lease-recovery suites.
• Census. cell-inventory-lock-census.test.ts still requires every lockCellInventory
  site to be classified and every raw relay_cells lock to be routed through a named
  helper, so re-adding a fleet-wide lock at any of the three removed sites fails the test.
• Statement deadline is retryable, not terminal. Unit test with a faked 57014 and a
  real-PostgreSQL test that drives pg_sleep(2) under a 300 ms deadline; both die when
  57014 is removed from retryablePostgresTransactionError.
• DDL is untimed. Unit test pins the schema pool's shape (max: 1,
  statement_timeout: 0) and the lifecycle order open schema → end schema → open serving,
  and asserts only CREATE … statements run before the serving pool exists. A
  real-PostgreSQL test holds ACCESS EXCLUSIVE on relay_invites (whose
  CREATE INDEX IF NOT EXISTS genuinely queues, unlike CREATE TABLE IF NOT EXISTS) and
  confirms the blocked backend is the …/schema connection and that the open still
  completes with a 200 ms serving deadline.

The timeout value and its derivation

Default stays 5 s, now overridable with ORCA_RELAY_POSTGRES_STATEMENT_TIMEOUT_MS.
A control renewal must land inside its own tick, which is
RELAY_PROTOCOL_LIMITS.controlPingIntervalMs * 2 = 30 s. A transaction gets
POSTGRES_TRANSACTION_ATTEMPTS = 3 tries, so the worst case a renewal spends in PostgreSQL
is attempts × timeout. 5 s keeps that at 15 s — half the tick — with room for the 2 s
connect timeout. The parser refuses 0: that is PostgreSQL's "no timeout", and a typo
should not disable the deadline this exists to enforce.

The DDL pool keeps lock_timeout (1 s) so a blocked CREATE INDEX yields to
applyPostgresSchema's bounded retry instead of pinning a connection forever; only
statement_timeout is disabled there.

Rollout

No protocol change and no schema change; wire-compatible in both directions.

Ships in the next cell roll, director first — the director is the process taking the
contended lock, and a director on the new build against cells on the old one still only
reduces contention. Note that each process now opens one extra max: 1 connection at
startup, ended before the serving pool opens; the Cloud SQL budget model reads poolMax
from config.ts and terraform, so it is unchanged, but a fleet-wide simultaneous restart
adds up to one transient connection per process.

Expect orca_relay_postgres_transaction_exhausted to fall: per the #471 rollout, the 500 ms
bound raised exhaustions ~30x because waiters that used to succeed slowly now fail fast, and
this PR removes the contention that made them wait. Watch cellInventoryHoldMsP95 and the
exhausted-retry rate on the director before rolling cells.

[coderabbitai]

📝 Walkthrough

Walkthrough

The relay assignment store now uses per-cell row locks for sticky assignment retries and reservation reconciliation. Reservation updates use atomic deltas. PostgreSQL database opening now applies schema DDL through an untimed pool and uses a configurable timeout for the serving pool. SQLSTATE 57014 is retryable. New unit and PostgreSQL integration tests verify locking behavior, pool lifecycle, timeout configuration, and transaction recovery.

Merge Risk: 🔵 Low · up to 88b96

An explicit database timeout override can disable the serving query deadline, allowing blocked queries to run indefinitely. Validate explicit overrides before merge.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description provides strong technical detail about the changes, rationale, testing, and rollout. However, it omits required template sections, leaves the mandatory Linked Issue as Fixes #, and d…	Add a valid linked issue number, add the missing ELI5, What Changed, Visual Proof, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist sections, and mark or explain the applicable testing and validation items.
Docstring Coverage	⚠️ Warning	Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 7 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: per-cell inventory locks, atomic delta counters, and configurable PostgreSQL statement timeouts. It is specific and concise enough for repository hist…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The description provides strong technical detail about the changes, rationale, testing, and rollout. However, it omits required template sections, leaves the mandatory Linked Issue as Fixes #, and does not provide the required Visual Proof entry or checklist confirmations.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 8e5744e2-df85-475a-b2e7-0d0e3234b3fc

📥 Commits

Reviewing files that changed from the base of the PR and between 7cb0547 and 88b96cf.

📒 Files selected for processing (7)

• cloud/apps/relay/src/assignment-store.ts
• cloud/apps/relay/src/cell-inventory-lock-census.test.ts
• cloud/apps/relay/src/cell-inventory-per-cell-locking-postgres.test.ts
• cloud/apps/relay/src/database-postgres-timeout.test.ts
• cloud/apps/relay/src/database-statement-timeout-postgres.test.ts
• cloud/apps/relay/src/database.ts
• cloud/apps/relay/src/postgres-transaction-recovery.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.

[coderabbitai]

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Hold the DDL lock beyond the serving deadline.

The schema pool uses statement_timeout: 0 and lock_timeout: 1_000, but releaseTable() runs as soon as the waiter appears. Hold the lock for more than 200 ms and less than 1 second, then assert that startup emits no schema retry or 57014 failure before releasing it.

[coderabbitai]

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🔎 Supported by static analysis

🌐 Web query:

For PostgreSQL, does setting the session statement_timeoutto0 disable statement timeouts?

💡 Result:

Yes, setting the PostgreSQL session parameter statement_timeout to 0 disables the statement timeout [1][2]. According to official PostgreSQL documentation, a value of zero is the default and explicitly disables the timeout, meaning statements will not be aborted due to a duration limit [1][3]. It is important to note that if you observe the timeout seemingly remaining active despite running SET statement_timeout = 0;, it may be due to connection pooling middleware, such as PgBouncer, operating in transaction mode [4]. In such configurations, the connection state is often reset between transactions, causing your SET command to be discarded before the subsequent statement is executed [4]. In these cases, you may need to adjust the configuration at the database or role level, or use session-level configuration within a transaction block if supported by your application architecture [4].

Citations:

• 1: https://www.postgresql.org/docs/current/runtime-config-client.html
• 2: https://www.postgresql.org/docs/19/runtime-config-client.html
• 3: https://www.postgresql.org/docs/17/runtime-config-client.html
• 4: https://stackoverflow.com/questions/76687320/postgresql-set-statement-timeout-0-doesnt-seem-to-work-why

---

🤖 get_repo_knowledge executed:

get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions

Length of output: 24479

---

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- changed area ---'
sed -n '1000,1065p' cloud/apps/relay/src/database.ts
printf '%s\n' '--- timeout helper references ---'
rg -n -C 8 'relayPostgresStatementTimeoutMs|statementTimeoutMs|statement_timeout' cloud/apps/relay/src cloud/apps/relay/test cloud/apps/relay/tests 2>/dev/null || true

Repository: stablyai/orca

Length of output: 25853

---

Validate explicit serving timeouts.

statementTimeoutMs: 0 reaches the serving pg.Pool unchanged. PostgreSQL treats statement_timeout = 0 as disabled, so this removes the serving-query deadline. Apply the existing integer-and-minimum validation to explicit overrides. Add tests for 0, negative, and fractional values.

[pullfrog]

ℹ️ No critical issues — one coverage-gap note and a comment nit inline.

Reviewed changes

Reviewed the per-cell inventory-lock narrowing, the atomic delta counter write, the DDL-on-untimed-pool split, and the retryable-57014 change, plus their census/postgres tests.

• Per-cell sticky-refresh lock — assignStickyOnce's retry reads the pin unlocked and locks only that one cell row before the assignment row, with a pin-moved re-check that rethrows database_lock_unavailable.
• Two-row reconciliation lock — reconcileReservationAccounting locks only {source, target} cells and reads the cell-id existence set unlocked.
• Atomic delta counter — the sticky control grant now uses adjustCellReservationAtomically (reserved_requests + 1 with an in-statement capacity guard) instead of a snapshot-derived absolute write.
• Untimed schema pool — DDL runs on a dedicated max: 1 pool with statement_timeout: 0, opened and closed before the serving pool opens.
• Retryable statement timeout — 57014 joins retryablePostgresTransactionError, so a statement deadline retries rather than surfacing as a terminal failure.

I traced the lock-ordering model myself and independently verified it against the concurrency lens: the assignment→activity→cell vs cell→assignment→activity asymmetry between reconciliation and placement is pre-existing and is broken by the NOWAIT / bounded lock_timeout / retry wrappers, so narrowing the two locks to per-cell rows removes cycle edges without adding any. The pin-moved re-check is correctly fenced by the FOR UPDATE assignment lock, and the delta write is a strict improvement.

ℹ️ The census ratchet no longer polices lockCellRows modes

lockCellRows now carries the same CellInventoryLockMode distinction the census exists to police (including a bounded-request default), but readCallSites() only matches lock(?:General)?CellInventory(, so the "never puts a sweep-reachable site on the bounded wait" and "routes every sweep-only site to NOWAIT" tests can no longer see it. Today the three sites are correct (reconcileReservationAccounting passes 'pool-default'; assignStickyOnce threads lockMode; the two changeActivity-family callers are request-only), so this is a gap rather than a bug — but the PR removed the census entries for assignStickyOnce and reconcileReservationAccounting without re-covering their new lockCellRows form, and a future sweep-reachable bounded lockCellRows call would now slip through undetected.

Technical details

# Extend the lock census to classify lockCellRows

## Affected sites
- cloud/apps/relay/src/cell-inventory-lock-census.test.ts:168 — `readCallSites()` regex `lock(?:General)?CellInventory\(` does not match `lockCellRows(`, so its three call sites are unclassified.
- cloud/apps/relay/src/assignment-store.ts:6985 — `lockCellRows` gains a `mode: CellInventoryLockMode = 'request'` parameter, the exact bounded-wait surface the census's sweep-safety invariants protect.

## Required outcome
- A sweep-reachable `lockCellRows` call that resolves to a bounded (`'request'`) or queueing wait fails the census, matching the existing guarantees for `lockCellInventory`.

## Suggested approach (optional)
- Extend `readCallSites` (and the `CENSUS` shape) to classify `lockCellRows(transaction, ids, mode)` calls by mode, then rely on the existing `derivedReachability` sweep tests unchanged.

ℹ️ Nitpicks

• The reconcileReservationAccounting comment states "only reconcileCells writes" relay_cells; configureCell (assignment-store.ts:2687) and cell-admission-migration-registration.ts insertCell also insert/upsert it. The safety-relevant fact is that no production path deletes a cell row, which is what keeps the unlocked read safe — wording it that way avoids implying the table has a single writer.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

The justification is slightly off: configureCell (assignment-store.ts:2687) and insertCell in cell-admission-migration-registration.ts:199 also write relay_cells. The safety-relevant fact is that no production path deletes a cell row (all three are inserts/upserts), which is what makes the unlocked read safe — worth wording it that way so a future editor doesn't assume the table has a single writer.