feat: add ConnectWithRetry function for database connections with retry logic#178
Conversation
Greptile SummaryThis PR introduces a generic
Confidence Score: 5/5Safe to merge — the changes fix two real pre-existing bugs and add well-guarded retry logic with no new defects introduced. The retry loop is correctly implemented, the mongo client leak on Ping failure is properly closed, and the silent nil-service bug in both init files is resolved by switching to panic. No new error paths are introduced that could leave the service in a broken state. No files require special attention.
|
| Filename | Overview |
|---|---|
| pkg/db/utils.go | Adds ConnectWithRetry generic function with proper maxAttempts guard, per-attempt error logging, and skip-sleep-on-last-attempt logic. Clean implementation. |
| pkg/db/global-infos/db.go | Adds Disconnect call on Ping failure to fix a resource leak where the mongo client was abandoned without cleanup. |
| pkg/db/management-user/db.go | Same Disconnect-on-Ping-failure fix as the other DB constructors. |
| pkg/db/messaging/db.go | Same Disconnect-on-Ping-failure fix as the other DB constructors. |
| pkg/db/participant-user/db.go | Same Disconnect-on-Ping-failure fix as the other DB constructors. |
| pkg/db/study/db.go | Same Disconnect-on-Ping-failure fix as the other DB constructors. |
| services/management-api/init.go | Wraps all DB connections with ConnectWithRetry; changes silent return on ParticipantUserDB/GlobalInfosDB failure to panic, making startup consistently fail-fast on any DB connection error. |
| services/participant-api/init.go | Same ConnectWithRetry adoption and return-to-panic fix as management-api/init.go. |
Sequence Diagram
sequenceDiagram
participant Init as initDBs()
participant CWR as ConnectWithRetry
participant Ctor as NewXxxDBService
participant Mongo as MongoDB
Init->>CWR: ConnectWithRetry(label, 10, 10s, func)
loop attempt 1..maxAttempts
CWR->>Ctor: connect()
Ctor->>Mongo: mongo.Connect()
Ctor->>Mongo: Ping()
alt Ping success
Mongo-->>Ctor: OK
Ctor-->>CWR: (service, nil)
CWR-->>Init: (service, nil)
else Ping failure
Mongo-->>Ctor: error
Ctor->>Mongo: Disconnect(context.Background())
Ctor-->>CWR: (nil, err)
CWR->>CWR: slog.Error(attempt, maxAttempts, err)
alt "attempt < maxAttempts"
CWR->>CWR: time.Sleep(retryDelay)
end
end
end
alt all attempts exhausted
CWR-->>Init: (nil, lastErr)
Init->>Init: slog.Error + panic(err)
end
Reviews (3): Last reviewed commit: "fix: ensure database disconnection on er..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| muDBService, err = db.ConnectWithRetry("Management User DB", dbConnectMaxAttempts, dbConnectRetryDelay, func() (*muDB.ManagementUserDBService, error) { | ||
| return muDB.NewManagementUserDBService(db.DBConfigFromYamlObj(conf.DBConfigs.ManagementUserDB, conf.AllowedInstanceIDs)) | ||
| }) | ||
| if err != nil { | ||
| slog.Error("Error connecting to Management User DB", slog.String("error", err.Error())) | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
ConnectWithRetry already logs an error message (with dbLabel, attempt count, and the error string) for every failed attempt including the last one. The slog.Error call in the caller immediately before panic(err) duplicates that final log entry, producing two nearly-identical error lines for the same failure. The same pattern appears for all five connections in this file and the four in participant-api/init.go.
| muDBService, err = db.ConnectWithRetry("Management User DB", dbConnectMaxAttempts, dbConnectRetryDelay, func() (*muDB.ManagementUserDBService, error) { | |
| return muDB.NewManagementUserDBService(db.DBConfigFromYamlObj(conf.DBConfigs.ManagementUserDB, conf.AllowedInstanceIDs)) | |
| }) | |
| if err != nil { | |
| slog.Error("Error connecting to Management User DB", slog.String("error", err.Error())) | |
| panic(err) | |
| } | |
| muDBService, err = db.ConnectWithRetry("Management User DB", dbConnectMaxAttempts, dbConnectRetryDelay, func() (*muDB.ManagementUserDBService, error) { | |
| return muDB.NewManagementUserDBService(db.DBConfigFromYamlObj(conf.DBConfigs.ManagementUserDB, conf.AllowedInstanceIDs)) | |
| }) | |
| if err != nil { | |
| panic(err) | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/management-api/init.go
Line: 126-132
Comment:
`ConnectWithRetry` already logs an error message (with `dbLabel`, attempt count, and the error string) for every failed attempt including the last one. The `slog.Error` call in the caller immediately before `panic(err)` duplicates that final log entry, producing two nearly-identical error lines for the same failure. The same pattern appears for all five connections in this file and the four in `participant-api/init.go`.
```suggestion
muDBService, err = db.ConnectWithRetry("Management User DB", dbConnectMaxAttempts, dbConnectRetryDelay, func() (*muDB.ManagementUserDBService, error) {
return muDB.NewManagementUserDBService(db.DBConfigFromYamlObj(conf.DBConfigs.ManagementUserDB, conf.AllowedInstanceIDs))
})
if err != nil {
panic(err)
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Warning Review limit reached
More reviews will be available in 35 minutes and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughA generic retry helper ConnectWithRetry is added in ChangesDatabase Connection Retry
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/management-api/init.go (1)
55-58: ⚡ Quick winCentralize retry configuration constants.
The retry constants
dbConnectMaxAttemptsanddbConnectRetryDelayare duplicated across both service initialization files. Consider moving them topkg/db/utils.goas package-level constants or exports to establish a single source of truth and prevent configuration drift.♻️ Proposed refactor
In
pkg/db/utils.go:package db import ( "context" "errors" "log/slog" "time" "go.mongodb.org/mongo-driver/bson" "go.mongodb.org/mongo-driver/mongo" ) +const ( + DefaultConnectMaxAttempts = 10 + DefaultConnectRetryDelay = 10 * time.Second +) + func ListCollectionIndexes(ctx context.Context, collection *mongo.Collection) ([]bson.M, error) {Then in both init files, replace the local constants with
db.DefaultConnectMaxAttemptsanddb.DefaultConnectRetryDelay.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/management-api/init.go` around lines 55 - 58, The two local retry constants dbConnectMaxAttempts and dbConnectRetryDelay are duplicated; move them into pkg/db/utils.go as exported package-level constants (e.g., DefaultConnectMaxAttempts int = 10 and DefaultConnectRetryDelay = 10 * time.Second) and update the references in services/management-api init (and the other init file) to use db.DefaultConnectMaxAttempts and db.DefaultConnectRetryDelay; ensure the exported names and types match usages (int and time.Duration) and update imports to include the pkg/db package.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@services/management-api/init.go`:
- Around line 55-58: The two local retry constants dbConnectMaxAttempts and
dbConnectRetryDelay are duplicated; move them into pkg/db/utils.go as exported
package-level constants (e.g., DefaultConnectMaxAttempts int = 10 and
DefaultConnectRetryDelay = 10 * time.Second) and update the references in
services/management-api init (and the other init file) to use
db.DefaultConnectMaxAttempts and db.DefaultConnectRetryDelay; ensure the
exported names and types match usages (int and time.Duration) and update imports
to include the pkg/db package.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 276fbe54-b660-4491-99db-6268dd8f39db
📒 Files selected for processing (3)
pkg/db/utils.goservices/management-api/init.goservices/participant-api/init.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
services/management-api/init.go (1)
121-159: 💤 Low valueDuplicate logging: slog.Error calls before panic are redundant (affects all DB connections in both services).
db.ConnectWithRetryalready logs each connection failure with structured details (db name, attempt number, maxAttempts, error) at each retry. The additionalslog.Errorcalls inservices/management-api/init.go(lines 125, 133, 141, 149, 157) andservices/participant-api/init.go(lines 315, 323, 331, 339) before eachpanic(err)create duplicate log entries for the same error.Since the panic stack trace will also include the error message, these explicit logging calls could be removed without losing diagnostic information. This is a minor maintainability improvement to reduce log noise.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/management-api/init.go` around lines 121 - 159, Remove the redundant slog.Error calls that immediately precede panics after db.ConnectWithRetry fails for each DB init (muDBService, messagingDBService, studyDBService, participantUserDBService, globalInfosDBService); db.ConnectWithRetry already logs structured retry/failure details, and the panic(err) will surface the error, so delete the slog.Error(...) lines in the initialization blocks that call db.ConnectWithRetry to avoid duplicate log entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/management-api/init.go`:
- Around line 121-127: The New*DBService constructors (e.g.,
NewManagementUserDBService, and the equivalents in pkg/db/messaging/db.go,
pkg/db/study/db.go, pkg/db/participant-user/db.go, pkg/db/global-infos/db.go)
currently call mongo.Connect and then return an error when dbClient.Ping fails
but do not call dbClient.Disconnect, leaking clients across retry attempts;
update each constructor so that if Ping (or any subsequent client initialization
step) returns an error you call dbClient.Disconnect(context.Background()) before
returning the error, ensuring the partially-created mongo client is cleaned up
on all failure paths.
---
Nitpick comments:
In `@services/management-api/init.go`:
- Around line 121-159: Remove the redundant slog.Error calls that immediately
precede panics after db.ConnectWithRetry fails for each DB init (muDBService,
messagingDBService, studyDBService, participantUserDBService,
globalInfosDBService); db.ConnectWithRetry already logs structured retry/failure
details, and the panic(err) will surface the error, so delete the
slog.Error(...) lines in the initialization blocks that call db.ConnectWithRetry
to avoid duplicate log entries.
🪄 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: f06bcf0d-a4e9-49cc-8dc0-9dfe2a58add4
📒 Files selected for processing (3)
pkg/db/utils.goservices/management-api/init.goservices/participant-api/init.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/db/utils.go
1 participant
Summary by CodeRabbit
New Features
Refactor