fix(foundation): close the input stream pcapkit opened, not the caller's (#610)

[JarryShaw]

Fixes #610

The defect

Extractor recorded how its input arrived and then acted on the opposite of it:

self._flag_s = isinstance(fin, str)   # set when `fin` is a path, i.e. we open it
...
if self._flag_s:
    self._ifile = open(ifnm, 'rb')    # pcapkit's handle

but _cleanup() closed under not self._flag_s. So it did both halves of the
wrong thing at once — it leaked the handle it owned, and closed the handle it did
not.

The same inverted polarity appeared a second time, one screen up, at the
SeekableReader that wraps a non-seekable input: stream_closing=not self._flag_s.
Because a handle pcapkit opens itself is always seekable and so never wrapped, that
argument was effectively a constant True — every non-seekable caller-supplied
stream was closed along with the wrapper pcapkit had built around it.

Re-verified on current main

6c3d1b0d9, CPython 3.14.7, measured from an immutable git archive snapshot in
/tmp (extraction.py sha256 ebe7967079ca…, re-hashed afterwards and unchanged):

path-given   : open handles to in.pcap after extract = 1
stream-given : caller handle closed by pcapkit = True
stream-given : caller can still read its own file = False (seek of closed file)

The line numbers in the issue have shifted: _cleanup's condition is at
extraction.py:1095-1098, and the stream_closing argument at extraction.py:969.

Closing the caller's stream is the more dangerous half. There is no warning and
no error — a caller that passes an open file it still intends to read simply finds
it closed, which is silent data loss.

The fix

Both sites now consult one predicate, Extractor._owns_input, so they cannot drift
apart on the rule again:

input	who opened it	closed by _cleanup?
fin is a path	pcapkit	yes
fin is a seekable stream	the caller	no
fin is a non-seekable stream	pcapkit wrapped the caller's	the wrapper, yes; the stream under it, no

Why this needed more than the one-line inversion

The issue asks that the fix make the cross-test contamination class impossible
rather than balance the counts, and inverting the condition alone does not. Running
the two files #610 names still left one warning:

tests/integration/test_runtime_extract.py        : 1
tests/protocols/misc/pcap/test_frame_runtime.py  : 0

test_runtime_extract.py:35 builds an auto=False extractor, reads two frames of
six, and walks away. _cleanup is reached on EOF, on interrupt, and from run()
— an extraction abandoned before EOF reaches it by no route at all. So there is an
Extractor.__del__ backstop here as well. It is documented as a backstop and not
the recommended route: collection is not deterministic, and Extractor sits in a
reference cycle through its engine, so the context manager remains the way to
control when the handle goes.

With both parts, the count from #610's own confirmation command goes to zero:

tree	unclosed-in.pcap ResourceWarnings
6c3d1b0d9	2
this branch	0

Evidence

tests/foundation/test_extraction.py asserted the leak was correct —
_flag_s = True, then assertFalse(named_file._ifile.closed_by_test). That is
inverted here, with the caller-supplied direction added next to it.

The new tests/foundation/test_extraction_ownership.py counts handles rather than
asserting the absence of a warning, because a leak test that cannot fail is worse
than none. Two independent measurements: a builtins.open recorder that keeps the
file objects Extractor opened, and a /proc/self/fd descriptor count (skipped
where /proc is absent).

Both trees measured with pcapkit.__file__ printed into the pytest header, since
this venv's editable install appends a finder that maps pcapkit to the main
checkout:

treeprobe: extraction.__file__ = /tmp/base610/pcapkit/foundation/extraction.py

14 failed, 5 passed, 5 warnings in 1.49s          # exit code 1
FAILED  ...::test_cleanup_is_safe_to_reach_twice
SUBFAILED(measurement='builtins.open handles still open') ...::test_path_given_extraction_leaks_no_handle
SUBFAILED(measurement='/proc/self/fd descriptors')        ...::test_path_given_extraction_leaks_no_handle
SUBFAILED(measurement='builtins.open handles still open') ...::test_extractor_abandoned_before_eof_still_releases_its_handle
SUBFAILED(check='handle not closed')      ...::test_caller_supplied_stream_stays_open_and_readable
SUBFAILED(check='handle still readable')  ...::test_caller_supplied_stream_stays_open_and_readable
SUBFAILED(check="caller's stream still open") ...::test_non_seekable_stream_survives_its_seekable_wrapper

with the counts named outright:

E   AssertionError: 1 != 0 : 1 of 1 handle(s) Extractor opened are still open
E   AssertionError: 2 != 1 : the descriptor count for the capture did not return to where it started
E   AssertionError: True is not false : Extractor closed the caller's stream
E   ValueError: seek of closed file
E   AssertionError: True is not false : Extractor closed the caller's stream through the wrapper

and on this branch:

treeprobe: extraction.__file__ = .../agent-afa86f3e6166714f8/pcapkit/foundation/extraction.py
10 passed, 4 warnings in 0.80s                    # exit code 0

Coverage cannot show this: every line the fix touches already executed, and it is
the branch that changed. The evidence axis is therefore the subtest count — 7
behavioural subtests and assertions that previously could not fail, all of which
now can.

Deliberately not fixed

• __exit__ still closes unconditionally. A caller who scopes an Extractor
  with with has asked for exactly that, and an existing test pins it. Changing it
  would be a contract change rather than a defect fix.
• A SeekableReader off-by-one, pcapkit/corekit/io.py:375. read() returns
  min(size, self._buffer_cur - 1) from the buffer, one byte short, so a
  non-seekable stream without peek — a bare io.BytesIO subclass — mis-reads
  the first record and fails with invalid magic number. It is why the existing
  suite only ever drives one with Extractor.run patched out, and why the test
  here uses a BufferedReader over a non-seekable raw, which is sys.stdin.buffer's
  real shape. Not this PR's file.
• SeekableReader.close() leaves a finaliser to fail. After closing, CPython's
  BufferedReader.__del__ calls flush(), which touches the already-closed
  _stream and prints ValueError: I/O operation on closed file as an ignored
  exception. Pre-existing, in io.py, and cosmetic.

[JarryShaw]

Note on landing order — these two PRs touch the same file and will conflict textually, trivially.

#639 (no_eof termination, #620) is independent of this one in substance: different functions, different root cause, no shared logic. But both add a private helper in the same place, between the # Utilities. header and def _cleanup — this PR adds Extractor._owns_input, #639 adds Extractor._eof_progressed. Whichever lands second gets one conflict there, and the resolution is to keep both methods, in either order. Verified with git merge-tree:

<<<<<<< fix/610-extractor-stream-ownership
    def _owns_input(self) -> 'bool':
=======
    def _eof_progressed(self) -> 'bool':
>>>>>>> fix/620-no-eof-termination

    def _cleanup(self) -> 'None':

tests/foundation/test_extraction.py auto-merges cleanly. docs/source/changelog/1.5.0.rst and CHANGELOG.md also conflict, as every concurrent PR does: keep both bullets and re-run python util/changelog_md.py rather than hand-editing the Markdown.

One substantive interaction worth knowing, in this direction only: #639 makes a no_eof extraction terminate, so it starts reaching _cleanup where it previously never did. That means #639 relies on this PR's ownership rule being right for its input handles to be released. Neither blocks the other.

[JarryShaw]

Cross-review: GOOD TO GO

Independent cross-review, as this PR was raised by an agent and AutoSDE-style review alone is not enough. Run on Sonnet 5 — a different model from the one that wrote the change (Opus 5) — briefed to falsify rather than confirm, read-only, with every claim required to come with evidence the reviewer obtained itself.

Provenance of the numbers it worked from: base 6c3d1b0d9, pcapkit/foundation/extraction.py sha256 ebe7967079ca… (matching this PR's stated pre-fix hash); head ecd22abf6, sha256 291226f672…. pcapkit/corekit/io.py is byte-identical on both trees (919e492381…), which independently confirms this PR touches only extraction.py and two test files.

Verdicts

Claim	Verdict
C1 ownership rule correct in all input shapes	CONFIRMED
C2 handle counts (1 → 0; caller's stream closed → open)	CONFIRMED, independently re-derived twice
C3 ResourceWarning 2 → 0	CONFIRMED
C4 new tests fail on baseline for behavioural reasons	CONFIRMED
C5 __del__ is safe	CONFIRMED
C6 nothing else regressed	CONFIRMED
C7 "deliberately not fixed" items real and pre-existing	CONFIRMED (all three)

On C5, which is the riskiest part of this PR because it is new API rather than an inversion, it verified four separate hazards with its own probes: a genuinely half-constructed instance (Extractor(fin='/nonexistent…'), which raises FileNotFound between the _flag_s and _ifile assignments) produced no "Exception ignored in __del__" on stderr; a caller-supplied stream abandoned before EOF came back open and readable; the Extractor ↔ Engine reference cycle stays collectable with __del__ present (weakref died, gc.garbage empty, 14 objects collected — PEP 442 verified rather than assumed); and __exit__-then-__del__ plus _cleanup() twice both raise nothing.

Three things it found that this PR does not mention

1. A fourth input shape exists in the code but is unreachable. _flag_s = True could in principle coexist with _ifile becoming a SeekableReader — a path naming a non-seekable special file. The reviewer confirmed _owns_input handles that case correctly anyway (stream_closing=self._flag_s closes both wrapper and the handle pcapkit opened), and then showed it cannot arise: extraction.py:613 gates on os.path.isfile(ifnm) before open(), and isfile() is False for a FIFO (verified with a real os.mkfifo, mode 0o10644). Regular files are always seekable, so the wrapping branch never fires with _flag_s set. Worth a note, not a defect.

2. The fix is more general than the issue's two files. Besides in.pcap going 2 → 0, the baseline also leaked 4 arp.pcap ResourceWarnings through the same path, and those also go to 0. This PR undersold itself.

3. The 12 tests/project/ failures are a separate pre-existing bug and deserve their own issue. TypeError: type 'ProtocolBase' is not subscriptable, order-dependent: it reproduces verbatim on 6c3d1b0d9 with none of this PR's code present, and disappears when tests/project/ runs in isolation. tests/project/, protocol.py and frame.py are byte-identical between the two trees. Not caused by this PR — but it means a combined run of tests/project/ after other directories is red on main today.

Disagreement it resolved internally, worth recording

Its own sub-agents split on C7(c), the SeekableReader.close() finaliser failure: one could not reproduce it with sys.unraisablehook and explicit gc.collect(). The reviewer reproduced it deterministically 4 times by letting the triggering construction crash unwind to natural, uncaught process exit, and got the identical output on the baseline too — where Extractor.__del__ does not exist — which is what establishes it as a property of the untouched io.py rather than anything this PR introduces.

Baseline/head figures it re-measured for itself

base 6c3d1b0d9 : 14 failed,  5 passed   exit 1
head ecd22abf6 : 10 passed              exit 0

classified as 7 behavioural failures (handle counts 1 != 0, True is not false: closed caller's stream, ValueError: seek of closed file) against 7 API-absence (AttributeError: … no attribute '_owns_input'/'__del__'), which matches this PR's own claim of 7 assertions that previously could not fail.

Not verified by this review, and stated as such: nothing was left unverified. The reviewer confirmed it left no runaway processes and removed its scratch snapshots.

[JarryShaw]

Amended to 7d615f38e to act on the first of the cross-review's three findings — the "fourth input shape".

Comment-only, no behaviour change. _owns_input's docstring now records why its two True cases are written as separate tests rather than folded together: they can in principle coincide (a path naming something non-seekable would be opened here and then wrapped), the answer must be True for both halves of that, and it cannot arise today because make_name admits a path only through os.path.isfile at extraction.py:613 — which is False for a FIFO or a device, and a regular file is always seekable. So the SeekableReader test is defensive rather than dead, and that is why stream_closing=self._flag_s passes the flag instead of a literal False.

Re-verified after the amend: tests/foundation/test_extraction_ownership.py tests/foundation/test_extraction.py tests/project/ → 118 passed, exit 0.

The cross-review's other two findings need no code change here:

• The extra 4 arp.pcap ResourceWarnings that also go to zero are additional evidence for the same fix, not a separate case.
• The 12 order-dependent tests/project/ failures (TypeError: type 'ProtocolBase' is not subscriptable) are pre-existing on main and want their own issue; protocol.py is not touched here.

[JarryShaw]

Rewritten: changelog entry moved out, rebased onto current main

This branch was force-pushed. Head is now 6d1570f90, one commit on top of
7b0df4a9c (origin/main at the time of the push).

What was dropped: this PR's bullet in docs/source/changelog/1.5.0.rst, and the
CHANGELOG.md regeneration that came with it. Nothing else. The code patch is
byte-identical to what was reviewed — verified by diffing the old and the new
patch restricted to the non-changelog paths, which came back empty. The commit
message is byte-identical too.

Where the entry went: #657, the shared long-lived changelog PR for the 1.5.0
cycle, verbatim — not reworded, reflowed or trimmed. It merges last.

Why: five open PRs each added a bullet at the same anchor in the entry file. Any
one of them merging re-conflicted the other four on the two changelog files, even
though none of their code files overlapped at all. That cycle had already cost this
wave thirteen rebases.

This PR now touches pcapkit/foundation/extraction.py and two test files — 3 files,
down from 5.

One real conflict remains, and it is not the changelog

This PR and #639 both insert a private helper between the # Utilities. header and
def _cleanup in pcapkit/foundation/extraction.py — _owns_input here,
_note_eof_progress there. Removing the changelog does not dissolve that;
merge-tree still reports it after the rebase. Resolution is keep both — they are
independent methods, and the merged file parses with both present.

Merge this one before #639. #620's fix makes no_eof reach _cleanup where it
previously never did, so it depends on this PR's ownership rule being right. Expect to
resolve that one hunk keep-both on #639. Every other pair among the five is
conflict-free in either order.

Heads-up on red CI, which this PR does not cause

Changelog drift and the matrix jobs fail here, and would fail on any branch cut from
current main. main itself is drifted: 375e9d411 (#638) hand-inserted three lines
into the generated CHANGELOG.md instead of running util/changelog_md.py. python util/changelog_md.py --check exits 1 on main and on this branch, and the two
changelog files here are byte-identical to main's — so the failure is inherited,
not introduced. The repair is the first commit of #657.