Repository navigation
Publish commits atomically, and run tests in CI - #2
Merged
Merged
Conversation
A commit needs exclusivity, so one of two racing writers wins, and atomic visibility, so a reader that sees a version sees all of it. Creating the version file with O_CREAT|O_EXCL and then writing into it gave only the first. For a moment the version existed but was empty: a reader in that window reported it as the latest version, and once the writer finished the same version number returned different data. The commit now goes to a uniquely named temp file, is fsynced, and is hard-linked to the version name. link() is atomic and fails if the target exists, so exclusivity is unchanged and the version name only ever points at complete contents. The log directory is fsynced on platforms that allow it, and a buffered write replaces os.write, whose return value was ignored and can be short. Temp names do not match the version pattern, so readers never see them; vacuum removes any left by a crashed writer. The old concurrency test simulated a stale writer one step at a time. These use real threads: racing writers produce exactly one winner, retrying writers lose and duplicate nothing with no gaps in the log, and a failed publish leaves no version and no temp file behind. The regression test freezes a writer as it starts writing commit bytes and checks what a reader sees. It passes on this code and fails on the original. A random stress test kept as well passed 6 of 6 times against the original, because the window lasted microseconds, and says so.
Python 3.11 to 3.13 on Linux, plus 3.13 on Windows and macOS. Commits are published with a hard link, so each OS runs against its native filesystem: ext4, NTFS and APFS. Each job also round-trips a table through the command line.
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Two commits: a correctness fix for how commits are published, and CI.
The bug
A commit needs two properties at once:
The old writer created
<version>.jsonwithO_CREAT|O_EXCLand then wrote into it. That gave exclusivity but not atomic visibility. For a moment the version existed and was empty. A reader in that window reported it as the latest version, and once the writer finished, the same version number returned different data. A version that changes after it becomes visible breaks the guarantee the whole format rests on.The fix
Write the commit to a uniquely named temp file,
fsyncit, then hard-link it to the version name.link()is atomic and fails if the target exists, so the losing writer still getsConcurrentWriteError, and the version name can only ever point at complete, durable contents.Also:
os.write, whose return value was ignored and can be short.vacuumremoves any that a crashed writer left behind.Tests
The old concurrency test simulated a stale writer one step at a time. The new ones use real threads:
vacuum.The regression test freezes a writer as it starts writing commit bytes, whether through
os.writeor a file object, and checks what a reader sees. It passes on this code and fails on the original. I checked a random stress test against the original too, and it passed 6 times out of 6: the window lasted microseconds, so random timing almost never hit it. That test is kept as a stress check, and its docstring says it can't catch this race on its own.19 tests, up from 12.
CI
Python 3.11 to 3.13 on Linux, plus 3.13 on Windows and macOS. Since commits now depend on hard links, each OS runs against its native filesystem: ext4, NTFS and APFS. Each job also round-trips a table through the CLI. Adds a status badge to the README.