From b086dea6d9c50c078c1715b795212eb5e529b720 Mon Sep 17 00:00:00 2001 From: Jeremy Rapin Date: Wed, 29 Apr 2026 22:07:50 +0200 Subject: [PATCH 1/3] [WIP] Update permission management --- exca/base.py | 42 ++------ exca/cachedict/core.py | 16 +-- exca/cachedict/dumpcontext.py | 17 +-- exca/cachedict/registry.py | 12 +-- exca/cachedict/test_cachedict.py | 17 +-- exca/cachedict/test_dumpcontext.py | 13 ++- exca/cachedict/test_registry.py | 7 +- exca/map.py | 12 +-- exca/slurm.py | 5 +- exca/steps/backends.py | 17 +++ exca/task.py | 9 +- exca/test_map.py | 12 +-- exca/test_task.py | 11 -- exca/test_utils.py | 161 +++++++++++++++++++++++++++++ exca/utils.py | 111 ++++++++++++++++++++ exca/workdir.py | 4 + 16 files changed, 354 insertions(+), 112 deletions(-) diff --git a/exca/base.py b/exca/base.py index 51143d80..db0dfed8 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,11 @@ 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: BaseInfra has private attrs, so pydantic installs + # `init_private_attributes` as model_post_init unless we define one. + # That bare function does not chain to super, which would break + # SubmititMixin's folder/cluster validation in MapInfra/TaskInfra MRO. 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 +244,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 +332,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 +352,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/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..11526bd6 100644 --- a/exca/test_utils.py +++ b/exca/test_utils.py @@ -581,3 +581,164 @@ 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", + [ + ("0", 0), + ("002", 0o002), + ("022", 0o022), + ("0o022", 0o022), + ("0O022", 0o022), + ("22", 0o022), + ("777", 0o777), + ], +) +def test_read_umask_env(monkeypatch: pytest.MonkeyPatch, raw: str, expected: int) -> None: + monkeypatch.setenv("EXCA_UMASK", raw) + assert utils._read_umask_env() == expected + + +def test_read_umask_env_unset(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("EXCA_UMASK", raising=False) + assert utils._read_umask_env() is None + + +@pytest.mark.parametrize( + "raw", + [ + "", # empty string + "abc", # not numeric + "0o9", # invalid octal digit + "0o", # prefix only + "9", # invalid octal digit + "0xff", # hex + "-1", # negative (out of range) + "1000", # 0o1000 = 512, above 0o777 + ], +) +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_apply_default_umask_noop_when_unset(umask_guard: None) -> None: + """When _DEFAULT_UMASK is None, apply_default_umask must not touch os.umask.""" + utils.set_default_umask(None) + before = os.umask(0o123) + os.umask(before) + utils.apply_default_umask() + assert os.umask(0) == before + os.umask(before) + + +def test_fix_permissions_noop_when_unset(tmp_path: Path, umask_guard: None) -> None: + """fix_permissions is a no-op when EXCA_UMASK was not set.""" + utils.set_default_umask(None) + fp = tmp_path / "f.txt" + fp.write_text("x") + fp.chmod(0o600) + utils.fix_permissions(fp) + assert oct(fp.stat().st_mode)[-3:] == "600" + + +def test_fix_permissions_warns_on_missing( + tmp_path: Path, umask_guard: None, caplog: pytest.LogCaptureFixture +) -> None: + """A missing path is a programming error: log a warning, don't raise.""" + utils.set_default_umask(0o022) + with caplog.at_level("WARNING", logger="exca.utils"): + utils.fix_permissions(tmp_path / "does-not-exist") + assert any("Failed to widen permissions" in r.message for r in caplog.records) + + +def test_fix_permissions_recursive(tmp_path: Path, umask_guard: None) -> None: + """Recursive widen distinguishes dir mode (0o755) from file mode (0o644).""" + utils.set_default_umask(0o022) + sub = tmp_path / "sub" / "deep" + sub.mkdir(parents=True) + fp = sub / "f.txt" + fp.write_text("x") + # simulate a teammate's restrictive pre-existing tree + fp.chmod(0o600) + sub.chmod(0o700) + (tmp_path / "sub").chmod(0o700) + utils.fix_permissions(tmp_path / "sub", recursive=True) + assert oct((tmp_path / "sub").stat().st_mode)[-3:] == "755" + assert oct(sub.stat().st_mode)[-3:] == "755" + assert oct(fp.stat().st_mode)[-3:] == "644" + + +def test_fix_permissions_up_to(tmp_path: Path, umask_guard: None) -> None: + """Widen leaf and every ancestor up to (not including) root.""" + utils.set_default_umask(0o022) + leaf = tmp_path / "logs" / "alice" / "%j" + leaf.mkdir(parents=True) + # restrict every dir between tmp_path and leaf, plus tmp_path itself + for d in (tmp_path / "logs", tmp_path / "logs" / "alice", leaf): + d.chmod(0o700) + tmp_path.chmod(0o700) + utils.fix_permissions_up_to(leaf, tmp_path) + assert oct(leaf.stat().st_mode)[-3:] == "755" + assert oct((tmp_path / "logs" / "alice").stat().st_mode)[-3:] == "755" + assert oct((tmp_path / "logs").stat().st_mode)[-3:] == "755" + # root itself is the boundary — must NOT be widened + assert oct(tmp_path.stat().st_mode)[-3:] == "700" + + +def test_fix_permissions_up_to_bails_when_not_under_root( + tmp_path: Path, umask_guard: None +) -> None: + """If leaf is not under root, no walk happens (no filesystem-root climb).""" + utils.set_default_umask(0o022) + 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_leaf_equals_root( + tmp_path: Path, umask_guard: None +) -> None: + """When leaf == root the walk is empty: nothing widened, including root.""" + utils.set_default_umask(0o022) + tmp_path.chmod(0o700) + utils.fix_permissions_up_to(tmp_path, tmp_path) + assert oct(tmp_path.stat().st_mode)[-3:] == "700" + + +def test_fix_permissions_up_to_noop_when_unset(tmp_path: Path, umask_guard: None) -> None: + """When EXCA_UMASK was not set, the helper is a pure no-op.""" + utils.set_default_umask(None) + leaf = tmp_path / "a" / "b" + leaf.mkdir(parents=True) + for d in (tmp_path / "a", leaf): + d.chmod(0o700) + utils.fix_permissions_up_to(leaf, tmp_path) + assert oct((tmp_path / "a").stat().st_mode)[-3:] == "700" + assert oct(leaf.stat().st_mode)[-3:] == "700" + + +def test_fix_permissions_up_to_normalizes_relative( + tmp_path: Path, umask_guard: None, monkeypatch: pytest.MonkeyPatch +) -> None: + """Relative leaf/root must be normalised — submitit absolute-resolves + its folder, so the chain check would otherwise silently bail.""" + utils.set_default_umask(0o022) + monkeypatch.chdir(tmp_path) + leaf = Path("logs") / "deep" + leaf.mkdir(parents=True) + (tmp_path / "logs").chmod(0o700) + leaf.chmod(0o700) + # leaf is relative, root is absolute (the TaskInfra failure shape) + utils.fix_permissions_up_to(leaf, tmp_path) + assert oct((tmp_path / "logs").stat().st_mode)[-3:] == "755" + assert oct((tmp_path / "logs" / "deep").stat().st_mode)[-3:] == "755" diff --git a/exca/utils.py b/exca/utils.py index 090e1b4e..4da31033 100644 --- a/exca/utils.py +++ b/exca/utils.py @@ -32,6 +32,117 @@ 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). + + Reads the cached ``_DEFAULT_UMASK`` directly rather than peeking via + ``os.umask(0)`` to avoid a process-global race in threaded code. + """ + 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) From 1b6684a123f55ea84bc3baca990e78837eeda015 Mon Sep 17 00:00:00 2001 From: Jeremy Rapin Date: Wed, 29 Apr 2026 22:39:08 +0200 Subject: [PATCH 2/3] fix --- exca/conftest.py | 30 +++++++++++ exca/test_utils.py | 132 +++++++++++++-------------------------------- exca/utils.py | 5 +- 3 files changed, 69 insertions(+), 98 deletions(-) create mode 100644 exca/conftest.py 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/test_utils.py b/exca/test_utils.py index 11526bd6..5cc8838e 100644 --- a/exca/test_utils.py +++ b/exca/test_utils.py @@ -591,113 +591,78 @@ def test_short_item_uid_idempotent(length: int) -> None: @pytest.mark.parametrize( "raw,expected", [ + (None, None), ("0", 0), - ("002", 0o002), ("022", 0o022), ("0o022", 0o022), - ("0O022", 0o022), ("22", 0o022), ("777", 0o777), ], ) -def test_read_umask_env(monkeypatch: pytest.MonkeyPatch, raw: str, expected: int) -> None: - monkeypatch.setenv("EXCA_UMASK", raw) +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 -def test_read_umask_env_unset(monkeypatch: pytest.MonkeyPatch) -> None: - monkeypatch.delenv("EXCA_UMASK", raising=False) - assert utils._read_umask_env() is None - - -@pytest.mark.parametrize( - "raw", - [ - "", # empty string - "abc", # not numeric - "0o9", # invalid octal digit - "0o", # prefix only - "9", # invalid octal digit - "0xff", # hex - "-1", # negative (out of range) - "1000", # 0o1000 = 512, above 0o777 - ], -) +@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_apply_default_umask_noop_when_unset(umask_guard: None) -> None: - """When _DEFAULT_UMASK is None, apply_default_umask must not touch os.umask.""" - utils.set_default_umask(None) - before = os.umask(0o123) - os.umask(before) - utils.apply_default_umask() - assert os.umask(0) == before - os.umask(before) - - -def test_fix_permissions_noop_when_unset(tmp_path: Path, umask_guard: None) -> None: - """fix_permissions is a no-op when EXCA_UMASK was not set.""" +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_warns_on_missing( +def test_fix_permissions( tmp_path: Path, umask_guard: None, caplog: pytest.LogCaptureFixture ) -> None: - """A missing path is a programming error: log a warning, don't raise.""" - utils.set_default_umask(0o022) - with caplog.at_level("WARNING", logger="exca.utils"): - utils.fix_permissions(tmp_path / "does-not-exist") - assert any("Failed to widen permissions" in r.message for r in caplog.records) - - -def test_fix_permissions_recursive(tmp_path: Path, umask_guard: None) -> None: - """Recursive widen distinguishes dir mode (0o755) from file mode (0o644).""" + """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") - # simulate a teammate's restrictive pre-existing tree - fp.chmod(0o600) - sub.chmod(0o700) - (tmp_path / "sub").chmod(0o700) + 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) - assert oct((tmp_path / "sub").stat().st_mode)[-3:] == "755" - assert oct(sub.stat().st_mode)[-3:] == "755" + 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 leaf and every ancestor up to (not including) root.""" + """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) - # restrict every dir between tmp_path and leaf, plus tmp_path itself - for d in (tmp_path / "logs", tmp_path / "logs" / "alice", leaf): + chain = [tmp_path / "logs", tmp_path / "logs" / "alice", leaf] + for d in [*chain, tmp_path]: d.chmod(0o700) - tmp_path.chmod(0o700) utils.fix_permissions_up_to(leaf, tmp_path) - assert oct(leaf.stat().st_mode)[-3:] == "755" - assert oct((tmp_path / "logs" / "alice").stat().st_mode)[-3:] == "755" - assert oct((tmp_path / "logs").stat().st_mode)[-3:] == "755" - # root itself is the boundary — must NOT be widened - assert oct(tmp_path.stat().st_mode)[-3:] == "700" - - -def test_fix_permissions_up_to_bails_when_not_under_root( - tmp_path: Path, umask_guard: None -) -> None: - """If leaf is not under root, no walk happens (no filesystem-root climb).""" - utils.set_default_umask(0o022) + 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) @@ -705,40 +670,17 @@ def test_fix_permissions_up_to_bails_when_not_under_root( assert oct(other.stat().st_mode)[-3:] == "700" -def test_fix_permissions_up_to_leaf_equals_root( - tmp_path: Path, umask_guard: None -) -> None: - """When leaf == root the walk is empty: nothing widened, including root.""" - utils.set_default_umask(0o022) - tmp_path.chmod(0o700) - utils.fix_permissions_up_to(tmp_path, tmp_path) - assert oct(tmp_path.stat().st_mode)[-3:] == "700" - - -def test_fix_permissions_up_to_noop_when_unset(tmp_path: Path, umask_guard: None) -> None: - """When EXCA_UMASK was not set, the helper is a pure no-op.""" - utils.set_default_umask(None) - leaf = tmp_path / "a" / "b" - leaf.mkdir(parents=True) - for d in (tmp_path / "a", leaf): - d.chmod(0o700) - utils.fix_permissions_up_to(leaf, tmp_path) - assert oct((tmp_path / "a").stat().st_mode)[-3:] == "700" - assert oct(leaf.stat().st_mode)[-3:] == "700" - - def test_fix_permissions_up_to_normalizes_relative( tmp_path: Path, umask_guard: None, monkeypatch: pytest.MonkeyPatch ) -> None: - """Relative leaf/root must be normalised — submitit absolute-resolves - its folder, so the chain check would otherwise silently bail.""" + """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) - (tmp_path / "logs").chmod(0o700) - leaf.chmod(0o700) - # leaf is relative, root is absolute (the TaskInfra failure shape) + for d in (tmp_path / "logs", leaf): + d.chmod(0o700) utils.fix_permissions_up_to(leaf, tmp_path) - assert oct((tmp_path / "logs").stat().st_mode)[-3:] == "755" - assert oct((tmp_path / "logs" / "deep").stat().st_mode)[-3:] == "755" + 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 4da31033..f8ad51ee 100644 --- a/exca/utils.py +++ b/exca/utils.py @@ -95,10 +95,9 @@ def fix_permissions(path: Path | str, recursive: bool = False) -> None: responsible for *path* existing — missing-path failures are logged as warnings (likely a programming error: caller skipped a mkdir or reordered calls). - - Reads the cached ``_DEFAULT_UMASK`` directly rather than peeking via - ``os.umask(0)`` to avoid a process-global race in threaded code. """ + # 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 From 8d3efc1a3baf90f4166d2dcbcb67048fafb67990 Mon Sep 17 00:00:00 2001 From: Jeremy Rapin Date: Thu, 30 Apr 2026 09:34:26 +0200 Subject: [PATCH 3/3] smaller --- exca/base.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/exca/base.py b/exca/base.py index db0dfed8..e6ab7f18 100644 --- a/exca/base.py +++ b/exca/base.py @@ -191,10 +191,8 @@ 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: BaseInfra has private attrs, so pydantic installs - # `init_private_attributes` as model_post_init unless we define one. - # That bare function does not chain to super, which would break - # SubmititMixin's folder/cluster validation in MapInfra/TaskInfra MRO. + # required: pydantic's auto-generated init_private_attributes hook + # would otherwise replace this and skip SubmititMixin's validators super().model_post_init(log__) def config(self, uid: bool = True, exclude_defaults: bool = False) -> ConfDict: