Skip to content

fix: set a 2 second API call attempt timeout on clients the cache builds - #201

Draft
SushaanthSrinivasan wants to merge 1 commit into
v2from
default-request-timeout
Draft

SushaanthSrinivasan wants to merge 1 commit into
v2from
default-request-timeout

Conversation

@SushaanthSrinivasan

Copy link
Copy Markdown

Description

Why is this change being made?

  1. The SDK sets no API call attempt timeout by default. That includes the Secrets Manager client the cache builds itself: with new SecretCache(), with a SecretsManagerClientBuilder passed in, or with a SecretCacheConfiguration that has no client. The only limit on an attempt is the HTTP client's socket timeout, which is 30 seconds for the default Apache client. If a Secrets Manager endpoint accepts the connection and then stops responding, each attempt waits 30 seconds, and with the SDK's default retries getSecretString can block the caller for about 2 minutes.
  2. DescribeSecret and GetSecretValue normally complete in well under 2 seconds. So a 2-second limit catches a connection that has stopped responding without cutting off normal calls. A timed out attempt is retried like any other network error, so one bad connection costs about 2 seconds rather than failing the call.

What is changing?

  1. There is a new constant, SecretCacheConfiguration.DEFAULT_API_CALL_ATTEMPT_TIMEOUT, set to 2 seconds.
  2. When the cache builds the client, it sets apiCallAttemptTimeout to that value, unless the builder passed in already sets its own attempt timeout. This covers new SecretCache(), new SecretCache(SecretsManagerClientBuilder), and new SecretCache(SecretCacheConfiguration) with no client, including the Post-Quantum TLS client. A built SecretsManagerClient passed in is used unchanged.
  3. The SDK needs a scheduled executor to enforce the attempt timeout. If a client isn't given one, the SDK starts a pool of timer threads for that client, and they stop only when the client is closed. The cache doesn't close its client, so all clients the cache builds share one executor instead, unless their builder sets its own. It has one daemon thread that starts on the first timed request and exits after a minute with nothing to do. Its context class loader is the library's, so it doesn't keep an application's class loader alive, and cancelled timeouts are removed right away.
  4. close() is unchanged: it clears the cache and doesn't close the client.
  5. The README has a new "Request Timeouts" section.

Behavior changes:

  • Clients the cache builds now give up on an attempt after 2 seconds. With the SDK's default retries, a call to an endpoint that never answers fails after about 9 seconds instead of about 2 minutes.
  • The settings are written into a SecretsManagerClientBuilder passed in, as the user agent already is, so other clients built from that builder later get them too.
  • The SDK rejects a zero attempt timeout, so no value turns it off. To allow slower calls, pass in a builder with a larger apiCallAttemptTimeout, or pass in a built client. Setting only apiCallTimeout or HTTP client timeouts on the builder doesn't replace the 2-second default.

There are no API changes beyond the new constant.

Related Links

  • Issue #, if available: N/A

Testing

How was this tested?

  1. New tests in SecretCacheTest:

    • testDefaultApiCallAttemptTimeout: the constant is 2 seconds.
    • testBuilderClientTimesOutOnHungEndpoint: points a builder passed in at a local socket that accepts the connection but never responds. With retries off, the call fails with ApiCallAttemptTimeoutException after 2 to 5 seconds.
    • testBuilderClientRetriesTimedOutAttempt: with 2 attempts, the same call takes 4 to 8 seconds, so the timed out attempt was retried.
    • testBuilderKeepsOwnAttemptTimeout: a builder that sets 200 ms keeps it.
    • testDefaultClientTimesOutOnHungEndpoint: a SecretCacheConfiguration with no client, configured only through system properties, also times out after 2 to 5 seconds.
    • testCachesShareOneTimeoutExecutor: six caches start no SDK timer threads (sdk-ScheduledExecutor), and exactly one cache timer thread exists. It's a daemon and uses the library's class loader.
    • testCacheStillWorksAfterClose: close() doesn't close the client, and the cache still works afterwards.

    The hung-endpoint tests use static dummy credentials and a local endpoint, so they need no AWS account.

  2. Each new test fails without the change.

  3. mvn package passes locally with JDK 21 (Corretto), with 0 SpotBugs issues and 0 Checkstyle violations.

When testing locally, provide testing artifact(s):

  1. Build:
    $ mvn -B clean package
    [INFO] Tests run: 43, Failures: 0, Errors: 0, Skipped: 0
    [INFO] You have 0 Checkstyle violations.
    [INFO] BugInstance size is 0
    [INFO] BUILD SUCCESS
    

Reviewee Checklist

Update the checklist after submitting the PR

  • I have reviewed, tested and understand all changes
    If not, why:
  • I have filled out the Description and Testing sections above
    If not, why:
  • Build and Unit tests are passing
    If not, why:
  • Unit test coverage check is passing
    If not, why: Codecov runs in CI on this PR.
  • I have ensured no sensitive information is leaking (i.e., no logging of sensitive fields, or otherwise)
    If not, why:
  • I have added explanatory comments for complex logic, new classes/methods and new tests
    If not, why:
  • I have updated README/documentation (if needed)
    If not, why:
  • I have clearly called out breaking changes (if any)
    If not, why:

Reviewer Checklist

All reviewers please ensure the following are true before reviewing:

  • Reviewee checklist has been accurately filled out
  • Code changes align with stated purpose in description
  • Test coverage adequately validates the changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The SDK sets no attempt timeout by default, so an endpoint that accepts
the connection and never answers could block getSecretString for about
2 minutes across retries. Clients the cache builds now get a 2 second
apiCallAttemptTimeout unless their builder sets one, and share one
daemon timer thread. A built client passed in is used unchanged.
@codecov

codecov Bot commented Sep 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.59%. Comparing base (dc47d68) to head (db109e5).

Files with missing lines Patch % Lines
.../amazonaws/secretsmanager/caching/SecretCache.java 95.23% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                 v2     #201      +/-   ##
============================================
+ Coverage     91.83%   94.59%   +2.75%     
- Complexity       97      103       +6     
============================================
  Files             6        6              
  Lines           245      259      +14     
  Branches         26       28       +2     
============================================
+ Hits            225      245      +20     
+ Misses           18       12       -6     
  Partials          2        2              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant