Serialise Recorder.record with a lock (fixes the intermittent test_concurrent_recording failure) - #99
Open
MyceliaLabsBV wants to merge 1 commit into
Open
MyceliaLabsBV wants to merge 1 commit into
MyceliaLabsBV wants to merge 1 commit into
Conversation
Recorder.record wrote with
with open(self.filename, "a", encoding="utf-8") as f:
json.dump(event, f)
f.write(...)
json.dump emits several small writes, so two threads appending to one
Recorder can interleave mid-line and lose or corrupt an event.
This is already visible in the suite:
tests/unit/test_recorder.py::TestRecorderErrorHandling::
test_concurrent_recording fails intermittently, typically with
"assert 4 == 5" -- one of five events lost.
Measured on the current main: 1 failure in 20 runs.
Measured with this change: 0 failures in 30 runs.
The fix serialises the event to a single string and performs one write
under a threading.Lock.
Low severity in normal use, since Swarm gives each agent its own
Recorder and its own file -- but it is a flaky test in CI, and the
underlying race is real for any code that shares a Recorder.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
The problem
Recorder.recordwrites with:json.dumpemits several small writes rather than one, so two threads appending through the sameRecordercan interleave mid-line and lose or corrupt an event.This is already visible in your own test suite.
tests/unit/test_recorder.py::TestRecorderErrorHandling::test_concurrent_recordingfails intermittently, typically withassert 4 == 5— one of five events gone.Measured, not assumed
main, 20 runsThe change
Serialise the event to a single string, then perform one write under a
threading.Lock.Severity
Low in normal use —
Swarmgives each agent its ownRecorderand its own file, so nothing in the shipped paths shares a writer. But it is a flaky test in CI, and the race is real for anything that does share aRecorder.Happy to drop this if you'd rather fix the test than the class, though the test looks like it is describing the intended contract.