Skip to content

Commit 5ae90de

Browse files
Psych0h3adkjlubick
andauthored
Fix HTTP connection reuse in engine health probes (#37)
## Description Periodic engine health checks close HTTP response bodies without reading them. With Go 1.26.7, this prevents the HTTP/1 transport from reusing those connections and opens a new connection for every poll. Draining ordinary responses before closing reduces unnecessary connection churn while PAIR monitors Ollama and LM Studio. The regression tests reproduce 32 connections for 32 polls before the fix and one connection for the same 32 polls after it, for both Content-Length and chunked responses across all three health-check paths. ## Release intent <!-- Keep the fences exactly as they are; CI parses between them. --> <!-- Set each bump to none, patch, minor, or major. Leave the key names. --> <!-- `services` must be >= the highest component bump. --> <!-- Set the title and body to n/a when no bump is a release. --> <!-- The release version users see is NOT declared here: it patch-bumps by itself whenever any bump above is a release. --> <!-- Keep the changelog body to prose and bullets. A line starting with "### " ends the section and silently truncates the rest. --> <!-- pair-release-intent:v1 --> ### Changelog title Fix HTTP connection reuse in engine health probes ### Changelog body Periodic engine monitoring now reuses HTTP/1 connections instead of opening a new connection for every health probe. ### Bumps - services: patch - nvpair-cluster-manager: none - nvpair-engine-manager: patch - nvpair-errors: none - nvpair-job-scheduler: none - nvpair-manual-nodes: none - nvpair-node-info: none - nvpair-node-scanner: none - nvpair-node-settings: none - nvpair-proxy: none - nvpair-tui: none - nvpair-ui-broker: patch - nvpair-workload-manager: none <!-- /pair-release-intent:v1 --> ## Scope - Drain response bodies before closing in the broker's Ollama and LM Studio checks and the engine manager's HTTP probe, including unsuccessful HTTP statuses. - Limit each drain to 1 MiB and retain the existing broker client timeout of 2 seconds and engine-manager probe deadline of 3 seconds. - Add tests for HTTP/1 connection reuse, unchanged status-based health decisions, bounded reads, and stalled-body deadlines. - Bump `nvpair-ui-broker` from 0.40.2 to 0.40.3 and `nvpair-engine-manager` from 0.17.4 to 0.17.5. No API, configuration, dependency, or desktop changes are required. This change addresses connection churn; it does not reset existing TCP state or establish a cause for any operating-system TCP reclamation failure. ## Validation After rebasing onto `develop`, the following passed on Windows amd64 with Go 1.27.0: - `go test ./... -count=1` from `services/shared` - `go test ./... -count=1` from `services/nvpair-engine-manager` - `go test ./... -count=1` from `services/nvpair-ui-broker` - Focused connection-reuse, bounded-drain, and deadline regression tests in all three affected modules - `node scripts/spdx-headers.mjs` — 1,023 files checked, with no missing, review-required, or unclassified headers - `git diff --check upstream/develop..HEAD` The original targeted validation also passed on macOS arm64 with Go 1.26.7 and `CGO_ENABLED=0`. No live engine operation was required because the regression tests exercise the real `net/http` HTTP/1 client and server over `net.Pipe` without consuming TCP source ports. ## Risk Health results continue to depend on the HTTP status, including when a body read fails. A stalled response can now keep the check active until the existing timeout or deadline, and an oversized or incomplete body can still cause the connection to be discarded. The drain is bounded in size and time and discards content without logging it. No data migration or wire-format change is involved. ## Checklist - [X] I have read the [Contributing Guidelines](https://github.com/NVIDIA/Personal-AI-Router/blob/main/CONTRIBUTING.md). - [X] Every commit is signed off (`git commit -s`), certifying the [Developer Certificate of Origin](https://developercertificate.org/). - [X] New or existing tests cover the change. - [X] Relevant documentation is updated. - [X] I checked the diff, changed filenames, and commit messages for credentials, private data, internal URLs, internal issue identifiers, and generated artifacts. - [X] I recorded the validation commands and results above. - [X] I declared version bumps in the release-intent block above. `services/versions.json` is written by automation — do not edit it by hand. --------- Signed-off-by: Psych0h3ad <41975091+Psych0h3ad@users.noreply.github.com> Signed-off-by: Kaylee Lubick <klubick@nvidia.com> Co-authored-by: Kaylee Lubick <klubick@nvidia.com>
1 parent cdc3e30 commit 5ae90de

7 files changed

Lines changed: 302 additions & 2 deletions

File tree

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package main
5+
6+
import (
7+
"context"
8+
"io"
9+
"net/http"
10+
"strings"
11+
"sync/atomic"
12+
"testing"
13+
"time"
14+
15+
"nvpair-shared/httpcon/testclient"
16+
)
17+
18+
func TestProbeHTTPReusesConnections(t *testing.T) {
19+
test := func(name string, chunked bool) {
20+
t.Run(name, func(t *testing.T) {
21+
var requests atomic.Int32
22+
client, connections := testclient.New(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
23+
if r.Header.Get(engineIdentityProbeHeader) != "1" {
24+
t.Error("missing engine identity header")
25+
}
26+
status := http.StatusOK
27+
if requests.Add(1)%2 == 0 {
28+
status = http.StatusServiceUnavailable
29+
}
30+
w.WriteHeader(status)
31+
if chunked {
32+
_ = http.NewResponseController(w).Flush()
33+
}
34+
_, _ = io.WriteString(w, `{"models":[],"status":"responding"}`)
35+
}))
36+
ex := &Executor{client: client}
37+
probe := &Probe{HTTP: "http://127.0.0.1:{port}/api/version"}
38+
const rounds = 32
39+
for i := 0; i < rounds; i++ {
40+
if got, want := ex.probe(context.Background(), probe, 1), i%2 == 0; got != want {
41+
t.Fatalf("poll %d: healthy = %v, want %v", i, got, want)
42+
}
43+
}
44+
if got := connections.Count(); got != 1 {
45+
t.Fatalf("accepted %d HTTP/1 connections for %d polls, want 1", got, rounds)
46+
}
47+
})
48+
}
49+
50+
test("content-length", false)
51+
test("chunked", true)
52+
}
53+
54+
func TestProbeHTTPBoundsBodyDrain(t *testing.T) {
55+
body := &healthProbeBody{reader: strings.NewReader(strings.Repeat("x", 4<<20))}
56+
ex := &Executor{client: &http.Client{Transport: healthProbeTransport(func(*http.Request) (*http.Response, error) {
57+
return &http.Response{StatusCode: http.StatusAccepted, Body: body, Header: make(http.Header)}, nil
58+
})}}
59+
probe := &Probe{HTTP: "http://127.0.0.1:{port}/", Status: http.StatusAccepted}
60+
if !ex.probe(context.Background(), probe, 1) {
61+
t.Fatal("body drain changed the configured HTTP status health result")
62+
}
63+
if body.read == 0 || body.read > 1<<20 || !body.closed {
64+
t.Fatalf("body read = %d, closed = %v; want bounded drain and close", body.read, body.closed)
65+
}
66+
}
67+
68+
func TestProbeHTTPBodyDrainHonorsDeadline(t *testing.T) {
69+
var body *healthProbeBody
70+
ex := &Executor{client: &http.Client{Transport: healthProbeTransport(func(req *http.Request) (*http.Response, error) {
71+
body = &healthProbeBody{ctx: req.Context()}
72+
return &http.Response{StatusCode: http.StatusOK, Body: body, Header: make(http.Header)}, nil
73+
})}}
74+
probe := &Probe{HTTP: "http://127.0.0.1:{port}/"}
75+
ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
76+
defer cancel()
77+
start := time.Now()
78+
if !ex.probe(ctx, probe, 1) {
79+
t.Fatal("body drain changed the HTTP status health result")
80+
}
81+
if !body.attempted || !body.closed || time.Since(start) > time.Second {
82+
t.Fatalf("stalled body did not drain and close within deadline: %+v", body)
83+
}
84+
}
85+
86+
type healthProbeTransport func(*http.Request) (*http.Response, error)
87+
88+
func (f healthProbeTransport) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) }
89+
90+
type healthProbeBody struct {
91+
reader io.Reader
92+
ctx context.Context
93+
read int
94+
attempted bool
95+
closed bool
96+
}
97+
98+
func (b *healthProbeBody) Read(p []byte) (int, error) {
99+
b.attempted = true
100+
if b.ctx != nil {
101+
<-b.ctx.Done()
102+
return 0, b.ctx.Err()
103+
}
104+
n, err := b.reader.Read(p)
105+
b.read += n
106+
return n, err
107+
}
108+
109+
func (b *healthProbeBody) Close() error { b.closed = true; return nil }

‎services/nvpair-engine-manager/lifecycle.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ import (
1919
"sync"
2020
"syscall"
2121
"time"
22+
23+
"nvpair-shared/httpcon"
2224
)
2325

2426
const (
@@ -871,7 +873,8 @@ func (e *Executor) probe(ctx context.Context, p *Probe, port int) bool {
871873
if err != nil {
872874
return false
873875
}
874-
resp.Body.Close()
876+
877+
httpcon.DrainAndClose(resp.Body)
875878
want := p.Status
876879
if want == 0 {
877880
want = 200

‎services/nvpair-ui-broker/advertiser.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"net/http"
1111
"time"
1212

13+
"nvpair-shared/httpcon"
1314
"nvpair-shared/noderec"
1415
)
1516

@@ -274,6 +275,6 @@ func checkEngineHealth(profile engineProxyProfile, client *http.Client, port int
274275
if err != nil {
275276
return false
276277
}
277-
resp.Body.Close()
278+
httpcon.DrainAndClose(resp.Body)
278279
return resp.StatusCode == http.StatusOK
279280
}
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package main
5+
6+
import (
7+
"io"
8+
"net/http"
9+
"sync/atomic"
10+
"testing"
11+
12+
"nvpair-shared/httpcon/testclient"
13+
)
14+
15+
func TestHealthChecksReuseConnections(t *testing.T) {
16+
test := func(name, path string, profile engineProxyProfile) {
17+
t.Run(name, func(t *testing.T) {
18+
testFraming := func(name string, chunked bool) {
19+
t.Run(name, func(t *testing.T) {
20+
var requests atomic.Int32
21+
client, connections := testclient.New(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
22+
if r.URL.Path != path {
23+
t.Errorf("path = %q, want %q", r.URL.Path, path)
24+
}
25+
status := http.StatusOK
26+
if requests.Add(1)%2 == 0 {
27+
status = http.StatusServiceUnavailable
28+
}
29+
w.WriteHeader(status)
30+
if chunked {
31+
_ = http.NewResponseController(w).Flush()
32+
}
33+
_, _ = io.WriteString(w, `{"models":[],"status":"responding"}`)
34+
}))
35+
const rounds = 32
36+
for i := 0; i < rounds; i++ {
37+
if got, want := checkEngineHealth(profile, client, 1), i%2 == 0; got != want {
38+
t.Fatalf("poll %d: healthy = %v, want %v", i, got, want)
39+
}
40+
}
41+
if got := connections.Count(); got != 1 {
42+
t.Fatalf("accepted %d HTTP/1 connections for %d polls, want 1", got, rounds)
43+
}
44+
})
45+
}
46+
47+
testFraming("content-length", false)
48+
testFraming("chunked", true)
49+
})
50+
}
51+
52+
test("ollama", "/", ollamaProxyProfile)
53+
test("lmstudio", "/v1/models", lmstudioProxyProfile)
54+
}

