Fix ENSDb SDK builder for "concrete" ENSIndexer Schema#1832
Conversation
…tests Previously, we were using `public` value, which masked the issues with the "concrete" ENSIndexer Schema structure.
Our way for "cloning" the `abstractEnsIndexerSchema` object was incorrect, and the produced "concrete" schema object were incomplete. This was made obvious by integration testing suite errors, e.g. https://github.com/namehash/ensnode/actions/runs/23557404679/job/68587898403\#step:5:6533
The `ensNodeMetadata` schema has been fully migrated to ENSNode Schema
🦋 Changeset detectedLatest commit: a9a08f4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 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.
|
|
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:
📝 WalkthroughWalkthroughRemoved the Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
tk-o
changed the title
Greptile SummaryThis PR fixes the integration test suite by correcting two root causes: the wrong schema name ( The simplification in Key changes:
Confidence Score: 4/5Safe to merge for the current single-ENSIndexer-instance deployment; the mutation bug is acknowledged, tracked, and does not affect production behavior today. Integration and unit tests pass. The core defect (mutation of the abstract schema singleton) is a known, intentional trade-off documented with a TODO, a skipped test, and issue #1830 — not a surprise regression. The only real risk is that issue #1830 is resolved slowly and a future caller inadvertently builds two concurrent schemas, hitting the silent corruption path. The prototype-preservation tests are now vacuously true and no longer provide safety net coverage, but that is a test-quality concern rather than a blocking bug. packages/ensdb-sdk/src/lib/drizzle.ts — the mutation approach requires issue #1830 to be resolved before multi-instance support can be added safely. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant buildConcreteEnsIndexerSchema
participant abstractEnsIndexerSchema as abstractEnsIndexerSchema<br/>(ES module singleton)
participant DrizzleClient
Note over abstractEnsIndexerSchema: Tables/enums with no schema name set
Caller->>buildConcreteEnsIndexerSchema: buildIndividualEnsDbSchemas("ensindexer_0")
buildConcreteEnsIndexerSchema->>abstractEnsIndexerSchema: mutate table[Table.Symbol.Schema] = "ensindexer_0"
buildConcreteEnsIndexerSchema->>abstractEnsIndexerSchema: mutate enum.schema = "ensindexer_0"
buildConcreteEnsIndexerSchema-->>Caller: return abstractEnsIndexerSchema (same reference, aliased)
Caller->>DrizzleClient: buildEnsDbDrizzleClient(connStr, concreteSchema)
DrizzleClient-->>Caller: Drizzle client targeting "ensindexer_0" ✅
Note over Caller,abstractEnsIndexerSchema: ⚠️ If called again with a different name (issue #1830):
Caller->>buildConcreteEnsIndexerSchema: buildIndividualEnsDbSchemas("ensindexer_1")
buildConcreteEnsIndexerSchema->>abstractEnsIndexerSchema: overwrite table[Table.Symbol.Schema] = "ensindexer_1"
Note over DrizzleClient: First client's tables now silently point to "ensindexer_1" ❌
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adjusts ENSDb schema wiring so the “concrete” ENSIndexer Drizzle schema is derived in a way that aligns with integration expectations, and removes an obsolete ENSIndexer-abstract metadata schema export.
Changes:
- Update integration test orchestrator to use a non-
publicENSIndexer schema name (ensindexer_0). - Change
buildConcreteEnsIndexerSchemaimplementation to set schema names by mutating the imported abstract schema objects (and skip the test that asserts non-mutation). - Remove
ensnode-metadata.schemafromensindexer-abstractexports and delete the file.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/integration-test-env/src/orchestrator.ts | Uses ensindexer_0 as the schema name for the integration environment. |
| packages/ensdb-sdk/src/lib/drizzle.ts | Alters concrete schema building logic to mutate shared table/enum objects. |
| packages/ensdb-sdk/src/lib/drizzle.test.ts | Skips the test that previously ensured the abstract schema was not mutated. |
| packages/ensdb-sdk/src/ensindexer-abstract/index.ts | Removes export of the ENSNode metadata schema from the ENSIndexer abstract bundle. |
| packages/ensdb-sdk/src/ensindexer-abstract/ensnode-metadata.schema.ts | Deletes the legacy metadata schema definition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ensdb-sdk/src/lib/drizzle.ts`:
- Around line 39-59: The code currently aliases abstractEnsIndexerSchema to
concreteEnsIndexerSchema and mutates the shared module objects; instead, create
a fresh concrete schema object per call by cloning each entry from
abstractEnsIndexerSchema (copying table and enum objects rather than referencing
them) and then set the schema name on the clones (use Table.Symbol.Schema for
tables and .schema for enums) before returning the new ConcreteEnsIndexerSchema;
update the loop that uses isTable and isPgEnum to operate on the cloned objects
so callers can safely request multiple different ensIndexerSchemaName values
without mutating the module singleton.
🪄 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: 6db3da44-b5a7-43f5-a899-b7af6abd3158
📒 Files selected for processing (5)
packages/ensdb-sdk/src/ensindexer-abstract/ensnode-metadata.schema.tspackages/ensdb-sdk/src/ensindexer-abstract/index.tspackages/ensdb-sdk/src/lib/drizzle.test.tspackages/ensdb-sdk/src/lib/drizzle.tspackages/integration-test-env/src/orchestrator.ts
💤 Files with no reviewable changes (2)
- packages/ensdb-sdk/src/ensindexer-abstract/index.ts
- packages/ensdb-sdk/src/ensindexer-abstract/ensnode-metadata.schema.ts
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/integration-test-env/src/orchestrator.ts`:
- Around line 200-226: The polling loop swallows all errors from
ensDbClient.getIndexingStatusSnapshot(), losing the real failure and allowing
the finally block to overwrite it when closing the client; modify the logic in
the try/catch around getIndexingStatusSnapshot() to capture and store the last
non-retry error (e.g., in a variable `lastPollingError`) instead of always
ignoring it, continue retrying only for transient conditions, and after the
timeout throw the original `lastPollingError` (or include it as the cause)
rather than a generic timeout; in the finally block, ensure you log any errors
from `ensDbClient.ensDb.$client.end()` instead of rethrowing or replacing the
primary error, and remove the now-unused `util.inspect` import.
🪄 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: dab08587-5dd3-4e5e-8c1e-9d4e13f87a26
📒 Files selected for processing (1)
packages/integration-test-env/src/orchestrator.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/chatty-monkeys-run.md:
- Line 5: Update the changeset note to mention that the fix for
buildConcreteEnsIndexerSchema uses an interim mutation approach rather than
cloning; explicitly state this is a known limitation and reference the planned
refactor tracked by issue `#1830` and the in-code TODO so readers know mutation is
temporary and will be addressed later.
🪄 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: ba7fa6b9-d578-4f02-8127-a88f33f8301c
📒 Files selected for processing (1)
.changeset/chatty-monkeys-run.md
Ensure ENSDb connection is closed before testing suite completes
…-concrete-ensindexer-schema
…placing the incomplete cloning approach with working mutation approach.
tk-o
force-pushed
the
fix/ensdb-builder-for-concrete-ensindexer-schema
branch
from
713a230 to
b45dea9
Compare
…-concrete-ensindexer-schema
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/integration-test-env/src/orchestrator.ts (1)
198-225:⚠️ Potential issue | 🟠 MajorPreserve polling root cause and prevent cleanup from masking failures.
Line 214 currently suppresses all DB polling errors, and Line 224 can replace the primary failure if
end()throws. This makes timeout/debugging noisy and can hide real regressions.Suggested fix
const start = Date.now(); log("Polling indexing status..."); + let lastPollError: unknown; try { while (Date.now() - start < timeoutMs) { checkAborted(); try { const snapshot = await ensDbClient.getIndexingStatusSnapshot(); + lastPollError = undefined; if (snapshot !== undefined) { const omnichainStatus = snapshot.omnichainSnapshot.omnichainStatus; log(`Omnichain status: ${omnichainStatus}`); if ( omnichainStatus === OmnichainIndexingStatusIds.Following || omnichainStatus === OmnichainIndexingStatusIds.Completed ) { log("Indexing reached target status"); return; } } - } catch { + } catch (error) { // indexer may not be ready yet + lastPollError = error; } await new Promise((r) => setTimeout(r, 3000)); } - throw new Error(`Indexing did not complete within ${timeoutMs / 1000}s`); + throw new Error( + `Indexing did not complete within ${timeoutMs / 1000}s`, + { cause: lastPollError }, + ); } finally { - console.log("Closing ENSDb client..."); - // `@ts-expect-error` - DrizzleClient.$client is not typed to have an `end` method, - // but in practice it does (e.g. pg's Client does). - await ensDbClient.ensDb.$client.end(); - console.log("ENSDb client closed"); + log("Closing ENSDb client..."); + try { + // `@ts-expect-error` - DrizzleClient.$client is not typed to have an `end` method, + // but in practice it does (e.g. pg's Client does). + await ensDbClient.ensDb.$client.end(); + log("ENSDb client closed"); + } catch (error) { + logError( + `Failed to close ENSDb client: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integration-test-env/src/orchestrator.ts` around lines 198 - 225, The polling code currently swallows all getIndexingStatusSnapshot() errors and the finally cleanup can mask the primary failure if ensDbClient.ensDb.$client.end() throws; fix by capturing the first (or last) polling error into a variable (e.g. lastPollingError) inside the inner catch around ensDbClient.getIndexingStatusSnapshot(), continue polling as before, and when timing out throw an Error that includes lastPollingError as the cause or appended message; also wrap the cleanup call await ensDbClient.ensDb.$client.end() in its own try/catch and if it throws, log that cleanup error but rethrow the original timeout/polling error (or the end error only if there was no prior primary error) so cleanup cannot replace the root cause.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/integration-test-env/src/orchestrator.ts`:
- Around line 198-225: The polling code currently swallows all
getIndexingStatusSnapshot() errors and the finally cleanup can mask the primary
failure if ensDbClient.ensDb.$client.end() throws; fix by capturing the first
(or last) polling error into a variable (e.g. lastPollingError) inside the inner
catch around ensDbClient.getIndexingStatusSnapshot(), continue polling as
before, and when timing out throw an Error that includes lastPollingError as the
cause or appended message; also wrap the cleanup call await
ensDbClient.ensDb.$client.end() in its own try/catch and if it throws, log that
cleanup error but rethrow the original timeout/polling error (or the end error
only if there was no prior primary error) so cleanup cannot replace the root
cause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3126e046-216c-436b-b1ec-3ff306cd895d
📒 Files selected for processing (2)
.changeset/chatty-monkeys-run.mdpackages/integration-test-env/src/orchestrator.ts
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Thanks for this update! Shared a few suggestions. Please feel welcome to merge when ready 👍
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // @ts-expect-error - DrizzleClient.$client is not typed to have an `end` method, | ||
| // but in practice it does (e.g. pg's Client does). | ||
| await ensDbClient.ensDb.$client.end(); | ||
| console.log("ENSDb client closed"); |
There was a problem hiding this comment.
The cleanup logic reaches into Drizzle internals (ensDbClient.ensDb.$client.end()) and suppresses typing with @ts-expect-error. This is brittle (a Drizzle type update that adds end will break the build because @ts-expect-error expects an error, and a driver change could make $client.end absent/different). Prefer a small explicit shutdown helper (e.g. a close() method on EnsDbReader) or a runtime/typed guard that checks for an end() function before calling it, without @ts-expect-error.
| // @ts-expect-error - DrizzleClient.$client is not typed to have an `end` method, | |
| // but in practice it does (e.g. pg's Client does). | |
| await ensDbClient.ensDb.$client.end(); | |
| console.log("ENSDb client closed"); | |
| const anyClient = ensDbClient as any; | |
| try { | |
| const rawEnsDb = anyClient?.ensDb; | |
| const rawUnderlyingClient = rawEnsDb?.$client; | |
| const endFn = rawUnderlyingClient?.end; | |
| if (typeof endFn === "function") { | |
| await endFn.call(rawUnderlyingClient); | |
| console.log("ENSDb client closed"); | |
| } else { | |
| console.log("ENSDb client has no 'end' method; skipping close"); | |
| } | |
| } catch (closeErr) { | |
| console.log("Failed to close ENSDb client cleanly:", closeErr); | |
| } |
| if ( | ||
| appliedNameForConcreteEnsIndexerSchema !== undefined && | ||
| appliedNameForConcreteEnsIndexerSchema !== ensIndexerSchemaName | ||
| ) { | ||
| throw new Error( | ||
| `buildConcreteEnsIndexerSchema was already called with schema "${appliedNameForConcreteEnsIndexerSchema}". ` + | ||
| `Calling it again with "${ensIndexerSchemaName}" would corrupt the previously built schema.`, | ||
| ); |
There was a problem hiding this comment.
This adds module-level state (appliedNameForConcreteEnsIndexerSchema) so schema-building becomes a process-wide singleton: after the first call, building for a different ensIndexerSchemaName throws. Since callers reach this via exported buildIndividualEnsDbSchemas/EnsDbReader, this constraint should be documented on the public API (and/or in the changeset) to avoid surprising runtime failures in multi-tenant or multi-db scenarios.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ensdb-sdk/src/lib/drizzle.test.ts (1)
137-167: 🧹 Nitpick | 🔵 TrivialTest descriptions reference "cloned tables" but implementation no longer clones.
This describe block tests "prototype and Symbol preservation" on "cloned tables," but the current implementation mutates the abstract schema in place—no cloning occurs. These tests now pass trivially because
concreteTableandabstractTableare the same object reference, not because properties are preserved through cloning.Consider either:
- Renaming the describe block and tests to reflect mutation-based behavior (e.g., "concrete tables — mutation does not break Drizzle compatibility")
- Removing these tests since they no longer validate the intended behavior
✏️ Suggested rename
-describe("concrete tables — prototype and Symbol preservation", () => { +describe("concrete tables — Drizzle compatibility after mutation", () => { const IsDrizzleTable = Symbol.for("drizzle:IsDrizzleTable"); const Columns = Symbol.for("drizzle:Columns"); const TableName = Symbol.for("drizzle:Name"); - it("preserves the Table prototype on cloned tables", () => { + it("maintains the Table prototype after schema name mutation", () => { // ... }); - it("preserves Symbol-keyed properties (IsDrizzleTable, Columns, TableName) on cloned tables", () => { + it("maintains Symbol-keyed properties (IsDrizzleTable, Columns, TableName) after mutation", () => { // ... }); - it("isTable() returns true for cloned concrete tables", () => { + it("isTable() returns true for concrete tables after mutation", () => { // ... }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ensdb-sdk/src/lib/drizzle.test.ts` around lines 137 - 167, The tests in the describe block assume tables are cloned but the implementation mutates the abstract schema in place, so concreteEnsIndexerSchema.v1Domain and abstractEnsIndexerSchema.v1Domain are the same object and tests pass trivially; update the tests to reflect mutation-based behavior by renaming the suite and specs (e.g., "concrete tables — mutation does not break Drizzle compatibility") and adjust assertions to verify that mutation preserves Drizzle compatibility (use buildIndividualEnsDbSchemas to produce schemas and assert isTable(concreteEnsIndexerSchema.v1Domain) and that Object.getPrototypeOf and the Symbol.for keys (Symbol.for("drizzle:IsDrizzleTable"), Symbol.for("drizzle:Columns"), Symbol.for("drizzle:Name")) exist on the mutated object), or alternatively remove these cloning-specific tests if you want to assert cloning behavior elsewhere; make changes around concreteEnsIndexerSchema, abstractEnsIndexerSchema, buildIndividualEnsDbSchemas, and isTable accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/ensdb-sdk/src/lib/drizzle.test.ts`:
- Around line 137-167: The tests in the describe block assume tables are cloned
but the implementation mutates the abstract schema in place, so
concreteEnsIndexerSchema.v1Domain and abstractEnsIndexerSchema.v1Domain are the
same object and tests pass trivially; update the tests to reflect mutation-based
behavior by renaming the suite and specs (e.g., "concrete tables — mutation does
not break Drizzle compatibility") and adjust assertions to verify that mutation
preserves Drizzle compatibility (use buildIndividualEnsDbSchemas to produce
schemas and assert isTable(concreteEnsIndexerSchema.v1Domain) and that
Object.getPrototypeOf and the Symbol.for keys
(Symbol.for("drizzle:IsDrizzleTable"), Symbol.for("drizzle:Columns"),
Symbol.for("drizzle:Name")) exist on the mutated object), or alternatively
remove these cloning-specific tests if you want to assert cloning behavior
elsewhere; make changes around concreteEnsIndexerSchema,
abstractEnsIndexerSchema, buildIndividualEnsDbSchemas, and isTable accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68a43991-b2da-4151-92e0-f9cca9ecc130
📒 Files selected for processing (3)
.changeset/chatty-monkeys-run.mdpackages/ensdb-sdk/src/lib/drizzle.test.tspackages/ensdb-sdk/src/lib/drizzle.ts
| console.log("Closing ENSDb client..."); | ||
| // @ts-expect-error - DrizzleClient.$client is not typed to have an `end` method, | ||
| // but in practice it does (e.g. pg's Client does). | ||
| await ensDbClient.ensDb.$client.end(); | ||
| console.log("ENSDb client closed"); |
There was a problem hiding this comment.
| console.log("Closing ENSDb client..."); | |
| // @ts-expect-error - DrizzleClient.$client is not typed to have an `end` method, | |
| // but in practice it does (e.g. pg's Client does). | |
| await ensDbClient.ensDb.$client.end(); | |
| console.log("ENSDb client closed"); | |
| log("Closing ENSDb client..."); | |
| // @ts-expect-error - DrizzleClient.$client is not typed to have an `end` method, | |
| // but in practice it does (e.g. pg's Client does). | |
| await ensDbClient.ensDb.$client.end(); | |
| log("ENSDb client closed"); |
Finally block uses console.log() instead of the standard log() function, causing inconsistent logging formatting
| } | ||
| } | ||
| } catch { | ||
| // indexer may not be ready yet |
There was a problem hiding this comment.
Bare catch block and finally block error masking in the indexing status polling loop
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
Why
Testing
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)