[Syedowais312]

Problem

When a container is removed externally (e.g. docker system prune) and
microcks start recreates it, the old instance entry was never removed from
config causing duplicate instance records to accumulate on every recreate.

Two root causes:

1. RemoveInstance matched by Name (not unique) instead of ContainerID (unique)
2. The autoRemove path in stop.go was passing instance.Name to RemoveInstance,
   silently failing to remove the correct entry
3. start.go was clearing instance.ContainerID = "" instead of calling
   RemoveInstance to actually remove the stale record from config

Changes

• cmd/start.go: call localConfig.RemoveInstance(instance.ContainerID) to
  remove the stale record before recreating, instead of just clearing the local variable
• cmd/stop.go: pass instance.ContainerID to RemoveInstance in the autoRemove path
• pkg/config/localconfig.go: RemoveInstance now matches by ContainerID instead of Name

Testing

Ran docker rm <container> while config shows status: Running, then
microcks start --name <instance> config shows a single clean entry with
the new container ID, no duplicates.

Realted: #456

Fix: #483

[Vaishnav88sk]

Matching by ContainerID instead of Name is the correct approach. Could you add a test case for the duplicate-entry scenario?

[Syedowais312]

Matching by ContainerID instead of Name is the correct approach. Could you add a test case for the duplicate-entry scenario?

i have added appropriate test cases in pkg/config/localconfig_test.go

[Vaishnav88sk]

Thanks for adding tests after the earlier review feedback.

One thing I want to call out: the PR title mentions “guard stop against missing containers”, but I don’t see an actual missing-container guard added in stop.go in this diff. So:

• add the missing guard behavior as well.

[Syedowais312]

Thanks for adding tests after the earlier review feedback.

One thing I want to call out: the PR title mentions “guard stop against missing containers”, but I don’t see an actual missing-container guard added in stop.go in this diff. So:

sorry i missed it earlier.
i have added a checks to verify whether the container still exists before trying to stop it.

[Vaishnav88sk]

Please resolve that. After that LGTM. 👍🏻

[Caesarsage]

Thanks @Syedowais312

The reconcile logic on master resets the stale entry in place (Status/ContainerID cleared, then reused) rather than appending a new record — see the "Reconcile before trusting it" block in cmd/start.go.

Could you re-verify duplicates still accumulate on a current build? If not, this can be closed.

[Syedowais312]

Thanks @Syedowais312

The reconcile logic on master resets the stale entry in place (Status/ContainerID cleared, then reused) rather than appending a new record — see the "Reconcile before trusting it" block in cmd/start.go.

Could you re-verify duplicates still accumulate on a current build? If not, this can be closed.

Hey @Caesarsage, I pulled the latest master and verified again. The bug still seems to exist.

The reason is around this line:

microcks-cli/cmd/start.go

Line 102 in 0dd8f55

instance.Status = ""

Here we clear instance.Status and instance.ContainerID when the old container no longer exists. Because the status becomes empty, the switch falls through to the default case and creates a new container.

But the old instance entry is still present in the config with the old container ID. Later, UpsertInstance compares using ContainerID, so the recreated instance has a new ID and gets appended instead of replacing the old entry.

In this PR, I removed the stale instance before recreating it. I also changed RemoveInstance to remove by ContainerID, because that is the exact stale entry we are trying to clean up.

[Syedowais312]

Hey, I've rebased the branch onto the latest master. Could you take a look when you get a chance?

[Vaishnav88sk]

Almost there! Thanks for moving StopContainer to the else block.

However, the return nil and the 'Run microcks start to bring it back' print statement are still inside the if !exists block. Because of that return nil, the code still exits early and skips the // update configs section at the bottom.

Could you please remove the return nil (and optionally the second print statement) so that execution falls through to the config cleanup?

It should look exactly like this:

  		if !exists {
  			fmt.Printf("Container for instance %s no longer exists\n", instance.Name)
  		} else {
  			if err := containerClient.StopContainer(instance.ContainerID); err != nil {
  				return errors.Wrap(errors.KindEnvironment, fmt.Errorf("failed to stop container: %w", err))
  			}
  			fmt.Println("")
  			log.Printf("Instance %s stopped successfully", instance.Name)
  		}

  		// update configs

[Syedowais312]

Thanks for the review! Removed the return nil ,but I kept the second print statement since it tells the user how to bring the container back, which seemed useful for UX? or would you prefer I remove it?

[Vaishnav88sk]

Let's keep it.

[Vaishnav88sk]

LGTM

[yada]

Verified the merge result (build, go vet, go test ./... pass) and ran it live with Docker (alpine image, --no-wait). Start, docker rm -f, then start again: master leaves 2 instance entries with the same name; this PR leaves 1. stop after an external docker rm fails on master with No such container (exit 14); with this PR it prints that the container no longer exists, suggests microcks start, and exits 0. RemoveInstance is only called from start and stop, so the switch to container ID is safe.

Disclosure: this comment was drafted with AI assistance, and the results were run locally.

[yada]

Verified locally. Disclosure: AI-assisted verification.