App Config - Startup retry

[mrm9084]

Description

Adds retry to startup. When all replicas fail there will be an attempt to retry the failed store for a period of time. By default 100s, minimal 30s, maximum 600s.

Also, refactors the load method to be split into a number of helper method to make this readable.

All SDK Contribution checklist:

• The pull request does not introduce [breaking changes]
• CHANGELOG is updated for new features, bug fixes or other significant changes.
• I have read the contribution guidelines.

General Guidelines and Best Practices

• Title of the pull request is clear and informative.
• There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

• Pull request includes test coverage for the included changes.

[Copilot]

Pull request overview

This pull request adds a startup retry mechanism to Azure App Configuration for Java to handle transient failures during application startup. When all replicas fail to load configuration, the provider will automatically retry with exponential backoff until a configurable timeout expires.

Changes:

• Added startup-timeout configuration property (default: 100s, min: 30s, max: 600s) to control retry duration during startup
• Refactored AzureAppConfigDataLoader.load() into smaller helper methods (loadConfiguration, attemptLoadFromClients, setupMonitoringState, handleReplicaFailure) for improved readability
• Implemented retry loop with intelligent backoff that waits until the next client becomes available before retrying

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
README.md	Added documentation for new startup-timeout configuration option
CHANGELOG.md	Documented the new startup retry feature
AppConfigurationProperties.java	Added startupTimeout field with default value and validation (30-600 seconds)
AzureAppConfigDataResource.java	Added startupTimeout parameter to constructor and getter method
AzureAppConfigDataLocationResolver.java	Passed startupTimeout from properties to resources
AzureAppConfigDataLoader.java	Refactored load method and implemented retry logic with backoff for startup failures
ConnectionManager.java	Added getMillisUntilNextClientAvailable() to calculate wait time until next replica is available
AppConfigurationReplicaClientFactory.java	Added wrapper method to expose getMillisUntilNextClientAvailable
ConfigStore.java	Minor code quality improvements (variable naming, isEmpty() usage)
ConnectionManagerTest.java	Added comprehensive tests for getMillisUntilNextClientAvailable method
AzureAppConfigDataResourceTest.java	Updated test constructor calls to include startupTimeout parameter
AzureAppConfigDataLoaderTest.java	Added tests for startup retry behavior and refresh non-retry behavior

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.

[Copilot]

There are no tests verifying the startupTimeout validation logic that was added. Consider adding tests to verify that: 1) null startupTimeout throws IllegalArgumentException, 2) values below 30 seconds throw IllegalArgumentException, 3) values above 600 seconds throw IllegalArgumentException, and 4) values within the valid range (30-600) are accepted.

[Copilot]

The condition at line 192 is redundant because it's already guaranteed by the while loop condition at line 181. The code will only reach line 192 if Instant.now().isBefore(deadline) is true. Consider removing this redundant check to simplify the code.

