fix(jsonl): abandon an over-long line as it streams instead of buffering it whole - #935
Open
joaovictor91123 wants to merge 1 commit into
Open
Conversation
…ing it whole max_line_bytes documents itself as "skip individual lines larger than this instead of allocating them whole". tail_lines did the opposite: bytes were pushed into current_line unconditionally and the size test only ran at the terminating newline, so an over-long line was read into memory in full and *then* discarded. The cap changed which lines were returned; it did not bound memory. That matters because tail_lines exists to stop exactly this — "loading the entire file on every poll grows memory with log age" (GeniePod#223). A single pathological line reintroduces it on the same polling paths: genie-api::routes tails 50 lines per dashboard request, and genie-core::tools::actuation tails the action log on the actuation path. An 8 MB line costs an 8 MB transient allocation on every poll, on a device whose governor is already making mode decisions off memory pressure — and the line is not even returned to the caller. Track an abandon flag instead: once current_line passes max_line_bytes, clear the buffer and discard the rest of that line until the newline that starts it. Peak buffering is then bounded by max_line_bytes regardless of the longest line in the file. The predicate stays inclusive (len > max abandons), so a line of exactly max_line_bytes is still returned, and the trailing-fragment path at the top of the file honours the same flag. Three tests, covering what the existing ones do not: * peak allocation during the call, measured with a peak-tracking global allocator in its own test binary. The behavioural tests pass just as well against an implementation that buffers the line whole, so the contract needs a test that watches the allocator. * an oversize line spanning 16 chunks. The existing oversize test uses a 512-byte line, which fits inside a single 4096-byte read, so the flag-survives-across-chunks path was untested. * the exact-length boundary, so an off-by-one here cannot silently start dropping lines that used to be returned. Closes GeniePod#930
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.
Summary
Closes #930.
max_line_bytesdocuments itself as:tail_linesdid the opposite. Bytes were pushed intocurrent_lineunconditionally, and the size test only ran once the terminating newline was
reached — so an over-long line was read into memory in full and only then
discarded. The cap changed which lines were returned; it did not bound memory.
Measured against
main, tailing a file whose middle line is 8 MiB withmax_line_bytes = 4096:16 MiB of peak allocation (the buffer doubles past the 8 MiB it needs) to
produce two short lines — the long one is not even returned to the caller.
Why it matters
tail_linesexists precisely to stop this: "loading the entire file on everypoll grows memory with log age" (#223). One pathological line reintroduces it
on the same polling paths:
genie-api::routestails 50 lines on a dashboard requestgenie-core::tools::actuationtails the action log on the actuation pathA runaway tool result, an embedded blob, or a partially-written record therefore
costs a multi-megabyte transient allocation on every poll, on an 8 GB Jetson
whose governor is already making mode decisions off memory pressure.
Changes
current_linepassesmax_line_bytes,clear the buffer and set a flag that discards the rest of that line's bytes
until the newline that starts it. Peak buffering is then bounded by
max_line_bytesregardless of the longest line in the file.len > maxabandons — so a line of exactlymax_line_bytesis still returned, as before.same flag, so an over-long first line is skipped rather than resurrected.
Real Behavior Proof
Tested profile / hardware (check all that apply):
jetsonraspberry_piportable_sbclaptopmacWhat I ran
x86_64 Linux laptop (Ubuntu 22.04 LTS, kernel 6.8.0-136-generic,
rustc 1.96.0 / cargo 1.96.0). No Jetson available to me — see the validation
gap below.
Then, to confirm the new test actually catches the bug rather than merely
passing: checked out
main'sjsonl.rsunderneath the new test file and ran itagain.
What I observed
Against
main's implementation with the new test in place:With the fix restored, the same test passes, and so does the rest of the
workspace: 141
genie-commontests + the new integration binary green,whole-workspace
cargo testgreen, clippy clean,cargo fmt --checkclean.Why an allocator-based test
The behavioural tests only observe which lines come back. They pass just as
well against an implementation that reads a 50 MB line into memory and then
throws it away — which is exactly the bug. So the contract needs a test that
watches the allocator:
crates/genie-common/tests/jsonl_tail_memory.rsinstalls a peak-tracking#[global_allocator], opens its measurement window after the fixture iswritten, and asserts peak growth during the call.
It lives in its own integration-test binary so the instrumentation cannot
perturb, or be perturbed by, any other test. The budget (1 MiB) is ~256x
max_line_bytesand ~8x below the line itself — wide enough that incidentalharness allocation cannot trip it, tight enough that buffering the line whole
cannot pass.
Two behavioural tests come with it, covering paths the existing ones do not:
512-byte line, which fits inside a single 4096-byte read — so the state that
has to survive across loop iterations (flag set in one chunk, cleared by a
newline several chunks later) was untested. The test also re-reads the same
file with a larger cap and asserts the line comes back, proving the skip is
driven by
max_line_bytesand not by the chunking.discarding lines that used to be returned.
Validation gap
I could not verify on Jetson hardware. The equivalent verification path: this is
pure buffer management inside one synchronous function — no async, no hardware
dependency, no model involvement. Its two observable properties are the lines it
returns and the memory it uses to return them, and both are asserted directly
above. The memory property is architecture-independent: the allocation being
removed is a
Vec<u8>sized by the input line, which is the same on aarch64 ason x86_64. Callers are unchanged.
Test plan
git checkout main -- crates/genie-common/src/jsonl.rs.cargo test -p genie-common --test jsonl_tail_memory— fails with the 16 MBfigure above.
git checkout HEAD -- crates/genie-common/src/jsonl.rs, rerun — passes.Notes for reviewers
#[global_allocator]is scoped to that one test binary and does notaffect any shipped code path or any other test.
is untouched.