refactor(errors): unify five retry implementations onto one retryWithBackoff primitive

[kojiwakayama]

Summary

Second PR in the code-reduction campaign (follows #3045, now rebased onto it in main). The survey found four HTTP clients plus the workflow executor each hand-rolling retry with different semantics — four different failure behaviors against the same backend. This PR makes errors/retryWithBackoff the single retry primitive and routes everything through it.

Net: −9 lines (+253/−262, 10 files), but the real win is 5 retry implementations → 1. 2634 unit tests pass; every migrated site has an anchoring test file that passed unmodified.

The primitive

retryWithBackoff(fn(signal, attempt), options) gains: explicit maxAttempts (the old implementations disagreed on whether maxRetries meant total attempts or retries-after-first — now it's unambiguous), shouldRetry, computeDelay, per-attempt timeoutMs with AbortSignal, onRetry, and wrapFinalError(lastError, lastAttempt).

Migrated call sites (semantics preserved per site, verified by existing tests)

Site	Preserved specifics
veryfront-api-client/requestWithRetry	spans, metrics, redacted logs, 4xx-non-429 no-retry, timeout warnings incl. final attempt
token/veryfront client	returns raw Response, 4xx pass-through, TOKEN_STORAGE_ERROR slug
fs/github client	rate-limit-aware delay (waits for reset) + jitter via computeDelay; total-attempt counting
fs/veryfront/withRetryOnTransient	single retry, isTransientError heuristics untouched
workflow step-executor + composite-node-execution	duplicated calculateRetryDelay, retryable classification (status set + code regex), and constants extracted to shared executor/retry-policy.ts; non-cooperative-error checks stay per-caller

Also removes a copy of the canonical sleep that survived #3045 as a private class method in step-executor.

Left alone deliberately

• proxy/retry.ts — proxy-specific behavior, genuinely different concern
• live-evals api-client "retry" — it's fixture polling, not transport retry
• workflow-client has no retry today; adding one would change behavior

Verification

• deno task verify:quick exit 0 (incl. module-boundary lint — uses focused leaf imports in cycle-sensitive platform files)
• deno task test:unit: 2634 passed / 0 failed
• Pre-push hook (fmt + full suite): passed on the rebased branch

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 69e941758c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[Copilot]

Pull request overview

This PR consolidates multiple retry implementations across workflow execution and platform adapters onto a single retryWithBackoff primitive in src/errors/error-handlers.ts, aiming to standardize retry semantics and reduce duplicated logic across the codebase.

Changes:

• Expanded retryWithBackoff into a shared retry primitive (attempt counting, retry classification hooks, per-attempt timeout, custom delay, retry callbacks, and final error wrapping).
• Migrated workflow executor retry classification and delay calculation into a shared src/workflow/executor/retry-policy.ts.
• Updated several HTTP/FS clients to use retryWithBackoff while preserving their site-specific semantics (logging/metrics, status-based retry rules, rate-limit-aware delays, etc.).

Verification

• Not run in this environment.
• Safest next step: run deno task verify:quick and deno task test:unit.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/workflow/executor/step-executor.ts	Reuses shared workflow retry policy and canonical abort-aware sleep.
src/workflow/executor/retry-policy.ts	New shared workflow retry classification and delay computation helpers.
src/workflow/executor/dag/composite-node-execution.ts	Switches composite-node retry classification/delay to shared workflow retry policy.
src/platform/adapters/veryfront-api-client/retry-handler.ts	Migrates API client retries to retryWithBackoff with preserved metrics/logging and 4xx rules.
src/platform/adapters/token/veryfront/api-client.ts	Migrates token storage client retries to retryWithBackoff, preserving Response pass-through and error wrapping.
src/platform/adapters/fs/veryfront/retry.ts	Replaces one-off transient retry helper with a retryWithBackoff call.
src/platform/adapters/fs/github/github-api-client.ts	Routes GitHub API retries through retryWithBackoff, including rate-limit-aware delay logic.
src/errors/index.ts	Re-exports RetryWithBackoffOptions alongside retryWithBackoff.
src/errors/error-handlers.ts	Implements the unified retryWithBackoff primitive with new hooks and per-attempt timeout.
src/errors/error-handlers.test.ts	Updates existing unit tests to use maxAttempts naming.

Comments suppressed due to low confidence (1)

src/errors/error-handlers.test.ts:105

• retryWithBackoff gained several new behaviors (per-attempt timeout AbortSignal, shouldRetry, computeDelay, wrapFinalError, onRetry), but the unit tests here still only cover the basic "eventually succeeds" and "fails after attempts" paths. Adding focused tests for at least shouldRetry short-circuit and timeoutMs (abort produces AbortError + no further retries when configured) would help prevent regressions as more call sites migrate.

  describe("retryWithBackoff", () => {
    it("should return result on first success", async () => {
      let attempts = 0;

      const result = await retryWithBackoff(async () => {
        await Promise.resolve();
        attempts++;
        return "success";
      });

      assertEquals(result, "success");
      assertEquals(attempts, 1);
    });

    it("should retry on failure and succeed", async () => {
      let attempts = 0;

      const result = await retryWithBackoff(
        async () => {
          await Promise.resolve();
          attempts++;
          if (attempts < 2) throw new Error("fail");
          return "success";
        },
        { maxAttempts: 3, initialDelay: 1 },
      );

      assertEquals(result, "success");
      assertEquals(attempts, 2);
    });

    it("should throw after max retries", async () => {
      let attempts = 0;

      await assertRejects(
        () =>
          retryWithBackoff(
            async () => {
              await Promise.resolve();
              attempts++;
              throw new Error("always fails");
            },
            { maxAttempts: 2, initialDelay: 1 },
          ),
        Error,
        "always fails",
      );

      assertEquals(attempts, 2);
    });
  });

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[kojiwakayama]

Responses to the review comments:

• Keep maxRetries as a backward-compatible alias (Codex, P2) — declining: retryWithBackoff had zero callers outside the errors module (verified by repo-wide search before the rename), and { maxRetries: 1 } against the new RetryWithBackoffOptions is a TypeScript excess-property compile error, not a silent fallback — so the described failure mode can't occur. The rename to maxAttempts is deliberate: the old implementations disagreed on what maxRetries meant (github: total attempts; api-client/token: retries after first try), which is precisely the inconsistency this PR removes.
• Non-Error values seen by shouldRetry/computeDelay vs normalized onRetry (Copilot) — intentional: shouldRetry receives the original thrown value so callers can do instanceof checks against their own error types, and the early rethrow preserves the original error identity. onRetry/wrapFinalError get the normalized Error for safe .message/.name access. Documented behavior.
• Tests for the new hook behaviors (Copilot) — agreed; adding focused tests for shouldRetry short-circuit, per-attempt timeoutMs abort, computeDelay override, and wrapFinalError to this branch shortly.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.