diff --git a/.changeset/autorestart-covers-thrown-health-checks.md b/.changeset/autorestart-covers-thrown-health-checks.md new file mode 100644 index 0000000000..818cc68715 --- /dev/null +++ b/.changeset/autorestart-covers-thrown-health-checks.md @@ -0,0 +1,30 @@ +--- +"@objectstack/core": patch +--- + +fix(core): `autoRestart` now fires for a health check that throws or times out, not only for one that returns a failure (#11852) + +`PluginHealthMonitor.performHealthCheck` reaches its failure handling by two +disjoint routes, and only one of them could ever restart the plugin. + +A check that **returned** a failure (`false` or `{ status: 'unhealthy' }`) +incremented `failureCounters`, cleared `successCounters`, and — once +`failureThreshold` consecutive failures accumulated — consulted `autoRestart` +and restarted the plugin. A check that **threw** took a separate `catch` block +that incremented `failureCounters` and stopped there: it never cleared +`successCounters` and never read `autoRestart`. Because `raceCheckTimeout` +rejects rather than resolving, every `timeout` overrun lands in that `catch`, +so a plugin that hung was marked `failed` and never restarted no matter how +many rounds passed or what `autoRestart` said. The severer of the two failure +modes was the one that could not trigger recovery. + +Both routes now funnel into one `recordFailedRound` step that owns the +counters, the `failureThreshold` comparison and the `autoRestart` decision, so +a thrown or timed-out check is restart-eligible on exactly the same terms as a +returned failure. + +The per-route *status* label is deliberately unchanged: a throw is still the +separate `failed` status applied immediately with no threshold, as +`content/docs/protocol/kernel/lifecycle.mdx` documents. Only the counters and +the restart decision are shared — those are what `failureThreshold` and +`autoRestart` declare, and neither names a route. diff --git a/packages/core/src/health-monitor.test.ts b/packages/core/src/health-monitor.test.ts index 25d8c9cfe1..8c8b8d4fb0 100644 --- a/packages/core/src/health-monitor.test.ts +++ b/packages/core/src/health-monitor.test.ts @@ -246,4 +246,188 @@ describe('PluginHealthMonitor', () => { }); }); }); + + // #11852 — `autoRestart` covers BOTH failure routes, not only the milder one. + // + // `performHealthCheck` fails two disjoint ways: the check RETURNS a failure + // (`false` / `{ status: 'unhealthy' }`), or it THROWS — which, because + // `raceCheckTimeout` rejects rather than resolving, includes every `timeout` + // overrun. Only the returned route ever reached `config.autoRestart`, so a + // plugin that threw or hung until its timeout — the severer failure of the + // two — was marked `failed` and never restarted, whatever the config said. + // + // These pin the observable consequence, never the source: `attemptRestart` + // is the only caller of `plugin.destroy()` and the only writer of + // `recovering`, so those two readings together mean a restart happened and + // nothing else can produce them. Asserting that some shared helper was + // called would be a tautology any refactor could satisfy. + describe('autoRestart covers both failure routes (#11852)', () => { + /** `calculateBackoff(0, 'fixed')` — the delay before the first restart. */ + const FIRST_RESTART_BACKOFF_MS = 1_000; + + const restartConfig = ( + overrides: Partial = {} + ): PluginHealthCheckParsed => ({ + interval: 10_000, + timeout: 100, + failureThreshold: 2, + successThreshold: 1, + autoRestart: true, + maxRestartAttempts: 3, + restartBackoff: 'fixed', + checkMethod: 'healthCheck', + ...overrides, + }); + + const restartable = (name: string, healthCheck: () => unknown) => { + const destroyed = { count: 0 }; + const plugin = { + name, + version: '1.0.0', + init: () => {}, + destroy: async () => { + destroyed.count++; + }, + healthCheck, + } as unknown as Plugin; + return { plugin, destroyed }; + }; + + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it('restarts a plugin whose check THROWS, once failureThreshold accumulates', async () => { + const config = restartConfig(); + const { plugin, destroyed } = restartable('throwing-plugin', async () => { + throw new Error('check exploded'); + }); + + monitor.registerPlugin('throwing-plugin', config); + monitor.startMonitoring('throwing-plugin', plugin); + + // Round 1 (the immediate initial check) is below `failureThreshold`. + await vi.advanceTimersByTimeAsync(0); + expect(monitor.getHealthStatus('throwing-plugin')).toBe('failed'); + expect(destroyed.count).toBe(0); + + // Round 2 reaches the threshold and arms the restart's backoff. + await vi.advanceTimersByTimeAsync(config.interval); + await vi.advanceTimersByTimeAsync(0); + // Not yet: the restart waits out `restartBackoff` first. Without this the + // reading below could be "restarted eventually" rather than "restarted". + expect(destroyed.count).toBe(0); + + await vi.advanceTimersByTimeAsync(FIRST_RESTART_BACKOFF_MS); + expect(destroyed.count).toBe(1); + expect(monitor.getHealthStatus('throwing-plugin')).toBe('recovering'); + + monitor.stopMonitoring('throwing-plugin'); + }); + + it('restarts a plugin whose check exceeds `timeout` — the severest route', async () => { + const config = restartConfig(); + // Never settles: the timeout guard is what ends every round. + const { plugin, destroyed } = restartable( + 'hanging-plugin', + () => new Promise(() => {}) + ); + + monitor.registerPlugin('hanging-plugin', config); + monitor.startMonitoring('hanging-plugin', plugin); + + // Round 1's guard rejects at +timeout. Below threshold: no restart. + await vi.advanceTimersByTimeAsync(config.timeout); + expect(monitor.getHealthStatus('hanging-plugin')).toBe('failed'); + expect(destroyed.count).toBe(0); + + // Round 2 begins at +interval and its own guard rejects at +timeout. + await vi.advanceTimersByTimeAsync(config.interval - config.timeout); + await vi.advanceTimersByTimeAsync(config.timeout); + expect(destroyed.count).toBe(0); + + await vi.advanceTimersByTimeAsync(FIRST_RESTART_BACKOFF_MS); + expect(destroyed.count).toBe(1); + expect(monitor.getHealthStatus('hanging-plugin')).toBe('recovering'); + + // The round is still reported as the timeout it was — restarting it does + // not relabel why it failed. + const report = monitor.getHealthReport('hanging-plugin'); + expect(report?.message).toBe(`Health check timeout after ${config.timeout}ms`); + expect(report?.checks).toEqual([ + { name: 'health-check', status: 'failed', message: report?.message }, + ]); + + monitor.stopMonitoring('hanging-plugin'); + }); + + it('still restarts a plugin whose check RETURNS a failure', async () => { + // The route that already worked. Without this pin, unifying the two + // routes could close the throw gap by opening one here instead. + const config = restartConfig(); + const { plugin, destroyed } = restartable('unhealthy-plugin', async () => false); + + monitor.registerPlugin('unhealthy-plugin', config); + monitor.startMonitoring('unhealthy-plugin', plugin); + + await vi.advanceTimersByTimeAsync(0); + expect(monitor.getHealthStatus('unhealthy-plugin')).toBe('degraded'); + expect(destroyed.count).toBe(0); + + await vi.advanceTimersByTimeAsync(config.interval); + await vi.advanceTimersByTimeAsync(FIRST_RESTART_BACKOFF_MS); + expect(destroyed.count).toBe(1); + expect(monitor.getHealthStatus('unhealthy-plugin')).toBe('recovering'); + + monitor.stopMonitoring('unhealthy-plugin'); + }); + + it('leaves a throwing plugin alone when `autoRestart` is false', async () => { + // The control. Without it, the pins above would also pass if every + // failure restarted unconditionally — which would be a different defect, + // not a fix. + const config = restartConfig({ autoRestart: false }); + const { plugin, destroyed } = restartable('opted-out-plugin', async () => { + throw new Error('check exploded'); + }); + + monitor.registerPlugin('opted-out-plugin', config); + monitor.startMonitoring('opted-out-plugin', plugin); + + await vi.advanceTimersByTimeAsync(0); + await vi.advanceTimersByTimeAsync(config.interval); + await vi.advanceTimersByTimeAsync(FIRST_RESTART_BACKOFF_MS); + + expect(destroyed.count).toBe(0); + expect(monitor.getHealthStatus('opted-out-plugin')).toBe('failed'); + + monitor.stopMonitoring('opted-out-plugin'); + }); + + it('keeps a throw at `failed` immediately, with no threshold', async () => { + // The documented rule this fix deliberately does NOT unify away: + // "A check that throws — including one that exceeds `timeout` — is the + // separate `failed` status, applied immediately with no threshold" + // (content/docs/protocol/kernel/lifecycle.mdx, "Custom Health Checks"). + // Sharing the counters and the restart decision must not turn a throw + // into the returned route's `degraded`. + const config = restartConfig({ failureThreshold: 10 }); + const { plugin, destroyed } = restartable('strict-plugin', async () => { + throw new Error('check exploded'); + }); + + monitor.registerPlugin('strict-plugin', config); + monitor.startMonitoring('strict-plugin', plugin); + + await vi.advanceTimersByTimeAsync(0); + expect(monitor.getHealthStatus('strict-plugin')).toBe('failed'); + expect(destroyed.count).toBe(0); + + monitor.stopMonitoring('strict-plugin'); + }); + }); }); diff --git a/packages/core/src/health-monitor.ts b/packages/core/src/health-monitor.ts index 2ead09b295..4539396df7 100644 --- a/packages/core/src/health-monitor.ts +++ b/packages/core/src/health-monitor.ts @@ -103,6 +103,10 @@ export class PluginHealthMonitor { let status: PluginHealthStatus = 'healthy'; let message: string | undefined; const checks: Array<{ name: string; status: 'passed' | 'failed' | 'warning'; message?: string }> = []; + // Which failure route this round took, if any. A round can fail two + // disjoint ways and both settle here, so that the counters, the threshold + // and `autoRestart` are consulted in exactly one place below. + let failureRoute: 'returned' | 'thrown' | undefined; try { // Check if plugin has a custom health check method @@ -144,31 +148,12 @@ export class PluginHealthMonitor { this.healthStatus.set(pluginName, 'healthy'); } } else { - this.failureCounters.set(pluginName, (this.failureCounters.get(pluginName) || 0) + 1); - this.successCounters.set(pluginName, 0); - - const failureCount = this.failureCounters.get(pluginName) || 0; - if (failureCount >= config.failureThreshold) { - this.healthStatus.set(pluginName, 'unhealthy'); - this.logger.warn('Plugin marked as unhealthy', { - plugin: pluginName, - failures: failureCount - }); - - // Attempt auto-restart if configured - if (config.autoRestart) { - await this.attemptRestart(pluginName, plugin, config); - } - } else { - this.healthStatus.set(pluginName, 'degraded'); - } + failureRoute = 'returned'; } } catch (error) { status = 'failed'; message = error instanceof Error ? error.message : 'Unknown error'; - this.failureCounters.set(pluginName, (this.failureCounters.get(pluginName) || 0) + 1); - this.healthStatus.set(pluginName, 'failed'); - + checks.push({ name: 'health-check', status: 'failed', @@ -179,6 +164,16 @@ export class PluginHealthMonitor { plugin: pluginName, error }); + + failureRoute = 'thrown'; + } + + // Both failure routes land here, and only here. Deliberately outside the + // `try`: `recordFailedRound` may await a restart, and a fault raised by the + // restart is not a health-check exception — catching it above would relabel + // it as one and push a second `health-check` entry for a check that ran. + if (failureRoute) { + await this.recordFailedRound(pluginName, plugin, config, failureRoute); } // Create health report @@ -195,6 +190,55 @@ export class PluginHealthMonitor { this.healthReports.set(pluginName, report); } + /** + * Handle one failed round — the single path BOTH failure routes take. + * + * `performHealthCheck` can fail two disjoint ways: the check *returns* a + * failure (`false` or `{ status: 'unhealthy' }`), or it *throws* — which by + * `raceCheckTimeout` includes every `timeout` overrun, the severest case of + * the two. The routes used to be handled in separate blocks, and only the + * returned one cleared `successCounters` or consulted `autoRestart`, so a + * plugin that hung until its timeout was marked `failed` and never restarted + * however `autoRestart` was set: the declared key covered only the milder + * half of the failures it names. + * + * What stays route-specific is the *status label*, deliberately. A throw is + * the separate `failed` status applied immediately with no threshold — that + * is the documented contract (`content/docs/protocol/kernel/lifecycle.mdx`, + * "Custom Health Checks") and is pinned by the timeout test. Only the + * counters and the restart decision are shared, because those are what + * `failureThreshold` and `autoRestart` declare, and neither names a route. + */ + private async recordFailedRound( + pluginName: string, + plugin: Plugin, + config: PluginHealthCheckParsed, + route: 'returned' | 'thrown' + ): Promise { + const failureCount = (this.failureCounters.get(pluginName) || 0) + 1; + this.failureCounters.set(pluginName, failureCount); + this.successCounters.set(pluginName, 0); + + const thresholdReached = failureCount >= config.failureThreshold; + + if (route === 'thrown') { + this.healthStatus.set(pluginName, 'failed'); + } else if (thresholdReached) { + this.healthStatus.set(pluginName, 'unhealthy'); + this.logger.warn('Plugin marked as unhealthy', { + plugin: pluginName, + failures: failureCount + }); + } else { + this.healthStatus.set(pluginName, 'degraded'); + } + + // Attempt auto-restart if configured — route-blind, by the same threshold. + if (thresholdReached && config.autoRestart) { + await this.attemptRestart(pluginName, plugin, config); + } + } + /** * Attempt to restart a plugin */