fix(gateway): bound stalled upstream requests - #494

I have read the CONTRIBUTING.md file.

YES

What kind of change does this PR introduce?

Bug fix / reliability proposal for #493.

This PR is intentionally presented as one possible implementation of the behavior requested in #493. It is not intended to prescribe this exact architecture: if the maintainers prefer a different timeout, error-mapping, or client-pool recovery design, the issue can be addressed independently of this proposal.

What is the current behavior?

Issue #493 documents an intermittent field failure in which the gateway's upstream forwarding path stopped completing requests to GitHub. The same OneCLI image had served the requests successfully before the incident. During the failure:

• the proxy accepted CONNECT;
• curl/OpenSSL completed the client-facing TLS handshake for both TLS 1.2 and TLS 1.3 but received no HTTP response before an external deadline;
• Git/libcurl-GnuTLS either remained pending or surfaced a secondary non-proper TLS termination;
• forcing HTTP/1.1 on the client-facing Git leg was not a reliable workaround;
• the gateway process/listener remained alive and did not show resource exhaustion;
• restarting OneCLI cleared the in-memory state;
• the same requests then succeeded with no Git HTTP/TLS overrides.

The initiating runtime fault could not be recovered after restart, so the report does not claim a proven cause. Cargo feature inspection also shows that this build's upstream reqwest client does not enable reqwest's HTTP/2 feature; the upstream pool is HTTP/1.1. A bad HTTP/1.1 keep-alive connection or other stuck outbound state remains plausible, but unproven.

The deterministic source-level defect is narrower: the gateway awaited upstream RequestBuilder::send() without an explicit connect timeout, response-header/request-send deadline, or recovery mechanism for the shared reqwest client pool. A local upstream that accepted a request and never sent response headers could therefore keep the forwarding task pending indefinitely.

Related but distinct work:

• Gateway returns 502 ("resolution_failed") when a host's AAAA record SERVFAILs, instead of falling back to IPv4 #424 concerns DNS-family failure and IPv4 fallback;
• GitHub connection is half-broken #251 concerned Git credential injection and was fixed separately;
• fix(gateway): forward absolute-form https:// proxy requests over TLS #259 concerned absolute-form HTTPS forwarding and a later wrong-port axios case.

What is the new behavior?

This proposal adds two startup-parsed transport settings:

GATEWAY_UPSTREAM_CONNECT_TIMEOUT_SECS=10
GATEWAY_UPSTREAM_RESPONSE_HEADER_TIMEOUT_SECS=120

Missing values use those defaults. Zero, negative, or non-integer values fail startup and name the offending setting.

For each upstream TLS policy, the gateway owns a shared generation-managed reqwest client:

• normal certificate verification;
• explicitly configured no-verification behavior.

A request leases the current generation for its TLS-policy slot. The gateway then:

1. applies the configured reqwest connect timeout;

2. bounds RequestBuilder::send() with the configured 120-second default;

3. leaves the response body unbounded after response headers arrive;

4. on expiry, returns a static sanitized HTTP 504 response:

   {
     "error": "upstream_timeout",
     "message": "Upstream did not return response headers before the gateway timeout."
   }

5. adds x-should-retry: false to prevent unsafe automatic replay;

6. rotates the affected client generation if and only if the timing-out lease still references the current generation;

7. does not retry or replay the failed request.

Subsequent requests lease the fresh generation and cannot reuse the retired generation's pool. Concurrent requests timing out from one retired generation perform at most one rotation.

The MITM path now leases the current upstream client per inner request rather than freezing one reqwest client for the entire lifetime of a CONNECT tunnel. Long-lived tunnels can therefore observe a later generation after recovery.

Additional context

Why this design

Bound failure rather than changing protocols

The reproducible defect is the unbounded wait, not a proven TLS-version or HTTP/2 bug. This proposal therefore does not:

• disable TLS verification;
• force TLS 1.2;
• force client-facing HTTP/1.1;
• disable pooling;
• add a GitHub/Git-specific special case.

