mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 16:02:32 +00:00
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
This commit is contained in:
@@ -136,12 +136,18 @@ export async function prepareProductionCapacityCell(config, overrides = {}) {
|
||||
const desiredState = config.mode === 'isolate' ? 'migration-only' : 'general'
|
||||
const membership = membershipWithStates(before.selector, { [config.cellId]: desiredState })
|
||||
const result = await applyExactAdmissionSelector(post, membership, {
|
||||
expectedCurrentSelector: before.selector
|
||||
expectedCurrentSelector: before.selector,
|
||||
// The stamp that tells the director this cell is parked for a restart rather
|
||||
// than held back as capacity, so it may re-place the hosts still on it. The
|
||||
// activate branch omits it, and moving to 'general' clears it in the same
|
||||
// statement that writes the state.
|
||||
...(config.mode === 'isolate' ? { rollIsolatedCells: [config.cellId] } : {})
|
||||
})
|
||||
return {
|
||||
changed: result.changed,
|
||||
generation: result.selector.generation,
|
||||
admissionState: desiredState
|
||||
admissionState: desiredState,
|
||||
rollIsolated: config.mode === 'isolate'
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -168,6 +168,36 @@ describe('production Relay capacity cell admission', () => {
|
||||
assert.doesNotMatch(fake.calls.map(({ path }) => path).join(','), /\/v1\/admin\/drain/)
|
||||
})
|
||||
|
||||
it('stamps the roll isolation on isolate and never on activate', async () => {
|
||||
// The stamp is what lets the director tell a cell parked for a restart from
|
||||
// an evacuation target or an Asia rollback, both of which must keep their
|
||||
// hosts. Only this call site may send it.
|
||||
const isolate = canaryFetch()
|
||||
await prepareProductionCapacityCell(
|
||||
{ ...config, mode: 'isolate' },
|
||||
{ fetch: isolate.fetch, token: 'token' }
|
||||
)
|
||||
const isolateApply = isolate.calls.find(
|
||||
({ path }) => path === '/v1/admin/admission-selector/apply'
|
||||
)
|
||||
assert.deepEqual(isolateApply.body.rollIsolatedCells, [config.cellId])
|
||||
|
||||
// Restore has to be a real apply, not the no-op an already-general cell
|
||||
// takes, or the assertion below proves nothing.
|
||||
isolate.calls.length = 0
|
||||
const restored = await prepareProductionCapacityCell(
|
||||
{ ...config, mode: 'activate' },
|
||||
{ fetch: isolate.fetch, token: 'token' }
|
||||
)
|
||||
assert.equal(restored.admissionState, 'general')
|
||||
const restoreApply = isolate.calls.find(
|
||||
({ path }) => path === '/v1/admin/admission-selector/apply'
|
||||
)
|
||||
assert.ok(restoreApply, 'restore must issue an apply')
|
||||
assert.equal(restoreApply.body.rollIsolatedCells, undefined)
|
||||
assert.ok(restoreApply.body.membership.general.includes(config.cellId))
|
||||
})
|
||||
|
||||
it('drains the selected cell independently after durable isolation', async () => {
|
||||
const fake = canaryFetch()
|
||||
const result = await prepareProductionCapacityCell(
|
||||
|
||||
@@ -162,6 +162,9 @@ export async function applyExactAdmissionSelector(post, membership, options = {}
|
||||
...(before.selector.generation === 0
|
||||
? { expectedMembershipSha256: membershipSha256(before.selector.membership) }
|
||||
: {}),
|
||||
// Only a same-cap roll's isolate passes this; every other caller omits it
|
||||
// and leaves the cell's hosts pinned, which is the pre-existing behaviour.
|
||||
...(options.rollIsolatedCells ? { rollIsolatedCells: options.rollIsolatedCells } : {}),
|
||||
membership: desired
|
||||
})
|
||||
} catch (error) {
|
||||
|
||||
Reference in New Issue
Block a user