Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a major refactoring of the AI chat bridge, unifying on a single ACP (Agent Client Protocol) bridge and removing the legacy ClaudeAgentBridge. This significantly simplifies the architecture. The PR also includes substantial improvements to the Tasks UI, introducing local-first operations for better performance, drag-and-drop reordering, and a new background investigation feature. Key reliability fixes have been implemented, including watchdogs to prevent hung ffmpeg processes in the video encoder and to avoid resource leaks from multiple capture timers. Overall, this is an excellent update that enhances performance, reliability, and user experience across the application.
| captureTimer?.invalidate() | ||
| captureTimer = Timer.scheduledTimer(withTimeInterval: RewindSettings.shared.captureInterval, repeats: true) { [weak self] _ in |
There was a problem hiding this comment.
This is a critical fix for preventing a resource leak. Without invalidating the existing captureTimer before creating a new one, multiple timers could be created and run concurrently, especially if handleDisplayChange is called multiple times in quick succession. This would lead to excessive CPU usage and memory consumption from redundant frame captures. Adding captureTimer?.invalidate() ensures that only one capture timer is active at any given time.
| // Terminate ffmpeg process forcefully. | ||
| // Use SIGTERM first, then SIGKILL after 2s if it doesn't respond. | ||
| // ffmpeg can get stuck in disk I/O when the system is under memory pressure, | ||
| // causing SIGTERM to be ignored (process is in uninterruptible sleep in the kernel). | ||
| // Without the SIGKILL fallback, zombie ffmpeg processes accumulate and eventually | ||
| // push the app's memory to 7GB+, making the entire system unresponsive. | ||
| if let process = ffmpegProcess { | ||
| process.terminate() | ||
| let pid = process.processIdentifier | ||
| process.terminate() // SIGTERM | ||
| ffmpegProcess = nil | ||
| Task.detached(priority: .background) { | ||
| try? await Task.sleep(nanoseconds: 2_000_000_000) | ||
| if process.isRunning { | ||
| logError("VideoChunkEncoder: ffmpeg still running 2s after SIGTERM — force killing PID \(pid)") | ||
| kill(pid, SIGKILL) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a critical reliability improvement. The previous implementation with just process.terminate() sends a SIGTERM, which can be ignored by a process stuck in an uninterruptible state (e.g., disk I/O under heavy memory pressure). Without the SIGKILL fallback, a hung ffmpeg process could block this actor indefinitely, leading to resource leaks and causing the entire frame capture pipeline to fail. This watchdog mechanism ensures the process is forcefully cleaned up, preventing such deadlocks.
|
Hey @m13v 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! 💜 |
1 participant
Automated sync from omi-desktop at 474e1e1f.
This PR was created by the sync pipeline.