feat: retry transient failures during upstream sync#951
Conversation
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughA new ChangesUpstream fetch retry logic
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
🧹 Nitpick comments (3)
application/tests/cre_main_test.py (1)
474-490: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only covers one of several retry branches.
This exercises the
RequestException-retry path, butfetch_upstream_jsonhas two other decision branches worth covering: immediate raise on non-retryable status (e.g. 404), and the retryable 5xx/429 status path plus final-exhaustionRuntimeError. Given this helper centralizes retry logic for both upstream call sites, covering these branches would catch regressions in the retry/raise decision logic.🤖 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 `@application/tests/cre_main_test.py` around lines 474 - 490, The fetch_upstream_json tests only cover the RequestException retry path, so add coverage for the other retry branches in application.cmd.cre_main.fetch_upstream_json: a non-retryable response like 404 should fail immediately without sleeping, and a retryable 5xx/429 response should retry until max attempts then raise RuntimeError after exhausting retries. Use the existing test class in cre_main_test.py and mock requests.get/time.sleep to verify the call count, sleep behavior, and final exception handling for these branches.application/cmd/cre_main.py (2)
41-84: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider honoring
Retry-Afteron 429 responses.The retry loop treats 429 the same as 5xx, sleeping
backoff_seconds * attemptregardless of what upstream tells you to wait. Respecting aRetry-Afterheader (when present) avoids hammering a rate-limited upstream and is standard practice for 429 handling.♻️ Suggested tweak
status_error = RuntimeError( f"cannot connect to upstream status code {response.status_code}" ) if response.status_code < 500 and response.status_code != 429: raise status_error last_error = status_error + if response.status_code == 429: + retry_after = response.headers.get("Retry-After") + if retry_after: + try: + backoff_seconds = max(backoff_seconds, float(retry_after)) + except ValueError: + pass🤖 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 `@application/cmd/cre_main.py` around lines 41 - 84, The retry logic in fetch_upstream_json currently ignores upstream rate-limit guidance for 429 responses and always sleeps using the local backoff. Update the retry path in fetch_upstream_json to detect a 429 response, read and honor the Retry-After header when present, and fall back to the existing backoff_seconds * attempt behavior only when the header is missing or invalid. Keep the existing retry and error handling structure intact.
637-648: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant fetch before cache check is now amplified by retries.
download_cre_from_upstreamcallsfetch_upstream_jsonbefore checkingif cre.id in imported_cres, so every already-imported node still triggers a network call — and now, on any transient failure, up tomax_attemptsretries with backoff before the result is discarded. Checking the cache before fetching would avoid this cost.♻️ Suggested fix
def download_cre_from_upstream(creid: str): + if creid in imported_cres: + return data = fetch_upstream_json(f"/id/{creid}") credict = data["data"] cre = defs.Document.from_dict(credict) if cre.id in imported_cres: return🤖 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 `@application/cmd/cre_main.py` around lines 637 - 648, `download_cre_from_upstream` is doing the upstream fetch before checking the `imported_cres` cache, so already-imported CREs still incur network calls and retries. Move the `if creid in imported_cres` check to the start of `download_cre_from_upstream` and return early before calling `fetch_upstream_json`; keep the rest of the flow (`defs.Document.from_dict`, `register_cre`, and recursive link traversal) unchanged.
🤖 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.
Nitpick comments:
In `@application/cmd/cre_main.py`:
- Around line 41-84: The retry logic in fetch_upstream_json currently ignores
upstream rate-limit guidance for 429 responses and always sleeps using the local
backoff. Update the retry path in fetch_upstream_json to detect a 429 response,
read and honor the Retry-After header when present, and fall back to the
existing backoff_seconds * attempt behavior only when the header is missing or
invalid. Keep the existing retry and error handling structure intact.
- Around line 637-648: `download_cre_from_upstream` is doing the upstream fetch
before checking the `imported_cres` cache, so already-imported CREs still incur
network calls and retries. Move the `if creid in imported_cres` check to the
start of `download_cre_from_upstream` and return early before calling
`fetch_upstream_json`; keep the rest of the flow (`defs.Document.from_dict`,
`register_cre`, and recursive link traversal) unchanged.
In `@application/tests/cre_main_test.py`:
- Around line 474-490: The fetch_upstream_json tests only cover the
RequestException retry path, so add coverage for the other retry branches in
application.cmd.cre_main.fetch_upstream_json: a non-retryable response like 404
should fail immediately without sleeping, and a retryable 5xx/429 response
should retry until max attempts then raise RuntimeError after exhausting
retries. Use the existing test class in cre_main_test.py and mock
requests.get/time.sleep to verify the call count, sleep behavior, and final
exception handling for these branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ab28980a-c90f-4911-b2da-5f1f9a498caf
📒 Files selected for processing (2)
application/cmd/cre_main.pyapplication/tests/cre_main_test.py
Bornunique911
force-pushed
the
feat/upstream-sync-retry
branch
from
0642fa6 to
a112bc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
application/tests/cre_main_test.py (1)
474-490: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStatic analysis "no timeout" hints are false positives.
These flag
@patch(...)decorator lines as external calls without a timeout, but they're mocking test doubles, not live network calls — no action needed there.Separately,
test_fetch_upstream_json_retries_transient_failuresdoesn't pass explicitmax_attempts/backoff_seconds, relying onfetch_upstream_json's environment-variable-driven defaults (CRE_UPSTREAM_MAX_ATTEMPTS,CRE_UPSTREAM_RETRY_BACKOFF_SECONDS). If these env vars are ever set differently in CI/dev, the test's retry expectation could silently break.♻️ Suggested fix for determinism
- data = main.fetch_upstream_json("/root_cres") + data = main.fetch_upstream_json( + "/root_cres", max_attempts=3, backoff_seconds=2 + )🤖 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 `@application/tests/cre_main_test.py` around lines 474 - 490, The retry test for fetch_upstream_json is nondeterministic because it relies on environment-driven defaults for max_attempts and backoff_seconds. Update test_fetch_upstream_json_retries_transient_failures in application.tests.cre_main_test to pass explicit retry settings to fetch_upstream_json, or otherwise isolate the CRE_UPSTREAM_MAX_ATTEMPTS and CRE_UPSTREAM_RETRY_BACKOFF_SECONDS values so the assertion on retry count and sleep remains stable. Keep the existing mocks for requests.get and time.sleep, but make the test independent of external env configuration.
🤖 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 `@application/cmd/cre_main.py`:
- Around line 56-90: The retry loop in the upstream fetch logic is reusing a
stale response across attempts because it relies on a locals() check. Reset
response at the start of each iteration in the fetch function and only inspect
Retry-After when the current requests.get call actually returned a 429 response.
Update the retry/backoff handling around the for attempt loop, response, and
last_error so each attempt uses only its own result and does not inherit headers
from a previous attempt.
---
Nitpick comments:
In `@application/tests/cre_main_test.py`:
- Around line 474-490: The retry test for fetch_upstream_json is
nondeterministic because it relies on environment-driven defaults for
max_attempts and backoff_seconds. Update
test_fetch_upstream_json_retries_transient_failures in
application.tests.cre_main_test to pass explicit retry settings to
fetch_upstream_json, or otherwise isolate the CRE_UPSTREAM_MAX_ATTEMPTS and
CRE_UPSTREAM_RETRY_BACKOFF_SECONDS values so the assertion on retry count and
sleep remains stable. Keep the existing mocks for requests.get and time.sleep,
but make the test independent of external env configuration.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3173da84-d9a8-4323-a8dc-4d0ff8d8261b
📒 Files selected for processing (2)
application/cmd/cre_main.pyapplication/tests/cre_main_test.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Bornunique911 <69379200+Bornunique911@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
LGTM. The fetch_upstream_json helper cleanly centralizes retry logic with sensible defaults (timeout, linear backoff, Retry-After on 429, fail-fast on non-retryable 4xx). Early imported_cres check avoids redundant fetches, and the four unit tests cover the main branches. CI green.
2 participants
Summary
This PR is split out from #900 to make review smaller and more focused.
It adds retry handling for transient upstream failures during upstream sync by introducing a focused upstream fetch helper and updating upstream graph download logic to reuse that helper. The intent is to let reviewers assess the retry/resilience behavior independently from fixture/importer follow-up work.
Issue reference:
Problem Fixed
PR #900 grouped multiple different concerns into a single larger review, including:
That made it harder to review the upstream retry behavior on its own.
For this part of the work, the useful standalone contribution is:
--upstream_syncmore resilient to transient failuresSolution
This PR adds:
fetch_upstream_json(...)helper inapplication/cmd/cre_main.pyRetry-Aftersupport for429responsesThis PR updates upstream graph download code to use the helper for:
/root_cres/id/<creid>Files in scope:
application/cmd/cre_main.pyapplication/tests/cre_main_test.pyTests