From 90bdbfeb8421fe0b752f0c6babc6054bdbea8fa8 Mon Sep 17 00:00:00 2001 From: Quick <31828688+Quick104@users.noreply.github.com> Date: Tue, 11 Aug 2026 10:31:39 -0400 Subject: [PATCH] fix(playback): expose conditional range outcomes (#594) * fix(playback): expose conditional range outcomes * fix(playback): classify rejected If-Range requests * fix(playback): evaluate If-Range diagnostics directly --- docs/architecture/direct-stream-resume.md | 14 ++ internal/playback/directplay.go | 119 +++++++++- internal/playback/directplay_test.go | 274 +++++++++++++++++++++- 3 files changed, 394 insertions(+), 13 deletions(-) diff --git a/docs/architecture/direct-stream-resume.md b/docs/architecture/direct-stream-resume.md index 5bc38cf2..1fcf59b4 100644 --- a/docs/architecture/direct-stream-resume.md +++ b/docs/architecture/direct-stream-resume.md @@ -31,6 +31,20 @@ entire current entity as `200 OK`, preventing bytes from different revisions from being combined. `If-None-Match` uses the same validator for ordinary conditional requests. +For a bounded chunk read, a client can instead send `Range` and `If-Match`. +When the validator matches, the server returns the requested `206` interval. +When it does not match, the server returns `412 Precondition Failed` without a +body. This contract avoids materializing a full entity when a bounded read's +validator is stale. It does not change `If-Range`: an `If-Range` mismatch still +ignores the range and returns the full current entity as `200 OK`, as required +by the HTTP range contract. + +Direct-stream completion logs record whether `If-Match` and `If-Range` were +present, a bounded conditional-result value, and short SHA-256 fingerprints for +the emitted and requested validators. The raw validators and media path are not +logged. The fingerprints are only correlation aids and are not protocol +validators. + Playback sessions already treat each transport request independently. Sequential ranged requests refresh transport activity, and cleanup never expires a session while one of those transport requests is active. A late diff --git a/internal/playback/directplay.go b/internal/playback/directplay.go index 8a211f2a..e64c8e2f 100644 --- a/internal/playback/directplay.go +++ b/internal/playback/directplay.go @@ -1,13 +1,16 @@ package playback import ( + "crypto/sha256" "fmt" "log/slog" "net/http" + "net/textproto" "os" "path/filepath" "strconv" "strings" + "time" "github.com/Silo-Server/silo-server/internal/httpstream" ) @@ -52,8 +55,8 @@ func MimeFromExtension(name string) string { // ServeDirectPlay serves a media file with HTTP byte-range support. // Uses http.ServeContent for proper range handling, which supports -// Range requests, conditional requests (If-Modified-Since, If-None-Match), -// and Content-Type detection. +// Range requests, conditional requests (including If-Match, If-Range, and +// If-None-Match), and Content-Type detection. func ServeDirectPlay(w http.ResponseWriter, r *http.Request, filePath string) error { // Media bodies routinely take longer than the server's absolute // WriteTimeout; roll the write deadline with progress instead. @@ -81,7 +84,8 @@ func ServeDirectPlay(w http.ResponseWriter, r *http.Request, filePath string) er } w.Header().Del("ETag") - if etag := directPlayEntityTag(f, stat); etag != "" { + etag := directPlayEntityTag(f, stat) + if etag != "" { w.Header().Set("ETag", etag) } @@ -89,7 +93,9 @@ func ServeDirectPlay(w http.ResponseWriter, r *http.Request, filePath string) er w.Header().Set("Content-Type", MimeFromExtension(filePath)) hadRange := len(r.Header.Values("Range")) > 0 + hadIfMatch := len(r.Header.Values("If-Match")) > 0 hadIfRange := len(r.Header.Values("If-Range")) > 0 + ifRangeResult := directStreamIfRangeResult(r, etag, stat.ModTime()) directStreamActive.Inc() http.ServeContent(w, r, stat.Name(), stat.ModTime(), f) outcome := streamWriter.Outcome(r.Context()) @@ -97,15 +103,27 @@ func ServeDirectPlay(w http.ResponseWriter, r *http.Request, filePath string) er bytesSent := streamWriter.BytesWritten() rangeStart := directStreamRangeStart(status, w.Header().Get("Content-Range")) recordDirectStreamEnd(outcome, status, bytesSent, rangeStart) - slog.InfoContext(r.Context(), "direct stream ended", + logAttrs := []any{ "component", "playback", "outcome", outcome, "status", status, "bytes_sent", bytesSent, "range_requested", hadRange, "range_start", rangeStart, + "had_if_match", hadIfMatch, "had_if_range", hadIfRange, - ) + "conditional_result", directStreamConditionalResult(status, hadIfMatch, hadIfRange, ifRangeResult), + } + if fingerprint := directStreamValidatorFingerprint(etag); fingerprint != "" { + logAttrs = append(logAttrs, "etag_fingerprint", fingerprint) + } + if fingerprint := directStreamHeaderFingerprint(r.Header, "If-Match"); fingerprint != "" { + logAttrs = append(logAttrs, "if_match_fingerprint", fingerprint) + } + if fingerprint := directStreamHeaderFingerprint(r.Header, "If-Range"); fingerprint != "" { + logAttrs = append(logAttrs, "if_range_fingerprint", fingerprint) + } + slog.InfoContext(r.Context(), "direct stream ended", logAttrs...) return nil } @@ -144,3 +162,94 @@ func directStreamRangeStart(status int, contentRange string) int64 { } return parsedStart } + +const ( + directStreamConditionalNone = "none" + directStreamConditionalIfMatchPassed = "if_match_passed" + directStreamConditionalIfMatchFailed = "if_match_failed" + directStreamConditionalIfRangeMatched = "if_range_matched" + directStreamConditionalIfRangeMismatched = "if_range_mismatched" + directStreamConditionalIfRangeNotEvaluated = "if_range_not_evaluated" +) + +func directStreamConditionalResult(status int, hadIfMatch, hadIfRange bool, ifRangeResult string) string { + switch { + case hadIfMatch && status == http.StatusPreconditionFailed: + return directStreamConditionalIfMatchFailed + case ifRangeResult != "" && status != http.StatusNotModified && status != http.StatusPreconditionFailed: + return ifRangeResult + case hadIfMatch: + return directStreamConditionalIfMatchPassed + case hadIfRange: + return directStreamConditionalIfRangeNotEvaluated + default: + return directStreamConditionalNone + } +} + +// directStreamIfRangeResult mirrors the If-Range decision made by +// http.ServeContent before range parsing. The final status cannot encode that +// decision reliably: ServeContent may reject a matched range with 416 or +// deliberately ignore matched aggregate ranges and return 200. +func directStreamIfRangeResult(r *http.Request, etag string, modtime time.Time) string { + if r.Method != http.MethodGet && r.Method != http.MethodHead { + return "" + } + if r.Header.Get("Range") == "" { + return "" + } + validator := r.Header.Get("If-Range") + if validator == "" { + return "" + } + if validatorETag := directStreamScanEntityTag(validator); validatorETag != "" { + if validatorETag == etag && validatorETag[0] == '"' { + return directStreamConditionalIfRangeMatched + } + return directStreamConditionalIfRangeMismatched + } + if modtime.IsZero() { + return directStreamConditionalIfRangeMismatched + } + validatorTime, err := http.ParseTime(validator) + if err == nil && validatorTime.Unix() == modtime.Unix() { + return directStreamConditionalIfRangeMatched + } + return directStreamConditionalIfRangeMismatched +} + +// directStreamScanEntityTag is the narrow ETag scanner needed to mirror +// net/http's unexported scanETag behavior for If-Range diagnostics. +func directStreamScanEntityTag(value string) string { + value = textproto.TrimString(value) + start := 0 + if strings.HasPrefix(value, "W/") { + start = 2 + } + if len(value[start:]) < 2 || value[start] != '"' { + return "" + } + for i := start + 1; i < len(value); i++ { + character := value[i] + switch { + case character == 0x21 || character >= 0x23 && character <= 0x7e || character >= 0x80: + case character == '"': + return value[:i+1] + default: + return "" + } + } + return "" +} + +func directStreamHeaderFingerprint(header http.Header, name string) string { + return directStreamValidatorFingerprint(strings.Join(header.Values(name), "\x00")) +} + +func directStreamValidatorFingerprint(validator string) string { + if strings.TrimSpace(validator) == "" { + return "" + } + digest := sha256.Sum256([]byte(validator)) + return fmt.Sprintf("%x", digest[:8]) +} diff --git a/internal/playback/directplay_test.go b/internal/playback/directplay_test.go index 60d3c000..55412f89 100644 --- a/internal/playback/directplay_test.go +++ b/internal/playback/directplay_test.go @@ -1,8 +1,11 @@ package playback import ( + "bytes" + "encoding/json" "fmt" "io" + "log/slog" "net/http" "net/http/httptest" "os" @@ -30,12 +33,15 @@ func TestServeDirectPlayHTTPContract(t *testing.T) { t.Fatal(err) } - serve := func(method, rangeHeader, ifRange, ifNoneMatch string) *httptest.ResponseRecorder { + serve := func(method, rangeHeader, ifMatch, ifRange, ifNoneMatch string) *httptest.ResponseRecorder { t.Helper() req := httptest.NewRequest(method, "/stream", nil) if rangeHeader != "" { req.Header.Set("Range", rangeHeader) } + if ifMatch != "" { + req.Header.Set("If-Match", ifMatch) + } if ifRange != "" { req.Header.Set("If-Range", ifRange) } @@ -49,7 +55,7 @@ func TestServeDirectPlayHTTPContract(t *testing.T) { return rr } - full := serve(http.MethodGet, "", "", "") + full := serve(http.MethodGet, "", "", "", "") if full.Code != http.StatusOK { t.Fatalf("full status = %d, want 200", full.Code) } @@ -72,7 +78,7 @@ func TestServeDirectPlayHTTPContract(t *testing.T) { } t.Run("HEAD", func(t *testing.T) { - rr := serve(http.MethodHead, "", "", "") + rr := serve(http.MethodHead, "", "", "", "") if rr.Code != http.StatusOK { t.Fatalf("status = %d, want 200", rr.Code) } @@ -144,7 +150,7 @@ func TestServeDirectPlayHTTPContract(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { resumesBefore := counterValue(t, directStreamRangeResumes) - rr := serve(http.MethodGet, tt.rangeHeader, "", "") + rr := serve(http.MethodGet, tt.rangeHeader, "", "", "") if rr.Code != tt.wantStatus { t.Fatalf("status = %d, want %d; body = %q", rr.Code, tt.wantStatus, rr.Body.String()) } @@ -169,7 +175,7 @@ func TestServeDirectPlayHTTPContract(t *testing.T) { if !validatorRequired { t.Skip("platform does not expose a durable file revision") } - rr := serve(http.MethodGet, "bytes=7-", etag, "") + rr := serve(http.MethodGet, "bytes=7-", "", etag, "") if rr.Code != http.StatusPartialContent { t.Fatalf("status = %d, want 206", rr.Code) } @@ -179,7 +185,7 @@ func TestServeDirectPlayHTTPContract(t *testing.T) { }) t.Run("stale If-Range", func(t *testing.T) { - rr := serve(http.MethodGet, "bytes=7-", "\"stale\"", "") + rr := serve(http.MethodGet, "bytes=7-", "", "\"stale\"", "") if rr.Code != http.StatusOK { t.Fatalf("status = %d, want 200", rr.Code) } @@ -188,11 +194,34 @@ func TestServeDirectPlayHTTPContract(t *testing.T) { } }) + t.Run("bounded range with matching If-Match", func(t *testing.T) { + if !validatorRequired { + t.Skip("platform does not expose a durable file revision") + } + rr := serve(http.MethodGet, "bytes=7-11", etag, "", "") + if rr.Code != http.StatusPartialContent { + t.Fatalf("status = %d, want 206", rr.Code) + } + if body := rr.Body.String(); body != content[7:12] { + t.Fatalf("body = %q, want %q", body, content[7:12]) + } + }) + + t.Run("bounded range with stale If-Match", func(t *testing.T) { + rr := serve(http.MethodGet, "bytes=7-11", "\"stale\"", "", "") + if rr.Code != http.StatusPreconditionFailed { + t.Fatalf("status = %d, want 412", rr.Code) + } + if rr.Body.Len() != 0 { + t.Fatalf("body length = %d, want 0", rr.Body.Len()) + } + }) + t.Run("If-None-Match", func(t *testing.T) { if !validatorRequired { t.Skip("platform does not expose a durable file revision") } - rr := serve(http.MethodGet, "", "", etag) + rr := serve(http.MethodGet, "", "", "", etag) if rr.Code != http.StatusNotModified { t.Fatalf("status = %d, want 304", rr.Code) } @@ -202,7 +231,219 @@ func TestServeDirectPlayHTTPContract(t *testing.T) { }) } -func TestServeDirectPlayChangedEntityRejectsOldIfRange(t *testing.T) { +func TestDirectStreamConditionalResult(t *testing.T) { + tests := []struct { + name string + status int + hadIfMatch bool + hadIfRange bool + ifRangeResult string + want string + }{ + {name: "unconditional", status: http.StatusOK, want: directStreamConditionalNone}, + {name: "If-Match passed", status: http.StatusPartialContent, hadIfMatch: true, want: directStreamConditionalIfMatchPassed}, + {name: "If-Match failed", status: http.StatusPreconditionFailed, hadIfMatch: true, want: directStreamConditionalIfMatchFailed}, + {name: "If-Range matched", status: http.StatusPartialContent, hadIfRange: true, ifRangeResult: directStreamConditionalIfRangeMatched, want: directStreamConditionalIfRangeMatched}, + {name: "If-Range mismatched", status: http.StatusOK, hadIfRange: true, ifRangeResult: directStreamConditionalIfRangeMismatched, want: directStreamConditionalIfRangeMismatched}, + {name: "If-Range not evaluated after 304", status: http.StatusNotModified, hadIfRange: true, ifRangeResult: directStreamConditionalIfRangeMatched, want: directStreamConditionalIfRangeNotEvaluated}, + {name: "If-Range not evaluated after 412", status: http.StatusPreconditionFailed, hadIfRange: true, ifRangeResult: directStreamConditionalIfRangeMismatched, want: directStreamConditionalIfRangeNotEvaluated}, + {name: "If-Range without Range", status: http.StatusOK, hadIfRange: true, want: directStreamConditionalIfRangeNotEvaluated}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := directStreamConditionalResult(tt.status, tt.hadIfMatch, tt.hadIfRange, tt.ifRangeResult); got != tt.want { + t.Fatalf("conditional result = %q, want %q", got, tt.want) + } + }) + } +} + +func TestDirectStreamIfRangeResult(t *testing.T) { + const currentETag = `"current"` + modtime := time.Date(2026, time.August, 11, 12, 0, 0, 0, time.UTC) + tests := []struct { + name string + method string + rangeHead string + validator string + modtime time.Time + want string + }{ + {name: "matching ETag", method: http.MethodGet, rangeHead: "bytes=2-4", validator: currentETag, modtime: modtime, want: directStreamConditionalIfRangeMatched}, + {name: "matching ETag with whitespace", method: http.MethodGet, rangeHead: "bytes=2-4", validator: ` "current" `, modtime: modtime, want: directStreamConditionalIfRangeMatched}, + {name: "stale ETag", method: http.MethodGet, rangeHead: "bytes=2-4", validator: `"stale"`, modtime: modtime, want: directStreamConditionalIfRangeMismatched}, + {name: "weak ETag", method: http.MethodGet, rangeHead: "bytes=2-4", validator: `W/"current"`, modtime: modtime, want: directStreamConditionalIfRangeMismatched}, + {name: "matching date", method: http.MethodHead, rangeHead: "bytes=2-4", validator: modtime.Format(http.TimeFormat), modtime: modtime, want: directStreamConditionalIfRangeMatched}, + {name: "stale date", method: http.MethodGet, rangeHead: "bytes=2-4", validator: modtime.Add(-time.Second).Format(http.TimeFormat), modtime: modtime, want: directStreamConditionalIfRangeMismatched}, + {name: "missing range", method: http.MethodGet, validator: currentETag, modtime: modtime}, + {name: "empty validator", method: http.MethodGet, rangeHead: "bytes=2-4", modtime: modtime}, + {name: "unsupported method", method: http.MethodPost, rangeHead: "bytes=2-4", validator: currentETag, modtime: modtime}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(tt.method, "/stream", nil) + if tt.rangeHead != "" { + req.Header.Set("Range", tt.rangeHead) + } + if tt.validator != "" { + req.Header.Set("If-Range", tt.validator) + } + if got := directStreamIfRangeResult(req, currentETag, tt.modtime); got != tt.want { + t.Fatalf("If-Range result = %q, want %q", got, tt.want) + } + }) + } + + t.Run("uses first If-Range header", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/stream", nil) + req.Header.Set("Range", "bytes=2-4") + req.Header["If-Range"] = []string{currentETag, `"stale"`} + if got := directStreamIfRangeResult(req, currentETag, modtime); got != directStreamConditionalIfRangeMatched { + t.Fatalf("If-Range result = %q, want %q", got, directStreamConditionalIfRangeMatched) + } + }) +} + +func TestDirectStreamValidatorFingerprint(t *testing.T) { + const validator = "\"private-validator\"" + fingerprint := directStreamValidatorFingerprint(validator) + if len(fingerprint) != 16 { + t.Fatalf("fingerprint length = %d, want 16", len(fingerprint)) + } + if strings.Contains(fingerprint, "private") || strings.Contains(fingerprint, "validator") { + t.Fatalf("fingerprint leaked validator text: %q", fingerprint) + } + if got := directStreamValidatorFingerprint(validator); got != fingerprint { + t.Fatalf("fingerprint = %q, want stable %q", got, fingerprint) + } + if got := directStreamValidatorFingerprint("\"different\""); got == fingerprint { + t.Fatalf("different validator produced the same fingerprint %q", got) + } + if got := directStreamValidatorFingerprint(" "); got != "" { + t.Fatalf("blank validator fingerprint = %q, want omitted", got) + } +} + +func TestServeDirectPlayConditionalDiagnostics(t *testing.T) { + if !platformRequiresDirectPlayValidator() { + t.Skip("platform does not expose a durable file revision") + } + + filePath := filepath.Join(t.TempDir(), "private-fixture.mp4") + if err := os.WriteFile(filePath, []byte("0123456789"), 0o600); err != nil { + t.Fatal(err) + } + + var logs bytes.Buffer + previousLogger := slog.Default() + slog.SetDefault(slog.New(slog.NewJSONHandler(&logs, nil))) + t.Cleanup(func() { slog.SetDefault(previousLogger) }) + initial := httptest.NewRecorder() + if err := ServeDirectPlay(initial, httptest.NewRequest(http.MethodGet, "/stream", nil), filePath); err != nil { + t.Fatal(err) + } + currentETag := initial.Header().Get("ETag") + if currentETag == "" { + t.Fatal("initial response omitted ETag") + } + + tests := []struct { + name string + rangeHeader string + headerName string + validator string + wantStatus int + wantResult string + fingerprintName string + }{ + { + name: "If-Match mismatch", + rangeHeader: "bytes=2-4", + headerName: "If-Match", + validator: "\"stale-private-if-match\"", + wantStatus: http.StatusPreconditionFailed, + wantResult: directStreamConditionalIfMatchFailed, + fingerprintName: "if_match_fingerprint", + }, + { + name: "If-Range mismatch", + rangeHeader: "bytes=2-4", + headerName: "If-Range", + validator: "\"stale-private-if-range\"", + wantStatus: http.StatusOK, + wantResult: directStreamConditionalIfRangeMismatched, + fingerprintName: "if_range_fingerprint", + }, + { + name: "If-Range match with unsatisfiable range", + rangeHeader: "bytes=999-1000", + headerName: "If-Range", + validator: currentETag, + wantStatus: http.StatusRequestedRangeNotSatisfiable, + wantResult: directStreamConditionalIfRangeMatched, + fingerprintName: "if_range_fingerprint", + }, + { + name: "If-Range match with ignored aggregate ranges", + rangeHeader: "bytes=0-9,0-9", + headerName: "If-Range", + validator: currentETag, + wantStatus: http.StatusOK, + wantResult: directStreamConditionalIfRangeMatched, + fingerprintName: "if_range_fingerprint", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + logs.Reset() + req := httptest.NewRequest(http.MethodGet, "/stream", nil) + req.Header.Set("Range", tt.rangeHeader) + req.Header.Set(tt.headerName, tt.validator) + rr := httptest.NewRecorder() + if err := ServeDirectPlay(rr, req, filePath); err != nil { + t.Fatal(err) + } + if rr.Code != tt.wantStatus { + t.Fatalf("status = %d, want %d", rr.Code, tt.wantStatus) + } + + var record map[string]any + if err := json.Unmarshal(logs.Bytes(), &record); err != nil { + t.Fatalf("decode structured log %q: %v", logs.String(), err) + } + if got := record["conditional_result"]; got != tt.wantResult { + t.Fatalf("conditional_result = %v, want %q", got, tt.wantResult) + } + if got := record["had_if_match"]; got != (tt.headerName == "If-Match") { + t.Fatalf("had_if_match = %v, want %v", got, tt.headerName == "If-Match") + } + if got := record[tt.fingerprintName]; got != directStreamValidatorFingerprint(tt.validator) { + t.Fatalf("%s = %v, want request fingerprint", tt.fingerprintName, got) + } + responseETag := rr.Header().Get("ETag") + if tt.wantStatus == http.StatusRequestedRangeNotSatisfiable { + // ServeContent strips ETag from its 416 response, but the end log + // still fingerprints the validator used for If-Range evaluation. + responseETag = currentETag + } + if got := record["etag_fingerprint"]; got != directStreamValidatorFingerprint(responseETag) { + t.Fatalf("etag_fingerprint = %v, want response ETag fingerprint", got) + } + if strings.Contains(logs.String(), strings.Trim(tt.validator, "\"")) { + t.Fatalf("structured log leaked raw request validator: %s", logs.String()) + } + if strings.Contains(logs.String(), strings.Trim(responseETag, "\"")) { + t.Fatalf("structured log leaked raw response validator: %s", logs.String()) + } + if strings.Contains(logs.String(), filePath) { + t.Fatalf("structured log leaked media path: %s", logs.String()) + } + }) + } +} + +func TestServeDirectPlayChangedEntityRejectsOldValidators(t *testing.T) { if !platformRequiresDirectPlayValidator() { t.Skip("platform does not expose a durable file revision") } @@ -275,6 +516,23 @@ func TestServeDirectPlayChangedEntityRejectsOldIfRange(t *testing.T) { if newETag := rr.Header().Get("ETag"); newETag == oldETag { t.Fatalf("ETag did not change after replacement: %q", newETag) } + + ifMatchRequest := httptest.NewRequest(http.MethodGet, "/stream", nil) + ifMatchRequest.Header.Set("Range", "bytes=5-8") + ifMatchRequest.Header.Set("If-Match", oldETag) + ifMatchResponse := httptest.NewRecorder() + if err := ServeDirectPlay(ifMatchResponse, ifMatchRequest, filePath); err != nil { + t.Fatal(err) + } + if ifMatchResponse.Code != http.StatusPreconditionFailed { + t.Fatalf("If-Match status = %d, want 412", ifMatchResponse.Code) + } + if ifMatchResponse.Body.Len() != 0 { + t.Fatalf("If-Match body length = %d, want 0", ifMatchResponse.Body.Len()) + } + if newETag := ifMatchResponse.Header().Get("ETag"); newETag == oldETag { + t.Fatalf("If-Match response did not expose the replacement ETag: %q", newETag) + } } func TestDirectPlayEntityTagOmitsUnsupportedRevision(t *testing.T) {