🚀 Advanced: tests: Improve stability of Subscribe tests by kuznetsss · Pull Request #6420 · XRPLF/rippled · GitHub

kuznetsss · 2026-02-25T17:40:44Z

High Level Overview of Change

This PR improves stability of Subscribe tests.

Context of Change

Subscribe tests were flaky because each test performs some operations (e.g. sends transactions) and waits for messages to appear in subscription with 100ms timeout. If tests are slow (because compiled in debug or the machine is slow) some of them could fail. This PR adds an attempt to synchronise background Env's thread and test's thread by ensuring that all the scheduled operations are started before test's thread starts to wait for a websocket message. This is done by limiting IO threads of the app inside Env to 1 and adding a synchronisation barrier after closing ledger.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

Copilot

Pull request overview

This PR addresses flaky Subscribe tests by introducing a synchronization mechanism to ensure WebSocket subscription messages have been initiated before tests check for them. The issue (#6399) was caused by race conditions where tests would check for messages before the server's background thread had posted the async write operations.

Changes:

Added syncClose() method to test Env class that closes a ledger and synchronizes with the server's io_context
Replaced all env.close() calls in Subscribe_test.cpp with BEAST_EXPECT(env.syncClose()) to ensure synchronization
Added lambda capture of this in sendPayments helper to support BEAST_EXPECT usage

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/test/jtx/Env.h	Added syncClose() method with comprehensive documentation that closes a ledger and waits for all posted async operations to start
src/test/rpc/Subscribe_test.cpp	Replaced env.close() with env.syncClose() throughout and fixed lambda capture to enable test assertions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov · 2026-02-25T18:22:58Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.8%. Comparing base (afc660a) to head (68ad67e).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #6420   +/-   ##
=======================================
  Coverage     79.8%   79.8%           
=======================================
  Files          858     858           
  Lines        67757   67761    +4     
  Branches      7558    7546   -12     
=======================================
+ Hits         54074   54089   +15     
+ Misses       13683   13672   -11

Files with missing lines	Coverage Δ
src/xrpld/app/main/Application.cpp	`70.4% <100.0%> (+0.4%)`	⬆️
src/xrpld/app/main/BasicApp.h	`100.0% <100.0%> (ø)`

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

godexsoft

Seems like this relies on the fact we have one thread in tests. If it changes there may be issues but for now this is best we can do by the looks of it 👍

kuznetsss · 2026-02-27T13:33:18Z

@godexsoft, you are right and it turned out that the application was not single threaded. I've added an assert into syncClose() and enforced the application to have only 1 IO thread in these tests.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

a1q123456

May be a good approach. We can merge it and see if it works.

godexsoft

Leaving few nits 👍

godexsoft · 2026-03-02T16:53:06Z

+            app().getNumberOfThreads() == 1,
+            "syncClose() is only useful on an application with a single thread");
+        auto const result = close();
+        auto server_barrier = std::make_shared<std::promise<void>>();


nit: Don't know if xrpld uses this style or camelCase but i guess camelCase should be preferred here

@copilot, can you fix this?

godexsoft · 2026-03-02T16:53:34Z

