From 0d75942a87087726fd0b77f35efa28dde885feed Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 7 Nov 2026 12:02:43 +0000 Subject: [PATCH 1/5] fix(metrics): a breaker state set before prom-client loads cannot overwrite one set after load Before load, setCircuitBreakerState keeps only the latest state and initialize() publishes it in the same run that sets isReady, beside the pending L1 stats, so no queued pre-load update resumes after a newer set. --- packages/cachekit/README.md | 8 +++---- packages/cachekit/src/cache.metrics.test.ts | 23 +++++++++++++++++++++ packages/cachekit/src/metrics/prometheus.ts | 22 ++++++++++++++------ 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/packages/cachekit/README.md b/packages/cachekit/README.md index 1fda53e..c53d952 100644 --- a/packages/cachekit/README.md +++ b/packages/cachekit/README.md @@ -893,10 +893,10 @@ last. prom-client loads in the background, starting on the event-loop turn after the cache is created, and no cache operation waits for it. Metrics recorded while it loads, including the first operations' durations, are recorded once it has -loaded. For the L1 gauges, a cache that wrote to L1 while loading publishes only -its latest L1 state, when its own load finishes. Until the next L1 write after -loading, a shared series can therefore read a cache other than the last one to -write. +loaded. For the L1 and circuit-breaker gauges, a cache that set them while +loading publishes only its latest values, when its own load finishes. Until the +next L1 write or operation after loading, a shared series can therefore read a +cache other than the last one to write. If `metrics` is enabled but `prom-client` is not installed, the SDK reports the failure once through the library logger and metrics degrade to no-ops — never diff --git a/packages/cachekit/src/cache.metrics.test.ts b/packages/cachekit/src/cache.metrics.test.ts index 05f9097..5c769ed 100644 --- a/packages/cachekit/src/cache.metrics.test.ts +++ b/packages/cachekit/src/cache.metrics.test.ts @@ -293,6 +293,29 @@ describe('Cache metrics wiring', () => { expect(await metricValue(registry, 'cachekit_l1_memory_bytes')).toBe(0); }); + // The same for the circuit-breaker gauge: a state set in the turn + // prom-client finishes registering lands after every state set before load. + it('a breaker state set as prom-client finishes loading is not overwritten by earlier ones', async () => { + let setAfterLoad = false; + class ObservedRegistry extends Registry { + override registerMetric(metric: Parameters[0]): void { + super.registerMetric(metric); + if ((metric as unknown as { name: string }).name === 'cachekit_circuit_breaker_state') + queueMicrotask(() => { + metrics.setCircuitBreakerState('closed'); + setAfterLoad = true; + }); + } + } + const registry = new ObservedRegistry(); + const metrics = new CacheMetrics({ registry }); + metrics.setCircuitBreakerState('open'); + expect(registry.getSingleMetric('cachekit_circuit_breaker_state')).toBeUndefined(); + await vi.waitFor(() => expect(setAfterLoad).toBe(true)); + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(await metricValue(registry, 'cachekit_circuit_breaker_state')).toBe(0); + }); + it('records hits and misses through wrap(), including the L1 SWR path', async () => { const registry = new Registry(); const cache = makeCache(registry); diff --git a/packages/cachekit/src/metrics/prometheus.ts b/packages/cachekit/src/metrics/prometheus.ts index 86f7c38..3879436 100644 --- a/packages/cachekit/src/metrics/prometheus.ts +++ b/packages/cachekit/src/metrics/prometheus.ts @@ -245,6 +245,8 @@ export class CacheMetrics implements MetricsCollector { private initFailed = false; /** The latest L1 stats set before prom-client loaded, published by initialize(). */ private pendingL1: [entries: number, memoryBytes: number] | null = null; + /** The latest breaker state set before prom-client loaded, published by initialize(). */ + private pendingBreaker: 'closed' | 'open' | 'half-open' | null = null; constructor(config: MetricsConfig = {}) { this.prefix = config.prefix ?? 'cachekit'; @@ -380,7 +382,9 @@ export class CacheMetrics implements MetricsCollector { this.isReady = true; if (this.pendingL1) this.writeL1Gauges(...this.pendingL1); + if (this.pendingBreaker) this.writeBreakerGauge(this.pendingBreaker); this.pendingL1 = null; + this.pendingBreaker = null; return true; } catch (error) { this.initError = error instanceof Error ? error : new Error(String(error)); @@ -649,13 +653,19 @@ export class CacheMetrics implements MetricsCollector { */ setCircuitBreakerState(state: 'closed' | 'open' | 'half-open'): void { if (this.isReady) { - try { - this.circuitBreakerGauge?.set(this.defaultLabels, breakerValue(state)); - } catch (error) { - this.handleError(error, 'setCircuitBreakerState'); - } + this.writeBreakerGauge(state); } else if (!this.initFailed) { - void this.updateCircuitBreakerState(state); + // Before load only the latest state matters, as in setL1Stats(). + if (this.pendingBreaker === null) void this.whenReady(); + this.pendingBreaker = state; + } + } + + private writeBreakerGauge(state: 'closed' | 'open' | 'half-open'): void { + try { + this.circuitBreakerGauge?.set(this.defaultLabels, breakerValue(state)); + } catch (error) { + this.handleError(error, 'setCircuitBreakerState'); } } } From 10adce6d491a681ce1323a268e9518b06651223b Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 7 Nov 2026 14:01:17 +0000 Subject: [PATCH 2/5] test(metrics): a breaker gauge error is reported and does not fail the load Covers the catch arm of writeBreakerGauge on both paths: initialize() publishing the pre-load state, and a state set after load. The gauge is registered by the application without the collector's default label, so real prom-client rejects the write. --- packages/cachekit/src/cache.metrics.test.ts | 24 +++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/packages/cachekit/src/cache.metrics.test.ts b/packages/cachekit/src/cache.metrics.test.ts index 5c769ed..164d912 100644 --- a/packages/cachekit/src/cache.metrics.test.ts +++ b/packages/cachekit/src/cache.metrics.test.ts @@ -316,6 +316,30 @@ describe('Cache metrics wiring', () => { expect(await metricValue(registry, 'cachekit_circuit_breaker_state')).toBe(0); }); + // A breaker gauge that rejects the write (here registered by the application + // without the default label) is reported, whether initialize() publishes the + // pre-load state or a state is set after load, and leaves the load succeeded. + it('a breaker gauge error is reported and does not fail the load', async () => { + const registry = new Registry(); + new Gauge({ name: 'cachekit_circuit_breaker_state', help: 'app-owned', registers: [registry] }); + const errors: Error[] = []; + const metrics = new CacheMetrics({ + registry, + defaultLabels: { service: 'api' }, + onError: (err) => errors.push(err), + }); + metrics.setCircuitBreakerState('open'); + await vi.waitFor(() => expect(errors).toHaveLength(1)); + metrics.setCircuitBreakerState('closed'); + expect(errors).toHaveLength(2); + expect(errors.map((e) => e.message)).toEqual([ + expect.stringContaining('"service"'), + expect.stringContaining('"service"'), + ]); + await metrics.recordError('timeout'); + expect(await metricValue(registry, 'cachekit_errors_total', { error_type: 'timeout' })).toBe(1); + }); + it('records hits and misses through wrap(), including the L1 SWR path', async () => { const registry = new Registry(); const cache = makeCache(registry); From aad1561194da12d42286de8769bd1cb93357c0f2 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 7 Nov 2026 15:59:00 +0000 Subject: [PATCH 3/5] fix(metrics): reporting a metric error never throws, whatever was thrown handleError and initialize() built an Error with String(error), which throws on a null-prototype object. A breaker or L1 gauge throwing such a value then failed the load during initialize()'s pending flush, and after load escaped the synchronous setters into the cache operation. Both now wrap the value through toError(), which falls back to a fixed message. --- packages/cachekit/src/cache.metrics.test.ts | 24 +++++++++++++++++++++ packages/cachekit/src/metrics/prometheus.ts | 16 ++++++++++++-- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/packages/cachekit/src/cache.metrics.test.ts b/packages/cachekit/src/cache.metrics.test.ts index 164d912..4be7ef5 100644 --- a/packages/cachekit/src/cache.metrics.test.ts +++ b/packages/cachekit/src/cache.metrics.test.ts @@ -340,6 +340,30 @@ describe('Cache metrics wiring', () => { expect(await metricValue(registry, 'cachekit_errors_total', { error_type: 'timeout' })).toBe(1); }); + // String() throws on a null-prototype object, so reporting one must not. + it('a breaker gauge throwing a value String() rejects is reported and fails nothing', async () => { + const registry = new Registry(); + const gauge = new Gauge({ + name: 'cachekit_circuit_breaker_state', + help: 'app-owned', + registers: [registry], + }); + gauge.set = () => { + throw Object.create(null); + }; + const errors: Error[] = []; + const metrics = new CacheMetrics({ registry, onError: (err) => errors.push(err) }); + metrics.setCircuitBreakerState('open'); + await vi.waitFor(() => expect(errors).toHaveLength(1)); + expect(() => metrics.setCircuitBreakerState('closed')).not.toThrow(); + expect(errors.map((e) => e.message)).toEqual([ + 'a value that cannot be converted to a string was thrown', + 'a value that cannot be converted to a string was thrown', + ]); + await metrics.recordError('timeout'); + expect(await metricValue(registry, 'cachekit_errors_total', { error_type: 'timeout' })).toBe(1); + }); + it('records hits and misses through wrap(), including the L1 SWR path', async () => { const registry = new Registry(); const cache = makeCache(registry); diff --git a/packages/cachekit/src/metrics/prometheus.ts b/packages/cachekit/src/metrics/prometheus.ts index 3879436..90bed08 100644 --- a/packages/cachekit/src/metrics/prometheus.ts +++ b/packages/cachekit/src/metrics/prometheus.ts @@ -387,7 +387,7 @@ export class CacheMetrics implements MetricsCollector { this.pendingBreaker = null; return true; } catch (error) { - this.initError = error instanceof Error ? error : new Error(String(error)); + this.initError = toError(error); return false; } } @@ -437,7 +437,7 @@ export class CacheMetrics implements MetricsCollector { * Proper error handling instead of silent failures. */ private handleError(error: unknown, context: string): void { - const err = error instanceof Error ? error : new Error(String(error)); + const err = toError(error); if (!this.invokeErrorHandler(err)) { logError(`[cachekit] Metrics error (${context}):`, err.message); @@ -673,6 +673,18 @@ export class CacheMetrics implements MetricsCollector { const breakerValue = (state: 'closed' | 'open' | 'half-open'): number => state === 'closed' ? 0 : state === 'half-open' ? 0.5 : 1; +/** Wrap a thrown value as an Error without throwing: String() itself throws on + * some values (a null-prototype object), and every caller is an error path + * that must not escape. */ +function toError(error: unknown): Error { + if (error instanceof Error) return error; + try { + return new Error(String(error)); + } catch { + return new Error('a value that cannot be converted to a string was thrown'); + } +} + /** * No-op metrics implementation for when metrics are disabled. */ From 72d2a3312b2791efff1db82a6f6c3b1679003e7b Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 7 Nov 2026 16:01:57 +0000 Subject: [PATCH 4/5] fix(metrics): wrapping a thrown value survives a Proxy that throws on instanceof toError() tested instanceof outside its try, and instanceof calls a Proxy's getPrototypeOf trap, which can throw. The whole conversion now sits inside the try. --- packages/cachekit/src/cache.metrics.test.ts | 23 ++++++++++++++++----- packages/cachekit/src/metrics/prometheus.ts | 11 +++++----- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/packages/cachekit/src/cache.metrics.test.ts b/packages/cachekit/src/cache.metrics.test.ts index 4be7ef5..2bc72fa 100644 --- a/packages/cachekit/src/cache.metrics.test.ts +++ b/packages/cachekit/src/cache.metrics.test.ts @@ -340,25 +340,38 @@ describe('Cache metrics wiring', () => { expect(await metricValue(registry, 'cachekit_errors_total', { error_type: 'timeout' })).toBe(1); }); - // String() throws on a null-prototype object, so reporting one must not. - it('a breaker gauge throwing a value String() rejects is reported and fails nothing', async () => { + // String() throws on a null-prototype object, and instanceof on a Proxy whose + // getPrototypeOf trap throws, so reporting either must not. + it('a breaker gauge throwing a value no Error can wrap is reported and fails nothing', async () => { const registry = new Registry(); const gauge = new Gauge({ name: 'cachekit_circuit_breaker_state', help: 'app-owned', registers: [registry], }); + const thrown: unknown[] = [ + Object.create(null), + new Proxy( + {}, + { + getPrototypeOf: () => { + throw new Error('trap'); + }, + } + ), + ]; gauge.set = () => { - throw Object.create(null); + throw thrown.shift(); }; const errors: Error[] = []; const metrics = new CacheMetrics({ registry, onError: (err) => errors.push(err) }); metrics.setCircuitBreakerState('open'); await vi.waitFor(() => expect(errors).toHaveLength(1)); expect(() => metrics.setCircuitBreakerState('closed')).not.toThrow(); + expect(thrown).toEqual([]); expect(errors.map((e) => e.message)).toEqual([ - 'a value that cannot be converted to a string was thrown', - 'a value that cannot be converted to a string was thrown', + 'a thrown value could not be converted to an Error', + 'a thrown value could not be converted to an Error', ]); await metrics.recordError('timeout'); expect(await metricValue(registry, 'cachekit_errors_total', { error_type: 'timeout' })).toBe(1); diff --git a/packages/cachekit/src/metrics/prometheus.ts b/packages/cachekit/src/metrics/prometheus.ts index 90bed08..004d7c2 100644 --- a/packages/cachekit/src/metrics/prometheus.ts +++ b/packages/cachekit/src/metrics/prometheus.ts @@ -673,15 +673,14 @@ export class CacheMetrics implements MetricsCollector { const breakerValue = (state: 'closed' | 'open' | 'half-open'): number => state === 'closed' ? 0 : state === 'half-open' ? 0.5 : 1; -/** Wrap a thrown value as an Error without throwing: String() itself throws on - * some values (a null-prototype object), and every caller is an error path - * that must not escape. */ +/** Wrap a thrown value as an Error without throwing: String() throws on a + * null-prototype object, and instanceof on a Proxy whose getPrototypeOf trap + * throws. Every caller is an error path that must not escape. */ function toError(error: unknown): Error { - if (error instanceof Error) return error; try { - return new Error(String(error)); + return error instanceof Error ? error : new Error(String(error)); } catch { - return new Error('a value that cannot be converted to a string was thrown'); + return new Error('a thrown value could not be converted to an Error'); } } From a4f26fb02f28b4c974f7d3bfbc952e603a09deb6 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 7 Nov 2026 16:12:15 +0000 Subject: [PATCH 5/5] fix(metrics): reporting a metric error survives an Error whose message getter throws toError() passes an Error through unchanged, so with no onError handler the library logger's message read could still throw: out of the post-load breaker setter, into initialize()'s catch from the pending flush, or as an unhandled rejection from the pre-load setter's readiness waiter. Both logger sites now read the message through messageOf(), which falls back to a fixed string. --- packages/cachekit/src/cache.metrics.test.ts | 53 +++++++++++++++++++++ packages/cachekit/src/metrics/prometheus.ts | 14 +++++- 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/packages/cachekit/src/cache.metrics.test.ts b/packages/cachekit/src/cache.metrics.test.ts index 2bc72fa..9d48fd2 100644 --- a/packages/cachekit/src/cache.metrics.test.ts +++ b/packages/cachekit/src/cache.metrics.test.ts @@ -3,6 +3,7 @@ import { Counter, Gauge, Registry } from 'prom-client'; import { createCache } from './cache.js'; import { ConfigurationError } from './errors.js'; import { CacheMetrics } from './metrics/prometheus.js'; +import { setLogger } from './logger.js'; import type { SecureCache } from './types/cache.js'; import type { Backend } from './backends/types.js'; import { InMemoryBackend, metricValue } from '../test/fixtures/metrics.js'; @@ -29,6 +30,14 @@ class FailingBackend implements Backend { async close(): Promise {} } +/** An Error whose message getter throws. */ +const unreadableError = (): Error => + Object.defineProperty(new Error(), 'message', { + get() { + throw new Error('message getter threw'); + }, + }); + describe('Cache metrics wiring', () => { const caches: SecureCache[] = []; @@ -49,6 +58,7 @@ describe('Cache metrics wiring', () => { afterEach(async () => { await Promise.all(caches.splice(0).map((c) => c.close())); + setLogger(null); }); it('records misses and get operations', async () => { @@ -377,6 +387,49 @@ describe('Cache metrics wiring', () => { expect(await metricValue(registry, 'cachekit_errors_total', { error_type: 'timeout' })).toBe(1); }); + // With no onError the library logger gets the error's message, and reading + // it can throw too, so neither report may let that escape. + it('a breaker gauge throwing an Error whose message getter throws is logged and fails nothing', async () => { + const logs: unknown[][] = []; + setLogger((message, error) => logs.push([message, error])); + const registry = new Registry(); + const gauge = new Gauge({ + name: 'cachekit_circuit_breaker_state', + help: 'app-owned', + registers: [registry], + }); + gauge.set = () => { + throw unreadableError(); + }; + const metrics = new CacheMetrics({ registry }); + metrics.setCircuitBreakerState('open'); + await vi.waitFor(() => expect(logs).toHaveLength(1)); + expect(() => metrics.setCircuitBreakerState('closed')).not.toThrow(); + expect(logs).toEqual([ + ['[cachekit] Metrics error (setCircuitBreakerState):', 'the error message could not be read'], + ['[cachekit] Metrics error (setCircuitBreakerState):', 'the error message could not be read'], + ]); + await metrics.recordError('timeout'); + expect(await metricValue(registry, 'cachekit_errors_total', { error_type: 'timeout' })).toBe(1); + }); + + it('a load failing with an Error whose message getter throws is logged once and rejects nothing', async () => { + const logs: unknown[][] = []; + setLogger((message, error) => logs.push([message, error])); + class FailingRegistry extends Registry { + override registerMetric(): void { + throw unreadableError(); + } + } + const metrics = new CacheMetrics({ registry: new FailingRegistry() }); + metrics.setCircuitBreakerState('open'); + await vi.waitFor(() => expect(logs).toHaveLength(1)); + await expect(metrics.recordMiss()).resolves.toBeUndefined(); + expect(logs).toEqual([ + [expect.stringContaining('failed to initialize'), 'the error message could not be read'], + ]); + }); + it('records hits and misses through wrap(), including the L1 SWR path', async () => { const registry = new Registry(); const cache = makeCache(registry); diff --git a/packages/cachekit/src/metrics/prometheus.ts b/packages/cachekit/src/metrics/prometheus.ts index 004d7c2..5efa7ec 100644 --- a/packages/cachekit/src/metrics/prometheus.ts +++ b/packages/cachekit/src/metrics/prometheus.ts @@ -408,7 +408,7 @@ export class CacheMetrics implements MetricsCollector { logError( '[cachekit] metrics are enabled but failed to initialize — install the optional ' + '`prom-client` peer dependency or set `metrics: false`:', - err.message + messageOf(err) ); } } @@ -440,7 +440,7 @@ export class CacheMetrics implements MetricsCollector { const err = toError(error); if (!this.invokeErrorHandler(err)) { - logError(`[cachekit] Metrics error (${context}):`, err.message); + logError(`[cachekit] Metrics error (${context}):`, messageOf(err)); } } @@ -684,6 +684,16 @@ function toError(error: unknown): Error { } } +/** Read an Error's message without throwing: toError() passes an Error + * through unchanged, and its message getter can throw too. */ +function messageOf(err: Error): string { + try { + return err.message; + } catch { + return 'the error message could not be read'; + } +} + /** * No-op metrics implementation for when metrics are disabled. */