fix(ephemeral): avoid name collisions when generating random container name (#2108)

Generate the random ephemeral container name in a loop that re-rolls if
the name already exists according to the container manager, and fail
loudly after a small number of attempts. Dry-run mode bypasses the
existence check to match the rest of the dry-run pipeline.

Also extends MockContainerManager with an optional ExistsFn override so
tests can simulate collisions without changing behavior for existing
tests (the default still returns false).

pkg/commands/ephemeral.go

@@ -11,7 +11,11 @@ import (
 	"github.com/89luca89/distrobox/pkg/ui"
 )
 
-const ephemeralCleanupTimeout = 30 * time.Second
+const (
+	ephemeralCleanupTimeout       = 30 * time.Second
+	ephemeralMaxNameGenAttempts   = 10
+	ephemeralRandomNameSuffixSize = 10
+)
 
 type EphemeralOptions struct {
 	CreateOptions
@@ -52,7 +56,11 @@ func NewEphemeralCommand(
 func (c *EphemeralCommand) Execute(ctx context.Context, opts EphemeralOptions) error {
 	name := opts.ContainerName
 	if name == "" {
-		name = makeRandomName()
+		generatedName, err := c.makeUniqueRandomName(ctx, opts.DryRun)
+		if err != nil {
+			return fmt.Errorf("ephemeral: %w", err)
+		}
+		name = generatedName
 	}
 
 	// create ephemeral container
@@ -92,13 +100,26 @@ func (c *EphemeralCommand) Execute(ctx context.Context, opts EphemeralOptions) e
 	return nil
 }
 
+// makeUniqueRandomName generates a random container name that does not
+// collide with an existing container. When dryRun is true, the existence
+// check is skipped (mirroring the rest of the dry-run pipeline, where no
+// container lookup is performed).
+func (c *EphemeralCommand) makeUniqueRandomName(ctx context.Context, dryRun bool) (string, error) {
+	for range ephemeralMaxNameGenAttempts {
+		name := makeRandomName()
+		if dryRun || !c.containerManager.Exists(ctx, name) {
+			return name, nil
+		}
+	}
+	return "", fmt.Errorf("failed to generate unique ephemeral container name after %d attempts", ephemeralMaxNameGenAttempts)
+}
+
 func makeRandomName() string {
 	const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
 	l := len(charset)
-	b := make([]byte, 10) //nolint:mnd // length of random part
+	b := make([]byte, ephemeralRandomNameSuffixSize)
 	for i := range b {
 		b[i] = charset[rand.IntN(l)] //nolint:gosec // cryptographic security not needed
 	}
-	// FIXME: avoid collisions
 	return fmt.Sprintf("distrobox-%s", string(b))
 }

pkg/commands/ephemeral_internal_test.go

@@ -0,0 +1,78 @@
+package commands
+
+import (
+	"bufio"
+	"context"
+	"io"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/89luca89/distrobox/pkg/config"
+	"github.com/89luca89/distrobox/pkg/internal/testutil"
+	"github.com/89luca89/distrobox/pkg/ui"
+)
+
+func newTestEphemeralCommand(mock *testutil.MockContainerManager) *EphemeralCommand {
+	progress := ui.NewDevNullProgress()
+	prompter := ui.NewPrompter(*bufio.NewReader(strings.NewReader("")), io.Discard)
+	printer := ui.NewPrinter(io.Discard, false)
+	return NewEphemeralCommand(&config.Values{}, mock, progress, printer, prompter)
+}
+
+func TestEphemeralCommand_MakeUniqueRandomName_FirstAttemptUnique(t *testing.T) {
+	mock := &testutil.MockContainerManager{}
+	cmd := newTestEphemeralCommand(mock)
+
+	name, err := cmd.makeUniqueRandomName(context.Background(), false)
+	require.NoError(t, err)
+	assert.True(t, strings.HasPrefix(name, "distrobox-"), "expected name to have distrobox- prefix, got %q", name)
+	require.Len(t, mock.Spy.Exists, 1)
+}
+
+func TestEphemeralCommand_MakeUniqueRandomName_RetriesAfterCollisions(t *testing.T) {
+	const collisions = 3
+
+	mock := &testutil.MockContainerManager{}
+	calls := 0
+	mock.ExistsFn = func(_ string) bool {
+		calls++
+		return calls <= collisions
+	}
+	cmd := newTestEphemeralCommand(mock)
+
+	name, err := cmd.makeUniqueRandomName(context.Background(), false)
+	require.NoError(t, err)
+	assert.True(t, strings.HasPrefix(name, "distrobox-"), "expected name to have distrobox- prefix, got %q", name)
+	require.Len(t, mock.Spy.Exists, collisions+1)
+}
+
+func TestEphemeralCommand_MakeUniqueRandomName_AllCollideReturnsError(t *testing.T) {
+	mock := &testutil.MockContainerManager{}
+	mock.ExistsFn = func(_ string) bool {
+		return true
+	}
+	cmd := newTestEphemeralCommand(mock)
+
+	name, err := cmd.makeUniqueRandomName(context.Background(), false)
+	require.Error(t, err)
+	assert.Empty(t, name)
+	assert.Contains(t, err.Error(), "failed to generate unique ephemeral container name")
+	require.Len(t, mock.Spy.Exists, ephemeralMaxNameGenAttempts)
+}
+
+func TestEphemeralCommand_MakeUniqueRandomName_DryRunSkipsExistenceCheck(t *testing.T) {
+	mock := &testutil.MockContainerManager{}
+	mock.ExistsFn = func(_ string) bool {
+		// would always collide, but dry-run path must skip this check
+		return true
+	}
+	cmd := newTestEphemeralCommand(mock)
+
+	name, err := cmd.makeUniqueRandomName(context.Background(), true)
+	require.NoError(t, err)
+	assert.True(t, strings.HasPrefix(name, "distrobox-"), "expected name to have distrobox- prefix, got %q", name)
+	assert.Empty(t, mock.Spy.Exists, "Exists must not be called in dry-run mode")
+}

pkg/internal/testutil/mock_containermanager.go

@@ -30,10 +30,15 @@ type ContainerManagerSpy struct {
 // CloneAsRoot returns a distinct MockContainerManager so tests can
 // distinguish calls made on the rootless vs root variant. The clone is
 // cached on RootClone after first creation.
+//
+// ExistsFn, when non-nil, overrides the default Exists behavior (which
+// always returns false). Tests can use it to simulate name collisions
+// or other lookup scenarios.
 type MockContainerManager struct {
 	Spy       ContainerManagerSpy
 	Root      bool
 	RootClone *MockContainerManager
+	ExistsFn  func(containerName string) bool
 }
 
 func (m *MockContainerManager) Name() string {
@@ -44,7 +49,12 @@ func (m *MockContainerManager) Name() string {
 func (m *MockContainerManager) CloneAsRoot() containermanager.ContainerManager {
 	m.Spy.CloneAsRoot = append(m.Spy.CloneAsRoot, []any{})
 	if m.RootClone == nil {
-		m.RootClone = &MockContainerManager{Root: true}
+		// Propagate behavior hooks (e.g. ExistsFn) to the clone so tests
+		// see consistent results between the rootless and root variants.
+		m.RootClone = &MockContainerManager{
+			Root:     true,
+			ExistsFn: m.ExistsFn,
+		}
 	}
 	return m.RootClone
 }
@@ -71,6 +81,9 @@ func (m *MockContainerManager) Remove(_ context.Context, containerName string, o
 
 func (m *MockContainerManager) Exists(_ context.Context, containerName string) bool {
 	m.Spy.Exists = append(m.Spy.Exists, []any{containerName})
+	if m.ExistsFn != nil {
+		return m.ExistsFn(containerName)
+	}
 	return false
 }