dynamic_modules: remote source: add caching for fetched modules by kanurag94 · Pull Request #43974 · envoyproxy/envoy

kanurag94 · 2026-03-16T10:56:23Z

Commit Message: dynamic_modules: remote source: add caching for fetched modules

Motivation
Follow up from #43818 and #43333
Next PR: nack_on_cache_miss support (NACK on cache miss + background fetch)

Additional Description:

newDynamicModuleFromBytes already writes fetched modules to a deterministic path based on SHA256. This PR adds a check before the async fetch: if the file already exists on disk, load it directly and skip the HTTP fetch entirely.

It is a simple approach that forms base for the next PR.

Risk Level: Low
Testing: Added unit tests
Docs Changes: NA
Release Notes: Added
Platform Specific Features: NA
[Optional Runtime guard:] Not needed
[Optional Fixes #Issue]: NA
[Optional Fixes commit #PR or SHA]: NA
[Optional Deprecated:]: NA
[Optional API Considerations:] NA

Signed-off-by: Anurag Aggarwal <kanurag94@gmail.com>

repokitteh-read-only · 2026-03-16T10:56:29Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #43974 was opened by kanurag94.

see: more, trace.

Signed-off-by: Anurag Aggarwal <kanurag94@gmail.com>

kanurag94 · 2026-03-17T05:32:24Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces an in-memory cache for remotely fetched dynamic modules, which is a great performance improvement. The implementation is straightforward, adding the caching logic at the factory level to avoid redundant HTTP fetches for modules with the same SHA256. The cache invalidation logic for missing or unloadable files is also handled correctly. The added unit tests are comprehensive and cover both cache hits and cache invalidation scenarios.

I have one suggestion regarding code maintainability, related to how the path for the cached module is determined. Overall, this is a solid contribution.

gemini-code-assist · 2026-03-17T05:35:29Z

+        auto cached_path = std::filesystem::temp_directory_path() /
+                           fmt::format("envoy_dynamic_module_{}.so", fetch_sha256);
+        remote_module_cache_[fetch_sha256] = cached_path.string();


This code reconstructs the path to the temporary file where the dynamic module was saved by newDynamicModuleFromBytes. This path generation logic appears to be duplicated from what newDynamicModuleFromBytes does internally, which creates a fragile dependency on its implementation details. If the file naming convention in newDynamicModuleFromBytes were to change, this caching logic would break.

A more robust approach would be for newDynamicModuleFromBytes to return the path of the created file along with the module pointer, or for the DynamicModule object to store the path from which it was loaded. This would eliminate the need to reconstruct the path here.

While changing newDynamicModuleFromBytes might be out of scope for this PR, it would be beneficial to centralize the path generation logic into a shared helper function to reduce duplication and improve long-term maintainability.

Signed-off-by: Anurag Aggarwal <kanurag94@gmail.com>

kanurag94 · 2026-03-17T06:17:47Z

/retest

wbpcode

Thanks for this great contribution. And tow comments are added. :)

wbpcode · 2026-03-17T07:36:05Z

+      auto cache_it = remote_module_cache_.find(sha256);
+      if (cache_it != remote_module_cache_.end()) {


Do we need the remote_module_cache_? I think we may could only need to check the std::filesystem::exists(cached_path) and if the file is there, then the cache hit.

Thank you so much for the review.
This makes sense. I was somehow trying to keep nack_on_cache_miss in my mind and not introduce a global cache key. But I think that's not avoidable in this PR.

wbpcode · 2026-03-17T07:37:21Z

+          if (dynamic_module.ok()) {
+            // Cache hit — skip async fetch entirely.
+            return buildFilterFactoryCallback(std::move(dynamic_module.value()), proto_config,
+                                              module_config, context, scope);
+          }


I don't think we need the fallback because the hash of the file is used for caching. The re-fecthing won't resolve the problem if the file/content is invalid?

Signed-off-by: Anurag Aggarwal <kanurag94@gmail.com>

paul-r-gall · 2026-03-17T12:43:18Z

/assign @wbpcode

wbpcode

LGTM. Thanks.

…yproxy#43974) **Commit Message**: dynamic_modules: remote source: add caching for fetched modules **Motivation** Follow up from envoyproxy#43818 and envoyproxy#43333 Next PR: `nack_on_cache_miss` support (NACK on cache miss + background fetch) **Additional Description**: `newDynamicModuleFromBytes` already writes fetched modules to a deterministic path based on SHA256. This PR adds a check before the async fetch: if the file already exists on disk, load it directly and skip the HTTP fetch entirely. It is a simple approach that forms base for the next PR. **Risk Level**: Low **Testing**: Added unit tests **Docs Changes**: NA **Release Notes**: Added Platform Specific Features: NA [Optional Runtime guard:] Not needed [Optional Fixes #Issue]: NA [Optional Fixes commit #PR or SHA]: NA [Optional Deprecated:]: NA [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] NA --------- Signed-off-by: Anurag Aggarwal <kanurag94@gmail.com>

…yproxy#43974) **Commit Message**: dynamic_modules: remote source: add caching for fetched modules **Motivation** Follow up from envoyproxy#43818 and envoyproxy#43333 Next PR: `nack_on_cache_miss` support (NACK on cache miss + background fetch) **Additional Description**: `newDynamicModuleFromBytes` already writes fetched modules to a deterministic path based on SHA256. This PR adds a check before the async fetch: if the file already exists on disk, load it directly and skip the HTTP fetch entirely. It is a simple approach that forms base for the next PR. **Risk Level**: Low **Testing**: Added unit tests **Docs Changes**: NA **Release Notes**: Added Platform Specific Features: NA [Optional Runtime guard:] Not needed [Optional Fixes #Issue]: NA [Optional Fixes commit #PR or SHA]: NA [Optional Deprecated:]: NA [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] NA --------- Signed-off-by: Anurag Aggarwal <kanurag94@gmail.com> Signed-off-by: Xuyang Tao <taoxuy@google.com>

…yproxy#43974) **Commit Message**: dynamic_modules: remote source: add caching for fetched modules **Motivation** Follow up from envoyproxy#43818 and envoyproxy#43333 Next PR: `nack_on_cache_miss` support (NACK on cache miss + background fetch) **Additional Description**: `newDynamicModuleFromBytes` already writes fetched modules to a deterministic path based on SHA256. This PR adds a check before the async fetch: if the file already exists on disk, load it directly and skip the HTTP fetch entirely. It is a simple approach that forms base for the next PR. **Risk Level**: Low **Testing**: Added unit tests **Docs Changes**: NA **Release Notes**: Added Platform Specific Features: NA [Optional Runtime guard:] Not needed [Optional Fixes #Issue]: NA [Optional Fixes commit #PR or SHA]: NA [Optional Deprecated:]: NA [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] NA --------- Signed-off-by: Anurag Aggarwal <kanurag94@gmail.com> Signed-off-by: Nick Shipilov <nick.shipilov.n@gmail.com>

…yproxy#43974) **Commit Message**: dynamic_modules: remote source: add caching for fetched modules **Motivation** Follow up from envoyproxy#43818 and envoyproxy#43333 Next PR: `nack_on_cache_miss` support (NACK on cache miss + background fetch) **Additional Description**: `newDynamicModuleFromBytes` already writes fetched modules to a deterministic path based on SHA256. This PR adds a check before the async fetch: if the file already exists on disk, load it directly and skip the HTTP fetch entirely. It is a simple approach that forms base for the next PR. **Risk Level**: Low **Testing**: Added unit tests **Docs Changes**: NA **Release Notes**: Added Platform Specific Features: NA [Optional Runtime guard:] Not needed [Optional Fixes #Issue]: NA [Optional Fixes commit #PR or SHA]: NA [Optional Deprecated:]: NA [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] NA --------- Signed-off-by: Anurag Aggarwal <kanurag94@gmail.com>

dynamic_modules: remote source: add caching for fetched modules

69465da

Signed-off-by: Anurag Aggarwal <kanurag94@gmail.com>

run format

9302b5c

Signed-off-by: Anurag Aggarwal <kanurag94@gmail.com>

kanurag94 marked this pull request as ready for review March 17, 2026 05:32

kanurag94 requested review from agrawroh, mathetake, mattklein123 and wbpcode as code owners March 17, 2026 05:32

gemini-code-assist bot reviewed Mar 17, 2026

View reviewed changes

review: shared method for temp file name

0c496b6

Signed-off-by: Anurag Aggarwal <kanurag94@gmail.com>

wbpcode reviewed Mar 17, 2026

View reviewed changes

review: simplify cache

06573cc

Signed-off-by: Anurag Aggarwal <kanurag94@gmail.com>

repokitteh-read-only bot assigned wbpcode Mar 17, 2026

kanurag94 requested a review from wbpcode March 18, 2026 02:36

wbpcode approved these changes Mar 18, 2026

View reviewed changes

wbpcode merged commit 4be2167 into envoyproxy:main Mar 18, 2026
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dynamic_modules: remote source: add caching for fetched modules#43974

dynamic_modules: remote source: add caching for fetched modules#43974
wbpcode merged 4 commits intoenvoyproxy:mainfrom
kanurag94:dym-local-cache

kanurag94 commented Mar 16, 2026 •

edited

Loading

Uh oh!

repokitteh-read-only bot commented Mar 16, 2026

Uh oh!

kanurag94 commented Mar 17, 2026

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Mar 17, 2026

Uh oh!

kanurag94 commented Mar 17, 2026

Uh oh!

wbpcode left a comment

Uh oh!

wbpcode Mar 17, 2026

Uh oh!

kanurag94 Mar 17, 2026

Uh oh!

wbpcode Mar 17, 2026

Uh oh!

paul-r-gall commented Mar 17, 2026

Uh oh!

wbpcode left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		auto cache_it = remote_module_cache_.find(sha256);
		if (cache_it != remote_module_cache_.end()) {

Conversation

kanurag94 commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Mar 16, 2026

Uh oh!

kanurag94 commented Mar 17, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

kanurag94 commented Mar 17, 2026

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

kanurag94 Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

wbpcode Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

paul-r-gall commented Mar 17, 2026

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kanurag94 commented Mar 16, 2026 •

edited

Loading