diff --git a/src/main.c b/src/main.c index 8a575c44f..defbef34e 100644 --- a/src/main.c +++ b/src/main.c @@ -78,6 +78,7 @@ enum { #ifdef _WIN32 #include /* CommandLineToArgvW — not pulled in by windows.h under WIN32_LEAN_AND_MEAN */ #include +#include /* CreateToolhelp32Snapshot — parent PID discovery for the parent-death watchdog */ #endif #include "ui/http_server.h" #include "ui/embedded_assets.h" @@ -390,7 +391,15 @@ static void signal_handler(int sig) { * otherwise linger forever blocked on stdin. POSIX has no portable "notify on * parent death" primitive (PR_SET_PDEATHSIG is Linux-only), so we poll getppid: * once the parent dies the process is reparented (ppid changes, typically to 1) - * and we shut down. Windows is unaffected (job objects handle this) — #ifndef. */ + * and we shut down. + * + * Windows used to be excluded here on the assumption that job objects cover it + * (#914 proved they do not): the KILL_ON_JOB_CLOSE job in subprocess.c only + * wraps processes CBM itself spawns. An MCP stdio server is spawned BY the + * client as its child, and Windows does not propagate parent termination to + * children, so the orphan lingers holding SQLite WAL read locks. The Windows + * branch below waits on a handle to the parent process instead — a signaled + * handle is exact (no PID-reuse window), so no polling of the PID is needed. */ #ifndef _WIN32 typedef struct { @@ -537,6 +546,110 @@ static bool client_start_parent_watchdog(pid_t initial_ppid) { } return true; } +#else /* _WIN32 */ +/* Windows parent-death watchdog — the #914 half of the story. + * + * The KILL_ON_JOB_CLOSE job in subprocess.c only contains processes CBM + * spawns itself; a stdio MCP server is the CLIENT's child, and Windows never + * propagates a parent's termination to its children, so a force-killed client + * leaves the server lingering on stdin while pinning SQLite WAL read locks. + * There is no reparenting to poll for either — instead we open a handle to + * the parent at startup and wait on it: the kernel signals a process handle + * exactly once, when the process terminates, and the held handle pins the + * process object, so PID reuse cannot fool the wait the way re-reading a ppid + * could. The 500 ms timeout exists only to re-check g_shutdown, mirroring the + * POSIX poll cadence. + * + * The worker path keeps its POSIX-only guard: workers are spawned by CBM's own + * subprocess layer inside a kill-on-close job, so containment there is already + * the job object's job. */ +typedef struct { + HANDLE parent_process; + bool exit_on_parent_death; +} parent_watchdog_config_t; + +static void *parent_watchdog_thread(void *arg) { + parent_watchdog_config_t config = *(parent_watchdog_config_t *)arg; + + while (!atomic_load(&g_shutdown)) { + DWORD wait_status = WaitForSingleObject(config.parent_process, 500); + if (wait_status == WAIT_OBJECT_0) { + static const char msg[] = "level=warn msg=parent.exited reason=handle_signaled\n"; + (void)_write(_fileno(stderr), msg, sizeof(msg) - 1); + if (config.exit_on_parent_death) { + /* Same deliberate hard stop as the POSIX branch: a lingering + * orphan must release its daemon connection, file locks and + * WAL read lock through kernel handle reclamation, and no + * atexit cleanup is trustworthy after the owning client is + * gone. */ + _exit(0); + } + request_shutdown(); + break; + } + if (wait_status != WAIT_TIMEOUT) { + break; /* handle became unwaitable — stop watching, never spin */ + } + } + return NULL; +} + +/* Toolhelp is the documented way to learn one's own parent PID on Windows; + * the PEB value is not exposed through any public API. Returns 0 when the + * lookup itself fails; callers treat that as "no parent signal available". */ +static DWORD win_parent_pid_from_snapshot(void) { + DWORD parent_pid = 0; + HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0); + if (snapshot == INVALID_HANDLE_VALUE) { + return 0; + } + PROCESSENTRY32W entry; + ZeroMemory(&entry, sizeof(entry)); + entry.dwSize = sizeof(entry); + DWORD self_pid = GetCurrentProcessId(); + if (Process32FirstW(snapshot, &entry)) { + do { + if (entry.th32ProcessID == self_pid) { + parent_pid = entry.th32ParentProcessID; + break; + } + } while (Process32NextW(snapshot, &entry)); + } + CloseHandle(snapshot); + return parent_pid; +} + +static bool client_start_parent_watchdog(DWORD initial_ppid) { + /* Mirrors the POSIX initial_ppid <= 1 bail-out: when no trustworthy parent + * signal exists at startup — snapshot failed, reserved PID, or the parent + * already exited so the handle cannot be opened — keep running and rely on + * the stdin EOF path instead of watching nothing or dying on a false + * alarm. A watchdog thread creation failure, by contrast, is fatal: the + * same fail-closed choice as POSIX. */ + if (initial_ppid == 0) { + return true; + } + HANDLE parent_process = OpenProcess(SYNCHRONIZE, FALSE, initial_ppid); + if (!parent_process) { + return true; + } + static parent_watchdog_config_t client_config; + client_config.parent_process = parent_process; + client_config.exit_on_parent_death = true; + cbm_thread_t watchdog; + if (cbm_thread_create(&watchdog, PARENT_WATCHDOG_STACK_SIZE, parent_watchdog_thread, + &client_config) != 0) { + CloseHandle(parent_process); + return false; + } + if (cbm_thread_detach(&watchdog) != 0) { + atomic_store(&g_shutdown, 1); + (void)cbm_thread_join(&watchdog); + CloseHandle(parent_process); + return false; + } + return true; +} #endif /* ── CLI mode ───────────────────────────────────────────────────── */ @@ -2527,6 +2640,11 @@ int main(int argc, char **argv) { cbm_alloc_init(); #ifndef _WIN32 pid_t process_initial_ppid = getppid(); +#else + /* Captured at the same instant as POSIX: the later OpenProcess in + * client_start_parent_watchdog must target the process that spawned us, + * not whatever may have recycled the PID meanwhile. */ + DWORD process_initial_ppid = win_parent_pid_from_snapshot(); #endif #ifdef _WIN32 { @@ -3114,7 +3232,9 @@ int main(int argc, char **argv) { "cbm-with-ui`.\n"); } } -#ifndef _WIN32 + /* The Windows branch of this call is the #914 fix: identical placement and + * failure handling as POSIX (fail-closed — a client that cannot arm its + * parent watchdog would linger as an orphan after the editor dies). */ if (!client_start_parent_watchdog(process_initial_ppid)) { (void)fprintf(stderr, "codebase-memory-mcp: parent-death watchdog could not start\n"); (void)cbm_daemon_runtime_client_close(g_daemon_client, MAIN_CLOSE_TIMEOUT_MS); @@ -3122,7 +3242,6 @@ int main(int argc, char **argv) { (void)main_version_cohort_close(&client_cohort_lease, &client_cohort_manager); return EXIT_FAILURE; } -#endif setup_signal_handlers(); int result = cbm_daemon_frontend_mcp_run(g_daemon_client, client_cohort_manager, stdin, stdout); diff --git a/tests/test_parent_watchdog.sh b/tests/test_parent_watchdog.sh index 512161a36..0c4bf91ed 100755 --- a/tests/test_parent_watchdog.sh +++ b/tests/test_parent_watchdog.sh @@ -7,16 +7,18 @@ # Strategy: launch the binary under a wrapper "parent" process (stdin kept open # via a FIFO so the server doesn't see EOF), record the child's PID, then kill # the wrapper. The watchdog should notice the changed ppid and exit within a -# few seconds. Skipped on Windows-like shells (the watchdog is POSIX-only). +# few seconds. On Windows (MSYS2 shells) the same tree applies: the wrapper is +# an MSYS bash process, the child a native binary whose parent handle the +# watchdog waits on; killing the wrapper terminates that parent process (#914). set -euo pipefail ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" BINARY="${CBM_TEST_BINARY:-${ROOT}/build/c/codebase-memory-mcp}" +windows_mode=0 case "$(uname -s)" in MINGW*|MSYS*|CYGWIN*) - echo "skipping parent watchdog test on Windows" - exit 0 + windows_mode=1 ;; esac @@ -30,20 +32,77 @@ source "${ROOT}/scripts/test-runtime.sh" cbm_test_runtime_init tmpdir="${CBM_TEST_RUNTIME_ROOT}" wrapper_pid="" +writer_pid="" + +kill_hard() { + # MSYS signal delivery does not reliably terminate the Windows process + # behind an MSYS pid (observed: a wrapper bash kept running after kill -9, + # so a child's real parent never actually died). Force-kill through + # TerminateProcess on the Windows pid as well. The winpid must be resolved + # BEFORE the msys kill: kill -9 removes the pid from the MSYS process table + # immediately, and the mapping would be lost. `ps -W` exists only on MSYS — + # under `set -o pipefail` a failing ps would abort this helper on POSIX — + # so the pipeline is guarded and simply yields an empty winpid elsewhere. + local pid="$1" winpid + winpid="$(ps -W 2>/dev/null | awk -v m="${pid}" '$1==m {print $4; exit}' || true)" + kill -9 "${pid}" 2>/dev/null || true + [[ -n "${winpid}" ]] && taskkill //F //PID "${winpid}" >/dev/null 2>&1 || true +} + cleanup() { if [[ -s "${tmpdir}/child.pid" ]]; then local child_pid child_pid="$(cat "${tmpdir}/child.pid" 2>/dev/null || true)" - [[ -n "${child_pid}" ]] && kill "${child_pid}" 2>/dev/null || true + [[ -n "${child_pid}" ]] && kill_hard "${child_pid}" || true + fi + # On Windows the pipe writer is an orphaned helper that outlives the wrapper + # by design (it holds stdin open); only the test knows its PID. + if [[ -n "${writer_pid}" ]]; then + kill_hard "${writer_pid}" || true fi - [[ -n "${wrapper_pid}" ]] && kill "${wrapper_pid}" 2>/dev/null || true + [[ -n "${wrapper_pid}" ]] && kill_hard "${wrapper_pid}" || true cbm_test_runtime_cleanup "${BINARY}" } trap cleanup EXIT -# Wrapper "parent": opens the FIFO read-write so it stays open, launches the -# MCP server with that FIFO as stdin, records the child PID, then waits. -cat >"${tmpdir}/wrapper.sh" <<'SH' +if (( windows_mode )); then + # MSYS FIFOs are not readable by native Windows binaries (the server blocks + # forever), so the stdin-holder is an anonymous PIPE instead: a writer helper + # forwards the initialize request once the test drops it into a file, then + # holds the pipe's write end open forever. Killing the wrapper orphans the + # writer too, so stdin never sees EOF — the child's exit can only come from + # the parent-death watchdog. + cat >"${tmpdir}/wrapper.sh" <<'SH' +#!/usr/bin/env bash +set -euo pipefail +echo $$ >"${TMPDIR_PATH}/writer.pid" +( + for _ in {1..300}; do + if [[ -s "${REQ_FILE}" ]]; then + cat "${REQ_FILE}" + exec sleep 3600 + fi + sleep 0.1 + done +) | "${CBM_BINARY}" >"${TMPDIR_PATH}/child.out" 2>"${TMPDIR_PATH}/child.err" & +echo "$!" >"${TMPDIR_PATH}/child.pid" +wait +SH + chmod +x "${tmpdir}/wrapper.sh" + : >"${tmpdir}/request.json" + + CBM_BINARY="${BINARY}" REQ_FILE="${tmpdir}/request.json" TMPDIR_PATH="${tmpdir}" \ + "${tmpdir}/wrapper.sh" & + wrapper_pid=$! + for _ in {1..50}; do + [[ -s "${tmpdir}/writer.pid" ]] && break + sleep 0.1 + done + [[ -s "${tmpdir}/writer.pid" ]] && writer_pid="$(cat "${tmpdir}/writer.pid")" +else + # Wrapper "parent": opens the FIFO read-write so it stays open, launches the + # MCP server with that FIFO as stdin, records the child PID, then waits. + cat >"${tmpdir}/wrapper.sh" <<'SH' #!/usr/bin/env bash set -euo pipefail exec 3<>"${FIFO}" @@ -51,12 +110,13 @@ exec 3<>"${FIFO}" echo "$!" >"${TMPDIR_PATH}/child.pid" wait SH -chmod +x "${tmpdir}/wrapper.sh" -mkfifo "${tmpdir}/stdin" + chmod +x "${tmpdir}/wrapper.sh" + mkfifo "${tmpdir}/stdin" -CBM_BINARY="${BINARY}" FIFO="${tmpdir}/stdin" TMPDIR_PATH="${tmpdir}" \ - "${tmpdir}/wrapper.sh" & -wrapper_pid=$! + CBM_BINARY="${BINARY}" FIFO="${tmpdir}/stdin" TMPDIR_PATH="${tmpdir}" \ + "${tmpdir}/wrapper.sh" & + wrapper_pid=$! +fi # Wait for the child PID file to appear. for _ in {1..50}; do @@ -80,9 +140,14 @@ fi # the frontend reached its stdio loop after installing the parent watchdog. # The old mem.init log sync point belonged to the pre-daemon architecture: the # shared daemon now owns memory initialization, so a frontend need not emit it. +if (( windows_mode )); then + request_target="${tmpdir}/request.json" +else + request_target="${tmpdir}/stdin" +fi printf '%s\n' \ '{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2024-11-05","capabilities":{},"clientInfo":{"name":"parent-watchdog-test","version":"1.0"}}}' \ - >"${tmpdir}/stdin" + >"${request_target}" for _ in {1..150}; do if [[ -s "${tmpdir}/child.out" ]] && grep -Eq '"id"[[:space:]]*:[[:space:]]*1' "${tmpdir}/child.out"; then @@ -98,8 +163,35 @@ if ! grep -Eq '"id"[[:space:]]*:[[:space:]]*1' "${tmpdir}/child.out" 2>/dev/null fi # Kill the wrapper parent: the orphaned child must now self-exit. -kill -9 "${wrapper_pid}" -wait "${wrapper_pid}" 2>/dev/null || true +if (( windows_mode )); then + # Two Windows-specific traps make "kill -9 $wrapper_pid" wrong here: + # 1. MSYS kill -9 does not reliably terminate the Windows process behind + # an MSYS pid (a wrapper bash keeps running), and + # 2. the wrapper's background PIPELINE puts an intermediate subshell bash + # between wrapper and server, and the watchdog watches the + # Windows-physical parent (Toolhelp ParentProcessId) — that subshell. + # Resolve the child's actual Windows parent and TerminateProcess exactly + # that one, mirroring a force-killed MCP client. The wrapper is NOT waited + # on yet: its wait covers the whole pipeline job, and the stdin-holding + # writer must survive until the child's exit is observed, or stdin would + # EOF and the test could pass without the watchdog doing anything. + child_winpid="$(ps -W 2>/dev/null | awk -v m="${child_pid}" '$1==m {print $4; exit}')" + if [[ -z "${child_winpid}" ]]; then + echo "child windows pid not found for msys pid ${child_pid}" >&2 + exit 3 + fi + parent_winpid="$(powershell.exe -NoProfile -Command \ + "(Get-CimInstance Win32_Process -Filter \"ProcessId=${child_winpid}\").ParentProcessId" \ + 2>/dev/null | tr -d '[:space:]')" + if [[ -z "${parent_winpid}" || ! "${parent_winpid}" =~ ^[0-9]+$ ]]; then + echo "could not resolve the child's windows parent pid" >&2 + exit 3 + fi + taskkill //F //PID "${parent_winpid}" >/dev/null 2>&1 || true +else + kill -9 "${wrapper_pid}" + wait "${wrapper_pid}" 2>/dev/null || true +fi deadline=$((SECONDS + 15)) while (( SECONDS < deadline )); do @@ -109,10 +201,14 @@ while (( SECONDS < deadline )); do fi # A zombie no longer holds stdin or runs the MCP loop; kill -0 still reports # it until launchd/test parent reaps it, so treat that as a successful exit. - child_state="$(ps -p "${child_pid}" -o stat= 2>/dev/null | tr -d '[:space:]' || true)" - if [[ "${child_state}" == Z* ]]; then - echo "ok: child ${child_pid} exited after parent death (zombie awaiting reap)" - exit 0 + # Windows has no zombie state — an exited process simply disappears from + # kill -0 — so this probe is POSIX-only. + if [[ "${windows_mode}" -eq 0 ]]; then + child_state="$(ps -p "${child_pid}" -o stat= 2>/dev/null | tr -d '[:space:]' || true)" + if [[ "${child_state}" == Z* ]]; then + echo "ok: child ${child_pid} exited after parent death (zombie awaiting reap)" + exit 0 + fi fi sleep 0.2 done