fix(proxy): negotiate the upstream HTTP protocol independently

[andybons]

Problem

HTTP/2 box clients received 502 responses from HTTP/1.1-only TLS upstreams such as Inngest ingest (inn.gs, POST /e/<key>). The proxy selected a bare http2.Transport from the client ALPN result, but that transport cannot fall back to HTTP/1.1.

Change

Use a shared http.Transport configured with http2.ConfigureTransports for h2 clients. Upstream TLS offers h2 and http/1.1, uses h2 when selected, and falls back to HTTP/1.1 for an HTTP/1.1 or absent ALPN selection. Host:port connection pools reuse negotiated connections across client tunnels without a probe handshake. Keep HTTP/1.1 client behavior unchanged and retain HTTP/2 health checks. Add a v0.25.1 / Fixed changelog entry dated 2026-10-05.

Verification

The new fallback regressions failed before the fix and pass afterward. Coverage includes h1-only and no-ALPN upstreams, h2, gRPC, streaming and trailers, hop-by-hop response headers, credential injection and logging, and per-host connection reuse across client tunnels. Independent review found no issues.

Passed: go build ./..., go test -race ./..., go vet ./..., go run ./cmd/skill-lint, repeated focused HTTP/2 tests, and git diff --check.

Do not merge automatically.

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 20s —— View job

---

Review of #77

The approach is sound. http.Transport plus http2.ConfigureTransports is the right way to get ALPN-driven h2 with an h1 fallback, and sharing the pool across tunnels is a good change. I found no credential-handling regressions: injection still happens before RoundTrip, and the pool is keyed by host:port only, so no credentials end up in connection state. I read the diff but did not run the tests.

Issues

1. SetUpstreamCAs is silently ignored after first use (proxy/upstream.go:23-26). The shared transport copies p.upstreamCAs once, when it is first created. Before this PR every tunnel read p.upstreamCAs fresh. Now a later SetUpstreamCAs call has no effect on h2 clients, while h1 clients still pick it up. That is an inconsistent trust configuration. Two ways to fix it:

   • Invalidate or close h2UpstreamTransport in SetUpstreamCAs (under p.mu).
   • Document that it must be called before serving, and have SetUpstreamCAs take p.mu.

   Also, SetUpstreamCAs writes the field without a lock, and getH2UpstreamTransport now reads it under p.mu.Lock. The race is pre-existing, but it is now easier to trigger.

2. The shared transport is never closed (proxy/upstream.go). Idle upstream h2 connections stay open until IdleConnTimeout (90s) and are never closed on proxy shutdown. h2 connections also have no ping-based reaping beyond ReadIdleTimeout. Call CloseIdleConnections() in the server stop path. Otherwise leak checks in tests and embedders such as moat may see lingering goroutines.

3. Minor: the upstream TLS config is duplicated. proxy.go still builds upstreamTLS and a dialer for the h1 path, and upstream.go rebuilds the same config and dialer. Divergence is the likely cause of bug 1 recurring, so it is worth factoring into one helper.

4. Minor: getH2UpstreamTransport takes the write lock on every h2 tunnel. p.mu is an RWMutex that many hot paths read-lock. Use a sync.Once or an RLock fast path to avoid contending with them.

Test gap: nothing exercises SetUpstreamCAs being called after the first h2 request. A test for that would catch issue 1.

Not merging, as requested.

[Copilot]

Copilot review overview

🔵 Needs a closer look

TLS negotiation and cross-tunnel pooling of credential-bearing traffic warrant final human validation.

Review effort: Balanced
Findings: None

What changed in this PR

Decouples upstream protocol negotiation from client HTTP/2 negotiation in Gatekeeper’s TLS-intercepting proxy, allowing HTTP/2 clients to reach HTTP/1.1-only upstreams.

Changes:

• Adds a shared upstream transport with HTTP/2 negotiation, HTTP/1.1 fallback, connection reuse, and health checks.
• Preserves HTTP/1.1 client behavior.
• Adds regression coverage and an Unreleased changelog entry.

File	Description
proxy/​upstream.go	Configures the shared, protocol-negotiating upstream transport.
proxy/​proxy.go	Uses the shared transport for HTTP/2 client tunnels.
proxy/​http2_test.go	Tests fallback, streaming, trailers, credential handling, and cross-tunnel connection reuse.
CHANGELOG.md	Documents the upstream protocol fallback fix.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[andybons]

Addressed the review in the follow-up commit:

1. CA replacement: verified with a red-first regression: replacing the trusted pool after the first h2 request still returned 200 instead of the expected 502 on a new client tunnel. SetUpstreamCAs now locks, invalidates the shared transport, and closes its idle connections. All CA readers use the same lock, including the existing Postgres reader. The test also restores trust and verifies success. Existing client tunnels retain their original trust configuration, matching both the pre-PR h2 behavior and the unchanged h1 path; this PR does not add live trust revocation within active tunnels.
2. Shutdown: verified with red-first tests for both proxy.Server.Stop and standalone gatekeeper.Server.Stop. Both now call Proxy.CloseIdleConnections. The 90-second idle timeout already provides eventual h2 cleanup; ping checks detect unresponsive peers rather than reap healthy idle connections. The new cleanup releases idle connections immediately at shutdown without changing the pre-existing handling of active/hijacked tunnels.
3. Shared configuration: both intercepted paths now use newUpstreamTransport for TLS, dial/handshake/header timeouts, and pooling settings; only ALPN/h2 health checks differ.
4. Contention: added an RLock fast path with a second check under the write lock. A sync.Once would prevent CA-driven invalidation, so it is not used.

Full build, race-enabled suite, vet, skill lint, repeated focused regressions, and diff checks pass. Copilot reported no concrete findings; credential injection remains request-scoped, not connection-scoped. No merge performed.

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 18s —— View job

---

Review of PR #77

I read the diff against origin/main and found no blocking issues. I did not run the tests. The /code-review skill failed to load, so this was a manual review.

What I checked

• Credential scoping: Injection is per request, and the shared transport is keyed by host:port. Pooling h2 connections across tunnels does not leak credentials between callers.
• Locking in getH2UpstreamTransport: The RLock fast path with a re-check under the write lock is correct. SetUpstreamCAs swaps the transport to nil under the lock and closes idle connections outside it, so there is no deadlock.
• Postgres CA read: It now goes through getUpstreamCAs(), which removes a data race with SetUpstreamCAs.
• TLS config: MinVersion is TLS 1.2 and RootCAs is snapshotted at transport creation. ForceAttemptHTTP2 plus ConfigureTransports gives the h2 → h1.1 fallback the PR describes.

Non-blocking observations

1. Retired transports are never fully closed. SetUpstreamCAs only calls CloseIdleConnections on the old transport. Active tunnels keep using it until they end, and it is then dropped. Its idle connections close after the 90s idle timeout. This is acceptable and matches the stated "no live revocation" scope, but a comment saying so would help.
2. CloseIdleConnections on shutdown only covers the h2 transport. The h1 path still builds a new http.Transport per tunnel via newUpstreamTransport, which is the same as before this PR. Idle h1 upstream connections are therefore not closed at Stop. This was already the case, so I'd leave it out of this PR's scope.
3. CloseIdleConnections has no doc comment. It is a new exported method on Proxy. Optional, but the repo is a library, so a one-line doc comment is worth adding.
4. Pre-existing: Proxy.mu is shared with unrelated state. The write lock in SetUpstreamCAs and in transport creation contends with other p.mu users. The hold times are short, so I don't consider it a problem.

I recommend merging once a human has confirmed the cross-tunnel pooling behaviour, as Copilot also suggested. I did not merge.

[andybons]

Read the full follow-up review. Addressed observations 1 and 3 with concise comments documenting retired-tunnel trust/idle-timeout semantics and the exported CloseIdleConnections scope. Observations 2 and 4 are unchanged pre-existing behavior: per-tunnel h1 transport lifecycle and the shared proxy mutex remain outside this fix. No further runtime changes were made. Full build, race-enabled suite, vet, skill lint, and diff checks pass again.

Cross-tunnel reuse is covered by TestHTTP2_UpstreamProtocolCache: separate h1/h2 upstream ports each see exactly one TLS connection across three fresh client tunnels. Injection and logs remain per request, not pool state. Nothing was merged.

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 17s —— View job

---

Review of #77 (re-review after follow-up commit)

The earlier findings are addressed. I read the diff and did not run the tests.

• CA replacement: SetUpstreamCAs now takes p.mu, drops the shared transport and closes its idle connections. getUpstreamCAs is used by the h1 path and the Postgres path.
• Shutdown: both Stop paths call CloseIdleConnections.
• Config duplication: both paths use newUpstreamTransport.
• Lock contention: the h2 path uses an RLock fast path with a recheck under the write lock, which is correct.

No credential leak, race or deadlock found. Credentials are injected per request, and the pool is keyed by host:port only.

Minor observations (non-blocking)

1. Behavior change for long-lived h2 requests (proxy/upstream.go, newUpstreamTransport). ResponseHeaderTimeout: 5 * time.Minute now also applies to the h2 path, because http2.ConfigureTransports honors the http.Transport setting. The old bare http2.Transport had no such limit. A gRPC or h2 call that waits more than 5 minutes for response headers would now fail. This is probably acceptable and matches the h1 path, but it is worth a line in the changelog or a deliberate decision.
2. MaxIdleConnsPerHost is left at the default of 2 on the shared transport. When an upstream falls back to HTTP/1.1, many concurrent client tunnels to the same host will churn connections beyond two idle ones. Consider raising it if h1-only upstreams like inn.gs see concurrent traffic.
3. A retired transport can leak connections. After SetUpstreamCAs swaps the transport, in-flight connections on the old one are only reclaimed by IdleConnTimeout (90s), and CloseIdleConnections on the new one never sees them. The code comment already notes this. Fine.

Not merging, as requested.

[andybons]

Addressed the new timeout observation: preserving long-lived h2/gRPC behavior is preferable to introducing an incidental five-minute deadline. The h2-capable transport now explicitly has no response-header timeout, as the previous bare h2 transport did; the h1 client path retains its existing five-minute timeout. The base settings remain shared. Added a red-first regression that failed with h2 response header timeout = 5m0s and documents both paths, plus a changelog clarification.

For the remaining observations: I retained the existing default per-host idle limit rather than add performance tuning to this correctness fix. Normal pool churn under bursts is distinct from the removed extra protocol-probe handshake; the host:port reuse regression still passes. Retired transports remain bounded by the existing 90-second idle timeout, as documented; there is no unbounded idle-connection leak or live trust-revocation change.

Full build, race-enabled suite, vet, skill lint, and diff checks pass. Nothing was merged.

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 17s —— View job

---

Review of #77 (after the latest push)

I found no blocking issues. I read the diff against origin/main and did not run the tests. I did not use the /code-review skill, so this was a manual review.

Checked and fine

• Credentials: Injection is still per request, before RoundTrip. The shared pool is keyed by host:port and holds no credential state.
• Locking:
  • getH2UpstreamTransport re-checks under the write lock.
  • SetUpstreamCAs swaps the transport under the lock and closes idle connections outside it, so there is no deadlock.
  • All upstreamCAs readers, including Postgres, now go through the lock.
• TLS: The minimum version is TLS 1.2. ForceAttemptHTTP2 plus ConfigureTransports gives the h2 → h1.1 fallback. Setting ResponseHeaderTimeout = 0 on the h2 transport keeps the earlier h2 behavior, which suits long-lived gRPC and streaming responses.
• Shutdown: defer CloseIdleConnections() in both Stop paths is nil-guarded.

Non-blocking notes

1. Per-tunnel h1 transports are still never closed. This was already true before the PR. Each h1 tunnel builds its own http.Transport through newUpstreamTransport and leaves its idle connections to the 90s timeout. Fold it into a later change if you want it.
2. Retired transports after SetUpstreamCAs: active tunnels keep the old trust until they end. The PR documents this, and it matches the h1 path.

The earlier feedback is addressed. Cross-tunnel pooling is covered by TestHTTP2_UpstreamProtocolCache. Final human sign-off on pooling credential-bearing traffic is still sensible, as Copilot suggested. I did not merge.

[andybons]

Thanks for the final re-review. Both remaining notes are unchanged, explicitly scoped behavior: h1 per-tunnel idle cleanup is pre-existing and remains outside this PR; active tunnels retain their original CA trust, as documented. No new changes are needed. All earlier findings are addressed, the cross-tunnel reuse regression passes, and the PR remains unmerged for human sign-off.

[claude]

Claude finished @andybons's task in 14s —— View job

---

Review of #77 (re-review after the h2 response-header timeout change)

I read the full diff against origin/main and found no new blocking issues. I did not run the tests, and the /code-review skill was not run. This was a manual read.

Checked

• Response-header timeout: getH2UpstreamTransport sets ResponseHeaderTimeout = 0 after the shared base config. That matches the old bare http2.Transport and leaves the h1 path's 5-minute limit unchanged. This addresses the earlier note about long-lived gRPC calls.
• Locking: The RLock fast path with a re-check under the write lock is correct. SetUpstreamCAs swaps the transport to nil under p.mu and calls CloseIdleConnections after unlocking, so there is no deadlock. The Postgres path now reads the pool through getUpstreamCAs().
• Credentials: Injection stays per request, and the pool is keyed by host:port, so nothing credential-bearing lives in connection state. TLS keeps MinVersion TLS 1.2 and a RootCAs snapshot taken when the transport is created.
• ALPN fallback: ForceAttemptHTTP2 plus ConfigureTransports gives h2 when selected and HTTP/1.1 otherwise, which fixes the inn.gs 502.

Non-blocking, already acknowledged

• The shared transport keeps the default MaxIdleConnsPerHost of 2. Bursty concurrent traffic to an h1-only upstream will open and close connections beyond that. This is a tuning question, not a correctness problem.
• Retired transports after SetUpstreamCAs are only reclaimed by the 90s idle timeout, and active tunnels keep their original trust. Both are documented in code comments.
• Per-tunnel h1 transports are still not closed at Stop. This is pre-existing and out of scope.

Not merging, as requested. A human should still confirm the cross-tunnel pooling behaviour.