fix(notifications): Notify.Send enqueues unbounded (maxRetries 0) and defers to sweep — no 30s script-thread block, no stranding

This commit is contained in:
Joseph Doherty
2026-07-08 20:56:09 -04:00
parent e2944d201c
commit 4bdd5f0e4a
4 changed files with 36 additions and 3 deletions
+1 -1
View File
@@ -132,7 +132,7 @@ Other peers in the `scadaproj` family (see `scadaproj/CLAUDE.md` for details): `
- Notification Service: SMTP with OAuth2 Client Credentials (Microsoft 365) or Basic Auth. BCC delivery, plain text.
- Notification delivery is central-only: sites store-and-forward notifications to the central cluster (target = central, not SMTP); sites never talk to SMTP. Notification lists and SMTP config are no longer deployed to sites; recipient resolution happens at central, at delivery time.
- Notification lists carry a `Type` discriminator (`Email` and `Sms`). `Notify.To("list")` is type-agnostic; delivery is via per-type `INotificationDeliveryAdapter` (Email via SMTP; Sms via Twilio REST — `SmsNotificationDeliveryAdapter`, no SDK, one POST per recipient, per-recipient rollup). List Type is fixed after creation.
- `Notify.Send` is async — returns a `NotificationId` (GUID, idempotency key) status handle immediately. `Notify.Status(notificationId)` returns a status record (status, retry count, last error, key timestamps); answered site-locally as `Forwarding` while still in the site S&F buffer, otherwise round-trips to central.
- `Notify.Send` is async and **enqueue-only** — it buffers the notification into the local SQLite S&F store and returns a `NotificationId` (GUID, idempotency key) status handle immediately; it never runs the forwarder's central Ask inline on the script thread (`deferToSweep: true` buffers due-immediately + kicks a background sweep), so its worst-case latency is the local insert whether central is up or down. It enqueues with `maxRetries: 0` (the "no limit" escape hatch), so notifications retry until central acks and are **never parked for retry exhaustion** — only a corrupt payload parks them (arch-review 02, Tasks 13/14). `Notify.Status(notificationId)` returns a status record (status, retry count, last error, key timestamps); answered site-locally as `Forwarding` while still in the site S&F buffer, otherwise round-trips to central.
- SMS delivery adapter (T9/T10, 2026-06-19): `SmsNotificationDeliveryAdapter` — Twilio REST, no SDK, HTTP Basic auth (`AccountSid:AuthToken`), one `POST .../Messages.json` per recipient. `SmsConfiguration` entity (`AccountSid`, `AuthToken` encrypted via Data Protection, `FromNumber`/`MessagingServiceSid`, `ApiBaseUrl`, timeout/retry) stored centrally; managed Admin-only via CLI `notification sms list|update` + Central UI `/notifications/sms`. Central-only (never deployed to sites). Per-recipient rollup: all-accepted → Delivered; any-transient → retry/park; mix → delivered-to-good + note; all-permanent → Park. `SmsConfiguration` (Auth Token in SecretsBlock) travels in Transport bundles via `--sms-configs`.
- Inbound API: `POST /api/{methodName}`, `X-API-Key` header, flat JSON, extended type system (Object, List).
@@ -49,9 +49,9 @@ flowchart TD
class I bad
```
For notifications, "delivery" means forwarding the message to the central cluster via CentralSite Communication; "success" is central's ack, on which the message is cleared. Notifications are retried at the fixed forward interval until central acks, but — like every other category — they are bounded by the engine's `DefaultMaxRetries` cap: a sustained central outage that exceeds `DefaultMaxRetries × forward-interval` will park the buffered notification, after which an operator can Retry/Discard it via the parked-message UI. Operationally, the cap is sized so the normal central-recovery window stays well inside it; "do not park" is the design's operational intent on the happy path, not an absolute invariant. Callers that genuinely require unbounded retry pass `maxRetries: 0` on `EnqueueAsync` (the documented "no limit" escape hatch — see `StoreAndForward-015`).
For notifications, "delivery" means forwarding the message to the central cluster via CentralSite Communication; "success" is central's ack, on which the message is cleared. `Notify.Send` is **enqueue-only**: it buffers the notification into the local SQLite S&F store and returns the `NotificationId` immediately — it never runs the forwarder's central Ask inline on the script thread (it passes `deferToSweep: true`, which buffers the row due-immediately and kicks a background sweep). Its worst-case latency is therefore the local SQLite insert, whether central is up or down; the actual forward happens on the sweep. `Notify.Send` also enqueues with `maxRetries: 0` — the documented "no limit" escape hatch (`StoreAndForward-015`) — so notifications are retried at the fixed forward interval until central acks and are **never parked for retry exhaustion**: a sustained central outage can no longer strand a notification behind per-message operator unparking. ("do not park" is thus the real behaviour for retry-exhaustion, not merely a happy-path aspiration — but see the two remaining parking causes below.)
There are now exactly two ways a notification parks. First, a **corrupt buffered payload** (a row whose stored JSON no longer deserialises to a `NotificationSubmit`, or deserialises to null): the forwarder returns the delivery-handler contract's permanent-failure signal (`false`) so the engine parks the row, preserving the payload for operator forensics. This **supersedes `StoreAndForward-018`**, which discarded such rows by reporting them as delivered — a silent data loss with no parked row, no central `Notifications` row, and no audit trail. Retrying a corrupt payload is pointless (it can never deserialise), so parking, not retrying, is the correct home for it. Second, retry exhaustion under the `DefaultMaxRetries` cap as described above — and once a `Notify.Send` caller passes `maxRetries: 0` (its unbounded-retry contract), that second cause is off, leaving corrupt payload as the only parking cause for those notifications.
There are now exactly two ways a notification parks. First, a **corrupt buffered payload** (a row whose stored JSON no longer deserialises to a `NotificationSubmit`, or deserialises to null): the forwarder returns the delivery-handler contract's permanent-failure signal (`false`) so the engine parks the row, preserving the payload for operator forensics. This **supersedes `StoreAndForward-018`**, which discarded such rows by reporting them as delivered — a silent data loss with no parked row, no central `Notifications` row, and no audit trail. Retrying a corrupt payload is pointless (it can never deserialise), so parking, not retrying, is the correct home for it. Second, retry exhaustion under the `DefaultMaxRetries` cap — but because `Notify.Send` enqueues with `maxRetries: 0` (its unbounded-retry contract, above), that second cause is **off for script-produced notifications**, leaving corrupt payload as their only parking cause. (Retry-exhaustion parking remains reachable only for a hypothetical notification enqueued with a positive `maxRetries`.)
For the cached-call categories (`ExternalCall` and `DatabaseWrite`), the operation tracking table is the status record and the S&F buffer is purely the retry mechanism. A cached call that succeeds on its first immediate attempt is written directly as a terminal `Delivered` tracking row and never enters the S&F buffer. When immediate delivery fails transiently, the message is buffered and its tracking row moves to `Pending`/`Retrying`; the buffered message carries its `TrackedOperationId` so the tracking row and the retry record stay linked. When immediate delivery fails **permanently** (e.g. HTTP 4xx), the message is not buffered — the error is returned synchronously to the calling script as before — but the tracking row is written directly as a terminal `Failed` row capturing the error. On every tracking-table status transition the site emits `CachedCallTelemetry` to central.
@@ -2199,6 +2199,16 @@ public class ScriptRuntimeContext
target: _listName,
payloadJson: payloadJson,
originInstanceName: _instanceName,
// 0 = the documented "no limit" escape hatch (StoreAndForward-015):
// notifications are retried until central acks and are never parked
// for retry exhaustion — a long central outage must not strand them
// behind per-message operator unparking. (Corrupt payloads still
// park — Task 14.)
maxRetries: 0,
// Never run the forwarder's 30s central Ask inline on the script
// thread: buffer due-immediately and kick the sweep. Send returns
// in milliseconds whether central is up or down.
deferToSweep: true,
messageId: notificationId);
_logger.LogDebug(
@@ -317,4 +317,27 @@ public class NotifyHelperTests : TestKit, IAsyncLifetime, IDisposable
// Not at central, not in the site buffer → genuinely unknown, NOT Forwarding.
Assert.Equal("Unknown", status.Status);
}
[Fact]
public async Task NotifySend_BuffersUnbounded_AndNeverInvokesForwarderInline()
{
// Task 13: Notify.Send must buffer with maxRetries: 0 (never park for retry
// exhaustion) and deferToSweep: true (never run the forwarder's central Ask
// inline on the scarce script thread). A central outage must return in
// milliseconds, and the row must not strand behind operator unparking.
var inlineInvoked = false;
_saf.RegisterDeliveryHandler(StoreAndForwardCategory.Notification,
_ => { inlineInvoked = true; throw new TimeoutException("central down"); });
var commProbe = CreateTestProbe();
var notify = CreateHelper(commProbe.Ref);
var notificationId = await notify.To("ops-list").Send("subject", "hello");
Assert.False(inlineInvoked); // pre-fix: Send ran the forwarder Ask inline
var row = await _saf.GetMessageByIdAsync(notificationId);
Assert.NotNull(row);
Assert.Equal(0, row!.MaxRetries); // pre-fix: DefaultMaxRetries → parked after a bounded outage
Assert.Equal(StoreAndForwardMessageStatus.Pending, row.Status);
}
}