fix(office): stop routines from bricking after their first fire - #3488
Conversation
…hook Review round 1 found the heavy-routine half of the routine-runs fix was dead in production: events.TaskMoved never carries to_step_name, so SyncRunStatus was never reached and heavy runs still gated on task_created until the 7-day backstop. The concurrency gate now self-heals by reading the linked task's real terminal state directly (applyConcurrencyPolicy -> selfHealIfTaskTerminal), independent of that event. Also stops recording a deduped lightweight fire as failed (ErrWakeupIdempotencyConflict is success by another fire, not a failure), and guards SyncRunStatus/GetRoutineRunByLinkedTaskID against an empty taskID matching an arbitrary lightweight run.
GitHub Actions
Inactive
|
Claude finished @nova28's task in 5m 15s —— View job FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready to merge The root cause analysis is correct and the fix is well-reasoned. The inline |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (10)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughRoutine runs now finalize lightweight dispatches immediately, ignore idempotency races as successful dispatches, recover stale heavy runs, and close linked runs when tasks reach terminal steps. SQLite adds repository operations for these lifecycle transitions. ChangesRoutine run lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to No concrete merge-blocking risk remains in the current changes. Sequence Diagram(s)sequenceDiagram
participant TaskEvent
participant OfficeService
participant RoutineService
participant SQLiteRepository
TaskEvent->>OfficeService: task.moved to terminal step
OfficeService->>RoutineService: SyncRunStatus(taskID, done or cancelled)
RoutineService->>SQLiteRepository: find linked routine run
RoutineService->>SQLiteRepository: conditionally close task_created run
SQLiteRepository-->>RoutineService: close result
RoutineService-->>OfficeService: synchronization result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit reads each line, Comment |
|
| Filename | Overview |
|---|---|
| apps/backend/internal/office/routines/service.go | Implements routine self-healing and lightweight finalization, but expires live gates, records failed wakeup dispatches as done, and includes comments that violate repository guidance. |
| apps/backend/internal/office/repository/sqlite/routines.go | Adds active-run expiration and task-state reconciliation, but omits the canonical terminal FAILED state and can ignore still-active old tasks. |
| apps/backend/internal/office/service/event_subscribers.go | Adds best-effort routine-run synchronization when terminal task-move events contain the required data. |
| apps/backend/internal/backendapp/main.go | Correctly wires the routine synchronizer before Office event subscribers are registered. |
| apps/backend/internal/office/routines/service_test.go | Adds substantial regression coverage, though it does not cover failed linked tasks, live tasks older than the cutoff, or dispatch errors leaving queued wakeups. |
| apps/backend/internal/office/repository/sqlite/routines_test.go | Covers the new repository operations but encodes expiration without checking a still-active linked task. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Fire[Routine fire] --> Gate{Concurrency gate}
Gate -->|active run found| TaskState{Linked task state}
TaskState -->|completed/cancelled| Close[Atomically close prior run]
TaskState -->|failed, currently missed| Block[Skip or coalesce until age cutoff]
TaskState -->|non-terminal but older than 7 days| Duplicate[Gate bypassed; duplicate task]
Close --> Materialize
Gate -->|no active run| Materialize{Routine type}
Materialize -->|heavy| Task[Create linked task]
Materialize -->|lightweight| Enqueue[Create wakeup request]
Enqueue --> Dispatch{Dispatch succeeds?}
Dispatch -->|yes| Done[Mark run done]
Dispatch -->|no, currently| FalseDone[Request remains queued; mark run done]
Reviews (1): Last reviewed commit: "fix(office): close the routine gate with..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 904f1cc252
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…res honestly PR review found two gaps in the routine-gate self-heal: GetTaskTerminalStatus didn't recognize a FAILED linked task as terminal, permanently gating the routine like the original bug; and a lightweight fire whose wakeup Dispatch call failed was still recorded as "done" even though nothing ever retries a stuck queued wakeup-request. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Thanks for the detailed review. I pushed The update keeps long-running tasks gated without an age cutoff, reconciles completed, failed, cancelled, archived, and missing linked tasks, checks older active runs, gives manual and webhook fires source-specific idempotency keys, records dispatch failures as failed, adds lookup indexes, and updates the scheduler specifications. Focused Office/backend race tests and specification lint pass. |
2 participants
Tip
PR walkthrough: Open the visual walkthrough
Today: A routine fires successfully once and then never fires again, silently, forever. The pre-installed "Coordinator heartbeat" routine self-bricked after its first fire: 323 consecutive coalesces over 28 days, zero
done/failed/cancelledrows ever written.After this: A routine's gate self-heals once its linked task reaches a terminal state, so the 2nd, 3rd, ... Nth fire actually launches a run again under both
skip_if_activeandcoalesce_if_active. Lightweight (taskless) fires no longer get stuck claiming a task was created, androutines.last_run_atis finally populated.Who hits this: Anyone relying on a recurring routine — lightweight or heavy — to fire more than once. A heavy routine with a static title was silently exposed to the same wall on its second fire; only a routine whose title happens to interpolate
{{datetime}}(like the built-in "WO conductor tick") was accidentally immune, because that made every fire's fingerprint unique.Scope: standalone bug fix, no sibling PRs.
Not here: the general
TaskMovedevent gap (to_step_nameis never populated by either production publisher) — it's a pre-existing issue that also affects blocker/parent finalization, not just routines. Left the event-drivenSyncRunStatuswiring in place as a harmless no-op that becomes live plumbing if that gap is ever fixed elsewhere, but the actual repair here is a pull-based self-heal that reads the task's terminal status directly and doesn't depend on it.A single hardcoded status check (
GetActiveRunForFingerprintlooking only forstatus = 'task_created', with no age bound and no join to the task's real state) treated a routine's first successful fire as permanently "still in flight." Nothing ever moved a run out oftask_created, so it blocked every later fire with the same dispatch fingerprint.Important Changes
applyConcurrencyPolicy) now self-heals inline: if the active run's linked task has reached a terminal state (read directly from the sharedtaskstable via a newGetTaskTerminalStatus), the run is closed out and the gate clears for this fire — independent of the deadTaskMovedevent path.task_created; an idempotency-conflict dedup now resolves todoneinstead offailed.UpdateRunStatusIfTaskCreated(WHERE status = 'task_created') makes the event-driven path and the gate's own self-heal race-safe against each other.routines.last_run_atis now written on every fire.notBeforeage bound remains the backstop for a crashed dispatch that never reaches a terminal task state.Validation
Backend/Go only — no
apps/web/files changed, so Playwright E2E is not required per the repo's exemption rule.go build ./...,go vet ./...— clean.go test ./internal/office/... ./internal/backendapp/...— all packagesok.gofmt -lon all 10 changed files — clean.golangci-lint run ./... --new-from-rev=<merge-base> --allow-parallel-runners— 0 issues; full-repogolangci-lint run ./... --allow-parallel-runners— 0 issues.make fmt,make typecheck,make lint(backend + web + harness + specs + architecture) — all clean.TestDispatch_HeavyRoutine_GateSelfHealsWhenTaskTerminatesWithoutEvent(bothskip_if_active/coalesce_if_activesubtests) drives the real production call chain (FireManual→dispatchRoutineRun→applyConcurrencyPolicy), marks the linked taskCOMPLETEDvia raw SQL with no call toSyncRunStatus, and asserts the second fire produces atask_createdrun instead of being skipped/coalesced. Verified as a genuine regression test by temporarily neutralizing the fix and confirming both subtests fail on the exact original symptom shape, then reverting.TestDispatch_LightweightRoutine_IdempotencyConflictIsNotFailed,TestGetTaskTerminalStatus,TestUpdateRunStatusIfTaskCreated,TestGetRoutineRunByLinkedTaskID_EmptyTaskIDGuarded.CGO_ENABLED=1 go test -tags fts5 ./...: everyinternal/office/**andinternal/backendapp*packageok. A handful of unrelated packages fail with a macOS tmpdir-path/resource-contention signature that touches none of this diff's files; independently reproduced identically against the merge-base in a scratch worktree, so treated as a declared pre-existing gap, not a blocker.internal/office/repository/sqlite/routines.gochanges are plain parameterized?/RebindSQL — noALTER TABLE, dialect branch, table rebuild, or SQLite-only syntax, and no migration files touched.KANDEV_TEST_POSTGRES_DSNwas unset in this environment — declared gap.pnpm run i18n:ratchet— no-op, no UI files touched.Possible Improvements
Low risk.
computeFingerprintstill keys on the routine title, so two heavy routines with identical static titles remain indistinguishable by dispatch fingerprint — out of scope here since the reported symptom is fixed by the terminal-state self-heal regardless of fingerprint collisions; worth a dedicated look if routine authoring conventions change.Checklist
apps/web/), I have added or updated Playwright e2e tests inapps/web/e2e/and verified them withmake test-e2e.docs/public/**and updated them or noted why no docs change is needed.