+        auto const result = close();
+        auto server_barrier = std::make_shared<std::promise<void>>();
+        auto future = server_barrier->get_future();
+        boost::asio::post(


Wasn't there a util we made to post instead of using asio directly?

I think that was for spawn(), not post()

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/test/jtx/Env.h:12

The include for vault.h is missing the closing > which will break compilation. Please restore #include <test/jtx/vault.h>.

#include <test/jtx/vault.h>
#include <test/unit_test/SuiteJournal.h>

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The `Subscribe` tests were flaky, because each test performs some operations (e.g. sends transactions) and waits for messages to appear in subscription with a 100ms timeout. If tests are slow (e.g. compiled in debug mode or a slow machine) then some of them could fail. This change adds an attempt to synchronize the background Env's thread and the test's thread by ensuring that all the scheduled operations are started before the test's thread starts to wait for a websocket message. This is done by limiting I/O threads of the app inside Env to 1 and adding a synchronization barrier after closing the ledger.

tests: Add syncClose() for async operation synchronisation in tests

33c0d68

Copilot AI review requested due to automatic review settings February 25, 2026 17:40

Copilot started reviewing on behalf of kuznetsss February 25, 2026 17:41 View session

Copilot AI reviewed Feb 25, 2026

View reviewed changes

Comment thread src/test/jtx/Env.h

Comment thread src/test/jtx/Env.h Outdated

Add missing include

2c660b6

kuznetsss requested review from a1q123456 and godexsoft February 25, 2026 18:00

Merge branch 'develop' into 6399_Fix_flaky_subscribe_test

3280979

Copilot AI review requested due to automatic review settings February 26, 2026 12:10

Copilot started reviewing on behalf of kuznetsss February 26, 2026 12:10 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

godexsoft approved these changes Feb 26, 2026

View reviewed changes

Make the app single threaded

bec3aea

kuznetsss requested review from Copilot and godexsoft February 27, 2026 13:35

Copilot started reviewing on behalf of kuznetsss February 27, 2026 13:35 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

Comment thread src/test/jtx/Env.h Outdated

Comment thread src/xrpld/app/main/Application.h Outdated

Update src/xrpld/app/main/Application.h

ce226bd

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings February 27, 2026 14:34

Copilot started reviewing on behalf of kuznetsss February 27, 2026 14:35 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

kuznetsss added 2 commits February 27, 2026 14:48

Fix race condition

0739050

Merge branch 'develop' into 6399_Fix_flaky_subscribe_test

4663e6d

Copilot AI review requested due to automatic review settings February 27, 2026 15:14

Copilot started reviewing on behalf of kuznetsss February 27, 2026 15:15 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

Comment thread src/xrpld/app/main/BasicApp.h

Comment thread src/test/jtx/Env.h

Comment thread src/test/jtx/Env.h

Comment thread src/xrpld/app/main/Application.h

Comment thread src/test/jtx/impl/envconfig.cpp

a1q123456 approved these changes Mar 2, 2026

View reviewed changes

Merge branch 'develop' into 6399_Fix_flaky_subscribe_test

d9c46d5

kuznetsss added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Mar 2, 2026

godexsoft reviewed Mar 2, 2026

View reviewed changes

Rename server_barrier to serverBarrier for consistency

8b70f59

Copilot AI review requested due to automatic review settings March 2, 2026 17:11

Merge branch 'develop' into 6399_Fix_flaky_subscribe_test

c70f183

Copilot started reviewing on behalf of kuznetsss March 2, 2026 17:11 View session

Fix formatting in Env.h

2c2d0c2

Copilot AI reviewed Mar 2, 2026

View reviewed changes

Update src/test/jtx/Env.h

2591171

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings March 2, 2026 17:28

Copilot started reviewing on behalf of kuznetsss March 2, 2026 17:28 View session

Fix include directive for vault.h in Env.h

68ad67e

Copilot AI reviewed Mar 2, 2026

View reviewed changes

Comment thread src/test/jtx/Env.h

Comment thread src/xrpld/app/main/Application.h

bthomee merged commit 5300e65 into XRPLF:develop Mar 3, 2026
3 checks passed

kuznetsss deleted the 6399_Fix_flaky_subscribe_test branch March 3, 2026 14:13

mvadari added this to the 3.2.0 milestone May 20, 2026

Uh oh!

Conversation

kuznetsss commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

Type of Change

API Impact

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

godexsoft left a comment

Choose a reason for hiding this comment

Uh oh!

kuznetsss commented Feb 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

a1q123456 left a comment

Choose a reason for hiding this comment

Uh oh!

godexsoft left a comment

Choose a reason for hiding this comment

Uh oh!

godexsoft Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

kuznetsss Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

kuznetsss Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

godexsoft Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

kuznetsss Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kuznetsss commented Feb 25, 2026 •

edited

Loading

codecov Bot commented Feb 25, 2026 •

edited

Loading