Skip to content

Commit 4eb4646

Browse files
committed
fix(sound): check dup2 return value and document race condition trade-off
Address Greptile code review feedback: 1. Check dup2 return value during stderr restoration and log errors via tracing::error if restoration fails. Only close original_stderr if dup2 succeeds, preserving it for potential debugging. 2. Add comprehensive documentation about the thread safety implications of temporarily redirecting process-wide stderr during ALSA init.
1 parent 1eb53f3 commit 4eb4646

1 file changed

Lines changed: 31 additions & 2 deletions

File tree

src/cortex-tui/src/sound.rs

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,22 @@ const APPROVAL_WAV: &[u8] = include_bytes!("sounds/approval.wav");
5353
/// hardware is available (e.g., "ALSA lib confmisc.c: cannot find card 0").
5454
/// This function suppresses those messages by temporarily redirecting stderr
5555
/// to /dev/null during initialization.
56+
///
57+
/// # Thread Safety Note
58+
///
59+
/// This function temporarily redirects the process-wide stderr file descriptor (fd 2)
60+
/// to /dev/null. This is a global operation that affects all threads. Any concurrent
61+
/// stderr writes from other threads or libraries will be silently dropped during the
62+
/// brief window when ALSA initialization occurs.
63+
///
64+
/// This trade-off is acceptable because:
65+
/// - The redirection window is very short (only during `OutputStream::try_default()`)
66+
/// - This function is called once at startup from a dedicated audio thread
67+
/// - ALSA error messages are noisy and unhelpful on headless systems
68+
/// - The alternative (letting ALSA spam stderr) degrades user experience significantly
69+
///
70+
/// If stricter isolation is needed, consider calling `init()` before spawning other
71+
/// threads that may write to stderr.
5672
#[cfg(all(feature = "audio", target_os = "linux"))]
5773
fn try_create_output_stream() -> Option<(rodio::OutputStream, rodio::OutputStreamHandle)> {
5874
use std::os::unix::io::AsRawFd;
@@ -110,8 +126,21 @@ fn try_create_output_stream() -> Option<(rodio::OutputStream, rodio::OutputStrea
110126
// Restore the original stderr
111127
// SAFETY: dup2 and close are safe with valid file descriptors
112128
unsafe {
113-
libc::dup2(original_stderr, 2);
114-
libc::close(original_stderr);
129+
let restore_result = libc::dup2(original_stderr, 2);
130+
if restore_result == -1 {
131+
// dup2 failed to restore stderr - this is a critical issue as stderr
132+
// will remain redirected to /dev/null for the rest of the process.
133+
// Log the error (which ironically may not be visible if stderr is broken).
134+
// Keep original_stderr open in case we can retry later or for debugging.
135+
tracing::error!(
136+
"Failed to restore stderr after ALSA initialization (dup2 returned -1). \
137+
Stderr may remain redirected to /dev/null."
138+
);
139+
} else {
140+
// Only close original_stderr if dup2 succeeded, as we may still need
141+
// it for retry attempts if restoration failed.
142+
libc::close(original_stderr);
143+
}
115144
}
116145

117146
match result {

0 commit comments

Comments
 (0)