Optimization for concurrent Key Vault Ref resolution - deduplicate requests to the same secret#328
Optimization for concurrent Key Vault Ref resolution - deduplicate requests to the same secret#328linglingye001 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds in-flight request deduplication to Key Vault secret resolution so that concurrent configuration settings referencing the same secret URI don’t trigger redundant Key Vault (or resolver) calls.
Changes:
- Added an in-flight promise map in
AzureKeyVaultSecretProvider.getSecretValue()to deduplicate concurrent secret fetches by secret identifier. - Added test coverage validating deduplication in parallel and sequential modes, secret-version independence, failure behavior, and refresh behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/keyvault/keyVaultSecretProvider.ts |
Introduces in-flight request deduplication for identical secret identifiers and ensures in-flight entries are cleared after completion. |
test/keyvault.test.ts |
Adds a new test suite covering request deduplication behavior across parallel/sequential resolution, versions, failures, and refresh rounds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await sleepInMs(60_000 + 100); | ||
| await settings.refresh(); |
linglingye001
force-pushed
the
linglingye/dedup-key-vault-ref
branch
from
03023cd to
0e635bb
Compare
| // Resolve every reference. In parallel mode the values are served from the warmed cache | ||
| // without any additional I/O. | ||
| for (const setting of secretReferences) { | ||
| const [key, value] = await this.#processKeyValue(setting); | ||
| resultHandler(key, value); | ||
| } |
| if (secretId === undefined) { | ||
| // The reference cannot be parsed; resolve it individually so the appropriate error | ||
| // is surfaced when it is processed below. | ||
| uniqueSecretReferences.push(setting); | ||
| } else if (!seenSecretIds.has(secretId)) { | ||
| seenSecretIds.add(secretId); | ||
| uniqueSecretReferences.push(setting); | ||
| } |
| // Return the cached value if available. The cache is invalidated externally (on secret refresh | ||
| // or when a key-value change is detected) so a stale value is never served. |
| // After the secret refresh interval elapses, the refresh round re-fetches once. | ||
| await sleepInMs(60_000 + 100); | ||
| await settings.refresh(); |
| * or undefined if the reference cannot be parsed. Used to deduplicate references that resolve to | ||
| * the same secret before resolving them. | ||
| */ | ||
| getSecretReferenceId(setting: ConfigurationSetting): string | undefined { |
There was a problem hiding this comment.
This is a stateless operation which will be used outside. It should not be a method under key vault kv adapter.
| } | ||
|
|
||
| // Invalidate cached secret values so the refresh round re-fetches them from Key Vault. | ||
| this.#keyVaultAdapter.clearCache(); |
There was a problem hiding this comment.
With the introduction of features such as secret refresh, parallel loading, and deduplication, AppConfigurationImpl now needs more fine-grained control over secret loading and caching.
When KeyValueAdapter.onChangeDetected is available, but we bypass it and directly call SecretProvider or Adapter.clearCache, I do not think the existing KeyVaultKeyValueAdapter abstraction remains a sound design.
There was a problem hiding this comment.
I think we can introduce preload(settings: ConfigurationSetting[]): Promise<void>; to IKeyVaultAdapter
This method will dedup and warm the secret cache.
SecretProvider needs to be updated. getSecretValue should now only serve secret value from cache (if no cache is found, fallback to send request), the timer gate should be removed.
The timer gate should apply for the preload path only.
No description provided.