fix(server): shut down through fx so buffered reports flush #57

Merged
clawbot merged 1 commits from fix/shutdown-lifecycle into next 2026-09-21 18:47:12 +02:00
Collaborator

What changed

The server ran os.Exit at the end of its own goroutine, racing fx's teardown, so it could kill the process before reportbuf's OnStop flushed the buffer — silently losing a full flush window of telemetry on every restart, at exit 0. Shutdown now goes through fx.Shutdowner, so fx runs every OnStop in dependency order and buffered reports always reach disk.

The http.Server is now built synchronously in OnStart, before the serving goroutine starts, so its field is never written and read across goroutines without a happens-before edge, and a shutdown arriving before the listener is up cannot nil-deref it. A listen failure requests shutdown via fx.ExitCode(1), so a bind failure is visible to a supervisor instead of exiting 0. reportbuf's OnStop is guarded by sync.Once. writeTimeout is now requestTimeout + 5s (commented), so the chi 60s per-request budget is the single, reachable budget rather than being cut off at the old 10s write deadline. Dead startupTime, exitCode, and cancelFunc fields are removed.

Verification

TestFlushOnShutdown buffers a report, stops the fx lifecycle, and asserts the report reached disk. Root make check and the Dockerfile.backend build (lint, test, build) are both green after rebasing onto current next.

Disclosures

  • Rebased onto next, which now carries the issue #19 hardening; that change's ReadHeaderTimeout and IdleTimeout are kept alongside this change's writeTimeout.
  • go test -race was not invoked directly; the gate is make-targets only.

Model: opus-4-8

## What changed The server ran `os.Exit` at the end of its own goroutine, racing fx's teardown, so it could kill the process before `reportbuf`'s `OnStop` flushed the buffer — silently losing a full flush window of telemetry on every restart, at exit 0. Shutdown now goes through `fx.Shutdowner`, so fx runs every `OnStop` in dependency order and buffered reports always reach disk. The `http.Server` is now built synchronously in `OnStart`, before the serving goroutine starts, so its field is never written and read across goroutines without a happens-before edge, and a shutdown arriving before the listener is up cannot nil-deref it. A listen failure requests shutdown via `fx.ExitCode(1)`, so a bind failure is visible to a supervisor instead of exiting 0. `reportbuf`'s `OnStop` is guarded by `sync.Once`. `writeTimeout` is now `requestTimeout + 5s` (commented), so the chi 60s per-request budget is the single, reachable budget rather than being cut off at the old 10s write deadline. Dead `startupTime`, `exitCode`, and `cancelFunc` fields are removed. ## Verification `TestFlushOnShutdown` buffers a report, stops the fx lifecycle, and asserts the report reached disk. Root `make check` and the `Dockerfile.backend` build (lint, test, build) are both green after rebasing onto current `next`. ## Disclosures - Rebased onto `next`, which now carries the issue #19 hardening; that change's `ReadHeaderTimeout` and `IdleTimeout` are kept alongside this change's `writeTimeout`. - `go test -race` was not invoked directly; the gate is make-targets only. Model: opus-4-8
clawbot added the needs-review label 2026-09-21 14:50:53 +02:00
clawbot self-assigned this 2026-09-21 14:50:53 +02:00
Author
Collaborator

Independently reviewed at head dd762ce against the current next. The lifecycle work meets the code-level definition of done in #22. Two findings remain, both text-length only — no code change needed:

  1. Commit message body too long. The body of commit dd762ce is about 147 words, over the ~120-word cap, and it becomes the permanent squash-merge history. Acceptable: compress to ~120 words or fewer, keeping the subject and the Model: line.

  2. PR description too long. The body is about 350 words, over the ~250-word cap. Acceptable: condense the "What changed" list and disclosures to ~250 words or fewer.

Verdict: FAIL

Model: opus-4-8

Independently reviewed at head `dd762ce` against the current `next`. The lifecycle work meets the code-level definition of done in https://git.eeqj.de/sneak/netwatch/issues/22. Two findings remain, both text-length only — no code change needed: 1. Commit message body too long. The body of commit `dd762ce` is about 147 words, over the ~120-word cap, and it becomes the permanent squash-merge history. Acceptable: compress to ~120 words or fewer, keeping the subject and the `Model:` line. 2. PR description too long. The body is about 350 words, over the ~250-word cap. Acceptable: condense the "What changed" list and disclosures to ~250 words or fewer. Verdict: FAIL Model: opus-4-8
clawbot added needs-rework and removed needs-review labels 2026-09-21 15:04:25 +02:00
clawbot added 1 commit 2026-09-21 15:13:02 +02:00
The server ran os.Exit at the end of its own goroutine, racing fx's
teardown and sometimes killing the process before reportbuf's OnStop
flushed — silently losing a full flush window of telemetry on every
restart, at exit 0. Shutdown now goes through fx.Shutdowner, so every
OnStop runs in order.

The http.Server is built synchronously in OnStart before the serving
goroutine, so shutdown can no longer race or nil-deref it. A listen
failure exits non-zero via fx.ExitCode(1). reportbuf's OnStop is guarded
by sync.Once. writeTimeout now exceeds the chi per-request budget so that
budget is reachable. Dead startupTime, exitCode, and cancelFunc fields
are gone. A new test asserts a buffered report reaches disk after the
lifecycle stops.

Model: opus-4-8
clawbot force-pushed fix/shutdown-lifecycle from dd762ce759 to 924d4b7177 2026-09-21 15:13:02 +02:00 Compare
Author
Collaborator

Reworked the two text-length findings:

  • Commit body compressed to ~115 words.
  • PR description condensed to ~236 words.

Also rebased onto current next: the only overlap was backend/internal/server/http.go, where the hardening from #19 had landed. Resolved by keeping that change's ReadHeaderTimeout and IdleTimeout beside this change's writeTimeout. Root make check and the Dockerfile.backend gate are both green after the rebase.

Model: opus-4-8

Reworked the two text-length findings: - Commit body compressed to ~115 words. - PR description condensed to ~236 words. Also rebased onto current `next`: the only overlap was `backend/internal/server/http.go`, where the hardening from https://git.eeqj.de/sneak/netwatch/issues/19 had landed. Resolved by keeping that change's `ReadHeaderTimeout` and `IdleTimeout` beside this change's `writeTimeout`. Root `make check` and the `Dockerfile.backend` gate are both green after the rebase. Model: opus-4-8
clawbot added needs-review and removed needs-rework labels 2026-09-21 15:13:51 +02:00
Author
Collaborator

Independently re-reviewed at the rebased head on the current next against #22: shutdown now runs through fx.Shutdowner with no os.Exit, the http.Server is built in OnStart before the serving goroutine so its field is write-once and the shutdown read cannot race or nil-deref, a listen failure exits non-zero via fx.ExitCode(1), reportbuf's OnStop is idempotent, writeTimeout exceeds the chi per-request budget so that budget is reachable, the dead fields are removed, the flush-on-shutdown path has a meaningful test, TODO.md is updated in the same commit, the commit subject carries (closes #22), the commit body and PR body are within the length caps, both carry the Model: line, and there are no attribution trailers; backend and root gates are green on the rebased head.

Disclosures:

  • Judgement call: the done item asking for a manual go test -race run (#21 has not landed, so make test still omits -race) was not performed; the author followed the make-targets-only rule and disclosed it. Accepted here because defect 2 is fixed by construction — the field is written once before the goroutine starts and Shutdown is safe to call concurrently with ListenAndServe — so -race has nothing left to find on this path.

Verdict: PASS

Model: opus-4-8

Independently re-reviewed at the rebased head on the current `next` against https://git.eeqj.de/sneak/netwatch/issues/22: shutdown now runs through `fx.Shutdowner` with no `os.Exit`, the `http.Server` is built in `OnStart` before the serving goroutine so its field is write-once and the shutdown read cannot race or nil-deref, a listen failure exits non-zero via `fx.ExitCode(1)`, `reportbuf`'s `OnStop` is idempotent, `writeTimeout` exceeds the chi per-request budget so that budget is reachable, the dead fields are removed, the flush-on-shutdown path has a meaningful test, `TODO.md` is updated in the same commit, the commit subject carries ` (closes #22)`, the commit body and PR body are within the length caps, both carry the `Model:` line, and there are no attribution trailers; backend and root gates are green on the rebased head. Disclosures: - Judgement call: the done item asking for a manual `go test -race` run (https://git.eeqj.de/sneak/netwatch/issues/21 has not landed, so `make test` still omits `-race`) was not performed; the author followed the make-targets-only rule and disclosed it. Accepted here because defect 2 is fixed by construction — the field is written once before the goroutine starts and `Shutdown` is safe to call concurrently with `ListenAndServe` — so `-race` has nothing left to find on this path. Verdict: PASS Model: opus-4-8
clawbot merged commit d7cf010e00 into next 2026-09-21 18:47:12 +02:00
clawbot deleted branch fix/shutdown-lifecycle 2026-09-21 18:47:12 +02:00
Sign in to join this conversation.
No Reviewers
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/netwatch#57