0.7.0

Author: nbari

.travis.yml

@@ -1,3 +1,34 @@
+## 0.7.0
+
+### Fixed
+* **`close()` no longer retries on `EINTR`** — Previously, the internal `close_retry` helper
+  looped on `EINTR`, which is unsafe on all modern Unixes:
+  - **Linux**: `close()` always releases the fd *before* returning `EINTR`.
+    Retrying can close an unrelated fd opened by another thread between attempts.
+  - **FreeBSD / macOS / other BSDs**: The fd state after `close()` + `EINTR` is
+    *unspecified* per POSIX 2008+ (Austin Group defect 529), so retrying is equally dangerous.
+  - The function has been renamed from `close_retry` to `close_once` and now calls `close()`
+    exactly once, treating both `EINTR` and `EBADF` as success — the same approach used by
+    Rust's `std::fs::File::drop()`, Go's runtime, and glibc internals.
+  - **Note**: `open()` and `dup2()` in `redirect_stdio()` still correctly retry on `EINTR`,
+    as those calls do *not* release resources on interruption.
+
+### Improved
+* **`daemon()` return value documentation** — Made it prominent that `daemon()` only ever
+  returns `Ok(Fork::Child)` or `Err(...)` to the caller; `Ok(Fork::Parent(_))` is never
+  returned because both parent processes call `_exit(0)` internally. Added recommended
+  `if let` and `match` usage patterns to the doc comment. Updated `#[must_use]` message
+  to reflect this guarantee.
+
+### Code Quality
+* Added `test_daemon_never_returns_parent` integration test confirming `Fork::Parent` is unreachable
+* Renamed `test_close_retry_ok_and_ebadf` to `test_close_once_ok_and_ebadf`
+* Replaced fragile `tty` command string-matching in `test_daemon_no_controlling_terminal` with
+  portable `open("/dev/tty")` check (works reliably across Linux, macOS, and BSDs)
+* Replaced fixed 100ms sleep in `test_getppid_after_parent_exits` with a retry loop (up to 1s),
+  preventing flaky failures on slow CI systems
+* Updated tests README to reflect new and renamed tests
+
 ## 0.6.0
 
 ### Breaking Changes

CHANGELOG.md

@@ -1,6 +1,6 @@
 [package]
 name = "fork"
-version = "0.6.0"
+version = "0.7.0"
 authors = ["Nicolas Embriz <nbari@tequila.io>"]
 description = "Library for creating a new process detached from the controlling terminal (daemon)"
 documentation = "https://docs.rs/fork/latest/fork/"

Cargo.toml

@@ -30,7 +30,7 @@ Add `fork` to your `Cargo.toml`:
 
 ```toml
 [dependencies]
-fork = "0.6.0"
+fork = "0.7.0"
 ```
 
 Or use cargo-add:

README.md

@@ -148,27 +148,28 @@ pub enum Fork {
     Child,
 }
 
-/// Close a file descriptor, retrying on `EINTR` and treating `EBADF` as success.
+/// Close a file descriptor without retrying on `EINTR`, treating `EBADF` as success.
+///
+/// On Linux, `close()` always releases the fd before returning `EINTR`, so retrying
+/// would risk closing an unrelated fd opened by another thread. On FreeBSD, macOS,
+/// and other Unixes the fd state after `EINTR` is unspecified (POSIX 2008+, Austin
+/// Group defect 529), making retry equally unsafe. The safe portable behavior is to
+/// call `close()` exactly once and treat `EINTR` as success — the same approach used
+/// by Rust's stdlib, Go's runtime, and glibc internals.
 #[inline]
-fn close_retry(fd: libc::c_int) -> io::Result<()> {
-    loop {
-        let res = unsafe { libc::close(fd) };
-        if res == 0 {
-            return Ok(());
-        }
-
-        let err = io::Error::last_os_error();
-
-        if err.kind() == io::ErrorKind::Interrupted {
-            continue;
-        }
+fn close_once(fd: libc::c_int) -> io::Result<()> {
+    let res = unsafe { libc::close(fd) };
+    if res == 0 {
+        return Ok(());
+    }
 
-        if err.raw_os_error() == Some(libc::EBADF) {
-            return Ok(());
-        }
+    let err = io::Error::last_os_error();
 
-        return Err(err);
+    if err.kind() == io::ErrorKind::Interrupted || err.raw_os_error() == Some(libc::EBADF) {
+        return Ok(());
     }
+
+    Err(err)
 }
 
 impl Fork {
@@ -307,7 +308,7 @@ pub fn chdir() -> io::Result<()> {
 /// ```
 pub fn close_fd() -> io::Result<()> {
     for fd in 0..=2 {
-        close_retry(fd)?;
+        close_once(fd)?;
     }
 
     Ok(())
@@ -377,7 +378,7 @@ pub fn redirect_stdio() -> io::Result<()> {
                 // Only close null_fd if it's > 2 (not one of the stdio fds we're duplicating to)
                 // If null_fd was 0, 1, or 2, we're in the process of duping it, so don't close
                 if null_fd > 2 {
-                    let _ = close_retry(null_fd);
+                    let _ = close_once(null_fd);
                 }
                 return Err(err);
             }
@@ -388,7 +389,7 @@ pub fn redirect_stdio() -> io::Result<()> {
     // Close the extra fd if it's > 2
     // (if null_fd was 0, 1, or 2, it's now dup'd to all three, so don't close)
     if null_fd > 2 {
-        close_retry(null_fd)?;
+        close_once(null_fd)?;
     }
 
     Ok(())
@@ -710,12 +711,51 @@ pub fn getppid() -> libc::pid_t {
 /// * `nochdir = false`, changes the current working directory to the root (`/`).
 /// * `noclose = false`, redirects stdin, stdout, and stderr to `/dev/null`
 ///
+/// # Return Value
+///
+/// This function only ever returns in the **daemon (grandchild) process**:
+///
+/// - `Ok(Fork::Child)` — You are the daemon. The original process and the
+///   intermediate child have already exited via `_exit(0)`.
+/// - `Err(...)` — A system call failed before the daemon could be created.
+///
+/// **`Ok(Fork::Parent(_))` is never returned** because both parent processes
+/// call `_exit(0)` internally. You do not need to match on it:
+///
+/// ```no_run
+/// use fork::{daemon, Fork};
+///
+/// // Recommended: use `if let` — no dead Parent arm needed
+/// if let Ok(Fork::Child) = daemon(false, false) {
+///     // Only the daemon reaches here
+///     loop {
+///         // daemon work…
+///         std::thread::sleep(std::time::Duration::from_secs(60));
+///     }
+/// }
+/// ```
+///
+/// If you prefer `match` for explicit error handling, mark the parent arm
+/// unreachable:
+///
+/// ```no_run
+/// use fork::{daemon, Fork};
+///
+/// match daemon(false, false) {
+///     Ok(Fork::Child) => {
+///         // daemon work…
+///     }
+///     Ok(Fork::Parent(_)) => unreachable!("daemon() exits both parent processes"),
+///     Err(err) => eprintln!("daemon failed: {err}"),
+/// }
+/// ```
+///
 /// # Implementation (double-fork)
 ///
-/// 1. **First fork** - Parent exits immediately.
-/// 2. **Session setup** - Child calls `setsid()`, optionally `chdir("/")`, and optionally redirects stdio.
-/// 3. **Second (double) fork** - Session-leader child exits immediately.
-/// 4. **Daemon continues** - Grandchild (daemon) runs with no controlling terminal.
+/// 1. **First fork** — Parent calls `_exit(0)` immediately.
+/// 2. **Session setup** — Child calls `setsid()`, optionally `chdir("/")`, and optionally redirects stdio.
+/// 3. **Second (double) fork** — Session-leader child calls `_exit(0)` immediately.
+/// 4. **Daemon continues** — Grandchild (daemon) runs with no controlling terminal.
 ///
 /// # Behavior Change in v0.4.0
 ///
@@ -750,7 +790,7 @@ pub fn getppid() -> libc::pid_t {
 ///        .expect("failed to execute process");
 ///}
 ///```
-#[must_use = "daemon result must be checked to determine if this is the daemon process"]
+#[must_use = "daemon() only returns Ok(Fork::Child) in the daemon process; check the result"]
 pub fn daemon(nochdir: bool, noclose: bool) -> io::Result<Fork> {
     // 1. First fork: detach from original parent; parent exits immediately
     match fork()? {
@@ -1119,19 +1159,19 @@ mod tests {
     }
 
     #[test]
-    fn test_close_retry_ok_and_ebadf() {
+    fn test_close_once_ok_and_ebadf() {
         // Create a pipe to obtain valid fds
         let mut fds = [0; 2];
         assert_eq!(unsafe { libc::pipe(&raw mut fds[0]) }, 0);
 
-        // Close write end via close_retry (should succeed)
-        close_retry(fds[1]).expect("close_retry should close valid fd");
+        // Close write end via close_once (should succeed)
+        close_once(fds[1]).expect("close_once should close valid fd");
 
         // Wrap read end in File to close it once; drop immediately
         let read_fd = fds[0];
         unsafe { std::fs::File::from_raw_fd(read_fd) };
 
         // Second close should be treated as success (EBADF path)
-        close_retry(read_fd).expect("EBADF should be treated as success");
+        close_once(read_fd).expect("EBADF should be treated as success");
     }
 }

src/lib.rs

@@ -33,6 +33,7 @@ Tests include:
 - **test_daemon_process_group** - Verifies daemon process group structure (double-fork pattern)
 - **test_daemon_with_command_execution** - Tests command execution in daemon context
 - **test_daemon_no_controlling_terminal** - Verifies daemon has no controlling terminal
+- **test_daemon_never_returns_parent** - Verifies daemon() only returns `Fork::Child`, never `Fork::Parent`
 
 ### `fork_tests.rs` - Fork Functionality Tests
 
@@ -72,7 +73,7 @@ Tests include:
 - **test_redirect_stdio_error_handling** - Propagates errors from failed redirection
 - **test_fd_reuse_corruption_scenario** - Demonstrates corruption risk when closing stdio
 - **test_close_fd_allows_fd_reuse** - Shows fd reuse when stdio is closed (expected panic)
-- **test_close_retry_ok_and_ebadf** - Unit-level check that repeated closes handle EINTR/EBADF gracefully
+- **test_close_once_ok_and_ebadf** - Unit-level check that close handles EBADF gracefully
 
 ### `waitpid_tests.rs` - Waitpid Comprehensive Tests
 
@@ -280,7 +281,7 @@ Integration tests provide coverage for:
 tests/
 ├── common/
 │   └── mod.rs               # Shared utilities (51 lines)
-├── daemon_tests.rs          # Daemon tests (271 lines, 5 tests)
+├── daemon_tests.rs          # Daemon tests (271 lines, 6 tests)
 ├── fork_tests.rs            # Fork tests (301 lines, 7 tests)
 ├── integration_tests.rs     # Advanced tests (284 lines, 7 tests)
 ├── stdio_redirect_tests.rs  # Stdio safety tests (313 lines, 7 tests)

tests/README.md

@@ -225,10 +225,10 @@ fn test_daemon_with_command_execution() {
 fn test_daemon_no_controlling_terminal() {
     // Tests that daemon has no controlling terminal
     // Expected behavior:
-    // 1. Daemon process is created
-    // 2. Daemon calls 'tty' command to check for terminal
-    // 3. tty command should return "not a tty" or similar error
-    // 4. This confirms daemon is properly detached from terminal
+    // 1. Daemon process is created via double-fork + setsid()
+    // 2. Daemon tries to open /dev/tty (the POSIX controlling terminal device)
+    // 3. open() should fail because the daemon has no controlling terminal
+    // 4. This confirms daemon is properly detached
     // 5. Critical for background service behavior
     let test_dir = setup_test_dir(get_unique_test_dir("daemon_no_tty"));
     let tty_file = test_dir.join("tty.info");
@@ -238,9 +238,9 @@ fn test_daemon_no_controlling_terminal() {
             assert!(wait_for_file(&tty_file, 500), "TTY info file should exist");
 
             let content = fs::read_to_string(&tty_file).expect("Failed to read tty file");
-            // When daemon has no controlling terminal, tty command should fail or return "not a tty"
-            assert!(
-                content.contains("not a tty") || content.contains("No such"),
+            assert_eq!(
+                content.trim(),
+                "no_ctty",
                 "Daemon should have no controlling terminal, got: {}",
                 content
             );
@@ -250,21 +250,69 @@ fn test_daemon_no_controlling_terminal() {
         }
         Fork::Child => {
             if let Ok(Fork::Child) = daemon(false, true) {
-                // Check if we have a controlling terminal
-                let output = Command::new("tty")
-                    .output()
-                    .expect("Failed to run tty command");
-
-                let tty_output = if output.stdout.is_empty() {
-                    String::from_utf8_lossy(&output.stderr).to_string()
+                // The POSIX way to check for a controlling terminal:
+                // opening /dev/tty fails when the process has none.
+                let fd =
+                    unsafe { libc::open(c"/dev/tty".as_ptr(), libc::O_RDONLY | libc::O_NOCTTY) };
+                let result = if fd == -1 {
+                    "no_ctty"
                 } else {
-                    String::from_utf8_lossy(&output.stdout).to_string()
+                    unsafe { libc::close(fd) };
+                    "has_ctty"
                 };
 
-                fs::write(&tty_file, tty_output).expect("Failed to write tty file");
-
+                fs::write(&tty_file, result).expect("Failed to write tty file");
                 exit(0);
             }
         }
     }
 }
+
+#[test]
+fn test_daemon_never_returns_parent() {
+    // Tests that daemon() never returns Ok(Fork::Parent(_))
+    // Expected behavior:
+    // 1. daemon() performs double-fork internally
+    // 2. Both parent processes call _exit(0) and never return
+    // 3. Only Ok(Fork::Child) is ever returned to the caller
+    // 4. The daemon writes "child" to a marker file to confirm
+    // 5. If Fork::Parent were ever returned, "parent" would be written instead
+    let test_dir = setup_test_dir(get_unique_test_dir("daemon_never_returns_parent"));
+    let marker_file = test_dir.join("result.marker");
+
+    match fork().expect("Failed to fork") {
+        Fork::Parent(_) => {
+            assert!(
+                wait_for_file(&marker_file, 500),
+                "Result marker file should exist"
+            );
+
+            let content = fs::read_to_string(&marker_file).expect("Failed to read marker file");
+            assert_eq!(
+                content.trim(),
+                "child",
+                "daemon() should only return Fork::Child, never Fork::Parent"
+            );
+
+            // Cleanup
+            let _ = fs::remove_dir_all(&test_dir);
+        }
+        Fork::Child => {
+            match daemon(false, true) {
+                Ok(Fork::Child) => {
+                    fs::write(&marker_file, "child").expect("Failed to write marker");
+                    exit(0);
+                }
+                Ok(Fork::Parent(_)) => {
+                    // This arm should be unreachable
+                    fs::write(&marker_file, "parent").expect("Failed to write marker");
+                    exit(1);
+                }
+                Err(_) => {
+                    fs::write(&marker_file, "error").expect("Failed to write marker");
+                    exit(2);
+                }
+            }
+        }
+    }
+}

tests/daemon_tests.rs

@@ -270,14 +270,20 @@ fn test_getppid_after_parent_exits() {
                     exit(0);
                 }
                 Ok(Fork::Child) => {
-                    // Give parent time to exit
-                    std::thread::sleep(std::time::Duration::from_millis(100));
-
-                    // Grandchild's parent should now be init (PID 1)
-                    let current_parent = getppid();
-                    assert_eq!(
-                        current_parent, 1,
-                        "Orphaned grandchild should be reparented to init (PID 1)"
+                    // Poll until reparented to init (PID 1) with a timeout
+                    let mut reparented = false;
+                    for _ in 0..50 {
+                        if getppid() == 1 {
+                            reparented = true;
+                            break;
+                        }
+                        std::thread::sleep(std::time::Duration::from_millis(20));
+                    }
+
+                    assert!(
+                        reparented,
+                        "Orphaned grandchild should be reparented to init (PID 1), got ppid={}",
+                        getppid()
                     );
 
                     // Verify we're not the original child