From 9f4cf7c6c400a56a51b4d31e4f2dc9b149ac46c4 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 16:38:24 +0000 Subject: [PATCH] resolver: a query cut short by the caller's context records no reason (closes #298) When the caller's context ends, the next query returns ErrContextCanceled before anything is sent. querySingleType recorded that as a network error, which outranks a timeout, so a nameserver that had only timed out could be reported as "network error: context canceled". It now records no reason for it. A try the context's deadline cuts short ends as a timeout. When the context has ended before a further try, tryExchange now returns that try's error instead of ErrContextCanceled, so such a nameserver is reported as timed out, not as answering. Model: opus-5-5 --- TODO.md | 4 ++ internal/resolver/iterative.go | 15 ++++- internal/resolver/iterative_internal_test.go | 32 ++++++++++ internal/resolver/resolver_test.go | 64 ++++++++++++++++++-- 4 files changed, 109 insertions(+), 6 deletions(-) diff --git a/TODO.md b/TODO.md index 9375cbd..4a5fdf8 100644 --- a/TODO.md +++ b/TODO.md @@ -19,6 +19,10 @@ milestone: https://git.eeqj.de/sneak/dnswatcher/milestone/7 # Completed Steps +- 2026-10-15: a query its caller's context ends before it is sent records no + reason about the nameserver, and a try the context's deadline cuts short is a + timeout, so a nameserver whose only failures were timeouts is `timeout`, not a + network error or `ok` (closes #298). - 2026-10-15: the resolver no longer says its query timeout grows on each try; every try waits the same 2 seconds, and the unused `timeoutMultiplier` is gone (closes #296). diff --git a/internal/resolver/iterative.go b/internal/resolver/iterative.go index 4d495f7..9156373 100644 --- a/internal/resolver/iterative.go +++ b/internal/resolver/iterative.go @@ -62,7 +62,10 @@ func checkCtx(ctx context.Context) error { } // tryExchange sends msg to addr over UDP, up to maxRetries times, until -// a reply comes. Every try waits the same queryTimeoutDuration. +// a reply comes. Every try waits the same queryTimeoutDuration, or less +// when ctx's deadline comes first. When ctx has ended before a further +// try, it returns the error of the try already made, such as a timeout, +// or ErrContextCanceled when none was made. func (r *Resolver) tryExchange( ctx context.Context, msg *dns.Msg, @@ -74,7 +77,11 @@ func (r *Resolver) tryExchange( for range maxRetries { if checkCtx(ctx) != nil { - return nil, ErrContextCanceled + if err == nil { + return nil, ErrContextCanceled + } + + return nil, err } resp, _, err = r.client.ExchangeContext(ctx, msg, addr) @@ -863,6 +870,10 @@ func (r *Resolver) querySingleType( if err != nil { err = fmt.Errorf("query %s @%s: %w", hostname, nsIP, err) } + case errors.Is(err, ErrContextCanceled): + // The caller's context ended before the query was sent. + // That says nothing about the nameserver, so no reason is + // recorded. case isTimeout(err): state.gotTimeout = true case errors.Is(err, ErrRefused): diff --git a/internal/resolver/iterative_internal_test.go b/internal/resolver/iterative_internal_test.go index f0f399c..a804f33 100644 --- a/internal/resolver/iterative_internal_test.go +++ b/internal/resolver/iterative_internal_test.go @@ -1,6 +1,8 @@ package resolver import ( + "context" + "log/slog" "strconv" "syscall" "testing" @@ -132,3 +134,33 @@ func TestReadReply(t *testing.T) { }) } } + +// TestQuerySingleType_ContextEnded asks a nameserver about one record +// type with a context that has already ended, so no query is sent. A +// query the context ended before it was sent says nothing about the +// nameserver and records no reason: a nameserver whose earlier query +// timed out is still reported as timed out, not as a network error. +func TestQuerySingleType_ContextEnded(t *testing.T) { + t.Parallel() + + r := NewFromLogger(slog.New(slog.DiscardHandler)) + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + resp := &NameserverResponse{Records: map[string][]string{}, Status: StatusOK} + + // The query for an earlier record type timed out. + state := queryState{gotTimeout: true} + + _, err := r.querySingleType( + ctx, []string{"192.0.2.1"}, "example.com.", dns.TypeA, resp, &state, + ) + require.ErrorIs(t, err, ErrContextCanceled) + assert.Equal(t, queryState{gotTimeout: true}, state) + + classifyResponse(resp, state) + + assert.Equal(t, StatusTimeout, resp.Status) + assert.Equal(t, "all queries timed out", resp.Error) +} diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go index 9bd98ed..9d105b7 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -1380,10 +1380,10 @@ func TestQueryNameserverIP_Timeout(t *testing.T) { r := newTestResolver(t) // Nothing answers at 192.0.2.1, a documentation address. The - // resolver tries each query twice, and the first try gives up - // after two seconds. A deadline that ends during the first try - // makes the status vary from run to run between error and - // timeout, so the deadline must outlast the first try. + // resolver tries each query twice, and each try gives up after two + // seconds. This deadline ends during the second try of the first + // record type; TestQueryNameserverIP_DeadlineDuringFirstTry covers + // one that ends during the first. ctx, cancel := context.WithTimeout( context.Background(), 3*time.Second, ) @@ -1399,6 +1399,62 @@ func TestQueryNameserverIP_Timeout(t *testing.T) { assert.NotEmpty(t, resp.Error) } +// TestQueryNameserverIP_DeadlineDuringFirstTry gives a query to +// 192.0.2.1, where nothing answers, a deadline that ends during its +// first try. That try ends at the deadline as a timeout, so the +// nameserver is reported as timed out, not as answering. +func TestQueryNameserverIP_DeadlineDuringFirstTry(t *testing.T) { + t.Parallel() + + r := newTestResolver(t) + + // Well under the two seconds one try waits. + ctx, cancel := context.WithTimeout( + context.Background(), 500*time.Millisecond, + ) + t.Cleanup(cancel) + + resp, err := r.QueryNameserverIP( + ctx, "unreachable.test.", "192.0.2.1", + "example.com", + ) + require.NoError(t, err) + + assert.Equal(t, resolver.StatusTimeout, resp.Status) + assert.NotEmpty(t, resp.Error) +} + +// TestQueryNameserverIP_CancelDuringFirstTry cancels the context while +// the first try of a query to 192.0.2.1, where nothing answers, waits +// for a reply. That try's timeout is all that is learned about the +// nameserver, so it is reported as timed out, not as answering. +func TestQueryNameserverIP_CancelDuringFirstTry(t *testing.T) { + t.Parallel() + + r := newTestResolver(t) + + // The first try starts at once and waits two seconds for a reply; + // the cancel comes a second in. The DNS client cuts a try short at + // the context's deadline but not at a cancel, so the try still + // waits its full two seconds and ends as a timeout a second after + // the cancel. By then the context has certainly ended, so no second + // try is made. With a deadline instead, the try ends at the moment + // the context does, and whether a second try is made depends on + // which the resolver sees first. + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + time.AfterFunc(time.Second, cancel) + + resp, err := r.QueryNameserverIP( + ctx, "unreachable.test.", "192.0.2.1", + "example.com", + ) + require.NoError(t, err) + + assert.Equal(t, resolver.StatusTimeout, resp.Status) + assert.NotEmpty(t, resp.Error) +} + // TestQueryNameserverIP_CancelledLogsNothing cancels the context while // a query to 192.0.2.1, where nothing answers, is waiting for a reply, // as shutdown does. The query was cut short, not failed, so nothing is