Finish the server shutdown tests; fix the two bugs they found
build / build-push (push) Successful in 22s
build / build-push (push) Successful in 22s
internal/server was the last package whose shutdown was tested by assumption: four transports, three shutdown shapes, and every one of them can strand a goroutine that a client is holding open. Both bugs below were live in the tree as handed over and neither had a test. An idle client could hold the whole shutdown open. The stream accept loop waits for its connections before Stop returns, and a connection parked in ReadMsg with no deadline in front of it only ends when the client goes away — ctx.Err() is checked between reads, so cancelling the context cannot reach a read that is already blocked. One connection that connected and said nothing was enough to make Stop sit there for the caller's entire grace period (15s in main) and then return context.DeadlineExceeded, which the app reports as "shutdown completed with errors"; and the cache snapshot the app takes right after Stop never happened, because the orchestrator had given up first. It is a shutdown DoS that costs the attacker one connection. The fix is a read deadline in the past on every accepted connection when the context is cancelled, and deliberately not a close: a query already inside the pipeline is past its read, so its answer still goes out, which is the invariant TestInFlightQueryIsAnsweredAcrossStop pins. The accept loop keeps a small registry of live connections for this — the accept path, not the query path, and no query ever takes that mutex. A UDP query in flight at Stop was lost and nothing waited for it. The stream paths register their connections with s.wait; servePacket did not register at all, so a UDP query already inside the pipeline was gone twice over: nothing waited for it, and the answer had nowhere to go once it was ready, because a datagram is written to the very socket the query arrived on. The visible damage was not the lost answer (the client retries) but the app, which snapshots the cache and closes the store the moment Stop returns — a query still in the pipeline could be writing to a store on its way out. Packets are now counted in s.wait, and the UDP listener's close wakes the read loop with an expired deadline and then waits, bounded by udpDrainGrace (250ms), for the loop and the packets it spawned before closing the socket. The bound is deliberate: a shutdown may not spend its grace period on one slow upstream, and past the bound the answer is dropped, which is what the client would see anyway. Stop returning did not mean the process was done, so nothing could tell a leaking shutdown from a clean one. TestStopLeavesNoGoroutineRunningServerCode counts goroutines whose stack still has a frame in the package — not runtime.NumGoroutine(), which in a test binary is net/http, crypto/tls and the race detector talking over each other — and it found the idle-connection bug above. TestStopIsNotBlockedByAnIdleStreamConnection sets a 60s DoT idle timeout on purpose so a config value cannot hide the regression behind a lucky tick, and TestInFlightQueryIsAnsweredAcrossStop now covers all four transports. harness.idleStream exists because a test that only dials a stream transport races the accept loop: Stop can close the listener while the connection is still in the backlog, the server never accepts it, and a shutdown assertion that needs a live server-side goroutine passes for the wrong reason. AGENTS.md gains the two durable invariants (no step in the shutdown may wait for a client; every query goroutine is registered with s.wait, UDP included) and the two test-hygiene traps. STATUS.md drops the UDP query lost at Stop from its pinned list, since it is now a test rather than an accident, and the plan starts at the pinned decisions that are actually left.
This commit is contained in:
1 parent
3a199bb310
commit
b6d70a50fe
5 files changed
+526
-41
No files matched your search
@@ -254,7 +254,34 @@ func (s *Server) serveUDP(ctx context.Context, addr string) error {
|
||||
// on every iteration: a deadline set once expires for good and turns the
|
||||
// loop into a spin that never reads another packet.
|
||||
const readTick = 500 * time.Millisecond
|
||||
s.add(listener{close: pc.Close, info: ListenerInfo{
|
||||
// Two counters, two jobs. s.wait is the lifecycle one: Stop must not
|
||||
// return while a query is still inside the engine, because the app
|
||||
// snapshots the cache and closes the store the moment Stop returns.
|
||||
// packets is the drain one, and only the reader goroutine ever waits on
|
||||
// it — after it has stopped adding to it, which is what makes waiting on
|
||||
// it legal at all.
|
||||
var packets sync.WaitGroup
|
||||
readerDone := make(chan struct{})
|
||||
s.add(listener{close: func() error {
|
||||
// A read deadline that has already expired wakes the read loop at
|
||||
// once instead of leaving it to the read tick, so a Stop with an
|
||||
// idle listener costs nothing. The loop returns on its next
|
||||
// iteration either way: the context is already cancelled by the time
|
||||
// Stop reaches this close.
|
||||
_ = pc.SetReadDeadline(aLongPast)
|
||||
// And then the queries already in the pipeline get their chance to
|
||||
// be answered on this socket. Unlike the stream transports, a UDP
|
||||
// answer has nowhere else to go: the write goes to the very socket
|
||||
// the query arrived on, so a packet that is written after Close is
|
||||
// simply lost and the client has nothing but its own retry. The wait
|
||||
// is bounded — a shutdown may not spend its whole grace period on one
|
||||
// slow upstream.
|
||||
select {
|
||||
case <-readerDone:
|
||||
case <-time.After(udpDrainGrace):
|
||||
}
|
||||
return pc.Close()
|
||||
}, info: ListenerInfo{
|
||||
Transport: "udp", Addr: pc.LocalAddr().String(), Enabled: true, Running: true,
|
||||
}})
|
||||
s.log.Info("dns listener up", "transport", "udp", "addr", pc.LocalAddr().String())
|
||||
@@ -262,6 +289,11 @@ func (s *Server) serveUDP(ctx context.Context, addr string) error {
|
||||
s.wait.Add(1)
|
||||
go func() {
|
||||
defer s.wait.Done()
|
||||
// The deferred order is the whole contract: drain the packets, then
|
||||
// report the reader done — which is what releases the close func
|
||||
// above — and only then leave s.wait.
|
||||
defer close(readerDone)
|
||||
defer packets.Wait()
|
||||
buf := make([]byte, dns.MaxMsgSize)
|
||||
for {
|
||||
if ctx.Err() != nil {
|
||||
@@ -290,7 +322,13 @@ func (s *Server) serveUDP(ctx context.Context, addr string) error {
|
||||
// on the hottest path in the process. servePacket only needs the
|
||||
// address to write back to, so pass what we already have.
|
||||
addr, _ := from.(*net.UDPAddr)
|
||||
go s.servePacket(ctx, pc, pkt, from, addr)
|
||||
packets.Add(1)
|
||||
s.wait.Add(1)
|
||||
go func() {
|
||||
defer packets.Done()
|
||||
defer s.wait.Done()
|
||||
s.servePacket(ctx, pc, pkt, from, addr)
|
||||
}()
|
||||
}
|
||||
}()
|
||||
return nil
|
||||
@@ -301,6 +339,18 @@ func isTimeout(err error) bool {
|
||||
return errors.As(err, &ne) && ne.Timeout()
|
||||
}
|
||||
|
||||
// udpDrainGrace is how long Stop waits for the queries already inside the
|
||||
// pipeline before it closes the UDP socket. See serveUDP: the point is not to
|
||||
// make a shutdown slow, it is to give an answer that is milliseconds from
|
||||
// being written somewhere it can still reach the client.
|
||||
const udpDrainGrace = 250 * time.Millisecond
|
||||
|
||||
// aLongPast is a read deadline that has already expired. Setting it is how a
|
||||
// blocked read is woken without closing anything under it: the read fails
|
||||
// with a timeout, which is what a shutdown wants, and a connection that still
|
||||
// owes the client an answer can still write it.
|
||||
var aLongPast = time.Unix(1, 0)
|
||||
|
||||
// servePacket answers one UDP query. The listener is passed in so the reply
|
||||
// goes out on the very same socket the query arrived on: a fresh socket per
|
||||
// answer would burn an ephemeral port and a connect handshake per query.
|
||||
@@ -425,11 +475,48 @@ func (s *Server) serveStream(ctx context.Context, ln net.Listener, transport str
|
||||
Transport: transport, Addr: ln.Addr().String(), Enabled: true, Running: true,
|
||||
}})
|
||||
s.log.Info("dns listener up", "transport", transport, "addr", ln.Addr().String())
|
||||
// The accepted connections, so that a cancellation can reach the ones
|
||||
// parked in a read. This is the accept path, not the query path: a
|
||||
// connection is added once and removed once, and no query ever takes this
|
||||
// mutex.
|
||||
var (
|
||||
connMu sync.Mutex
|
||||
conns = map[net.Conn]struct{}{}
|
||||
)
|
||||
s.wait.Add(1)
|
||||
go func() {
|
||||
defer s.wait.Done()
|
||||
var wg sync.WaitGroup
|
||||
defer wg.Wait()
|
||||
// A connection that is open but idle is the one thing a cancelled
|
||||
// context cannot reach by itself: the loop below checks ctx.Err()
|
||||
// between reads, so a client that connects and then says nothing
|
||||
// parks in ReadMsg until the client goes away. Stop waits for these
|
||||
// goroutines, so the shutdown would sit there until the caller's
|
||||
// grace period ran out and then report a context error — a shutdown
|
||||
// DoS that costs a client one connection.
|
||||
//
|
||||
// The nudge is a read deadline in the past, not a close: a query
|
||||
// already inside the pipeline is past its read, so its answer is
|
||||
// still written. That is the invariant
|
||||
// TestInFlightQueryIsAnsweredAcrossStop pins, and closing the
|
||||
// connection here would break it.
|
||||
stop := make(chan struct{})
|
||||
defer close(stop)
|
||||
wg.Add(1)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
select {
|
||||
case <-stop:
|
||||
return
|
||||
case <-ctx.Done():
|
||||
}
|
||||
connMu.Lock()
|
||||
for c := range conns {
|
||||
_ = c.SetReadDeadline(aLongPast)
|
||||
}
|
||||
connMu.Unlock()
|
||||
}()
|
||||
for {
|
||||
conn, err := ln.Accept()
|
||||
if err != nil {
|
||||
@@ -441,9 +528,17 @@ func (s *Server) serveStream(ctx context.Context, ln net.Listener, transport str
|
||||
}
|
||||
return
|
||||
}
|
||||
connMu.Lock()
|
||||
conns[conn] = struct{}{}
|
||||
connMu.Unlock()
|
||||
wg.Add(1)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
defer func() {
|
||||
connMu.Lock()
|
||||
delete(conns, conn)
|
||||
connMu.Unlock()
|
||||
}()
|
||||
s.serveConn(ctx, conn, transport, idleTimeout)
|
||||
}()
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user