daemon: `dora node replace` racing a restart of the same node misattributes the restart-zombie's events (higher generation) to the replacement — silent hang / misrouted outputs

[phil-opp]

⚠️ This issue was created by a scheduled, automated Claude code-review check. It was not filed by a human. Please verify the finding before acting on it. This is a race, so it is intermittent, but the analysis below is concrete and the consequences are severe.

Summary

The node-lifecycle generation guard introduced with dora node replace (#2988, commit 79afcc8) drops strictly-older node events only:

// binaries/daemon/src/lib.rs:347
fn event_generation_is_stale(entry_generation: u64, event_generation: u64) -> bool {
    event_generation < entry_generation
}

Its justification (doc comment at 339-346 and the call-site comment at 1685-1695) is that "a newer-than-entry generation is always the entry's own racing restart successor," so a newer generation must be let through or the successor's one-shot Subscribe is lost. That invariant holds for the restart mechanism — but dora node replace breaks it, because a replacement is a brand-new incarnation with its own generation that can be lower than a concurrent restart of the outgoing incarnation. When that happens, the outgoing incarnation's restart-zombie carries a higher generation than the replacement, so its Subscribe/SendOut/OutputsDone events pass the gate and are applied to the replacement.

Why the inversion is possible

Both the replacement spawn and the restart loop draw from the same global monotonic counter, and the replacement's generation is minted early (before its slow build), while restart is disabled late:

1. spawn_node mints the replacement's generation from the global counter before the build:
   binaries/daemon/src/spawn/spawner.rs:465 → let generation = crate::running_dataflow::next_node_generation();
2. The restart loop mints its successor's generation from the same global counter:
   binaries/daemon/src/spawn/prepared.rs:525 → let new_generation = crate::running_dataflow::next_node_generation();
3. In the replace handler the outgoing incarnation's restart is only disabled after the replacement is fully spawned:
   binaries/daemon/src/lib.rs:2956 → outgoing.disable_restart(); (spawn is at 2937-2941; commit/entry-removal at 2952-2953).
4. The daemon event loop is blocked on the inline build+spawn the whole time — handle_coordinator_event is awaited inline at lib.rs:1656, and the ReplaceNode arm awaits task.await (build, 2937) and prepared.spawn().await (2941) — so the outgoing node's exit, its restart's ProcessHandleReplaced, and the zombie's connection events all queue up behind the commit.

Concrete sequence

Node X has restart_policy: always (or on-failure), current entry generation = 9.

1. dora node replace X … arrives. Handler calls spawn_node, minting G_replace = 10 (global → 11), then blocks the event loop on the build + process spawn.
2. During that window X's process exits (a clean exit for an always node, or a crash). X's restart loop is a separate task, still enabled (disable_restart not yet reached). It mints new_generation = 11 (> 10), stores it into X's listener's shared generation_counter, and spawns a zombie process on the still-open old listener.
3. The zombie connects and its Subscribe{11} / SendOut{11} / OutputsDone{11} events are stamped generation 11 and queued (event loop still blocked).
4. Build finishes; replace commits: old entry (gen 9) removed, replacement inserted at gen 10, handler returns.
5. Event loop drains the queue:
   • Zombie's ProcessHandleReplaced{prev=9,new=11} → entry gen is 10 ≠ 9 → RejectedStale → drop-kills the zombie. Good.
   • But the zombie's already-queued Event::Node{generation=11} hit the gate: event_generation_is_stale(10, 11) = (11 < 10) = false → not stale → applied to the replacement's entry.

Impact

• Subscribe{11} reinstalls subscribe_channels[X] pointing at the (dying) zombie. Landing after the replacement's own Subscribe{10}, the replacement receives no inputs and silently hangs.
• OutputsDone{11}/CloseOutputs{11} runs handle_outputs_done for X, propagating output-closed downstream — consumers stop reading the replacement's outputs.
• SendOut{11} injects spurious/duplicate output under X's id.

The exit path is already safe because it uses exact matches_generation (running_dataflow.rs:125), so the zombie's terminal SpawnedNodeResult{11} is correctly dropped. That asymmetry is the tell: terminal events are exact-match (sound against a higher-generation orphan), but control/data Event::Nodes are strictly-older (unsound against one).

Suggested fixes (either closes it)

• Targeted: disable the outgoing incarnation's restart loop at the top of the replace handler, before spawn_node mints the replacement generation and before the build — re-enabling it only if a pre-commit step fails (to preserve the "outgoing left running on failure" guarantee).
• Robust: stop trusting "any newer generation is my successor." Track the specific successor generation the entry's own restart loop announced (e.g. a pending_successor_generation on RunningNode, set when the loop advances the counter and cleared when ProcessHandleReplaced lands), and accept an Event::Node only when its generation equals the entry generation or that announced successor — dropping every other newer generation.

Note the inline build+spawn on the event loop (lib.rs:2937-2941) both blocks the whole daemon for the build duration and widens this race window.

[phil-opp]

Triaging this as part of an automated pass over open issues. Confirmed still present on current main — the three code facts the analysis rests on all hold:

• The generation guard is strictly-older, not equality: event_generation_is_stale(entry, event) = event < entry (binaries/daemon/src/lib.rs:379-381), and the Event::Node gate uses it at lib.rs:1837. A newer-than-entry generation is deliberately let through.
• The replacement generation is minted early — let generation = crate::running_dataflow::next_node_generation(); at binaries/daemon/src/spawn/spawner.rs:482, before the build — while the restart loop mints its successor from the same global counter at binaries/daemon/src/spawn/prepared.rs:525.
• The outgoing incarnation's restart is disabled late: outgoing.disable_restart(); at lib.rs:3220, which runs after spawn_node (:3188), the build task.await (:3201) and prepared.spawn().await (:3202-3205). The daemon event loop is blocked on that inline build+spawn the whole time.

So a restart-zombie of the outgoing incarnation can carry a generation higher than the replacement's, and its Subscribe/SendOut/OutputsDone pass the < gate and land on the replacement's entry — matching the concrete sequence in the report. (The exit path stays safe: SpawnedNodeResult uses exact matches_generation, so only the control/data Event::Nodes are exposed.)

I'm not opening a fix PR yet — the two suggested fixes each carry a trade-off that a maintainer should choose between, and this is a silent-data-loss path where a wrong fix is worse than the low-probability race:

1. Targeted (disable the outgoing restart at the top of the replace handler). disable_restart() is one-way today (lib.rs:5288 documents it as having "no re-enable"). Disabling it before the build means that for the entire (slow) build+spawn window the outgoing has no restart policy; if the outgoing then crashes during the build and the replace subsequently fails before commit, the outgoing is gone and never restarts — regressing the feat(cli): atomic dora node replace for live node swaps #2927 "any failure up to and including the spawn leaves the current incarnation running" guarantee. Re-enabling on the failure path doesn't fully recover it, because the restart loop may already have observed the disable and cancelled at its re-check. So this needs a genuine re-enable/rollback design, not a one-line move.

2. Robust (track the entry's own announced successor generation). This is the sound fix, but it can't be purely local: the guard would accept an Event::Node{gen} only when gen == entry.generation or gen == entry.pending_successor_generation. The successor generation is minted inside the detached restart-loop task (prepared.rs:525) and the daemon only learns of it today via the zombie's own connection events / ProcessHandleReplaced. Distinguishing "my successor" from "a foreign incarnation's zombie" therefore requires a new announce channel from the restart loop to the main loop, set before the successor's Subscribe can arrive — and getting that ordering wrong reintroduces exactly the lost-Subscribe failure the strictly-older guard was added to prevent (feat(cli): atomic dora node replace for live node swaps #2988 review, finding 1). I don't want to land that unverified against a race I can't reproduce here.

Flagging for maintainer triage on which direction to take. Once the approach is chosen I'm happy to implement it (including the successor-generation feedback channel for option 2). Note also the inline build+spawn on the event loop (lib.rs:3188-3205) both blocks the whole daemon for the build and widens this window, so it's worth considering independently.

---

Generated by Claude Code