server: set ReadTimeout, WriteTimeout, and IdleTimeout (closes #99) #118

clawbot · 2026-08-09T07:42:27+02:00

clawbot commented

2026-08-09 07:42:27 +02:00

Closes #99.

internal/server/server.go constructed its http.Server with only ReadHeaderTimeout set. ReadTimeout, WriteTimeout, and IdleTimeout defaulted to zero, which in net/http means no limit — past the header phase a peer could hold a connection open forever, responses had no write deadline, and keep-alive connections were never reaped. REPO_POLICIES.md requires all four before 1.0.

Values

Field	Value	Why
`ReadHeaderTimeout`	10s	unchanged
`ReadTimeout`	15s	whole request, headers + body
`WriteTimeout`	75s	handler execution + response flush
`IdleTimeout`	120s	keep-alive reaping

All four are named consts in a single documented block in server.go alongside the existing readHeaderTimeout.

ReadTimeout 15s — every route in this service is a bodyless GET, so the read deadline only ever needs to cover headers. The 5s over ReadHeaderTimeout is slack, not a real allowance; it exists so a body dribbled a byte at a time cannot hold the read side open indefinitely. It must be >= ReadHeaderTimeout or the header deadline becomes unreachable.
WriteTimeout 75s — must exceed the 60s chimw.Timeout(requestTimeout) handler budget. 60s + 15s of response-flush allowance.
IdleTimeout 120s — the only clients are browsers on the dashboard and a Prometheus scraper. 120s sits above the common scrape intervals (15s/30s/60s) so the scraper reuses its connection rather than reconnecting every cycle, while an abandoned connection is still reaped inside two minutes.

The `WriteTimeout` vs handler-budget relationship

net/http arms the write deadline in a deferred call at the end of conn.readRequest — i.e. once the request headers have been read — so on a plaintext connection it covers handler execution and the response write, not just the write:

// go/src/net/http/server.go, conn.readRequest
if d := c.server.WriteTimeout; d &gt; 0 {
    defer func() {
        c.rwc.SetWriteDeadline(time.Now().Add(d))
    }()
}

If writeTimeout were <= requestTimeout the server would sever the connection before a handler that legitimately consumed its full 60s budget could emit anything, making that budget unreachable in practice. The const block's comment states this, and TestWriteTimeoutExceedsHandlerBudget pins it so a future edit to either number fails the build rather than silently breaking the invariant.

Structure

The http.Server literal moved into an unexported newHTTPServer(listenAddr, handler) in the same file, called from Run(). This keeps the literal in server.go (DoD 1) while making the configuration assertable without binding a socket.

Tests

New internal/server/export_test.go (matching the existing export_test.go convention in internal/handlers and internal/notify) and internal/server/server_test.go in package server_test:

TestHTTPServerTimeoutsAreSet — all four fields non-zero (DoD 4).
TestWriteTimeoutExceedsHandlerBudget — WriteTimeout > requestTimeout.
TestReadTimeoutCoversHeaderTimeout — ReadTimeout >= ReadHeaderTimeout.
TestHTTPServerAddrAndHandler — the constructor cannot drop the address or handler.

Every assertion compares configured field values. Nothing measures elapsed time, so these cannot flake the way the two recent duration-asserting tests did.

Docs

The timeouts are compile-time constants, not env vars, so per DoD 5 there is nothing to add to the README env-var table. A Server timeouts subsection under HTTP API documents the four values and the handler-budget relationship so they are discoverable. TODO.md is updated in the same commit as the work.

Verification

make check green: 4.8s wall with GOFLAGS=-count=1 (3.1s warm). Well under the 20s policy ceiling.
GOFLAGS=-count=1 make test run 10 consecutive times under -race with the cache bypassed: 10 pass, 0 fail, no FAIL line in any package on any run.
docker build --no-cache . (not bare script/cibuild, which would have been served from the layer cache and reported a false green — see #115): real build, 1m20s total, with RUN make check genuinely executing in-container for 35.1s and reporting 0 issues.
.golangci.yml untouched: sha256sum still 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb. golangci-lint pin unchanged.

Scope

Confined to internal/server/server.go plus new test files, README, and TODO.md. routes.go, internal/middleware, internal/watcher, and internal/resolver are untouched, so there is no conflict with PR #97 or PR #112. Security headers (#98), rate limiting (#100), http.MaxBytesReader (#101), and CORS scoping are not folded in. No DNS is involved and none is mocked.

Closes #99. `internal/server/server.go` constructed its `http.Server` with only `ReadHeaderTimeout` set. `ReadTimeout`, `WriteTimeout`, and `IdleTimeout` defaulted to zero, which in `net/http` means *no limit* — past the header phase a peer could hold a connection open forever, responses had no write deadline, and keep-alive connections were never reaped. `REPO_POLICIES.md` requires all four before 1.0. ## Values | Field | Value | Why | |---|---|---| | `ReadHeaderTimeout` | 10s | unchanged | | `ReadTimeout` | 15s | whole request, headers + body | | `WriteTimeout` | 75s | handler execution + response flush | | `IdleTimeout` | 120s | keep-alive reaping | All four are named `const`s in a single documented block in `server.go` alongside the existing `readHeaderTimeout`. - **`ReadTimeout` 15s** — every route in this service is a bodyless `GET`, so the read deadline only ever needs to cover headers. The 5s over `ReadHeaderTimeout` is slack, not a real allowance; it exists so a body dribbled a byte at a time cannot hold the read side open indefinitely. It must be >= `ReadHeaderTimeout` or the header deadline becomes unreachable. - **`WriteTimeout` 75s** — must exceed the 60s `chimw.Timeout(requestTimeout)` handler budget. 60s + 15s of response-flush allowance. - **`IdleTimeout` 120s** — the only clients are browsers on the dashboard and a Prometheus scraper. 120s sits above the common scrape intervals (15s/30s/60s) so the scraper reuses its connection rather than reconnecting every cycle, while an abandoned connection is still reaped inside two minutes. ## The `WriteTimeout` vs handler-budget relationship `net/http` arms the write deadline in a deferred call at the end of `conn.readRequest` — i.e. once the request headers have been read — so on a plaintext connection it covers handler execution *and* the response write, not just the write: ```go // go/src/net/http/server.go, conn.readRequest if d := c.server.WriteTimeout; d > 0 { defer func() { c.rwc.SetWriteDeadline(time.Now().Add(d)) }() } ``` If `writeTimeout` were <= `requestTimeout` the server would sever the connection before a handler that legitimately consumed its full 60s budget could emit anything, making that budget unreachable in practice. The const block's comment states this, and `TestWriteTimeoutExceedsHandlerBudget` pins it so a future edit to either number fails the build rather than silently breaking the invariant. ## Structure The `http.Server` literal moved into an unexported `newHTTPServer(listenAddr, handler)` in the same file, called from `Run()`. This keeps the literal in `server.go` (DoD 1) while making the configuration assertable without binding a socket. ## Tests New `internal/server/export_test.go` (matching the existing `export_test.go` convention in `internal/handlers` and `internal/notify`) and `internal/server/server_test.go` in `package server_test`: - `TestHTTPServerTimeoutsAreSet` — all four fields non-zero (DoD 4). - `TestWriteTimeoutExceedsHandlerBudget` — `WriteTimeout` > `requestTimeout`. - `TestReadTimeoutCoversHeaderTimeout` — `ReadTimeout` >= `ReadHeaderTimeout`. - `TestHTTPServerAddrAndHandler` — the constructor cannot drop the address or handler. Every assertion compares configured field values. Nothing measures elapsed time, so these cannot flake the way the two recent duration-asserting tests did. ## Docs The timeouts are compile-time constants, not env vars, so per DoD 5 there is nothing to add to the README env-var table. A `Server timeouts` subsection under `HTTP API` documents the four values and the handler-budget relationship so they are discoverable. `TODO.md` is updated in the same commit as the work. ## Verification - `make check` green: 4.8s wall with `GOFLAGS=-count=1` (3.1s warm). Well under the 20s policy ceiling. - `GOFLAGS=-count=1 make test` run 10 consecutive times under `-race` with the cache bypassed: **10 pass, 0 fail**, no `FAIL` line in any package on any run. - `docker build --no-cache .` (not bare `script/cibuild`, which would have been served from the layer cache and reported a false green — see #115): real build, **1m20s** total, with `RUN make check` genuinely executing in-container for 35.1s and reporting `0 issues`. - `.golangci.yml` untouched: `sha256sum` still `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. golangci-lint pin unchanged. ## Scope Confined to `internal/server/server.go` plus new test files, README, and `TODO.md`. `routes.go`, `internal/middleware`, `internal/watcher`, and `internal/resolver` are untouched, so there is no conflict with PR #97 or PR #112. Security headers (#98), rate limiting (#100), `http.MaxBytesReader` (#101), and CORS scoping are not folded in. No DNS is involved and none is mocked.

clawbot added the needs-review label 2026-08-09 07:42:27 +02:00

clawbot self-assigned this 2026-08-09 07:42:31 +02:00

clawbot commented

2026-08-09 07:42:48 +02:00

Verification summary

Single commit 02b63a4, branch fix/99-server-timeouts, based on origin/main at 9347a28. TODO.md is in the same commit as the work.

What changed

internal/server/server.go — the four timeouts as named constants in one documented block; the http.Server literal extracted into newHTTPServer(listenAddr, handler) and called from Run().
internal/server/export_test.go (new) — exports newHTTPServer and the requestTimeout handler budget for the external test package, following the export_test.go convention already used in internal/handlers and internal/notify.
internal/server/server_test.go (new) — four tests, all asserting on configured field values.
README.md — Server timeouts subsection under HTTP API.
TODO.md — dated Completed Steps entry.

How the WriteTimeout relationship was verified

Read conn.readRequest in the local Go stdlib (/usr/local/go/src/net/http/server.go:993-997): the write deadline is installed by a defer that runs when readRequest returns, i.e. after headers are parsed and before the handler runs, so the deadline spans handler execution plus the response write. That is the reason writeTimeout (75s) must be strictly greater than requestTimeout (60s); TestWriteTimeoutExceedsHandlerBudget encodes the invariant so it cannot regress silently. I deliberately did not write a test that starts a server and measures elapsed time — this repo has already produced two flaky duration-asserting tests, and the invariant is fully expressible as a comparison of two constants.

Runs

Check	Result
`GOFLAGS=-count=1 make check`	green, 4.8s wall (3.1s warm)
`GOFLAGS=-count=1 make test` x10, `-race`	10 pass / 0 fail; no `FAIL` line in any package on any run; `internal/server` 1.02-1.05s each
`docker build --no-cache .`	green, 1m20s; `RUN make check` really executed (35.1s in-container, `0 issues`)

I ran docker build --no-cache . rather than plain script/cibuild on purpose: on this tree script/cibuild is bare docker build ., so RUN make check comes back from the layer cache in under a second having run nothing (#115). The 1m20s figure above is a genuine cold build.

Not satisfied / caveats

Nothing in the definition of done is unmet. Two notes for the reviewer:

DoD 5 asks for README documentation "if it documents server tunables". These are compile-time constants rather than env vars, so the env-var table is unchanged; I added a prose subsection instead so the values are discoverable rather than leaving them undocumented.
make fmt in this repo formats Go only (gofmt -s, goimports); there is no prettier config, so the Markdown edits were hand-formatted to match the surrounding style.

.golangci.yml and the golangci-lint pin are untouched. routes.go, internal/middleware, internal/watcher, and internal/resolver are untouched, so this does not collide with PR #97 or PR #112. No DNS anywhere in this change, mocked or otherwise.

## Verification summary Single commit `02b63a4`, branch `fix/99-server-timeouts`, based on `origin/main` at `9347a28`. `TODO.md` is in the same commit as the work. **What changed** - `internal/server/server.go` — the four timeouts as named constants in one documented block; the `http.Server` literal extracted into `newHTTPServer(listenAddr, handler)` and called from `Run()`. - `internal/server/export_test.go` (new) — exports `newHTTPServer` and the `requestTimeout` handler budget for the external test package, following the `export_test.go` convention already used in `internal/handlers` and `internal/notify`. - `internal/server/server_test.go` (new) — four tests, all asserting on configured field values. - `README.md` — `Server timeouts` subsection under `HTTP API`. - `TODO.md` — dated Completed Steps entry. **How the `WriteTimeout` relationship was verified** Read `conn.readRequest` in the local Go stdlib (`/usr/local/go/src/net/http/server.go:993-997`): the write deadline is installed by a `defer` that runs when `readRequest` returns, i.e. after headers are parsed and before the handler runs, so the deadline spans handler execution plus the response write. That is the reason `writeTimeout` (75s) must be strictly greater than `requestTimeout` (60s); `TestWriteTimeoutExceedsHandlerBudget` encodes the invariant so it cannot regress silently. I deliberately did **not** write a test that starts a server and measures elapsed time — this repo has already produced two flaky duration-asserting tests, and the invariant is fully expressible as a comparison of two constants. **Runs** | Check | Result | |---|---| | `GOFLAGS=-count=1 make check` | green, 4.8s wall (3.1s warm) | | `GOFLAGS=-count=1 make test` x10, `-race` | 10 pass / 0 fail; no `FAIL` line in any package on any run; `internal/server` 1.02-1.05s each | | `docker build --no-cache .` | green, 1m20s; `RUN make check` really executed (35.1s in-container, `0 issues`) | I ran `docker build --no-cache .` rather than plain `script/cibuild` on purpose: on this tree `script/cibuild` is bare `docker build .`, so `RUN make check` comes back from the layer cache in under a second having run nothing (#115). The 1m20s figure above is a genuine cold build. **Not satisfied / caveats** Nothing in the definition of done is unmet. Two notes for the reviewer: - DoD 5 asks for README documentation "if it documents server tunables". These are compile-time constants rather than env vars, so the env-var table is unchanged; I added a prose subsection instead so the values are discoverable rather than leaving them undocumented. - `make fmt` in this repo formats Go only (`gofmt -s`, `goimports`); there is no prettier config, so the Markdown edits were hand-formatted to match the surrounding style. `.golangci.yml` and the golangci-lint pin are untouched. `routes.go`, `internal/middleware`, `internal/watcher`, and `internal/resolver` are untouched, so this does not collide with PR #97 or PR #112. No DNS anywhere in this change, mocked or otherwise.

clawbot referenced this pull request

2026-08-09 07:44:52 +02:00

fmt tooling is incomplete: fmt-check does not verify goimports, and no formatter covers Markdown #119

clawbot commented

2026-08-09 07:52:15 +02:00

Independent review of PR #118 — verdict: PASS

Reviewed at head 02b63a4, base main 9347a28, in an isolated worktree. Nothing was modified or committed; all mutations described below were reverted and the tree verified clean (git diff HEAD --stat empty, no untracked files).

Definition of done

DoD	Status	Evidence
1. All four fields set on the literal in `server.go`	met	`internal/server/server.go:124-137` — `newHTTPServer` sets `ReadTimeout`, `ReadHeaderTimeout`, `WriteTimeout`, `IdleTimeout`. Literal is still in `server.go`.
2. Named `const`s in the same file	met	`internal/server/server.go:36-81`, one documented block alongside `readHeaderTimeout`. No inline magic numbers.
3. `WriteTimeout` > handler budget, relationship stated in a comment	met	75s > 60s; relationship stated at `server.go:36-51`.
4. Test asserts all four non-zero	met, proven by mutation	see below
5. README documents the values	met	prose subsection; timeouts are compile-time consts, so the env-var table is correctly left alone.
6. `make check` green, `TODO.md` same commit	met	single commit `02b63a4` contains both.

Verification of the central technical claim

The claim that WriteTimeout must exceed the 60s handler budget is correct, and I verified it against the pinned toolchain rather than the local one. The local Go here is go1.26.5; the Dockerfile digest sha256:f6751d82... resolves to go1.25.7, and go.mod declares go 1.25.5. I extracted net/http/server.go from the pinned image directly. conn.readRequest there reads:

c.rwc.SetReadDeadline(hdrDeadline)
if d := c.server.WriteTimeout; d &gt; 0 {
    defer func() {
        c.rwc.SetWriteDeadline(time.Now().Add(d))
    }()
}

The defer fires when readRequest returns — after headers are parsed, before ServeHTTP is dispatched at server.go:2109 — so the write deadline does span handler execution plus the response flush. The deadline is cleared after finishRequest (c.rwc.SetWriteDeadline(time.Time{})). Claim holds at both go1.25.7 and go1.26.5; the code is byte-identical at those lines. The author's citation was against the unpinned local stdlib, but the conclusion is unaffected.

Coherence of the four values

IdleTimeout 120s. Confirmed the zero-value hazard was real: pinned stdlib Server.idleTimeout() returns s.IdleTimeout if non-zero else s.ReadTimeout. Before this change both were zero, so there was genuinely no idle reaping at all. Now correctly non-zero. 120s above 15/30/60s scrape intervals is sound.
ReadTimeout 15s vs ReadHeaderTimeout 10s. The ordering is correct and 15s is sufficient. I specifically checked the obvious failure mode — whether a 15s read deadline could kill a handler legitimately running for 60s — and it cannot: for a bodyless request conn.serve calls w.conn.r.startBackgroundRead() before dispatching the handler, and startBackgroundRead does cr.rwc.SetReadDeadline(time.Time{}), clearing the read deadline for the duration of the handler. Every route in this service is a bodyless GET, so this path always applies. No defect.
WriteTimeout 75s. 15s of flush allowance over the budget is defensible and is reasoned rather than asserted.

Test quality — proven by mutation, not by reading

TestWriteTimeoutExceedsHandlerBudget is not tautological. requestTimeout is the real const at routes.go:15 and is aliased (not copied) by export_test.go:19 as const RequestTimeout time.Duration = requestTimeout. I proved the test reaches both sides independently:

Mutation	Result
Delete the `IdleTimeout:` field from `newHTTPServer`	FAIL — `server_test.go:53: IdleTimeout must be non-zero, got 0s`
Lower `writeTimeout` 75s to 30s in `server.go`	FAIL — `WriteTimeout (30s) must exceed handler budget (1m0s)`
Raise `requestTimeout` 60s to 90s in `routes.go` (untouched by this PR)	FAIL — `WriteTimeout (1m15s) must exceed handler budget (1m30s)`

The third mutation is the decisive one: the test catches drift originating in a different file that this PR does not modify, so the invariant is genuinely pinned rather than restated. All three reverted, tree clean.

Declining to write a timing-based test was the right call given this repo's flake history, and the invariant is fully expressible as a value comparison. Using <= 0 rather than == 0 is also the more correct predicate, since a negative IdleTimeout/ReadHeaderTimeout means "no timeout" in net/http.

Gate results (run by me, not taken on trust)

GOFLAGS=-count=1 make check: green 4 consecutive runs, 8.8s / 4.1s / 3.7s / 4.6s. make fmt-check clean, 0 issues from lint.
docker build --no-cache .: 1m22s, green. I did not rely on script/cibuild — per #115 it is bare docker build . and would have served RUN make check from the layer cache.
Negative control: planted a deliberately failing test in internal/server, re-ran docker build --no-cache . — build failed in 1m15s with exactly the predicted output (REVIEWER_NEGATIVE_CONTROL_SENTINEL, then process "/bin/sh -c make check" did not complete successfully: exit code: 2). A cached layer cannot produce a predicted failure, so the suite provably executes in the containerised build. Sentinel file removed; tree clean.
CI on 02b63a4: success (check / check (push), 37s).
Mergeable: 02b63a4 fast-forwards from origin/main 9347a28 (merge-base equals the main tip). No conflicts possible.

Hard constraints

.golangci.yml sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb — unchanged.
golangci-lint pin c0d3ddc9cf3faa61a4e378e879ece580256d76e5 unchanged in both Dockerfile:8 and script/bootstrap:14.
go.mod / go.sum unchanged; no new dependency.
No DNS involved and none mocked — the only dns matches in the diff are the module path sneak.berlin/go/dnswatcher/....
No attribution trailers, no Co-Authored-By, no assistant/vendor references anywhere in the diff, commit message, or PR body.
Commit title ends with (closes #99); TODO.md in the same commit; single commit; inclusive terminology clean.
export_test.go is a genuine _test.go file, so it does not ship in the production binary. It exposes only NewHTTPServer and RequestTimeout, and follows the exact comment convention already used in internal/handlers/export_test.go and internal/notify/export_test.go. It does not repeat the internal/state/state_test_helper.go mistake tracked in #111.
Scope clean: git diff 9347a28 02b63a4 --stat touches only README.md, TODO.md, internal/server/server.go, and two new test files. routes.go, internal/middleware, internal/watcher, internal/resolver untouched — no collision with #97 or #112. No rate limiting, no http.MaxBytesReader, no CORS change, no security headers.
Config-fails-loudly-at-startup: not applicable; these are compile-time constants, and no config parsing was added.
Markdown: make fmt-check passes. Added README lines are all 79 columns or less with aligned table pipes, consistent with the surrounding section and with the 80-column policy at REPO_POLICIES.md:177. The absent prettier tooling is a repo gap, not a defect of this PR.

Non-blocking findings

internal/server/server_test.go:83 — the stated rationale is backwards. The comment says "a smaller ReadTimeout would make ReadHeaderTimeout unreachable". Per the pinned stdlib, readHeaderTimeout() returns s.ReadHeaderTimeout whenever it is non-zero, and c.rwc.SetReadDeadline(hdrDeadline) applies it directly — so the header phase keeps its full 10s no matter how small ReadTimeout is. What actually breaks is the opposite: readRequest later does if !hdrDeadline.Equal(wholeReqDeadline) { c.rwc.SetReadDeadline(wholeReqDeadline) }, so a ReadTimeout below ReadHeaderTimeout would install an already-expired whole-request deadline and kill any body read instantly. The asserted invariant (ReadTimeout >= ReadHeaderTimeout) is right and worth pinning; only the explanation is inverted. The same inverted wording appears in the PR body and in the plan comment on #99. Acceptable would be: "a smaller ReadTimeout would install an already-expired whole-request deadline once the headers are read." Comment-only; no behavioural impact.
Nothing pins that Run() actually uses newHTTPServer. All four tests exercise newHTTPServer directly. If a future refactor reverted server.go:145 to an inline &http.Server{...} literal without the timeouts, every test in this PR would still pass and the exposure would silently return — which is precisely the regression DoD 4 exists to prevent. The extraction into a constructor is the right structure; the test just stops one call short. Acceptable would be a test that constructs a Server, invokes the wiring, and asserts on s.httpServer's four fields, or minimally a comment on Run() noting the constructor is the single source of truth. Not blocking, since the current tests do satisfy DoD 4 as written and the mutation results above show they bite.
TestHTTPServerAddrAndHandler is low value. It asserts a two-field struct literal copies its own arguments. Harmless, cheap, and it does document intent, but it is close to testing the compiler.

Verdict

PASS. The change does what it claims, the central net/http argument is correct at the pinned toolchain, the values are reasoned rather than asserted, the tests are non-vacuous under mutation, the containerised suite provably runs, CI is green, the branch fast-forwards onto main, and every hard constraint holds. The three findings above are comment- and coverage-polish, not defects in the shipped behaviour.

## Independent review of PR #118 — verdict: PASS Reviewed at head `02b63a4`, base `main` `9347a28`, in an isolated worktree. Nothing was modified or committed; all mutations described below were reverted and the tree verified clean (`git diff HEAD --stat` empty, no untracked files). ### Definition of done | DoD | Status | Evidence | |---|---|---| | 1. All four fields set on the literal in `server.go` | met | `internal/server/server.go:124-137` — `newHTTPServer` sets `ReadTimeout`, `ReadHeaderTimeout`, `WriteTimeout`, `IdleTimeout`. Literal is still in `server.go`. | | 2. Named `const`s in the same file | met | `internal/server/server.go:36-81`, one documented block alongside `readHeaderTimeout`. No inline magic numbers. | | 3. `WriteTimeout` > handler budget, relationship stated in a comment | met | 75s > 60s; relationship stated at `server.go:36-51`. | | 4. Test asserts all four non-zero | met, proven by mutation | see below | | 5. README documents the values | met | prose subsection; timeouts are compile-time consts, so the env-var table is correctly left alone. | | 6. `make check` green, `TODO.md` same commit | met | single commit `02b63a4` contains both. | ### Verification of the central technical claim The claim that `WriteTimeout` must exceed the 60s handler budget is **correct**, and I verified it against the *pinned* toolchain rather than the local one. The local Go here is `go1.26.5`; the Dockerfile digest `sha256:f6751d82...` resolves to `go1.25.7`, and `go.mod` declares `go 1.25.5`. I extracted `net/http/server.go` from the pinned image directly. `conn.readRequest` there reads: ```go c.rwc.SetReadDeadline(hdrDeadline) if d := c.server.WriteTimeout; d > 0 { defer func() { c.rwc.SetWriteDeadline(time.Now().Add(d)) }() } ``` The `defer` fires when `readRequest` returns — after headers are parsed, before `ServeHTTP` is dispatched at `server.go:2109` — so the write deadline does span handler execution plus the response flush. The deadline is cleared after `finishRequest` (`c.rwc.SetWriteDeadline(time.Time{})`). Claim holds at both `go1.25.7` and `go1.26.5`; the code is byte-identical at those lines. The author's citation was against the unpinned local stdlib, but the conclusion is unaffected. ### Coherence of the four values - **`IdleTimeout` 120s.** Confirmed the zero-value hazard was real: pinned stdlib `Server.idleTimeout()` returns `s.IdleTimeout` if non-zero **else `s.ReadTimeout`**. Before this change both were zero, so there was genuinely no idle reaping at all. Now correctly non-zero. 120s above 15/30/60s scrape intervals is sound. - **`ReadTimeout` 15s vs `ReadHeaderTimeout` 10s.** The ordering is correct and 15s is sufficient. I specifically checked the obvious failure mode — whether a 15s read deadline could kill a handler legitimately running for 60s — and it **cannot**: for a bodyless request `conn.serve` calls `w.conn.r.startBackgroundRead()` before dispatching the handler, and `startBackgroundRead` does `cr.rwc.SetReadDeadline(time.Time{})`, clearing the read deadline for the duration of the handler. Every route in this service is a bodyless `GET`, so this path always applies. No defect. - **`WriteTimeout` 75s.** 15s of flush allowance over the budget is defensible and is reasoned rather than asserted. ### Test quality — proven by mutation, not by reading `TestWriteTimeoutExceedsHandlerBudget` is **not** tautological. `requestTimeout` is the real const at `routes.go:15` and is aliased (not copied) by `export_test.go:19` as `const RequestTimeout time.Duration = requestTimeout`. I proved the test reaches both sides independently: | Mutation | Result | |---|---| | Delete the `IdleTimeout:` field from `newHTTPServer` | **FAIL** — `server_test.go:53: IdleTimeout must be non-zero, got 0s` | | Lower `writeTimeout` 75s to 30s in `server.go` | **FAIL** — `WriteTimeout (30s) must exceed handler budget (1m0s)` | | Raise `requestTimeout` 60s to 90s in `routes.go` (untouched by this PR) | **FAIL** — `WriteTimeout (1m15s) must exceed handler budget (1m30s)` | The third mutation is the decisive one: the test catches drift originating in a *different file* that this PR does not modify, so the invariant is genuinely pinned rather than restated. All three reverted, tree clean. Declining to write a timing-based test was the right call given this repo's flake history, and the invariant is fully expressible as a value comparison. Using `<= 0` rather than `== 0` is also the more correct predicate, since a negative `IdleTimeout`/`ReadHeaderTimeout` means "no timeout" in `net/http`. ### Gate results (run by me, not taken on trust) - `GOFLAGS=-count=1 make check`: green **4 consecutive runs**, 8.8s / 4.1s / 3.7s / 4.6s. `make fmt-check` clean, `0 issues` from lint. - `docker build --no-cache .`: **1m22s**, green. I did not rely on `script/cibuild` — per #115 it is bare `docker build .` and would have served `RUN make check` from the layer cache. - **Negative control:** planted a deliberately failing test in `internal/server`, re-ran `docker build --no-cache .` — build failed in **1m15s** with exactly the predicted output (`REVIEWER_NEGATIVE_CONTROL_SENTINEL`, then `process "/bin/sh -c make check" did not complete successfully: exit code: 2`). A cached layer cannot produce a predicted failure, so the suite provably executes in the containerised build. Sentinel file removed; tree clean. - CI on `02b63a4`: `success` (`check / check (push)`, 37s). - Mergeable: `02b63a4` fast-forwards from `origin/main` `9347a28` (merge-base equals the main tip). No conflicts possible. ### Hard constraints - `.golangci.yml` sha256 `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb` — unchanged. - golangci-lint pin `c0d3ddc9cf3faa61a4e378e879ece580256d76e5` unchanged in both `Dockerfile:8` and `script/bootstrap:14`. - `go.mod` / `go.sum` unchanged; no new dependency. - No DNS involved and none mocked — the only `dns` matches in the diff are the module path `sneak.berlin/go/dnswatcher/...`. - No attribution trailers, no `Co-Authored-By`, no assistant/vendor references anywhere in the diff, commit message, or PR body. - Commit title ends with ` (closes #99)`; `TODO.md` in the same commit; single commit; inclusive terminology clean. - `export_test.go` is a genuine `_test.go` file, so it does not ship in the production binary. It exposes only `NewHTTPServer` and `RequestTimeout`, and follows the exact comment convention already used in `internal/handlers/export_test.go` and `internal/notify/export_test.go`. It does **not** repeat the `internal/state/state_test_helper.go` mistake tracked in #111. - Scope clean: `git diff 9347a28 02b63a4 --stat` touches only `README.md`, `TODO.md`, `internal/server/server.go`, and two new test files. `routes.go`, `internal/middleware`, `internal/watcher`, `internal/resolver` untouched — no collision with #97 or #112. No rate limiting, no `http.MaxBytesReader`, no CORS change, no security headers. - Config-fails-loudly-at-startup: not applicable; these are compile-time constants, and no config parsing was added. - Markdown: `make fmt-check` passes. Added README lines are all 79 columns or less with aligned table pipes, consistent with the surrounding section and with the 80-column policy at `REPO_POLICIES.md:177`. The absent prettier tooling is a repo gap, not a defect of this PR. ### Non-blocking findings 1. **`internal/server/server_test.go:83` — the stated rationale is backwards.** The comment says "a smaller ReadTimeout would make ReadHeaderTimeout unreachable". Per the pinned stdlib, `readHeaderTimeout()` returns `s.ReadHeaderTimeout` whenever it is non-zero, and `c.rwc.SetReadDeadline(hdrDeadline)` applies it directly — so the header phase keeps its full 10s no matter how small `ReadTimeout` is. What actually breaks is the opposite: `readRequest` later does `if !hdrDeadline.Equal(wholeReqDeadline) { c.rwc.SetReadDeadline(wholeReqDeadline) }`, so a `ReadTimeout` below `ReadHeaderTimeout` would install an already-expired whole-request deadline and kill any body read instantly. The asserted invariant (`ReadTimeout >= ReadHeaderTimeout`) is right and worth pinning; only the explanation is inverted. The same inverted wording appears in the PR body and in the plan comment on #99. Acceptable would be: "a smaller ReadTimeout would install an already-expired whole-request deadline once the headers are read." Comment-only; no behavioural impact. 2. **Nothing pins that `Run()` actually uses `newHTTPServer`.** All four tests exercise `newHTTPServer` directly. If a future refactor reverted `server.go:145` to an inline `&http.Server{...}` literal without the timeouts, every test in this PR would still pass and the exposure would silently return — which is precisely the regression DoD 4 exists to prevent. The extraction into a constructor is the right structure; the test just stops one call short. Acceptable would be a test that constructs a `Server`, invokes the wiring, and asserts on `s.httpServer`'s four fields, or minimally a comment on `Run()` noting the constructor is the single source of truth. Not blocking, since the current tests do satisfy DoD 4 as written and the mutation results above show they bite. 3. **`TestHTTPServerAddrAndHandler` is low value.** It asserts a two-field struct literal copies its own arguments. Harmless, cheap, and it does document intent, but it is close to testing the compiler. ### Verdict **PASS.** The change does what it claims, the central `net/http` argument is correct at the pinned toolchain, the values are reasoned rather than asserted, the tests are non-vacuous under mutation, the containerised suite provably runs, CI is green, the branch fast-forwards onto `main`, and every hard constraint holds. The three findings above are comment- and coverage-polish, not defects in the shipped behaviour.

clawbot commented

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

[manager] Independent adversarial review returned PASS with no blocking findings — see the reviewer's verdict above. Labeling merge-ready and assigning to @sneak.

This is the most rigorously verified PR in the 1.0 series so far, and the verification methodology is worth recording because it is now the standard here.

What made this review conclusive

A negative control on the Docker gate. The reviewer planted a failing test, ran docker build --no-cache ., and got a failure in 1m15s carrying the predicted sentinel plus process "/bin/sh -c make check" did not complete successfully: exit code: 2. Then reverted and confirmed the tree was clean. A cached layer cannot produce a specifically predicted failure, so this is proof the suite ran — not the inference from wall-clock time that #115 showed can be wrong.

Mutation testing that reached across files. Three mutations, all failing as required, all reverted with git diff HEAD --stat empty:

dropping IdleTimeout → IdleTimeout must be non-zero, got 0s
writeTimeout 75s→30s → WriteTimeout (30s) must exceed handler budget (1m0s)
requestTimeout 60s→90s in the untouched routes.go → WriteTimeout (1m15s) must exceed handler budget (1m30s)

That third one is the important one. It proves TestWriteTimeoutExceedsHandlerBudget reaches the real cross-file constant rather than a local copy — which is exactly the tautology I asked the reviewer to rule out.

The stdlib claim was checked against the pinned toolchain, not the local one. The author cited local Go; the reviewer resolved the Dockerfile digest to go1.25.7 (local is go1.26.5), extracted net/http/server.go from the pinned image, and confirmed the WriteTimeout defer in conn.readRequest is byte-identical at both versions and does span handler execution. The conclusion holds, but it now holds on evidence from the toolchain that actually builds this.

The obvious attack failed cleanly. ReadTimeout 15s against a 60s handler looks like it should sever long requests — it does not, because startBackgroundRead calls SetReadDeadline(time.Time{}) before dispatch on bodyless requests. No defect. Also confirmed idleTimeout() falls back to ReadTimeout when zero, meaning the pre-existing hole this PR closes was real: both were zero.

Also worth noting the author's judgement call was right — they deliberately wrote no timing-based test, citing this repo's two recent flaky duration-asserting tests, and asserted configured values instead. That is the correct lesson to have drawn from #113.

Non-blocking findings — tracked, not forgiven

Filed as #120. Two of the three deserve comment:

The rationale comment at internal/server/server_test.go:83 is inverted. It claims a smaller ReadTimeout would make ReadHeaderTimeout unreachable. It would not — the pinned stdlib's readHeaderTimeout() returns s.ReadHeaderTimeout whenever non-zero and applies it directly, so the header phase keeps its full 10s. What actually breaks is the whole-request deadline, which gets installed already-expired. The asserted invariant is correct; only the explanation is backwards. I am letting this merge rather than spending a rework-plus-fresh-review cycle on a comment, but it is not cosmetic: a confidently wrong explanation of subtle stdlib deadline semantics is the kind of thing a future maintainer reasons from, and the same wording appears in the PR body and in the plan comment on #99.

Nothing pins that Run() actually uses newHTTPServer. All four tests call the constructor directly. A refactor reverting server.go:145 to an inline &http.Server{...} without timeouts would leave every test green. That is a partial miss of DoD item 4, whose stated purpose was "so a future refactor cannot silently drop one" — the tests protect the constructor, not the call site. I am recording it as a miss rather than pretending the contract was fully met.

Neither justifies blocking a correct, well-evidenced change, but both are on the 1.0 milestone.

Constraints verified

.golangci.yml sha256 exact match; lint pin unchanged in Dockerfile:8 and script/bootstrap:14; go.mod/go.sum untouched; no DNS; no vendor references or attribution trailers; title ends with (closes #99); TODO.md in the same commit; scope clean — no routes.go, middleware, watcher, or resolver changes, so no collision with #97 or #112. export_test.go is a genuine _test.go file and does not repeat the internal/state problem tracked in #111. Fast-forwards from origin/main 9347a28.

make check: 4 consecutive cache-bypassed green runs (8.8s, 4.1s, 3.7s, 4.6s), well inside the 20s ceiling.

**[manager]** Independent adversarial review returned **PASS** with no blocking findings — see the reviewer's verdict above. Labeling `merge-ready` and assigning to @sneak. This is the most rigorously verified PR in the 1.0 series so far, and the verification methodology is worth recording because it is now the standard here. ## What made this review conclusive **A negative control on the Docker gate.** The reviewer planted a failing test, ran `docker build --no-cache .`, and got a failure in 1m15s carrying the *predicted sentinel* plus `process "/bin/sh -c make check" did not complete successfully: exit code: 2`. Then reverted and confirmed the tree was clean. A cached layer cannot produce a specifically predicted failure, so this is proof the suite ran — not the inference from wall-clock time that #115 showed can be wrong. **Mutation testing that reached across files.** Three mutations, all failing as required, all reverted with `git diff HEAD --stat` empty: - dropping `IdleTimeout` → `IdleTimeout must be non-zero, got 0s` - `writeTimeout` 75s→30s → `WriteTimeout (30s) must exceed handler budget (1m0s)` - `requestTimeout` 60s→90s **in the untouched `routes.go`** → `WriteTimeout (1m15s) must exceed handler budget (1m30s)` That third one is the important one. It proves `TestWriteTimeoutExceedsHandlerBudget` reaches the real cross-file constant rather than a local copy — which is exactly the tautology I asked the reviewer to rule out. **The stdlib claim was checked against the *pinned* toolchain, not the local one.** The author cited local Go; the reviewer resolved the Dockerfile digest to **go1.25.7** (local is go1.26.5), extracted `net/http/server.go` from the pinned image, and confirmed the `WriteTimeout` `defer` in `conn.readRequest` is byte-identical at both versions and does span handler execution. The conclusion holds, but it now holds on evidence from the toolchain that actually builds this. **The obvious attack failed cleanly.** `ReadTimeout` 15s against a 60s handler looks like it should sever long requests — it does not, because `startBackgroundRead` calls `SetReadDeadline(time.Time{})` before dispatch on bodyless requests. No defect. Also confirmed `idleTimeout()` falls back to `ReadTimeout` when zero, meaning the pre-existing hole this PR closes was real: both were zero. Also worth noting the author's judgement call was right — they deliberately wrote **no timing-based test**, citing this repo's two recent flaky duration-asserting tests, and asserted configured values instead. That is the correct lesson to have drawn from #113. ## Non-blocking findings — tracked, not forgiven Filed as **#120**. Two of the three deserve comment: **The rationale comment at `internal/server/server_test.go:83` is inverted.** It claims a smaller `ReadTimeout` would make `ReadHeaderTimeout` unreachable. It would not — the pinned stdlib's `readHeaderTimeout()` returns `s.ReadHeaderTimeout` whenever non-zero and applies it directly, so the header phase keeps its full 10s. What actually breaks is the *whole-request* deadline, which gets installed already-expired. **The asserted invariant is correct; only the explanation is backwards.** I am letting this merge rather than spending a rework-plus-fresh-review cycle on a comment, but it is not cosmetic: a confidently wrong explanation of subtle stdlib deadline semantics is the kind of thing a future maintainer reasons *from*, and the same wording appears in the PR body and in the plan comment on #99. **Nothing pins that `Run()` actually uses `newHTTPServer`.** All four tests call the constructor directly. A refactor reverting `server.go:145` to an inline `&http.Server{...}` without timeouts would leave every test green. That is a **partial miss of DoD item 4**, whose stated purpose was "so a future refactor cannot silently drop one" — the tests protect the constructor, not the call site. I am recording it as a miss rather than pretending the contract was fully met. Neither justifies blocking a correct, well-evidenced change, but both are on the 1.0 milestone. ## Constraints verified `.golangci.yml` sha256 exact match; lint pin unchanged in `Dockerfile:8` and `script/bootstrap:14`; `go.mod`/`go.sum` untouched; no DNS; no vendor references or attribution trailers; title ends with ` (closes #99)`; `TODO.md` in the same commit; scope clean — no `routes.go`, middleware, watcher, or resolver changes, so **no collision with #97 or #112**. `export_test.go` is a genuine `_test.go` file and does **not** repeat the `internal/state` problem tracked in #111. Fast-forwards from `origin/main` `9347a28`. `make check`: 4 consecutive cache-bypassed green runs (8.8s, 4.1s, 3.7s, 4.6s), well inside the 20s ceiling.

clawbot added merge-ready and removed needs-review labels 2026-08-09 07:53:28 +02:00

clawbot removed their assignment 2026-08-09 07:53:29 +02:00

sneak was assigned by clawbot

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

clawbot referenced this pull request

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

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

clawbot commented

2026-08-09 08:04:42 +02:00

[manager] Lint result revalidated — merge-ready stands.

A host-wide defect came to light after this PR was labeled: golangci-lint uses a single shared cache and lock per user account, and many concurrent sessions on this machine lint different repositories from throwaway worktrees. A run on a sibling repo returned 399 issues attributed to a worktree path belonging to another session, and runs can also fail with Error: parallel golangci-lint is running — a non-result that reads as a failure. Filed as #121.

Re-ran make lint on this PR's head 02b63a4 in a fresh worktree with an isolated cache (GOLANGCI_LINT_CACHE pointed at a dedicated temporary directory):

0 issues.

Validity checked against both void conditions: no parallel golangci-lint is running, and no file paths outside the worktree it ran in. Sound result; label unaffected.

The reviewer's Docker negative control and cross-file mutation tests are unaffected by this defect — a planted failing test surfacing its predicted sentinel is test execution, not lint, and no lint cache can fabricate it. That evidence stands as recorded.

The only other output was a pre-existing gomodguard deprecation warning, unrelated to this change — now tracked in #123 (corrected from an earlier version of this comment, which cited the wrong number; #122 is a pull request).

**[manager] Lint result revalidated — `merge-ready` stands.** A host-wide defect came to light after this PR was labeled: `golangci-lint` uses a single shared cache and lock per user account, and many concurrent sessions on this machine lint different repositories from throwaway worktrees. A run on a sibling repo returned **399 issues attributed to a worktree path belonging to another session**, and runs can also fail with `Error: parallel golangci-lint is running` — a non-result that reads as a failure. Filed as #121. **Re-ran `make lint` on this PR's head `02b63a4` in a fresh worktree with an isolated cache** (`GOLANGCI_LINT_CACHE` pointed at a dedicated temporary directory): ``` 0 issues. ``` Validity checked against both void conditions: **no** `parallel golangci-lint is running`, and **no** file paths outside the worktree it ran in. Sound result; label unaffected. The reviewer's Docker negative control and cross-file mutation tests are unaffected by this defect — a planted failing *test* surfacing its predicted sentinel is test execution, not lint, and no lint cache can fabricate it. That evidence stands as recorded. The only other output was a pre-existing `gomodguard` deprecation warning, unrelated to this change — now tracked in **#123** (corrected from an earlier version of this comment, which cited the wrong number; #122 is a pull request).

clawbot referenced this pull request

2026-08-09 08:05:15 +02:00

DECISION NEEDED: gomodguard linter is deprecated, and .golangci.yml can only be changed by you #123

clawbot referenced this pull request

2026-08-09 08:47:33 +02:00

DECISION/ACCESS NEEDED: the CI job log Gitea returns for a commit does not correspond to that commit #126

clawbot referenced this pull request

2026-08-09 16:56:48 +02:00

go.mod is untidy: golang.org/x/sync listed both direct and indirect, so script/bootstrap mutates a tracked file #132

clawbot referenced this pull request

2026-08-10 14:30:21 +02:00

TOP PRIORITY: consolidate all open PRs onto one next branch, one PR #135

clawbot changed title from ~~server: set ReadTimeout, WriteTimeout, and IdleTimeout (closes #99)~~ to WIP: server: set ReadTimeout, WriteTimeout, and IdleTimeout (closes #99)

2026-08-10 14:39:36 +02:00

clawbot added needs-rebase and removed merge-ready labels 2026-08-10 14:41:15 +02:00

sneak was unassigned by clawbot

2026-08-10 14:41:25 +02:00

clawbot self-assigned this 2026-08-10 14:41:25 +02:00

clawbot changed title from ~~WIP: server: set ReadTimeout, WriteTimeout, and IdleTimeout (closes #99)~~ to server: set ReadTimeout, WriteTimeout, and IdleTimeout (closes #99)

2026-08-10 15:20:49 +02:00

clawbot changed target branch from main to next

2026-08-10 15:20:49 +02:00

clawbot added 1 commit 2026-08-10 15:20:49 +02:00

server: set ReadTimeout, WriteTimeout, and IdleTimeout (closes #99 )

check / check (push) Successful in 37s

Details

02b63a4e65

The http.Server literal only set ReadHeaderTimeout. The other three
timeouts defaulted to zero, which in net/http means no limit: past the
header phase a peer could hold a connection open forever, responses had
no write deadline, and keep-alive connections were never reaped.
REPO_POLICIES.md requires all four before 1.0.

All four are now named constants in internal/server/server.go:

  ReadHeaderTimeout  10s  (unchanged)
  ReadTimeout        15s  whole request; every route is a bodyless GET
  WriteTimeout       75s  handler execution plus response flush
  IdleTimeout       120s  keep-alive reaping

WriteTimeout must exceed the 60s chimw.Timeout handler budget in
routes.go. net/http arms the write deadline once the request headers
have been read, so it covers handler execution as well as the response
write; a smaller value would sever the connection before a handler that
legitimately used its full budget could respond, making that budget
unreachable. The 15s difference is the response-flush allowance. The
comment on the const block states this relationship.

IdleTimeout sits above the common Prometheus scrape intervals so the
scraper reuses its connection instead of reconnecting each cycle, while
an abandoned connection is still reaped within two minutes.

The http.Server literal moved into newHTTPServer so the configuration
is testable without binding a socket. Tests assert all four fields are
non-zero, that WriteTimeout exceeds the handler budget, and that
ReadTimeout covers ReadHeaderTimeout; they compare configured values
only and measure no elapsed time, so they cannot flake.

check / check (push) Successful in 37s

Details

This pull request has changes conflicting with the target branch.

TODO.md

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/99-server-timeouts:fix/99-server-timeouts

git checkout fix/99-server-timeouts

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/dnswatcher#118

server: set ReadTimeout, WriteTimeout, and IdleTimeout (closes #99) #118

Values

The WriteTimeout vs handler-budget relationship

Structure

Tests

Docs

Verification

Scope

Verification summary

Independent review of PR #118 — verdict: PASS

Definition of done

Verification of the central technical claim

Coherence of the four values

Test quality — proven by mutation, not by reading

Gate results (run by me, not taken on trust)

Hard constraints

Non-blocking findings

Verdict

What made this review conclusive

Non-blocking findings — tracked, not forgiven

Constraints verified

Checkout

The `WriteTimeout` vs handler-budget relationship