From 4bdd5f0e4a29e1c8ada92345e78db357da4b5869 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 8 Jul 2026 20:56:09 -0400 Subject: [PATCH] =?UTF-8?q?fix(notifications):=20Notify.Send=20enqueues=20?= =?UTF-8?q?unbounded=20(maxRetries=200)=20and=20defers=20to=20sweep=20?= =?UTF-8?q?=E2=80=94=20no=2030s=20script-thread=20block,=20no=20stranding?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CLAUDE.md | 2 +- .../requirements/Component-StoreAndForward.md | 4 ++-- .../Scripts/ScriptRuntimeContext.cs | 10 ++++++++ .../Scripts/NotifyHelperTests.cs | 23 +++++++++++++++++++ 4 files changed, 36 insertions(+), 3 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 436b7afa..6a4cdc0e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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). diff --git a/docs/requirements/Component-StoreAndForward.md b/docs/requirements/Component-StoreAndForward.md index 4e14996f..2aa69b32 100644 --- a/docs/requirements/Component-StoreAndForward.md +++ b/docs/requirements/Component-StoreAndForward.md @@ -49,9 +49,9 @@ flowchart TD class I bad ``` -For notifications, "delivery" means forwarding the message to the central cluster via Central–Site 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 Central–Site 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. diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRuntimeContext.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRuntimeContext.cs index 7654b5a5..e75f8d7f 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRuntimeContext.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRuntimeContext.cs @@ -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( diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/NotifyHelperTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/NotifyHelperTests.cs index 99fa1130..8b1f7f4f 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/NotifyHelperTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/NotifyHelperTests.cs @@ -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); + } }