gofr-dev / gofr-dev/gofr

Graceful stop establishes no happens-before edge: lost server shutdown (nil-check-as-started) + cron Stop() without job join

Open
#3,801 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
20.9k
Forks
1.8k
Avg merge
5d 18h
Merged PRs (30d)
39

Description

Follow-up from the review on #3800. Two lifecycle bugs share one shape: **a "stop" that establishes no happens-before edge with the thing it is stopping.** #3800 fixes the memory-safety half (the `-race`-visible data race on `srv`); these two ordering bugs remain and are *not* visible to `-race`.

## 1. Lost shutdown: `srv == nil` doubles as "not started yet"

`run.go` starts the shutdown handler before the servers:

```go
a.startShutdownHandler(ctx, timeout) // parks on <-ctx.Done()
a.startTelemetryIfEnabled()
a.startAllServers(ctx) // servers assign srv in here
```

A termination signal arriving in that window is observed by a `Shutdown()` that sees `srv == nil`, reports success, and returns — then `run()` brings the listener up and serves forever:

```
t0 SIGTERM arrives
t1 shutdown g: Shutdown() -> lock -> srv == nil -> unlock -> return nil ("nothing to stop")
t2 serve g: s.srv = srv
t3 srv.ListenAndServe() <== serves forever
t4 wg.Wait() never returns -> process hangs until SIGKILL
```

Reproduced on the #3800 branch: shutdown returns `nil`, then the listener comes up and stays up. In practice this is a SIGTERM landing right after start (crashloop, fast rollout, failed readiness) where the pod ignores graceful shutdown and hangs until SIGKILL.

Root cause: `srv` is both the server handle and the "has it started?" flag; a nil check carries no happens-before edge, so a mutex fixes the read/write hazard (#3800) but not the ordering. Applies to `httpServer`, `metricServer`, `mcpServer`.

**Fix direction:** a per-server `started chan struct{}` (or a lifecycle state) that `Shutdown` waits on — turning "not started yet" into "wait until started (or until start is known not to happen), then stop" instead of "nothing to do". Must also handle the case where the server errors out before assigning `srv` (cert validation failure, blocked port) so `Shutdown` cannot block forever.

## 2. Cron `Stop()` does not join in-flight jobs (and ignores the shutdown context)

```go
func (c *Crontab) Stop() {
c.once.Do(func() {
c.ticker.Stop()
close(c.done)
})
}
```

`Stop()` halts future ticks but never joins a job that a tick already dispatched — `runScheduled` fires `go j.run(container)` with no `WaitGroup`. So a job dispatched just before `Stop()` keeps running after the caller has moved on. In tests this is the `panic: Fail in goroutine after TestCronTab_AddJob has completed` (a job hits the gomock mock after the test finished); #3800 mitigates it with `defer c.Stop()` + `AnyTimes()`, which narrows the window but cannot close it.

Separately, `App.Shutdown(ctx)` calls `a.cron.Stop()` (gofr.go) but `Stop()` takes **no `context`**, so it cannot honour the shutdown timeout. A naive `wg.Wait()` join would let a hung/long cron job block shutdown indefinitely — which is why this needs a deliberate, context-aware design rather than a drop-in.

**Fix direction:** add a `WaitGroup` around `go j.run(...)`, give `Stop(ctx context.Context)` a context, and join in-flight jobs bounded by that context (wait, else `ListenAndServe`-style force-return on timeout). Wire the shutdown context through from `App.Shutdown`.

## Why one issue

Both are the same defect shape; fixing them together keeps the "graceful stop must establish a happens-before edge with start/dispatch" invariant in one place rather than solving it twice. `-race` points at neither directly — it only flagged the adjacent memory hazard that #3800 closes.

Contributor guide

Open the contributing guide

Research direction

Start with run.go's shutdown and server-start sequence, then inspect httpServer, metricServer, and mcpServer lifecycle handling. Trace Crontab.Stop, runScheduled, and App.Shutdown in gofr.go, and run TestCronTab_AddJob to observe the unfinished-job failure. Done means shutdown ordering is preserved, cron jobs are joined within the shutdown context, and startup errors cannot leave shutdown waiting forever.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.