test(distributed): warm up cluster per class and report cluster state on latch timeout - #224
Conversation
…state on latch timeout Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Test run: full core suite (Devin)
Flake count: 14 → 8 vs. the pre-change baseline measured on the same machine
All 8 remaining flakes passed on the first retry. Caveat: single sample per branch. New diagnostics are doing their jobBoth nodes already show all 3 clients in the room in-cluster while the client latch is short by 1 → replication completed; the gap is on the client ack side. That attribution was impossible before. Warm-up runs once per class + costTemporary marker print in Not covered by the new snapshot: |
|
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughChangesDistributed test stability
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DistributedCommonTest
participant SocketA
participant SocketB
participant ClusterNodes
participant ReplicationLatch
DistributedCommonTest->>SocketA: connect
DistributedCommonTest->>SocketB: connect
DistributedCommonTest->>ClusterNodes: join shared warm-up room
ClusterNodes->>ReplicationLatch: signal room membership replication
DistributedCommonTest->>ReplicationLatch: await warm-up completion
DistributedCommonTest->>SocketA: disconnect
DistributedCommonTest->>SocketB: disconnect
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedCommonTest.java`:
- Around line 138-143: Update the warm-up flow in DistributedCommonTest around
awaitOrFail and awaitRoomSync to establish a single deadline using
WARMUP_TIMEOUT_SECS before connecting, joining, and synchronizing. Pass each
phase only its remaining time, and preserve failure behavior when the deadline
is exhausted so the entire warm-up remains within the documented 120-second
budget.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc5e46f8-d6e6-4bee-b8a9-942c4ce07880
📒 Files selected for processing (1)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedCommonTest.java
| awaitOrFail(connectLatch, WARMUP_TIMEOUT_SECS, "Warm-up clients failed to connect"); | ||
|
|
||
| a.emit("join-room", room); | ||
| b.emit("join-room", room); | ||
| awaitOrFail(joinLatch, WARMUP_TIMEOUT_SECS, "Warm-up clients failed to join room"); | ||
| awaitRoomSync(room, 2); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Enforce the 120-second budget end-to-end.
Each latch can wait 120 seconds, and awaitRoomSync() adds another independent 120-second wait. A slow successful warm-up can therefore take nearly six minutes, not the documented 120 seconds. Create one deadline and pass its remaining time to each phase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedCommonTest.java`
around lines 138 - 143, Update the warm-up flow in DistributedCommonTest around
awaitOrFail and awaitRoomSync to establish a single deadline using
WARMUP_TIMEOUT_SECS before connecting, joining, and synchronizing. Pass each
phase only its remaining time, and preserve failure behavior when the deadline
is exhausted so the entire warm-up remains within the documented 120-second
budget.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
Description
The
Distributed*suites flake under load (14 retried flakes in a recent full run, all in the container-heavy Hazelcast/Kafka/NATS classes; Redis never flaked). Two problems make that hard to act on:@BeforeAlland immediately runs a test, so Kafka partition discovery / Hazelcast cluster formation / NATS dispatcher setup eats into that test's 30sOP_TIMEOUT_SECSlatch."Clients failed to join room"doesn't distinguish "client never gotjoin-ok" from "membership never replicated to the peer node", so every flake is unattributable.Both are fixed in the shared base class only; no test semantics or timeouts change.
Type of Change
Changes Made
@BeforeEach warmUpCluster()inDistributedCommonTestperforms one connect → join →awaitRoomSyncround trip per class (guarded by anAtomicBoolean; all subclasses arePER_CLASS), with its own 120s budget so bootstrap cost sits outside the per-test latches.awaitOrFailis now an instance method that appends a cluster snapshot to every timeout failure:latch remainingshows how far the operation got; the per-node room maps show whether replication lagged. Snapshot collection is exception-safe and only runs on the failure path.Testing
mvn testmvn -pl netty-socketio-core test -Dtest=DistributedKafkaMultiChannelMemoryTest,DistributedNATSSingleChannelMemoryTest→ 24/24 green. Kafka class went 17.4s → 18.4s, i.e. the warm-up adds ~1s per class, not per test.mvn -pl netty-socketio-core verify -DskipTests(checkstyle + license) passes.Additional Notes
This is deliberately diagnostic-first: it does not raise
OP_TIMEOUT_SECS(already 30s, withawaitRoomSyncat 2 min — a timeout there means something never arrived, not that it arrived late). If flakes persist after this, the failure output will name the culprit instead of us guessing. Independent of #220 — branched frommain.Link to Devin session: https://app.devin.ai/sessions/78bf7322d0ff4d139f1f0c157954bfad
Requested by: @sanjomo
Summary by CodeRabbit