Skip to content

ENSRainbow client graceful retry mechanism#1735

Open
djstrong wants to merge 14 commits intomainfrom
214-ensrainbow-client-graceful-retry-mechanism
Open

ENSRainbow client graceful retry mechanism#1735
djstrong wants to merge 14 commits intomainfrom
214-ensrainbow-client-graceful-retry-mechanism

Conversation

@djstrong
Copy link
Contributor

@djstrong djstrong commented Mar 9, 2026

Lite PR

Tip: Review docs on the ENSNode PR process

Summary

  • Add retry-with-backoff inside labelByLabelHash (graphnode-helpers) for ENSRainbow heal() calls during indexing, using p-retry: retries on network errors and transient HealServerError (500) responses (up to 3 retries, 1–30s timeout range).
  • When retries exhaust, throw a new Error with a descriptive message and attach the last thrown error as cause so logs and stack traces retain the underlying failure and retry context.
  • Centralize spy cleanup in graphnode-helpers.test.ts: add afterEach(() => vi.restoreAllMocks()) in the retry-behavior describe and remove per-test warnSpy.mockRestore() so the console.warn spy does not leak across tests.

Why

  • A single transient ENSRainbow failure (network blip or 500) during indexing causes the indexer to fail. Retrying with backoff adds resilience so one error doesn’t cause disproportionate trouble. Relates to #214.
  • Preserving the original error as cause improves debuggability when retries are exhausted.

Testing

  • graphnode-helpers.test.ts: 14 tests, including a “retry behavior” describe with tests for: retry on network/fetch failure then success, retry on HealServerError then success, no retry on HealNotFoundError/HealBadRequestError, exhaustion on persistent network failures, exhaustion on persistent HealServerError (with assertion that the thrown error has the expected cause). Spy cleanup is done in afterEach with vi.restoreAllMocks().

Notes for Reviewer (Optional)

  • Only the heal() path used by labelByLabelHash is retried; other ENSRainbow client usage (e.g. config/health/count/version) is unchanged.
  • When retries exhaust (network or HealServerError), the function throws. For HealServerError exhaustion we throw a new Error with a structured message and the last p-retry error as cause (not a returned HealServerError), so the API contract remains “throw on unrecoverable failure” and logs keep full context.
  • Retry options are in code (defaults: retries: 3, minTimeout: 1_000, maxTimeout: 30_000), not env variables.

Pre-Review Checklist (Blocking)

  • This PR does not introduce significant changes and is low-risk to review quickly.
  • Relevant changesets are included (or are not required)

Copilot AI review requested due to automatic review settings March 9, 2026 11:57
@changeset-bot
Copy link

changeset-bot bot commented Mar 9, 2026

🦋 Changeset detected

Latest commit: d8d3cd9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 19 packages
Name Type
ensindexer Patch
ensadmin Patch
ensrainbow Patch
ensapi Patch
fallback-ensapi Patch
@ensnode/datasources Patch
@ensnode/ensrainbow-sdk Patch
@ensnode/ensnode-schema Patch
@ensnode/ensnode-react Patch
@ensnode/ensnode-sdk Patch
@ensnode/ponder-sdk Patch
@ensnode/ponder-subgraph Patch
@ensnode/shared-configs Patch
@docs/ensnode Patch
@docs/ensrainbow Patch
@docs/mintlify Patch
@namehash/ens-referrals Patch
@namehash/namehash-ui Patch
@ensnode/integration-test-env Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link
Contributor

vercel bot commented Mar 9, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
admin.ensnode.io Ready Ready Preview, Comment Mar 12, 2026 9:39pm
ensnode.io Ready Ready Preview, Comment Mar 12, 2026 9:39pm
ensrainbow.io Ready Ready Preview, Comment Mar 12, 2026 9:39pm

