Calling PitchforkToml::all_merged() inside tokio::spawn for every hook execution means that all configuration files are read from disk and parsed every time a hook is fired. While this ensures the hook uses the absolute latest configuration, it can be inefficient, especially for frequently triggered hooks like on_retry in a scenario where a service is flapping. This could lead to performance issues due to repeated disk I/O and parsing overhead.

Since using the latest config is an explicit design goal mentioned in the PR, this is a trade-off. However, it's worth considering if there's a middle ground, perhaps involving a cached configuration that's refreshed periodically, to mitigate potential performance bottlenecks.

There is significant code duplication for handling the 'ready' state. The logic to notify readiness (including flushing logs, sending on the ready_tx channel, and firing the on_ready hook) is repeated in 6 different places within the select! loop. This makes the code harder to maintain and prone to errors if changes are not applied consistently to all copies.

Consider refactoring this logic into a private helper function within the tokio::spawn block. This function could encapsulate setting ready_notified, sending on ready_tx, and firing the hook.

For example:

async fn notify_ready(
    id: &str,
    ready_notified: &mut bool,
    ready_tx: &mut Option<oneshot::Sender<Result<(), Option<i32>>>>,
    log_appender: &mut BufWriter<tokio::fs::File>,
    // ... other params for fire_hook
) {
    if *ready_notified { return; }
    info!("daemon {} ready", id);
    *ready_notified = true;
    let _ = log_appender.flush().await;
    if let Some(tx) = ready_tx.take() {
        let _ = tx.send(Ok(()));
    }
    fire_hook(HookType::OnReady, /* ... */);
}

You could then call notify_ready(...).await in each of the readiness detection branches.

The logic for handling a daemon's failure is duplicated. This block for a successful wait() but failed exit code is very similar to the block in the final else clause (lines 605-629) which handles a wait() error. Both blocks update the daemon status to Errored and fire the on_fail hook if retries are exhausted.

This duplication can be consolidated into a helper function to improve maintainability. The function could take the exit_code as an argument and perform the necessary state updates and hook firing.

Example refactoring:

async fn handle_failure(id: &str, exit_code: i32, hook_retry_count: u32, hook_retry: Retry, /*...other params*/) {
    let err_status = DaemonStatus::Errored(exit_code);
    if let Err(e) = SUPERVISOR
        .upsert_daemon(UpsertDaemonOpts {
            id: id.to_string(),
            pid: None,
            status: err_status,
            last_exit_success: Some(false),
            ..Default::default()
        })
        .await
    {
        error!("Failed to update daemon state for {id}: {e}");
    }
    // Fire on_fail hook if retries are exhausted
    if hook_retry_count >= hook_retry {
        fire_hook(
            HookType::OnFail,
            id.to_string(),
            // ... params
            vec![("PITCHFORK_EXIT_CODE".to_string(), exit_code.to_string())],
        );
    }
}

You could then call this from both failure paths with the appropriate exit_code.

This test logic seems a bit confusing. It appears to start the fail_retry_hook daemon twice. The second run_command_with_env call, which sets PITCHFORK_INTERVAL_SECS=1, is likely ineffective. The supervisor process is started by the first run_command call and will not pick up the environment variable from the second call.

The test probably passes because the first start command uses --delay 1, which triggers the wait_ready retry logic in supervisor/lifecycle.rs. This logic has its own backoff schedule and doesn't depend on PITCHFORK_INTERVAL_SECS.

To make this test clearer and more robust, consider either:

1. Relying only on the wait_ready retry logic and removing the second run_command call.
2. If testing the background retry mechanism is intended, ensure the supervisor is started with the PITCHFORK_INTERVAL_SECS=1 environment variable from the very beginning.

test_hook_on_fail_after_retries starts the same daemon twice (first with --delay 1, then again with PITCHFORK_INTERVAL_SECS=1). The second invocation likely won’t affect the already-running supervisor/daemon, and may just return DaemonAlreadyRunning, making the test flaky or ineffective. Make this a single start call: either (a) remove the first start and run once with run_command_with_env (setting PITCHFORK_INTERVAL_SECS up front), or (b) keep the --delay 1 path and delete the second start entirely.

fire_hook calls PitchforkToml::all_merged() inside a tokio::spawn task, and all_merged() does blocking filesystem I/O. This can block Tokio worker threads under load (especially if hooks fire frequently on retries). Consider loading the merged config via tokio::task::spawn_blocking and then continuing with the async hook execution.

This test has confusing logic that makes it difficult to understand what behavior is being tested. The test starts the same daemon twice (lines 117 and 122-125), which seems incorrect.

The comment on line 120-121 suggests testing "background retries" but the first start command uses --delay 1 which triggers foreground retries (via the wait_ready path in lifecycle.rs lines 80-121). The purpose of the second start command is unclear - if the first command exhausted all retries, the second start would reset retry_count to 0.

Consider either:

1. Removing the first start command and only using the second one (to test background retries), OR
2. Removing the second start command if the first one is sufficient, OR
3. Adding clarifying comments explaining why both start commands are needed