Conversation
🦋 Changeset detectedLatest commit: 39b700a The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ENSDb persistence and a background writer: Drizzle factory, ENSDb client and singleton, writer worker + starter singleton, indexer/indexing-status singletons, tests/mocks, package dependency additions, and updates the API handler to use singletons and start the writer. Changes
Sequence DiagramsequenceDiagram
participant API as "ensnode-api"
participant Worker as "EnsDbWriterWorker (singleton)"
participant Indexer as "IndexingStatusBuilder / EnsIndexerClient"
participant Client as "EnsDbClient"
participant DB as "PostgreSQL"
API->>Worker: startEnsDbWriterWorker()
rect rgba(100,150,255,0.5)
Note over Worker,Indexer: initial validation & config fetch
Worker->>Indexer: getValidatedEnsIndexerPublicConfig()
Indexer->>Client: request stored config
Client->>DB: SELECT metadata by key
DB-->>Client: stored config or none
Client-->>Worker: config (or undefined)
end
rect rgba(100,150,255,0.5)
Note over Worker: initial upserts
Worker->>Client: upsertEnsDbVersion()
Client->>DB: upsert version metadata
Worker->>Client: upsertEnsIndexerPublicConfig()
Client->>DB: upsert config metadata
end
rect rgba(150,200,100,0.5)
Note over Worker: streaming snapshots
loop every interval
Worker->>Indexer: buildIndexingStatusSnapshot()
Indexer-->>Worker: snapshot
Worker->>Client: upsertIndexingStatusSnapshot()
Client->>DB: upsert serialized snapshot
DB-->>Client: OK
end
end
alt Worker stops normally
Worker-->>API: stopped
else Unrecoverable error / retries exhausted
Worker-->>API: error -> process abort
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Introduces an ENSDb “writer worker” in ENSIndexer to persist ENSIndexer metadata into ENSDb, and updates the ensnode-sdk ENSDb client interface naming to be explicitly ENSIndexer-scoped.
Changes:
- Add
EnsDbClient(Drizzle-based) for reading/writing ENSNode metadata records in ENSDb, plus unit tests/mocks. - Add
EnsDbWriterWorkerthat upserts ENSDb version + ENSIndexer public config once, then periodically upserts indexing status snapshots (with retries), plus unit tests. - Wire the worker + new singletons into ENSIndexer’s Hono API handler; add required deps (
drizzle-orm,@ponder/client,@types/pg).
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds/threads @types/pg through relevant dependency snapshots. |
| packages/ensnode-sdk/src/ensdb/client.ts | Renames ENSDb client query/mutation methods to ENSIndexer-specific names. |
| apps/ensindexer/src/lib/indexing-status-builder/singleton.ts | Introduces singleton instance for IndexingStatusBuilder. |
| apps/ensindexer/src/lib/ensindexer-client/singleton.ts | Introduces singleton EnsIndexerClient configured via app config. |
| apps/ensindexer/src/lib/ensdb-writer-worker/singleton.ts | Creates a singleton EnsDbWriterWorker wired with required dependencies. |
| apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts | Implements the worker loop, validation, and retry behavior. |
| apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts | Adds unit tests for worker behavior (currently has signature mismatches). |
| apps/ensindexer/src/lib/ensdb-client/singleton.ts | Adds singleton EnsDbClient for ENSIndexer. |
| apps/ensindexer/src/lib/ensdb-client/ensdb-client.ts | Implements ENSDb client queries/mutations for ENSNode metadata. |
| apps/ensindexer/src/lib/ensdb-client/ensdb-client.test.ts | Adds unit tests for ENSDb client behavior. |
| apps/ensindexer/src/lib/ensdb-client/ensdb-client.mock.ts | Adds fixtures for ENSDb client tests (public config, serialized snapshot). |
| apps/ensindexer/src/lib/ensdb-client/drizzle.ts | Adds makeDrizzle helper (adapted from ENSApi). |
| apps/ensindexer/ponder/src/api/handlers/ensnode-api.ts | Starts the writer worker on module load and swaps in singleton builder usage. |
| apps/ensindexer/package.json | Adds needed runtime/dev deps for Drizzle and pg typings. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts
Outdated
Show resolved
Hide resolved
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
![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/ensindexer/package.json`:
- Line 49: Update the dependency entry for "@types/pg" in package.json to use
the workspace catalog reference (matching how "@types/node" is declared) instead
of the hardcoded "8.16.0"; if "@types/pg" is not yet listed in the pnpm
workspace catalog, add it there with the desired version and then change the
package.json entry to "catalog:`@types/pg`" (or the exact catalog key used in your
monorepo) so the monorepo versioning pattern remains consistent.
In `@apps/ensindexer/ponder/src/api/handlers/ensnode-api.ts`:
- Around line 24-32: Replace the current un-awaited promise handling for
ensDbWriterWorker.runWithRetries so it doesn't create an unhandled rejection:
either move the call into a ponder.on("setup", ...) handler and await
ensDbWriterWorker.runWithRetries({ maxRetries: 3 }) so failures can be thrown to
fail setup, or keep it fire-and-forget but mark failure deterministically by
using void ensDbWriterWorker.runWithRetries(...).catch(...) and inside the catch
call ensDbWriterWorker.stop(), log the error and set process.exitCode = 1;
update the code around ensDbWriterWorker.runWithRetries and related stop/logging
accordingly.
In `@apps/ensindexer/src/lib/ensdb-client/ensdb-client.ts`:
- Around line 131-136: Remove the redundant `@returns` JSDoc tag from the JSDoc
block that begins "Get ENSNode metadata record" in ensdb-client.ts (the doc for
the ENS node metadata retrieval method); keep the summary and other tags such as
`@throws` but delete the `@returns` line so the comment no longer restates the
method summary.
In `@apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts`:
- Line 242: The call to worker.runWithRetries currently passes a numeric
argument but the method signature expects an options object; change the
invocation of runWithRetries on the worker instance to pass an object like {
maxRetries: 2 } (mirror the same fix you applied at the earlier call near line
210) so the call matches the runWithRetries({ maxRetries: number }) signature.
- Line 210: The test calls worker.runWithRetries(2) but runWithRetries expects
an object parameter; change the call to pass an object like
worker.runWithRetries({ maxRetries: 2 }) (update the runPromise assignment and
any other test invocations of runWithRetries accordingly) so the signature for
runWithRetries and the worker variable usage match the expected { maxRetries:
number } shape.
In `@apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts`:
- Around line 158-160: Remove the redundant JSDoc `@returns` lines from the method
docblocks that describe returning the in-memory config object (the block that
currently says "In-memory config object, if the validation is successful or if
there is no stored config.") as well as the similar docblock around lines
204-207; keep the summary and `@throws` entries but delete the repetitive `@returns`
tags so the method documentation follows repo style.
- Around line 113-136: The retry loop in runWithRetries is unbounded because it
loops while (!this.isStopped) and only checks maxRetries after sleeping, which
can cause runaway timers; change the loop to explicitly bound retries (e.g.,
while (attempt < maxRetries && !this.isStopped)) or otherwise include an attempt
check in the loop condition so the delay on Line 130 cannot execute beyond
maxRetries; keep the existing behavior of throwing an Error with cause when
attempts are exhausted (the throw that references attempt and error should
remain inside the failure branch) and continue using
INDEXING_STATUS_RECORD_UPDATE_INTERVAL for the sleep duration.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
apps/ensindexer/package.jsonapps/ensindexer/ponder/src/api/handlers/ensnode-api.tsapps/ensindexer/src/lib/ensdb-client/drizzle.tsapps/ensindexer/src/lib/ensdb-client/ensdb-client.mock.tsapps/ensindexer/src/lib/ensdb-client/ensdb-client.test.tsapps/ensindexer/src/lib/ensdb-client/ensdb-client.tsapps/ensindexer/src/lib/ensdb-client/singleton.tsapps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.tsapps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.tsapps/ensindexer/src/lib/ensdb-writer-worker/singleton.tsapps/ensindexer/src/lib/ensindexer-client/singleton.tsapps/ensindexer/src/lib/indexing-status-builder/singleton.tspackages/ensnode-sdk/src/ensdb/client.ts
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts
Outdated
Show resolved
Hide resolved
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts
Outdated
Show resolved
Hide resolved
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts
Outdated
Show resolved
Hide resolved
Greptile SummaryThis PR introduces Key findings:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant API as api/index.ts
participant EP as ensdb-writer-worker-entrypoint.ts
participant SNG as singleton.ts (worker)
participant WRK as EnsDbWriterWorker
participant IDX as EnsIndexerClient
participant DB as EnsDbClient (pg.Pool)
participant ISB as IndexingStatusBuilder
API->>EP: import (side-effect)
EP->>SNG: startEnsDbWriterWorker()
SNG->>IDX: pRetry → config() [up to 4 attempts]
IDX-->>SNG: EnsIndexerPublicConfig (availability check, result discarded)
SNG->>WRK: new EnsDbWriterWorker(ensDbClient, ensIndexerClient, indexingStatusBuilder)
SNG->>WRK: run()
WRK->>DB: getEnsIndexerPublicConfig()
WRK->>IDX: config()
DB-->>WRK: storedConfig (or undefined)
IDX-->>WRK: inMemoryConfig
alt storedConfig exists
WRK->>WRK: validateEnsIndexerPublicConfigCompatibility(stored, inMemory)
Note over WRK: throws → catch in SNG → process.exitCode=1
end
WRK->>DB: upsertEnsDbVersion(versionInfo.ensDb)
WRK->>DB: upsertEnsIndexerPublicConfig(inMemoryConfig)
WRK->>WRK: setInterval(upsertIndexingStatusSnapshot, 1s)
loop Every 1 second
WRK->>ISB: getOmnichainIndexingStatusSnapshot()
ISB-->>WRK: omnichainSnapshot
WRK->>WRK: validate (not Unstarted)
WRK->>WRK: buildCrossChainIndexingStatusSnapshotOmnichain(snapshot, time)
WRK->>DB: upsertIndexingStatusSnapshot(crossChainSnapshot)
end
Note over SNG: process.on(SIGINT/SIGTERM) → ensDbWriterWorker.stop()
SNG-->>WRK: stop() → clearInterval (pool NOT closed)
Last reviewed commit: 39b700a |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts
Outdated
Show resolved
Hide resolved
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts
Outdated
Show resolved
Hide resolved
tk-o
force-pushed
the
feat/ensdb-writer-worker-for-ensindexer
branch
from
c6ce77f to
328e5c1
Compare
tk-o
force-pushed
the
feat/ensdb-writer-worker-for-ensindexer
branch
from
328e5c1 to
c39e3ea
Compare
tk-o
force-pushed
the
feat/ensdb-writer-worker-for-ensindexer
branch
from
c39e3ea to
eed564c
Compare
ENSIndexer modules can import the singleton instance from `@/lib/ensdb-client`.
tk-o
force-pushed
the
feat/ensdb-writer-worker-for-ensindexer
branch
from
eed564c to
8208a39
Compare
Implement `AbortSignal`, and ensure only one worker instance can be started.
…enable storing ENSNode metadata in ENSDb.
tk-o
force-pushed
the
feat/ensdb-writer-worker-for-ensindexer
branch
from
65e36f7 to
e8e5d01
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts
Outdated
Show resolved
Hide resolved
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts
Outdated
Show resolved
Hide resolved
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Hey great to see this. Reviewed and shared feedback 👍
.changeset/afraid-ducks-strive.md
Outdated
| "ensindexer": minor | ||
| --- | ||
|
|
||
| Introduced `EnsDbClient` and `EnsDbWriterWorker` to enable storing ENSNode metadata in ENSDb. |
There was a problem hiding this comment.
| Introduced `EnsDbClient` and `EnsDbWriterWorker` to enable storing ENSNode metadata in ENSDb. | |
| Introduced `EnsDbClient` and `EnsDbWriterWorker` to enable storing metadata in ENSDb. |
Goal: Suggest we not call it ENSNode metadata and instead use more specific terminology for it. As I understand the metadata we store there is not for an entire ENSNode instance but just for the services like ENSIndexer and ENSRainbow that live "beneath" ENSDb.
| const app = new Hono(); | ||
| const indexingStatusBuilder = new IndexingStatusBuilder(localPonderClient); | ||
|
|
||
| startEnsDbWriterWorker(); |
There was a problem hiding this comment.
Are you sure this is the ideal place for this idea? I note this file is an API handler.
There was a problem hiding this comment.
Yeah, we should create a new file in ponder/src and to be executed at runtime 👍
There was a problem hiding this comment.
Turns out, that our worker file must live inside ponder/src/api dir, so Ponder app builder does not complain with build error.
| @@ -0,0 +1,24 @@ | |||
| // This file was copied 1-to-1 from ENSApi. | |||
There was a problem hiding this comment.
@tk-o Can you please check this comment from Copilot?
| @@ -0,0 +1,24 @@ | |||
| // This file was copied 1-to-1 from ENSApi. | |||
There was a problem hiding this comment.
Why do we need to copy it? Why can't we move it into a package that can be reused across these apps?
I understand here we might be doing something special with Ponder -- but then we should be documenting here WHY we are doing what we are doing.
There was a problem hiding this comment.
Sure, will add the comment saying that we don't currently have a backend-oriented package through which we could share backend-only modules.
| * | ||
| * This client exists to provide an abstraction layer for interacting with ENSDb. | ||
| * It enables ENSIndexer and ENSApi to decouple from each other, and use | ||
| * the ENSDb Client as the integration point between the two. |
There was a problem hiding this comment.
| * the ENSDb Client as the integration point between the two. | |
| * ENSDb as the integration point between the two (via ENSDb Client). |
| /** | ||
| * ENSDb Writer Worker | ||
| * | ||
| * A worker responsible for writing ENSIndexer-related data into ENSDb, including: |
There was a problem hiding this comment.
| * A worker responsible for writing ENSIndexer-related data into ENSDb, including: | |
| * A worker responsible for writing ENSIndexer-related metadata into ENSDb, including: |
| attempt += 1; | ||
|
|
||
| try { | ||
| await this.run(); |
There was a problem hiding this comment.
I'm not so clear what happens here if an error happens. It seems that attempt is never reset back to 0 if there's a sequence of error, error, success?
There was a problem hiding this comment.
I'll improve the retry logic with p-retry, no point in re-implementing such a common pattern 👍
| try { | ||
| validateEnsIndexerPublicConfigCompatibility(storedConfig, inMemoryConfig); | ||
| } catch (error) { | ||
| const errorMessage = `In-memory ENSIndexer Public Config object is not compatible with its counterpart stored in ENSDb.`; |
There was a problem hiding this comment.
It will be very helpful for operators if this gives visibility on what caused the incompatibility.
| * Validation criteria are defined in the function body. | ||
| * @returns void when the worker is stopped. | ||
| */ | ||
| private async *getIndexingStatusSnapshotStream(): AsyncGenerator<CrossChainIndexingStatusSnapshot> { |
There was a problem hiding this comment.
I was assuming we would use a similar strategy for infinite async loops both here and in SWRCache. Is there a special reason why we used different strategies in each of these?
When we have a similar need, prefer we reuse our design patterns unless there's a special reason not to. Otherwise if we have too many ways of doing what is fundamentally the same thing it makes it harder for people to work on our code.
There was a problem hiding this comment.
The idea I had was to express the recurring task clearly. That's why I used a stream + for-await approach.
Sure, we can apply interval running the task as well, just like it's done in SWRCache 👍
| * Interval in seconds between two consecutive attempts to upsert | ||
| * the Indexing Status Snapshot record into ENSDb. | ||
| */ | ||
| const INDEXING_STATUS_RECORD_UPDATE_INTERVAL: Duration = 5; |
There was a problem hiding this comment.
Maybe reduce to 1 second?
Applies background tasks approach known from `SWRCache` implementation.
Initially, I wanted to implement some helpers myself, but then, after seeing AI PR feedback, I figured I was rebuilding `p-retry` in a way... Here is the (now closed) PR: #1709
No point in running the worker if its dependencies are not available to be loaded.
|
@greptile review |
| public async run(): Promise<void> { | ||
| // Fetch data required for task 1 and task 2. | ||
| const inMemoryConfig = await this.getValidatedEnsIndexerPublicConfig(); | ||
|
|
||
| // Task 1: upsert ENSDb version into ENSDb. | ||
| console.log(`[EnsDbWriterWorker]: Upserting ENSDb version into ENSDb...`); | ||
| await this.ensDbClient.upsertEnsDbVersion(inMemoryConfig.versionInfo.ensDb); | ||
| console.log( | ||
| `[EnsDbWriterWorker]: ENSDb version upserted successfully: ${inMemoryConfig.versionInfo.ensDb}`, | ||
| ); | ||
|
|
||
| // Task 2: upsert of EnsIndexerPublicConfig into ENSDb. | ||
| console.log(`[EnsDbWriterWorker]: Upserting ENSIndexer Public Config into ENSDb...`); | ||
| await this.ensDbClient.upsertEnsIndexerPublicConfig(inMemoryConfig); | ||
| console.log(`[EnsDbWriterWorker]: ENSIndexer Public Config upserted successfully`); | ||
|
|
||
| // Task 3: recurring upsert of Indexing Status Snapshot into ENSDb. | ||
| this.indexingStatusInterval = setInterval( | ||
| () => this.upsertIndexingStatusSnapshot(), | ||
| secondsToMilliseconds(INDEXING_STATUS_RECORD_UPDATE_INTERVAL), | ||
| ); | ||
| } |
There was a problem hiding this comment.
run() is not guarded against multiple calls
run() unconditionally calls setInterval every time it is invoked. A second call before stop() would start a second interval; the reference to the first interval is overwritten by this.indexingStatusInterval = setInterval(...), making it impossible to clear the first one. The singleton guard in singleton.ts prevents this in production, but the class itself has no self-protection — a test or any future caller can trigger the leak.
Consider adding an early guard:
| public async run(): Promise<void> { | |
| // Fetch data required for task 1 and task 2. | |
| const inMemoryConfig = await this.getValidatedEnsIndexerPublicConfig(); | |
| // Task 1: upsert ENSDb version into ENSDb. | |
| console.log(`[EnsDbWriterWorker]: Upserting ENSDb version into ENSDb...`); | |
| await this.ensDbClient.upsertEnsDbVersion(inMemoryConfig.versionInfo.ensDb); | |
| console.log( | |
| `[EnsDbWriterWorker]: ENSDb version upserted successfully: ${inMemoryConfig.versionInfo.ensDb}`, | |
| ); | |
| // Task 2: upsert of EnsIndexerPublicConfig into ENSDb. | |
| console.log(`[EnsDbWriterWorker]: Upserting ENSIndexer Public Config into ENSDb...`); | |
| await this.ensDbClient.upsertEnsIndexerPublicConfig(inMemoryConfig); | |
| console.log(`[EnsDbWriterWorker]: ENSIndexer Public Config upserted successfully`); | |
| // Task 3: recurring upsert of Indexing Status Snapshot into ENSDb. | |
| this.indexingStatusInterval = setInterval( | |
| () => this.upsertIndexingStatusSnapshot(), | |
| secondsToMilliseconds(INDEXING_STATUS_RECORD_UPDATE_INTERVAL), | |
| ); | |
| } | |
| public async run(): Promise<void> { | |
| if (this.indexingStatusInterval !== null) { | |
| throw new Error("EnsDbWriterWorker is already running. Call stop() before run()."); | |
| } | |
| // Fetch data required for task 1 and task 2. | |
| const inMemoryConfig = await this.getValidatedEnsIndexerPublicConfig(); |
| await pRetry(() => ensIndexerClient.config(), { | ||
| retries: 3, | ||
| onFailedAttempt: ({ error, attemptNumber, retriesLeft }) => { | ||
| console.warn( | ||
| `ENSIndexer Config fetch attempt ${attemptNumber} failed (${error.message}). ${retriesLeft} retries left.`, | ||
| ); | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Redundant config fetch — result is discarded
The pRetry block fetches ensIndexerClient.config() solely to verify availability, but the result is never used. ensDbWriterWorker.run() (line 44) then calls this.ensIndexerClient.config() again internally via getValidatedEnsIndexerPublicConfig(), causing two round-trips for the same data on startup.
Consider either:
- Returning the config from
pRetryand passing it into a modifiedrun(config)signature, or - Moving the retry logic inside
getValidatedEnsIndexerPublicConfig()so retries apply to both the availability check and the actual read in one call.
| await ensDbWriterWorker.run(); | ||
| } catch (error) { | ||
| // Abort the worker on error to trigger resources cleanup | ||
| ensDbWriterWorker.stop(); | ||
|
|
||
| console.error("EnsDbWriterWorker encountered an error:", error); | ||
|
|
||
| // Re-throw the error to ensure the application shuts down with a non-zero exit code. | ||
| process.exitCode = 1; | ||
| throw error; | ||
| } | ||
|
|
||
| // Handle graceful shutdown on process termination signals | ||
| process.on("SIGINT", () => ensDbWriterWorker.stop()); | ||
| process.on("SIGTERM", () => ensDbWriterWorker.stop()); |
There was a problem hiding this comment.
DB connection pool is never closed on shutdown
EnsDbClient creates a pg.Pool internally through drizzle(databaseUrl, ...). The stop() method only clears the setInterval but does not close the underlying pool. The SIGINT/SIGTERM handlers registered on lines 57–58 call stop() but leave the pool open.
An open pg.Pool holds a timer internally that keeps the Node.js event loop alive, which can prevent a clean graceful exit if the host process relies on the event loop draining naturally. If Ponder always calls process.exit() explicitly this is moot, but adding an explicit end() call would be safer and more explicit.
Consider exposing a close() method on EnsDbClient that calls db.$client.end() and wiring it into EnsDbWriterWorker.stop():
// In EnsDbClient
public async close(): Promise<void> {
await this.db.$client.end();
}
// In EnsDbWriterWorker.stop()
public stop(): void {
if (this.indexingStatusInterval) {
clearInterval(this.indexingStatusInterval);
this.indexingStatusInterval = null;
}
// optionally await this.ensDbClient.close()
}| * | ||
| * @returns selected record in ENSDb. |
There was a problem hiding this comment.
Incorrect JSDoc @throws description
The method returns undefined when zero records are found (lines 159–161), so it does not throw in the "not found" case. It only throws when result.length > 1. The current JSDoc is misleading:
@throws when exactly one matching metadata record was not found
should read something like:
@throws when more than one matching metadata record is found (should be impossible given the PK constraint on 'key')
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.indexingStatusInterval = setInterval( | ||
| () => this.upsertIndexingStatusSnapshot(), | ||
| secondsToMilliseconds(INDEXING_STATUS_RECORD_UPDATE_INTERVAL), | ||
| ); |
There was a problem hiding this comment.
setInterval(() => this.upsertIndexingStatusSnapshot(), ...) schedules an async task every second without awaiting completion. If an upsert takes longer than the interval (slow DB / network hiccup), multiple upsertIndexingStatusSnapshot() calls can overlap and create concurrent writes/log spam. Consider switching to a self-scheduling setTimeout loop that awaits each iteration (or add a simple in-flight/lock guard) so only one snapshot write runs at a time.
|
|
||
| import { startEnsDbWriterWorker } from "@/lib/ensdb-writer-worker/singleton"; | ||
|
|
||
| startEnsDbWriterWorker(); |
There was a problem hiding this comment.
This entrypoint invokes the async startEnsDbWriterWorker() without awaiting or attaching a .catch(...). If startup fails (e.g., incompatible config), the rejection becomes unhandled and shutdown behavior depends on Node's unhandled-rejection mode. Please explicitly handle the returned promise (e.g., top-level await or void startEnsDbWriterWorker().catch(...) that logs and terminates) so failures reliably stop the process.
| startEnsDbWriterWorker(); | |
| void startEnsDbWriterWorker().catch((error) => { | |
| console.error("Failed to start ENSDb Writer Worker:", error); | |
| process.exit(1); | |
| }); |
| * The worker will run indefinitely until its internal AbortSignal is triggered, | ||
| * for example due to a process termination signal or an internal error, at |
There was a problem hiding this comment.
The JSDoc claims the worker runs until its internal AbortSignal is triggered, but the current EnsDbWriterWorker implementation only uses setInterval + stop() and doesn't use an AbortController/signal. Please either update this documentation to match the actual lifecycle control or implement the abort-signal based shutdown described here (and in the PR description).
| * The worker will run indefinitely until its internal AbortSignal is triggered, | |
| * for example due to a process termination signal or an internal error, at | |
| * The worker will run indefinitely until it is stopped via {@link EnsDbWriterWorker.stop}, | |
| * for example in response to a process termination signal or an internal error, at |
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
EnsDbClientclass for ENSIndexer (includes queries and mutations).EnsDbWriterWorkerto handle tasks of writing ENSNode metadata into ENSDbEnsDbWriterWorkerusesAbortControllerinternally to manage worker lifecycle (running, stopped).startEnsDbWriterWorkerfunction creates a singleton instance forEnsDbWriterWorkerand sets up error handling.Why
Testing
ENSIndexer logs demo
Notes for Reviewer (Optional)
EnsDbClientQueryinterface) from ENSDb that will be available via a new package so our customers could use it in their backend workloads. That update is out of scope for this PR, but when it's live, theapps/ensindexer/src/lib/ensdb-client/ensdb-client.tsimplementation could simply extend the "read-only" client and only add the mutation methods (seeEnsDbClientMutationinterface).Pre-Review Checklist (Blocking)
Resolves #1252