trails-cool/trails
Watch
1
1
Fork
You've already forked trails
 0
Code Issues 2 Pull requests Projects Releases Packages Activity Actions 10

fix(cd-infra): don't SIGHUP a Prometheus that compose just started #24

Merged
ullrich merged 1 commit from fix/cd-infra-prometheus-sighup-race into main 2026-07-27 12:33:30 +00:00
Conversation 0 Commits 1 Files changed 1 +27 -1
ullrich commented 2026-07-27 12:22:38 +00:00
Owner
Copy link

Fixes #23. One-line guard plus the reasoning that makes it obvious in six months.

What happened

cd-infra run #94 started Prometheus and killed it one second later, taking metrics and alert evaluation down for ~16 minutes:

12:04:40.266  Container trails-cool-prometheus-1 Starting
12:04:40.527  Container trails-cool-prometheus-1 Started
12:04:41.577  Container trails-cool-prometheus-1 Killing
12:04:41.638  Container trails-cool-prometheus-1 Killed
12:08:00.069  trails-cool-prometheus-1 did not become ready (last status: exited)

Why

The config-reload branch gated on container identity alone:

if [ -n "$PROMETHEUS_BEFORE_ID" ] && [ "$PROMETHEUS_BEFORE_ID" = "$PROMETHEUS_AFTER_ID" ]; then
  docker compose kill -s SIGHUP prometheus
fi

The intent is right — Prometheus mounts its config directory, so a replaced config is visible live and SIGHUP applies it without recreating the container. The flaw is that docker inspect answers for a stopped container too, verified on the flagship with a throwaway container:

container state .Id .State.Running
created, never started 9f7f1020e584 false
stopped after running 9f7f1020e584 false
live prometheus 6fac3fb66ada true

So a Prometheus that entered the deploy stopped got started by up -d under the same ID, the IDs matched, and the branch fired ~1s into WAL replay — before Prometheus installs its SIGHUP handler, and SIGHUP's default disposition is terminate.

ID equality was standing in for "was already running". Those diverge in exactly one state: stopped-but-present. Which is also the state where the reload is least necessary, since a just-started process already loaded the new config.

The fix

PROMETHEUS_BEFORE_RUNNING=$(docker inspect -f '{{.State.Running}}' trails-cool-prometheus-1 2>/dev/null || echo false)
...
if [ "$PROMETHEUS_BEFORE_RUNNING" = "true" ] \
  && [ -n "$PROMETHEUS_BEFORE_ID" ] \
  && [ "$PROMETHEUS_BEFORE_ID" = "$PROMETHEUS_AFTER_ID" ]; then
  docker compose --env-file .env kill -s SIGHUP prometheus
fi

Truth table across all five reachable states, old logic vs new:

scenario old new correct
running, untouched (stale config in memory) HUP HUP HUP
running, recreated (new config already) skip skip skip
STOPPED, started by up -d HUP skip skip
absent, created by up -d skip skip skip
absent throughout skip skip skip

Only the failing row changes. The reload path this branch exists for is untouched.

Verification

  • Truth table above executed as a script against both implementations, not reasoned about
  • .State.Running semantics confirmed against real Docker on the flagship (table above); probe container removed
  • bash -n on the deploy script extracted from the YAML block scalar — the \ line continuations survive extraction intact
  • YAML structure parses

Not in scope

#23's open question stands. Something SIGTERMed Prometheus at 11:55:07 and recreated five containers around 11:53, both before run #94's SSH step began at 12:04:27 — that earlier partial execution is what created the stopped-but-present state this fix now handles safely. concurrency: cancel-in-progress: true on deploy-infra is the obvious suspect. This PR makes the consequence harmless; it does not explain the cause, so I've left #23 open for it.

Also left alone: the readiness gate. It behaved correctly — refusing to report success is why this was a noisy 16-minute outage instead of a silent one.

Note on verifying this

This changes .forgejo/workflows/cd-infra.yml, which is not matched by cd-infra's own paths: ["infrastructure/**"] filter — so merging will not trigger a deploy, and main keeps run #94's red status until something does. A workflow_dispatch after merge would both confirm green and exercise the intended HUP path (Prometheus is running now, so BEFORE_RUNNING=true and the reload happens as designed).

Fixes #23. One-line guard plus the reasoning that makes it obvious in six months. ## What happened `cd-infra` run #94 started Prometheus and killed it one second later, taking metrics and alert evaluation down for ~16 minutes: ``` 12:04:40.266 Container trails-cool-prometheus-1 Starting 12:04:40.527 Container trails-cool-prometheus-1 Started 12:04:41.577 Container trails-cool-prometheus-1 Killing 12:04:41.638 Container trails-cool-prometheus-1 Killed 12:08:00.069 trails-cool-prometheus-1 did not become ready (last status: exited) ``` ## Why The config-reload branch gated on container identity alone: ```bash if [ -n "$PROMETHEUS_BEFORE_ID" ] && [ "$PROMETHEUS_BEFORE_ID" = "$PROMETHEUS_AFTER_ID" ]; then docker compose kill -s SIGHUP prometheus fi ``` The intent is right — Prometheus mounts its config *directory*, so a replaced config is visible live and SIGHUP applies it without recreating the container. The flaw is that **`docker inspect` answers for a stopped container too**, verified on the flagship with a throwaway container: | container state | `.Id` | `.State.Running` | |---|---|---| | created, never started | `9f7f1020e584` | `false` | | stopped after running | `9f7f1020e584` | `false` | | live prometheus | `6fac3fb66ada` | `true` | So a Prometheus that entered the deploy stopped got started by `up -d` under the **same ID**, the IDs matched, and the branch fired ~1s into WAL replay — before Prometheus installs its SIGHUP handler, and SIGHUP's default disposition is *terminate*. ID equality was standing in for "was already running". Those diverge in exactly one state: stopped-but-present. Which is also the state where the reload is **least** necessary, since a just-started process already loaded the new config. ## The fix ```bash PROMETHEUS_BEFORE_RUNNING=$(docker inspect -f '{{.State.Running}}' trails-cool-prometheus-1 2>/dev/null || echo false) ... if [ "$PROMETHEUS_BEFORE_RUNNING" = "true" ] \ && [ -n "$PROMETHEUS_BEFORE_ID" ] \ && [ "$PROMETHEUS_BEFORE_ID" = "$PROMETHEUS_AFTER_ID" ]; then docker compose --env-file .env kill -s SIGHUP prometheus fi ``` Truth table across all five reachable states, old logic vs new: | scenario | old | new | correct | |---|---|---|---| | running, untouched (stale config in memory) | HUP | **HUP** | HUP | | running, recreated (new config already) | skip | skip | skip | | **STOPPED, started by `up -d`** | **HUP** | **skip** | skip | | absent, created by `up -d` | skip | skip | skip | | absent throughout | skip | skip | skip | Only the failing row changes. The reload path this branch exists for is untouched. ## Verification - Truth table above executed as a script against both implementations, not reasoned about - `.State.Running` semantics confirmed against real Docker on the flagship (table above); probe container removed - `bash -n` on the deploy script extracted from the YAML block scalar — the `\` line continuations survive extraction intact - YAML structure parses ## Not in scope **#23's open question stands.** Something SIGTERMed Prometheus at 11:55:07 and recreated five containers around 11:53, both *before* run #94's SSH step began at 12:04:27 — that earlier partial execution is what created the stopped-but-present state this fix now handles safely. `concurrency: cancel-in-progress: true` on `deploy-infra` is the obvious suspect. This PR makes the consequence harmless; it does not explain the cause, so I've left #23 open for it. Also left alone: the readiness gate. It behaved correctly — refusing to report success is why this was a noisy 16-minute outage instead of a silent one. ## Note on verifying this This changes `.forgejo/workflows/cd-infra.yml`, which is **not** matched by cd-infra's own `paths: ["infrastructure/**"]` filter — so merging will not trigger a deploy, and `main` keeps run #94's red status until something does. A `workflow_dispatch` after merge would both confirm green and exercise the intended HUP path (Prometheus is running now, so `BEFORE_RUNNING=true` and the reload happens as designed).
fix(cd-infra): don't SIGHUP a Prometheus that compose just started
All checks were successful
CI / Dockerfile Package Check (pull_request) Successful in 22s
Details
CI / Security Scan (pull_request) Successful in 49s
Details
CI / Checks (pull_request) Successful in 3m22s
Details
CI / Visual Tests (pull_request) Successful in 3m23s
Details
CI / Journal Image Smoke Test (pull_request) Successful in 3m32s
Details
CI / E2E Tests (pull_request) Successful in 5m53s
Details
48a3ae9c0f
 
cd-infra run #94 started Prometheus and killed it one second later,
taking metrics and alert evaluation down for ~16 minutes:

  12:04:40.266  Container trails-cool-prometheus-1 Starting
  12:04:40.527  Container trails-cool-prometheus-1 Started
  12:04:41.577  Container trails-cool-prometheus-1 Killing
  12:04:41.638  Container trails-cool-prometheus-1 Killed
  12:08:00.069  trails-cool-prometheus-1 did not become ready

The config-reload branch gated on container identity alone:

  if [ -n "$BEFORE_ID" ] && [ "$BEFORE_ID" = "$AFTER_ID" ]; then
    docker compose kill -s SIGHUP prometheus
  fi

`docker inspect` answers for a stopped container too — verified on the
flagship: a created-but-never-started container reports a non-empty
`.Id` with `.State.Running=false`. So a Prometheus that entered the
deploy stopped got started by `up -d` under the *same* ID, the IDs
matched, and the branch fired ~1s into WAL replay — before Prometheus
installs its SIGHUP handler, and SIGHUP's default disposition is
terminate.

ID equality was standing in for "was already running", and the two
diverge exactly when the container was stopped. Gate on `.State.Running`
instead. A container compose just started is already holding the new
config, so skipping the reload there is correct independent of the crash.

Truth table (old vs new), all five reachable states:

  running, untouched        HUP  -> HUP   (unchanged; this is the point)
  running, recreated       skip -> skip
  STOPPED, started by up   HUP  -> skip  (the bug)
  absent, created by up    skip -> skip
  absent throughout        skip -> skip

Only the failing case changes behaviour.

The readiness gate below correctly refused to report success, which is
why this surfaced as a failed deploy rather than silent loss of
monitoring. Not touched here.

Refs #23

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X8hNxgYp777FRqYtVmQNaU
ullrich deleted branch fix/cd-infra-prometheus-sighup-race 2026-07-27 12:33:30 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
dependencies
Dependency updates raised by Renovate (renovate.json5 labels)

  
major
Major version bump — always needs human review, never auto-merged

  
security-pin
Touches a pnpm.overrides CVE remediation pin, not an ordinary dependency

No labels
dependencies
major
security-pin
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
trails-cool/trails!24
No description provided.