Shutdown hang in v0.107.79: recursive serverLock read lock through query logging?

[Michael-Zebratrix]

We investigated an AdGuard Home shutdown hang in an isolated lab. Is this a known problem, and would you prefer a bug issue or a private report before discussing a patch?

Reduced reproduction

We reduced the initial deployment-specific test to the official, unmodified AdGuard Home v0.107.79 Linux AMD64 binary and a Python standard-library-only harness. It runs in a fresh Linux network namespace with loopback only and no default route, on a Debian 13 VM. No LXC, NetBird, proxy, TLS, systemd or simulated network failure participates in this reduced case.

1. Start a synthetic UDP DNS responder on 127.0.0.2:5353, returning an A record with TTL 0 for generated .lab.test questions.
2. Start AdGuard with DNS on 127.0.0.1:5354, that sole upstream, query logging enabled (size_memory: 1), statistics disabled and all configured runtime client sources, including reverse DNS, disabled. The diagnostic pprof listener is loopback-only.
3. Use 16 local workers to send ordinary A questions for about 0.10–0.15 seconds, then send SIGTERM to the test AdGuard process.
4. Stop the clients after the bounded shutdown wait. If the process remains alive, capture goroutine stacks twice, two seconds apart.

In one completed campaign, attempts 1 and 2 exited with code 0. Attempt 3 remained blocked approximately five seconds after SIGTERM, with the same lock cycle present in both captures after the clients had stopped; the local upstream remained available throughout. The harness then explicitly killed only its disposable AdGuard child after preserving the evidence. This cleanup is not a successful shutdown or a workaround.

This demonstrates that the original VPN/upstream-failure setup is unnecessary. It is still a timing-dependent reproduction, not a deterministic regression test or a measurement of real-world failure frequency. A self-contained Python harness is prepared and can be provided; it verifies the exact binary digest and refuses to run outside an isolated network namespace.

Captured cycle and source analysis

Reduced transcription of the late capture, with arguments and addresses removed; both paths use the same dnsforward.Server, verified against the original capture:

Shutdown goroutine — waiting for RWMutex.Lock:
  sync.(*RWMutex).Lock
  dnsforward.(*Server).Close                  dnsforward.go:282
  home.closeDNSServer
  home.stopDNSServer
  home.cleanup

Query goroutine — waiting for RWMutex.RLock:
  sync.(*RWMutex).RLock
  dnsforward.(*Server).IsBlockedClient       dnsforward.go:906
  [client lookup via clientOrArtificial / findMultiple]
  querylog.(*queryLog).ShouldLog
  dnsforward.(*Server).shouldLog
  dnsforward.(*Server).processQueryLogsAndStats

processQueryLogsAndStats holds serverLock.RLock() while the query-log client lookup reaches IsBlockedClient, which requests a second read lock on the same server. A pending writer prevents that second read lock from succeeding, while the first reader prevents the writer from proceeding.

Sources for the exact release: outer lock, client lookup, IsBlockedClient, Go RWMutex semantics.

Source inspection also finds writers for DNS configuration, protection and access-list changes. We have not reproduced the cycle through those operations, nor demonstrated an unauthenticated DNS-only trigger.

Related reports and question

We searched open/closed issues and PRs for the function names, shutdown hangs and recursive locks, without finding an exact match. #8359 concerns a different statistics/bbolt lock; #8485 was ultimately attributed to buffering and an external watchdog; #8522 discusses nested read-lock risk in another path, not an identified fix for this chain.

We have not tested a newer release as a fix or built a patched binary. Simply removing the outer lock needs care because shutdown clears the query-log/statistics references.

Would narrowing or restructuring the lock scope be the preferred direction, with a deterministic regression test for an outer reader, a pending writer and a re-entrant client lookup? We can provide the reduced harness and prepare a proposed patch after confirming the reporting channel and approach.

[hippi345]

yeah this still looks real on master. processQueryLogsAndStats holds serverLock.RLock() across shouldLog, and IsBlockedClient takes RLock() again further down. Go's RWMutex doesn't allow recursive read locks once a writer is waiting, so outer RLock -> Close() waiting on Lock() -> inner RLock blocks forever. Not a harness artifact.

Since the trigger needs a writer (shutdown or an admin-side config change) and plain DNS clients can't cause that, I'd file it as a normal bug with your two stack traces, not a security report. CONTRIBUTING wants an issue before a PR anyway.

Two obvious fix shapes: copy queryLog/stats and the config bits into locals under the lock and call them after releasing it, or add an isBlockedClientLocked variant that assumes the lock is held. A small deterministic test (outer RLock, Lock() in a goroutine, then the re-entrant call) would make either easy to review.

[Michael-Zebratrix]

I opened #8658 with the reproduction, proposed fix, and VM/production validation results. Could the maintainers review the approach there before I submit a PR?