Skip duplicate managed service launch when already alive - #50
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an idempotency check to rocm serve <model> --managed so repeating the command doesn’t spawn a second managed-service process for the same model, reducing port conflicts and duplicate dashboard entries.
Changes:
- Adds an early-exit guard in
start_managed_serviceintended to detect an already-live managed service and skip spawning. - Introduces a helper (
existing_live_managed_service) to load/refresh service liveness and decide whether to skip. - Adds unit tests covering live, dead, and missing-manifest scenarios for the guard behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3714
to
+3736
| // Idempotency guard: if a managed service for this engine+model is already | ||
| // alive, surface it and spawn nothing. A second `serve --managed` (e.g. the | ||
| // chat assistant re-issuing the same request) is treated as satisfied, not | ||
| // an error. Stale/dead services fall through and relaunch normally. | ||
| if let Some(existing) = existing_live_managed_service(&paths, service_id) { | ||
| println!("managed service already running"); | ||
| println!(" service_id: {service_id}"); | ||
| println!(" endpoint: {}", existing.endpoint_url); | ||
| println!(" status: {}", existing.status); | ||
| println!(" note: existing service detected; no second process spawned"); | ||
| record_cli_audit_event( | ||
| &paths, | ||
| "service", | ||
| "managed_service_launch_skipped", | ||
| "info", | ||
| format!( | ||
| "skipped duplicate managed launch service_id={service_id} status={}", | ||
| existing.status | ||
| ), | ||
| Some(service_id), | ||
| ); | ||
| return Ok(()); | ||
| } |
Comment on lines
+11931
to
+11946
| /// Idempotency guard for managed launches: returns the existing record when a | ||
| /// managed service with this `service_id` is already alive. `load_managed_service` | ||
| /// refreshes liveness, so a stale manifest (dead PID) demotes to "stopped" and | ||
| /// yields `None` — letting a genuine relaunch proceed. Prevents a second | ||
| /// `serve --managed` for the same engine+model from spawning a duplicate process | ||
| /// once the TUI job-bridge guard has cleared. | ||
| fn existing_live_managed_service( | ||
| paths: &AppPaths, | ||
| service_id: &str, | ||
| ) -> Option<ManagedServiceRecord> { | ||
| if !paths.service_manifest_path(service_id).exists() { | ||
| return None; | ||
| } | ||
| let record = load_managed_service(paths, service_id).ok()?; | ||
| managed_service_is_live(&record).then_some(record) | ||
| } |
Comment on lines
+17408
to
+17432
| let mut record = ManagedServiceRecord::new( | ||
| &paths, | ||
| "svc-dup-live", | ||
| "lemonade", | ||
| "qwen", | ||
| "qwen", | ||
| "127.0.0.1", | ||
| 11500, | ||
| "managed", | ||
| std::process::id(), | ||
| None, | ||
| None, | ||
| None, | ||
| ); | ||
| record.status = "starting".to_owned(); | ||
| record.engine_pid = Some(std::process::id()); | ||
| record.write()?; | ||
|
|
||
| let found = existing_live_managed_service(&paths, "svc-dup-live"); | ||
| let _ = fs::remove_dir_all(root); | ||
|
|
||
| let found = found.expect("a live managed service should be detected"); | ||
| assert_eq!(found.service_id, "svc-dup-live"); | ||
| assert!(managed_service_is_live(&found)); | ||
| Ok(()) |
| record.engine_pid = Some(999_999_999); | ||
| record.write()?; | ||
|
|
||
| let found = existing_live_managed_service(&paths, "svc-dup-dead"); |
| fn missing_manifest_allows_launch() { | ||
| // No manifest on disk → nothing to detect, launch proceeds. | ||
| let (root, paths) = test_paths("dup-managed-missing"); | ||
| let found = existing_live_managed_service(&paths, "svc-absent"); |
Collaborator
Author
|
Good catch from Copilot — this was a real defect. Fixed in 05e7180:
Note: the License-header CI failure is unrelated and pre-existing on |
volen-silo
force-pushed
the
fix/duplicate-managed-service
branch
from
June 24, 2026 13:02
05e7180 to
59a4186
Compare
r0x0r
approved these changes
Jun 25, 2026
volen-silo
force-pushed
the
fix/duplicate-managed-service
branch
from
June 25, 2026 11:18
d890078 to
8ee0cc4
Compare
volen-silo
enabled auto-merge
June 25, 2026 11:18
volen-silo
disabled auto-merge
June 25, 2026 13:32
volen-silo
enabled auto-merge
June 25, 2026 13:32
start_managed_service spawned a new background process unconditionally, so a second `serve --managed` for the same engine+model (e.g. the chat assistant re-issuing the request) produced a duplicate process, port conflict, and a doubled dashboard entry. The TUI job-bridge guard only covers job state and clears once the first job detaches. Guard at the single launch choke point: if a live manifest already exists for the service_id, surface it and spawn nothing (idempotent success). Stale/dead records refresh to "stopped" and relaunch normally.
generate_service_id embeds unix_time_millis(), so every launch mints a unique service_id — the previous guard keyed on it would never match an existing service and the duplicate still spawned. Scan existing managed services by (engine, canonical_model_id) via load_managed_services (which refreshes liveness, newest-first) and skip on the first live match, reporting the existing service_id. Tests now use distinct service_ids for the same engine+model.
volen-silo
force-pushed
the
fix/duplicate-managed-service
branch
from
June 25, 2026 13:35
8ee0cc4 to
125766a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
rocm serve <model> --managedspawns a managed-service process unconditionally. Running it a second time for the same engine+model — e.g. the chat assistant re-issuingrocm serve <path> --engine <e> --managedvia therocm_commandtool, or re-confirming the serve wizard — spawns a second process for the same model/port. Both run simultaneously, causing port conflicts and a doubled entry in the dashboard.The TUI job-bridge has an idempotency guard, but it only covers TUI job state — not the OS process. Once the first job completes (the managed daemon detaches quickly), the guard clears and a second launch succeeds.
Fix
start_managed_serviceis the single choke point for every managed launch. Added an idempotency guard there:existing_live_managed_service(paths, service_id)reuses the existingload_managed_service(which refreshes liveness) +managed_service_is_live. Sinceservice_id = generate_service_id(engine, canonical_model_id), the same engine+model always collides and is detectable.managed_service_launch_skippedaudit event, and returnOk(())— no second process spawned. Treated as idempotent success so the chat assistant sees the repeat request as satisfied, not failed.stoppedand yieldNone, so a genuine relaunch still proceeds.The file-based manifest persists across the job-bridge race, so this is robust where the TUI guard wasn't.
Tests
duplicate_managed_launch_is_detected_when_live— live PID ⇒ detected.dead_managed_service_allows_relaunch— dead PID ⇒ relaunch allowed.missing_manifest_allows_launch— no manifest ⇒ launch proceeds.cargo test -p rocm(managed suite) andcargo clippy -p rocm --all-targetsboth green.Known limitation
Out of scope: two truly-concurrent launches within ~200ms before either writes a live manifest. The realistic "asked twice" sequence is fully serialized, so the file guard catches it.