fix(office): don't fire paused or archived routines on cron, manual run, or webhook (#3535)
Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
This commit is contained in:
Henry
@@ -122,6 +122,35 @@ func (p RoutineCatchUpPolicy) Valid() bool {
|
||||
return false
|
||||
}
|
||||
|
||||
// RoutineStatus is the operator-set lifecycle state of a Routine. It gates
|
||||
// every fire path: a routine only dispatches while it holds a firing status.
|
||||
// See docs/specs/office/requirements/routine-status-gating.md.
|
||||
type RoutineStatus string
|
||||
|
||||
// Routine status values. The `Routine.Status` struct field stays a bare
|
||||
// string (no migration, no write-side validation); these constants are for
|
||||
// comparison, not storage.
|
||||
const (
|
||||
RoutineStatusActive RoutineStatus = "active"
|
||||
RoutineStatusPaused RoutineStatus = "paused"
|
||||
RoutineStatusArchived RoutineStatus = "archived"
|
||||
)
|
||||
|
||||
// String implements fmt.Stringer.
|
||||
func (s RoutineStatus) String() string { return string(s) }
|
||||
|
||||
// CanFire reports whether a routine holding this status may dispatch a run.
|
||||
// Allowlist, not denylist: only "active" and the empty string (the
|
||||
// NOT NULL DEFAULT 'active' column's "no writer set one" case) fire.
|
||||
// Comparison is byte-exact — no case folding, no trimming — so "Active" and
|
||||
// " active" do not fire, and any value outside the three declared constants
|
||||
// suppresses along with them. There is deliberately no Valid() counterpart:
|
||||
// nothing here validates a status on write, so a write-side predicate would
|
||||
// have no caller.
|
||||
func (s RoutineStatus) CanFire() bool {
|
||||
return s == RoutineStatusActive || s == ""
|
||||
}
|
||||
|
||||
// BudgetScopeType selects what a BudgetPolicy applies to.
|
||||
type BudgetScopeType string
|
||||
|
||||
|
||||
@@ -0,0 +1,30 @@
|
||||
package models
|
||||
|
||||
import "testing"
|
||||
|
||||
// TestRoutineStatusCanFire covers AC-OFFICE-ROUTINE-STATUS-001.1 through
|
||||
// -001.4: an allowlist, not a denylist, and a byte-exact comparison with no
|
||||
// case folding and no trimming.
|
||||
func TestRoutineStatusCanFire(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
status RoutineStatus
|
||||
want bool
|
||||
}{
|
||||
{"active fires", RoutineStatusActive, true},
|
||||
{"empty string fires", "", true},
|
||||
{"paused suppresses", RoutineStatusPaused, false},
|
||||
{"archived suppresses", RoutineStatusArchived, false},
|
||||
{"unrecognized value suppresses", "some_future_status", false},
|
||||
{"different case does not fire", "Active", false},
|
||||
{"leading space does not fire", " active", false},
|
||||
{"trailing space does not fire", "active ", false},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
if got := tt.status.CanFire(); got != tt.want {
|
||||
t.Errorf("RoutineStatus(%q).CanFire() = %v, want %v", string(tt.status), got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -99,6 +99,27 @@ func (r *Repository) ClaimTrigger(ctx context.Context, triggerID string, oldNext
|
||||
return rows > 0, err
|
||||
}
|
||||
|
||||
// AdvanceTriggerWithoutFiring moves a trigger's cursor by compare-and-set
|
||||
// without recording a fire. Unlike ClaimTrigger, it never touches
|
||||
// last_fired_at: that column is evidence a fire happened, and a suppressed
|
||||
// slot must leave none. The oldNextRunAt predicate gives it the same
|
||||
// concurrency guarantee as ClaimTrigger — the loser of a race changes no
|
||||
// rows and returns false, nil.
|
||||
func (r *Repository) AdvanceTriggerWithoutFiring(
|
||||
ctx context.Context, triggerID string, oldNextRunAt, newNextRunAt time.Time,
|
||||
) (bool, error) {
|
||||
res, err := r.db.ExecContext(ctx, r.db.Rebind(`
|
||||
UPDATE office_routine_triggers
|
||||
SET next_run_at = ?, updated_at = ?
|
||||
WHERE id = ? AND next_run_at = ?
|
||||
`), newNextRunAt, time.Now().UTC(), triggerID, oldNextRunAt)
|
||||
if err != nil {
|
||||
return false, err
|
||||
}
|
||||
rows, err := res.RowsAffected()
|
||||
return rows > 0, err
|
||||
}
|
||||
|
||||
// UpdateTriggerNextRun updates the next_run_at for a trigger.
|
||||
func (r *Repository) UpdateTriggerNextRun(ctx context.Context, triggerID string, nextRunAt *time.Time) error {
|
||||
_, err := r.db.ExecContext(ctx, r.db.Rebind(`
|
||||
|
||||
@@ -0,0 +1,117 @@
|
||||
package sqlite_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/kandev/kandev/internal/office/models"
|
||||
)
|
||||
|
||||
// TestAdvanceTriggerWithoutFiring_AdvancesCursor covers the repository half
|
||||
// of REQ-OFFICE-ROUTINE-STATUS-002: a suppressed slot's cursor moves without
|
||||
// stamping any fire evidence (last_fired_at stays nil, matching
|
||||
// AC-OFFICE-ROUTINE-STATUS-001.7 — this is why ClaimTrigger cannot be
|
||||
// reused for suppression).
|
||||
func TestAdvanceTriggerWithoutFiring_AdvancesCursor(t *testing.T) {
|
||||
repo := newTestRepo(t)
|
||||
ctx := context.Background()
|
||||
|
||||
routine := &models.Routine{
|
||||
WorkspaceID: "ws-1",
|
||||
Name: "Advance Test",
|
||||
TaskTemplate: "{}",
|
||||
Status: "paused",
|
||||
ConcurrencyPolicy: "skip_if_active",
|
||||
Variables: "{}",
|
||||
}
|
||||
if err := repo.CreateRoutine(ctx, routine); err != nil {
|
||||
t.Fatalf("create routine: %v", err)
|
||||
}
|
||||
|
||||
oldNext := time.Date(2026, 5, 1, 9, 0, 0, 0, time.UTC)
|
||||
trigger := &models.RoutineTrigger{
|
||||
RoutineID: routine.ID,
|
||||
Kind: "cron",
|
||||
CronExpression: "0 9 * * *",
|
||||
Timezone: "UTC",
|
||||
NextRunAt: &oldNext,
|
||||
Enabled: true,
|
||||
}
|
||||
if err := repo.CreateRoutineTrigger(ctx, trigger); err != nil {
|
||||
t.Fatalf("create trigger: %v", err)
|
||||
}
|
||||
|
||||
newNext := time.Date(2026, 5, 2, 9, 0, 0, 0, time.UTC)
|
||||
advanced, err := repo.AdvanceTriggerWithoutFiring(ctx, trigger.ID, oldNext, newNext)
|
||||
if err != nil {
|
||||
t.Fatalf("advance: %v", err)
|
||||
}
|
||||
if !advanced {
|
||||
t.Fatal("expected the compare-and-set to succeed")
|
||||
}
|
||||
|
||||
due, err := repo.GetDueTriggers(ctx, newNext.Add(time.Minute))
|
||||
if err != nil {
|
||||
t.Fatalf("get due: %v", err)
|
||||
}
|
||||
if len(due) != 1 {
|
||||
t.Fatalf("expected 1 due trigger after advance, got %d", len(due))
|
||||
}
|
||||
got := due[0]
|
||||
if got.NextRunAt == nil || !got.NextRunAt.Equal(newNext) {
|
||||
t.Errorf("next_run_at = %v, want %v", got.NextRunAt, newNext)
|
||||
}
|
||||
if got.LastFiredAt != nil {
|
||||
t.Errorf("last_fired_at = %v, want nil — suppression must not record a fire", got.LastFiredAt)
|
||||
}
|
||||
}
|
||||
|
||||
// TestAdvanceTriggerWithoutFiring_LosesRaceWhenCursorAlreadyMoved covers
|
||||
// AC-OFFICE-ROUTINE-STATUS-002.6/-002.7: the compare-and-set predicate is
|
||||
// the same shape as ClaimTrigger's, so a stale oldNextRunAt changes no rows.
|
||||
func TestAdvanceTriggerWithoutFiring_LosesRaceWhenCursorAlreadyMoved(t *testing.T) {
|
||||
repo := newTestRepo(t)
|
||||
ctx := context.Background()
|
||||
|
||||
routine := &models.Routine{
|
||||
WorkspaceID: "ws-1",
|
||||
Name: "Race Test",
|
||||
TaskTemplate: "{}",
|
||||
Status: "paused",
|
||||
ConcurrencyPolicy: "skip_if_active",
|
||||
Variables: "{}",
|
||||
}
|
||||
if err := repo.CreateRoutine(ctx, routine); err != nil {
|
||||
t.Fatalf("create routine: %v", err)
|
||||
}
|
||||
|
||||
oldNext := time.Date(2026, 5, 1, 9, 0, 0, 0, time.UTC)
|
||||
trigger := &models.RoutineTrigger{
|
||||
RoutineID: routine.ID,
|
||||
Kind: "cron",
|
||||
CronExpression: "0 9 * * *",
|
||||
Timezone: "UTC",
|
||||
NextRunAt: &oldNext,
|
||||
Enabled: true,
|
||||
}
|
||||
if err := repo.CreateRoutineTrigger(ctx, trigger); err != nil {
|
||||
t.Fatalf("create trigger: %v", err)
|
||||
}
|
||||
|
||||
newNext := time.Date(2026, 5, 2, 9, 0, 0, 0, time.UTC)
|
||||
first, err := repo.AdvanceTriggerWithoutFiring(ctx, trigger.ID, oldNext, newNext)
|
||||
if err != nil || !first {
|
||||
t.Fatalf("first advance: advanced=%v, err=%v", first, err)
|
||||
}
|
||||
|
||||
// A second evaluation racing on the stale cursor must lose: no rows
|
||||
// change, no error.
|
||||
second, err := repo.AdvanceTriggerWithoutFiring(ctx, trigger.ID, oldNext, newNext.AddDate(0, 0, 1))
|
||||
if err != nil {
|
||||
t.Fatalf("second advance err: %v", err)
|
||||
}
|
||||
if second {
|
||||
t.Error("second advance should have lost the compare-and-set")
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,170 @@
|
||||
package routines
|
||||
|
||||
import (
|
||||
"context"
|
||||
"sync"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/jmoiron/sqlx"
|
||||
_ "github.com/mattn/go-sqlite3"
|
||||
"go.uber.org/zap"
|
||||
"go.uber.org/zap/zapcore"
|
||||
"go.uber.org/zap/zaptest/observer"
|
||||
|
||||
"github.com/kandev/kandev/internal/common/logger"
|
||||
"github.com/kandev/kandev/internal/office/repository/sqlite"
|
||||
)
|
||||
|
||||
type raceNoopActivity struct{}
|
||||
|
||||
func (raceNoopActivity) LogActivity(context.Context, string, string, string, string, string, string, string) {
|
||||
}
|
||||
func (raceNoopActivity) LogActivityWithRun(
|
||||
context.Context, string, string, string, string, string, string, string, string, string,
|
||||
) {
|
||||
}
|
||||
|
||||
// newRaceTestService builds a RoutineService with SetMaxOpenConns(1), the
|
||||
// convention this codebase's other SQLite-backed race tests use to
|
||||
// serialize the DB connection while leaving Go-level goroutine scheduling
|
||||
// free to interleave.
|
||||
func newRaceTestService(t *testing.T) (*RoutineService, *observer.ObservedLogs) {
|
||||
t.Helper()
|
||||
db, err := sqlx.Open("sqlite3", ":memory:")
|
||||
if err != nil {
|
||||
t.Fatalf("open sqlite: %v", err)
|
||||
}
|
||||
db.SetMaxOpenConns(1)
|
||||
t.Cleanup(func() { _ = db.Close() })
|
||||
|
||||
repo, err := sqlite.NewWithDB(db, db, nil)
|
||||
if err != nil {
|
||||
t.Fatalf("new repo: %v", err)
|
||||
}
|
||||
core, observed := observer.New(zapcore.DebugLevel)
|
||||
log, err := logger.NewFromZap(zap.New(core))
|
||||
if err != nil {
|
||||
t.Fatalf("new logger: %v", err)
|
||||
}
|
||||
return NewRoutineService(repo, log, raceNoopActivity{}), observed
|
||||
}
|
||||
|
||||
// raceTestTrigger creates a routine plus a cron trigger with an explicit
|
||||
// past next_run_at, returning both. Using the repo directly (rather than
|
||||
// the CreateRoutineTrigger convenience, which computes next_run_at from
|
||||
// "now") gives the test a known, already-due cursor to race on.
|
||||
func raceTestTrigger(t *testing.T, svc *RoutineService, status string) (*Routine, *RoutineTrigger) {
|
||||
t.Helper()
|
||||
ctx := context.Background()
|
||||
routine := &Routine{
|
||||
WorkspaceID: "ws-1",
|
||||
Name: "Race " + status,
|
||||
TaskTemplate: "",
|
||||
AssigneeAgentProfileID: "agent-1",
|
||||
Status: status,
|
||||
ConcurrencyPolicy: "always_create",
|
||||
}
|
||||
if err := svc.CreateRoutine(ctx, routine); err != nil {
|
||||
t.Fatalf("create routine: %v", err)
|
||||
}
|
||||
oldNext := time.Date(2026, 6, 1, 12, 0, 0, 0, time.UTC)
|
||||
trigger := &RoutineTrigger{
|
||||
RoutineID: routine.ID,
|
||||
Kind: "cron",
|
||||
CronExpression: "* * * * *",
|
||||
Timezone: "UTC",
|
||||
NextRunAt: &oldNext,
|
||||
Enabled: true,
|
||||
}
|
||||
if err := svc.repo.CreateRoutineTrigger(ctx, trigger); err != nil {
|
||||
t.Fatalf("create trigger: %v", err)
|
||||
}
|
||||
return routine, trigger
|
||||
}
|
||||
|
||||
// TestProcessCronTrigger_ConcurrentSuppression_AdvancesAtMostOnceAndLogsOnce
|
||||
// covers AC-OFFICE-ROUTINE-STATUS-002.6 and Decision 11: two evaluations
|
||||
// racing on the same due slot of a non-firing routine advance the cursor
|
||||
// at most once, fire nothing, and log the suppression at most once — never
|
||||
// twice, which is what an unconditional log (rather than one gated on the
|
||||
// compare-and-set actually succeeding) would produce.
|
||||
//
|
||||
// This interleaving cannot happen through the production cron loop —
|
||||
// internal/scheduler/cron's fanOut waits on every handler goroutine before
|
||||
// its next tick, so TickScheduledTriggers passes cannot overlap — hence
|
||||
// calling the unexported processCronTrigger directly with two goroutines
|
||||
// sharing one pre-race trigger snapshot, exactly as the routine-status-
|
||||
// gating plan's suggested test coverage prescribes.
|
||||
func TestProcessCronTrigger_ConcurrentSuppression_AdvancesAtMostOnceAndLogsOnce(t *testing.T) {
|
||||
svc, observed := newRaceTestService(t)
|
||||
ctx := context.Background()
|
||||
routine, trigger := raceTestTrigger(t, svc, "paused")
|
||||
now := trigger.NextRunAt.Add(time.Minute)
|
||||
|
||||
var wg sync.WaitGroup
|
||||
for i := 0; i < 2; i++ {
|
||||
wg.Add(1)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
snapshot := *trigger
|
||||
_ = svc.processCronTrigger(ctx, &snapshot, now)
|
||||
}()
|
||||
}
|
||||
wg.Wait()
|
||||
|
||||
runs, err := svc.ListRoutineRuns(ctx, routine.ID, 10, 0)
|
||||
if err != nil {
|
||||
t.Fatalf("list runs: %v", err)
|
||||
}
|
||||
if len(runs) != 0 {
|
||||
t.Errorf("expected no run rows from a suppressed race, got %d", len(runs))
|
||||
}
|
||||
|
||||
triggers, err := svc.ListRoutineTriggers(ctx, routine.ID)
|
||||
if err != nil || len(triggers) != 1 {
|
||||
t.Fatalf("list triggers: triggers=%v err=%v", triggers, err)
|
||||
}
|
||||
if got := triggers[0].NextRunAt; got == nil || !got.After(now) {
|
||||
t.Errorf("next_run_at = %v, want a single advance to a slot after %v", got, now)
|
||||
}
|
||||
if triggers[0].LastFiredAt != nil {
|
||||
t.Errorf("last_fired_at = %v, want nil", triggers[0].LastFiredAt)
|
||||
}
|
||||
|
||||
if n := observed.FilterMessage("routine slot suppressed").Len(); n != 1 {
|
||||
t.Errorf("suppression logged %d times, want exactly 1 (Decision 11: only a successful "+
|
||||
"advance logs; the race loser must log nothing)", n)
|
||||
}
|
||||
}
|
||||
|
||||
// TestProcessCronTrigger_ConcurrentFiring_ExactlyOneFires covers
|
||||
// AC-OFFICE-ROUTINE-STATUS-002.10: two evaluations racing on the same due
|
||||
// slot of a firing routine dispatch exactly one run. Unchanged from
|
||||
// today's ClaimTrigger CAS — this test pins that the reordered control
|
||||
// flow (routine read before claim) did not regress it.
|
||||
func TestProcessCronTrigger_ConcurrentFiring_ExactlyOneFires(t *testing.T) {
|
||||
svc, _ := newRaceTestService(t)
|
||||
ctx := context.Background()
|
||||
routine, trigger := raceTestTrigger(t, svc, "active")
|
||||
now := trigger.NextRunAt.Add(time.Minute)
|
||||
|
||||
var wg sync.WaitGroup
|
||||
for i := 0; i < 2; i++ {
|
||||
wg.Add(1)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
snapshot := *trigger
|
||||
_ = svc.processCronTrigger(ctx, &snapshot, now)
|
||||
}()
|
||||
}
|
||||
wg.Wait()
|
||||
|
||||
runs, err := svc.ListRoutineRuns(ctx, routine.ID, 10, 0)
|
||||
if err != nil {
|
||||
t.Fatalf("list runs: %v", err)
|
||||
}
|
||||
if len(runs) != 1 {
|
||||
t.Errorf("expected exactly 1 run row from a firing race, got %d", len(runs))
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,444 @@
|
||||
package routines_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/jmoiron/sqlx"
|
||||
_ "github.com/mattn/go-sqlite3"
|
||||
"go.uber.org/zap"
|
||||
"go.uber.org/zap/zapcore"
|
||||
"go.uber.org/zap/zaptest/observer"
|
||||
|
||||
"github.com/kandev/kandev/internal/common/logger"
|
||||
"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"
|
||||
)
|
||||
|
||||
// newObservedRoutineService builds a RoutineService backed by in-memory
|
||||
// SQLite whose logger's entries can be inspected — used by the tests below
|
||||
// to assert the bounded ("at most once per trigger per process") log
|
||||
// entries the status-gating design requires.
|
||||
func newObservedRoutineService(t *testing.T) (*routines.RoutineService, *observer.ObservedLogs) {
|
||||
t.Helper()
|
||||
svc, observed, _ := newObservedRoutineServiceWithRepo(t)
|
||||
return svc, observed
|
||||
}
|
||||
|
||||
// newObservedRoutineServiceWithRepo is newObservedRoutineService plus the
|
||||
// backing repository, for tests that need to write a trigger row directly
|
||||
// (bypassing CreateRoutineTrigger's validation) to model data that predates
|
||||
// or otherwise bypassed that validation.
|
||||
func newObservedRoutineServiceWithRepo(
|
||||
t *testing.T,
|
||||
) (*routines.RoutineService, *observer.ObservedLogs, routines.Repository) {
|
||||
t.Helper()
|
||||
db, err := sqlx.Open("sqlite3", ":memory:")
|
||||
if err != nil {
|
||||
t.Fatalf("open sqlite: %v", err)
|
||||
}
|
||||
t.Cleanup(func() { _ = db.Close() })
|
||||
|
||||
repo, err := sqlite.NewWithDB(db, db, nil)
|
||||
if err != nil {
|
||||
t.Fatalf("new repo: %v", err)
|
||||
}
|
||||
|
||||
core, observed := observer.New(zapcore.DebugLevel)
|
||||
log, err := logger.NewFromZap(zap.New(core))
|
||||
if err != nil {
|
||||
t.Fatalf("new logger: %v", err)
|
||||
}
|
||||
return routines.NewRoutineService(repo, log, &noopActivity{}), observed, repo
|
||||
}
|
||||
|
||||
// createStatusGatedRoutine creates a routine with an every-minute cron
|
||||
// trigger and returns it plus the trigger's initially-computed
|
||||
// (auto-assigned) next_run_at.
|
||||
func createStatusGatedRoutine(
|
||||
t *testing.T, svc *routines.RoutineService, status string,
|
||||
) (*models.Routine, time.Time) {
|
||||
t.Helper()
|
||||
ctx := context.Background()
|
||||
routine := &models.Routine{
|
||||
WorkspaceID: "ws-1",
|
||||
Name: "Status Gate " + status,
|
||||
TaskTemplate: "",
|
||||
AssigneeAgentProfileID: "agent-1",
|
||||
Status: status,
|
||||
ConcurrencyPolicy: "always_create",
|
||||
}
|
||||
if err := svc.CreateRoutine(ctx, routine); err != nil {
|
||||
t.Fatalf("create routine: %v", err)
|
||||
}
|
||||
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)
|
||||
}
|
||||
triggers, err := svc.ListRoutineTriggers(ctx, routine.ID)
|
||||
if err != nil || len(triggers) != 1 {
|
||||
t.Fatalf("list triggers: triggers=%v err=%v", triggers, err)
|
||||
}
|
||||
return routine, *triggers[0].NextRunAt
|
||||
}
|
||||
|
||||
// assertSuppressed drives one tick past the trigger's due time and asserts
|
||||
// the AC-OFFICE-ROUTINE-STATUS-001 contract: no run, no wakeup, no fire
|
||||
// evidence, and the cursor moved to the first slot strictly after the tick
|
||||
// time (never banked, per REQ-002).
|
||||
func assertSuppressed(t *testing.T, svc *routines.RoutineService, routine *models.Routine, oldNext time.Time) {
|
||||
t.Helper()
|
||||
ctx := context.Background()
|
||||
enq := &fakeWakeupEnqueuer{}
|
||||
svc.SetWakeupEnqueuer(enq)
|
||||
|
||||
tickNow := oldNext.Add(2 * time.Minute)
|
||||
if err := svc.TickScheduledTriggers(ctx, tickNow); err != nil {
|
||||
t.Fatalf("tick: %v", err)
|
||||
}
|
||||
|
||||
if len(enq.created) != 0 {
|
||||
t.Errorf("expected no wakeup request, got %d", len(enq.created))
|
||||
}
|
||||
runs, err := svc.ListRoutineRuns(ctx, routine.ID, 10, 0)
|
||||
if err != nil {
|
||||
t.Fatalf("list runs: %v", err)
|
||||
}
|
||||
if len(runs) != 0 {
|
||||
t.Errorf("expected no run row for a suppressed slot, got %d", len(runs))
|
||||
}
|
||||
|
||||
triggers, err := svc.ListRoutineTriggers(ctx, routine.ID)
|
||||
if err != nil || len(triggers) != 1 {
|
||||
t.Fatalf("list triggers: triggers=%v err=%v", triggers, err)
|
||||
}
|
||||
got := triggers[0]
|
||||
if got.LastFiredAt != nil {
|
||||
t.Errorf("last_fired_at = %v, want nil — suppression must leave no fire evidence", got.LastFiredAt)
|
||||
}
|
||||
wantNext, err := shared.NextCronTime(got.CronExpression, got.Timezone, tickNow)
|
||||
if err != nil {
|
||||
t.Fatalf("compute expected next: %v", err)
|
||||
}
|
||||
if got.NextRunAt == nil || !got.NextRunAt.Equal(wantNext) {
|
||||
t.Errorf("next_run_at = %v, want %v (first slot strictly after the tick, not banked)", got.NextRunAt, wantNext)
|
||||
}
|
||||
}
|
||||
|
||||
func TestTickScheduledTriggers_PausedRoutineSuppresses(t *testing.T) {
|
||||
svc, _ := newObservedRoutineService(t)
|
||||
routine, oldNext := createStatusGatedRoutine(t, svc, "paused")
|
||||
assertSuppressed(t, svc, routine, oldNext)
|
||||
}
|
||||
|
||||
func TestTickScheduledTriggers_ArchivedRoutineSuppresses(t *testing.T) {
|
||||
svc, _ := newObservedRoutineService(t)
|
||||
routine, oldNext := createStatusGatedRoutine(t, svc, "archived")
|
||||
assertSuppressed(t, svc, routine, oldNext)
|
||||
}
|
||||
|
||||
func TestTickScheduledTriggers_UnrecognizedStatusSuppresses(t *testing.T) {
|
||||
svc, _ := newObservedRoutineService(t)
|
||||
routine, oldNext := createStatusGatedRoutine(t, svc, "on_hold")
|
||||
assertSuppressed(t, svc, routine, oldNext)
|
||||
}
|
||||
|
||||
// TestTickScheduledTriggers_EmptyStatusFires covers
|
||||
// AC-OFFICE-ROUTINE-STATUS-001.4 and -006.5: an empty stored status means
|
||||
// "no writer set one" and must keep firing exactly as "active" does.
|
||||
func TestTickScheduledTriggers_EmptyStatusFires(t *testing.T) {
|
||||
svc, _ := newObservedRoutineService(t)
|
||||
routine, oldNext := createStatusGatedRoutine(t, svc, "")
|
||||
ctx := context.Background()
|
||||
enq := &fakeWakeupEnqueuer{}
|
||||
svc.SetWakeupEnqueuer(enq)
|
||||
|
||||
if err := svc.TickScheduledTriggers(ctx, oldNext.Add(2*time.Minute)); err != nil {
|
||||
t.Fatalf("tick: %v", err)
|
||||
}
|
||||
if len(enq.created) != 1 {
|
||||
t.Fatalf("expected 1 wakeup request for an empty-status routine, got %d", len(enq.created))
|
||||
}
|
||||
runs, err := svc.ListRoutineRuns(ctx, routine.ID, 10, 0)
|
||||
if err != nil {
|
||||
t.Fatalf("list runs: %v", err)
|
||||
}
|
||||
if len(runs) != 1 {
|
||||
t.Fatalf("expected 1 run row, got %d", len(runs))
|
||||
}
|
||||
}
|
||||
|
||||
// TestTickScheduledTriggers_ResumeAfterSuppression_ReportsZeroMissedTicks
|
||||
// covers AC-OFFICE-ROUTINE-STATUS-002.2/-002.3/-002.4: a routine paused
|
||||
// through an entire suppression window (here, one evaluation collapsing a
|
||||
// ~10-slot backlog in a single tick, simulating an outage inside the pause)
|
||||
// resumes at the cursor the suppression left — strictly in the future — and
|
||||
// its first fire reports zero missed ticks, never the backlog the
|
||||
// suppression window spanned. The payload only carries "missed_ticks" when
|
||||
// it is greater than zero (marshalRoutinePayload), so its absence here is
|
||||
// the assertion.
|
||||
func TestTickScheduledTriggers_ResumeAfterSuppression_ReportsZeroMissedTicks(t *testing.T) {
|
||||
svc, _ := newObservedRoutineService(t)
|
||||
routine, oldNext := createStatusGatedRoutine(t, svc, "paused")
|
||||
ctx := context.Background()
|
||||
enq := &fakeWakeupEnqueuer{}
|
||||
svc.SetWakeupEnqueuer(enq)
|
||||
|
||||
// One suppressing evaluation, ten slots (minutes) after the original
|
||||
// due time: the outage-inside-a-pause case in AC-002.4.
|
||||
suppressAt := oldNext.Add(10 * time.Minute)
|
||||
if err := svc.TickScheduledTriggers(ctx, suppressAt); err != nil {
|
||||
t.Fatalf("suppressing tick: %v", err)
|
||||
}
|
||||
triggers, err := svc.ListRoutineTriggers(ctx, routine.ID)
|
||||
if err != nil || len(triggers) != 1 {
|
||||
t.Fatalf("list triggers: triggers=%v err=%v", triggers, err)
|
||||
}
|
||||
resumeCursor := *triggers[0].NextRunAt
|
||||
if !resumeCursor.After(suppressAt) {
|
||||
t.Fatalf("cursor = %v, want strictly after the suppressing evaluation %v", resumeCursor, suppressAt)
|
||||
}
|
||||
|
||||
routine.Status = "active"
|
||||
if err := svc.UpdateRoutine(ctx, routine); err != nil {
|
||||
t.Fatalf("resume routine: %v", err)
|
||||
}
|
||||
|
||||
// Tick exactly at the cursor the suppression left: the trigger is due
|
||||
// and every intervening slot is in the future relative to the pause, so
|
||||
// per -002.3 the fixed-set-of-cursors-in-the-future condition holds.
|
||||
if err := svc.TickScheduledTriggers(ctx, resumeCursor); err != nil {
|
||||
t.Fatalf("resuming tick: %v", err)
|
||||
}
|
||||
if len(enq.created) != 1 {
|
||||
t.Fatalf("expected exactly 1 wakeup request on resume, got %d", len(enq.created))
|
||||
}
|
||||
if strings.Contains(enq.created[0].Payload, "missed_ticks") {
|
||||
t.Errorf("payload = %q, must not carry missed_ticks (want 0 missed, absent from payload)",
|
||||
enq.created[0].Payload)
|
||||
}
|
||||
runs, err := svc.ListRoutineRuns(ctx, routine.ID, 10, 0)
|
||||
if err != nil {
|
||||
t.Fatalf("list runs: %v", err)
|
||||
}
|
||||
if len(runs) != 1 {
|
||||
t.Fatalf("expected exactly 1 run row on resume, got %d", len(runs))
|
||||
}
|
||||
}
|
||||
|
||||
// TestTickScheduledTriggers_NonUTCScheduleSuppressionAdvancesCursor is
|
||||
// Decision 8's regression: the match check must convert into the trigger's
|
||||
// timezone before comparing, or every non-UTC schedule freezes its cursor
|
||||
// on every suppressed slot. A daily 9am America/New_York cron is ~13:00
|
||||
// UTC; comparing the UTC instant against the spec's wall-clock "9" would
|
||||
// never match.
|
||||
func TestTickScheduledTriggers_NonUTCScheduleSuppressionAdvancesCursor(t *testing.T) {
|
||||
svc, observed := newObservedRoutineService(t)
|
||||
ctx := context.Background()
|
||||
|
||||
routine := &models.Routine{
|
||||
WorkspaceID: "ws-1",
|
||||
Name: "Non-UTC Paused",
|
||||
TaskTemplate: "",
|
||||
AssigneeAgentProfileID: "agent-1",
|
||||
Status: "paused",
|
||||
ConcurrencyPolicy: "always_create",
|
||||
}
|
||||
if err := svc.CreateRoutine(ctx, routine); err != nil {
|
||||
t.Fatalf("create routine: %v", err)
|
||||
}
|
||||
if err := svc.CreateRoutineTrigger(ctx, &models.RoutineTrigger{
|
||||
RoutineID: routine.ID,
|
||||
Kind: "cron",
|
||||
CronExpression: "0 9 * * *",
|
||||
Timezone: "America/New_York",
|
||||
Enabled: true,
|
||||
}); err != nil {
|
||||
t.Fatalf("create trigger: %v", err)
|
||||
}
|
||||
triggers, err := svc.ListRoutineTriggers(ctx, routine.ID)
|
||||
if err != nil || len(triggers) != 1 {
|
||||
t.Fatalf("list triggers: triggers=%v err=%v", triggers, err)
|
||||
}
|
||||
oldNext := *triggers[0].NextRunAt
|
||||
|
||||
tickNow := oldNext.Add(time.Minute)
|
||||
if err := svc.TickScheduledTriggers(ctx, tickNow); err != nil {
|
||||
t.Fatalf("tick: %v", err)
|
||||
}
|
||||
|
||||
wantNext, err := shared.NextCronTime("0 9 * * *", "America/New_York", tickNow)
|
||||
if err != nil {
|
||||
t.Fatalf("compute expected next: %v", err)
|
||||
}
|
||||
triggers, err = svc.ListRoutineTriggers(ctx, routine.ID)
|
||||
if err != nil || len(triggers) != 1 {
|
||||
t.Fatalf("list triggers: triggers=%v err=%v", triggers, err)
|
||||
}
|
||||
got := triggers[0].NextRunAt
|
||||
if got == nil || got.Equal(oldNext) {
|
||||
t.Fatalf("next_run_at did not advance (got %v, old %v) — the match check is rejecting a "+
|
||||
"valid non-UTC slot, freezing the cursor", got, oldNext)
|
||||
}
|
||||
if !got.Equal(wantNext) {
|
||||
t.Errorf("next_run_at = %v, want %v", got, wantNext)
|
||||
}
|
||||
if n := observed.FilterMessage("routine cursor not advanced").Len(); n != 0 {
|
||||
t.Errorf("expected no cursor-not-advanced log entries for a valid non-UTC slot, got %d", n)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTickScheduledTriggers_NeverMatchingExpression_CursorUnchangedAndLoggedOnce
|
||||
// covers AC-OFFICE-ROUTINE-STATUS-002.1/-002.8: a trigger whose stored cron
|
||||
// expression can never fire (Feb 30 never occurs) must not have its cursor
|
||||
// silently advanced when it turns up due, and the failure log is bounded to
|
||||
// once per trigger per process (AC-OFFICE-ROUTINE-STATUS-003.6) even across
|
||||
// repeated ticks. CreateRoutineTrigger now rejects an unsatisfiable
|
||||
// expression at creation time (shared.NextCronTime returns
|
||||
// shared.ErrUnsatisfiableCron instead of a silent fallback), so this
|
||||
// fixture writes the trigger row directly through the repository to model
|
||||
// data that predates that validation or otherwise bypassed it (e.g. a
|
||||
// direct database edit), which is the case suppressCronSlot's error path
|
||||
// still has to handle defensively.
|
||||
func TestTickScheduledTriggers_NeverMatchingExpression_CursorUnchangedAndLoggedOnce(t *testing.T) {
|
||||
svc, observed, repo := newObservedRoutineServiceWithRepo(t)
|
||||
ctx := context.Background()
|
||||
|
||||
routine := &models.Routine{
|
||||
WorkspaceID: "ws-1",
|
||||
Name: "Never Matching",
|
||||
TaskTemplate: "",
|
||||
AssigneeAgentProfileID: "agent-1",
|
||||
Status: "paused",
|
||||
ConcurrencyPolicy: "always_create",
|
||||
}
|
||||
if err := svc.CreateRoutine(ctx, routine); err != nil {
|
||||
t.Fatalf("create routine: %v", err)
|
||||
}
|
||||
staleNext := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC)
|
||||
if err := repo.CreateRoutineTrigger(ctx, &models.RoutineTrigger{
|
||||
RoutineID: routine.ID,
|
||||
Kind: "cron",
|
||||
CronExpression: "0 0 30 2 *", // Feb 30 never exists
|
||||
Timezone: "UTC",
|
||||
NextRunAt: &staleNext,
|
||||
Enabled: true,
|
||||
}); err != nil {
|
||||
t.Fatalf("create trigger: %v", err)
|
||||
}
|
||||
|
||||
for i := 0; i < 3; i++ {
|
||||
tickNow := staleNext.Add(time.Duration(i+1) * time.Minute)
|
||||
if err := svc.TickScheduledTriggers(ctx, tickNow); err != nil {
|
||||
t.Fatalf("tick %d: %v", i, err)
|
||||
}
|
||||
}
|
||||
|
||||
triggers, err := svc.ListRoutineTriggers(ctx, routine.ID)
|
||||
if err != nil || len(triggers) != 1 {
|
||||
t.Fatalf("list triggers: triggers=%v err=%v", triggers, err)
|
||||
}
|
||||
if got := triggers[0].NextRunAt; got == nil || !got.Equal(staleNext) {
|
||||
t.Errorf("next_run_at = %v, want unchanged %v — an unsatisfiable expression must not "+
|
||||
"silently advance the cursor", got, staleNext)
|
||||
}
|
||||
if n := observed.FilterMessage("routine cursor not advanced").Len(); n != 1 {
|
||||
t.Errorf("cursor-not-advanced logged %d times across 3 ticks, want 1 (AC-003.6 bound)", n)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTickScheduledTriggers_MissingRoutine_NotDisarmedAndLoggedOnce covers
|
||||
// AC-OFFICE-ROUTINE-STATUS-003.2/-003.3: an orphaned trigger (its routine
|
||||
// deleted) is never disarmed and the read failure is logged at most once
|
||||
// per process. This fixture only exists because the in-memory SQLite test
|
||||
// harness opens without `_foreign_keys=on` (production enables it, so
|
||||
// DeleteRoutine's cascade normally takes the trigger with it) — see
|
||||
// apps/backend/CLAUDE.md and the routine-status-gating plan.
|
||||
func TestTickScheduledTriggers_MissingRoutine_NotDisarmedAndLoggedOnce(t *testing.T) {
|
||||
svc, observed := newObservedRoutineService(t)
|
||||
routine, oldNext := createStatusGatedRoutine(t, svc, "active")
|
||||
ctx := context.Background()
|
||||
|
||||
if err := svc.DeleteRoutine(ctx, routine.ID); err != nil {
|
||||
t.Fatalf("delete routine: %v", err)
|
||||
}
|
||||
|
||||
for i := 0; i < 3; i++ {
|
||||
tickNow := oldNext.Add(time.Duration(i+2) * time.Minute)
|
||||
if err := svc.TickScheduledTriggers(ctx, tickNow); err != nil {
|
||||
t.Fatalf("tick %d: %v", i, err)
|
||||
}
|
||||
}
|
||||
|
||||
// ListTriggersByRoutineID is a plain SELECT by routine_id, so it still
|
||||
// finds the orphaned row even though the routine is gone.
|
||||
triggers, err := svc.ListRoutineTriggers(ctx, routine.ID)
|
||||
if err != nil || len(triggers) != 1 {
|
||||
t.Fatalf("list triggers: triggers=%v err=%v", triggers, err)
|
||||
}
|
||||
if got := triggers[0].NextRunAt; got == nil || !got.Equal(oldNext) {
|
||||
t.Errorf("next_run_at = %v, want unchanged %v — an unreadable routine must never be disarmed", got, oldNext)
|
||||
}
|
||||
if n := observed.FilterMessage("routine unreadable for due trigger").Len(); n != 1 {
|
||||
t.Errorf("unreadable-routine logged %d times across 3 ticks, want 1 (AC-003.6 bound)", n)
|
||||
}
|
||||
}
|
||||
|
||||
func TestFireManual_NonFiringRoutine_ReturnsRoutineNotFiringError(t *testing.T) {
|
||||
for _, status := range []string{"paused", "archived", "disabled"} {
|
||||
t.Run(status, func(t *testing.T) {
|
||||
svc := newTestRoutineService(t)
|
||||
ctx := context.Background()
|
||||
routine := createTestRoutine(t, svc, "Refuse "+status, "always_create")
|
||||
routine.Status = status
|
||||
if err := svc.UpdateRoutine(ctx, routine); err != nil {
|
||||
t.Fatalf("update routine status: %v", err)
|
||||
}
|
||||
|
||||
_, err := svc.FireManual(ctx, routine.ID, nil)
|
||||
var notFiring *routines.RoutineNotFiringError
|
||||
if !errors.As(err, ¬Firing) {
|
||||
t.Fatalf("FireManual err = %v, want *RoutineNotFiringError", err)
|
||||
}
|
||||
if notFiring.Status != status {
|
||||
t.Errorf("notFiring.Status = %q, want %q", notFiring.Status, status)
|
||||
}
|
||||
|
||||
runs, err := svc.ListRoutineRuns(ctx, routine.ID, 10, 0)
|
||||
if err != nil {
|
||||
t.Fatalf("list runs: %v", err)
|
||||
}
|
||||
if len(runs) != 0 {
|
||||
t.Errorf("expected no run row on a refused manual fire, got %d", len(runs))
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestFireManual_OtherError_NotWrappedAsNotFiring proves the errors.As
|
||||
// branch in the handler is type-discriminated: a routine that does not
|
||||
// exist keeps its ordinary error (and 500 mapping in the handler), it does
|
||||
// not surface as a status refusal.
|
||||
func TestFireManual_OtherError_NotWrappedAsNotFiring(t *testing.T) {
|
||||
svc := newTestRoutineService(t)
|
||||
ctx := context.Background()
|
||||
|
||||
_, err := svc.FireManual(ctx, "does-not-exist", nil)
|
||||
if err == nil {
|
||||
t.Fatal("expected an error for a missing routine")
|
||||
}
|
||||
var notFiring *routines.RoutineNotFiringError
|
||||
if errors.As(err, ¬Firing) {
|
||||
t.Errorf("a missing-routine error must not be a *RoutineNotFiringError, got %v", err)
|
||||
}
|
||||
}
|
||||
@@ -188,6 +188,11 @@ func (h *Handler) runRoutine(c *gin.Context) {
|
||||
_ = c.ShouldBindJSON(&req)
|
||||
run, err := h.svc.FireManual(c.Request.Context(), c.Param("id"), req.Variables)
|
||||
if err != nil {
|
||||
var notFiring *RoutineNotFiringError
|
||||
if errors.As(err, ¬Firing) {
|
||||
c.JSON(http.StatusConflict, routineNotFiringBody(notFiring.Status))
|
||||
return
|
||||
}
|
||||
writeDispatchError(c, err)
|
||||
return
|
||||
}
|
||||
@@ -293,6 +298,10 @@ func (h *Handler) fireWebhookTrigger(c *gin.Context) {
|
||||
c.JSON(http.StatusNotFound, gin.H{"error": "routine not found"})
|
||||
return
|
||||
}
|
||||
if !models.RoutineStatus(routine.Status).CanFire() {
|
||||
c.JSON(http.StatusConflict, routineNotFiringBody(routine.Status))
|
||||
return
|
||||
}
|
||||
|
||||
run, err := h.svc.DispatchRoutineRunWithIdempotencyKey(
|
||||
ctx, routine, trigger, "webhook", vars, c.GetHeader("Idempotency-Key"))
|
||||
@@ -303,6 +312,20 @@ func (h *Handler) fireWebhookTrigger(c *gin.Context) {
|
||||
c.JSON(http.StatusOK, gin.H{"run_id": run.ID, "status": run.Status})
|
||||
}
|
||||
|
||||
// routineNotFiringBody is the shared 409 body for a fire refused on
|
||||
// routine status, returned identically by the manual and webhook routes.
|
||||
// `error` is a human-readable fallback for a caller with no localized
|
||||
// copy; `error_code` is what a surface recognizes to select its own
|
||||
// localized message; `status` is the observed value, verbatim, so the
|
||||
// surface can interpolate it.
|
||||
func routineNotFiringBody(status string) gin.H {
|
||||
return gin.H{
|
||||
"error": fmt.Sprintf("routine cannot fire: status is %q", status),
|
||||
"error_code": RoutineNotFiringErrorCode,
|
||||
"status": status,
|
||||
}
|
||||
}
|
||||
|
||||
// redactTriggerSecrets clears the Secret field on each trigger to prevent
|
||||
// plaintext webhook secrets from leaking in API responses.
|
||||
// NOTE: Webhook secrets are stored as plaintext because HMAC verification
|
||||
|
||||
@@ -0,0 +1,185 @@
|
||||
package routines_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
|
||||
"github.com/gin-gonic/gin"
|
||||
|
||||
"github.com/kandev/kandev/internal/office/models"
|
||||
"github.com/kandev/kandev/internal/office/routines"
|
||||
)
|
||||
|
||||
// newStatusGateTestRouter registers routine routes on a fresh gin engine backed by
|
||||
// svc, so the tests below drive the real HTTP handlers rather than calling
|
||||
// service methods directly — the 409 body shape is part of the contract
|
||||
// (AC-OFFICE-ROUTINE-STATUS-004.4) and only the handler builds it.
|
||||
func newStatusGateTestRouter(svc *routines.RoutineService) *gin.Engine {
|
||||
gin.SetMode(gin.TestMode)
|
||||
engine := gin.New()
|
||||
group := engine.Group("/api/v1/office")
|
||||
routines.RegisterRoutes(group, routines.NewHandler(svc))
|
||||
return engine
|
||||
}
|
||||
|
||||
type statusRefusalBody struct {
|
||||
Error string `json:"error"`
|
||||
ErrorCode string `json:"error_code"`
|
||||
Status string `json:"status"`
|
||||
}
|
||||
|
||||
func decodeStatusRefusal(t *testing.T, rec *httptest.ResponseRecorder) statusRefusalBody {
|
||||
t.Helper()
|
||||
var body statusRefusalBody
|
||||
if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil {
|
||||
t.Fatalf("decode body %q: %v", rec.Body.String(), err)
|
||||
}
|
||||
return body
|
||||
}
|
||||
|
||||
func TestRunRoutine_PausedRoutine_Returns409WithStatusCode(t *testing.T) {
|
||||
svc := newTestRoutineService(t)
|
||||
router := newStatusGateTestRouter(svc)
|
||||
routine := createTestRoutine(t, svc, "HTTP Paused", "always_create")
|
||||
routine.Status = "paused"
|
||||
if err := svc.UpdateRoutine(context.Background(), routine); err != nil {
|
||||
t.Fatalf("update routine: %v", err)
|
||||
}
|
||||
|
||||
req := httptest.NewRequest(http.MethodPost, "/api/v1/office/routines/"+routine.ID+"/run", nil)
|
||||
rec := httptest.NewRecorder()
|
||||
router.ServeHTTP(rec, req)
|
||||
|
||||
if rec.Code != http.StatusConflict {
|
||||
t.Fatalf("status = %d, want %d; body=%s", rec.Code, http.StatusConflict, rec.Body.String())
|
||||
}
|
||||
body := decodeStatusRefusal(t, rec)
|
||||
if body.ErrorCode != "routine_not_firing" {
|
||||
t.Errorf("error_code = %q, want routine_not_firing", body.ErrorCode)
|
||||
}
|
||||
if body.Status != "paused" {
|
||||
t.Errorf("status field = %q, want paused", body.Status)
|
||||
}
|
||||
if body.Error == "" {
|
||||
t.Error("expected a non-empty human-readable error string")
|
||||
}
|
||||
|
||||
runs, err := svc.ListRoutineRuns(context.Background(), routine.ID, 10, 0)
|
||||
if err != nil {
|
||||
t.Fatalf("list runs: %v", err)
|
||||
}
|
||||
if len(runs) != 0 {
|
||||
t.Errorf("expected no run row, got %d", len(runs))
|
||||
}
|
||||
}
|
||||
|
||||
func TestRunRoutine_MissingRoutine_StaysAt500(t *testing.T) {
|
||||
svc := newTestRoutineService(t)
|
||||
router := newStatusGateTestRouter(svc)
|
||||
|
||||
req := httptest.NewRequest(http.MethodPost, "/api/v1/office/routines/does-not-exist/run", nil)
|
||||
rec := httptest.NewRecorder()
|
||||
router.ServeHTTP(rec, req)
|
||||
|
||||
if rec.Code != http.StatusInternalServerError {
|
||||
t.Errorf("status = %d, want %d — a missing routine keeps its pre-existing mapping "+
|
||||
"(named out of scope), it must not become 409/404", rec.Code, http.StatusInternalServerError)
|
||||
}
|
||||
}
|
||||
|
||||
func createWebhookTrigger(t *testing.T, svc *routines.RoutineService, routineID string) *models.RoutineTrigger {
|
||||
t.Helper()
|
||||
trigger := &models.RoutineTrigger{
|
||||
RoutineID: routineID,
|
||||
Kind: "webhook",
|
||||
PublicID: "wh-" + routineID,
|
||||
SigningMode: "none",
|
||||
Enabled: true,
|
||||
}
|
||||
if err := svc.CreateRoutineTrigger(context.Background(), trigger); err != nil {
|
||||
t.Fatalf("create webhook trigger: %v", err)
|
||||
}
|
||||
return trigger
|
||||
}
|
||||
|
||||
func TestFireWebhookTrigger_PausedRoutine_Returns409WithStatusCode(t *testing.T) {
|
||||
svc := newTestRoutineService(t)
|
||||
router := newStatusGateTestRouter(svc)
|
||||
routine := createTestRoutine(t, svc, "Webhook Paused", "always_create")
|
||||
routine.Status = "paused"
|
||||
if err := svc.UpdateRoutine(context.Background(), routine); err != nil {
|
||||
t.Fatalf("update routine: %v", err)
|
||||
}
|
||||
trigger := createWebhookTrigger(t, svc, routine.ID)
|
||||
|
||||
req := httptest.NewRequest(
|
||||
http.MethodPost, "/api/v1/office/routine-triggers/"+trigger.PublicID+"/fire", nil)
|
||||
rec := httptest.NewRecorder()
|
||||
router.ServeHTTP(rec, req)
|
||||
|
||||
if rec.Code != http.StatusConflict {
|
||||
t.Fatalf("status = %d, want %d; body=%s", rec.Code, http.StatusConflict, rec.Body.String())
|
||||
}
|
||||
body := decodeStatusRefusal(t, rec)
|
||||
if body.ErrorCode != "routine_not_firing" {
|
||||
t.Errorf("error_code = %q, want routine_not_firing", body.ErrorCode)
|
||||
}
|
||||
if body.Status != "paused" {
|
||||
t.Errorf("status field = %q, want paused", body.Status)
|
||||
}
|
||||
}
|
||||
|
||||
func TestFireWebhookTrigger_FiringRoutine_Returns200(t *testing.T) {
|
||||
svc := newTestRoutineService(t)
|
||||
router := newStatusGateTestRouter(svc)
|
||||
routine := createTestRoutine(t, svc, "Webhook Firing", "always_create")
|
||||
trigger := createWebhookTrigger(t, svc, routine.ID)
|
||||
|
||||
req := httptest.NewRequest(
|
||||
http.MethodPost, "/api/v1/office/routine-triggers/"+trigger.PublicID+"/fire", nil)
|
||||
rec := httptest.NewRecorder()
|
||||
router.ServeHTTP(rec, req)
|
||||
|
||||
if rec.Code != http.StatusOK {
|
||||
t.Fatalf("status = %d, want %d; body=%s", rec.Code, http.StatusOK, rec.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
// TestFireWebhookTrigger_BadSignature_RefusedRegardlessOfStatus covers
|
||||
// AC-OFFICE-ROUTINE-STATUS-004.3: the signature check runs before the
|
||||
// status check, so an unauthenticated caller cannot use the response code
|
||||
// to probe a paused routine's status.
|
||||
func TestFireWebhookTrigger_BadSignature_RefusedRegardlessOfStatus(t *testing.T) {
|
||||
svc := newTestRoutineService(t)
|
||||
router := newStatusGateTestRouter(svc)
|
||||
routine := createTestRoutine(t, svc, "Webhook Bad Sig", "always_create")
|
||||
routine.Status = "paused"
|
||||
if err := svc.UpdateRoutine(context.Background(), routine); err != nil {
|
||||
t.Fatalf("update routine: %v", err)
|
||||
}
|
||||
trigger := &models.RoutineTrigger{
|
||||
RoutineID: routine.ID,
|
||||
Kind: "webhook",
|
||||
PublicID: "wh-badsig-" + routine.ID,
|
||||
SigningMode: "bearer",
|
||||
Secret: "correct-secret",
|
||||
Enabled: true,
|
||||
}
|
||||
if err := svc.CreateRoutineTrigger(context.Background(), trigger); err != nil {
|
||||
t.Fatalf("create trigger: %v", err)
|
||||
}
|
||||
|
||||
req := httptest.NewRequest(
|
||||
http.MethodPost, "/api/v1/office/routine-triggers/"+trigger.PublicID+"/fire", nil)
|
||||
req.Header.Set("Authorization", "Bearer wrong-secret")
|
||||
rec := httptest.NewRecorder()
|
||||
router.ServeHTTP(rec, req)
|
||||
|
||||
if rec.Code != http.StatusUnauthorized {
|
||||
t.Fatalf("status = %d, want %d (signature refusal must win over the status refusal); body=%s",
|
||||
rec.Code, http.StatusUnauthorized, rec.Body.String())
|
||||
}
|
||||
}
|
||||
@@ -2,6 +2,7 @@ package routines_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"database/sql"
|
||||
"expvar"
|
||||
"testing"
|
||||
"time"
|
||||
@@ -82,14 +83,18 @@ func TestTickScheduledTriggers_IncrementsTriggerClaimedCounter(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// AC-003.9: a claim that persists must count even when the routine
|
||||
// lookup afterward fails, so the claim can't be attributed to a
|
||||
// workspace. Without the fallback, a claim that already wrote to the
|
||||
// database silently vanished from office_loop_trigger_claimed_total
|
||||
// (Review round 1, R1-4) — the counter must reflect the persisted
|
||||
// write, not the lookup that happens to follow it.
|
||||
func TestTickScheduledTriggers_ClaimedCounterSurvivesRoutineLookupFailure(t *testing.T) {
|
||||
svc := newTestRoutineService(t)
|
||||
// AC-OFFICE-ROUTINE-STATUS-003.3 (routine-status-gating) supersedes the
|
||||
// pre-status-gate assumption this test originally checked (loop-liveness
|
||||
// AC-003.9: a claim that persists must count even when the routine lookup
|
||||
// afterward fails). The status gate now reads the routine — to decide
|
||||
// CanFire() — before ClaimTrigger runs, so an orphaned trigger is caught at
|
||||
// that read and never reaches the claim at all: there is no longer a window
|
||||
// where a claim persists but attribution fails. AC-OFFICE-ROUTINE-STATUS-003.3
|
||||
// requires exactly this: an unreadable routine must not fire and must not
|
||||
// disarm or otherwise modify the trigger, so office_loop_trigger_claimed_total
|
||||
// (attributed or unattributed) must not move either.
|
||||
func TestTickScheduledTriggers_OrphanedTrigger_NotClaimed(t *testing.T) {
|
||||
svc, db := newTestRoutineServiceWithDB(t)
|
||||
ctx := context.Background()
|
||||
|
||||
routine := newLightweightTestRoutine(t, svc)
|
||||
@@ -102,26 +107,37 @@ func TestTickScheduledTriggers_ClaimedCounterSurvivesRoutineLookupFailure(t *tes
|
||||
}); err != nil {
|
||||
t.Fatalf("create trigger: %v", err)
|
||||
}
|
||||
enq := &fakeWakeupEnqueuer{}
|
||||
svc.SetWakeupEnqueuer(enq)
|
||||
|
||||
// Orphan the trigger: delete the routine it points at without
|
||||
// touching the trigger row, so ClaimTrigger still succeeds but the
|
||||
// subsequent GetRoutineFromConfig fails.
|
||||
// Orphan the trigger: delete the routine it points at without touching
|
||||
// the trigger row (this test's in-memory DB has no _foreign_keys=on, so
|
||||
// the ON DELETE CASCADE does not fire), so GetDueTriggers still surfaces
|
||||
// it but GetRoutineFromConfig fails before any claim is attempted.
|
||||
if err := svc.DeleteRoutine(ctx, routine.ID); err != nil {
|
||||
t.Fatalf("delete routine: %v", err)
|
||||
}
|
||||
|
||||
key := "workspace=_unattributed"
|
||||
before := loopCounterInt(t, "office_loop_trigger_claimed_total", key)
|
||||
claimedKey := "workspace=" + routine.WorkspaceID
|
||||
unattrKey := "workspace=_unattributed"
|
||||
claimedBefore := loopCounterInt(t, "office_loop_trigger_claimed_total", claimedKey)
|
||||
unattrBefore := loopCounterInt(t, "office_loop_trigger_claimed_total", unattrKey)
|
||||
|
||||
if err := svc.TickScheduledTriggers(ctx, time.Now().UTC().Add(2*time.Minute)); err != nil {
|
||||
t.Fatalf("tick: %v", err)
|
||||
}
|
||||
|
||||
after := loopCounterInt(t, "office_loop_trigger_claimed_total", key)
|
||||
if after != before+1 {
|
||||
t.Fatalf("trigger_claimed[_unattributed] delta = %d, want 1", after-before)
|
||||
if after := loopCounterInt(t, "office_loop_trigger_claimed_total", claimedKey); after != claimedBefore {
|
||||
t.Fatalf("trigger_claimed[workspace] delta = %d, want 0: an orphaned trigger must not be claimed", after-claimedBefore)
|
||||
}
|
||||
if after := loopCounterInt(t, "office_loop_trigger_claimed_total", unattrKey); after != unattrBefore {
|
||||
t.Fatalf("trigger_claimed[_unattributed] delta = %d, want 0: an orphaned trigger must not be claimed", after-unattrBefore)
|
||||
}
|
||||
|
||||
var lastFiredAt sql.NullString
|
||||
if err := db.Get(&lastFiredAt, `SELECT last_fired_at FROM office_routine_triggers WHERE routine_id = ?`, routine.ID); err != nil {
|
||||
t.Fatalf("read last_fired_at: %v", err)
|
||||
}
|
||||
if lastFiredAt.Valid {
|
||||
t.Fatalf("trigger was claimed (last_fired_at = %q) despite its routine being deleted; "+
|
||||
"AC-OFFICE-ROUTINE-STATUS-003.3 forbids modifying an orphaned trigger", lastFiredAt.String)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -53,6 +53,7 @@ type Repository interface {
|
||||
GetTriggerByPublicID(ctx context.Context, publicID string) (*RoutineTrigger, error)
|
||||
GetDueTriggers(ctx context.Context, now time.Time) ([]*RoutineTrigger, error)
|
||||
ClaimTrigger(ctx context.Context, triggerID string, oldNextRunAt time.Time) (bool, error)
|
||||
AdvanceTriggerWithoutFiring(ctx context.Context, triggerID string, oldNextRunAt, newNextRunAt time.Time) (bool, error)
|
||||
UpdateTriggerNextRun(ctx context.Context, triggerID string, nextRunAt *time.Time) error
|
||||
ListStrandedTriggers(ctx context.Context, olderThan time.Time) ([]*RoutineTrigger, error)
|
||||
ReconcileTriggerNextRun(ctx context.Context, triggerID string, nextRunAt time.Time) (bool, error)
|
||||
@@ -143,6 +144,7 @@ type RoutineService struct {
|
||||
wakeup WakeupEnqueuer
|
||||
workflowEnsurer RoutineWorkflowEnsurer
|
||||
taskCreator RoutineTaskCreator
|
||||
stuckLog stuckTriggerLog
|
||||
pauseGate shared.PauseGate
|
||||
}
|
||||
|
||||
@@ -476,17 +478,28 @@ func (s *RoutineService) processCronTrigger(ctx context.Context, trigger *Routin
|
||||
if trigger.NextRunAt == nil {
|
||||
return nil
|
||||
}
|
||||
// Read the routine and decide before claiming. A read failure or a
|
||||
// missing routine leaves the cursor untouched so the trigger stays due
|
||||
// and the next tick retries — the read cannot tell "deleted" from
|
||||
// "failed", so acting on either would permanently disarm a trigger
|
||||
// whose routine is only temporarily unreadable.
|
||||
routine, err := s.GetRoutineFromConfig(ctx, trigger.RoutineID)
|
||||
if err != nil {
|
||||
if s.stuckLog.shouldLog(trigger.ID, outcomeUnreadableRoutine) {
|
||||
s.logger.Error("routine unreadable for due trigger",
|
||||
zap.String("trigger_id", trigger.ID), zap.Error(err))
|
||||
}
|
||||
return nil
|
||||
}
|
||||
if !models.RoutineStatus(routine.Status).CanFire() {
|
||||
return s.suppressCronSlot(ctx, trigger, routine, now)
|
||||
}
|
||||
claimed, err := s.repo.ClaimTrigger(ctx, trigger.ID, *trigger.NextRunAt)
|
||||
if err != nil || !claimed {
|
||||
return err
|
||||
}
|
||||
routine, err := s.GetRoutineFromConfig(ctx, trigger.RoutineID)
|
||||
if err != nil {
|
||||
// The claim already persisted, so the counter must still see it
|
||||
// even though the workspace it belongs to can't be resolved.
|
||||
service.IncLoopTriggerClaimed(service.LoopUnattributedWorkspace)
|
||||
return fmt.Errorf("get routine: %w", err)
|
||||
}
|
||||
// The routine was read and approved before the claim, so its workspace
|
||||
// is already known at the point where the persisted claim is counted.
|
||||
service.IncLoopTriggerClaimed(routine.WorkspaceID)
|
||||
result := computeCatchUp(trigger, routine, now)
|
||||
if result.Unknown {
|
||||
@@ -542,6 +555,53 @@ func (s *RoutineService) processCronTrigger(ctx context.Context, trigger *Routin
|
||||
return err
|
||||
}
|
||||
|
||||
// suppressCronSlot handles a due cron trigger for a routine that does not
|
||||
// hold a firing status: no claim, no run row, no wakeup, no task. The
|
||||
// cursor advances to the first slot the trigger's cron expression names
|
||||
// strictly after now, rather than by one slot from the old cursor, so a
|
||||
// suppression window collapses in one evaluation instead of being walked.
|
||||
func (s *RoutineService) suppressCronSlot(
|
||||
ctx context.Context, trigger *RoutineTrigger, routine *Routine, now time.Time,
|
||||
) error {
|
||||
next, err := s.computeSuppressionCursor(trigger, now)
|
||||
if err != nil {
|
||||
if s.stuckLog.shouldLog(trigger.ID, outcomeCursorNotAdvanced) {
|
||||
s.logger.Warn("routine cursor not advanced",
|
||||
zap.String("trigger_id", trigger.ID), zap.Error(err))
|
||||
}
|
||||
return nil
|
||||
}
|
||||
advanced, err := s.repo.AdvanceTriggerWithoutFiring(ctx, trigger.ID, *trigger.NextRunAt, next)
|
||||
if err != nil {
|
||||
if s.stuckLog.shouldLog(trigger.ID, outcomeCursorNotAdvanced) {
|
||||
s.logger.Warn("routine cursor not advanced",
|
||||
zap.String("trigger_id", trigger.ID), zap.Error(err))
|
||||
}
|
||||
return nil
|
||||
}
|
||||
if !advanced {
|
||||
// Lost the compare-and-set: another evaluation already advanced
|
||||
// this slot. Not a failure, so nothing is logged.
|
||||
return nil
|
||||
}
|
||||
s.logger.Info("routine slot suppressed",
|
||||
zap.String("routine_id", routine.ID),
|
||||
zap.String("trigger_id", trigger.ID),
|
||||
zap.String("status", routine.Status),
|
||||
zap.Time("old_next_run_at", *trigger.NextRunAt),
|
||||
zap.Time("new_next_run_at", next))
|
||||
return nil
|
||||
}
|
||||
|
||||
// computeSuppressionCursor computes the first slot the trigger's cron
|
||||
// expression names strictly after now, in the trigger's timezone.
|
||||
// NextCronTime itself guarantees the returned time is one the expression
|
||||
// actually names, or reports shared.ErrUnsatisfiableCron — there is no
|
||||
// separate match check to run here.
|
||||
func (s *RoutineService) computeSuppressionCursor(trigger *RoutineTrigger, now time.Time) (time.Time, error) {
|
||||
return shared.NextCronTime(trigger.CronExpression, trigger.Timezone, now)
|
||||
}
|
||||
|
||||
// catchUpResult is what computeCatchUp returns instead of a bare
|
||||
// (int, time.Time, error): a single value object so no call site can
|
||||
// discard a computed re-arm time on the error path.
|
||||
@@ -1132,7 +1192,11 @@ func (s *RoutineService) applyConcurrencyPolicy(
|
||||
}
|
||||
}
|
||||
|
||||
// FireManual dispatches a routine run from a manual trigger.
|
||||
// FireManual dispatches a routine run from a manual trigger. When the
|
||||
// routine does not hold a firing status it returns *RoutineNotFiringError
|
||||
// instead of dispatching — the gate sits here, on the routine this call
|
||||
// already read, rather than in the handler re-reading it, so the decision
|
||||
// and the dispatch share one read.
|
||||
func (s *RoutineService) FireManual(
|
||||
ctx context.Context, routineID string, variableValues map[string]string,
|
||||
) (*RoutineRun, error) {
|
||||
@@ -1140,6 +1204,9 @@ func (s *RoutineService) FireManual(
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("get routine: %w", err)
|
||||
}
|
||||
if !models.RoutineStatus(routine.Status).CanFire() {
|
||||
return nil, &RoutineNotFiringError{Status: routine.Status}
|
||||
}
|
||||
return s.DispatchRoutineRun(ctx, routine, nil, "manual", variableValues)
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,67 @@
|
||||
package routines
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"sync"
|
||||
)
|
||||
|
||||
// RoutineNotFiringErrorCode is the stable machine-readable code the API
|
||||
// returns when a manual or webhook fire is refused because the routine's
|
||||
// status does not permit it. The frontend selects localized copy from this
|
||||
// code rather than the human-readable error string.
|
||||
const RoutineNotFiringErrorCode = "routine_not_firing"
|
||||
|
||||
// RoutineNotFiringError is returned by FireManual when the routine's
|
||||
// status does not permit a fire. The handler recognizes it with
|
||||
// errors.As before its catch-all and maps it to HTTP 409; every other
|
||||
// FireManual error — including a missing routine — keeps its existing
|
||||
// 500 mapping.
|
||||
type RoutineNotFiringError struct {
|
||||
// Status is the observed status, reported verbatim in the response
|
||||
// body so the surface can name it without re-reading the routine.
|
||||
Status string
|
||||
}
|
||||
|
||||
func (e *RoutineNotFiringError) Error() string {
|
||||
return fmt.Sprintf("routine cannot fire: status is %q", e.Status)
|
||||
}
|
||||
|
||||
// stuckTriggerOutcome names the two outcomes that leave a due cron trigger
|
||||
// un-advanced: the routine backing it could not be read, or its cursor
|
||||
// could not be advanced. Both re-evaluate every tick for the life of the
|
||||
// process, so their log entries are bounded to once per (trigger, outcome);
|
||||
// the suppression-succeeded entry itself is not one of these and is never
|
||||
// deduplicated.
|
||||
type stuckTriggerOutcome string
|
||||
|
||||
const (
|
||||
outcomeUnreadableRoutine stuckTriggerOutcome = "unreadable_routine"
|
||||
outcomeCursorNotAdvanced stuckTriggerOutcome = "cursor_not_advanced"
|
||||
)
|
||||
|
||||
// stuckTriggerLog bounds the two "trigger stayed due" log entries to at
|
||||
// most once per (trigger id, outcome) per process. The comparison key is
|
||||
// the trigger id and the outcome kind only — no observed status, no error
|
||||
// value — and the check-and-insert is one atomic operation, so concurrent
|
||||
// callers observing the same pair cannot both log.
|
||||
type stuckTriggerLog struct {
|
||||
mu sync.Mutex
|
||||
seen map[string]struct{}
|
||||
}
|
||||
|
||||
// shouldLog reports whether this (trigger, outcome) pair has not yet been
|
||||
// logged in this process, recording it before returning so a concurrent
|
||||
// caller observing the same pair gets false.
|
||||
func (l *stuckTriggerLog) shouldLog(triggerID string, outcome stuckTriggerOutcome) bool {
|
||||
key := triggerID + "|" + string(outcome)
|
||||
l.mu.Lock()
|
||||
defer l.mu.Unlock()
|
||||
if l.seen == nil {
|
||||
l.seen = make(map[string]struct{})
|
||||
}
|
||||
if _, logged := l.seen[key]; logged {
|
||||
return false
|
||||
}
|
||||
l.seen[key] = struct{}{}
|
||||
return true
|
||||
}
|
||||
@@ -0,0 +1,71 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { t } from "@/lib/i18n";
|
||||
|
||||
import { ApiError } from "@/lib/api/client";
|
||||
import { isRoutineNotFiringError, routineNotFiringMessage } from "./routine-not-firing";
|
||||
|
||||
const FALLBACK_KEY = "office:failedToRunRoutine";
|
||||
|
||||
function notFiringError(status: string): ApiError {
|
||||
return new ApiError("routine cannot fire", 409, {
|
||||
error: "routine cannot fire",
|
||||
error_code: "routine_not_firing",
|
||||
status,
|
||||
});
|
||||
}
|
||||
|
||||
describe("isRoutineNotFiringError", () => {
|
||||
it("recognizes the status-refusal error code", () => {
|
||||
expect(isRoutineNotFiringError(notFiringError("paused"))).toBe(true);
|
||||
});
|
||||
|
||||
it("does not recognize an unrelated ApiError", () => {
|
||||
const other = new ApiError("boom", 500, { error: "boom" });
|
||||
expect(isRoutineNotFiringError(other)).toBe(false);
|
||||
});
|
||||
|
||||
it("does not recognize a plain Error", () => {
|
||||
expect(isRoutineNotFiringError(new Error("boom"))).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("routineNotFiringMessage", () => {
|
||||
const translateWithStatusLabels = (key: string, options?: Record<string, unknown>): string => {
|
||||
if (key === "office:routineStatusPaused") return "Pausada";
|
||||
if (key === "office:routineStatusArchived") return "Arquivada";
|
||||
if (key === "office:routineNotFiring") {
|
||||
return `Cannot run: routine status is ${String(options?.status ?? "")}`;
|
||||
}
|
||||
return key;
|
||||
};
|
||||
|
||||
it.each([
|
||||
["paused", "Pausada"],
|
||||
["archived", "Arquivada"],
|
||||
])("uses the translated label for %s", (status, translatedStatus) => {
|
||||
expect(
|
||||
routineNotFiringMessage(notFiringError(status), translateWithStatusLabels, FALLBACK_KEY),
|
||||
).toBe(`Cannot run: routine status is ${translatedStatus}`);
|
||||
});
|
||||
|
||||
it("keeps an unknown status as diagnostic text", () => {
|
||||
expect(
|
||||
routineNotFiringMessage(notFiringError("on_hold"), translateWithStatusLabels, FALLBACK_KEY),
|
||||
).toBe("Cannot run: routine status is on_hold");
|
||||
});
|
||||
|
||||
it("renders localized copy naming the observed status", () => {
|
||||
const msg = routineNotFiringMessage(notFiringError("paused"), t, FALLBACK_KEY);
|
||||
expect(msg).not.toBe(t(FALLBACK_KEY));
|
||||
expect(msg).toContain("Paused");
|
||||
});
|
||||
|
||||
it("falls back to the server message for any other Error", () => {
|
||||
const err = new Error("some other failure");
|
||||
expect(routineNotFiringMessage(err, t, FALLBACK_KEY)).toBe("some other failure");
|
||||
});
|
||||
|
||||
it("falls back to the fallback key when there is no message", () => {
|
||||
expect(routineNotFiringMessage("not an error", t, FALLBACK_KEY)).toBe(t(FALLBACK_KEY));
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,46 @@
|
||||
import { ApiError } from "@/lib/api/client";
|
||||
|
||||
/**
|
||||
* Stable machine-readable code the backend returns when a manual or
|
||||
* webhook fire is refused because the routine's status does not permit it
|
||||
* (apps/backend/internal/office/routines/status_gate.go). The frontend
|
||||
* selects localized copy from this code rather than the server's
|
||||
* human-readable string, which cannot be translated.
|
||||
*/
|
||||
export const ROUTINE_NOT_FIRING_ERROR_CODE = "routine_not_firing";
|
||||
|
||||
export function isRoutineNotFiringError(error: unknown): error is ApiError {
|
||||
return error instanceof ApiError && error.errorCode === ROUTINE_NOT_FIRING_ERROR_CODE;
|
||||
}
|
||||
|
||||
type Translate = (key: string, options?: Record<string, unknown>) => string;
|
||||
|
||||
const ROUTINE_STATUS_LABEL_KEYS: Record<string, string> = {
|
||||
active: "office:routineStatusActive",
|
||||
archived: "office:routineStatusArchived",
|
||||
paused: "office:routineStatusPaused",
|
||||
};
|
||||
|
||||
function refusedStatus(error: ApiError, t: Translate): string {
|
||||
if (!error.body || typeof error.body !== "object") return "";
|
||||
const status = (error.body as { status?: unknown }).status;
|
||||
if (typeof status !== "string") return "";
|
||||
const labelKey = ROUTINE_STATUS_LABEL_KEYS[status];
|
||||
return labelKey ? t(labelKey) : status;
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolves the "Run now" failure toast. A status refusal renders localized
|
||||
* copy naming the observed status, interpolated from the response body's
|
||||
* `status` field rather than parsed out of the server's English sentence
|
||||
* (AC-OFFICE-ROUTINE-STATUS-006.4, -006.6). Any other error keeps the
|
||||
* existing behavior: the server message when there is one, else the
|
||||
* caller's fallback key.
|
||||
*/
|
||||
export function routineNotFiringMessage(error: unknown, t: Translate, fallbackKey: string): string {
|
||||
if (isRoutineNotFiringError(error)) {
|
||||
return t("office:routineNotFiring", { status: refusedStatus(error, t) });
|
||||
}
|
||||
if (error instanceof Error && error.message.trim()) return error.message;
|
||||
return t(fallbackKey);
|
||||
}
|
||||
@@ -0,0 +1,25 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
import { isRoutineFiring } from "./routine-status";
|
||||
|
||||
describe("isRoutineFiring", () => {
|
||||
it("fires for active and the empty string", () => {
|
||||
expect(isRoutineFiring("active")).toBe(true);
|
||||
expect(isRoutineFiring("")).toBe(true);
|
||||
});
|
||||
|
||||
it("does not fire for paused or archived", () => {
|
||||
expect(isRoutineFiring("paused")).toBe(false);
|
||||
expect(isRoutineFiring("archived")).toBe(false);
|
||||
});
|
||||
|
||||
it("does not fire for an unrecognized value", () => {
|
||||
expect(isRoutineFiring("on_hold")).toBe(false);
|
||||
});
|
||||
|
||||
it("is byte-exact: no case folding, no trimming", () => {
|
||||
expect(isRoutineFiring("Active")).toBe(false);
|
||||
expect(isRoutineFiring(" active")).toBe(false);
|
||||
expect(isRoutineFiring("active ")).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,11 @@
|
||||
/**
|
||||
* Byte-exact allowlist mirroring the backend's `RoutineStatus.CanFire()`
|
||||
* (apps/backend/internal/office/models/enums.go): "active" and the empty
|
||||
* string (the column's "no writer set one" default) fire; "paused",
|
||||
* "archived", or any other value does not. No case folding, no trimming —
|
||||
* the comparison has to match the scheduler's exactly, or the UI promises a
|
||||
* fire the backend will not produce.
|
||||
*/
|
||||
export function isRoutineFiring(status: string): boolean {
|
||||
return status === "active" || status === "";
|
||||
}
|
||||
@@ -1,54 +1,97 @@
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { cleanup, render, screen, fireEvent, within } from "@testing-library/react";
|
||||
import { cleanup, fireEvent, render, screen, within } from "@testing-library/react";
|
||||
import { StateProvider } from "@/components/state-provider";
|
||||
import { defaultOfficeState } from "@/lib/state/slices/office/office-slice";
|
||||
import type { Routine, RoutineTrigger } from "@/lib/state/slices/office/types";
|
||||
|
||||
vi.mock("@/lib/routing/client-router", () => ({
|
||||
useRouter: () => ({
|
||||
push: vi.fn(),
|
||||
replace: vi.fn(),
|
||||
refresh: vi.fn(),
|
||||
back: vi.fn(),
|
||||
forward: vi.fn(),
|
||||
prefetch: vi.fn(),
|
||||
}),
|
||||
}));
|
||||
|
||||
vi.mock("@/components/state-provider", () => ({
|
||||
useAppStore: (selector: (state: unknown) => unknown) =>
|
||||
selector({
|
||||
workspaces: { activeId: "ws-1" },
|
||||
office: { agentProfilesByWorkspaceId: { "ws-1": [] } },
|
||||
}),
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/api/domains/office-api", () => ({
|
||||
updateRoutine: vi.fn(),
|
||||
runRoutine: vi.fn(),
|
||||
createRoutineTrigger: vi.fn(),
|
||||
deleteRoutineTrigger: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/toast/sonner", () => ({
|
||||
toast: { success: vi.fn(), error: vi.fn() },
|
||||
}));
|
||||
|
||||
import { RoutineDetailView } from "./routine-detail-view";
|
||||
|
||||
afterEach(() => cleanup());
|
||||
vi.mock("@/lib/routing/client-router", () => ({
|
||||
useRouter: () => ({ refresh: vi.fn(), push: vi.fn(), replace: vi.fn() }),
|
||||
}));
|
||||
|
||||
const BASE_ROUTINE: Routine = {
|
||||
vi.mock("@/lib/api/domains/office-api", async () => {
|
||||
const actual = await vi.importActual<typeof import("@/lib/api/domains/office-api")>(
|
||||
"@/lib/api/domains/office-api",
|
||||
);
|
||||
return {
|
||||
...actual,
|
||||
updateRoutine: vi.fn().mockResolvedValue({}),
|
||||
runRoutine: vi.fn().mockResolvedValue({}),
|
||||
};
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup();
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
const WORKSPACE_ID = "ws-1";
|
||||
const TIMESTAMP = "2026-05-04T00:00:00Z";
|
||||
|
||||
const baseRoutine: Routine = {
|
||||
id: "routine-1",
|
||||
workspaceId: "ws-1",
|
||||
name: "Nightly digest",
|
||||
workspaceId: WORKSPACE_ID,
|
||||
name: "Nightly sync",
|
||||
taskTemplate: {},
|
||||
status: "active",
|
||||
concurrencyPolicy: "coalesce_if_active",
|
||||
createdAt: "2026-01-01T00:00:00Z",
|
||||
updatedAt: "2026-01-01T00:00:00Z",
|
||||
createdAt: TIMESTAMP,
|
||||
updatedAt: TIMESTAMP,
|
||||
};
|
||||
|
||||
const cronTrigger: RoutineTrigger = {
|
||||
id: "trigger-1",
|
||||
routineId: "routine-1",
|
||||
kind: "cron",
|
||||
cronExpression: "*/5 * * * *",
|
||||
timezone: "UTC",
|
||||
nextRunAt: "2026-05-05T00:00:00Z",
|
||||
enabled: true,
|
||||
createdAt: TIMESTAMP,
|
||||
updatedAt: TIMESTAMP,
|
||||
};
|
||||
|
||||
const NO_TRIGGERS: RoutineTrigger[] = [];
|
||||
|
||||
function renderDetailView(routine: Routine, triggers: RoutineTrigger[]) {
|
||||
return render(
|
||||
<StateProvider
|
||||
initialState={{
|
||||
workspaces: { activeId: WORKSPACE_ID, items: [] },
|
||||
office: { ...defaultOfficeState.office },
|
||||
}}
|
||||
>
|
||||
<RoutineDetailView initialRoutine={routine} initialTriggers={triggers} />
|
||||
</StateProvider>,
|
||||
);
|
||||
}
|
||||
|
||||
describe("RoutineDetailView next-fire display", () => {
|
||||
it("hides the next-fire countdown as soon as status is changed to paused, before saving", async () => {
|
||||
renderDetailView(baseRoutine, [cronTrigger]);
|
||||
|
||||
expect(screen.getByText(/Next fire: 5\/5\/2026/)).toBeTruthy();
|
||||
|
||||
const statusField = screen.getByText("Status").closest("div") as HTMLElement;
|
||||
fireEvent.click(within(statusField).getByRole("combobox"));
|
||||
const listbox = await screen.findByRole("listbox");
|
||||
fireEvent.click(within(listbox).getByRole("option", { name: "Paused" }));
|
||||
|
||||
expect(screen.getByText("Next fire: -")).toBeTruthy();
|
||||
expect(screen.queryByText(/Next fire: 5\/5\/2026/)).toBeNull();
|
||||
});
|
||||
|
||||
it("keeps showing the next-fire countdown for a routine loaded as active", () => {
|
||||
renderDetailView(baseRoutine, [cronTrigger]);
|
||||
expect(screen.getByText(/Next fire: 5\/5\/2026/)).toBeTruthy();
|
||||
});
|
||||
|
||||
it("shows no next-fire countdown for a routine loaded as paused", () => {
|
||||
renderDetailView({ ...baseRoutine, status: "paused" }, [cronTrigger]);
|
||||
expect(screen.getByText("Next fire: -")).toBeTruthy();
|
||||
});
|
||||
});
|
||||
|
||||
// AC-OFFICE-ROUTINE-CATCHUP-003.8: the catch_up_max control renders under
|
||||
// the summarizing policy and is absent under skip_missed, in the detail
|
||||
// view exactly as in the create dialog. Regression coverage for the
|
||||
@@ -58,30 +101,25 @@ const NO_TRIGGERS: RoutineTrigger[] = [];
|
||||
// input renders at all.
|
||||
describe("RoutineDetailView catch-up max control (AC-003.8)", () => {
|
||||
it("renders the catch-up max input when the routine is summarize_missed", () => {
|
||||
render(
|
||||
<RoutineDetailView
|
||||
initialRoutine={{ ...BASE_ROUTINE, catchUpPolicy: "summarize_missed", catchUpMax: 25 }}
|
||||
initialTriggers={NO_TRIGGERS}
|
||||
/>,
|
||||
renderDetailView(
|
||||
{ ...baseRoutine, catchUpPolicy: "summarize_missed", catchUpMax: 25 },
|
||||
NO_TRIGGERS,
|
||||
);
|
||||
expect(screen.getByText(/catch-up max/i)).toBeTruthy();
|
||||
expect(screen.getByRole("spinbutton")).toBeTruthy();
|
||||
});
|
||||
|
||||
it("does not render the catch-up max input when the routine is skip_missed", () => {
|
||||
render(
|
||||
<RoutineDetailView
|
||||
initialRoutine={{ ...BASE_ROUTINE, catchUpPolicy: "skip_missed" }}
|
||||
initialTriggers={NO_TRIGGERS}
|
||||
/>,
|
||||
);
|
||||
renderDetailView({ ...baseRoutine, catchUpPolicy: "skip_missed" }, NO_TRIGGERS);
|
||||
expect(screen.queryByText(/catch-up max/i)).toBeNull();
|
||||
expect(screen.queryByRole("spinbutton")).toBeNull();
|
||||
});
|
||||
|
||||
it("falls back to summarize_missed (not the retired value) when catchUpPolicy is unset", () => {
|
||||
const { catchUpPolicy: _omit, ...withoutPolicy } = BASE_ROUTINE;
|
||||
render(<RoutineDetailView initialRoutine={withoutPolicy} initialTriggers={NO_TRIGGERS} />);
|
||||
const { catchUpPolicy: _omit, ...withoutPolicy } = baseRoutine as Routine & {
|
||||
catchUpPolicy?: string;
|
||||
};
|
||||
renderDetailView(withoutPolicy as Routine, NO_TRIGGERS);
|
||||
expect(screen.getByText(/catch-up max/i)).toBeTruthy();
|
||||
expect(screen.getByRole("spinbutton")).toBeTruthy();
|
||||
});
|
||||
@@ -91,11 +129,9 @@ describe("RoutineDetailView catch-up max control (AC-003.8)", () => {
|
||||
// state a single summarized wake, matching the create dialog.
|
||||
describe("RoutineDetailView catch-up policy labeling (AC-003.6)", () => {
|
||||
it("labels the summarizing policy option as a single wake", () => {
|
||||
render(
|
||||
<RoutineDetailView
|
||||
initialRoutine={{ ...BASE_ROUTINE, catchUpPolicy: "summarize_missed", catchUpMax: 25 }}
|
||||
initialTriggers={NO_TRIGGERS}
|
||||
/>,
|
||||
renderDetailView(
|
||||
{ ...baseRoutine, catchUpPolicy: "summarize_missed", catchUpMax: 25 },
|
||||
NO_TRIGGERS,
|
||||
);
|
||||
// Comboboxes in DOM order: status, assignee, concurrency policy,
|
||||
// catch-up policy, then the trigger card's kind select.
|
||||
|
||||
@@ -21,6 +21,8 @@ import {
|
||||
import type { Routine, RoutineTrigger } from "@/lib/state/slices/office/types";
|
||||
import { timeAgo } from "@/lib/utils/time";
|
||||
import { useOfficeTopbar } from "../../components/office-topbar-context";
|
||||
import { isRoutineFiring } from "../../lib/routine-status";
|
||||
import { routineNotFiringMessage } from "../../lib/routine-not-firing";
|
||||
import { useTranslation } from "react-i18next";
|
||||
|
||||
// Lift the form state out of the component so the file stays under the
|
||||
@@ -113,7 +115,7 @@ export function RoutineDetailView({ initialRoutine, initialTriggers }: RoutineDe
|
||||
await runRoutine(routine.id);
|
||||
toast.success(t("office:routineFired"));
|
||||
} catch (err) {
|
||||
toast.error(err instanceof Error ? err.message : t("office:failedToRunRoutine"));
|
||||
toast.error(routineNotFiringMessage(err, t, "office:failedToRunRoutine"));
|
||||
}
|
||||
}, [routine.id]);
|
||||
|
||||
@@ -139,7 +141,10 @@ export function RoutineDetailView({ initialRoutine, initialTriggers }: RoutineDe
|
||||
<div className="p-6 space-y-6 max-w-3xl">
|
||||
<DetailGeneralCard draft={draft} update={update} agents={agents} />
|
||||
<DetailTriggerCard draft={draft} update={update} />
|
||||
<DetailReadOnlyCard lastFiredAt={lastFired} nextRunAt={cronTrigger?.nextRunAt ?? null} />
|
||||
<DetailReadOnlyCard
|
||||
lastFiredAt={lastFired}
|
||||
nextRunAt={isRoutineFiring(draft.status) ? (cronTrigger?.nextRunAt ?? null) : null}
|
||||
/>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -23,6 +23,7 @@ import { timeAgo } from "@/lib/utils/time";
|
||||
import type { TFunction } from "i18next";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { CONCURRENCY_POLICY_LABEL_KEYS } from "../lib/label-keys";
|
||||
import { isRoutineFiring } from "../lib/routine-status";
|
||||
|
||||
/**
|
||||
* A routine's concurrency policy is a wire value; only its label is copy.
|
||||
@@ -82,14 +83,15 @@ export function RoutineRow({
|
||||
const concurrencyPolicy =
|
||||
routine.concurrencyPolicy ?? (routineRaw.concurrency_policy as string | undefined) ?? "";
|
||||
const assignee = agents.find((a) => a.id === assigneeId);
|
||||
const isActive = routine.status === "active";
|
||||
const isActive = isRoutineFiring(routine.status);
|
||||
const template = routine.taskTemplate as { title?: string; description?: string } | undefined;
|
||||
const cronTrigger = triggers.find((t) => t.kind === "cron");
|
||||
const nextFire = nextFireText(t, triggers);
|
||||
const nextFire = isActive ? nextFireText(t, triggers) : "";
|
||||
|
||||
return (
|
||||
<div>
|
||||
<div
|
||||
data-testid={`routine-row-${routine.id}`}
|
||||
className="flex items-center gap-3 px-4 py-2.5 hover:bg-accent/50 transition-colors cursor-pointer"
|
||||
onClick={() => onClick(routine.id)}
|
||||
>
|
||||
@@ -174,6 +176,7 @@ function RoutineActions({ onRunNow, onDelete }: { onRunNow: () => void; onDelete
|
||||
</Tooltip>
|
||||
<DropdownMenuContent align="end">
|
||||
<DropdownMenuItem
|
||||
data-testid="routine-run-now"
|
||||
className="cursor-pointer"
|
||||
onClick={(e) => {
|
||||
e.stopPropagation();
|
||||
|
||||
@@ -27,6 +27,7 @@ import { RoutineRow } from "./routine-row";
|
||||
import { RunRow } from "./run-row";
|
||||
import { CreateRoutineDialog } from "./create-routine-dialog";
|
||||
import { EmptyState } from "../components/shared/empty-state";
|
||||
import { routineNotFiringMessage } from "../lib/routine-not-firing";
|
||||
import { useTranslation } from "react-i18next";
|
||||
|
||||
type RoutineFormData = {
|
||||
@@ -204,7 +205,7 @@ export function RoutinesContent() {
|
||||
setRuns(await fetchRuns());
|
||||
toast.success(t("office:routineStarted"));
|
||||
} catch (err) {
|
||||
toast.error(err instanceof Error ? err.message : t("office:failedToRunRoutine"));
|
||||
toast.error(routineNotFiringMessage(err, t, "office:failedToRunRoutine"));
|
||||
}
|
||||
},
|
||||
[fetchRuns, setRuns],
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import { test, expect } from "../../fixtures/office-fixture";
|
||||
import { waitForHttp } from "../../helpers/causal-waits";
|
||||
|
||||
test.describe("Routines UI", () => {
|
||||
test("routine created via API appears in page", async ({ testPage, officeApi, officeSeed }) => {
|
||||
@@ -8,4 +9,59 @@ test.describe("Routines UI", () => {
|
||||
await testPage.goto("/office/routines");
|
||||
await expect(testPage.getByText("E2E Test Routine")).toBeVisible({ timeout: 10_000 });
|
||||
});
|
||||
|
||||
// A paused routine's cron-suppression behavior is covered by Go unit
|
||||
// tests; this spec covers only the UI surfaces the frontend owns: the
|
||||
// row reflects the paused status, and "Run Now" from either surface is
|
||||
// refused with the status-gated toast.
|
||||
test("paused routine shows off and refuses run now", async ({
|
||||
testPage,
|
||||
apiClient,
|
||||
officeApi,
|
||||
officeSeed,
|
||||
}) => {
|
||||
const routine = (await officeApi.createRoutine(officeSeed.workspaceId, {
|
||||
name: "E2E Paused Routine",
|
||||
})) as { id: string };
|
||||
expect(routine.id).toBeTruthy();
|
||||
|
||||
await testPage.goto("/office/routines");
|
||||
const row = testPage.getByTestId(`routine-row-${routine.id}`);
|
||||
await expect(row).toBeVisible({ timeout: 10_000 });
|
||||
await expect(row.getByText("On", { exact: true })).toBeVisible({ timeout: 10_000 });
|
||||
|
||||
const paused = await apiClient.rawRequest("PATCH", `/api/v1/office/routines/${routine.id}`, {
|
||||
status: "paused",
|
||||
});
|
||||
expect(paused.ok).toBe(true);
|
||||
|
||||
await testPage.reload();
|
||||
await expect(row).toBeVisible({ timeout: 10_000 });
|
||||
await expect(row.getByText("Off", { exact: true })).toBeVisible({ timeout: 10_000 });
|
||||
|
||||
// DropdownMenuContent renders in a portal outside the row's DOM
|
||||
// subtree, so the trigger is found scoped to the row but the menu
|
||||
// item itself is located page-wide once open.
|
||||
const rowRunRefused = waitForHttp(testPage, "POST", /\/routines\/[^/]+\/run$/, {
|
||||
predicate: (r) => r.status() === 409,
|
||||
});
|
||||
await row.getByRole("button").last().click();
|
||||
await testPage.getByTestId("routine-run-now").click();
|
||||
await rowRunRefused;
|
||||
await expect(testPage.getByText(/Cannot run: routine status is paused/i).first()).toBeVisible({
|
||||
timeout: 10_000,
|
||||
});
|
||||
|
||||
await testPage.goto(`/office/routines/${routine.id}`);
|
||||
await expect(testPage.getByText(/E2E Paused Routine/).first()).toBeVisible({ timeout: 10_000 });
|
||||
|
||||
const detailRunRefused = waitForHttp(testPage, "POST", /\/routines\/[^/]+\/run$/, {
|
||||
predicate: (r) => r.status() === 409,
|
||||
});
|
||||
await testPage.getByRole("button", { name: "Run now" }).click();
|
||||
await detailRunRefused;
|
||||
await expect(testPage.getByText(/Cannot run: routine status is paused/i).first()).toBeVisible({
|
||||
timeout: 10_000,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -724,6 +724,7 @@
|
||||
"routineCreated": "Routine created",
|
||||
"routineDeleted": "Routine deleted",
|
||||
"routineFired": "Routine fired",
|
||||
"routineNotFiring": "Cannot run: routine status is {{status}}",
|
||||
"routinePaused": "Routine paused",
|
||||
"routines": "Routines",
|
||||
"routinesAutomaticallyCreateTasksOnA": "Routines automatically create tasks on a schedule or webhook trigger.",
|
||||
|
||||
@@ -724,6 +724,7 @@
|
||||
"routineCreated": "Ŕōũţĩńē ćŕēàţēď",
|
||||
"routineDeleted": "Ŕōũţĩńē ďēĺēţēď",
|
||||
"routineFired": "Ŕōũţĩńē ƒĩŕēď",
|
||||
"routineNotFiring": "Ćàńńōţ ŕũń: ŕōũţĩńē śţàţũś ĩś {{status}}",
|
||||
"routinePaused": "Ŕōũţĩńē ƥàũśēď",
|
||||
"routines": "Ŕōũţĩńēś",
|
||||
"routinesAutomaticallyCreateTasksOnA": "Ŕōũţĩńēś àũţōḿàţĩćàĺĺŷ ćŕēàţē ţàśķś ōń à śćĥēďũĺē ōŕ ŵēƀĥōōķ ţŕĩĝĝēŕ.",
|
||||
|
||||
@@ -724,6 +724,7 @@
|
||||
"routineCreated": "Rotina criada",
|
||||
"routineDeleted": "Rotina eliminada",
|
||||
"routineFired": "Rotina disparada",
|
||||
"routineNotFiring": "Não é possível executar: o estado da rotina é {{status}}",
|
||||
"routinePaused": "Rotina pausada",
|
||||
"routines": "Rotinas",
|
||||
"routinesAutomaticallyCreateTasksOnA": "As rotinas criam tarefas automaticamente com base num agendamento ou num acionador de webhook.",
|
||||
|
||||
@@ -724,6 +724,7 @@
|
||||
"routineCreated": "例行程序已创建",
|
||||
"routineDeleted": "例行程序已删除",
|
||||
"routineFired": "例行程序已触发",
|
||||
"routineNotFiring": "无法运行：例行程序状态为 {{status}}",
|
||||
"routinePaused": "例行程序已暂停",
|
||||
"routines": "例行程序",
|
||||
"routinesAutomaticallyCreateTasksOnA": "例行程序按计划或 webhook 触发自动创建任务。",
|
||||
|
||||
@@ -724,6 +724,7 @@
|
||||
"routineCreated": "例行程序已建立",
|
||||
"routineDeleted": "例行程序已刪除",
|
||||
"routineFired": "例行程序已觸發",
|
||||
"routineNotFiring": "無法執行：例行程序狀態為 {{status}}",
|
||||
"routinePaused": "例行程序已暫停",
|
||||
"routines": "例行程序",
|
||||
"routinesAutomaticallyCreateTasksOnA": "例行程序按計劃或 webhook 觸發自動建立任務。",
|
||||
|
||||
@@ -724,6 +724,7 @@
|
||||
"routineCreated": "例行程式已建立",
|
||||
"routineDeleted": "例行程式已刪除",
|
||||
"routineFired": "例行程式已觸發",
|
||||
"routineNotFiring": "無法執行：例行程式狀態為 {{status}}",
|
||||
"routinePaused": "例行程式已暫停",
|
||||
"routines": "例行程式",
|
||||
"routinesAutomaticallyCreateTasksOnA": "例行程式按計劃或 webhook 觸發自動建立任務。",
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
---
|
||||
created: 2026-09-08
|
||||
status: done
|
||||
requirements:
|
||||
- REQ-OFFICE-ROUTINE-STATUS-001
|
||||
- REQ-OFFICE-ROUTINE-STATUS-002
|
||||
- REQ-OFFICE-ROUTINE-STATUS-003
|
||||
- REQ-OFFICE-ROUTINE-STATUS-004
|
||||
- REQ-OFFICE-ROUTINE-STATUS-006
|
||||
system_design:
|
||||
- ../../specs/office/system-design/routine-status-gating.md
|
||||
legacy_specs: []
|
||||
---
|
||||
|
||||
# Implementation Plan: Office Routine Status Gating
|
||||
|
||||
## Overview
|
||||
|
||||
Routine status must control every routine fire path. This plan records the
|
||||
delivery package for cron suppression, manual and webhook refusals, and the
|
||||
frontend status surfaces.
|
||||
|
||||
## Scope
|
||||
|
||||
- Read routine status before a cron trigger claim.
|
||||
- Advance suppressed cron cursors without creating fire evidence.
|
||||
- Refuse manual and webhook fires for non-firing routines.
|
||||
- Keep refusal responses and frontend messages localizable.
|
||||
- Keep routine list and detail views consistent with the firing status.
|
||||
|
||||
## Work orders
|
||||
|
||||
- [Task 01: Enforce routine status gating](task-01-enforce-routine-status-gating.md)
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
cd apps/backend
|
||||
go test ./internal/office/routines
|
||||
cd ../..
|
||||
python3 scripts/lint-spec-files.py --all
|
||||
```
|
||||
@@ -0,0 +1,91 @@
|
||||
---
|
||||
id: "01-enforce-routine-status-gating"
|
||||
title: "Enforce routine status gating"
|
||||
status: done
|
||||
wave: 1
|
||||
depends_on: []
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-OFFICE-ROUTINE-STATUS-001
|
||||
- REQ-OFFICE-ROUTINE-STATUS-002
|
||||
- REQ-OFFICE-ROUTINE-STATUS-003
|
||||
- REQ-OFFICE-ROUTINE-STATUS-004
|
||||
- REQ-OFFICE-ROUTINE-STATUS-006
|
||||
acceptance_criteria:
|
||||
- AC-OFFICE-ROUTINE-STATUS-001.1
|
||||
- AC-OFFICE-ROUTINE-STATUS-001.2
|
||||
- AC-OFFICE-ROUTINE-STATUS-001.3
|
||||
- AC-OFFICE-ROUTINE-STATUS-001.4
|
||||
- AC-OFFICE-ROUTINE-STATUS-001.5
|
||||
- AC-OFFICE-ROUTINE-STATUS-001.6
|
||||
- AC-OFFICE-ROUTINE-STATUS-001.7
|
||||
- AC-OFFICE-ROUTINE-STATUS-001.8
|
||||
- AC-OFFICE-ROUTINE-STATUS-002.1
|
||||
- AC-OFFICE-ROUTINE-STATUS-002.2
|
||||
- AC-OFFICE-ROUTINE-STATUS-002.3
|
||||
- AC-OFFICE-ROUTINE-STATUS-002.4
|
||||
- AC-OFFICE-ROUTINE-STATUS-002.5
|
||||
- AC-OFFICE-ROUTINE-STATUS-002.6
|
||||
- AC-OFFICE-ROUTINE-STATUS-002.7
|
||||
- AC-OFFICE-ROUTINE-STATUS-002.8
|
||||
- AC-OFFICE-ROUTINE-STATUS-002.10
|
||||
- AC-OFFICE-ROUTINE-STATUS-003.1
|
||||
- AC-OFFICE-ROUTINE-STATUS-003.2
|
||||
- AC-OFFICE-ROUTINE-STATUS-003.3
|
||||
- AC-OFFICE-ROUTINE-STATUS-003.4
|
||||
- AC-OFFICE-ROUTINE-STATUS-003.5
|
||||
- AC-OFFICE-ROUTINE-STATUS-003.6
|
||||
- AC-OFFICE-ROUTINE-STATUS-004.1
|
||||
- AC-OFFICE-ROUTINE-STATUS-004.2
|
||||
- AC-OFFICE-ROUTINE-STATUS-004.3
|
||||
- AC-OFFICE-ROUTINE-STATUS-004.4
|
||||
- AC-OFFICE-ROUTINE-STATUS-004.5
|
||||
- AC-OFFICE-ROUTINE-STATUS-004.6
|
||||
- AC-OFFICE-ROUTINE-STATUS-004.7
|
||||
- AC-OFFICE-ROUTINE-STATUS-006.1
|
||||
- AC-OFFICE-ROUTINE-STATUS-006.2
|
||||
- AC-OFFICE-ROUTINE-STATUS-006.3
|
||||
- AC-OFFICE-ROUTINE-STATUS-006.4
|
||||
- AC-OFFICE-ROUTINE-STATUS-006.5
|
||||
- AC-OFFICE-ROUTINE-STATUS-006.6
|
||||
system_design:
|
||||
- ../../specs/office/system-design/routine-status-gating.md
|
||||
---
|
||||
|
||||
# Task 01: Enforce routine status gating
|
||||
|
||||
## Summary
|
||||
|
||||
Make routine status a load-bearing gate for cron, manual, and webhook fires.
|
||||
Suppressed cron slots advance their cursor without writing fire evidence. The
|
||||
routine list and detail views show the same firing rule as the backend.
|
||||
|
||||
## In scope
|
||||
|
||||
- Add the shared firing-status predicate and the compare-and-set cursor update.
|
||||
- Read and decide routine status before claiming a cron trigger.
|
||||
- Return a typed refusal for manual and webhook requests.
|
||||
- Localize refusal status labels while preserving unknown values for diagnosis.
|
||||
- Cover status transitions, cursor behavior, API responses, and UI surfaces.
|
||||
- Keep the routine cron calculation on the shared `NextCronTime` helper.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- A workspace-wide kill switch.
|
||||
- Changes to routine storage schema or trigger ownership.
|
||||
- Changes to the wakeup dispatcher concurrency policy.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
(cd apps/backend && go test ./internal/office/routines)
|
||||
(cd apps && pnpm --filter @kandev/web test -- --run app/office/lib/routine-not-firing.test.ts)
|
||||
python3 scripts/lint-spec-files.py --all
|
||||
```
|
||||
|
||||
## Results
|
||||
|
||||
The routine service gates all fire paths on status. Suppressed cron cursors use
|
||||
the shared cron helper and leave no run, wakeup, task, or fire timestamp. The
|
||||
web client translates known refusal statuses and keeps unknown values visible.
|
||||
The focused backend and frontend tests and specification lint pass.
|
||||
@@ -0,0 +1,322 @@
|
||||
---
|
||||
status: active
|
||||
system: office
|
||||
created: 2026-09-08
|
||||
owners:
|
||||
- kandev
|
||||
---
|
||||
|
||||
# Office Routine Status Gating Requirements
|
||||
|
||||
## Overview
|
||||
|
||||
`office_routines.status` accepts `active`, `paused` and `archived`, and nothing that
|
||||
fires a routine reads it. A scheduled routine dispatches on its cron slot whatever its
|
||||
status, and so do the manual fire (`POST /api/v1/office/routines/:id/run`) and the
|
||||
webhook fire (`POST /api/v1/office/routine-triggers/:publicId/fire`). An operator who
|
||||
pauses a routine gets a persisted status, an "Off" badge, and a routine that keeps
|
||||
launching agents.
|
||||
|
||||
This capability makes `status` load-bearing at every point a routine can fire. It is
|
||||
the primitive an Office-wide kill switch needs, not that switch.
|
||||
|
||||
## Terminology
|
||||
|
||||
- **Slot:** one scheduled firing time from a cron expression.
|
||||
- **Fire:** dispatching a routine run: an `office_routine_runs` row, then a wakeup
|
||||
request (lightweight routine) or a task (heavy routine).
|
||||
- **Suppress:** decline a slot on status, leaving no run row, wakeup or task.
|
||||
- **Cursor:** `office_routine_triggers.next_run_at`, the single persisted value
|
||||
deciding when a cron trigger is next due.
|
||||
- **Firing status:** the statuses under which a routine may fire: `active` plus the
|
||||
empty string.
|
||||
- **Outage catch-up:** the existing `catch_up_policy` / `catch_up_max` behavior
|
||||
replaying slots missed because the backend was down.
|
||||
|
||||
## Requirements
|
||||
|
||||
### REQ-OFFICE-ROUTINE-STATUS-001: A non-firing routine does not fire on its schedule
|
||||
|
||||
**Intent:** Pause is the operator's only means of stopping a routine short of
|
||||
deleting it. It has to actually stop it, on every path, or nothing built on top of
|
||||
it can be trusted.
|
||||
|
||||
**User story:** As an operator, I want a paused routine to stop launching agents, so
|
||||
pausing is a real control and not a label.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-ROUTINE-STATUS-001.1:** When a cron trigger is due and its routine's
|
||||
status is `paused`, the system shall not fire the slot.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-001.2:** When a cron trigger is due and its routine's
|
||||
status is `archived`, the system shall not fire the slot.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-001.3:** When a cron trigger is due and its routine's
|
||||
status is any value other than `active`, `paused`, `archived`, or the empty
|
||||
string, the system shall not fire the slot. Membership is decided on the stored
|
||||
bytes, with no case folding and no trimming, so `Active` and `" active"` do not fire.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-001.4:** When a cron trigger is due and its routine's
|
||||
status is `active` or the empty string, the system shall fire the slot with the
|
||||
same run row, wakeup payload, task creation, concurrency policy and
|
||||
outage-catch-up attribution it produces today.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-001.5:** When a slot is suppressed, the system shall
|
||||
create no `office_routine_runs` row for it, including no row with status
|
||||
`skipped` or `cancelled`.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-001.6:** When a slot is suppressed, the system shall
|
||||
create no agent wakeup request and no task.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-001.7:** When a slot is suppressed, the system shall
|
||||
leave the trigger's `last_fired_at` and the routine's `last_run_at` unchanged.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-001.8:** When a slot is suppressed and its cursor is
|
||||
advanced, the system shall record the routine id, trigger id, and observed status
|
||||
in a structured log entry. This entry is not deduplicated: it occurs at most once
|
||||
per scheduled slot, so its volume is the routine's own schedule. A suppression whose
|
||||
cursor could not be advanced is recorded under -002.8 instead, which -003.6 bounds.
|
||||
|
||||
### REQ-OFFICE-ROUTINE-STATUS-002: Suppression drops the slot and never banks it
|
||||
|
||||
**Intent:** A pause is an operator decision, not an outage. If suppressed slots
|
||||
accumulated as missed ticks, resuming a routine, or releasing a kill switch, would
|
||||
fire up to `catch_up_max` runs at once: the control would trade a runaway loop for a
|
||||
burst.
|
||||
|
||||
**User story:** As an operator, I want resuming a paused routine to start it at its
|
||||
next scheduled time, so the pause does not become a backlog that fires all at once.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-ROUTINE-STATUS-002.1:** When a slot is suppressed, the system shall
|
||||
set the trigger's cursor to the first slot produced by its cron expression and
|
||||
timezone strictly after the evaluation time. The value written shall be a time
|
||||
that expression names; a computed time that does not match it shall be handled as
|
||||
a failure to compute (-002.8) rather than stored.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-002.2:** When a routine returns to a firing status, the
|
||||
first fire shall occur at the first slot due at or after the cursor left by the last
|
||||
suppressed evaluation, and no earlier.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-002.3:** When a routine is returned to a firing status
|
||||
and every one of its cron cursors is at a time in the future at the moment of the
|
||||
resume, the first fire shall report zero missed ticks, regardless of how long it was
|
||||
suppressed or what its `catch_up_policy` and `catch_up_max` are. Any suppressing
|
||||
evaluation leaves a cursor in the future (-002.1); one still in the past means no
|
||||
evaluation has run since the pause.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-002.4:** When the backend was not running for part of
|
||||
a suppression window, the first fire after the routine returns to a firing status
|
||||
shall report zero missed ticks for slots inside that window, under the same
|
||||
future-cursor condition as -002.3. One suppressing evaluation suffices however long
|
||||
the outage: the advance in -002.1 is measured from the evaluation time, not the old
|
||||
cursor, so it collapses the backlog rather than walking it.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-002.5:** When a routine holds a firing status, the
|
||||
system shall apply `catch_up_policy` and `catch_up_max` exactly as today, with no
|
||||
change to counts, cursor placement or reported missed ticks.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-002.6:** When two evaluations race on the same due
|
||||
slot of a routine that does not hold a firing status, the system shall advance
|
||||
the cursor at most once and shall fire nothing.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-002.7:** When an evaluation observes a cursor that
|
||||
another evaluation has already advanced, the system shall take no action for
|
||||
that slot.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-002.8:** When the new cursor cannot be computed or
|
||||
cannot be written, the system shall still not fire, shall leave the stored
|
||||
cursor unchanged, and shall record the failure in a structured log entry. The
|
||||
trigger remains due and is suppressed again next tick, subject to the repeat
|
||||
bound in -003.6. "Cannot be computed" covers a malformed expression, an unloadable
|
||||
timezone, and an expression naming no slot within the search horizon.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-002.10:** When two evaluations race on the same due
|
||||
slot of a routine that holds a firing status, exactly one shall fire it and the
|
||||
other shall take no action. Unchanged from today.
|
||||
|
||||
### REQ-OFFICE-ROUTINE-STATUS-003: The decision is made before the trigger is claimed
|
||||
|
||||
**Intent:** Claiming a trigger records a fire: it stamps `last_fired_at` and clears
|
||||
the cursor. A suppressed slot must never leave that evidence, and a slot the system
|
||||
could not decide must never be left un-armed.
|
||||
|
||||
**User story:** As an operator, I want a routine the scheduler could not evaluate to
|
||||
be retried rather than silently retired, so a transient read failure does not
|
||||
permanently kill a schedule.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-ROUTINE-STATUS-003.1:** When a due trigger is evaluated, the system
|
||||
shall determine the routine's status before performing any state change that
|
||||
records a fire.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-003.2:** When the routine backing a due trigger
|
||||
cannot be read, the system shall not fire, shall leave the cursor unchanged so the
|
||||
trigger remains due, and shall log the error as observed, subject to the repeat
|
||||
bound in -003.6. The entry shall not assert the routine was deleted: the read path
|
||||
reports an absent row and a failed read identically.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-003.3:** When the routine backing a due trigger does
|
||||
not exist, the system shall not fire, shall leave the cursor unchanged, and shall
|
||||
not disarm, delete or otherwise modify the trigger, because the read that would
|
||||
justify doing so cannot distinguish a deleted routine from a failed read. Such a
|
||||
trigger is cleared by the existing orphan reconciler, not by this path.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-003.4:** A status write shall govern only slots
|
||||
evaluated after it commits: a slot already past the gate completes its fire even if
|
||||
the routine is paused immediately afterwards, and a slot already suppressed stays
|
||||
suppressed even if it is resumed immediately afterwards.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-003.5:** Evaluating one due trigger shall not change
|
||||
the outcome of any other due trigger in the same tick. For a fixed set of routine
|
||||
statuses, the set of slots fired and the set suppressed shall be identical under
|
||||
any evaluation order. A status write committing while a tick is in flight is not
|
||||
a fixed set: per AC-OFFICE-ROUTINE-STATUS-003.4 it may land on either side of any
|
||||
trigger's evaluation, and which triggers observe it is deliberately unspecified.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-003.6:** The two outcomes that leave a trigger due
|
||||
without advancing it, an unreadable routine (-003.2) and a cursor that could not be
|
||||
computed or written (-002.8), shall each be logged at most once per trigger per
|
||||
process. The comparison key is the trigger id and which of those two outcomes it
|
||||
is, and nothing else: no observed status, no error value. Both outcomes leave the
|
||||
trigger due, so it is re-evaluated every tick and an unbounded rule would write one
|
||||
entry every tick for the life of the process. The bound covers the logging only,
|
||||
not the repeated evaluation, and the state backing it is per-process, so a restart
|
||||
logs each outcome once more.
|
||||
|
||||
### REQ-OFFICE-ROUTINE-STATUS-004: Manual and webhook fires respect status
|
||||
|
||||
**Intent:** The cron path is the loudest hole, not the only one. A pause holding on
|
||||
the schedule but not on "Run now" or an inbound webhook makes the operator work out
|
||||
which door an unwanted run came through.
|
||||
|
||||
**User story:** As an operator, I want every way of firing a routine to respect its
|
||||
status, so pausing means one thing everywhere.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-ROUTINE-STATUS-004.1:** When a manual fire targets a routine without
|
||||
a firing status, the system shall refuse the request with HTTP 409 and shall
|
||||
create no run row, wakeup request, or task.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-004.2:** When a webhook fire targets a trigger whose
|
||||
routine has no firing status, the system shall refuse the request with HTTP 409
|
||||
and shall create no run row, wakeup request, or task.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-004.3:** When an enabled webhook fire has an invalid
|
||||
signature, the system shall refuse it on the signature regardless of routine
|
||||
status, so the response does not disclose status. Disabled triggers retain their
|
||||
existing refusal precedence.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-004.4:** When a fire is refused on status, the
|
||||
response body shall name the observed status and shall carry a stable
|
||||
machine-readable code identifying a status refusal, distinct from every other
|
||||
refusal the same routes can return. The code is what lets a surface render its own
|
||||
localized message (-006.4, -006.6) instead of displaying a server string.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-004.5:** When a manual or webhook fire targets a
|
||||
routine with a firing status, the system shall behave exactly as it does today.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-004.6:** A refused fire shall leave the routine's
|
||||
`last_run_at` and every trigger's cursor and `last_fired_at` unchanged.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-004.7:** A manual or webhook fire shall read the
|
||||
routine's status once and that decision shall govern the whole request. A status
|
||||
write committing after the read shall neither cancel a dispatch already permitted
|
||||
nor rescue one already refused, and the system shall not re-read the status to
|
||||
narrow that window. With AC-OFFICE-ROUTINE-STATUS-003.4, which covers cron slots
|
||||
only, this makes all three fire paths decide once, at the gate.
|
||||
|
||||
### REQ-OFFICE-ROUTINE-STATUS-006: The routine surfaces do not promise a fire that will not happen
|
||||
|
||||
**Intent:** Suppression advances the cursor, so a paused routine keeps a populated
|
||||
`next_run_at` that the list row renders as a countdown for a fire that never comes.
|
||||
|
||||
**User story:** As an operator, I want a paused routine's row to show no next fire,
|
||||
so the screen matches what the scheduler will do.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-ROUTINE-STATUS-006.1:** When a routine does not hold a firing
|
||||
status, neither the routines list nor the routine detail view shall render a
|
||||
next-fire countdown for it.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-006.2:** When a routine holds a firing status, both
|
||||
shall render the next-fire countdown exactly as they do today.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-006.3:** When a routine's status is `paused` or
|
||||
`archived`, the list row's enabled toggle and badge shall read as off.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-006.4:** When a fire is refused on status, the
|
||||
surface that requested it shall show a message naming the routine's status
|
||||
rather than a generic failure, selected from the code in -004.4 rather than from
|
||||
the response's human-readable string.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-006.5:** When a routine's status is the empty string,
|
||||
the list row shall read as on and render its next-fire countdown, matching the
|
||||
scheduler's treatment of that value. Reading a routine shall report the stored
|
||||
status verbatim rather than substituting a replacement, so the surface observes the
|
||||
value the scheduler observes.
|
||||
- **AC-OFFICE-ROUTINE-STATUS-006.6:** Every string added by this capability and
|
||||
shown to an operator shall be localized through the existing i18n pipeline in all
|
||||
supported locales. A server-side message carried in a response body is a fallback
|
||||
for a surface that has no localized copy, not the localized copy itself.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- **The Office-wide kill switch** (card `b73f82b6`). This supplies the per-routine
|
||||
primitive only: no workspace flag, no bulk status write, no single control. A
|
||||
follow-up needs to know only that `paused` now stops every fire path and resuming
|
||||
banks nothing.
|
||||
- **Changing outage catch-up.** `catch_up_policy`, `catch_up_max` and their missed
|
||||
tick count belong to card `1a06962a`, constrained here one way only: a suppression
|
||||
window never contributes missed ticks.
|
||||
- **Collapsing a stale cursor when a routine is resumed** (cut from this capability
|
||||
during spec review; a follow-up would own it). Suppression only advances a cursor
|
||||
when a tick actually runs, and `cron.Loop` does not tick at startup: its first tick
|
||||
fires one full `DefaultTickInterval` (30s) in, and its own comment tells handlers
|
||||
not to rely on a startup tick. So after every restart there is a window in which a
|
||||
routine paused before the restart still holds a cursor deep in the past. Resuming
|
||||
inside that window reaches the firing path with that cursor intact, and
|
||||
`computeRoutineMissed` walks the whole paused interval and reports up to
|
||||
`catch_up_max - 1` missed ticks on that one fire. The same applies to a status write
|
||||
that does not go through the Office API at all, such as a direct database edit.
|
||||
AC-OFFICE-ROUTINE-STATUS-002.3 and -002.4 are therefore conditioned on a suppressing
|
||||
evaluation having run. Accepted here: a stale missed count on exactly one fire, never
|
||||
a routine that stops firing. A follow-up that closes it should know three things.
|
||||
First, the repair must advance the cursor and never clear it, so that a skipped or
|
||||
failed repair degrades to this same stale count rather than to a routine that never
|
||||
fires again. Second, it must be best-effort and outside the status write's
|
||||
transaction, because a cursor write failure must never refuse an operator's resume.
|
||||
Third, a routine may hold several cron triggers on different expressions and
|
||||
timezones (nothing in the schema or the create path constrains it to one), so the
|
||||
repair is a loop of per-trigger compare-and-sets, each computing its own next slot;
|
||||
a single bulk `UPDATE ... WHERE next_run_at <= ?` would cross-assign one trigger's
|
||||
next-fire time onto another.
|
||||
- **Validating the status value at the writer** (cut from this capability during spec
|
||||
review; a follow-up would own it). The gate reads an allowlist, so an unrecognized
|
||||
stored status stops a routine (-001.3). Rejecting unrecognized values on write would
|
||||
turn a typo into a 400 the operator sees rather than a routine that quietly stopped,
|
||||
and would make the value set closed rather than merely filtered. It is cut because
|
||||
it drags three further problems in with it, all of which a follow-up must solve
|
||||
together rather than one at a time. First, `PATCH /api/v1/office/routines/:id`
|
||||
resolves through a read-modify-write with no version column, and `UpdateRoutine`
|
||||
writes the whole row, so two concurrent callers lose each other's status writes;
|
||||
once status is load-bearing that lost update silently resumes a paused routine, and
|
||||
a validation AC that does not also state a concurrency rule does not deliver the
|
||||
guarantee it appears to. Second, `office/service/config_import.go` `applyRoutines`
|
||||
updates an existing routine through that same full-row write, reached from config
|
||||
sync, so it writes the status column too and preserves a pause only by echoing a
|
||||
fresh read; the other two bulk writers genuinely omit the column via
|
||||
`UpdateRoutineConfigFields`, so the three paths do not share one mechanism and need
|
||||
separate treatment. Third, once the endpoint rejects an out-of-set value, the detail
|
||||
view becomes a trap: it seeds its draft with `routine.status ?? "active"`, which does
|
||||
not catch the empty string, and submits `status` on every save, so a routine holding
|
||||
a value outside the set becomes uneditable in every other field. The fix belongs at
|
||||
the surface (omit an unchanged status from the update) rather than at the writer,
|
||||
because exempting the stored value at the writer would let any unrecognized value
|
||||
survive a round-trip, which is the trap the validation exists to close.
|
||||
- **Stopping a run already in flight.** Pausing does not cancel a dispatched run;
|
||||
that is agent-level pause's contract (`guardAgentStatus`). The window in
|
||||
AC-OFFICE-ROUTINE-STATUS-003.4 and -004.7 is accepted: closing it needs a second
|
||||
gate in the wakeup dispatcher, which narrows it without eliminating it.
|
||||
- **A force-run override.** "Run now" is refused, not read as a human override. The
|
||||
route back is to set `active`. An override may be added later, but must not arrive
|
||||
by omission, which is how this defect existed.
|
||||
- **Disarming or deleting triggers on archive.** `archived` and `paused` suppress
|
||||
identically; archiving deletes no triggers or history, so un-archiving restores
|
||||
the schedule with no repair step.
|
||||
- **Reaping the triggers of a deleted routine.** Per AC-OFFICE-ROUTINE-STATUS-003.3
|
||||
this capability neither deletes nor disarms them. Deleting a routine already
|
||||
cascades to its triggers wherever the database enforces foreign keys, so the orphan
|
||||
is rare; where one exists it costs one log entry per process (-003.6) plus one
|
||||
routine lookup per tick until the startup reconciler clears it. Rescheduling that
|
||||
reconciler, and giving the gate a read that tells a deleted routine from an
|
||||
unreadable one, both belong to that follow-up.
|
||||
- **Ordering of due-trigger evaluation.** `GetDueTriggers` has no `ORDER BY` and this
|
||||
capability adds none, which is why AC-OFFICE-ROUTINE-STATUS-003.5 is written as
|
||||
independence between triggers rather than as a fixed order.
|
||||
- **The manual fire route's error mapping beyond the status refusal.** Naming a
|
||||
routine that does not exist still returns 500, not 404. Pre-existing; only the
|
||||
status refusal and its code are added here.
|
||||
- **Status-aware dashboard projections**, **bulk or scheduled status changes**, and
|
||||
**expvar counters for suppressed slots**. No status breakdown in rollups, no
|
||||
"pause all", no scheduled pause window, no auto-pause on failure; logs only.
|
||||
|
||||
## System design
|
||||
|
||||
The technical design is in
|
||||
[Office Routine Status Gating System Design](../system-design/routine-status-gating.md).
|
||||
@@ -0,0 +1,473 @@
|
||||
---
|
||||
status: current
|
||||
system: office
|
||||
requirements:
|
||||
- REQ-OFFICE-ROUTINE-STATUS-001
|
||||
- REQ-OFFICE-ROUTINE-STATUS-002
|
||||
- REQ-OFFICE-ROUTINE-STATUS-003
|
||||
- REQ-OFFICE-ROUTINE-STATUS-004
|
||||
- REQ-OFFICE-ROUTINE-STATUS-006
|
||||
---
|
||||
|
||||
# Office Routine Status Gating System Design
|
||||
|
||||
## Purpose and boundaries
|
||||
|
||||
Office owns `office_routines.status`, the `office_routine_triggers.next_run_at`
|
||||
cursor, and all three routine fire paths. This design makes status the single
|
||||
gate those paths consult, and defines what a suppressed slot does to the cursor.
|
||||
|
||||
Adjacent contracts this design uses but does not own:
|
||||
|
||||
- The shared cron loop (`internal/scheduler/cron`) drives `TickScheduledTriggers`
|
||||
every `DefaultTickInterval` (30s). It owns cadence, not routine policy.
|
||||
- The wakeup dispatcher (`internal/office/wakeup`) turns a routine wakeup request
|
||||
into a run. It reads the routine only for its concurrency policy and gains no
|
||||
status responsibility here.
|
||||
- Outage catch-up (`computeRoutineMissed`) is owned by the routine catch-up
|
||||
capability. This design calls it only on the firing path.
|
||||
|
||||
## Requirement mapping
|
||||
|
||||
| Requirement | Design section |
|
||||
| --- | --- |
|
||||
| `REQ-OFFICE-ROUTINE-STATUS-001` | [The firing-status predicate](#the-firing-status-predicate) |
|
||||
| `REQ-OFFICE-ROUTINE-STATUS-002` | [Cursor advance on suppression](#cursor-advance-on-suppression) |
|
||||
| `REQ-OFFICE-ROUTINE-STATUS-003` | [Control flow](#control-flow) |
|
||||
| `REQ-OFFICE-ROUTINE-STATUS-004` | [Manual and webhook fires](#manual-and-webhook-fires) |
|
||||
| `REQ-OFFICE-ROUTINE-STATUS-006` | [Frontend surfaces](#frontend-surfaces) |
|
||||
|
||||
## Components and responsibilities
|
||||
|
||||
- **`office/models`** owns the firing-status predicate and the status constants it
|
||||
compares against, alongside the existing `RoutineConcurrencyPolicy` and
|
||||
`RoutineCatchUpPolicy` types in `enums.go`, so no caller re-derives the rule from
|
||||
string literals.
|
||||
- **`office/shared`** owns cron expression evaluation. `NextCronTime` validates and
|
||||
parses the five-field expression, applies its timezone and DST rules, and reports
|
||||
an impossible expression with `ErrUnsatisfiableCron`. The routines service uses
|
||||
this existing helper for both suppression and catch-up, so it does not maintain a
|
||||
second cron parser.
|
||||
- **`office/routines` service** owns the gate on both the cron path and the manual
|
||||
path. `processCronTrigger` consults the predicate before claiming; `FireManual`
|
||||
consults it on the routine it already reads and returns a refusal its handler can
|
||||
distinguish. `DispatchRoutineRun` and `DispatchRoutineRunWithMissed` are unchanged.
|
||||
- **`office/routines` handler** owns the HTTP status codes and response bodies for
|
||||
the two refusals, and owns the webhook gate, which sits on the routine that handler
|
||||
already holds. Create and update take no new validation.
|
||||
- **`office/repository/sqlite`** owns the compare-and-set that advances a cursor
|
||||
without recording a fire.
|
||||
- **`apps/web` routines list** owns the display of a non-firing routine.
|
||||
|
||||
## Data and contracts
|
||||
|
||||
### Routine status
|
||||
|
||||
`office_routines.status` is `TEXT NOT NULL DEFAULT 'active'` with no check
|
||||
constraint. This design adds no migration and no write-side validation: the stored
|
||||
value set is unchanged, and the only new rule is how the fire paths read it.
|
||||
|
||||
```
|
||||
RoutineStatusActive = "active"
|
||||
RoutineStatusPaused = "paused"
|
||||
RoutineStatusArchived = "archived"
|
||||
```
|
||||
|
||||
`CanFire()` accepts `active` and the empty string and rejects everything else,
|
||||
including values that are not one of the three constants at all. It compares the
|
||||
stored bytes with no case folding and no trimming, so `Active` and `" active"` are
|
||||
non-firing (AC-OFFICE-ROUTINE-STATUS-001.3). There is deliberately no `Valid()`
|
||||
counterpart: nothing in this capability validates a status on write, so a predicate
|
||||
that guards writes would have no caller. Adding one is named out of scope in the
|
||||
requirements, together with the three problems it drags in.
|
||||
|
||||
### API
|
||||
|
||||
Routes are mounted on `officeRoutePrefix = "/api/v1/office"`
|
||||
(`backendapp/office_scope.go`), so every path below carries that prefix.
|
||||
|
||||
- `POST /api/v1/office/routines/:id/run` returns 409 when the routine cannot fire.
|
||||
- `POST /api/v1/office/routine-triggers/:publicId/fire` returns 409 the same way,
|
||||
evaluated after `verifySignature` and after the existing `trigger.Enabled` check.
|
||||
|
||||
Both refusals carry the same body shape, and the shape is the contract, not just the
|
||||
prose in it (AC-OFFICE-ROUTINE-STATUS-004.4):
|
||||
|
||||
```
|
||||
{ "error": "<human-readable sentence naming the status>",
|
||||
"error_code": "routine_not_firing",
|
||||
"status": "<the observed status, verbatim>" }
|
||||
```
|
||||
|
||||
`error_code` is what makes the refusal discriminable. The existing `error` field is
|
||||
already promoted into `err.message` by `lib/api/client.ts` `throwFromResponse`, and
|
||||
both "Run now" handlers toast that message verbatim, so without a code the operator
|
||||
would read a Go-side English string — which cannot pass through the i18next pipeline
|
||||
AC-OFFICE-ROUTINE-STATUS-006.6 requires, and which the surface cannot tell apart from
|
||||
any other failure. On the manual route that means a 500, which is all `runRoutine`
|
||||
returns today; on the webhook route it would additionally collide with the
|
||||
`trigger is disabled` 409 that route already returns. The
|
||||
repository already has this mechanism and this convention: `ApiError` parses a body
|
||||
`error_code` into `.errorCode`, and `lib/api/task-delete-errors.ts` and
|
||||
`lib/api/domains/canvas-error-copy.ts` are the two existing consumers. `status` is
|
||||
reported verbatim so the surface can interpolate it into localized copy rather than
|
||||
parse it back out of a sentence. The `error` string remains a fallback for any caller
|
||||
with no localized copy, which is why it is not itself subject to -006.6.
|
||||
|
||||
### Repository
|
||||
|
||||
One new method advances a cursor without recording a fire:
|
||||
|
||||
```
|
||||
AdvanceTriggerWithoutFiring(ctx, triggerID string, oldNextRunAt, newNextRunAt time.Time) (bool, error)
|
||||
```
|
||||
|
||||
It is a single statement, `UPDATE office_routine_triggers SET next_run_at = ?,
|
||||
updated_at = ? WHERE id = ? AND next_run_at = ?`, returning whether a row
|
||||
changed. It exists because `ClaimTrigger` cannot be reused: that statement sets
|
||||
`last_fired_at = now` and clears the cursor, both of which are evidence of a fire
|
||||
that did not happen (AC-OFFICE-ROUTINE-STATUS-001.7). The `next_run_at` predicate
|
||||
is the same compare-and-set, so it carries the same concurrency guarantee: the
|
||||
loser of a race changes no rows and does nothing
|
||||
(AC-OFFICE-ROUTINE-STATUS-002.6, -002.7).
|
||||
|
||||
## Control flow
|
||||
|
||||
`processCronTrigger` reorders into: read routine, decide, then act.
|
||||
|
||||
1. Return early when the trigger has no cursor. Unchanged.
|
||||
2. **Read the routine.** A read failure or a missing routine returns without
|
||||
touching the cursor, so the trigger stays due and the next tick retries
|
||||
(AC-OFFICE-ROUTINE-STATUS-003.2, -003.3). This is the read that today happens
|
||||
after the claim, where a failure leaves the cursor cleared and the trigger
|
||||
permanently dead.
|
||||
3. **Evaluate `CanFire()`.** When it is false, compute the first slot strictly
|
||||
after `now` from the trigger's cron expression and timezone with
|
||||
`shared.NextCronTime`, call `AdvanceTriggerWithoutFiring` with the observed cursor
|
||||
as the compare value, log routine id, trigger id, and observed status, and return.
|
||||
No claim, no run row, no wakeup, no task.
|
||||
4. **Claim the trigger.** Unchanged, and only reached on the firing path.
|
||||
5. **Compute outage catch-up and dispatch.** Unchanged.
|
||||
|
||||
### Cron cursor calculation
|
||||
|
||||
The suppression path uses `shared.NextCronTime` as its single cursor calculation.
|
||||
The helper validates the strict five-field syntax, resolves the trigger timezone,
|
||||
and applies the shared DST policy before returning the next slot.
|
||||
|
||||
`NextCronTime` returns the first match strictly after its argument and returns the
|
||||
result in UTC. It performs the wall-clock calculation in the trigger's timezone,
|
||||
applies the shared DST policy, and reports `ErrUnsatisfiableCron` when no future date
|
||||
can satisfy the expression. Suppression uses that error contract directly. It leaves
|
||||
the cursor unchanged on an error, so the trigger remains due and the operator can fix
|
||||
the expression. No separate match predicate or duplicate parser is needed.
|
||||
|
||||
### The firing-status predicate
|
||||
|
||||
`CanFire()` is an allowlist. `active` and `""` fire; `paused`, `archived`, and
|
||||
any unrecognized value do not. The empty string is admitted because the column
|
||||
default is `active`, so an empty stored value means no writer set one. Reading it
|
||||
as non-firing would stop routines that fire today, and this design never changes
|
||||
behavior for a routine nobody paused.
|
||||
|
||||
### Cursor advance on suppression
|
||||
|
||||
Advancing to the first slot strictly after `now`, rather than by one slot from
|
||||
the old cursor, is what makes AC-OFFICE-ROUTINE-STATUS-002.4 hold. If the backend
|
||||
was down for part of a suppression window, the trigger comes back due with a backlog;
|
||||
advancing from `now` collapses that backlog in one evaluation instead of walking it. A
|
||||
suppression window therefore costs one evaluation per scheduled slot while it lasts,
|
||||
and contributes nothing to any later catch-up computation, because
|
||||
`computeRoutineMissed` reads a cursor that was never left in the past.
|
||||
|
||||
This holds only once a suppressing evaluation has run, which is why
|
||||
AC-OFFICE-ROUTINE-STATUS-002.3 and -002.4 are both conditioned on one. `cron.Loop`
|
||||
uses a ticker and does not tick at startup, so after a restart there is a window of at
|
||||
least `DefaultTickInterval` in which a routine paused before the restart still holds
|
||||
its old cursor. A resume inside that window reaches step 5 with that cursor intact and
|
||||
reports the missed ticks the pause accumulated, on that one fire. Repairing it needs a
|
||||
mechanism attached to the status write itself, which is named out of scope in the
|
||||
requirements along with the three constraints such a repair has to satisfy.
|
||||
|
||||
The alternative, clearing the cursor on suppression and re-arming it on resume,
|
||||
was rejected for the same reason it is rejected in that out-of-scope entry: it makes
|
||||
correctness depend on a resume hook, so any writer that sets `status = 'active'`
|
||||
without going through it, config sync, a direct database edit, a future bulk
|
||||
operation, leaves a routine that never fires again. Advancing rather than clearing
|
||||
means the worst case is a stale count on one fire, never a dead schedule. Nothing
|
||||
about *stopping* a routine depends on any of this, which is the property that matters
|
||||
for the kill switch built on top of it.
|
||||
|
||||
## Manual and webhook fires
|
||||
|
||||
Both refuse rather than suppress: there is no cursor to advance, and the caller
|
||||
is synchronous and can be told why. The webhook path evaluates status after
|
||||
`verifySignature` so an unauthenticated caller cannot probe a routine's status
|
||||
through the response code (AC-OFFICE-ROUTINE-STATUS-004.3). The status check sits
|
||||
alongside the existing `trigger.Enabled` 409, which is the same shape of refusal.
|
||||
|
||||
That existing `trigger.Enabled` check runs *before* the signature, and is
|
||||
deliberately left there. It leaks the same class of fact this design is careful not
|
||||
to leak, but moving it changes the behavior of a path this capability does not own,
|
||||
so the two checks are intentionally ordered differently and the inconsistency is
|
||||
recorded rather than quietly fixed.
|
||||
|
||||
The two paths look symmetrical from outside and are not symmetrical inside, so the
|
||||
gate sits in a different place on each. The webhook handler already reads the routine
|
||||
itself, with `GetRoutine`, after verifying the signature and before dispatching; its
|
||||
gate is one branch on the routine it is holding, and that read is the single read
|
||||
AC-OFFICE-ROUTINE-STATUS-004.7 requires.
|
||||
|
||||
The manual route has no such seam. `runRoutine` delegates the whole operation to one
|
||||
service call, `FireManual`, which performs its own routine read and then dispatches,
|
||||
and the handler maps every error it returns to 500. So the gate goes **inside
|
||||
`FireManual`**, on the routine that call already read, and the refusal is returned as
|
||||
a value the handler can recognize and map to 409 with the body in
|
||||
[the API contract](#api). Two alternatives were rejected. Reading the routine again in
|
||||
the handler to decide before calling would violate -004.7 by deciding on a different
|
||||
read from the one that dispatches, and would double the cost of the most common path.
|
||||
Leaving the refusal as an ordinary error would surface a paused routine as a 500,
|
||||
which -004.1 forbids and which the operator cannot act on.
|
||||
|
||||
Concretely, the refusal is a distinct error type carrying the observed status as a
|
||||
field, and the handler recognizes it with `errors.As` before its existing catch-all.
|
||||
The type is what carries the status: the body in [the API contract](#api) reports that
|
||||
value verbatim, and `FireManual` is the only place that read it, so a shared sentinel
|
||||
compared with `errors.Is` could not deliver it. Recognition must also be by type rather
|
||||
than by message, because every *other* error `FireManual` returns keeps its present 500
|
||||
mapping — including a routine that does not exist, which stays 500 rather than becoming
|
||||
404 (named out of scope in the requirements). `office/dashboard` already does exactly
|
||||
this: `ApprovalsPendingError` (`dashboard/decisions.go`) carries its payload as a field
|
||||
and `respondStatusUpdateError` (`dashboard/handler.go`) type-asserts it into a 409 body
|
||||
while everything else falls through to one generic status. This design follows that
|
||||
shape rather than inventing a second one, and `FireManual`'s single caller means no
|
||||
other call site has to learn about the new type.
|
||||
|
||||
Splitting `FireManual` into separate fetch and dispatch calls is a reasonable future
|
||||
refactor and is not required here: the gate needs the routine and the dispatch needs
|
||||
the same routine, and one call that reads once satisfies both.
|
||||
|
||||
## Frontend surfaces
|
||||
|
||||
Two routes read routine status: the list (`app/office/routines/`) and the detail
|
||||
view (`app/office/routines/[id]/`). Both render a next-fire time, so both are gated
|
||||
here; beyond that and how it reports a refused "Run now", the detail view is unchanged.
|
||||
|
||||
- `routine-row.tsx` computes `isActive` as `routine.status === "active"` and uses
|
||||
it for the badge and the toggle. That predicate is widened to the same
|
||||
firing-status rule the backend uses, so a routine with an empty stored status
|
||||
reads as on rather than off (AC-OFFICE-ROUTINE-STATUS-006.5). The mapping stays
|
||||
`active`/empty to on, everything else to off, so `archived` continues to render
|
||||
as off with no new state. The list's enabled toggle already writes only `active`
|
||||
or `paused`, so widening the read side introduces no new written value.
|
||||
- `nextFireText` renders a countdown from any cron trigger carrying a
|
||||
`next_run_at`. It is gated on the same predicate. Without this gate, suppression
|
||||
would leave a paused routine advertising a fire that will not happen, which is
|
||||
the failure mode this capability exists to remove, reintroduced one layer up.
|
||||
- The detail view renders the same promise through a different component:
|
||||
`routine-detail-view.tsx` passes its cron trigger's `nextRunAt` to
|
||||
`DetailReadOnlyCard`, which prints `office:nextFire` unconditionally. These are the
|
||||
only two places a next-fire time reaches an operator, so gating one and not the other
|
||||
would leave the defect reachable at `/office/routines/:id`. The card is gated on the
|
||||
same predicate and renders the same empty state it already shows when no cron trigger
|
||||
carries a `next_run_at` (AC-OFFICE-ROUTINE-STATUS-006.1, -006.2). Its `lastFiredAt`
|
||||
line is unaffected: a past fire is a fact, not a promise.
|
||||
- The `handleRunNow` handlers on both routes today toast the API error message
|
||||
verbatim. They instead branch on `ApiError.errorCode`: the status-refusal code from
|
||||
[the API contract](#api) selects new localized copy in the `office` namespace,
|
||||
interpolating the `status` field, and anything else keeps the existing fallback
|
||||
toast. This is the pattern `lib/api/task-delete-errors.ts` already uses, and it is
|
||||
what makes AC-OFFICE-ROUTINE-STATUS-006.4 and -006.6 satisfiable together: a message
|
||||
naming the status, in the operator's locale, without the backend owning display copy.
|
||||
|
||||
The detail view's status control is untouched. It seeds its draft with
|
||||
`routine.status ?? "active"` and submits `status` on every save, which is harmless
|
||||
while nothing validates the field. It stops being harmless the moment a writer
|
||||
rejects out-of-set values, and that is exactly why the two are cut together: the
|
||||
requirements' out-of-scope entry for write validation carries the trap and its fix,
|
||||
so a follow-up cannot ship one without the other.
|
||||
|
||||
## Failure and recovery
|
||||
|
||||
- **Routine read fails.** No fire, cursor untouched, trigger stays due, logged
|
||||
under the repeat bound. The next tick retries. This is the fail-closed direction
|
||||
required by [ADR 0009](../../../decisions/0009-fail-closed-gc-semantics.md): the
|
||||
consequential action is launching an agent, so an unreadable status does not
|
||||
authorize it.
|
||||
- **Routine is gone for good.** Deleting a routine normally takes its triggers with
|
||||
it: `DeleteRoutine`'s statement targets only `office_routines`, but
|
||||
`office_routine_triggers` declares
|
||||
`FOREIGN KEY (routine_id) REFERENCES office_routines(id) ON DELETE CASCADE` and
|
||||
the production connection enables foreign keys, so the delete cascades and no
|
||||
orphan is created. An orphaned trigger is therefore the exception, not the normal
|
||||
outcome of a delete: it needs a database with foreign keys disabled (which the
|
||||
service's unit-test harness is, since it opens `:memory:` without the flag), or a
|
||||
row removed by some path the constraint does not cover. Where one does exist,
|
||||
`infra/reconcile.go` reaps it at startup, matching triggers against the routines
|
||||
table.
|
||||
Such a trigger lands in the case above and stays due rather than retiring itself,
|
||||
which is a real change: today the claim nulls the cursor before the failing read,
|
||||
so the trigger goes quiet by accident of the bug this design fixes. It is
|
||||
nonetheless left alone, and the reason is the read, not the frequency.
|
||||
`GetRoutineFromConfig` swallows the underlying error and falls back to a name
|
||||
scan, so an absent row and a failed read arrive identically; disarming on that
|
||||
signal would be deleting live data on a lookup that failed, which is precisely
|
||||
ADR 0009's incident. The rule has to hold for the common case it also covers, a
|
||||
routine that is present but momentarily unreadable, where disarming would be
|
||||
simply wrong.
|
||||
The residual cost is worth stating accurately rather than optimistically. Each
|
||||
such tick costs the fallback scan inside `GetRoutineFromConfig`, which is
|
||||
`SELECT * FROM office_routines ORDER BY name` across every workspace, not an
|
||||
indexed read; the repeat bound below bounds the log entries but not that query. It
|
||||
is accepted because the state is rare and self-clearing at the next restart.
|
||||
Making the reconciler cheaper or more frequent is named out of scope in the
|
||||
requirements.
|
||||
- **The new cursor cannot be computed.** `shared.NextCronTime` rejects a malformed
|
||||
expression, an unloadable timezone, or an unsatisfiable expression. Suppression
|
||||
leaves the cursor unchanged, logs under the repeat bound, and retries on the next
|
||||
tick. The trigger stays due, which is correct: the routine is not firing either way,
|
||||
and the misconfiguration is the operator's to fix.
|
||||
- **Cursor advance write fails.** Same outcome: no fire, cursor unchanged, logged
|
||||
under the repeat bound, re-evaluated next tick. The evaluation repeats every tick
|
||||
until the write succeeds; the log entry does not.
|
||||
- **Compare-and-set loses a race.** No rows change and the loser returns. Exactly
|
||||
one evaluation advances the cursor and none fires.
|
||||
- **Status changes mid-evaluation.** The gate reads status once per slot and the
|
||||
decision stands for that slot in both directions: a slot already past the gate
|
||||
completes its fire, and a slot already suppressed stays suppressed even if the
|
||||
routine is resumed a moment later, having consumed its cursor advance
|
||||
(AC-OFFICE-ROUTINE-STATUS-003.4). The practical effect of the second case is
|
||||
that resuming a routine exactly on a slot boundary may skip that one slot.
|
||||
Cancelling in-flight work is agent-level pause's contract, not this one.
|
||||
- **`TickScheduledTriggers` already tolerates a per-trigger error**: it logs and
|
||||
continues to the next trigger. Suppression returns nil, so it does not enter
|
||||
that path.
|
||||
|
||||
## Persistence
|
||||
|
||||
No schema change and no migration. The only new write is
|
||||
`AdvanceTriggerWithoutFiring`, which touches `next_run_at` and `updated_at` on
|
||||
one `office_routine_triggers` row per call, one call per suppressed slot. Run
|
||||
history, trigger rows, and routine rows are untouched by suppression, so a routine
|
||||
paused for a week and then resumed has the same durable state it would have had if it
|
||||
had never been due, apart from an advanced cursor.
|
||||
|
||||
Across a restart, a suppressed routine behaves identically: its cursor is in the
|
||||
future, the trigger is not due, and nothing replays.
|
||||
|
||||
## Security
|
||||
|
||||
The webhook ordering rule is the only trust-boundary change: signature first,
|
||||
status second, so response codes do not disclose a routine's status to an
|
||||
unauthenticated caller. The 409 body names the status, which is deliberate and safe
|
||||
in that order, because only a caller that has already proved possession of the signing
|
||||
secret can reach it. Manual fires are already inside the authenticated Office API and
|
||||
its workspace scoping (`officeWorkspaceScopeMiddleware` resolves `/routines/:id` to
|
||||
its owning workspace), which this design does not change.
|
||||
|
||||
## Observability
|
||||
|
||||
Structured zap logs only, from the routines service logger
|
||||
(`component=routines-service`). Three entries, under two different rules, and the
|
||||
split is the point:
|
||||
|
||||
- **Suppressed slot** — routine id, trigger id, observed status, old and new cursor.
|
||||
**Not deduplicated.** This entry is written only when the cursor advance succeeds
|
||||
(AC-OFFICE-ROUTINE-STATUS-001.8), so the trigger stops being due and the next entry
|
||||
cannot arrive until the routine's next scheduled slot. Its volume is therefore the
|
||||
routine's own schedule, which is what an operator watching a pause take effect wants
|
||||
to see, and it needs no bound. Deduplicating it would be actively wrong: a routine
|
||||
paused, resumed, and paused again would log only the first pause.
|
||||
- **Unreadable routine on a due trigger** — trigger id, and the error as observed. It
|
||||
does not say the routine was deleted, because the read cannot tell that from a
|
||||
failed read (AC-OFFICE-ROUTINE-STATUS-003.2).
|
||||
- **Cursor not advanced** — trigger id, and whether the slot could not be computed or
|
||||
could not be written.
|
||||
|
||||
The last two are bounded to **once per trigger per process**
|
||||
(AC-OFFICE-ROUTINE-STATUS-003.6). They are the two outcomes that leave the trigger due
|
||||
without advancing it, so it is re-evaluated every tick and an unbounded rule would
|
||||
write one entry every 30s for the life of the process. The comparison key is the
|
||||
trigger id and which of the two outcomes it is, and nothing else. Keying on a
|
||||
distinguishing value as well — the error text, an error class, the observed status —
|
||||
was considered and rejected: it buys re-logging when the failure mode changes, at the
|
||||
cost of a rule whose behavior depends on how errors are classified, which is not
|
||||
something an acceptance criterion can test deterministically. The state is a
|
||||
per-process in-memory set of `(trigger id, outcome)` pairs, bounded by trigger count
|
||||
times two, and a restart logs each outcome once more, which is what an operator
|
||||
reading a fresh log wants anyway.
|
||||
|
||||
That set is shared mutable state, and the check and the insert are one atomic
|
||||
operation whose result decides whether the entry is written. The reason is a boundary,
|
||||
not an observed race, and it is worth stating precisely because the obvious
|
||||
justification is wrong. Today's wiring cannot overlap two `TickScheduledTriggers`
|
||||
passes: `cron.Loop` starts one goroutine per *handler* per tick but then waits on all
|
||||
of them before the loop returns to its ticker, there is a single routines handler with
|
||||
a single call site, and `TickScheduledTriggers` walks its due triggers sequentially. So
|
||||
one pass finishes before the next begins, and a plain map would in fact be safe.
|
||||
|
||||
It is required anyway, because that serialization belongs to `internal/scheduler/cron`,
|
||||
which [Purpose and boundaries](#purpose-and-boundaries) names as a contract this design
|
||||
uses and does not own. A bound this capability promises must not rest on a cadence
|
||||
another package is free to change — and a second process, a test driving the service
|
||||
directly, or a future per-trigger fan-out would each break it. The failure is also
|
||||
silent and non-deterministic: a non-atomic test-then-insert lets two passes both find
|
||||
the pair absent and log twice, breaking the "at most once" in
|
||||
AC-OFFICE-ROUTINE-STATUS-003.6, while an unguarded map is a data race outright. The
|
||||
guard costs one lock; discovering its absence costs a flaky assertion nobody can
|
||||
reproduce.
|
||||
|
||||
No expvar counter is added; that is named out of scope in the requirements. A
|
||||
suppressed slot writes no `office_routine_runs` row, so the runs list of a
|
||||
routine paused for a week is empty for that week rather than holding one row per
|
||||
missed slot.
|
||||
|
||||
## Prior art
|
||||
|
||||
### Our own prior reasoning (wiki)
|
||||
|
||||
**Receipt:** searched for the tool, not the vault. None of the 57 skills in
|
||||
`~/.claude/skills` is `wiki-query`; `wiki-query`, `wiki-switch` and `qmd` are absent
|
||||
from `PATH`; `~/.obsidian-wiki` does not exist, so there is no symlink to resolve and
|
||||
no `OBSIDIAN_VAULT_PATH` to report. No vault was queried, no grep fallback ran. This
|
||||
leg did not run.
|
||||
|
||||
### What others shipped (saas-kb)
|
||||
|
||||
**Receipt:** the only MCP tools exposed to this session are `mcp__kandev__*`, and
|
||||
`search_fsm_docs` is not among them, so `saas-kb` is not configured here. No query
|
||||
was issued and no `ai_sdlc` filter applied. This leg did not run.
|
||||
|
||||
### In-repo prior art, used instead
|
||||
|
||||
Both external legs were unavailable; the substitute is this repository's own record.
|
||||
|
||||
- **Fail-closed on an uncertain signal.**
|
||||
[ADR 0009](../../../decisions/0009-fail-closed-gc-semantics.md) followed a garbage
|
||||
collector deleting 307 live worktrees by reading a failed lookup as "orphan, delete":
|
||||
a consequential action needs a positive signal, never the absence of a negative one.
|
||||
REQ-OFFICE-ROUTINE-STATUS-001 reads status as an allowlist for that reason, and
|
||||
-003 declines to disarm a trigger on the same grounds. The deliberate contrast is
|
||||
`checkIdleSkip`, which fails open because its consequential action is skipping work.
|
||||
- **A status gate that refuses rather than defers.** `guardAgentStatus`
|
||||
(`office/scheduler/run.go`) and `heartbeatAgentRuntime.AllowFire` (`backendapp/cron.go`)
|
||||
both refuse to queue work for a paused agent, with no override. -004 follows that,
|
||||
which is why "Run now" is a 409. Neither faces a cursor, so both are prior art for
|
||||
the refusal, not the catch-up decision.
|
||||
- **A discriminable API refusal the frontend localizes.** `lib/api/task-delete-errors.ts`
|
||||
and `lib/api/domains/canvas-error-copy.ts` already read `ApiError.errorCode` to choose
|
||||
localized copy rather than displaying a server string. -004.4 follows that convention
|
||||
rather than inventing a second one.
|
||||
- **The contract named the field and never defined it.**
|
||||
[Scheduler system design part 1](scheduler-01.md) lists `status: active | paused |
|
||||
archived` and says the coordinator routine can be paused, but nothing states what
|
||||
pausing does. That silence is this defect.
|
||||
|
||||
**What we are doing differently:** nothing departs from a position already taken here.
|
||||
The one decision with no in-repo precedent is REQ-OFFICE-ROUTINE-STATUS-002, dropping
|
||||
suppressed slots rather than banking them, because no existing gate sits in front of a
|
||||
schedule cursor. Banking them would turn a pause into a burst on resume.
|
||||
|
||||
## Related decisions
|
||||
|
||||
- [ADR 0009: Fail-closed GC semantics](../../../decisions/0009-fail-closed-gc-semantics.md)
|
||||
Reference in New Issue
Block a user