mirror of
https://github.com/QuantumNous/new-api.git
synced 2026-08-31 02:41:34 +00:00
txgo
b518d0033b
* fix(relay): bound the wait for upstream response headers (fixes unbounded heap growth) The relay transport sets a dial timeout, a TLS handshake timeout and an expect-continue timeout, but nothing bounds how long it waits for the upstream *response headers* after the request has been written. An upstream that accepts the connection and then never answers -- without sending FIN/RST, which is what happens when a NAT/firewall silently drops the flow or the provider hangs -- parks the goroutine in net/http.(*persistConn).roundTrip forever. That goroutine keeps the whole request alive, which in practice means three copies of the request body stay reachable for the lifetime of the process: the raw bytes from io.ReadAll in CreateBodyStorageFromReader, the decoded messages held as json.RawMessage, and the re-marshalled upstream body from common.Marshal. BodyStorageCleanup cannot help here: it runs after c.Next() returns, and for these requests c.Next() never returns. Measured on v1.0.0-rc.23 in production (see #6947 for the full evidence): - 23 goroutines stuck in persistConn.roundTrip on a single 40h-old instance, blocked between 353 and 1894 minutes (5.9h to 31.5h) - 96.9% of the live heap, sampled after a forced GC, attributable to those three body copies (HeapAlloc 892 MiB surviving three GC cycles; HeapObjects dropping 30x while bytes dropped only 25%) - the live floor grows with uptime: 33.7 MiB at 0.1h, 89.2 at 13.8h, 510.0 at 40.1h, 955.2 at 146.8h, OOMKilled at 172.9h -- same image, same config, same load Doubling the memory limit and adding GOMEMLIMIT only moved the OOM from 132h to 172.9h. RELAY_TIMEOUT (http.Client.Timeout) cannot be used for this: it covers the whole response read and would cut legitimate long streaming calls, which is why it defaults to 0. ResponseHeaderTimeout only bounds the wait for the headers; streaming after they arrive is unaffected. The default is deliberately generous. Non-streaming upstreams usually send the response headers only once generation has finished, so the value has to leave room for a long completion. 1800s is 12x shorter than the shortest hang observed here while leaving several times the headroom a normal non-streaming request needs; 0 restores the previous unbounded behaviour. The assignment goes next to the other transport.* lines rather than inside the else branch: newRelayHTTPTransport() normally takes the http.DefaultTransport.Clone() path, and DefaultTransport does not set ResponseHeaderTimeout either. This repo already sets ResponseHeaderTimeout on its other outbound transports (controller/model_sync.go, controller/ratio_sync.go); the relay path appears to have been missed. Refs #6947. Likely also the root cause of #6731, which reported the same symptom (production OOM on /v1/responses after ~64h) but was closed for template reasons. * review: clamp overflowing timeout values and switch the test to testify Addresses the two CodeRabbit findings on this PR. Overflow (common/init.go:113): a RELAY_RESPONSE_HEADER_TIMEOUT beyond ~9.2e9 seconds overflows time.Duration and can wrap into a *tiny positive* timeout, which would cut every relay request instead of only the stuck ones. The value is now clamped before the conversion, with regression tests for both the negative and the overflowing input. I did not add fail-on-startup validation for negative values, for two reasons: the existing `if seconds > 0` guard already treats them as "disabled", and the neighbouring env-driven timeouts in this file are less strict still -- RelayIdleConnTimeout is converted with no guard at all. Failing startup on a bad value would be a behaviour change out of step with the rest of the file; happy to add it if you'd prefer that direction repo-wide. Test style: switched to testify (require.Equal / require.Zero / require.Positive), which is what every other test under service/ uses. go build, go vet and go test ./common/... ./service/... pass. (`go build ./...` fails on the `web/dist` embed both with and without this change -- the frontend bundle is not checked in.)