-
Notifications
You must be signed in to change notification settings - Fork 28
feat(reminders): operator visibility into reminder failures and skips (#1494) #1503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Aaronontheweb
merged 4 commits into
netclaw-dev:dev
from
Aaronontheweb:fix/1494-reminder-failure-visibility
Jun 26, 2026
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
1b75c2c
feat(reminders): operator visibility into reminder failures and skips…
Aaronontheweb be4e1dc
fix(reminders): address code-review findings on #1494
Aaronontheweb f7f48b9
Merge branch 'dev' into fix/1494-reminder-failure-visibility
Aaronontheweb 1d39a74
Merge branch 'dev' into fix/1494-reminder-failure-visibility
Aaronontheweb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| // ----------------------------------------------------------------------- | ||
| // <copyright file="IReminderChannelNotifier.cs" company="Petabridge, LLC"> | ||
| // Copyright (C) 2026 - 2026 Petabridge, LLC <https://petabridge.com> | ||
| // </copyright> | ||
| // ----------------------------------------------------------------------- | ||
| using Netclaw.Tools; | ||
|
|
||
| namespace Netclaw.Actors.Reminders; | ||
|
|
||
| /// <summary> | ||
| /// 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 <see cref="ChannelDeliveryTargetInfo"/> and plain text). | ||
| /// Fire-and-forget (<c>void</c>) — the manager must never block on channel | ||
| /// delivery, mirroring <see cref="Configuration.IOperationalNotificationSink"/>. | ||
| /// </summary> | ||
| public interface IReminderChannelNotifier | ||
| { | ||
| /// <summary> | ||
| /// Posts <paramref name="text"/> to <paramref name="target"/>. Must be | ||
| /// thread-safe and must not throw — delivery failures are the | ||
| /// implementation's problem to log, never the caller's to handle. | ||
| /// </summary> | ||
| void NotifyFailure(ChannelDeliveryTargetInfo target, string text); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// 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. | ||
| /// </summary> | ||
| public sealed class NullReminderChannelNotifier : IReminderChannelNotifier | ||
| { | ||
| public static readonly NullReminderChannelNotifier Instance = new(); | ||
|
|
||
| private NullReminderChannelNotifier() | ||
| { | ||
| } | ||
|
|
||
| public void NotifyFailure(ChannelDeliveryTargetInfo target, string text) | ||
| { | ||
| // Intentionally does nothing. | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,20 +37,25 @@ 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; | ||
| private readonly TimeProvider _timeProvider; | ||
| private readonly ReminderDefinitionStore _definitionStore; | ||
| private readonly ReminderHistoryStore _historyStore; | ||
| private readonly IOperationalNotificationSink _notificationSink; | ||
| private readonly IReminderChannelNotifier _channelNotifier; | ||
| private readonly ILoggingAdapter _log; | ||
|
|
||
| private IReminderClient? _client; | ||
|
|
||
| private readonly ActiveExecutionTracker _activeExecutions = new(); | ||
| private readonly Queue<ReminderId> _deferredQueue = new(); | ||
| private readonly Dictionary<ReminderId, int> _failureCounts = []; | ||
| private readonly Dictionary<ReminderId, int> _skipCounts = []; | ||
|
|
||
| public ReminderManagerActor( | ||
| ISessionPipeline pipeline, | ||
|
|
@@ -59,7 +64,8 @@ public ReminderManagerActor( | |
| TimeProvider timeProvider, | ||
| ReminderDefinitionStore definitionStore, | ||
| ReminderHistoryStore historyStore, | ||
| IOperationalNotificationSink notificationSink) | ||
| IOperationalNotificationSink notificationSink, | ||
| IReminderChannelNotifier channelNotifier) | ||
| { | ||
| _pipeline = pipeline; | ||
| _defaults = defaults; | ||
|
|
@@ -68,6 +74,7 @@ public ReminderManagerActor( | |
| _definitionStore = definitionStore; | ||
| _historyStore = historyStore; | ||
| _notificationSink = notificationSink; | ||
| _channelNotifier = channelNotifier; | ||
| _log = Context.GetLogger(); | ||
|
|
||
| ReceiveAsync<SaveReminderCommand>(HandleSaveAsync); | ||
|
|
@@ -83,6 +90,7 @@ public ReminderManagerActor( | |
|
|
||
| ReceiveAsync<ReconcileReminders>(_ => HandleReconcileAsync()); | ||
| Receive<GetReminderHealthQuery>(_ => HandleGetHealth()); | ||
| ReceiveAsync<GetReminderStatusQuery>(HandleGetStatusAsync); | ||
| } | ||
|
|
||
| protected override void PreStart() | ||
|
|
@@ -417,6 +425,7 @@ private async Task<ReminderStateResponse> DisableReminderInternalAsync(ReminderI | |
| await CancelScheduleOnlyAsync(id); | ||
|
|
||
| _failureCounts.Remove(id); | ||
| _skipCounts.Remove(id); | ||
| RemoveFromDeferredQueue(id); | ||
|
|
||
| _log.Info("Disabled reminder '{0}'", id.Value); | ||
|
|
@@ -433,6 +442,7 @@ private async Task DeleteReminderInternalAsync(ReminderId id) | |
| _definitionStore.Delete(id); | ||
| await CancelScheduleOnlyAsync(id); | ||
| _failureCounts.Remove(id); | ||
| _skipCounts.Remove(id); | ||
| RemoveFromDeferredQueue(id); | ||
|
|
||
| try | ||
|
|
@@ -576,8 +586,7 @@ private async Task HandleReminderFiredAsync(ReminderEnvelope<ReminderPayload> 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 +630,44 @@ private async Task HandleReminderFiredAsync(ReminderEnvelope<ReminderPayload> en | |
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// 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 <c>netclaw reminder status</c> so the silent-skip pattern from | ||
| /// #1492/#1494 (49 skips in a day, unnoticed) is now visible to operators. | ||
| /// </summary> | ||
| 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); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// 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. | ||
| /// </summary> | ||
| 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 +706,19 @@ private async Task HandleExecutionCompletedAsync(ReminderExecutionCompleted comp | |
| ["error"] = completed.ErrorMessage ?? "unknown", | ||
| })); | ||
|
|
||
| // Surface the failure where the operator expects this reminder's | ||
| // 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( | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM |
||
| definition, | ||
| $"Reminder \"{title}\" failed: {completed.ErrorMessage ?? "unknown error"}"); | ||
| } | ||
|
|
||
| if (count >= FailurePauseThreshold) | ||
| { | ||
| _log.Warning("Reminder '{0}' hit failure threshold ({1}), disabling", | ||
|
|
@@ -679,6 +739,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 +871,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 +1053,50 @@ 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(new ReminderStatusResponse( | ||
| query.Id, Found: false, Enabled: false, Executing: false, | ||
| NextFire: null, ConsecutiveFailures: 0, SkippedDuplicates: 0, | ||
| RecentHistory: [])); | ||
| return; | ||
| } | ||
|
|
||
| // 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: schedulesTask.Result.GetValueOrDefault(query.Id.Value), | ||
| ConsecutiveFailures: _failureCounts.GetValueOrDefault(query.Id), | ||
| SkippedDuplicates: _skipCounts.GetValueOrDefault(query.Id), | ||
| 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(new Status.Failure(ex)); | ||
| } | ||
| } | ||
|
|
||
| private sealed record ScheduleAttempt(bool IsSuccess, DateTimeOffset? NextFire, string? ErrorMessage) : INoSerializationVerificationNeeded | ||
| { | ||
| public static ScheduleAttempt Ok(DateTimeOffset? nextFire) => new(true, nextFire, null); | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM