Fix all code-review findings (structure, data types, bugs)

app/application.go

@@ -18,11 +18,13 @@ import (
 
 // Application represents the main application with embedded static files
 type Application struct {
-	container     *Container
-	httpServer    *http.Server
-	tftpServer    *tftp.Server
-	staticFS      embed.FS
-	staticHandler *handlers.StaticHandlers
+	container        *Container
+	handlerContainer *handlers.Container
+	httpServer       *http.Server
+	httpErrCh        chan error
+	tftpServer       *tftp.Server
+	staticFS         embed.FS
+	staticHandler    *handlers.StaticHandlers
 }
 
 // NewApplicationWithStatic creates a new application instance with embedded static files
@@ -35,10 +37,22 @@ func NewApplicationWithStatic(staticFS embed.FS) (*Application, error) {
 	// Create static file handler
 	staticHandler := handlers.NewStaticHandlers(staticFS, container.Config.HTTP.Dir)
 
+	// Build the handler container once and reuse it for Start/GetContainer.
+	handlerContainer := &handlers.Container{
+		ServerService:   container.ServerService,
+		LeaseService:    container.LeaseService,
+		OSImageService:  container.OSImageService,
+		SyslinuxService: container.SyslinuxService,
+		IPXEService:     container.IPXEService,
+		Config:          container.Config,
+	}
+
 	return &Application{
-		container:     container,
-		staticFS:      staticFS,
-		staticHandler: staticHandler,
+		container:        container,
+		handlerContainer: handlerContainer,
+		httpErrCh:        make(chan error, 1),
+		staticFS:         staticFS,
+		staticHandler:    staticHandler,
 	}, nil
 }
 
@@ -51,18 +65,8 @@ func (a *Application) Start() error {
 	}
 	log.Printf("TFTP server started on port 69, serving from %s", a.container.Config.TFTP.Dir)
 
-	// Setup HTTP handlers with dependency injection
-	handlerContainer := &handlers.Container{
-		ServerService:   a.container.ServerService,
-		LeaseService:    a.container.LeaseService,
-		OSImageService:  a.container.OSImageService,
-		SyslinuxService: a.container.SyslinuxService,
-		IPXEService:     a.container.IPXEService,
-		Config:          a.container.Config,
-	}
-
 	// Create HTTP router with injected dependencies and static file handling
-	router := routes.SetupWithContainerAndStatic(handlerContainer, a.staticHandler)
+	router := routes.SetupWithContainerAndStatic(a.handlerContainer, a.staticHandler)
 	log.Printf("Embedded HTTP server configured")
 
 	// Create HTTP server
@@ -71,11 +75,15 @@ func (a *Application) Start() error {
 		Handler: router,
 	}
 
-	// Start HTTP server
+	// Start HTTP server. Failures are propagated via httpErrCh instead of
+	// killing the process from inside the goroutine.
 	go func() {
 		log.Printf("HTTP API server started on port %s", a.container.Config.HTTP.Port)
 		if err := a.httpServer.ListenAndServe(); err != nil && err != http.ErrServerClosed {
-			log.Fatalf("HTTP server failed: %v", err)
+			select {
+			case a.httpErrCh <- err:
+			default:
+			}
 		}
 	}()
 
@@ -84,6 +92,24 @@ func (a *Application) Start() error {
 
 // Rest of the Application methods remain the same...
 func (a *Application) Stop() error {
+	// Stop any running DHCP servers first
+	if a.container.ServerService != nil {
+		ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+		servers, err := a.container.ServerService.GetAllServers(ctx)
+		if err != nil {
+			log.Printf("Error listing DHCP servers for shutdown: %v", err)
+		} else {
+			for _, server := range servers {
+				if server.Started {
+					if err := a.container.ServerService.StopServer(ctx, server.ID); err != nil {
+						log.Printf("Error stopping DHCP server %s: %v", server.ID, err)
+					}
+				}
+			}
+		}
+		cancel()
+	}
+
 	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
 	defer cancel()
 
@@ -113,8 +139,17 @@ func (a *Application) Run() error {
 	signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM)
 
 	log.Println("Application started. Press Ctrl+C to stop.")
-	<-quit
-	log.Println("Shutting down application...")
+	select {
+	case <-quit:
+		log.Println("Shutting down application...")
+	case err := <-a.httpErrCh:
+		// The HTTP server died on its own; shut everything else down and
+		// report the failure instead of log.Fatalf-ing from a goroutine.
+		if stopErr := a.Stop(); stopErr != nil {
+			log.Printf("Error during shutdown after HTTP failure: %v", stopErr)
+		}
+		return fmt.Errorf("HTTP server failed: %w", err)
+	}
 
 	if err := a.Stop(); err != nil {
 		return fmt.Errorf("failed to stop application: %w", err)
@@ -124,14 +159,7 @@ func (a *Application) Run() error {
 	return nil
 }
 
-// GetContainer returns the application's container for access to services
+// GetContainer returns the application's shared handler container
 func (a *Application) GetContainer() *handlers.Container {
-	return &handlers.Container{
-		ServerService:   a.container.ServerService,
-		LeaseService:    a.container.LeaseService,
-		OSImageService:  a.container.OSImageService,
-		SyslinuxService: a.container.SyslinuxService,
-		IPXEService:     a.container.IPXEService,
-		Config:          a.container.Config,
-	}
+	return a.handlerContainer
 }

app/container.go

@@ -27,7 +27,7 @@ type Container struct {
 }
 
 // NewContainer creates and wires up all dependencies
-func NewContainer() (*Container, error) {
+func NewContainer() (c *Container, err error) {
 	// Load configuration
 	cfg, err := config.LoadDefault()
 	if err != nil {
@@ -39,13 +39,19 @@ func NewContainer() (*Container, error) {
 	if err != nil {
 		return nil, fmt.Errorf("failed to initialize database: %w", err)
 	}
+	// Don't leak the database if a later construction step fails.
+	defer func() {
+		if err != nil {
+			_ = database.Close()
+		}
+	}()
 
 	// Create repositories
 	serverRepo := dhcp.NewBoltServerRepository(database, cfg.DB.Bucket+"_servers")
 	leaseRepo := dhcp.NewBoltLeaseRepository(database, cfg.DB.Bucket+"_leases")
 	osImageRepo := osimage.NewOSImageRepository(database)
 	downloadStatusRepo := osimage.NewDownloadStatusRepository(database)
-	syslinuxRepo, err := syslinux.NewBoltRepository(database.GetDB())
+	syslinuxRepo, err := syslinux.NewBoltRepository(database)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create syslinux repository: %w", err)
 	}
@@ -57,7 +63,7 @@ func NewContainer() (*Container, error) {
 	syslinuxService := syslinux.NewService(syslinuxRepo, syslinux.GetDefaultConfig())
 	ipxeService := ipxe.NewService(cfg, osImageService)
 
-	return &Container{
+	c = &Container{
 		Config:             cfg,
 		Database:           database,
 		ServerRepo:         serverRepo,
@@ -70,7 +76,8 @@ func NewContainer() (*Container, error) {
 		SyslinuxRepo:       syslinuxRepo,
 		SyslinuxService:    syslinuxService,
 		IPXEService:        ipxeService,
-	}, nil
+	}
+	return c, nil
 }
 
 // Close closes all resources held by the container

testdata/cli.go → cli/cli.go

@@ -1,4 +1,4 @@
-package testdata
+package cli
 
 import (
 	"context"

testdata/cli_test.go → cli/cli_test.go

@@ -1,4 +1,4 @@
-package testdata
+package cli
 
 import (
 	"flag"

testdata/mock.go → cli/mock.go

@@ -1,4 +1,4 @@
-package testdata
+package cli
 
 import (
 	"context"

config/config.go

@@ -3,6 +3,7 @@ package config
 import (
 	"fmt"
 	"os"
+	"strconv"
 )
 
 // Config represents the application configuration with immutable design
@@ -13,6 +14,10 @@ type Config struct {
 	HTTP      HTTPConfig
 	Provision ProvisionConfig
 	OSImages  OSImageConfig
+	// ServerIP overrides the IP address advertised to network-boot clients.
+	// Empty means auto-detect; can also be set via the SERVER_IP environment
+	// variable, which takes precedence.
+	ServerIP string
 }
 
 type DBConfig struct {
@@ -51,9 +56,10 @@ type OSDefinition struct {
 }
 
 type OSVersion struct {
-	DisplayName   string   `json:"display_name"`
-	BaseURL       string   `json:"base_url"`
-	Architectures []string `json:"architectures"`
+	DisplayName      string   `json:"display_name"`
+	BaseURL          string   `json:"base_url"`
+	Architectures    []string `json:"architectures"`
+	ExpectedChecksum string   `json:"expected_checksum,omitempty"` // Optional SHA256 hex digest; when set, each downloaded file for this version must match it
 }
 
 // ConfigBuilder provides a builder pattern for configuration
@@ -85,6 +91,7 @@ func NewConfigBuilder() *ConfigBuilder {
 				Dir: getEnv("PROV_DIR", "./public/provision"),
 			},
 			OSImages: getDefaultOSImageConfig(),
+			ServerIP: getEnv("SERVER_IP", ""),
 		},
 	}
 }
@@ -113,11 +120,38 @@ func (cb *ConfigBuilder) Build() (*Config, error) {
 		return nil, fmt.Errorf("invalid configuration: %w", err)
 	}
 
-	// Return a copy to maintain immutability
+	// Return a copy to maintain immutability. The OS image catalog holds
+	// nested maps and slices, so it needs a deep copy.
 	config := cb.config
+	config.OSImages = deepCopyOSImageConfig(cb.config.OSImages)
 	return &config, nil
 }
 
+// deepCopyOSImageConfig deep-copies the OS image catalog: sources, nested
+// version maps, and architecture slices.
+func deepCopyOSImageConfig(src OSImageConfig) OSImageConfig {
+	dst := OSImageConfig{
+		Sources: make(map[string]OSDefinition, len(src.Sources)),
+	}
+	for osName, def := range src.Sources {
+		defCopy := OSDefinition{
+			DisplayName: def.DisplayName,
+			KernelFile:  def.KernelFile,
+			InitrdFile:  def.InitrdFile,
+			Versions:    make(map[string]OSVersion, len(def.Versions)),
+		}
+		for ver, v := range def.Versions {
+			vCopy := v
+			if v.Architectures != nil {
+				vCopy.Architectures = append([]string(nil), v.Architectures...)
+			}
+			defCopy.Versions[ver] = vCopy
+		}
+		dst.Sources[osName] = defCopy
+	}
+	return dst
+}
+
 // validate ensures the configuration is valid
 func (cb *ConfigBuilder) validate() error {
 	if cb.config.DB.DBPath == "" {
@@ -129,6 +163,18 @@ func (cb *ConfigBuilder) validate() error {
 	if cb.config.DB.Bucket == "" {
 		return fmt.Errorf("database bucket cannot be empty")
 	}
+	if port, err := strconv.Atoi(cb.config.HTTP.Port); err != nil || port < 1 || port > 65535 {
+		return fmt.Errorf("HTTP port must be a number between 1 and 65535, got %q", cb.config.HTTP.Port)
+	}
+	if cb.config.TFTP.Dir == "" {
+		return fmt.Errorf("TFTP directory cannot be empty")
+	}
+	if cb.config.HTTP.Dir == "" {
+		return fmt.Errorf("HTTP directory cannot be empty")
+	}
+	if cb.config.Provision.Dir == "" {
+		return fmt.Errorf("provision directory cannot be empty")
+	}
 	return nil
 }
 

config/config_test.go

@@ -31,3 +31,82 @@ func TestConfigBuilder(t *testing.T) {
 	assert.Equal(t, "test.db", cfg.DB.DBFile)
 	assert.Equal(t, "test", cfg.DB.Bucket)
 }
+
+func TestConfigBuilder_InvalidHTTPPort(t *testing.T) {
+	for _, port := range []string{"", "abc", "0", "65536", "-1", "80.5"} {
+		b := NewConfigBuilder()
+		b.config.HTTP.Port = port
+		_, err := b.Build()
+		assert.Error(t, err, "port %q should be rejected", port)
+	}
+}
+
+func TestConfigBuilder_ValidHTTPPort(t *testing.T) {
+	for _, port := range []string{"1", "80", "8080", "65535"} {
+		b := NewConfigBuilder()
+		b.config.HTTP.Port = port
+		cfg, err := b.Build()
+		assert.NoError(t, err, "port %q should be accepted", port)
+		assert.Equal(t, port, cfg.HTTP.Port)
+	}
+}
+
+func TestConfigBuilder_EmptyDirsRejected(t *testing.T) {
+	b := NewConfigBuilder()
+	b.config.TFTP.Dir = ""
+	_, err := b.Build()
+	assert.Error(t, err)
+
+	b = NewConfigBuilder()
+	b.config.HTTP.Dir = ""
+	_, err = b.Build()
+	assert.Error(t, err)
+
+	b = NewConfigBuilder()
+	b.config.Provision.Dir = ""
+	_, err = b.Build()
+	assert.Error(t, err)
+}
+
+func TestConfigBuilder_DeepCopyOSImages(t *testing.T) {
+	b := NewConfigBuilder()
+	cfg, err := b.Build()
+	assert.NoError(t, err)
+
+	// Mutating the built config's catalog must not affect the builder's
+	// internal catalog (or a subsequently built config).
+	cfg.OSImages.Sources["ubuntu"].Versions["22.04"] = OSVersion{DisplayName: "mutated"}
+	archs := cfg.OSImages.Sources["ubuntu"].Versions["20.04"].Architectures
+	archs[0] = "mutated-arch"
+
+	cfg2, err := b.Build()
+	assert.NoError(t, err)
+	assert.Equal(t, "22.04 LTS", cfg2.OSImages.Sources["ubuntu"].Versions["22.04"].DisplayName)
+	assert.Equal(t, []string{"x86_64"}, cfg2.OSImages.Sources["ubuntu"].Versions["20.04"].Architectures)
+}
+
+func TestOSVersion_ExpectedChecksum(t *testing.T) {
+	v := OSVersion{
+		DisplayName:      "22.04 LTS",
+		BaseURL:          "https://example.com/",
+		Architectures:    []string{"x86_64"},
+		ExpectedChecksum: "abc123",
+	}
+	assert.Equal(t, "abc123", v.ExpectedChecksum)
+
+	// Optional: zero value is empty
+	var empty OSVersion
+	assert.Empty(t, empty.ExpectedChecksum)
+}
+
+func TestLoadDefault_ServerIPEnv(t *testing.T) {
+	t.Setenv("SERVER_IP", "10.11.12.13")
+	cfg, err := LoadDefault()
+	assert.NoError(t, err)
+	assert.Equal(t, "10.11.12.13", cfg.ServerIP)
+
+	t.Setenv("SERVER_IP", "")
+	cfg, err = LoadDefault()
+	assert.NoError(t, err)
+	assert.Empty(t, cfg.ServerIP)
+}