Skip to content

Increase disconnect await timeout in flaky P2P rejection tests #10267

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
siladu merged 3 commits into from
Apr 20, 2026
Merged

Increase disconnect await timeout in flaky P2P rejection tests #10267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
093a6f1
Increase disconnect await timeout in flaky P2P rejection tests
siladu Apr 17, 2026
fb05683
Merge branch 'main' into test/flaky-p2p
siladu Apr 17, 2026
09fc926
Merge branch 'main' into test/flaky-p2p
siladu Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/P2PNetworkTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,9 @@ public void rejectIncomingConnectionFromDenylistedPeer() throws Exception {
// Check connection is made, and then a disconnect is registered at remote
Assertions.assertThat(connectFuture.get(5L, TimeUnit.SECONDS).getPeerInfo().getNodeId())
.isEqualTo(localId);
Assertions.assertThat(peerFuture.get(5L, TimeUnit.SECONDS).getPeerInfo().getNodeId())
Assertions.assertThat(peerFuture.get(30L, TimeUnit.SECONDS).getPeerInfo().getNodeId())
.isEqualTo(localId);
assertThat(reasonFuture.get(5L, TimeUnit.SECONDS))
assertThat(reasonFuture.get(30L, TimeUnit.SECONDS))
.isEqualByComparingTo(DisconnectReason.UNKNOWN);
Comment on lines 211 to 216
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as in P2PPlainNetworkTest: please consider centralizing the 30s value into a named constant (potentially shared between these two test classes) so future adjustments don’t require updating multiple call sites.

Copilot uses AI. Check for mistakes.
}
}
Expand Down Expand Up @@ -261,9 +261,9 @@ public void rejectIncomingConnectionFromDisallowedPeer() throws Exception {
final Bytes localId = localEnode.getNodeId();
Assertions.assertThat(connectFuture.get(5L, TimeUnit.SECONDS).getPeerInfo().getNodeId())
.isEqualTo(localId);
Assertions.assertThat(peerFuture.get(5L, TimeUnit.SECONDS).getPeerInfo().getNodeId())
Assertions.assertThat(peerFuture.get(30L, TimeUnit.SECONDS).getPeerInfo().getNodeId())
.isEqualTo(localId);
assertThat(reasonFuture.get(5L, TimeUnit.SECONDS))
assertThat(reasonFuture.get(30L, TimeUnit.SECONDS))
.isEqualByComparingTo(DisconnectReason.UNKNOWN);
}
}
Expand Down
8 changes: 4 additions & 4 deletions ...reum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/P2PPlainNetworkTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,9 +256,9 @@ public void rejectIncomingConnectionFromDenylistedPeer() throws Exception {
// Check connection is made, and then a disconnect is registered at remote
Assertions.assertThat(connectFuture.get(5L, TimeUnit.SECONDS).getPeerInfo().getNodeId())
.isEqualTo(localId);
Assertions.assertThat(peerFuture.get(5L, TimeUnit.SECONDS).getPeerInfo().getNodeId())
Assertions.assertThat(peerFuture.get(30L, TimeUnit.SECONDS).getPeerInfo().getNodeId())
.isEqualTo(localId);
assertThat(reasonFuture.get(5L, TimeUnit.SECONDS))
assertThat(reasonFuture.get(30L, TimeUnit.SECONDS))
.isEqualByComparingTo(DisconnectReason.UNKNOWN);
Comment on lines 257 to 262
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new 30s timeout is a duplicated magic number and is likely to be repeated across multiple tests. Consider introducing a named constant (e.g., DISCONNECT_AWAIT_TIMEOUT_SECONDS) and adding a short comment explaining why it must be higher under CI contention. This makes future tuning safer and avoids inconsistencies between similar tests.

Copilot uses AI. Check for mistakes.
}
}
Expand Down Expand Up @@ -307,9 +307,9 @@ public void rejectIncomingConnectionFromDisallowedPeer() throws Exception {
final Bytes localId = localEnode.getNodeId();
Assertions.assertThat(connectFuture.get(5L, TimeUnit.SECONDS).getPeerInfo().getNodeId())
.isEqualTo(localId);
Assertions.assertThat(peerFuture.get(5L, TimeUnit.SECONDS).getPeerInfo().getNodeId())
Assertions.assertThat(peerFuture.get(30L, TimeUnit.SECONDS).getPeerInfo().getNodeId())
.isEqualTo(localId);
assertThat(reasonFuture.get(5L, TimeUnit.SECONDS))
assertThat(reasonFuture.get(30L, TimeUnit.SECONDS))
.isEqualByComparingTo(DisconnectReason.UNKNOWN);
Comment on lines 308 to 313
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Increasing timeouts in tests can significantly extend wall-clock time when something actually regresses (each failure can now stall an extra ~25s). To limit CI impact, consider using a shared constant plus a shorter default overridden only for these known-flaky disconnect awaits, or ensuring these longer awaits are only applied to the specific futures that are proven to be delayed.

Copilot uses AI. Check for mistakes.
}
}
Expand Down
Loading