Server's fx OnStart hook returns nil as soon as it spawns the serving goroutine, so a failed listen was only ever discovered inside that goroutine. It logged the error and called s.cancelFunc(), which nothing outside the goroutine observes: fx reported RUNNING and the process stayed alive with nothing bound. The service was down and looked up, so systemd Restart=on-failure and Docker restart policies never fired.
Approach
ServerParams now takes fx.Shutdowner. On a listen failure the server logs the error as before and then asks the Shutdowner to stop the app with server.ListenFailureExitCode (1). Shutdown goes through fx's normal stop sequence, so every OnStop hook — the HTTP drain, the delivery engine, the healthcheck, the DB manager, the database close — still runs. The clean-shutdown path itself is untouched.
The context cancel is kept, moved into shutdownOnListenFailure, where it only unwinds serve()'s own wait.
Server.exitCode is removed. Its only writer set it to zero in cleanShutdown and its only reader was serve()'s return value, which Run discards; the process status is fx's to decide. Left in place it would have become a real data race, since the new path makes cleanShutdown and serve()'s return run concurrently under -race.
Regression test
TestListenFailure_ShutsDownTheApp (internal/server/listen_failure_test.go) occupies a kernel-chosen port, starts the wired app on it, and requires a non-zero shutdown signal from app.Wait() within two seconds, then requires the stop sequence to complete. It reuses newTestEnv's wired collaborators rather than stubs, so the Server under test is the one that ships.
It fails without the fix. Reverting only the http.go hunk:
listen_failure_test.go:80: listen failure left the app running
--- FAIL: TestListenFailure_ShutsDownTheApp (2.43s)
FAIL sneak.berlin/go/webhooker/internal/server 2.704s
Definition of done
Item
Evidence
listen failure calls fx.Shutdowner.Shutdown(), process exits non-zero
manual repro below, exit code 1
second start on a bound PORT exits non-zero within a second or so, logging the bind error
280 ms, exit 1, bind error logged
regression test asserts termination rather than RUNNING
TestListenFailure_ShutsDownTheApp, fails without the fix
no change to the clean-shutdown drain
SIGTERM still exits 0 in 20 ms with all 7 OnStop hooks run
Gate evidence
make check — exit 0.
Cache-defeated container build on the rebased head, docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . — exit 0, zero (cached) lines anywhere in the log:
#22 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#22 95.57 0 issues.
#22 DONE 97.7s
#35 [builder 9/11] RUN make test
#35 122.1 --- PASS: TestListenFailure_ShutsDownTheApp (1.22s)
#35 DONE 123.2s
The built image was removed; no containers were started.
Manual repro
Two instances of make build's binary on the same PORT, second one started while the first holds the port:
second instance exit code: 1 in 280 ms
{"level":"ERROR","msg":"listen error","error":"listen tcp :43219: bind: address already in use"}
[Fx] HOOK OnStop ...database.New.func2() ... ran successfully in 186.506µs
The whole stop sequence, including the database close, runs before the non-zero exit. Before this change the same command sat at [Fx] RUNNING indefinitely.
Clean shutdown of a healthy instance, unchanged:
clean shutdown exit code: 0 in 20 ms
OnStop hooks completed: 7
Closes https://git.eeqj.de/sneak/webhooker/issues/200
## Problem
`Server`'s fx `OnStart` hook returns `nil` as soon as it spawns the serving goroutine, so a failed listen was only ever discovered inside that goroutine. It logged the error and called `s.cancelFunc()`, which nothing outside the goroutine observes: fx reported RUNNING and the process stayed alive with nothing bound. The service was down and looked up, so systemd `Restart=on-failure` and Docker restart policies never fired.
## Approach
- `ServerParams` now takes `fx.Shutdowner`. On a listen failure the server logs the error as before and then asks the Shutdowner to stop the app with `server.ListenFailureExitCode` (1). Shutdown goes through fx's normal stop sequence, so every `OnStop` hook — the HTTP drain, the delivery engine, the healthcheck, the DB manager, the database close — still runs. The clean-shutdown path itself is untouched.
- The context cancel is kept, moved into `shutdownOnListenFailure`, where it only unwinds `serve()`'s own wait.
- `Server.exitCode` is removed. Its only writer set it to zero in `cleanShutdown` and its only reader was `serve()`'s return value, which `Run` discards; the process status is fx's to decide. Left in place it would have become a real data race, since the new path makes `cleanShutdown` and `serve()`'s return run concurrently under `-race`.
## Regression test
`TestListenFailure_ShutsDownTheApp` (`internal/server/listen_failure_test.go`) occupies a kernel-chosen port, starts the wired app on it, and requires a non-zero shutdown signal from `app.Wait()` within two seconds, then requires the stop sequence to complete. It reuses `newTestEnv`'s wired collaborators rather than stubs, so the `Server` under test is the one that ships.
It fails without the fix. Reverting only the `http.go` hunk:
```
listen_failure_test.go:80: listen failure left the app running
--- FAIL: TestListenFailure_ShutsDownTheApp (2.43s)
FAIL sneak.berlin/go/webhooker/internal/server 2.704s
```
## Definition of done
| Item | Evidence |
| --- | --- |
| listen failure calls `fx.Shutdowner.Shutdown()`, process exits non-zero | manual repro below, exit code 1 |
| second start on a bound `PORT` exits non-zero within a second or so, logging the bind error | 280 ms, exit 1, bind error logged |
| regression test asserts termination rather than RUNNING | `TestListenFailure_ShutsDownTheApp`, fails without the fix |
| no change to the clean-shutdown drain | SIGTERM still exits 0 in 20 ms with all 7 `OnStop` hooks run |
## Gate evidence
`make check` — exit 0.
Cache-defeated container build on the rebased head, `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — exit 0, zero `(cached)` lines anywhere in the log:
```
#22 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#22 95.57 0 issues.
#22 DONE 97.7s
#35 [builder 9/11] RUN make test
#35 122.1 --- PASS: TestListenFailure_ShutsDownTheApp (1.22s)
#35 DONE 123.2s
```
The built image was removed; no containers were started.
## Manual repro
Two instances of `make build`'s binary on the same `PORT`, second one started while the first holds the port:
```
second instance exit code: 1 in 280 ms
{"level":"ERROR","msg":"listen error","error":"listen tcp :43219: bind: address already in use"}
[Fx] HOOK OnStop ...database.New.func2() ... ran successfully in 186.506µs
```
The whole stop sequence, including the database close, runs before the non-zero exit. Before this change the same command sat at `[Fx] RUNNING` indefinitely.
Clean shutdown of a healthy instance, unchanged:
```
clean shutdown exit code: 0 in 20 ms
OnStop hooks completed: 7
```
`TODO.md` untouched, per https://git.eeqj.de/sneak/webhooker/issues/112.
The fx OnStart hook returned as soon as the serving goroutine was
spawned, so a failed listen was discovered only inside that goroutine.
It logged the error and cancelled the server's own context, which
nothing outside the goroutine observes: fx reported RUNNING and the
process stayed alive with nothing bound. The service was down and
looked up, so systemd Restart=on-failure and Docker restart policies
never fired.
The Server now takes fx.Shutdowner and, on a listen failure, asks it to
stop the app with ListenFailureExitCode. Shutdown runs through fx's
normal stop sequence, so every OnStop hook — including the HTTP drain
and the database close — still executes; the clean-shutdown path is
untouched.
Server.exitCode goes with it. Its only writer set it to zero during
cleanShutdown and its only reader was serve()'s return value, which Run
discards; the process status is fx's to decide. Left in place it would
have been a genuine data race, since the new path makes cleanShutdown
and serve()'s return run concurrently under -race.
Verified by TestListenFailure_ShutsDownTheApp, which occupies a port,
starts the wired app on it, and requires a non-zero shutdown signal
within two seconds. Reverting the fix fails it with "listen failure
left the app running".
PASS — satisfies every item of the definition of done in #200; no defects found.
Disclosure: my independent cache-defeated gate on bfbf416 did not go green overall. make test failed with internal/handlers timing out at 90s (TestFailedLogin_LogLineDoesNotTrackUsernameSize, 14 parallel subtests at ~50s each). Not attributable to this PR: internal/handlers is byte-identical between next and this head and does not import internal/server, and I reproduced the identical timeout on origin/next with no change applied (host load average 122 on 48 cores). Everything the change is responsible for was green — lint 0 issues. in 122.3s, zero (cached) lines, internal/server clean under -race, and reverting only the internal/server/http.go hunk reproduces listen_failure_test.go:84: listen failure left the app running exactly as claimed. Gitea CI on bfbf416 is green.
Forward flag for #201, not a defect here: on this new path fx.App.Run calls os.Exit(1) itself, so a defer in main never runs — whereas the clean-shutdown path exits 0 by returning from main, where a defer would run. A DATA_DIR lock released via defer in main would therefore work on docker stop and silently leak on listen failure; the release has to be an fx OnStop hook.
PASS — satisfies every item of the definition of done in https://git.eeqj.de/sneak/webhooker/issues/200; no defects found.
Disclosure: my independent cache-defeated gate on `bfbf416` did not go green overall. `make test` failed with `internal/handlers` timing out at 90s (`TestFailedLogin_LogLineDoesNotTrackUsernameSize`, 14 parallel subtests at ~50s each). Not attributable to this PR: `internal/handlers` is byte-identical between `next` and this head and does not import `internal/server`, and I reproduced the identical timeout on `origin/next` with no change applied (host load average 122 on 48 cores). Everything the change is responsible for was green — lint `0 issues.` in 122.3s, zero `(cached)` lines, `internal/server` clean under `-race`, and reverting only the `internal/server/http.go` hunk reproduces `listen_failure_test.go:84: listen failure left the app running` exactly as claimed. Gitea CI on `bfbf416` is green.
Forward flag for https://git.eeqj.de/sneak/webhooker/issues/201, not a defect here: on this new path `fx.App.Run` calls `os.Exit(1)` itself, so a `defer` in `main` never runs — whereas the clean-shutdown path exits 0 by returning from `main`, where a `defer` would run. A `DATA_DIR` lock released via `defer` in `main` would therefore work on `docker stop` and silently leak on listen failure; the release has to be an fx `OnStop` hook.
clawbot
merged commit a13e5b7ded into next2026-08-20 06:42:37 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #200
Problem
Server's fxOnStarthook returnsnilas soon as it spawns the serving goroutine, so a failed listen was only ever discovered inside that goroutine. It logged the error and calleds.cancelFunc(), which nothing outside the goroutine observes: fx reported RUNNING and the process stayed alive with nothing bound. The service was down and looked up, so systemdRestart=on-failureand Docker restart policies never fired.Approach
ServerParamsnow takesfx.Shutdowner. On a listen failure the server logs the error as before and then asks the Shutdowner to stop the app withserver.ListenFailureExitCode(1). Shutdown goes through fx's normal stop sequence, so everyOnStophook — the HTTP drain, the delivery engine, the healthcheck, the DB manager, the database close — still runs. The clean-shutdown path itself is untouched.shutdownOnListenFailure, where it only unwindsserve()'s own wait.Server.exitCodeis removed. Its only writer set it to zero incleanShutdownand its only reader wasserve()'s return value, whichRundiscards; the process status is fx's to decide. Left in place it would have become a real data race, since the new path makescleanShutdownandserve()'s return run concurrently under-race.Regression test
TestListenFailure_ShutsDownTheApp(internal/server/listen_failure_test.go) occupies a kernel-chosen port, starts the wired app on it, and requires a non-zero shutdown signal fromapp.Wait()within two seconds, then requires the stop sequence to complete. It reusesnewTestEnv's wired collaborators rather than stubs, so theServerunder test is the one that ships.It fails without the fix. Reverting only the
http.gohunk:Definition of done
fx.Shutdowner.Shutdown(), process exits non-zeroPORTexits non-zero within a second or so, logging the bind errorTestListenFailure_ShutsDownTheApp, fails without the fixOnStophooks runGate evidence
make check— exit 0.Cache-defeated container build on the rebased head,
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .— exit 0, zero(cached)lines anywhere in the log:The built image was removed; no containers were started.
Manual repro
Two instances of
make build's binary on the samePORT, second one started while the first holds the port:The whole stop sequence, including the database close, runs before the non-zero exit. Before this change the same command sat at
[Fx] RUNNINGindefinitely.Clean shutdown of a healthy instance, unchanged:
TODO.mduntouched, per #112.PASS — satisfies every item of the definition of done in #200; no defects found.
Disclosure: my independent cache-defeated gate on
bfbf416did not go green overall.make testfailed withinternal/handlerstiming out at 90s (TestFailedLogin_LogLineDoesNotTrackUsernameSize, 14 parallel subtests at ~50s each). Not attributable to this PR:internal/handlersis byte-identical betweennextand this head and does not importinternal/server, and I reproduced the identical timeout onorigin/nextwith no change applied (host load average 122 on 48 cores). Everything the change is responsible for was green — lint0 issues.in 122.3s, zero(cached)lines,internal/serverclean under-race, and reverting only theinternal/server/http.gohunk reproduceslisten_failure_test.go:84: listen failure left the app runningexactly as claimed. Gitea CI onbfbf416is green.Forward flag for #201, not a defect here: on this new path
fx.App.Runcallsos.Exit(1)itself, so adeferinmainnever runs — whereas the clean-shutdown path exits 0 by returning frommain, where adeferwould run. ADATA_DIRlock released viadeferinmainwould therefore work ondocker stopand silently leak on listen failure; the release has to be an fxOnStophook.