# Milestone 16: contained behavioral fixes Goal: fix the verified bugs and silent failures from `TECH_DEBT.md` themes 3, 5, 6 and 7 — each one localized, each with a test that fails before and passes after. No refactors: the duplication work is milestone 18, and a refactor that must simultaneously fix behavior is how mirrored copies diverge further. **PROVISIONAL.** Written before milestone 15 was built. m15 touches `build.zig`, workflows, fuzz files, `filter_integration_test.zig`, `cli.zig`, `docs_drift_test.zig` and `logging.zig` — re-verify line references in those files before starting. ## Rulings (binding) ### 1. `loadSource` propagates cancellation `src/filter/manager.zig:541-544` and `:547-550`: two identical catch sites fold `error.Canceled` into `loadFailure(...)`, recording a bogus "Canceled" load-failed status, consuming the one-shot cancellation, and publishing a snapshot with the source excluded. Ten other catch sites in the file propagate it correctly. Add `if (err == error.Canceled) return error.Canceled;` beside the existing OutOfMemory arm at both sites — `Manager.Error` already has the member. Unit test: a reader that fails with Canceled during `loadSource` makes the whole call return Canceled and writes no status. ### 2. `commitStatus` never drops an outcome silently `src/filter/manager.zig:1237-1250`: the id-match loop falls through with no log when a source has no status entry (reachable via a startupPass / web-insert race; milestone-5 policy says every non-ok state is recorded and surfaced). Add a `log.warn` after the loop naming the source id. Unit test: committing a status for an unknown id emits the warning and does not crash. ### 3. Downloads and compiles leave the writer lock `src/filter/manager.zig:609-624`: `refreshAll` holds `writer_lock` across every download (300 s budget each, `app.zig:85`) and every compile; every web mutation ends in `Manager.reload` on the same lock, so an unrelated rule save parks uncancelably — on a request path with no timeout, occupying one of 64 web slots — until the pass ends. The download-to-tmp flow already exists (`download` writes `.raw.tmp` via `compiledName`, with `deleteQuietly` defers at :646-648); the lock is just taken too early. Restructure: - New `refresh_lock: std.Io.Mutex` on Manager, serializing refresh passes against each other only. - Split `refreshOne` (:626-729): the fetch/detect/compile stages (:650, :659, :668) run under `refresh_lock` alone; the publish stage (:711), the status commit and the final `reloadLocked` run under `writer_lock`. - `refreshAll` takes `refresh_lock` for the pass, and `writer_lock` only per-source around publish plus once for the final reload. `refreshSource` (:582-586) follows the same split. - The lock-ordering rule, documented on both mutex fields: `refresh_lock` is never acquired while holding `writer_lock`. - `refresh_lock` serializes blocklist-**directory maintenance** too, not just refresh passes: `pruneOrphans` (:1040, invariant comment at :1130-1136) today assumes every temp-file writer holds `writer_lock`; once downloads write `.raw.tmp`/`.list.tmp`/`.wild.tmp` under `refresh_lock` alone, a concurrent source deletion (blocklists.zig delete → reload → pruneFiles) could sweep an in-flight refresh's temp files. `pruneOrphans` therefore takes `refresh_lock` first (then `writer_lock` if it needs it — same order as everywhere), and its invariant comment is rewritten. Regression test: a source deletion during a stalled refresh must not delete the stalled refresh's temp files. Integration test (`filter_integration_test.zig`): with a refresh pass parked on a deliberately stalled fixture route, a concurrent rule mutation completes without waiting for the pass. DNS serving was never affected and stays covered by the existing tests. ### 4. Truncated responses are never cached `src/cache/dns_cache.zig:106-131`: `classify` reads `rcode` and `ancount` and never `flags.tc`, so a TC=1 answer from a misbehaving upstream is cached for up to `max_ttl_seconds` (86 400 s) and re-served — TC bit intact, even over TCP, which can loop retrying clients (RFC 2181 §9). Add `if (p.header.flags.tc) return null;` (`flags.tc` is `src/dns/header.zig:20`). `transport.validateResponse` is deliberately untouched: rejecting TC there would change failover semantics for every exchange, and the forward client legitimately reads TC for its UDP-to-TCP retry (`forward_client.zig:178`). Record: considered, rejected. Unit test beside the existing classify tests: a TC=1 NOERROR response classifies as null. ### 5. The format sniffer stops eating hosts files with `##` banners `src/filter/parsers.zig:54-73`: `hasAbpMarker` runs before `isComment`, and `isElementHiding` (:86-93) matches `##` (and `#@#`, `#?#`, `#$#`, `#%#`) unanchored with no comment guard — unlike the `$` branch two lines later, which is guarded. A hosts file with a `##` banner parses whole as ABP: `0.0.0.0` enters the domain set and hosts lines with inline URL comments are silently dropped (real blocking loss, reproduced with the URLhaus banner style). The milestone-5 as-built note (specs/milestone-5.md:1677-1678) already *claims* the separators are matched anchored — the code never was. Guarding the branch with `!isComment` is a circular no-op (`isComment` consults `isElementHiding`); do not attempt it. Fix — a positional predicate in `isElementHiding`: a separator at offset `p` counts as element hiding only when - `p == 0` and the character after the separator is not whitespace, `#`, or end of line (a generic rule `##.ad` counts; a banner `## Title`, `####`, or bare `##` does not), or - `p > 0` and `line[p - 1]` is not whitespace and not `#` (a rule `example.com##.ad` counts; prose `see ## below` does not). Required test cases, in `parsers.zig`: the URLhaus banner style sniffs as hosts; `##.ad-banner` sniffs as ABP; `example.com##.ad` sniffs as ABP; `#@#exception` after a domain sniffs as ABP; a `#`-initial line containing `##` sniffs as a comment. The fuzz target (`blocklist_fuzz.zig`) already covers `detectFormat` for crashes. ### 6. VACUUM respects the disk monitor `src/storage/retention.zig`: `runOnce` (:78) does prune → checkpoint → (every 7th pass, :94-95) VACUUM, and takes no monitor — the word does not appear in the file — while its two sibling tasks gate on `disk_monitor.writesAllowed()` (logger.zig:386 as a parameter, manager.zig:1058 as a field). VACUUM is the most expensive write the program makes and fires unconditionally on the exact filesystem the monitor watches, then fails SQLITE_FULL with no retry for seven daily passes. `runOnce` and `run` gain `monitor: ?*disk_monitor.Monitor` (the logger's parameter shape). Only the VACUUM step gates: prune and checkpoint keep running — they free space. A skipped vacuum increments a new `Stats.vacuums_gated` and retries on the *next* pass (drop the modulo-only trigger for a `passes_since_vacuum >= vacuum_every_passes` counter that resets on success). `app.zig:561` passes the monitor like :560 does for the logger. Tests: extend the cadence tests (:223) with a gated pass — the vacuum is skipped, counted, and runs on the next allowed pass. ### 7. An oversized Cookie header degrades loudly, and the session survives `src/web/server.zig:612-620` (`copyHeader`): a header value over the buffer (cookie buffer = `http_util.max_cookie_len` = 1024, http_util.zig:35) returns `""` with no log — under the documented reverse-proxy-on-shared- domain deployment (foreign cookies riding along), every request silently 401s and the operator sees an unexplained login loop. `copyHeader` stays generic. The cookie call site (:510) changes: when the raw header value exceeds the buffer, extract only the session pair by running `http_util.cookieValue` (:200 — it already parses pairs and returns a slice of the original header) with the existing session-cookie name constant against the full value, copy that pair into the buffer as `=`, and emit one `log.debug` with the dropped size (the `.web_server` scope at :55; no secret is logged). If even the pair does not fit, keep the empty result but still log. Integration test (`web_integration_test.zig`): a request with a 2 KiB cookie header whose session pair is valid is authenticated; the same header without the pair 401s. ### 8. Live view detects fatal SSE rejections `web/src/features/live/useLiveQueries.ts`: per the WHATWG spec a non-200 response fails an EventSource permanently after one error event, so the 3-consecutive-errors threshold (:29, :125-133) is unreachable on exactly the 429/401 paths it was built for — the UI shows "Reconnecting…" forever and the capped state and session probe are dead. `FakeEventSource` has no readyState, so tests pass — the mocked-network class again. - `EventSourceLike` (:9-13) gains `readyState: number`; export `const EVENT_SOURCE_CLOSED = 2`. The browser `EventSource` satisfies it structurally. - In the error handler: `readyState === EVENT_SOURCE_CLOSED` is a permanent failure — close, set `capped`, run the session probe immediately, bypassing the counter. Transient errors (browser auto-retry pending) keep the existing counter path. - `fakeEventSource.ts` gains `readyState` with the real lifecycle (CONNECTING → OPEN on `emit("open")`, CLOSED on `close()` and on `failFatal()`, a new test helper). - New tests: a fatal rejection (single error event, readyState CLOSED) reaches `capped` and calls `probeSession`; a transient error still takes three to trip. ### 9. A timed-out DoH handshake is a handshake failure `src/server/doh_server.zig:311-315` counts a handshake-race timeout as `idle_timeouts`; DoT (:313-317) counts it as `tls_handshake_failures`, which is the recorded spec ruling. Since DoH requests have no other timer, its `idle_timeouts` metric can only ever mean handshake stalls — the same exported name with disjoint semantics per listener. Align DoH's `.timed_out` arm to `tls_handshake_failures`. ### 10. Idle DoH keep-alive connections are reclaimed `src/server/doh_server.zig:70`: `idle_timeout` bounds only the handshake; its own doc comment admits requests have none. DoH stubs hold keep-alives by design, and with no TCP keepalive a vanished peer pins one of 64 slots until restart — while DoT on the same LAN reclaims after 10 s. Extend the existing race to `receiveHead` (:333), the wait for the next request on a keep-alive connection, using the DoT out-param precedent (readPrefix's `out_len`): a small wrapper writes the received `Request` (or its error) through a pointer so the raced function stays `anyerror!void`. On `.timed_out`: bump `idle_timeouts` (now truthfully named again after ruling 9) and close the connection. Preserve the existing error triage at :334-346 (`HttpConnectionClosing` and `ReadFailed` return uncounted; the three structural errors count `bad_requests`/`connection_errors` as today). The body read and `handleRequest` stay untimed, like the web listener. New integration-gated test mirroring the DoT idle test (dot_server.zig:1098): a client that completes the handshake and one request then goes quiet is closed after a short idle budget; `idle_timeouts == 1`, `tls_handshake_failures == 0`. DoH currently has no idle test at all. ### 11. SSE shutdown does not wait for a heartbeat `src/web/sse.zig`: the Hub has no shutdown signal, so graceful drain parks up to 15 s (`heartbeat_interval`, web/handlers/live.zig:32) per idle subscriber — the web `beginShutdown` (web/server.zig:593-605) shuts down sockets but nothing wakes a task inside `Hub.wait`. Measured: the SSE integration test budgets 40 s for exactly this (web_integration_test.zig:74-75). - `Wake` (:33) gains `.closed`. Hub gains `closing: bool` and `pub fn close(self: *Hub, io: std.Io) void`: under the mutex, set the flag and `event.set` every active slot. `wait` returns `.closed` immediately when the flag is set. - `live.zig:114`: `.closed => return`. - `web/server.zig` `deinit` (:349): call the hub's close (via the state's hub pointer) immediately before `beginShutdown` (:360). - Test: a subscriber parked in `wait` returns `.closed` promptly after `close`; the W10 integration teardown no longer stalls (tighten `sse_budget` only if the heartbeat assertion itself allows it). ### 12. The API limiter's sweep runs `src/web/api_limiter.zig:210`: `sweep` exists, preallocates `stale_keys` so it never allocates, is tested — and has no production caller; only the DNS limiter got scheduled. Once 4096 distinct addresses have been seen, the table stays full forever and every unknown-address request pays an O(4096) eviction scan under the limiter mutex. `runMaintenance` (app.zig:709) gains an `api_limiter: ?*ApiLimiter` parameter, wired at :565 from the instance built at :380-388. In the loop, beside the two existing tasks: `_ = api.sweep(io, Clock.awake.now(io));` — note it locks itself, unlike the DNS limiter. Test: the maintenance-loop integration coverage asserts a stale bucket disappears. ### 13. UDP/53 and TCP/53 stats reach /metrics `src/server/udp_server.zig:38-49` and `tcp_server.zig:54-60`: both stats structs are written and read by nothing outside their own integration tests — `dropped_no_slot`/`dropped_oversize` have no metric, no API field, and errors log below the default level, while the DoT/DoH siblings export equivalent counters. The module doc sells "dropped and counted"; the count is unobservable. - Both gain `pub const Snapshot` + `pub fn snapshotStats` in the exact DoT shape (dot_server.zig:61, :232). Field names stay as-is (the `accepted`/`connections` unification is milestone-18 work). - `WebState` (src/web/server.zig) gains `udp_listeners: []const *udp_server.UdpServer = &.{}` and `tcp_listeners: []const *tcp_server.TcpServer = &.{}` — S4 adds the fields with exactly these names and types; S2 consumes them (the app builds four listeners: udp6/udp4/tcp6/tcp4, app.zig:506-528). - `metrics.zig`: sum each family across its listeners into one `nxdns_udp_server_*_total` / `nxdns_tcp_server_*_total` group via the existing `counterGroup` derivation; extend the name-assertion test (:722-743). - `app.zig` wires the slices. ### 14. Forward-client counters survive the query `src/local/forward_client.zig:44-57`: `Stats` (queries, udp_truncated, foreign_datagrams, failures) is instrumented, documented as preventing "an unrecorded failure mode" — and the only production caller builds a stack-local client per query (handler.zig:394) and drops it, so the spoofing signal does not exist. `Handler.Stats` (handler.zig:130-154) gains `forward_udp_truncated`, `forward_foreign_datagrams`, `forward_failures` (atomic, like the other 17; queries are already counted by `forward_zone_answers`). After the exchange in `viaForwardZone`, fetchAdd the client's counters into them. The metrics reflection (`dns_stat_fields`, metrics.zig:48) picks the new fields up without an edit — assert the three new `nxdns_dns_*_total` names in the metrics test. `ForwardClient.Stats` itself stays plain and per-instance. ### 15. The TLS server keeps the concrete transport cause `src/platform/tls_server.zig:418-438`: both BIO callbacks discard the stashed cause — `net_reader.err` / `net_writer.err` (which hold Reset/Timeout/**Canceled**, std Io/net.zig:1260/:1324) are never read anywhere in the file — so a routine idle-budget cancel surfaces as a warn "mbedtls_ssl_read failed" and peer resets are indistinguishable from timeouts. The client side solved exactly this (dot_client.zig `concreteRead`/`concreteWrite`, :303-318). - `ServerStream` gains `recv_cause: ?anyerror = null`, `send_cause: ?anyerror = null`; `bioRecv`/`bioSend` stash `self.net_reader.err`/`self.net_writer.err` on failure before returning the mbedtls code. - `ReadError` (:216-223) widens with `Canceled`; the read path (`readIntoBuffer`, :334) maps a stashed `error.Canceled` to it instead of `TlsFailed`. Callers' race harnesses already treat cancellation separately. - Unit tests in the file's existing stub style: a reader whose `err` is Canceled yields `error.Canceled`; a Reset yields `TlsFailed` with the cause stashed for logging (ruling 16). ### 16. Peer misbehavior logs at debug, like the read path already rules `src/platform/tls_server.zig`: the read path deliberately logs a peer TCP drop at debug (:362-364, "a louder level would be a log-spam vector"), then `close` warns on close_notify against the same dead socket (:311) and `handshake` warns per probe (:287). `.tls_server` is not in the dedup scope set (logging.zig:103-108), so peer-driven warns can evict genuine warnings from the rotating log. - `report` (:474-479) gains a level parameter. The close_notify site passes debug when `peer_closed` is set or the stashed cause (ruling 15) is a peer-class error; the handshake site likewise. Local/config failures keep warn. - Add `.tls_server` to `isDedupScope` (logging.zig:103-108) and to its membership test (:637-644). **Scope widened during implementation (orchestrator ruling):** the level choice also applies to the `ssl_read` and `ssl_write` report sites, not only the two named ones. Ruling 15's "cause stashed for logging" wording requires the read site to consume the level, and an unconditional warn on the write half would keep the same spam vector open through `close` → `flush` against a dead peer. Peer-class causes are decided by pure helpers (`isPeerCause`, `causeLevel`); `check` still passes warn. ### 17. The query log heals its own gaps `web/src/features/queries/QueryLogPage.tsx`: the hand-rolled accumulation (`extra`, `cursorOverride`, `generation`, :79-98) develops a silent mid-table row gap when the base page refetches after 30 s staleness — the newest-100 boundary moves up while `extra` starts below the old cursor, and the stale override means load-more never heals it. On a live DNS server the trigger is routine. Migrate to `useInfiniteQuery`: the endpoint already maps onto it (`getNextPageParam: (last) => last.next_before ?? undefined`; keyset pagination confirmed server-side, handlers/queries.zig:103-114). A background refetch then refetches all pages in order — consistent, no gap. The three behaviors the old code carried by hand: staleness discard is handled by the query itself; keep the `isPlaceholderData` disable on the button; the 401 branch is already covered by the global cache-level `handleUnauthorized` (queryClient.ts:29-30). Delete `extra`, `cursorOverride`, `generation`, `loadingMore`, `moreError`. Rewrite the five load-more tests; add one for the gap scenario: base refetch with new rows between page renders leaves no discontinuity. ### 18. Password hashing leaves the config lock `src/web/handlers/settings.zig`: `applyPut` holds `config_lock` from :286 to :341, and the argon2id call (t=2, m=19 MiB, :373-391) sits inside it at :312 — every settings GET (:407-409 takes the same lock) and every mutation stalls for the hash duration on the Pi 5. The login path already does it right: copy under lock, hash unlocked, re-check generation under lock (auth.zig:47-71), and the LiveHash generation check (auth.zig:73-79) already closes the concurrent-install race. Move the password-length check and the `hashPassword` call before the lock acquisition; everything from `loadConfig` on stays inside. The hash input is the parsed patch only, so nothing under the lock is needed. Tests: the existing applyPut suite passes unchanged; add one that actually distinguishes the new behavior — "both complete" already held before the fix, the GET merely waited out the hash. A stall seam under `builtin.is_test` (the milestone-15 rotation-seam shape) parks `hashPassword` on an event; the test starts a password PUT, waits until the hash is parked, completes a settings GET **while the hash is still parked**, then releases the seam and joins the PUT. ## Sessions S1-S5 run in parallel. One cross-session interface, fixed here: S4 adds the `WebState.udp_listeners`/`tcp_listeners` fields exactly as ruling 13 names them; S2 wires and consumes them without touching `web/server.zig`. ### Session S1: filter Owns `src/filter/manager.zig`, `src/filter/parsers.zig`, `src/filter/filter_integration_test.zig`. Rulings 1, 2, 3, 5. ### Session S2: cache, retention, counters, wiring Owns `src/cache/dns_cache.zig`, `src/storage/retention.zig`, `src/app.zig`, `src/server/udp_server.zig`, `src/server/tcp_server.zig`, `src/local/forward_client.zig`, `src/server/handler.zig`, `src/web/metrics.zig`, and the storage/server integration tests those touch. Rulings 4, 6, 12, 13 (except the WebState fields), 14. ### Session S3: TLS listeners Owns `src/platform/tls_server.zig`, `src/platform/logging.zig`, `src/server/doh_server.zig`. Rulings 9, 10, 15, 16. ### Session S4: web server Owns `src/web/server.zig`, `src/web/sse.zig`, `src/web/handlers/live.zig`, `src/web/handlers/settings.zig`, `src/web/web_integration_test.zig`, plus the ruling-13 field additions. Rulings 7, 11, 18. ### Session S5: frontend Owns `web/src/features/live/*`, `web/src/features/queries/*`. Rulings 8, 17. ### Orchestrator Strikes the closed findings in `TECH_DEBT.md`; updates the stale milestone-5 as-built line if ruling 5 changed its truth value. ## Acceptance (milestone complete) - [ ] Both `loadSource` catch sites propagate Canceled; the new unit test passes; no status row ever reads "Canceled". - [ ] `commitStatus` warns on an unknown id (test). - [ ] With a stalled download in flight, a concurrent rule mutation completes (integration test); the lock-ordering comment exists on both mutexes; a source deletion during a stalled refresh leaves the refresh's temp files alone (regression test). - [ ] A TC=1 response classifies as null (test); `validateResponse` is untouched. - [ ] The five sniffer cases pass; the URLhaus banner file parses as hosts. - [ ] A gated pass skips only the vacuum, counts `vacuums_gated`, and vacuums on the next allowed pass (test). - [ ] The 2 KiB-cookie tests pass: session extracted, non-session 401s, debug line emitted. - [ ] A fatal SSE rejection reaches `capped` and probes the session (frontend test); a transient error still takes three. - [ ] DoH `.timed_out` handshake counts `tls_handshake_failures`; the metrics name test still passes. - [ ] The new DoH idle test passes: quiet keep-alive closed, `idle_timeouts == 1`. - [ ] `Hub.close` wakes a parked subscriber promptly (test); web `deinit` calls it before `beginShutdown`. - [ ] The API limiter sweep runs in maintenance (test). - [ ] `/metrics` carries `nxdns_udp_server_*` and `nxdns_tcp_server_*` families summed over four listeners (name test extended). - [ ] `/metrics` carries the three new `nxdns_dns_forward_*` counters. - [ ] The TLS-server cause tests pass: Canceled surfaces as Canceled, never as a warn line; close_notify after a peer drop logs debug; `.tls_server` is in the dedup set. - [ ] The query log uses `useInfiniteQuery`; the gap-scenario test passes; the accumulation state is gone. - [ ] Hashing runs before `config_lock`; the settings suite passes. - [ ] Full suite green: `zig build test -Dintegration`, `npm run test`, `npm run typecheck`. ## Anti-requirements - No listener-core extraction, no shared repo helpers, no shared transport helpers — milestone 18. - No stats field renames (`accepted` vs `connections`) — milestone 18. - No config key renames and no trusted-proxy setting — milestone 17. - No `useInfiniteQuery` migration anywhere but the query log. - No new metrics beyond the families this spec names. - No timeout on DoH request bodies or handlers — only `receiveHead` races.