hiero-ledger / hiero-ledger/hiero-sdk-cpp
[Intermediate]: Fix flaky `NetworkIntegrationTests.NodeSelectionPrioritizesHealthyNodes` caused by node-selection state drift
- Dominant language
- C++
- Stars
- 42
- Forks
- 108
- Avg merge
- 11h 45m
- Merged PRs (30d)
- 2
Description
### 🧩 Intermediate
This issue is suited for contributors who are comfortable navigating the **Hiero C++ SDK** codebase and ready to own a solution end-to-end.
Intermediate issues involve investigation, reasoning about trade-offs, and choosing between implementation approaches — not just following step-by-step instructions.
> [!IMPORTANT]
> ### 📋 About Intermediate Issues
>
> Intermediate Issues encourage deeper problem-solving and technical ownership.
>
> They often:
> - Span multiple related files or components
> - Involve investigating existing behavior before changing it
> - Leave room for contributor judgment and implementation decisions
> - Have more than one reasonable solution
>
> Contributors are expected to explain their approach in the pull request and be open to discussion during review.
---
### 👾 Description of the Issue
`NetworkIntegrationTests.NodeSelectionPrioritizesHealthyNodes` is the only integration test that exercises multi-node selection (every other integration test uses the single-node Solo client from `config/local_node.json`). It fails intermittently on PR CI — almost always on the **Release** CTest pass, after the Debug pass has already finished — and it is consistently the *only* test that fails when this happens.
The flake is not random environment noise. It is a structural mismatch between what the test asserts and how the SDK's node-selection state machine behaves. There are four distinct issues that compound:
**1. `Executable` increments per-node backoff but never updates `BaseNetwork::mHealthyNodes`.**
`src/sdk/main/src/Executable.cc` calls `node->increaseBackoff()` directly (lines 209, 233, 281), which only updates `BaseNode::mReadmitTime`. It does **not** call `BaseNetwork::increaseBackoff(node)`, which is the function that does `mHealthyNodes.erase(node)` (`src/sdk/main/src/impl/BaseNetwork.cc:98-103`). Result: after a node fails, `BaseNode::isHealthy()` correctly returns `false`, but the network-level `mHealthyNodes` set still contains it. Every subsequent query that calls `getNumberOfMostHealthyNodes` therefore re-includes the bad node in its candidate list (`src/sdk/main/src/impl/BaseNetwork.cc:202-245`), undermining the test's premise that the bad node will be deprioritized after one failure.
**2. `getNodeIndexForExecute` does not wrap around when scanning for a healthy node.**
`src/sdk/main/src/Executable.cc:580` reads:
```cpp
for (unsigned int i = attempt % nodes.size(); i < nodes.size(); ++i)
```
This iterates from `attempt % size` to the end, but never wraps. With two nodes and `attempt = 1`, the loop only inspects index 1. If the unhealthy node happens to be at index 1, the function returns it (as the candidate-with-shortest-backoff fallback) and the executable then `sleep_for(getRemainingTimeForBackoff())` and retries the same dead node, even though a healthy node exists at index 0. Because the candidate list returned by `getNodeAccountIdsForExecute` is built by walking an `unordered_set` with `Utilities::getRandomNumber`, the order of the resulting `vector` is non-deterministic across runs — so this trap is hit unpredictably.
**3. The "fake" port `127.0.0.1:99999` has undefined gRPC behavior.**
`src/sdk/tests/integration/NetworkIntegrationTests.cc:73` uses `127.0.0.1:99999`. Port `99999` exceeds the 16-bit TCP port range, but `BaseNodeAddress::fromString` (`src/sdk/main/src/impl/BaseNodeAddress.cc:36`) accepts it as `unsigned int 99999` without range validation. gRPC C++ behavior on an out-of-range port varies by version and platform: some versions reject resolution and `WaitForConnected` returns false at the full 10-second `GET_STATE_TIMEOUT` (`src/sdk/main/include/impl/BaseNode.h:175`); others may briefly report the channel as connectable, causing `BaseNode::channelFailedToConnect()` to return `false` and the executable to spend the full gRPC deadline waiting on a dead socket before the request times out. Release-mode timing makes the latter case more likely, which is the dominant reason this is the *Release* job that fails.
**4. Solo is shared between the Debug and Release CTest passes, run sequentially.**
The `Code / Build (Linux, linux-x64)` job runs `Start CTest suite (Debug)` first (~30 minutes of integration load against the single Solo node), then `Start CTest suite (Release)`. By the time the Release pass runs this test, Solo has accumulated state and is slower to respond. The good-node response is closer to the request deadline, leaving less margin to absorb the bad-node detours from items 1–3.
The test passes locally and on most CI runs because all four issues are probabilistic. When ordering, gRPC state, and Solo load align unfavorably — most often on the Release pass after Debug has warmed up Solo — the test is the canary that fails. No other integration test is affected because no other integration test introduces a deliberately bad node into a multi-node `Network`.
Relevant files:
```
src/sdk/main/src/Executable.cc
src/sdk/main/src/impl/BaseNetwork.cc
src/sdk/main/include/impl/BaseNetwork.h
src/sdk/main/src/impl/BaseNode.cc
src/sdk/main/include/impl/BaseNode.h
src/sdk/main/src/impl/BaseNodeAddress.cc
src/sdk/tests/integration/NetworkIntegrationTests.cc
```
### 🔁 Steps to Reproduce
The flake reproduces against any Solo-backed CI job that runs both the Debug and Release CTest passes sequentially. A representative failure on a recent PR:
1. Open or push to a PR against `main` and let `Code / Build (Linux, linux-x64)` run.
2. Observe that `Start CTest suite (Debug)` succeeds.
3. Observe that `Start CTest suite (Release)` fails with:
```
The following tests FAILED:
1809 - NetworkIntegrationTests.NodeSelectionPrioritizesHealthyNodes (Failed)
```
4. Re-run the failed job — the test typically passes on retry.
Reference failure: PR #1594, run `25568179613`, job `75056885870` (`NetworkIntegrationTests.NodeSelectionPrioritizesHealthyNodes` failed in Release; PR contents are unrelated to network code).
To reproduce locally with higher hit rate, configure both Debug and Release builds with `BUILD_TESTS=ON`, run the full Debug CTest suite first against a freshly started Solo network, then run the Release suite without restarting Solo:
```bash
cmake --preset linux-x64-debug -DBUILD_TESTS=ON
cmake --build -j 6 --preset linux-x64-debug
cmake --preset linux-x64-release -DBUILD_TESTS=ON
cmake --build -j 6 --preset linux-x64-release
# Start Solo once, leave it running across both ctest runs.
ctest -j 6 -C Debug --test-dir build/linux-x64-debug -E NodeUpdateTransactionIntegrationTests
ctest -j 6 -C Release --test-dir build/linux-x64-release -R NodeSelectionPrioritizesHealthyNodes --repeat until-fail:20
```
### ✅ Expected Behavior
After a node fails to connect or returns a non-retryable error:
- `BaseNetwork::mHealthyNodes` no longer contains that node until its readmit time elapses.
- `getNumberOfMostHealthyNodes` returns only currently-healthy nodes (or sleeps until at least one is healthy when none are).
- `getNodeIndexForExecute` examines every node in the candidate list — wrapping past the start index when needed — before falling back to "least-bad unhealthy candidate."
- The integration test's three sequential `AccountBalanceQuery` calls all succeed deterministically, regardless of `unordered_map` iteration order, gRPC channel-state timing, or whether the Debug pass ran before the Release pass.
### ❌ Actual Behavior
- `BaseNetwork::mHealthyNodes` retains the bad node forever after `node->increaseBackoff()` is called from `Executable.cc`.
- Subsequent queries re-roll the same bad node into their candidate list.
- `getNodeIndexForExecute` can isolate the bad node on a given attempt because the iteration starts at `attempt % size` and never wraps, returning the bad node as a "least-bad" candidate even when a healthy node exists.
- On Release, gRPC's faster state-machine transitions occasionally let `channelFailedToConnect()` return `false` for `127.0.0.1:99999`, so the request actually waits the full gRPC deadline against a dead socket.
- The cumulative effect drains the 10-attempt budget against the 2-minute request timeout, and the second or third `EXPECT_NO_THROW` in the test throws `MaxAttemptsExceededException`.
Representative tail of the failing CTest log:
```
99% tests passed, 1 tests failed out of 2083
The following tests FAILED:
1809 - NetworkIntegrationTests.NodeSelectionPrioritizesHealthyNodes (Failed)
Errors while running CTest
##[error]Process completed with exit code 8.
```
### 🌐 Environment
- SDK version: `main` (reproduced through `v0.55.0`)
- Operating system: Ubuntu (GitHub-hosted `ubuntu-latest` runner)
- Compiler and version: GCC (default on `ubuntu-latest`), reproducible under Clang 17 locally
- Hiero network: Solo (local kind-based Hiero network used by integration tests)
- Build presets affected: `linux-x64-release` (most common), occasionally `linux-x64-debug` under heavy parallel load
### ✔️ Acceptance Criteria
- [ ] `Executable` calls the **network-level** `increaseBackoff(node)` (the `BaseNetwork` overload that erases the node from `mHealthyNodes`) at all three failure sites in `Executable.cc` (channel-connect failure, retryable gRPC status, `RETRY_WITH_ANOTHER_NODE`). Per-node `BaseNode::increaseBackoff()` is no longer called directly from `Executable.cc`.
- [ ] `Executable::getNodeIndexForExecute` wraps around the candidate list so every node is inspected exactly once before the function falls back to the least-bad unhealthy candidate. The fix is documented inline with a one-line comment explaining the wrap.
- [ ] `BaseNodeAddress::fromString` rejects ports outside the valid TCP range (1–65535) with a clear `std::invalid_argument`. Existing valid uses are unaffected.
- [ ] `NetworkIntegrationTests.NodeSelectionPrioritizesHealthyNodes` is rewritten to:
- Use a syntactically valid but reliably refused address (e.g. `127.0.0.1:1`) for the bad node.
- Pick the "good" node deterministically (e.g. `std::min_element` by `AccountId`) instead of `*originalNetwork.begin()`.
- Pin the client's retry budget with `setMaxAttempts`, `setRequestTimeout`, and `setGrpcDeadline` so a single bad-node attempt cannot consume the entire request window.
- [ ] A new unit test (no Solo required) covers `Executable::getNodeIndexForExecute` wrap-around and the `BaseNetwork::increaseBackoff`/`mHealthyNodes` invariant directly, so future regressions surface without depending on a live network.
- [ ] The full `ctest` suite (Debug and Release) passes, including running `NodeSelectionPrioritizesHealthyNodes` 20 times in a row via `--repeat until-fail:20` against a Debug-warmed Solo.
- [ ] No unrelated behavior or public API changes are introduced.
- [ ] Existing tests continue to pass.
### 🤔 Additional Information
**Suggested implementation outline:**
1. In `src/sdk/main/src/Executable.cc`, replace the three direct `node->increaseBackoff()` calls (lines 209, 233, 281) with `client.getClientNetwork()->increaseBackoff(node)`. The `Client` is already in scope as the `client` parameter.
2. In `src/sdk/main/src/Executable.cc:580`, change `getNodeIndexForExecute` to:
```cpp
const std::size_t size = nodes.size();
for (std::size_t k = 0; k < size; ++k)
{
const std::size_t i = (static_cast(attempt) + k) % size;
// existing healthy / candidate logic
}
```
3. In `src/sdk/main/src/impl/BaseNodeAddress.cc:36`, after the `from_chars` parse, add a range check `if (port == 0 || port > 65535)` and throw `std::invalid_argument`.
4. In `src/sdk/tests/integration/NetworkIntegrationTests.cc:63-89`, swap the bad address and the "good node" pick as described in the acceptance criteria, and call `setMaxAttempts(2).setRequestTimeout(30s).setGrpcDeadline(2s)` on the custom `Client`.
5. Add a new unit test file (e.g. `src/sdk/tests/unit/NetworkNodeSelectionUnitTests.cc`) that constructs a `BaseNetwork`/`Network` with stubbed `Node`s and asserts:
- `BaseNetwork::increaseBackoff(node)` removes the node from `mHealthyNodes`.
- `getNodeIndexForExecute` selects a healthy node at index 0 when `attempt = 1` and the unhealthy node is at index 1 (and vice versa).
**Why this is *Intermediate* and not *Beginner* or *Advanced*:**
The fix is multi-file and requires understanding the interaction between `BaseNode`, `BaseNetwork`, and `Executable`, plus the `unordered_set` ordering and gRPC channel-state semantics. There are several reasonable refactors (e.g. centralizing all backoff updates on `BaseNetwork` and making `BaseNode::increaseBackoff` private; introducing an explicit "node-selection result" struct), and the contributor will need to choose one and defend it in review. The goals, surface area, and acceptance criteria are concrete enough that this does not require system-wide redesign or protocol-level work, so it sits squarely in the Intermediate band rather than Advanced.
**Background reference:** the failing test was added in PR #1273 (commit `a1a0ba4`). The PR description notes the author was unable to set up a multi-node Solo deployment, so the test as merged was never validated against the multi-node code path it claims to exercise.
---
### 📋 Step-by-Step Contribution Guide
To help keep contributions consistent and easy to review, we recommend following these steps:
- [ ] Comment `/assign` to request the issue
- [ ] Wait for assignment
- [ ] Fork the repository and create a branch
- [ ] Set up the project using the instructions in `README.md`
- [ ] Make the requested changes
- [ ] Sign each commit using `-s -S`
- [ ] Push your branch and open a pull request
Read [Workflow Guide](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/docs/training/workflow.md) for step-by-step workflow guidance.
Read [README.md](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/README.md) for setup instructions.
❗ Pull requests **cannot be merged** without `S` and `s` signed commits.
See the [Signing Guide](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/docs/training/signing.md).
### 🤔 Additional Information
If you have questions while working on this issue, feel free to ask!
You can reach the community and maintainers here: [Hiero-SDK-C++ Discord](https://discord.com/channels/905194001349627914/1337424839761465364)
Whether you need help understanding the existing node-selection code, the gRPC channel-state semantics, or confirming your implementation approach — we're happy to help.
Contributor guide
Research direction
Start with the failure sites in src/sdk/main/src/Executable.cc and the node-selection logic around getNodeIndexForExecute, then read BaseNetwork.cc, BaseNodeAddress.cc, and NetworkIntegrationTests.cc. Run the targeted integration test and inspect the existing network and node tests before making changes. Done means the requested unit coverage exists, the integration test is deterministic, and the Debug and Release suites pass, including the 20-repeat run.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cmake, cpp, grpc
- Domain
- networking, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 45/100