kindling: proxyless as a raced bootstrap transport for config fetches - #16
Merged
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds bounded cached proxyless dialing, exposes it as a Kindling transport, adds candidate limits and cache controls, updates dependency wiring, and documents cold-search behavior. ChangesProxyless transport integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Kindling
participant ProxylessTransport
participant connect_cached
participant StrategyCache
participant flint_dial
Kindling->>ProxylessTransport: race transport connection
ProxylessTransport->>connect_cached: connect with space, host, port, cache, network
connect_cached->>StrategyCache: retry cached winner
connect_cached->>flint_dial: race bounded candidate destination dials
flint_dial-->>connect_cached: winning strategy and TLS stream
connect_cached-->>ProxylessTransport: return connection
ProxylessTransport-->>Kindling: return transport stream
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Chunk 5 of the proxyless port, and the use case that motivated it: reach a config endpoint directly — no proxy, no exit hop — by searching for an un-poisoned resolver plus opening-handshake shaping the local network does not block. `ProxylessTransport` implements `ConnectionTransport`, so Kindling races it exactly like the fronted and direct-h2 legs via `with_proxyless` / `push_proxyless`. It earns its place as a race member rather than a replacement: it costs no infrastructure and burns no fronting domains, so it is free to lose on an open network (where the direct dial wins) and survives where DNS is poisoned or the ClientHello is being classified. Adds `flint_proxyless::connect_cached`: search by attempting the *real* verified dial to the target and return the winning connection. For this use case the target host is its own sufficient oracle, so probe-then-dial would pay two handshakes per candidate to learn the same thing; the certificate check is unchanged, so the oracle is exactly as strong. `find`'s multi-domain form stays for the different question — "what works on this network" — where one accidentally-open path must not vouch for the rest. The interaction that needed real care is the cold start. A first connection on a new network is a search, not a dial, and can outlast `RaceOptions::attempt_timeout` (default 15s). Being killed mid-search is worse than merely slow: no winner is recorded, so the cache never populates and every later attempt fails identically — a slow cold start would present as a permanently broken transport. Two mitigations, both documented on the type: - `with_max_candidates` bounds the cold search. It trims **resolvers**, never wire plans, so capping cannot silently drop a shaping strategy the network specifically requires, and always leaves at least one resolver. - `warm` runs the search outside any race, so the first in-race connect is a single dial against a cached winner. Preferable to raising the timeout for every transport. Also re-exports the types this crate's own API cannot be spelled without (`Resolver`, `WirePlan`, `DialPolicy`, `BoxedTlsStream`) — a caller could not build a `Space` otherwise — and mirrors that in flint-kindling, matching how the fronted types are already surfaced. `async-trait` moves from dev-deps to deps, since implementing `#[async_trait] ConnectionTransport` needs the macro at build time. Gate: fmt, clippy --workspace --all-targets -D warnings, cargo test --workspace (flint-kindling 17 tests, 5 new; the existing fronted/race tests unaffected), cargo doc with zero warnings on both crates, and cargo check -p flint-kindling --features boring — the engine both legs of a proxyless dial need — all clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
myleshorton
force-pushed
the
fisk/kindling-proxyless
branch
from
July 29, 2026 20:43
2cafcf3 to
6495229
Compare
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 56 minutes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/flint-kindling/src/proxyless.rs`:
- Around line 74-99: Make `search_space` enforce `max_candidates` as a true
upper bound, including values below `self.space.wires.len()` and `max == 0`; do
not let retaining a resolver expand the result beyond the requested limit.
Update `with_max_candidates` or the candidate representation to reject/normalize
undersized limits or model the truncated candidates directly, and revise the
existing test around `search_space` to assert the candidate count never exceeds
the configured maximum.
In `@crates/flint-proxyless/src/lib.rs`:
- Around line 369-390: Apply the existing per-attempt timeout to the cached
`dial` call in the fast path and to each candidate `dial` inside the
`race_windowed` closure. Handle timeout errors like other dial failures: forget
the cached winner or reject the raced candidate so recovery and window refilling
continue.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da8a0d0b-2610-4fb2-81b2-c99ae582624c
📒 Files selected for processing (8)
crates/flint-dns/src/lib.rscrates/flint-dns/src/pool.rscrates/flint-kindling/Cargo.tomlcrates/flint-kindling/src/lib.rscrates/flint-kindling/src/proxyless.rscrates/flint-proxyless/src/cache.rscrates/flint-proxyless/src/lib.rsdocs/design.md
There was a problem hiding this comment.
Pull request overview
Adds “proxyless” as a first-class, raced Kindling transport for bootstrap/config fetches by integrating flint-proxyless into the Kindling transport set and introducing a direct-to-target cached connect path.
Changes:
- Add
flint_proxyless::connect_cached()to search the resolver×wire space by dialing the real target and returning the winning verified TLS stream. - Introduce
ProxylessTransportinflint-kindlingimplementingConnectionTransport, including cache warming and candidate-capping for cold starts. - Re-export proxyless-related API surface from
flint-proxylessandflint-kindling, and wire up theboringfeature/dependencies accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/design.md | Documents proxyless as a Kindling race member and the cold-start timeout/caching mitigations. |
| crates/flint-proxyless/src/lib.rs | Adds connect_cached and re-exports API types needed to consume the crate ergonomically. |
| crates/flint-kindling/src/proxyless.rs | New ProxylessTransport implementing ConnectionTransport, with max-candidate trimming and warm/forget helpers. |
| crates/flint-kindling/src/lib.rs | Exposes proxyless module/transport, adds with_proxyless/push_proxyless, and re-exports proxyless types. |
| crates/flint-kindling/Cargo.toml | Adds flint-proxyless + async-trait deps and extends boring feature to include proxyless. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design.md`:
- Around line 331-333: Update the documentation around `with_max_candidates` to
clarify that the cap limits resolver count rather than wire-plan count, is not
strict when all wire plans must be retained, and enforces a minimum of one
resolver.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 424af595-d9a5-45d3-828f-c32580b6536a
📒 Files selected for processing (5)
crates/flint-kindling/Cargo.tomlcrates/flint-kindling/src/lib.rscrates/flint-kindling/src/proxyless.rscrates/flint-proxyless/src/lib.rsdocs/design.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/flint-kindling/src/proxyless.rs
- crates/flint-kindling/src/lib.rs
…talling the race Review catches (CodeRabbit + Copilot, PR #16), both verified and both real. Three of the four comments converged on the same defect, which is a fair signal it mattered. **The cap was not a bound.** `search_space` kept at least one resolver but every wire plan, so `with_max_candidates(1)` against 3 plans produced 3 candidates. `max == 0` disabled the cap entirely. The knob exists to bound cold-search duration so it fits inside the caller's timeout, so a cap that silently exceeds itself defeats its only purpose. My own test asserted `resolvers.len() == 1` without checking the total, which is exactly how it slipped through. Now strict at every size. Resolvers are trimmed first, keeping every wire plan while the budget allows — resolver diversity is the cheaper thing to lose, since the pool is deliberately redundant while each shaping plan is a distinct evasion strategy — and only a cap below the plan count sacrifices plans, keeping the first (cheapest) ones because enumeration is wire-major. `0` is treated as `1`: a cap that searched nothing would switch the transport off rather than bound it. The replacement test asserts `len() <= max` for every cap from 1 to 20, plus the plan-sacrifice and zero cases. **Unbounded dials could stall the whole search.** Neither the cached fast path nor the raced candidates had a per-attempt deadline. `flint_dial::dial` does not bound its TCP connect and a censor's usual move is to blackhole rather than refuse, so a filtered address stalls instead of erroring — and under windowing a stalled candidate keeps its slot, so `race_windowed` never refills and the search waits on its least responsive candidate. That is the precise failure this PR's own docs warned about (killed mid-search → no winner recorded → cache never populates → every later attempt fails identically), left unfixed in the code. `flint-dns` already bounds each resolver attempt for the same stated reason, so this was an inconsistency as much as a bug. Adds a 5s per-attempt bound applied to the cached dial, each raced candidate, and `probe` — matching flint-dns's value and chosen so the arithmetic works out: at window 4, N candidates finish within `ceil(N / 4) × 5s`, so a cap of 8 lands under the 15s `RaceOptions` default. A timeout is treated as an ordinary losing candidate, and on the cached path it forgets the winner, so the bound is not defeated on the path that runs most often. Design §6.2 now documents the strict cap and that arithmetic, since it is the reason the cap has to be strict rather than advisory. I did not take CodeRabbit's suggested doc wording, which described the non-strict behaviour as intended — fixing the code was the better answer than documenting the wart. Gate: fmt, clippy --workspace --all-targets -D warnings, cargo test --workspace (flint-kindling 19 tests, flint-proxyless 13), cargo doc with zero warnings on both crates, and cargo check -p flint-kindling --features boring all clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 43 minutes. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/flint-proxyless/src/lib.rs:431
connect_cachedreports total failure asFindError::AllFailed, but that error’s Display text is specific tofind("failed to reach the test domains"). When a direct-to-host connect fails, callers will see a misleading message about test domains even though none were involved. Consider adding a separate error variant for connect failures (e.g.ConnectAllFailed { host, port, tried }) or makingAllFailed’s message target-agnostic and havingfind/connect_cachedwrap it with context.
Err(_errors) => Err(FindError::AllFailed { tried: total }),
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/flint-kindling/src/proxyless.rs`:
- Around line 142-147: Update connect in the proxyless connection flow so
stale-cache retry time is included within the outer RaceOptions::attempt_timeout
budget, preventing fallback attempts from being cancelled before caching a new
winner. Preserve the existing fallback behavior while ensuring the total cached
retry plus candidate attempts fits the deadline, and add a regression test
covering a stale cache followed by eight candidates.
In `@docs/design.md`:
- Around line 328-331: The cold-start explanation in the “with_max_candidates”
discussion overstates the consequence of a cancelled search. Replace the claim
that every later attempt fails identically with wording that later attempts may
repeat the same cold-search failure, while preserving the surrounding
cache-population and retry context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f8eb190-013b-43ed-90fe-8f417d865e55
📒 Files selected for processing (5)
crates/flint-kindling/Cargo.tomlcrates/flint-kindling/src/lib.rscrates/flint-kindling/src/proxyless.rscrates/flint-proxyless/src/lib.rsdocs/design.md
Review catches (CodeRabbit, PR #16 round 2). Both are about claims I made, and the first is a real arithmetic error rather than a wording nit. **The budget was computed for the wrong case.** I wrote that at window 4 a cap of 8 "lands under" the 15s `RaceOptions` default, from `ceil(N / 4) × 5s` = 10s. That holds only for a cold cache. On the path that actually runs most often — a cached winner that has since been blocked — the cached dial spends its own 5s attempt *before* the search starts, so the worst case is `5s + ceil(N / 4) × 5s` = 15s: exactly the outer deadline, cancellable at the boundary, and cancelled precisely when it would otherwise have cached a new winner. The recommendation is now a cap of **4** (10s total), with the stale-cache leg spelled out in the constant's docs, the transport's docs, and design §6.2 so the number is derivable rather than asserted. I did not implement the suggested concurrent fallback: retrying the cached winner while also racing the full space would double the traffic on every stale hit, on exactly the networks where being noisy is the risk. Sizing the cap correctly gets the same outcome for free. I also skipped the proposed stale-cache-plus-eight-candidate regression test — it would assert wall-clock timing with no fake clock and a real network, so it would be flaky and would not actually pin the property, which is arithmetic rather than behavioural. **"Every later attempt fails identically" overstated it.** A cancelled cold search leaves the cache empty, so the next attempt repeats the search — but whether that one succeeds depends on candidate timing, so the failure is not strictly permanent. Reworded to say later attempts repeat the same cold search rather than benefiting from it, and that the problem is being uninformed rather than doomed: a transport that keeps re-running a search it never finishes *looks* broken. Documentation only, no behaviour change. Gate: fmt, clippy --workspace --all-targets -D warnings, cargo test --workspace, and cargo doc with zero warnings on both crates. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
crates/flint-kindling/src/proxyless.rs:114
search_space()assumes the space always has at least one wire plan, but it only uses.max(1)to avoid division-by-zero. If a caller constructs aSpacewithwires.is_empty()(possible because fields are public /Space::default()), this trimming logic can return a still-emptySpace(len = 0) even when resolvers exist, making the transport fail withEmptySpacedespite a potentially valid configuration.
let mut trimmed = self.space.clone();
let wires = trimmed.wires.len().max(1);
if max < wires {
crates/flint-kindling/src/proxyless.rs:153
with_max_candidatesis documented as capping how many candidates a cold search will try, butconnect()callsconnect_cached()with the already-trimmedspace. That means the cached winner is also forced to be within the cap: if the previous winner’s resolver/wire is trimmed out,connect_cachedwill drop the cache entry without retrying it and immediately search the capped space instead. This can reduce success rate and contradicts the timeout arithmetic described in the type docs (which assumes a stale cached attempt always happens before the search). Consider separating “cache retry” from “capped search” (e.g., retry cached winner against the fullself.space, then only run the search against the capped space).
async fn connect(&self, host: &str) -> io::Result<Self::Stream> {
let space = self.search_space();
let (_strategy, stream) =
flint_proxyless::connect_cached(&space, host, self.port, &self.cache, &self.network)
.await
crates/flint-proxyless/src/lib.rs:406
connect_cachedintroduces substantial new behavior (cache retry + bounded per-attempt dial + windowed racing + winner recording), but it isn’t unit-tested in this crate. Existingfind_with/probeinjection makes the search logic testable without network/TLS;connect_cachedcurrently hard-wiresdial, so it can only be exercised via live networking. Consider adding an injectableconnect_cached_with(or similar) so the caching and fallback logic can be covered by deterministic unit tests.
pub async fn connect_cached(
space: &Space,
host: &str,
port: u16,
cache: &StrategyCache,
network: &str,
) -> Result<(Strategy, BoxedTlsStream), FindError> {
if space.is_empty() {
return Err(FindError::EmptySpace {
resolvers: space.resolvers.len(),
wires: space.wires.len(),
});
}
// Fast path: retry whatever last worked here, bounded so a since-blackholed winner cannot hang the
// caller. A failure is not fatal — it means this network has changed, so fall through to a fresh
// search rather than giving up. A timeout counts as a failure for exactly that reason: otherwise the
// bound would be defeated on the very path that runs most often.
if let Some(entry) = cache.winner(network) {
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.
Registers proxyless as a raced Kindling transport for config fetches — the use case that motivated this whole port.
ProxylessTransportimplementsConnectionTransport, so Kindling races it exactly like the fronted and direct-h2 legs viawith_proxyless/push_proxyless.Builds on #13 (resolver axis), #14 (the search), and #15 (authenticated dials).
Why it belongs in the race rather than replacing anything
It costs no infrastructure and burns no fronting domains, so it is free to lose. On an open network the direct dial wins outright; where DNS is poisoned or the ClientHello is being classified, this is the leg that survives.
flowchart LR K["Kindling::connect(host)"] --> R["race_boxed — window 4"] R --> A["direct-h2"] R --> B["fronted-tls"] R --> C["fronted-meek"] R --> D["proxyless"] subgraph PX["ProxylessTransport"] direction TB D1["cached winner for this network?"] D2["yes → single verified dial"] D3["no → race resolver × wire<br/>by dialing the real target"] D4["record winner"] D1 --> D2 D1 --> D3 --> D4 end D --> PX PX --> S["first verified TLS stream wins"] A --> S B --> S C --> Sconnect_cached: the target is its own oracleNew in
flint-proxyless. Rather than probe test domains and then dial the target, it searches by attempting the real verified dial and returns the winning connection.That halves the handshakes — probe-then-dial pays two per candidate to learn the same fact — and the oracle is exactly as strong, because the certificate still has to verify against the target host.
find's multi-domain form stays, because it answers a genuinely different question:find/find_cachedconnect_cachedCache discipline is unchanged from #14: a cached strategy is retried, never trusted, and dropped the moment it fails, so a strategy the censor has since caught cannot pin the client to a dead path.
The cold-start trap, and why there are two knobs
This is the part that needed real care. The first connection on a new network is a search, not a dial — up to
resolver × wirecandidates, each a DNS lookup plus a TLS handshake, four at a time. That can outlastRaceOptions::attempt_timeout(default 15s,flint-transport/src/lib.rs:60).Being killed mid-search is worse than merely slow: no winner is recorded, so the cache never populates and every later attempt fails identically. A slow cold start would present as a permanently broken transport, on exactly the slow, lossy networks this feature targets.
RaceOptionsis per-race, not per-transport, so the transport cannot simply ask for a longer budget. Two mitigations instead, both documented on the type:with_max_candidatesbounds the cold search. It trims resolvers, never wire plans — capping must not silently drop a shaping strategy the network specifically requires — and always leaves at least one resolver. Tests cover both properties.warmruns the search outside any race, so the first in-race connect is a single dial against a cached winner. Preferable to raising the timeout for every transport.API ergonomics
flint-proxylessnow re-exports the types its own API cannot be spelled without —Resolver,WirePlan,DialPolicy,BoxedTlsStream. A caller literally could not construct aSpacebefore this, sinceSpace::newtakesVec<Resolver>.flint-kindlingmirrors that, matching how the fronted types are already surfaced.async-traitmoves from dev-deps to deps: implementing#[async_trait] ConnectionTransportneeds the macro at build time, not just in tests.Verification
cargo fmt --all -- --check— cleancargo clippy --workspace --all-targets -- -D warnings— cleancargo test --workspace— clean; flint-kindling 17 tests, 5 new, and the existing fronted/race tests are unaffectedcargo doc— zero warnings on both cratescargo check -p flint-kindling --features boring— clean; the engine both legs of a proxyless dial needNew tests: the transport names itself for race diagnostics, an uncapped search uses the whole space, a cap trims resolvers while keeping every wire plan, a cap always leaves at least one resolver, and an empty space fails without touching the network.
Not covered
No live assertion that a proxyless config fetch actually completes end-to-end — that needs egress and the
boringengine, so it stays a#[ignore]d live test inflint-proxyless. The cold-start timeout interaction above is reasoned from the defaultRaceOptionsand mitigated by construction, but the real numbers (how long a cold search takes on a hostile network) are unknown until measured in the field; that measurement is what should set the recommendedmax_candidates.🤖 Generated with Claude Code
https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
Summary by CodeRabbit
proxylesstransport to Kindling withwith_proxyless/push_proxyless, including per-network strategy caching pluswarmandforget.connect_cachedin flint-proxyless for reconnects with automatic fallback to a fresh raced search.with_max_candidatescapping (including safe handling for0).boring,async-trait, andtokiofeatures.