Repository navigation
Give each thread its own random number generator - #2783
Closed
neil-marcellini wants to merge 4 commits into
Closed
neil-marcellini wants to merge 4 commits into
neil-marcellini wants to merge 4 commits into
Conversation
Contributor
Author
|
Closing as explained here. |
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.
Details
(Neil's AI agent)
SRandom::_generatorwas a single process-globalmt19937_64with no mutex and nothread_local, and_distribution64was likewise shared mutable state. Every draw advances that state, so concurrent draws from Bedrock's worker threads were a data race that handed the same "random" value to more than one thread.This is not theoretical. Production Auth logs show four different threads on the same host receiving the identical reportID in the same microsecond:
The new test measures how bad it was: of 160,000 draws across 16 threads, only about 31,000 were unique before the fix. Roughly four in five concurrent draws were duplicates. That breaks the uniqueness assumption behind every randomly generated ID in Auth (reportIDs, reportActionIDs, transactionIDs), and read-then-insert guards like
Transaction::generateIDcannot save us, since two commands handed the same value both read "not present" and both insert.The fix makes
_generatorand_distribution64thread_local. The per-thread generator is seeded fromrandom_devicemixed with the thread id, becauserandom_devicecan hand the same 32-bit value to two threads that seed at the same moment, which would leave them generating identical sequences. A mutex would also close the race but would serialize every ID draw.One caveat on the test, which is written up in a comment alongside it: it fails every time on an arm64 dev machine, but it can pass on the x86-64 machines CI runs on, where the window between reading and advancing the generator's state is only a few nanoseconds wide. I confirmed that — CI came back green twice on the test-only commit with the bug still present. So it is a reliable local reproducer and a best-effort CI guard, and a green CI run on it is not evidence either way.
Fixed Issues
For https://github.com/Expensify/Expensify/issues/676823
Tests
(Neil's AI agent) Automated tests were added.
LibStuff::testRandomIsThreadSafedraws 10,000 values fromSRandom::rand64()on each of 16 threads and asserts that all 160,000 are distinct. It fails 5 out of 5 runs without the fix (19–28% unique draws) and the fixture passes 39/39 with it.Internal Testing Reminder: when changing bedrock, please compile auth against your new changes
(Neil's AI agent) Auth compiles and links cleanly against this change.