fix(api): stop leaking the probe's skip_play_count into the stream url

api/stream_util.go

@@ -11,6 +11,10 @@ import (
 // tryFindWorkingUrl attempts to validate a media link by checking if it can serve content.
 // It tries the primary URL first, then falls back to mirrors if needed.
 // Returns the first valid URL found or the main URL if nothing works.
+//
+// The returned URL carries no probe artifacts: callers hand it straight to a
+// client, and a stray skip_play_count would stop the serving node from
+// recording the listen.
 func tryFindWorkingUrl(mediaLink *dbv1.MediaLink) *url.URL {
 	mainURL, err := url.Parse(mediaLink.Url)
 	if err != nil {
@@ -34,11 +38,18 @@ func tryFindWorkingUrl(mediaLink *dbv1.MediaLink) *url.URL {
 		Timeout: 5 * time.Second,
 	}
 	for _, u := range urls {
-		q := u.Query()
+		// Probe on a COPY. skip_play_count exists so this two-byte probe is not
+		// counted as a listen, but it belongs to the probe alone -- mutating u
+		// would carry the flag into the URL we hand the client, and the node
+		// serving /tracks/cidstream/:cid returns early from logTrackListen when
+		// it sees it. That silently suppressed the play for every caller of
+		// /v1/tracks/:id/stream.
+		probe := *u
+		q := probe.Query()
 		q.Set("skip_play_count", "true")
-		u.RawQuery = q.Encode()
+		probe.RawQuery = q.Encode()
 
-		req, err := http.NewRequest("GET", u.String(), nil)
+		req, err := http.NewRequest("GET", probe.String(), nil)
 		if err != nil {
 			continue
 		}

api/stream_util_test.go

@@ -0,0 +1,99 @@
+package api
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"sync"
+	"testing"
+
+	"api.audius.co/api/dbv1"
+)
+
+// recordingHost is a stand-in for a content node: it records the query string of
+// every request it receives so a test can assert what the probe sent.
+type recordingHost struct {
+	mu      sync.Mutex
+	queries []url.Values
+	status  int
+}
+
+func (h *recordingHost) handler() http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		h.mu.Lock()
+		h.queries = append(h.queries, r.URL.Query())
+		h.mu.Unlock()
+		w.WriteHeader(h.status)
+	}
+}
+
+// The probe must send skip_play_count so a two-byte range request is not counted
+// as a listen -- but the URL handed back to the caller must not carry it. A node
+// serving /tracks/cidstream/:cid returns early from logTrackListen when it sees
+// that flag, so a leaked probe artifact silently suppresses the play.
+func TestReturnedUrlHasNoProbeArtifacts(t *testing.T) {
+	host := &recordingHost{status: http.StatusPartialContent}
+	srv := httptest.NewServer(host.handler())
+	defer srv.Close()
+
+	got := tryFindWorkingUrl(&dbv1.MediaLink{Url: srv.URL + "/tracks/cidstream/abc?signature=sig"})
+	if got == nil {
+		t.Fatal("expected a url")
+	}
+
+	if _, leaked := got.Query()["skip_play_count"]; leaked {
+		t.Errorf("returned url carries skip_play_count=%q; the serving node will not record the play",
+			got.Query().Get("skip_play_count"))
+	}
+	if got.Query().Get("signature") != "sig" {
+		t.Errorf("signature lost from returned url: %q", got.String())
+	}
+
+	host.mu.Lock()
+	defer host.mu.Unlock()
+	if len(host.queries) != 1 {
+		t.Fatalf("expected 1 probe, got %d", len(host.queries))
+	}
+	if host.queries[0].Get("skip_play_count") != "true" {
+		t.Error("the probe itself must set skip_play_count, or probing inflates play counts")
+	}
+}
+
+// The no-working-host fallback returns the main URL, which is also urls[0] --
+// so it must not have been mutated by its own probe attempt.
+func TestFallbackUrlHasNoProbeArtifacts(t *testing.T) {
+	host := &recordingHost{status: http.StatusInternalServerError}
+	srv := httptest.NewServer(host.handler())
+	defer srv.Close()
+
+	got := tryFindWorkingUrl(&dbv1.MediaLink{Url: srv.URL + "/tracks/cidstream/abc?signature=sig"})
+	if got == nil {
+		t.Fatal("expected the main url as fallback")
+	}
+	if _, leaked := got.Query()["skip_play_count"]; leaked {
+		t.Error("fallback url carries skip_play_count from its own probe")
+	}
+}
+
+// Mirrors are probed the same way and must come back equally clean.
+func TestMirrorUrlHasNoProbeArtifacts(t *testing.T) {
+	dead := httptest.NewServer((&recordingHost{status: http.StatusInternalServerError}).handler())
+	defer dead.Close()
+	live := httptest.NewServer((&recordingHost{status: http.StatusOK}).handler())
+	defer live.Close()
+
+	liveURL, _ := url.Parse(live.URL)
+	got := tryFindWorkingUrl(&dbv1.MediaLink{
+		Url:     dead.URL + "/tracks/cidstream/abc?signature=sig",
+		Mirrors: []string{live.URL},
+	})
+	if got == nil {
+		t.Fatal("expected the mirror")
+	}
+	if got.Host != liveURL.Host {
+		t.Fatalf("expected mirror host %s, got %s", liveURL.Host, got.Host)
+	}
+	if _, leaked := got.Query()["skip_play_count"]; leaked {
+		t.Error("mirror url carries skip_play_count from its probe")
+	}
+}