Skip to content

Clear stdlib ABC caches in purge_modules (fixes flaky 3.10 suite) - #381

Merged
JarryShaw merged 1 commit into
mainfrom
fix/purge-modules-abc-cache
Sep 14, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/purge-modules-abc-cache

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Fixes a latent test-harness bug that surfaces as intermittent, order-dependent failures on Python 3.10. It is what blocks CI on #378, but it is not an ESP bug — it predates that branch and affects the whole suite.

Symptom

On 3.10, the full suite intermittently fails one of:

  • test_context_registry_normalisationRegistryError: not a protocol context: 'ESP'
  • test_*_option_constructors_cover_* (ipv4/mh/ipv6) → AttributeError: 'dict' object has no attribute 'to_dict'

Every one of these passes when its file runs alone. Which test fails depends on collection order.

Root cause

tests/_support.purge_modules(['pcapkit']) — called in almost every setUp — drops pcapkit from sys.modules so the next test re-imports it fresh. But the stdlib collections.abc ABCs are never purged. Each re-import rebuilds pcapkit's Mapping subclasses (Info, Schema, ContextRegistry, ProtocolContext, EnumSchema) as brand-new class objects, and that repeated subclass churn corrupts the C-level _abc_impl instance-check caches on the shared ABCs.

The caches then return stale answers for immortal built-ins:

  • isinstance({}, collections.abc.Mapping)False, so ContextRegistry.make({'ESP': ctx}) skips its Mapping branch, iterates the dict's keys, and calls register('ESP').
  • isinstance({}, Schema)True, so Schema.to_dict() (schema.py:437) is called on a plain dict.

It self-heals by teardown as the cache token advances, which is exactly why it is invisible per-file and only bites the full suite.

Confirmed pre-existing: the commit before the ESP const/vendor work fails identically, and pcapkit/corekit/infoclass.py:270 already carries a defensive isinstance(dict_, (dict, collections.abc.Mapping)) — a prior workaround for the same fragility in one spot.

Fix

purge_modules now resets the ABC caches after the purge, via abc._reset_caches over the collections.abc members. It is a CPython internal (present on the C _abc and pure-python _py_abc backends), guarded with getattr so a future runtime without it degrades to the old occasionally-flaky behaviour rather than erroring. gc.collect() was tried and does not work — the poison is in the ABC caches, not in dead subclasses.

Verification (Python 3.10)

#378 will be rebased onto this once it merges.

purge_modules() drops pcapkit from sys.modules so the next test re-imports it
fresh, but the collections.abc ABCs are never purged. Every re-import rebuilds
pcapkit's Mapping subclasses (Info, Schema, ContextRegistry, ProtocolContext,
EnumSchema) as new class objects, and that churn corrupts the C-level
_abc_impl instance-check caches on the shared ABCs. Those caches then give
stale answers for immortal built-ins: isinstance({}, collections.abc.Mapping)
returns False, or isinstance({}, Schema) returns True, until the cache token
happens to advance.

The effect is order-dependent and self-heals by teardown, so every test file
passes in isolation while the full suite fails intermittently on Python 3.10 -
ContextRegistry.make() taking a dict down its Iterable branch and registering
the dict's keys, or Schema.to_dict() being called on a plain dict. The
maintainer had already worked around one instance of this at
pcapkit/corekit/infoclass.py:270 with a belt-and-braces isinstance check.

purge_modules now resets the ABC caches (abc._reset_caches, guarded so a future
runtime without it degrades rather than errors) after the purge, which fixes
the root rather than another symptom.

3.10 CI selection: 347 passed, 14 skipped, 156 subtests passed, 0 failed.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new _reset_abc_caches() implementation calls abc._reset_caches with an argument, which is incompatible with CPython’s no-arg internal API and will raise TypeError during test setup.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR addresses intermittent, order-dependent test failures on Python 3.10 by strengthening the test harness’s module-purge behavior to avoid stale collections.abc instance-check cache results across repeated pcapkit re-imports.

Changes:

  • Add a helper to reset stdlib ABC caches after pcapkit is purged from sys.modules.
  • Invoke the ABC-cache reset from purge_modules to reduce cross-test contamination.
File summaries
File Description
tests/_support.py Adds _reset_abc_caches() and calls it from purge_modules() to mitigate flaky failures caused by stale ABC instance-check caches across repeated re-imports.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/_support.py

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The change is test-harness-only, uses guarded internal APIs with a safe fallback, and directly targets the documented Python 3.10 flakiness mechanism.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@JarryShaw
JarryShaw force-pushed the fix/purge-modules-abc-cache branch from 62b8303 to d0def08 Compare September 14, 2026 22:23
@JarryShaw

Copy link
Copy Markdown
Owner Author

Reverted the tests: handle abc cache reset API variants commit (force-pushed back to d0def088) — it broke the whole suite rather than hardening it, and the concern it addressed does not hold.

abc._reset_caches has a stable signature across CPython versions: it takes exactly one argument, the class. There is no no-arg form. So the amended code's reset() raises TypeError on every supported Python (3.10 and 3.14 both confirmed: _abc._reset_caches() takes exactly one argument (0 given)), which is swallowed, and control falls to the _abc_caches_clear() fallback. That fallback is what breaks CI: on 3.13/3.14 Mapping._abc_caches_clear is an unbound method requiring cls, so calling it with no argument raises TypeError: ABCMeta._abc_caches_clear() missing 1 required positional argument: 'cls' — and that raise is not inside the try, so it propagates straight out of purge_modules. Since almost every test's setUp calls purge_modules, every one of them errors — hence "all failing".

The original per-class form (abc._reset_caches(obj) for each collections.abc ABC) is the correct call and is already guarded with getattr(abc, '_reset_caches', None), which degrades gracefully if a future runtime ever drops the helper. Verified green on 3.10 (347 passed / 14 skipped / 0 failed) and applied onto #378's branch (328 passed / 27 skipped / 0 failed).

@JarryShaw
JarryShaw merged commit 114726c into main Sep 14, 2026
98 checks passed
@JarryShaw
JarryShaw deleted the fix/purge-modules-abc-cache branch September 17, 2026 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants