feat: establish terminal reconnection foundation #18693
Conversation
- Update ConnectionStatus type: replace 'initializing' with 'connecting' - Create useRetry hook with exponential backoff logic - Add comprehensive tests for useRetry hook - Export useRetry from hooks index Implements: - Initial delay: 1 second - Max delay: 30 seconds - Backoff multiplier: 2 - Max retry attempts: 10 Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
- Fix startRetrying to immediately perform first retry - Adjust retry scheduling conditions - Fix delay calculation for exponential backoff Still debugging test failures
- Fix attemptCount to represent attempts started, not completed - Fix exponential backoff delay calculation - Fix retry scheduling conditions for proper max attempts handling - All 10 useRetry tests now pass - No regressions in existing test suite Implements correct behavior: - attemptCount increments when retry starts - Exponential backoff: 1s, 2s, 4s, 8s, 16s, 30s (capped) - Respects maxAttempts limit - Manual retry cancels automatic retries - State resets properly on success Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
- Add parentheses around arrow function parameter - Fix indentation Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
- Replace setTimeout/setInterval with window.setTimeout/window.setInterval - Replace clearTimeout/clearInterval with window.clearTimeout/window.clearInterval - Fixes TypeScript error: Type 'Timeout' is not assignable to type 'number' - Ensures proper browser environment timer types Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
Convert useRetry hook from multiple useState calls to a single useReducer for cleaner state management. This improves code clarity and makes state transitions more predictable. Changes: - Replace 5 useState calls with single useReducer - Add RetryState interface and RetryAction union type - Implement retryReducer function for all state transitions - Update all state access to use state object - Replace setState calls with dispatch calls throughout Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
417b053 to
dd7adda
Compare
There was a problem hiding this comment.
I found the state hard to follow, is that just me? I think the code could be significantly simpler (simpler to me at least). For example (not tested):
import { useEffect, useRef, useState } from "react";
import { useEffectEvent } from "./hookPolyfills";
export type RetryState = "idle" | "retry" | "countdown";
interface RetryOptions {
delayMs: number;
enabled: boolean;
maxDelayMs: number;
multiplier: number;
onRetry: () => Promise<void>;
}
export const useRetry = ({ delayMs, enabled, maxDelayMs, multiplier, onRetry }: RetryOptions) => {
const [retryState, setRetryState] = useState<RetryState>("idle");
const [attempt, setAttempt] = useState(0);
const [countdown, setCountdown] = useState(0);
const onRetryEvent = useEffectEvent(onRetry);
useEffect(() => {
setRetryState(enabled ? "countdown" : "idle");
}, [enabled]);
useEffect(() => {
switch (retryState) {
case "idle":
setAttempt(0);
break;
case "retry":
let aborted = false;
onRetryEvent().then(() => {
if (!aborted) {
setRetryState("idle");
}
}).catch(() => {
if (!aborted) {
setRetryState("countdown");
setAttempt(attempt + 1); // probably better to set earlier or together with the state
}
});
return () => {
aborted = true;
};
case "countdown":
const delay = Math.min(delayMs * multiplier ** (attempt - 1), maxDelayMs);
const timer = setTimeout(() => setRetryState("retry"), delay);
const start = Date.now();
const interval = setInterval(() => setCountdown(Math.max(0, delay - (Date.now() - start))), 1000);
return () => {
clearTimeout(timer);
clearInterval(interval);
};
}
}, [attempt, retryState, delayMs, multiplier, maxDelayMs, onRetryEvent]);
return {
attempt,
// countdown will be null if the next retry is not scheduled.
countdown: retryState === "countdown" ? countdown : null,
};
};
site/src/hooks/useRetry.ts
Outdated
| /** | ||
| * Maximum number of retry attempts | ||
| */ | ||
| maxAttempts: number; |
There was a problem hiding this comment.
Does there need to be a max number of attempts? Any reason we cannot keep trying?
There was a problem hiding this comment.
IMO it is nice to have to avoid spam the API with unecessary requests. eg. A user leaves the terminal page open for a few hours. Wdyt?
There was a problem hiding this comment.
If the user has the terminal page open, that means they want to stay connected, right?
It is possible they could have forgotten to close the window, but at least for me, if I have a terminal up I would expect it to be still connected when I come back.
It would be nice to have parity with SSH at least, I think we have a really long timeout there. I forget exactly what, I think it is at least a day or two though? Something else to consider is that our other ways of connecting, through the IDEs, all will continue to reconnect (I think???), so the web terminal would be the odd one out.
site/src/hooks/useRetry.ts
Outdated
| /** | ||
| * Initial delay in milliseconds | ||
| */ | ||
| initialDelay: number; |
There was a problem hiding this comment.
This is probably just a matter of opinion so feel free to ignore, but I was at first confused with initialDelay because we use it in every loop (multiplied), not just the initial one, so I wonder if we could call this delay (or delayMs) instead.
site/src/hooks/useRetry.ts
Outdated
| /** | ||
| * Whether a retry is currently in progress (manual or automatic) | ||
| */ | ||
| isRetrying: boolean; |
There was a problem hiding this comment.
Without having seen the planned usage yet, my assumption is that when users pass in callback they will already have a variable for whether the callback is running, so I wonder if we need isRetrying since it gives info that the caller already has?
For example the terminal will already have a connection status state it can use, making this one redundant, since you could setTerminalState("connecting") inside onRetry(), and then use terminalState instead of isRetrying.
Actually, the caller will always need to have their own state anyway, so they know when to call start to begin the retry.
There was a problem hiding this comment.
Not exactly. It is only retrying if it is calling the function at a second time. Eg. when connecting at the firt time, it should not be considered as a retry, only after that. Makes sense?
There was a problem hiding this comment.
I definitely could be missed something, but what I saw in the code is that isRetrying is set the every time onRetry() is called, not the second time. startRetrying() calls performRetry() which calls dispatch("START_RETRY") which sets isRetrying: true and then calls onRetryEvent().
Since isRetrying is always set when onRetry() is called, the caller could simply do setTerminalState("reconnecting") as part of onRetry() to get a reconnecting state.
|
@code-asher thanks a lot for the review! I think I have a better sense on how to simplify the hook usage and make code more understandable. |
- Created useWithRetry hook with simple interface (call, retryAt, isLoading) - Implements exponential backoff with configurable options - Includes comprehensive tests covering all scenarios - Added usage examples for different configurations - Follows existing code patterns and uses constants for configuration Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
- Remove example file as requested - Fix circular dependency issue using useRef pattern - Ensure proper cleanup and timer management - Implementation follows existing codebase patterns Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
- Remove useRetry.ts and useRetry.test.ts files - Update hooks index.ts to remove useRetry export - useWithRetry provides simpler interface for retry functionality Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
- Remove options parameter - hook now uses fixed configuration - Update max attempts to 10 (from 3) - Update max delay to 10 minutes (from 8 seconds) - Remove countdown logic - no interval ref needed - Consolidate state into single RetryState object - Calculate delay inline where it's used - Remove separate executeFunction - logic moved into call function - Only use functions for reusable logic (clearTimer) - Update tests to match new implementation - Run formatting and linting checks Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
- Do not reset attemptCount when no more attempts are possible - Add attemptCount to UseWithRetryResult interface - Return attemptCount in hook result for tracking - Update tests to verify attemptCount preservation - Add comprehensive test for attemptCount behavior This allows consumers to track how many attempts were made, even after all retry attempts have been exhausted. Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
|
@code-asher I just came up with a better solution, I guess. Please let me know what you think 🙏 |
| clearTimeout(); | ||
|
|
||
| const executeAttempt = async (attempt: number): Promise<void> => { | ||
| setState((prev) => ({ ...prev, isLoading: true, attemptCount: attempt })); |
There was a problem hiding this comment.
Is it be weird for retryAt to still be set since it will be a time in the past now?
Also I wonder if we explicitly do retryAt: prev.retryAt, I feel like the spread makes it too easy to forget to set some state (like in the previous version).
Oh also if the user manually calls this then the retryAt will be wrong for a while (until fn() finishes).
| }; | ||
|
|
||
| await executeAttempt(0); | ||
| }, [fn, clearTimeout]); |
There was a problem hiding this comment.
Should we use that useEffectEvent thing or is the expectation that the user memoizes the function they pass in?
| })); | ||
|
|
||
| timeoutRef.current = window.setTimeout(() => { | ||
| setState((prev) => ({ ...prev, retryAt: undefined })); |
There was a problem hiding this comment.
Oh I see you unset retryAt here. Should we do it in executeAttempt() to also cover manually calling?
| ); | ||
|
|
||
| setState((prev) => ({ | ||
| ...prev, |
There was a problem hiding this comment.
Should we explicitly attemptCount: prev.attemptCount to avoid accidentally forgetting props that need to be set?
| }, delayMs); | ||
| } else { | ||
| setState((prev) => ({ | ||
| ...prev, |
| retryAt: new Date(Date.now() + delayMs), | ||
| })); | ||
|
|
||
| timeoutRef.current = window.setTimeout(() => { |
There was a problem hiding this comment.
Could this code still run after an unmount, if fn was in flight while unmounting? I think it would, right? So we might start a new timeout even though the component is unmounted?
| } | ||
| }, []); | ||
|
|
||
| const call = useCallback(async () => { |
There was a problem hiding this comment.
Oh wait one more thing, just want to check on the async intent here. Is the idea that call should return a promise that resolves once fn completes successfully? Right now it resolves after the first run of fn (whether it succeeds or fails). Did you have a use in mind for this?
Implements:
Related to: coder/internal#659