Server timeout tests protect the constructor but not the call site, and carry an inverted rationale comment #120

New Issue

clawbot · 2026-08-09T07:53:56+02:00

clawbot commented

2026-08-09 07:53:56 +02:00

Non-blocking findings from the review of PR #118, collected here rather than spent on another review cycle. The timeout values themselves are correct and thoroughly verified — nothing here questions them.

Applies to internal/server/server.go and internal/server/server_test.go as they exist after #118 merges.

1. The regression test does not cover the regression it was written for

#99's definition of done, item 4, asked for a test asserting all four timeout fields are non-zero "so a future refactor cannot silently drop one."

The tests deliver half of that. All four call newHTTPServer(...) directly and assert on its output. Nothing asserts that Run() actually uses newHTTPServer. A refactor that reverts internal/server/server.go:145 to an inline &http.Server{Addr: ..., Handler: ...} — dropping every timeout — would leave the entire suite green, because the tests would still be exercising a constructor the running server no longer calls.

The protection is on the constructor; the risk is at the call site. This is a partial miss of the original contract, recorded rather than waved through.

2. The rationale comment is backwards

internal/server/server_test.go:83 explains the ReadTimeout >= ReadHeaderTimeout invariant by claiming a smaller ReadTimeout "would make ReadHeaderTimeout unreachable."

That is not what happens. Verified against the pinned toolchain (go1.25.7, resolved from the Dockerfile digest): readHeaderTimeout() returns s.ReadHeaderTimeout whenever it is non-zero and applies it directly, so the header phase keeps its full 10s regardless of ReadTimeout. What actually breaks is the whole-request deadline — if !hdrDeadline.Equal(wholeReqDeadline) { c.rwc.SetReadDeadline(wholeReqDeadline) } installs a deadline that has already expired.

The asserted invariant is correct. Only the explanation is wrong. That is precisely what makes it worth fixing: a confidently stated, incorrect account of subtle stdlib deadline semantics is what a future maintainer will reason from when deciding whether a change is safe.

The same wording appears in three places and all three need correcting: the test comment, the body of PR #118, and the plan comment on #99.

3. `TestHTTPServerAddrAndHandler` is near-tautological

It asserts that a two-field struct literal copies its two arguments. It cannot realistically fail. Either strengthen it into something that could, or drop it — a test that cannot fail is a maintenance cost that pays no rent.

Definition of done

A test pins that the server actually run by Run() carries the timeouts — i.e. the wiring, not just the constructor. Whatever shape you choose, it must fail if Run() is changed to build its http.Server inline without timeouts. That failure mode is the entire point; do not write something that merely re-asserts the constructor from a different angle.
The inverted rationale corrected in all three places, stating that a too-small ReadTimeout installs an already-expired whole-request deadline. Do not overcorrect into a new wrong claim — check it against the pinned toolchain in the Dockerfile, not whatever Go is on your machine, and cite what you read.
TestHTTPServerAddrAndHandler either strengthened or removed, with the choice justified in the PR description.
No timing-based assertions. #118 deliberately avoided them after this repo produced two separate flaky duration-asserting tests (see #113). Assert on configuration and wiring, never on elapsed time.
Prove item 1 by mutation. Revert Run() to an inline &http.Server{...} without timeouts, confirm the new test fails, revert, confirm git status is clean and make check is green. Report what you mutated and the exact failure it produced. A code-reading argument is not sufficient — the whole finding is that the existing tests look like they cover this and do not.
make check green; TODO.md updated in the same commit.

The finishing commit's title must end with (closes #N) referencing this issue.

Scope

Do not change any timeout value. ReadHeaderTimeout 10s, ReadTimeout 15s, WriteTimeout 75s, IdleTimeout 120s were each reasoned about and independently verified in #118 — including confirmation that ReadTimeout 15s does not sever a 60s handler, because startBackgroundRead clears the read deadline before dispatch on bodyless requests. If you believe a value is wrong, say so in the PR description with evidence; do not quietly adjust one.

Do not touch routes.go, internal/middleware, internal/watcher, or internal/resolver.

Sequencing

Blocked until PR #118 merges — every line referenced here only exists on that branch.

Hard constraints

DNS is never mocked in this repository. Nothing here involves DNS.
Do not modify .golangci.yml (sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb); do not change the golangci-lint pin.
make targets and script/ entrypoints only — no direct go test / go build / go clean / golangci-lint.
If you run the Docker gate, note script/cibuild can report a false green on an unchanged tree (#115). Force a real build and use a negative control.

Non-blocking findings from the review of [PR #118](https://git.eeqj.de/sneak/dnswatcher/pulls/118), collected here rather than spent on another review cycle. The timeout values themselves are correct and thoroughly verified — nothing here questions them. Applies to `internal/server/server.go` and `internal/server/server_test.go` as they exist after #118 merges. ## 1. The regression test does not cover the regression it was written for #99's definition of done, item 4, asked for a test asserting all four timeout fields are non-zero **"so a future refactor cannot silently drop one."** The tests deliver half of that. All four call `newHTTPServer(...)` directly and assert on its output. **Nothing asserts that `Run()` actually uses `newHTTPServer`.** A refactor that reverts `internal/server/server.go:145` to an inline `&http.Server{Addr: ..., Handler: ...}` — dropping every timeout — would leave the entire suite green, because the tests would still be exercising a constructor the running server no longer calls. The protection is on the constructor; the risk is at the call site. This is a partial miss of the original contract, recorded rather than waved through. ## 2. The rationale comment is backwards `internal/server/server_test.go:83` explains the `ReadTimeout` >= `ReadHeaderTimeout` invariant by claiming a smaller `ReadTimeout` **"would make `ReadHeaderTimeout` unreachable."** That is not what happens. Verified against the pinned toolchain (go1.25.7, resolved from the `Dockerfile` digest): `readHeaderTimeout()` returns `s.ReadHeaderTimeout` whenever it is non-zero and applies it directly, so the header phase keeps its full 10s regardless of `ReadTimeout`. What actually breaks is the **whole-request** deadline — `if !hdrDeadline.Equal(wholeReqDeadline) { c.rwc.SetReadDeadline(wholeReqDeadline) }` installs a deadline that has already expired. **The asserted invariant is correct. Only the explanation is wrong.** That is precisely what makes it worth fixing: a confidently stated, incorrect account of subtle stdlib deadline semantics is what a future maintainer will reason *from* when deciding whether a change is safe. The same wording appears in three places and all three need correcting: the test comment, the body of PR #118, and the plan comment on #99. ## 3. `TestHTTPServerAddrAndHandler` is near-tautological It asserts that a two-field struct literal copies its two arguments. It cannot realistically fail. Either strengthen it into something that could, or drop it — a test that cannot fail is a maintenance cost that pays no rent. ## Definition of done 1. A test pins that the server actually run by `Run()` carries the timeouts — i.e. the wiring, not just the constructor. Whatever shape you choose, it must **fail** if `Run()` is changed to build its `http.Server` inline without timeouts. That failure mode is the entire point; do not write something that merely re-asserts the constructor from a different angle. 2. The inverted rationale corrected in all three places, stating that a too-small `ReadTimeout` installs an already-expired whole-request deadline. Do not overcorrect into a new wrong claim — check it against the **pinned** toolchain in the `Dockerfile`, not whatever Go is on your machine, and cite what you read. 3. `TestHTTPServerAddrAndHandler` either strengthened or removed, with the choice justified in the PR description. 4. **No timing-based assertions.** #118 deliberately avoided them after this repo produced two separate flaky duration-asserting tests (see #113). Assert on configuration and wiring, never on elapsed time. 5. **Prove item 1 by mutation.** Revert `Run()` to an inline `&http.Server{...}` without timeouts, confirm the new test fails, revert, confirm `git status` is clean and `make check` is green. Report what you mutated and the exact failure it produced. A code-reading argument is not sufficient — the whole finding is that the existing tests look like they cover this and do not. 6. `make check` green; `TODO.md` updated in the same commit. The finishing commit's title must end with ` (closes #N)` referencing this issue. ## Scope **Do not change any timeout value.** `ReadHeaderTimeout` 10s, `ReadTimeout` 15s, `WriteTimeout` 75s, `IdleTimeout` 120s were each reasoned about and independently verified in #118 — including confirmation that `ReadTimeout` 15s does *not* sever a 60s handler, because `startBackgroundRead` clears the read deadline before dispatch on bodyless requests. If you believe a value is wrong, say so in the PR description with evidence; do not quietly adjust one. Do not touch `routes.go`, `internal/middleware`, `internal/watcher`, or `internal/resolver`. ## Sequencing **Blocked until PR #118 merges** — every line referenced here only exists on that branch. ## Hard constraints - DNS is never mocked in this repository. Nothing here involves DNS. - Do not modify `.golangci.yml` (sha256 `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`); do not change the golangci-lint pin. - `make` targets and `script/` entrypoints only — no direct `go test` / `go build` / `go clean` / `golangci-lint`. - If you run the Docker gate, note `script/cibuild` can report a false green on an unchanged tree (#115). Force a real build and use a negative control.

clawbot added this to the 1.0 milestone 2026-08-09 07:53:56 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/dnswatcher#120

Server timeout tests protect the constructor but not the call site, and carry an inverted rationale comment #120