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

Closed
opened 2026-08-09 03:50:20 +02:00 by clawbot · 1 comment
Collaborator

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.

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 self-assigned this 2026-09-29 13:01:20 +02:00
Author
Collaborator

Plan, checked against current next:

  • Signal handling: fx's path stays; the server's own handler goes. fx already listens for SIGINT and SIGTERM and then runs every stop hook in order. The server's handler only cancelled a context, and the server's stop hook returned without waiting for the shutdown that cancel started, so the process could exit before the HTTP server drained or Sentry flushed. The shutdown work moves into the server's stop hook, which fx waits for. SIGPIPE stays ignored, as it is not a shutdown signal.
  • Exit code: cmd/pixad gets a small function that starts the fx app, waits for a signal or a shutdown request, stops the app and returns the exit code. main passes that code to os.Exit. A listen error asks fx to shut down with exit code 1; today it stops the HTTP server and the process keeps running until it gets a signal. A failure to start or stop exits 1, and a signal exits 0.
  • Sentry: initialization moves into the server's start hook and returns its error, so fx fails startup and runs the stop hooks of whatever had already started.
  • Waiting for image processing: after the HTTP server stops, the server waits until no image holds a processing slot from #64, within the same 5 s limit. Each request already waits for its own processing, but with #160 processing can outlive its request. On timeout it logs how many images were still being processed, and the process exits 1, because work was abandoned.
  • The eviction loop is left to #102.

Model: opus-5-5

Plan, checked against current `next`: - **Signal handling: fx's path stays; the server's own handler goes.** fx already listens for SIGINT and SIGTERM and then runs every stop hook in order. The server's handler only cancelled a context, and the server's stop hook returned without waiting for the shutdown that cancel started, so the process could exit before the HTTP server drained or Sentry flushed. The shutdown work moves into the server's stop hook, which fx waits for. SIGPIPE stays ignored, as it is not a shutdown signal. - **Exit code:** `cmd/pixad` gets a small function that starts the fx app, waits for a signal or a shutdown request, stops the app and returns the exit code. `main` passes that code to `os.Exit`. A listen error asks fx to shut down with exit code 1; today it stops the HTTP server and the process keeps running until it gets a signal. A failure to start or stop exits 1, and a signal exits 0. - **Sentry:** initialization moves into the server's start hook and returns its error, so fx fails startup and runs the stop hooks of whatever had already started. - **Waiting for image processing:** after the HTTP server stops, the server waits until no image holds a processing slot from https://git.eeqj.de/sneak/pixa/issues/64, within the same 5 s limit. Each request already waits for its own processing, but with https://git.eeqj.de/sneak/pixa/pulls/160 processing can outlive its request. On timeout it logs how many images were still being processed, and the process exits 1, because work was abandoned. - The eviction loop is left to https://git.eeqj.de/sneak/pixa/issues/102. Model: opus-5-5
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/pixa#86