From 3f3cf55002056fc3ca00c83c1d7af3c85093d2bd Mon Sep 17 00:00:00 2001
From: d3v1l1989 <64855033+d3v1l1989@users.noreply.github.com>
Date: Tue, 16 Jun 2026 17:14:14 +0200
Subject: [PATCH] fix(jellycompat): return 404 for HLS segments of a failed
 transcode (#163)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

HandleHLSSegment mapped every non-ErrSegmentNotFound error from the
segment-retrieval/recovery path to a generic 500 "Failed to load segment".
When a transcode process starts and then exits non-zero, WaitForSegment
returns a wrapped playback.ErrTranscodeFailed, which fell through to that
500 — observed in production as repeated 500s on seg_00000.ts that drove an
8x client retry storm and crash-log uploads.

The segment will never materialize once its transcode has died, so this is a
not-found condition: Jellyfin's DynamicHls handler falls through to a
PhysicalFileResult for the absent file, which ASP.NET serves as 404, never
500. Map ErrTranscodeFailed to 404 alongside ErrSegmentNotFound via a small
extracted hlsSegmentErrorResponse helper, reserving 500 for genuinely
unexpected errors. Adds a unit test pinning the mapping.
---
 internal/jellycompat/streams.go               | 24 +++++++--
 .../jellycompat/streams_segment_error_test.go | 50 +++++++++++++++++++
 2 files changed, 69 insertions(+), 5 deletions(-)
 create mode 100644 internal/jellycompat/streams_segment_error_test.go

diff --git a/internal/jellycompat/streams.go b/internal/jellycompat/streams.go
index 6dd30775..0386a399 100644
--- a/internal/jellycompat/streams.go
+++ b/internal/jellycompat/streams.go
@@ -458,11 +458,8 @@ func (h *PlaybackHandler) HandleHLSSegment(w http.ResponseWriter, r *http.Reques
 		}
 	}
 	if err != nil {
-		if errors.Is(err, playback.ErrSegmentNotFound) {
-			writeError(w, http.StatusNotFound, "NotFound", "Segment not found")
-			return
-		}
-		writeError(w, http.StatusInternalServerError, "ServerError", "Failed to load segment")
+		status, code, message := hlsSegmentErrorResponse(err)
+		writeError(w, status, code, message)
 		return
 	}
 
@@ -473,6 +470,23 @@ func (h *PlaybackHandler) HandleHLSSegment(w http.ResponseWriter, r *http.Reques
 	http.ServeFile(w, r, segmentPath)
 }
 
+// hlsSegmentErrorResponse maps a segment-retrieval error to a Jellyfin-faithful
+// HTTP status. A segment that is absent (ErrSegmentNotFound) or whose transcode
+// process started and then exited non-zero (ErrTranscodeFailed, surfaced by
+// WaitForSegment after the recovery/restart path is exhausted) will never
+// materialize. Jellyfin serves both as 404: its DynamicHls segment handler falls
+// through to a PhysicalFileResult for the missing file, which ASP.NET returns as
+// 404, never 500. Reserve 500 for genuinely unexpected errors (e.g. a stat
+// failure on a file that does exist).
+func hlsSegmentErrorResponse(err error) (status int, code, message string) {
+	switch {
+	case errors.Is(err, playback.ErrSegmentNotFound), errors.Is(err, playback.ErrTranscodeFailed):
+		return http.StatusNotFound, "NotFound", "Segment not found"
+	default:
+		return http.StatusInternalServerError, "ServerError", "Failed to load segment"
+	}
+}
+
 // HandleSubtitleStream proxies subtitle requests through the native stream subtitle route.
 func (h *PlaybackHandler) HandleSubtitleStream(w http.ResponseWriter, r *http.Request) {
 	session := SessionFromContext(r.Context())
diff --git a/internal/jellycompat/streams_segment_error_test.go b/internal/jellycompat/streams_segment_error_test.go
new file mode 100644
index 00000000..2d789738
--- /dev/null
+++ b/internal/jellycompat/streams_segment_error_test.go
@@ -0,0 +1,50 @@
+package jellycompat
+
+import (
+	"errors"
+	"fmt"
+	"net/http"
+	"testing"
+
+	"github.com/Silo-Server/silo-server/internal/playback"
+)
+
+// TestHLSSegmentErrorResponse pins the never-500 contract for the HLS segment
+// handler: a segment that will never materialize (absent, or whose transcode
+// process started then died) maps to 404 like Jellyfin, and only genuinely
+// unexpected errors keep the 500.
+func TestHLSSegmentErrorResponse(t *testing.T) {
+	tests := []struct {
+		name       string
+		err        error
+		wantStatus int
+		wantCode   string
+	}{
+		{"segment not found", playback.ErrSegmentNotFound, http.StatusNotFound, "NotFound"},
+		{"transcode failed", playback.ErrTranscodeFailed, http.StatusNotFound, "NotFound"},
+		{
+			// WaitForSegment wraps the ffmpeg exit error as
+			// fmt.Errorf("%w: %v", ErrTranscodeFailed, waitErr) — this is exactly the
+			// error that hit the catch-all 500 in production (Fire TV, seg_00000.ts).
+			name:       "wrapped transcode failed",
+			err:        fmt.Errorf("%w: exit status 1", playback.ErrTranscodeFailed),
+			wantStatus: http.StatusNotFound,
+			wantCode:   "NotFound",
+		},
+		{
+			name:       "unexpected error stays 500",
+			err:        errors.New("stat segment: permission denied"),
+			wantStatus: http.StatusInternalServerError,
+			wantCode:   "ServerError",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			status, code, _ := hlsSegmentErrorResponse(tt.err)
+			if status != tt.wantStatus || code != tt.wantCode {
+				t.Fatalf("hlsSegmentErrorResponse(%v) = (%d, %q), want (%d, %q)",
+					tt.err, status, code, tt.wantStatus, tt.wantCode)
+			}
+		})
+	}
+}