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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 83a4453 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 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 |
📝 WalkthroughWalkthroughHTTP server now starts immediately while database download/validation runs in the background; added 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 ENSRainbow SDK
Client->>Entrypoint: start container
Entrypoint->>HTTPServer: create pending server (no DB)
Entrypoint->>HTTPServer: start HTTP server immediately
Entrypoint->>Bootstrap: start background DB bootstrap
HTTPServer-->>Client: accept connections (liveness /health OK)
Client->>SDK: client.ready() (poll)
SDK->>HTTPServer: GET /ready
HTTPServer-->>SDK: 503 ServiceUnavailable (bootstrapping)
SDK-->>Client: not ready
Bootstrap->>Database: download, extract, validate, open
Bootstrap->>HTTPServer: attachDb(database)
HTTPServer-->>Bootstrap: isReady() = true
Client->>SDK: client.ready() (retry)
SDK->>HTTPServer: GET /ready
HTTPServer-->>SDK: 200 { status: "ok" }
SDK-->>Client: ready
Client->>HTTPServer: GET /v1/heal/...
HTTPServer->>HTTPServer: isReady() check passes
HTTPServer-->>Client: 200 (heal response)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 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 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).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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}
| await ensRainbowServer.attachDb(db); | ||
| return buildEnsRainbowPublicConfig(await buildDbConfig(ensRainbowServer)); |
There was a problem hiding this comment.
Database handle not closed when non-abort errors occur 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.
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.
- 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.
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.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| - **New `GET /ready` endpoint**: returns `200 { status: "ok" }` once the database is attached, or `503 Service Unavailable` while ENSRainbow is still bootstrapping. `/health` is now a pure liveness probe that succeeds as soon as the HTTP server is listening. | ||
| - **503 responses for API routes during bootstrap**: `/v1/heal`, `/v1/labels/count`, and `/v1/config` return a structured `ServiceUnavailableError` (`errorCode: 503`) until the database is ready. | ||
| - **New Docker entrypoint**: the container now runs `pnpm --filter ensrainbow run entrypoint` (implemented in Node via `tsx src/cli.ts entrypoint`), which replaces `scripts/entrypoint.sh` and the `netcat` workaround. |
There was a problem hiding this comment.
The changeset says the container runs pnpm --filter ensrainbow run entrypoint, but the Dockerfile now sets WORKDIR /app/apps/ensrainbow and runs pnpm run entrypoint. Consider updating this line to match the actual container invocation (or vice versa) so the migration note is unambiguous.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
apps/ensrainbow/src/cli.ts (1)
39-46: 🧹 Nitpick | 🔵 TrivialInterface still reflects pre-coerce types only; post-coerce branded values leak into
entrypointCommanduntyped.
EntrypointCommandCliArgsnow matches the yargs option types (string/number), which is correct for the pre-coerce value. However, the.coerce(...)calls on lines 166 and 174 mutate the runtime values to brandedLabelSetId/LabelSetVersion, so at the handler on lines 183-195,argv["label-set-id"]/argv["label-set-version"]are typed as primitives while at runtime they are branded types passed toentrypointCommand.If
entrypointCommandrequires the branded types, the call compiles only because of brand compatibility edges; consider either casting at the call site or typing the interface to reflect the post-coerce shape so the handler carries correct types.Also applies to: 183-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensrainbow/src/cli.ts` around lines 39 - 46, EntrypointCommandCliArgs currently describes pre-coerce primitive types while the yargs .coerce(...) transforms argv["label-set-id"] / argv["label-set-version"] into branded LabelSetId / LabelSetVersion, so update types to match the post-coerce shape or perform an explicit cast at the call site to avoid leaking branded values as primitives; specifically either change EntrypointCommandCliArgs to use LabelSetId and LabelSetVersion for "label-set-id"/"label-set-version" so the handler and entrypointCommand(arg...) receive correctly typed branded values, or leave the interface as-is and cast argv["label-set-id"] and argv["label-set-version"] to LabelSetId/LabelSetVersion when invoking entrypointCommand.docs/ensnode.io/src/content/docs/ensrainbow/usage/api.mdx (1)
154-161:⚠️ Potential issue | 🟡 MinorStale error documentation: 503 bootstrap case still missing from
/v1/healand/v1/labels/counterror sections.The endpoint summary (line 16) and the new "Liveness vs. Readiness" section already declare these routes return
503 ServiceUnavailableErrorwhile the DB is bootstrapping, but:
- Lines 154-161 still attribute "database is not initialized" to a 500 response.
- Lines 183-191 (
/v1/labels/countError Responses) still list only the 500 variant.Please align these two subsections with the new 503 semantics documented earlier in the file.
Suggested direction
-- `500 Internal Server Error`: When an unexpected error occurs or database is not initialized +- `500 Internal Server Error`: When an unexpected error occurs ```json { "status": "error", "error": "Internal server error", "errorCode": 500 }
+-
503 Service Unavailable: While the database is still bootstrapping (see "Liveness vs. Readiness" above)And a matching 503 example in the `/v1/labels/count` Error Responses section. </details> Also applies to: 183-191 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/ensnode.io/src/content/docs/ensrainbow/usage/api.mdxaround lines 154 -
161, Update the error response docs for the /v1/heal and /v1/labels/count
sections to reflect the new 503 bootstrap semantics: replace or augment the
current "database is not initialized" 500 description with a503 Service Unavailableentry explaining it occurs while the DB is bootstrapping
(referencing the "Liveness vs. Readiness" section), and add a matching 503 JSON
example alongside the existing 500 example; target the/v1/heal Error Responsesand/v1/labels/count Error Responsessubsections so both list both
the 500 Internal Server Error example and the new 503 Service Unavailable
example for bootstrapping.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@apps/ensrainbow/src/commands/entrypoint-command.test.ts:
- Around line 167-188: The test currently attaches
process.once("unhandledRejection", unhandledRejection) but never removes it and
only waits one macrotask after invoking sigtermHandler, which can leak the mock
across tests and race the close rejection; update the test around
entrypointCommand/sigtermHandler to explicitly remove the listener (call
process.removeListener or use the returned cleanup) after the assertion and
instead of setTimeout(r,0) await the cleanup of the server by awaiting
localHandle.close() (or drive/await
bootstrapSettled/closeHttpServer/ensRainbowServer.close deterministically)
before asserting unhandledRejection was not called so the listener is cleaned
and the close-chain rejection has settled.In
@apps/ensrainbow/src/commands/entrypoint-command.ts:
- Around line 95-96: Replace the two console.log calls in entrypoint-command
(the config dump lines using stringifyConfig and options) with the project
logger so the output is level-aware and consistent; call logger.info (or
logger.debug if you prefer lower verbosity) and pass the same
stringifyConfig(options, { pretty: true }) string as the message so the config
is emitted via the existing logging pipeline instead of stdout.- Around line 267-274: The freshly-opened DB path doesn't ensure the DB is
closed if ensRainbowServer.attachDb(db) throws; update the block around
ENSRainbowDB.open(dbSubdir) so that after opening the DB you run attachDb in a
try/catch (or finally) and call safeClose(db) if attachDb throws or if
signal.aborted, then rethrow the error (or throw BootstrapAbortedError for
aborts) — mirror the existing-DB branch's pattern used for tracking attach state
to guarantee the LevelDB handle is released even on attach failures.In
@packages/ensrainbow-sdk/src/client.ts:
- Around line 401-410: The failing test mocks fetch but doesn't set
response.ok/status, and since client.health() now throws when !response.ok (see
health()), update the test in src/client.test.ts to mock fetch to return an
object with ok: true and a numeric status (e.g., 200) and optionally statusText,
plus the existing json() implementation, so the health() call no longer hits the
error branch and returns the mocked JSON.
Duplicate comments:
In@apps/ensrainbow/src/cli.ts:
- Around line 39-46: EntrypointCommandCliArgs currently describes pre-coerce
primitive types while the yargs .coerce(...) transforms argv["label-set-id"] /
argv["label-set-version"] into branded LabelSetId / LabelSetVersion, so update
types to match the post-coerce shape or perform an explicit cast at the call
site to avoid leaking branded values as primitives; specifically either change
EntrypointCommandCliArgs to use LabelSetId and LabelSetVersion for
"label-set-id"/"label-set-version" so the handler and entrypointCommand(arg...)
receive correctly typed branded values, or leave the interface as-is and cast
argv["label-set-id"] and argv["label-set-version"] to LabelSetId/LabelSetVersion
when invoking entrypointCommand.In
@docs/ensnode.io/src/content/docs/ensrainbow/usage/api.mdx:
- Around line 154-161: Update the error response docs for the /v1/heal and
/v1/labels/count sections to reflect the new 503 bootstrap semantics: replace or
augment the current "database is not initialized" 500 description with a503 Service Unavailableentry explaining it occurs while the DB is bootstrapping
(referencing the "Liveness vs. Readiness" section), and add a matching 503 JSON
example alongside the existing 500 example; target the/v1/heal Error Responsesand/v1/labels/count Error Responsessubsections so both list both
the 500 Internal Server Error example and the new 503 Service Unavailable
example for bootstrapping.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: ASSERTIVE **Plan**: Pro **Run ID**: `11f89471-0dee-4946-b3da-7f60f4aaaff7` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 8dcebcbff2588ee4cf1a0e5f741ffa062d03af21 and a438a6acfa134a866c101d95fa4eaa08ddd498ff. </details> <details> <summary>📒 Files selected for processing (12)</summary> * `.changeset/ready-endpoint-bg-bootstrap.md` * `apps/ensrainbow/src/cli.ts` * `apps/ensrainbow/src/commands/entrypoint-command.test.ts` * `apps/ensrainbow/src/commands/entrypoint-command.ts` * `apps/ensrainbow/src/commands/server-command.test.ts` * `apps/ensrainbow/src/commands/server-command.ts` * `apps/ensrainbow/src/lib/api.ts` * `apps/ensrainbow/src/lib/server.ts` * `apps/ensrainbow/src/utils/http-server.ts` * `docs/ensnode.io/src/content/docs/ensrainbow/usage/api.mdx` * `packages/ensrainbow-sdk/src/client.test.ts` * `packages/ensrainbow-sdk/src/client.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| if (!response.ok) { | ||
| throw new Error( | ||
| `ENSRainbow health check failed (HTTP ${response.status}${ | ||
| response.statusText ? ` ${response.statusText}` : "" | ||
| })`, | ||
| ); | ||
| } | ||
|
|
||
| return response.json() as Promise<EnsRainbow.HealthResponse>; | ||
| } |
There was a problem hiding this comment.
health() behavior change breaks existing unit test.
Static analysis reports src/client.test.ts > should return a positive health check failing with ENSRainbow health check failed (HTTP undefined). Since health() now throws on !response.ok, existing tests that mock fetch with only a json() (and no ok: true/status: 200) fall through this branch and fail. The test mock needs to be updated alongside this runtime change — otherwise CI stays red.
🩹 Likely fix in src/client.test.ts around the failing case
- global.fetch = vi.fn().mockResolvedValue({
- json: () => Promise.resolve({ status: "ok" }),
- });
+ global.fetch = vi.fn().mockResolvedValue({
+ ok: true,
+ status: 200,
+ json: () => Promise.resolve({ status: "ok" }),
+ });🧰 Tools
🪛 GitHub Check: Unit Tests
[failure] 402-402: src/client.test.ts > EnsRainbowApiClient > should return a positive health check
Error: ENSRainbow health check failed (HTTP undefined)
❯ EnsRainbowApiClient.health src/client.ts:402:13
❯ src/client.test.ts:267:22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ensrainbow-sdk/src/client.ts` around lines 401 - 410, The failing
test mocks fetch but doesn't set response.ok/status, and since client.health()
now throws when !response.ok (see health()), update the test in
src/client.test.ts to mock fetch to return an object with ok: true and a numeric
status (e.g., 200) and optionally statusText, plus the existing json()
implementation, so the health() call no longer hits the error branch and returns
the mocked JSON.
…perations - Updated logging in the entrypoint command to use the logger for better consistency and clarity. - Enhanced error handling in the database bootstrap process to ensure proper closure of database connections on abort signals. - Refactored the `close` method in the server class to prevent multiple shutdown calls and ensure safe database closure. - Cleaned up code formatting for improved readability in various functions.
- Updated comments and documentation to clarify the readiness checks and bootstrap process. - Enhanced the `/ready` endpoint to ensure it accurately reflects the state of both the database and public configuration. - Improved clarity in the `EntrypointCommandHandle` interface regarding bootstrap completion and error handling. - Adjusted the CLI options documentation to better describe the post-coercion values. - Minor adjustments to comments in the database bootstrap pipeline for better understanding.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ensrainbow-sdk/src/client.ts (1)
444-452:⚠️ Potential issue | 🟡 MinorMinor:
config()should also distinguish 503 bootstrap errors from permanent failures.
apps/ensrainbow/src/lib/api.tsnow returns a structured 503 on/v1/configuntil bootstrap completes, butconfig()collapses every non-OK response into the same generic message using onlyresponse.statusText. Callers cannot tell whether to retry (bootstrap-in-progress) or to fail fast (misrouting/500). Consider mirroringready()’s 503 branch for consistency.♻️ Proposed tweak
async config(): Promise<EnsRainbow.ENSRainbowPublicConfig> { const response = await fetch(new URL("/v1/config", this.options.endpointUrl)); if (!response.ok) { - throw new Error(`Failed to fetch ENSRainbow config: ${response.statusText}`); + const statusSuffix = `HTTP ${response.status}${ + response.statusText ? ` ${response.statusText}` : "" + }`; + if (response.status === 503) { + throw new Error(`ENSRainbow config: service not ready yet (${statusSuffix})`); + } + throw new Error(`Failed to fetch ENSRainbow config (${statusSuffix})`); } return response.json() as Promise<EnsRainbow.ENSRainbowPublicConfig>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ensrainbow-sdk/src/client.ts` around lines 444 - 452, The config() method currently throws a generic Error for any non-OK response and thus hides retryable bootstrap-in-progress 503s; update config() (in packages/ensrainbow-sdk/src/client.ts) to mirror ready()’s 503 handling for the GET /v1/config call: when response.status === 503 parse the structured JSON body (as ready() does), create/throw a distinct bootstrap-in-progress error (or include a retryable flag/message) so callers can distinguish transient bootstrap 503s from permanent failures, otherwise keep the existing error behavior for other non-OK statuses and return the parsed EnsRainbow.ENSRainbowPublicConfig on success.
♻️ Duplicate comments (1)
apps/ensrainbow/src/commands/entrypoint-command.ts (1)
255-273:⚠️ Potential issue | 🟡 MinorDB lock leaks if
writeFile(markerFile)orbuildDbConfig()throws after a successfulattachDb.The catch block only closes the attached server for abort errors (
error instanceof BootstrapAbortedError || signal.aborted). A non-abort failure fromwriteFile(e.g., EIO / ENOSPC) or frombuildDbConfig(e.g.,labelCount()surfacing a database read error) afterdbAttached = truefalls through tothrow errorwithout closing the server, leaving the LevelDB lock held. In production this is masked by the subsequentprocess.exit(1), but for programmatic callers and tests the lock leaks.🛡️ Proposed fix
} catch (error) { if (!dbAttached) { await safeClose(db); - } else if (error instanceof BootstrapAbortedError || signal.aborted) { + } else { try { await ensRainbowServer.close(); } catch (closeError) { logger.warn( closeError, - "Failed to close server while aborting after DB attach; continuing", + "Failed to close server after post-attach bootstrap failure; continuing", ); } } throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensrainbow/src/commands/entrypoint-command.ts` around lines 255 - 273, The catch block must always close the attached server when attachDb succeeded to avoid LevelDB lock leaks; change the error handling so that if dbAttached is true you attempt to await ensRainbowServer.close() (wrap in its own try/catch and logger.warn on failure) before rethrowing the error, and still ensure the DB handle is closed (use safeClose(db) appropriately) so both the server and DB are cleaned up when writeFile(markerFile) or buildDbConfig(...) throws after attachDb; update the catch in entrypoint-command.ts referencing dbAttached, ensRainbowServer.close, safeClose(db), writeFile(markerFile) and buildDbConfig to implement this unconditional server-close behavior for non-abort errors as well.
🤖 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/commands/entrypoint-command.ts`:
- Around line 111-126: The shutdown handler (close) currently aborts
bootstrapping then awaits bootstrapSettled and runs closeHttpServer(httpServer)
before ensRainbowServer.close(), so if closeHttpServer throws the latter never
runs and the LevelDB lock leaks; change close to run each cleanup step
independently: after bootstrapAborter.abort() await bootstrapSettled, then call
closeHttpServer(httpServer) and ensRainbowServer.close() in separate try/catch
blocks (or use Promise.allSettled) so both are attempted regardless of the
other's failure, collect any errors and after all steps log them and rethrow an
aggregated or first error; keep alreadyClosed guard and logger messages intact
and reference the close function, closeHttpServer, ensRainbowServer.close,
bootstrapSettled, and bootstrapAborter.abort when making the changes.
In `@apps/ensrainbow/src/lib/server.ts`:
- Around line 130-135: The code re-casts _serverLabelSet in heal and
buildDbConfig after relying on the invariant set by attachDb; consolidate that
check into requireDb by either (a) changing requireDb to return a typed pair {
db, serverLabelSet } or (b) adding a new requireReady() method that asserts
this.db and this._serverLabelSet and returns both typed values, then update heal
and buildDbConfig to call the new helper instead of casting _serverLabelSet;
reference the methods requireDb, heal, buildDbConfig, attachDb and the field
_serverLabelSet when making the change so the invariant is centralized and the
type casts can be removed.
---
Outside diff comments:
In `@packages/ensrainbow-sdk/src/client.ts`:
- Around line 444-452: The config() method currently throws a generic Error for
any non-OK response and thus hides retryable bootstrap-in-progress 503s; update
config() (in packages/ensrainbow-sdk/src/client.ts) to mirror ready()’s 503
handling for the GET /v1/config call: when response.status === 503 parse the
structured JSON body (as ready() does), create/throw a distinct
bootstrap-in-progress error (or include a retryable flag/message) so callers can
distinguish transient bootstrap 503s from permanent failures, otherwise keep the
existing error behavior for other non-OK statuses and return the parsed
EnsRainbow.ENSRainbowPublicConfig on success.
---
Duplicate comments:
In `@apps/ensrainbow/src/commands/entrypoint-command.ts`:
- Around line 255-273: The catch block must always close the attached server
when attachDb succeeded to avoid LevelDB lock leaks; change the error handling
so that if dbAttached is true you attempt to await ensRainbowServer.close()
(wrap in its own try/catch and logger.warn on failure) before rethrowing the
error, and still ensure the DB handle is closed (use safeClose(db)
appropriately) so both the server and DB are cleaned up when
writeFile(markerFile) or buildDbConfig(...) throws after attachDb; update the
catch in entrypoint-command.ts referencing dbAttached, ensRainbowServer.close,
safeClose(db), writeFile(markerFile) and buildDbConfig to implement this
unconditional server-close behavior for non-abort errors as well.
🪄 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: 9244b088-ae65-4bda-a408-b70ba28d1ac4
📒 Files selected for processing (12)
apps/ensindexer/src/lib/ensrainbow/singleton.tsapps/ensrainbow/src/cli.tsapps/ensrainbow/src/commands/entrypoint-command.test.tsapps/ensrainbow/src/commands/entrypoint-command.tsapps/ensrainbow/src/lib/api.tsapps/ensrainbow/src/lib/server.tsapps/ensrainbow/src/utils/http-server.tsdocs/ensnode.io/src/content/docs/ensrainbow/concepts/glossary.mdxdocs/ensnode.io/src/content/docs/ensrainbow/index.mdxdocs/ensnode.io/src/content/docs/ensrainbow/usage/index.mdxpackages/ensrainbow-sdk/src/client.test.tspackages/ensrainbow-sdk/src/client.ts
- Updated the PublicConfigBuilder to await readiness before fetching ENSRainbow config, improving the reliability of public configuration retrieval. - Added error handling to ensure that config fetching does not proceed if the readiness check fails. - Enhanced unit tests to cover new readiness logic and error scenarios, ensuring robust behavior during configuration retrieval.
…-download-database-in-background
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let alreadyClosed = false; | ||
| const close = async () => { | ||
| if (alreadyClosed) return; | ||
| alreadyClosed = true; | ||
| logger.info("Shutting down server..."); | ||
| bootstrapAborter.abort(); | ||
| // Wait for bootstrap cleanup before closing shared resources. | ||
| await bootstrapSettled; | ||
|
|
||
| let shutdownError: unknown; | ||
|
|
||
| try { | ||
| await closeHttpServer(httpServer); | ||
| } catch (error) { | ||
| shutdownError = error; | ||
| logger.error(error, "Failed to close HTTP server during shutdown"); | ||
| } | ||
|
|
||
| try { | ||
| await ensRainbowServer.close(); | ||
| } catch (error) { | ||
| if (shutdownError === undefined) { | ||
| shutdownError = error; | ||
| } | ||
| logger.error(error, "Failed to close ENSRainbow server/database during shutdown"); | ||
| } | ||
|
|
||
| if (shutdownError !== undefined) { | ||
| throw shutdownError; | ||
| } | ||
|
|
||
| logger.info("Server shutdown complete"); |
There was a problem hiding this comment.
close() is not re-entrant: if shutdown is triggered via the SIGTERM/SIGINT handler (which calls close() without awaiting it), a subsequent explicit handle.close() call will hit alreadyClosed and resolve immediately without waiting for the in-flight shutdown to finish. This can lead to callers thinking the server/DB are closed while the close is still running. Consider tracking and returning a shared closePromise (similar to shutdownPromise in server-command) so all callers can await the same shutdown work.
| let alreadyClosed = false; | |
| const close = async () => { | |
| if (alreadyClosed) return; | |
| alreadyClosed = true; | |
| logger.info("Shutting down server..."); | |
| bootstrapAborter.abort(); | |
| // Wait for bootstrap cleanup before closing shared resources. | |
| await bootstrapSettled; | |
| let shutdownError: unknown; | |
| try { | |
| await closeHttpServer(httpServer); | |
| } catch (error) { | |
| shutdownError = error; | |
| logger.error(error, "Failed to close HTTP server during shutdown"); | |
| } | |
| try { | |
| await ensRainbowServer.close(); | |
| } catch (error) { | |
| if (shutdownError === undefined) { | |
| shutdownError = error; | |
| } | |
| logger.error(error, "Failed to close ENSRainbow server/database during shutdown"); | |
| } | |
| if (shutdownError !== undefined) { | |
| throw shutdownError; | |
| } | |
| logger.info("Server shutdown complete"); | |
| let closePromise: Promise<void> | undefined; | |
| const close = async () => { | |
| if (closePromise !== undefined) { | |
| return closePromise; | |
| } | |
| closePromise = (async () => { | |
| logger.info("Shutting down server..."); | |
| bootstrapAborter.abort(); | |
| // Wait for bootstrap cleanup before closing shared resources. | |
| await bootstrapSettled; | |
| let shutdownError: unknown; | |
| try { | |
| await closeHttpServer(httpServer); | |
| } catch (error) { | |
| shutdownError = error; | |
| logger.error(error, "Failed to close HTTP server during shutdown"); | |
| } | |
| try { | |
| await ensRainbowServer.close(); | |
| } catch (error) { | |
| if (shutdownError === undefined) { | |
| shutdownError = error; | |
| } | |
| logger.error(error, "Failed to close ENSRainbow server/database during shutdown"); | |
| } | |
| if (shutdownError !== undefined) { | |
| throw shutdownError; | |
| } | |
| logger.info("Server shutdown complete"); | |
| })(); | |
| return closePromise; |
| // Clean stale state from previous aborted attempts. | ||
| rmSync(downloadTempDir, { recursive: true, force: true }); | ||
| mkdirSync(downloadTempDir, { recursive: true }); |
There was a problem hiding this comment.
The bootstrap path uses synchronous filesystem operations (rmSync/mkdirSync) while the HTTP server is already running. Deleting or recreating large directories (e.g. the LevelDB dbSubdir) can block the Node event loop, potentially making /health//ready intermittently unresponsive during bootstrap or fallback re-download. Consider switching to async fs.promises.rm/mkdir (or offloading deletes to a child process/worker) so probes remain responsive under heavy disk I/O.
|
|
||
| - **New `GET /ready` endpoint**: returns `200 { status: "ok" }` once the database is attached, or `503 Service Unavailable` while ENSRainbow is still bootstrapping. `/health` is now a pure liveness probe that succeeds as soon as the HTTP server is listening. | ||
| - **503 responses for API routes during bootstrap**: `/v1/heal`, `/v1/labels/count`, and `/v1/config` return a structured `ServiceUnavailableError` (`errorCode: 503`) until the database is ready. | ||
| - **New Docker entrypoint**: the container now runs `pnpm --filter ensrainbow run entrypoint` (implemented in Node via `tsx src/cli.ts entrypoint`), which replaces `scripts/entrypoint.sh` and the `netcat` workaround. |
There was a problem hiding this comment.
The changeset says the container runs pnpm --filter ensrainbow run entrypoint, but the Dockerfile sets WORKDIR /app/apps/ensrainbow and uses ENTRYPOINT ["pnpm", "run", "entrypoint"] (no filter). Consider updating the changeset text to match the actual container entrypoint so release notes don’t mislead operators.
| - **New Docker entrypoint**: the container now runs `pnpm --filter ensrainbow run entrypoint` (implemented in Node via `tsx src/cli.ts entrypoint`), which replaces `scripts/entrypoint.sh` and the `netcat` workaround. | |
| - **New Docker entrypoint**: the container now runs `pnpm run entrypoint` (implemented in Node via `tsx src/cli.ts entrypoint`), which replaces `scripts/entrypoint.sh` and the `netcat` workaround. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/public-config-builder/public-config-builder.ts`:
- Line 21: Add a short JSDoc description for the new readiness hook field and
constructor parameter so it matches neighboring docs: document the private field
waitForEnsRainbowReady and the corresponding constructor parameter as "a
one-time async readiness hook called before the first ensRainbowClient.config()
invocation" (or similar), and update the constructor JSDoc to include this
parameter alongside the existing ensRainbowClient and immutablePublicConfig
entries to make the contract explicit for readers.
In `@apps/ensrainbow/src/commands/entrypoint-command.ts`:
- Around line 218-241: The catch block currently checks abort and rethrows
before cleaning up DB handles, which can leak LevelDB locks; change the flow so
that regardless of signal.aborted or BootstrapAbortedError you first attempt to
close any opened handles (call ensRainbowServer.close() when existingDbAttached,
and safeClose(existingDb) when existingDb !== undefined), then if the abort
condition (signal.aborted or error instanceof BootstrapAbortedError) is true
rethrow without performing rmSync or re-download logic; only perform
rmSync(dbSubdir, ...) and the logger.warn about re-downloading when not aborted.
Ensure you keep the existing guarded try/catch around ensRainbowServer.close()
and maintain the same log messages/behavior when continuing to fallback.
🪄 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: e8af5e58-7aee-4a38-ba7b-8343815a78f3
📒 Files selected for processing (8)
apps/ensindexer/src/lib/public-config-builder/public-config-builder.test.tsapps/ensindexer/src/lib/public-config-builder/public-config-builder.tsapps/ensindexer/src/lib/public-config-builder/singleton.tsapps/ensrainbow/package.jsonapps/ensrainbow/src/commands/entrypoint-command.tsapps/ensrainbow/src/commands/server-command.tsapps/ensrainbow/src/lib/server.tspackages/ensrainbow-sdk/src/client.test.ts
| * the ENSIndexer Public Config. | ||
| */ | ||
| private ensRainbowClient: EnsRainbow.ApiClient; | ||
| private waitForEnsRainbowReady: () => Promise<void>; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document the new waitForEnsRainbowReady field/param for consistency.
Neighboring fields (ensRainbowClient, immutablePublicConfig) have JSDoc explaining their purpose, and the constructor's JSDoc only documents ensRainbowClient. The new readiness-hook field and parameter are introduced without similar context, which makes the "called once before the first ensRainbowClient.config()" contract non-obvious to readers.
📝 Proposed doc addition
private ensRainbowClient: EnsRainbow.ApiClient;
+ /**
+ * Readiness hook awaited once before the first ENSRainbow `config()` call.
+ * Defaults to a no-op so tests and other callers can opt out.
+ */
private waitForEnsRainbowReady: () => Promise<void>;
@@
/**
* `@param` ensRainbowClient ENSRainbow Client instance used to fetch ENSRainbow Public Config
+ * `@param` waitForEnsRainbowReady Optional readiness hook awaited before the first
+ * `ensRainbowClient.config()` call on the initial `getPublicConfig()` invocation.
*/
constructor(
ensRainbowClient: EnsRainbow.ApiClient,
waitForEnsRainbowReady: () => Promise<void> = async () => {},
) {As per coding guidelines: "Maintain comment consistency within a file: if most types, schemas, or declarations lack comments, do not add a comment to a single one" — the inverse applies here since the other fields in this class are documented.
Also applies to: 31-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensindexer/src/lib/public-config-builder/public-config-builder.ts` at
line 21, Add a short JSDoc description for the new readiness hook field and
constructor parameter so it matches neighboring docs: document the private field
waitForEnsRainbowReady and the corresponding constructor parameter as "a
one-time async readiness hook called before the first ensRainbowClient.config()
invocation" (or similar), and update the constructor JSDoc to include this
parameter alongside the existing ensRainbowClient and immutablePublicConfig
entries to make the contract explicit for readers.
| } catch (error) { | ||
| if (error instanceof BootstrapAbortedError || signal.aborted) { | ||
| throw error; | ||
| } | ||
| // Ensure LevelDB lock is released before fallback re-extract. | ||
| if (existingDbAttached) { | ||
| try { | ||
| await ensRainbowServer.close(); | ||
| } catch (closeError) { | ||
| logger.warn( | ||
| closeError, | ||
| "Failed to close server while falling back to re-download; continuing", | ||
| ); | ||
| } | ||
| } else if (existingDb !== undefined) { | ||
| await safeClose(existingDb); | ||
| } | ||
| rmSync(dbSubdir, { recursive: true, force: true }); | ||
| logger.warn( | ||
| error, | ||
| "Existing ENSRainbow database failed to open or validate; re-downloading from scratch", | ||
| ); | ||
| // Fall through to re-download. | ||
| } |
There was a problem hiding this comment.
Existing-DB catch may leak the LevelDB handle when abort races with attachDb failure.
In this catch block, the abort/aborted check rethrows BEFORE the DB-handle cleanup. If ENSRainbowDB.open(dbSubdir) succeeded (existingDb set) but attachDb then throws for a non-abort reason (e.g., db.validate({ lite: true }) returning false) while signal.aborted has become true due to a concurrent close(), the handler rethrows without calling safeClose(existingDb). That leaks the LevelDB LOCK on the reused data directory for the rest of the process lifetime — fine under process.exit(1) in production, but can produce flaky state in direct-invocation tests.
Closing the handle is safe regardless of abort status (the subsequent rmSync(dbSubdir, …) / fall-through is what we want to skip on abort, not the handle close).
🛡️ Proposed fix: close the handle first, then honor abort
} catch (error) {
- if (error instanceof BootstrapAbortedError || signal.aborted) {
- throw error;
- }
- // Ensure LevelDB lock is released before fallback re-extract.
+ // Always release any opened handle before deciding how to proceed.
if (existingDbAttached) {
try {
await ensRainbowServer.close();
} catch (closeError) {
logger.warn(
closeError,
- "Failed to close server while falling back to re-download; continuing",
+ "Failed to close server after existing-DB failure; continuing",
);
}
} else if (existingDb !== undefined) {
await safeClose(existingDb);
}
+ if (error instanceof BootstrapAbortedError || signal.aborted) {
+ throw error;
+ }
rmSync(dbSubdir, { recursive: true, force: true });
logger.warn(
error,
"Existing ENSRainbow database failed to open or validate; re-downloading from scratch",
);
// Fall through to re-download.
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensrainbow/src/commands/entrypoint-command.ts` around lines 218 - 241,
The catch block currently checks abort and rethrows before cleaning up DB
handles, which can leak LevelDB locks; change the flow so that regardless of
signal.aborted or BootstrapAbortedError you first attempt to close any opened
handles (call ensRainbowServer.close() when existingDbAttached, and
safeClose(existingDb) when existingDb !== undefined), then if the abort
condition (signal.aborted or error instanceof BootstrapAbortedError) is true
rethrow without performing rmSync or re-download logic; only perform
rmSync(dbSubdir, ...) and the logger.warn about re-downloading when not aborted.
Ensure you keep the existing guarded try/catch around ensRainbowServer.close()
and maintain the same log messages/behavior when continuing to fallback.
Greptile SummaryThis PR replaces the blocking Three P2 issues were found in the bootstrap lifecycle code worth addressing before a production rollout. Confidence Score: 4/5Safe to merge with the P2 LevelDB leak fix applied; all reported issues are edge-case races during shutdown rather than steady-state correctness problems. All findings are P2: a LevelDB handle leak under a concurrent abort+DB-error race (unlikely in production), an assignment-order issue in attachDb that could allow a double-attach only in a zero-latency microtask race (also very unlikely), and dangling process.once listeners after programmatic close(). The core architecture—pending server, attachDb, isReady gating, /ready endpoint, and graceful shutdown—is well-reasoned and thoroughly tested. apps/ensrainbow/src/commands/entrypoint-command.ts and apps/ensrainbow/src/lib/server.ts deserve a second look for the resource-management edge cases identified. Important Files Changed
Sequence DiagramsequenceDiagram
participant Docker as Docker / Orchestrator
participant CLI as entrypointCommand
participant HTTP as HTTP Server
participant BG as Background Bootstrap
participant DB as ENSRainbowDB
Docker->>CLI: start container
CLI->>HTTP: serve() on :3223 (immediate)
CLI->>BG: setTimeout(runDbBootstrap, 0)
CLI-->>Docker: entrypointCommand resolves (HTTP is live)
Docker->>HTTP: GET /health → 200 ok (liveness ✓)
Docker->>HTTP: GET /ready → 503 (not yet ready)
BG->>DB: ENSRainbowDB.open() or download+extract
DB-->>BG: db handle
BG->>HTTP: ensRainbowServer.attachDb(db)
Note over BG,HTTP: isReady() → true, cachedPublicConfig set
BG-->>CLI: bootstrapComplete resolves
Docker->>HTTP: GET /ready → 200 ok (readiness ✓)
Docker->>HTTP: GET /v1/heal/:labelhash → 200/404
Note over Docker,HTTP: SIGTERM received
Docker->>CLI: close()
CLI->>BG: bootstrapAborter.abort()
CLI->>CLI: await bootstrapSettled
BG-->>CLI: bootstrap aborted / finished
CLI->>HTTP: closeHttpServer()
CLI->>DB: ensRainbowServer.close()
Reviews (1): Last reviewed commit: "Merge branch 'main' into 1610-start-ensr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| mkdirSync(dataDir, { recursive: true }); | ||
|
|
||
| if (existsSync(markerFile) && existsSync(dbSubdir)) { | ||
| logger.info( | ||
| `Found existing ENSRainbow marker at ${markerFile}; attempting to open existing database at ${dbSubdir}`, | ||
| ); | ||
| // Track DB ownership so cleanup chooses the correct close path. | ||
| let existingDb: ENSRainbowDB | undefined; | ||
| let existingDbAttached = false; | ||
| try { | ||
| throwIfAborted(signal); | ||
| existingDb = await ENSRainbowDB.open(dbSubdir); | ||
| if (signal.aborted) { | ||
| await safeClose(existingDb); | ||
| throw new BootstrapAbortedError(); | ||
| } |
There was a problem hiding this comment.
In the "reuse existing DB" path, when attachDb throws a non-abort error while signal.aborted has become true (e.g., SIGTERM arrives while db.validate() or db.getLabelSet() is in-flight), the catch block re-throws without closing existingDb:
if (error instanceof BootstrapAbortedError || signal.aborted) {
throw error; // existingDb was opened but attachDb threw before this.db was set
}
// Only reached when signal is NOT aborted — won't fire in this scenario
} else if (existingDb !== undefined) {
await safeClose(existingDb);
}Because existingDbAttached is still false, neither the ensRainbowServer.close() branch nor safeClose(existingDb) runs. The existingDb handle leaks, holding the LevelDB lock until process exit. On a crash-restart the next ENSRainbowDB.open() call may fail to acquire the lock.
The fix is to close existingDb in the re-throw path when it was opened but not attached:
if (error instanceof BootstrapAbortedError || signal.aborted) {
if (!existingDbAttached && existingDb !== undefined) {
await safeClose(existingDb);
}
throw error;
}| await closeHttpServer(httpServer); | ||
| } catch (error) { | ||
| shutdownError = error; | ||
| logger.error(error, "Failed to close HTTP server during shutdown"); | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
close()
process.once("SIGTERM", closeFromSignal) and process.once("SIGINT", closeFromSignal) are registered unconditionally, but when close() is called directly (e.g., from test teardown or future callers), those listeners remain until a signal fires. The alreadyClosed guard makes subsequent calls harmless, but the dangling listeners will show up in process.listenerCount("SIGTERM") and can interfere in test environments that assert no leaked listeners.
Consider removing them inside close():
process.once("SIGTERM", closeFromSignal);
process.once("SIGINT", closeFromSignal);
// inside close():
process.removeListener("SIGTERM", closeFromSignal);
process.removeListener("SIGINT", closeFromSignal);|
|
||
| this._serverLabelSet = await db.getLabelSet(); | ||
| this.db = db; | ||
| } | ||
|
|
There was a problem hiding this comment.
attachDb sets _serverLabelSet before this.db, creating a brief window where the duplicate-attach guard can be bypassed
this._serverLabelSet = await db.getLabelSet(); // _serverLabelSet is now set
this.db = db; // this.db is still undefined hereIf attachDb is called a second time in the microtask gap between these two lines (e.g., a concurrent bootstrap retry), if (this.db !== undefined) would still be false and the guard would allow a second DB to be attached. Swapping the assignment order (this.db = db first, then this._serverLabelSet) would make the guard reliable—isReady() correctly requires both fields, so routing to the ready handlers before both are set isn't possible anyway.
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)