Stopping the server left IRC listener sessions in the database: #159
stop in internal/ircserver now waits up to stopTimeout (5 s) for every connection's serve to return, cleanup included, so each session is ended, and its channel peers sent the QUIT, before the database closes. A connection accepted while stop runs is closed instead of served, so nothing joins the wait after it has begun.
Each connection's cleanup now runs on a context that stopping does not cancel. Waiting alone changed nothing: stop cancels the server context first, so the first query in BroadcastQuit failed and nothing was deleted. The new test exposed this.
The HTTP server's stop hook now calls cleanShutdown, which runs httpServer.Shutdown and waits up to shutdownTimeout (5 s) for requests in progress. httpServer is set in the start hook before the serving goroutine starts. The server's own SIGINT/SIGTERM watcher is removed: fx already runs the stop hooks on those signals, and the watcher's shutdown raced them. SIGPIPE is still ignored.
Disclosures:
Deviation: CONVENTIONS.md §4 shows the server watching signals itself; this removes that, for the reason above.
A web client's long poll open at stop holds the HTTP stop until the poll ends or 5 s pass.
No test reaches a connection accepted while stop runs.
Bug elsewhere, unfixed: a failed HTTP listen is only logged, and the process runs on without HTTP.
Model: opus-5-5
Stopping the server left IRC listener sessions in the database: https://git.eeqj.de/sneak/neoirc/issues/159
- `stop` in `internal/ircserver` now waits up to `stopTimeout` (5 s) for every connection's `serve` to return, cleanup included, so each session is ended, and its channel peers sent the QUIT, before the database closes. A connection accepted while `stop` runs is closed instead of served, so nothing joins the wait after it has begun.
- Each connection's cleanup now runs on a context that stopping does not cancel. Waiting alone changed nothing: `stop` cancels the server context first, so the first query in `BroadcastQuit` failed and nothing was deleted. The new test exposed this.
- The HTTP server's stop hook now calls `cleanShutdown`, which runs `httpServer.Shutdown` and waits up to `shutdownTimeout` (5 s) for requests in progress. `httpServer` is set in the start hook before the serving goroutine starts. The server's own SIGINT/SIGTERM watcher is removed: fx already runs the stop hooks on those signals, and the watcher's shutdown raced them. SIGPIPE is still ignored.
Disclosures:
- Deviation: `CONVENTIONS.md` §4 shows the server watching signals itself; this removes that, for the reason above.
- A web client's long poll open at stop holds the HTTP stop until the poll ends or 5 s pass.
- No test reaches a connection accepted while `stop` runs.
- Bug elsewhere, unfixed: a failed HTTP listen is only logged, and the process runs on without HTTP.
Model: opus-5-5
internal/server/server_test.go, TestStopWaitsForRequestInProgress, with Stop in internal/server/export_test.go: the test calls cleanShutdown through a test-only Stop that copies the stop hook's body, and never runs the stop hook that New registers. That hook calling cleanShutdown is the HTTP half of the fix for #159, and it is untested: a stop hook that does nothing, as before the fix, passes every test. Acceptable: the test stops the server through the stop hook New registers (for example with an fxtest app, as the internal/handlers tests already build the server), so a stop hook that does not wait for the request in progress fails it.
Judgement calls:
The CONVENTIONS.md §4 deviation is accepted: fx's Run already runs the stop hooks on SIGINT and SIGTERM, and the server's own watcher would run the HTTP shutdown a second time alongside them.
A web client's long poll open at stop holds the stop for the full shutdownTimeout and logs "server clean shutdown failed" each time; accepted as disclosed, since the issue asks the stop hook to wait.
No test covers a connection accepted while stop runs; that window cannot be reached reliably from a test without a hook added only for it, so it is not raised.
README.md still describes server.go as doing signal handling; left alone, as it still ignores SIGPIPE.
Model: opus-5-5
**FAIL** (needs rework)
1. `internal/server/server_test.go`, `TestStopWaitsForRequestInProgress`, with `Stop` in `internal/server/export_test.go`: the test calls `cleanShutdown` through a test-only `Stop` that copies the stop hook's body, and never runs the stop hook that `New` registers. That hook calling `cleanShutdown` is the HTTP half of the fix for https://git.eeqj.de/sneak/neoirc/issues/159, and it is untested: a stop hook that does nothing, as before the fix, passes every test. Acceptable: the test stops the server through the stop hook `New` registers (for example with an `fxtest` app, as the `internal/handlers` tests already build the server), so a stop hook that does not wait for the request in progress fails it.
Judgement calls:
- The `CONVENTIONS.md` §4 deviation is accepted: fx's `Run` already runs the stop hooks on SIGINT and SIGTERM, and the server's own watcher would run the HTTP shutdown a second time alongside them.
- A web client's long poll open at stop holds the stop for the full `shutdownTimeout` and logs "server clean shutdown failed" each time; accepted as disclosed, since the issue asks the stop hook to wait.
- No test covers a connection accepted while `stop` runs; that window cannot be reached reliably from a test without a hook added only for it, so it is not raised.
- `README.md` still describes `server.go` as doing signal handling; left alone, as it still ignores SIGPIPE.
Model: opus-5-5
TestStopWaitsForRequestInProgress now starts the app with the providers cmd/neoircd uses and stops it through fx, so it runs the stop hook server.New registers, with a long poll in progress that the test releases through the broker. The test-only NewTestHTTPServer, Start and Stop are removed.
Also:
New TestStopReturnsWhileCleanupIsStuck covers the five-second bound on the IRC listener's wait, which no test covered; the stopTimeout comment now says it is the longest wait.
PR body: the long-poll line is reworded, and it now says no test reaches a connection accepted while stop runs.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/neoirc/pulls/166#issuecomment-152187:
1. `TestStopWaitsForRequestInProgress` now starts the app with the providers `cmd/neoircd` uses and stops it through fx, so it runs the stop hook `server.New` registers, with a long poll in progress that the test releases through the broker. The test-only `NewTestHTTPServer`, `Start` and `Stop` are removed.
Also:
- New `TestStopReturnsWhileCleanupIsStuck` covers the five-second bound on the IRC listener's wait, which no test covered; the `stopTimeout` comment now says it is the longest wait.
- PR body: the long-poll line is reworded, and it now says no test reaches a connection accepted while `stop` runs.
Model: opus-5-5
internal/ircserver/server_test.go, TestStopEndsSessions: rebased onto current next, the package's tests do not build, because the test calls db.Database.CountChannelMembers, which the fix for #153 removed from next. The gate fails, and the test that the definition of done asks for does not run. Acceptable: rebase onto current next and count the channel's members with a query next still has (for example GetChannelMemberIDs), with the gate green on the rebased tree.
Judgement calls:
The cleanup's context has no deadline of its own. At stop it is bounded by stopTimeout and then by the database closing, and a normal client disconnect had no deadline before this change either; not raised.
go srv.serveUntilShutdown() in startHTTP is not wrapped in an anonymous function as CODE_STYLEGUIDE_GO.md asks; the line moved unchanged from the old serve, so it is not raised.
Model: opus-5-5
**FAIL** (needs rework)
1. `internal/ircserver/server_test.go`, `TestStopEndsSessions`: rebased onto current `next`, the package's tests do not build, because the test calls `db.Database.CountChannelMembers`, which the fix for https://git.eeqj.de/sneak/neoirc/issues/153 removed from `next`. The gate fails, and the test that the definition of done asks for does not run. Acceptable: rebase onto current `next` and count the channel's members with a query `next` still has (for example `GetChannelMemberIDs`), with the gate green on the rebased tree.
Judgement calls:
- The cleanup's context has no deadline of its own. At stop it is bounded by `stopTimeout` and then by the database closing, and a normal client disconnect had no deadline before this change either; not raised.
- `go srv.serveUntilShutdown()` in `startHTTP` is not wrapped in an anonymous function as `CODE_STYLEGUIDE_GO.md` asks; the line moved unchanged from the old `serve`, so it is not raised.
Model: opus-5-5
Stopping the server returned before the IRC listener's connections had
run their cleanup, and the database closed under them, so their
sessions, clients and channel memberships stayed and their nicks stayed
taken after a restart.
The IRC listener's stop now waits up to five seconds for every
connection's cleanup, which now runs on a context that stopping does
not cancel. The HTTP server's stop hook now runs Shutdown itself and
waits for it; httpServer is set before serving starts, and the server's
own signal watcher, which raced fx's stop hooks, is gone.
Model: opus-5-5
Rebased onto current next. TestStopEndsSessions now reads the channel's members with GetChannelMemberIDs and checks that the peer is the only one left.
Also: the PR body's long-poll line is shortened; nothing else in it changed.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/neoirc/pulls/166#issuecomment-152341:
1. Rebased onto current `next`. `TestStopEndsSessions` now reads the channel's members with `GetChannelMemberIDs` and checks that the peer is the only one left.
Also: the PR body's long-poll line is shortened; nothing else in it changed.
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.
Stopping the server left IRC listener sessions in the database: #159
stopininternal/ircservernow waits up tostopTimeout(5 s) for every connection'sserveto return, cleanup included, so each session is ended, and its channel peers sent the QUIT, before the database closes. A connection accepted whilestopruns is closed instead of served, so nothing joins the wait after it has begun.stopcancels the server context first, so the first query inBroadcastQuitfailed and nothing was deleted. The new test exposed this.cleanShutdown, which runshttpServer.Shutdownand waits up toshutdownTimeout(5 s) for requests in progress.httpServeris set in the start hook before the serving goroutine starts. The server's own SIGINT/SIGTERM watcher is removed: fx already runs the stop hooks on those signals, and the watcher's shutdown raced them. SIGPIPE is still ignored.Disclosures:
CONVENTIONS.md§4 shows the server watching signals itself; this removes that, for the reason above.stopruns.Model: opus-5-5
FAIL (needs rework)
internal/server/server_test.go,TestStopWaitsForRequestInProgress, withStopininternal/server/export_test.go: the test callscleanShutdownthrough a test-onlyStopthat copies the stop hook's body, and never runs the stop hook thatNewregisters. That hook callingcleanShutdownis the HTTP half of the fix for #159, and it is untested: a stop hook that does nothing, as before the fix, passes every test. Acceptable: the test stops the server through the stop hookNewregisters (for example with anfxtestapp, as theinternal/handlerstests already build the server), so a stop hook that does not wait for the request in progress fails it.Judgement calls:
CONVENTIONS.md§4 deviation is accepted: fx'sRunalready runs the stop hooks on SIGINT and SIGTERM, and the server's own watcher would run the HTTP shutdown a second time alongside them.shutdownTimeoutand logs "server clean shutdown failed" each time; accepted as disclosed, since the issue asks the stop hook to wait.stopruns; that window cannot be reached reliably from a test without a hook added only for it, so it is not raised.README.mdstill describesserver.goas doing signal handling; left alone, as it still ignores SIGPIPE.Model: opus-5-5
3f936925b9toe03e3a50c2Rework for #166 (comment):
TestStopWaitsForRequestInProgressnow starts the app with the providerscmd/neoircduses and stops it through fx, so it runs the stop hookserver.Newregisters, with a long poll in progress that the test releases through the broker. The test-onlyNewTestHTTPServer,StartandStopare removed.Also:
TestStopReturnsWhileCleanupIsStuckcovers the five-second bound on the IRC listener's wait, which no test covered; thestopTimeoutcomment now says it is the longest wait.stopruns.Model: opus-5-5
FAIL (needs rework)
internal/ircserver/server_test.go,TestStopEndsSessions: rebased onto currentnext, the package's tests do not build, because the test callsdb.Database.CountChannelMembers, which the fix for #153 removed fromnext. The gate fails, and the test that the definition of done asks for does not run. Acceptable: rebase onto currentnextand count the channel's members with a querynextstill has (for exampleGetChannelMemberIDs), with the gate green on the rebased tree.Judgement calls:
stopTimeoutand then by the database closing, and a normal client disconnect had no deadline before this change either; not raised.go srv.serveUntilShutdown()instartHTTPis not wrapped in an anonymous function asCODE_STYLEGUIDE_GO.mdasks; the line moved unchanged from the oldserve, so it is not raised.Model: opus-5-5
e03e3a50c2to70913dd6bfRework for #166 (comment):
next.TestStopEndsSessionsnow reads the channel's members withGetChannelMemberIDsand checks that the peer is the only one left.Also: the PR body's long-poll line is shortened; nothing else in it changed.
Model: opus-5-5
PASS
Model: opus-5-5