fix: stop the trainer when the desktop engine quits

[praisonai-triage-agent]

Fixes #4491

Problem

On macOS/Linux, quitting the desktop app orphaned a running fine-tune. The trainer is spawned with start_new_session=True (engine/training.py), so a signal aimed at the engine's process group never reaches it. The engine's exit handler was register_exit_signals(lambda *_: sys.exit(0)) (engine/server.py), so on quit the engine simply exited, the trainer was reparented to init (ppid 1), and it kept the GPU with nothing left able to find or stop it.

Fix

The engine is the only process that knows the trainer's group. Its exit handler now calls Trainer.stop() before exiting; Trainer.stop() → _terminate_group already SIGTERMs the trainer's own session/group. Sending one signal is well within the 2s Engine::shutdown allows before SIGKILL. Windows was already correct (taskkill /T) and is unaffected.

def _stop_everything(*_):
    if _TRAINER is not None:
        try:
            _TRAINER.stop()
        except Exception:  # never block the quit
            pass
    sys.exit(0)

register_exit_signals(_stop_everything)

Test

Added QuitStopsTheTrainer to engine/test_train_routes.py (POSIX-only). It spawns a real engine, starts a run whose stub trainer records its pid then sleeps, sends SIGTERM to the engine exactly as the Tauri shell does, and asserts the trainer pid is gone.

• Verified the test fails without the fix (the trainer (pid ...) outlived the engine quit) and passes with it.
• Full suite: 24 tests pass.

Minimal, backward-compatible; no new API surface.

Generated with Claude Code

[praisonai-triage-agent]

@coderabbitai review

[praisonai-triage-agent]

/review

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[greptile-apps]

Greptile Summary

The PR updates desktop-engine shutdown handling so an active fine-tuning process group is stopped before the engine exits.

• Replaces the direct signal-triggered exit with trainer cleanup followed by engine exit.
• Adds a POSIX integration test that starts a real engine, signals it, and verifies the trainer terminates.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
src/praisonai-desktop/engine/server.py	The exit-signal handler now stops the active trainer before terminating the engine; no eligible blocking issue was identified.
src/praisonai-desktop/engine/test_train_routes.py	Adds POSIX integration coverage proving that an engine SIGTERM also terminates its active trainer.

Sequence Diagram

sequenceDiagram
    participant Desktop as Desktop shell
    participant Engine as Python engine
    participant Trainer as Trainer process group
    Desktop->>Engine: SIGTERM
    Engine->>Trainer: Trainer.stop()
    Trainer-->>Engine: Termination requested
    Engine->>Engine: sys.exit(0)

Loading

_{Reviews (2): Last reviewed commit: "fix: stop the trainer when the desktop e..." | Re-trigger Greptile}

[MervinPraison]

@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Review changes in this PR. Python SDK: praisonaiagents, praisonai. TypeScript SDK: src/praisonai-ts/. Do NOT modify src/praisonai-rust. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding.

Phase 1: Review per AGENTS.md

1. Protocol-driven: check heavy implementations vs core SDK
2. Backward compatible: ensure zero feature regressions
3. Performance: no hot-path regressions
4. SDK value: review in depth whether the change genuinely adds value to the SDK — never add features for the sake of adding them. It must strengthen the SDK (simpler, more user-friendly, robust, world-class, secure). If it does not clearly add value, request changes or recommend rejecting/closing rather than merging scope creep
5. Do not bloat the Agent class with additional params — only if absolutely required; we already support many params.
6. Repo routing: agent-callable tools → PraisonAI-Tools; lifecycle plugins → PraisonAI-Plugins; optional sandbox backends → PraisonAI-Plugins (praisonai.sandbox entry point) — request changes if wrongly added to praisonaiagents/

Phase 2: FIX Valid Issues
7. For any VALID bugs or architectural flaws found by Gemini, CodeRabbit, Qodo, Copilot, or any other reviewer: implement the fix
8. Also independently identify and fix any gaps or issues you find in the changed code — do not rely only on prior reviewer feedback
9. Push all code fixes directly to THIS branch (do NOT create a new PR)
10. Comment a summary of exact files modified and what you skipped

Phase 3: Final Verdict
11. If all issues are resolved, approve the PR / close the Issue
12. If blocking issues remain, request changes / leave clear action items

[praisonai-triage-agent]

Claude finished @MervinPraison's task —— View job

---

Final Architecture Review — PR #4508 ✅ APPROVE

• Read AGENTS.md & changed files
• Review server.py shutdown handler
• Review test_train_routes.py test
• Check reviewer feedback + independent review
• Verify tests (24/24 pass)
• Final verdict

---

Verdict: Approve. No code changes needed.

Scope note: The change lives in src/praisonai-desktop/engine/, not in the praisonaiagents/praisonai/praisonai-ts SDK surface. It adds zero SDK API surface, no new params, no Agent bloat, no new dependencies — so it's fully aligned with the "lightweight and powerful" mandate. This is a pure process-lifecycle bug fix.

Correctness — verified end to end

• The root cause is real: the trainer is spawned with start_new_session=True (training.py:462), so a signal to the engine's process group never reaches the fine-tune. The old handler lambda *_: sys.exit(0) left it reparented to init, holding the GPU. ✔️
• The fix routes through the already-tested path: _stop_everything → Trainer.stop() → _terminate_group (training.py:482), which SIGTERMs the trainer's own session/group. _terminate_group also guards against killpg hitting the engine's own group (training.py:516), so the engine never signals itself. ✔️
• One SIGTERM is well within the shell's 2s pre-SIGKILL window. Windows is untouched and was already correct via taskkill /T. ✔️

Robustness of the handler

• if _TRAINER is not None: — correct: the trainer is lazily built on first /train/* use, so a chat-only session never constructs one and quit stays instant. ✔️
• except Exception: pass around stop() guarantees quit is never blocked by a teardown failure. ✔️
• register_exit_signals already tolerates missing signals per platform (server.py:189). ✔️

Test quality

• QuitStopsTheTrainer is a genuine end-to-end regression: spawns a real engine, starts a stub trainer that records its pid then sleeps 300s, sends SIGTERM to the engine exactly as the Tauri shell does, and asserts the pid is gone. Correctly @skipIf on Windows (different mechanism). ✔️
• I ran the suite locally: 24/24 pass, including the new test. The claim that it fails without the fix is credible given the mechanism.

Reviewer feedback triage

• Greptile (5/5, no blocking issues) — agree.
• Qodo — reviews paused, no actionable content.
• No valid bugs or architectural flaws raised by any reviewer; none found independently.

Files modified by me: none — the PR is already correct and minimal.
Skipped: nothing actionable.

Job run • Branch claude/issue-4491-20260827-1337

[MervinPraison]

@claude this PR has merge conflicts with main. Please rebase onto latest main, resolve conflicts (keep this PR's intent, merge in newer main logic), run targeted tests, and force-push with --force-with-lease. Comment which files you resolved. Do not bloat the Agent class with additional params — only if absolutely required; we already support many params. Keep the resolution minimal — the aim is a lightweight and powerful package; do not introduce scope creep while resolving conflicts.

[praisonai-triage-agent]

Claude finished @MervinPraison's task —— View job

---

Rebased PR #4508 onto latest main ✅

• Setup git identity & fetch main
• Rebase onto origin/main, resolve conflicts
• Run targeted tests
• Force-push with --force-with-lease

---

Conflict resolved

src/praisonai-desktop/engine/test_train_routes.py — one conflict at the tail of the file. main had added the ChatsListing test class (from #4496); this PR adds QuitStopsTheTrainer. Both are independent additions with no overlap, so I kept both classes (separated by the standard two blank lines). All required imports (json, tempfile, shutil, signal, os, pathlib) were already present.

src/praisonai-desktop/engine/server.py auto-merged cleanly — no conflict.

Tests

Ran the full engine suite via unittest (pytest isn't installed in this env):

Ran 26 tests in 4.918s — OK

Both QuitStopsTheTrainer.test_quitting_the_engine_kills_the_running_trainer and main's ChatsListing tests pass. No regressions.

Scope

Resolution was minimal — no logic changes, no SDK surface touched, no Agent params added. Force-pushed with --force-with-lease (747e3fedb → 6d7df6990).

Job run • Branch claude/issue-4491-20260827-1337

[praisonai-triage-agent]

Merge gate scan — eligible for assessment. Claude merge gate will assess and may auto-merge if MERGE_GATE_VERDICT: APPROVE.

[praisonai-triage-agent]

Merged by Claude PR merge gate (claude-merge-gate.yml).
Verdict: MERGE_GATE_VERDICT: APPROVE
SHA: 6d7df69
Method: merge