fix: deployment data-safety and container hardening #66
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/deploy-data-safety"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Hardens the Docker deployment so data cannot silently land in the container layer and the container no longer runs as root.
What changed
/app/data/doujin-manager.db→/app/data/db/doujin-manager.db(Program.cs, README, DEPLOYMENT.md). Previously the default DB sat one level above thedbvolume mount point, so an unconfigured deployment wrote its database to the ephemeral container layer and lost it on recreate.aspnet:10.0base image creates theappuser (uid 1654) but does not switch to it — only the chiseled variants do. The Dockerfile now chowns/app/dataand setsUSER $APP_UIDexplicitly.docker-publish.ymlgains atestjob (restore/build/test inbackend/, mirroring ci.yml) thatbuild-and-pushdepends on, so a red main can never ship as:latest. A per-refconcurrencygroup queues runs instead of racing:latestpushes (no cancel-in-progress, so a half-cancelled push cannot leave a stale tag).mem_limit: 2g(image recompression / large ZIP uploads cannot OOM the NAS),no-new-privileges, and json-file log rotation (10m × 3) so unbounded stdout logs stop eating the system partition.app/,vendor/,.venv/, etc. — onlybackend/is COPYed, and the old context was several GB.dbvolume, and tochown -R 1654:1654any root-owned bind mounts before upgrading.Follow-ups
concurrencyis a relatively recent Forgejo Actions feature; confirm the git.kagaku.eu instance version honors it (it is silently ignored — harmlessly — if unsupported, but the per-ref queuing of:latestpushes would then not be enforced).🤖 Generated with Claude Code
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 85.7%
DoujinManager.Infrastructure - 94.5%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 85.5%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 28.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A PR that reads the base image Dockerfile instead of assuming~ You caught that
aspnet:10.0creates theappuser (uid 1654, exposed as$APP_UID) but only the chiseled variants actually switch to it — I verified the base of this branch, and yes, the old image had noUSERdirective at all. It ran as root. And you even found the old troubleshooting row that claimed it ran as 1654 and fixed it. Delightful~ ♡The
USER $APP_UID+chown -R $APP_UID:$APP_UIDpattern is exactly the canonical Microsoft-documented form, the compose hardening trio is correctly motivated (SkiaSharp recompression OOM vs. the NAS ♪), and thetestjob mirrors ci.yml line-for-line (sameruns-on: dotnet, sameworking-directory, minus coverage) withneeds:chaining. I ran the mirrored job locally: build 0 errors, 386/386 tests pass (16 + 62 + 1 + 307).But fufu~... this PR is titled data-safety, and its own safety net — §8.1 — walks the operator straight into a crash loop. I can't let that pass~ ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
deploy/DEPLOYMENT.md:243-246 — "Named volumes keep working (the image chowns /app/data)" is false for pre-existing volumes, which is precisely the case §8.1 exists for. Docker copies image ownership into a volume only when the volume is empty; the image-layer
chownnever touches content already inside a mounted volume. Every prior compose deployment ran as root (your own diagnosis! the base image had noUSER) withdb:/app/data/dbmounted andDOUJIN_MANAGER_DB_PATHpointing into it — so the nameddbvolume holds a root-owned 644 db file inside a root-owned 755 directory (dir ownership was copied from the old root-built image on first mount). The new container (uid 1654) cannot open the database nor create-wal/-shmfiles →MigrateAsyncthrows on startup → crash loop underrestart: unless-stopped. The standard Portainer upgrade following this note as written dies.Fix: document the one-time ownership repair, e.g.
docker run --rm -v <stack>_db:/data alpine chown -R 1654:1654 /data(compose prefixes the volume name with the stack name), and reword the sentence to "empty or newly created named volumes keep working; volumes holding root-owned data from the root-era images need a one-time chown." (The bind-mount advice in point 2 is correct — those map to host dirs, hostchown -Rworks. It's only the named-volume claim that's wrong.)deploy/DEPLOYMENT.md:233-238 — the
docker cprescue drops a non-1654-owned file into the volume with no chown step.docker cpinto a container preserves the host file's uid:gid (long-standing behavior, docker/cli#4382) — on a Synology that's uid 1026 or similar, and even root ≠ 1654. Afterdocker restart, the app (1654) can't write the rescued database → same crash loop, one step later. Fufu~ you wouldn't leave a rescue procedure that strangles the patient, would you? ♡Fix: add
docker exec --user root doujin-manager chown 1654:1654 /app/data/db/doujin-manager.dbbefore thedocker restart(--user rootworks regardless of the image'sUSER).docs/adr/0010-angela-style-docker-data-layout.md:18-33 and docs/PROJECT_PLAN.md:453-471 still document the old layout as current. The Dockerfile's own comment cites ADR-0010 as the authority for the data directories ("used by ADR-0010") — but after this PR, that authority's conceptual paths and env-var table describe a database location that no longer exists anywhere in the codebase, plus a
DOUJIN_MANAGER_LOG_DIRthat was never implemented. PROJECT_PLAN.md's storage section has the same stale paths. This repo amends ADRs in place when reality moves (ADR-0020 carries a "this supersedes the original approach" note in its Consequences) — please do the same here.Fix: a 2-3 line note in ADR-0010's Consequences (default DB path moved to
/app/data/db/doujin-manager.dbso the default lands inside thedbvolume mount; container runs as non-root uid 1654) and the matching path updates in PROJECT_PLAN.md.💡 Little ideas (non-blocking)~
testjob, docs/app-only pushes to main now also pay ~4-5 minutes of backend restore/build/test (my local mirror took5 min). Still the right call (tag pushes can't take a), but the stated cost is now understated.pathsfilter — learned the hard way with v0.2.0, I read the commentconcurrency, the fallback is the same group on thebuild-and-pushjob (job-level concurrency is the older, more widely supported form). Worth resolving that follow-up issue once observed.✅ What I liked~
USERbehavior verified rather than assumed, and the stale troubleshooting row hunted down and corrected. That's the kind of care that makes me giddy~ ♪chown -Rfolded into the existingmkdirRUN— one layer, no image bloat, correct$APP_UID:$APP_UID(the base image creates both user and group with that id).concurrencygroup with nocancel-in-progress, and the reasoning written down: a half-cancelled push must never leave a stale:latest. Tag refs get their own group (refs/tags/...vsrefs/heads/main) so releases never queue behind main builds. Correct on every axis I checked..dockerignoreadditions match reality — the repo root really does carryapp/andvendor/(GB-scale Flutter context), and the Dockerfile only ever COPYsbackend/. All three modified YAML files parse cleanly and the job graph (test→build-and-push) is wired correctly./app/data/backups), and README + DEPLOYMENT env tables were both updated in lockstep.Test coverage: no new executable branches — Program.cs is a literal swap, the rest is Dockerfile/compose/workflow/docs which this repo's test suite can't reach; the workflow's first live execution happens on merge (push-to-main only). YAML validated structurally and against the ci.yml sibling instead.
Fix those §8.1 ownership holes and stamp the ADR, and this is a genuinely excellent hardening PR~ The container escaping root made me very happy. ♡
Automated review by Jibril · 2026-08-15
CI/CD: absent for head
dfed5f8(PR just opened, no coverage bot yet) · Local checks: build 0 errors (36 pre-existing warnings), 386/386 tests passThanks for the thorough review, Jibril — all three blocking items plus idea 1 are addressed in
084a3fa:/app/dataownership; volumes holding root-owned data from the root-era images need a one-time repair. Documented the exact command with the stack-prefix caveat:docker run --rm -v <stack>_db:/data alpine chown -R 1654:1654 /data(to be run per data volume with the old stack stopped). The bind-mount advice stays as it was.docker exec --user root doujin-manager chown 1654:1654 /app/data/db/doujin-manager.dbbefore thedocker restart, with a comment notingdocker cppreserves the host file's uid:gid./app/data/db/doujin-manager.dbso the default lands inside thedbvolume mount; container runs as non-root uid 1654;DOUJIN_MANAGER_LOG_DIRwas never implemented — logs go to stdout/stderr). PROJECT_PLAN.md's storage section now shows the current DB path and drops the never-implemented log dir/env var.Non-blocking ideas:
concurrency, which we can only observe after merge.Verification: docs/comment-only changes; workflow YAML re-validated (parses cleanly), and the backend still builds with 0 errors and passes 386/386 tests.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Scarlet, you didn't just patch my blockers — you understood them~ ♡
084a3fais a surgical +25/-9 across exactly 4 files (DEPLOYMENT.md, ADR-0010, PROJECT_PLAN.md, docker-publish.yml comment), zero production drift — I byte-verified:git diff dfed5f8..084a3fa -- backend/ app/ deploy/Dockerfile deploy/docker-compose.ymlis empty. Let me confirm each fix against the actual tree~Verdict: ✅ Looks good to me~
✅ All three blockers, closed properly~
/app/dataownership into an empty volume on first mount)" and names the exact failure mode ("the image-layerchownnever touches content already inside a mounted volume"). That is precisely the Docker volume-copy semantic I was worried about, stated as a mechanism rather than a promise — my favorite kind of documentation~ And the repair commanddocker run --rm -v <stack>_db:/data alpine chown -R 1654:1654 /dataincludes the stack-prefix caveat and "with the old stack stopped" (running chown under a live container risks a mid-write race on the SQLite file — you thought of that too). ♪docker cprescue chown —docker exec --user root doujin-manager chown 1654:1654 /app/data/db/doujin-manager.dbnow sits between the copy and the restart, with a comment explaining why (docker cppreserves the host uid:gid). The rescue procedure no longer strangles the patient. Fufu~ ♡DOUJIN_MANAGER_LOG_DIRnever implemented → stdout/stderr), exactly the in-place amendment pattern this repo uses (ADR-0020 style). PROJECT_PLAN.md's path block and env table now show/app/data/db/doujin-manager.dband the never-implemented log dir/env var are gone. I swept the whole tree: the only remaining old-path references are the ones that belong — the upgrade note naming its copy source, and ADR-0010's historical decision body under its amendment. Clean~And non-blocking idea 1 too: the workflow header now prices the trade at "~4-5-minute test + image build" — matches my local mirror's ~5 min, so the comment finally tells the truth. The concurrency follow-up stays open post-merge as you reasoned — correct, it's only observable live.
✅ What I liked~
docker cppreserves the host file's uid:gid". Future operators will understand the class of bug, not just this instance. That makes me very happy~<stack>_dbanddbhas bitten every Portainer user at least once, and now it won't~Verified: YAML re-parses cleanly; no executable code changed in this commit (byte-identical production tree to the round-1
dfed5f8review, which already passed 386/386 locally against me), so no new branches to test — the first live run of the workflow still happens on merge, as before.The container escaped root, the data lands in the volume, and the rescue paths no longer crash-loop. This is a genuinely excellent hardening PR and I'm satisfied~ Ship it. ♡
Automated review by Jibril · 2026-08-15
CI/CD: stale for
084a3fa(coverage bot 6221 covers dfed5f8-era run) · Local checks: skipped — docs-only commit, production tree byte-identical todfed5f8(verified via git diff)