Add timeout and retry logic to Azure token fetch

[fivetran-rahulprakash]

Problem

The getAccessToken() method in AzureCredentialsStorageIntegration used an unbounded blocking call which could hang indefinitely if Azure's token endpoint was slow or unresponsive. This could lead to:

• Thread pool exhaustion in high-concurrency scenarios
• Cascading failures when Azure AD experiences degraded performance
• Poor user experience with no visibility into token fetch failures

Solution

This PR adds defensive timeout and retry mechanisms using Project Reactor's built-in capabilities:

• 15-second timeout per individual token request attempt to prevent indefinite blocking
• Exponential backoff retry (3 attempts with delays: 2s, 4s, 8s) with 50% jitter to prevent thundering herd during mass failures
• 90-second overall timeout as a safety net for the complete operation
• Intelligent retry filtering for known transient Azure AD errors:
  • AADSTS50058 - Token endpoint timeout
  • AADSTS50078 - Service temporarily unavailable
  • AADSTS700084 - Token refresh required
  • 503 - Service unavailable
  • 429 - Too many requests
• Enhanced logging for better observability (warnings on errors, info on retries)

Testing

• Code leverages existing reactor dependencies (no new dependencies)
• Follows existing Polaris patterns for reactive error handling

Benefits

• Improves system resilience to transient Azure service issues
• Prevents indefinite blocking that could cascade to request timeouts
• Provides better observability with structured logging
• Uses well-established retry patterns with exponential backoff and jitter

Checklist

• 🛡️ Don't disclose security issues! (Not applicable - this is a resilience improvement)
• 🔗 Clearly explained why the changes are needed
• 🧪 Manually tested via compilation; reactive behavior follows reactor-core semantics
• 💡 Added comprehensive Javadoc explaining the retry strategy
• 🧾 Updated CHANGELOG.md (awaiting maintainer guidance on format)
• 📚 Updated documentation (no user-facing config changes)

[dimas-b]

Nice improvement! Thanks for your contribution, @fivetran-rahulprakash !

[dimas-b]

We have RealmConfig here, could you add a general setting in FeatureConfiguration for this timeout. I suppose it could applicable to other integrations too (but, of course, in this PR we can concentrate on Azure only)

[fivetran-rahulprakash]

I've added four new configuration constants in FeatureConfiguration:
CLOUD_API_TIMEOUT_SECONDS (default: 15) - Per-attempt timeout
CLOUD_API_RETRY_COUNT (default: 3) - Number of retry attempts
CLOUD_API_RETRY_DELAY_SECONDS (default: 2) - Initial delay for exponential backoff
CLOUD_API_RETRY_JITTER_MILLIS (default: 500) - Maximum jitter to prevent thundering herd
These use generic naming (CLOUD_API_*) rather than Azure-specific names, making them reusable for future implementations.

[dimas-b]

Having backoff settings configurable could also be helpful.

[RahulPrakash96]

Addressed

[dimas-b]

TimeoutException is already handled by isRetriableAzureException, right?

[RahulPrakash96]

Yes, you're absolutely right! I missed that. Removed the duplicate check now. Thank you for catching that!

[dimas-b]

Why do we need this on top of .timeout() (line 337)?

[RahulPrakash96]

Good point! I initially added the overall timeout as a safety net to ensure we never block indefinitely, but you're right it's unnecessary. The combination of per-attempt timeout and .retryWhen() with exponential backoff already provides sufficient protection. Removed it now. Thanks for the feedback!

[fivetran-rahulprakash]

Thank you @dimas-b for the thorough review and excellent suggestions! I've addressed all your comments

[fivetran-rahulprakash]

I chose to use milliseconds instead of a 0-1 jitter factor for a few reasons:

User clarity - It's more intuitive for operators to specify "500 milliseconds of jitter" rather than understanding what "0.5 jitter factor" means (50% of the retry delay)
Concrete vs relative - Millis gives direct control over the maximum random delay added, while a factor requires understanding how it interacts with the exponential backoff delays
Consistency - All other time-based configs use concrete units (seconds/millis) rather than abstract factors
Predictability - With millis, the max jitter is always clear regardless of retry delay values

The small conversion cost (jitterMillis / 1000.0) is negligible compared to the benefits of making the config more operator friendly. Happy to change to 0-1 factor if you prefer that approach though!

[dimas-b]

LGTM overall, just some minor comments about the new config.

[dimas-b]

Would you mind using millis for initialDelaySeconds... in some cases even 1 sec may be too long. Let's delegate what the min delay should be to the admin user who configures it.

Same for timeoutSeconds... I hope Azure SDK supports millis.

[RahulPrakash96]

Thanks for the suggestion!

Changed both to milliseconds:
CLOUD_API_TIMEOUT_MILLIS (default: 15000ms)
CLOUD_API_RETRY_DELAY_MILLIS (default: 2000ms)

[dimas-b]

Sorry, one more minor comment from my side... Otherwise LGTM 👍

[dimas-b]

I'm not sure, I fully understand this logic... per javadoc of reactor.util.retry.RetryBackoffSpec.jitter() the factor applies to the "computed delay", which may not be 1000 ms 🤔 How can the user reason about what the CLOUD_API_RETRY_JITTER_MILLIS value of 750 (for example) means?

Would it not be simpler to use the 0.0-1.0 factor value in the config?

[RahulPrakash96]

Ah, I went with millis initially to keep it consistent with the other timeout/delay configs (which are all in milliseconds), thinking it'd be more straightforward to work with absolute values.

But you're right - that doesn't really make sense here since the jitter factor gets applied to the computed delay, which changes with each retry (2s → 4s → 8s). So 750 would mean something different on each attempt, which is pretty confusing.

Changed it to CLOUD_API_RETRY_JITTER_FACTOR using the 0.0-1.0 range

[dimas-b]

LGTM 👍 Thanks again @fivetran-rahulprakash !

I'll leave it open for some more time for other reviewers to comment (if there's interest)

[fivetran-rahulprakash]

LGTM 👍 Thanks again @fivetran-rahulprakash !

I'll leave it open for some more time for other reviewers to comment (if there's interest)

Thanks! Just following up - are we good to merge 🤔

[flyrain]

Thanks a lot for working on it, @fivetran-rahulprakash ! LGTM overall. Left some comments!

[flyrain]

Can we log the full stack trace instead of error.getMessage() only for better debuggability?

[RahulPrakash96]

Done! Changed to LOGGER.warn("Error fetching Azure access token for tenant {}", tenantId, error) which will log the full exception with stack trace. 😄

[flyrain]

Should we add 1 to retryCount? Sot it matches the word "attempt" here? The total attempt number would be 1 + retryCount:

int maxAttempts = retryCount + 1;
...

LOGGER.info("Retrying Azure token fetch for tenant {} (attempt {}/{})",
            tenantId,
            retrySignal.totalRetries() + 1,
            maxAttempts);

[RahulPrakash96]

Modified

[flyrain]

CLOUD_API_TIMEOUT_MILLIS may over-communicate its scope. Would a more precise name like STORAGE_API_TIMEOUT_MILLIS or even AZURE_STORAGE_API_TIMEOUT_MILLIS be clearer?

If s3/GCS/MinIO doesn't have similar configuration or cannot be applied with the same timeout, I'd suggest to have a prefix AZURE_ to make it clear.

[RahulPrakash96]

Good call! I've changed it to STORAGE_API_* to be more specific.

While only Azure uses these configs right now, the timeout/retry pattern applies to other cloud storage providers (AWS S3, GCP) too - their SDKs have similar async patterns that would benefit from this. So keeping the naming generic makes it easier to reuse when we add resilience to those integrations later.

Updated the descriptions to clarify it's currently Azure only but designed for future reuse. Let me know if you'd prefer something different!

[flyrain]

@fivetran-rahulprakash , thanks for the change! One small concern, different storage backends may eventually need different timeout/retry configs. We can always introduce a broader-scope config like STORAGE_API_TIMEOUT_MILLIS later if we find something truly common across all storage types, but evolving in the opposite direction is harder. Once a wide-scope config is adopted and used in different places, shrinking it down to a more specific scope becomes much more painful. WDYT?

[RahulPrakash96]

@flyrain I've renamed the configs to use AZURE_* prefix. Thanks for the feedback!

[fivetran-rahulprakash]

Thanks a lot for working on it, @fivetran-rahulprakash ! LGTM overall. Left some comments!

Thanks for the thorough review, @flyrain! I've addressed all your comments

[flyrain]

+1. Thanks a lot for working on it, @fivetran-rahulprakash ! Thanks for the review, @dimas-b @jbonofre !