The limit for client-initiated navigations becomes adjustable and is named after what it counts - #666
Open
aech wants to merge 1 commit into
Open
The limit for client-initiated navigations becomes adjustable and is named after what it counts#666aech wants to merge 1 commit into
aech wants to merge 1 commit into
Conversation
The loop in navigate_with_wait_post_inner limits how many documents a navigation chain may load. It counts navigations the page itself triggers after each load: assignments to location and form submissions, which CDP's Page.ClientNavigationReason calls scriptInitiated and formSubmission*. HTTP 3xx redirects never reach this loop. They are followed one layer down, in obscura-net, with their own limit and their own error. The message still read "Too many redirects (limit 10)", so a chain of client-initiated navigations looked like a redirect storm. This name sends an investigation to the wrong layer. It points at the redirect chain while the cause is a JavaScript loop. The limit was also a function-local constant with no way to raise it. The low default is right and stays 10, but an endpoint that chains longer than nine navigations for good reasons was simply unreachable. It is now a per-page value with a fallback to the environment, in the same form navigation_timeout already has: page.set_navigation_chain_limit(20) programmatic, takes precedence OBSCURA_NAV_CHAIN_LIMIT=20 in operation neither set unchanged default 10 The number counts documents, the first navigation included. So 10 allows the requested document and nine client-initiated navigations on top. The error message names this unit. The number is now something an operator acts on, and "limit 10" next to the word "navigations" would read as ten navigations and send them exactly one too low. Values below 1 are raised to 1 on both paths. A zero would let the loop run without a single iteration, the page would report success and have loaded nothing. Only an unreadable value from the environment falls back to the default. A 0 therefore means the same on both paths: load the first document and do not chain further.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is about
The loop in
navigate_with_wait_post_innerlimits how many documents a navigation chain may load. It counts navigations the page itself triggers after each load: assignments tolocationand form submissions. In the language of CDP these arescriptInitiatedandformSubmissionGet/formSubmissionPostfromPage.ClientNavigationReason.HTTP 3xx redirects never reach this loop. They are followed one layer down, in
obscura-net, with their own limit and their own error.Two things were wrong here.
The name pointed at the wrong layer
The message read
Too many redirects (limit 10). A chain of client-initiated navigations thus read as a redirect storm.The name costs time. I investigated an abort in an SAP login chain and measured the redirect chain, because the message spoke of redirects. The cause was one layer up, in a JavaScript loop.
On top of that, the name was assigned twice.
ObscuraNetError::TooManyRedirectsmeans real HTTP redirects. After this change, "Too many redirects" means only that across the whole tree.The limit could not be raised
const REDIRECT_LIMIT: usize = 10stood inside the function. No CDP parameter, no env, no setter.The low default is right and stays. It is what stops a page that resets
locationon every load. What was wrong is only that an endpoint that chains longer for good reasons remained unreachable.What changes
The limit gets the shape
navigation_timeoutalready has: a per-page field, a setter, a getter, and a fallback to the environment.The number means how many documents a chain may load, the first one included. So the default of 10 allows the requested document and nine client-initiated navigations on top. That is exactly the previous behaviour.
The message names this unit explicitly, because the number is now something an operator acts on. If the word "navigations" had only
limit 10next to it, one would read ten navigations and, when eleven are needed, set the limit to eleven, exactly one too low.Values below 1 are raised to 1 on both paths. A zero would let the loop run without a single iteration, the page would report success and have loaded nothing. Only an unreadable value from the environment falls back to the default.
OBSCURA_NAV_CHAIN_LIMIT=0andset_navigation_chain_limit(0)therefore mean the same: load the first document and do not chain further.The new switch stands in
docs/Environment-variables.mdnext to its twinOBSCURA_NAV_TIMEOUT_MS.Deliberately not included: a CDP parameter on
Page.navigateand a switch on the CLI.navigation_timeoutmanages without both, and a non-standard parameter on a CDP command would be the larger intervention.Tests
Seven new tests in
crates/obscura-browser/src/page.rs. Three check the pure function that reads the environment, as the tests fornavigation_timeoutdemonstrate. Four drive a real chain against a local server whose pages setlocation.href.The four chain tests always set their limit themselves. If they inherited it, the page would read
OBSCURA_NAV_CHAIN_LIMITfrom the process environment, and anyone running the suite with exactly the switch this PR introduces would see it fail through no fault of the code. Measured: withOBSCURA_NAV_CHAIN_LIMITat 20, 3, 0, and an unreadable value, all seven stay green.Seven mutations, each caught by exactly the test that means it, plus two unmutated runs as a baseline, one of them with the environment variable set:
cargo nextest run --features render -p obscura-browserruns green with 70 tests,cargo check --workspaceand--features render,stealthwithout errors.Note on scope
The rename changes a public variant of
PageErrorand with it the string a CDP client sees.domains/page.rspasses the error through viamap_err(|e| e.to_string()). Exactly this string is the subject of the change. No other place in the tree reads the variant or the text, checked across the whole tree.Part of #664.