fix: improve Linux fzf IPC compatibility#1
Conversation
- Add curl retry loop (3 attempts, 50ms backoff) before falling back to nc - Restore HTTP/1.1 framing for nc fallback path - Retry each nc binary 3 times before moving to next - Use timeout-wrapped nc as final fallback for Linux distros - Bump fzf version check from flag-only to >= 0.48.0 for change:reload: support
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 833-834: The fzf minimum-version check in the change:reload gate
is too low; update the call to checkFzfVersionMin used where change:reload is
enforced (the return currently uses "0.42.0") to require "0.56.1" instead so fzf
>= 0.56.1 is enforced (this ensures the --listen + change:reload race fix is
present). Locate the return using checkFzfVersionMin in the change:reload
handling and replace the version string "0.42.0" with "0.56.1".
In `@cmd/fpf/dynamic_reload.go`:
- Around line 247-257: The retry loop around performReloadHandshake(conn,
config) stores failures into lastErr but still returns lastErr after a later
successful attempt, producing stale error reports; update the loop so that when
performReloadHandshake succeeds you return nil immediately (or explicitly reset
lastErr to nil before returning), and only set lastErr on error cases (in
function performReloadHandshake and the loop surrounding it) so successful
handshakes don't return a previous error.
In `@cmd/fpf/ipc_actions.go`:
- Around line 60-73: The runCurlReload function currently ignores errors from
cmd.Start(), can dereference a nil cmd.Process, and always returns nil so
callers (like runIPCReload) can't detect failures; update runCurlReload to
capture the error returned by cmd.Start(), if err != nil return that error
immediately, only call cmd.Process.Release() after verifying cmd.Process != nil,
and return nil only on success so the caller can fallback on failure. Also
ensure any resources are cleaned on Start error and propagate the error up
rather than swallowing it.
- Around line 76-94: runNetcatReload currently sends raw "reload ...\n" over
TCP; change it to construct a proper HTTP/1.1 POST request with headers and
Content-Length and send that over netcat. Locate runNetcatReload and use
hostPort[0] and hostPort[1] (from strings.Split(host, ":")) to connect, compute
body := fmt.Sprintf("reload(%s)", query) (or "reload "+query if parentheses are
not desired), then build request := fmt.Sprintf("POST / HTTP/1.1\r\nHost:
%s\r\nContent-Length: %d\r\n\r\n%s", hostPort[0], len(body), body) and send that
via netcat/timeoutCmd; remove or repurpose the unused timeoutCmd variable if
necessary. Ensure CRLFs are used for headers and Content-Length matches the byte
length of the body.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4edc5c0a-a120-4d63-8cd6-2d57f7bc389e
📒 Files selected for processing (5)
cmd/fpf/cli_runtime.gocmd/fpf/dynamic_reload.gocmd/fpf/ipc_actions.gofpfpackage.json
📜 Review details
🔇 Additional comments (1)
package.json (1)
3-3: Version bump looks correct for this release scope.
| // change:reload: requires fzf >= 0.42.0 | ||
| return checkFzfVersionMin("0.42.0") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the minimum fzf version required for stable --listen+change:reload behavior used by external IPC reload requests?
💡 Result:
Minimum fzf v0.56.1.
Reason: v0.56.1 fixed a race condition that could leave “stale results after reload”, which directly affects setups that combine change:reload (frequent reloads) with external --listen IPC POST reload requests. [1]
(--listen itself (HTTP server accepting POST action sequences like reload(...)) has existed since v0.36.0.) [2]
Sources:
[1] (github.com)
[2] (github.com)
Citations:
- 1: https://github.com/junegunn/fzf/blob/master/CHANGELOG.md
- 2: https://github.com/junegunn/fzf/blob/master/CHANGELOG.md
Minimum fzf version gate is incorrect and too permissive.
Line 834 uses 0.42.0, but stable --listen + change:reload behavior requires fzf >= 0.56.1. This version fixed a race condition that could leave stale results after reload, which directly impacts external IPC reload requests. The current gate would allow incompatible versions and break dynamic reload at runtime.
🔧 Proposed fix
- // change:reload: requires fzf >= 0.42.0
- return checkFzfVersionMin("0.42.0")
+ // change:reload: requires fzf >= 0.56.1 (race condition fix)
+ return checkFzfVersionMin("0.56.1")🤖 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 833 - 834, The fzf minimum-version check
in the change:reload gate is too low; update the call to checkFzfVersionMin used
where change:reload is enforced (the return currently uses "0.42.0") to require
"0.56.1" instead so fzf >= 0.56.1 is enforced (this ensures the --listen +
change:reload race fix is present). Locate the return using checkFzfVersionMin
in the change:reload handling and replace the version string "0.42.0" with
"0.56.1".
| for attempt := 1; attempt <= maxReloadAttempts; attempt++ { | ||
| if err := performReloadHandshake(conn, config); err != nil { | ||
| lastErr = err | ||
| if attempt < maxReloadAttempts { | ||
| sleepDuration := time.Duration(attempt*attempt*50) * time.Millisecond | ||
| time.Sleep(sleepDuration) | ||
| continue | ||
| } | ||
| } | ||
| return lastErr | ||
| } |
There was a problem hiding this comment.
Retry loop returns a stale error even after a successful retry.
After any earlier failure, lastErr remains set. A later successful handshake still returns that old error.
🔧 Proposed fix
func handleIPCReload(conn net.Conn, config any) error {
var lastErr error
for attempt := 1; attempt <= maxReloadAttempts; attempt++ {
if err := performReloadHandshake(conn, config); err != nil {
lastErr = err
if attempt < maxReloadAttempts {
sleepDuration := time.Duration(attempt*attempt*50) * time.Millisecond
time.Sleep(sleepDuration)
continue
}
- }
- return lastErr
+ return lastErr
+ }
+ return nil
}
return lastErr
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for attempt := 1; attempt <= maxReloadAttempts; attempt++ { | |
| if err := performReloadHandshake(conn, config); err != nil { | |
| lastErr = err | |
| if attempt < maxReloadAttempts { | |
| sleepDuration := time.Duration(attempt*attempt*50) * time.Millisecond | |
| time.Sleep(sleepDuration) | |
| continue | |
| } | |
| } | |
| return lastErr | |
| } | |
| for attempt := 1; attempt <= maxReloadAttempts; attempt++ { | |
| if err := performReloadHandshake(conn, config); err != nil { | |
| lastErr = err | |
| if attempt < maxReloadAttempts { | |
| sleepDuration := time.Duration(attempt*attempt*50) * time.Millisecond | |
| time.Sleep(sleepDuration) | |
| continue | |
| } | |
| return lastErr | |
| } | |
| return nil | |
| } | |
| return lastErr |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/fpf/dynamic_reload.go` around lines 247 - 257, The retry loop around
performReloadHandshake(conn, config) stores failures into lastErr but still
returns lastErr after a later successful attempt, producing stale error reports;
update the loop so that when performReloadHandshake succeeds you return nil
immediately (or explicitly reset lastErr to nil before returning), and only set
lastErr on error cases (in function performReloadHandshake and the loop
surrounding it) so successful handshakes don't return a previous error.
| func runCurlReload(host, query string) error { | ||
| // fzf 0.42+ supports change:reload: which sends HTTP/1.1 POST | ||
| url := "http://" + host + "/fzf_reload" | ||
| payload := "reload " + query | ||
|
|
||
| reloadCmd := buildDynamicReloadCommandForQuery(managerOverride, fallbackFile, managerListCSV, query, bypassQueryCache) | ||
| actionPayload := "change-prompt(Search> )+reload(" + reloadCmd + ")" | ||
| cmd := exec.Command("curl", "-s", "-X", "POST", "-d", payload, | ||
| "--max-time", "1", | ||
| "-H", "Content-Type: application/octet-stream", | ||
| url) | ||
|
|
||
| return sendFzfListenAction(actionPayload) | ||
| // Run without waiting — fire and forget, fzf handles it | ||
| cmd.Start() | ||
| cmd.Process.Release() | ||
| return nil |
There was a problem hiding this comment.
Curl path can panic and falsely report success, preventing fallback.
Line 71 ignores Start() errors, Line 72 can dereference a nil Process, and the function always returns nil. That makes runIPCReload treat failed sends as success.
🔧 Proposed fix
func runCurlReload(host, query string) error {
// fzf 0.42+ supports change:reload: which sends HTTP/1.1 POST
url := "http://" + host + "/fzf_reload"
payload := "reload " + query
cmd := exec.Command("curl", "-s", "-X", "POST", "-d", payload,
"--max-time", "1",
+ "--fail",
"-H", "Content-Type: application/octet-stream",
url)
- // Run without waiting — fire and forget, fzf handles it
- cmd.Start()
- cmd.Process.Release()
- return nil
+ if err := cmd.Run(); err != nil {
+ return fmt.Errorf("curl reload failed: %w", err)
+ }
+ return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func runCurlReload(host, query string) error { | |
| // fzf 0.42+ supports change:reload: which sends HTTP/1.1 POST | |
| url := "http://" + host + "/fzf_reload" | |
| payload := "reload " + query | |
| reloadCmd := buildDynamicReloadCommandForQuery(managerOverride, fallbackFile, managerListCSV, query, bypassQueryCache) | |
| actionPayload := "change-prompt(Search> )+reload(" + reloadCmd + ")" | |
| cmd := exec.Command("curl", "-s", "-X", "POST", "-d", payload, | |
| "--max-time", "1", | |
| "-H", "Content-Type: application/octet-stream", | |
| url) | |
| return sendFzfListenAction(actionPayload) | |
| // Run without waiting — fire and forget, fzf handles it | |
| cmd.Start() | |
| cmd.Process.Release() | |
| return nil | |
| func runCurlReload(host, query string) error { | |
| // fzf 0.42+ supports change:reload: which sends HTTP/1.1 POST | |
| url := "http://" + host + "/fzf_reload" | |
| payload := "reload " + query | |
| cmd := exec.Command("curl", "-s", "-X", "POST", "-d", payload, | |
| "--max-time", "1", | |
| "--fail", | |
| "-H", "Content-Type: application/octet-stream", | |
| url) | |
| if err := cmd.Run(); err != nil { | |
| return fmt.Errorf("curl reload failed: %w", err) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/fpf/ipc_actions.go` around lines 60 - 73, The runCurlReload function
currently ignores errors from cmd.Start(), can dereference a nil cmd.Process,
and always returns nil so callers (like runIPCReload) can't detect failures;
update runCurlReload to capture the error returned by cmd.Start(), if err != nil
return that error immediately, only call cmd.Process.Release() after verifying
cmd.Process != nil, and return nil only on success so the caller can fallback on
failure. Also ensure any resources are cleaned on Start error and propagate the
error up rather than swallowing it.
| func runNetcatReload(host, query string) error { | ||
| // For older fzf: use netcat to send fzf IPC protocol message | ||
| // Protocol: one line per event, \n terminated | ||
| // Event format: "event payload\n" | ||
| hostPort := strings.Split(host, ":") | ||
| if len(hostPort) != 2 { | ||
| return fmt.Errorf("invalid FPF_FZF_LISTEN_HOST: %s (expected host:port)", host) | ||
| } | ||
|
|
||
| return strings.Join(parts, " ") | ||
| } | ||
| payload := fmt.Sprintf("reload %s\n", query) | ||
|
|
||
| func sendFzfListenAction(actionPayload string) error { | ||
| fzfPort := strings.TrimSpace(os.Getenv("FZF_PORT")) | ||
| if fzfPort == "" { | ||
| return fmt.Errorf("missing FZF_PORT") | ||
| // Use timeout(1) wrapper for compatibility across Linux/macOS | ||
| // timeout on Linux, gtimeout on macOS (coreutils) | ||
| timeoutCmd := "timeout" | ||
| if _, err := exec.LookPath("timeout"); err != nil { | ||
| timeoutCmd = "gtimeout" | ||
| } | ||
| _ = timeoutCmd // reserved for future netcat variant use | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For fzf --listen, what exact request format is accepted on the listen socket for reload actions (raw action line vs HTTP/1.1 POST /fzf_reload)?
💡 Result:
fzf --listen runs a tiny HTTP server. For triggering a reload you must send an HTTP/1.x POST to / (not a raw “action line” on the socket, and not POST /fzf_reload). The request line must start with POST / HTTP…; GET /?... is used only for the JSON state endpoint. [1]
The POST body is interpreted as an fzf action list string (same syntax as in --bind, e.g. reload(...), reload-sync(...), chained with +). fzf reads Content-Length, then parses the body via parseSingleActionList(strings.Trim(body, "\r\n")), and queues the resulting actions. [2]
Minimal example (TCP):
POST / HTTP/1.1
Host: localhost
Content-Length: 16
reload(ps -ef)(Optionally include X-API-Key: ... if fzf was started with an API key.) [2][1]
Sources: fzf server.go request parsing / POST handling [1][2]; release notes describing --listen as an HTTP server with GET / JSON state [3].
[1] (github.com)
[2] (github.com)
[3] (junegunn.github.io)
Citations:
- 1: https://github.com/junegunn/fzf/blob/master/src/server.go
- 2: https://github.com/junegunn/fzf/blob/master/src/server.go
- 3: https://junegunn.github.io/fzf/releases/0.54.0/
HTTP/1.1 POST format required but raw protocol sent instead.
runNetcatReload sends raw reload ...\n over TCP, but fzf's --listen requires HTTP/1.1 POST to / with proper headers and Content-Length. The payload must be HTTP-framed like:
POST / HTTP/1.1
Host: localhost
Content-Length: 16
reload(ps -ef)
The timeout/nc setup is currently unused. Update the payload to use proper HTTP/1.1 formatting before sending over netcat.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/fpf/ipc_actions.go` around lines 76 - 94, runNetcatReload currently sends
raw "reload ...\n" over TCP; change it to construct a proper HTTP/1.1 POST
request with headers and Content-Length and send that over netcat. Locate
runNetcatReload and use hostPort[0] and hostPort[1] (from strings.Split(host,
":")) to connect, compute body := fmt.Sprintf("reload(%s)", query) (or "reload
"+query if parentheses are not desired), then build request := fmt.Sprintf("POST
/ HTTP/1.1\r\nHost: %s\r\nContent-Length: %d\r\n\r\n%s", hostPort[0], len(body),
body) and send that via netcat/timeoutCmd; remove or repurpose the unused
timeoutCmd variable if necessary. Ensure CRLFs are used for headers and
Content-Length matches the byte length of the body.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 4 unresolved review comments. A stacked PR containing fixes has been created.
Time taken: |
fix: CodeRabbit auto-fixes for PR #1
1 participant
Fixes real-time fuzzy search on Linux by adding curl retries, proper HTTP/1.1 framing for nc fallback, and bumping fzf version check from 0.42.0 to 0.48.0.