chore: update h2#827
Conversation
bindsi
left a comment
There was a problem hiding this comment.
Requesting changes — not over style, but because this change may regress the very issue it's meant to fix (#686). Please verify before merge.
🔴 The patch likely no longer covers the gRPC/UDS path
The single functional change in the lockfile is the patched fork moving from the 0.3.x line to 0.4.15:
name = "h2"
- version = "0.4.14" source = registry
+ version = "0.4.15" source = git+…/project-akri/h2?branch=patch-authority#e01e8842…
The base pinned the fork at h2 0.3.26. Rebasing patch-authority onto h2 master moved it to the 0.4.x line. But Akri's gRPC stack is pinned to tonic = "0.10" (discovery-utils/Cargo.toml) → hyper 0.14 → h2 ^0.3, and this PR doesn't bump tonic/hyper.
A [patch.crates-io] only replaces a crate when its version is semver-compatible with what consumers request. 0.4.15 is not compatible with ^0.3, so the patch can no longer apply to the tonic 0.10 / hyper 0.14 path — that h2 falls back to the unpatched registry 0.3.x. So the "malformed authority" fix likely stops applying exactly where #686 lives. Cargo probably now also emits a patch for 'h2' … was not used in the crate graph warning (non-fatal, easy to miss — and a basic udev smoke test won't necessarily exercise the malformed-authority path, which is why local testing may have looked fine).
Could you confirm with cargo tree -i h2 (and a cargo build watching for the "patch … was not used" warning) that the agent/discovery-utils gRPC actually resolves h2 to the fork? If it resolves to registry 0.3.x, the options are:
- keep the fork on the 0.3.x line (don't rebase past 0.3), or
- upgrade tonic → 0.12+ / hyper → 1.x so the runtime uses h2 0.4.x where the patch now lands (a larger change, not in this PR).
🟠 Scope creep under a chore: update h2 title
This is Cargo.lock-only, but it's a broad cargo update: ~40 unrelated crates bump (actix-web 4.13→4.14, syn everywhere, bytes, brotli, jiff now pulling in defmt+bitflags 1.x, getrandom, env_logger, plus removals of id-arena/leb128fmt/rustls-native-certs 0.7.3). For a supply-chain-sensitive patched-dependency change, an unrelated full lockfile refresh makes it harder to audit and to bisect a regression. Suggest narrowing to cargo update -p h2 (+ required transitive), or explicitly noting the full refresh in the description.
✅ Good
The fork is org-owned (project-akri/h2) and pinned to an immutable commit SHA — exactly the right supply-chain pattern.
Happy to approve once the cargo tree -i h2 output confirms the fork still covers the gRPC path. Thanks!
Signed-off-by: Gaurav Gahlot <gaurav.gahlot@ionos.com>
|
Thanks for the review @bindsi. Looked into both points: 1. gRPC coverage. The concern assumed tonic 0.10, but this branch is already on tonic 0.13, so the UDS/gRPC path resolves h2 through hyper 1.x — the 0.4.x line, not 0.3. The fork covers the whole tonic path; the unpatched registry 2. Scope. Narrowed to h2 only — the lockfile diff is now just h2. I pinned the patch to the commit ( |
bindsi
left a comment
There was a problem hiding this comment.
Thanks for reworking this — the narrowing to a minimal diff and pinning by rev instead of branch are both real improvements (the rev pin is exactly the right fix for the "patch silently drifts on cargo update" problem, and the inline comment explaining it is great). 👍
But I don't think this closes #686 yet, and Cargo.lock confirms it: h2 is still under [[patch.unused]] after the change:
[[patch.unused]]
name = "h2"
version = "0.4.15"
source = "git+…/project-akri/h2?rev=e01e8842…"
[[patch.unused]] is Cargo telling us the patch applies to nothing in the graph. The reason is a version mismatch: the gRPC-over-UDS path is tonic = "0.10" → hyper 0.14 → registry h2 0.3.27, but the fork is now 0.4.15. A [patch.crates-io] only substitutes when the patched version matches what's locked, and 0.4.15 can't replace 0.3.27 (nor the h2 0.4.14 on the hyper-1.x path). So the "malformed authority" fix isn't actually being compiled in — same state as main, where the fork (0.3.26) was already [[patch.unused]] against registry 0.3.27.
To actually land the fix, one of:
- Point the fork/patch at an h2
0.3.27commit that carries the authority change (so it matches whattonic 0.10/hyper 0.14 resolve to), or - Upgrade tonic → 0.12+ / hyper → 1.x so the runtime uses
h2 0.4.x, where a0.4.15patch would apply.
Quick way to confirm either way: after building, cargo tree -i h2 and check there's no [[patch.unused]] block for h2 in Cargo.lock. Happy to approve once h2 shows up as an applied patch on the gRPC path. Thanks again!
What this PR does / why we need it:
Fixes: #686
Special notes for your reviewer:
patch-authorityon the latestmasterudevexample.If applicable:
cargo fmt)cargo build)cargo clippy)cargo test)cargo doc)