# 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). ### 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.