Skip to content

refactor(docker): supervise the container with s6-overlay - #4019

Draft
gantoine wants to merge 2 commits into
masterfrom
refactor/s6-overlay-supervision
Draft

refactor(docker): supervise the container with s6-overlay#4019
gantoine wants to merge 2 commits into
masterfrom
refactor/s6-overlay-supervision

Conversation

@gantoine

Copy link
Copy Markdown
Member

Description

Explain the changes or enhancements you are proposing with this pull request.

Supersedes #3990, which fixed the unbounded shutdown wait in docker/init_scripts/init. This takes the alternative route discussed there: rather than bounding the wait, replace the hand-rolled supervisor with one that already solves this class of problem.

init was ~400 lines of process supervision: PID files, /proc polling, a 5s watchdog loop, and a trap-based shutdown whose wait was unbounded. A process that refused to exit stranded PID 1, and the container then sat Up serving nothing, which no restart policy can detect because policies only react to exits.

s6-overlay replaces it. Ordering and readiness gates that were sequential bash become a declared dependency graph, and the failure modes become configuration rather than code:

romm-init ─ romm-valkey ─ romm-valkey-ready ─ romm-migrations ─ romm-startup ─┬─ romm-gunicorn ─ romm-gunicorn-ready ─ romm-nginx
                                                                              ├─ romm-rq-worker
                                                                              ├─ romm-rq-scheduler
                                                                              ├─ romm-watcher
                                                                              └─ romm-sync-watcher

docker-entrypoint.sh is unchanged and still execs /init, which is now s6's. Every process, environment variable and conditional from the old script carried over.

Changes

  • timeout-kill (10s) bounds SIGTERM before SIGKILL, and flag-timeout-killpg escalates to the whole process group. No process can hold PID 1 open.
  • S6_BEHAVIOUR_IF_STAGE2_FAILS=2 exits the container when a startup step fails, so the restart policy retries. This is the self-healing path for the reported case, where the daemon's restart policy starts containers at host boot without honouring compose's depends_on and RomM comes up before its database.
  • valkey gets --shutdown-on-sigterm force, so it stops refusing to exit when the snapshot it writes on shutdown cannot be written. This is the root cause of the reported hang, and unlike setting REDIS_SAVE_POLICY empty it costs no persistence.
  • A crashed service is restarted on its own instead of taking the whole container down. Its leftover children are swept first, see the note below.
  • s6-overlay v3.2.3.2 is pinned by version and SHA256 per architecture, for linux/amd64 and linux/arm64.

One regression found and fixed during testing

Hard-killing nginx's master leaves its workers orphaned and still holding :8080, so every replacement fails to bind. The first version of this branch restart-looped forever on that.

master is not immune either: I tested it, and the same kill takes the whole container down (exit 1, after 20 Address in use errors), relying on the restart policy to clear the orphans. Each longrun now has a finish script that sweeps the dead service's process group before s6 brings it back, which makes this strictly better than today: the service recovers and the container stays up.

Behaviour change for read-only root filesystems

/run must now be mounted exec, since s6 executes its generated stage scripts from there, and Docker's --tmpfs /run defaults to noexec. Documented in docs/BACKEND_ARCHITECTURE.md.

Worth saying plainly that a read-only root filesystem does not fully work on master today either: it fails earlier, on /etc/nginx/conf.d/default.conf, before s6 is involved. I did not try to fix that pre-existing part here.

Testing

Built the image and exercised it against a real MariaDB. All results below are from the built image, not the fast iteration image:

Scenario Result
Startup, heartbeat HTTP 200 after 10s, no warnings or deprecations in the log
Read-only /redis-data, docker stop -t 60 5.6s, exit 0 (master: 60.3s, exit 137, SIGKILLed by docker)
Database unreachable at boot exits 1 after 9s, restart policy retries (master: wedges)
Service ignoring SIGTERM, with children 15.4s, exit 0, PID 1 never stranded
nginx, gunicorn and rq worker each kill -9ed all respawn, heartbeat 200, 0 bind failures, container stays up
Optional services off, then on stay down / come up, each started exactly once, no restart loop
External Redis (REDIS_HOST set) internal valkey stays down, 0 local valkey-server processes
Non-root (--user 1000:1000) heartbeat 200
Read-only rootfs (/run mounted exec) heartbeat 200

Also shellcheck and bash -n clean across all 21 shell scripts, and trunk fmt && trunk check clean.

Draft, because

Checklist

Please check all that apply.

  • I've tested the changes locally
  • I've updated relevant comments
  • I've assigned reviewers for this PR
  • I've added unit tests that cover the changes

There is no shell test harness in the repo. The verification above is manual, against real containers, and reproducible from the table.

AI assistance disclosure

Per CONTRIBUTING.md: this change was written with AI assistance (Claude Code). The AI wrote the s6 service tree, the Dockerfile changes and the docs, and ran the verification above. Reviewed before opening.

The init script was a hand-rolled supervisor: PID files, /proc polling, a
5s watchdog loop and a trap-based shutdown. Its shutdown wait was unbounded,
so a process that refused to exit stranded PID 1 and left the container up
and serving nothing, which no restart policy can detect.

s6-rc replaces it with a declared service graph. Ordering and readiness
gates that were sequential bash become dependencies, and the failure modes
become configuration:

- timeout-kill bounds SIGTERM before SIGKILL, and flag-timeout-killpg
  escalates to the whole process group.
- S6_BEHAVIOUR_IF_STAGE2_FAILS=2 exits the container when a startup step
  fails, so the restart policy retries instead of the container wedging.
- valkey gets --shutdown-on-sigterm force, so it stops refusing to exit
  when the snapshot it writes on shutdown cannot be written.

A crashed service is now restarted on its own instead of taking the whole
container down. Its leftover children are swept first, because children
reparented from a hard-killed master keep holding its listening sockets and
would make every replacement fail to bind.

Optional services have no conditional mechanism in s6-rc, so they start,
check their environment variable, and take themselves down with s6-svc -O.

Running with a read-only root filesystem now needs /run mounted exec, since
s6 executes its generated stage scripts from there.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@XenuIsWatching

Copy link
Copy Markdown
Contributor

Heads up: romm-rq-scheduler/run carries over the same four-flag gate as init, and that condition is the root cause of a bug I just opened #4033 for.

if [[ ${ENABLE_SCHEDULED_RESCAN:-false} != "true" &&
	${ENABLE_SCHEDULED_UPDATE_SWITCH_TITLEDB:-false} != "true" &&
	${ENABLE_SCHEDULED_UPDATE_LAUNCHBOX_METADATA:-false} != "true" &&
	${ENABLE_SCHEDULED_CLEANUP_ORPHANED_RESOURCES:-false} != "true" ]]; then
	debug_log "No scheduled tasks are enabled, not starting the RQ scheduler"
	disable_service

Nothing else respects that condition, in three separate ways:

  • startup.py registers cleanup_netplay, cleanup_upload_tmp and cleanup_zip_cache unconditionally, all enabled=True with cron strings. On a default install none of them ever run.
  • ENABLE_SCHEDULED_CONVERT_IMAGES_TO_WEBP, ENABLE_SCHEDULED_RETROACHIEVEMENTS_PROGRESS_SYNC and ENABLE_SYNC_PUSH_PULL gate periodic tasks but are absent from the list, so enabling any one alone silently does nothing.
  • The filesystem watcher defers its rescans through the scheduler (watcher.py:214), and ENABLE_RESCAN_ON_FILESYSTEM_CHANGE is not in the list either. That is the reported failure: the watcher accumulates delayed scan_platforms entries nothing will ever execute, fix(scan): refuse concurrent scans and stop queued ones #3974's guard reads them as a queued scan, and every manual scan is refused permanently with "A scan is already in progress".

Worth noting entrypoint.sh starts the scheduler unconditionally and always has, so the two entry paths already disagree about this on master. #4033 drops the condition in init to match.

Not a blocker for this PR, just flagging so it does not get re-introduced. Whichever of the two lands second will need to carry the change across, and in the s6 layout that means dropping the disable_service branch from romm-rq-scheduler/run entirely.

Separately, and much smaller: this drops the romm service healthcheck that was in #3990. I have no objection, the bounded shutdown is the part that mattered and --shutdown-on-sigterm force is a better fix than what I had. Just noting it since a hung PID 1 stays unobservable without one.

I verified the valkey change against 5.1.0 on my own hardware, read-only /redis-data with snapshots configured:

shutdown-on-sigterm=default  ->  26s, exit 137   (SIGKILLed by docker)
shutdown-on-sigterm=force    ->   1s, exit 0

Nice.

@gantoine gantoine added the on-hold Pending further research or blocked by another issue label Aug 1, 2026
The s6 service carried over the four-flag ENABLE_SCHEDULED_* gate from the
init script, which #4033 has since removed on master. Since this branch
deletes that script, the fix would be lost on rebase.

Nothing else respects the condition: startup.py registers the netplay,
upload-tmp and zip-cache cleanups unconditionally, three more flags gate
periodic tasks absent from the list, and the watcher defers its rescans
through the scheduler. Any of those leaves jobs in the scheduler registry
with no process to run them, and a stuck delayed scan_platforms entry reads
to the concurrent scan guard as a scan already queued, refusing every
manual scan.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

on-hold Pending further research or blocked by another issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants