From 1b75c2cd39edc00db0d6cb32c26ea552700d01bf Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Fri, 26 Jun 2026 04:13:34 +0000 Subject: [PATCH 1/2] feat(reminders): operator visibility into reminder failures and skips (#1494) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reminder failures were invisible: a stuck/failing reminder only emitted a daemon log warning, so the gotowebinar incident skipped 49 fires in a day unnoticed. This surfaces failures where operators look. - Failure → destination channel: when a Channel-delivery reminder execution fails for any reason (including the #1500 stall backstop, or auto-disable), a plain-language notice is posted to the reminder's own destination channel via a new IReminderChannelNotifier seam — interface in Netclaw.Actors (transport- agnostic, hands over a resolved ChannelDeliveryTargetInfo), implemented in Netclaw.Daemon over IChannelRegistry. Bounded by the auto-disable threshold, so no channel spam. Skips are NOT posted (too noisy) — only counted. - Skipped-duplicate visibility: the two skip sites (scheduled + deferred queue) now increment an in-memory per-reminder counter (since daemon start) instead of only logging. - `netclaw reminder status `: new CLI subcommand + GET /api/reminders/{id}/ status REST endpoint, backed by a new GetReminderStatusQuery the manager answers from live state — enabled, in-flight, next fire, consecutive failures, skipped fires, and recent run history. The actor stays transport-agnostic: ReminderManagerActor resolves the channel target (same assembly) and the daemon notifier owns the channel post; a NullReminderChannelNotifier Null Object serves tests / no-channel daemons (a required dependency, not a nullable one). Tests: ReminderChannelFailureNotifier posts the right ChannelSendRequest (destination + DM kinds); status query returns per-reminder health and not-found. Full Netclaw.Actors.Tests (2482) and Netclaw.Daemon.Tests (754) green. Updates the netclaw-operations skill (scheduling reference, 2.19.0 → 2.20.0). Note: the behavioral eval suite (skill-content edit) needs a live model endpoint not available here — run in CI / before merge. --- .../.system/files/netclaw-operations/SKILL.md | 2 +- .../references/scheduling.md | 22 ++++ .../Reminders/ReminderManagerActorTests.cs | 44 +++++++- .../Reminders/IReminderChannelNotifier.cs | 45 ++++++++ .../Reminders/ReminderExecutionActor.cs | 2 +- .../Reminders/ReminderManagerActor.cs | 104 +++++++++++++++++- .../Reminders/ReminderProtocol.cs | 24 ++++ src/Netclaw.Cli/Daemon/DaemonApi.cs | 7 ++ src/Netclaw.Cli/Reminder/ReminderCommand.cs | 79 +++++++++++++ .../ReminderChannelFailureNotifierTests.cs | 84 ++++++++++++++ src/Netclaw.Daemon/Program.cs | 7 ++ .../ReminderChannelFailureNotifier.cs | 79 +++++++++++++ .../ReminderEndpointRouteBuilderExtensions.cs | 34 ++++++ 13 files changed, 524 insertions(+), 9 deletions(-) create mode 100644 src/Netclaw.Actors/Reminders/IReminderChannelNotifier.cs create mode 100644 src/Netclaw.Daemon.Tests/Reminders/ReminderChannelFailureNotifierTests.cs create mode 100644 src/Netclaw.Daemon/Reminders/ReminderChannelFailureNotifier.cs diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index 7f00a4c59..dec7f1351 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.19.0" + version: "2.20.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 10b3330f0..097c95d05 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/scheduling.md +++ b/feeds/skills/.system/files/netclaw-operations/references/scheduling.md @@ -58,6 +58,28 @@ Reminders that hit 5 consecutive execution 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. +**Failure visibility.** When a reminder execution fails for any reason — including +the 20-minute stall backstop that recovers a wedged run — the failure is posted +as a plain-language notice to the reminder's **destination channel** (for +`channel`-delivery reminders), so the operator sees it where they expect that +reminder's output. This is bounded by the auto-disable threshold (at most a few +notices plus the disabled notice), not the unbounded skip stream. + +A *skipped* fire (one that arrives while the prior execution is still running) is +**not** posted to the channel — it would be too noisy — but it is counted and +surfaced by the status command: + +``` +netclaw reminder status +``` + +`status` shows, per reminder: whether it's enabled, whether an execution is in +flight right now, when it next fires, the consecutive-failure count, the +skipped-fire count (since daemon start), and recent run history. Reach for it +when a reminder seems to have silently stopped doing its job — a high skip count +means a prior run is wedged (it should self-recover within ~20 minutes), and a +rising failure count points at a misconfigured or broken reminder. + If `audience` is omitted during conversational scheduling, the reminder inherits the audience of the channel/session that created it. A reminder cannot be minted with broader audience than the creator currently holds; lowering the diff --git a/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs b/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs index b10cc9d86..2886b270b 100644 --- a/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs +++ b/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs @@ -73,7 +73,8 @@ protected override void ConfigureAkka(AkkaConfigurationBuilder builder, IService TimeProvider.System, definitionStore, historyStore, - _notificationSink)), + _notificationSink, + NullReminderChannelNotifier.Instance)), "reminder-manager-test"); registry.Register(reminderManager); @@ -174,6 +175,44 @@ public async Task Health_query_on_empty_manager_returns_zeros() Assert.Equal(0, health.FailedCount); } + [Fact] + public async Task Status_query_returns_per_reminder_health() + { + var manager = await GetManagerAsync(); + + var definition = CreateDefinition("test-status", "Check status"); + var authorization = new ReminderAudienceAuthorizationContext(TrustAudience.Team, "test"); + await manager.Ask( + new SaveReminderCommand(definition, Authorization: authorization), 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.True(status.Enabled); + Assert.False(status.Executing); + Assert.Equal(0, status.ConsecutiveFailures); + Assert.Equal(0, status.SkippedDuplicates); + Assert.NotNull(status.NextFire); + Assert.Empty(status.RecentHistory); + } + + [Fact] + public async Task Status_query_for_unknown_reminder_returns_not_found() + { + var manager = await GetManagerAsync(); + + var status = await manager.Ask( + new GetReminderStatusQuery(new ReminderId("does-not-exist")), + TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + + Assert.False(status.Found); + Assert.False(status.Enabled); + Assert.Equal(0, status.ConsecutiveFailures); + Assert.Equal(0, status.SkippedDuplicates); + Assert.Empty(status.RecentHistory); + } + [Fact] public async Task Reconcile_deletes_zombie_oneshot_reminders() { @@ -411,7 +450,8 @@ public async Task Startup_emits_alert_for_legacy_reminder_missing_trust_fields() TimeProvider.System, store, new ReminderHistoryStore(paths), - sink)), + sink, + NullReminderChannelNotifier.Instance)), "legacy-reminder-alert-manager"); // The legacy-schema alert is emitted synchronously inside PreStart, and diff --git a/src/Netclaw.Actors/Reminders/IReminderChannelNotifier.cs b/src/Netclaw.Actors/Reminders/IReminderChannelNotifier.cs new file mode 100644 index 000000000..44848de1f --- /dev/null +++ b/src/Netclaw.Actors/Reminders/IReminderChannelNotifier.cs @@ -0,0 +1,45 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Netclaw.Tools; + +namespace Netclaw.Actors.Reminders; + +/// +/// Posts an operator-facing notice to a reminder's destination channel when an +/// execution fails. Implemented in the daemon over the channel outbound +/// registry so the actor layer stays transport-agnostic (the manager hands over +/// an already-resolved and plain text). +/// Fire-and-forget (void) — the manager must never block on channel +/// delivery, mirroring . +/// +public interface IReminderChannelNotifier +{ + /// + /// Posts to . Must be + /// thread-safe and must not throw — delivery failures are the + /// implementation's problem to log, never the caller's to handle. + /// + void NotifyFailure(ChannelDeliveryTargetInfo target, string text); +} + +/// +/// No-op notifier for environments with no channel outbound path (e.g. tests, or +/// a daemon with no channels configured). A real Null Object — explicit, not a +/// silent fallback: callers still get a non-null required dependency. +/// +public sealed class NullReminderChannelNotifier : IReminderChannelNotifier +{ + public static readonly NullReminderChannelNotifier Instance = new(); + + private NullReminderChannelNotifier() + { + } + + public void NotifyFailure(ChannelDeliveryTargetInfo target, string text) + { + // Intentionally does nothing. + } +} diff --git a/src/Netclaw.Actors/Reminders/ReminderExecutionActor.cs b/src/Netclaw.Actors/Reminders/ReminderExecutionActor.cs index 1fe2ddacf..047fbc92f 100644 --- a/src/Netclaw.Actors/Reminders/ReminderExecutionActor.cs +++ b/src/Netclaw.Actors/Reminders/ReminderExecutionActor.cs @@ -487,7 +487,7 @@ private static string BuildChannelDeliveryGuidance(ReminderDefinition definition "Transport and address may be missing or invalid."); } - private static ChannelDeliveryTargetInfo? ResolveChannelDeliveryTarget(ReminderDefinition definition) + internal static ChannelDeliveryTargetInfo? ResolveChannelDeliveryTarget(ReminderDefinition definition) { if (definition.Delivery.Target is not null) return definition.Delivery.Target; diff --git a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs index 63a92d741..43254f34e 100644 --- a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs +++ b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs @@ -44,6 +44,7 @@ public sealed partial class ReminderManagerActor : ReceiveActor private readonly ReminderDefinitionStore _definitionStore; private readonly ReminderHistoryStore _historyStore; private readonly IOperationalNotificationSink _notificationSink; + private readonly IReminderChannelNotifier _channelNotifier; private readonly ILoggingAdapter _log; private IReminderClient? _client; @@ -51,6 +52,7 @@ public sealed partial class ReminderManagerActor : ReceiveActor private readonly ActiveExecutionTracker _activeExecutions = new(); private readonly Queue _deferredQueue = new(); private readonly Dictionary _failureCounts = []; + private readonly Dictionary _skipCounts = []; public ReminderManagerActor( ISessionPipeline pipeline, @@ -59,7 +61,8 @@ public ReminderManagerActor( TimeProvider timeProvider, ReminderDefinitionStore definitionStore, ReminderHistoryStore historyStore, - IOperationalNotificationSink notificationSink) + IOperationalNotificationSink notificationSink, + IReminderChannelNotifier channelNotifier) { _pipeline = pipeline; _defaults = defaults; @@ -68,6 +71,7 @@ public ReminderManagerActor( _definitionStore = definitionStore; _historyStore = historyStore; _notificationSink = notificationSink; + _channelNotifier = channelNotifier; _log = Context.GetLogger(); ReceiveAsync(HandleSaveAsync); @@ -83,6 +87,7 @@ public ReminderManagerActor( ReceiveAsync(_ => HandleReconcileAsync()); Receive(_ => HandleGetHealth()); + ReceiveAsync(HandleGetStatusAsync); } protected override void PreStart() @@ -417,6 +422,7 @@ private async Task DisableReminderInternalAsync(ReminderI await CancelScheduleOnlyAsync(id); _failureCounts.Remove(id); + _skipCounts.Remove(id); RemoveFromDeferredQueue(id); _log.Info("Disabled reminder '{0}'", id.Value); @@ -433,6 +439,7 @@ private async Task DeleteReminderInternalAsync(ReminderId id) _definitionStore.Delete(id); await CancelScheduleOnlyAsync(id); _failureCounts.Remove(id); + _skipCounts.Remove(id); RemoveFromDeferredQueue(id); try @@ -576,8 +583,7 @@ private async Task HandleReminderFiredAsync(ReminderEnvelope en if (_activeExecutions.IsExecuting(reminderId)) { - _log.Warning("reminder_skipped_duplicate_execution reminder_id={0} title={1}", - reminderId.Value, definition.Title); + RecordSkippedDuplicate(reminderId, definition.Title, "scheduled"); await _client!.AckAsync(envelope); return; } @@ -621,6 +627,44 @@ private async Task HandleReminderFiredAsync(ReminderEnvelope en } } + /// + /// Records a skipped fire (a fire that arrived while a prior execution of the + /// same reminder was still running). Counted in-memory since daemon start and + /// surfaced by netclaw reminder status so the silent-skip pattern from + /// #1492/#1494 (49 skips in a day, unnoticed) is now visible to operators. + /// + private void RecordSkippedDuplicate(ReminderId reminderId, string title, string source) + { + var count = _skipCounts.GetValueOrDefault(reminderId) + 1; + _skipCounts[reminderId] = count; + _log.Warning( + "reminder_skipped_duplicate_execution reminder_id={0} title={1} source={2} skip_count={3}", + reminderId.Value, title, source, count); + } + + /// + /// Posts an operator-facing failure notice to a reminder's destination + /// channel. Only Channel-delivery reminders have such a channel; CurrentSession + /// and None failures are surfaced via the operational alert sink instead. The + /// notifier is fire-and-forget — never blocks or throws into the manager. + /// + private void PostFailureNoticeToChannel(ReminderDefinition? definition, string text) + { + if (definition is not { Delivery.Kind: DeliveryKind.Channel }) + return; + + var target = ReminderExecutionActor.ResolveChannelDeliveryTarget(definition); + if (target is null) + { + _log.Warning( + "Reminder '{0}' failed but its channel delivery target could not be resolved; no channel notice posted.", + definition.Id.Value); + return; + } + + _channelNotifier.NotifyFailure(target, text); + } + private async Task HandleExecutionCompletedAsync(ReminderExecutionCompleted completed) { if (!_activeExecutions.Remove(completed.Id)) @@ -659,6 +703,15 @@ private async Task HandleExecutionCompletedAsync(ReminderExecutionCompleted comp ["error"] = completed.ErrorMessage ?? "unknown", })); + // Surface the failure where the operator expects this reminder's + // output: its destination channel. Bounded by the auto-disable + // threshold below, so a permanently-broken reminder posts at most + // FailurePauseThreshold notices plus the disabled notice — not the + // unbounded skip stream that #1494 also makes visible via status. + PostFailureNoticeToChannel( + definition, + $"Reminder \"{title}\" failed: {completed.ErrorMessage ?? "unknown error"}"); + if (count >= FailurePauseThreshold) { _log.Warning("Reminder '{0}' hit failure threshold ({1}), disabling", @@ -679,6 +732,11 @@ private async Task HandleExecutionCompletedAsync(ReminderExecutionCompleted comp ["failureCount"] = count.ToString(), })); + PostFailureNoticeToChannel( + definition, + $"Reminder \"{title}\" was automatically disabled after {count} consecutive failures. " + + $"Last error: {completed.ErrorMessage ?? "unknown error"}"); + await DisableReminderInternalAsync(completed.Id); _failureCounts.Remove(completed.Id); } @@ -806,8 +864,7 @@ private async Task ProcessDeferredQueueAsync() if (_activeExecutions.IsExecuting(nextId)) { - _log.Warning("reminder_skipped_duplicate_execution reminder_id={0} title={1} source=deferred_queue", - nextId.Value, definition.Title); + RecordSkippedDuplicate(nextId, definition.Title, "deferred_queue"); continue; } @@ -989,6 +1046,43 @@ private void HandleGetHealth() _failureCounts.Count)); } + private async Task HandleGetStatusAsync(GetReminderStatusQuery query) + { + var replyTo = Sender; + try + { + var definition = _definitionStore.Get(query.Id); + if (definition is null) + { + replyTo.Tell(NotFoundStatus(query.Id)); + return; + } + + var schedules = await ListScheduledRemindersAsync(); + var history = await _historyStore.ReadAsync(query.Id, 10); + + replyTo.Tell(new ReminderStatusResponse( + query.Id, + Found: true, + Enabled: definition.Enabled, + Executing: _activeExecutions.IsExecuting(query.Id), + NextFire: schedules.GetValueOrDefault(query.Id.Value), + ConsecutiveFailures: _failureCounts.GetValueOrDefault(query.Id), + SkippedDuplicates: _skipCounts.GetValueOrDefault(query.Id), + RecentHistory: history)); + } + catch (Exception ex) + { + _log.Error(ex, "Error getting status for reminder '{0}'", query.Id.Value); + replyTo.Tell(NotFoundStatus(query.Id)); + } + + static ReminderStatusResponse NotFoundStatus(ReminderId id) => new( + id, Found: false, Enabled: false, Executing: false, + NextFire: null, ConsecutiveFailures: 0, SkippedDuplicates: 0, + RecentHistory: []); + } + private sealed record ScheduleAttempt(bool IsSuccess, DateTimeOffset? NextFire, string? ErrorMessage) : INoSerializationVerificationNeeded { public static ScheduleAttempt Ok(DateTimeOffset? nextFire) => new(true, nextFire, null); diff --git a/src/Netclaw.Actors/Reminders/ReminderProtocol.cs b/src/Netclaw.Actors/Reminders/ReminderProtocol.cs index 5700a19f6..15c1da6d0 100644 --- a/src/Netclaw.Actors/Reminders/ReminderProtocol.cs +++ b/src/Netclaw.Actors/Reminders/ReminderProtocol.cs @@ -404,6 +404,30 @@ public sealed record ReminderHealthResponse( int ActiveExecutions, int FailedCount) : IReminderResponse, INoSerializationVerificationNeeded; +/// +/// Query sent to for the per-reminder +/// operational status surfaced by netclaw reminder status <id>. +/// +public sealed record GetReminderStatusQuery(ReminderId Id) : IReminderQuery, INoSerializationVerificationNeeded; + +/// +/// Response to : per-reminder health for an +/// operator — whether the reminder exists/is enabled, whether an execution is in +/// flight right now, when it next fires, the consecutive-failure and +/// skipped-duplicate counts (in-memory since daemon start), and recent run +/// history. Lets netclaw reminder status answer "is this reminder healthy +/// or is it silently failing/skipping?" — the gap that hid #1492. +/// +public sealed record ReminderStatusResponse( + ReminderId Id, + bool Found, + bool Enabled, + bool Executing, + DateTimeOffset? NextFire, + int ConsecutiveFailures, + int SkippedDuplicates, + IReadOnlyList RecentHistory) : IReminderResponse, INoSerializationVerificationNeeded; + } public sealed record ReminderInfo( diff --git a/src/Netclaw.Cli/Daemon/DaemonApi.cs b/src/Netclaw.Cli/Daemon/DaemonApi.cs index 08f1af603..e0fa2e512 100644 --- a/src/Netclaw.Cli/Daemon/DaemonApi.cs +++ b/src/Netclaw.Cli/Daemon/DaemonApi.cs @@ -179,6 +179,13 @@ public async Task GetReminderHistoryAsync(string id, int la return await client.GetAsync($"{_endpoint}/api/reminders/{id}/history?last={last}", cts.Token); } + public async Task GetReminderStatusAsync(string id, CancellationToken ct = default) + { + using var cts = CreateTimeoutCts(DefaultTimeout, ct); + var client = CreateHttpClient(); + return await client.GetAsync($"{_endpoint}/api/reminders/{id}/status", cts.Token); + } + public async Task EnableReminderAsync(string id, CancellationToken ct = default) { using var cts = CreateTimeoutCts(DefaultTimeout, ct); diff --git a/src/Netclaw.Cli/Reminder/ReminderCommand.cs b/src/Netclaw.Cli/Reminder/ReminderCommand.cs index 1090cebf0..da9d00ea0 100644 --- a/src/Netclaw.Cli/Reminder/ReminderCommand.cs +++ b/src/Netclaw.Cli/Reminder/ReminderCommand.cs @@ -59,6 +59,7 @@ public static async Task RunAsync(string[] args, DaemonApi? daemonApi) "import" => await RunImportAsync(daemonApi, args), "show" => await RunShowAsync(daemonApi, args), "history" => await RunHistoryAsync(daemonApi, args), + "status" => await RunStatusAsync(daemonApi, args), _ => WriteHelp() }; } @@ -552,6 +553,83 @@ private static async Task RunHistoryAsync(DaemonApi api, string[] args) } } + private static async Task RunStatusAsync(DaemonApi api, string[] args) + { + if (args.Length < 3) + { + Console.Error.WriteLine("Usage: netclaw reminder status "); + return 1; + } + + var id = args[2]; + + try + { + using var response = await api.GetReminderStatusAsync(id); + + if (response.StatusCode == System.Net.HttpStatusCode.NotFound) + { + Console.Error.WriteLine($"[FAIL] Reminder '{id}' not found."); + return 1; + } + + if (!response.IsSuccessStatusCode) + { + Console.Error.WriteLine($"[FAIL] daemon returned {(int)response.StatusCode}"); + return 1; + } + + var json = await response.Content.ReadAsStringAsync(); + var status = JsonSerializer.Deserialize(json, JsonOptions); + if (status is null) + { + Console.Error.WriteLine("[FAIL] could not parse status response."); + return 1; + } + + Console.WriteLine($"Reminder: {status.Id}"); + Console.WriteLine($"Enabled: {status.Enabled}"); + Console.WriteLine($"Executing now: {status.Executing}"); + Console.WriteLine($"Next fire: {status.NextFire ?? "—"}"); + Console.WriteLine($"Consecutive fails: {status.ConsecutiveFailures}"); + Console.WriteLine($"Skipped (duplicate): {status.SkippedDuplicates}"); + + var history = status.RecentHistory ?? []; + if (history.Length == 0) + { + Console.WriteLine("Recent history: none"); + } + else + { + Console.WriteLine("Recent history:"); + foreach (var r in history.Take(5)) + { + var outcome = r.Success ? "ok" : "failed"; + var err = string.IsNullOrEmpty(r.ErrorMessage) ? "" : $" — {r.ErrorMessage}"; + Console.WriteLine($" {r.FiredAt:u} {outcome}{err}"); + } + } + + return 0; + } + catch (Exception ex) + { + Console.Error.WriteLine($"[FAIL] unable to reach daemon: {ex.Message}"); + Console.Error.WriteLine(" fix: run `netclaw daemon start` and retry."); + return 1; + } + } + + /// CLI-side projection of the daemon's reminder status JSON. + private sealed record ReminderStatusView( + string Id, + bool Enabled, + bool Executing, + string? NextFire, + int ConsecutiveFailures, + int SkippedDuplicates, + HistoryRecord[]? RecentHistory); + private static int WriteHelp() { Console.WriteLine("Usage: netclaw reminder "); @@ -567,6 +645,7 @@ private static int WriteHelp() Console.WriteLine(" validate Validate reminder file"); Console.WriteLine(" show Show reminder details"); Console.WriteLine(" history [--last N] Show recent execution history (default: 20)"); + Console.WriteLine(" status Show operational status: failures, skipped fires, in-flight"); Console.WriteLine(); Console.WriteLine("Create options:"); Console.WriteLine(" --name Human-readable title (defaults to <id>)"); diff --git a/src/Netclaw.Daemon.Tests/Reminders/ReminderChannelFailureNotifierTests.cs b/src/Netclaw.Daemon.Tests/Reminders/ReminderChannelFailureNotifierTests.cs new file mode 100644 index 000000000..745d34364 --- /dev/null +++ b/src/Netclaw.Daemon.Tests/Reminders/ReminderChannelFailureNotifierTests.cs @@ -0,0 +1,84 @@ +// ----------------------------------------------------------------------- +// <copyright file="ReminderChannelFailureNotifierTests.cs" company="Petabridge, LLC"> +// Copyright (C) 2026 - 2026 Petabridge, LLC <https://petabridge.com> +// </copyright> +// ----------------------------------------------------------------------- +using Microsoft.Extensions.Logging.Abstractions; +using Netclaw.Channels; +using Netclaw.Daemon.Reminders; +using Netclaw.Tools; +using Xunit; + +namespace Netclaw.Daemon.Tests.Reminders; + +public class ReminderChannelFailureNotifierTests +{ + [Fact] + public async Task NotifyFailure_posts_text_to_the_resolved_destination_channel() + { + var key = ChannelDescriptorKey.Create("slack"); + var client = new CapturingOutboundClient(key); + var notifier = new ReminderChannelFailureNotifier( + new StubChannelRegistry(client), + NullLogger<ReminderChannelFailureNotifier>.Instance); + + var target = new ChannelDeliveryTargetInfo("slack", "destination", "C12345"); + notifier.NotifyFailure(target, "Reminder \"gotowebinar\" failed: stalled"); + + // Fire-and-forget — await the capture (a real signal, not a sleep). + var request = await client.Captured.WaitAsync(TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + + Assert.Equal(ChannelAddressKind.Destination, request.AddressKind); + Assert.Equal("C12345", request.TargetId); + Assert.Contains("failed", request.Text); + } + + [Fact] + public async Task NotifyFailure_maps_direct_message_kind() + { + var key = ChannelDescriptorKey.Create("slack"); + var client = new CapturingOutboundClient(key); + var notifier = new ReminderChannelFailureNotifier( + new StubChannelRegistry(client), + NullLogger<ReminderChannelFailureNotifier>.Instance); + + var target = new ChannelDeliveryTargetInfo("slack", "direct_message", "U999"); + notifier.NotifyFailure(target, "Reminder failed"); + + var request = await client.Captured.WaitAsync(TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + + Assert.Equal(ChannelAddressKind.DirectMessage, request.AddressKind); + Assert.Equal("U999", request.TargetId); + } + + private sealed class CapturingOutboundClient(ChannelDescriptorKey key) : IChannelOutboundClient + { + private readonly TaskCompletionSource<ChannelSendRequest> _captured = + new(TaskCreationOptions.RunContinuationsAsynchronously); + + public ChannelDescriptorKey Key { get; } = key; + + public Task<ChannelSendRequest> Captured => _captured.Task; + + public Task<string> SendMessageAsync(ChannelSendRequest request, CancellationToken ct = default) + { + _captured.TrySetResult(request); + return Task.FromResult("ok"); + } + } + + /// <summary>Minimal registry that only resolves the outbound client under test.</summary> + private sealed class StubChannelRegistry(IChannelOutboundClient client) : IChannelRegistry + { + public IChannelOutboundClient GetOutboundClient(ChannelDescriptorKey key) => client; + + public IReadOnlyCollection<ChannelDescriptor> ListChannels() => throw new NotImplementedException(); + public ChannelDescriptor GetChannel(ChannelDescriptorKey key) => throw new NotImplementedException(); + public ValueTask<ChannelRuntimeSnapshot> GetSnapshotAsync(ChannelDescriptorKey key, CancellationToken cancellationToken = default) => throw new NotImplementedException(); + public IChannelAddressResolver GetResolver(ChannelDescriptorKey key, ChannelAddressKind addressKind) => throw new NotImplementedException(); + public IChannelOutputRenderer GetOutputRenderer(ChannelDescriptorKey key) => throw new NotImplementedException(); + public ValueTask<ChannelAddressResolutionResult> ResolveAddressAsync(ChannelAddressResolutionRequest request, CancellationToken cancellationToken = default) => throw new NotImplementedException(); + public ValueTask<ChannelAddressResolutionResult> ListDestinationsAsync(ChannelDescriptorKey key, CancellationToken cancellationToken = default) => throw new NotImplementedException(); + public ValueTask<ChannelOutputRenderResult> RenderOutputAsync(ChannelOutputRenderRequest request, CancellationToken cancellationToken = default) => throw new NotImplementedException(); + } +} diff --git a/src/Netclaw.Daemon/Program.cs b/src/Netclaw.Daemon/Program.cs index 0975afb59..1d0219560 100644 --- a/src/Netclaw.Daemon/Program.cs +++ b/src/Netclaw.Daemon/Program.cs @@ -751,6 +751,13 @@ static void ConfigureDaemonServices( services.AddSingleton<IOperationalNotificationSink>(NullNotificationSink.Instance); } + // Posts reminder failure notices to the reminder's destination channel. + // IChannelRegistry is always registered (AddChannelRegistry below), so the + // real notifier is always available; it no-ops gracefully for reminders whose + // channel has no outbound client. + services.AddSingleton<Netclaw.Actors.Reminders.IReminderChannelNotifier, + Netclaw.Daemon.Reminders.ReminderChannelFailureNotifier>(); + // Daemon lifecycle notifier (startup/shutdown webhooks + logging) services.AddSingleton<DaemonLifecycleNotifier>(); diff --git a/src/Netclaw.Daemon/Reminders/ReminderChannelFailureNotifier.cs b/src/Netclaw.Daemon/Reminders/ReminderChannelFailureNotifier.cs new file mode 100644 index 000000000..65eb1f195 --- /dev/null +++ b/src/Netclaw.Daemon/Reminders/ReminderChannelFailureNotifier.cs @@ -0,0 +1,79 @@ +// ----------------------------------------------------------------------- +// <copyright file="ReminderChannelFailureNotifier.cs" company="Petabridge, LLC"> +// Copyright (C) 2026 - 2026 Petabridge, LLC <https://petabridge.com> +// </copyright> +// ----------------------------------------------------------------------- +using Microsoft.Extensions.Logging; +using Netclaw.Actors.Reminders; +using Netclaw.Channels; +using Netclaw.Tools; + +namespace Netclaw.Daemon.Reminders; + +/// <summary> +/// Daemon-side <see cref="IReminderChannelNotifier"/>: posts a reminder's failure +/// notice to its destination channel via the channel outbound registry, so the +/// operator sees the failure where they expect that reminder's output. Lives in +/// the daemon (not the actor) because <see cref="IChannelRegistry"/> is a channel +/// concern; the actor layer stays transport-agnostic. Fire-and-forget — never +/// blocks or throws into the reminder manager; delivery failures are logged. +/// </summary> +internal sealed class ReminderChannelFailureNotifier : IReminderChannelNotifier +{ + private readonly IChannelRegistry _registry; + private readonly ILogger<ReminderChannelFailureNotifier> _logger; + + public ReminderChannelFailureNotifier( + IChannelRegistry registry, + ILogger<ReminderChannelFailureNotifier> logger) + { + _registry = registry; + _logger = logger; + } + + public void NotifyFailure(ChannelDeliveryTargetInfo target, string text) + => _ = PostAsync(target, text); + + private async Task PostAsync(ChannelDeliveryTargetInfo target, string text) + { + try + { + if (!ChannelAddressKindWire.TryParse(target.DestinationKind, out var addressKind)) + { + _logger.LogWarning( + "Reminder failure notice not posted: unrecognized destination kind '{DestinationKind}' for channel '{ChannelKey}'.", + target.DestinationKind, target.ChannelKey); + return; + } + + IChannelOutboundClient outbound; + try + { + outbound = _registry.GetOutboundClient(ChannelDescriptorKey.Create(target.ChannelKey)); + } + catch (InvalidOperationException) + { + _logger.LogWarning( + "Reminder failure notice not posted: channel '{ChannelKey}' has no registered outbound client.", + target.ChannelKey); + return; + } + + // Outbound clients return an "Error: ..." string for expected send + // failures rather than throwing, so inspect the result. + var result = await outbound.SendMessageAsync( + new ChannelSendRequest(addressKind, target.DestinationId, text)); + + if (result.StartsWith("Error:", StringComparison.Ordinal)) + _logger.LogWarning( + "Reminder failure notice to channel '{ChannelKey}' was rejected: {Result}", + target.ChannelKey, result); + } + catch (Exception ex) + { + _logger.LogWarning(ex, + "Failed to post reminder failure notice to channel '{ChannelKey}'.", + target.ChannelKey); + } + } +} diff --git a/src/Netclaw.Daemon/Reminders/ReminderEndpointRouteBuilderExtensions.cs b/src/Netclaw.Daemon/Reminders/ReminderEndpointRouteBuilderExtensions.cs index 1a2cd1ba0..382b71399 100644 --- a/src/Netclaw.Daemon/Reminders/ReminderEndpointRouteBuilderExtensions.cs +++ b/src/Netclaw.Daemon/Reminders/ReminderEndpointRouteBuilderExtensions.cs @@ -307,6 +307,30 @@ public static IEndpointRouteBuilder MapReminderEndpoints(this IEndpointRouteBuil .WithName("GetReminderHistory") .WithSummary("Get recent fire history for a reminder."); + reminders.MapGet("/{id}/status", async ValueTask<Results<Ok<ReminderStatusDto>, NotFound<ReminderErrorResponse>>> ( + string id, + IRequiredActor<ReminderManagerActorKey> actor, + CancellationToken ct) => + { + var manager = await actor.GetAsync(ct); + var status = await manager.Ask<ReminderStatusResponse>( + new GetReminderStatusQuery(new ReminderId(id)), TimeSpan.FromSeconds(10), ct); + + if (!status.Found) + return TypedResults.NotFound(new ReminderErrorResponse($"Reminder '{id}' not found.")); + + return TypedResults.Ok(new ReminderStatusDto( + Id: status.Id.Value, + Enabled: status.Enabled, + Executing: status.Executing, + NextFire: SetReminderTool.FormatTimestamp(status.NextFire), + ConsecutiveFailures: status.ConsecutiveFailures, + SkippedDuplicates: status.SkippedDuplicates, + RecentHistory: status.RecentHistory)); + }) + .WithName("GetReminderStatus") + .WithSummary("Get per-reminder operational status: in-flight, consecutive failures, skipped fires, recent history."); + return app; } @@ -381,6 +405,16 @@ internal sealed record ReminderDetailDto( string? DeliveryInstructions, string? Audience); +/// <summary>Per-reminder operational status projection (see <c>GET /{id}/status</c>).</summary> +internal sealed record ReminderStatusDto( + string Id, + bool Enabled, + bool Executing, + string? NextFire, + int ConsecutiveFailures, + int SkippedDuplicates, + IReadOnlyList<HistoryRecord> RecentHistory); + /// <summary>Acknowledgement carrying a human-readable message.</summary> internal sealed record ReminderMessageResponse(string Message); From be4e1dc76abb4f0cf06f434c99fffce465a8e63a Mon Sep 17 00:00:00 2001 From: Aaron Stannard <aaron@petabridge.com> Date: Fri, 26 Jun 2026 16:36:03 +0000 Subject: [PATCH 2/2] fix(reminders): address code-review findings on #1494 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified findings from the workflow code review: - LlmSessionTestBase: register IReminderChannelNotifier (the new required reminder-manager dependency) so DI-resolved reminder managers in session integration tests initialize instead of dying with ActorInitializationException. - HandleGetStatusAsync: a transient read failure (schedule fetch / history IO) no longer impersonates "not found" — it faults the Ask (Status.Failure → the endpoint maps to 5xx) so the operator sees a real error instead of being told a wedged reminder was deleted. That was a silent fallback on the very feature meant to expose them. - Status query: read and display the same RecentHistoryCount (5) records, newest first — the CLI previously showed the OLDEST 5 of a last-10 window, hiding the most recent (failing) runs. - Channel notice: post the per-failure notice only below the auto-disable threshold; on the threshold-hitting failure the disabled notice already carries the last error, so both was double noise on the most important event. - NextFire: pass null through (FormatTimestamp renders null as "unknown"); the CLI shows "not scheduled" for disabled/unscheduled reminders. - Status handler runs its two independent backend reads concurrently (Task.WhenAll). Full Netclaw.Actors.Tests (2482) green; reminder + Daemon notifier tests green. Not changed (verified lower-severity / pre-existing patterns): whole-list scheduler fetch (matches existing GetReminder), notifier↔ChannelSendTools and CLI-subcommand scaffolding duplication. --- .../Sessions/LlmSessionTestBase.cs | 1 + .../Reminders/ReminderManagerActor.cs | 50 ++++++++++++------- src/Netclaw.Cli/Reminder/ReminderCommand.cs | 8 +-- .../ReminderEndpointRouteBuilderExtensions.cs | 4 +- 4 files changed, 41 insertions(+), 22 deletions(-) diff --git a/src/Netclaw.Actors.Tests/Sessions/LlmSessionTestBase.cs b/src/Netclaw.Actors.Tests/Sessions/LlmSessionTestBase.cs index c98468f51..97b941644 100644 --- a/src/Netclaw.Actors.Tests/Sessions/LlmSessionTestBase.cs +++ b/src/Netclaw.Actors.Tests/Sessions/LlmSessionTestBase.cs @@ -63,6 +63,7 @@ protected sealed override void ConfigureServices(HostBuilderContext context, ISe services.AddSingleton<ReminderDefinitionStore>(); services.AddSingleton<ReminderHistoryStore>(); services.AddSingleton<IOperationalNotificationSink>(NullNotificationSink.Instance); + services.AddSingleton<IReminderChannelNotifier>(NullReminderChannelNotifier.Instance); ConfigureSessionServices(services); services.AddLlmSessionCompositeRecords(); } diff --git a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs index 43254f34e..eddfe5d2c 100644 --- a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs +++ b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs @@ -37,6 +37,9 @@ public sealed partial class ReminderManagerActor : ReceiveActor /// </summary> internal const int FailurePauseThreshold = 5; + /// <summary>Recent run records returned by the per-reminder status query.</summary> + internal const int RecentHistoryCount = 5; + private readonly ISessionPipeline _pipeline; private readonly EffectivePolicyDefaults _defaults; private readonly SchedulingConfig _schedulingConfig; @@ -704,13 +707,17 @@ private async Task HandleExecutionCompletedAsync(ReminderExecutionCompleted comp })); // Surface the failure where the operator expects this reminder's - // output: its destination channel. Bounded by the auto-disable - // threshold below, so a permanently-broken reminder posts at most - // FailurePauseThreshold notices plus the disabled notice — not the - // unbounded skip stream that #1494 also makes visible via status. - PostFailureNoticeToChannel( - definition, - $"Reminder \"{title}\" failed: {completed.ErrorMessage ?? "unknown error"}"); + // output: its destination channel. Only below the threshold — on the + // threshold-hitting failure the disabled notice below already carries + // the last error, so posting both would double the noise on the most + // important event. Bounded overall by the threshold, never the + // unbounded skip stream that #1494 makes visible via status instead. + if (count < FailurePauseThreshold) + { + PostFailureNoticeToChannel( + definition, + $"Reminder \"{title}\" failed: {completed.ErrorMessage ?? "unknown error"}"); + } if (count >= FailurePauseThreshold) { @@ -1054,33 +1061,40 @@ private async Task HandleGetStatusAsync(GetReminderStatusQuery query) var definition = _definitionStore.Get(query.Id); if (definition is null) { - replyTo.Tell(NotFoundStatus(query.Id)); + replyTo.Tell(new ReminderStatusResponse( + query.Id, Found: false, Enabled: false, Executing: false, + NextFire: null, ConsecutiveFailures: 0, SkippedDuplicates: 0, + RecentHistory: [])); return; } - var schedules = await ListScheduledRemindersAsync(); - var history = await _historyStore.ReadAsync(query.Id, 10); + // Two independent backend reads — run them concurrently so the + // query's latency is max(schedule, history) instead of their sum. + // Neither touches actor state until both complete. + var schedulesTask = ListScheduledRemindersAsync(); + var historyTask = _historyStore.ReadAsync(query.Id, RecentHistoryCount); + await Task.WhenAll(schedulesTask, historyTask); replyTo.Tell(new ReminderStatusResponse( query.Id, Found: true, Enabled: definition.Enabled, Executing: _activeExecutions.IsExecuting(query.Id), - NextFire: schedules.GetValueOrDefault(query.Id.Value), + NextFire: schedulesTask.Result.GetValueOrDefault(query.Id.Value), ConsecutiveFailures: _failureCounts.GetValueOrDefault(query.Id), SkippedDuplicates: _skipCounts.GetValueOrDefault(query.Id), - RecentHistory: history)); + RecentHistory: historyTask.Result)); } catch (Exception ex) { + // The definition existed, so this is a transient read failure — NOT a + // missing reminder. Faulting the Ask surfaces a real error (the + // endpoint maps it to 5xx); replying not-found here would tell the + // operator a wedged reminder was deleted, the silent fallback this + // very feature exists to expose. _log.Error(ex, "Error getting status for reminder '{0}'", query.Id.Value); - replyTo.Tell(NotFoundStatus(query.Id)); + replyTo.Tell(new Status.Failure(ex)); } - - static ReminderStatusResponse NotFoundStatus(ReminderId id) => new( - id, Found: false, Enabled: false, Executing: false, - NextFire: null, ConsecutiveFailures: 0, SkippedDuplicates: 0, - RecentHistory: []); } private sealed record ScheduleAttempt(bool IsSuccess, DateTimeOffset? NextFire, string? ErrorMessage) : INoSerializationVerificationNeeded diff --git a/src/Netclaw.Cli/Reminder/ReminderCommand.cs b/src/Netclaw.Cli/Reminder/ReminderCommand.cs index da9d00ea0..3acc3301c 100644 --- a/src/Netclaw.Cli/Reminder/ReminderCommand.cs +++ b/src/Netclaw.Cli/Reminder/ReminderCommand.cs @@ -590,7 +590,7 @@ private static async Task<int> RunStatusAsync(DaemonApi api, string[] args) Console.WriteLine($"Reminder: {status.Id}"); Console.WriteLine($"Enabled: {status.Enabled}"); Console.WriteLine($"Executing now: {status.Executing}"); - Console.WriteLine($"Next fire: {status.NextFire ?? "—"}"); + Console.WriteLine($"Next fire: {status.NextFire ?? "not scheduled"}"); Console.WriteLine($"Consecutive fails: {status.ConsecutiveFailures}"); Console.WriteLine($"Skipped (duplicate): {status.SkippedDuplicates}"); @@ -601,8 +601,10 @@ private static async Task<int> RunStatusAsync(DaemonApi api, string[] args) } else { - Console.WriteLine("Recent history:"); - foreach (var r in history.Take(5)) + Console.WriteLine("Recent history (newest first):"); + // History arrives oldest-first; reverse so the most recent runs + // (the ones an operator diagnosing a failure cares about) lead. + foreach (var r in history.Reverse()) { var outcome = r.Success ? "ok" : "failed"; var err = string.IsNullOrEmpty(r.ErrorMessage) ? "" : $" — {r.ErrorMessage}"; diff --git a/src/Netclaw.Daemon/Reminders/ReminderEndpointRouteBuilderExtensions.cs b/src/Netclaw.Daemon/Reminders/ReminderEndpointRouteBuilderExtensions.cs index 382b71399..ce1dc6001 100644 --- a/src/Netclaw.Daemon/Reminders/ReminderEndpointRouteBuilderExtensions.cs +++ b/src/Netclaw.Daemon/Reminders/ReminderEndpointRouteBuilderExtensions.cs @@ -323,7 +323,9 @@ public static IEndpointRouteBuilder MapReminderEndpoints(this IEndpointRouteBuil Id: status.Id.Value, Enabled: status.Enabled, Executing: status.Executing, - NextFire: SetReminderTool.FormatTimestamp(status.NextFire), + // Pass null through (FormatTimestamp renders null as the literal + // "unknown"); the CLI shows "not scheduled" for an absent next fire. + NextFire: status.NextFire is null ? null : SetReminderTool.FormatTimestamp(status.NextFire), ConsecutiveFailures: status.ConsecutiveFailures, SkippedDuplicates: status.SkippedDuplicates, RecentHistory: status.RecentHistory));