fix(nginx): reload via SIGHUP instead of starting a second master

[omarsy]

Incident

Greffer nginx logged a reload storm, ~every 300s (the CRL sync interval):

nginx: [emerg] bind() to 0.0.0.0:443 failed (98: Address in use)
/root/ DELETE revoked.crl
Reloading Nginx Configuration
nginx: [emerg] bind() to 0.0.0.0:443 failed (98: Address in use)

Root cause

nginx/nginx.sh "reloaded" by running bare nginx, which starts a second master that cannot bind 443 (the running master holds it), errors Address in use, and exits. Two consequences:

1. Cert rotation was silently broken. After the first successful start, no nginx.sh-driven reload ever took effect, so a rotated cert was never picked up until a full container restart. A latent outage: the day a greffer cert expires before a restart, the manager edge starts rejecting it.
2. The manager's periodic CRL copy into /root/revoked.crl tripped the watch every sync interval, producing the failed-reload storm. (This nginx never references the CRL: no ssl_crl in nginx.conf.)

The running master kept serving throughout, so this was not itself the user-facing 502 (that was the manager edge, see greffon/manager#101). But it was masking a real reliability bug.

Fix

• Use nginx -s reload (SIGHUP: keeps listening sockets, re-execs workers) when a master is already running; use nginx only to start one.
• Start once up front so a container restart with certs already on disk brings nginx up without waiting for an inotify event.
• Exclude *.crl from the watch, since this nginx never loads the CRL. That also silences the storm regardless of chore: remove dead CRL-to-nginx sync (nginx never loads the file) #54.

Testing

• sh -n nginx/nginx.sh passes. Logic reviewed against pidof nginx / nginx -t semantics.
• Not run in a container locally (no Docker in the build env). Please validate in staging: confirm nginx starts on cert install, a cert change triggers a single Reloading Nginx Configuration + successful SIGHUP (no Address in use), and CRL writes no longer trigger reloads.

Relation to #54

Independent of #54 (remove dead CRL sync) and complementary: this makes the reload mechanism correct; #54 removes the dead CRL copy that was tripping it. Note #54's deploy ordering constraint called out in greffon/manager#101.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 639c6db8e5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Keep watching until both cert files are written

When _install_cert copies /root/pem.crt and then /root/cert.key as separate Docker writes (app/workers/register.py), this now wakes on the .crt event. inotifywait --help confirms that without --monitor it exits after one received event, so the script stops watching while nginx -t runs; if the key write lands in that window, that event is lost and nginx returns without starting/reloading until some unrelated later /root change. This makes initial cert install or renewal racy; add a debounce/quiet-period or otherwise wait for the cert/key pair before testing/reloading.

Useful? React with 👍 / 👎.

[omarsy]

Deep-review pass — applied hardening

Reviewed adversarially against busybox/ash and the alpine image. Confirmed: pidof present and matches the master; --exclude '\.crl$' correctly excludes revoked.crl while reacting to pem.crt/cert.key/ca.pem (the old --exclude .crt was unanchored and actually excluded the cert); the SIGHUP fix is sound; ash-compatible; no bashisms.

Hardening applied since the first push:

• inotifywait -t 60: the manager installs the cert as three separate put_archive writes, and the one-shot watch can miss a settling event that lands mid-nginx -t. The timeout makes a missed event recoverable (and brings a dead master back) without reloading when nothing changed.
• Surface nginx -t failures when the cert is present, instead of silently returning, so a genuinely broken config is visible in logs (previously indistinguishable from "certs not yet installed").

Known, accepted for this hotfix (follow-up): nginx still daemonizes, so PID 1 is this script and a master crash is only recovered via the 60s poll, not a container restart. Restructuring to a foreground master is out of scope here.

sh -n + shellcheck clean. Container runtime needs a staging pass (no Docker in the build env).

[omarsy]

@codex thanks, addressed in a18e10b (you reviewed the pre-hardening commit 639c6db).

Switched the watch to inotifywait --monitor, so no event is lost: the kernel queues events while nginx -t/reload runs, and the iteration where both pem.crt and cert.key are present always converges. An outer while loop respawns the monitor (with a short back-off) if it ever dies. This replaces the interim 60s-timeout approach and fully closes the cert/key install+renewal race you flagged, not just the "nginx never starts" case.

[chatgpt-codex-connector]

To use Codex here, create an environment for this repo.