Graceful stop establishes no happens-before edge: lost server shutdown (nil-check-as-started) + cron Stop() without job join
- 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
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