Shutdown correctness: exit code always 0, two competing signal handlers, Sentry calls os.Exit from a goroutine #86

New Issue

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

clawbot commented

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

Verified against main at 61f42e6.

Three related defects in process lifecycle:

1. The computed exit code is discarded. internal/server/server.go:110-130 — serve() computes and returns an exit code; Run() (:86-89) throws it away. pixad therefore exits 0 even when it shut down because of an error. Any supervisor (systemd, uPaaS per #17, Docker restart policies, CI) that keys off exit status sees a clean exit from a crash, so a failing daemon reads as a deliberate stop and may not be restarted or alerted on.

2. Two independent signal handlers. fx.Run() (cmd/pixad/main.go:59) installs its own SIGINT/SIGTERM handling, and server.serve installs a second at internal/server/server.go:113-122. Both fire on the same signal. Two shutdown paths racing on one signal is how "sometimes it hangs on stop" and "sometimes it exits before flushing" bugs are born, and neither is reproducible on demand.

3. Sentry initialization calls os.Exit(1) from a goroutine (server.go:104), bypassing fx's lifecycle entirely — no OnStop hooks run, so the database and any in-flight work are abandoned rather than closed.

Related, and worth resolving in the same pass: cleanShutdown (server.go:132-146) has a 5 s timeout but does not wait for in-flight libvips work. With #55 this also interacts with StopEviction, which blocks with no timeout while a reconciliation pass finishes — see the contextcheck decision recorded on PR #54.

Definition of done

The exit code computed by serve() reaches os.Exit; a shutdown caused by an error exits non-zero, a clean signal-triggered shutdown exits 0.
Exactly one signal handling path. Prefer fx's, and remove the duplicate, unless there is a concrete reason the custom one must own it — state the reasoning either way.
Sentry initialization failure is surfaced as an fx startup error rather than os.Exit from a goroutine, so lifecycle hooks run.
cleanShutdown waits for in-flight image processing, bounded by its timeout; on timeout it logs what was still outstanding rather than exiting silently.
Tests where practical: exit code propagation is testable by factoring the code path out of main; signal handling generally is not, so state explicitly in the PR what was verified manually.
make check green.

Coordination

Point 4 overlaps the eviction-loop cancellability decision flagged on PR #54. Do this after both open PRs land, and resolve the two together — they are the same question about who owns shutdown.

Verified against `main` at `61f42e6`. Three related defects in process lifecycle: **1. The computed exit code is discarded.** `internal/server/server.go:110-130` — `serve()` computes and returns an exit code; `Run()` (`:86-89`) throws it away. `pixad` therefore exits 0 even when it shut down because of an error. Any supervisor (systemd, uPaaS per #17, Docker restart policies, CI) that keys off exit status sees a clean exit from a crash, so a failing daemon reads as a deliberate stop and may not be restarted or alerted on. **2. Two independent signal handlers.** `fx.Run()` (`cmd/pixad/main.go:59`) installs its own SIGINT/SIGTERM handling, and `server.serve` installs a second at `internal/server/server.go:113-122`. Both fire on the same signal. Two shutdown paths racing on one signal is how "sometimes it hangs on stop" and "sometimes it exits before flushing" bugs are born, and neither is reproducible on demand. **3. Sentry initialization calls `os.Exit(1)` from a goroutine** (`server.go:104`), bypassing fx's lifecycle entirely — no `OnStop` hooks run, so the database and any in-flight work are abandoned rather than closed. Related, and worth resolving in the same pass: `cleanShutdown` (`server.go:132-146`) has a 5 s timeout but does not wait for in-flight libvips work. With #55 this also interacts with `StopEviction`, which blocks with no timeout while a reconciliation pass finishes — see the `contextcheck` decision recorded on PR #54. ## Definition of done 1. The exit code computed by `serve()` reaches `os.Exit`; a shutdown caused by an error exits non-zero, a clean signal-triggered shutdown exits 0. 2. Exactly one signal handling path. Prefer fx's, and remove the duplicate, unless there is a concrete reason the custom one must own it — state the reasoning either way. 3. Sentry initialization failure is surfaced as an fx startup error rather than `os.Exit` from a goroutine, so lifecycle hooks run. 4. `cleanShutdown` waits for in-flight image processing, bounded by its timeout; on timeout it logs what was still outstanding rather than exiting silently. 5. Tests where practical: exit code propagation is testable by factoring the code path out of `main`; signal handling generally is not, so state explicitly in the PR what was verified manually. 6. `make check` green. ## Coordination Point 4 overlaps the eviction-loop cancellability decision flagged on PR #54. Do this **after** both open PRs land, and resolve the two together — they are the same question about who owns shutdown.

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

clawbot referenced this issue

2026-08-09 16:10:39 +02:00

Tracking: 1.0.0 sequencing and critical path #103

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/pixa#86