Skip to content

Fleet controller - Compensate VMSS capacity for phantom drift one settled mutation at a time - #10540

Merged
potatoqualitee merged 15 commits into
developmentfrom
fleet-fix
Aug 8, 2026
Merged

Fleet controller - Compensate VMSS capacity for phantom drift one settled mutation at a time#10540
potatoqualitee merged 15 commits into
developmentfrom
fleet-fix

Conversation

@potatoqualitee

@potatoqualitee potatoqualitee commented Aug 8, 2026

Copy link
Copy Markdown
Member

Problem

On Flexible orchestration, deleting spent ephemeral VMs one at a time leaves sku.capacity (nominal) above the number of instances that exist (actual) — observed in production as sku=9 with 6 VMs. A capacity PATCH creates newValue − nominal instances, not newValue − actual, so scaling "to target" under drift silently under-creates: a ten-runner lane sat at six VMs. Two earlier designs failed: normalize-then-scale cancels in-flight creations, and one-plan-step-per-pass lets churn-minted drift starve creation entirely (the fleet drained 8→5→3 against a target of 20).

⛔→✅ Merge gate: RESOLVED — scale-in is destructive, branches reworked

The experiment never had to be run; production answered on 2026-08-08:

  • Down-PATCHes delete live instances. During the drain, six consecutive normalization passes each deleted exactly nominal − newValue live VMs (App Insights decision traces matched against per-VM lifecycle, verified pass by pass).
  • It killed this PR's own CI run. Run 31273376357 (19:01 UTC): all ten SQL lanes died with "self-hosted runner lost communication" and zero failed steps — healthy jobs whose VMs were deleted underneath them, in simultaneous batches (3 at 19:18:40, 3 at 19:20:40 — the multi-VM down-PATCH signature). The deployed controller's normalization branch woodchipped its own busy runners mid-job.
  • The gap is conserved. Every capacity PATCH (up or down) preserves the nominal-over-actual gap; per-VM deletes grow it; it clears only at the zero-crossing, where a down-PATCH has no members left to take. Normalization can therefore never catch the drift it chases — it just deletes runners forever.

The original normalization/reclaim branches were destructive and are gone. What replaced them:

What this does (reworked)

At most one capacity mutation per settled pass, chosen by Get-FleetCapacityStep:

  • Mutate only from an ARM terminal state (Succeeded/Failed/Canceled); unknown/transitional states and negative telemetry skip the pass.
  • Shortfall → compensated scale-out: PATCH to min(35, nominal + (target − actual)), which creates exactly target − actual instances regardless of drift, in one fire-and-forget mutation. If the fleet is empty and the step would clip at the 35 ceiling (creating fewer than target, then pinning there), the reclaim to zero wins the pass instead and the next pass creates the full target from a clean nominal.
  • The only down-step is the reclaim to zero, on a settled pass with no members — it deletes nothing and repays the entire drift at once. A drifted nominal above an occupied fleet stays untouched, costing at most gap-many slots of ceiling headroom until the fleet next empties (which it does nightly under the 20-minute warm-down).
  • The fleet inventory is listed after the settled-state read, so a down-PATCH can never act on a list that predates a just-completed scale-out.
  • Fail closed on garbled telemetry: a capacity read without a usable integral sku.capacity throws TransientFleetException instead of coercing to a false nominal-0 (which would turn compensation into a down-PATCH from Azure's real figure) or crashing at the [int] cast. List payloads must carry their arrays (value, runners) as actual arrays — any other shape throws rather than flattening into a false empty inventory.
  • The zero reclaim needs two witnesses: GitHub must show zero online runners (a runner cannot be online without a live VM behind it) and a confirming second inventory read must come back empty; one recanting witness skips the PATCH for the pass. A null inventory read fails the pass outright.
  • Nominal/actual arrive from ARM unvalidated (the janitor runbook treats capacity above the ceiling as a real state); only the controller-chosen target keeps its range gate. A runaway nominal no longer crash-loops — it rides along and unwinds through the zero-crossing.

Adversarial review status

# Finding Status
1 Scale-in count semantics on a drifted Flex VMSS unverified ANSWERED — destructive (production telemetry above); branches reworked here
2 Stale-low VM list could drive a down-PATCH below real membership Fixed (read-order + GitHub corroboration)
3 ValidateRange(0,35) on ARM-derived inputs → uncaught crash-loop Fixed here
4 Nominal ratchets to the 35 ceiling under sustained churn Accepted: unwinds at the nightly zero-crossing; worst case is gap-many slots of headroom
5 Persistent allocation failure ratchets silently with no distinct alarm Follow-up
6 Legacy CLI (reconcile-runner-fleet.ps1) is a second, unserialized capacity writer if run manually Follow-up / ops note
7 Unit-test gaps Fixed (126 tests green; destructive-move tests flipped to the proven semantics)

Deploy status

Already deployed (operator ran deploy-controller.ps1 from this branch, 2026-08-08). Verified in production since: the fleet sits drift-free at capacity 4 with exactly 4 VMs, and a full 10-lane CI run completed with every runner surviving — the sawtooth is gone. Merging (squash, per repo convention) aligns development with what is already running.

🤖 Generated with Claude Code

claude added 15 commits August 8, 2026 18:02
…g it first

One plan step per settled pass starves creation: ephemeral deletes mint
fresh phantom capacity between passes, normalization wins the slot every
time, and the fleet drained from 8 VMs to 3 against a target of 20
watching it live. Raising nominal by exactly the shortfall makes Azure
create target-minus-actual instances however stale the bookkeeping is,
in a single mutation with nothing to await. Normalization now runs only
on settled passes where demand is already met, or when the compensated
step is pinned against the 35 ceiling and reclaiming headroom is the
only way forward.

(do Get-DbaBuild)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An unknown or missing provisioningState is indistinguishable from an
operation in flight, and mutating on it risks the overlapping PATCH the
settled gate exists to prevent. Allowlist the three terminal states
instead of denylisting the transitional ones; skipping a garbled pass
costs one safety tick, overlapping costs cancelled instances.

(do Get-DbaBuild)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…heck

The VM list feeding actual_capacity was taken before the capacity GET,
so a pass arriving just as a scale-out finished could see Succeeded with
a list that predates the created instances. A stale-low count is the one
input that makes a down-PATCH land below the real membership, where it
stops being bookkeeping and starts deleting live runners. Reading the
provisioning state first makes the settled check vouch for the list:
settled proves the prior mutation finished, so a list taken afterwards
cannot be missing its members.

(do Get-DbaBuild)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…crashing

Nominal and actual arrive from ARM, and the janitor runbook already
treats capacity above the 35-runner ceiling as a real state worth
warning about. A ValidateRange on those parameters turns that inventory
into a ParameterBindingException on every pass -- which is not
TransientFleetException, so nothing catches it: the reap keeps running
while scaling and the heartbeat never execute again until a human
intervenes. Drop the gate on the two telemetry parameters and keep it on
target, which the controller computes itself from the MAX_RUNNERS
setting. A runaway nominal now falls into the reclaim branch and
normalizes back under the ceiling on the next settled pass, which is the
self-heal the runbook wants. Pin that path and the other uncovered
branches -- Canceled recovery, compensation when actual runs ahead of
nominal, normalize-to-zero, surplus with demand met, and the single-slot
ceiling reclaim -- with tests.

(do Get-DbaBuild)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…zation in tests

Review round 1 flagged two untested behaviors. The reconcile ordering fix had no
regression test, so a mocked full pass now feeds Get-FleetCapacityStep a fleet
that only materializes after the capacity read: the old list-before-read order
would price the step from an empty inventory and fail the parameter filter. And
Get-FleetCapacityStep had no case for an actual membership above the 35 ceiling;
the new case pins that normalization returns the real count unclamped, because
clamping would turn a bookkeeping correction into the deletion of live runners.

(do Get-DbaBuild)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…andate

Review round 2: the mock fixtures in the capacity step ordering test used
single-line multi-entry hashtables, which cannot carry the mandatory aligned
assignments. Expanded to multiline with the equals signs lined up; the three
VM fixtures collapse into one loop instead of three near-identical literals.

(do Get-DbaBuild)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ream redirect

Review round 3. The ordering test redirected the information stream with 6>,
which PowerShell v3 does not parse; the suite's other reconcile-driving tests
let the Write-Host noise through, so this one now does too. And the ceiling
comment gets its original opening sentence back verbatim -- every capacity
function in this file still shares a ValidateRange on its policy-derived
parameters -- with the no-gate rationale for the ARM-derived telemetry kept
as its own sentences after it.

(do Get-DbaBuild)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tive

Review round 4. A garbled ARM read that surfaced a negative nominal could have
flowed through the compensation arithmetic into a negative PATCH body. Negative
telemetry now skips the settled pass the same way an unsettled state does, so
the next read starts clean; an in-body guard rather than ValidateRange, because
an attribute would crash the pass and recreate the exact failure mode the
missing range gates avoid. Two tests pin the skip.

(do Get-DbaBuild)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Production proved the scale-in semantics question the PR was gated on: on
Flexible orchestration a capacity down-PATCH deletes nominal-minus-newValue
live instances, observed six consecutive times during the 2026-08-08 drain and
then again when a normalization pass killed ten busy runners mid-job in a
single CI run. Every capacity PATCH conserves the nominal-over-actual gap, so
normalizing can never catch the drift it chases; the gap clears only at the
zero-crossing, where a down-PATCH has no members left to take.

Get-FleetCapacityStep therefore keeps the compensated scale-out and drops
every other down-step: the one reclaim it still emits is to zero, on a settled
pass with no members, which deletes nothing and repays the whole drift at
once. A drifted nominal above an occupied fleet stays untouched and costs at
most gap-many slots of ceiling headroom until the fleet next empties.

(do Get-DbaBuild)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…oborate the zero reclaim

Two guards at the call site close the remaining delete-live-instances routes.
A capacity read that arrives without sku.capacity used to coerce through [int]
into a false nominal of zero, which turns the compensated scale-out into a
down-PATCH from Azure's real figure; the pass now throws
TransientFleetException instead of pricing a step from a guessed number. And
because the only remaining down-step is the reclaim to zero computed from the
ARM VM list, a falsely-empty list is checked against GitHub before the PATCH:
a runner cannot be online without a live VM behind it, so any online count
contradicting an empty list marks the list stale and skips the reclaim.

The ordering Describe grows a shared BeforeEach harness and three new tests
covering the read-then-list order, the malformed-read bailout, and both sides
of the online-runner corroboration.

(do Get-DbaBuild)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t an empty fleet

@($null).Count is 1, so a state object whose Vms list never materialized would
impersonate a single live VM instead of failing loudly, and a null runner list
would slide through the reclaim corroboration as zero online. The capacity
block now throws TransientFleetException when either list is null, keeping
every figure that prices a capacity step a real observation.

(do Get-DbaBuild)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Get-FleetState flattens a garbled-but-200 list response into a clean empty
array, and ARM list endpoints are eventually consistent, so a single empty
read is not proof of an empty fleet. The reclaim to zero now requires GitHub
to show no online runner AND a confirming second inventory read to come back
empty; either witness recanting skips the PATCH for the pass. The extra ARM
call is paid only on the rare empty-fleet path, never on the hot scale-out
path.

(do Get-DbaBuild)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A 200 without its list array used to flatten through @() into a clean empty
inventory: Invoke-ArmList silently emitted nothing for a page without a value
array, and Get-FleetState read a runners-less GitHub payload as zero runners.
Both endpoints always ship their array even when it is empty, so a missing
array is proof of a garbled read, not an empty fleet, and both now throw
TransientFleetException. The malformed shapes are exercised through the real
Get-FleetState rather than a mocked state object.

(do Get-DbaBuild)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pped step

A compensated scale-out that clips at the 35 ceiling over an empty fleet
creates fewer than target instances and then pins there until the drift
unwinds (nominal 30, actual 0, target 10 would PATCH to 35 and create
five). With zero members the zero-crossing reclaim is free, so it now
wins that pass and the next pass creates the full target from a clean
nominal. A clipped step over a nonzero fleet keeps its current behavior,
since reclaiming there would delete the live members.

Also fail the pass on a non-integral sku.capacity -- the [int] cast
would throw InvalidCastException past the TransientFleetException catch
and crash the invocation -- and tighten the two list-shape guards to
demand actual arrays, since a scalar or object where the array belongs
survives a null check and still flattens into a false empty inventory.

(do Get-DbaBuild)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
[int]::TryParse replaces the digits regex and the cast: a digit-only
string past Int32.MaxValue passed the regex and then overflowed the
cast, which throws past the TransientFleetException catch and crashes
the invocation instead of skipping the pass. A parsed negative still
rides through to the policy's negative-telemetry guard, keeping the
division of labor -- unparseable garble is refused at the read,
out-of-domain numbers are the policy's call.

Also reword the reclaim rationale that still described Get-FleetState
flattening garbled pages: the shape guards make those throw now, and
the two-witness check exists for well-shaped stale pages from
eventually-consistent list endpoints.

(do Get-DbaBuild)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@potatoqualitee

Copy link
Copy Markdown
Member Author

damn, bout time

@potatoqualitee
potatoqualitee merged commit 134a225 into development Aug 8, 2026
31 of 32 checks passed
@potatoqualitee
potatoqualitee deleted the fleet-fix branch August 8, 2026 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants