fix(mongo): keep the client and register metrics when MongoDB is down at startup by NitinKumar004 · Pull Request #4405 · gofr-dev/gofr

NitinKumar004 · 2026-09-30T17:07:46Z

What

When MongoDB was unreachable at startup, or the config was invalid, Connect returned before creating the database handle and before registering the app_mongo_stats histogram. Every later operation and HealthCheck then dereferenced a nil *mongo.Database and panicked, and nothing ever retried. The hostname/database metric labels and the health details were also always empty, because c.uri/c.database were never assigned.

Changes

Histogram. app_mongo_stats is registered at the start of Connect, before any step that can fail.
Labels and health details. c.uri and c.database are set as soon as the URI is parsed, so both are populated. c.uri holds the host only, never the full URI, which can contain credentials.
Keep the client when the first ping fails. The error is logged and the driver client is kept. The driver connects lazily and reconnects on its own, so operations, the health check and metrics start working once MongoDB is reachable, without restarting the app. Until then, an operation fails after the caller's context deadline or the driver's server-selection timeout (30s by default).
Invalid config. When no driver client can be created, every GoFr method returns a "not connected to MongoDB" error instead of panicking: all CRUD ops, CreateCollection, Drop and StartSession. Tracing, the query log and the histogram are still recorded, as in the ArangoDB fix (fix(arangodb): register metrics and return not-connected errors when Connect fails #4373).
HealthCheck reports DOWN with the reason in details.error, both when not connected and when the ping fails.
Nil results. UpdateByID and UpdateMany no longer dereference a nil result when the driver returns an error, for example a server-selection timeout while MongoDB is down.
Docs. docs/datasources/mongodb/page.md describes the startup-failure behaviour.

Notes

A leak goes away. Previously a failed ping discarded the driver client without calling Disconnect, which leaked its SDAM (topology monitoring) goroutines. Keeping the client removes that leak.
Caveat about the embedded field. Client still embeds *mongo.Database, which is public API and is kept. With an invalid config that field is nil, and driver methods promoted from it (Collection, Client, RunCommand, ...) bypass the new guard. This is documented on the field and in the docs. With a valid config and the server down, the field is now non-nil, so only the invalid-config case remains.

Testing

New and updated tests:
- TestClient_Connect (table): invalid config, and server unreachable (127.0.0.1:1). It replaces Test_NewMongoClient and Test_NewMongoClientError.
- TestClient_NotConnected: every op plus StartSession returns the not-connected error.
- TestClient_ServerUnreachable: every op returns the driver error, and health is DOWN with host/database/error details.
- TestClient_HealthCheck_NotConnected, plus an assertion on details.error in the ping-failure path.
Mutations. Each of these fails the tests:
- moving the histogram back after the ping;
- returning early on a ping failure;
- dropping an op guard, or the HealthCheck guard;
- dropping the c.uri/c.database assignments;
- dropping the nil-result check in UpdateByID;
- dropping the ping-failure details.error.
Gates. Coverage of the mongo module goes from 86.2% to 93.8%. golangci-lint v2.12.2 reports 0 issues, the same as development, and go test -race passes.
Verified against a real mongo:7 container, with the app started while MongoDB was down:
- /insert returned a 500, with no panic.
- Health was DOWN with host, database and error details.
- After starting MongoDB, and without restarting the app, /insert and /count succeeded and health turned UP.
- app_mongo_stats carried hostname="127.0.0.1" and database="e2e".
- With an invalid URI, every call returned the not-connected error, again with no panic.

Fixes #4392

… at startup Connect returned before creating the database handle and before registering the app_mongo_stats histogram whenever the first ping failed (or the config was invalid), and nothing retried. Every later operation and HealthCheck then dereferenced a nil *mongo.Database and panicked. The hostname/database metric labels and health details were also always empty because c.uri/c.database were never set. Register the histogram first, set the labels as soon as the URI parses, and keep the driver client when the ping fails: the driver connects lazily and reconnects on its own, so the app recovers once MongoDB is reachable. With an invalid config every GoFr method returns a "not connected" error instead of panicking, and HealthCheck reports DOWN with the reason in details.error. UpdateByID and UpdateMany no longer dereference a nil result when the driver returns an error. Fixes gofr-dev#4392

Copilot

Copilot review overview

🟡 Changes recommended

UpdateMany discards valid modification counts when the driver returns a result alongside an error.

Review effort: Balanced
Findings: 1

Open (1)

Preserve UpdateMany result counts returned with errors · New

What changed in this PR

Improves MongoDB startup-failure handling so GoFr can recover from outages without restarting.

Changes:

Registers metrics early and retains the driver client after a failed ping.
Guards wrapped operations and adds health-check error details.
Adds failure-path tests and documents recovery behavior.

File	Description
pkg/gofr/datasource/mongo/mongo.go	Adds recovery handling, guards, and populated metric labels.
pkg/gofr/datasource/mongo/mongo_test.go	Tests invalid configuration and unreachable-server behavior.
docs/datasources/mongodb/page.md	Documents startup failures and embedded-driver limitations.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Umang01-hash

Reviewed end-to-end and verified independently against the PR head — approving.

Proven live against a real mongo:7: with the container paused at startup, InsertOne returns an error (no panic) and health is DOWN with host/database/error details; after unpausing, operations recover in the same process without a restart and health turns UP, with app_mongo_stats carrying hostname/database. The headline auto-recovery claim holds.

What I checked myself (not from the description):

Driver semantics (v1.17.10): mongo.Connect is lazy/non-blocking and reconnects via SDAM, so keeping the client after a failed ping genuinely recovers. All 14 ops guard via db(); the UpdateByID/UpdateMany nil-result-on-error derefs are fixed with no sibling left unguarded.
Observability: histogram registered before every early return; done() deferred before the guard so metrics + spans record even while down; labels bounded; no credential leak (host-only c.uri); name/label keys consistent.
Tests are fail-on-revert (re-ran the ping-failure early-return mutation myself → suite goes red), table-driven asserting error identity + Health contents.
Gates: gofmt/vet/build clean, -race passes, golangci-lint 0 new issues, coverage 86.2%→93.8%, no breaking API change, docs updated.

One follow-up worth doing (not blocking): the auto-recovery path (down → up → op succeeds) isn't unit-tested — only the necessary condition (Database stays non-nil) is asserted. I confirmed it via a real-mongo E2E, but an mtest/toxiproxy regression test would keep the headline behavior from silently breaking later.

Nits below are optional.

Umang01-hash · 2026-10-05T09:14:42Z

+	}
+}
+
+func TestClient_NotConnected(t *testing.T) {


The advertised auto-recovery (server down at Connect → later reachable → op succeeds in the same process) isn't unit-tested here — only the necessary condition (Database stays non-nil after a failed ping) is asserted. I verified the full recovery against a real mongo:7, but a regression test (mtest or a toxiproxy-gated server) would guard the headline behavior in CI. Not blocking.

Agreed, that's the gap. The unit tests only check what recovery depends on: the client is kept, Database is non-nil, and operations return errors instead of panicking. The actual down-then-up recovery isn't covered. mtest's mock deployment can't simulate a server coming back, so a real test needs a toxiproxy- or container-backed server. I'd like to do that as a separate change rather than grow this PR. Thanks for running the pause/unpause check against a real mongo:7.

Umang01-hash · 2026-10-05T09:14:42Z

+	// client, which does not happen when the config is invalid. GoFr's wrapped methods return errNotConnected in
+	// that case, but driver methods promoted from this field (Collection, Client, RunCommand, ...) bypass that
+	// guard and must not be called while it is nil.
 	*mongo.Database


Nicely documented. Just confirming the sharp edge is understood: with an invalid config this stays nil and promoted methods (Collection/RunCommand/Client) bypass db() and panic. Not reachable via the wrapped API, so fine as-is.

Yes, that's intended. With an invalid config Database stays nil, and only the wrapped methods guard it through db(). The godoc on the field says to use the wrapped API, and the docs page notes that the embedded driver handles aren't usable until the client is configured.

…it with an error On a write error mongo-driver still returns an UpdateResult for UpdateMany (processWriteError maps WriteCommandError to rrMany), so a write-concern timeout reports how many documents were modified. The nil-result guard returned 0 for every error and dropped that count. Return the count whenever a result exists, and 0 only when the driver returns none, which is the case for UpdateByID on a write error.

Umang01-hash

Verified the fix end-to-end with a real MongoDB (Docker): down at startup → no panic, bounded failure, health DOWN with the dial error in details; then bringing Mongo up → the same client recovers on the first retry with no restart and health flips UP. Exactly the intended behavior, and a clear improvement over the previous nil-panic.

One operational note, not blocking: with Mongo unreachable and no REQUEST_TIMEOUT set, a handler op blocks on the driver's server-selection timeout (~30s) — I saw 50 no-deadline ops hold 50 goroutines until cancel. It's fully mitigated by REQUEST_TIMEOUT (the driver honors ctx-cancel instantly), but might be worth a line in the docs change, or exposing the driver's ServerSelectionTimeout. Your call.

NitinKumar004 and others added 2 commits September 30, 2026 22:37

Merge branch 'development' into fix/mongo-connect-failure

06a1540

Umang01-hash requested a balanced review from Copilot October 5, 2026 08:56

Copilot started reviewing on behalf of Umang01-hash October 5, 2026 08:56 View session

Copilot AI reviewed Oct 5, 2026

View reviewed changes

Comment thread pkg/gofr/datasource/mongo/mongo.go Outdated

Umang01-hash previously approved these changes Oct 5, 2026

View reviewed changes

NitinKumar004 dismissed Umang01-hash’s stale review via 6993aa7 October 5, 2026 15:14

Umang01-hash reviewed Oct 6, 2026

View reviewed changes

Navigation Menu

Repository navigation

fix(mongo): keep the client and register metrics when MongoDB is down at startup - #4405

fix(mongo): keep the client and register metrics when MongoDB is down at startup#4405
NitinKumar004 wants to merge 3 commits into
gofr-dev:developmentfrom
NitinKumar004:fix/mongo-connect-failure

NitinKumar004 commented Sep 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Umang01-hash left a comment

Uh oh!

Umang01-hash Oct 5, 2026

Uh oh!

NitinKumar004 Oct 5, 2026

Uh oh!

Umang01-hash Oct 5, 2026

Uh oh!

NitinKumar004 Oct 5, 2026

Uh oh!

Umang01-hash left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

NitinKumar004 commented Sep 30, 2026

What

Changes

Notes

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Umang01-hash left a comment

Choose a reason for hiding this comment

Uh oh!

Umang01-hash Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

NitinKumar004 Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

Umang01-hash Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

NitinKumar004 Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

Umang01-hash left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants