daemon: restart_loop that breaks after announcing `SpawnedNodeResult{restart:true}` leaks the node in `running_nodes` → dataflow never finishes (stop-vs-restart race and respawn-failure both hit it)

[phil-opp]

This issue was created by a scheduled automated Claude check (an unattended code-review routine). The finding was verified by reading the full event chain across prepared.rs, lib.rs, and running_dataflow.rs, but has not been reproduced with a running dataflow — treat the trigger scenarios as read-from-code, and please sanity-check before acting.

Summary

RawSpawner::restart_loop emits DoraEvent::SpawnedNodeResult { restart, .. } to the daemon before the backoff sleep and the final disable_restart re-check. On the restart == true branch the daemon treats that event as "a respawn is coming" — it only notifies downstream (NodeRestarted) and deliberately does not remove the node from running_nodes or record a terminal result. It then relies on the restart_loop to follow up with either a ProcessHandleReplaced (successful respawn) or a later SpawnedNodeResult { restart:false } (gave up).

But there are break paths that exit restart_loop after SpawnedNodeResult{restart:true} has already been sent, and none of them send a corrective terminal event. The node is then stuck in running_nodes forever, and because it is non-dynamic, the dataflow can never satisfy the finish condition — dora stop hangs until an external timeout.

The event restart:true is sent, then the loop can break

binaries/daemon/src/spawn/prepared.rs, restart_loop:

// line 351-365: emitted unconditionally, restart may be `true`
let event = DoraEvent::SpawnedNodeResult {
    dataflow_id: self.dataflow_id,
    node_id: self.node.id.clone(),
    exit_status,
    dynamic_node: self.node.kind.dynamic(),
    restart,          // <-- true
    restart_count,
    pid: exited_pid,
}.into();
// ...
let _ = self.daemon_tx.clone().send(event).await;   // daemon now expects a follow-up

if restart {
    if let Some(base_delay) = config.restart_delay {
        // ... line 385:
        tokio::time::sleep(backoff).await;           // .await point — stop can race in here
    }

    // line 399-408: final re-check
    if disable_restart.load(atomic::Ordering::Acquire) {
        logger.log(/* "restart cancelled: inputs closed before respawn" */).await;
        break;                                       // (A) leaves WITHOUT any corrective event
    }

    restart_count += 1;
    // ...
    let result = self.clone().spawn_inner(&mut logger, op_rx_new, finished_tx).await;
    match result {
        Ok(NodeKind::Spawned { pid }) => { /* sends ProcessHandleReplaced */ }
        Ok(NodeKind::Dynamic) => { /* log */ break; }
        Err(err) => {
            logger.log(/* "failed to restart node: {err}" */).await;
            break;                                    // (B) also leaves WITHOUT any corrective event
        }
    }
}

The task is tokio::spawn'd and its JoinHandle is dropped (prepared.rs:206), so when the task ends via break nothing observes it. There is no post-loop code that emits a terminal SpawnedNodeResult.

Why the leak is permanent

Daemon handling in binaries/daemon/src/lib.rs (DoraEvent::SpawnedNodeResult, from :4839):

• On restart == true (:5057) it only fans out NodeEvent::NodeRestarted to downstream subscribers. It does not call handle_node_stop, so the node stays in running_nodes.
• Only the restart == false branch (:5146, ending at :5162) calls handle_node_stop(...), which is what removes the node and can trigger finish_dataflow.

handle_node_stop_inner (:4535) gates finishing on every remaining running node being dynamic:

let should_finish = !dataflow.pending_nodes.local_nodes_pending()
    && dataflow.running_nodes.iter().all(|(_id, n)| n.node_config.dynamic);

A leaked non-dynamic node keeps this false for every other node's stop, so finish_dataflow is never called.

stop_all (binaries/daemon/src/running_dataflow.rs:430) sets disable_restart on all nodes (:448-450) and stop_sent = true (:509), then should_finish_immediately (:514) returns WaitForNodes because the leaked node is non-dynamic. And the finish-straggler watchdog cannot rescue it — finish_stragglers (:745) early-returns an empty list whenever stop_sent is true:

pub(crate) fn finish_stragglers(&self, grace: Duration, now_millis: u64) -> Vec<NodeId> {
    if self.stop_sent || !self.open_external_mappings.is_empty() {
        return Vec::new();
    }
    // ...
}

So after a dora stop, nothing removes the leaked node and the dataflow hangs. The only terminal signal to the CLI (DataflowDaemonResult) is sent from finish_dataflow, which never runs.

Concrete triggers

(A) Stop-vs-restart race — needs restart_delay to widen the window:

1. A node with restart_policy: Always (or OnFailure) and restart_delay set crashes.
2. restart_loop sends SpawnedNodeResult{restart:true}, downstream gets NodeRestarted, and it enters the backoff sleep (prepared.rs:385).
3. During that sleep the operator runs dora stop → stop_all sets disable_restart on every node.
4. The loop wakes, the re-check at :399 sees disable_restart == true, logs "restart cancelled: inputs closed before respawn", and breaks at :407.
5. The node is never removed; dora stop blocks on DataflowDaemonResult until an external timeout.

(B) Respawn failure — does not even need the stop race:

1. A node with restart_policy: Always/OnFailure crashes.
2. restart_loop sends SpawnedNodeResult{restart:true}, passes the disable_restart re-check, and calls spawn_inner.
3. spawn_inner returns Err(..) (e.g. the executable was removed/replaced, fork/exec fails under resource pressure, working dir gone), so the loop logs "failed to restart node" and breaks at :498.
4. The daemon was told restart:true and never hears back; the node lingers in running_nodes and the dataflow can't finish.

Suggested direction

When restart_loop abandons a restart it has already announced as restart:true, it must deliver a corrective terminal signal so the daemon removes the node and can finish the dataflow — e.g. re-send SpawnedNodeResult { restart:false, .. } (with the same exit_status/pid) on the break paths at prepared.rs:407 and :498, or route those cases through the normal stop path. Note the interaction with the pid-staleness guard added in #2893 (lib.rs:4875): the corrective event should carry the original exited_pid so it isn't discarded as a stale exit.

Notes

• Distinct from daemon: two node-lifecycle paths still keyed by node id alone, so a dead incarnation can act on a live one #2926 (incarnation identity on ProcessHandleReplaced / dataflow_node_results during remove→add races) — that is about a dead incarnation acting on a live one; this is about a restart:true announcement with no terminal follow-up.
• Not covered by existing tests: tests/node-lifecycle-e2e.rs pins the remove→re-add orderings, not a stop (or respawn failure) racing an in-progress restart-with-backoff.

[phil-opp]

This looks already resolved on main.

RawSpawner::restart_loop in binaries/daemon/src/spawn/prepared.rs now emits a corrective terminal SpawnedNodeResult { restart: false, .. } on every break path that had already announced restart: true, via a shared send_terminal_exit helper (prepared.rs:279) that carries the original exited_pid (so it isn't discarded by the #2893 pid-staleness guard) and restart: false (so the daemon runs handle_node_stop and can finish the dataflow):

• (A) stop-vs-restart cancel — the disable_restart re-check after the backoff sleep now calls send_terminal_exit(...) before break (prepared.rs:462-467).
• (B) respawn failure — the Err(err) arm after spawn_inner calls send_terminal_exit(...) before break (prepared.rs:570-582).
• The Ok(NodeKind::Dynamic) arm settles the same debt (prepared.rs:555-568).

The running_nodes leak / never-finishing-dataflow described here no longer reproduces on those paths, so no further code change looks necessary — suggest closing.

---

Generated by Claude Code