fix(wrapper): make engine download crash-safe and resumable - #55
Merged
Conversation
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).
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
bin/devkitwrapper that bootstraps the engine binary on first run was not crash-safe. Users could end up in an infinite resume-from-corrupt-state loop whenever Claude Code's MCP launcher killed the wrapper mid-download — which is exactly what happens on lifecycle races at plugin update time and on slow connections under the MCPinitializetimeout.Root cause
/bin/sh's default SIGTERM handler exits the shell without propagating the signal to its curl child. When the wrapper was killed mid-download:The symptom looked like "first-run download timing out," but the real failure was concurrent writers on the same file.
Fix
Four changes in
bin/devkit:Explicit child-PID tracking + INT/TERM trap.
DL_PID+cleanup_downloader()+trap 'cleanup_downloader; exit 143' INT TERM. Kills the downloader by PID (notkill 0, which would nuke shared process groups in test harnesses and other invocation contexts).run_downloaderhelper backgrounds the downloader, parks its PID inDL_PID, waits, and clearsDL_PIDon return. Both download paths route through it so the trap always has a target.Persistent, resumable staging. Replaced
mktemp -d+ EXIT/INT/TERMrmtrap with persistent files alongside the final engine path:${engine_name}.partial— downloaded withcurl -fsSL -C -(orwget -c), resumes from current byte count on next invocation. Survives SIGTERM because nothing removes it.devkit-checksums-v${VERSION}.txt— refetched fresh each run (tiny file, resume has no value).Split the single
download()function intodownload_resumeanddownload_freshfor clarity.Corrupt-partial recovery. On checksum mismatch the partial is deleted before
die. Without this, a bad partial (disk corruption, bad resume offset, MITM) would cause an infinite resume-from-bad-bytes loop.Also extended the stale-file cleanup to match partials and checksum files from old versions, while preserving the current version's in-progress files.
Test plan
Verified end-to-end against the real v2.1.2 GitHub release:
curl -C -, final SHA256 matches.shellcheck -s sh bin/devkit— clean.sh -n bin/devkit— clean.Net: +95 / -27. No engine code touched.