fix(sim): keep the autoscaler controlling when a knob is not a number

src/sim/behaviour-control.autoscaler.test.ts

@@ -0,0 +1,114 @@
+import { describe, expect, it } from 'vitest';
+import { Engine } from './engine';
+import { makeNode } from './presets';
+import type { NodeStats, Topology } from './types';
+
+/*
+ * None of the autoscaler's knobs is among the nine config numbers
+ * `isTopology` checks, so a shared link, a `.breakscale` file and a restored
+ * session can all hand the controller a value that is not a number. What it
+ * does with one is a property of the controller.
+ *
+ * The reading that matters is not the knob, it is whether the fleet still
+ * grows: a controller that quietly stops controlling looks exactly like a
+ * design that did not need to scale.
+ */
+
+function topology(patch: Record<string, unknown>): Topology {
+  const client = { ...makeNode('client', 0, 0), id: 'client' };
+  client.config = { ...client.config, rps: 400 };
+  const svc = { ...makeNode('service', 200, 0), id: 'svc' };
+  svc.config = {
+    ...svc.config,
+    capacity: 2,
+    serviceMs: 40,
+    instances: 1,
+  } as typeof svc.config;
+  const auto = { ...makeNode('autoscaler', 200, 160), id: 'auto' };
+  auto.config = { ...auto.config, ...patch } as typeof auto.config;
+  return {
+    nodes: [client, svc, auto],
+    edges: [
+      { id: 'e1', from: 'client', to: 'svc', weight: 1 },
+      { id: 'e2', from: 'auto', to: 'svc', weight: 1, control: true },
+    ],
+  };
+}
+
+function run(patch: Record<string, unknown>) {
+  const engine = new Engine(topology(patch), 7);
+  // Thirty simulated seconds: several cooldowns, so the controller has had
+  // every chance to act.
+  for (let i = 0; i < 1800; i += 1) engine.advance(1000 / 60);
+  const snapshot = engine.snapshot();
+  return {
+    auto: snapshot.nodes['auto'] as NodeStats,
+    svc: snapshot.nodes['svc'] as NodeStats,
+  };
+}
+
+const KNOBS = [
+  'targetUtil',
+  'minCapacity',
+  'maxCapacity',
+  'scaleStepPct',
+  'cooldownMs',
+  'warmupMs',
+] as const;
+
+describe('an autoscaler given a knob that is not a number', () => {
+  const baseline = run({});
+
+  it('scales the fleet in the baseline design', () => {
+    // Anchors the rest: this load genuinely needs more than one instance.
+    expect(baseline.svc.instances).toBeGreaterThan(5);
+    expect(baseline.svc.totalCompleted).toBeGreaterThan(5000);
+  });
+
+  for (const key of KNOBS) {
+    it(`still scales when ${key} is NaN`, () => {
+      // Before this change targetUtil, scaleStepPct and warmupMs each left
+      // the fleet at one instance and the design served 1520 requests where
+      // the baseline serves 6756.
+      const { svc } = run({ [key]: Number.NaN });
+      expect(svc.instances).toBeGreaterThan(5);
+      expect(svc.totalCompleted).toBeGreaterThan(4000);
+    });
+  }
+
+  // Where the module's documented fallback is also what the node carries,
+  // a NaN is indistinguishable from the value it replaced -- which is the
+  // sharpest available statement that the guard changed nothing real.
+  for (const key of ['targetUtil', 'minCapacity', 'scaleStepPct'] as const) {
+    it(`is identical to the baseline when ${key} is NaN`, () => {
+      const { svc } = run({ [key]: Number.NaN });
+      expect(svc.instances).toBe(baseline.svc.instances);
+      expect(svc.totalCompleted).toBe(baseline.svc.totalCompleted);
+    });
+  }
+
+  it('publishes a setpoint that is a number', () => {
+    const { auto } = run({ targetUtil: Number.NaN });
+    expect(Number.isNaN(auto.setpoint as number)).toBe(false);
+  });
+
+  it('publishes a target instance count that is a number', () => {
+    const { auto } = run({ scaleStepPct: Number.NaN });
+    expect(Number.isNaN(auto.targetInstances as number)).toBe(false);
+  });
+
+  it('leaves a design the editor can produce exactly where it was', () => {
+    // The knobs a reader sets are read the same way they always were, so a
+    // design that names all six is untouched by the guard.
+    const explicit = run({
+      targetUtil: 0.7,
+      minCapacity: 1,
+      maxCapacity: 12,
+      cooldownMs: 3000,
+      scaleStepPct: 0.5,
+      warmupMs: 4000,
+    });
+    expect(explicit.svc.instances).toBe(baseline.svc.instances);
+    expect(explicit.svc.totalCompleted).toBe(baseline.svc.totalCompleted);
+  });
+});

src/sim/behaviour-control.ts

