From 2dfd67b490f13abe8aee67d8f28551d34c14a081 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 9 Nov 2026 22:19:41 +0000 Subject: [PATCH] Tests no longer connect to test servers they closed (closes #257) Four tests closed a test server or stopped discriminator, and then connected to its port. Another test process could take that port in between and receive the connection. - TestUnreachableRouteWatchLetsTheRequestThroughAndSaysWhy and TestUnreachableRoutewatchFailsTheQueryAndTheLogOmitsTheClient, formerly TestRefusedConnection..., point routewatch at 224.0.0.1, a multicast address to which TCP cannot connect. - TestStalledRemoteLogEndpointHoldsUpNoRequest keeps its endpoint open and closes each connection it takes, instead of closing the endpoint. - TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain gives rproxy an address the test holds throughout. The test forwards each connection there to the running discriminator, and closes it while discriminator is stopped. No change to non-test code. Model: opus-5-5 --- internal/discriminator/routewatchtags_test.go | 10 +- internal/routewatch/routewatch_test.go | 9 +- internal/sww/rproxy_test.go | 103 ++++++++++++++++-- internal/sww/sww_test.go | 17 ++- 4 files changed, 120 insertions(+), 19 deletions(-) diff --git a/internal/discriminator/routewatchtags_test.go b/internal/discriminator/routewatchtags_test.go index 33327c9..4b3e41b 100644 --- a/internal/discriminator/routewatchtags_test.go +++ b/internal/discriminator/routewatchtags_test.go @@ -153,14 +153,10 @@ func TestUnsetRouteWatchChangesNothing(t *testing.T) { func TestUnreachableRouteWatchLetsTheRequestThroughAndSaysWhy(t *testing.T) { t.Parallel() - // A stand-in that is closed at once, so that the port refuses connections. - dead := httptest.NewServer(http.NotFoundHandler()) - url := dead.URL - - dead.Close() - + // Another test can take a closed server's port at once, so routewatch's + // address here is a multicast address, to which TCP cannot connect. s, _, _ := startWithClock(t, "", map[string]string{ - lookupSource: off, routeWatchURL: url, + lookupSource: off, routeWatchURL: "http://224.0.0.1", }) wantTagsMissing(t, s.get(fromDE, http.StatusOK, requestlog.ActionForward), diff --git a/internal/routewatch/routewatch_test.go b/internal/routewatch/routewatch_test.go index 60c197a..c74b1af 100644 --- a/internal/routewatch/routewatch_test.go +++ b/internal/routewatch/routewatch_test.go @@ -239,12 +239,17 @@ func TestAnswerRoutewatchCannotBeTakenFromFails(t *testing.T) { } } -func TestRefusedConnectionFailsTheQueryAndTheLogOmitsTheClient(t *testing.T) { +func TestUnreachableRoutewatchFailsTheQueryAndTheLogOmitsTheClient(t *testing.T) { t.Parallel() f := newFixture(t, answering(routedAnswer)) - f.server.Close() + // Another test can take a closed server's port at once, so routewatch's + // address here is a multicast address, to which TCP cannot connect. + f.client = routewatch.New(routewatch.Params{ + URL: "http://224.0.0.1/", Now: f.clk.Now, + ProcessLog: slog.New(slog.NewTextHandler(f.log, nil)), + }) f.route() f.waitFor(routewatch.RequestFailed) diff --git a/internal/sww/rproxy_test.go b/internal/sww/rproxy_test.go index 2e7810d..167e5ae 100644 --- a/internal/sww/rproxy_test.go +++ b/internal/sww/rproxy_test.go @@ -5,7 +5,6 @@ import ( "context" "encoding/pem" "io" - "maps" "net" "net/http" "net/http/httptest" @@ -14,6 +13,7 @@ import ( "path/filepath" "strconv" "strings" + "sync" "sync/atomic" "syscall" "testing" @@ -195,7 +195,13 @@ func TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain(t *testing.T) env := discriminatorSettings(t) addr, _, stop := startDiscriminator(t, env) - site, out := startRProxy(t, "http://"+addr, startApp(t)) + + // rproxy asks discriminator through an address the test holds throughout, + // so no other test can take it while discriminator is stopped. + backend, forwardTo := startForwarder(t) + forwardTo(addr) + + site, out := startRProxy(t, "http://"+backend, startApp(t)) // A visitor who keeps asking throughout, and is never refused. visitorDone := make(chan struct{}) @@ -221,6 +227,10 @@ func TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain(t *testing.T) wantStatus(t, site+bannedProbe, placed, http.StatusForbidden) wantStatus(t, site+"/", placed, http.StatusForbidden) + + // rproxy's connections are closed at once from here, before discriminator + // stops and frees its port. + forwardTo("") stop() // With discriminator down, the site answers, and rproxy says the default did it. @@ -232,12 +242,14 @@ func TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain(t *testing.T) t.Errorf("rproxy's line is %v", line) } - // Back at its address, with its state files, it applies the ban again. - restarted := maps.Clone(env) - restarted[listenAddr] = addr - _, _, stop = startDiscriminator(t, restarted) + // Started again with its state files, it applies the ban again. + addr, _, stop = startDiscriminator(t, env) + forwardTo(addr) - defer stop() + defer func() { + forwardTo("") + stop() + }() wantStatus(t, site+"/", placed, http.StatusForbidden) @@ -249,6 +261,83 @@ func TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain(t *testing.T) } } +// startForwarder listens at a free port until the test ends, and returns its +// address and what sets the address it forwards to. It forwards each +// connection it takes to that address, and closes the connection at once while +// the address is "". +func startForwarder(t *testing.T) (string, func(addr string)) { + t.Helper() + + ctx := t.Context() + + listener, err := (&net.ListenConfig{}).Listen(ctx, "tcp", localhost+":0") + if err != nil { + t.Fatalf("listen: %v", err) + } + + t.Cleanup(func() { + _ = listener.Close() + }) + + var ( + mu sync.Mutex + to string + ) + + // dial connects to the address set, and returns nil while it is "". It + // holds the lock throughout, so once another address is set, nothing + // connects to the one before. + dial := func() net.Conn { + mu.Lock() + defer mu.Unlock() + + if to == "" { + return nil + } + + conn, err := (&net.Dialer{}).DialContext(ctx, "tcp", to) + if err != nil { + return nil + } + + return conn + } + + go func() { + for { + conn, err := listener.Accept() + if err != nil { + return + } + + upstream := dial() + if upstream == nil { + _ = conn.Close() + + continue + } + + go pipe(upstream, conn) + go pipe(conn, upstream) + } + }() + + return listener.Addr().String(), func(addr string) { + mu.Lock() + defer mu.Unlock() + + to = addr + } +} + +// pipe copies src to dst until src ends, then closes both, which ends the copy +// the other way too. +func pipe(dst, src net.Conn) { + _, _ = io.Copy(dst, src) + _ = dst.Close() + _ = src.Close() +} + // answersNormally reports whether a request to url from the client at from, as // X-Forwarded-For names it, gets the app's answer. func answersNormally(ctx context.Context, url, from string) bool { diff --git a/internal/sww/sww_test.go b/internal/sww/sww_test.go index 8638cfa..3087637 100644 --- a/internal/sww/sww_test.go +++ b/internal/sww/sww_test.go @@ -975,9 +975,20 @@ func TestStalledRemoteLogEndpointHoldsUpNoRequest(t *testing.T) { t.Errorf("no line dropped in the metrics:\n%s", metrics) } - // Closed, the endpoint refuses the connection made to send the - // lines still waiting at the stop, which then does not wait. - _ = endpoint.Close() + // From here the endpoint closes each connection it takes, so the + // one made to send the lines still waiting at the stop fails at + // once. Closing the endpoint instead would free its port for + // another test to take. + go func() { + for { + conn, err := endpoint.Accept() + if err != nil { + return + } + + _ = conn.Close() + } + }() }) out.line(t, "type", "request")