Repository navigation
fix: retire the previous config on reload - #27
Merged
Merged
Conversation
After a config change, a share of requests kept being served by the previous routing. Not during the swap: still happening fifteen minutes later, on fresh connections. Twelve sequential requests to a route whose upstream had been changed: 403 403 502 404 403 403 404 502 403 404 404 403 The 502s are Caddy dialing an address that no longer appeared in any config. Inherited listeners were wrapped so Close is a no-op, because the descriptor belongs to the privileged parent and closing it means nothing can bind :443 again (ADR 0002). But Close is also how Caddy retires a server: it closes, waits for the accept loop to end, and considers the old config gone. A listener that ignores Close never ends that loop, so two servers kept accepting from one socket and each connection went to whichever won the race. listen.Handoff owns the socket instead. One goroutine calls Accept on the real descriptor; each config load takes a generation that receives from it, and taking a new one retires the previous, whose Accept then returns net.ErrClosed. The descriptor is still never closed. Verified by mutation: dropping the retire step in Next reproduces the original failure, with the test reporting that two servers now share one socket. The end-to-end daemon test cannot cover this. It runs without a privileged parent, so Caddy binds its own listeners and never takes this path. The guard is at the unit level. listen.KeepOpen is now unused outside its own test. Left in place because ADR 0002 explains it and Handoff relies on the same rule for the half it still gets right. Closes #26
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.
Closes #26.
After a config change, a share of requests kept being served by the previous routing. Not during the swap: still happening fifteen minutes later, on fresh connections. Twelve sequential requests to a route whose upstream had just been changed:
The 502s are Caddy dialing an address that no longer appears in any config.
Cause
Inherited listeners were wrapped so
Closeis a no-op. That half is correct and necessary: the descriptor belongs to the privileged parent, and closing it means nothing in the process can bind:443again (ADR 0002).But
Closeis also how Caddy retires a server. It closes the listener, waits for the accept loop to end, and considers the old config gone. A listener that ignoresClosenever ends that loop, so two servers kept accepting from one socket and each connection went to whichever won the race.Closemeant both "release the descriptor" and "this server is finished". Only the first was obviously in play whenKeepOpenwas written.Fix
listen.Handoffowns the socket. Exactly one goroutine callsAccepton the real descriptor, and each config load takes a generation that receives from it. Taking a new generation retires the previous one, whoseAcceptthen returnsnet.ErrClosed, which is the signal Caddy waits for. The descriptor is still never closed.Retirement is signalled on a separate channel rather than by closing the connection channel: the accept loop selects on a send to it, and closing a channel you are about to send on panics. An unbuffered send only completes when paired with a receive, so a generation that takes retirement instead never swallows a connection — the send does not happen and the loop offers it to the next generation.
Testing
go test ./... -racepasses on all 11 packages.GOOS=linuxandGOOS=windowsbuild clean.Verified by mutation: dropping the retire step in
Nextreproduces the original bug, and the test says so in those terms —The end-to-end daemon test cannot cover this. It runs without a privileged parent, so Caddy binds its own listeners and never reaches this path. The guard is at the unit level, which is worth knowing if someone later assumes the integration test would catch a regression here.
Notes
listen.KeepOpenis now unused outside its own test. Left in place because ADR 0002 explains it andHandoffrelies on the same rule for the half it still gets right. Say if you would rather it went.ADR 0002 gains the second half of the story, including why shipping only the first half looked like it worked.