Skip to content

Fix incorrect pyc cache loaded when test file is updated in short time #13293

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from

Conversation

jnhyperion
Copy link

@jnhyperion jnhyperion commented Mar 13, 2025

See: #13292

The root cause is the cache is using file st_mtime int value to check if it's out of date, it's not so accurate when the file is updated in short time. Using st_mtime_ns is the better solution.

@Pierre-Sassoulas Pierre-Sassoulas changed the title fix https://github.com/pytest-dev/pytest/issues/13292 Fix incorrect pyc cache loaded when test file is updated in short time Mar 13, 2025
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jnhyperion, this looks like a good change to me.

Can you please add a changelog entry? (see changelog/README.rst file)

The tests failures are due to #13291, rebasing on main after that PR is merged should make CI pass.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Mar 14, 2025
@@ -317,7 +317,7 @@ def _write_pyc_fp(
flags = b"\x00\x00\x00\x00"
fp.write(flags)
# as of now, bytecode header expects 32-bit numbers for size and mtime (#4903)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed it before, but this comment seems to negate this change -- it takes the lower 32 bits while st_mtime_ns needs at least 64 bits, e.g.

>>> import os
>>> st = os.stat('/home')
>>> print(st.st_mtime_ns)
1669828167393028173
>>> print(st.st_mtime_ns & 0xFFFFFFFF)
23537741
>>> print(int(st.st_mtime) & 0xFFFFFFFF)
1669828167

Basically, the pyc timestamp format is 32 bits, and we are trying to stay compatible with it (even though it's not absolutely necessary as we do our own loading). The same problem occurs with python itself, not just pytest.

See:
python/cpython#121376
https://discuss.python.org/t/change-pyc-file-format-to-record-more-accurate-timestamp-and-other-information/57815

Also related: #11418

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct, this is more complex than I expected, I used to run the same test without pytest, but it didn't reproduce somehow, so I thought it's only caused by pytest. Actually, the similar issue still exists in python's import system, and pytest's cache rewrite inherited the same issue. Looks like no proper solution for the issue until now, perhaps we should close the PR.

@nicoddemus
Copy link
Member

Closing then as #13293 (comment), we can easily reopen if needed.

Thanks @jnhyperion!

@nicoddemus nicoddemus closed this Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants