Skip to content

fix(coordinator): persist ready-barrier release even when the status read fails - #3117

Draft
phil-opp wants to merge 1 commit into
mainfrom
claude/clever-wright-jqbttc-3115-barrier-persist
Draft

fix(coordinator): persist ready-barrier release even when the status read fails#3117
phil-opp wants to merge 1 commit into
mainfrom
claude/clever-wright-jqbttc-3115-barrier-persist

Conversation

@phil-opp

Copy link
Copy Markdown
Collaborator

Summary

Fixes #3115.

broadcast_all_nodes_ready flips the in-memory ready_barrier_released flag unconditionally (via release_barrier_message) but, on the failed-barrier path, only persisted the durable record when it could resolve a status. A store.get_dataflow error (or Ok(None)) skipped the entire write, leaving the flag true in memory but stale on disk.

If the in-memory RunningDataflow is then destroyed (coordinator restart or orphan reclaim) before a later status-persist re-captures the flag, a daemon that was disconnected when the broadcast fired reconnects, reads ready_barrier_released == false from the stale record, is never replayed the release, and parks its nodes for the life of the dataflow — exactly the #2998 reconnect-hang window that #3013 set out to close, on the store-I/O-trouble path where durability matters most.

Fix

Decouple persisting the release + verdict from resolving the status, as the issue suggested. Extract persist_ready_barrier_release, which:

  • always writes the record (the ready_barrier_released flag + exited_before_subscribe verdict come from make_record), and
  • makes only the status conditional. A successful barrier promotes to Running; a failed barrier preserves the record's current status when it can be read, and otherwise falls back to Pending — a non-promoting status the reconcile path can still advance — so it never promotes to Running.

The key property restored: a failed-barrier release leaves record.ready_barrier_released == true on disk even when the status read fails.

Tests

New regression tests in binaries/coordinator/src/lib.rs:

  • failed_barrier_release_persists_when_status_read_fails — a FailingReadStore whose get_dataflow always errors; asserts the release is still persisted (fails on the old code, which skipped the write).
  • failed_barrier_release_persists_when_record_absent — the Ok(None) companion path.
  • successful_barrier_release_persists_running — confirms a successful barrier still promotes to Running.

Full dora-coordinator suite passes (125 lib + 22 + 4); cargo fmt --check and cargo clippy -- -D warnings clean.


Generated by Claude Code

…read fails

`broadcast_all_nodes_ready` flips the in-memory `ready_barrier_released`
flag unconditionally (via `release_barrier_message`) but, on the
failed-barrier path, only persisted the durable record when it could
resolve a status. A `store.get_dataflow` error (or `Ok(None)`) skipped
the entire write, leaving the flag `true` in memory but stale on disk.

If the in-memory `RunningDataflow` is then destroyed (coordinator
restart or orphan reclaim) before a later status-persist re-captures the
flag, a daemon that was disconnected when the broadcast fired reconnects,
reads `ready_barrier_released == false` from the stale record, is never
replayed the release, and parks its nodes for the life of the dataflow --
exactly the #2998 reconnect-hang window that #3013 set out to close, on
the store-I/O-trouble path where durability matters most.

Decouple persisting the release + verdict from resolving the status:
extract `persist_ready_barrier_release`, which always writes the record
and only makes the *status* conditional. A failed barrier preserves the
record's current status when it can be read and otherwise falls back to
`Pending` (a non-promoting status the reconcile path can still advance),
so it never promotes to `Running`.

Adds regression tests covering the status-read-error and no-prior-record
failed-barrier paths (release still persisted) and the successful-barrier
path (still promoted to `Running`).

Closes #3115

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PCXs8nkFNV7e4AhQLRVSSH
@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 Author

🤖 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. Decoupling the (now unconditional) durable record write from the (conditional) status resolution correctly closes the reconnect-hang window on the store-read-error path — previously a read error or Ok(None) skipped the whole write, leaving the in-memory ready_barrier_released flag true but the durable record stale. The Pending fallback on a read error is safe, since a failed-barrier dataflow (non-empty exited_before_subscribe) never reached Running, so there is no more-advanced status to regress. FailingReadStore forces a get_dataflow error while still delegating writes and proves the release is persisted with ready_barrier_released == true and never promoted to Running; the Ok(None) and success paths are covered too.


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.

coordinator: failed ready-barrier release is not persisted when the store status read fails, re-opening the #2998 reconnect-hang window

2 participants