Stop using the caller's result limit as the transport page size

_get_paginated_results set PaginationConfig={'MaxItems': limit,
'PageSize': limit}. Those are different things: MaxItems is how many
results the caller asked for, PageSize is how many the service returns per
request. Tying them means a small limit walks a large collection in tiny
increments, and against a scan that matches sparsely that is pathological -
the same filtered describe_images took 977.6s at a page size of 5 against
10.0s at 1000, returning one image either way.

Ask for a full page regardless of the limit and let MaxItems do the
bounding. At worst one page more data is fetched than was wanted, and a
limit larger than a page is served over several requests.

The page size is clamped to whatever the service model declares for the
operation, because EC2's ceilings differ - DescribeRouteTables allows 100
where most allow 1000 - and exceeding one is a hard InvalidParameterValue
rather than a clamp. Where no ceiling is declared, 500 sits inside the
tightest documented one (DescribeVolumes). Deliberately not max(limit,
...), so an oversized limit is never pushed at a service that would
reject it - which the previous code could do.

This was not test-only: DEFAULT_RESULT_LIMIT is 50, so every paginated AWS
call fetched 50 at a time.

Changing the page size moves where page boundaries fall, and the resume
token handed back to callers is defined in terms of those boundaries.
Nothing covered that round trip - test_cloud_helpers exercises the result
list classes in isolation and test_aws_vm_types pages a memoised catalogue
- so add tests that page a collection with a small limit and assert every
object is visited exactly once and paging terminates.

CHANGELOG.rst

@@ -2,6 +2,18 @@
 ------------------
 
 ## Fixes
+* **Paginated AWS calls no longer use the caller's result limit as the
+  transport page size.** ``BotoEC2Service._get_paginated_results`` set
+  ``PaginationConfig={'MaxItems': limit, 'PageSize': limit}``, conflating how
+  many results the caller wants with how many the service returns per
+  request. Against a scan that matches sparsely that walks the collection in
+  tiny increments: the same filtered ``describe_images`` took 977.6s at a
+  page size of 5 and 10.0s at 1000, for one result either way. ``MaxItems``
+  still bounds what the caller receives; ``PageSize`` is now a full page,
+  clamped to whatever ceiling the service model declares for the operation
+  (``DescribeRouteTables`` allows 100 where most allow more, and exceeding a
+  ceiling is a hard ``InvalidParameterValue``). Since ``DEFAULT_RESULT_LIMIT``
+  is 50, every paginated AWS call was affected, not just filtered searches.
 * **``AWSImageService.find`` no longer scans every public image to run its
   tag search.** ``find(label=...)`` issues two ``describe_images`` calls, one
   filtered on ``name`` and one on ``tag:Name``, and neither was scoped by

cloudbridge/providers/aws/helpers.py

@@ -25,6 +25,14 @@ log = logging.getLogger(__name__)
 
 T = TypeVar("T")
 
+# How many items to ask the service for per request, independent of how many
+# the caller wants back. Small enough to sit inside every EC2 describe's
+# documented ceiling that the service model does not declare (DescribeVolumes
+# is the tightest of those, at 500), large enough that a sparse filtered scan
+# does not turn into thousands of round trips. Operations that do declare a
+# ceiling are clamped to it - see BotoEC2Service._page_size.
+DEFAULT_PAGE_SIZE = 500
+
 
 def trim_empty_params(params_dict: dict[str, Any]) -> dict[str, Any]:
     """
@@ -153,6 +161,40 @@ class BotoGenericService(object):
         else:
             return None
 
+    def _page_size(self, client: Any, list_op: str, limit: int) -> int:
+        """
+        Transport page size for a paginated call.
+
+        ``PageSize`` is how many items the service returns per request;
+        ``MaxItems`` is how many the caller asked for. Using the caller's
+        limit for both makes a small limit walk a large scan in tiny
+        increments: a filtered ``describe_images`` that takes 10.0s at a page
+        size of 1000 takes 977.6s at 5, for the same single result. Ask for a
+        full page regardless of the limit and let ``MaxItems`` do the
+        bounding - at worst one page more data is fetched than was wanted,
+        and a limit larger than a page is served over several requests.
+
+        EC2's per-operation ceilings differ - ``DescribeRouteTables`` allows
+        100 where most allow 1000 - and exceeding one is a hard
+        ``InvalidParameterValue`` rather than a clamp, so defer to whatever
+        the service model declares for this operation.
+        """
+        # Deliberately not max(limit, ...): a caller asking for more than a
+        # page still gets it, over several requests, rather than having an
+        # oversized limit pushed at a service that would reject it.
+        page_size = DEFAULT_PAGE_SIZE
+        api_name = client.meta.method_to_api_mapping.get(list_op)
+        if not api_name:
+            return page_size
+        input_shape = client.meta.service_model.operation_model(
+            api_name).input_shape
+        max_results = input_shape.members.get('MaxResults') if input_shape \
+            else None
+        # Not every operation declares bounds, even where the documentation
+        # gives them; fall back to a size known to be within all of them.
+        ceiling = max_results.metadata.get('max') if max_results else None
+        return min(page_size, ceiling) if ceiling else page_size
+
     def _get_list_operation(self) -> str:
         """
         This function discovers the list operation for a particular resource
