Repository navigation
fix(harness): single-flight pinned whip install + consolidate the patch into whip-binary.ts - #57
Merged
Conversation
…ewers shared one staging path
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
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.
Follow-up to #54/#55/#56: the merged single-process staging still failed on cold runners (monorepo run 36179981281): reviewers ran concurrently inside one loupe process, all shared one PID, so #55's "per-process" staging path
whip.download-${process.pid}was actually one shared file — four simultaneous curls, the winner renamed first, the losers hit ENOENT and resolved null → "CLI is not installed" (migrations/code/zdr at ms 21.888–21.974; docs won and completed at 23.715). Reruns passed because the winner's binary became the cache hit.Two changes in this PR:
1. Single-flight + safe fallback (fixes the race)
installPinnedWhipmemoizes the in-flight promise — all concurrent reviewers in one process await a single download; the memo clears on completion/failure so nothing caches an error.mkdtempSyncdir (same-process and cross-process safe) and renames atomically into~/.loupe/bin/whip.dest: if a concurrent winner already installed a complete binary, return it instead of null — even a losing racer never reports "not installed" again.2. One home for the patch logic (refactor)
The identity check, pinned download, staging, single-flight, and resolution grew across three PRs inside
index.ts. Now all of it lives inpackages/harness/src/whip-binary.tsas one focused module —PINNED_WHIP_TAG,isGenuineWhipOnPath,installPinnedWhip, and a single entry pointresolveWhipBinary(logger)— whichindex.ts's whip harness imports and calls in exactly two places (available(),review()). Future changes (bumping the pin, adopting whipcode v1+) are one-file edits.Verified:
oxfmt --check,oxlint,tsc --buildclean; 484/484 tests passing.Note
Medium Risk
Changes how the whip CLI is downloaded and resolved on CI runners; bugs could still break concurrent reviews or leave a bad cache, but scope is isolated to harness whip fallback logic.
Overview
Fixes a cold-runner race when several whip reviewers start at once inside one loupe process: concurrent pinned installs no longer share one staging file and falsely report the CLI missing.
Single-flight install memoizes
installPinnedWhipso parallel callers await one download; the memo clears when that attempt finishes or fails. Staging moves to a uniquemkdtempdirectory per attempt (instead ofwhip.download-${pid}), with atomic rename into~/.loupe/bin/whip. Failed curls/renames re-check the cache path and return an already-installed binary when another attempt won.Refactor: identity check, pin tag, download, and
resolveWhipBinarymove out ofindex.tsintowhip-binary.ts; the whip harness only importsresolveWhipBinaryforavailable()andreview(). The “using pinned release” info log runs insideresolveWhipBinaryrather than in the harness body.Reviewed by Cursor Bugbot for commit adb6bb8. Bugbot is set up for automated code reviews on this repo. Configure here.