fix(container): prevent panic on non-positive REMOTE_LOG_FETCH_INTERVAL (#4419)

[Git-rey-08]

Description

• Fixes REMOTE_LOG_FETCH_INTERVAL=0 or negative crashes the app with "non-positive interval for NewTicker" #4419
• Validates in to ensure it is positive (). If negative or zero, it defaults to 15 seconds and logs an error instead of passing an invalid duration.
• In , guards against non-positive intervals by returning immediately after the initial check instead of starting a ticker with an invalid interval (which causes a panic).
• Added unit tests in both and covering zero, negative, and invalid values.

Breaking Changes (if applicable):

None.

Additional Information:

All unit tests pass and code coverage is preserved.

Checklist:

• I have formatted my code using and .
• 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.

[NitinKumar004]

Thanks for picking this up, @Git-rey-08. I checked it at 4733b084 locally, with a real GoFr app built from development and from this branch.

What I verified

The fix does what #4419 asks. Same tiny app with a remote log server, REMOTE_LOG_FETCH_INTERVAL set three ways:

	interval	process	GET /hi	"invalid value" log
development	0	dies (panic: non-positive interval for NewTicker)	–	no
development	-10	dies (same panic)	–	no
this PR	0	up	200	yes
this PR	-10	up	200	yes
both	5	up	200	no

Build, vet and gofmt are clean, and go test -race on pkg/gofr/container passes. The races -race reports in remotelogger are already on development (14 there, 13 here), so they're not from this PR.

Blocking

1. Two new lint findings that the Code Quality job will flag (golangci-lint v2.12.2, same config as CI):

   pkg/gofr/container/container.go:123:3: missing whitespace above this line (too many statements above if) (wsl_v5)
   pkg/gofr/logging/remotelogger/dynamic_level_logger_test.go:704:3: missing whitespace above this line (invalid statement above assign) (wsl_v5)
   

   Both need a blank line: one before isInvalid := ..., and one between w.Header().Set(...) and body := ....

Should fix

2. The new tests can't fail for the bug they're meant to catch.

   • remotelogger.New starts UpdateLogLevel with go l.UpdateLogLevel(), so the NewTicker panic happens on a background goroutine. assert.NotPanics only recovers panics on the test's own goroutine. Both tests return before that goroutine gets to the ticker.
   • I checked this with mutations, running at -count=1 as CI does:
     • Remove the if r.levelFetchInterval <= 0 { return } guard: TestRemoteLogger_UpdateLevel_NonPositiveInterval passes, and the whole remotelogger package passes too (5/5 runs).
     • Remove || levelFetchConfig <= 0 from the container: TestContainer_RemoteLogFetchIntervalValidation passes. The logger guard hides the change, and the test doesn't check the log or the interval.
     • Revert both (i.e. development): the new container test still passes on its own. The package only fails because the leftover goroutine panics later, in the middle of some other test.

   Two deterministic versions that do fail on those mutations. I ran both, including with -race -count=3:

   // dynamic_level_logger_test.go: call UpdateLogLevel synchronously, so a NewTicker panic is on the test goroutine.
   func TestRemoteLogger_UpdateLogLevel_NonPositiveIntervalReturns(t *testing.T) {
   	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
   		_, _ = w.Write([]byte(`{"data":{"serviceName":"test-service","logLevel":"DEBUG"}}`))
   	}))
   	defer srv.Close()
   
   	for _, interval := range []time.Duration{0, -5 * time.Second} {
   		r := &remoteLogger{remoteURL: srv.URL, levelFetchInterval: interval,
   			currentLevel: logging.INFO, Logger: logging.NewLogger(logging.INFO)}
   
   		assert.NotPanics(t, r.UpdateLogLevel)
   		assert.Equal(t, logging.DEBUG, r.currentLevel, "the initial fetch still runs")
   	}
   }

   // container_test.go: assert the fallback actually happened, using an httptest server instead of 127.0.0.1:8080.
   func TestContainer_RemoteLogFetchIntervalInvalidLogsDefault(t *testing.T) {
   	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
   		_, _ = w.Write([]byte(`{"data":{"serviceName":"x","logLevel":"INFO"}}`))
   	}))
   	defer srv.Close()
   
   	for _, interval := range []string{"0", "-10", "invalid"} {
   		out := testutil.StderrOutputForFunc(func() {
   			NewContainer(config.NewMockConfig(map[string]string{
   				"REMOTE_LOG_URL": srv.URL, "REMOTE_LOG_FETCH_INTERVAL": interval,
   			}))
   		})
   
   		assert.Contains(t, out, "invalid value for REMOTE_LOG_FETCH_INTERVAL", "interval %q", interval)
   	}
   }

   mutation	tests in this PR	tests above
   drop the remotelogger <= 0 guard	pass	fail
   drop || levelFetchConfig <= 0 in the container	pass	fail

Smaller notes

• nit: with the guard, remotelogger.New(..., 0) now means "fetch once, never poll". The container no longer passes 0, so only direct callers of New see this, but a one-line comment on the guard would make it clear that it's intentional.
• nit: docs/references/configs could say the value must be a positive number of seconds and that anything else falls back to 15.
• nit: the PR description lost its inline code (e.g. "Validates in to ensure it is positive ()"). Probably the backticks got stripped when pasting.

Requesting changes for the lint findings, since Code Quality will fail on them. With those fixed and the tests tightened, this looks good to me.

[Umang01-hash]

Verified the bug and the fix end-to-end — the two-layer approach is the right shape (container clamps <=0→15s + warns; the UpdateLogLevel guard is the real root-cause fix for every New caller). Two things before this can merge:

1. CI Code Quality (lint) is red — 2 wsl_v5 violations (new issues), please fix:

• container.go:123 — blank line needed above isInvalid := ("too many statements above if").
• dynamic_level_logger_test.go:704 — blank line needed above body := ("invalid statement above assign").

2. The new tests don't actually catch this regression. The panic fires in the goroutine New spawns, after an async fetch — assert.NotPanics can't recover another goroutine's panic, and nothing waits for it. I ran both new tests against the un-fixed source and they pass green, so they wouldn't fail if the guard is reverted. Suggest calling UpdateLogLevel() synchronously with a non-positive interval (it returns immediately with the guard; on the old code NewTicker(0) panics in the calling goroutine, so NotPanics fails-on-revert). That gives a real regression guard.

Logic is correct otherwise — build/vet/fmt clean, no breaking API change, scope is tight.

[Umang01-hash]

wsl_v5 (CI red): "too many statements above if". Add a blank line above this isInvalid := line so only one statement is cuddled with the if.

[Umang01-hash]

wsl_v5 (CI red): "invalid statement above assign". Add a blank line above body :=.

[Umang01-hash]

This doesn't guard the regression: New panics in a spawned goroutine after an async fetch, and assert.NotPanics only catches panics in the calling goroutine (no wait either). I ran this test against the un-fixed source and it passes. Call UpdateLogLevel() synchronously with a non-positive interval instead — it returns via the new guard, and on the old code NewTicker(0) panics in-line so this would fail-on-revert.