Commit Graph
3 Commits
Author SHA1 Message Date
Jinwoo Hong 84f58a1fc7 fix(relay): refuse a redial at once while the host's own release holds its row (#24225)
* fix(relay): refuse a redial at once while the host's own release holds its row

During an Asia drain the host whose socket closes is the one that redials.
Its release on the draining cell locks its assignment row first, then waits
on the cell's busy row for up to the lock timeout. The director's sticky
and placement paths waited on that assignment row inside the single sticky
slot, and the sticky path then locked the busy cell row itself before it
checked isolation. The slot backed up and dials timed out fleet-wide.

Both paths now take the host's assignment row NOWAIT and throw
RelayAssignmentRowBusyError when it is held. /v1/assign answers that with
503, Retry-After 1 and error assignment_row_busy, logged with its own
reason. The sticky path decides isolation before it touches the pinned
cell row. An isolated retry keeps its own tier as its retry scope, so a
busy lock inside it no longer falls to the all-rows path. The local drain
arm drops its zero-release-failures bar, which the Asia arm never had.

The drain harness gains a departing-host arm: each host releases its own
lease, then redials after 150, 400 or 1000 ms on the desktop client's
5-5.5 s pacing. At 400 ms, main rejected 83 of 180 first dials by sticky
wait timeout, placed 11.6/s with 3.1 director backends lock-waiting, and
took 11.2 s at p95 from release to placed. Now: 16 fast refusals, 18/s,
no lock waits, 5.7 s at p95.

Claude-Session: ced32ebb-7155-4413-adad-1eccd14c2010

* fix(relay): wait briefly for a calm host's row and keep the dead-cell sweep going

The dead-cell sweep treated RelayAssignmentRowBusyError as fatal, so one
busy host ended the sweep for every later host each tick. It now skips
that host and carries on.

The sticky path refused a busy row at once for every host. A calm host
redialling after its own clean close often meets its own short release,
and a refusal costs it the client's 5 s assign gate. When the pinned cell
is general and live, the sticky path now waits up to 1 s for the row
before refusing. A roll-isolated, parked or dead cell still gets the
immediate refusal. Placement keeps NOWAIT, because it holds cell rows
while it would wait. A resume refused for a busy row now carries
Retry-After 1 as well.

The departing-host harness arm now bounds the busy refusals at 20% of
hosts and the p95 at 8 s, and counts unexpected errors apart from
retryable refusals.

Claude-Session: ced32ebb-7155-4413-adad-1eccd14c2010

* fix(relay): never wait on a host's row while the sticky retry holds its cell row

The inventory-first sticky retry takes the pinned cell row before the
assignment row. With the calm-host bounded wait it could then wait up to
1 s on the assignment row while holding the cell row, the reverse of the
ranked lock order, against this host's own release, which holds its row
and wants the cell's. The bounded wait now applies only when no cell row
is held; the retry stays NOWAIT.

Claude-Session: ced32ebb-7155-4413-adad-1eccd14c2010
2026-09-30 17:58:30 -04:00
Jinwoo Hong ed462b2caa fix(relay): re-place hosts off a draining cell without locking its row (#24216)
* test(relay): reproduce drain-release contention against director placement

Adds a Postgres harness that drives releases from an isolated cell over a
171 ms per-statement pool while five directors re-place reconnecting hosts
through the sticky lane. At 18 releases/s placements fall from 20/s to about
5/s and every active director backend is blocked on relay_cells.

Moves the per-statement delay pool into a shared test fixture so the
rehome target-row test and this harness use one implementation.

Claude-Session: ced32ebb-7155-4413-adad-1eccd14c2010

* fix(relay): re-place hosts off a draining cell without locking its row

Sticky re-placement of a host whose cell is isolated for a roll locked
every relay_cells row. During an Asia drain the source row is held by the
cell's own releases for a round trip each, so the placement waited on it,
the single sticky slot backed up, and /v1/assign returned 503 fleet-wide.

The isolation decision now comes from an unlocked read, and the placement
locks only the same-region general rows it can move to. It no longer writes
the source row: the host's source leases stay, and each one's own release
or expiry takes its units back off the source. The assignment keeps its
activity counters and adds one control instead of resetting them. With no
same-region headroom the path falls back to the all-rows lock, as before.

Dormant hosts hold no units, so their placement also locks only the
general rows and skips the zero write to their old cell.

The drain harness now asserts the after picture: 20 placements/s at Asia
latency with no director lock waits, against 9.2/s and 4.8/s before.

Claude-Session: ced32ebb-7155-4413-adad-1eccd14c2010

* fix(relay): take a moved host's units off the cell that holds them

After a narrowed re-placement a host keeps leases on its old cell while
its assignment row names the new one. Two paths charged the row's whole
counted total to the row's cell: aggregate expiry, and the lease deletion
in dead-cell and stranded re-placement. Both over-charged the new cell and
left the old cell's units stranded.

Aggregate expiry now skips hosts that still hold any lease; the lease
sweep takes each lease's units off its own cell. Placement frees each
deleted lease's units on that lease's cell, charges the old cell only for
units no lease backs, and sets the counters from the leases it keeps plus
the new control. The narrowed path runs only when the counters already
match the leases, so it never needs to write the old cell's row.

The all-rows re-placement off an isolated cell follows the same rule, so
it no longer decrements the source at placement either.

Claude-Session: ced32ebb-7155-4413-adad-1eccd14c2010

* fix(relay): try other regions before the all-rows lock when re-placing off a roll

With every same-region neighbour at its connection cap, the narrowed path
found no target and fell back to the all-rows lock behind the busy source
row, which is the drain brownout again. It now tries a second tier, general
cells in every other region, in its own transaction over one ordered
lockCellRows, still never the source row. Only when no general cell in any
region has room does it fall back to the all-rows path, which keeps the pin.

This changes the policy from #21911, which refused to re-place an isolated
host across a region. The unit tests that encoded that rule now assert the
tier order instead.

The drain harness gains a US cell and an arm with every Asia neighbour
capped: 200 of 200 dials placed cross-region at 20/s with no lock waits,
against 0 placed and 144 sticky rejections on the previous head. Its pass
bars are now the rejection share and the lock-waiting share, not the
placement rate a slow runner's pacing can move.

Claude-Session: ced32ebb-7155-4413-adad-1eccd14c2010

* fix(relay): take the narrowed path for hosts whose counters sit below their leases

Main's old placement reset a moved host's counters while keeping its source
leases, and those leases' releases floored the counters at zero. Such hosts
hold fewer counted units than lease units, and requiring equality sent them
down the all-rows path behind the busy source row.

The narrowed path now requires only that the host holds no units no lease
backs, the one case that needs a write to the old row. Its placement
already rebuilds the counters from the kept leases plus the new control,
so a drifted host is healed by its next re-placement.

Claude-Session: ced32ebb-7155-4413-adad-1eccd14c2010

* test(relay): judge the local drain arm on completion and lock waits, not rate

The local-latency arm asserted at least 18 placements/s at a 20/s dial rate,
which a slow runner's pacing alone can miss. It now asserts what the Asia
arm does: no dial failures, sticky rejections under 10% of dials, every
other dial placed, and director lock-waiting under half a backend.

Claude-Session: ced32ebb-7155-4413-adad-1eccd14c2010
2026-09-30 17:00:03 -04:00
Jinwoo Hong c8a5580659 fix(relay): re-place hosts off a cell isolated for a roll (#21911)
* fix(relay): re-place hosts off a cell isolated for a roll

A roll isolates a cell by moving it out of the 'general' admission class; the
cell then refuses every attach with 4503. The director never noticed, because
the only liveness test it applies to a host's current cell reads
`relay_cell_runtime.ready` and the heartbeat, and an isolated cell keeps
heartbeating ready=1 for the whole drain. So every host on that cell was handed
its own dead cell, closed, and handed it back — 500-1,900 hosts looping for
13-16 minutes per cell roll, at ~6 dials each per minute, with no neighbour
absorbing anything.

The sticky lane now treats a live incumbent whose admission is 'migration-only'
— the state a roll's isolate step writes — the same way it treats a dead one:
it returns null, which means "fall through to placement". The placement lane
had the identical hole eleven lines further down, so it takes the same
predicate; without that second swap the sticky change is inert, because
placement would hand the pin straight back (a draining cell has more headroom
than anyone). An isolated incumbent skips the dead-cell fence branch: that
branch exists to prove an unreachable cell stopped serving a host, and this one
is reachable and enforces the epoch itself.

'existing-only' is deliberately untouched — those cells serve the hosts they
already hold, and only `assignmentStrandedOnUnservedCell` may release that pin.
A host with an open `relay_assignment_migrations` row keeps its pin too, so
this stays disjoint from the migration machinery.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

* fix(relay): gate re-placement on a roll-isolation marker, not on admission

Review of the first commit found the predicate wrong. `migration-only` is an
admission class, not a drain signal: an Asia `--mode rollback`, an evacuation or
forward-recovery target awaiting a separate promote dispatch, a failed same-cap
wave's re-isolate, an abandoned migration retired on its target and a rehome
settlement all park loaded cells there durably, with no migration lease and no
open migration row. All five were indistinguishable from a roll's isolate, so
the first commit would have converted `operate-relay-asia-admission --mode
rollback` from a reversible admission flip into a mass move of ~4,000 hosts —
and, because `leastLoadedCell` treated region as a preference, into us-central1.

The signal is now an explicit stamp. `relay_cell_admission` gains a nullable
`roll_isolated_at`, added through the shared schema runner's catalog pre-check
so a migrated database takes no relation lock on boot and an un-migrated one
gets a catalog-only rewrite. The same-cap isolate step is its only writer, via a
new optional `rollIsolatedCells` on the selector apply; the same UPDATE that
writes the state clears the stamp whenever a cell leaves 'migration-only', so a
restore cannot leave one behind and a failed wave's re-isolate keeps the one it
has. Every other admission writer omits the field, so its cells stay unmarked
and their hosts stay pinned. Old directors ignore the field; old callers never
send it.

Region is now a constraint rather than a preference on this path only: a
re-placement must find a general, live cell with connection headroom in the
host's own region, or the pin is kept and one
`orca_relay_sticky_replacement_deferred` event is logged. Cross-region spill is
no longer reachable here.

The fence bypass is narrowed to a live incumbent. It was always a no-op for the
intended case, and for a stamped cell that stops heartbeating while still
holding sockets it reopened split-brain; that cell now takes the dead-cell path
unchanged.

Also: the hot-path admission reader no longer throws on an unrecognised state —
it sits on every sticky dial and the rule it feeds is "move the host", so an
unreadable row has to mean "don't". And the sticky lane reads the admission row
once for both the stranded rule and the stamp instead of twice.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

* fix(relay): emit the re-placement events after the transaction commits

CodeRabbit on assignment-store.ts:1075. Both events were written where they are
decided, which is inside assignOnce's transaction. A reservation or lease write
failing after that point rolls the placement back, but a line already on stdout
cannot be rolled back with it — so the canary this PR asks an operator to read
would count re-placements that never happened, and a Postgres transaction retry
could leave a stale line behind as well.

The transaction now returns its events alongside the RelayAssignment and the
caller flushes them once it has resolved. Returning them rather than setting a
variable in the enclosing scope is what makes the retry case safe too: only the
attempt that committed can carry its events out. assign()'s signature is
unchanged; the extra shape lives entirely inside assignOnce.

orca_relay_sticky_replacement_deferred was moved the same way. It cost one more
push into the array that already existed, and it is decided inside the same
transaction, so leaving it behind would have been the odd case rather than the
cheap one.

The new test injects a failure on the first write after the decision, asserts no
event is emitted, and asserts the assignment is still on its original cell —
without that second assertion the absence would only prove the emit was early,
not that it would have been wrong. A control dial with nothing injected emits
exactly one event, so the case cannot pass on a broken harness. With the emit
put back inside the transaction, it fails.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

* fix(relay): expire the roll stamp, correct the wire note, assert the stamp landed

Delta review findings B, D and E. A (the deferral path's cost) is deliberately
not implemented; it is now written up under Follow-ups in the PR body as
required before any Asia roll, because it cannot fire in a US canary.

B, which also closes C: the stamp was written, carried and never compared to
anything. A roll isolates and restores one cell inside ~15 minutes, so a stamp
older than two hours is not a roll in progress. It is a failed wave whose
failsafe re-isolated a possibly healthy cell and is waiting on an operator — the
postmortem in this tree records gaps of hours — or an orphan left by a director
rollback whose restore wrote 'general' without the clause that clears the stamp,
which the selector's 'keep' branch would then preserve until some later park
reactivated it. Both want the same answer and it is the pre-existing one: keep
the pin. One comparison against a value already on the row.

The bound takes the caller's `now` rather than reading the clock again, so one
assign reasons about one instant; the stamp's age is now a thing that decides
whether a host moves, and two clock reads could disagree across it.

D: the comment beside the new request field claimed an updated caller reaching
an older director "is simply ignored". The schema is .strict(), so it is a 400.
That fails closed — the isolate aborts before MUTATION_STARTED is set and
nothing is written — but it is a deploy ordering constraint, and it was
undocumented. The comment now says so and the PR body's rollout notes carry it.

E: nothing read the `rollIsolated` the script already prints, so an older script
against a newer director would silently produce today's behaviour and the canary
would read as "the fix did nothing" with no way to tell that from a wrong
premise. Both isolate steps now assert it, beside the generation they already
parse.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
2026-09-21 04:18:57 -04:00