Fix fork name collisions and running-source restore ordering

Author: sjmiller609

lib/instances/fork.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"hash/crc32"
 	"os"
+	"path/filepath"
 	"strings"
 	"time"
 
@@ -21,7 +22,7 @@ import (
 
 // forkInstance creates a new instance by cloning a stopped or standby source
 // instance. It returns the newly created fork and the requested final target
-// state; callers apply the target state transition outside the source lock.
+// state; callers apply remaining target state transitions outside the source lock.
 func (m *manager) forkInstance(ctx context.Context, id string, req ForkInstanceRequest) (*Instance, State, error) {
 	log := logger.FromContext(ctx)
 	log.InfoContext(ctx, "forking instance", "source_instance_id", id, "fork_name", req.Name)
@@ -68,6 +69,22 @@ func (m *manager) forkInstance(ctx context.Context, id string, req ForkInstanceR
 				}
 			}
 		}
+
+		// For Firecracker running-source forks, restoring the fork may temporarily alias
+		// the source data directory. Restore the fork while source remains standby and
+		// under lock, then restore the source.
+		if forkErr == nil && targetState == StateRunning {
+			restoredFork, err := m.applyForkTargetState(ctx, forked.Id, StateRunning)
+			if err != nil {
+				forkErr = fmt.Errorf("restore forked instance before source restore: %w", err)
+				if cleanupErr := m.cleanupForkInstanceOnError(ctx, forked.Id); cleanupErr != nil {
+					forkErr = fmt.Errorf("%v; additionally failed to cleanup forked instance %s: %v", forkErr, forked.Id, cleanupErr)
+				}
+			} else {
+				forked = restoredFork
+			}
+		}
+
 		log.InfoContext(ctx, "restoring source instance after running fork", "source_instance_id", id)
 		_, restoreErr := m.restoreInstance(ctx, id)
 
@@ -193,6 +210,13 @@ func (m *manager) forkInstanceFromStoppedOrStandby(ctx context.Context, id strin
 		return nil, err
 	}
 
+	existsByMetadata, err := m.instanceNameExists(req.Name)
+	if err != nil {
+		return nil, fmt.Errorf("check instance name availability: %w", err)
+	}
+	if existsByMetadata {
+		return nil, fmt.Errorf("%w: instance name '%s' already exists", ErrAlreadyExists, req.Name)
+	}
 	if stored.NetworkEnabled {
 		exists, err := m.networkManager.NameExists(ctx, req.Name, "")
 		if err != nil {
@@ -325,6 +349,25 @@ func validateForkVolumeSafety(volumes []VolumeAttachment) error {
 	return nil
 }
 
+func (m *manager) instanceNameExists(name string) (bool, error) {
+	metaFiles, err := m.listMetadataFiles()
+	if err != nil {
+		return false, err
+	}
+
+	for _, metaFile := range metaFiles {
+		id := filepath.Base(filepath.Dir(metaFile))
+		meta, err := m.loadMetadata(id)
+		if err != nil {
+			continue
+		}
+		if meta.Name == name {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
 func resolveForkTargetState(requested State, sourceState State) (State, error) {
 	if requested == "" {
 		switch sourceState {

lib/instances/fork_test.go

@@ -159,6 +159,52 @@ func TestForkInstance_CleansUpOnTargetTransitionError(t *testing.T) {
 	assert.Equal(t, sourceID, entries[0].Name())
 }
 
+func TestForkInstanceRejectsDuplicateNameForNonNetworkedSource(t *testing.T) {
+	manager, _ := setupTestManager(t)
+	ctx := context.Background()
+
+	sourceID := "fork-duplicate-name-source"
+	require.NoError(t, manager.ensureDirectories(sourceID))
+
+	now := time.Now()
+	sourceMeta := &metadata{StoredMetadata: StoredMetadata{
+		Id:                sourceID,
+		Name:              sourceID,
+		Image:             "docker.io/library/alpine:latest",
+		CreatedAt:         now,
+		StoppedAt:         &now,
+		HypervisorType:    hypervisor.TypeCloudHypervisor,
+		HypervisorVersion: "test",
+		SocketPath:        paths.New(manager.paths.DataDir()).InstanceSocket(sourceID, "cloud-hypervisor.sock"),
+		DataDir:           paths.New(manager.paths.DataDir()).InstanceDir(sourceID),
+		VsockCID:          42,
+		VsockSocket:       paths.New(manager.paths.DataDir()).InstanceVsockSocket(sourceID),
+	}}
+	require.NoError(t, manager.saveMetadata(sourceMeta))
+
+	existingID := "fork-duplicate-name-existing"
+	require.NoError(t, manager.ensureDirectories(existingID))
+	existingMeta := &metadata{StoredMetadata: StoredMetadata{
+		Id:                existingID,
+		Name:              "duplicate-name",
+		Image:             "docker.io/library/alpine:latest",
+		CreatedAt:         now,
+		StoppedAt:         &now,
+		HypervisorType:    hypervisor.TypeCloudHypervisor,
+		HypervisorVersion: "test",
+		SocketPath:        paths.New(manager.paths.DataDir()).InstanceSocket(existingID, "cloud-hypervisor.sock"),
+		DataDir:           paths.New(manager.paths.DataDir()).InstanceDir(existingID),
+		VsockCID:          43,
+		VsockSocket:       paths.New(manager.paths.DataDir()).InstanceVsockSocket(existingID),
+	}}
+	require.NoError(t, manager.saveMetadata(existingMeta))
+
+	_, err := manager.ForkInstance(ctx, sourceID, ForkInstanceRequest{Name: "duplicate-name"})
+	require.Error(t, err)
+	assert.ErrorIs(t, err, ErrAlreadyExists)
+	assert.Contains(t, err.Error(), "already exists")
+}
+
 func TestCloneStoredMetadataForFork_DeepCopiesReferenceFields(t *testing.T) {
 	startedAt := time.Now().Add(-2 * time.Minute)
 	stoppedAt := time.Now().Add(-1 * time.Minute)