fix(corekit): stop a malformed TCP SACK's exception type depending on sys.modules state (#525)

CHANGELOG.md

@@ -34,6 +34,7 @@ The largest release since 1.0, and the first recorded here as it happened rather
 - **Fixed** -- constant lookups that rejected a value the registry defines. `RouterAlert(0)` is the only value [RFC 2113](https://datatracker.ietf.org/doc/html/rfc2113) defines and the one IGMP, RSVP and MLD actually send, and it was discarded because the vendor crawler skipped a header row IANA's CSV does not have; IPX `Socket(0)` is that protocol's own default, so `bytes(IPX(...))` crashed on its own defaults; and two FTP `_missing_` overrides were plain methods rather than classmethods, so every unregistered value raised `TypeError` instead of extending the enumeration (#492, #503).
 - **Fixed** -- `format='text'` raised `AttributeError` before writing anything, naming a `dictdumper.Text` that has never existed. It now points at `Tree`, as the `'txt'` alias beside it already did.
 - **Fixed** -- 45 places where a documentation page contradicted the code (#413), ambiguous cross-references and five autodoc signature failures (#416), and `Extractor`'s documented exception plus 40 phantom or stale `Args:` labels (#501).
+- **Fixed** -- which exception a malformed TCP SACK option raised depended on unrelated process state: a clean interpreter raised `ProtocolError` as documented, but a process that had already popped `pcapkit.corekit.fields.misc` from `sys.modules` -- which the `#439` ABC-cache regression tests do in every case's `setUp`/`tearDown` -- raised `FieldValueError` instead, from a different layer entirely, before the documented check was even reached (#525). The cause was `ListField.unpack` resolving `SchemaField` through a function-local import re-run on every call; a module popped and reimported mid-process comes back as a second, distinct class, so `isinstance` against it silently misclassified the field and billed each item by its declared length instead of by what it actually consumed. **Any caller relying on the previously-observed** `FieldValueError` **for this case now gets** `ProtocolError` **instead, deterministically**, matching the method's own docstring. Fixed by importing at module level instead.
 - **Fixed** -- two dropped-keyword/wrong-cast defects flagged in review during this release and never filed until now: HIP's `_make_param_encrypted` passed `cipher=` to a schema with no such field, so the value was silently dropped and an AES-cipher `ENCRYPTED` parameter built through `make` packed without its IV; and IPv6-Route's `RPL.post_process`, which runs on every `Schema.pack` and not only after a parse, assumed `self.addresses` was still the concatenated `bytes` a parse leaves it as, and raised slicing the `list[bytes]` a `make`-built multi-address header actually holds there (#556).
 
 Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) and `1.5.0b3` (2026-09-19), all published as prereleases and so resolved only by `pip install --pre`. `1.5.0b1` half-shipped: the tag, the GitHub release and the Conda deployments landed, but PyPI rejected the wheel because `twine check` found a Sphinx-only `:mod:` role in `README.rst`, which `pyproject.toml` declares as the dynamic long description. `1.5.0b2` is what reshipped it -- the release workflow is version-driven, so an existing version cannot republish -- and `1.5.0b3` followed the CI change that stops a TestPyPI outage from costing a release its wheels (#497, #498).

docs/source/changelog/1.5.0.rst

@@ -282,6 +282,21 @@ pull requests between #326 and #509.
   (#413), ambiguous cross-references and five autodoc signature failures (#416),
   and ``Extractor``'s documented exception plus 40 phantom or stale ``Args:``
   labels (#501).
+* **Fixed** -- which exception a malformed TCP SACK option raised depended on
+  unrelated process state: a clean interpreter raised ``ProtocolError`` as
+  documented, but a process that had already popped
+  ``pcapkit.corekit.fields.misc`` from ``sys.modules`` -- which the ``#439``
+  ABC-cache regression tests do in every case's ``setUp``/``tearDown`` --
+  raised ``FieldValueError`` instead, from a different layer entirely, before
+  the documented check was even reached (#525). The cause was
+  ``ListField.unpack`` resolving ``SchemaField`` through a function-local
+  import re-run on every call; a module popped and reimported mid-process comes
+  back as a second, distinct class, so ``isinstance`` against it silently
+  misclassified the field and billed each item by its declared length instead
+  of by what it actually consumed. **Any caller relying on the
+  previously-observed** ``FieldValueError`` **for this case now gets**
+  ``ProtocolError`` **instead, deterministically**, matching the method's own
+  docstring. Fixed by importing at module level instead.
 * **Fixed** -- two dropped-keyword/wrong-cast defects flagged in review during
   this release and never filed until now: HIP's ``_make_param_encrypted`` passed
   ``cipher=`` to a schema with no such field, so the value was silently dropped

pcapkit/corekit/fields/collections.py

@@ -6,6 +6,7 @@
 from typing import TYPE_CHECKING, Generic, TypeVar, cast
 
 from pcapkit.corekit.fields.field import FieldBase
+from pcapkit.corekit.fields.misc import SchemaField
 from pcapkit.corekit.fields.numbers import NumberField
 from pcapkit.corekit.multidict import OrderedMultiDict
 from pcapkit.utilities.compat import List
@@ -140,7 +141,34 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'byte
         if self._item_type is None:
             return file.read(length)
 
-        from pcapkit.corekit.fields.misc import SchemaField
+        # NOTE: ``SchemaField`` used to be imported here, inside the method body,
+        # rather than at module level -- there is no cyclic import to dodge by
+        # doing so; :mod:`pcapkit.corekit.fields.misc` does not import this
+        # module. A local ``from ... import`` re-resolves against
+        # :data:`sys.modules` on *every* call, and normally that is a cheap
+        # dict lookup returning the same cached module -- except when something
+        # has popped :mod:`pcapkit.corekit.fields.misc` out of
+        # :data:`sys.modules` since this ``ListField`` was built (the ``#439``
+        # regression tests do exactly that, to get a clean ABC-cache state
+        # between cases). The next call re-executes the module from scratch and
+        # mints a *second*, distinct ``SchemaField`` class, while
+        # ``self._item_type`` -- built long before, from the first one -- is
+        # still an instance of the original. ``isinstance`` against the new
+        # class then reports :data:`False` for an item that plainly is a
+        # ``SchemaField``, ``is_schema`` goes the wrong way, and this method's
+        # other branch bills each item by its *declared* length unconditionally
+        # instead of by what it actually consumed -- which is what let a
+        # malformed TCP SACK option's length check raise
+        # :exc:`~pcapkit.utilities.exceptions.FieldValueError` from here instead
+        # of :exc:`~pcapkit.utilities.exceptions.ProtocolError` from
+        # :meth:`TCP._read_mode_sack
+        # <pcapkit.protocols.transport.tcp.TCP._read_mode_sack>`, depending on
+        # unrelated things that ran earlier in the same process. C.f. #525.
+        # Importing at module level, once, for the life of this module object,
+        # closes that off: ``SchemaField`` then stays whatever class this
+        # module resolved at its own import time, regardless of what happens to
+        # :mod:`pcapkit.corekit.fields.misc`'s entry in :data:`sys.modules`
+        # afterwards.
         is_schema = isinstance(self._item_type, SchemaField)
 
         # NOTE: The item-typed branch below sizes each item by ``field.length``,

tests/protocols/transport/test_tcp_sack_length_unit.py

@@ -33,6 +33,32 @@
 *this* docstring, which is the direction that actually lets a malformed packet
 through.
 
+:meth:`SACKLengthTests.test_invalid_sack_length_is_rejected` used to accept
+either :exc:`~pcapkit.utilities.exceptions.ProtocolError` or
+:exc:`~pcapkit.utilities.exceptions.FieldValueError`, because which one arrived
+depended on process state rather than on the packet -- filed as #525. The root
+cause was :meth:`ListField.unpack
+<pcapkit.corekit.fields.collections.ListField.unpack>` resolving
+:class:`~pcapkit.corekit.fields.misc.SchemaField` through a *function-local*
+import on every call, instead of once at module load: ordinarily a cheap
+:data:`sys.modules` hit, but a class that has been popped from
+:data:`sys.modules` since this field was built (which is exactly what the
+``#439`` ABC-cache regression tests do, in every case's ``setUp``/``tearDown``,
+to get a clean cache between them) comes back as a second, distinct class on
+the next call. ``isinstance(self._item_type, SchemaField)`` then answers
+:data:`False` for an item that plainly is one, ``ListField.unpack`` bills it by
+its declared length instead of by what it actually consumed, and *that* is what
+let the length check go negative and raise
+:exc:`~pcapkit.utilities.exceptions.FieldValueError` from
+:mod:`pcapkit.corekit.fields.collections` before ``_read_mode_sack`` was ever
+reached. Hoisting the import to module level in that fix closed it: the
+exception is now always :exc:`~pcapkit.utilities.exceptions.ProtocolError`,
+regardless of what ran earlier in the process, and the assertion below is
+narrowed to match.
+:meth:`SACKLengthTests.test_sack_exception_type_survives_a_sys_modules_purge`
+pins the mechanism directly, by reproducing the exact
+:data:`sys.modules` manipulation that used to flip it.
+
 One deliberate limit, so a later reader does not think it an oversight: the
 check implements exactly the rule the docstring states, ``(length - 2) % 8 ==
 0``. :rfc:`2018` also wants at least one block and at most four, so ``length=2``
@@ -49,10 +75,11 @@
 from __future__ import annotations
 
 import io
+import sys
 import unittest
 
 from pcapkit.protocols.transport.tcp import TCP
-from pcapkit.utilities.exceptions import FieldValueError, ProtocolError
+from pcapkit.utilities.exceptions import ProtocolError
 
 #: TCP option kind for SACK, per :rfc:`2018`.
 SACK = 5
@@ -117,53 +144,91 @@ def test_valid_sack_lengths_are_accepted(self) -> 'None':
     def test_invalid_sack_length_is_rejected(self) -> 'None':
         """A length that is not ``8n + 2`` is rejected.
 
-        Every one of these parsed without complaint before the check was
+        Every one of these parsed without complaint before the #519 check was
         added, which is the whole point of the case. ``11`` and ``14`` are the
         interesting ones -- a remainder of 1 and of 4 -- because the block list
         happily consumes as many whole 8-octet items as it can find and
         silently ignores the tail.
 
-        Two different exceptions satisfy this, and which one arrives depends on
-        global state rather than on the packet, so the assertion deliberately
-        accepts either:
-
-        * :exc:`~pcapkit.utilities.exceptions.ProtocolError` from
-          :meth:`TCP._read_mode_sack
-          <pcapkit.protocols.transport.tcp.TCP._read_mode_sack>`, the check
-          this module exists for. This is what a freshly started interpreter
-          produces for all six lengths.
-        * :exc:`~pcapkit.utilities.exceptions.FieldValueError` from
-          :meth:`ListField.unpack
-          <pcapkit.corekit.fields.collections.ListField.unpack>`, whose schema
-          branch decrements the remaining length by ``len(data)`` per item and
-          raises once it goes negative. This one was observed in a process that
-          had already run other parts of the suite, where the option schema is
-          unpacked before ``_read_mode_sack`` is reached, so the lower layer
-          notices first and reports instead.
-
-        Be careful how much that second bullet is trusted. The *observation* is
-        real and is filed as #525, but the trigger is not pinned down: a
-        deliberate attempt to force it by running
-        :file:`tests/protocols/schema/` first in the same process did **not**
-        reproduce it, and ``ProtocolError`` still won for all six lengths. So
-        the honest statement is that the exception class depends on process
-        state by some route not yet identified -- not that any particular test
-        ordering selects it.
-
-        Which is why the assertion is a union rather than a single class: a
-        caller cannot reliably catch one, and pinning whichever one happens to
-        arrive here would make this test fail for reasons that have nothing to
-        do with SACK. Naming both rather than a bare
-        :exc:`~pcapkit.utilities.exceptions.BaseError` still keeps a third,
-        unexpected exception a failure. The union records the problem; #525
-        owns solving it.
+        This used to accept either :exc:`~pcapkit.utilities.exceptions.ProtocolError`
+        or :exc:`~pcapkit.utilities.exceptions.FieldValueError`, since which one
+        arrived depended on unrelated process state rather than on the packet
+        -- #525. That is now fixed at the root (see the module docstring and
+        :meth:`test_sack_exception_type_survives_a_sys_modules_purge` below),
+        so the exception is pinned to the single documented type:
+        :exc:`~pcapkit.utilities.exceptions.ProtocolError` from
+        :meth:`TCP._read_mode_sack
+        <pcapkit.protocols.transport.tcp.TCP._read_mode_sack>`, which is the
+        check this module exists for.
 
         """
         for length in (3, 11, 14, 17, 19, 25):
             with self.subTest(length=length):
-                with self.assertRaises((ProtocolError, FieldValueError)) as caught:
+                with self.assertRaises(ProtocolError) as caught:
                     parse(sack_option(length, length + 8))
-                self.assertRegex(str(caught.exception), 'invalid (format|length)')
+                self.assertRegex(str(caught.exception), 'invalid format')
+
+    def test_sack_exception_type_survives_a_sys_modules_purge(self) -> 'None':
+        """The exception type does not depend on :data:`sys.modules` state.
+
+        This pins the actual mechanism behind #525, rather than merely
+        recording the symptom. :meth:`ListField.unpack
+        <pcapkit.corekit.fields.collections.ListField.unpack>` used to resolve
+        :class:`~pcapkit.corekit.fields.misc.SchemaField` through a
+        function-local import, re-run on every call. That is an ordinary
+        :data:`sys.modules` cache hit almost always -- except when something
+        has popped :mod:`pcapkit.corekit.fields.misc` out of
+        :data:`sys.modules` since this ``ListField`` was built, which is
+        exactly what the ``#439`` ABC-cache regression tests do in every
+        case's ``setUp``/``tearDown`` (see
+        :mod:`tests.protocols.schema.test_schema_metaclass_abc_cache_unit`) to
+        get a clean cache between cases. The next call then re-executes
+        :mod:`pcapkit.corekit.fields.misc` from scratch and mints a *second*,
+        distinct ``SchemaField`` class, while the ``sack`` field's
+        ``item_type`` -- built long before, from the first one -- is still an
+        instance of the original. ``isinstance`` against the new class then
+        answers :data:`False` for an item that plainly is a ``SchemaField``,
+        and ``ListField.unpack`` takes its other branch, which bills each item
+        by its *declared* length unconditionally instead of by what it
+        actually consumed -- letting the length check go negative and raise
+        :exc:`~pcapkit.utilities.exceptions.FieldValueError` from
+        :mod:`pcapkit.corekit.fields.collections` before
+        ``_read_mode_sack`` is ever reached.
+
+        Reproducing that does not need the schema test suite at all -- the
+        :data:`sys.modules` manipulation alone is sufficient, and doing just
+        that is what makes this a targeted regression test rather than a
+        repeat of the same "run some other tests first" guess #525 opened
+        with. Before the fix, this exact removal made
+        :meth:`ListField.unpack <pcapkit.corekit.fields.collections.ListField.unpack>`
+        re-import :mod:`pcapkit.corekit.fields.misc` on its next call and mint
+        the mismatched second ``SchemaField`` class described above; asserting
+        that :mod:`pcapkit.corekit.fields.misc` is *not* back in
+        :data:`sys.modules` immediately after ``parse()`` returns is what
+        confirms the fix (a module-level import, resolved once) rather than
+        just a passing assertion for the wrong reason.
+
+        """
+        self.assertIn(
+            'pcapkit.corekit.fields.misc', sys.modules,
+            'test setup expects pcapkit to already be imported',
+        )
+        removed = sys.modules.pop('pcapkit.corekit.fields.misc')
+        try:
+            with self.assertRaises(ProtocolError) as caught:
+                parse(sack_option(11, 11 + 8))
+            self.assertRegex(str(caught.exception), 'invalid format')
+
+            # The fix hoisted the import to module level, so nothing during
+            # ``parse()`` re-triggers it: the module stays absent from
+            # ``sys.modules`` until this test restores it below. Before the
+            # fix, ``ListField.unpack``'s function-local import would have put
+            # it right back.
+            self.assertNotIn('pcapkit.corekit.fields.misc', sys.modules)
+        finally:
+            # NOTE: restore, rather than leave the module state disturbed for
+            # whatever the test runner collects next.
+            sys.modules['pcapkit.corekit.fields.misc'] = removed
 
     def test_documented_rule_is_the_implemented_rule(self) -> 'None':
         """The check is exactly ``(length - 2) % 8 == 0``, no more.