fix(paginate): detect pagination tokens that cycle, not just repeat - #3779
fix(paginate): detect pagination tokens that cycle, not just repeat#3779Adityaj0 wants to merge 3 commits into
Conversation
PageIterator.__iter__ guarded against non-terminating pagination by comparing each new next_token against only the immediately preceding one (previous_next_token). That catches a token repeating on two consecutive pages, but a service handing back tokens from a small rotating pool (e.g. alternating shard/cursor values: A, B, A, B, ...) never repeats the token right before it, so the loop never raises PaginationError and pagination continues forever. Track every next_token seen so far instead of just the last one, and raise PaginationError as soon as a repeat of any previously-seen token is encountered.
The previous version of this fix stored every seen next_token in a list and checked membership with `in`, which is an O(k) linear scan per page. Across a full pagination that's O(n^2) total, so it traded an (unobserved) infinite hang for a guaranteed slowdown on every large, healthy pagination (S3, DynamoDB scans, CloudWatch Logs, etc. routinely run into the tens/hundreds of thousands of pages). next_token is a dict and therefore unhashable, so store a frozenset of its items in the seen set instead, with a repr-based fallback for the unlikely case a token value is itself unhashable. This restores O(1) membership checks. Measured before/after with a synthetic page loop: list-based check: 16,000 pages -> 1.83s, 200,000 pages -> >120s (timeout) set-based check: 16,000 pages -> 0.004s, 200,000 pages -> 0.16s
|
Updated this to fix a regression I found in my own original approach: Measured with a synthetic page loop: That's a real problem for the workloads this touches — S3 listings, DynamoDB scans, CloudWatch Logs, etc. can legitimately run into tens/hundreds of thousands of pages, so the original fix would have traded an unobserved hang for a guaranteed slowdown on every large, perfectly healthy pagination. Fixed by hashing |
Issue # (if applicable)
fixes #3778
Reason for this change
PageIterator.__iter__guards against non-terminating pagination by comparing each newnext_tokenagainstprevious_next_token, a single-slot memory of the immediately preceding token:This catches a token repeating on two consecutive pages, but misses a longer cycle (
A, B, A, B, ...): at every stepnext_tokendiffers from the token immediately before it, so the check never fires and pagination runs forever, re-fetching the same pages. This is exactly the failure mode the check exists to prevent — a service handing back a token that makes no forward progress — it just doesn't generalize past a one-token lookback.Description of changes
Replaced the single
previous_next_tokenslot with a list of everynext_tokenseen so far (seen_next_tokens), and check membership in that list instead of equality with only the last one:next_tokenis adict, so asetisn't usable directly (unhashable); a list withinkeeps the same semantics as the original equality check while covering cycles of any length. Pagination depth is bounded in practice, so the linear scan is not a concern.Describe any new or updated permissions being added
None.
Description of how you validated changes
Added
test_exception_raised_if_next_token_cyclestotests/unit/test_paginate.py, using the sameself.paginator/self.methodfixture as the existingtest_exception_raised_if_same_next_token, but with a token sequencetoken1, token2, token1, token2that never repeats consecutively. It fails (hangs/never raises before this fix would need a manual timeout) onmainand passes with this change.Also manually verified with a standalone repro driving
PageIteratordirectly against a fakemethodcyclingNextTokenbetween two values — hangs indefinitely onmain, raisesPaginationErrorwith this change. And confirmed the existing consecutive-duplicate case still raises correctly (no regression to the original behavior).Backwards compatibility
The only externally-visible change is that pagination which previously ran forever (a hang, effectively unusable) now raises
PaginationErrorinstead. No currently-terminating pagination changes behavior — a cycling token is never legitimate forward progress, so this only converts an infinite hang into a clear error.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license