@djstrong djstrong changed the title add plan ENSRainbow client graceful retry mechanism Mar 9, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 9, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR introduces a retry mechanism with exponential backoff (up to 3 attempts, 1–30s timeout) for ENSRainbow API heal() calls during indexing. A new wrapper class delegates most operations directly but wraps heal() with retry logic using p-retry. Type signatures are updated across the codebase from EnsRainbowApiClient to EnsRainbow.ApiClient to reflect the new wrapper interface.

Changes

Cohort / File(s) Summary
Retry Wrapper & Factory
apps/ensindexer/src/lib/ensrainbow-client-with-retry.ts, apps/ensindexer/src/lib/ensraibow-api-client.ts
New wrapper class implementing EnsRainbow.ApiClient with retry logic for heal(); factory updated to return wrapper. Retry distinguishes transient errors (network/HealServerError) from non-transient (HealNotFoundError, HealBadRequestError). Includes per-attempt failure logging.
Type Updates
apps/ensindexer/src/lib/public-config-builder/public-config-builder.ts, apps/ensindexer/src/lib/public-config-builder/public-config-builder.test.ts
Type references updated from EnsRainbowApiClient to EnsRainbow.ApiClient in class field, constructor parameter, and test mocks. No behavioral logic changes.
Retry Logic & Tests
apps/ensindexer/src/lib/graphnode-helpers.ts, apps/ensindexer/src/lib/graphnode-helpers.test.ts
Implements exponential backoff retry for heal() calls with error classification (transient vs. non-transient). Comprehensive test suite validates retry behavior, exhaustion handling, and warning logs. p-retry mocked in tests with zero timeouts.
Documentation
.cursor/plans/ensrainbow_client_retry_wrapper_e7106d29.plan.md, .changeset/purple-clubs-strive.md
Plan document and changeset entry describing retry mechanism and benefits.

Sequence Diagram

