[Jinwoo-H]

	Files	Added	Deleted	Net
Test	9	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​689	$\color{#cf222e}{\Huge{\mathbf{−}}}$​21	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​668
Prod	5	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​178	$\color{#cf222e}{\Huge{\mathbf{−}}}$​47	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​131

ELI5

When the relay moves an idle desktop host to a closer region, the cell doing the move used to lock the capacity row of every cell in the fleet for the whole move. From an Asia cell each database step is slow, so that lock stayed on for about 3.6 seconds. Anything else that needed any cell's row timed out. Now the move locks only the one cell it is moving the host to. It takes that lock at the very end, so the lock is held for about one database round trip.

What Changed

Before. An idle rehome that started on an Asia cell locked all relay_cells rows. It also locked every row in the runtime, capability and safety tables. Then it ran about 20 more statements at about 175 ms each before COMMIT. For those ~3.6 s, hosts anywhere in the fleet failed. Reconnects, control renewals, sticky refreshes and placements hit their 0.5-1 s lock timeouts and retried. The 2026-09-22 investigation tied 78% of all relay lock-timeout cancels over 49 hours to these holds.

After. No other cell's row is locked at any point. The target row is held from its statement to COMMIT. In a test that adds 150 ms to every statement, that measured 169-176 ms.

Mechanism. The change is in commitIdleRegionalRehome and startRegionalRehomeCandidate in assignment-store.ts.

• The commit reads the cell inventory, admission, runtime, capability and safety tables without locks. It uses them to pick the target and for the fleet-safety check, which is a heuristic and tolerates a snapshot.
• The global control row and the worker row stay locked, and are now NOWAIT. They serialise the rate budget, the open-migration count and the hard-cap read. A second commit fails fast instead of queueing behind one that is crossing the ocean.
• The only cell lock is the target row. One statement locks the target's cell row and admission row NOWAIT, re-checks enabled, general admission and capacity, and adds the reservation. It is the last statement before COMMIT.
• If the target changed admission, filled up, or is locked by another writer such as an admission change, the whole commit rolls back. The cell answers deferred with the existing candidate-ineligible reason, so the director moves on to its next candidate. The cell also logs orca_relay_idle_rehome_target_deferred with the cause.
• NOWAIT on that row is required. The commit takes the host's assignment row before the cell row, and placement takes them in the other order, so a wait here could deadlock.
• The hold is now sampled with a site label. It still feeds cellInventoryHoldMsMax, so the planned alert on that field still sees rehome holds. New runtime-metric fields cellInventoryHoldMaxSite, rehomeTargetRowHoldMsMax and rehomeTargetRowHolds tell rehome holds apart from full-inventory holds.

Wire. Nothing changes between director and cell. The deferral uses a reason the director already accepts. The cell's status body is untouched, and the new fields appear only in the runtime-metrics log line.

Why

Alternatives considered:

• Run the commit on the director, which is about 1 ms from the database. The cell has to fence the idle host session while the commit runs, and that fence is in-process state on the cell. Moving the commit needs a two-phase protocol, a new cell route and capability negotiation for mixed versions. It also still needs an Asia cell rollout. This change gets the same lock relief with no wire change.
• Drop only the relay_cells lock. That leaves fleet-wide locks on the runtime, capability and safety tables, which cell heartbeats write. None of those locks protected a decision, so all four are gone.
• Reuse the existing atomic reservation update. That is a plain UPDATE that waits up to the 1 s lock timeout, and after a wait it can act on an admission value from before the change. The new statement locks NOWAIT and re-checks admission on the rows it locked.

Known limit. The connection hard-cap read is still a plain read that the target write does not re-check. A concurrent placement can therefore overshoot the cap by a few hosts. The control row still serialises rehome commits against each other.

Rollout. This needs a new cell image on the Asia cells, because they run the commit. The operator will roll it out as a separate Asia cell wave, one cell at a time. US-source commits were never the problem, and they pick this up with the next US image. This PR deploys nothing.

Linked Issue

No tracked issue. This is step 2(a) of the relay lock-convoy fix plan from the 2026-09-22 investigation.

Visual Proof

N/A. Server-side locking change with no UI.

Testing

Run in cloud/apps/relay with pnpm test against a fresh postgres:17 container. Result: 102 files and 918 tests passed. The one skipped test needs a separate database on port 55440. The idle-regional-rehome-store suite also passed against that database, 17 of 17. pnpm typecheck passes. The Postgres project also passed three times in a row, each on a fresh database.

New tests:

• Latency proof (regional-rehome-target-row-lock-postgres.test.ts). Every statement, BEGIN and COMMIT included, gets 150 ms of client-side delay. Before each of the 37 round trips, a second connection tries FOR UPDATE NOWAIT on the source, target and a bystander cell in all four tables. Every row except the target cell row is lockable at every trip. The target cell row is locked only at the trip before COMMIT, and the hold sample is between 150 and 300 ms.
• Admission flip committed after selection. The commit answers deferred / candidate-ineligible and leaves nothing behind: no attempt, no migration, no reservation. The host stays on its source, and the region-correction outcome report shows nothing for the target.
• Admission writer holding the target row. The commit defers at once instead of waiting or retrying, counts one lock-unavailable and records no hold.
• Lock order (assignment-store-lock-order.test.ts). The first rehome case pins the order control, worker, host rows, then the target cell row last. It pins NOWAIT on the control, worker and target rows, and checks that no other cell-table row is locked.
• Census and samples. The census entry is updated, and there are new tests for the site-labelled hold samples.

Each new Postgres test was checked by breaking the code on purpose. Putting the inventory lock back, removing NOWAIT, or removing the admission re-check each makes the matching test fail. Removing NOWAIT or putting the inventory lock back also fails the lock-order test.

One existing test changed. The commit no longer locks the safety table, so "rechecks fleet safety under locks" now commits the safety change before the commit starts.

• I manually tested these changes locally
• Automated tests added/updated, or explained why not below

Review

Agent skill upstream boundary

• Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

This is relay server code only, with no desktop, mobile, SSH or platform impact. Mixed director and cell versions stay compatible. An old director reads the new deferral as a known reason, and a new cell answers old directors unchanged.

Checklist

• This PR is small and focused
• I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
• Before/after screenshots or videos attached for UI changes, or N/A with reason
• Self-reviewed for correctness, security, and performance
• Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
• pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 592fb812-d15b-49ab-8996-876624159cc4

📥 Commits

Reviewing files that changed from the base of the PR and between a2a78ab and 9c920a4.

📒 Files selected for processing (11)

• cloud/apps/relay/src/assignment-store-lock-order.test.ts
• cloud/apps/relay/src/assignment-store.ts
• cloud/apps/relay/src/cell-inventory-hold-samples.test.ts
• cloud/apps/relay/src/cell-inventory-hold-samples.ts
• cloud/apps/relay/src/cell-inventory-lock-census.test.ts
• cloud/apps/relay/src/cell-inventory-lock-contention.test.ts
• cloud/apps/relay/src/database.ts
• cloud/apps/relay/src/regional-rehome-postgres.test.ts
• cloud/apps/relay/src/regional-rehome-store.test.ts
• cloud/apps/relay/src/regional-rehome-target-row-lock-postgres.test.ts
• cloud/docs/orca-relay-operations.md

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

---

📝 Walkthrough

Walkthrough

The idle-rehome commit now reads candidate inventory without locks and reserves the target cell through a final NOWAIT row lock. It returns a deferred outcome if the target is no longer eligible or another writer holds its row. Lock-hold metrics now identify the lock site and report target-row hold duration and count. Tests cover lock order, target-row contention, deferral, and measured holds. The operations guide documents the updated commit behavior and metrics.

Priority: ➖ Normal

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 15.79% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 10 files. (1 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: locking only the target cell row, last and NOWAIT, during the rehome commit.
Description check	✅ Passed	The description covers the required ELI5, changes, rationale, visual proof, testing, review, notes, and checklist sections. However, the template requires a linked issue, while the description states …
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: Docstring Coverage

Explanation

Docstring coverage is 15.79% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 10 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

---

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.}

[pullfrog]

ℹ️ No critical issues — one observability gap and a test-robustness note.

Reviewed changes

• Rehome commit drops the fleet-wide cell lock. commitIdleRegionalRehome now reads the cell inventory, runtime, capability and safety tables unlocked, keeps the global control and worker rows locked but NOWAIT, and takes its only relay_cells lock — the target row plus its relay_cell_admission row — as the last statement before COMMIT (reserveRegionalRehomeTargetRow).
• Target re-check folds into the locking statement. One WITH ... FOR UPDATE OF cell, admission NOWAIT ... UPDATE re-checks enabled, admission_state = 'general' and request capacity, then adds the reservation; any miss rolls the whole transaction back to deferred / candidate-ineligible and logs orca_relay_idle_rehome_target_deferred.
• Lock-scoped telemetry. CellInventoryHoldSamples gains a hold-site label and the fields cellInventoryHoldMaxSite, rehomeTargetRowHoldMsMax and rehomeTargetRowHolds; RelayLockOptions gains holdSite and lockClauseInStatement, with lockedStatement() centralizing the FOR UPDATE[ NOWAIT] suffix.
• Coverage and docs. A new lock-order test pins control → worker → host rows → target row last/NOWAIT; Postgres tests measure the ~one-round-trip hold under injected latency and the admission-flip / busy-row deferrals; the lock census and orca-relay-operations.md are updated.

ℹ️ New hold fields are emitted in the runtime-metrics log but not registered as Cloud Monitoring metrics

index.ts spreads consumeRelayCellInventoryHold(database) into the orca_relay_runtime_metrics event, so rehomeTargetRowHoldMsMax and rehomeTargetRowHolds reach the log. But google_logging_metric.relay_snapshot is built with for_each = merge(local.relay_runtime_metrics, ...) in cloud/infra/terraform/relay-observability.tf, and neither field is in that map, so no metric series is created for them. The Asia rollout proof reads the raw log event, so it can still see them, but any Cloud Monitoring alert or dashboard on these two fields would find nothing.

Technical details

# Register the rehome target-row hold fields, or document them as log-only

## Affected sites
- `cloud/apps/relay/src/index.ts:97` — emits the new fields inside `orca_relay_runtime_metrics`.
- `cloud/infra/terraform/relay-observability.tf:52-105` — `local.relay_runtime_metrics`; `rehomeTargetRowHoldMsMax` and `rehomeTargetRowHolds` are absent.
- `cloud/infra/terraform/relay-observability.tf:300-309` — `relay_snapshot` logging metric is created only for keys in `local.relay_runtime_metrics`.
- `cloud/docs/orca-relay-operations.md:495-510` — describes the fields as runtime metrics.

## Required outcome
- Either add numeric entries for `rehomeTargetRowHoldMsMax` and `rehomeTargetRowHolds` to `local.relay_runtime_metrics`, or state in the docs that these fields are log-only context and the alert remains on `cellInventoryHoldMsMax` (with `cellInventoryHoldMaxSite` naming the holder).
- `cellInventoryHoldMaxSite` is a string, so it cannot be a `value_extractor` on the DISTRIBUTION metric; it would need a label extractor, which the shared `relay_snapshot` resource deliberately avoids (adding labels recreates the live metrics). Log-only is the reasonable choice for it.

## Open questions for the human
- Is a future alert planned on `rehomeTargetRowHoldMsMax`, or is `cellInventoryHoldMsMax` plus the site label sufficient?

ℹ️ Nitpicks

• regional-rehome-target-row-lock-postgres.test.ts:242 bounds the busy-row commit at 40 * STATEMENT_DELAY_MS while the commit itself is ~37 delayed trips, leaving only a few hundred ms of headroom and coupling the assertion to the current statement count. A bound expressed against the injected delay independently of the statement count (or an assertion that no lock_timeout wait occurred) would be less brittle on a slow CI Postgres.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}