Implement Phase 3 retry behavior and type refactors#2
Merged
adelrodriguez merged 1 commit intomainfrom Feb 23, 2026
Merged
Conversation
WalkthroughSplit sync vs async run/catch shapes, made TryBuilder generic over configured run errors, rewrote executeRun to support retries/backoffs and sync→async transitions, added retry utilities and runtime guards, refined error code types, and expanded tests and docs with multi‑variant error examples and type assertions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
adelrodriguez
changed the base branch from
02-22-feat_add_execution_runner
to
graphite-base/2
adelrodriguez
force-pushed
the
02-22-add_async_support_to_run_method_with_error_mapping
branch
from
4a7c447 to
daa5bc3
Compare
adelrodriguez
changed the base branch from
graphite-base/2
to
02-22-refactor_retry_and_timeout_types_with_improved_documentation
adelrodriguez
force-pushed
the
02-22-refactor_retry_and_timeout_types_with_improved_documentation
branch
from
88617c1 to
72fb8cd
Compare
adelrodriguez
force-pushed
the
02-22-add_async_support_to_run_method_with_error_mapping
branch
from
daa5bc3 to
7cb3592
Compare
adelrodriguez
force-pushed
the
02-22-refactor_retry_and_timeout_types_with_improved_documentation
branch
from
72fb8cd to
5f999ea
Compare
adelrodriguez
force-pushed
the
02-22-add_async_support_to_run_method_with_error_mapping
branch
2 times, most recently
from
430aad4 to
d1ec5ac
Compare
adelrodriguez
force-pushed
the
02-22-refactor_retry_and_timeout_types_with_improved_documentation
branch
from
5f999ea to
9cf5642
Compare
adelrodriguez
force-pushed
the
02-22-add_async_support_to_run_method_with_error_mapping
branch
from
d1ec5ac to
8a95ade
Compare
adelrodriguez
force-pushed
the
02-22-refactor_retry_and_timeout_types_with_improved_documentation
branch
from
9cf5642 to
154a2e1
Compare
Base automatically changed from
02-22-refactor_retry_and_timeout_types_with_improved_documentation
to
main
February 22, 2026 23:29
adelrodriguez
force-pushed
the
02-22-add_async_support_to_run_method_with_error_mapping
branch
4 times, most recently
from
758b033 to
d6184a8
Compare
adelrodriguez
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return input(ctx) | ||
| } catch (error) { | ||
| return new UnhandledException({ cause: error }) | ||
| return await tryFn(ctx) |
There was a problem hiding this comment.
If the tryFn returns a rejected promise instead of throwing synchronously, the await will catch it but the rejection won't be caught by the try-catch on line 195. The error will propagate through Promise.resolve(result) on line 211, where the .catch() handler on line 211-213 will handle it. While this works correctly, the control flow is implicit and relies on promise chaining behavior.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/runner.ts
Line: 196
Comment:
If the `tryFn` returns a rejected promise instead of throwing synchronously, the `await` will catch it but the rejection won't be caught by the try-catch on line 195. The error will propagate through `Promise.resolve(result)` on line 211, where the `.catch()` handler on line 211-213 will handle it. While this works correctly, the control flow is implicit and relies on promise chaining behavior.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
.context/plan.md (1)
10-10: LGTM — Phase 2 correctly marked complete.The checkbox update aligns with the PR's delivered scope: async overloads, Promise detection,
Panicon async catch-throw, and updated test coverage.One minor doc inconsistency to consider: Phase 0 (Line 39) still lists
MaybePromise<T>as a foundational core type, but the PR summary indicates it was removed as unused intypes.ts. The plan could note this deviation to keep Phase 0's exit state accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.context/plan.md at line 10, Phase 0's exit state still mentions the core type MaybePromise<T> even though the PR removed it from types.ts; update the plan text for "Phase 0" to reflect that MaybePromise<T> was removed (or mark it as deprecated/removed) so the plan's documented exit state matches the code changes — locate the "Phase 0" section and replace or annotate the MaybePromise<T> mention accordingly.src/lib/__tests__/utils.test.ts (1)
9-14: Consider adding a test for callable thenables (thetypeof value === "function"branch).The implementation of
isPromiseLikeexplicitly handlestypeof value === "function", but there's no test exercising that path. Also, theReflect.setusage here could be simplified.💡 Suggested additional test cases
it("returns true for thenable objects", () => { - const thenable: Record<string, unknown> = {} - Reflect.set(thenable, "then", (_resolve: (value: string) => void) => null) - + const thenable = { then: () => null } expect(isPromiseLike(thenable)).toBe(true) }) + + it("returns true for callable thenables", () => { + const fn = Object.assign(() => {}, { then: () => null }) + expect(isPromiseLike(fn)).toBe(true) + }) + + it("returns false for objects with non-function then", () => { + expect(isPromiseLike({ then: "not a function" })).toBe(false) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/utils.test.ts` around lines 9 - 14, Add a unit test that exercises the typeof value === "function" branch of isPromiseLike by creating a callable thenable (e.g., const thenable = () => {}; thenable.then = (resolve: (v: string) => void) => null) and asserting isPromiseLike(thenable) === true; replace Reflect.set usage with simple property assignment to keep the test concise and clearly target the isPromiseLike function's callable-path.src/lib/__tests__/runner.test.ts (1)
84-105: Preferexpect().rejects.toThrow()for async rejection assertions.The manual try/catch pattern works but has a subtle issue: if
await resultunexpectedly resolves, line 101 throws a plainError, and the assertion on line 103 would fail with a confusing message. Usingexpect().rejectsproduces a clearer failure.♻️ Cleaner async Panic assertion
- it("throws Panic when async catch rejects", async () => { - const result = executeRun( + it("throws Panic when async catch rejects", async () => { + const result = executeRun( {}, { catch: async () => { await Promise.resolve() throw new Error("catch failed") }, try: async () => { await Promise.resolve() throw new Error("boom") }, } ) - try { - await result - throw new Error("Expected Panic rejection") - } catch (error) { - expect(error).toBeInstanceOf(Panic) - } + expect(result).rejects.toThrow(Panic) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/runner.test.ts` around lines 84 - 105, Replace the manual try/catch assertion with Jest's promise rejection matcher: call executeRun(...) to get the promise (result) and use await expect(result).rejects.toBeInstanceOf(Panic) so the test fails clearly if the promise resolves or rejects with the wrong type; update the test around the executeRun invocation and the Panic assertion accordingly.src/__tests__/index.test.ts (1)
112-130: Same suggestion: preferexpect().rejects.toThrow()for the Panic rejection test.Same pattern as noted in
src/lib/__tests__/runner.test.ts— the manual try/catch can produce confusing failures ifawait resultunexpectedly resolves.♻️ Cleaner async Panic assertion
it("throws Panic when async catch rejects", async () => { const result = run({ catch: async () => { await Promise.resolve() throw new Error("catch failed") }, try: async () => { await Promise.resolve() throw new Error("boom") }, }) - try { - await result - throw new Error("Expected Panic rejection") - } catch (error) { - expect(error).toBeInstanceOf(Panic) - } + expect(result).rejects.toThrow(Panic) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/index.test.ts` around lines 112 - 130, The test "throws Panic when async catch rejects" uses a manual try/catch that can mask unexpected resolves; replace the manual await + try/catch on the promise variable result with Jest's async assertion: await expect(result).rejects.toBeInstanceOf(Panic). Locate the test block that constructs result (the run call with try/catch async functions) and swap the final manual assertion with the single-line rejects assertion referencing Panic.src/lib/runner.ts (2)
52-67:maxDelayMscap only applies to exponential backoff.Linear backoff (
baseDelay * attempt) also grows without bound. If someone configures linear backoff with a high retry limit, delays can become unexpectedly large. Consider applying themaxDelayMscap uniformly after the switch.♻️ Proposed refactor to apply maxDelayMs to all strategies
case "exponential": delay = baseDelay * 2 ** (attempt - 1) - - if (policy.maxDelayMs !== undefined) { - delay = Math.min(delay, policy.maxDelayMs) - } - break } + if (policy.maxDelayMs !== undefined) { + delay = Math.min(delay, policy.maxDelayMs) + } + if (!policy.jitter || delay <= 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/runner.ts` around lines 52 - 67, The backoff logic only applies policy.maxDelayMs for the "exponential" case; update the retry delay calculation so the maxDelayMs cap is applied uniformly after the switch. Keep the existing switch on policy.backoff (cases "constant", "linear", "exponential") that sets delay from baseDelay and attempt, then after the switch check if policy.maxDelayMs !== undefined and set delay = Math.min(delay, policy.maxDelayMs) so linear and constant also get capped; modify the block around the symbols delay, baseDelay, attempt, and policy.maxDelayMs in src/lib/runner.ts accordingly.
91-95:shouldRetryreceives a fresh context, not the live execution context.
createContext(config)builds a newTryCtxinstead of reusing the one fromexecuteRun. If theshouldRetrycallback ever needs to inspect state that was attached to the running context bytryFn, it won't see it. Consider accepting the existingctxas a parameter and snapshotting only the retry fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/runner.ts` around lines 91 - 95, The current code creates a new TryCtx via createContext(config) before calling policy.shouldRetry, which prevents shouldRetry from seeing any state added to the running context by executeRun/tryFn; instead, reuse the existing execution context (the live ctx from executeRun) and only mutate or snapshot the retry-specific fields (ctx.retry.attempt and ctx.retry.limit) before calling policy.shouldRetry so shouldRetry receives the live TryCtx with up-to-date state; if you must avoid mutating the original, create a shallow copy of the existing ctx and update only the retry subfields, then pass that to policy.shouldRetry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/builder.ts`:
- Around line 59-68: The overload ordering for run is causing incorrect
inference when async try functions are passed; reorder the overload declarations
so overloads that accept AsyncRunTryFn<T> and async object-form variants appear
before the SyncRunTryFn<T> and sync object-form variants (i.e., place the
Promise-returning overloads above the non-promise ones) so TypeScript will pick
the more specific async signatures first and produce the correct return types
(refer to the run function and types SyncRunTryFn, AsyncRunTryFn, RunCatchFn,
RunInput, and UnhandledException to locate and adjust the declarations).
In `@src/lib/runner.ts`:
- Around line 112-131: The overload signatures for executeRun do not include the
possible control/terminal error types returned by
handleAttemptErrorSync/handleAttemptErrorAsync (RetryExhaustedError,
CancellationError, TimeoutError), so callers may receive those types
unexpectedly; update the executeRun overload return unions (all exported
overloads) to include RetryExhaustedError | CancellationError | TimeoutError
wherever the implementation may return those (e.g., the T | E and Promise<T | E>
variants) or alternatively change the implementation paths in
handleAttemptErrorSync/handleAttemptErrorAsync to call finalizeError so those
control errors are routed into the catch mapper; locate symbols executeRun,
handleAttemptErrorSync, handleAttemptErrorAsync, and finalizeError to apply the
chosen fix consistently across sync and async overloads.
- Around line 148-171: handleAttemptErrorSync currently re-evaluates
shouldAttemptRetry when delegating to handleAttemptErrorAsync and also recurses
synchronously via executeAttemptSync when delay === 0; to fix, hoist the retry
decision and exhaustion checks so they run exactly once per attempt (calculate
shouldAttemptRetry(attempt,error,config) and isRetryExhausted once in
handleAttemptErrorSync) and pass the decision/result into
handleAttemptErrorAsync (e.g., add a skipChecks or decision parameter) to avoid
double-calling shouldAttemptRetry, and replace the tail recursion path where
delay <= 0 with an iterative loop (increment attempt and continue) instead of
calling executeAttemptSync recursively to prevent unbounded sync recursion;
update references in handleAttemptErrorSync, handleAttemptErrorAsync,
executeAttemptSync, shouldAttemptRetry, calculateRetryDelay, isRetryExhausted,
and finalizeError accordingly.
---
Nitpick comments:
In @.context/plan.md:
- Line 10: Phase 0's exit state still mentions the core type MaybePromise<T>
even though the PR removed it from types.ts; update the plan text for "Phase 0"
to reflect that MaybePromise<T> was removed (or mark it as deprecated/removed)
so the plan's documented exit state matches the code changes — locate the "Phase
0" section and replace or annotate the MaybePromise<T> mention accordingly.
In `@src/__tests__/index.test.ts`:
- Around line 112-130: The test "throws Panic when async catch rejects" uses a
manual try/catch that can mask unexpected resolves; replace the manual await +
try/catch on the promise variable result with Jest's async assertion: await
expect(result).rejects.toBeInstanceOf(Panic). Locate the test block that
constructs result (the run call with try/catch async functions) and swap the
final manual assertion with the single-line rejects assertion referencing Panic.
In `@src/lib/__tests__/runner.test.ts`:
- Around line 84-105: Replace the manual try/catch assertion with Jest's promise
rejection matcher: call executeRun(...) to get the promise (result) and use
await expect(result).rejects.toBeInstanceOf(Panic) so the test fails clearly if
the promise resolves or rejects with the wrong type; update the test around the
executeRun invocation and the Panic assertion accordingly.
In `@src/lib/__tests__/utils.test.ts`:
- Around line 9-14: Add a unit test that exercises the typeof value ===
"function" branch of isPromiseLike by creating a callable thenable (e.g., const
thenable = () => {}; thenable.then = (resolve: (v: string) => void) => null) and
asserting isPromiseLike(thenable) === true; replace Reflect.set usage with
simple property assignment to keep the test concise and clearly target the
isPromiseLike function's callable-path.
In `@src/lib/runner.ts`:
- Around line 52-67: The backoff logic only applies policy.maxDelayMs for the
"exponential" case; update the retry delay calculation so the maxDelayMs cap is
applied uniformly after the switch. Keep the existing switch on policy.backoff
(cases "constant", "linear", "exponential") that sets delay from baseDelay and
attempt, then after the switch check if policy.maxDelayMs !== undefined and set
delay = Math.min(delay, policy.maxDelayMs) so linear and constant also get
capped; modify the block around the symbols delay, baseDelay, attempt, and
policy.maxDelayMs in src/lib/runner.ts accordingly.
- Around line 91-95: The current code creates a new TryCtx via
createContext(config) before calling policy.shouldRetry, which prevents
shouldRetry from seeing any state added to the running context by
executeRun/tryFn; instead, reuse the existing execution context (the live ctx
from executeRun) and only mutate or snapshot the retry-specific fields
(ctx.retry.attempt and ctx.retry.limit) before calling policy.shouldRetry so
shouldRetry receives the live TryCtx with up-to-date state; if you must avoid
mutating the original, create a shallow copy of the existing ctx and update only
the retry subfields, then pass that to policy.shouldRetry.
1ad5ea0 to
35b5bdb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/lib/__tests__/runner.test.ts (1)
106-127: Same Panic assertion pattern asindex.test.ts— considerexpect(...).rejects.toThrow(Panic)here too.Same optional suggestion as in
src/__tests__/index.test.tsline 124: the manual try/catch with sentinel throw could be replaced with a more declarative.rejects.toThrow(Panic)assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/runner.test.ts` around lines 106 - 127, The test uses a manual try/catch with a sentinel throw to assert that executeRun(...) rejects with a Panic; replace that pattern by asserting the returned promise rejects declaratively, e.g. use expect(result).rejects.toThrow(Panic) (ensure you don't await the promise before passing it to expect so Jest can handle the rejection), referencing the executeRun invocation and the Panic class in the assertion.src/lib/__tests__/retry.test.ts (1)
289-320: Consider extracting thesetTimeoutmock into a shared helper.The
setTimeoutmock pattern is duplicated across the linear (lines 290-301) and exponential (lines 323-334) backoff tests. A small helper (e.g.,withMockedTimers(fn)) that captures delays and auto-restores the original would reduce duplication and make future backoff tests easier to add.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/retry.test.ts` around lines 289 - 320, Extract the duplicated setTimeout mocking into a shared test helper (e.g., withMockedTimers) that accepts a callback and returns captured delays while auto-restoring globalThis.setTimeout; update the tests that call executeRun (the linear and exponential backoff cases) to use this helper and assert on the returned delays and result (RetryExhaustedError) instead of duplicating the mock logic; ensure the helper preserves original setTimeout, pushes timeout values into an array, invokes the handler immediately, and restores the original in a finally block.src/lib/__tests__/utils.test.ts (1)
1-65: Missing test coverage forsleep.The
sleeputility is exported fromutils.tsbut has no tests here. Consider adding basic cases:sleep(0)resolves immediately,sleep(-1)resolves immediately, andsleep(positive)resolves after a delay (using a fake timer or similar approach).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/utils.test.ts` around lines 1 - 65, Tests are missing for the sleep utility; add a new describe/it block in src/lib/__tests__/utils.test.ts that imports sleep and verifies sleep(0) and sleep(-1) resolve immediately and that sleep(positiveMs) delays resolution (use a fake timer or time-measurement strategy to assert the delay). Specifically, add tests referencing the sleep function: one asserting immediate resolution for 0, one for negative values, and one asserting a measurable pause for a positive millisecond value (use Bun's test timer utilities or manual Date.now checks to avoid flaky timing).src/__tests__/index.test.ts (1)
112-130: Nit: consider usingexpect(...).rejects.toThrow(Panic)for consistency.The manual try/catch with a sentinel throw on line 126 works, but if the rejection never happens, the test could pass silently if the
throw new Error("Expected Panic rejection")line is somehow skipped. Bun's test runner supportsexpect(promise).rejects.toThrow(Panic)which is more declarative and avoids the sentinel pattern.That said, if you've found this pattern more reliable for your Panic flow, feel free to keep it.
♻️ Proposed refactor
- try { - await result - throw new Error("Expected Panic rejection") - } catch (error) { - expect(error).toBeInstanceOf(Panic) - } + expect(result).rejects.toThrow(Panic)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/index.test.ts` around lines 112 - 130, Test uses a manual try/catch sentinel pattern; replace it with Bun's promise assertion to be more declarative: update the "throws Panic when async catch rejects" test to assert that the promise returned by run({...}) rejects with Panic by using expect(result).rejects.toThrow(Panic) (referencing the run function and Panic class) instead of the try { await result; throw new Error(...) } catch (...) pattern so the test fails correctly if the promise does not reject.src/lib/retry.ts (2)
59-74: Adddefault: assertUnreachableto match the exhaustiveness guard innormalizeRetryPolicy.Both
normalizeRetryPolicyandcalculateRetryDelayswitch overpolicy.backoff, but only the former has adefault: return assertUnreachable(policy). A future backoff variant added to the discriminated union would silently produce adelayof0here instead of a compile-time error.♻️ Proposed fix
case "exponential": delay = baseDelay * 2 ** (attempt - 1) if (policy.maxDelayMs !== undefined) { delay = Math.min(delay, policy.maxDelayMs) } break + default: + return assertUnreachable(policy.backoff) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/retry.ts` around lines 59 - 74, The switch in calculateRetryDelay over policy.backoff is missing an exhaustiveness guard; add a default case that calls assertUnreachable(policy) (matching normalizeRetryPolicy) so any future backoff variant triggers a compile-time/typecheck error instead of silently leaving delay unset; keep the existing cases (constant, linear, exponential) and put default: return assertUnreachable(policy) (or throw/assert) to satisfy the exhaustiveness check for policy.backoff.
102-104:ctx.retry.limit = policy.limitis redundant.
createContext(config)already initialisesctx.retry.limittoconfig.retry?.limit ?? 1. Sincepolicyisconfig.retry(verified non-null at line 88), the reassignment on line 104 is a no-op.♻️ Proposed fix
const ctx = createContext(config) ctx.retry.attempt = attempt - ctx.retry.limit = policy.limit return policy.shouldRetry(error, ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/retry.ts` around lines 102 - 104, The assignment ctx.retry.limit = policy.limit is redundant because createContext(config) already sets ctx.retry.limit from config.retry?.limit ?? 1 and policy is the same config.retry; remove the redundant line to avoid a no-op. Locate the createContext call and the surrounding retry initialization (symbols: createContext, ctx, config, policy, ctx.retry.limit) and delete the reassignment so ctx.retry.limit remains sourced only from createContext.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/runner.ts`:
- Around line 42-56: The overloads for executeRun (the synchronous overload
signatures returning T | UnhandledException and T | E) are incorrect because
synchronous paths can return a Promise when config.retry.delayMs > 0 (see
executeAttemptSync, handleAttemptErrorSync delegating to async
handleAttemptErrorAsync); update the affected overload signatures (the first and
third executeRun overload declarations) to include the Promise variant (e.g., T
| UnhandledException | Promise<T | UnhandledException> and T | E | Promise<T |
E>) so callers get the correct widened return type, and apply the same widening
to the corresponding run overloads in builder.ts (the run overloads referenced
in the review) to keep signatures consistent across modules.
---
Duplicate comments:
In `@src/lib/builder.ts`:
- Around line 66-73: The third overload for run (run<T, C>(options: { try:
SyncRunTryFn<T>; catch: SyncRunCatchFn<C> }): T | C | E) incorrectly forces a
synchronous return while executeRun (and .retry with delayMs > 0) can produce a
Promise; update that overload to mirror the async/sync widening used elsewhere
(e.g., change its return to Promise<T | C | E> or to a union including Promise)
so run and executeRun signatures stay consistent when .retry is used; adjust the
overload for the run method in builder.ts that references SyncRunTryFn and
SyncRunCatchFn and ensure any related types/consumers compile against the
widened return.
In `@src/lib/runner.ts`:
- Around line 98-163: handleAttemptErrorSync currently calls
checkShouldAttemptRetry and calculateRetryDelay, then delegates to
handleAttemptErrorAsync which recomputes both (causing double side-effects and
different jitter) and uses recursion for zero-delay retries (risking stack
overflow). Fix by: compute shouldRetry = checkShouldAttemptRetry(error, attempt,
config) and delay = calculateRetryDelay(attempt, config) once in
handleAttemptErrorSync; if !shouldRetry handle the exhausted/finalize cases; if
delay <= 0 perform retries iteratively (replace the recursive executeAttemptSync
chain with a while loop that updates attempt, ctx.retry.attempt and re-invokes
executeAttemptSync) to avoid growing the stack; if delay > 0 call a revised
handleAttemptErrorAsync(error, attempt, delay) that awaits the provided delay
(remove its own calculateRetryDelay/checkShouldAttemptRetry calls) and then
tryFn/recursively call handleAttemptErrorAsync only with the next attempt.
Ensure references to checkShouldAttemptRetry, calculateRetryDelay,
executeAttemptSync, handleAttemptErrorAsync, handleAttemptErrorSync, and
ctx.retry.attempt are used to locate the changes.
---
Nitpick comments:
In `@src/__tests__/index.test.ts`:
- Around line 112-130: Test uses a manual try/catch sentinel pattern; replace it
with Bun's promise assertion to be more declarative: update the "throws Panic
when async catch rejects" test to assert that the promise returned by run({...})
rejects with Panic by using expect(result).rejects.toThrow(Panic) (referencing
the run function and Panic class) instead of the try { await result; throw new
Error(...) } catch (...) pattern so the test fails correctly if the promise does
not reject.
In `@src/lib/__tests__/retry.test.ts`:
- Around line 289-320: Extract the duplicated setTimeout mocking into a shared
test helper (e.g., withMockedTimers) that accepts a callback and returns
captured delays while auto-restoring globalThis.setTimeout; update the tests
that call executeRun (the linear and exponential backoff cases) to use this
helper and assert on the returned delays and result (RetryExhaustedError)
instead of duplicating the mock logic; ensure the helper preserves original
setTimeout, pushes timeout values into an array, invokes the handler
immediately, and restores the original in a finally block.
In `@src/lib/__tests__/runner.test.ts`:
- Around line 106-127: The test uses a manual try/catch with a sentinel throw to
assert that executeRun(...) rejects with a Panic; replace that pattern by
asserting the returned promise rejects declaratively, e.g. use
expect(result).rejects.toThrow(Panic) (ensure you don't await the promise before
passing it to expect so Jest can handle the rejection), referencing the
executeRun invocation and the Panic class in the assertion.
In `@src/lib/__tests__/utils.test.ts`:
- Around line 1-65: Tests are missing for the sleep utility; add a new
describe/it block in src/lib/__tests__/utils.test.ts that imports sleep and
verifies sleep(0) and sleep(-1) resolve immediately and that sleep(positiveMs)
delays resolution (use a fake timer or time-measurement strategy to assert the
delay). Specifically, add tests referencing the sleep function: one asserting
immediate resolution for 0, one for negative values, and one asserting a
measurable pause for a positive millisecond value (use Bun's test timer
utilities or manual Date.now checks to avoid flaky timing).
In `@src/lib/retry.ts`:
- Around line 59-74: The switch in calculateRetryDelay over policy.backoff is
missing an exhaustiveness guard; add a default case that calls
assertUnreachable(policy) (matching normalizeRetryPolicy) so any future backoff
variant triggers a compile-time/typecheck error instead of silently leaving
delay unset; keep the existing cases (constant, linear, exponential) and put
default: return assertUnreachable(policy) (or throw/assert) to satisfy the
exhaustiveness check for policy.backoff.
- Around line 102-104: The assignment ctx.retry.limit = policy.limit is
redundant because createContext(config) already sets ctx.retry.limit from
config.retry?.limit ?? 1 and policy is the same config.retry; remove the
redundant line to avoid a no-op. Locate the createContext call and the
surrounding retry initialization (symbols: createContext, ctx, config, policy,
ctx.retry.limit) and delete the reassignment so ctx.retry.limit remains sourced
only from createContext.
35b5bdb to
a66a1d0
Compare
| return delay | ||
| } | ||
|
|
||
| return Math.floor(Math.random() * delay) |
There was a problem hiding this comment.
Full jitter returns random value from 0 to delay, which could result in zero delay. Consider documenting that jitter uses "full jitter" strategy (uniform random [0, delay]) rather than "equal jitter" (delay/2 + random [0, delay/2])
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/retry.ts
Line: 81
Comment:
Full jitter returns random value from 0 to `delay`, which could result in zero delay. Consider documenting that jitter uses "full jitter" strategy (uniform random [0, delay]) rather than "equal jitter" (delay/2 + random [0, delay/2])
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/lib/types/core.ts (1)
8-11: ConsiderreadonlyonRetryInfofields to signal intent.
attemptandlimitare semantically read-only from the user's perspective (mutations have no effect on retry counting sincerunner.tstracks state via the closureattemptparameter). Marking themreadonlymakes the contract explicit and prevents confusing user-side mutations.♻️ Suggested change
export interface RetryInfo { - attempt: number - limit: number + readonly attempt: number + readonly limit: number }This would require a cast in
runner.tsat the mutation sites:;(ctx.retry as { attempt: number }).attempt = attempt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/types/core.ts` around lines 8 - 11, Mark the RetryInfo interface fields as readonly to express intent (update RetryInfo: attempt and limit -> readonly), then update all mutation sites that currently assign to ctx.retry.attempt in runner.ts to cast before assignment (e.g., cast ctx.retry to a mutable shape when writing the attempt) so the code compiles while preserving the readonly contract for consumers; reference the RetryInfo interface and the assignment sites in runner.ts where ctx.retry.attempt is set.src/__tests__/types.test.ts (1)
78-121: Minor coverage gap — async try/catch variant missing fortimeoutandsignal.The
"with retry"section tests all four combinations (sync fn, async fn, sync try/catch, async try/catch). Both"with timeout"and"with signal"stop at three, skipping theasync try / sync catch → Promise<T | E | TimeoutError/CancellationError>variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/types.test.ts` around lines 78 - 121, Add the missing async try/catch type tests for both timeout and signal: call timeout(5000).run and signal(ac.signal).run with an object where try is an async function (e.g. async () => 42) and catch returns the error literal (e.g. () => "err" as const), and assert the resulting type is Promise<number | "err" | TimeoutError> for timeout and Promise<number | "err" | CancellationError> for signal using the same Expect<Equal<...>> pattern as the other tests to match the async try/catch coverage in the "with retry" section.src/lib/__tests__/runner.test.ts (1)
106-127: Simplify async Panic assertion with.rejects.Bun's
expectsupports.rejectsdirectly on a Promise value, so the manual try/catch block can be replaced with a single line that also provides a clearer failure message.♻️ Suggested refactor
- it("throws Panic when async catch rejects", async () => { - const result = executeRun( - {}, - { - catch: async () => { - await Promise.resolve() - throw new Error("catch failed") - }, - try: async () => { - await Promise.resolve() - throw new Error("boom") - }, - } - ) - - try { - await result - throw new Error("Expected Panic rejection") - } catch (error) { - expect(error).toBeInstanceOf(Panic) - } - }) + it("throws Panic when async catch rejects", async () => { + const result = executeRun( + {}, + { + catch: async () => { + await Promise.resolve() + throw new Error("catch failed") + }, + try: async () => { + await Promise.resolve() + throw new Error("boom") + }, + } + ) + + await expect(result).rejects.toBeInstanceOf(Panic) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/runner.test.ts` around lines 106 - 127, The test "throws Panic when async catch rejects" manually awaits the Promise from executeRun and uses a try/catch to assert a Panic; replace that manual pattern with Bun's promise assertion by calling expect(result).rejects.toBeInstanceOf(Panic) to simplify and clarify the assertion for the Promise returned by executeRun, updating the test case that constructs result with executeRun({},{ try: ..., catch: ... }) and removing the try/catch/throw scaffolding.src/lib/utils.ts (1)
7-15:sleepwill block timeout and cancellation during backoff delays (Phase 4/5 concern).
handleAttemptErrorAsynccallssleep(delay)with no cancellation path. When Phase 4 (total-scope timeout) and Phase 5 (AbortSignal) are implemented, a backoff delay in progress will run to completion regardless of timeout expiry or abort, breaking the total-timeout and cancellation guarantees described in the design doc.Consider accepting an optional
AbortSignalnow so the signature doesn't need a breaking change later:
⚠️ Suggested API change (forward-compatible)-export function sleep(ms: number): Promise<void> { +export function sleep(ms: number, signal?: AbortSignal): Promise<void> { if (ms <= 0) { return Promise.resolve() } return new Promise((resolve, reject) => { - setTimeout(resolve, ms) + const id = setTimeout(resolve, ms) + signal?.addEventListener("abort", () => { + clearTimeout(id) + reject(signal.reason) + }, { once: true }) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils.ts` around lines 7 - 15, The sleep function (sleep) blocks timeout/abort because it offers no cancellation; change its signature to accept an optional AbortSignal (e.g., sleep(ms: number, signal?: AbortSignal): Promise<void>) and implement the promise so it resolves after setTimeout but also listens for signal.aborted to reject or resolve early and cleans up the timeout and event listener; update callers such as handleAttemptErrorAsync to pass through their AbortSignal so backoff can be interrupted without a breaking API change later.src/lib/retry.ts (1)
60-75: Consider adding an exhaustiveness guard to thecalculateRetryDelayswitch.
normalizeRetryPolicyhasdefault: return assertUnreachable(policy)butcalculateRetryDelaydoes not. If a fourth backoff type is added toRetryPolicy,calculateRetryDelaysilently falls through and returns0delay instead of producing a compile-time error.♻️ Proposed fix
case "exponential": delay = baseDelay * 2 ** (attempt - 1) if (policy.maxDelayMs !== undefined) { delay = Math.min(delay, policy.maxDelayMs) } break + default: + return assertUnreachable(policy) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/retry.ts` around lines 60 - 75, The switch in calculateRetryDelay is not exhaustive so new backoff kinds could silently return 0; update the switch in calculateRetryDelay to include a default/exhaustiveness guard that calls the existing assertUnreachable (or similar) with the policy.backoff (or the whole policy) to force a compile-time error for unknown backoff types and ensure the function always returns a valid delay; reference the calculateRetryDelay function, the policy.backoff discriminant, and the existing assertUnreachable utility used by normalizeRetryPolicy when adding this guard.src/lib/builder.ts (1)
71-71: Rename implementation signature'sEto avoid shadowing the class-level generic.The implementation parameter
run<T, E>usesEfor the user's catch-result type, butEis already the class-level generic that tracks config-induced errors (ConfigRunErrors). The overloads themselves useCfor the same catch-result role (lines 64–69). Renaming the implementation parameter toCis consistent with the overloads and avoids reader confusion.♻️ Proposed fix
- run<T, E>(input: RunInput<T, E>) { + run<T, C>(input: RunInput<T, C>) { return executeRun(this.#config, input) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/builder.ts` at line 71, The implementation signature for the run method is shadowing the class-level generic ConfigRunErrors by using E; rename the implementation generic from E to C to match the overloads and avoid confusion: change run<T, E>(input: RunInput<T, E>) to run<T, C>(input: RunInput<T, C>) and update all internal usages/annotations inside the run method body from E to C (and ensure consistency with the existing overloads that already use C and with the class-level generics like ConfigRunErrors).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/runner.ts`:
- Around line 120-138: The retry path currently re-evaluates
checkShouldAttemptRetry twice and risks unbounded synchronous recursion; to fix,
change the control flow so the "enter async loop" and "re-check then enter"
paths are distinct: add an optional flag (e.g., alreadyChecked) to
handleAttemptErrorAsync so handleAttemptErrorSync passes alreadyChecked=true
when it determines a positive delay (thus skipping the second check inside
handleAttemptErrorAsync), and convert the synchronous retry path
(executeAttemptSync / handleAttemptErrorSync) to an iterative loop (or a
tail-call style loop inside executeAttemptSync that increments attempt and
reuses calculateRetryDelay) instead of recursive calls to avoid growing the call
stack. Ensure all references use checkShouldAttemptRetry,
handleAttemptErrorSync, handleAttemptErrorAsync, executeAttemptSync, and
calculateRetryDelay.
- Around line 55-63: The current sync overloads for executeRun (the signatures
returning T | UnhandledException and T | E for SyncRunTryFn inputs) are unsafe
because executeRun can return a Promise at runtime when config.retry.delayMs > 0
(it delegates to handleAttemptErrorAsync), so change the public overloads to
consistently return Promise<...> (i.e. make executeRun always return Promise<T |
UnhandledException> / Promise<T | E>) or alternatively restrict the sync
overload to only apply when delayMs === 0 and document the trade-off with a NOTE
comment; update the overloads for executeRun, and any related type declarations
for SyncRunTryFn/AsyncRunTryFn usage to match the chosen approach so callers
won’t receive a silently dropped Promise.
---
Nitpick comments:
In `@src/__tests__/types.test.ts`:
- Around line 78-121: Add the missing async try/catch type tests for both
timeout and signal: call timeout(5000).run and signal(ac.signal).run with an
object where try is an async function (e.g. async () => 42) and catch returns
the error literal (e.g. () => "err" as const), and assert the resulting type is
Promise<number | "err" | TimeoutError> for timeout and Promise<number | "err" |
CancellationError> for signal using the same Expect<Equal<...>> pattern as the
other tests to match the async try/catch coverage in the "with retry" section.
In `@src/lib/__tests__/runner.test.ts`:
- Around line 106-127: The test "throws Panic when async catch rejects" manually
awaits the Promise from executeRun and uses a try/catch to assert a Panic;
replace that manual pattern with Bun's promise assertion by calling
expect(result).rejects.toBeInstanceOf(Panic) to simplify and clarify the
assertion for the Promise returned by executeRun, updating the test case that
constructs result with executeRun({},{ try: ..., catch: ... }) and removing the
try/catch/throw scaffolding.
In `@src/lib/builder.ts`:
- Line 71: The implementation signature for the run method is shadowing the
class-level generic ConfigRunErrors by using E; rename the implementation
generic from E to C to match the overloads and avoid confusion: change run<T,
E>(input: RunInput<T, E>) to run<T, C>(input: RunInput<T, C>) and update all
internal usages/annotations inside the run method body from E to C (and ensure
consistency with the existing overloads that already use C and with the
class-level generics like ConfigRunErrors).
In `@src/lib/retry.ts`:
- Around line 60-75: The switch in calculateRetryDelay is not exhaustive so new
backoff kinds could silently return 0; update the switch in calculateRetryDelay
to include a default/exhaustiveness guard that calls the existing
assertUnreachable (or similar) with the policy.backoff (or the whole policy) to
force a compile-time error for unknown backoff types and ensure the function
always returns a valid delay; reference the calculateRetryDelay function, the
policy.backoff discriminant, and the existing assertUnreachable utility used by
normalizeRetryPolicy when adding this guard.
In `@src/lib/types/core.ts`:
- Around line 8-11: Mark the RetryInfo interface fields as readonly to express
intent (update RetryInfo: attempt and limit -> readonly), then update all
mutation sites that currently assign to ctx.retry.attempt in runner.ts to cast
before assignment (e.g., cast ctx.retry to a mutable shape when writing the
attempt) so the code compiles while preserving the readonly contract for
consumers; reference the RetryInfo interface and the assignment sites in
runner.ts where ctx.retry.attempt is set.
In `@src/lib/utils.ts`:
- Around line 7-15: The sleep function (sleep) blocks timeout/abort because it
offers no cancellation; change its signature to accept an optional AbortSignal
(e.g., sleep(ms: number, signal?: AbortSignal): Promise<void>) and implement the
promise so it resolves after setTimeout but also listens for signal.aborted to
reject or resolve early and cleans up the timeout and event listener; update
callers such as handleAttemptErrorAsync to pass through their AbortSignal so
backoff can be interrupted without a breaking API change later.
adelrodriguez
force-pushed
the
02-22-add_async_support_to_run_method_with_error_mapping
branch
from
a66a1d0 to
f1a524d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.context/api-proposal.md (1)
74-74:⚠️ Potential issue | 🟡 MinorInaccurate comment: numeric shorthand uses constant backoff, not linear.
try$.retry(3)normalizes to{ backoff: "constant", delayMs: 0, limit: 3 }vianormalizeRetryPolicy. The comment says "linear backoff" which is misleading.📝 Proposed fix
-try$.retry(3) // Retry a specific number of times with linear backoff +try$.retry(3) // Retry a specific number of times with constant backoff (no delay)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.context/api-proposal.md at line 74, The comment on the example using try$.retry(3) is incorrect: normalizeRetryPolicy converts numeric shorthand into a constant-backoff policy ({ backoff: "constant", delayMs: 0, limit: 3 }), not a linear one; update the comment to state "constant backoff" (or explicitly show the normalized policy) and/or revise the example to use the correct API if you intended linear backoff—see try$.retry, normalizeRetryPolicy and the retry policy shape for reference.
🧹 Nitpick comments (6)
src/lib/retry.ts (3)
60-75: Add adefaultcase withassertUnreachablefor consistency.
normalizeRetryPolicy(line 41-42) usesassertUnreachablein its switch over the same discriminant.calculateRetryDelayomits it, leavingdelaysilently at0if a new backoff variant is added but not handled here.♻️ Proposed fix
break + default: + assertUnreachable(policy) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/retry.ts` around lines 60 - 75, The switch in calculateRetryDelay over policy.backoff can fall through silently for unknown variants; add a default case that calls assertUnreachable(policy.backoff) (same helper used in normalizeRetryPolicy) so any new backoff enum triggers a compile-time/runtime assertion instead of leaving delay as 0; update the switch in calculateRetryDelay to include this default path referencing policy.backoff and assertUnreachable.
103-107: Context created then immediately mutated — consider passing attempt directly.
createContext(config)always setsretry.attempt = 1, which is then overwritten on line 104. This works but is slightly wasteful and obscures intent. A lighter approach would be to build the context inline or add anattemptparameter tocreateContext.♻️ Possible simplification
- const ctx = createContext(config) - ctx.retry.attempt = attempt - ctx.retry.limit = policy.limit + const ctx: TryCtx = { + signal: config.signal, + retry: { attempt, limit: policy.limit }, + }This would require importing
TryCtxfrom./types/core.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/retry.ts` around lines 103 - 107, The code creates a context via createContext(config) then immediately mutates ctx.retry.attempt — instead pass the attempt into the context construction or build the TryCtx inline to avoid the temporary incorrect default. Update createContext to accept an optional attempt parameter (or add a helper overload) and call createContext(config, attempt), or construct a TryCtx object with retry.attempt set to attempt and pass that to policy.shouldRetry; ensure references to createContext, ctx.retry.attempt, policy.shouldRetry (and TryCtx if you choose inline construction) are updated accordingly.
77-82: Full jitter can reduce delay to zero — verify this is the intended behavior.
Math.floor(Math.random() * delay)yields a value in[0, delay), meaning the effective delay can be0on any retry attempt. This is the "full jitter" strategy (as recommended by AWS), but if the intent is to always have some minimum delay, consider "decorrelated jitter" or "equal jitter" (delay/2 + Math.random() * delay/2).If full jitter is intentional, a brief comment documenting the choice would help future readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/retry.ts` around lines 77 - 82, The current full-jitter implementation (checking policy.jitter and returning Math.floor(Math.random() * delay)) allows a zero delay; either change the jitter computation to enforce a non-zero minimum (e.g., replace the return with an equal-jitter formula like Math.floor(delay/2 + Math.random() * (delay/2)) so result is in [delay/2, delay)) or, if full jitter (allowing zero) is intentional, add a short explanatory comment next to the policy.jitter check and the return Math.floor(Math.random() * delay) line documenting that full jitter (including 0ms) is expected per AWS guidance.src/lib/builder.ts (1)
77-79: Implementation signature shadows the class genericE.The class declares
TryBuilder<E extends ConfigRunErrors>, and the overload signatures useCfor the catch-handler error type. However, the implementation signature reintroducesEasrun<T, E>(input: RunInput<T, E>), shadowing the class-levelEwith a completely different meaning. This won't cause a runtime issue (implementation signatures aren't visible to callers), but it can confuse readers.♻️ Proposed fix — use `C` to match the overloads
- run<T, E>(input: RunInput<T, E>) { + run<T, C>(input: RunInput<T, C>) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/builder.ts` around lines 77 - 79, The implementation signature for TryBuilder.run currently redefines the generic E and shadows the class-level E; change the implementation generic to match the overloads (use C) so the method signature reads run<T, C>(input: RunInput<T, C>) and forwards to executeRun(this.#config, input); update the generic name in the implementation only (keep class declaration TryBuilder<E extends ConfigRunErrors> and the overloads untouched) to avoid confusing shadowing.src/lib/types/retry.ts (1)
22-31: Consider whether linear backoff should also supportmaxDelayMs.Linear delay grows as
baseDelay * attempt, which can become large for high attempt limits. Unlike exponential backoff which hasmaxDelayMsas a cap, linear backoff forbids it vianever. This is a reasonable design choice if attempt limits are expected to be small, but worth confirming it's intentional since users might want to cap linear delays too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/types/retry.ts` around lines 22 - 31, The LinearBackoffRetryPolicy currently forbids maxDelayMs via "maxDelayMs?: never"; change it to allow an optional numeric cap (e.g., "maxDelayMs?: number") so linear backoff can be capped like exponential backoff, and update the JSDoc for LinearBackoffRetryPolicy to document that baseDelay * attempt is applied and optionally capped by maxDelayMs; modify any usage/validation that checks LinearBackoffRetryPolicy to accept and honor maxDelayMs (refer to the LinearBackoffRetryPolicy and BaseRetryPolicy types and any retry logic that reads maxDelayMs).src/lib/__tests__/retry.test.ts (1)
289-353:setTimeoutmock works but is fragile for concurrent/parallel test runs.The direct
globalThis.setTimeoutreplacement is acceptable given bun:test's sequential in-file execution and thetry/finallycleanup. Just be aware that if the test runner ever parallelizes tests within a file, or if another async operation fires during the test, the mock could interfere.Consider extracting the mock/restore pattern into a small helper if more delay-based tests are added in the future to reduce duplication between the two test cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/retry.test.ts` around lines 289 - 353, The tests directly replace globalThis.setTimeout in two places which is fragile; extract a small helper to install and restore a mocked setTimeout that records delays and immediately invokes handlers, and reuse it in the tests around executeRun so both cases (linear and exponential backoff) call the helper instead of duplicating the mock/restore logic; reference the existing usages of globalThis.setTimeout, the try/finally blocks, executeRun, and RetryExhaustedError when implementing the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.context/api-proposal.md:
- Line 74: The comment on the example using try$.retry(3) is incorrect:
normalizeRetryPolicy converts numeric shorthand into a constant-backoff policy
({ backoff: "constant", delayMs: 0, limit: 3 }), not a linear one; update the
comment to state "constant backoff" (or explicitly show the normalized policy)
and/or revise the example to use the correct API if you intended linear
backoff—see try$.retry, normalizeRetryPolicy and the retry policy shape for
reference.
---
Nitpick comments:
In `@src/lib/__tests__/retry.test.ts`:
- Around line 289-353: The tests directly replace globalThis.setTimeout in two
places which is fragile; extract a small helper to install and restore a mocked
setTimeout that records delays and immediately invokes handlers, and reuse it in
the tests around executeRun so both cases (linear and exponential backoff) call
the helper instead of duplicating the mock/restore logic; reference the existing
usages of globalThis.setTimeout, the try/finally blocks, executeRun, and
RetryExhaustedError when implementing the helper.
In `@src/lib/builder.ts`:
- Around line 77-79: The implementation signature for TryBuilder.run currently
redefines the generic E and shadows the class-level E; change the implementation
generic to match the overloads (use C) so the method signature reads run<T,
C>(input: RunInput<T, C>) and forwards to executeRun(this.#config, input);
update the generic name in the implementation only (keep class declaration
TryBuilder<E extends ConfigRunErrors> and the overloads untouched) to avoid
confusing shadowing.
In `@src/lib/retry.ts`:
- Around line 60-75: The switch in calculateRetryDelay over policy.backoff can
fall through silently for unknown variants; add a default case that calls
assertUnreachable(policy.backoff) (same helper used in normalizeRetryPolicy) so
any new backoff enum triggers a compile-time/runtime assertion instead of
leaving delay as 0; update the switch in calculateRetryDelay to include this
default path referencing policy.backoff and assertUnreachable.
- Around line 103-107: The code creates a context via createContext(config) then
immediately mutates ctx.retry.attempt — instead pass the attempt into the
context construction or build the TryCtx inline to avoid the temporary incorrect
default. Update createContext to accept an optional attempt parameter (or add a
helper overload) and call createContext(config, attempt), or construct a TryCtx
object with retry.attempt set to attempt and pass that to policy.shouldRetry;
ensure references to createContext, ctx.retry.attempt, policy.shouldRetry (and
TryCtx if you choose inline construction) are updated accordingly.
- Around line 77-82: The current full-jitter implementation (checking
policy.jitter and returning Math.floor(Math.random() * delay)) allows a zero
delay; either change the jitter computation to enforce a non-zero minimum (e.g.,
replace the return with an equal-jitter formula like Math.floor(delay/2 +
Math.random() * (delay/2)) so result is in [delay/2, delay)) or, if full jitter
(allowing zero) is intentional, add a short explanatory comment next to the
policy.jitter check and the return Math.floor(Math.random() * delay) line
documenting that full jitter (including 0ms) is expected per AWS guidance.
In `@src/lib/types/retry.ts`:
- Around line 22-31: The LinearBackoffRetryPolicy currently forbids maxDelayMs
via "maxDelayMs?: never"; change it to allow an optional numeric cap (e.g.,
"maxDelayMs?: number") so linear backoff can be capped like exponential backoff,
and update the JSDoc for LinearBackoffRetryPolicy to document that baseDelay *
attempt is applied and optionally capped by maxDelayMs; modify any
usage/validation that checks LinearBackoffRetryPolicy to accept and honor
maxDelayMs (refer to the LinearBackoffRetryPolicy and BaseRetryPolicy types and
any retry logic that reads maxDelayMs).
This was referenced Feb 23, 2026
This was referenced Mar 5, 2026
Merged
This was referenced Mar 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Summary by CodeRabbit
New Features
Tests
Documentation
Greptile Summary
This PR implements Phase 3 of the retry system with comprehensive backoff strategies (constant, linear, exponential), jitter support, and intelligent sync/async execution switching. The implementation maintains synchronous execution when possible and only switches to async when encountering promises or retry delays, preventing stack overflow with a continuation-based loop for zero-delay retries.
Key Changes:
executeRunwith attempt limits,shouldRetrypredicate, and backoff delaysRetryExhaustedErrorflow and ensures control errors (Panic, TimeoutError, CancellationError) bypass retry and catch mappingtypes/run.ts,types/retry.ts,types/builder.ts,types/core.ts) with sync/async discrimination viaNonPromisehelperImplementation Highlights:
CONTINUE_SYNC) for efficient sync retry loopsshouldRetrycall with correct attempt numberConfidence Score: 5/5
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD Start[executeRun called] --> AttemptSync[executeAttemptSync] AttemptSync --> TryExec[Execute tryFn] TryExec --> CheckPromise{Is result<br/>Promise-like?} CheckPromise -->|Yes| AsyncPath[Attach rejection handler] CheckPromise -->|No| SyncReturn[Return success value] TryExec -->|Throws| CheckControl{Is control<br/>error?} CheckControl -->|Yes| ReturnError[Return error immediately] CheckControl -->|No| EvalRetry[evaluateRetryDecision] EvalRetry --> ShouldRetry{shouldAttemptRetry?} ShouldRetry -->|No| CheckExhausted{isRetryExhausted?} CheckExhausted -->|Yes| RetryExhausted[Return RetryExhaustedError] CheckExhausted -->|No| Finalize[finalizeError] ShouldRetry -->|Yes| CheckDelay{delay > 0?} CheckDelay -->|No| SyncContinue[Continue sync loop] CheckDelay -->|Yes| SwitchAsync[Switch to async path] SwitchAsync --> AsyncSleep[await sleep] AsyncPath --> AsyncSleep AsyncSleep --> NextAttempt[Increment attempt] NextAttempt --> AsyncRetry[Recursive retry] Finalize --> HasCatch{Has catch<br/>handler?} HasCatch -->|Yes| ExecCatch[executeCatch] HasCatch -->|No| UnhandledExc[Return UnhandledException] SyncContinue --> AttemptSync ExecCatch --> CatchOk[Return mapped error] ExecCatch -->|Throws| PanicError[Throw Panic]Last reviewed commit: f1a524d
Context used:
dashboard- AGENTS.md (source)