fix: ENSApi initialization logic#2335
Conversation
Sometimes, the ENSNode Metadata record is written to the ENSDb instance with a delay (up to several miunte). This fix allows the DI container initialization logic to wait for up to 15 minutes for the ENSDb instance to become ready and only then proceed with initialization logic that depends on the ENSNode Metadata record presence.
Pass error objects with the `err` field into `logger.error` method call.
🦋 Changeset detectedLatest commit: 878fad7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@greptile review |
📝 WalkthroughWalkthroughAdds a new ChangesENSDb Readiness Gate for DI Init
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR makes ENSApi startup resilient to ENSDb/indexer cold-start ordering by waiting for ENSDb to report “ready” (i.e., metadata initialized) before continuing DI initialization, avoiding a hard crash when metadata isn’t available yet.
Changes:
- Added a
waitForEnsDbToBeReadyretry helper (15 retries × 60s) and wired it intodi.init()after the health check and before cache initialization. - Added unit tests for the retry helper and extended the DI bootstrap test to cover the retry path.
- Adjusted logging in
index.tsto ensure thrown errors are logged under theerrkey, and added a Changeset entry forensapi.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/ensapi/src/lib/ensdb.ts | Introduces bounded retry helper that waits for ENSDb readiness before proceeding. |
| apps/ensapi/src/lib/ensdb.test.ts | Adds unit tests validating immediate readiness, retry success, and timeout failure behavior. |
| apps/ensapi/src/index.ts | Normalizes error logging to structured { err } payloads for DI init and shutdown paths. |
| apps/ensapi/src/di.ts | Calls the readiness-wait helper during DI init to prevent early cache reads from crashing startup. |
| apps/ensapi/src/config/config.singleton.test.ts | Adds a bootstrap test that exercises the readiness retry path via fake timers. |
| .changeset/ensapi-ensdb-ready.md | Documents the behavior change and bumps ensapi as a patch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| }, | ||
| { | ||
| retries: 15, // This allows for a total of over 15 minutes of retries with 1 minute between attempts. |
| "ensapi": patch | ||
| --- | ||
|
|
||
| ENSApi now waits for ENSDb to become ready before initializing its dependency graph. This prevents startup crashes when ENSApi starts before the relevant ENSNode Metadata record is available in ENSDb instance. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .catch((err) => { | ||
| logger.error( | ||
| { ensIndexerSchemaName, err }, | ||
| `ENSDb readiness check failed after multiple attempts`, | ||
| ); | ||
|
|
||
| // Throw the error to terminate the ENSApi process due to the failed readiness check of a critical dependency | ||
| throw err; | ||
| }); |
There was a problem hiding this comment.
.catch() block inside waitForEnsDbToBeReady logs the error at ERROR level and then re-throws. The caller in di.ts re-throws a wrapped error, which is then caught and logged again at ERROR level by index.ts. This means a single readiness-exhaustion event produces two ERROR log entries — one from ensdb.ts and one from index.ts — with different shapes and stack traces, which can confuse operators. Removing the logger.error call here (keeping only the re-throw) leaves a single, structured ERROR log at the index.ts boundary where process.exit(1) is also triggered.
| .catch((err) => { | |
| logger.error( | |
| { ensIndexerSchemaName, err }, | |
| `ENSDb readiness check failed after multiple attempts`, | |
| ); | |
| // Throw the error to terminate the ENSApi process due to the failed readiness check of a critical dependency | |
| throw err; | |
| }); | |
| .catch((err) => { | |
| // Throw the error to terminate the ENSApi process due to the failed readiness check of a critical dependency. | |
| // The caller (di.init → index.ts) is responsible for logging and exiting. | |
| throw err; | |
| }); |
| } | ||
| }, | ||
| { | ||
| retries: 15, // This allows for a total of over 15 minutes of retries with 1 minute between attempts. |
There was a problem hiding this comment.
| retries: 15, // This allows for a total of over 15 minutes of retries with 1 minute between attempts. | |
| retries: 15, // This allows for a total of up to 15 minutes of retries (15 retries × 60 s interval). |
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!
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : "Unknown error"; | ||
| throw new Error( | ||
| `DI container initialization failed: could not connect to ENSDb due to ${errorMessage}`, | ||
| ); | ||
| throw new Error(`DI container initialization failed: ${errorMessage}`); |
There was a problem hiding this comment.
cause option preserves the original for structured loggers and tooling that can surface error chains.
| } catch (error) { | |
| const errorMessage = error instanceof Error ? error.message : "Unknown error"; | |
| throw new Error( | |
| `DI container initialization failed: could not connect to ENSDb due to ${errorMessage}`, | |
| ); | |
| throw new Error(`DI container initialization failed: ${errorMessage}`); | |
| } catch (error) { | |
| const errorMessage = error instanceof Error ? error.message : "Unknown error"; | |
| throw new Error(`DI container initialization failed: ${errorMessage}`, { cause: error }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ensapi/src/lib/ensdb.test.ts`:
- Around line 54-67: The test suite for waitForEnsDbToBeReady currently only
covers the case where isReady returns false, but does not test the scenario
where isReady() throws or rejects. Add a new test case that creates an
EnsDbClient with an isReady mock that throws an error, then verify that
waitForEnsDbToBeReady immediately rejects with that error without attempting any
retries. This ensures the fail-fast behavior is preserved when isReady() throws
rather than returns false.
In `@apps/ensapi/src/lib/ensdb.ts`:
- Around line 25-33: The retry logic in the ensDbClient.isReady() check does not
distinguish between an intentional "not ready" signal (which should trigger
retry) and actual exceptions thrown by isReady() (which may be non-transient
infrastructure issues). Wrap the ensDbClient.isReady() call in a try-catch block
to separate these two failure modes: when isReady() returns false, throw an
error to trigger retry as intended, but when isReady() throws an exception,
handle it separately (either fail fast or apply different retry logic).
Additionally, add test coverage for scenarios where the isReady() method itself
throws an exception, not just when it returns false.
🪄 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: 6967f067-2258-4d9e-b9fe-7c4775036151
📒 Files selected for processing (6)
.changeset/ensapi-ensdb-ready.mdapps/ensapi/src/config/config.singleton.test.tsapps/ensapi/src/di.tsapps/ensapi/src/index.tsapps/ensapi/src/lib/ensdb.test.tsapps/ensapi/src/lib/ensdb.ts
| it("rejects when the ENSDb instance never becomes ready", async () => { | ||
| const ensDbClient = createEnsDbClient({ isReady: false }); | ||
|
|
||
| const promise = waitForEnsDbToBeReady(ensDbClient); | ||
| // Attach a no-op catch to prevent the promise from being reported as an | ||
| // unhandled rejection while the fake timers are advancing. | ||
| promise.catch(() => {}); | ||
|
|
||
| // Advance past all 15 retry intervals (15 × 60s = 900s). | ||
| await vi.advanceTimersByTimeAsync(900_000); | ||
|
|
||
| await expect(promise).rejects.toThrow("ENSDb instance is not ready yet."); | ||
| expect(ensDbClient.isReady).toHaveBeenCalledTimes(16); // initial + 15 retries | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a regression test for the isReady() rejection path.
Current cases cover boolean readiness states but not isReady() throwing/rejecting. Add one test that asserts immediate rejection (and no long retry loop) so fail-fast behavior is locked in.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ensapi/src/lib/ensdb.test.ts` around lines 54 - 67, The test suite for
waitForEnsDbToBeReady currently only covers the case where isReady returns
false, but does not test the scenario where isReady() throws or rejects. Add a
new test case that creates an EnsDbClient with an isReady mock that throws an
error, then verify that waitForEnsDbToBeReady immediately rejects with that
error without attempting any retries. This ensures the fail-fast behavior is
preserved when isReady() throws rather than returns false.
| return pRetry( | ||
| async () => { | ||
| const isEnsDbInstanceReady = await ensDbClient.isReady(); | ||
|
|
||
| if (!isEnsDbInstanceReady) { | ||
| throw new Error("ENSDb instance is not ready yet."); | ||
| } | ||
| }, | ||
| { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification for retry scope + missing rejection-path coverage.
sed -n '25,48p apps/ensapi/src/lib/ensdb.ts'
rg -n "mockRejectedValue|rejects.*isReady|rejects.*connection|isReady\\(\\).*throw" apps/ensapi/src/lib/ensdb.test.tsRepository: namehash/ensnode
Length of output: 216
🏁 Script executed:
#!/bin/bash
# Read lines 25-48 from the target file
cat -n apps/ensapi/src/lib/ensdb.ts | sed -n '25,48p'
echo "---"
# Search for test coverage of error/rejection paths
rg -A5 "mockRejectedValue|rejects|isReady.*throw|Error.*ENSDb" apps/ensapi/src/lib/ensdb.test.ts | head -50Repository: namehash/ensnode
Length of output: 1643
🏁 Script executed:
#!/bin/bash
# Check for complete test coverage and other error scenarios
echo "=== Looking for all tests in ensdb.test.ts ==="
rg -n "it\(" apps/ensapi/src/lib/ensdb.test.ts
echo ""
echo "=== Check if there are tests for thrown errors from isReady ==="
rg -B3 "throw.*isReady|isReady.*throw" apps/ensapi/src/lib/ensdb.test.ts
echo ""
echo "=== Read the complete waitForEnsDbToBeReady function ==="
sed -n '21,62p' apps/ensapi/src/lib/ensdb.tsRepository: namehash/ensnode
Length of output: 1977
Retry logic conflates transient "not ready" with non-retryable errors from isReady().
The retry mechanism treats all errors within the operation callback identically. If isReady() throws an exception (network failure, timeout, misconfiguration), it will retry the full 15-minute window instead of failing fast. This masks infrastructure problems during startup.
Consider distinguishing between intentional "not ready" signals (deserving retry) and exceptions from isReady() itself (potentially non-transient). Additionally, no test coverage exists for scenarios where isReady() throws—only for when it returns false.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ensapi/src/lib/ensdb.ts` around lines 25 - 33, The retry logic in the
ensDbClient.isReady() check does not distinguish between an intentional "not
ready" signal (which should trigger retry) and actual exceptions thrown by
isReady() (which may be non-transient infrastructure issues). Wrap the
ensDbClient.isReady() call in a try-catch block to separate these two failure
modes: when isReady() returns false, throw an error to trigger retry as
intended, but when isReady() throws an exception, handle it separately (either
fail fast or apply different retry logic). Additionally, add test coverage for
scenarios where the isReady() method itself throws an exception, not just when
it returns false.
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
waitForEnsDbToBeReadyhelper inapps/ensapi/src/lib/ensdb.tswith a 15-minute retry window (15 retries × 60 seconds).apps/ensapi/src/index.tsto ensure error objects are logged under theerrkey.Why
process.exit(1)at startup if it initialized before the ENSDb metadata was ready.Testing
pnpm -F ensapi test— 297 passed.pnpm -F ensapi typecheck— passed.pnpm typecheck(all workspaces) — passed.pnpm linton changed files — no issues.Notes for Reviewer (Optional)
ensDbClient.isHealthy()has already verified connectivity, so the loop only waits for metadata readiness.waitForEnsDbToBeReadyhelper does not cache its promise becausedi.init()runs once per process.Pre-Review Checklist (Blocking)