Shutdown is broken: os.Exit races fx OnStop hooks, losing buffered reports on every restart #22

New Issue

clawbot · 2026-08-09T03:41:42+02:00

clawbot commented

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

Problem

Three defects in the shutdown path. The first causes silent data loss on every single restart. Verified on main at fbfe1df. These are correctness bugs, not style issues.

1. `os.Exit` in `run()` pre-empts fx's `OnStop` hooks — buffered telemetry is lost

backend/internal/server/server.go:91-94:

func (s *Server) run() {
    exitCode := s.serve()
    os.Exit(exitCode)
}

On SIGTERM, the server's own signal goroutine (server.go:103-115) cancels the context, serve() returns, and run() calls os.Exit immediately. That races fx's own signal handling and can terminate the process before fx runs the OnStop hooks of the other components.

The critical casualty is backend/internal/reportbuf/reportbuf.go:78-83 — reportbuf's OnStop is the only code path that flushes buffered reports to disk on shutdown. When os.Exit wins the race, everything accepted since the last periodic flush is discarded.

Worst case that is bounded by the existing config: up to a full flush window of accepted telemetry is thrown away on each restart, and the process still exits 0, so nothing anywhere reports a problem.

2. Unsynchronized cross-goroutine access to `s.httpServer` — nil deref on early shutdown

s.httpServer is assigned inside the serveUntilShutdown goroutine at internal/server/http.go:19. It is read and dereferenced from a different goroutine in cleanShutdown at internal/server/server.go:138 (s.httpServer.Shutdown(...)).

There is no synchronization between the spawn at server.go:117-119 and the <-ctx.Done() -> cleanShutdown at server.go:121-122. A signal arriving in the window before http.go:19 executes produces a nil-pointer panic. Independently of the nil case, it is an unsynchronized read/write of the same field — a genuine data race.

This is invisible today because make test runs without -race (see #21).

3. `close(b.done)` will panic if `OnStop` runs twice

internal/reportbuf/reportbuf.go:78-83 closes the channel with no sync.Once guard. CODE_STYLEGUIDE_GO.md: "Always handle the case where a channel might be closed. This prevents panic and ensures graceful shutdowns."

exitCode can never be non-zero. server.go:39 is only ever assigned 0 (server.go:130). A listen failure (http.go:36-42) logs, cancels, and the process still exits 0. Failures are unobservable to any supervisor.
WriteTimeout (10s) contradicts middleware.Timeout (60s). http.go:11-12 vs routes.go:10,21. The 60s per-handler budget is unreachable — the server kills the write at 10s, so the chi timeout is dead configuration. Pick one coherent budget.
startupTime is dead. Set at server.go:63, never read. Real uptime comes from internal/healthcheck/healthcheck.go.

Definition of done

os.Exit is removed from the shutdown path. The server requests shutdown through fx (inject fx.Shutdowner and call Shutdown()) so fx runs every component's OnStop in dependency order.
Prove the fix: with a report buffered and not yet flushed, send SIGTERM and confirm the report is on disk afterwards. Describe the verification in the PR. A test that asserts the flush-on-shutdown path runs is strongly preferred over manual verification.
s.httpServer is no longer written and read from different goroutines without synchronization. Construct it before spawning the serving goroutine, or guard it. Shutdown must be safe when it arrives before the listener is up — no nil deref.
reportbuf's OnStop is idempotent; a second invocation does not panic.
exitCode reflects reality — a listen failure results in a non-zero process exit.
WriteTimeout and the chi request timeout are made mutually coherent, with a comment stating the intended budget.
Dead startupTime field is removed, or wired up and used.
Tests covering the shutdown path run clean under -race. Note that #21 adds -race to make test; if #21 has not landed, run the race detector manually for this work and say so in the PR.
cd backend && make check passes; root make check passes.
TODO.md updated in the same commit.
Commit title ends with (closes #N).

Implementation requirements

Follow GO_HTTP_SERVER_CONVENTIONS.md for lifecycle and graceful shutdown, but do not copy its os.Exit-adjacent shape where it conflicts with correct fx teardown — fx owns the process lifetime here.
Do not paper over item 2 with a mutex around a field that should simply be initialized earlier. Prefer restructuring so the ordering hazard cannot exist.
Keep the change focused on lifecycle correctness. Security headers, timeouts-as-policy, and CORS belong to #19 and #20.
make targets only; never raw go invocations.
No attribution trailers in the commit message.

## Problem Three defects in the shutdown path. The first causes **silent data loss on every single restart**. Verified on `main` at `fbfe1df`. These are correctness bugs, not style issues. ### 1. `os.Exit` in `run()` pre-empts fx's `OnStop` hooks — buffered telemetry is lost `backend/internal/server/server.go:91-94`: ```go func (s *Server) run() { exitCode := s.serve() os.Exit(exitCode) } ``` On SIGTERM, the server's own signal goroutine (`server.go:103-115`) cancels the context, `serve()` returns, and `run()` calls `os.Exit` immediately. That races fx's own signal handling and can terminate the process **before fx runs the `OnStop` hooks of the other components**. The critical casualty is `backend/internal/reportbuf/reportbuf.go:78-83` — `reportbuf`'s `OnStop` is the **only** code path that flushes buffered reports to disk on shutdown. When `os.Exit` wins the race, everything accepted since the last periodic flush is discarded. Worst case that is bounded by the existing config: up to a full flush window of accepted telemetry is thrown away on each restart, and the process still exits `0`, so nothing anywhere reports a problem. ### 2. Unsynchronized cross-goroutine access to `s.httpServer` — nil deref on early shutdown `s.httpServer` is assigned inside the `serveUntilShutdown` goroutine at `internal/server/http.go:19`. It is read and dereferenced from a **different** goroutine in `cleanShutdown` at `internal/server/server.go:138` (`s.httpServer.Shutdown(...)`). There is no synchronization between the spawn at `server.go:117-119` and the `<-ctx.Done()` -> `cleanShutdown` at `server.go:121-122`. A signal arriving in the window before `http.go:19` executes produces a **nil-pointer panic**. Independently of the nil case, it is an unsynchronized read/write of the same field — a genuine data race. This is invisible today because `make test` runs without `-race` (see #21). ### 3. `close(b.done)` will panic if `OnStop` runs twice `internal/reportbuf/reportbuf.go:78-83` closes the channel with no `sync.Once` guard. `CODE_STYLEGUIDE_GO.md`: "Always handle the case where a channel might be closed. This prevents panic and ensures graceful shutdowns." ### Related, same area - **`exitCode` can never be non-zero.** `server.go:39` is only ever assigned `0` (`server.go:130`). A listen failure (`http.go:36-42`) logs, cancels, and the process still exits `0`. Failures are unobservable to any supervisor. - **`WriteTimeout` (10s) contradicts `middleware.Timeout` (60s).** `http.go:11-12` vs `routes.go:10,21`. The 60s per-handler budget is unreachable — the server kills the write at 10s, so the chi timeout is dead configuration. Pick one coherent budget. - **`startupTime` is dead.** Set at `server.go:63`, never read. Real uptime comes from `internal/healthcheck/healthcheck.go`. ## Definition of done - [ ] `os.Exit` is removed from the shutdown path. The server requests shutdown through fx (inject `fx.Shutdowner` and call `Shutdown()`) so fx runs every component's `OnStop` in dependency order. - [ ] Prove the fix: with a report buffered and not yet flushed, send SIGTERM and confirm the report is on disk afterwards. Describe the verification in the PR. A test that asserts the flush-on-shutdown path runs is strongly preferred over manual verification. - [ ] `s.httpServer` is no longer written and read from different goroutines without synchronization. Construct it before spawning the serving goroutine, or guard it. Shutdown must be safe when it arrives before the listener is up — no nil deref. - [ ] `reportbuf`'s `OnStop` is idempotent; a second invocation does not panic. - [ ] `exitCode` reflects reality — a listen failure results in a non-zero process exit. - [ ] `WriteTimeout` and the chi request timeout are made mutually coherent, with a comment stating the intended budget. - [ ] Dead `startupTime` field is removed, or wired up and used. - [ ] Tests covering the shutdown path run clean under `-race`. Note that #21 adds `-race` to `make test`; if #21 has not landed, run the race detector manually for this work and say so in the PR. - [ ] `cd backend && make check` passes; root `make check` passes. - [ ] `TODO.md` updated in the same commit. - [ ] Commit title ends with ` (closes #N)`. ## Implementation requirements - Follow `GO_HTTP_SERVER_CONVENTIONS.md` for lifecycle and graceful shutdown, but **do not** copy its `os.Exit`-adjacent shape where it conflicts with correct fx teardown — fx owns the process lifetime here. - Do not paper over item 2 with a mutex around a field that should simply be initialized earlier. Prefer restructuring so the ordering hazard cannot exist. - Keep the change focused on lifecycle correctness. Security headers, timeouts-as-policy, and CORS belong to #19 and #20. - `make` targets only; never raw `go` invocations. - No attribution trailers in the commit message.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:41:42 +02:00

clawbot referenced this issue

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

Report ingest correctness: returns 200 on storage failure, 400 on oversize, logs untrusted body #23

clawbot referenced this issue

2026-08-09 03:43:57 +02:00

Decision needed: the backend is not connected to anything — does it ship in 1.0? #25

Sign in to join this conversation.