Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughRDB stores now use a two-stage GORM lifecycle: a throwaway connection runs migrations/version checks and is closed, then a fresh runtime pool is opened, tuned, atomically stored, and used for runtime DB access. RDBConfigStore exposes RunMigration(fn) and RefreshConnectionPool and routes all DB access through s.DB(). Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Builder as newPostgresConfigStore
participant MigrateDB as MigrationDB (throwaway)
participant RuntimeDB as RuntimeDB (main pool)
Caller->>Builder: create store
Builder->>MigrateDB: open migration GORM (mDb)
Builder->>MigrateDB: triggerMigrations(ctx, mDb)
alt migrations fail
MigrateDB-->>Builder: error
Builder->>MigrateDB: close mDb
Builder-->>Caller: return error
else migrations succeed
MigrateDB-->>Builder: success
Builder->>MigrateDB: close mDb
Builder->>RuntimeDB: open/configure fresh runtime GORM
Builder->>RuntimeDB: apply pool tuning
Builder-->>Caller: return initialized store
end
sequenceDiagram
participant Caller
participant Store as RDBConfigStore
participant NewPool as NewRuntimeDB
participant OldPool as OldRuntimeDB
Caller->>Store: RefreshConnectionPool(ctx)
Store->>NewPool: open new runtime pool
alt open fails
NewPool-->>Store: error
Store-->>Caller: return error
else open succeeds
NewPool-->>Store: new *gorm.DB
Store->>OldPool: atomic swap (s.db.Swap(new))
Store->>OldPool: close old pool
Store-->>Caller: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
|
|
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
Confidence Score: 5/5Safe to merge — the previously flagged P0/P1 issues (empty RefreshConnectionPool body, migration pool leak on success and on runtime-pool-open failure) are all resolved in this revision. All remaining findings are P2 style suggestions; the core two-pool fix and atomic swap logic are correct, connection cleanup is handled on every code path in configstore/postgres.go, and the test helpers are properly updated. framework/logstore/postgres.go — minor close-error discard on the runtime pool failure path (line 115). Important Files Changed
Reviews (5): Last reviewed commit: "run migrations on separate connection to..." | Re-trigger Greptile |
akshaydeo
changed the base branch from
04-18-compilation_fixes
to
graphite-base/2831
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
akshaydeo
force-pushed
the
graphite-base/2831
branch
from
867b4d2 to
4559d6a
Compare
akshaydeo
force-pushed
the
04-19-separate_conenction_for_migrations
branch
from
ac6d6b7 to
882c688
Compare
akshaydeo
force-pushed
the
04-19-separate_conenction_for_migrations
branch
from
882c688 to
55fe2be
Compare
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/configstore/postgres.go (1)
53-81:⚠️ Potential issue | 🟠 MajorDefer close the migration DB connection immediately after opening.
mDbis only closed iftriggerMigrations()fails. On the success path or if the secondgorm.Open()call fails at line 79, the migration connection remains open, leaking a database connection.Proposed fix
mDb, err := gorm.Open(postgres.New(postgres.Config{ DSN: dsn, }), &gorm.Config{ Logger: newGormLogger(logger), }) if err != nil { return nil, err } + mSqlDB, err := mDb.DB() + if err != nil { + return nil, err + } + defer func() { + if closeErr := mSqlDB.Close(); closeErr != nil { + logger.Error("failed to close DB connection: %v", closeErr) + } + }() // Run migrations if err := triggerMigrations(ctx, mDb); err != nil { - // Closing the DB connection - if sqlDB, dbErr := mDb.DB(); dbErr == nil { - if closeErr := sqlDB.Close(); closeErr != nil { - logger.Error("failed to close DB connection: %v", closeErr) - } - } return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/postgres.go` around lines 53 - 81, Open migration DB (mDb) then immediately obtain its underlying *sql.DB via mDb.DB() and defer closing it so the connection is always released; keep using triggerMigrations(ctx, mDb) as before and remove the current ad-hoc close-on-error logic. Concretely, after gorm.Open that returns mDb, call sqlDB, _ := mDb.DB() and add a defer func() { if err := sqlDB.Close(); err != nil { logger.Error("failed to close migration DB: %v", err) } }() so the migration connection is closed on both success and failure before you open the separate pool (db).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/rdb.go`:
- Around line 123-126: RefreshConnectionPool on RDBConfigStore declares an error
return but has an empty body; implement it to return an error on failure similar
to Ping(). Update the method RefreshConnectionPool to use the existing DB handle
(s.db) with the provided ctx and perform a lightweight check (e.g.
s.db.WithContext(ctx).Exec("SELECT 1").Error) or obtain the underlying sql.DB
and call PingContext(ctx) (via sqlDB, e.g. sqlDB.DB().PingContext(ctx)) and
return that error so all code paths return an error value.
---
Outside diff comments:
In `@framework/configstore/postgres.go`:
- Around line 53-81: Open migration DB (mDb) then immediately obtain its
underlying *sql.DB via mDb.DB() and defer closing it so the connection is always
released; keep using triggerMigrations(ctx, mDb) as before and remove the
current ad-hoc close-on-error logic. Concretely, after gorm.Open that returns
mDb, call sqlDB, _ := mDb.DB() and add a defer func() { if err := sqlDB.Close();
err != nil { logger.Error("failed to close migration DB: %v", err) } }() so the
migration connection is closed on both success and failure before you open the
separate pool (db).
🪄 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: CHILL
Plan: Pro
Run ID: 0d0c8906-c647-450f-8637-05b08aa770a9
📒 Files selected for processing (3)
framework/configstore/postgres.goframework/configstore/rdb.goframework/configstore/store.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
framework/configstore/rdb.go (1)
124-126:⚠️ Potential issue | 🔴 Critical
RefreshConnectionPoolis still non-compilable due to missing return value.Line 124 declares an
errorreturn, but Lines 125-126 provide no return path.💡 Minimal fix
func (s *RDBConfigStore) RefreshConnectionPool(ctx context.Context) error { - + return s.db.WithContext(ctx).Exec("SELECT 1").Error }#!/bin/bash set -euo pipefail # Verify that RefreshConnectionPool declares an error return and whether its body has a return statement. python - <<'PY' from pathlib import Path p = Path("framework/configstore/rdb.go") lines = p.read_text().splitlines() sig = "func (s *RDBConfigStore) RefreshConnectionPool(ctx context.Context) error" start = next((i for i, l in enumerate(lines, 1) if sig in l), None) if start is None: raise SystemExit("method signature not found") brace = 0 end = None body = [] for i in range(start, len(lines) + 1): line = lines[i - 1] brace += line.count("{") if i > start: body.append((i, line)) brace -= line.count("}") if i > start and brace == 0: end = i break has_return = any("return" in l for _, l in body) print(f"method_range={start}-{end}") print(f"has_return_statement={has_return}") for n, l in body: print(f"{n}: {l}") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` around lines 124 - 126, The method RefreshConnectionPool on RDBConfigStore currently has no return path and fails to compile; update its implementation to perform the intended connection-pool refresh logic and ensure it returns an error value (e.g., return nil on success or an appropriate error on failure). Locate func (s *RDBConfigStore) RefreshConnectionPool(ctx context.Context) error, implement the pool-refresh steps you need (using s, ctx, and any internal pool fields/methods) and add a final return statement returning nil for success or the encountered error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@framework/configstore/rdb.go`:
- Around line 124-126: The method RefreshConnectionPool on RDBConfigStore
currently has no return path and fails to compile; update its implementation to
perform the intended connection-pool refresh logic and ensure it returns an
error value (e.g., return nil on success or an appropriate error on failure).
Locate func (s *RDBConfigStore) RefreshConnectionPool(ctx context.Context)
error, implement the pool-refresh steps you need (using s, ctx, and any internal
pool fields/methods) and add a final return statement returning nil for success
or the encountered error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d2cd7e2-42ca-4d9f-8651-6db84706d3da
📒 Files selected for processing (3)
framework/configstore/postgres.goframework/configstore/rdb.goframework/configstore/store.go
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/configstore/store.go
- framework/configstore/postgres.go
akshaydeo
force-pushed
the
04-19-separate_conenction_for_migrations
branch
2 times, most recently
from
9fd0550 to
5dabb03
Compare
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)
framework/configstore/rdb.go (1)
3844-3849:⚠️ Potential issue | 🟠 Major
RunMigrationstill bypasses the fresh-connection path.Line 3848 constructs the migrator from
s.DB()instead ofMigrateOnFreshConnection. That leaves the original cached-plan failure available to any caller that invokesRunMigrationdirectly after the runtime pool has already prepared statements. This entry point should use the same throwaway-connection flow as the startup path, then refresh the runtime pool on success.Suggested fix
func (s *RDBConfigStore) RunMigration(ctx context.Context, migration *migrator.Migration) error { if migration == nil { return fmt.Errorf("migration cannot be nil") } - m := migrator.New(s.DB(), migrator.DefaultOptions, []*migrator.Migration{migration}) - return m.Migrate() + if err := s.MigrateOnFreshConnection(ctx, func(ctx context.Context, db *gorm.DB) error { + m := migrator.New(db.WithContext(ctx), migrator.DefaultOptions, []*migrator.Migration{migration}) + return m.Migrate() + }); err != nil { + return err + } + return s.RefreshConnectionPool(ctx) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` around lines 3844 - 3849, RunMigration currently constructs the migrator with s.DB(), bypassing the startup throwaway-connection flow; change RunMigration to build/run the migrator via the MigrateOnFreshConnection path used at startup (i.e. use the same MigrateOnFreshConnection helper instead of s.DB() when creating the migrator in RunMigration), and after a successful m.Migrate() call refresh the runtime connection pool the same way the startup code does so prepared-statement cache issues are cleared for future callers.
🧹 Nitpick comments (2)
framework/configstore/migrations_test.go (1)
1125-1130: Keep one test on the real pool-refresh path.This helper now replaces
refreshPoolFnwith a no-op, and all other test helpers in the codebase do the same. While this keeps the existing test suites stable, the real implementation inpostgres.go(which opens a fresh connection, applies pool tuning, atomically swaps, and closes the old pool) is never exercised by any unit test. Please add or preserve at least one test that exercisesRefreshConnectionPoolwith a real PostgreSQL backend to catch regressions in the actual pool lifecycle logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations_test.go` around lines 1125 - 1130, The helper currently stubs out refreshPoolFn on RDBConfigStore making tests miss the real pool lifecycle; restore or add one unit/integration test that invokes RefreshConnectionPool against a real Postgres backend instead of the no-op: update the test setup so RDBConfigStore.refreshPoolFn is not replaced for that test (or add a new test case) and call store.RefreshConnectionPool(ctx) to exercise the code path implemented in postgres.go (the actual pool-open, tuning, swap and close sequence), using the same DB instance created by store.db.Store(db) so the real pool swap runs; ensure teardown closes pools to avoid leaks.framework/configstore/rdb_test.go (1)
56-62: Add a hook-level regression test for the new migration lifecycle.This helper now wires
migrateOnFreshFn/refreshPoolFn, but the file still doesn't assert that migration code actually goes through those hooks. A small spy-based test here would catch regressions where DDL accidentally falls back toDB()again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb_test.go` around lines 56 - 62, Add a spy-based test that ensures migrations go through the hook fields instead of directly calling s.DB(): create an RDBConfigStore instance and replace s.migrateOnFreshFn with a spy function that records it was invoked and captures the *gorm.DB passed in, and replace s.refreshPoolFn with a spy that records invocation; also replace s.db.Store(db) with a sentinel db (or wrap the DB accessor) that will set a flag or panic if s.DB() is invoked directly during the migration path; then run the same migration-exercising call used in the test suite (the public migration path exercised by this file) and assert the migrateOnFreshFn and refreshPoolFn spies were called and that the direct s.DB() access flag/panic was not triggered. Ensure you reference RDBConfigStore, migrateOnFreshFn, refreshPoolFn and DB() in the test so regressions are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/logstore/postgres.go`:
- Around line 64-73: The migration "closePool" currently swallows sqlDB.Close()
errors and allows NewLogStore to continue initializing; change the close logic
so that closePool (used for the throwaway migration pool) returns an error when
sqlDB.Close() fails and propagate that error up in NewLogStore to abort startup
instead of proceeding to open the runtime pool. Concretely, update the closePool
function referenced as closePool/db.DB()/sqlDB.Close to return an error, check
that error after calling closePool in NewLogStore, and return the error from
NewLogStore (failing startup) if closing the migration pool fails; apply the
same change to the other close call at the second location that currently logs
and continues (lines referenced in review: 98-101).
---
Outside diff comments:
In `@framework/configstore/rdb.go`:
- Around line 3844-3849: RunMigration currently constructs the migrator with
s.DB(), bypassing the startup throwaway-connection flow; change RunMigration to
build/run the migrator via the MigrateOnFreshConnection path used at startup
(i.e. use the same MigrateOnFreshConnection helper instead of s.DB() when
creating the migrator in RunMigration), and after a successful m.Migrate() call
refresh the runtime connection pool the same way the startup code does so
prepared-statement cache issues are cleared for future callers.
---
Nitpick comments:
In `@framework/configstore/migrations_test.go`:
- Around line 1125-1130: The helper currently stubs out refreshPoolFn on
RDBConfigStore making tests miss the real pool lifecycle; restore or add one
unit/integration test that invokes RefreshConnectionPool against a real Postgres
backend instead of the no-op: update the test setup so
RDBConfigStore.refreshPoolFn is not replaced for that test (or add a new test
case) and call store.RefreshConnectionPool(ctx) to exercise the code path
implemented in postgres.go (the actual pool-open, tuning, swap and close
sequence), using the same DB instance created by store.db.Store(db) so the real
pool swap runs; ensure teardown closes pools to avoid leaks.
In `@framework/configstore/rdb_test.go`:
- Around line 56-62: Add a spy-based test that ensures migrations go through the
hook fields instead of directly calling s.DB(): create an RDBConfigStore
instance and replace s.migrateOnFreshFn with a spy function that records it was
invoked and captures the *gorm.DB passed in, and replace s.refreshPoolFn with a
spy that records invocation; also replace s.db.Store(db) with a sentinel db (or
wrap the DB accessor) that will set a flag or panic if s.DB() is invoked
directly during the migration path; then run the same migration-exercising call
used in the test suite (the public migration path exercised by this file) and
assert the migrateOnFreshFn and refreshPoolFn spies were called and that the
direct s.DB() access flag/panic was not triggered. Ensure you reference
RDBConfigStore, migrateOnFreshFn, refreshPoolFn and DB() in the test so
regressions are caught.
🪄 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: CHILL
Plan: Pro
Run ID: 8f1ac7f0-7777-4dab-b0c3-14a1ffb6148c
📒 Files selected for processing (12)
framework/configstore/dlock_test.goframework/configstore/encryption.goframework/configstore/encryption_test.goframework/configstore/migrations.goframework/configstore/migrations_test.goframework/configstore/postgres.goframework/configstore/prompts.goframework/configstore/rdb.goframework/configstore/rdb_test.goframework/configstore/sqlite.goframework/configstore/store.goframework/logstore/postgres.go
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/configstore/store.go
- framework/configstore/postgres.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
framework/logstore/postgres.go (1)
64-73:⚠️ Potential issue | 🟠 MajorFail initialization if the migration pool cannot be closed.
Line 64-73 swallows close failures, and Line 98 continues startup even if migration-pool close fails. That weakens the two-pool guarantee and can leave pre-migration connections alive. Please make
closePoolreturnerrorand fail fast when closingmDbfails.💡 Proposed fix
- closePool := func(db *gorm.DB) { + closePool := func(db *gorm.DB) error { if db == nil { - return + return nil } - if sqlDB, err := db.DB(); err == nil { - if closeErr := sqlDB.Close(); closeErr != nil { - logger.Error("failed to close DB connection: %v", closeErr) - } + sqlDB, err := db.DB() + if err != nil { + return err } + return sqlDB.Close() } @@ - if err := mDb.Raw("SELECT current_setting('server_version_num')::int").Scan(&pgVersionNum).Error; err != nil { - closePool(mDb) + if err := mDb.Raw("SELECT current_setting('server_version_num')::int").Scan(&pgVersionNum).Error; err != nil { + _ = closePool(mDb) return nil, err } if pgVersionNum < 160000 { - closePool(mDb) + _ = closePool(mDb) return nil, fmt.Errorf("postgres version is lower than 16, please upgrade to 16 or higher") } @@ - if err := triggerMigrations(ctx, mDb); err != nil { - closePool(mDb) + if err := triggerMigrations(ctx, mDb); err != nil { + _ = closePool(mDb) return nil, err } - closePool(mDb) + if err := closePool(mDb); err != nil { + return nil, fmt.Errorf("close migration db connection: %w", err) + } @@ sqlDB, err := db.DB() if err != nil { - closePool(db) + _ = closePool(db) return nil, err }Also applies to: 86-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/postgres.go` around lines 64 - 73, The closePool helper currently swallows errors; change closePool to return error (signature closePool(db *gorm.DB) error), propagate and check its return where it's used (notably when closing the migration pool mDb), and if closePool returns an error log it and return that error to abort initialization (fail fast) so startup does not continue when closing mDb fails; ensure the implementation returns any sqlDB.Close() error (use logger.Errorf or logger.Error with the error) instead of ignoring it, and update all callers (the places around mDb handling and the other pool close at lines shown) to handle the error and stop startup on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/rdb.go`:
- Around line 178-185: The code is incorrectly caching the runtime DB handle
(e.g., txDB := s.DB() or baseQuery := s.DB()) which can be closed by
RefreshConnectionPool; update all non-transactional sites in this file
(including the blocks around the existing DB() use and the other occurrences
around the 362-368 region) to call s.DB() immediately before each database
operation instead of storing it once up-front, ensure transactional code still
uses a single *gorm.DB from Begin/Commit/Rollback when needed, and remove or
refactor any txDB/baseQuery variables in non-transactional methods so every
statement invokes s.DB() anew to avoid using a closed *sql.DB.
- Around line 195-204: The methods RDBConfigStore.RunMigration and
RefreshConnectionPool currently call the function fields migrateOnFreshFn and
refreshPoolFn without nil checks; change each to guard the corresponding field
on s (migrateOnFreshFn and refreshPoolFn) and return a clear error if the hook
is nil rather than dereferencing and panicking—e.g., check s.migrateOnFreshFn !=
nil in RunMigration and s.refreshPoolFn != nil in RefreshConnectionPool and
return an error (with context mentioning the method name) when the hook is not
set; otherwise call the hook as before.
In `@framework/logstore/asyncjob_test.go`:
- Around line 89-92: capturedCtx is nil when you call SetValue, causing a panic;
before any SetValue calls (the instances using capturedCtx, SetValue,
schemas.BifrostContext and
schemas.BifrostContextKey*/BifrostContextKeyVirtualKey), initialize capturedCtx
(e.g. assign it a concrete value via a constructor like
schemas.NewBifrostContext() if available or &schemas.BifrostContext{}), then
call SetValue; apply this initialization at each occurrence where capturedCtx is
declared and immediately used.
---
Duplicate comments:
In `@framework/logstore/postgres.go`:
- Around line 64-73: The closePool helper currently swallows errors; change
closePool to return error (signature closePool(db *gorm.DB) error), propagate
and check its return where it's used (notably when closing the migration pool
mDb), and if closePool returns an error log it and return that error to abort
initialization (fail fast) so startup does not continue when closing mDb fails;
ensure the implementation returns any sqlDB.Close() error (use logger.Errorf or
logger.Error with the error) instead of ignoring it, and update all callers (the
places around mDb handling and the other pool close at lines shown) to handle
the error and stop startup on failure.
🪄 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: CHILL
Plan: Pro
Run ID: 703a7267-e1f6-48d1-8357-45de6a08809d
📒 Files selected for processing (13)
framework/configstore/dlock_test.goframework/configstore/encryption.goframework/configstore/encryption_test.goframework/configstore/migrations.goframework/configstore/migrations_test.goframework/configstore/postgres.goframework/configstore/prompts.goframework/configstore/rdb.goframework/configstore/rdb_test.goframework/configstore/sqlite.goframework/configstore/store.goframework/logstore/asyncjob_test.goframework/logstore/postgres.go
✅ Files skipped from review due to trivial changes (4)
- framework/configstore/rdb_test.go
- framework/configstore/prompts.go
- framework/configstore/postgres.go
- framework/configstore/encryption_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- framework/configstore/sqlite.go
- framework/configstore/encryption.go
- framework/configstore/dlock_test.go
- framework/configstore/store.go
akshaydeo
force-pushed
the
04-19-separate_conenction_for_migrations
branch
from
5dabb03 to
98ebb2f
Compare
Contributor
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)
framework/configstore/rdb.go (1)
3198-3224:⚠️ Potential issue | 🟠 MajorReject empty
ScopeIDfor non-global routing rules.These checks only reject
nil. A rule with a non-global scope andScopeID: bifrost.Ptr("")still gets created or updated here, butGetRoutingRulesByScopelater rejects empty scope IDs, so the rule becomes unreachable.Based on learnings: Enforce routing rules: scopeID must be set for all non-global scopes (team, customer, virtual_key). Only global scope is allowed to have an empty or NULL scopeID.🛡️ Suggested guard
- if rule.Scope != "" && rule.Scope != "global" && rule.ScopeID == nil { + if rule.Scope != "" && rule.Scope != "global" && (rule.ScopeID == nil || strings.TrimSpace(*rule.ScopeID) == "") { return fmt.Errorf("scopeID is required for non-global scope '%s'", rule.Scope) } @@ - if rule.Scope != "" && rule.Scope != "global" && rule.ScopeID == nil { + if rule.Scope != "" && rule.Scope != "global" && (rule.ScopeID == nil || strings.TrimSpace(*rule.ScopeID) == "") { return fmt.Errorf("scopeID is required for non-global scope '%s'", rule.Scope) }Also applies to: 3247-3273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` around lines 3198 - 3224, The validation currently only rejects nil ScopeID for non-global scopes; update the checks in the save/update blocks that use rule.Scope and rule.ScopeID (the block using database := s.DB(), query := database.WithContext(ctx)... and the similar block around lines 3247-3273) to also reject empty-string ScopeIDs (e.g., if rule.Scope != "" && rule.Scope != "global" && (rule.ScopeID == nil || *rule.ScopeID == "") return an error). Ensure the error message mirrors the existing one and reference GetRoutingRulesByScope behavior by enforcing that non-global scopes (team, customer, virtual_key) must have a non-empty ScopeID.
♻️ Duplicate comments (1)
framework/configstore/rdb.go (1)
377-408:⚠️ Potential issue | 🟠 MajorStop caching
s.DB()across multi-step operations.Line 377, Line 1211, and Line 3198 still pin the pre-refresh handle in
txDB,baseQuery, ordatabaseand then reuse it for later statements. AfterRefreshConnectionPoolswaps and closes the old pool, those later calls can fail with a closed-DB error. Please apply the same per-operations.DB()pattern here and at the other remaining non-transactional sites in this file.♻️ Example fix pattern
- var txDB *gorm.DB - if len(tx) > 0 { - txDB = tx[0] - } else { - txDB = s.DB() - } + currentDB := func() *gorm.DB { + if len(tx) > 0 && tx[0] != nil { + return tx[0] + } + return s.DB() + } @@ - if err := txDB.WithContext(ctx).Clauses( + if err := currentDB().WithContext(ctx).Clauses( clause.OnConflict{Also applies to: 1211-1241, 3198-3303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` around lines 377 - 408, UpdateProvidersConfig (and other non-transactional code in this file like usages that built a persistent baseQuery or stored a database handle) currently caches a single DB handle in variables such as txDB/database/baseQuery and reuses it across multiple statements, which breaks when RefreshConnectionPool swaps and closes the old pool; fix by calling s.DB() immediately before each independent DB operation instead of reusing a previously stored handle—e.g., in UpdateProvidersConfig call cur := s.DB() (or use s.DB().WithContext(ctx) inline) for the Create/Clauses call inside the loop (and apply the same change to other sites that assign txDB/baseQuery/database outside the per-operation scope) so every statement uses a fresh live connection handle.
🤖 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 `@framework/configstore/rdb.go`:
- Around line 3198-3224: The validation currently only rejects nil ScopeID for
non-global scopes; update the checks in the save/update blocks that use
rule.Scope and rule.ScopeID (the block using database := s.DB(), query :=
database.WithContext(ctx)... and the similar block around lines 3247-3273) to
also reject empty-string ScopeIDs (e.g., if rule.Scope != "" && rule.Scope !=
"global" && (rule.ScopeID == nil || *rule.ScopeID == "") return an error).
Ensure the error message mirrors the existing one and reference
GetRoutingRulesByScope behavior by enforcing that non-global scopes (team,
customer, virtual_key) must have a non-empty ScopeID.
---
Duplicate comments:
In `@framework/configstore/rdb.go`:
- Around line 377-408: UpdateProvidersConfig (and other non-transactional code
in this file like usages that built a persistent baseQuery or stored a database
handle) currently caches a single DB handle in variables such as
txDB/database/baseQuery and reuses it across multiple statements, which breaks
when RefreshConnectionPool swaps and closes the old pool; fix by calling s.DB()
immediately before each independent DB operation instead of reusing a previously
stored handle—e.g., in UpdateProvidersConfig call cur := s.DB() (or use
s.DB().WithContext(ctx) inline) for the Create/Clauses call inside the loop (and
apply the same change to other sites that assign txDB/baseQuery/database outside
the per-operation scope) so every statement uses a fresh live connection handle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9290406-9fe1-4a43-932b-679e10208bc5
📒 Files selected for processing (13)
framework/configstore/dlock_test.goframework/configstore/encryption.goframework/configstore/encryption_test.goframework/configstore/migrations.goframework/configstore/migrations_test.goframework/configstore/postgres.goframework/configstore/prompts.goframework/configstore/rdb.goframework/configstore/rdb_test.goframework/configstore/sqlite.goframework/configstore/store.goframework/logstore/asyncjob_test.goframework/logstore/postgres.go
✅ Files skipped from review due to trivial changes (5)
- framework/configstore/encryption_test.go
- framework/logstore/asyncjob_test.go
- framework/configstore/prompts.go
- framework/configstore/encryption.go
- framework/configstore/postgres.go
🚧 Files skipped from review as they are similar to previous changes (3)
- framework/configstore/sqlite.go
- framework/logstore/postgres.go
- framework/configstore/migrations.go
This was referenced Apr 19, 2026
Merged
akshaydeo
force-pushed
the
04-19-separate_conenction_for_migrations
branch
from
98ebb2f to
f0e0659
Compare
18 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
framework/configstore/postgres.go (1)
40-50:⚠️ Potential issue | 🟠 MajorPropagate pool-close failures instead of only logging them.
closeDbConncurrently swallowsdb.DB()/sqlDB.Close()errors, so Line 115, Line 137, and Line 156 can all report success while the old pool is still alive. That weakens the main guarantee of this PR: the pre-migration/runtime pool may survive after DDL, andRefreshConnectionPoolcan silently fail to complete.💡 Proposed fix
-func closeDbConn(db *gorm.DB, logger schemas.Logger) { - sqlDB, err := db.DB() - if err != nil { - logger.Error("failed to resolve *sql.DB for close: %v", err) - return - } - if err := sqlDB.Close(); err != nil { - logger.Error("failed to close DB connection: %v", err) - } +func closeDbConn(db *gorm.DB) error { + if db == nil { + return nil + } + sqlDB, err := db.DB() + if err != nil { + return fmt.Errorf("resolve *sql.DB for close: %w", err) + } + if err := sqlDB.Close(); err != nil { + return fmt.Errorf("close db connection: %w", err) + } + return nil }Then propagate the returned error at the migration-pool close, the deferred
migrateOnFreshFnclose, and the old-pool close inrefreshPoolFn(joining it with the primary error on failure paths if needed).Run the following read-only script to confirm that
closeDbConncannot currently propagate close failures and that all callers ignore them:#!/bin/bash set -euo pipefail echo "closeDbConn definition:" rg -n -C2 'func closeDbConn\(db \*gorm\.DB, logger schemas\.Logger\)' framework/configstore/postgres.go echo echo "closeDbConn call sites:" rg -n -C2 'closeDbConn\(' framework/configstore/postgres.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/postgres.go` around lines 40 - 50, closeDbConn currently swallows errors; change its signature from closeDbConn(db *gorm.DB, logger schemas.Logger) to return error (e.g., closeDbConn(...) error), have it return the db.DB() or sqlDB.Close() error (wrapped with context via fmt.Errorf or errors.Join when needed), then update all call sites (RefreshConnectionPool, refreshPoolFn, migrateOnFreshFn and any deferred closes) to check and propagate/aggregate the returned error instead of silently logging—on failure paths join the pool-close error with the primary error (errors.Join or similar) and on success return nil so callers can correctly report when the old pool failed to close.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/migrations.go`:
- Around line 80-89: Update the exported RunSingleMigration function's docstring
to explicitly state that callers supplying their own *gorm.DB are responsible
for serialization/locking: e.g., "If you call RunSingleMigration directly
(outside MigrateOnFreshConnection callbacks), you must acquire the appropriate
advisory/serialization lock for multi-node safety; the built-in
triggerMigrations path already acquires this lock." Alternatively, if you prefer
API-level enforcement, add an optional parameter (e.g., acquireLock bool or a
LockProvider interface) to RunSingleMigration and implement advisory lock
acquisition when true; reference the RunSingleMigration function and the
triggerMigrations code path when making this change.
---
Duplicate comments:
In `@framework/configstore/postgres.go`:
- Around line 40-50: closeDbConn currently swallows errors; change its signature
from closeDbConn(db *gorm.DB, logger schemas.Logger) to return error (e.g.,
closeDbConn(...) error), have it return the db.DB() or sqlDB.Close() error
(wrapped with context via fmt.Errorf or errors.Join when needed), then update
all call sites (RefreshConnectionPool, refreshPoolFn, migrateOnFreshFn and any
deferred closes) to check and propagate/aggregate the returned error instead of
silently logging—on failure paths join the pool-close error with the primary
error (errors.Join or similar) and on success return nil so callers can
correctly report when the old pool failed to close.
🪄 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: CHILL
Plan: Pro
Run ID: ad683a0d-40a4-4321-acd3-040ea79a3f17
📒 Files selected for processing (13)
framework/configstore/dlock_test.goframework/configstore/encryption.goframework/configstore/encryption_test.goframework/configstore/migrations.goframework/configstore/migrations_test.goframework/configstore/postgres.goframework/configstore/prompts.goframework/configstore/rdb.goframework/configstore/rdb_test.goframework/configstore/sqlite.goframework/configstore/store.goframework/logstore/asyncjob_test.goframework/logstore/postgres.go
✅ Files skipped from review due to trivial changes (2)
- framework/configstore/prompts.go
- framework/configstore/rdb.go
🚧 Files skipped from review as they are similar to previous changes (4)
- framework/configstore/sqlite.go
- framework/configstore/store.go
- framework/logstore/asyncjob_test.go
- framework/configstore/encryption_test.go
18 tasks
18 tasks
18 tasks
Contributor
Author
Merge activity
|
dominictayloruk
pushed a commit
to dominictayloruk/bifrost
that referenced
this pull request
Apr 21, 2026
## Summary Fixes a Postgres error (`ERROR: cached plan must not change result type (SQLSTATE 0A000)`) that occurred when running database migrations on the same connection pool used for regular queries. Migrations are now run on a dedicated, short-lived database connection that is closed after migrations complete, while the main connection pool is created separately afterward. ## Changes - Migrations are now executed on a separate, dedicated database connection rather than the main connection pool. This prevents prepared statement cache conflicts that arise when schema changes (from migrations) invalidate cached query plans on an existing connection. - The migration connection is explicitly closed after migrations finish, regardless of success or failure. - A `RefreshConnectionPool` method has been added to the `ConfigStore` interface and `RDBConfigStore` to support refreshing the connection pool when needed. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Run the service against a Postgres instance where migrations need to be applied and verify that the `cached plan must not change result type` error no longer occurs on startup. ```sh go test ./framework/configstore/... ``` ## Breaking changes - [x] Yes - [ ] No `ConfigStore` interface now includes `RefreshConnectionPool(ctx context.Context) error`. Any external implementations of this interface will need to implement this new method. ## Related issues Closes the issue related to `ERROR: cached plan must not change result type (SQLSTATE 0A000)` during migrations. ## Security considerations No security implications. The migration connection uses the same credentials and SSL configuration as the main pool and is closed immediately after use. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
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
2 participants
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.
Summary
Fixes a Postgres error (
ERROR: cached plan must not change result type (SQLSTATE 0A000)) that occurred when running database migrations on the same connection pool used for regular queries. Migrations are now run on a dedicated, short-lived database connection that is closed after migrations complete, while the main connection pool is created separately afterward.Changes
RefreshConnectionPoolmethod has been added to theConfigStoreinterface andRDBConfigStoreto support refreshing the connection pool when needed.Type of change
Affected areas
How to test
Run the service against a Postgres instance where migrations need to be applied and verify that the
cached plan must not change result typeerror no longer occurs on startup.go test ./framework/configstore/...Breaking changes
ConfigStoreinterface now includesRefreshConnectionPool(ctx context.Context) error. Any external implementations of this interface will need to implement this new method.Related issues
Closes the issue related to
ERROR: cached plan must not change result type (SQLSTATE 0A000)during migrations.Security considerations
No security implications. The migration connection uses the same credentials and SSL configuration as the main pool and is closed immediately after use.
Checklist
docs/contributing/README.mdand followed the guidelines