fix(docker): repair data directory ownership per directory - #155
Merged
Conversation
A chunked import returned 500 before its first byte, because
/app/data/recordings was root-owned while /app/data itself was owned by
uid 1000. Both entrypoints decided whether to repair ownership by
reading the data root alone:
[ "$(stat -c %u /app/data)" = "1000" ] || chown -R appuser:appuser /app/data
That guard was written to answer whether the recursive chown was needed,
and was read as whether the tree was correctly owned. The two coincide
only while the tree is always created as a whole, so a child recreated
under an already-correct parent was skipped, and the preceding mkdir -p
is a no-op on a directory that already exists.
The api service made that state reachable on its own. It declared a
second bind of ./data/recordings at /app/recordings, which nothing has
ever read; every storage path resolves under /app/data through
RECORDINGS_DIR. Docker materialises each declared bind source
independently and as root, so deleting the host recordings directory was
enough to have it recreated root-owned beneath an appuser-owned parent,
with no manual step involved. Remove the mount and the image directory
it was paired with.
Repair moves into backend/entrypoint_common.sh, sourced by both
entrypoints, which had already begun to drift apart. Each write-critical
directory is created if missing and checked on its own, and only the
ones that are wrong are repaired. A mis-owned data root still takes a
single recursive pass, since that is the first boot on a fresh ./data
when the tree is empty; a correct root costs one stat per directory and
never walks a large recordings library.
Repair is also non-fatal now. Both entrypoints run under set -e, and
some supported bind-mount backends, notably a Windows-drive mount under
Docker Desktop, either reject chown or ignore it. A container that
refused to boot there would hide the diagnostic it is trying to
surface, so failures warn and continue.
Refs: #153, docs/DEVELOPMENT.md
init_chunked_import committed its recording row before creating the upload temp directory, so a filesystem failure escaped as an unhandled 500 with the row already durable. Every retry added another recording stuck in UPLOADING that had never received a byte; one report accumulated ten of them against a single transcript. Those rows are not only clutter. _has_active_live_capture reads any UPLOADING recording as a capture in flight, which suppresses the per-task model cache release, so orphans pin the live ASR model in worker memory indefinitely. On a single-card host that is the whole VRAM budget, held by rows representing uploads that never started. Wrap the directory creation, delete the row it just created, and return 503 naming the cause rather than 500 naming nothing. The sibling upload endpoints already write their file before inserting a row, so they cannot orphan anything and are unchanged. Add probe_recordings_storage, which creates and removes a real file rather than testing for existence. A bind mount owned by root leaves every path present and every write refused, so existence answers the wrong question. Add a reaper for rows already stranded, run by the daily cleanup task. Its criteria are deliberately strict, so an upload or live capture in flight can never be caught: older than 24 hours, no chunk rows, no file on disk, and nothing in its temp directory. Anything that received a single byte satisfies one of the last three. Rows are soft-deleted, in line with every other deletion path. Refs: #153
A mis-owned bind mount was invisible to every readiness check: the database, queue, workers and ffmpeg were all fine, so the install reported itself healthy right up to the first failed upload. The container healthcheck agreed, because /api/health proves only that the process is running, which was never in doubt. Add storage as a readiness component, backed by the write probe, and treat it as blocking in the pipeline summary. The API also logs the condition explicitly at startup, so the cause appears in the log at boot rather than in a traceback hours later. /api/health is deliberately left alone. The frontend and nginx services both gate on the api container reporting healthy, so failing the liveness probe would take the UI down and leave an operator with no surface to read the diagnosis on. The condition belongs on the readiness surface, which already models blocking reasons, while the container keeps serving. Settings > System renders its health cards from an explicit list rather than from whatever the API returns, so the card and the shared type are added alongside. Refs: #153, docs/DEPLOYMENT.md
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Description
A chunked import returned HTTP 500 before its first byte whenever
/app/data/recordingswas root-owned while/app/dataitself was owned by uid 1000, and each failed attempt left another recording stranded inUPLOADING.Both entrypoints decided whether to repair ownership by reading the data root alone:
That guard was written to answer whether the recursive chown was needed, and was read as whether the tree was correctly owned. The two coincide only while the tree is always created as a whole, so a child recreated under an already-correct parent was skipped, and the preceding
mkdir -pis a no-op on a directory that already exists.The
apiservice made that state reachable on its own. It declared a second bind of./data/recordingsat/app/recordings, which nothing has ever read; every storage path resolves under/app/datathroughRECORDINGS_DIR. Docker materialises each declared bind source independently and as root, so deleting the host recordings directory was enough to have it recreated root-owned beneath an appuser-owned parent, with no manual step involved.Three further gaps, each fixed here:
PathManager.repair_data_permissionsruns after the gosu drop, as appuser, so it can only log the condition and never fix it. That is the misleadingCould not set permissions on directory /app/data/recordingsline in the report./api/healthproves the process is running, which was never in doubt, so the container reported healthy while structurally unable to accept an upload.UPLOADINGrows, and_has_active_live_capturereads any such row as a capture in flight. That suppresses the per-task model cache release, so orphaned rows pin the live ASR model in worker memory indefinitely.Changes:
backend/entrypoint_common.sh, sourced by both entrypoints, which had already begun to drift apart. Each write-critical directory is created if missing and checked on its own, and only the ones that are wrong are repaired. A mis-owned data root still takes a single recursive pass, since that is the first boot on a fresh./datawhen the tree is empty; a correct root costs one stat per directory and never walks a large recordings library. Repair is non-fatal, because some supported bind-mount backends reject or ignorechownand a container that refused to boot there would hide the diagnostic it is trying to surface../data/recordings:/app/recordingsmount is removed, along with the image directory it was paired with.init_chunked_importrolls back the row it just committed and returns 503 naming the cause. Newprobe_recordings_storagecreates and removes a real file rather than testing for existence, since a root-owned mount leaves every path present and every write refused./api/healthis deliberately unchanged. Thefrontendandnginxservices both gate on theapicontainer reporting healthy, so failing the liveness probe would take the UI down and leave an operator with no surface to read the diagnosis on.No new dependencies.
Fixes #153
Type of change
Checks run
source .venv/bin/activate && pytest(1134 passed)python scripts/check.py(lint, format, whitespace, filesize, heldpins, typecheck, docs, alembic, tests)cd frontend && npm run lintcd frontend && npm run test(320 passed, 52 files)cd frontend && npm run buildpython3 scripts/validate_docs.pypython3 scripts/validate_alembic.pyMigration impact
Documentation impact
docs/DEPLOYMENT.mdgains a Data Directory Ownership section covering the per-directory repair, the best-effort behaviour on bind-mount backends that rejectchown, how the condition is surfaced, and the removed mount for anyone carrying a Compose override derived from an older example.docs/DEVELOPMENT.mddrops the same mount from its two example stacks.Security impact
The root-to-
appuserprivilege boundary described indocs/SECURITY.mdis preserved unchanged: the entrypoints still enter as root only to repair bind-mount ownership and immediately drop viagosu. Only the repair block between those two points was rewritten. Verified end-to-end below.Manual verification
Container-level, against the real
nojoin-api:localandnojoin-worker:localimages in isolated throwaway containers:/app/datauid 1000,/app/data/recordingsuid 0) and confirmedrecording_upload_temp_dir(id, create=True)raisedPermissionError: [Errno 13] ... 'data/recordings/temp', matching the traceback in the issue.probe_recordings_storage()returnsok=True.probe_recordings_storage()returnsok=Falsewith the actionable message, confirming the health component and startup log fire on the real failure.worker_entrypoint.shend-to-end, as the release smoke test does: repair runs, thenidreportsuid=1000(appuser). The privilege drop is intact.Shell-level, four ownership scenarios:
./data: single recursive pass, all directories created and owned correctly.appuserintorecordings/tempsucceeds.Not applicable to this change: browser capture flows (
frontend/src/lib/capture/untouched) and the recording context menu (RecordingCard.tsx/Sidebar.tsxuntouched). The only frontend change is the new Storage readiness card in Settings > System, which renders through the existing health card component.