The OneCLI upstream reqwest client is already HTTP/1.1-only in this build. Pool rotation is still useful because it prevents future requests from leasing the same set of HTTP/1.1 keep-alive connections or other client-local state after a timeout.

Separate connection and send/header bounds

A connection timeout contains DNS/TCP/TLS establishment. The second bound contains a connection that accepts the request path but does not produce response headers.

The second setting is intentionally not a total response timeout. Once headers arrive, SSE, long downloads, and delayed/streamed response bodies continue without this deadline.

Important reqwest semantic: RequestBuilder::send() covers request transmission as well as waiting for response headers. A slow streaming request upload therefore counts against GATEWAY_UPSTREAM_RESPONSE_HEADER_TIMEOUT_SECS. This is disclosed in code and .env.example; it is the main compatibility trade-off in this proposal. reqwest does not expose a Go-style timer that begins strictly after the complete request body has been written, while reqwest's total timeout would incorrectly cap streamed response bodies.

Rotate after timeout

A timeout alone bounds one task but does not prove that a client pool has forgotten the state associated with that request. Rebuilding the affected client slot guarantees that later requests lease a new pool generation.

Generation comparison keeps concurrent recovery idempotent:

five requests lease generation 7
first timeout rotates 7 -> 8
remaining generation-7 timeouts observe stale leases and do not rotate again
next request leases generation 8

The old client remains reference-counted for requests already using it; it is not destroyed underneath in-flight work.

Keep TLS policies independent

The existing standard and skip-verification clients had different security behavior. This patch keeps them in independent slots and records the certificate policy needed to rebuild each slot. A timeout in an explicitly no-verify destination does not alter or rotate the normal-verification slot, and vice versa.

GATEWAY_DANGER_ACCEPT_INVALID_CERTS retains its existing global behavior. WebSocket TLS connectors and raw tunnels are unchanged.

Do not replay

The gateway cannot safely infer that a timed-out request was not accepted upstream. Automatically replaying a POST, streamed upload, or other non-idempotent operation could duplicate a side effect. The proposal therefore returns a controlled error, marks it no-automatic-retry, and leaves any explicit retry decision to the caller/application.

This deliberately sacrifices automatic GET recovery in favor of replay safety across all methods.

Known trade-offs and limitations

• Per-slot rather than per-host rotation: one stalled host rotates the shared client slot and discards healthy keep-alive connections for other hosts using that TLS policy. Requests continue to work, but may reconnect. Per-host pools could reduce this collateral at the cost of more state and eviction policy; it is not included here.
• Slow request uploads count against the bound: described above. Response bodies after headers do not.
• Already-leased requests: a request can lease a generation before a manual-approval hold. If another request rotates the slot during that hold, the approved request may still attempt once using its retired client. If it also times out, its stale generation check will not rotate the current pool again. Re-leasing immediately before send is a possible refinement.
• Timeout scope: this PR covers upstream reqwest connection/send behavior. It does not add deadlines to credential resolution, OAuth refresh, database access, WebSocket forwarding, or raw CONNECT tunnels.
• No claim about the original trigger: the patch guarantees bounded failure/recovery from the demonstrated state; it does not prove or eliminate every possible initiating network fault.

Observability and sanitization

The new timeout response is static and never echoes:

• upstream URLs or query strings;
• authorization/proxy-authorization headers;
• credential values;
• request/response bodies;
• CA material.

The new timeout event adds only:

• HTTP method;
• host with userinfo and port removed;
• TLS-policy slot;
• leased generation;
• configured timeout;
• whether this event rotated the slot.

Existing surrounding request spans may carry the gateway's normal request metadata; raw field-test logs are therefore not copied into this PR or #493. The issue and PR contain no credentials, agent/project identifiers, private endpoints, authorization headers, private CA material, or raw sensitive traces.

Regression tests added

Focused transport/config tests cover:

• safe 10-second/120-second defaults;
• positive environment-value parsing;
• startup rejection for zero and invalid values;
• one rotation for multiple stale leases;
• independence of verified and no-verify generations.

Focused forwarding tests use real local TCP listeners and cover:

• an upstream that accepts the request and sends no headers is bounded by the helper's own deadline;
• timeout maps to sanitized HTTP 504 upstream_timeout;
• a query marker is not echoed in the timeout body;
• the no-automatic-retry header is present;
• timeout rotates the current generation and the next lease completes against a healthy server;
• five concurrent timeouts rotate one generation once;
• headers arriving before the deadline followed by a body delayed beyond it still succeed;
• a timed-out POST reaches the upstream exactly once;
• observable host logging strips userinfo and port.

Verification results

All commands below were run against the complete uncommitted proposal diff.

cargo fmt --all -- --check
PASS

cargo clippy -- -D warnings
PASS

cargo test gateway::upstream::tests -- --nocapture
6 passed, 0 failed

cargo test gateway::forward::tests -- --nocapture
20 passed, 0 failed

cargo test -- --nocapture
531 passed, 0 failed

pnpm check
9/9 tasks successful
Prettier check passed
Prisma format check passed

pnpm build
Gateway release build passed
Next.js production build passed

git diff --check
PASS

The fresh clone required the repository's documented generation step before monorepo type checking/building:

pnpm db:generate

No production configuration was copied into the source clone.

Official-image build

The repository's official Dockerfile built successfully with the proposal:

docker build -f docker/Dockerfile \
  --build-arg APP_VERSION=1.45.0-upstream-timeout-proposal \
  -t onecli:upstream-timeout-proposal .

End-to-end fault injection

A custom gateway from that image ran as an isolated sidecar on an alternate port. Production remained on its existing port and binary.

A disposable local HTTP server provided deterministic endpoints:

1. first request accepts and withholds headers;
2. second request to the same host returns immediately;
3. response headers return immediately but the response body waits three seconds;
4. POST accepts once and withholds headers while a separate counter records deliveries.

The sidecar used one-second test-only settings. Sanitized results:

first silent request:
  HTTP 504 in 1.021 s
  static sanitized body: yes
  x-should-retry: false
  generation 0 rotated: true

next request to the same host:
  HTTP 200 in 0.006 s

slow response body:
  HTTP 200 in 3.005 s
  complete body received despite one-second send/header bound

timed-out POST:
  HTTP 504 in 1.003 s
  upstream observed request count: exactly 1
  generation 1 rotated: true

The custom sidecar also exercised the existing GitHub connection paths without retaining response bodies or credentials:

GitHub smart-HTTP path: MITM, one injection applied, response received
GitHub API identity path: MITM, one injection applied, HTTP 200

The sidecar, fault server, and temporary binary were then stopped/removed. Production health remained HTTP 200 on both web and gateway health endpoints.

Post-test production controls

The proposal was not deployed to production. The currently released OneCLI process still passed:

default Git fetch through OneCLI: success
GitHub API credential-injection path: HTTP 200
repository-local Git HTTP/TLS overrides: unset
global Git HTTP/TLS overrides: unset

These controls confirm the recovered production service remained healthy; they are not claimed as validation of deployed proposal behavior.

Independent review

A fresh read-only Fable 5 review of the complete diff returned APPROVE with no critical or blocking findings. Its non-blocking trade-offs are disclosed above: request-upload timing, per-slot cross-host pool rotation, pre-approval stale leases, and conservative no-retry behavior.

Maintainer choice

The requested behavior in #493 is:

• no indefinitely pending upstream forward;
• controlled sanitized failure;
• automatic escape from suspect pooled client state;
• no TLS weakening;
• no unsafe request replay;
• no response-body timeout after headers.

This PR is a tested proposal for those properties. The maintainers are explicitly invited to implement #493 differently, adjust the defaults/configuration surface, request a smaller first patch, or use this branch only as a reproducer/reference.