fixing duplicate migration runs for logstore - #4416
Merged
akshaydeo merged 1 commit intoJun 15, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughMigration orchestration in both ChangesMigration Orchestration and DDL/DML Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
akshaydeo
marked this pull request as ready for review
June 15, 2026 16:36
Contributor
Confidence Score: 4/5Safe to merge for correctness; the core migration-deduplication fix is sound and the configstore change is a clean extension to SQLite. The only remaining rough edge is cosmetic debug logging left in logstore's new helper. The root-cause fix is well-targeted: moving the dialect check inside the framework/logstore/migrations.go — the new Important Files Changed
Reviews (1): Last reviewed commit: "fixing duplicate migration runs for logs..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
|
Contributor
Author
Merge activity
|
akshaydeo
added a commit
that referenced
this pull request
Jun 15, 2026
## Summary
Removes noisy, redundant `logger.Info` calls that were logging truncated SQL statement strings immediately before executing those same statements in both `configstore` and `logstore` migration functions. Additionally, removes the Postgres-only guard from `areThereAnyPendingMigrations` in both stores so that the preflight migration check runs for all database dialects (including SQLite), aligning behavior across environments.
## Changes
- Removed ~100 `logger.Info` calls that logged partial, truncated SQL strings (e.g. `"executing ALTER TABLE config_keys ADD COLUMN name VARCHAR(255)\").Error; err != nil { retur"`) immediately before executing those statements — these logs were misleading and added no diagnostic value.
- Removed the `if db.Dialector.Name() == "postgres"` guard from `areThereAnyPendingMigrations` in `configstore/migrations.go`, making the pending-migration preflight check dialect-agnostic and consistent with the fail-open behavior already documented in the comment.
- Extracted `areThereAnyPendingMigrations` in `logstore/migrations.go` as a named function (mirroring the configstore pattern) and removed the Postgres-only guard from `triggerMigrations`, so SQLite deployments also benefit from the preflight skip and double-check-after-lock optimizations.
- Added structured logging in the logstore `areThereAnyPendingMigrations` function to enumerate each pending migration ID.
- Moved the SQLite dialect guard inside the `migrationUpdateTimestampFormat` migrate closure so the migration step is always registered but skips execution on non-SQLite dialects, keeping the migration table consistent across dialects.
## Type of change
- [ ] Bug fix
- [ ] Feature
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs
## How to test
```sh
go test ./framework/configstore/... ./framework/logstore/...
```
Verify that on startup with a fresh SQLite database, migrations run without errors and the migration preflight check correctly skips re-running already-applied steps. Verify the same on Postgres. Confirm that log output no longer contains truncated SQL strings prefixed with `"executing"`.
## Breaking changes
- [ ] Yes
- [x] No
## Security considerations
None. Changes are limited to migration orchestration logic and log verbosity.
## 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 was referenced Jun 16, 2026
Merged
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
Removes noisy, redundant
logger.Infocalls that were logging truncated SQL statement strings immediately before executing those same statements in bothconfigstoreandlogstoremigration functions. Additionally, removes the Postgres-only guard fromareThereAnyPendingMigrationsin both stores so that the preflight migration check runs for all database dialects (including SQLite), aligning behavior across environments.Changes
logger.Infocalls that logged partial, truncated SQL strings (e.g."executing ALTER TABLE config_keys ADD COLUMN name VARCHAR(255)\").Error; err != nil { retur") immediately before executing those statements — these logs were misleading and added no diagnostic value.if db.Dialector.Name() == "postgres"guard fromareThereAnyPendingMigrationsinconfigstore/migrations.go, making the pending-migration preflight check dialect-agnostic and consistent with the fail-open behavior already documented in the comment.areThereAnyPendingMigrationsinlogstore/migrations.goas a named function (mirroring the configstore pattern) and removed the Postgres-only guard fromtriggerMigrations, so SQLite deployments also benefit from the preflight skip and double-check-after-lock optimizations.areThereAnyPendingMigrationsfunction to enumerate each pending migration ID.migrationUpdateTimestampFormatmigrate closure so the migration step is always registered but skips execution on non-SQLite dialects, keeping the migration table consistent across dialects.Type of change
Affected areas
How to test
go test ./framework/configstore/... ./framework/logstore/...Verify that on startup with a fresh SQLite database, migrations run without errors and the migration preflight check correctly skips re-running already-applied steps. Verify the same on Postgres. Confirm that log output no longer contains truncated SQL strings prefixed with
"executing".Breaking changes
Security considerations
None. Changes are limited to migration orchestration logic and log verbosity.
Checklist
docs/contributing/README.mdand followed the guidelines