From cc1b13ccbfaa3c6818839f66d1bb0627203bd204 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=9Cbertreiber?=
 <5731814+troiganto@users.noreply.github.com>
Date: Mon, 7 Oct 2024 22:19:20 +0200
Subject: [PATCH] intersphinx: fix flipped use of `intersphinx_cache_limit`.
 (#12905)

Co-authored-by: Nico Madysa <nico.madysa@cern.ch>
Co-authored-by: Adam Turner <9087854+aa-turner@users.noreply.github.com>
---
 CHANGES.rst                                   |  4 ++
 sphinx/ext/intersphinx/_load.py               |  6 ++-
 tests/test_extensions/test_ext_intersphinx.py | 41 +++++++++++++++----
 3 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/CHANGES.rst b/CHANGES.rst
index d7eb80cc5..a6edae8f9 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -135,6 +135,10 @@ Bugs fixed
   ``SOURCE_DATE_EPOCH`` for entries that match the current system clock year,
   and disallow substitution of future years.
   Patch by James Addison and Adam Turner.
+* #12905: intersphinx: fix flipped use of :confval:`intersphinx_cache_limit`,
+  which always kept the cache for positive values, and always refreshed it for
+  negative ones.
+  Patch by Nico Madysa.
 
 Testing
 -------
diff --git a/sphinx/ext/intersphinx/_load.py b/sphinx/ext/intersphinx/_load.py
index b891d0d99..27b116734 100644
--- a/sphinx/ext/intersphinx/_load.py
+++ b/sphinx/ext/intersphinx/_load.py
@@ -210,9 +210,13 @@ def _fetch_inventory_group(
     config: Config,
     srcdir: Path,
 ) -> bool:
-    if config.intersphinx_cache_limit < 0:
+    if config.intersphinx_cache_limit >= 0:
+        # Positive value: cache is expired if its timestamp is below
+        # `now - X days`.
         cache_time = now - config.intersphinx_cache_limit * 86400
     else:
+        # Negative value: cache is expired if its timestamp is below
+        # zero, which is impossible.
         cache_time = 0
 
     updated = False
diff --git a/tests/test_extensions/test_ext_intersphinx.py b/tests/test_extensions/test_ext_intersphinx.py
index 99dde92a5..ffc990326 100644
--- a/tests/test_extensions/test_ext_intersphinx.py
+++ b/tests/test_extensions/test_ext_intersphinx.py
@@ -745,30 +745,57 @@ if TYPE_CHECKING:
 
 
 @pytest.mark.sphinx('html', testroot='root')
-def test_intersphinx_cache_limit(app):
+@pytest.mark.parametrize(
+    ('cache_limit', 'expected_expired'),
+    [
+        (5, False),
+        (1, True),
+        (0, True),
+        (-1, False),
+    ],
+)
+def test_intersphinx_cache_limit(app, monkeypatch, cache_limit, expected_expired):
     url = 'https://example.org/'
-    app.config.intersphinx_cache_limit = -1
+    app.config.intersphinx_cache_limit = cache_limit
     app.config.intersphinx_mapping = {
         'inv': (url, None),
     }
     # load the inventory and check if it's done correctly
     intersphinx_cache: dict[str, InventoryCacheEntry] = {
-        url: ('', 0, {}),  # 0 is a timestamp, make sure the entry is expired
+        url: ('inv', 0, {}),  # Timestamp of last cache write is zero.
     }
     validate_intersphinx_mapping(app, app.config)
-    load_mappings(app)
 
-    now = int(time.time())
+    # The test's `now` is two days after the cache was created.
+    now = 2 * 86400
+    monkeypatch.setattr('time.time', lambda: now)
+
+    # `_fetch_inventory_group` calls `_fetch_inventory`.
+    # We replace it with a mock to test whether it has been called.
+    # If it has been called, it means the cache had expired.
+    mock_fetch_inventory = mock.Mock(return_value=('inv', now, {}))
+    monkeypatch.setattr(
+        'sphinx.ext.intersphinx._load._fetch_inventory', mock_fetch_inventory
+    )
+
     for name, (uri, locations) in app.config.intersphinx_mapping.values():
         project = _IntersphinxProject(name=name, target_uri=uri, locations=locations)
-        # no need to read from remote
-        assert not _fetch_inventory_group(
+        updated = _fetch_inventory_group(
             project=project,
             cache=intersphinx_cache,
             now=now,
             config=app.config,
             srcdir=app.srcdir,
         )
+        # If we hadn't mocked `_fetch_inventory`, it would've made
+        # a request to `https://example.org/` and found no inventory
+        # file. That would've been an error, and `updated` would've been
+        # False even if the cache had expired. The mock makes it behave
+        # "correctly".
+        assert updated is expected_expired
+    # Double-check: If the cache was expired, `mock_fetch_inventory`
+    # must've been called.
+    assert mock_fetch_inventory.called is expected_expired
 
 
 def test_intersphinx_fetch_inventory_group_url():