fix(corekit): keep SeekableReader's position bookkeeping in step (#643, #644) - #663
Conversation
Differential evidence for the thing #633's first revision got wrongThe PR body claims the fix does not turn a loud failure into a silent one. That is the specific way #633's first revision failed review, so it is worth measuring rather than asserting. Every 2090 cells: 11 setup states × 19 operations × 5 buffer sizes × Zero loud-to-quiet. No input on which base raised now silently returns. That is the property that matters, and it is a measurement rather than a reading of the diff. The 175 quiet-to-loud cells, each judged against the dataA new refusal is only defensible if base's answer was wrong. Classified by recomputing
And the 115 cells where both return a valueSame judgement applied: Examples of the 70, showing the NUL substitution #644 describes and the cursor drift #643 describes: The one thing to discloseThe 5 Scripts are scratch files under |
30adbc5 to
66a3c1b
Compare
Cross-review verdictNEEDS CHANGESReviewed by an independent subagent on a different model (Sonnet) from the one that wrote the change (Opus), briefed to falsify each load-bearing claim rather than confirm it, and to attack the It found one real defect. Recorded here rather than folded away, then fixed in The finding
Reproduced independently before acting on it: It is correct and it is my error. Neither issue asked for a zero-length read to start failing, and a request for no octets genuinely needs none, so the window has nothing to say about it. My own differential grid missed it because it included The fix, in
|
#644) - seek assigned _tell before validating it, so a refused seek left the position at the rejected offset and the next read returned the wrong octet. The target is now validated before it is committed. - SEEK_CUR and SEEK_END never checked the resulting position, so a negative absolute _tell was reachable. The check now applies under every whence, and with buffer_save=True, where the old refusal was skipped entirely and seek returned the negative position as though it had worked. - peek read through self._buffer and advanced its cursor while leaving _tell alone, so the next buffered read started from the wrong octet. It now reads through the view and leaves the cursor where it found it. - readline, read1 and peek capped size with min(size, _buffer_cur - 1): a count less one, measured from the window's base rather than the position read from, which handed back NUL padding at a plausible length and so suppressed the top-up from the stream. All three now derive the count from the position, as read already did. - A position the window has dropped raises rather than being answered from the buffer's stale cursor -- except for a zero-length read, which wants no octets and so is answered without consulting the window at all. tests/corekit/test_io.py: 31 passed, 35 subtests, 100% coverage of the module. 3000 random operation sequences: 6729 inconsistent states and 446 undocumented exceptions before, zero of each after.
66a3c1b to
a38fd2c
Compare
Cross-review, second passGOOD TO GOSame reviewer, same model (Sonnet, different from the authoring Opus), resumed with its own context rather than restarted, and asked to verify the fix for the finding it raised — not to re-bless the change. Its verdict, verbatim at the top of its report: What it checked, with its own measurements:
Two notes on provenance, so the verdict is not overstatedThe reviewer verified The comment documents the very trade-off its item 3 endorses. I had tried One further probe of mine, after its report, on the path it suggested and I had not tested: |
Cross-review, third pass — GOOD TO GO confirmed at the actual final headSame reviewer (Sonnet, vs authoring Opus), asked to re-verify because the head moved again after its second pass ( GOOD TO GO, independently re-derived rather than assumed from the prior pass:
No divergence found beyond the |
1 participant
Fixes #643
Fixes #644
One PR, not two. The two issues are independently diagnosable but not independently patchable:
peek's buffered read is one line, and both fixes rewrite it — #643 because reading throughself._bufferadvances a cursor a preview must not move, #644 because the cap on that read is wrong. Splitting them would put two PRs on the same line. The joint reproductions each issue records also only become correct once both halves land, so the tests for them could not be written in the first PR of a pair. The two defects are kept separable inside the change instead: distinct commit-message bullets, distinct test methods, andFixesfor each.What was wrong
#643 — position bookkeeping is not maintained across operations. Four faces, one root cause: nothing kept the three position variables in step across an operation that did not complete normally, and
seek's resync was the only thing that ever restored them.seekleft_tellalready moved. Eachwhencebranch assigned_tellbefore anything validated the result, so a refusal left the position at the rejected target with the resync skipped. A caller that catchesSeekErrorand reasonably believes the position unchanged read from the rejected offset, silently.SEEK_CUR/SEEK_ENDnever checked the resulting position was non-negative. OnlySEEK_SETlooked at its offset, sotell()of-100and-96were reachable — and the message conflated a position that cannot exist with one that has merely slid out of the window, which is ordinary and recoverable withbuffer_save=True.peekdesynced the buffer's cursor from_tell. It correctly never touched_tell, but its buffered branch read throughself._buffer, advancing thatBytesIO's cursor with nothing to put it back.tell()then reported the same number in both a peeked and an unpeeked run while the bytes differed, so a caller had no way to detect it.buffer_save=Truethe refusal was skipped entirely. The window refusal is conditional on there being no buffer file, and a negative position is not that case:seek(-5, io.SEEK_CUR)returned-5, and the failure surfaced later as a bareOSError: [Errno 22]from insideread.#644 — the
- 1off-by-one.min(size, self._buffer_cur - 1)is wrong twice:_buffer_curis measured from the window's base rather than from the position being read from, and the- 1is short of even that. The wrong-origin half is the worse one — the buffer is allocated full of NUL padding, so over-asking returns real octets followed by padding at the correct length, which makessize_remzero and skips the top-up from the stream, so the real data is never fetched at all.Two corrections to the issues, both measured on
0c7f2b7c9:read's cap was already corrected byda381f259(fix(corekit): keep SeekableReader's buffer content when truncating it (#622) #633's landing).readline,read1andpeekare the three that were still wrong.read's cases are kept in the tests as regression guards and are the subtests that pass on base.Extractor. It does not reproduce on currentmain— see below.The invariant, and how it is preserved by construction
The buffer is a sliding window whose octet 0 sits at absolute
_buffer_set, and_buffer_set + _buffer_curis whatseekreads as stream consumption. #633's first revision broke exactly that by clamping_buffer_curalone, turning a loudValueErrorinto silent corruption.The fix does not adjust either member to make a read fit. It adds
_seek_buffer(), which treats the buffer's own cursor as a derived quantity —_tell - _buffer_set, recomputed at each point of use — and returns the count genuinely available from the position,_buffer_cur - (_tell - _buffer_set). So:_buffer_setor_buffer_cur; the pair is only ever written by_write_bufferandtruncate, both unchanged here. It therefore cannot drift from what was actually consumed.seekvalidates its target into a local and commits_tellonly after both refusals, so a refused seek mutates nothing at all — face 1 and 2 are fixed by not writing, which is the strongest form of the guarantee.Demonstrated, not asserted.
test_the_window_base_and_content_pointer_track_the_stream_consumptiondrives eight operations against a stream that counts what has actually been taken off it, and checks_buffer_set + _buffer_cur == stream.consumedafter every one — plus the bounds — rather than only at the end. An output assertion can pass while the bookkeeping behind it is already wrong, which is how #633's first revision reached review.Evidence
Every face has a test that fails on base and passes here. Base run, with the library file swapped to
origin/main's and the new tests kept:The three passing are the deliberate regression guards:
readfor both halves of #644, andSEEK_SETfor the negative check, which base already got right. A fourth test added later,test_a_zero_length_read_is_answered_without_consulting_the_window, also passes on base by design -- it guards behaviour base already had and this PR's first revision broke, so it is a guard against the fix rather than a demonstration of a defect. The assertion diffs, verbatim:This branch:
Coverage of the module,
coverage run -m pytest(nopytest-cov), goes from 100% of 215 statements to 100% of 235 (88 branches, 0 missed, 0 partial) — the one line that would have been left uncovered was the old refusal inseek, now dead because the new guard turns that position away before_tellis committed, so it is removed rather than left unreachable.mypyandpylinton the file report exactly what they reported on base: one[override]note on therawproperty at:85and oneconsider-using-withat:115, both in untouched code.Inputs discriminate.
b'\x00'is exactly the buffer's NUL padding, sobytes(range(50))— which the issues use — cannot tell a padded answer from a real one; the tests usebytes(range(1, 51))andb'abcde'/b'0123456789XXX…'instead. A head-padded and a tail-padded implementation differ observably on these: growingb'abcd'to 8 givesb'abcd\x00\x00\x00\x00'one way andb'\x00\x00\x00\x00abcd'the other, and the wrong-origin cases additionally assertb'\x00' not inthe result, which no padded answer can satisfy.Fuzzing
3000 pseudo-random operation sequences × 10 operations, over
read/read1/readline/peek/seek(all threewhence)/truncate, across ten buffer sizes withbuffer_saveand stream-peekavailability both varied, checking the consumption invariant, the bounds, data correctness against the absolute offset read from, and position-stability under refusal:Zero on four further seeds as well (150,000 operations total).
The
Extractormagic-number claim: it does not reproduceBoth issues assert that
peek(4)atpcapkit/foundation/extraction.py:991corrupts the PCAP magic number. Measured against the realExtractor, drivingexamples/captures/in.pcapthrough a genuineos.pipe()so theSeekableReaderwrapper is actually constructed: it does not._magiccame backb'\xd4\xc3\xb2\xa1'in every configuration, on both trees, at every buffer size tried — and atio.DEFAULT_BUFFER_SIZE, the default, the extraction is byte-identical to the same file parsed through an ordinary seekableopen().The reason the issues saw it is that they were measured on
375e9d411, beforeda381f259(#633) removed the- 1fromread. That- 1was the mechanism that dropped the\xa1: on375e9d411the cap gavemin(16, 3) = 3, so the buffered part returned three octets and the 13-octet top-up came from a stream already at offset 4. Running the issues' own synthetic reproduction againstorigin/mainnow returns the correctb'\xd4\xc3\xb2\xa1RESTOFHEADER'. The claim was true when written and is no longer reachable.A narrower failure does exist nearby, and is worth recording rather than repeating as the same thing: with
buffer_size < 4and a raw stream offering neitherpeeknorread1,peek(4)reads more than the window can hold, so_write_bufferadvances_buffer_setpast a_tellthat a preview must not move. Base fails that loudly withProtocolError: invalid magic numberfrom the shifted header; this branch fails it loudly withSeekError: cannot read before the beginning of the buffer: 0 < 2. Loud either way, and the new message names the actual problem — but the underlying question of whetherpeekshould consume more than it can retain is a design question this PR does not answer.Deliberately not changed
truncateshould raise givenwritable()isFalseis an API break and the owner's call. The misspelledwriteableis left alone for the same reason — it belongs with that decision.docs/changelog/1.5.0.rstin docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657.peek's andread1's# pragma: no covertop-up blocks are untouched.