Skip to content

fix(modal): reuse env secrets across sandbox operations#1806

Closed
EazyReal wants to merge 1 commit into
harbor-framework:mainfrom
VmaxAI:modal-secret-cache
Closed

fix(modal): reuse env secrets across sandbox operations#1806
EazyReal wants to merge 1 commit into
harbor-framework:mainfrom
VmaxAI:modal-secret-cache

Conversation

@EazyReal

@EazyReal EazyReal commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Problem

The Modal environment injects env into sandboxes via Secret.from_dict, which creates an ephemeral Modal Secret per object. Under high concurrency, identical env payloads produced a fresh Secret on every sandbox startup and every exec, so a startup/setup burst could trip Modal's workspace-wide secret create-rate limit even though the env payload never changed.

Fix

Cache Secret.from_dict objects by an order-independent env-payload key and reuse them for both sandbox creation (_secrets_config) and per-command execution (_sdk_exec). Named Modal secrets (Secret.from_name) are untouched.

Tests

tests/unit/environments/test_modal.py::TestEnvSecretCache covers same-payload reuse, distinct-payload separation, and reuse through both _secrets_config and _sdk_exec.

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Enjoy a better diff viewing experience by clicking one of these URLs:

Modal creates an ephemeral Secret for every Secret.from_dict object.
Under high concurrency, identical env payloads produced a fresh Secret
per sandbox startup and per exec, so a startup/setup burst could hit
Modal's workspace-wide secret create-rate limit even though the env
payload never changed.

Cache Secret.from_dict objects by an order-independent env-payload key
and reuse them for both sandbox creation (_secrets_config) and per-exec
(_sdk_exec). Named secrets are unchanged.
@EazyReal EazyReal force-pushed the modal-secret-cache branch from 25d8424 to 207f07d Compare June 17, 2026 06:49
@EazyReal EazyReal closed this Jun 17, 2026
@EazyReal EazyReal reopened this Jun 17, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

Open in Devin Review

Comment on lines +78 to +79
_ENV_SECRET_CACHE: dict[_EnvSecretCacheKey, Any] = {}
_ENV_SECRET_CACHE_LOCK = Lock()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚩 Module-level cache grows without bound and retains secrets indefinitely

The _ENV_SECRET_CACHE dict at src/harbor/environments/modal.py:78 is never evicted or cleared during the lifetime of a process. Each unique env-var payload (from _secrets_config for persistent env, and from _sdk_exec for per-exec merged env) adds an entry. In typical usage (one persistent env per environment, compose env vars per DinD environment), the number of entries is bounded by the number of distinct environments, which is manageable. However, in a long-running orchestrator process that runs many jobs with varying --ae flags over time, entries accumulate indefinitely. The cached Secret objects hold references to env var dicts that may contain sensitive tokens/API keys, keeping them in memory longer than necessary. Consider adding a bounded cache (e.g., functools.lru_cache or an explicit TTL/size limit) if this is expected to run in long-lived processes.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@EazyReal

Copy link
Copy Markdown
Contributor Author

Recreated as # for visibility (same branch, rebased on latest main).

@EazyReal

Copy link
Copy Markdown
Contributor Author

Recreated as #1969 for visibility (rebased on latest main).

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.

1 participant