Skip to content

Tolerate concurrent sys.path_importer_cache races in the loader (#61734)#69790

Open
ggiesen wants to merge 2 commits into
saltstack:3006.xfrom
ggiesen:fix-61734-invalidate-caches
Open

Tolerate concurrent sys.path_importer_cache races in the loader (#61734)#69790
ggiesen wants to merge 2 commits into
saltstack:3006.xfrom
ggiesen:fix-61734-invalidate-caches

Conversation

@ggiesen

@ggiesen ggiesen commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Makes the module loader tolerate a shared-cache race that fails module loads on
a busy deltaproxy.

When several loaders run in one process and load modules concurrently -- e.g. a
deltaproxy hosting many sub-proxy minions -- they share the global
sys.path_importer_cache. The loader's __clean_sys_path runs after each
module load and calls importlib.invalidate_caches(), which walks that cache
and deletes stale entries. If another loader already removed an entry that
CPython is walking, invalidate_caches() raises KeyError (or RuntimeError
if the mapping changed size mid-iteration). That exception propagated up through
_load_module and failed the module function that happened to trigger the
cleanup, e.g.:

File ".../salt/loader/lazy.py", line ..., in __clean_sys_path
    importlib.invalidate_caches()
File ".../importlib/__init__.py", line 71, in invalidate_caches
    finder.invalidate_caches()
File "<frozen importlib._bootstrap_external>", line ..., in invalidate_caches
KeyError: '/usr/local/lib/python37.zip'

The fix wraps invalidate_caches() so the race is tolerated -- the entry was
going to be invalidated regardless -- and drops stale FileFinder entries from
sys.path_importer_cache with pop() instead of del, so the same concurrent
removal cannot raise between the presence check and the delete.

What issues does this PR fix or reference?

Fixes #61734

Previous Behavior

On a deltaproxy running many sub-proxy minions, a scheduled/custom module call
would intermittently fail with KeyError from importlib.invalidate_caches.
The reporter notes it scales with the number of sub-proxies per deltaproxy and
that catching the error in the loader resolves it.

New Behavior

The concurrent sys.path_importer_cache race is tolerated; the module load
completes.

Merge requirements satisfied?

  • Docs
  • Changelog
  • Tests written/updated

tests/pytests/unit/loader/test_lazy.py::test_clean_sys_path_tolerates_shared_cache_race
patches importlib.invalidate_caches to raise KeyError/RuntimeError and
asserts __clean_sys_path does not propagate it. It fails against the current
code. (The race itself is timing-dependent and hard to reproduce
deterministically -- matching the report -- so the test exercises the tolerance
directly.)

Independent of #69787 (both touch salt/loader/lazy.py but in different
functions -- __populate_sys_path/_load_module there vs __clean_sys_path
here -- so they merge cleanly).

Commits signed with GPG?

No

…stack#61734)

When several loaders run in one process and load modules concurrently -- e.g. a
deltaproxy hosting many sub-proxy minions -- they share the global
sys.path_importer_cache. importlib.invalidate_caches(), called by the loader's
__clean_sys_path after each module load, walks that cache and deletes stale
entries; a second loader that already removed an entry makes CPython raise
KeyError (or RuntimeError if the mapping changed size mid-iteration), which
propagated up and failed the module function that triggered the cleanup.

Wrap invalidate_caches() so that race is tolerated (the entry was going to be
invalidated regardless), and drop stale FileFinder entries from
sys.path_importer_cache with pop() instead of del so a concurrent removal
between the presence check and the delete cannot raise either.
@ggiesen
ggiesen requested a review from a team as a code owner July 12, 2026 00:51
@dwoz dwoz added the test:full Run the full test suite label Jul 12, 2026
@twangboy twangboy added this to the Sulphur v3006.28 milestone Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants