fix: Add better log when JA hit Windows path length limit (#403)

* fix: Add better log when JA hit Windows path length limit Signed-off-by: Alex Tran <[email protected]> * fix: only throw error when we confirm long path registry is off Signed-off-by: Alex Tran <[email protected]> * fix:add mock for path length registry check function in unit tests Signed-off-by: Alex Tran <[email protected]> * fix:remove duplicated code Signed-off-by: Alex Tran <[email protected]> * fix:change if else logic to cover all cases Signed-off-by: Alex Tran <[email protected]> * fix:format Signed-off-by: Alex Tran <[email protected]> * fix:fixing integ test to block false positive Signed-off-by: Alex Tran <[email protected]> --------- Signed-off-by: Alex Tran <[email protected]>
aws-deadline · Jul 18, 2024 · 0344537 · 0344537
1 parent 74293ac
commit 0344537
Show file tree

Hide file tree

Showing 4 changed files with 262 additions and 2 deletions.
diff --git a/src/deadline/job_attachments/_utils.py b/src/deadline/job_attachments/_utils.py
@@ -5,6 +5,8 @@
 from pathlib import Path
 from typing import Tuple, Union
 import uuid
+import ctypes
+import sys
 
 
 __all__ = [
@@ -86,3 +88,13 @@ def _is_relative_to(path1: Union[Path, str], path2: Union[Path, str]) -> bool:
         return True
     except ValueError:
         return False
+
+
+def _is_windows_file_path_limit() -> bool:
+    if sys.platform == "win32":
+        ntdll = ctypes.WinDLL("ntdll")
+        ntdll.RtlAreLongPathsEnabled.restype = ctypes.c_ubyte
+        ntdll.RtlAreLongPathsEnabled.argtypes = ()
+
+        return bool(ntdll.RtlAreLongPathsEnabled())
+    return True
diff --git a/src/deadline/job_attachments/download.py b/src/deadline/job_attachments/download.py
@@ -7,6 +7,7 @@
 import io
 import os
 import re
+import sys
 import time
 from collections import defaultdict
 from datetime import datetime
@@ -67,11 +68,13 @@
     _set_fs_group_for_posix,
     _set_fs_permission_for_windows,
 )
-from ._utils import _is_relative_to, _join_s3_paths
+from ._utils import _is_relative_to, _join_s3_paths, _is_windows_file_path_limit
 
 download_logger = getLogger("deadline.job_attachments.download")
 
 S3_DOWNLOAD_MAX_CONCURRENCY = 10
+WINDOWS_MAX_PATH_LENGTH = 260
+TEMP_DOWNLOAD_ADDED_CHARS_LENGTH = 9
 
 
 def get_manifest_from_s3(
@@ -516,6 +519,25 @@ def process_client_error(exc: ClientError, status_code: int):
             error_details=str(bce),
         ) from bce
     except Exception as e:
+        # Add 9 to account for .Hex value when file in the middle of downloading in windows paths
+        # For example: file test.txt when download will be test.txt.H4SD9Ddj
+        if (
+            len(str(local_file_name)) + TEMP_DOWNLOAD_ADDED_CHARS_LENGTH >= WINDOWS_MAX_PATH_LENGTH
+        ) and sys.platform == "win32":
+            uncPath = str(local_file_name).startswith("\\\\?\\")
+            if not uncPath:
+                # Path don't start with \\?\ -> Long path error
+                raise AssetSyncError(
+                    "Your file path is longer than what Windows allow.\n"
+                    + "This could be the error if you do not enable longer file path in Windows"
+                )
+            elif not _is_windows_file_path_limit():
+                # Path start with \\?\ but do not enable registry -> Undefined error
+                raise AssetSyncError(
+                    f"{e}\nUNC notation exist, but long path registry not enabled. Undefined error"
+                ) from e
+
+        # Path start with \\?\ and registry is enable, something else cause this error
         raise AssetSyncError(e) from e
 
     download_logger.debug(f"Downloaded {file.path} to {str(local_file_name)}")
@@ -1097,7 +1119,9 @@ def set_root_path(self, original_root: str, new_root: str) -> None:
         new_root = str(os.path.normpath(Path(new_root).absolute()))
 
         if original_root not in self.outputs_by_root:
-            raise ValueError(f"The root path {original_root} was not found in output manifests.")
+            raise ValueError(
+                f"The root path {original_root} was not found in output manifests {self.outputs_by_root}."
+            )
 
         if new_root == original_root:
             return

diff --git a/test/integ/deadline_job_attachments/test_job_attachments.py b/test/integ/deadline_job_attachments/test_job_attachments.py
@@ -34,6 +34,7 @@
 from deadline.job_attachments._utils import (
     _get_unique_dest_dir_name,
 )
+from deadline.job_attachments._utils import _is_windows_file_path_limit
 from .conftest import is_windows_non_admin
 
 
@@ -1487,3 +1488,48 @@ def test_download_outputs_bucket_wrong_account(
             task_id=sync_outputs.step0_task0_id,
         )
         job_output_downloader.download_job_output()
+
+
+@pytest.mark.integ
+@pytest.mark.skipif(
+    _is_windows_file_path_limit(),
+    reason="This test is for Windows max file path length error, skipping this if Windows path limit is extended",
+)
+def test_download_outputs_windows_max_file_path_length_exception(
+    job_attachment_test: JobAttachmentTest,
+    sync_outputs: SyncOutputsOutput,
+):
+    """
+    Test that if trying to download outputs to a file path that
+    longer than 260 chars in Windows, the correct error is thrown.
+    """
+    long_root_path = Path(__file__).parent / str("A" * 135)
+
+    job_attachment_settings = get_queue(
+        farm_id=job_attachment_test.farm_id,
+        queue_id=job_attachment_test.queue_id,
+        deadline_endpoint_url=job_attachment_test.deadline_endpoint,
+    ).jobAttachmentSettings
+
+    if job_attachment_settings is None:
+        raise Exception("Job attachment settings must be set for this test.")
+
+    job_output_downloader = download.OutputDownloader(
+        s3_settings=job_attachment_settings,
+        farm_id=job_attachment_test.farm_id,
+        queue_id=job_attachment_test.queue_id,
+        job_id=sync_outputs.job_id,
+        step_id=sync_outputs.step0_id,
+        task_id=sync_outputs.step0_task0_id,
+    )
+    job_output_downloader.set_root_path(str(job_attachment_test.ASSET_ROOT), str(long_root_path))
+
+    # WHEN
+    with pytest.raises(
+        AssetSyncError,
+        match=(
+            "Your file path is longer than what Windows allow.\n"
+            + "This could be the error if you do not enable longer file path in Windows"
+        ),
+    ):
+        job_output_downloader.download_job_output()
diff --git a/test/unit/deadline_job_attachments/test_download.py b/test/unit/deadline_job_attachments/test_download.py
@@ -1963,6 +1963,184 @@ def test_download_file_error_message_on_timeout(self):
                 " or contact support for further assistance."
             ) in str(exc.value)
 
+    @mock_aws
+    @pytest.mark.skipif(
+        sys.platform == "win32",
+        reason="This test is for Linux path only.",
+    )
+    def test_windows_long_path_exception_PosixOS(self):
+        mock_s3_client = MagicMock()
+        mock_future = MagicMock()
+        mock_transfer_manager = MagicMock()
+        mock_transfer_manager.download.return_value = mock_future
+        mock_future.result.side_effect = Exception("Test exception")
+
+        file_path = ManifestPathv2023_03_03(
+            path="very/long/input/to/test/windows/max/file/path/for/error/handling/when/downloading/assest/from/job/attachment.txt",
+            hash="path",
+            size=1,
+            mtime=1234000000,
+        )
+
+        local_path = "Users/path/to/a/very/long/file/path/that/exceeds/the/windows/max/path/length/for/testing/max/file/path/error/handling/when/download/or/syncing/assest/using/job/attachment"
+
+        with patch(
+            f"{deadline.__package__}.job_attachments.download.get_s3_client",
+            return_value=mock_s3_client,
+        ), patch(
+            f"{deadline.__package__}.job_attachments.download.get_s3_transfer_manager",
+            return_value=mock_transfer_manager,
+        ), patch(
+            f"{deadline.__package__}.job_attachments.download.Path.mkdir"
+        ):
+            with pytest.raises(AssetSyncError) as exc:
+                download_file(
+                    file_path,
+                    HashAlgorithm.XXH128,
+                    local_path,
+                    "test-bucket",
+                    "rootPrefix/Data",
+                    mock_s3_client,
+                )
+
+        expected_message = "Test exception"
+        assert str(exc.value) == expected_message
+
+    @mock_aws
+    @pytest.mark.skipif(
+        sys.platform != "win32",
+        reason="This test is for Windows path only.",
+    )
+    def test_windows_long_path_exception_WindowsOS(self):
+        mock_s3_client = MagicMock()
+        mock_future = MagicMock()
+        mock_transfer_manager = MagicMock()
+        mock_transfer_manager.download.return_value = mock_future
+        mock_future.result.side_effect = Exception("Test exception")
+
+        file_path = ManifestPathv2023_03_03(
+            path="very/long/input/to/test/windows/max/file/path/for/error/handling/when/downloading/assest/from/job/attachment.txt",
+            hash="path",
+            size=1,
+            mtime=1234000000,
+        )
+
+        local_path = "C:\\path\\to\\a\\very\\long\\file\\path\\that\\exceeds\\the\\windows\\max\\path\\length\\for\\testing\\max\\file\\path\\error\\handling\\when\\download\\or\\syncing\\assest\\using\\job\\attachment"
+
+        with patch(
+            f"{deadline.__package__}.job_attachments.download.get_s3_client",
+            return_value=mock_s3_client,
+        ), patch(
+            f"{deadline.__package__}.job_attachments.download.get_s3_transfer_manager",
+            return_value=mock_transfer_manager,
+        ), patch(
+            f"{deadline.__package__}.job_attachments.download.Path.mkdir"
+        ):
+            with pytest.raises(AssetSyncError) as exc:
+                download_file(
+                    file_path,
+                    HashAlgorithm.XXH128,
+                    local_path,
+                    "test-bucket",
+                    "rootPrefix/Data",
+                    mock_s3_client,
+                )
+
+        expected_message = "Your file path is longer than what Windows allow.\nThis could be the error if you do not enable longer file path in Windows"
+        assert str(exc.value) == expected_message
+
+    @mock_aws
+    @pytest.mark.skipif(
+        sys.platform != "win32",
+        reason="This test is for Windows path only.",
+    )
+    def test_windows_long_path_UNC_notation_WindowsOS(self):
+        mock_s3_client = MagicMock()
+        mock_future = MagicMock()
+        mock_transfer_manager = MagicMock()
+        mock_transfer_manager.download.return_value = mock_future
+        mock_future.result.side_effect = Exception("Test exception")
+
+        file_path = ManifestPathv2023_03_03(
+            path="very/long/input/to/test/windows/max/file/path/for/error/handling/when/downloading/assest/from/job/attachment.txt",
+            hash="path",
+            size=1,
+            mtime=1234000000,
+        )
+
+        local_path = "\\\\?\\C:\\path\\to\\a\\very\\long\\file\\path\\that\\exceeds\\the\\windows\\max\\path\\length\\for\\testing\\max\\file\\path\\error\\handling\\when\\download\\or\\syncing\\assest\\using\\job\\attachment"
+
+        with patch(
+            f"{deadline.__package__}.job_attachments.download.get_s3_client",
+            return_value=mock_s3_client,
+        ), patch(
+            f"{deadline.__package__}.job_attachments.download.get_s3_transfer_manager",
+            return_value=mock_transfer_manager,
+        ), patch(
+            f"{deadline.__package__}.job_attachments.download._is_windows_file_path_limit",
+            return_value=False,
+        ), patch(
+            f"{deadline.__package__}.job_attachments.download.Path.mkdir"
+        ):
+            with pytest.raises(AssetSyncError) as exc:
+                download_file(
+                    file_path,
+                    HashAlgorithm.XXH128,
+                    local_path,
+                    "test-bucket",
+                    "rootPrefix/Data",
+                    mock_s3_client,
+                )
+
+        expected_message = "Test exception\nUNC notation exist, but long path registry not enabled. Undefined error"
+        assert str(exc.value) == expected_message
+
+    @mock_aws
+    @pytest.mark.skipif(
+        sys.platform != "win32",
+        reason="This test is for Windows path only.",
+    )
+    def test_windows_long_path_UNC_notation_and_registry_WindowsOS(self):
+        mock_s3_client = MagicMock()
+        mock_future = MagicMock()
+        mock_transfer_manager = MagicMock()
+        mock_transfer_manager.download.return_value = mock_future
+        mock_future.result.side_effect = Exception("Test exception")
+
+        file_path = ManifestPathv2023_03_03(
+            path="very/long/input/to/test/windows/max/file/path/for/error/handling/when/downloading/assest/from/job/attachment.txt",
+            hash="path",
+            size=1,
+            mtime=1234000000,
+        )
+
+        local_path = "\\\\?\\C:\\path\\to\\a\\very\\long\\file\\path\\that\\exceeds\\the\\windows\\max\\path\\length\\for\\testing\\max\\file\\path\\error\\handling\\when\\download\\or\\syncing\\assest\\using\\job\\attachment"
+
+        with patch(
+            f"{deadline.__package__}.job_attachments.download.get_s3_client",
+            return_value=mock_s3_client,
+        ), patch(
+            f"{deadline.__package__}.job_attachments.download.get_s3_transfer_manager",
+            return_value=mock_transfer_manager,
+        ), patch(
+            f"{deadline.__package__}.job_attachments.download._is_windows_file_path_limit",
+            return_value=True,
+        ), patch(
+            f"{deadline.__package__}.job_attachments.download.Path.mkdir"
+        ):
+            with pytest.raises(AssetSyncError) as exc:
+                download_file(
+                    file_path,
+                    HashAlgorithm.XXH128,
+                    local_path,
+                    "test-bucket",
+                    "rootPrefix/Data",
+                    mock_s3_client,
+                )
+
+        expected_message = "Test exception"
+        assert str(exc.value) == expected_message
+
 
 @pytest.mark.parametrize("manifest_version", [ManifestVersion.v2023_03_03])
 class TestFullDownloadPrefixesWithSlashes: