Skip to content

fix(daemon): write PID and install signal handlers in foreground mode#554

Open
Maple-Aikon wants to merge 1 commit into
tirth8205:mainfrom
Maple-Aikon:fix/foreground-daemon-pid-signals
Open

fix(daemon): write PID and install signal handlers in foreground mode#554
Maple-Aikon wants to merge 1 commit into
tirth8205:mainfrom
Maple-Aikon:fix/foreground-daemon-pid-signals

Conversation

@Maple-Aikon

Copy link
Copy Markdown

Linked issue

Closes a daemon lifecycle bug found while dogfooding crg-daemon start --foreground. No public issue number — happy to open one if maintainer prefers a tracked issue.

What & why

crg-daemon start --foreground leaks the PID file and ignores SIGTERM/SIGHUP. Cause: _handle_start short-circuits to daemonize() when is_daemon_running() is False. The non-daemonize (foreground) branch returns early without writing the PID file or installing signal handlers, so:

  • crg-daemon stop fails with "PID file missing"
  • The process cannot be terminated by signal
  • crg watch / crg add workflows that need crg-daemon running in foreground become un-killable

The Windows branch of daemonize() already does write_pid() + self._setup_signal_handlers() — this PR applies the same pattern to the Linux/macOS foreground branch.

Diff

 def _handle_start(args: argparse.Namespace) -> None:
     """Start the daemon process."""
-    from .daemon import WatchDaemon, is_daemon_running, load_config
+    from .daemon import WatchDaemon, is_daemon_running, load_config, write_pid

     if is_daemon_running():
         print("Error: Daemon is already running.")

     if not args.foreground:
         daemon.daemonize()
+    else:
+        write_pid()
+        daemon._setup_signal_handlers()

     daemon.run_forever()

uv.lock was auto-updated by the cherry-pick (one transitive bump) — happy to drop it from the PR if maintainer prefers source-only diffs.

How it was tested

# Cherry-picked onto v2.3.6 base (b72413c)
$ uv run pytest tests/test_daemon.py -v --tb=short -q
============================== 51 passed in 0.29s ==============================

$ uvx ruff check code_review_graph/daemon_cli.py
All checks passed!

$ uvx mypy code_review_graph/daemon_cli.py --ignore-missing-imports --no-strict-optional
Success: no issues found in 1 source file

Manual functional test (with isolated HOME):

# Start foreground daemon
$ HOME=/tmp/crg-test/home uv run crg-daemon start --foreground &
# Process stays alive, PID file written:
$ cat /tmp/crg-test/home/.code-review-graph/daemon.pid
14469

# Send SIGTERM
$ kill -TERM 14466
# Daemon log shows graceful shutdown:
INFO: Received signal 15 — shutting down
INFO: Daemon stopped
# Process exits 0 within 5s

Before the fix: SIGTERM was ignored and the process had to be killed with SIGKILL.

Checklist

  • Tests added for new functionality — N/A, this is a bug fix that brings the foreground branch in line with the Windows branch already covered by existing tests
  • All tests pass: uv run pytest tests/test_daemon.py --tb=short -q → 51/51
  • Linting passes: uvx ruff check code_review_graph/daemon_cli.py → clean
  • Type checking passes: uvx mypy code_review_graph/daemon_cli.py --ignore-missing-imports --no-strict-optional → clean
  • Lines are at most 100 characters
  • Docs updated where behavior changed — happy to add a note to docs/CLI.md about foreground signal handling if maintainer wants

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