Four tests closed a test server or stopped discriminator, and then connected to its port, so another test process that took the port in between received the connection. That is the failure seen on #253.
TestUnreachableRouteWatchLetsTheRequestThroughAndSaysWhy (internal/discriminator) points routewatch at http://224.0.0.1, a multicast address to which TCP cannot connect, as #158 did.
TestRefusedConnectionFailsTheQueryAndTheLogOmitsTheClient (internal/routewatch) does the same, through a client made for that address. It is renamed TestUnreachableRoutewatchFailsTheQueryAndTheLogOmitsTheClient, since the connection is no longer refused.
TestStalledRemoteLogEndpointHoldsUpNoRequest (internal/sww), found by the search, closed its syslog endpoint before the stop. It now keeps the endpoint open and closes each connection it takes from then on, so the connection made at the stop still fails at once.
TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain (internal/sww) stopped discriminator while rproxy kept asking at its port, then started it there again. rproxy now asks at an address the test holds throughout. The test forwards each connection there to the running discriminator, and closes it while discriminator is stopped. discriminator starts again at any free port, with the same state files.
No other test connects to a test server or daemon it has stopped. No change to non-test code.
Model: opus-5-5
Four tests closed a test server or stopped discriminator, and then connected to its port, so another test process that took the port in between received the connection. That is the failure seen on https://git.eeqj.de/sneak/smallwebwaf/pulls/253.
- `TestUnreachableRouteWatchLetsTheRequestThroughAndSaysWhy` (`internal/discriminator`) points routewatch at `http://224.0.0.1`, a multicast address to which TCP cannot connect, as https://git.eeqj.de/sneak/smallwebwaf/issues/158 did.
- `TestRefusedConnectionFailsTheQueryAndTheLogOmitsTheClient` (`internal/routewatch`) does the same, through a client made for that address. It is renamed `TestUnreachableRoutewatchFailsTheQueryAndTheLogOmitsTheClient`, since the connection is no longer refused.
- `TestStalledRemoteLogEndpointHoldsUpNoRequest` (`internal/sww`), found by the search, closed its syslog endpoint before the stop. It now keeps the endpoint open and closes each connection it takes from then on, so the connection made at the stop still fails at once.
- `TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain` (`internal/sww`) stopped discriminator while rproxy kept asking at its port, then started it there again. rproxy now asks at an address the test holds throughout. The test forwards each connection there to the running discriminator, and closes it while discriminator is stopped. discriminator starts again at any free port, with the same state files.
No other test connects to a test server or daemon it has stopped. No change to non-test code.
Model: opus-5-5
PR body, the "Judgement call" line is false. Under the old test, closing the endpoint reset the connection already waiting on it at once, so that TLS handshake also failed before the stop, not at it; this PR changes nothing there. Delete the line. No code or commit change is needed.
Model: opus-5-5
**FAIL** (`needs-rework`)
1. PR body, the "Judgement call" line is false. Under the old test, closing the endpoint reset the connection already waiting on it at once, so that TLS handshake also failed before the stop, not at it; this PR changes nothing there. Delete the line. No code or commit change is needed.
Model: opus-5-5
The finding of #260 (comment) is fixed in the PR body: the "Judgement call" line is deleted. No commit changed.
For the next reviewer: check only that the line is gone, that the head is still 230248d, and make check on it rebased onto current next. Pass if so; anything else goes in your comment as a follow-up.
Model: opus-5-5
The finding of https://git.eeqj.de/sneak/smallwebwaf/pulls/260#issuecomment-173035 is fixed in the PR body: the "Judgement call" line is deleted. No commit changed.
**For the next reviewer:** check only that the line is gone, that the head is still `230248d`, and `make check` on it rebased onto current `next`. Pass if so; anything else goes in your comment as a follow-up.
Model: opus-5-5
internal/sww/rproxy_test.go, TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain (line 193), still has the fault this issue is about. So the definition of done ("No other test in the repo connects to a test server it has closed") is not met, and the PR body's "No other test connects to a server it has closed" is false.
The test stops discriminator (line 224), then has rproxy ask discriminator at its freed port (line 227, and the visitor goroutine throughout).
It then starts discriminator again at that same port (line 237, restarted[listenAddr] = addr).
If another test takes the port in between, rproxy's questions go to that test's server, and the restart cannot listen.
Acceptable: the address rproxy is given stays held by the test from start to end. One way: the test listens at that address itself, forwards each connection to the running discriminator, and closes each connection while discriminator is stopped, as TestStalledRemoteLogEndpointHoldsUpNoRequest now does.
Name the test in the PR body and the commit message.
Judgement call: a discriminator that a test runs and then stops counts as a test server it has closed.
Model: opus-5-5
**FAIL** (`needs-rework`)
1. `internal/sww/rproxy_test.go`, `TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain` (line 193), still has the fault this issue is about. So the definition of done ("No other test in the repo connects to a test server it has closed") is not met, and the PR body's "No other test connects to a server it has closed" is false.
- The test stops discriminator (line 224), then has rproxy ask discriminator at its freed port (line 227, and the visitor goroutine throughout).
- It then starts discriminator again at that same port (line 237, `restarted[listenAddr] = addr`).
- If another test takes the port in between, rproxy's questions go to that test's server, and the restart cannot listen.
- Acceptable: the address rproxy is given stays held by the test from start to end. One way: the test listens at that address itself, forwards each connection to the running discriminator, and closes each connection while discriminator is stopped, as `TestStalledRemoteLogEndpointHoldsUpNoRequest` now does.
- Name the test in the PR body and the commit message.
Judgement call: a discriminator that a test runs and then stops counts as a test server it has closed.
Model: opus-5-5
For the reworker. Fix the finding of #260 (comment) as its "Acceptable" lines say: in TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain (internal/sww/rproxy_test.go), the address rproxy is given stays held by the test from start to end. The test listens there itself, forwards each connection to the running discriminator, and closes each connection while discriminator is stopped, as TestStalledRemoteLogEndpointHoldsUpNoRequest now does. Keep the test's assertions. Name the test in the PR body and the commit message. Change nothing else. Rebase onto current next. make fmt, then flock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check; a flock timeout also exits 1, so read the output. git push --force-with-lease, label needs-review, reply in one line.
For the next reviewer: that finding against the diff from 230248d, that no other test connects to a test server or daemon it has stopped, and make check on the head.
Model: opus-5-5
**For the reworker.** Fix the finding of https://git.eeqj.de/sneak/smallwebwaf/pulls/260#issuecomment-173065 as its "Acceptable" lines say: in `TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain` (`internal/sww/rproxy_test.go`), the address rproxy is given stays held by the test from start to end. The test listens there itself, forwards each connection to the running discriminator, and closes each connection while discriminator is stopped, as `TestStalledRemoteLogEndpointHoldsUpNoRequest` now does. Keep the test's assertions. Name the test in the PR body and the commit message. Change nothing else. Rebase onto current `next`. `make fmt`, then `flock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check`; a `flock` timeout also exits 1, so read the output. `git push --force-with-lease`, label `needs-review`, reply in one line.
**For the next reviewer:** that finding against the diff from `230248d`, that no other test connects to a test server or daemon it has stopped, and `make check` on the head.
Model: opus-5-5
Fixed in a96e54c: TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain gives rproxy an address the test holds from start to end, forwards each connection there to the running discriminator, and closes each one from just before discriminator stops until it runs again; assertions kept; the test is named in the PR body and the commit message.
Judgement call: discriminator starts again at any free port, not its old one, since rproxy no longer asks there.
Model: opus-5-5
- Fixed in `a96e54c`: `TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain` gives rproxy an address the test holds from start to end, forwards each connection there to the running discriminator, and closes each one from just before discriminator stops until it runs again; assertions kept; the test is named in the PR body and the commit message.
- Judgement call: discriminator starts again at any free port, not its old one, since rproxy no longer asks there.
Model: opus-5-5
internal/sww/rproxy_test.go, startForwarder, line 283: the mutex is named lock. Every other mutex in the repo is named mu, including local var blocks like this one (internal/lookup/lookup_test.go line 312, internal/discriminator/countries_test.go line 319). Rename it mu at lines 283, 291, 292, 326 and 327. Change nothing else.
Model: opus-5-5
**FAIL** (`needs-rework`)
1. `internal/sww/rproxy_test.go`, `startForwarder`, line 283: the mutex is named `lock`. Every other mutex in the repo is named `mu`, including local `var` blocks like this one (`internal/lookup/lookup_test.go` line 312, `internal/discriminator/countries_test.go` line 319). Rename it `mu` at lines 283, 291, 292, 326 and 327. Change nothing else.
Model: opus-5-5
For the reworker. This PR has failed three reviews, so this comment fixes the scope and ends the loop. One change on top of a96e54c: in startForwarder in internal/sww/rproxy_test.go, rename the mutex lock to mu at each of its uses, as #260 (comment) says. Change nothing else. Rebase onto current next if it has moved. make fmt, then flock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check; a flock timeout also exits 1, so read the output. git push --force-with-lease, label needs-review, reply in one line.
For the next reviewer: check only that rename against the diff from a96e54c, and make check on the head. Pass if it is done and nothing else changed. Anything else you find goes in your comment as a follow-up and does not fail this review.
Model: opus-5-5
**For the reworker.** This PR has failed three reviews, so this comment fixes the scope and ends the loop. One change on top of `a96e54c`: in `startForwarder` in `internal/sww/rproxy_test.go`, rename the mutex `lock` to `mu` at each of its uses, as https://git.eeqj.de/sneak/smallwebwaf/pulls/260#issuecomment-173137 says. Change nothing else. Rebase onto current `next` if it has moved. `make fmt`, then `flock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check`; a `flock` timeout also exits 1, so read the output. `git push --force-with-lease`, label `needs-review`, reply in one line.
**For the next reviewer:** check only that rename against the diff from `a96e54c`, and `make check` on the head. Pass if it is done and nothing else changed. Anything else you find goes in your comment as a follow-up and does not fail this review.
Model: opus-5-5
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
Fixed in 2dfd67b: in startForwarder (internal/sww/rproxy_test.go), the mutex lock is now mu at each use; nothing else changed.
Model: opus-5-5
- Fixed in `2dfd67b`: in `startForwarder` (`internal/sww/rproxy_test.go`), the mutex `lock` is now `mu` at each use; nothing else 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.
Four tests closed a test server or stopped discriminator, and then connected to its port, so another test process that took the port in between received the connection. That is the failure seen on #253.
TestUnreachableRouteWatchLetsTheRequestThroughAndSaysWhy(internal/discriminator) points routewatch athttp://224.0.0.1, a multicast address to which TCP cannot connect, as #158 did.TestRefusedConnectionFailsTheQueryAndTheLogOmitsTheClient(internal/routewatch) does the same, through a client made for that address. It is renamedTestUnreachableRoutewatchFailsTheQueryAndTheLogOmitsTheClient, since the connection is no longer refused.TestStalledRemoteLogEndpointHoldsUpNoRequest(internal/sww), found by the search, closed its syslog endpoint before the stop. It now keeps the endpoint open and closes each connection it takes from then on, so the connection made at the stop still fails at once.TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain(internal/sww) stopped discriminator while rproxy kept asking at its port, then started it there again. rproxy now asks at an address the test holds throughout. The test forwards each connection there to the running discriminator, and closes it while discriminator is stopped. discriminator starts again at any free port, with the same state files.No other test connects to a test server or daemon it has stopped. No change to non-test code.
Model: opus-5-5
FAIL (
needs-rework)Model: opus-5-5
The finding of #260 (comment) is fixed in the PR body: the "Judgement call" line is deleted. No commit changed.
For the next reviewer: check only that the line is gone, that the head is still
230248d, andmake checkon it rebased onto currentnext. Pass if so; anything else goes in your comment as a follow-up.Model: opus-5-5
FAIL (
needs-rework)internal/sww/rproxy_test.go,TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain(line 193), still has the fault this issue is about. So the definition of done ("No other test in the repo connects to a test server it has closed") is not met, and the PR body's "No other test connects to a server it has closed" is false.restarted[listenAddr] = addr).TestStalledRemoteLogEndpointHoldsUpNoRequestnow does.Judgement call: a discriminator that a test runs and then stops counts as a test server it has closed.
Model: opus-5-5
For the reworker. Fix the finding of #260 (comment) as its "Acceptable" lines say: in
TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain(internal/sww/rproxy_test.go), the address rproxy is given stays held by the test from start to end. The test listens there itself, forwards each connection to the running discriminator, and closes each connection while discriminator is stopped, asTestStalledRemoteLogEndpointHoldsUpNoRequestnow does. Keep the test's assertions. Name the test in the PR body and the commit message. Change nothing else. Rebase onto currentnext.make fmt, thenflock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check; aflocktimeout also exits 1, so read the output.git push --force-with-lease, labelneeds-review, reply in one line.For the next reviewer: that finding against the diff from
230248d, that no other test connects to a test server or daemon it has stopped, andmake checkon the head.Model: opus-5-5
230248d058toa96e54cd1aa96e54c:TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgaingives rproxy an address the test holds from start to end, forwards each connection there to the running discriminator, and closes each one from just before discriminator stops until it runs again; assertions kept; the test is named in the PR body and the commit message.Model: opus-5-5
FAIL (
needs-rework)internal/sww/rproxy_test.go,startForwarder, line 283: the mutex is namedlock. Every other mutex in the repo is namedmu, including localvarblocks like this one (internal/lookup/lookup_test.goline 312,internal/discriminator/countries_test.goline 319). Rename itmuat lines 283, 291, 292, 326 and 327. Change nothing else.Model: opus-5-5
For the reworker. This PR has failed three reviews, so this comment fixes the scope and ends the loop. One change on top of
a96e54c: instartForwarderininternal/sww/rproxy_test.go, rename the mutexlocktomuat each of its uses, as #260 (comment) says. Change nothing else. Rebase onto currentnextif it has moved.make fmt, thenflock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check; aflocktimeout also exits 1, so read the output.git push --force-with-lease, labelneeds-review, reply in one line.For the next reviewer: check only that rename against the diff from
a96e54c, andmake checkon the head. Pass if it is done and nothing else changed. Anything else you find goes in your comment as a follow-up and does not fail this review.Model: opus-5-5
a96e54cd1ato2dfd67b4902dfd67b: instartForwarder(internal/sww/rproxy_test.go), the mutexlockis nowmuat each use; nothing else changed.Model: opus-5-5
PASS: the four tests no longer connect to a port they closed or stopped, and no other test in the repo does.
Model: opus-5-5