The get_process_using_port function shells out to lsof and ps on Unix systems. This approach has a few drawbacks:

• It creates a dependency on external command-line tools that may not be installed.
• It's less portable and can be fragile if the output format of these tools changes.

The listeners crate, which is already a dependency in this project, can provide this information in a more robust and platform-independent way. You can use listeners::get_all() to get a list of all listening sockets and their associated processes, and then find the process using the conflicting port.

This change would replace both the unix and not(unix) implementations of get_process_using_port with a single, more reliable one. You'll also need to add use listeners; at the top of the file.

fn get_process_using_port(port: u16) -> Option<(u32, String)> {
    listeners::get_all()
        .ok()?
        .into_iter()
        .find(|listener| listener.socket.port() == port)
        .map(|listener| (listener.process.pid, listener.process.name))
}

The maximum number of auto-bump attempts is set to 3. This might be too low in a busy environment where several consecutive ports are taken. Consider increasing this to a higher value, for example, 10, to make the auto-bump feature more robust.

The number of attempts reported in the NoAvailablePort error is MAX_BUMP_ATTEMPTS. However, the loop runs from 0..=MAX_BUMP_ATTEMPTS, which is MAX_BUMP_ATTEMPTS + 1 total attempts (including the initial one). The error message should reflect the total number of attempts to avoid confusion.

The current check for port availability doesn't correctly handle port 0. In networking, binding to port 0 is a special case that asks the OS for any available ephemeral port. This implementation binds to port 0 and immediately drops the listener, which succeeds but doesn't reserve a port or determine which port was assigned. The daemon would then receive PORT=0 and might fail or bind to a different ephemeral port than intended, and readiness checks on port 0 would fail.

If you intend to support dynamic port assignment, you should handle port 0 by binding to it, retrieving the assigned port number from the TcpListener, and using that port number going forward. This would need to be coordinated with the other ports being checked.

The port availability check binds to 127.0.0.1. This may not correctly detect a port conflict if a process is listening on 0.0.0.0 (all interfaces) or another specific network interface. A process can bind to 0.0.0.0:8080 and another to 127.0.0.1:8080 simultaneously on some systems, which would lead to this check passing incorrectly. To make the check more robust, you should bind to 0.0.0.0.

The run command, which is used for ad-hoc daemons, doesn't seem to support the new port management features (--expected-port, --auto-bump-port). The StartOptions are hardcoded with expected_port: None and auto_bump_port: false. To be consistent with the start command and provide full functionality for ad-hoc daemons, consider adding these flags to the Run struct and passing them through.

The field port: Vec<u16> in RunOptions is a bit ambiguous. It represents the expected ports for the daemon to use. However, in the Daemon struct, port represents the resolved ports after auto-bumping. For better clarity and consistency across the codebase (e.g., with StartOptions), consider renaming this field to expected_port. This change would require updates in other parts of the code where RunOptions is constructed or used, but it would improve overall code clarity.

When retrying a failed daemon, the RunOptions are created using port: daemon.port, which is the previously resolved port. This could cause issues if that port is no longer available. It would be more robust to use the daemon.original_port and let the port resolution logic (including auto-bumping) run again, ensuring consistent behavior with an initial start. Also, auto_bump_port is hardcoded to false, which prevents finding a new port if the previously resolved one is now taken. It should use daemon.auto_bump_port.

When restarting a daemon due to a cron trigger, the RunOptions are created using port: daemon.port, which is the previously resolved port. It would be more robust and consistent to use the daemon.original_port and let the port resolution logic (including auto-bumping) run again. This ensures the behavior is the same as a fresh start, in case the port availability has changed.

When restarting a watched daemon, it's using the previously resolved port (daemon.port). For consistency and robustness, it's better to use daemon.original_port and let the full port resolution logic run again. This ensures the behavior is the same as a fresh start.

Using saturating_add here is problematic. If an expected_port is near u16::MAX, it will get stuck at 65535 on subsequent bumps, repeatedly checking a port that might be taken. More importantly, the overflow check on line 885 is currently dead code because saturating_add never wraps to 0, and the comment on line 884 is also incorrect.

Using wrapping_add will make the overflow check work as intended and correctly handle bumping ports near the top of the u16 range. You should also update the comment on line 884 to mention wrapping_add.

The listeners::get_all() call inside this function performs blocking I/O (e.g., reading from /proc or running lsof). Since this function is called from an async context in check_ports_available, it should be wrapped in tokio::task::spawn_blocking to avoid blocking the async runtime. This can cause performance issues and stalls.

You'll need to make this function async and then .await the call to it in check_ports_available on line 863.

This if/else block can be expressed more concisely using bool::then_some, which is idiomatic in modern Rust.

            expected_port: (!self.expected_port.is_empty()).then_some(self.expected_port.clone()),

This if/else block can be expressed more concisely using bool::then_some, which is idiomatic in modern Rust.

            expected_port: (!self.expected_port.is_empty()).then_some(self.expected_port.clone()),

The attempts count here is incorrect. It should reflect the number of attempts made so far, which is bump_offset + 1, not the maximum number of attempts.

This logic for determining which ports to display can be simplified using Option combinators for better readability.

        let ports_to_show = daemon
            .filter(|d| !d.port.is_empty())
            .map(|d| d.port.clone())
            .unwrap_or_else(|| cfg.expected_port.clone());

For consistency with the run and start commands, consider renaming this flag to --expected-port and the field to expected_port. The run and start commands use --expected-port for this purpose, while using --port for the readiness check port. This change would make the CLI more consistent and less confusing for users.

If you make this change, you'll also need to update line 215 to use self.expected_port.

Using unwrap_or_else would be slightly more efficient here, as it avoids cloning daemon_config.expected_port if opts.expected_port is Some.

For consistency with the start command, it would be beneficial to also print the resolved ports here. This provides useful feedback to the user, especially when port auto-bumping occurs during the restart.

            for (id, start_time, resolved_ports) in &result.started {
                if let Err(e) = print_startup_logs(id, *start_time) {
                    debug!("Failed to print startup logs for {id}: {e}");
                }
                if !resolved_ports.is_empty() {
                    let port_str = resolved_ports
                        .iter()
                        .map(|p| p.to_string())
                        .collect::<Vec<_>>()
                        .join(", ");
                    println!("Daemon '{id}' restarted on port(s): {port_str}");
                }
            }

This can be made more concise and efficient by using Itertools::join, which is already a dependency in the project. This avoids creating an intermediate Vec.

You'll need to add use itertools::Itertools; to the top of the file.

This can be made more concise and efficient by using Itertools::join, which is already a dependency in the project. This avoids creating an intermediate Vec.

You'll need to add use itertools::Itertools; to the top of the file.

The return type tuple (String, Option<DateTime<Local>>, Option<i32>, Vec<u16>) is becoming complex, as indicated by the #[allow(clippy::type_complexity)]. To improve readability and maintainability, consider defining a dedicated struct for this result. This would make the code easier to understand and modify in the future.

For example:

struct SpawnTaskResult {
    id: String,
    start_time: Option<DateTime<Local>>,
    exit_code: Option<i32>,
    resolved_ports: Vec<u16>,
}

You could then change the return type to tokio::task::JoinHandle<SpawnTaskResult>.

The maximum number of auto-bump attempts is hardcoded. Consider making this configurable in pitchfork.toml to provide more flexibility for users who might need more or fewer attempts depending on their environment (e.g., a port_bump_attempts field in the daemon config).