feat: add Close() to mongoClient for graceful shutdown#109
feat: add Close() to mongoClient for graceful shutdown#109Prabhjot-Sethi wants to merge 1 commit intomainfrom
Conversation
Add io.Closer implementation to mongoClient so callers can type-assert StoreClient to io.Closer and disconnect the connection pool during graceful shutdown. Without this, the MongoDB driver pool leaks until process exit.
WalkthroughAdded a Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db136b8f80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // connection pool. Implements io.Closer so callers can type-assert | ||
| // StoreClient to io.Closer for graceful shutdown. | ||
| func (c *mongoClient) Close() error { | ||
| return c.client.Disconnect(context.Background()) |
There was a problem hiding this comment.
Close() calls Disconnect(context.Background()), but Mongo driver's Disconnect waits for in-use connections to be returned and only stops waiting when the context is canceled/deadlined. In shutdown paths with a stuck/leaked DB operation, this can block indefinitely and prevent graceful process termination; using a bounded timeout context inside Close() avoids hanging services while still releasing the pool in normal cases.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
db/mongo.go (1)
558-564: Add a timeout toDisconnect()for more robust graceful shutdown.Using
context.Background()without a timeout meansDisconnect()could hang indefinitely if the MongoDB server is unresponsive. According to MongoDB's Go driver v2 documentation, a bounded timeout is the recommended pattern for shutdown—when the deadline expires, the driver force-closes any remaining in-use connections rather than waiting indefinitely.While the current implementation works, adding a timeout improves reliability during application shutdown. The pattern elsewhere in this file (lines 251, 406) uses
context.Background()for stream cleanup within operation contexts, which is different from a publicClose()method called during application termination.Recommended: Add a shutdown timeout
func (c *mongoClient) Close() error { - return c.client.Disconnect(context.Background()) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + return c.client.Disconnect(ctx) }This requires adding
"time"to imports if not already present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/mongo.go` around lines 558 - 564, The Close method currently calls c.client.Disconnect(context.Background()) which can hang; change Close (in mongoClient.Close) to use a context with a bounded timeout (e.g., context.WithTimeout) and pass that to Disconnect, cancelling the context after Disconnect returns; also add the "time" import if missing and ensure proper ctx cancellation (defer cancel()) so the driver force-closes when the deadline expires.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@db/mongo.go`:
- Around line 558-564: The Close method currently calls
c.client.Disconnect(context.Background()) which can hang; change Close (in
mongoClient.Close) to use a context with a bounded timeout (e.g.,
context.WithTimeout) and pass that to Disconnect, cancelling the context after
Disconnect returns; also add the "time" import if missing and ensure proper ctx
cancellation (defer cancel()) so the driver force-closes when the deadline
expires.
1 participant
Summary
Close()method tomongoClientthat callsDisconnect()on the underlying*mongo.Clientio.Closerso callers can type-assertStoreClienttoio.Closerfor graceful shutdownMotivation
Downstream services (e.g., slack-adapter) wrap
StoreClientwith a closeable adapter that needs to disconnect on shutdown. CurrentlyStoreClienthas no close path, forcing services to maintain a parallel raw*mongo.Clientjust for disconnect. This one-line method eliminates that workaround.Test plan
go build ./db/...passesgo vet ./db/...passesSummary by CodeRabbit