config: stop startup on an invalid DNS or TLS interval (closes #177)

DNSWATCHER_DNS_INTERVAL and DNSWATCHER_TLS_INTERVAL were parsed with
time.ParseDuration and silently replaced by the default when that failed,
so a value like 5 or 1d gave hourly checks with no hint why, and zero or
negative values were accepted. Both now go through parseInterval, which
returns an error naming the variable and the value, and startup stops the
same way it does for invalid targets. An unset or empty variable still gets
its default from setupViper. The three tests that pinned the old fallback
are replaced. The README says what a valid value looks like.

Model: opus-5-5

Dockerfile

@@ -34,8 +34,11 @@ COPY . .
 # Run the tests - build fails if any test fails
 RUN make test
 
-# Build the binary
-RUN make build
+# Build the binary. .dockerignore leaves out .git, so `git describe` in
+# the Makefile cannot find the version here: script/docker passes it as
+# --build-arg VERSION, and a build that passes none reports `dev`.
+ARG VERSION=dev
+RUN make build VERSION="${VERSION}"
 
 # Runtime stage
 # alpine 3.21, 2026-02-28

Makefile

@@ -1,6 +1,8 @@
 .PHONY: all bootstrap setup build lint fmt fmt-check test check clean hooks docker
 
 BINARY := dnswatcher
+# `make build VERSION=...` overrides this; the Dockerfile does so, as the
+# image has no .git to describe.
 VERSION := $(shell git describe --tags --always --dirty 2>/dev/null || echo "dev")
 LDFLAGS := -X main.Version=$(VERSION)
 

README.md

@@ -320,8 +320,8 @@ the following precedence (highest to lowest):
 | `DNSWATCHER_SLACK_WEBHOOK`      | Slack incoming webhook URL                 | `""`        |
 | `DNSWATCHER_MATTERMOST_WEBHOOK` | Mattermost incoming webhook URL            | `""`        |
 | `DNSWATCHER_NTFY_TOPIC`         | ntfy topic URL                             | `""`        |
-| `DNSWATCHER_DNS_INTERVAL`       | DNS check interval                         | `1h`        |
-| `DNSWATCHER_TLS_INTERVAL`       | TLS check interval                         | `12h`       |
+| `DNSWATCHER_DNS_INTERVAL`       | DNS check interval, a positive duration such as `30m`; empty means the default, anything else stops startup | `1h` |
+| `DNSWATCHER_TLS_INTERVAL`       | TLS check interval, a positive duration such as `6h`; empty means the default, anything else stops startup | `12h` |
 | `DNSWATCHER_TLS_EXPIRY_WARNING` | Days before expiry to warn                 | `7`         |
 | `DNSWATCHER_SENTRY_DSN`         | Sentry DSN for error reporting             | `""`        |
 | `DNSWATCHER_MAINTENANCE_MODE`   | Enable maintenance mode                    | `false`     |
@@ -335,6 +335,14 @@ is a misconfiguration, so dnswatcher fails fast with a clear error message
 rather than running silently. Set `DNSWATCHER_TARGETS` to a comma-separated
 list of DNS names before starting.
 
+**`DNSWATCHER_DNS_INTERVAL` and `DNSWATCHER_TLS_INTERVAL`** take a positive
+duration: a number followed by a unit such as `s`, `m` or `h`, for example
+`90s`, `30m`, `1h` or `1h30m`. There is no unit for days; write `24h`. An
+unset or empty variable (`DNSWATCHER_DNS_INTERVAL=`) means the default. If
+either is set to anything else, including a bare number or a zero or negative
+duration, dnswatcher refuses to start with an error naming the variable and
+the value.
+
 ### Example `.env`
 
 ```sh
@@ -480,7 +488,8 @@ them. We provide:
 - `script/check` — run test, lint, and fmt-check
 - `script/docker` — build the Docker image tagged via `script/projectname`, with
   `--no-cache-filter=lint,builder` so the lint stage and the builder stage,
-  which runs the tests, run on every invocation
+  which runs the tests, run on every invocation, and with the version from
+  `git describe` passed as `--build-arg VERSION`
 - `script/cibuild` — CI entrypoint: `docker build` with
   `--no-cache-filter=lint,builder`, so the lint stage and the builder stage,
   which runs the tests, run on every invocation, because a cached build lints
@@ -502,11 +511,14 @@ make clean      # Remove build artifacts
 
 ### Build-Time Variables
 
-Version is injected via `-ldflags`:
+`make build` sets the version with `-ldflags "-X main.Version=..."`, taking
+it from `git describe --tags --always --dirty`, or from `VERSION` when given
+on the command line (`make build VERSION=1.2.3`). The version appears in the
+startup log and in the health check response.
 
-```sh
-go build -ldflags "-X main.Version=$(git describe --tags --always)" ./cmd/dnswatcher
-```
+The Docker image has no `.git`, so the `Dockerfile` takes the version as
+`--build-arg VERSION`. `make docker` passes it; a plain `docker build`
+passes none, and that image reports `dev`.
 
 ---
 

TODO.md

@@ -20,6 +20,14 @@ https://git.eeqj.de/sneak/dnswatcher/issues/104
 
 # Completed Steps
 
+- 2026-10-01: a `DNSWATCHER_DNS_INTERVAL` or `DNSWATCHER_TLS_INTERVAL` that is
+  not a positive duration stops startup; empty means the default (closes #177).
+- 2026-10-01: the image built by `make docker` reports the `git describe`
+  version, not `dev`, and the startup log now shows it (closes #109).
+- 2026-10-01: two notify shutdown tests always release the delivery they hold,
+  so a drain that returns early fails them instead of hanging (closes #176).
+- 2026-10-01: `script/install-precommit` asks git for the repository's git
+  directory, so `make hooks` also works where `.git` is a file (closes #129).
 - 2026-10-01: `TODO.md` brought up to date: open issues listed by URL, every
   Completed Steps entry cut to at most two lines (closes #146).
 - 2026-10-01: wildcard CORS now applies only to the public routes, not to
@@ -89,11 +97,8 @@ https://git.eeqj.de/sneak/dnswatcher/issues/104
 - nameserver IP address changes: https://git.eeqj.de/sneak/dnswatcher/issues/105
 - `DNSWATCHER_SENTRY_DSN` does nothing:
   https://git.eeqj.de/sneak/dnswatcher/issues/107
-- invalid DNS or TLS interval silently replaced by the default:
-  https://git.eeqj.de/sneak/dnswatcher/issues/177
 - rate limit on `/metrics` Basic Auth:
   https://git.eeqj.de/sneak/dnswatcher/issues/101
-- images report version `dev`: https://git.eeqj.de/sneak/dnswatcher/issues/109
 - trial run of the finished image:
   https://git.eeqj.de/sneak/dnswatcher/issues/149
 - 1.0 readiness: run it with a real config and read the logs:
@@ -101,12 +106,8 @@ https://git.eeqj.de/sneak/dnswatcher/issues/104
 - `goimports` in `make fmt-check`, Markdown formatting:
   https://git.eeqj.de/sneak/dnswatcher/issues/119
 - final state save at shutdown: https://git.eeqj.de/sneak/dnswatcher/issues/114
-- `internal/notify` shutdown tests hang when a drain returns early:
-  https://git.eeqj.de/sneak/dnswatcher/issues/176
 - README accuracy sweep: https://git.eeqj.de/sneak/dnswatcher/issues/108
 - README sections required by policy:
   https://git.eeqj.de/sneak/dnswatcher/issues/173
-- `script/install-precommit` in a linked worktree:
-  https://git.eeqj.de/sneak/dnswatcher/issues/129
 - fixed root server order: https://git.eeqj.de/sneak/dnswatcher/issues/138
 - review toward 1.0: https://git.eeqj.de/sneak/dnswatcher/issues/144

cmd/dnswatcher/main.go

@@ -63,6 +63,7 @@ func main() {
 				return n
 			},
 		),
+		fx.Invoke(func(l *logger.Logger) { l.Identify() }),
 		fx.Invoke(func(*server.Server, *watcher.Watcher) {}),
 	).Run()
 }

internal/config/config.go

@@ -28,6 +28,13 @@ var ErrNoTargets = errors.New(
 	"no monitoring targets configured: set DNSWATCHER_TARGETS environment variable",
 )
 
+// ErrInvalidInterval is returned when DNSWATCHER_DNS_INTERVAL or
+// DNSWATCHER_TLS_INTERVAL is set but is not a positive duration. An empty
+// value counts as unset and means the default.
+var ErrInvalidInterval = errors.New(
+	"interval must be a positive duration such as 30m or 1h",
+)
+
 // Params contains dependencies for Config.
 type Params struct {
 	fx.In
@@ -125,18 +132,14 @@ func buildConfig(
 		}
 	}
 
-	dnsInterval, err := time.ParseDuration(
-		viper.GetString("DNS_INTERVAL"),
-	)
+	dnsInterval, err := parseInterval("DNS_INTERVAL")
 	if err != nil {
-		dnsInterval = defaultDNSInterval
+		return nil, err
 	}
 
-	tlsInterval, err := time.ParseDuration(
-		viper.GetString("TLS_INTERVAL"),
-	)
+	tlsInterval, err := parseInterval("TLS_INTERVAL")
 	if err != nil {
-		tlsInterval = defaultTLSInterval
+		return nil, err
 	}
 
 	domains, hostnames, err := parseAndValidateTargets()
@@ -168,6 +171,22 @@ func buildConfig(
 	return cfg, nil
 }
 
+// parseInterval reads the DNSWATCHER_-prefixed setting key as a duration. A
+// value that does not parse, or is zero or negative, is an error naming the
+// variable and the value; an unset variable has its default from setupViper.
+func parseInterval(key string) (time.Duration, error) {
+	value := viper.GetString(key)
+
+	interval, err := time.ParseDuration(value)
+	if err != nil || interval <= 0 {
+		return 0, fmt.Errorf(
+			"invalid DNSWATCHER_%s %q: %w", key, value, ErrInvalidInterval,
+		)
+	}
+
+	return interval, nil
+}
+
 func parseAndValidateTargets() ([]string, []string, error) {
 	domains, hostnames, err := ClassifyTargets(
 		parseCSV(viper.GetString("TARGETS")),

internal/config/config_test.go

@@ -1,6 +1,7 @@
 package config_test
 
 import (
+	"strconv"
 	"testing"
 	"time"
 
@@ -113,33 +114,40 @@ func TestNew_OnlyEmptyCSVSegments(t *testing.T) {
 	assert.ErrorIs(t, err, config.ErrNoTargets)
 }
 
-func TestNew_InvalidDNSInterval_FallsBackToDefault(t *testing.T) {
+// TestNew_InvalidIntervalStopsStartup checks values that must stop startup;
+// TestNew_DefaultValues and TestNew_EmptyIntervalMeansDefault check that an
+// unset or empty interval means the default.
+func TestNew_InvalidIntervalStopsStartup(t *testing.T) {
+	variables := []string{"DNSWATCHER_DNS_INTERVAL", "DNSWATCHER_TLS_INTERVAL"}
+	values := []string{
+		"banana", // not a duration
+		"5",      // no unit
+		"1d",     // days are not a unit time.ParseDuration knows
+		"0",      // zero
+		"-1h",    // negative
+	}
+
+	for _, variable := range variables {
+		for _, value := range values {
+			t.Run(variable+"="+value, func(t *testing.T) {
 				viper.Reset()
 				t.Setenv("DNSWATCHER_TARGETS", "example.com")
-	t.Setenv("DNSWATCHER_DNS_INTERVAL", "banana")
+				t.Setenv(variable, value)
 
-	cfg, err := config.New(nil, newTestParams(t))
-	require.NoError(t, err)
-	assert.Equal(t, time.Hour, cfg.DNSInterval,
-		"invalid DNS interval should fall back to 1h default")
+				_, err := config.New(nil, newTestParams(t))
+				require.ErrorIs(t, err, config.ErrInvalidInterval)
+				require.ErrorContains(t, err, variable)
+				require.ErrorContains(t, err, strconv.Quote(value))
+			})
+		}
+	}
 }
 
-func TestNew_InvalidTLSInterval_FallsBackToDefault(t *testing.T) {
+func TestNew_EmptyIntervalMeansDefault(t *testing.T) {
 	viper.Reset()
 	t.Setenv("DNSWATCHER_TARGETS", "example.com")
-	t.Setenv("DNSWATCHER_TLS_INTERVAL", "notaduration")
-
-	cfg, err := config.New(nil, newTestParams(t))
-	require.NoError(t, err)
-	assert.Equal(t, 12*time.Hour, cfg.TLSInterval,
-		"invalid TLS interval should fall back to 12h default")
-}
-
-func TestNew_BothIntervalsInvalid(t *testing.T) {
-	viper.Reset()
-	t.Setenv("DNSWATCHER_TARGETS", "example.com")
-	t.Setenv("DNSWATCHER_DNS_INTERVAL", "xyz")
-	t.Setenv("DNSWATCHER_TLS_INTERVAL", "abc")
+	t.Setenv("DNSWATCHER_DNS_INTERVAL", "")
+	t.Setenv("DNSWATCHER_TLS_INTERVAL", "")
 
 	cfg, err := config.New(nil, newTestParams(t))
 	require.NoError(t, err)

internal/notify/shutdown_test.go

@@ -143,6 +143,12 @@ func TestDrainWaitsForInFlightDelivery(t *testing.T) {
 	srv := blockingNtfyServer(entered, release, &served)
 	defer srv.Close()
 
+	// srv.Close waits for the handler, so release it however the
+	// test ends; otherwise a drain that returns early hangs the
+	// package instead of failing this test.
+	releaseHandler := sync.OnceFunc(func() { close(release) })
+	defer releaseHandler()
+
 	topicURL, _ := url.Parse(srv.URL)
 
 	svc := notify.NewTestService(http.DefaultTransport)
@@ -167,9 +173,7 @@ func TestDrainWaitsForInFlightDelivery(t *testing.T) {
 	// delay alone.
 	start := time.Now()
 
-	timer := time.AfterFunc(inFlightHold, func() {
-		close(release)
-	})
+	timer := time.AfterFunc(inFlightHold, releaseHandler)
 	defer timer.Stop()
 
 	ctx, cancel := context.WithTimeout(
@@ -268,7 +272,7 @@ func TestDrainBoundedByContextDeadline(t *testing.T) {
 	// all never returns here (the delivery is parked in a backoff
 	// that never fires), so an unbounded drain must fail this
 	// test promptly instead of hanging the package until the test
-	// binary's 30s timeout.
+	// binary's -timeout.
 	returned := make(chan struct{})
 
 	go func() {
@@ -447,6 +451,11 @@ func TestNewRegistersDrainingStopHook(t *testing.T) {
 	srv := blockingNtfyServer(entered, release, &served)
 	defer srv.Close()
 
+	// As in TestDrainWaitsForInFlightDelivery: release the handler
+	// however the test ends, before srv.Close waits for it.
+	releaseHandler := sync.OnceFunc(func() { close(release) })
+	defer releaseHandler()
+
 	lifecycle := &recordingLifecycle{}
 	svc := newNotifyService(t, lifecycle, srv.URL)
 
@@ -472,9 +481,7 @@ func TestNewRegistersDrainingStopHook(t *testing.T) {
 		t.Fatal("delivery never reached the endpoint")
 	}
 
-	timer := time.AfterFunc(inFlightHold, func() {
-		close(release)
-	})
+	timer := time.AfterFunc(inFlightHold, releaseHandler)
 	defer timer.Stop()
 
 	ctx, cancel := context.WithTimeout(

script/docker

@@ -12,7 +12,15 @@ ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)"
 
 main() {
     cd "$ROOT"
-    docker build --no-cache-filter=lint,builder -t "$("$SCRIPT_DIR/projectname")" .
+    # Own line: a failing command substitution inside an argument does
+    # not trip `set -e`, so the inline form degrades silently to an
+    # empty constant. VERSION is computed here because .dockerignore
+    # excludes .git, so `git describe` in a build stage cannot find it.
+    version="$(git describe --tags --always --dirty 2>/dev/null || true)"
+    [ -n "$version" ] || version="unknown"
+    docker build --no-cache-filter=lint,builder \
+        --build-arg VERSION="$version" \
+        -t "$("$SCRIPT_DIR/projectname")" .
 }
 
 main "$@"

script/install-precommit

@@ -7,7 +7,20 @@ ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
 
 main() {
     cd "$ROOT"
-    hook=".git/hooks/pre-commit"
+    # Stop if this directory is not the top of its own git checkout, for
+    # example a copy inside another repository, whose hook must not be
+    # replaced.
+    if [ "$(git rev-parse --show-toplevel)" != "$ROOT" ]; then
+        echo "install-precommit: $ROOT is not the top of a git checkout" >&2
+        exit 1
+    fi
+    # Ask git for the repository's own git directory: .git is a file, not
+    # a directory, in some checkouts (for example a clone made with
+    # --separate-git-dir). core.hooksPath is deliberately not followed, so
+    # the hook is never written outside this repository.
+    hooks="$(git rev-parse --git-common-dir)/hooks"
+    mkdir -p "$hooks"
+    hook="$hooks/pre-commit"
     printf '#!/bin/sh\nset -e\nscript/precommit\n' > "$hook"
     chmod +x "$hook"
     echo "pre-commit hook installed: runs script/precommit"