Fix https://github.com/alcounit/browser-controller/issues/8

README.md

@@ -317,6 +317,53 @@ kubectl get browserconfig default-browser-config -o yaml
 
 ---
 
+## Resource Cleanup
+
+The controller is responsible for ensuring that both the `Browser` CR and its associated Pod are always cleaned up, regardless of the failure mode. Cleanup is enforced through a **finalizer** (`browserpod.selenosis.io/finalizer`) placed on every `Browser` CR.
+
+### Mechanism
+
+Every `Browser` CR receives the finalizer on creation. The controller uses two internal primitives:
+
+- **`deletePod`** — force-deletes the Pod with `gracePeriodSeconds=0`. Ignores `NotFound` (safe to call even if Pod is already gone).
+- **`deleteBrowser`** — removes the finalizer from the `Browser` CR, then calls `Delete` on the CR. After the finalizer is removed Kubernetes completes the deletion immediately.
+
+Most failure paths follow a **two-step process** across two reconcile cycles:
+
+1. **First reconcile** — detects the failure, force-deletes the Pod, sets `Browser.status.phase=Failed` with a descriptive message.
+2. **Second reconcile** — sees `status.phase=Failed`, calls `deletePod` (no-op if already gone) and `deleteBrowser`, which removes the CR.
+
+### Cleanup Scenarios
+
+| Scenario | Pod | Browser CR |
+|---|---|---|
+| No matching `BrowserConfig` | never created | set to `Failed` → next reconcile: **deleted** |
+| `podCreationTimeout` exceeded (pod stuck `Pending` > 5 min) | force-deleted (grace=0) | set to `Failed` → next reconcile: **deleted** |
+| `PodPending` + container `Terminated` | force-deleted (grace=0) | set to `Failed` → next reconcile: **deleted** |
+| `PodPending` + container `Waiting` with non-transient reason (`CrashLoopBackOff`, `ErrImagePull`, `ImagePullBackOff`, etc.) | force-deleted (grace=0) | set to `Failed` → next reconcile: **deleted** |
+| Pod phase `Failed` | force-deleted (grace=0) | set to `Failed` → next reconcile: **deleted** |
+| `Browser.status.phase=Failed` (failed early exit — any of the above on the next reconcile) | force-deleted (grace=0) | finalizer removed → `Delete` → **deleted** |
+| Critical container (`browser` or `seleniferous`) `Terminated` while pod is `Running` | **deleted** via OwnerReference GC after CR deletion | `deleteBrowser` → finalizer removed → **deleted** |
+| `Browser` CR `DeletionTimestamp` set (external `kubectl delete`) | explicit `Delete` in `handleDeletion`, waits for pod termination | finalizer removed after pod is gone → **deleted** |
+| Pod `DeletionTimestamp` set while CR is alive | already terminating | `deleteBrowser` triggered → **deleted** |
+| Pod stuck `Terminating` beyond `podDeletionTimeout` (5 min) | force-deleted (grace=0, best-effort) | finalizer removed regardless → **deleted** |
+
+### `Browser.status.message` on Failure
+
+Every failure path writes a human-readable message to `Browser.status.message` before deletion:
+
+| Cause | Example message |
+|---|---|
+| No `BrowserConfig` | `Browser configuration not found` |
+| Creation timeout | `pod creation timeout exceeded after 5m0s, container browser: ContainerCreating` |
+| Container terminated | `pod container browser terminated: OOMKilled (exit code 137)` |
+| Container not ready | `pod container browser failed: CrashLoopBackOff - back-off restarting failed container` |
+| Pod failed | `pod has failed with reason: OOMKilled - container exceeded memory limit` |
+
+This message is available in `Browser.status` until the CR is removed and is propagated as an SSE event by `browser-service`, making it observable to clients before the CR disappears.
+
+---
+
 ## Build & Generate
 
 This project uses `make` to generate code, manifests, and build the controller image.

apis/browserconfig/v1/browser_config.go

@@ -205,7 +205,7 @@ func (b *BrowserVersionConfigSpec) mergeWithSpec(t *BrowserConfigSpec) {
 
 	b.Tolerations = mergeTolerationPtr(t.Template.Tolerations, b.Tolerations)
 	b.HostAliases = mergeHostAliasPtr(t.Template.HostAliases, b.HostAliases)
-	b.VolumeMounts = mergeVolumeMountsPtr(b.VolumeMounts, t.Template.VolumeMounts)
+	b.VolumeMounts = mergeVolumeMountsPtr(t.Template.VolumeMounts, b.VolumeMounts)
 
 	originalSidecars := b.Sidecars
 
@@ -227,7 +227,7 @@ func (b *BrowserVersionConfigSpec) mergeWithSpec(t *BrowserConfigSpec) {
 
 	originalInitContainers := b.InitContainers
 
-	b.InitContainers = mergeSidecarPtr(b.InitContainers, t.Template.InitContainers)
+	b.InitContainers = mergeSidecarPtr(t.Template.InitContainers, b.InitContainers)
 
 	if originalInitContainers != nil && t.Template.InitContainers != nil {
 		for i := range *b.InitContainers {
@@ -288,24 +288,28 @@ func mergeEnvPtr(template, override *[]corev1.EnvVar) *[]corev1.EnvVar {
 		return nil
 	}
 
-	result := map[string]corev1.EnvVar{}
+	// Build merged slice preserving template order; override vars replace template vars in-place,
+	// new override-only vars are appended at the end.
+	index := make(map[string]int) // name -> position in merged
+	merged := make([]corev1.EnvVar, 0)
+
 	if template != nil {
 		for _, env := range *template {
-			result[env.Name] = env
+			index[env.Name] = len(merged)
+			merged = append(merged, env)
 		}
 	}
 
 	if override != nil {
 		for _, env := range *override {
-			result[env.Name] = env
+			if i, exists := index[env.Name]; exists {
+				merged[i] = env
+			} else {
+				merged = append(merged, env)
+			}
 		}
 	}
 
-	merged := make([]corev1.EnvVar, 0, len(result))
-	for _, env := range result {
-		merged = append(merged, env)
-	}
-
 	return &merged
 }
 
@@ -440,7 +444,10 @@ func (s *Sidecar) mergeWithTemplate(t *Sidecar) {
 		s.Command = t.Command
 	}
 
-	s.WorkingDir = t.WorkingDir
+	if s.WorkingDir == nil {
+		s.WorkingDir = t.WorkingDir
+	}
+
 	s.Env = mergeEnvPtr(t.Env, s.Env)
 	s.Ports = mergeContainerPortPtr(t.Ports, s.Ports)
 	s.VolumeMounts = mergeVolumeMountPtr(t.VolumeMounts, s.VolumeMounts)