Suggested change

	if (Instant.now().isBefore(deadline)) {
	long elapsedSeconds = Instant.now().getEpochSecond() - startTime.getEpochSecond();
	Long backoffSeconds = getBackoffDuration(elapsedSeconds);
	// If backoff is null, elapsed time exceeds fixed intervals - use exponential backoff
	if (backoffSeconds == null) {
	postFixedWindowAttempts++;
	// Convert nanoseconds to seconds
	backoffSeconds = BackoffTimeCalculator.calculateBackoff(postFixedWindowAttempts) / 1_000_000_000L;
	}
	// Don't wait longer than remaining time until deadline
	long remainingSeconds = deadline.getEpochSecond() - Instant.now().getEpochSecond();
	long waitSeconds = Math.min(backoffSeconds, remainingSeconds);
	if (waitSeconds > 0) {
	logger.debug("All replicas in backoff for store: " + resource.getEndpoint()
	+ ". Waiting " + waitSeconds + "s before retry (elapsed: " + elapsedSeconds + "s).");
	try {
	Thread.sleep(waitSeconds * 1000);
	} catch (InterruptedException e) {
	Thread.currentThread().interrupt();
	return lastException;
	}
	long elapsedSeconds = Instant.now().getEpochSecond() - startTime.getEpochSecond();
	Long backoffSeconds = getBackoffDuration(elapsedSeconds);
	// If backoff is null, elapsed time exceeds fixed intervals - use exponential backoff
	if (backoffSeconds == null) {
	postFixedWindowAttempts++;
	// Convert nanoseconds to seconds
	backoffSeconds = BackoffTimeCalculator.calculateBackoff(postFixedWindowAttempts) / 1_000_000_000L;
	}
	// Don't wait longer than remaining time until deadline
	long remainingSeconds = deadline.getEpochSecond() - Instant.now().getEpochSecond();
	long waitSeconds = Math.min(backoffSeconds, remainingSeconds);
	if (waitSeconds > 0) {
	logger.debug("All replicas in backoff for store: " + resource.getEndpoint()
	+ ". Waiting " + waitSeconds + "s before retry (elapsed: " + elapsedSeconds + "s).");
	try {
	Thread.sleep(waitSeconds * 1000);
	} catch (InterruptedException e) {
	Thread.currentThread().interrupt();
	return lastException;

[Copilot]

At line 204, remainingSeconds could potentially be negative if there's a delay between the while condition check at line 181 and reaching line 204. While unlikely in practice, this could result in negative values being passed to Math.min() at line 205, which would then result in a negative waitSeconds. The check at line 207 prevents sleeping with negative values, but it would be clearer to use Math.max(0, deadline.getEpochSecond() - Instant.now().getEpochSecond()) to ensure remainingSeconds is never negative.

Suggested change

	long remainingSeconds = deadline.getEpochSecond() - Instant.now().getEpochSecond();
	long remainingSeconds = Math.max(0L, deadline.getEpochSecond() - Instant.now().getEpochSecond());

[Copilot]

The variable secondClientMock is created but never used. It can be removed to clean up the test.

Suggested change

	// Create a second client mock for the successful retry
	AppConfigurationReplicaClient secondClientMock = Mockito.mock(AppConfigurationReplicaClient.class);
	lenient().when(secondClientMock.getEndpoint()).thenReturn(ENDPOINT);

[Copilot]

The method getMillisUntilNextClientAvailable is not used anywhere in the production code. It's only called in tests. This suggests that either the method should be removed as dead code, or the retry logic in AzureAppConfigDataLoader should be using this method to determine optimal wait times instead of fixed backoff intervals. Consider removing this method or integrating it into the actual retry logic.

[Copilot]

The method getMillisUntilNextClientAvailable is not used anywhere in the production code. It's only called in tests. This suggests that either the method should be removed as dead code, or it should be integrated into the retry logic. Consider removing this method or using it in the startup retry implementation.

Suggested change

	* Gets the duration in milliseconds until the next client becomes available for the specified store.
	*
	* @param originEndpoint the origin configuration store endpoint
	* @return duration in milliseconds until next client is available, or 0 if one is available now
	*/
	long getMillisUntilNextClientAvailable(String originEndpoint) {
	return CONNECTIONS.get(originEndpoint).getMillisUntilNextClientAvailable();
	}
	/**

[Copilot]

The fixed backoff intervals extend to 600 seconds, but the comment at line 90 says "0-100 seconds elapsed". This threshold value (100) should match the first interval threshold and appears correct. However, the last interval at line 92 covers "200-600 seconds elapsed" which seems inconsistent with the default and maximum timeout of 100-600 seconds. Since the default timeout is 100 seconds and the minimum is 30 seconds, many users will never reach the higher backoff intervals defined here. Consider whether these intervals align with the expected timeout ranges.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

[Copilot]

In attemptLoadFromClients, sourceList.addAll(createSettings(...)) happens before feature flags/monitoring are fully loaded. If a replica fails after settings were added (e.g., exception in createFeatureFlags or setupMonitoringState), the method proceeds to the next replica without rolling back, potentially mixing property sources from multiple replicas in a single load attempt. Consider building into a temporary list and only merging into sourceList once the full load for a replica succeeds (or clearing/rolling back to the previous size on failure).

[Copilot]

secondClientMock is created but never used in this test, which adds noise and can confuse the intended scenario. Please remove it or incorporate it into the stubbing/verification to reflect the "retry then succeed" path being tested.

Suggested change

	// Create a second client mock for the successful retry
	AppConfigurationReplicaClient secondClientMock = Mockito.mock(AppConfigurationReplicaClient.class);
	lenient().when(secondClientMock.getEndpoint()).thenReturn(ENDPOINT);
	// Setup mocks:
	// - First getNextActiveClient(true) returns clientMock which will throw
	// - First getNextActiveClient(false) returns null (no more replicas in first attempt)
	// - Second getNextActiveClient(true) returns null (simulating success path)
	// Setup mocks to simulate a retry after initial client failure:
	// - First getNextActiveClient(true) returns clientMock which will throw
	// - First getNextActiveClient(false) returns null (no more replicas in first attempt)
	// - Second getNextActiveClient(true) returns null (simulating success path with no more failures)

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

[Copilot]

When getNextActiveClient returns null on line 248, attemptLoadFromClients returns null (no exception), which is treated as success in lines 187-188. However, getNextActiveClient returns null in two scenarios: (1) no clients are configured, and (2) clients exist but all are currently in backoff. During startup retry, case (2) should trigger continued retries with backoff until clients become available or timeout expires, but the current implementation treats it as immediate success, breaking out of the retry loop. This means startup might succeed with empty configuration when all replicas are temporarily unavailable. Consider having attemptLoadFromClients return a specific exception (e.g., NoAvailableClientsException) when getNextActiveClient returns null to distinguish "no attempt made" from "attempt succeeded", allowing the retry logic to work as intended.

Suggested change

	* Attempts to load configuration from available clients.
	*
	* @param sourceList the list to populate with property sources
	* @return the exception if all clients failed, null on success
	*/
	private Exception attemptLoadFromClients(List<EnumerablePropertySource<?>> sourceList) {
	boolean reloadFailed = false;
	Exception lastException = null;
	AppConfigurationReplicaClient client = replicaClientFactory.getNextActiveClient(resource.getEndpoint(), true);
	* Exception indicating that no App Configuration clients were available to load configuration.
	* <p>
	* This is used internally to distinguish between a successful configuration load and the case
	* where no attempt could be made because there were no active clients (either none configured
	* or all currently in backoff).
	*/
	private static final class NoAvailableClientsException extends Exception {
	private static final long serialVersionUID = 1L;
	NoAvailableClientsException(String message) {
	super(message);
	}
	}
	/**
	* Attempts to load configuration from available clients.
	*
	* @param sourceList the list to populate with property sources
	* @return the exception if all clients failed or no clients were available, {@code null} on success
	*/
	private Exception attemptLoadFromClients(List<EnumerablePropertySource<?>> sourceList) {
	boolean reloadFailed = false;
	Exception lastException = null;
	AppConfigurationReplicaClient client = replicaClientFactory.getNextActiveClient(resource.getEndpoint(), true);
	if (client == null) {
	// No active clients are currently available (either none configured or all in backoff).
	// Signal to the caller that no attempt was made so that startup retry logic can continue.
	return new NoAvailableClientsException(
	"No active App Configuration clients available for endpoint: " + resource.getEndpoint());
	}

[Copilot]

The validation throws IllegalArgumentException when startupTimeout is null, but the property has a default value of Duration.ofSeconds(100) defined at line 42. This means the null check will never be true unless someone explicitly calls setStartupTimeout(null). Consider removing this check since it's redundant with the default value, or document why explicit null setting should be prevented.

[Copilot]

Missing test coverage for the new startupTimeout validation logic added in lines 137-142. Consider adding tests to verify: (1) validation accepts valid timeout values (e.g., 30s, 100s, 600s), (2) validation rejects timeout less than 30 seconds, (3) validation rejects timeout greater than 600 seconds, and (4) validation handles null timeout (if that's an important edge case despite the default value). This follows the existing test pattern seen for refreshInterval validation in minValidWatchTime test at line 105.

[Copilot]

The multiplication waitSeconds * 1000 could potentially overflow for very large waitSeconds values (greater than Long.MAX_VALUE / 1000, approximately 9.2 million seconds). While the current logic caps waitSeconds based on the deadline and backoff intervals making overflow unlikely in practice, consider using Math.multiplyExact or adding a guard check to ensure safe multiplication and provide clearer error handling if overflow occurs.

Suggested change

	Thread.sleep(waitSeconds * 1000);
	long waitMillis;
	try {
	waitMillis = Math.multiplyExact(waitSeconds, 1000L);
	} catch (ArithmeticException ex) {
	// In the unlikely event of overflow, cap to the maximum sleep duration.
	waitMillis = Long.MAX_VALUE;
	}
	Thread.sleep(waitMillis);

[Copilot]

The calculation Instant.now().getEpochSecond() - startTime.getEpochSecond() may not accurately reflect elapsed time during the retry loop because it's called multiple times at different points in the loop (lines 193, 204, and within the while condition on line 181). Each call to Instant.now() captures a slightly different time, which could lead to minor inconsistencies. Consider capturing Instant.now() once at the beginning of each iteration and reusing that value for all time-related calculations within that iteration to ensure consistency.