From fb2b552d5bc4e2aaa0ce162256c06e7ca9239e12 Mon Sep 17 00:00:00 2001
From: Thomas Skowron <th@skowron.eu>
Date: Fri, 4 Sep 2026 10:53:45 +0200
Subject: [PATCH] fix: close response body before checking status code

TileFetcher.download deferred closing the HTTP response body only
after the status-code switch, so any non-200 response (404, 5xx,
etc.) returned early without closing the body, leaking the
underlying connection's file descriptor. Move the defer right after
the request succeeds so the body is always closed.

Add regression tests covering the 404 and 500 paths.
---
 tile_fetcher.go      |   3 +-
 tile_fetcher_test.go | 112 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 113 insertions(+), 2 deletions(-)
 create mode 100644 tile_fetcher_test.go

diff --git a/tile_fetcher.go b/tile_fetcher.go
index 9e45eac..7f31a07 100644
--- a/tile_fetcher.go
+++ b/tile_fetcher.go
@@ -116,6 +116,7 @@ func (t *TileFetcher) download(url string) ([]byte, error) {
 	if err != nil {
 		return nil, err
 	}
+	defer resp.Body.Close()
 
 	switch resp.StatusCode {
 	case http.StatusOK:
@@ -128,8 +129,6 @@ func (t *TileFetcher) download(url string) ([]byte, error) {
 		return nil, fmt.Errorf("GET %s: %s", url, resp.Status)
 	}
 
-	defer resp.Body.Close()
-
 	contents, err := io.ReadAll(resp.Body)
 	if err != nil {
 		return nil, err
diff --git a/tile_fetcher_test.go b/tile_fetcher_test.go
new file mode 100644
index 0000000..a1e8ad1
--- /dev/null
+++ b/tile_fetcher_test.go
@@ -0,0 +1,112 @@
+package sm
+
+import (
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+// closeTrackingBody wraps an io.ReadCloser and records whether Close was called.
+type closeTrackingBody struct {
+	io.ReadCloser
+	closed bool
+}
+
+func (b *closeTrackingBody) Close() error {
+	b.closed = true
+	return b.ReadCloser.Close()
+}
+
+// closeTrackingTransport wraps a Transport and captures the closeTrackingBody
+// of the last response it produced, so a test can check whether it was closed.
+type closeTrackingTransport struct {
+	http.RoundTripper
+	lastBody *closeTrackingBody
+}
+
+func (t *closeTrackingTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	resp, err := t.RoundTripper.RoundTrip(req)
+	if err != nil {
+		return resp, err
+	}
+	wrapped := &closeTrackingBody{ReadCloser: resp.Body}
+	t.lastBody = wrapped
+	resp.Body = wrapped
+	return resp, nil
+}
+
+// withInstrumentedDefaultClient temporarily swaps http.DefaultClient's
+// Transport for one that tracks response body closes, and returns a
+// restore func plus the tracker itself.
+func withInstrumentedDefaultClient(t *testing.T) (*closeTrackingTransport, func()) {
+	t.Helper()
+	base := http.DefaultClient.Transport
+	if base == nil {
+		base = http.DefaultTransport
+	}
+	tracker := &closeTrackingTransport{RoundTripper: base}
+	orig := http.DefaultClient.Transport
+	http.DefaultClient.Transport = tracker
+	return tracker, func() {
+		http.DefaultClient.Transport = orig
+	}
+}
+
+// TestTileFetcherDownloadClosesBodyOn404 verifies that TileFetcher.download
+// closes the HTTP response body even when the server responds with a
+// non-200 status. Leaving it unclosed leaks the underlying connection's
+// file descriptor since it is never returned to the transport's pool.
+func TestTileFetcherDownloadClosesBodyOn404(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusNotFound)
+		_, _ = w.Write([]byte("not found"))
+	}))
+	defer srv.Close()
+
+	tracker, restore := withInstrumentedDefaultClient(t)
+	defer restore()
+
+	provider := NewTileProviderOpenStreetMaps()
+	fetcher := NewTileFetcher(provider, nil, true)
+
+	_, err := fetcher.download(srv.URL)
+	if err != errTileNotFound {
+		t.Fatalf("expected errTileNotFound, got %v", err)
+	}
+
+	if tracker.lastBody == nil {
+		t.Fatal("no response body was observed")
+	}
+	if !tracker.lastBody.closed {
+		t.Error("response body was not closed on 404 response, leaking the underlying connection")
+	}
+}
+
+// TestTileFetcherDownloadClosesBodyOnServerError mirrors the 404 case for
+// the generic "unexpected status code" error path.
+func TestTileFetcherDownloadClosesBodyOnServerError(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusInternalServerError)
+		_, _ = w.Write([]byte("boom"))
+	}))
+	defer srv.Close()
+
+	tracker, restore := withInstrumentedDefaultClient(t)
+	defer restore()
+
+	provider := NewTileProviderOpenStreetMaps()
+	fetcher := NewTileFetcher(provider, nil, true)
+
+	_, err := fetcher.download(srv.URL)
+	if err == nil {
+		t.Fatal("expected an error for 500 response")
+	}
+
+	if tracker.lastBody == nil {
+		t.Fatal("no response body was observed")
+	}
+	if !tracker.lastBody.closed {
+		t.Error("response body was not closed on 500 response, leaking the underlying connection")
+	}
+}