From 8f6e81e59ced844e4ab38782de80fd4fb7d71b77 Mon Sep 17 00:00:00 2001 From: Brandon Haney <121782102+Brandon-Haney@users.noreply.github.com> Date: Fri, 5 Jun 2026 12:23:49 -0500 Subject: [PATCH] Fix: chown directories created during caching to PUID/PGID The container runs as root so it can move files of any ownership, but raw os.makedirs() then left new array/cache folders owned by root:root (0755). On Unraid this blocked Sonarr/Radarr from importing into newly created library folders (#178). Add a shared create_dir_with_ownership() helper in core/system_utils.py that chowns every newly created directory level to PUID/PGID (or the source file's owner when those are unset), the same way file copies are already handled. Route the move/copy destination directory creation in the caching, eviction, upgrade, restore, symlink, and audit paths through it. FileUtils.create_directory_with_permissions now delegates to the helper so there is a single implementation. --- core/app.py | 12 ++- core/file_operations.py | 21 ++-- core/system_utils.py | 142 ++++++++++++++++++---------- tests/test_system_utils.py | 79 +++++++++++++++- tools/audit_cache.py | 7 +- web/services/cache_service.py | 6 +- web/services/maintenance_service.py | 14 +-- 7 files changed, 206 insertions(+), 75 deletions(-) diff --git a/core/app.py b/core/app.py index f670ad60..d7f02198 100644 --- a/core/app.py +++ b/core/app.py @@ -26,7 +26,7 @@ from core import __version__ from core.config import ConfigManager from core.logging_config import LoggingManager, reset_warning_error_flag -from core.system_utils import SystemDetector, FileUtils, SingleInstanceLock, get_disk_usage, get_array_direct_path, detect_zfs, set_zfs_prefixes, format_bytes +from core.system_utils import SystemDetector, FileUtils, SingleInstanceLock, get_disk_usage, get_array_direct_path, detect_zfs, set_zfs_prefixes, format_bytes, create_dir_with_ownership from core.plex_api import PlexManager, OnDeckItem from core.file_operations import MultiPathModifier, SiblingFileFinder, FileFilter, FileMover, PlexcachedRestorer, CacheTimestampTracker, WatchlistTracker, OnDeckTracker, CachePriorityManager, PlexcachedMigration, get_media_identity, find_matching_plexcached, is_directory_level_file from core.pinned_media import PinnedMediaTracker, resolve_pins_to_paths @@ -801,7 +801,9 @@ def _ensure_cache_path_exists(self, cache_path: str) -> None: """Ensure a cache directory exists, creating it if necessary.""" if not os.path.exists(cache_path): try: - os.makedirs(cache_path, exist_ok=True) + # Honor PUID/PGID so a cache dir created while running as root + # isn't left root:root. + create_dir_with_ownership(cache_path) logging.info(f"[CONFIG] Created missing cache directory: {cache_path}") except OSError as e: raise FileNotFoundError(f"Cannot create cache directory {cache_path}: {e}") @@ -1749,7 +1751,7 @@ def _handle_upgrade_plexcached(self, old_path: str, new_path: str, if not os.path.isfile(new_plexcached) and os.path.isfile(new_cache_path): try: new_array_dir = os.path.dirname(new_array_path) - os.makedirs(new_array_dir, exist_ok=True) + create_dir_with_ownership(new_array_dir, new_cache_path) shutil.copy2(new_cache_path, new_plexcached) # Verify size match src_size = os.path.getsize(new_cache_path) @@ -2659,7 +2661,7 @@ def _run_smart_eviction(self, needed_space_bytes: int = 0) -> tuple: import shutil try: array_direct_dir = os.path.dirname(array_direct_path) - os.makedirs(array_direct_dir, exist_ok=True) + create_dir_with_ownership(array_direct_dir, cache_path) shutil.copy2(cache_path, array_direct_path) logging.debug(f"Copied upgraded file to array: {array_direct_path}") # Verify copy succeeded using array-direct path @@ -2695,7 +2697,7 @@ def _run_smart_eviction(self, needed_space_bytes: int = 0) -> tuple: # If we copy to /mnt/user/, Unraid's cache policy may put the file # back on cache (if shareUseCache=yes), causing data loss array_direct_dir = os.path.dirname(array_direct_path) - os.makedirs(array_direct_dir, exist_ok=True) + create_dir_with_ownership(array_direct_dir, cache_path) shutil.copy2(cache_path, array_direct_path) logging.info(f"[EVICTION] Created array copy before eviction: {os.path.basename(array_direct_path)}") # Verify copy succeeded using array-direct path diff --git a/core/file_operations.py b/core/file_operations.py index 8cc3a633..d1bed3c7 100644 --- a/core/file_operations.py +++ b/core/file_operations.py @@ -16,7 +16,7 @@ import re from core.logging_config import get_console_lock -from core.system_utils import resolve_user0_to_disk, get_disk_free_space_bytes, get_disk_number_from_path, get_array_direct_path, format_bytes +from core.system_utils import resolve_user0_to_disk, get_disk_free_space_bytes, get_disk_number_from_path, get_array_direct_path, format_bytes, create_dir_with_ownership if TYPE_CHECKING: from core.config import PathMapping @@ -2364,10 +2364,10 @@ def _migrate_single_file(self, args: Tuple[str, str, str]) -> int: self._active_files[thread_id] = (filename, file_size) self._print_progress() - # Ensure directory exists + # Ensure directory exists with PUID/PGID ownership (running as root, + # a raw makedirs would leave new array folders root:root). array_dir = os.path.dirname(plexcached_file) - if not os.path.exists(array_dir): - os.makedirs(array_dir, exist_ok=True) + create_dir_with_ownership(array_dir, cache_file) # Copy cache file to array as .plexcached (preserving ownership on Linux) if self.is_unraid: @@ -3109,7 +3109,7 @@ def _create_symlink(self, symlink_path: str, target_path: str) -> bool: os.remove(symlink_path) parent_dir = os.path.dirname(symlink_path) if parent_dir and not os.path.isdir(parent_dir): - os.makedirs(parent_dir, exist_ok=True) + create_dir_with_ownership(parent_dir, target_path) os.symlink(target_path, symlink_path) logging.debug(f"Created symlink: {symlink_path} -> {target_path}") return True @@ -5342,7 +5342,10 @@ def combined_stop_check(): display_src = self._translate_to_host_path(cache_file) if self.file_utils.is_docker else None array_direct_file = get_array_direct_path(array_file) array_direct_dir = os.path.dirname(array_direct_file) - os.makedirs(array_direct_dir, exist_ok=True) + # Create with PUID/PGID ownership — running as root, a raw + # makedirs would leave new array folders root:root and block + # Sonarr/Radarr from writing into them. + self.file_utils.create_directory_with_permissions(array_direct_dir, cache_file) def combined_stop_check(): if self._stop_requested: @@ -5373,7 +5376,9 @@ def combined_stop_check(): # CRITICAL: Copy to /mnt/user0/ (array direct), NOT /mnt/user/ (FUSE) array_direct_file = get_array_direct_path(array_file) array_direct_dir = os.path.dirname(array_direct_file) - os.makedirs(array_direct_dir, exist_ok=True) + # Create with PUID/PGID ownership (see note above) so new array + # folders aren't left root:root when running as root. + self.file_utils.create_directory_with_permissions(array_direct_dir, cache_file) def combined_stop_check(): if self._stop_requested: @@ -5530,7 +5535,7 @@ def _create_symlink(self, symlink_path: str, target_path: str) -> bool: # Ensure parent directory exists parent_dir = os.path.dirname(symlink_path) if parent_dir and not os.path.isdir(parent_dir): - os.makedirs(parent_dir, exist_ok=True) + create_dir_with_ownership(parent_dir, target_path) os.symlink(target_path, symlink_path) logging.debug(f"Created symlink: {symlink_path} -> {target_path}") diff --git a/core/system_utils.py b/core/system_utils.py index 36ee07ab..542cbf8f 100644 --- a/core/system_utils.py +++ b/core/system_utils.py @@ -109,6 +109,91 @@ def get_array_direct_path(user_share_path: str) -> str: return user_share_path +def create_dir_with_ownership( + path: str, + src_file_for_permissions: Optional[str] = None, + permissions: int = 0o777, +) -> None: + """Create ``path`` (and any missing parents) with correct ownership/permissions. + + The container runs as root so it can move files of any ownership, but raw + ``os.makedirs()`` then leaves new directories owned by ``root:root`` with a + umask-derived mode (typically 0755). On Unraid that blocks Sonarr/Radarr from + writing into newly created library folders. This helper chowns/chmods every + newly created directory level so they match PUID/PGID (or the source file's + owner when PUID/PGID are unset), the same way file copies are handled. + + Owner resolution: PUID/PGID environment variables take precedence; otherwise + the owner of ``src_file_for_permissions`` is used (when it exists). chown/chmod + are Linux-only and best-effort — failures (e.g. filesystem without ownership + support) are logged at DEBUG and never raised. No-op if ``path`` already exists. + + Args: + path: Directory path to create. + src_file_for_permissions: File whose owner is used when PUID/PGID are unset. + permissions: Mode applied to newly created directories (default 0o777). + """ + if os.path.exists(path): + return + + # Non-Linux (Windows dev/tests): just create the tree; no POSIX ownership. + if not hasattr(os, "chown"): + os.makedirs(path, exist_ok=True) + return + + # Resolve target ownership: PUID/PGID env override, else source file's owner. + target_uid: Optional[int] = None + target_gid: Optional[int] = None + for env_name, setter in (("PUID", "uid"), ("PGID", "gid")): + env_val = os.environ.get(env_name) + if env_val: + try: + if setter == "uid": + target_uid = int(env_val) + else: + target_gid = int(env_val) + except ValueError: + pass # Invalid env value — fall back to source ownership below. + + if (target_uid is None or target_gid is None) and src_file_for_permissions: + try: + stat_info = os.stat(src_file_for_permissions) + if target_uid is None: + target_uid = stat_info.st_uid + if target_gid is None: + target_gid = stat_info.st_gid + except OSError: + pass # Source missing — leave any unresolved id as None (skip chown). + + # Track every directory level we are about to create so we can chown each one, + # not just the leaf (os.makedirs only returns after creating the whole chain). + dirs_to_create = [] + current = path + while current and not os.path.exists(current): + dirs_to_create.append(current) + parent = os.path.dirname(current) + if parent == current: # Reached filesystem root. + break + current = parent + dirs_to_create.reverse() # Closest existing ancestor downward. + + original_umask = os.umask(0) + try: + os.makedirs(path, exist_ok=True) + for dir_path in dirs_to_create: + if target_uid is not None and target_gid is not None: + try: + os.chown(dir_path, target_uid, target_gid) + except (PermissionError, OSError) as e: + logging.debug(f"Could not set directory ownership for {dir_path}: {e}") + try: + os.chmod(dir_path, permissions) + except (PermissionError, OSError) as e: + logging.debug(f"Could not set directory permissions for {dir_path}: {e}") + finally: + os.umask(original_umask) + + def parse_size_bytes(size_str: str) -> int: """Parse a human-readable size string and return bytes. @@ -934,53 +1019,14 @@ def create_directory_with_permissions(self, path: str, src_file_for_permissions: If PUID/PGID environment variables are set, those values are used for ownership. Otherwise, the source file's ownership is used. + + Thin instance wrapper around the module-level create_dir_with_ownership() + so the same logic is shared with callers that have no FileUtils instance + (e.g. the web service layer). """ logging.debug(f"Creating directory with permissions: {path}") - - if not os.path.exists(path): - if self.is_linux: - # Get the permissions of the source file - stat_info = os.stat(src_file_for_permissions) - src_uid = stat_info.st_uid - src_gid = stat_info.st_gid - - # Use PUID/PGID if set, otherwise use source ownership - target_uid = self.puid if self.puid is not None else src_uid - target_gid = self.pgid if self.pgid is not None else src_gid - - # Find the first existing ancestor directory - # We need to track which directories we create so we can chown them all - dirs_to_create = [] - current = path - while current and not os.path.exists(current): - dirs_to_create.append(current) - parent = os.path.dirname(current) - if parent == current: # Reached root - break - current = parent - - # Reverse so we create from closest existing ancestor downward - dirs_to_create.reverse() - - original_umask = os.umask(0) - os.makedirs(path, exist_ok=True) - - # Set ownership and permissions on ALL newly created directories - for dir_path in dirs_to_create: - try: - os.chown(dir_path, target_uid, target_gid) - except (PermissionError, OSError) as e: - logging.debug(f"Could not set directory ownership for {dir_path}: {e}") - - try: - os.chmod(dir_path, self.permissions) - except (PermissionError, OSError) as e: - logging.debug(f"Could not set directory permissions for {dir_path}: {e}") - - os.umask(original_umask) - logging.debug(f"Directory created with permissions (Linux): {path} ({len(dirs_to_create)} level(s), uid={target_uid}, gid={target_gid})") - else: # Windows platform - os.makedirs(path, exist_ok=True) - logging.debug(f"Directory created (Windows): {path}") - else: - logging.debug(f"Directory already exists: {path}") \ No newline at end of file + if os.path.exists(path): + logging.debug(f"Directory already exists: {path}") + return + create_dir_with_ownership(path, src_file_for_permissions, permissions=self.permissions) + logging.debug(f"Directory created with permissions: {path}") \ No newline at end of file diff --git a/tests/test_system_utils.py b/tests/test_system_utils.py index 22f22d92..5732b7f0 100644 --- a/tests/test_system_utils.py +++ b/tests/test_system_utils.py @@ -9,7 +9,7 @@ import sys from datetime import datetime, timedelta from pathlib import Path -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch # conftest.py handles fcntl/apscheduler mocking and path setup from core.system_utils import ( @@ -18,6 +18,7 @@ translate_host_to_container_path, remove_from_exclude_file, remove_from_timestamps_file, + create_dir_with_ownership, ) @@ -262,3 +263,79 @@ def test_preserves_other_entries(self, tmp_path): assert len(data) == 2 assert "/mnt/cache/a.mkv" in data assert "/mnt/cache/c.mkv" in data + + +# ============================================================================ +# TestCreateDirWithOwnership +# ============================================================================ + +class TestCreateDirWithOwnership: + """Tests for create_dir_with_ownership(). + + Guards the fix for #178: directories created while running as root must be + chowned to PUID/PGID so they don't end up root:root and block Sonarr/Radarr. + """ + + def test_creates_nested_tree(self, tmp_path, monkeypatch): + """Creates the full directory chain when it doesn't exist.""" + monkeypatch.delenv("PUID", raising=False) + monkeypatch.delenv("PGID", raising=False) + target = tmp_path / "a" / "b" / "c" + + create_dir_with_ownership(str(target)) + + assert target.is_dir() + + def test_noop_when_exists(self, tmp_path): + """Existing directory is left untouched and no chown is attempted.""" + existing = tmp_path / "already" + existing.mkdir() + + with patch("core.system_utils.os.chown", create=True) as mock_chown: + create_dir_with_ownership(str(existing), str(existing)) + + mock_chown.assert_not_called() + + def test_chowns_each_created_level_to_puid_pgid(self, tmp_path, monkeypatch): + """PUID/PGID env values are applied to every newly created level.""" + monkeypatch.setenv("PUID", "99") + monkeypatch.setenv("PGID", "100") + src = tmp_path / "src.mkv" + src.write_text("data") + + # tmp_path/dest exists already; only show/season are newly created. + dest_root = tmp_path / "dest" + dest_root.mkdir() + target = dest_root / "Show" / "Season 01" + + with patch("core.system_utils.os.chown", create=True) as mock_chown, \ + patch("core.system_utils.os.chmod"): + create_dir_with_ownership(str(target), str(src)) + + assert target.is_dir() + chowned = {call.args[0] for call in mock_chown.call_args_list} + assert str(target) in chowned + assert str(dest_root / "Show") in chowned + # The pre-existing ancestor must NOT be re-chowned. + assert str(dest_root) not in chowned + # Every chown targets the configured PUID/PGID. + for call in mock_chown.call_args_list: + assert call.args[1:] == (99, 100) + + def test_invalid_puid_falls_back_to_source_owner(self, tmp_path, monkeypatch): + """A non-numeric PUID is ignored; the source file's owner is used.""" + monkeypatch.setenv("PUID", "notanumber") + monkeypatch.delenv("PGID", raising=False) + src = tmp_path / "src.mkv" + src.write_text("data") + src_stat = os.stat(str(src)) + target = tmp_path / "dest" / "Show" + + with patch("core.system_utils.os.chown", create=True) as mock_chown, \ + patch("core.system_utils.os.chmod"): + create_dir_with_ownership(str(target), str(src)) + + assert target.is_dir() + assert mock_chown.call_args_list, "chown should be attempted" + for call in mock_chown.call_args_list: + assert call.args[1:] == (src_stat.st_uid, src_stat.st_gid) diff --git a/tools/audit_cache.py b/tools/audit_cache.py index d4ef1edc..61c8592b 100644 --- a/tools/audit_cache.py +++ b/tools/audit_cache.py @@ -17,7 +17,7 @@ if PROJECT_ROOT_INIT not in sys.path: sys.path.insert(0, PROJECT_ROOT_INIT) -from core.system_utils import get_array_direct_path +from core.system_utils import get_array_direct_path, create_dir_with_ownership from core.file_operations import VIDEO_EXTENSIONS, SUBTITLE_EXTENSIONS, MEDIA_EXTENSIONS # Get script directory and resolve project root @@ -426,8 +426,9 @@ def sync_to_array(dry_run=True): print(f" -> {array_path}") else: try: - # Create destination directory if needed - os.makedirs(array_dir, exist_ok=True) + # Create destination directory if needed (honor PUID/PGID so new + # array folders aren't left root:root) + create_dir_with_ownership(array_dir, cache_path) # Use rsync to copy file (preserves permissions, shows progress) cmd = ['rsync', '-avh', '--progress', cache_path, array_path] diff --git a/web/services/cache_service.py b/web/services/cache_service.py index 5dedaa37..5ddbc415 100644 --- a/web/services/cache_service.py +++ b/web/services/cache_service.py @@ -11,7 +11,7 @@ from dataclasses import dataclass from web.config import DATA_DIR, CONFIG_DIR, SETTINGS_FILE -from core.system_utils import get_disk_usage, detect_zfs, get_array_direct_path, parse_size_bytes, format_bytes, translate_container_to_host_path, translate_host_to_container_path, remove_from_exclude_file, remove_from_timestamps_file +from core.system_utils import get_disk_usage, detect_zfs, get_array_direct_path, parse_size_bytes, format_bytes, translate_container_to_host_path, translate_host_to_container_path, remove_from_exclude_file, remove_from_timestamps_file, create_dir_with_ownership from core.file_operations import get_media_identity, find_matching_plexcached, save_json_atomically, SUBTITLE_EXTENSIONS, is_video_file @@ -1715,7 +1715,7 @@ def evict_file(self, cache_path: str) -> Dict[str, Any]: # delete the "original" cache file. array_direct_path = get_array_direct_path(array_path) array_direct_dir = os.path.dirname(array_direct_path) - os.makedirs(array_direct_dir, exist_ok=True) + create_dir_with_ownership(array_direct_dir, cache_path) shutil.copy2(cache_path, array_direct_path) # Verify copy succeeded on array if os.path.exists(array_direct_path): @@ -2072,7 +2072,7 @@ def _handle_upgrade_plexcached( if not os.path.isfile(new_plexcached): try: new_array_dir = os.path.dirname(new_array_path) - os.makedirs(new_array_dir, exist_ok=True) + create_dir_with_ownership(new_array_dir, new_cache_path) shutil.copy2(new_cache_path, new_plexcached) src_size = os.path.getsize(new_cache_path) dst_size = os.path.getsize(new_plexcached) diff --git a/web/services/maintenance_service.py b/web/services/maintenance_service.py index 294d2ac7..0241f0ae 100644 --- a/web/services/maintenance_service.py +++ b/web/services/maintenance_service.py @@ -14,7 +14,7 @@ from typing import Callable, Dict, List, Optional, Set, Any, Tuple from web.config import DATA_DIR, CONFIG_DIR, SETTINGS_FILE -from core.system_utils import get_array_direct_path, format_bytes, translate_container_to_host_path, translate_host_to_container_path, remove_from_exclude_file, remove_from_timestamps_file +from core.system_utils import get_array_direct_path, format_bytes, translate_container_to_host_path, translate_host_to_container_path, remove_from_exclude_file, remove_from_timestamps_file, create_dir_with_ownership from core.file_operations import PLEXCACHED_EXTENSION, VIDEO_EXTENSIONS, SUBTITLE_EXTENSIONS, MEDIA_EXTENSIONS @@ -1536,7 +1536,7 @@ def _sync_worker(cache_path: str) -> Tuple[str, bool, Optional[str]]: else: array_dir = os.path.dirname(array_path) - os.makedirs(array_dir, exist_ok=True) + create_dir_with_ownership(array_dir, cache_path) worker_cb = aggregator.make_worker_callback() if aggregator else None self._copy_with_progress(cache_path, array_path, worker_cb) @@ -1625,7 +1625,7 @@ def _sync_worker(cache_path: str) -> Tuple[str, bool, Optional[str]]: else: # No backup/duplicate - copy to array first array_dir = os.path.dirname(array_path) - os.makedirs(array_dir, exist_ok=True) + create_dir_with_ownership(array_dir, cache_path) # Copy file to array with progress self._copy_with_progress(cache_path, array_path, bytes_progress_callback) @@ -1772,7 +1772,7 @@ def _protect_worker(cache_path: str) -> Tuple[str, bool, Optional[str]]: return (cache_path, True, None) array_dir = os.path.dirname(array_path) - os.makedirs(array_dir, exist_ok=True) + create_dir_with_ownership(array_dir, cache_path) cache_size = os.path.getsize(cache_path) if os.path.exists(cache_path) else 0 logging.info(f"Copying {os.path.basename(cache_path)} ({cache_size / (1024**3):.2f} GB) to array...") @@ -1852,7 +1852,7 @@ def _protect_worker(cache_path: str) -> Tuple[str, bool, Optional[str]]: else: # Need to create backup - copy file to array array_dir = os.path.dirname(array_path) - os.makedirs(array_dir, exist_ok=True) + create_dir_with_ownership(array_dir, cache_path) cache_size = os.path.getsize(cache_path) if os.path.exists(cache_path) else 0 logging.info(f"Copying {os.path.basename(cache_path)} ({cache_size / (1024**3):.2f} GB) to array...") @@ -1991,7 +1991,7 @@ def _cache_worker(cache_path: str) -> Tuple[str, bool, Optional[str]]: return (cache_path, False, f"{os.path.basename(cache_path)}: Not found on array") cache_dir = os.path.dirname(cache_path) - os.makedirs(cache_dir, exist_ok=True) + create_dir_with_ownership(cache_dir, source) src_size = os.path.getsize(source) worker_cb = aggregator.make_worker_callback() if aggregator else None @@ -2065,7 +2065,7 @@ def _cache_worker(cache_path: str) -> Tuple[str, bool, Optional[str]]: try: cache_dir = os.path.dirname(cache_path) - os.makedirs(cache_dir, exist_ok=True) + create_dir_with_ownership(cache_dir, source) src_size = os.path.getsize(source) logging.info(f"Caching pinned {os.path.basename(cache_path)} ({src_size / (1024**3):.2f} GB)...")