fix(coordinator): answer a log subscription before replaying its backlog

[phil-opp]

Problem

ControlEvent::LogSubscribe (and BuildLogSubscribe) registered the new subscriber, then replayed the whole buffered backlog (up to MAX_BUFFERED_LOG_MESSAGES = 10 000) into it, and only then answered found_tx.

The other end of that channel is the WS task in ws_control.rs. It is parked on found_rx.await and does not drain log_rx (capacity 64) until found_tx fires. So during the replay:

• after 64 messages, every send_log_message hits its 100 ms timeout;
• the coordinator's single event loop stalls for about 100 × 100 ms = 10 s, holding up all daemon and CLI traffic;
• after 100 consecutive timeouts the subscriber is evicted. The backlog has already been taken, so the rest of it is lost, and found_tx still answers true.

How to hit it: dora start --detach a dataflow that logs a few hundred lines, then run dora logs <df> --follow. You get the first 64 lines and then nothing; meanwhile the whole coordinator was frozen for ~10 s.

Fix

Register the subscriber and answer found_tx before replaying. The WS task then sends the subscribe reply and returns to its select!, where it drains log_rx while the replay flows. Ordering is unchanged: anything queued before the reply is forwarded after it. Both arms now share a small attach_log_subscriber helper.

Validation

Class: C (binaries/coordinator/**)

• New regression test handlers::tests::attach_log_subscriber_delivers_backlog_larger_than_channel. A 200-message backlog goes into a 4-slot channel, with a task that, like ws_control, waits for found_rx before draining. It asserts all 200 arrive and the subscriber is kept.
  • RED on the old ordering: assertion failed: subscriber must not be evicted.
  • GREEN with the fix.
• cargo test -p dora-coordinator: ✅ 169 + 23 + 5 passed. cargo clippy -p dora-coordinator --all-targets -D warnings: ✅. cargo fmt --check: ✅.
• Workspace-wide checks (full cargo test --all, fault-tolerance E2E, contract tests, cargo check --examples) were run on the combined diff of this review batch. Results are in the batch summary comment.

Not addressed

The replay still runs on the coordinator event loop, one awaited send per message. If the same WS session sends another request mid-replay (that arm awaits its reply without draining log_rx), the stall can still happen. A per-subscriber forwarding task, or try_send plus re-buffering, would remove it entirely. That is a larger change, left out to keep this one minimal; the common dora logs --follow path sends nothing else.

---

🤖 Machine-generated. This PR was written by Claude (Claude Code) during an automated codebase review. The finding was checked by hand against current main, but please review it as you would any external contribution.

🤖 Generated with Claude Code

https://claude.ai/code/session_01R8CSx6yQ4JbrB3dqSiU2ME

---

Generated by Claude Code

[trunk-io]

😎 Merged successfully - details.

[phil-opp]

Automated review batch — 2026-10-01 (base b2df5ec)

This batch comes from an in-depth automated review of the codebase. Every finding was checked by hand against origin/main, and against the currently open PRs to avoid duplicates.

Opened PRs

Correctness

• fix(coordinator): answer a log subscription before replaying its backlog #3676 — coordinator: dora logs --follow on a busy detached dataflow stalled the event loop for about 10 s and lost the buffered backlog. The fix answers the subscription before replaying it.
• fix(coordinator): relaunch a restarted dataflow with its original launch context #3677 — coordinator: dora restart relaunched without the working dir, session, build id or write_events_to, so relative node paths and built nodes broke.
• fix(ros2-bridge): reject a null sequence, array or nested-message row #3678 — ros2-bridge: a null sequence, array or nested-message row was published as its masked values instead of being rejected.
• fix(cli): bracket an IPv6 host in the dora cluster upgrade scp target #3680 — cli: dora cluster upgrade built an invalid scp target for IPv6 hosts.

Correctness / security

• fix(cli,record-node): pass the recorded descriptor base64-encoded, past env expansion #3681 — cli + record-node: dora record ran $VAR expansion over the whole descriptor text, so it failed on unset variables and wrote secret values into the .drec header.
• fix(daemon): log a node's non-Register first request by kind, not payload #3679 — daemon: a non-Register first frame was Debug-formatted, so up to 64 MiB of payload ended up in a single log line.

Validation on the combined diff of all six PRs

• Per-PR: cargo fmt --check, cargo clippy -D warnings and tests for each affected crate all pass. Each regression test was shown to fail without its fix.

• cargo test --all (Python crates and dora-examples excluded): passes, except tests that bind zenoh to tcp/[::]:0. This sandbox has no IPv6, so those fail with "Address family not supported by protocol". With a DORA_ZENOH_CONFIG_OVERLAY that listens on IPv4, only the 16 tests that build their own zenoh config still fail, all with that same error. These are environmental, not caused by these changes.

• cargo check --examples: ✅

• fault-tolerance-e2e: ✅ 16/16, with the IPv4 overlay.

• Contract tests: the Rust ones pass. The 4 Python ones fail here because pyarrow and the dora Python module are not installed in the sandbox.

• make qa-fast: lockfile, fmt, clippy, unwrap-budget, secret-files, publish-graph, package-includes and ci-reporting pass. typos passes after installing it. qa-breaking --fast reports no breaking changes against v1.0.1. cargo-audit is not installed here; the only dependency change is a direct base64 0.22 edge, and that crate was already in Cargo.lock.

• /code-review and /simplify were run on the combined diff. Their findings were applied where they fit:

  • base64 instead of hex;
  • write_events_to added to the restart context;
  • start_dataflow now takes the LaunchContext;
  • the null check moved into the ros2 serializer's per-member loop;
  • shared user@ formatting for ssh_target/scp_target.

  Findings that were deferred are listed in each PR's "Not addressed" section.

Rejected findings (not opened)

• Topic-subscribe rollback stops at its first per-daemon failure (topic_debug.rs). It overlaps the in-flight topic-debug rework in fix(daemon,coordinator): keep topic debug frames off the control channel #3536 and feat(daemon,coordinator): negotiated binary topic debug frames, sent in chunks #3636.
• Artifact HTTP endpoint and ArtifactStore are dead code. Removing a route is a product decision for maintainers, not a fix.
• RedbStore::delete_dataflow scans every param row sorting after the prefix. The result is correct and the path is cold (dora clean). The wrong key-format doc comment is minor.
• dora logs on a log over ~300 KB exceeds the coordinator's 1 MiB WS frame limit and drops the daemon link. This is real, but the fix needs wire-format or truncation design work, so it should be its own issue.
• The daemon doesn't answer an undecodable node request, so the node hangs. An existing test (undecodable_frame_keeps_the_connection) documents keeping the connection open as intended behaviour, so changing it needs maintainer input.
• The extension table caps entries but not bytes. This is a resource-limit policy choice, and needs a decision on the byte budget.
• cpu_affinity pre_exec calls eprintln! and isn't validated. A deadlock needs a lock held at fork time, which is timing-dependent; validation is a separate feature.
• The operator runtime treats an input_timeout InputClosed as final and drops InputRecovered. This is a fault-tolerance semantics change across both runtime backends, too large for this batch.
• InputTracker and the runtime main loop clone ids on every message. These are small allocation wins with no measured impact.
• arrow-v58 feature not built in CI; DoraArray::type_name doc example. These are CI or doc tweaks with low value per PR.
• A RUST_LOG per-target directive can widen the stdout and OTLP filters (.or(parsed)). This is real, but it sits next to the open tracing: default zenoh to warn on the OTLP span layer #3658 on the same filter, so it is better folded in there.
• ros2-bridge-node exits on one bad input. Whether to warn and skip or fail fast is a design question for the bridge owners.
• mavlink2-bridge-node's UDP wake packet fails on an IPv6 bind. The only effect is a 500 ms shutdown grace on an uncommon setup.
• dora record ignores module outputs; log file names collide for node ids ending in .<digits>. Both are real, but both need design decisions: module expansion in record, and the log naming scheme.

---

🤖 Machine-generated by Claude (Claude Code) during an automated review run.

---

Generated by Claude Code

[phil-opp]

Audit (cargo-audit + cargo-deny) failed with error[yanked]: detected yanked crate for yoke-derive 0.8.3. That version was yanked from crates.io and is pinned in main's Cargo.lock, so the failure isn't caused by this PR. The fix is #3682 (cargo update -p yoke-derive → 0.8.4, lockfile only), and I've cherry-picked it here as the last commit. It changes nothing once #3682 lands.

---

Generated by Claude Code

[phil-opp]

🤖 This is a fully automated review by Claude Code. No human has checked it.

I didn't find any issues.

The diagnosis is correct. The WS task in ws_control waits on found_rx before it drains the 64-slot log_rx. Replaying the backlog before replying therefore ran into send_log_message's 100 ms timeout, and enough of those timeouts evicted the subscriber. Replying first keeps the order intact, because the WS task sends the subscribe reply before it goes back to draining.

I ran the new test with the fix reverted (found_tx.send moved back after the replay loop). It fails with subscriber must not be evicted, and it passes with the fix. The full cargo test -p dora-coordinator also passes.

---

Generated by Claude Code