fix(metrics,retry): gold standard code review corrections

fix(metrics,retry): gold standard code review corrections

src/Utils/prometheus-metrics.ts

@@ -20,13 +20,16 @@
  * - Circuit breaker metrics
  * - Environment variable configuration
  *
- * Configuration via environment variables:
+ * Configuration via environment variables (supports BAILEYS_PROMETHEUS_* and METRICS_* prefixes):
  * - BAILEYS_PROMETHEUS_ENABLED: Enable/disable metrics (default: false)
  * - BAILEYS_PROMETHEUS_PORT: Port for HTTP metrics server (default: 9092)
+ * - BAILEYS_PROMETHEUS_HOST: Host/IP to bind (default: 127.0.0.1)
  * - BAILEYS_PROMETHEUS_PATH: Path for metrics endpoint (default: /metrics)
  * - BAILEYS_PROMETHEUS_PREFIX: Prefix for all metrics (default: baileys)
  * - BAILEYS_PROMETHEUS_LABELS: JSON string with default labels (e.g. {"environment":"production"})
  * - BAILEYS_PROMETHEUS_COLLECT_DEFAULT: Collect default Node.js metrics (default: true)
+ * - BAILEYS_PROMETHEUS_INCLUDE_SYSTEM: Include system metrics like CPU/memory (default: true)
+ * - BAILEYS_PROMETHEUS_COLLECT_INTERVAL_MS: Interval for system metrics collection (default: 10000)
  *
  * @module Utils/prometheus-metrics
  */
@@ -140,8 +143,11 @@ export function loadMetricsConfig(): MetricsConfig {
 		host: process.env.BAILEYS_PROMETHEUS_HOST || process.env.METRICS_HOST || '127.0.0.1',
 		path: process.env.BAILEYS_PROMETHEUS_PATH || process.env.METRICS_PATH || '/metrics',
 		prefix: process.env.BAILEYS_PROMETHEUS_PREFIX || process.env.METRICS_PREFIX || 'baileys',
-		defaultLabels: parseLabelsFromEnv(process.env.BAILEYS_PROMETHEUS_LABELS),
+		// Support both env prefixes for labels
-		includeSystem: (process.env.BAILEYS_PROMETHEUS_COLLECT_DEFAULT ?? process.env.METRICS_INCLUDE_SYSTEM) !== 'false',
+		defaultLabels: parseLabelsFromEnv(process.env.BAILEYS_PROMETHEUS_LABELS ?? process.env.METRICS_LABELS),
+		// Separate flag for system metrics (CPU/memory) - independent from collectDefaultMetrics
+		includeSystem: (process.env.BAILEYS_PROMETHEUS_INCLUDE_SYSTEM ?? process.env.METRICS_INCLUDE_SYSTEM) !== 'false',
+		// Flag for prom-client default Node.js metrics
 		collectDefaultMetrics: (process.env.BAILEYS_PROMETHEUS_COLLECT_DEFAULT ?? process.env.METRICS_COLLECT_DEFAULT) !== 'false',
 		collectIntervalMs: parseInt(process.env.BAILEYS_PROMETHEUS_COLLECT_INTERVAL_MS || process.env.METRICS_COLLECT_INTERVAL_MS || '10000', 10),
 	}
@@ -161,6 +167,19 @@ export type MetricType = 'counter' | 'gauge' | 'histogram' | 'summary'
  */
 export type Labels = Record<string, string>
 
+/**
+ * Convert labels to a stable key string for lookups
+ * Sorts keys to ensure consistent ordering regardless of object property order
+ */
+function stableLabelsToKey(labels: Labels): string {
+	const sortedKeys = Object.keys(labels).sort()
+	const sortedObj: Labels = {}
+	for (const key of sortedKeys) {
+		sortedObj[key] = labels[key]!
+	}
+	return JSON.stringify(sortedObj)
+}
+
 /**
  * Default histogram buckets (in ms)
  */
@@ -312,7 +331,7 @@ export class Counter implements BaseMetric {
 	}
 
 	private labelsToKey(labels: Labels): string {
-		return JSON.stringify(labels)
+		return stableLabelsToKey(labels)
 	}
 
 	/**
@@ -462,7 +481,7 @@ export class Gauge implements BaseMetric {
 	}
 
 	private labelsToKey(labels: Labels): string {
-		return JSON.stringify(labels)
+		return stableLabelsToKey(labels)
 	}
 
 	/**
@@ -662,7 +681,7 @@ export class Histogram implements BaseMetric {
 	}
 
 	private labelsToKey(labels: Labels): string {
-		return JSON.stringify(labels)
+		return stableLabelsToKey(labels)
 	}
 
 	/**
@@ -831,7 +850,7 @@ export class Summary implements BaseMetric {
 	}
 
 	private labelsToKey(labels: Labels): string {
-		return JSON.stringify(labels)
+		return stableLabelsToKey(labels)
 	}
 
 	/**
@@ -1202,7 +1221,26 @@ export class MetricsServer {
 			}
 
 			this.server = createServer(async (req: IncomingMessage, res: ServerResponse) => {
-				if (req.url === this.config.path && req.method === 'GET') {
+				// Parse URL to handle querystrings and trailing slashes
+				// Wrapped in try/catch to handle malformed URLs gracefully
+				let pathname: string
+				try {
+					const parsedUrl = new URL(req.url || '/', `http://${req.headers.host || 'localhost'}`)
+					pathname = parsedUrl.pathname.replace(/\/+$/, '') || '/'
+				} catch {
+					// Malformed URL - return 400 Bad Request
+					res.writeHead(400, { 'Content-Type': 'application/json' })
+					res.end(JSON.stringify({
+						error: 'Bad Request',
+						message: 'Malformed URL',
+						timestamp: new Date().toISOString()
+					}))
+					return
+				}
+
+				const configPath = this.config.path.replace(/\/+$/, '') || '/'
+
+				if (pathname === configPath && req.method === 'GET') {
 					try {
 						// Collect system metrics before responding
 						this.systemCollector?.collect()
@@ -1211,7 +1249,6 @@ export class MetricsServer {
 						res.writeHead(200, { 'Content-Type': this.registry.contentType() })
 						res.end(metricsOutput)
 					} catch (error) {
-						// FIX: More descriptive error message
 						const errorMessage = error instanceof Error ? error.message : 'Unknown error'
 						console.error(`[Prometheus] Error collecting metrics: ${errorMessage}`)
 						res.writeHead(500, { 'Content-Type': 'application/json' })
@@ -1221,7 +1258,7 @@ export class MetricsServer {
 							timestamp: new Date().toISOString()
 						}))
 					}
-				} else if (req.url === '/health' && req.method === 'GET') {
+				} else if (pathname === '/health' && req.method === 'GET') {
 					res.writeHead(200, { 'Content-Type': 'application/json' })
 					res.end(JSON.stringify({ status: 'ok', timestamp: new Date().toISOString() }))
 				} else {

src/Utils/retry-utils.ts

@@ -131,15 +131,17 @@ export function calculateDelay(
 	multiplier: number,
 	jitter: number
 ): number {
+	// Normalize attempt to ensure valid calculation (must be >= 1)
+	const normalizedAttempt = Math.max(attempt, 1)
 	let delay: number
 
 	switch (strategy) {
 		case 'exponential':
-			delay = baseDelay * Math.pow(multiplier, attempt - 1)
+			delay = baseDelay * Math.pow(multiplier, normalizedAttempt - 1)
 			break
 
 		case 'linear':
-			delay = baseDelay * attempt
+			delay = baseDelay * normalizedAttempt
 			break
 
 		case 'constant':
@@ -147,7 +149,7 @@ export function calculateDelay(
 			break
 
 		case 'fibonacci': {
-			const fib = fibonacciNumber(attempt)
+			const fib = fibonacciNumber(normalizedAttempt)
 			delay = baseDelay * fib
 			break
 		}
@@ -155,7 +157,7 @@ export function calculateDelay(
 		case 'stepped': {
 			// Uses pre-defined delay array directly (ignores baseDelay/multiplier)
 			// Falls back to last delay if attempt exceeds array length
-			const index = Math.min(attempt - 1, RETRY_BACKOFF_DELAYS.length - 1)
+			const index = Math.min(normalizedAttempt - 1, RETRY_BACKOFF_DELAYS.length - 1)
 			delay = RETRY_BACKOFF_DELAYS[index] ?? RETRY_BACKOFF_DELAYS[0] ?? baseDelay
 			break
 		}
@@ -164,15 +166,15 @@ export function calculateDelay(
 			delay = baseDelay
 	}
 
-	// Apply max delay cap
+	// Apply jitter BEFORE capping to maxDelay
-	delay = Math.min(delay, maxDelay)
-
-	// Apply jitter
 	if (jitter > 0) {
 		const jitterAmount = delay * jitter
 		delay = delay + (Math.random() * 2 - 1) * jitterAmount
 	}
 
+	// Apply max delay cap AFTER jitter to ensure we never exceed maxDelay
+	delay = Math.min(delay, maxDelay)
+
 	return Math.max(0, Math.round(delay))
 }
 
@@ -196,7 +198,15 @@ function fibonacciNumber(n: number): number {
  */
 async function sleep(ms: number, signal?: AbortSignal): Promise<void> {
 	return new Promise((resolve, reject) => {
-		const timer = setTimeout(resolve, ms)
+		let abortHandler: (() => void) | undefined
+
+		const timer = setTimeout(() => {
+			// Cleanup abort listener on normal completion to prevent memory leak
+			if (signal && abortHandler) {
+				signal.removeEventListener('abort', abortHandler)
+			}
+			resolve()
+		}, ms)
 
 		if (signal) {
 			if (signal.aborted) {
@@ -205,7 +215,7 @@ async function sleep(ms: number, signal?: AbortSignal): Promise<void> {
 				return
 			}
 
-			const abortHandler = () => {
+			abortHandler = () => {
 				clearTimeout(timer)
 				reject(new RetryAbortedError(0))
 			}
@@ -324,8 +334,9 @@ export async function retry<T>(
 				result = await operation(context)
 			}
 
-			// Success
+			// Success - only count as retry success if this wasn't the first attempt
-			if (config.collectMetrics) {
+			if (config.collectMetrics && attempt > 1) {
+				// This was a successful retry (not first attempt)
 				metrics.retries.inc({ operation: config.operationName })
 			}
 
@@ -339,9 +350,9 @@ export async function retry<T>(
 			const shouldRetry = await config.shouldRetry(lastError, attempt)
 
 			if (!shouldRetry || attempt >= config.maxAttempts) {
-				// Final failure
+				// Final failure - use dedicated retry exhausted metric
 				if (config.collectMetrics) {
-					metrics.errors.inc({ category: 'retry', code: 'exhausted' })
+					metrics.retryExhausted.inc({ operation: config.operationName })
 				}
 
 				config.onFailure(lastError, attempt)

src/__tests__/Utils/retry-utils.test.ts

@@ -16,6 +16,8 @@ import {
 	retryConfigs,
 	getRetryDelayWithJitter,
 	getAllRetryDelaysWithJitter,
+	RETRY_BACKOFF_DELAYS,
+	RETRY_JITTER_FACTOR,
 	type RetryContext,
 } from '../../Utils/retry-utils.js'
 
@@ -615,3 +617,29 @@ describe('getAllRetryDelaysWithJitter', () => {
 		expect(allSame).toBe(false)
 	})
 })
+
+describe('retryConfigs.rsocket consistency', () => {
+	/**
+	 * This test ensures the hardcoded values in retryConfigs.rsocket
+	 * stay synchronized with RETRY_BACKOFF_DELAYS and RETRY_JITTER_FACTOR.
+	 *
+	 * The values are hardcoded to avoid ESM initialization order issues,
+	 * but this test will fail if someone updates the constants without
+	 * updating retryConfigs.rsocket.
+	 */
+	it('should have maxAttempts equal to RETRY_BACKOFF_DELAYS.length', () => {
+		expect(retryConfigs.rsocket.maxAttempts).toBe(RETRY_BACKOFF_DELAYS.length)
+	})
+
+	it('should have baseDelay equal to RETRY_BACKOFF_DELAYS[0]', () => {
+		expect(retryConfigs.rsocket.baseDelay).toBe(RETRY_BACKOFF_DELAYS[0])
+	})
+
+	it('should have maxDelay equal to last RETRY_BACKOFF_DELAYS element', () => {
+		expect(retryConfigs.rsocket.maxDelay).toBe(RETRY_BACKOFF_DELAYS[RETRY_BACKOFF_DELAYS.length - 1])
+	})
+
+	it('should have jitter equal to RETRY_JITTER_FACTOR', () => {
+		expect(retryConfigs.rsocket.jitter).toBe(RETRY_JITTER_FACTOR)
+	})
+})