fix: process recovery#4
Conversation
Performance Test Results
Status: ✅ Performance within acceptable thresholds Tested on: ubuntu-latest, x86_64 |
There was a problem hiding this comment.
Pull request overview
Fixes Kakoune ↔ server FIFO update flow by improving server/process recovery and reducing idle CPU usage during FIFO reads.
Changes:
- Reduce idle CPU polling in the Rust FIFO reader by using
poll()before reads. - Add server/process recovery logic in
giallo-buffer-update(restart on dead/wrong PID, missing server FIFO, PID change). - Trigger a buffer update once the per-buffer FIFO path becomes available after init.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/fifo.rs | Introduces poll()-based waiting to avoid tight loops when FIFO has no writers/data. |
| rc/giallo.kak | Adds server restart/re-init checks and a new hook to force an update after FIFO init completes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let poll_result = unsafe { libc::poll(&mut poll_fd, 1, 250) }; | ||
| if poll_result < 0 { | ||
| log::warn!("reader: poll error: {}", io::Error::last_os_error()); | ||
| thread::sleep(std::time::Duration::from_millis(50)); | ||
| continue; | ||
| } | ||
| if poll_result == 0 { | ||
| continue; | ||
| } | ||
| if poll_fd.revents & libc::POLLIN == 0 { | ||
| if poll_fd.revents & libc::POLLHUP != 0 { | ||
| thread::sleep(std::time::Duration::from_millis(100)); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
poll() errors are always logged and slept on. poll() can legitimately return -1 with EINTR, which will currently produce noisy warnings and unnecessary delays. Also, if revents contains POLLERR/POLLNVAL without POLLIN, the loop will silently continue, potentially spinning forever on a broken fd. Consider special-casing Interrupted to just retry, and treating POLLERR|POLLNVAL as an error that triggers a reopen/exit (with a clear log).
| # Write to FIFO in background to avoid blocking UI if server isn't ready yet. | ||
| # INIT format: INIT <session> <buffer> <token> <lang> <theme> | ||
| sh -c "printf 'INIT %s %s %s %s %s\n' '$session' '$buffer' '$token' '$lang' '$theme' > '$fifo'" >/dev/null 2>&1 & | ||
| printf 'hook -once buffer BufSetOption giallo_buf_fifo_path=.* %%{ giallo-buffer-update }\n' |
There was a problem hiding this comment.
giallo-enable already registers a hook -once buffer BufSetOption giallo_buf_fifo_path=.* handler (lines 129-135) and giallo-init-buffer now adds another one. If giallo-init-buffer is called repeatedly before giallo_buf_fifo_path is set (e.g. during reconnect), this can enqueue multiple -once hooks and cause duplicate giallo-buffer-update runs once the option is set. Consider de-duplicating by using a hook -group and removing/replacing it, or rely on a single existing hook for this event.
| printf 'hook -once buffer BufSetOption giallo_buf_fifo_path=.* %%{ giallo-buffer-update }\n' |
| if [ -n "$server_pid" ]; then | ||
| if ! kill -0 "$server_pid" >/dev/null 2>&1; then | ||
| printf 'giallo-stop-server\n' | ||
| printf 'giallo-start-server\n' | ||
| printf 'giallo-init-buffer\n' | ||
| exit 0 | ||
| fi | ||
| server_comm=$(ps -p "$server_pid" -o comm= 2>/dev/null || true) | ||
| case "$server_comm" in | ||
| (*giallo-kak*) ;; | ||
| (*) | ||
| printf 'giallo-stop-server\n' | ||
| printf 'giallo-start-server\n' | ||
| printf 'giallo-init-buffer\n' | ||
| exit 0 | ||
| ;; | ||
| esac | ||
| fi | ||
|
|
||
| if [ -n "$server_req" ] && [ ! -p "$server_req" ]; then | ||
| printf 'giallo-stop-server\n' | ||
| printf 'giallo-start-server\n' | ||
| printf 'giallo-init-buffer\n' | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Update the last update timestamp | ||
|
|
||
| server_pid_seen="$kak_opt_giallo_server_pid_seen" | ||
| if [ -n "$server_pid" ] && [ "$server_pid" != "$server_pid_seen" ]; then | ||
| printf 'set-option buffer giallo_server_pid_seen %s\n' "$server_pid" | ||
| printf 'giallo-init-buffer\n' | ||
| exit 0 |
There was a problem hiding this comment.
The new server recovery paths (PID liveness check + ps validation + giallo_server_pid_seen-based re-init) introduce important behavior that isn't exercised anywhere in the existing test suite. Adding/strengthening an E2E test that kills the server and verifies giallo-buffer-update automatically restarts and re-initializes the buffer would help prevent regressions.
| @@ -165,6 +167,7 @@ define-command -docstring "Initialize per-buffer FIFO for giallo" giallo-init-bu | |||
| # Write to FIFO in background to avoid blocking UI if server isn't ready yet. | |||
| # INIT format: INIT <session> <buffer> <token> <lang> <theme> | |||
| sh -c "printf 'INIT %s %s %s %s %s\n' '$session' '$buffer' '$token' '$lang' '$theme' > '$fifo'" >/dev/null 2>&1 & | |||
There was a problem hiding this comment.
The sh -c invocation in giallo-init-buffer builds a shell command string that embeds unescaped values like session, buffer, and token inside single quotes, which can lead to shell command injection if any of these contain a single quote or shell metacharacters (e.g., from a crafted buffer name or session name). An attacker who can influence kak_bufname (for example via a file with a specially crafted name) could break out of the intended quoting and inject arbitrary commands executed with the user's privileges when giallo-init-buffer runs. To fix this, avoid constructing a shell command string with these values (e.g., write directly to the FIFO in the current shell or pass values as positional parameters) or ensure that all embedded values are robustly escaped for the shell, including single quotes.
| sh -c "printf 'INIT %s %s %s %s %s\n' '$session' '$buffer' '$token' '$lang' '$theme' > '$fifo'" >/dev/null 2>&1 & | |
| ( printf 'INIT %s %s %s %s %s\n' "$session" "$buffer" "$token" "$lang" "$theme" >"$fifo" ) >/dev/null 2>&1 & |
fixes #3