feat(office): report routine schedule state and harden coordinator install (#3763)
Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
This commit is contained in:
Henry
@@ -0,0 +1,180 @@
|
||||
---
|
||||
requirements:
|
||||
- REQ-OFFICE-ROUTINE-ARMING-001
|
||||
- REQ-OFFICE-ROUTINE-ARMING-002
|
||||
- REQ-OFFICE-ROUTINE-ARMING-003
|
||||
- REQ-OFFICE-COORDINATOR-INSTALL-001
|
||||
system_design:
|
||||
- ../../specs/office/system-design/routine-schedule-state.md
|
||||
- ../../specs/office/system-design/coordinator-install-idempotency.md
|
||||
created: 2026-09-17
|
||||
status: done
|
||||
---
|
||||
|
||||
# Implementation Plan: Office Routine Arming Visibility and Coordinator Install Idempotency
|
||||
|
||||
## Overview
|
||||
|
||||
An Office routine has two independent switches — intent (`office_routines.status`)
|
||||
and schedule state (whether its triggers can actually fire it) — and the product
|
||||
today reports only intent. This plan makes schedule state visible everywhere
|
||||
intent already is (routine reads, an unattended-install startup scan) and closes
|
||||
the one identity defect that lets onboarding duplicate the pre-installed
|
||||
coordinator routine. It repairs nothing: no criterion in any of the three
|
||||
requirement documents enables, disables, creates, or deletes a trigger, and none
|
||||
changes the existing routine-status dispatch gates. The scheduler already checks
|
||||
status before it claims a cron slot, and the manual and webhook paths reject
|
||||
non-firing statuses; this plan only makes the independent schedule state visible.
|
||||
|
||||
Two system-design documents back this plan:
|
||||
[Routine Schedule State](../../specs/office/system-design/routine-schedule-state.md)
|
||||
covers REQ-OFFICE-ROUTINE-ARMING-001/002/003 (classification, the read-path
|
||||
report, and the startup scan — one vertical feature contract split across two
|
||||
requirement documents by scope), and
|
||||
[Coordinator Install Idempotency](../../specs/office/system-design/coordinator-install-idempotency.md)
|
||||
covers REQ-OFFICE-COORDINATOR-INSTALL-001. Both were written after Build,
|
||||
grounded directly in the merged implementation, rather than before it — the
|
||||
three requirement documents already pinned every table, column, function, and
|
||||
endpoint this plan touches at file-and-line precision, so drafting the designs
|
||||
speculatively ahead of code would have either duplicated that text or drifted
|
||||
from it. This plan itself still carries the remaining architecture decisions —
|
||||
package placement, response shape, verification boundaries — that the
|
||||
requirements deliberately left to Build.
|
||||
|
||||
Work lands in three waves: classification is the foundation everything else
|
||||
reads (Wave 1); the read-path report, the startup scan, and the coordinator
|
||||
install fix are three independent consumers of it and can proceed in parallel
|
||||
(Wave 2); the frontend surface depends on the backend response shape Wave 2
|
||||
defines (Wave 3).
|
||||
|
||||
## Code facts confirmed on this branch (2026-09-17)
|
||||
|
||||
All paths relative to `apps/backend`.
|
||||
|
||||
- **Schema.** `office_routines` and `office_routine_triggers` are defined in
|
||||
`internal/office/repository/sqlite/base.go`, `createRoutineTables`
|
||||
(L414-467) — one dialect-agnostic file for both SQLite and PostgreSQL via
|
||||
`internal/db/dialect`. There is no separate PostgreSQL schema file.
|
||||
- **Trigger reads today.** `internal/office/repository/sqlite/routines.go`:
|
||||
`GetDueTriggers` (L65-78) filters `kind='cron' AND enabled=1 AND
|
||||
next_run_at IS NOT NULL AND next_run_at <= ?` with no `ORDER BY` and does not
|
||||
read `office_routines.status`. `RoutineService.processCronTrigger` reads the
|
||||
routine and applies its status gate before `ClaimTrigger`; the query is only
|
||||
the first stage of that path. `ClaimTrigger` and `UpdateTriggerNextRun` remain
|
||||
the trigger-state writers.
|
||||
- **Coordinator install today.** `internal/office/routines/service.go`:
|
||||
`CreateDefaultCoordinatorRoutine` (L145-196) calls `findCoordinatorRoutine`
|
||||
(L197-217), which matches workspace + assignee + canonical name +
|
||||
`hasCronTrigger` (L222-239, ignores `enabled`, matches on kind + exact
|
||||
expression only). Its two production callers —
|
||||
`internal/office/agents/service.go:572` and
|
||||
`internal/office/onboarding/service.go:767` — both discard the returned
|
||||
routine and only warn-log the error (`if _, err := ...`), which is why
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.10 forbids reporting through a return
|
||||
value.
|
||||
- **Cron evaluation.** `internal/office/shared/cron.go`: `NextCronTime`
|
||||
validates the timezone, requires exactly 5 fields, follows the shared
|
||||
day-of-month/day-of-week OR and DST rules, and returns `ErrUnsatisfiableCron`
|
||||
when an otherwise valid expression has no possible occurrence. Schedule
|
||||
classification uses the same occurrence path.
|
||||
- **Startup sequencing.** `internal/office/infra/reconcile.go`:
|
||||
`Reconciler.ReconcileAll` (L34-54) is synchronous with a void return;
|
||||
`createTriggersForNewRoutines` (L113-132) gives every trigger-less routine
|
||||
a `manual` trigger. Called from `internal/backendapp/main.go:1378-1380`,
|
||||
immediately followed by `log.Info("Office reconciliation complete")` and,
|
||||
a few lines later (~L1393), `services.Office.RegisterEventSubscribers`.
|
||||
That gap is the wiring point for the startup-scan completion signal
|
||||
required by AC-OFFICE-ROUTINE-ARMING-003.1 — nothing today observes
|
||||
`ReconcileAll` returning.
|
||||
- **The actual routine list/detail response.**
|
||||
`internal/office/routines/handler.go`'s `listRoutines` (L45-52) and
|
||||
`getRoutine` (L98-105) back `GET /workspaces/:wsId/routines` and
|
||||
`GET /routines/:id`, returning `RoutineListResponse`/`RoutineResponse`
|
||||
(`internal/office/routines/dto.go:34-41`) — today just `*Routine`
|
||||
(intent only). This, not the dashboard, is Task 02's target.
|
||||
Separately, `internal/office/dashboard/service_agents.go`'s
|
||||
`ListRoutines` block inside `runSoftQueries` (~L551-561) only counts
|
||||
`status == "active"` into the workspace KPI tile's `RoutineCount` and is
|
||||
not in scope.
|
||||
- **Trigger HTTP routes**, `internal/office/routines/handler.go`,
|
||||
`RegisterRoutes` (L30-42): `GET`/`POST /routines/:id/triggers`,
|
||||
`DELETE /routine-triggers/:triggerId`,
|
||||
`POST /routine-triggers/:publicId/fire`. No route edits, enables, or
|
||||
disables a trigger — confirms `## Out of scope` → *Disabling a trigger from
|
||||
the UI* in the visibility document.
|
||||
- **Metrics prior art.** `internal/orchestrator/office_stall_metrics.go`
|
||||
(`expvar.NewMap` + label builder, L20-46) and
|
||||
`internal/workflow/signalmetrics/` are the two labelled-counter
|
||||
conventions already in the codebase; this plan's new counters follow the
|
||||
same shape rather than inventing a third.
|
||||
- **Persistence conformance.** `internal/persistence/requiredstores` already
|
||||
registers office's schema owner (`catalog.go:52`) and
|
||||
`internal/persistence/storeconformance` already has a fixed office
|
||||
adapter (`adapters.go:224-229`). No new table is required by any
|
||||
criterion in these three documents; Task 05's serialization mechanism must
|
||||
be chosen so that stays true (see its risks).
|
||||
|
||||
## Waves
|
||||
|
||||
### Wave 1 — classify
|
||||
|
||||
- [x] [Task 01: Classify a routine's schedule state](task-01-classify-schedule-state.md)
|
||||
|
||||
### Wave 2 — three independent consumers of classification
|
||||
|
||||
- [x] [Task 02: Report intent and schedule state on routine reads (API)](task-02-report-schedule-state-api.md)
|
||||
- [x] [Task 04: Surface unarmed routines at startup](task-04-startup-scan.md)
|
||||
- [x] [Task 05: Match the coordinator routine on identity, not mutable trigger state](task-05-coordinator-install-idempotency.md)
|
||||
|
||||
### Wave 3 — frontend
|
||||
|
||||
- [x] [Task 03: Render the intent/schedule-state distinction in the routines UI](task-03-report-schedule-state-ui.md)
|
||||
|
||||
Tasks 02, 04, and 05 touch disjoint files and have independent verification
|
||||
boundaries; once Task 01 lands they may proceed in parallel. Task 03 depends
|
||||
only on Task 02's response shape, not on Task 04 or Task 05.
|
||||
|
||||
## Required workflow verification
|
||||
|
||||
After all task-level checks pass:
|
||||
|
||||
1. `cd apps/backend && gofmt -l $(git diff --name-only main -- '*.go')` — must
|
||||
be empty.
|
||||
2. `make -C apps/backend test` (full suite; the office and persistence
|
||||
packages are the ones this plan can regress).
|
||||
3. `make -C apps/backend lint`.
|
||||
4. If Task 05 adds a schema owner (see its risks):
|
||||
`go run ./cmd/sqlguard ./internal` and
|
||||
`go test -race ./internal/persistence/storeconformance -count=1` from
|
||||
`apps/backend`.
|
||||
5. `cd apps/web && pnpm run typecheck && pnpm run lint`.
|
||||
6. `cd apps/web && pnpm run i18n:check`.
|
||||
7. Playwright coverage per Task 03 (`cd apps/web && pnpm e2e:run`, scoped to
|
||||
the affected spec — see that task).
|
||||
8. Commit the explicit changed paths with a Conventional Commit message.
|
||||
|
||||
## Risks and non-goals
|
||||
|
||||
- **Keep repairs separate from visibility.** The operator-initiated re-arm
|
||||
action remains outside this plan. A work order that starts writing `enabled`
|
||||
or `next_run_at` outside Task 05's coordinator-install path has drifted from
|
||||
scope. Schedule classification must continue to report an impossible cron
|
||||
expression as `trigger_invalid`, matching `NextCronTime` and the scheduler's
|
||||
permanent-disarm behavior.
|
||||
- **Dispatch grace is one constant, not two.** AC-OFFICE-ROUTINE-ARMING-001.10
|
||||
requires classification (Task 01) and the startup scan (Task 04) to share
|
||||
the exact same 60-second compile-time constant. Define it once in Task 01
|
||||
and import it in Task 04; do not redeclare it.
|
||||
- **Task 05's serialization mechanism is Build's choice, but three
|
||||
candidates are pre-ruled-out in the requirement's own `## Out of scope`:**
|
||||
a bare default-isolation transaction (both callers can still both read
|
||||
"absent"), a PostgreSQL advisory lock (no SQLite equivalent), and an
|
||||
in-process-only guard (fails the documented cross-process domain). See
|
||||
Task 05 for the constraint set the chosen mechanism must satisfy.
|
||||
- **No coverage percentage is a project gate.** New tests must assert the
|
||||
specific acceptance criteria named in each task, not a line-coverage
|
||||
target.
|
||||
- **Translation completeness.** Task 03's copy must exist in all five
|
||||
shipped locales (`pt-pt`, `zh-cn`, `zh-hk`, `zh-tw`, plus source English)
|
||||
before `pnpm run i18n:check` passes; use `pnpm run i18n:zh-hant` for the
|
||||
Traditional Chinese pair rather than hand-translating.
|
||||
@@ -0,0 +1,125 @@
|
||||
---
|
||||
id: "01-classify-schedule-state"
|
||||
title: "Classify a routine's schedule state"
|
||||
status: done
|
||||
wave: 1
|
||||
depends_on: []
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-OFFICE-ROUTINE-ARMING-001
|
||||
acceptance_criteria:
|
||||
- AC-OFFICE-ROUTINE-ARMING-001.1
|
||||
- AC-OFFICE-ROUTINE-ARMING-001.2
|
||||
- AC-OFFICE-ROUTINE-ARMING-001.3
|
||||
- AC-OFFICE-ROUTINE-ARMING-001.4
|
||||
- AC-OFFICE-ROUTINE-ARMING-001.5
|
||||
- AC-OFFICE-ROUTINE-ARMING-001.6
|
||||
- AC-OFFICE-ROUTINE-ARMING-001.7
|
||||
- AC-OFFICE-ROUTINE-ARMING-001.8
|
||||
- AC-OFFICE-ROUTINE-ARMING-001.9
|
||||
- AC-OFFICE-ROUTINE-ARMING-001.10
|
||||
- AC-OFFICE-ROUTINE-ARMING-001.11
|
||||
- AC-OFFICE-ROUTINE-ARMING-001.12
|
||||
- AC-OFFICE-ROUTINE-ARMING-001.13
|
||||
system_design:
|
||||
- ../../specs/office/system-design/routine-schedule-state.md
|
||||
---
|
||||
|
||||
# Task 01: Classify a routine's schedule state
|
||||
|
||||
Satisfies REQ-OFFICE-ROUTINE-ARMING-001 (AC-OFFICE-ROUTINE-ARMING-001.1 through
|
||||
.13).
|
||||
|
||||
## Scope
|
||||
|
||||
A pure, total classification of one routine's schedule state from its trigger
|
||||
rows alone, plus the batched-read-with-per-routine-fallback semantics that
|
||||
AC-001.12 and AC-001.13 require, plus the unarmed-cron-trigger list AC-001.8
|
||||
requires alongside it. This task defines the dispatch-grace constant
|
||||
(AC-001.10) that Task 04 (startup scan) also consumes — define it once here.
|
||||
|
||||
## Exclusions
|
||||
|
||||
- No HTTP response shape, no dashboard change, no UI (Task 02/03).
|
||||
- No startup scan (Task 04).
|
||||
- No coordinator-install change (Task 05) — it consumes this task's output but
|
||||
this task does not touch `routines/service.go`'s coordinator-install path.
|
||||
- No repair: this task's function set never writes `office_routine_triggers`
|
||||
or `office_routines`.
|
||||
|
||||
## Acceptance conditions
|
||||
|
||||
1. `ClassifyRoutine(triggers []models.RoutineTrigger, now time.Time) (ScheduleState, []UnarmedCronTrigger)`
|
||||
(exact name/package left to Build; suggested home:
|
||||
`internal/office/routines`, since that package already owns trigger
|
||||
dispatch logic and is already imported by `office/agents` and
|
||||
`office/onboarding`) implements the nine-rule table in rule order,
|
||||
stopping at first match (AC-001.1), never reads `office_routines.status`
|
||||
(AC-001.2), and treats a past-due or exactly-at-`now` `next_run_at` as
|
||||
rule 1 (AC-001.3, AC-001.4). It returns every applicable unarmed-cron
|
||||
reason per trigger, in the fixed order (`enabled=false`, then
|
||||
"not schedulable", then "stalled past dispatch grace"), in trigger order
|
||||
(`created_at` ascending, `id` ascending) (AC-001.8, AC-001.9, AC-001.11).
|
||||
A `dispatchGrace` constant of exactly `60 * time.Second`, unexported and
|
||||
with no environment/config/flag override, backs rule 2 and the unarmed
|
||||
list's third reason (AC-001.10).
|
||||
2. A batch classification entry point reads every named routine's trigger
|
||||
rows in one query, captures one `now` for the whole call, and on a batch
|
||||
read failure (or one undecodable row) re-reads exactly the affected
|
||||
routines individually — once each — reporting `unknown` only for those
|
||||
whose individual re-read also fails, reusing the batch call's captured
|
||||
`now` rather than recapturing it per routine (AC-001.5, AC-001.12,
|
||||
AC-001.13). A routine whose reads never fail counts as one query
|
||||
contribution regardless of how many other routines are classified in the
|
||||
same call (feeds Task 02's AC-002.7 query-count bound).
|
||||
3. Table-driven tests cover: every one of the 9 rules in isolation; the
|
||||
rule-1/rule-9 interaction of AC-001.9 (armed on one trigger, unarmed list
|
||||
non-empty from a sibling); the never-fired exclusion of AC-001.11; the
|
||||
batch-then-per-routine-fallback path of AC-001.13 with a fixture that
|
||||
forces one row in a multi-routine batch to fail decode; and identical
|
||||
output on SQLite and PostgreSQL for the same rows (AC-001.6, via the
|
||||
`storeconformance`/`KANDEV_TEST_POSTGRES_DSN` pattern already used
|
||||
elsewhere in this package's tests).
|
||||
|
||||
## Verification
|
||||
|
||||
- `cd apps/backend && go test -tags fts5 ./internal/office/routines/... ./internal/office/repository/sqlite/...`
|
||||
- New tests must first fail against the current tree, where no classifier
|
||||
exists.
|
||||
- `KANDEV_TEST_POSTGRES_DSN=<dsn> go test -tags fts5 ./internal/office/repository/sqlite/... -run TestClassify` (or
|
||||
equivalent name) for the PostgreSQL leg of AC-001.6.
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- new: `apps/backend/internal/office/routines/arming.go`
|
||||
- new: `apps/backend/internal/office/routines/arming_test.go`
|
||||
- `apps/backend/internal/office/repository/sqlite/routines.go` — add the
|
||||
batch trigger-read query and the single-routine fallback read.
|
||||
- `apps/backend/internal/office/repository/sqlite/routines_test.go`
|
||||
|
||||
## Dependencies
|
||||
|
||||
None. This is the foundation for Tasks 02, 04, and 05.
|
||||
|
||||
## Parallelism
|
||||
|
||||
Must land before Wave 2. Nothing in Wave 1 can run in parallel with it.
|
||||
|
||||
## Result
|
||||
|
||||
Implemented in `internal/office/routines/arming.go` (unexported `dispatchGrace`,
|
||||
`ClassifyRoutine`, `ClassifyRoutines`, `RoutineTriggerReader`) plus
|
||||
`ListTriggersByRoutineIDs` and an `, id` trigger-order tiebreak on
|
||||
`ListTriggersByRoutineID` in `internal/office/repository/sqlite/routines.go`.
|
||||
Tests: `arming_test.go` (all 9 rules, precedence, unarmed-reason ordering, the
|
||||
armed/unarmed-sibling interaction, trigger order, batch-then-fallback),
|
||||
`arming_dialect_postgres_test.go` (AC-001.6, gated on
|
||||
`KANDEV_TEST_POSTGRES_DSN`, not run in this environment), and a new
|
||||
`TestListTriggersByRoutineIDs_Batch` in `routines_test.go` covering the id
|
||||
tiebreak and the empty-routine/empty-input cases.
|
||||
|
||||
Verification: `gofmt -l` clean; `go test -tags fts5
|
||||
./internal/office/routines/... ./internal/office/repository/sqlite/...`
|
||||
passes except the pre-existing, unrelated `TestMigrate_PriorityIdempotent`
|
||||
failure (reproduced on the unmodified tree before this task's changes, via
|
||||
`git stash`); `golangci-lint run` clean on both packages.
|
||||
@@ -0,0 +1,176 @@
|
||||
---
|
||||
id: "02-report-schedule-state-api"
|
||||
title: "Report intent and schedule state on routine reads (API)"
|
||||
status: done
|
||||
wave: 2
|
||||
depends_on: ["01-classify-schedule-state"]
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-OFFICE-ROUTINE-ARMING-002
|
||||
acceptance_criteria:
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.1
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.4
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.5
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.6
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.7
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.11
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.12
|
||||
system_design:
|
||||
- ../../specs/office/system-design/routine-schedule-state.md
|
||||
---
|
||||
|
||||
# Task 02: Report intent and schedule state on routine reads (API)
|
||||
|
||||
Satisfies REQ-OFFICE-ROUTINE-ARMING-002's data-shape criteria:
|
||||
AC-OFFICE-ROUTINE-ARMING-002.1, .4 (data only — rendering is Task 03), .5,
|
||||
.6, .7, .11, .12. The rendering-only criteria (.2, .3, .8, .9, .10) belong
|
||||
to Task 03, which depends on this task's response shape.
|
||||
|
||||
## Scope
|
||||
|
||||
Extend the routine list response and the single-routine response to carry,
|
||||
per routine: intent (`office_routines.status`, unchanged), schedule state,
|
||||
and the unarmed cron trigger list, both from Task 01's classifier — without
|
||||
growing the query count with the number of routines in the workspace.
|
||||
|
||||
## Exclusions
|
||||
|
||||
- No new classification logic — call Task 01's batch entry point.
|
||||
- No UI change (Task 03).
|
||||
- No startup scan (Task 04).
|
||||
- No coordinator-install change (Task 05).
|
||||
|
||||
## Acceptance conditions
|
||||
|
||||
1. The routine list response and the single-routine response each carry
|
||||
intent, schedule state, and the unarmed cron trigger list per routine, in
|
||||
one call each (AC-002.1). The pair is not required to be a transactional
|
||||
snapshot: intent comes from the routine row, schedule state from the
|
||||
trigger rows, and neither is omitted because the other read failed — a
|
||||
routine whose triggers could not be read still carries its intent
|
||||
alongside schedule state `unknown` (AC-002.12).
|
||||
2. Producing the list costs a bounded number of queries that does not grow
|
||||
with routine count on the all-succeeds path: one query for routines, one
|
||||
batch query for triggers (Task 01's batch entry point), each isolated
|
||||
Task-001.13 re-read counted as the documented exception rather than a
|
||||
regression (AC-002.7).
|
||||
3. Response shape distinguishes, without prescribing rendering: a schedule
|
||||
state of `unscheduled_manual_only` or `unscheduled_no_trigger` is
|
||||
reportable as "no schedule" (AC-002.5); `unknown` is reportable as
|
||||
undetermined, distinct from armed or broken (AC-002.6); `event_only` is
|
||||
never bundled with "no schedule" and never asks for a trigger to be
|
||||
created (AC-002.11); and for a non-empty unarmed list, whether at least
|
||||
one entry is schedulable is derivable by the caller without it having to
|
||||
inspect the dispatch-grace math itself — an empty list carries neither
|
||||
side of that distinction (AC-002.4).
|
||||
|
||||
## Verification
|
||||
|
||||
- `cd apps/backend && go test -tags fts5 ./internal/office/dashboard/... ./internal/office/routines/...`
|
||||
- Test: routine list response for a fixture with all nine schedule states
|
||||
present carries the right state and unarmed list per routine.
|
||||
- Test: query-count assertion (via a counting DB wrapper or an existing
|
||||
budget-test pattern in this package) proves AC-002.7 on an all-succeeds
|
||||
fixture of N routines, and separately on a fixture that forces one
|
||||
Task-001.13 re-read.
|
||||
- Test: a routine whose trigger read fails carries its real intent value
|
||||
alongside `unknown` (AC-002.12).
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `apps/backend/internal/office/routines/handler.go` — `listRoutines`
|
||||
(L45-52) and `getRoutine` (L98-105) are the actual routine list/detail
|
||||
HTTP handlers (`GET /workspaces/:wsId/routines`, `GET /routines/:id`);
|
||||
call Task 01's `ClassifyRoutines`/`ClassifyRoutine` here and populate the
|
||||
extended response.
|
||||
- `apps/backend/internal/office/routines/dto.go` — `RoutineResponse` (L34)
|
||||
and `RoutineListResponse` (L39) currently carry only `*Routine`
|
||||
(`{routine}` / `{routines}` — intent only). Extend them to also carry
|
||||
schedule state and the unarmed cron trigger list per routine (e.g. a
|
||||
wrapper type embedding `*Routine` plus the two new fields, so intent and
|
||||
schedule state travel in the same JSON object per AC-002.1).
|
||||
- Note: `apps/backend/internal/office/dashboard/service_agents.go`'s
|
||||
`ListRoutines` block (~L551-561) is a separate, pre-existing aggregate —
|
||||
it only counts `status == "active"` into the workspace dashboard's KPI
|
||||
tile and is not this requirement's target; leave it alone unless it also
|
||||
needs the distinction (out of scope unless the dashboard tile itself is
|
||||
asked to change).
|
||||
- Corresponding `*_test.go` files (`internal/office/routines/handler_test.go`
|
||||
or new).
|
||||
|
||||
## Dependencies
|
||||
|
||||
Task 01.
|
||||
|
||||
## Parallelism
|
||||
|
||||
Independent of Task 04 and Task 05 once Task 01 lands — no shared files.
|
||||
|
||||
## Result
|
||||
|
||||
Implemented:
|
||||
|
||||
- `apps/backend/internal/office/routines/dto.go` — added `RoutineWithSchedule`
|
||||
(embeds `*Routine`, adds `schedule_state` and `unarmed_cron_triggers` JSON
|
||||
fields). `RoutineResponse.Routine` and `RoutineListResponse.Routines` now
|
||||
carry `*RoutineWithSchedule` / `[]*RoutineWithSchedule` instead of the bare
|
||||
`*Routine`.
|
||||
- `apps/backend/internal/office/routines/service.go` — added
|
||||
`ListTriggersByRoutineIDs` to the `Repository` interface (structurally
|
||||
required so `s.repo` satisfies Task 01's `RoutineTriggerReader`), and added
|
||||
`AttachScheduleState(ctx, []*Routine) ([]*RoutineWithSchedule, error)`,
|
||||
which calls `ClassifyRoutines` once per call (one batch trigger query,
|
||||
independent of routine count) and pairs each routine's intent with its
|
||||
schedule state and unarmed list.
|
||||
- `apps/backend/internal/office/routines/handler.go` — `listRoutines` and a
|
||||
new `withScheduleState` helper (used by `createRoutine`, `getRoutine`,
|
||||
`updateRoutine`) call `AttachScheduleState` before building the response.
|
||||
No other file in `internal/` constructs `RoutineResponse`/
|
||||
`RoutineListResponse` (confirmed via grep), so no other call sites needed
|
||||
updates.
|
||||
|
||||
AC-002.1: `RoutineWithSchedule` carries intent (`Status`, from the embedded
|
||||
`*Routine`), `ScheduleState`, and `UnarmedCronTriggers` in one JSON object.
|
||||
AC-002.12: covered by `TestAttachScheduleState_TriggerReadFailureKeepsIntent`
|
||||
— a routine whose trigger reads fail (batch and single) keeps its real
|
||||
`Status` alongside `ScheduleStateUnknown`.
|
||||
AC-002.7: covered by `TestAttachScheduleState_QueryCountBound` (one batch
|
||||
call, zero single calls, for 5 routines) and
|
||||
`TestAttachScheduleState_BatchFailureFallsBackPerRoutine` (one batch call,
|
||||
exactly one single call per routine on the documented fallback path), both
|
||||
via a `countingRepo` decorator wrapping a real in-memory-SQLite-backed
|
||||
repository.
|
||||
AC-002.5/.6/.11/.4: satisfied structurally — `ScheduleState` is a plain
|
||||
string enum a caller switches on directly (no additional derivation logic
|
||||
needed in this task), and `UnarmedCronTrigger.Reasons` already carries
|
||||
`not_schedulable` distinctly from `stalled`/`disabled`, so an empty list
|
||||
carries neither and a non-empty list is inspectable without dispatch-grace
|
||||
math. Verified in `TestAttachScheduleState_AllStatesFixture`, which covers
|
||||
armed, trigger_invalid, trigger_disabled, unscheduled_manual_only, and
|
||||
unscheduled_no_trigger routines in one list response.
|
||||
|
||||
New test file:
|
||||
`apps/backend/internal/office/routines/schedule_response_test.go` — 4 tests
|
||||
(`TestAttachScheduleState_AllStatesFixture`,
|
||||
`TestAttachScheduleState_QueryCountBound`,
|
||||
`TestAttachScheduleState_BatchFailureFallsBackPerRoutine`,
|
||||
`TestAttachScheduleState_TriggerReadFailureKeepsIntent`), all backed by a
|
||||
real `*routines.RoutineService` over an in-memory SQLite repository (via a
|
||||
`countingRepo` decorator), not just `ClassifyRoutines` in isolation (already
|
||||
covered by Task 01's tests).
|
||||
|
||||
Verification:
|
||||
|
||||
- `go build -tags fts5 ./...` — clean.
|
||||
- `go test -tags fts5 ./internal/office/routines/...` — `ok`.
|
||||
- `go test -tags fts5 ./internal/office/dashboard/...` — `ok`.
|
||||
- `go test -tags fts5 ./internal/office/repository/sqlite/...` — one
|
||||
pre-existing, unrelated failure (`TestMigrate_PriorityIdempotent`,
|
||||
confirmed via `git stash` against the clean tree during Task 01; not
|
||||
touched by this task).
|
||||
- `gofmt -l` on the new/changed files — clean.
|
||||
- `golangci-lint run ./internal/office/routines/... --new-from-rev=<merge-base>`
|
||||
— `0 issues` (fixed a goconst hit on the repeated `gin.H{"error": ...}`
|
||||
literal by extracting `respondInternalError` + a `jsonErrorKey` constant in
|
||||
`handler.go`; fixed staticcheck QF1008 redundant-embedded-selector hints in
|
||||
the new test file).
|
||||
@@ -0,0 +1,151 @@
|
||||
---
|
||||
id: "03-report-schedule-state-ui"
|
||||
title: "Render the intent/schedule-state distinction in the routines UI"
|
||||
status: done
|
||||
wave: 3
|
||||
depends_on: ["02-report-schedule-state-api"]
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-OFFICE-ROUTINE-ARMING-002
|
||||
acceptance_criteria:
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.2
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.3
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.4
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.5
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.6
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.8
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.9
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.10
|
||||
- AC-OFFICE-ROUTINE-ARMING-002.11
|
||||
system_design:
|
||||
- ../../specs/office/system-design/routine-schedule-state.md
|
||||
---
|
||||
|
||||
# Task 03: Render the intent/schedule-state distinction in the routines UI
|
||||
|
||||
Satisfies REQ-OFFICE-ROUTINE-ARMING-002's rendering criteria:
|
||||
AC-OFFICE-ROUTINE-ARMING-002.2, .3, .8, .9, .10, and the rendering half of
|
||||
.4, .5, .6, .11 (data shape is Task 02).
|
||||
|
||||
## Scope
|
||||
|
||||
Routines list and detail view render both switches. This is a labels-and-
|
||||
distinctions change only — the operator-initiated re-arm action was cut from
|
||||
scope (see the requirement's `## Out of scope`), so there is no mutating flow
|
||||
to build or test here.
|
||||
|
||||
## Exclusions
|
||||
|
||||
- No action, button, or endpoint that repairs a trigger or changes intent.
|
||||
- No new backend response fields (Task 02).
|
||||
|
||||
## Acceptance conditions
|
||||
|
||||
1. The routines list visually and semantically distinguishes: `active` +
|
||||
`armed` from `active` + anything else (AC-002.2); not-`active` from
|
||||
`active`-but-broken, so deliberate and broken never render the same
|
||||
(AC-002.3); and renders a distinct label for each of the five groups —
|
||||
`armed`; `{trigger_invalid, trigger_unscheduled, trigger_disabled}`;
|
||||
`event_only`; `{unscheduled_manual_only, unscheduled_no_trigger}`;
|
||||
`unknown` — such that two routines in different groups never share a
|
||||
label (AC-002.10). None of this distinction is carried by colour alone;
|
||||
each has accompanying text, an icon, or an accessible label (AC-002.9).
|
||||
2. A routine whose unarmed list has at least one schedulable entry is
|
||||
visually distinguished from one where none are (needs re-arming vs. needs
|
||||
editing), and this rendering is entirely suppressed when the unarmed list
|
||||
is empty rather than defaulting to either message (AC-002.4).
|
||||
`unscheduled_manual_only`/`unscheduled_no_trigger` render as "no
|
||||
schedule" (AC-002.5); `unknown` renders as undetermined (AC-002.6);
|
||||
`event_only` never renders as "no schedule" and is never directed to
|
||||
create a trigger (AC-002.11). No rendering here offers a repair action.
|
||||
3. All copy added by this task is routed through `t()`/`<Trans>` and present
|
||||
in English plus the four other shipped locales (`pt-pt`, `zh-cn`,
|
||||
`zh-hk`, `zh-tw`); `pnpm run i18n:check` and `pnpm run i18n:ratchet` both
|
||||
pass (AC-002.8).
|
||||
|
||||
## Verification
|
||||
|
||||
- `cd apps/web && pnpm run typecheck`
|
||||
- `cd apps/web && pnpm run lint`
|
||||
- `cd apps/web && pnpm run i18n:check`
|
||||
- Component test(s) covering the five-group label mapping and the
|
||||
schedulable/non-schedulable unarmed-list distinction.
|
||||
- Playwright: extend or add a spec under `apps/web/e2e/` covering the
|
||||
routines list/detail rendering for a fixture spanning all nine schedule
|
||||
states (read-only assertions only — no mutating interaction to drive).
|
||||
Confirm the e2e profile enables `KANDEV_FEATURES_OFFICE` before writing
|
||||
the spec.
|
||||
- `cd apps/web && pnpm e2e:run` scoped to the new/changed spec (respect the
|
||||
one-worker-per-shard and memory-aware shard budget in
|
||||
`apps/web/e2e/README.md`).
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- Routines list and detail components under `apps/web` (Office routines
|
||||
surface — exact path to be located during implementation; search for the
|
||||
existing routine-status badge component).
|
||||
- `apps/web/src/locales/en/*.json` plus `pt-pt`, `zh-cn`, `zh-hk`, `zh-tw`
|
||||
(use `pnpm run i18n:zh-hant` for the Traditional Chinese pair).
|
||||
- New or extended Playwright spec under `apps/web/e2e/`.
|
||||
|
||||
## Dependencies
|
||||
|
||||
Task 02.
|
||||
|
||||
## Parallelism
|
||||
|
||||
Independent of Task 04 and Task 05.
|
||||
|
||||
## Result
|
||||
|
||||
- `apps/web/app/office/routines/schedule-state.ts`: pure read/classify
|
||||
helpers (`readScheduleState`, `readUnarmedCronTriggers`,
|
||||
`scheduleStateGroup`, `hasSchedulableUnarmedEntry`) following the repo's
|
||||
established camelCase-typed/snake_case-at-runtime fallback pattern for the
|
||||
two new wire fields from Task 02.
|
||||
- `apps/web/app/office/routines/schedule-state-badge.tsx`: `ScheduleStateBadge`
|
||||
(five-group label + icon + tooltip, AC-002.9/.10) and
|
||||
`UnarmedScheduleHint` (schedulable-vs-not distinction, suppressed when the
|
||||
unarmed list is empty, AC-002.4/.5/.6/.11). Both render regardless of
|
||||
`routine.status`, so intent and schedule state stay visually independent
|
||||
(AC-002.1/.2/.3).
|
||||
- Wired into `routine-row.tsx` (list) and `routine-detail-view.tsx`'s
|
||||
`DetailReadOnlyCard` (detail view).
|
||||
- 12 new `office:` copy keys added to `en` plus `pt-pt`, `zh-cn` (hand
|
||||
translated) and `zh-hk`/`zh-tw` (via `pnpm run i18n:zh-hant`); pseudo-locale
|
||||
regenerated and scoped to only these keys.
|
||||
- Unit tests: `schedule-state.test.ts` (pure logic, all 8 reachable states to
|
||||
5 groups) and `schedule-state-badge.test.tsx` (rendering, using
|
||||
`TooltipProvider` per the repo's Radix tooltip test convention).
|
||||
- E2E: `apps/web/e2e/tests/office/routine-schedule-state.spec.ts` seeds one
|
||||
routine per reachable schedule state and asserts the routines list renders
|
||||
the correct AC-002.10 label group for each, plus AC-002.11 (`event_only`
|
||||
never shows "No schedule").
|
||||
- Confirmed `KANDEV_FEATURES_OFFICE` is already `e2e: "true"` in
|
||||
`profiles.yaml`; no profile change needed.
|
||||
- **Backend API gap found and fixed**: the public routine-trigger API
|
||||
cannot produce `trigger_invalid`, `trigger_unscheduled`, or
|
||||
`trigger_disabled` — `RoutineService.CreateRoutineTrigger` validates the
|
||||
cron expression and always computes a non-null `NextRunAt`, and there is
|
||||
no PATCH-trigger route to disable one after creation. Added a
|
||||
`KANDEV_E2E_MOCK`-gated test-harness route,
|
||||
`POST /api/v1/_test/routine-triggers`
|
||||
(`apps/backend/internal/office/testharness/routine_triggers.go` +
|
||||
`routine_triggers_test.go`), that inserts a trigger row directly via the
|
||||
repository, bypassing service-layer validation, following the existing
|
||||
`seedRun`/`seedComment` harness pattern. Exposed as
|
||||
`ApiClient.seedRoutineTrigger` in `apps/web/e2e/helpers/api-client.ts`.
|
||||
- `unknown` (a live trigger-read failure) has no fixture-reachable trigger
|
||||
shape at all — it requires simulating a database read error — so it is
|
||||
intentionally not covered by this spec. It has backend unit coverage in
|
||||
`arming_test.go` (Task 01) instead.
|
||||
- The fixture therefore spans 7 of the 9 raw states, covering all 5 render
|
||||
groups (`armed`, `broken` via 3 distinct raw states, `event_only`,
|
||||
`no_schedule` via 2 distinct raw states); `unknown` is the one documented
|
||||
gap.
|
||||
- Verification run: `pnpm run typecheck`, `pnpm run lint`, `pnpm run
|
||||
i18n:check`, `pnpm run i18n:ratchet`, `npx vitest run` on both new test
|
||||
files, `go build ./...`, `gofmt -l`, `golangci-lint run
|
||||
./internal/office/testharness/...`, `go test
|
||||
./internal/office/testharness/...`, and `pnpm e2e:run
|
||||
tests/office/routine-schedule-state.spec.ts` — all pass.
|
||||
@@ -0,0 +1,210 @@
|
||||
---
|
||||
id: "04-startup-scan"
|
||||
title: "Surface unarmed routines at startup"
|
||||
status: done
|
||||
wave: 2
|
||||
depends_on: ["01-classify-schedule-state"]
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-OFFICE-ROUTINE-ARMING-003
|
||||
acceptance_criteria:
|
||||
- AC-OFFICE-ROUTINE-ARMING-003.1
|
||||
- AC-OFFICE-ROUTINE-ARMING-003.2
|
||||
- AC-OFFICE-ROUTINE-ARMING-003.3
|
||||
- AC-OFFICE-ROUTINE-ARMING-003.4
|
||||
- AC-OFFICE-ROUTINE-ARMING-003.5
|
||||
- AC-OFFICE-ROUTINE-ARMING-003.6
|
||||
- AC-OFFICE-ROUTINE-ARMING-003.7
|
||||
- AC-OFFICE-ROUTINE-ARMING-003.8
|
||||
- AC-OFFICE-ROUTINE-ARMING-003.9
|
||||
- AC-OFFICE-ROUTINE-ARMING-003.10
|
||||
- AC-OFFICE-ROUTINE-ARMING-003.11
|
||||
- AC-OFFICE-ROUTINE-ARMING-003.12
|
||||
- AC-OFFICE-ROUTINE-ARMING-003.13
|
||||
- AC-OFFICE-ROUTINE-ARMING-003.14
|
||||
system_design:
|
||||
- ../../specs/office/system-design/routine-schedule-state.md
|
||||
---
|
||||
|
||||
# Task 04: Surface unarmed routines at startup
|
||||
|
||||
Satisfies REQ-OFFICE-ROUTINE-ARMING-003 (AC-OFFICE-ROUTINE-ARMING-003.1
|
||||
through .14).
|
||||
|
||||
## Scope
|
||||
|
||||
A read-only pass at Office startup, run once reconciliation has returned,
|
||||
that classifies every enumerable routine in every enumerable workspace
|
||||
(via Task 01) and emits structured logs plus `expvar` counters. No
|
||||
production behavior changes if the scan is skipped or fails.
|
||||
|
||||
## Exclusions
|
||||
|
||||
- No classification logic of its own — consumes Task 01.
|
||||
- No repair, no retry, no periodic re-scan, no API.
|
||||
- No coordinator-install change (Task 05).
|
||||
|
||||
## Acceptance conditions
|
||||
|
||||
1. The scan runs after `infra.Reconciler.ReconcileAll` (main.go:1378-1380)
|
||||
has returned, ordered by an explicit completion signal passed from the
|
||||
startup sequence — not a timer, not an inspection of trigger rows.
|
||||
Invoked without that signal (exercised only in a test), it classifies
|
||||
nothing, emits one warning-level record, and increments the skipped
|
||||
counter with a reason distinct from enumeration and per-routine
|
||||
classification failure (AC-003.1). It iterates workspaces by
|
||||
`workspaces.created_at` then `workspaces.id`, and within each, routines
|
||||
by `office_routines.created_at` then `office_routines.id` (AC-003.2).
|
||||
Office startup does not wait for the scan and no scan outcome, including
|
||||
a database error, changes startup's result (AC-003.10). It writes
|
||||
nothing and never enables, disables, creates, or deletes a trigger
|
||||
(AC-003.11).
|
||||
2. Per routine reached, exactly one record: warning for `active` +
|
||||
{`trigger_disabled`, `trigger_unscheduled`, `trigger_invalid`} naming
|
||||
workspace, routine, state, and unarmed cron triggers in trigger order
|
||||
(AC-003.3); warning for `active` + {`unscheduled_manual_only`,
|
||||
`unscheduled_no_trigger`} naming workspace, routine, and state with no
|
||||
trigger list, since the routine owns no cron trigger (AC-003.14);
|
||||
informational, no warning, for not-`active` (AC-003.4); informational,
|
||||
no warning, for `active` + any other non-`unknown` state (AC-003.5,
|
||||
scoped to exclude the .3 and .14 cases); and for `unknown`, one
|
||||
informational record naming workspace/routine/state regardless of
|
||||
intent, which both counts once under the observation counter (AC-003.6)
|
||||
and once under the skipped counter (AC-003.7) — intentional double
|
||||
counting, not a bug (AC-003.9).
|
||||
3. Enumeration failures fail closed and are counted distinguishably: failing
|
||||
to enumerate the workspace list at all increments the skipped counter
|
||||
with its own reason, emits one warning, and returns without classifying
|
||||
anything (AC-003.8); failing to enumerate one workspace's routines keeps
|
||||
every record already emitted for earlier workspaces, increments the
|
||||
skipped counter once for that workspace, emits one warning naming it,
|
||||
does not retry it, and continues to the next workspace in order
|
||||
(AC-003.13).
|
||||
4. Running the scan twice over unchanged data emits the same records in the
|
||||
same order with no database change, where "same" excludes wall-clock
|
||||
fields, counters, and a trigger that crossed the dispatch-grace boundary
|
||||
between the two scans, and where the comparison only applies between two
|
||||
fully-successful scans (AC-003.12).
|
||||
|
||||
## Verification
|
||||
|
||||
- `cd apps/backend && go test -tags fts5 ./internal/office/routines/... ./internal/office/infra/... ./internal/backendapp/...`
|
||||
- Test: scan invoked without the completion signal classifies nothing and
|
||||
increments the "no signal" skipped reason (AC-003.1's test branch).
|
||||
- Test: fixture spanning all schedule states produces exactly the record
|
||||
level/content matrix of AC-003.3/.4/.5/.9/.14, one record per routine.
|
||||
- Test: workspace-enumeration failure and mid-scan per-workspace
|
||||
enumeration failure each fail closed per AC-003.8/.13, and startup itself
|
||||
never fails or blocks on any scan outcome (AC-003.10).
|
||||
- Test: two scans over unchanged fixture data produce identical records
|
||||
under the AC-003.12 equality (excluding wall-clock/counters and any
|
||||
grace-boundary-straddling trigger).
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- new: `apps/backend/internal/office/routines/startup_scan.go`
|
||||
- new: `apps/backend/internal/office/routines/startup_scan_test.go`
|
||||
- new: `apps/backend/internal/office/routines/arming_metrics.go` (expvar
|
||||
counters, following `internal/orchestrator/office_stall_metrics.go`'s
|
||||
`expvar.NewMap` + label-builder shape)
|
||||
- `apps/backend/internal/office/infra/reconcile.go` — emit or return the
|
||||
completion signal `ReconcileAll` currently has no way to expose.
|
||||
- `apps/backend/internal/backendapp/main.go` (~L1378-1393) — wire the
|
||||
signal from `ReconcileAll` into the new scan, launched so it does not
|
||||
block startup.
|
||||
|
||||
## Dependencies
|
||||
|
||||
Task 01 (classification, and the shared `dispatchGrace` constant it
|
||||
defines).
|
||||
|
||||
## Parallelism
|
||||
|
||||
Independent of Task 02 and Task 05 once Task 01 lands — no shared files.
|
||||
|
||||
## Result
|
||||
|
||||
Implemented:
|
||||
|
||||
- `apps/backend/internal/office/repository/sqlite/routine_arming_scan.go`
|
||||
(new) — `ListWorkspaceIDsOrdered` (`SELECT id FROM workspaces ORDER BY
|
||||
created_at, id`) and `ListRoutinesOrdered` (per-workspace, same ordering),
|
||||
separate from the UI's name-ordered `ListRoutines`/workspace listing.
|
||||
- `apps/backend/internal/office/routines/arming_metrics.go` (new) — two
|
||||
`expvar.Map` counters (`office_routine_arming_scan_observations_total`,
|
||||
`office_routine_arming_scan_skipped_total`), following
|
||||
`internal/orchestrator/office_stall_metrics.go`'s label-string shape, with
|
||||
the four skip reasons (`no_signal`, `workspace_enum_failed`,
|
||||
`routine_enum_failed`, `classification_unknown`).
|
||||
- `apps/backend/internal/office/routines/startup_scan.go` (new) —
|
||||
`ReconcileSignal` (only constructible via `SignalReconcileComplete`, so an
|
||||
unauthorized/test caller holds the refused zero value), the
|
||||
`StartupScanReader` interface (Task 01's `RoutineTriggerReader` plus the
|
||||
two new ordered listers), and `RunStartupScan`, which classifies every
|
||||
workspace's routines via Task 01's `ClassifyRoutines` (one batch trigger
|
||||
read per workspace) and emits exactly one structured log record per
|
||||
routine reached, per the AC-003.3/.4/.5/.9/.14 matrix.
|
||||
- `apps/backend/internal/office/infra/reconcile.go` — `ReconcileAll` now
|
||||
returns `routines.ReconcileSignal` (via `SignalReconcileComplete()`)
|
||||
instead of nothing, so a caller has an explicit, non-timer, non-inspection
|
||||
way to know reconciliation returned. Existing callers that ignore the
|
||||
return value keep compiling unchanged.
|
||||
- `apps/backend/internal/backendapp/main.go` (~L1377-1384) — after
|
||||
`reconciler.ReconcileAll(ctx)` returns, launches
|
||||
`go officeroutines.RunStartupScan(ctx, reconcileSignal, repos.Office, log,
|
||||
time.Now().UTC())`. `repos.Office` (`*sqlite.Repository`) satisfies
|
||||
`StartupScanReader` structurally (confirmed by `go build` succeeding with
|
||||
no adapter needed). The scan runs in its own goroutine so office startup
|
||||
does not wait for it (AC-003.10).
|
||||
|
||||
AC-003.1: `TestRunStartupScan_NoSignalClassifiesNothing` uses a
|
||||
`panicIfCalledReader` (panics on any method call) to prove the no-signal
|
||||
path makes zero reader calls, emits exactly one warning, and increments
|
||||
`no_signal`.
|
||||
AC-003.2: enumeration order comes from the two new `ORDER BY created_at, id`
|
||||
repository methods; not independently retested here since Task 01/02 already
|
||||
cover the tiebreak mechanics this reuses.
|
||||
AC-003.3/.4/.5/.9/.14: `TestRunStartupScan_AllStatesFixtureRecordMatrix`
|
||||
covers all five branches in one workspace (cannot-fire with a trigger list,
|
||||
no-schedule without one, an active+armed "observed" info record, a
|
||||
not-active routine regardless of its schedule state, and a forced-`unknown`
|
||||
routine), and asserts both the `unknown` observation-counter increment and
|
||||
the `classification_unknown` skip-counter increment fire together
|
||||
(intentional double counting per the spec).
|
||||
AC-003.6/.7: covered throughout via `armingScanObservationsTotal`/
|
||||
`armingScanSkippedTotal` counter-delta assertions.
|
||||
AC-003.8: `TestRunStartupScan_WorkspaceEnumerationFailure` — one warning,
|
||||
`workspace_enum_failed` incremented, no routine ever reached.
|
||||
AC-003.10: the scan never returns an error or a value main.go could act on;
|
||||
launched via `go`, it cannot block or fail startup by construction.
|
||||
AC-003.11: the scan only calls the two new read-only listers and Task 01's
|
||||
read-only trigger reader; no write path exists on `StartupScanReader`.
|
||||
AC-003.12: `TestRunStartupScan_IdempotentOverUnchangedData` runs the scan
|
||||
twice with the same fixed `now` and compares each run's log records
|
||||
(level, message, context fields) via `reflect.DeepEqual`, excluding
|
||||
wall-clock fields by never logging any.
|
||||
AC-003.13: `TestRunStartupScan_PerWorkspaceRoutineEnumerationFailureContinues`
|
||||
— 3 workspaces, the middle one's routine enumeration forced to fail; asserts
|
||||
the earlier and later workspaces' records are preserved, the scan continues
|
||||
past the failure, and `routine_enum_failed` increments exactly once.
|
||||
|
||||
New files:
|
||||
`apps/backend/internal/office/repository/sqlite/routine_arming_scan.go`,
|
||||
`apps/backend/internal/office/routines/arming_metrics.go`,
|
||||
`apps/backend/internal/office/routines/startup_scan.go`,
|
||||
`apps/backend/internal/office/routines/startup_scan_test.go` (in-package
|
||||
`routines` test file, needed to assert directly on the unexported expvar
|
||||
counters, following `internal/orchestrator/office_stall_visibility_test.go`'s
|
||||
precedent for the same reason).
|
||||
|
||||
Verification:
|
||||
|
||||
- `go build -tags fts5 ./...` — clean.
|
||||
- `go test -tags fts5 ./internal/office/routines/... ./internal/office/infra/... ./internal/backendapp/...`
|
||||
— all `ok`.
|
||||
- `go test -tags fts5 ./internal/office/repository/sqlite/...` — one
|
||||
pre-existing, unrelated failure (`TestMigrate_PriorityIdempotent`,
|
||||
confirmed during Task 01; not touched by this task).
|
||||
- `gofmt -l` on the new/changed files — clean.
|
||||
- `golangci-lint run ./internal/office/infra/... ./internal/backendapp/... ./internal/office/routines/... ./internal/office/repository/sqlite/... --new-from-rev=<merge-base>`
|
||||
— `0 issues`.
|
||||
@@ -0,0 +1,272 @@
|
||||
---
|
||||
id: "05-coordinator-install-idempotency"
|
||||
title: "Match the coordinator routine on identity, not mutable trigger state"
|
||||
status: done
|
||||
wave: 2
|
||||
depends_on: ["01-classify-schedule-state"]
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-OFFICE-COORDINATOR-INSTALL-001
|
||||
acceptance_criteria:
|
||||
- AC-OFFICE-COORDINATOR-INSTALL-001.1
|
||||
- AC-OFFICE-COORDINATOR-INSTALL-001.2
|
||||
- AC-OFFICE-COORDINATOR-INSTALL-001.3
|
||||
- AC-OFFICE-COORDINATOR-INSTALL-001.4
|
||||
- AC-OFFICE-COORDINATOR-INSTALL-001.5
|
||||
- AC-OFFICE-COORDINATOR-INSTALL-001.6
|
||||
- AC-OFFICE-COORDINATOR-INSTALL-001.7
|
||||
- AC-OFFICE-COORDINATOR-INSTALL-001.8
|
||||
- AC-OFFICE-COORDINATOR-INSTALL-001.9
|
||||
- AC-OFFICE-COORDINATOR-INSTALL-001.10
|
||||
- AC-OFFICE-COORDINATOR-INSTALL-001.11
|
||||
- AC-OFFICE-COORDINATOR-INSTALL-001.12
|
||||
- AC-OFFICE-COORDINATOR-INSTALL-001.13
|
||||
- AC-OFFICE-COORDINATOR-INSTALL-001.14
|
||||
- AC-OFFICE-COORDINATOR-INSTALL-001.15
|
||||
system_design:
|
||||
- ../../specs/office/system-design/coordinator-install-idempotency.md
|
||||
---
|
||||
|
||||
# Task 05: Match the coordinator routine on identity, not mutable trigger state
|
||||
|
||||
Satisfies REQ-OFFICE-COORDINATOR-INSTALL-001 (AC-OFFICE-COORDINATOR-INSTALL-001.1
|
||||
through .15).
|
||||
|
||||
## Scope
|
||||
|
||||
Redefine coordinator-routine identity as workspace + assignee agent +
|
||||
canonical routine name only, complete a half-finished install (routine with
|
||||
no cron trigger) instead of duplicating it, handle the case where more than
|
||||
one existing routine already matches that identity, and serialize
|
||||
check-and-create against concurrent installs across processes sharing one
|
||||
database.
|
||||
|
||||
## Exclusions
|
||||
|
||||
- Does not change the schedule-state definition (Task 01) — only reports it
|
||||
(AC-OFFICE-COORDINATOR-INSTALL-001.5).
|
||||
- Does not merge, delete, or otherwise touch duplicate routines created by a
|
||||
prior buggy install beyond selecting one to complete
|
||||
(AC-OFFICE-COORDINATOR-INSTALL-001.3).
|
||||
- Does not add `enabled` to the identity match — forbidden by
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.1, and explicitly the trap the source
|
||||
card's framing invites.
|
||||
- Does not add a database uniqueness constraint over the identity columns
|
||||
(AC-OFFICE-COORDINATOR-INSTALL-001.9 forbids it; upgraded installs already
|
||||
violate it).
|
||||
|
||||
## Acceptance conditions
|
||||
|
||||
1. `findCoordinatorRoutine` matches on workspace + assignee + canonical name
|
||||
only — no trigger presence, expression, `enabled`, or `next_run_at` term
|
||||
(AC-001.1). A single match: return it, create nothing
|
||||
(AC-001.2). No cron trigger on the match: create the canonical cron
|
||||
trigger (canonical expression, canonical timezone, `enabled = true`,
|
||||
`next_run_at` = first canonical occurrence strictly after one instant
|
||||
captured once at install start) and return the existing routine; if that
|
||||
next-occurrence computation fails, reject and report without touching
|
||||
the routine (it is a defect in the installer's own constants, unreachable
|
||||
by any input); if trigger creation itself fails, reject, report, and
|
||||
leave the routine in place so a later install reaches this same branch
|
||||
(AC-001.4). Any cron trigger already present, canonical or not, enabled
|
||||
or not: leave every trigger unchanged, create no second one, report
|
||||
schedule state (AC-001.5). Never change `status` on an existing match
|
||||
(AC-001.6). Empty workspace, assignee, or canonical name: reject before
|
||||
the identity lookup, reported distinguishably from a read failure
|
||||
(AC-001.14). No install-call ever retries a failure internally — each
|
||||
rejection returns to the caller as an error, optionally alongside the
|
||||
routine found or created, matching what both callers already do today
|
||||
(AC-001.15).
|
||||
2. More than one existing routine matches the identity: select the one with
|
||||
the earliest `office_routines.created_at`, ties by `id` ascending;
|
||||
delete none; report that more than one was found; then run the selected
|
||||
routine through the same .4/.5 branch a single match would take
|
||||
(AC-001.3). No match at all: create one routine with that identity and
|
||||
`status = 'active'` plus its canonical trigger; a routine-insert failure
|
||||
rejects and creates no trigger; a trigger-creation failure after a
|
||||
successful routine insert leaves the routine in place and reports the
|
||||
failure so a later install completes it via AC-001.4 (AC-001.12). An
|
||||
identity-lookup failure, or a failure reading the existing routine's
|
||||
triggers (the read that decides AC-001.4 vs. AC-001.5), rejects and
|
||||
creates nothing — never treated as "absent" (AC-001.7).
|
||||
3. Two concurrent installs for the same workspace and assignee create at
|
||||
most one new coordinator routine and at most one new canonical trigger
|
||||
between them, on both SQLite and PostgreSQL, via a serialization
|
||||
mechanism whose domain is every process sharing the database — not an
|
||||
in-process guard alone (AC-001.8, AC-001.9, AC-001.11). Acquisition never
|
||||
blocks indefinitely: it acquires or fails within a single named
|
||||
compile-time 5-second bound, not configurable by environment, config, or
|
||||
flag (AC-001.9). Failure to acquire, or losing a serialized section to a
|
||||
concurrent install, rejects, creates nothing, and reports contention
|
||||
distinguishably from AC-001.7's failures and is not retried within the
|
||||
call (AC-001.13). Context cancellation while waiting likewise rejects and
|
||||
creates nothing, reported distinguishably from both contention and
|
||||
AC-001.7 (AC-001.13).
|
||||
4. Every condition above that must be reported (AC-001.3's duplicate count
|
||||
and selection, AC-001.5's schedule state) is a structured log record plus
|
||||
a counter per condition — never an additional return value, since both
|
||||
production callers discard everything but the error today (AC-001.10).
|
||||
Each record names workspace, assignee, and the routine it concerns.
|
||||
Counters count calls in which a condition was *observed*, including
|
||||
calls that are ultimately rejected. The whole requirement behaves
|
||||
identically on SQLite and PostgreSQL (AC-001.11).
|
||||
|
||||
## Design constraint for the serialization mechanism (AC-001.9, AC-001.13)
|
||||
|
||||
Pick a mechanism satisfying: (a) blocks/serializes across processes sharing
|
||||
one database, not merely goroutines in one process; (b) works identically on
|
||||
SQLite and PostgreSQL; (c) bounds acquisition at a hard 5s; (d) distinguishes
|
||||
"could not acquire" (contention) from "acquired, then lost the race inside
|
||||
the critical section" from context cancellation. The requirement's own
|
||||
`## Out of scope` names three mechanisms that individually fail one of
|
||||
these and explicitly forbids treating them as interchangeable: a bare
|
||||
default-isolation-level transaction (does not stop two callers both reading
|
||||
"absent"), a PostgreSQL advisory lock (no SQLite equivalent), an in-process
|
||||
mutex (fails the cross-process domain). A row-level lock strategy
|
||||
(`SELECT ... FOR UPDATE`-equivalent per dialect, or a small dedicated lock
|
||||
row keyed by workspace+assignee with a bounded wait) is a plausible
|
||||
candidate Build should evaluate against all four constraints before
|
||||
building it.
|
||||
|
||||
**If the chosen mechanism adds a new table or columns**, add it to
|
||||
`internal/persistence/requiredstores` and
|
||||
`internal/persistence/storeconformance` per the backend schema-owner rule,
|
||||
and add the fresh/replay conformance tests that requires — this is a risk to
|
||||
flag during implementation, not a decision to make in this work order.
|
||||
|
||||
## Verification
|
||||
|
||||
- `cd apps/backend && go test -tags fts5 ./internal/office/routines/... ./internal/office/agents/... ./internal/office/onboarding/...`
|
||||
- Test: identity match ignores an edited or absent trigger (regression for
|
||||
the original card's bug — a routine with a non-canonical or deleted
|
||||
trigger still matches).
|
||||
- Test: install against a triggerless existing match creates exactly the
|
||||
canonical trigger and returns the existing routine (AC-001.4), and running
|
||||
it again is a no-op via AC-001.5.
|
||||
- Test: two or more matching routines — selects earliest `created_at`/`id`,
|
||||
deletes none, reports the count, and still completes the selected one's
|
||||
schedule (AC-001.3).
|
||||
- Test: concurrent-install race — two goroutines (or two DB connections
|
||||
simulating two processes) racing `CreateDefaultCoordinatorRoutine` for the
|
||||
same identity produce exactly one routine and one trigger; run against
|
||||
both SQLite and `KANDEV_TEST_POSTGRES_DSN` PostgreSQL.
|
||||
- Test: empty workspace/assignee/canonical-name is rejected before any read
|
||||
(AC-001.14).
|
||||
- Test: contention and context-cancellation rejections are distinguishable
|
||||
from each other and from AC-001.7's failures in the returned error/report.
|
||||
- If a new schema owner was added: `go run ./cmd/sqlguard ./internal` and
|
||||
`go test -race ./internal/persistence/storeconformance -count=1`.
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `apps/backend/internal/office/routines/service.go` —
|
||||
`CreateDefaultCoordinatorRoutine`, `findCoordinatorRoutine`,
|
||||
`hasCronTrigger` (L145-239).
|
||||
- `apps/backend/internal/office/repository/sqlite/routines.go` — new
|
||||
identity-lookup query and whatever serialization primitive is chosen.
|
||||
- new: `apps/backend/internal/office/routines/coordinator_install_metrics.go`
|
||||
(structured records + counters, following the same `expvar.NewMap`
|
||||
convention as Task 04).
|
||||
- `apps/backend/internal/office/agents/service.go`,
|
||||
`apps/backend/internal/office/onboarding/service.go` — no behavior change
|
||||
expected (both already discard the return value except the error), but
|
||||
their tests should assert the discard contract still holds.
|
||||
- Corresponding `*_test.go` files, plus a PostgreSQL-gated concurrency test.
|
||||
|
||||
## Dependencies
|
||||
|
||||
Task 01 (this task reports Task 01's schedule state via AC-001.5; the
|
||||
requirement document states it "cannot ship before that one").
|
||||
|
||||
## Parallelism
|
||||
|
||||
Independent of Task 02 and Task 04 once Task 01 lands — no shared files
|
||||
with either.
|
||||
|
||||
## Result
|
||||
|
||||
Implemented as two separately-locked, separately-committed phases rather
|
||||
than one transaction, after TDD caught a real defect in the first cut: a
|
||||
single all-or-nothing transaction covering both routine-creation and
|
||||
trigger-creation would roll back an already-decided routine on a
|
||||
trigger-creation failure, violating AC-001.12's "leaves the routine in
|
||||
place" requirement. Splitting into two phases (each its own lock + tx)
|
||||
fixes this — a trigger-creation failure can now only abort the trigger's
|
||||
own transaction, never the routine's already-committed one.
|
||||
|
||||
**Serialization mechanism** (resolves the work order's open design
|
||||
question): a real database transaction on both dialects, holding, on
|
||||
PostgreSQL only, a `pg_try_advisory_xact_lock` polled every 25ms inside a
|
||||
context bounded to a hard 5s (`coordinatorInstallLockTimeout`, not
|
||||
configurable by env/config/flag). On SQLite the existing single
|
||||
writer-connection pool (`internal/db.OpenSQLite`'s `SetMaxOpenConns(1)`)
|
||||
already serializes writers process-locally; PostgreSQL needs the explicit
|
||||
advisory lock because a plain transaction under READ COMMITTED does not
|
||||
stop two callers from both observing "absent". This combines (rather than
|
||||
picks one of) the three mechanisms the requirement's Out-of-scope section
|
||||
individually rules out.
|
||||
|
||||
Files:
|
||||
- `apps/backend/internal/office/models/coordinator_install.go` (new) —
|
||||
`models.ErrCoordinatorInstallContention` sentinel and the
|
||||
`models.CoordinatorInstallTx` interface, placed in the shared `models`
|
||||
package (rather than in `sqlite` or `routines`) so neither package has to
|
||||
import the other.
|
||||
- `apps/backend/internal/office/repository/sqlite/coordinator_install.go`
|
||||
(new) — `WithCoordinatorInstallLock` (generic lock-key + tx runner),
|
||||
`CoordinatorInstallLockKey` (workspace+assignee+name), the separate
|
||||
`CoordinatorInstallTriggerLockKey` (routine id), `InstallCoordinatorRoutine`
|
||||
(routine-existence phase), `EnsureCoordinatorTrigger` (trigger-existence
|
||||
phase, its own lock/tx), `acquirePostgresAdvisoryLock`,
|
||||
`classifyCoordinatorInstallWaitErr`.
|
||||
- `apps/backend/internal/office/repository/sqlite/routines.go` — added
|
||||
`CreateRoutineTriggerTx`, refactored `CreateRoutineTrigger` to share an
|
||||
`insertRoutineTrigger` helper.
|
||||
- `apps/backend/internal/office/routines/coordinator_install_metrics.go`
|
||||
(new) — `coordinatorInstallConditionsTotal` expvar map, 12 condition
|
||||
constants, `coordinatorInstallObserved`.
|
||||
- `apps/backend/internal/office/routines/service.go` — `Repository`
|
||||
interface gained `InstallCoordinatorRoutine` and
|
||||
`EnsureCoordinatorTrigger`. `CreateDefaultCoordinatorRoutine` now runs
|
||||
`ensureCoordinatorRoutine` (AC-001.2/.3/.12's routine half) then
|
||||
`EnsureCoordinatorTrigger`'s decide callback (AC-001.4/.5/.12's trigger
|
||||
half, via `decideCoordinatorTrigger`), each reported through the shared
|
||||
`reportCoordinatorLockFailure` (contention / cancelled / a
|
||||
caller-supplied fallback condition for the read-before-decide failing).
|
||||
|
||||
Tests (all new):
|
||||
- `apps/backend/internal/office/routines/coordinator_install_test.go`
|
||||
(internal `package routines`, for `coordinatorInstallConditionsTotal`
|
||||
access) — empty identity (AC-001.14), no-match create (AC-001.12),
|
||||
single match with any cron trigger unchanged (AC-001.2/.5), no-trigger
|
||||
match completes install (AC-001.4) plus re-entrancy, duplicate matches
|
||||
select earliest and leave the rest untouched (AC-001.3), status never
|
||||
changes (AC-001.6), identity-lookup and trigger-read failures rejected
|
||||
and distinguishable (AC-001.7), routine-insert failure creates no
|
||||
trigger, trigger-create failure leaves the routine in place and a
|
||||
follow-up call completes it (AC-001.12), contention vs. cancellation vs.
|
||||
lookup-failure are counted on distinct counters (AC-001.13), and a real
|
||||
two-goroutine race against one shared SQLite writer connection creates at
|
||||
most one routine and one trigger (AC-001.8/.9).
|
||||
- `apps/backend/internal/office/routines/coordinator_install_dialect_postgres_test.go`
|
||||
(external `package routines_test`, `testutil.PostgresDSNFromEnv` gated,
|
||||
skips without `KANDEV_TEST_POSTGRES_DSN`) — fresh install + idempotent
|
||||
repeat produce the same shape on SQLite and PostgreSQL (AC-001.11), and a
|
||||
genuine concurrent-install race against PostgreSQL (where the advisory
|
||||
lock actually runs) creates at most one routine and trigger (AC-001.8/.9).
|
||||
|
||||
No new database table or column was introduced, so
|
||||
`internal/persistence/requiredstores`/`storeconformance` and `sqlguard`
|
||||
were not needed.
|
||||
|
||||
Verification: `go build -tags fts5 ./...` clean; `gofmt -l` clean on every
|
||||
touched/new file; `go vet -tags fts5 ./internal/office/...` clean;
|
||||
`golangci-lint run ./internal/office/models/... ./internal/office/repository/sqlite/... ./internal/office/routines/... --new-from-rev=<merge-base> --timeout=5m`
|
||||
→ 0 issues; `go test -tags fts5 -race -count=1 ./internal/office/routines/... ./internal/office/agents/... ./internal/office/onboarding/... ./internal/office/wakeup/...`
|
||||
all pass, including the two pre-existing coordinator-install tests
|
||||
(`TestCreateDefaultCoordinatorRoutine_Idempotent`,
|
||||
`TestRoutine_EndToEnd_CoordinatorHeartbeatFire`) unchanged. The new
|
||||
PostgreSQL-gated tests skip locally (`KANDEV_TEST_POSTGRES_DSN` unset). The
|
||||
one pre-existing, unrelated failure (`TestMigrate_PriorityIdempotent` in
|
||||
`internal/office/repository/sqlite`) is confirmed present before this
|
||||
task's changes and out of scope.
|
||||
@@ -0,0 +1,259 @@
|
||||
---
|
||||
status: draft
|
||||
system: office
|
||||
created: 2026-09-17
|
||||
owners:
|
||||
- kandev
|
||||
---
|
||||
|
||||
# Office Coordinator Install Idempotency Requirements
|
||||
|
||||
## Overview
|
||||
|
||||
Onboarding pre-installs a `Coordinator heartbeat` routine. The installer decides
|
||||
whether that routine is already present by matching the workspace, the assignee
|
||||
agent, the canonical routine name, **and** the presence of a cron trigger whose
|
||||
expression equals the canonical one. That last clause is the problem: trigger
|
||||
presence and trigger expression are mutable, so a routine that was installed
|
||||
successfully can stop matching itself later.
|
||||
|
||||
Two paths reach that state on an install running today. A half-finished install
|
||||
leaves a routine row with no trigger at all, because trigger creation is a
|
||||
second write that can fail after the routine is created and whose error is
|
||||
warn-logged by both callers rather than rolled back. And a coordinator trigger
|
||||
whose expression stops equalling the canonical one leaves a routine that no
|
||||
longer matches itself. No endpoint edits a trigger in place — the trigger routes
|
||||
are list, create, and delete — so that state is reached by deleting the
|
||||
coordinator's trigger and creating a replacement carrying a different expression,
|
||||
or by editing the row directly. Which path was taken matters less than the
|
||||
result, because identity must not depend on a value that any of them can change.
|
||||
Either way, re-running onboarding sees no match and creates a second coordinator
|
||||
routine, so the workspace ends up with two heartbeats or, more often, two
|
||||
routines neither of which fires.
|
||||
|
||||
The match is *not* sensitive to `enabled` today — the installer never reads that
|
||||
column. This document's job is to keep it that way while removing the two
|
||||
mutable clauses it does match on. Making identity depend on `enabled`, which a
|
||||
reader coming from the card's "disabled trigger" framing might reasonably assume
|
||||
is already the case or is the fix, would **add a third duplication path** rather
|
||||
than close one, and is forbidden by AC-OFFICE-COORDINATOR-INSTALL-001.1.
|
||||
|
||||
This document does not define or repair a schedule. It consumes the
|
||||
classification of
|
||||
[Office Routine Arming Visibility](routine-arming-visibility.md) — schedule state
|
||||
is defined there, and AC-OFFICE-COORDINATOR-INSTALL-001.5 requires the install to
|
||||
report it — so this document cannot ship before that one.
|
||||
|
||||
## Terminology
|
||||
|
||||
This document uses the terms defined in
|
||||
[Office Routine Arming Visibility § Terminology](routine-arming-visibility.md):
|
||||
*fire*, *armed*, *intent*, and *schedule state*. It adds one:
|
||||
|
||||
- **Canonical:** the routine name, cron expression, and timezone that onboarding
|
||||
itself supplies for the coordinator routine. Canonical values are the
|
||||
installer's own constants, not operator input.
|
||||
|
||||
## Requirements
|
||||
|
||||
### REQ-OFFICE-COORDINATOR-INSTALL-001: Match the coordinator routine on identity, not on mutable trigger state
|
||||
|
||||
**Intent:** The pre-installed coordinator routine is matched on name plus the
|
||||
presence of a cron trigger carrying an exact expression. A routine whose trigger
|
||||
creation failed, or whose expression was edited, therefore stops matching itself
|
||||
and the installer creates a second coordinator routine. Identity must not depend
|
||||
on mutable trigger state.
|
||||
|
||||
**User story:** As an operator re-running onboarding on an upgraded install, I
|
||||
want one coordinator routine, with its schedule completed rather than duplicated.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-COORDINATOR-INSTALL-001.1:** The coordinator routine's identity shall be
|
||||
the workspace, the assignee agent, and the canonical routine name, and shall
|
||||
not depend on any trigger's `enabled` value, `next_run_at`, cron expression, or
|
||||
presence.
|
||||
- **AC-OFFICE-COORDINATOR-INSTALL-001.2:** Installing the coordinator routine when a
|
||||
routine with that identity already exists shall return the existing routine and
|
||||
shall not create a second one, whatever its schedule state.
|
||||
- **AC-OFFICE-COORDINATOR-INSTALL-001.3:** When more than one existing routine
|
||||
satisfies that identity, the install shall return the one with the earliest
|
||||
`office_routines.created_at`, breaking ties by `office_routines.id` ascending.
|
||||
It shall delete none of them and shall report that more than one was found. The
|
||||
routine it selects shall then be subject to
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.4 and AC-OFFICE-COORDINATOR-INSTALL-001.5
|
||||
exactly as a single match would be, so a duplicated half-finished install is
|
||||
completed rather than left permanently unable to fire. The routines it did not
|
||||
select shall be left entirely untouched, including their triggers.
|
||||
- **AC-OFFICE-COORDINATOR-INSTALL-001.4:** Installing the coordinator routine when a
|
||||
routine with that identity exists with no cron trigger shall create the
|
||||
canonical cron trigger for it and return the existing routine, completing an
|
||||
install whose trigger creation previously failed. The created trigger shall
|
||||
carry the canonical cron expression, the canonical timezone, `enabled = true`,
|
||||
and a `next_run_at` equal to the first occurrence of the canonical expression
|
||||
evaluated in the canonical timezone strictly after a single instant captured
|
||||
once at the start of the install. A repaired trigger whose `next_run_at` is
|
||||
null is not a completed install: it classifies `trigger_unscheduled` and still
|
||||
cannot fire, which is the state this criterion exists to leave behind. When
|
||||
that next occurrence cannot be computed, the install shall be rejected and
|
||||
shall report the failure, leaving the existing routine in place; because the
|
||||
expression and timezone are the installer's own constants, that failure is a
|
||||
defect in the constants and no input can reach it. When that trigger creation fails, the install shall report
|
||||
the failure and shall leave the existing routine in place rather than deleting
|
||||
it, so that the next install reaches this same criterion and completes the
|
||||
schedule. This criterion is therefore re-entrant: running it again against a
|
||||
routine it already repaired takes the
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.5 branch and changes nothing.
|
||||
- **AC-OFFICE-COORDINATOR-INSTALL-001.5:** Installing the coordinator routine when a
|
||||
routine with that identity exists with any cron trigger, enabled or disabled,
|
||||
canonical expression or not, shall leave every trigger unchanged, shall create
|
||||
no second trigger, and shall report the routine's schedule state.
|
||||
- **AC-OFFICE-COORDINATOR-INSTALL-001.6:** Installing the coordinator routine shall
|
||||
never change an existing routine's `status`.
|
||||
- **AC-OFFICE-COORDINATOR-INSTALL-001.7:** When the identity lookup itself fails, or
|
||||
when the read of the existing routine's triggers that decides between
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.4 and AC-OFFICE-COORDINATOR-INSTALL-001.5
|
||||
fails, the install shall be rejected and shall create nothing. An indeterminate
|
||||
read shall never be treated as "absent": neither as an absent routine, which
|
||||
duplicates the routine, nor as an absent trigger, which duplicates the
|
||||
trigger.
|
||||
- **AC-OFFICE-COORDINATOR-INSTALL-001.8:** Two concurrent installs for the same
|
||||
workspace and assignee shall between them create at most one new coordinator
|
||||
routine and at most one new canonical cron trigger, whichever order the two
|
||||
callers interleave. This bounds what the install *creates*; it is not a
|
||||
uniqueness guarantee over the table. Duplicate routines a previous install
|
||||
already created remain, and are handled by
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.3; triggers that already exist remain, and
|
||||
are handled by AC-OFFICE-COORDINATOR-INSTALL-001.5.
|
||||
- **AC-OFFICE-COORDINATOR-INSTALL-001.9:** No database uniqueness constraint shall
|
||||
be added to enforce
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.8. Upgraded installs already hold duplicate
|
||||
coordinator routines, so a unique index over the identity columns would fail to
|
||||
apply on exactly the installs this document exists to fix, and the alternative
|
||||
of deleting rows to make it apply is out of scope below. The guarantee shall
|
||||
instead be met by serializing the check-and-create against concurrent installs
|
||||
for the same workspace and assignee. The domain of that serialization shall be
|
||||
every installer call that can reach the same database, not merely every call
|
||||
within one process: the guarantee shall hold when two processes share a
|
||||
database, which rules out a purely in-process guard as the sole mechanism. It
|
||||
shall hold on both dialects named in
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.11, which rules out any mechanism available
|
||||
on only one of them. Acquisition shall not block indefinitely: it shall either
|
||||
acquire or fail within a single named compile-time bound of 5 seconds, and that
|
||||
bound shall not be operator-configurable — no environment variable,
|
||||
configuration key, or runtime flag shall expose it — because both of the
|
||||
installer's callers run on a user-facing onboarding path, where an unbounded
|
||||
wait is indistinguishable from a hang.
|
||||
- **AC-OFFICE-COORDINATOR-INSTALL-001.10:** The reports required by
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.3 and
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.5 shall be a structured record plus a counter
|
||||
per condition, not an additional value returned from the install call. The
|
||||
routine the call already returns under
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.2 and
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.3 is unchanged; what this criterion forbids
|
||||
is carrying the *reports* out that way. Both of the installer's callers today
|
||||
discard everything the call returns except the error, and neither is being
|
||||
changed to inspect a richer return, so a report delivered as a return value
|
||||
would report to nobody. Each record shall name the workspace, the assignee
|
||||
agent, and the routine it concerns — for
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.3, the routine it selected, and the number of
|
||||
routines that matched. Each condition that applies shall be reported exactly
|
||||
once per install call. A call that is rejected shall still report every
|
||||
condition it had already detected before rejecting, in particular the
|
||||
duplicate-routines condition of AC-OFFICE-COORDINATOR-INSTALL-001.3, which
|
||||
describes state the call found in the database rather than work the call
|
||||
performed. These counters therefore count the calls in which a condition was
|
||||
observed, not the installs that completed, so a duplicated coordinator is
|
||||
reported on the runs that fail as well as the runs that succeed.
|
||||
- **AC-OFFICE-COORDINATOR-INSTALL-001.11:** The install shall behave identically on
|
||||
SQLite and on PostgreSQL.
|
||||
- **AC-OFFICE-COORDINATOR-INSTALL-001.12:** Installing the coordinator routine when
|
||||
no routine with that identity exists shall create one routine carrying that
|
||||
identity, with `status = 'active'`, together with its canonical cron trigger
|
||||
carrying every field required by AC-OFFICE-COORDINATOR-INSTALL-001.4, and shall
|
||||
return the created routine. When the routine insert fails, the install shall be
|
||||
rejected, shall create no trigger, and shall report the failure. When the
|
||||
routine insert succeeds and the trigger creation fails, the install shall leave
|
||||
the created routine in place and report the failure, so that the next install
|
||||
reaches AC-OFFICE-COORDINATOR-INSTALL-001.4 and completes the schedule; this is
|
||||
the half-finished install this document exists to make recoverable, and it is
|
||||
reached by the same path whether the first install failed here or before this
|
||||
document existed.
|
||||
- **AC-OFFICE-COORDINATOR-INSTALL-001.13:** When the serialization required by
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.9 cannot be acquired, or the serialized
|
||||
section fails because a concurrent install held it, the install shall be
|
||||
rejected, shall create nothing, and shall report contention distinguishably
|
||||
from the failures of AC-OFFICE-COORDINATOR-INSTALL-001.7. It shall not retry
|
||||
within the same call. A rejected install leaves the routine the winning caller
|
||||
created, so the next install returns it under
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.2; contention therefore costs a warning, not
|
||||
a duplicate and not a lost coordinator. Reaching the acquisition bound of
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.9 without acquiring is contention and shall be
|
||||
reported as contention. Cancellation of the caller's context while waiting shall
|
||||
likewise reject the install and create nothing, and shall be reported
|
||||
distinguishably from both contention and the failures of
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.7, so that a shutdown part-way through
|
||||
onboarding is not recorded as a duplicate-install race.
|
||||
|
||||
- **AC-OFFICE-COORDINATOR-INSTALL-001.14:** The install shall reject a call whose
|
||||
workspace, assignee agent, or canonical routine name is empty, before performing
|
||||
the identity lookup, and shall create nothing. An empty value is not an
|
||||
identity: accepting one lets a malformed call match, or create, a routine that
|
||||
no real workspace or agent owns, and AC-OFFICE-COORDINATOR-INSTALL-001.1 makes
|
||||
those three values the whole of identity. This keeps the guard the installer
|
||||
applies today and extends it to the third component. The rejection shall be
|
||||
reported distinguishably from the read failures of
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.7, because nothing was read.
|
||||
- **AC-OFFICE-COORDINATOR-INSTALL-001.15:** No failure in this requirement shall
|
||||
be retried inside the same install call: not the identity lookup or the trigger
|
||||
read of AC-OFFICE-COORDINATOR-INSTALL-001.7, not the trigger creation of
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.4, not the routine insert or trigger creation
|
||||
of AC-OFFICE-COORDINATOR-INSTALL-001.12, and not the serialization of
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.9 and
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.13. Each rejection returns to its caller, and
|
||||
the next install is what reaches the criterion that completes the work; that is
|
||||
the recovery model the rest of this requirement is built on, and an in-call
|
||||
retry would only lengthen a failure on an onboarding path. A rejected install
|
||||
shall surface its failure as an error, because the error is the only channel
|
||||
either caller reads. It may also return the routine it found or created
|
||||
alongside that error, as the installer does today: no criterion here depends on
|
||||
that value, so Build is free to keep or drop it.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- **Defining or repairing a schedule.** The classification belongs to
|
||||
[Office Routine Arming Visibility](routine-arming-visibility.md). This document
|
||||
*consumes* it — AC-OFFICE-COORDINATOR-INSTALL-001.5 reports schedule state, so
|
||||
reporting is in scope and only the definition is not. It creates a canonical
|
||||
trigger where none exists and otherwise touches no trigger.
|
||||
- **Repairing a coordinator routine that is present but unarmed.** An install
|
||||
reports the schedule state and stops. It does not enable a disabled trigger and
|
||||
does not re-schedule a trigger whose `next_run_at` is null, even its own
|
||||
canonical one, because it cannot tell an operator's deliberate choice from a
|
||||
defect. The operator-initiated action that would repair it is deferred; see
|
||||
that document's `## Out of scope`, which cites the successor card.
|
||||
- **Merging or deleting duplicate coordinator routines** created by an earlier
|
||||
install. The install reports that more than one was found and deletes none;
|
||||
deciding which to keep needs an operator. This is what makes the uniqueness
|
||||
constraint of AC-OFFICE-COORDINATOR-INSTALL-001.9 unavailable, and the two
|
||||
decisions stand or fall together.
|
||||
- **Correcting a coordinator trigger whose cron expression an operator edited.**
|
||||
Identity stops depending on the expression; the edited value is left alone.
|
||||
- **Rolling back a half-finished install.** This document completes one on the
|
||||
next run rather than unwinding it, and does not change either caller's existing
|
||||
choice to warn-log a failed install and continue.
|
||||
- **Which serialization mechanism is used.**
|
||||
AC-OFFICE-COORDINATOR-INSTALL-001.9 states the property, its domain, and the
|
||||
two constraints that narrow the choice; AC-OFFICE-COORDINATOR-INSTALL-001.13
|
||||
states what contention does. Which conforming mechanism Build picks is its
|
||||
decision. The three candidates are not interchangeable and this document does
|
||||
not offer them as though they were: a transaction at the default isolation
|
||||
level of either dialect does not by itself stop two callers both reading
|
||||
"absent" and both inserting, a PostgreSQL advisory lock has no SQLite
|
||||
equivalent, and an in-process guard fails the cross-process domain. Whatever is
|
||||
chosen must satisfy .9 and .13 as written.
|
||||
- **Other pre-installed routines.** Onboarding installs one routine today. A
|
||||
second would need this contract restated for it, not inherited silently.
|
||||
- **Whether the coordinator routine should exist at all,** and what its canonical
|
||||
cron expression should be. This document keeps the install idempotent; it does
|
||||
not revisit the values being installed.
|
||||
@@ -0,0 +1,174 @@
|
||||
---
|
||||
status: draft
|
||||
system: office
|
||||
created: 2026-09-17
|
||||
owners:
|
||||
- kandev
|
||||
---
|
||||
|
||||
# Office Routine Arming Startup Scan Requirements
|
||||
|
||||
## Overview
|
||||
|
||||
The install this capability exists for is unattended: nobody opens the routines
|
||||
page for weeks, so the read-path surface of
|
||||
[Office Routine Arming Visibility](routine-arming-visibility.md) leaves a dead
|
||||
loop undetected exactly where detection matters most. This document owns the
|
||||
read-only scan that runs once at Office startup and reports what the classifier
|
||||
found, to logs and to counters.
|
||||
|
||||
It classifies nothing of its own: schedule state, the unarmed cron trigger list,
|
||||
and the dispatch grace are all defined by that document, and this one consumes
|
||||
them. It was authored inside it and moved here unchanged in identity to stay
|
||||
inside the requirement-file size limit: the requirement is still
|
||||
`REQ-OFFICE-ROUTINE-ARMING-003` and its criteria are still numbered
|
||||
`AC-OFFICE-ROUTINE-ARMING-003.1` onward, so every reference to them elsewhere
|
||||
still resolves.
|
||||
|
||||
## Terminology
|
||||
|
||||
This document uses the terms defined in
|
||||
[Office Routine Arming Visibility § Terminology](routine-arming-visibility.md),
|
||||
unchanged: *fire*, *intent*, *schedule state*, *armed*, *schedulable*, *dispatch
|
||||
grace*, *trigger order*, *unarmed cron trigger*, and *surface*.
|
||||
|
||||
## Requirements
|
||||
|
||||
### REQ-OFFICE-ROUTINE-ARMING-003: Surface unarmed routines at startup
|
||||
|
||||
**Intent:** The install this capability exists for is unattended. Nobody opens
|
||||
the routines page for weeks, so a UI-only surface leaves the dead loop undetected
|
||||
exactly where detection matters most.
|
||||
|
||||
**User story:** As an operator reading logs or metrics after an upgrade, I want a
|
||||
routine that is switched on and cannot fire to have left a trace.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-ROUTINE-ARMING-003.1:** During Office startup the system shall
|
||||
classify every routine it can enumerate, in every workspace it can enumerate,
|
||||
and report the result. The scan shall read only after Office startup
|
||||
reconciliation has returned — the pass that gives every trigger-less routine a
|
||||
`manual` trigger — whether or not that pass succeeded, so that a routine the
|
||||
scan reports as `unscheduled_no_trigger` is one reconciliation left without a
|
||||
trigger rather than one the scan happened to read first. The ordering shall be
|
||||
established by an explicit completion signal handed to the scan by the startup
|
||||
sequence that ran reconciliation, and by nothing else: not a timer, not a retry,
|
||||
and not an inspection of trigger rows. The scan is either invoked with that
|
||||
signal or invoked without it. Invoked without it, the scan shall classify
|
||||
nothing, emit a warning-level record, and increment the skipped counter of
|
||||
AC-OFFICE-ROUTINE-ARMING-003.7 with a reason distinguishing this cause both from
|
||||
enumeration failure and from per-routine classification failure. The startup
|
||||
path that exists today always supplies the signal, so that branch is not reached
|
||||
in production; it exists so that a caller which cannot establish the ordering is
|
||||
refused rather than reporting a routine as `unscheduled_no_trigger` before
|
||||
reconciliation has given it a trigger, and it is reached in a test by invoking
|
||||
the scan without the signal.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-003.2:** The scan shall iterate workspaces ordered
|
||||
by `workspaces.created_at` ascending, breaking ties by `workspaces.id`
|
||||
ascending, and within each workspace shall iterate routines ordered by
|
||||
`office_routines.created_at` ascending, breaking ties by `office_routines.id`
|
||||
ascending, so that two scans over unchanged data report in the same order.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-003.3:** For a routine whose intent is `active` and
|
||||
whose schedule state is `trigger_disabled`, `trigger_unscheduled` or
|
||||
`trigger_invalid`, the system shall emit a warning-level record naming the
|
||||
workspace, the routine, the schedule state, and every unarmed cron trigger in
|
||||
trigger order.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-003.4:** For a routine whose intent is not `active`,
|
||||
the system shall emit an informational record stating that the routine is
|
||||
deliberately off, and shall not emit a warning.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-003.5:** For a routine whose intent is `active` and
|
||||
whose schedule state is neither one of those in
|
||||
AC-OFFICE-ROUTINE-ARMING-003.3 nor one of those in
|
||||
AC-OFFICE-ROUTINE-ARMING-003.14 nor `unknown`, the system shall emit an
|
||||
informational record and no warning.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-003.6:** The system shall publish a counter of scan
|
||||
observations labelled by workspace, intent, and schedule state.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-003.7:** When the scan cannot classify a routine it
|
||||
enumerated, the system shall increment a skipped counter labelled by reason, so
|
||||
that a scan failing closed is visible rather than silent.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-003.8:** When the scan cannot enumerate the
|
||||
workspace list at all, it shall increment that skipped counter with a reason
|
||||
distinguishing enumeration failure from per-routine classification failure,
|
||||
emit a warning-level record, and return without classifying anything.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-003.9:** A routine whose schedule state is `unknown`
|
||||
shall emit an informational record naming the workspace, the routine, and the
|
||||
`unknown` state, whatever its intent, and shall not emit a warning. An unreadable
|
||||
row is not evidence of a broken routine. That record is the only one emitted for
|
||||
such a routine: it stands in place of the record
|
||||
AC-OFFICE-ROUTINE-ARMING-003.4 would otherwise require when the intent is not
|
||||
`active`, so every routine the scan reaches produces exactly one record. The
|
||||
routine shall increment the skipped counter of AC-OFFICE-ROUTINE-ARMING-003.7
|
||||
once, and shall also be counted once by the observation counter of
|
||||
AC-OFFICE-ROUTINE-ARMING-003.6 under the `unknown` schedule state. Incrementing
|
||||
both is intended, not double counting: the two answer different questions — how
|
||||
many routines were observed in each state, and how many the scan could not
|
||||
decide.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-003.10:** Office startup shall complete without
|
||||
waiting for the scan to finish, and no scan outcome, including a database
|
||||
error, shall fail startup or change its result.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-003.11:** The scan shall write nothing, per
|
||||
AC-OFFICE-ROUTINE-ARMING-001.7, and shall not enable, disable, create, or
|
||||
delete a trigger.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-003.12:** Running the scan twice over unchanged data
|
||||
shall emit the same records, in the same order, and shall leave the database
|
||||
unchanged. Two records are the same when they carry the same level, the same
|
||||
workspace, the same routine, the same schedule state, and the same unarmed cron
|
||||
triggers in the same order; wall-clock timestamps, durations, and fields the
|
||||
logging layer adds are excluded from the comparison, as are counters, which
|
||||
accumulate. The equality is asserted only where classification does not depend
|
||||
on where the classification instant falls relative to the dispatch grace: a
|
||||
routine is comparable between two scans when each of its cron triggers is on
|
||||
the same side of the grace at both classification instants of
|
||||
AC-OFFICE-ROUTINE-ARMING-001.12, where a null `last_fired_at` counts as outside
|
||||
the grace at every instant. A trigger inside the grace at one scan and
|
||||
outside it at the other changes schedule state with no database change, by the
|
||||
design of rule 2, and is excluded from the comparison rather than counted as a
|
||||
violation. The equality is likewise asserted only between two scans that each
|
||||
enumerated the workspace list and every workspace's routines successfully and
|
||||
classified every routine they enumerated: a scan in which
|
||||
AC-OFFICE-ROUTINE-ARMING-003.8, AC-OFFICE-ROUTINE-ARMING-003.13, or a
|
||||
per-routine classification failure occurred emits a different set of records with
|
||||
no database change, by the design of those criteria, and is excluded from the
|
||||
comparison. This criterion constrains classification, not the database's
|
||||
willingness to answer.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-003.14:** For a routine whose intent is `active` and
|
||||
whose schedule state is `unscheduled_manual_only` or `unscheduled_no_trigger`,
|
||||
the system shall emit a warning-level record naming the workspace, the routine,
|
||||
and the schedule state, and stating that the routine has no schedule. The record
|
||||
shall carry no unarmed cron trigger list, because a routine in either state owns
|
||||
no cron trigger. This is the state
|
||||
[Office Routine Arming Visibility](routine-arming-visibility.md) names as the
|
||||
sharper failure — switched on, presenting as on, incapable of firing — which is
|
||||
why it warns rather than informs. The scan cannot tell a routine an operator
|
||||
fires by hand from one whose schedule was never created, and this criterion does
|
||||
not ask it to: either way the routine will not fire on a schedule, the operator
|
||||
is the only one who can say which was meant, and the cost of saying so is one
|
||||
warning per such routine per startup.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-003.13:** When the scan enumerates the workspace
|
||||
list but then fails to enumerate the routines of one workspace, it shall retain
|
||||
every record already emitted for earlier workspaces, increment the skipped
|
||||
counter of AC-OFFICE-ROUTINE-ARMING-003.7 once for that workspace with the
|
||||
enumeration-failure reason, emit a warning-level record naming it, and continue
|
||||
to the next workspace in the order of
|
||||
AC-OFFICE-ROUTINE-ARMING-003.2. It shall not retry that workspace within the
|
||||
same scan and shall not abandon the scan.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- **Classifying a routine.** The classification table, the unarmed cron trigger
|
||||
list, and the dispatch grace belong to
|
||||
[Office Routine Arming Visibility](routine-arming-visibility.md). This document
|
||||
reports what that classifier returns and defines no state of its own.
|
||||
- **Repair of any kind.** The scan writes nothing, per
|
||||
AC-OFFICE-ROUTINE-ARMING-003.11. It does not re-arm, and the operator-initiated
|
||||
re-arm action is deferred entirely — see that document's `## Out of scope`.
|
||||
- **Running the scan again.** This is one pass at startup. A periodic re-scan, an
|
||||
operator-invoked re-scan, and an API that returns the scan's findings are each
|
||||
separate work; the read-path response of
|
||||
REQ-OFFICE-ROUTINE-ARMING-002 already reports the same values on demand.
|
||||
- **Operator notification.** Inbox items, mail, and paging are separate work. The
|
||||
scan emits records and counters and stops.
|
||||
- **What startup reconciliation itself does.** AC-OFFICE-ROUTINE-ARMING-003.1
|
||||
orders the scan after it and reports the result; it does not change which
|
||||
triggers reconciliation creates.
|
||||
@@ -0,0 +1,318 @@
|
||||
---
|
||||
status: draft
|
||||
system: office
|
||||
created: 2026-09-17
|
||||
owners:
|
||||
- kandev
|
||||
---
|
||||
|
||||
# Office Routine Arming Visibility Requirements
|
||||
|
||||
## Overview
|
||||
|
||||
An Office routine fires on its own only when a cron trigger reaches the
|
||||
scheduler. Several independent states stop that happening, and none is reported
|
||||
anywhere: the operator sees a routine badged "On" and cannot tell a running loop
|
||||
from one somebody deliberately turned off, or from one silently dead.
|
||||
|
||||
This capability makes the distinction **visible** and repairs nothing.
|
||||
Operator-initiated repair was specified alongside it and deferred to a separate
|
||||
card; see `## Out of scope`. Until that lands, an operator shown a broken
|
||||
schedule still repairs it by hand.
|
||||
|
||||
The live install at spec time has zero armed cron triggers anywhere: one routine
|
||||
is `paused` with a disabled `cron` trigger and renders "Off", and one is `active`
|
||||
with only a `manual` trigger, so it renders **"On"**, has no schedule at all, and
|
||||
does not fire. The second is the sharper failure: switched on, presenting as on,
|
||||
incapable of firing.
|
||||
|
||||
Every term in this contract is an Office primitive: `office_routines.status`,
|
||||
`office_routine_triggers`, and the dispatch tick in `internal/office/routines`.
|
||||
|
||||
This document owns the classification and the read path. The startup scan that
|
||||
reports the same classification to logs and counters on an unattended install is
|
||||
`REQ-OFFICE-ROUTINE-ARMING-003`, which lives in
|
||||
[Office Routine Arming Startup Scan](routine-arming-startup-scan.md) to stay
|
||||
inside the requirement-file size limit. The two ship together.
|
||||
|
||||
## Prior art
|
||||
|
||||
No external knowledge base was reachable, so the prior art below is in-repo.
|
||||
|
||||
[Office Stall Visibility](stall-visibility.md) is the same shape: a state no
|
||||
detector reports, made visible without granting anything permission to recover
|
||||
it. This document reuses its `Surface` definition, its detection/recovery split,
|
||||
and its structured-log-plus-`expvar` convention, and differs in one way: stall
|
||||
recovery is forbidden on principle, whereas here recovery is legitimate and
|
||||
merely deferred, which is why `## Out of scope` names a successor card rather
|
||||
than closing the question.
|
||||
|
||||
## Terminology
|
||||
|
||||
- **Fire:** the scheduler tick claims a trigger and dispatches a routine run. A
|
||||
routine that fires and finds no work has fired.
|
||||
- **Intent:** the operator's on/off choice, held in `office_routines.status`.
|
||||
`active` means run this routine; any other value means deliberately off.
|
||||
- **Schedule state:** whether the routine's triggers can fire it on a schedule,
|
||||
computed from the trigger rows alone, without reference to intent.
|
||||
- **Armed:** schedule state `armed` — some cron trigger can fire the routine on
|
||||
a schedule with no further operator action.
|
||||
- **Schedulable:** a cron trigger for which the shared scheduler can compute a
|
||||
next occurrence. The expression and timezone must be valid, and the
|
||||
expression must name at least one real calendar slot. The scheduler's
|
||||
day-of-month/day-of-week OR rule and DST handling apply here too. A trigger
|
||||
that fails these checks is not schedulable, and no amount of re-arming makes
|
||||
it fire.
|
||||
- **Dispatch grace:** a named duration of 60 seconds, separating a trigger
|
||||
mid-dispatch from one permanently stuck. See
|
||||
AC-OFFICE-ROUTINE-ARMING-001.10.
|
||||
- **Trigger order:** `created_at` ascending, ties broken by `id` ascending. Every
|
||||
ordered output over triggers in this document uses it.
|
||||
- **Unarmed cron trigger:** a cron trigger of the routine that cannot currently
|
||||
fire, carrying the reasons why. See AC-OFFICE-ROUTINE-ARMING-001.8.
|
||||
- **Surface:** report a state to an operator without changing routine, trigger,
|
||||
or run state.
|
||||
|
||||
Intent and schedule state are two switches, and this document owns the
|
||||
classification and reporting of the second one. Classification reads trigger
|
||||
rows only. Dispatch uses the routine status separately: the cron path checks
|
||||
the status before it claims a slot, and the manual and webhook paths reject a
|
||||
non-firing status. This document reports both values and does not change those
|
||||
dispatch gates.
|
||||
|
||||
## Requirements
|
||||
|
||||
### REQ-OFFICE-ROUTINE-ARMING-001: Classify a routine's schedule state
|
||||
|
||||
**Intent:** Every unarmed routine is unarmed for exactly one reason, and the
|
||||
reason determines what the operator must do next. A single boolean cannot carry
|
||||
that, and a state computed from intent cannot separate deliberate from broken.
|
||||
|
||||
**User story:** As an operator, I want each routine to report why it will or will
|
||||
not fire, so I can tell a paused loop from a dead one.
|
||||
|
||||
Schedule state is a total function of the routine's trigger rows alone. The first
|
||||
matching rule wins; the ordering is the contract, not an implementation detail.
|
||||
|
||||
| # | Condition on the routine's triggers | State |
|
||||
|---|---|---|
|
||||
| 0 | The trigger rows could not be read | `unknown` |
|
||||
| 1 | Some `cron` trigger has `enabled = true` and `next_run_at` non-null | `armed` |
|
||||
| 2 | Some `cron` trigger has `enabled = true`, `next_run_at` null, and `last_fired_at` no older than the dispatch grace | `armed` |
|
||||
| 3 | Some `cron` trigger has `enabled = true` and is not schedulable | `trigger_invalid` |
|
||||
| 4 | Some `cron` trigger has `enabled = true` | `trigger_unscheduled` |
|
||||
| 5 | At least one `cron` trigger exists | `trigger_disabled` |
|
||||
| 6 | Some `webhook` trigger has `enabled = true`, and no `cron` trigger exists | `event_only` |
|
||||
| 7 | At least one trigger exists, none of kind `cron` | `unscheduled_manual_only` |
|
||||
| 8 | No trigger rows exist | `unscheduled_no_trigger` |
|
||||
|
||||
Rule 2 exists because the scheduler clears `next_run_at` when it claims a trigger
|
||||
and restores it a few statements later; without it a healthy routine that happens
|
||||
to be mid-dispatch classifies as broken. Rules 3 and 4 therefore describe an
|
||||
enabled cron trigger whose `next_run_at` has been null for longer than that,
|
||||
which is a permanent stall. Rule 3 precedes rule 4 so the more actionable
|
||||
diagnosis wins when a routine owns an unschedulable enabled cron trigger
|
||||
alongside a merely unscheduled one. Rule 5 is reached only when no cron trigger is
|
||||
enabled, because rules 1 to 4 between them match every routine that has one.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-ROUTINE-ARMING-001.1:** The system shall assign every routine
|
||||
exactly one schedule state from the table above, applying the rules in the
|
||||
order given and stopping at the first match.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-001.2:** The system shall compute schedule state
|
||||
without reading `office_routines.status`, so that a routine's schedule state is
|
||||
identical whether its intent is `active` or not.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-001.3:** A cron trigger with `enabled = true` whose
|
||||
`next_run_at` is in the past shall match rule 1. A past-due trigger is due, not
|
||||
broken. This says how rule 1 reads a past instant and does not override the
|
||||
table: the same trigger with `enabled = false` reaches rule 5.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-001.4:** A cron trigger with `enabled = true` whose
|
||||
`next_run_at` is exactly the classification instant of
|
||||
AC-OFFICE-ROUTINE-ARMING-001.12 shall match rule 1, on the same terms.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-001.5:** When reading the routine's trigger rows
|
||||
fails, the system shall report `unknown` and shall not report `armed`. A row
|
||||
the system cannot decode is a row it could not read.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-001.6:** The system shall produce the same schedule
|
||||
state for the same trigger rows on SQLite and on PostgreSQL.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-001.7:** Classification shall write no routine,
|
||||
trigger, or run state.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-001.8:** Alongside the schedule state, the system
|
||||
shall report the routine's unarmed cron triggers: every cron trigger that is
|
||||
`enabled = false`, or is not schedulable, or is `enabled = true` and
|
||||
schedulable with a null `next_run_at` and a `last_fired_at` either null or
|
||||
older than the dispatch grace. Each entry shall carry the trigger's `id` and
|
||||
**every** one of those three reasons that applies to it, ordered as the reasons
|
||||
are listed in this criterion; a single entry may carry more than one. The list
|
||||
shall be in trigger order.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-001.9:** A routine may be `armed` and still report a
|
||||
non-empty unarmed cron trigger list: rule 1 matches on one trigger while
|
||||
another remains broken.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-001.10:** The dispatch grace shall be a single named
|
||||
compile-time constant of 60 seconds, used identically by classification and by
|
||||
the startup scan. It shall not be operator-configurable: no environment
|
||||
variable, configuration key, or runtime flag shall expose it.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-001.11:** A cron trigger with `enabled = true`, a
|
||||
null `next_run_at`, and a null `last_fired_at` shall not match rule 2. A
|
||||
trigger that has never fired is not mid-dispatch. Schedule state is the
|
||||
routine's and is decided by first match across all its triggers, so such a
|
||||
trigger sets the routine's state under rule 3 or rule 4 only when no sibling
|
||||
matched an earlier rule; a sibling matching rule 1 leaves the routine `armed`
|
||||
and puts this one on the unarmed cron trigger list, per
|
||||
AC-OFFICE-ROUTINE-ARMING-001.9.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-001.12:** One classification of one routine shall
|
||||
read that routine's trigger rows once and derive both the schedule state and
|
||||
the unarmed cron trigger list from that single read, and shall compare every
|
||||
`last_fired_at` against one instant captured once for that classification, so
|
||||
that no two triggers of a routine are judged against different clocks. The
|
||||
bounded read is the one the classification derives its answer from: when a batch
|
||||
read fails and the per-routine re-read of AC-OFFICE-ROUTINE-ARMING-001.13
|
||||
supplies the rows, that re-read is the single read, and the instant stays the one
|
||||
captured for the classification the re-read belongs to rather than being
|
||||
re-captured per routine. A failed read derives nothing and does not count against
|
||||
this criterion.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-001.13:** A trigger-read failure shall make
|
||||
`unknown` exactly those routines whose own trigger rows were not read. When one
|
||||
query reads the trigger rows of several routines and that query fails, or one
|
||||
row in its result cannot be decoded, the system shall re-read each routine of
|
||||
that batch individually and report `unknown` only for those whose individual
|
||||
read also fails, so one undecodable row makes at most its own routine
|
||||
`unknown`, never every routine in the workspace. That re-read shall be
|
||||
attempted at most once per routine per classification.
|
||||
|
||||
### REQ-OFFICE-ROUTINE-ARMING-002: Report intent and schedule state on routine reads
|
||||
|
||||
**Intent:** The operator's only view of a routine today is a badge driven by
|
||||
intent alone, which is why a routine with no schedule renders as "On". Both
|
||||
values must arrive in one response, so the surface cannot show one without the
|
||||
other.
|
||||
|
||||
**User story:** As an operator opening the routines list, I want to see both
|
||||
whether a routine is switched on and whether it can actually fire.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-ROUTINE-ARMING-002.1:** The routine list response and the single
|
||||
routine response shall each carry, per routine, the intent value, the schedule
|
||||
state from REQ-OFFICE-ROUTINE-ARMING-001, and the unarmed cron trigger list
|
||||
from AC-OFFICE-ROUTINE-ARMING-001.8.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-002.2:** The routines list shall distinguish a
|
||||
routine whose intent is `active` and whose schedule state is `armed` from one
|
||||
whose intent is `active` and whose schedule state is anything else.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-002.3:** The routines list shall distinguish a
|
||||
routine whose intent is not `active` from a routine whose intent is `active`
|
||||
but which cannot fire, so that deliberate and broken never render the same.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-002.4:** The surface shall distinguish a routine
|
||||
whose unarmed cron trigger list holds at least one schedulable entry from one
|
||||
whose unarmed entries are none of them schedulable, whatever its schedule
|
||||
state: the first needs only its triggers re-armed, the second needs its cron
|
||||
expression or timezone edited. A routine `armed` on one trigger and broken on
|
||||
another still carries the distinction. The distinction applies only where the
|
||||
unarmed cron trigger list is non-empty: over an empty list "none of them
|
||||
schedulable" holds vacuously, so a routine with an empty list shall render
|
||||
neither side of it rather than be told to edit an expression it has no problem
|
||||
with. Neither rendering shall offer an action that performs the repair;
|
||||
see `## Out of scope`.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-002.5:** For a routine whose schedule state is
|
||||
`unscheduled_manual_only` or `unscheduled_no_trigger`, the surface shall report
|
||||
that the routine has no schedule.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-002.6:** A routine whose schedule state is `unknown`
|
||||
shall render as undetermined rather than as armed or as broken.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-002.7:** The routine list response shall be produced
|
||||
with a number of database queries that does not grow with the number of
|
||||
routines in the workspace. This bounds the path where every read succeeds; the
|
||||
per-routine re-read AC-OFFICE-ROUTINE-ARMING-001.13 requires after a failed
|
||||
batch read is exempt, at most one extra read per routine of that batch, because
|
||||
isolating the bad routine is worth more than the query count on a path that is
|
||||
already failing.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-002.8:** All copy added by this capability shall be
|
||||
routed through the translation layer and present in every shipped locale.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-002.9:** Every distinction this requirement demands
|
||||
shall be carried by text, an icon, or an accessible label, never by colour
|
||||
alone.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-002.10:** The surface shall render a distinct label
|
||||
for each of these groups: `armed`; `trigger_invalid`, `trigger_unscheduled` or
|
||||
`trigger_disabled`; `event_only`; `unscheduled_manual_only` or
|
||||
`unscheduled_no_trigger`; and `unknown`. Two routines in different groups shall
|
||||
never render the same label.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-002.11:** A routine whose schedule state is
|
||||
`event_only` shall not be reported as having no schedule and shall not be
|
||||
directed to create a trigger. It fires when its event arrives.
|
||||
- **AC-OFFICE-ROUTINE-ARMING-002.12:** Intent and schedule state shall be carried
|
||||
in the same response for the same routine, and neither shall be omitted because
|
||||
the other was unavailable: a routine whose triggers could not be read carries
|
||||
its intent alongside schedule state `unknown`. The pair is not required to be a
|
||||
transactional snapshot. Intent is read from the routine row and schedule state
|
||||
from the trigger rows, so a write landing between those reads can yield a pair
|
||||
never simultaneously true in the database. That is accepted, not prevented:
|
||||
nothing here acts on the pair, and the next read reports the new value.
|
||||
|
||||
## Out of scope
|
||||
|
||||
Each exclusion is a contract: a later change that wants one needs its own
|
||||
requirement.
|
||||
|
||||
- **The startup scan.** Not excluded from the capability, only from this file:
|
||||
`REQ-OFFICE-ROUTINE-ARMING-003` is in
|
||||
[Office Routine Arming Startup Scan](routine-arming-startup-scan.md) and is
|
||||
part of the same delivery.
|
||||
- **Repair of any kind.** Nothing here enables, disables, creates, or deletes a
|
||||
trigger, and nothing here changes intent. Not automatically, and not on an
|
||||
operator's request either — see the two entries below.
|
||||
- **The operator-initiated re-arm action. Deferred to task
|
||||
`b0382916-da13-44a5-85be-064ae6a9533c`.** A requirement for it was written
|
||||
alongside this document and cut before Build, so this surface reports a broken
|
||||
schedule and offers nothing that repairs it. What the successor needs: an
|
||||
explicit action over one named routine, setting `enabled = true` and a fresh
|
||||
`next_run_at` on exactly that routine's unarmed cron triggers that are
|
||||
schedulable, in trigger order, in one transaction with no partial application;
|
||||
never creating a trigger, never inferring a cron expression, never
|
||||
re-scheduling an already-armed trigger, and never reading intent to decide what
|
||||
to repair. It must keep four rejections distinct: no cron trigger to repair
|
||||
(including `event_only`, which per AC-OFFICE-ROUTINE-ARMING-002.11 must not be
|
||||
told to create one); cron triggers present but none schedulable; caller not
|
||||
authorized for the owning workspace; no such routine. It consumes
|
||||
AC-OFFICE-ROUTINE-ARMING-001.8's unarmed list and this document's
|
||||
classification, so it depends on this document and not the reverse. Three
|
||||
contract defects in the cut text travelled to that card unfixed and are
|
||||
recorded there.
|
||||
- **Changing intent gating.** Routine status gating is owned by the routine
|
||||
status requirement. The existing scheduler consumes a due cron slot when the
|
||||
routine is not allowed to fire, and the manual and webhook handlers reject
|
||||
non-firing statuses. This capability only reports the status and schedule
|
||||
state; it does not change those gates.
|
||||
- **The coordinator install.** Keeping onboarding from duplicating the
|
||||
pre-installed routine is
|
||||
[Office Coordinator Install Idempotency](coordinator-install-idempotency.md).
|
||||
- **Cron evaluation changes.** Cron parsing and next-occurrence calculation use
|
||||
the shared `NextCronTime` implementation. It follows `crontab(5)` OR semantics
|
||||
for restricted day-of-month/day-of-week fields, applies the documented DST
|
||||
policy, and returns `ErrUnsatisfiableCron` for an expression with no possible
|
||||
occurrence. The classifier reports that last case as not schedulable. Changes
|
||||
to those scheduler rules are outside this capability.
|
||||
- **The routine list's own ordering.** This document pins the order of every
|
||||
trigger list it reports, and the startup scan document pins its own iteration
|
||||
order. The order routines appear in the list response is pre-existing behaviour
|
||||
and is not changed here.
|
||||
- **Editing a schedule.** Changing a cron expression, timezone, webhook secret or
|
||||
signing mode is not classification, and would not be re-arming either: a
|
||||
trigger that is not schedulable cannot be repaired by re-arming it and needs
|
||||
its expression edited.
|
||||
- **Disabling a trigger from the UI.** No endpoint sets
|
||||
`office_routine_triggers.enabled` to false today and this document adds none.
|
||||
Whether the column should be retired is not decided here.
|
||||
- **Using `office_routines.last_run_at` as schedule state.** Dispatch updates
|
||||
this column through `TouchRoutineLastRun`; it is a run-history value, not an
|
||||
input to schedule-state classification.
|
||||
- **Runs stranded in `claimed`,** and any reaper for them. A stranded run does not
|
||||
affect whether the next tick fires.
|
||||
- **Operator notification.** Inbox items, mail, and paging are separate work.
|
||||
- **Assignee validity.** A routine armed against a deleted or paused assignee
|
||||
agent is `armed` here. Whether the woken agent can act is another contract.
|
||||
- **Trigger reconciliation against a config source.** Config sync creates routines
|
||||
without triggers and the infrastructure reconciler gives them a `manual` one,
|
||||
which is how the live install reached `unscheduled_manual_only`. This document
|
||||
makes that state visible and changes what neither creates.
|
||||
- **Webhook delivery.** Whether an `event_only` routine's caller actually posts to
|
||||
it, and whether its signature verifies, is not observed here. `event_only` says
|
||||
a schedule is absent and an event trigger is enabled, nothing more.
|
||||
@@ -0,0 +1,194 @@
|
||||
---
|
||||
status: current
|
||||
system: office
|
||||
requirements:
|
||||
- REQ-OFFICE-COORDINATOR-INSTALL-001
|
||||
---
|
||||
|
||||
# Office Coordinator Install Idempotency System Design
|
||||
|
||||
## Purpose and boundaries
|
||||
|
||||
Onboarding pre-installs a `Coordinator heartbeat` routine for a coordinator
|
||||
agent. The installer previously matched an existing routine on workspace +
|
||||
assignee + canonical name **and** a cron trigger with the exact canonical
|
||||
expression — a mutable-state match that let a half-finished install (routine
|
||||
row created, trigger creation failed) or a later trigger edit make the
|
||||
installer duplicate the routine on the next run. This design redefines
|
||||
identity as workspace + assignee + canonical name only, completes a
|
||||
half-finished install instead of duplicating it, and serializes install
|
||||
attempts across every process sharing the database so two concurrent
|
||||
installs can never both observe "absent." It does not touch dispatch or any
|
||||
other trigger-management path; the schedule-state classification it uses to
|
||||
report an already-scheduled routine is owned by
|
||||
[routine-schedule-state.md](routine-schedule-state.md).
|
||||
|
||||
## Requirement mapping
|
||||
|
||||
| Requirement | Design section |
|
||||
| --- | --- |
|
||||
| `REQ-OFFICE-COORDINATOR-INSTALL-001` | [Identity and decision flow](#identity-and-decision-flow), [Locking](#locking), [Failure and recovery](#failure-and-recovery) |
|
||||
|
||||
## Components and responsibilities
|
||||
|
||||
- **`RoutineService.CreateDefaultCoordinatorRoutine`**
|
||||
(`internal/office/routines/service.go`) — the sole entry point, called by
|
||||
`internal/office/agents/service.go` and
|
||||
`internal/office/onboarding/service.go`. Runs the routine phase and the
|
||||
trigger phase as two separately-locked, separately-committed steps.
|
||||
- **`Repository.InstallCoordinatorRoutine` /
|
||||
`Repository.EnsureCoordinatorTrigger`**
|
||||
(`internal/office/repository/sqlite/coordinator_install.go`) — the two
|
||||
locked, transactional read-decide-write boundaries the service phases run
|
||||
inside.
|
||||
- **`Repository.WithCoordinatorInstallLock`** (same file) — the shared
|
||||
cross-process serialization mechanism both boundaries use, keyed by an
|
||||
opaque lock key.
|
||||
- **`decideCoordinatorTrigger` / `selectOrCreateCoordinatorRoutine`**
|
||||
(`internal/office/routines/service.go`) — the decision logic each locked
|
||||
boundary runs: create-or-select for the routine phase, create-or-leave for
|
||||
the trigger phase.
|
||||
|
||||
## Data and contracts
|
||||
|
||||
### Identity
|
||||
|
||||
`(workspace_id, assignee_agent_profile_id, name=CoordinatorRoutineName)` is
|
||||
the entire identity. `hasAnyCronTrigger` — any cron trigger, enabled or not,
|
||||
canonical expression or not — is the only fact the trigger phase uses to
|
||||
decide whether to act; it never reads `enabled`, `next_run_at`, or the
|
||||
trigger's expression.
|
||||
|
||||
### Lock keys
|
||||
|
||||
- `CoordinatorInstallLockKey(workspaceID, agentID, canonicalName)` —
|
||||
`"office-coordinator-install:|<workspace>|<agent>|<name>"`, guards the
|
||||
routine phase.
|
||||
- `CoordinatorInstallTriggerLockKey(routineID)` —
|
||||
`"office-coordinator-install-trigger:<routine_id>"`, guards the trigger
|
||||
phase. Deliberately a distinct namespace so a routine ID can never collide
|
||||
with an identity key.
|
||||
- Both are exported functions, not exported namespace constants, so no
|
||||
caller reassembles a key itself — the same rationale
|
||||
`internal/workflow/repository.ParticipantRoleSeatLockKey` uses.
|
||||
|
||||
### `models.CoordinatorInstallTx`
|
||||
|
||||
The tx-scoped writer interface `decide` callbacks use:
|
||||
`ListTriggersByRoutineID`, `CreateRoutine`, `CreateRoutineTrigger` — all
|
||||
transaction-bound, so a `decide` callback cannot escape the lock's
|
||||
transaction boundary.
|
||||
|
||||
### `models.ErrCoordinatorInstallContention`
|
||||
|
||||
The sentinel error `WithCoordinatorInstallLock` returns when
|
||||
`coordinatorInstallLockTimeout` (5s, compile-time constant, no
|
||||
configuration surface) is reached before the lock is won —
|
||||
distinguished from the caller's own context ending by comparing which
|
||||
context is actually expired (see
|
||||
[Failure and recovery](#failure-and-recovery)).
|
||||
|
||||
## Control flow
|
||||
|
||||
### Identity and decision flow
|
||||
|
||||
`CreateDefaultCoordinatorRoutine` runs two phases in sequence, both against
|
||||
the same captured `now`:
|
||||
|
||||
1. **Routine phase** — `InstallCoordinatorRoutine` opens the identity lock,
|
||||
lists every routine matching identity (ordered `created_at, id`), and
|
||||
hands them to `selectOrCreateCoordinatorRoutine`: zero matches creates a
|
||||
routine; one or more selects the earliest, logging a warning (and an
|
||||
`expvar` observation) if more than one matched.
|
||||
2. **Trigger phase** — once the routine phase has committed,
|
||||
`EnsureCoordinatorTrigger` opens the routine's own trigger lock, reads its
|
||||
triggers, and hands them to `decideCoordinatorTrigger`: any existing cron
|
||||
trigger (of any state) is left untouched and its schedule state logged
|
||||
(via the shared classifier); no cron trigger creates the canonical one,
|
||||
with `next_run_at` computed from the single `now` the outer call captured.
|
||||
|
||||
The two phases are separate transactions on purpose: a trigger-creation
|
||||
failure must never roll back an already-committed routine.
|
||||
|
||||
### Locking
|
||||
|
||||
`WithCoordinatorInstallLock` runs `fn` inside one transaction, bounded to
|
||||
`coordinatorInstallLockTimeout` via a context derived from (not identical to)
|
||||
the caller's context:
|
||||
|
||||
- **SQLite:** the writer connection is the only open connection
|
||||
(`internal/db.OpenSQLite`'s `SetMaxOpenConns(1)`), and SQLite's own
|
||||
file-level lock plus `busy_timeout` already serializes a second process's
|
||||
write transaction — opening the transaction is the whole mechanism, the
|
||||
same pattern `internal/workflow/repository.EnsureRoleSeat` already
|
||||
establishes.
|
||||
- **PostgreSQL:** a plain transaction does not by itself stop two callers
|
||||
both reading "absent" under `READ COMMITTED`, so the transaction
|
||||
additionally holds `pg_try_advisory_xact_lock(hashtextextended(lockKey,
|
||||
0))`, acquired via a bounded poll (`coordinatorInstallLockPollInterval`,
|
||||
25ms) rather than the blocking `pg_advisory_xact_lock`, so the same hard
|
||||
acquisition bound applies on both dialects. The lock releases automatically
|
||||
at commit/rollback.
|
||||
|
||||
The transaction commits when `fn` returns nil, rolls back otherwise.
|
||||
|
||||
## Failure and recovery
|
||||
|
||||
`reportCoordinatorLockFailure` classifies every non-nil outcome from either
|
||||
locked phase into exactly one reportable condition, checked in this order:
|
||||
|
||||
1. `decision.errored` — `decide` already reported at its own failure site
|
||||
(`routine_create_failed`, `trigger_create_failed`, or
|
||||
`next_occurrence_failed`); nothing reported again here, even though the
|
||||
busy-text/deadline heuristic below could otherwise also match `decide`'s
|
||||
own error text.
|
||||
2. `errors.Is(err, models.ErrCoordinatorInstallContention)` — the lock
|
||||
timeout was reached → `contention`.
|
||||
3. `errors.Is(err, context.Canceled) || errors.Is(err,
|
||||
context.DeadlineExceeded)` on the **caller's own** context → `cancelled`,
|
||||
distinguishable from contention because `classifyCoordinatorInstallWaitErr`
|
||||
checks `callerCtx.Err()` before falling back to the bounded-context
|
||||
classification.
|
||||
4. `!decision.ran` — the pre-`decide` read (identity lookup or trigger read)
|
||||
failed before `decide` ever ran → the caller-supplied fallback condition
|
||||
(`lookup_failed` or `trigger_read_failed`).
|
||||
5. Otherwise — `decide` ran and returned nil, so the failure is the
|
||||
transaction's own commit → `commit_failed`.
|
||||
|
||||
`classifyCoordinatorInstallWaitErr` folds `context.DeadlineExceeded`,
|
||||
`sql.ErrTxDone` (what `database/sql` surfaces from `Commit` when the bounded
|
||||
context's own deadline already auto-rolled the transaction back), and the
|
||||
SQLite `"database is locked"` busy text into the contention bucket, but only
|
||||
after confirming the caller's own context has not itself ended.
|
||||
|
||||
## Persistence
|
||||
|
||||
No new tables. Both phases write only `office_routines` and
|
||||
`office_routine_triggers` through the existing tx-scoped repository methods.
|
||||
`internal/persistence/requiredstores` / `storeconformance` already register
|
||||
office's schema owner; this design adds no new one.
|
||||
|
||||
## Security
|
||||
|
||||
No new authorization surface — the install path runs from onboarding and
|
||||
agent-provisioning flows that already own the workspace/agent identity they
|
||||
pass in; an empty `workspace_id` or `agent_id` is rejected before any lock is
|
||||
acquired.
|
||||
|
||||
## Observability
|
||||
|
||||
`office_coordinator_install_conditions_total` (`expvar.Map`,
|
||||
`internal/office/routines/coordinator_install_metrics.go`), labelled
|
||||
`condition;workspace;assignee`. Conditions: `empty_identity`,
|
||||
`lookup_failed`, `trigger_read_failed`, `duplicate_matches`,
|
||||
`schedule_state`, `trigger_completed`, `routine_created`,
|
||||
`next_occurrence_failed`, `trigger_create_failed`, `routine_create_failed`,
|
||||
`contention`, `cancelled`, `commit_failed`. Each names a condition
|
||||
*observed*, not a call that completed successfully — a call ultimately
|
||||
rejected still counts the conditions it detected before rejecting. Also
|
||||
emitted as structured (`zap`) log lines at the same sites.
|
||||
|
||||
## Related decisions
|
||||
|
||||
None — this design does not introduce a durable boundary or contract beyond
|
||||
what REQ-OFFICE-COORDINATOR-INSTALL-001 itself specifies.
|
||||
@@ -0,0 +1,183 @@
|
||||
---
|
||||
status: current
|
||||
system: office
|
||||
requirements:
|
||||
- REQ-OFFICE-ROUTINE-ARMING-001
|
||||
- REQ-OFFICE-ROUTINE-ARMING-002
|
||||
- REQ-OFFICE-ROUTINE-ARMING-003
|
||||
---
|
||||
|
||||
# Office Routine Schedule State System Design
|
||||
|
||||
## Purpose and boundaries
|
||||
|
||||
An Office routine has two independent switches: intent
|
||||
(`office_routines.status`, operator-set) and schedule state (whether its
|
||||
triggers can actually fire it, derived). The product reported only intent.
|
||||
This design adds a second, read-only classification, surfaces it everywhere
|
||||
intent is already surfaced (routine list/detail reads, the routines UI), and
|
||||
adds an unattended-install detection path (a startup scan) for the same
|
||||
classification. It repairs nothing — no code path here creates, edits, or
|
||||
enables/disables a trigger. Dispatch keeps its existing status gates: the due
|
||||
trigger query is status-independent, then the service checks the routine before
|
||||
claiming a cron slot, while the manual and webhook paths reject non-firing
|
||||
statuses. See [coordinator-install-idempotency.md](coordinator-install-idempotency.md)
|
||||
for the one trigger-creation path this plan touches.
|
||||
|
||||
## Requirement mapping
|
||||
|
||||
| Requirement | Design section |
|
||||
| --- | --- |
|
||||
| `REQ-OFFICE-ROUTINE-ARMING-001` | [Classification](#classification) |
|
||||
| `REQ-OFFICE-ROUTINE-ARMING-002` | [Read-path reporting](#read-path-reporting), [Frontend rendering](#frontend-rendering) |
|
||||
| `REQ-OFFICE-ROUTINE-ARMING-003` | [Startup scan](#startup-scan) |
|
||||
|
||||
## Components and responsibilities
|
||||
|
||||
- **Classifier** (`apps/backend/internal/office/routines/arming.go`) — pure,
|
||||
total function from one routine's trigger rows to a `ScheduleState` plus an
|
||||
unarmed-cron-trigger list. No I/O, no routine status input.
|
||||
- **Batch classification** (`ClassifyRoutines`, same file) — the read
|
||||
boundary every consumer shares: one batched trigger read across many
|
||||
routines, with a per-routine fallback read when the batch read itself
|
||||
fails.
|
||||
- **Read-path reporting** (`RoutineService.AttachScheduleState`,
|
||||
`internal/office/routines/service.go`) — the API consumer: wraps the
|
||||
existing list/get routine responses with the classification.
|
||||
- **Startup scan** (`RunStartupScan`,
|
||||
`internal/office/routines/startup_scan.go`) — the unattended-install
|
||||
consumer: a read-only pass over every enumerable workspace and routine at
|
||||
boot, ordered after reconciliation, that logs and counts what it finds
|
||||
instead of returning it to a caller.
|
||||
- **Frontend rendering**
|
||||
(`apps/web/app/office/routines/schedule-state.ts`,
|
||||
`schedule-state-badge.tsx`) — collapses the nine wire `ScheduleState`
|
||||
values into five label groups and renders `ScheduleStateBadge` /
|
||||
`UnarmedScheduleHint` on the routine list and detail views
|
||||
(`routine-row.tsx`).
|
||||
|
||||
## Data and contracts
|
||||
|
||||
### `ScheduleState`
|
||||
|
||||
One of nine values (`internal/office/routines/arming.go`):
|
||||
`armed`, `trigger_invalid`, `trigger_unscheduled`, `trigger_disabled`,
|
||||
`event_only`, `unscheduled_manual_only`, `unscheduled_no_trigger`, `unknown`.
|
||||
`unknown` is reserved for `ClassifyRoutines`' per-routine fallback-read
|
||||
failure; `ClassifyRoutine` itself never returns it.
|
||||
|
||||
### Classification rule table
|
||||
|
||||
`ClassifyRoutine(triggers []*RoutineTrigger, now time.Time)` evaluates, in
|
||||
order, stopping at the first match:
|
||||
|
||||
1. Any enabled cron trigger with a next occurrence, or one that fired within
|
||||
the last `dispatchGrace` (60s, a compile-time constant shared verbatim
|
||||
with the startup scan) and hasn't been recomputed yet → `armed`.
|
||||
2. Any enabled cron trigger for which the shared scheduler cannot compute a
|
||||
next occurrence (`shared.NextCronTime`), including an impossible but
|
||||
syntactically valid expression → `trigger_invalid`.
|
||||
3. Any enabled, schedulable cron trigger with no next occurrence recorded →
|
||||
`trigger_unscheduled`.
|
||||
4. One or more cron triggers exist but none matched 1-3 (all disabled) →
|
||||
`trigger_disabled`.
|
||||
5. No cron trigger, but an enabled webhook trigger exists → `event_only`.
|
||||
6. No cron trigger, but some other (e.g. manual) trigger exists →
|
||||
`unscheduled_manual_only`.
|
||||
7. No trigger at all → `unscheduled_no_trigger`.
|
||||
|
||||
Rule 1 does not require schedulability: a trigger that already claimed a next
|
||||
occurrence still fires on it even if edited into an unparseable expression
|
||||
afterward — `unarmedCronTriggers` reports that separately, so the same
|
||||
trigger can be simultaneously `armed` and listed as unarmed-with-reasons.
|
||||
|
||||
### `UnarmedCronTrigger`
|
||||
|
||||
`{trigger_id, reasons: UnarmedReason[]}`, one entry per cron trigger matching
|
||||
the disabled-OR-not-schedulable-OR-stalled-past-grace predicate, independent
|
||||
of the routine's overall `ScheduleState`. `UnarmedReason` is `disabled`,
|
||||
`not_schedulable`, or `stalled`; more than one may apply to the same trigger.
|
||||
|
||||
### API contract
|
||||
|
||||
`RoutineListResponse` / `RoutineResponse`
|
||||
(`internal/office/routines/dto.go`) embed `RoutineWithSchedule`:
|
||||
`{*Routine, schedule_state, unarmed_cron_triggers}` (JSON tags), returned
|
||||
from `GET /workspaces/:wsId/routines`, `GET /routines/:id`, and the
|
||||
create/update routine handlers (`internal/office/routines/handler.go`).
|
||||
`AttachScheduleState` classifies with one `time.Now().UTC()` capture per
|
||||
call — never per-routine — so every routine in one response is judged
|
||||
against the same instant.
|
||||
|
||||
### Frontend contract
|
||||
|
||||
`readScheduleState` / `readUnarmedCronTriggers`
|
||||
(`apps/web/app/office/routines/schedule-state.ts`) tolerate the raw
|
||||
snake_case wire shape directly, matching the codebase's established
|
||||
`fetchJson`-does-not-case-convert pattern. `scheduleStateGroup` collapses the
|
||||
nine wire values into five label groups (`armed`, `broken`, `event_only`,
|
||||
`no_schedule`, `unknown`) so two routines in different groups never render
|
||||
the same label. `hasSchedulableUnarmedEntry` distinguishes an unarmed entry
|
||||
that only needs re-arming from one whose expression needs editing first.
|
||||
|
||||
## Control flow
|
||||
|
||||
**Read path:** handler calls `AttachScheduleState(ctx, routines)` →
|
||||
`ClassifyRoutines` batch-reads triggers for all routine IDs via one
|
||||
repository call → falls back to `ListTriggersByRoutineID` per routine only if
|
||||
the batch call errors → `ClassifyRoutine` runs the pure rule table per
|
||||
routine → handler serializes `RoutineWithSchedule`.
|
||||
|
||||
**Startup scan:** `Reconciler.ReconcileAll` returns a `ReconcileSignal`
|
||||
(the only way to construct one is `SignalReconcileComplete`, called after
|
||||
`ReconcileAll` returns — a zero-value signal, which an unauthorized caller or
|
||||
a refusal-path test would hold, carries no completion) →
|
||||
`internal/backendapp/main.go` launches `RunStartupScan` in a goroutine after
|
||||
logging "Office reconciliation complete" → the scan enumerates workspaces,
|
||||
then routines per workspace, then classifies each batch via the same
|
||||
`ClassifyRoutines` path the read API uses → emits one structured log record
|
||||
and one `expvar` observation per routine, plus a skip counter for any input
|
||||
read that failed.
|
||||
|
||||
## Failure and recovery
|
||||
|
||||
- A batch trigger read failure degrades to per-routine reads; a
|
||||
per-routine read failure yields `ScheduleStateUnknown` for that routine
|
||||
only, never an error returned to the caller.
|
||||
- The startup scan fails closed at every enumeration boundary (no signal, no
|
||||
workspaces, one workspace's routines unreadable) by skipping and counting
|
||||
the skip, never retrying and never aborting the rest of the scan.
|
||||
- The scan is fire-and-forget (`go officeroutines.RunStartupScan(...)`): no
|
||||
outcome inside it can fail or delay Office startup.
|
||||
|
||||
## Persistence
|
||||
|
||||
No new tables or columns. The classifier and the scan are read-only over
|
||||
`office_routines` and `office_routine_triggers`
|
||||
(`internal/office/repository/sqlite/base.go`).
|
||||
|
||||
## Security
|
||||
|
||||
Read-only; no new authorization surface. Existing routine-read
|
||||
authorization (workspace scoping) applies unchanged, since the classification
|
||||
is attached to the same response the caller already had access to.
|
||||
|
||||
## Observability
|
||||
|
||||
- `office_routine_arming_scan_observations_total` — every routine the
|
||||
startup scan classified, labelled `workspace`, `intent`, `schedule_state`.
|
||||
- `office_routine_arming_scan_skipped_total` — a scan step abandoned because
|
||||
an input could not be read, labelled `reason`
|
||||
(`no_signal`, `workspace_enum_failed`, `routine_enum_failed`,
|
||||
`classification_unknown`).
|
||||
- Both are `expvar.Map` counters (`internal/office/routines/arming_metrics.go`),
|
||||
exposed via the stdlib `/debug/vars` handler in dev mode, following
|
||||
`internal/orchestrator/office_stall_metrics.go`'s label-string convention.
|
||||
- The scan also emits one structured (`zap`) log record per routine reached,
|
||||
at `Warn` for a routine that cannot fire or has no schedule, `Info`
|
||||
otherwise.
|
||||
|
||||
## Related decisions
|
||||
|
||||
None — this design does not introduce a durable boundary or contract beyond
|
||||
what the linked requirements themselves specify.
|
||||
Reference in New Issue
Block a user