fix(memory-storage): stop re-adds from duplicating and resetting pending requests - #2074
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes the in-memory RequestQueue client by eliminating the O(n) deque.remove scan when re-adding already-pending requests with forefront=True, replacing it with an appendleft + “lazy stale entry” skipping approach in fetch_next_request() / is_empty().
Changes:
- Reworked
MemoryRequestQueueClient.add_batch_of_requests()so forefront re-adds of pending requests no longer scan/remove from the pendingdeque. - Added stale-entry skipping in
fetch_next_request()and front-pruning inis_empty()to support the new “tombstone” behavior. - Added regression tests to guard the performance fix and ordering/dedup semantics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/crawlee/storage_clients/_memory/_request_queue_client.py |
Implements tombstone-based forefront repositioning and stale-entry skipping/pruning logic. |
tests/unit/storage_clients/_memory/test_memory_rq_client.py |
Adds regression tests validating no deque.remove usage and preserves order/dedup semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d2ca2a0 to
02429c4
Compare
`MemoryRequestQueueClient.add_batch_of_requests` repositioned an already-pending request to the forefront with `deque.remove`, a linear scan of the whole pending deque. Re-adding a batch of K already-pending requests to a deque of N was O(K*N). Forefront re-adds are a real path: tiered-proxy retries set `request.forefront = True` and re-enqueue while the request is still pending. The forefront reposition now just `appendleft`s the request and leaves the old entry in place. The new object is registered in `_requests_by_unique_key`, superseding the old one, and `fetch_next_request` and `is_empty` skip entries whose object is no longer the one registered for their unique key. Because a forefront reposition always enqueues the live copy ahead of the entry it supersedes, `is_empty` can safely prune stale entries from the front. Only forefront re-adds create such tombstones. A regular re-add of an already-pending request leaves both the deque entry and its registered object untouched, so the identity check never misfires and no request is dropped. Forefront-LIFO / regular-FIFO order and deduplication are unchanged. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Replace lazy deque tombstones with an ordered mapping so forefront repositioning remains constant-time without stale-entry growth. Keep request updates on regular re-adds, update reclaimed request instances in the index, and leave is_empty side-effect free. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
02429c4 to
009c2dd
Compare
|
pushed a revision addressing the latest review and ci feedback. |
|
@anxkhn Thank you. For future reviews, I'd really appreciate it if you could address all the feedback in a single follow-up commit, rather than splitting the changes across multiple commits, merging them into earlier commits, and force-pushing it. Your current approach makes the changes really hard to review. |
|
understood, i’ll keep future review fixes in a single follow-up commit without rewriting earlier commits. |
vdusek
left a comment
There was a problem hiding this comment.
Thanks. It's much cleaner after the rewrite. Two things left.
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
|
@vdusek addressed both new threads, rebased, and re-requesting review. thank you. |
This comment was marked as outdated.
This comment was marked as outdated.
|
@Mantisus could you please check this as well? |
OrderedDictkeyed by unique key instead of adeque. Repositioning a request to the forefront is now O(1) instead of an O(n)deque.remove()scan, so re-adding K already-pending requests is O(K) instead of O(K*N) — measured 22.4s → 0.004s for 2000 forefront re-adds into a 20k-request queue. The scan only triggered for requests already waiting in the queue, which is whatadd_requests(..., forefront=True)hits when a handler re-discovers URLs that are still pending.pending_request_countat -15. Duplicates are now impossible by construction.retry_count(a request could retry forever),label(dispatch to a different handler) anduser_data. On a regular re-add it left the two out of sync, soget_requestandfetch_next_requestdisagreed. This restores the documented contract ("Duplicates will be identified but not re-added to the queue") and matches the file-system, SQL and Redis clients, as well as@crawlee/memory-storagein Crawlee for JS.get_requestconsistent after a reclaim: it now returns the reclaimed object, the same onefetch_next_requesthands back.datetime.now()calls on a batch add.✍️ Drafted by Claude Code