[jussielo-amd]

Why

run_loop in the dash daemon's runner was one function of about 730 lines. Three near-duplicate blocks upserted rediscovered services, announced new ones and retired vanished ones, once each for Docker, the Lemonade endpoint and the managed-service registry, and the same three telemetry maps (the generation trackers and the TTFT and TPOT baselines) were threaded through every one of them by hand. This restructures that function without changing what it does, and gives ticks_per the doc comment that had landed on its neighbour. Completes ROCMAI-852.

What changes

Four commits carry the change, so each can be read on its own, followed by a merge of main and three review follow-up commits.

The first adds crates/rocm-dash-daemon/tests/discovery_reconcile.rs and nothing else. It drives the real run_loop through the managed-service registry and the Lemonade endpoint and pins what the broadcast stream shows: a service is announced once however many ticks rediscover it, it keeps its scraped KV cache and request counts on ticks that do not scrape, it starts clean when the registry moves it to another port (the old endpoint's reading must not leak onto the new one), and a Lemonade endpoint that stops answering is announced gone and leaves the snapshots. These pass on the unmodified runner.

The second is the refactor. Docker and the registry now go through one Reconcile::reconcile_set, and Lemonade through Reconcile::reconcile_lemonade, both built from the same upsert, announce and retire steps. A DiscoverySource value only selects the log wording, so the log fields and messages are the same as before. The per-instance telemetry maps move into TelemetryState and the per-source known sets into DiscoveryState. run_loop goes from 733 to 570 lines; what remains is the GPU adoption, the scrapes and the snapshot assembly, which this change leaves alone. The existing purge_telemetry_state and invalidate_on_endpoint_change functions and their tests are untouched, and the new state type calls them. The commit also adds unit tests for the helper next to the code.

The third moves one doc line: ticks_per now carries its own doc comment, and broadcast_and_persist keeps its own.

The fourth puts the announced-once test under one deadline for its whole observation instead of a fresh deadline per event.

The merge commit brings in main, which had moved 21 commits on; none of them touch the daemon crate, so it merged without conflicts. CI had only run for the first push of this branch, so this merge also starts the full set of lanes on the new head.

The first review follow-up commit changes tests and one test-fixture doc line only, in response to review: the Lemonade rediscovery watch now runs under one deadline, the managed endpoint-change test now asserts that no discovered or gone announcement for the service appears while the port moves, the registry record is written through a temp file and a rename so a discovery tick can never read half a record, and the Lemonade lifecycle unit test now pins that the superseded instance stays in state. The runner code is not touched by it.

The last two commits change comments only, in response to a further review round and a sweep of the same kind of defect across what this PR adds: the module doc of discovery_reconcile.rs now says which source each test drives through the loop and which behaviours are covered only by the unit tests in runner.rs; the snapshot_where doc says exactly how its deadline works; the endpoint-change test comment no longer says the instance "stayed" on its new port, which the test does not check; and in runner.rs the DiscoverySource and reconcile_set docs, a loop comment that named a constant that does not exist, and a lifecycle-test comment that claimed a carry-over the test cannot observe now match the code. The second of the two qualifies the heading comment of the reconcile unit-test section in runner.rs: the registry and Lemonade sources are driven end to end in discovery_reconcile.rs, except the registry's retirement of a vanished service and Lemonade's change of id, which only the unit tests cover. No code, assertion or test name changed in either.

Two things are deliberately not changed. The test clock stays in the runner: test_clock_offset_path is a runtime option that the end-to-end scenarios use against the real daemon binary, so it cannot become test-only code. And while moving the Lemonade block I noticed that the old comment said a changed host or port is treated as "old gone, new discovered", but the code only purges the old id's telemetry and announces the new id; it never retires the old instance. I kept that behaviour, corrected the comment to say what the code does, and did not fix it here because that would change what the dashboard shows. It is worth its own ticket, and the lifecycle unit test now fails if the behaviour changes without that being a deliberate decision.

Verification

Run locally on the merged head, on a debug build:

• cargo test -p rocm-dash-daemon (re-run on the comment-only head): 51 unit, 4 discovery_reconcile, 9 end_to_end and 9 telemetry_contract tests pass. The last two are the existing suites over run_loop and pass unchanged. discovery_reconcile was run five more times in a row, 4 of 4 each time, to check it is not flaky.

• cargo fmt --all --check, cargo clippy -p rocm-dash-daemon --all-targets -- -D warnings, cargo xtask manifest --check and cargo xtask check-crate-edges are clean. The leak scan over the diff against main and the conflict-marker grep are clean, and the commit hooks (fmt, license headers, link check, signing) passed on each commit.

• The test names of the crate against main differ only by the nine tests this PR adds (four in discovery_reconcile, five reconcile_* unit tests); none were removed or renamed.

• Each discovery_reconcile test was shown to fail when its target is broken, one targeted edit at a time against the unmodified runner, restored afterwards: the managed block no longer carrying live fields (managed_rediscovery_keeps_live_scrape_fields_between_scrapes failed: "kv cache was dropped on a tick without a scrape: None"), the managed block ignoring an endpoint change (managed_endpoint_change_starts_the_instance_clean failed: "the old endpoint's kv cache leaked onto the new one"), the managed block announcing on every pass (managed_service_is_announced_once_across_rediscovery failed: "the service must be announced exactly once"), and the Lemonade block never retiring (lemonade_endpoint_is_announced_scraped_and_announced_gone timed out waiting for the gone event). Only the targeted test failed each time. The helper's unit tests got the same treatment on the new code.

• The review follow-up assertions were shown to fail the same way. Making reconcile_set announce the service when its port changes turns managed_endpoint_change_starts_the_instance_clean red with "an endpoint change must not announce the service as discovered or gone: ["discovered"]". Simulating a record caught mid-write (a truncated file, completed 700 ms later) makes the same test fail with ["gone", "discovered"], which is what a non-atomic write would look like to the runner. Removing the superseded Lemonade instance from state on an id change fails the new contains_key("l1") assertion. Making the Lemonade rediscovery watch's count unreachable fails after 20 seconds with "timed out after 20s waiting for 6 snapshots with no second announcement of the Lemonade endpoint; seen so far: 100 snapshots", and making the Lemonade block announce on every pass still trips the second-announcement panic inside the watch.

• Which waits are bounded as a whole: the three watches that count a condition across events (announced-once, the managed endpoint change and the Lemonade rediscovery) run through the observe helper under one deadline and fail after 20 seconds naming what they waited for. snapshot_where stops retrying once 20 seconds have passed. Two waits are not under a single deadline across calls: the ten-snapshot loop in managed_rediscovery_keeps_live_scrape_fields_between_scrapes is count-limited and each of its snapshot waits has its own 20 second deadline, and the one-shot next_event waits each have one deadline per wait. Neither can loop forever.

• Coverage, measured for this crate alone with cargo llvm-cov -p rocm-dash-daemon (the same flags the coverage gate uses, but without the other crates' tests): line coverage of the crate is 83.35% on main and 89.18% on this head, against a committed floor of 82.62%.

Not run

The full workspace tests, the tests/e2e-cucumber suite (including the dash scenarios that exercise this loop through the real binary), the Windows lane and the workspace-wide cargo xtask coverage gate were not run; CI carries them. The Docker source cannot be driven end to end without a container daemon, so its reconcile is covered only by the reconcile_set unit tests. The Docker-specific lines that remain in run_loop, building the seen set from container ids and holding the discovery warning, are exercised by no test here.

Overlaps

#812, which also edited runner.rs and which the ticket asked this change to land after, has merged, and this branch has merged main since (0f28c84), re-applying #812's windowed latency handling through TelemetryState. #631 also edits runner.rs and still conflicts with this branch there (and in crates/rocm-dash-core/src/metrics.rs, which this PR does not touch), so whichever of the two lands second will need to resolve that by hand. #447 merges cleanly. This PR does not touch docs/architecture.md.

Scope

This is a daemon-internal refactor with no change to what a user can observe, so it adds no new end-to-end scenario; the new integration tests pin the broadcast stream instead, and the existing dash scenarios plus end_to_end and telemetry_contract cover the rest.

[Copilot]

🟡 Changes recommended

The discovery reconciliation test can hang indefinitely on empty snapshots because each event resets the deadline; the entire observation loop needs a timeout to ensure a bounded failure.

1 open issue

• Bound the snapshot observation loop with a timeout · New

What changed in this PR

This PR simplifies service discovery in the dashboard daemon by consolidating reconciliation logic while preserving existing behavior.

Changes:

• Introduces shared reconciliation helpers and groups discovery and telemetry state.
• Adds integration and unit tests for announcements, telemetry preservation, endpoint changes, and service removal.
• Moves the ticks_per doc comment to the correct function.

File	Description
crates/​rocm-dash-daemon/​tests/​discovery_reconcile.rs	Adds broadcast-stream characterization tests for managed services and Lemonade.
crates/​rocm-dash-daemon/​src/​runner.rs	Extracts shared reconciliation logic, adds helper tests, and corrects doc-comment placement.

_{Review effort: Balanced}

[nowycondro]

🤖 Automated review (Claude Code) · 675d6e3

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — no blocking findings

Full review of the whole change (both changed files, read completely, plus the surrounding unchanged run_loop context and the collaborators the new tests depend on). Implements ROCMAI-852 ("bundle the loop locals into structs, extract one discovery-reconcile helper, keep MissedTickBehavior::Skip"); the ticket also lists a precondition this PR's own description frames differently — see Deployment notes.

Blocking

None

Non-blocking

• Stale doc comment on a test fixture — crates/rocm-dash-daemon/tests/discovery_reconcile.rs:94-111
vllm_metrics()'s doc comment claims "a growing token counter," but the function returns a fixed, unchanging metrics body (vllm:generation_tokens_total{model="mock"} 100) on every call — there is no counter, closure-captured state, or increment logic anywhere, and spawn_server(|_| vllm_metrics()) serves the identical static body to every request. None of the file's four tests assert on gen_tps/counter-differencing from this fixture, so there's no masked coverage gap, but the comment misdescribes the fixture to a future reader.
  Confidence 85 · mechanical · Fix: reword to "a fixed token counter" (or make it actually increment if a future test needs the rate-tracker's differencing path exercised through this fixture).

• Pre-existing error-handling asymmetry between discovery sources, now shared through one call pattern — crates/rocm-dash-daemon/src/runner.rs:510-527 (Docker) vs :545-558 (managed registry)
  Docker's discover_async() returns a real Result; on Err, reconcile_set is simply not invoked and the known set is left untouched, so a transient Docker-daemon hiccup can't cause a mass "gone" storm. The managed-service registry's load_service_records/discover_managed_services are infallible by construction (an unreadable directory or malformed record silently becomes "zero services"), and reconcile_set is still called unconditionally with whatever disc.seen comes back — a transient registry-directory read glitch could read as "everything vanished" and retire every managed instance. This asymmetry predates this PR and lives entirely in registry.rs (untouched by this diff); it is called out here because centralizing dispatch through reconcile_set is what makes the asymmetry visible in one place.
  Confidence 70 · architectural · Fix: worth a follow-up to harden discover_managed_services's read-failure path to distinguish "read failed" from "confirmed empty," mirroring Docker's defensive pattern — out of scope for this refactor PR itself.

Decisions for the author

• Lemonade's reconcile path never retires the superseded id on an endpoint change — answered-by-intent
  reconcile_lemonade (runner.rs:1102-1125) purges the old id's telemetry and announces the new id on an identity change, but never calls retire for the old id — so the stale Instance is never removed from State.instances, is broadcast in every snapshot thereafter, and (since the vLLM-scrape-target exclusion filter only checks the current lemonade_id, runner.rs:575) keeps getting polled as a dead vLLM target, generating ongoing spurious scrape warnings. The PR body answers this directly: "the code only purges the old id's telemetry and announces the new id; it never retires the old instance. I kept that behaviour, corrected the comment to say what the code does, and did not fix it here because that would change what the dashboard shows. It is worth its own ticket." Would otherwise have been a non-blocking (pre-existing, pattern-relevant) finding — it is unchanged by this refactor, which in fact makes the one-line fix easier to land later (one call site now, not three). Confirm a follow-up ticket is actually filed for it.

• Docker's seen-computation line has no test reaching it beyond type-check — answered-by-intent
  runner.rs:515-516 (let seen: HashSet<String> = svcs.iter().map(|s| s.container_id.clone()).collect();) is exercised by neither the hand-built reconcile_set unit tests nor the end-to-end integration tests, since Docker can't be driven without a container daemon (confirmed independently: DockerDiscovery's sync ServiceDiscovery impl returns Unsupported). The PR body states this plainly: "The Docker-specific lines that remain in run_loop, building the seen set from container ids and holding the discovery warning, are exercised by no test here." Would otherwise have been a minor non-blocking finding given the line's triviality — disclosed rather than hidden.

• "One helper" slightly overstates the unification — tradeoff
  reconcile_set (Docker/Managed, whole-set-report shape) and reconcile_lemonade (single-probe, no-retire-on-id-change shape) are two distinct patterns sharing only the three genuinely-common primitives (upsert/announce/retire). This is disclosed honestly in the code's own doc comments rather than hidden, so it's a naming/documentation call rather than a defect. A future 4th discovery source shaped like neither existing pattern would need a third method; the author may want a more pointed name than reconcile_lemonade (e.g. one that flags "no retire on id change") to guard against accidental reuse by a dissimilar source — or leave it as is.

Positive signals

• Characterization tests landed in their own commit before the restructuring commit, and the PR's own verification notes record exactly which test failed for which deliberately-broken target — real, demonstrated behavioral-equivalence discipline, not just an assertion of it.
• observe() in discovery_reconcile.rs correctly fixes the classic "fresh deadline per call" test-hang hazard (the same one Copilot's review flagged on an earlier commit in this PR) with a single overall deadline plus a progress callback naming what was being waited for — good test design, and the fix is itself covered by the commit that introduces it.

Deployment notes

• Merge-order dependency: Jira ROCMAI-852 states "Waits on. #812 edits runner.rs; land after it and rebase" — a one-directional constraint — while this PR's own "Overlaps" section frames it symmetrically ("whichever lands second will need a rebase"). PR #812 is still open/unmerged as of this review. Confirm the intended merge order before landing.
• CI not yet run for the exact reviewed commit: GitHub Actions (CodeQL, E2E self-hosted, CI) all report success, but only for the prior commit on this branch (e7addca0), not yet for the current head (675d6e3d, which only changes a test helper's timeout logic). Confirm CI completes successfully for 675d6e3d before merging.

What this covered

Full review of crates/rocm-dash-daemon/src/runner.rs (complete file, both before/after the refactor — the pre-refactor version was pulled at the merge-base commit to verify behavioral equivalence line-by-line) and crates/rocm-dash-daemon/tests/discovery_reconcile.rs (complete, new file), at commit dcb6bcc3…675d6e3d. Also read: registry.rs and lemonade.rs/docker.rs collaborator semantics only as far as needed to verify the new tests' assumptions (unchanged, out of diff scope). Ran as three independent fan-out passes (code-correctness over the coupled file group; the four source-reading passes — agent-instruction adherence, history, prior-changes, code-comments; and a five-question design review), consolidated here, plus this consolidating pass's own CI/Jira/overlap checks.

Did not run: no Rust toolchain was available in the review sandbox, so no candidate was confirmed by compiling or executing the test suite — all code-level findings above are argued from reading the code (and, where noted, from diffing against the pre-refactor version), not from a run. This review's own confirmation of CI results came from GitHub's reported check statuses, not from re-running anything.
Not reconciled against this PR's own review discussion (the Copilot bot review and the author's own comment on this PR) — this review was conducted blind to that discussion, as the method requires.

🤖 Generated with Claude Code

[juhovainio]

I went through the whole diff. The runner refactor itself looks behaviour-preserving: I compared the old and new Docker, Lemonade and managed blocks side by side and found no drift in purge/upsert/announce order or in the log fields. Two things should be fixed before merge, plus two smaller test-honesty points (inline).

Blocking

1. No CI has run on this head beyond the docs build. Only docs/readthedocs.com is reported, and mergeStateStatus is BLOCKED. The PR relies on the workspace tests, the dash scenarios and the coverage floors, and none of that evidence exists yet. Please get the full lanes running and green.
2. The PR body doesn't match the diff. It says three commits, but there are four. It also says every wait in discovery_reconcile.rs is now bounded as a whole, yet the Lemonade test still loops while snapshots < 6 { next_event(...) } and gives each call a fresh 20 s deadline. That is exactly the pattern the body says was removed. Either route that loop through observe, or narrow the claim, and update the commit count.

Non-blocking

• The doc on managed_endpoint_change_starts_the_instance_clean says the change "must not be announced as a new service", but the test only checks that kv_cache/running_reqs are None.
• The Lemonade "old id is never retired" quirk is documented but not pinned by any assertion.

Also worth noting: the ticket says to land this after #812, which is still open.

[juhovainio]

The while snapshots < 6 loop that starts here calls next_event repeatedly, so every iteration gets a fresh 20 s deadline. That is the pattern the PR body says was replaced with observe. Use observe here too, or narrow the body claim.

[jussielo-amd]

Fixed in 93379e5: the Lemonade snapshot wait now goes through observe, so the whole loop shares one deadline instead of each next_event call getting a fresh 20 s. I also checked the unreachable case by making the count unreachable: it now fails after 20 s with the snapshots seen so far instead of waiting forever.

[juhovainio]

The doc says the change "must not be announced as a new service", but the test never asserts that (only kv/reqs are None). It would also pass if a non-atomic fs::write were caught mid-write, retiring and re-announcing the service. Assert there is no extra InstanceDiscovered/InstanceGone for SVC_ID, and write the record via temp file + rename.

[jussielo-amd]

Done in 93379e5. The test now watches the event stream from the record change until three snapshots after it, and asserts that no InstanceDiscovered or InstanceGone for SVC_ID is emitted. write_record writes a .json.tmp file and renames it, so the reader can never see a partial file. I confirmed both assertions go red: making reconcile_set announce on a port change fails with ["discovered"], and simulating a record caught mid-write (truncated, completed 700 ms later) fails with ["gone","discovered"].

[juhovainio]

This pins that l1's telemetry is purged but not that l1 stays in state.instances (the documented "old instance is not retired" quirk). Add assert!(pass.state.instances.contains_key("l1")) with a comment pointing at the follow-up, so a future change to that behaviour is deliberate.

[jussielo-amd]

Added in 93379e5: assert!(pass.state.instances.contains_key("l1")) with a comment that retiring the old instance later is a deliberate behaviour change. I checked it fails when reconcile_lemonade removes l1 on an id change. The change itself is tracked as ROCMAI-866 (the old Lemonade instance is never retired when its host or port changes, so it stays in every snapshot and keeps being scraped as a vLLM target).

[jussielo-amd]

Pushed bb35c42 (main merged in) and 93379e5 (the test fixes from review), which starts the full CI lanes on this head; before this only the docs build had reported, because the later pushes never triggered workflows. The description is corrected: it now says four commits plus the merge and the follow-up, and the claim about bounded waits is precise (every wait in discovery_reconcile.rs is now bounded as a whole; the remaining count-limited loops are named). Local results: fmt and clippy clean, cargo test -p rocm-dash-daemon passes (51 unit, 4 discovery, 9 end-to-end, 9 telemetry), the discovery test was run five more times without a failure, and per-crate coverage is 83.35% on main versus 89.18% here against a floor of 82.62%.

On ordering: the ticket asks for this to land after #812, which is still open (changes requested). #812 merges cleanly into main but conflicts with this branch in runner.rs, in four hunks (two in the run_loop local declarations, where this PR's discovery and telemetry state structs replace the maps #812 extends, and two in the TTFT/TPOT scrape handling). Whichever lands second will need a mechanical re-apply of those hunks; I am happy to do that for either order.

[Copilot]

🟢 Approval recommended

The discovery reconciliation refactor has no active review issues.

No open issues 🎉

1 resolved since last review

• Bound the snapshot observation loop with a timeout

_{Review effort: Balanced}

[jussielo-amd]

Pushed a1a71d4 and 1afbda1, both comment-only, after a last pass over the doc comments, test comments and test names this PR adds. Everything the review asked for was already fixed and answered above; these two commits fix the same kind of problem elsewhere. The module doc of discovery_reconcile.rs and the DiscoverySource and reconcile_set docs in runner.rs now say which behaviour each test covers. The registry's retirement of a vanished service and Lemonade's change of id are driven only by the unit tests in runner.rs, not by the integration file. The snapshot_where doc no longer claims one deadline for the whole wait, a comment that named a constant that does not exist now names discovery_ticks, and two test comments no longer claim more than the test asserts. No code line changed, and fmt, clippy and cargo test -p rocm-dash-daemon are clean on the new head. The description is updated to match the commit list.

[jussielo-amd]

Merged main into this branch (0f28c84, a plain merge commit, no history rewrite) because #812 landed and conflicted in crates/rocm-dash-daemon/src/runner.rs. Nothing in the earlier review fixes changed. The conflict was in run_loop: this PR keeps the loop's state in DiscoveryState and TelemetryState, while #812 added windowed latency tracking (LatencyWindows, observe_latency, rebaseline_latency) against the old local variables. I kept this PR's structure and re-applied #812's latency handling through the new state bundles, including two places that merged without a marker but still named the removed locals (the snapshot assembly and the Pass::scrape test helper). The resulting runner.rs differs from this PR's previous head only by #812's change set. cargo test -p rocm-dash-daemon passes (all #812 tests and all of this PR's tests are present), clippy and fmt are clean, and breaking the relocated TTFT snapshot call turns ttft_is_held_then_cleared_when_requests_stop red.

fixed

[juhovainio]

Re-reviewed at 0f28c84. No blocking findings.

I compared reconcile_set / reconcile_lemonade against the removed Docker, Lemonade and managed blocks: ordering (upsert, announce if unknown, retire the difference), live-field carry-over, telemetry purge on id change and log wording all match. The ticket scope (struct bundling, one reconcile helper, ticks_per doc fix, characterisation tests first, MissedTickBehavior::Skip untouched) is covered, and CI is green.

Non-blocking:

• The PR body's "Overlaps" section is stale: #812 has merged and the PR is now mergeable, so the "whichever lands second resolves by hand" text no longer applies. Worth updating so the body matches the final state.
• The TelemetryState wrapper methods only forward to same-named free functions (inline comment).
• The Docker-specific lines left in run_loop remain untested, as the PR notes.
• The Lemonade id-change gap (old instance not retired) is preserved and now pinned by a test; please make sure it has its own ticket rather than living only in a comment.

Note: I read this from the diff and CI results; I did not rebuild locally or re-run the mutation checks.

[juhovainio]

Non-blocking: purge and invalidate_on_endpoint_change only forward to same-named free functions that are otherwise used only by unit tests. Consider making the methods the real implementation (and updating those tests) so each operation has one name. Optional.

[jussielo-amd]

Done in f088812: the methods are now the implementation, the free functions are gone, and the two tests call the methods.

[jussielo-amd]

Thanks for the re-review. Going through the non-blocking notes: the PR body's "Overlaps" section is updated to the current state (#812 has merged and this branch has merged main; #631 is the one that still conflicts, in runner.rs). The Lemonade id-change gap has its own ticket, ROCMAI-866 ("retire the old Lemonade instance when its host or port changes"), which is picked up after this PR merges, and the lifecycle test that pins the current behaviour is the one that should change with it. The Docker-specific lines left in run_loop stay untested, as the body says, because exercising them needs a Docker socket. On the forwarding wrappers on TelemetryState (purge and invalidate_on_endpoint_change calling the same-named free functions, which only unit tests otherwise use): I am making the methods the real implementation and moving those tests onto them so each operation has one name, in a follow-up commit on this branch.

[jussielo-amd]

Pushed f088812, the follow-up I mentioned: TelemetryState::purge and TelemetryState::invalidate_on_endpoint_change now hold the implementation and the two same-named free functions are deleted, so each operation has one name. The two unit tests moved onto the methods with their assertions unchanged (the purge test is renamed to telemetry_state_purge_removes_all_three_maps_for_id because its old name mentioned the removed function). I checked each still fails when its target is broken: dropping the prev_tpot removal from purge, replacing the tracker invalidation with a no-op, and dropping the prev_ttft removal on an endpoint change each turn a test red, and I restored each edit. The crate's test counts are unchanged (56 unit, 4, 9 and 11 integration).