fix(coordinator): reject Stop/StopByName on a dataflow with a pending…

[GuTS805]

initiate_restart() sends StopDataflow and registers a PendingRestart under
the dataflow's UUID, but leaves the entry in running_dataflows until
DataflowFinishedOnDaemon fires. Stop/StopByName only checked
running_dataflows, not pending_restarts, so a concurrent Stop would fall
through to stop_dataflow(), succeed, and get queued — then silently lose
when the pending restart resolved first and spawned a new incarnation
under a fresh UUID. The caller saw a clean 'stop succeeded' while the
dataflow kept running under a different UUID.

Reproduced live 3/3 with dora restart + dora stop fired concurrently on
a real coordinator+daemon+node setup. Now rejects the stop with a clear
error instead of silently losing.

Why not a duplicate

• 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) #2936 (open) — daemon-level node restart-loop leak in a single daemon's
  running_nodes, not this coordinator-level Restart control-request path
• daemon: #2970 can call dataflow.start() on an already-stopping dataflow (barrier-completed-by-node-death start isn't gated on stop_sent) #3053 (open) — dataflow.start() firing on an already-stopping dataflow via
  the startup barrier, a different trigger mechanism
• coordinator: stale PendingRestart leaks reply_sender and hangs caller if a daemon dies mid-restart #2131 (closed) — stale PendingRestart hangs the caller when a daemon dies
  mid-restart; already fixed by draining pending_restarts on disconnect.
  Doesn't cover a concurrent Stop racing a healthy in-progress Restart.
• No existing e2e test (daemon-reconnect-e2e.rs, fault-tolerance-e2e.rs,
  node-lifecycle-e2e.rs, ws_control_tests.rs) exercises Restart racing Stop
  on the same UUID.
• Unrelated to PR fix(cli): normalize workspace path to forward slashes for CMake templates #3040 (CMake path fix) and PR fix(coordinator): box CachedResult::Cached to fix large_enum_variant on Windows #3001 (CachedResult boxing) —
  this branch is based cleanly on origin/main and touches neither file.

Verification

• cargo check -p dora-coordinator — clean
• cargo clippy -p dora-coordinator -- -D warnings — clean (aside from the
  pre-existing unrelated large_enum_variant warning tracked by fix(coordinator): box CachedResult::Cached to fix large_enum_variant on Windows #3001)
• cargo fmt --all -- --check — clean
• cargo test -p dora-coordinator --lib — 107 passed, 0 failed
• Live repro: dora restart & dora stop & fired concurrently,
  3/3 runs — 2/3 hit the new guard directly (clear rejection error), 1/3
  the restart had already resolved before stop arrived so it correctly
  reported the now-genuinely-finished old UUID (no silent loss in any run)

[trunk-io]

😎 Merged successfully - details.

[phil-opp]

🤖 Automated review by Claude — this is a fully automated review with no human in the loop. Treat it as advisory.

Reviewed the diff — no correctness issues found. The pending-restart guard is correctly placed in both the Stop and StopByName branches after UUID resolution, and it rejects only while a restart is genuinely in-flight (pending_restarts.contains_key(&uuid)), closing the silent stop-vs-restart race where a Stop would "succeed" against the old UUID while the restart spawned a new incarnation under a fresh UUID.

One observation: this change ships without an automated test. The race is subtle and was verified only by manual repro, and the repo's TDD policy asks for a regression test at the tier that reproduces the bug. An integration test in binaries/coordinator/tests/ that stages a pending restart and asserts Stop/StopByName are rejected would lock in the behavior and guard against regressions.

---

Generated by Claude Code

[GuTS805]

Added an e2e regression test addressing this: tests/stop-restart-race-e2e.rs.

It reuses the shared stop-delay-node fixture's DORA_TEST_STOP_DELAY_MS
knob to hold the node's shutdown open for 5s after Stop, which keeps the
restart's DataflowFinishedOnDaemon from arriving that long — so the
coordinator's pending_restarts entry is guaranteed still present when the
concurrently-issued stop reaches it. That makes the test deterministic
rather than timing-dependent (unlike the original bug, which only
reproduced 2 or 3 times out of 3 with zero artificial delay).

The test starts a real coordinator + daemon, fires dora restart then
dora stop on the same UUID, and asserts:

• stop fails with the pending-restart rejection error (not a silent
  success)
• restart still completes normally under a fresh UUID

Verified cargo check and cargo fmt --all -- --check clean on the new
file. cargo clippy on it is currently blocked by the same pre-existing,
unrelated large_enum_variant issue in dora-coordinator tracked by #3001
— not something this file introduces.

[phil-opp]

🤖 Automated review by Claude — fully automated, no human in the loop; please verify before acting.

Re-reviewed the latest commit (f162d951), which adds tests/stop-restart-race-e2e.rs. This resolves the earlier note about the fix shipping without a regression test. The test is genuine: it stages a real coordinator+daemon, uses the stop-delay-node DORA_TEST_STOP_DELAY_MS knob to hold the node's shutdown open so pending_restarts reliably still holds the UUID when stop lands, and asserts stop fails with the "is being restarted" rejection (it would hit the dora stop unexpectedly succeeded panic if the guard were reverted), while confirming the restart still comes back under a fresh UUID. No new issues found in the coordinator guard itself.

---

Generated by Claude Code

[phil-opp]

Thanks for chasing this down — the race is real and worth fixing. initiate_restart inserts the PendingRestart but leaves the old UUID in running_dataflows, so a concurrent Stop gets parked in stop_reply_senders and is later answered DataflowStopped success from the finished-dataflow path, which then spawns a new incarnation. The caller really does see success while the dataflow lives on.

My concern is with the chosen remedy rather than the diagnosis.

1. The guard also blocks --force, leaving no way out

binaries/coordinator/src/lib.rs:906 (Stop) and :968 (StopByName):

if pending_restarts.contains_key(&dataflow_uuid) {
    let _ = reply_sender.send(Err(eyre!(
        "dataflow `{dataflow_uuid}` is being restarted – cannot stop it until the restart finishes"
    )));
    continue;
}

force is destructured at :894 and never consulted, so dora stop --force is refused and kills nothing — even though binaries/cli/src/command/stop.rs:41 documents it as "Force stop the dataflow by immediately terminating all its processes".

Concretely: dora restart --grace-duration 5m <uuid>, a node that ignores Event::Stop, and the daemon won't SIGKILL for ~7.5 minutes. For that entire window there is no way to stop the dataflow — Restart is already self-guarded, so every control verb is refused and the only escape is Destroy, i.e. tearing the coordinator down. The guard is also the first statement in the arm, ahead of the already-stopped fast path, and dora stop --all now aborts the whole batch if any single dataflow is mid-restart.

Suggestion: a Stop landing on a pending restart should cancel the restart rather than refuse — remove the PendingRestart and reply Err to the parked restart caller. That pattern already exists in this file, in the daemon-disconnect path at lib.rs:3195-3208. Last-writer-wins, and the dataflow actually ends up stopped. If you'd rather keep the rejection, force at minimum needs to bypass it.

2. The new test never runs in CI

The e2e itself is good — real coordinator, daemon and CLI, and it would genuinely fail against the first commit. But it lands in the root dora-examples package, which is excluded from cargo test --all at .github/workflows/ci.yml:233, and that job is if: github.event_name != 'pull_request' in any case. It isn't added to any of the explicit -p dora-examples --test … steps, and clippy --all / check --all don't build [[test]] targets — so even a compile error here would go green. Adding a step that names --test stop-restart-race-e2e would fix that.

Two smaller things: the fixed std::thread::sleep(300ms) before issuing the stop will flake on a loaded runner (deriving the window from an observable — poll until the restart is registered — would make it deterministic); and the test currently asserts stop_stderr.contains("is being restarted") and panics if the stop succeeds, so it pins the semantics under discussion in §1. Worth settling that first so you don't write the test twice.

3. Spawn-timeout path leaves a permanently stale entry

check_spawn_timeouts (lib.rs:3425) takes no pending_restarts and drops the dataflow at :3505 without draining it; draining only happens in DataflowFinishedOnDaemon's Entry::Occupied arm and in the disconnect path. initiate_restart only requires the UUID to be in running_dataflows, which it is while the spawn is pending, so the stale entry is reachable.

That leak was previously inert. With this guard placed ahead of the fast path it becomes user-visible: Stop for that UUID returns "is being restarted" forever, and the parked restart caller is never answered either. Worth draining pending_restarts there as part of this change.

---

Happy to look again once §1 is settled — that decision drives both the code and the test, so it's the one to pin down first.

< generated by claude code >

[GuTS805]

@phil-opp Addressed all three points:

1. Replaced the outright rejection with cancel-and-proceed: Stop/StopByName
   now cancels the pending restart via a shared cancel_pending_restart()
   helper (same pattern as the existing daemon-disconnect path) and lets the
   stop proceed normally — --force is no longer blocked.
2. Added the explicit CI step (Run coordinator stop/restart race E2E) and
   swapped the fixed sleep for polling a new "restart pending" coordinator
   log line.
3. check_spawn_timeouts now drains pending_restarts via the same helper
   when it removes a stale entry.

Test rewritten accordingly: asserts stop now succeeds, restart gets a
clear cancellation error, and no replacement incarnation is spawned.

[phil-opp]

Re-reviewed the revised diff (efd6ee5), which replaces the outright rejection with cancel-and-proceed. Traced the new flow against the surrounding code and it addresses all three earlier points cleanly:

• --force / --all no longer blocked. Stop/StopByName now call cancel_pending_restart and fall through to stop_dataflow with the caller's own grace_duration/force, so a force-stop during a long-grace restart actually terminates. Because the coordinator event loop processes events serially, the cancel + re-stop_dataflow is atomic with respect to DataflowFinishedOnDaemon, so no new incarnation can slip in between: the old UUID is still in running_dataflows (stop_dataflow keeps the entry until the finish event), and the second StopDataflow to the daemon is idempotent. When the finish event later arrives, pending_restarts no longer holds the UUID, so the parked stop reply is answered success and no replacement is spawned. stop --all no longer aborts the batch either.
• Spawn-timeout leak. check_spawn_timeouts now drains pending_restarts for the timed-out UUID via the shared helper, so a subsequent Stop on it is no longer permanently wedged and the parked restart caller is answered.
• CI. The e2e is now invoked explicitly via --test stop-restart-race-e2e, and it asserts the right post-conditions (stop succeeds, restart errors with the cancellation message, no replacement incarnation).

No correctness issues found in the guard itself.

One minor, non-blocking note: the check_spawn_timeouts comment says the stuck spawn is "the new incarnation initiate_restart kicked off," but a PendingRestart is keyed by the old UUID and the new incarnation is only spawned after DataflowFinishedOnDaemon — the actually-reachable case is a restart issued while the old dataflow was still spawning. The drain is correct either way; only the comment's framing is off.

_{Automated review; treat as advisory.}

---

Generated by Claude Code

[phil-opp]

🤖 Automated review by Claude (fully automated — may contain mistakes)

Re-checked after the latest commit (7233292c, "docs: fix imprecise comment on check_spawn_timeouts drain"). It is a comment-only change and directly addresses the minor framing note from my previous review: the check_spawn_timeouts comment now correctly explains that PendingRestart is keyed by the old UUID and the reachable case is a restart requested while the old dataflow was still spawning (not a new incarnation). No code behavior changed. No new issues; looks safe to merge.

---

Generated by Claude Code

[phil-opp]

Thanks! Needs a main merge to fix the Audit check, then this is ready to land.

[phil-opp]

/trunk merge