Skip to content

Commit 0f0d11f

Browse files
committed
fix: address remaining review comments
- Consolidate aioboto3 import: cache module ref on self._aioboto3 in __init__, remove duplicate import in _get_session - Default save_max_retries to 10 (safe cap) instead of -1 (infinite) - Normalize metadata keys to lowercase in _flatten_metadata with case-collision warning (S3 metadata keys are case-insensitive) - Handle ClientError in _head helper: return None for objects deleted between list_objects_v2 and head_object calls, skip in result loop
1 parent 5cc19df commit 0f0d11f

1 file changed

Lines changed: 34 additions & 9 deletions

File tree

src/google/adk_community/artifacts/s3_artifact_service.py

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -37,24 +37,25 @@ def __init__(
3737
self,
3838
bucket_name: str,
3939
aws_configs: Optional[dict[str, Any]] = None,
40-
save_max_retries: int = -1,
40+
save_max_retries: int = 10,
4141
):
4242
"""Initializes the S3 artifact service.
4343
4444
Args:
4545
bucket_name: The name of the S3 bucket to use.
4646
aws_configs: Extra kwargs forwarded to the aioboto3 S3 client.
4747
Use this to pass region_name, endpoint_url (for MinIO), etc.
48-
save_max_retries: Maximum retries on version conflict. -1 means
49-
retry indefinitely.
48+
save_max_retries: Maximum retries on version conflict. Defaults
49+
to 10. Pass -1 to retry indefinitely.
5050
"""
5151
try:
52-
import aioboto3 # noqa: F401
52+
import aioboto3
5353
except ImportError as exc:
5454
raise ImportError(
5555
"aioboto3 is required to use S3ArtifactService. "
5656
"Install it with: pip install google-adk-community[s3]"
5757
) from exc
58+
self._aioboto3 = aioboto3
5859

5960
self.bucket_name = bucket_name
6061
self.aws_configs: dict[str, Any] = aws_configs or {}
@@ -63,11 +64,9 @@ def __init__(
6364
self._session_lock = asyncio.Lock()
6465

6566
async def _get_session(self):
66-
import aioboto3
67-
6867
async with self._session_lock:
6968
if self._session is None:
70-
self._session = aioboto3.Session()
69+
self._session = self._aioboto3.Session()
7170
return self._session
7271

7372
@asynccontextmanager
@@ -93,7 +92,18 @@ def _flatten_metadata(metadata: Optional[dict[str, Any]]) -> dict[str, str]:
9392
"""
9493
if not metadata:
9594
return {}
96-
flat = {str(k): json.dumps(v) for k, v in metadata.items()}
95+
# S3 user-metadata keys are case-insensitive; normalise to
96+
# lowercase to avoid silent collisions.
97+
flat: dict[str, str] = {}
98+
for k, v in metadata.items():
99+
lower_key = str(k).lower()
100+
if lower_key in flat:
101+
logger.warning(
102+
"Metadata key collision after case-normalisation: %r "
103+
"overwrites a previous key.",
104+
k,
105+
)
106+
flat[lower_key] = json.dumps(v)
97107
# Include the x-amz-meta- prefix overhead that S3 adds per key.
98108
total = sum(
99109
S3ArtifactService._S3_META_PREFIX_LEN
@@ -408,8 +418,21 @@ async def list_artifact_versions(
408418
async with self._client() as s3:
409419

410420
async def _head(key: str):
421+
"""Return head_object result, or None if the object was deleted."""
422+
from botocore.exceptions import ClientError
423+
411424
async with sem:
412-
return await s3.head_object(Bucket=self.bucket_name, Key=key)
425+
try:
426+
return await s3.head_object(Bucket=self.bucket_name, Key=key)
427+
except ClientError as e:
428+
code = e.response.get("Error", {}).get("Code", "")
429+
if code in ("NoSuchKey", "404"):
430+
logger.debug(
431+
"Object %s deleted between list and head; skipping.",
432+
key,
433+
)
434+
return None
435+
raise
413436

414437
paginator = s3.get_paginator("list_objects_v2")
415438
async for page in paginator.paginate(
@@ -423,6 +446,8 @@ async def _head(key: str):
423446
heads = await asyncio.gather(*head_tasks)
424447

425448
for obj, head in zip(page_objects, heads):
449+
if head is None:
450+
continue
426451
version_str = obj["Key"].split("/")[-1]
427452
try:
428453
version = int(version_str)

0 commit comments

Comments
 (0)