Retry KMS requests on transient errors - #1953
Conversation
Add libmongocrypt CAPI bindings for KMS retry support and wire retry logic through the sync and reactive driver stacks. Transient KMS HTTP and network errors are retried with backoff delays managed by libmongocrypt; retry is enabled unconditionally. - Add native bindings: mongocrypt_setopt_retry_kms, mongocrypt_kms_ctx_usleep, mongocrypt_kms_ctx_feed_with_retry, mongocrypt_kms_ctx_fail - Add sleepMicroseconds(), feedAndRetry(), fail() to MongoKeyDecryptor - Enable KMS retry unconditionally in MongoCryptImpl - Rewrite sync Crypt.decryptKey() with retry loop, timeout-aware - Add retry logic to reactive KeyManagementService.decryptKey() - Fix TlsChannelImpl.read() to preserve bytes delivered alongside close_notify (already fixed upstream in marianobarrios/tls-channel) - Add spec Section 24 KMS retry integration tests (sync + reactive) - Add Evergreen CI task for KMS retry tests JAVA-5391
There was a problem hiding this comment.
Pull request overview
Adds libmongocrypt-backed retry support for KMS requests and wires it through the sync and reactive driver encryption flows, including a small TLS-channel fix needed for correct KMS response handling and new CI coverage for the retry prose tests.
Changes:
- Introduce new libmongocrypt CAPI/JNA bindings and surface them via
MongoKeyDecryptorto drive sleep/backoff and retry decisions. - Implement retry loops in sync
Crypt.decryptKey()and reactiveKeyManagementService.decryptKey()using libmongocrypt’s retry signals and operation-timeout awareness. - Add KMS retry prose tests (sync + reactive) and an Evergreen task/script to run them.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| mongodb-crypt/src/main/com/mongodb/internal/crypt/capi/MongoKeyDecryptorImpl.java | Implements new retry-related native calls (usleep, feed_with_retry, fail) on the KMS ctx wrapper. |
| mongodb-crypt/src/main/com/mongodb/internal/crypt/capi/MongoKeyDecryptor.java | Extends decryptor API with retry hooks and a default initial KMS read size constant. |
| mongodb-crypt/src/main/com/mongodb/internal/crypt/capi/MongoCryptImpl.java | Enables KMS retry option in libmongocrypt during initialization. |
| mongodb-crypt/src/main/com/mongodb/internal/crypt/capi/CAPI.java | Adds JNA native bindings for KMS retry APIs. |
| driver-sync/src/main/com/mongodb/client/internal/Crypt.java | Reworks KMS decryption to loop retries with backoff and timeout checks. |
| driver-reactive-streams/src/main/com/mongodb/reactivestreams/client/internal/crypt/KeyManagementService.java | Adds reactive retry flow with libmongocrypt-provided delay and retry decisions. |
| driver-core/src/main/com/mongodb/internal/connection/tlschannel/impl/TlsChannelImpl.java | Preserves bytes produced alongside TLS close_notify instead of immediately returning -1. |
| driver-sync/src/test/functional/com/mongodb/client/AbstractClientSideEncryptionKmsRetryProseTest.java | Adds shared prose tests for KMS retry behaviors (TCP/HTTP retry, exhausted retries, timeout mid-retry). |
| driver-sync/src/test/functional/com/mongodb/client/ClientSideEncryptionKmsRetryProseTest.java | Sync concrete test wiring for the shared retry prose tests. |
| driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/ClientSideEncryptionKmsRetryProseTest.java | Reactive concrete test wiring (via sync adapter) for the shared retry prose tests. |
| .evergreen/run-kms-retry-tests.sh | Adds CI script to run sync + reactive KMS retry prose tests with required trust material. |
| .evergreen/.evg.yml | Adds Evergreen function/task/buildvariant wiring to execute the KMS retry test script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Assigned |
stIncMale
left a comment
stIncMale
left a comment
There was a problem hiding this comment.
Some questions and suggestions after a partial review (the smaller part of it).
| bytesToReturn = res.bytesProduced; | ||
| if (res.wasClosed) { | ||
| return -1; | ||
| // JAVA-5391: return any bytes produced alongside close_notify; the next read | ||
| // sees shutdownReceived and returns -1. Fixed in upstream marianobarrios/tls-channel; | ||
| // this is the minimal patch until the vendored snapshot is refreshed. | ||
| return bytesToReturn > 0 ? bytesToReturn : -1; |
There was a problem hiding this comment.
Could you please
- Either explain the problem in this GitHub thread, or link to an existing explanation / bug report.
- Also explain why the fix is required for the current PR.
- Share a link to the upstream PR/commit with the fix.
I think, given that this change is in a vendored code, we should do it in a separate PR, to avoid complicating the future work of updating the vendored code.
There was a problem hiding this comment.
Yes it was causing the tests to fail (I can't recall which one) but it was not returning bytes that needed to be read. Most likely the mock kms server.
There was a problem hiding this comment.
Ah yes it was:
ClientSideEncryptionKmsRetryProseTest > testCreateDataKeyAndEncryptWithHttpRetry(String) > Case 2: HTTP retry with aws FAILED
If causes: MongoClientException: Exception in encryption library: Allocation size must be greater than zero.
It doesn't happen on sync because it uses SSLSocket/InputStream, which correctly delivers data before signalling EOF on close_notify.
It isn't triggered by gcp or azure because the failpoint fires on the OAuth token request. So the actual key operation uses another connection and succeeds. As AWS doesn't use a token, and fails with data unconsumed on the wire.
According to Claude:
The TLS spec allows close_notify to arrive in the same flight as application data. > Any real-world TLS server can legitimately:
- Send Connection: close and the response in one write (common for HTTP/1.0 or error responses)
- Send a response and immediately initiate TLS shutdown
- Have the OS coalesce the response and close_notify into a single TCP segment due to Nagle's algorithm or buffering
TLSChannelImpl fixed this behaviour in 0.5.0
There was a problem hiding this comment.
I am pausing the review until we address the somewhat structurally important concerns:
- #1953 (comment)
- #1953 (comment)
- #1953 (comment)
- #1953 (comment) - the tests should pass. Update: they pass.
…nd added a rule to AGENTS.md
Add libmongocrypt CAPI bindings for KMS retry support and wire retry logic through the sync and reactive driver stacks. Transient KMS HTTP and network errors are retried with backoff delays managed by libmongocrypt; retry is enabled unconditionally.
JAVA-5391