Conversation
… readiness endpoint
- ENSRainbow now starts its HTTP server immediately, allowing for non-blocking container startup.
- Introduced a new `GET /ready` endpoint to indicate database readiness, returning `200 { status: "ok" }` once ready, and `503 Service Unavailable` during bootstrap.
- Updated API routes to return structured `ServiceUnavailableError` responses while the database is bootstrapping.
- Replaced the previous Docker entrypoint script with a new command that handles the database download and validation in the background.
- Enhanced the ENSIndexer to wait for the new `/ready` endpoint instead of `/health`.
- Migration note: switch from polling `GET /health` to `GET /ready` for database readiness checks.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
🦋 Changeset detectedLatest commit: 89ac55f The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 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 |
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 24 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughIntroduced asynchronous database bootstrapping for ENSRainbow: HTTP server starts immediately while database download and validation proceed in background. Added new Changes
Sequence DiagramsequenceDiagram
actor Client
participant EntryPoint as Entrypoint Command
participant HTTPServer as HTTP Server
participant Bootstrap as DB Bootstrap
participant Database as Database
participant SDK as SDK Client
Client->>EntryPoint: Start ENSRainbow container
EntryPoint->>HTTPServer: Create pending server (no DB)
EntryPoint->>HTTPServer: Start HTTP server immediately
EntryPoint->>Bootstrap: Schedule background bootstrap (setTimeout)
HTTPServer-->>Client: HTTP server ready at :3223
Client->>SDK: Call client.ready()
SDK->>HTTPServer: GET /ready
HTTPServer-->>SDK: 503 Service Unavailable (DB not attached)
SDK-->>Client: Not ready
Bootstrap->>Database: Download prebuilt DB
Bootstrap->>Database: Extract archive
Bootstrap->>Database: Validate database
Bootstrap->>HTTPServer: attachDb(database)
HTTPServer-->>Bootstrap: DB attached, isReady() = true
Bootstrap-->>EntryPoint: Bootstrap complete
Client->>SDK: Call client.ready() (retry)
SDK->>HTTPServer: GET /ready
HTTPServer-->>SDK: 200 { status: "ok" }
SDK-->>Client: Ready
Client->>SDK: Call client.heal(labelhash)
SDK->>HTTPServer: GET /v1/heal/labelhash
HTTPServer->>HTTPServer: isReady() check passes
HTTPServer-->>SDK: 200 (heal response)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates ENSRainbow’s startup lifecycle so the HTTP server binds immediately while the prebuilt DB is bootstrapped in the background, adding a dedicated readiness signal (GET /ready) and propagating that readiness concept through the SDK and ENSIndexer.
Changes:
- Add
GET /readyand return structured503 Service Unavailableresponses from DB-dependent routes until the DB is attached. - Introduce a Node-based container entrypoint (
pnpm run entrypoint) that runs DB download/validation asynchronously while serving/healthimmediately. - Update
@ensnode/ensrainbow-sdk(client.ready(), 503 error types) and migrate ENSIndexer readiness polling from/healthto/ready.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ensrainbow-sdk/src/consts.ts | Add 503 ServiceUnavailable error code constant. |
| packages/ensrainbow-sdk/src/client.ts | Add ready() client method; model 503 service-unavailable responses; exclude transient 503s from heal caching. |
| packages/ensrainbow-sdk/src/client.test.ts | Add tests for client.ready() behavior on 200/503. |
| docs/ensnode.io/src/content/docs/ensrainbow/usage/api.mdx | Document liveness vs readiness and /ready semantics + example 503 body. |
| apps/ensrainbow/src/lib/server.ts | Add “pending” server mode, attachDb, and DbNotReadyError gating when DB isn’t attached. |
| apps/ensrainbow/src/lib/api.ts | Add /ready route and consistent 503 responses for DB-dependent endpoints using a shared helper. |
| apps/ensrainbow/src/commands/server-command.ts | Update API wiring to use a public-config supplier function. |
| apps/ensrainbow/src/commands/server-command.test.ts | Add coverage for /ready and pending/attach lifecycle (503→200 transitions). |
| apps/ensrainbow/src/commands/entrypoint-command.ts | New Node entrypoint that starts HTTP immediately and bootstraps DB in the background. |
| apps/ensrainbow/src/commands/entrypoint-command.test.ts | Add tests for idempotent bootstrap path + pending server smoke test. |
| apps/ensrainbow/src/cli.ts | Add entrypoint CLI command and arg parsing/coercion. |
| apps/ensrainbow/scripts/entrypoint.sh | Remove legacy shell entrypoint (and netcat workaround). |
| apps/ensrainbow/package.json | Add entrypoint script. |
| apps/ensrainbow/Dockerfile | Switch container entrypoint to pnpm run entrypoint; remove netcat dependency. |
| apps/ensindexer/src/lib/ensrainbow/singleton.ts | Poll readiness via client.ready() instead of health(). |
| .changeset/ready-endpoint-bg-bootstrap.md | Release notes + migration guidance for the new readiness endpoint and entrypoint behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(healthData).toEqual({ status: "ok" }); | ||
| const readyRes = await fetch(`${endpoint}/ready`); | ||
| expect(readyRes.status).toBe(503); | ||
|
|
There was a problem hiding this comment.
Remove the trailing whitespace on this blank line to satisfy formatting/linting (Biome typically flags trailing spaces).
Suggested change
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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 `@apps/ensrainbow/src/cli.ts`:
- Around line 39-46: EntrypointCommandCliArgs currently types
"label-set-version" as number but yargs declares it as type: "string" and then
.coerce("label-set-version", buildLabelSetVersion); update the
EntrypointCommandCliArgs interface to match the actual value lifecycle — either
change "label-set-version" to string if you mean the raw CLI input, or to the
post-coercion branded/parsed type returned by buildLabelSetVersion (e.g.,
LabelSetVersion or string|LabelSetVersion) so the field accurately reflects
pre/post-coerce usage and avoids type confusion when using label-set-version
elsewhere.
In `@apps/ensrainbow/src/commands/entrypoint-command.test.ts`:
- Around line 73-76: The pre-bootstrap /ready assertion is flaky because the
existing-DB bootstrap uses setTimeout(..., 0) and may complete before the fetch;
update the test around readyRes/fetch(`${endpoint}/ready`) and
handle.bootstrapComplete to be deterministic by either (A) removing the
intermediate assertion (do not expect 503 before bootstrapping) and only assert
the final state after awaiting handle.bootstrapComplete, or (B) gate the
bootstrap in the test by injecting a test hook/mock so the bootstrap does not
run until you explicitly release it, then assert readyRes.status is 503 before
releasing and 200 after await handle.bootstrapComplete; refer to the readyRes
variable, fetch(`${endpoint}/ready`) call, and handle.bootstrapComplete when
applying the change.
In `@apps/ensrainbow/src/commands/entrypoint-command.ts`:
- Around line 154-168: The catch path must ensure any opened DB handle and its
files are closed/removed before falling back to re-download: when calling
ENSRainbowDB.open(dbSubdir) capture the returned instance (db), and if
subsequent operations (ensRainbowServer.attachDb(db) or buildDbConfig/
buildEnsRainbowPublicConfig) fail, call db.close() (or the DB instance's proper
close/shutdown method) and remove the dbSubdir files (fs.rm or rimraf
equivalent) before proceeding; update the try/catch so the close+remove runs
when db was successfully opened but later steps threw, and keep the existing
logger.warn message to record the fallback.
- Around line 97-103: The shutdown code awaits httpServer.close() even though
`@hono/node-server` returns a Node http.Server whose close is callback-based; wrap
httpServer.close(...) in a Promise inside the close() function so awaiting
actually waits until the server has finished closing (resolve in the close
callback, reject on error) before proceeding to await ensRainbowServer.close();
update the close function (referencing alreadyClosed, httpServer.close,
ensRainbowServer.close, and logger.info) to use this Promise wrapper so the
shutdown fully waits for active connections to finish.
In `@apps/ensrainbow/src/lib/api.ts`:
- Around line 131-137: The /ready handler currently only checks server.isReady()
(in api.get("/ready")), but the routes /v1/config and /v1/labels/count depend on
publicConfigSupplier() being populated after attachDb(), so /ready can return
200 prematurely; change the readiness check to require both server.isReady() and
that the same config supplier used by the other routes (publicConfigSupplier or
an equivalent "configLoaded" flag/state set after publicConfigSupplier() is
populated in the entrypoint) indicates availability before returning 200, i.e.,
gate api.get("/ready") on both server.isReady() and
publicConfigSupplier/populated-state so clients only proceed when /v1/config and
/v1/labels/count will also be healthy.
In `@apps/ensrainbow/src/lib/server.ts`:
- Around line 245-250: The close() method currently awaits this.db.close() while
isReady() still returns true, allowing concurrent handlers to acquire a DB
handle that is being closed; fix it by capturing the DB handle into a local
variable, immediately flip readiness by setting this.db = undefined and
this._serverLabelSet = undefined, then await the capturedHandle.close(); ensure
you reference close(), this.db, isReady(), and _serverLabelSet when making the
change so readiness is cleared before awaiting the close.
In `@docs/ensnode.io/src/content/docs/ensrainbow/usage/api.mdx`:
- Line 17: Update the API docs to replace all leftover `GET /v1/version` details
with `GET /v1/config` and ensure the documented response matches the
readiness-aware `ENSRainbowPublicConfig` shape: include fields `version`,
`dbSchemaVersion`, `labelSet` (with `labelSetId` and `highestLabelSetVersion`)
and add `recordsCount`. Also change the error handling doc to use
`ServiceUnavailableError` with HTTP 503 for DB bootstrap/not-ready scenarios
(remove the old 500 "database is not initialized" case). Ensure curl examples
and JSON samples use `curl https://api.ensrainbow.io/v1/config` and the updated
JSON response shape.
🪄 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: 11f4de95-3df8-4bb0-9699-37adf1b169a5
📒 Files selected for processing (16)
.changeset/ready-endpoint-bg-bootstrap.mdapps/ensindexer/src/lib/ensrainbow/singleton.tsapps/ensrainbow/Dockerfileapps/ensrainbow/package.jsonapps/ensrainbow/scripts/entrypoint.shapps/ensrainbow/src/cli.tsapps/ensrainbow/src/commands/entrypoint-command.test.tsapps/ensrainbow/src/commands/entrypoint-command.tsapps/ensrainbow/src/commands/server-command.test.tsapps/ensrainbow/src/commands/server-command.tsapps/ensrainbow/src/lib/api.tsapps/ensrainbow/src/lib/server.tsdocs/ensnode.io/src/content/docs/ensrainbow/usage/api.mdxpackages/ensrainbow-sdk/src/client.test.tspackages/ensrainbow-sdk/src/client.tspackages/ensrainbow-sdk/src/consts.ts
💤 Files with no reviewable changes (1)
- apps/ensrainbow/scripts/entrypoint.sh
…-download-database-in-background
- Updated the `/ready` endpoint to require both database attachment and public config population for a successful `200` response. - Adjusted API routes to return `ServiceUnavailableError` for `/v1/heal`, `/v1/labels/count`, and `/v1/config` during bootstrap. - Introduced a grace period for child process termination during shutdown. - Changed the `label-set-version` option type from string to number for better validation.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Comment on lines
+160
to
+161
| await ensRainbowServer.attachDb(db); | ||
| return buildEnsRainbowPublicConfig(await buildDbConfig(ensRainbowServer)); |
Contributor
There was a problem hiding this comment.
Database handle not closed when buildDbConfig fails after attachDb succeeds on newly extracted database, leaving stale DB locks
- Added support for graceful shutdown during the bootstrap process, allowing in-flight operations to be aborted and child processes to be terminated properly. - Introduced a new `BootstrapAbortedError` to handle shutdown scenarios during database operations. - Updated the entrypoint command to ensure that partially-opened LevelDB handles are closed promptly, preventing resource leaks. - Enhanced the database bootstrap pipeline to respect abort signals, ensuring a clean exit during shutdown.
…shutdown - Introduced a new utility function `closeHttpServer` to handle the closure of HTTP servers gracefully, ensuring all active connections are completed before shutdown. - Updated the entrypoint and server commands to utilize `closeHttpServer`, improving resource management during server shutdown. - Adjusted tests to reflect changes in the readiness check, ensuring consistent behavior during bootstrap.
- Enhanced error handling in the entrypoint command to ensure proper closure of existing database connections when a bootstrap operation fails. - Introduced logic to manage the state of the database connection, allowing for a clean re-download of the database if the initial opening fails. - Updated logging to provide clearer warnings during database operation failures, improving debugging and resource management.
- Updated the `close` method to ensure readiness state is flipped before awaiting the database closure, preventing concurrent handlers from accessing a closing database. - Introduced a local reference to the database for safer closure management, enhancing error handling during shutdown scenarios.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
apps/ensrainbow/src/commands/server-command.ts:56
shutdownis registered directly as a SIGTERM/SIGINT handler but it’s async and can throw/reject (it rethrows after logging). Since Node doesn’t await signal handlers, this can surface as an unhandled promise rejection during shutdown. Prefer wrapping the handler (e.g.() => void shutdown().catch(...)) and avoid rethrowing from the signal path.
const shutdown = async () => {
logger.info("Shutting down server...");
try {
await closeHttpServer(httpServer);
await db.close();
logger.info("Server shutdown complete");
} catch (error) {
logger.error(error, "Error during shutdown:");
throw error;
}
};
process.on("SIGTERM", shutdown);
process.on("SIGINT", shutdown);
} catch (error) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+220
to
+223
| afterAll(async () => { | ||
| try { | ||
| if (pendingServer) await pendingServer.close(); | ||
| await pendingEnsRainbowServer.close(); |
There was a problem hiding this comment.
serve() returns a Node server whose close() is callback-based, so await pendingServer.close() resolves immediately and can leave the port open / race subsequent teardown. Use the new closeHttpServer() helper (or promisify close) here to make cleanup deterministic and avoid flaky tests/resource leaks.
Comment on lines
+335
to
+342
| logger.info(`Extracting ${archivePath} into ${dataDir}`); | ||
| mkdirSync(dataDir, { recursive: true }); | ||
| await spawnChild( | ||
| "tar", | ||
| ["-xzf", archivePath, "-C", dataDir, "--strip-components=1"], | ||
| {}, | ||
| signal, | ||
| ); |
There was a problem hiding this comment.
The download/extract path doesn’t remove any existing dataDir/dbSubdir contents before running tar. If a previous bootstrap attempt was aborted mid-extraction, stale files can remain and tar won’t delete them, potentially leaving a permanently-corrupt on-disk DB that fails validation on every restart. Consider deleting the target dbSubdir (or clearing dataDir) before extraction when the marker is absent.
Comment on lines
+144
to
+146
| if (options.registerSignalHandlers !== false) { | ||
| process.on("SIGTERM", close); | ||
| process.on("SIGINT", close); |
There was a problem hiding this comment.
close is registered directly as a SIGTERM/SIGINT handler but it’s async and can throw/reject (it rethrows after logging). Node won’t await signal handlers, so a rejection here can become an unhandled promise rejection during shutdown. Wrap the handler (e.g. () => void close().catch(...)) and avoid rethrowing from the signal path so shutdown failures are logged but don’t produce unhandled rejections.
Suggested change
| if (options.registerSignalHandlers !== false) { | |
| process.on("SIGTERM", close); | |
| process.on("SIGINT", close); | |
| const handleSignalClose = () => { | |
| void close().catch(() => { | |
| // `close()` already logs shutdown failures; swallow here so signal-triggered shutdown | |
| // does not create an unhandled promise rejection. | |
| }); | |
| }; | |
| if (options.registerSignalHandlers !== false) { | |
| process.on("SIGTERM", handleSignalClose); | |
| process.on("SIGINT", handleSignalClose); |
- Updated the entrypoint command to resolve the bootstrap promise on shutdown abort, improving error handling during database bootstrap. - Refactored the server command to ensure graceful shutdown by preventing multiple shutdown calls and handling errors more effectively. - Enhanced the `closeHttpServer` utility to treat already stopped servers as a no-op, improving robustness during shutdown scenarios.
| afterAll(async () => { | ||
| try { | ||
| if (pendingServer) await pendingServer.close(); | ||
| await pendingEnsRainbowServer.close(); |
Contributor
There was a problem hiding this comment.
Using await pendingServer.close() doesn't properly wait for the server to close because Node's http.Server.close() is callback-based, not promise-based.
| throw new Error( | ||
| `Expected database archive file not found at ${archivePath} after download completed`, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Tar extraction to dataDir doesn't remove existing files first, so if a previous bootstrap was aborted mid-extraction, stale files remain and tar won't delete them, leaving a permanently corrupt database
Comment on lines
+146
to
+147
| process.on("SIGTERM", close); | ||
| process.on("SIGINT", close); |
Contributor
There was a problem hiding this comment.
Suggested change
| process.on("SIGTERM", close); | |
| process.on("SIGINT", close); | |
| const handleSignal = () => { | |
| close().catch((error) => { | |
| logger.error(error, "Unhandled error in signal handler:"); | |
| process.exit(1); | |
| }); | |
| }; | |
| process.on("SIGTERM", handleSignal); | |
| process.on("SIGINT", handleSignal); |
Signal handlers for SIGTERM/SIGINT register the async close() function directly, which can throw/reject but Node won't await it, causing unhandled promise rejections during shutdown
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
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.
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
netcatworkaround from Implement netcat listener for open port scanning on Render #1607).GET /readyreadiness endpoint;/healthremains a pure liveness probe. Core API routes (/v1/heal/:labelhash,/v1/labels/count,/v1/config) return503 Service Unavailableuntil the DB is attached.ensrainbow-sdkexposes aclient.ready()method andENSIndexer's readiness polling now uses it instead ofhealth().Why
netcat-based orchestration workaround from Implement netcat listener for open port scanning on Render #1607 now that the server itself exposes the right probes.ENSRainbowServeris the proper solution requested by the team.Testing
pnpm -F ensrainbow -F @ensnode/ensrainbow-sdk -F ensindexer typecheck— all clean.pnpm test --project ensrainbow— 160/160 pass, including newentrypoint-command.test.tsand new pending-state cases inserver-command.test.ts(asserting/healthis 200 immediately,/readyand core routes are 503 untilattachDb(), then 200/functional).pnpm test packages/ensrainbow-sdk/src/client.test.ts— 25/25 pass, including new 200 and 503 cases forclient.ready().pnpm test --project ensindexer— 201/201 pass, covering the migratedwaitForEnsRainbowToBeReadyretry loop.ENTRYPOINT ["pnpm", "run", "entrypoint"], netcat removed,wget/tarretained) is covered by unit tests that spawn the command and assert the HTTP lifecycle, but a manualdocker compose upsmoke test would be a good reviewer sanity check.Notes for Reviewer (Optional)
ENSRainbowServernow has a "pending" state:createPending()starts without a DB,attachDb(db)transitions to ready,isReady()gates API handlers, andclose()disposes the DB.heal()/labelCount()throwDbNotReadyErrorbefore attach.api.tsgained aPublicConfigSupplierso/v1/configcan lazily read DB-backed config only once the DB is attached; unavailable → sharedbuildServiceUnavailableBody()helper for consistent 503 responses.entrypointCommandreturns anEntrypointCommandHandle { close(), bootstrapComplete }so tests (and future callers) can await the background bootstrap and release the LevelDB lock cleanly. On bootstrap failure itprocess.exit(1).scripts/entrypoint.shis deleted; the Node.js entrypoint spawns the existingdownload-prebuilt-database.shviachild_processand shells out totarfor extraction — no logic duplication.docs/ensnode.io/src/content/docs/ensrainbow/usage/api.mdxwith a "Liveness vs. Readiness" section and/readyexamples. Changeset:.changeset/ready-endpoint-bg-bootstrap.mdwith a migration note for SDK consumers.isCacheableHealResponsewas updated to explicitly excludeHealServiceUnavailableErrorso clients don't cache transient 503s during bootstrap.Pre-Review Checklist (Blocking)