Invalidate rewritten pyc files by source hash

changelog/11418.improvement.rst

@@ -0,0 +1,11 @@
+The cached rewritten modules in ``__pycache__`` are now invalidated by a hash of
+the source instead of its modification time, using the checked-hash pyc format
+from :pep:`552`.
+
+Timestamps make the cache useless wherever source files get fresh modification
+times, most notably in CI: a checkout gives every file a new mtime, so a
+restored cache is discarded in full and every test module is rewritten again.
+Hashing the source costs roughly 15 microseconds per file and makes the cache
+survive that.
+
+Existing timestamp based caches are ignored and rewritten once.

changelog/13292.bugfix.rst

@@ -0,0 +1,3 @@
+Fixed a stale rewritten module being used when a test file is modified twice
+within the same second without changing its size -- the pyc header could only
+record whole-second timestamps. The cache is now keyed on a hash of the source.

src/_pytest/assertion/rewrite.py

@@ -19,7 +19,6 @@
 import os
 from pathlib import Path
 from pathlib import PurePath
-import struct
 import sys
 import tokenize
 import types
@@ -176,11 +175,11 @@ def exec_module(self, module: types.ModuleType) -> None:
         co = _read_pyc(fn, pyc, state.trace)
         if co is None:
             state.trace(f"rewriting {fn!r}")
-            source_stat, co = _rewrite_test(fn, self.config)
+            source_hash, co = _rewrite_test(fn, self.config)
             if write:
                 self._writing_pyc = True
                 try:
-                    _write_pyc(state, co, source_stat, pyc)
+                    _write_pyc(state, co, source_hash, pyc)
                 finally:
                     self._writing_pyc = False
         else:
@@ -297,34 +296,28 @@ def get_resource_reader(self, name: str) -> TraversableResources:
         return FileReader(types.SimpleNamespace(path=self._rewritten_names[name]))  # type: ignore[arg-type]
 
 
-def _write_pyc_fp(
-    fp: IO[bytes], source_stat: os.stat_result, co: types.CodeType
-) -> None:
+def _write_pyc_fp(fp: IO[bytes], source_hash: bytes, co: types.CodeType) -> None:
     # Technically, we don't have to have the same pyc format as
     # (C)Python, since these "pycs" should never be seen by builtin
     # import. However, there's little reason to deviate.
     fp.write(importlib.util.MAGIC_NUMBER)
-    # https://www.python.org/dev/peps/pep-0552/
-    flags = b"\x00\x00\x00\x00"
-    fp.write(flags)
-    # as of now, bytecode header expects 32-bit numbers for size and mtime (#4903)
-    mtime = int(source_stat.st_mtime) & 0xFFFFFFFF
-    size = source_stat.st_size & 0xFFFFFFFF
-    # "<LL" stands for 2 unsigned longs, little-endian.
-    fp.write(struct.pack("<LL", mtime, size))
+    # A checked-hash pyc, per https://peps.python.org/pep-0552/: bit 0 marks
+    # the pyc as hash-based, bit 1 requests that the hash is always verified.
+    # Timestamps are unusable for us: a fresh checkout, or any cache restore,
+    # gives every source file a new mtime and invalidates the whole cache.
+    fp.write(b"\x03\x00\x00\x00")
+    # 64-bit source hash, as computed by importlib.util.source_hash().
+    fp.write(source_hash[:8])
     fp.write(marshal.dumps(co))
 
 
 def _write_pyc(
-    state: AssertionState,
-    co: types.CodeType,
-    source_stat: os.stat_result,
-    pyc: Path,
+    state: AssertionState, co: types.CodeType, source_hash: bytes, pyc: Path
 ) -> bool:
     proc_pyc = f"{pyc}.{os.getpid()}"
     try:
         with open(proc_pyc, "wb") as fp:
-            _write_pyc_fp(fp, source_stat, co)
+            _write_pyc_fp(fp, source_hash, co)
     except OSError as e:
         state.trace(f"error writing pyc file at {proc_pyc}: errno={e.errno}")
         return False
@@ -340,15 +333,15 @@ def _write_pyc(
     return True
 
 
-def _rewrite_test(fn: Path, config: Config) -> tuple[os.stat_result, types.CodeType]:
-    """Read and rewrite *fn* and return the code object."""
-    stat = os.stat(fn)
+def _rewrite_test(fn: Path, config: Config) -> tuple[bytes, types.CodeType]:
+    """Read and rewrite *fn* and return its source hash and code object."""
     source = fn.read_bytes()
+    source_hash = importlib.util.source_hash(source)
     strfn = str(fn)
     tree = ast.parse(source, filename=strfn)
     rewrite_asserts(tree, source, strfn, config)
     co = compile(tree, strfn, "exec", dont_inherit=True)
-    return stat, co
+    return source_hash, co
 
 
 def _read_pyc(
@@ -364,9 +357,6 @@ def _read_pyc(
         return None
     with fp:
         try:
-            stat_result = os.stat(source)
-            mtime = int(stat_result.st_mtime)
-            size = stat_result.st_size
             data = fp.read(16)
         except OSError as e:
             trace(f"_read_pyc({source}): OSError {e}")
@@ -378,16 +368,16 @@ def _read_pyc(
         if data[:4] != importlib.util.MAGIC_NUMBER:
             trace(f"_read_pyc({source}): invalid pyc (bad magic number)")
             return None
-        if data[4:8] != b"\x00\x00\x00\x00":
+        if data[4:8] != b"\x03\x00\x00\x00":
             trace(f"_read_pyc({source}): invalid pyc (unsupported flags)")
             return None
-        mtime_data = data[8:12]
-        if int.from_bytes(mtime_data, "little") != mtime & 0xFFFFFFFF:
-            trace(f"_read_pyc({source}): out of date")
+        try:
+            source_hash = importlib.util.source_hash(source.read_bytes())
+        except OSError as e:
+            trace(f"_read_pyc({source}): OSError {e}")
             return None
-        size_data = data[12:16]
-        if int.from_bytes(size_data, "little") != size & 0xFFFFFFFF:
-            trace(f"_read_pyc({source}): invalid pyc (incorrect size)")
+        if source_hash[:8] != data[8:16]:
+            trace(f"_read_pyc({source}): out of date")
             return None
         try:
             co = marshal.load(fp)

testing/test_assertrewrite.py

@@ -9,6 +9,7 @@
 from functools import partial
 import glob
 import importlib
+from importlib.util import source_hash
 import inspect
 import marshal
 import os
@@ -1324,13 +1325,14 @@ def test_write_pyc(self, pytester: Pytester, tmp_path) -> None:
         config = pytester.parseconfig()
         state = AssertionState(config, "rewrite")
         tmp_path.joinpath("source.py").touch()
-        source_path = str(tmp_path)
+        source_bytes = tmp_path.joinpath("source.py").read_bytes()
         pycpath = tmp_path.joinpath("pyc")
         co = compile("1", "f.py", "single")
-        assert _write_pyc(state, co, os.stat(source_path), pycpath)
+        hash = source_hash(source_bytes)
+        assert _write_pyc(state, co, hash, pycpath)
 
         with mock.patch.object(os, "replace", side_effect=OSError):
-            assert not _write_pyc(state, co, os.stat(source_path), pycpath)
+            assert not _write_pyc(state, co, hash, pycpath)
 
     def test_resources_provider_for_loader(self, pytester: Pytester) -> None:
         """
@@ -1403,8 +1405,37 @@ def test_read_pyc_success(self, tmp_path: Path, pytester: Pytester) -> None:
 
         fn.write_text("def test(): assert True", encoding="utf-8")
 
-        source_stat, co = _rewrite_test(fn, config)
-        _write_pyc(state, co, source_stat, pyc)
+        hash, co = _rewrite_test(fn, config)
+        _write_pyc(state, co, hash, pyc)
+        assert _read_pyc(fn, pyc, state.trace) is not None
+
+        pyc_bytes = pyc.read_bytes()
+        assert pyc_bytes[4] == 3  # checked-hash flag set
+        assert pyc_bytes[8:16] == hash[:8]
+
+    def test_read_pyc_ignores_mtime(self, tmp_path: Path, pytester: Pytester) -> None:
+        """A pyc stays valid when only the mtime of the source changes.
+
+        This is what makes the cache survive a fresh checkout or a restored
+        CI cache, where every source file gets a new mtime.
+        """
+        from _pytest.assertion import AssertionState
+        from _pytest.assertion.rewrite import _read_pyc
+        from _pytest.assertion.rewrite import _rewrite_test
+        from _pytest.assertion.rewrite import _write_pyc
+
+        config = pytester.parseconfig()
+        state = AssertionState(config, "rewrite")
+
+        fn = tmp_path / "source.py"
+        pyc = Path(str(fn) + "c")
+        fn.write_text("def test(): assert True", encoding="utf-8")
+
+        hash, co = _rewrite_test(fn, config)
+        _write_pyc(state, co, hash, pyc)
+
+        new_mtime = os.stat(fn).st_mtime + 3600
+        os.utime(fn, (new_mtime, new_mtime))
         assert _read_pyc(fn, pyc, state.trace) is not None
 
     def test_read_pyc_more_invalid(self, tmp_path: Path) -> None:
@@ -1417,40 +1448,71 @@ def test_read_pyc_more_invalid(self, tmp_path: Path) -> None:
         source.write_bytes(source_bytes)
 
         magic = importlib.util.MAGIC_NUMBER
-
-        flags = b"\x00\x00\x00\x00"
-
-        mtime = b"\x58\x3c\xb0\x5f"
-        mtime_int = int.from_bytes(mtime, "little")
-        os.utime(source, (mtime_int, mtime_int))
-
-        size = len(source_bytes).to_bytes(4, "little")
-
+        flags = b"\x03\x00\x00\x00"
+        hash = source_hash(source_bytes)[:8]
         code = marshal.dumps(compile(source_bytes, str(source), "exec"))
 
         # Good header.
-        pyc.write_bytes(magic + flags + mtime + size + code)
+        pyc.write_bytes(magic + flags + hash + code)
         assert _read_pyc(source, pyc, print) is not None
 
         # Too short.
-        pyc.write_bytes(magic + flags + mtime)
+        pyc.write_bytes(magic + flags + hash[:4])
         assert _read_pyc(source, pyc, print) is None
 
         # Bad magic.
-        pyc.write_bytes(b"\x12\x34\x56\x78" + flags + mtime + size + code)
+        pyc.write_bytes(b"\x12\x34\x56\x78" + flags + hash + code)
         assert _read_pyc(source, pyc, print) is None
 
-        # Unsupported flags.
-        pyc.write_bytes(magic + b"\x00\xff\x00\x00" + mtime + size + code)
+        # Unsupported flags -- including the timestamp based pycs written by
+        # pytest<9.3 and by CPython itself.
+        for bad_flags in (
+            b"\x00\x00\x00\x00",
+            b"\x01\x00\x00\x00",
+            b"\x00\xff\x00\x00",
+        ):
+            pyc.write_bytes(magic + bad_flags + hash + code)
+            assert _read_pyc(source, pyc, print) is None
+
+        # Bad hash.
+        pyc.write_bytes(magic + flags + b"\x00" * 8 + code)
         assert _read_pyc(source, pyc, print) is None
 
-        # Bad mtime.
-        pyc.write_bytes(magic + flags + b"\x58\x3d\xb0\x5f" + size + code)
+        # Missing source.
+        pyc.write_bytes(magic + flags + hash + code)
+        source.unlink()
         assert _read_pyc(source, pyc, print) is None
 
-        # Bad size.
-        pyc.write_bytes(magic + flags + mtime + b"\x99\x00\x00\x00" + code)
-        assert _read_pyc(source, pyc, print) is None
+    def test_rewrite_picks_up_edit_within_one_mtime_second(
+        self, pytester: Pytester
+    ) -> None:
+        """Regression test for #13292.
+
+        The pyc header can only hold a whole-second timestamp, so a file
+        edited twice within the same second used to be served from a stale
+        pyc. Hashing the source instead sidesteps the resolution problem.
+        """
+        source = pytester.path / "test_edited.py"
+        pyc_dir = source.parent / "__pycache__"
+
+        # both revisions are the same size, so only the content differs
+        before = "def test_aaa(): assert True\n"
+        after = "def test_bbb(): assert None\n"
+        assert len(before) == len(after)
+
+        source.write_text(before, encoding="utf-8")
+        assert pytester.runpytest_subprocess("-q").ret == 0
+        (pyc,) = pyc_dir.glob("test_edited.*.pyc")
+        mtime = os.stat(source).st_mtime
+
+        source.write_text(after, encoding="utf-8")
+        # pin the mtime so the edit is indistinguishable by timestamp
+        os.utime(source, (mtime, mtime))
+        assert pyc.exists()  # the pyc written by the first run is still there
+
+        result = pytester.runpytest_subprocess("-q")
+        result.stdout.fnmatch_lines(["*test_bbb*"])
+        assert result.ret != 0
 
     def test_reload_is_same_and_reloads(self, pytester: Pytester) -> None:
         """Reloading a (collected) module after change picks up the change."""