fix(coordinator): make multi-daemon stop best-effort instead of aborting on first failure

[phil-opp]

🤖 Machine-generated PR. Opened by Claude (Anthropic) during an automated code-review pass of the repository. Please review carefully before merging.

Issue

stop_dataflow (binaries/coordinator/src/handlers.rs) iterates over dataflow.daemons and uses ? on each step — the connection lookup, send_and_receive, and the reply match. The first daemon that errors returns Err from the whole function; the CLI Stop / StopByName handlers just forward that error and do nothing else. Daemons later in iteration never receive StopDataflow.

Failure scenario

A 3-daemon dataflow on daemons A < B < C (dataflow.daemons is a sorted BTreeSet). Daemon B disconnects. User runs dora stop:

1. A gets StopDataflow and stops its nodes.
2. At B, daemon_connections.get_mut(B) returns None → bail! → function returns early.
3. Healthy daemon C never gets the stop — its nodes keep running as orphans, consuming resources, until C independently disconnects.

The CLI only sees the error about B.

Fix

Make the loop best-effort: attempt the stop on every daemon, collect per-daemon failures, and return an aggregated error only after all daemons have been attempted. This mirrors the established pattern already used by run::rollback_spawned_daemons, whose comment explicitly states "one daemon's failure must not abort rollback for the remaining daemons."

The success path is unchanged (all daemons succeed → same Ok); only the failure path changes from short-circuit to best-effort with an aggregated error listing each failing daemon.

Known limitation — restart widens the blast radius (follow-up)

Best-effort stop interacts with the restart path (initiate_restart, binaries/coordinator/src/lib.rs). Step 2 of a restart stops the old dataflow and, on Err from stop_dataflow, replies with the error and returns without registering a PendingRestart. With best-effort stop, a dora restart across daemons A, B, C where B is unreachable now actively stops A and C and then abandons the restart — leaving the dataflow half-dead, where the old first-failure abort touched fewer daemons before giving up. Same failure class, wider reach.

This is a pre-existing restart-under-partial-failure semantics question (should a partial stop still proceed to a PendingRestart, or roll forward?), not something this PR should decide unilaterally — flagging it here for a maintainer call / follow-up rather than changing restart behavior in a stop-teardown fix. Thanks to @phil-opp for catching this.

Validation

• New regression test stop_dataflow_is_best_effort_when_a_daemon_fails (handlers.rs): two daemons where the one iterated first is unreachable; asserts the healthy daemon still receives a StopDataflow and that the error aggregates the per-daemon outcome. RED under the old ? code (the healthy daemon is never reached).
• cargo test -p dora-coordinator — all pass (incl. the new test).
• cargo clippy -p dora-coordinator --all-targets -- -D warnings — clean.
• cargo fmt --all -- --check — clean.

[trunk-io]

😎 Merged successfully - details.

[phil-opp]

Automated review by Claude — fully automated, no human in the loop.

The best-effort rework looks correct: each daemon's stop runs in its own async { … }.await block, per-daemon failures are collected into errors, and the aggregated error is returned only after every daemon has been attempted — so a single unreachable daemon no longer orphans nodes on the remaining healthy daemons. The mutable daemon_connections.get_mut borrow is scoped to each iteration, and the success path (Ok(dataflow)) is unchanged. No correctness issue found.

One gap worth addressing: there is no regression test. This changes coordinator stop semantics from short-circuit to best-effort, and the central guarantee — that a daemon appearing after a failing one in dataflow.daemons still receives StopDataflow — is exactly what a test should pin. The existing suite doesn't exercise this path (the PR's cited test counts are pre-existing tests). Consider an integration test in binaries/coordinator/tests/ with a failing/disconnected middle daemon, asserting the later daemon is still stopped and the aggregated error names the failing one.

---

Generated by Claude Code

[phil-opp]

Best-effort is the right call here, and the caller does still learn which daemons failed: handlers.rs builds a per-daemon "{id}: {err:#}" and all three callers propagate it. A half-stopped dataflow is also not wedged — the CLI gets an immediate Err, and an unreachable daemon is pruned by cleanup_disconnected_daemons_from_running_dataflows once the 30s inbound-heartbeat threshold fires.

Two things:

No test. This is a behavior change in distributed teardown with no RED test at any tier. Nothing pins the actual property — that daemon B's failure still lets daemon A stop.

The restart path widens the blast radius. binaries/coordinator/src/lib.rs:4795 replies and returns on Err without inserting a PendingRestart. With best-effort stop, a dora restart across three daemons where B is unreachable now actively stops A and C and then abandons the restart, leaving the dataflow half-dead — where the old first-failure abort touched fewer daemons. Same failure class, wider reach. Worth a line in the body or a follow-up.

[phil-opp]

Thanks — both points are fair.

Test. Added stop_dataflow_is_best_effort_when_a_daemon_fails (handlers.rs, pushed in d2651a8). It builds a two-daemon dataflow where the daemon iterated first is unreachable — broken sorts before healthy in the BTreeSet<DaemonId> (ordered by machine id) — and asserts the healthy daemon still receives a StopDataflow, plus that the error aggregates as 1 of 2 daemon(s) and names the failed one. It's RED under the old first-failure ? code: the healthy daemon is never reached, so its recorded-request log is empty. The daemon connection is mocked at the mpsc/pending_replies seam (a healthy responder that completes the reply oneshot; a broken one whose receiver is dropped so the first send fails), the same seam orphan_stop_sends_stopdataflow_to_reporting_daemon uses.

Restart blast radius. Confirmed — initiate_restart (lib.rs) stops the old dataflow and, on Err, returns without registering a PendingRestart, so a partial best-effort stop now leaves A/C stopped and the restart abandoned. That's a restart-under-partial-failure semantics decision (proceed to a PendingRestart for the survivors, or roll forward?) rather than something to settle inside a stop-teardown fix, so I've documented it as a known limitation in the PR body and left the restart path unchanged for a maintainer call / follow-up. Happy to open a follow-up issue or take a direction if you have a preference.

---

Generated by Claude Code

[github-actions]

@phil-opp the Trunk merge queue failed for this PR.

See the Trunk merge-status comment for details.

Posted as a new comment so GitHub sends an email — Trunk's sticky comment is edited in place and won't trigger a notification.

[phil-opp]

Pushed a fix — this is ready to re-queue.

The merge-queue run failed to compile with missing field 'ready_barrier_released' in initializer of RunningDataflow (handlers.rs). Root cause: main added the ready_barrier_released field to RunningDataflow after this branch was created, and the dataflow_on test builder I added didn't set it. The branch built in isolation but not when merged with main — exactly what the queue caught (and it took the batched #2897/#2898 down with it, since the queue compiles the batch together).

Fix: rebased onto the latest main and set ready_barrier_released: false in the test builder, matching every other RunningDataflow initializer. Verified locally on the new base with Rust 1.95.0: cargo test -p dora-coordinator passes (incl. stop_dataflow_is_best_effort_when_a_daemon_fails), fmt clean.

Sorry for the batch disruption to #2897/#2898 — both are fine on their own and should pass once re-tested without the stale build of this PR in the batch.

---

Generated by Claude Code

[phil-opp]

🤖 Fully automated review by Claude — this review was generated end-to-end by an automated agent with no human vetting. Treat it accordingly.

Re-reviewed after the new commit. It adds the regression test the earlier pass asked for: stop_dataflow_is_best_effort_when_a_daemon_fails. Because broken sorts before healthy in the BTreeSet<DaemonId>, it genuinely exercises the "failing daemon iterated first" case that the old first-failure ? regressed, and it asserts both that the healthy daemon still receives StopDataflow and that the aggregated error names the failed daemon. The ready_barrier_released field addition also fixes the merge-queue compile break.

The best-effort loop itself reads correctly (per-daemon work scoped in its own future, failures aggregated and surfaced only after every daemon is attempted). Leaving restart-under-partial-failure (initiate_restart returning on Err without registering a PendingRestart) as a documented follow-up rather than deciding those semantics inside a stop-teardown fix seems like the right scoping. No new issues found.

---

Generated by Claude Code