fix(viewer): isolate chafa terminal IO#62
Conversation
Reviewer's GuideRefactors the chafa viewer loader to run chafa in a fully isolated, non-interactive way using Command::output with terminal probing/passthrough disabled, simplifying process handling while preserving error reporting and ANSI parsing. Sequence diagram for isolated chafa viewer executionsequenceDiagram
participant Viewer as ViewerLoader
participant Cmd as Command
participant Chafa as chafa_process
Viewer->>Cmd: Command::new(chafa)
Cmd->>Cmd: arg(-f symbols)
Cmd->>Cmd: arg(--probe off)
Cmd->>Cmd: arg(--passthrough none)
Cmd->>Cmd: arg(--polite on)
Cmd->>Cmd: arg(--size <widthxheight>)
Cmd->>Cmd: arg(-- path)
Cmd->>Cmd: stdin(Stdio::null)
Cmd->>Chafa: output()
Chafa-->>Cmd: std::process::Output
alt out.status.success()
Cmd->>Cmd: stdout.into_text()
alt into_text Ok
Cmd-->>Viewer: Text
else into_text Err
Cmd-->>Viewer: Text::raw("Failed to parse ANSI: ...")
end
else out.status not success
Cmd->>Cmd: String::from_utf8_lossy(stderr)
Cmd-->>Viewer: Text::raw("Chafa error: ...")
end
Note over Viewer: On spawn error: Text::raw("Failed to execute chafa ...")
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the execution of the chafa command by switching from a spawned child process with a manual timeout loop to a blocking .output() call, while also disabling terminal probing and passthrough to prevent input interference. However, removing the timeout mechanism introduces a risk where a hung chafa process could leak background threads and system resources indefinitely. It is recommended to retain a timeout mechanism to safely terminate the process if it hangs.
| match std::process::Command::new("chafa") | ||
| .arg("-f") | ||
| .arg("symbols") | ||
| .arg("--probe") | ||
| .arg("off") | ||
| .arg("--passthrough") | ||
| .arg("none") | ||
| .arg("--polite") | ||
| .arg("on") | ||
| .arg("--size") | ||
| .arg(&size_str) | ||
| .arg("--") | ||
| .arg(path) | ||
| .stdout(std::process::Stdio::piped()) | ||
| .stderr(std::process::Stdio::piped()) | ||
| .spawn() | ||
| .stdin(std::process::Stdio::null()) | ||
| .output() | ||
| { | ||
| Ok(c) => c, | ||
| Err(e) => return Text::raw(format!("Failed to execute chafa (is it installed?): {}", e)), | ||
| }; | ||
|
|
||
| let deadline = Instant::now() + CHAFA_TIMEOUT; | ||
| loop { | ||
| match child.try_wait() { | ||
| Ok(Some(status)) => { | ||
| let out = child | ||
| .wait_with_output() | ||
| .unwrap_or_else(|_| std::process::Output { | ||
| status, | ||
| stdout: Vec::new(), | ||
| stderr: Vec::new(), | ||
| }); | ||
| if out.status.success() { | ||
| return match out.stdout.into_text() { | ||
| Ok(text) => text, | ||
| Err(e) => Text::raw(format!("Failed to parse ANSI: {}", e)), | ||
| }; | ||
| } | ||
| let err_msg = String::from_utf8_lossy(&out.stderr); | ||
| return Text::raw(format!("Chafa error: {}", err_msg)); | ||
| } | ||
| Ok(None) => { | ||
| if Instant::now() >= deadline { | ||
| let _ = child.kill(); | ||
| let _ = child.wait(); | ||
| return Text::raw("Chafa timed out".to_string()); | ||
| } | ||
| thread::sleep(Duration::from_millis(50)); | ||
| } | ||
| Err(e) => { | ||
| return Text::raw(format!("Chafa wait error: {}", e)); | ||
| } | ||
| Ok(out) if out.status.success() => match out.stdout.into_text() { | ||
| Ok(text) => text, | ||
| Err(e) => Text::raw(format!("Failed to parse ANSI: {}", e)), | ||
| }, | ||
| Ok(out) => { | ||
| let err_msg = String::from_utf8_lossy(&out.stderr); | ||
| Text::raw(format!("Chafa error: {}", err_msg)) | ||
| } | ||
| Err(e) => Text::raw(format!("Failed to execute chafa (is it installed?): {}", e)), | ||
| } |
There was a problem hiding this comment.
By switching from a spawned child with a timeout loop to a blocking .output() call, there is no longer any protection against chafa hanging indefinitely (e.g., when processing a corrupted or extremely complex image, or due to a bug in chafa).
Since ImagePreviewLoader spawns a new thread and drops the old one when navigating files, a hung chafa process will leak both the background thread and the chafa process itself. Over time, this can exhaust system resources (PIDs and memory).
Consider retaining a timeout mechanism to ensure the process is killed if it exceeds a reasonable limit (e.g., 10 seconds).
let mut child = match std::process::Command::new("chafa")
.arg("-f")
.arg("symbols")
.arg("--probe")
.arg("off")
.arg("--passthrough")
.arg("none")
.arg("--polite")
.arg("on")
.arg("--size")
.arg(&size_str)
.arg("--")
.arg(path)
.stdin(std::process::Stdio::null())
.stdout(std::process::Stdio::piped())
.stderr(std::process::Stdio::piped())
.spawn()
{
Ok(c) => c,
Err(e) => return Text::raw(format!("Failed to execute chafa (is it installed?): {}", e)),
};
let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10);
loop {
match child.try_wait() {
Ok(Some(status)) => {
let out = child
.wait_with_output()
.unwrap_or_else(|_| std::process::Output {
status,
stdout: Vec::new(),
stderr: Vec::new(),
});
if out.status.success() {
return match out.stdout.into_text() {
Ok(text) => text,
Err(e) => Text::raw(format!("Failed to parse ANSI: {}", e)),
};
}
let err_msg = String::from_utf8_lossy(&out.stderr);
return Text::raw(format!("Chafa error: {}", err_msg));
}
Ok(None) => {
if std::time::Instant::now() >= deadline {
let _ = child.kill();
let _ = child.wait();
return Text::raw("Chafa timed out".to_string());
}
std::thread::sleep(std::time::Duration::from_millis(50));
}
Err(e) => {
return Text::raw(format!("Chafa wait error: {}", e));
}
}
}![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- By switching from a manual loop with
CHAFA_TIMEOUTtoCommand::output(), the chafa call can now block indefinitely; consider reintroducing a timeout (e.g., using a separate thread or async with a timeout) so a hung chafa process cannot freeze the viewer. - For non-successful chafa executions you only surface
stderr, but wheninto_text()fails you return only the parse error; consider including at least part ofstderrin that case as well to make diagnosing chafa issues easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- By switching from a manual loop with `CHAFA_TIMEOUT` to `Command::output()`, the chafa call can now block indefinitely; consider reintroducing a timeout (e.g., using a separate thread or async with a timeout) so a hung chafa process cannot freeze the viewer.
- For non-successful chafa executions you only surface `stderr`, but when `into_text()` fails you return only the parse error; consider including at least part of `stderr` in that case as well to make diagnosing chafa issues easier.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Greptile SummaryThis PR fixes a bug where
Confidence Score: 3/5The terminal-isolation fix is correct and addresses a real UX bug, but removing the 10-second hard timeout introduces a new reliability hole — a hung chafa process will leave its background thread blocked indefinitely with no cleanup path. The core change (chafa terminal isolation via --probe off / --passthrough none) is sound. The concern is the dropped timeout: the original code killed the child after 10 seconds and cleaned up cleanly; the replacement output() call blocks forever if chafa stalls on a corrupt image or a filesystem hiccup. Since ImagePreviewLoader drops the JoinHandle without joining, orphaned threads accumulate silently. This is a real regression in robustness for the image-preview path. src/ui/viewer/loader.rs — specifically the run_chafa function and how the calling ImagePreviewLoader thread handles a non-terminating child process. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as UI Thread
participant IPL as ImagePreviewLoader (bg thread)
participant Chafa as chafa process
UI->>IPL: ImagePreviewLoader::start(path, w, h)
IPL->>Chafa: "Command::new("chafa")<br/>--probe off --passthrough none<br/>--polite on --size WxH<br/>stdin=null"
Note over Chafa: Renders image to ANSI symbols<br/>No terminal probing/passthrough
Chafa-->>IPL: stdout (ANSI text), exit status
IPL->>IPL: stdout.into_text()
IPL-->>UI: tx.send((width, height, text))
UI->>UI: try_recv() → render preview
Note over IPL,Chafa: If chafa hangs: thread blocks indefinitely<br/>(no timeout/kill in new code)
Reviews (1): Last reviewed commit: "fix(viewer): isolate chafa terminal IO" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub fn run_chafa(path: &Path, width: u16, height: u16) -> Text<'static> { | ||
| let size_str = format!("{}x{}", width, height); | ||
| let mut child = match std::process::Command::new("chafa") | ||
| // Keep terminal probing and passthrough disabled. If chafa talks directly | ||
| // to the terminal, crossterm can read those responses as viewer input and | ||
| // open the search dialog instead of showing the image preview. | ||
| match std::process::Command::new("chafa") | ||
| .arg("-f") | ||
| .arg("symbols") | ||
| .arg("--probe") | ||
| .arg("off") | ||
| .arg("--passthrough") | ||
| .arg("none") | ||
| .arg("--polite") | ||
| .arg("on") | ||
| .arg("--size") | ||
| .arg(&size_str) | ||
| .arg("--") | ||
| .arg(path) | ||
| .stdout(std::process::Stdio::piped()) | ||
| .stderr(std::process::Stdio::piped()) | ||
| .spawn() | ||
| .stdin(std::process::Stdio::null()) | ||
| .output() | ||
| { | ||
| Ok(c) => c, | ||
| Err(e) => return Text::raw(format!("Failed to execute chafa (is it installed?): {}", e)), | ||
| }; | ||
|
|
||
| let deadline = Instant::now() + CHAFA_TIMEOUT; | ||
| loop { | ||
| match child.try_wait() { | ||
| Ok(Some(status)) => { | ||
| let out = child | ||
| .wait_with_output() | ||
| .unwrap_or_else(|_| std::process::Output { | ||
| status, | ||
| stdout: Vec::new(), | ||
| stderr: Vec::new(), | ||
| }); | ||
| if out.status.success() { | ||
| return match out.stdout.into_text() { | ||
| Ok(text) => text, | ||
| Err(e) => Text::raw(format!("Failed to parse ANSI: {}", e)), | ||
| }; | ||
| } | ||
| let err_msg = String::from_utf8_lossy(&out.stderr); | ||
| return Text::raw(format!("Chafa error: {}", err_msg)); | ||
| } | ||
| Ok(None) => { | ||
| if Instant::now() >= deadline { | ||
| let _ = child.kill(); | ||
| let _ = child.wait(); | ||
| return Text::raw("Chafa timed out".to_string()); | ||
| } | ||
| thread::sleep(Duration::from_millis(50)); | ||
| } | ||
| Err(e) => { | ||
| return Text::raw(format!("Chafa wait error: {}", e)); | ||
| } | ||
| Ok(out) if out.status.success() => match out.stdout.into_text() { | ||
| Ok(text) => text, | ||
| Err(e) => Text::raw(format!("Failed to parse ANSI: {}", e)), | ||
| }, | ||
| Ok(out) => { | ||
| let err_msg = String::from_utf8_lossy(&out.stderr); | ||
| Text::raw(format!("Chafa error: {}", err_msg)) | ||
| } | ||
| Err(e) => Text::raw(format!("Failed to execute chafa (is it installed?): {}", e)), | ||
| } |
There was a problem hiding this comment.
The old implementation used CHAFA_TIMEOUT (10 s) with an explicit child.kill() + child.wait() fallback. The new code calls .output(), which blocks until the child exits. If chafa hangs on a malformed or very large image, the ImagePreviewLoader background thread never unblocks. The Drop impl drops the JoinHandle without joining it (let _ = self._handle.take()), so the thread is simply orphaned. Rapid file-panel navigation across many images could accumulate tens of stuck threads, each holding OS resources until the chafa child eventually exits (or the process does).
Previously, the `chafa` process could hang indefinitely when rendering an image preview, blocking the UI. This change introduces a 10-second timeout for `chafa` by spawning it and monitoring its execution. If `chafa` does not complete within the deadline, it is killed, and a timeout error is reported.
1 participant
Summary by Sourcery
Isolate chafa execution from the terminal to prevent its IO from interfering with viewer input handling and simplify process management.
Bug Fixes:
Enhancements: