The fixes had the bugs

· 10 min read
Contents

Marking a fix done sounds like the easy part. Write the change, write a test, watch the suite go green, tick the box. I had 45 findings for my LAN proxy for local models, from a round of research into what coding agents and other clients send and what llama.cpp's endpoints expect. I had an afternoon of fixes, and a green suite.

The fixes then went through an independent re-review, pass after pass, each one starting from the findings and the code rather than from my summary of what I'd done. Seven passes found 11, 8, 10, 4, 2, 1 and 0 bugs. They weren't new findings about clients. They were in the fixes, and in the fixes of the fixes, and two of them could take the whole proxy down.

Remove the fix, run the tests#

The rule for "done" was strict: a code path, plus a test that fails when the fix is taken out. Taking it out is mechanical. Each claimed fix gets a mutant, a one-string swap that undoes it, applied to a copy of the package in a directory whose name starts with _, which go test ./... skips. One of the first 32:

("hello-local", "proxy.go", 'case "/api/hello": //', 'case "/api/hello-off": //', False),

That one turns off the local answer to HEAD /api/hello, the probe Claude Code sends at every start, so the request falls through to a GPU box instead. If a test fails, the mutant is killed and the fix is proven. If nothing fails, the fix has no guard. Four of the 32 survived:

hello-local                SURVIVED   no test failed
[…]
early-responses-shape      SURVIVED   no test failed
[…]
acao-on-models             SURVIVED   no test failed
[…]
max-body-64mib             SURVIVED   no test failed

The /api/hello test passed with the fix and without it, because its only fake host was up and answered the forwarded probe with a 200 as well. The test asserted the right status for the wrong reason. A new test uses two hosts and no default host, so only the proxy itself can answer, and the mutant dies.

A field for pi, an open connection for Zed#

The first pass's worst bug came from a fix that worked. pi's llama.cpp mode rejects a model list unless every entry has a status field, so the proxy started adding one. Zed reads the same field to decide whether it is talking to a llama.cpp router:

/// Whether this entry came from a server running in router mode.
pub fn is_router_entry(&self) -> bool {
    self.status.is_some()
}

and a router gets a subscription: Zed holds GET /models/sse open and reconnects five seconds after it ends. The proxy counted every forwarded request as in flight, including that one, and an engine switch waits until nothing is in flight (why it waits). With Zed open on a laptop, a request for a model on another engine would wait out the full 15-minute budget and get an error, and idle stop would never run, so the benchmark queue on that box would never get its GPU back. Nobody ran Zed against it; the pass found it in Zed's source and pinned it with a test. Read-only requests stopped counting as in flight.

The fix for starvation brought the ping-pong back#

That pass also found, by reading the code, that a waiting switch could starve: under steady traffic for the running engine, in-flight could stay above zero for good. The fix made new requests for the current engine queue behind a waiting switch. The second pass found what that did to the 30-second hold-down, which exists so that two clients on two engines can't flip a box back and forth. The repro test, asserting the behaviour of that version:

    pass2_test.go:298: REPRO: a second flint request inside fork's hold-down was held; started [a/fork a/mainline a/fork]
--- PASS: TestP2_HoldDownNoLongerProtectsTheFreshEngine (0.41s)

A second Flint request arrived while the fork engine, the llama.cpp build that serves Flint, was inside its hold-down, was held because a switch to mainline was waiting, and ran only after the box had gone to mainline and back: one switch per request, the exact pattern the hold-down was there to stop. The same pass found that a client which gave up while its switch waited still moved the GPU, for nobody. Both fixes came with tests that fail without them.

Two ways to crash a Go proxy from a goroutine#

The third pass reviewed fixes plus a batch of new features, and found 10 bugs; that is the bump in the 11, 8, 10, 4, 2, 1, 0. The worst was in the keepalive. A streaming request that waits on a silent upstream gets SSE comments from a ticker goroutine. When the handler finished, it closed the ticker's stop channel, but a ticker that had already left its select would still take the lock, see that the upstream never answered, and write a comment into a response whose handler was gone. Under -race that's a data race against net/http's own buffer pool. Without it, in five of nine runs:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x5077b2]

goroutine 1804 [running]:
bufio.(*Writer).Flush(0x800?)
	~/.local/opt/go/src/bufio/bufio.go:636 +0x12
net/http.(*chunkWriter).flush(0x53c4fcc67c0?)
	~/.local/opt/go/src/net/http/server.go:405 +0x39
net/http.(*response).FlushError(0x53c4faab680)
	~/.local/opt/go/src/net/http/server.go:1729 +0x4b
net/http.(*response).Flush(0x1?)
	~/.local/opt/go/src/net/http/server.go:1721 +0x13
