[WIP] Update permission management

exca/base.py

@@ -128,10 +128,6 @@ def model_with_infra_validator_before(obj: tp.Any) -> tp.Any:
 
 class BaseInfra(pydantic.BaseModel):
     folder: Path | str | None = None
-    # general permission for folders and files
-    # use os.chmod / path.chmod compatible numbers, or None to deactivate
-    # eg: 0o777 for all rights to all users
-    permissions: int | str | None = 0o777
     # {folder} will be replaced by the class folder
     # {user} by user id and %j by job id
     logs: Path | str = "{folder}/logs/{user}/%j"
@@ -195,8 +191,9 @@ def _exclude_from_cls_uid(self) -> list[str]:
         return list(set(type(self).model_fields) - {"version"})
 
     def model_post_init(self, log__: tp.Any) -> None:
+        # required: pydantic's auto-generated init_private_attributes hook
+        # would otherwise replace this and skip SubmititMixin's validators
         super().model_post_init(log__)
-        self._set_permissions(None)  # set compatibility for permissions as string
 
     def config(self, uid: bool = True, exclude_defaults: bool = False) -> ConfDict:
         """Exports the task configuration as a ConfigDict
@@ -245,15 +242,6 @@ def _check_configs(self, write: bool = True) -> None:
         )
         dump.check_and_write(xpfolder, write=write)
         state.checked_configs = True
-        # Set permissions on written files
-        if write:
-            for name in ("uid", "full-uid", "config"):
-                fp = xpfolder / f"{name}.yaml"
-                if fp.exists():
-                    try:
-                        self._set_permissions(fp)
-                    except (OSError, FileNotFoundError):
-                        pass
 
     def _factory(self) -> str:
         state = _fast_state(self)
@@ -342,8 +330,11 @@ def uid_folder(self, create: bool = False) -> Path | None:
         if not create:
             return folder
         folder.mkdir(exist_ok=True, parents=True)
-        self._set_permissions(self.folder)
-        self._set_permissions(folder)
+        # Widen pre-existing folders that umask cannot reach: the user-provided
+        # cache root, and the uid subfolder when a teammate or aborted run
+        # already created it.
+        utils.fix_permissions(self.folder)
+        utils.fix_permissions(folder)
         return folder
 
     def iter_cached(self) -> tp.Iterable[pydantic.BaseModel]:
@@ -359,23 +350,6 @@ def iter_cached(self) -> tp.Iterable[pydantic.BaseModel]:
             cfg = ConfDict.from_yaml(fp)
             yield cls(**cfg)
 
-    def _set_permissions(self, path: str | Path | None) -> None:
-        if isinstance(self.permissions, str):
-            if not self.permissions:
-                self.permissions = None
-            elif self.permissions == "a+rwx":
-                self.permissions = 0o777
-            else:
-                raise ValueError(f"No compatibility for permissions {self.permissions}")
-            msg = "infra.permissions set to %s by compatibility mode"
-            logger.warning(msg, self.permissions)
-        if path is not None and self.permissions is not None:
-            try:
-                Path(path).chmod(self.permissions)
-            except Exception as e:
-                msg = f"Failed to set permission to {self.permissions} on '{path}'\n({e})"
-                logger.warning(msg)
-
     def clone_obj(self, *args: dict[str, tp.Any], **kwargs: tp.Any) -> tp.Any:
         """Create a new decorated object by applying a diff config to the underlying object"""
         if args:

exca/cachedict/core.py

@@ -86,10 +86,6 @@ class CacheDict(tp.Generic[X]):
         If `None`, the type will be deduced automatically (Json for JSON-serializable values,
         or a type-specific handler for numpy arrays, tensors, etc.).
         Loading is handled using the cache_type specified in info files.
-    permissions: optional int
-        permissions for generated files
-        use os.chmod / path.chmod compatible numbers, or None to deactivate
-        eg: 0o777 for all rights to all users
 
     Usage
     -----
@@ -119,22 +115,14 @@ def __init__(
         folder: Path | str | None,
         keep_in_ram: bool = False,
         cache_type: None | str = None,
-        permissions: int | None = 0o777,
     ) -> None:
         self.folder = None if folder is None else Path(folder)
-        self.permissions = permissions
         self.cache_type = cache_type
         self._keep_in_ram = keep_in_ram
         if self.folder is None and not keep_in_ram:
             raise ValueError("At least folder or keep_in_ram should be activated")
         if self.folder is not None:
             self.folder.mkdir(exist_ok=True)
-            if self.permissions is not None:
-                try:
-                    self.folder.chmod(self.permissions)
-                except Exception as e:
-                    msg = f"Failed to set permission to {self.permissions} on {self.folder}\n({e})"
-                    logger.warning(msg)
         # file cache access and RAM cache
         self._ram_data: dict[str, X] = {}
         self._key_info: dict[str, DumpInfo] = {}
@@ -145,7 +133,7 @@ def __init__(
         # DumpContext for this folder (load/delete; writes use per-thread _write_ctx)
         self._dumper: DumpContext | None = None
         if self.folder is not None:
-            self._dumper = DumpContext(self.folder, permissions=self.permissions)
+            self._dumper = DumpContext(self.folder)
         self._local = threading.local()  # per-thread write context, see _write_ctx
 
     def __repr__(self) -> str:
@@ -290,7 +278,7 @@ def write(self) -> tp.Iterator["CacheDict[X]"]:
         if self._write_ctx is not None:
             raise RuntimeError("Cannot re-open an already open writer")
         if self.folder is not None:
-            self._write_ctx = DumpContext(self.folder, permissions=self.permissions)
+            self._write_ctx = DumpContext(self.folder)
         try:
             if self._write_ctx is not None:
                 with self._write_ctx:

exca/cachedict/dumpcontext.py

@@ -114,13 +114,10 @@ class DumpContext:
     DATA_DIR = "data"
     INFO_SUFFIX = "-info.jsonl"
 
-    def __init__(
-        self, folder: str | Path, *, key: str = "", permissions: int | None = None
-    ) -> None:
+    def __init__(self, folder: str | Path, *, key: str = "") -> None:
         self.folder = Path(folder)
         self.key = key
         self.level: int = -1
-        self.permissions = permissions
         self.options = DumpOptions()
         # write state
         self._thread_id = threading.get_native_id()
@@ -200,15 +197,6 @@ def __enter__(self) -> tp.Self:
         return self
 
     def __exit__(self, *exc: tp.Any) -> None:
-        if self.permissions is not None:
-            for fp in self._created_files:
-                try:
-                    fp.chmod(self.permissions)
-                    if fp.is_dir():
-                        for child in fp.rglob("*"):
-                            child.chmod(self.permissions)
-                except Exception:
-                    logger.warning("Failed to set permissions on %s", fp, exc_info=True)
         if self._stack is None:
             raise RuntimeError("DumpContext.__exit__ called without __enter__")
         try:
@@ -218,7 +206,8 @@ def __exit__(self, *exc: tp.Any) -> None:
             self._created_files.clear()
 
     def _ensure_parent(self, path: Path) -> None:
-        """Create parent directories and track them for permission setting."""
+        """Create the parent directory and track it so :meth:`key_path` can
+        detect same-context filename collisions via ``_created_files``."""
         parent = path.parent
         if parent != self.folder and not parent.exists():
             parent.mkdir(parents=True, exist_ok=True)

exca/cachedict/registry.py

@@ -71,9 +71,8 @@ class AdvisoryRegistry:
     _SCHEMA: tp.ClassVar[str]  # passed to executescript(), multi-statement OK
     _LABEL: tp.ClassVar[str]  # short prefix in log messages
 
-    def __init__(self, folder: Path | str, permissions: int | None = 0o777) -> None:
+    def __init__(self, folder: Path | str) -> None:
         self.db_path = Path(folder) / self._DB_NAME
-        self.permissions = permissions
         self._conn: sqlite3.Connection | None = None
 
     def _connect(self) -> sqlite3.Connection:
@@ -98,15 +97,6 @@ def _connect(self) -> sqlite3.Connection:
         )
         conn.execute("PRAGMA journal_mode=DELETE")
         conn.executescript(self._SCHEMA)
-        if self.permissions is not None:
-            try:
-                self.db_path.chmod(self.permissions)
-            except Exception:
-                logger.warning(
-                    "Failed to set permissions on %s",
-                    self.db_path,
-                    exc_info=True,
-                )
         self._conn = conn
         return conn
 

exca/cachedict/test_cachedict.py

@@ -105,8 +105,13 @@ def test_data_dump_suffix(tmp_path: Path, data: tp.Any) -> None:
 )
 @pytest.mark.parametrize("keep_in_ram", (True, False))
 def test_specialized_dump(
-    tmp_path: Path, data: tp.Any, cache_type: str, keep_in_ram: bool
+    tmp_path: Path,
+    data: tp.Any,
+    cache_type: str,
+    keep_in_ram: bool,
+    umask_guard: None,
 ) -> None:
+    utils.set_default_umask(0)  # writes get 0o777 on dirs, 0o666 on files
     memmap_cache_size = 10
     if cache_type.endswith(":0"):
         cache_type = cache_type[:-2]
@@ -133,12 +138,12 @@ def test_specialized_dump(
         assert files, "Some memmaps should stay open"
     del cache
     gc.collect()
-    # check permissions
-    octal_permissions = oct(tmp_path.stat().st_mode)[-3:]
-    assert octal_permissions == "777", f"Wrong permissions for {tmp_path}"
+    # umask 0 ⇒ dirs get 0o777, files get 0o666 (no +x). tmp_path itself was
+    # created by pytest before the umask install, so we don't assert its mode.
     for fp in tmp_path.rglob("*"):
-        octal_permissions = oct(fp.stat().st_mode)[-3:]
-        assert octal_permissions == "777", f"Wrong permissions for {fp}"
+        expected = "777" if fp.is_dir() else "666"
+        actual = oct(fp.stat().st_mode)[-3:]
+        assert actual == expected, f"Wrong permissions for {fp}: {actual}"
     # after del, all files should be closed
     files = proc.open_files()
     assert not files, "No file should remain open after del cache"

exca/cachedict/test_dumpcontext.py

@@ -13,6 +13,8 @@
 import numpy as np
 import pytest
 
+from exca import utils
+
 from .dumpcontext import DumpContext
 
 # =============================================================================
@@ -120,13 +122,16 @@ def test_shared_file_lifecycle(tmp_path: Path) -> None:
     assert (tmp_path / name1).read_bytes() == b"hello"
 
 
-def test_context_permissions(tmp_path: Path) -> None:
-    ctx = DumpContext(tmp_path, permissions=0o755)
+def test_context_permissions(tmp_path: Path, umask_guard: None) -> None:
+    utils.set_default_umask(0o022)  # dirs get 0o755, files get 0o644
+    ctx = DumpContext(tmp_path)
     with ctx:
         f, name = ctx.shared_file(".data")
         f.write(b"test")
-    mode = oct((tmp_path / name).stat().st_mode)[-3:]
-    assert mode == "755"
+    file_mode = oct((tmp_path / name).stat().st_mode)[-3:]
+    dir_mode = oct((tmp_path / DumpContext.DATA_DIR).stat().st_mode)[-3:]
+    assert file_mode == "644"
+    assert dir_mode == "755"
 
 
 # =============================================================================

exca/cachedict/test_registry.py

@@ -14,6 +14,7 @@
 
 import pytest
 
+from exca import utils
 from exca.cachedict import registry
 from exca.steps import errors
 
@@ -186,8 +187,10 @@ def test_graceful_degradation(
     reg2.close()
 
 
-def test_permissions_applied(tmp_path: Path) -> None:
-    reg = errors.ErrorRegistry(tmp_path, permissions=0o600)
+def test_permissions_applied(tmp_path: Path, umask_guard: None) -> None:
+    # files get 0o600 on creation: open(0o666) & ~0o066 = 0o600
+    utils.set_default_umask(0o066)
+    reg = errors.ErrorRegistry(tmp_path)
     reg.record(["a"])
     mode = stat.S_IMODE((tmp_path / "errors.db").stat().st_mode)
     assert mode == 0o600

exca/conftest.py

@@ -0,0 +1,30 @@
+# Copyright (c) Meta Platforms, Inc. and affiliates.
+# All rights reserved.
+#
+# This source code is licensed under the license found in the
+# LICENSE file in the root directory of this source tree.
+
+"""Shared fixtures for the exca test suite."""
+
+import os
+import typing as tp
+
+import pytest
+
+from exca import utils
+
+
+@pytest.fixture
+def umask_guard() -> tp.Iterator[None]:
+    """Save and restore process umask + cached default around a test.
+
+    Used by tests that call :func:`utils.set_default_umask` so they don't
+    leak the value into other tests."""
+    prev_default = utils._DEFAULT_UMASK
+    prev_umask = os.umask(0)
+    os.umask(prev_umask)
+    try:
+        yield
+    finally:
+        utils.set_default_umask(prev_default)
+        os.umask(prev_umask)

exca/map.py

@@ -18,7 +18,7 @@
 import numpy as np
 import pydantic
 
-from . import base, slurm
+from . import base, slurm, utils
 from .cachedict import CacheDict, inflight
 from .utils import ShortItemUid
 
@@ -194,10 +194,6 @@ def cache_dict(self) -> CacheDict[tp.Any]:
             raise RuntimeError(f"Infra was not applied: {self!r}")
         cache_type = imethod.cache_type
         cache_path = self.uid_folder(create=True)
-        if isinstance(self.permissions, str):
-            self._set_permissions(None)
-        if isinstance(self.permissions, str):
-            raise RuntimeError("infra.permissions should have been an integer")
         cd: CacheDict[tp.Any] = CacheDict(
             folder=cache_path,
             keep_in_ram=self.keep_in_ram,
@@ -211,8 +207,7 @@ def _inflight_registry(self) -> inflight.InflightRegistry | None:
         cache_folder = self.uid_folder()
         if cache_folder is None:
             return None
-        perm = self.permissions if isinstance(self.permissions, int) else None
-        return inflight.InflightRegistry(cache_folder, permissions=perm)
+        return inflight.InflightRegistry(cache_folder)
 
     # pylint: disable=unused-argument
     def apply(
@@ -510,6 +505,9 @@ def _method_override_futures(self, items: tp.Sequence[tp.Any]) -> tp.Iterator[tp
     def _call_and_store(
         self, items: tp.Sequence[tp.Any], use_cache_dict: bool = True
     ) -> dict[str, tp.Any]:
+        # Worker entry for MapInfra: re-apply umask before cache_dict mkdirs the
+        # uid folder. _run_method's own apply_default_umask runs too late here.
+        utils.apply_default_umask()
         d: dict[str, tp.Any] = self.cache_dict if use_cache_dict else {}  # type: ignore
         imethod = self._infra_method
         if imethod is None:

exca/slurm.py

@@ -20,7 +20,7 @@
 import submitit
 from submitit.core import utils as submitit_utils
 
-from . import base
+from . import base, utils
 from .workdir import WorkDir
 
 submitit.Job._results_timeout_s = 4  # avoid too long a wait
@@ -237,6 +237,9 @@ def _work_env(self) -> tp.Iterator[None]:
                     submitit_utils.cloudpickle_dump = base_dump
 
     def _run_method(self, *args: tp.Any, **kwargs: tp.Any) -> tp.Any:
+        # Worker entry for TaskInfra: re-apply umask after exca import in case
+        # unrelated code reset it between import and dispatch.
+        utils.apply_default_umask()
         if not isinstance(self, base.BaseInfra):
             raise RuntimeError("This can only run on BaseInfra subclasses")
         if self.workdir is not None: