Final state persistence at shutdown depends on implicit fx hook ordering and is untested #114

New Issue

clawbot · 2026-08-09T07:06:08+02:00

clawbot commented

2026-08-09 07:06:08 +02:00

Surfaced while implementing #106 and deliberately kept out of that PR's scope.

The README promises (README.md:454-456) that shutdown persists final state to disk. It does — but not for the reason the README implies, and nothing verifies it.

Current state (audited against `origin/main`, commit `9347a28`)

The watcher does not save on its own shutdown path:

internal/watcher/watcher.go:93-98 — the watcher's OnStop hook only calls w.cancel().
internal/watcher/watcher.go:148-151 — the Run loop's ctx.Done() branch logs "watcher stopped" and returns. No saveState().

The final persist comes entirely from a different package's hook: internal/state/state.go:153-155 registers an OnStop that calls state.Save().

That works only because fx runs OnStop hooks in reverse registration order, and cmd/dnswatcher/main.go:41-47 happens to provide state.New before notify.New and watcher.New. So state's OnStop runs after the watcher's, and any in-memory mutation from the last check cycle is still captured.

The behaviour is correct today. The problems are that it is implicit and unverified:

Nothing documents that the guarantee rests on provider ordering in main.go. Reordering those fx.Provide lines — a change that looks entirely cosmetic — could silently invert the hook order and start losing the last cycle's observations.
There is no test asserting that a state mutation made during the final check cycle survives shutdown. internal/watcher's TestGracefulShutdown covers clean exit, not durability of the last write.
The README's phrasing attributes the persist to the shutdown sequence generally, which reads as though the watcher is responsible for it.

This is a durability property of a monitoring daemon — the last thing observed before a restart is exactly the baseline that determines whether the next run emits a false-positive change notification. It should not rest on an undocumented accident of declaration order.

Definition of done

The ordering guarantee is made explicit rather than incidental. Either:
- have the watcher persist state on its own shutdown path (in the ctx.Done() branch or its OnStop), so correctness no longer depends on another package's hook running afterwards; or
- keep the current split but add a comment at both internal/state/state.go:153-155 and internal/watcher/watcher.go:93-98 naming the dependency, plus a comment in cmd/dnswatcher/main.go warning that provider order is load-bearing.
Prefer the first — a guarantee enforced by code beats a guarantee enforced by a comment. If you pick it, make sure state is not saved twice in a way that could race; State.Save() takes the lock, but confirm.
A test asserts the durability property directly: run a check cycle that mutates state, trigger shutdown, and verify the mutation is present in the persisted file afterwards. This must fail if the save is removed.
If provider order in main.go remains load-bearing after your change, a test or comment must make that explicit enough that a future reorder is caught.
README wording updated so the shutdown description matches the actual mechanism.
make check green; make test wall time stays well under the 20-second ceiling; TODO.md updated in the same commit.

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

Hard constraints

DNS is never mocked in this repository. Note that PR #97 removes mockResolver and resolver.NewFromLoggerWithClient entirely and moves the watcher tests onto the real iterative resolver against live DNS, with TESTING.md's live-DNS policy applying to every package. Do not add, restore, or recreate any fake/stub DNS client, resolver, transport, or nameserver. The port checker, TLS checker, and notifier remain legitimate test doubles — they are not DNS.
Do not modify .golangci.yml; do not change the golangci-lint pin.

Sequencing

Blocked until PR #97 merges. #97 rewrites internal/watcher/watcher_test.go wholesale (+409/-483), and the test required by item 2 lands in that file. Starting before #97 merges guarantees the test is destroyed in the rebase.

Also sequenced after #106 (notification drain), which touches the same shutdown sequence from the internal/notify side.

Surfaced while implementing #106 and deliberately kept out of that PR's scope. The README promises (`README.md:454-456`) that shutdown persists final state to disk. It does — but not for the reason the README implies, and nothing verifies it. ## Current state (audited against `origin/main`, commit `9347a28`) The watcher does **not** save on its own shutdown path: - `internal/watcher/watcher.go:93-98` — the watcher's `OnStop` hook only calls `w.cancel()`. - `internal/watcher/watcher.go:148-151` — the `Run` loop's `ctx.Done()` branch logs `"watcher stopped"` and returns. No `saveState()`. The final persist comes entirely from a **different package's** hook: `internal/state/state.go:153-155` registers an `OnStop` that calls `state.Save()`. That works only because fx runs `OnStop` hooks in reverse registration order, and `cmd/dnswatcher/main.go:41-47` happens to provide `state.New` before `notify.New` and `watcher.New`. So state's `OnStop` runs *after* the watcher's, and any in-memory mutation from the last check cycle is still captured. The behaviour is correct today. The problems are that it is **implicit** and **unverified**: 1. Nothing documents that the guarantee rests on provider ordering in `main.go`. Reordering those `fx.Provide` lines — a change that looks entirely cosmetic — could silently invert the hook order and start losing the last cycle's observations. 2. There is no test asserting that a state mutation made during the final check cycle survives shutdown. `internal/watcher`'s `TestGracefulShutdown` covers clean exit, not durability of the last write. 3. The README's phrasing attributes the persist to the shutdown sequence generally, which reads as though the watcher is responsible for it. This is a durability property of a monitoring daemon — the last thing observed before a restart is exactly the baseline that determines whether the *next* run emits a false-positive change notification. It should not rest on an undocumented accident of declaration order. ## Definition of done 1. The ordering guarantee is made explicit rather than incidental. Either: - have the watcher persist state on its own shutdown path (in the `ctx.Done()` branch or its `OnStop`), so correctness no longer depends on another package's hook running afterwards; **or** - keep the current split but add a comment at both `internal/state/state.go:153-155` and `internal/watcher/watcher.go:93-98` naming the dependency, plus a comment in `cmd/dnswatcher/main.go` warning that provider order is load-bearing. Prefer the first — a guarantee enforced by code beats a guarantee enforced by a comment. If you pick it, make sure state is not saved twice in a way that could race; `State.Save()` takes the lock, but confirm. 2. A test asserts the durability property directly: run a check cycle that mutates state, trigger shutdown, and verify the mutation is present in the persisted file afterwards. This must fail if the save is removed. 3. If provider order in `main.go` remains load-bearing after your change, a test or comment must make that explicit enough that a future reorder is caught. 4. README wording updated so the shutdown description matches the actual mechanism. 5. `make check` green; `make test` wall time stays well under the 20-second ceiling; `TODO.md` updated in the same commit. The finishing commit's title must end with ` (closes #N)` referencing this issue. ## Hard constraints - **DNS is never mocked in this repository.** Note that [PR #97](https://git.eeqj.de/sneak/dnswatcher/pulls/97) removes `mockResolver` and `resolver.NewFromLoggerWithClient` entirely and moves the watcher tests onto the real iterative resolver against live DNS, with `TESTING.md`'s live-DNS policy applying to every package. Do not add, restore, or recreate any fake/stub DNS client, resolver, transport, or nameserver. The port checker, TLS checker, and notifier remain legitimate test doubles — they are not DNS. - Do not modify `.golangci.yml`; do not change the golangci-lint pin. ## Sequencing **Blocked until PR #97 merges.** #97 rewrites `internal/watcher/watcher_test.go` wholesale (+409/-483), and the test required by item 2 lands in that file. Starting before #97 merges guarantees the test is destroyed in the rebase. Also sequenced after #106 (notification drain), which touches the same shutdown sequence from the `internal/notify` side.

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

clawbot referenced this issue

2026-08-09 07:35:34 +02:00

1.0/mvp #66

clawbot referenced this issue

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

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

Sign in to join this conversation.