feat(sdk): introduce LLMProfileStore for persisted LLM configurations#1928
feat(sdk): introduce LLMProfileStore for persisted LLM configurations#1928VascoSch92 wants to merge 8 commits intomainfrom
LLMProfileStore for persisted LLM configurations#1928Conversation
Coverage Report •
|
||||||||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review Summary
Overall, this is a well-implemented feature with good test coverage and solid thread-safety using filelock. The locking logic is correct and properly protects concurrent access. However, there is a critical security issue with path traversal that must be addressed.
Key Findings:
- ✅ Thread locking implementation is correct and safe
- ✅ Excellent test coverage including concurrency tests
- ✅ Good use of atomic file operations
⚠️ Critical: Path traversal vulnerability in filename handling- 💡 Consider additional filename validation
Regarding your open question about filename restrictions: Yes, you should restrict filenames to prevent path traversal attacks and filesystem compatibility issues. See inline comments for details.
| self.base_dir = Path(base_dir) if base_dir is not None else _DEFAULT_PROFILE_DIR | ||
| # ensure directory existence | ||
| self.base_dir.mkdir(parents=True, exist_ok=True) | ||
| self._file_lock = FileLock(self.base_dir / ".profiles.lock") |
There was a problem hiding this comment.
🟢 Nit: The lock file is created inside base_dir, which works correctly but could theoretically be problematic if the entire directory is deleted by another process.
Optional improvement: Consider using a dedicated lock location:
lock_dir = Path.home() / ".openhands" / "locks"
lock_dir.mkdir(parents=True, exist_ok=True)
self._file_lock = FileLock(lock_dir / f"profiles_{hash(self.base_dir)}.lock")However, the current approach is simpler and works well for the expected use case.
| with tempfile.NamedTemporaryFile( | ||
| mode="w", dir=self.base_dir, suffix=".tmp", delete=False | ||
| ) as tmp: | ||
| tmp.write(profile_json) | ||
| tmp_path = Path(tmp.name) | ||
|
|
||
| Path.replace(tmp_path, profile_path) | ||
| logger.info(f"[Profile Store] Saved profile `{name}` at {profile_path}") |
There was a problem hiding this comment.
✅ Good: Atomic write pattern using temp file + replace is the right approach for reliability.
Minor note: If an exception occurs during llm.model_dump_json() or tmp.write(), the temp file might be left behind. This is acceptable because:
- It uses
.tmpsuffix making it easy to identify - The lock is properly released (context manager)
- Leftover temp files don't affect correctness
Optional improvement: Add cleanup in a finally block if you want to be extra tidy:
tmp_path = None
try:
with tempfile.NamedTemporaryFile(...) as tmp:
tmp.write(profile_json)
tmp_path = Path(tmp.name)
Path.replace(tmp_path, profile_path)
finally:
if tmp_path and tmp_path.exists() and not profile_path.exists():
tmp_path.unlink(missing_ok=True)But this adds complexity for minimal benefit.
| def test_delete_nonexistent_profile(profile_store: LLMProfileStore) -> None: | ||
| """Test that deleting a nonexistent profile doesn't raise an error.""" | ||
| profile_store.delete("nonexistent") | ||
|
|
||
|
|
||
| def test_concurrent_saves(tmp_path: Path) -> None: | ||
| """Test that concurrent saves don't corrupt data.""" | ||
| store = LLMProfileStore(base_dir=tmp_path) | ||
| num_threads = 10 | ||
| results: list[int] = [] | ||
| errors: list[tuple[int, Exception]] = [] | ||
|
|
||
| def save_profile(index: int) -> None: | ||
| try: | ||
| llm = LLM( | ||
| usage_id=f"test-{index}", | ||
| model=f"model-{index}", | ||
| temperature=0.1 * index, | ||
| ) | ||
| store.save(f"profile_{index}", llm) | ||
| results.append(index) | ||
| except Exception as e: | ||
| errors.append((index, e)) | ||
|
|
||
| threads = [ | ||
| threading.Thread(target=save_profile, args=(i,)) for i in range(num_threads) | ||
| ] | ||
|
|
||
| for t in threads: | ||
| t.start() | ||
| for t in threads: | ||
| t.join() | ||
|
|
||
| assert len(errors) == 0, f"Errors occurred: {errors}" | ||
| assert len(results) == num_threads | ||
|
|
There was a problem hiding this comment.
✅ Excellent: The concurrent tests are well-designed and properly verify thread-safety. This gives confidence that the locking implementation works correctly under concurrent access.
Good coverage of:
- Concurrent writes
- Mixed reads/writes
- Proper error collection
- Result verification
This addresses the core concern about thread safety!
VascoSch92
changed the title
LLMProfileStore for persisted LLM configurationsLLMProfileStore for persisted LLM configurations
|
Looks great to me!
I think the nice UX here is:
Profile filenames:
Profile filename = profile ID, as the PR does. (Optional, someday (tm): profile name as distinct from ID. We totally could, but don't need to, IMHO. The ID is user-set, they will name it whatever they find readable. Client apps can add more display niceties.) |
Ok! I will do the changes (already proposed by openhands ;)) and i will add an example because the class is in the public API. |
Co-authored-by: OpenHands Bot <contact@all-hands.dev>
Co-authored-by: OpenHands Bot <contact@all-hands.dev>
|
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
There was a problem hiding this comment.
@VascoSch92 you should create a PR in OpenHands/docs for this new feature (with the same branch name as this one), so the docs workflow will be successful
There was a problem hiding this comment.
Yes it is on my todo list ;)
4 participants
Summary
Following the discussion in #1882, this PR introduces a new class to manage persisted LLM profiles.
Could you please take a look at the thread locking logic? I am not an expert in this area and want to ensure it is implemented correctly.
Open question: Should we allow any filename for saved profiles, or should we restrict the choice of filenames?
Checklist
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:e6466f4-pythonRun
All tags pushed for this build
About Multi-Architecture Support
e6466f4-python) is a multi-arch manifest supporting both amd64 and arm64e6466f4-python-amd64) are also available if needed