diff --git a/apps/backend/internal/db/sqlguard/exemptions.json b/apps/backend/internal/db/sqlguard/exemptions.json index 9bff08d2e8..f734e36f24 100644 --- a/apps/backend/internal/db/sqlguard/exemptions.json +++ b/apps/backend/internal/db/sqlguard/exemptions.json @@ -65,6 +65,8 @@ {"file": "internal/office/repository/sqlite/base_migrations.go", "symbol": "backfillFTS", "rule": "conflict-syntax", "reason": "SQLite FTS backfill uses SQLite conflict semantics"}, {"file": "internal/office/repository/sqlite/base_migrations.go", "symbol": "runTaskPriorityRecreate", "rule": "sqlite-catalog", "reason": "SQLite-only table-recreation migration"}, {"file": "internal/office/repository/sqlite/base_migrations.go", "symbol": "tasksTableExists", "rule": "sqlite-catalog", "reason": "The Office repository is explicitly SQLite-only"}, + {"file": "internal/office/repository/sqlite/catchup_migrations.go", "symbol": "routineTableExists", "rule": "sqlite-catalog", "reason": "The Office repository is explicitly SQLite-only"}, + {"file": "internal/office/repository/sqlite/catchup_migrations.go", "symbol": "migrateRoutineDefaultPolicyRebuild", "rule": "sqlite-catalog", "reason": "SQLite-only table-recreation migration"}, {"file": "internal/office/repository/sqlite/dashboard.go", "symbol": "QueryRunActivity", "rule": "sqlite-date-function", "reason": "The Office repository is explicitly SQLite-only"}, {"file": "internal/office/repository/sqlite/labels.go", "symbol": "GetOrCreateLabel", "rule": "conflict-syntax", "reason": "The Office repository is explicitly SQLite-only"}, {"file": "internal/office/repository/sqlite/labels.go", "symbol": "AddLabelToTask", "rule": "conflict-syntax", "reason": "The Office repository is explicitly SQLite-only"}, @@ -107,6 +109,8 @@ {"file": "internal/agent/settings/store/sqlite.go", "symbol": "UpdateAgentProfileEnabled", "rule": "boolean-integer", "reason": "The agent settings schema stores user_modified as an INTEGER flag"}, {"file": "internal/office/configsync/store.go", "symbol": "UpsertConfigForWorkspace", "rule": "boolean-integer", "reason": "The Office config-sync schema stores last_ok as an INTEGER flag"}, {"file": "internal/office/repository/sqlite/routines.go", "symbol": "GetDueTriggers", "rule": "boolean-integer", "reason": "The Office SQLite schema stores trigger enabled as an INTEGER flag"}, + {"file": "internal/office/repository/sqlite/routines.go", "symbol": "ListStrandedTriggers", "rule": "boolean-integer", "reason": "The Office SQLite schema stores trigger enabled as an INTEGER flag"}, + {"file": "internal/office/repository/sqlite/routines.go", "symbol": "ReconcileTriggerNextRun", "rule": "boolean-integer", "reason": "The Office SQLite schema stores trigger enabled as an INTEGER flag"}, {"file": "internal/plugins/marketplace/sourcestore.go", "symbol": "EnsureBuiltin", "rule": "boolean-integer", "reason": "The marketplace source schema stores enabled and builtin as INTEGER flags"}, {"file": "internal/plugins/state/instance_store.go", "symbol": "List", "rule": "boolean-integer", "reason": "The plugin instance-state schema stores deleted as an INTEGER tombstone"}, {"file": "internal/plugins/state/instance_store.go", "symbol": "Set", "rule": "boolean-integer", "reason": "The plugin instance-state schema stores deleted as an INTEGER tombstone"}, diff --git a/apps/backend/internal/office/AGENTS.md b/apps/backend/internal/office/AGENTS.md index 63a42a8cc8..a0cbf11c69 100644 --- a/apps/backend/internal/office/AGENTS.md +++ b/apps/backend/internal/office/AGENTS.md @@ -4,7 +4,7 @@ ## Spec authority -`docs/specs/office/` (15 files, plus later requirements/system-design additions below) is the authority for what Office is and why — it outranks any card, register, or comment. **Where code and spec disagree, that is a defect in one of them; do not silently follow the code.** A sixteenth office-tagged spec, [per-agent + per-role tier selection](../../../../docs/specs/office-agent-tier-routing/spec.md), lives in a sibling directory, not under `docs/specs/office/`. +`docs/specs/office/` (16 files, plus later requirements/system-design additions below) is the authority for what Office is and why — it outranks any card, register, or comment. **Where code and spec disagree, that is a defect in one of them; do not silently follow the code.** A seventeenth office-tagged spec, [per-agent + per-role tier selection](../../../../docs/specs/office-agent-tier-routing/spec.md), lives in a sibling directory, not under `docs/specs/office/`. | Spec | Status | Covers | |---|---|---| @@ -12,6 +12,7 @@ | `agents.md` | draft | Persistent Office agent identity vs. execution profiles | | `tasks.md` | draft | Office task model: assignee, reviewers/approvers, blockers, subtasks, per-agent working memory | | `scheduler.md` | draft | Autonomous wakeup pipeline: assignments/comments/approvals, routines, idle skip, retry/backoff | +| `routine-catch-up.md` | shipped | Resuming after downtime produces exactly one run per due trigger, never one per missed tick; `catch_up_max` bounds only how many ticks are counted/reported, never how many runs fire; policy `enqueue_missed_with_cap` renamed to `summarize_missed` (deprecated alias accepted forever) | | `runtime.md` | draft | Error-handling contract for the agent runtime; **2026-08-17 amendment**: provider classification/recovery is superseded by `../platform/provider-error-recovery.md` and `../agents/dynamic-agent-routing.md` — read the amendment banner before the body | | `routing.md` | **archived** | Provider routing; superseded by `../agents/dynamic-agent-routing.md` — do not treat as current | | `costs.md` | in-progress | Cost tracking and budget management | diff --git a/apps/backend/internal/office/models/catchup.go b/apps/backend/internal/office/models/catchup.go new file mode 100644 index 0000000000..7ec518345e --- /dev/null +++ b/apps/backend/internal/office/models/catchup.go @@ -0,0 +1,58 @@ +package models + +import "fmt" + +// NormaliseCatchUpPolicy maps a raw stored or request value onto the +// canonical RoutineCatchUpPolicy set. The deprecated alias, an empty +// value, and any unrecognized value all normalize to the summarizing +// policy — the same "fail open toward reporting" default the write +// funnel and the read-side Scan hook below share. +func NormaliseCatchUpPolicy(raw string) RoutineCatchUpPolicy { + if RoutineCatchUpPolicy(raw) == CatchUpPolicySkipMissed { + return CatchUpPolicySkipMissed + } + return CatchUpPolicySummarizeMissed +} + +// Scan implements sql.Scanner so every SELECT into a RoutineCatchUpPolicy +// field normalizes the deprecated alias and any unrecognized stored +// value to the canonical one on the way out of the database. This is +// the single read-side chokepoint: GetRoutine and ListRoutines are +// SELECT * through sqlx StructScan with no other post-scan hook. +func (p *RoutineCatchUpPolicy) Scan(value any) error { + switch v := value.(type) { + case nil: + *p = NormaliseCatchUpPolicy("") + case string: + *p = NormaliseCatchUpPolicy(v) + case []byte: + *p = NormaliseCatchUpPolicy(string(v)) + default: + return fmt.Errorf("RoutineCatchUpPolicy: unsupported scan type %T", value) + } + return nil +} + +// Bounds a stored or requested catch_up_max is clamped to. +// CatchUpMaxDefault is the floor a value below 1 is raised to; +// CatchUpMaxCeiling is the ceiling a value above it is lowered to. +// Named constants so both bounds exist in exactly one place in Go. +const ( + CatchUpMaxDefault = 25 + CatchUpMaxCeiling = 1000 +) + +// NormaliseCatchUpMax clamps v to [1, CatchUpMaxCeiling], mapping a +// value below 1 to CatchUpMaxDefault. A fixpoint: normalizing an +// already-normalized value returns it unchanged, so applying it more +// than once (write funnel plus a belt-and-braces use-time clamp) is +// always safe. +func NormaliseCatchUpMax(v int) int { + if v < 1 { + return CatchUpMaxDefault + } + if v > CatchUpMaxCeiling { + return CatchUpMaxCeiling + } + return v +} diff --git a/apps/backend/internal/office/models/catchup_test.go b/apps/backend/internal/office/models/catchup_test.go new file mode 100644 index 0000000000..0d44dc3fb8 --- /dev/null +++ b/apps/backend/internal/office/models/catchup_test.go @@ -0,0 +1,109 @@ +package models_test + +import ( + "testing" + + "github.com/kandev/kandev/internal/office/models" +) + +func TestNormaliseCatchUpPolicy(t *testing.T) { + cases := []struct { + name string + in string + want models.RoutineCatchUpPolicy + }{ + {"canonical", "summarize_missed", models.CatchUpPolicySummarizeMissed}, + {"deprecated alias", "enqueue_missed_with_cap", models.CatchUpPolicySummarizeMissed}, + {"skip_missed unchanged", "skip_missed", models.CatchUpPolicySkipMissed}, + {"empty defaults to summarize", "", models.CatchUpPolicySummarizeMissed}, + {"unknown defaults to summarize", "garbage", models.CatchUpPolicySummarizeMissed}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := models.NormaliseCatchUpPolicy(tc.in); got != tc.want { + t.Errorf("NormaliseCatchUpPolicy(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +func TestRoutineCatchUpPolicy_Scan(t *testing.T) { + cases := []struct { + name string + in any + want models.RoutineCatchUpPolicy + }{ + {"string alias", "enqueue_missed_with_cap", models.CatchUpPolicySummarizeMissed}, + {"bytes alias", []byte("enqueue_missed_with_cap"), models.CatchUpPolicySummarizeMissed}, + {"string canonical", "summarize_missed", models.CatchUpPolicySummarizeMissed}, + {"string skip", "skip_missed", models.CatchUpPolicySkipMissed}, + {"nil", nil, models.CatchUpPolicySummarizeMissed}, + {"unknown string", "totally-unknown", models.CatchUpPolicySummarizeMissed}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var p models.RoutineCatchUpPolicy + if err := p.Scan(tc.in); err != nil { + t.Fatalf("Scan(%v): %v", tc.in, err) + } + if p != tc.want { + t.Errorf("Scan(%v) = %q, want %q", tc.in, p, tc.want) + } + }) + } +} + +func TestRoutineCatchUpPolicy_Scan_UnsupportedType(t *testing.T) { + var p models.RoutineCatchUpPolicy + if err := p.Scan(42); err == nil { + t.Fatal("Scan(42) error = nil, want error for unsupported scan type") + } +} + +func TestRoutineCatchUpPolicy_Valid(t *testing.T) { + valid := []models.RoutineCatchUpPolicy{ + models.CatchUpPolicySummarizeMissed, + models.CatchUpPolicyEnqueueMissedWithCap, + models.CatchUpPolicySkipMissed, + } + for _, p := range valid { + if !p.Valid() { + t.Errorf("Valid(%q) = false, want true", p) + } + } + if models.RoutineCatchUpPolicy("bogus").Valid() { + t.Error("Valid(bogus) = true, want false") + } +} + +func TestNormaliseCatchUpMax(t *testing.T) { + cases := []struct { + name string + in int + want int + }{ + {"in range untouched", 5, 5}, + {"zero clamps to default", 0, models.CatchUpMaxDefault}, + {"negative clamps to default", -3, models.CatchUpMaxDefault}, + {"exactly one untouched", 1, 1}, + {"exactly ceiling untouched", 1000, 1000}, + {"above ceiling clamps down", 5000, models.CatchUpMaxCeiling}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := models.NormaliseCatchUpMax(tc.in); got != tc.want { + t.Errorf("NormaliseCatchUpMax(%d) = %d, want %d", tc.in, got, tc.want) + } + }) + } +} + +func TestNormaliseCatchUpMax_Fixpoint(t *testing.T) { + for _, in := range []int{-5, 0, 1, 25, 1000, 5000} { + once := models.NormaliseCatchUpMax(in) + twice := models.NormaliseCatchUpMax(once) + if once != twice { + t.Errorf("NormaliseCatchUpMax not a fixpoint at %d: once=%d twice=%d", in, once, twice) + } + } +} diff --git a/apps/backend/internal/office/models/enums.go b/apps/backend/internal/office/models/enums.go index bf69d8c96d..232b104d43 100644 --- a/apps/backend/internal/office/models/enums.go +++ b/apps/backend/internal/office/models/enums.go @@ -84,12 +84,25 @@ func (p RoutineConcurrencyPolicy) Valid() bool { } // RoutineCatchUpPolicy controls what happens when a scheduled routine -// missed fires (e.g. backend was down). +// missed fires (e.g. backend was down). Whatever the policy, a claim +// dispatches at most one routine run — the policy only decides whether +// the gap crossed is reported to the woken agent. See +// docs/specs/office/requirements/routine-catch-up.md. type RoutineCatchUpPolicy string // Routine catch-up policy values. The DB default is -// `enqueue_missed_with_cap`; the alternative skips missed fires entirely. +// `summarize_missed`; the alternative skips gap reporting entirely. const ( + // CatchUpPolicySummarizeMissed is the canonical summarizing policy: + // the gap crossed by a claim is recorded and delivered to the woken + // agent. Named for what happens (one wake, gap summarized), not for + // the discarded enqueue-N-runs design the old name implied. + CatchUpPolicySummarizeMissed RoutineCatchUpPolicy = "summarize_missed" + // CatchUpPolicyEnqueueMissedWithCap is the deprecated alias for + // CatchUpPolicySummarizeMissed. Retained forever — never delete — + // so NormaliseCatchUpPolicy has a name to compare persisted rows + // and API requests against; installs that already stored it must + // keep reading and writing successfully. CatchUpPolicyEnqueueMissedWithCap RoutineCatchUpPolicy = "enqueue_missed_with_cap" CatchUpPolicySkipMissed RoutineCatchUpPolicy = "skip_missed" ) @@ -97,10 +110,13 @@ const ( // String implements fmt.Stringer. func (p RoutineCatchUpPolicy) String() string { return string(p) } -// Valid reports whether p is one of the declared RoutineCatchUpPolicy values. +// Valid reports whether p is one of the declared RoutineCatchUpPolicy +// values, including the deprecated alias — a client sending the old +// value on create/update gets 200, not 400; NormaliseCatchUpPolicy +// normalizes the persisted value to the canonical one. func (p RoutineCatchUpPolicy) Valid() bool { switch p { - case CatchUpPolicyEnqueueMissedWithCap, CatchUpPolicySkipMissed: + case CatchUpPolicySummarizeMissed, CatchUpPolicyEnqueueMissedWithCap, CatchUpPolicySkipMissed: return true } return false diff --git a/apps/backend/internal/office/models/models.go b/apps/backend/internal/office/models/models.go index 8e3a81398d..64df520228 100644 --- a/apps/backend/internal/office/models/models.go +++ b/apps/backend/internal/office/models/models.go @@ -604,19 +604,29 @@ type RoutineTrigger struct { } // RoutineRun represents a single run of a routine. +// +// CatchUpMissedTicks/CatchUpFirstMissedAt/CatchUpTruncated are the gap +// summary measured for the claim that created this run — written once +// at creation and never modified afterward (AC-OFFICE-ROUTINE-CATCHUP-002.10). +// CatchUpMissedTicks is nil when no gap summary was recorded; per +// AC-002.3, absence is never represented as a stored zero, so a nil +// check (not a zero check) is the presence test. type RoutineRun struct { - ID string `json:"id" db:"id"` - RoutineID string `json:"routine_id" db:"routine_id"` - TriggerID string `json:"trigger_id" db:"trigger_id"` - Source string `json:"source" db:"source"` - Status RoutineRunStatus `json:"status" db:"status"` - TriggerPayload string `json:"trigger_payload" db:"trigger_payload"` - LinkedTaskID string `json:"linked_task_id" db:"linked_task_id"` - CoalescedIntoRunID string `json:"coalesced_into_run_id" db:"coalesced_into_run_id"` - DispatchFingerprint string `json:"dispatch_fingerprint" db:"dispatch_fingerprint"` - StartedAt *time.Time `json:"started_at" db:"started_at"` - CompletedAt *time.Time `json:"completed_at" db:"completed_at"` - CreatedAt time.Time `json:"created_at" db:"created_at"` + ID string `json:"id" db:"id"` + RoutineID string `json:"routine_id" db:"routine_id"` + TriggerID string `json:"trigger_id" db:"trigger_id"` + Source string `json:"source" db:"source"` + Status RoutineRunStatus `json:"status" db:"status"` + TriggerPayload string `json:"trigger_payload" db:"trigger_payload"` + LinkedTaskID string `json:"linked_task_id" db:"linked_task_id"` + CoalescedIntoRunID string `json:"coalesced_into_run_id" db:"coalesced_into_run_id"` + DispatchFingerprint string `json:"dispatch_fingerprint" db:"dispatch_fingerprint"` + CatchUpMissedTicks *int `json:"catch_up_missed_ticks,omitempty" db:"catch_up_missed_ticks"` + CatchUpFirstMissedAt *time.Time `json:"catch_up_first_missed_at,omitempty" db:"catch_up_first_missed_at"` + CatchUpTruncated bool `json:"catch_up_truncated" db:"catch_up_truncated"` + StartedAt *time.Time `json:"started_at" db:"started_at"` + CompletedAt *time.Time `json:"completed_at" db:"completed_at"` + CreatedAt time.Time `json:"created_at" db:"created_at"` // SkipReason carries exactly one value today, "workspace_paused", // distinguishing a workspace-pause skip from every other skip cause // (e.g. skip_if_active concurrency). Empty for a run not skipped by a diff --git a/apps/backend/internal/office/repository/sqlite/base.go b/apps/backend/internal/office/repository/sqlite/base.go index 2e5b18498a..124709caad 100644 --- a/apps/backend/internal/office/repository/sqlite/base.go +++ b/apps/backend/internal/office/repository/sqlite/base.go @@ -507,7 +507,7 @@ func (r *Repository) createRoutineTables() error { assignee_agent_profile_id TEXT DEFAULT '', status TEXT NOT NULL DEFAULT 'active', concurrency_policy TEXT DEFAULT 'skip_if_active', - catch_up_policy TEXT NOT NULL DEFAULT 'enqueue_missed_with_cap', + catch_up_policy TEXT NOT NULL DEFAULT 'summarize_missed', catch_up_max INTEGER NOT NULL DEFAULT 25, variables TEXT DEFAULT '{}', last_run_at TIMESTAMP, @@ -542,6 +542,9 @@ func (r *Repository) createRoutineTables() error { linked_task_id TEXT DEFAULT '', coalesced_into_run_id TEXT DEFAULT '', dispatch_fingerprint TEXT DEFAULT '', + catch_up_missed_ticks INTEGER, + catch_up_first_missed_at TIMESTAMP, + catch_up_truncated INTEGER NOT NULL DEFAULT 0, started_at TIMESTAMP, completed_at TIMESTAMP, created_at TIMESTAMP NOT NULL, diff --git a/apps/backend/internal/office/repository/sqlite/base_migrations.go b/apps/backend/internal/office/repository/sqlite/base_migrations.go index 968b8e9e7a..4d92465b6e 100644 --- a/apps/backend/internal/office/repository/sqlite/base_migrations.go +++ b/apps/backend/internal/office/repository/sqlite/base_migrations.go @@ -63,6 +63,7 @@ func (r *Repository) runMigrations() error { r.migrateWakeWaveColumns() r.migrate.Apply("task_workspace_groups.ownership_generation", `ALTER TABLE task_workspace_groups ADD COLUMN ownership_generation INTEGER NOT NULL DEFAULT 1`) + r.migrateRoutineCatchUp() r.migrateBudgetPolicyRevision() r.migrateWorkspacePauseSkipAttribution() r.migrateLoopLivenessCausationID() diff --git a/apps/backend/internal/office/repository/sqlite/catchup_migration_replay_postgres_test.go b/apps/backend/internal/office/repository/sqlite/catchup_migration_replay_postgres_test.go new file mode 100644 index 0000000000..17f7812dcc --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/catchup_migration_replay_postgres_test.go @@ -0,0 +1,84 @@ +package sqlite_test + +import ( + "strings" + "testing" + + "github.com/kandev/kandev/internal/office/repository/sqlite" + taskrepo "github.com/kandev/kandev/internal/task/repository/sqlite" + "github.com/kandev/kandev/internal/testutil" +) + +// TestMigrate_RoutineCatchUpPolicyDefaultRebuildPostgres is the Postgres +// counterpart to TestMigrate_RoutineCatchUpPolicyDefaultRebuild +// (catchup_migration_replay_test.go). The SQLite migration detects +// staleness via sqlite_master/PRAGMA table_info, neither of which exist on +// Postgres, so without a dedicated Postgres path the rebuild silently never +// runs there and an upgraded Postgres install's office_routines.catch_up_policy +// column keeps the retired 'enqueue_missed_with_cap' DEFAULT forever +// (AC-OFFICE-ROUTINE-CATCHUP-003.3: "the column's stored default shall be +// summarize_missed on an upgraded install ... Inspecting the live schema +// ... shall not show the retired value"). +// +// Skips unless KANDEV_TEST_POSTGRES_DSN is set. +func TestMigrate_RoutineCatchUpPolicyDefaultRebuildPostgres(t *testing.T) { + db := testutil.OpenIsolatedPostgres(t, testutil.PostgresDSNFromEnv(t)) + + // office_routines' schema init runs after the task repository's, mirroring + // production boot order (see runs_inflight_postgres_test.go). + if _, err := taskrepo.NewWithDB(db, db, nil); err != nil { + t.Fatalf("init task repo: %v", err) + } + + // Fresh init creates office_routines with the current (correct) default. + if _, err := sqlite.NewWithDB(db, db, nil); err != nil { + t.Fatalf("init office repo (fresh schema): %v", err) + } + + // Simulate an upgraded install: force the column's DEFAULT back to the + // retired literal, as if this schema predated the rename. + if _, err := db.Exec( + `ALTER TABLE office_routines ALTER COLUMN catch_up_policy SET DEFAULT 'enqueue_missed_with_cap'`, + ); err != nil { + t.Fatalf("seed legacy default: %v", err) + } + + // Re-opening the repository must detect and correct the stale default. + if _, err := sqlite.NewWithDB(db, db, nil); err != nil { + t.Fatalf("init office repo (replay migrations): %v", err) + } + + dflt := currentCatchUpPolicyDefault(t, db) + if strings.Contains(dflt, "enqueue_missed_with_cap") { + t.Fatalf("catch_up_policy DEFAULT still names the retired value: %q", dflt) + } + if !strings.Contains(dflt, "summarize_missed") { + t.Fatalf("catch_up_policy DEFAULT = %q, want it to contain summarize_missed", dflt) + } + + // Idempotency: a second replay against an already-correct default must + // not error, and must leave it unchanged. + if _, err := sqlite.NewWithDB(db, db, nil); err != nil { + t.Fatalf("second init should be idempotent: %v", err) + } + if got := currentCatchUpPolicyDefault(t, db); !strings.Contains(got, "summarize_missed") { + t.Fatalf("catch_up_policy DEFAULT after replay = %q, want it to contain summarize_missed", got) + } +} + +func currentCatchUpPolicyDefault(t *testing.T, db interface { + Get(dest interface{}, query string, args ...interface{}) error +}) string { + t.Helper() + var dflt string + if err := db.Get(&dflt, ` + SELECT column_default + FROM information_schema.columns + WHERE table_schema = current_schema() + AND table_name = 'office_routines' + AND column_name = 'catch_up_policy' + `); err != nil { + t.Fatalf("read column_default: %v", err) + } + return dflt +} diff --git a/apps/backend/internal/office/repository/sqlite/catchup_migration_replay_test.go b/apps/backend/internal/office/repository/sqlite/catchup_migration_replay_test.go new file mode 100644 index 0000000000..a8d00fd9df --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/catchup_migration_replay_test.go @@ -0,0 +1,235 @@ +package sqlite_test + +import ( + "database/sql" + "strings" + "testing" + "time" + + "github.com/jmoiron/sqlx" + _ "github.com/mattn/go-sqlite3" + + "github.com/kandev/kandev/internal/office/repository/sqlite" +) + +// TestMigrate_RoutineCatchUpPolicyDefaultRebuild seeds a legacy +// office_routines table (as it existed before this change: DEFAULT +// 'enqueue_missed_with_cap', no gap-summary columns on office_routine_runs) +// with a routine, a trigger, and a run already attached, then asserts the +// office repo migration: +// - rewrites the stored DEFAULT to 'summarize_missed' (AC-003.3, visible +// on the live schema of an upgraded install, not just in a fresh one) +// - normalizes the pre-existing 'enqueue_missed_with_cap' row value +// - does not cascade-delete the routine's trigger or run rows, which is +// the hazard PRAGMA foreign_keys=OFF exists to avoid during the +// drop+rename table-rebuild dance +// - preserves every column and timestamp on the surviving rows +func TestMigrate_RoutineCatchUpPolicyDefaultRebuild(t *testing.T) { + dbPath := t.TempDir() + "/test.db?_journal_mode=WAL&_foreign_keys=on" + db, err := sqlx.Open("sqlite3", dbPath) + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + + createdAt := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + if _, err := db.Exec(` + CREATE TABLE office_routines ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL, + name TEXT NOT NULL, + description TEXT DEFAULT '', + task_template TEXT NOT NULL DEFAULT '{}', + assignee_agent_profile_id TEXT DEFAULT '', + status TEXT NOT NULL DEFAULT 'active', + concurrency_policy TEXT DEFAULT 'skip_if_active', + catch_up_policy TEXT NOT NULL DEFAULT 'enqueue_missed_with_cap', + catch_up_max INTEGER NOT NULL DEFAULT 25, + variables TEXT DEFAULT '{}', + last_run_at TIMESTAMP, + created_at TIMESTAMP NOT NULL, + updated_at TIMESTAMP NOT NULL + ); + CREATE TABLE office_routine_triggers ( + id TEXT PRIMARY KEY, + routine_id TEXT NOT NULL, + kind TEXT NOT NULL, + cron_expression TEXT DEFAULT '', + timezone TEXT DEFAULT '', + public_id TEXT DEFAULT '', + signing_mode TEXT DEFAULT '', + secret TEXT DEFAULT '', + next_run_at TIMESTAMP, + last_fired_at TIMESTAMP, + enabled INTEGER DEFAULT 1, + created_at TIMESTAMP NOT NULL, + updated_at TIMESTAMP NOT NULL, + FOREIGN KEY (routine_id) REFERENCES office_routines(id) ON DELETE CASCADE + ); + CREATE TABLE office_routine_runs ( + id TEXT PRIMARY KEY, + routine_id TEXT NOT NULL, + trigger_id TEXT DEFAULT '', + source TEXT NOT NULL, + status TEXT NOT NULL DEFAULT 'received', + trigger_payload TEXT DEFAULT '{}', + linked_task_id TEXT DEFAULT '', + coalesced_into_run_id TEXT DEFAULT '', + dispatch_fingerprint TEXT DEFAULT '', + started_at TIMESTAMP, + completed_at TIMESTAMP, + created_at TIMESTAMP NOT NULL, + FOREIGN KEY (routine_id) REFERENCES office_routines(id) ON DELETE CASCADE + ); + `); err != nil { + t.Fatalf("seed legacy schema: %v", err) + } + + if _, err := db.Exec(` + INSERT INTO office_routines ( + id, workspace_id, name, description, task_template, + assignee_agent_profile_id, status, concurrency_policy, + catch_up_policy, catch_up_max, variables, last_run_at, created_at, updated_at + ) VALUES ( + 'routine-legacy', 'ws-1', 'Legacy routine', 'desc', '{}', + 'agent-1', 'active', 'skip_if_active', + 'enqueue_missed_with_cap', 25, '{}', NULL, ?, ? + )`, createdAt, createdAt); err != nil { + t.Fatalf("seed legacy routine: %v", err) + } + if _, err := db.Exec(` + INSERT INTO office_routines ( + id, workspace_id, name, description, task_template, + assignee_agent_profile_id, status, concurrency_policy, + catch_up_policy, catch_up_max, variables, last_run_at, created_at, updated_at + ) VALUES ( + 'routine-max-below-floor', 'ws-1', 'Below floor', 'desc', '{}', + 'agent-1', 'active', 'skip_if_active', + 'summarize_missed', 0, '{}', NULL, ?, ? + )`, createdAt, createdAt); err != nil { + t.Fatalf("seed below-floor routine: %v", err) + } + if _, err := db.Exec(` + INSERT INTO office_routines ( + id, workspace_id, name, description, task_template, + assignee_agent_profile_id, status, concurrency_policy, + catch_up_policy, catch_up_max, variables, last_run_at, created_at, updated_at + ) VALUES ( + 'routine-max-above-ceiling', 'ws-1', 'Above ceiling', 'desc', '{}', + 'agent-1', 'active', 'skip_if_active', + 'summarize_missed', 5000, '{}', NULL, ?, ? + )`, createdAt, createdAt); err != nil { + t.Fatalf("seed above-ceiling routine: %v", err) + } + if _, err := db.Exec(` + INSERT INTO office_routine_triggers ( + id, routine_id, kind, cron_expression, timezone, enabled, created_at, updated_at + ) VALUES ('trigger-legacy', 'routine-legacy', 'cron', '* * * * *', 'UTC', 1, ?, ?) + `, createdAt, createdAt); err != nil { + t.Fatalf("seed legacy trigger: %v", err) + } + if _, err := db.Exec(` + INSERT INTO office_routine_runs ( + id, routine_id, trigger_id, source, status, trigger_payload, created_at + ) VALUES ('run-legacy', 'routine-legacy', 'trigger-legacy', 'cron', 'received', '{}', ?) + `, createdAt); err != nil { + t.Fatalf("seed legacy run: %v", err) + } + + if _, err := sqlite.NewWithDB(db, db, nil); err != nil { + t.Fatalf("init office repo (run migrations): %v", err) + } + + // The stored DEFAULT clause itself must now read summarize_missed. + rows, err := db.Queryx(`PRAGMA table_info(office_routines)`) + if err != nil { + t.Fatalf("pragma table_info: %v", err) + } + defer func() { _ = rows.Close() }() + dfltFound := false + for rows.Next() { + var cid int + var name, ctype string + var notnull, pk int + var dflt sql.NullString + if err := rows.Scan(&cid, &name, &ctype, ¬null, &dflt, &pk); err != nil { + t.Fatalf("scan pragma: %v", err) + } + if name == "catch_up_policy" { + dfltFound = true + if strings.Contains(dflt.String, "enqueue_missed_with_cap") { + t.Fatalf("catch_up_policy DEFAULT still names the retired value: %q", dflt.String) + } + if !strings.Contains(dflt.String, "summarize_missed") { + t.Fatalf("catch_up_policy DEFAULT = %q, want it to contain summarize_missed", dflt.String) + } + } + } + if !dfltFound { + t.Fatal("catch_up_policy column not found after migration") + } + + // The pre-existing row's value is normalized. + var policy string + var routineCreatedAt, routineUpdatedAt time.Time + if err := db.QueryRow( + `SELECT catch_up_policy, created_at, updated_at FROM office_routines WHERE id = 'routine-legacy'`, + ).Scan(&policy, &routineCreatedAt, &routineUpdatedAt); err != nil { + t.Fatalf("select migrated routine: %v", err) + } + if policy != "summarize_missed" { + t.Errorf("routine-legacy catch_up_policy = %q, want summarize_missed", policy) + } + if !routineCreatedAt.Equal(createdAt) { + t.Errorf("routine-legacy created_at = %v, want %v (preserved across rebuild)", routineCreatedAt, createdAt) + } + + // catch_up_max out-of-range values are normalized once on upgrade + // (AC-OFFICE-ROUTINE-CATCHUP-001.13): below the floor snaps to the + // default, above the ceiling clamps down to it. + var belowFloorMax, aboveCeilingMax int + if err := db.QueryRow( + `SELECT catch_up_max FROM office_routines WHERE id = 'routine-max-below-floor'`, + ).Scan(&belowFloorMax); err != nil { + t.Fatalf("select below-floor routine: %v", err) + } + if belowFloorMax != 25 { + t.Errorf("routine-max-below-floor catch_up_max = %d, want 25 (default floor)", belowFloorMax) + } + if err := db.QueryRow( + `SELECT catch_up_max FROM office_routines WHERE id = 'routine-max-above-ceiling'`, + ).Scan(&aboveCeilingMax); err != nil { + t.Fatalf("select above-ceiling routine: %v", err) + } + if aboveCeilingMax != 1000 { + t.Errorf("routine-max-above-ceiling catch_up_max = %d, want 1000 (ceiling)", aboveCeilingMax) + } + + // The trigger and run rows must survive the drop+rename — this is the + // cascade PRAGMA foreign_keys=OFF exists to prevent. + var triggerCount, runCount int + if err := db.QueryRow(`SELECT COUNT(*) FROM office_routine_triggers WHERE id = 'trigger-legacy'`).Scan(&triggerCount); err != nil { + t.Fatalf("count trigger: %v", err) + } + if triggerCount != 1 { + t.Errorf("trigger-legacy survived = %d rows, want 1 (cascaded away by the rebuild)", triggerCount) + } + if err := db.QueryRow(`SELECT COUNT(*) FROM office_routine_runs WHERE id = 'run-legacy'`).Scan(&runCount); err != nil { + t.Fatalf("count run: %v", err) + } + if runCount != 1 { + t.Errorf("run-legacy survived = %d rows, want 1 (cascaded away by the rebuild)", runCount) + } + + // Re-running the migration is a no-op: the guard sees a non-stale + // default and skips the rebuild. + if _, err := sqlite.NewWithDB(db, db, nil); err != nil { + t.Fatalf("second init should be idempotent: %v", err) + } + if err := db.QueryRow(`SELECT COUNT(*) FROM office_routines WHERE id = 'routine-legacy'`).Scan(&runCount); err != nil { + t.Fatalf("count routine after replay: %v", err) + } + if runCount != 1 { + t.Errorf("routine-legacy count after replay = %d, want 1", runCount) + } +} diff --git a/apps/backend/internal/office/repository/sqlite/catchup_migrations.go b/apps/backend/internal/office/repository/sqlite/catchup_migrations.go new file mode 100644 index 0000000000..f2b3112452 --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/catchup_migrations.go @@ -0,0 +1,188 @@ +package sqlite + +import ( + "context" + "database/sql" + "fmt" + "strings" + + "go.uber.org/zap" + + "github.com/kandev/kandev/internal/office/models" +) + +// migrateRoutineCatchUp applies every schema and data migration for the +// routine catch-up capability (docs/specs/office/requirements/routine-catch-up.md): +// the three gap-summary columns on office_routine_runs, the deprecated +// catch_up_policy alias value on existing rows, the catch_up_max bound +// clamps, and the office_routines table rebuild that fixes the column's +// stored DEFAULT for upgraded installs (AC-003.3). +func (r *Repository) migrateRoutineCatchUp() { + _ = r.migrate.Apply("office_routine_runs.catch_up_missed_ticks", + `ALTER TABLE office_routine_runs ADD COLUMN catch_up_missed_ticks INTEGER`) + _ = r.migrate.Apply("office_routine_runs.catch_up_first_missed_at", + `ALTER TABLE office_routine_runs ADD COLUMN catch_up_first_missed_at TIMESTAMP`) + _ = r.migrate.Apply("office_routine_runs.catch_up_truncated", + `ALTER TABLE office_routine_runs ADD COLUMN catch_up_truncated INTEGER NOT NULL DEFAULT 0`) + + _ = r.migrate.Apply("office_routines.catch_up_policy_rename", + fmt.Sprintf( + `UPDATE office_routines SET catch_up_policy = '%s' WHERE catch_up_policy = '%s'`, + models.CatchUpPolicySummarizeMissed, models.CatchUpPolicyEnqueueMissedWithCap, + )) + _ = r.migrate.Apply("office_routines.catch_up_max_floor", + fmt.Sprintf(`UPDATE office_routines SET catch_up_max = %d WHERE catch_up_max < 1`, models.CatchUpMaxDefault)) + _ = r.migrate.Apply("office_routines.catch_up_max_ceiling", + fmt.Sprintf(`UPDATE office_routines SET catch_up_max = %d WHERE catch_up_max > %d`, + models.CatchUpMaxCeiling, models.CatchUpMaxCeiling)) + + if err := r.migrateRoutineDefaultPolicyRebuild(); err != nil { + if r.log != nil { + r.log.Warn("routine catch-up default-policy rebuild failed", zap.Error(err)) + } else { + fmt.Println("office sqlite migrate routine catch-up default policy:", err) + } + return + } + if r.log != nil { + r.log.Info("migration applied", zap.String("name", "office_routines.catch_up_policy_default_rebuild")) + } + + // migrateRoutineDefaultPolicyRebuild above only ever fires on SQLite + // (its staleness probe queries sqlite_master/PRAGMA table_info, which + // don't exist on Postgres); this covers the same DEFAULT correction on + // Postgres, which needs no table rebuild — see + // migrateRoutineDefaultPolicyRebuildPostgres. + r.applyRoutineDefaultPolicyRebuildPostgres() +} + +// routineTableExists returns true when office_routines is present. Mirrors +// tasksTableExists's shape for the same reason: a minimal single-domain +// test repository may not have created it yet. +func (r *Repository) routineTableExists() bool { + var exists int + err := r.db.QueryRow( + "SELECT 1 FROM sqlite_master WHERE type='table' AND name='office_routines'", + ).Scan(&exists) + return err == nil && exists == 1 +} + +// routineCatchUpPolicyDefaultIsStale reports whether office_routines' +// catch_up_policy column still carries the retired literal as its stored +// DEFAULT. SQLite has no ALTER COLUMN ... SET DEFAULT, so this is the guard +// that makes migrateRoutineDefaultPolicyRebuild's table-recreate idempotent +// — it only runs while the live schema disagrees. +func (r *Repository) routineCatchUpPolicyDefaultIsStale() bool { + rows, err := r.db.Queryx(`PRAGMA table_info(office_routines)`) + if err != nil { + return false + } + defer func() { _ = rows.Close() }() + for rows.Next() { + var cid int + var name, ctype string + var notnull, pk int + var dflt sql.NullString + if err := rows.Scan(&cid, &name, &ctype, ¬null, &dflt, &pk); err != nil { + return false + } + if name != "catch_up_policy" { + continue + } + return strings.Contains(dflt.String, string(models.CatchUpPolicyEnqueueMissedWithCap)) + } + return false +} + +// migrateRoutineDefaultPolicyRebuild recreates office_routines with a +// corrected catch_up_policy DEFAULT ('summarize_missed'), following the +// table-rebuild pattern runTaskPriorityRecreate already establishes in this +// package: acquire a dedicated connection so PRAGMA and the DDL share one +// SQLite session, disable foreign keys for the duration (office_routine_runs +// and office_routine_triggers both declare +// ON DELETE CASCADE REFERENCES office_routines(id), and dropping the old +// table with enforcement on would cascade every routine run and trigger out +// of existence), then create/copy/drop/rename. Every column is mirrored +// verbatim; only the DEFAULT clause changes. The CREATE/INSERT/DROP/RENAME +// sequence runs inside one transaction (unlike runTaskPriorityRecreate's +// autocommitted steps): SQLite fully supports DDL inside a transaction, and +// this table is dropped and recreated in place rather than alongside a +// still-present sibling, so a crash between DROP and RENAME with no +// transaction would leave neither the original nor the renamed table behind. +func (r *Repository) migrateRoutineDefaultPolicyRebuild() error { + if !r.routineTableExists() { + return nil + } + if !r.routineCatchUpPolicyDefaultIsStale() { + return nil + } + + ctx := context.Background() + conn, err := r.db.Conn(ctx) + if err != nil { + return fmt.Errorf("acquire migration connection: %w", err) + } + defer func() { _ = conn.Close() }() + + if _, err := conn.ExecContext(ctx, `PRAGMA foreign_keys=OFF`); err != nil { + return fmt.Errorf("disable foreign keys: %w", err) + } + defer func() { _, _ = conn.ExecContext(ctx, `PRAGMA foreign_keys=ON`) }() + + tx, err := conn.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("begin routine default-policy migration transaction: %w", err) + } + defer func() { _ = tx.Rollback() }() + + for _, stmt := range routineDefaultPolicyMigrationStatements() { + if _, err := tx.ExecContext(ctx, stmt); err != nil { + return fmt.Errorf("routine default-policy migration step failed: %w", err) + } + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("commit routine default-policy migration: %w", err) + } + return nil +} + +// routineDefaultPolicyMigrationStatements returns the ordered SQL that +// rebuilds office_routines with the corrected catch_up_policy DEFAULT. The +// column list mirrors createRoutineTables' office_routines shape exactly; +// row values (including catch_up_policy itself, already normalized by the +// value-level UPDATE that runs immediately before this in migrateRoutineCatchUp) +// are copied verbatim, so ordering relative to that UPDATE does not matter. +func routineDefaultPolicyMigrationStatements() []string { + return []string{ + `CREATE TABLE office_routines_new ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL, + name TEXT NOT NULL, + description TEXT DEFAULT '', + task_template TEXT NOT NULL DEFAULT '{}', + assignee_agent_profile_id TEXT DEFAULT '', + status TEXT NOT NULL DEFAULT 'active', + concurrency_policy TEXT DEFAULT 'skip_if_active', + catch_up_policy TEXT NOT NULL DEFAULT 'summarize_missed', + catch_up_max INTEGER NOT NULL DEFAULT 25, + variables TEXT DEFAULT '{}', + last_run_at TIMESTAMP, + created_at TIMESTAMP NOT NULL, + updated_at TIMESTAMP NOT NULL + )`, + `INSERT INTO office_routines_new ( + id, workspace_id, name, description, task_template, + assignee_agent_profile_id, status, concurrency_policy, + catch_up_policy, catch_up_max, variables, last_run_at, + created_at, updated_at + ) SELECT + id, workspace_id, name, COALESCE(description,''), + COALESCE(task_template,'{}'), COALESCE(assignee_agent_profile_id,''), + COALESCE(status,'active'), COALESCE(concurrency_policy,'skip_if_active'), + catch_up_policy, catch_up_max, COALESCE(variables,'{}'), last_run_at, + created_at, updated_at + FROM office_routines`, + `DROP TABLE office_routines`, + `ALTER TABLE office_routines_new RENAME TO office_routines`, + } +} diff --git a/apps/backend/internal/office/repository/sqlite/catchup_migrations_postgres.go b/apps/backend/internal/office/repository/sqlite/catchup_migrations_postgres.go new file mode 100644 index 0000000000..6fe99caf1a --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/catchup_migrations_postgres.go @@ -0,0 +1,69 @@ +package sqlite + +import ( + "database/sql" + "errors" + "fmt" + "strings" + + "go.uber.org/zap" + + "github.com/kandev/kandev/internal/db/dialect" + "github.com/kandev/kandev/internal/office/models" +) + +// migrateRoutineDefaultPolicyRebuildPostgres is the PostgreSQL companion to +// migrateRoutineDefaultPolicyRebuild: that function detects staleness via +// sqlite_master and PRAGMA table_info, neither of which exist on Postgres, +// so without this companion an upgraded Postgres install's +// office_routines.catch_up_policy column keeps the retired +// 'enqueue_missed_with_cap' DEFAULT forever, violating +// AC-OFFICE-ROUTINE-CATCHUP-003.3. Unlike SQLite, Postgres supports +// ALTER COLUMN ... SET DEFAULT directly, so no table rebuild is needed here +// at all (mirrors migrateTaskPriorityToTextPostgres's dialect split for the +// same class of migration in internal/task/repository/sqlite). +func (r *Repository) migrateRoutineDefaultPolicyRebuildPostgres() error { + if !dialect.IsPostgres(r.db.DriverName()) { + return nil + } + + var dflt sql.NullString + err := r.db.Get(&dflt, ` + SELECT column_default + FROM information_schema.columns + WHERE table_schema = current_schema() + AND table_name = 'office_routines' + AND column_name = 'catch_up_policy' + `) + if errors.Is(err, sql.ErrNoRows) { + return nil + } + if err != nil { + return fmt.Errorf("inspect office_routines.catch_up_policy default: %w", err) + } + if !dflt.Valid || !strings.Contains(dflt.String, string(models.CatchUpPolicyEnqueueMissedWithCap)) { + return nil + } + + if _, err := r.db.Exec(fmt.Sprintf( + `ALTER TABLE office_routines ALTER COLUMN catch_up_policy SET DEFAULT '%s'`, + models.CatchUpPolicySummarizeMissed, + )); err != nil { + return fmt.Errorf("set office_routines.catch_up_policy default: %w", err) + } + return nil +} + +// applyRoutineDefaultPolicyRebuildPostgres runs the Postgres rebuild and +// logs its own outcome, matching migrateRoutineCatchUp's existing +// warn-and-continue handling for the SQLite path: a failed schema-hygiene +// fix must never block boot. +func (r *Repository) applyRoutineDefaultPolicyRebuildPostgres() { + if err := r.migrateRoutineDefaultPolicyRebuildPostgres(); err != nil { + if r.log != nil { + r.log.Warn("routine catch-up default-policy postgres rebuild failed", zap.Error(err)) + } else { + fmt.Println("office sqlite migrate routine catch-up default policy (postgres):", err) + } + } +} diff --git a/apps/backend/internal/office/repository/sqlite/catchup_repo_test.go b/apps/backend/internal/office/repository/sqlite/catchup_repo_test.go new file mode 100644 index 0000000000..9724b2f937 --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/catchup_repo_test.go @@ -0,0 +1,415 @@ +package sqlite_test + +import ( + "context" + "testing" + "time" + + "github.com/kandev/kandev/internal/office/models" +) + +func seedCatchUpRoutine(t *testing.T, repo interface { + CreateRoutine(ctx context.Context, r *models.Routine) error +}, id string, catchUpMax int) *models.Routine { + t.Helper() + routine := &models.Routine{ + ID: id, + WorkspaceID: "ws-1", + Name: "routine-" + id, + Status: "active", + ConcurrencyPolicy: models.ConcurrencyPolicyAlwaysCreate, + CatchUpMax: catchUpMax, + Variables: "{}", + } + if err := repo.CreateRoutine(context.Background(), routine); err != nil { + t.Fatalf("CreateRoutine: %v", err) + } + return routine +} + +// TestCreateRoutine_CatchUpMaxNormalized covers AC-OFFICE-ROUTINE-CATCHUP-001.13 +// on the create path: 0 clamps to the default floor, 5000 clamps to the +// ceiling, and the mutated struct returned to the caller (not only a +// subsequent GET) already reports the normalized value. +func TestCreateRoutine_CatchUpMaxNormalized(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + + zero := &models.Routine{ID: "r-zero", WorkspaceID: "ws-1", Name: "zero", Variables: "{}", CatchUpMax: 0} + if err := repo.CreateRoutine(ctx, zero); err != nil { + t.Fatalf("CreateRoutine: %v", err) + } + if zero.CatchUpMax != models.CatchUpMaxDefault { + t.Errorf("in-place CatchUpMax = %d, want %d", zero.CatchUpMax, models.CatchUpMaxDefault) + } + got, err := repo.GetRoutine(ctx, "r-zero") + if err != nil { + t.Fatalf("GetRoutine: %v", err) + } + if got.CatchUpMax != models.CatchUpMaxDefault { + t.Errorf("read-back CatchUpMax = %d, want %d", got.CatchUpMax, models.CatchUpMaxDefault) + } + + over := &models.Routine{ID: "r-over", WorkspaceID: "ws-1", Name: "over", Variables: "{}", CatchUpMax: 5000} + if err := repo.CreateRoutine(ctx, over); err != nil { + t.Fatalf("CreateRoutine: %v", err) + } + if over.CatchUpMax != models.CatchUpMaxCeiling { + t.Errorf("in-place CatchUpMax = %d, want %d", over.CatchUpMax, models.CatchUpMaxCeiling) + } + got, err = repo.GetRoutine(ctx, "r-over") + if err != nil { + t.Fatalf("GetRoutine: %v", err) + } + if got.CatchUpMax != models.CatchUpMaxCeiling { + t.Errorf("read-back CatchUpMax = %d, want %d", got.CatchUpMax, models.CatchUpMaxCeiling) + } +} + +// TestCreateRoutineTx_CatchUpMaxNormalized is CreateRoutine's transaction +// sibling — config-sync's own creation path, which bypasses CreateRoutine +// entirely, must clamp identically. +func TestCreateRoutineTx_CatchUpMaxNormalized(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + tx, err := repo.Writer().BeginTxx(ctx, nil) + if err != nil { + t.Fatalf("BeginTxx: %v", err) + } + routine := &models.Routine{ID: "r-tx-over", WorkspaceID: "ws-1", Name: "over", Variables: "{}", CatchUpMax: 5000} + if err := repo.CreateRoutineTx(ctx, tx, routine); err != nil { + t.Fatalf("CreateRoutineTx: %v", err) + } + if err := tx.Commit(); err != nil { + t.Fatalf("Commit: %v", err) + } + if routine.CatchUpMax != models.CatchUpMaxCeiling { + t.Errorf("in-place CatchUpMax = %d, want %d", routine.CatchUpMax, models.CatchUpMaxCeiling) + } + got, err := repo.GetRoutine(ctx, "r-tx-over") + if err != nil { + t.Fatalf("GetRoutine: %v", err) + } + if got.CatchUpMax != models.CatchUpMaxCeiling { + t.Errorf("read-back CatchUpMax = %d, want %d", got.CatchUpMax, models.CatchUpMaxCeiling) + } +} + +// TestUpdateRoutine_CatchUpMaxNormalized covers the update path — the hole +// that existed on both ends before this change (UpdateRoutine wrote +// catch_up_max completely unbound). +func TestUpdateRoutine_CatchUpMaxNormalized(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + routine := seedCatchUpRoutine(t, repo, "r-upd", 25) + + routine.CatchUpMax = 0 + if err := repo.UpdateRoutine(ctx, routine); err != nil { + t.Fatalf("UpdateRoutine: %v", err) + } + if routine.CatchUpMax != models.CatchUpMaxDefault { + t.Errorf("in-place CatchUpMax after 0 = %d, want %d", routine.CatchUpMax, models.CatchUpMaxDefault) + } + + routine.CatchUpMax = 5000 + if err := repo.UpdateRoutine(ctx, routine); err != nil { + t.Fatalf("UpdateRoutine: %v", err) + } + if routine.CatchUpMax != models.CatchUpMaxCeiling { + t.Errorf("in-place CatchUpMax after 5000 = %d, want %d", routine.CatchUpMax, models.CatchUpMaxCeiling) + } + got, err := repo.GetRoutine(ctx, routine.ID) + if err != nil { + t.Fatalf("GetRoutine: %v", err) + } + if got.CatchUpMax != models.CatchUpMaxCeiling { + t.Errorf("read-back CatchUpMax = %d, want %d", got.CatchUpMax, models.CatchUpMaxCeiling) + } +} + +// TestGetDueTriggers_OrderedByNextRunThenID covers AC-OFFICE-ROUTINE-CATCHUP-001.7: +// triggers due at the same instant are returned in ascending id order, and +// an earlier next_run_at always sorts first regardless of id. +func TestGetDueTriggers_OrderedByNextRunThenID(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + routine := seedCatchUpRoutine(t, repo, "r-order", 25) + + now := time.Date(2026, 5, 10, 12, 0, 0, 0, time.UTC) + tied := now.Add(-time.Minute) + earlier := now.Add(-2 * time.Minute) + + mustCreateTrigger := func(id string, next time.Time) { + t.Helper() + if err := repo.CreateRoutineTrigger(ctx, &models.RoutineTrigger{ + ID: id, RoutineID: routine.ID, Kind: "cron", CronExpression: "* * * * *", + NextRunAt: &next, Enabled: true, + }); err != nil { + t.Fatalf("CreateRoutineTrigger(%s): %v", id, err) + } + } + // Two triggers tied on next_run_at, seeded out of id order, plus one + // due earlier that must sort first despite a "larger" id. + mustCreateTrigger("trigger-z", tied) + mustCreateTrigger("trigger-a", tied) + mustCreateTrigger("trigger-m-earliest", earlier) + + due, err := repo.GetDueTriggers(ctx, now) + if err != nil { + t.Fatalf("GetDueTriggers: %v", err) + } + if len(due) != 3 { + t.Fatalf("len(due) = %d, want 3", len(due)) + } + gotIDs := []string{due[0].ID, due[1].ID, due[2].ID} + wantIDs := []string{"trigger-m-earliest", "trigger-a", "trigger-z"} + for i := range wantIDs { + if gotIDs[i] != wantIDs[i] { + t.Errorf("due[%d].ID = %q, want %q (got order %v)", i, gotIDs[i], wantIDs[i], gotIDs) + } + } +} + +// TestListStrandedTriggers_LiveClaimIsNotStale covers the discriminator +// AC-OFFICE-ROUTINE-CATCHUP-001.9 relies on: a claim taken this tick +// (updated_at inside catchUpReclaimAfter) must not be returned as stranded. +func TestListStrandedTriggers_LiveClaimIsNotStale(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + routine := seedCatchUpRoutine(t, repo, "r-live", 25) + + now := time.Now().UTC() + claimAt := now.Add(-time.Minute) + if err := repo.CreateRoutineTrigger(ctx, &models.RoutineTrigger{ + ID: "trigger-live", RoutineID: routine.ID, Kind: "cron", CronExpression: "* * * * *", + NextRunAt: &claimAt, Enabled: true, + }); err != nil { + t.Fatalf("CreateRoutineTrigger: %v", err) + } + claimed, err := repo.ClaimTrigger(ctx, "trigger-live", claimAt) + if err != nil || !claimed { + t.Fatalf("ClaimTrigger: claimed=%v err=%v", claimed, err) + } + + // A claim taken just now (updated_at very fresh) is well inside any + // reasonable reclaim window, so a cutoff far in the past finds nothing. + stranded, err := repo.ListStrandedTriggers(ctx, now.Add(-10*time.Minute)) + if err != nil { + t.Fatalf("ListStrandedTriggers: %v", err) + } + for _, tr := range stranded { + if tr.ID == "trigger-live" { + t.Fatalf("live claim returned as stranded: %+v", tr) + } + } + + // The same row IS stranded once the cutoff is recent enough to include it. + stranded, err = repo.ListStrandedTriggers(ctx, now.Add(time.Minute)) + if err != nil { + t.Fatalf("ListStrandedTriggers: %v", err) + } + found := false + for _, tr := range stranded { + if tr.ID == "trigger-live" { + found = true + } + } + if !found { + t.Fatal("trigger-live not found once cutoff is recent enough") + } +} + +// TestReconcileTriggerNextRun_CASSemantics covers the reconciliation +// write's compare-and-swap: it only affects a row still claimed (next_run_at +// IS NULL, enabled = 1), so a second reconciler racing the first affects +// zero rows and a disabled trigger is left alone. +func TestReconcileTriggerNextRun_CASSemantics(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + routine := seedCatchUpRoutine(t, repo, "r-cas", 25) + + next := time.Now().UTC().Add(-time.Minute) + if err := repo.CreateRoutineTrigger(ctx, &models.RoutineTrigger{ + ID: "trigger-cas", RoutineID: routine.ID, Kind: "cron", CronExpression: "* * * * *", + NextRunAt: &next, Enabled: true, + }); err != nil { + t.Fatalf("CreateRoutineTrigger: %v", err) + } + claimed, err := repo.ClaimTrigger(ctx, "trigger-cas", next) + if err != nil || !claimed { + t.Fatalf("ClaimTrigger: claimed=%v err=%v", claimed, err) + } + + armTo := time.Now().UTC().Add(time.Minute) + won, err := repo.ReconcileTriggerNextRun(ctx, "trigger-cas", armTo) + if err != nil { + t.Fatalf("ReconcileTriggerNextRun: %v", err) + } + if !won { + t.Fatal("first reconcile did not win the CAS") + } + + // A second reconciler racing the first affects zero rows: next_run_at + // is no longer null, so the WHERE predicate matches nothing. + againTo := armTo.Add(time.Minute) + wonAgain, err := repo.ReconcileTriggerNextRun(ctx, "trigger-cas", againTo) + if err != nil { + t.Fatalf("second ReconcileTriggerNextRun: %v", err) + } + if wonAgain { + t.Fatal("second reconcile won the CAS, want it to affect zero rows") + } +} + +// TestReconcileTriggerNextRun_ArmedValueWins asserts the first CAS write +// actually landed the intended next_run_at (not just that it reported +// success), by re-listing due triggers at that instant. +func TestReconcileTriggerNextRun_ArmedValueWins(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + routine := seedCatchUpRoutine(t, repo, "r-cas2", 25) + + next := time.Now().UTC().Add(-time.Minute) + if err := repo.CreateRoutineTrigger(ctx, &models.RoutineTrigger{ + ID: "trigger-cas2", RoutineID: routine.ID, Kind: "cron", CronExpression: "* * * * *", + NextRunAt: &next, Enabled: true, + }); err != nil { + t.Fatalf("CreateRoutineTrigger: %v", err) + } + if claimed, err := repo.ClaimTrigger(ctx, "trigger-cas2", next); err != nil || !claimed { + t.Fatalf("ClaimTrigger: claimed=%v err=%v", claimed, err) + } + + armTo := time.Now().UTC().Add(time.Hour) + if won, err := repo.ReconcileTriggerNextRun(ctx, "trigger-cas2", armTo); err != nil || !won { + t.Fatalf("ReconcileTriggerNextRun: won=%v err=%v", won, err) + } + + due, err := repo.GetDueTriggers(ctx, armTo.Add(time.Minute)) + if err != nil { + t.Fatalf("GetDueTriggers: %v", err) + } + found := false + for _, tr := range due { + if tr.ID == "trigger-cas2" { + found = true + if tr.NextRunAt == nil || !tr.NextRunAt.Equal(armTo) { + t.Errorf("NextRunAt = %v, want %v", tr.NextRunAt, armTo) + } + } + } + if !found { + t.Fatal("reconciled trigger not found among due triggers at the armed instant") + } +} + +// TestCreateRoutineRun_GapSummaryRoundTrip covers the NULL-versus-zero +// distinction (AC-OFFICE-ROUTINE-CATCHUP-002.3): a run created with no gap +// summary must read back with a nil CatchUpMissedTicks, not a stored zero, +// while a run created with one round-trips exactly. +func TestCreateRoutineRun_GapSummaryRoundTrip(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + routine := seedCatchUpRoutine(t, repo, "r-gap", 25) + + noGap := &models.RoutineRun{ + ID: "run-no-gap", RoutineID: routine.ID, Source: "cron", + Status: models.RoutineRunStatusReceived, TriggerPayload: "{}", + } + if err := repo.CreateRoutineRun(ctx, noGap); err != nil { + t.Fatalf("CreateRoutineRun (no gap): %v", err) + } + + missed := 5 + firstMissed := time.Date(2026, 5, 10, 11, 55, 0, 0, time.UTC) + withGap := &models.RoutineRun{ + ID: "run-with-gap", RoutineID: routine.ID, Source: "cron", + Status: models.RoutineRunStatusReceived, TriggerPayload: "{}", + CatchUpMissedTicks: &missed, CatchUpFirstMissedAt: &firstMissed, CatchUpTruncated: true, + } + if err := repo.CreateRoutineRun(ctx, withGap); err != nil { + t.Fatalf("CreateRoutineRun (with gap): %v", err) + } + + runs, err := repo.ListRoutineRuns(ctx, routine.ID, 10, 0) + if err != nil { + t.Fatalf("ListRoutineRuns: %v", err) + } + byID := map[string]*models.RoutineRun{} + for _, r := range runs { + byID[r.ID] = r + } + + gotNoGap, ok := byID["run-no-gap"] + if !ok { + t.Fatal("run-no-gap not found") + } + if gotNoGap.CatchUpMissedTicks != nil { + t.Errorf("no-gap CatchUpMissedTicks = %v, want nil", *gotNoGap.CatchUpMissedTicks) + } + if gotNoGap.CatchUpFirstMissedAt != nil { + t.Errorf("no-gap CatchUpFirstMissedAt = %v, want nil", *gotNoGap.CatchUpFirstMissedAt) + } + if gotNoGap.CatchUpTruncated { + t.Error("no-gap CatchUpTruncated = true, want false") + } + + gotWithGap, ok := byID["run-with-gap"] + if !ok { + t.Fatal("run-with-gap not found") + } + if gotWithGap.CatchUpMissedTicks == nil || *gotWithGap.CatchUpMissedTicks != missed { + t.Errorf("with-gap CatchUpMissedTicks = %v, want %d", gotWithGap.CatchUpMissedTicks, missed) + } + if gotWithGap.CatchUpFirstMissedAt == nil || !gotWithGap.CatchUpFirstMissedAt.Equal(firstMissed) { + t.Errorf("with-gap CatchUpFirstMissedAt = %v, want %v", gotWithGap.CatchUpFirstMissedAt, firstMissed) + } + if !gotWithGap.CatchUpTruncated { + t.Error("with-gap CatchUpTruncated = false, want true") + } +} + +// TestCreateRoutineRun_GapSummaryWriteOnce covers AC-OFFICE-ROUTINE-CATCHUP-002.10: +// the three gap columns are unchanged after a run transitions through +// coalesced and failed. +func TestCreateRoutineRun_GapSummaryWriteOnce(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + routine := seedCatchUpRoutine(t, repo, "r-write-once", 25) + + missed := 3 + firstMissed := time.Date(2026, 5, 10, 11, 55, 0, 0, time.UTC) + run := &models.RoutineRun{ + ID: "run-once", RoutineID: routine.ID, Source: "cron", + Status: models.RoutineRunStatusReceived, TriggerPayload: "{}", + CatchUpMissedTicks: &missed, CatchUpFirstMissedAt: &firstMissed, CatchUpTruncated: false, + } + if err := repo.CreateRoutineRun(ctx, run); err != nil { + t.Fatalf("CreateRoutineRun: %v", err) + } + + if err := repo.UpdateRunCoalesced(ctx, run.ID, "run-other"); err != nil { + t.Fatalf("UpdateRunCoalesced: %v", err) + } + if err := repo.UpdateRunStatus(ctx, run.ID, models.RoutineRunStatusFailed, ""); err != nil { + t.Fatalf("UpdateRunStatus: %v", err) + } + + runs, err := repo.ListRoutineRuns(ctx, routine.ID, 10, 0) + if err != nil { + t.Fatalf("ListRoutineRuns: %v", err) + } + if len(runs) != 1 { + t.Fatalf("len(runs) = %d, want 1", len(runs)) + } + got := runs[0] + if got.Status != models.RoutineRunStatusFailed { + t.Fatalf("Status = %q, want failed", got.Status) + } + if got.CatchUpMissedTicks == nil || *got.CatchUpMissedTicks != missed { + t.Errorf("CatchUpMissedTicks = %v, want %d (unchanged)", got.CatchUpMissedTicks, missed) + } + if got.CatchUpFirstMissedAt == nil || !got.CatchUpFirstMissedAt.Equal(firstMissed) { + t.Errorf("CatchUpFirstMissedAt = %v, want %v (unchanged)", got.CatchUpFirstMissedAt, firstMissed) + } +} diff --git a/apps/backend/internal/office/repository/sqlite/routines.go b/apps/backend/internal/office/repository/sqlite/routines.go index b60d841d2f..0669f51278 100644 --- a/apps/backend/internal/office/repository/sqlite/routines.go +++ b/apps/backend/internal/office/repository/sqlite/routines.go @@ -9,6 +9,7 @@ import ( "github.com/google/uuid" "github.com/jmoiron/sqlx" + "github.com/kandev/kandev/internal/db/dialect" "github.com/kandev/kandev/internal/office/models" ) @@ -60,7 +61,9 @@ func (r *Repository) GetTriggerByPublicID(ctx context.Context, publicID string) return &t, err } -// GetDueTriggers returns cron triggers that are due to fire. +// GetDueTriggers returns cron triggers that are due to fire, ordered by +// next_run_at then id so that TickScheduledTriggers processes the same +// trigger set in a stable order across ticks (AC-OFFICE-ROUTINE-CATCHUP-001.7). // Routine status filtering is done in the service layer via ConfigLoader. func (r *Repository) GetDueTriggers(ctx context.Context, now time.Time) ([]*models.RoutineTrigger, error) { var triggers []*models.RoutineTrigger @@ -68,6 +71,7 @@ func (r *Repository) GetDueTriggers(ctx context.Context, now time.Time) ([]*mode SELECT * FROM office_routine_triggers WHERE kind = 'cron' AND enabled = 1 AND next_run_at IS NOT NULL AND next_run_at <= ? + ORDER BY next_run_at ASC, id ASC `), now) if err != nil { return nil, err @@ -103,6 +107,48 @@ func (r *Repository) UpdateTriggerNextRun(ctx context.Context, triggerID string, return err } +// ListStrandedTriggers returns enabled cron triggers whose next_run_at is +// null (a claim was taken via ClaimTrigger) and whose updated_at is older +// than olderThan. A claim taken this tick or the previous one is never +// returned; only a claim abandoned before that — a process that stopped +// between ClaimTrigger and the re-arm write — is stale +// (AC-OFFICE-ROUTINE-CATCHUP-001.9). +func (r *Repository) ListStrandedTriggers(ctx context.Context, olderThan time.Time) ([]*models.RoutineTrigger, error) { + var triggers []*models.RoutineTrigger + err := r.ro.SelectContext(ctx, &triggers, r.ro.Rebind(` + SELECT * FROM office_routine_triggers + WHERE kind = 'cron' AND enabled = 1 + AND next_run_at IS NULL AND updated_at < ? + ORDER BY id ASC + `), olderThan) + if err != nil { + return nil, err + } + if triggers == nil { + triggers = []*models.RoutineTrigger{} + } + return triggers, nil +} + +// ReconcileTriggerNextRun arms a stranded trigger's next_run_at. Unlike +// UpdateTriggerNextRun (the live claimant's own unconditional re-arm), this +// write is a CAS on `next_run_at IS NULL AND enabled = 1`: a second +// reconciling processor affects zero rows, and a trigger disabled or armed +// between the read and this write is left alone. Returns true if this call +// won the CAS. +func (r *Repository) ReconcileTriggerNextRun(ctx context.Context, triggerID string, nextRunAt 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 IS NULL AND enabled = 1 + `), nextRunAt, time.Now().UTC(), triggerID) + if err != nil { + return false, err + } + rows, err := res.RowsAffected() + return rows > 0, err +} + // DeleteRoutineTrigger deletes a trigger by ID. func (r *Repository) DeleteRoutineTrigger(ctx context.Context, id string) error { _, err := r.db.ExecContext(ctx, r.db.Rebind( @@ -118,13 +164,6 @@ func (r *Repository) CreateRoutine(ctx context.Context, routine *models.Routine) now := time.Now().UTC() routine.CreatedAt = now routine.UpdatedAt = now - - if routine.CatchUpPolicy == "" { - routine.CatchUpPolicy = "enqueue_missed_with_cap" - } - if routine.CatchUpMax <= 0 { - routine.CatchUpMax = 25 - } return r.insertRoutine(ctx, r.db, routine) } @@ -138,16 +177,19 @@ func (r *Repository) CreateRoutineTx(ctx context.Context, tx *sqlx.Tx, routine * now := time.Now().UTC() routine.CreatedAt = now routine.UpdatedAt = now - if routine.CatchUpPolicy == "" { - routine.CatchUpPolicy = "enqueue_missed_with_cap" - } - if routine.CatchUpMax <= 0 { - routine.CatchUpMax = 25 - } return r.insertRoutine(ctx, tx, routine) } +// insertRoutine is the single write-time normalization funnel for +// catch_up_policy and catch_up_max: it reassigns both fields on the +// passed-in struct, before the statement binds them, so a handler that +// serializes the same *Routine back to the caller reports the normalized +// value rather than the submitted one (AC-OFFICE-ROUTINE-CATCHUP-001.13, +// AC-OFFICE-ROUTINE-CATCHUP-003.1). See +// docs/specs/office/system-design/routine-catch-up-01.md. func (r *Repository) insertRoutine(ctx context.Context, ext sqlx.ExtContext, routine *models.Routine) error { + routine.CatchUpPolicy = models.NormaliseCatchUpPolicy(string(routine.CatchUpPolicy)) + routine.CatchUpMax = models.NormaliseCatchUpMax(routine.CatchUpMax) _, err := ext.ExecContext(ctx, r.db.Rebind(` INSERT INTO office_routines ( id, workspace_id, name, description, task_template, @@ -196,15 +238,17 @@ func (r *Repository) ListRoutines(ctx context.Context, workspaceID string) ([]*m return routines, nil } -// UpdateRoutine updates an existing routine. It deliberately does not -// carry last_run_at (AC-OFFICE-LOOP-LIVENESS-001.8): that column only -// ever moves forward, written by TouchRoutineLastRun from a dispatch, and -// a caller here may be holding a routine loaded before a fire (or one -// that never populated the field) — writing it unconditionally would let -// that stale or nil snapshot silently move the value backwards or clear -// it. routine.LastRunAt stays on the struct for reads. +// UpdateRoutine updates an existing routine. It applies the same write-time +// catch_up_policy/catch_up_max normalization funnel as insertRoutine, on the +// passed-in struct, so this second, independent writer of both columns +// cannot diverge from the create path (AC-OFFICE-ROUTINE-CATCHUP-001.13, +// AC-OFFICE-ROUTINE-CATCHUP-003.1). It deliberately does not carry +// last_run_at: that column only moves forward through TouchRoutineLastRun, +// so a stale read-modify-write cannot move it backwards or clear it. func (r *Repository) UpdateRoutine(ctx context.Context, routine *models.Routine) error { routine.UpdatedAt = time.Now().UTC() + routine.CatchUpPolicy = models.NormaliseCatchUpPolicy(string(routine.CatchUpPolicy)) + routine.CatchUpMax = models.NormaliseCatchUpMax(routine.CatchUpMax) _, err := r.db.ExecContext(ctx, r.db.Rebind(` UPDATE office_routines SET name = ?, description = ?, task_template = ?, @@ -289,7 +333,11 @@ func (r *Repository) deleteRoutine(ctx context.Context, ext sqlx.ExtContext, id return err } -// CreateRoutineRun creates a new routine run record. +// CreateRoutineRun creates a new routine run record. The three catch-up +// gap-summary columns are written once, here, from whatever the caller set +// on run (nil when the claim measured no gap); nothing updates them +// afterward, so a run that later transitions to coalesced or failed keeps +// the gap that was measured for its tick (AC-OFFICE-ROUTINE-CATCHUP-002.10). func (r *Repository) CreateRoutineRun(ctx context.Context, run *models.RoutineRun) error { if run.ID == "" { run.ID = uuid.New().String() @@ -300,11 +348,14 @@ func (r *Repository) CreateRoutineRun(ctx context.Context, run *models.RoutineRu INSERT INTO office_routine_runs ( id, routine_id, trigger_id, source, status, trigger_payload, linked_task_id, coalesced_into_run_id, dispatch_fingerprint, + catch_up_missed_ticks, catch_up_first_missed_at, catch_up_truncated, started_at, completed_at, created_at, causation_id - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `), run.ID, run.RoutineID, run.TriggerID, run.Source, run.Status, run.TriggerPayload, run.LinkedTaskID, run.CoalescedIntoRunID, - run.DispatchFingerprint, run.StartedAt, run.CompletedAt, run.CreatedAt, run.CausationID) + run.DispatchFingerprint, run.CatchUpMissedTicks, run.CatchUpFirstMissedAt, + dialect.BoolToInt(run.CatchUpTruncated), run.StartedAt, run.CompletedAt, run.CreatedAt, + run.CausationID) return err } diff --git a/apps/backend/internal/office/repository/sqlite/routines_tx_test.go b/apps/backend/internal/office/repository/sqlite/routines_tx_test.go index a662a3fe54..ed5c1a4d17 100644 --- a/apps/backend/internal/office/repository/sqlite/routines_tx_test.go +++ b/apps/backend/internal/office/repository/sqlite/routines_tx_test.go @@ -50,9 +50,11 @@ func TestCreateRoutineTx_CommitPersists(t *testing.T) { got.TaskTemplate != want.TaskTemplate || got.ConcurrencyPolicy != want.ConcurrencyPolicy { t.Errorf("got %+v, want %+v", got, want) } - // CreateRoutineTx must apply the same defaulting as CreateRoutine. - if got.CatchUpPolicy != "enqueue_missed_with_cap" || got.CatchUpMax != 25 { - t.Errorf("catch-up defaults = (%q, %d), want (enqueue_missed_with_cap, 25)", got.CatchUpPolicy, got.CatchUpMax) + // CreateRoutineTx must apply the same defaulting as CreateRoutine + // (AC-OFFICE-ROUTINE-CATCHUP-003.1: no creation path persists the + // deprecated alias, even when CatchUpPolicy is left unset). + if got.CatchUpPolicy != "summarize_missed" || got.CatchUpMax != 25 { + t.Errorf("catch-up defaults = (%q, %d), want (summarize_missed, 25)", got.CatchUpPolicy, got.CatchUpMax) } } diff --git a/apps/backend/internal/office/repository/sqlite/wakeup_gap_strip_test.go b/apps/backend/internal/office/repository/sqlite/wakeup_gap_strip_test.go new file mode 100644 index 0000000000..5328f3900a --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/wakeup_gap_strip_test.go @@ -0,0 +1,163 @@ +package sqlite_test + +import ( + "context" + "testing" + + officemodels "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/office/repository/sqlite" + "github.com/kandev/kandev/internal/office/shared" +) + +// mergeEntryPoint abstracts over the two coalesce-merge call sites so the +// table test below exercises both with identical fixtures and assertions +// (docs/specs/office/system-design/routine-catch-up-02.md "Testing": "Every +// case below runs against BOTH merge entry points ... That is not +// belt-and-braces: until the refactor lands they are two separate SQL +// statements, and a suite that exercises only the helper passes green with +// the promoted-run path fully broken"). +type mergeEntryPoint struct { + name string + merge func(t *testing.T, repo *sqlite.Repository, ctx context.Context, requestID, runID string) +} + +var mergeEntryPoints = []mergeEntryPoint{ + { + name: "MarkWakeupRequestCoalesced", + merge: func(t *testing.T, repo *sqlite.Repository, ctx context.Context, requestID, runID string) { + t.Helper() + if err := repo.MarkWakeupRequestCoalesced(ctx, requestID, runID); err != nil { + t.Fatalf("MarkWakeupRequestCoalesced: %v", err) + } + }, + }, + { + name: "PromoteRunAndCoalesceWakeupIfQueued", + merge: func(t *testing.T, repo *sqlite.Repository, ctx context.Context, requestID, runID string) { + t.Helper() + ok, err := repo.PromoteRunAndCoalesceWakeupIfQueued(ctx, requestID, runID, shared.RunReasonRoutineDispatchEvent) + if err != nil { + t.Fatalf("PromoteRunAndCoalesceWakeupIfQueued: %v", err) + } + if !ok { + t.Fatal("PromoteRunAndCoalesceWakeupIfQueued: expected queued run to accept the promote") + } + }, + }, +} + +// seedGapStripRun creates a queued run with the given snapshot, ready for +// either merge entry point (PromoteRunAndCoalesceWakeupIfQueued requires +// status "queued"). +func seedGapStripRun(t *testing.T, repo *sqlite.Repository, ctx context.Context, id, snapshot string) { + t.Helper() + run := &officemodels.Run{ + ID: id, AgentProfileID: "agent-1", Reason: shared.RunReasonRoutineDispatchCron, + Payload: "{}", Status: "queued", CoalescedCount: 1, ContextSnapshot: snapshot, + } + if err := repo.CreateRun(ctx, run); err != nil { + t.Fatalf("create run: %v", err) + } +} + +// TestMergeEntryPoints_GapCarryingWakeIntoGaplessRun covers the direction +// AC-002.10 forbids: a gap-carrying routine wake coalescing into an +// in-flight gapless run must leave that run gapless. +func TestMergeEntryPoints_GapCarryingWakeIntoGaplessRun(t *testing.T) { + for _, ep := range mergeEntryPoints { + t.Run(ep.name, func(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + seedGapStripRun(t, repo, ctx, "run-gapless-"+ep.name, `{"routine_id":"r-1"}`) + + reqID := "req-gap-" + ep.name + if err := repo.CreateWakeupRequest(ctx, &sqlite.WakeupRequest{ + ID: reqID, AgentProfileID: "agent-1", Source: "routine", + Payload: `{"routine_id":"r-1","missed_ticks":5,"missed_since":"2026-05-10T11:55:00Z","missed_truncated":true}`, + }); err != nil { + t.Fatalf("create wakeup: %v", err) + } + + ep.merge(t, repo, ctx, reqID, "run-gapless-"+ep.name) + + gotRun, err := repo.GetRunByID(ctx, "run-gapless-"+ep.name) + if err != nil { + t.Fatalf("get run: %v", err) + } + for _, key := range []string{"missed_ticks", "missed_since", "missed_truncated"} { + if contains(gotRun.ContextSnapshot, key) { + t.Errorf("context_snapshot = %q, must not carry %q from a coalesced wake", gotRun.ContextSnapshot, key) + } + } + if !contains(gotRun.ContextSnapshot, `"routine_id":"r-1"`) { + t.Errorf("context_snapshot = %q, want routine_id preserved", gotRun.ContextSnapshot) + } + }) + } +} + +// TestMergeEntryPoints_GaplessWakeIntoGapCarryingRun covers the other +// direction: a gapless wake coalescing into a run that already carries a +// gap must leave the original gap intact, not half-overwritten. +func TestMergeEntryPoints_GaplessWakeIntoGapCarryingRun(t *testing.T) { + for _, ep := range mergeEntryPoints { + t.Run(ep.name, func(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + seedGapStripRun(t, repo, ctx, "run-gap-"+ep.name, + `{"routine_id":"r-1","missed_ticks":5,"missed_since":"2026-05-10T11:55:00Z","missed_truncated":true}`) + + reqID := "req-nogap-" + ep.name + if err := repo.CreateWakeupRequest(ctx, &sqlite.WakeupRequest{ + ID: reqID, AgentProfileID: "agent-1", Source: "routine", + Payload: `{"routine_id":"r-1"}`, + }); err != nil { + t.Fatalf("create wakeup: %v", err) + } + + ep.merge(t, repo, ctx, reqID, "run-gap-"+ep.name) + + gotRun, err := repo.GetRunByID(ctx, "run-gap-"+ep.name) + if err != nil { + t.Fatalf("get run: %v", err) + } + if !contains(gotRun.ContextSnapshot, `"missed_ticks":5`) { + t.Errorf("context_snapshot = %q, want the original gap statement intact", gotRun.ContextSnapshot) + } + if !contains(gotRun.ContextSnapshot, `"missed_truncated":true`) { + t.Errorf("context_snapshot = %q, want the original truncated flag intact", gotRun.ContextSnapshot) + } + }) + } +} + +// TestMergeEntryPoints_NonRoutinePayloadRoundTripsUnchanged proves the +// json_remove strip is safe for a source that never writes the three +// catch-up keys: the merge is byte-for-byte unaffected. +func TestMergeEntryPoints_NonRoutinePayloadRoundTripsUnchanged(t *testing.T) { + for _, ep := range mergeEntryPoints { + t.Run(ep.name, func(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + seedGapStripRun(t, repo, ctx, "run-nonroutine-"+ep.name, `{"existing":"old"}`) + + reqID := "req-comment-" + ep.name + if err := repo.CreateWakeupRequest(ctx, &sqlite.WakeupRequest{ + ID: reqID, AgentProfileID: "agent-1", Source: "comment", + Payload: `{"task_id":"t-1","comment_id":"c-1"}`, + }); err != nil { + t.Fatalf("create wakeup: %v", err) + } + + ep.merge(t, repo, ctx, reqID, "run-nonroutine-"+ep.name) + + gotRun, err := repo.GetRunByID(ctx, "run-nonroutine-"+ep.name) + if err != nil { + t.Fatalf("get run: %v", err) + } + if !contains(gotRun.ContextSnapshot, `"task_id":"t-1"`) || !contains(gotRun.ContextSnapshot, `"comment_id":"c-1"`) { + t.Errorf("context_snapshot = %q, want the non-routine payload merged unchanged", gotRun.ContextSnapshot) + } + }) + } +} diff --git a/apps/backend/internal/office/repository/sqlite/wakeup_json_patch_pin_test.go b/apps/backend/internal/office/repository/sqlite/wakeup_json_patch_pin_test.go new file mode 100644 index 0000000000..d9176368ad --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/wakeup_json_patch_pin_test.go @@ -0,0 +1,66 @@ +package sqlite_test + +import ( + "go/ast" + "go/parser" + "go/token" + "path/filepath" + "runtime" + "testing" +) + +// TestWakeupRequestsJSONPatchAppearsOnce is a source-level pinning +// assertion for the coalesce-merge unification +// (docs/specs/office/system-design/routine-catch-up-01.md, "Delivering the +// gap to the agent"): wakeup_requests.go must contain exactly one SQL +// occurrence of json_patch, meaning exactly one place a coalescing wakeup +// request's payload is merged into a run's context_snapshot. Two SQL sites +// would mean only one of them carries the AC-OFFICE-ROUTINE-CATCHUP-002.10 +// gap-key strip (json_remove of missed_ticks/missed_since/missed_truncated), +// silently reintroducing the bug this design fixes. +// +// The naive `grep -c json_patch` on this file returns THREE lines: the two +// SQL string literals this test counts, plus the helper's own doc comment, +// which names the function in prose. Parsing with go/parser and counting +// only *ast.BasicLit string literals excludes the comment by construction — +// an unqualified "exactly once" assertion would FAIL against the correct +// implementation. +func TestWakeupRequestsJSONPatchAppearsOnce(t *testing.T) { + _, sourceFile, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("runtime.Caller failed") + } + targetPath := filepath.Join(filepath.Dir(sourceFile), "wakeup_requests.go") + + fileSet := token.NewFileSet() + file, err := parser.ParseFile(fileSet, targetPath, nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", targetPath, err) + } + + count := 0 + ast.Inspect(file, func(n ast.Node) bool { + lit, ok := n.(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return true + } + if containsJSONPatch(lit.Value) { + count++ + } + return true + }) + if count != 1 { + t.Fatalf("json_patch appears in %d SQL string literal(s) in wakeup_requests.go, want exactly 1 "+ + "(the single mergeWakeupPayloadIntoRunSnapshotWith call site)", count) + } +} + +func containsJSONPatch(s string) bool { + const needle = "json_patch" + for i := 0; i+len(needle) <= len(s); i++ { + if s[i:i+len(needle)] == needle { + return true + } + } + return false +} diff --git a/apps/backend/internal/office/repository/sqlite/wakeup_requests.go b/apps/backend/internal/office/repository/sqlite/wakeup_requests.go index 10151299b5..f1bbe7dadb 100644 --- a/apps/backend/internal/office/repository/sqlite/wakeup_requests.go +++ b/apps/backend/internal/office/repository/sqlite/wakeup_requests.go @@ -9,6 +9,7 @@ import ( "time" "github.com/jackc/pgx/v5/pgconn" + "github.com/jmoiron/sqlx" ) // Wakeup request status constants — kept in sync with the spec's @@ -234,14 +235,7 @@ func (r *Repository) PromoteRunAndCoalesceWakeupIfQueued( `), runID); err != nil { return false, err } - if _, err := tx.ExecContext(ctx, tx.Rebind(` - UPDATE runs - SET context_snapshot = json_patch( - COALESCE(NULLIF(context_snapshot, ''), '{}'), - (SELECT payload FROM agent_wakeup_requests WHERE id = ?) - ) - WHERE id = ? - `), requestID, runID); err != nil { + if err := mergeWakeupPayloadIntoRunSnapshotWith(ctx, tx, requestID, runID); err != nil { return false, err } if err := tx.Commit(); err != nil { @@ -261,18 +255,46 @@ func (r *Repository) bumpRunCoalescedCount(ctx context.Context, runID string) er } // mergeWakeupPayloadIntoRunSnapshot merges the wakeup request's payload -// into the target run's context_snapshot via SQLite's json_patch (top- -// level merge: keys from the request payload overwrite same-named keys -// already on the snapshot). When the snapshot is empty / NULL it -// initialises to "{}" first so the patch lands on a valid object. +// into the target run's context_snapshot. Delegates to +// mergeWakeupPayloadIntoRunSnapshotWith on r.db — the non-transactional +// merge site; PromoteRunAndCoalesceWakeupIfQueued calls the same helper on +// its own tx instead of duplicating the SQL, so there is exactly one +// json_patch call site for both merge entry points. func (r *Repository) mergeWakeupPayloadIntoRunSnapshot( ctx context.Context, requestID, runID string, ) error { - _, err := r.db.ExecContext(ctx, r.db.Rebind(` + return mergeWakeupPayloadIntoRunSnapshotWith(ctx, r.db, requestID, runID) +} + +// mergeWakeupPayloadIntoRunSnapshotWith merges the wakeup request's +// payload into the target run's context_snapshot via SQLite's json_patch +// (top-level merge: keys from the request payload overwrite same-named +// keys already on the snapshot). When the snapshot is empty / NULL it +// initialises to "{}" first so the patch lands on a valid object. +// +// The three routine catch-up gap-summary keys are stripped from the +// incoming payload before the patch via json_remove: a coalesced or +// promoted wakeup request never adds, changes, or removes the gap +// statement an existing run's agent already carries +// (AC-OFFICE-ROUTINE-CATCHUP-002.10) — each run's gap belongs only to the +// claim that created it. json_remove of an absent key is a no-op in +// SQLite, so this is byte-for-byte safe for every non-routine wakeup +// source, none of which writes these keys. +// +// exec is r.db for the non-transactional caller or a *sqlx.Tx for the +// transactional one — sqlx.ExtContext (which both satisfy) already +// includes Rebind via its embedded binder interface. +func mergeWakeupPayloadIntoRunSnapshotWith( + ctx context.Context, exec sqlx.ExtContext, requestID, runID string, +) error { + _, err := exec.ExecContext(ctx, exec.Rebind(` UPDATE runs SET context_snapshot = json_patch( COALESCE(NULLIF(context_snapshot, ''), '{}'), - (SELECT payload FROM agent_wakeup_requests WHERE id = ?) + json_remove( + (SELECT payload FROM agent_wakeup_requests WHERE id = ?), + '$.missed_ticks', '$.missed_since', '$.missed_truncated' + ) ) WHERE id = ? `), requestID, runID) diff --git a/apps/backend/internal/office/routines/catch_up_test.go b/apps/backend/internal/office/routines/catch_up_test.go index f797449150..7f2c95958e 100644 --- a/apps/backend/internal/office/routines/catch_up_test.go +++ b/apps/backend/internal/office/routines/catch_up_test.go @@ -7,33 +7,40 @@ import ( "github.com/kandev/kandev/internal/office/models" ) -// TestComputeRoutineMissed_HappyPath: NextRunAt == now, no backlog — -// expect runCount=1 (one fire due now), advance to next cron tick. -func TestComputeRoutineMissed_HappyPath(t *testing.T) { +// TestComputeCatchUp_HappyPath: NextRunAt == now, no backlog — expect +// ElapsedTicks=1 (one fire due now), NextRunAt advances to the next cron +// tick, and no gap summary (FirstMissedAt zero, not truncated). +func TestComputeCatchUp_HappyPath(t *testing.T) { t0 := time.Date(2026, 5, 10, 12, 0, 0, 0, time.UTC) trigger := &RoutineTrigger{ CronExpression: "* * * * *", Timezone: "", NextRunAt: &t0, } - routine := &Routine{CatchUpPolicy: models.CatchUpPolicyEnqueueMissedWithCap, CatchUpMax: 25} + routine := &Routine{CatchUpPolicy: models.CatchUpPolicySummarizeMissed, CatchUpMax: 25} - count, advanceTo, err := computeRoutineMissed(trigger, routine, t0) - if err != nil { - t.Fatalf("computeRoutineMissed: %v", err) + result := computeCatchUp(trigger, routine, t0) + if result.Unknown { + t.Fatalf("Unknown = true, want false") } - if count != 1 { - t.Errorf("count = %d, want 1", count) + if result.ElapsedTicks != 1 { + t.Errorf("ElapsedTicks = %d, want 1", result.ElapsedTicks) } - if !advanceTo.Equal(t0.Add(time.Minute)) { - t.Errorf("advanceTo = %v, want %v", advanceTo, t0.Add(time.Minute)) + if !result.FirstMissedAt.IsZero() { + t.Errorf("FirstMissedAt = %v, want zero (ElapsedTicks <= 1)", result.FirstMissedAt) + } + if result.Truncated { + t.Error("Truncated = true, want false") + } + if !result.NextRunAt.Equal(t0.Add(time.Minute)) { + t.Errorf("NextRunAt = %v, want %v", result.NextRunAt, t0.Add(time.Minute)) } } -// TestComputeRoutineMissed_BackendDownTenMinutes: NextRunAt is 10 minutes -// before now with an "every minute" cron — expect 11 ticks counted (10 -// missed + 1 due now), advance to the next-minute tick. -func TestComputeRoutineMissed_BackendDownTenMinutes(t *testing.T) { +// TestComputeCatchUp_BackendDownTenMinutes: NextRunAt is 10 minutes before +// now with an "every minute" cron — expect 11 elapsed ticks (10 missed + 1 +// due now), FirstMissedAt exactly the armed next_run_at, not truncated. +func TestComputeCatchUp_BackendDownTenMinutes(t *testing.T) { now := time.Date(2026, 5, 10, 12, 10, 0, 0, time.UTC) tenAgo := now.Add(-10 * time.Minute) trigger := &RoutineTrigger{ @@ -41,24 +48,30 @@ func TestComputeRoutineMissed_BackendDownTenMinutes(t *testing.T) { Timezone: "", NextRunAt: &tenAgo, } - routine := &Routine{CatchUpPolicy: models.CatchUpPolicyEnqueueMissedWithCap, CatchUpMax: 25} + routine := &Routine{CatchUpPolicy: models.CatchUpPolicySummarizeMissed, CatchUpMax: 25} - count, advanceTo, err := computeRoutineMissed(trigger, routine, now) - if err != nil { - t.Fatalf("computeRoutineMissed: %v", err) + result := computeCatchUp(trigger, routine, now) + if result.Unknown { + t.Fatalf("Unknown = true, want false") } - if count != 11 { - t.Errorf("count = %d, want 11", count) + if result.ElapsedTicks != 11 { + t.Errorf("ElapsedTicks = %d, want 11", result.ElapsedTicks) } - if !advanceTo.Equal(now.Add(time.Minute)) { - t.Errorf("advanceTo = %v, want %v", advanceTo, now.Add(time.Minute)) + if !result.FirstMissedAt.Equal(tenAgo) { + t.Errorf("FirstMissedAt = %v, want %v (exact armed next_run_at)", result.FirstMissedAt, tenAgo) + } + if result.Truncated { + t.Error("Truncated = true, want false") + } + if !result.NextRunAt.Equal(now.Add(time.Minute)) { + t.Errorf("NextRunAt = %v, want %v", result.NextRunAt, now.Add(time.Minute)) } } -// TestComputeRoutineMissed_HitsCap: 100 minutes missed with cap 5 — -// expect runCount=5 (capped), advanceTo = next future tick from now so -// the trigger leaves the catch-up window cleanly. -func TestComputeRoutineMissed_HitsCap(t *testing.T) { +// TestComputeCatchUp_HitsCap: 100 minutes missed with cap 5 — expect +// ElapsedTicks=5 (capped), Truncated=true, FirstMissedAt still exact and +// un-truncated, NextRunAt strictly after now. +func TestComputeCatchUp_HitsCap(t *testing.T) { now := time.Date(2026, 5, 10, 12, 0, 0, 0, time.UTC) hundredAgo := now.Add(-100 * time.Minute) trigger := &RoutineTrigger{ @@ -66,44 +79,69 @@ func TestComputeRoutineMissed_HitsCap(t *testing.T) { Timezone: "", NextRunAt: &hundredAgo, } - routine := &Routine{CatchUpPolicy: models.CatchUpPolicyEnqueueMissedWithCap, CatchUpMax: 5} + routine := &Routine{CatchUpPolicy: models.CatchUpPolicySummarizeMissed, CatchUpMax: 5} - count, advanceTo, err := computeRoutineMissed(trigger, routine, now) - if err != nil { - t.Fatalf("computeRoutineMissed: %v", err) + result := computeCatchUp(trigger, routine, now) + if result.Unknown { + t.Fatalf("Unknown = true, want false") } - if count != 5 { - t.Errorf("count = %d, want 5 (capped)", count) + if result.ElapsedTicks != 5 { + t.Errorf("ElapsedTicks = %d, want 5 (capped)", result.ElapsedTicks) } - if !advanceTo.After(now) { - t.Errorf("advanceTo = %v, want a time after %v", advanceTo, now) + if !result.Truncated { + t.Error("Truncated = false, want true") + } + if !result.FirstMissedAt.Equal(hundredAgo) { + t.Errorf("FirstMissedAt = %v, want %v (exact, even truncated)", result.FirstMissedAt, hundredAgo) + } + if !result.NextRunAt.After(now) { + t.Errorf("NextRunAt = %v, want a time after %v", result.NextRunAt, now) } } -// TestComputeRoutineMissed_DefaultCap: CatchUpMax of 0 falls back to -// the default (25). 100 minutes missed produces runCount=25. -func TestComputeRoutineMissed_DefaultCap(t *testing.T) { +// TestComputeCatchUp_DefaultCap: CatchUpMax of 0 clamps to the default +// (25) via models.NormaliseCatchUpMax. 100 minutes missed produces +// ElapsedTicks=25 (AC-001.4). +func TestComputeCatchUp_DefaultCap(t *testing.T) { now := time.Date(2026, 5, 10, 12, 0, 0, 0, time.UTC) hundredAgo := now.Add(-100 * time.Minute) trigger := &RoutineTrigger{ CronExpression: "* * * * *", NextRunAt: &hundredAgo, } - // CatchUpMax=0 → defaultCatchUpMax (25). - routine := &Routine{CatchUpPolicy: models.CatchUpPolicyEnqueueMissedWithCap, CatchUpMax: 0} + routine := &Routine{CatchUpPolicy: models.CatchUpPolicySummarizeMissed, CatchUpMax: 0} - count, _, err := computeRoutineMissed(trigger, routine, now) - if err != nil { - t.Fatalf("computeRoutineMissed: %v", err) - } - if count != defaultCatchUpMax { - t.Errorf("count = %d, want %d (defaultCatchUpMax)", count, defaultCatchUpMax) + result := computeCatchUp(trigger, routine, now) + if result.ElapsedTicks != models.CatchUpMaxDefault { + t.Errorf("ElapsedTicks = %d, want %d (CatchUpMaxDefault)", result.ElapsedTicks, models.CatchUpMaxDefault) } } -// TestComputeRoutineMissed_HourlyCron: NextRunAt is 5 hours before now -// with an hourly cron — expect 6 ticks counted (5 missed + 1 due). -func TestComputeRoutineMissed_HourlyCron(t *testing.T) { +// TestComputeCatchUp_CeilingClamp: CatchUpMax of 5000 clamps to the +// ceiling (1000) via models.NormaliseCatchUpMax (AC-001.4). A gap of 2000 +// minutes with an every-minute cron produces ElapsedTicks capped at 1000, +// not 2000. +func TestComputeCatchUp_CeilingClamp(t *testing.T) { + now := time.Date(2026, 5, 10, 12, 0, 0, 0, time.UTC) + longAgo := now.Add(-2000 * time.Minute) + trigger := &RoutineTrigger{ + CronExpression: "* * * * *", + NextRunAt: &longAgo, + } + routine := &Routine{CatchUpPolicy: models.CatchUpPolicySummarizeMissed, CatchUpMax: 5000} + + result := computeCatchUp(trigger, routine, now) + if result.ElapsedTicks != models.CatchUpMaxCeiling { + t.Errorf("ElapsedTicks = %d, want %d (CatchUpMaxCeiling)", result.ElapsedTicks, models.CatchUpMaxCeiling) + } + if !result.Truncated { + t.Error("Truncated = false, want true") + } +} + +// TestComputeCatchUp_HourlyCron: NextRunAt is 5 hours before now with an +// hourly cron — expect 6 elapsed ticks (5 missed + 1 due). +func TestComputeCatchUp_HourlyCron(t *testing.T) { now := time.Date(2026, 5, 10, 12, 0, 0, 0, time.UTC) fiveAgo := now.Add(-5 * time.Hour) trigger := &RoutineTrigger{ @@ -112,14 +150,166 @@ func TestComputeRoutineMissed_HourlyCron(t *testing.T) { } routine := &Routine{CatchUpMax: 25} - count, advanceTo, err := computeRoutineMissed(trigger, routine, now) - if err != nil { - t.Fatalf("computeRoutineMissed: %v", err) + result := computeCatchUp(trigger, routine, now) + if result.ElapsedTicks != 6 { + t.Errorf("ElapsedTicks = %d, want 6", result.ElapsedTicks) } - if count != 6 { - t.Errorf("count = %d, want 6", count) - } - if !advanceTo.Equal(now.Add(time.Hour)) { - t.Errorf("advanceTo = %v, want %v", advanceTo, now.Add(time.Hour)) + if !result.NextRunAt.Equal(now.Add(time.Hour)) { + t.Errorf("NextRunAt = %v, want %v", result.NextRunAt, now.Add(time.Hour)) + } +} + +// TestComputeCatchUp_ElapsedTicksOne_NoFirstMissedAt: exactly one elapsed +// tick means missedTicks == 0, so FirstMissedAt stays zero even though the +// walk succeeded — this is the boundary buildGapSummary relies on to +// record no gap summary. +func TestComputeCatchUp_ElapsedTicksOne_NoFirstMissedAt(t *testing.T) { + now := time.Date(2026, 5, 10, 12, 0, 30, 0, time.UTC) + oneAgo := now.Add(-30 * time.Second) + trigger := &RoutineTrigger{ + CronExpression: "* * * * *", + NextRunAt: &oneAgo, + } + routine := &Routine{CatchUpPolicy: models.CatchUpPolicySummarizeMissed, CatchUpMax: 25} + + result := computeCatchUp(trigger, routine, now) + if result.ElapsedTicks != 1 { + t.Fatalf("ElapsedTicks = %d, want 1", result.ElapsedTicks) + } + if !result.FirstMissedAt.IsZero() { + t.Errorf("FirstMissedAt = %v, want zero", result.FirstMissedAt) + } +} + +// TestComputeCatchUp_MalformedExpression: an unparseable cron expression +// drives Unknown=true and NextRunAt exactly 24 hours after the processing +// instant (AC-001.11) — the failure is deterministic in the stored +// (expression, timezone) pair, so the re-arm is the trigger's permanent +// dispatch cadence until it is deleted and recreated. +func TestComputeCatchUp_MalformedExpression(t *testing.T) { + now := time.Date(2026, 5, 10, 12, 0, 0, 0, time.UTC) + past := now.Add(-time.Hour) + trigger := &RoutineTrigger{ + CronExpression: "not a cron expression", + NextRunAt: &past, + } + routine := &Routine{CatchUpPolicy: models.CatchUpPolicySummarizeMissed, CatchUpMax: 25} + + result := computeCatchUp(trigger, routine, now) + if !result.Unknown { + t.Fatal("Unknown = false, want true") + } + want := now.Add(catchUpFallbackInterval) + if !result.NextRunAt.Equal(want) { + t.Errorf("NextRunAt = %v, want %v (now + 24h)", result.NextRunAt, want) + } + // AC-001.11 requires the eventual warning to name "the underlying + // error", which means it has to survive on catchUpResult rather than + // being discarded at the point of failure. + if result.Err == nil { + t.Error("Err = nil, want the underlying NextCronTime parse error") + } + + // A second tick a day later dispatches once more, not repeatedly: the + // re-armed NextRunAt is itself now due, and re-computing from it + // produces the same 24h fallback rather than a hot loop. + again := computeCatchUp(trigger, routine, result.NextRunAt) + if !again.Unknown { + t.Fatal("second computeCatchUp: Unknown = false, want true") + } + if !again.NextRunAt.Equal(result.NextRunAt.Add(catchUpFallbackInterval)) { + t.Errorf("second NextRunAt = %v, want %v", again.NextRunAt, result.NextRunAt.Add(catchUpFallbackInterval)) + } +} + +// TestBuildGapSummary_SkipMissedRecordsNothing: skip_missed never records +// a gap summary, whatever the elapsed-tick count (AC-002.7). +func TestBuildGapSummary_SkipMissedRecordsNothing(t *testing.T) { + routine := &Routine{CatchUpPolicy: models.CatchUpPolicySkipMissed} + result := catchUpResult{ElapsedTicks: 10, FirstMissedAt: time.Now(), Truncated: true} + if gap := buildGapSummary(routine, result); gap != nil { + t.Errorf("gap = %+v, want nil", gap) + } +} + +// TestBuildGapSummary_UnknownRecordsNothing: a failed walk never records a +// gap, even though ElapsedTicks is technically non-meaningful (AC-001.6). +func TestBuildGapSummary_UnknownRecordsNothing(t *testing.T) { + routine := &Routine{CatchUpPolicy: models.CatchUpPolicySummarizeMissed} + result := catchUpResult{Unknown: true} + if gap := buildGapSummary(routine, result); gap != nil { + t.Errorf("gap = %+v, want nil", gap) + } +} + +// TestBuildGapSummary_ZeroMissedRecordsNothing: exactly one elapsed tick +// (missedTicks == 0) records no gap summary, whatever truncated would say +// (AC-002.2). +func TestBuildGapSummary_ZeroMissedRecordsNothing(t *testing.T) { + routine := &Routine{CatchUpPolicy: models.CatchUpPolicySummarizeMissed} + result := catchUpResult{ElapsedTicks: 1} + if gap := buildGapSummary(routine, result); gap != nil { + t.Errorf("gap = %+v, want nil", gap) + } +} + +// TestBuildGapSummary_CatchUpMaxOne: catch_up_max == 1 makes missedTicks +// structurally zero regardless of the gap's real size — an opt-out of gap +// reporting, not an edge case (AC-002.11). +func TestBuildGapSummary_CatchUpMaxOne(t *testing.T) { + now := time.Date(2026, 5, 10, 12, 0, 0, 0, time.UTC) + longAgo := now.Add(-100 * time.Hour) + trigger := &RoutineTrigger{CronExpression: "* * * * *", NextRunAt: &longAgo} + routine := &Routine{CatchUpPolicy: models.CatchUpPolicySummarizeMissed, CatchUpMax: 1} + + // Truncation and a zero missed-tick count can co-occur only at exactly + // catch_up_max == 1 (above 1 a truncated count is always >= cap - 1); + // what matters here is that no gap summary is ever produced, whatever + // Truncated says. + result := computeCatchUp(trigger, routine, now) + if gap := buildGapSummary(routine, result); gap != nil { + t.Errorf("gap = %+v, want nil (catch_up_max == 1 opts out)", gap) + } +} + +// TestBuildGapSummary_SummarizeMissedRecordsGap: the happy path — a +// multi-tick gap under summarize_missed records the count, the exact +// first-missed timestamp, and truncated. +func TestBuildGapSummary_SummarizeMissedRecordsGap(t *testing.T) { + first := time.Date(2026, 5, 10, 11, 55, 0, 0, time.UTC) + result := catchUpResult{ElapsedTicks: 6, FirstMissedAt: first, Truncated: true} + routine := &Routine{CatchUpPolicy: models.CatchUpPolicySummarizeMissed} + + gap := buildGapSummary(routine, result) + if gap == nil { + t.Fatal("gap = nil, want non-nil") + } + if gap.MissedTicks != 5 { + t.Errorf("MissedTicks = %d, want 5", gap.MissedTicks) + } + if !gap.FirstMissed.Equal(first) { + t.Errorf("FirstMissed = %v, want %v", gap.FirstMissed, first) + } + if !gap.Truncated { + t.Error("Truncated = false, want true") + } +} + +// TestBuildGapSummary_DeprecatedAliasStillSummarizes proves the alias +// normalizes to summarize_missed's behaviour rather than being treated as +// an unrecognized (and thus also summarizing, per models.NormaliseCatchUpPolicy) +// value by accident — the policy comparison in buildGapSummary only ever +// special-cases skip_missed, so any non-skip value, including the raw +// alias constant, produces a gap summary. +func TestBuildGapSummary_DeprecatedAliasStillSummarizes(t *testing.T) { + result := catchUpResult{ElapsedTicks: 3, FirstMissedAt: time.Now()} + routine := &Routine{CatchUpPolicy: models.CatchUpPolicyEnqueueMissedWithCap} + + gap := buildGapSummary(routine, result) + if gap == nil { + t.Fatal("gap = nil, want non-nil") + } + if gap.MissedTicks != 2 { + t.Errorf("MissedTicks = %d, want 2", gap.MissedTicks) } } diff --git a/apps/backend/internal/office/routines/handler.go b/apps/backend/internal/office/routines/handler.go index c0373a07c4..1fe5bb4169 100644 --- a/apps/backend/internal/office/routines/handler.go +++ b/apps/backend/internal/office/routines/handler.go @@ -104,7 +104,7 @@ func (h *Handler) createRoutine(c *gin.Context) { } catchUpPolicy := models.RoutineCatchUpPolicy(req.CatchUpPolicy) if catchUpPolicy == "" { - catchUpPolicy = models.CatchUpPolicyEnqueueMissedWithCap + catchUpPolicy = models.CatchUpPolicySummarizeMissed } if !concurrencyPolicy.Valid() { c.JSON(http.StatusBadRequest, gin.H{"error": "invalid concurrency_policy: " + req.ConcurrencyPolicy}) diff --git a/apps/backend/internal/office/routines/handler_catchup_test.go b/apps/backend/internal/office/routines/handler_catchup_test.go new file mode 100644 index 0000000000..518ce60304 --- /dev/null +++ b/apps/backend/internal/office/routines/handler_catchup_test.go @@ -0,0 +1,97 @@ +package routines_test + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gin-gonic/gin" + + "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/office/routines" +) + +// newTestRoutineRouter mounts the routine HTTP routes over a fresh +// RoutineService backed by in-memory SQLite. +func newTestRoutineRouter(t *testing.T) *gin.Engine { + t.Helper() + gin.SetMode(gin.TestMode) + svc := newTestRoutineService(t) + router := gin.New() + routines.RegisterRoutes(router.Group("/api/v1"), routines.NewHandler(svc)) + return router +} + +func doRoutineRequest(t *testing.T, router *gin.Engine, method, path, body string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(method, path, strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + return rec +} + +// TestCreateRoutineHandler_CatchUpMaxNormalizedInResponse covers +// AC-OFFICE-ROUTINE-CATCHUP-001.13: a routine read back through the API +// must report the same normalized catch_up_max the tick honours, not the +// raw value the client submitted. +func TestCreateRoutineHandler_CatchUpMaxNormalizedInResponse(t *testing.T) { + router := newTestRoutineRouter(t) + + rec := doRoutineRequest(t, router, http.MethodPost, "/api/v1/workspaces/ws-1/routines", + `{"name":"Handler routine","catch_up_max":5000,"variables":"{}"}`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201, body = %s", rec.Code, rec.Body.String()) + } + var resp routines.RoutineResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode response: %v", err) + } + if resp.Routine.CatchUpMax != models.CatchUpMaxCeiling { + t.Errorf("response catch_up_max = %d, want %d (ceiling)", resp.Routine.CatchUpMax, models.CatchUpMaxCeiling) + } +} + +// TestUpdateRoutineHandler_CatchUpMaxNormalizedInResponse is the same +// contract on the PATCH path: submitting a below-floor value must come +// back normalized in the same response, not only on a subsequent GET. +func TestUpdateRoutineHandler_CatchUpMaxNormalizedInResponse(t *testing.T) { + router := newTestRoutineRouter(t) + + createRec := doRoutineRequest(t, router, http.MethodPost, "/api/v1/workspaces/ws-1/routines", + `{"name":"Handler routine","catch_up_max":25,"variables":"{}"}`) + if createRec.Code != http.StatusCreated { + t.Fatalf("create status = %d, want 201, body = %s", createRec.Code, createRec.Body.String()) + } + var created routines.RoutineResponse + if err := json.Unmarshal(createRec.Body.Bytes(), &created); err != nil { + t.Fatalf("decode create response: %v", err) + } + + updateRec := doRoutineRequest(t, router, http.MethodPatch, "/api/v1/routines/"+created.Routine.ID, + `{"catch_up_max":0}`) + if updateRec.Code != http.StatusOK { + t.Fatalf("update status = %d, want 200, body = %s", updateRec.Code, updateRec.Body.String()) + } + var updated routines.RoutineResponse + if err := json.Unmarshal(updateRec.Body.Bytes(), &updated); err != nil { + t.Fatalf("decode update response: %v", err) + } + if updated.Routine.CatchUpMax != models.CatchUpMaxDefault { + t.Errorf("response catch_up_max = %d, want %d (default floor)", updated.Routine.CatchUpMax, models.CatchUpMaxDefault) + } + + getRec := doRoutineRequest(t, router, http.MethodGet, "/api/v1/routines/"+created.Routine.ID, "") + if getRec.Code != http.StatusOK { + t.Fatalf("get status = %d, want 200, body = %s", getRec.Code, getRec.Body.String()) + } + var fetched routines.RoutineResponse + if err := json.Unmarshal(getRec.Body.Bytes(), &fetched); err != nil { + t.Fatalf("decode get response: %v", err) + } + if fetched.Routine.CatchUpMax != models.CatchUpMaxDefault { + t.Errorf("read-back catch_up_max = %d, want %d", fetched.Routine.CatchUpMax, models.CatchUpMaxDefault) + } +} diff --git a/apps/backend/internal/office/routines/service.go b/apps/backend/internal/office/routines/service.go index fd1eb29d0e..827bf05579 100644 --- a/apps/backend/internal/office/routines/service.go +++ b/apps/backend/internal/office/routines/service.go @@ -28,6 +28,11 @@ import ( // concrete adapter translates the sqlite layer's // ErrWakeupIdempotencyConflict into this sentinel so routines never // imports office/repository/sqlite directly (see routineWakeupAdapter). +// It is not a materialisation failure (AC-OFFICE-ROUTINE-CATCHUP-001.10 +// second sentence): a duplicate wakeup request within the same +// wall-clock minute is a successful dedup, so +// materialiseLightweightRoutineRun absorbs it as a terminal "done" run +// rather than failing. var ErrWakeupAlreadyRequested = errors.New("wakeup request already requested") // ErrInvalidTrigger indicates a trigger create request that failed @@ -49,6 +54,8 @@ type Repository interface { GetDueTriggers(ctx context.Context, now time.Time) ([]*RoutineTrigger, error) ClaimTrigger(ctx context.Context, triggerID string, oldNextRunAt 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) DeleteRoutineTrigger(ctx context.Context, id string) error CreateRoutineRun(ctx context.Context, run *RoutineRun) error @@ -185,7 +192,7 @@ const CoordinatorRoutineCron = "*/5 * * * *" // CreateDefaultCoordinatorRoutine installs the pre-baked // "Coordinator heartbeat" routine for a coordinator-role agent. The // shape is described in office-heartbeat-as-routine: lightweight -// (empty task_template), coalesce_if_active, enqueue_missed_with_cap +// (empty task_template), coalesce_if_active, summarize_missed // with a max of 25, status=active, with a single cron trigger firing // every five minutes (UTC). // @@ -210,7 +217,7 @@ func (s *RoutineService) CreateDefaultCoordinatorRoutine( AssigneeAgentProfileID: agentID, Status: "active", ConcurrencyPolicy: models.ConcurrencyPolicyCoalesceIfActive, - CatchUpPolicy: models.CatchUpPolicyEnqueueMissedWithCap, + CatchUpPolicy: models.CatchUpPolicySummarizeMissed, CatchUpMax: 25, Variables: "{}", } @@ -401,7 +408,27 @@ func (s *RoutineService) ListAllRoutineRuns(ctx context.Context, wsID string, li // -- Dispatch -- -// TickScheduledTriggers queries due cron triggers, claims each, and dispatches. +// catchUpFallbackInterval is the re-arm interval computeCatchUp uses when +// the elapsed-tick walk fails for a reason other than +// shared.ErrUnsatisfiableCron (AC-OFFICE-ROUTINE-CATCHUP-001.11). The +// failure is deterministic in the trigger's stored (expression, timezone) +// pair, not in the time argument: re-arming to the original due time +// instead would make the trigger due again on the very next 30-second +// scheduler tick, retrying (and failing) forever with no backoff. 24 hours +// caps the trigger at one dispatch a day until it is deleted and +// recreated, matching shared.findNextMatch's own give-up interval. +const catchUpFallbackInterval = 24 * time.Hour + +// catchUpReclaimAfter is how stale a claimed-but-never-armed trigger's +// updated_at must be before the reconciliation pass in TickScheduledTriggers +// treats it as abandoned rather than a live claim still mid-tick — enough +// headroom that a claim taken this tick or the previous one is never +// reconciled (AC-OFFICE-ROUTINE-CATCHUP-001.9). +const catchUpReclaimAfter = 60 * time.Second + +// TickScheduledTriggers queries due cron triggers, claims and dispatches +// each in ascending (next_run_at, id) order (AC-001.7), then reconciles +// any trigger left claimed-but-unarmed by a process that stopped mid-tick. func (s *RoutineService) TickScheduledTriggers(ctx context.Context, now time.Time) error { service.IncLoopCronTick(now.Format(time.RFC3339)) triggers, err := s.repo.GetDueTriggers(ctx, now) @@ -414,9 +441,37 @@ func (s *RoutineService) TickScheduledTriggers(ctx context.Context, now time.Tim zap.String("trigger_id", trigger.ID), zap.Error(err)) } } + s.reconcileStrandedTriggers(ctx, now) return nil } +// reconcileStrandedTriggers arms enabled cron triggers whose claim was +// abandoned before the re-arm write landed. It never dispatches a run by +// this path (AC-OFFICE-ROUTINE-CATCHUP-001.9): the row cannot distinguish +// "crashed after claiming" from "crashed after claiming and dispatching", +// and a spurious wake is the more expensive error. A parse failure leaves +// next_run_at null and only warns — no claim was taken by this path, so no +// run is owed and AC-001.11's fallback does not apply. +func (s *RoutineService) reconcileStrandedTriggers(ctx context.Context, now time.Time) { + stale, err := s.repo.ListStrandedTriggers(ctx, now.Add(-catchUpReclaimAfter)) + if err != nil { + s.logger.Warn("list stranded triggers", zap.Error(err)) + return + } + for _, trigger := range stale { + next, err := shared.NextCronTime(trigger.CronExpression, trigger.Timezone, now) + if err != nil { + s.logger.Warn("reconcile stranded trigger: compute next run", + zap.String("trigger_id", trigger.ID), zap.Error(err)) + continue + } + if _, err := s.repo.ReconcileTriggerNextRun(ctx, trigger.ID, next); err != nil { + s.logger.Warn("reconcile stranded trigger: arm", + zap.String("trigger_id", trigger.ID), zap.Error(err)) + } + } +} + func (s *RoutineService) processCronTrigger(ctx context.Context, trigger *RoutineTrigger, now time.Time) error { if trigger.NextRunAt == nil { return nil @@ -433,51 +488,41 @@ func (s *RoutineService) processCronTrigger(ctx context.Context, trigger *Routin return fmt.Errorf("get routine: %w", err) } service.IncLoopTriggerClaimed(routine.WorkspaceID) - // Catch-up cap: count how many cron ticks elapsed between the missed - // trigger.NextRunAt (inclusive) and now, capped at routine.CatchUpMax. - // Mirror of the agent_heartbeat catch-up math, adapted for cron - // expressions (no fixed interval — we walk NextCronTime). - runCount, advanceTo, err := computeRoutineMissed(trigger, routine, now) - if err != nil { - if errors.Is(err, shared.ErrUnsatisfiableCron) { + result := computeCatchUp(trigger, routine, now) + if result.Unknown { + if errors.Is(result.Err, shared.ErrUnsatisfiableCron) { // The expression can never fire again. ClaimTrigger already // cleared next_run_at; leave it cleared rather than re-arming, // which would dispatch nothing but retry (and fail) forever. s.logger.Error("cron expression unsatisfiable; trigger permanently disarmed", - zap.String("trigger_id", trigger.ID), zap.Error(err)) - return err + zap.String("trigger_id", trigger.ID), zap.Error(result.Err)) + return result.Err } - // Any other failure (e.g. the timezone database is temporarily - // unavailable) is presumed recoverable: re-arm to the original due - // time so the next tick retries instead of leaving the trigger - // disarmed forever once the underlying issue clears. - if rearmErr := s.repo.UpdateTriggerNextRun(ctx, trigger.ID, trigger.NextRunAt); rearmErr != nil { - s.logger.Warn("re-arm trigger after recoverable catch-up failure failed", - zap.String("trigger_id", trigger.ID), zap.Error(rearmErr)) - } - s.logger.Warn("compute routine catch-up failed; will retry next tick", - zap.String("trigger_id", trigger.ID), zap.Error(err)) - return err + // AC-OFFICE-ROUTINE-CATCHUP-001.11: any other computation failure + // (e.g. an unparseable expression/timezone, or the timezone + // database being temporarily unavailable) arms next_run_at to + // now+24h below rather than leaving the trigger disarmed forever + // or retrying every 30-second tick with no backoff. + s.logger.Warn("compute routine catch-up failed", + zap.String("trigger_id", trigger.ID), + zap.String("cron_expression", trigger.CronExpression), + zap.Error(result.Err)) } // The claimed tick: UpdateTriggerNextRun below advances the trigger // row's next_run_at to the next slot, so trigger.NextRunAt is captured // here, before that write, and carried through to the key builder // rather than re-read afterward. claimedTick := trigger.NextRunAt - if err := s.repo.UpdateTriggerNextRun(ctx, trigger.ID, &advanceTo); err != nil { + if err := s.repo.UpdateTriggerNextRun(ctx, trigger.ID, &result.NextRunAt); err != nil { + // AC-001.2: an arming-write failure means this claim dispatches + // nothing at all; next_run_at is left null for the reconciliation + // pass (AC-001.9) to arm on a later tick. That tick is lost. s.logger.Warn("update trigger next_run_at failed", zap.String("trigger_id", trigger.ID), zap.Error(err)) + return fmt.Errorf("arm trigger: %w", err) } - missedForPayload := 0 - if routine.CatchUpPolicy == models.CatchUpPolicySkipMissed { - // skip_missed: collapse all misses, fire once with no attribution. - missedForPayload = 0 - } else if runCount > 0 { - // enqueue_missed_with_cap (default and unknowns): fire once, surface - // runCount-1 as missed_ticks ("you missed N since the last fire"). - missedForPayload = runCount - 1 - } - _, err = s.DispatchRoutineRunWithMissed(ctx, routine, trigger, shared.RoutineSourceCron, nil, missedForPayload, claimedTick) + gap := buildGapSummary(routine, result) + run, err := s.dispatchRoutineRun(ctx, routine, trigger, shared.RoutineSourceCron, nil, "", gap, claimedTick) if errors.Is(err, shared.ErrWorkspacePaused) { // A confirmed operator pause is not a cron-tick failure — the // blocked fire is already recorded as a skipped run, and @@ -485,55 +530,114 @@ func (s *RoutineService) processCronTrigger(ctx context.Context, trigger *Routin // would otherwise page on-call for expected behaviour. return nil } + if err != nil && run != nil { + // The status flip has one home: whichever branch of dispatch + // failed, the run created for this claim is marked failed exactly + // once, here (AC-001.10). + if updateErr := s.repo.UpdateRunStatus(ctx, run.ID, models.RoutineRunStatusFailed, ""); updateErr != nil { + s.logger.Warn("mark routine run failed", + zap.String("run_id", run.ID), zap.Error(updateErr)) + } + } return err } -const defaultCatchUpMax = 25 +// 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. +type catchUpResult struct { + // ElapsedTicks is >= 1 on success (Unknown false); includes the tick + // due now. Not meaningful when Unknown is true. + ElapsedTicks int + // FirstMissedAt is the armed next_run_at at claim time, read directly + // rather than derived from the walk so it stays exact under + // truncation. Zero when ElapsedTicks <= 1. + FirstMissedAt time.Time + // Truncated is true only when the walk stopped at the cap with the + // cursor still at or before the processing instant. + Truncated bool + // NextRunAt is strictly after the processing instant on every path, + // including Unknown — it is never `now`. On Unknown it is the + // AC-001.11 fallback (now + catchUpFallbackInterval); the caller + // overrides it with a permanent disarm only for + // shared.ErrUnsatisfiableCron. + NextRunAt time.Time + // Unknown is true when the walk failed; ElapsedTicks, FirstMissedAt + // and Truncated are not meaningful in that case. + Unknown bool + // Err is the underlying cron computation error when Unknown is true. + // Nil otherwise. + Err error +} -// computeRoutineMissed counts cron ticks between trigger.NextRunAt -// (inclusive — that's the tick "due now") and `now`, capped at -// routine.CatchUpMax (default 25). Returns the count plus the next -// future tick the trigger should re-arm to. -// -// For an "every minute" cron with a 10-minute backend outage, this -// returns 11 (10 missed + 1 due now) and the next-minute tick. With a -// cap of 5, it returns 5 and aligns the cursor to the next future tick -// from `now` so the trigger leaves the catch-up window cleanly. -func computeRoutineMissed(trigger *RoutineTrigger, routine *Routine, now time.Time) (int, time.Time, error) { - cap := routine.CatchUpMax - if cap <= 0 { - cap = defaultCatchUpMax - } - cursor := *trigger.NextRunAt - runCount := 0 - for runCount < cap && !cursor.After(now) { - runCount++ +// computeCatchUp walks cron ticks between trigger.NextRunAt (inclusive — +// that's the tick "due now") and now, capped at +// models.NormaliseCatchUpMax(routine.CatchUpMax). It replaces +// computeRoutineMissed: catch_up_max bounds only how many elapsed ticks are +// counted, never how many runs are dispatched +// (AC-OFFICE-ROUTINE-CATCHUP-001.3). +func computeCatchUp(trigger *RoutineTrigger, routine *Routine, now time.Time) catchUpResult { + cap := models.NormaliseCatchUpMax(routine.CatchUpMax) + firstMissedAt := *trigger.NextRunAt + cursor := firstMissedAt + elapsed := 0 + for elapsed < cap && !cursor.After(now) { + elapsed++ next, err := shared.NextCronTime(trigger.CronExpression, trigger.Timezone, cursor) if err != nil { - return runCount, cursor, fmt.Errorf("next cron tick: %w", err) + return catchUpResult{NextRunAt: now.Add(catchUpFallbackInterval), Unknown: true, Err: err} } cursor = next } - if runCount == cap && !cursor.After(now) { - // Hit the cap with more pending ticks. Advance cursor to the next - // future tick from now to leave the catch-up window cleanly. + truncated := false + if elapsed == cap && !cursor.After(now) { + // Hit the cap with more pending ticks. Advance the cursor to the + // next future tick from now so the trigger leaves the catch-up + // window cleanly, and record the count as a lower bound. next, err := shared.NextCronTime(trigger.CronExpression, trigger.Timezone, now) if err != nil { - return runCount, cursor, fmt.Errorf("next cron tick from now: %w", err) + return catchUpResult{NextRunAt: now.Add(catchUpFallbackInterval), Unknown: true, Err: err} } cursor = next + truncated = true } - if runCount == 0 { - // Defensive: ClaimTrigger should only succeed when NextRunAt <= now, - // so the loop must have iterated at least once. If clock skew leaves - // us here, fire once and advance by one tick. - next, err := shared.NextCronTime(trigger.CronExpression, trigger.Timezone, *trigger.NextRunAt) - if err != nil { - return 1, *trigger.NextRunAt, fmt.Errorf("defensive next cron tick: %w", err) - } - return 1, next, nil + result := catchUpResult{ElapsedTicks: elapsed, NextRunAt: cursor, Truncated: truncated} + if elapsed > 1 { + result.FirstMissedAt = firstMissedAt } - return runCount, cursor, nil + return result +} + +// gapSummary is the durable measurement of one claim's gap, threaded from +// processCronTrigger through the dispatch chain into the created run's +// three catch_up_* columns and, for a lightweight routine under the +// summarizing policy, the wakeup payload. A nil *gapSummary means no gap +// summary applies for this claim. +type gapSummary struct { + MissedTicks int + FirstMissed time.Time + Truncated bool +} + +// buildGapSummary decides whether a claim's computed catch-up result +// produces a gap summary. A gap summary exists only when the walk +// succeeded, the policy is summarize_missed, and at least one tick was +// missed — collapsing what would otherwise be several near-identical states +// into one rule (AC-002.1, AC-002.2, AC-002.7, AC-002.11, AC-002.12: manual +// and webhook fires never reach this function, since only processCronTrigger +// calls it). +func buildGapSummary(routine *Routine, result catchUpResult) *gapSummary { + if result.Unknown { + return nil // AC-001.6: never report a gap that was not measured. + } + if routine.CatchUpPolicy == models.CatchUpPolicySkipMissed { + return nil // AC-002.7 + } + missed := result.ElapsedTicks - 1 + if missed < 1 { + return nil // AC-002.2 (zero missed); AC-002.11 (catch_up_max == 1). + } + return &gapSummary{MissedTicks: missed, FirstMissed: result.FirstMissedAt, Truncated: result.Truncated} } // DispatchRoutineRun resolves variables, applies concurrency policy, creates run. @@ -549,6 +653,10 @@ func computeRoutineMissed(trigger *RoutineTrigger, routine *Routine, now time.Ti // - heavy (task_template != "") — create a real task in the routine // system workflow (auto_start_agent on the start step kicks off the // agent). LinkedTaskID is the new task's id. +// +// Manual and webhook fires call this directly and never carry a gap +// summary (AC-OFFICE-ROUTINE-CATCHUP-002.12) — gap measurement applies only +// to a cron trigger's armed next_run_at, computed by processCronTrigger. func (s *RoutineService) DispatchRoutineRun( ctx context.Context, routine *Routine, @@ -556,7 +664,7 @@ func (s *RoutineService) DispatchRoutineRun( source string, provided map[string]string, ) (*RoutineRun, error) { - return s.dispatchRoutineRun(ctx, routine, trigger, source, provided, "", 0, nil) + return s.dispatchRoutineRun(ctx, routine, trigger, source, provided, "", nil, nil) } // DispatchRoutineRunWithIdempotencyKey dispatches a fire with an explicit @@ -570,34 +678,14 @@ func (s *RoutineService) DispatchRoutineRunWithIdempotencyKey( provided map[string]string, idempotencyKey string, ) (*RoutineRun, error) { - return s.dispatchRoutineRun(ctx, routine, trigger, source, provided, idempotencyKey, 0, nil) + return s.dispatchRoutineRun(ctx, routine, trigger, source, provided, idempotencyKey, nil, nil) } -// DispatchRoutineRunWithMissed is the cron-tick entry point that -// surfaces the catch-up cap's missed-tick count to the wakeup payload. -// missedTicks is set to N-1 by the cron tick when N consecutive cron -// ticks fell within the catch-up window — the agent gets one fire and -// learns "you missed N-1 ticks" via wakeup.RoutinePayload.MissedTicks. -// Manual fires (UI / API) call DispatchRoutineRun directly with no -// missed-tick attribution. -// // claimedTick is the scheduled tick processCronTrigger claimed off the // trigger row before advancing it — the dedup key's occurrence identity // for a cron fire. It travels as a parameter rather than being re-read // from the trigger row, which by dispatch time already names the next -// slot. -func (s *RoutineService) DispatchRoutineRunWithMissed( - ctx context.Context, - routine *Routine, - trigger *RoutineTrigger, - source string, - provided map[string]string, - missedTicks int, - claimedTick *time.Time, -) (*RoutineRun, error) { - return s.dispatchRoutineRun(ctx, routine, trigger, source, provided, "", missedTicks, claimedTick) -} - +// slot. nil for manual/webhook fires, which have no claimed slot. func (s *RoutineService) dispatchRoutineRun( ctx context.Context, routine *Routine, @@ -605,7 +693,7 @@ func (s *RoutineService) dispatchRoutineRun( source string, provided map[string]string, idempotencyKey string, - missedTicks int, + gap *gapSummary, claimedTick *time.Time, ) (*RoutineRun, error) { now := time.Now().UTC() @@ -639,8 +727,18 @@ func (s *RoutineService) dispatchRoutineRun( // and copied onto every wakeup request and run this fire produces. CausationID: uuid.New().String(), } + if gap != nil { + missed := gap.MissedTicks + firstMissed := gap.FirstMissed + run.CatchUpMissedTicks = &missed + run.CatchUpFirstMissedAt = &firstMissed + run.CatchUpTruncated = gap.Truncated + } + // The gap summary is written once, here, before applyConcurrencyPolicy + // runs — a run later marked skipped, coalesced or failed still carries + // the gap measured for its tick (AC-002.10). if err := s.repo.CreateRoutineRun(ctx, run); err != nil { - return nil, fmt.Errorf("create run: %w", err) + return nil, fmt.Errorf("create run: %w", err) // AC-001.12: no run row exists. } // AC-003.1/AC-003.9 (Review round 3, R3-4): office_loop_routine_run_total @@ -678,7 +776,7 @@ func (s *RoutineService) dispatchRoutineRun( return run, nil } - if err := s.materialiseRoutineRun(ctx, routine, run, tmpl, title, description, vars, source, idempotencyKey, missedTicks, claimedTick); err != nil { + if err := s.materialiseRoutineRun(ctx, routine, run, tmpl, title, description, vars, source, idempotencyKey, gap, claimedTick); err != nil { return run, err } // A nil error only means materialiseRoutineRun's terminal write @@ -708,7 +806,7 @@ func (s *RoutineService) dispatchRoutineRun( // pause reason in the response body" without a second PauseState read — // which would race a resume between the block decision and the response. // Every existing errors.Is(err, shared.ErrWorkspacePaused) call site -// (processCronTrigger below, writeDispatchError, scheduler/wakeup) keeps +// (processCronTrigger above, writeDispatchError, scheduler/wakeup) keeps // matching unchanged, since Unwrap chains to the shared sentinel. type pausedDispatchError struct { workspaceID string @@ -763,8 +861,10 @@ func (s *RoutineService) checkPauseGate(ctx context.Context, workspaceID, routin // (see SyncRunStatus). The lightweight path has no task to wait on, so // it resolves to a terminal status (done/failed) immediately; see // materialiseLightweightRoutineRun. -// missedTicks is forwarded to the lightweight wakeup payload; the heavy -// path doesn't attribute it (the agent reads context from the task). +// gap is forwarded to the lightweight wakeup payload; the heavy path +// doesn't render it — mutating a user's template text with scheduler +// metadata would be a worse contract than leaving the gap on the routine +// run where the API exposes it (AC-002.6). func (s *RoutineService) materialiseRoutineRun( ctx context.Context, routine *Routine, @@ -774,13 +874,13 @@ func (s *RoutineService) materialiseRoutineRun( vars map[string]string, source string, idempotencyKey string, - missedTicks int, + gap *gapSummary, claimedTick *time.Time, ) error { if tmpl.Title != "" && s.workflowEnsurer != nil && s.taskCreator != nil { return s.materialiseHeavyRoutineRun(ctx, routine, run, title, description) } - return s.materialiseLightweightRoutineRun(ctx, routine, run, vars, source, idempotencyKey, missedTicks, claimedTick) + return s.materialiseLightweightRoutineRun(ctx, routine, run, vars, source, idempotencyKey, gap, claimedTick) } // materialiseHeavyRoutineRun creates a real task in the routine system @@ -828,9 +928,11 @@ func (s *RoutineService) materialiseHeavyRoutineRun( // wakeup queue because no background poller retries direct dispatch. // LinkedTaskID stays empty for lightweight runs. // -// missedTicks > 0 surfaces in the wakeup payload when the cron tick -// collapsed N missed fires into one (catch-up cap policy -// enqueue_missed_with_cap). Zero on the happy path. +// gap, when non-nil, surfaces in the wakeup payload as the gap this claim +// measured (catch-up policy summarize_missed). CreateWakeupRequest and +// Dispatch errors are returned rather than absorbed — the only exception is +// the idempotency-key-conflict sentinel, which is a successful dedup, not a +// failed dispatch, and stays a no-op (AC-OFFICE-ROUTINE-CATCHUP-001.10). func (s *RoutineService) materialiseLightweightRoutineRun( ctx context.Context, routine *Routine, @@ -838,14 +940,14 @@ func (s *RoutineService) materialiseLightweightRoutineRun( vars map[string]string, source string, idempotencyKey string, - missedTicks int, + gap *gapSummary, claimedTick *time.Time, ) error { if s.wakeup == nil || routine.AssigneeAgentProfileID == "" { return s.finalizeLightweightRun(ctx, run, models.RoutineRunStatusDone) } idemKey := buildRoutineIdempotencyKey(source, routine.ID, run.TriggerID, idempotencyKey, claimedTick, run.ID) - payloadStr, _ := marshalRoutinePayload(routine.ID, vars, missedTicks) + payloadStr, _ := marshalRoutinePayload(routine.ID, vars, gap) req := &WakeupRequest{ ID: uuid.New().String(), AgentProfileID: routine.AssigneeAgentProfileID, @@ -934,17 +1036,24 @@ func buildRoutineIdempotencyKey( // marshalRoutinePayload renders the wakeup-request payload for a // routine fire as JSON. The shape mirrors wakeup.RoutinePayload: -// {routine_id, variables, missed_ticks}. missed_ticks is set by the -// cron tick when the catch-up cap collapsed N missed fires into one. -func marshalRoutinePayload(routineID string, vars map[string]string, missedTicks int) (string, error) { +// {routine_id, variables, missed_ticks, missed_since, missed_truncated}. +// The three catch-up fields are set together, from gap, only when the +// claim measured at least one missed tick under the summarize_missed +// policy — they state the ticks counted and reported, never the ticks +// fired (only one run is ever dispatched per claim). +func marshalRoutinePayload(routineID string, vars map[string]string, gap *gapSummary) (string, error) { body := map[string]any{ "routine_id": routineID, } if len(vars) > 0 { body["variables"] = vars } - if missedTicks > 0 { - body["missed_ticks"] = missedTicks + if gap != nil { + body["missed_ticks"] = gap.MissedTicks + body["missed_since"] = gap.FirstMissed.UTC().Format(time.RFC3339) + if gap.Truncated { + body["missed_truncated"] = true + } } b, err := json.Marshal(body) if err != nil { diff --git a/apps/backend/internal/office/routines/service_catchup_test.go b/apps/backend/internal/office/routines/service_catchup_test.go new file mode 100644 index 0000000000..cd6bceed78 --- /dev/null +++ b/apps/backend/internal/office/routines/service_catchup_test.go @@ -0,0 +1,644 @@ +package routines_test + +import ( + "context" + "errors" + "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/wakeup" +) + +// wrapRepo wraps a real routines.Repository (the sqlite implementation) and +// lets individual tests force specific methods to fail, to exercise error +// paths that are otherwise unreachable through the public service API +// against a healthy in-memory database. +type wrapRepo struct { + routines.Repository + failUpdateTriggerNextRun bool + failCreateRoutineRun bool +} + +func (w *wrapRepo) UpdateTriggerNextRun(ctx context.Context, triggerID string, nextRunAt *time.Time) error { + if w.failUpdateTriggerNextRun { + return errors.New("simulated arm-write failure") + } + return w.Repository.UpdateTriggerNextRun(ctx, triggerID, nextRunAt) +} + +func (w *wrapRepo) CreateRoutineRun(ctx context.Context, run *models.RoutineRun) error { + if w.failCreateRoutineRun { + return errors.New("simulated create-run failure") + } + return w.Repository.CreateRoutineRun(ctx, run) +} + +// newWrappedTestRoutineService is newTestRoutineService plus access to the +// wrapRepo (to flip failure switches) and the raw *sqlx.DB (to backdate +// rows directly, as no service method can). +func newWrappedTestRoutineService(t *testing.T) (*routines.RoutineService, *wrapRepo, *sqlx.DB) { + 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) + } + wrapped := &wrapRepo{Repository: repo} + return routines.NewRoutineService(wrapped, logger.Default(), &noopActivity{}), wrapped, db +} + +func mustCreateCronRoutineAndTrigger(t *testing.T, svc *routines.RoutineService, taskTemplate string) *models.Routine { + t.Helper() + ctx := context.Background() + routine := &models.Routine{ + WorkspaceID: "ws-1", + Name: "Cron routine", + TaskTemplate: taskTemplate, + AssigneeAgentProfileID: "agent-1", + Status: "active", + ConcurrencyPolicy: models.ConcurrencyPolicyAlwaysCreate, + } + 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) + } + return routine +} + +// TestProcessCronTrigger_ArmFailureAbortsDispatch covers AC-OFFICE-ROUTINE-CATCHUP-001.2: +// when the re-arm write itself fails, the tick dispatches nothing for that +// claim and no routine run is created. +func TestProcessCronTrigger_ArmFailureAbortsDispatch(t *testing.T) { + svc, wrapped, _ := newWrappedTestRoutineService(t) + ctx := context.Background() + routine := mustCreateCronRoutineAndTrigger(t, svc, "") + + wrapped.failUpdateTriggerNextRun = true + if err := svc.TickScheduledTriggers(ctx, time.Now().UTC().Add(2*time.Minute)); err != nil { + t.Fatalf("tick: %v", err) + } + + runs, err := svc.ListRoutineRuns(ctx, routine.ID, 10, 0) + if err != nil { + t.Fatalf("list runs: %v", err) + } + if len(runs) != 0 { + t.Fatalf("routine runs = %d, want 0 (arm failure must abort dispatch entirely)", len(runs)) + } +} + +// TestDispatchRoutineRun_CreateRunFailureRecordsNothing covers AC-OFFICE-ROUTINE-CATCHUP-001.12: +// when the run row itself cannot be created, dispatch returns an error and +// no run row exists — there is nothing to mark failed. +func TestDispatchRoutineRun_CreateRunFailureRecordsNothing(t *testing.T) { + svc, wrapped, _ := newWrappedTestRoutineService(t) + ctx := context.Background() + routine := mustCreateCronRoutineAndTrigger(t, svc, "") + + wrapped.failCreateRoutineRun = true + if _, err := svc.FireManual(ctx, routine.ID, nil); err == nil { + t.Fatal("expected error from FireManual when CreateRoutineRun fails") + } + + wrapped.failCreateRoutineRun = false + runs, err := svc.ListRoutineRuns(ctx, routine.ID, 10, 0) + if err != nil { + t.Fatalf("list runs: %v", err) + } + if len(runs) != 0 { + t.Fatalf("routine runs = %d, want 0", len(runs)) + } +} + +// TestProcessCronTrigger_LightweightMaterialiseFailureMarksRunFailed covers +// AC-OFFICE-ROUTINE-CATCHUP-001.10: a lightweight routine's +// CreateWakeupRequest failure (not an idempotency conflict) marks the +// claim's run failed. +func TestProcessCronTrigger_LightweightMaterialiseFailureMarksRunFailed(t *testing.T) { + svc := newTestRoutineService(t) + ctx := context.Background() + routine := newLightweightTestRoutine(t, svc) + if err := svc.CreateRoutineTrigger(ctx, &models.RoutineTrigger{ + RoutineID: routine.ID, Kind: "cron", CronExpression: "* * * * *", Timezone: "UTC", Enabled: true, + }); err != nil { + t.Fatalf("create trigger: %v", err) + } + + svc.SetWakeupEnqueuer(&failingCreateWakeupEnqueuer{createErr: errors.New("simulated dispatcher failure")}) + + if err := svc.TickScheduledTriggers(ctx, time.Now().UTC().Add(2*time.Minute)); err != nil { + t.Fatalf("tick: %v", err) + } + + runs, err := svc.ListRoutineRuns(ctx, routine.ID, 10, 0) + if err != nil { + t.Fatalf("list runs: %v", err) + } + if len(runs) != 1 { + t.Fatalf("routine runs = %d, want 1", len(runs)) + } + if runs[0].Status != models.RoutineRunStatusFailed { + t.Errorf("status = %q, want failed", runs[0].Status) + } +} + +// TestProcessCronTrigger_IdempotencyConflictDoesNotMarkRunFailed covers the +// second sentence of AC-OFFICE-ROUTINE-CATCHUP-001.10: a wakeup request +// refused as an idempotency-key duplicate is a successful dedup, not a +// materialisation failure, and must not flip the run to failed. +func TestProcessCronTrigger_IdempotencyConflictDoesNotMarkRunFailed(t *testing.T) { + svc := newTestRoutineService(t) + ctx := context.Background() + routine := newLightweightTestRoutine(t, svc) + if err := svc.CreateRoutineTrigger(ctx, &models.RoutineTrigger{ + RoutineID: routine.ID, Kind: "cron", CronExpression: "* * * * *", Timezone: "UTC", Enabled: true, + }); err != nil { + t.Fatalf("create trigger: %v", err) + } + + svc.SetWakeupEnqueuer(&failingCreateWakeupEnqueuer{createErr: routines.ErrWakeupAlreadyRequested}) + + if err := svc.TickScheduledTriggers(ctx, time.Now().UTC().Add(2*time.Minute)); err != nil { + t.Fatalf("tick: %v", err) + } + + runs, err := svc.ListRoutineRuns(ctx, routine.ID, 10, 0) + if err != nil { + t.Fatalf("list runs: %v", err) + } + if len(runs) != 1 { + t.Fatalf("routine runs = %d, want 1", len(runs)) + } + // The concurrency-policy outcome (task_created, since nothing else was + // active) must survive — an idempotency dedup is not a failure. + if runs[0].Status == models.RoutineRunStatusFailed { + t.Error("status = failed, want the concurrency-policy outcome preserved (idempotency dedup is not a failure)") + } +} + +// TestProcessCronTrigger_HeavyMaterialiseFailureMarksRunFailed covers the +// heavy-path half of AC-OFFICE-ROUTINE-CATCHUP-001.10: a task-creation +// failure marks the run failed instead of leaving it stuck at "received". +func TestProcessCronTrigger_HeavyMaterialiseFailureMarksRunFailed(t *testing.T) { + svc := newTestRoutineService(t) + ctx := context.Background() + routine := mustCreateCronRoutineAndTrigger(t, svc, `{"title":"T","description":"D"}`) + + svc.SetWorkflowEnsurer(&fakeWorkflowEnsurer{}) + svc.SetTaskCreator(&failingTaskCreator{}) + + if err := svc.TickScheduledTriggers(ctx, time.Now().UTC().Add(2*time.Minute)); err != nil { + t.Fatalf("tick: %v", err) + } + + runs, err := svc.ListRoutineRuns(ctx, routine.ID, 10, 0) + if err != nil { + t.Fatalf("list runs: %v", err) + } + if len(runs) != 1 { + t.Fatalf("routine runs = %d, want 1", len(runs)) + } + if runs[0].Status != models.RoutineRunStatusFailed { + t.Errorf("status = %q, want failed (was stuck at received before this change)", runs[0].Status) + } +} + +type failingTaskCreator struct{} + +func (f *failingTaskCreator) CreateOfficeTaskInWorkflow( + _ context.Context, _, _, _, _, _, _ string, +) (string, error) { + return "", errors.New("simulated task creation failure") +} + +// TestCreateRoutineTrigger_RejectsEmptyCronExpression covers the first half +// of AC-OFFICE-ROUTINE-CATCHUP-001.8: a cron trigger must never be +// persisted with a null next_run_at, so an empty expression is rejected +// outright rather than silently created unarmed. +func TestCreateRoutineTrigger_RejectsEmptyCronExpression(t *testing.T) { + svc := newTestRoutineService(t) + ctx := context.Background() + routine := createTestRoutine(t, svc, "AC-001.8 empty", "always_create") + + err := svc.CreateRoutineTrigger(ctx, &models.RoutineTrigger{ + RoutineID: routine.ID, Kind: "cron", CronExpression: "", Timezone: "UTC", Enabled: true, + }) + if err == nil { + t.Fatal("expected an error creating a cron trigger with an empty expression") + } + + triggers, listErr := svc.ListRoutineTriggers(ctx, routine.ID) + if listErr != nil { + t.Fatalf("list triggers: %v", listErr) + } + if len(triggers) != 0 { + t.Fatalf("triggers = %d, want 0 (rejected trigger must not be persisted)", len(triggers)) + } +} + +// TestCreateRoutineTrigger_RejectsUnparseableCronExpression covers the +// second half of AC-OFFICE-ROUTINE-CATCHUP-001.8: an expression that +// shared.NextCronTime cannot parse is rejected at creation time rather than +// persisted with a null next_run_at. +func TestCreateRoutineTrigger_RejectsUnparseableCronExpression(t *testing.T) { + svc := newTestRoutineService(t) + ctx := context.Background() + routine := createTestRoutine(t, svc, "AC-001.8 unparseable", "always_create") + + err := svc.CreateRoutineTrigger(ctx, &models.RoutineTrigger{ + RoutineID: routine.ID, Kind: "cron", CronExpression: "not a cron expression", Timezone: "UTC", Enabled: true, + }) + if err == nil { + t.Fatal("expected an error creating a cron trigger with an unparseable expression") + } + + triggers, listErr := svc.ListRoutineTriggers(ctx, routine.ID) + if listErr != nil { + t.Fatalf("list triggers: %v", listErr) + } + if len(triggers) != 0 { + t.Fatalf("triggers = %d, want 0 (rejected trigger must not be persisted)", len(triggers)) + } +} + +// TestTickScheduledTriggers_ReconciliationArmsStrandedTriggerWithoutDispatch +// covers AC-OFFICE-ROUTINE-CATCHUP-001.9 at the service level: a trigger +// claimed (next_run_at set to NULL) by a process that crashed before the +// re-arm write landed gets armed by the next tick's reconciliation pass, +// without dispatching a spurious run for the abandoned claim. +func TestTickScheduledTriggers_ReconciliationArmsStrandedTriggerWithoutDispatch(t *testing.T) { + svc, wrapped, db := newWrappedTestRoutineService(t) + ctx := context.Background() + routine := mustCreateCronRoutineAndTrigger(t, svc, "") + + triggers, err := svc.ListRoutineTriggers(ctx, routine.ID) + if err != nil || len(triggers) != 1 { + t.Fatalf("list triggers: %v (len=%d)", err, len(triggers)) + } + triggerID := triggers[0].ID + + // Simulate a claim that landed (next_run_at -> NULL) but whose re-arm + // write never happened, aged well past catchUpReclaimAfter. + staleUpdatedAt := time.Now().UTC().Add(-10 * time.Minute) + if _, err := wrapped.ClaimTrigger(ctx, triggerID, *triggers[0].NextRunAt); err != nil { + t.Fatalf("claim trigger: %v", err) + } + if _, err := db.ExecContext(ctx, + "UPDATE office_routine_triggers SET updated_at = ? WHERE id = ?", staleUpdatedAt, triggerID, + ); err != nil { + t.Fatalf("backdate updated_at: %v", err) + } + + if err := svc.TickScheduledTriggers(ctx, time.Now().UTC()); err != nil { + t.Fatalf("tick: %v", err) + } + + runs, err := svc.ListRoutineRuns(ctx, routine.ID, 10, 0) + if err != nil { + t.Fatalf("list runs: %v", err) + } + if len(runs) != 0 { + t.Fatalf("routine runs = %d, want 0 (reconciliation must never dispatch)", len(runs)) + } + + reArmed, err := svc.ListRoutineTriggers(ctx, routine.ID) + if err != nil { + t.Fatalf("list triggers: %v", err) + } + if reArmed[0].NextRunAt == nil { + t.Fatal("NextRunAt still nil after reconciliation, want armed") + } + if !reArmed[0].NextRunAt.After(time.Now().UTC().Add(-time.Minute)) { + t.Errorf("NextRunAt = %v, want a time close to/after now (re-armed from `now`, not from the stale claim)", *reArmed[0].NextRunAt) + } +} + +// TestTickScheduledTriggers_MalformedExpressionRearmsWithDispatch verifies +// that a legacy trigger with a malformed expression dispatches exactly one +// run and is re-armed to the processing instant plus 24 hours +// (AC-OFFICE-ROUTINE-CATCHUP-001.11), not left un-dispatched or re-armed to +// its original due time. New trigger creation rejects this input before it +// is persisted; this covers a row created before that validation existed. +func TestTickScheduledTriggers_MalformedExpressionRearmsWithDispatch(t *testing.T) { + svc, wrapped, _ := newWrappedTestRoutineService(t) + ctx := context.Background() + + routine := &models.Routine{ + WorkspaceID: "ws-1", Name: "Bad cron", AssigneeAgentProfileID: "agent-1", + Status: "active", ConcurrencyPolicy: models.ConcurrencyPolicyAlwaysCreate, + } + if err := svc.CreateRoutine(ctx, routine); err != nil { + t.Fatalf("create routine: %v", err) + } + // Bypass CreateRoutineTrigger's own AC-001.8 rejection: seed a + // malformed-but-armed trigger directly through the repo, as a legacy + // row (created before AC-001.8) would look. Arm it in the past so it's + // due. + past := time.Now().UTC().Add(-time.Hour) + if err := wrapped.CreateRoutineTrigger(ctx, &models.RoutineTrigger{ + RoutineID: routine.ID, Kind: "cron", CronExpression: "garbage", Timezone: "UTC", + Enabled: true, NextRunAt: &past, + }); err != nil { + t.Fatalf("seed malformed trigger: %v", err) + } + + before := time.Now().UTC() + if err := svc.TickScheduledTriggers(ctx, before); err != nil { + t.Fatalf("tick: %v", err) + } + after := time.Now().UTC() + runs, err := svc.ListRoutineRuns(ctx, routine.ID, 10, 0) + if err != nil { + t.Fatalf("list runs: %v", err) + } + if len(runs) != 1 { + t.Fatalf("routine runs after malformed trigger = %d, want 1", len(runs)) + } + triggers, err := svc.ListRoutineTriggers(ctx, routine.ID) + if err != nil { + t.Fatalf("list triggers: %v", err) + } + if len(triggers) != 1 || triggers[0].NextRunAt == nil { + t.Fatalf("triggers = %+v, want one re-armed trigger", triggers) + } + got := *triggers[0].NextRunAt + if got.Before(before.Add(24*time.Hour)) || got.After(after.Add(24*time.Hour)) { + t.Fatalf("next_run_at = %v, want within [%v, %v] (processing instant + 24h)", + got, before.Add(24*time.Hour), after.Add(24*time.Hour)) + } +} + +// TestTickScheduledTriggers_MalformedExpressionLogsUnderlyingError covers +// AC-OFFICE-ROUTINE-CATCHUP-001.11: the warning emitted when the elapsed-tick +// computation fails must name the underlying cron-parse error, not just the +// trigger ID and expression. +func TestTickScheduledTriggers_MalformedExpressionLogsUnderlyingError(t *testing.T) { + core, logs := observer.New(zapcore.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("create observer logger: %v", err) + } + + 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) + } + svc := routines.NewRoutineService(repo, log, &noopActivity{}) + + ctx := context.Background() + routine := &models.Routine{ + WorkspaceID: "ws-1", Name: "Bad cron", AssigneeAgentProfileID: "agent-1", + Status: "active", ConcurrencyPolicy: models.ConcurrencyPolicyAlwaysCreate, + } + if err := svc.CreateRoutine(ctx, routine); err != nil { + t.Fatalf("create routine: %v", err) + } + past := time.Now().UTC().Add(-time.Hour) + if err := repo.CreateRoutineTrigger(ctx, &models.RoutineTrigger{ + RoutineID: routine.ID, Kind: "cron", CronExpression: "garbage", Timezone: "UTC", + Enabled: true, NextRunAt: &past, + }); err != nil { + t.Fatalf("seed malformed trigger: %v", err) + } + + if err := svc.TickScheduledTriggers(ctx, time.Now().UTC()); err != nil { + t.Fatalf("tick: %v", err) + } + + entries := logs.FilterMessage("compute routine catch-up failed").All() + if len(entries) == 0 { + t.Fatal("expected a 'compute routine catch-up failed' warning") + } + if errVal, ok := entries[0].ContextMap()["error"]; !ok || errVal == "" { + t.Fatalf("warning did not name the underlying error, context = %#v", entries[0].ContextMap()) + } +} + +// TestTickScheduledTriggers_GapExceedingCapProducesExactlyOneRun covers +// AC-OFFICE-ROUTINE-CATCHUP-001.1 and 001.3 together at the service level: +// a gap spanning far more ticks than catch_up_max produces exactly one +// run and one wakeup dispatch on resume — catch_up_max bounds only the +// counted tick total (here capped at 3), never the number of runs created. +func TestTickScheduledTriggers_GapExceedingCapProducesExactlyOneRun(t *testing.T) { + svc, _, db := newWrappedTestRoutineService(t) + ctx := context.Background() + + routine := &models.Routine{ + WorkspaceID: "ws-1", Name: "Gapped", AssigneeAgentProfileID: "agent-1", + Status: "active", ConcurrencyPolicy: models.ConcurrencyPolicyAlwaysCreate, + CatchUpPolicy: models.CatchUpPolicySummarizeMissed, CatchUpMax: 3, + } + 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) + } + // The backend was "down" far longer than catch_up_max ticks can count. + longDown := time.Now().UTC().Add(-100 * time.Minute) + triggers, err := svc.ListRoutineTriggers(ctx, routine.ID) + if err != nil || len(triggers) != 1 { + t.Fatalf("list triggers: %v (len=%d)", err, len(triggers)) + } + if _, err := db.ExecContext(ctx, + "UPDATE office_routine_triggers SET next_run_at = ? WHERE id = ?", longDown, triggers[0].ID, + ); err != nil { + t.Fatalf("backdate next_run_at: %v", err) + } + + enq := &fakeWakeupEnqueuer{} + svc.SetWakeupEnqueuer(enq) + + if err := svc.TickScheduledTriggers(ctx, time.Now().UTC()); err != nil { + t.Fatalf("tick: %v", err) + } + + runs, err := svc.ListRoutineRuns(ctx, routine.ID, 10, 0) + if err != nil { + t.Fatalf("list runs: %v", err) + } + if len(runs) != 1 { + t.Fatalf("routine runs = %d, want exactly 1 regardless of gap size", len(runs)) + } + if len(enq.dispatched) != 1 { + t.Fatalf("wakeup dispatches = %d, want exactly 1", len(enq.dispatched)) + } + run := runs[0] + if run.CatchUpMissedTicks == nil { + t.Fatal("CatchUpMissedTicks = nil, want set") + } + if *run.CatchUpMissedTicks != routine.CatchUpMax-1 { + t.Errorf("CatchUpMissedTicks = %d, want %d (catch_up_max - 1)", *run.CatchUpMissedTicks, routine.CatchUpMax-1) + } + if !run.CatchUpTruncated { + t.Error("CatchUpTruncated = false, want true") + } + if run.CatchUpFirstMissedAt == nil || !run.CatchUpFirstMissedAt.Equal(longDown) { + t.Errorf("CatchUpFirstMissedAt = %v, want %v", run.CatchUpFirstMissedAt, longDown) + } + + // AC-OFFICE-ROUTINE-CATCHUP-002.5: the gap must also be readable from + // the actual wakeup-request payload dispatched to the agent, not only + // from the run's DB columns. + if len(enq.created) != 1 { + t.Fatalf("wakeup requests created = %d, want exactly 1", len(enq.created)) + } + var payload wakeup.RoutinePayload + if err := wakeup.UnmarshalPayload(enq.created[0].Payload, &payload); err != nil { + t.Fatalf("unmarshal wakeup payload: %v", err) + } + if payload.MissedTicks != routine.CatchUpMax-1 { + t.Errorf("payload MissedTicks = %d, want %d", payload.MissedTicks, routine.CatchUpMax-1) + } + if !payload.MissedTruncated { + t.Error("payload MissedTruncated = false, want true") + } + if payload.MissedSince != longDown.UTC().Format(time.RFC3339) { + t.Errorf("payload MissedSince = %q, want %q", payload.MissedSince, longDown.UTC().Format(time.RFC3339)) + } +} + +// TestTickScheduledTriggers_HeavyRoutineGapLeavesTaskUnmodified covers +// AC-OFFICE-ROUTINE-CATCHUP-002.6: a heavy routine dispatched across a +// measured gap must render its task title/description exactly as the +// template does, with the gap readable only through the routine run, not +// injected into the task the agent sees. +func TestTickScheduledTriggers_HeavyRoutineGapLeavesTaskUnmodified(t *testing.T) { + svc, _, db := newWrappedTestRoutineService(t) + ctx := context.Background() + + routine := &models.Routine{ + WorkspaceID: "ws-1", Name: "Heavy gapped", AssigneeAgentProfileID: "agent-1", + TaskTemplate: `{"title":"Daily review","description":"Fixed description"}`, + Status: "active", + ConcurrencyPolicy: models.ConcurrencyPolicyAlwaysCreate, + CatchUpPolicy: models.CatchUpPolicySummarizeMissed, CatchUpMax: 25, + } + 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) + } + longDown := time.Now().UTC().Add(-10 * time.Minute) + triggers, err := svc.ListRoutineTriggers(ctx, routine.ID) + if err != nil || len(triggers) != 1 { + t.Fatalf("list triggers: %v (len=%d)", err, len(triggers)) + } + if _, err := db.ExecContext(ctx, + "UPDATE office_routine_triggers SET next_run_at = ? WHERE id = ?", longDown, triggers[0].ID, + ); err != nil { + t.Fatalf("backdate next_run_at: %v", err) + } + + wf := &fakeWorkflowEnsurer{} + tc := &fakeTaskCreator{} + svc.SetWorkflowEnsurer(wf) + svc.SetTaskCreator(tc) + + if err := svc.TickScheduledTriggers(ctx, time.Now().UTC()); err != nil { + t.Fatalf("tick: %v", err) + } + + if tc.captured.title != "Daily review" { + t.Errorf("task title = %q, want exactly %q (unmodified by the gap)", tc.captured.title, "Daily review") + } + if tc.captured.description != "Fixed description" { + t.Errorf("task description = %q, want exactly %q (unmodified by the gap)", + tc.captured.description, "Fixed description") + } + + runs, err := svc.ListRoutineRuns(ctx, routine.ID, 10, 0) + if err != nil { + t.Fatalf("list runs: %v", err) + } + if len(runs) != 1 { + t.Fatalf("routine runs = %d, want 1", len(runs)) + } + run := runs[0] + if run.CatchUpMissedTicks == nil || *run.CatchUpMissedTicks < 1 { + t.Fatalf("CatchUpMissedTicks = %v, want set and >= 1 (gap summary readable via the run)", run.CatchUpMissedTicks) + } + if run.CatchUpFirstMissedAt == nil || !run.CatchUpFirstMissedAt.Equal(longDown) { + t.Errorf("CatchUpFirstMissedAt = %v, want %v", run.CatchUpFirstMissedAt, longDown) + } + if run.LinkedTaskID != "task-routine-1" { + t.Errorf("LinkedTaskID = %q, want task-routine-1 (heavy dispatch happened)", run.LinkedTaskID) + } +} + +// TestFireManual_RecordsNoGapSummaryDespitePendingTriggerGap covers +// AC-OFFICE-ROUTINE-CATCHUP-002.12: gap measurement applies only to a cron +// trigger's own armed claim. A manual fire on a routine whose cron trigger +// has a large backdated next_run_at (i.e. would produce a gap on its own +// tick) must still record no gap summary at all. +func TestFireManual_RecordsNoGapSummaryDespitePendingTriggerGap(t *testing.T) { + svc, _, db := newWrappedTestRoutineService(t) + ctx := context.Background() + + routine := &models.Routine{ + WorkspaceID: "ws-1", Name: "Manual despite gap", AssigneeAgentProfileID: "agent-1", + Status: "active", ConcurrencyPolicy: models.ConcurrencyPolicyAlwaysCreate, + CatchUpPolicy: models.CatchUpPolicySummarizeMissed, CatchUpMax: 25, + } + 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: %v (len=%d)", err, len(triggers)) + } + longDown := time.Now().UTC().Add(-100 * time.Minute) + if _, err := db.ExecContext(ctx, + "UPDATE office_routine_triggers SET next_run_at = ? WHERE id = ?", longDown, triggers[0].ID, + ); err != nil { + t.Fatalf("backdate next_run_at: %v", err) + } + + run, err := svc.FireManual(ctx, routine.ID, nil) + if err != nil { + t.Fatalf("fire manual: %v", err) + } + if run.CatchUpMissedTicks != nil { + t.Errorf("CatchUpMissedTicks = %v, want nil (manual fire records no gap)", *run.CatchUpMissedTicks) + } + if run.CatchUpFirstMissedAt != nil { + t.Errorf("CatchUpFirstMissedAt = %v, want nil", *run.CatchUpFirstMissedAt) + } + if run.CatchUpTruncated { + t.Error("CatchUpTruncated = true, want false") + } +} diff --git a/apps/backend/internal/office/routines/service_cron_advance_failure_test.go b/apps/backend/internal/office/routines/service_cron_advance_failure_test.go index b7e0a69439..ab8e9c262a 100644 --- a/apps/backend/internal/office/routines/service_cron_advance_failure_test.go +++ b/apps/backend/internal/office/routines/service_cron_advance_failure_test.go @@ -98,9 +98,14 @@ func TestTickScheduledTriggers_UnsatisfiableExpression_DisarmsInsteadOfLooping(t // TestTickScheduledTriggers_RecoverableCatchUpFailure_RearmsForRetry is a // regression test for a catch-up failure that is NOT expression // unsatisfiability (e.g. the timezone database is transiently unavailable). -// Unlike the unsatisfiable case, this must re-arm next_run_at to the -// original due time so the next tick retries once the underlying issue -// clears, instead of leaving the trigger permanently disarmed. +// Unlike the unsatisfiable case (AC-OFFICE-ROUTINE-CATCHUP-001.11 governs +// only "elapsed-tick computation fails", never a syntactically-impossible +// expression), this arms next_run_at to the processing instant plus 24 +// hours and still dispatches exactly one run for the claim — REQ-001's +// "one resume, one wake" is unconditional, and AC-001.11 says as much +// ("shall therefore dispatch at most one run per day until it is deleted +// and recreated") — rather than leaving the trigger permanently disarmed +// or silently skipping the claim it already took. func TestTickScheduledTriggers_RecoverableCatchUpFailure_RearmsForRetry(t *testing.T) { db, err := sqlx.Open("sqlite3", ":memory:") if err != nil { @@ -146,11 +151,13 @@ func TestTickScheduledTriggers_RecoverableCatchUpFailure_RearmsForRetry(t *testi enq := &fakeWakeupEnqueuer{} svc.SetWakeupEnqueuer(enq) - if err := svc.TickScheduledTriggers(ctx, time.Now().UTC()); err != nil { + before := time.Now().UTC() + if err := svc.TickScheduledTriggers(ctx, before); err != nil { t.Fatalf("tick scheduled triggers: %v", err) } - if len(enq.created) != 0 { - t.Fatalf("expected no dispatch on a recoverable catch-up failure, got %d", len(enq.created)) + after := time.Now().UTC() + if len(enq.created) != 1 { + t.Fatalf("expected exactly one dispatch on a recoverable catch-up failure (AC-001.11), got %d", len(enq.created)) } triggers, err := repo.ListTriggersByRoutineID(ctx, routine.ID) @@ -161,9 +168,11 @@ func TestTickScheduledTriggers_RecoverableCatchUpFailure_RearmsForRetry(t *testi t.Fatalf("expected 1 trigger, got %d", len(triggers)) } if triggers[0].NextRunAt == nil { - t.Fatalf("next_run_at = nil, want re-armed to the original due time so the trigger retries") + t.Fatalf("next_run_at = nil, want re-armed to processing instant + 24h") } - if !triggers[0].NextRunAt.Equal(due) { - t.Fatalf("next_run_at = %v, want re-armed to original due time %v", *triggers[0].NextRunAt, due) + got := *triggers[0].NextRunAt + if got.Before(before.Add(24*time.Hour)) || got.After(after.Add(24*time.Hour)) { + t.Fatalf("next_run_at = %v, want within [%v, %v] (processing instant + 24h)", + got, before.Add(24*time.Hour), after.Add(24*time.Hour)) } } diff --git a/apps/backend/internal/office/routines/test_helpers.go b/apps/backend/internal/office/routines/test_helpers.go new file mode 100644 index 0000000000..23066aec3b --- /dev/null +++ b/apps/backend/internal/office/routines/test_helpers.go @@ -0,0 +1,18 @@ +package routines + +import "time" + +// MarshalRoutinePayloadForTest exposes marshalRoutinePayload to external +// test packages so cross-package tests (e.g. the prompt builder's gap +// rendering, AC-OFFICE-ROUTINE-CATCHUP-002.5) can round-trip through the +// real wakeup-request wire format instead of a hand-written JSON literal +// that could silently drift from what dispatch actually sends. +func MarshalRoutinePayloadForTest( + routineID string, vars map[string]string, missedTicks int, firstMissed time.Time, truncated bool, +) (string, error) { + var gap *gapSummary + if missedTicks > 0 { + gap = &gapSummary{MissedTicks: missedTicks, FirstMissed: firstMissed, Truncated: truncated} + } + return marshalRoutinePayload(routineID, vars, gap) +} diff --git a/apps/backend/internal/office/routines/wakeup_enqueue_fail_disposition_test.go b/apps/backend/internal/office/routines/wakeup_enqueue_fail_disposition_test.go index e43eefa250..86232ec3ac 100644 --- a/apps/backend/internal/office/routines/wakeup_enqueue_fail_disposition_test.go +++ b/apps/backend/internal/office/routines/wakeup_enqueue_fail_disposition_test.go @@ -13,9 +13,15 @@ import ( // generic (non-ErrWakeupAlreadyRequested) error, but still lets Dispatch/ // FailWakeupRequest succeed trivially — they are never reached once // CreateWakeupRequest itself fails. -type failingCreateWakeupEnqueuer struct{ fakeWakeupEnqueuer } +type failingCreateWakeupEnqueuer struct { + fakeWakeupEnqueuer + createErr error +} func (f *failingCreateWakeupEnqueuer) CreateWakeupRequest(context.Context, *routines.WakeupRequest) error { + if f.createErr != nil { + return f.createErr + } return errors.New("wakeup store unavailable") } diff --git a/apps/backend/internal/office/service/catchup_prompt_test.go b/apps/backend/internal/office/service/catchup_prompt_test.go new file mode 100644 index 0000000000..e656805692 --- /dev/null +++ b/apps/backend/internal/office/service/catchup_prompt_test.go @@ -0,0 +1,143 @@ +package service_test + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/kandev/kandev/internal/office/routines" + "github.com/kandev/kandev/internal/office/service" + "github.com/kandev/kandev/internal/office/shared" +) + +// routineCatchUpReasons are the three reasons AC-OFFICE-ROUTINE-CATCHUP-002.5 +// requires the gap to render under. Event is included specifically because +// PromoteRunAndCoalesceWakeupIfQueued can rewrite a cron-created run's +// reason to it after the gap was already measured and stored — a +// Cron-only gate would silently drop that gap. +var routineCatchUpReasons = []string{ + shared.RunReasonRoutineDispatchCron, + shared.RunReasonRoutineDispatchEvent, + shared.RunReasonRoutineDispatch, +} + +// TestBuildPrompt_RoutineGap_RendersUnderEachRoutineReason covers +// AC-OFFICE-ROUTINE-CATCHUP-002.5: a run whose ContextSnapshot carries a +// measured gap renders a wake-context line under all three routine-dispatch +// reasons, including the count and the first-missed timestamp, and states +// the count is a lower bound when truncated. +func TestBuildPrompt_RoutineGap_RendersUnderEachRoutineReason(t *testing.T) { + svc := newTestService(t) + ctx := context.Background() + + firstMissed := time.Date(2026, 5, 10, 11, 55, 0, 0, time.UTC) + snapshot, err := routines.MarshalRoutinePayloadForTest("r-1", nil, 5, firstMissed, true) + if err != nil { + t.Fatalf("marshal routine payload: %v", err) + } + + for _, reason := range routineCatchUpReasons { + t.Run(reason, func(t *testing.T) { + pc := service.BuildPromptContextWithSnapshotForTest(svc, ctx, reason, "{}", snapshot) + if pc.MissedTicks != 5 { + t.Fatalf("MissedTicks = %d, want 5", pc.MissedTicks) + } + if pc.MissedSince != "2026-05-10T11:55:00Z" { + t.Errorf("MissedSince = %q, want 2026-05-10T11:55:00Z", pc.MissedSince) + } + if !pc.MissedTruncated { + t.Error("MissedTruncated = false, want true") + } + + prompt := service.BuildPrompt(pc) + if !strings.Contains(prompt, "missed 5") { + t.Errorf("prompt = %q, want it to state the missed-tick count", prompt) + } + if !strings.Contains(prompt, "tick") { + t.Errorf("prompt = %q, want it to describe missed ticks, not runs (a tick never becomes a run)", prompt) + } + if strings.Contains(prompt, "run") { + t.Errorf("prompt = %q, want no mention of a missed run — only one run is ever dispatched", prompt) + } + if !strings.Contains(prompt, "2026-05-10T11:55:00Z") { + t.Errorf("prompt = %q, want it to state the first-missed timestamp", prompt) + } + if !strings.Contains(prompt, "lower bound") { + t.Errorf("prompt = %q, want it to state the count is a lower bound (truncated)", prompt) + } + }) + } +} + +// TestBuildPrompt_RoutineGap_NoSummaryRendersNoStatement covers AC-002.3: +// when the ContextSnapshot carries no gap (a merely-late tick, or a policy +// that opts out), the run's reason still gates on being a routine reason, +// but no missed-tick statement is rendered at all. +func TestBuildPrompt_RoutineGap_NoSummaryRendersNoStatement(t *testing.T) { + svc := newTestService(t) + ctx := context.Background() + + pc := service.BuildPromptContextWithSnapshotForTest( + svc, ctx, shared.RunReasonRoutineDispatchCron, "{}", `{"routine_id":"r-1"}`, + ) + if pc.MissedTicks != 0 { + t.Fatalf("MissedTicks = %d, want 0", pc.MissedTicks) + } + prompt := service.BuildPrompt(pc) + if strings.Contains(prompt, "missed") { + t.Errorf("prompt = %q, want no missed-tick statement", prompt) + } +} + +// TestBuildPrompt_RoutineGap_LegacyPayloadWithoutMissedSinceIsIgnored covers +// the upgrade path: a wakeup request queued by a pre-catch-up build wrote +// only {"routine_id":...,"missed_ticks":N} — missed_since did not exist yet +// — and can sit undispatched across a deploy. Decoding that legacy shape +// must not populate the catch-up fields from an incomplete payload, or the +// rendered prompt states a tick count with no timestamp ("since ."). +func TestBuildPrompt_RoutineGap_LegacyPayloadWithoutMissedSinceIsIgnored(t *testing.T) { + svc := newTestService(t) + ctx := context.Background() + + legacySnapshot := `{"routine_id":"r1","missed_ticks":5}` + pc := service.BuildPromptContextWithSnapshotForTest(svc, ctx, shared.RunReasonRoutineDispatchCron, "{}", legacySnapshot) + if pc.MissedTicks != 0 { + t.Fatalf("MissedTicks = %d, want 0 (a legacy payload with no missed_since must not populate the gap fields)", pc.MissedTicks) + } + if pc.MissedSince != "" { + t.Errorf("MissedSince = %q, want empty", pc.MissedSince) + } + + prompt := service.BuildPrompt(pc) + if strings.Contains(prompt, "missed") { + t.Errorf("prompt = %q, want no missed-tick statement for a legacy payload missing missed_since", prompt) + } + if strings.Contains(prompt, "since .") { + t.Errorf("prompt = %q, must never render a malformed empty timestamp", prompt) + } +} + +// TestBuildPrompt_RoutineGap_NonRoutineReasonIgnoresSnapshot proves the +// decode is gated on the run's reason: a non-routine reason with the exact +// same context_snapshot renders nothing, keeping every other run's prompt +// byte-identical to before this parameter existed. +func TestBuildPrompt_RoutineGap_NonRoutineReasonIgnoresSnapshot(t *testing.T) { + svc := newTestService(t) + ctx := context.Background() + + snapshot, err := routines.MarshalRoutinePayloadForTest( + "r-1", nil, 5, time.Date(2026, 5, 10, 11, 55, 0, 0, time.UTC), true, + ) + if err != nil { + t.Fatalf("marshal routine payload: %v", err) + } + pc := service.BuildPromptContextWithSnapshotForTest(svc, ctx, service.RunReasonHeartbeat, "{}", snapshot) + if pc.MissedTicks != 0 { + t.Fatalf("MissedTicks = %d, want 0 (non-routine reason must not decode the snapshot)", pc.MissedTicks) + } + prompt := service.BuildPrompt(pc) + if strings.Contains(prompt, "missed 5") { + t.Errorf("prompt = %q, want no missed-tick statement for a non-routine reason", prompt) + } +} diff --git a/apps/backend/internal/office/service/prompt_builder.go b/apps/backend/internal/office/service/prompt_builder.go index 543ad0a018..0a43114c26 100644 --- a/apps/backend/internal/office/service/prompt_builder.go +++ b/apps/backend/internal/office/service/prompt_builder.go @@ -89,6 +89,16 @@ type PromptContext struct { // renders metadata + the fetch-tool name so the agent can call // get_task_document_kandev to read content. HandoffContext *v1.TaskContext + + // Routine catch-up gap fields. Populated by buildPromptContext only + // when the run's reason is one of the three routine-dispatch reasons + // and its ContextSnapshot decodes a wakeup.RoutinePayload carrying a + // gap (AC-OFFICE-ROUTINE-CATCHUP-002.5). MissedTicks is 0 when no gap + // was measured or reported for this run's claim, in which case + // BuildPrompt renders no wake-context line at all. + MissedTicks int + MissedSince string + MissedTruncated bool } // BuildPrompt generates a structured prompt for a run reason. @@ -127,6 +137,7 @@ func BuildPrompt(pc *PromptContext) string { prompt = fmt.Sprintf("You have been woken for reason: %s.", pc.Reason) } prompt = appendOneTimeInstructions(prompt, pc.OneTimeInstructions) + prompt = appendMissedTicksSection(prompt, pc) prompt = appendHandoffSection(prompt, pc.HandoffContext) return appendRuntimeContext(prompt, pc) } @@ -141,6 +152,27 @@ func appendOneTimeInstructions(prompt, instructions string) string { return prompt + "\n\n## One-time workflow move instructions\n\n" + instructions } +// appendMissedTicksSection renders the gap a routine's cron tick measured +// for this run's claim, when one was recorded (AC-OFFICE-ROUTINE-CATCHUP-002.5). +// Renders nothing when MissedTicks is 0 — the no-gap-summary case (walk +// failed, policy is skip_missed, catch_up_max is 1, or the tick was merely +// late with no missed ticks) must produce no missed-tick statement at all +// (AC-002.3), and a manual or webhook fire never sets MissedTicks in the +// first place (AC-002.12). +func appendMissedTicksSection(prompt string, pc *PromptContext) string { + if pc == nil || pc.MissedTicks <= 0 { + return prompt + } + if !strings.HasSuffix(prompt, "\n") { + prompt += "\n" + } + line := fmt.Sprintf("\nYou missed %d scheduled tick(s) since %s.", pc.MissedTicks, pc.MissedSince) + if pc.MissedTruncated { + line += " This count is a lower bound (more ticks were missed than could be counted)." + } + return prompt + line +} + // appendHandoffSection renders the office task-handoffs context block // (Related tasks / Documents available / Workspace) onto every run // prompt that carries a HandoffContext. Document bodies are diff --git a/apps/backend/internal/office/service/scheduler_integration.go b/apps/backend/internal/office/service/scheduler_integration.go index 920f724c50..c7d913955a 100644 --- a/apps/backend/internal/office/service/scheduler_integration.go +++ b/apps/backend/internal/office/service/scheduler_integration.go @@ -14,6 +14,7 @@ import ( "github.com/kandev/kandev/internal/office/pause" officeruntime "github.com/kandev/kandev/internal/office/runtime" "github.com/kandev/kandev/internal/office/shared" + "github.com/kandev/kandev/internal/office/wakeup" v1 "github.com/kandev/kandev/pkg/api/v1" ) @@ -458,7 +459,7 @@ func (si *SchedulerIntegration) assembleAgentPrompt( runCtx officeruntime.RunContext, instructionsDir, agentsMD string, ) string { - pc := si.buildPromptContext(ctx, run.Reason, run.Payload) + pc := si.buildPromptContext(ctx, run.Reason, run.Payload, run.ContextSnapshot) pc.RunID = runCtx.RunID pc.AgentID = runCtx.AgentID pc.SessionID = runCtx.SessionID @@ -1036,13 +1037,18 @@ func (si *SchedulerIntegration) resolveExecutorForRun( return si.svc.ResolveExecutor(ctx, "", agent.ID, projectID, "") } -// buildPromptContext assembles a PromptContext from run data. +// buildPromptContext assembles a PromptContext from run data. contextSnapshot +// is run.ContextSnapshot — decoded as a wakeup.RoutinePayload only when +// reason is one of the three routine-dispatch reasons, so every other run's +// prompt stays byte-identical to before this parameter existed +// (AC-OFFICE-ROUTINE-CATCHUP-002.5). func (si *SchedulerIntegration) buildPromptContext( - ctx context.Context, reason, payload string, + ctx context.Context, reason, payload, contextSnapshot string, ) *PromptContext { parsed := ParseRunPayload(payload) pc := &PromptContext{Reason: reason} pc.OneTimeInstructions = parsed[RunPayloadOneTimeInstructionsKey] + applyRoutineCatchUpContext(pc, reason, contextSnapshot) if taskID := parsed["task_id"]; taskID != "" { si.enrichTaskContext(ctx, pc, taskID) @@ -1089,6 +1095,35 @@ func (si *SchedulerIntegration) buildPromptContext( return pc } +// applyRoutineCatchUpContext decodes contextSnapshot as a +// wakeup.RoutinePayload and copies its catch-up fields onto pc, but only +// when reason is one of the three routine-dispatch reasons. Gating on all +// three, not just RunReasonRoutineDispatchCron, matters: +// PromoteRunAndCoalesceWakeupIfQueued can rewrite an in-flight run's reason +// to RunReasonRoutineDispatchEvent after a cron claim already measured and +// stored the gap, so a Cron-only gate would silently drop it +// (AC-OFFICE-ROUTINE-CATCHUP-002.5). A manual or webhook fire never writes +// these fields, so gating the other two reasons this way is safe: there is +// nothing to decode for them. Any run with a different reason is left +// byte-identical to before this function existed. +func applyRoutineCatchUpContext(pc *PromptContext, reason, contextSnapshot string) { + switch reason { + case shared.RunReasonRoutineDispatchCron, shared.RunReasonRoutineDispatchEvent, shared.RunReasonRoutineDispatch: + default: + return + } + var routinePayload wakeup.RoutinePayload + if err := wakeup.UnmarshalPayload(contextSnapshot, &routinePayload); err != nil { + return + } + if routinePayload.MissedTicks <= 0 || routinePayload.MissedSince == "" { + return + } + pc.MissedTicks = routinePayload.MissedTicks + pc.MissedSince = routinePayload.MissedSince + pc.MissedTruncated = routinePayload.MissedTruncated +} + // enrichHandoffContext populates pc.HandoffContext from the office // task-handoffs context API so the run prompt's BuildPrompt can render // the Related-tasks / Documents-available / Workspace section. diff --git a/apps/backend/internal/office/service/test_helpers.go b/apps/backend/internal/office/service/test_helpers.go index 4e295f94c3..58e34b89d0 100644 --- a/apps/backend/internal/office/service/test_helpers.go +++ b/apps/backend/internal/office/service/test_helpers.go @@ -67,7 +67,17 @@ func ParseRateLimitResetTimeForTest(errMsg string, now time.Time) *time.Time { // This is exposed so integration tests in the _test package can verify prompt building. func BuildPromptContextForTest(svc *Service, ctx context.Context, reason, payload string) *PromptContext { si := &SchedulerIntegration{svc: svc, logger: svc.logger} - return si.buildPromptContext(ctx, reason, payload) + return si.buildPromptContext(ctx, reason, payload, "") +} + +// BuildPromptContextWithSnapshotForTest is BuildPromptContextForTest plus +// contextSnapshot (run.ContextSnapshot), the run-reason-gated source for +// the routine catch-up gap fields (AC-OFFICE-ROUTINE-CATCHUP-002.5). A +// separate helper, rather than extending BuildPromptContextForTest's +// signature, so its nine existing callers stay untouched. +func BuildPromptContextWithSnapshotForTest(svc *Service, ctx context.Context, reason, payload, contextSnapshot string) *PromptContext { + si := &SchedulerIntegration{svc: svc, logger: svc.logger} + return si.buildPromptContext(ctx, reason, payload, contextSnapshot) } // CoalesceRoutineWakeupForTest creates a wakeup-request carrying the diff --git a/apps/backend/internal/office/wakeup/payloads.go b/apps/backend/internal/office/wakeup/payloads.go index 57727de521..3064b0d28e 100644 --- a/apps/backend/internal/office/wakeup/payloads.go +++ b/apps/backend/internal/office/wakeup/payloads.go @@ -56,15 +56,20 @@ type AgentErrorPayload struct { // (free-form JSON) so the dispatcher / scriptengine can render them // into the prompt at run-build time. // -// MissedTicks is set by the routines cron tick when the catch-up cap -// collapsed N missed fires into one. Zero on the happy path; non-zero -// signals "you missed N ticks since the last fire" — the prompt -// builder surfaces it in the wake context so the agent can decide -// whether the gap matters. +// MissedTicks, MissedSince and MissedTruncated are the gap this claim +// measured, written together only when the routine's cron tick counted at +// least one missed tick under the summarize_missed policy +// (docs/specs/office/requirements/routine-catch-up.md, +// AC-OFFICE-ROUTINE-CATCHUP-002.1). They state the ticks counted and +// reported, never the ticks fired — exactly one run is ever dispatched per +// claim. All three are omitempty, so a gapless fire's payload is +// byte-identical to one with no catch-up fields at all. type RoutinePayload struct { - RoutineID string `json:"routine_id"` - Variables map[string]any `json:"variables,omitempty"` - MissedTicks int `json:"missed_ticks,omitempty"` + RoutineID string `json:"routine_id"` + Variables map[string]any `json:"variables,omitempty"` + MissedTicks int `json:"missed_ticks,omitempty"` + MissedSince string `json:"missed_since,omitempty"` + MissedTruncated bool `json:"missed_truncated,omitempty"` } // SelfPayload is the wakeup-request payload for an agent-initiated diff --git a/apps/web/app/office/routines/[id]/routine-detail-view.test.tsx b/apps/web/app/office/routines/[id]/routine-detail-view.test.tsx new file mode 100644 index 0000000000..17073dbf4e --- /dev/null +++ b/apps/web/app/office/routines/[id]/routine-detail-view.test.tsx @@ -0,0 +1,109 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { cleanup, render, screen, fireEvent, within } from "@testing-library/react"; +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()); + +const BASE_ROUTINE: Routine = { + id: "routine-1", + workspaceId: "ws-1", + name: "Nightly digest", + taskTemplate: {}, + status: "active", + concurrencyPolicy: "coalesce_if_active", + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-01T00:00:00Z", +}; + +const NO_TRIGGERS: RoutineTrigger[] = []; + +// 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 +// enqueue_missed_with_cap -> summarize_missed rename, which silently +// dropped this control when only the option values/defaults were updated +// without also updating the two "===" guards that decide whether the +// 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( + , + ); + 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( + , + ); + 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(); + expect(screen.getByText(/catch-up max/i)).toBeTruthy(); + expect(screen.getByRole("spinbutton")).toBeTruthy(); + }); +}); + +// AC-OFFICE-ROUTINE-CATCHUP-003.6: the summarizing policy's own label must +// 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( + , + ); + // Comboboxes in DOM order: status, assignee, concurrency policy, + // catch-up policy, then the trigger card's kind select. + const catchUpPolicyCombobox = screen.getAllByRole("combobox")[3]; + fireEvent.click(catchUpPolicyCombobox); + const option = within(screen.getByRole("listbox")).getByRole("option", { + name: /summarize missed/i, + }); + expect(option.textContent).toMatch(/once|single/i); + }); +}); diff --git a/apps/web/app/office/routines/[id]/routine-detail-view.tsx b/apps/web/app/office/routines/[id]/routine-detail-view.tsx index 6f932aeac2..4c9ef6c4be 100644 --- a/apps/web/app/office/routines/[id]/routine-detail-view.tsx +++ b/apps/web/app/office/routines/[id]/routine-detail-view.tsx @@ -56,7 +56,7 @@ function buildDraft(routine: Routine, triggers: RoutineTrigger[]): DraftState { status: (routine.status as DraftState["status"]) ?? "active", assigneeAgentProfileId: routine.assigneeAgentProfileId ?? "", concurrencyPolicy: routine.concurrencyPolicy ?? "coalesce_if_active", - catchUpPolicy: routine.catchUpPolicy ?? "enqueue_missed_with_cap", + catchUpPolicy: routine.catchUpPolicy ?? "summarize_missed", catchUpMax: routine.catchUpMax ?? 25, triggerKind, cronExpression: cron?.cronExpression ?? "", @@ -163,7 +163,7 @@ function DetailGeneralCard({ - {draft.catchUpPolicy === "enqueue_missed_with_cap" && ( + {draft.catchUpPolicy === "summarize_missed" && ( - - {t("office:enqueueMissedWithCap")} + + {t("office:summarizeMissed")} {t("office:skipMissed")} diff --git a/apps/web/app/office/routines/create-routine-dialog.test.tsx b/apps/web/app/office/routines/create-routine-dialog.test.tsx new file mode 100644 index 0000000000..a08d738c61 --- /dev/null +++ b/apps/web/app/office/routines/create-routine-dialog.test.tsx @@ -0,0 +1,105 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { cleanup, render, screen, fireEvent, within } from "@testing-library/react"; +import type { AgentProfile } from "@/lib/state/slices/office/types"; + +import { CreateRoutineDialog } from "./create-routine-dialog"; + +afterEach(() => cleanup()); + +const AGENT = { + id: "agent-1", + workspaceId: "ws-1", + name: "Worker", + role: "worker", + status: "idle", + agentProfileId: "profile-1", + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-01T00:00:00Z", + permissions: {}, + pauseReason: "", + budgetMonthlyCents: 0, + maxConcurrentSessions: 1, +} as AgentProfile; + +function renderDialog() { + return render( + , + ); +} + +// Advances from step 0 (Details) to step 2 (Schedule), where the catch-up +// policy control and the conditional catch_up_max input live. Step 0 +// requires a name and an assignee before "Next" is enabled; step 1 (task +// template) has no required fields. +function goToScheduleStep() { + fireEvent.change(screen.getByLabelText("Name"), { target: { value: "Nightly digest" } }); + fireEvent.click(screen.getAllByRole("combobox")[0]); + fireEvent.click(within(screen.getByRole("listbox")).getByRole("option", { name: "Worker" })); + fireEvent.click(screen.getByRole("button", { name: /next/i })); + fireEvent.click(screen.getByRole("button", { name: /next/i })); +} + +// AC-OFFICE-ROUTINE-CATCHUP-003.8: the catch_up_max control renders under +// the summarizing policy (the create dialog's default) and disappears +// under skip_missed. Regression coverage for the enqueue_missed_with_cap +// -> summarize_missed rename, which silently drops this control if only +// the option values/defaults are updated without the "===" guard deciding +// whether the input renders at all. +describe("CreateRoutineDialog catch-up max control (AC-003.8)", () => { + it("renders the catch-up max input by default (summarize_missed)", () => { + renderDialog(); + goToScheduleStep(); + expect(screen.getByText(/catch-up max/i)).toBeTruthy(); + expect(screen.getByRole("spinbutton")).toBeTruthy(); + }); + + it("hides the catch-up max input after selecting skip missed", () => { + renderDialog(); + goToScheduleStep(); + expect(screen.getByText(/catch-up max/i)).toBeTruthy(); + + // Comboboxes on the Schedule step, in DOM order: trigger kind, + // concurrency policy, catch-up policy. + const catchUpPolicyCombobox = screen.getAllByRole("combobox")[2]; + fireEvent.click(catchUpPolicyCombobox); + fireEvent.click( + within(screen.getByRole("listbox")).getByRole("option", { name: /skip missed/i }), + ); + + expect(screen.queryByText(/catch-up max/i)).toBeNull(); + expect(screen.queryByRole("spinbutton")).toBeNull(); + }); + + it("submits summarize_missed (not the retired enqueue_missed_with_cap value) as the default policy", () => { + const onSubmit = vi.fn(); + render( + , + ); + goToScheduleStep(); + fireEvent.change(screen.getByPlaceholderText("0 9 * * *"), { target: { value: "0 9 * * *" } }); + fireEvent.click(screen.getByRole("button", { name: /create/i })); + + expect(onSubmit).toHaveBeenCalledWith( + expect.objectContaining({ catchUpPolicy: "summarize_missed" }), + ); + }); +}); + +// AC-OFFICE-ROUTINE-CATCHUP-003.6: the summarizing policy's own label must +// state a single summarized wake, separately from catch_up_max's label +// (which already states the bound is on ticks counted, not runs created). +// A learner who only reads the policy dropdown — never expanding the +// conditional catch_up_max input under it — must still see this. +describe("CreateRoutineDialog catch-up policy labeling (AC-003.6)", () => { + it("labels the summarizing policy option as a single wake", () => { + renderDialog(); + goToScheduleStep(); + + const catchUpPolicyCombobox = screen.getAllByRole("combobox")[2]; + fireEvent.click(catchUpPolicyCombobox); + const option = within(screen.getByRole("listbox")).getByRole("option", { + name: /summarize missed/i, + }); + expect(option.textContent).toMatch(/once|single/i); + }); +}); diff --git a/apps/web/app/office/routines/create-routine-dialog.tsx b/apps/web/app/office/routines/create-routine-dialog.tsx index 2bca459a78..75b297e886 100644 --- a/apps/web/app/office/routines/create-routine-dialog.tsx +++ b/apps/web/app/office/routines/create-routine-dialog.tsx @@ -51,7 +51,7 @@ const INITIAL_ROUTINE_STATE: RoutineFormState = { taskDesc: "", assignee: "", concurrency: "coalesce_if_active", - catchUpPolicy: "enqueue_missed_with_cap", + catchUpPolicy: "summarize_missed", catchUpMax: 25, triggerKind: "cron", cronExpr: "", @@ -286,8 +286,8 @@ function PolicyFields({ - - {t("office:enqueueMissedWithCap")} + + {t("office:summarizeMissed")} {t("office:skipMissed")} @@ -298,7 +298,7 @@ function PolicyFields({ {t("office:whatHappensToTicksMissedWhile")}

