fix: Implement missing backoffMultiplier and timeout in RecoveryOptions

- Add timeout functionality to RecoveryManager.attemptRecovery() using Promise.race()
- Implement backoffMultiplier support in NetworkRecoveryStrategy and BaseRecoveryStrategy
- Update calculateBackoffDelay() to accept configurable backoff multiplier
- Add comprehensive tests for timeout and custom configuration handling
- Ensure backward compatibility while addressing reviewer feedback

Fixes code reviewer feedback about unused RecoveryOptions properties.

Author: Eric Oliver

src/cli/recovery/NetworkRecoveryStrategy.ts

@@ -9,11 +9,13 @@ import { BaseRecoveryStrategy } from "./RecoveryStrategy"
 export class NetworkRecoveryStrategy extends BaseRecoveryStrategy {
 	private readonly maxAttempts: number = 3
 	private readonly baseDelay: number = 1000
+	private readonly backoffMultiplier: number = 2
 
-	constructor(maxAttempts?: number, baseDelay?: number) {
+	constructor(maxAttempts?: number, baseDelay?: number, backoffMultiplier?: number) {
 		super()
 		if (maxAttempts) this.maxAttempts = maxAttempts
 		if (baseDelay) this.baseDelay = baseDelay
+		if (backoffMultiplier) this.backoffMultiplier = backoffMultiplier
 	}
 
 	canRecover(error: Error, context: ErrorContext): boolean {
@@ -47,7 +49,7 @@ export class NetworkRecoveryStrategy extends BaseRecoveryStrategy {
 		// Exponential backoff retry for other network errors
 		for (let attempt = 1; attempt <= this.maxAttempts; attempt++) {
 			if (attempt > 1) {
-				const delay = this.calculateBackoffDelay(attempt, this.baseDelay)
+				const delay = this.calculateBackoffDelay(attempt, this.baseDelay, this.backoffMultiplier)
 				await this.delay(delay)
 				this.logRecoveryAttempt(error, attempt, context)
 			}

src/cli/recovery/RecoveryStrategy.ts

@@ -20,8 +20,13 @@ export abstract class BaseRecoveryStrategy implements RecoveryStrategy {
 	/**
 	 * Calculate exponential backoff delay
 	 */
-	protected calculateBackoffDelay(attempt: number, baseDelay: number = 1000, maxDelay: number = 30000): number {
-		const delay = Math.min(baseDelay * Math.pow(2, attempt - 1), maxDelay)
+	protected calculateBackoffDelay(
+		attempt: number,
+		baseDelay: number = 1000,
+		backoffMultiplier: number = 2,
+		maxDelay: number = 30000,
+	): number {
+		const delay = Math.min(baseDelay * Math.pow(backoffMultiplier, attempt - 1), maxDelay)
 		// Add jitter to prevent thundering herd
 		return delay + Math.random() * delay * 0.1
 	}

src/cli/services/RecoveryManager.ts

@@ -35,8 +35,57 @@ export class RecoveryManager implements IRecoveryManager {
 	}
 
 	async attemptRecovery(error: Error, context: ErrorContext, options: RecoveryOptions = {}): Promise<RecoveryResult> {
-		const { maxAttempts = 3, enableRollback = true, strategies = this.strategies } = options
+		const {
+			maxAttempts = 3,
+			enableRollback = true,
+			strategies = this.strategies,
+			timeout,
+			backoffMultiplier,
+		} = options
+
+		const recoveryPromise = this.executeRecoveryStrategies(
+			error,
+			context,
+			strategies,
+			maxAttempts,
+			enableRollback,
+			backoffMultiplier,
+		)
+
+		// Set up overall timeout if specified
+		if (timeout) {
+			const timeoutPromise = new Promise<RecoveryResult>((_, reject) => {
+				setTimeout(() => {
+					reject(new Error(`Recovery timeout after ${timeout}ms`))
+				}, timeout)
+			})
+
+			try {
+				return await Promise.race([recoveryPromise, timeoutPromise])
+			} catch (timeoutError) {
+				return {
+					success: false,
+					finalError: timeoutError instanceof Error ? timeoutError : new Error(String(timeoutError)),
+					suggestions: [
+						"Recovery operation timed out",
+						"Consider increasing timeout value",
+						"Check for hanging operations",
+					],
+				}
+			}
+		} else {
+			return await recoveryPromise
+		}
+	}
 
+	private async executeRecoveryStrategies(
+		error: Error,
+		context: ErrorContext,
+		strategies: RecoveryStrategy[],
+		maxAttempts: number,
+		enableRollback: boolean,
+		backoffMultiplier?: number,
+	): Promise<RecoveryResult> {
 		// Find applicable recovery strategies
 		const applicableStrategies = strategies.filter((strategy) => strategy.canRecover(error, context))
 
@@ -50,7 +99,14 @@ export class RecoveryManager implements IRecoveryManager {
 		// Try each strategy in order
 		for (const strategy of applicableStrategies) {
 			try {
-				const result = await strategy.recover(error, context)
+				// Pass backoff configuration to strategies that support it
+				const result = await this.executeStrategyWithOptions(
+					strategy,
+					error,
+					context,
+					maxAttempts,
+					backoffMultiplier,
+				)
 
 				if (result.success) {
 					return result
@@ -88,6 +144,30 @@ export class RecoveryManager implements IRecoveryManager {
 		}
 	}
 
+	private async executeStrategyWithOptions(
+		strategy: RecoveryStrategy,
+		error: Error,
+		context: ErrorContext,
+		maxAttempts: number,
+		backoffMultiplier?: number,
+	): Promise<RecoveryResult> {
+		// Check if strategy supports configurable options and if custom options are provided
+		if (
+			strategy.constructor.name === "NetworkRecoveryStrategy" &&
+			(maxAttempts !== 3 || backoffMultiplier !== undefined)
+		) {
+			// Only create configured strategy if error can be handled by NetworkRecoveryStrategy
+			if (strategy.canRecover(error, context)) {
+				const configurableStrategy = new NetworkRecoveryStrategy(maxAttempts, undefined, backoffMultiplier)
+				// NetworkRecoveryStrategy.canRecover already verified this is a compatible error
+				return await configurableStrategy.recover(error as any, context)
+			}
+		}
+
+		// Use strategy as-is for other strategies or default configuration
+		return await strategy.recover(error, context)
+	}
+
 	async rollbackOperation(operationId: string): Promise<void> {
 		const operationState = this.operationStates.get(operationId)
 		if (!operationState) {

src/cli/services/__tests__/RecoveryManager.test.ts

@@ -138,6 +138,94 @@ describe("RecoveryManager", () => {
 
 			expect(strategyWithRollback.rollback).not.toHaveBeenCalled()
 		})
+
+		it("should respect timeout option and fail recovery if timeout is exceeded", async () => {
+			const slowStrategy: RecoveryStrategy = {
+				canRecover: jest.fn().mockReturnValue(true),
+				recover: jest.fn().mockImplementation(async () => {
+					// Simulate a slow recovery operation
+					await new Promise((resolve) => setTimeout(resolve, 200))
+					return { success: true }
+				}),
+				rollback: jest.fn().mockResolvedValue(undefined),
+			}
+
+			// Clear existing strategies and add only our slow strategy
+			recoveryManager = new RecoveryManager()
+			recoveryManager.addStrategy(slowStrategy)
+
+			const error = new Error("Test error")
+			const result = await recoveryManager.attemptRecovery(error, mockContext, {
+				timeout: 100, // Very short timeout
+			})
+
+			expect(result.success).toBe(false)
+			expect(result.finalError?.message).toContain("Recovery timeout after 100ms")
+			expect(result.suggestions).toContain("Recovery operation timed out")
+		})
+
+		it("should complete recovery if within timeout", async () => {
+			const fastStrategy: RecoveryStrategy = {
+				canRecover: jest.fn().mockReturnValue(true),
+				recover: jest.fn().mockResolvedValue({ success: true }),
+				rollback: jest.fn().mockResolvedValue(undefined),
+			}
+
+			// Clear existing strategies and add only our fast strategy
+			recoveryManager = new RecoveryManager()
+			recoveryManager.addStrategy(fastStrategy)
+
+			const error = new Error("Test error")
+			const result = await recoveryManager.attemptRecovery(error, mockContext, {
+				timeout: 1000, // Generous timeout
+			})
+
+			expect(result.success).toBe(true)
+			expect(fastStrategy.recover).toHaveBeenCalledWith(error, mockContext)
+		})
+
+		it("should handle custom maxAttempts and backoffMultiplier options", async () => {
+			const networkError = new NetworkError("Connection failed", "NET_ERROR", 500)
+
+			const result = await recoveryManager.attemptRecovery(networkError, mockContext, {
+				backoffMultiplier: 1.5,
+				maxAttempts: 2,
+			})
+
+			// The test should complete without errors - the implementation creates a new strategy internally
+			expect(result).toBeDefined()
+			expect(typeof result.success).toBe("boolean")
+		})
+
+		it("should use default options when none provided", async () => {
+			const mockStrategy: RecoveryStrategy = {
+				canRecover: jest.fn().mockReturnValue(true),
+				recover: jest.fn().mockResolvedValue({ success: true }),
+				rollback: jest.fn().mockResolvedValue(undefined),
+			}
+
+			// Clear existing strategies and add only our mock strategy
+			recoveryManager = new RecoveryManager()
+			recoveryManager.addStrategy(mockStrategy)
+
+			const error = new Error("Test error")
+			const result = await recoveryManager.attemptRecovery(error, mockContext)
+
+			expect(result.success).toBe(true)
+			expect(mockStrategy.recover).toHaveBeenCalledWith(error, mockContext)
+		})
+
+		it("should handle timeout option properly", async () => {
+			const networkError = new NetworkError("Connection failed", "NET_ERROR", 500)
+
+			const result = await recoveryManager.attemptRecovery(networkError, mockContext, {
+				timeout: 5000, // 5 second timeout
+			})
+
+			// Should complete within reasonable time
+			expect(result).toBeDefined()
+			expect(typeof result.success).toBe("boolean")
+		})
 	})
 
 	describe("operation state management", () => {