diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7e760d3..27cc41f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,7 +34,6 @@ jobs: permissions: contents: read packages: write - continue-on-error: ${{ matrix.distro == 'alpine' }} strategy: fail-fast: false matrix: diff --git a/CHANGELOG.md b/CHANGELOG.md index 35be435..83469b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ All notable changes to this project are documented here. The format follows [Kee - A multi-distro test harness: `docker/` images for Debian (glibc/GNU) and Alpine (musl/busybox), `scripts/test-linux.sh` to run the BATS suite inside them and ShellCheck in the `koalaman/shellcheck-alpine` image, and a CI matrix that builds each image with a GHCR build cache. Tooling only β€” `lib/mcpserver_core.sh` and its consumer-facing surface are unchanged. +### Fixed + +- The tool lifecycle now works on BusyBox. The sentinel's group id and the cancellation path's group-liveness check now read full `ps -A -o` listings (`pid` and `pgid` columns) filtered in Bash, instead of `pgrep -g` and a `ps -p` selection BusyBox does not have. BusyBox `pgrep -g` usage errors exit 1, the same status an emptied group reports, so a cancellation read that as the group already gone and ended the grace period early, dropping the `SIGKILL` escalation that should have followed; the sentinel's `ps -o pgid= -p` read failed outright. `pgrep` is no longer used anywhere in the SDK, and Alpine/BusyBox consumers need no procps. + ## [4.0.0] - 2026-09-12 ### Added diff --git a/README.md b/README.md index 9cbf809..e6f991e 100644 --- a/README.md +++ b/README.md @@ -2,14 +2,13 @@ A Bash framework for writing [Model Context Protocol](https://modelcontextprotocol.io) servers. Handles the JSON-RPC 2.0 stdio loop, tool dispatch, argument validation against each tool's `inputSchema`, and logging. -One file, `lib/mcpserver_core.sh`. It sources nothing and needs `jq`, plus `ps` and `mkfifo` for the tool lifecycle. `pgrep` is optional; the section below says what a system without it gives up. +One file, `lib/mcpserver_core.sh`. It sources nothing and needs `jq`, plus `ps` and `mkfifo` for the tool lifecycle. ## πŸ“Œ Requirements - Bash 4.1+ β€” the file allocates file descriptors with `{var}` redirection, which arrived in 4.1. - `jq` 1.7+ β€” below that floor, jq parses every number to a double, so the validator's `integer` check cannot see a fraction the double rounded away. -- `ps` and `mkfifo` β€” the tool lifecycle. `run_mcp_server` creates the lifeline with `mkfifo`, and the sentinel that kills a tool's process group once the server is gone reads that group's id from `ps`. Both ship with macOS and standard Linux. BusyBox `ps` lacks the flags this lifecycle needs, so Alpine consumers need procps. -- `pgrep` (optional) β€” how a cancellation tells a live tool process group from an emptied one, so a tool that dies on the `SIGTERM` is reaped at once rather than after the two-second grace. macOS and standard Linux ship it. Without it a cancellation still escalates to `SIGKILL`, but it waits the whole grace first, because the fallback check counts the sentinel that guards the group. BusyBox `pgrep` lacks the needed `-g` flag, so Alpine consumers need procps. +- `ps` and `mkfifo` β€” the tool lifecycle. `run_mcp_server` creates the lifeline with `mkfifo`; the sentinel that kills a tool's process group once the server is gone, and the cancellation path that tells a live group from an emptied one, both read a full `ps -A -o` listing (`pid` and `pgid` columns) and filter it in Bash rather than selecting by pid or group. That shape is common to procps, BSD/macOS, and BusyBox `ps`, so BusyBox is enough and Alpine consumers need no procps. > [!NOTE] > macOS ships Bash 3.2. Install a current Bash (`brew install bash`) or run servers under one. @@ -83,7 +82,7 @@ Two properties of tool dispatch to write against: ### Cancelling and shutting down -A client cancels an in-flight call by sending `notifications/cancelled` with the request's id in `params.requestId`. The server stops the tool's process group β€” `SIGTERM`, then `SIGKILL` for whatever is left of it β€” and sends no response for that id. A cancellation that names nothing in flight is logged and dropped. A tool that dies on the `SIGTERM` is reaped at once rather than after the two-second grace: the sentinel that guards the group outlives the tool by design, so it is not counted as a live member when the grace is measured. That measurement is `pgrep`'s; on a system without it the group's raw liveness is the measure and such a cancellation waits the full grace. +A client cancels an in-flight call by sending `notifications/cancelled` with the request's id in `params.requestId`. The server stops the tool's process group β€” `SIGTERM`, then `SIGKILL` for whatever is left of it β€” and sends no response for that id. A cancellation that names nothing in flight is logged and dropped. A tool that dies on the `SIGTERM` is reaped at once rather than after the two-second grace: the sentinel that guards the group outlives the tool by design, so it is not counted as a live member when the grace is measured. The grace is measured against a full `ps` listing of the group's members; when `ps` cannot answer, the group's raw liveness is the measure, which counts the sentinel, and such a cancellation waits the full grace. The tool's process group is the containment boundary. Anything the tool leaves running in it β€” a background child it never waits for β€” is killed when the call ends, on success as much as on cancellation. A tool that must outlive the call has to leave the group itself by detaching into a new session. Bash offers no builtin for that and macOS ships no `setsid(1)`, so such a tool needs its own double-fork. @@ -138,16 +137,14 @@ Or run it in containers, which need Docker: ```bash ./scripts/test-linux.sh # ShellCheck, then Debian and Alpine -./scripts/test-linux.sh debian # one distro: no ShellCheck, and a failure always gates +./scripts/test-linux.sh debian # one distro: no ShellCheck ``` Set `BASE_IMAGE_DEBIAN` or `BASE_IMAGE_ALPINE` to override that build's base image; set `JQ_VERSION` to install that upstream static jq release instead of the distro package. Debian is the glibc/GNU run. Alpine adds only Bash and jq to musl/busybox, so a suite that leans on a tool the dev machine happens to carry fails there. -The Alpine run currently fails. BusyBox `ps` lacks the test harness liveness probes (`ps -o state= -p` in `tests/test_helper/mcp_client.bash` and duplicates in `tests/cancellation.bats` and `tests/lifecycle.bats`) and `ps -Ao state=,args=` in `tests/lifecycle.bats`. The hardcoded `/usr/bin/true` in `tests/lifecycle.bats` fails for a different reason: Alpine has no such path. Consumer-visible SDK calls also need procps: the sentinel uses `ps -o pgid= -p` in `lib/mcpserver_core.sh`, and the cancellation path reads BusyBox `pgrep -g` usage errors as an empty group. Until those sites are fixed, BusyBox `ps` and `pgrep` are not enough for the SDK's tool lifecycle. The CI Alpine leg reports its result without gating merges. - -That tolerance belongs to a bare run only, because a bare run reports on both distros at once. Naming a distro asks for that distro's verdict: the ShellCheck stage is skipped, and any failure gates, Alpine included. +Both suites gate, in bare and named runs alike. Lint with ShellCheck before pushing: diff --git a/lib/mcpserver_core.sh b/lib/mcpserver_core.sh index 12df99e..1da077f 100755 --- a/lib/mcpserver_core.sh +++ b/lib/mcpserver_core.sh @@ -946,39 +946,43 @@ _sentinel_pid_for() { # A group whose only live member is the sentinel therefore reads as dead, and # the caller's grace loop ends. The sentinel is not forgotten: _kill_tool_group # clears whatever is left of the group once the tool is reaped. -# A group the kernel has already released is dead here too β€” pgrep matches +# A group the kernel has already released is dead here too β€” the scan matches # nothing β€” but a caller that must not signal a recycled group id gates on # `kill -0 -- "-"` for that, not on this. -# pgrep's exit status is read, not just its output: 0 means it found members and -# 1 means the group holds none, while anything else β€” 127 for a binary that is -# not there, and pgrep's own failure codes β€” is UNKNOWN. An unknown liveness -# degrades to the previous whole-group check rather than reading as an empty -# group: read as empty it would end the caller's grace loop at once and skip the -# SIGKILL that follows it, leaving a tool that ignores TERM alive. The fallback -# counts the sentinel, so it costs the full grace β€” the safe direction, which -# delays a kill rather than dropping it. +# The members come from a full `ps` listing filtered here rather than from +# `pgrep -g` or a `ps` selection flag: BusyBox ships both binaries without +# group selection, and its usage error exits 1 β€” the same status that means +# "no members" β€” so a selecting call cannot tell an emptied group from a probe +# that never ran. The full listing with `pid` and `pgid` columns is common to +# procps, BSD and BusyBox `ps`. A `ps` that fails anyway β€” 127 for a binary +# that is not there, or its own failure codes β€” is UNKNOWN, and an unknown +# liveness degrades to the previous whole-group check rather than reading as an +# empty group: read as empty it would end the caller's grace loop at once and +# skip the SIGKILL that follows it, leaving a tool that ignores TERM alive. The +# fallback counts the sentinel, so it costs the full grace β€” the safe +# direction, which delays a kill rather than dropping it. # Args: $1 = group id, $2 = sentinel pid (empty when unknown) _tool_group_has_live_member() { local pgid="$1" local sentinel_pid="$2" - local members="" - local pgrep_rc=0 + local listing="" + local ps_rc=0 # Guarded so a missing binary, which reaches a subshell as a 127 exit rather # than through errexit, cannot end the caller. - members="$(pgrep -g "$pgid" 2>/dev/null)" || pgrep_rc=$? - if [[ $pgrep_rc -ne 0 && $pgrep_rc -ne 1 ]]; then + listing="$(ps -A -o pgid=,pid= 2>/dev/null)" || ps_rc=$? + if [[ ${ps_rc} -ne 0 ]]; then kill -0 -- "-${pgid}" 2>/dev/null return fi - local member - while IFS= read -r member; do - if [[ -z "$member" || "$member" == "$sentinel_pid" ]]; then + local entry_pgid member + while read -r entry_pgid member _; do + if [[ "${entry_pgid}" != "${pgid}" || -z "${member}" || "${member}" == "${sentinel_pid}" ]]; then continue fi return 0 - done <<< "$members" + done <<< "${listing}" return 1 } @@ -1102,7 +1106,9 @@ process_request() { # in this design never writes. # The group id comes from ps because every subshell inherits the main shell's # $$, and this sentinel shares the group of the wrapper that spawned it, so -# neither $$ nor its own pid names the group it has to kill. +# neither $$ nor its own pid names the group it has to kill. It is read from a +# full listing filtered by pid rather than a `-p` selection, which BusyBox `ps` +# does not have. # It ignores TERM, INT and HUP: a cancellation TERMs the tool's whole group, and # a sentinel that died there would leave a tool group that ignores TERM with # nothing left to kill it once the server itself is gone. The group SIGKILL that @@ -1112,8 +1118,14 @@ _lifeline_sentinel() { trap '' TERM INT HUP - local pgid - pgid="$(ps -o pgid= -p "${BASHPID}" | tr -d '[:space:]')" + local pgid="" + local entry_pid entry_pgid + while read -r entry_pid entry_pgid _; do + if [[ "${entry_pid}" == "${BASHPID}" ]]; then + pgid="${entry_pgid}" + break + fi + done < <(ps -A -o pid=,pgid= 2>/dev/null) local line="" IFS= read -r -u "${read_fd}" line || true diff --git a/scripts/test-linux.sh b/scripts/test-linux.sh index e4d6480..1932493 100755 --- a/scripts/test-linux.sh +++ b/scripts/test-linux.sh @@ -28,14 +28,6 @@ record_stage() { STAGE_RESULTS+=("$2") } -# A bare run covers both distros and reports one result, so Alpine's BusyBox -# gaps are tolerated there. Naming a distro asks for that distro's verdict, so -# its failure gates. Remove Alpine from this set once README Β§Testing's BusyBox -# follow-up is fixed. -is_allowed_failure() { - [[ "${distros_defaulted}" -eq 1 && "$1" == "alpine" ]] -} - base_image_for() { case "$1" in debian) printf '%s' "${BASE_IMAGE_DEBIAN:-}" ;; @@ -91,7 +83,6 @@ run_distro_stage() { no_build=0 distros=() -distros_defaulted=0 for arg in "$@"; do case "${arg}" in @@ -121,7 +112,6 @@ fi if [[ "${#distros[@]}" -eq 0 ]]; then distros=(debian alpine) - distros_defaulted=1 if ! run_shellcheck_stage; then record_stage shellcheck FAIL else @@ -131,11 +121,7 @@ fi for distro in "${distros[@]}"; do if ! run_distro_stage "${distro}"; then - if is_allowed_failure "${distro}"; then - record_stage "${distro}" "FAIL (allowed)" - else - record_stage "${distro}" FAIL - fi + record_stage "${distro}" FAIL else record_stage "${distro}" PASS fi @@ -145,7 +131,7 @@ failed=0 printf '\n%s\n' "==> Summary" for i in "${!STAGE_NAMES[@]}"; do printf '%s %s\n' "${STAGE_RESULTS[$i]}" "${STAGE_NAMES[$i]}" - if [[ "${STAGE_RESULTS[$i]}" != "PASS" && "${STAGE_RESULTS[$i]}" != "FAIL (allowed)" ]]; then + if [[ "${STAGE_RESULTS[$i]}" != "PASS" ]]; then failed=1 fi done diff --git a/tests/cancellation.bats b/tests/cancellation.bats index 9665554..c952a57 100644 --- a/tests/cancellation.bats +++ b/tests/cancellation.bats @@ -10,8 +10,8 @@ # handle_tools_call, which never reads the caller's stdin at all β€” the same # reason a tools/call replayed after an in-flight call is answered without # reading the stream either. -# The group's liveness check must never read a `pgrep` that could not answer as -# an empty group, which is what would drop the SIGKILL escalation. +# The group's liveness check must never read a `ps` scan that could not answer +# as an empty group, which is what would drop the SIGKILL escalation. # A line the client sent in pieces must not withhold the response of the call # it arrived behind, whether the rest of it never arrives or the client closes # stdin first, and a fragment may only be joined to a line whose read succeeded. @@ -57,7 +57,7 @@ _mcp_wait_for_exit() { local deadline=$(( SECONDS + limit )) local state="" while (( SECONDS < deadline )); do - state="$(ps -o state= -p "${pid}" 2>/dev/null | tr -d '[:space:]')" + state="$(_mcp_proc_state "${pid}")" if [[ -z "${state}" || "${state}" == Z* ]]; then return 0 fi @@ -386,18 +386,24 @@ teardown() { assert_success } -@test "a pgrep that cannot answer still lets a TERM-ignoring tool be killed" { - # Telling a live process group from an emptied one is pgrep's job, and its - # exit status is the answer: 0 means it found members, 1 means it found none. - # A pgrep that fails for any other reason β€” here a binary reporting 127, the - # status a shell gives for one that is not there β€” has not answered at all, - # and an unanswered liveness must not read as an empty group: the grace loop - # would end at once, the SIGKILL that follows it would be skipped, and a tool - # that ignores TERM would outlive its own cancellation. +@test "a ps scan that cannot answer still lets a TERM-ignoring tool be killed" { + # Telling a live process group from an emptied one is the ps member scan's + # job: members in the listing mean live, none mean emptied. A ps that fails + # instead β€” here a binary reporting 127, the status a shell gives for one + # that is not there β€” has not answered at all, and an unanswered liveness + # must not read as an empty group: the grace loop would end at once, the + # SIGKILL that follows it would be skipped, and a tool that ignores TERM + # would outlive its own cancellation. + # The stub fails only the liveness scan's column signature and hands every + # other invocation to the real binary, so the harness's own ps reads and + # the sentinel's group-id read stay answered. + local real_ps + real_ps="$(type -P ps)" local stub_dir="${BATS_TEST_TMPDIR}/stub-bin" mkdir -p "${stub_dir}" - printf '#!/bin/sh\nexit 127\n' > "${stub_dir}/pgrep" - chmod +x "${stub_dir}/pgrep" + printf '#!/bin/sh\ncase "$*" in *"pgid=,pid="*) exit 127 ;; esac\nexec %s "$@"\n' \ + "${real_ps}" > "${stub_dir}/ps" + chmod +x "${stub_dir}/ps" PATH="${stub_dir}:${PATH}" export PATH diff --git a/tests/lifecycle.bats b/tests/lifecycle.bats index b961569..91f8889 100644 --- a/tests/lifecycle.bats +++ b/tests/lifecycle.bats @@ -57,7 +57,7 @@ _mcp_wait_for_exit() { local deadline=$(( SECONDS + limit )) local state="" while (( SECONDS < deadline )); do - state="$(ps -o state= -p "${pid}" 2>/dev/null | tr -d '[:space:]')" + state="$(_mcp_proc_state "${pid}")" if [[ -z "${state}" || "${state}" == Z* ]]; then return 0 fi @@ -103,7 +103,7 @@ _mcp_fixture_procs() { if [[ "${line}" == *"${CANCELLATION_SERVER}"* && "${line}" == *"${marker}"* ]]; then count=$(( count + 1 )) fi - done < <(ps -Ao state=,args=) + done < <(ps -A -o stat=,args=) printf '%s' "${count}" } @@ -506,8 +506,10 @@ teardown() { @test "mcp_start_server fails loudly when the server exits immediately" { # The harness's contract: a server that died at startup has not started, so # starting one is an error at the start rather than a mystery on the first - # write. A binary that exits before the check is exactly that server. - run mcp_start_server /usr/bin/true + # write. A binary that exits before the check is exactly that server. Its + # path is resolved rather than hardcoded: `true` lives under /usr/bin on + # macOS and /bin on Alpine. + run mcp_start_server "$(type -P true)" assert_failure assert_output --partial 'exited within 0.2s' diff --git a/tests/test_helper/mcp_client.bash b/tests/test_helper/mcp_client.bash index 0470a06..26e1370 100644 --- a/tests/test_helper/mcp_client.bash +++ b/tests/test_helper/mcp_client.bash @@ -34,6 +34,22 @@ MCP_CLIENT_FD="" MCP_CLIENT_FD_OPEN=0 +# Print the process state of , empty when it has no ps entry. Read from a +# full listing filtered here because BusyBox `ps` has no `-p` selection, and +# through the `stat` keyword, the one spelling procps, BSD and BusyBox share β€” +# `state` is procps and BSD only. +_mcp_proc_state() { + local pid="$1" + local entry_pid state + while read -r entry_pid state _; do + if [[ "${entry_pid}" == "${pid}" ]]; then + printf '%s' "${state}" + return 0 + fi + done < <(ps -A -o pid=,stat= 2>/dev/null) + return 0 +} + # Start with a FIFO as its stdin and its stdout and stderr # captured to files under a fresh temp dir. Blocks only as long as the FIFO # rendezvous takes, then fails loudly if the server is gone 0.2s in β€” a server @@ -83,7 +99,7 @@ mcp_start_server() { # it succeeds for a zombie, so a server that died within the 0.2s would # count as started and the failure would surface on the first write instead. local state - state="$(ps -o state= -p "${MCP_SERVER_PID}" 2>/dev/null | tr -d '[:space:]')" + state="$(_mcp_proc_state "${MCP_SERVER_PID}")" if [[ -z "${state}" || "${state}" == Z* ]]; then printf 'mcp_start_server: server %s exited within 0.2s\n' "${MCP_SERVER_PID}" >&2 if [[ -s "${MCP_SERVER_ERR}" ]]; then