lore-transport: Happy eyeballs strategy for QUIC fallback connections across resolved addresses - #28
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the QUIC client connection logic to use a Happy Eyeballs-style strategy when a hostname resolves to multiple socket addresses, avoiding long stalls on an unreachable first address while preserving existing per-address configuration and error behavior.
Changes:
- Collect resolved socket addresses and attempt connections concurrently with a fixed stagger (250ms).
- Introduce
connect_happy_eyeballsto schedule attempts and return the first successful connection. - Add unit tests covering stalled-first-attempt fallback, immediate advance on failure, no fallback after first success, and all-fail behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ragnarula
left a comment
There was a problem hiding this comment.
This is neat! I wasn't aware of the happy eyeballs technique before. Left some minor comments to address.
e8bbffe to
fe91bcf
Compare
|
This looks like a pretty good addition now! However, please remove the additional |
|
@mjansson Ok, I removed the unnecessary attribution, should I now squash the commits, or you'll do it on your end? |
Doesn't matter, but all the commits need to be with the DCO sign-off - the |
ee2c100 to
f457a07
Compare
|
@mjansson Done, one commit, signed-off as per DCO. |
f457a07 to
c85f9c1
Compare
c85f9c1 to
c552b7b
Compare
|
We'll get this merged once the intake and attribution process is in place, ideally early next week. |
|
Import failed: GH reports conflict: PR not mergeable |
|
Import failed: GH reports conflict: PR not mergeable |
|
@ahaczewski can you update to main to unblock the intake process? We cannot handle conflicts yet |
Signed-off-by: Andrzej Haczewski <ahaczewski@gmail.com>
c552b7b to
5afffc2
Compare
|
Imported as Lore CR-294. |
|
Closed by mirrored commit ac86246. |
… across resolved addresses Use a Happy Eyeballs strategy (see [RFC8305](https://www.rfc-editor.org/info/rfc8305/)) when the QUIC client connects to a hostname that resolves to multiple socket addresses. The client now: - preserves the resolver's preferred first address family; - interleaves IPv6 and IPv4 candidates while preserving relative order within each family; - starts the first address immediately; - starts subsequent attempts with a 250-millisecond stagger; - advances immediately when all active attempts fail; - limits concurrent attempts to 10; - returns the first successful connection and cancels the remaining attempts. The existing per-address endpoint setup and final error behavior remain unchanged. ## Why The previous implementation awaited QUIC addresses sequentially. If `localhost` resolved to `::1` before `127.0.0.1` while `loreserver` listened on its default IPv4 address, the IPv6 attempt consumed the full 30-second QUIC idle timeout before IPv4 was attempted. This caused commands such as `lore history` to take about 30 seconds because repository initialization had started a background QUIC pre-warm, even if not required. Before: ```text remote_url = "lore://localhost:41337" real 30.05 ``` Using `127.0.0.1` directly completed in `0.08s`, confirming that address fallback was responsible for the delay. Closes #27 ## Testing Added unit coverage for: - IPv6-first and IPv4-first address-family interleaving; - starting a fallback while the first attempt remains stalled; - advancing immediately after an attempt fails; - avoiding fallback when the first address succeeds; - returning failure when every address fails; - bounding the number of concurrent attempts. Verification performed: ```console cargo +nightly fmt --all -- --check cargo test -p lore-transport cargo clippy -p lore-transport --all-targets -- -D warnings --no-deps ``` All 28 `lore-transport` tests pass. The original reproduction using `lore://localhost:41337` now completes in: ```text real 0.38 ``` A remote immutable-store query also demonstrated the complete fallback: ```text QUIC connecting to localhost at [::1]:41337 QUIC connecting to localhost at 127.0.0.1:41337 Success QUIC connecting to 127.0.0.1:41337 QUIC connection ... complete in 254ms real 0.27 ``` ### Note on tests The `lore-transport/src/quic/client.rs` file did not contain any test previously, and if they are wrongly placed, let me know and I'll make amendments. ## Alternative approach The alternative would be to prevent QUIC from connecting for commands that do not require QUIC connection. I rejected that solution due to sweeping changes required. ## AI disclosure OpenAI GPT-5.5 was used to improve upon the first version of this PR. CoPilot-generated code review snippets were used to amend the PR with suggested fixes. ``` Imported-PR: #28 Imported-From: 5afffc2 Imported-Base: 6f14447 Imported-Merge: 8583075 Imported-Author: Andrzej Haczewski (ahaczewski) Signed-off-by: Andrzej Haczewski <ahaczewski@gmail.com> GH-URL: #28 ``` Lore-RevId: 482 Lore-Signature: a625a714b2e4ada93734f6a98bc5c071191943f6c107e051960c38ea364bf5c7
Use a Happy Eyeballs strategy (see RFC8305) when the QUIC client connects
to a hostname that resolves to multiple socket addresses.
The client now:
The existing per-address endpoint setup and final error behavior remain unchanged.
Why
The previous implementation awaited QUIC addresses sequentially. If
localhostresolved to::1before127.0.0.1whileloreserverlistened onits default IPv4 address, the IPv6 attempt consumed the full 30-second QUIC idle
timeout before IPv4 was attempted.
This caused commands such as
lore historyto take about 30seconds because repository initialization had started a background QUIC
pre-warm, even if not required.
Before:
Using
127.0.0.1directly completed in0.08s, confirming that addressfallback was responsible for the delay.
Closes #27
Testing
Added unit coverage for:
Verification performed:
All 28
lore-transporttests pass.The original reproduction using
lore://localhost:41337now completes in:A remote immutable-store query also demonstrated the complete fallback:
Note on tests
The
lore-transport/src/quic/client.rsfile did not contain any test previously,and if they are wrongly placed, let me know and I'll make amendments.
Alternative approach
The alternative would be to prevent QUIC from connecting for commands
that do not require QUIC connection. I rejected that solution due to sweeping
changes required.
AI disclosure
OpenAI GPT-5.5 was used to improve upon the first version of this PR.
CoPilot-generated code review snippets were used to amend the PR with
suggested fixes.