@@ -54,6 +54,21 @@ const DEFAULT_COOLDOWN_MS = 5000;
 const DEFAULT_STEP_PCT = 0.5;
 const DEFAULT_WARMUP_MS = 0;
 
+/**
+ * One optional knob, or its fallback.
+ *
+ * `??` delivers the promise above for an UNSET field and not for one set to
+ * something that is not a number, and none of these knobs is among the nine
+ * config numbers `isTopology` checks -- so a shared link, a `.breakscale`
+ * file and a restored session can all hand the controller a NaN. NaN then
+ * survives `Math.floor`, `Math.max` and `clamp01` alike, because it fails
+ * every comparison those are written from, and the controller stops
+ * controlling.
+ */
+function knob(value: number | undefined, fallback: number): number {
+  return Number.isFinite(value) ? (value as number) : fallback;
+}
+
 interface AutoscalerState {
   /** Simulated time of the last decision; -Infinity means "never decided". */
   lastDecisionMs: number;
@@ -185,7 +200,7 @@ const autoscaler: ComponentBehaviour = {
       // ...and starts a fresh observation window, because the new node's
       // smoothed utilisation is not a signal yet.
       st.observeUntilMs =
-        ctx.now + Math.max(0, state.config.cooldownMs ?? DEFAULT_COOLDOWN_MS);
+        ctx.now + Math.max(0, knob(state.config.cooldownMs, DEFAULT_COOLDOWN_MS));
     }
     if (watched === '') return;
 
@@ -218,7 +233,7 @@ const autoscaler: ComponentBehaviour = {
     if (st.warmupDueMs >= 0) return;
 
     const cfg = state.config;
-    const cooldown = Math.max(0, cfg.cooldownMs ?? DEFAULT_COOLDOWN_MS);
+    const cooldown = Math.max(0, knob(cfg.cooldownMs, DEFAULT_COOLDOWN_MS));
 
     // Hold off until the watched node's utilisation is a real measurement
     // rather than an average still climbing out of its initial zero.
@@ -241,13 +256,16 @@ const autoscaler: ComponentBehaviour = {
     // the unit they bound is now INSTANCES -- the fleet size, not the thread
     // count. For every topology written before instances existed the two
     // readings coincide, because those nodes run exactly one instance.
-    const minInst = Math.max(1, Math.floor(cfg.minCapacity ?? DEFAULT_MIN_INSTANCES));
+    const minInst = Math.max(
+      1,
+      Math.floor(knob(cfg.minCapacity, DEFAULT_MIN_INSTANCES)),
+    );
     const maxInst = Math.max(
       minInst,
-      Math.floor(cfg.maxCapacity ?? DEFAULT_MAX_INSTANCES),
+      Math.floor(knob(cfg.maxCapacity, DEFAULT_MAX_INSTANCES)),
     );
-    const target = clamp01(cfg.targetUtil ?? DEFAULT_TARGET_UTIL);
-    const step = Math.max(0.01, cfg.scaleStepPct ?? DEFAULT_STEP_PCT);
+    const target = clamp01(knob(cfg.targetUtil, DEFAULT_TARGET_UTIL));
+    const step = Math.max(0.01, knob(cfg.scaleStepPct, DEFAULT_STEP_PCT));
     // An instance count is integral, so a step must move at least one machine:
     // a small percentage of a small fleet would otherwise round to a permanent
     // no-op and the controller would silently do nothing forever.
@@ -300,7 +318,7 @@ const autoscaler: ComponentBehaviour = {
       return;
     }
 
-    const warmup = Math.max(0, cfg.warmupMs ?? DEFAULT_WARMUP_MS);
+    const warmup = Math.max(0, knob(cfg.warmupMs, DEFAULT_WARMUP_MS));
     if (warmup === 0) {
       st.targetInstances = want;
       ctx.setScale(watched, want);
@@ -338,12 +356,12 @@ const autoscaler: ComponentBehaviour = {
     stats.pendingInstances = scaling ? Math.max(0, wanted - live) : 0;
     stats.scaling = scaling;
     stats.watchedUtil = st.watchedId ? (ctx.utilizationOf(st.watchedId) ?? 0) : 0;
-    stats.setpoint = clamp01(state.config.targetUtil ?? DEFAULT_TARGET_UTIL);
+    stats.setpoint = clamp01(knob(state.config.targetUtil, DEFAULT_TARGET_UTIL));
 
     // Which of the three waits it is in, and how much of it is left. Resolved
     // in the same order onTick() applies them, so the label never claims the
     // controller is free to act when the next tick will find it blocked.
-    const cooldown = Math.max(0, state.config.cooldownMs ?? DEFAULT_COOLDOWN_MS);
+    const cooldown = Math.max(0, knob(state.config.cooldownMs, DEFAULT_COOLDOWN_MS));
     if (scaling) {
       stats.scalePhase = 'warming';
       stats.phaseRemainingMs = Math.max(0, st.warmupDueMs - ctx.now);