Add timeouts for botocore & fix exception when calling tidy

.github/workflows/test.yml

@@ -4,7 +4,7 @@ env:
   DEFAULT_PYTHON: "3.11"
   PRE_COMMIT_CACHE: ~/.cache/pre-commit
   KEY_PREFIX: base-venv
-  CACHE_VERSION: 1
+  CACHE_VERSION: 2
 # yamllint disable-line rule:truthy
 on:
   push:

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changelog
 
+# v26.28.0
+
+- Add support for specifying botore timeouts and retries for s3 transfers/executions
+- Fix issue in tidy throwing an exception when the s3 client was already closed
+
 # v26.18.0
 
 - Fix a bug where the S3 client was not being properly closed and garbage collected, which could lead to resource leaks and issues with too many open connections. This was caused by the `tidy` method not setting the `s3_client` attribute to `None` after closing it, which meant that the botocore objects were not being garbage collected.

README.md

@@ -140,6 +140,20 @@ JSON configs for transfers can be defined as follows:
 ]
 ```
 
+### Default S3 client timeout behaviour
+
+S3 transfers now apply botocore defaults even when not specified in the protocol:
+
+- `botocoreReadTimeout`: `60`
+- `botocoreConnectTimeout`: `10`
+- `max_attempts`: `0`
+
+These are socket/connect and retry settings, not a total transfer deadline. A large
+upload or download that is still making progress can continue past the batch timeout
+request and still complete successfully. The defaults mainly prevent stalled S3 calls
+from sitting in botocore retries for longer than necessary. All three values can still
+be overridden per protocol when needed.
+
 ## Example S3 upload with flag files
 
 ```json

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "otf-addons-aws"
-version = "v26.18.0"
+version = "v26.28.0"
 authors = [{ name = "Adam McDonagh", email = "adam@elitemonkey.net" }]
 license = { text = "GPLv3" }
 classifiers = [
@@ -14,7 +14,7 @@ classifiers = [
 ]
 keywords = ["automation", "task", "framework", "aws", "s3", "ssm", "otf"]
 dependencies = [
-    "boto3 >= 1.26",
+    "boto3 >= 1.43",
     "jsonpath-ng >= 1.7.0",
     "opentaskpy >= v26.18.1",
 ]
@@ -24,8 +24,8 @@ requires-python = ">=3.11"
 
 [project.optional-dependencies]
 dev = [
-    "awscli",
-    "awscli-local",
+    "awscli >= 1.45.51",
+    "awscli-local >= 0.22.2",
     "pytest-shell",
     "types-requests",
     "types-python-dateutil",
@@ -57,7 +57,7 @@ dev = [
 profile = 'black'
 
 [tool.bumpver]
-current_version = "v26.18.0"
+current_version = "v26.28.0"
 version_pattern = "vYY.WW.PATCH[-TAG]"
 commit_message = "bump version {old_version} -> {new_version}"
 commit = true
@@ -373,8 +373,6 @@ ignore = [
     "D407", # Section name underlining
     "E501", # line too long
     "E731", # do not assign a lambda expression, use a def
-    # Ignored due to performance: https://github.com/charliermarsh/ruff/issues/2923
-    "UP038", # Use `X | Y` in `isinstance` call instead of `(X, Y)`
 ]
 
 

src/opentaskpy/addons/aws/remotehandlers/creds.py

@@ -96,6 +96,8 @@ def get_aws_client(  # pylint: disable=too-many-positional-arguments
     kwargs = {}
     if os.environ.get("AWS_ENDPOINT_URL"):
         kwargs["endpoint_url"] = os.environ.get("AWS_ENDPOINT_URL")
+    if credentials.get("region_name"):
+        kwargs["region_name"] = credentials["region_name"]
 
     if assume_role_arn:
         logger.info(f"Assuming role: {assume_role_arn}")

src/opentaskpy/addons/aws/remotehandlers/s3.py

@@ -7,6 +7,7 @@
 
 import boto3
 import opentaskpy.otflogging
+from botocore.config import Config
 from botocore.exceptions import ClientError
 from dateutil.tz import tzlocal
 from opentaskpy.remotehandlers.remotehandler import (
@@ -17,6 +18,32 @@
 from .creds import get_aws_client, set_aws_creds
 
 MAX_OBJECTS_PER_QUERY = 100
+DEFAULT_S3_BOTOCORE_READ_TIMEOUT = 60
+DEFAULT_S3_BOTOCORE_CONNECT_TIMEOUT = 10
+DEFAULT_S3_BOTOCORE_MAX_ATTEMPTS = 0
+
+
+def _build_botocore_config(protocol: dict) -> Config:
+    """Build a botocore client config from the task protocol settings.
+
+    Defaults are applied so S3 transfers do not inherit botocore retry behaviour that
+    can keep a batch blocked longer than expected after a timeout request.
+    """
+    config_options = {
+        "read_timeout": protocol.get(
+            "botocoreReadTimeout", DEFAULT_S3_BOTOCORE_READ_TIMEOUT
+        ),
+        "connect_timeout": protocol.get(
+            "botocoreConnectTimeout", DEFAULT_S3_BOTOCORE_CONNECT_TIMEOUT
+        ),
+        "retries": {
+            "max_attempts": protocol.get(
+                "max_attempts", DEFAULT_S3_BOTOCORE_MAX_ATTEMPTS
+            )
+        },
+    }
+
+    return Config(**config_options)
 
 
 class S3Transfer(RemoteTransferHandler):
@@ -74,12 +101,15 @@ def validate_or_refresh_creds(self) -> None:
             if self.temporary_creds:
                 self.logger.info("Renewing temporary credentials")
 
+            client_config = _build_botocore_config(self.spec["protocol"])
+
             client_result = get_aws_client(
                 "s3",
                 self.credentials,
                 token_expiry_seconds=self.token_expiry_seconds,
                 assume_role_arn=self.assume_role_arn,
                 assume_role_external_id=self.assume_role_external_id,
+                config=client_config,
             )
             self.temporary_creds = (
                 client_result["temporary_creds"]
@@ -484,7 +514,8 @@ def create_flag_files(self) -> int:
 
     def tidy(self) -> None:
         """Tidy up the S3 client."""
-        self.s3_client.close()
+        if self.s3_client and hasattr(self.s3_client, "close"):
+            self.s3_client.close()
         self.s3_client = None  # allow botocore objects to be garbage collected
 
 
@@ -593,5 +624,6 @@ def execute(self) -> bool:
 
     def tidy(self) -> None:
         """Tidy up the S3 client."""
-        self.s3_client.close()
+        if self.s3_client and hasattr(self.s3_client, "close"):
+            self.s3_client.close()
         self.s3_client = None  # allow botocore objects to be garbage collected

src/opentaskpy/addons/aws/remotehandlers/schemas/transfer/s3_destination/protocol.json

@@ -27,6 +27,15 @@
     },
     "region_name": {
       "type": "string"
+    },
+    "botocoreReadTimeout": {
+      "type": "integer"
+    },
+    "botocoreConnectTimeout": {
+      "type": "integer"
+    },
+    "max_attempts": {
+      "type": "integer"
     }
   },
   "required": ["name"],

src/opentaskpy/addons/aws/remotehandlers/schemas/transfer/s3_source/protocol.json

@@ -27,6 +27,15 @@
     },
     "region_name": {
       "type": "string"
+    },
+    "botocoreReadTimeout": {
+      "type": "integer"
+    },
+    "botocoreConnectTimeout": {
+      "type": "integer"
+    },
+    "max_attempts": {
+      "type": "integer"
     }
   },
   "required": ["name"],

src/opentaskpy/plugins/lookup/aws/secrets_manager.py

@@ -110,15 +110,15 @@ def run(**kwargs):  # type: ignore[no-untyped-def]
 
                 # If the result is a list, then return the first element, with a warning
                 # that there's more than one
-                if isinstance(result, list) and isinstance(result[0], (str, int)):
+                if isinstance(result, list) and isinstance(result[0], str | int):
                     logger.warning(
                         f"JSONPath returned a list of length {len(result)}. Returning "
                         + "only the first element"
                     )
                     result = result[0]
 
                 # If the result is not a string or an int, then raise an exception
-                if not isinstance(result, (str, int)):
+                if not isinstance(result, str | int):
                     if fail_on_exception:
                         raise LookupPluginError(
                             f"JSONPath returned a value of type {type(result)}. Expected a string or int"

tests/fixtures/localstack.py

@@ -53,6 +53,7 @@ def credentials(floci, cleanup_credentials):
     os.environ["AWS_ACCESS_KEY_ID"] = "test"
     os.environ["AWS_SECRET_ACCESS_KEY"] = "test"
     os.environ["AWS_REGION"] = "eu-west-1"
+    os.environ["AWS_DEFAULT_REGION"] = "eu-west-1"
     os.environ["AWS_ENDPOINT_URL"] = floci
 
 
@@ -64,6 +65,8 @@ def cleanup_credentials():
         del os.environ["AWS_SECRET_ACCESS_KEY"]
     if os.environ.get("AWS_REGION"):
         del os.environ["AWS_REGION"]
+    if os.environ.get("AWS_DEFAULT_REGION"):
+        del os.environ["AWS_DEFAULT_REGION"]
     if os.environ.get("AWS_ENDPOINT_URL"):
         del os.environ["AWS_ENDPOINT_URL"]
     if os.environ.get("ASSUME_ROLE_ARN"):