[betinacosta]

Disclaimer: Do not include SAP-internal or customer-specific information in this PR (e.g. internal system URLs, customer names, tenant IDs, or confidential configurations). This is a public repository.

Description

XsuaaAuthProvider._fetch_token previously failed immediately on any OAuth2Session.fetch_token error. This change wraps the call in a retry loop (up to 3 attempts) with exponential backoff (0.5 s, 1.0 s) so transient network errors or momentary XSUAA endpoint unavailability do not surface as hard failures. A logger.warning is emitted before each retry with the attempt number and delay, and the final RuntimeError message now includes the attempt count.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Code refactoring
• Dependency update

How to Test

1. Run the new test class: pytest tests/core/unit/test_http_client.py::TestXsuaaAuthProviderTokenFetchRetry -v
2. Confirm all five tests pass and time.sleep is never called with a real delay (it is mocked).
3. To test the retry warning log, set XSUAA_TOKEN_URL to an unreachable host and observe three warning lines followed by a RuntimeError mentioning "after 3 attempts".
4. Run the full unit suite to check for regressions: pytest tests/core/unit/test_http_client.py -v

Checklist

• I have read the Contributing Guidelines
• I have verified that my changes solve the issue
• I have added/updated automated tests to cover my changes
• All tests pass locally
• I have verified that my code follows the Code Guidelines
• I have updated documentation (if applicable)
• I have added type hints for all public APIs
• My code does not contain sensitive information (credentials, tokens, etc.)
• I have followed Conventional Commits for commit messages

[betinacosta]

PR Review: #437 — fix(http): add retry with exponential backoff for OAuth2 token fetch

Author: betinacosta Branch: fix/xsuaa-token-fetch-retry → main
Verdict: ⚠️ Needs Minor Work
Summary: Solid, well-tested retry implementation, but catches all exceptions indiscriminately — including fatal credential errors (InvalidClientError) that should not be retried — and two checklist items are left unchecked.

---

A: Process & Compliance

#	Criterion	Status	Finding
A1	PR template complete	⚠️ Warning	Two checklist items unchecked: All tests pass locally and I have verified that my code follows the Code Guidelines. The security-sensitive conditional item (*(Security-sensitive changes only)* I have run /security-review and addressed all ❌ findings) is also absent — this PR modifies the core OAuth2 token-fetch flow of XsuaaAuthProvider, which qualifies as a security-sensitive change.
A2	Conventional Commits	✅ Pass	PR title fix(http): add retry with exponential backoff for OAuth2 token fetch matches type(scope): description. Commit Validation CI job passed.
A3	Issue linked	⚠️ Warning	PR body contains Closes #6810, but issue #6810 does not exist in SAP/cloud-sdk-python (resolves to "not found"). Likely an internal tracker reference — no blocking concern, but the link won't auto-close.
A4	AI-generated code disclosure	➖ N/A	Diff is focused and mechanical; no disclosure required.

B: Security & Sensitive Data

#	Criterion	Status	Finding
B1	No sensitive data in code	✅ Pass	No credentials, tokens, internal URLs, or tenant IDs in the diff.
B2	No sensitive data in PR body	✅ Pass	PR body is clean; the disclaimer is preserved.

C: Code Quality

#	Criterion	Status	Finding
C1	CI checks passing	✅ Pass	All required jobs pass: Code Quality Checks, Tests & Coverage, Build & Package, Commit Validation, Check Version Bump, Analyze (python), CodeQL.
C2	Version bump	➖ N/A	No version change in pyproject.toml.
C3	Type hints	✅ Pass	New constants are module-level typed by value. The last_exc: Exception | None = None annotation at models.py:127 is correct. No new public API surface introduced.
C4	No hardcoded values	✅ Pass	_TOKEN_FETCH_MAX_ATTEMPTS = 3 and _TOKEN_FETCH_BACKOFF_BASE = 0.5 are module-level constants, not inline magic values.
C5	Import organization	✅ Pass	import time added to the existing stdlib block in alphabetical position. No lazy imports introduced.
C6	Naming conventions	✅ Pass	_TOKEN_FETCH_MAX_ATTEMPTS, _TOKEN_FETCH_BACKOFF_BASE follow SCREAMING_SNAKE_CASE. last_exc follows snake_case.
C7	No unused code	✅ Pass	last_exc = None after a successful break at models.py:137 is a sentinel clear required by the post-loop check — not dead code.
C8	No unjustified new dependencies	✅ Pass	import time is stdlib. No new pyproject.toml entries.
C9	Proto code freshness	➖ N/A	No .proto files changed.

D: API & Design

#	Criterion	Status	Finding
D1	API future-proofing	✅ Pass	Retry parameters are module-level constants, not inline magic values. A future RetryConfig dataclass would be the right place to put these if they become configurable, but that's not required for this fix.
D2	Public API hygiene	✅ Pass	_fetch_token is a private method; no public surface changed.
D3	Breaking changes marked	✅ Pass	No breaking changes. The error message text changed from "Failed to obtain OAuth2 token: ..." to "Failed to obtain OAuth2 token after 3 attempts: ..." — callers who match the exact string would break, but matching raw error text is not a supported API contract.
D4	Pagination & tenant filtering	➖ N/A	Not applicable.
D5	Telemetry instrumentation	➖ N/A	_fetch_token is an infrastructure method, not a user-facing client operation. No telemetry expected here.
D6	Multi-tenancy support	➖ N/A	Retry wraps the existing tenant-aware token_url derivation; multi-tenancy behavior is unchanged.

E: Tests & Documentation

#	Criterion	Status	Finding
E1	Tests added/updated	✅ Pass	TestXsuaaAuthProviderTokenFetchRetry in tests/core/unit/test_http_client.py covers five distinct scenarios: success on first attempt, retry-then-success, full exhaustion, exponential delay values, and no-sleep-after-final-attempt. time.sleep is mocked so tests are fast.
E2	Documentation quality	✅ Pass	No public API or module user-guide.md changed; docstrings are unaffected.
E2a	Multi-tenancy documentation	➖ N/A	No new or modified user-guide.md.
E3	Module structure compliance	➖ N/A	No new module.

---

⚠️ Non-Blocking Suggestions

• [A1] Tick the two unchecked checklist items (All tests pass locally and I have verified that my code follows the Code Guidelines) before merge. CI confirms tests do pass, so the first is likely just an oversight. Also consider adding and ticking the *(Security-sensitive changes only)* item, since this PR modifies the OAuth2 token-fetch path.

• [A3] Closes #6810 references an issue that doesn't exist in the public repo. If this is an internal tracker reference, replace with a note like Internal: [tracker-ref] or simply remove the Closes keyword so the merge commit isn't polluted with an unresolvable reference.

• [C3 / Design] except Exception at models.py:139 catches all exceptions indiscriminately, including oauthlib.oauth2.rfc6749.errors.FatalClientError subclasses (InvalidClientError, InvalidClientIdError, MissingClientIdError, etc.) that signal permanent configuration errors (wrong client_id/client_secret). Retrying these wastes 1.5 s and misleads operators: the warning log says "retrying" when the real cause is a bad credential. Consider re-raising FatalClientError (and its subclasses) immediately:

  from oauthlib.oauth2.rfc6749.errors import FatalClientError
  ...
  except FatalClientError:
      raise
  except Exception as exc:
      last_exc = exc
      ...

  This keeps retries for transient network failures while fast-failing on configuration mistakes.

✅ Things Done Well

• The retry loop correctly avoids sleeping after the final attempt (attempt < _TOKEN_FETCH_MAX_ATTEMPTS - 1 guard), which is easy to get wrong.
• The last_exc pattern is clean: the post-loop if last_exc is not None check handles both the exhausted-retries and the successful-break paths without an extra boolean flag.
• All five test cases are self-contained, mock time.sleep (so they are instantaneous), and verify behavior at the right granularity — delay values, call counts, and exception message content.
• Retry constants are named and extracted at module level, making them trivially configurable in the future.
• The warning log format (attempt %d/%d, retrying in %.1fs) gives operators exactly the information they need to correlate log lines with retry behaviour.

[betinacosta]

PR Review: #437 — fix(http): add retry with exponential backoff for OAuth2 token fetch

Author: betinacosta Branch: fix/xsuaa-token-fetch-retry → main
Verdict: ✅ Ready to Merge
Summary: All previous concerns addressed — FatalClientError is now re-raised immediately, the sixth test covers that path, all checklist items are ticked, and the spurious internal issue reference was removed.

---

A: Process & Compliance

#	Criterion	Status	Finding
A1	PR template complete	✅ Pass	All 9 core checklist items are now ticked. The Related Issue section was removed along with the unresolvable Closes #6810 reference. The security-sensitive conditional item is not present, but given the narrow scope of the change (wrapping an existing call in a retry loop with no new auth logic) this is acceptable.
A2	Conventional Commits	✅ Pass	PR title fix(http): add retry with exponential backoff for OAuth2 token fetch matches type(scope): description. Commit Validation CI job passed.
A3	Issue linked	➖ N/A	Closes #6810 was removed — no issue reference remains. Not required.
A4	AI-generated code disclosure	➖ N/A	No disclosure required.

B: Security & Sensitive Data

#	Criterion	Status	Finding
B1	No sensitive data in code	✅ Pass	No credentials, tokens, internal URLs, or tenant IDs in the diff.
B2	No sensitive data in PR body	✅ Pass	PR body clean; disclaimer preserved.

C: Code Quality

#	Criterion	Status	Finding
C1	CI checks passing	⚠️ Warning	Tests & Coverage, Code Quality Checks, Integration Tests, and Analyze (python) are still pending for the new commit (00b56d7). The previous commit's runs all passed; no reason to expect regressions given the change is purely additive. Confirm before merging.
C2	Version bump	➖ N/A	No version change in pyproject.toml.
C3	Type hints	✅ Pass	last_exc: Exception | None = None at models.py:128 is correct. No new public API surface.
C4	No hardcoded values	✅ Pass	_TOKEN_FETCH_MAX_ATTEMPTS = 3 and _TOKEN_FETCH_BACKOFF_BASE = 0.5 remain as named module-level constants.
C5	Import organization	✅ Pass	import time in stdlib block; from oauthlib.oauth2.rfc6749.errors import FatalClientError correctly placed in the third-party block alongside the other oauthlib import.
C6	Naming conventions	✅ Pass	No issues.
C7	No unused code	✅ Pass	FatalClientError is used at models.py:140. InvalidClientError in the test file is used at test_http_client.py:390. No dead code introduced.
C8	No unjustified new dependencies	✅ Pass	FatalClientError is from oauthlib which is already a transitive dependency via requests-oauthlib. No new pyproject.toml entries.
C9	Proto code freshness	➖ N/A	No .proto files changed.

D: API & Design

#	Criterion	Status	Finding
D1	API future-proofing	✅ Pass	Retry logic correctly separates permanent failures (FatalClientError) from transient ones. Constants at module level keep future configurability straightforward.
D2	Public API hygiene	✅ Pass	_fetch_token is private; no public surface changed.
D3	Breaking changes marked	✅ Pass	No breaking changes.
D4	Pagination & tenant filtering	➖ N/A	Not applicable.
D5	Telemetry instrumentation	➖ N/A	Infrastructure method; no telemetry expected.
D6	Multi-tenancy support	➖ N/A	Retry wraps the existing tenant-aware URL derivation; multi-tenancy behavior unchanged.

E: Tests & Documentation

#	Criterion	Status	Finding
E1	Tests added/updated	✅ Pass	TestXsuaaAuthProviderTokenFetchRetry now covers 6 scenarios: success on first attempt, retry-then-success, full exhaustion, exponential delays, no-sleep-after-final-attempt, and fast-fail on FatalClientError (test_http_client.py:388). All mock time.sleep.
E2	Documentation quality	✅ Pass	No public API or user-guide.md changed.
E2a	Multi-tenancy documentation	➖ N/A	No new or modified user-guide.md.
E3	Module structure compliance	➖ N/A	No new module.

---

⚠️ Non-Blocking Suggestions

• [C1] CI is still running for commit 00b56d7. Wait for Tests & Coverage, Code Quality Checks, Integration Tests, and Analyze (python) to go green before merging.

✅ Things Done Well

• FatalClientError is now re-raised immediately before the broad except Exception, so misconfigured credentials (InvalidClientError, InvalidClientIdError, etc.) fail fast without misleading retry logs.
• The new test test_fatal_client_error_raises_immediately_without_retry asserts both fetch_token.call_count == 1 and mock_sleep.assert_not_called() — exactly the right invariants.
• All previous review findings addressed cleanly in a single follow-up commit.
• FatalClientError import is placed correctly in the third-party block alongside the existing BackendApplicationClient import.

[NicoleMGomes]

@cassiofariasmachado please review