fix(docker): address round-2 review of the all-in-one image (#1042)

- Reverted the engine-resolution reordering. Resolving before the PostgreSQL
  wait broke a real upgrade path: probePgData deliberately reports "occupied"
  when Postgres is unreachable, and the target database may not exist until the
  wait loop creates it — so an install that declares no DATABASE_CLIENT but has
  a populated legacy SQLite file would resolve to ambiguous-both-populated and
  refuse to boot. The resolver is back in its original position and the wait is
  now skipped only when sqlite3 is EXPLICITLY declared, which is what this image
  does. Installs that declare nothing keep today's ordering byte for byte.

- Backup subdirectories are created at startup. Creating /data/backup alone was
  not enough: on a bind mount the subdirectories baked into the image are
  hidden, and the backup services do not create them, so /backup/picpeak and
  /backup/database failed with ENOENT.

- BACKUP_DIR points at the real path, not the /backup symlink. `chown -R` on a
  symlink argument acts on the link, so the target under a bind-mounted /data
  was never adopted and the writability preflight failed as UID 1001.

- Branding is rendered at startup, not baked at build. A build-time sed fixed
  the literal-token bug but silently made BRAND_TITLE/BRAND_DESCRIPTION inert
  for this image. docker-entrypoint.aio.sh keeps index.html as a template and
  renders it with envsubst before exec'ing wait-for-db.sh — same contract, same
  defaults and same two-variable allowlist as the frontend image.

- /fonts/ added to the SPA fallback exclusions. Both font secureStatic mounts
  call next() on a miss, so a missing font was answering 200 text/html.

Not changed: the workflow_dispatch push opt-out. That push-decision block is
byte-identical in build-backend, build-aio and build-frontend, so it is
pre-existing behaviour rather than something this job introduces; the reviewer
agreed on being shown the evidence that fixing it only here would make AIO the
odd one out.

Verified on a rebuilt image, named volume and bind mount: both healthy, default
title "PicPeak" and BRAND_TITLE="Studio Nord Galerie" rendering in <title> and
og:title, /backup/{picpeak,database} present and writable on the host side.
This commit is contained in:
Paul Nothaft
2026-08-16 23:25:48 +02:00
parent 167eeaa271
commit b507932d69
4 changed files with 106 additions and 50 deletions
+17 -10
View File
@@ -95,7 +95,8 @@ RUN rm -rf /usr/local/lib/node_modules/npm /usr/local/bin/npm /usr/local/bin/npx
# not provide that executable. The compose backend image omits it because # not provide that executable. The compose backend image omits it because
# it always runs Postgres; this image defaults to SQLite, so without it the # it always runs Postgres; this image defaults to SQLite, so without it the
# built-in database backup fails with ENOENT. # built-in database backup fails with ENOENT.
RUN apk add --no-cache dumb-init postgresql-client sqlite ffmpeg su-exec \ # gettext — envsubst, used by docker-entrypoint.aio.sh to render index.html
RUN apk add --no-cache dumb-init postgresql-client sqlite ffmpeg su-exec gettext \
fontconfig ttf-dejavu ttf-liberation poppler-utils exiftool && \ fontconfig ttf-dejavu ttf-liberation poppler-utils exiftool && \
fc-cache -f fc-cache -f
@@ -108,14 +109,15 @@ COPY --chown=nodejs:nodejs backend/ ./
COPY --from=frontend-builder --chown=nodejs:nodejs /build/dist ./public COPY --from=frontend-builder --chown=nodejs:nodejs /build/dist ./public
# index.html ships with ${BRAND_TITLE} / ${BRAND_DESCRIPTION} placeholders that # index.html ships with ${BRAND_TITLE} / ${BRAND_DESCRIPTION} placeholders that
# frontend/docker-entrypoint.sh substitutes at container start. This image runs # frontend/docker-entrypoint.sh renders at container start. This image runs no
# no nginx and therefore never runs that entrypoint, so without this every AIO # nginx and never invokes that entrypoint, so keep the built file as a template
# install would serve a literal "${BRAND_TITLE}" as its <title> and in every # and render it on startup instead (docker-entrypoint.aio.sh) — baking the
# og:/twitter: card. Render them here with the same defaults that entrypoint # defaults at build time would fix the literal-token bug but silently make
# applies when the vars are unset. # BRAND_TITLE/BRAND_DESCRIPTION inert for this image.
RUN sed -i 's/\${BRAND_TITLE}/PicPeak/g; s/\${BRAND_DESCRIPTION}/Photo gallery shared with PicPeak./g' \ RUN mv ./public/index.html ./public/index.html.template
./public/index.html && \
! grep -q '\${BRAND_' ./public/index.html COPY --chown=nodejs:nodejs docker-entrypoint.aio.sh /usr/local/bin/docker-entrypoint.aio.sh
RUN chmod +x /usr/local/bin/docker-entrypoint.aio.sh
RUN chmod -R a+r /app && chmod +x wait-for-db.sh RUN chmod -R a+r /app && chmod +x wait-for-db.sh
@@ -142,6 +144,11 @@ ENV SERVE_FRONTEND=true \
DATABASE_PATH=/data/db/picpeak.db \ DATABASE_PATH=/data/db/picpeak.db \
STORAGE_PATH=/data/storage \ STORAGE_PATH=/data/storage \
LOG_DIR=/data/logs \ LOG_DIR=/data/logs \
# Point the startup chown/preflight at the REAL directory, not the /backup
# symlink: `chown -R` on a symlink argument acts on the link itself, so the
# target under a bind-mounted /data would never be adopted and the
# writability preflight would fail as UID 1001.
BACKUP_DIR=/data/backup \
PORT=3000 PORT=3000
# /backup is where migrations 029 + 030 seed the built-in backup destinations # /backup is where migrations 029 + 030 seed the built-in backup destinations
@@ -169,4 +176,4 @@ HEALTHCHECK --interval=30s --timeout=5s --start-period=60s --retries=3 \
# No USER directive: the container starts as root so wait-for-db.sh can chown a # No USER directive: the container starts as root so wait-for-db.sh can chown a
# bind-mounted /data to UID 1001 before dropping privileges via su-exec (#484). # bind-mounted /data to UID 1001 before dropping privileges via su-exec (#484).
ENTRYPOINT ["dumb-init", "--"] ENTRYPOINT ["dumb-init", "--"]
CMD ["./wait-for-db.sh", "node", "server.js"] CMD ["/usr/local/bin/docker-entrypoint.aio.sh", "node", "server.js"]
+1 -1
View File
@@ -967,7 +967,7 @@ if (typeof serveFrontendIndexPath === 'string') {
// /uploads are backend-owned file routes whose middleware calls next() when // /uploads are backend-owned file routes whose middleware calls next() when
// the file is missing, and swallowing that into a 200 index.html would turn // the file is missing, and swallowing that into a 200 index.html would turn
// a missing photo into an HTML body served under an image URL. // a missing photo into an HTML body served under an image URL.
const BACKEND_OWNED = ['/api/', '/photos/', '/thumbnails/', '/uploads/', '/health']; const BACKEND_OWNED = ['/api/', '/photos/', '/thumbnails/', '/uploads/', '/fonts/', '/health'];
app.get('*', (req, res, next) => { app.get('*', (req, res, next) => {
if (BACKEND_OWNED.some((prefix) => req.path.startsWith(prefix))) return next(); if (BACKEND_OWNED.some((prefix) => req.path.startsWith(prefix))) return next();
return res.sendFile(serveFrontendIndexPath); return res.sendFile(serveFrontendIndexPath);
+53 -39
View File
@@ -29,7 +29,7 @@ unset _pair _var _file _cur
# single-container image (#1042) points all three under one mounted volume, # single-container image (#1042) points all three under one mounted volume,
# so these must follow the same env vars the app itself reads rather than # so these must follow the same env vars the app itself reads rather than
# hard-coding /app — otherwise the checks below guard directories nothing uses. # hard-coding /app — otherwise the checks below guard directories nothing uses.
DATA_DIRS="${STORAGE_PATH:-/app/storage} ${DATA_DIR:-/app/data} ${LOG_DIR:-/app/logs}" DATA_DIRS="${STORAGE_PATH:-/app/storage} ${DATA_DIR:-/app/data} ${LOG_DIR:-/app/logs} ${BACKUP_DIR:-/backup}"
# Create the roots before touching them. With the compose layout each of the # Create the roots before touching them. With the compose layout each of the
# three is its own mount point, so they always exist — but the single-container # three is its own mount point, so they always exist — but the single-container
@@ -72,46 +72,20 @@ for _dir in $DATA_DIRS; do
fi fi
done done
# Resolve which database engine this boot should use (#1038) BEFORE migrations # Everything below this point is PostgreSQL readiness. On SQLite the database is
# run — and, since #1042, before the PostgreSQL readiness wait below, so a # a file this process opens itself — there is no daemon to wait for — so the
# SQLite install never blocks on a Postgres that will never answer. # loop would block forever on a host that will never answer. That is the
# single-container case (#1042), where the image DECLARES sqlite3.
# #
# An install that has been unknowingly running on SQLite (the image used to # The test is deliberately the raw env var, not the resolver's answer. Resolving
# leave NODE_ENV unset, so # first would change behaviour for installs that declare nothing: probePgData
# knexfile.js fell back to sqlite3 and ignored DB_HOST/DB_USER/DB_PASSWORD) # reports "occupied" when Postgres is unreachable, and the target database may
# keeps serving from its SQLite file instead of coming up against an empty # not exist until the loop below creates it — so an unset-client install with a
# Postgres. The exported value survives the `exec` below, so the migration # populated legacy SQLite file would resolve to ambiguous-both-populated and
# runner and the server agree on the engine. # refuse to start. Those installs keep today's ordering exactly: wait, create,
RESOLVED_DB_CLIENT="$(node scripts/resolve-db-engine.js)" # then resolve.
RESOLVER_STATUS=$?
# Exit 3 means two populated databases with no record of which is current
# (#1038). Starting either would hide the other's data, so stop here — the
# resolver has already printed what to do.
if [ "$RESOLVER_STATUS" = "3" ]; then
exit 1
fi
# Validate rather than trust: anything unexpected on stdout (a stray log line
# from a library that writes to the console) must not become DATABASE_CLIENT,
# which would break knexfile for every process that follows.
case "$RESOLVED_DB_CLIENT" in
pg|sqlite3)
export DATABASE_CLIENT="$RESOLVED_DB_CLIENT"
;;
"")
>&2 echo "Database engine resolver returned nothing; falling back to the configured client."
;;
*)
>&2 echo "Database engine resolver returned an unexpected value; ignoring it and falling back to the configured client."
;;
esac
# Everything below this point is PostgreSQL readiness. On SQLite the
# database is a file this process opens itself — there is no daemon to
# wait for — so the wait loop would block forever on a host that will
# never answer. That is exactly the single-container case (#1042), where
# SQLite is the documented default and no DB_HOST is set.
if [ "${DATABASE_CLIENT:-}" = "sqlite3" ]; then if [ "${DATABASE_CLIENT:-}" = "sqlite3" ]; then
echo "Database engine: sqlite3 — skipping the PostgreSQL readiness wait." echo "Database engine: sqlite3 declared — skipping the PostgreSQL readiness wait."
else else
host="${DB_HOST:-postgres}" host="${DB_HOST:-postgres}"
@@ -191,9 +165,49 @@ echo "Ensuring storage directories exist..."
STORAGE_BASE="${STORAGE_PATH:-/app/storage}" STORAGE_BASE="${STORAGE_PATH:-/app/storage}"
mkdir -p "$STORAGE_BASE/events/active" "$STORAGE_BASE/events/archived" "$STORAGE_BASE/thumbnails" 2>/dev/null || true mkdir -p "$STORAGE_BASE/events/active" "$STORAGE_BASE/events/archived" "$STORAGE_BASE/thumbnails" 2>/dev/null || true
# Backup destinations seeded by migrations 029 + 030 (/backup/picpeak and
# /backup/database). Creating the root alone is not enough: on a bind mount the
# subdirectories baked into the image are hidden, and the backup services do not
# create them, so a scheduled or manual backup fails with ENOENT.
BACKUP_BASE="${BACKUP_DIR:-/backup}"
mkdir -p "$BACKUP_BASE/picpeak" "$BACKUP_BASE/database" 2>/dev/null || true
# Resolve which database engine this boot should use (#1038) BEFORE migrations # Resolve which database engine this boot should use (#1038) BEFORE migrations
# run, while the Postgres target is still untouched. An install that has been # run, while the Postgres target is still untouched. An install that has been
# Resolve which database engine this boot should use (#1038) BEFORE migrations
# run — and, since #1042, before the PostgreSQL readiness wait below, so a
# SQLite install never blocks on a Postgres that will never answer.
#
# An install that has been unknowingly running on SQLite (the image used to
# leave NODE_ENV unset, so
# knexfile.js fell back to sqlite3 and ignored DB_HOST/DB_USER/DB_PASSWORD)
# keeps serving from its SQLite file instead of coming up against an empty
# Postgres. The exported value survives the `exec` below, so the migration
# runner and the server agree on the engine.
RESOLVED_DB_CLIENT="$(node scripts/resolve-db-engine.js)"
RESOLVER_STATUS=$?
# Exit 3 means two populated databases with no record of which is current
# (#1038). Starting either would hide the other's data, so stop here — the
# resolver has already printed what to do.
if [ "$RESOLVER_STATUS" = "3" ]; then
exit 1
fi
# Validate rather than trust: anything unexpected on stdout (a stray log line
# from a library that writes to the console) must not become DATABASE_CLIENT,
# which would break knexfile for every process that follows.
case "$RESOLVED_DB_CLIENT" in
pg|sqlite3)
export DATABASE_CLIENT="$RESOLVED_DB_CLIENT"
;;
"")
>&2 echo "Database engine resolver returned nothing; falling back to the configured client."
;;
*)
>&2 echo "Database engine resolver returned an unexpected value; ignoring it and falling back to the configured client."
;;
esac
# Run migrations (use safe runner in production). Invoked via node directly — # Run migrations (use safe runner in production). Invoked via node directly —
# the runtime image no longer ships npm (see Dockerfile: its bundled deps kept # the runtime image no longer ships npm (see Dockerfile: its bundled deps kept
# tripping CVE scanners while npm itself never runs in production). # tripping CVE scanners while npm itself never runs in production).
+35
View File
@@ -0,0 +1,35 @@
#!/bin/sh
# All-in-one entrypoint (#1042).
#
# The compose deployment renders index.html in the frontend container via
# frontend/docker-entrypoint.sh. This image runs no nginx, so that never
# happens — and Vite does not substitute ${BRAND_TITLE} at build time, so the
# built index.html ships the placeholders verbatim. Without this step every
# AIO install would serve a literal "${BRAND_TITLE}" as its <title> and in
# every og:/twitter: card.
#
# Kept as a startup step rather than a build-time sed so BRAND_TITLE and
# BRAND_DESCRIPTION behave the same way they do for the frontend image:
# set the env var, restart the container, new branding. Baking them at build
# would silently make those variables inert.
set -e
TEMPLATE=/app/public/index.html.template
RENDERED=/app/public/index.html
if [ -f "$TEMPLATE" ]; then
# Same defaults frontend/docker-entrypoint.sh applies when the vars are unset.
: "${BRAND_TITLE:=PicPeak}"
: "${BRAND_DESCRIPTION:=Photo gallery shared with PicPeak.}"
# Substitute ONLY these two, exactly like the frontend entrypoint: letting
# envsubst expand every ${...} it finds would mangle unrelated content.
export BRAND_TITLE BRAND_DESCRIPTION
envsubst '${BRAND_TITLE} ${BRAND_DESCRIPTION}' < "$TEMPLATE" > "$RENDERED"
# The app runs as UID 1001 after the privilege drop below; the render happens
# while we are still root, so hand the result over explicitly.
chown nodejs:nodejs "$RENDERED" 2>/dev/null || true
fi
exec ./wait-for-db.sh "$@"