Skip to content

Commit b7ba52d

Browse files
authored
test: fix flaky request ordering tests in shared RQ access mode (#931)
### Description - `test_request_ordering_with_mixed_operations[shared]` flaked on master ([CI run](https://github.com/apify/apify-sdk-python/actions/runs/26878160055/job/79270923478)) with the reclaimed forefront request being fetched before the newly added forefront request. - In the shared request queue access mode, the relative order of requests added or reclaimed close together is not guaranteed due to the API propagation delay (see #808). The order-sensitive tests (`test_request_ordering_with_mixed_operations`, `test_forefront_requests_ordering`) now only verify that forefront requests come before regular ones in shared mode, asserting membership instead of exact positions. The strict ordering assertions are kept for the deterministic single access mode. - Additionally, based on review findings: - In shared mode, the tests now wait for forefront operations to propagate to the queue head before draining the queue, so the forefront-before-regular boundary assertions are not exposed to the same propagation delay either. - `test_request_reclaim_with_forefront` had the identical reclaim-to-forefront-then-fetch strict ordering assertion and gets the same propagation wait (its assertion stays strict, since after propagation it is deterministic). - `test_forefront_requests_ordering` now asserts `len(fetched_urls) == 5`, so the set-based shared-mode assertions cannot mask duplicate or extra fetches. - The propagation-delay rationale is documented once in the module-level note; the inline comments point at it. - Verified locally: 10/10 consecutive runs of each affected `[shared]` variant passed against the real API (before the follow-up commit, which only makes the assertions stricter/more robust).
1 parent 371e96a commit b7ba52d

1 file changed

Lines changed: 70 additions & 18 deletions

File tree

tests/integration/test_request_queue.py

Lines changed: 70 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,12 @@
2626
from apify.storage_clients._apify._models import ApifyRequestQueueMetadata
2727

2828
# In shared mode, there is a propagation delay between operations so we use test helper
29-
# `call_with_exp_backoff` for exponential backoff. See https://github.com/apify/apify-sdk-python/issues/808.
29+
# `call_with_exp_backoff` for exponential backoff. The delay also means that the relative order of requests
30+
# added or reclaimed close together is not guaranteed, so order-sensitive tests wait for propagation and
31+
# relax exact-order assertions in shared mode. See https://github.com/apify/apify-sdk-python/issues/808.
32+
33+
# How long to wait in shared mode for forefront operations to propagate to the queue head before fetching.
34+
_SHARED_MODE_PROPAGATION_DELAY = 10
3035

3136

3237
async def test_add_and_fetch_requests(
@@ -162,6 +167,11 @@ async def test_forefront_requests_ordering(
162167
total_count = await rq.get_total_count()
163168
Actor.log.info(f'Added 2 forefront requests, total in queue: {total_count}')
164169

170+
if rq_access_mode == 'shared':
171+
# Wait for the forefront requests to propagate to the queue head, so that no regular request is fetched
172+
# before them (see the note on shared-mode propagation delay at the top of this module).
173+
await asyncio.sleep(_SHARED_MODE_PROPAGATION_DELAY)
174+
165175
# Fetch requests and verify order.
166176
fetched_urls = []
167177
while next_request := await call_with_exp_backoff(rq.fetch_next_request, rq_access_mode=rq_access_mode):
@@ -170,17 +180,30 @@ async def test_forefront_requests_ordering(
170180
await rq.mark_request_as_handled(next_request)
171181

172182
# Forefront requests should come first (in reverse order of addition)
173-
expected_order = [
174-
'https://example.com/priority2',
175-
'https://example.com/priority1',
176-
'https://example.com/1',
177-
'https://example.com/2',
178-
'https://example.com/3',
179-
]
180-
assert fetched_urls == expected_order, (
181-
f'fetched_urls={fetched_urls}',
182-
f'expected_order={expected_order}',
183-
)
183+
assert len(fetched_urls) == 5, f'fetched_urls={fetched_urls}'
184+
if rq_access_mode == 'shared':
185+
# In shared mode, the relative order of requests added close together is not guaranteed (see the note at
186+
# the top of this module). Only verify that the forefront requests come before the regular ones.
187+
assert set(fetched_urls[:2]) == {'https://example.com/priority1', 'https://example.com/priority2'}, (
188+
f'fetched_urls={fetched_urls}'
189+
)
190+
assert set(fetched_urls[2:]) == {
191+
'https://example.com/1',
192+
'https://example.com/2',
193+
'https://example.com/3',
194+
}, f'fetched_urls={fetched_urls}'
195+
else:
196+
expected_order = [
197+
'https://example.com/priority2',
198+
'https://example.com/priority1',
199+
'https://example.com/1',
200+
'https://example.com/2',
201+
'https://example.com/3',
202+
]
203+
assert fetched_urls == expected_order, (
204+
f'fetched_urls={fetched_urls}',
205+
f'expected_order={expected_order}',
206+
)
184207

185208

186209
async def test_request_unique_key_behavior(
@@ -301,6 +324,11 @@ async def test_request_reclaim_with_forefront(
301324
await rq.reclaim_request(first_request, forefront=True)
302325
Actor.log.info('Request reclaimed to forefront')
303326

327+
if rq_access_mode == 'shared':
328+
# Wait for the reclaimed request to propagate to the queue head, so that no regular request is fetched
329+
# before it (see the note on shared-mode propagation delay at the top of this module).
330+
await asyncio.sleep(_SHARED_MODE_PROPAGATION_DELAY)
331+
304332
# The reclaimed request should be fetched first again. A reclaimed request may take a moment to reappear
305333
# at the queue head (eventually-consistent API state), even in single mode, so poll until it does.
306334
next_request = await poll_until_condition(rq.fetch_next_request, lambda result: result is not None)
@@ -797,7 +825,13 @@ async def test_request_ordering_with_mixed_operations(
797825
# Fetch one and reclaim to forefront.
798826
request1 = await call_with_exp_backoff(rq.fetch_next_request, rq_access_mode=rq_access_mode)
799827
assert request1 is not None, f'request1={request1}'
800-
assert request1.url == 'https://example.com/1', f'request1.url={request1.url}'
828+
if rq_access_mode == 'shared':
829+
# In shared mode, the relative order of requests added close together is not guaranteed (see the note at
830+
# the top of this module), so the first fetched request may be either of the two initial ones.
831+
assert request1.url in {'https://example.com/1', 'https://example.com/2'}, f'request1.url={request1.url}'
832+
else:
833+
assert request1.url == 'https://example.com/1', f'request1.url={request1.url}'
834+
remaining_url = ({'https://example.com/1', 'https://example.com/2'} - {request1.url}).pop()
801835
Actor.log.info(f'Fetched request: {request1.url}')
802836

803837
await rq.reclaim_request(request1, forefront=True)
@@ -807,6 +841,11 @@ async def test_request_ordering_with_mixed_operations(
807841
await rq.add_request('https://example.com/priority', forefront=True)
808842
Actor.log.info('Added new forefront request')
809843

844+
if rq_access_mode == 'shared':
845+
# Wait for the forefront operations to propagate to the queue head, so that the regular request is not
846+
# fetched before them (see the note on shared-mode propagation delay at the top of this module).
847+
await asyncio.sleep(_SHARED_MODE_PROPAGATION_DELAY)
848+
810849
# Fetch all requests and verify forefront behavior.
811850
urls_ordered = list[str]()
812851
while next_request := await call_with_exp_backoff(rq.fetch_next_request, rq_access_mode=rq_access_mode):
@@ -818,12 +857,25 @@ async def test_request_ordering_with_mixed_operations(
818857
# Verify that we got all 3 requests
819858
assert len(urls_ordered) == 3, f'len(urls_ordered)={len(urls_ordered)}'
820859

821-
assert urls_ordered[0] == 'https://example.com/priority', f'urls_ordered[0]={urls_ordered[0]}'
822-
assert urls_ordered[1] == request1.url, (
823-
f'urls_ordered[1]={urls_ordered[1]}',
824-
f'request1.url={request1.url}',
860+
if rq_access_mode == 'shared':
861+
# In shared mode, the relative order of two forefront operations performed close together (the reclaim of
862+
# request1 and the add of the priority request) is not guaranteed (see the note at the top of this
863+
# module). Only verify that both forefront requests come before the regular one.
864+
assert set(urls_ordered[:2]) == {'https://example.com/priority', request1.url}, (
865+
f'urls_ordered={urls_ordered}',
866+
f'request1.url={request1.url}',
867+
)
868+
else:
869+
assert urls_ordered[0] == 'https://example.com/priority', f'urls_ordered[0]={urls_ordered[0]}'
870+
assert urls_ordered[1] == request1.url, (
871+
f'urls_ordered[1]={urls_ordered[1]}',
872+
f'request1.url={request1.url}',
873+
)
874+
875+
assert urls_ordered[2] == remaining_url, (
876+
f'urls_ordered[2]={urls_ordered[2]}',
877+
f'remaining_url={remaining_url}',
825878
)
826-
assert urls_ordered[2] == 'https://example.com/2', f'urls_ordered[2]={urls_ordered[2]}'
827879
Actor.log.info('Request ordering verified successfully')
828880

829881

0 commit comments

Comments
 (0)