From b9b6079af030b2b49e953faebdf0018ef3aaf1c4 Mon Sep 17 00:00:00 2001 From: Henry Date: Mon, 24 Aug 2026 13:55:06 +0000 Subject: [PATCH] fix(office): make idle-skip gate actually skip periodic routine wakeups (#2973) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> --- .../internal/office/routines/service.go | 4 +- .../internal/office/routines/service_test.go | 9 +- .../routines/wo46_run_reason_drift_test.go | 107 ++++++++++++++++ apps/backend/internal/office/scheduler/run.go | 10 +- apps/backend/internal/office/service/run.go | 10 +- .../office/service/scheduler_integration.go | 7 +- .../wo46_idle_skip_routine_dispatch_test.go | 115 ++++++++++++++++++ .../internal/office/shared/runreasons.go | 58 +++++++++ 8 files changed, 307 insertions(+), 13 deletions(-) create mode 100644 apps/backend/internal/office/routines/wo46_run_reason_drift_test.go create mode 100644 apps/backend/internal/office/service/wo46_idle_skip_routine_dispatch_test.go create mode 100644 apps/backend/internal/office/shared/runreasons.go diff --git a/apps/backend/internal/office/routines/service.go b/apps/backend/internal/office/routines/service.go index cae286413e..6c616bde7c 100644 --- a/apps/backend/internal/office/routines/service.go +++ b/apps/backend/internal/office/routines/service.go @@ -394,7 +394,7 @@ func (s *RoutineService) processCronTrigger(ctx context.Context, trigger *Routin // runCount-1 as missed_ticks ("you missed N since the last fire"). missedForPayload = runCount - 1 } - _, err = s.DispatchRoutineRunWithMissed(ctx, routine, trigger, "cron", nil, missedForPayload) + _, err = s.DispatchRoutineRunWithMissed(ctx, routine, trigger, shared.RoutineSourceCron, nil, missedForPayload) return err } @@ -621,7 +621,7 @@ func (s *RoutineService) materialiseLightweightRoutineRun( ID: uuid.New().String(), AgentProfileID: routine.AssigneeAgentProfileID, Source: "routine", - Reason: "routine_dispatch", + Reason: shared.RoutineDispatchReason(run.Source), Payload: payloadStr, IdempotencyKey: idemKey, RequestedAt: time.Now().UTC(), diff --git a/apps/backend/internal/office/routines/service_test.go b/apps/backend/internal/office/routines/service_test.go index c5fea1629d..b42ba840ae 100644 --- a/apps/backend/internal/office/routines/service_test.go +++ b/apps/backend/internal/office/routines/service_test.go @@ -12,6 +12,7 @@ import ( "github.com/kandev/kandev/internal/office/models" "github.com/kandev/kandev/internal/office/repository/sqlite" "github.com/kandev/kandev/internal/office/routines" + "github.com/kandev/kandev/internal/office/shared" taskservice "github.com/kandev/kandev/internal/task/service" ) @@ -265,8 +266,12 @@ func TestDispatch_LightweightRoutine_EnqueuesWakeup(t *testing.T) { if got.Source != "routine" { t.Errorf("source = %q, want routine", got.Source) } - if got.Reason != "routine_dispatch" { - t.Errorf("reason = %q, want routine_dispatch", got.Reason) + // FireManual is event/user-triggered, not periodic (WO-46 Review round + // 1, Blocker 1): it must get RunReasonRoutineDispatchEvent, never the + // cron-only RunReasonRoutineDispatch the idle-skip gate treats as a + // skippable periodic wake. + if got.Reason != shared.RunReasonRoutineDispatchEvent { + t.Errorf("reason = %q, want %q", got.Reason, shared.RunReasonRoutineDispatchEvent) } if !strings.Contains(got.Payload, `"routine_id":"`+routine.ID+`"`) { t.Errorf("payload missing routine_id, got %q", got.Payload) diff --git a/apps/backend/internal/office/routines/wo46_run_reason_drift_test.go b/apps/backend/internal/office/routines/wo46_run_reason_drift_test.go new file mode 100644 index 0000000000..6bb2b6338b --- /dev/null +++ b/apps/backend/internal/office/routines/wo46_run_reason_drift_test.go @@ -0,0 +1,107 @@ +package routines_test + +import ( + "context" + "testing" + "time" + + "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/office/routines" + "github.com/kandev/kandev/internal/office/shared" +) + +// newLightweightTestRoutine builds an active, taskless routine assigned +// to agent-1. Shared setup for both halves of the WO-46 anti-drift lock +// below. +func newLightweightTestRoutine(t *testing.T, svc *routines.RoutineService) *models.Routine { + t.Helper() + routine := &models.Routine{ + WorkspaceID: "ws-1", + Name: "Lightweight", + TaskTemplate: "", // lightweight + AssigneeAgentProfileID: "agent-1", + Status: "active", + ConcurrencyPolicy: "always_create", + } + if err := svc.CreateRoutine(context.Background(), routine); err != nil { + t.Fatalf("create routine: %v", err) + } + return routine +} + +// TestDispatch_LightweightRoutine_ManualFireIsNotPeriodicTasklessWake is +// half of the WO-46 anti-drift lock (Review round 1, Blocker 2). A manual +// "Fire now" is event/user-triggered, not periodic — per +// docs/specs/office/scheduler.md ("Event-triggered wakeups always +// proceed - the skip applies only to periodic wakes") the idle-skip gate +// must never swallow it. Drives the reason through the real writer +// (materialiseLightweightRoutineRun via FireManual) so a future change +// that makes FireManual periodic-tagged again fails here first — this is +// the case Review round 1 found: the original version of this test +// asserted the opposite of what production behavior should be. +func TestDispatch_LightweightRoutine_ManualFireIsNotPeriodicTasklessWake(t *testing.T) { + svc := newTestRoutineService(t) + ctx := context.Background() + + routine := newLightweightTestRoutine(t, svc) + + enq := &fakeWakeupEnqueuer{} + svc.SetWakeupEnqueuer(enq) + + if _, err := svc.FireManual(ctx, routine.ID, map[string]string{"name": "alpha"}); err != nil { + t.Fatalf("fire manual: %v", err) + } + if len(enq.created) != 1 { + t.Fatalf("expected 1 wakeup-request created, got %d", len(enq.created)) + } + + got := enq.created[0].Reason + if shared.IsPeriodicTasklessWake(got) { + t.Errorf("shared.IsPeriodicTasklessWake(%q) = true, want false — "+ + "a manual Fire-now would be silently swallowed by the idle-skip "+ + "gate for any SkipIdleRuns assignee with no actionable tasks", got) + } +} + +// TestDispatch_LightweightRoutine_CronFireIsPeriodicTasklessWake is the +// other half of the WO-46 anti-drift lock. A cron-driven fire is the +// only routine trigger that represents a periodic, unattended wake — +// the class the idle-skip gate exists to catch. Drives the reason +// through the real cron path (TickScheduledTriggers → +// materialiseLightweightRoutineRun) rather than asserting against a +// copied literal, so the writer and shared.IsPeriodicTasklessWake +// cannot silently diverge again the way RunReasonHeartbeat did. +func TestDispatch_LightweightRoutine_CronFireIsPeriodicTasklessWake(t *testing.T) { + svc := newTestRoutineService(t) + ctx := context.Background() + + routine := newLightweightTestRoutine(t, svc) + if err := svc.CreateRoutineTrigger(ctx, &models.RoutineTrigger{ + RoutineID: routine.ID, + Kind: "cron", + CronExpression: "* * * * *", + Timezone: "UTC", + Enabled: true, + }); err != nil { + t.Fatalf("create trigger: %v", err) + } + + enq := &fakeWakeupEnqueuer{} + svc.SetWakeupEnqueuer(enq) + + // The trigger's next_run_at is the next full minute after creation; + // ticking 2 minutes out guarantees it is due. + if err := svc.TickScheduledTriggers(ctx, time.Now().UTC().Add(2*time.Minute)); err != nil { + t.Fatalf("tick scheduled triggers: %v", err) + } + if len(enq.created) != 1 { + t.Fatalf("expected 1 wakeup-request created, got %d", len(enq.created)) + } + + got := enq.created[0].Reason + if !shared.IsPeriodicTasklessWake(got) { + t.Errorf("shared.IsPeriodicTasklessWake(%q) = false, want true — "+ + "the idle-skip gate would silently stop recognizing production "+ + "cron-triggered routine-dispatch runs", got) + } +} diff --git a/apps/backend/internal/office/scheduler/run.go b/apps/backend/internal/office/scheduler/run.go index 03df134440..bd99e8a724 100644 --- a/apps/backend/internal/office/scheduler/run.go +++ b/apps/backend/internal/office/scheduler/run.go @@ -19,6 +19,7 @@ import ( "github.com/kandev/kandev/internal/office/repository/sqlite" "github.com/kandev/kandev/internal/office/routing" "github.com/kandev/kandev/internal/office/service" + "github.com/kandev/kandev/internal/office/shared" ) // ErrRoutingNotSupported is returned by TaskStarter.StartTaskWithRoute @@ -61,9 +62,12 @@ const ( RunReasonTaskChildrenCompleted = "task_children_completed" RunReasonApprovalResolved = "approval_resolved" RunReasonRoutineTrigger = "routine_trigger" - RunReasonHeartbeat = "heartbeat" - RunReasonBudgetAlert = "budget_alert" - RunReasonAgentError = "agent_error" + // RunReasonHeartbeat aliases shared.RunReasonHeartbeat so this package's + // local constant and the shared idle-skip classifier cannot drift apart + // the way the un-aliased pair did before WO-46 (Review round 1, S2). + RunReasonHeartbeat = shared.RunReasonHeartbeat + RunReasonBudgetAlert = "budget_alert" + RunReasonAgentError = "agent_error" // Reactivity-pipeline reasons. RunReasonTaskUnblocked = "task_unblocked" // status: blocked → not blocked diff --git a/apps/backend/internal/office/service/run.go b/apps/backend/internal/office/service/run.go index 6748060b72..56138e66bf 100644 --- a/apps/backend/internal/office/service/run.go +++ b/apps/backend/internal/office/service/run.go @@ -12,6 +12,7 @@ import ( "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/office/shared" "github.com/kandev/kandev/internal/runs/commentkeys" runsservice "github.com/kandev/kandev/internal/runs/service" ) @@ -24,9 +25,12 @@ const ( RunReasonTaskChildrenCompleted = "task_children_completed" RunReasonApprovalResolved = "approval_resolved" RunReasonRoutineTrigger = "routine_trigger" - RunReasonHeartbeat = "heartbeat" - RunReasonBudgetAlert = "budget_alert" - RunReasonAgentError = "agent_error" + // RunReasonHeartbeat aliases shared.RunReasonHeartbeat so this package's + // local constant and the shared idle-skip classifier cannot drift apart + // the way the un-aliased pair did before WO-46 (Review round 1, S2). + RunReasonHeartbeat = shared.RunReasonHeartbeat + RunReasonBudgetAlert = "budget_alert" + RunReasonAgentError = "agent_error" ) // These reasons were persisted by earlier workflow templates. Keep them diff --git a/apps/backend/internal/office/service/scheduler_integration.go b/apps/backend/internal/office/service/scheduler_integration.go index 58e434d7ac..1cc5abad37 100644 --- a/apps/backend/internal/office/service/scheduler_integration.go +++ b/apps/backend/internal/office/service/scheduler_integration.go @@ -11,6 +11,7 @@ import ( "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/office/models" officeruntime "github.com/kandev/kandev/internal/office/runtime" + "github.com/kandev/kandev/internal/office/shared" v1 "github.com/kandev/kandev/pkg/api/v1" ) @@ -766,12 +767,12 @@ func isAgentActive(status models.AgentStatus) bool { } // checkIdleSkip returns true if the run should be skipped because the agent -// is configured to skip idle heartbeats and has no actionable tasks assigned. -// Returns false (do not skip) on any DB error to fail open. +// is configured to skip idle periodic wakes and has no actionable tasks +// assigned. Returns false (do not skip) on any DB error to fail open. func (si *SchedulerIntegration) checkIdleSkip( ctx context.Context, run *models.Run, agent *models.AgentInstance, ) bool { - if run.Reason != RunReasonHeartbeat { + if !shared.IsPeriodicTasklessWake(run.Reason) { return false } if !agent.SkipIdleRuns { diff --git a/apps/backend/internal/office/service/wo46_idle_skip_routine_dispatch_test.go b/apps/backend/internal/office/service/wo46_idle_skip_routine_dispatch_test.go new file mode 100644 index 0000000000..88ac67b29f --- /dev/null +++ b/apps/backend/internal/office/service/wo46_idle_skip_routine_dispatch_test.go @@ -0,0 +1,115 @@ +package service_test + +import ( + "context" + "testing" + + "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/office/service" + "github.com/kandev/kandev/internal/office/shared" +) + +// TestIdleSkip_RoutineDispatchNoTasks_Skipped is the WO-46 regression test. +// Production routine dispatch (internal/office/routines) queues runs with +// reason "routine_dispatch", not RunReasonHeartbeat — checkIdleSkip must +// recognize that reason too, or the idle-skip gate is unreachable in +// production even though the existing RunReasonHeartbeat-driven tests pass. +func TestIdleSkip_RoutineDispatchNoTasks_Skipped(t *testing.T) { + mock := &mockTaskStarter{} + svc := newTestService(t, service.ServiceOptions{TaskStarter: mock}) + ctx := context.Background() + + agent := &models.AgentInstance{ + WorkspaceID: "ws-1", + Name: "idle-worker-routine-dispatch", + Role: models.AgentRoleWorker, + Status: models.AgentStatusIdle, + ExecutorPreference: `{"type":"worktree"}`, + } + if err := svc.CreateAgentInstance(ctx, agent); err != nil { + t.Fatalf("create agent: %v", err) + } + // Worker defaults to skip_idle_runs=true, no tasks assigned. + + if err := svc.QueueRun(ctx, agent.ID, shared.RunReasonRoutineDispatch, `{}`, ""); err != nil { + t.Fatalf("queue: %v", err) + } + + service.RunSchedulerTick(svc, ctx) + + // mock.callCount() == 0 is not asserted here: a taskless run never + // reaches StartTask regardless of whether the idle-skip gate fired + // correctly, so it can't discriminate a working gate from a broken one + // (WO-46 Review round 1, S1). The activity-entry and queue-drained + // checks below are what actually prove the skip happened. + + // Queue should be empty (run was finished). + next, err := svc.ClaimNextRun(ctx) + if err != nil { + t.Fatalf("claim after tick: %v", err) + } + if next != nil { + t.Error("expected queue to be empty after idle skip") + } + + // Activity log should have a run_idle_skipped entry. + entries, err := svc.ListActivity(ctx, "ws-1", 50) + if err != nil { + t.Fatalf("list activity: %v", err) + } + found := false + for _, e := range entries { + if e.Action == "run_idle_skipped" { + found = true + break + } + } + if !found { + t.Error("expected run_idle_skipped activity entry for reason=routine_dispatch") + } +} + +// TestIdleSkip_RoutineDispatch_CoordinatorNotSkipped is the complementary +// case to TestIdleSkip_RoutineDispatchNoTasks_Skipped (PR #2973 review +// round 1, github-actions suggestion): a coordinator (CEO role) defaults to +// SkipIdleRuns=false because its heartbeat purpose is self-directed and +// does not require a directly assigned task, so a routine_dispatch fire for +// one must never be idle-skipped. checkIdleSkip's guard order happens to +// check the reason before SkipIdleRuns, so this passes today, but nothing +// pinned the coordinator side of the contract — a future reorder of the +// guards in checkIdleSkip would silently break it without this test. +func TestIdleSkip_RoutineDispatch_CoordinatorNotSkipped(t *testing.T) { + mock := &mockTaskStarter{} + svc := newTestService(t, service.ServiceOptions{TaskStarter: mock}) + ctx := context.Background() + + agent := &models.AgentInstance{ + WorkspaceID: "ws-2", + Name: "coordinator-no-skip", + Role: models.AgentRoleCEO, + Status: models.AgentStatusIdle, + ExecutorPreference: `{"type":"worktree"}`, + } + if err := svc.CreateAgentInstance(ctx, agent); err != nil { + t.Fatalf("create agent: %v", err) + } + if agent.SkipIdleRuns { + t.Fatalf("CEO role should default to SkipIdleRuns=false") + } + + if err := svc.QueueRun(ctx, agent.ID, shared.RunReasonRoutineDispatch, `{}`, ""); err != nil { + t.Fatalf("queue: %v", err) + } + + service.RunSchedulerTick(svc, ctx) + + entries, err := svc.ListActivity(ctx, "ws-2", 50) + if err != nil { + t.Fatalf("list activity: %v", err) + } + for _, e := range entries { + if e.Action == "run_idle_skipped" { + t.Error("coordinator run must not be idle-skipped when SkipIdleRuns=false") + } + } +} diff --git a/apps/backend/internal/office/shared/runreasons.go b/apps/backend/internal/office/shared/runreasons.go new file mode 100644 index 0000000000..3e2b354036 --- /dev/null +++ b/apps/backend/internal/office/shared/runreasons.go @@ -0,0 +1,58 @@ +package shared + +// Run reason constants shared between routine writers (internal/office/routines) +// and scheduler readers (internal/office/service) so the two cannot silently +// drift out of sync with each other. +const ( + // RunReasonRoutineDispatch is set by RoutineService when it materializes + // a lightweight (taskless) routine wakeup triggered by its cron + // schedule — the only routine trigger that represents a periodic, + // unattended wake. See RoutineDispatchReason. + RunReasonRoutineDispatch = "routine_dispatch" + + // RunReasonRoutineDispatchEvent is set by RoutineService when a + // lightweight routine fires from a manual "Fire now" or an inbound + // webhook. Both are event/user-triggered, not periodic — per + // docs/specs/office/scheduler.md ("Event-triggered wakeups always + // proceed - the skip applies only to periodic wakes") a run carrying + // this reason must never be treated as a skippable idle wake. + RunReasonRoutineDispatchEvent = "routine_dispatch_event" + + // RunReasonHeartbeat is retired: the agent-level heartbeat cron was + // replaced by the coordinator-heartbeat routine, so no production writer + // sets it any more. Kept so any pre-retirement run row still queued is + // still recognized as a periodic taskless wake. + RunReasonHeartbeat = "heartbeat" + + // RoutineSourceCron identifies a routine fire triggered by its cron + // schedule (RoutineRun.Source). Defined here — rather than as a + // literal at the producer call site — so RoutineDispatchReason and + // its one production caller (routines.processCronTrigger) share a + // single source of truth and cannot drift apart the way + // RunReasonHeartbeat did. + RoutineSourceCron = "cron" +) + +// IsPeriodicTasklessWake reports whether reason represents a periodic, +// taskless wake — the class of run the idle-skip gate is allowed to skip +// when the agent has no actionable tasks assigned. +func IsPeriodicTasklessWake(reason string) bool { + switch reason { + case RunReasonRoutineDispatch, RunReasonHeartbeat: + return true + default: + return false + } +} + +// RoutineDispatchReason returns the run-reason value a lightweight +// routine wakeup should carry for the given RoutineRun.Source. Only a +// cron-driven fire is periodic; a manual "Fire now" or an inbound +// webhook fire is event/user-triggered and must always proceed even +// when the assignee has SkipIdleRuns set — see IsPeriodicTasklessWake. +func RoutineDispatchReason(source string) string { + if source == RoutineSourceCron { + return RunReasonRoutineDispatch + } + return RunReasonRoutineDispatchEvent +}