tests: retry client-routes POST on transient 5xx after decommission

[mykaul]

Summary

test_should_survive_full_node_replacement_through_nlb decommissions the original nodes and then immediately POSTs to a surviving node's REST API (/v2/client-routes). decommission() returning doesn't guarantee the surviving node's gossip/topology state and REST subsystem have converged yet, so the POST can transiently hit an HTTP 500 while topology settles — this has been failing intermittently in CI across many unrelated PRs.

This is a distinct bug from #948/#949, which was a TcpProxy socket-lifecycle race in the test's own TCP-forwarding threads (confirmed by reviewing #949's diff — no REST/HTTP code was touched there).

Fix

post_client_routes() now retries up to 5 attempts with a 1s backoff, but only for 5xx responses — 4xx or other errors still raise immediately so real bugs aren't masked. Also logs the response body on failure (previously discarded), which the issue flagged as blocking diagnosis of past failures.

Considered reusing the existing wait_until_not_raised test helper instead of a dedicated loop, but its bare except: swallows any exception (including 4xx) and retries it too, only surfacing whatever the last attempt raises — that would silently retry real client errors instead of failing fast. A loop scoped to 5xx-only is the correct fix, not just a shorter one.

Testing

No live Scylla/CCM cluster available to run the integration test end-to-end here. Verified: the file collects cleanly (pytest tests/integration/standard/test_client_routes.py --collect-only), and a throwaway fake-HTTP-server check confirmed the retry transparently survives 2 transient 500s then succeeds, while a persistent 500 still raises after 5 attempts.

Fixes #931

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

Next included review available in 53 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Advanced

Run ID: dc40a545-9091-45e0-bdf5-78651f45c367

📥 Commits

Reviewing files that changed from the base of the PR and between aa1915d and c3ca6c1.

📒 Files selected for processing (1)

• tests/integration/standard/test_client_routes.py

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-scylladb]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Non-5xx errors receive unnecessary retries 🐞 Bug ≡ Correctness

Description

The retry predicate accepts every HTTP error code at or above 500 instead of only codes in the
500–599 range. A non-standard status such as 600 therefore waits and retries four times before
failing, contrary to the helper's documented fail-fast behavior for errors outside 5xx.

Code

tests/integration/standard/test_client_routes.py[258]

+            if e.code >= 500 and attempt < max_attempts:

Relevance

●●● Strong

The documented 5xx-only retry behavior requires an upper bound; status 600 should fail immediately.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The helper explicitly documents that only 5xx responses should be retried, but the changed predicate
has no upper bound and controls the retry continuation.

tests/integration/standard/test_client_routes.py[237-240]
tests/integration/standard/test_client_routes.py[256-263]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

Issue description
The retry condition uses `e.code >= 500`, so HTTP error codes above 599 are retried even though the helper is intended to retry only 5xx responses.

Fix Focus Areas
- tests/integration/standard/test_client_routes.py[258-258]

Recommended Fix
Change the predicate to `500 <= e.code < 600` so only standard 5xx responses are retried; preserve immediate failure for 4xx and other status codes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Repeated route posts leak HTTP responses 🐞 Bug ☼ Reliability

Description

The success path stores the object returned by urlopen() but returns without closing it, while the
new HTTP-error path reads the error body without closing that response either. Calls made repeatedly
during route updates can consequently retain HTTP connections or file descriptors until garbage
collection, making the test process less reliable under repeated retries and updates.

Code

tests/integration/standard/test_client_routes.py[R253-255]

+            response = urllib.request.urlopen(req)
+            log.info("Routes posted successfully (status %d)", response.status)
+            return

Relevance

●●● Strong

Explicitly closing both success and error responses prevents connection and descriptor leaks during
repeated retries.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The changed loop introduces multiple request attempts, but the success branch returns immediately
after logging and the error branch reads the body without an explicit close. The helper is invoked
repeatedly by the integration test's route-update and node-replacement flows.

tests/integration/standard/test_client_routes.py[241-265]
tests/integration/standard/test_client_routes.py[1066-1110]
tests/integration/standard/test_client_routes.py[1143-1148]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

Issue description
`post_client_routes()` does not explicitly close successful responses or `HTTPError` response objects, and the new retry loop can create several response objects per invocation.

Fix Focus Areas
- tests/integration/standard/test_client_routes.py[253-265]

Recommended Fix
Use a context manager around each successful `urlopen()` response and close `HTTPError` objects after reading their bodies, preferably with cleanup in a `finally` block so both retry and terminal-error paths release resources.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Cross-repo context — repo relationships

Review mode: 🚀 Fast: This is a localized, self-contained retry-loop change in one test helper, avoiding production, security, and contract-critical paths.

Tip of the day

💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-scylladb]

1. Non-5xx errors receive unnecessary retries 🐞 Bug ≡ Correctness

The retry predicate accepts every HTTP error code at or above 500 instead of only codes in the
500–599 range. A non-standard status such as 600 therefore waits and retries four times before
failing, contrary to the helper's documented fail-fast behavior for errors outside 5xx.

Agent Prompt

Issue description
The retry condition uses `e.code >= 500`, so HTTP error codes above 599 are retried even though the helper is intended to retry only 5xx responses.

Fix Focus Areas
- tests/integration/standard/test_client_routes.py[258-258]

Recommended Fix
Change the predicate to `500 <= e.code < 600` so only standard 5xx responses are retried; preserve immediate failure for 4xx and other status codes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-scylladb]

2. Repeated route posts leak http responses 🐞 Bug ☼ Reliability

The success path stores the object returned by urlopen() but returns without closing it, while the
new HTTP-error path reads the error body without closing that response either. Calls made repeatedly
during route updates can consequently retain HTTP connections or file descriptors until garbage
collection, making the test process less reliable under repeated retries and updates.

Agent Prompt

Issue description
`post_client_routes()` does not explicitly close successful responses or `HTTPError` response objects, and the new retry loop can create several response objects per invocation.

Fix Focus Areas
- tests/integration/standard/test_client_routes.py[253-265]

Recommended Fix
Use a context manager around each successful `urlopen()` response and close `HTTPError` objects after reading their bodies, preferably with cleanup in a `finally` block so both retry and terminal-error paths release resources.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools