Repository navigation
feat(dbmanager): implement enterprise dbmanager and integrate with co… - #77
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughNew declarative dbmanager package and Nest module centralize schema provisioning (SchemaPlan/DatabaseManager). The API removes inline DDL, imports DbManagerModule, and services/tests now call dbManager.applyPlan for lazy namespace and gateway table provisioning. Changes
Sequence Diagram(s)sequenceDiagram
participant Trigger as TriggerExecutorService
participant Conn as ConnectorsService
participant DbMgr as DbManager<br/>(SqlDatabaseManager)
participant Drizzle as DrizzleDb
Conn->>Drizzle: begin tx, create workspace schema name (return schemaName)
Conn-->>Conn: tx returns schemaName
Conn->>DbMgr: applyPlan(schemaName, SchemaPlan.NAMESPACE_ONLY)
DbMgr->>Drizzle: validate schemaName & CREATE SCHEMA IF NOT EXISTS
Drizzle-->>DbMgr: schema created/existed
Trigger->>DbMgr: applyPlan(schemaName, SchemaPlan.GATEWAY_ACTIVE)
DbMgr->>Drizzle: CREATE TABLE inbound_gateway ... + CREATE INDEX ...
Drizzle-->>DbMgr: table/indexes created
DbMgr-->>Trigger: resolved
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/api/src/modules/connections/connectors.service.spec.ts`:
- Around line 591-600: The test's ConfigService mock returns undefined for
get(), causing regionContext validation in ConnectorsService to throw before
applyPlan runs; update the TestingModule providers to supply a ConfigService
mock whose get() returns a valid regionContext object (matching the shape
checked by the service) so regionContext validation (the code around
regionContext in ConnectorsService) passes and the test can exercise applyPlan
and the failingDbManager path; keep the rest of the mocks (mockProviderRegistry,
mockEncryptionService, DATABASE_CONNECTION, DB_MANAGER) unchanged.
In `@apps/api/src/modules/connections/connectors.service.ts`:
- Line 345: The schemaName construction using `schemaName =
\`ws_${safeToken}_${hashedSuffix}\`` may approach PostgreSQL's 63-byte
identifier limit (max ~60 here) so add an explicit constraint/check: define a
constant like MAX_PG_IDENTIFIER_LENGTH = 63 (and a derived MAX_SCHEMA_NAME_LEN)
and ensure `safeToken` and/or `hashedSuffix` are truncated or validated before
composing `schemaName` (or throw a clear error); update the code paths that set
`safeToken`/`hashedSuffix` (references: `schemaName`, `safeToken`,
`hashedSuffix` in connectors.service) and add a short comment documenting the
length rationale.
In `@apps/api/src/modules/dbmanager/dbmanager.module.ts`:
- Around line 11-18: The factory's parameter drizzleDb in the provider for
DB_MANAGER should be explicitly typed to improve clarity and IDE support; import
the type alias (e.g., import type { DrizzleDb } from '@nexiom/database') and
update the useFactory signature to accept drizzleDb: DrizzleDb so the created
instance new SqlDatabaseManager(drizzleDb) has proper type information, leaving
the inject array (DATABASE_CONNECTION) unchanged.
In `@apps/api/src/modules/trigger/trigger-executor.service.ts`:
- Around line 441-445: The current explicit three-way comparator inside the
.sort((a, b) => { if (a < b) return -1; if (a > b) return 1; return 0; }) is
verbose—replace it with the idiomatic string comparator using localeCompare
(e.g., a.localeCompare(b)); if values may be null/undefined, normalize them
first (e.g., (a ?? '').localeCompare(b ?? '')) to avoid runtime errors; update
the comparator where it appears in trigger-executor.service.ts.
In `@docs/architecture/schema_lifecycle_and_provisioning.md`:
- Around line 18-24: The documentation incorrectly claims that provisioned
tables include sync_log, sync_cursor, and outbound_gateway while
SqlDatabaseManager.provisionGatewayTables() currently only creates
inbound_gateway; update the doc text in Stage 2 to reflect the actual behavior
by either (A) changing the "Tables Created" list to only include inbound_gateway
and adding a short note that sync_log, sync_cursor, and outbound_gateway are
planned additions, or (B) implement creation of the missing tables inside
SqlDatabaseManager.provisionGatewayTables() so the docs match the code;
reference SqlDatabaseManager.provisionGatewayTables() and the Stage 2 "Route
Activation (The \"Kernel\" Event)" section to locate where to change the docs or
the implementation.
- Around line 45-56: The documentation example is inconsistent: it calls
dbmanager.ensureKernelReady(schema) while the actual DatabaseManager API exposes
applyPlan(schemaName, plan); update the doc snippet in processLayerJob to call
the real method (e.g., dbmanager.applyPlan(schema, planOrEmpty) or show a small
wrapper method on DatabaseManager named ensureKernelReady that delegates to
applyPlan) so the doc matches the implementation and reference the symbols
processLayerJob, dbmanager.ensureKernelReady, and DatabaseManager.applyPlan when
making the change.
In `@packages/dbmanager/src/impl/sql-database-manager.ts`:
- Around line 23-26: The applyPlan flow can leave the schema partially created
if provisionGatewayTables fails; modify the function that runs CREATE SCHEMA and
calls provisionGatewayTables (e.g., applyPlan) to run all DDL inside an explicit
database transaction: begin a transaction via this.db.$client (e.g., BEGIN or
the client's transaction API), execute `CREATE SCHEMA IF NOT EXISTS
"${schemaName}";` and then call provisionGatewayTables and any subsequent DDL,
and on success COMMIT, otherwise ROLLBACK on error; ensure you use the same
client/connection for the whole transaction and surface the original error after
rollback.
- Around line 39-43: The code currently returns early when plan ===
SchemaPlan.REPLICA_ACTIVE, causing replica tables to never be provisioned;
update the logic in sql-database-manager (the method handling SchemaPlan) to
either call the intended provisionReplicaTables(schemaName) implementation
before returning or, if that implementation isn’t ready, throw a clear,
descriptive Error when SchemaPlan.REPLICA_ACTIVE is requested so callers don’t
get a silent success; reference SchemaPlan.REPLICA_ACTIVE and
provisionReplicaTables(schemaName) (or the containing method name if you prefer)
and ensure the error message states that REPLICA_ACTIVE provisioning is not yet
implemented.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
apps/api/package.jsonapps/api/src/app/app.module.tsapps/api/src/db/create-inbound-gateway.tsapps/api/src/modules/connections/connectors.service.spec.tsapps/api/src/modules/connections/connectors.service.tsapps/api/src/modules/dbmanager/dbmanager.module.tsapps/api/src/modules/trigger/trigger-executor.service.spec.tsapps/api/src/modules/trigger/trigger-executor.service.tsapps/api/src/modules/trigger/trigger.module.tsdocs/architecture/schema_lifecycle_and_provisioning.mdpackages/dbmanager/package.jsonpackages/dbmanager/src/impl/sql-database-manager.tspackages/dbmanager/src/index.tspackages/dbmanager/src/interfaces.tspackages/dbmanager/tsconfig.jsonpackages/dbmanager/tsconfig.tsbuildinfo
💤 Files with no reviewable changes (1)
- apps/api/src/db/create-inbound-gateway.ts
Comment on lines
+591
to
+600
| const moduleFail: TestingModule = await Test.createTestingModule({ | ||
| providers: [ | ||
| ConnectorsService, | ||
| { provide: ProviderRegistryService, useValue: mockProviderRegistry }, | ||
| { provide: ConfigService, useValue: { get: vi.fn() } }, | ||
| { provide: EncryptionService, useValue: mockEncryptionService }, | ||
| { provide: DATABASE_CONNECTION, useValue: mockDb as unknown }, | ||
| { provide: DB_MANAGER, useValue: failingDbManager }, | ||
| ], | ||
| }).compile(); |
Contributor
There was a problem hiding this comment.
Test will fail due to missing regionContext mock.
The ConfigService mock at line 595 returns undefined for all get() calls, causing the regionContext validation (lines 290-298 in the service) to throw InternalServerErrorException before applyPlan is ever called. This means the test isn't actually exercising the applyPlan failure path.
🐛 Proposed fix
{ provide: ProviderRegistryService, useValue: mockProviderRegistry },
- { provide: ConfigService, useValue: { get: vi.fn() } },
+ { provide: ConfigService, useValue: { get: vi.fn().mockReturnValue('us-east-1') } },
{ provide: EncryptionService, useValue: mockEncryptionService },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const moduleFail: TestingModule = await Test.createTestingModule({ | |
| providers: [ | |
| ConnectorsService, | |
| { provide: ProviderRegistryService, useValue: mockProviderRegistry }, | |
| { provide: ConfigService, useValue: { get: vi.fn() } }, | |
| { provide: EncryptionService, useValue: mockEncryptionService }, | |
| { provide: DATABASE_CONNECTION, useValue: mockDb as unknown }, | |
| { provide: DB_MANAGER, useValue: failingDbManager }, | |
| ], | |
| }).compile(); | |
| const moduleFail: TestingModule = await Test.createTestingModule({ | |
| providers: [ | |
| ConnectorsService, | |
| { provide: ProviderRegistryService, useValue: mockProviderRegistry }, | |
| { provide: ConfigService, useValue: { get: vi.fn().mockReturnValue('us-east-1') } }, | |
| { provide: EncryptionService, useValue: mockEncryptionService }, | |
| { provide: DATABASE_CONNECTION, useValue: mockDb as unknown }, | |
| { provide: DB_MANAGER, useValue: failingDbManager }, | |
| ], | |
| }).compile(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/modules/connections/connectors.service.spec.ts` around lines 591
- 600, The test's ConfigService mock returns undefined for get(), causing
regionContext validation in ConnectorsService to throw before applyPlan runs;
update the TestingModule providers to supply a ConfigService mock whose get()
returns a valid regionContext object (matching the shape checked by the service)
so regionContext validation (the code around regionContext in ConnectorsService)
passes and the test can exercise applyPlan and the failingDbManager path; keep
the rest of the mocks (mockProviderRegistry, mockEncryptionService,
DATABASE_CONNECTION, DB_MANAGER) unchanged.
| const finalProviderToken = sanitizedProvider || 'unknown'; | ||
| const safeToken = finalProviderToken.substring(0, 40); | ||
| const workspaceSchemaName = `ws_${safeToken}_${hashedSuffix}`; | ||
| const schemaName = `ws_${safeToken}_${hashedSuffix}`; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verify schema name length constraints.
The generated schemaName pattern ws_${safeToken}_${hashedSuffix} could be up to 3 + 40 + 1 + 16 = 60 characters. PostgreSQL limits identifier names to 63 bytes by default, so this is safe, but worth confirming the constraint is intentional.
📝 Consider adding a constant for documentation
+ // Max schema name: ws_ (3) + provider (40) + _ (1) + hash (16) = 60 chars (< PG 63-byte limit)
const schemaName = `ws_${safeToken}_${hashedSuffix}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const schemaName = `ws_${safeToken}_${hashedSuffix}`; | |
| // Max schema name: ws_ (3) + provider (40) + _ (1) + hash (16) = 60 chars (< PG 63-byte limit) | |
| const schemaName = `ws_${safeToken}_${hashedSuffix}`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/modules/connections/connectors.service.ts` at line 345, The
schemaName construction using `schemaName = \`ws_${safeToken}_${hashedSuffix}\``
may approach PostgreSQL's 63-byte identifier limit (max ~60 here) so add an
explicit constraint/check: define a constant like MAX_PG_IDENTIFIER_LENGTH = 63
(and a derived MAX_SCHEMA_NAME_LEN) and ensure `safeToken` and/or `hashedSuffix`
are truncated or validated before composing `schemaName` (or throw a clear
error); update the code paths that set `safeToken`/`hashedSuffix` (references:
`schemaName`, `safeToken`, `hashedSuffix` in connectors.service) and add a short
comment documenting the length rationale.
Comment on lines
+362
to
+368
| // 3. Apply the initial schema plan (NAMESPACE_ONLY) outside the transaction | ||
| // so the DDL runs on its own connection and does not silently escape the | ||
| // Drizzle tx scope (db.$client vs the transaction's dedicated connection). | ||
| await this.dbManager.applyPlan( | ||
| workspaceSchemaName, | ||
| SchemaPlan.NAMESPACE_ONLY, | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the structure of the connectors.service.ts file
fd connectors.service.ts --type fRepository: pramodnarayana/nexiom
Length of output: 119
🏁 Script executed:
# Search for the storeOAuthConnection function and surrounding context
rg -n "storeOAuthConnection" --type=ts -A 70 | head -100Repository: pramodnarayana/nexiom
Length of output: 9517
🏁 Script executed:
# Search for applyPlan function definition
rg -n "applyPlan\s*\(" --type=ts -B 2 -A 10Repository: pramodnarayana/nexiom
Length of output: 4011
🏁 Script executed:
# Search for PENDING_PROVISIONING or similar provisioning statuses
rg -n "PENDING_PROVISIONING|ProvisioningStatus|ConnectionStatus" --type=tsRepository: pramodnarayana/nexiom
Length of output: 1328
🏁 Script executed:
# Check for retry mechanisms and error handling patterns
rg -n "retry|Retry|RETRY" --type=ts -A 3 | head -80Repository: pramodnarayana/nexiom
Length of output: 6145
🏁 Script executed:
# Search for connectionStorageRegistry definition and usage
rg -n "connectionStorageRegistry" --type=ts -B 2 -A 5 | head -100Repository: pramodnarayana/nexiom
Length of output: 9413
🏁 Script executed:
# Get the full storeOAuthConnection function to see complete error handling
sed -n '278,385p' apps/api/src/modules/connections/connectors.service.tsRepository: pramodnarayana/nexiom
Length of output: 3523
🏁 Script executed:
# Check the full AppConnectionStatus enum definition
cat packages/database/src/schema/tenant.ts | head -30Repository: pramodnarayana/nexiom
Length of output: 1323
🏁 Script executed:
# Search for background jobs or reconciliation mechanisms
rg -n "reconcil|background|cron|job|cleanup" --type=ts -i | grep -E "(reconcil|background|cron|cleanup)" | head -40Repository: pramodnarayana/nexiom
Length of output: 4646
🏁 Script executed:
# Check if there's any mechanism to handle orphaned connections
rg -n "orphan|dangling|incomplete|unprovisioned" --type=ts -iRepository: pramodnarayana/nexiom
Length of output: 1251
🏁 Script executed:
# Look for error recovery or compensating transaction patterns
rg -n "compensat|rollback|cleanup|recover" --type=ts -i | head -30Repository: pramodnarayana/nexiom
Length of output: 2937
Fix partial failure vulnerability: applyPlan failure after transaction commits orphans metadata.
If applyPlan fails after the transaction commits, appConnections will be persisted with ACTIVE status while the schema remains unprovisioned. The system exposes this inconsistency with no recovery mechanism.
Implement one of:
- Compensating transaction: Delete the
connectionStorageRegistryrow onapplyPlanfailure - Retry with backoff: Retry
applyPlanbefore throwing (it is idempotent viaCREATE SCHEMA IF NOT EXISTS) - Provisioning status: Add a
PENDING_SCHEMAstatus toAppConnectionStatusand mark the connection during provisioning, with a background reconciler to clean up stale entries
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/modules/connections/connectors.service.spec.ts (1)
566-619: 🧹 Nitpick | 🔵 TrivialAssert
applyPlancall contract in the failure-path test.The test checks thrown error but not that
DB_MANAGER.applyPlanwas called with the expected plan. Add explicit call assertions to ensure this path is exercised as intended.✅ Suggested test hardening
import { DB_MANAGER } from '../dbmanager/dbmanager.module'; +import { SchemaPlan } from '@nexiom/dbmanager'; @@ await expect( failingService.storeOAuthConnection({ @@ }), ).rejects.toThrow(InternalServerErrorException); + + expect(failingDbManager.applyPlan).toHaveBeenCalledTimes(1); + expect(failingDbManager.applyPlan).toHaveBeenCalledWith( + expect.stringMatching(/^ws_[a-z0-9_]+$/), + SchemaPlan.NAMESPACE_ONLY, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/connections/connectors.service.spec.ts` around lines 566 - 619, Add assertions that DB_MANAGER.applyPlan was invoked with the expected plan and invocation count in this failure-path test: after calling failingService.storeOAuthConnection(...).rejects.toThrow(...), assert the failingDbManager.applyPlan mock was called once (toHaveBeenCalledTimes(1)) and that it received the expected plan shape using toHaveBeenCalledWith/expect.objectContaining (e.g., expect(failingDbManager.applyPlan).toHaveBeenCalledWith(expect.objectContaining({ operations: expect.any(Array) }) or the exact plan structure your implementation builds). Reference the failingDbManager mock, DB_MANAGER.applyPlan, and ConnectorsService.storeOAuthConnection when adding these assertions.
♻️ Duplicate comments (1)
apps/api/src/modules/connections/connectors.service.ts (1)
302-368:⚠️ Potential issue | 🟠 MajorAvoid committing
ACTIVEmetadata before provisioning succeeds.The transaction commits
appConnections.status = ACTIVEand storage registry rows beforeapplyPlanruns. IfapplyPlanfails, connection metadata remains active while infrastructure is not provisioned.Use a two-phase flow (provisioning status → ACTIVE on success) or add compensating failure handling so persisted state cannot claim readiness prematurely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/connections/connectors.service.ts` around lines 302 - 368, The code currently upserts appConnections with status ACTIVE inside db.transaction before calling this.dbManager.applyPlan, which may mark the connection ready even if applyPlan fails; modify the flow to use a two-phase state: during the initial tx (in the block using db.transaction where appConnections is upserted and connectionStorageRegistry is inserted) set the connection status to a provisional state such as PROVISIONING or PENDING (instead of AppConnectionStatus.ACTIVE) and return schemaName; then call this.dbManager.applyPlan(workspaceSchemaName, SchemaPlan.NAMESPACE_ONLY) outside the transaction and only after it succeeds perform a separate update (e.g. update appConnections by id) to set status = AppConnectionStatus.ACTIVE (and updatedAt); additionally, on applyPlan failure run a compensating rollback/update (e.g. mark appConnections as FAILED or remove the connectionStorageRegistry row) to avoid leaving the record in a ready state—refer to appConnections, connectionStorageRegistry, db.transaction, and applyPlan when locating the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture/schema_lifecycle_and_provisioning.md`:
- Line 56: The "1. Benefits of this Timing" heading restarts numbering; update
the ordered list item label to continue the sequence (change "1." to "5.") so
the section follows the previous "4." heading; look for the heading text
"Benefits of this Timing" in the document and adjust its numeric prefix
accordingly.
---
Outside diff comments:
In `@apps/api/src/modules/connections/connectors.service.spec.ts`:
- Around line 566-619: Add assertions that DB_MANAGER.applyPlan was invoked with
the expected plan and invocation count in this failure-path test: after calling
failingService.storeOAuthConnection(...).rejects.toThrow(...), assert the
failingDbManager.applyPlan mock was called once (toHaveBeenCalledTimes(1)) and
that it received the expected plan shape using
toHaveBeenCalledWith/expect.objectContaining (e.g.,
expect(failingDbManager.applyPlan).toHaveBeenCalledWith(expect.objectContaining({
operations: expect.any(Array) }) or the exact plan structure your implementation
builds). Reference the failingDbManager mock, DB_MANAGER.applyPlan, and
ConnectorsService.storeOAuthConnection when adding these assertions.
---
Duplicate comments:
In `@apps/api/src/modules/connections/connectors.service.ts`:
- Around line 302-368: The code currently upserts appConnections with status
ACTIVE inside db.transaction before calling this.dbManager.applyPlan, which may
mark the connection ready even if applyPlan fails; modify the flow to use a
two-phase state: during the initial tx (in the block using db.transaction where
appConnections is upserted and connectionStorageRegistry is inserted) set the
connection status to a provisional state such as PROVISIONING or PENDING
(instead of AppConnectionStatus.ACTIVE) and return schemaName; then call
this.dbManager.applyPlan(workspaceSchemaName, SchemaPlan.NAMESPACE_ONLY) outside
the transaction and only after it succeeds perform a separate update (e.g.
update appConnections by id) to set status = AppConnectionStatus.ACTIVE (and
updatedAt); additionally, on applyPlan failure run a compensating
rollback/update (e.g. mark appConnections as FAILED or remove the
connectionStorageRegistry row) to avoid leaving the record in a ready
state—refer to appConnections, connectionStorageRegistry, db.transaction, and
applyPlan when locating the changes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
apps/api/src/modules/connections/connectors.service.spec.tsapps/api/src/modules/connections/connectors.service.tsapps/api/src/modules/dbmanager/dbmanager.module.tsapps/api/src/modules/trigger/trigger-executor.service.tsdocs/architecture/schema_lifecycle_and_provisioning.mdpackages/dbmanager/src/impl/sql-database-manager.tspackages/dbmanager/tsconfig.tsbuildinfo
| await db.withSchema(schema).insert(...); | ||
| } | ||
|
|
||
| 1. Benefits of this Timing |
Contributor
There was a problem hiding this comment.
Fix section numbering in the heading.
Line 56 resets numbering to 1. after section 4. This should continue the sequence (likely 5.) to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture/schema_lifecycle_and_provisioning.md` at line 56, The "1.
Benefits of this Timing" heading restarts numbering; update the ordered list
item label to continue the sequence (change "1." to "5.") so the section follows
the previous "4." heading; look for the heading text "Benefits of this Timing"
in the document and adjust its numeric prefix accordingly.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/dbmanager/src/impl/sql-database-manager.ts (2)
39-43:⚠️ Potential issue | 🟠 Major
REPLICA_ACTIVEstill returns without replica provisioning.
Line 41–Line 42reports success but does not create replica structures. This can cause deferred runtime failures in callers expecting replica readiness.🛠️ Proposed fix
// 3. (Future) Ensure Replica Tables exist // TODO: implement provisionReplicaTables(schemaName) before enabling REPLICA_ACTIVE in production if (plan === SchemaPlan.REPLICA_ACTIVE) { - return; + throw new Error( + 'SchemaPlan.REPLICA_ACTIVE is not implemented yet. Use SchemaPlan.GATEWAY_ACTIVE until replica provisioning is available.', + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dbmanager/src/impl/sql-database-manager.ts` around lines 39 - 43, The branch that exits early when plan === SchemaPlan.REPLICA_ACTIVE must actually provision replica structures instead of returning; update the block in sql-database-manager.ts to call an implementation like provisionReplicaTables(schemaName) (implement that function if missing) and await it before returning, propagate/log any errors and only return after successful provisioning so callers can rely on replica readiness; reference the SchemaPlan.REPLICA_ACTIVE check and the new provisionReplicaTables(schemaName) call.
23-33:⚠️ Potential issue | 🟠 MajorProvisioning flow is non-atomic across multiple DDL statements.
If any later DDL fails after earlier statements succeed, the schema is left partially provisioned (despite the comment around
Line 47–Line 49). Wrap the full plan execution in an explicit transaction.🛠️ Proposed fix
async applyPlan(schemaName: string, plan: SchemaPlan): Promise<void> { this.validateSchemaName(schemaName); + await this.db.$client.query('BEGIN'); + try { - // 1. Always ensure namespace exists (Minimum baseline for all plans) - await this.db.$client.query( - `CREATE SCHEMA IF NOT EXISTS "${schemaName}";`, - ); + // 1. Always ensure namespace exists (Minimum baseline for all plans) + await this.db.$client.query( + `CREATE SCHEMA IF NOT EXISTS "${schemaName}";`, + ); - if (plan === SchemaPlan.NAMESPACE_ONLY) { - return; - } + if (plan === SchemaPlan.NAMESPACE_ONLY) { + await this.db.$client.query('COMMIT'); + return; + } - // 2. Ensure Gateway Tables exist - await this.provisionGatewayTables(schemaName); + // 2. Ensure Gateway Tables exist + await this.provisionGatewayTables(schemaName); - if (plan === SchemaPlan.GATEWAY_ACTIVE) { - return; - } + if (plan === SchemaPlan.GATEWAY_ACTIVE) { + await this.db.$client.query('COMMIT'); + return; + } - // 3. (Future) Ensure Replica Tables exist - // TODO: implement provisionReplicaTables(schemaName) before enabling REPLICA_ACTIVE in production - if (plan === SchemaPlan.REPLICA_ACTIVE) { - return; - } + // 3. (Future) Ensure Replica Tables exist + // TODO: implement provisionReplicaTables(schemaName) before enabling REPLICA_ACTIVE in production + if (plan === SchemaPlan.REPLICA_ACTIVE) { + throw new Error( + 'SchemaPlan.REPLICA_ACTIVE is not implemented yet.', + ); + } + await this.db.$client.query('COMMIT'); + } catch (error) { + await this.db.$client.query('ROLLBACK'); + throw error; + } }Also applies to: 46-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dbmanager/src/impl/sql-database-manager.ts` around lines 23 - 33, The provisioning flow executes multiple DDLs outside a transaction, so wrap the entire plan in an explicit DB transaction: begin a transaction via this.db.$client (or its transaction helper), then run the CREATE SCHEMA query and the subsequent steps (including the check for SchemaPlan.NAMESPACE_ONLY and the call to this.provisionGatewayTables(schemaName)), commit on success and rollback on any error, rethrow the error after rollback; ensure the early return for SchemaPlan.NAMESPACE_ONLY happens inside the transaction so the transaction can be committed/rolled back appropriately.
🤖 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/dbmanager/src/impl/sql-database-manager.ts`:
- Around line 39-43: The branch that exits early when plan ===
SchemaPlan.REPLICA_ACTIVE must actually provision replica structures instead of
returning; update the block in sql-database-manager.ts to call an implementation
like provisionReplicaTables(schemaName) (implement that function if missing) and
await it before returning, propagate/log any errors and only return after
successful provisioning so callers can rely on replica readiness; reference the
SchemaPlan.REPLICA_ACTIVE check and the new provisionReplicaTables(schemaName)
call.
- Around line 23-33: The provisioning flow executes multiple DDLs outside a
transaction, so wrap the entire plan in an explicit DB transaction: begin a
transaction via this.db.$client (or its transaction helper), then run the CREATE
SCHEMA query and the subsequent steps (including the check for
SchemaPlan.NAMESPACE_ONLY and the call to
this.provisionGatewayTables(schemaName)), commit on success and rollback on any
error, rethrow the error after rollback; ensure the early return for
SchemaPlan.NAMESPACE_ONLY happens inside the transaction so the transaction can
be committed/rolled back appropriately.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
.gitignorepackages/dbmanager/src/impl/sql-database-manager.ts
This was referenced Mar 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ntrol plane
Summary by CodeRabbit
New Features
Documentation
Tests