Skip to content

Add e2e concurrent/stress test for AcquireTokenSilent via broker, Fixes AB#3582859#2504

Open
Copilot wants to merge 14 commits into
devfrom
copilot/add-e2e-concurrent-stress-test
Open

Add e2e concurrent/stress test for AcquireTokenSilent via broker, Fixes AB#3582859#2504
Copilot wants to merge 14 commits into
devfrom
copilot/add-e2e-concurrent-stress-test

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 30, 2026

  • Reply to @rpdome's question about startBarrier (stale automated-reviewer comment)
  • Apply all 4 automated-review comments from pullrequestreview-4206635904:
    • Input validation in ConcurrentAcquireTokenSilentHelper.run() (threadCount/iterations/perWaveTimeoutSec > 0)
    • Separate InterruptedException handling in barrier catch block (restores interrupt status)
    • Wrap requester.request() in try/catch(Throwable) with latch countDown on synchronous throw
    • Use exception.getErrorCode() + exception.toString() in test error recording

@github-actions
Copy link
Copy Markdown

❌ Work item link check failed. Description does not contain AB#{ID}.

Click here to Learn more.

@rpdome
Copy link
Copy Markdown
Member

rpdome commented Apr 30, 2026

@copilot this should serve as a stress test as well. Increase the amount of thread and execution per thread to match with the UI. (I assume you cannot use the test app for some reason, which is okay).

@rpdome rpdome marked this pull request as ready for review April 30, 2026 03:38
@rpdome rpdome requested a review from a team as a code owner April 30, 2026 03:38
Copilot AI review requested due to automatic review settings April 30, 2026 03:38
@rpdome
Copy link
Copy Markdown
Member

rpdome commented Apr 30, 2026

@copilot consider sharing the concurrency implementation between the automation library and msaltestapp

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 30, 2026

@copilot this should serve as a stress test as well. Increase the amount of thread and execution per thread to match with the UI. (I assume you cannot use the test app for some reason, which is okay)....

Done in commit a54f69d. Updated to match the UI defaults exactly:

  • 13 threads (CONCURRENT_THREADS = 13, matching concurrent_count = 13 in the layout)
  • 1000 iterations per thread (ITERATIONS_PER_THREAD = 1000, matching concurrent_iterations = 1000 in the layout)

The test now uses a CyclicBarrier(13) so all 13 threads fire acquireTokenSilentAsync(forceRefresh=true) simultaneously on each of the 1000 waves. Each thread waits for its own per-request latch before proceeding to the next iteration, ensuring the barrier correctly aligns the next wave. THREAD_SCOPES was expanded to 13 unique scope combinations (one per thread) to prevent CommandDispatcher deduplication. Total timeout is 4 hours to give ample headroom for the full 13 × 1000 = 13,000 requests.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new concurrency/stress scenario for brokered AcquireTokenSilent: (1) an executor utility intended to power the testapp’s “Run Concurrent” UI, and (2) a new broker E2E instrumentation test that launches multiple acquireTokenSilentAsync requests simultaneously and fails if any callback gets stuck.

Changes:

  • Add ConcurrentAcquireTokenExecutor implementation for the testapp concurrent ATS UI flow.
  • Add a new broker E2E instrumentation test that runs concurrent AcquireTokenSilent with forceRefresh=true and a global completion timeout.

Reviewed changes

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

File Description
testapps/testapp/src/main/java/com/microsoft/identity/client/testapp/ConcurrentAcquireTokenExecutor.java Adds an executor to run concurrent silent-token waves and report progress back to the UI.
msalautomationapp/src/androidTest/java/com/microsoft/identity/client/msal/automationapp/testpass/broker/concurrent/TestCaseConcurrentAcquireTokenSilent.java Adds an instrumentation test that stresses brokered silent requests with coordinated concurrency and stuck detection.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 30, 2026

@copilot consider sharing the concurrency implementation between the automation library and msaltestapp

Done in commit 588db9c. Extracted a new ConcurrentAcquireTokenSilentHelper class alongside the test in the concurrent package. It encapsulates:

  • THREAD_SCOPES – the 13 scope combinations used for per-thread deduplication
  • scopesForThread(int) – returns a mutable scope list for a given thread index
  • SilentTokenRequester – interface the caller implements to issue one MSAL call per (thread, iteration) slot
  • StressResult – holds the completion flag and error list
  • run(...) – the CyclicBarrier/CountDownLatch orchestration; callers just pass the thread/iteration counts, timeouts, and a SilentTokenRequester lambda

TestCaseConcurrentAcquireTokenSilent now only contains MSAL setup, the per-request callback lambda, and assertions — all barrier/latch/thread management is in the helper.

ConcurrentAcquireTokenExecutor in the testapp can't share compiled code (different Gradle modules with no common dependency), but its Javadoc now explicitly notes the parallel design with the automation helper so both are easy to keep in sync.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • dl.google.com
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-opens=java.base/java.nio.charset=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.xml/javax.xml.namespace=ALL-UNNAMED -XX:MaxMetaspaceSize=512m -XX:+HeapDumpOnOutOfMemoryError -Xmx2048m -Dfile.encoding=UTF-8 -Duser.country -Duser.language=en -Duser.variant (dns block)
  • www.puppycrawl.com
    • Triggering command: /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/java/bin/java /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/java/bin/java -jar /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/xml/tools/xml-extractor.jar --fileList=/tmp/codeql-scratch-38ad52fff39af1e6/dbs/java/working/files-to-index11400206081483856222.list --sourceArchiveDir=/tmp/codeql-scratch-38ad52fff39af1e6/dbs/java/src --outputDir=/tmp/codeql-scratch-38ad52fff39af1e6/dbs/java/trap/java (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@rpdome
Copy link
Copy Markdown
Member

rpdome commented Apr 30, 2026

@copilot can we move the shared code into uiautomationutilities and have the test app consume it? What's the downside?

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 30, 2026

@copilot can we move the shared code into uiautomationutilities and have the test app consume it? What's the downside?

Technically possible, but the downsides are meaningful:

  1. Cross-repo coordination required. uiautomationutilities lives in the common submodule (microsoft-authentication-library-common-for-android). Any addition there needs a separate PR in that repo plus a submodule bump here — significant overhead for a small utility class.

  2. New test-infrastructure dependency in the testapp. The testapp currently has zero test library dependencies (no :uiautomationutilities, no Espresso, no UiAutomator). Adding it as implementation would pull all of that into the production APK. Adding it as androidTestImplementation wouldn't help because ConcurrentAcquireTokenExecutor lives in the main source set (it powers a UI button, not an instrumentation test).

  3. Only a thin slice could actually be shared. ConcurrentAcquireTokenExecutor depends on testapp-specific types (MsalWrapper, RequestOptions, INotifyOperationResultCallback, etc.) that can't move to uiautomationutilities. Only the bare THREAD_SCOPES array and the CyclicBarrier/CountDownLatch skeleton (~80 lines of pure Java) would be shared — not worth the cross-repo overhead.

  4. Muddies uiautomationutilities's purpose. That module is generic UI-automation plumbing (prompt handlers, activity/device helpers). Adding MSAL-specific concurrency stress logic there conflates two different concerns.

The current approach — both sides implement the same pattern, with Javadoc cross-references — avoids all of the above while keeping the code trivially readable in each module. Happy to make the change if you feel the reuse outweighs these costs.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 30, 2026

@copilot do we still need total timeout? If not lets get rid of it

Removed in commit 85f1ca2. totalTimeoutSec is no longer a parameter of ConcurrentAcquireTokenSilentHelper.run(). The per-wave timeout already guarantees every thread exits (a timed-out wave sets stopped and resets the barrier, causing all threads to break into their finally/countDown()). The helper now derives the safety backstop internally as iterations × perWaveTimeoutSec so an unexpected hang is still bounded, but callers no longer need to think about it.

…estapp/ConcurrentAcquireTokenExecutor.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown

❌ Invalid work item number: AB#3582859

##. Work item number must be a valid integer.

Click here to learn more.

1 similar comment
@github-actions
Copy link
Copy Markdown

❌ Invalid work item number: AB#3582859

##. Work item number must be a valid integer.

Click here to learn more.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 30, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • dl.google.com
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-opens=java.base/java.nio.charset=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.xml/javax.xml.namespace=ALL-UNNAMED -XX:MaxMetaspaceSize=512m -XX:&#43;HeapDumpOnOutOfMemoryError -Xmx2048m -Dfile.encoding=UTF-8 -Duser.country -Duser.language=en -Duser.variant (dns block)
  • www.puppycrawl.com
    • Triggering command: /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/java/bin/java /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/java/bin/java -jar /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/xml/tools/xml-extractor.jar --fileList=/tmp/codeql-scratch-38ad52fff39af1e6/dbs/java/working/files-to-index6674653172767394534.list --sourceArchiveDir=/tmp/codeql-scratch-38ad52fff39af1e6/dbs/java/src --outputDir=/tmp/codeql-scratch-38ad52fff39af1e6/dbs/java/trap/java (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@github-actions
Copy link
Copy Markdown

❌ Work item link check failed. Description does not contain AB#{ID}.

Click here to Learn more.

@github-actions
Copy link
Copy Markdown

✅ Work item link check complete. Description contains link AB#3582859 to an Azure Boards work item.

@github-actions github-actions Bot changed the title Add e2e concurrent/stress test for AcquireTokenSilent via broker Add e2e concurrent/stress test for AcquireTokenSilent via broker, Fixes AB#3582859 Apr 30, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@rpdome
Copy link
Copy Markdown
Member

rpdome commented Apr 30, 2026

@copilot apply changes based on the comments in this thread

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 30, 2026

@copilot apply changes based on the comments in this thread

Done in commit 79ad944. Applied all four items from that review:

  1. Input validationrun() now throws IllegalArgumentException early if threadCount, iterations, or perWaveTimeoutSec is ≤ 0.
  2. InterruptedException separated – the barrier catch block now catches InterruptedException first and calls Thread.currentThread().interrupt() before breaking, so the interrupt signal is preserved.
  3. requester.request() wrapped – a try/catch(Throwable) around the dispatch call records the exception as an error string and counts down the wave latch once, ensuring the wave can time out cleanly instead of hanging.
  4. Richer error messagesonError now logs exception.getErrorCode() alongside exception.toString() so triage output includes the MSAL error code.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • dl.google.com
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-opens=java.base/java.nio.charset=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.xml/javax.xml.namespace=ALL-UNNAMED -XX:MaxMetaspaceSize=512m -XX:&#43;HeapDumpOnOutOfMemoryError -Xmx2048m -Dfile.encoding=UTF-8 -Duser.country -Duser.language=en -Duser.variant (dns block)
  • www.puppycrawl.com
    • Triggering command: /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/java/bin/java /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/java/bin/java -jar /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/xml/tools/xml-extractor.jar --fileList=/tmp/codeql-scratch-38ad52fff39af1e6/dbs/java/working/files-to-index15710701697670083590.list --sourceArchiveDir=/tmp/codeql-scratch-38ad52fff39af1e6/dbs/java/src --outputDir=/tmp/codeql-scratch-38ad52fff39af1e6/dbs/java/trap/java (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@rpdome rpdome requested a review from a team as a code owner April 30, 2026 17:04
@github-actions
Copy link
Copy Markdown

❌ Work item link check failed. Description does not contain AB#{ID}.

Click here to Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants