Skip to content

Conversation

@lhoguin
Copy link
Contributor

@lhoguin lhoguin commented Sep 19, 2025

And eof crashes.

The problem is that we may end up trying to read more data from the file when scanning, despite being at the end of the file. This results in the current Acc to be returned instead of the remaining data being parsed.

This results in some messages at the end of the file being truncated off despite still being in memory (and still pointing to the end of the original file, well past the truncation point).

Tentative fix for #14181 (reply in thread)

Someone please double check that there's no off-by-one error.

@lhoguin lhoguin force-pushed the loic-fix-cq-eof-eof branch from 5ea57d8 to 9246583 Compare September 19, 2025 14:00
@lukebakken lukebakken self-assigned this Sep 19, 2025
@lukebakken lukebakken self-requested a review September 19, 2025 14:16
@lukebakken
Copy link
Collaborator

cc @gomoripeti

@lhoguin
Copy link
Contributor Author

lhoguin commented Sep 19, 2025

CQ test for file scanning is now failing. I'll look on Monday.

@lhoguin lhoguin force-pushed the loic-fix-cq-eof-eof branch from 9246583 to 8294c3f Compare September 19, 2025 14:55
@lhoguin
Copy link
Contributor Author

lhoguin commented Sep 19, 2025

I've changed it to +8, but will need to double check if that's right.

@lhoguin
Copy link
Contributor Author

lhoguin commented Sep 19, 2025

For greater clarity I will change it to Size + 9 =< before this can be merged.

@lhoguin lhoguin force-pushed the loic-fix-cq-eof-eof branch from 8294c3f to 52363c2 Compare September 22, 2025 12:23
@lhoguin
Copy link
Contributor Author

lhoguin commented Sep 22, 2025

I am adding a test case that demonstrates the bug and the fix and can indeed confirm that it happens because of this off-by-nine (ie current code works if we are outside of this range).

@lhoguin lhoguin force-pushed the loic-fix-cq-eof-eof branch from 52363c2 to 1a7e92a Compare September 22, 2025 13:30
@michaelklishin michaelklishin added this to the 4.3.0 milestone Sep 22, 2025
@lhoguin lhoguin changed the title DO NOT MERGE CQ shared: Fix off-by-nine error leading to lost messages CQ shared: Fix off-by-nine error leading to lost messages Sep 22, 2025
@lhoguin lhoguin marked this pull request as ready for review September 22, 2025 18:29
And `eof` crashes.

The problem is that we may end up trying to read more data
from the file when scanning, despite being at the end of the
file. This results in the current Acc to be returned instead
of the remaining data being parsed.

This results in some messages at the end of the file being
truncated off despite still being in memory (and still pointing
to the end of the original file, well past the truncation point).
@lhoguin
Copy link
Contributor Author

lhoguin commented Sep 23, 2025

@lukebakken Did you change something in the commit?

@lukebakken
Copy link
Collaborator

No, sorry, I just clicked the "Update with rebase" button here.

@lukebakken
Copy link
Collaborator

lukebakken commented Sep 23, 2025

Update - I hit the eof bug one more time while using main, so I've re-applied this PR and restarted my tests. I'm certain this PR fixes this issue, but just to be sure I'll let my env run all week. @trvrnrth is also testing.

@lhoguin
Copy link
Contributor Author

lhoguin commented Sep 24, 2025

I will merge this now. The tests have confirmed it fixes a bug. If we still hit eof it may be a different bug with the same symptoms.

@lhoguin lhoguin merged commit 8ea901b into main Sep 24, 2025
287 checks passed
@lhoguin lhoguin deleted the loic-fix-cq-eof-eof branch September 24, 2025 10:50
lhoguin added a commit that referenced this pull request Sep 24, 2025
CQ shared: Fix off-by-nine error leading to lost messages (backport #14576)
lhoguin added a commit that referenced this pull request Sep 24, 2025
CQ shared: Fix off-by-nine error leading to lost messages (backport #14576) (backport #14599)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants