Handle Qwen CLI refresh process cleanup

[matdev83]

Summary

• ensure the Qwen OAuth connector terminates the background CLI refresh process during cleanup
• add focused unit tests that exercise the new cleanup logic for normal, hung, and error cases

Testing

• PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest -o addopts='' tests/unit/connectors/test_qwen_oauth_cleanup.py -vv
• PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest -o addopts='' (fails: missing optional dev dependencies such as pytest-asyncio, respx, pytest_httpx, hypothesis)

---

https://chatgpt.com/codex/tasks/task_e_68ec2685724083339dafb8594c547032

Summary by CodeRabbit

• Bug Fixes
  • Improved shutdown reliability for the OAuth connector. The background refresh process now terminates gracefully with a timeout and is force-killed if unresponsive, preventing hangs during exit.
  • Cleanup is resilient to errors and ensures background tasks stop and resources are released, avoiding orphaned processes and reducing crash risk during logout or app close. Users should notice smoother shutdowns and fewer lingering processes.

[coderabbitai]

Walkthrough

Destructor logic in QwenOAuthConnector now performs guarded shutdown of a background CLI refresh process: attempts graceful terminate with timeout, escalates to kill if needed, suppresses exceptions, and clears the process reference. Unit tests added to validate normal, hung, and error scenarios for cleanup, including file watcher stop and process handling.

Changes

Cohort / File(s)	Summary of Changes
Connector cleanup logic src/connectors/qwen_oauth.py	Destructor updated to safely stop background CLI refresh: check process existence/state, terminate with timeout, kill on timeout, suppress exceptions, and set _cli_refresh_process to None.
Unit tests for cleanup tests/unit/connectors/test_qwen_oauth_cleanup.py	Added tests covering normal shutdown, hung termination requiring kill, and error resilience. Uses mocks/patches to simulate process behavior and asserts file watcher stop and process reference clearing.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App as App
  participant Conn as QwenOAuthConnector
  participant Watch as FileWatcher
  participant Proc as CLI Refresh Process

  App->>Conn: __del__ / cleanup()
  rect rgb(238, 245, 255)
    note right of Conn: Guarded shutdown
    Conn->>Watch: stop()
    alt Process exists and running
      Conn->>Proc: terminate()
      Conn->>Proc: wait(timeout)
      alt wait times out
        Conn->>Proc: kill()
        Conn->>Proc: wait(timeout)
      else exits in time
        note over Proc,Conn: Process ended
      end
    else No process / not running
      note right of Conn: Skip process termination
    end
    note right of Conn: Suppress exceptions
    Conn->>Conn: _cli_refresh_process = None
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I twitch my ears at shutdown’s hum,
Soft paws press “terminate,” then “kill” if numb.
Watchers nap, the process done—
No squeaks of error, not a one.
Burrow tidy, I thump with cheer:
Cleanup’s clear, no ghosts left here. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title succinctly captures the primary change of ensuring the Qwen CLI refresh process is cleaned up during teardown, directly reflecting the core update without irrelevant details.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch codex/fix-improper-resource-management-bug-mglqge

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 326702b and f56828a.

📒 Files selected for processing (2)

• src/connectors/qwen_oauth.py (1 hunks)
• tests/unit/connectors/test_qwen_oauth_cleanup.py (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Avoid using emojis in Python code comments or docstrings
Follow PEP 8 and use type hints for all functions
Import order must be: standard library, third-party, then local imports, separated by blank lines
Naming conventions: snake_case for variables and functions; PascalCase for classes
Error handling: use specific exceptions and include meaningful error messages
Prefer f-strings for string formatting

Files:

• tests/unit/connectors/test_qwen_oauth_cleanup.py
• src/connectors/qwen_oauth.py

src/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.py: Catch specific exceptions; avoid broad except Exception blocks
If a broad exception must be caught, log with exc_info=True and re-raise a specific custom exception
Use the most specific exception class from src.core.common.exceptions that accurately describes the error
Provide clear, helpful error messages and include relevant details in the details dictionary of custom exceptions

Files:

• src/connectors/qwen_oauth.py

🪛 Ruff (0.13.3)

src/connectors/qwen_oauth.py

918-920: try-except-pass detected, consider logging the exception

(S110)

---

918-918: Do not catch blind exception: Exception

(BLE001)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: test (3.10)
• GitHub Check: Analyze (python)

🔇 Additional comments (5)

src/connectors/qwen_oauth.py (1)

908-923: LGTM! Proper destructor cleanup pattern.

The process cleanup logic is well-structured: graceful termination with timeout, escalation to kill if needed, and guaranteed reference clearing in the finally block. The broad exception handling and suppression are appropriate here—destructors must defensively handle all errors to avoid issues during interpreter shutdown when logging or re-raising could fail.

Static analysis warnings (S110, BLE001) are false positives in this destructor context.

tests/unit/connectors/test_qwen_oauth_cleanup.py (4)

15-21: LGTM! Fixture provides proper test isolation.

The fixture correctly creates a connector instance with mocked dependencies for isolated unit testing.

---

23-38: LGTM! Test covers normal cleanup scenario.

The test correctly verifies that cleanup stops file watching, terminates the process, and clears the process reference.

---

41-59: LGTM! Test covers hung process escalation.

The test correctly verifies the terminate → timeout → kill → wait escalation path, ensuring the process is forcefully killed when graceful termination stalls.

---

62-76: LGTM! Test verifies error suppression.

The test correctly verifies that exceptions during cleanup are suppressed and the process reference is still cleared, ensuring robust destructor behavior.

---

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