IT: fight flakiness in C++ integration test_metrics - #490
Conversation
The CI suddenly started failing the test. I added a short sleep loop to recover if a suspected race occurs.
📝 WalkthroughWalkthroughThe metrics integration test adds platform-specific headers for sleeping. After restarting a node, it polls refreshed connection metrics for up to 50 iterations. The test sleeps for 1 ms between attempts and asserts that two connections are restored. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds retry polling to reduce reconnection-related flakiness in the metrics integration test.
Changes:
- Polls connection metrics after restarting a node.
- Adds platform-specific system headers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (unsigned i = 0; i < 50; ++i) { | ||
| metrics = session.metrics(); | ||
| if (metrics.stats.total_connections == 2) { | ||
| break; | ||
| } | ||
| msleep(1); | ||
| } |
![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
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
tests/src/integration/tests/test_metrics.cpp-19-24 (1)
19-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSplit the patch into logically separate commits.
Keep the platform-specific header change separate from the metrics polling change. They address different concerns.
As per coding guidelines,
**/*: Split the patch into logically separate commits.Also applies to: 64-72
🤖 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 `@tests/src/integration/tests/test_metrics.cpp` around lines 19 - 24, Separate the platform-specific header update around the _WIN32 include block from the metrics polling changes into distinct commits. Keep the header change isolated, and place the polling-related modifications in a separate commit without combining the two concerns.Source: Coding guidelines
🤖 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 `@tests/src/integration/tests/test_metrics.cpp`:
- Around line 64-72: Replace the fixed 50-iteration loop around
session.metrics() with a bounded wall-clock reconnect deadline, polling until
total_connections reaches 2 or the deadline expires. After the polling wait,
take one final metrics sample before the assertion so the latest state is
evaluated.
---
Other comments:
In `@tests/src/integration/tests/test_metrics.cpp`:
- Around line 19-24: Separate the platform-specific header update around the
_WIN32 include block from the metrics polling changes into distinct commits.
Keep the header change isolated, and place the polling-related modifications in
a separate commit without combining the two concerns.
🪄 Autofix
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: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 403dcdee-4b93-4108-8d40-326d8d32c3af
📒 Files selected for processing (1)
tests/src/integration/tests/test_metrics.cpp
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
scylladb/scylladb(auto-detected)
| // `start_node()` finish may race with driver's reconnecting to the node. | ||
| // Let's give the driver more time to reconnect to fight flakiness. | ||
| for (unsigned i = 0; i < 50; ++i) { | ||
| metrics = session.metrics(); | ||
| if (metrics.stats.total_connections == 2) { | ||
| break; | ||
| } | ||
| msleep(1); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 '\bstart_node\s*\(|\bwith_constant_reconnect\s*\(|\bmsleep\s*\(|\bwait_for_port\s*\(' \
tests/src/integrationRepository: scylladb/cpp-rs-driver
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test_metrics.cpp ---'
cat -n tests/src/integration/tests/test_metrics.cpp | sed -n '35,80p'
printf '%s\n' '--- Integration::start_node ---'
cat -n tests/src/integration/integration.cpp | sed -n '420,445p'
printf '%s\n' '--- CCM::Bridge::start_node ---'
cat -n tests/src/integration/ccm/bridge.cpp | sed -n '790,830p'
printf '%s\n' '--- CCM readiness helpers and timing constants ---'
rg -n -C 8 'bool CCM::Bridge::is_node_(up|down)|CCM_NAP|is_node_up\(|is_node_down\(' \
tests/src/integration/ccm/bridge.cpp tests/src/integration/ccm/bridge.hppRepository: scylladb/cpp-rs-driver
Length of output: 20913
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- reconnect configuration and scheduling ---'
rg -n -C 5 'constant_reconnect|reconnect_delay|reconnect.*delay|reconnect.*schedule|schedule.*reconnect' \
src tests | head -n 240
printf '%s\n' '--- existing bounded wait patterns ---'
rg -n -C 4 'start_timer\(\)|elapsed_time\(\)|MAXIMUM_WAIT_TIME|WAIT_FOR_NAP|wait_for_node' \
tests/src/integration | head -n 240
printf '%s\n' '--- source-level loop verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
path = Path("tests/src/integration/tests/test_metrics.cpp")
text = path.read_text()
match = re.search(
r"for \(unsigned i = 0; i < (\d+); \+\+i\) \{\s*"
r"metrics = session\.metrics\(\);\s*"
r"if \(metrics\.stats\.total_connections == 2\) \{\s*"
r"break;\s*"
r"\}\s*"
r"msleep\((\d+)\);",
text,
)
assert match, "StatsConnections polling loop not found"
iterations, sleep_ms = map(int, match.groups())
print(f"poll iterations: {iterations}")
print(f"sleep calls on an unsuccessful path: {iterations}")
print(f"intentional sleep budget on that path: {iterations * sleep_ms} ms")
print("metrics refresh after the final sleep: no")
PYRepository: scylladb/cpp-rs-driver
Length of output: 25544
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'with_constant_reconnect|wait_for_node|total_connections.*2|start_node\(1\)' \
tests | head -n 200 || trueRepository: scylladb/scylladb
Length of output: 238
Use a wall-clock reconnect deadline.
start_node() only confirms that the node accepts connections; it does not wait for the driver to reconnect. This loop provides only 50 ms of sleep time and does not refresh metrics after its final sleep. Use a bounded wall-clock wait and take a final metrics sample before the assertion.
🤖 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 `@tests/src/integration/tests/test_metrics.cpp` around lines 64 - 72, Replace
the fixed 50-iteration loop around session.metrics() with a bounded wall-clock
reconnect deadline, polling until total_connections reaches 2 or the deadline
expires. After the polling wait, take one final metrics sample before the
assertion so the latest state is evaluated.
The CI suddenly started failing the test. I added a short sleep loop to recover if a suspected race occurs.
Pre-review checklist
[ ] I have provided docstrings for the public items that I want to introduce.[ ] I have adjusted the documentation in./docs/source/.[ ] I have implemented Rust unit tests for the features/changes introduced.[ ] I have enabled appropriate tests inMakefilein{SCYLLA,CASSANDRA}_(NO_VALGRIND_)TEST_FILTER.[ ] I added appropriateFixes:annotations to PR description.