fix: [proxy/engines] correct off-by-one byte ranges and handle ignored io.ReadAll errors (#948)

Signed-off-by: Chris Randles <randles.chris@gmail.com>

Author: crandles

pkg/proxy/engines/document.go

@@ -137,7 +137,7 @@ func (d *HTTPDocument) getByteRanges() byterange.Ranges {
 	} else if ranges := d.RangeParts.Ranges(); len(ranges) > 0 {
 		return ranges
 	}
-	return byterange.Ranges{byterange.Range{Start: 0, End: d.ContentLength}}
+	return byterange.Ranges{byterange.Range{Start: 0, End: d.ContentLength - 1}}
 }
 
 // ShallowCopy returns a shallow copy of the HTTPDocument with a fresh headerLock.

pkg/proxy/engines/document_test.go

@@ -235,6 +235,66 @@ func TestLoadRangeParts(t *testing.T) {
 	}
 }
 
+func TestGetByteRanges(t *testing.T) {
+	tests := []struct {
+		name      string
+		doc       *HTTPDocument
+		wantStart int64
+		wantEnd   int64
+		wantLen   int
+	}{
+		{
+			name: "returns stored ranges when present",
+			doc: &HTTPDocument{
+				ContentLength: 100,
+				Ranges:        byterange.Ranges{{Start: 10, End: 20}},
+			},
+			wantStart: 10,
+			wantEnd:   20,
+			wantLen:   1,
+		},
+		{
+			name: "returns range parts ranges when no stored ranges",
+			doc: &HTTPDocument{
+				ContentLength: 100,
+				RangeParts: byterange.MultipartByteRanges{
+					byterange.Range{Start: 5, End: 15}: &byterange.MultipartByteRange{
+						Range:   byterange.Range{Start: 5, End: 15},
+						Content: make([]byte, 11),
+					},
+				},
+			},
+			wantStart: 5,
+			wantEnd:   15,
+			wantLen:   1,
+		},
+		{
+			name: "returns full content range with inclusive end",
+			doc: &HTTPDocument{
+				ContentLength: 100,
+			},
+			wantStart: 0,
+			wantEnd:   99, // ContentLength - 1, not ContentLength
+			wantLen:   1,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ranges := tt.doc.getByteRanges()
+			if len(ranges) != tt.wantLen {
+				t.Fatalf("expected %d ranges, got %d", tt.wantLen, len(ranges))
+			}
+			if ranges[0].Start != tt.wantStart {
+				t.Errorf("expected Start=%d, got %d", tt.wantStart, ranges[0].Start)
+			}
+			if ranges[0].End != tt.wantEnd {
+				t.Errorf("expected End=%d, got %d", tt.wantEnd, ranges[0].End)
+			}
+		})
+	}
+}
+
 func TestGetByterangeChunkBody(t *testing.T) {
 	d := &HTTPDocument{
 		Body: []byte{0, 1, 2, 3, 4, 5},

pkg/proxy/engines/objectproxycache.go

@@ -75,7 +75,10 @@ func handleCachePartialHit(pr *proxyRequest) error {
 	d := pr.cacheDocument
 	resp := pr.upstreamResponse
 	if pr.isPartialResponse {
-		b, _ := io.ReadAll(pr.upstreamReader)
+		b, err := io.ReadAll(pr.upstreamReader)
+		if err != nil {
+			return err
+		}
 		d2 := &HTTPDocument{}
 
 		d2.ParsePartialContentBody(resp, b)
@@ -86,7 +89,7 @@ func handleCachePartialHit(pr *proxyRequest) error {
 		d.RangeParts.Merge(d2.RangeParts)
 		d.Ranges = d.RangeParts.Ranges()
 		d.StoredRangeParts = d.RangeParts.PackableMultipartByteRanges()
-		err := d.FulfillContentBody()
+		err = d.FulfillContentBody()
 
 		if err == nil {
 			pr.upstreamResponse.Body = io.NopCloser(bytes.NewReader(d.Body))

pkg/proxy/engines/proxy_request.go

@@ -203,7 +203,7 @@ func (pr *proxyRequest) prepareRevalidationRequest() {
 		if len(pr.wantedRanges) > 0 {
 			wr = pr.wantedRanges
 		} else {
-			wr = byterange.Ranges{{Start: 0, End: cl}}
+			wr = byterange.Ranges{{Start: 0, End: cl - 1}}
 		}
 
 		// revalRanges are the ranges we have in cache that have expired, but the user needs
@@ -660,7 +660,12 @@ func (pr *proxyRequest) reconstituteResponses() {
 				wg.Go(func() {
 					// oh snap. so we have some partial content to merge in, but the original cache document
 					// is now invalid. lets go ahead and reset it.
-					b, _ := io.ReadAll(resp.Body)
+					b, err := io.ReadAll(resp.Body)
+					if err != nil {
+						logger.Error("error reading revalidation response body",
+							logging.Pairs{"detail": err.Error()})
+						return
+					}
 					appendLock.Lock()
 					parts.ParsePartialContentBody(resp, b)
 					appendLock.Unlock()
@@ -682,7 +687,12 @@ func (pr *proxyRequest) reconstituteResponses() {
 				appendLock.Unlock()
 
 				if resp.StatusCode == http.StatusPartialContent {
-					b, _ := io.ReadAll(resp.Body)
+					b, err := io.ReadAll(resp.Body)
+					if err != nil {
+						logger.Error("error reading origin response body",
+							logging.Pairs{"detail": err.Error()})
+						return
+					}
 					appendLock.Lock()
 					parts.ParsePartialContentBody(resp, b)
 					appendLock.Unlock()

pkg/proxy/engines/proxy_request_test.go

@@ -18,6 +18,8 @@ package engines
 
 import (
 	"bytes"
+	"errors"
+	"io"
 	"net/http"
 	"sync"
 	"testing"
@@ -338,6 +340,37 @@ func TestPrepareRevalidationRequestNoRange(t *testing.T) {
 	}
 }
 
+func TestPrepareRevalidationRequestDefaultRangeInclusiveEnd(t *testing.T) {
+	logger.SetLogger(logging.ConsoleLogger(level.Error))
+	r, _ := http.NewRequest(http.MethodGet, "http://127.0.0.1/", nil)
+
+	o := &bo.Options{DearticulateUpstreamRanges: true}
+	r = request.SetResources(r, request.NewResources(o, nil, nil, nil, nil, nil))
+
+	pr := proxyRequest{
+		Request:          r,
+		upstreamRequest:  r,
+		cachingPolicy:    &CachingPolicy{},
+		upstreamResponse: &http.Response{},
+		cacheDocument: &HTTPDocument{
+			ContentLength: 100,
+			Ranges:        byterange.Ranges{byterange.Range{Start: 30, End: 40}},
+		},
+		cacheStatus: status.LookupStatusPartialHit,
+		// no wantedRanges — triggers default range using ContentLength
+	}
+	pr.prepareRevalidationRequest()
+
+	// with no wantedRanges, the default wanted range is 0 to ContentLength-1 (99)
+	// revalRanges = neededRanges.CalculateDeltas(wr, cl) with nil neededRanges gives wr back
+	// that's 1 range so rh = revalRanges.String() = "bytes=0-99"
+	v := pr.revalidationRequest.Header.Get(headers.NameRange)
+	expected := "bytes=0-99"
+	if v != expected {
+		t.Errorf("expected %s got %s", expected, v)
+	}
+}
+
 func TestPrepareUpstreamRequests(t *testing.T) {
 	logger.SetLogger(logging.ConsoleLogger(level.Error))
 	r, _ := http.NewRequest(http.MethodGet, "http://127.0.0.1/", nil)
@@ -400,3 +433,87 @@ func TestReconstituteResponses(t *testing.T) {
 		t.Errorf("expected %d got %d", 0, len(pr.originRequests))
 	}
 }
+
+// errReader is an io.ReadCloser that always returns an error
+type errReader struct{ err error }
+
+func (e *errReader) Read([]byte) (int, error) { return 0, e.err }
+func (e *errReader) Close() error             { return nil }
+
+func TestReconstituteResponsesReadError(t *testing.T) {
+	logger.SetLogger(logging.ConsoleLogger(level.Error))
+
+	r1, _ := http.NewRequest(http.MethodGet, "http://127.0.0.1/", nil)
+	r2, _ := http.NewRequest(http.MethodGet, "http://127.0.0.1/", nil)
+	baseReq, _ := http.NewRequest(http.MethodGet, "http://127.0.0.1/", nil)
+	baseReq = request.SetResources(baseReq, request.NewResources(&bo.Options{}, nil, nil, nil, nil, nil))
+
+	readErr := errors.New("simulated read error")
+
+	pr := &proxyRequest{
+		mapLock:       &sync.Mutex{},
+		cachingPolicy: &CachingPolicy{},
+		originRequests: []*http.Request{r1, r2},
+		originResponses: []*http.Response{
+			{
+				StatusCode: http.StatusPartialContent,
+				Header:     http.Header{headers.NameContentRange: []string{"bytes 0-3/10"}},
+				Body:       &errReader{err: readErr},
+			},
+			{
+				StatusCode: http.StatusPartialContent,
+				Header:     http.Header{headers.NameContentRange: []string{"bytes 4-9/10"}},
+				Body:       io.NopCloser(bytes.NewReader([]byte("456789"))),
+			},
+		},
+		cacheDocument: &HTTPDocument{ContentLength: 10},
+	}
+	pr.Request = baseReq
+
+	// should not panic; the error reader's goroutine logs and returns
+	pr.reconstituteResponses()
+
+	// the second response should still have been processed
+	if pr.upstreamResponse == nil {
+		t.Error("expected upstream response to be set")
+	}
+}
+
+func TestReconstituteResponsesRevalidationReadError(t *testing.T) {
+	logger.SetLogger(logging.ConsoleLogger(level.Error))
+
+	r1, _ := http.NewRequest(http.MethodGet, "http://127.0.0.1/", nil)
+	reval, _ := http.NewRequest(http.MethodGet, "http://127.0.0.1/", nil)
+	baseReq, _ := http.NewRequest(http.MethodGet, "http://127.0.0.1/", nil)
+	baseReq = request.SetResources(baseReq, request.NewResources(&bo.Options{}, nil, nil, nil, nil, nil))
+
+	readErr := errors.New("simulated revalidation read error")
+
+	pr := &proxyRequest{
+		mapLock:       &sync.Mutex{},
+		cachingPolicy: &CachingPolicy{},
+		revalidationRequest: reval,
+		revalidationResponse: &http.Response{
+			StatusCode: http.StatusPartialContent,
+			Header:     http.Header{headers.NameContentRange: []string{"bytes 0-4/10"}},
+			Body:       &errReader{err: readErr},
+		},
+		originRequests: []*http.Request{r1},
+		originResponses: []*http.Response{
+			{
+				StatusCode: http.StatusPartialContent,
+				Header:     http.Header{headers.NameContentRange: []string{"bytes 5-9/10"}},
+				Body:       io.NopCloser(bytes.NewReader([]byte("56789"))),
+			},
+		},
+		cacheDocument: &HTTPDocument{ContentLength: 10},
+	}
+	pr.Request = baseReq
+
+	// should not panic; the revalidation error reader's goroutine logs and returns
+	pr.reconstituteResponses()
+
+	if pr.upstreamResponse == nil {
+		t.Error("expected upstream response to be set")
+	}
+}