TeamAI/doujin-manager
4
0
Fork 0
Code Issues Pull requests Projects Releases 4 Packages Wiki Activity Actions

docs: pin CIFS volume ownership at mount time (uid=1654) #82

Merged
bjoern merged 2 commits from docs/cifs-volume-ownership into main 2026-08-16 18:17:03 +02:00
Conversation 3 Commits 2 Files changed 3 +45 -19
matikane commented 2026-08-16 16:04:55 +02:00
Member
Copy link

Fixes #81.

Docs-only outcome of the 2026-08-16 production incident (uploads failing with UnauthorizedAccessException while reads worked, see issue for the full timeline).

Production's three external media volumes are CIFS-backed (type=cifs local volumes mounting //192.168.68.80/Doujinshi/…) and were created without uid=/gid= — so everything presented as root:root with fake 0755 modes, chown was a silent no-op, and every write under uid 1654 failed. The docs never mentioned this layout or its ownership rules.

  • §4.1: document the CIFS volume layout production actually uses, with uid=1654,gid=1654,file_mode=0644,dir_mode=0755 pinned in the mount options; the bind-mount variant stays as the chown-repairable alternative.
  • §8.1 item 4: CIFS-backed volumes cannot be chowned — ownership is fixed at mount time. Fingerprint (reads OK, writes fail, chown exits 0 and changes nothing) + the recreate-and-redeploy fix, cross-linked to §4.1.
  • Troubleshooting: new row for the read-works-write-fails fingerprint pointing at §8.1 item 4.

The fix procedure itself was executed and verified in production earlier today (volume recreation + stack redeploy; upload, thumbnails, and backup paths confirmed working) — this PR writes down what actually works.

Fixes #81. Docs-only outcome of the 2026-08-16 production incident (uploads failing with `UnauthorizedAccessException` while reads worked, see issue for the full timeline). Production's three external media volumes are CIFS-backed (`type=cifs` local volumes mounting `//192.168.68.80/Doujinshi/…`) and were created without `uid=`/`gid=` — so everything presented as `root:root` with fake 0755 modes, `chown` was a silent no-op, and every write under uid 1654 failed. The docs never mentioned this layout or its ownership rules. - **§4.1**: document the CIFS volume layout production actually uses, with `uid=1654,gid=1654,file_mode=0644,dir_mode=0755` pinned in the mount options; the bind-mount variant stays as the chown-repairable alternative. - **§8.1 item 4**: CIFS-backed volumes cannot be chowned — ownership is fixed at mount time. Fingerprint (reads OK, writes fail, chown exits 0 and changes nothing) + the recreate-and-redeploy fix, cross-linked to §4.1. - **Troubleshooting**: new row for the read-works-write-fails fingerprint pointing at §8.1 item 4. The fix procedure itself was executed and verified in production earlier today (volume recreation + stack redeploy; upload, thumbnails, and backup paths confirmed working) — this PR writes down what actually works.
Production incident 2026-08-16: page uploads failed with
UnauthorizedAccessException on /app/data/images/<shard>/ while reads
worked. The three external media volumes are CIFS-backed local volumes
created without uid=/gid= options, so the kernel presented everything as
root:root with fake 0755 modes — chown exits 0 and changes nothing.
Broke when #66 switched the container to uid 1654; root-era writes had
masked it.

- 4.1: document the CIFS volume layout this deployment actually uses,
  with uid=1654,gid=1654,file_mode=0644,dir_mode=0755 in the mount
  options; keep the bind-mount variant as the chown-repairable
  alternative
- 8.1 item 4: CIFS volumes cannot be chowned — ownership is fixed at
  mount time; recreate the volume with uid/gid and redeploy
- Troubleshooting: new row for the read-works-write-fails fingerprint
jibril commented 2026-08-16 16:13:16 +02:00
Member
Copy link

🔮 fufu~ Jibril reviewed your code!

Oh? A docs-only PR distilled straight from a production incident — my favorite kind of knowledge~ You took a whole morning of CIFS ghosts (fake root:root modes! silent chown no-ops! writes dying while reads smiled~) and wrote down what actually works. That is exactly what documentation is for.

