#1973

fix(desktop): wait for the backend to actually exit before quitting

Opencarlosjarenomwants to merge intofrom

What Problem This Solves

Quitting could leave the backend running, still holding its port, so the next launch failed to start. Two independent defects combined to produce it.

On the Electron side, both shutdown paths resolved as soon as a signal had been delivered:

  • kill() completing means kill(2) succeeded, not that the process acted on it. The backend's cleanup_resources() drains the execution service, run coordinator, cloud sync worker and a WebSocket pool — that takes seconds, and the app quit straight through it.
  • The setTimeout(..., 1000) that sends SIGKILL was dropped whenever the caller unwound first, which is the normal case on quit. The signal was scheduled on a timer that the exiting process never reached.
  • Only the parent pid was signalled, while the backend spawns workers. The workers are what hold the port, and they survived their parent.

cleanupPythonProcess also called removeAllListeners() before signalling, discarding the close event — the only reliable evidence that the process actually terminated.

On the backend side, removing the PID file ran unguarded and before the telemetry, thread-pool and WebSocket cleanup:

pid_file = dir / "run.pid"
if pid_file.exists():
    pid_file.unlink()          # not guarded

atexit also removes that file, so a racing unlink raised FileNotFoundError and skipped every step after it — including the TerminalToolkit pool, whose non-daemon threads are precisely what prevent the process from exiting.

Solution Details

  • New electron/main/utils/processTree.ts with terminateProcessTree(): signals the process group, escalates SIGTERM → SIGKILL, and resolves only once the child reports it exited. It returns false when the process is still alive after the force window, so the caller can say the port may stay held rather than quitting silently over a live backend.
  • releaseChildListeners() drops the stdout/stderr listeners a long-lived child accumulates, and deliberately keeps close/exit/error — dropping those is what made the old cleanup unverifiable.
  • Both call sites now use it: cleanupPythonProcess in index.ts, and the startup-failure path in init.ts, which had the same defect.
  • Backend: the PID file is now removed last, guarded, and logged rather than propagated. Bookkeeping no longer comes before real cleanup.

Note the tree-kill dependency is no longer used by index.ts and its import is gone; processTree.ts signals the group directly, which is what init.ts already did elsewhere in the same codebase.

Evidence

7 tests in test/unit/electron/main/processTree.test.ts:

TestPins
waits for the process to actually exit before resolvingthe core defect — asserts the promise is still pending before exit
signals the process group, not just the parentasserts the negative pid
escalates to SIGKILL when SIGTERM is ignoredescalation
reports failure when the process survives SIGKILLno silent success
does nothing for a process that is already goneno signal to a dead pid
keeps the listeners that report terminationthe removeAllListeners regression

3 tests in backend/tests/app/test_shutdown_cleanup.py cover the ordering and the surviving cleanup. Reintroducing the original unguarded, early unlink fails test_resource_cleanup_survives_a_failed_pid_file_removal and passes the other two, so the suite is pinned to this behaviour.

  • vitest run test/unit/electron/main/processTree.test.ts — 7 passed
  • pytest tests/app/test_shutdown_cleanup.py — 3 passed
  • npm run type-check — clean
  • ruff check / ruff format — clean; the repo's own pre-commit hooks (Ruff, Bandit) pass on the commit

test/unit/electron/main/envUtil.test.ts has 14 failures on main before this change, unrelated to it (util.isString is not a function — the jsdom environment resolves path to a browser polyfill). Untouched here to keep the diff focused.

User Impact

Quitting actually stops the backend before the app exits, so the port is free for the next launch. If a backend ever refuses to die, that is now logged instead of silently leaving it running.

Closes #1671

Activity

3
carlosjarenom1mo ago

Rebased onto current main and re-verified while it waited for review:

  • npm run type-check and eslint are clean.
  • Full backend suite: 3696 passed, 12 skipped — identical to the main baseline.
  • Conflicts with main resolved by hand; the fix is now scoped to just the files it needs.

The linked issue is still open, and this is ready for a look whenever you have time.