Skip to content

Commit af20ea5

Browse files
Prvnkmr337prsaminathan
andauthored
Add flight-controlled thread pool expansion for silent requests, Fixes AB#3492151 (#2865)
Fixes: [AB#3492151](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3492151) Problem: The Broker's silent token request thread pool (sSilentExecutor) is fixed at 5 threads. Under high load scenarios with multiple concurrent ATS requests from various client apps (Teams, Outlook, OneAuth), this can cause request queuing, increased latency, and potential timeouts. Solution: Introduce a flight-controlled mechanism to expand the thread pool from 5 to 8 threads, applying ONLY to the Broker module (not MSAL client apps). Changes: - CommonFlight.java: Add ENABLE_EXPANDED_BROKER_SILENT_THREAD_POOL flight (default: false) - CommandDispatcher.java: Add initializeSilentExecutorForBroker() and resetSilentRequestExecutorWithSize() methods with graceful executor shutdown handling - AndroidBrokerFlightsManager.kt: Call initializeBrokerSilentExecutor() during Broker initialization after flights are loaded Key design decisions: - Uses Android process isolation: Broker runs in separate process, so static sSilentExecutor is independent from MSAL client apps - Broker-only initialization: AndroidBrokerFlightsManager is Broker-specific code that MSAL apps never call - Flight-controlled: Safe rollout via ECS with instant rollback capability - Graceful shutdown: Existing executor is properly terminated before creating expanded pool --------- Co-authored-by: prsaminathan <[email protected]>
1 parent 876d59d commit af20ea5

File tree

8 files changed

+347
-7
lines changed

8 files changed

+347
-7
lines changed

changelog.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ vNext
55
- [MINOR] Cleanup code related to emitting ests telemetry (#2868)
66
- [MINOR] Skip account aggregation when responding to AcquireTokenSilent api call for OneAuth (#2863)
77
- [MINOR] Introduce locks at NameValueStorageFileManagerSimpleCacheImpl layer (#2842)
8+
- [MINOR] Add flight-controlled option to expand Broker CommandDispatcher silent thread pool to 8 threads (default remains 5; MSAL clients unaffected) (#2865)
89
- [PATCH] Implement State-Based Timeout Classification for Silent Token Requests (#2870)
910
- [MINOR] Added Authentication Constants to be used for broker version and name (#2859)
1011
- [PATCH] Adding WebAppsNonce to JWT request body (#2867)

common/src/androidTest/java/com/microsoft/identity/common/CommandDispatcherTest.java

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333

3434
import com.microsoft.identity.common.components.AndroidPlatformComponentsFactory;
3535
import com.microsoft.identity.common.internal.commands.RefreshOnCommand;
36+
import com.microsoft.identity.common.java.AuthenticationConstants;
3637
import com.microsoft.identity.common.java.cache.CacheRecord;
3738
import com.microsoft.identity.common.java.cache.ICacheRecord;
3839
import com.microsoft.identity.common.java.commands.BaseCommand;
@@ -54,6 +55,7 @@
5455
import com.microsoft.identity.common.java.dto.IdTokenRecord;
5556
import com.microsoft.identity.common.java.dto.RefreshTokenRecord;
5657
import com.microsoft.identity.common.java.exception.ClientException;
58+
import com.microsoft.identity.common.java.exception.ErrorStrings;
5759
import com.microsoft.identity.common.java.exception.ServiceException;
5860
import com.microsoft.identity.common.java.exception.TerminalException;
5961
import com.microsoft.identity.common.java.providers.oauth2.AuthorizationResult;
@@ -433,6 +435,114 @@ public void testResetSilentRequestExecutor() throws Exception {
433435
Assert.assertEquals(TEST_RESULT_STR, result.getResult());
434436
}
435437

438+
/**
439+
* Tests that initializeSilentExecutorWithExpandedPool() correctly expands the thread pool
440+
* when called with a valid Broker package name (Azure Authenticator).
441+
*/
442+
@Test
443+
public void testInitializeSilentExecutorWithExpandedPool_WithValidBrokerPackage() throws Exception {
444+
CommandDispatcher.clearState();
445+
446+
// Test with Azure Authenticator package
447+
CommandDispatcher.initializeSilentExecutorWithExpandedPool(
448+
AuthenticationConstants.Broker.AZURE_AUTHENTICATOR_APP_PACKAGE_NAME);
449+
450+
// Verify executor is functional
451+
verifyExecutorIsFunctional();
452+
453+
CommandDispatcher.clearState();
454+
}
455+
456+
/**
457+
* Tests that initializeSilentExecutorWithExpandedPool() works with all valid Broker packages.
458+
*/
459+
@Test
460+
public void testInitializeSilentExecutorWithExpandedPool_AllBrokerPackages() throws Exception {
461+
final String[] brokerPackages = {
462+
AuthenticationConstants.Broker.AZURE_AUTHENTICATOR_APP_PACKAGE_NAME,
463+
AuthenticationConstants.Broker.LTW_APP_PACKAGE_NAME,
464+
AuthenticationConstants.Broker.COMPANY_PORTAL_APP_PACKAGE_NAME
465+
};
466+
467+
for (final String packageName : brokerPackages) {
468+
CommandDispatcher.clearState();
469+
CommandDispatcher.initializeSilentExecutorWithExpandedPool(packageName);
470+
verifyExecutorIsFunctional();
471+
}
472+
473+
CommandDispatcher.clearState();
474+
}
475+
476+
/**
477+
* Tests that initializeSilentExecutorWithExpandedPool() throws ClientException
478+
* when called with a non-Broker package name.
479+
*/
480+
@Test
481+
public void testInitializeSilentExecutorWithExpandedPool_WithNonBrokerPackage_ThrowsException() throws Exception {
482+
CommandDispatcher.clearState();
483+
484+
try {
485+
CommandDispatcher.initializeSilentExecutorWithExpandedPool("com.some.msal.app");
486+
Assert.fail("Expected ClientException to be thrown for non-Broker package");
487+
} catch (final ClientException e) {
488+
Assert.assertEquals(ErrorStrings.BROKER_ONLY_OPERATION, e.getErrorCode());
489+
// Verify message does NOT contain the package name (no sensitive info leak)
490+
Assert.assertFalse(e.getMessage().contains("com.some.msal.app"));
491+
} finally {
492+
CommandDispatcher.clearState();
493+
}
494+
}
495+
496+
/**
497+
* Tests that initializeSilentExecutorWithExpandedPool() throws NullPointerException
498+
* when called with null package name due to @NonNull annotation enforcement.
499+
* Passing null is a programming error and should fail fast.
500+
*/
501+
@Test(expected = NullPointerException.class)
502+
public void testInitializeSilentExecutorWithExpandedPool_WithNullPackage_ThrowsException() throws Exception {
503+
CommandDispatcher.clearState();
504+
try {
505+
CommandDispatcher.initializeSilentExecutorWithExpandedPool(null);
506+
} finally {
507+
CommandDispatcher.clearState();
508+
}
509+
}
510+
511+
/**
512+
* Tests that initializeSilentExecutorWithExpandedPool() throws ClientException
513+
* when called with empty package name.
514+
*/
515+
@Test
516+
public void testInitializeSilentExecutorWithExpandedPool_WithEmptyPackage_ThrowsException() throws Exception {
517+
CommandDispatcher.clearState();
518+
519+
try {
520+
CommandDispatcher.initializeSilentExecutorWithExpandedPool("");
521+
Assert.fail("Expected ClientException to be thrown for empty package");
522+
} catch (final ClientException e) {
523+
Assert.assertEquals(ErrorStrings.BROKER_ONLY_OPERATION, e.getErrorCode());
524+
} finally {
525+
CommandDispatcher.clearState();
526+
}
527+
}
528+
529+
/**
530+
* Helper method to verify the silent executor is functional after initialization.
531+
*/
532+
private void verifyExecutorIsFunctional() throws Exception {
533+
final CountDownLatch testLatch = new CountDownLatch(1);
534+
final TestCommand testCommand = getTestCommand(testLatch);
535+
536+
final FinalizableResultFuture<CommandResult> future = CommandDispatcher.submitSilentReturningFuture(testCommand);
537+
testLatch.await();
538+
539+
final CommandResult result = future.get();
540+
Assert.assertEquals(ICommandResult.ResultStatus.COMPLETED, result.getStatus());
541+
Assert.assertEquals(TEST_RESULT_STR, result.getResult());
542+
Assert.assertTrue(future.isDone());
543+
future.isCleanedUp();
544+
}
545+
436546
private TestCommand getTestCommand(final CountDownLatch testLatch) {
437547
return new TestCommand(
438548
getEmptyTestParams(),

common/src/main/java/com/microsoft/identity/common/internal/result/MsalBrokerResultAdapter.java

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
import static com.microsoft.identity.common.java.AuthenticationConstants.Broker.BROKER_REQUEST_RECEIVED_TIMESTAMP;
2626
import static com.microsoft.identity.common.java.AuthenticationConstants.Broker.BROKER_RESPONSE_GENERATION_TIMESTAMP;
27+
import static com.microsoft.identity.common.java.AuthenticationConstants.Broker.BROKER_SILENT_EXECUTOR_POOL_SIZE;
2728
import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.Broker.BROKER_ACCOUNTS;
2829
import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.Broker.BROKER_ACCOUNTS_COMPRESSED;
2930
import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.Broker.BROKER_ACTIVITY_NAME;
@@ -65,6 +66,7 @@
6566
import com.microsoft.identity.common.java.broker.BrokerPerformanceMetrics;
6667
import com.microsoft.identity.common.java.cache.CacheRecord;
6768
import com.microsoft.identity.common.java.cache.ICacheRecord;
69+
import com.microsoft.identity.common.java.controllers.CommandDispatcher;
6870
import com.microsoft.identity.common.java.commands.AcquirePrtSsoTokenBatchResult;
6971
import com.microsoft.identity.common.java.commands.AcquirePrtSsoTokenResult;
7072
import com.microsoft.identity.common.java.commands.webapps.WebAppsAccountItem;
@@ -537,11 +539,16 @@ public BrokerPerformanceMetrics getBrokerPerformanceMetricsFromBundle(@NonNull f
537539
BROKER_RESPONSE_GENERATION_TIMESTAMP,
538540
INVALID_TIMESTAMP
539541
);
542+
final int silentExecutorPoolSize = resultBundle.getInt(
543+
BROKER_SILENT_EXECUTOR_POOL_SIZE,
544+
CommandDispatcher.getDefaultSilentExecutorPoolSize()
545+
);
540546

541547
if (brokerRequestReceivedTimestamp != INVALID_TIMESTAMP && brokerResponseGenerationTimestamp != INVALID_TIMESTAMP) {
542548
return new BrokerPerformanceMetrics(
543549
brokerRequestReceivedTimestamp,
544-
brokerResponseGenerationTimestamp
550+
brokerResponseGenerationTimestamp,
551+
silentExecutorPoolSize
545552
);
546553
} else {
547554
Logger.warn(TAG, "Broker performance metrics not found in the result bundle.");

common4j/src/main/com/microsoft/identity/common/java/AuthenticationConstants.java

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -521,6 +521,21 @@ public static final class AAD {
521521
@NoArgsConstructor(access = AccessLevel.PRIVATE)
522522
public static final class Broker {
523523

524+
/**
525+
* Azure Authenticator app package name.
526+
*/
527+
public static final String AZURE_AUTHENTICATOR_APP_PACKAGE_NAME = "com.azure.authenticator";
528+
529+
/**
530+
* Company Portal app package name.
531+
*/
532+
public static final String COMPANY_PORTAL_APP_PACKAGE_NAME = "com.microsoft.windowsintune.companyportal";
533+
534+
/**
535+
* Package name of the Link To Windows app.
536+
*/
537+
public static final String LTW_APP_PACKAGE_NAME = "com.microsoft.appmanager";
538+
524539
/**
525540
* Default timeout for broker tasks/futures.
526541
*/
@@ -578,6 +593,12 @@ public static final class Broker {
578593
*/
579594
public static final String BROKER_RESPONSE_GENERATION_TIMESTAMP = "broker_response_generation_timestamp";
580595

596+
/**
597+
* Key for the silent executor pool size in broker response bundles.
598+
* Used by clients (e.g., OneAuth) to recreate BrokerPerformanceMetrics.
599+
*/
600+
public static final String BROKER_SILENT_EXECUTOR_POOL_SIZE = "broker_silent_executor_pool_size";
601+
581602
/**
582603
* Broker application version name.
583604
*/

common4j/src/main/com/microsoft/identity/common/java/broker/BrokerPerformanceMetrics.kt

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
// THE SOFTWARE.
2323
package com.microsoft.identity.common.java.broker
2424

25+
import com.microsoft.identity.common.java.controllers.CommandDispatcher
2526
import com.microsoft.identity.common.java.logging.Logger
2627

2728
/**
@@ -39,7 +40,14 @@ class BrokerPerformanceMetrics(
3940
* Timestamp when the broker generated the authentication response (epoch milliseconds).
4041
* Used to calculate both broker handling time and response latency.
4142
*/
42-
val brokerResponseGenerationTimestamp: Long
43+
val brokerResponseGenerationTimestamp: Long,
44+
45+
/**
46+
* The configured pool size of the silent executor at the time of the request.
47+
* Helps diagnose concurrency-related performance issues.
48+
* Defaults to the standard silent executor pool size.
49+
*/
50+
val silentExecutorPoolSize: Int
4351
) {
4452
/**
4553
* Time spent by broker processing the request (in milliseconds).
@@ -70,7 +78,8 @@ class BrokerPerformanceMetrics(
7078
"brokerHandlingTime=$brokerHandlingTime, " +
7179
"responseLatency=$responseLatency, " +
7280
"brokerRequestReceivedTimestamp=$brokerRequestReceivedTimestamp, " +
73-
"brokerResponseGenerationTimestamp=$brokerResponseGenerationTimestamp)"
81+
"brokerResponseGenerationTimestamp=$brokerResponseGenerationTimestamp, " +
82+
"silentExecutorPoolSize=$silentExecutorPoolSize)"
7483
}
7584

7685
companion object {

0 commit comments

Comments
 (0)