Fix the picker timeout, loader requests cancelling each other, and unreadable rejections - #79
Merged
Merged
Conversation
Clicking "Pick credential…" without a sign-in window already open reported "timed out reading the sign-in window", while holding a perfectly good session cookie. Opening the window first and then picking worked, which is the tell. `read_session` starts by reading `localStorage`, which means evaluating script in the page — and that fails until the page has loaded. The `?` propagated the failure, throwing away the cookie sweep with it. But cookies come from the Rust side rather than from the page: they need no script and are readable the moment the window exists. The one part that was working was being discarded because a different part wasn't ready. So the eval failure is now held rather than propagated, and only reported if the cookie sweep also comes back empty — nothing to show is still worth saying, and it was the honest answer all along for that case only. The retry loop needed a matching change, or this would have traded one bug for another. It stopped at the first non-empty read; with cookies arriving before the page does, that would hand back a cookies-only snapshot the instant the window opened, and miss a credential kept in `localStorage` entirely. It now settles on a *complete* read and keeps a cookies-only one as the fallback, so neither source loses to the other's timing. Also drops the worst case from ~43s (eight attempts each waiting out a 5s eval timeout) to returning as soon as the page answers.
Two more things from the same investigation, both of which were making the real problem harder to see. "request cancelled" — `HttpState` keys in-flight requests by `RequestSpec::id`, and `HashMap::insert` under an existing key drops the old cancel sender. Dropping it is exactly what the receiver is waiting for, so the earlier request returns `Cancelled`. Every loader request for a section shared one id, so any two that overlapped killed each other: a "Fetch a sample" while a background refresh was in flight, and which one died came down to timing. Nothing cancels a loader request by id — only the window does that, for requests a person sent — so the id had no reason to be predictable in the first place. Second, a rejected manifest now reports where the response came from when it left the origin the request was aimed at. Compared by origin rather than by whole URL: following a redirect within the same host is ordinary and says nothing, while leaving the host is what silently sheds the credential (`reqwest_strips` lists `cookie` and `authorization`, so those ride reqwest's own policy, which drops them cross-host). "403" and "403, having ended up on a different host" are different problems wearing the same status, and only one of them is the API's fault. `LoaderResponse` carries the resolved request URL for that comparison, because the loader itself only ever holds the relative form. The sidebar's loader error is now selectable and has a Copy button. The first thing anyone does with an error they can't act on is try to send it to someone who can, and "I'm not sure how to give you the exact error" is a bug report about this app, not about the person writing it.
Merged
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.
Three things from investigating a
403on a Better Auth session cookie. None is the 403 itself — but two of them were making it harder to see, and the third is a bug found on the way.1. "Pick credential…" timed out with a good cookie in hand
read_sessionreadslocalStoragefirst, which needs script evaluation, which fails until the page has loaded. The?propagated that failure and threw away the cookie sweep with it — even though cookies come from the Rust side, need no script, and are readable the moment the window exists.So clicking Pick credential… from a closed window reported "timed out reading the sign-in window"; opening the window first, then picking, worked.
The eval failure is now held and reported only if the cookie sweep also comes back empty. The retry loop in
snapshot()needed a matching change or this would trade one bug for another: it stopped at the first non-empty read, and since cookies arrive before the page does, that would hand back a cookies-only snapshot the instant the window opened and miss a credential kept inlocalStorage. It now settles on a complete read, keeping a cookies-only one as the fallback.Worst case drops from ~43s (eight attempts each waiting out a 5s eval timeout) to as soon as the page answers.
2. "request cancelled"
HttpStatekeys in-flight requests byRequestSpec::id, andHashMap::insertunder an existing key drops the old cancel sender — which is precisely what the receiver is waiting on. Every loader request for a section shared one id, so any two that overlapped killed each other: a Fetch a sample while a background refresh was out, with timing deciding which died.Nothing cancels a loader request by id — only the window does that, for requests a person sent — so it had no reason to be predictable. Each now gets a unique handle.
3. A rejection that came from a different host now says so
reqwest_strips(http.rs:357) listscookieandauthorization, so those credentials ride reqwest's own redirect policy — which drops them on a cross-host hop. A403after being redirected off-origin is a completely different problem from a403from your own API, and they looked identical.LoaderResponsenow carries the resolved request URL alongside the final one (the loader itself only holds the relative form), and a rejection reports the origin it ended up on. Compared by origin, not whole URL: a same-host redirect is ordinary and says nothing.The sidebar's loader error is also selectable with a Copy button — "I'm not sure how to give you the exact error" is a bug report about this app, not about the person writing it.
Testing
cargo fmt --check,cargo clippy --no-default-features --all-targets -- -D warnings,cargo test(133 passed, 3 new: cross-host redirect note, same-origin redirect stays quiet, unique cancel handles),pnpm check(0 errors),pnpm test:e2e(316 passed).