From c596373f30ed5eddebbc667b780f10609434390b Mon Sep 17 00:00:00 2001 From: Dylan Jeffers Date: Wed, 14 Oct 2026 14:52:49 -0700 Subject: [PATCH] Revert "fix(api): stop leaking the probe's skip_play_count into the stream url (#1027)" This reverts commit c4c780103042e7be96720f93a40bcd7724b2c2fa. #1027 made every /v1/tracks/:id/stream request count as a play. The serving node records a listen per request with no dedupe or bot filtering, and each /stream call mints a fresh signature, so callers that re-request the URL (audio elements buffering or seeking, crawlers, servers) inflate counts. Since it deployed on 2026-10-12 plays went from ~30k/day to ~190k/day, all of it signed-out traffic, and play-count milestone notifications went from ~550/day to ~2,000/day. Restore the previous behavior until plays from /stream can be deduped at the node. Co-Authored-By: Claude Opus 5.5 --- api/stream_util.go | 17 ++----- api/stream_util_test.go | 99 ----------------------------------------- 2 files changed, 3 insertions(+), 113 deletions(-) delete mode 100644 api/stream_util_test.go diff --git a/api/stream_util.go b/api/stream_util.go index 156bd1c0..a4d8aade 100644 --- a/api/stream_util.go +++ b/api/stream_util.go @@ -11,10 +11,6 @@ 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 { @@ -38,18 +34,11 @@ func tryFindWorkingUrl(mediaLink *dbv1.MediaLink) *url.URL { Timeout: 5 * time.Second, } for _, u := range urls { - // 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 := u.Query() q.Set("skip_play_count", "true") - probe.RawQuery = q.Encode() + u.RawQuery = q.Encode() - req, err := http.NewRequest("GET", probe.String(), nil) + req, err := http.NewRequest("GET", u.String(), nil) if err != nil { continue } diff --git a/api/stream_util_test.go b/api/stream_util_test.go deleted file mode 100644 index 5471d3a8..00000000 --- a/api/stream_util_test.go +++ /dev/null @@ -1,99 +0,0 @@ -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") - } -}