Skip to content

fix: guard litellm modify_params context#3248

Merged
csmith49 merged 1 commit into
OpenHands:mainfrom
he-yufeng:fix/litellm-modify-params-lock
May 14, 2026
Merged

fix: guard litellm modify_params context#3248
csmith49 merged 1 commit into
OpenHands:mainfrom
he-yufeng:fix/litellm-modify-params-lock

Conversation

@he-yufeng

Copy link
Copy Markdown
Contributor

Summary

  • serialize litellm.modify_params save/set/restore with an RLock
  • cover the context manager with a threaded regression test
  • keep existing retry/completion paths unchanged

Fixes #3137.

To verify

  • python -m py_compile openhands-sdk\openhands\sdk\llm\llm.py tests\sdk\llm\test_llm_completion.py
  • python -m ruff check openhands-sdk\openhands\sdk\llm\llm.py tests\sdk\llm\test_llm_completion.py
  • python -m ruff format --check openhands-sdk\openhands\sdk\llm\llm.py tests\sdk\llm\test_llm_completion.py
  • python -m pytest tests\sdk\llm\test_llm_completion.py -q

@he-yufeng he-yufeng force-pushed the fix/litellm-modify-params-lock branch from 9eb80d5 to aa5abea Compare May 14, 2026 15:54
@he-yufeng

Copy link
Copy Markdown
Contributor Author

Rebased this branch onto current main and resolved the LLM token-limit conflict by keeping the new _effective_max_* private attrs alongside the modify_params lock.

Revalidated locally after the rebase:

  • .venv\Scripts\python.exe -m pytest tests\sdk\llm\test_llm_completion.py -q --basetemp C:\dev\GITHUB-clean\_tmp\software-agent-sdk-pytest - 17 passed
  • .venv\Scripts\python.exe -m ruff check openhands-sdk\openhands\sdk\llm\llm.py tests\sdk\llm\test_llm_completion.py
  • .venv\Scripts\python.exe -m ruff format --check openhands-sdk\openhands\sdk\llm\llm.py tests\sdk\llm\test_llm_completion.py
  • .venv\Scripts\python.exe -m py_compile openhands-sdk\openhands\sdk\llm\llm.py tests\sdk\llm\test_llm_completion.py
  • git diff --check

@enyst enyst requested a review from csmith49 May 14, 2026 19:24
@enyst

enyst commented May 14, 2026

Copy link
Copy Markdown
Member

Thank you for the contribution! Calvin's investigation flagged this and wrote the tracking issue. @csmith49 I would love it if you could maybe take a look at the PR?

@csmith49 csmith49 requested a review from all-hands-bot May 14, 2026 19:28

@all-hands-bot all-hands-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟢 Good taste - Clean, minimal fix for a real concurrency bug.

[RISK ASSESSMENT]
🟢 LOW - Internal synchronization fix with no API or behavior changes. The RLock correctly serializes access to global litellm state, and the regression test thoroughly validates the thread-safety guarantee.

VERDICT:
Worth merging - Straightforward concurrency fix with proper testing.

KEY INSIGHT:
Using a ClassVar RLock is exactly right here since litellm.modify_params is global state that needs process-wide synchronization.

@csmith49 csmith49 merged commit 12b654a into OpenHands:main May 14, 2026
32 checks passed
StressTestor pushed a commit to StressTestor/software-agent-sdk that referenced this pull request Jun 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: litellm.modify_params global state race condition under concurrency

4 participants