‎services/shared/httpcon/httpcon.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
// Package httpcon provides helpers for HTTP connections.
5+
package httpcon
6+
7+
import "io"
8+
9+
const maxDrainBytes = 1 << 20
10+
11+
// DrainAndClose discards a bounded response body before closing it so normal
12+
// HTTP/1 responses reach EOF and their connections can return to the transport
13+
// pool. Without this, connections cannot be reused and this can lead to socket
14+
// exhaustion. The caller is responsible for bounding stalled reads with a
15+
// request context or client timeout.
16+
func DrainAndClose(body io.ReadCloser) {
17+
_, _ = io.Copy(io.Discard, io.LimitReader(body, maxDrainBytes))
18+
_ = body.Close()
19+
}
Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
// Package testclient provides in-memory HTTP clients for connection tests.
5+
package testclient
6+
7+
import (
8+
"context"
9+
"net"
10+
"net/http"
11+
"sync"
12+
"sync/atomic"
13+
"testing"
14+
"time"
15+
)
16+
17+
// ConnectionCounter counts connections accepted by a client returned by New.
18+
type ConnectionCounter struct {
19+
accepted atomic.Int32
20+
}
21+
22+
// Count returns the number of HTTP/1 connections accepted by the test server.
23+
func (c *ConnectionCounter) Count() int32 {
24+
return c.accepted.Load()
25+
}
26+
27+
// New returns an HTTP client backed by an in-memory server and a counter for
28+
// the connections that server accepts. It avoids consuming TCP source ports,
29+
// so connection-reuse regressions remain testable after port exhaustion.
30+
func New(t *testing.T, handler http.Handler) (*http.Client, *ConnectionCounter) {
31+
t.Helper()
32+
listener := &pipeListener{conns: make(chan net.Conn), done: make(chan struct{})}
33+
server := &http.Server{Handler: handler}
34+
go func() { _ = server.Serve(listener) }()
35+
t.Cleanup(func() { _ = server.Close() })
36+
37+
connections := &ConnectionCounter{}
38+
transport := &http.Transport{DialContext: func(ctx context.Context, _, _ string) (net.Conn, error) {
39+
clientConn, serverConn := net.Pipe()
40+
select {
41+
case listener.conns <- serverConn:
42+
connections.accepted.Add(1)
43+
return clientConn, nil
44+
case <-ctx.Done():
45+
_ = clientConn.Close()
46+
_ = serverConn.Close()
47+
return nil, ctx.Err()
48+
}
49+
}}
50+
client := &http.Client{Transport: transport, Timeout: 2 * time.Second}
51+
t.Cleanup(client.CloseIdleConnections)
52+
return client, connections
53+
}
54+
55+
type pipeListener struct {
56+
conns chan net.Conn
57+
done chan struct{}
58+
once sync.Once
59+
}
60+
61+
func (l *pipeListener) Accept() (net.Conn, error) {
62+
select {
63+
case conn := <-l.conns:
64+
return conn, nil
65+
case <-l.done:
66+
return nil, net.ErrClosed
67+
}
68+
}
69+
70+
func (l *pipeListener) Close() error {
71+
l.once.Do(func() { close(l.done) })
72+
return nil
73+
}
74+
75+
func (l *pipeListener) Addr() net.Addr { return &net.TCPAddr{Port: 1} }
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package testclient
5+
6+
import (
7+
"io"
8+
"net/http"
9+
"testing"
10+
11+
"nvpair-shared/httpcon"
12+
)
13+
14+
func TestConnectionCounterCountsDistinctConnections(t *testing.T) {
15+
client, connections := New(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
16+
_, _ = io.WriteString(w, "ok")
17+
}))
18+
19+
const requests = 2
20+
for i := 0; i < requests; i++ {
21+
req, err := http.NewRequest(http.MethodGet, "http://example.test/", nil)
22+
if err != nil {
23+
t.Fatal(err)
24+
}
25+
// Request.Close makes the transport reject connection reuse before it
26+
// handles the response body. This forces a new connection so the
27+
// counter proves it can count two of them.
28+
req.Close = true
29+
resp, err := client.Do(req)
30+
if err != nil {
31+
t.Fatal(err)
32+
}
33+
httpcon.DrainAndClose(resp.Body)
34+
}
35+
36+
if got := connections.Count(); got != requests {
37+
t.Fatalf("accepted %d HTTP/1 connections, want %d", got, requests)
38+
}
39+
}

0 commit comments

Comments
 (0)