fix: rotate the LinkContentFetcher user agent per fetch, not per component - #12364
Conversation
…onent `run()` fetches URLs concurrently through a ThreadPoolExecutor and `run_async()` gathers them, but the rotation cursor lived on the component as `current_user_agent_idx`. Every in-flight request read and wrote the same counter: a retry triggered by one URL rotated the user agent for all the others, and each fetch that finished reset the counter to 0 underneath the requests still running. With several URLs retrying at once, the retries mostly went out with the un-rotated user agent — the feature silently did not do what it documents. Give each fetch its own cursor: a local in `_get_response` that the tenacity `after` callback advances, and a local in `_get_response_async`. `_get_headers` now takes the user agent for the attempt instead of reading component state. `current_user_agent_idx` and `_switch_user_agent` were that shared state and are gone. Neither is part of the documented API, and neither survives serialization. Fixes deepset-ai#12287 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Hi @pcbeingused333, thanks for your interest in contributing to Haystack! 🙏 This is an automated message to help us keep the review queue healthy. |
|
Thanks bot — both points are fair, and the duplicate one is on me: I should have checked the issue for a linked PR before opening this. Apologies for the noise. On the overlap with #12289: it does address the same root cause, and @iamyuviii got there first. Two differences that may or may not matter to you:
I have no attachment to whose branch lands. Happy to do whichever you prefer:
@iamyuviii — no intention of stepping on your PR. If you'd rather take the test across yourself, please do, it's yours. On the second warning: understood, I won't open anything further until #12358 and #12359 have been looked at. |
|
@davidsbatista is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
davidsbatista
left a comment
davidsbatista
left a comment
There was a problem hiding this comment.
looks good - I've adjusted the release notes, one must use double ticks
2 participants
Related Issues
Proposed Changes:
The rotation cursor lived on the component as
current_user_agent_idx, butrun()fetches URLs concurrently through aThreadPoolExecutorandrun_async()gathers them. Every in-flight request read and wrote that one counter, which produced two overlapping failures:finally: self.current_user_agent_idx = 0in_fetch/_fetch_asyncreset the counter underneath the requests still running.With several URLs retrying at once the retries mostly went out with the un-rotated user agent, so the feature silently did not do what it documents.
The fix gives each fetch its own cursor:
_get_responsebecomes a real method that builds its tenacity wrapper per call, withuser_agent_idxas a local that theaftercallback advances vianonlocal. It used to be a closure built once in__init__and stored onself, which is what forced the state to be shared._get_response_asynckeeps its own local cursor in the retry loop._get_headersnow takes the user agent for the attempt instead of reading component state.current_user_agent_idxand_switch_user_agentwere that shared state, so they are gone. Neither is documented API, and the counter never survived serialization anyway: the component has no customto_dict/from_dictand the default path serializes__init__parameters, which the counter was not — a round-tripped component always came back at0.No behaviour change for the single-URL case, which is why this was easy to miss.
How did you test it?
test_user_agent_rotation_is_independent_per_urlfetches 8 URLs with 4 user agents, fails every URL exactly once, and asserts that all 8 succeed onuser_agents[1]— each URL rotated exactly once, on its own.I checked the test is not vacuous: with
haystack/components/fetchers/link_content.pyreverted tomainand only the test kept, it fails with every URL reportingua-0instead ofua-1. That is the bug itself — the retries going out un-rotated — rather than an incidental assertion mismatch.Full file: 32 passed.
hatch run test:typesclean across 427 source files,hatch run fmt-checkclean.Notes for the reviewer
Two things worth a look:
The tenacity wrapper is now rebuilt per call instead of once in
__init__. That is deliberate — the cursor has to be per-fetch, and the wrapper closes over it — but it is the one real cost of this approach. It is a closure construction against a network round trip, so it did not seem worth optimising; happy to hoist the static parts if you would rather._get_headerschanged signature (() ->becomes(user_agent: str) ->). It is private and has one caller per path, but flagging it in case something downstream reaches for it.The test patches
wait_exponentialat module level after constructing the fetcher, which only works because the decorator is now built at call time. That is intentional coupling to the new design — under the old code there was no way to skip the backoff without patching the instance attribute.Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.