From cb88deef1d45d6cd78c6922553bb70e562d7c0f0 Mon Sep 17 00:00:00 2001 From: Tym Rabchuk Date: Fri, 10 Apr 2026 11:10:10 -0400 Subject: [PATCH 1/2] fix(wrapper): make engine download crash-safe and resumable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: the wrapper assumed /bin/sh would propagate SIGTERM to its curl child. It doesn't. When Claude Code's MCP launcher killed the wrapper mid-download (e.g. on startup timeout), curl was orphaned and kept writing to the tmp path. The next wrapper invocation spawned its own curl on the same file, the two writers clobbered each other, and the partial failed checksum. Users on slow connections or those hit by lifecycle races at plugin update time saw every restart fail the same way — a permanent loop rather than a transient "try again" moment. Fix: - Track the downloader PID explicitly (DL_PID) and install an INT/TERM trap that kills it by PID before the wrapper exits. Avoids `kill 0` which would nuke shared process groups like test harnesses. - Replace the mktemp tmp_dir + EXIT/INT/TERM rm trap with a persistent partial file (${engine_name}.partial) alongside the final engine path. Same filesystem so mv(2) is atomic, and nothing removes it on signal — so a subsequent run can resume via `curl -C -` instead of restarting from byte 0. - Split download() into download_resume (curl -C - / wget -c) and download_fresh (for the tiny checksums file where resume offers no value). Both route through a run_downloader helper that backgrounds the process, parks its PID, waits, and clears DL_PID on return. - On checksum mismatch, purge the partial so a bad resume offset can't loop forever on corrupt bytes. - Stale cleanup now matches engines, partials, and checksum files for old versions, preserving all three current-version files. Verified end-to-end against the real v2.1.2 GitHub release: clean first run, killed-then-resumed run, and corrupt-partial recovery all produce the expected SHA256 (or fail cleanly without looping). --- bin/devkit | 122 +++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 95 insertions(+), 27 deletions(-) diff --git a/bin/devkit b/bin/devkit index 9b83266..e044233 100755 --- a/bin/devkit +++ b/bin/devkit @@ -5,6 +5,13 @@ # This wrapper downloads the matching release asset from GitHub on first # run, verifies its SHA256, caches it alongside this script, and execs it. # +# Downloads are resumable and crash-safe: the partial file lives next to +# the final engine path (no TERM trap removes it), so if Claude Code's +# MCP initialize timeout kills the wrapper mid-download, the next run +# picks up where the last one stopped via `curl -C -` instead of +# restarting from zero. A corrupt partial (checksum mismatch) is purged +# so the next run starts fresh instead of resuming bad bytes. +# # All log output goes to stderr. Stdout stays clean because the MCP stdio # protocol runs over it when Claude Code invokes `devkit mcp`. @@ -19,6 +26,24 @@ RELEASE_REPO="devkit" log() { printf 'devkit: %s\n' "$*" >&2; } die() { log "$*"; exit 1; } +# Track the currently-running downloader PID so our INT/TERM trap can +# kill it explicitly. Without this, /bin/sh's default SIGTERM handler +# exits the wrapper but leaves curl orphaned and still writing to the +# partial file. A subsequent wrapper run then starts its own curl on +# the same file, the two writers clobber each other, and the partial +# fails checksum — forcing a restart-from-zero on every run. Explicit +# PID tracking (vs `kill 0`) avoids nuking whatever process group we +# happen to share with the caller (e.g. test harnesses). +DL_PID= +cleanup_downloader() { + if [ -n "$DL_PID" ]; then + kill "$DL_PID" 2>/dev/null + wait "$DL_PID" 2>/dev/null + DL_PID= + fi +} +trap 'cleanup_downloader; exit 143' INT TERM + # Fast path: developer-built binary next to this script (from `make install-plugin`). LOCAL_DEV="$SCRIPT_DIR/devkit-engine" if [ -x "$LOCAL_DEV" ]; then @@ -61,13 +86,43 @@ sha256_of() { fi } -download() { +# run_downloader runs the chosen downloader in the background and +# parks its PID in DL_PID so the INT/TERM trap can kill it cleanly. +# Returns the downloader's exit code. +run_downloader() { + "$@" & + DL_PID=$! + wait "$DL_PID" + rc=$? + DL_PID= + return $rc +} + +# download_resume: idempotent, resumable HTTP download to $out. Uses +# `curl -C -` (or `wget -c`) to continue from the current file size. If +# the server doesn't support range requests, both fall back to a full +# re-download automatically. +download_resume() { + url=$1 + out=$2 + if command -v curl >/dev/null 2>&1; then + run_downloader curl -fsSL --retry 2 --retry-delay 2 -C - -o "$out" "$url" + elif command -v wget >/dev/null 2>&1; then + run_downloader wget -q -c -O "$out" "$url" + else + die "need curl or wget to download engine binary" + fi +} + +# download_fresh: non-resumable download for tiny files (checksums) +# where resume offers no benefit and a stale file would be a hazard. +download_fresh() { url=$1 out=$2 if command -v curl >/dev/null 2>&1; then - curl -fsSL --retry 2 --retry-delay 2 -o "$out" "$url" + run_downloader curl -fsSL --retry 2 --retry-delay 2 -o "$out" "$url" elif command -v wget >/dev/null 2>&1; then - wget -q -O "$out" "$url" + run_downloader wget -q -O "$out" "$url" else die "need curl or wget to download engine binary" fi @@ -89,42 +144,55 @@ ensure_engine() { return 0 fi - # Remove stale cached engines from old versions in the same directory. - find "$SCRIPT_DIR" -maxdepth 1 -name 'devkit-engine-v*' ! -name "$engine_name" \ + # Staging paths live next to the final engine on the same filesystem + # so mv(2) is atomic and the partial survives SIGTERM (no trap + # removes it). A subsequent wrapper invocation resumes via curl -C -. + partial="$SCRIPT_DIR/${engine_name}.partial" + sums_file="$SCRIPT_DIR/devkit-checksums-v${VERSION}.txt" + + # Remove stale cached engines, partials, and checksum files from old + # versions. Current version's partial and checksums are preserved so + # an interrupted download can resume. + find "$SCRIPT_DIR" -maxdepth 1 \ + \( -name 'devkit-engine-v*' -o -name 'devkit-checksums-v*' \) \ + ! -name "$engine_name" \ + ! -name "${engine_name}.partial" \ + ! -name "devkit-checksums-v${VERSION}.txt" \ -type f -exec rm -f {} + 2>/dev/null || true tag="v${VERSION}" asset="devkit-${PLATFORM}${ext}" base_url="https://github.com/${RELEASE_OWNER}/${RELEASE_REPO}/releases/download/${tag}" - log "first-run: downloading engine ${tag} (${PLATFORM})…" - - # Per-invocation temp dir so two simultaneous first runs never - # share tmp paths. Each process cleans up its own dir on exit; - # the final mv(2) is atomic within a filesystem, so whichever - # writer lands last produces the correct (bit-identical) binary. - tmp_dir=$(mktemp -d "${TMPDIR:-/tmp}/devkit-engine-XXXXXX") \ - || die "mktemp -d failed" - tmp_bin="$tmp_dir/${engine_name}" - tmp_sums="$tmp_dir/checksums.txt" - trap 'rm -rf "$tmp_dir"' EXIT INT TERM + if [ -f "$partial" ]; then + log "resuming download of engine ${tag} (${PLATFORM})…" + else + log "first-run: downloading engine ${tag} (${PLATFORM})…" + fi - download "${base_url}/${asset}" "$tmp_bin" \ + download_resume "${base_url}/${asset}" "$partial" \ || die "download failed: ${base_url}/${asset}" - download "${base_url}/checksums.txt" "$tmp_sums" \ + download_fresh "${base_url}/checksums.txt" "$sums_file" \ || die "checksum file download failed from ${base_url}/checksums.txt" - expected=$(awk -v name="$asset" '$2 == name || $2 == "*"name { print $1; exit }' "$tmp_sums") - [ -n "$expected" ] || die "no checksum entry for $asset in release $tag" + expected=$(awk -v name="$asset" '$2 == name || $2 == "*"name { print $1; exit }' "$sums_file") + if [ -z "$expected" ]; then + rm -f "$sums_file" + die "no checksum entry for $asset in release $tag" + fi - actual=$(sha256_of "$tmp_bin") - [ "$actual" = "$expected" ] \ - || die "checksum mismatch for $asset (expected $expected, got $actual)" + actual=$(sha256_of "$partial") + if [ "$actual" != "$expected" ]; then + # Partial is corrupt (bad resume, disk issue, MITM). Remove it + # so the next run starts from zero instead of looping on a bad + # resume offset. + rm -f "$partial" "$sums_file" + die "checksum mismatch for $asset (expected $expected, got $actual)" + fi - chmod +x "$tmp_bin" - mv -f "$tmp_bin" "$ENGINE_PATH" - rm -rf "$tmp_dir" - trap - EXIT INT TERM + chmod +x "$partial" + mv -f "$partial" "$ENGINE_PATH" + rm -f "$sums_file" log "installed engine at $ENGINE_PATH" } From 1c588b2f112ab3fd9f5f55618320fd3ede34c147 Mon Sep 17 00:00:00 2001 From: Tym Rabchuk Date: Fri, 10 Apr 2026 11:23:52 -0400 Subject: [PATCH 2/2] fix(wrapper): address mega-pr review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight fixes on top of the initial crash-safe download work, covering the blocker and major issues from the mega-pr review: - **Blocker: `set -eu` + `wait` race in `run_downloader`.** A bare `wait "$DL_PID"` aborted the script on non-zero child exit before `rc=$?` could capture the code, silencing every `|| die "..."` in the call site. Switched to `wait "$DL_PID" || rc=$?` which is POSIX-exempt from `set -e` and propagates the real failure. - **sha256 tool detection moved to startup.** The old `sha256_of` called `die` inside a `$(...)` substitution — which only exits the subshell, leaving `$actual=""`, which the checksum branch then misreported as a mismatch and deleted the (valid) partial. Detect the tool once at script top into `SHA256_CMD`, so `die` runs in the parent shell if neither `sha256sum` nor `shasum` is present. - **Pre-complete partial fast path.** If a previous run finished the download but crashed between checksum verification and `mv`, the next run would ask curl to resume a complete file and get a 416 error (false "download failed"). Now we fetch the checksums file first, and if the existing partial already matches the expected SHA256, we skip `download_resume` and install directly. - **`chmod` / `mv` failures must be loud.** Under `set -e` these aborted silently, leaving a verified-but-unlinked partial on disk. Added explicit `|| die "..."` on each so the user sees why the install failed. - **EXIT trap** for defense-in-depth. Catches orphaned downloaders if any future `die` fires mid-download before `cleanup_downloader` clears `DL_PID`. The happy-path `exec` replaces the shell so the EXIT trap never fires on success. - **`cleanup_downloader`** now tolerates `kill`/`wait` failures with `|| true` so the trap itself cannot fall afoul of `set -e`. - **`rm -f` cleanup paths** use `|| true` where a failure is inconsequential (best-effort removal of a just-consumed sums file or a corrupt partial we're already abandoning). - **Comment cleanup.** Dropped PR-history narration ("(vs `kill 0`)", speculation about Claude Code's process group isolation, the "falls back automatically" claim about `curl -C -` which is not actually true when a server rejects Range). Rewrote the top-of-file block to describe the actual invariant rather than the old behaviour. Verified end-to-end against the v2.1.2 release: - Clean first run: installs correctly, SHA matches. - Killed mid-download → second run resumes (or restarts fresh if the kill landed before any bytes hit disk), both paths converge to a matching SHA. - Corrupt partial seeded: purged on mismatch, no loop. - Pre-complete partial seeded (crash between verify and mv): fast path takes over, installs without a re-download, SHA matches. - `shellcheck -s sh` and `dash -n` both clean. --- bin/devkit | 139 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 80 insertions(+), 59 deletions(-) diff --git a/bin/devkit b/bin/devkit index e044233..27d7147 100755 --- a/bin/devkit +++ b/bin/devkit @@ -5,12 +5,12 @@ # This wrapper downloads the matching release asset from GitHub on first # run, verifies its SHA256, caches it alongside this script, and execs it. # -# Downloads are resumable and crash-safe: the partial file lives next to -# the final engine path (no TERM trap removes it), so if Claude Code's -# MCP initialize timeout kills the wrapper mid-download, the next run -# picks up where the last one stopped via `curl -C -` instead of -# restarting from zero. A corrupt partial (checksum mismatch) is purged -# so the next run starts fresh instead of resuming bad bytes. +# Downloads are resumable and crash-safe. The partial file is staged +# next to the final engine path, and the INT/TERM trap only terminates +# the downloader — it does not remove the partial. A subsequent wrapper +# invocation resumes via `curl -C -` (or `wget -c`) from the current +# file size. A checksum mismatch purges the partial so corrupt bytes +# cannot trap a future run in a resume-from-bad-offset loop. # # All log output goes to stderr. Stdout stays clean because the MCP stdio # protocol runs over it when Claude Code invokes `devkit mcp`. @@ -26,23 +26,25 @@ RELEASE_REPO="devkit" log() { printf 'devkit: %s\n' "$*" >&2; } die() { log "$*"; exit 1; } -# Track the currently-running downloader PID so our INT/TERM trap can -# kill it explicitly. Without this, /bin/sh's default SIGTERM handler -# exits the wrapper but leaves curl orphaned and still writing to the -# partial file. A subsequent wrapper run then starts its own curl on -# the same file, the two writers clobber each other, and the partial -# fails checksum — forcing a restart-from-zero on every run. Explicit -# PID tracking (vs `kill 0`) avoids nuking whatever process group we -# happen to share with the caller (e.g. test harnesses). +# Track the downloader PID so the INT/TERM trap can terminate curl +# explicitly. /bin/sh does not forward signals to foreground children, +# so without this the wrapper exits on TERM but curl keeps writing to +# the partial file; the next wrapper run then races its own curl on +# the same file and the two writers corrupt each other's bytes. DL_PID= cleanup_downloader() { if [ -n "$DL_PID" ]; then - kill "$DL_PID" 2>/dev/null - wait "$DL_PID" 2>/dev/null + kill "$DL_PID" 2>/dev/null || true + wait "$DL_PID" 2>/dev/null || true DL_PID= fi } +# INT/TERM: kill the downloader and exit with a signal-style status. +# EXIT: defense-in-depth for `die` paths that abort mid-download — the +# exec at the end of the happy path replaces the shell, so the EXIT +# trap never fires on success. trap 'cleanup_downloader; exit 143' INT TERM +trap 'cleanup_downloader' EXIT # Fast path: developer-built binary next to this script (from `make install-plugin`). LOCAL_DEV="$SCRIPT_DIR/devkit-engine" @@ -75,33 +77,40 @@ detect_platform() { PLATFORM="${os}-${arch}" } +# Detect the sha256 tool at startup rather than inside sha256_of. +# If selection were done inside a $(...) call site, `die` on missing +# tools would exit the subshell only, leaving the caller with an empty +# string that the checksum branch would misreport as a mismatch. +if command -v sha256sum >/dev/null 2>&1; then + SHA256_CMD=sha256sum +elif command -v shasum >/dev/null 2>&1; then + SHA256_CMD="shasum -a 256" +else + die "need sha256sum or shasum to verify downloads" +fi + sha256_of() { - f=$1 - if command -v sha256sum >/dev/null 2>&1; then - sha256sum "$f" | awk '{print $1}' - elif command -v shasum >/dev/null 2>&1; then - shasum -a 256 "$f" | awk '{print $1}' - else - die "need sha256sum or shasum to verify downloads" - fi + $SHA256_CMD "$1" | awk '{print $1}' } -# run_downloader runs the chosen downloader in the background and -# parks its PID in DL_PID so the INT/TERM trap can kill it cleanly. -# Returns the downloader's exit code. +# Background $@ so cleanup_downloader can kill it via DL_PID. +# `|| rc=$?` is required: under `set -e`, a bare `wait "$DL_PID"` +# with a non-zero child exit would abort the script before the +# caller's `|| die` ever runs, silencing the real failure. run_downloader() { "$@" & DL_PID=$! - wait "$DL_PID" - rc=$? + rc=0 + wait "$DL_PID" || rc=$? DL_PID= - return $rc + return "$rc" } -# download_resume: idempotent, resumable HTTP download to $out. Uses -# `curl -C -` (or `wget -c`) to continue from the current file size. If -# the server doesn't support range requests, both fall back to a full -# re-download automatically. +# download_resume: resumable HTTP download to $out. Uses `curl -C -` +# (or `wget -c`) to continue from the file's current size. GitHub +# release assets honour Range requests; if that ever stops being true +# the resume will surface as a download error or a checksum mismatch +# on the next verification step, both of which purge the partial. download_resume() { url=$1 out=$2 @@ -144,15 +153,14 @@ ensure_engine() { return 0 fi - # Staging paths live next to the final engine on the same filesystem - # so mv(2) is atomic and the partial survives SIGTERM (no trap - # removes it). A subsequent wrapper invocation resumes via curl -C -. + # Staging paths live next to the final engine on the same + # filesystem so the install mv(2) is atomic. partial="$SCRIPT_DIR/${engine_name}.partial" sums_file="$SCRIPT_DIR/devkit-checksums-v${VERSION}.txt" - # Remove stale cached engines, partials, and checksum files from old - # versions. Current version's partial and checksums are preserved so - # an interrupted download can resume. + # Sweep stale artifacts from old versions. Current version's + # engine, partial, and checksums file are preserved so an + # interrupted download can resume on the next invocation. find "$SCRIPT_DIR" -maxdepth 1 \ \( -name 'devkit-engine-v*' -o -name 'devkit-checksums-v*' \) \ ! -name "$engine_name" \ @@ -164,35 +172,48 @@ ensure_engine() { asset="devkit-${PLATFORM}${ext}" base_url="https://github.com/${RELEASE_OWNER}/${RELEASE_REPO}/releases/download/${tag}" - if [ -f "$partial" ]; then - log "resuming download of engine ${tag} (${PLATFORM})…" - else - log "first-run: downloading engine ${tag} (${PLATFORM})…" - fi - - download_resume "${base_url}/${asset}" "$partial" \ - || die "download failed: ${base_url}/${asset}" + # Fetch the checksums file first so we have a source of truth + # before touching the binary. Tiny file, no resume benefit. download_fresh "${base_url}/checksums.txt" "$sums_file" \ || die "checksum file download failed from ${base_url}/checksums.txt" expected=$(awk -v name="$asset" '$2 == name || $2 == "*"name { print $1; exit }' "$sums_file") if [ -z "$expected" ]; then - rm -f "$sums_file" + rm -f "$sums_file" || true die "no checksum entry for $asset in release $tag" fi - actual=$(sha256_of "$partial") - if [ "$actual" != "$expected" ]; then - # Partial is corrupt (bad resume, disk issue, MITM). Remove it - # so the next run starts from zero instead of looping on a bad - # resume offset. - rm -f "$partial" "$sums_file" - die "checksum mismatch for $asset (expected $expected, got $actual)" + # Fast path: a pre-existing partial that already matches the + # expected checksum means a prior run finished the download but + # crashed before the atomic install. Skip the resume (which would + # hit a 416 on a complete file) and install directly. + if [ -f "$partial" ] && [ "$(sha256_of "$partial")" = "$expected" ]; then + log "partial already complete — installing engine ${tag} (${PLATFORM})" + else + if [ -f "$partial" ]; then + log "resuming download of engine ${tag} (${PLATFORM})…" + else + log "first-run: downloading engine ${tag} (${PLATFORM})…" + fi + download_resume "${base_url}/${asset}" "$partial" \ + || die "download failed: ${base_url}/${asset}" + + actual=$(sha256_of "$partial") + if [ "$actual" != "$expected" ]; then + # Bad resume offset, disk corruption, or MITM. Purge the + # partial so the next run starts fresh instead of looping + # on a stuck bad-bytes resume. + rm -f "$partial" "$sums_file" || true + die "checksum mismatch for $asset (expected $expected, got $actual)" + fi fi - chmod +x "$partial" - mv -f "$partial" "$ENGINE_PATH" - rm -f "$sums_file" + # chmod before mv so $ENGINE_PATH is never observed without the + # exec bit. Any failure in these three steps must be fatal with a + # clear message — `set -e` alone exits without one. + chmod +x "$partial" || die "chmod +x failed on $partial" + mv -f "$partial" "$ENGINE_PATH" || die "install failed: mv $partial -> $ENGINE_PATH" + rm -f "$sums_file" || true log "installed engine at $ENGINE_PATH" }