sequenceDiagram
    participant Indexer as Indexing Process
    participant Wrapper as Retry Wrapper
    participant Retry as p-retry
    participant Client as ENSRainbow Client
    participant API as ENSRainbow API

    Indexer->>Wrapper: heal(labelHash)
    Wrapper->>Retry: execute with retry config
    loop Retry Loop (up to 3 attempts)
        Retry->>Client: heal(labelHash)
        Client->>API: HTTP request
        alt Success (HealSuccess)
            API-->>Client: label resolved
            Client-->>Retry: return LiteralLabel
            Retry-->>Wrapper: success
        else Transient Error (HealServerError / Network)
            API-->>Client: error
            Client-->>Retry: throw error
            Retry->>Wrapper: log warning, retry
        else Non-Transient (404 / 400)
            API-->>Client: error
            Client-->>Retry: throw HealNotFoundError or HealBadRequestError
            Retry-->>Wrapper: no retry, fail immediately
        end
    end
    Wrapper-->>Indexer: result or throw non-recoverable error
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 A wrapper hops around with grace,
Retrying heals at gentle pace,
Three chances granted, backoff sweet,
Transient errors taste defeat! ✨
Yet network blips won't break the race,
The indexer runs with retry's embrace! 🌈

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 17.39% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding a graceful retry mechanism to the ENSRainbow client, which aligns with the primary objective of the PR.
Description check ✅ Passed PR description is well-structured following the template with all required sections: Summary (3 bullets), Why (with issue reference), Testing (detailed test approach), Notes for Reviewer, and completed Pre-Review Checklist.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 214-ensrainbow-client-graceful-retry-mechanism
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a Cursor “plan” document describing the intended approach for implementing an ENSRainbow API client retry wrapper in ensindexer (issue #214).

Changes:

  • Introduces a new plan file outlining retry-wrapper design, wiring, and test strategy.
  • Documents current ensindexer call paths (startup/config fetch and heal path) and why long-duration retries are needed.
  • Proposes using p-retry with warning logs on failed attempts.

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.cursor/plans/ensrainbow_client_retry_wrapper_e7106d29.plan.md:
- Line 67: The wrapper cannot be a plain object typed as EnsRainbowApiClient if
EnsRainbowApiClient is a concrete class; update the plan and implementation by
either (a) extracting/defining an interface (e.g. IEnsRainbowClient) that
declares the public methods used (heal, config, getOptions, optional health) and
type the wrapper and factory to return that interface, (b) make the wrapper
actually extend EnsRainbowApiClient so it is an instance of the class and can be
returned as EnsRainbowApiClient, or (c) keep structural typing but ensure the
wrapper implements all methods and change the factory return type to the
interface/union to reflect compatibility; update references to
EnsRainbowApiClient and the factory return type accordingly so consumers remain
type-safe.
- Around line 39-44: Update the plan to explicitly recommend making retry timing
configurable via environment variables rather than fixed defaults: propose
specific env var names (ENSRAINBOW_RETRY_MAX_WAIT_MS,
ENSRAINBOW_RETRY_MIN_TIMEOUT_MS, ENSRAINBOW_RETRY_ATTEMPTS) with suggested
defaults (e.g. ~20min max wait, min timeout 30_000–60_000 ms, 40 attempts), note
that these defaults can be tuned per environment (dev/staging/prod), and include
a short caution to document how these values interact with the worker's existing
p-retry/backoff behavior so operators can adjust without code changes.
- Around line 77-81: Add a test that simulates the long-duration startup retry
by exercising the ENSRainbow retry wrapper (reference ENSRainbowClientWithRetry
or the factory getENSRainbowApiClient) to ensure it tolerates many consecutive
failures before succeeding; make the wrapper's backoff/delay configurable
(expose a short test-specific backoff via constructor params or env) so tests
can mock timers and advance time (use fake timers to simulate e.g. 10 failed
attempts then success without waiting 20 minutes), assert number of attempts,
that console.warn is called on each failure, and that final success returns the
result; also document/update how existing tests mock getENSRainbowApiClient to
either return a mock client behind the wrapper or mock the factory so the
wrapper is not exercised.
- Around line 23-27: The review points out nested retries between the
EnsDbWriterWorker's p-retry around getValidatedEnsIndexerPublicConfig (which
calls publicConfigBuilder.getPublicConfig -> ensRainbowClient.config) and the
new retry wrapper; resolve by choosing one of the approaches: (A) make the
wrapper detect "startup mode" (via an env flag or a parameter passed from
startEnsDbWriterWorker) and only apply long-duration retry there while using
short retries for runtime heal()/config(); or (B) remove/reduce the worker's
p-retry and let the wrapper own all retry behavior (adjusting wrapper retry
counts/backoff accordingly); implement the chosen approach by updating
startEnsDbWriterWorker/startup call site to pass the mode or by modifying
EnsDbWriterWorker's retry configuration so getValidatedEnsIndexerPublicConfig no
longer nests retries with ensRainbowClient.config, and document the behavior
change.
- Line 42: The ordered list formatting in the markdown uses explicit incremental
numbers (e.g., "2. **Health endpoint**") which violates the project's
auto-numbering style; update all ordered list items in this file (e.g., the line
containing "2. **Health endpoint**") to use "1." for every list item so the list
uses Markdown's automatic numbering convention.
- Around line 42-44: The retry wrapper currently considers wrapping client
methods indiscriminately; exclude the health() method from the retry policy (or
make per-method policies configurable) so health() calls remain fast-failing for
orchestration, while keeping heal() and config() wrapped with the existing
retry/backoff; update the retry wrapper (ensrainbow_client_retry_wrapper / retry
decorator) to check the method name (health) and either bypass retries or apply
a separate no/low-retry policy, and expose a simple per-method config map so
callers can override behavior if needed.
- Around line 45-48: Clarify and reconcile retry semantics between functions:
explicitly state that heal() returns discriminated union errors
(HealServerError, HealNotFoundError, HealBadRequestError) and does not throw on
HTTP error responses while config() throws on !response.ok, and either (a)
update the retry wrapper to treat returned HealServerError (and any
non-cacheable marker / server-5xx-like errorCode) as retryable by inspecting
heal()’s response object, or (b) document that heal() errors are non-retryable
and adjust the plan/questions to call out this thrown-vs-returned distinction
and justify which error classes (HealServerError vs
HealBadRequestError/HealNotFoundError) should be retried. Ensure you reference
heal(), config(), HealServerError/HealNotFoundError/HealBadRequestError and the
non-cacheable marker when updating the text or implementing the wrapper.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: a64265fb-17d8-4a2a-b731-0e9e196e94f8

📥 Commits

Reviewing files that changed from the base of the PR and between b3b1a54 and 9ff9c6a.

📒 Files selected for processing (1)
  • .cursor/plans/ensrainbow_client_retry_wrapper_e7106d29.plan.md

…lures during indexing. Enhance overview to clarify retry mechanism for `heal()` with backoff and logging, while ensuring `config()` remains a plain delegation. Introduce a wrapper class for `heal()` with retry logic using p-retry, allowing for long-duration waits at startup. Adjust retry policy parameters for better control.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
.cursor/plans/ensrainbow_client_retry_wrapper_e7106d29.plan.md (1)

22-22: ⚠️ Potential issue | 🟡 Minor

Align this sentence with the later heal() error semantics.

Line 22 still says network failures can “return HealServerError”, but the later section correctly says network/fetch failures throw and HTTP server failures are returned as HealServerError. Keeping both versions in the plan is confusing.

Suggested fix
-The SDK uses `fetch()`; network failures cause `heal()` to throw (or return HealServerError). Applying the wrapper at the factory ensures `heal()` gets retries without touching handlers.
+The SDK uses `fetch()`; network/fetch failures cause `heal()` to throw, while HTTP server failures are returned as `HealServerError`. Applying the wrapper at the factory ensures `heal()` gets retries without touching handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.cursor/plans/ensrainbow_client_retry_wrapper_e7106d29.plan.md at line 22,
Update the sentence to match the later semantics: state that fetch()/network
failures cause heal() to throw (not return) an exception while HTTP server
failures are returned as a HealServerError; mention applying the wrapper at the
factory ensures heal() gets retries without changing handlers. Reference heal(),
HealServerError, fetch(), and the factory/wrapper so the author knows which
concepts to edit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.cursor/plans/ensrainbow_client_retry_wrapper_e7106d29.plan.md:
- Around line 18-20: In the plan file
.cursor/plans/ensrainbow_client_retry_wrapper_e7106d29.plan.md the repo-relative
Markdown links like "[apps/ensindexer/src/lib/ensraibow-api-client.ts]" and
other "[apps/...]" entries are resolving incorrectly from the .cursor/plans
directory; update those link targets to be rebased one level up (change
"apps/..." to "../../apps/...") throughout the file so they point to the correct
repository paths (apply the same "../../" prefix to all subsequent links that
start with "apps/").

---

Duplicate comments:
In @.cursor/plans/ensrainbow_client_retry_wrapper_e7106d29.plan.md:
- Line 22: Update the sentence to match the later semantics: state that
fetch()/network failures cause heal() to throw (not return) an exception while
HTTP server failures are returned as a HealServerError; mention applying the
wrapper at the factory ensures heal() gets retries without changing handlers.
Reference heal(), HealServerError, fetch(), and the factory/wrapper so the
author knows which concepts to edit.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 55ab0f22-9ef6-4de5-8920-f23145c5c38a

📥 Commits

Reviewing files that changed from the base of the PR and between 9ff9c6a and c913838.

📒 Files selected for processing (1)
  • .cursor/plans/ensrainbow_client_retry_wrapper_e7106d29.plan.md

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.


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

