diff --git a/exca/base.py b/exca/base.py index 51143d80..e6ab7f18 100644 --- a/exca/base.py +++ b/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: diff --git a/exca/cachedict/core.py b/exca/cachedict/core.py index d8ef83f8..292825ff 100644 --- a/exca/cachedict/core.py +++ b/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: diff --git a/exca/cachedict/dumpcontext.py b/exca/cachedict/dumpcontext.py index 8c9f8970..6ea13086 100644 --- a/exca/cachedict/dumpcontext.py +++ b/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) diff --git a/exca/cachedict/registry.py b/exca/cachedict/registry.py index f1fd0fc9..cdc62081 100644 --- a/exca/cachedict/registry.py +++ b/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 diff --git a/exca/cachedict/test_cachedict.py b/exca/cachedict/test_cachedict.py index 09ed65e6..8ff76901 100644 --- a/exca/cachedict/test_cachedict.py +++ b/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" diff --git a/exca/cachedict/test_dumpcontext.py b/exca/cachedict/test_dumpcontext.py index 22157fae..0a1c24fc 100644 --- a/exca/cachedict/test_dumpcontext.py +++ b/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" # ============================================================================= diff --git a/exca/cachedict/test_registry.py b/exca/cachedict/test_registry.py index cf400b59..512ee831 100644 --- a/exca/cachedict/test_registry.py +++ b/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 diff --git a/exca/conftest.py b/exca/conftest.py new file mode 100644 index 00000000..6220439e --- /dev/null +++ b/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) diff --git a/exca/map.py b/exca/map.py index 34f4bf2a..f167d8dd 100644 --- a/exca/map.py +++ b/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: diff --git a/exca/slurm.py b/exca/slurm.py index 3da6b89d..6f573b50 100644 --- a/exca/slurm.py +++ b/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: diff --git a/exca/steps/backends.py b/exca/steps/backends.py index b855e59e..63efd31c 100644 --- a/exca/steps/backends.py +++ b/exca/steps/backends.py @@ -129,6 +129,17 @@ def ensure_folders(self) -> None: """Create necessary directories.""" self.cache_folder.mkdir(parents=True, exist_ok=True) self.job_folder.mkdir(parents=True, exist_ok=True) + # Widen shared parents in case a teammate's earlier run created them + # with stricter umask (the per-item job_folder leaf is fresh so umask + # already covers it). + utils.fix_permissions(self.cache_folder) + utils.fix_permissions(self.job_folder.parent) + # The logs parent is created lazily by submitit at submit time; + # widen on subsequent runs only, when a teammate's earlier run + # would have left it with stricter perms. + logs = self.step_folder / "logs" + if logs.exists(): + utils.fix_permissions(logs) def clear_cache(self) -> None: """Clear cache and job folder for this item.""" @@ -165,6 +176,8 @@ def __init__( self.cache_type = cache_type def __call__(self, *args: tp.Any) -> None: + # Worker entry for Steps: re-apply umask before any folder/file write. + utils.apply_default_umask() self.paths.ensure_folders() cd: exca.cachedict.CacheDict[tp.Any] = exca.cachedict.CacheDict( folder=self.paths.cache_folder, cache_type=self.cache_type @@ -312,6 +325,10 @@ def _check_configs(self, write: bool = True) -> None: step = self._configured_step() folder = self.paths.step_folder folder.mkdir(exist_ok=True, parents=True) + # Widen the user-provided step_folder if pre-existing with stricter perms. + # Done here rather than in ensure_folders so the Backend.job() path + # (which calls _check_configs without ensure_folders) is also covered. + utils.fix_permissions(folder) # Use the full aligned chain as the config (list of steps) # This ensures consistent configs whether written by chain or step diff --git a/exca/task.py b/exca/task.py index c3a49735..7a42a390 100644 --- a/exca/task.py +++ b/exca/task.py @@ -240,7 +240,10 @@ def job_array( else: executor.update_parameters(slurm_array_parallelism=max_workers) executor.folder.mkdir(exist_ok=True, parents=True) - self._set_permissions(executor.folder) + # Widen executor.folder and every shared log-tree ancestor down + # to self.folder; the latter is itself widened by uid_folder. + assert self.folder is not None # validated in SubmititMixin.model_post_init + utils.fix_permissions_up_to(executor.folder, Path(self.folder)) name = self.uid().split("/", maxsplit=1)[0] # select jobs to run statuses: dict[Status, list[TaskInfra]] = collections.defaultdict(list) @@ -326,8 +329,6 @@ def _set_job( with utils.temporary_save_path(job_path) as tmp: with tmp.open("wb") as f: pickle.dump(job, f) - self._set_permissions(job_path) - # dump config self._check_configs(write=True) return job @@ -367,6 +368,8 @@ def job(self) -> submitit.Job[tp.Any] | LocalJob: job._name = self._factory() # for better logging message else: executor.folder.mkdir(exist_ok=True, parents=True) + assert self.folder is not None # validated in SubmititMixin.model_post_init + utils.fix_permissions_up_to(executor.folder, Path(self.folder)) with self._work_env(): job = executor.submit(self._run_method) logger.info( diff --git a/exca/test_map.py b/exca/test_map.py index f0552394..ec77ca3e 100644 --- a/exca/test_map.py +++ b/exca/test_map.py @@ -14,7 +14,7 @@ import pydantic import pytest -from . import helpers +from . import helpers, utils from .map import MapInfra, to_chunks PACKAGE = MapInfra.__module__.split(".", maxsplit=1)[0] @@ -157,15 +157,15 @@ def test_find_slurm_job(tmp_path: Path) -> None: assert job.uid_config == {"param1": 13} -def test_map_infra_perm(tmp_path: Path) -> None: - whatever = Whatever(infra={"folder": tmp_path, "permissions": 0o777}) # type: ignore +def test_map_infra_perm(tmp_path: Path, umask_guard: None) -> None: + utils.set_default_umask(0) # widens new+pre-existing folders to 0o777 + whatever = Whatever(infra={"folder": tmp_path}) # type: ignore xpfold = whatever.infra.uid_folder() assert xpfold is not None xpfold.mkdir(parents=True) - before = xpfold.stat().st_mode + xpfold.chmod(0o700) # simulate a teammate's restrictive pre-existing folder _ = list(whatever.process([1, 2, 2, 3])) - after = xpfold.stat().st_mode - assert after > before + assert oct(xpfold.stat().st_mode)[-3:] == "777" def test_map_infra_debug(tmp_path: Path) -> None: diff --git a/exca/test_task.py b/exca/test_task.py index 51e96b3b..f5f215a8 100644 --- a/exca/test_task.py +++ b/exca/test_task.py @@ -435,17 +435,6 @@ class Whenever(Whatever): # type: ignore _ = whenever.process() -def test_permissions(tmp_path: Path) -> None: - infra = Whatever(infra1={"permissions": "a+rwx"}).infra1 # type: ignore - fp = tmp_path / "test" / "whatever" / "text.txt" - fp.parent.mkdir(parents=True) - fp.touch() - before = fp.stat().st_mode - infra._set_permissions(fp) - after = fp.stat().st_mode - assert after > before - - class D2(pydantic.BaseModel): model_config = pydantic.ConfigDict(extra="forbid") uid: tp.Literal["D2"] = "D2" diff --git a/exca/test_utils.py b/exca/test_utils.py index e9af63c6..5cc8838e 100644 --- a/exca/test_utils.py +++ b/exca/test_utils.py @@ -581,3 +581,106 @@ def test_short_item_uid_idempotent(length: int) -> None: short = ShortItemUid(str, 256) once = short("a" * length) assert short(once) == once + + +# ----------------------------------------------------------------------------- +# umask helpers +# ----------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "raw,expected", + [ + (None, None), + ("0", 0), + ("022", 0o022), + ("0o022", 0o022), + ("22", 0o022), + ("777", 0o777), + ], +) +def test_read_umask_env( + monkeypatch: pytest.MonkeyPatch, raw: str | None, expected: int | None +) -> None: + if raw is None: + monkeypatch.delenv("EXCA_UMASK", raising=False) + else: + monkeypatch.setenv("EXCA_UMASK", raw) + assert utils._read_umask_env() == expected + + +@pytest.mark.parametrize("raw", ["", "abc", "0o", "0o9", "-1", "1000"]) +def test_read_umask_env_invalid(monkeypatch: pytest.MonkeyPatch, raw: str) -> None: + monkeypatch.setenv("EXCA_UMASK", raw) + with pytest.raises(ValueError, match="EXCA_UMASK"): + utils._read_umask_env() + + +def test_helpers_noop_when_unset(tmp_path: Path, umask_guard: None) -> None: + """All helpers share the ``_DEFAULT_UMASK is None`` early return.""" + utils.set_default_umask(None) + fp = tmp_path / "f.txt" + fp.write_text("x") + fp.chmod(0o600) + sentinel = 0o123 + os.umask(sentinel) + utils.apply_default_umask() + utils.fix_permissions(fp) + utils.fix_permissions_up_to(fp, tmp_path) + assert os.umask(sentinel) == sentinel + assert oct(fp.stat().st_mode)[-3:] == "600" + + +def test_fix_permissions( + tmp_path: Path, umask_guard: None, caplog: pytest.LogCaptureFixture +) -> None: + """Recursive widen splits dir/file modes; missing path warns rather than raises.""" + utils.set_default_umask(0o022) + sub = tmp_path / "sub" / "deep" + sub.mkdir(parents=True) + fp = sub / "f.txt" + fp.write_text("x") + for p in (tmp_path / "sub", sub, fp): + p.chmod(0o600 if p.is_file() else 0o700) + utils.fix_permissions(tmp_path / "sub", recursive=True) + for d in (tmp_path / "sub", sub): + assert oct(d.stat().st_mode)[-3:] == "755" + assert oct(fp.stat().st_mode)[-3:] == "644" + with caplog.at_level("WARNING", logger="exca.utils"): + utils.fix_permissions(tmp_path / "missing") + assert any("Failed to widen" in r.message for r in caplog.records) + + +def test_fix_permissions_up_to(tmp_path: Path, umask_guard: None) -> None: + """Widen the chain leaf→root (root excluded); bail when leaf is not under root.""" + utils.set_default_umask(0o022) + leaf = tmp_path / "logs" / "alice" / "%j" + leaf.mkdir(parents=True) + chain = [tmp_path / "logs", tmp_path / "logs" / "alice", leaf] + for d in [*chain, tmp_path]: + d.chmod(0o700) + utils.fix_permissions_up_to(leaf, tmp_path) + for d in chain: + assert oct(d.stat().st_mode)[-3:] == "755" + assert oct(tmp_path.stat().st_mode)[-3:] == "700" # root excluded + other = tmp_path / "other" + other.mkdir() + other.chmod(0o700) + utils.fix_permissions_up_to(other, tmp_path / "missing") + assert oct(other.stat().st_mode)[-3:] == "700" + + +def test_fix_permissions_up_to_normalizes_relative( + tmp_path: Path, umask_guard: None, monkeypatch: pytest.MonkeyPatch +) -> None: + """Submitit absolute-resolves executor.folder; helper must do the same so a + relative ``self.folder`` root doesn't silently bypass the chain check.""" + utils.set_default_umask(0o022) + monkeypatch.chdir(tmp_path) + leaf = Path("logs") / "deep" + leaf.mkdir(parents=True) + for d in (tmp_path / "logs", leaf): + d.chmod(0o700) + utils.fix_permissions_up_to(leaf, tmp_path) + for d in (tmp_path / "logs", tmp_path / "logs" / "deep"): + assert oct(d.stat().st_mode)[-3:] == "755" diff --git a/exca/utils.py b/exca/utils.py index 090e1b4e..f8ad51ee 100644 --- a/exca/utils.py +++ b/exca/utils.py @@ -32,6 +32,116 @@ DISCRIMINATOR_FIELD = "#infra#pydantic#discriminator" T = tp.TypeVar("T", bound=pydantic.BaseModel) +# ----------------------------------------------------------------------------- +# Permissions / umask +# +# exca uses a process-wide umask (configured via the EXCA_UMASK env var) to +# control file modes for cached artefacts. Helpers below install/re-apply the +# default and widen pre-existing folders that umask cannot reach. +# +# Default is unset: the process inherits its shell umask and fix_permissions is +# a no-op. Set EXCA_UMASK (e.g. "0o002") to opt into shared-cache modes. +# ----------------------------------------------------------------------------- + +# read by apply_default_umask / fix_permissions; written only by set_default_umask +_DEFAULT_UMASK: int | None = None + + +def _read_umask_env() -> int | None: + """Parse ``EXCA_UMASK`` as an octal mask, matching the ``umask`` shell + command convention. Accepts ``"022"``, ``"0o022"``, ``"22"`` (all 0o022). + Returns None only when ``EXCA_UMASK`` is not set in the environment + (caller inherits shell umask). Empty string, non-octal input, and + out-of-range values (must be in ``0..0o777``) are rejected loudly — + typically a missed quote or shell-script accident.""" + raw = os.environ.get("EXCA_UMASK") + if raw is None: + return None + s = raw.removeprefix("0o").removeprefix("0O") + msg = f"EXCA_UMASK={raw!r} must be an octal mask in 0..0o777 (e.g. '0o022', '022', or '22')" + try: + val = int(s, 8) + except ValueError as e: + raise ValueError(msg) from e + if not 0 <= val <= 0o777: + raise ValueError(msg) + return val + + +def set_default_umask(value: int | None) -> None: + """Install *value* as the process umask, and cache it for later + re-application via :func:`apply_default_umask`. Pass ``None`` to + leave the shell umask alone.""" + global _DEFAULT_UMASK + _DEFAULT_UMASK = value + if value is not None: + os.umask(value) + + +def apply_default_umask() -> None: + """Re-install the cached umask. Called at worker entry points so a + fresh process re-applies after exca's import-time install (in case + unrelated code reset the umask between import and dispatch).""" + if _DEFAULT_UMASK is not None: + os.umask(_DEFAULT_UMASK) + + +def fix_permissions(path: Path | str, recursive: bool = False) -> None: + """Widen *path* to the mode the configured umask permits. + + No-op when ``EXCA_UMASK`` is unset (user opted out of exca managing + modes). Use only where umask cannot reach: pre-existing folders, + mode-preserving copies (e.g. ``shutil.copytree``). Caller is + responsible for *path* existing — missing-path failures are logged + as warnings (likely a programming error: caller skipped a mkdir or + reordered calls). + """ + # read the cached value rather than os.umask(0)+restore: the peek + # races against threadpool workers writing through CacheDict + if _DEFAULT_UMASK is None: + return + dir_mode = 0o777 & ~_DEFAULT_UMASK + file_mode = 0o666 & ~_DEFAULT_UMASK + p = Path(path) + try: + p.chmod(dir_mode if p.is_dir() else file_mode) + except Exception: + logger.warning("Failed to widen permissions on %s", p, exc_info=True) + return + if recursive and p.is_dir(): + for child in p.rglob("*"): + try: + child.chmod(dir_mode if child.is_dir() else file_mode) + except Exception: + logger.debug("Failed to widen %s", child, exc_info=True) + + +def fix_permissions_up_to(leaf: Path | str, root: Path | str) -> None: + """Widen *leaf* and every ancestor up to (but not including) *root*. + + Used to widen log/cache parent chains created via ``mkdir(parents=True)``, + where intermediate dirs may pre-exist with stricter perms from a + teammate's earlier run. No-op when ``EXCA_UMASK`` is unset, or when + *leaf* is not under *root*. Both arguments are normalised to absolute + paths so callers can pass user-provided relative roots safely. + """ + if _DEFAULT_UMASK is None: + return + # submitit resolves executor.folder to absolute via expanduser/absolute; + # mirror that here so a relative self.folder doesn't silently bypass. + leaf_p = Path(leaf).expanduser().absolute() + root_p = Path(root).expanduser().absolute() + if leaf_p != root_p and root_p not in leaf_p.parents: + return + p = leaf_p + while p != root_p: + fix_permissions(p) + p = p.parent + + +# install at module import: covers controller-side writes +set_default_umask(_read_umask_env()) + def _get_uid_info( model: pydantic.BaseModel, ignore_discriminator: bool = False diff --git a/exca/workdir.py b/exca/workdir.py index 0d53e946..c01abf65 100644 --- a/exca/workdir.py +++ b/exca/workdir.py @@ -18,6 +18,8 @@ import pydantic import yaml as _yaml +from . import utils + logger = logging.getLogger(__name__) @@ -150,6 +152,8 @@ def activate(self) -> tp.Iterator[None]: else: out.parent.mkdir(exist_ok=True, parents=True) shutil.copyfile(path, out, follow_symlinks=True) + # copytree/copyfile preserve source mode regardless of umask + utils.fix_permissions(out, recursive=True) logger.info("Copied %s to %s", path, out) if self._commits: string: str = _yaml.safe_dump(self._commits)