apollo_config_manager: retry a dynamic config dispatch the component rejected

[matanl-starkware]

Fixes #14925. Independent of #14924 — branched from main, touches only the dispatch branch of update_config.

Problem

update_config advanced latest_node_dynamic_config before awaiting set_node_dynamic_config:

self.latest_node_dynamic_config = node_dynamic_config.clone();   // <-- advanced first
match self.config_manager_client.set_node_dynamic_config(node_dynamic_config.clone()).await {
    Ok(()) => { ... }
    Err(e) => Err(format!("Failed to update dynamic config: {:?}", e).into()),   // no revert, no metric
}

When the dispatch failed, two things went wrong together:

1. Never retried. latest_ already held the new value, so every later tick took the latest == new early return and did nothing. The node kept running the old config indefinitely, while the runner behaved as though the new one had been applied.
2. Never alerted. Unlike the load and validate paths, this one did not increment CONFIG_MANAGER_UPDATE_ERRORS, so config_manager_update_error_increase could not fire. The dropped update was completely silent.

This is the one path where a config change can be lost with no signal. set_node_dynamic_config can legitimately fail — the receiving component validates the dynamic config, and it is a channel-based call that can error under load.

Change

Record the config only once the component has accepted it, and count the failure:

• latest_node_dynamic_config is assigned inside the Ok arm. Leaving it untouched on failure means the next 20s tick sees a difference and retries by itself.
• The Err arm increments CONFIG_MANAGER_UPDATE_ERRORS and logs, so a persistent failure surfaces through the existing alert instead of going quiet. A single transient rejection self-heals on the next tick.

Tests

• failed_dispatch_is_retried_on_the_next_update — a client that always rejects must be called twice across two update_config calls. Verified this fails on the old ordering (mockall reports the second dispatch never happens).
• accepted_dispatch_is_not_repeated_while_the_config_is_unchanged — the counterpart, pinning that a successful dispatch is still recorded, so an unchanged file is not re-dispatched every tick.

cargo test -p apollo_config_manager — 17 passed. scripts/rust_fmt.sh --check and clippy clean.

Scope

Found while reviewing #14924 but present on main well before it; kept separate so an unrelated pre-existing bug does not ride along on that PR.

🤖 Generated with Claude Code

[cursor]

PR Summary

Medium Risk
Touches live config propagation to node components; the change corrects silent config loss but alters when state is committed on partial failure.

Overview
Fixes a bug where a rejected dynamic config dispatch was treated as applied, leaving the node on the old config with no retry and no metric.

latest_node_dynamic_config is now updated only after set_node_dynamic_config succeeds. On failure, the runner leaves the previous baseline unchanged so the next periodic or filesystem-triggered update still sees a diff and retries dispatch.

Failed dispatches now increment CONFIG_MANAGER_UPDATE_ERRORS and log at error level (metric help text extended to include dispatch). Two tests lock in retry-after-failure and no redundant dispatch after a successful apply.

^{Reviewed by Cursor Bugbot for commit 5f764e4. Bugbot is set up for automated code reviews on this repo. Configure here.}

[reviewable-StarkWare]

This change is 

[matanl-starkware]

Reviewed with an independent adversarial pass. Verdict: no high-severity findings; the retry path was mutation-tested (reverting only config_manager_runner.rs to main makes failed_dispatch_is_retried_on_the_next_update fail at its own expect_err, not via a mock-internal panic).

Applied two of the findings in this push:

• Metric description was stale — now (load/validate/dispatch), since the counter covers dispatch failures too.
• The new error! used {e:?} while the load-error path in the same function uses {e}. Aligned to Display, so both error paths grep the same way during an incident.

Two notes for reviewers, neither applied:

Log volume on a permanently-rejected config. Because latest_node_dynamic_config no longer advances on failure, a config the component keeps rejecting is retried every 20s, and log_config_diff re-emits one info! per changed key each time — a 5-key bad push left overnight is ~10k duplicate lines. There is partial precedent (the load-error path already re-logs per tick), but the multi-line diff re-log is new. Options if we care: log the diff only when it changes, or only on the first failure. Happy to add it here or follow up.

Err-after-apply divergence (unreachable today). If the client ever returns Err after the component already applied the config, the runner keeps the old value; a subsequent revert to that old value would then look like "no change" and never dispatch, leaving the component ahead. Unreachable in current deployments — config_manager is always local_with_remote_disabled, and LocalComponentClient::send panics rather than returning Err, so the only reachable Err is the component's own InvalidConfig, where it definitively did not apply. Noting it in case remote mode is ever enabled.

On the design trade-off itself: retrying forever with a firing p2 is clearly better than the old silent drop. The component rejects before writing, so there is no state thrash, and the alert now stays up exactly as long as the problem does.

[itamar-starkware]

@itamar-starkware reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on matanl-starkware).

[matanl-starkware]

@matanl-starkware reviewed 3 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on matanl-starkware).

[gkaempfer]

Security scan complete — no issues detected.

---

Generated by Claude Code