Buffer JSON before writing response in writeJSON (y9)

Previously writeJSON wrote the status header before encoding, so an
encode failure produced a misleading 200 (or other caller status) with
a truncated body. Marshal into a bytes.Buffer first; on error emit 500
with a JSON error envelope instead.

3 files changed, 94 insertions, 6 deletions

diff --git a/player-server/internal/api/handlers.go b/player-server/internal/api/handlers.go
index f04ec19..8635c92 100644
--- a/player-server/internal/api/handlers.go
+++ b/player-server/internal/api/handlers.go
@@ -1,6 +1,7 @@
 package api
 
 import (
+	"bytes"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -26,12 +27,26 @@ func fileETag(size int64, modTime time.Time) string {
 // Helpers
 // ------------------------------------------------------------------
 
+// writeJSON serialises data to JSON and writes it to w with the given status.
+//
+// Marshalling happens into an in-memory buffer BEFORE any status or body is
+// written to the response. This avoids the previous footgun where the headers
+// (and a 200/whatever status) were committed first and then encoding failed
+// halfway through, producing a response with a misleading success status and a
+// truncated/invalid body. If encoding fails we instead emit a 500 with a small
+// JSON error payload so callers always see a consistent error shape.
 func writeJSON(w http.ResponseWriter, status int, data interface{}) {
-	w.Header().Set("Content-Type", "application/json")
-	w.WriteHeader(status)
-	if err := json.NewEncoder(w).Encode(data); err != nil {
+	var buf bytes.Buffer
+	if err := json.NewEncoder(&buf).Encode(data); err != nil {
 		slog.Error("encode json", "err", err)
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusInternalServerError)
+		_, _ = w.Write([]byte(`{"error":"internal encoding error"}`))
+		return
 	}
+	w.Header().Set("Content-Type", "application/json")
+	w.WriteHeader(status)
+	_, _ = w.Write(buf.Bytes())
 }
 
 func writeError(w http.ResponseWriter, status int, message string) {
diff --git a/player-server/internal/api/handlers_more_test.go b/player-server/internal/api/handlers_more_test.go
index 4c441cd..f26da93 100644
--- a/player-server/internal/api/handlers_more_test.go
+++ b/player-server/internal/api/handlers_more_test.go
@@ -147,11 +147,18 @@ func Test_readJSON_nilBody(t *testing.T) {
 }
 
 func Test_writeJSON_encodeError(t *testing.T) {
-	// channel cannot be JSON-encoded, triggering the error path
+	// Channels cannot be JSON-encoded, triggering the error path.
+	//
+	// Historically writeJSON wrote the response status BEFORE attempting to
+	// encode, so an encode failure produced a 200 with a corrupt body. After
+	// y9 the function marshals into an in-memory buffer first; if encoding
+	// fails it must emit 500 with a JSON error envelope instead of silently
+	// committing the caller's success status. This test now guards that
+	// fixed contract.
 	rr := httptest.NewRecorder()
 	writeJSON(rr, http.StatusOK, make(chan int))
-	if rr.Code != http.StatusOK {
-		t.Fatalf("expected %d, got %d", http.StatusOK, rr.Code)
+	if rr.Code != http.StatusInternalServerError {
+		t.Fatalf("expected %d on encode failure, got %d", http.StatusInternalServerError, rr.Code)
 	}
 }
 
diff --git a/player-server/internal/api/handlers_writejson_test.go b/player-server/internal/api/handlers_writejson_test.go
new file mode 100644
index 0000000..4b20486
--- /dev/null
+++ b/player-server/internal/api/handlers_writejson_test.go
@@ -0,0 +1,66 @@
+package api
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+)
+
+// TestWriteJSON_Success covers the happy path: a value that encodes cleanly
+// should reach the client with the caller-supplied status and the expected
+// Content-Type header. This guards against accidental regressions in the
+// buffered-encode path (e.g. forgetting to write buf.Bytes()).
+func TestWriteJSON_Success(t *testing.T) {
+	rr := httptest.NewRecorder()
+	writeJSON(rr, http.StatusCreated, map[string]string{"hello": "world"})
+
+	if rr.Code != http.StatusCreated {
+		t.Fatalf("expected status %d, got %d", http.StatusCreated, rr.Code)
+	}
+	if ct := rr.Header().Get("Content-Type"); ct != "application/json" {
+		t.Fatalf("expected Content-Type application/json, got %q", ct)
+	}
+	var decoded map[string]string
+	if err := json.Unmarshal(rr.Body.Bytes(), &decoded); err != nil {
+		t.Fatalf("body is not valid JSON: %v (body=%q)", err, rr.Body.String())
+	}
+	if decoded["hello"] != "world" {
+		t.Fatalf("expected hello=world, got %v", decoded)
+	}
+}
+
+// TestWriteJSON_EncodeFailureYields500 is the regression test for the bug
+// fixed in y9: previously writeJSON wrote the status header BEFORE attempting
+// to encode the body, so a value that json cannot marshal (channels, funcs)
+// produced a 200 with a corrupt body. After the refactor we marshal into a
+// bytes.Buffer first; if that fails we MUST emit 500 plus a fallback JSON
+// error payload instead of the caller's success status.
+func TestWriteJSON_EncodeFailureYields500(t *testing.T) {
+	rr := httptest.NewRecorder()
+	// chan int is not marshalable by encoding/json; this forces the failure
+	// branch without needing a custom MarshalJSON implementation.
+	unmarshalable := make(chan int)
+
+	writeJSON(rr, http.StatusOK, unmarshalable)
+
+	if rr.Code != http.StatusInternalServerError {
+		t.Fatalf("expected status %d on encode failure, got %d", http.StatusInternalServerError, rr.Code)
+	}
+	if ct := rr.Header().Get("Content-Type"); ct != "application/json" {
+		t.Fatalf("expected Content-Type application/json, got %q", ct)
+	}
+	body := rr.Body.String()
+	// Sanity: body must be valid JSON and look like an error envelope.
+	if !strings.Contains(body, `"error"`) {
+		t.Fatalf("expected fallback error body to contain \"error\" key, got %q", body)
+	}
+	var decoded map[string]string
+	if err := json.Unmarshal([]byte(body), &decoded); err != nil {
+		t.Fatalf("fallback body is not valid JSON: %v (body=%q)", err, body)
+	}
+	if decoded["error"] == "" {
+		t.Fatalf("expected non-empty error field, got %v", decoded)
+	}
+}