Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix file corruption in Electric.Utils.external_merge_sort() #2350

Merged
merged 2 commits into from
Feb 19, 2025

Conversation

alco
Copy link
Member

@alco alco commented Feb 18, 2025

When the input file is small enough to fit into a single chunk. The problem is that File.stream!, by default, looks for line break characters and normalizes any \r\n into \n when reading from the file. So the data it writes into the target file is different from the one it read from chunk_0 if it so happens that the latter has the sequence of bytes <<13, 10>> in it.

Copy link
Contributor

@msfstef msfstef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

alco and others added 2 commits February 19, 2025 13:25
When the input file is small enough to fit into a single chunk. The
problem is that File.stream!, by default, looks for line break
characters and normalizes any \r\n into \n when reading from the file.
So the data it writes into the target file is different from the one it
read from chunk_0 if it so happens that the latter has the sequence of
bytes <<13, 10>> in it.

The issue was reliably reproduced by running

    mix test test/electric/utils_test.exs:30 --repeat-until-failure 1000

Co-authored-by: Garry Hill <garry@electric-sql.com>
@alco alco force-pushed the alco/external-merge-sort branch from 0654f1a to f56eb3f Compare February 19, 2025 12:25
@alco alco merged commit 49dd88f into main Feb 19, 2025
35 checks passed
@alco alco deleted the alco/external-merge-sort branch February 19, 2025 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants