Skip to content

fix: read directory entries in name order so packs are reproducible - #93

Open
duranaaron wants to merge 1 commit into
unnamed:mainfrom
duranaaron:fix/deterministic-directory-read-order
Open

fix: read directory entries in name order so packs are reproducible#93
duranaaron wants to merge 1 commit into
unnamed:mainfrom
duranaaron:fix/deterministic-directory-read-order

Conversation

@duranaaron

Copy link
Copy Markdown

DirectoryFileTreeReader walks a folder with File.listFiles(), which returns entries in whatever order the file system stores them. That order is a property of the file system rather than of the files, so two machines holding the same folder can read it in two different orders.

ResourceContainerImpl keeps its resources in LinkedHashMaps and MinecraftResourcePackWriter writes them in insertion order, so the read order becomes the zip entry order. Two servers with identical files then build packs that have identical content but different bytes, and so different hashes.

I hit this on a Minecraft network where two backends held byte-identical resource files and produced a different pack hash each. The two servers had separate ext4 file systems, and ext4 orders directory entries by a hash of the file name seeded with a random value written at mkfs time, so the two orders never matched. It does not reproduce on a single machine, because both directories are then on the same file system.

This sorts each folder's entries by name before reading them, so the read order depends only on the file names.

The added test writes files in a non-alphabetical order and checks that the reader returns them sorted. Without the change it fails with:

expected: [a.txt, m.txt, z.txt, sub/b.txt, sub/y.txt]
but was:  [z.txt, m.txt, a.txt, sub/y.txt, sub/b.txt]

All 41 tests in creative-serializer-minecraft pass with the change.

One unrelated note: creative-api does not compile on main right now, because SpecialRender.playerHead() is declared twice, at lines 167 and 277. I patched that locally so I could run the tests, and left it out of this branch.

@FixedDev

Copy link
Copy Markdown
Member

lgtm

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.

2 participants