Uh oh!
There was an error while loading. Please reload this page.
fix(fleet): stream history event logs instead of materializing full lists - #438
Merged
Merged
Conversation
…ists _read_full_log now yields events one at a time and _scan_history_events accepts an Iterable, so a large retained event log is read in O(1) memory (one line + one parsed dict at a time) instead of loading every event into a list before scanning. Fixes a memory blowup when building Fleet history entries from large logs.
…e in #438 review Blocking: - Fix _read_full_log's docstring: memory is bounded by the largest single line (not O(1)) since `for raw_line in f` reads a whole line before yielding; a 5 MiB single-line log is still materialized whole. - Correct the false "sole consumer drains to completion" claim, which the same commit's own tests contradict by consuming the generator directly and abandoning it early. - Rename test_corrupt_log_raises_only_on_exhaustion to test_corrupt_log_raises_on_first_next and assert the real (stronger) guarantee via next() instead of list(), and correct _CorruptEventLogError's docstring to match (no drain-dependent timing exists for an all-garbage log). Recommendations applied: - Widen the per-line parse guard to catch RecursionError so one hostile line can't silently discard an otherwise-valid log. - Count and log skipped unparseable lines so partial corruption is no longer silent. - Split build_history_entries' blanket except into an expected OSError/_CorruptEventLogError branch (warning) and an unexpected Exception branch (error), so a real defect in _scan_history_events isn't hidden at warning level forever. - Fix the stale "in the tail" wording on total_tokens (History reads the whole log, not a tail). - Narrow the json-module monkeypatch to json.loads via patch.object (stops breaking on an unrelated json.* addition to history.py). - Drop the redundant `not isinstance(result, list | Sequence)` assertion (list is already a Sequence; the isgenerator assertion subsumes it) and its now-unused import. - Remove a dead _write_log call and a redundant local module import in existing tests. - Add a spy test pinning that _build_entry actually streams into _scan_history_events rather than materializing a list first. - Add coverage for a log with one parseable line among garbage (must not be treated as corrupt) and for the deferred-OSError contract on a missing path. - Narrow test_scan_accepts_a_one_shot_iterator's docstring/assertions to what it actually pins (single-pass consumption via a yield counter, not mere non-indexing). - Add a CHANGELOG entry and extend AGENTS.md's fleet/history.py bullet to document the single-pass generator contract. Skipped (see review response): wrapping _read_full_log in contextlib.closing at the _build_entry call site, and the matching `assert inspect.isgenerator(gen)` treatment in the lazy-parse test as originally worded -- contextlib.closing requires _SupportsClose, which the deliberately-kept Iterator return annotation doesn't satisfy, and `uv run ty check src` confirms the resulting type error. Docstring restructuring into Google-style Yields/Raises sections was also skipped as disproportionate to this pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)
marked this pull request as ready for review
August 14, 2026 16:47
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 15, 2026
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
_read_full_logis now a generator that yields events one at a time instead of building a full list._scan_history_eventsaccepts anIterableand makes a single forward pass, so it can consume the streaming generator.Closes#436
Test plan
uv run pytest tests/test_fleet/test_history.py -q— 36 passed