fix(proxy): preserve master_key set by initialize() during lifespan startup - #22483
fix(proxy): preserve master_key set by initialize() during lifespan startup#22483giulio-leone wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR fixes a one-line bug in
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| tests/test_litellm/proxy/test_preserve_master_key.py | New regression test correctly uses async with to enter the @asynccontextmanager and patches all external dependencies; premium_user module-level state is not restored in the finally block, which may pollute subsequent tests. |
| litellm/proxy/proxy_server.py | Minimal, correct fix: the master_key env-var assignment is now guarded by if _env_master_key is not None, preserving any value previously set by initialize() when the env var is absent. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[proxy_startup_event starts] --> B[init_verbose_loggers]
B --> C{premium_user is False?}
C -- Yes --> D[premium_user = license_check.is_premium]
C -- No --> E[Read LITELLM_MASTER_KEY env var]
D --> E
E --> F{env var result is not None?}
F -- Yes --> G[master_key = env var value\nenv var takes precedence]
F -- No --> H[master_key unchanged\nconfig-provided value preserved]
G --> I[Load config and DB setup]
H --> I
Last reviewed commit: cd6241c
There was a problem hiding this comment.
Pull request overview
Fixes a bug where proxy_startup_event() unconditionally overwrote the global master_key with the result of get_secret_str("LITELLM_MASTER_KEY"), which could return None when the env var is not set — discarding any value previously set by initialize().
Changes:
- Guard the
master_keyassignment so it only overwrites the existing value whenLITELLM_MASTER_KEYenv var is actually set
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for the summary — accurate assessment. |
|
Quick intervention note to unblock this PR efficiently: I’m seeing a repeated failure pattern across the latest LiteLLM PRs:
Recommended resolution order (lowest risk):
If helpful, I can post a per-PR checklist with the first failing job links and a minimal fix order. |
|
All review feedback has been addressed in the latest push. The PR is ready for re-review and merge. Thank you! 🙏 |
Verifies that a master_key set by initialize() from config is not overwritten when LITELLM_MASTER_KEY env var is absent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| if _env_master_key: | ||
| master_key = _env_master_key |
There was a problem hiding this comment.
Use is not None for explicit env-var-presence check
The truthiness guard if _env_master_key: treats an empty string the same as None. If LITELLM_MASTER_KEY is explicitly set to an empty string in the environment, the current check would silently ignore it and preserve the config-provided value. For a security credential, an explicit is not None check is more semantically precise — it only falls back to the config value when the env var is genuinely absent (returns None), not when it is falsy.
Consider changing the guard to:
if _env_master_key is not None:- Test used 'await' on @asynccontextmanager which never enters the body; changed to 'async with' so the startup code actually executes. - Changed truthiness check to 'is not None' for master_key guard to correctly handle empty-string edge case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Closing to reduce PR volume. The fix remains valid — happy to resubmit individually if the team finds it useful. |
Summary
Fixes a bug where
proxy_startup_event()unconditionally overwrote the globalmaster_keywith the result ofget_secret_str("LITELLM_MASTER_KEY"), which returnsNonewhen the env var is not set. This caused themaster_keypreviously set byinitialize()to be lost.Changes
master_keyif the env var lookup returns a non-None valueSupersedes #22396 (rebased on main)