Repository navigation
fix(relay): let a host leave a dead reserve cell without an operator fence - #27458
Conversation
…fence A host whose reserve home cell died got 503 relay_home_cell_unavailable until the cell came back: the database path demands a committed fence for every capped cell. On its own /v1/assign, a host now moves off a reserve cell (Postgres records it as reserve) when the cell's heartbeat is stale and this director's seat-feed polls have gone unanswered for 45 s, unless more than a third of the polled cells look the same. Sweeps, migration pins and capped db cells keep the fence.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to A host might be moved off a cell that has since been switched to db mode, if a director holds a stale reserve-mode cache. This earlier concern remains open and should be confirmed or resolved before merge. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 11 files. (2 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check The title clearly and concisely describes the primary change: allowing a host to leave a dead reserve cell without an operator fence. Description check The description is complete and well aligned with the template. It explains the user impact, mechanism, safeguards, alternatives, testing, operational prerequisites, and unchanged behavior. It documen… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 11 files. (2 skipped: 2 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![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: Advanced
- Run ID:
99a5df00-892b-4b67-bc6a-340553ff0b7c
📒 Files selected for processing (10)
cloud/apps/relay/src/app.tscloud/apps/relay/src/assignment-dead-reserve-cell-postgres.test.tscloud/apps/relay/src/assignment-isolated-cell-replacement.test.tscloud/apps/relay/src/assignment-store.tscloud/apps/relay/src/cell-poll-reachability.test.tscloud/apps/relay/src/cell-poll-reachability.tscloud/apps/relay/src/public-assignment-reconnect-lane.test.tscloud/apps/relay/src/relay-region-app.test.tscloud/apps/relay/src/relay-server.tscloud/apps/relay/src/shadow-seat-directory.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| cells: () => cells, | ||
| startedAt, | ||
| tick: () => round(true), | ||
| pollsDead: (cellId, at) => reachability.dead(cellId, cells.length, at), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1540,1605p' cloud/apps/relay/src/assignment-store.ts
sed -n '650,685p' cloud/apps/relay/src/shadow-seat-directory.ts
sed -n '744,792p' cloud/apps/relay/src/shadow-seat-directory.tsRepository: stablyai/orca
Length of output: 6362
Use the same cell snapshot for polling and pollsDead.
Each later round starts refreshCells() and then polls the previous cells array. The refresh can replace cells before pollsDead runs, so cells.length can include cells that this round has not polled. For example, two unreachable cells from a three-cell snapshot pass unreachable * 3 <= 6 after a refresh adds three cells, even though two-thirds of the polled cells are unreachable.
The assignment path uses this result to allow a host-requested move when the current cell is reserve, unheard, unpinned, and cellPollsDead returns true. Capture the array used by polls and use that snapshot's length in pollsDead.
Suggested fix
let cellListRetryAt = 0
let lastSummaryAt = now()
let stopped = false
+ let lastPolledCells = cells
const identity = reusedIdentityToken(() => tokenProvider(audience), now, IDENTITY_TOKEN_REUSE_MS)
@@
- const polls = cells.map((cell) => poll(cell))
+ const polledCells = cells
+ lastPolledCells = polledCells
+ const polls = polledCells.map((cell) => poll(cell))
@@
- pollsDead: (cellId, at) => reachability.dead(cellId, cells.length, at),
+ pollsDead: (cellId, at) => reachability.dead(cellId, lastPolledCells.length, at),…d floor the move on the map - No verdict when half or more of the dead cell's own region fails with it (a cross-region partition), more than a third of the fleet does, or fewer than 6 cells are polled. A withheld region or fleet verdict logs orca_relay_dead_cell_verdict_withheld, which alerts. - The move needs the step-5 plan's epoch floor, so it mints above every seat the map knows; without the plan the fence is kept. A test revives the old cell's seat above the row and checks that the ledger never writes the row back. - The move's log line carries the fleet and region counts.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
d648182b-9514-4e1f-9a92-700bff641645
📒 Files selected for processing (9)
cloud/apps/relay/src/assignment-dead-reserve-cell-postgres.test.tscloud/apps/relay/src/assignment-store.tscloud/apps/relay/src/cell-poll-reachability.test.tscloud/apps/relay/src/cell-poll-reachability.tscloud/apps/relay/src/relay-server.tscloud/apps/relay/src/shadow-seat-directory.tscloud/dev/fixtures/terraform-root-partition/families.jsoncloud/dev/scripts/relay-reserve-dead-man-alert.test.mjscloud/infra/terraform/relay-observability.tf
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
…guards; gate the alert - Denominators hold only cells that served this director a feed page and have capacity: legacy rows answering 404 no longer fill the 6-cell sample or dilute the fleet and region ratios. - A peer counts as failing for the guards on any current streak (a failure in the last 5 s), so a staggered partition cannot let its first cell cross 45 s while its neighbours read healthy. - Every withheld verdict is logged, sample included (a cell missing from the feed cells is one). - Tests: ready=0 and enabled=0 with failing polls keep the fence (drain then death is fenced). - The withheld metric and alert are gated by relay_dead_cell_verdict_alert (default false, on in staging); the alert docs now describe poll failures, not heartbeats.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Read and lock the current admit mode before granting the dead-reserve… · assignment-store.ts:1568
cloud/apps/relay/src/assignment-store.ts:1568
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRead and lock the current admit mode before granting the dead-reserve exception.
When
assignOncepassestransaction,reserveModeCells(transaction)returns the cached read without querying the database. That cached read remains usable for up toRESERVE_MODE_STALE_MS(60 seconds), although the normal refresh interval is 5 seconds.If another director changes the cell to
db, the stale set can still contain the cell.deadCellFenceRequirementreportscappedwhen a connection-limit row exists, but the exception is inside thefence.cappedbranch and only excludesfence.pinned. A dead verdict can therefore bypass the fence for a capped db-mode cell.Read and lock the cell's
relay_cell_admit_modesrow in this transaction, and use that current result instead of the cached set for this exception.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
3a31a193-66c7-4b88-8640-544061798a57
📒 Files selected for processing (9)
cloud/apps/relay/src/assignment-dead-reserve-cell-postgres.test.tscloud/apps/relay/src/assignment-store.tscloud/apps/relay/src/cell-poll-reachability.test.tscloud/apps/relay/src/cell-poll-reachability.tscloud/apps/relay/src/shadow-seat-directory.tscloud/dev/scripts/relay-reserve-dead-man-alert.test.mjscloud/infra/terraform/environments/staging.tfvarscloud/infra/terraform/relay-observability.tfcloud/infra/terraform/variables.tf
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
…ector can still judge it A director that restarted after a reserve cell died never received a feed page from it, so the cell was never in the sample and its verdict stayed withheld as 'sample'. The candidate now always counts when it has capacity (the store asks only about a cell Postgres records as reserve, and only feed images run reserve); peers still count only once they have served a feed. Tests: a restart during a lone outage decides, a partition begun before the restart withholds by region, and the capacity filter. The Terraform comments now say to apply the metric first and the policy minutes later.
ELI5
A relay server ("cell") can die and not come back. When that happened to a cell in the new "reserve" mode, every desktop that had been on it was told "your cell is unavailable" over and over, and never got moved to a healthy cell until the dead one was restarted. Now, once the director has two independent signs that the cell is really gone, a desktop that asks is moved to a healthy cell.
What Changed
Before. On staging, reserve cell c2 was shut down. For 4.5 minutes, every desktop that had been on it got
503 relay_home_cell_unavailable, about 150 per 30 s per director. Not one moved to c1, which was live and had room. The outage ended only when c2 was started again.Why it happened.
After. A desktop that asks for a new cell on
/v1/assignis moved off its home cell, with the epoch bumped by one and its activity leases deleted, when all of these hold:unheard). A cell that is not ready, or is disabled for a drain, never qualifies, so drain-then-death keeps the fence;Each move logs
orca_relay_dead_reserve_cell_host_replaced, with the fleet and region counts. Every withheld verdict logsorca_relay_dead_cell_verdict_withheld(once a minute per cell). A new log metric and alert,relay_dead_cell_verdict_withheld, notifies on it. It is gated behindrelay_dead_cell_verdict_alert: default false, so the production plan is unchanged; true in staging. The first apply creates the metric before the policy.Unchanged:
evacuateDeadCells);Why
Signals. Heartbeats (cell → director → Postgres) and seat-feed polls (director → load balancer → cell) take different paths. Requiring both rules out every case where a live cell must keep its hosts:
Who moves. Only a host that asks is moved, and it asks because its own connection is gone. Connected hosts are never touched, and nothing double-seats. Reserve placement already moves an asking host off an unreachable reserve cell when another reserve cell exists. This brings the database path in line with that.
Alternatives considered:
Why db-mode cells are left out. A dead Asia db cell auto-moving ~3k hosts at once through the database path would recreate the source-row contention brownout seen in full-cell drains. Autoheal brings such a cell back in 3–5 min with its hosts intact. They are a deliberate follow-up.
Follow-ups, not in this PR:
/v1/assign.Director only.
/v1/assign, and a cell's store has no poller, so cell behaviour is unchanged. No cell image roll is needed.ORCA_RELAY_SHADOW_SEAT_FEED_CELLS. Without the poller this change does nothing there. Set it toall, and turn onrelay_dead_cell_verdict_alert, before the first production cell is flipped to reserve.Linked Issue
N/A (internal relay step-5 staging finding).
Visual Proof
N/A: server-side relay placement, with no UI.
Testing
cell-poll-reachability.test.ts:sampleincluded;assignment-dead-reserve-cell-postgres.test.ts(real PostgreSQL):evacuateDeadCellsmoves 0;ready=0orenabled=0with polls failing ≥ 45 s: 503, no move (the mutation that drops theunheardcheck fails both);ORCA_RELAY_TEST_POSTGRES_URLon a fresh, dedicated Postgres 16 container: 152 files, 1,326 passed, 1 skipped. Cloudnode --testscripts, including the alert contract and the Terraform root partition: 1,008 passed.tsc --noEmitis clean, andterraform fmt -checkis clean.Review
Agent skill upstream boundary
Notes
Not affected: SSH, mobile, desktop clients, or the wire format. Directors and cells update independently, and no new field crosses between them.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)