Fix pre-existing code quality bugs (#289)

[nitrobass24]

Summary

Fixes 7 pre-existing code quality bugs identified during the Ruff review in #289:

• Busy-wait CPU spike on shutdown: AppProcess.terminate() had a while self.is_alive(): pass loop burning CPU. Added time.sleep(0.05) to yield the CPU.
• Race condition on queue internals: ExtractDispatch accessed self.__task_queue.queue (the raw deque) without holding the queue mutex. Wrapped all accesses in with self.__task_queue.mutex:.
• user=None produces "None@host": Sshcp formatted "{}@{}".format(self.__user, self.__host) unconditionally. Added _remote_address() helper that omits user when None.
• Wrong boolean in test wait loop: test_extract_ignores_duplicate_calls used and instead of or, causing premature loop exit.
• urlopen not closed: WebhookNotifier._send_post() didn't close the response from urlopen. Wrapped in with statement.
• Popen not awaited (test_extract): setUpClass used subprocess.Popen without waiting for completion. Replaced with subprocess.run(..., check=True).
• fnull not closed (test_controller): _create_archive leaked an open(os.devnull) handle. Replaced with subprocess.run(..., stdout=subprocess.DEVNULL, check=True).

Test plan

• All 18 existing test_dispatch.py tests pass
• Added 3 new unit tests for Sshcp._remote_address() (user set, user None, default)
• ruff format and ruff check pass on all changed files
• Full unit test suite passes (550 passed; pre-existing lftp failures unrelated)

Closes #289

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Fixed race conditions in task queue operations during concurrent access.
  • Resolved SSH connection issues when user credentials are not specified.
  • Improved HTTP response handling to prevent connection resource leaks.

• Performance

  • Optimized process termination to use efficient polling instead of busy-waiting, reducing CPU usage.

[coderabbitai]

Warning

Rate limit exceeded

@nitrobass24 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 19 minutes and 16 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c95c2e06-4a78-47bb-b578-0b32d880710a

📥 Commits

Reviewing files that changed from the base of the PR and between 17cc5e7 and 38d2052.

📒 Files selected for processing (1)

• src/python/ssh/sshcp.py

📝 Walkthrough

Walkthrough

This PR addresses multiple pre-existing code quality issues identified during the Ruff linting review (#289). Changes include: replacing a CPU-spinning busy-wait loop with throttled sleep, adding synchronization around queue inspection to prevent race conditions, ensuring HTTP response handles are properly closed via context managers, handling None user values in SSH connections, fixing subprocess resource leaks, and correcting test wait logic.

Changes

Cohort / File(s)	Summary
Concurrency & Synchronization src/python/common/app_process.py, src/python/controller/extract/dispatch.py	Replaced busy-wait CPU spin loop with time.sleep(0.05) throttling in process termination. Added explicit mutex acquisition around queue inspection in dispatch to prevent race conditions when reading/checking queued tasks and peeking at next task.
Resource Management src/python/controller/notifier.py, src/python/tests/integration/test_controller/test_controller.py, src/python/tests/integration/test_controller/test_extract/test_extract.py	Wrapped HTTP POST calls in context manager for proper response handle closure. Replaced subprocess.Popen with subprocess.run() using subprocess.DEVNULL and check=True to ensure output suppression and proper error handling without resource leaks.
SSH Path Handling src/python/ssh/sshcp.py, src/python/tests/unittests/test_ssh/test_sshcp_remote_address.py	Added _remote_address() helper method to handle user=None case, preventing invalid None@host formatting. Updated SSH execution paths to use this helper. Added comprehensive unit tests covering user-set, user-None, and default-user scenarios.
Test Logic src/python/tests/unittests/test_controller/test_extract/test_dispatch.py	Corrected spin-wait conditions from and to or in two test methods (test_extract_calls_listener_on_completed, test_extract_ignores_duplicate_calls) to fix premature loop termination that could cause incorrect test assertions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Migrate multiprocessing from fork to spawn #232: Modifies the AppProcess class in src/python/common/app_process.py; this PR updates terminate() behavior while the related PR addresses multiprocessing logging/startup behavior in the same class.

Poem

🐰 A rabbit's ode to cleaner code,
where mutexes guard the queue abode,
no more spinning, just gentle sleep,
resources closed, promises we keep,
None becomes host when users depart,
process and stream both play their part! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 35.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix pre-existing code quality bugs (#289)' accurately describes the main objective of the PR: addressing multiple pre-existing bugs identified during a Ruff review.
Linked Issues check	✅ Passed	The PR successfully addresses all the real bugs and resource leaks specified in issue #289: CPU busy-wait in AppProcess.terminate() fixed with time.sleep(), queue race condition fixed with mutex guards, user=None formatting fixed with _remote_address() helper, boolean logic in test wait loop corrected from 'and' to 'or', urlopen response properly closed with context manager, and subprocess leaks eliminated using subprocess.run() with check=True and DEVNULL.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with the coding requirements in issue #289; no out-of-scope modifications are present. Design decisions and deferred items (os.replace permissions, remote path joining, no-enabled-pairs logic) mentioned in the issue were appropriately excluded from this PR.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/ruff-real-bugs

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/python/ssh/sshcp.py`:
- Around line 42-46: Sshcp.copy() still builds a remote target with a direct
"{user}@{host}" format which can produce "None@host"; update the Sshcp.copy()
implementation to call the existing helper _remote_address() (the method defined
as def _remote_address(self) -> str) wherever the code constructs the remote
"user@host" string so the logic that returns just host when user is None is
reused and the None@host output is eliminated.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 66afc1e1-8ada-4d64-9e3c-a1d7b5fec68e

📥 Commits

Reviewing files that changed from the base of the PR and between 1fa5dee and 17cc5e7.

📒 Files selected for processing (8)

• src/python/common/app_process.py
• src/python/controller/extract/dispatch.py
• src/python/controller/notifier.py
• src/python/ssh/sshcp.py
• src/python/tests/integration/test_controller/test_controller.py
• src/python/tests/integration/test_controller/test_extract/test_extract.py
• src/python/tests/unittests/test_controller/test_extract/test_dispatch.py
• src/python/tests/unittests/test_ssh/test_sshcp_remote_address.py