Repository navigation
web: Close drops connections that have asked nothing - #640
Merged
Merged
Conversation
http.Server.Shutdown counts a connection with no request on it yet as busy until it is five seconds old. A client's spare connection, such as a cluster peer's second dial or a browser's preconnect, held every stop that long, and a node under a five-second shutdown_timeout missed its deadline: the cluster fault tests failed about one run in twenty with "stop Sable integration node: exit status 1" (seen in Short pass on TestRecoveryPromotionKeepsSOASerialsMonotonic). The web server now tracks connections in StateNew and closes them when Close begins.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
The web server now drops connections that never sent a request when it closes, which ends the cluster test flake: red-then-green unit test (C186), the flaky test 20 of 20 and the whole cluster fault package 3 of 3 on this head (C187). All 6 GitHub checks are green on 7a201b9, Short pass included, and it merges cleanly onto today's main with the web race tests passing.
Red, then green
PATH=/usr/local/go/bin:$PATH GOEXPERIMENT=jsonv2 GOFLAGS=-p=1 go test ./internal/web/ -run '^TestCloseDoesNotWaitForAConnectionThatSentNothing$' -count=1Checks
/home/peak/work/dhcp-server-for-sable/out/flake-restore/pr640-prove.sh, exit 0Risks
The flake was about 1 run in 20, so passing runs are odds, not proof: 80 of 80 and 50 of 50 single-test runs plus 3 whole-package runs since the fix, against 2 of 40 failing on main. A request that lands on a brand-new connection in the instant Close begins is dropped and the client has to retry; that window only exists during a stop. The two backup-restore cluster tests that went red in Short pass on #630 and #633 (TestPrimaryRestoreFromBackupBringsTheClusterBack, TestPrimaryRestoreKeepsSOASerialsMonotonic) were never reproduced here, so I haven't shown they share this cause; if they go red after this merges, that's a second bug. The branch is one merge behind main (#632), so it needs Update branch before it merges and the checks have to go green again on that commit. Not run: a real browser's preconnect against a production stop, Postgres.
Prover
Checking now.
Every command above was run by Fortysix's Basecamp itself, on the branch named.