fix(container): stop a typed-nil pub/sub client escaping, and stop isNil panicking by aryanmehrotra · Pull Request #4164 · gofr-dev/gofr

aryanmehrotra · 2026-09-08T08:34:32Z

Description:

Two bugs in the same guard, which have to ship together.

1. A typed nil passes != nil. kafka.New and google.New return a typed nil when they reject an incomplete config. Assigned into the PubSub interface, that is not equal to nil — so health.go admitted it and called Health() on a nil receiver.

SQL and Redis beside it already used isNil. This guard was the odd one out. Also filtered in GetSubscriber() and GetPublisher(), so the typed nil cannot escape the container at all. Both, not one: they return the same field, so a handler writing ctx.GetPublisher().Publish(...) hits the identical nil receiver — it just surfaces on a request rather than at startup, and guarding only one leaves the next reader guessing whether the asymmetry was deliberate.

2. isNil itself panicked.

return !val.IsValid() || val.IsNil()   // before

reflect.Value.IsNil panics on a value whose kind cannot be nil (reflect/value.go, the panic(&ValueError{"reflect.Value.IsNil", ...}) at the end of IsNil). App.AddPubSub, App.AddMongo and the rest take an interface, so a caller whose implementation has value receivers can hand over a struct rather than a pointer — ordinary Go, and nothing in the signature discourages it. GoFr writes such implementations itself (sqlMockDB's methods are on a value receiver); it just happens to pass them by address, which is why nothing in-tree hits this today.

That panic took the process down: Container.Close calls isNil, and it is reached from the shutdown goroutine in startShutdownHandler, which has no recover. isNil now switches on Kind first; anything not nillable is present by definition.

What each guard actually prevents

Worth stating precisely, because the two paths differ:

Path	Without the guard
`GetSubscriber` → `App.Subscribe`	process death at startup. The `GetSubscriber() == nil` guard admits the typed nil, the subscription is registered, and `handleSubscription` then calls `Subscribe` on the nil receiver — outside the recover it installs around the handler. `errgroup` does not recover either: `golang.org/x/sync@v0.23.0/errgroup/errgroup.go` documents that it deliberately does not propagate panics from `f()`
`GetPublisher` → a handler	panic on a request
`Health`	not a crash — `runCheck`'s deferred recover catches it. It surfaced as a `"pubsub": DOWN` entry reading `health check panicked`, putting the app into DEGRADED over a dependency it does not have
`Close` (bug 2 only)	process death — no recover in the shutdown goroutine

Breaking Changes (if applicable):

None — and fix 2 is what keeps it that way.

⚠️ Shipped alone, fix 1 would be a breaking change for anyone implementing these interfaces on a value receiver: making the guard stricter is exactly what makes the panic reachable. That is why both are in one PR.


Exported API	unchanged
Health response shape	unchanged
Value-receiver implementations	no longer crash

Additional Information:

No new dependencies.
pubsub_health_nil_test.go covers the typed nil through Health, GetSubscriber and GetPublisher, plus a value-receiver implementation that panicked before. The typed nil is constructed directly rather than by way of google.New rejecting an incomplete config, so the tests do not depend on what that constructor happens to validate.
The health test asserts the pubsub key is absent from the health map, not NotPanics. runCheck already recovers, so a NotPanics assertion passes with or without the guard and proves nothing; the absent key fails when the guard is reverted. Verified by reverting each guard locally: TestHealthOmitsTypedNilPubSub, TestGetSubscriberFiltersTypedNil and TestGetPublisherFiltersTypedNil all fail without it, and TestIsNilHandlesNonNillableKinds fails without the Kind switch.
Cost of the added isNil on the accessors: +2.0 ns/op, 0 B/op, 0 allocs/op. Measured on -benchtime=5000000x -count=5 -cpu=1, Go 1.26.3 / darwin-arm64 (Apple M4): isNil on a real client 2.35 ns/op (0 B, 0 allocs) against 0.33 ns/op (0 B, 0 allocs) for a plain != nil. Typed-nil and unset cases are cheaper still (1.98 / 1.31 ns/op). GetPublisher is the one that can sit on a per-request path, and 2 ns against a network publish is not a trade worth the nil-receiver panic. (An earlier revision of this description quoted +5.5 ns from 6.8 vs 1.3 ns; that figure did not reproduce and is superseded by the numbers above.)
isNil carries //nolint:exhaustive — the default branch is the point, and the repo sets default-signifies-exhaustive: false. Same convention as metrics/exporters/otlp.go.
Out of scope, found while auditing this. PubSub is not the only field reachable as a typed nil — sql.NewSQL returns *sql.DB and redis.NewClient returns *redis.Redis, both of which return nil on a rejected config and are assigned straight into their interface fields. Every guard on them already uses isNil, with one exception: AddDBResolver in external_db.go compares a.container.SQL == nil, so its "Primary SQL connection must be configured" Fatal does not fire when SQL config was rejected. That is the only remaining plain comparison against a datasource field in the repo (grep -rEn '\.(SQL|Redis|PubSub|Mongo|…) *[!=]= *nil', non-test). Not touched here — it is a different call path and belongs in its own change.
Composes with perf(deps): let a build omit the datasource drivers and GraphQL engine it does not use #4167: its pubsub_backends_disabled.go stubs return an untyped nil for createMqttPubSub and leave c.PubSub unset for the other two, which this guard handles unchanged.

Checklist:

I have formatted my code using goimport and golangci-lint.
All new code is covered by unit tests.
This PR does not decrease the overall code coverage.
I have reviewed the code comments and documentation for clarity.

…Nil panicking Two related nil-handling defects in the container. The pub/sub constructors assign the result of google.New or kafka.New straight into Container.PubSub, and those return a TYPED nil when they reject an incomplete config -- a GOOGLE backend with no GOOGLE_PROJECT_ID, for instance. A typed nil inside an interface is not equal to nil, so every plain != nil guard downstream admitted it and then called a method on a nil receiver. Container.Health used such a guard where the SQL and Redis checks beside it already used isNil, so a request to the health endpoint panicked. Worse, App.Subscribe guards on GetSubscriber() == nil and then runs the client in an errgroup goroutine whose only recover wraps the user's handler, so the panic there escaped and killed the process at startup, before any request arrived. Filtering inside GetSubscriber fixes every caller at once. Doing that exposed the second defect. isNil called reflect.Value.IsNil unconditionally, and that PANICS on a value whose kind cannot be nil. A datasource field can legitimately hold one: implementing pubsub.Client, Redis or DB on a value receiver is ordinary Go, and gofr's own tests do it -- with the first fix alone, TestApp_SubscriberInitialize crashed on a struct-valued mock. Close and Health have called isNil on those same fields all along, so this was already reachable; routing GetSubscriber through it only made it easy to hit. isNil now checks the kind first and reports a non-nillable value as present, which it is.

PiyushSingh-ZS

Both bugs are real, and I agree they have to ship together. One thing I think is missing before this lands.

Verified

kafka.New is declared func New(conf *Config, logger pubsub.Logger, metrics Metrics) *kafkaClient and returns a bare nil when validateConfigs fails (pkg/gofr/datasource/pubsub/kafka/kafka.go). Assigned into the pubsub.Client interface that is a non-nil interface holding a nil pointer, so c.PubSub != nil admits it. The premise holds.
reflect.Value.IsNil does panic for Struct, String, Int and Array, so the second bug is reachable for any implementation on a value receiver.
The argument for one PR rather than two is right: fix 1 alone makes the panic in fix 2 newly reachable, because tightening the guard is exactly what routes value-receiver implementations into isNil.

The Kind switch is the correct shape, and "anything not nillable is present by definition" is the right default.

`GetPublisher` was left behind

func (c *Container) GetPublisher() pubsub.Publisher {
	return c.PubSub          // unchanged
}

func (c *Container) GetSubscriber() pubsub.Subscriber {
	if isNil(c.PubSub) {     // fixed
		return nil
	}

	return c.PubSub
}

The reasoning in the new doc comment — "Filtering here fixes all of them at once" — applies identically to the publisher. A handler doing ctx.GetPublisher().Publish(...) against a typed-nil Kafka or Google client hits the same nil receiver; it just surfaces on a request instead of at startup.

Worth fixing in this PR rather than a follow-up, because the asymmetry is now more misleading than it was before: a reader seeing one of the two guarded will reasonably assume the guarded one is the house style and that the other was considered and deliberately left. A test case alongside TestGetSubscriberFiltersTypedNil would pin it.

Nits

assert.True(t, c.GetSubscriber() == nil) with the //nolint:testifylint: the justification is correct and genuinely subtle (a reflective nil check cannot fail here, so require.Nil would pass with or without the filter). My only worry is that it reads as a lint violation to skim past, and someone will "fix" it back. A named helper — assertPlainNil(t, v) — would carry the intent to the next reader without relying on the comment being read.
typedNilPubSubContainer leans on PUBSUB_BACKEND=GOOGLE with no project id producing the typed nil. That is true today but it is an indirect way to construct the state under test, and it breaks if google.New's validation changes. Constructing the typed nil directly would make the test independent of that.

Interaction worth noting

If #4167 lands, its pubsub_backends_disabled.go stubs return an untyped nil for createMqttPubSub and leave c.PubSub unset for the other two, which composes correctly with this guard. Might be worth cross-linking so whoever merges second knows the two were considered together.

… something Review follow-ups on #4164. GetPublisher was left behind. It returns the same field as GetSubscriber, so a handler writing ctx.GetPublisher().Publish(...) against a typed-nil Kafka or Google client hits the identical nil receiver -- it just surfaces on a request instead of at startup. Guarding one and not the other is worse than guarding neither: the next reader has to guess whether the asymmetry was deliberate. TestHealthSurvivesTypedNilPubSub asserted NotPanics and was vacuous. runCheck already recovers (health.go:276), so the panic never escaped with or without the guard -- the test passed either way. What the guard actually prevents is a "pubsub": DOWN entry reading "health check panicked", which put the app into DEGRADED over a dependency it does not have. The test now asserts the pubsub key is absent, which fails when the guard is reverted. The comment above the guard is corrected the same way: the crash is on the GetSubscriber path, where App.Subscribe's errgroup has no recover, not here. The typed nil is now constructed directly rather than by way of google.New rejecting an incomplete GOOGLE config, so the tests no longer depend on what that constructor happens to validate. assertPlainNil replaces the inline //nolint:testifylint, carrying to the next reader why a reflective assert.Nil cannot be used here rather than relying on a comment being read.

Unrelated to this PR: a golang.org/x/net go.mod hash the workspace picked up while the tests were run locally.

aryanmehrotra · 2026-09-18T11:50:16Z

All three addressed at 7c0510846, and chasing the second one turned up a defect in the PR's own test.

GetPublisher is guarded. You're right that the asymmetry was worse than either choice — a reader seeing one guarded would reasonably conclude the other was considered and deliberately left. TestGetPublisherFiltersTypedNil pins it, and fails when the guard is removed.

⚠️ TestHealthSurvivesTypedNilPubSub was vacuous. Adding a test case made me check what the old one actually caught, and the answer is nothing: runCheck already recovers (health.go:276), so the nil-receiver panic never escaped Health with or without the isNil guard, and require.NotPanics passed either way.

What the guard really prevents is this:

"pubsub": {"status":"DOWN","details":{"error":"health check panicked: ..."}}
"status": "DEGRADED"

An app with no usable pub/sub reporting a DOWN dependency and a degraded service, with a stack fragment for a reason. The test now asserts the pubsub key is absent, which is what "no pub/sub configured" has always looked like — and it fails when the guard is reverted.

The PR's claim that this path "took the process down" was overstated, and the comment above the guard is corrected: the crash is on the GetSubscriber path, where App.Subscribe's errgroup has no recover.

Nit 1 — the //nolint is now a named helper, assertPlainNil, carrying the reason (a reflective assert.Nil reports a typed nil as nil, so it would pass with or without the filter) in a doc comment rather than a trailing comment someone skims past.

Nit 2 — the typed nil is constructed directly rather than by way of google.New rejecting an incomplete GOOGLE config, so the tests no longer depend on what that constructor happens to validate. The fake's Health dereferences its receiver for the same reason googleClient.Health does, so the state under test is the real shape.

Cross-link added to #4167 for the pubsub_backends_disabled.go interaction.

The comment said health.go:276, which was never right: the recover is at 271 on development and 282 here. A line number in a comment is wrong the next time anything above it moves, so name the function instead.

Three of them asserted more than the tree supports: - "something GoFr's own tests do" for a struct value in a datasource field. They do not -- sqlMockDB's methods are on a value receiver but every assignment takes its address. The real justification is the public signature: App.AddPubSub and friends take an interface, so a caller can hand over a value, and nothing discourages it. - "took the process down" applied to every isNil caller. Health's does not: runCheck recovers. Close's does, via startShutdownHandler's goroutine, so name that one. - "escaped an errgroup with no recover" without saying where. The panic is in handleSubscription's Subscribe call, outside the recover it installs around the handler; x/sync v0.23.0 errgroup.go documents that it deliberately does not propagate panics from f. Comments only.

akshat-kumar-singhal

Approve with nits. isNil now covers every nillable kind (Chan, Func, Interface, Map, Pointer, Slice, UnsafePointer) with no panic path left, and the other callers (Close, the Health loop, migration's own isNil) behave correctly with it. Restoring container.go/health.go to base fails all four new tests (health reports "health check panicked", both getter tests fail, isNil panics on a struct).

Nit (arguably should-fix): root cause left in place — container.go:572, container.go:589
kafka.New/google.New can still store a typed nil in the exported Container.PubSub, so direct readers (ctx.Container.PubSub, future internal callers) get a non-nil interface wrapping a nil receiver. Suggest only assigning a real client, e.g. if cl := google.New(...); cl != nil { c.PubSub = cl }, keeping the getter filtering as defence in depth.

Nit — container.go:442: ctx.GetPublisher().Publish(...) still panics — now a nil-interface panic instead of a nil-receiver one. The guard helps callers that check != nil; the comment shouldn't imply the panic goes away.

Nit (tests): TestIsNilHandlesNonNillableKinds doesn't cover nil map/slice/chan/func; nothing tests end-to-end that App.Subscribe refuses a typed-nil client (the crash described), and no app-level struct-valued pub/sub mock test was added despite the commit message mentioning TestApp_SubscriberInitialize.

…he getters Review findings from akshat-kumar-singhal on #4164. **The root cause is fixed rather than only filtered.** kafka.New and google.New return a bare nil of their concrete type when they reject a config, and assigning that straight into the pubsub.Client interface is what manufactures the typed nil: a non-nil interface holding a nil pointer, which every `!= nil` check in the codebase admits. Container.PubSub is exported, so a direct reader -- ctx.Container.PubSub, or any future internal caller -- gets no filtering at all. Both constructors now assign only a real client, and the getters stay as defense in depth rather than as the only thing between a misconfiguration and a nil receiver. TestRejectedPubSubConfigLeavesTheExportedFieldNil pins it with a plain == nil comparison, because assert.Nil reflects and would pass on the typed nil. Reverting the google guard fails it with "got a *google.googleClient in the interface". **The GetPublisher comment no longer overclaims.** An unconditional ctx.GetPublisher().Publish(...) still panics, now on a nil interface instead of a nil receiver; what the filter fixes is every caller that does check. The comment says so. **isNil's nillable kinds are now covered exhaustively** -- map, slice, chan, func, interface and unsafe.Pointer alongside the pointer, in both directions. The Kind switch enumerates them, so a kind dropped from it would otherwise fall through to "not nillable, therefore present": the wrong answer, in the safe-looking direction. Verified: gofmt, go vet, typos and go test clean; golangci-lint reports 1 goconst, identical to the merge base. A single TestIntegration_ServerTimeout failure seen while running two packages together did not reproduce -- 4/4 clean afterwards, and 3/3 in isolation.

aryanmehrotra · 2026-09-24T10:49:01Z

Thanks @akshat-kumar-singhal — the root-cause item is taken, in 92e75bfcf. @PiyushSingh-ZS, your GetPublisher point was already addressed at 0a07edb05 (guard plus TestGetPublisherFiltersTypedNil), along with the named assertPlainNil helper you asked for.

Root cause: fixed at the constructors

You were right that filtering at the getters leaves the bad value in an exported field. kafka.New and google.New both return a bare nil of their concrete type on a rejected config, and assigning that into pubsub.Client is what manufactures the typed nil in the first place. Both now assign only a real client, as you suggested.

TestRejectedPubSubConfigLeavesTheExportedFieldNil pins it, and it is load-bearing — reverting just the google guard gives:

--- FAIL: TestRejectedPubSubConfigLeavesTheExportedFieldNil/google_with_no_project_id
    a rejected config must leave PubSub plainly nil, got a *google.googleClient in the interface

The assertion is a plain != nil, not assert.Nil, for the same reason assertPlainNil exists: assert.Nil reflects and would pass on a typed nil, proving nothing.

The GetPublisher comment no longer overclaims

Your reading is correct and the comment now says it outright: an unconditional ctx.GetPublisher().Publish(...) still panics, on a nil interface rather than a nil receiver. What the filter fixes is every caller that does check and whose guard the typed nil used to walk through. With the constructors fixed, the getters are defense in depth rather than the only line of it.

isNil coverage

Extended to every nillable kind in both directions — map, slice, chan, func, interface and unsafe.Pointer alongside the pointer. The reason is in the comment: the Kind switch enumerates them, so a kind dropped from it falls through to "not nillable, therefore present", which is the wrong answer in the safe-looking direction and would not show up as a panic.

Not done, and why

The end-to-end App.Subscribe test and the app-level struct-valued mock both live in package gofr, not container. They are worth having, but a cross-package test of container internals in this PR would widen it past the two bugs it exists to fix — happy to do it as a follow-up, or here if you would rather it not wait. The commit message reference to TestApp_SubscriberInitialize is stale; I will fix that wording.

Verification

gofmt, go vet, typos and go test clean. golangci-lint reports 1 goconst, identical to the merge base — pre-existing.

One honest note: a single TestIntegration_ServerTimeout failure appeared while I was running ./pkg/gofr/ and ./pkg/gofr/container/ in one invocation. It did not reproduce — 4/4 clean on the full package afterwards and 3/3 in isolation — and development passes the same run, so it is load-sensitivity in that timing test rather than anything here.

Umang01-hash

Deep-reviewed and verified end-to-end at head a91b62c1. Both bugs are real and the coupling is right (fix 1 alone makes the isNil panic reachable for value-receiver implementers).

Verified independently:

Not breaking: isNil is unexported; GetPublisher/GetSubscriber/Health signatures unchanged (pre-existing methods); constructors internal.
Constructor guard sound: kafka.New→kafkaClient, google.New→googleClient (concrete pointers), so client != nil catches the nil correctly.
isNil rewrite correct: switches on Kind first, covering exactly the 7 nillable kinds; everything else is present.
6 fail-on-revert tests: reverted both files — each fails reproducing the exact symptoms (health "pubsub": DOWN "health check panicked" → DEGRADED; panic: reflect: call of reflect.Value.IsNil on struct Value). Tests assert honestly (plain == nil, not reflective assert.Nil; health key ABSENT, not NotPanics).
Real app E2E: with a rejected GOOGLE config, development crashes at startup (SIGSEGV in the errgroup goroutine, no recover); this PR starts clean ("subscriber not initialized"). Exactly the claimed process-death path.
gofmt/vet/build/-race clean, golangci-lint 0 issues, coverage 9.0%→9.1%.

The deferred AddDBResolver == nil is correctly left to its own PR. LGTM.

Conflict in pkg/gofr/container/container.go: development (#4164) changed createKafkaPubSub and createGooglePubSub to assign c.PubSub only when the constructor returns a real client, so a rejected config no longer leaves a typed nil in the exported field. This branch had moved both functions to pubsub_backends.go behind the gofr_nopubsub tag, so the conflict is resolved by keeping them there and carrying #4164's guard (and its comments) over to the new location. Re-measured the slim-builds figures at this merge, darwin/arm64, Go 1.26.3: 827 packages by default, 614 / 783 / 807 per tag, 550 with all three; examples/http-server 60,008,402 -> 43,384,642 bytes, 27.7%.

Two conflicts, both where this branch moved code behind a build tag and development edited it in place: - metrics/exporters/otlp.go: development's otel v1.45 signal-path repair (a path-less METRICS_URL posting to "/") lands in otlp_transport.go, where buildOTLPExporter now lives, with httpEndpointWithSignalPath beside it. Its new otlp_signalpath_test.go is tagged !gofr_nootlp like the other transport tests. - container/container.go: #4164's typed-nil guard on the Kafka and Google constructors is carried to pubsub_backends.go. Re-measured the slim-builds counts at this merge, darwin/arm64, Go 1.26.3: 827 default; 614 / 787 / 783 / 807 / 817 / 825 per tag; 411 with all six.

aryanmehrotra force-pushed the fix/pubsub-health-nil-guard branch from 3b07bde to c508146 Compare September 8, 2026 08:50

Merge branch 'development' into fix/pubsub-health-nil-guard

90b0f33

PiyushSingh-ZS reviewed Sep 16, 2026

View reviewed changes

This was referenced Sep 16, 2026

perf(deps): let a build omit the datasource drivers and GraphQL engine it does not use #4167

Merged

perf(deps): let a build omit the gRPC server, Dgraph migrator and OTLP exporters it does not use #4168

Merged

aryanmehrotra mentioned this pull request Sep 17, 2026

feat(tracing): add a trace exporter registry, resource and shutdown flush #4206

Merged

4 tasks done

aryanmehrotra added 2 commits September 18, 2026 15:48

chore: drop the go.work.sum line a local toolchain run added

7c05108

Unrelated to this PR: a golang.org/x/net go.mod hash the workspace picked up while the tests were run locally.

aryanmehrotra added 3 commits September 18, 2026 23:40

Merge remote-tracking branch 'origin/development' into audit4164

1b10c08

Merge remote-tracking branch 'origin/development' into work4164

00275c9

test(container): cite runCheck's recover by name, not by line

21ea192

The comment said health.go:276, which was never right: the recover is at 271 on development and 282 here. A line number in a comment is wrong the next time anything above it moves, so name the function instead.

aryanmehrotra requested a review from Umang01-hash September 23, 2026 10:26

akshat-kumar-singhal approved these changes Sep 24, 2026

View reviewed changes

Merge branch 'development' into fix/pubsub-health-nil-guard

a91b62c

Umang01-hash approved these changes Sep 24, 2026

View reviewed changes

aryanmehrotra added 2 commits September 25, 2026 10:13

Merge branch 'development' into fix/pubsub-health-nil-guard

47dd286

Merge branch 'development' into fix/pubsub-health-nil-guard

cd3c4ce

aryanmehrotra merged commit 2bcc326 into development Sep 25, 2026
19 checks passed

aryanmehrotra deleted the fix/pubsub-health-nil-guard branch September 25, 2026 05:09

Navigation Menu

Repository navigation

fix(container): stop a typed-nil pub/sub client escaping, and stop isNil panicking - #4164

fix(container): stop a typed-nil pub/sub client escaping, and stop isNil panicking#4164
aryanmehrotra merged 12 commits into
developmentfrom
fix/pubsub-health-nil-guard

aryanmehrotra commented Sep 8, 2026 •

edited

Loading

Uh oh!

PiyushSingh-ZS left a comment

Uh oh!

aryanmehrotra commented Sep 18, 2026

Uh oh!

akshat-kumar-singhal left a comment

Uh oh!

aryanmehrotra commented Sep 24, 2026

Uh oh!

Umang01-hash left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

aryanmehrotra commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PiyushSingh-ZS left a comment

Choose a reason for hiding this comment

Verified

GetPublisher was left behind

Nits

Interaction worth noting

Uh oh!

aryanmehrotra commented Sep 18, 2026

Uh oh!

akshat-kumar-singhal left a comment

Choose a reason for hiding this comment

Uh oh!

aryanmehrotra commented Sep 24, 2026

Root cause: fixed at the constructors

The GetPublisher comment no longer overclaims

isNil coverage

Not done, and why

Verification

Uh oh!

Umang01-hash left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aryanmehrotra commented Sep 8, 2026 •

edited

Loading

`GetPublisher` was left behind