Skip to content

ALPN-based h2 upstream dispatch + fix silently-dropped JS-wrapper config fields#24

Open
kriszyp wants to merge 3 commits into
mainfrom
kris/h2-upstream-dispatch
Open

ALPN-based h2 upstream dispatch + fix silently-dropped JS-wrapper config fields#24
kriszyp wants to merge 3 commits into
mainfrom
kris/h2-upstream-dispatch

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 8, 2026

Copy link
Copy Markdown
Member

Summary

Completes the HTTP/2 story on the symphony side (pairs with HarperFast/harper#1707, Harper's cleartext-h2 UDS mirror): symphony stays a byte-pump — no HTTP parsing — and gains protocol-aware upstream dispatch.

  • UDS upstreams can be marked protocol: 'h2' (matching the protocol: h2 line in Harper's -h2.sock UDS metadata yaml). build_destinations partitions a route's upstreams into the default (h1) destination + an optional destination_h2; after the TLS handshake, connections that negotiated h2 in ALPN are pumped to the h2 destination, everything else to the default.
  • Config invariants (root-cause enforcement rather than runtime corruption): any h2-capable route (http2: true or h2-marked upstreams) rejects sourceAddressHeader: 'xForwardedFor' — header injection would splice into the middle of the h2 connection preface; PROXY protocol rides before the preface and works for both protocols. h2 upstreams on a passthrough route are also rejected (no ALPN visibility without terminating). All-h2-upstream routes are rejected (h1 clients need somewhere to go). Runtime backstop: suspended-route resolutions supply their own source mode, so h2-negotiated connections skip XFF injection instead of corrupting the preface.
  • Bug fix riding along: toJsRoute/toJsUpstream in the TS wrapper were silently dropping fields the Rust side supports — route http2 (ALPN advertisement, dead since e72b220) and upstream pid/tid (the CPU-aware balancer tiebreaker) never reached the addon. Verified no deployed consumer sets http2: true (host-manager/central-manager grepped), so re-arming it has no blast radius, but reviewers should know the flag was inert until now.

Where to look

  • src/router.rs build_destinations — the upstream partition + invariants.
  • src/proxy_conn.rs — the post-handshake ALPN selection + runtime XFF guard.
  • The dropped-fields fix in ts/proxy.ts is two mapping lines but is the reason the http2 option now does something.

Notes

  • 6 new tests in __test__/h2-dispatch.spec.ts (dispatch h2/h1 with PROXY-precedes-preface byte-order assertions, plus the four config-rejection cases). Full suite 38/39 — the one failure (server.spec.js "writes a status file with pid and version", 0.5.0 vs 0.4.0) is pre-existing on main from the version-sync commit.
  • cargo clippy pre-existing errors on main (cargo clippy --all-targets fails on main (10 pre-existing errors under deny(clippy::all), clippy 1.94) #21) unchanged by this diff.
  • Cross-model review (Gemini diff-only + Harper-domain adjudication) ran pre-PR; its two significant findings (the XFF gap for non-split h2 routes, a racy test assertion) are fixed in the second commit. Open suggestion I deliberately did not take: promoting the http2/h2-upstream mismatch warnings to hard errors — warnings preserve the pre-existing behavior of http2: true alone.

Drafted by an LLM (Claude Fable 5).

kriszyp and others added 2 commits July 7, 2026 22:10
…s in the JS wrapper

Routes can now mark UDS upstreams with protocol: 'h2' (e.g. Harper's -h2.sock
mirror, discovered via its 'protocol: h2' UDS metadata): connections that
negotiate h2 in ALPN are forwarded there, everything else to the unmarked
upstreams. Config invariants: h2 upstreams require a non-XFF source-address
mode (header injection would corrupt h2 frames) and at least one default
upstream; http2=false with h2 upstreams (unreachable) and http2=true with no
h2 upstream (default upstream must speak h2) both warn.

Also fixes toJsRoute/toJsUpstream silently dropping fields the Rust side
supports: route http2 (ALPN advertisement, dead since e72b220) and upstream
pid/tid (CPU-aware balancer tiebreaker, dead through the JS API) never reached
the addon.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- xForwardedFor is now rejected for ANY h2-capable route (http2=true too, not
  just split-h2 destinations): an h2 client's preface would otherwise get an
  XFF header spliced into it. Runtime guard as well: suspended-route
  resolutions supply their own source mode, so h2-negotiated connections skip
  XFF injection rather than corrupt the preface.
- h2-marked upstreams on a passthrough route are a config error (symphony
  never sees negotiated ALPN without terminating).
- De-raced the PROXY-precedes-preface test: assert on the accumulated stream,
  not the first recv (header and pumped bytes are separate writes).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces ALPN-based upstream dispatch, allowing HTTP/2 connections to be routed to specific h2-marked UDS upstreams while other traffic goes to default HTTP/1.x upstreams. It includes configuration validation to prevent invalid setups (such as combining HTTP/2 with X-Forwarded-For headers or using h2 upstreams without TLS termination) and adds comprehensive integration tests. The review feedback highlights three key improvements: forwarding the protocol field in the TypeScript wrapper to ensure TCP protocol validation is reachable, using safe slice access (.first()) instead of direct indexing to avoid potential panics, and replacing eprintln! with tracing::warn! to align with the application's logging framework.

Comment thread src/proxy.rs
Comment thread src/router.rs Outdated
Comment thread src/router.rs
… rejection, un-hardcode server.spec version

The server.spec version fix also un-reds symphony's test CI, broken on main
since the 0.5.0 version sync hardcoded-expectation mismatch.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kriszyp kriszyp marked this pull request as ready for review July 8, 2026 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant