fix: AppendSurface survives a torn write, and counts what it skips - #898
Merged
Conversation
A writer killed between its record and the terminator leaves a remnant line.
Three consequences, all measured before this change:
1. append() raised on the remnant and kept raising: the guarded append point
was BRICKED permanently, because every later writer re-reads the whole file
to compute the next sequence number and dies on the same line.
2. The next record GLUED onto the remnant, producing one unparseable line and
losing a record that had itself completed and fsynced.
3. records() raised, so every reader of the surface died too.
Both scans now skip a line they cannot read AND count it (malformed_lines,
reflecting the most recent full scan). Skipping alone is silent, and silence is
the defect: the remnant sits in the file while a caller sees a healthy-looking
surface with a contiguous sequence. append() also repairs a missing terminator
before writing, so a completed record is never swallowed by an incomplete one.
MalformedLine is reused from the state machine rather than re-declared, so the
two append-only surfaces in this tree report a torn line in one vocabulary.
Four witnesses, five mutations, each mutation killing witnesses whose failure
TYPE matches it: removing the terminator repair kills by AssertionError (glue is
wrong content, not an exception); removing either scan's guard kills by
JSONDecodeError and KeyError, exactly the types the guard catches; silencing
either count kills only that path's count assertions. Tear-fixture count for
this surface: 4, previously 0.
Ref #897 - closes at promotion
Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
r2 (Sentinel) — BLOCK, bound to head f051ab8. RAN: at the exact head, a valid JSON row with Scope: AppendSurface torn-line handling at the bound head. |
Two review blocks on this PR, both correct, and both about the SHAPE of the guard rather than a case it was missing. FIRST: an except tuple over untrusted file content is a DENYLIST, and a denylist leaks by construction. Valid JSON seq 1e999 parses to inf, int(inf) raises OverflowError, and no list written from the parse side would have predicted it, because the COERCION AFTER the parse is what invents the new failure. Asking what else escaped the same guard found a sibling: deeply nested JSON raises RecursionError from json.loads itself and bricks identically. So the coercion is gone. Everything after the parse validates types rather than converting them, and inf is refused because a float is not a sequence number, with no exception existing to catch. SECOND: the decode is a raise site BELOW the parse. The exception type was never the problem, since UnicodeDecodeError subclasses ValueError which was already caught; the OPERATION that raises it sat outside the guarded region. In text mode the decode happens while the iterator MANUFACTURES the line, so bytes are converted before any try block can see them. The fix guarded the transformation and left the ACQUISITION unguarded. Operationally that was worse than a brick: a whole-file decode means ONE bad byte anywhere destroys every record in the surface, including the thousands written correctly around it. The file is now read as bytes and decoded one line at a time, which is what a JSONL file actually is, so a bad byte costs its own line. A line passes through four layers - read, decode, parse, shape-check - and the last three are each guarded where they happen. Naming the enumeration rather than claiming completeness, because two earlier claims that the raise-site set was closed were each broken by the next reviewer: unbounded line length is a resource limit on the READ layer and is deliberately not defended, stated in the class docstring and the PR body rather than left to be discovered. Also pinned, found when a new witness exposed a wrong expectation in a fixture of mine rather than a defect in the code: a line can be unreadable as a RECORD while its sequence number is perfectly readable, and numbering honours it anyway. No writer is ever issued a number a reader can already see on disk. Seventeen hostile-content and undecodable-byte rows plus a confinement witness proving ten good records survive a bad byte written between them. Ten mutation rows, measured with an extractor that reads only E-prefixed traceback lines and is proven against a green control - an earlier extractor read exception names out of parametrized TEST IDS and reported types that were never raised. One row was discarded as contaminated, its kills coming from a sloppy edit rather than the defect, and re-run clean. Ref #897 Co-Authored-By: Claude <noreply@anthropic.com>
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.
What
AppendSurface(Prototype 2,gr2/prototypes/contribution_protocol.py) is the declared append-only surface: one guarded append point, exclusive lock across write + flush + fsync, arrival-ordered sequence numbers. A writer killed between its record and the terminator leaves a remnant line, and three things followed. All three were measured, not inferred:append()raised on the remnant and kept raising — the surface was BRICKED permanently. Every later writer re-reads the whole file to compute the next sequence number, so every later writer died on the same line. The failure sits on the WRITE path, behind the lock, and is not survivable by retrying.records()raised, so every reader died too.Both scans now skip a line they cannot read and count it (
malformed_lines, reflecting the most recent full scan). The count is the load-bearing half: skipping alone is silent, and the remnant otherwise sits in the file while a caller sees a healthy-looking surface with a contiguous sequence — a loss invisible by construction.append()also repairs a missing terminator before writing, so a completed record is never swallowed by an incomplete one.Two review blocks, and both times the guard was the wrong SHAPE, not missing a case
The range is two commits and is worth reading as one: the original fix, and one commit carrying both corrections. Each block moved the guard down a layer, and the second one is where it belongs.
Block 1 — the guard was a denylist. Valid JSON
seqof1e999parses toinf;int(inf)raisesOverflowError, which was not in the except tuple, so both scans still bricked permanently. Asking what else escaped the same guard found a sibling the review had not named: deeply nested JSON raisesRecursionErrorfromjson.loadsitself. Two escapes from one guard is not two bugs. An except tuple over untrusted content enumerates the failures imaginable from the parse side, while the coercion after the parse invents new ones —int()applied to a valuejsoncan legitimately produce raises a type no parse-side list predicts. So the coercion was removed: everything after the parse validates types rather than converting them, andinfis refused because a float is not a sequence number, with no exception existing to catch.Block 2 — the guard was in the wrong place. A raw invalid UTF-8 byte raised
UnicodeDecodeErrorin both scans before any JSON handling. The type was never the problem:UnicodeDecodeErrorsubclassesValueError, which was already caught. The operation that raises it sat outside the guarded region — in text mode the decode happens while the iterator manufactures the line, so the bytes are converted before anytrycan see them. The fix guarded the transformation and left the acquisition unguarded. A line does not arrive as text; it is manufactured as text, and manufacturing it is content-dependent.Operationally that was worse than a brick: a whole-file decode means one bad byte anywhere destroys every record in the surface, including the thousands written correctly around it. The file is now read as bytes and decoded one line at a time, which is what a JSONL file actually is, so a bad byte costs exactly its own line.
What the two blocks have in common is the useful part: both times the fix was sound about the case in front of it and wrong about the boundary it was defending. A line passes through four layers — read, decode, parse, shape-check — and each of the last three is now guarded where it happens.
Evidence (RAN)
seqas an object,seqas a bool — a bool is an int in Python — a non-string writer, a non-object payload, a non-string timestamp, a bare array, a bare string); six undecodable-byte rows (invalid0xff, lone surrogate, truncated multi-byte sequence, overlong encoding, orphan continuation byte, pure binary); and structural witnesses for the remnant surviving twice, the following record being a separate whole line, a fresh handle reporting the same finding, the numbering semantic below, and confinement — ten good records surviving a bad byte written between them.UnicodeDecodeError, each naming its own fixture's byte; reverting the read to a whole-file text decode kills the same seven;errors="replace"instead of refusing turns a bad line into a fake record and kills six; reverting to the denylist form kills exactly the two escapes by realOverflowErrorandRecursionError; droppingRecursionErrorkills only the nesting row; restoring the coercion kills five; accepting a bool kills only the bool row; dropping the writer type check kills only the writer row; silencing either counter kills only that path's count assertions.OverflowErrorandRecursionErrorfor runs where neither was raised. It agreed with what I expected, which is when a harness is least questioned. The corrected extractor reads onlyE-prefixed traceback lines and is proven against a green control that must yield zero. (b) One mutation row was discarded as contaminated — 15 of its 22 kills were anAttributeErrorfrom a sloppy edit rather than from the defect — and re-run clean, where it kills 7 for the right reason.test_contribution_protocol.py,test_propagation_contribution.py,test_propagation_state_machine.py,test_propagation_daemon.py— 127 passed, 1 xfailed, against a measured baseline of 104 passed, 1 xfailed on cleandevwith these files stashed. Delta is exactly the 23 new witnesses. Import resolution pinned from inside pytest (thegr2package is synthesized byconftest.py, so an out-of-pytest import proves nothing).ruff checkandruff format --checkclean on both files.Not in scope
Premium boundary: grip is OSS; this is local file mechanics over opaque identifiers and carries no identity, org, or policy content.
Ref #897