shrugs and others added 5 commits March 10, 2026 11:29
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: lightwalker.eth <126201998+lightwalker-eth@users.noreply.github.com>
Implement a new `EnsRainbowClientWithRetry` class to handle transient failures in `heal()` with exponential backoff and logging. Update the `getENSRainbowApiClient` function to return this new client. Add tests to ensure retry behavior works as expected, including handling various error scenarios and logging warnings on failed attempts.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/ensindexer/src/lib/ensrainbow-client-with-retry.ts`:
- Around line 43-76: Add a short inline comment above the catch block (around
the return of lastServerError in heal) explaining the deliberate asymmetric
error handling: that when the inner call returns a HealServerError (detected by
isHealError and ErrorCode.ServerError) we store and return that error after
retries to preserve API compatibility with callers like labelByLabelHash (in
graphnode-helpers.ts) that expect heal to resolve with a HealServerError value,
whereas actual network/fetch exceptions are rethrown after retry exhaustion;
reference heal and lastServerError in the comment to make intent clear to future
maintainers.

In `@packages/integration-test-env/src/orchestrator.ts`:
- Around line 349-355: The execaSync invocation currently sets env to only
ENSNODE_URL, which omits inheriting process.env (so PATH and other vars are
lost); update the execaSync call that runs pnpm test:integration to merge the
current environment and override ENSNODE_URL (e.g. env: { ...process.env,
ENSNODE_URL: `http://localhost:${ENSAPI_PORT}` }) — mirror the same fix used in
spawnService so the test runner gets PATH and other required env vars while
still passing ENSNODE_URL to the process.
- Around line 156-164: The spawn call is replacing the whole environment instead
of inheriting parent env; update spawnService to merge process.env with the
provided env before passing to spawn (e.g., const mergedEnv = { ...process.env,
...env } and pass mergedEnv), so subprocess created by spawn (the subprocess
variable) retains PATH, HOME, etc., while still allowing overrides from the
provided env.
- Around line 188-191: The dynamic import of EnsIndexerClient inside
pollIndexingStatus causes inconsistent imports; add EnsIndexerClient to the
existing static import alongside OmnichainIndexingStatusIds from
"@ensnode/ensnode-sdk", then replace the dynamic new (await
import(...)).EnsIndexerClient usage in pollIndexingStatus with a direct new
EnsIndexerClient({ url: new URL(ENSINDEXER_URL) }) to keep imports consistent
and improve startup performance.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: a6852a3a-1659-4aa0-a5b6-5392fc19a9f9

📥 Commits

Reviewing files that changed from the base of the PR and between c913838 and a77acd7.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (29)
  • .changeset/fluffy-hairs-draw.md
  • .changeset/purple-clubs-strive.md
  • .changeset/strong-baboons-help.md
  • .github/workflows/test_ci.yml
  • apps/ensapi/src/graphql-api/schema/domain.ts
  • apps/ensapi/src/graphql-api/schema/query.integration.test.ts
  • apps/ensapi/src/handlers/ensnode-graphql-api.ts
  • apps/ensapi/src/handlers/subgraph-api.ts
  • apps/ensapi/src/middleware/require-core-plugin.middleware.ts
  • apps/ensapi/src/test/integration/ensnode-graphql-api-client.ts
  • apps/ensapi/src/test/integration/global-setup.ts
  • apps/ensindexer/ponder/ponder.config.ts
  • apps/ensindexer/src/config/validations.ts
  • apps/ensindexer/src/lib/ensraibow-api-client.ts
  • apps/ensindexer/src/lib/ensrainbow-client-with-retry.test.ts
  • apps/ensindexer/src/lib/ensrainbow-client-with-retry.ts
  • apps/ensindexer/src/lib/public-config-builder/public-config-builder.test.ts
  • apps/ensindexer/src/lib/public-config-builder/public-config-builder.ts
  • apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts
  • apps/ensindexer/src/plugins/index.ts
  • apps/ensrainbow/scripts/download-prebuilt-database.sh
  • package.json
  • packages/ensnode-schema/src/schemas/ensv2.schema.ts
  • packages/ensnode-schema/src/schemas/protocol-acceleration.schema.ts
  • packages/ensnode-sdk/src/graphql-api/example-queries.ts
  • packages/integration-test-env/LICENSE
  • packages/integration-test-env/README.md
  • packages/integration-test-env/package.json
  • packages/integration-test-env/src/orchestrator.ts
💤 Files with no reviewable changes (2)
  • apps/ensapi/src/middleware/require-core-plugin.middleware.ts
  • apps/ensindexer/src/plugins/index.ts

@@ -0,0 +1,69 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor planning artifact committed to repository

This file is a Cursor AI editor planning artifact (.cursor/plans/) and is an internal development tool artifact that should not be committed to the repository. It is not relevant to the production codebase and will add noise to the git history.

Consider adding .cursor/ to the project's .gitignore to prevent this from happening again.


if (isHealError(response) && response.errorCode === ErrorCode.ServerError) {
lastServerError = response;
throw new Error(response.error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning message loses error type context on HealServerError

When a HealServerError response is received, the code converts it to a thrown Error(response.error). The resulting onFailedAttempt warning message only includes the server's error text (e.g., "Internal server error"), making it ambiguous whether the failure was an HTTP 500 response or a network-level throw.

Including the errorCode in the warning would make logs more actionable when diagnosing transient vs. network failures:

Suggested change
throw new Error(response.error);
throw Object.assign(new Error(response.error), { errorCode: response.errorCode });

And in onFailedAttempt, use (error as any).errorCode or define a typed error class to surface the HTTP status in the log line.

Copy link
Member

@lightwalker-eth lightwalker-eth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djstrong Thanks for updates 👍 Reviewed and shared feedback.

@vercel vercel bot temporarily deployed to Preview – admin.ensnode.io March 12, 2026 19:51 Inactive
@vercel vercel bot temporarily deployed to Preview – ensrainbow.io March 12, 2026 19:51 Inactive
@vercel vercel bot temporarily deployed to Preview – ensnode.io March 12, 2026 19:51 Inactive
@djstrong djstrong requested a review from Copilot March 12, 2026 19:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comments suppressed due to low confidence (1)

apps/ensindexer/src/lib/ensraibow-api-client.ts:20

  • The PR description/plan says getENSRainbowApiClient() should return a retrying wrapper (e.g. EnsRainbowClientWithRetry) so all heal() callers get retries transparently, but this factory still returns the raw EnsRainbowApiClient. Either wire the retry wrapper in here (and add the missing wrapper module), or update the PR description/plan/changeset to reflect that retries are implemented at the labelByLabelHash() call site instead.
export function getENSRainbowApiClient(): EnsRainbow.ApiClient {
  const ensRainbowApiClient = new EnsRainbowApiClient({
    endpointUrl: config.ensRainbowUrl,
    labelSet: config.labelSet,
  });

  if (
    ensRainbowApiClient.getOptions().endpointUrl ===
    EnsRainbowApiClient.defaultOptions().endpointUrl
  ) {
    console.warn(
      `Using default public ENSRainbow server which may cause increased network latency. For production, use your own ENSRainbow server that runs on the same network as the ENSIndexer server.`,
    );
  }

  return ensRainbowApiClient;

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

You can also share your feedback on Copilot code review. Take the survey.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/ensindexer/src/lib/ensraibow-api-client.ts (1)

5-20: ⚠️ Potential issue | 🟠 Major

Wire retry handling transparently into this factory or delegate explicitly.

The factory returns a raw EnsRainbowApiClient without retry wrapping. While labelByLabelHash() in graphnode-helpers.ts wraps the specific heal() call with pRetry(), this retry logic is not centralized at the factory boundary. Other call sites like public-config-builder/singleton.ts:4 receive the raw client and have no retry handling. Either return a wrapped client from the factory that handles retries transparently, or document that callers are responsible for implementing retry logic where needed.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/ensindexer/src/lib/ensraibow-api-client.ts` around lines 5 - 20, The
factory getENSRainbowApiClient currently returns a raw EnsRainbowApiClient
without centralized retry behavior; update getENSRainbowApiClient to return a
client wrapper that transparently retries network/heal calls (use the same
pRetry options used in labelByLabelHash or a shared retry policy) so callers
(e.g., public-config-builder/singleton.ts) don't need to implement retries;
implement the wrapper around EnsRainbowApiClient methods that perform network
requests (or decorate EnsRainbowApiClient.heal/labelByLabelHash-equivalents) and
ensure the wrapper delegates successful responses and surfaces errors after
retry exhaustion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/ensindexer/src/lib/graphnode-helpers.test.ts`:
- Around line 152-169: Add a global cleanup so spies are always restored: remove
per-test warnSpy.mockRestore() calls and add an afterEach that calls
vi.restoreAllMocks() (or vi.resetAllMocks() as appropriate) alongside existing
beforeEach clearing; update tests referencing labelByLabelHash to rely on the
centralized afterEach cleanup to avoid leaking the console.warn spy across the
four test cases.

---

Outside diff comments:
In `@apps/ensindexer/src/lib/ensraibow-api-client.ts`:
- Around line 5-20: The factory getENSRainbowApiClient currently returns a raw
EnsRainbowApiClient without centralized retry behavior; update
getENSRainbowApiClient to return a client wrapper that transparently retries
network/heal calls (use the same pRetry options used in labelByLabelHash or a
shared retry policy) so callers (e.g., public-config-builder/singleton.ts) don't
need to implement retries; implement the wrapper around EnsRainbowApiClient
methods that perform network requests (or decorate
EnsRainbowApiClient.heal/labelByLabelHash-equivalents) and ensure the wrapper
delegates successful responses and surfaces errors after retry exhaustion.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 86fef2d2-0f3c-4a6f-ad82-d124201605fe

📥 Commits

Reviewing files that changed from the base of the PR and between 39a6604 and e293949.

📒 Files selected for processing (3)
  • apps/ensindexer/src/lib/ensraibow-api-client.ts
  • apps/ensindexer/src/lib/graphnode-helpers.test.ts
  • apps/ensindexer/src/lib/graphnode-helpers.ts

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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


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

You can also share your feedback on Copilot code review. Take the survey.

…mocks and improve error handling in retry logic. Update error throwing to include cause for better debugging.
@vercel vercel bot temporarily deployed to Preview – ensrainbow.io March 12, 2026 21:38 Inactive
@vercel vercel bot temporarily deployed to Preview – ensnode.io March 12, 2026 21:38 Inactive
@vercel vercel bot temporarily deployed to Preview – admin.ensnode.io March 12, 2026 21:38 Inactive
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comments suppressed due to low confidence (1)

apps/ensindexer/src/lib/graphnode-helpers.ts:86

  • The network-error exhaustion path rethrows the original error after mutating error.message, but does not wrap/attach the underlying failure as cause. This contradicts the PR description (“attach the last thrown error as cause when retries exhaust”) and also drops useful context (original message) for callers; consider throwing a new Error with a descriptive message (include endpoint + labelHash) and set { cause: error } (similar to apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts:177-180) instead of mutating and rethrowing the original error.
    // Not recoverable; causes the ENSIndexer process to terminate.
    if (error instanceof Error) {
      error.message = `ENSRainbow Heal Request Failed: ENSRainbow unavailable at '${ensRainbowApiClient.getOptions().endpointUrl}'.`;
    }

    throw error;

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

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +38 to +39
* @throws if the labelHash is not correctly formatted, or a transient error persists after all
* retries are exhausted.
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated @throws doc no longer mentions non-transient Heal errors (e.g. HealBadRequestError / 400). The function still throws for any isHealError(response) other than NotFound, so the doc should include that it can throw immediately on non-retryable ENSRainbow error responses as well (not only on “transient error persists after retries”).

Suggested change
* @throws if the labelHash is not correctly formatted, or a transient error persists after all
* retries are exhausted.
* @throws if ENSRainbow returns a non-transient Heal error (any Heal error other than NotFound,
* e.g. HealBadRequestError / 400), if the labelHash is not correctly formatted, or if a
* transient error (network failure or HealServerError / 500) persists after all retries are
* exhausted.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants