-
Notifications
You must be signed in to change notification settings - Fork 928
feat: establish terminal reconnection foundation #18693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c115f13
c10349b
3f45b74
834b2e5
d398265
dd7adda
5766fc0
b1e453b
d4326fb
8323192
3022566
bde014c
cb363db
55036a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,250 @@ | ||
import { act, renderHook } from "@testing-library/react"; | ||
import { useWithRetry } from "./useWithRetry"; | ||
|
||
// Mock timers | ||
jest.useFakeTimers(); | ||
|
||
describe("useWithRetry", () => { | ||
let mockFn: jest.Mock; | ||
|
||
beforeEach(() => { | ||
mockFn = jest.fn(); | ||
jest.clearAllTimers(); | ||
}); | ||
|
||
afterEach(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
|
||
it("should initialize with correct default state", () => { | ||
const { result } = renderHook(() => useWithRetry(mockFn)); | ||
|
||
expect(result.current.isLoading).toBe(false); | ||
expect(result.current.retryAt).toBe(undefined); | ||
}); | ||
|
||
it("should execute function successfully on first attempt", async () => { | ||
mockFn.mockResolvedValue(undefined); | ||
|
||
const { result } = renderHook(() => useWithRetry(mockFn)); | ||
|
||
await act(async () => { | ||
await result.current.call(); | ||
}); | ||
|
||
expect(mockFn).toHaveBeenCalledTimes(1); | ||
expect(result.current.isLoading).toBe(false); | ||
expect(result.current.retryAt).toBe(undefined); | ||
}); | ||
|
||
it("should set isLoading to true during execution", async () => { | ||
let resolvePromise: () => void; | ||
const promise = new Promise<void>((resolve) => { | ||
resolvePromise = resolve; | ||
}); | ||
mockFn.mockReturnValue(promise); | ||
|
||
const { result } = renderHook(() => useWithRetry(mockFn)); | ||
|
||
act(() => { | ||
result.current.call(); | ||
}); | ||
|
||
expect(result.current.isLoading).toBe(true); | ||
|
||
await act(async () => { | ||
resolvePromise!(); | ||
await promise; | ||
}); | ||
|
||
expect(result.current.isLoading).toBe(false); | ||
}); | ||
|
||
it("should retry on failure with exponential backoff", async () => { | ||
mockFn | ||
.mockRejectedValueOnce(new Error("First failure")) | ||
.mockRejectedValueOnce(new Error("Second failure")) | ||
.mockResolvedValueOnce(undefined); | ||
|
||
const { result } = renderHook(() => useWithRetry(mockFn)); | ||
|
||
// Start the call | ||
await act(async () => { | ||
await result.current.call(); | ||
}); | ||
|
||
expect(mockFn).toHaveBeenCalledTimes(1); | ||
expect(result.current.isLoading).toBe(false); | ||
expect(result.current.retryAt).not.toBe(null); | ||
|
||
// Fast-forward to first retry (1 second) | ||
await act(async () => { | ||
jest.advanceTimersByTime(1000); | ||
}); | ||
|
||
expect(mockFn).toHaveBeenCalledTimes(2); | ||
expect(result.current.isLoading).toBe(false); | ||
expect(result.current.retryAt).not.toBe(null); | ||
|
||
// Fast-forward to second retry (2 seconds) | ||
await act(async () => { | ||
jest.advanceTimersByTime(2000); | ||
}); | ||
|
||
expect(mockFn).toHaveBeenCalledTimes(3); | ||
expect(result.current.isLoading).toBe(false); | ||
expect(result.current.retryAt).toBe(undefined); | ||
}); | ||
|
||
it("should stop retrying after max attempts (10)", async () => { | ||
mockFn.mockRejectedValue(new Error("Always fails")); | ||
|
||
const { result } = renderHook(() => useWithRetry(mockFn)); | ||
|
||
// Start the call | ||
await act(async () => { | ||
await result.current.call(); | ||
}); | ||
|
||
expect(mockFn).toHaveBeenCalledTimes(1); | ||
expect(result.current.isLoading).toBe(false); | ||
expect(result.current.retryAt).not.toBe(null); | ||
|
||
// Fast-forward through all retries | ||
for (let i = 1; i < 10; i++) { | ||
const delay = Math.min(1000 * 2 ** (i - 1), 600000); // exponential backoff with max delay | ||
await act(async () => { | ||
jest.advanceTimersByTime(delay); | ||
}); | ||
expect(mockFn).toHaveBeenCalledTimes(i + 1); | ||
} | ||
|
||
// After 10 attempts, should stop retrying | ||
expect(result.current.isLoading).toBe(false); | ||
expect(result.current.retryAt).toBe(undefined); | ||
}); | ||
|
||
it("should respect max delay of 10 minutes", async () => { | ||
mockFn.mockRejectedValue(new Error("Always fails")); | ||
|
||
const { result } = renderHook(() => useWithRetry(mockFn)); | ||
|
||
// Start the call | ||
await act(async () => { | ||
await result.current.call(); | ||
}); | ||
|
||
expect(result.current.isLoading).toBe(false); | ||
|
||
// Fast-forward through several retries to reach max delay | ||
// After attempt 9, delay would be 1000 * 2^9 = 512000ms, which is less than 600000ms (10 min) | ||
// After attempt 10, delay would be 1000 * 2^10 = 1024000ms, which should be capped at 600000ms | ||
|
||
// Skip to attempt 9 (delay calculation: 1000 * 2^8 = 256000ms) | ||
for (let i = 1; i < 9; i++) { | ||
const delay = 1000 * 2 ** (i - 1); | ||
await act(async () => { | ||
jest.advanceTimersByTime(delay); | ||
}); | ||
} | ||
|
||
expect(mockFn).toHaveBeenCalledTimes(9); | ||
expect(result.current.retryAt).not.toBe(null); | ||
|
||
// The 9th retry should use max delay (600000ms = 10 minutes) | ||
await act(async () => { | ||
jest.advanceTimersByTime(600000); | ||
}); | ||
|
||
expect(mockFn).toHaveBeenCalledTimes(10); | ||
expect(result.current.isLoading).toBe(false); | ||
expect(result.current.retryAt).toBe(undefined); | ||
}); | ||
|
||
it("should cancel previous retry when call is invoked again", async () => { | ||
mockFn | ||
.mockRejectedValueOnce(new Error("First failure")) | ||
.mockResolvedValueOnce(undefined); | ||
|
||
const { result } = renderHook(() => useWithRetry(mockFn)); | ||
|
||
// Start the first call | ||
await act(async () => { | ||
await result.current.call(); | ||
}); | ||
|
||
expect(mockFn).toHaveBeenCalledTimes(1); | ||
expect(result.current.isLoading).toBe(false); | ||
expect(result.current.retryAt).not.toBe(null); | ||
|
||
// Call again before retry happens | ||
await act(async () => { | ||
await result.current.call(); | ||
}); | ||
|
||
expect(mockFn).toHaveBeenCalledTimes(2); | ||
expect(result.current.isLoading).toBe(false); | ||
expect(result.current.retryAt).toBe(undefined); | ||
|
||
// Advance time to ensure previous retry was cancelled | ||
await act(async () => { | ||
jest.advanceTimersByTime(5000); | ||
}); | ||
|
||
expect(mockFn).toHaveBeenCalledTimes(2); // Should not have been called again | ||
}); | ||
|
||
it("should set retryAt when scheduling retry", async () => { | ||
mockFn | ||
.mockRejectedValueOnce(new Error("Failure")) | ||
.mockResolvedValueOnce(undefined); | ||
|
||
const { result } = renderHook(() => useWithRetry(mockFn)); | ||
|
||
// Start the call | ||
await act(async () => { | ||
await result.current.call(); | ||
}); | ||
|
||
const retryAt = result.current.retryAt; | ||
expect(retryAt).not.toBe(null); | ||
expect(retryAt).toBeInstanceOf(Date); | ||
|
||
// retryAt should be approximately 1 second in the future | ||
const expectedTime = Date.now() + 1000; | ||
const actualTime = retryAt!.getTime(); | ||
expect(Math.abs(actualTime - expectedTime)).toBeLessThan(100); // Allow 100ms tolerance | ||
|
||
// Advance past retry time | ||
await act(async () => { | ||
jest.advanceTimersByTime(1000); | ||
}); | ||
|
||
expect(result.current.retryAt).toBe(undefined); | ||
}); | ||
|
||
it("should cleanup timer on unmount", async () => { | ||
mockFn.mockRejectedValue(new Error("Failure")); | ||
|
||
const { result, unmount } = renderHook(() => useWithRetry(mockFn)); | ||
|
||
// Start the call to create timer | ||
await act(async () => { | ||
await result.current.call(); | ||
}); | ||
|
||
expect(result.current.isLoading).toBe(false); | ||
expect(result.current.retryAt).not.toBe(null); | ||
|
||
// Unmount should cleanup timer | ||
unmount(); | ||
|
||
// Advance time to ensure timer was cleared | ||
await act(async () => { | ||
jest.advanceTimersByTime(5000); | ||
}); | ||
|
||
// Function should not have been called again | ||
expect(mockFn).toHaveBeenCalledTimes(1); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
import { useCallback, useEffect, useRef, useState } from "react"; | ||
|
||
const MAX_ATTEMPTS = 10; | ||
const DELAY_MS = 1_000; | ||
const MAX_DELAY_MS = 600_000; // 10 minutes | ||
// Determines how much the delay between retry attempts increases after each | ||
// failure. | ||
const MULTIPLIER = 2; | ||
|
||
interface UseWithRetryResult { | ||
call: () => Promise<void>; | ||
retryAt: Date | undefined; | ||
isLoading: boolean; | ||
} | ||
|
||
interface RetryState { | ||
isLoading: boolean; | ||
retryAt: Date | undefined; | ||
attemptCount: number; | ||
} | ||
|
||
/** | ||
* Hook that wraps a function with automatic retry functionality | ||
* Provides a simple interface for executing functions with exponential backoff retry | ||
*/ | ||
export function useWithRetry(fn: () => Promise<void>): UseWithRetryResult { | ||
const [state, setState] = useState<RetryState>({ | ||
isLoading: false, | ||
retryAt: undefined, | ||
attemptCount: 0, | ||
}); | ||
|
||
const timeoutRef = useRef<number | null>(null); | ||
|
||
const clearTimeout = useCallback(() => { | ||
if (timeoutRef.current) { | ||
window.clearTimeout(timeoutRef.current); | ||
timeoutRef.current = null; | ||
} | ||
}, []); | ||
|
||
const call = useCallback(async () => { | ||
clearTimeout(); | ||
|
||
const executeAttempt = async (attempt: number): Promise<void> => { | ||
setState((prev) => ({ ...prev, isLoading: true, attemptCount: attempt })); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it be weird for Also I wonder if we explicitly do Oh also if the user manually calls this then the |
||
|
||
try { | ||
await fn(); | ||
setState({ isLoading: false, retryAt: undefined, attemptCount: 0 }); | ||
} catch (error) { | ||
// Since attempts start from 0, we need to add +1 to make the condition work | ||
// This ensures exactly MAX_ATTEMPTS total attempts (attempt 0, 1, 2, ..., 9) | ||
if (attempt + 1 < MAX_ATTEMPTS) { | ||
const delayMs = Math.min( | ||
DELAY_MS * MULTIPLIER ** attempt, | ||
MAX_DELAY_MS, | ||
); | ||
|
||
setState((prev) => ({ | ||
...prev, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we explicitly |
||
isLoading: false, | ||
retryAt: new Date(Date.now() + delayMs), | ||
})); | ||
|
||
timeoutRef.current = window.setTimeout(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this code still run after an unmount, if |
||
setState((prev) => ({ ...prev, retryAt: undefined })); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see you unset |
||
executeAttempt(attempt + 1); | ||
}, delayMs); | ||
} else { | ||
setState((prev) => ({ | ||
...prev, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||
isLoading: false, | ||
retryAt: undefined, | ||
})); | ||
} | ||
} | ||
}; | ||
|
||
await executeAttempt(0); | ||
}, [fn, clearTimeout]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use that |
||
|
||
useEffect(() => { | ||
return () => { | ||
clearTimeout(); | ||
}; | ||
}, [clearTimeout]); | ||
|
||
return { | ||
call, | ||
retryAt: state.retryAt, | ||
isLoading: state.isLoading, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait one more thing, just want to check on the
async
intent here. Is the idea thatcall
should return a promise that resolves oncefn
completes successfully? Right now it resolves after the first run offn
(whether it succeeds or fails). Did you have a use in mind for this?