Skip to content

fix(foundation): give no_eof a way to stop, so extract() returns (#620) - #639

Merged
JarryShaw merged 1 commit into
mainfrom
fix/620-no-eof-termination
Sep 23, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/620-no-eof-termination

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 22, 2026

Copy link
Copy Markdown
Owner

Fixes #620

Revised after cross-review. Two claims in the first version of this description were falsified by an independent reviewer and are corrected below — the soundness argument was over-general, and the stated reason the in-process deadline fails was wrong. See the cross-review comment for the detail.

The defect

extract(..., no_eof=True) never returned. End of stream was detected correctly —
ExtractionWarning: EOF reached fired, as the issue notes — but all three loops
that handle it read the same thing:

except (EOFError, StopIteration):
    warn('EOF reached', ExtractionWarning, stacklevel=stacklevel())

    if self._flag_n:
        continue        # ... and nothing else stops the loop

no_eof suppressed the error that ended the loop without supplying any other
ending. The issue reproduced two of the three; __call__ carries its own copy, so
that is covered here too. A grep for _flag_n confirms there is no fourth site.

Real line numbers on 6c3d1b0d9, the issue's having shifted: record_frames at
extraction.py:698-708, __next__ at 1017-1030, __call__ at 1041-1054.

Re-verified on main

From an immutable git archive snapshot (extraction.py sha256 ebe7967079ca…,
unchanged afterwards), CPython 3.14.7:

control: no_eof absent -> RETURNED: frames = 6                  exit 0
subject: no_eof=True   -> Timeout (0:00:10)!                    exit 1
  File "/tmp/base610/pcapkit/protocols/protocol.py", line ??? in __init__

Where the termination condition comes from

The issue asks what "no EOF" should mean when the stream really is exhausted, and
says outright that its own mechanism is inference rather than measurement. So this
was traced.

End of stream originates in one place — pcapkit/utilities/decorators.py:270,
raise StreamEOFError('prepare: end of stream', quiet=True) — reached when
prepare measures the bytes remaining and gets zero. It restores the position
before raising, so the position at end of stream is stable:

frames read before first EOF = 6
  EOF #1: tell()=605  StreamEOFError      ... EOF #6: tell()=605  StreamEOFError
distinct positions across 6 EOFs = 1 -> CONSTANT
position == file size ? True

Two consecutive ends of stream at the same position therefore mean nothing arrived
between them. Extractor._note_eof_progress is that check, and each handler now
reads if self._flag_n and self._note_eof_progress(): continue.

What is safe, and what this narrows

A pipe is unaffected. pcapkit/__main__.py sets no_eof = args.fin == '-', so
pcapkit - reads a live capture off stdin — and a read on a pipe whose writer is
open but idle blocks; it does not report end of stream. Measured, a pipe fed in
two halves with a 1.5s pause and then closed:

[read] frame 1 at t=0.00s
[read] frame 2 at t=1.50s     <- the read BLOCKED across the pause
[writer] closed the write end
[read] frames 3..6 at t=2.01s
[eof ] #1..#8 at t=2.01s tell()=605   <- and then span forever on main

So a paused pipe never reaches the check at all, and the last two lines are a
second bug the issue does not mention: pcapkit - hung on any finished stdin.
Verified through the real CLI — cat examples/captures/in.pcap | python -m pcapkit -
goes from killed at 25s to exit 0 with a byte-identical 19,641-byte dump.

A seekable input is narrowed, deliberately. This is the correction: a regular
file does not block, so its two probes fall microseconds apart and a file still
being appended to now ends at the data present when the extraction reached it.
Measured both ways, appending on a record boundary 0.6s in:

tree frames
6c3d1b0d9 1, 2, 3, 4, 5, 6 — the sixth at t=0.61s
this branch 1, 2, 3, 4, 5

That is a behaviour change and it is chosen rather than overlooked. The previous
behaviour was unbounded by construction — it is the defect #620 reports — so some
stopping rule had to exist. A timed grace period was considered and rejected: it
would make the cut-off intermittent rather than absent, reintroducing the hang
for a slow writer while being harder to reason about than a crisp rule. Following a
growing file wants a policy of its own — a grace, a poll interval, an explicit
follow mode — which is a feature rather than part of a hang fix. The decision is
pinned by NoEOFOverAGrowingFileStopsAtWhatIsThere, which also fails on the
baseline by hanging.

Bounding how much that costs: an append landing mid-record raises
ValueError: read length must be non-negative or -1 on both trees, so no_eof
over a growing file only ever worked for a writer flushing whole records.
Pre-existing, and not addressed here.

__call__ still raises EOFError, deliberately

__call__ must return a frame or raise, and once the input is finished there is no
frame — so EOFError is the only truthful answer. That matches the plain form,
which tests/integration/test_frame_iteration.py:79 pins on an extractor built
without no_eof, so nothing in the suite contradicts the new behaviour.

Why the tests use a child process, not the suite's own deadline

This is the second correction. The first version claimed tests/_support.time_limit
fails because the parse path's two except Exception handlers absorb the
SIGALRM-raised TimeoutError. That is wrong, and was falsified by tapping the
logger hierarchy across the retries: zero records from
pcapkit/utilities/decorators.py or pcapkit/protocols/protocol.py, with a
positive control proving the tap worked. prepare raises end of stream before
any next-layer decode, so neither handler is entered.

What is actually true is narrower and still decisive: the in-process deadline is
delivered intermittently here. It usually expires on time, and once escaped
entirely, running past ten minutes and reaching 13.2 GB RSS before I killed it.
The escape has not reproduced. The memory is the retry loop emitting tens of
thousands of EOF reached records a second, which the runner retains — not handler
logging, of which there is none.

An intermittent guard against a hang is worse than no guard, because the run it
misses is a wedged suite rather than a red test. So the end-to-end cases are
bounded from outside the interpreter: a child process with a subprocess
wall-clock timeout, which cannot be absorbed or missed, plus an RLIMIT_AS cap of
2 GiB so a child that does hang cannot hurt the host while it waits to be killed.
The child also asserts which tree it imported — this project is normally installed
editable, and the editable finder will happily serve a different checkout. The
cases driven by a stand-in engine keep time_limit, which is sound there because
no parse path is involved.

Evidence

Both trees with pcapkit.__file__ printed into the pytest header.

Baseline 6c3d1b0d9exit code 1, read from a file, bounded, 3:01, no wedge:

treeprobe: extraction.__file__ = /tmp/base620/pcapkit/foundation/extraction.py
12 failed, 1 passed, 1 warning in 181.26s (0:03:01)
E   AssertionError: the extraction did not return within 30s, which is the
    non-termination #620 reports. Child output so far: ...
      ... x5: the three loops, the stdin case, and the growing-file case
/tmp/base620/tests/_support.py:72: TimeoutError: did not finish within 30s
E   AttributeError: 'Extractor' object has no attribute '_note_eof_progress'

Being honest about the 12: 6 are behavioural — five child-bounded
non-terminations plus the TimeoutError on the retry-while-growing case — and
6 are merely _note_eof_progress not existing yet, which proves nothing about
behaviour. The 6 are the regression test.

This branch — exit code 0:

treeprobe: extraction.__file__ = .../pcapkit/foundation/extraction.py
tests/foundation/test_extraction_no_eof.py tests/foundation/test_extraction.py
tests/project/ tests/cli/ tests/integration/test_frame_iteration.py
137 passed, 227 warnings in 18.06s

The retry direction is not merely asserted, it is mutation-tested by the
cross-review: _note_eof_progress → return False (the flag silently a no-op) fails
6 tests; → return True (the original bug) fails 9.

Coverage cannot show the change: if self._flag_n: already executed, and it is the
branch that moved. The evidence axis is the case count — 11 cases where there were
none, 6 of which fail behaviourally without the fix.

Notes for the reviewer

  • tests/foundation/test_extraction.py's _bare_extractor gains
    extractor._eof_mark = None. The EOF handlers consult it, so an instance built by
    hand and driven through them has to carry it.
  • tests/project/test_documentation_claims.py's docstring described the condition as
    if self._flag_n: continue, which is no longer what ships, in a file whose purpose
    is stopping documentation from stating untrue things. Corrected — and note its AST
    pin passes both before and after this change, so it pins the documented sense of
    the flag rather than that the loop ends.
  • _note_eof_progress records the mark as a side effect, so it is not idempotent
    — a second call for one end of stream spends the retry. The name says so and the
    docstring warns; the three call sites each call it once.
  • Rebased onto cfb81d3f6; one commit, 0 behind.

@JarryShaw

Copy link
Copy Markdown
Owner Author

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

#636 (input-stream ownership, #610) is independent of this one in substance: different function, 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._eof_progressed, #636 adds Extractor._owns_input. 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 — this PR adds _eof_mark to _bare_extractor, #636 changes an assertion some 350 lines away. 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: this PR makes a no_eof extraction terminate, so it starts reaching _cleanup where it previously never did at all. On main today _cleanup has its stream ownership inverted, which is what #636 fixes — so with this PR alone, a terminating no_eof extraction that was given a path still leaks its handle, and one given a stream gets the caller's stream closed. That is #610's defect rather than this one's, and it is reached more often once this lands. Neither PR blocks the other, but #636 is the one worth landing first.

@JarryShaw
JarryShaw force-pushed the fix/620-no-eof-termination branch 2 times, most recently from fe83ed0 to cf0d404 Compare September 22, 2026 06:19
@JarryShaw

Copy link
Copy Markdown
Owner Author

Amended to fe83ed058. Test-helper only, no library change and no change to what any test asserts.

ChildBoundedTestCase.run_bounded read the partial child output as (exc.output or b"").decode(...). I measured what subprocess.TimeoutExpired.output actually is on this interpreter, expecting str under text=True:

exc.output type   = bytes
exc.output repr   = b'PARTIAL\n'

So bytes — the .decode() was correct on CPython 3.14.7, and the baseline run did exercise that path. But it is an implementation detail rather than a documented guarantee, and this suite runs on 3.10 through 3.15, where a str would turn the informative "the extraction did not return within 30s" failure into an AttributeError that says nothing about #620. It now accepts either.

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

@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review: NEEDS CHANGES — now addressed in cf0d40461

Independent cross-review, as this PR was raised by an agent. Run on Haiku 4.5 — a different model from the one that wrote the change (Opus 5) — briefed to falsify rather than confirm, read-only. It returned NEEDS CHANGES with three required changes, and it was right about all three. I verified its central finding myself before accepting it, rather than taking the report on trust.

Provenance: base 6c3d1b0d9, extraction.py sha256 ebe7967079ca…; the reviewed head was 2f1ec9df5, sha256 7f5c57f1ce72…. Every measurement it made printed and asserted pcapkit.__file__, from immutable git archive snapshots.

Verdicts

Claim Verdict
C1 reproduction real on main CONFIRMED
C2 exactly three affected loops, all three fixed CONFIRMED
C3 termination signal sound derivation CONFIRMED; soundness claim REFUTED for seekable inputs
C4 no_eof not reduced to a no-op CONFIRMED (mutation-tested both directions)
C5 live-pipe measurement measurement CONFIRMED to the centisecond; conclusion REFUTED as generalised
C6 except Exception absorption outcome partly confirmed; mechanism REFUTED
C7 tests fail on baseline, 5 behavioural / 6 API-absence CONFIRMED, split exact
C8 nothing regressed CONFIRMED, exit 0
C9 EOFError from __call__ defensible CONFIRMED as defensible

Worth noting on C4: it did not take the retry test on trust, it mutation-tested it. _note_eof_progress → return False (the flag silently a no-op) produced 6 failures; → return True (the original bug) produced 9. Both directions are genuinely pinned.

Required change 1 — a real regression, which I reproduced myself

The soundness argument was made on a pipe, where a blocking read masks the question, and I generalised it to all inputs. A seekable regular file does not block: it reports end of stream at once, so the two probes fall microseconds apart and any pause in the writer ends the extraction.

I re-ran this independently rather than accepting the report, driving through __next__, appending on a record boundary 0.6s in:

BASE 6c3d1b0d9  [read] frames 1-5 at t<=0.01s, frame 6 at t=0.61s   VERDICT ALL-SIX
HEAD            [read] frames 1-5 at t<=0.01s, StopIteration        VERDICT CUT-SHORT-AT-5

Confirmed. And the old _flag_n comment named this case verbatim — "useful when the input file is a live capture".

The reviewer offered two remedies: a retry budget/backoff, or narrow the claim. I took the second, deliberately, because the first is worse: the previous behaviour was unbounded by construction — that is the defect — so some stopping rule had to be chosen, and a timed grace period would make the cut-off intermittent rather than absent, which is harder to reason about than a crisp rule and would reintroduce a hang for a slow writer. Following a growing file wants a policy of its own (grace, poll interval, or an explicit follow mode), and that is a feature rather than part of a hang fix.

So: the claim is narrowed in all five places it appeared (both no_eof docstrings, _note_eof_progress, the _flag_n declaration, and both changelogs), and there is now a test — NoEOFOverAGrowingFileStopsAtWhatIsThere — pinning the decision so it stays a decision rather than becoming folklore. It fails on the baseline too, by hanging, so it doubles as another reproduction.

One thing that bounds how much this costs, which the reviewer also found and I verified on both trees: an append landing mid-record raises ValueError: read length must be non-negative or -1 on 6c3d1b0d9 and here. So no_eof over a growing file only ever worked for a writer flushing whole records. Pre-existing, untouched, now recorded.

Required change 2 — my stated mechanism was not supported by measurement

I claimed the in-process deadline fails because the parse path's two except Exception handlers absorb the SIGALRM-raised TimeoutError. The reviewer falsified that properly:

  • It tapped the logger hierarchy across the retries and saw zero records from pcapkit.utilities.decorators or pcapkit.protocols.protocol — with a positive control proving the tap worked. Reading the code confirms why: prepare raises end of stream before any next-layer decode, so neither handler is reached.
  • time_limit mostly held — 5/5 standalone, 4/4 across four isolating conditions, and 3/3 on a pytest re-run. The escape I saw is real but not reproducible, and it was specific to the __call__ form; the other two forms were bounded by time_limit in the same run, which directly contradicts "it does not hold for a case that parses a real capture".
  • The 13.2 GB is better explained by the retry loop emitting tens of thousands of EOF reached records a second and the runner retaining them — its own log held 463,770 of them — not by handler logging, of which there was none.

Corrected in the test module docstring, both changelogs and the commit message, with the two wrong guesses recorded as ruled out so nobody makes them again. The engineering conclusion is unchanged and, as the reviewer notes, stronger than I argued it: an intermittently delivered in-process deadline against a hang is worse than none, because the run it misses is a wedged suite rather than a red test.

Required change 3 — done

tests/project/test_documentation_claims.py::test_documented_sense_matches_the_code's docstring still described the condition as if self._flag_n: continue, which is no longer what ships — in a file whose entire purpose is stopping documentation from stating untrue things. Corrected, and while there I recorded the reviewer's sharper observation: that AST pin no longer distinguishes the defect from the fix (it passed before and after), so it pins the documented sense of the flag, not that the loop ends. The ending is pinned by tests/foundation/test_extraction_no_eof.py.

Also, its _no_eof_line compares only the single line beginning no_eof:, so "pinned to agree" in my PR body overstated it — the continuation text is not covered by that check, though the reviewer verified it is in fact identical.

The other findings, acted on

  • _eof_progressed was a predicate with a hidden write — a later logger.debug(f'{self._eof_progressed()}') would silently consume a retry. Renamed to _note_eof_progress, so the name states the mutation, with an explicit Warning: that it is not idempotent.
  • 3 commits behind origin/main — both this branch and fix(foundation): close the input stream pcapkit opened, not the caller's (#610) #636 are rebased onto cfb81d3f6 and are each exactly 1 commit ahead, 0 behind.
  • pcapkit - verified through the real CLI, which the reviewer did and I had not: cat examples/captures/in.pcap | python -m pcapkit - goes from killed at 25s to exit 0 with a byte-identical 19,641-byte dump. Added to the changelog.
  • Missing full stops on the two no_eof paragraphs — left as they are, deliberately: no Args: entry in either file ends with one, so adding them would be the inconsistency.

Figures after the corrections

HEAD cf0d40461 : tests/foundation/test_extraction_no_eof.py tests/foundation/test_extraction.py
                 tests/project/ tests/cli/ tests/integration/test_frame_iteration.py
                 137 passed, 227 warnings in 18.06s                          exit 0

BASE 6c3d1b0d9 : tests/foundation/test_extraction_no_eof.py (bounded)
                 12 failed, 1 passed, 1 warning in 181.26s (0:03:01)          exit 1

Now 6 behavioural failures on the baseline (five child-bounded non-terminations, plus the TimeoutError on the retry-while-growing case) against 6 API-absence, up from 5 — the growing-file test is the new behavioural one.

Left for a follow-up rather than fixed here

  • Following a growing file, which needs a deliberate policy as above.
  • The mid-record ValueError: read length must be non-negative or -1, pre-existing on both trees and not in this PR's file.
  • The order-dependent TypeError: type 'ProtocolBase' is not subscriptable failures in tests/project/. Note this reviewer did not see them, in either plugin configuration, where my own runs and fix(foundation): close the input stream pcapkit opened, not the caller's (#610) #636's reviewer did — so the trigger is narrower than "runs after another directory" and it wants its own investigation.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Rewritten: changelog entry moved out, rebased onto current main

This branch was force-pushed. Head is now 6e15a98a1, 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, pcapkit/interface/core.py and
three test files — 5 files, down from 7.

One real conflict remains, and it is not the changelog

This PR and #636 both insert a private helper between the # Utilities. header and
def _cleanup in pcapkit/foundation/extraction.py_note_eof_progress here,
_owns_input 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 #636 before this one. This fix makes no_eof reach _cleanup where it
previously never did, so it depends on #610's ownership rule being right. Expect to
resolve that one hunk keep-both here. 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.

JarryShaw added a commit that referenced this pull request Sep 22, 2026
The bullet #639 originally carried, moved here verbatim so that #639 touches only
`pcapkit/foundation/extraction.py`, `pcapkit/interface/core.py` and its three test
files.

Covers: `extract(..., no_eof=True)` never returning, and the progress check that
now ends it -- including the deliberate narrowing for a seekable input still being
appended to.

43 lines added to the entry file; `CHANGELOG.md` regenerated, not edited.
@JarryShaw
JarryShaw force-pushed the fix/620-no-eof-termination branch 2 times, most recently from 07270e8 to c1c8ab6 Compare September 22, 2026 21:17
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review of c1c8ab6e5: GOOD TO GO

This supersedes the previous cross-review verdict, which read "NEEDS CHANGES — now
addressed in cf0d40461"
. cf0d40461 no longer exists on this branch — the PR was
rebased onto a moved main (onto c37d80c85, the commit that merged #636) and the
conflict was resolved as c1c8ab6e5, one commit, 5 files changed, 638 insertions(+), 12 deletions(-). This review is of c1c8ab6e5 from scratch; nothing about the old
verdict should be read as still applying.

Run on a different model from whichever authored the change, read-only, briefed to
falsify rather than confirm. Every number below is from a command I ran myself in a
scratch worktree at /tmp (since removed), never from the PR description.

Tree provenance

pcapkit.__file__ = /tmp/pr639-review-scratch/wt/pcapkit/__init__.py   (asserted under that tree before any other import)

The rebase conflict resolution (mine to check, not the author's)

  • Both methods present, syntactically valid, callable on Extractor — confirmed via
    inspect.signature: _owns_input(self) -> 'bool' and _note_eof_progress(self) -> 'bool', both bound and callable.
  • Nothing dropped from either side. git diff 6e15a98a1 c1c8ab6e5 -- pcapkit/foundation/extraction.py (the pre-second-rebase PR head vs. the current head)
    shows the diff is purely additive — it layers in _owns_input, __del__, and the
    stream_closing polarity fix from fix(foundation): close the input stream pcapkit opened, not the caller's (#610) #636 on top of an unchanged _note_eof_progress and
    unchanged call sites. Confirmed independently against fix(foundation): close the input stream pcapkit opened, not the caller's (#610) #636's own contribution on main
    as the other parent.
  • _eof_mark initialization is safe on a partially-constructed instance, but not for
    the reason the brief worried about. _eof_mark = None is set very early in __init__
    (immediately after _flag_n), well before _ifile is ever assigned. _note_eof_progress
    is only reachable from record_frames / __next__ / __call__ — and record_frames
    is invoked unconditionally as self.run(), the last statement in __init__. So by
    construction, _note_eof_progress can never run before _eof_mark exists. The partial-
    construction hazard _owns_input guards against (via __dict__.get) is specific to
    __del__, which _note_eof_progress is never called from. No interference between the
    two methods.
  • One real, minor defect found in the resolution itself: there is no blank line
    between _owns_input's last line and def _note_eof_progress — a splice artifact of
    the "keep both" merge. pycodestyle --select=E301 flags it:
    extraction.py:1182:5: E301 expected 1 blank line, found 0. This repo's CI does not
    run flake8/pycodestyle (.github/workflows/lint.yml runs bandit/mypy/vermin/pylint,
    all advisory), so it's not a merge blocker, but it's real and worth a one-line fix.
  • fix(foundation): close the input stream pcapkit opened, not the caller's (#610) #636's own ownership suite passes on this branch, exit code read from file, not
    inferred from the summary line:
    tests/foundation/test_extraction_ownership.py: 10 passed, 5 warnings, 11 subtests passed in 0.75s
    EXIT:0
    
    No SUBFAIL anywhere in the log.
  • I also checked the 8 commits main has gained since the PR's rebase point
    (c37d80c85..main, none in this PR): b34f132f6 (SeekableReader position-bookkeeping
    fixes) and c22429872 (protocol construction-keyword guard) are the only ones touching
    anything nearby. b34f132f6's changes are bug fixes to seek/tell/peek edge cases
    (invalid seeks, negative positions, buffer-window math) — they make tell() more
    consistent, not less, and don't change the monotonic-position contract
    _note_eof_progress relies on. Not a concern for this PR, which doesn't touch
    corekit/io.py at all.

Call-site count and idempotency

Exactly 3 call sites, via grep -n "_note_eof_progress()": record_frames:717,
__next__:1051, __call__:1079. Each is inside a distinct, mutually exclusive
except (EOFError, StopIteration) handler, and each calls it exactly once via
short-circuit if self._flag_n and self._note_eof_progress():. No fourth site, no
double-call on any path.

Termination rule — mutation-tested independently, not accepted on trust

Ran the fast unit classes (EOFProgressSignal, NoEOFStillRetriesAnInputThatGrows)
against three hand-written mutants of _note_eof_progress:

mutant result
baseline (unmodified) 6/6 pass
always return False (flag silently a no-op) 5/6 fail
always return True (the original #620 bug) 6/6 fail + 1 error
position >= previous (off-by-one) 2/6 fail + 1 error (mock exhausted retrying past the expected stop point)

The off-by-one variant the brief specifically asked about is caught. except (OSError, ValueError): return False — confirmed via the existing suite's closed stream /
unseekable subtests, and independently: a real os.pipe() wrapped directly in
io.BufferedReader (not yet wrapped in SeekableReader) raises OSError: [Errno 29] Illegal seek on .tell(). In the real Extractor.__init__ flow this raw reader gets
wrapped in SeekableReader before _note_eof_progress ever sees it, and my own pipe
test (below) confirms SeekableReader.tell() does not raise for a live pipe — so
this except clause is a defensive backstop (a seekable stream closed mid-extraction),
not something a live pipe normally hits. False is the safe answer either way: it
errs toward stopping rather than reintroducing the unbounded loop #620 reports.

The pipe-safety claim — independently reproduced with a real pipe, not accepted from the description

Built a real os.pipe() with a background thread that writes 5 frames, sleeps 1.2s,
then writes the rest and closes. Ran the actual Extractor over it with no_eof=True,
instrumented _note_eof_progress itself:

frames: [(1, 0.0), (2, 1.2), (3, 1.21), (4, 1.21), (5, 1.21), (6, 1.21)]
_note_eof_progress call log (time, returned): [(1.214, True), (1.214, False)]

_note_eof_progress was called exactly twice, both after the pause ended — never
during it. That's a direct confirmation that the blocking read absorbs the entire pause
before EOF-detection is ever reached, exactly as the docstring claims. All 6 frames came
back despite the 1.2s pause.

One gap this surfaced: the checked-in suite doesn't have this test.
test_a_pipe_whose_writer_closed_terminates only covers a non-seekable stream that's
already fully written (terminates-on-exhaustion), not a real pause-then-resume with
timing. I could verify the claim myself, but nothing in CI would catch a future
regression to this specific behavior. Worth a follow-up test, not a blocker — the
growing-file narrowing (the actual behavior change) is pinned, with real threading and
a real 0.6s sleep, and it passed in my run.

Test suite run against this branch, exit codes read from file

tests/foundation/test_extraction_no_eof.py tests/foundation/test_extraction.py tests/project/
119 passed, 7 warnings, 499 subtests passed in 17.49s
EXIT:0

No SUBFAIL under any PASSED line in either run.

Test discrimination

tests/project/test_documentation_claims.py::test_documented_sense_matches_the_code
honestly can't and doesn't claim to distinguish the fix from the #620 bug — its own
AST walk looks for if on _flag_n with a continue in the body, which the
pre-fix if self._flag_n: continue also satisfies. It pins the documented sense of
the flag, exactly as its docstring says, and nothing more.

EOFProgressSignal and NoEOFStillRetriesAnInputThatGrows do the real discrimination,
per the mutation table above.

Not independently re-verified

  • The exact frame-count numbers in the PR body's own measurements (6 vs. 5 on
    6c3d1b0d9 for the growing-file case) — I didn't check out the pre-fix tree myself;
    the current-tree behavior is verified above and the test passes.
  • Behavior on Python versions other than 3.14.7 (only interpreter with dependencies
    installed here). Nothing in the diff looks version-sensitive to me, but per the
    standing caution here, flagging rather than asserting it.
  • resource.RLIMIT_AS in the new test file's CHILD_PREAMBLE is POSIX-only
    (import resource doesn't exist on Windows). Harmless today — lint.yml and
    unit-tests.yml both run ubuntu-latest only — but worth knowing if the matrix ever
    grows a Windows job.

Bottom line

No functional defect found. Two minor, non-blocking items: the missing blank line
(E301) left by the rebase resolution, and the missing real-pipe-pause regression test.
Neither changes the verdict.

)

* All three loops that handle end of stream -- `record_frames`, `__next__` and
  `__call__` -- read `if self._flag_n: continue` with nothing else to stop them,
  so `no_eof=True` suppressed the error that ended the loop without supplying
  any other ending and an exhausted input was retried forever.
* Added `Extractor._note_eof_progress`, which compares the input's position
  against its position at the previous end of stream. `prepare` restores the
  position before raising, so two ends of stream at the same position mean
  nothing arrived between them. Each handler now retries only while the input is
  still producing.
* A pipe is unaffected: a read there blocks while the writer is open but idle
  rather than reporting end of stream, so a paused pipe never reaches the check.
* Deliberate narrowing, documented and pinned by a test: a *seekable* input does
  not block, so a file still being appended to now ends at the data present when
  the extraction reached it -- six frames before, five after. The old behaviour
  was unbounded by construction, which is the defect, so a stopping rule had to
  be chosen; a timed grace would make the cut-off intermittent rather than absent.
* `pcapkit -` hung on any finished stdin as well, which #620 does not mention.
* Both docstrings for `no_eof` now describe the retry, its end and its limits.
  They are pinned to agree by `tests/project/test_documentation_claims.py`, whose
  own stale description of the condition is corrected here too.

The end-to-end tests bound themselves with a child process rather than
`tests._support.time_limit`, which proved *intermittent* on this loop: usually
on time, once escaping entirely and reaching 13.2 GB RSS. Two guesses at the
cause are recorded as ruled out rather than asserted.

Measured on 6c3d1b0: 12 failed / 1 passed before, 11 passed after.
@JarryShaw
JarryShaw force-pushed the fix/620-no-eof-termination branch from c1c8ab6 to 49d9577 Compare September 22, 2026 23:07
@JarryShaw
JarryShaw merged commit 79494a0 into main Sep 23, 2026
26 checks passed
@JarryShaw
JarryShaw deleted the fix/620-no-eof-termination branch September 23, 2026 02:24
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

None yet

Development

Successfully merging this pull request may close these issues.

extract(no_eof=True) never returns: EOF is detected but nothing stops the loop

1 participant