fix(office): make routine catch-up enqueue a summarized wake (#3538)
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,346 @@
|
||||
---
|
||||
status: draft
|
||||
system: office
|
||||
created: 2026-09-07
|
||||
owners:
|
||||
- kandev
|
||||
---
|
||||
|
||||
# Office Routine Catch-Up Requirements
|
||||
|
||||
## Overview
|
||||
|
||||
A routine's cron trigger can come due while the backend is not running to
|
||||
process it. On resume its armed `next_run_at` is in the past, sometimes far in
|
||||
the past. What the system does at that moment is a correctness property of an
|
||||
unattended loop: it decides how much work resumes, how much is spent in the
|
||||
first minute after restart, and what the prompt says. The launcher rejects
|
||||
taskless runs, so lightweight runs may not reach an agent session.
|
||||
|
||||
Four surfaces describe this moment, and they do not agree.
|
||||
|
||||
| Surface | What it says |
|
||||
|---|---|
|
||||
| Policy value `enqueue_missed_with_cap` | missed ticks are enqueued, up to a cap |
|
||||
| Field `catch_up_max`, default 25 | the cap is 25 |
|
||||
| `scheduler-01.md` and `scheduler-02.md` | "fire missed ticks up to the cap (default 25)" |
|
||||
| Web UI label `office:enqueueMissedWithCap` (5 locales) | "Enqueue missed (with cap)" |
|
||||
| The code (`routines/service.go`) | counts up to 25 elapsed ticks, dispatches **exactly one** run |
|
||||
|
||||
The code is silently authoritative. An operator sizing spend or work-in-progress
|
||||
from the policy name, the field, the design doc, or the UI control would be
|
||||
wrong by a factor of 25.
|
||||
|
||||
This capability resolves the disagreement in favour of the code and makes
|
||||
every other surface state it. **One resume produces one wake.** It also
|
||||
supplies what the code lacks: a durable, observable account of the gap crossed,
|
||||
so collapsing the backlog is a reported fact rather than a silent one.
|
||||
|
||||
### Why one wake, and not twenty-five
|
||||
|
||||
The decision is chosen here, not inherited from the code. Enqueueing up to
|
||||
`catch_up_max` runs was rejected on four grounds: the idempotency key already
|
||||
collapses a fan-out to one wake; the concurrency policy would then skip or merge
|
||||
whatever survived; a fan-out creates a burst hazard before either bounding
|
||||
control exists; and there is no per-tick work to replay. Each is verified
|
||||
against the tree, with its mechanism and citations, in
|
||||
[Why the fan-out was rejected](../system-design/routine-catch-up-01.md#why-the-fan-out-was-rejected).
|
||||
|
||||
## Terminology
|
||||
|
||||
- **Claim:** the atomic acquisition of a due cron trigger by one processor;
|
||||
exactly one may hold a given claim.
|
||||
- **Processing instant:** the time the scheduler uses for the tick in which a
|
||||
claim is taken.
|
||||
- **Gap:** the interval from the trigger's armed `next_run_at` at claim time up
|
||||
to the processing instant.
|
||||
- **Elapsed ticks:** how many times the trigger's cron expression matches within
|
||||
the gap, including the tick that came due and is being dispatched now.
|
||||
- **Missed ticks:** elapsed ticks minus one — the ticks that came due and will
|
||||
never be dispatched.
|
||||
- **Gap summary:** the durable record of a gap: missed-tick count, first-missed
|
||||
timestamp, and whether the count was truncated.
|
||||
- **Summarizing policy:** records the gap in the lightweight prompt, named
|
||||
`summarize_missed` here.
|
||||
- **Lightweight / heavy routine:** empty versus non-empty `task_template`, as
|
||||
in [Office Scheduler](scheduler.md).
|
||||
|
||||
## Requirements
|
||||
|
||||
### REQ-OFFICE-ROUTINE-CATCHUP-001: One resume, one wake
|
||||
|
||||
**Intent:** Freeze the resume behaviour as a stated contract rather than an
|
||||
emergent property of a counting loop, and close the boundary cases the
|
||||
implementation leaves to invention: ordering, concurrent claims, a failed
|
||||
computation, a crash-orphaned trigger, and a dispatch that fails after arming.
|
||||
|
||||
**As an** operator running an unattended workspace, **I want** a restart after
|
||||
an outage to produce a bounded, predictable amount of agent work, **so that** a
|
||||
long outage cannot turn into a burst of spend I did not authorize.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-001.1:** When a due cron trigger is claimed, the
|
||||
system shall dispatch exactly one routine run for that claim, whatever the
|
||||
number of elapsed ticks in the gap and whatever the catch-up policy. The only
|
||||
cases producing none are a failed arming write (AC-001.2), an unsatisfiable
|
||||
expression (AC-001.11), and a run that cannot be created (AC-001.12); no case
|
||||
produces two.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-001.2:** When a claim is processed and the
|
||||
elapsed-tick computation succeeds, the system shall arm the trigger's
|
||||
`next_run_at` to the first match of its cron expression strictly after the
|
||||
processing instant. This shall hold identically under every catch-up policy.
|
||||
The failure path is governed by AC-001.6 and AC-001.11 instead. When the
|
||||
arming write itself fails, the system shall dispatch nothing for that claim
|
||||
and leave `next_run_at` null for AC-001.9 to arm later; that tick is lost and
|
||||
no routine run records it.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-001.3:** `catch_up_max` shall bound only the
|
||||
number of elapsed ticks the system counts. It shall have no effect on the
|
||||
number of runs, tasks, or wakeup requests created per claim, which is at most
|
||||
one.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-001.4:** When the stored `catch_up_max` is less
|
||||
than 1, the system shall use 25; when it is greater than 1000, it shall use
|
||||
1000. Both clamp rather than reject, so no stored value can stop a routine
|
||||
firing.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-001.5:** When two processors evaluate the same
|
||||
due trigger concurrently, exactly one shall take the claim and dispatch;
|
||||
the other shall dispatch nothing, record no gap summary, and change no
|
||||
trigger state.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-001.6:** When the elapsed-tick computation fails,
|
||||
the system shall record no gap summary. For `ErrUnsatisfiableCron` it shall
|
||||
leave `next_run_at` null and dispatch no run. For another failure it shall
|
||||
arm `next_run_at` to the processing instant plus 24 hours and dispatch one
|
||||
run. It shall warn with the underlying error.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-001.7:** Within one scheduler tick, due triggers
|
||||
shall be processed in ascending `office_routine_triggers.next_run_at` order,
|
||||
with ties broken by ascending `office_routine_triggers.id`.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-001.8:** When a cron trigger is created, the
|
||||
system shall arm `next_run_at` to the first match of its expression strictly
|
||||
after the creation instant, and shall reject the creation when that
|
||||
expression is empty or when it or the timezone cannot be parsed. A trigger of
|
||||
kind `cron` shall never be persisted with a null `next_run_at` at creation,
|
||||
on every creation path, including one that writes the trigger row directly.
|
||||
**No operation that mutates an existing trigger's cron expression, timezone,
|
||||
or enabled flag exists** (see `## Out of scope`); changing any of the three is
|
||||
a delete followed by a create, which arms by this same criterion. A recreated
|
||||
trigger shall never report a gap spanning the interval in which it did not
|
||||
exist, so a deliberate pause is never a gap.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-001.9:** When an enabled cron trigger has a null
|
||||
`next_run_at`, a subsequent scheduler tick shall arm it to the first match
|
||||
strictly after that tick's processing instant and shall not dispatch a run for
|
||||
it, so that a process stopping between claiming a trigger and arming it does
|
||||
not leave the trigger permanently unable to fire. The write shall be
|
||||
conditional on `next_run_at` still being null when applied, so a concurrent
|
||||
reconciliation is never overwritten: exactly one processor shall arm it, and
|
||||
none shall dispatch by way of this path. A trigger whose claim is still in
|
||||
flight shall not be armed by it. A trigger with `enabled` false shall not be
|
||||
armed by this path. When the expression or timezone cannot be parsed, the
|
||||
system shall leave `next_run_at` null, warn, and dispatch nothing; no
|
||||
computation-failure fallback applies because no claim was taken and no run is
|
||||
owed.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-001.10:** When a routine run has been created and
|
||||
its materialisation then fails — the heavy path failing to create the task, or
|
||||
the lightweight path failing to create or to dispatch the wakeup request — the
|
||||
system shall set that run's status to `failed` and shall not re-dispatch it. A
|
||||
wakeup request refused because its idempotency key already exists is **not** a
|
||||
materialisation failure; that run's status shall be left as the outcome the
|
||||
concurrency policy assigned.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-001.11:** When the elapsed-tick computation fails
|
||||
after a claim, the system shall warn with the trigger and underlying error.
|
||||
For `ErrUnsatisfiableCron`, it shall leave `next_run_at` null and dispatch no
|
||||
run. For another failure, it shall arm `next_run_at` to the processing instant
|
||||
plus 24 hours and dispatch one run. Neither path shall record a gap summary.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-001.12:** When the routine run for a claim cannot
|
||||
be created at all, the system shall dispatch nothing and record no gap
|
||||
summary, and shall not re-attempt that tick. The trigger remains armed forward
|
||||
per AC-001.2 or AC-001.11, whichever governed the claim, or stays disarmed
|
||||
for an unsatisfiable expression. The absence of a routine run is the only
|
||||
record of that tick.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-001.13:** `catch_up_max` shall be normalized to
|
||||
the value AC-001.4 defines before it is persisted, on every create and update
|
||||
path, and rows written before this change shall be normalized once on upgrade.
|
||||
A routine read back through the API, and every web UI control showing it,
|
||||
shall report the same `catch_up_max` the tick honours.
|
||||
|
||||
### REQ-OFFICE-ROUTINE-CATCHUP-002: The gap is reported, not discarded silently
|
||||
|
||||
**Intent:** Answer what is discarded past the cap and whether it is recorded
|
||||
anywhere. Nothing is discarded, because no run was ever going to be created for
|
||||
a missed tick; what is bounded is the counting. The exact gap boundary is
|
||||
preserved even when the count is truncated.
|
||||
|
||||
**As an** assembled run after an outage, **I want** prompt context to state the
|
||||
gap and whether its count is exact, **so that** an agent can choose what to do.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-002.1:** When a claim's gap contains at least one
|
||||
missed tick and the policy is `summarize_missed`, the system shall record a
|
||||
gap summary on the routine run containing the missed-tick count, the
|
||||
timestamp of the first missed tick, and whether the count was truncated.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-002.2:** The missed-tick count shall exclude the
|
||||
tick being dispatched. When that count is zero, the system shall record no
|
||||
gap summary, whatever the value of truncated.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-002.3:** When no gap summary is recorded, the
|
||||
routine run shall carry no missed-tick field, and the agent's assembled
|
||||
prompt shall contain no missed-tick statement. Absence shall not be
|
||||
represented as a count of zero.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-002.4:** When `catch_up_max` is greater than 1
|
||||
and the elapsed-tick count reaches it while further ticks remain within the
|
||||
gap, the system shall record the missed-tick count as `catch_up_max - 1`,
|
||||
shall record truncated as true, and shall record the first-missed-tick
|
||||
timestamp exactly and un-truncated. At a `catch_up_max` of 1 this criterion
|
||||
does not apply and AC-002.11 governs.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-002.5:** When the routine is lightweight and the
|
||||
policy is `summarize_missed` and a gap summary was recorded, the assembled
|
||||
prompt for the run created by that claim shall state the missed-tick count and
|
||||
the first-missed-tick timestamp, and, when truncated is true, shall state that
|
||||
the count is a lower bound.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-002.6:** When the routine is heavy and the policy
|
||||
is `summarize_missed` and a gap summary was recorded, it shall be readable
|
||||
through the routine-run API. The created task's title and description shall be
|
||||
exactly what the routine's template renders, unmodified by the gap. A heavy
|
||||
routine under `skip_missed` records none, per AC-002.7.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-002.7:** When the policy is `skip_missed`, the
|
||||
system shall record no gap summary and the assembled prompt shall contain no
|
||||
missed-tick statement, whatever the number of elapsed ticks.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-002.8:** A recorded gap summary shall remain
|
||||
readable after the routine run reaches a terminal status and after a backend
|
||||
restart.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-002.9:** The system shall record the gap as
|
||||
measured, without attributing a cause. A gap produced by a backend outage, a
|
||||
suspended host, and a forward clock correction shall be recorded identically.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-002.10:** A gap summary shall be written exactly
|
||||
once, as part of the creation of the routine run it belongs to, and shall not
|
||||
be modified afterwards by any later status transition, including `skipped`,
|
||||
`coalesced`, `failed`, `done`, and `cancelled`. When more than one routine run
|
||||
is created for the same routine — two triggers due in the same tick, or a
|
||||
manual fire alongside a cron fire — each run shall carry only the summary
|
||||
measured for its own claim, and shall neither read nor overwrite another run's.
|
||||
A wakeup request refused as an idempotency duplicate, or coalesced into an
|
||||
in-flight run, shall not rewrite any gap summary, shall not alter the gap
|
||||
statement that run's agent already carries, and shall not give a gap statement
|
||||
to a run that had none.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-002.11:** When `catch_up_max` is 1, the
|
||||
missed-tick count is zero for every gap, and the system shall therefore record
|
||||
no gap summary for that routine however long the gap. Setting `catch_up_max`
|
||||
to 1 is an opt-out of gap reporting.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-002.12:** A routine run created by a manual or a
|
||||
webhook fire shall record no gap summary. Gap measurement applies only to a
|
||||
cron trigger's armed `next_run_at`.
|
||||
|
||||
### REQ-OFFICE-ROUTINE-CATCHUP-003: Every surface states the same behaviour
|
||||
|
||||
**Intent:** `enqueue_missed_with_cap` is a false statement in an enum value, two
|
||||
design documents, and a UI control in five locales. Renaming it is the
|
||||
deliverable; accepting the old value forever is what makes the rename safe for
|
||||
installs that already store it.
|
||||
|
||||
**As an** operator reading the policy control, **I want** its name to describe
|
||||
what happens, **so that** I do not size spend or work-in-progress from a name
|
||||
that is 25 times wrong.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-003.1:** The summarizing catch-up policy shall be
|
||||
named `summarize_missed`. A routine created with no catch-up policy supplied
|
||||
shall be persisted with `summarize_missed`, **whatever creation path is
|
||||
used** — the HTTP create handler, config-sync reconciliation, the
|
||||
pre-installed coordinator routine, or any direct repository insert. No
|
||||
creation path shall persist the deprecated alias.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-003.2:** `enqueue_missed_with_cap` shall be
|
||||
accepted on routine create and routine update as a deprecated alias for
|
||||
`summarize_missed`, and shall be normalized to `summarize_missed` before
|
||||
persistence.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-003.3:** A routine persisted before this change
|
||||
with `catch_up_policy = 'enqueue_missed_with_cap'` shall read as
|
||||
`summarize_missed`, and the column's stored default shall be
|
||||
`summarize_missed` **on an upgraded install as well as on a fresh one**.
|
||||
Inspecting the live schema of a database that predates this change shall not
|
||||
show the retired value.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-003.4:** `enqueue_missed_with_cap` shall not
|
||||
appear in any API response body or in any web UI control.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-003.5:** A stored `catch_up_policy` value that is
|
||||
neither `summarize_missed`, nor `skip_missed`, nor the deprecated alias shall
|
||||
be treated as `summarize_missed` and shall not prevent the routine from
|
||||
firing.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-003.6:** The web UI shall label the summarizing
|
||||
policy so that it states a single summarized wake, and shall label
|
||||
`catch_up_max` so that it states the bound is on the number of ticks counted
|
||||
and reported, not on the number of runs created. This copy shall be provided
|
||||
in `en`, `pt-pt`, `zh-cn`, `zh-hk`, and `zh-tw`.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-003.7:** `system-design/scheduler-01.md` and
|
||||
`system-design/scheduler-02.md` shall not describe missed cron ticks as
|
||||
fired, enqueued, or dispatched as multiple runs, and shall reference this
|
||||
requirement for catch-up semantics rather than restating them. Neither
|
||||
document shall name `enqueue_missed_with_cap` as the catch-up policy value
|
||||
except where it explicitly labels it a deprecated alias. Both shall state the
|
||||
routine wakeup payload's catch-up fields wherever they describe that payload's
|
||||
shape. The constraint is scoped to those two files; this document necessarily
|
||||
quotes the retired value.
|
||||
- **AC-OFFICE-ROUTINE-CATCHUP-003.8:** Under the summarizing policy the web UI
|
||||
shall render the `catch_up_max` control and allow it to be edited, in both the
|
||||
routine-create dialog and the routine-detail view. Under `skip_missed` it
|
||||
shall not be rendered. The rename shall not make the control unreachable.
|
||||
|
||||
## Out of scope
|
||||
|
||||
Each exclusion is a decision, not an omission.
|
||||
|
||||
- **Enqueueing N missed runs.** Rejected on the four grounds in the
|
||||
Overview. Reopening it depends on work-in-progress enforcement and an enforced
|
||||
budget ceiling landing first, and on the wakeup idempotency key gaining a
|
||||
generation component.
|
||||
- **Renaming `catch_up_max`.** Its meaning changes (a bound on counting, not on
|
||||
runs) but the name stays accurate enough, and renaming the column, the API
|
||||
field and the web state would double the migration surface for no change in
|
||||
observable behaviour. The UI copy is corrected instead, under AC-003.6. Its
|
||||
stored-versus-effective divergence is **not** deferred: AC-001.13 closes it.
|
||||
- **A trigger-update operation.** There is none today: the trigger surface is
|
||||
create, list, delete and webhook-fire, and the only mutation of an existing
|
||||
trigger row is the scheduler's `next_run_at` re-arm. Editing a cron
|
||||
expression, timezone or enabled flag therefore means delete-then-create.
|
||||
Building the endpoint is a feature in its own right, including a decision
|
||||
about what an in-flight claim does when the expression changes underneath it.
|
||||
An update path added later inherits AC-001.8's rule: arm strictly after the
|
||||
change instant.
|
||||
- **Downgrade compatibility.** A payload carrying `summarize_missed` sent to a
|
||||
build predating this change will be rejected by it; the alias is read-forward
|
||||
only. No exported artifact carries it: the config-sync YAML has no catch-up
|
||||
field.
|
||||
- **A paused routine still fires.** `office_routines.status` is never consulted
|
||||
in the cron tick path; only `office_routine_triggers.enabled` gates firing. A
|
||||
defect, but not catch-up specific; a follow-up card carries it.
|
||||
- **Cron correctness at the tick level.** Day-of-month versus day-of-week
|
||||
conjunction and DST behaviour are gap 23, carded. A claimed unsatisfiable
|
||||
expression follows AC-001.11; matching rules remain outside this document.
|
||||
- **Work-in-progress limits and budget enforcement.** Gaps 11 and 19, carded.
|
||||
This document creates no work that would need them.
|
||||
- **Retention of gap summaries.** They inherit whatever retention
|
||||
`office_routine_runs` gets, which is none today (gap 20, carded).
|
||||
|
||||
## Prior art
|
||||
|
||||
**Wiki leg — did not run.** Config resolved from `~/.obsidian-wiki/config.henry`
|
||||
via the `@henry` pin, giving
|
||||
`OBSIDIAN_VAULT_PATH=/Users/henry/Documents/henry/wiki`, collection `wiki`.
|
||||
Reading the vault returns `EPERM` (macOS privacy protection on `~/Documents`
|
||||
from this worktree), and neither `obsidian-wiki` nor `qmd` is on `PATH` or as an
|
||||
MCP tool. An environment failure, not an empty result: the vault may hold a
|
||||
position here and was not consulted.
|
||||
|
||||
**saas-kb leg — did not run.** The `saas-kb` MCP server and its
|
||||
`search_fsm_docs` tool are absent from this session; the `ai_sdlc` slice was not
|
||||
queried.
|
||||
|
||||
**What informed it instead:** the tree, and
|
||||
`Forge/docs/kandev/office/BETA-REQUIREMENTS.md` gap 24 and section 7a.
|
||||
|
||||
## References
|
||||
|
||||
- [Office Scheduler](scheduler.md) — the wakeup pipeline this sits inside.
|
||||
- Catch-Up System Design
|
||||
[part 1](../system-design/routine-catch-up-01.md),
|
||||
[part 2](../system-design/routine-catch-up-02.md)
|
||||
- [Scheduler System Design part 1](../system-design/scheduler-01.md),
|
||||
[part 2](../system-design/scheduler-02.md) — the documents AC-003.7 corrects.
|
||||
@@ -0,0 +1,522 @@
|
||||
---
|
||||
status: draft
|
||||
system: office
|
||||
requirements:
|
||||
- REQ-OFFICE-ROUTINE-CATCHUP-001
|
||||
- REQ-OFFICE-ROUTINE-CATCHUP-002
|
||||
---
|
||||
|
||||
# Office Routine Catch-Up System Design, part 1: the tick and the gap
|
||||
|
||||
## Purpose and boundaries
|
||||
|
||||
This design makes the routine cron tick's resume behaviour explicit, gives the
|
||||
gap it crosses a durable record, and carries that record into the assembled
|
||||
prompt for eligible lightweight runs — REQ-001 and REQ-002. The existing shared
|
||||
launcher rejects taskless runs, so prompt assembly is the boundary covered here;
|
||||
actual taskless session delivery is a separate shared-scheduler contract.
|
||||
Renaming the policy value that misdescribes all of it is REQ-003, in
|
||||
[part 2](routine-catch-up-02.md).
|
||||
|
||||
The Office system owns the outcome: the trigger table, the routine-run table,
|
||||
the wakeup queue, and the routine prompt path are all Office primitives.
|
||||
|
||||
Adjacent contracts read and constrained, but not owned:
|
||||
|
||||
- `internal/scheduler/cron` — the shared 30-second cron loop that calls
|
||||
`RoutineService.TickScheduledTriggers`. Its interval and handler ordering are
|
||||
unchanged.
|
||||
- `internal/office/shared/cron.go` — the cron evaluator. Its tick times are
|
||||
taken as given; its day-of-month/day-of-week conjunction, DST behaviour, and
|
||||
unsatisfiable-expression fallback are gap 23 and out of scope.
|
||||
- `internal/office/wakeup` — the wakeup queue, its idempotency index, and the
|
||||
dispatcher's concurrency policy. Unchanged except that `RoutinePayload` gains
|
||||
two fields alongside the existing `MissedTicks`.
|
||||
|
||||
## Requirement mapping
|
||||
|
||||
| Requirement | Design section |
|
||||
|---|---|
|
||||
| REQ-OFFICE-ROUTINE-CATCHUP-001 | [The tick](#the-tick), [Normalizing catch_up_max](#normalizing-catch_up_max), [Ordering, claiming and re-arming](#ordering-claiming-and-re-arming), [Dispatch failure](#dispatch-failure) |
|
||||
| REQ-OFFICE-ROUTINE-CATCHUP-002 | [The gap summary](#the-gap-summary), [Delivering the gap to the agent](#delivering-the-gap-to-the-agent) |
|
||||
| REQ-OFFICE-ROUTINE-CATCHUP-003 | [part 2](routine-catch-up-02.md): Renaming the policy, Documentation consistency |
|
||||
|
||||
## Current behaviour, as measured
|
||||
|
||||
Recorded because these claims are load-bearing and none is written down today.
|
||||
|
||||
- `processCronTrigger` (`internal/office/routines/service.go:360-397`) claims
|
||||
the trigger, calls `computeRoutineMissed`, arms `next_run_at`, then calls
|
||||
`DispatchRoutineRunWithMissed` **once**, passing `runCount - 1`.
|
||||
- `computeRoutineMissed` (`:409-447`) walks `NextCronTime` from the armed
|
||||
`next_run_at` until the cursor passes `now` or the count reaches
|
||||
`routine.CatchUpMax` (default 25 when the stored value is not positive).
|
||||
Nothing bounds `CatchUpMax` above: `routines/handler.go` assigns it from the
|
||||
request unvalidated on both create and update, and the walk is one synchronous
|
||||
`NextCronTime` call per counted tick inside the 30-second scheduler tick.
|
||||
- The count reaches the wakeup request's payload as `missed_ticks`
|
||||
(`marshalRoutinePayload`, `:665`). `createFreshRun` copies that payload onto
|
||||
`runs.context_snapshot` **verbatim** (`wakeup/dispatcher.go:291`); the
|
||||
coalesce paths do not, and `json_patch` it in instead, at **two separate SQL
|
||||
sites** that do not share an implementation — see
|
||||
[Delivering the gap to the agent](#delivering-the-gap-to-the-agent).
|
||||
- **It stops there.** `buildPromptContext` is called with `run.Payload`
|
||||
(`service/scheduler_integration.go:375`), and `createFreshRun` sets
|
||||
`Payload: "{}"` on every wakeup-derived run. `prompt_builder.go` contains no
|
||||
reader for `ContextSnapshot`. No web view renders `context_snapshot` either —
|
||||
it is typed in `apps/web/lib/api/domains/office-runs-api.ts:113` and never
|
||||
read. `missed_ticks` is written and never observed, by an agent or a human.
|
||||
|
||||
Two further measurements shape the design:
|
||||
|
||||
- `GetDueTriggers` (`repository/sqlite/routines.go:65-79`) has no `ORDER BY`.
|
||||
- `ClaimTrigger` (`:84-96`) sets `next_run_at = NULL` as its CAS and stamps
|
||||
`updated_at`; `UpdateTriggerNextRun` re-arms in a separate, unconditional
|
||||
statement. A process that stops between the two leaves an enabled cron trigger
|
||||
with a null `next_run_at`, which `GetDueTriggers` filters out — permanently.
|
||||
That `updated_at` stamp is what lets the reconciliation pass tell a live claim
|
||||
from an abandoned one.
|
||||
- `models.RoutineRunStatusFailed` is declared in `models/enums.go` and set
|
||||
nowhere in the tree.
|
||||
|
||||
## Why the fan-out was rejected
|
||||
|
||||
The requirement records the decision; this is the mechanism behind each of its
|
||||
four grounds, verified against the tree.
|
||||
|
||||
1. **It is not implementable without changing a different contract.** The wakeup
|
||||
idempotency key is `routine:<routine_id>:<trigger_id>:<unix_minute>`
|
||||
(`buildRoutineIdempotencyKey`) and `agent_wakeup_requests.idempotency_key`
|
||||
carries a partial unique index. N requests created inside one tick share one
|
||||
wall-clock minute, so the second through Nth are rejected as idempotency
|
||||
conflicts and absorbed as a warning. A naive fan-out silently produces one
|
||||
wake anyway. Re-keying on scheduled tick time is the generation-identity work
|
||||
tracked separately as gap 26.
|
||||
2. **Whichever concurrency policy is in force would collapse them regardless.**
|
||||
The shipped default is `skip_if_active` — both `routines/handler.go`'s create
|
||||
path and the `office_routines.concurrency_policy` column default name it, and
|
||||
it drops a request while a run is in flight. `coalesce_if_active`, which only
|
||||
the pre-installed coordinator routine sets, merges the request into the
|
||||
in-flight run instead. Either way N requests become one wake. A fan-out is
|
||||
observable only under the third policy, `always_create` — that is the
|
||||
routine-layer name an operator sets; the wakeup layer calls the same policy
|
||||
`always_enqueue` after `dispatcher.go` maps it.
|
||||
3. **Under `always_create` it would fan out into a system with no
|
||||
work-in-progress limit and a budget check that fails open.**
|
||||
`max_concurrent_sessions` is persisted and has no scheduler consumer;
|
||||
`checkBudget` returns allow on checker error. Both are separately carded and
|
||||
unshipped. Building the fan-out first builds the hazard before the two
|
||||
controls that would bound it.
|
||||
4. **There is no per-tick work to replay.** A lightweight routine's payload is
|
||||
the routine id plus variables, and the time-valued built-ins (`{{date}}`,
|
||||
`{{datetime}}`) are resolved at dispatch time, not at tick time. A replayed
|
||||
missed tick would carry today's values and be indistinguishable from the
|
||||
current one.
|
||||
|
||||
## The tick
|
||||
|
||||
`computeRoutineMissed` is replaced by a function returning one value object
|
||||
instead of `(int, time.Time, error)`, so the caller receives the computation
|
||||
error and handles trigger state explicitly:
|
||||
|
||||
```go
|
||||
type catchUpResult struct {
|
||||
ElapsedTicks int // >= 1 on success; includes the tick due now
|
||||
FirstMissedAt time.Time // zero when ElapsedTicks <= 1
|
||||
Truncated bool // ElapsedTicks reached the cap with ticks still pending
|
||||
NextRunAt time.Time // success or recoverable-error re-arm
|
||||
Unknown bool // the walk failed; ElapsedTicks is not meaningful
|
||||
}
|
||||
```
|
||||
|
||||
Invariants the constructor guarantees, so no call site has to restate them:
|
||||
|
||||
- `NextRunAt` is strictly after the processing instant on a successful walk or
|
||||
a recoverable failure. On `Unknown`, `computeCatchUp` supplies the
|
||||
processing instant plus 24 hours. `ErrUnsatisfiableCron` leaves the claimed
|
||||
trigger disarmed; another failure re-arms it and dispatches one run without a
|
||||
gap summary. Both paths log the underlying error.
|
||||
- `FirstMissedAt` is the armed `next_run_at` at claim time — read directly, not
|
||||
derived from the walk, so it is exact even when `Truncated` is true.
|
||||
- `Truncated` is true only when the walk stopped at the cap with the cursor
|
||||
still at or before the processing instant.
|
||||
|
||||
`missedTicks = ElapsedTicks - 1`, floored at zero. The catch-up policy decides
|
||||
whether that number is *reported*, never whether runs are *created*: the tick
|
||||
dispatches exactly one run either way. `skip_missed` and `summarize_missed`
|
||||
therefore differ in exactly one observable: whether the eligible lightweight
|
||||
run's assembled prompt includes the gap context. Whether a taskless run reaches
|
||||
an agent session remains the separate shared-launcher contract described above.
|
||||
|
||||
**A gap summary exists only when `missedTicks >= 1`.** This single rule collapses
|
||||
what would otherwise be four near-identical states into two, and it is why
|
||||
`Unknown` needs no representation in storage:
|
||||
|
||||
- `Unknown` (the walk failed) drives the error path and no gap summary. The
|
||||
caller either disarms an unsatisfiable trigger or re-arms a recoverable
|
||||
failure for 24 hours and dispatches one run. A due trigger has at least one
|
||||
elapsed tick but may have no *missed* one, so an unmeasured gap is a false
|
||||
positive (AC-001.6).
|
||||
- `catch_up_max == 1` makes `missedTicks` structurally zero, so that
|
||||
configuration never produces a summary. That is a named consequence, not an
|
||||
edge case to detect (AC-002.11).
|
||||
- A truncated count is always at least `catch_up_max - 1`, so truncation and a
|
||||
zero count cannot co-occur for any `catch_up_max` above 1.
|
||||
|
||||
## Normalizing catch_up_max
|
||||
|
||||
AC-001.4 clamps `catch_up_max` at **use** time, which is the fail-open safety
|
||||
net: no stored value, however it got there, can stop a routine firing. On its
|
||||
own that reintroduces the defect this whole capability exists to close, one
|
||||
field over. A stored 5000 would keep being returned by `GetRoutine` /
|
||||
`ListRoutines`, rendered in the routine-detail form, and edited there, while the
|
||||
tick honoured 1000 — an operator sizing spend from the field would be wrong by
|
||||
a factor of five, which is the Overview's disagreement table with a different
|
||||
number in it.
|
||||
|
||||
Note the divergence is not created by the upper bound; it exists today on the
|
||||
lower one. `CreateRoutine` / `CreateRoutineTx` clamp `<= 0` to 25 at write time,
|
||||
but `UpdateRoutine` (`repository/sqlite/routines.go:200`) writes the field with
|
||||
no bound at all and `routines/handler.go:297` assigns it unvalidated, so a
|
||||
stored 0 is reachable now and already reads back as 0 while the tick uses 25.
|
||||
AC-001.4's upper bound extends an existing hole rather than digging a new one,
|
||||
which is why AC-001.13 closes both ends at once.
|
||||
|
||||
`CatchUpMax` is a plain `int` (`models/models.go:548`), not a named type, so the
|
||||
`sql.Scanner` hook [part 2](routine-catch-up-02.md) uses for
|
||||
`catch_up_policy` has nothing to attach to. Changing its type to get one would
|
||||
ripple through the DTO, the JSON contract and the web state for no behavioural
|
||||
gain — the same reasoning that keeps `## Out of scope`'s "Renaming
|
||||
`catch_up_max`" excluded.
|
||||
|
||||
**Normalize on write instead, which the policy field cannot do and this one
|
||||
can — through one named funnel, applied at the repository.**
|
||||
`models.NormaliseCatchUpMax(int) int` is the `catch_up_max` sibling of part 2's
|
||||
`NormaliseCatchUpPolicy`. It applies AC-001.4's clamp and is the only place the
|
||||
two bounds exist in Go, as named constants beside it. Today neither bound has a
|
||||
single point of change: `defaultCatchUpMax = 25` lives only in
|
||||
`routines/service.go:401` and no `1000` appears anywhere.
|
||||
|
||||
**It is applied in the repository, not in the handlers, because the repository
|
||||
is the only exhaustive choke point.** `catch_up_max` reaches SQL at exactly two
|
||||
statements: `insertRoutine` (`repository/sqlite/routines.go:150`), the shared
|
||||
INSERT that both `CreateRoutine` and `CreateRoutineTx` call, and `UpdateRoutine`
|
||||
(:200). Config-sync's own update path, `updateRoutineConfigFields` (:243), does
|
||||
not write the column at all. Normalizing in those two functions covers every
|
||||
present and future writer by construction.
|
||||
|
||||
Clamping in `routines/handler.go` instead would not, and the enumeration is the
|
||||
whole point of this section: **two independent service layers reach the same
|
||||
repository methods without passing through that handler** —
|
||||
`office/service.Service.CreateRoutine` and `UpdateRoutine` (`service/service.go`
|
||||
:713 and :732), the second of which `config_import.go`'s `applyRoutines` (:259)
|
||||
calls for every imported routine. That is the same bypass shape that lost
|
||||
`catch_up_policy` to `CreateRoutineTx`, and the same reasoning part 2 uses to put
|
||||
the policy's read-side normalization in `sql.Scanner` rather than in each query.
|
||||
The existing `<= 0 → 25` clamps in `CreateRoutine` and `CreateRoutineTx` are
|
||||
removed in favour of the funnel call in `insertRoutine`, so the lower bound stops
|
||||
being stated twice as well.
|
||||
|
||||
Existing rows are corrected by one `r.migrate.Apply` step alongside the policy
|
||||
migration. This SQL is the one place the bounds are restated outside the funnel,
|
||||
so it carries the same two values by construction:
|
||||
|
||||
```sql
|
||||
UPDATE office_routines SET catch_up_max = 25 WHERE catch_up_max < 1;
|
||||
UPDATE office_routines SET catch_up_max = 1000 WHERE catch_up_max > 1000;
|
||||
```
|
||||
|
||||
The boundary is exclusive on both ends and the SQL says so: `< 1` and `> 1000`,
|
||||
so a stored 1 and a stored 1000 are both left alone, matching AC-001.4's "less
|
||||
than 1" and "greater than 1000" exactly. There is no null case to consider —
|
||||
the column is `catch_up_max INTEGER NOT NULL DEFAULT 25` (`base.go:424`), so
|
||||
every row matches one predicate or neither, and a `NULL` cannot silently escape
|
||||
a comparison. Both statements are idempotent, and their order relative to the
|
||||
`office_routines` table rebuild in [part 2](routine-catch-up-02.md) does not
|
||||
matter: the rebuild copies column values verbatim, so normalizing before or
|
||||
after it yields the same rows.
|
||||
|
||||
Four consequences of clamping at the write, stated because a builder would
|
||||
otherwise have to derive them. **The funnel reassigns `routine.CatchUpMax` on the
|
||||
passed-in struct, before the statement binds that field**, rather than
|
||||
normalizing the bind parameter alone — mutating
|
||||
in place as the `CreateRoutine` clamp it replaces already does
|
||||
(`routines.go:125-127`), with `UpdateRoutine` gaining the same behaviour.
|
||||
Load-bearing, not stylistic: `routines/handler.go` serializes the SAME `*Routine`
|
||||
it handed the service on both paths (`:95` create, `:136` update), so a bind-only
|
||||
clamp would store 1000 while that one response reported the submitted 5000 —
|
||||
AC-001.13's own divergence, reopened for one round-trip.
|
||||
`NormaliseCatchUpMax` is a **fixpoint** —
|
||||
normalizing an already-normalized value returns it unchanged — so applying it
|
||||
twice is safe and a belt-and-braces call in a handler cannot corrupt anything.
|
||||
Two concurrent updates of one routine row stay last-write-wins, unchanged by this
|
||||
design, and both writers store a normalized value, so the row is in range
|
||||
whichever lands second. And because the clamp sits on what the repository
|
||||
*writes* rather than on what the request *carries*, an update that omits
|
||||
`catch_up_max` altogether — it is `*int` with `omitempty` (`routines/dto.go:24`),
|
||||
so omitting it means "leave it alone" — still rewrites the stored value
|
||||
normalized. An out-of-range row is therefore corrected by the next edit routed
|
||||
through `Repository.UpdateRoutine`, not only by the migration. It is **not**
|
||||
corrected by config-sync's column-scoped update (`config/import.go`,
|
||||
`configsync/reconcile_routines.go`), which does not write the column at all, so a
|
||||
routine only ever touched that way waits for the migration.
|
||||
|
||||
Write-side normalization plus a migration makes AC-001.4's use-time clamp
|
||||
belt-and-braces rather than load-bearing, exactly as the `sql.Scanner` does for
|
||||
the policy: a row that somehow escapes both still ticks correctly, it just also
|
||||
now reads correctly. AC-001.4 is unchanged and still governs the tick.
|
||||
|
||||
## Ordering, claiming and re-arming
|
||||
|
||||
- `GetDueTriggers` gains `ORDER BY next_run_at ASC, id ASC`. `id` is the named
|
||||
tiebreak column; it is a primary key, so the order is total and stable.
|
||||
- Claiming is unchanged. The CAS on `next_run_at` already gives exactly one
|
||||
winner; the loser returns before computing or recording anything.
|
||||
- The claim-then-arm window is closed by a reconciliation pass in
|
||||
`TickScheduledTriggers`: enabled cron triggers with a null `next_run_at` are
|
||||
armed to the first match strictly after the processing instant and are **not**
|
||||
dispatched in that tick. Re-arming rather than firing is deliberate: the row
|
||||
cannot distinguish "crashed after claiming" from "crashed after claiming and
|
||||
dispatching", and a spurious wake is the more expensive error.
|
||||
- **The pass runs after the due-trigger loop, and only on stale rows.** A null
|
||||
`next_run_at` is precisely what `ClaimTrigger` writes to take a claim, so a
|
||||
live claim and an abandoned one are identical in the row. Without a second
|
||||
predicate the pass would arm a trigger whose claimant is still mid-tick.
|
||||
`ClaimTrigger` also sets `updated_at`, which is the discriminator: the pass
|
||||
considers only rows whose `updated_at` is older than `catchUpReclaimAfter`,
|
||||
**60 seconds** — two scheduler intervals, so a claim taken in this tick or the
|
||||
one before is never reconciled, while an abandoned one is recovered on the
|
||||
following tick. Running after the due-trigger loop means a trigger claimed and
|
||||
armed within this same tick is already non-null and never reaches the pass.
|
||||
This is what AC-001.9's "none shall dispatch **by way of this path**" scopes:
|
||||
a live claimant's own dispatch is legitimate and unaffected.
|
||||
- That reconciliation write is also a CAS. The `UPDATE` carries `WHERE id = ?
|
||||
AND next_run_at IS NULL AND enabled = 1`, so a second reconciling processor
|
||||
affects zero rows and returns, and a trigger disabled or armed between the
|
||||
select and the write is left alone. Without the predicate two processors would
|
||||
each compute their own `NextCronTime(expr, tz, now)` from slightly different
|
||||
instants and the later write would silently move a trigger another had already
|
||||
armed. Note what the CAS does **not** buy: it cannot protect against a live
|
||||
claimant, whose own re-arm through `UpdateTriggerNextRun` is an unconditional
|
||||
write that wins regardless. The staleness guard is what keeps the two paths
|
||||
disjoint; the CAS only orders reconcilers among themselves.
|
||||
`UpdateTriggerNextRun` stays unconditional and the reconciliation path needs a
|
||||
conditional sibling rather than reusing it.
|
||||
- Arming on create uses `NextCronTime(expr, tz, now)` and rejects the trigger
|
||||
when that call errors (`CreateRoutineTrigger`, `service.go:305-314`).
|
||||
**AC-001.8's invariant is enforced in the service: `CreateRoutineTrigger` is
|
||||
the only place that may arm a cron trigger.** Today's guard is
|
||||
`if t.Kind == "cron" && t.CronExpression != ""`, which lets an empty
|
||||
expression fall through and persist a cron trigger with a null `next_run_at`;
|
||||
dropping that second condition and rejecting the empty case is the change
|
||||
AC-001.8 asks for. One caller reaches the repository directly —
|
||||
`infra/reconcile.go`'s `createTriggersForNewRoutines` calls
|
||||
`repo.CreateRoutineTrigger` past the service. It is named here because this
|
||||
design already lost `catch_up_policy` to exactly this shape of bypass
|
||||
(config-sync reaching `CreateRoutineTx`). It writes `Kind: "manual"` only, so
|
||||
it cannot violate the cron invariant and needs no change; the rule it inherits
|
||||
is that any future path creating a `cron` trigger goes through the service,
|
||||
not the repository. **There is no trigger-update path to align with it.** The
|
||||
repository interface exposes `CreateRoutineTrigger`, `UpdateTriggerNextRun`
|
||||
(the scheduler's own re-arm, `next_run_at` only) and `DeleteRoutineTrigger`;
|
||||
the HTTP surface is `GET`/`POST /routines/:id/triggers` and
|
||||
`DELETE /routine-triggers/:triggerId` plus the webhook fire; and the web
|
||||
client has only `deleteRoutineTrigger`. Changing a cron expression, timezone
|
||||
or enabled flag is therefore a delete followed by a create, which arms from
|
||||
the create instant and satisfies AC-001.8 with no new code. The update
|
||||
endpoint is excluded in the requirement's `## Out of scope`, so this design
|
||||
creates no trigger-mutation surface.
|
||||
|
||||
## Dispatch failure
|
||||
|
||||
AC-001.10 and AC-001.12 have no implementation to point at today:
|
||||
`models.RoutineRunStatusFailed` is declared in `models/enums.go` and **set
|
||||
nowhere in the tree**. Both dispatch paths lose their failures:
|
||||
|
||||
- Heavy (`materialiseHeavyRoutineRun`) returns an error when
|
||||
`EnsureRoutineWorkflow` or `CreateOfficeTaskInWorkflow` fails, without
|
||||
touching run status, so the run sits at `received` forever.
|
||||
- Lightweight (`materialiseLightweightRoutineRun`) logs a `Warn` and returns
|
||||
`nil` for both `CreateWakeupRequest` and `Dispatch` failures, so the failure
|
||||
never reaches `processCronTrigger` as an error at all.
|
||||
|
||||
Three changes, in the order the tick executes:
|
||||
|
||||
1. **`CreateRoutineRun` fails.** `dispatchRoutineRun` already returns
|
||||
`(nil, err)` here, before any run row exists. There is nothing to mark and
|
||||
nothing to re-attempt; the trigger was armed forward at the previous step, so
|
||||
the tick is simply lost. AC-001.12 makes that the stated contract rather than
|
||||
an accident. The caller logs and returns.
|
||||
2. **The lightweight path stops swallowing.** `CreateWakeupRequest` and
|
||||
`Dispatch` errors are returned rather than absorbed, with one exception: the
|
||||
sentinel the office repository returns for an idempotency-key conflict stays
|
||||
a no-op and is still absorbed as a `Warn`. That distinction is the whole
|
||||
point of AC-001.10's second sentence — a duplicate is a successful dedup, not
|
||||
a failed dispatch, and marking it `failed` would turn correct coalescing into
|
||||
a red run in the UI.
|
||||
3. **The status flip has one home.** `processCronTrigger` owns it: when
|
||||
`DispatchRoutineRunWithMissed` returns a non-nil run together with an error,
|
||||
it calls `UpdateRunStatus(run.ID, RoutineRunStatusFailed, "")`. Putting it in
|
||||
the caller rather than in each materialise path means the heavy and
|
||||
lightweight branches need no duplicate error handling, and the run is marked
|
||||
exactly once whichever branch failed.
|
||||
|
||||
The gap summary is untouched by any of this — it was written with the run row
|
||||
(see below), so a run that later goes `failed` still carries the gap that was
|
||||
measured for its tick.
|
||||
|
||||
## The gap summary
|
||||
|
||||
Stored on `office_routine_runs`, which both the lightweight and the heavy path
|
||||
create — one durable surface for both routine shapes. Three columns, added via
|
||||
`r.migrate.Apply` in `repository/sqlite/base_migrations.go` and to the
|
||||
`CREATE TABLE` in `base.go:448`:
|
||||
|
||||
| Column | Type | Meaning |
|
||||
|---|---|---|
|
||||
| `catch_up_missed_ticks` | `INTEGER` nullable | missed ticks; **NULL means no gap summary** |
|
||||
| `catch_up_first_missed_at` | `TIMESTAMP` nullable | exact timestamp of the first missed tick |
|
||||
| `catch_up_truncated` | `INTEGER NOT NULL DEFAULT 0` | 1 when the count is a lower bound |
|
||||
|
||||
Nullable rather than `NOT NULL DEFAULT 0` because AC-002.3 requires absence to
|
||||
be distinguishable from a measured zero. Per the rule above, a NULL
|
||||
`catch_up_missed_ticks` always implies `catch_up_first_missed_at` is NULL and
|
||||
`catch_up_truncated` is 0; the three columns are written together or not at all.
|
||||
|
||||
Explicit columns rather than folding into `trigger_payload`: that column is the
|
||||
resolved-variables contract, and a queryable "which routines resumed after a
|
||||
gap" is the operability question gap 21 will need.
|
||||
|
||||
The columns are written once, in the same repository call that creates the
|
||||
routine run, before the concurrency policy is applied. A run that is
|
||||
subsequently marked `skipped`, `coalesced` or `failed` therefore still carries
|
||||
the gap that was measured — the gap is a property of the tick, not of the
|
||||
dispatch outcome.
|
||||
|
||||
That write-once property, not any claim about deduplication, is what satisfies
|
||||
AC-002.10, and the ordering is worth stating because the obvious reading is
|
||||
wrong: `dispatchRoutineRun` calls `CreateRoutineRun` **unconditionally and
|
||||
first**, before `applyConcurrencyPolicy` and long before
|
||||
`materialiseLightweightRoutineRun` inserts the wakeup request carrying the
|
||||
idempotency key. `CreateRoutineRun` is neither gated by nor aware of the
|
||||
`agent_wakeup_requests` unique index, so a wakeup rejected as a duplicate does
|
||||
**not** prevent a second `office_routine_runs` row — it only prevents a second
|
||||
wake. Two rows, each with its own measured summary, is the correct outcome: no
|
||||
row is ever rewritten, because nothing writes these columns after the insert.
|
||||
|
||||
For a cron claim two rows are hard to reach anyway — `ClaimTrigger` is a CAS on
|
||||
`next_run_at`, the trigger is always re-armed strictly forward, and the
|
||||
idempotency key is per *trigger* and per wall-clock minute. The reachable
|
||||
multi-run cases are two triggers on one routine due in the same tick, and a
|
||||
manual or webhook fire alongside a cron fire; AC-002.12 gives the latter no gap
|
||||
summary at all. Note that AC-002.10 constrains the agent-run
|
||||
`runs.context_snapshot` as well as these rows — see
|
||||
[Delivering the gap to the agent](#delivering-the-gap-to-the-agent), where the
|
||||
coalesce merge is the surface that can actually rewrite a delivered gap.
|
||||
|
||||
They surface on the routine-run API DTO and are the source for any future UI.
|
||||
|
||||
## Delivering the gap to the agent
|
||||
|
||||
Three hops, of which only the last is new.
|
||||
|
||||
1. `marshalRoutinePayload` already carries `missed_ticks`; it gains
|
||||
`missed_since` (RFC 3339) and `missed_truncated`. `wakeup.RoutinePayload`
|
||||
gains the matching fields. Both remain `omitempty`, so a gapless fire has a
|
||||
payload byte-identical to today's.
|
||||
2. **One payload column feeds two destinations, and only one of them may
|
||||
carry the gap.** `createFreshRun` sets `ContextSnapshot: req.Payload`
|
||||
verbatim (`wakeup/dispatcher.go:291`) and is unchanged — that is the path
|
||||
AC-002.5 needs the three fields on. The **coalesce** paths merge the same
|
||||
persisted `agent_wakeup_requests.payload` row into an existing run's
|
||||
snapshot with `json_patch`, RFC 7386 merge-patch, so every top-level key the
|
||||
incoming payload carries **overwrites** the one already there. Left alone
|
||||
that gives two wrong outcomes: a second gap-carrying routine wake replaces
|
||||
the first claim's measurement with its own, and because the three fields are
|
||||
`omitempty`, a *gapless* later wake does not clear them, so the run keeps a
|
||||
gap belonging to a tick it is not answering.
|
||||
|
||||
**There are TWO merge sites, not one, and they do not share an
|
||||
implementation.** `MarkWakeupRequestCoalesced`
|
||||
(`repository/sqlite/wakeup_requests.go:168`) calls
|
||||
`mergeWakeupPayloadIntoRunSnapshot` (:256, whose `json_patch` is at :265).
|
||||
`PromoteRunAndCoalesceWakeupIfQueued` (:193) does **not** call that helper:
|
||||
it carries its own inline copy of the identical `json_patch` statement at
|
||||
:229-238, executed on `tx` rather than `r.db` because it runs inside the
|
||||
promote transaction. `grep -n json_patch` over that file returns THREE lines:
|
||||
these two SQL sites, plus the helper's own doc comment, which names the
|
||||
function in prose and survives the unification.
|
||||
Naming only the helper would send a builder to patch one of them, and the
|
||||
one left unpatched is the promoted-run path — the same path
|
||||
[part 2](routine-catch-up-02.md)'s test plan already singles out as the one
|
||||
a naive implementation misses for the prompt gate.
|
||||
|
||||
**The strip cannot live at the routine layer.** The routine layer writes the
|
||||
payload once, at wakeup-request creation, and `agent_wakeup_requests` has a
|
||||
single `payload` column. At that moment it is not yet known whether the
|
||||
request will become a fresh run or be coalesced — the dispatcher decides
|
||||
later, and both destinations read the same stored row. Stripping at
|
||||
creation would strip the fresh-run path too, which AC-002.5 forbids; not
|
||||
stripping leaves both merge sites carrying the keys. So the placement this
|
||||
design previously asserted is not implementable, and the decision is
|
||||
corrected here.
|
||||
|
||||
**The strip happens at the merge, in exactly one place.** The two merge
|
||||
sites are unified: the `json_patch` statement moves into a single helper
|
||||
taking an executor (`sqlx.ExtContext`, satisfied by both `r.db` and `tx`),
|
||||
and `PromoteRunAndCoalesceWakeupIfQueued` calls it inside its transaction
|
||||
instead of duplicating the SQL. That refactor is what makes "one merge path"
|
||||
a true statement rather than the false one this design shipped. The helper
|
||||
patches `json_remove(payload, '$.missed_ticks', '$.missed_since',
|
||||
'$.missed_truncated')` rather than `payload`.
|
||||
|
||||
Putting three catch-up key names in an otherwise generic helper is the cost,
|
||||
and it is the right trade against the alternative of a second payload column
|
||||
read only by `createFreshRun`: `json_remove` of an absent key is a no-op in
|
||||
SQLite (verified: `json_remove('{"a":1}', '$.missed_ticks')` returns
|
||||
`{"a":1}`), so the helper stays byte-for-byte correct for every other wakeup
|
||||
source, none of which writes these keys. A named constant holds the key list
|
||||
beside the helper so the two never drift.
|
||||
|
||||
The decision itself is unchanged: the three keys are written once, by the
|
||||
wake that created the run, and no coalesced request adds, changes or removes
|
||||
them (AC-002.10). Nothing is lost, because each claim's gap is already
|
||||
durable on its own `office_routine_runs` row and readable through the
|
||||
routine-run API (AC-002.6, AC-002.8). The assembled prompt carries one
|
||||
coherent statement about the tick its run was created for, rather than a
|
||||
silent mixture of two claims' measurements.
|
||||
3. `buildPromptContext` is called with `run.Payload`, which is `"{}"` for every
|
||||
wakeup-derived run. The call site at `scheduler_integration.go:375` passes
|
||||
`run.ContextSnapshot` as an additional argument, and `buildPromptContext`
|
||||
decodes it as `wakeup.RoutinePayload` when the run's reason is a routine
|
||||
dispatch. **The constant to test is not `RunReasonRoutine`, which does not
|
||||
exist.** `shared/runreasons.go` declares three:
|
||||
`RunReasonRoutineDispatchCron`, `RunReasonRoutineDispatchEvent`, and the
|
||||
legacy `RunReasonRoutineDispatch`. The decode gates on **all three**, not on
|
||||
Cron alone. Cron alone is the tempting choice, since only a cron claim ever
|
||||
measures a gap (AC-002.12), and it is wrong:
|
||||
`PromoteRunAndCoalesceWakeupIfQueued` rewrites an in-flight run's `reason`,
|
||||
so a cron-created run carrying a correctly measured gap can be holding the
|
||||
Event reason by the time its prompt is built, and a Cron-only gate would drop
|
||||
that gap silently. Gating on all three is safe in the other direction,
|
||||
because a manual or webhook claim never writes the fields at all, so an
|
||||
Event-reason run has nothing to render. `PromptContext` gains
|
||||
`MissedTicks int`, `MissedSince string`, `MissedTruncated bool`, and
|
||||
`BuildPrompt` renders a wake-context line when `MissedTicks > 0`.
|
||||
|
||||
Scoping the decode to the routine reasons keeps every other run's prompt
|
||||
byte-identical, which is what makes this change safe to land without a prompt
|
||||
regression sweep across all reasons.
|
||||
|
||||
The heavy path does not deliver the gap to the prompt: it creates a task from
|
||||
the routine's template, and mutating a user's template text with scheduler
|
||||
metadata is a worse contract than leaving the gap on the routine run where the
|
||||
API exposes it (AC-002.6).
|
||||
|
||||
## Renaming the policy, documentation and tests
|
||||
|
||||
REQ-003's rename, the corrections to `scheduler-01.md` / `scheduler-02.md`, and
|
||||
the test plan for both parts live in
|
||||
[part 2](routine-catch-up-02.md). They are a separate deliverable: this part
|
||||
changes what the system does on resume, part 2 changes what every surface says
|
||||
about it.
|
||||
@@ -0,0 +1,291 @@
|
||||
---
|
||||
status: draft
|
||||
system: office
|
||||
requirements:
|
||||
- REQ-OFFICE-ROUTINE-CATCHUP-003
|
||||
---
|
||||
|
||||
# Office Routine Catch-Up System Design, part 2: renaming the policy
|
||||
|
||||
Part 1 ([routine-catch-up-01.md](routine-catch-up-01.md)) covers the tick, the
|
||||
claim, the gap summary and its delivery to the agent — REQ-001 and REQ-002.
|
||||
This part covers REQ-003 alone: renaming `enqueue_missed_with_cap` to
|
||||
`summarize_missed` across every surface that names it, and the test plan for
|
||||
both parts.
|
||||
|
||||
The split is by deliverable, not by convenience. Part 1 changes what the system
|
||||
*does* on resume; this part changes what every surface *says* about it, which is
|
||||
a wider but shallower blast radius — a Go enum, six creation paths, a SQLite
|
||||
table rebuild, two frontend files, five locale catalogs and two design
|
||||
documents.
|
||||
|
||||
## Renaming the policy
|
||||
|
||||
`models.CatchUpPolicySummarizeMissed = "summarize_missed"` replaces
|
||||
`CatchUpPolicyEnqueueMissedWithCap` as the canonical value.
|
||||
`CatchUpPolicyEnqueueMissedWithCap` is retained as a deprecated alias constant,
|
||||
not deleted, so the normalizer has a name to compare against.
|
||||
|
||||
One normalization funnel, `models.NormaliseCatchUpPolicy(string) RoutineCatchUpPolicy`:
|
||||
|
||||
| Input | Result |
|
||||
|---|---|
|
||||
| `summarize_missed` | `summarize_missed` |
|
||||
| `enqueue_missed_with_cap` | `summarize_missed` (deprecated alias) |
|
||||
| `skip_missed` | `skip_missed` |
|
||||
| empty, or anything else | `summarize_missed` |
|
||||
|
||||
`RoutineCatchUpPolicy.Valid()` accepts the alias so an API client sending the
|
||||
old value gets 200, not 400; the persisted and returned value is the new one,
|
||||
satisfying AC-003.2 and AC-003.4 together.
|
||||
|
||||
**Where the funnel is applied.** AC-003.1 binds every creation path, not just
|
||||
the HTTP one, so the enumeration has to be exhaustive:
|
||||
|
||||
| Entry | Why it is on the list |
|
||||
|---|---|
|
||||
| `routines/handler.go` create | Sets its own default when the request omits the field |
|
||||
| `routines/handler.go` update | Assigns the request value straight onto the routine |
|
||||
| `Repository.CreateRoutine` | **Injects the literal old value** when the field is empty |
|
||||
| `Repository.CreateRoutineTx` | Same injection, separate copy of the code |
|
||||
| `CreateDefaultCoordinatorRoutine` | Hard-codes the old constant on the pre-installed routine |
|
||||
| Read paths (`GetRoutine`, `ListRoutines`) | Normalize the alias and unknown values on the way out |
|
||||
|
||||
The two repository entries are the ones a line-by-line reading of the HTTP layer
|
||||
misses, and they are not hypothetical: config-sync's routine reconciler builds a
|
||||
`models.Routine` with **no `CatchUpPolicy` set at all** and calls
|
||||
`CreateRoutineTx` directly, bypassing the handler and its defaulting. Left
|
||||
unchanged, every config-sync-created routine would keep writing the retired
|
||||
string forever, after the one-time migration had cleaned up every pre-existing
|
||||
row — a permanent AC-003.1 violation on a live path.
|
||||
|
||||
**Read-side mechanism.** `GetRoutine` and `ListRoutines` are `SELECT *` through
|
||||
`sqlx` `StructScan`/`SelectContext`; there is no post-scan hook to put the
|
||||
normalizer in, so "the row scanner" is not an existing function to edit.
|
||||
Implement `sql.Scanner` on `RoutineCatchUpPolicy` and normalize there. That
|
||||
covers every present and future read path in one place, where per-call
|
||||
post-processing would have to be added to each new query and would be forgotten
|
||||
by exactly the same reasoning that produced this defect. `driver.Valuer` is not
|
||||
needed: writes already go through the funnel above.
|
||||
|
||||
Note the earlier claim that "Office routines are not part of the config-sync
|
||||
YAML contract" is about the *field* — the YAML carries no catch-up policy — and
|
||||
must not be read as "config-sync does not create routines". It does.
|
||||
|
||||
**Migrating persisted rows and the stored default.** Two separate problems, and
|
||||
one `UPDATE` only solves the first.
|
||||
|
||||
Row *values* migrate with a single `r.migrate.Apply` step:
|
||||
`UPDATE office_routines SET catch_up_policy = 'summarize_missed' WHERE catch_up_policy = 'enqueue_missed_with_cap'`.
|
||||
With the `sql.Scanner` normalization above this is belt-and-braces rather than
|
||||
load-bearing — a row that escapes it still reads correctly.
|
||||
|
||||
The column's *stored default* needs more, and AC-003.3 requires it on upgraded
|
||||
installs too. Editing the `DEFAULT 'enqueue_missed_with_cap'` text in
|
||||
`createRoutineTables` fixes nothing on an existing database: the statement is
|
||||
`CREATE TABLE IF NOT EXISTS` and is a no-op there, and SQLite has no
|
||||
`ALTER COLUMN ... SET DEFAULT`. Changing it requires a table rebuild, following
|
||||
the pattern this repository already documents (`apps/backend/AGENTS.md`,
|
||||
"Table-rebuild migrations") and already uses in this very file —
|
||||
`runTaskPriorityRecreate` in `base_migrations.go` is the worked precedent:
|
||||
acquire a dedicated connection, `PRAGMA foreign_keys=OFF` with a deferred
|
||||
re-enable, create `office_routines_new` with the corrected default, `INSERT ...
|
||||
SELECT` every column, `DROP TABLE office_routines`, rename.
|
||||
|
||||
The `foreign_keys=OFF` is not incidental. `office_routine_runs` declares
|
||||
`FOREIGN KEY (routine_id) REFERENCES office_routines(id) ON DELETE CASCADE`, so
|
||||
dropping the old table with enforcement on would cascade every routine run out
|
||||
of existence. Mirror every column in the replacement `CREATE TABLE` and the copy
|
||||
list, and add a replay regression test proving values and timestamps survive —
|
||||
the run rows in particular.
|
||||
|
||||
Leaving the default clause stale because no insert exercises it was rejected: an
|
||||
operator reading an upgraded install's live schema would see a default naming a
|
||||
value the system no longer treats as canonical, the same
|
||||
disagreement-between-surfaces defect this capability exists to close.
|
||||
|
||||
**Frontend.** Replace *every* occurrence of the `enqueue_missed_with_cap`
|
||||
literal in `create-routine-dialog.tsx` and `routine-detail-view.tsx` — do not
|
||||
work from a fixed line list, which is what produced the omission this design
|
||||
previously shipped. There are four distinct kinds of site, and only two are
|
||||
`SelectItem` values:
|
||||
|
||||
| Kind | Effect if missed |
|
||||
|---|---|
|
||||
| `SelectItem value=` (both files) | The control writes the retired value |
|
||||
| Create-dialog initial state default | New routines are created with the retired value |
|
||||
| Detail-view fallback default | An unset policy renders as the retired value |
|
||||
| `catchUpPolicy === "enqueue_missed_with_cap"` guards (both files) | **The `catch_up_max` input stops rendering entirely** |
|
||||
|
||||
The last row is why AC-003.8 exists. Those two comparisons decide whether the
|
||||
cap input is shown at all; once every routine reads as `summarize_missed` they
|
||||
are permanently false, so a builder who updates only the option values and
|
||||
defaults ships a UI where the cap field has silently vanished — while still
|
||||
passing AC-003.4 and AC-003.6, neither of which observes the control.
|
||||
|
||||
The i18n key `office:enqueueMissedWithCap` is replaced (not re-worded in place —
|
||||
the key name would then lie too) by `office:summarizeMissed`, and
|
||||
`office:beyondThisCountMissedTicksAre` is replaced by a key stating that the
|
||||
bound is on the ticks counted and reported. Both across `en`, `pt-pt`, `zh-cn`,
|
||||
`zh-hk`, `zh-tw`; use `pnpm run i18n:zh-hant` for the Traditional pair and
|
||||
`pnpm run i18n:pseudo` to regenerate the pseudo catalog. No em dash in any
|
||||
locale value.
|
||||
|
||||
## Documentation consistency
|
||||
|
||||
Three distinct corrections, and AC-003.7 requires all three. Work by **grepping
|
||||
both files for `enqueue_missed_with_cap`, for "missed", and for `missed_ticks`,
|
||||
and correcting every hit** — the lists below name the sites that exist today and
|
||||
are illustrative, not a work list to tick off. This design shipped that mistake
|
||||
once already, in the frontend notes above, and repeating it here would leave
|
||||
the fourth surface disagreeing after a capability whose entire purpose is to
|
||||
stop surfaces disagreeing.
|
||||
|
||||
*Statements that imply multiple fires:* `scheduler-01.md`'s "Catch-up policy"
|
||||
section ("fire missed ticks up to the cap"), `scheduler-02.md`'s failure-mode
|
||||
table row, its restart scenario, and the retention paragraph's "catch-up cap
|
||||
(default 25) drops missed routine ticks" sentence. Each is corrected to state
|
||||
one summarized wake and to reference
|
||||
[the requirement](../requirements/routine-catch-up.md) rather than restating its
|
||||
terms.
|
||||
|
||||
*Places that merely name the value:* `scheduler-01.md`'s routine-field list
|
||||
(`catch_up_policy: enqueue_missed_with_cap (default, cap 25)`), its worked
|
||||
routine-config example block, and `scheduler-02.md`'s schema column comment.
|
||||
None claims multiple fires, so the first correction leaves them untouched — and
|
||||
they would then name a policy value the system no longer accepts as canonical,
|
||||
which is the exact class of defect this capability exists to remove. All become
|
||||
`summarize_missed`, with the alias mentioned only where it is labelled
|
||||
deprecated.
|
||||
|
||||
*Places that document the routine wakeup payload's shape:* `scheduler-01.md`'s
|
||||
run-reason table row (`{routine_id, variables, missed_ticks?}`) and
|
||||
`scheduler-02.md`'s `RoutinePayload` struct listing, whose `MissedTicks` field
|
||||
carries a "when catch-up cap collapsed N fires" comment. Neither names the
|
||||
policy value nor claims multiple fires, so neither correction above reaches
|
||||
them, yet both describe a contract this design extends with `missed_since` and
|
||||
`missed_truncated`. Each gains the two new fields, and the `MissedTicks` comment
|
||||
is corrected to say the count is of ticks *counted and reported*, never fired.
|
||||
|
||||
`docs/specs/office/README.md` gains both new files in its specification map.
|
||||
|
||||
## Testing
|
||||
|
||||
Backend, in `internal/office/routines/`:
|
||||
|
||||
- `catch_up_test.go` already covers the counting loop and the default cap; it is
|
||||
extended for `FirstMissedAt` exactness under truncation, `Truncated`, the
|
||||
`ElapsedTicks == 1` no-summary case, the failure path's strictly-after re-arm,
|
||||
and both `catch_up_max` clamps — `0 -> 25` and `5000 -> 1000` (AC-001.4). The
|
||||
existing tests referencing `CatchUpPolicyEnqueueMissedWithCap` move to the new
|
||||
constant, with one test retained on the alias to prove normalization.
|
||||
- The `catch_up_max == 1` boundary (AC-002.4, AC-002.11): a gap spanning many
|
||||
ticks records **no** gap summary, and in particular does not record
|
||||
`truncated = true` with a zero count. This is the case where the guard added
|
||||
to AC-002.4 is the only thing standing between two ACs, so assert the absence
|
||||
directly rather than inferring it from a count.
|
||||
- A tick test asserting exactly one routine run and one wakeup request for a
|
||||
gap spanning more than `catch_up_max` ticks — the AC-001.1 regression that
|
||||
would have caught the original misnaming.
|
||||
- A `GetDueTriggers` ordering test over rows with equal `next_run_at`.
|
||||
- Reconciliation (AC-001.9): an enabled cron trigger with a null `next_run_at`
|
||||
and a stale `updated_at` is armed and does not dispatch; a second reconciler
|
||||
affects zero rows and does not move `next_run_at`; and a trigger whose
|
||||
`updated_at` is inside `catchUpReclaimAfter` is left untouched, which is the
|
||||
live-claim case the CAS alone does not cover.
|
||||
- Re-arm failure (AC-001.2): `UpdateTriggerNextRun` returns an error, no routine
|
||||
run is created for that claim, and a later tick's reconciliation arms the
|
||||
trigger without dispatching.
|
||||
- A malformed-expression case asserting the re-arm lands exactly 24 hours after
|
||||
the processing instant and that a second tick a day later dispatches once
|
||||
more, not repeatedly (AC-001.11).
|
||||
- Dispatch-failure cases (AC-001.10): heavy-path task creation fails and the run
|
||||
reads `failed`; lightweight `CreateWakeupRequest` fails and the run reads
|
||||
`failed`; an idempotency-conflict sentinel does **not** mark the run failed.
|
||||
And AC-001.12: `CreateRoutineRun` itself fails, no run row exists, and the
|
||||
trigger stays armed forward.
|
||||
- A write-once case for AC-002.10: a run carrying a gap summary is transitioned
|
||||
through `coalesced` and `failed`, and the three columns are unchanged after
|
||||
each.
|
||||
|
||||
Wakeup, in `internal/office/wakeup/` and its repository — the coalesce merge
|
||||
(AC-002.10). **Every case below runs against BOTH merge entry points**,
|
||||
`MarkWakeupRequestCoalesced` and `PromoteRunAndCoalesceWakeupIfQueued`, as a
|
||||
table test over the two. That is not belt-and-braces: until the refactor in
|
||||
[part 1](routine-catch-up-01.md#delivering-the-gap-to-the-agent) lands they are
|
||||
two separate SQL statements, and a suite that exercises only the helper passes
|
||||
green with the promoted-run path fully broken — the exact failure the design
|
||||
text previously invited. What the table does **not** do is catch a third merge
|
||||
site: a table over two named entry points cannot observe a statement that is not
|
||||
in it, so do not rely on it for that. The guard for that is a separate
|
||||
source-level pinning assertion: that `json_patch` appears exactly once in
|
||||
`wakeup_requests.go` **outside comments**, once the unification has landed.
|
||||
|
||||
The qualifier is the whole assertion, not pedantry. `grep -c json_patch` on that
|
||||
file returns **three** lines today, and only two of them are SQL: the third is the
|
||||
helper's own doc comment, which names the function in prose and survives the
|
||||
refactor untouched. An unqualified "exactly once" would therefore FAIL against a
|
||||
correct implementation, and the builder would be left inventing the real
|
||||
predicate. Assert on SQL occurrences — parse the file, or match the statement
|
||||
rather than the bare identifier.
|
||||
|
||||
The in-tree precedent for a test that reads Go source and asserts a structural
|
||||
count is `internal/common/subproc/raw_git_audit_test.go`'s
|
||||
`TestProductionGitCommandsUseTheAdmissionSeam`, which walks the tree with
|
||||
`go/parser` and reports violations by file and line. It is **not** the auth
|
||||
middleware allowlist: `apps/backend/CLAUDE.md` describes that one as a "pinning
|
||||
test", but `auth/httpmw/middleware_test.go`'s `TestEnabledModeAllowlistMatrix`
|
||||
issues real HTTP requests and asserts status codes per path — a behavioral
|
||||
matrix, and the wrong shape to model this on.
|
||||
|
||||
Two directions per entry point, and the second is the one `omitempty` alone
|
||||
would pass for the wrong reason: a gap-carrying routine wake coalescing into a
|
||||
gapless in-flight run leaves that run gapless, and a gapless wake coalescing
|
||||
into a gap-carrying run leaves the original gap intact rather than
|
||||
half-overwriting it. Add a third: a non-routine wakeup source whose payload
|
||||
contains none of the three keys round-trips byte-identically through the merge,
|
||||
which is what makes the `json_remove` safe to put in a shared helper.
|
||||
|
||||
Repository: round-trip of the three columns including the NULL-versus-zero
|
||||
distinction, in `routines_config_fields_test.go`'s neighbourhood. Plus a
|
||||
default-injection test covering `CreateRoutine` and `CreateRoutineTx` with an
|
||||
empty `CatchUpPolicy` (AC-003.1), and a replay regression test for the table
|
||||
rebuild proving every column and timestamp survives and that
|
||||
`office_routine_runs` rows are not cascaded away (AC-003.3).
|
||||
|
||||
`catch_up_max` write normalization (AC-001.13) is tested on **every** path that
|
||||
can currently store an out-of-range value, which is what makes the test
|
||||
meaningful — and that is both repository statements, not only the update.
|
||||
`CreateRoutine` and `CreateRoutineTx` clamp `<= 0 → 25` today and have no upper
|
||||
bound at all, so a create carrying `5000` persists `5000`; the create paths are
|
||||
therefore in scope by the same rule that puts the update path there. So:
|
||||
`CreateRoutine`, `CreateRoutineTx` and `UpdateRoutine` each with `0` and with
|
||||
`5000`, each read back at the clamped value rather than the submitted one, plus
|
||||
the handler's create and update paths end to end — **asserting on the create and
|
||||
update RESPONSE BODY itself, not only on a subsequent `GET`**. Those two
|
||||
responses are the only place part 1's in-place-reassignment decision
|
||||
([Normalizing catch_up_max](routine-catch-up-01.md#normalizing-catch_up_max)) is
|
||||
observable: both handlers serialize the same `*Routine` they passed to the
|
||||
service, so a test that POSTs `5000` and then re-reads the routine with `GET`
|
||||
passes identically whether the funnel mutated the struct or only the SQL bind,
|
||||
and would not catch the one divergence AC-001.13 exists to prevent. Cover
|
||||
`office/service.Service.UpdateRoutine` as well — it is the second service layer
|
||||
reaching the repository without the routines handler, and it is what
|
||||
`config_import.go` calls, so it is the regression that would prove the clamp was
|
||||
put in the handler instead of the repository. Plus a migration
|
||||
replay proving a pre-existing row at `0` and one at `5000` are corrected in
|
||||
place, and that a row already in range is left untouched — the read-back is the
|
||||
assertion AC-001.13 actually names, so assert on the returned routine, not on
|
||||
the tick's behaviour, which AC-001.4 already covers.
|
||||
|
||||
Prompt: `BuildPromptContextForTest` cases asserting the wake context renders for
|
||||
a run with a gap under **each** of the three routine reasons — including
|
||||
`RunReasonRoutineDispatchEvent`, which is the promoted-run case a Cron-only gate
|
||||
would silently drop (AC-002.5) — and that a non-routine reason with the same
|
||||
`context_snapshot` renders nothing.
|
||||
|
||||
Frontend: a store/API test that `summarize_missed` round-trips, a test that the
|
||||
`catch_up_max` control renders under the summarizing policy and not under
|
||||
`skip_missed` (AC-003.8), and `pnpm run i18n:check` for the five locales.
|
||||
|
||||
E2E is proposed rather than assumed; the surfaces are listed in the task plan.
|
||||
@@ -55,7 +55,7 @@ A SQLite-persisted queue of "wake this agent up" requests. Every periodic, event
|
||||
|
||||
| Source | Trigger | Payload |
|
||||
|--------|---------|---------|
|
||||
| `routine` | A routine's cron / webhook / manual trigger fires | `{routine_id, variables, missed_ticks?}` |
|
||||
| `routine` | A routine's cron / webhook / manual trigger fires | `{routine_id, variables, missed_ticks?, missed_since?, missed_truncated?}` |
|
||||
| `comment` | Comment posted on a task assigned to this agent (non-self). Also the channel pathway: inbound Telegram/Slack messages become comments on a channel task. | `{task_id, comment_id}` |
|
||||
| `agent_error` | A sub-agent's session failed (escalation to coordinator) | `{failed_agent_id, failed_session_id?, run_id?, error}` |
|
||||
| `self` | Agent self-wake via tool call | `{reason, payload?}` |
|
||||
@@ -112,7 +112,7 @@ Routine fields:
|
||||
- `assignee_agent_instance_id` - who gets the resulting task / run.
|
||||
- `status`: `active` | `paused` | `archived`.
|
||||
- `concurrency_policy`: `coalesce_if_active` (default) | `skip_if_active` | `always_enqueue`.
|
||||
- `catch_up_policy`: `enqueue_missed_with_cap` (default, cap 25) | `skip_missed`.
|
||||
- `catch_up_policy`: `summarize_missed` (default, cap 25; `enqueue_missed_with_cap` is a deprecated alias) | `skip_missed`.
|
||||
- `catch_up_max`: integer, default 25.
|
||||
- `task_template`: JSON. Empty means **lightweight** routine (taskless run per fire). Non-empty means **heavy** routine (fresh task created on the `routine` workflow).
|
||||
- `variables`: declared template variables (type, default, required).
|
||||
@@ -144,9 +144,17 @@ Evaluated at dispatch by querying for an in-flight run for the same routine fing
|
||||
|
||||
#### Catch-up policy
|
||||
|
||||
If the scheduler was down and missed cron ticks:
|
||||
- `skip_missed`: fire only the current tick.
|
||||
- `enqueue_missed_with_cap` (default, cap 25): fire missed ticks up to the cap; dropped ticks are not recorded individually but summarized into the next prompt's wake context ("you missed N ticks since X").
|
||||
If the scheduler was down and missed cron ticks, resuming always produces exactly
|
||||
one run per due trigger, never one run per missed tick. `catch_up_policy` only
|
||||
governs whether that one run's gap is measured and reported:
|
||||
- `skip_missed`: no gap is recorded or reported.
|
||||
- `summarize_missed` (default, cap 25; `enqueue_missed_with_cap` is a deprecated
|
||||
alias): the gap (missed-tick count, first-missed timestamp, truncated flag) is
|
||||
measured, capped at `catch_up_max`, and summarized into the run's wake context
|
||||
("you missed N ticks since X").
|
||||
|
||||
See [the routine catch-up requirement](../requirements/routine-catch-up.md) for
|
||||
the full contract.
|
||||
|
||||
#### The pre-installed coordinator routine
|
||||
|
||||
@@ -158,7 +166,7 @@ description: "Wakes the coordinator every 5 minutes to check workspace a
|
||||
assignee_agent_id: <new coordinator agent id>
|
||||
status: active
|
||||
concurrency_policy: coalesce_if_active
|
||||
catch_up_policy: enqueue_missed_with_cap
|
||||
catch_up_policy: summarize_missed
|
||||
catch_up_max: 25
|
||||
task_template: ""
|
||||
variables: []
|
||||
|
||||
@@ -71,7 +71,7 @@ description TEXT
|
||||
assignee_agent_id TEXT NOT NULL
|
||||
status TEXT NOT NULL -- active | paused | archived
|
||||
concurrency_policy TEXT NOT NULL -- coalesce_if_active | skip_if_active | always_enqueue
|
||||
catch_up_policy TEXT NOT NULL -- enqueue_missed_with_cap | skip_missed
|
||||
catch_up_policy TEXT NOT NULL -- summarize_missed (enqueue_missed_with_cap deprecated alias) | skip_missed
|
||||
catch_up_max INT NOT NULL DEFAULT 25
|
||||
task_template TEXT NOT NULL DEFAULT '' -- JSON; empty -> lightweight
|
||||
variables TEXT NOT NULL DEFAULT '[]' -- JSON array
|
||||
@@ -148,9 +148,11 @@ last_wakeup_finished_at TIMESTAMP
|
||||
|
||||
```go
|
||||
type RoutinePayload struct {
|
||||
RoutineID string `json:"routine_id"`
|
||||
Variables map[string]any `json:"variables,omitempty"`
|
||||
MissedTicks int `json:"missed_ticks,omitempty"` // when catch-up cap collapsed N fires
|
||||
RoutineID string `json:"routine_id"`
|
||||
Variables map[string]any `json:"variables,omitempty"`
|
||||
MissedTicks int `json:"missed_ticks,omitempty"` // ticks counted and reported for this claim's gap, excluding the tick dispatched
|
||||
MissedSince string `json:"missed_since,omitempty"` // RFC3339 timestamp of the first missed tick
|
||||
MissedTruncated bool `json:"missed_truncated,omitempty"` // true when the gap exceeded catch_up_max
|
||||
}
|
||||
type CommentPayload struct {
|
||||
TaskID string `json:"task_id"`
|
||||
@@ -287,7 +289,7 @@ A formal per-route authorization model (workspace membership, admin role, RBAC o
|
||||
| Atomic checkout finds task locked by another agent | Wakeup skipped, no retry. |
|
||||
| Summary builder fails on successful taskless run | Previous summary left intact; failure logged; next successful run rebuilds. |
|
||||
| Coordinator's pre-installed routine fails to install at onboarding | Logged + warned; coordinator's agent detail UI shows "no scheduled wake-ups" empty state. User can install one manually. |
|
||||
| Routine cron tick missed (scheduler down) | `enqueue_missed_with_cap` (default): fire missed ticks up to cap=25 with "missed N ticks" in next prompt context. `skip_missed`: fire current tick only. |
|
||||
| Routine cron tick missed (scheduler down) | Resume always produces exactly one run, never one per missed tick. `summarize_missed` (default, cap 25): the gap is measured and summarized as "missed N ticks" in the next prompt context. `skip_missed`: no gap is recorded or reported. See [the routine catch-up requirement](../requirements/routine-catch-up.md). |
|
||||
| Webhook signature verification fails | Trigger rejected with 401; no routine run created. |
|
||||
| Graceful shutdown begins | Queue and cron loops cancel and join before repositories and SQLite close; no scheduler logs `database is closed`. |
|
||||
|
||||
@@ -297,7 +299,7 @@ Survives a kandev process restart: all `agent_wakeup_requests` rows including `q
|
||||
|
||||
Does NOT survive (reconstructed on next tick): in-memory claim leases - a `claimed` wakeup whose process died is picked up by the staleness/recovery path; the scheduler's claim query is the source of truth. The unstarted-task recovery sweep suppresses duplicates with its `NOT EXISTS` check on `runs`: any queued, claimed, or finished run of any age blocks redispatch, while failed and cancelled runs do not.
|
||||
|
||||
Retention: the idempotency lookup window is 24 hours, while persisted idempotency keys remain unique; summary cap 8 KB per row; routine run history retained for inspection (no automatic prune in scope here); catch-up cap (default 25) drops missed routine ticks beyond it (not recorded individually). A live linked task remains in the concurrency gate regardless of age.
|
||||
Retention: the idempotency lookup window is 24 hours, while persisted idempotency keys remain unique; summary cap 8 KB per row; routine run history retained for inspection (no automatic prune in scope here); catch-up cap (default 25) bounds only how many missed ticks are counted and reported in the gap summary, never how many runs are dispatched — see [the routine catch-up requirement](../requirements/routine-catch-up.md). A live linked task remains in the concurrency gate regardless of age.
|
||||
|
||||
The scheduler reads all `queued` and unexpired-retry wakeup requests on boot and resumes processing them.
|
||||
|
||||
@@ -326,7 +328,7 @@ The scheduler reads all `queued` and unexpired-retry wakeup requests on boot and
|
||||
|
||||
- **GIVEN** a routine with a webhook trigger and `signing_mode=hmac_sha256`, **WHEN** an external system POSTs to the trigger URL with valid signature and payload `{"branch": "release/2.0"}`, **THEN** the routine fires with `{{branch}}` resolved to "release/2.0" in the task template.
|
||||
|
||||
- **GIVEN** the scheduler was down for 3 hours, **WHEN** it restarts, **THEN** routines with `catch_up_policy=skip_missed` fire only the current tick; routines with `enqueue_missed_with_cap` fire missed ticks up to the cap (default 25), with overflow summarized as "missed N ticks" in the next prompt context.
|
||||
- **GIVEN** the scheduler was down for 3 hours, **WHEN** it restarts, **THEN** each due trigger produces exactly one run, regardless of `catch_up_policy`; routines with `catch_up_policy=skip_missed` record no gap; routines with `catch_up_policy=summarize_missed` (default) record the gap, capped at `catch_up_max` (default 25), summarized as "missed N ticks" in the next prompt context. See [the routine catch-up requirement](../requirements/routine-catch-up.md).
|
||||
|
||||
- **GIVEN** a user on the routines page, **WHEN** they click "Run Now" on a routine with a required `{{reason}}` variable, **THEN** a modal prompts for the variable value before firing.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user