fix: auto-upgrade fzf to fix live search race condition on Linux#3
fix: auto-upgrade fzf to fix live search race condition on Linux#3Timmy6942025 merged 4 commits intomasterfrom
Conversation
fzf < 0.56.1 has a race condition (junegunn/fzf#4070) where change:reload: bindings silently fail during rapid typing - the reader process is terminated before it properly initializes, causing the list to stop updating until the user presses Ctrl+R or waits for the process to finish naturally. Debian/Ubuntu repos ship fzf 0.44.1 which is affected. macOS (Homebrew) and Arch (pacman) ship latest and are unaffected. Changes: - ensureFzfGo() now checks fzf >= 0.56.1, not just existence - If outdated, tries package manager upgrade first - Falls back to downloading latest release binary from GitHub (tar.gz/zip) - Binary installed to ~/.local/share/fpf/fzf/ (preferred over system fzf) - All fzf invocations use resolveFzfBinaryPath() to pick the best binary - Supports Linux (tar.gz), macOS (tar.gz), and Windows (zip)
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe runtime now prefers a per-user fzf binary (~/.local/share/fpf/fzf), probes its version/capabilities via a resolved path, and adds a GitHub-release-based local installer (download, verify checksums, extract, write executable) as a fallback/upgrade path when package-manager installs are unavailable or insufficient. Changes
Sequence Diagram(s)sequenceDiagram
participant fpf as fpf Runtime
participant resolver as Resolver
participant pkgmgr as Package Manager
participant gh as GitHub Release API
participant fs as File System
participant fzf as fzf Binary
fpf->>resolver: ensureFzfGo()
resolver->>fzf: resolveFzfBinaryPath()
alt binary present (local or PATH)
resolver->>fzf: check version >= 0.56.1?
alt version >= 0.56.1
resolver-->>fpf: success
else version < 0.56.1
resolver->>pkgmgr: attempt upgrade
alt pkgmgr succeeds
pkgmgr-->>resolver: upgraded binary available
else pkgmgr fails
resolver->>gh: fetch latest release & checksums
gh-->>resolver: release archive + checksums
resolver->>fs: verify checksum, extract archive, write executable to local dir
fs-->>resolver: local fzf available
end
resolver-->>fpf: success or warned
end
else binary missing
resolver->>pkgmgr: attempt install
alt pkgmgr succeeds
pkgmgr-->>resolver: installed binary
else pkgmgr fails
resolver->>gh: fetch latest release & checksums
gh-->>resolver: release archive + checksums
resolver->>fs: verify checksum, extract archive, write executable to local dir
fs-->>resolver: local fzf available
end
resolver-->>fpf: success or error
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @Timmy6942025. * #3 (comment) The following files were modified: * `cmd/fpf/cli_runtime.go`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/fpf/cli_runtime.go`:
- Around line 1391-1407: The current logic in fzf bootstrap short-circuits when
buildFzfBootstrapCandidatesGo(managers) returns no candidates, preventing the
release-binary fallback; change the control flow so that if len(candidates)==0
you do not return an error but instead proceed to attempt the release fallback
via installFzfFromReleaseFallbackGo(), while preserving the existing behavior
when candidates exist (iterate candidates using installFzfWithManagerGo and only
if those fail fall back to installFzfFromReleaseFallbackGo()). Update messaging
around joinManagerLabelsGo and managerLabelGo accordingly and ensure final
checks still call fzfCommandAvailableGo() before returning an error.
- Around line 1411-1431: ensureFzfGo currently swallows upgrade failures and
always returns nil, allowing the caller to set reloadCmd even when the fzf
version is still incompatible; change ensureFzfGo to return an error (or an
explicit success boolean) when it fails to upgrade to fzfMinVersionChangeReload,
and update the caller to check that result before setting reloadCmd so
runFuzzySelectorGo will not bind change:reload when fzfSupportsResultBindGo() is
false; locate ensureFzfGo, the caller that sets reloadCmd, and
runFuzzySelectorGo/fzfSupportsResultBindGo to implement the error/flag
propagation and conditional binding.
- Around line 1215-1245: The URL construction in the fzf installer is wrong and
GOARCH isn't mapped to fzf's release arch names; update the code around
resolveLatestFzfVersion(), goos/goarch, binaryName and url so the fzf asset name
uses a dash between version and platform (fzf-{version}-{os}_{arch}.tar.gz or
.zip) and map runtime.GOARCH values to fzf's expected arch strings (e.g., arm ->
armv7/armv6/armv5 as appropriate, amd64 -> amd64, 386 -> 386, arm64 -> arm64,
etc.) before building the URL; keep the windows .zip vs other .tar.gz
distinction and still set binaryName to "fzf.exe" on windows and join to target
with filepath.Join.
- Around line 1350-1362: The writeExecutable function should use os.CreateTemp
instead of constructing tmp as target + ".tmp": create a unique temp file (e.g.,
via os.CreateTemp with the target directory and pattern), copy the reader into
that file, check and handle errors from f.Close() before attempting cleanup or
rename, and finally use os.Rename to atomically replace target; update all error
paths to remove the temp file (os.Remove) only after a successful Close or when
Close returns an error, and ensure any Close error is returned instead of being
ignored.
- Around line 1286-1303: The downloadAndInstallFzf function currently extracts
the downloaded asset without integrity checks; update it to fetch the expected
SHA256 digest for the chosen asset (using the release metadata or the
platform-specific .sha256/sums asset), download the asset into a temporary file
or buffer, compute its SHA256, compare it to the expected digest for
binaryName/platform, and return an error and abort before calling extractZipFzf
or extractTarGzFzf if the checksums do not match; ensure you use the same
binaryName/asset selector logic as in downloadAndInstallFzf to pick the correct
digest and clean up temp files on failure.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c93b014-55d2-4139-95e1-23ec7216fec0
📒 Files selected for processing (1)
cmd/fpf/cli_runtime.go
📝 Add docstrings to `fix/fzf-live-search-linux`
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 4 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
♻️ Duplicate comments (5)
cmd/fpf/cli_runtime.go (5)
1387-1403: 🧹 Nitpick | 🔵 TrivialClose error not checked before rename; consider
os.CreateTemp.The
f.Close()call on line 1401 doesn't check for errors before renaming. IfClose()fails (e.g., due to disk full during final flush), the rename could move an incomplete file. Additionally, usingos.CreateTempwould avoid potential filename collisions.♻️ Proposed improvement with error checking
func writeExecutable(target string, reader io.Reader) error { - tmp := target + ".tmp" - f, err := os.OpenFile(tmp, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o755) + dir := filepath.Dir(target) + f, err := os.CreateTemp(dir, ".fzf-install-*.tmp") if err != nil { return err } + tmp := f.Name() if _, err := io.Copy(f, reader); err != nil { f.Close() _ = os.Remove(tmp) return err } - f.Close() + if err := f.Close(); err != nil { + _ = os.Remove(tmp) + return err + } + if err := os.Chmod(tmp, 0o755); err != nil { + _ = os.Remove(tmp) + return err + } return os.Rename(tmp, target) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fpf/cli_runtime.go` around lines 1387 - 1403, The writeExecutable function should use os.CreateTemp to create a unique temp file, write the contents, call f.Sync() (optional) and then check and handle the error returned by f.Close() before performing os.Rename; if Close returns an error, remove the temp file and return that error to avoid renaming a possibly incomplete file; ensure the temp file is created with the correct permission (0755) and that failures from io.Copy, Close, and Rename all clean up the temp file and return the appropriate error (update references to tmp and the function name writeExecutable accordingly).
1449-1465:⚠️ Potential issue | 🟠 MajorRelease fallback not attempted when no package managers are available.
When
buildFzfBootstrapCandidatesGoreturns an empty list (lines 1451-1453), the function returns an error immediately without attemptinginstallFzfFromReleaseFallbackGo(). This breaks fzf bootstrap on systems with network access but no detected package managers (e.g., minimal containers, custom environments).🐛 Proposed fix to allow release fallback
func ensureFzfGo(managers []string) error { if !fzfCommandAvailableGo() { candidates := buildFzfBootstrapCandidatesGo(managers) - if len(candidates) == 0 { - return fmt.Errorf("fzf is required and no compatible manager is available to auto-install it") - } - fmt.Fprintf(os.Stderr, "fzf is missing. Auto-installing with: %s\n", joinManagerLabelsGo(candidates)) - for _, manager := range candidates { - fmt.Fprintf(os.Stderr, "Attempting fzf install with %s\n", managerLabelGo(manager)) - if err := installFzfWithManagerGo(manager); err == nil && fzfCommandAvailableGo() { - break + if len(candidates) > 0 { + fmt.Fprintf(os.Stderr, "fzf is missing. Auto-installing with: %s\n", joinManagerLabelsGo(candidates)) + for _, manager := range candidates { + fmt.Fprintf(os.Stderr, "Attempting fzf install with %s\n", managerLabelGo(manager)) + if err := installFzfWithManagerGo(manager); err == nil && fzfCommandAvailableGo() { + break + } } } if !fzfCommandAvailableGo() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fpf/cli_runtime.go` around lines 1449 - 1465, When buildFzfBootstrapCandidatesGo returns an empty slice the code currently returns early; instead, if no candidates are found call installFzfFromReleaseFallbackGo() and then re-check fzfCommandAvailableGo() before returning an error. Modify the branch handling len(candidates) == 0 to print a message about no package managers found, attempt installFzfFromReleaseFallbackGo(), and only return an error if that fallback fails or fzfCommandAvailableGo() remains false; otherwise continue as if install succeeded. Reference functions: buildFzfBootstrapCandidatesGo, installFzfFromReleaseFallbackGo, fzfCommandAvailableGo, joinManagerLabelsGo, managerLabelGo, and installFzfWithManagerGo to keep existing logging and checks.
1468-1489:⚠️ Potential issue | 🟠 MajorWarning printed but
change:reloadbinding still enabled for outdated fzf.When the upgrade fails (lines 1484-1487),
ensureFzfGoprints a warning but returnsnil. The caller then proceeds to setreloadCmd, andrunFuzzySelectorGo(line 650) still bindschange:reloadfor fzf versions that don't support theresultkey. This means users with fzf < 0.56.1 will experience the race condition the PR aims to fix.Consider either:
- Returning an error to prevent proceeding with an incompatible fzf version
- Propagating a flag to disable
change:reloadwhen the minimum version isn't met- Having
fzfSupportsResultBindGo()also check the version minimum🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fpf/cli_runtime.go` around lines 1468 - 1489, ensureFzfGo currently logs a warning and returns nil when it cannot upgrade fzf, letting the caller proceed and bind change:reload to incompatible fzf; change ensureFzfGo to return a non-nil error when it fails to ensure a compatible fzf (after installFzfFromReleaseFallbackGo and upgrade attempts) so the caller can abort or fall back, and update the caller of ensureFzfGo (the code that sets reloadCmd / calls runFuzzySelectorGo) to stop binding change:reload when ensureFzfGo returns an error; alternatively, if you prefer preserving a non-error path, add a boolean return from ensureFzfGo (e.g., ensureFzfGo(...) (bool, error)) or introduce an explicit flag (fzfCompatible bool) that runFuzzySelectorGo and fzfSupportsResultBindGo use to avoid binding change:reload for versions < fzfMinVersionChangeReload.
1266-1271:⚠️ Potential issue | 🔴 CriticalURL format mismatch will cause 404 errors on all platforms.
The URL construction uses an underscore between version and OS (
fzf-%s_%s), but fzf releases use a dash (fzf-{version}-{os}_{arch}). Additionally,runtime.GOARCHvalues for 32-bit ARM (arm) don't map to fzf's asset names (armv5,armv6,armv7).Current code produces:
fzf-0.70.0_linux_amd64.tar.gz
Actual fzf asset name:fzf-0.70.0-linux_amd64.tar.gz🐛 Proposed fix for URL format and ARM mapping
+func mapGoarchToFzfArch(goarch string) string { + switch goarch { + case "arm": + return "armv7" // Most common ARM variant; consider detecting via runtime + case "arm64", "amd64", "386": + return goarch + default: + return goarch + } +} + func installFzfFromReleaseFallbackGo() bool { // ... existing code ... goos := runtime.GOOS goarch := runtime.GOARCH + fzfArch := mapGoarchToFzfArch(goarch) // ... existing code ... var url string if goos == "windows" { - url = fmt.Sprintf("https://github.com/junegunn/fzf/releases/download/v%s/fzf-%s_%s.zip", version, version, goos+"_"+goarch) + url = fmt.Sprintf("https://github.com/junegunn/fzf/releases/download/v%s/fzf-%s-%s_%s.zip", version, version, goos, fzfArch) } else { - url = fmt.Sprintf("https://github.com/junegunn/fzf/releases/download/v%s/fzf-%s_%s.tar.gz", version, version, goos+"_"+goarch) + url = fmt.Sprintf("https://github.com/junegunn/fzf/releases/download/v%s/fzf-%s-%s_%s.tar.gz", version, version, goos, fzfArch) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fpf/cli_runtime.go` around lines 1266 - 1271, The URL is built with the wrong separator and doesn't map 32-bit ARM correctly; update the construction in the code that sets url (the block using goos, goarch, version) to use the dash between version and OS in the asset name (format "fzf-{version}-{os}_{arch}.{ext}" instead of "fzf-{version}_{os}.{ext}") and normalize ARM by mapping when goarch == "arm" to the proper fzf arch strings using runtime.GOARM (e.g. "armv5","armv6","armv7") before formatting; implement a small helper (e.g. normalizeArch or mapArch) and use it in the fmt.Sprintf that produces url, preserving the existing .zip for windows and .tar.gz otherwise.
1314-1335:⚠️ Potential issue | 🟠 MajorMissing integrity verification for downloaded binary.
The download function installs the fzf binary without verifying its SHA256 checksum. GitHub fzf releases publish SHA256 digests inline on the release page. Without verification, a MITM attacker or compromised CDN could serve a malicious binary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/fpf/cli_runtime.go`:
- Around line 1387-1403: The writeExecutable function should use os.CreateTemp
to create a unique temp file, write the contents, call f.Sync() (optional) and
then check and handle the error returned by f.Close() before performing
os.Rename; if Close returns an error, remove the temp file and return that error
to avoid renaming a possibly incomplete file; ensure the temp file is created
with the correct permission (0755) and that failures from io.Copy, Close, and
Rename all clean up the temp file and return the appropriate error (update
references to tmp and the function name writeExecutable accordingly).
- Around line 1449-1465: When buildFzfBootstrapCandidatesGo returns an empty
slice the code currently returns early; instead, if no candidates are found call
installFzfFromReleaseFallbackGo() and then re-check fzfCommandAvailableGo()
before returning an error. Modify the branch handling len(candidates) == 0 to
print a message about no package managers found, attempt
installFzfFromReleaseFallbackGo(), and only return an error if that fallback
fails or fzfCommandAvailableGo() remains false; otherwise continue as if install
succeeded. Reference functions: buildFzfBootstrapCandidatesGo,
installFzfFromReleaseFallbackGo, fzfCommandAvailableGo, joinManagerLabelsGo,
managerLabelGo, and installFzfWithManagerGo to keep existing logging and checks.
- Around line 1468-1489: ensureFzfGo currently logs a warning and returns nil
when it cannot upgrade fzf, letting the caller proceed and bind change:reload to
incompatible fzf; change ensureFzfGo to return a non-nil error when it fails to
ensure a compatible fzf (after installFzfFromReleaseFallbackGo and upgrade
attempts) so the caller can abort or fall back, and update the caller of
ensureFzfGo (the code that sets reloadCmd / calls runFuzzySelectorGo) to stop
binding change:reload when ensureFzfGo returns an error; alternatively, if you
prefer preserving a non-error path, add a boolean return from ensureFzfGo (e.g.,
ensureFzfGo(...) (bool, error)) or introduce an explicit flag (fzfCompatible
bool) that runFuzzySelectorGo and fzfSupportsResultBindGo use to avoid binding
change:reload for versions < fzfMinVersionChangeReload.
- Around line 1266-1271: The URL is built with the wrong separator and doesn't
map 32-bit ARM correctly; update the construction in the code that sets url (the
block using goos, goarch, version) to use the dash between version and OS in the
asset name (format "fzf-{version}-{os}_{arch}.{ext}" instead of
"fzf-{version}_{os}.{ext}") and normalize ARM by mapping when goarch == "arm" to
the proper fzf arch strings using runtime.GOARM (e.g. "armv5","armv6","armv7")
before formatting; implement a small helper (e.g. normalizeArch or mapArch) and
use it in the fmt.Sprintf that produces url, preserving the existing .zip for
windows and .tar.gz otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d70575a6-ce42-4f95-8578-f0cf989ba26f
📒 Files selected for processing (1)
cmd/fpf/cli_runtime.go
📜 Review details
🔇 Additional comments (7)
cmd/fpf/cli_runtime.go (7)
3-20: LGTM on imports and version constant.The added imports support the archive extraction and HTTP download functionality. The minimum version constant
0.56.1correctly references the fzf version that fixes the race condition (junegunn/fzf#4070).Also applies to: 35-36
1184-1222: LGTM on fzf binary resolution helpers.The local binary path logic correctly:
- Uses XDG-style directory (
~/.local/share/fpf/fzf)- Handles Windows (
fzf.exe) vs Unix (fzf) binary names- Verifies execute permission on Unix, skips on Windows
- Falls back to system PATH when local binary is unavailable
1288-1312: LGTM on version resolution logic.The redirect-based version detection is a clean approach that avoids parsing JSON from the GitHub API. The
CheckRedirectfunction correctly stops at the first redirect to extract the version from theLocationheader.
1337-1360: LGTM on tar.gz extraction.The extraction logic correctly:
- Wraps the gzip reader around tar reader
- Uses
filepath.Baseto handle nested paths- Verifies the entry is a regular file (
tar.TypeReg)- Returns clear error when binary not found
1362-1385: LGTM on ZIP extraction.Reading the entire ZIP into memory is acceptable given fzf archives are small (~3-4MB). The
defer rc.Close()inside the loop is correct since the function returns immediately after finding the binary.
661-662: LGTM on consistent fzf binary resolution.All fzf subprocess invocations now use
resolveFzfBinaryPath(), ensuring the local per-user binary is preferred when available. This is consistently applied acrossrunFuzzySelectorGo,fzfSupportsListenGo,checkFzfVersionMin, andfzfSupportsResultBindGo.
1117-1134: LGTM on fzf availability check update.The function now correctly checks for a local binary first (via
localFzfBinaryPath()) before falling back to the system PATH. Test mode overrides are preserved.
Fixed 1 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
1 participant
Problem
Live search (
change:reload:) silently stops updating on Linux when the user types quickly. The user has to manually press Ctrl+R to refresh the list.This is caused by a race condition in fzf versions < 0.56.1 (junegunn/fzf#4070): when reloading input very quickly, fzf's reader terminate method is called before the new reader process is properly spawned. The process is never killed and input is not reloaded until it completes naturally.
Debian/Ubuntu repos ship fzf 0.44.1 which is affected. macOS (Homebrew) ships latest and is unaffected.
Solution
ensureFzfGo()now checks fzf version >= 0.56.1 in addition to checking existence:.tar.gzformat.zipformat~/.local/share/fpf/fzf/and preferred over system fzfAll fzf invocations now use
resolveFzfBinaryPath()which checks the local binary first.Platform coverage
Testing
go buildpassesgo vetcleaninstallFzfFromReleaseFallbackGopreserved