fix: stop old pods seeding skills under a newer release's version

.github/workflows/release.yml

@@ -66,6 +66,8 @@ jobs:
           context: .
           file: backend/Dockerfile
           push: true
+          build-args: |
+            VERSION=${{ steps.meta.outputs.version }}
           tags: ${{ steps.meta.outputs.tags }}
           labels: ${{ steps.meta.outputs.labels }}
 

.goreleaser.yml

@@ -38,6 +38,7 @@ builds:
       - arm64
     ldflags:
       - -s -w
+      - -X main.buildVersion={{.Version}}
 
 archives:
   - id: skillsctl

backend/Dockerfile

@@ -8,7 +8,12 @@ RUN go mod download
 
 # Build
 COPY . .
-RUN CGO_ENABLED=0 GOOS=linux go build -ldflags="-s -w" -o /skillsctl-server ./backend/cmd/server
+# VERSION stamps main.buildVersion so the server only seeds skills under the
+# release it was actually built from.
+ARG VERSION=dev
+RUN CGO_ENABLED=0 GOOS=linux go build \
+    -ldflags="-s -w -X main.buildVersion=${VERSION}" \
+    -o /skillsctl-server ./backend/cmd/server
 
 # Final image
 FROM scratch

backend/cmd/server/main.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"context"
+	"fmt"
 	"log"
 	"net/http"
 	"os"
@@ -20,6 +21,11 @@ import (
 	"github.com/nebari-dev/skillsctl/skills"
 )
 
+// buildVersion is the release this binary was built from, injected at build
+// time with -ldflags "-X main.buildVersion=<version>". Local builds leave it
+// at "dev".
+var buildVersion = "dev"
+
 func main() {
 	port := envOr("PORT", "8080")
 	dbPath := envOr("DB_PATH", "skillsctl.db")
@@ -63,12 +69,14 @@ func main() {
 	repo := sqlitestore.New(db)
 
 	// Seed default skills (idempotent - skips if version already exists)
-	appVersion := envOr("APP_VERSION", "0.0.0")
 	skillsctlUsage, err := skills.FS.ReadFile("skillsctl-usage.md")
 	if err != nil {
 		log.Fatalf("read embedded skill: %v", err)
 	}
-	if err := seed.Run(context.Background(), repo, appVersion, seed.DefaultSkills(skillsctlUsage)); err != nil {
+	seedVersion, err := resolveSeedVersion(buildVersion, os.Getenv("APP_VERSION"))
+	if err != nil {
+		log.Printf("seed: refusing to seed (%v)", err)
+	} else if err := seed.Run(context.Background(), repo, seedVersion, seed.DefaultSkills(skillsctlUsage)); err != nil {
 		log.Fatalf("seed skills: %v", err)
 	}
 
@@ -120,6 +128,31 @@ func envOr(key, fallback string) string {
 	return fallback
 }
 
+// resolveSeedVersion decides which version the embedded skills are published
+// under. Seeded content comes from this binary, so the version must identify
+// this binary: publishing under a version built from different content makes
+// that version permanently wrong, because published versions are immutable.
+//
+// A stamped binary therefore refuses to seed when APP_VERSION disagrees with
+// its own build version. That disagreement happens whenever a pod from an
+// older image reads a configmap that a newer release already updated, which is
+// exactly the window a rolling update opens.
+//
+// Unstamped builds (local development, "dev") have no version to compare
+// against and fall back to APP_VERSION.
+func resolveSeedVersion(build, appVersion string) (string, error) {
+	if build == "" || build == "dev" {
+		if appVersion == "" {
+			return "0.0.0", nil
+		}
+		return appVersion, nil
+	}
+	if appVersion != "" && appVersion != build {
+		return "", fmt.Errorf("APP_VERSION %q does not match build version %q; this binary must not publish skills as %s", appVersion, build, appVersion)
+	}
+	return build, nil
+}
+
 func isDevMode() bool {
 	v := strings.ToLower(os.Getenv("DEV_MODE"))
 	return v == "1" || v == "true" || v == "yes"

backend/cmd/server/main_test.go

@@ -30,3 +30,43 @@ func TestIsDevMode(t *testing.T) {
 		})
 	}
 }
+
+func TestResolveSeedVersion(t *testing.T) {
+	tests := []struct {
+		name       string
+		build      string
+		appVersion string
+		want       string
+		wantErr    bool
+	}{
+		{name: "stamped build, matching APP_VERSION", build: "0.2.2", appVersion: "0.2.2", want: "0.2.2"},
+		{name: "stamped build, no APP_VERSION", build: "0.2.2", appVersion: "", want: "0.2.2"},
+		{name: "stamped build, APP_VERSION ahead", build: "0.2.0", appVersion: "0.2.1", wantErr: true},
+		{name: "stamped build, APP_VERSION behind", build: "0.2.2", appVersion: "0.2.1", wantErr: true},
+		{name: "dev build falls back to APP_VERSION", build: "dev", appVersion: "9.9.9", want: "9.9.9"},
+		{name: "dev build with no APP_VERSION", build: "dev", appVersion: "", want: "0.0.0"},
+		{name: "empty build treated as unstamped", build: "", appVersion: "1.2.3", want: "1.2.3"},
+		{name: "empty build and no APP_VERSION", build: "", appVersion: "", want: "0.0.0"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := resolveSeedVersion(tt.build, tt.appVersion)
+			if tt.wantErr {
+				if err == nil {
+					t.Fatalf("resolveSeedVersion(%q, %q) = %q, want error", tt.build, tt.appVersion, got)
+				}
+				if got != "" {
+					t.Errorf("resolveSeedVersion(%q, %q) returned version %q alongside error, want empty", tt.build, tt.appVersion, got)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("resolveSeedVersion(%q, %q) unexpected error: %v", tt.build, tt.appVersion, err)
+			}
+			if got != tt.want {
+				t.Errorf("resolveSeedVersion(%q, %q) = %q, want %q", tt.build, tt.appVersion, got, tt.want)
+			}
+		})
+	}
+}

backend/internal/seed/seed.go

@@ -44,6 +44,7 @@ func Run(ctx context.Context, repo store.Repository, version string, skills []Sk
 		err := repo.CreateSkillVersion(ctx, skill, ver, s.Content)
 		if err != nil {
 			if errors.Is(err, store.ErrAlreadyExists) {
+				log.Printf("seed: %s@%s already present, skipping", s.Name, version)
 				continue
 			}
 			if errors.Is(err, store.ErrPermissionDenied) {

chart/templates/deployment.yaml

@@ -6,6 +6,16 @@ metadata:
     {{- include "skillsctl.labels" . | nindent 4 }}
 spec:
   replicas: {{ .Values.replicaCount }}
+  {{- if and .Values.persistence.enabled (ne .Values.persistence.accessMode "ReadWriteMany") }}
+  # Any access mode short of ReadWriteMany binds the volume to one node, so the
+  # default RollingUpdate deadlocks: it holds the old pod until the new one is
+  # Ready, and the new pod cannot attach until the old one releases the claim.
+  # Recreate also avoids running two image generations at once, which would let
+  # an older pod seed skills under the newer release's version. Inverted on
+  # purpose, so an unset or unexpected accessMode still gets the safe strategy.
+  strategy:
+    type: Recreate
+  {{- end }}
   selector:
     matchLabels:
       {{- include "skillsctl.selectorLabels" . | nindent 6 }}

chart/values.yaml

@@ -27,7 +27,10 @@ service:
 persistence:
   enabled: true
   storageClassName: ""  # uses cluster default if empty
-  accessMode: ReadWriteOnce  # set to ReadWriteMany for HA
+  # ReadWriteOnce forces the Deployment to strategy: Recreate, since an RWO
+  # volume cannot attach to the new pod while the old one still holds it.
+  # Upgrades therefore have a brief gap. Set to ReadWriteMany for HA.
+  accessMode: ReadWriteOnce
   size: 1Gi
   mountPath: /data