diff --git a/internal/daemon/vm_create.go b/internal/daemon/vm_create.go index 1ed4e85..08302c7 100644 --- a/internal/daemon/vm_create.go +++ b/internal/daemon/vm_create.go @@ -115,9 +115,12 @@ func (d *Daemon) reserveVM(ctx context.Context, requestedName string, image mode } name = generated } - if _, err := d.FindVM(ctx, name); err == nil { + // Exact-name lookup. Using FindVM here would also match a new name + // that merely prefixes some existing VM's id or another VM's name, + // falsely rejecting perfectly valid names. + if _, err := d.store.GetVMByName(ctx, name); err == nil { return model.VMRecord{}, fmt.Errorf("vm name already exists: %s", name) - } else if !errors.Is(err, sql.ErrNoRows) && !strings.Contains(err.Error(), "not found") { + } else if !errors.Is(err, sql.ErrNoRows) { return model.VMRecord{}, err } diff --git a/internal/daemon/vm_create_test.go b/internal/daemon/vm_create_test.go new file mode 100644 index 0000000..81b1fe3 --- /dev/null +++ b/internal/daemon/vm_create_test.go @@ -0,0 +1,86 @@ +package daemon + +import ( + "context" + "path/filepath" + "strings" + "testing" + + "banger/internal/model" + "banger/internal/paths" +) + +// TestReserveVMAllowsNameThatPrefixesExistingVM is a regression for a +// correctness bug in the name-uniqueness check: reserveVM used to +// route through FindVM, which falls back to prefix-matching on both +// ids and names. That meant a perfectly valid new name like "beta" +// could be rejected simply because an existing VM's id or name +// started with "beta". Exact-name lookup via store.GetVMByName fixes +// it. The test seeds a VM whose id and name are long strings, then +// tries to reserve a new VM with a name that's a prefix of each — +// both must succeed. +func TestReserveVMAllowsNameThatPrefixesExistingVM(t *testing.T) { + ctx := context.Background() + tmp := t.TempDir() + d := &Daemon{ + store: openDaemonStore(t), + layout: paths.Layout{VMsDir: filepath.Join(tmp, "vms"), RuntimeDir: filepath.Join(tmp, "runtime")}, + config: model.DaemonConfig{BridgeIP: model.DefaultBridgeIP}, + } + + existing := testVM("longname-sandbox-foobar", "image-x", "172.16.0.50") + upsertDaemonVM(t, ctx, d.store, existing) + + image := testImage("image-x") + image.ID = "image-x" + image.Name = "image-x" + if err := d.store.UpsertImage(ctx, image); err != nil { + t.Fatalf("UpsertImage: %v", err) + } + + // New VM name is a prefix of the existing id (which is + // "longname-sandbox-foobar-id" per testVM). Old FindVM-based check + // would reject this. + if vm, err := d.reserveVM(ctx, "longname", image, model.VMSpec{VCPUCount: 1, MemoryMiB: 128}); err != nil { + t.Fatalf("reserveVM(prefix of id): %v", err) + } else if vm.Name != "longname" { + t.Fatalf("reserveVM returned name=%q, want longname", vm.Name) + } + + // Prefix of the existing name ("longname-sandbox") must also work. + if vm, err := d.reserveVM(ctx, "longname-sandbox", image, model.VMSpec{VCPUCount: 1, MemoryMiB: 128}); err != nil { + t.Fatalf("reserveVM(prefix of name): %v", err) + } else if vm.Name != "longname-sandbox" { + t.Fatalf("reserveVM returned name=%q, want longname-sandbox", vm.Name) + } +} + +// TestReserveVMRejectsExactDuplicateName confirms the uniqueness +// check still catches actual collisions after the FindVM → GetVMByName +// switch. +func TestReserveVMRejectsExactDuplicateName(t *testing.T) { + ctx := context.Background() + tmp := t.TempDir() + d := &Daemon{ + store: openDaemonStore(t), + layout: paths.Layout{VMsDir: filepath.Join(tmp, "vms"), RuntimeDir: filepath.Join(tmp, "runtime")}, + config: model.DaemonConfig{BridgeIP: model.DefaultBridgeIP}, + } + existing := testVM("sandbox", "image-x", "172.16.0.51") + upsertDaemonVM(t, ctx, d.store, existing) + + image := testImage("image-x") + image.ID = "image-x" + image.Name = "image-x" + if err := d.store.UpsertImage(ctx, image); err != nil { + t.Fatalf("UpsertImage: %v", err) + } + + _, err := d.reserveVM(ctx, "sandbox", image, model.VMSpec{VCPUCount: 1, MemoryMiB: 128}) + if err == nil { + t.Fatal("reserveVM with duplicate name should have failed") + } + if !strings.Contains(err.Error(), "already exists") { + t.Fatalf("err = %v, want 'already exists'", err) + } +} diff --git a/internal/store/store.go b/internal/store/store.go index 7ddc941..f87b559 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -203,6 +203,20 @@ func (s *Store) GetVMByID(ctx context.Context, id string) (model.VMRecord, error return scanVMRow(row) } +// GetVMByName is the exact-name lookup used for creation-time +// uniqueness checks. Unlike GetVM (which matches id OR name) and +// Daemon.FindVM (which also falls back to prefix-matching), this +// returns sql.ErrNoRows for anything except a literal name hit, so +// a new VM can't be rejected just because its name prefixes an +// existing VM's id or an existing VM's name. +func (s *Store) GetVMByName(ctx context.Context, name string) (model.VMRecord, error) { + row := s.db.QueryRowContext(ctx, ` + SELECT id, name, image_id, guest_ip, state, created_at, updated_at, last_touched_at, + spec_json, runtime_json, stats_json + FROM vms WHERE name = ?`, name) + return scanVMRow(row) +} + func (s *Store) ListVMs(ctx context.Context) ([]model.VMRecord, error) { rows, err := s.db.QueryContext(ctx, ` SELECT id, name, image_id, guest_ip, state, created_at, updated_at, last_touched_at,