Prevent sys.modules cleanup during concurrent imports - #2250

Not a maintainer, but I hit #2247 independently and can confirm this is the right fix.

I built a deterministic reproducer (no reliance on timing luck) and ran it against this branch. It installs a meta-path finder for victim_slow whose exec_module blocks on an Event, and patches file_and_path_for_module — which runs inside the sys_modules_saved() block — to wait until the victim thread is mid-exec_module. Then it mutates sys.path and calls should_trace(), which is what 6208c42e (#2082) made re-run set_matchers_depending_on_syspath().

Against current main:

restore() deleted: ['victim_slow']
victim import outcome: KeyError: 'victim_slow'

With this PR's patch applied:

restore() deleted: [[], []]
'victim_slow' still in sys.modules = True
victim import outcome: no error

The detail I find most convincing is in between those two lines: with the patch, the victim thread never reaches exec_module at all while the block is open. It is parked on the import lock for the whole window, so there is nothing left to delete when restore() runs. The interleaving becomes impossible, not merely improbable.

Why this is better than the fix I tried. My own attempt was to have SysModuleSaver.restore() skip modules whose spec._initializing is still true. That also makes my reproducer pass, but it is check-then-act: it shrinks the window rather than closing it, and it depends on ModuleSpec._initializing, which is private in the same way _ImportLockContext is. Holding the import lock is the better trade. I did not open a PR for mine.

On the private API, since that is the obvious objection to this approach. importlib._bootstrap._ImportLockContext is not public, so I checked how far the risk actually reaches:

• It exists and works on CPython 3.13 and 3.14. I measured a concurrent import_module in another thread blocking for the full 1.5s that the context was held.
• Per-module locks largely superseded the global import lock back in 3.3, so it was worth confirming the global one still does anything — it does, and it blocks the entire competing import, not part of it.
• This PR's own CI run already exercises pypy-3.10, pypy-3.11, 3.14t and 3.15t across ubuntu/macos/windows, all green. So every interpreter coveragepy currently ships for provides it today, including the free-threaded builds where the import machinery differs most.

It could still disappear in some future CPython, so the # type: ignore[attr-defined] is doing real work and a defensive fallback would be belt-and-braces. I do not think it is a blocker given the above.

Two smaller points:

• Blast radius is one call site. sys_modules_saved() is used exactly once, at coverage/inorout.py:313. The lock serialises imports against that one block, not against the tracer generally.
• tests/test_misc.py passes for me with the patch applied (38 passed), and the new test_doesnt_interrupt_another_thread_importing is a genuine regression test — it fails without the fix, because started.wait(0.1) returns True when the importer is free to run.

@DSeaStar you've been very active in this repo, thanks! Do you use Discord? I'd like to chat with you in the #coveragepy channel if that works for you.

You have correctly identified that my concern about this is the private import lock.

Claude had some ideas for other ways to solve the problem: https://claude.ai/share/4812a770-3d00-44c2-a8a6-d0a81a99c7e3

@nedbat Thanks — I don't use Discord, but I'm happy to keep this here (or by email if you'd rather take it off-thread).

I can't open the claude.ai share from where I am — the domain redirects me to an "unavailable in region" page — so I can't see the alternatives Claude came up with. If you paste the shortlist inline I'll work through it properly; I'd rather not guess at what's in there and re-propose something already rejected.

Two things to add to what I wrote above, one of which I think changes the calculus.

1. Correction: sys_modules_saved() has two call sites, not one. I said above it was used only at inorout.py:313. It is also used at misc.py:96, inside import_third_party(). That matters for this PR, because import_third_party() does a full import_module() inside the block, not a spec lookup. Holding the global import lock across a whole third-party import is a materially heavier prospect than the inorout case: if the module being imported starts a thread that imports something, that thread blocks on the lock we are holding. (I checked that the lock is reentrant for the same thread on 3.13, so the ordinary cases are fine — I'm flagging this as specific to the import_third_party caller, not as a blocker.)

2. There is a way to fix the reported bug with no lock and no private API.

The load-bearing fact is that file_and_path_for_module() (inorout.py:105) does not import anything — it calls importlib.util.find_spec(). I verified on 3.13:

• find_spec("pkg") on a cold sys.modules adds nothing at all.
• find_spec("pkg.sub") adds only pkg, and executes pkg/__init__.py.
• The target module itself is never registered. (For contrast, import_module("pkg.sub") does register pkg.sub.)

So at the inorout.py:313 call site, the set of modules the block can possibly introduce is statically known: the proper prefixes of the source_pkgs entries, and nothing else. That makes a second fix available — narrow what restore() removes instead of locking the whole block:

class SysModuleSaver:
    def __init__(self, expected: Iterable[str] = ()) -> None:
        self.expected = set(expected)
        self.old_modules = set(sys.modules)

    def restore(self) -> None:
        for m in set(sys.modules) - self.old_modules:
            if m in self.expected:
                del sys.modules[m]

with inorout.py passing the ancestor names for each source_pkg.

The consequence I would dwell on: for a dot-free source_pkgs entry — the common case — find_spec adds nothing, so today's restore() cannot do any useful work at that call site. Every module it deletes there is collateral damage. That is precisely the #2247 failure. Narrowing the set removes that entire class of bug without going near the import lock.

Residual: if another thread genuinely imports one of those ancestor packages concurrently, we could still delete it. That is a much narrower window than "any module in the process", and it is the one case where the lock would still buy something.

So the choice as I see it is: this PR's lock (closes the race outright; costs a private API and serialises imports at two call sites) versus narrowing (no private API, no lock, fixes the reported symptom, leaves the ancestor-package case open). If you prefer the lock I think the patch here is correct and my only real objection is the maintenance risk. Happy to write up either one — say which.

It's getting to be hard to follow all of the threads here. I want the reproducer in #2247 turned into a test in the test suite, with enough explanation that it can be understood. It's using exotic import tools, so some commentary would be good. Then we can discuss the best solution.

Claude suggested using audit hooks to track exactly what modules we were importing:

sys.addaudithook() with the "import" audit event (PEP 578, Python 3.8+). It fires for every module import, including the parent-package imports that importlib.util.find_spec triggers internally, so you can capture module names as they happen instead of diffing sys.modules before/after.

Since the hook fires globally for all threads, filter by thread identity so you only see imports your call caused, not a concurrent one:

import sys
import threading
import contextlib

_local = threading.local()

def _import_audit_hook(event, args):
    if event == "import" and getattr(_local, "stack", None):
        _local.stack[-1].append(args[0])  # args[0] is the module name

sys.addaudithook(_import_audit_hook)  # install once, at module load

@contextlib.contextmanager
def sys_modules_saved():
    if not hasattr(_local, "stack"):
        _local.stack = []
    imported = []
    _local.stack.append(imported)
    try:
        yield
    finally:
        _local.stack.pop()
        for name in imported:
            sys.modules.pop(name, None)

This is fully public API, and it sidesteps the lock entirely: you're not diffing shared state or racing another thread's import — you only ever remove names that the audit hook attributed to your own thread during your own with block.

Caveats:

• sys.addaudithook hooks can't be removed once added, so install it once at import time, not per-call (as above).
• The "import" event is documented to fire on the attempt, before the module is necessarily fully in sys.modules; in practice for your use case (cleanup on failure) that's fine since you still want to remove partially-initialized entries.
• Thread-local stack means nested sys_modules_saved() calls on the same thread compose correctly (each level only pops its own names).

I worked with Claude to turn #2247's reproducer into a test that could live in the test suite:

    def test_2247(self) -> None:
        # Coverage.py sometimes imports modules for its own purposes, and
        # afterward removes anything new from sys.modules so the import leaves
        # no trace.  But sys.modules is process-wide: a module that another
        # thread imported during that window is removed too, and that thread's
        # import then fails.
        #
        # The window is normally microseconds wide.  Three Events hold it open
        # and force one exact interleaving of the two threads:
        #
        #   1. main enters coverage's window, sets `window`, and waits.
        #   2. the victim thread wakes and imports victim_slow.  importlib
        #      registers the module in sys.modules and calls exec_module,
        #      which sets `in_exec` and waits.
        #   3. main wakes and leaves the window, so coverage deletes
        #      victim_slow.
        #   4. main sets `release`.  The victim wakes, exec_module returns, and
        #      importlib looks victim_slow back up in sys.modules -> KeyError.
        #
        # `release` is what makes the failure deterministic: it holds the
        # victim inside exec_module until the delete has already happened.
        window, in_exec, release = (threading.Event() for _ in range(3))
        victim_exc: BaseException | None = None

        orig = coverage.inorout.file_and_path_for_module

        def hooked(name: str) -> tuple[str | None, list[str]]:
            # Coverage calls this from inside the window, so this is step 1.
            window.set()
            in_exec.wait(10)
            return orig(name)

        # The loader and finder import a fake module named "victim_slow",
        # standing in for any real import slow enough to overlap coverage's
        # window.  The loader stalls in exec_module to make it slow;
        # create_module returns None so importlib builds the module itself.
        class VictimLoader(importlib.abc.Loader):
            """Loads victim_slow, slowly."""

            def create_module(
                self, spec_unused: importlib.machinery.ModuleSpec
            ) -> ModuleType | None:
                """Let importlib make the module object itself."""
                return None

            def exec_module(self, module_unused: ModuleType) -> None:
                """Stall, so the import overlaps coverage's window."""
                # Step 2: importlib has already put the module in sys.modules.
                in_exec.set()
                release.wait(10)

        class VictimFinder(importlib.abc.MetaPathFinder):
            """Finds victim_slow, and nothing else."""

            def find_spec(
                self,
                fullname: str,
                path_unused: Sequence[str] | None = None,
                target_unused: ModuleType | None = None,
            ) -> importlib.machinery.ModuleSpec | None:
                """Claim victim_slow, and nothing else."""
                if fullname == "victim_slow":
                    return importlib.util.spec_from_loader(fullname, VictimLoader())
                return None

        def victim_import() -> None:
            nonlocal victim_exc
            window.wait(10)
            try:
                import victim_slow  # pylint: disable=import-error, unused-import
            except Exception as exc:
                # The threading machinery would only print this to stderr,
                # failing nothing, so hand it back to the main thread instead.
                victim_exc = exc

        finder = VictimFinder()
        old_path = list(sys.path)
        victim = threading.Thread(target=victim_import, daemon=True)
        coverage.inorout.file_and_path_for_module = hooked  # type: ignore[assignment]
        sys.meta_path.insert(0, finder)
        try:
            victim.start()
            # The package doesn't have to exist: the damage is done by the
            # window, not by what coverage imports inside it.
            cov = coverage.Coverage(source_pkgs=["some_pkg_that_does_not_exist"])
            try:
                # Opening the window, with step 3 happening as it closes.
                cov.start()
                cov.stop()
                # Changing sys.path makes coverage open the window again while
                # measuring, so this isn't only a start-up hazard.
                sys.path.append("/nonexistent")
                assert cov._inorout is not None
                cov._inorout.should_trace("/some/module.py", None)
            finally:
                cov.stop()
            in_modules = "victim_slow" in sys.modules
        finally:
            release.set()  # step 4
            victim.join(10)
            coverage.inorout.file_and_path_for_module = orig
            sys.meta_path.remove(finder)
            sys.path[:] = old_path
            sys.modules.pop("victim_slow", None)

        # Both of these fail today.  The KeyError is what users actually
        # report; the missing sys.modules entry is the cause, checked
        # separately so a partial fix can't quietly pass.
        assert victim_exc is None, f"victim thread's import raised {victim_exc!r}"
        assert in_modules, "coverage deleted the victim thread's module"

I ran the test you posted, so this isn't a "looks right to me". Three findings.

1. It is a valid regression test

On unpatched main it fails with exactly the reported symptom:

victim thread exception : KeyError('victim_slow')
'victim_slow' survived  : False

With #2250's misc.py change applied, it passes. Worth merging.

2. The 10-second waits are dead weight, and steps 2-4 describe the unfixed behaviour only

I logged every opening of the window. Unpatched:

window openings (t, victim reached exec_module): [(0.223, True), (0.235, True)]

The victim reaches exec_module in about 12 ms, so the interleaving your steps 2-4 describe really does happen.

With #2250:

window openings (t, victim reached exec_module): [(1.231, False), (1.243, True)]

The first window waited the entire timeout and in_exec was still unset — the victim was blocked at the import lock for the whole window and never reached exec_module at all. The second window opened after the lock was released, by which point victim_slow was already in the saver's snapshot, so restore() correctly left it alone.

That is the fix working. But it also means that with any lock-based fix no interleaving ever occurs, so in_exec.wait(10) always burns the full ten seconds. I measured the test at 10.4 s under the patch.

Dropping the three waits to 1 s / 5 s / 5 s brings it to 1.4 s under the patch, and it still fails on unpatched main in 0.36 s — the victim needs ~12 ms to reach exec_module, so 1 s is an ~80x margin and stays deterministic. I verified both directions. I'd also note in the comment that steps 2-4 only describe the unfixed path, otherwise the next reader will be confused about why the timeout is there.

3. On the audit-hook alternative

I was suspicious of it, so I checked the part that worried me. The risk would be that import fires for a module served from sys.modules, which would make restore() evict a module that predates the window — worse than the current bug. It does not:

So recorded names are always freshly-loaded modules, and the thread-local filter correctly ignores the victim thread (confirmed: events raised on another thread see an empty stack). The design is sound, and it also confirms what I found earlier — find_spec() imports only ancestor packages, never the target.

Cost is not a problem either. One audited event goes from 33 ns to 94 ns on 3.13 here (~60 ns per event), and importing unittest raises only 199 events, asyncio 222 — you'd need roughly 17 million audited events to spend a second.

The thing that would decide it for me is PEP 578's API Availability section:

sys.addaudithook() and sys.audit() should exist but may do nothing ... it should not assume that its call will have any effect.

The failure mode is silent: on an implementation where the hook is a no-op, imported is always empty and sys_modules_saved() quietly stops cleaning up — no error, no warning, and the test above would never catch it. #2250's CI already runs pypy-3.10/pypy-3.11, and I can't check PyPy from here.

If you'd rather have the public API, that risk is cheap to close with a one-time self-test at install, falling back to the lock:

probe = "<any module not yet in sys.modules>"
sys.modules.pop(probe, None)
recorded = []
_local.stack.append(recorded)
try:
    __import__(probe)
finally:
    _local.stack.pop()
hook_works = bool(recorded)

I verified this on CPython 3.13: a fresh import is recorded, and a cached re-import records nothing — so it is not a false positive.

My own read: the lock is three lines, already green on every interpreter your CI covers including free-threading, and fails loudly if the private API ever disappears. The hook is public API but needs the self-test above before I'd trust it on PyPy. Either way, the test is the piece worth landing first — it fails for the right reason and doesn't care which fix you choose.

It depends on the call site, and there are two:

• inorout.py:313 — a file_and_path_for_module() loop (find_spec). Short.
• misc.py:96 import_third_party() — a full import_module(). That's the long one you're picturing.

#2247 only needs the first. So I'd take the lock out of sys_modules_saved() and put it around the inorout.py block instead. import_third_party() has the same race today; locking it would block every other thread's imports for a whole third-party import, which is worse than the bug it fixes.

(The lock is re-entrant per thread, so nested imports on the same thread are unaffected.)

I can't push to this branch — say the word and I'll paste the diff.

The new test has two problems in CI, both fixable.

1. Pylint etc fails — tests/test_misc.py:214 and :221, C0116 on VictimLoader.exec_module and VictimFinder.find_spec. VictimLoader.create_module is not flagged: pylint exempts overrides, and it overrides importlib.abc.Loader.create_module (which has a docstring); exec_module and find_spec have no base method to inherit one from, so they need their own.

2. The test is flaky — it failed on the 3.12 and 3.15 macOS jobs with:

assert victim_exc is None          <- passed
assert in_modules                  <- failed: coverage deleted the victim thread's module

That combination doesn't describe the bug. victim_exc is None means the victim's import never raised, so nothing was actually deleted out from under it; it means the victim simply hadn't imported yet when in_modules was read.

I instrumented the interleaving and ran it locally (Python 3.14, Windows). With the lock in place the victim can never get inside the window, so in_exec.wait(10) can only ever time out:

win1 open   t=0.042   victim_in_sys=False
win1 waited in_exec=False  t=10.042    <- full 10s timeout
win1 closed t=10.042
victim exec_module enter   t=10.043    <- only now, after the lock is free
win2 open   t=10.048  victim_in_sys=True
win2 closed t=10.048

The victim reaches exec_module about 5 ms after window 1 closes, and whether the test passes comes down to whether that landed before in_modules was read — a pure scheduling race. 6/6 runs passed here; with the CPU loaded the same runs stretched to 22–49 s, so the margin is milliseconds wide, which is what a macOS runner under xdist will lose. I couldn't reproduce the failure locally, only show that the window is that narrow.

The fix is to take the reading after the victim has definitely finished:

        finally:
            release.set()
            victim.join(10)
            in_modules = "victim_slow" in sys.modules
            coverage.inorout.file_and_path_for_module = orig
            sys.meta_path.remove(finder)
            sys.path[:] = old_path
            sys.modules.pop("victim_slow", None)

Both directions then hold deterministically: unpatched main fails in 0.11 s with KeyError('victim_slow'), this branch passes (10.3 s). The dead wait is inherent — once the lock closes the race, no interleaving ever happens, so in_exec.wait() can only time out. If you want the time back, the waits can come down: at 5 s it's 5.1 s patched / 0.11 s unpatched, and the victim needs ~10 ms to reach exec_module, so the margin is still large.

I can't push to this branch — say the word and I'll paste the full diff.

Quality is green now — the docstrings cleared Pylint etc.

Tests/Coverage are still red, and the new head pins down why. In the 3.10/macOS job (run 34821080644, job 103902593370) this test ran four times inside the same job, and the result tracks the tracer core, not chance:

• ctrace — 1630 passed, 23 skipped → passed
• pytrace — 1 failed, 1598 passed, 54 skipped → failed (assert in_modules, tests/test_misc.py:268)
• the --lf -vvvvv reruns repeat it: ctrace 1 passed in 11.40s, pytrace failed again (~20 s)

So it isn't a coin flip. Under PyTracer the victim thread is slow enough that it has not imported victim_slow yet when line 258 reads sys.modules. victim_exc is None passes because nothing was deleted — the import simply hadn't happened yet.

The read is the problem, not the fix:

             finally:
                 cov.stop()
-            in_modules = "victim_slow" in sys.modules
         finally:
             release.set()
             victim.join(10)
+            in_modules = "victim_slow" in sys.modules

Reading it after join() makes the assertion wait for the victim instead of racing it. I measured both orders locally (Python 3.14, Windows): unpatched → KeyError('victim_slow') in 0.06 s, i.e. still fails for the right reason; patched → passes. I could not reproduce the pytrace failure locally — on this box the victim wins by ~4 ms on every run — so the core-vs-core reading above comes from the job log, not from a local repro.

Two smaller things while you're in there:

• With the lock in place in_exec.wait(10) always burns the full 10 s, because the interleaving never happens; the victim only reaches exec_module milliseconds after the window closes. Those 10 s numbers can come down a lot.
• victim.join(10) can itself time out (that is probably the ~20 s in the pytrace log), so an assert not victim.is_alive() after the join would make a genuine hang report as a hang, rather than as "coverage deleted the module".

Tests is fully green now — moving in_modules after victim.join() cleared both macOS failures.

Coverage still fails, but in exactly one job: 3.15t on ubuntu, under pytrace (ctrace in that same job: 1619 passed, 34 skipped). The signature is new, and it comes from the victim thread's import itself:

AttributeError: 'NoneType' object has no attribute 'add'

That job passed on the previous head ce5323b5 (all 28 matrix jobs success there), and this diff only changes when main reads sys.modules — it can't reach the victim thread's import_module call. So this looks timing/environment specific rather than a regression from this commit; I'd re-run the 3.15t job before chasing it. I can't reproduce locally (no free-threaded 3.15 here), and I haven't identified which .add is on None — flagging it as observed, not diagnosed.