From d0707d585bd7e8b4157139d7096cc2f8e50ee384 Mon Sep 17 00:00:00 2001 From: Peter Honeder Date: Mon, 31 Aug 2026 10:26:55 +0300 Subject: [PATCH 1/2] Harden CI and make its lint step actually lint The check job ran pull request code with a writable token, no time limit, and actions on mutable tags. Its lint step ran no linter at all. .github/workflows/ci.yml: - Read-only token. - persist-credentials: false, so the token is not left in .git/config. - timeout-minutes on both jobs, instead of the 6 hour default. - Actions pinned to commit SHAs. - Force-pushing a PR cancels the superseded run. - Installs golangci-lint from a pinned, checksummed tarball. The runner image does not ship one. Makefile: make lint now fails when golangci-lint is missing and CI is set, instead of falling back to go vet. .golangci.yml: new. Removes the default output limits, which cap findings at 3 of a kind and 50 per linter and keep only one per line. .github/dependabot.yml: new. Keeps the pins current. 23 lint fixes, so the job passes. Mostly unchecked error returns from Close and Fprintf. Three others: tint.NewHandler is deprecated, replaced with tint.NewTextHandler; a deliberately discarded config.Load return now uses _; one switch is now tagged. Fork PRs are gated by a repository setting rather than this file. It is set to require approval for all outside collaborators. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FnZG8whBis7HMNVNAWFxDr --- .github/dependabot.yml | 28 +++++++++++++++++++ .github/workflows/ci.yml | 46 ++++++++++++++++++++++++++++--- .golangci.yml | 10 +++++++ Makefile | 6 +++- cmd/demo/main.go | 12 ++++---- cmd/llm-proxy/main.go | 8 +++--- internal/config/main_test.go | 2 +- internal/obs/logging.go | 2 +- internal/proxy/auth_test.go | 4 +-- internal/proxy/handler.go | 8 +++--- internal/proxy/main_test.go | 2 +- internal/proxy/routing_test.go | 4 +-- internal/testutil/fakeupstream.go | 8 +++--- 13 files changed, 110 insertions(+), 30 deletions(-) create mode 100644 .github/dependabot.yml create mode 100644 .golangci.yml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..e590722 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,28 @@ +version: 2 + +updates: + # The workflow pins actions to commit SHAs, which is the point — a mutable + # tag is a stranger's push away from running in CI. Pinning without this + # file just trades a supply-chain hole for a stale one, so let Dependabot + # move the pins; it rewrites the SHA and the trailing version comment + # together. + - package-ecosystem: github-actions + directory: / + schedule: + interval: weekly + day: monday + # Two first-party actions. Separate PRs for them is noise, not signal. + groups: + actions: + patterns: ["*"] + + - package-ecosystem: gomod + directory: / + schedule: + interval: weekly + day: monday + groups: + # Patch and minor bumps land as one PR that CI either passes or does + # not. Majors stay separate, because those are the ones worth reading. + go-minor: + update-types: [minor, patch] diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 43f5e19..1300f8d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,32 +11,70 @@ on: - cron: "0 6 * * 1" workflow_dispatch: +# go test runs arbitrary code from the PR, so its token gets read and nothing +# else. Also set in repo settings; this copy is the one that shows up in a diff. +permissions: + contents: read + +# Cancel a superseded PR run on force-push. Not main or the weekly sweep: +# each of those runs is the record for one commit. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + jobs: check: runs-on: ubuntu-latest + # The default is 6 hours. The race suite takes a couple of minutes. + timeout-minutes: 15 + env: + # The runner image ships no golangci-lint, and `make lint` used to fall + # back to `go vet` when it was missing, so this step passed for months + # without linting. Pinned like gitleaks, and to the version developers + # run locally, so CI and a laptop disagree only when the code differs. + GOLANGCI_VERSION: 2.13.2 + GOLANGCI_SHA256: 2277d43b98ec0054280f2ac26b53268bae97682444678a59a657dd565da021d6 steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # Without this the job's token is left behind in .git/config, where + # the pull request's own test code can read it. + persist-credentials: false - - uses: actions/setup-go@v7 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version-file: go.mod - # fmt + vet + lint + the full suite under -race. + - name: Install golangci-lint + run: | + set -euo pipefail + tarball="golangci-lint-${GOLANGCI_VERSION}-linux-amd64.tar.gz" + curl -sSfL -o "$tarball" \ + "https://github.com/golangci/golangci-lint/releases/download/v${GOLANGCI_VERSION}/${tarball}" + echo "${GOLANGCI_SHA256} ${tarball}" | sha256sum -c - + tar -xzf "$tarball" --strip-components=1 \ + "golangci-lint-${GOLANGCI_VERSION}-linux-amd64/golangci-lint" + sudo install golangci-lint /usr/local/bin/golangci-lint + + # fmt + vet + lint + the full suite under -race. CI is set by Actions, so + # a missing linter fails here instead of quietly downgrading to go vet. - run: make check secrets: runs-on: ubuntu-latest + timeout-minutes: 10 env: # Pinned and checksummed. An unpinned scanner is a supply-chain hole in # the one job whose purpose is supply-chain hygiene. GITLEAKS_VERSION: 8.30.1 GITLEAKS_SHA256: 551f6fc83ea457d62a0d98237cbad105af8d557003051f41f3e7ca7b3f2470eb steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: # gitleaks walks real history; the default shallow clone gives it one # commit and nothing to walk. fetch-depth: 0 + persist-credentials: false - name: Install gitleaks run: | diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..f8553c8 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,10 @@ +version: "2" + +issues: + # The defaults stop at 3 of a kind and 50 per linter, which quietly turns a + # full run into a sample. A gate has to report everything it found. + max-issues-per-linter: 0 + max-same-issues: 0 + # One issue per line by default, so a second linter's finding on an already + # flagged line is dropped. That hid an SA1019 deprecation behind a govet hint. + uniq-by-line: false diff --git a/Makefile b/Makefile index 2d57761..815079b 100644 --- a/Makefile +++ b/Makefile @@ -64,11 +64,15 @@ bench: vet: go vet ./... -## lint: golangci-lint when installed, otherwise go vet +## lint: golangci-lint when installed, otherwise go vet (an error under CI) .PHONY: lint lint: ifdef GOLANGCI golangci-lint run ./... +else ifdef CI + @echo "golangci-lint is not installed; the go vet fallback would report a" >&2 + @echo "green lint step that linted almost nothing. Install it in the workflow." >&2 + @exit 1 else @echo "golangci-lint not installed; falling back to go vet (make tools to install)" @go vet ./... diff --git a/cmd/demo/main.go b/cmd/demo/main.go index ec9a2e9..ec999cb 100644 --- a/cmd/demo/main.go +++ b/cmd/demo/main.go @@ -252,14 +252,14 @@ func streamHandler(n int, end ending, finish string) http.HandlerFunc { if err != nil { return } - defer conn.Close() + defer func() { _ = conn.Close() }() writeHead(rw) for i := 0; i < n; i++ { frame := fmt.Sprintf( `data: {"id":"chatcmpl-demo","model":"demo-model","choices":[{"index":0,"delta":{"content":"tok%d"}}]}`+"\n\n", i) if end == endTruncate && i == n-1 { - fmt.Fprintf(rw, "%x\r\n%s", len(frame), frame[:len(frame)/2]) + _, _ = fmt.Fprintf(rw, "%x\r\n%s", len(frame), frame[:len(frame)/2]) _ = rw.Flush() return } @@ -291,7 +291,7 @@ func slowFirstToken(think time.Duration) http.HandlerFunc { if err != nil { return } - defer conn.Close() + defer func() { _ = conn.Close() }() writeHead(rw) time.Sleep(think) @@ -311,7 +311,7 @@ func errorMidStream() http.HandlerFunc { if err != nil { return } - defer conn.Close() + defer func() { _ = conn.Close() }() writeHead(rw) writeChunk(rw, `data: {"choices":[{"index":0,"delta":{"content":"partial answer"}}]}`+"\n\n") writeChunk(rw, `data: {"error":{"message":"the model backend crashed","type":"server_error"}}`+"\n\n") @@ -358,7 +358,7 @@ func writeHead(rw *bufio.ReadWriter) { } func writeChunk(rw *bufio.ReadWriter, data string) { - fmt.Fprintf(rw, "%x\r\n%s\r\n", len(data), data) + _, _ = fmt.Fprintf(rw, "%x\r\n%s\r\n", len(data), data) _ = rw.Flush() } @@ -370,7 +370,7 @@ func hangUpMidStream(addr string) { return } body := `{"model":"demo-model","stream":true,"messages":[{"role":"user","content":"hello"}]}` - fmt.Fprintf(conn, "POST /demo/v1/chat/completions HTTP/1.1\r\nHost: demo\r\n"+ + _, _ = fmt.Fprintf(conn, "POST /demo/v1/chat/completions HTTP/1.1\r\nHost: demo\r\n"+ "Content-Type: application/json\r\nContent-Length: %d\r\n\r\n%s", len(body), body) buf := make([]byte, 4096) diff --git a/cmd/llm-proxy/main.go b/cmd/llm-proxy/main.go index fe928b8..632fbc5 100644 --- a/cmd/llm-proxy/main.go +++ b/cmd/llm-proxy/main.go @@ -57,7 +57,7 @@ func run() error { fs.BoolVar(&f.check, "check", false, "validate the config, print the route table and exit") fs.BoolVar(&f.showVersion, "version", false, "print the version and exit") fs.Usage = func() { - fmt.Fprintf(fs.Output(), "llm-proxy — an OpenAI-compatible wire-level debugging proxy\n\nusage: llm-proxy [flags]\n\n") + _, _ = fmt.Fprintf(fs.Output(), "llm-proxy — an OpenAI-compatible wire-level debugging proxy\n\nusage: llm-proxy [flags]\n\n") fs.PrintDefaults() } if err := fs.Parse(os.Args[1:]); err != nil { @@ -75,7 +75,7 @@ func run() error { " see llm-proxy.example.yaml for a starting point") } - cfg, warnings, err := config.Load(path) + cfg, _, err := config.Load(path) if err != nil { return fmt.Errorf("config %s:\n%w", path, err) } @@ -83,7 +83,7 @@ func run() error { // Re-validate after the flags, and keep *these* warnings: -listen can move // the proxy onto a public address, and the warning about an unguarded // listener has to reflect where it will actually listen. - warnings, err = cfg.Validate() + warnings, err := cfg.Validate() if err != nil { return fmt.Errorf("config %s:\n%w", path, err) } @@ -187,7 +187,7 @@ func printRouteTable(c *config.Config, path string, warnings []config.Warning) { } for _, r := range c.Routes { - auth := "none" + var auth string switch { case r.APIKeyEnv == "": auth = "client-supplied only" diff --git a/internal/config/main_test.go b/internal/config/main_test.go index 37686ea..0fd68ef 100644 --- a/internal/config/main_test.go +++ b/internal/config/main_test.go @@ -10,6 +10,6 @@ import ( // llm-proxy is likely to have it exported — and without this every test that // assumes an unguarded proxy would fail on their machine and pass in CI. func TestMain(m *testing.M) { - os.Unsetenv("LLM_PROXY_TOKENS") + _ = os.Unsetenv("LLM_PROXY_TOKENS") os.Exit(m.Run()) } diff --git a/internal/obs/logging.go b/internal/obs/logging.go index 3b58d2e..93d5060 100644 --- a/internal/obs/logging.go +++ b/internal/obs/logging.go @@ -60,7 +60,7 @@ func NewLogger(o Options) *Logger { var consoleHandler slog.Handler if pretty { - consoleHandler = tint.NewHandler(out, &tint.Options{ + consoleHandler = tint.NewTextHandler(out, &tint.Options{ Level: level, TimeFormat: "15:04:05.000", NoColor: !color, diff --git a/internal/proxy/auth_test.go b/internal/proxy/auth_test.go index ab0b261..e6bfa66 100644 --- a/internal/proxy/auth_test.go +++ b/internal/proxy/auth_test.go @@ -32,7 +32,7 @@ func TestUnauthenticatedRequestIsRejected(t *testing.T) { if err != nil { t.Fatal(err) } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusUnauthorized { t.Fatalf("Status = %d, want 401", resp.StatusCode) @@ -154,7 +154,7 @@ func TestWrongTokenIsRejected(t *testing.T) { if err != nil { t.Fatal(err) } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusUnauthorized { t.Errorf("Status = %d, want 401", resp.StatusCode) diff --git a/internal/proxy/handler.go b/internal/proxy/handler.go index 45eabd0..b19fdca 100644 --- a/internal/proxy/handler.go +++ b/internal/proxy/handler.go @@ -360,7 +360,7 @@ func (h *routeHandler) deliver( cancelUp context.CancelCauseFunc, prefix []byte, ) { - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() streaming := isEventStream(resp.Header) analyzer := analyze.New(h.srv.now) @@ -540,8 +540,8 @@ func (h *routeHandler) postmortem(rec *record.Request, status int, a *record.Att if status >= 400 && rec.Fault() == nil { env := oaierr.Parse(errBody) - switch { - case status == http.StatusTooManyRequests: + switch status { + case http.StatusTooManyRequests: rec.SetFault(statusFault(fault.KindRateLimited, status, env)) default: if which, ok := h.srv.ctxMatchers.Match(env, errBody); ok { @@ -776,7 +776,7 @@ func decodeForDisplay(body []byte, encoding string) []byte { if err != nil { return body } - defer zr.Close() + defer func() { _ = zr.Close() }() // A capped peek is usually a partial gzip stream, so a read error still // leaves whatever decoded successfully. out, _ := io.ReadAll(io.LimitReader(zr, maxErrorBody)) diff --git a/internal/proxy/main_test.go b/internal/proxy/main_test.go index 3676125..a16e187 100644 --- a/internal/proxy/main_test.go +++ b/internal/proxy/main_test.go @@ -8,6 +8,6 @@ import ( // See the note in internal/config: the suite must not inherit the operator's // LLM_PROXY_TOKENS, or every test would get a 401 from its own proxy. func TestMain(m *testing.M) { - os.Unsetenv("LLM_PROXY_TOKENS") + _ = os.Unsetenv("LLM_PROXY_TOKENS") os.Exit(m.Run()) } diff --git a/internal/proxy/routing_test.go b/internal/proxy/routing_test.go index 9ce9fe0..794ade7 100644 --- a/internal/proxy/routing_test.go +++ b/internal/proxy/routing_test.go @@ -147,7 +147,7 @@ func TestUnknownRouteReturnsParseableError(t *testing.T) { if err != nil { t.Fatal(err) } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusNotFound { t.Errorf("Status = %d, want 404", resp.StatusCode) @@ -189,7 +189,7 @@ func TestHealthAndRoutesEndpoints(t *testing.T) { if err != nil { t.Fatal(err) } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() var routes struct { Routes []struct { diff --git a/internal/testutil/fakeupstream.go b/internal/testutil/fakeupstream.go index 8de9d87..4ebd09e 100644 --- a/internal/testutil/fakeupstream.go +++ b/internal/testutil/fakeupstream.go @@ -91,7 +91,7 @@ func NewUpstream(t *testing.T, scripts ...Script) *Upstream { t.Helper() u := &Upstream{scripts: scripts} u.Server = httptest.NewServer(http.HandlerFunc(u.serve)) - t.Cleanup(u.Server.Close) + t.Cleanup(u.Close) return u } @@ -164,7 +164,7 @@ func (u *Upstream) serveStream(w http.ResponseWriter, s Script) { if err != nil { panic(err) } - defer conn.Close() + defer func() { _ = conn.Close() }() var head bytes.Buffer fmt.Fprintf(&head, "HTTP/1.1 %d %s\r\n", statusOr(s.Status), http.StatusText(statusOr(s.Status))) @@ -204,7 +204,7 @@ func (u *Upstream) serveStream(w http.ResponseWriter, s Script) { // The final frame is where a truncation has to happen, so it is // written as a partial chunk rather than a complete one. if s.Ending == EndTruncateMidFrame && i == len(frames)-1 { - fmt.Fprintf(rw, "%x\r\n%s", len(frame), frame[:len(frame)/2]) + _, _ = fmt.Fprintf(rw, "%x\r\n%s", len(frame), frame[:len(frame)/2]) _ = rw.Flush() resetOrClose(conn, false) return @@ -266,7 +266,7 @@ func splitFrames(payload string) []string { } func writeChunk(rw *bufio.ReadWriter, data string) { - fmt.Fprintf(rw, "%x\r\n%s\r\n", len(data), data) + _, _ = fmt.Fprintf(rw, "%x\r\n%s\r\n", len(data), data) _ = rw.Flush() } From bfa527947a2d899efae896941132a27f2ac667ba Mon Sep 17 00:00:00 2001 From: Peter Honeder Date: Mon, 31 Aug 2026 10:27:44 +0300 Subject: [PATCH 2/2] Blame the client only when it left with bytes still owed to it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestNonStreamingTruncatedBody failed about one full-suite run in four, blaming the client for a body the vendor cut mid-JSON: Side = client (kind=client_disconnect_ctx), want upstream verdict: your LLM tool hung up first. The vendor did not interrupt anything. postmortem demoted an upstream fault to a client one by reading rec.ClientGone() — live, mutable state — after the copy loop had already reached its verdict. The client watcher stays armed until ServeHTTP returns, well past that point, so any client close landing in the window retroactively rewrote the answer. The client was doing nothing wrong. The upstream sends a well-formed HTTP response whose body ends mid-JSON: 64 bytes, Content-Length satisfied. The proxy forwards all 64. The client sees a complete response and closes, which is what any client without keep-alives does on every successful request. Whether the vendor got blamed for its own truncated body came down to whether that FIN was processed before one line of Go ran. Real clients hit this, not just the test harness. AsInduced is a fallback, not the main path. A client that hangs up mid-stream is already handled structurally: the proxy cancels the upstream read itself with a stamped ErrClientGone, and fromCancellation reads that cause — which is why TestClientDisconnectsWhileUpstreamIsSilent can assert the verdict is not induced. So the demotion only needs the narrower causal question, and now asks it as a single atomic read: was the client gone, *and* were bytes still owed to it? responseWasComplete already compares BytesToClient against BytesFromUpstream, so the comparison is not a new idea here. A client that received every byte read from the upstream interrupted nothing, whenever its FIN arrives. The ordering stops mattering rather than being won. Verified by forcing the race with a temporary sleep before the check: the old predicate fails 10/10, the new one passes 10/10, and the forced failure is character-identical to the flake. 0 failures in 12 consecutive full-suite runs under -race, against 1 in 4 before. The second ClientGone() use in postmortem is deliberately left alone. That one wants the late observation, with completeness deciding. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016X5Bmd7YUcjf9ydMGMtWQg --- internal/proxy/handler.go | 11 +++++- internal/proxy/statuses_test.go | 32 ++++++++++++++++ internal/record/record.go | 15 ++++++++ internal/record/record_test.go | 68 +++++++++++++++++++++++++++++++++ 4 files changed, 125 insertions(+), 1 deletion(-) create mode 100644 internal/record/record_test.go diff --git a/internal/proxy/handler.go b/internal/proxy/handler.go index b19fdca..58d0685 100644 --- a/internal/proxy/handler.go +++ b/internal/proxy/handler.go @@ -518,7 +518,16 @@ func (h *routeHandler) postmortem(rec *record.Request, status int, a *record.Att // consequence of that departure, not an independent vendor failure. Left // alone it would blame the vendor for whatever its socket did in response // to a cancelled read. - if f != nil && f.Side == fault.SideUpstream && rec.ClientGone() { + // + // "Had already gone" has to mean gone *before the response was finished*, + // which is why this asks whether bytes were left undelivered rather than + // whether the client is gone now. The watcher stays armed until the handler + // returns — well past this point — so a client that took every byte the + // upstream sent and then closed, which is what any client without + // keep-alives does on success, would otherwise reach in and rewrite a + // vendor truncation as its own fault, depending purely on whether its FIN + // landed before this line ran. + if f != nil && f.Side == fault.SideUpstream && rec.ClientLeftMidResponse() { f = fault.AsInduced(f) } diff --git a/internal/proxy/statuses_test.go b/internal/proxy/statuses_test.go index 80fc485..908c714 100644 --- a/internal/proxy/statuses_test.go +++ b/internal/proxy/statuses_test.go @@ -5,6 +5,7 @@ import ( "net/http" "strings" "testing" + "time" "github.com/peterhoneder/llm-proxy/internal/fault" "github.com/peterhoneder/llm-proxy/internal/record" @@ -149,6 +150,37 @@ func TestNonStreamingTruncatedBody(t *testing.T) { requireKind(t, snap, fault.KindTruncatedBody) } +// The same truncation, but with the client hanging up the instant it has the +// body — which is what every client without keep-alives does on every request. +// +// The client watcher stays armed until the handler returns, so its stamp can +// land while the verdict is still being drawn. It must not change the answer: +// the client took every byte the upstream sent, so it interrupted nothing, and +// the vendor's body still ended mid-JSON. Attribution here is decided by +// undelivered bytes, not by which goroutine got there first. +func TestClientHangUpAfterTruncatedBodyStillBlamesUpstream(t *testing.T) { + t.Parallel() + upstream := newRawUpstream(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"id":"cmpl-9","choices":[{"index":0,"message":{"content":"cut o`) + }) + h := newHarnessWithUpstream(t, upstream) + + c := testutil.Dial(t, h.addr) + c.Send("POST", "/vendor/v1/chat/completions", "localhost", `{"model":"m","messages":[]}`) + c.ReadStatusLine() + c.ReadHeaders() + c.ReadSome(time.Second) + c.HangUp() + + snap := h.waitForRecord(t, 3*time.Second) + requireSide(t, snap, fault.SideUpstream) + requireKind(t, snap, fault.KindTruncatedBody) + if snap.Fault.Induced { + t.Error("a client that received the whole body cannot have induced the truncation") + } +} + func TestNonStreamingCompleteResponse(t *testing.T) { t.Parallel() h := newHarness(t, testutil.Script{ diff --git a/internal/record/record.go b/internal/record/record.go index 20f06bc..bd71c9f 100644 --- a/internal/record/record.go +++ b/internal/record/record.go @@ -389,6 +389,21 @@ func (r *Request) ClientGone() bool { return !r.ClientGoneAt.IsZero() } +// ClientLeftMidResponse reports whether the client went away while the proxy +// still had bytes to hand it. +// +// The bare fact that a client is gone says nothing about causation. A client +// with keep-alives disabled closes the connection on every successful request, +// once it has everything it asked for, and the watcher is still armed while the +// verdict is being drawn. Pairing the departure with undelivered bytes is what +// separates a client that interrupted the response from one that was simply +// finished with it. +func (r *Request) ClientLeftMidResponse() bool { + r.mu.Lock() + defer r.mu.Unlock() + return !r.ClientGoneAt.IsZero() && r.BytesToClient < r.BytesFromUpstream +} + // AddRead records bytes read from the upstream. func (r *Request) AddRead(n int, at time.Time) { r.lastUpstreamByte.Store(at.UnixNano()) diff --git a/internal/record/record_test.go b/internal/record/record_test.go new file mode 100644 index 0000000..f61c9ad --- /dev/null +++ b/internal/record/record_test.go @@ -0,0 +1,68 @@ +package record + +import ( + "net/http/httptest" + "testing" + "time" +) + +func newTestRequest(t *testing.T) *Request { + t.Helper() + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + return New("req-1", "conn-1", "vendor", "openai", r, time.Now()) +} + +// ClientLeftMidResponse is what stops a client's departure from rewriting a +// verdict it had no part in. The distinction it draws is causal, not temporal: +// a client is only implicated if the proxy still had bytes for it. +func TestClientLeftMidResponse(t *testing.T) { + t.Parallel() + + now := time.Now() + + t.Run("gone with bytes still undelivered", func(t *testing.T) { + t.Parallel() + rec := newTestRequest(t) + rec.AddRead(64, now) + rec.AddDelivered(20, now) + rec.SetClientGone(now) + if !rec.ClientLeftMidResponse() { + t.Error("a client that left with 44 bytes still owed to it interrupted the response") + } + }) + + // The case that made the truncated-body verdict a coin toss: a client + // without keep-alives closes the connection on every successful request, + // and the watcher is still armed while the verdict is drawn. + t.Run("gone having received everything", func(t *testing.T) { + t.Parallel() + rec := newTestRequest(t) + rec.AddRead(64, now) + rec.AddDelivered(64, now) + rec.SetClientGone(now) + if rec.ClientLeftMidResponse() { + t.Error("a client that received every byte read from the upstream interrupted nothing") + } + }) + + t.Run("still connected", func(t *testing.T) { + t.Parallel() + rec := newTestRequest(t) + rec.AddRead(64, now) + rec.AddDelivered(20, now) + if rec.ClientLeftMidResponse() { + t.Error("the client never went away") + } + }) + + // A bodyless response reads and delivers nothing. Equal counts, so no + // departure can be blamed on the response being unfinished. + t.Run("gone with nothing to deliver", func(t *testing.T) { + t.Parallel() + rec := newTestRequest(t) + rec.SetClientGone(now) + if rec.ClientLeftMidResponse() { + t.Error("with no bytes on either side there is nothing the client cut short") + } + }) +}