fix: scheduler picks next free run-number suffix to avoid ghost runs

AlexsJones · claude · AlexsJones · commit 205829a2c152 · 2026-04-05T09:43:34.000+01:00
Resolves a bug where re-enabling a PersonaPack (or otherwise recreating
a SympoziumSchedule after its AgentRun children were left behind) caused
the scheduler to silently no-op instead of creating real runs.

Root cause: the scheduler computed AgentRun names as
  {schedule}-{status.TotalRuns+1}
When the SympoziumSchedule was deleted, its `status.TotalRuns` counter
went with it — but the AgentRun resources owned by the schedule had
already been severed from their owner by earlier cleanup, so they
persisted as orphans. On recreation, TotalRuns reset to 0, the
scheduler tried to create `{schedule}-1` which already existed, the
apiserver returned AlreadyExists, the scheduler swallowed the error
and logged "Created scheduled AgentRun" anyway, incremented
TotalRuns, and advanced LastRunTime/NextRunTime. The user saw the
schedule claim it was firing but no new runs ever appeared — the
status was pointing at 14-hour-old orphan runs.

Fix:
  - New nextScheduledRunNumber() helper lists AgentRuns labelled with
    sympozium.ai/schedule=&lt;name&gt;, finds the maximum numeric suffix
    actually present, and picks max(TotalRuns+1, maxObserved+1). This
    is self-healing: if the schedule has been recreated N times, it
    still picks a unique suffix on the first attempt.
  - Added a bounded collision-retry loop (up to 100 attempts). On
    AlreadyExists it bumps the suffix and tries again, logging each
    collision. This guards against races with concurrent reconciles
    or with any other actor creating runs against the schedule's
    label namespace.
  - TotalRuns is now set to the actual suffix used (not blindly
    incremented), keeping it aligned with observable state.

Verified live: forcing a reconcile against the
platform-team-security-guardian-schedule (which had orphans at
suffixes 1..5) correctly created schedule-6 and schedule-7.

Adds 5 targeted unit tests (sympoziumschedule_collision_test.go):
  - NoExistingRuns: baseline — honours TotalRuns+1
  - RecoversFromOrphanedRuns: the regression, picks 10 past 1..9
  - HonoursHigherTotalRuns: counter wins when it's ahead of orphans
  - IgnoresOtherSchedulesRuns: label filtering works
  - IgnoresMalformedSuffixes: non-numeric names don't confuse the
    max-suffix scan

Co-Authored-By: Claude Opus 4.6 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/internal/controller/sympoziumschedule_collision_test.go b/internal/controller/sympoziumschedule_collision_test.go
@@ -0,0 +1,138 @@
+package controller
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	sympoziumv1alpha1 "github.com/sympozium-ai/sympozium/api/v1alpha1"
+)
+
+// TestNextScheduledRunNumber_NoExistingRuns: clean slate → returns
+// TotalRuns+1 as expected.
+func TestNextScheduledRunNumber_NoExistingRuns(t *testing.T) {
+	schedule := &sympoziumv1alpha1.SympoziumSchedule{
+		ObjectMeta: metav1.ObjectMeta{Name: "sched", Namespace: "default"},
+		Status:     sympoziumv1alpha1.SympoziumScheduleStatus{TotalRuns: 2},
+	}
+	r, _ := newScheduleTestReconciler(t, schedule)
+
+	n, err := r.nextScheduledRunNumber(context.Background(), schedule)
+	if err != nil {
+		t.Fatalf("nextScheduledRunNumber: %v", err)
+	}
+	if n != 3 {
+		t.Errorf("got %d, want 3 (TotalRuns+1)", n)
+	}
+}
+
+// TestNextScheduledRunNumber_RecoversFromOrphanedRuns is the regression
+// guard: simulates a schedule that was deleted and recreated, so
+// TotalRuns reset to 0 but orphan AgentRuns from the previous
+// incarnation still exist with suffixes 1..9. The scheduler must pick
+// 10, not 1.
+func TestNextScheduledRunNumber_RecoversFromOrphanedRuns(t *testing.T) {
+	schedule := &sympoziumv1alpha1.SympoziumSchedule{
+		ObjectMeta: metav1.ObjectMeta{Name: "sched", Namespace: "default"},
+		Status:     sympoziumv1alpha1.SympoziumScheduleStatus{TotalRuns: 0},
+	}
+	objs := []client.Object{schedule}
+	for i := 1; i <= 9; i++ {
+		objs = append(objs, makeOrphanRun("sched", i))
+	}
+	r, _ := newScheduleTestReconciler(t, objs...)
+
+	n, err := r.nextScheduledRunNumber(context.Background(), schedule)
+	if err != nil {
+		t.Fatalf("nextScheduledRunNumber: %v", err)
+	}
+	if n != 10 {
+		t.Errorf("got %d, want 10 (must skip orphans 1..9)", n)
+	}
+}
+
+// TestNextScheduledRunNumber_HonoursHigherTotalRuns: when TotalRuns is
+// ahead of observed orphans, use TotalRuns+1.
+func TestNextScheduledRunNumber_HonoursHigherTotalRuns(t *testing.T) {
+	schedule := &sympoziumv1alpha1.SympoziumSchedule{
+		ObjectMeta: metav1.ObjectMeta{Name: "sched", Namespace: "default"},
+		Status:     sympoziumv1alpha1.SympoziumScheduleStatus{TotalRuns: 50},
+	}
+	r, _ := newScheduleTestReconciler(t, schedule, makeOrphanRun("sched", 3))
+
+	n, err := r.nextScheduledRunNumber(context.Background(), schedule)
+	if err != nil {
+		t.Fatalf("nextScheduledRunNumber: %v", err)
+	}
+	if n != 51 {
+		t.Errorf("got %d, want 51", n)
+	}
+}
+
+// TestNextScheduledRunNumber_IgnoresOtherSchedulesRuns: only runs whose
+// label matches THIS schedule count. Runs belonging to other schedules
+// are ignored.
+func TestNextScheduledRunNumber_IgnoresOtherSchedulesRuns(t *testing.T) {
+	schedule := &sympoziumv1alpha1.SympoziumSchedule{
+		ObjectMeta: metav1.ObjectMeta{Name: "sched", Namespace: "default"},
+		Status:     sympoziumv1alpha1.SympoziumScheduleStatus{TotalRuns: 0},
+	}
+	own := makeOrphanRun("sched", 7)
+	other := makeOrphanRun("other-sched", 99)
+	r, _ := newScheduleTestReconciler(t, schedule, own, other)
+
+	n, err := r.nextScheduledRunNumber(context.Background(), schedule)
+	if err != nil {
+		t.Fatalf("nextScheduledRunNumber: %v", err)
+	}
+	if n != 8 {
+		t.Errorf("got %d, want 8 (should ignore other-sched's run)", n)
+	}
+}
+
+// TestNextScheduledRunNumber_IgnoresMalformedSuffixes: runs with
+// non-numeric suffixes are skipped.
+func TestNextScheduledRunNumber_IgnoresMalformedSuffixes(t *testing.T) {
+	schedule := &sympoziumv1alpha1.SympoziumSchedule{
+		ObjectMeta: metav1.ObjectMeta{Name: "sched", Namespace: "default"},
+		Status:     sympoziumv1alpha1.SympoziumScheduleStatus{TotalRuns: 0},
+	}
+	goodRun := makeOrphanRun("sched", 5)
+	// Same prefix, non-numeric suffix — must be skipped.
+	bogus := &sympoziumv1alpha1.AgentRun{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "sched-abc123",
+			Namespace: "default",
+			Labels:    map[string]string{"sympozium.ai/schedule": "sched"},
+		},
+		Spec: sympoziumv1alpha1.AgentRunSpec{InstanceRef: "i", Task: "x"},
+	}
+	r, _ := newScheduleTestReconciler(t, schedule, goodRun, bogus)
+
+	n, err := r.nextScheduledRunNumber(context.Background(), schedule)
+	if err != nil {
+		t.Fatalf("nextScheduledRunNumber: %v", err)
+	}
+	if n != 6 {
+		t.Errorf("got %d, want 6 (only numeric suffixes counted)", n)
+	}
+}
+
+func makeOrphanRun(scheduleName string, suffix int) *sympoziumv1alpha1.AgentRun {
+	return &sympoziumv1alpha1.AgentRun{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      fmt.Sprintf("%s-%d", scheduleName, suffix),
+			Namespace: "default",
+			Labels: map[string]string{
+				"sympozium.ai/schedule": scheduleName,
+			},
+		},
+		Spec: sympoziumv1alpha1.AgentRunSpec{
+			InstanceRef: "any-instance",
+			Task:        "x",
+		},
+	}
+}
diff --git a/internal/controller/sympoziumschedule_controller.go b/internal/controller/sympoziumschedule_controller.go
@@ -3,6 +3,8 @@ package controller
 import (
 	"context"
 	"fmt"
+	"strconv"
+	"strings"
 	"time"
 
 	"github.com/go-logr/logr"
@@ -19,6 +21,11 @@ import (
 	sympoziumv1alpha1 "github.com/sympozium-ai/sympozium/api/v1alpha1"
 )
 
+// maxScheduleRunCreateRetries bounds the collision-retry loop when the
+// scheduler's chosen run suffix clashes with an existing AgentRun (e.g. from
+// a prior incarnation of this schedule before it was deleted and recreated).
+const maxScheduleRunCreateRetries = 100
+
 const sympoziumScheduleFinalizer = "sympozium.ai/schedule-finalizer"
 
 // SympoziumScheduleReconciler reconciles SympoziumSchedule objects.
@@ -164,8 +171,20 @@ func (r *SympoziumScheduleReconciler) Reconcile(ctx context.Context, req ctrl.Re
 		return ctrl.Result{RequeueAfter: 60 * time.Second}, nil
 	}
 
+	// Pick the next available run number. The naive `TotalRuns+1` choice
+	// collides when this schedule has been deleted and recreated (e.g.
+	// PersonaPack disabled then re-enabled) because the counter resets
+	// but the old AgentRun resources persist. List existing runs that
+	// belong to this schedule, find the highest numeric suffix, and
+	// start from there.
+	nextNum, err := r.nextScheduledRunNumber(ctx, schedule)
+	if err != nil {
+		log.Error(err, "failed to list existing scheduled runs; falling back to TotalRuns counter")
+		nextNum = int(schedule.Status.TotalRuns) + 1
+	}
+
 	// Create the AgentRun.
-	runName := fmt.Sprintf("%s-%d", schedule.Name, schedule.Status.TotalRuns+1)
+	runName := fmt.Sprintf("%s-%d", schedule.Name, nextNum)
 	agentRun := &sympoziumv1alpha1.AgentRun{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      runName,
@@ -212,20 +231,40 @@ func (r *SympoziumScheduleReconciler) Reconcile(ctx context.Context, req ctrl.Re
 		agentRun.Spec.Skills = append(agentRun.Spec.Skills, skill)
 	}
 
-	if err := r.Create(ctx, agentRun); err != nil {
+	// Collision-retry loop: if another actor (or a race with our own
+	// prior reconcile) created the same name, bump the suffix and try
+	// again. Bounded to avoid an unbounded spin.
+	for attempt := 0; attempt < maxScheduleRunCreateRetries; attempt++ {
+		err := r.Create(ctx, agentRun)
+		if err == nil {
+			break
+		}
 		if !errors.IsAlreadyExists(err) {
 			log.Error(err, "failed to create AgentRun")
 			return ctrl.Result{RequeueAfter: 30 * time.Second}, nil
 		}
+		log.Info("scheduled run name collision; trying next suffix",
+			"run", runName, "attempt", attempt+1)
+		nextNum++
+		runName = fmt.Sprintf("%s-%d", schedule.Name, nextNum)
+		agentRun.ObjectMeta.Name = runName
+		if attempt == maxScheduleRunCreateRetries-1 {
+			log.Error(fmt.Errorf("exceeded collision retries"),
+				"could not pick a unique scheduled run name", "lastAttempted", runName)
+			return ctrl.Result{RequeueAfter: 60 * time.Second}, nil
+		}
 	}
 
 	log.Info("Created scheduled AgentRun", "run", runName, "type", schedule.Spec.Type)
 
-	// Update status.
+	// Update status. Use the actual number that was committed (after any
+	// collision retries) so TotalRuns stays in sync with observable state.
 	nowMeta := metav1.Now()
 	schedule.Status.LastRunTime = &nowMeta
 	schedule.Status.LastRunName = runName
-	schedule.Status.TotalRuns++
+	if int64(nextNum) > schedule.Status.TotalRuns {
+		schedule.Status.TotalRuns = int64(nextNum)
+	}
 
 	// Recompute next run from now.
 	next := sched.Next(now)
@@ -241,6 +280,50 @@ func (r *SympoziumScheduleReconciler) Reconcile(ctx context.Context, req ctrl.Re
 	return ctrl.Result{RequeueAfter: delay}, nil
 }
 
+// nextScheduledRunNumber returns the next numeric suffix to use when naming
+// a scheduled AgentRun, picking the higher of:
+//
+//   - status.TotalRuns + 1 (the counter the scheduler maintains), and
+//   - 1 + the maximum observed suffix on existing AgentRuns that belong to
+//     this schedule (identified via the sympozium.ai/schedule label).
+//
+// The second branch handles the case where this schedule was previously
+// deleted (e.g. PersonaPack disabled) and recreated: TotalRuns resets to 0
+// but orphan AgentRuns from the previous incarnation persist with names like
+// `<schedule>-1`, `<schedule>-2`, … Picking a suffix that's already in use
+// would collide silently and leave the scheduler emitting ghost "created"
+// log lines without actually creating runs.
+func (r *SympoziumScheduleReconciler) nextScheduledRunNumber(ctx context.Context, schedule *sympoziumv1alpha1.SympoziumSchedule) (int, error) {
+	base := int(schedule.Status.TotalRuns) + 1
+	var runs sympoziumv1alpha1.AgentRunList
+	if err := r.List(ctx, &runs,
+		client.InNamespace(schedule.Namespace),
+		client.MatchingLabels{"sympozium.ai/schedule": schedule.Name},
+	); err != nil {
+		return base, err
+	}
+	prefix := schedule.Name + "-"
+	maxObserved := 0
+	for i := range runs.Items {
+		name := runs.Items[i].Name
+		if !strings.HasPrefix(name, prefix) {
+			continue
+		}
+		suffix := strings.TrimPrefix(name, prefix)
+		n, err := strconv.Atoi(suffix)
+		if err != nil || n <= 0 {
+			continue
+		}
+		if n > maxObserved {
+			maxObserved = n
+		}
+	}
+	if maxObserved+1 > base {
+		return maxObserved + 1, nil
+	}
+	return base, nil
+}
+
 // readMemoryConfigMap reads the MEMORY.md content from the instance's memory
 // ConfigMap. Returns empty string if not found.
 func (r *SympoziumScheduleReconciler) readMemoryConfigMap(ctx context.Context, namespace, instanceName string) string {
