Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a manually triggered GitHub Actions workflow to build the Tauri app on selectable runner OSes, and updates the Tantivy BM25 index initialization to handle writer-lock contention by attempting lock-file cleanup and retrying writer creation.
Changes:
- Add a new
workflow_dispatchworkflow to run a Tauri build on user-selected GitHub runner OS versions and upload the resulting bundles as artifacts. - Update
BM25Index::opento detect Tantivy writer lock contention and attempt lock-file removal before retryingIndex::writer(...).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| genhat-desktop/src-tauri/src/rag/search.rs | Adds lock-busy detection and a “stale lock cleanup” path when creating the Tantivy writer. |
| .github/workflows/manual-os-build.yml | Introduces a manual multi-OS build workflow (with model placeholder creation and artifact upload). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Err(e) if Self::is_lock_busy(&e) => { | ||
| log::warn!( | ||
| "BM25 writer lock busy at {}; attempting stale lock cleanup", | ||
| index_dir.display() | ||
| ); | ||
| Self::cleanup_stale_writer_lock(index_dir)?; | ||
| index | ||
| .writer(50_000_000) | ||
| .map_err(|e| format!("Writer error after stale lock cleanup: {e}"))? | ||
| } |
There was a problem hiding this comment.
Treating TantivyError::LockFailure(...LockBusy...) as a stale-lock condition and deleting the lock file is unsafe. LockBusy typically indicates another live IndexWriter still holds the OS-level lock; removing the lock path can allow a second writer to be created concurrently, risking index corruption. Prefer a bounded retry/backoff and/or return a clear error instructing the user to close the other running instance; only perform cleanup if you can prove the lock is stale (e.g., successfully acquire the lock yourself before removing).
| if lock_path.exists() { | ||
| std::fs::remove_file(&lock_path).map_err(|e| { | ||
| format!( | ||
| "Failed to remove stale Tantivy writer lock file {}: {e}", | ||
| lock_path.display() | ||
| ) | ||
| })?; | ||
| log::warn!( | ||
| "Removed stale Tantivy writer lock file {}", | ||
| lock_path.display() | ||
| ); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
cleanup_stale_writer_lock hard-codes the lock filename and unconditionally removes it if it exists. Besides the concurrency risk, this is brittle across Tantivy versions (lock naming/location is an internal detail) and the exists() pre-check introduces an avoidable TOCTOU race. Consider removing the exists() check and handling NotFound, and prefer a Tantivy-provided lock handling mechanism (or verify staleness by acquiring the lock) instead of relying on a hard-coded path.
| if lock_path.exists() { | |
| std::fs::remove_file(&lock_path).map_err(|e| { | |
| format!( | |
| "Failed to remove stale Tantivy writer lock file {}: {e}", | |
| lock_path.display() | |
| ) | |
| })?; | |
| log::warn!( | |
| "Removed stale Tantivy writer lock file {}", | |
| lock_path.display() | |
| ); | |
| } | |
| Ok(()) | |
| match std::fs::remove_file(&lock_path) { | |
| Ok(()) => { | |
| log::warn!( | |
| "Removed stale Tantivy writer lock file {}", | |
| lock_path.display() | |
| ); | |
| Ok(()) | |
| } | |
| Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), | |
| Err(e) => Err(format!( | |
| "Failed to remove stale Tantivy writer lock file {}: {e}", | |
| lock_path.display() | |
| )), | |
| } |
Added test runners