From 3900b00b2215234390faa8a7998e61af9ba34d1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=91=A8=E6=96=87=E7=91=84?= Date: Fri, 28 Aug 2026 20:33:04 +0800 Subject: [PATCH 1/2] fix(main): exit Windows stdio servers when the spawning client dies (#914) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The POSIX parent-death watchdog (#407) was excluded on Windows with the comment "job objects handle this". They do not: the KILL_ON_JOB_CLOSE job in subprocess.c only wraps processes CBM spawns itself. An MCP stdio server is spawned BY the client as its child, and Windows never propagates a parent's termination to its children, so a force-killed client (editor crash, task manager, CI timeout) leaves the server lingering forever blocked on stdin. The orphan pins SQLite WAL read locks, blocking checkpoints (#1083) and feeding the delete_project permission-denied chain from #914. Windows has no reparenting to poll for, so instead of getppid polling the watchdog opens a SYNCHRONIZE handle to the parent at startup and waits on it: the kernel signals a process handle exactly once on termination, and the held handle pins the process object, so PID reuse cannot fool the wait. The 500 ms loop timeout exists only to re-check g_shutdown, mirroring the POSIX poll cadence. On a signaled parent the thread takes the same deliberate _exit(0) as POSIX: after the owning client is gone, kernel handle reclamation is the only trustworthy release for the daemon connection, file locks and the WAL read lock. When no trustworthy parent signal exists at startup (snapshot failed, reserved PID, parent already exited, or the handle cannot be opened), the client keeps running and leans on the stdin EOF path - the same fail-open choice as the POSIX initial_ppid <= 1 bail-out - while a watchdog thread creation failure stays fail-closed. Workers are unchanged: they are spawned inside CBM's own kill-on-close job, which is the containment the old comment assumed everyone had. The parent-watchdog shell test now runs on MSYS2 instead of skipping: an MSYS bash wrapper launches the native server over an anonymous pipe (an MSYS FIFO is not readable by native binaries), a writer helper holds the write end open so the exit can only come from the watchdog, and the kill resolves the child's Windows-physical parent (the pipeline subshell, not the wrapper script process - the watchdog watches the Toolhelp ParentProcessId) and TerminateProcesses exactly that one, because MSYS kill -9 does not reliably terminate the Windows process behind an MSYS pid. Verified against the pre-fix binary: same test, same tree, the old build leaves the server running after the parent dies; this build exits within one watchdog tick. Signed-off-by: 周文瑄 --- src/main.c | 125 ++++++++++++++++++++++++++++++- tests/test_parent_watchdog.sh | 134 +++++++++++++++++++++++++++++----- 2 files changed, 236 insertions(+), 23 deletions(-) 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..264e230f5 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,75 @@ 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. + local pid="$1" winpid + winpid="$(ps -W 2>/dev/null | awk -v m="${pid}" '$1==m {print $4; exit}')" + 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}" + 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}" fi - [[ -n "${wrapper_pid}" ]] && kill "${wrapper_pid}" 2>/dev/null || true + [[ -n "${wrapper_pid}" ]] && kill_hard "${wrapper_pid}" 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 +108,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 +138,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 +161,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 +199,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 From 41f52a0f3b948468c468ddcca36489eb5f42fff3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=91=A8=E6=96=87=E7=91=84?= Date: Fri, 28 Aug 2026 21:18:14 +0800 Subject: [PATCH 2/2] test(watchdog): guard the ps -W pipeline for POSIX cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught the macOS run exiting 1 AFTER the watchdog assertion passed ('ok: child exited after parent death' followed by exit code 1): the kill_hard helper resolved the Windows pid with 'ps -W', an MSYS-only flag. Under 'set -euo pipefail' the failing ps aborts the helper, the '[[ -n ]] && kill_hard' statement inherits that status, and the EXIT trap turns a passing test into a red job. Linux ps would fail the same way. Guard the pipeline (empty winpid on POSIX, where the flag does not exist) and restore the bare '|| true' tail on every cleanup kill, which the original script had and my refactor dropped. Signed-off-by: 周文瑄 --- tests/test_parent_watchdog.sh | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/test_parent_watchdog.sh b/tests/test_parent_watchdog.sh index 264e230f5..0c4bf91ed 100755 --- a/tests/test_parent_watchdog.sh +++ b/tests/test_parent_watchdog.sh @@ -40,9 +40,11 @@ kill_hard() { # 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. + # 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}')" + 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 } @@ -51,14 +53,14 @@ cleanup() { if [[ -s "${tmpdir}/child.pid" ]]; then local child_pid child_pid="$(cat "${tmpdir}/child.pid" 2>/dev/null || true)" - [[ -n "${child_pid}" ]] && kill_hard "${child_pid}" + [[ -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}" + kill_hard "${writer_pid}" || true fi - [[ -n "${wrapper_pid}" ]] && kill_hard "${wrapper_pid}" + [[ -n "${wrapper_pid}" ]] && kill_hard "${wrapper_pid}" || true cbm_test_runtime_cleanup "${BINARY}" } trap cleanup EXIT