fix: read the rollback position before the write retry loop#126
Merged
Conversation
`IncrementalAssocWriter._write_buf` read its rollback position inside the
try-block it protects:
for attempt in ...:
try:
pos = self._file.tell()
self._file.write(buf)
...
except OSError as e:
self._file.seek(pos) # pos unbound if tell() was what failed
If `tell()` raised, `pos` was never assigned, so the rollback raised
`UnboundLocalError`. That is not an `OSError`, so it bypassed the retry, the
rollback, and `_cleanup_partial`, and escaped `write()`'s documented
`Raises: OSError` contract with a misleading message. Reproduced with an
ESPIPE from `tell()`:
UnboundLocalError: cannot access local variable 'pos' where it is not
associated with a value
`tell()` now runs once before the loop. The position is the same on every
attempt, since a failed write is truncated back to it, so nothing is lost by
not retrying it — and a failure there means nothing was written, so there is
no partial state to roll back. The context manager still deletes the partial
output on the way out.
The file handle is also bound to a local with an explicit open check, instead
of dereferencing `self._file` five times behind a guard that lives in three
separate callers. That clears the seven pyrefly baseline entries in this file
(107 -> 100), which were all this method reaching through the optional
attribute.
Regression test added first and confirmed failing against the old code:
`test_write_buf_tell_failure_surfaces_as_oserror`.
`pytest tests/` — 2242 passed, 10 skipped, 3 xfailed. All four LMM modes on
both backends still produce byte-identical .assoc.txt output.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A latent bug in
IncrementalAssocWriter._write_buf, found while clearing that file'spyrefly baseline entries. 107 → 100 entries, and the write path no longer has a failure
mode that bypasses its own error handling.
The bug
The rollback position was read inside the try-block it protects:
If
tell()raised — ESPIPE on a pipe or FIFO, a bad fd —poswas unbound, so the rollbackraised
UnboundLocalError. That is not anOSError, so it fell straight through thehandler that was supposed to deal with it:
seek/truncaterollback_cleanup_partial()write()'s documentedRaises: OSErrorcontractand the caller got this instead of the disk error:
Reproduced by making
tell()raiseOSError(ESPIPE).The fix
tell()runs once, before the loop. That is correct rather than merely convenient:nothing is lost by not re-reading it per attempt.
and letting the
OSErrorpropagate is the honest signal. The context manager stilldeletes the partial output on the way out (asserted in the test).
The handle is also bound to a local with an explicit open check, rather than dereferencing
self._filefive times behind a guard that lives in three separate callers. A private methodthat dereferences an optional attribute shouldn't depend on every caller remembering.
Baseline
That is what clears this file's 7 entries — all of them were this one method reaching
through the optional attribute. 107 → 100.
Verification
Regression test written first and confirmed failing against the old code
(
test_write_buf_tell_failure_surfaces_as_oserror), in the style the file already uses forits other injected-I/O-failure tests:
pytest tests/test_incremental_writer.pypytest tests/.assoc.txtruff check/ruff format --checkpyrefly check --baselineon 3.11.13/numpy 2.4.6 and 3.13.5/numpy 2.5.1Note for the next baseline pass
tests/test_incremental_writer.pyholds the single largest remaining cluster at 23entries — and they are all one root cause, not 23:
writer._fileisTextIO | None, andevery test that injects an I/O failure does
writer._file.write = .... One small helper thatreturns the narrowed handle collapses the whole cluster. The new test here already avoids
adding a 24th, so this PR grows the baseline by nothing.