From b6e4f8d5d3c4f77f0fcb28537015779951056337 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Wed, 12 Aug 2026 10:26:05 -0700 Subject: [PATCH 1/2] feat!: shorten the session working directory name for Windows MAX_PATH The applications a job runs on Windows -- After Effects and Cinema4D among them -- use the legacy Win32 file APIs, which cap a path at MAX_PATH (260 characters) regardless of the LongPathsEnabled registry value and regardless of whether this library prefixes its own paths with \\?\. Nothing but a shorter path helps them. The session working directory name was the full session id used verbatim as the mkdtemp() prefix. For a Deadline Cloud session that is 40 characters (`session-` plus a 32 character hex uuid) plus mkdtemp()'s 8 random characters: 48 characters, of which the uuid and the random suffix were both sources of uniqueness. Only one of them needs to be. Use the last SESSION_DIR_ID_LENGTH (6) characters of the session id as the prefix instead, giving a 14 character name -- 34 characters recovered. The tail rather than the head because a session id conventionally leads with constant text saying what kind of id it is, so a leading slice can carry no correlation at all. Uniqueness is unaffected. mkdtemp() creates each candidate name with an exclusive mkdir() and retries on FileExistsError (verified identical in CPython 3.9 through 3.14, the supported range), so it cannot return an existing directory however short the prefix is. The truncated id is purely an operator-facing label; the full id is unchanged and still appears on every log record for the session and immediately beside this directory's path at initialization. Also shorten the embedded files subdirectory prefix from "embedded_files" to "ef", recovering 12 more characters from every embedded file path. That directory nests inside the session working directory, so its name is charged to everything beneath it. BREAKING CHANGE: the on-disk name of the session working directory is no longer prefixed with the session id, and the embedded files subdirectory is named "ef" rather than "embedded_files". Neither name is part of the public Python interface -- Session.working_directory and Session.files_directory are unchanged, as is OPENJD_SESSION_WORKING_DIR -- but anything that pattern-matches the directory name must be updated. Known consumer: deadline-cloud-worker-agent's test/e2e/test_worker_config.py asserts a `session-[a-f0-9]{32}` directory name, and its docs/state.md documents the old shape. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- src/openjd/sessions/_session.py | 27 +++- test/openjd/sessions_v0/test_session.py | 176 ++++++++++++++++++++++++ 2 files changed, 202 insertions(+), 1 deletion(-) diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index 468628a3..9a81c204 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. @@ -2146,8 +2162,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/test/openjd/sessions_v0/test_session.py b/test/openjd/sessions_v0/test_session.py index 75caf1a0..7be14eda 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 @@ -57,6 +58,7 @@ 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 +550,180 @@ 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: + # 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 + # Guard on the budget itself, independent of the constant: this cost 48. + assert len(session.working_directory.name) <= 8 + + def test_working_directory_name_carries_no_part_of_the_session_id(self) -> None: + # prefix="" rather than None: None would restore mkdtemp()'s "tmp" template, + # so guard the distinction and not just the absence of the id. + + # GIVEN + job_params = dict[str, ParameterValue]() + + # WHEN + with Session( + session_id=self.DEADLINE_SHAPED_ID, job_parameter_values=job_params + ) as session: + # THEN + name = session.working_directory.name + assert not name.startswith("tmp") + assert "session" not in name + assert not any( + self.DEADLINE_SHAPED_ID[-n:] in name for n in range(4, len(self.DEADLINE_SHAPED_ID)) + ) + + @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 + ) + assert all( + getattr(r, "session_id", self.DEADLINE_SHAPED_ID) == self.DEADLINE_SHAPED_ID + for r in caplog.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[BaseException] = [] + + 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 BaseException 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. From 732c800ed019c634dae952f416fd7bafea7187a8 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Wed, 12 Aug 2026 20:10:35 -0700 Subject: [PATCH 2/2] Address review: deterministic dir-name tests + doc callouts - test_full_session_id_remains_in_the_log: filter to openjd.sessions records and default the session_id getattr to None, so a dropped LoggerAdapter fails the assertion instead of passing vacuously. - Replace the probabilistic name-inspection assertions with a deterministic check that TempDir is called with prefix="". - Assert the name length as a <= budget ceiling rather than == to an incidental CPython detail. - Narrow the concurrent test's except from BaseException to Exception (CodeQL). - Fix TempDir.prefix docstring (default is None -> "tmp", not ""). - Note on working_directory / retain_working_dir that the directory name no longer carries session identity; the log holds that mapping. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- src/openjd/sessions/_session.py | 10 +++++ src/openjd/sessions/_tempdir.py | 4 +- test/openjd/sessions_v0/test_session.py | 54 ++++++++++++++++--------- 3 files changed, 47 insertions(+), 21 deletions(-) diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index 9a81c204..7be303c6 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -417,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. @@ -632,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. 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 7be14eda..916b16f7 100644 --- a/test/openjd/sessions_v0/test_session.py +++ b/test/openjd/sessions_v0/test_session.py @@ -54,6 +54,7 @@ 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 @@ -564,6 +565,12 @@ class TestSessionWorkingDirectoryName: 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]() @@ -572,28 +579,29 @@ def test_working_directory_name_length_is_bounded(self) -> None: session_id=self.DEADLINE_SHAPED_ID, job_parameter_values=job_params ) as session: # THEN - assert len(session.working_directory.name) == SESSION_DIR_NAME_LENGTH - # Guard on the budget itself, independent of the constant: this cost 48. - assert len(session.working_directory.name) <= 8 + assert len(session.working_directory.name) <= SESSION_DIR_NAME_LENGTH - def test_working_directory_name_carries_no_part_of_the_session_id(self) -> None: - # prefix="" rather than None: None would restore mkdtemp()'s "tmp" template, - # so guard the distinction and not just the absence of the id. + 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 Session( - session_id=self.DEADLINE_SHAPED_ID, job_parameter_values=job_params - ) as session: - # THEN - name = session.working_directory.name - assert not name.startswith("tmp") - assert "session" not in name - assert not any( - self.DEADLINE_SHAPED_ID[-n:] in name for n in range(4, len(self.DEADLINE_SHAPED_ID)) - ) + 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: @@ -617,9 +625,15 @@ def test_full_session_id_remains_in_the_log(self, caplog: pytest.LogCaptureFixtu 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", self.DEADLINE_SHAPED_ID) == self.DEADLINE_SHAPED_ID - for r in caplog.records + getattr(r, "session_id", None) == self.DEADLINE_SHAPED_ID for r in session_records ) @pytest.mark.usefixtures("tmp_path") # built-in fixture @@ -662,7 +676,7 @@ def test_concurrent_sessions_in_one_root_get_distinct_directories(self, tmp_path job_params = dict[str, ParameterValue]() sessions: list[Session] = [] lock = threading.Lock() - errors: list[BaseException] = [] + errors: list[Exception] = [] def make_session() -> None: try: @@ -671,7 +685,7 @@ def make_session() -> None: job_parameter_values=job_params, session_root_directory=tmp_path, ) - except BaseException as err: # pragma: nocover - only on a failure + except Exception as err: # pragma: nocover - only on a failure with lock: errors.append(err) return