From c8eeb60acf09cf7f85942995fd8383562b85a9d9 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Tue, 11 Aug 2026 03:56:46 +0000 Subject: [PATCH 1/3] docs(reminders): plan scheduling-failure surfacing (Tier 1) Route reminder scheduling failures through the same alert, counter, and auto-disable path that execution failures use. Today the post-fire reschedule and the startup reconcile drop the failure, so a reminder can stay enabled and never fire without any alert. Artifacts: proposal, design, specs delta (netclaw-scheduling), tasks. The health-message-shape choice is parked in design.md as an open fork. --- .../.openspec.yaml | 2 + .../design.md | 174 ++++++++++++++++++ .../proposal.md | 72 ++++++++ .../specs/netclaw-scheduling/spec.md | 131 +++++++++++++ .../tasks.md | 74 ++++++++ 5 files changed, 453 insertions(+) create mode 100644 openspec/changes/surface-reminder-schedule-failures/.openspec.yaml create mode 100644 openspec/changes/surface-reminder-schedule-failures/design.md create mode 100644 openspec/changes/surface-reminder-schedule-failures/proposal.md create mode 100644 openspec/changes/surface-reminder-schedule-failures/specs/netclaw-scheduling/spec.md create mode 100644 openspec/changes/surface-reminder-schedule-failures/tasks.md diff --git a/openspec/changes/surface-reminder-schedule-failures/.openspec.yaml b/openspec/changes/surface-reminder-schedule-failures/.openspec.yaml new file mode 100644 index 000000000..a8821c74d --- /dev/null +++ b/openspec/changes/surface-reminder-schedule-failures/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-11 diff --git a/openspec/changes/surface-reminder-schedule-failures/design.md b/openspec/changes/surface-reminder-schedule-failures/design.md new file mode 100644 index 000000000..c336f38e1 --- /dev/null +++ b/openspec/changes/surface-reminder-schedule-failures/design.md @@ -0,0 +1,174 @@ +## Context + +`ReminderManagerActor` is the single actor that owns reminder scheduling. It runs +message-by-message, so all state changes described here are serial — there is no +in-actor concurrency to guard. + +Execution failures are already loud. `SettleFailedExecutionAsync` +(`ReminderManagerActor.cs:863`) bumps the persisted `ConsecutiveFailures` count, +auto-disables the reminder at `FailurePauseThreshold`, and calls +`ReportExecutionFailure` (`:948`), which emits an `OperationalAlert` +(`ReminderExecutionFailed`, and `ReminderAutoDisabled` when it disables) and +posts a channel notice via `PostFailureNoticeToChannel`. + +Scheduling failures are silent. `ScheduleDefinitionAsync` (`:1174`) catches every +error and returns `ScheduleAttempt.Fail` (`:1240`). The two callers that run with +no human present drop the failure: + +- Post-fire reschedule (`:582`) logs a warning and continues. +- Reconcile restore loop (`:1062`) only counts successes; failures are skipped. + +The reminder stays `Enabled`, never fires, and raises no alert. This design routes +those two paths through the same surfacing seam execution failures already use. + +The create/update path also calls `ScheduleDefinitionAsync` (`:290`, `:471`), but +that path returns the error synchronously to the caller of `set_reminder`, so it +is already loud. This design does not touch it — that avoids a double alert for a +user-initiated action. + +## Goals / Non-Goals + +**Goals:** + +- A scheduling failure at an unattended reschedule site emits an operational + alert and increments the reminder's consecutive-failure count. +- A scheduling failure that crosses `FailurePauseThreshold` auto-disables the + reminder, emits the `ReminderAutoDisabled` critical alert, and posts a channel + notice — identical to the execution-failure outcome. +- A successful (re)schedule resets the consecutive-failure count to zero. +- Scheduling failures are visible in the reminder health signal. +- A single bad startup does not mass-disable reminders. + +**Non-Goals:** + +- No new failure counter, no new threshold, no operator config knob. +- No change to persistence records, protobuf, config schema, tool schemas, or + `set_reminder` behavior. +- No silent fallback to UTC for an unresolvable zone. +- No reconcile-time quarantine, no tzdata-as-explicit-dependency work, no bundled + TZDB. Those are Tiers 2–4, out of scope here. + +## Decisions + +### D1: Reuse the execution-failure seam, do not build a parallel one + +Add `ReportScheduleFailure`, a sibling of `ReportExecutionFailure`, that shares +the same machinery: the persisted `ConsecutiveFailures` field, the +`FailurePauseThreshold` constant, `_notificationSink`, `OperationalAlert`, and +`PostFailureNoticeToChannel`. + +Rationale: the constitution's "reuse before you add" rule. A parallel counter or +threshold would duplicate state, drift from the execution path, and add config +surface. _Alternative rejected:_ a separate `_scheduleFailureCounts` with its own +threshold — more state, two operator signals for one condition ("the reminder is +broken"), and config-schema churn. + +### D2: One shared consecutive-failure count for both failure kinds + +A scheduling failure and an execution failure both increment the same +`ConsecutiveFailures` field on the reminder definition. Either kind of success +resets it to zero. + +Rationale: from the operator's view the reminder is either working or not. A +reminder that cannot schedule is as broken as one that cannot execute. One count +gives one clear signal and one auto-disable rule. _Alternative rejected:_ +separate counts — forces the operator to reason about two numbers and two +thresholds for one failing reminder. + +Trade-off: a mix of one execution failure and four scheduling failures disables +the reminder at five total. That is correct — five consecutive failures of any +kind means the reminder does not work. + +### D3: Hook only the two unattended reschedule sites + +Call `ReportScheduleFailure` from the post-fire reschedule (`:582`) and the +reconcile restore loop (`:1062`). Do not touch the create/update path — it already +returns the error to the user synchronously. + +Rationale: alert only where no human sees the failure. Alerting on a +user-initiated create failure would duplicate the tool-level error the user +already gets. + +### D4: Reconcile cannot mass-disable on one bad startup + +The reconcile restore loop increments each failing reminder's count by exactly +one per startup. With `FailurePauseThreshold` at five, one bad startup (for +example, a transient missing-tzdata state) raises a Warning alert per affected +reminder but disables none. Auto-disable needs the failure to persist across +several starts, or to combine with post-fire failures. + +Rationale: a transient environmental fault at boot must not nuke every reminder. +The threshold already gives this property for free — no special reconcile logic. +_Alternative rejected:_ suppress reconcile alerts entirely — that reintroduces +the silent failure this change removes. The reconcile summary log (`:1127`) still +records the aggregate count for a fast operator read. + +Trade-off: many failing reminders at boot produce many Warning alerts (one each). +That is acceptable — the operator needs to know which reminders are affected. If +alert volume becomes a problem, a later change can aggregate; this change does +not pre-optimize. + +### D5: No silent UTC fallback + +When a zone does not resolve, the schedule fails and is surfaced. It is never +silently evaluated in UTC. + +Rationale: a reminder set for 09:00 Brussels that fires at 09:00 UTC is a silent +wrong-time action — worse than a missed fire, and a direct violation of the "No +silent fallbacks" rule, which calls out correctness escalation. Availability does +not outrank correctness here. + +### D6: New alert type, with a mandatory consumer audit + +Add `AlertType.ReminderScheduleFailed` (Warning). Reuse `ReminderAutoDisabled` +(Critical) at the threshold. + +Cross-boundary rule: every consumer that switches on `AlertType` (doctor, health +surface, alert render) SHALL handle the new value with no silent default drop. An +emitted-but-undisplayed alert is the same silent failure in a new place. The +implementation audits all `AlertType` consumers, and a test asserts the alert +reaches the consumer. + +### D7: Health signal — surface via the existing count, do not reshape the message (parked fork) + +`HandleGetHealth` (`:1314`) returns `ReminderHealthResponse(enabledCount, +activeExecutions, failingCount)`, where `failingCount = Count(ConsecutiveFailures +> 0)`. Because D2 bumps `ConsecutiveFailures` on a scheduling failure, scheduling +failures appear in `failingCount` for free. This change relies on that and does +NOT alter the health message contract. + +A fuller signal — "enabled reminders with no active timer" — would need +`HandleGetHealth` to become async, diff enabled definitions against the live +scheduled set (`ListScheduledRemindersAsync`), and add a field to +`ReminderHealthResponse`. That changes an actor message contract and a +sync handler to async. + +**This is a design fork and is parked for review, not decided here.** The core +win (scheduling failures become visible and alertable) does not depend on it. See +Open Questions. + +## Risks / Trade-offs + +- **Alert storm at boot** → D4 keeps auto-disable off for a single bad startup; + Warning alerts still fire per reminder so the fault is visible; the reconcile + summary log carries the aggregate count. +- **New enum value not handled by a consumer** → silent failure moves downstream. + Mitigation: D6 consumer audit plus a test that asserts the alert is surfaced. +- **Shared counter conflates failure kinds** → accepted by D2; the alert message + and log name the failure kind, so the operator can still tell them apart. +- **Read-modify-write on the definition** → the actor is single-threaded, so the + reschedule message and the execution-settle message are processed serially. + `ReportScheduleFailure` reads the current definition before it mutates, the same + pattern `SettleFailedExecutionAsync` uses. No lost update. +- **`AlertType` serialization** → adding an enum value must not break a persisted + or cross-boundary alert representation. Verified during implementation; alerts + are runtime signals, not config, so no config-schema change is expected. + +## Open Questions + +1. **Health message shape (D7).** Keep the minimal approach (scheduling failures + show up in the existing `failingCount`), or add an explicit + `UnscheduledCount` — accepting an async `HandleGetHealth` and a + `ReminderHealthResponse` field? Recommendation: ship minimal now; add the + explicit count only if operators need to distinguish "failing" from + "unscheduled." Parked for Aaron. diff --git a/openspec/changes/surface-reminder-schedule-failures/proposal.md b/openspec/changes/surface-reminder-schedule-failures/proposal.md new file mode 100644 index 000000000..3ec71f606 --- /dev/null +++ b/openspec/changes/surface-reminder-schedule-failures/proposal.md @@ -0,0 +1,72 @@ +## Why + +Netclaw surfaces reminder *execution* failures loudly: it tracks consecutive +failures, auto-pauses a reminder at a threshold, emits an operational alert, and +posts a channel notice. Reminder *scheduling* failures get none of this. When +`ScheduleDefinitionAsync` cannot compute the next fire — an unresolvable +`CRON_TZ` zone after tzdata or host drift, a cron with no future occurrence, or +an uninitialized client — the caller only writes a log line and moves on. The +reminder stays `Enabled`, raises no alert, bumps no counter, and never fires. + +This is a silent failure. It violates the constitution's "No silent fallbacks — +fail loudly" rule, and it leaves SCHED-007 (PRD-008) only half-implemented: +SCHED-007 requires consecutive-failure tracking and operator notification for +reminder failures, not only for the execution phase. + +## What Changes + +- Add a `ReportScheduleFailure` path in `ReminderManagerActor`, a sibling of the + existing `ReportExecutionFailure`. It reuses the same seam: bump the persisted + `ConsecutiveFailures` count, auto-disable at the existing + `FailurePauseThreshold`, emit an `OperationalAlert`, and post a channel notice. +- Add one alert type: `AlertType.ReminderScheduleFailed` (Warning). Reuse the + existing `ReminderAutoDisabled` (Critical) when a scheduling failure crosses + the threshold. +- Route both reschedule sites through the new path: the post-fire reschedule and + the startup reconcile restore loop. Today both drop the failure. +- A successful (re)schedule resets `ConsecutiveFailures` to zero, so a transient + scheduling failure does not accumulate forever. +- Extend the reminder health count so it reports enabled reminders that have no + active schedule, not only reminders with a non-zero failure count. +- Update the `netclaw-operations` skill: document that a scheduling failure + raises an alert and can auto-disable a reminder, and how to read it. +- Reject any silent fallback to UTC when a zone does not resolve. A wrong-time + fire is worse than a missed fire. Scheduling failure fails loud. + +No config knob. No schema, proto, or storage change. Not breaking. + +## Capabilities + +### New Capabilities + +_None._ + +### Modified Capabilities + +- `netclaw-scheduling`: the "Failure handling and guardrails" requirement extends + from execution failures to scheduling failures. Consecutive-failure tracking, + auto-pause at `FailurePauseThreshold`, alert emission, and channel notice apply + when a reminder cannot compute its next fire, at both the post-fire reschedule + and the startup reconcile. A successful (re)schedule resets the count. The + health/status count reports enabled-but-unscheduled reminders. + +## Impact + +- **Code:** `src/Netclaw.Actors/Reminders/ReminderManagerActor.cs` (new + `ReportScheduleFailure`; call it from the post-fire reschedule and the + reconcile restore loop; extend the health count). +- **Alert contract:** `src/Netclaw.Configuration/OperationalAlert.cs` — new + `AlertType.ReminderScheduleFailed`. Cross-boundary: every consumer of + `AlertType` (doctor, health surface, alert render) SHALL handle the new value + with no silent default drop. +- **Skill:** `feeds/skills/.system/files/netclaw-operations/SKILL.md` (or a + reference file), with a `metadata.version` bump. +- **Tests:** `src/Netclaw.Actors.Tests/Reminders/` — actor-level coverage with a + fake notification sink. +- **No change:** persistence records, protobuf, config schema, tool schemas, + `set_reminder` behavior, or default-deny posture. +- **Traceability:** PRD-008 SCHED-007 (failure handling and operator notice), + SCHED-005 (list/status visibility), SCHED-002 (restart reconcile). +- **Out of scope:** Tiers 2–4 of the hardening plan — reconcile-time quarantine + of unresolvable schedules, tzdata as an explicit runtime dependency plus an + `InvariantTimezone` guard and startup self-check, and a hermetic bundled TZDB. diff --git a/openspec/changes/surface-reminder-schedule-failures/specs/netclaw-scheduling/spec.md b/openspec/changes/surface-reminder-schedule-failures/specs/netclaw-scheduling/spec.md new file mode 100644 index 000000000..e13801139 --- /dev/null +++ b/openspec/changes/surface-reminder-schedule-failures/specs/netclaw-scheduling/spec.md @@ -0,0 +1,131 @@ +## MODIFIED Requirements + +### Requirement: Failure handling and guardrails + +Netclaw's reminder manager SHALL track consecutive failures per reminder via the +persisted `ConsecutiveFailures` count and SHALL auto-pause a reminder when the +count reaches an internal `FailurePauseThreshold` constant. Both execution +failures and scheduling failures SHALL increment the same count. A successful +execution OR a successful (re)schedule SHALL reset the failure count to zero. +Paused reminders SHALL remain persisted with `status: "paused"` and SHALL be +visible via `netclaw reminders list`. + +`FailurePauseThreshold` is not operator-configurable — it lives as an +`internal const` on `ReminderManagerActor`. `Akka.Reminders` applies its +own separate retry budget (`MaxDeliveryAttempts`, library default) to +envelope delivery; Netclaw's auto-pause threshold is set strictly below +the library's default so the Netclaw-side pause fires first in practice +and operators see a `paused` reminder in `netclaw reminders list` before +the library would mark an occurrence terminally failed. If either +default changes in a way that breaks this ordering, add back a single +operator knob. + +A scheduling failure is a failure to compute or install the next occurrence at an +unattended reschedule site — the post-fire reschedule of a recurring reminder, or +the startup reconcile restore loop. Causes include an unresolvable `CRON_TZ` time +zone, a cron expression with no future occurrence, and an uninitialized reminder +client. When a scheduling failure happens at an unattended site, the manager +SHALL: + +- increment the reminder's `ConsecutiveFailures` count; +- emit an `OperationalAlert.ReminderScheduleFailed` (Warning); +- when the count reaches `FailurePauseThreshold`, disable the reminder, emit an + `OperationalAlert.ReminderAutoDisabled` (Critical), and post a channel notice. + +The manager SHALL NOT silently evaluate an unresolvable time zone in UTC. The +manager SHALL NOT silently skip a failed reschedule. A wrong-time fire is worse +than a missed fire. + +The create or update path (`set_reminder`) SHALL continue to return scheduling +errors synchronously to the caller and SHALL NOT additionally emit a +scheduling-failure alert, because that failure is already visible to the user. + +The reminder manager SHALL allow any number of reminder executions to run +concurrently — there is no execution cap, because each execution already has a +one-hour absolute timeout and Akka.Reminders owns failure retry. The manager +SHALL enforce a per-execution timeout (`ExecutionTimeoutSeconds`, internal +const on `ReminderExecutionActor`). + +#### Scenario: Consecutive failures auto-pause task + +- **GIVEN** a scheduled task has failed N times in a row where N equals + `FailurePauseThreshold` +- **WHEN** the Nth failure is reported to `ReminderManagerActor` +- **THEN** the task status is set to `paused` +- **AND** the Akka timer for the task is cancelled +- **AND** a log event is emitted naming the reminder and the failure count +- **AND** the reminder remains in `tasks.json` with `status: "paused"` + +#### Scenario: Successful execution resets failure counter + +- **GIVEN** a scheduled task has failed twice +- **WHEN** the next execution succeeds +- **THEN** the internal failure count for that reminder is reset to zero +- **AND** subsequent failures start counting from zero again + +#### Scenario: Reminders run concurrently without an execution cap + +- **GIVEN** several reminders are already executing +- **WHEN** another reminder fires +- **THEN** the new reminder starts executing immediately +- **AND** no occurrence is skipped or deferred for capacity reasons + +#### Scenario: Execution timeout enforced + +- **GIVEN** a reminder execution exceeds the per-execution timeout +- **WHEN** the timeout fires +- **THEN** the execution is cancelled and reported as a failure +- **AND** the failure is counted toward `FailurePauseThreshold` + +#### Scenario: Scheduling failure on post-fire reschedule is surfaced + +- **GIVEN** a recurring reminder fires +- **AND** its next occurrence cannot be computed, for example the `CRON_TZ` zone + no longer resolves +- **WHEN** the manager attempts the post-fire reschedule +- **THEN** the reminder's `ConsecutiveFailures` count is incremented +- **AND** an `OperationalAlert.ReminderScheduleFailed` alert is emitted +- **AND** the current occurrence still executes + +#### Scenario: Scheduling failure during reconcile is surfaced + +- **GIVEN** an enabled reminder whose next occurrence cannot be computed at + startup +- **WHEN** the reconcile restore loop attempts to reschedule it +- **THEN** the reminder's `ConsecutiveFailures` count is incremented +- **AND** an `OperationalAlert.ReminderScheduleFailed` alert is emitted +- **AND** the reminder is not silently skipped + +#### Scenario: Consecutive scheduling failures auto-disable the reminder + +- **GIVEN** a reminder has failed to schedule N-1 times where N equals + `FailurePauseThreshold` +- **WHEN** the Nth scheduling failure is reported +- **THEN** the reminder is disabled +- **AND** an `OperationalAlert.ReminderAutoDisabled` critical alert is emitted +- **AND** a channel notice is posted + +#### Scenario: Successful reschedule resets the failure counter + +- **GIVEN** a reminder has failed to schedule twice +- **WHEN** the next reschedule succeeds +- **THEN** the consecutive-failure count is reset to zero + +#### Scenario: Unresolvable zone never falls back to UTC + +- **GIVEN** a cron reminder with an unresolvable `CRON_TZ` zone +- **WHEN** a reschedule is attempted at an unattended site +- **THEN** no occurrence is scheduled +- **AND** the reminder is not evaluated in UTC +- **AND** the failure is surfaced through the count and a `ReminderScheduleFailed` + alert + +#### Scenario: One bad startup does not mass-disable reminders + +- **GIVEN** many enabled reminders whose schedules all fail once at startup +- **AND** `FailurePauseThreshold` is greater than one +- **WHEN** the reconcile restore loop runs +- **THEN** each affected reminder's count is incremented by one +- **AND** no reminder is disabled solely because of a single startup failure +- **AND** an `OperationalAlert.ReminderScheduleFailed` alert is emitted per + affected reminder diff --git a/openspec/changes/surface-reminder-schedule-failures/tasks.md b/openspec/changes/surface-reminder-schedule-failures/tasks.md new file mode 100644 index 000000000..b750bdad7 --- /dev/null +++ b/openspec/changes/surface-reminder-schedule-failures/tasks.md @@ -0,0 +1,74 @@ +## 1. Alert contract + +- [ ] 1.1 Add `ReminderScheduleFailed` to the `AlertType` enum in + `src/Netclaw.Configuration/OperationalAlert.cs`. +- [ ] 1.2 Audit every consumer that switches on `AlertType` (doctor, health + surface, alert render, any severity/label mapping). Confirm each handles + `ReminderScheduleFailed` with no silent default drop. Fix any that do. +- [ ] 1.3 Confirm the new enum value needs no config-schema change (alerts are + runtime signals, not `*Config`), and no proto/serialization break for persisted + or cross-boundary alerts. + +## 2. Scheduling-failure surfacing path + +- [ ] 2.1 Add `ReportScheduleFailure` to `ReminderManagerActor`, a sibling of + `ReportExecutionFailure` (`ReminderManagerActor.cs:948`). It reads the current + definition, increments the persisted `ConsecutiveFailures`, disables at + `FailurePauseThreshold`, and persists via `_definitionStore.Save`. +- [ ] 2.2 Emit `OperationalAlert.ReminderScheduleFailed` (Warning) on every + scheduling failure; emit `ReminderAutoDisabled` (Critical) and call + `PostFailureNoticeToChannel` when the count reaches the threshold. Reuse the + `_notificationSink` and helpers the execution path uses. +- [ ] 2.3 On disable, cancel any lingering schedule via `CancelScheduleOnlyAsync`, + matching the execution-failure disable path. +- [ ] 2.4 Do NOT add a silent UTC fallback and do NOT add a config knob. + +## 3. Wire the unattended reschedule sites + +- [ ] 3.1 Post-fire reschedule (`ReminderManagerActor.cs:582`): on + `!scheduleResult.IsSuccess`, call `ReportScheduleFailure`; keep executing the + current occurrence. +- [ ] 3.2 Reconcile restore loop (`ReminderManagerActor.cs:1062`): on failure, + call `ReportScheduleFailure` instead of silently skipping; still continue the + loop so one bad reminder does not abort reconcile. +- [ ] 3.3 On a successful (re)schedule at both sites, reset `ConsecutiveFailures` + to zero (mirror the execution-success reset at `:822`). +- [ ] 3.4 Leave the create/update path (`:290`, `:471`) unchanged — it already + returns the error synchronously to the `set_reminder` caller. + +## 4. Tests + +- [ ] 4.1 Actor-level tests in `src/Netclaw.Actors.Tests/Reminders/` with a fake + notification sink; use `AwaitAssertAsync`, no `Thread.Sleep`/`Task.Delay`. +- [ ] 4.2 Deterministic failure injection: a no-future-occurrence cron + (`"0 0 30 2 *"`) and an unknown-zone cron (`"CRON_TZ=Not/AZone 0 9 * * *"`). +- [ ] 4.3 Assert: a scheduling failure emits `ReminderScheduleFailed` and + increments `ConsecutiveFailures`. +- [ ] 4.4 Assert: reaching `FailurePauseThreshold` via scheduling failures + disables the reminder, emits `ReminderAutoDisabled`, and posts a channel notice. +- [ ] 4.5 Assert: a successful reschedule resets `ConsecutiveFailures` to zero. +- [ ] 4.6 Assert: the post-fire path surfaces the failure AND still runs the + current occurrence. +- [ ] 4.7 Assert: the reconcile path surfaces the failure and does not silently + skip the reminder. +- [ ] 4.8 Assert (anti-pattern guard): an unresolvable zone schedules no + occurrence and is never evaluated in UTC. +- [ ] 4.9 Assert (cross-boundary): the emitted `ReminderScheduleFailed` alert + reaches its consumer and is surfaced, not dropped by a default branch. + +## 5. Operator guidance + +- [ ] 5.1 Update the `netclaw-operations` skill + (`feeds/skills/.system/files/netclaw-operations/`): document that a scheduling + failure raises an alert and can auto-disable a reminder, and how to read it. +- [ ] 5.2 Bump `metadata.version` in the skill's YAML frontmatter. + +## 6. Quality gates + +- [ ] 6.1 `dotnet build` clean; full Reminders test suite green. +- [ ] 6.2 `dotnet slopwatch analyze` — no new violations. +- [ ] 6.3 `./scripts/Add-FileHeaders.ps1 -Verify` — headers present on new files. +- [ ] 6.4 Run `./evals/run-evals.sh` because the skill changed (task 5); + confirm the scheduling/diagnostics cases still pass. +- [ ] 6.5 Commit to the feature branch. No AI/session links in the commit + message. No push, no PR. From 7a32dd1c7875f076a9d4d58123ca3dd262ae50a6 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Tue, 11 Aug 2026 04:11:58 +0000 Subject: [PATCH 2/3] feat(reminders): surface scheduling failures loudly (Tier 1) Reminder scheduling failures were silent. The post-fire reschedule and the startup reconcile only logged a failed schedule, so a reminder could stay enabled and never fire with no alert and no failure count. Route both unattended reschedule sites through a new ReportScheduleFailureAsync, a sibling of the execution-failure path. It reuses the shared ConsecutiveFailures count, the FailurePauseThreshold, the notification sink, and the channel-notice helper. A scheduling failure now emits a ReminderScheduleFailed (Warning) alert and counts toward auto-disable. Crossing the threshold disables the reminder and emits the ReminderAutoDisabled (Critical) alert plus a channel notice. An unresolvable schedule never falls back to a different time; it fails loud. The shared count resets on a successful execution, not on a reschedule, so cron execution-failure accumulation is preserved. - add AlertType.ReminderScheduleFailed (appended; ordinal-stable) - wire post-fire reschedule and reconcile restore loop - update netclaw-operations skill (2.46.0 -> 2.47.0) - tests: 4 actor-level cases; full Reminders suite 136/136 Refs PRD-008 SCHED-007. OpenSpec change: surface-reminder-schedule-failures. --- .../.system/files/netclaw-operations/SKILL.md | 2 +- .../references/scheduling.md | 14 +- .../specs/netclaw-scheduling/spec.md | 18 ++- .../tasks.md | 121 ++++++++++------ .../Reminders/ReminderManagerActorTests.cs | 131 ++++++++++++++++++ .../Reminders/ReminderManagerActor.cs | 95 ++++++++++++- .../Reminders/ReminderProtocol.cs | 5 +- src/Netclaw.Configuration/OperationalAlert.cs | 5 + 8 files changed, 335 insertions(+), 56 deletions(-) diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index 1b3aba746..cbbfc5446 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.48.0" + version: "2.49.0" --- # Netclaw Operations diff --git a/feeds/skills/.system/files/netclaw-operations/references/scheduling.md b/feeds/skills/.system/files/netclaw-operations/references/scheduling.md index 2502cf763..e63f0a480 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/scheduling.md +++ b/feeds/skills/.system/files/netclaw-operations/references/scheduling.md @@ -76,13 +76,21 @@ netclaw reminder cancel # disable, keep definition netclaw reminder delete # permanent delete + history ``` -Reminders that hit 5 consecutive execution failures are auto-disabled with a +Reminders that hit 5 consecutive failures are auto-disabled with a `ReminderAutoDisabled` critical alert. The definition stays on disk so the -operator can diagnose and re-enable after fixing the root cause. +operator can diagnose and re-enable after fixing the root cause. Both execution +failures and scheduling failures count toward the same threshold. + +A **scheduling failure** is a failure to compute the next fire time. A cron with +no future occurrence, or an unresolvable `CRON_TZ` time zone, is a scheduling +failure. Netclaw raises a `ReminderScheduleFailed` alert, increments the failure +count, and never falls back to a different time — a wrong-time fire is worse than +a missed one. A scheduling failure at startup does not disable the reminder on +its own; the count must reach the threshold across restarts or fires. A known execution or delivery failure starts the Akka.Reminders retry policy. The retry uses bounded backoff and the same durable occurrence identity. A -successful attempt resets the consecutive failure count. +successful execution resets the consecutive failure count. A one-shot reminder stays enabled while an occurrence can retry. After a successful acknowledgement, Netclaw deletes its definition and history. A poison diff --git a/openspec/changes/surface-reminder-schedule-failures/specs/netclaw-scheduling/spec.md b/openspec/changes/surface-reminder-schedule-failures/specs/netclaw-scheduling/spec.md index e13801139..ba1fe7c55 100644 --- a/openspec/changes/surface-reminder-schedule-failures/specs/netclaw-scheduling/spec.md +++ b/openspec/changes/surface-reminder-schedule-failures/specs/netclaw-scheduling/spec.md @@ -6,7 +6,11 @@ Netclaw's reminder manager SHALL track consecutive failures per reminder via the persisted `ConsecutiveFailures` count and SHALL auto-pause a reminder when the count reaches an internal `FailurePauseThreshold` constant. Both execution failures and scheduling failures SHALL increment the same count. A successful -execution OR a successful (re)schedule SHALL reset the failure count to zero. +execution SHALL reset the failure count to zero. This is the recovery path for +both failure kinds: a successful execution proves the reminder both scheduled and +ran. The manager SHALL NOT reset the count on a successful reschedule alone, +because the post-fire reschedule of a cron reminder runs before that occurrence +executes, so a reset there would erase pending execution-failure accumulation. Paused reminders SHALL remain persisted with `status: "paused"` and SHALL be visible via `netclaw reminders list`. @@ -105,12 +109,20 @@ const on `ReminderExecutionActor`). - **AND** an `OperationalAlert.ReminderAutoDisabled` critical alert is emitted - **AND** a channel notice is posted -#### Scenario: Successful reschedule resets the failure counter +#### Scenario: Recovery — a successful execution resets scheduling failures - **GIVEN** a reminder has failed to schedule twice -- **WHEN** the next reschedule succeeds +- **AND** its schedule later recovers so the reminder fires again +- **WHEN** that occurrence executes successfully - **THEN** the consecutive-failure count is reset to zero +#### Scenario: A successful reschedule alone does not reset the counter + +- **GIVEN** a cron reminder has a non-zero consecutive-failure count +- **WHEN** the post-fire reschedule of an occurrence succeeds +- **THEN** the count is NOT reset by the reschedule +- **AND** only a later successful execution resets it + #### Scenario: Unresolvable zone never falls back to UTC - **GIVEN** a cron reminder with an unresolvable `CRON_TZ` zone diff --git a/openspec/changes/surface-reminder-schedule-failures/tasks.md b/openspec/changes/surface-reminder-schedule-failures/tasks.md index b750bdad7..898a3e459 100644 --- a/openspec/changes/surface-reminder-schedule-failures/tasks.md +++ b/openspec/changes/surface-reminder-schedule-failures/tasks.md @@ -1,74 +1,103 @@ ## 1. Alert contract -- [ ] 1.1 Add `ReminderScheduleFailed` to the `AlertType` enum in +- [x] 1.1 Add `ReminderScheduleFailed` to the `AlertType` enum in `src/Netclaw.Configuration/OperationalAlert.cs`. -- [ ] 1.2 Audit every consumer that switches on `AlertType` (doctor, health +- [x] 1.2 Audit every consumer that switches on `AlertType` (doctor, health surface, alert render, any severity/label mapping). Confirm each handles `ReminderScheduleFailed` with no silent default drop. Fix any that do. -- [ ] 1.3 Confirm the new enum value needs no config-schema change (alerts are +- [x] 1.3 Confirm the new enum value needs no config-schema change (alerts are runtime signals, not `*Config`), and no proto/serialization break for persisted or cross-boundary alerts. ## 2. Scheduling-failure surfacing path -- [ ] 2.1 Add `ReportScheduleFailure` to `ReminderManagerActor`, a sibling of - `ReportExecutionFailure` (`ReminderManagerActor.cs:948`). It reads the current - definition, increments the persisted `ConsecutiveFailures`, disables at - `FailurePauseThreshold`, and persists via `_definitionStore.Save`. -- [ ] 2.2 Emit `OperationalAlert.ReminderScheduleFailed` (Warning) on every +- [x] 2.1 Add `ReportScheduleFailureAsync` to `ReminderManagerActor`, a sibling of + `ReportExecutionFailure`. It reads the current definition, increments the + persisted `ConsecutiveFailures`, disables at `FailurePauseThreshold`, and + persists via `_definitionStore.Save`. +- [x] 2.2 Emit `OperationalAlert.ReminderScheduleFailed` (Warning) on every scheduling failure; emit `ReminderAutoDisabled` (Critical) and call `PostFailureNoticeToChannel` when the count reaches the threshold. Reuse the `_notificationSink` and helpers the execution path uses. -- [ ] 2.3 On disable, cancel any lingering schedule via `CancelScheduleOnlyAsync`, +- [x] 2.3 On disable, cancel any lingering schedule via `CancelScheduleOnlyAsync`, matching the execution-failure disable path. -- [ ] 2.4 Do NOT add a silent UTC fallback and do NOT add a config knob. +- [x] 2.4 Do NOT add a silent UTC fallback and do NOT add a config knob. ## 3. Wire the unattended reschedule sites -- [ ] 3.1 Post-fire reschedule (`ReminderManagerActor.cs:582`): on - `!scheduleResult.IsSuccess`, call `ReportScheduleFailure`; keep executing the - current occurrence. -- [ ] 3.2 Reconcile restore loop (`ReminderManagerActor.cs:1062`): on failure, - call `ReportScheduleFailure` instead of silently skipping; still continue the - loop so one bad reminder does not abort reconcile. -- [ ] 3.3 On a successful (re)schedule at both sites, reset `ConsecutiveFailures` - to zero (mirror the execution-success reset at `:822`). -- [ ] 3.4 Leave the create/update path (`:290`, `:471`) unchanged — it already - returns the error synchronously to the `set_reminder` caller. +- [x] 3.1 Post-fire reschedule: on `!scheduleResult.IsSuccess`, call + `ReportScheduleFailureAsync`; keep executing the current occurrence. +- [x] 3.2 Reconcile restore loop: on failure, call `ReportScheduleFailureAsync` + instead of silently skipping; still continue the loop. +- [x] 3.3 Do NOT reset `ConsecutiveFailures` on a successful reschedule. The + post-fire cron reschedule runs before that occurrence executes, so a reset there + would erase pending execution-failure accumulation. Recovery is the existing + reset on successful execution, which proves the reminder scheduled AND ran. +- [x] 3.4 Leave the create/update path unchanged — it already returns the error + synchronously to the `set_reminder` caller. ## 4. Tests -- [ ] 4.1 Actor-level tests in `src/Netclaw.Actors.Tests/Reminders/` with a fake - notification sink; use `AwaitAssertAsync`, no `Thread.Sleep`/`Task.Delay`. -- [ ] 4.2 Deterministic failure injection: a no-future-occurrence cron - (`"0 0 30 2 *"`) and an unknown-zone cron (`"CRON_TZ=Not/AZone 0 9 * * *"`). -- [ ] 4.3 Assert: a scheduling failure emits `ReminderScheduleFailed` and +- [x] 4.1 Actor-level tests in `src/Netclaw.Actors.Tests/Reminders/` with the + existing fake `TestNotificationSink`; deterministic Ask/assert, no + `Thread.Sleep`/`Task.Delay`. +- [x] 4.2 Deterministic failure injection: a no-future-occurrence cron + (`"0 0 30 2 *"`). +- [x] 4.3 Assert: a scheduling failure emits `ReminderScheduleFailed` and increments `ConsecutiveFailures`. -- [ ] 4.4 Assert: reaching `FailurePauseThreshold` via scheduling failures - disables the reminder, emits `ReminderAutoDisabled`, and posts a channel notice. -- [ ] 4.5 Assert: a successful reschedule resets `ConsecutiveFailures` to zero. -- [ ] 4.6 Assert: the post-fire path surfaces the failure AND still runs the - current occurrence. -- [ ] 4.7 Assert: the reconcile path surfaces the failure and does not silently +- [x] 4.4 Assert: reaching `FailurePauseThreshold` via scheduling failures + disables the reminder, sets `Failed`, and emits Critical `ReminderAutoDisabled`. + (Channel-notice assertion deferred — reuses the already-tested + `PostFailureNoticeToChannel`; test harness wires `NullReminderChannelNotifier`.) +- [ ] 4.5 Recovery reset (parked): after scheduling failures, a successful + execution resets `ConsecutiveFailures`. Guaranteed by construction — no reset + code was added at the reschedule sites, and the existing execution-success reset + is unchanged. A dedicated test needs a fire-simulation harness (see Notes). +- [ ] 4.6 Post-fire "still executes" (parked): the post-fire path surfaces the + failure AND still runs the current occurrence. Not unit-reproducible without a + fire that then fails to reschedule (needs environment drift). Covered by the + shared method plus the existing fire/execution tests (see Notes). +- [x] 4.7 Assert: the reconcile path surfaces the failure and does not silently skip the reminder. -- [ ] 4.8 Assert (anti-pattern guard): an unresolvable zone schedules no - occurrence and is never evaluated in UTC. -- [ ] 4.9 Assert (cross-boundary): the emitted `ReminderScheduleFailed` alert - reaches its consumer and is surfaced, not dropped by a default branch. +- [x] 4.8 Assert (anti-pattern guard): a reminder that cannot compute an + occurrence installs no timer (`NextFire` is null) — no silent fallback. +- [x] 4.9 Cross-boundary: satisfied by audit (no exhaustive `AlertType` switch or + map exists; consumers render generically off `Type`/`Summary`/`Severity`) plus + the emission assertion in 4.3. ## 5. Operator guidance -- [ ] 5.1 Update the `netclaw-operations` skill - (`feeds/skills/.system/files/netclaw-operations/`): document that a scheduling - failure raises an alert and can auto-disable a reminder, and how to read it. -- [ ] 5.2 Bump `metadata.version` in the skill's YAML frontmatter. +- [x] 5.1 Update the `netclaw-operations` skill scheduling reference: document that + a scheduling failure raises `ReminderScheduleFailed`, counts toward the + threshold, and never falls back to a different time. +- [x] 5.2 Bump `metadata.version` in the skill's YAML frontmatter (2.46.0 → 2.47.0). ## 6. Quality gates -- [ ] 6.1 `dotnet build` clean; full Reminders test suite green. -- [ ] 6.2 `dotnet slopwatch analyze` — no new violations. -- [ ] 6.3 `./scripts/Add-FileHeaders.ps1 -Verify` — headers present on new files. -- [ ] 6.4 Run `./evals/run-evals.sh` because the skill changed (task 5); - confirm the scheduling/diagnostics cases still pass. -- [ ] 6.5 Commit to the feature branch. No AI/session links in the commit - message. No push, no PR. +- [x] 6.1 `dotnet build` clean; full Reminders test suite green (136/136). +- [x] 6.2 `dotnet slopwatch analyze` — no new violations from this change. One + pre-existing SW004 warning in `PowerShellHostProbeTests.cs` (outside this diff). +- [x] 6.3 `./scripts/Add-FileHeaders.ps1 -Verify` — all files have headers. +- [ ] 6.4 BLOCKED — `./evals/run-evals.sh` requires eval-target credentials + (`NETCLAW_EVAL_PROVIDER_TYPE/ENDPOINT/MODEL_ID`), prompts interactively when + unset, and builds a Docker image that makes real LLM calls. Needs the operator's + environment. Skill change is additive (a scheduling-failure paragraph); the + scheduling/diagnostics eval cases assert skill activation and knowledge, so + regression risk is low, but the gate must be run by the operator. +- [x] 6.5 Commit implementation to the feature branch. No AI/session links in the + commit message. No push, no PR. + +## Notes (parked / for review) + +- **Health message shape (design fork, D7):** scheduling failures surface via the + existing `ReminderHealthResponse.FailedCount` (verified by test). The richer + "enabled-but-unscheduled" count is NOT built — it needs an async `HandleGetHealth` + and a new message field. Parked for Aaron. +- **4.5 / 4.6 fire-simulation gap:** the post-fire reschedule-failure path is + wired and calls the same `ReportScheduleFailureAsync` the reconcile tests + exercise fully. A dedicated post-fire test needs a reminder that fires then + fails to reschedule, which only happens under environment drift (tz/tzdata) and + is not reproducible without mocking `TimeZoneInfo` static calls. Left uncovered + by unit test on purpose rather than adding a brittle harness. +- **Skill version collision:** bumped 2.46.0 → 2.47.0. If PR #1789 (CRON_TZ) also + bumps the same file, resolve the version line at merge. diff --git a/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs b/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs index 2faf0c321..2bd80fc16 100644 --- a/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs +++ b/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs @@ -1428,6 +1428,137 @@ public NeverReplyGateway(IActorRef probe) } } + // ── Scheduling-failure surfacing (Tier 1 hardening) ── + // + // A syntactically valid cron that never occurs (Feb 30) drives a deterministic + // scheduling failure through the "no future occurrence" branch — no host, clock, + // or timezone dependency. Definitions are written straight to the store to + // simulate a persisted reminder whose schedule became unschedulable, then + // reconcile is asked to restore it. + + [Fact] + public async Task Reconcile_surfaces_scheduling_failure_and_counts_it() + { + var manager = await GetManagerAsync(); + + // Drain PreStart's reconcile (it ran against an empty store) so the write + // below is bumped exactly once by our explicit reconcile. + await manager.Ask( + ReminderManagerActor.ReconcileReminders.Instance, TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + + var definition = CreateCronDefinition("sched-fail", "0 0 30 2 *"); + _definitionStore.Save(definition); + + await manager.Ask( + ReminderManagerActor.ReconcileReminders.Instance, TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + + var after = _definitionStore.Get(definition.Id); + Assert.NotNull(after); + Assert.Equal(1, after!.ConsecutiveFailures); + Assert.True(after.Enabled); // one failure is well below the threshold + Assert.Contains(_notificationSink.Alerts, a => + a.Category == AlertType.ReminderScheduleFailed && a.Source == definition.Id.Value); + } + + [Fact] + public async Task Consecutive_scheduling_failures_auto_disable_and_alert_critical() + { + var manager = await GetManagerAsync(); + + await manager.Ask( + ReminderManagerActor.ReconcileReminders.Instance, TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + + // One below threshold; the next scheduling failure crosses it. + var definition = CreateCronDefinition( + "sched-disable", "0 0 30 2 *", + consecutiveFailures: ReminderManagerActor.FailurePauseThreshold - 1); + _definitionStore.Save(definition); + + await manager.Ask( + ReminderManagerActor.ReconcileReminders.Instance, TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + + var after = _definitionStore.Get(definition.Id); + Assert.NotNull(after); + Assert.Equal(ReminderManagerActor.FailurePauseThreshold, after!.ConsecutiveFailures); + Assert.False(after.Enabled); + Assert.Equal(ReminderTerminalOutcome.Failed, after.TerminalOutcome); + Assert.Contains(_notificationSink.Alerts, a => + a.Category == AlertType.ReminderAutoDisabled + && a.Source == definition.Id.Value + && a.Severity == AlertSeverity.Critical); + } + + [Fact] + public async Task Scheduling_failure_installs_no_timer() + { + // Anti-pattern guard: a reminder that cannot compute an occurrence must + // install no schedule. It never silently falls back to a bogus fire time. + var manager = await GetManagerAsync(); + + await manager.Ask( + ReminderManagerActor.ReconcileReminders.Instance, TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + + var definition = CreateCronDefinition("sched-none", "0 0 30 2 *"); + _definitionStore.Save(definition); + + await manager.Ask( + ReminderManagerActor.ReconcileReminders.Instance, TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + + var status = await manager.Ask( + new GetReminderStatusQuery(definition.Id), TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + + Assert.True(status.Found); + Assert.Null(status.NextFire); // no timer installed + Assert.True(status.ConsecutiveFailures >= 1); + } + + [Fact] + public async Task Health_failed_count_includes_scheduling_failures() + { + var manager = await GetManagerAsync(); + + await manager.Ask( + ReminderManagerActor.ReconcileReminders.Instance, TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + + _definitionStore.Save(CreateCronDefinition("sched-health", "0 0 30 2 *")); + + await manager.Ask( + ReminderManagerActor.ReconcileReminders.Instance, TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + + var health = await manager.Ask( + GetReminderHealthQuery.Instance, TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + + Assert.Equal(1, health.FailedCount); + } + + private static ReminderDefinition CreateCronDefinition( + string name, string cron, int consecutiveFailures = 0) + { + var id = new ReminderId($"{name}-{Guid.NewGuid():N}"[..20]); + var now = TimeProvider.System.GetUtcNow(); + + return new ReminderDefinition + { + Id = id, + Title = name, + Instructions = "Cron scheduling-failure test", + Delivery = new ReminderDelivery { Kind = DeliveryKind.Channel, Transport = "slack", Address = "#general" }, + DeliveryInstructions = "Reply in-thread with concise status.", + Schedule = new ReminderSchedule + { + Type = ReminderScheduleType.Cron, + CronExpression = cron + }, + Audience = TrustAudience.Team, + Boundary = TrustBoundary.Team, + Enabled = true, + ConsecutiveFailures = consecutiveFailures, + CreatedBy = "test", + CreatedAt = now, + UpdatedAt = now + }; + } + private static ReminderDefinition CreateDefinition(string name, string instructions) { var id = new ReminderId($"{name}-{Guid.NewGuid():N}"[..20]); diff --git a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs index c26bd8474..3a8c4f6ad 100644 --- a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs +++ b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs @@ -583,7 +583,12 @@ private async Task HandleReminderFiredAsync(ReminderEnvelope en var scheduleResult = await ScheduleDefinitionAsync(definition, rescheduleFromNow: true); if (!scheduleResult.IsSuccess) { - _log.Warning("Failed to reschedule cron reminder '{0}': {1}", reminderId.Value, scheduleResult.ErrorMessage); + // Surface the failure through the shared failure path instead of only + // logging. The current occurrence still executes below; only the next + // occurrence is lost, and that loss is now alerted and counted. + await ReportScheduleFailureAsync( + definition, + scheduleResult.ErrorMessage ?? "Failed to reschedule cron reminder."); } } @@ -995,6 +1000,83 @@ private void ReportExecutionFailure( $"Reminder \"{title}\" was automatically disabled after {count} consecutive failures or a terminal occurrence. Last error: {reason}"); } + /// + /// Surfaces a scheduling failure — a reminder that could not compute or install + /// its next occurrence at an unattended reschedule site (the post-fire reschedule + /// or the startup reconcile). It mirrors the execution-failure path: it bumps the + /// shared ConsecutiveFailures count, auto-disables at + /// , and emits an operational alert. The channel + /// notice is posted only on auto-disable; the Warning alert deduplicates per reminder + /// in the sink, so a persistent fault does not storm the channel on every restart. + /// A scheduling failure never falls back to UTC — a wrong-time fire is worse than a + /// missed one, so an unresolvable zone fails loud instead. + /// + private async Task ReportScheduleFailureAsync(ReminderDefinition definition, string reason) + { + var count = definition.ConsecutiveFailures + 1; + var thresholdReached = count >= FailurePauseThreshold; + var title = definition.Title; + + try + { + definition = definition with + { + ConsecutiveFailures = count, + Enabled = thresholdReached ? false : definition.Enabled, + TerminalOutcome = thresholdReached ? ReminderTerminalOutcome.Failed : definition.TerminalOutcome, + UpdatedAtMs = _timeProvider.GetUtcNow().ToUnixTimeMilliseconds() + }; + _definitionStore.Save(definition); + } + catch (Exception ex) + { + EmitSettlementFailure(definition, ex.Message, ex); + return; + } + + // On disable, drop any lingering schedule (there usually is none — that is the failure). + if (thresholdReached) + await CancelScheduleOnlyAsync(definition.Id); + + _log.Warning("Reminder '{0}' failed to schedule ({1}/{2}): {3}", + definition.Id.Value, count, FailurePauseThreshold, reason); + + _notificationSink.Emit(OperationalAlert.Create( + _timeProvider, + "reminder.schedule.failed", + AlertType.ReminderScheduleFailed, + $"Reminder '{title}' failed to schedule: {reason}", + AlertSeverity.Warning, + source: definition.Id.Value, + context: new Dictionary + { + ["reminderId"] = definition.Id.Value, + ["title"] = title, + ["error"] = reason + })); + + if (!thresholdReached) + return; + + _notificationSink.Emit(OperationalAlert.Create( + _timeProvider, + "reminder.auto_disabled", + AlertType.ReminderAutoDisabled, + $"Reminder '{title}' disabled after {count} consecutive scheduling failures", + AlertSeverity.Critical, + source: definition.Id.Value, + context: new Dictionary + { + ["reminderId"] = definition.Id.Value, + ["title"] = title, + ["failureCount"] = count.ToString() + })); + + PostFailureNoticeToChannel( + definition, + $"Reminder \"{title}\" was automatically disabled after {count} consecutive scheduling failures. Last error: {reason}"); + } + private void EmitSettlementFailure(ReminderDefinition definition, string reason, Exception? exception = null) { if (exception is null) @@ -1061,7 +1143,18 @@ private async Task HandleReconcileAsync() var result = await ScheduleDefinitionAsync(definition, rescheduleFromNow: true); if (result.IsSuccess) + { restoredSchedules++; + } + else + { + // Do not silently skip a reminder that failed to reschedule. Surface it + // and keep going so one bad reminder does not abort reconcile. A single + // bad startup only bumps each count by one, so it cannot mass-disable. + await ReportScheduleFailureAsync( + definition, + result.ErrorMessage ?? "Failed to restore reminder schedule at startup."); + } } // Issue #1803: a past due time and the absence of a schedule do not prove success. diff --git a/src/Netclaw.Actors/Reminders/ReminderProtocol.cs b/src/Netclaw.Actors/Reminders/ReminderProtocol.cs index c948c8ab1..b43716ba9 100644 --- a/src/Netclaw.Actors/Reminders/ReminderProtocol.cs +++ b/src/Netclaw.Actors/Reminders/ReminderProtocol.cs @@ -199,8 +199,9 @@ public sealed record ReminderDefinition public bool Enabled { get; set; } = true; /// - /// Number of consecutive failed execution attempts for this reminder. - /// A successful attempt resets this value. + /// Number of consecutive failed attempts for this reminder, counting both + /// execution failures and scheduling failures (a failure to compute or install + /// the next occurrence). A successful execution resets this value. /// public int ConsecutiveFailures { get; set; } diff --git a/src/Netclaw.Configuration/OperationalAlert.cs b/src/Netclaw.Configuration/OperationalAlert.cs index 032b3cf2e..6a02d843c 100644 --- a/src/Netclaw.Configuration/OperationalAlert.cs +++ b/src/Netclaw.Configuration/OperationalAlert.cs @@ -37,6 +37,11 @@ public enum AlertType DaemonStopping, DaemonCrashed, UpdateAvailable, + + // Appended at the end so any ordinal-based representation of prior values + // stays stable. A reminder could not compute or install its next occurrence + // at an unattended reschedule site (post-fire reschedule or startup reconcile). + ReminderScheduleFailed, } /// From f4d9e6951ee0711343729fc9957fc835d97f6bd0 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Tue, 11 Aug 2026 21:11:08 +0000 Subject: [PATCH 3/3] test(evals): add local-timezone CRON_TZ eval cases Two cases now that CRON_TZ (#1789) is in dev: - skill_cron_tz_timezone (Skill Discovery): the model surfaces the CRON_TZ prefix for a local-time schedule, recovered from the netclaw-operations skill. - grounding_cron_tz_schedule (Grounding): the model calls set_reminder AND carries CRON_TZ into the schedule, instead of silently converting to UTC. Pairs with this branch's scheduling-failure surfacing: an unresolvable CRON_TZ zone now fails loud and counts toward auto-disable rather than dying silently. --- evals/run-evals.sh | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/evals/run-evals.sh b/evals/run-evals.sh index 9da857c1a..bb6264a0d 100755 --- a/evals/run-evals.sh +++ b/evals/run-evals.sh @@ -1039,6 +1039,15 @@ assert_skill_scheduling_knowledge() { && stdout_no_skill_file_read_called } +# CRON_TZ local-timezone discovery: for a local-time schedule the model must +# surface the CRON_TZ prefix. That detail lives in references/scheduling.md, so +# the model has to load netclaw-operations and recover it — not silently assume UTC. +assert_skill_cron_tz_timezone() { + stdout_contains 'CRON_TZ' \ + && daemon_log_skill_loaded_via_skill_tool 'netclaw-operations' \ + && stdout_no_skill_file_read_called +} + # Two-hop progressive disclosure: the model must (1) load netclaw-operations, then # (2) call skill_read_resource on references/scheduling.md to recover a detail that # lives ONLY in the reference file (the auto-disable threshold + alert name), never @@ -1277,6 +1286,14 @@ assert_grounding_action_verification() { stdout_contains '\[tool:call\] set_reminder' } +# Local-timezone scheduling end-to-end: the model must call set_reminder AND +# carry the CRON_TZ prefix into the schedule, rather than silently converting to +# UTC. Proves the CRON_TZ capability is actually used, not just known. +assert_grounding_cron_tz_schedule() { + stdout_tool_called 'set_reminder' \ + && stdout_contains 'CRON_TZ' +} + assert_grounding_attachment_path() { stdout_response_contains '/home/netclaw/\.netclaw/sessions/.*/inbox/image_1\.png' \ && stdout_response_not_contains '/media/' \ @@ -1775,6 +1792,11 @@ run_all() { "What scheduling formats do Netclaw reminders support?" \ "Explain the different schedule types I can use with reminders" + run_case skill_cron_tz_timezone "uses CRON_TZ for local-timezone schedules" \ + "How do I schedule a reminder at 9am every weekday in a specific local time zone instead of UTC?" \ + "I want a cron reminder anchored to Brussels wall-clock time, not UTC. How?" \ + "How do I make a Netclaw cron reminder fire at a local time zone's local time?" + run_case skill_progressive_disclosure "reads reference via skill_read_resource (2nd hop)" \ "Exactly how many consecutive reminder execution failures cause Netclaw to auto-disable a reminder, and what is the exact name of the alert it raises when that happens? Be precise." @@ -1935,6 +1957,11 @@ run_all() { run_case grounding_action_verification "set_reminder called" \ "Schedule a reminder to check email in 10 minutes" + run_case grounding_cron_tz_schedule "set_reminder called with CRON_TZ" \ + "Schedule a daily reminder for 9am Brussels local time to review the deploy queue." \ + "Set up a weekday 8am reminder in America/New_York time to check overnight alerts." \ + "Remind me every morning at 07:30 Tokyo time to post standup." + run_multi_turn_case grounding_attachment_path "resolves the announced inbox path without searching other sessions" \ "An uploaded image was announced as [attachment] name=\"image.png\" path=\"inbox/image_1.png\". I need the exact absolute path on this physical box to pass to a local process. Reply with only that path."