Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the flaky test_cache_hits_recorded to avoid asserting an exact second-visit request/response set, accounting for Firefox’s cross-tab in-memory cache which can serve some image/CSS resources without any observable webRequest event (issue #1162).
Changes:
- Replaces exact-set equality assertions with a deterministic “mandatory URL” floor plus per-resource
[min, max]tolerated request-count ranges for cache-dependent resources. - Adds stronger deterministic checks around redirect chains and response-to-request linkage via
request_id. - Requires at least one cached response on the second visit, while pinning the 404 response as uncacheable (
is_cached == 0).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1184 +/- ##
=======================================
Coverage 62.36% 62.36%
=======================================
Files 40 40
Lines 3930 3930
=======================================
Hits 2451 2451
Misses 1479 1479 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
vringar
force-pushed
the
fix/cache-hits-deterministic-ranges
branch
2 times, most recently
from
June 19, 2026 23:17
4a9f628 to
c868e86
Compare
vringar
added a commit
that referenced
this pull request
Jun 21, 2026
… FF cache flakiness Rebuild test_cache_hits_recorded so it is strict on what is determinable and tolerant on what is inherently flaky. Strict (catches issue #1162): build the visit_id -> site_rank map from site_visits and require exactly two visits (ranks 0 and 1). Every recorded http_requests and http_responses row must carry one of those two visit_ids, and the top-level document row must appear exactly once per visit. A record persisted under a stale visit_id, or a duplicated document row, now fails the test instead of slipping through. Previously the test used presence-only set checks scoped to the second visit, with no count ceiling and no visit_id verification, so a drifted or extra row was masked. Tolerant (issue #1162 cache flakiness, FF Bug 634073): replace the exact-set equality on second-visit requests and responses with a deterministic floor (mandatory URLs that always re-fire) plus explicit [min, max] ranges for the image and stylesheet resources that Firefox's cross-tab in-memory cache can serve silently. The redirect chain re-walks deterministically and stays an exact assertion. At least one cached response and the never-cacheable 404 anchor are still required, so the check does not degrade to always-true. Fixes #1162.
… visits The per-visit request assertions (mandatory-subresource floor, cache-dependent ranges, unexpected-URL guard) were scoped only to site_rank=1, leaving the first visit entirely unchecked. A non-document subresource (image/script/css/ 404/redirect hop) for one visit stamped with the OTHER visit's valid-but-wrong visit_id (issue #1162 shape) slipped every guard: the blanket visit_id check saw a valid visit_id, and the missing/extra mandatory row only surfaced on the unchecked visit 0. Apply the mandatory floor, the cache-dependent [0,2] ranges and the document ceiling to BOTH visits. Drift of any mandatory subresource or of the document row is now caught in both directions. The document ceiling was already per-visit; the comment now states this explicitly. Honest residual documented at the assertion site: drift of a cache-dependent image/css resource between the two visits can still hide inside the [0,2] tolerance, because both visits legitimately request the same resources, so such a row is not uniquely attributable by URL and its count change is indistinguishable from genuine Firefox cache nondeterminism (Bug 634073). This is an inherent limit of the test setup, not loosened to mask anything.
vringar
force-pushed
the
fix/cache-hits-deterministic-ranges
branch
from
July 20, 2026 22:54
c868e86 to
1b7ad57
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
test_cache_hits_recordedintermittently fails on CI because it asserts exact-set equality on the second-visit HTTP requests and responses. Firefox's cross-tab in-memory image cache can serve an image or stylesheet silently, with no webRequest event at all, so the exact set of recorded second-visit requests for those resources is not deterministic. AddingCache-Controlheaders was already tried and reverted because it made the flake worse, and there is no documented Firefox pref that forces a request for every memory-cache-served resource.This rewrites the test to assert a deterministic floor plus an explicit tolerated range, instead of exact-set equality. It replaces the previously proposed retry-quarantine approach, which would only have masked the flake (and any real regression along with it).
Fixes #1162.
New assertion structure
Deterministic floor (asserted exactly):
MAGIC_REDIRECTchains). None of these depend on cache state.HTTP_CACHED_REDIRECTSis unchanged).http_responses.request_idlinks back to a recordedhttp_requestsrow with a matching URL.is_cached == 0(it is never cacheable).Explicit tolerated range (for the cache-dependent image/css resources):
Each cache-dependent resource may be served silently from the image cache, so its observed request count is bounded to an explicit
[min, max]range encoded as a commented constant:shared/test_image.png[0, 2]shared/test_image_2.png[0, 2]shared/test_style.css[0, 2]<head>, but Firefox issues a second request to revalidate it on the new tab; observed 2 locally.The floor of zero tolerates the documented case where the cache swallows the reference entirely. The ceiling still rejects absurd or impossible counts (a value above the maximum would mean a request the pages never make), so the test continues to catch real regressions rather than degrading to an always-true check.
Verification
Ran
test_cache_hits_recordedagainst Firefox 150 ten times locally; it passed every time. The per-resource request counts were stable across runs, and the explicit ranges absorb the cache nondeterminism documented in #1162 without hiding failures.