Skip to content

logreader: read segments in order, not discovery order - #38686

Open
lgoyal6 wants to merge 1 commit into
commaai:masterfrom
lgoyal6:logreader-segment-order
Open

logreader: read segments in order, not discovery order#38686
lgoyal6 wants to merge 1 commit into
commaai:masterfrom
lgoyal6:logreader-segment-order

Conversation

@lgoyal6

@lgoyal6 lgoyal6 commented Aug 22, 2026

Copy link
Copy Markdown

Description

auto_source accumulates valid_files: dict[int, str] across two nested loops: filenames (rlog, then qlog) at logreader.py:169 and sources at :170. Each pass prunes needed_seg_idxs down to the segments still missing (:178), so a later pass only ever adds the gaps, and those entries land at the end of the dict. The return handed back .values(), which is insertion order, not segment order:

valid_files |= files                                                         # :175
needed_seg_idxs = [idx for idx in needed_seg_idxs if idx not in valid_files]  # :178
if len(needed_seg_idxs) == 0:
  return list(valid_files.values())                                          # :182

Whenever a lower-numbered segment is resolved after a higher-numbered one, the returned list is out of order. Two ways that happens in normal use:

  1. AUTO mode falling back to qlogs. If a middle segment's rlog is missing, the rlog pass returns the segments around it and the qlog pass fills the hole last. This is the path that already logs "N/M rlogs were not found, falling back to qlogs for those segments..." (:192), so it is an expected case, not an exotic one.
  2. Plain RLOG mode, no fallback involved. The default source list is four sources (:249) and each returns only the segments it has. If the first source is missing an early segment that a later source supplies, the order is wrong.

LogReader.__iter__ (:268) reads the list in the order given, so segments come out shuffled and logMonoTime moves backwards across segment boundaries. sort_by_time does not cover this: it sorts self._ents within a single file (:120).

The existing tests cannot observe it. local_source and local_auto_source return the same path for every segment via dict.fromkeys (test_logreader.py:84, :89), so ordering is invisible, and local_auto_source returns {} for rlogs, which makes the qlog pass fill every segment in one call and therefore in order. test_sort_by_time (:254) reads a single file.

Fix: return the files ordered by segment index. seg_idxs is always ascending, since a descending slice always evaluates to empty (10:0:-1 and 0:6:-2 both give []), so this is exactly the requested order and can never reorder a valid request.

Verification

Three segments where segment 1 has no rlog, resolved through a fake source, so no network or device is needed:

before: ['seg0/rlog.zst', 'seg2/rlog.zst', 'seg1/qlog.zst']
after:  ['seg0/rlog.zst', 'seg1/qlog.zst', 'seg2/rlog.zst']

End to end, same setup with real logs written by save_log and read back through LogReader(..., sort_by_time=True):

before: logMonoTime [0, 2000000000, 1000000000]   # time goes backwards 1s
after:  logMonoTime [0, 1000000000, 2000000000]

Also reproduced with no qlog fallback at all: two rlog-only sources, the first holding segments 1 and 2 and the second holding segment 0, returned [seg1, seg2, seg0] before and [seg0, seg1, seg2] after.

openpilot/tools/lib/tests/test_logreader.py: 56 tests pass (22 skipped, network). ruff, ty, and codespell clean.

auto_source accumulates valid_files across the filename and source
loops, pruning needed_seg_idxs each pass, so segments resolved by a
later pass land at the end of the dict. Returning .values() gave
insertion order, so a route whose segments come from mixed sources was
read out of order and logMonoTime moved backwards across segment
boundaries. sort_by_time does not cover this; it sorts within one file.

Return the files ordered by segment index. seg_idxs is always ascending,
since a descending slice always evaluates to empty.
@github-actions github-actions Bot added the tools label Aug 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Process replay diff report

Replays driving segments through this PR and compares the behavior to master.
Please review any changes carefully to ensure they are expected.

✅ 0 changed, 66 passed, 0 errors

@lgoyal6

lgoyal6 commented Aug 22, 2026

Copy link
Copy Markdown
Author

The two red checks are the fork-runner GL gap, not this change.

Because this PR comes from a fork, the runner selection falls through to GitHub's hosted image (Image: ubuntu-24.04) rather than the namespace-profile machines. That image has no libGLESv2.so.2, so raylib's headless backend cannot load:

File ".../site-packages/raylib/__init__.py", line 33, in _load_cffi
ImportError: libGLESv2.so.2: cannot open shared object file: No such file or directory
ImportError: failed to load raylib headless backend extension _raylib_cffi_headless

Create UI Report dies on the same import. tools/setup_dependencies.sh does not install any GL/GLES packages, so the library has to come from the runner image.

unit tests is otherwise clean, with that one UI test as the only failure:

856 passed, 43 skipped, 1 xfailed, 1 failed in 85.74s
FAILED openpilot.selfdrive.ui.tests.test_raylib_ui.TestRaylibUi.test_raylib_ui

I also ran this workflow against untouched master on my fork as a baseline, and it returns the identical 856 passed, 43 skipped, 1 xfailed, 1 failed with the same single failure, so this change adds none.

Everything that does exercise the change passes: static analysis, build release, build macOS, and process replay (0 changed, 66 passed, 0 errors).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant