From ade6a327a4b65e19f06f74f48f66cbde4401c849 Mon Sep 17 00:00:00 2001 From: Julien Neuhart Date: Mon, 7 Sep 2026 17:54:25 +0200 Subject: [PATCH] fix(chromium): drop settled requests from the per-conversion network map --- pkg/modules/chromium/network_aggregate.go | 21 ++++- .../chromium/network_aggregate_test.go | 83 +++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-) diff --git a/pkg/modules/chromium/network_aggregate.go b/pkg/modules/chromium/network_aggregate.go index 3431f261..f119d5e7 100644 --- a/pkg/modules/chromium/network_aggregate.go +++ b/pkg/modules/chromium/network_aggregate.go @@ -11,6 +11,15 @@ import ( // pathological page cannot grow the set without limit. const maxTrackedOrigins = 64 +// maxTrackedRequests bounds the request id to URL map. Entries are dropped as +// soon as the request settles, so the map normally holds only what is in +// flight, but a request that never reports a loading-finished or +// loading-failed event never settles. Without a cap, a page that opens +// requests it never resolves would grow the map for the whole conversion, at +// the cost of one full response URL per entry. Losing an entry only costs the +// heaviest-resource URL attribution for that request. +const maxTrackedRequests = 1024 + // networkAggregate accumulates per-conversion network activity from Chromium // DevTools events. It is safe for concurrent use by the chromedp event listener // goroutine and the conversion goroutine that reads the snapshot afterwards. @@ -60,7 +69,9 @@ func (a *networkAggregate) onResponseReceived(ev *network.EventResponseReceived) a.origins[origin] = struct{}{} } } - a.requestURLByID[ev.RequestID] = ev.Response.URL + if len(a.requestURLByID) < maxTrackedRequests { + a.requestURLByID[ev.RequestID] = ev.Response.URL + } } // onLoadingFinished records a successfully completed request and its size, @@ -81,6 +92,11 @@ func (a *networkAggregate) onLoadingFinished(ev *network.EventLoadingFinished) { a.heaviestBytes = size a.heaviestURL = a.requestURLByID[ev.RequestID] } + + // The request has settled and nothing reads its URL again. Dropping it + // keeps the map proportional to the requests in flight rather than to + // every request the page ever made. + delete(a.requestURLByID, ev.RequestID) } // onLoadingFailed records a request that failed to complete. @@ -94,6 +110,9 @@ func (a *networkAggregate) onLoadingFailed(ev *network.EventLoadingFailed) { a.requestCount++ a.failedCount++ + + // Settled, like a finished request: its URL is never read again. + delete(a.requestURLByID, ev.RequestID) } func (a *networkAggregate) snapshot() networkStats { diff --git a/pkg/modules/chromium/network_aggregate_test.go b/pkg/modules/chromium/network_aggregate_test.go index bfa49b9d..422bdd84 100644 --- a/pkg/modules/chromium/network_aggregate_test.go +++ b/pkg/modules/chromium/network_aggregate_test.go @@ -98,3 +98,86 @@ func TestNetworkAggregate_ConcurrentSafe(t *testing.T) { t.Errorf("requestCount = %d, want 100", got) } } + +// TestNetworkAggregate_SettledRequestsAreDropped covers the growth where every +// response URL stayed in the map for the whole conversion even though nothing +// reads it again once the request settles. +func TestNetworkAggregate_SettledRequestsAreDropped(t *testing.T) { + a := newNetworkAggregate() + + for i := range 500 { + id := network.RequestID(fmt.Sprintf("r%d", i)) + a.onResponseReceived(&network.EventResponseReceived{ + RequestID: id, + Response: &network.Response{URL: fmt.Sprintf("https://host.example.com/%d", i)}, + }) + + if i%2 == 0 { + a.onLoadingFinished(&network.EventLoadingFinished{RequestID: id, EncodedDataLength: 10}) + continue + } + + a.onLoadingFailed(&network.EventLoadingFailed{RequestID: id}) + } + + a.mu.Lock() + tracked := len(a.requestURLByID) + a.mu.Unlock() + + if tracked != 0 { + t.Errorf("tracked requests = %d, want 0: settled requests must not be retained", tracked) + } + + // The bookkeeping the map feeds must survive the pruning. + got := a.snapshot() + if got.requestCount != 500 { + t.Errorf("requestCount = %d, want 500", got.requestCount) + } + if got.failedCount != 250 { + t.Errorf("failedCount = %d, want 250", got.failedCount) + } +} + +// TestNetworkAggregate_UnsettledRequestCap verifies the ceiling that applies +// when requests never settle, which is the only way the map can still grow. +func TestNetworkAggregate_UnsettledRequestCap(t *testing.T) { + a := newNetworkAggregate() + + for i := range maxTrackedRequests + 500 { + a.onResponseReceived(&network.EventResponseReceived{ + RequestID: network.RequestID(fmt.Sprintf("r%d", i)), + Response: &network.Response{URL: fmt.Sprintf("https://host.example.com/%d", i)}, + }) + } + + a.mu.Lock() + tracked := len(a.requestURLByID) + a.mu.Unlock() + + if tracked != maxTrackedRequests { + t.Errorf("tracked requests = %d, want %d (capped)", tracked, maxTrackedRequests) + } +} + +// TestNetworkAggregate_HeaviestURLSurvivesPruning guards the attribution the +// map exists for: the URL must still be resolved before the entry is dropped. +func TestNetworkAggregate_HeaviestURLSurvivesPruning(t *testing.T) { + a := newNetworkAggregate() + + a.onResponseReceived(&network.EventResponseReceived{ + RequestID: "small", + Response: &network.Response{URL: "https://example.com/small.css"}, + }) + a.onLoadingFinished(&network.EventLoadingFinished{RequestID: "small", EncodedDataLength: 10}) + + a.onResponseReceived(&network.EventResponseReceived{ + RequestID: "big", + Response: &network.Response{URL: "https://example.com/big.png"}, + }) + a.onLoadingFinished(&network.EventLoadingFinished{RequestID: "big", EncodedDataLength: 4096}) + + got := a.snapshot() + if got.heaviestURL != "https://example.com/big.png" || got.heaviestBytes != 4096 { + t.Errorf("heaviest = (%q, %d), want (%q, 4096)", got.heaviestURL, got.heaviestBytes, "https://example.com/big.png") + } +}