test: deflake fastutf8stream destroy and reopen tests - #65554
Conversation
Sorry, something went wrong.
Both tests read the destination file with no ordering guarantee against the fs.write() that Utf8Stream still has in flight, so under load the read can observe an empty file. In test-fastutf8stream-destroy the read is issued right after destroy(). In test-fastutf8stream-reopen it is ordered on 'drain', documented as emitted when the buffer has drained enough to allow continued writing, which says nothing about the bytes being observable in the file. The reopen path also emits a 'drain' of its own from a nextTick before the write has landed. Order both reads on 'write' instead, documented as emitted when a write operation has completed and emitted from #release() once the underlying write returned. In sync mode it is emitted from within write(), so the listener is attached before the write call. No data is lost by Utf8Stream here: re-reading the file after a failed assertion shows the expected content. This corrects an expectation of the tests, not the runtime. Signed-off-by: Christian Aurich <christian.aurichzm@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #65554 +/- ##
==========================================
+ Coverage 90.05% 90.06% +0.01%
==========================================
Files 751 751
Lines 254420 254420
Branches 47975 47972 -3
==========================================
+ Hits 229121 229156 +35
+ Misses 16483 16445 -38
- Partials 8816 8819 +3 see 32 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
Sorry, something went wrong.
|
Fast-track has been requested by @panva. Please 👍 to approve. |
Sorry, something went wrong.
|
pending node-stress-single-test started by @sxa Edit: appears to ✅ |
Sorry, something went wrong.
So I'm not sure we've necessarily proven anything as non of those reproduced the original error :-) Have just kicked off another from the current main on four platforms at https://ci.nodejs.org/view/Stress/job/node-stress-single-test/852 |
Sorry, something went wrong.
You're right that those runs don't prove much yet. One important difference from the reproducer I used is concurrency. The race is between the fs.write() that is still in flight and the subsequent readFile() path, which is not ordered after that write has completed. I was able to make it reproduce often enough by running each test file in 24 concurrent processes, with a separate TEST_THREAD_ID per process so they do not share a tmpdir. Across two Linux x64 environments, the rates varied quite a bit for destroy and were more stable for reopen:
Taking the lower destroy rate and assuming roughly independent runs, 100 runs still have about a 75% chance of producing zero failures, and even 1000 runs have about a 5% chance. So 844 coming back clean is an expected outcome rather than evidence against the race, and 853 may well come back clean too without disproving anything. Sequential runs on an otherwise idle worker are also much less favorable for reproducing the contention I was using. Every failure I observed had the same signature: readFile() returning '' at the assertion this patch reorders. I did not observe another failure mode in the stress runs. With the patch applied, both tests produced 0 failures in 8640 runs each in the environment that produced the 0.29% and 1.76% baselines, where those rates would correspond to roughly 25 and 152 failures. The ordering issue itself does not depend on reproducing the flake every time: drain is not a completion barrier for the write being asserted, whereas the write event used here is emitted from #release() after the underlying write operation completes. Happy to share the stress setup or run other configurations if useful. |
Sorry, something went wrong.
test-fastutf8stream-destroy and test-fastutf8stream-reopen can
read their destination files before the underlying write operation has
completed.
In test-fastutf8stream-destroy, readFile() is called immediately
after destroy(), while an asynchronous fs.write() may still be in
flight.
In test-fastutf8stream-reopen, the reads are ordered on 'drain'.
However, 'drain' indicates that the internal buffer has drained
sufficiently to allow continued writing; it does not guarantee that the
write being asserted has completed. In the reopen path, a 'drain' is
scheduled with process.nextTick() after 'ready', so it can be observed
before the subsequent write has completed.
Order these reads on the 'write' event instead, which is emitted from
#release() after the underlying write operation completes.
For the synchronous reopen path, the 'write' listener is registered
before calling write(), since the event can be emitted from within the
write() call.
This only changes test synchronization. No Utf8Stream runtime behavior
is changed.
Testing
Before the change:
After the change, locally on Linux x64:
Refs: https://github.com/nodejs/reliability/blob/main/reports/2026-08-20.md
Refs: https://github.com/nodejs/reliability/blob/main/reports/2026-08-26.md