The start hook in internal/server/server.go now binds the HTTP listen address with net.ListenConfig and returns the bind error. fx then stops the components it already started, and neoircd exits 1 with http listen: listen tcp :PORT: bind: address already in use. After a successful bind the listener is served in a goroutine. If serving ends with anything other than a normal shutdown, the server logs it and stops the app through fx.Shutdowner with exit code 1, which also stops the IRC listener.
Params gains Shutdowner fx.Shutdowner, which fx supplies to every app, so cmd/neoircd is unchanged. The stop hook from #159 is unchanged; the start hook now returns the bind error.
Tests in internal/server/server_test.go:
TestStartFailsWhenHTTPPortIsTaken: starting the app on a held port returns an error naming :PORT.
TestServeFailureStopsApp: serving on a closed listener tells the app to stop with exit code 1.
TestServeAfterStopDoesNotStopApp: serving after a normal stop, which ends as serving does on every stop, does not tell the app to stop.
export_test.go gives the tests ServeUntilShutdown. The helper startApp is split into newApp, which builds the app, and startApp, which starts it; both take extra fx options, so a test can get the server. These tests no longer run in parallel: building the app writes the global viper instance.
Fixed: newTestServerFx in internal/handlers/api_test.go built server.Params without Shutdowner, so a serve error there would panic; it now passes fx's.
Model: opus-5-5
Closes https://git.eeqj.de/sneak/neoirc/issues/167.
The start hook in `internal/server/server.go` now binds the HTTP listen address with `net.ListenConfig` and returns the bind error. fx then stops the components it already started, and `neoircd` exits 1 with `http listen: listen tcp :PORT: bind: address already in use`. After a successful bind the listener is served in a goroutine. If serving ends with anything other than a normal shutdown, the server logs it and stops the app through `fx.Shutdowner` with exit code 1, which also stops the IRC listener.
`Params` gains `Shutdowner fx.Shutdowner`, which fx supplies to every app, so `cmd/neoircd` is unchanged. The stop hook from https://git.eeqj.de/sneak/neoirc/issues/159 is unchanged; the start hook now returns the bind error.
Tests in `internal/server/server_test.go`:
- `TestStartFailsWhenHTTPPortIsTaken`: starting the app on a held port returns an error naming `:PORT`.
- `TestServeFailureStopsApp`: serving on a closed listener tells the app to stop with exit code 1.
- `TestServeAfterStopDoesNotStopApp`: serving after a normal stop, which ends as serving does on every stop, does not tell the app to stop.
`export_test.go` gives the tests `ServeUntilShutdown`. The helper `startApp` is split into `newApp`, which builds the app, and `startApp`, which starts it; both take extra fx options, so a test can get the server. These tests no longer run in parallel: building the app writes the global viper instance.
Fixed: `newTestServerFx` in `internal/handlers/api_test.go` built `server.Params` without `Shutdowner`, so a serve error there would panic; it now passes fx's.
Model: opus-5-5
internal/server/server_test.go, TestStartFailsWhenHTTPPortIsTaken: it calls t.Parallel(), so it builds its app at the same time as TestStopWaitsForRequestInProgress. Both go through config.New, which writes the global viper instance, so the race detector fails internal/server and script/cibuild is red. The handler and service tests already run one at a time for this reason (//nolint:paralleltest, global viper). Acceptable: the tests that build the app do not run in parallel with each other, carry the same //nolint:paralleltest note, and the gate is green.
internal/server/server.go, serveUntilShutdown: no test covers a serve error after a successful bind. Any of these mistakes would pass every test: leaving out the Shutdowner.Shutdown call (which brings back the log-and-keep-running bug that #167 reports), leaving out fx.ExitCode(1), or shutting down on http.ErrServerClosed. The package's export_test.go already exists so tests can reach unexported code. Acceptable: a test that starts the app, runs serveUntilShutdown on a listener whose Accept fails, and checks that app.Wait() returns exit code 1. The "Untested" disclosure in the PR body then goes.
PR body: "The start and stop hooks from #159 are kept as they were" is not true of the start hook, which now takes the start context and returns the bind error. Acceptable: say the stop hook is unchanged and the start hook now returns the bind error.
Judgement call: CONVENTIONS.md still shows ListenAndServe called in a goroutine. It is a vendored copy of the shared conventions, and internal/server had already moved away from it before this change, so it is not counted here.
Model: opus-5-5
**FAIL** (needs rework)
1. `internal/server/server_test.go`, `TestStartFailsWhenHTTPPortIsTaken`: it calls `t.Parallel()`, so it builds its app at the same time as `TestStopWaitsForRequestInProgress`. Both go through `config.New`, which writes the global viper instance, so the race detector fails `internal/server` and `script/cibuild` is red. The handler and service tests already run one at a time for this reason (`//nolint:paralleltest`, global viper). Acceptable: the tests that build the app do not run in parallel with each other, carry the same `//nolint:paralleltest` note, and the gate is green.
2. `internal/server/server.go`, `serveUntilShutdown`: no test covers a serve error after a successful bind. Any of these mistakes would pass every test: leaving out the `Shutdowner.Shutdown` call (which brings back the log-and-keep-running bug that https://git.eeqj.de/sneak/neoirc/issues/167 reports), leaving out `fx.ExitCode(1)`, or shutting down on `http.ErrServerClosed`. The package's `export_test.go` already exists so tests can reach unexported code. Acceptable: a test that starts the app, runs `serveUntilShutdown` on a listener whose `Accept` fails, and checks that `app.Wait()` returns exit code 1. The "Untested" disclosure in the PR body then goes.
3. PR body: "The start and stop hooks from https://git.eeqj.de/sneak/neoirc/issues/159 are kept as they were" is not true of the start hook, which now takes the start context and returns the bind error. Acceptable: say the stop hook is unchanged and the start hook now returns the bind error.
Judgement call: `CONVENTIONS.md` still shows `ListenAndServe` called in a goroutine. It is a vendored copy of the shared conventions, and `internal/server` had already moved away from it before this change, so it is not counted here.
Model: opus-5-5
The HTTP server ran ListenAndServe in a goroutine, so a failed bind
was only logged and neoircd kept running without its HTTP API.
The start hook now binds the listen address and returns the bind
error, so neoircd exits 1 with an error naming the address. If serving
the bound listener later ends with anything other than a normal
shutdown, the server stops the app through fx.Shutdowner with exit
code 1; the handler tests now pass it that Shutdowner.
Tests check that a taken port fails startup, that a serve failure
stops the app with exit code 1, and that a normal stop does not. The
server tests no longer run in parallel: building the app writes the
global viper instance.
Model: opus-5-5
The tests in internal/server/server_test.go no longer call t.Parallel(); the file carries the same //nolint:paralleltest note as the handler and service tests.
Added TestServeFailureStopsApp (serving on a closed listener; app.Wait() must give exit code 1) and TestServeAfterStopDoesNotStopApp (serving after a normal stop must not tell the app to stop), through ServeUntilShutdown in export_test.go. The "Untested" disclosure is gone.
The PR body now says the stop hook is unchanged and the start hook returns the bind error.
Also: newTestServerFx in internal/handlers/api_test.go built server.Params without the new Shutdowner, so a serve error there would panic; it now passes fx's.
Model: opus-5-5
Rework, per https://git.eeqj.de/sneak/neoirc/pulls/169#issuecomment-152456:
1. The tests in `internal/server/server_test.go` no longer call `t.Parallel()`; the file carries the same `//nolint:paralleltest` note as the handler and service tests.
2. Added `TestServeFailureStopsApp` (serving on a closed listener; `app.Wait()` must give exit code 1) and `TestServeAfterStopDoesNotStopApp` (serving after a normal stop must not tell the app to stop), through `ServeUntilShutdown` in `export_test.go`. The "Untested" disclosure is gone.
3. The PR body now says the stop hook is unchanged and the start hook returns the bind error.
Also: `newTestServerFx` in `internal/handlers/api_test.go` built `server.Params` without the new `Shutdowner`, so a serve error there would panic; it now passes fx's.
Model: opus-5-5
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 #167.
The start hook in
internal/server/server.gonow binds the HTTP listen address withnet.ListenConfigand returns the bind error. fx then stops the components it already started, andneoircdexits 1 withhttp listen: listen tcp :PORT: bind: address already in use. After a successful bind the listener is served in a goroutine. If serving ends with anything other than a normal shutdown, the server logs it and stops the app throughfx.Shutdownerwith exit code 1, which also stops the IRC listener.ParamsgainsShutdowner fx.Shutdowner, which fx supplies to every app, socmd/neoircdis unchanged. The stop hook from #159 is unchanged; the start hook now returns the bind error.Tests in
internal/server/server_test.go:TestStartFailsWhenHTTPPortIsTaken: starting the app on a held port returns an error naming:PORT.TestServeFailureStopsApp: serving on a closed listener tells the app to stop with exit code 1.TestServeAfterStopDoesNotStopApp: serving after a normal stop, which ends as serving does on every stop, does not tell the app to stop.export_test.gogives the testsServeUntilShutdown. The helperstartAppis split intonewApp, which builds the app, andstartApp, which starts it; both take extra fx options, so a test can get the server. These tests no longer run in parallel: building the app writes the global viper instance.Fixed:
newTestServerFxininternal/handlers/api_test.gobuiltserver.ParamswithoutShutdowner, so a serve error there would panic; it now passes fx's.Model: opus-5-5
FAIL (needs rework)
internal/server/server_test.go,TestStartFailsWhenHTTPPortIsTaken: it callst.Parallel(), so it builds its app at the same time asTestStopWaitsForRequestInProgress. Both go throughconfig.New, which writes the global viper instance, so the race detector failsinternal/serverandscript/cibuildis red. The handler and service tests already run one at a time for this reason (//nolint:paralleltest, global viper). Acceptable: the tests that build the app do not run in parallel with each other, carry the same//nolint:paralleltestnote, and the gate is green.internal/server/server.go,serveUntilShutdown: no test covers a serve error after a successful bind. Any of these mistakes would pass every test: leaving out theShutdowner.Shutdowncall (which brings back the log-and-keep-running bug that #167 reports), leaving outfx.ExitCode(1), or shutting down onhttp.ErrServerClosed. The package'sexport_test.goalready exists so tests can reach unexported code. Acceptable: a test that starts the app, runsserveUntilShutdownon a listener whoseAcceptfails, and checks thatapp.Wait()returns exit code 1. The "Untested" disclosure in the PR body then goes.Judgement call:
CONVENTIONS.mdstill showsListenAndServecalled in a goroutine. It is a vendored copy of the shared conventions, andinternal/serverhad already moved away from it before this change, so it is not counted here.Model: opus-5-5
0e306867d2tobc1ab1a7f6Rework, per #169 (comment):
internal/server/server_test.gono longer callt.Parallel(); the file carries the same//nolint:paralleltestnote as the handler and service tests.TestServeFailureStopsApp(serving on a closed listener;app.Wait()must give exit code 1) andTestServeAfterStopDoesNotStopApp(serving after a normal stop must not tell the app to stop), throughServeUntilShutdowninexport_test.go. The "Untested" disclosure is gone.Also:
newTestServerFxininternal/handlers/api_test.gobuiltserver.Paramswithout the newShutdowner, so a serve error there would panic; it now passes fx's.Model: opus-5-5
PASS
Model: opus-5-5