feat(stateless-r2): custom-domain R2 target with HTTP/2, keeping the raw S3 endpoint path - #187
Merged
Merged
Conversation
…raw S3 endpoint path Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d Access id, retry/redirect/validator-adapter tests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude review status
🛠️ Review did not finish Attempted This round did not publish: MODEL_ACTION_FAILED in phase review_retry. Anything listed below is from the last round that did. Re-run the workflow or push a new commit to try again. |
|
Label check: this PR currently has no labels applied. Based on the diff and title ( |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0513fe05f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
flyq
marked this pull request as draft
August 15, 2026 07:45
The probe is the tool that validates a Cloudflare custom domain before any client is cut over to it, so its four defects all landed on the acceptance path itself: - `resolve_hashes` advanced `n` by the last element of a range it never checked was non-empty. A gateway answering a batch with fewer items than requested walked `n` past the end and panicked on `chunk[chunk.len() - 1]`. The loop is now driven by the block range, the gateway-echoed batch id is bounds-checked instead of indexed raw, results are deduped, and a short resolve warns rather than silently shrinking every rung. - The ALPN probe used a redirect-following client, so on an Access-protected domain it followed the 302 to the login page and reported *that* host's negotiated version — the one measurement the probe exists to make. - Latency was measured from task spawn, so it included the probe's own wait on the concurrency semaphore. Tighter rungs therefore looked slower than looser ones on an identical link, which inverts the reading used to size `--r2-max-concurrent-requests`. Queue wait is now subtracted (the fetcher already reports it) and shown as its own column. - Every `R2GetError` was discarded into a counter, leaving a rejected token, an unattached domain, a wrong bucket and an unreachable host indistinguishable. Failures are now aggregated per kind with a sample message, which surfaces things like Cloudflare's `error code: 1010`. Also documents that the probe carries the production header set — notably no `User-Agent` — which makes it a truer acceptance check than `curl` against a zone whose Browser Integrity Check challenges UA-less requests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… acceptance" This reverts commit 53ff309.
Quality pass over the custom-domain diff; no behaviour change. `R2ObjectFetcher`'s two constructors had grown a shared prologue. Extracted `parse_target_origin` (both origin guards, each keeping its own error text), `base_client` (timeout, connect timeout, redirect policy — with the two redirect rationales stated once) and `concurrency_permits` (the `unwrap_or(MAX_PERMITS).max(1)` clamp). Each constructor now shows only what actually differs between the targets: `http1_only()` against the three h2 knobs. Added `R2ObjectFetcher::origin()`, delegated through both binaries' adapters. That replaces four `parse_endpoint` re-runs done purely to build a log line, and four copies of the comment explaining why the parsed origin is logged instead of the raw operator flag — the invariant now lives on the accessor. The validator's two sites log after construction rather than before, so a constructor error no longer announces a source that was never built. The trace server's wiring was an `if let` / `else` / four-tuple `match` nest that wrote `Some(Arc::new(source))` at two depths; it is now a two-arm chain producing an `Option`, wrapped once. Its comment claimed clap enforced the target exclusion — `validate_args` does, and the clap group deliberately admits both so the error can name them; an edit trusting that comment would have dropped the real check. The validator's 58-line, five-level-deep `WitnessSource::R2` arm moved into `build_r2_client`. Both binaries now assemble the Access pair with `Option::zip` instead of a four-arm match. `mock_r2_capturing` captured whatever a single 4096-byte read returned, while the new tests assert on the *absence* of `authorization` / `cf-access-client-id` in that capture — an assertion a partial capture cannot make honestly. It now reads to the header terminator, bounded at 64 KiB. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The probe is an operator tool for measuring a real bucket, not something the workspace needs to build or ship. Keeping it in `examples/` put a program nobody compiles for correctness into every `cargo check --all-targets`, and carried a `serde_json` dev-dependency that only it used. Removing it takes that dev-dependency with it — nothing under `src/` uses `serde_json`, and no documentation or CI job referenced the example. The tool itself is not lost: it stays in this branch's history, and the out-of-tree copy under the gitignored `validator-data/` remains the parking spot it has been since 2026-08-06. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Applies the code-review findings for the R2 custom-domain target. **Shared JSON-RPC client pinned to HTTP/1.1.** `stateless-r2` enables reqwest's `http2` feature and Cargo unifies features workspace-wide, so the one client `stateless-common` shares across data, witness and report providers had begun offering h2 — and the endpoint accepts it. That would put multi-MB witness payloads on a single non-adaptive h2 connection per host (hyper's fixed 5MB/2MB windows) over intercontinental links, which is the hazard this PR's own fetcher doc describes and mitigates only for its new client. Moving the RPC path to h2 deserves its own measured change. **Access credentials become client default headers.** They are validated as header values at construction and marked sensitive, so a trailing newline out of a secret store fails once, by flag name and without echoing the value, instead of becoming a reqwest builder error per GET — classified retryable and so retried forever with no startup failure. Authentication is now a property of the client rather than of one code path, which also empties the custom-domain arm of `request_parts` and ends its borrowing of the signing module's `Header` alias. **Refuses to leak credentials over plaintext.** An Access token on a non-loopback `http://` origin is rejected at startup; loopback stays allowed because that is the mock and port-forward shape. Both targets now also reject a scheme reqwest cannot drive, which previously passed startup and then failed per-GET as a retryable transport error against a permanently broken URL. **Sends a `User-Agent`.** Cloudflare's Browser Integrity Check, on by default on many zones, challenges requests without one — arriving here as a non-retryable 403 on every GET, with no fallback in the validator's R2 mode. **Connection lifetime stated, not inherited.** `pool_idle_timeout(None)` stops the pool reaping the idle h2 connection on its 90s default, which silently undercut the keep-alive pings' documented job of surviving gaps between request waves; `http2_keep_alive_timeout` is now explicit. **Target-selection errors name the flag.** Clap's `requires` wiring could not: this workspace builds clap without `error-context`, so it could only say "one or more required arguments were not provided". The shapes that tripped it are the ones an operator hits following the README's S3 → custom-domain migration. Both binaries now check in their own validation and name the offending flag, and the validator no longer starts silently while ignoring leftover S3 configuration. The four comments blaming an imaginary clap limitation are corrected. **Observability.** The configured target is published as a constant-1 info gauge, so the target-less R2 series can be attributed during a rollout, and the stream-cap guidance for `--r2-max-concurrent-requests` is on the flag itself. Docs follow the behaviour, including the crate tables and the validator env-var list, which omitted the three new variables. Tests: credential validation, plaintext refusal with the loopback exemption, scheme rejection on both targets, the user agent on the wire, the tuning flags' group in both directions, and the migration shapes. The h2 knobs themselves stay uncovered — the mocks are plaintext HTTP/1.1 and asserting them needs a TLS+ALPN mock. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both defects came from the previous commit and were found by a quality pass over it. **A blank value accused the wrong flags.** `reject_conflicting_r2_targets` mixed two predicates: non-emptiness decided mutual exclusion, bare presence decided leftovers. So a blank `..._R2_CUSTOM_DOMAIN=` beside a working S3 configuration fell past the exclusion test, landed in the leftover branch, and told the operator to unset their entire real S3 config — where the trace server, which sweeps emptiness first, correctly reported the blank line. One migration runbook, two contradictory diagnoses. The validator now adopts that same order: `reject_empty_r2_values` covers all seven `--r2-*` flags before target selection runs, which leaves selection with a single predicate and removes the paragraph that existed to defend the asymmetry. Both binaries now emit byte-identical text for a blank domain, a blank endpoint, and a leftover bucket. **Two metrics were described as the wrong type.** An insertion split an existing `describe_counter!` block, leaving `r2_witness_errors_total` — a counter, and the series the R2 alarms `rate()` over — published as `# TYPE ... gauge`, and the new info gauge described as a counter. Corrected, and all 32 metrics in the module were checked to have a `describe_*` matching how they are written. The duplication that let the first defect hide is reduced. `target_label()` moves to `R2ObjectFetcher` beside `origin()`, mirroring `R2GetError::kind()`: the label vocabulary now belongs to the enum it names instead of being hand-copied into both binaries and paired with a constructor by eye at four call sites. `require_r2` is defined through `optional_r2`, so an empty value gets the same diagnosis whichever target owns the flag. The `error-context` rationale went from six copies to one per binary — two copies had already drifted, which is how the false claims below were noticed. `request_parts` becomes `request()` returning a `RequestBuilder`: with credentials on the client's default headers one arm always returned an empty `Vec`, so the tuple, the placeholder, the re-application loop and the `Header` import all go. The per-attempt seam is unchanged — a SigV4 signature is timestamped and still cannot be reused. Comment corrections, all stale as of the previous commit: two claims that clap keeps the S3 flags all-or-nothing (those attributes were deleted), a superseded line left above its replacement, a reference to the old `reject_dual_r2_targets` name, and a doc block orphaned onto the const inserted beneath it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Troublor
requested changes
Aug 19, 2026
Review found the target-selection policy implemented twice — free functions in the validator, inline in the trace server's `validate_args` — and, in the same change that introduced the pair, already diverged three ways: the trace server named every missing member of an incomplete credential quad while the validator reported them one flag at a time; the validator swept empty values twice; and the shared tuning flags were gated on the trace server but accepted-and-ignored on the validator. `stateless_common::validate_r2_flags` now owns the rules — empty values, target exclusion, leftovers from the documented S3 → custom-domain migration, quad completeness, the Access pair, and tuning flags with nothing to tune — while each binary supplies its own flag spellings. The three divergences go with it, and the four local helpers they were spread across are deleted. **The Access pair leaves clap.** `requires_all` produced exactly the unnamed "one or more required arguments were not provided" this work set out to eliminate, and it was the last coherence rule still enforced there. It also carried a latent risk: both call sites build the credentials with `Option::zip`, where one half alone yields `None` — a legitimate configuration, since an IP-allowlisted domain needs no token — so nothing downstream would object to a cleanly-constructed, working, unauthenticated client. The only thing preventing that was a clap attribute; now it is a checked invariant. **Tuning flags become `Option`** rather than clap defaults, so "explicitly set" stays distinguishable and setting one with no target is rejected by name instead of silently ignored. **The validator stops judging R2 flags it never reads.** Both checks ran before the `--witness-source` match, so they applied to every startup including RPC mode, the default, where none of the seven values is ever consulted — a fleet templating one env file across roles, where an unset variable renders as a blank line, would crash-loop hosts this work was not meant to touch. They now run inside the R2 arm. The trace server keeps checking on every startup: that reach predates this work and operators rely on a bad `--r2-*` value failing fast rather than surfacing later as `kind="missing"`, the counter watched for bucket gaps. Same rules, each binary choosing when to apply them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Version selection on the custom-domain target is pure ALPN — there is no
`http2_prior_knowledge`, deliberately, since it would break the plaintext
loopback path the mocks and port-forwards rely on. So an origin that does not
offer h2 simply answers normally and nothing notices: the three h2 knobs go
inert, `r2_target_info` keeps asserting the target that was *configured*, and
the multiplexing this target exists for is gone. A grey-clouded DNS record, a
non-Cloudflare origin, or a zone with HTTP/2 disabled all land there.
The degraded state was also worse than the S3 target it replaced.
`pool_idle_timeout(None)` was set so the single multiplexed connection survives
the gaps between request waves; on an HTTP/1.1 pool it means idle sockets are
never reaped, and reqwest's `pool_max_idle_per_host` defaults to `usize::MAX`,
so nothing bounded them either — where the S3 client reaps at 90s.
Two independent fixes. The fetcher now records the protocol of the first
response and warns once, naming what it actually negotiated — once rather than
per GET, which at this call rate would be thousands of lines a minute — and
publishes it as `..._r2_negotiated_http_version_info{version}`, so the question
is answerable on a dashboard instead of in one host's log. A separate gauge from
the target one on purpose: that answers "what was configured" and can be
published at startup, while this is only knowable after a response, and folding
both into one gauge would strand the startup series at 1 forever beside the
corrected one. And `pool_max_idle_per_host` is now bounded by the concurrency
cap, so the fallback path cannot accumulate sockets nothing will close.
The test asserts the downgrade is visible against the existing plaintext mock —
which is to say it pins the fact that every custom-domain test in this suite has
been exercising the degraded path all along. That is why none of them caught it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The acceptance run on tko sat at exactly the edge's per-connection stream limit and showed connection-level failures, which I first read as hyper opening surplus connections once the limit was reached. Reading the pool says otherwise: `PoolClient::is_open` on an h2 connection reports liveness, not stream capacity (`hyper::client::dispatch::UnboundedSender::is_ready` is `!giver.is_canceled()`), the dispatch channel behind it is unbounded, and `Pool::connecting` admits at most one h2 connect task per host. One client is therefore one connection, and requests past the stream limit queue inside it rather than earning a connection of their own. So exactly at the limit is the intended sizing, not a hazard: every permit maps to a stream slot and nothing queues. Only over-subscription is worth naming, and that is what the startup advisory now covers. Records the provenance the guidance had been asserting without one — the value is read from Cloudflare's own `SETTINGS_MAX_CONCURRENT_STREAMS`, with the command to re-read it per zone — states that the limit bounds the process rather than the request, and documents the interaction on the validator, where `--witness-max-concurrent-requests` sizes the R2 semaphore and the RPC gateway at once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`--r2-connections` (default 1, so nothing changes unless it is set) holds that many `reqwest::Client`s for the custom-domain target and hands them out round-robin. One client is one HTTP/2 connection and hyper opens no second one when the first saturates — `PoolClient::is_open` reports liveness rather than stream capacity, the dispatch channel behind it is unbounded, and `Pool::connecting` admits at most one h2 connect task per host. Everything past the edge's per-connection stream limit therefore waits inside the connection, where the wait escapes the queue-wait metric and still counts against the per-attempt timeout. Holding several clients is the only way to lift that ceiling, and the acceptance run measured it as linear: 32 connections sustained 4.93M cold GETs at ~560/s each, the same per-connection rate a single client reaches. The availability argument is the stronger one, and it is why this lands with the target rather than after it: a single connection is a single point of failure, and when it drops, every GET riding it fails together. The validator's R2 mode has no RPC fallback to absorb that. Connections are therefore handed out per *attempt*, not per fetch, so a retry leaves the connection that just failed instead of retrying into the same fault. The concurrency cap stays fetcher-wide and is split evenly across connections, rounded up: rounding down would leave some connection at zero permits and wedge every GET routed to it. Permits live on the connection rather than on the fetcher, because the limit that binds is per connection. The startup advisory now measures the per-connection share, so spreading the same concurrency wider clears it — which is the fix it is pointing at. The S3 target keeps one connection by construction: HTTP/1.1 already opens a socket per in-flight GET there, so a count set alongside it is rejected by name rather than honoured into a spread that never happens, as is a count of zero. The count is published as `debug_trace_r2_connections` / `r2_connections` and logged with the target at startup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
flyq
marked this pull request as ready for review
August 20, 2026 03:26
`r2probe.sh` is the self-extracting build of the R2 custom-domain probe — an operations tool that reads the production fetcher, not something the binaries build or CI runs. It reached `bin/stateless-validator/src/.probe_test/` by a blanket `git add -A`, where a dot-directory kept it out of sight. The probe itself lives under `validator-data/`, which `.gitignore` covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e30440d4e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
`--r2-connections` arrived as a `usize`, so a blank `..._R2_CONNECTIONS=` line — what a templated env file renders for a variable a given role does not set — aborted startup at clap parse with "invalid value for one of the arguments", naming nothing, since this workspace builds clap without `error-context`. On the validator it aborted even under `--witness-source rpc`, where every `--r2-*` flag is deliberately unread: the same shape a reviewer raised against the empty-value sweep, reintroduced by the flag that was added after it was fixed. It now travels as text and is parsed by `parse_r2_connections` alongside the other R2 rules, so a blank value is named where the flags are actually read, and stays inert where they are not. The count is also now rejected above the cap it divides. That is the one configuration where the permit split — which rounds up, deliberately, because rounding down would leave a connection at zero permits and wedge every GET routed to it — could put more than a rounding residue over the operator's cap, and it is nonsense on its own terms: more connections than permits leaves some of them permanently idle. Rejected rather than clamped, matching how the S3 case is handled: clamping would hand back fewer connections than the published gauge reports. The negotiated-protocol gauge moves off the success path in both adapters. The fetcher latches the version on any HTTP response, but the adapters only published after a decoded witness (validator) or a successful GET (trace server), so a configuration 403 or a run of frontier 404s hid the answer to "did this target come up on h2?" precisely while that was the question. Also corrects two stale doc claims the same reviewer's thread exposed: the validator's `--r2-connect-timeout-ms` no longer promises a rejection that `tuning: &[]` makes unreachable there, and `build_r2_client` no longer links to `reject_conflicting_r2_targets`, which this branch deleted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ving Quality pass over the branch. Four things the diff had spread out, plus one behaviour the spreading was hiding. **The connection pick is now work-conserving.** Committing to the rotating cursor's connection and *then* waiting on its semaphore partitioned one budget of `max` into `N` budgets of `max/N`, which queues distinctly worse at the same offered load, and stranded a GET behind a connection whose permits were held by a slow transfer while another sat idle. `ConnectionPool::try_acquire` now searches from the cursor for a connection that has room. Only when every connection is full does a fetch wait, and it waits on the cursor's own pick rather than on whichever has the most room — under saturation that one is the connection that just dropped every GET riding it, so choosing by capacity would steer the wait into the fault. The slice and the cursor that indexes it are one object now; the modulo is only meaningful against that slice's length. **`R2Target` carries the connection count it validated.** The verdict was a bare discriminant, so the count was parsed a second time in each binary behind a `?` that could not fail, and the trace server discarded the verdict entirely and re-derived the target three more times by hand — one of them by re-destructuring the S3 quad the validator had just proved complete. That is the drift this PR exists to end, reappearing inside it. `parse_r2_connections` is private again. **The negotiated protocol is pushed, not polled.** `observe_version` already knew the moment it learned the protocol, but only stored it, so both adapters carried a byte-identical `publish_negotiated_version` with its own process-wide `Once` and had to split `.await?` into three lines to poll on the failure path. A callback registered at construction replaces all of it. `AtomicU8` plus a label array plus a code mapping becomes `OnceLock<&'static str>`, which gives the same exactly-one-winner semantics without the unchecked array index. And because `Target` now knows the protocol it expects, observation is no longer gated on the target — that branch existed only because the warning text names one. **One home for the per-connection share.** `max.div_ceil(connections)` was written three times with three different absent-cap defaults, for three things that must agree: the permits, the idle-socket bound, and the share the startup advisory reports. An advisory naming a share the semaphores do not hand out would be worse than none. Fixed the idle bound's absent-cap arm while there: it was the one that did not divide, so an unset cap multiplied the fetcher-wide idle bound by the connection count. Also: a fallible test constructor so five call sites stop spelling out six arguments, one `BackoffPolicy → RetryPacing` conversion per adapter instead of two, `map_or` instead of a `Duration → u64 ms → Duration` round trip, and four comments describing clap groups and `requires` attributes this branch removed. Skipped: parsing the loopback host through `url::Host` rather than by hand would mean a new dependency on a leaf crate mega-reth consumes, or a breaking change to `parse_endpoint`'s signature — more than the eight lines are worth. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Handing out a connection and choosing the one to wait on were two calls, each stepping the round-robin cursor. On the saturated path both ran, so the cursor advanced by two per attempt — and a stride sharing a factor with the connection count reaches only some of them. At the shape this flag exists for, 16 connections under a cap of 1024, half the connections were never waited on while the other half took every waiter. `acquire_or_wait_on` now returns either the acquired permit or the connection to wait on, from a single cursor step. Measured before and after: filling to the cap was already exactly 64 per connection (the permits divide evenly, so that much was forced), but the waiters went from [8,0,8,0,…] to 4 on each of the 16. `a_saturated_pool_spreads_evenly_over_every_connection` pins both halves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codespell flags the -eable spelling. Doc comment only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 071456922e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
A review flagged `http://[::1]:8080` as a case where `parse_endpoint` drops the brackets, rebuilds the origin as `http://::1:8080`, and the plaintext- credential refusal then fires on what is really a port-forward. It does not: `Url::host_str` keeps the brackets on an IPv6 host, so the rebuilt origin is still a parseable authority and the loopback check reads `::1` out of it. Worth a test rather than a reply, because nothing pinned the round trip and the two halves live in different modules — a future change to how the origin is reassembled would break the mock and port-forward path with a refusal that names exposure, which is the least helpful error it could give. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Troublor
approved these changes
Aug 21, 2026
vincent-k2026
approved these changes
Aug 21, 2026
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
3 participants
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.
Summary
stateless-r2gains a second read target: a Cloudflare custom domain fronting the witness bucket — unsigned GETs of/{key}(no bucket segment), optional Cloudflare Access service-token headers (CF-Access-Client-Id/Secret), and HTTP/2 via the reqwesthttp2feature with adaptive flow-control windows and keep-alive pings.--r2-connections(default 1, so nothing changes unless it is set) spreads those GETs over several HTTP/2 connections. The SigV4 S3 path is byte-for-byte unchanged and now pinnedhttp1_only, so its wire behavior is independent of the feature flip. Both binaries expose the target as--r2-custom-domainplus the optional--r2-access-client-id/--r2-access-client-secretpair (the id is redacted like the secret — it alone can look up the token); the two targets are mutually exclusive, enforced post-parse so the error names both flags even when they arrive through env vars.Why
R2's bare S3 endpoint negotiates HTTP/1.1 only (ALPN-verified live), so N in-flight GETs hold N connections, capped by the egress IP's adaptive connection budget — the constraint behind today's
--r2-max-concurrent-requests=48. A custom domain rides the h2-capable CDN edge: concurrency decouples from connection count, the steady-state handshake load collapses to a few long-lived multiplexed connections instead of a socket per in-flight GET, and the immutable witness objects become edge-cacheable. Steady state is the qualifier that matters: version selection is pure ALPN, so hyper takes no connect lock before the handshake and every queued GET opens its own until one lands in the pool. Cold start and every reconnect therefore still burst up to the per-connection concurrency — the mechanism behind thekind=connectfailures reported below.Where one connection stops, and why
--r2-connectionsexistsOne
reqwest::Clientis one HTTP/2 connection, and hyper opens no second one to relieve a saturated one. The pool hands every checkout the same connection back — an h2 entry is reserved as a shared clone and re-inserted on each take (Reservation::Shared,IdlePopper::pop) — and the only health check it applies isPoolClient::is_open, which reports liveness rather than stream capacity: it delegates tohyper::client::dispatch::UnboundedSender::is_ready, which is!giver.is_canceled(), over a dispatch channel that is unbounded. A live-but-saturated connection is therefore always found and reused, and the connect path is never entered on its account.(
Pool::connecting, which does serialize h2 connects, is not what does this here: its lock is gated onver == Ver::Http2, which hyper-util sets only underhttp2_only(true)— reqwest'shttp2_prior_knowledge(). This target deliberately selects by ALPN instead, so that the plaintext loopback path the mocks use keeps working, which leavesver == Ver::Auto.)So a request past the edge's
SETTINGS_MAX_CONCURRENT_STREAMSneither fails nor earns a connection of its own — it waits inside the connection, where the wait still counts against the per-attempt timeout and escapes the permit-queue accounting:debug_trace_r2_witness_queue_wait_secondsnever sees it, and on the validator, which publishes no queue-wait series and subtracts permit wait out ofwitness_fetch_r2_time_seconds, it lands in that histogram as R2 time.That makes the edge's per-connection stream limit a per-process ceiling, and
--r2-connectionsis the only thing that lifts it. Connections are handed out round-robin per attempt rather than per fetch, so a retry leaves the connection that just failed instead of retrying into the same fault. The in-flight cap still bounds all of them together, split evenly and rounded up — rounding down would leave a connection at zero permits and wedge every GET routed to it — so raising the connection count alone spreads the same concurrency thinner rather than lifting the ceiling. That cap is--r2-max-concurrent-requestson the trace server; on the validator it is--witness-max-concurrent-requests, which sizes the RPC gateway too, so there the two consumers trade off against each other.The availability argument is what justifies landing this alongside the target rather than after it: a single connection is a single point of failure, and when it drops every GET riding it fails together. The validator's R2 mode has no RPC fallback to absorb that.
Acceptance measurements
Measured on one host against the delivered domain. The probe drives the production fetcher through a
pathdependency; it lives with the ops tooling and is not part of this diff.The stream limit is read, not assumed. Re-read it per zone — the whole concurrency story hangs on this one number.
What this target is worth, on the workload it was built for
The same host ran a one-hour soak against the S3 target on 2026-08-11 in a fixed shape — 15 concurrent JSON-RPC batches of 20 blocks, 300 blocks in flight,
debug_traceBlockByNumber+callTracerover historical blocks. Re-running that exact shape against the custom domain, on this branch:The old run's own conclusion named this fix: the S3 endpoint speaks HTTP/1.1 only, one request per connection, so 48 permits were the ceiling and two thirds of every witness fetch was spent waiting for one. The queue is now gone, and throughput on the unchanged workload is 2.2×.
Pushed further on the same build — five 300-second rungs from 300 to 1500 blocks in flight, each on a block range no earlier rung had touched — the target absorbed 1,175,400 blocks with zero R2 failures of any kind: no
missing, nothrottled, nostatus, noconnect, onetransportin 1.6M GETs. R2 semaphore queue wait stayed at 0 ms throughout, andwitness_r2served 100% of fetches with the generator and public gateway untouched on the request path. Witness stage grew only 83 → 197 ms across a 5× concurrency increase; what did grow was the local node behindeth_getHeaderByHash/eth_getBlockByHash, 2.4 → 115 ms and 3.9 → 125 ms. R2 is no longer the constraint on this path — which is the result this target was added to produce.Cold vs warm, and the negative-cache gate
Two single-pass runs over disjoint historical ranges, 32 connections × 100:
cf-cache-statusLike for like on the same blocks: +59% throughput, −46% latency. The gain is in the body of the distribution, not the tail — p50 and p90 each drop a bucket, the p99 bucket is unchanged, and the share over 1600 ms rises from 0.002% to 0.043%. Functional acceptance passed alongside: Access enforcing, credentials accepted, an authenticated fetch of a real object, ALPN
h2, and a 404 that goesMISS → EXPIREDand neverHIT, which is the negative-cache gate the deployment note below depends on.Failures, and what is not claimed
At 32 connections × 100 — one connection per 100 streams, exactly the edge's advertised limit — 1.40% of GETs failed cold and 2.43% warm, essentially all
kind=connect;transportnever exceeded 2 per minute. Stating only what follows from the code: version selection is ALPN, so hyper takes no connect lock before the handshake and every queued GET on a connection that has none yet opens its own, which makes a cold or dropped connection a burst of up to the per-connection share of simultaneous handshakes against the connect timeout. Some of the count is fan-out instead — the task that wins the post-ALPN lock leaves the rest queued, andPoolInner::connectedclears that list when it fails. Which effect dominates is not established, and neither is why the connections drop...._r2_witness_errors_total{kind="connect"}is the series to watch. None of this appeared at the concurrencies the trace-server workload actually uses: the five-rung ladder above ran clean.Deployment note
Any edge cache rule that makes these objects cacheable must set 404s to bypass cache: readers probe objects before the uploader PUTs them, and a negatively-cached 404 would pin a pre-upload frontier miss for the TTL (the validator's R2 mode has no RPC fallback) and can false-fire the below-band
kind="missing"bucket-integrity alarm. The caveat is carried in the flag docs, README, and AGENTS.md; the ops runbook for the domain setup includes the concrete Cache Rule setting.Sizing: what has to stay at or below the edge's stream limit is the per-connection share — the in-flight cap divided by
--r2-connections— not the total, and the fetcher warns at startup when that share exceeds it. Raising--r2-connectionson its own therefore buys fault isolation at unchanged throughput, which makes it a clean single-variable first step; raising throughput needs the cap raised with it. The count is published asdebug_trace_r2_connections/r2_connections.Testing
Wire-shape tests through a new capturing mock (
mock_r2_capturing): bucketless unsigned custom-domain path, Access headers present including on retry attempts, the S3 path's bucket prefix + SigV4 authorization pinned, bare 404 →Missing, redirects surfaced asStatus(Access login bounce) on both clients, andDebugredaction of the Access pair through the fetcher. Sharding adds the per-connection permit split including the round-up that keeps a connection off zero, round-robin hand-out across connections, the saturated pool spreading evenly over every connection rather than a subset, and the startup advisory firing only on per-connection over-subscription — silent exactly at the limit, which is the intended sizing, and silent when unlimited, which is the default.Every R2 argument rule is covered by
validate_r2_flags's own unit tests (crates/stateless-common/src/r2_args.rs), each asserting the error names the offending flag. The trace server's CLI tests drive those rules throughvalidate_args: mutual exclusion naming both flags, the all-or-nothing Access pair, empty-value rejection (a blankR2_CUSTOM_DOMAIN=line over a working S3 config gets the named empty-value error, not a phantom target conflict), the shared tuning flags accepted with either target and rejected by name with none, a zero connection count, and a count set alongside the S3 endpoint. The validator's CLI test pins the other half — that all of those shapes parse — since its R2 flags are validated post-parse and only under--witness-source r2.An IPv6 loopback origin is pinned end to end (
http://[::1]:8080accepted with credentials, brackets surviving the origin rebuild, a public IPv6 host still refused over plaintext).cargo test --workspace(55 stateless-r2 + 55 stateless-common + 159 debug-trace-server + 40 stateless-validator),cargo clippy --workspace --all-targets --all-features,cargo fmt --check, andcargo sort --checkare all clean.Notes for reviewers
The reqwest
http2feature propagates by Cargo feature unification — first inside this workspace, then to consuming ones (mega-reth picks it up on the coordinated bump). Unpinned clients start offering h2 via ALPN, which servers are free to decline. Two clients here must not move, and this PR pins bothhttp1_only: the S3 fetcher, which keeps the signed path's wire behavior fixed regardless of what the endpoint ever negotiates, andstateless-common's shared JSON-RPC client (rpc_client.rs), whose endpoints do accept h2 — that would put the multi-MB witness payloads on one shared connection per host with adaptive windows off. Moving the RPC path to h2 is a change worth measuring on its own, not one to inherit from a feature flip.Multi-connection sharding was originally deferred here pending measurements against the real domain. The measurements arrived and it landed in this PR instead: the availability argument turned out to be the stronger one, and it is not something a follow-up should leave open on a target whose validator mode has no fallback. Still out of scope: HTTP/3 (the edge advertises
alt-svc: h3, but reqwest's h3 support is unstable and gated behind aRUSTFLAGScfg), and recalibrating the deployed concurrency cap, which is a configuration change rather than a code one.🤖 Generated with Claude Code