@@ -195,7 +237,9 @@ class BotoGenericService(object):
         paginator = client.get_paginator(list_op)
         PaginationConfig: dict[str, Any] = {}
         if limit:
-            PaginationConfig = {'MaxItems': limit, 'PageSize': limit}
+            PaginationConfig = {
+                'MaxItems': limit,
+                'PageSize': self._page_size(client, list_op, limit)}
 
         if marker:
             PaginationConfig.update({'StartingToken': marker})

tests/test_aws_pagination.py

@@ -0,0 +1,110 @@
+"""
+Unit tests for ``BotoEC2Service`` server-side pagination.
+
+``_get_paginated_results`` hands boto3 a ``PaginationConfig`` built from two
+separate numbers: ``MaxItems``, how many results the caller asked for, and
+``PageSize``, how many the service returns per request. They used to be the
+same value, which made a small result limit walk a large scan in tiny
+increments - a filtered ``describe_images`` measured 977.6s at a page size of
+5 against 10.0s at 1000, for the same single result.
+
+Separating them moves where page boundaries fall, and the resume token
+handed back to callers is defined in terms of those boundaries. These tests
+pin the round trip: paging a collection with a small limit must still visit
+every object exactly once and terminate, whatever the transport page size.
+
+Exercised against moto so they run in CI without cloud credentials.
+"""
+import unittest
+
+from moto import mock_aws
+
+from cloudbridge.factory import CloudProviderFactory
+from cloudbridge.factory import ProviderList
+from cloudbridge.providers.aws.helpers import DEFAULT_PAGE_SIZE
+
+# More objects than one page at the result limit below, so paging is forced.
+OBJECT_COUNT = 7
+RESULT_LIMIT = 2
+
+
+class AWSPaginationTestCase(unittest.TestCase):
+
+    def setUp(self):
+        self.mock = mock_aws()
+        self.mock.start()
+        self.provider = CloudProviderFactory().create_provider(
+            ProviderList.AWS,
+            {'aws_access_key': 'a', 'aws_secret_key': 'b',
+             'aws_region_name': 'us-east-1',
+             'default_result_limit': RESULT_LIMIT})
+
+    def tearDown(self):
+        self.mock.stop()
+
+    def _create_volumes(self):
+        created = []
+        for i in range(OBJECT_COUNT):
+            vol = self.provider.storage.volumes.create(
+                'cb-page-%d' % i, 1)
+            created.append(vol.id)
+        return created
+
+    def test_paging_visits_every_object_exactly_once(self):
+        expected = self._create_volumes()
+
+        seen = []
+        page = self.provider.storage.volumes.list()
+        seen.extend(o.id for o in page)
+        pages = 1
+        while page.is_truncated:
+            page = self.provider.storage.volumes.list(marker=page.marker)
+            seen.extend(o.id for o in page)
+            pages += 1
+            self.assertLess(pages, OBJECT_COUNT + 5,
+                            "Paging failed to terminate; the resume token is "
+                            "probably not advancing.")
+
+        self.assertEqual(sorted(seen), sorted(expected),
+                         "Paging must visit every object exactly once.")
+        self.assertEqual(len(seen), len(set(seen)),
+                         "Paging returned duplicates across pages.")
+
+    def test_a_page_holds_no_more_than_the_result_limit(self):
+        # The caller's limit bounds what comes back, independently of how
+        # much was fetched per request to produce it.
+        self._create_volumes()
+        page = self.provider.storage.volumes.list()
+        self.assertLessEqual(len(page), RESULT_LIMIT)
+
+    def test_transport_page_size_is_independent_of_the_result_limit(self):
+        # The regression this guards: PageSize tracking the result limit is
+        # what made a sparse scan pathological.
+        # pylint:disable=protected-access
+        svc = self.provider.storage.volumes.svc
+        client = self.provider.ec2_conn.meta.client
+        page_size = svc._page_size(client, 'describe_volumes', RESULT_LIMIT)
+        self.assertEqual(page_size, DEFAULT_PAGE_SIZE)
+        self.assertNotEqual(page_size, RESULT_LIMIT)
+
+    def test_page_size_is_clamped_to_the_operations_ceiling(self):
+        # DescribeRouteTables allows 100 where most EC2 describes allow more,
+        # and exceeding a ceiling is a hard InvalidParameterValue.
+        # pylint:disable=protected-access
+        svc = self.provider.storage.volumes.svc
+        client = self.provider.ec2_conn.meta.client
+        self.assertEqual(
+            svc._page_size(client, 'describe_route_tables', RESULT_LIMIT),
+            100)
+
+    def test_page_size_survives_an_unknown_operation(self):
+        # pylint:disable=protected-access
+        svc = self.provider.storage.volumes.svc
+        client = self.provider.ec2_conn.meta.client
+        self.assertEqual(
+            svc._page_size(client, 'not_an_operation', RESULT_LIMIT),
+            DEFAULT_PAGE_SIZE)
+
+
+if __name__ == "__main__":
+    unittest.main()