Skip to content

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

Open
GuTS805 wants to merge 1 commit into
dora-rs:mainfrom
GuTS805:fix/coordinator-stop-restart-race
Open

fix(coordinator): reject Stop/StopByName on a dataflow with a pending…#3114
GuTS805 wants to merge 1 commit into
dora-rs:mainfrom
GuTS805:fix/coordinator-stop-restart-race

Conversation

@GuTS805

@GuTS805 GuTS805 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

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

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)

… restart

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.
@trunk-io

trunk-io Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

Copy link
Copy Markdown
Collaborator

🤖 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants