fix(docker): bound the shutdown wait so a stuck process cannot strand PID 1 - #3990
fix(docker): bound the shutdown wait so a stuck process cannot strand PID 1#3990XenuIsWatching wants to merge 1 commit into
Conversation
|
why a DB migrate loop when the healthcheck would cover this case? |
|
Fair — given the shutdown fix, the migrate loop is largely redundant. Once PID 1 can actually exit, the path self-heals: migrations fail → One correction on the premise though: a healthcheck can't recover this on its own, because Docker healthchecks take no action — they only set status. Nothing restarts an unhealthy container in plain Compose; that needs an external agent (autoheal) or an orchestrator (Swarm/k8s). I added the healthcheck because a hung PID 1 is otherwise completely invisible, not because it fixes anything. And if you mean the existing RomM started two minutes before its database with that So: want me to cut it down to just the bounded wait + healthcheck, or keep a slimmer version of the DB wait? The only argument for keeping one is cost — each failed |
|
@XenuIsWatching i want you to use your meat brain and decide 😉 |
… PID 1 stop_process_pid signalled each managed process and then waited on it forever. A process that refuses to exit therefore wedges PID 1, and the container sits "Up" serving nothing. No restart policy can detect that, because policies only react to exits. Valkey reaches exactly that state: it aborts its own shutdown when the snapshot it writes on exit cannot be written, so on a read-only /redis-data it catches SIGTERM and keeps running. Because the init script traps EXIT, every fatal error path runs shutdown(), including a failed startup migration when the database is not reachable yet. The daemon's restart policy does not honour compose's depends_on at host boot, so RomM regularly starts before its database and takes that path. The result is permanent downtime that needs a manual restart. Wait STOP_TIMEOUT (default 10s) for a signalled process, then escalate to SIGKILL, with a second bounded wait and a warning if even that fails. The failing path now ends in an exit, which a restart policy can act on. Add a healthcheck to the compose example as well, since a hung PID 1 is otherwise invisible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
95c25f4 to
0597158
Compare
alright, i updated it along with the description and title |
|
Closing in favour of #4019, which goes after the same hang from the other end: replacing the hand-rolled supervisor with s6-overlay, where the bounded shutdown is a config file rather than code we maintain. Your diagnosis is what made that possible, and the root cause (valkey aborting its own shutdown on a failed snapshot) is fixed directly there with |
Description
Explain the changes or enhancements you are proposing with this pull request.
stop_process_pidsignals each managed process and then waits on it forever:A process that refuses to exit wedges PID 1. The container then sits
Upserving nothing, and no restart policy can detect it, because policies only react to exits.Valkey reaches exactly that state. It aborts its own shutdown when the snapshot it writes on exit cannot be written, so on a read-only
/redis-datait catches SIGTERM, fails the save, and keeps running.Getting there is routine, because
trap 'exited=1 && shutdown' SIGINT SIGTERM EXITmeans every fatal error path runsshutdown(), exactly when the environment is most likely to be broken. The path I hit is a failed startup migration: the daemon's restart policy starts every container at once at host boot and does not honour compose'sdepends_on(that is applied bydocker compose up), so RomM regularly comes up before its database,alembic upgrade headfails, anderror_logends inexit 1.Observed three times on a host whose storage layer briefly remounts read-only during boot:
psinside the wedged container showed onlybash /init,valkey-server, and a livesleep 0.1. Every reboot needed a manualdocker restart.Changes
stop_process_pidwaitsSTOP_TIMEOUT(default 10s), then escalates toSIGKILL, with a second bounded wait and a warning if even that fails. A process that will not exit can no longer strand PID 1.rommservice in the compose example, hitting the existing unauthenticated/api/heartbeat.STOP_TIMEOUTdocumented inenv.templateanddocs/BACKEND_ARCHITECTURE.md.Why this is the whole fix
Bounding the wait is enough on its own. Once PID 1 can exit, the failure becomes self-healing: migrations fail,
exit 1, shutdown completes, the container exits, andrestart: unless-stoppedbrings it back and retries until the database is up.An earlier revision of this PR also retried the migrations in-process, but per review that only saves a few container restarts, which the restart policy already handles. It is a startup-time optimization riding along in a hang fix, and it cost two tunables the project would have to support, so I dropped it. Worth revisiting separately if the restart churn bothers anyone in practice.
One clarification, since it came up in review and is easy to get backwards: a healthcheck cannot recover this by itself. Docker healthchecks take no action, they only set status; nothing restarts an unhealthy container in plain Compose without an external agent (autoheal) or an orchestrator. The healthcheck here exists to make a hung PID 1 observable, not to fix it. Note also that the mechanism that failed in the trace above was
romm-db's healthcheck combined withdepends_on: condition: service_healthy, which the daemon skips at host boot.Note on
REDIS_SAVE_POLICY48259b3 (after 5.0.0) added
REDIS_SAVE_POLICY, and setting it empty sidesteps the valkey hang, since valkey skips the shutdown snapshot when no save point is configured. But the default is3600 1, which configures one, so the hang is reachable on current master. That is a workaround at the cost of persistence, not a fix, and it only addresses valkey, whereas the unbounded wait is a hazard for every managed process.Testing
stop_process_pidis exercised by a test script that extracts the function verbatim from the init script and runs it under the sameerrexit/nounset/pipefail/inherit_errexitoptions. An untested error path is the reason this bug exists, so I did not want to eyeball it.SIGKILL.trap "" TERM(valkey's exact behaviour): killed afterSTOP_TIMEOUTwith the expected warning, instead of hanging.Also verified:
bash -n,shellcheck,shfmt 3.6.0,prettier 3.9.5,yamllint 1.38.0all clean. The compose example produces no new yamllint findings versusmaster(23 pre-existing on both).docker compose config, and the healthcheck command runs successfully inside a live container.$$defers expansion to the container so a customROMM_PORTstill works; without that, anyone settingROMM_PORTwould get a permanently unhealthy container.--save "3600 1"it survives SIGTERM, with--save ""it exits cleanly.Checklist
Please check all that apply.
The init script has no test harness in-repo, so the coverage above lives in a standalone script that sources the function rather than in a committed suite. Happy to add a
bats(or similar) target if the project wants shell tests committed.AI assistance disclosure
Per
CONTRIBUTING.md: this change was written with AI assistance (Claude Code). The root-cause diagnosis came from a real reproduction on my own hardware; the AI wrote the patch and the test script, and I reviewed the result. NoFixes #: I searched open issues and did not find one covering this.