[…]

The frames below those are the proxy's own: the log recorder's Flush, then upstreamWait.flush, comment and loop, in the goroutine newUpstreamWait started. A panic in a goroutine nobody recovers ends the process, and every stream through the proxy with it. The window per abandoned wait is microseconds; with real 15-second ticks it would have been rare. The fix is a done flag that the handler sets under the same lock and the ticker checks right after taking it.

The fourth pass found the other crash, in the fix for a third-pass bug. The WebUI's stream lookup asked the GPU boxes one after another with no timeout, so it got parallel, two seconds per box, by running the existing httputil.ReverseProxy in a goroutine per box:

for host, ids := range perHost {
	wg.Add(1)
	go func() {
		defer wg.Done()
		ctx, cancel := context.WithTimeout(r.Context(), p.lookupWait)
		defer cancel()
		b, _ := json.Marshal(map[string]any{"conversation_ids": ids})
		sub := httptest.NewRecorder()
		out, _ := http.NewRequestWithContext(ctx, http.MethodPost, "/v1/streams/lookup", bytes.NewReader(b))
		out.Header.Set("Content-Type", "application/json")
		p.proxies[host].ServeHTTP(sub, out)

When an upstream stalls in the middle of a body, ReverseProxy gives up by panicking with http.ErrAbortHandler, but only if it thinks a server will recover the panic. This is how it decides:

func shouldPanicOnCopyError(req *http.Request) bool {
	if inOurTests {
		// Our tests know to handle this panic.
		return true
	}
	if req.Context().Value(http.ServerContextKey) != nil {
		// We seem to be running under an HTTP server, so
		// it'll recover the panic.
		return true
	}
	// Otherwise act like Go 1.10 and earlier to not break
	// existing tests.
	return false
}

It asks the request's context, not the goroutine. The sub-request's context came from the client's request, so it carried http.ServerContextKey, and the panic went off in a plain goroutine that no HTTP server was watching:

2026/10/24 21:34:33 httputil: ReverseProxy read error during body copy: context deadline exceeded
panic: net/http: abort Handler

goroutine 68 [running]:
net/http/httputil.(*ReverseProxy).ServeHTTP(0x358e701f8690, {0x8995f0, 0x358e701a70c0}, 0x358e7020d680)
	~/.local/opt/go/src/net/http/httputil/reverseproxy.go:612 +0x1345
[…]

The next frame down is the lookup's own goroutine, lookupStreams.func1. Before that change, the same panic ran on the handler's goroutine and net/http swallowed it. The lookup now uses a plain http.Client with a deadline. The fifth pass then noticed that this new client, unlike the reverse proxy, would honour HTTP_PROXY from the environment. Nothing set it, so it was latent, and it was fixed too.

A fake upstream that never sees the client leave#

Tests have their own way to fail: by hanging instead. A fake GPU box that blocks until the client goes away (<-r.Context().Done()) never wakes up if it doesn't read the request body first. net/http starts the background read that notices a closed connection only when the body has been read to the end:

if requestBodyRemains(req.Body) {
	registerOnHitEOF(req.Body, w.conn.r.startBackgroundRead)
} else {
	w.conn.r.startBackgroundRead()
}

A third-pass probe built on such a fake couldn't even be rerun in the fourth: cleanup waited on a handler that would never return, because httptest.Server.Close waits for every active request. The house rule now: a blocking fake reads the body and is released in a defer. The third pass also noted a test that caught its mutant in only 7 of 10 runs: it depended on timing, not on the fix.

When to stop#

After the fifth pass nothing above LOW was left in the bug lists, but three findings were still partial, two of them with work left. They got their own change and a sixth pass, which found one more LOW: a client that left while queued behind another request's switch still counted in the queue. Its first 100-run flake check never finished. It was running in the tmux pane that an unrelated OOM kill took down that night, which has its own post; the rerun, in its own systemd unit, passed. The LOW got a fix with the pass's tests ported, and a seventh pass over that fix found nothing new: nine of its ten mutants died to the shipped tests, and the tenth, which only changes the queue place shown to waiting clients and on /status, to the pass's own test. Those tests went into the release that followed.

That is the stop rule, and it's a dull one: a pass that finds nothing above LOW, and totals that don't move. As of 23:30 on 24 October the review's tally reads 42 of the 45 findings done and 3 needing no change, after seven passes and 170 mutants, 136 of them killed. Most survivors became tests and a few guarded checks that turned out redundant. A handful are still untested, every one of them LOW.

The suite was green going into every one of those passes, including the two with a crash in them. Green said the tests passed. It never said which fixes they tested.

Filed under: go, testing, proxy, concurrency, code-review

Sources#