In-flight notification goroutines are not awaited at shutdown, so alerts are lost #106

New Issue

clawbot · 2026-08-09T03:39:20+02:00

clawbot commented

2026-08-09 03:39:20 +02:00

The README promises (README.md:454-456):

> Shutdown: Persist final state to disk, complete in-flight notifications, stop gracefully.

Nothing in the codebase completes in-flight notifications. On shutdown the process can exit while a Slack / Mattermost / ntfy delivery is mid-retry, and that alert is silently lost.

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

Each backend dispatch fires an unmanaged goroutine — dispatchNtfy (internal/notify/notify.go:197-225), dispatchSlack (:227-255), dispatchMattermost (:257-285). Each uses context.WithoutCancel(ctx), which is deliberate and correct as far as it goes: it stops a cancelled request context from killing a delivery already under way.

But context.WithoutCancel only detaches the goroutine from cancellation. It does not make anything wait for it.

notify.New (internal/notify/notify.go:121-172) accepts an fx.Lifecycle parameter and never calls lifecycle.Append. There is no OnStop hook, no sync.WaitGroup, no drain step of any kind. So fx runs its OnStop hooks, main() returns, and the process exits — while retry loops are still sleeping.

The window is not small. internal/notify/retry.go:13-25 retries up to DefaultMaxRetries = 5 times with backoff delays up to DefaultMaxDelay = 60s. A notification that hits a temporarily failing webhook can legitimately still be in its backoff sleep minutes after the shutdown signal, and it will simply vanish.

This is the exact failure mode that matters most: the alert most likely to be lost is the one being retried because the endpoint is already having trouble.

Definition of done

internal/notify tracks its in-flight delivery goroutines — a sync.WaitGroup incremented at dispatch and decremented on completion is the obvious mechanism.
notify.New registers an fx.Lifecycle OnStop hook that waits for in-flight deliveries to drain. The fx.Lifecycle parameter it already accepts is currently unused; wire it up.
The drain is bounded. It must respect the context.Context fx passes to OnStop and give up when that context expires, so a permanently dead webhook cannot hang shutdown forever. When the drain times out with deliveries still outstanding, log at warn level how many were abandoned — silently dropping them is what this issue is fixing, so do not do it silently.
After the drain begins, newly submitted notifications must not be able to extend it indefinitely. Ensure the shutdown path cannot livelock against a stream of new dispatches.
No data race. make test runs with -race; the WaitGroup Add must happen on the dispatching goroutine before it starts the worker, never inside the worker itself.
Tests cover: a delivery in progress at shutdown is allowed to finish; a delivery stuck retrying against a dead endpoint does not hang shutdown past the context deadline and is logged as abandoned. Use httptest servers, as the existing internal/notify tests already do. Do not sleep for real backoff durations — the retry timings must be injectable or overridable in tests so the suite stays well inside the 20-second make test ceiling. The existing tests in internal/notify/retry_test.go already keep their sleeps in the 10-100ms range; follow that.
Confirm the README's shutdown claim is now actually true, and adjust the wording if the bounded-drain semantics differ from what it currently promises. README and behaviour must agree when this closes.
make check is green, and TODO.md is updated in the same commit as the work.

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

The README's shutdown claim also implies the watcher persists final state on stop. State is persisted at shutdown, but via an unrelated OnStop hook in internal/state/state.go:153-155 rather than by the watcher — the watcher's own OnStop (internal/watcher/watcher.go:93-98) only cancels, and the Run loop's ctx.Done() branch (watcher.go:148-151) just logs and returns without saving. That ordering dependency is worth a look but is not part of this issue; note it in your PR description if you touch that area, and do not expand scope.

The README promises (`README.md:454-456`): > **Shutdown**: Persist final state to disk, **complete in-flight notifications**, stop gracefully. Nothing in the codebase completes in-flight notifications. On shutdown the process can exit while a Slack / Mattermost / ntfy delivery is mid-retry, and that alert is silently lost. ## Current state (audited against `origin/main`, commit `9347a28`) Each backend dispatch fires an unmanaged goroutine — `dispatchNtfy` (`internal/notify/notify.go:197-225`), `dispatchSlack` (`:227-255`), `dispatchMattermost` (`:257-285`). Each uses `context.WithoutCancel(ctx)`, which is deliberate and correct as far as it goes: it stops a cancelled request context from killing a delivery already under way. But `context.WithoutCancel` only detaches the goroutine from cancellation. It does not make anything **wait** for it. `notify.New` (`internal/notify/notify.go:121-172`) accepts an `fx.Lifecycle` parameter and **never calls `lifecycle.Append`**. There is no `OnStop` hook, no `sync.WaitGroup`, no drain step of any kind. So fx runs its `OnStop` hooks, `main()` returns, and the process exits — while retry loops are still sleeping. The window is not small. `internal/notify/retry.go:13-25` retries up to `DefaultMaxRetries = 5` times with backoff delays up to `DefaultMaxDelay = 60s`. A notification that hits a temporarily failing webhook can legitimately still be in its backoff sleep **minutes** after the shutdown signal, and it will simply vanish. This is the exact failure mode that matters most: the alert most likely to be lost is the one being retried because the endpoint is already having trouble. ## Definition of done 1. `internal/notify` tracks its in-flight delivery goroutines — a `sync.WaitGroup` incremented at dispatch and decremented on completion is the obvious mechanism. 2. `notify.New` registers an `fx.Lifecycle` `OnStop` hook that waits for in-flight deliveries to drain. The `fx.Lifecycle` parameter it already accepts is currently unused; wire it up. 3. The drain is **bounded**. It must respect the `context.Context` fx passes to `OnStop` and give up when that context expires, so a permanently dead webhook cannot hang shutdown forever. When the drain times out with deliveries still outstanding, log at warn level how many were abandoned — silently dropping them is what this issue is fixing, so do not do it silently. 4. After the drain begins, newly submitted notifications must not be able to extend it indefinitely. Ensure the shutdown path cannot livelock against a stream of new dispatches. 5. No data race. `make test` runs with `-race`; the `WaitGroup` `Add` must happen on the dispatching goroutine before it starts the worker, never inside the worker itself. 6. Tests cover: a delivery in progress at shutdown is allowed to finish; a delivery stuck retrying against a dead endpoint does not hang shutdown past the context deadline and is logged as abandoned. Use `httptest` servers, as the existing `internal/notify` tests already do. **Do not sleep for real backoff durations** — the retry timings must be injectable or overridable in tests so the suite stays well inside the 20-second `make test` ceiling. The existing tests in `internal/notify/retry_test.go` already keep their sleeps in the 10-100ms range; follow that. 7. Confirm the README's shutdown claim is now actually true, and adjust the wording if the bounded-drain semantics differ from what it currently promises. README and behaviour must agree when this closes. 8. `make check` is green, and `TODO.md` is updated in the same commit as the work. The finishing commit's title must end with ` (closes #N)` referencing this issue. ## Related, but out of scope The README's shutdown claim also implies the watcher persists final state on stop. State *is* persisted at shutdown, but via an unrelated `OnStop` hook in `internal/state/state.go:153-155` rather than by the watcher — the watcher's own `OnStop` (`internal/watcher/watcher.go:93-98`) only cancels, and the `Run` loop's `ctx.Done()` branch (`watcher.go:148-151`) just logs and returns without saving. That ordering dependency is worth a look but is **not** part of this issue; note it in your PR description if you touch that area, and do not expand scope.

clawbot added this to the 1.0 milestone 2026-08-09 03:39:20 +02:00

clawbot referenced this issue

2026-08-09 03:39:41 +02:00

DECISION NEEDED: DNSWATCHER_SENTRY_DSN is documented and accepted but does nothing #107

clawbot referenced this issue

2026-08-09 03:40:07 +02:00

README accuracy sweep: architecture omissions, undocumented state field, overclaimed MAINTENANCE_MODE and /metrics #108

clawbot referenced this issue

2026-08-09 03:42:01 +02:00

1.0/mvp #66

clawbot commented

2026-08-09 06:58:57 +02:00

Implementation plan (branch fix/106-notify-shutdown-drain, from origin/main at 9347a28):

1. Track in-flight deliveries (internal/notify/shutdown.go, new file)

Add to Service: inFlight sync.WaitGroup, drainMu sync.Mutex guarding a draining bool, outstanding atomic.Int64, and an abandon chan struct{} closed once when a drain gives up.
New unexported startDelivery(endpoint string, fn func()): takes drainMu, refuses the dispatch if draining is already set (logging at warn with the endpoint), otherwise increments the counters and starts the worker via inFlight.Go(...) — the counter increment happens on the dispatching goroutine, before the worker exists, never inside it. outstanding is decremented before Done fires so the abandoned count read on the timeout path is accurate.

2. Collapse the three dispatchers

dispatchNtfy / dispatchSlack / dispatchMattermost (notify.go:197-285) are three copies of the same body. They become nil-check + one call to a shared svc.dispatch(ctx, endpoint, fn) that keeps the existing context.WithoutCancel(ctx) semantics, routes through startDelivery, and logs the post-retry failure as today. Behaviour is unchanged; it just stops the tracking logic from being triplicated (and keeps dupl quiet).

3. OnStop hook (definition of done 2, 3, 4)

notify.New stops discarding its fx.Lifecycle and appends a hook whose OnStop calls svc.drain(ctx):

sets draining under the mutex first, so deliveries submitted after the drain starts are refused rather than queued — a stream of new notifications cannot extend the drain (item 4);
waits on inFlight.Wait() in a helper goroutine and selects that against ctx.Done(), so the wait is bounded by whatever fx passes to OnStop (the app sets no fx.StopTimeout, so this is fx's 15s default);
on expiry: closes abandon and logs at warn with the outstanding count and ctx.Err(). deliverWithRetry's existing select gains an abandon case, so retries still sleeping in backoff return ErrDeliveryAbandoned promptly instead of lingering. In-flight HTTP requests keep their existing 10s client timeout.

4. Tests (internal/notify/shutdown_test.go, external package notify_test)

Using httptest servers and the retry knobs that already exist (SetRetryConfig, SetSleepFunc in export_test.go) so nothing sleeps for real backoff — all waits stay in the 10-100ms band, matching retry_test.go:

a delivery in progress when the drain starts is allowed to finish (handler blocks briefly, drain returns only after the request completed);
a delivery retrying against an endpoint that always fails does not hold shutdown past the OnStop context deadline, and the abandoned count is logged at warn — asserted by draining with an already-short deadline and checking the drain returns well inside it;
a dispatch submitted after the drain began is refused, not awaited;
-race clean.

export_test.go gets Drain(ctx) (and an outstanding-count accessor) shims rather than exporting new production API.

5. Docs (item 7) and bookkeeping (item 8)

README.md:455-456 gets reworded from the unqualified "complete in-flight notifications" to the bounded semantics actually implemented (wait, bounded by the shutdown timeout; anything still outstanding is abandoned and logged). TODO.md updated in the same commit as the work, make fmt run over the markdown, make check green before the PR.

Out of scope and untouched: the watcher-vs-state shutdown ordering noted at the bottom of this issue, and everything in internal/watcher / internal/resolver.

Implementation plan (branch `fix/106-notify-shutdown-drain`, from `origin/main` at `9347a28`): **1. Track in-flight deliveries (`internal/notify/shutdown.go`, new file)** - Add to `Service`: `inFlight sync.WaitGroup`, `drainMu sync.Mutex` guarding a `draining bool`, `outstanding atomic.Int64`, and an `abandon chan struct{}` closed once when a drain gives up. - New unexported `startDelivery(endpoint string, fn func())`: takes `drainMu`, refuses the dispatch if `draining` is already set (logging at warn with the endpoint), otherwise increments the counters and starts the worker via `inFlight.Go(...)` — the counter increment happens on the dispatching goroutine, before the worker exists, never inside it. `outstanding` is decremented before `Done` fires so the abandoned count read on the timeout path is accurate. **2. Collapse the three dispatchers** `dispatchNtfy` / `dispatchSlack` / `dispatchMattermost` (`notify.go:197-285`) are three copies of the same body. They become nil-check + one call to a shared `svc.dispatch(ctx, endpoint, fn)` that keeps the existing `context.WithoutCancel(ctx)` semantics, routes through `startDelivery`, and logs the post-retry failure as today. Behaviour is unchanged; it just stops the tracking logic from being triplicated (and keeps `dupl` quiet). **3. `OnStop` hook (definition of done 2, 3, 4)** `notify.New` stops discarding its `fx.Lifecycle` and appends a hook whose `OnStop` calls `svc.drain(ctx)`: - sets `draining` under the mutex first, so deliveries submitted after the drain starts are refused rather than queued — a stream of new notifications cannot extend the drain (item 4); - waits on `inFlight.Wait()` in a helper goroutine and `select`s that against `ctx.Done()`, so the wait is bounded by whatever fx passes to `OnStop` (the app sets no `fx.StopTimeout`, so this is fx's 15s default); - on expiry: closes `abandon` and logs at **warn** with the outstanding count and `ctx.Err()`. `deliverWithRetry`'s existing `select` gains an `abandon` case, so retries still sleeping in backoff return `ErrDeliveryAbandoned` promptly instead of lingering. In-flight HTTP requests keep their existing 10s client timeout. **4. Tests (`internal/notify/shutdown_test.go`, external `package notify_test`)** Using `httptest` servers and the retry knobs that already exist (`SetRetryConfig`, `SetSleepFunc` in `export_test.go`) so nothing sleeps for real backoff — all waits stay in the 10-100ms band, matching `retry_test.go`: - a delivery in progress when the drain starts is allowed to finish (handler blocks briefly, drain returns only after the request completed); - a delivery retrying against an endpoint that always fails does not hold shutdown past the `OnStop` context deadline, and the abandoned count is logged at warn — asserted by draining with an already-short deadline and checking the drain returns well inside it; - a dispatch submitted after the drain began is refused, not awaited; - `-race` clean. `export_test.go` gets `Drain(ctx)` (and an outstanding-count accessor) shims rather than exporting new production API. **5. Docs (item 7) and bookkeeping (item 8)** `README.md:455-456` gets reworded from the unqualified "complete in-flight notifications" to the bounded semantics actually implemented (wait, bounded by the shutdown timeout; anything still outstanding is abandoned and logged). `TODO.md` updated in the same commit as the work, `make fmt` run over the markdown, `make check` green before the PR. Out of scope and untouched: the watcher-vs-state shutdown ordering noted at the bottom of this issue, and everything in `internal/watcher` / `internal/resolver`.

clawbot referenced this issue from a commit

2026-08-09 07:03:28 +02:00

notify: drain in-flight deliveries at shutdown (closes #106)

clawbot referenced a pull request that will close this issue

2026-08-09 07:03:55 +02:00

notify: drain in-flight deliveries at shutdown (closes #106) #113

clawbot referenced this issue

2026-08-09 07:04:25 +02:00

notify: drain in-flight deliveries at shutdown (closes #106) #113

clawbot referenced this issue

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

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