fix(core): make successThreshold bind from every status that records a failure

.changeset/success-threshold-binds-from-every-failed-status.md

@@ -0,0 +1,49 @@
+---
+"@objectstack/core": patch
+---
+
+fix(core): `successThreshold` now binds from every status that records a failure, so a declared count above 2 stops being unreachable (#11955)
+
+`PluginHealthMonitor` consulted `successThreshold` only while a plugin's status
+was `unhealthy` or `degraded`. The first success in a recovery wrote
+`recovering` — a status that gate did not name — so the **second** success took
+the outer `else` and went straight to `healthy` without the counter being read
+at all. `failed` was in neither set either, so a plugin whose check threw
+recovered on its **first** success.
+
+The declared value was therefore capped in practice:
+
+| Status when the successes start | Consecutive successes actually required |
+| :--- | :--- |
+| `unhealthy` / `degraded` | 2, whatever `successThreshold` said |
+| `failed` / `recovering` | 1, whatever `successThreshold` said |
+
+A declared `successThreshold: 5` was indistinguishable from `2`. The default is
+`1`, which is exactly the value at which the defect is invisible — every
+declared value above it was the one that misbehaved.
+
+The counter is now consulted on the way out of every status that records an
+observed failure — `degraded`, `unhealthy`, `failed` and `recovering` — so
+`successThreshold: N` requires N consecutive successes from each of them, as
+its declaration says ("Consecutive successes needed to mark healthy"). The gate
+is a map that is exhaustive over `PluginHealthStatus`, so a status added to the
+spec fails to compile until it is placed on one side or the other; that is what
+`recovering` slipped through before.
+
+`healthy` and `unknown` still promote on the first success, deliberately: the
+count is declared as a **recovery** criterion ("Number of consecutive successes
+to recover from unhealthy state") and neither of those records a failure to
+recover from — `unknown` is the status `registerPlugin` writes before any check
+has run.
+
+**Behaviour change, only for configs that declare `successThreshold` above 1.**
+At the default `1` every route is byte-for-byte what it was: one success has
+always been enough and still is. A plugin declaring a higher count now takes
+the number of consecutive successes it asked for before it is reported
+`healthy`, including after a `failed` round and after an `autoRestart`.
+
+This also makes #11852's `successCounters` reset load-bearing. That fix cleared
+the counter on the thrown failure route, and could not be pinned: the counter's
+only read site was unreachable with a stale non-zero value, so any test would
+have passed for the wrong reason. With `failed` gated on the counter, a throw
+that interrupts a recovery now demonstrably starts the count over.

packages/core/src/health-monitor.test.ts

@@ -430,4 +430,239 @@ describe('PluginHealthMonitor', () => {
  monitor.stopMonitoring('strict-plugin');
  });
  });
+
+ // `successThreshold` is declared as "Consecutive successes needed to mark
+ // healthy", and was read only while status was `unhealthy` or `degraded`.
+ // The first success wrote `recovering` — a status the gate did not name — so
+ // the second success took the outer `else` and reached `healthy` without the
+ // counter being consulted at all; `failed` was in neither set either, so a
+ // plugin that threw recovered on its FIRST success. A declared 5 was
+ // indistinguishable from 2, and from 1 when recovery started at `failed`.
+ //
+ // Every case below declares `successThreshold: 3`, never the default `1` —
+ // 1 is exactly the value at which the defect is invisible.
+ //
+ // These read `getHealthStatus` only. Asserting that some status set is
+ // consulted would be a tautology any refactor could satisfy; what the
+ // declaration promises is the number of consecutive successes a plugin needs
+ // before it is called healthy, so that is what is counted.
+ describe('`successThreshold` binds from every status that records a failure (#11955)', () => {
+ const THRESHOLD = 3;
+ const INTERVAL_MS = 10_000;
+ /** `calculateBackoff(0, 'fixed')` — the delay before the first restart. */
+ const FIRST_RESTART_BACKOFF_MS = 1_000;
+
+ const thresholdConfig = (
+ overrides: Partial<PluginHealthCheckParsed> = {}
+ ): PluginHealthCheckParsed => ({
+ interval: INTERVAL_MS,
+ timeout: 100,
+ failureThreshold: 2,
+ successThreshold: THRESHOLD,
+ autoRestart: false,
+ maxRestartAttempts: 3,
+ restartBackoff: 'fixed',
+ checkMethod: 'healthCheck',
+ ...overrides,
+ });
+
+ type CheckMode = 'pass' | 'return-failure' | 'throw';
+
+ /** A plugin whose check outcome is switched between rounds. */
+ const switchable = (name: string, initial: CheckMode) => {
+ const mode = { current: initial };
+ const destroyed = { count: 0 };
+ const plugin = {
+ name,
+ version: '1.0.0',
+ init: () => {},
+ destroy: async () => {
+ destroyed.count++;
+ },
+ healthCheck: async () => {
+ if (mode.current === 'throw') throw new Error('check exploded');
+ if (mode.current === 'return-failure') return false;
+ return true;
+ },
+ } as unknown as Plugin;
+ return { plugin, mode, destroyed };
+ };
+
+ /** Advance to the next scheduled round and let its check settle. */
+ const nextRound = async () => {
+ await vi.advanceTimersByTimeAsync(INTERVAL_MS);
+ await vi.advanceTimersByTimeAsync(0);
+ };
+
+ beforeEach(() => {
+ vi.useFakeTimers();
+ });
+
+ afterEach(() => {
+ vi.useRealTimers();
+ });
+
+ it('requires all three successes to leave `unhealthy`', async () => {
+ const config = thresholdConfig();
+ const { plugin, mode } = switchable('unhealthy-plugin', 'return-failure');
+
+ monitor.registerPlugin('unhealthy-plugin', config);
+ monitor.startMonitoring('unhealthy-plugin', plugin);
+
+ await vi.advanceTimersByTimeAsync(0);
+ expect(monitor.getHealthStatus('unhealthy-plugin')).toBe('degraded');
+ await nextRound();
+ expect(monitor.getHealthStatus('unhealthy-plugin')).toBe('unhealthy');
+
+ mode.current = 'pass';
+ await nextRound();
+ expect(monitor.getHealthStatus('unhealthy-plugin')).toBe('recovering');
+ // The round that used to promote: status is `recovering` going in, which
+ // the old gate did not name, so the counter went unread and 2 of 3
+ // successes was enough.
+ await nextRound();
+ expect(monitor.getHealthStatus('unhealthy-plugin')).toBe('recovering');
+
+ await nextRound();
+ expect(monitor.getHealthStatus('unhealthy-plugin')).toBe('healthy');
+
+ monitor.stopMonitoring('unhealthy-plugin');
+ });
+
+ it('requires all three successes to leave `degraded`', async () => {
+ // `failureThreshold: 5` keeps the single failure below `unhealthy`, so
+ // the successes start from `degraded` itself.
+ const config = thresholdConfig({ failureThreshold: 5 });
+ const { plugin, mode } = switchable('degraded-plugin', 'return-failure');
+
+ monitor.registerPlugin('degraded-plugin', config);
+ monitor.startMonitoring('degraded-plugin', plugin);
+
+ await vi.advanceTimersByTimeAsync(0);
+ expect(monitor.getHealthStatus('degraded-plugin')).toBe('degraded');
+
+ mode.current = 'pass';
+ await nextRound();
+ expect(monitor.getHealthStatus('degraded-plugin')).toBe('recovering');
+ await nextRound();
+ expect(monitor.getHealthStatus('degraded-plugin')).toBe('recovering');
+ await nextRound();
+ expect(monitor.getHealthStatus('degraded-plugin')).toBe('healthy');
+
+ monitor.stopMonitoring('degraded-plugin');
+ });
+
+ it('requires all three successes to leave `failed` — not one', async () => {
+ // `failed` was in neither set the gate named, so the first success after
+ // a throw went straight to `healthy` whatever the declared count said.
+ const config = thresholdConfig({ failureThreshold: 5 });
+ const { plugin, mode } = switchable('thrown-plugin', 'throw');
+
+ monitor.registerPlugin('thrown-plugin', config);
+ monitor.startMonitoring('thrown-plugin', plugin);
+
+ await vi.advanceTimersByTimeAsync(0);
+ expect(monitor.getHealthStatus('thrown-plugin')).toBe('failed');
+
+ mode.current = 'pass';
+ await nextRound();
+ expect(monitor.getHealthStatus('thrown-plugin')).toBe('recovering');
+ await nextRound();
+ expect(monitor.getHealthStatus('thrown-plugin')).toBe('recovering');
+ await nextRound();
+ expect(monitor.getHealthStatus('thrown-plugin')).toBe('healthy');
+
+ monitor.stopMonitoring('thrown-plugin');
+ });
+
+ it('requires all three successes to leave the `recovering` a restart wrote', async () => {
+ // `recovering` reached from the restart path rather than from the
+ // success branch: `attemptRestart` writes it with both counters zeroed,
+ // so this is the status as a genuine STARTING point, not as a value the
+ // gate under test just produced.
+ const config = thresholdConfig({ failureThreshold: 1, autoRestart: true });
+ const { plugin, mode, destroyed } = switchable('restarted-plugin', 'throw');
+
+ monitor.registerPlugin('restarted-plugin', config);
+ monitor.startMonitoring('restarted-plugin', plugin);
+
+ await vi.advanceTimersByTimeAsync(0);
+ // The restart waits out `restartBackoff` before it destroys.
+ expect(destroyed.count).toBe(0);
+ await vi.advanceTimersByTimeAsync(FIRST_RESTART_BACKOFF_MS);
+ expect(destroyed.count).toBe(1);
+ expect(monitor.getHealthStatus('restarted-plugin')).toBe('recovering');
+
+ mode.current = 'pass';
+ await nextRound();
+ expect(monitor.getHealthStatus('restarted-plugin')).toBe('recovering');
+ await nextRound();
+ expect(monitor.getHealthStatus('restarted-plugin')).toBe('recovering');
+ await nextRound();
+ expect(monitor.getHealthStatus('restarted-plugin')).toBe('healthy');
+
+ monitor.stopMonitoring('restarted-plugin');
+ });
+
+ it('starts the count over after a throw interrupts a recovery (#11852)', async () => {
+ // The pin #11852 correctly declined to fake. That card fixed the `catch`
+ // path's missing `successCounters` reset and could not test it: the
+ // counter's only read site was unreachable with a stale non-zero value,
+ // so any test would have passed for the wrong reason. Gating `failed` on
+ // the counter is what makes the reset observable — and load-bearing.
+ //
+ // Two successes accumulate, then the check throws. With the reset, the
+ // recovery restarts from zero and three more successes are needed; with
+ // the stale counter the third success below would carry the count to 3
+ // and promote immediately.
+ const config = thresholdConfig({ failureThreshold: 5 });
+ const { plugin, mode } = switchable('interrupted-plugin', 'return-failure');
+
+ monitor.registerPlugin('interrupted-plugin', config);
+ monitor.startMonitoring('interrupted-plugin', plugin);
+
+ await vi.advanceTimersByTimeAsync(0);
+ expect(monitor.getHealthStatus('interrupted-plugin')).toBe('degraded');
+
+ mode.current = 'pass';
+ await nextRound();
+ await nextRound();
+ expect(monitor.getHealthStatus('interrupted-plugin')).toBe('recovering');
+
+ mode.current = 'throw';
+ await nextRound();
+ expect(monitor.getHealthStatus('interrupted-plugin')).toBe('failed');
+
+ mode.current = 'pass';
+ await nextRound();
+ // Success #1 of a fresh three, not #3 of a carried-over count.
+ expect(monitor.getHealthStatus('interrupted-plugin')).toBe('recovering');
+ await nextRound();
+ expect(monitor.getHealthStatus('interrupted-plugin')).toBe('recovering');
+ await nextRound();
+ expect(monitor.getHealthStatus('interrupted-plugin')).toBe('healthy');
+
+ monitor.stopMonitoring('interrupted-plugin');
+ });
+
+ it('marks a never-checked plugin healthy on its first success', async () => {
+ // The boundary of the chosen semantics, pinned so it cannot drift: the
+ // count is a RECOVERY criterion ("Number of consecutive successes to
+ // recover from unhealthy state"), and `unknown` — declared "Health
+ // status cannot be determined", the status `registerPlugin` writes —
+ // records no failure to recover from. A fresh plugin is healthy on its
+ // first passing check however high `successThreshold` is declared.
+ const config = thresholdConfig();
+ const { plugin } = switchable('fresh-plugin', 'pass');
+
+ monitor.registerPlugin('fresh-plugin', config);
+ expect(monitor.getHealthStatus('fresh-plugin')).toBe('unknown');
+
+ monitor.startMonitoring('fresh-plugin', plugin);
+ await vi.advanceTimersByTimeAsync(0);
+ expect(monitor.getHealthStatus('fresh-plugin')).toBe('healthy');
+
+ monitor.stopMonitoring('fresh-plugin');
+ });
+ });
 });

packages/core/src/health-monitor.ts

@@ -8,6 +8,40 @@ import type {
 import type { ObjectLogger } from './logger.js';
 import type { Plugin } from './types.js';
 
+/**
+ * Is a success from `status` still subject to `successThreshold`?
+ *
+ * `successThreshold` is declared as "Consecutive successes needed to mark
+ * healthy" (`PluginHealthCheckSchema`, `@objectstack/spec/kernel`), so the
+ * counter stays in force on the way out of every status that records an
+ * OBSERVED failure — `recovering` included. `recovering` is declared "Plugin
+ * is in recovery process": recovery under way, not finished, and the count is
+ * exactly its completion criterion. Consulting the counter only from
+ * `unhealthy`/`degraded` capped the declared value at 2, because the first
+ * success moved the plugin to `recovering` — a status the gate did not name —
+ * so the second success bypassed the counter entirely and went straight to
+ * `healthy`. `failed` was skipped for the same reason and recovered on its
+ * first success (#11955).
+ *
+ * `healthy` and `unknown` promote on the first success: neither records a
+ * failure to recover from. `unknown` is declared "Health status cannot be
+ * determined" — the status a plugin is registered in before any check has run,
+ * so there is nothing for the recovery count to count back from.
+ *
+ * Exhaustive over `PluginHealthStatus` deliberately: a status added to
+ * `PluginHealthStatusSchema` fails to compile here until this map says which
+ * side it falls on, so the gate cannot quietly acquire a second bypass the way
+ * `recovering` did.
+ */
+const RECOVERY_IS_THRESHOLD_GATED: Record<PluginHealthStatus, boolean> = {
+ degraded: true,
+ unhealthy: true,
+ failed: true,
+ recovering: true,
+ healthy: false,
+ unknown: false,
+};
+
 /**
  * Plugin Health Monitor
  * 
@@ -134,9 +168,11 @@ export class PluginHealthMonitor {
  this.successCounters.set(pluginName, (this.successCounters.get(pluginName) || 0) + 1);
  this.failureCounters.set(pluginName, 0);
 
- // Recover from unhealthy state if we have enough successes
- const currentStatus = this.healthStatus.get(pluginName);
- if (currentStatus === 'unhealthy' || currentStatus === 'degraded') {
+ // Recover only once `successThreshold` consecutive successes have
+ // accumulated — from every status that records an observed failure,
+ // not just the two the gate used to name (#11955).
+ const currentStatus = this.healthStatus.get(pluginName) ?? 'unknown';
+ if (RECOVERY_IS_THRESHOLD_GATED[currentStatus]) {
  const successCount = this.successCounters.get(pluginName) || 0;
  if (successCount >= config.successThreshold) {
  this.healthStatus.set(pluginName, 'healthy');