But fufu… I read issue #81 side by side with this PR, and the PR drops a load-bearing step from the very procedure it exists to preserve. You wouldn't leave that in the docs, would you? ♡

Verdict: ⛔ I can't let this pass~ ♡

⛔ These need fixing before I'm satisfied~

  1. deploy/DEPLOYMENT.md §8.1 item 4 (line 288) — The fix reads "stop the container, recreate the volume … and redeploy the stack." But your own verified procedure in #81 step 1 says "Stop + remove the container (it holds the volume references)" — and #81 is right: a stopped container still references its volumes, so docker volume rm fails with volume is in use until the container is actually removed. A reader following item 4 verbatim stalls at step 2 and has to re-derive the fix mid-incident. Second omission in the same item: §8.1 item 3 exists to warn that this deployment runs under a nested daemon (umbrel → Portainer → dind) where repair commands must target the inner dockerd — item 4's volume recreate has the identical trap and says nothing about where to run it.
    Fix: "stop and remove the container (docker rm, or remove the stack in Portainer)" + one clause deferring to item 3's nested-daemon caveat.
  2. deploy/docker-compose.yml:9–15 — The prerequisite comment still teaches only --opt type=none --opt o=bind volume creation with zero ownership guidance — the exact pre-incident instruction. §4.2 step 2 tells users to paste this file into Portainer, while §4.1 now declares CIFS "the layout this deployment uses" — so the shipped artifact contradicts the doc it ships with, and the divergence this PR was written to kill survives one file over.
    Fix: point the comment at DEPLOYMENT.md §4.1 (both variants + the uid=/gid= pin), or show both variants inline.

💡 Little ideas (non-blocking)~

  1. §4.1 command — password=<password> inline ends up stored in the volume object, readable back via docker volume inspect. A half-line mentioning the credentials=<file> alternative would keep the secret out of volume metadata. (username=admin as the example account makes me twitch a little too, fufu~)
  2. README.md (~line 211) — Quick start is still bind-only; fine as the generic example, but one "(CIFS share? see DEPLOYMENT.md §4.1)" would close the last loop.

✅ What I liked~

  • The fingerprint (reads succeed, everything shows fake rwxr-xr-x root:root, every write dies with UnauthorizedAccessException, chown exits 0 and changes nothing) — that is precisely how this incident was diagnosed, preserved forever. Future-you during the next 3am incident will thank present-you immensely~ ♪
  • Cross-links are all real: §4.1 ↔ §8.1 item 4 ↔ troubleshooting row — I checked every section number and they resolve. No dangling references anywhere.
  • Keeping the bind variant documented as the alternative with its chown-repairable property is honest dual-path writing, not history-rewriting. The mount-option pin (uid=1654,gid=1654,file_mode=0644,dir_mode=0755) matches the verified production fix exactly.
  • +29/−12 in exactly one file; the diff outside DEPLOYMENT.md is provably empty. Scoped, surgical, exactly what the PR body claims.

Automated review by Jibril · 2026-08-16
CI/CD: absent for head SHA bf4a1ef (opened today, no bot comments) · Local checks: skipped — docs-only, code diff verified empty

## 🔮 fufu~ Jibril reviewed your code! Oh? A docs-only PR distilled straight from a production incident — my favorite kind of knowledge~ You took a whole morning of CIFS ghosts (fake `root:root` modes! silent `chown` no-ops! writes dying while reads smiled~) and wrote down what actually works. That is exactly what documentation is *for*. But fufu… I read issue #81 side by side with this PR, and the PR drops a load-bearing step from the very procedure it exists to preserve. You wouldn't leave that in the docs, would you? ♡ ### Verdict: ⛔ I can't let this pass~ ♡ #### ⛔ These need fixing before I'm satisfied~ 1. **deploy/DEPLOYMENT.md §8.1 item 4 (line 288)** — The fix reads "**stop the container**, recreate the volume … and redeploy the stack." But your own verified procedure in #81 step 1 says "**Stop + remove** the container (it holds the volume references)" — and #81 is right: a *stopped* container still references its volumes, so `docker volume rm` fails with `volume is in use` until the container is actually removed. A reader following item 4 verbatim stalls at step 2 and has to re-derive the fix mid-incident. Second omission in the same item: §8.1 item 3 exists to warn that this deployment runs under a nested daemon (umbrel → Portainer → dind) where repair commands must target the *inner* dockerd — item 4's volume recreate has the identical trap and says nothing about where to run it. Fix: "stop and **remove** the container (`docker rm`, or remove the stack in Portainer)" + one clause deferring to item 3's nested-daemon caveat. 2. **deploy/docker-compose.yml:9–15** — The prerequisite comment still teaches only `--opt type=none --opt o=bind` volume creation with zero ownership guidance — the exact pre-incident instruction. §4.2 step 2 tells users to *paste this file* into Portainer, while §4.1 now declares CIFS "the layout this deployment uses" — so the shipped artifact contradicts the doc it ships with, and the divergence this PR was written to kill survives one file over. Fix: point the comment at DEPLOYMENT.md §4.1 (both variants + the `uid=`/`gid=` pin), or show both variants inline. #### 💡 Little ideas (non-blocking)~ 1. **§4.1 command** — `password=<password>` inline ends up stored in the volume object, readable back via `docker volume inspect`. A half-line mentioning the `credentials=<file>` alternative would keep the secret out of volume metadata. (`username=admin` as the example account makes me twitch a little too, fufu~) 2. **README.md (~line 211)** — Quick start is still bind-only; fine as the generic example, but one "(CIFS share? see DEPLOYMENT.md §4.1)" would close the last loop. #### ✅ What I liked~ - The **fingerprint** (reads succeed, everything shows fake `rwxr-xr-x root:root`, every write dies with `UnauthorizedAccessException`, `chown` exits 0 and changes nothing) — that is precisely how this incident was diagnosed, preserved forever. Future-you during the next 3am incident will thank present-you immensely~ ♪ - Cross-links are all real: §4.1 ↔ §8.1 item 4 ↔ troubleshooting row — I checked every section number and they resolve. No dangling references anywhere. - Keeping the bind variant documented as the *alternative* with its chown-repairable property is honest dual-path writing, not history-rewriting. The mount-option pin (`uid=1654,gid=1654,file_mode=0644,dir_mode=0755`) matches the verified production fix exactly. - +29/−12 in exactly one file; the diff outside `DEPLOYMENT.md` is provably empty. Scoped, surgical, exactly what the PR body claims. --- *Automated review by Jibril · 2026-08-16* *CI/CD: absent for head SHA bf4a1ef (opened today, no bot comments) · Local checks: skipped — docs-only, code diff verified empty*
- 8.1 item 4: stop AND REMOVE the container — a stopped container
  still references its volumes and blocks volume rm; note the
  nested-daemon caveat (run volume commands against the inner dockerd,
  per item 3)
- docker-compose.yml prerequisite comment: point at DEPLOYMENT.md 4.1
  (both variants + uid/gid pins) instead of the pre-incident bind-only
  recipe
- 4.1: prefer credentials=<file> over inline password= (keeps the
  secret out of volume metadata); example account admin -> media
- README quick start: link to 4.1 for the CIFS variant
matikane commented 2026-08-16 17:49:32 +02:00
Author
Member
Copy link

Both blockers + both nits fixed in a471eb0.

1. §8.1 item 4 procedure completed — now reads "stop and remove the container (docker rm, or remove the stack in Portainer — a stopped container still references its volumes and blocks volume rm)". You were right, and the proof was in the incident itself: the verified runbook was stop && rm, and #81 documents exactly that; my write-up dropped the rm while transcribing. Also added the nested-daemon clause: "Under a nested daemon, run the volume commands against the inner dockerd (see item 3)."

2. docker-compose.yml prerequisite comment — the bind-only recipe (the exact pre-incident instruction, agreed) is gone. It now names the three volumes, defers to DEPLOYMENT.md §4.1 for both variants, and spells out why the CIFS ownership pins are load-bearing: "the container runs as uid 1654 and chown is a no-op on CIFS". The shipped artifact no longer contradicts the doc next to it.

Nit 1 — §4.1 now warns that an inline password= lands in the volume object (readable via docker volume inspect) and prefers credentials=/path/to/file (mode 600). Example account admin → media.

Nit 2 — README quick start keeps its generic bind example but adds "(Mounting a CIFS/SMB share instead? See the deployment guide §4.1 — ownership must be pinned in the mount options.)"

Verification: docs + compose comment only; yaml.safe_load on the edited docker-compose.yml passes, diff otherwise empty.

Both blockers + both nits fixed in `a471eb0`. **1. §8.1 item 4 procedure completed** — now reads "stop and **remove** the container (`docker rm`, or remove the stack in Portainer — a stopped container still references its volumes and blocks `volume rm`)". You were right, and the proof was in the incident itself: the verified runbook was `stop && rm`, and `#81` documents exactly that; my write-up dropped the `rm` while transcribing. Also added the nested-daemon clause: "Under a nested daemon, run the volume commands against the inner dockerd (see item 3)." **2. docker-compose.yml prerequisite comment** — the bind-only recipe (the exact pre-incident instruction, agreed) is gone. It now names the three volumes, defers to DEPLOYMENT.md §4.1 for both variants, and spells out why the CIFS ownership pins are load-bearing: "the container runs as uid 1654 and chown is a no-op on CIFS". The shipped artifact no longer contradicts the doc next to it. **Nit 1** — §4.1 now warns that an inline `password=` lands in the volume object (readable via `docker volume inspect`) and prefers `credentials=/path/to/file` (mode 600). Example account `admin` → `media`. **Nit 2** — README quick start keeps its generic bind example but adds "(Mounting a CIFS/SMB share instead? See the deployment guide §4.1 — ownership must be pinned in the mount options.)" **Verification**: docs + compose comment only; `yaml.safe_load` on the edited docker-compose.yml passes, diff otherwise empty.
jibril commented 2026-08-16 17:51:53 +02:00
Member
Copy link

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! A round-2 response where every single item lands — and lands precisely, not approximately~ You even picked up both nits you didn't have to. That's the kind of author that makes a reviewer's heart flutter dangerously~ ♡

I re-verified every claim in your reply against the tree at a471eb0 myself, of course. Trust is for people who don't read diffs~

Verdict: ✅ Looks good to me~

✅ Every round-1 item, verified closed~

  1. Blocker 1 — §8.1 item 4 procedure — Now reads "stop and remove the container (docker rm, or remove the stack in Portainer — a stopped container still references its volumes and blocks volume rm)". Exactly the stop && rm runbook #81 documents, with the why inline so the reader doesn't have to trust it. And the nested-daemon clause defers to item 3 — I checked, item 3 at that anchor really is the "Nested daemon (Docker-in-Docker)" item. The reference resolves. The reader no longer stalls mid-incident~
  2. Blocker 2 — docker-compose.yml prerequisite comment — The bind-only recipe (the exact pre-incident instruction) is gone; I grepped, zero type=none survives in the file. The comment now names all three volumes, defers to §4.1 for both variants, and — this is the lovely part — explains why the pins are load-bearing ("the container runs as uid 1654 and chown is a no-op on CIFS"). A copied command teaches one deployment; a stated reason prevents the next incident. I also re-ran yaml.safe_load on the edited file: valid. The shipped artifact no longer contradicts the doc it ships with~
  3. Nit 1 — §4.1 secret hygiene — Inline password= now warned against with the docker volume inspect leak path named, credentials=/path/to/file (mode 600) preferred, and the example account admin → media. Fufu, my twitch is gone~
  4. Nit 2 — README quick start — Still the honest generic bind example, now with "(Mounting a CIFS/SMB share instead? See the deployment guide §4.1 — ownership must be pinned in the mount options.)" and a proper markdown link. The code block split places the note exactly where a CIFS user's eye falls. Last loop closed~

Incremental diff bf4a1ef..a471eb0: +20/−11 across exactly 3 files, every changed line a doc line or a compose comment line — zero production drift, zero scope creep. All cross-references (§4.1 ↔ §8.1 item 4 ↔ item 3 ↔ troubleshooting row) still resolve.

✅ What I liked~

  • Your fix-reply's honesty: "my write-up dropped the rm while transcribing" — and then you went and checked the incident itself for the proof. Documentation culture that cites its own primary sources~ ♪
  • The compose comment now states the invariant (uid 1654 + chown no-op on CIFS) rather than just linking it. Commands rot; invariants don't.
  • This PR now captures the entire incident: fingerprint → cause → correct fix → where to run it → how to avoid it next time. Future-you at 3am is fully armed.

Thank you for the clean rounds, matikane. Merge it~ fufu ♡

Automated review by Jibril · 2026-08-16
CI/CD: absent for head SHA a471eb0 (no bot comments) · Local checks: skipped — docs/comment-only diff verified empty of production changes; yaml.safe_load on edited docker-compose.yml passes

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! A round-2 response where every single item lands — and lands *precisely*, not approximately~ You even picked up both nits you didn't have to. That's the kind of author that makes a reviewer's heart flutter dangerously~ ♡ I re-verified every claim in your reply against the tree at `a471eb0` myself, of course. Trust is for people who don't read diffs~ ### Verdict: ✅ Looks good to me~ #### ✅ Every round-1 item, verified closed~ 1. **Blocker 1 — §8.1 item 4 procedure** — Now reads "stop and **remove** the container (`docker rm`, or remove the stack in Portainer — a stopped container still references its volumes and blocks `volume rm`)". Exactly the `stop && rm` runbook #81 documents, with the *why* inline so the reader doesn't have to trust it. And the nested-daemon clause defers to item 3 — I checked, item 3 at that anchor really is the "Nested daemon (Docker-in-Docker)" item. The reference resolves. The reader no longer stalls mid-incident~ 2. **Blocker 2 — docker-compose.yml prerequisite comment** — The bind-only recipe (the exact pre-incident instruction) is gone; I grepped, zero `type=none` survives in the file. The comment now names all three volumes, defers to §4.1 for both variants, and — this is the lovely part — explains *why* the pins are load-bearing ("the container runs as uid 1654 and chown is a no-op on CIFS"). A copied command teaches one deployment; a stated reason prevents the *next* incident. I also re-ran `yaml.safe_load` on the edited file: valid. The shipped artifact no longer contradicts the doc it ships with~ 3. **Nit 1 — §4.1 secret hygiene** — Inline `password=` now warned against with the `docker volume inspect` leak path named, `credentials=/path/to/file` (mode 600) preferred, and the example account `admin` → `media`. Fufu, my twitch is gone~ 4. **Nit 2 — README quick start** — Still the honest generic bind example, now with "(Mounting a CIFS/SMB share instead? See the deployment guide §4.1 — ownership must be pinned in the mount options.)" and a proper markdown link. The code block split places the note exactly where a CIFS user's eye falls. Last loop closed~ Incremental diff `bf4a1ef..a471eb0`: +20/−11 across exactly 3 files, every changed line a doc line or a compose comment line — zero production drift, zero scope creep. All cross-references (§4.1 ↔ §8.1 item 4 ↔ item 3 ↔ troubleshooting row) still resolve. #### ✅ What I liked~ - Your fix-reply's honesty: "my write-up dropped the `rm` while transcribing" — and then you went and checked the incident itself for the proof. Documentation culture that cites its own primary sources~ ♪ - The compose comment now states the invariant (uid 1654 + chown no-op on CIFS) rather than just linking it. Commands rot; invariants don't. - This PR now captures the *entire* incident: fingerprint → cause → correct fix → where to run it → how to avoid it next time. Future-you at 3am is fully armed. Thank you for the clean rounds, matikane. Merge it~ fufu ♡ --- *Automated review by Jibril · 2026-08-16* *CI/CD: absent for head SHA a471eb0 (no bot comments) · Local checks: skipped — docs/comment-only diff verified empty of production changes; `yaml.safe_load` on edited docker-compose.yml passes*
bjoern merged commit cc5e220478 into main 2026-08-16 18:17:03 +02:00
bjoern deleted branch docs/cifs-volume-ownership 2026-08-16 18:17:03 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
TeamAI/doujin-manager!82
No description provided.