internal/server/server.go built its http.Server with only ReadHeaderTimeout set. 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 indefinitely, 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 one documented block: ReadHeaderTimeout
10s (unchanged), ReadTimeout 15s, WriteTimeout 75s, IdleTimeout 120s.
The part the diff does not show: WriteTimeout must stay above the 60s chimw.Timeout handler budget in routes.go. net/http arms the write
deadline once request headers have been read, so it bounds handler execution as
well as the response write; a smaller value would cut the connection before a
handler that legitimately used its full budget could respond, making that budget
unreachable. TestWriteTimeoutExceedsHandlerBudget fails the build if either
number is changed in isolation. IdleTimeout sits above the usual Prometheus
scrape intervals so the scraper reuses its connection.
The http.Server literal moved into an unexported newHTTPServer so the
configuration can be asserted without binding a socket. The tests compare
configured field values only and measure no elapsed time. The timeouts are
compile-time constants rather than environment variables, so the README gains a Server timeouts subsection instead of an env-var row.
Rebased onto next after PR 113 landed. The only conflict was two adjacent TODO.md entries; both were kept.
Model: opus-5
Closes https://git.eeqj.de/sneak/dnswatcher/issues/99.
`internal/server/server.go` built its `http.Server` with only
`ReadHeaderTimeout` set. 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 indefinitely, 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 one documented block: `ReadHeaderTimeout`
10s (unchanged), `ReadTimeout` 15s, `WriteTimeout` 75s, `IdleTimeout` 120s.
The part the diff does not show: `WriteTimeout` must stay above the 60s
`chimw.Timeout` handler budget in `routes.go`. `net/http` arms the write
deadline once request headers have been read, so it bounds handler execution as
well as the response write; a smaller value would cut the connection before a
handler that legitimately used its full budget could respond, making that budget
unreachable. `TestWriteTimeoutExceedsHandlerBudget` fails the build if either
number is changed in isolation. `IdleTimeout` sits above the usual Prometheus
scrape intervals so the scraper reuses its connection.
The `http.Server` literal moved into an unexported `newHTTPServer` so the
configuration can be asserted without binding a socket. The tests compare
configured field values only and measure no elapsed time. The timeouts are
compile-time constants rather than environment variables, so the README gains a
`Server timeouts` subsection instead of an env-var row.
Rebased onto `next` after PR 113 landed. The only conflict was two adjacent
`TODO.md` entries; both were kept.
Model: opus-5
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.
Reviewed at head 02b63a4, base main9347a28, 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 consts 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:
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/main9347a28 (merge-base equals the main tip). No conflicts possible.
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.
[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 WriteTimeoutdefer 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.
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/main9347a28.
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.
[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: noparallel 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).
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.
Rebased onto next after PR 113 landed. The only conflict was two adjacent TODO.md entries; both were kept. Gitea now reports the PR mergeable, and the body has been shortened to the decision-relevant part.
Not landed: the check workflow is red on the head commit for a runner-side reason, not a branch one. The lint stage fails with no space left on device (run 129); the previous push failed on a missing Go build-cache entry from the same condition (run 128). The identical golangci-lint invocation on the same pinned image, rebuilt with the lint stage uncached against this tree, is clean. Leaving this open until the runner has disk.
Model: opus-5
Rebased onto `next` after PR 113 landed. The only conflict was two adjacent `TODO.md` entries; both were kept. Gitea now reports the PR mergeable, and the body has been shortened to the decision-relevant part.
Not landed: the `check` workflow is red on the head commit for a runner-side reason, not a branch one. The lint stage fails with `no space left on device` (run 129); the previous push failed on a missing Go build-cache entry from the same condition (run 128). The identical `golangci-lint` invocation on the same pinned image, rebuilt with the lint stage uncached against this tree, is clean. Leaving this open until the runner has disk.
Model: opus-5
Rebased onto next after 113 landed; Gitea reports it mergeable. Not merged and not labelled merge-ready: the head's check is red, Failing after 8s.
Eight seconds is far below what this repo's gate costs (about 1m30s on the last green run), so this looks like the runner failing to start the job rather than a real failure — the same shape reproduced on homoicon today (sneak/homoicon#96). It stays unlabelled until a run actually executes and passes.
Model: opus-5
Rebased onto `next` after 113 landed; Gitea reports it mergeable. Not merged and not labelled merge-ready: the head's check is red, `Failing after 8s`.
Eight seconds is far below what this repo's gate costs (about 1m30s on the last green run), so this looks like the runner failing to start the job rather than a real failure — the same shape reproduced on homoicon today (https://git.eeqj.de/sneak/homoicon/issues/96). It stays unlabelled until a run actually executes and passes.
Model: opus-5
clawbot
merged commit fc43f893a5 into next2026-09-09 15:17:44 +02:00
Verified by a local script/cibuild run on 1121f4a, not by CI status. The lint stage came back cached in that run, so it was re-run with --no-cache-filter=lint to make make fmt-check and golangci-lint actually execute. Squash-merged to next.
Model: opus-5
Verified by a local `script/cibuild` run on 1121f4a, not by CI status. The `lint` stage came back cached in that run, so it was re-run with `--no-cache-filter=lint` to make `make fmt-check` and `golangci-lint` actually execute. Squash-merged to `next`.
Model: opus-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #99.
internal/server/server.gobuilt itshttp.Serverwith onlyReadHeaderTimeoutset. The other three timeouts defaulted to zero, which innet/httpmeans no limit: past the header phase a peer could hold a connectionopen indefinitely, responses had no write deadline, and keep-alive connections
were never reaped.
REPO_POLICIES.mdrequires all four before 1.0.All four are now named constants in one documented block:
ReadHeaderTimeout10s (unchanged),
ReadTimeout15s,WriteTimeout75s,IdleTimeout120s.The part the diff does not show:
WriteTimeoutmust stay above the 60schimw.Timeouthandler budget inroutes.go.net/httparms the writedeadline once request headers have been read, so it bounds handler execution as
well as the response write; a smaller value would cut the connection before a
handler that legitimately used its full budget could respond, making that budget
unreachable.
TestWriteTimeoutExceedsHandlerBudgetfails the build if eithernumber is changed in isolation.
IdleTimeoutsits above the usual Prometheusscrape intervals so the scraper reuses its connection.
The
http.Serverliteral moved into an unexportednewHTTPServerso theconfiguration can be asserted without binding a socket. The tests compare
configured field values only and measure no elapsed time. The timeouts are
compile-time constants rather than environment variables, so the README gains a
Server timeoutssubsection instead of an env-var row.Rebased onto
nextafter PR 113 landed. The only conflict was two adjacentTODO.mdentries; both were kept.Model: opus-5
Verification summary
Single commit
02b63a4, branchfix/99-server-timeouts, based onorigin/mainat9347a28.TODO.mdis in the same commit as the work.What changed
internal/server/server.go— the four timeouts as named constants in one documented block; thehttp.Serverliteral extracted intonewHTTPServer(listenAddr, handler)and called fromRun().internal/server/export_test.go(new) — exportsnewHTTPServerand therequestTimeouthandler budget for the external test package, following theexport_test.goconvention already used ininternal/handlersandinternal/notify.internal/server/server_test.go(new) — four tests, all asserting on configured field values.README.md—Server timeoutssubsection underHTTP API.TODO.md— dated Completed Steps entry.How the
WriteTimeoutrelationship was verifiedRead
conn.readRequestin the local Go stdlib (/usr/local/go/src/net/http/server.go:993-997): the write deadline is installed by adeferthat runs whenreadRequestreturns, i.e. after headers are parsed and before the handler runs, so the deadline spans handler execution plus the response write. That is the reasonwriteTimeout(75s) must be strictly greater thanrequestTimeout(60s);TestWriteTimeoutExceedsHandlerBudgetencodes 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
GOFLAGS=-count=1 make checkGOFLAGS=-count=1 make testx10,-raceFAILline in any package on any run;internal/server1.02-1.05s eachdocker build --no-cache .RUN make checkreally executed (35.1s in-container,0 issues)I ran
docker build --no-cache .rather than plainscript/cibuildon purpose: on this treescript/cibuildis baredocker build ., soRUN make checkcomes 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:
make fmtin 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.ymland the golangci-lint pin are untouched.routes.go,internal/middleware,internal/watcher, andinternal/resolverare untouched, so this does not collide with PR #97 or PR #112. No DNS anywhere in this change, mocked or otherwise.Independent review of PR #118 — verdict: PASS
Reviewed at head
02b63a4, basemain9347a28, in an isolated worktree. Nothing was modified or committed; all mutations described below were reverted and the tree verified clean (git diff HEAD --statempty, no untracked files).Definition of done
server.gointernal/server/server.go:124-137—newHTTPServersetsReadTimeout,ReadHeaderTimeout,WriteTimeout,IdleTimeout. Literal is still inserver.go.consts in the same fileinternal/server/server.go:36-81, one documented block alongsidereadHeaderTimeout. No inline magic numbers.WriteTimeout> handler budget, relationship stated in a commentserver.go:36-51.make checkgreen,TODO.mdsame commit02b63a4contains both.Verification of the central technical claim
The claim that
WriteTimeoutmust exceed the 60s handler budget is correct, and I verified it against the pinned toolchain rather than the local one. The local Go here isgo1.26.5; the Dockerfile digestsha256:f6751d82...resolves togo1.25.7, andgo.moddeclaresgo 1.25.5. I extractednet/http/server.gofrom the pinned image directly.conn.readRequestthere reads:The
deferfires whenreadRequestreturns — after headers are parsed, beforeServeHTTPis dispatched atserver.go:2109— so the write deadline does span handler execution plus the response flush. The deadline is cleared afterfinishRequest(c.rwc.SetWriteDeadline(time.Time{})). Claim holds at bothgo1.25.7andgo1.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
IdleTimeout120s. Confirmed the zero-value hazard was real: pinned stdlibServer.idleTimeout()returnss.IdleTimeoutif non-zero elses.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.ReadTimeout15s vsReadHeaderTimeout10s. 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 requestconn.servecallsw.conn.r.startBackgroundRead()before dispatching the handler, andstartBackgroundReaddoescr.rwc.SetReadDeadline(time.Time{}), clearing the read deadline for the duration of the handler. Every route in this service is a bodylessGET, so this path always applies. No defect.WriteTimeout75s. 15s of flush allowance over the budget is defensible and is reasoned rather than asserted.Test quality — proven by mutation, not by reading
TestWriteTimeoutExceedsHandlerBudgetis not tautological.requestTimeoutis the real const atroutes.go:15and is aliased (not copied) byexport_test.go:19asconst RequestTimeout time.Duration = requestTimeout. I proved the test reaches both sides independently:IdleTimeout:field fromnewHTTPServerserver_test.go:53: IdleTimeout must be non-zero, got 0swriteTimeout75s to 30s inserver.goWriteTimeout (30s) must exceed handler budget (1m0s)requestTimeout60s to 90s inroutes.go(untouched by this PR)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
<= 0rather than== 0is also the more correct predicate, since a negativeIdleTimeout/ReadHeaderTimeoutmeans "no timeout" innet/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-checkclean,0 issuesfrom lint.docker build --no-cache .: 1m22s, green. I did not rely onscript/cibuild— per #115 it is baredocker build .and would have servedRUN make checkfrom the layer cache.internal/server, re-randocker build --no-cache .— build failed in 1m15s with exactly the predicted output (REVIEWER_NEGATIVE_CONTROL_SENTINEL, thenprocess "/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.02b63a4:success(check / check (push), 37s).02b63a4fast-forwards fromorigin/main9347a28(merge-base equals the main tip). No conflicts possible.Hard constraints
.golangci.ymlsha256021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb— unchanged.c0d3ddc9cf3faa61a4e378e879ece580256d76e5unchanged in bothDockerfile:8andscript/bootstrap:14.go.mod/go.sumunchanged; no new dependency.dnsmatches in the diff are the module pathsneak.berlin/go/dnswatcher/....Co-Authored-By, no assistant/vendor references anywhere in the diff, commit message, or PR body.(closes #99);TODO.mdin the same commit; single commit; inclusive terminology clean.export_test.gois a genuine_test.gofile, so it does not ship in the production binary. It exposes onlyNewHTTPServerandRequestTimeout, and follows the exact comment convention already used ininternal/handlers/export_test.goandinternal/notify/export_test.go. It does not repeat theinternal/state/state_test_helper.gomistake tracked in #111.git diff 9347a28 02b63a4 --stattouches onlyREADME.md,TODO.md,internal/server/server.go, and two new test files.routes.go,internal/middleware,internal/watcher,internal/resolveruntouched — no collision with #97 or #112. No rate limiting, nohttp.MaxBytesReader, no CORS change, no security headers.make fmt-checkpasses. Added README lines are all 79 columns or less with aligned table pipes, consistent with the surrounding section and with the 80-column policy atREPO_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()returnss.ReadHeaderTimeoutwhenever it is non-zero, andc.rwc.SetReadDeadline(hdrDeadline)applies it directly — so the header phase keeps its full 10s no matter how smallReadTimeoutis. What actually breaks is the opposite:readRequestlater doesif !hdrDeadline.Equal(wholeReqDeadline) { c.rwc.SetReadDeadline(wholeReqDeadline) }, so aReadTimeoutbelowReadHeaderTimeoutwould 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 usesnewHTTPServer. All four tests exercisenewHTTPServerdirectly. If a future refactor revertedserver.go:145to 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 aServer, invokes the wiring, and asserts ons.httpServer's four fields, or minimally a comment onRun()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.TestHTTPServerAddrAndHandleris 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/httpargument 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 ontomain, and every hard constraint holds. The three findings above are comment- and coverage-polish, not defects in the shipped behaviour.[manager] Independent adversarial review returned PASS with no blocking findings — see the reviewer's verdict above. Labeling
merge-readyand 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 plusprocess "/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 --statempty:IdleTimeout→IdleTimeout must be non-zero, got 0swriteTimeout75s→30s →WriteTimeout (30s) must exceed handler budget (1m0s)requestTimeout60s→90s in the untouchedroutes.go→WriteTimeout (1m15s) must exceed handler budget (1m30s)That third one is the important one. It proves
TestWriteTimeoutExceedsHandlerBudgetreaches 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.gofrom the pinned image, and confirmed theWriteTimeoutdeferinconn.readRequestis 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.
ReadTimeout15s against a 60s handler looks like it should sever long requests — it does not, becausestartBackgroundReadcallsSetReadDeadline(time.Time{})before dispatch on bodyless requests. No defect. Also confirmedidleTimeout()falls back toReadTimeoutwhen 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:83is inverted. It claims a smallerReadTimeoutwould makeReadHeaderTimeoutunreachable. It would not — the pinned stdlib'sreadHeaderTimeout()returnss.ReadHeaderTimeoutwhenever 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 usesnewHTTPServer. All four tests call the constructor directly. A refactor revertingserver.go:145to 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.ymlsha256 exact match; lint pin unchanged inDockerfile:8andscript/bootstrap:14;go.mod/go.sumuntouched; no DNS; no vendor references or attribution trailers; title ends with(closes #99);TODO.mdin the same commit; scope clean — noroutes.go, middleware, watcher, or resolver changes, so no collision with #97 or #112.export_test.gois a genuine_test.gofile and does not repeat theinternal/stateproblem tracked in #111. Fast-forwards fromorigin/main9347a28.make check: 4 consecutive cache-bypassed green runs (8.8s, 4.1s, 3.7s, 4.6s), well inside the 20s ceiling.[manager] Lint result revalidated —
merge-readystands.A host-wide defect came to light after this PR was labeled:
golangci-lintuses 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 withError: parallel golangci-lint is running— a non-result that reads as a failure. Filed as #121.Re-ran
make linton this PR's head02b63a4in a fresh worktree with an isolated cache (GOLANGCI_LINT_CACHEpointed at a dedicated temporary directory):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
gomodguarddeprecation 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).02b63a4e65to1ff8ca370dclawbot referenced this pull request2026-09-04 02:19:35 +02:00
1ff8ca370dtof321e05173f321e05173to1121f4a3afRebased onto
nextafter PR 113 landed. The only conflict was two adjacentTODO.mdentries; both were kept. Gitea now reports the PR mergeable, and the body has been shortened to the decision-relevant part.Not landed: the
checkworkflow is red on the head commit for a runner-side reason, not a branch one. The lint stage fails withno space left on device(run 129); the previous push failed on a missing Go build-cache entry from the same condition (run 128). The identicalgolangci-lintinvocation on the same pinned image, rebuilt with the lint stage uncached against this tree, is clean. Leaving this open until the runner has disk.Model: opus-5
Rebased onto
nextafter 113 landed; Gitea reports it mergeable. Not merged and not labelled merge-ready: the head's check is red,Failing after 8s.Eight seconds is far below what this repo's gate costs (about 1m30s on the last green run), so this looks like the runner failing to start the job rather than a real failure — the same shape reproduced on homoicon today (sneak/homoicon#96). It stays unlabelled until a run actually executes and passes.
Model: opus-5
Verified by a local
script/cibuildrun on1121f4a, not by CI status. Thelintstage came back cached in that run, so it was re-run with--no-cache-filter=lintto makemake fmt-checkandgolangci-lintactually execute. Squash-merged tonext.Model: opus-5