Skip to content

fix(registry): unregistering a custom protocol removes its name from __proto__ (#1505) - #1530

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1505-registry-name-leak
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1505-registry-name-leak

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

Description of your pull request and other information

All nine code registrars end in register_protocol(module) (protocols.py:235). Nothing undid that write, so registering a code back restored the code but left the name.

  • Their tails now call a new _register_name(module, *(destination, code)). When the write changes a name, it pushes a lease onto that name's stack, recording what it displaced and which codes hold it.
  • Each lease stands for one write, not one class, so one class can hold two leases: a built-in can take its name back from a custom class shadowing it. Releasing the top lease restores what it displaced. Releasing a buried lease passes what it displaced to the lease above it.
  • A name that existed before the registration is never leased, so a restore never removes it. register_linktype and register_apptype hold the name from every destination they write.
  • The lease tables are guarded by a module RLock, not .pop(key, None): a lock keeps the stack consistent as well as avoiding KeyError.
  • The Gap(1505) rows are deleted. test_registry_name_restore_unit.py is new: 6 of its 8 tests fail on main, and 2 fail on 5c16f4f47 (the cycle test and the fuzz).
  • The fuzz is seeded: 2000 sequences, checked after every call. It finds residue on 1366/2000 sequences on main, 96/2000 on 5c16f4f47 and 0 on this head. 30000 more sequences on 3 other seeds also come back clean.
  • foundation.py has no sibling: none of its registrars writes a second registry. fix(protocols): restoring a protocol registration puts back the class, not the shipped ModuleDescriptor #1504's sites are untouched.

Ran one at a time, on 81efa6035: tests/foundation/registry, tests/protocols/test_dispatch_registry_unit.py, tests/project, tests/test_docstring_contract.py, tests/test_tier_guard.py, all green. mypy and pylint report nothing new on protocols.py.

Closes #1505

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 5c16f4f47. Cross-reviewed on Sonnet; the author ran Opus.

Confirmed:

Blocking: a lease cycle leaks the custom name, a regression from main. I re-derived it in a git archive tree of each commit:

register_tcp(80, Custom_IPv4); register_tcp(8080, builtin_IPv4)
register_tcp(8080, shipped_8080); register_tcp(80, shipped_80)
  • main ends with names['IPV4'] as the built-in IPv4.
  • This PR ends with probe.IPv4, the custom class.

When two leases displace each other, _unlink_name drops the other lease even though it still has holders. A fuzz of 5,000 sequences mixing custom classes with built-in names left 149 residues.

Should fix:

  • With 8 threads cycling register and restore, del _NAME_LEASES[...] and del _CODE_LEASES[...] (:290, :293, :317) raise KeyError. main does not raise there. Use .pop(key, None) or a lock.
  • The register_protocol docstring still says the registrars end in register_protocol(module).

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 90.09% (unit tier, Python 3.14, 87dae5977, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 843 2356 756 92.08%
pcapkit/corekit 2161 66 700 33 96.26%
pcapkit/dumpkit 242 2 88 2 98.79%
pcapkit/foundation 2740 101 974 47 94.94%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 17086 238 4628 190 97.98%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 431 4 124 4 98.56%
pcapkit/vendor 4409 2342 1006 157 43.25%

Per-file detail: the coverage-html artifact of this run.

…__proto__ (#1505)

- The nine code registrars (register_linktype, register_pcap, register_pcapng,
  register_ethertype, register_transtype, register_apptype, register_tcp,
  register_udp, register_sctp) end in a new _register_name: register_protocol,
  plus a lease on the by-name entry when the write changed it, recording what
  it displaced and which (destination, code) pairs still hold it.
- Leases stack per name, one per write, so a class can hold two (a built-in
  retaking its name from a custom shadow). Releasing the top one restores what
  it displaced; releasing a buried one hands that to the lease above. A name
  held before the registration is never leased, so no restore removes it.
- The lease tables are guarded by a module RLock.
- Delete the Gap(1505) rows; add test_registry_name_restore_unit.py, with a
  seeded fuzz checked after every call.
@JarryShaw
JarryShaw force-pushed the fix/1505-registry-name-leak branch from 5c16f4f to 87dae59 Compare October 9, 2026 23:21
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 87dae5977. This was a Sonnet delta review; the author ran Opus.

  • The lease cycle is fixed. I re-derived the 4-call repro on an archive of this head: names['IPV4'] ends as the built-in.
    • The reviewer fuzzed 20,000 trials in each of three pools (distinct, colliding and built-in-colliding names) and found 0 residue and 0 leftover leases. 5c16f4f47 left 149 residues.
    • A new all-registrar fuzz checks the lease-table invariants after every call, over 33,000 trials: 0 problems. The same harness reports 146 against 5c16f4f47.
  • Threads. 8 threads × 3,000 cycles at a 1e-6 switch interval raise 0 exceptions, where 5c16f4f47 raised 161 KeyErrors.
  • Deadlock. The RLock is taken in one place (protocols.py:371), and no import happens under it. A showwarning hook that re-enters a registrar is fine.
  • Tests. Fail-before reproduces at 96/2000 on 5c16f4f47 and 1366/2000 on main.

Non-blocking:

  • The lock does not cover the dispatch write (cls.register(...) runs before _register_name). A forced interleave can briefly leave a name held after its code is restored, until the next registration at that code clears it. Unforced stress did not reproduce it.
  • The in-repo fuzz covers tcp, udp and apptype only.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 9, 2026
@JarryShaw
JarryShaw merged commit c3d4120 into main Oct 10, 2026
40 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 10, 2026
@JarryShaw
JarryShaw deleted the fix/1505-registry-name-leak branch October 10, 2026 01:53
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(registry): a custom protocol's name stays in pcapkit.protocols.__proto__ after it is unregistered

1 participant