Repository navigation
harness/store: decode the JSONL tail window losslessly - #128
theamazinghenk wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesJSONL append handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change allows append-store tail windows to handle UTF-8 continuation bytes without losing records or freezing affected files. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Title checkExplanation The title clearly identifies the JSONL tail-window decoding change. However, “losslessly” is technically imprecise because the implementation uses lossy UTF-8 conversion at an incomplete character boundary. ✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Warning Your free Security trial is over. An organization admin can activate Security or dismiss this notice. Comment |
|
Duplicate — this fix is already merged upstream as #109. Closing to avoid noise. Apologies for the duplicate. |
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0065 · 63,577 in / 1,584 out · 8,131 cached (13%) · deepseek/deepseek-v4-flash, openrouter/openai/text-embedding-3-small, z-ai/glm-5.2 · 218 embedded
critique: $0.0019 · 23,038 in / 247 out · 0 cached (0%) · deepseek/deepseek-v4-flash
security: $0.0031 · 22,534 in / 840 out · 8,131 cached (36%) · deepseek/deepseek-v4-flash, z-ai/glm-5.2
tests: $0.0010 · 11,932 in / 88 out · 0 cached (0%) · deepseek/deepseek-v4-flash
description: $0.0004 · 4,410 in / 111 out · 0 cached (0%) · deepseek/deepseek-v4-flash
How this change flows1 changed behaviour across 10 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 44 further behaviours left out to keep the diagram readable. flowchart LR
n0["jsonl_rejects_unsafe_stream_names<br/>changed"]:::changed
n1["new"]:::impacted
n2["expect"]:::impacted
n3["tick"]:::impacted
n4["...ves_its_card_blocked_rather_than_stranded"]:::impacted
n5["Result"]:::impacted
n6["complete_run"]:::impacted
n0 -->|calls| n1
n0 -->|tests| n1
n3 -->|calls| n2
n3 -->|tests| n2
n4 -->|calls| n2
n4 -->|tests| n2
n4 -->|calls| n3
n4 -->|calls| n6
n4 -->|tests| n6
n6 -->|uses| n5
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
JsonlAppendStore::appendfails permanently on any stream whose file happens tohave a UTF-8 continuation byte at offset
len - 4096:The append fails, so the record is lost. Nothing else reports it — the file that
would carry the trace is the one that was not written. And it does not recover:
the failed append left the file unchanged, so
lenis the same next time, solen - 4096points at the same broken byte. One unlucky em-dash freezes thestream forever.
JsonlAppendStore::next_offset(src/harness/store/mod.rs:328) avoids readingthe whole file by seeking to a fixed-size tail window:
startis an arbitrary byte offset — nobody placed it on a character boundary.read_to_stringrequires the entire read to be valid UTF-8, so ifstartlandsinside a multi-byte character the read fails, and with it the whole append.
With ASCII this never happens: every character is one byte, so every offset is a
character boundary. It shows up as soon as the content is not English. An em-dash
(
—) and a euro sign (€) are three bytes each; the more non-ASCII in the file,the higher the chance that byte
len - 4096falls inside a character. Note thatonly a continuation byte (
10xxxxxx, 128–191) is fatal — a leading byte is avalid character boundary and decodes fine, which is part of why this looks
intermittent.
It is effectively a die rolled on every append:
lenchanges with every writtenline, so the window slides and points at a different byte each time. One bad roll
is enough, because after that the file stops growing.
Observed in production on a session journal of 8003 bytes.
8003 - 4096 = 3907,and bytes 3905–3907 are
e2 80 94— U+2014, an em-dash. The read thereforestarts on the third byte of that character. The file has been stuck at 8003
bytes since the first failure. Of the 17 journal files in that directory exactly
one was frozen; the other sixteen happen to have an ordinary byte at
len - 4096.Read bytes, then decode:
read_to_endinto aVec<u8>, thenString::from_utf8_lossy.Lossy decoding is not a concession here, for a reason already present in the
function: when the window does not start at the beginning of the file
(
start > 0),next_offsetdiscards the first line anyway, because it may becut in half (
complete_from). The damaged bytes are by definition at the startof the window, i.e. in exactly the line that is discarded. No replacement
character can ever reach a line that is used. And when
start == 0the windowbegins at the start of the file — always a character boundary — so
from_utf8_lossyreplaces nothing.Thirteen lines, nine of them comment. No new dependency. No behaviour change for
files that work today.
A file that is already frozen starts growing again. No migration or repair
step is needed: once
next_offsetstops tripping, the next append on such astream succeeds and the file grows past its stuck length.
jsonl_append_survives_a_tail_window_that_starts_mid_characterinsrc/harness/store/test.rs. It does not guess: it pads the first record one byteat a time until byte
len - 4096is provably a UTF-8 continuation byte(
b & 0b1100_0000 == 0b1000_0000), and only then writes the file. So it failsalways rather than occasionally.
Checked in both directions:
5e026cdit fails with exactly the production message —Validation("append store read error: stream did not contain valid UTF-8");harness::storestay green.Summary by CodeRabbit
Bug Fixes
Tests