Skip to content

fix(daemon): retry finished dataflow reports after reconnect - #3118

Draft
SunSunSun689 wants to merge 1 commit into
dora-rs:mainfrom
SunSunSun689:fix/finish-dataflow-local-cleanup
Draft

fix(daemon): retry finished dataflow reports after reconnect#3118
SunSunSun689 wants to merge 1 commit into
dora-rs:mainfrom
SunSunSun689:fix/finish-dataflow-local-cleanup

Conversation

@SunSunSun689

Copy link
Copy Markdown

Summary

Fixes a daemon cleanup ordering bug where finish_dataflow could leave a completed dataflow in the daemon’s local running map if reporting AllNodesFinished to the coordinator failed during a disconnect.
The fix makes local teardown independent from coordinator delivery:

  • signal listener shutdown and remove the dataflow from self.running before
    reporting completion

  • keep the existing error return when coordinator reporting fails, so
    reconnect behavior is preserved

  • store failed finish reports in pending_finished_dataflows

  • retry pending AllNodesFinished reports on the next coordinator connection
    before sending StatusReport

This prevents the daemon from reporting an already-finished dataflow as still running after reconnect, while still giving the coordinator another chance to receive the terminal result.

Notes

This fixes the direct failure path where the coordinator send channel is already closed and send_event() returns an error. It does not change the broader fire-and-forget semantics of CoordinatorSender; if a message is accepted into the local send channel but the background websocket writer later fails, there is still no coordinator-level ack.

@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

@SunSunSun689
SunSunSun689 marked this pull request as draft August 11, 2026 06:31

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 issues found. The reordering so that local cleanup (removing the dataflow from running + signalling listener shutdown) happens before the coordinator report correctly decouples local cleanup from the send result. The reconnect retry path only re-sends entries that were actually stashed on a send failure and removes each entry only on success, so there's no duplicate-report or lost-entry risk. finish_dataflow_cleans_local_state_when_coordinator_send_fails is a real regression test: it drops the receiver to force a genuine send failure, then asserts error propagation, running cleared, the pending entry retained, listener shutdown fired, and a fresh sender draining the retried AllNodesFinished event.

One minor, non-blocking note: on reconnect, if a stashed report fails to send again, the ? aborts the rest of the reconnect status report. That's a benign transient edge case, not a defect.


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