diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index 468628a3..7be303c6 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -74,6 +74,22 @@ __all__ = ("SessionState", "Session", "EnvironmentIdentifier") +SESSION_DIR_NAME_LENGTH = 8 +"""Length of a session working directory's name. + +- Every character is charged against MAX_PATH (260) for the applications a job + runs, which are not long-path aware, so `LongPathsEnabled` and `\\\\?\\` do not + help them. +- The name is mkdtemp()'s 8 random characters and nothing else. Passing the + 40-character session id as a prefix cost 48. +- The id was pure label there: mkdtemp() generates the random characters and + creates each candidate with an exclusive mkdir(), retrying under a taken name. +- Directory -> session stays recoverable from the session log, which the Worker + Agent writes to `//.log` and which + outlives the directory. See _create_working_directory. +""" + + class SessionState(str, Enum): READY = "ready" """The state of a Session when it is ready to run actions. @@ -401,6 +417,12 @@ def __init__( within all actions running within this session. Defaults to None. retain_working_dir (bool, optional): If set, then the Session's Working Directory is not deleted when this Session object is deleted. Defaults to False. + Note: the working directory's *name* no longer encodes the session id + (it is mkdtemp()'s random characters and nothing else), so a retained + directory is not attributable to a session by its name alone. The + session log records the working directory's full path against the + session id; that log is how a retained or orphaned directory is + resolved back to its session. user (Optional[SessionUser]): The specific OS user to run subprocesses as, and whom will have permissions to the Session's Working Directory. Defaults to the current process user. @@ -616,6 +638,10 @@ def working_directory(self) -> Path: This is available in a Job Template's format string expressions as Session.WorkingDirectory + The directory's name is generated (mkdtemp()'s random characters) and + carries no part of the session id, so do not parse identity out of it; + the session id is recorded against this path in the session log instead. + Raises: RuntimeError: If this Session has no working directory, which means construction did not complete. @@ -2146,8 +2172,17 @@ def _create_working_directory(self) -> TempDir: ), ) + # prefix="" is deliberate, and is not the same as omitting it: mkdtemp() + # substitutes its "tmp" template for a prefix of None, costing 3 characters. + # See SESSION_DIR_NAME_LENGTH. + # # Raises: RuntimeError - return TempDir(dir=root_dir, prefix=self._session_id, user=self._user, logger=self._logger) + return TempDir( + dir=root_dir, + prefix="", + user=self._user, + logger=self._logger, + ) def _create_files_directory(self) -> TempDir: """Creates the subdirectory of the working directory in which we'll materialize diff --git a/src/openjd/sessions/_tempdir.py b/src/openjd/sessions/_tempdir.py index c96f9702..92d46ce2 100644 --- a/src/openjd/sessions/_tempdir.py +++ b/src/openjd/sessions/_tempdir.py @@ -261,7 +261,9 @@ def __init__( dir (Optional[Path]): The directory in which to create the temp dir. Defaults to tempfile.gettempdir(). prefix (Optional[str]): A prefix to use in the name of the generated temp dir. - Defaults to "". + Defaults to None, which mkdtemp() replaces with its "tmp" template + (3 characters). Pass "" for no prefix at all -- a shorter name, and + not the same as omitting the argument. user (Optional[SessionUser]): A group that will own the created directory. The group-write bit will be set on the directory if this option is supplied. Defaults to this process' effective user/group. diff --git a/test/openjd/sessions_v0/test_session.py b/test/openjd/sessions_v0/test_session.py index 75caf1a0..916b16f7 100644 --- a/test/openjd/sessions_v0/test_session.py +++ b/test/openjd/sessions_v0/test_session.py @@ -5,6 +5,7 @@ import os import stat import sys +import threading import time import uuid from datetime import timedelta @@ -53,10 +54,12 @@ LogContent, ) from openjd.sessions import _path_mapping as path_mapping_impl_mod +from openjd.sessions import _session as session_module from openjd.sessions._action_filter import ActionMessageKind from openjd.sessions._os_checker import is_posix, is_windows from openjd.sessions._logging import LoggerAdapter, LogExtraInfo from openjd.sessions._session import ( + SESSION_DIR_NAME_LENGTH, EnvironmentVariableChange, EnvironmentVariableSetChange, EnvironmentVariableUnsetChange, @@ -548,6 +551,193 @@ class StatReturn: ) +class TestSessionWorkingDirectoryName: + """The session working directory's name is a Windows MAX_PATH budget decision. + + - Every character is charged against MAX_PATH for the applications a job runs. + - Uniqueness is all the name carries; mkdtemp() supplies it. The uniqueness + tests are a regression guard on that, not a test of entropy. + - Directory -> session is the log's job, not the name's. See + test_full_session_id_remains_in_the_log. + """ + + # session-<32 hex uuid>, as a Deadline Cloud session id is shaped. + DEADLINE_SHAPED_ID = f"session-{uuid.uuid4().hex}" + + def test_working_directory_name_length_is_bounded(self) -> None: + # A ceiling, not an equality: the exact count is mkdtemp()'s to choose + # (CPython currently yields 8 characters, hence the constant) and a stdlib + # change to a *shorter* name would not be a regression. What must not + # regress is the budget -- passing the 40-character session id as a prefix + # cost 48. + + # GIVEN + job_params = dict[str, ParameterValue]() + + # WHEN + with Session( + session_id=self.DEADLINE_SHAPED_ID, job_parameter_values=job_params + ) as session: + # THEN + assert len(session.working_directory.name) <= SESSION_DIR_NAME_LENGTH + + def test_working_directory_is_created_with_an_empty_prefix(self) -> None: + # prefix="" rather than None is the load-bearing choice: None restores + # mkdtemp()'s "tmp" template (3 characters) and the session id (48) is what + # we removed. Assert the argument we pass, not a sample of the random name: + # inspecting the name for the *absence* of a prefix is probabilistic -- a + # random [a-z0-9_] name can happen to begin "tmp" -- while the call is exact. + + # GIVEN + job_params = dict[str, ParameterValue]() + + # WHEN + with patch.object(session_module, "TempDir", wraps=session_module.TempDir) as mock_tempdir: + with Session( + session_id=self.DEADLINE_SHAPED_ID, job_parameter_values=job_params + ) as session: + # THEN + # The first TempDir call is the working directory; the second is + # the nested embedded_files directory. + assert mock_tempdir.call_args_list[0].kwargs["prefix"] == "" + # ... and nothing of the session id survived into the name. + assert self.DEADLINE_SHAPED_ID not in session.working_directory.name + + @pytest.mark.usefixtures("caplog") # built-in fixture + def test_full_session_id_remains_in_the_log(self, caplog: pytest.LogCaptureFixture) -> None: + # The only thing that resolves an orphaned directory back to a session now. + # Must not be relaxed: announced at init, logged beside the working + # directory's path, and on every record as the `session_id` extra. + + # GIVEN + job_params = dict[str, ParameterValue]() + + # WHEN + with Session( + session_id=self.DEADLINE_SHAPED_ID, job_parameter_values=job_params + ) as session: + # THEN + assert any( + m == f"Initializing Open Job Description Session: {self.DEADLINE_SHAPED_ID}" + for m in caplog.messages + ) + assert any( + m == f"Session Working Directory: {str(session.working_directory)}" + for m in caplog.messages + ) + # caplog.records collects from every logger propagating to root, so + # filter to this library's before asserting the extra is present -- + # and default to None, not the expected id, so a record that is + # *missing* the extra fails rather than passing vacuously (which is + # precisely the regression -- a dropped LoggerAdapter -- this guards). + session_records = [r for r in caplog.records if r.name.startswith("openjd.sessions")] + assert session_records + assert all( + getattr(r, "session_id", None) == self.DEADLINE_SHAPED_ID for r in session_records + ) + + @pytest.mark.usefixtures("tmp_path") # built-in fixture + def test_many_sessions_in_one_root_get_distinct_directories(self, tmp_path: Path) -> None: + # Guard on mkdtemp()'s exclusive-create. Sessions sharing one root is the + # normal case for a worker host. All held open at once, so no directory is + # free by virtue of an earlier one having been cleaned up. + + # GIVEN + job_params = dict[str, ParameterValue]() + sessions: list[Session] = [] + + try: + # WHEN + for _ in range(64): + sessions.append( + Session( + session_id=f"session-{uuid.uuid4().hex}", + job_parameter_values=job_params, + session_root_directory=tmp_path, + ) + ) + + # THEN + names = [s.working_directory.name for s in sessions] + assert len(set(names)) == len(names) + assert all(s.working_directory.is_dir() for s in sessions) + # ... and the same for the nested embedded files directories. + assert all(s.files_directory.is_dir() for s in sessions) + finally: + for session in sessions: + session.cleanup() + + @pytest.mark.usefixtures("tmp_path") # built-in fixture + def test_concurrent_sessions_in_one_root_get_distinct_directories(self, tmp_path: Path) -> None: + # The same under real concurrency: mkdtemp() is create-or-fail, not + # check-then-create, so there is no window between picking and creating. + + # GIVEN + job_params = dict[str, ParameterValue]() + sessions: list[Session] = [] + lock = threading.Lock() + errors: list[Exception] = [] + + def make_session() -> None: + try: + session = Session( + session_id=f"session-{uuid.uuid4().hex}", + job_parameter_values=job_params, + session_root_directory=tmp_path, + ) + except Exception as err: # pragma: nocover - only on a failure + with lock: + errors.append(err) + return + with lock: + sessions.append(session) + + threads = [threading.Thread(target=make_session) for _ in range(16)] + + try: + # WHEN + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + # THEN + assert errors == [] + names = [s.working_directory.name for s in sessions] + assert len(names) == len(threads) + assert len(set(names)) == len(names) + finally: + for session in sessions: + session.cleanup() + + def test_directory_names_are_portable_across_platforms(self) -> None: + # One generator runs on every platform, so the name must be legal on all of + # them: no Windows-reserved character or device name. + # + # Lower case specifically: NTFS folds case, so names differing only in case + # would be two directories on Linux and one on Windows. mkdtemp()'s alphabet + # is lower case only, so that cannot arise. + + # GIVEN + job_params = dict[str, ParameterValue]() + windows_reserved_chars = set('<>:"/\\|?*') + windows_reserved_names = {"con", "prn", "aux", "nul"} | { + f"{stem}{n}" for stem in ("com", "lpt") for n in range(1, 10) + } + + # WHEN + with Session( + session_id=self.DEADLINE_SHAPED_ID, job_parameter_values=job_params + ) as session: + for name in (session.working_directory.name, session.files_directory.name): + # THEN + assert not (set(name) & windows_reserved_chars) + assert not any(ord(c) < 32 for c in name) + assert not name.endswith((".", " ")) + assert name.split(".")[0] not in windows_reserved_names + assert name == name.lower() + + class TestSessionCallbacks: """Making sure that the session methods that are callbacks also call the user-provided callback with the expected data.