- {state.catchUpPolicy === "enqueue_missed_with_cap" && ( + {state.catchUpPolicy === "summarize_missed" && (

- {t("office:beyondThisCountMissedTicksAre")} + {t("office:beyondThisCountMissedTicksAreNotCounted")}

)} diff --git a/apps/web/e2e/helpers/office-api-client.ts b/apps/web/e2e/helpers/office-api-client.ts index 00cb153e7d..f4809e6d89 100644 --- a/apps/web/e2e/helpers/office-api-client.ts +++ b/apps/web/e2e/helpers/office-api-client.ts @@ -565,7 +565,7 @@ export class OfficeApiClient { async createRoutine( wsId: string, - data: { name: string; description?: string }, + data: { name: string; description?: string; catch_up_policy?: string }, ): Promise> { const res = await this.request<{ routine: Record }>( "POST", @@ -575,6 +575,11 @@ export class OfficeApiClient { return res.routine ?? (res as unknown as Record); } + async getRoutine(id: string): Promise> { + const res = await this.request<{ routine: Record }>("GET", `/routines/${id}`); + return res.routine ?? (res as unknown as Record); + } + /** Manual fire (AC-OFFICE-KILL-SWITCH-002.4). Raw: a paused workspace answers 409. */ async runRoutine(routineId: string): Promise { return this.rawRequest("POST", `/routines/${routineId}/run`); diff --git a/apps/web/e2e/tests/office/mobile-routine-catch-up-policy-ui.spec.ts b/apps/web/e2e/tests/office/mobile-routine-catch-up-policy-ui.spec.ts new file mode 100644 index 0000000000..48a0b6bbff --- /dev/null +++ b/apps/web/e2e/tests/office/mobile-routine-catch-up-policy-ui.spec.ts @@ -0,0 +1,86 @@ +/** + * Mobile-Chrome interaction and geometry coverage for the routine catch-up + * policy control. The create-dialog and detail-view components have no + * mobile-specific branching, so touch interactions prove the same policy + * behavior on a phone viewport while the overflow assertion protects layout. + */ +import { type Page } from "@playwright/test"; +import { expect, test } from "../../fixtures/office-fixture"; +import { assertNoDocumentHorizontalOverflow } from "../../helpers/layout-assertions"; + +const RETIRED_LABEL = /enqueue.?missed.?with.?cap/i; +const SUMMARIZE_MISSED_LABEL = /Summarize missed.*(once|single)/i; + +function catchUpPolicyCombobox(page: Page) { + return page.getByText("Catch-up policy", { exact: true }).locator("..").getByRole("combobox"); +} + +function catchUpMaxInput(page: Page) { + return page.getByText("Catch-up max", { exact: true }).locator("..").getByRole("spinbutton"); +} + +async function assertMobileCatchUpPolicyToggles(page: Page) { + const catchUpMax = catchUpMaxInput(page); + + await expect(catchUpPolicyCombobox(page)).toHaveText(SUMMARIZE_MISSED_LABEL); + await expect(catchUpMax).toHaveValue("25"); + await catchUpMax.fill("42"); + await expect(catchUpMax).toHaveValue("42"); + + await catchUpPolicyCombobox(page).tap(); + await page.getByRole("option", { name: "Skip missed" }).tap(); + await expect(page.getByText("Catch-up max", { exact: true })).toHaveCount(0); + + await catchUpPolicyCombobox(page).tap(); + await page.getByRole("option", { name: SUMMARIZE_MISSED_LABEL }).tap(); + await expect(catchUpMaxInput(page)).toHaveValue("42"); + await expect(page.getByText(RETIRED_LABEL)).toHaveCount(0); +} + +test.describe("Mobile routine catch-up policy control", () => { + test("create dialog: catch-up policy control responds to touch input", async ({ + testPage, + prCapture, + }) => { + await testPage.goto("/office/routines"); + await testPage.getByRole("button", { name: "New Routine" }).tap(); + + await testPage.getByLabel("Name").fill("E2E Mobile Catch-up Dialog"); + await testPage.getByText("Assignee", { exact: true }).locator("..").getByRole("combobox").tap(); + await testPage.getByRole("option", { name: "CEO" }).tap(); + await testPage.getByRole("button", { name: "Next" }).tap(); + await testPage.getByRole("button", { name: "Next" }).tap(); + + await expect(testPage.getByText("Catch-up policy", { exact: true })).toBeVisible(); + await assertMobileCatchUpPolicyToggles(testPage); + await assertNoDocumentHorizontalOverflow(testPage); + + await prCapture.screenshot("mobile-create-dialog-catch-up-policy", { + caption: "Create Routine dialog on a phone viewport with the catch-up policy control", + }); + }); + + test("detail view: catch-up policy control responds to touch input", async ({ + officeApi, + officeSeed, + testPage, + prCapture, + }) => { + const routine = (await officeApi.createRoutine(officeSeed.workspaceId, { + name: "E2E Mobile Catch-up Detail Toggle", + })) as { id: string }; + expect(routine.id).toBeTruthy(); + + await testPage.goto(`/office/routines/${routine.id}`); + await expect(testPage.getByText("E2E Mobile Catch-up Detail Toggle")).toBeVisible({ + timeout: 10_000, + }); + + await assertMobileCatchUpPolicyToggles(testPage); + await assertNoDocumentHorizontalOverflow(testPage); + + await prCapture.screenshot("mobile-detail-view-catch-up-policy", { + caption: "Routine detail view on a phone viewport with the catch-up policy control", + }); + }); +}); diff --git a/apps/web/e2e/tests/office/routine-catch-up-policy-ui.spec.ts b/apps/web/e2e/tests/office/routine-catch-up-policy-ui.spec.ts new file mode 100644 index 0000000000..a2f0d0f814 --- /dev/null +++ b/apps/web/e2e/tests/office/routine-catch-up-policy-ui.spec.ts @@ -0,0 +1,131 @@ +import { type Page } from "@playwright/test"; +import { test, expect } from "../../fixtures/office-fixture"; + +/** + * Routine catch-up policy rename (gap 24): `enqueue_missed_with_cap` -> + * `summarize_missed`, with the old value accepted forever as a deprecated + * alias on write and normalized away on read. + * + * NOTE ON SCOPE: `apps/web/lib/state/slices/office/types.ts`'s `Routine` + * type (pre-existing, unchanged by this diff) declares every multi-word + * field in camelCase (`catchUpPolicy`, `catchUpMax`, `concurrencyPolicy`, + * `assigneeAgentProfileId`, `taskTemplate`, `workspaceId`), but the backend + * (`apps/backend/internal/office/models/models.go`) serializes and binds + * all of them in snake_case. Neither the Create Routine dialog's submit + * nor the detail view's Save round-trips any of those fields — they're + * silently dropped on write and read back as `undefined` (falling through + * to each field's hardcoded UI default) regardless of what's actually + * persisted. This is pre-existing (present unchanged at the merge base) + * and spans the whole Routines feature, not just catch-up policy — filed + * as a follow-up, not fixed here. Consequence for this spec: the create + * dialog's and detail view's catch-up-policy controls can only be proven + * correct through pure client-side interaction (matching AC-003.8's own + * "component test, not a flow" framing) — a real "seed via API, load the + * page, expect the persisted policy to render" round trip is not currently + * possible for either surface. The alias-normalization contract itself + * (AC-003.1) is still verified for real over HTTP, via the API layer. + */ + +const RETIRED_LABEL = /enqueue.?missed.?with.?cap/i; + +function catchUpPolicyCombobox(page: Page) { + return page.getByText("Catch-up policy", { exact: true }).locator("..").getByRole("combobox"); +} + +// AC-OFFICE-ROUTINE-CATCHUP-003.6: the summarizing policy's own label +// states a single summarized wake, distinct from catch_up_max's label +// (which states the bound is on ticks counted, not runs created). +const SUMMARIZE_MISSED_LABEL = /Summarize missed.*(once|single)/i; + +async function assertCatchUpPolicyToggles(page: Page, catchUpMaxInput: () => Promise | void) { + // Default: summarize_missed, catch-up max visible. + await expect(catchUpPolicyCombobox(page)).toHaveText(SUMMARIZE_MISSED_LABEL); + await catchUpMaxInput(); + + await catchUpPolicyCombobox(page).click(); + await page.getByRole("option", { name: "Skip missed" }).click(); + await expect(page.getByText("Catch-up max", { exact: true })).toHaveCount(0); + + await catchUpPolicyCombobox(page).click(); + await page.getByRole("option", { name: SUMMARIZE_MISSED_LABEL }).click(); + await catchUpMaxInput(); + + await expect(page.getByText(RETIRED_LABEL)).toHaveCount(0); +} + +test.describe("Routine catch-up policy UI", () => { + test("create dialog: catch-up policy control visibility toggles live", async ({ + testPage, + prCapture, + }) => { + await testPage.goto("/office/routines"); + await testPage.getByRole("button", { name: "New Routine" }).click(); + + await testPage.getByLabel("Name").fill("E2E Catch-up Dialog"); + await testPage + .getByText("Assignee", { exact: true }) + .locator("..") + .getByRole("combobox") + .click(); + await testPage.getByRole("option", { name: "CEO" }).click(); + await testPage.getByRole("button", { name: "Next" }).click(); + await testPage.getByRole("button", { name: "Next" }).click(); + + await assertCatchUpPolicyToggles(testPage, async () => { + await expect(testPage.getByLabel("Catch-up max")).toHaveValue("25"); + }); + await prCapture.screenshot("create-dialog-catch-up-policy", { + caption: + "Create Routine dialog with the summarize_missed catch-up policy and its catch-up max field", + }); + }); + + test("detail view: catch-up policy control visibility toggles live", async ({ + officeApi, + officeSeed, + testPage, + prCapture, + }) => { + const routine = (await officeApi.createRoutine(officeSeed.workspaceId, { + name: "E2E Catch-up Detail Toggle", + })) as { id: string }; + expect(routine.id).toBeTruthy(); + + await testPage.goto(`/office/routines/${routine.id}`); + await expect(testPage.getByText("E2E Catch-up Detail Toggle")).toBeVisible({ + timeout: 10_000, + }); + + await assertCatchUpPolicyToggles(testPage, async () => { + await expect(testPage.getByText("Catch-up max", { exact: true })).toBeVisible(); + }); + await prCapture.screenshot("detail-view-catch-up-policy", { + caption: + "Routine detail view with the summarize_missed catch-up policy and its catch-up max field", + }); + }); + + test("API: the deprecated catch-up policy alias is accepted and normalized on read", async ({ + officeApi, + officeSeed, + testPage, + }) => { + const routine = (await officeApi.createRoutine(officeSeed.workspaceId, { + name: "E2E Catch-up Legacy Alias", + catch_up_policy: "enqueue_missed_with_cap", + })) as { id: string }; + expect(routine.id).toBeTruthy(); + + // Real HTTP round trip against the running backend (not just the Go + // repository-level unit tests): the deprecated alias was accepted on + // write and is normalized away on read, per AC-003.1/AC-003.2. + const stored = await officeApi.getRoutine(routine.id); + expect(stored["catch_up_policy"]).toBe("summarize_missed"); + + // The detail page loads cleanly for a routine created with the alias, + // and the retired string never leaks into rendered copy. + await testPage.goto(`/office/routines/${routine.id}`); + await expect(testPage.getByText("E2E Catch-up Legacy Alias")).toBeVisible({ timeout: 10_000 }); + await expect(testPage.getByText(RETIRED_LABEL)).toHaveCount(0); + }); +}); diff --git a/apps/web/lib/state/slices/office/types.ts b/apps/web/lib/state/slices/office/types.ts index b81abbb44e..e0cf7dfc07 100644 --- a/apps/web/lib/state/slices/office/types.ts +++ b/apps/web/lib/state/slices/office/types.ts @@ -202,6 +202,12 @@ export type RoutineRun = { linkedTaskId?: string; coalescedIntoRunId?: string; dispatchFingerprint?: string; + // Gap summary measured for the claim that created this run (absent when no + // gap was recorded — never a stored zero). See + // docs/specs/office/requirements/routine-catch-up.md AC-002. + catchUpMissedTicks?: number; + catchUpFirstMissedAt?: string; + catchUpTruncated?: boolean; startedAt?: string; completedAt?: string; createdAt: string; diff --git a/apps/web/src/locales/en/office.json b/apps/web/src/locales/en/office.json index da02e4a97c..596dc70583 100644 --- a/apps/web/src/locales/en/office.json +++ b/apps/web/src/locales/en/office.json @@ -98,7 +98,7 @@ "backToTasks": "Back to tasks", "barTotal": "{{label}}: {{total}}", "beingWorkedOn": "being worked on", - "beyondThisCountMissedTicksAre": "Beyond this count, missed ticks are dropped (default 25)", + "beyondThisCountMissedTicksAreNotCounted": "Beyond this count, missed ticks are not counted or reported (default 25). The agent still wakes exactly once.", "blockedActionRequired": "Blocked; action required", "blockNewTasks": "Block New Tasks", "boardView": "Board view", @@ -298,7 +298,6 @@ "effectiveActionsThisAgentCanRequest": "Effective actions this agent can request during Office runs.", "eGCodeReview": "e.g. code-review", "eGFrontendWorker": "e.g. Frontend Worker", - "enqueueMissedWithCap": "Enqueue missed (with cap)", "entityId": "Entity ID", "events": "Events", "exceeded": "Exceeded", @@ -862,6 +861,7 @@ "subscriptionQuota": "Subscription Quota", "succeeded": "Succeeded", "successRate": "Success Rate", + "summarizeMissed": "Summarize missed (wakes once)", "sync": "Sync", "syncTheWorkspaceDatabaseWithOn": "Sync the workspace database with on-disk YAML files, or download a portable export bundle.", "system": "System", diff --git a/apps/web/src/locales/pseudo/office.json b/apps/web/src/locales/pseudo/office.json index 237adbe1ea..dbfaf2a1c8 100644 --- a/apps/web/src/locales/pseudo/office.json +++ b/apps/web/src/locales/pseudo/office.json @@ -98,7 +98,7 @@ "backToTasks": "Ɓàćķ ţō ţàśķś", "barTotal": "{{label}}: {{total}}", "beingWorkedOn": "ƀēĩńĝ ŵōŕķēď ōń", - "beyondThisCountMissedTicksAre": "Ɓēŷōńď ţĥĩś ćōũńţ, ḿĩśśēď ţĩćķś àŕē ďŕōƥƥēď (ďēƒàũĺţ 25)", + "beyondThisCountMissedTicksAreNotCounted": "Ɓēŷōńď ţĥĩś ćōũńţ, ḿĩśśēď ţĩćķś àŕē ńōţ ćōũńţēď ōŕ ŕēƥōŕţēď (ďēƒàũĺţ 25). Ţĥē àĝēńţ śţĩĺĺ ŵàķēś ēxàćţĺŷ ōńćē.", "blockedActionRequired": "Ɓĺōćķēď; àćţĩōń ŕēqũĩŕēď", "blockNewTasks": "Ɓĺōćķ Ńēŵ Ţàśķś", "boardView": "Ɓōàŕď vĩēŵ", @@ -298,7 +298,6 @@ "effectiveActionsThisAgentCanRequest": "Ēƒƒēćţĩvē àćţĩōńś ţĥĩś àĝēńţ ćàń ŕēqũēśţ ďũŕĩńĝ Ōƒƒĩćē ŕũńś.", "eGCodeReview": "ē.ĝ. ćōďē-ŕēvĩēŵ", "eGFrontendWorker": "ē.ĝ. Ƒŕōńţēńď Ŵōŕķēŕ", - "enqueueMissedWithCap": "Ēńqũēũē ḿĩśśēď (ŵĩţĥ ćàƥ)", "entityId": "Ēńţĩţŷ ĨĎ", "events": "Ēvēńţś", "exceeded": "Ēxćēēďēď", @@ -862,6 +861,7 @@ "subscriptionQuota": "Śũƀśćŕĩƥţĩōń Qũōţà", "succeeded": "Śũććēēďēď", "successRate": "Śũććēśś Ŕàţē", + "summarizeMissed": "Śũḿḿàŕĩźē ḿĩśśēď (ŵàķēś ōńćē)", "sync": "Śŷńć", "syncTheWorkspaceDatabaseWithOn": "Śŷńć ţĥē ŵōŕķśƥàćē ďàţàƀàśē ŵĩţĥ ōń-ďĩśķ ŶÀḾĹ ƒĩĺēś, ōŕ ďōŵńĺōàď à ƥōŕţàƀĺē ēxƥōŕţ ƀũńďĺē.", "system": "Śŷśţēḿ", diff --git a/apps/web/src/locales/pt-pt/office.json b/apps/web/src/locales/pt-pt/office.json index 84cf9d46b7..d71112ebf3 100644 --- a/apps/web/src/locales/pt-pt/office.json +++ b/apps/web/src/locales/pt-pt/office.json @@ -98,7 +98,7 @@ "backToTasks": "Voltar às tarefas", "barTotal": "{{label}}: {{total}}", "beingWorkedOn": "em curso", - "beyondThisCountMissedTicksAre": "Acima deste número, os ticks perdidos são descartados (pré-definição: 25)", + "beyondThisCountMissedTicksAreNotCounted": "Acima deste número, os ticks perdidos não são contados nem comunicados (pré-definição: 25). O agente continua a acordar exatamente uma vez.", "blockedActionRequired": "Bloqueada; é necessária uma ação", "blockNewTasks": "Bloquear novas tarefas", "boardView": "Vista de quadro", @@ -298,7 +298,6 @@ "effectiveActionsThisAgentCanRequest": "Ações efetivas que este agente pode pedir durante as execuções do Office.", "eGCodeReview": "por exemplo, code-review", "eGFrontendWorker": "por exemplo, Trabalhador de Frontend", - "enqueueMissedWithCap": "Colocar em fila os perdidos (com limite)", "entityId": "ID da entidade", "events": "Eventos", "exceeded": "Excedido", @@ -862,6 +861,7 @@ "subscriptionQuota": "Quota da subscrição", "succeeded": "Bem-sucedidas", "successRate": "Taxa de sucesso", + "summarizeMissed": "Resumir os perdidos (acorda uma vez)", "sync": "Sincronizar", "syncTheWorkspaceDatabaseWithOn": "Sincronize a base de dados da área de trabalho com os ficheiros YAML em disco ou transfira um pacote de exportação portátil.", "system": "Sistema", diff --git a/apps/web/src/locales/zh-cn/office.json b/apps/web/src/locales/zh-cn/office.json index 9f49598b39..88e24cf0ef 100644 --- a/apps/web/src/locales/zh-cn/office.json +++ b/apps/web/src/locales/zh-cn/office.json @@ -98,7 +98,7 @@ "backToTasks": "返回任务", "barTotal": "{{label}}：{{total}}", "beingWorkedOn": "处理中", - "beyondThisCountMissedTicksAre": "超过此数量后，错过的触发会被丢弃（默认 25）", + "beyondThisCountMissedTicksAreNotCounted": "超过此数量后，错过的触发不会被计入或报告（默认 25）。代理仍然只会被唤醒一次。", "blockedActionRequired": "已阻塞；需要操作", "blockNewTasks": "阻止新任务", "boardView": "看板视图", @@ -298,7 +298,6 @@ "effectiveActionsThisAgentCanRequest": "此智能体在办公室运行期间可请求的有效操作。", "eGCodeReview": "例如 code-review", "eGFrontendWorker": "例如 Frontend Worker", - "enqueueMissedWithCap": "将错过的加入队列（有上限）", "entityId": "实体 ID", "events": "事件", "exceeded": "已超出", @@ -862,6 +861,7 @@ "subscriptionQuota": "订阅配额", "succeeded": "成功", "successRate": "成功率", + "summarizeMissed": "汇总错过的（只唤醒一次）", "sync": "同步", "syncTheWorkspaceDatabaseWithOn": "将工作区数据库与磁盘上的 YAML 同步，或下载可移植导出包。", "system": "系统", diff --git a/apps/web/src/locales/zh-hk/office.json b/apps/web/src/locales/zh-hk/office.json index 24d873b1d4..607525b753 100644 --- a/apps/web/src/locales/zh-hk/office.json +++ b/apps/web/src/locales/zh-hk/office.json @@ -98,7 +98,7 @@ "backToTasks": "返回任務", "barTotal": "{{label}}：{{total}}", "beingWorkedOn": "處理中", - "beyondThisCountMissedTicksAre": "超過此數量後，錯過的觸發會被丟棄（預設 25）", + "beyondThisCountMissedTicksAreNotCounted": "超過此數量後，錯過的觸發不會被計入或報告（預設 25）。代理仍然只會被喚醒一次。", "blockedActionRequired": "已阻塞；需要操作", "blockNewTasks": "阻止新任務", "boardView": "看板視圖", @@ -298,7 +298,6 @@ "effectiveActionsThisAgentCanRequest": "此代理程式在辦公室執行期間可請求的有效操作。", "eGCodeReview": "例如 code-review", "eGFrontendWorker": "例如 Frontend Worker", - "enqueueMissedWithCap": "將錯過的加入佇列（有上限）", "entityId": "實體 ID", "events": "事件", "exceeded": "已超出", @@ -862,6 +861,7 @@ "subscriptionQuota": "訂閲配額", "succeeded": "成功", "successRate": "成功率", + "summarizeMissed": "彙總錯過的（只喚醒一次）", "sync": "同步", "syncTheWorkspaceDatabaseWithOn": "將工作區數據庫與磁碟上的 YAML 同步，或下載可移植匯出包。", "system": "系統", diff --git a/apps/web/src/locales/zh-tw/office.json b/apps/web/src/locales/zh-tw/office.json index bda14f2929..1bb1a62c3d 100644 --- a/apps/web/src/locales/zh-tw/office.json +++ b/apps/web/src/locales/zh-tw/office.json @@ -98,7 +98,7 @@ "backToTasks": "返回任務", "barTotal": "{{label}}：{{total}}", "beingWorkedOn": "處理中", - "beyondThisCountMissedTicksAre": "超過此數量後，錯過的觸發會被丟棄（預設 25）", + "beyondThisCountMissedTicksAreNotCounted": "超過此數量後，錯過的觸發不會被計入或報告（預設 25）。代理仍然只會被喚醒一次。", "blockedActionRequired": "已阻塞；需要操作", "blockNewTasks": "阻止新任務", "boardView": "看板檢視", @@ -298,7 +298,6 @@ "effectiveActionsThisAgentCanRequest": "此代理程式在辦公室執行期間可請求的有效操作。", "eGCodeReview": "例如 code-review", "eGFrontendWorker": "例如 Frontend Worker", - "enqueueMissedWithCap": "將錯過的加入佇列（有上限）", "entityId": "實體 ID", "events": "事件", "exceeded": "已超出", @@ -862,6 +861,7 @@ "subscriptionQuota": "訂閱配額", "succeeded": "成功", "successRate": "成功率", + "summarizeMissed": "彙總錯過的（只喚醒一次）", "sync": "同步", "syncTheWorkspaceDatabaseWithOn": "將工作區資料庫與磁碟上的 YAML 同步，或下載可移植匯出包。", "system": "系統", diff --git a/docs/plans/routine-catch-up/plan.md b/docs/plans/routine-catch-up/plan.md new file mode 100644 index 0000000000..95fc532813 --- /dev/null +++ b/docs/plans/routine-catch-up/plan.md @@ -0,0 +1,92 @@ +--- +created: 2026-09-09 +status: done +requirements: + - REQ-OFFICE-ROUTINE-CATCHUP-001 + - REQ-OFFICE-ROUTINE-CATCHUP-002 + - REQ-OFFICE-ROUTINE-CATCHUP-003 +system_design: + - ../../specs/office/system-design/routine-catch-up-01.md + - ../../specs/office/system-design/routine-catch-up-02.md +--- + +# Implementation plan: Office routine catch-up semantics (gap 24) + +## Overview + +Four surfaces disagreed about what happens when a routine's cron trigger comes +due while the backend is down: the policy name `enqueue_missed_with_cap`, the +`catch_up_max` field, the scheduler design docs, and the web UI label all said +missed ticks are enqueued up to a cap, while the code counted elapsed ticks and +then dispatched **exactly one** run regardless of the count. This plan makes +the code's behavior the deliberate, reviewed contract: one resume produces one +wake, `catch_up_max` bounds only how many elapsed ticks are counted, and the +crossed gap (missed-tick count, first-missed timestamp, truncated flag) is +measured, stored durably on the routine run, and — for a lightweight routine +under the summarizing policy — rendered into the woken agent's prompt. The +policy value is renamed `enqueue_missed_with_cap` -> `summarize_missed`, with +the old value accepted forever as a deprecated alias on write and normalized +away on read. + +## Scope + +In scope: the tick/claim/gap measurement and its durable storage (REQ-001), the +gap's delivery to the agent for a lightweight routine under the summarizing +policy (REQ-002), and renaming the misdescribing policy value across every +surface that names it — the Go enum, six creation paths, a SQLite table +rebuild, two frontend files, five locale catalogs, and both design documents +(REQ-003). + +Out of scope: enqueueing N missed runs, renaming `catch_up_max` itself, a +trigger-update operation, downgrade compatibility, a paused routine still +firing, tick-level cron correctness (a separate gap), WIP/budget enforcement +during catch-up, and retention of gap summaries. + +## Technical approach + +`routines/service.go`'s `computeCatchUp` replaces the previous +`computeRoutineMissed`: it walks cron ticks capped at +`models.NormaliseCatchUpMax(routine.CatchUpMax)`, but the tick dispatches +exactly one run either way — the cap bounds counting, never dispatch. +`buildGapSummary` turns that result into a durable `catch_up_missed_ticks` / +`catch_up_first_missed_at` / `catch_up_truncated` record on the created +`RoutineRun`, written once at creation so a run later marked skipped, +coalesced or failed still carries the gap measured for its tick. For a +lightweight routine under `summarize_missed`, the wakeup payload and prompt +builder surface the same three fields. `models/catchup.go` owns the +policy rename (with the deprecated alias normalized on read and write) and the +`catch_up_max` clamp, funneled through `insertRoutine`/`UpdateRoutine` so every +write path and the one-time upgrade migration agree. + +## Implementation waves and parallel candidates + +- [x] [Task 01: Tick, claim, and gap durability](task-01-tick-and-gap.md) (`done`) — REQ-001, REQ-002 +- [x] [Task 02: Rename the catch-up policy across every surface](task-02-policy-rename.md) (`done`) — REQ-003 + +Sequential: task 02's rename touches the same `Routine`/`RoutineTrigger` +plumbing task 01 introduces, and both share the frontend catch-up policy +control. + +## Verification results + +Full `internal/office/...` suite green (`go test ./internal/office/...`), +`go vet`, `golangci-lint run --new-from-rev= ./...` 0 issues, +`go run ./cmd/sqlguard ./internal` clean, `pnpm run i18n:check` / +`i18n:ratchet` clean (5 locales + pseudo), `pnpm vitest run +app/office/routines` green, `python3 scripts/lint-spec-files.py --all` clean. +E2E: `routine-catch-up-policy-ui.spec.ts` and the mobile counterpart, plus the +`routines-ui`/`routines`/`routine-fire` regression trio, all green against a +rebuilt frontend/backend E2E bundle. A real PostgreSQL run proved the +default-policy migration's dialect-gated `ALTER TABLE ... SET DEFAULT` path +(no table rebuild needed on PG). + +## Risks and out of scope + +- `catch_up_max` reaches SQL at exactly two statements (`insertRoutine`, + `UpdateRoutine`) — the single normalization funnel every write path shares. +- Nothing is discarded past the cap: no run was ever going to exist for a + missed tick, so truncation bounds the count, never the gap's start + timestamp. +- A pre-existing, unrelated mismatch between the Routines create-dialog's + camelCase request fields and the backend's snake_case JSON tags (also + affecting `concurrencyPolicy`) is tracked separately, not fixed here. diff --git a/docs/plans/routine-catch-up/task-01-tick-and-gap.md b/docs/plans/routine-catch-up/task-01-tick-and-gap.md new file mode 100644 index 0000000000..198f31eab0 --- /dev/null +++ b/docs/plans/routine-catch-up/task-01-tick-and-gap.md @@ -0,0 +1,115 @@ +--- +id: "01-tick-and-gap" +title: "Tick, claim, and gap durability" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-OFFICE-ROUTINE-CATCHUP-001 + - REQ-OFFICE-ROUTINE-CATCHUP-002 +acceptance_criteria: + - AC-OFFICE-ROUTINE-CATCHUP-001.1 + - AC-OFFICE-ROUTINE-CATCHUP-001.2 + - AC-OFFICE-ROUTINE-CATCHUP-001.3 + - AC-OFFICE-ROUTINE-CATCHUP-001.4 + - AC-OFFICE-ROUTINE-CATCHUP-001.5 + - AC-OFFICE-ROUTINE-CATCHUP-001.6 + - AC-OFFICE-ROUTINE-CATCHUP-001.7 + - AC-OFFICE-ROUTINE-CATCHUP-001.8 + - AC-OFFICE-ROUTINE-CATCHUP-001.9 + - AC-OFFICE-ROUTINE-CATCHUP-001.10 + - AC-OFFICE-ROUTINE-CATCHUP-001.11 + - AC-OFFICE-ROUTINE-CATCHUP-001.12 + - AC-OFFICE-ROUTINE-CATCHUP-001.13 + - AC-OFFICE-ROUTINE-CATCHUP-002.1 + - AC-OFFICE-ROUTINE-CATCHUP-002.2 + - AC-OFFICE-ROUTINE-CATCHUP-002.3 + - AC-OFFICE-ROUTINE-CATCHUP-002.4 + - AC-OFFICE-ROUTINE-CATCHUP-002.5 + - AC-OFFICE-ROUTINE-CATCHUP-002.6 + - AC-OFFICE-ROUTINE-CATCHUP-002.7 + - AC-OFFICE-ROUTINE-CATCHUP-002.8 + - AC-OFFICE-ROUTINE-CATCHUP-002.9 + - AC-OFFICE-ROUTINE-CATCHUP-002.10 + - AC-OFFICE-ROUTINE-CATCHUP-002.11 + - AC-OFFICE-ROUTINE-CATCHUP-002.12 +system_design: + - ../../specs/office/system-design/routine-catch-up-01.md +--- + +# Task 01: Tick, claim, and gap durability + +## Summary + +Make a cron trigger due while the backend is down produce exactly one run on +resume, whatever the gap size, and give that gap a durable, observable record. + +## In scope + +- `apps/backend/internal/office/routines/service.go`: `computeCatchUp` + (replaces `computeRoutineMissed`), `buildGapSummary`, `processCronTrigger`, + `reconcileStrandedTriggers`, `CreateRoutineTrigger` cron-satisfiability + validation. +- `apps/backend/internal/office/models/catchup.go`: `NormaliseCatchUpMax` + clamp [1, 1000]; `RoutineRun.CatchUpMissedTicks` / + `CatchUpFirstMissedAt` / `CatchUpTruncated`. +- `apps/backend/internal/office/repository/sqlite/{routines.go, + catchup_migrations.go,catchup_migrations_postgres.go,wakeup_requests.go}`: + schema default, idempotent `ADD COLUMN`, table-rebuild migration (SQLite) / + dialect-gated `ALTER TABLE ... SET DEFAULT` (Postgres); `GetDueTriggers` + ordering; `ListStrandedTriggers` / `ReconcileTriggerNextRun`. +- `apps/backend/internal/office/service/{prompt_builder.go, + scheduler_integration.go}`: `appendMissedTicksSection`, + `applyRoutineCatchUpContext`. + +## Acceptance + +- A cron trigger with any elapsed-tick count on resume dispatches exactly one + run; `catch_up_max` bounds only how many ticks are counted. +- The gap (missed-tick count, first-missed timestamp, truncated flag) is + stored durably on the created `RoutineRun`, written once at creation so a + run later marked skipped, coalesced, or failed still carries it. +- A cron trigger of kind "cron" is never persisted with a null `next_run_at` + at creation; a claim left unarmed by a process stopping mid-tick is + reconciled by a later tick without dispatching a spurious run. +- When the elapsed-tick computation fails, `next_run_at` arms to the + processing instant plus 24 hours with a warning naming the trigger and the + underlying error; a syntactically valid but permanently unsatisfiable + expression is instead left disarmed rather than retried forever. + +## Verification + +```bash +cd apps/backend && go test ./internal/office/routines/... ./internal/office/repository/sqlite/... -count=1 +cd apps/backend && go run ./cmd/sqlguard ./internal +cd apps/backend && KANDEV_TEST_POSTGRES_DSN=... go test ./internal/office/repository/sqlite/... -run Postgres -count=1 +``` + +## Files likely touched + +- `apps/backend/internal/office/routines/service.go` +- `apps/backend/internal/office/models/catchup.go` +- `apps/backend/internal/office/repository/sqlite/routines.go` +- `apps/backend/internal/office/repository/sqlite/catchup_migrations.go` +- `apps/backend/internal/office/repository/sqlite/catchup_migrations_postgres.go` +- `apps/backend/internal/office/repository/sqlite/wakeup_requests.go` +- `apps/backend/internal/office/service/prompt_builder.go` +- `apps/backend/internal/office/service/scheduler_integration.go` + +## Dependencies + +None. + +## Parallelism + +Sequential with task 02, which renames the policy this task's dispatch path +reads. + +## Results + +Implemented with TDD across the build history recorded in this task's Kandev +plan: the tick/claim/reconciliation path, the gap-summary funnel, the +SQLite/Postgres schema and migrations, and the prompt/wakeup payload fields. +Full `internal/office` suite green; a real PostgreSQL run proved the +dialect-gated default-policy migration. diff --git a/docs/plans/routine-catch-up/task-02-policy-rename.md b/docs/plans/routine-catch-up/task-02-policy-rename.md new file mode 100644 index 0000000000..3545699728 --- /dev/null +++ b/docs/plans/routine-catch-up/task-02-policy-rename.md @@ -0,0 +1,89 @@ +--- +id: "02-policy-rename" +title: "Rename the catch-up policy across every surface" +status: done +wave: 1 +depends_on: ["01-tick-and-gap"] +plan: "plan.md" +requirements: + - REQ-OFFICE-ROUTINE-CATCHUP-003 +acceptance_criteria: + - AC-OFFICE-ROUTINE-CATCHUP-003.1 + - AC-OFFICE-ROUTINE-CATCHUP-003.2 + - AC-OFFICE-ROUTINE-CATCHUP-003.3 + - AC-OFFICE-ROUTINE-CATCHUP-003.4 + - AC-OFFICE-ROUTINE-CATCHUP-003.5 + - AC-OFFICE-ROUTINE-CATCHUP-003.6 + - AC-OFFICE-ROUTINE-CATCHUP-003.7 + - AC-OFFICE-ROUTINE-CATCHUP-003.8 +system_design: + - ../../specs/office/system-design/routine-catch-up-02.md +--- + +# Task 02: Rename the catch-up policy across every surface + +## Summary + +`enqueue_missed_with_cap` is a false statement of what the code does (fires +once, never enqueues N runs). Rename it to `summarize_missed` everywhere it is +written or displayed, while accepting the old value forever as a deprecated +alias so existing rows and API callers keep working. + +## In scope + +- `apps/backend/internal/office/models/catchup.go`: the policy enum, + `NormaliseCatchUpPolicy` (accepts the deprecated alias on write, normalizes + on read), the one-time upgrade migration for pre-existing rows. +- `apps/web/app/office/routines/{create-routine-dialog.tsx, + [id]/routine-detail-view.tsx}` and `apps/web/src/locales/*/office.json` + (5 languages + pseudo): the policy label, including the "(wakes once)" + qualifier. +- `docs/specs/office/system-design/scheduler-01.md` / + `scheduler-02.md`: correct the design-doc prose that previously matched the + misdescribing name. + +## Acceptance + +- The policy value is `summarize_missed`; `enqueue_missed_with_cap` is + accepted as a deprecated alias on every write path and normalized away on + every read path, including a one-time upgrade pass over existing rows. +- The web UI's catch-up policy label states, in every required locale, that + the policy wakes once rather than enqueueing per missed tick. +- `scheduler-01.md` and `scheduler-02.md` no longer describe the retired + enqueue-N-runs behavior. + +## Verification + +```bash +cd apps/backend && go test ./internal/office/models/... ./internal/office/repository/sqlite/... -run CatchUpPolicy -count=1 +cd apps/web && pnpm run i18n:check && pnpm run i18n:ratchet +cd apps/web && pnpm vitest run app/office/routines +python3 scripts/lint-spec-files.py --all +``` + +## Files likely touched + +- `apps/backend/internal/office/models/catchup.go` +- `apps/web/app/office/routines/create-routine-dialog.tsx` +- `apps/web/app/office/routines/[id]/routine-detail-view.tsx` +- `apps/web/src/locales/*/office.json` +- `docs/specs/office/system-design/scheduler-01.md` +- `docs/specs/office/system-design/scheduler-02.md` + +## Dependencies + +Depends on task 01 for the `Routine.CatchUpPolicy` plumbing this rename reads +and writes. + +## Parallelism + +Sequential with task 01 (shared frontend control and Go enum). + +## Results + +Implemented with TDD: policy rename plus deprecated-alias normalization on +both write and read paths, upgrade migration for pre-existing rows, frontend +label update across both routine dialogs and all required locales (including +the "(wakes once)" qualifier added during PR review), and both scheduler +design docs corrected. `i18n:check` / `i18n:ratchet` clean; full frontend +vitest suite green. diff --git a/docs/specs/office/requirements/routine-catch-up.md b/docs/specs/office/requirements/routine-catch-up.md new file mode 100644 index 0000000000..7a69b12dba --- /dev/null +++ b/docs/specs/office/requirements/routine-catch-up.md @@ -0,0 +1,346 @@ +--- +status: draft +system: office +created: 2026-09-07 +owners: + - kandev +--- + +# Office Routine Catch-Up Requirements + +## Overview + +A routine's cron trigger can come due while the backend is not running to +process it. On resume its armed `next_run_at` is in the past, sometimes far in +the past. What the system does at that moment is a correctness property of an +unattended loop: it decides how much work resumes, how much is spent in the +first minute after restart, and what the prompt says. The launcher rejects +taskless runs, so lightweight runs may not reach an agent session. + +Four surfaces describe this moment, and they do not agree. + +| Surface | What it says | +|---|---| +| Policy value `enqueue_missed_with_cap` | missed ticks are enqueued, up to a cap | +| Field `catch_up_max`, default 25 | the cap is 25 | +| `scheduler-01.md` and `scheduler-02.md` | "fire missed ticks up to the cap (default 25)" | +| Web UI label `office:enqueueMissedWithCap` (5 locales) | "Enqueue missed (with cap)" | +| The code (`routines/service.go`) | counts up to 25 elapsed ticks, dispatches **exactly one** run | + +The code is silently authoritative. An operator sizing spend or work-in-progress +from the policy name, the field, the design doc, or the UI control would be +wrong by a factor of 25. + +This capability resolves the disagreement in favour of the code and makes +every other surface state it. **One resume produces one wake.** It also +supplies what the code lacks: a durable, observable account of the gap crossed, +so collapsing the backlog is a reported fact rather than a silent one. + +### Why one wake, and not twenty-five + +The decision is chosen here, not inherited from the code. Enqueueing up to +`catch_up_max` runs was rejected on four grounds: the idempotency key already +collapses a fan-out to one wake; the concurrency policy would then skip or merge +whatever survived; a fan-out creates a burst hazard before either bounding +control exists; and there is no per-tick work to replay. Each is verified +against the tree, with its mechanism and citations, in +[Why the fan-out was rejected](../system-design/routine-catch-up-01.md#why-the-fan-out-was-rejected). + +## Terminology + +- **Claim:** the atomic acquisition of a due cron trigger by one processor; + exactly one may hold a given claim. +- **Processing instant:** the time the scheduler uses for the tick in which a + claim is taken. +- **Gap:** the interval from the trigger's armed `next_run_at` at claim time up + to the processing instant. +- **Elapsed ticks:** how many times the trigger's cron expression matches within + the gap, including the tick that came due and is being dispatched now. +- **Missed ticks:** elapsed ticks minus one — the ticks that came due and will + never be dispatched. +- **Gap summary:** the durable record of a gap: missed-tick count, first-missed + timestamp, and whether the count was truncated. +- **Summarizing policy:** records the gap in the lightweight prompt, named + `summarize_missed` here. +- **Lightweight / heavy routine:** empty versus non-empty `task_template`, as + in [Office Scheduler](scheduler.md). + +## Requirements + +### REQ-OFFICE-ROUTINE-CATCHUP-001: One resume, one wake + +**Intent:** Freeze the resume behaviour as a stated contract rather than an +emergent property of a counting loop, and close the boundary cases the +implementation leaves to invention: ordering, concurrent claims, a failed +computation, a crash-orphaned trigger, and a dispatch that fails after arming. + +**As an** operator running an unattended workspace, **I want** a restart after +an outage to produce a bounded, predictable amount of agent work, **so that** a +long outage cannot turn into a burst of spend I did not authorize. + +#### Acceptance criteria + +- **AC-OFFICE-ROUTINE-CATCHUP-001.1:** When a due cron trigger is claimed, the + system shall dispatch exactly one routine run for that claim, whatever the + number of elapsed ticks in the gap and whatever the catch-up policy. The only + cases producing none are a failed arming write (AC-001.2), an unsatisfiable + expression (AC-001.11), and a run that cannot be created (AC-001.12); no case + produces two. +- **AC-OFFICE-ROUTINE-CATCHUP-001.2:** When a claim is processed and the + elapsed-tick computation succeeds, the system shall arm the trigger's + `next_run_at` to the first match of its cron expression strictly after the + processing instant. This shall hold identically under every catch-up policy. + The failure path is governed by AC-001.6 and AC-001.11 instead. When the + arming write itself fails, the system shall dispatch nothing for that claim + and leave `next_run_at` null for AC-001.9 to arm later; that tick is lost and + no routine run records it. +- **AC-OFFICE-ROUTINE-CATCHUP-001.3:** `catch_up_max` shall bound only the + number of elapsed ticks the system counts. It shall have no effect on the + number of runs, tasks, or wakeup requests created per claim, which is at most + one. +- **AC-OFFICE-ROUTINE-CATCHUP-001.4:** When the stored `catch_up_max` is less + than 1, the system shall use 25; when it is greater than 1000, it shall use + 1000. Both clamp rather than reject, so no stored value can stop a routine + firing. +- **AC-OFFICE-ROUTINE-CATCHUP-001.5:** When two processors evaluate the same + due trigger concurrently, exactly one shall take the claim and dispatch; + the other shall dispatch nothing, record no gap summary, and change no + trigger state. +- **AC-OFFICE-ROUTINE-CATCHUP-001.6:** When the elapsed-tick computation fails, + the system shall record no gap summary. For `ErrUnsatisfiableCron` it shall + leave `next_run_at` null and dispatch no run. For another failure it shall + arm `next_run_at` to the processing instant plus 24 hours and dispatch one + run. It shall warn with the underlying error. +- **AC-OFFICE-ROUTINE-CATCHUP-001.7:** Within one scheduler tick, due triggers + shall be processed in ascending `office_routine_triggers.next_run_at` order, + with ties broken by ascending `office_routine_triggers.id`. +- **AC-OFFICE-ROUTINE-CATCHUP-001.8:** When a cron trigger is created, the + system shall arm `next_run_at` to the first match of its expression strictly + after the creation instant, and shall reject the creation when that + expression is empty or when it or the timezone cannot be parsed. A trigger of + kind `cron` shall never be persisted with a null `next_run_at` at creation, + on every creation path, including one that writes the trigger row directly. + **No operation that mutates an existing trigger's cron expression, timezone, + or enabled flag exists** (see `## Out of scope`); changing any of the three is + a delete followed by a create, which arms by this same criterion. A recreated + trigger shall never report a gap spanning the interval in which it did not + exist, so a deliberate pause is never a gap. +- **AC-OFFICE-ROUTINE-CATCHUP-001.9:** When an enabled cron trigger has a null + `next_run_at`, a subsequent scheduler tick shall arm it to the first match + strictly after that tick's processing instant and shall not dispatch a run for + it, so that a process stopping between claiming a trigger and arming it does + not leave the trigger permanently unable to fire. The write shall be + conditional on `next_run_at` still being null when applied, so a concurrent + reconciliation is never overwritten: exactly one processor shall arm it, and + none shall dispatch by way of this path. A trigger whose claim is still in + flight shall not be armed by it. A trigger with `enabled` false shall not be + armed by this path. When the expression or timezone cannot be parsed, the + system shall leave `next_run_at` null, warn, and dispatch nothing; no + computation-failure fallback applies because no claim was taken and no run is + owed. +- **AC-OFFICE-ROUTINE-CATCHUP-001.10:** When a routine run has been created and + its materialisation then fails — the heavy path failing to create the task, or + the lightweight path failing to create or to dispatch the wakeup request — the + system shall set that run's status to `failed` and shall not re-dispatch it. A + wakeup request refused because its idempotency key already exists is **not** a + materialisation failure; that run's status shall be left as the outcome the + concurrency policy assigned. +- **AC-OFFICE-ROUTINE-CATCHUP-001.11:** When the elapsed-tick computation fails + after a claim, the system shall warn with the trigger and underlying error. + For `ErrUnsatisfiableCron`, it shall leave `next_run_at` null and dispatch no + run. For another failure, it shall arm `next_run_at` to the processing instant + plus 24 hours and dispatch one run. Neither path shall record a gap summary. +- **AC-OFFICE-ROUTINE-CATCHUP-001.12:** When the routine run for a claim cannot + be created at all, the system shall dispatch nothing and record no gap + summary, and shall not re-attempt that tick. The trigger remains armed forward + per AC-001.2 or AC-001.11, whichever governed the claim, or stays disarmed + for an unsatisfiable expression. The absence of a routine run is the only + record of that tick. +- **AC-OFFICE-ROUTINE-CATCHUP-001.13:** `catch_up_max` shall be normalized to + the value AC-001.4 defines before it is persisted, on every create and update + path, and rows written before this change shall be normalized once on upgrade. + A routine read back through the API, and every web UI control showing it, + shall report the same `catch_up_max` the tick honours. + +### REQ-OFFICE-ROUTINE-CATCHUP-002: The gap is reported, not discarded silently + +**Intent:** Answer what is discarded past the cap and whether it is recorded +anywhere. Nothing is discarded, because no run was ever going to be created for +a missed tick; what is bounded is the counting. The exact gap boundary is +preserved even when the count is truncated. + +**As an** assembled run after an outage, **I want** prompt context to state the +gap and whether its count is exact, **so that** an agent can choose what to do. + +#### Acceptance criteria + +- **AC-OFFICE-ROUTINE-CATCHUP-002.1:** When a claim's gap contains at least one + missed tick and the policy is `summarize_missed`, the system shall record a + gap summary on the routine run containing the missed-tick count, the + timestamp of the first missed tick, and whether the count was truncated. +- **AC-OFFICE-ROUTINE-CATCHUP-002.2:** The missed-tick count shall exclude the + tick being dispatched. When that count is zero, the system shall record no + gap summary, whatever the value of truncated. +- **AC-OFFICE-ROUTINE-CATCHUP-002.3:** When no gap summary is recorded, the + routine run shall carry no missed-tick field, and the agent's assembled + prompt shall contain no missed-tick statement. Absence shall not be + represented as a count of zero. +- **AC-OFFICE-ROUTINE-CATCHUP-002.4:** When `catch_up_max` is greater than 1 + and the elapsed-tick count reaches it while further ticks remain within the + gap, the system shall record the missed-tick count as `catch_up_max - 1`, + shall record truncated as true, and shall record the first-missed-tick + timestamp exactly and un-truncated. At a `catch_up_max` of 1 this criterion + does not apply and AC-002.11 governs. +- **AC-OFFICE-ROUTINE-CATCHUP-002.5:** When the routine is lightweight and the + policy is `summarize_missed` and a gap summary was recorded, the assembled + prompt for the run created by that claim shall state the missed-tick count and + the first-missed-tick timestamp, and, when truncated is true, shall state that + the count is a lower bound. +- **AC-OFFICE-ROUTINE-CATCHUP-002.6:** When the routine is heavy and the policy + is `summarize_missed` and a gap summary was recorded, it shall be readable + through the routine-run API. The created task's title and description shall be + exactly what the routine's template renders, unmodified by the gap. A heavy + routine under `skip_missed` records none, per AC-002.7. +- **AC-OFFICE-ROUTINE-CATCHUP-002.7:** When the policy is `skip_missed`, the + system shall record no gap summary and the assembled prompt shall contain no + missed-tick statement, whatever the number of elapsed ticks. +- **AC-OFFICE-ROUTINE-CATCHUP-002.8:** A recorded gap summary shall remain + readable after the routine run reaches a terminal status and after a backend + restart. +- **AC-OFFICE-ROUTINE-CATCHUP-002.9:** The system shall record the gap as + measured, without attributing a cause. A gap produced by a backend outage, a + suspended host, and a forward clock correction shall be recorded identically. +- **AC-OFFICE-ROUTINE-CATCHUP-002.10:** A gap summary shall be written exactly + once, as part of the creation of the routine run it belongs to, and shall not + be modified afterwards by any later status transition, including `skipped`, + `coalesced`, `failed`, `done`, and `cancelled`. When more than one routine run + is created for the same routine — two triggers due in the same tick, or a + manual fire alongside a cron fire — each run shall carry only the summary + measured for its own claim, and shall neither read nor overwrite another run's. + A wakeup request refused as an idempotency duplicate, or coalesced into an + in-flight run, shall not rewrite any gap summary, shall not alter the gap + statement that run's agent already carries, and shall not give a gap statement + to a run that had none. +- **AC-OFFICE-ROUTINE-CATCHUP-002.11:** When `catch_up_max` is 1, the + missed-tick count is zero for every gap, and the system shall therefore record + no gap summary for that routine however long the gap. Setting `catch_up_max` + to 1 is an opt-out of gap reporting. +- **AC-OFFICE-ROUTINE-CATCHUP-002.12:** A routine run created by a manual or a + webhook fire shall record no gap summary. Gap measurement applies only to a + cron trigger's armed `next_run_at`. + +### REQ-OFFICE-ROUTINE-CATCHUP-003: Every surface states the same behaviour + +**Intent:** `enqueue_missed_with_cap` is a false statement in an enum value, two +design documents, and a UI control in five locales. Renaming it is the +deliverable; accepting the old value forever is what makes the rename safe for +installs that already store it. + +**As an** operator reading the policy control, **I want** its name to describe +what happens, **so that** I do not size spend or work-in-progress from a name +that is 25 times wrong. + +#### Acceptance criteria + +- **AC-OFFICE-ROUTINE-CATCHUP-003.1:** The summarizing catch-up policy shall be + named `summarize_missed`. A routine created with no catch-up policy supplied + shall be persisted with `summarize_missed`, **whatever creation path is + used** — the HTTP create handler, config-sync reconciliation, the + pre-installed coordinator routine, or any direct repository insert. No + creation path shall persist the deprecated alias. +- **AC-OFFICE-ROUTINE-CATCHUP-003.2:** `enqueue_missed_with_cap` shall be + accepted on routine create and routine update as a deprecated alias for + `summarize_missed`, and shall be normalized to `summarize_missed` before + persistence. +- **AC-OFFICE-ROUTINE-CATCHUP-003.3:** A routine persisted before this change + with `catch_up_policy = 'enqueue_missed_with_cap'` shall read as + `summarize_missed`, and the column's stored default shall be + `summarize_missed` **on an upgraded install as well as on a fresh one**. + Inspecting the live schema of a database that predates this change shall not + show the retired value. +- **AC-OFFICE-ROUTINE-CATCHUP-003.4:** `enqueue_missed_with_cap` shall not + appear in any API response body or in any web UI control. +- **AC-OFFICE-ROUTINE-CATCHUP-003.5:** A stored `catch_up_policy` value that is + neither `summarize_missed`, nor `skip_missed`, nor the deprecated alias shall + be treated as `summarize_missed` and shall not prevent the routine from + firing. +- **AC-OFFICE-ROUTINE-CATCHUP-003.6:** The web UI shall label the summarizing + policy so that it states a single summarized wake, and shall label + `catch_up_max` so that it states the bound is on the number of ticks counted + and reported, not on the number of runs created. This copy shall be provided + in `en`, `pt-pt`, `zh-cn`, `zh-hk`, and `zh-tw`. +- **AC-OFFICE-ROUTINE-CATCHUP-003.7:** `system-design/scheduler-01.md` and + `system-design/scheduler-02.md` shall not describe missed cron ticks as + fired, enqueued, or dispatched as multiple runs, and shall reference this + requirement for catch-up semantics rather than restating them. Neither + document shall name `enqueue_missed_with_cap` as the catch-up policy value + except where it explicitly labels it a deprecated alias. Both shall state the + routine wakeup payload's catch-up fields wherever they describe that payload's + shape. The constraint is scoped to those two files; this document necessarily + quotes the retired value. +- **AC-OFFICE-ROUTINE-CATCHUP-003.8:** Under the summarizing policy the web UI + shall render the `catch_up_max` control and allow it to be edited, in both the + routine-create dialog and the routine-detail view. Under `skip_missed` it + shall not be rendered. The rename shall not make the control unreachable. + +## Out of scope + +Each exclusion is a decision, not an omission. + +- **Enqueueing N missed runs.** Rejected on the four grounds in the + Overview. Reopening it depends on work-in-progress enforcement and an enforced + budget ceiling landing first, and on the wakeup idempotency key gaining a + generation component. +- **Renaming `catch_up_max`.** Its meaning changes (a bound on counting, not on + runs) but the name stays accurate enough, and renaming the column, the API + field and the web state would double the migration surface for no change in + observable behaviour. The UI copy is corrected instead, under AC-003.6. Its + stored-versus-effective divergence is **not** deferred: AC-001.13 closes it. +- **A trigger-update operation.** There is none today: the trigger surface is + create, list, delete and webhook-fire, and the only mutation of an existing + trigger row is the scheduler's `next_run_at` re-arm. Editing a cron + expression, timezone or enabled flag therefore means delete-then-create. + Building the endpoint is a feature in its own right, including a decision + about what an in-flight claim does when the expression changes underneath it. + An update path added later inherits AC-001.8's rule: arm strictly after the + change instant. +- **Downgrade compatibility.** A payload carrying `summarize_missed` sent to a + build predating this change will be rejected by it; the alias is read-forward + only. No exported artifact carries it: the config-sync YAML has no catch-up + field. +- **A paused routine still fires.** `office_routines.status` is never consulted + in the cron tick path; only `office_routine_triggers.enabled` gates firing. A + defect, but not catch-up specific; a follow-up card carries it. +- **Cron correctness at the tick level.** Day-of-month versus day-of-week + conjunction and DST behaviour are gap 23, carded. A claimed unsatisfiable + expression follows AC-001.11; matching rules remain outside this document. +- **Work-in-progress limits and budget enforcement.** Gaps 11 and 19, carded. + This document creates no work that would need them. +- **Retention of gap summaries.** They inherit whatever retention + `office_routine_runs` gets, which is none today (gap 20, carded). + +## Prior art + +**Wiki leg — did not run.** Config resolved from `~/.obsidian-wiki/config.henry` +via the `@henry` pin, giving +`OBSIDIAN_VAULT_PATH=/Users/henry/Documents/henry/wiki`, collection `wiki`. +Reading the vault returns `EPERM` (macOS privacy protection on `~/Documents` +from this worktree), and neither `obsidian-wiki` nor `qmd` is on `PATH` or as an +MCP tool. An environment failure, not an empty result: the vault may hold a +position here and was not consulted. + +**saas-kb leg — did not run.** The `saas-kb` MCP server and its +`search_fsm_docs` tool are absent from this session; the `ai_sdlc` slice was not +queried. + +**What informed it instead:** the tree, and +`Forge/docs/kandev/office/BETA-REQUIREMENTS.md` gap 24 and section 7a. + +## References + +- [Office Scheduler](scheduler.md) — the wakeup pipeline this sits inside. +- Catch-Up System Design + [part 1](../system-design/routine-catch-up-01.md), + [part 2](../system-design/routine-catch-up-02.md) +- [Scheduler System Design part 1](../system-design/scheduler-01.md), + [part 2](../system-design/scheduler-02.md) — the documents AC-003.7 corrects. diff --git a/docs/specs/office/system-design/routine-catch-up-01.md b/docs/specs/office/system-design/routine-catch-up-01.md new file mode 100644 index 0000000000..552a234a03 --- /dev/null +++ b/docs/specs/office/system-design/routine-catch-up-01.md @@ -0,0 +1,522 @@ +--- +status: draft +system: office +requirements: + - REQ-OFFICE-ROUTINE-CATCHUP-001 + - REQ-OFFICE-ROUTINE-CATCHUP-002 +--- + +# Office Routine Catch-Up System Design, part 1: the tick and the gap + +## Purpose and boundaries + +This design makes the routine cron tick's resume behaviour explicit, gives the +gap it crosses a durable record, and carries that record into the assembled +prompt for eligible lightweight runs — REQ-001 and REQ-002. The existing shared +launcher rejects taskless runs, so prompt assembly is the boundary covered here; +actual taskless session delivery is a separate shared-scheduler contract. +Renaming the policy value that misdescribes all of it is REQ-003, in +[part 2](routine-catch-up-02.md). + +The Office system owns the outcome: the trigger table, the routine-run table, +the wakeup queue, and the routine prompt path are all Office primitives. + +Adjacent contracts read and constrained, but not owned: + +- `internal/scheduler/cron` — the shared 30-second cron loop that calls + `RoutineService.TickScheduledTriggers`. Its interval and handler ordering are + unchanged. +- `internal/office/shared/cron.go` — the cron evaluator. Its tick times are + taken as given; its day-of-month/day-of-week conjunction, DST behaviour, and + unsatisfiable-expression fallback are gap 23 and out of scope. +- `internal/office/wakeup` — the wakeup queue, its idempotency index, and the + dispatcher's concurrency policy. Unchanged except that `RoutinePayload` gains + two fields alongside the existing `MissedTicks`. + +## Requirement mapping + +| Requirement | Design section | +|---|---| +| REQ-OFFICE-ROUTINE-CATCHUP-001 | [The tick](#the-tick), [Normalizing catch_up_max](#normalizing-catch_up_max), [Ordering, claiming and re-arming](#ordering-claiming-and-re-arming), [Dispatch failure](#dispatch-failure) | +| REQ-OFFICE-ROUTINE-CATCHUP-002 | [The gap summary](#the-gap-summary), [Delivering the gap to the agent](#delivering-the-gap-to-the-agent) | +| REQ-OFFICE-ROUTINE-CATCHUP-003 | [part 2](routine-catch-up-02.md): Renaming the policy, Documentation consistency | + +## Current behaviour, as measured + +Recorded because these claims are load-bearing and none is written down today. + +- `processCronTrigger` (`internal/office/routines/service.go:360-397`) claims + the trigger, calls `computeRoutineMissed`, arms `next_run_at`, then calls + `DispatchRoutineRunWithMissed` **once**, passing `runCount - 1`. +- `computeRoutineMissed` (`:409-447`) walks `NextCronTime` from the armed + `next_run_at` until the cursor passes `now` or the count reaches + `routine.CatchUpMax` (default 25 when the stored value is not positive). + Nothing bounds `CatchUpMax` above: `routines/handler.go` assigns it from the + request unvalidated on both create and update, and the walk is one synchronous + `NextCronTime` call per counted tick inside the 30-second scheduler tick. +- The count reaches the wakeup request's payload as `missed_ticks` + (`marshalRoutinePayload`, `:665`). `createFreshRun` copies that payload onto + `runs.context_snapshot` **verbatim** (`wakeup/dispatcher.go:291`); the + coalesce paths do not, and `json_patch` it in instead, at **two separate SQL + sites** that do not share an implementation — see + [Delivering the gap to the agent](#delivering-the-gap-to-the-agent). +- **It stops there.** `buildPromptContext` is called with `run.Payload` + (`service/scheduler_integration.go:375`), and `createFreshRun` sets + `Payload: "{}"` on every wakeup-derived run. `prompt_builder.go` contains no + reader for `ContextSnapshot`. No web view renders `context_snapshot` either — + it is typed in `apps/web/lib/api/domains/office-runs-api.ts:113` and never + read. `missed_ticks` is written and never observed, by an agent or a human. + +Two further measurements shape the design: + +- `GetDueTriggers` (`repository/sqlite/routines.go:65-79`) has no `ORDER BY`. +- `ClaimTrigger` (`:84-96`) sets `next_run_at = NULL` as its CAS and stamps + `updated_at`; `UpdateTriggerNextRun` re-arms in a separate, unconditional + statement. A process that stops between the two leaves an enabled cron trigger + with a null `next_run_at`, which `GetDueTriggers` filters out — permanently. + That `updated_at` stamp is what lets the reconciliation pass tell a live claim + from an abandoned one. +- `models.RoutineRunStatusFailed` is declared in `models/enums.go` and set + nowhere in the tree. + +## Why the fan-out was rejected + +The requirement records the decision; this is the mechanism behind each of its +four grounds, verified against the tree. + +1. **It is not implementable without changing a different contract.** The wakeup + idempotency key is `routine:::` + (`buildRoutineIdempotencyKey`) and `agent_wakeup_requests.idempotency_key` + carries a partial unique index. N requests created inside one tick share one + wall-clock minute, so the second through Nth are rejected as idempotency + conflicts and absorbed as a warning. A naive fan-out silently produces one + wake anyway. Re-keying on scheduled tick time is the generation-identity work + tracked separately as gap 26. +2. **Whichever concurrency policy is in force would collapse them regardless.** + The shipped default is `skip_if_active` — both `routines/handler.go`'s create + path and the `office_routines.concurrency_policy` column default name it, and + it drops a request while a run is in flight. `coalesce_if_active`, which only + the pre-installed coordinator routine sets, merges the request into the + in-flight run instead. Either way N requests become one wake. A fan-out is + observable only under the third policy, `always_create` — that is the + routine-layer name an operator sets; the wakeup layer calls the same policy + `always_enqueue` after `dispatcher.go` maps it. +3. **Under `always_create` it would fan out into a system with no + work-in-progress limit and a budget check that fails open.** + `max_concurrent_sessions` is persisted and has no scheduler consumer; + `checkBudget` returns allow on checker error. Both are separately carded and + unshipped. Building the fan-out first builds the hazard before the two + controls that would bound it. +4. **There is no per-tick work to replay.** A lightweight routine's payload is + the routine id plus variables, and the time-valued built-ins (`{{date}}`, + `{{datetime}}`) are resolved at dispatch time, not at tick time. A replayed + missed tick would carry today's values and be indistinguishable from the + current one. + +## The tick + +`computeRoutineMissed` is replaced by a function returning one value object +instead of `(int, time.Time, error)`, so the caller receives the computation +error and handles trigger state explicitly: + +```go +type catchUpResult struct { + ElapsedTicks int // >= 1 on success; includes the tick due now + FirstMissedAt time.Time // zero when ElapsedTicks <= 1 + Truncated bool // ElapsedTicks reached the cap with ticks still pending + NextRunAt time.Time // success or recoverable-error re-arm + Unknown bool // the walk failed; ElapsedTicks is not meaningful +} +``` + +Invariants the constructor guarantees, so no call site has to restate them: + +- `NextRunAt` is strictly after the processing instant on a successful walk or + a recoverable failure. On `Unknown`, `computeCatchUp` supplies the + processing instant plus 24 hours. `ErrUnsatisfiableCron` leaves the claimed + trigger disarmed; another failure re-arms it and dispatches one run without a + gap summary. Both paths log the underlying error. +- `FirstMissedAt` is the armed `next_run_at` at claim time — read directly, not + derived from the walk, so it is exact even when `Truncated` is true. +- `Truncated` is true only when the walk stopped at the cap with the cursor + still at or before the processing instant. + +`missedTicks = ElapsedTicks - 1`, floored at zero. The catch-up policy decides +whether that number is *reported*, never whether runs are *created*: the tick +dispatches exactly one run either way. `skip_missed` and `summarize_missed` +therefore differ in exactly one observable: whether the eligible lightweight +run's assembled prompt includes the gap context. Whether a taskless run reaches +an agent session remains the separate shared-launcher contract described above. + +**A gap summary exists only when `missedTicks >= 1`.** This single rule collapses +what would otherwise be four near-identical states into two, and it is why +`Unknown` needs no representation in storage: + +- `Unknown` (the walk failed) drives the error path and no gap summary. The + caller either disarms an unsatisfiable trigger or re-arms a recoverable + failure for 24 hours and dispatches one run. A due trigger has at least one + elapsed tick but may have no *missed* one, so an unmeasured gap is a false + positive (AC-001.6). +- `catch_up_max == 1` makes `missedTicks` structurally zero, so that + configuration never produces a summary. That is a named consequence, not an + edge case to detect (AC-002.11). +- A truncated count is always at least `catch_up_max - 1`, so truncation and a + zero count cannot co-occur for any `catch_up_max` above 1. + +## Normalizing catch_up_max + +AC-001.4 clamps `catch_up_max` at **use** time, which is the fail-open safety +net: no stored value, however it got there, can stop a routine firing. On its +own that reintroduces the defect this whole capability exists to close, one +field over. A stored 5000 would keep being returned by `GetRoutine` / +`ListRoutines`, rendered in the routine-detail form, and edited there, while the +tick honoured 1000 — an operator sizing spend from the field would be wrong by +a factor of five, which is the Overview's disagreement table with a different +number in it. + +Note the divergence is not created by the upper bound; it exists today on the +lower one. `CreateRoutine` / `CreateRoutineTx` clamp `<= 0` to 25 at write time, +but `UpdateRoutine` (`repository/sqlite/routines.go:200`) writes the field with +no bound at all and `routines/handler.go:297` assigns it unvalidated, so a +stored 0 is reachable now and already reads back as 0 while the tick uses 25. +AC-001.4's upper bound extends an existing hole rather than digging a new one, +which is why AC-001.13 closes both ends at once. + +`CatchUpMax` is a plain `int` (`models/models.go:548`), not a named type, so the +`sql.Scanner` hook [part 2](routine-catch-up-02.md) uses for +`catch_up_policy` has nothing to attach to. Changing its type to get one would +ripple through the DTO, the JSON contract and the web state for no behavioural +gain — the same reasoning that keeps `## Out of scope`'s "Renaming +`catch_up_max`" excluded. + +**Normalize on write instead, which the policy field cannot do and this one +can — through one named funnel, applied at the repository.** +`models.NormaliseCatchUpMax(int) int` is the `catch_up_max` sibling of part 2's +`NormaliseCatchUpPolicy`. It applies AC-001.4's clamp and is the only place the +two bounds exist in Go, as named constants beside it. Today neither bound has a +single point of change: `defaultCatchUpMax = 25` lives only in +`routines/service.go:401` and no `1000` appears anywhere. + +**It is applied in the repository, not in the handlers, because the repository +is the only exhaustive choke point.** `catch_up_max` reaches SQL at exactly two +statements: `insertRoutine` (`repository/sqlite/routines.go:150`), the shared +INSERT that both `CreateRoutine` and `CreateRoutineTx` call, and `UpdateRoutine` +(:200). Config-sync's own update path, `updateRoutineConfigFields` (:243), does +not write the column at all. Normalizing in those two functions covers every +present and future writer by construction. + +Clamping in `routines/handler.go` instead would not, and the enumeration is the +whole point of this section: **two independent service layers reach the same +repository methods without passing through that handler** — +`office/service.Service.CreateRoutine` and `UpdateRoutine` (`service/service.go` +:713 and :732), the second of which `config_import.go`'s `applyRoutines` (:259) +calls for every imported routine. That is the same bypass shape that lost +`catch_up_policy` to `CreateRoutineTx`, and the same reasoning part 2 uses to put +the policy's read-side normalization in `sql.Scanner` rather than in each query. +The existing `<= 0 → 25` clamps in `CreateRoutine` and `CreateRoutineTx` are +removed in favour of the funnel call in `insertRoutine`, so the lower bound stops +being stated twice as well. + +Existing rows are corrected by one `r.migrate.Apply` step alongside the policy +migration. This SQL is the one place the bounds are restated outside the funnel, +so it carries the same two values by construction: + +```sql +UPDATE office_routines SET catch_up_max = 25 WHERE catch_up_max < 1; +UPDATE office_routines SET catch_up_max = 1000 WHERE catch_up_max > 1000; +``` + +The boundary is exclusive on both ends and the SQL says so: `< 1` and `> 1000`, +so a stored 1 and a stored 1000 are both left alone, matching AC-001.4's "less +than 1" and "greater than 1000" exactly. There is no null case to consider — +the column is `catch_up_max INTEGER NOT NULL DEFAULT 25` (`base.go:424`), so +every row matches one predicate or neither, and a `NULL` cannot silently escape +a comparison. Both statements are idempotent, and their order relative to the +`office_routines` table rebuild in [part 2](routine-catch-up-02.md) does not +matter: the rebuild copies column values verbatim, so normalizing before or +after it yields the same rows. + +Four consequences of clamping at the write, stated because a builder would +otherwise have to derive them. **The funnel reassigns `routine.CatchUpMax` on the +passed-in struct, before the statement binds that field**, rather than +normalizing the bind parameter alone — mutating +in place as the `CreateRoutine` clamp it replaces already does +(`routines.go:125-127`), with `UpdateRoutine` gaining the same behaviour. +Load-bearing, not stylistic: `routines/handler.go` serializes the SAME `*Routine` +it handed the service on both paths (`:95` create, `:136` update), so a bind-only +clamp would store 1000 while that one response reported the submitted 5000 — +AC-001.13's own divergence, reopened for one round-trip. +`NormaliseCatchUpMax` is a **fixpoint** — +normalizing an already-normalized value returns it unchanged — so applying it +twice is safe and a belt-and-braces call in a handler cannot corrupt anything. +Two concurrent updates of one routine row stay last-write-wins, unchanged by this +design, and both writers store a normalized value, so the row is in range +whichever lands second. And because the clamp sits on what the repository +*writes* rather than on what the request *carries*, an update that omits +`catch_up_max` altogether — it is `*int` with `omitempty` (`routines/dto.go:24`), +so omitting it means "leave it alone" — still rewrites the stored value +normalized. An out-of-range row is therefore corrected by the next edit routed +through `Repository.UpdateRoutine`, not only by the migration. It is **not** +corrected by config-sync's column-scoped update (`config/import.go`, +`configsync/reconcile_routines.go`), which does not write the column at all, so a +routine only ever touched that way waits for the migration. + +Write-side normalization plus a migration makes AC-001.4's use-time clamp +belt-and-braces rather than load-bearing, exactly as the `sql.Scanner` does for +the policy: a row that somehow escapes both still ticks correctly, it just also +now reads correctly. AC-001.4 is unchanged and still governs the tick. + +## Ordering, claiming and re-arming + +- `GetDueTriggers` gains `ORDER BY next_run_at ASC, id ASC`. `id` is the named + tiebreak column; it is a primary key, so the order is total and stable. +- Claiming is unchanged. The CAS on `next_run_at` already gives exactly one + winner; the loser returns before computing or recording anything. +- The claim-then-arm window is closed by a reconciliation pass in + `TickScheduledTriggers`: enabled cron triggers with a null `next_run_at` are + armed to the first match strictly after the processing instant and are **not** + dispatched in that tick. Re-arming rather than firing is deliberate: the row + cannot distinguish "crashed after claiming" from "crashed after claiming and + dispatching", and a spurious wake is the more expensive error. +- **The pass runs after the due-trigger loop, and only on stale rows.** A null + `next_run_at` is precisely what `ClaimTrigger` writes to take a claim, so a + live claim and an abandoned one are identical in the row. Without a second + predicate the pass would arm a trigger whose claimant is still mid-tick. + `ClaimTrigger` also sets `updated_at`, which is the discriminator: the pass + considers only rows whose `updated_at` is older than `catchUpReclaimAfter`, + **60 seconds** — two scheduler intervals, so a claim taken in this tick or the + one before is never reconciled, while an abandoned one is recovered on the + following tick. Running after the due-trigger loop means a trigger claimed and + armed within this same tick is already non-null and never reaches the pass. + This is what AC-001.9's "none shall dispatch **by way of this path**" scopes: + a live claimant's own dispatch is legitimate and unaffected. +- That reconciliation write is also a CAS. The `UPDATE` carries `WHERE id = ? + AND next_run_at IS NULL AND enabled = 1`, so a second reconciling processor + affects zero rows and returns, and a trigger disabled or armed between the + select and the write is left alone. Without the predicate two processors would + each compute their own `NextCronTime(expr, tz, now)` from slightly different + instants and the later write would silently move a trigger another had already + armed. Note what the CAS does **not** buy: it cannot protect against a live + claimant, whose own re-arm through `UpdateTriggerNextRun` is an unconditional + write that wins regardless. The staleness guard is what keeps the two paths + disjoint; the CAS only orders reconcilers among themselves. + `UpdateTriggerNextRun` stays unconditional and the reconciliation path needs a + conditional sibling rather than reusing it. +- Arming on create uses `NextCronTime(expr, tz, now)` and rejects the trigger + when that call errors (`CreateRoutineTrigger`, `service.go:305-314`). + **AC-001.8's invariant is enforced in the service: `CreateRoutineTrigger` is + the only place that may arm a cron trigger.** Today's guard is + `if t.Kind == "cron" && t.CronExpression != ""`, which lets an empty + expression fall through and persist a cron trigger with a null `next_run_at`; + dropping that second condition and rejecting the empty case is the change + AC-001.8 asks for. One caller reaches the repository directly — + `infra/reconcile.go`'s `createTriggersForNewRoutines` calls + `repo.CreateRoutineTrigger` past the service. It is named here because this + design already lost `catch_up_policy` to exactly this shape of bypass + (config-sync reaching `CreateRoutineTx`). It writes `Kind: "manual"` only, so + it cannot violate the cron invariant and needs no change; the rule it inherits + is that any future path creating a `cron` trigger goes through the service, + not the repository. **There is no trigger-update path to align with it.** The + repository interface exposes `CreateRoutineTrigger`, `UpdateTriggerNextRun` + (the scheduler's own re-arm, `next_run_at` only) and `DeleteRoutineTrigger`; + the HTTP surface is `GET`/`POST /routines/:id/triggers` and + `DELETE /routine-triggers/:triggerId` plus the webhook fire; and the web + client has only `deleteRoutineTrigger`. Changing a cron expression, timezone + or enabled flag is therefore a delete followed by a create, which arms from + the create instant and satisfies AC-001.8 with no new code. The update + endpoint is excluded in the requirement's `## Out of scope`, so this design + creates no trigger-mutation surface. + +## Dispatch failure + +AC-001.10 and AC-001.12 have no implementation to point at today: +`models.RoutineRunStatusFailed` is declared in `models/enums.go` and **set +nowhere in the tree**. Both dispatch paths lose their failures: + +- Heavy (`materialiseHeavyRoutineRun`) returns an error when + `EnsureRoutineWorkflow` or `CreateOfficeTaskInWorkflow` fails, without + touching run status, so the run sits at `received` forever. +- Lightweight (`materialiseLightweightRoutineRun`) logs a `Warn` and returns + `nil` for both `CreateWakeupRequest` and `Dispatch` failures, so the failure + never reaches `processCronTrigger` as an error at all. + +Three changes, in the order the tick executes: + +1. **`CreateRoutineRun` fails.** `dispatchRoutineRun` already returns + `(nil, err)` here, before any run row exists. There is nothing to mark and + nothing to re-attempt; the trigger was armed forward at the previous step, so + the tick is simply lost. AC-001.12 makes that the stated contract rather than + an accident. The caller logs and returns. +2. **The lightweight path stops swallowing.** `CreateWakeupRequest` and + `Dispatch` errors are returned rather than absorbed, with one exception: the + sentinel the office repository returns for an idempotency-key conflict stays + a no-op and is still absorbed as a `Warn`. That distinction is the whole + point of AC-001.10's second sentence — a duplicate is a successful dedup, not + a failed dispatch, and marking it `failed` would turn correct coalescing into + a red run in the UI. +3. **The status flip has one home.** `processCronTrigger` owns it: when + `DispatchRoutineRunWithMissed` returns a non-nil run together with an error, + it calls `UpdateRunStatus(run.ID, RoutineRunStatusFailed, "")`. Putting it in + the caller rather than in each materialise path means the heavy and + lightweight branches need no duplicate error handling, and the run is marked + exactly once whichever branch failed. + +The gap summary is untouched by any of this — it was written with the run row +(see below), so a run that later goes `failed` still carries the gap that was +measured for its tick. + +## The gap summary + +Stored on `office_routine_runs`, which both the lightweight and the heavy path +create — one durable surface for both routine shapes. Three columns, added via +`r.migrate.Apply` in `repository/sqlite/base_migrations.go` and to the +`CREATE TABLE` in `base.go:448`: + +| Column | Type | Meaning | +|---|---|---| +| `catch_up_missed_ticks` | `INTEGER` nullable | missed ticks; **NULL means no gap summary** | +| `catch_up_first_missed_at` | `TIMESTAMP` nullable | exact timestamp of the first missed tick | +| `catch_up_truncated` | `INTEGER NOT NULL DEFAULT 0` | 1 when the count is a lower bound | + +Nullable rather than `NOT NULL DEFAULT 0` because AC-002.3 requires absence to +be distinguishable from a measured zero. Per the rule above, a NULL +`catch_up_missed_ticks` always implies `catch_up_first_missed_at` is NULL and +`catch_up_truncated` is 0; the three columns are written together or not at all. + +Explicit columns rather than folding into `trigger_payload`: that column is the +resolved-variables contract, and a queryable "which routines resumed after a +gap" is the operability question gap 21 will need. + +The columns are written once, in the same repository call that creates the +routine run, before the concurrency policy is applied. A run that is +subsequently marked `skipped`, `coalesced` or `failed` therefore still carries +the gap that was measured — the gap is a property of the tick, not of the +dispatch outcome. + +That write-once property, not any claim about deduplication, is what satisfies +AC-002.10, and the ordering is worth stating because the obvious reading is +wrong: `dispatchRoutineRun` calls `CreateRoutineRun` **unconditionally and +first**, before `applyConcurrencyPolicy` and long before +`materialiseLightweightRoutineRun` inserts the wakeup request carrying the +idempotency key. `CreateRoutineRun` is neither gated by nor aware of the +`agent_wakeup_requests` unique index, so a wakeup rejected as a duplicate does +**not** prevent a second `office_routine_runs` row — it only prevents a second +wake. Two rows, each with its own measured summary, is the correct outcome: no +row is ever rewritten, because nothing writes these columns after the insert. + +For a cron claim two rows are hard to reach anyway — `ClaimTrigger` is a CAS on +`next_run_at`, the trigger is always re-armed strictly forward, and the +idempotency key is per *trigger* and per wall-clock minute. The reachable +multi-run cases are two triggers on one routine due in the same tick, and a +manual or webhook fire alongside a cron fire; AC-002.12 gives the latter no gap +summary at all. Note that AC-002.10 constrains the agent-run +`runs.context_snapshot` as well as these rows — see +[Delivering the gap to the agent](#delivering-the-gap-to-the-agent), where the +coalesce merge is the surface that can actually rewrite a delivered gap. + +They surface on the routine-run API DTO and are the source for any future UI. + +## Delivering the gap to the agent + +Three hops, of which only the last is new. + +1. `marshalRoutinePayload` already carries `missed_ticks`; it gains + `missed_since` (RFC 3339) and `missed_truncated`. `wakeup.RoutinePayload` + gains the matching fields. Both remain `omitempty`, so a gapless fire has a + payload byte-identical to today's. +2. **One payload column feeds two destinations, and only one of them may + carry the gap.** `createFreshRun` sets `ContextSnapshot: req.Payload` + verbatim (`wakeup/dispatcher.go:291`) and is unchanged — that is the path + AC-002.5 needs the three fields on. The **coalesce** paths merge the same + persisted `agent_wakeup_requests.payload` row into an existing run's + snapshot with `json_patch`, RFC 7386 merge-patch, so every top-level key the + incoming payload carries **overwrites** the one already there. Left alone + that gives two wrong outcomes: a second gap-carrying routine wake replaces + the first claim's measurement with its own, and because the three fields are + `omitempty`, a *gapless* later wake does not clear them, so the run keeps a + gap belonging to a tick it is not answering. + + **There are TWO merge sites, not one, and they do not share an + implementation.** `MarkWakeupRequestCoalesced` + (`repository/sqlite/wakeup_requests.go:168`) calls + `mergeWakeupPayloadIntoRunSnapshot` (:256, whose `json_patch` is at :265). + `PromoteRunAndCoalesceWakeupIfQueued` (:193) does **not** call that helper: + it carries its own inline copy of the identical `json_patch` statement at + :229-238, executed on `tx` rather than `r.db` because it runs inside the + promote transaction. `grep -n json_patch` over that file returns THREE lines: + these two SQL sites, plus the helper's own doc comment, which names the + function in prose and survives the unification. + Naming only the helper would send a builder to patch one of them, and the + one left unpatched is the promoted-run path — the same path + [part 2](routine-catch-up-02.md)'s test plan already singles out as the one + a naive implementation misses for the prompt gate. + + **The strip cannot live at the routine layer.** The routine layer writes the + payload once, at wakeup-request creation, and `agent_wakeup_requests` has a + single `payload` column. At that moment it is not yet known whether the + request will become a fresh run or be coalesced — the dispatcher decides + later, and both destinations read the same stored row. Stripping at + creation would strip the fresh-run path too, which AC-002.5 forbids; not + stripping leaves both merge sites carrying the keys. So the placement this + design previously asserted is not implementable, and the decision is + corrected here. + + **The strip happens at the merge, in exactly one place.** The two merge + sites are unified: the `json_patch` statement moves into a single helper + taking an executor (`sqlx.ExtContext`, satisfied by both `r.db` and `tx`), + and `PromoteRunAndCoalesceWakeupIfQueued` calls it inside its transaction + instead of duplicating the SQL. That refactor is what makes "one merge path" + a true statement rather than the false one this design shipped. The helper + patches `json_remove(payload, '$.missed_ticks', '$.missed_since', + '$.missed_truncated')` rather than `payload`. + + Putting three catch-up key names in an otherwise generic helper is the cost, + and it is the right trade against the alternative of a second payload column + read only by `createFreshRun`: `json_remove` of an absent key is a no-op in + SQLite (verified: `json_remove('{"a":1}', '$.missed_ticks')` returns + `{"a":1}`), so the helper stays byte-for-byte correct for every other wakeup + source, none of which writes these keys. A named constant holds the key list + beside the helper so the two never drift. + + The decision itself is unchanged: the three keys are written once, by the + wake that created the run, and no coalesced request adds, changes or removes + them (AC-002.10). Nothing is lost, because each claim's gap is already + durable on its own `office_routine_runs` row and readable through the + routine-run API (AC-002.6, AC-002.8). The assembled prompt carries one + coherent statement about the tick its run was created for, rather than a + silent mixture of two claims' measurements. +3. `buildPromptContext` is called with `run.Payload`, which is `"{}"` for every + wakeup-derived run. The call site at `scheduler_integration.go:375` passes + `run.ContextSnapshot` as an additional argument, and `buildPromptContext` + decodes it as `wakeup.RoutinePayload` when the run's reason is a routine + dispatch. **The constant to test is not `RunReasonRoutine`, which does not + exist.** `shared/runreasons.go` declares three: + `RunReasonRoutineDispatchCron`, `RunReasonRoutineDispatchEvent`, and the + legacy `RunReasonRoutineDispatch`. The decode gates on **all three**, not on + Cron alone. Cron alone is the tempting choice, since only a cron claim ever + measures a gap (AC-002.12), and it is wrong: + `PromoteRunAndCoalesceWakeupIfQueued` rewrites an in-flight run's `reason`, + so a cron-created run carrying a correctly measured gap can be holding the + Event reason by the time its prompt is built, and a Cron-only gate would drop + that gap silently. Gating on all three is safe in the other direction, + because a manual or webhook claim never writes the fields at all, so an + Event-reason run has nothing to render. `PromptContext` gains + `MissedTicks int`, `MissedSince string`, `MissedTruncated bool`, and + `BuildPrompt` renders a wake-context line when `MissedTicks > 0`. + +Scoping the decode to the routine reasons keeps every other run's prompt +byte-identical, which is what makes this change safe to land without a prompt +regression sweep across all reasons. + +The heavy path does not deliver the gap to the prompt: it creates a task from +the routine's template, and mutating a user's template text with scheduler +metadata is a worse contract than leaving the gap on the routine run where the +API exposes it (AC-002.6). + +## Renaming the policy, documentation and tests + +REQ-003's rename, the corrections to `scheduler-01.md` / `scheduler-02.md`, and +the test plan for both parts live in +[part 2](routine-catch-up-02.md). They are a separate deliverable: this part +changes what the system does on resume, part 2 changes what every surface says +about it. diff --git a/docs/specs/office/system-design/routine-catch-up-02.md b/docs/specs/office/system-design/routine-catch-up-02.md new file mode 100644 index 0000000000..1b4ba26397 --- /dev/null +++ b/docs/specs/office/system-design/routine-catch-up-02.md @@ -0,0 +1,291 @@ +--- +status: draft +system: office +requirements: + - REQ-OFFICE-ROUTINE-CATCHUP-003 +--- + +# Office Routine Catch-Up System Design, part 2: renaming the policy + +Part 1 ([routine-catch-up-01.md](routine-catch-up-01.md)) covers the tick, the +claim, the gap summary and its delivery to the agent — REQ-001 and REQ-002. +This part covers REQ-003 alone: renaming `enqueue_missed_with_cap` to +`summarize_missed` across every surface that names it, and the test plan for +both parts. + +The split is by deliverable, not by convenience. Part 1 changes what the system +*does* on resume; this part changes what every surface *says* about it, which is +a wider but shallower blast radius — a Go enum, six creation paths, a SQLite +table rebuild, two frontend files, five locale catalogs and two design +documents. + +## Renaming the policy + +`models.CatchUpPolicySummarizeMissed = "summarize_missed"` replaces +`CatchUpPolicyEnqueueMissedWithCap` as the canonical value. +`CatchUpPolicyEnqueueMissedWithCap` is retained as a deprecated alias constant, +not deleted, so the normalizer has a name to compare against. + +One normalization funnel, `models.NormaliseCatchUpPolicy(string) RoutineCatchUpPolicy`: + +| Input | Result | +|---|---| +| `summarize_missed` | `summarize_missed` | +| `enqueue_missed_with_cap` | `summarize_missed` (deprecated alias) | +| `skip_missed` | `skip_missed` | +| empty, or anything else | `summarize_missed` | + +`RoutineCatchUpPolicy.Valid()` accepts the alias so an API client sending the +old value gets 200, not 400; the persisted and returned value is the new one, +satisfying AC-003.2 and AC-003.4 together. + +**Where the funnel is applied.** AC-003.1 binds every creation path, not just +the HTTP one, so the enumeration has to be exhaustive: + +| Entry | Why it is on the list | +|---|---| +| `routines/handler.go` create | Sets its own default when the request omits the field | +| `routines/handler.go` update | Assigns the request value straight onto the routine | +| `Repository.CreateRoutine` | **Injects the literal old value** when the field is empty | +| `Repository.CreateRoutineTx` | Same injection, separate copy of the code | +| `CreateDefaultCoordinatorRoutine` | Hard-codes the old constant on the pre-installed routine | +| Read paths (`GetRoutine`, `ListRoutines`) | Normalize the alias and unknown values on the way out | + +The two repository entries are the ones a line-by-line reading of the HTTP layer +misses, and they are not hypothetical: config-sync's routine reconciler builds a +`models.Routine` with **no `CatchUpPolicy` set at all** and calls +`CreateRoutineTx` directly, bypassing the handler and its defaulting. Left +unchanged, every config-sync-created routine would keep writing the retired +string forever, after the one-time migration had cleaned up every pre-existing +row — a permanent AC-003.1 violation on a live path. + +**Read-side mechanism.** `GetRoutine` and `ListRoutines` are `SELECT *` through +`sqlx` `StructScan`/`SelectContext`; there is no post-scan hook to put the +normalizer in, so "the row scanner" is not an existing function to edit. +Implement `sql.Scanner` on `RoutineCatchUpPolicy` and normalize there. That +covers every present and future read path in one place, where per-call +post-processing would have to be added to each new query and would be forgotten +by exactly the same reasoning that produced this defect. `driver.Valuer` is not +needed: writes already go through the funnel above. + +Note the earlier claim that "Office routines are not part of the config-sync +YAML contract" is about the *field* — the YAML carries no catch-up policy — and +must not be read as "config-sync does not create routines". It does. + +**Migrating persisted rows and the stored default.** Two separate problems, and +one `UPDATE` only solves the first. + +Row *values* migrate with a single `r.migrate.Apply` step: +`UPDATE office_routines SET catch_up_policy = 'summarize_missed' WHERE catch_up_policy = 'enqueue_missed_with_cap'`. +With the `sql.Scanner` normalization above this is belt-and-braces rather than +load-bearing — a row that escapes it still reads correctly. + +The column's *stored default* needs more, and AC-003.3 requires it on upgraded +installs too. Editing the `DEFAULT 'enqueue_missed_with_cap'` text in +`createRoutineTables` fixes nothing on an existing database: the statement is +`CREATE TABLE IF NOT EXISTS` and is a no-op there, and SQLite has no +`ALTER COLUMN ... SET DEFAULT`. Changing it requires a table rebuild, following +the pattern this repository already documents (`apps/backend/AGENTS.md`, +"Table-rebuild migrations") and already uses in this very file — +`runTaskPriorityRecreate` in `base_migrations.go` is the worked precedent: +acquire a dedicated connection, `PRAGMA foreign_keys=OFF` with a deferred +re-enable, create `office_routines_new` with the corrected default, `INSERT ... +SELECT` every column, `DROP TABLE office_routines`, rename. + +The `foreign_keys=OFF` is not incidental. `office_routine_runs` declares +`FOREIGN KEY (routine_id) REFERENCES office_routines(id) ON DELETE CASCADE`, so +dropping the old table with enforcement on would cascade every routine run out +of existence. Mirror every column in the replacement `CREATE TABLE` and the copy +list, and add a replay regression test proving values and timestamps survive — +the run rows in particular. + +Leaving the default clause stale because no insert exercises it was rejected: an +operator reading an upgraded install's live schema would see a default naming a +value the system no longer treats as canonical, the same +disagreement-between-surfaces defect this capability exists to close. + +**Frontend.** Replace *every* occurrence of the `enqueue_missed_with_cap` +literal in `create-routine-dialog.tsx` and `routine-detail-view.tsx` — do not +work from a fixed line list, which is what produced the omission this design +previously shipped. There are four distinct kinds of site, and only two are +`SelectItem` values: + +| Kind | Effect if missed | +|---|---| +| `SelectItem value=` (both files) | The control writes the retired value | +| Create-dialog initial state default | New routines are created with the retired value | +| Detail-view fallback default | An unset policy renders as the retired value | +| `catchUpPolicy === "enqueue_missed_with_cap"` guards (both files) | **The `catch_up_max` input stops rendering entirely** | + +The last row is why AC-003.8 exists. Those two comparisons decide whether the +cap input is shown at all; once every routine reads as `summarize_missed` they +are permanently false, so a builder who updates only the option values and +defaults ships a UI where the cap field has silently vanished — while still +passing AC-003.4 and AC-003.6, neither of which observes the control. + +The i18n key `office:enqueueMissedWithCap` is replaced (not re-worded in place — +the key name would then lie too) by `office:summarizeMissed`, and +`office:beyondThisCountMissedTicksAre` is replaced by a key stating that the +bound is on the ticks counted and reported. Both across `en`, `pt-pt`, `zh-cn`, +`zh-hk`, `zh-tw`; use `pnpm run i18n:zh-hant` for the Traditional pair and +`pnpm run i18n:pseudo` to regenerate the pseudo catalog. No em dash in any +locale value. + +## Documentation consistency + +Three distinct corrections, and AC-003.7 requires all three. Work by **grepping +both files for `enqueue_missed_with_cap`, for "missed", and for `missed_ticks`, +and correcting every hit** — the lists below name the sites that exist today and +are illustrative, not a work list to tick off. This design shipped that mistake +once already, in the frontend notes above, and repeating it here would leave +the fourth surface disagreeing after a capability whose entire purpose is to +stop surfaces disagreeing. + +*Statements that imply multiple fires:* `scheduler-01.md`'s "Catch-up policy" +section ("fire missed ticks up to the cap"), `scheduler-02.md`'s failure-mode +table row, its restart scenario, and the retention paragraph's "catch-up cap +(default 25) drops missed routine ticks" sentence. Each is corrected to state +one summarized wake and to reference +[the requirement](../requirements/routine-catch-up.md) rather than restating its +terms. + +*Places that merely name the value:* `scheduler-01.md`'s routine-field list +(`catch_up_policy: enqueue_missed_with_cap (default, cap 25)`), its worked +routine-config example block, and `scheduler-02.md`'s schema column comment. +None claims multiple fires, so the first correction leaves them untouched — and +they would then name a policy value the system no longer accepts as canonical, +which is the exact class of defect this capability exists to remove. All become +`summarize_missed`, with the alias mentioned only where it is labelled +deprecated. + +*Places that document the routine wakeup payload's shape:* `scheduler-01.md`'s +run-reason table row (`{routine_id, variables, missed_ticks?}`) and +`scheduler-02.md`'s `RoutinePayload` struct listing, whose `MissedTicks` field +carries a "when catch-up cap collapsed N fires" comment. Neither names the +policy value nor claims multiple fires, so neither correction above reaches +them, yet both describe a contract this design extends with `missed_since` and +`missed_truncated`. Each gains the two new fields, and the `MissedTicks` comment +is corrected to say the count is of ticks *counted and reported*, never fired. + +`docs/specs/office/README.md` gains both new files in its specification map. + +## Testing + +Backend, in `internal/office/routines/`: + +- `catch_up_test.go` already covers the counting loop and the default cap; it is + extended for `FirstMissedAt` exactness under truncation, `Truncated`, the + `ElapsedTicks == 1` no-summary case, the failure path's strictly-after re-arm, + and both `catch_up_max` clamps — `0 -> 25` and `5000 -> 1000` (AC-001.4). The + existing tests referencing `CatchUpPolicyEnqueueMissedWithCap` move to the new + constant, with one test retained on the alias to prove normalization. +- The `catch_up_max == 1` boundary (AC-002.4, AC-002.11): a gap spanning many + ticks records **no** gap summary, and in particular does not record + `truncated = true` with a zero count. This is the case where the guard added + to AC-002.4 is the only thing standing between two ACs, so assert the absence + directly rather than inferring it from a count. +- A tick test asserting exactly one routine run and one wakeup request for a + gap spanning more than `catch_up_max` ticks — the AC-001.1 regression that + would have caught the original misnaming. +- A `GetDueTriggers` ordering test over rows with equal `next_run_at`. +- Reconciliation (AC-001.9): an enabled cron trigger with a null `next_run_at` + and a stale `updated_at` is armed and does not dispatch; a second reconciler + affects zero rows and does not move `next_run_at`; and a trigger whose + `updated_at` is inside `catchUpReclaimAfter` is left untouched, which is the + live-claim case the CAS alone does not cover. +- Re-arm failure (AC-001.2): `UpdateTriggerNextRun` returns an error, no routine + run is created for that claim, and a later tick's reconciliation arms the + trigger without dispatching. +- A malformed-expression case asserting the re-arm lands exactly 24 hours after + the processing instant and that a second tick a day later dispatches once + more, not repeatedly (AC-001.11). +- Dispatch-failure cases (AC-001.10): heavy-path task creation fails and the run + reads `failed`; lightweight `CreateWakeupRequest` fails and the run reads + `failed`; an idempotency-conflict sentinel does **not** mark the run failed. + And AC-001.12: `CreateRoutineRun` itself fails, no run row exists, and the + trigger stays armed forward. +- A write-once case for AC-002.10: a run carrying a gap summary is transitioned + through `coalesced` and `failed`, and the three columns are unchanged after + each. + +Wakeup, in `internal/office/wakeup/` and its repository — the coalesce merge +(AC-002.10). **Every case below runs against BOTH merge entry points**, +`MarkWakeupRequestCoalesced` and `PromoteRunAndCoalesceWakeupIfQueued`, as a +table test over the two. That is not belt-and-braces: until the refactor in +[part 1](routine-catch-up-01.md#delivering-the-gap-to-the-agent) lands they are +two separate SQL statements, and a suite that exercises only the helper passes +green with the promoted-run path fully broken — the exact failure the design +text previously invited. What the table does **not** do is catch a third merge +site: a table over two named entry points cannot observe a statement that is not +in it, so do not rely on it for that. The guard for that is a separate +source-level pinning assertion: that `json_patch` appears exactly once in +`wakeup_requests.go` **outside comments**, once the unification has landed. + +The qualifier is the whole assertion, not pedantry. `grep -c json_patch` on that +file returns **three** lines today, and only two of them are SQL: the third is the +helper's own doc comment, which names the function in prose and survives the +refactor untouched. An unqualified "exactly once" would therefore FAIL against a +correct implementation, and the builder would be left inventing the real +predicate. Assert on SQL occurrences — parse the file, or match the statement +rather than the bare identifier. + +The in-tree precedent for a test that reads Go source and asserts a structural +count is `internal/common/subproc/raw_git_audit_test.go`'s +`TestProductionGitCommandsUseTheAdmissionSeam`, which walks the tree with +`go/parser` and reports violations by file and line. It is **not** the auth +middleware allowlist: `apps/backend/CLAUDE.md` describes that one as a "pinning +test", but `auth/httpmw/middleware_test.go`'s `TestEnabledModeAllowlistMatrix` +issues real HTTP requests and asserts status codes per path — a behavioral +matrix, and the wrong shape to model this on. + +Two directions per entry point, and the second is the one `omitempty` alone +would pass for the wrong reason: a gap-carrying routine wake coalescing into a +gapless in-flight run leaves that run gapless, and a gapless wake coalescing +into a gap-carrying run leaves the original gap intact rather than +half-overwriting it. Add a third: a non-routine wakeup source whose payload +contains none of the three keys round-trips byte-identically through the merge, +which is what makes the `json_remove` safe to put in a shared helper. + +Repository: round-trip of the three columns including the NULL-versus-zero +distinction, in `routines_config_fields_test.go`'s neighbourhood. Plus a +default-injection test covering `CreateRoutine` and `CreateRoutineTx` with an +empty `CatchUpPolicy` (AC-003.1), and a replay regression test for the table +rebuild proving every column and timestamp survives and that +`office_routine_runs` rows are not cascaded away (AC-003.3). + +`catch_up_max` write normalization (AC-001.13) is tested on **every** path that +can currently store an out-of-range value, which is what makes the test +meaningful — and that is both repository statements, not only the update. +`CreateRoutine` and `CreateRoutineTx` clamp `<= 0 → 25` today and have no upper +bound at all, so a create carrying `5000` persists `5000`; the create paths are +therefore in scope by the same rule that puts the update path there. So: +`CreateRoutine`, `CreateRoutineTx` and `UpdateRoutine` each with `0` and with +`5000`, each read back at the clamped value rather than the submitted one, plus +the handler's create and update paths end to end — **asserting on the create and +update RESPONSE BODY itself, not only on a subsequent `GET`**. Those two +responses are the only place part 1's in-place-reassignment decision +([Normalizing catch_up_max](routine-catch-up-01.md#normalizing-catch_up_max)) is +observable: both handlers serialize the same `*Routine` they passed to the +service, so a test that POSTs `5000` and then re-reads the routine with `GET` +passes identically whether the funnel mutated the struct or only the SQL bind, +and would not catch the one divergence AC-001.13 exists to prevent. Cover +`office/service.Service.UpdateRoutine` as well — it is the second service layer +reaching the repository without the routines handler, and it is what +`config_import.go` calls, so it is the regression that would prove the clamp was +put in the handler instead of the repository. Plus a migration +replay proving a pre-existing row at `0` and one at `5000` are corrected in +place, and that a row already in range is left untouched — the read-back is the +assertion AC-001.13 actually names, so assert on the returned routine, not on +the tick's behaviour, which AC-001.4 already covers. + +Prompt: `BuildPromptContextForTest` cases asserting the wake context renders for +a run with a gap under **each** of the three routine reasons — including +`RunReasonRoutineDispatchEvent`, which is the promoted-run case a Cron-only gate +would silently drop (AC-002.5) — and that a non-routine reason with the same +`context_snapshot` renders nothing. + +Frontend: a store/API test that `summarize_missed` round-trips, a test that the +`catch_up_max` control renders under the summarizing policy and not under +`skip_missed` (AC-003.8), and `pnpm run i18n:check` for the five locales. + +E2E is proposed rather than assumed; the surfaces are listed in the task plan. diff --git a/docs/specs/office/system-design/scheduler-01.md b/docs/specs/office/system-design/scheduler-01.md index 45bd8aec06..aeb05902da 100644 --- a/docs/specs/office/system-design/scheduler-01.md +++ b/docs/specs/office/system-design/scheduler-01.md @@ -55,7 +55,7 @@ A SQLite-persisted queue of "wake this agent up" requests. Every periodic, event | Source | Trigger | Payload | |--------|---------|---------| -| `routine` | A routine's cron / webhook / manual trigger fires | `{routine_id, variables, missed_ticks?}` | +| `routine` | A routine's cron / webhook / manual trigger fires | `{routine_id, variables, missed_ticks?, missed_since?, missed_truncated?}` | | `comment` | Comment posted on a task assigned to this agent (non-self). Also the channel pathway: inbound Telegram/Slack messages become comments on a channel task. | `{task_id, comment_id}` | | `agent_error` | A sub-agent's session failed (escalation to coordinator) | `{failed_agent_id, failed_session_id?, run_id?, error}` | | `self` | Agent self-wake via tool call | `{reason, payload?}` | @@ -112,7 +112,7 @@ Routine fields: - `assignee_agent_instance_id` - who gets the resulting task / run. - `status`: `active` | `paused` | `archived`. - `concurrency_policy`: `coalesce_if_active` (default) | `skip_if_active` | `always_enqueue`. -- `catch_up_policy`: `enqueue_missed_with_cap` (default, cap 25) | `skip_missed`. +- `catch_up_policy`: `summarize_missed` (default, cap 25; `enqueue_missed_with_cap` is a deprecated alias) | `skip_missed`. - `catch_up_max`: integer, default 25. - `task_template`: JSON. Empty means **lightweight** routine (taskless run per fire). Non-empty means **heavy** routine (fresh task created on the `routine` workflow). - `variables`: declared template variables (type, default, required). @@ -144,9 +144,17 @@ Evaluated at dispatch by querying for an in-flight run for the same routine fing #### Catch-up policy -If the scheduler was down and missed cron ticks: -- `skip_missed`: fire only the current tick. -- `enqueue_missed_with_cap` (default, cap 25): fire missed ticks up to the cap; dropped ticks are not recorded individually but summarized into the next prompt's wake context ("you missed N ticks since X"). +If the scheduler was down and missed cron ticks, resuming always produces exactly +one run per due trigger, never one run per missed tick. `catch_up_policy` only +governs whether that one run's gap is measured and reported: +- `skip_missed`: no gap is recorded or reported. +- `summarize_missed` (default, cap 25; `enqueue_missed_with_cap` is a deprecated + alias): the gap (missed-tick count, first-missed timestamp, truncated flag) is + measured, capped at `catch_up_max`, and summarized into the run's wake context + ("you missed N ticks since X"). + +See [the routine catch-up requirement](../requirements/routine-catch-up.md) for +the full contract. #### The pre-installed coordinator routine @@ -158,7 +166,7 @@ description: "Wakes the coordinator every 5 minutes to check workspace a assignee_agent_id: status: active concurrency_policy: coalesce_if_active -catch_up_policy: enqueue_missed_with_cap +catch_up_policy: summarize_missed catch_up_max: 25 task_template: "" variables: [] diff --git a/docs/specs/office/system-design/scheduler-02.md b/docs/specs/office/system-design/scheduler-02.md index 12b8530d8e..23131716d0 100644 --- a/docs/specs/office/system-design/scheduler-02.md +++ b/docs/specs/office/system-design/scheduler-02.md @@ -71,7 +71,7 @@ description TEXT assignee_agent_id TEXT NOT NULL status TEXT NOT NULL -- active | paused | archived concurrency_policy TEXT NOT NULL -- coalesce_if_active | skip_if_active | always_enqueue -catch_up_policy TEXT NOT NULL -- enqueue_missed_with_cap | skip_missed +catch_up_policy TEXT NOT NULL -- summarize_missed (enqueue_missed_with_cap deprecated alias) | skip_missed catch_up_max INT NOT NULL DEFAULT 25 task_template TEXT NOT NULL DEFAULT '' -- JSON; empty -> lightweight variables TEXT NOT NULL DEFAULT '[]' -- JSON array @@ -148,9 +148,11 @@ last_wakeup_finished_at TIMESTAMP ```go type RoutinePayload struct { - RoutineID string `json:"routine_id"` - Variables map[string]any `json:"variables,omitempty"` - MissedTicks int `json:"missed_ticks,omitempty"` // when catch-up cap collapsed N fires + RoutineID string `json:"routine_id"` + Variables map[string]any `json:"variables,omitempty"` + MissedTicks int `json:"missed_ticks,omitempty"` // ticks counted and reported for this claim's gap, excluding the tick dispatched + MissedSince string `json:"missed_since,omitempty"` // RFC3339 timestamp of the first missed tick + MissedTruncated bool `json:"missed_truncated,omitempty"` // true when the gap exceeded catch_up_max } type CommentPayload struct { TaskID string `json:"task_id"` @@ -287,7 +289,7 @@ A formal per-route authorization model (workspace membership, admin role, RBAC o | Atomic checkout finds task locked by another agent | Wakeup skipped, no retry. | | Summary builder fails on successful taskless run | Previous summary left intact; failure logged; next successful run rebuilds. | | Coordinator's pre-installed routine fails to install at onboarding | Logged + warned; coordinator's agent detail UI shows "no scheduled wake-ups" empty state. User can install one manually. | -| Routine cron tick missed (scheduler down) | `enqueue_missed_with_cap` (default): fire missed ticks up to cap=25 with "missed N ticks" in next prompt context. `skip_missed`: fire current tick only. | +| Routine cron tick missed (scheduler down) | Resume always produces exactly one run, never one per missed tick. `summarize_missed` (default, cap 25): the gap is measured and summarized as "missed N ticks" in the next prompt context. `skip_missed`: no gap is recorded or reported. See [the routine catch-up requirement](../requirements/routine-catch-up.md). | | Webhook signature verification fails | Trigger rejected with 401; no routine run created. | | Graceful shutdown begins | Queue and cron loops cancel and join before repositories and SQLite close; no scheduler logs `database is closed`. | @@ -297,7 +299,7 @@ Survives a kandev process restart: all `agent_wakeup_requests` rows including `q Does NOT survive (reconstructed on next tick): in-memory claim leases - a `claimed` wakeup whose process died is picked up by the staleness/recovery path; the scheduler's claim query is the source of truth. The unstarted-task recovery sweep suppresses duplicates with its `NOT EXISTS` check on `runs`: any queued, claimed, or finished run of any age blocks redispatch, while failed and cancelled runs do not. -Retention: the idempotency lookup window is 24 hours, while persisted idempotency keys remain unique; summary cap 8 KB per row; routine run history retained for inspection (no automatic prune in scope here); catch-up cap (default 25) drops missed routine ticks beyond it (not recorded individually). A live linked task remains in the concurrency gate regardless of age. +Retention: the idempotency lookup window is 24 hours, while persisted idempotency keys remain unique; summary cap 8 KB per row; routine run history retained for inspection (no automatic prune in scope here); catch-up cap (default 25) bounds only how many missed ticks are counted and reported in the gap summary, never how many runs are dispatched — see [the routine catch-up requirement](../requirements/routine-catch-up.md). A live linked task remains in the concurrency gate regardless of age. The scheduler reads all `queued` and unexpired-retry wakeup requests on boot and resumes processing them. @@ -326,7 +328,7 @@ The scheduler reads all `queued` and unexpired-retry wakeup requests on boot and - **GIVEN** a routine with a webhook trigger and `signing_mode=hmac_sha256`, **WHEN** an external system POSTs to the trigger URL with valid signature and payload `{"branch": "release/2.0"}`, **THEN** the routine fires with `{{branch}}` resolved to "release/2.0" in the task template. -- **GIVEN** the scheduler was down for 3 hours, **WHEN** it restarts, **THEN** routines with `catch_up_policy=skip_missed` fire only the current tick; routines with `enqueue_missed_with_cap` fire missed ticks up to the cap (default 25), with overflow summarized as "missed N ticks" in the next prompt context. +- **GIVEN** the scheduler was down for 3 hours, **WHEN** it restarts, **THEN** each due trigger produces exactly one run, regardless of `catch_up_policy`; routines with `catch_up_policy=skip_missed` record no gap; routines with `catch_up_policy=summarize_missed` (default) record the gap, capped at `catch_up_max` (default 25), summarized as "missed N ticks" in the next prompt context. See [the routine catch-up requirement](../requirements/routine-catch-up.md). - **GIVEN** a user on the routines page, **WHEN** they click "Run Now" on a routine with a required `{{reason}}` variable, **THEN** a modal prompts for the variable value before firing.