Refactor retry and timeout types with improved documentation#3
Conversation
WalkthroughRefactors retry and timeout types and normalization: introduces discriminated retry backoff interfaces and RetryOptions (number | policy), changes numeric shorthand to constant backoff, adds TimeoutPolicy, tightens normalization logic, updates builder API, adds tests, and an assertUnreachable utility; includes one planning note. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
| export function normalizeRetryPolicy(policy: RetryOptions): RetryPolicy { | ||
| if (typeof policy === "number") { | ||
| return { | ||
| backoff: "linear", | ||
| backoff: "linear" as const, | ||
| delayMs: 0, | ||
| limit: policy, | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| backoff: policy.backoff ?? "linear", | ||
| delayMs: policy.delayMs ?? 0, | ||
| ...policy, | ||
| } | ||
| return policy | ||
| } |
There was a problem hiding this comment.
Check that retry logic (Phase 3) handles optional delayMs field - currently no default is applied when object form is used
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/retry.ts
Line: 3-13
Comment:
Check that retry logic (Phase 3) handles optional `delayMs` field - currently no default is applied when object form is used
How can I resolve this? If you propose a fix, please make it concise.88617c1 to
72fb8cd
Compare
src/lib/retry.ts
Outdated
| delayMs: policy.delayMs ?? 0, | ||
| ...policy, | ||
| } | ||
| return policy |
There was a problem hiding this comment.
Returning the policy object directly breaks the normalization contract - the old implementation filled in defaults (delayMs: 0, backoff: "linear"). Now { limit: 2 } without backoff causes type errors since RetryPolicy requires the discriminant field.
| return policy | |
| return { | |
| delayMs: 0, | |
| jitter: undefined, | |
| shouldRetry: undefined, | |
| ...policy, | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/retry.ts
Line: 12
Comment:
Returning the policy object directly breaks the normalization contract - the old implementation filled in defaults (`delayMs: 0`, `backoff: "linear"`). Now `{ limit: 2 }` without `backoff` causes type errors since `RetryPolicy` requires the discriminant field.
```suggestion
return {
delayMs: 0,
jitter: undefined,
shouldRetry: undefined,
...policy,
}
```
How can I resolve this? If you propose a fix, please make it concise.| return { | ||
| backoff: "linear", | ||
| backoff: "constant", |
There was a problem hiding this comment.
Changed from "linear" to "constant" backoff. The design doc (.context/design.md:327) specifies number shorthand should normalize to { limit: 3, delayMs: 0, backoff: "linear" }, not "constant".
| return { | |
| backoff: "linear", | |
| backoff: "constant", | |
| return { | |
| backoff: "linear", |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/retry.ts
Line: 5-6
Comment:
Changed from `"linear"` to `"constant"` backoff. The design doc (`.context/design.md:327`) specifies number shorthand should normalize to `{ limit: 3, delayMs: 0, backoff: "linear" }`, not `"constant"`.
```suggestion
return {
backoff: "linear",
```
How can I resolve this? If you propose a fix, please make it concise.
src/lib/__tests__/retry.test.ts
Outdated
| }) | ||
|
|
||
| it("defaults omitted backoff to constant", () => { | ||
| expect(normalizeRetryPolicy({ limit: 2 })).toEqual({ |
There was a problem hiding this comment.
{ limit: 2 } without backoff field fails TypeScript compilation - RetryPolicy is now a discriminated union requiring the backoff discriminant. Either fix the test input or adjust the type system to make backoff optional with a default.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/__tests__/retry.test.ts
Line: 16
Comment:
`{ limit: 2 }` without `backoff` field fails TypeScript compilation - `RetryPolicy` is now a discriminated union requiring the `backoff` discriminant. Either fix the test input or adjust the type system to make `backoff` optional with a default.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| describe("normalizeRetryPolicy", () => { | ||
| it("normalizes number shorthand to constant backoff", () => { | ||
| expect(normalizeRetryPolicy(3)).toEqual({ |
There was a problem hiding this comment.
Test expects "constant" but design doc specifies number shorthand normalizes to "linear" backoff (see .context/design.md:327)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/__tests__/retry.test.ts
Line: 8
Comment:
Test expects `"constant"` but design doc specifies number shorthand normalizes to `"linear"` backoff (see `.context/design.md:327`)
How can I resolve this? If you propose a fix, please make it concise.
src/lib/__tests__/retry.test.ts
Outdated
| it("defaults omitted backoff to constant", () => { | ||
| expect(normalizeRetryPolicy({ limit: 2 })).toEqual({ | ||
| backoff: "constant", | ||
| delayMs: 0, | ||
| jitter: undefined, | ||
| limit: 2, | ||
| shouldRetry: undefined, | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Test expects normalized output to include delayMs: 0, jitter: undefined, and shouldRetry: undefined, but normalizeRetryPolicy now returns the input object unchanged (just { limit: 2 }). The assertion will fail.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/__tests__/retry.test.ts
Line: 15-23
Comment:
Test expects normalized output to include `delayMs: 0`, `jitter: undefined`, and `shouldRetry: undefined`, but `normalizeRetryPolicy` now returns the input object unchanged (just `{ limit: 2 }`). The assertion will fail.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/__tests__/retry.test.ts`:
- Around line 15-23: The test about defaulting omitted backoff should be removed
or replaced because it fails type-checking and the function normalizeRetryPolicy
no longer fills defaults; update the test file to either delete this spec or
change it to assert pass-through behavior by providing a valid RetryOptions
object (include an explicit backoff literal) and expecting normalizeRetryPolicy
to return the same object (reference normalizeRetryPolicy, RetryOptions, and
RetryPolicy to find the relevant types and function).
- Around line 25-50: Update the two tests for normalizeRetryPolicy to reflect
that the function no longer injects defaults: change the test names to remove
"with default delay" and assert that providing { backoff: "linear", limit: 2 }
returns the same object (no delayMs added), and that providing { backoff:
"exponential", limit: 4, maxDelayMs: 1000 } returns the same object (no delayMs
added but maxDelayMs remains); remove the noisy jitter: undefined and
shouldRetry: undefined expectations so the assertions match the function's
behavior of returning inputs as-is for normalizeRetryPolicy.
In `@src/lib/types.ts`:
- Around line 71-80: The JSDoc on ConstantBackoffRetryPolicy incorrectly says
"Omit for constant backoff" implying the backoff field is optional; update the
comment for the exported interface ConstantBackoffRetryPolicy (and its backoff
property) to reflect that backoff: "constant" is a required discriminant and not
optional — replace the misleading wording with a clear statement like "Required
discriminant: use a fixed delay between attempts" so the docs match the current
type.
| it("normalizes linear policy with default delay", () => { | ||
| expect(normalizeRetryPolicy({ backoff: "linear", limit: 2 })).toEqual({ | ||
| backoff: "linear", | ||
| delayMs: 0, | ||
| jitter: undefined, | ||
| limit: 2, | ||
| shouldRetry: undefined, | ||
| }) | ||
| }) | ||
|
|
||
| it("normalizes exponential policy and preserves maxDelayMs", () => { | ||
| expect( | ||
| normalizeRetryPolicy({ | ||
| backoff: "exponential", | ||
| limit: 4, | ||
| maxDelayMs: 1000, | ||
| }) | ||
| ).toEqual({ | ||
| backoff: "exponential", | ||
| delayMs: 0, | ||
| jitter: undefined, | ||
| limit: 4, | ||
| maxDelayMs: 1000, | ||
| shouldRetry: undefined, | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Both tests assert synthesized defaults that normalizeRetryPolicy no longer populates — fix expectations and test names
Both the linear (lines 25-33) and exponential (lines 35-50) tests expect delayMs: 0 to be set when delayMs is omitted from the input. This was the old normalization behavior, which the PR intentionally removed. normalizeRetryPolicy now returns object inputs as-is, so neither delayMs: 0 nor any other default is injected.
The jitter: undefined / shouldRetry: undefined fields in the expected objects are benign — toEqual ignores object keys with undefined properties — but they add noise and should be removed for clarity.
The test names ("normalizes … with default delay") are also stale descriptions of the removed behavior and should be updated.
🔧 Proposed fixes for both tests
- it("normalizes linear policy with default delay", () => {
- expect(normalizeRetryPolicy({ backoff: "linear", limit: 2 })).toEqual({
- backoff: "linear",
- delayMs: 0,
- jitter: undefined,
- limit: 2,
- shouldRetry: undefined,
- })
- })
+ it("passes through a linear backoff policy unchanged", () => {
+ expect(normalizeRetryPolicy({ backoff: "linear", limit: 2 })).toEqual({
+ backoff: "linear",
+ limit: 2,
+ })
+ })
- it("normalizes exponential policy and preserves maxDelayMs", () => {
- expect(
- normalizeRetryPolicy({
- backoff: "exponential",
- limit: 4,
- maxDelayMs: 1000,
- })
- ).toEqual({
- backoff: "exponential",
- delayMs: 0,
- jitter: undefined,
- limit: 4,
- maxDelayMs: 1000,
- shouldRetry: undefined,
- })
- })
+ it("passes through an exponential backoff policy and preserves maxDelayMs", () => {
+ expect(
+ normalizeRetryPolicy({
+ backoff: "exponential",
+ limit: 4,
+ maxDelayMs: 1000,
+ })
+ ).toEqual({
+ backoff: "exponential",
+ limit: 4,
+ maxDelayMs: 1000,
+ })
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("normalizes linear policy with default delay", () => { | |
| expect(normalizeRetryPolicy({ backoff: "linear", limit: 2 })).toEqual({ | |
| backoff: "linear", | |
| delayMs: 0, | |
| jitter: undefined, | |
| limit: 2, | |
| shouldRetry: undefined, | |
| }) | |
| }) | |
| it("normalizes exponential policy and preserves maxDelayMs", () => { | |
| expect( | |
| normalizeRetryPolicy({ | |
| backoff: "exponential", | |
| limit: 4, | |
| maxDelayMs: 1000, | |
| }) | |
| ).toEqual({ | |
| backoff: "exponential", | |
| delayMs: 0, | |
| jitter: undefined, | |
| limit: 4, | |
| maxDelayMs: 1000, | |
| shouldRetry: undefined, | |
| }) | |
| }) | |
| it("passes through a linear backoff policy unchanged", () => { | |
| expect(normalizeRetryPolicy({ backoff: "linear", limit: 2 })).toEqual({ | |
| backoff: "linear", | |
| limit: 2, | |
| }) | |
| }) | |
| it("passes through an exponential backoff policy and preserves maxDelayMs", () => { | |
| expect( | |
| normalizeRetryPolicy({ | |
| backoff: "exponential", | |
| limit: 4, | |
| maxDelayMs: 1000, | |
| }) | |
| ).toEqual({ | |
| backoff: "exponential", | |
| limit: 4, | |
| maxDelayMs: 1000, | |
| }) | |
| }) |
🧰 Tools
🪛 GitHub Check: unit
[failure] 42-42: error: expect(received).toEqual(expected)
{
"backoff": "exponential",
-
"delayMs": 0,
-
"jitter": undefined,
"limit": 4,
"maxDelayMs": 1000, -
"shouldRetry": undefined,
} -
Expected - 3
-
Received + 0
at <anonymous> (/home/runner/work/hardtry/hardtry/src/lib/__tests__/retry.test.ts:42:7)
[failure] 26-26: error: expect(received).toEqual(expected)
{
"backoff": "linear",
-
"delayMs": 0,
-
"jitter": undefined,
"limit": 2, -
"shouldRetry": undefined,
} -
Expected - 3
-
Received + 0
at <anonymous> (/home/runner/work/hardtry/hardtry/src/lib/__tests__/retry.test.ts:26:67)
🤖 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 25 - 50, Update the two tests
for normalizeRetryPolicy to reflect that the function no longer injects
defaults: change the test names to remove "with default delay" and assert that
providing { backoff: "linear", limit: 2 } returns the same object (no delayMs
added), and that providing { backoff: "exponential", limit: 4, maxDelayMs: 1000
} returns the same object (no delayMs added but maxDelayMs remains); remove the
noisy jitter: undefined and shouldRetry: undefined expectations so the
assertions match the function's behavior of returning inputs as-is for
normalizeRetryPolicy.
72fb8cd to
5f999ea
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/design.md (1)
153-189:⚠️ Potential issue | 🟡 MinorDesign doc code snippets are stale after the type refactoring.
The
BuilderConfigandTryBuildercode examples in the design doc don't reflect the actual types:
- Line 156:
timeout?: TimeoutOptions→ should betimeout?: TimeoutPolicy(matchessrc/lib/types.tsline 127)- Line 177:
retry(policy: number | RetryPolicy)→ should beretry(policy: RetryOptions)(matchessrc/lib/builder.tsline 30)- Line 184:
timeout(options: number | TimeoutOptions)→ should betimeout(options: TimeoutOptions)(matchessrc/lib/builder.tsline 37)📝 Proposed fix
interface BuilderConfig { retry?: RetryPolicy - timeout?: TimeoutOptions + timeout?: TimeoutPolicy signal?: AbortSignal wraps?: WrapFn[] }- retry(policy: number | RetryPolicy): TryBuilder { + retry(policy: RetryOptions): TryBuilder {- timeout(options: number | TimeoutOptions): TryBuilder { + timeout(options: TimeoutOptions): TryBuilder {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.context/design.md around lines 153 - 189, Update the design doc code snippets to match the refactored types: change BuilderConfig's timeout property from TimeoutOptions to TimeoutPolicy, update TryBuilder.retry signature from retry(policy: number | RetryPolicy) to retry(policy: RetryOptions), and update TryBuilder.timeout signature from timeout(options: number | TimeoutOptions) to timeout(options: TimeoutOptions); keep the rest of the examples identical so they match the actual types defined in src/lib/types.ts and src/lib/builder.ts and reference the same symbol names (BuilderConfig, TryBuilder, retry, timeout).
🧹 Nitpick comments (2)
src/lib/types.ts (1)
49-58: Consider whether linear backoff should also supportmaxDelayMs.Linear delay grows as
delayMs * attempt, which can become arbitrarily large for high attempt counts. CurrentlymaxDelayMs?: neverprevents setting a cap, whileExponentialBackoffRetryPolicyallows it. If a user configures a highlimitwith linear backoff, there's no way to bound the delay.This may be an intentional design choice, but worth confirming — if so, a brief JSDoc note explaining why would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/types.ts` around lines 49 - 58, LinearBackoffRetryPolicy currently forbids maxDelayMs (maxDelayMs?: never) which prevents capping the linear growth of delay (delayMs * attempt); either allow an optional maxDelayMs on LinearBackoffRetryPolicy or add a JSDoc note explaining the deliberate exclusion. To fix, update the LinearBackoffRetryPolicy definition to make maxDelayMs?: number (matching ExponentialBackoffRetryPolicy behavior) so callers can bound delay for high limit values, or add a clear JSDoc comment on LinearBackoffRetryPolicy explaining why a cap is not permitted and documenting the expected growth (delayMs * attempt) and risks for large limit values; reference LinearBackoffRetryPolicy, BaseRetryPolicy, ExponentialBackoffRetryPolicy, delayMs, attempt, and maxDelayMs in the change so reviewers can locate and validate the update.src/lib/retry.ts (1)
13-41: Consider consolidating theconstantandlinearbranches.Both branches produce identical output shapes (same fields, same defaults) — only the
backoffdiscriminant differs. This is fine for explicitness, but if the pattern grows with more backoff strategies, a shared helper could reduce repetition.♻️ Optional: shared helper
+function normalizeBasePolicy( + policy: RetryPolicy, +): Omit<RetryPolicy, "backoff" | "maxDelayMs"> { + return { + delayMs: policy.delayMs ?? 0, + jitter: policy.jitter, + limit: policy.limit, + shouldRetry: policy.shouldRetry, + } +} + // Then in the switch: // case "constant": -// return { backoff: "constant", delayMs: policy.delayMs ?? 0, ... } +// return { ...normalizeBasePolicy(policy), backoff: "constant" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/retry.ts` around lines 13 - 41, The switch over policy.backoff duplicates the "constant" and "linear" branches; consolidate them by merging the two cases into one that returns the shared shape and sets backoff to policy.backoff (or otherwise derives the discriminant) so you avoid repeating delayMs, jitter, limit, and shouldRetry; update the switch around policy.backoff in src/lib/retry.ts (the "constant" and "linear" cases) to use a single branch or a small helper that builds the common object and keep the "exponential" case separate.
🤖 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/design.md:
- Around line 153-189: Update the design doc code snippets to match the
refactored types: change BuilderConfig's timeout property from TimeoutOptions to
TimeoutPolicy, update TryBuilder.retry signature from retry(policy: number |
RetryPolicy) to retry(policy: RetryOptions), and update TryBuilder.timeout
signature from timeout(options: number | TimeoutOptions) to timeout(options:
TimeoutOptions); keep the rest of the examples identical so they match the
actual types defined in src/lib/types.ts and src/lib/builder.ts and reference
the same symbol names (BuilderConfig, TryBuilder, retry, timeout).
---
Duplicate comments:
In `@src/lib/types.ts`:
- Around line 71-80: The JSDoc for the ConstantBackoffRetryPolicy.backoff
property is wrong — it says "Omit for constant backoff" even though backoff:
"constant" is required; update the comment on the backoff property in the
ConstantBackoffRetryPolicy interface to accurately describe that this field is a
required discriminant indicating a fixed delay between attempts (e.g., "Use a
fixed delay between attempts; set to 'constant'"), and leave the maxDelayMs
comment as "Not supported for constant backoff."
---
Nitpick comments:
In `@src/lib/retry.ts`:
- Around line 13-41: The switch over policy.backoff duplicates the "constant"
and "linear" branches; consolidate them by merging the two cases into one that
returns the shared shape and sets backoff to policy.backoff (or otherwise
derives the discriminant) so you avoid repeating delayMs, jitter, limit, and
shouldRetry; update the switch around policy.backoff in src/lib/retry.ts (the
"constant" and "linear" cases) to use a single branch or a small helper that
builds the common object and keep the "exponential" case separate.
In `@src/lib/types.ts`:
- Around line 49-58: LinearBackoffRetryPolicy currently forbids maxDelayMs
(maxDelayMs?: never) which prevents capping the linear growth of delay (delayMs
* attempt); either allow an optional maxDelayMs on LinearBackoffRetryPolicy or
add a JSDoc note explaining the deliberate exclusion. To fix, update the
LinearBackoffRetryPolicy definition to make maxDelayMs?: number (matching
ExponentialBackoffRetryPolicy behavior) so callers can bound delay for high
limit values, or add a clear JSDoc comment on LinearBackoffRetryPolicy
explaining why a cap is not permitted and documenting the expected growth
(delayMs * attempt) and risks for large limit values; reference
LinearBackoffRetryPolicy, BaseRetryPolicy, ExponentialBackoffRetryPolicy,
delayMs, attempt, and maxDelayMs in the change so reviewers can locate and
validate the update.
5f999ea to
9cf5642
Compare
9cf5642 to
154a2e1
Compare

This pull request refactors the type system to improve API ergonomics and type safety:
Type System Improvements:
RetryPolicyintoRetryOptions(user input) andRetryPolicy(internal representation)TimeoutOptionsinto user input type andTimeoutPolicyinternal typeLinearBackoffRetryPolicyandExponentialBackoffRetryPolicymaxDelayMs?: neverconstraint to linear backoff to prevent invalid configurationsAPI Changes:
RetryOptions,TimeoutOptions)normalizeRetryPolicy()- now expects fully formed policiesnormalizeTimeoutOptions()to handle the new type structureDocumentation:
scope: "attempt"timeout semanticsThe changes maintain backward compatibility while providing better type checking and clearer API boundaries between user-facing and internal types.
Summary by CodeRabbit
Refactor
Documentation
Tests
Greptile Summary
This PR refactors the type system to improve API ergonomics and type safety by splitting user-facing input types from internal policy representations.
Key Changes:
ConstantBackoffRetryPolicy,LinearBackoffRetryPolicy,ExponentialBackoffRetryPolicy) with properbackoffdiscriminant fieldsRetryOptions,TimeoutOptions) from internal policy types (RetryPolicy,TimeoutPolicy) to provide clearer API boundariesmaxDelayMs?: neverconstraints to constant and linear backoff types to prevent invalid configurations"linear"to"constant"backoff (intentional breaking change documented in design.md)normalizeRetryPolicy()to use exhaustive switch statement withassertUnreachable()helper for type safetybackofffield now required for object form)Quality:
Confidence Score: 5/5
Important Files Changed
Class Diagram
%%{init: {'theme': 'neutral'}}%% classDiagram class RetryOptions { <<union>> number | RetryPolicy } class RetryPolicy { <<union>> } class BaseRetryPolicy { +number limit +number? delayMs +boolean? jitter +Function? shouldRetry } class ConstantBackoffRetryPolicy { +string backoff = "constant" +never? maxDelayMs } class LinearBackoffRetryPolicy { +string backoff = "linear" +never? maxDelayMs } class ExponentialBackoffRetryPolicy { +string backoff = "exponential" +number? maxDelayMs } class TimeoutOptions { <<union>> number | TimeoutPolicy } class TimeoutPolicy { +number ms +string scope = "total" } RetryPolicy <|-- ConstantBackoffRetryPolicy RetryPolicy <|-- LinearBackoffRetryPolicy RetryPolicy <|-- ExponentialBackoffRetryPolicy BaseRetryPolicy <|-- ConstantBackoffRetryPolicy BaseRetryPolicy <|-- LinearBackoffRetryPolicy BaseRetryPolicy <|-- ExponentialBackoffRetryPolicy RetryOptions --> RetryPolicy TimeoutOptions --> TimeoutPolicyLast reviewed commit: 154a2e1