Skip to content

Conversation

@devin-ai-integration
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Nov 24, 2025

Fix external STT server lifecycle to prevent multiple instances

Summary

Fixes a race condition in the external STT server lifecycle management that caused multiple server processes to accumulate when switching between models (e.g., ParakeetV2 ↔ ParakeetV3).

Root Cause: When switching models, stop_all_stt_servers() would wait for the actor to be removed from the registry, but the actual process cleanup happened asynchronously in the post_stop() hook. This meant new servers could start spawning before old processes were fully terminated.

Solution:

  • Added wait_for_process_cleanup() function that verifies external STT processes are actually terminated before returning from stop_stt_server()
  • Waits up to 5 seconds for graceful termination (checking every 100ms)
  • Falls back to force kill if process doesn't terminate in time
  • Added helper functions to hypr_host crate for process verification
  • NEW: Refactored cleanup logic with dependency injection to enable unit testing

Changes:

  • crates/host/src/lib.rs: Added has_processes_matching() and wait_for_processes_to_terminate() functions
  • plugins/local-stt/src/server/supervisor.rs:
    • Added wait_for_process_cleanup() call after stopping external STT servers
    • Added ProcessCleanupDeps struct for dependency injection
    • Added wait_for_process_cleanup_with() that accepts test dependencies
    • Added 3 unit tests covering graceful termination, force kill, and edge cases
  • crates/host/Cargo.toml: Added tokio dependency with time feature for async sleep

Updates Since Last Revision

Added unit tests for the cleanup logic using dependency injection:

  • test_cleanup_process_terminates_gracefully: Verifies no force kill when process exits cleanly
  • test_cleanup_process_never_terminates: Verifies force kill is called and sleep happens after
  • test_cleanup_process_kill_returns_zero: Verifies no sleep when kill finds no processes

The cleanup logic is now testable without spawning real processes or waiting real time.

Review & Testing Checklist for Human

⚠️ CRITICAL - This PR has NOT been tested with actual STT server processes running

The unit tests verify the cleanup logic in isolation but do NOT test the full lifecycle with real processes. Manual testing is essential.

  • Test model switching: Switch between ParakeetV2 and ParakeetV3 multiple times and verify only ONE "stt" process is running at any time (use ps aux | grep stt or Activity Monitor)
  • Test timing: Verify the 5-second timeout is appropriate - model switching shouldn't feel sluggish, but should wait long enough for processes to terminate
  • Test app shutdown: Quit the app and verify all STT processes are cleaned up properly
  • Verify schema.json changes: The owhisper/schema.json file has property reordering - confirm these changes are intentional and don't break anything
  • Check process matching safety: Verify the "stt" substring matching doesn't accidentally match/kill unrelated processes on your system

Recommended Test Plan

  1. Start the app and select an external STT model (ParakeetV2 or ParakeetV3)
  2. Open Activity Monitor / Task Manager and filter for "stt" processes
  3. Switch to a different external STT model
  4. Verify only one "stt" process exists after the switch completes
  5. Repeat steps 3-4 several times to ensure consistency
  6. Quit the app and verify all "stt" processes are terminated

Notes

  • The fix only applies to ServerType::External (Am models), not ServerType::Internal (Whisper models) since internal servers don't spawn external processes
  • The process matching uses substring matching on process names containing "stt" - this should be safe but worth double-checking
  • Unit tests use mocked dependencies and don't verify actual process cleanup behavior
  • The ProcessCleanupDeps struct uses function pointers with Pin<Box<Future>> for testability - the type signature is verbose but necessary for async testing

Session: https://app.devin.ai/sessions/0d06a16e24d441a087fec63fc1e92a45
Requested by: yujonglee (@yujonglee)

…r cleanup

- Add process verification functions to hypr_host crate:
  - has_processes_matching(): Check if processes matching a pattern exist
  - wait_for_processes_to_terminate(): Wait for processes to terminate with timeout

- Update supervisor to wait for process cleanup after stopping external STT servers:
  - wait_for_process_cleanup(): Ensures old process is fully terminated before returning
  - Waits up to 5 seconds for graceful termination
  - Falls back to force kill if process doesn't terminate in time

- Add tokio dependency with time feature to host crate for async sleep

- Fix test to avoid tautological assertion

This fixes the race condition where new STT server processes would start
before old ones were fully terminated, leading to multiple server instances
running simultaneously.

Co-Authored-By: yujonglee <[email protected]>
@devin-ai-integration
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@coderabbitai
Copy link

coderabbitai bot commented Nov 24, 2025

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch devin/1763954023-fix-stt-server-lifecycle

Comment @coderabbitai help to get the list of available commands and usage tips.

@netlify
Copy link

netlify bot commented Nov 24, 2025

Deploy Preview for hyprnote ready!

Name Link
🔨 Latest commit c16af19
🔍 Latest deploy log https://app.netlify.com/projects/hyprnote/deploys/6923d9b3c69834000898c0b9
😎 Deploy Preview https://deploy-preview-1829--hyprnote.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

- Refactor wait_for_process_cleanup to accept ProcessCleanupDeps struct
- ProcessCleanupDeps holds injected functions for process checking, killing, and sleeping
- Add production() constructor that uses real hypr_host functions
- Add wait_for_process_cleanup_with() that accepts test dependencies

Tests added:
- test_cleanup_process_terminates_gracefully: Verifies no force kill when process exits
- test_cleanup_process_never_terminates: Verifies force kill is called and sleep happens
- test_cleanup_process_kill_returns_zero: Verifies no sleep when kill finds no processes

This makes the cleanup logic testable without spawning real processes or waiting real time.

Co-Authored-By: yujonglee <[email protected]>
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.

1 participant