From b58e3288f5345f0fdc33dd7dc604731fae7994c1 Mon Sep 17 00:00:00 2001 From: Marcus Pasell <3690498+rickyrombo@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:26:32 -0700 Subject: [PATCH] fix(api): stop leaking the probe's skip_play_count into the stream url tryFindWorkingUrl probes each candidate host with a two-byte range request and sets skip_play_count=true so the probe is not counted as a listen. It set that on the candidate itself and then returned the same URL, so the flag rode along into what the caller hands the client. The node serving /tracks/cidstream/:cid returns early from logTrackListen when it sees skip_play_count, so every play resolved through /v1/tracks/:id/stream went unrecorded. Verified against production: the Location header from /v1/tracks/{id}/stream carries skip_play_count=true today. The fallback path is affected too, since the returned mainURL is urls[0] and was mutated by its own probe attempt. Probe on a copy and return the candidate untouched. redirectToStream still honours an explicit skip_play_count from the caller. Tests assert the returned URL is clean on the primary, fallback, and mirror paths, and that the probe itself still sets the flag -- confirmed failing against the previous behaviour. --- api/stream_util.go | 17 +++++-- api/stream_util_test.go | 99 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 3 deletions(-) create mode 100644 api/stream_util_test.go diff --git a/api/stream_util.go b/api/stream_util.go index a4d8aade..156bd1c0 100644 --- a/api/stream_util.go +++ b/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 } diff --git a/api/stream_util_test.go b/api/stream_util_test.go new file mode 100644 index 00000000..5471d3a8 --- /dev/null +++ b/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") + } +}