Skip to content

Commit f28ef3b

Browse files
huangyiireneclaude
andauthored
fix(services): notify's run summary stops claiming a delivery that dead-lettered (#7747) (#7875)
A stack booted without the `push` channel registered, running a flow whose notify node targets `['push']`, produced two operator-facing records that contradicted each other: `sys_notification_delivery` held `status: 'dead'`, `error: "channel 'push' not registered"`, while the flow-run summary reported `status: 'success', acted: 1`. The seam is `EmitResult.delivered`. With the durable outbox in play (ADR-0030 P1), `emit()` returns as soon as the `(recipient x channel)` rows are enqueued and the dispatcher decides the outcome afterwards — but `delivered` counted those enqueued rows under a name that says they arrived, and `notify` fed the number straight into `acted`. A count minted before any send attempt then survived the dead-letter unrevised; nothing ever revisits it. - `EmitResult` separates the two counts. `delivered` now means a channel ACCEPTED the delivery — terminal and observed, which only the inline (P0) fan-out can report. New `enqueued` carries the outbox path's accepted rows: durable, unsent, outcome pending on `sys_notification_delivery`. - The notify node counts only delivered toward `acted`, and reports `unmeasuredEffect` when deliveries are merely enqueued — the qualifier a `connector_action` already uses for an effect the platform cannot count, and deliberately not a bare `acted: 0`, which would claim the run did nothing. The broken-sweep alert is `selected > 0 AND acted = 0 AND unmeasured = 0`, so a pending delivery suppresses the alert without asserting success. Node output gains `enqueued` next to `delivered` and `notificationId`. The run still reports `success`: the flow did everything it can do synchronously, and failing it would let a channel registered a moment later retroactively break the flow. Notify must not block on a downstream channel, so "delivered" is not a claim it is ever positioned to make — it simply stops making it. Tests wire the REAL MessagingService + NotificationDispatcher behind the notify node and assert on the two durable records (folded run summary, outbox row), not on call counts — the finding is that those records disagree. Reverse- verified: on origin/main the durable assertions pass and the summary asserts `acted: 1, unmeasured: 0`. Pin updated deliberately: `messaging-service.test.ts` asserted `delivered: 2 // 2 enqueued (accepted)` — the conflation written down — now `enqueued: 2, delivered: 0`. `connector-nodes.test.ts:292` is unaffected (it pins connector, not notify, accounting) and stays green. Claude-Session: https://claude.ai/code/session_01LGwDLmaML1LtLmQ4F4Aq7z Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2ff87a2 commit f28ef3b

7 files changed

Lines changed: 345 additions & 11 deletions

File tree

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
---
2+
"@objectstack/service-messaging": patch
3+
"@objectstack/service-automation": patch
4+
---
5+
6+
fix(services): a notify flow-run summary no longer reports a delivery the delivery record dead-lettered (#7747)
7+
8+
Boot a stack without the `push` channel registered, fire a flow whose `notify`
9+
node targets `['push']`, and the two records an operator can read **contradicted
10+
each other**: `sys_notification_delivery` held `status: 'dead'`,
11+
`error: "channel 'push' not registered"`, while the flow-run summary said
12+
`status: 'success', acted: 1`. Nothing was delivered, and the surface built to
13+
answer "did this sweep actually do anything" (#4354) said it had.
14+
15+
The seam is `EmitResult.delivered`. With the durable outbox in play (ADR-0030
16+
P1), `emit()` returns as soon as the `(recipient × channel)` rows are enqueued —
17+
the dispatcher sends and decides the outcome afterwards — but `delivered`
18+
counted those *enqueued* rows anyway, under a name that says they arrived. The
19+
`notify` node then fed that number straight into `acted`, so a count minted
20+
before any send attempt survived unrevised through the dead-letter. It was never
21+
a "stale by a moment" number either: nothing ever revisits it.
22+
23+
- `EmitResult` now separates the two. `delivered` means a channel **accepted**
24+
the delivery — a terminal, observed outcome, which only the inline (P0)
25+
fan-out can report. New `enqueued` carries the outbox path's accepted rows:
26+
durable, unsent, outcome pending on `sys_notification_delivery`.
27+
- The `notify` node counts only what was delivered toward `acted`. When
28+
deliveries are merely enqueued it reports `unmeasuredEffect` instead — the
29+
qualifier a `connector_action` already uses for an effect the platform cannot
30+
count, and deliberately **not** a bare `acted: 0`, which would claim the run
31+
did nothing. The broken-sweep alert is
32+
`selected > 0 AND acted = 0 AND unmeasured = 0`, so a pending delivery
33+
suppresses the alert without asserting success. The node's output gains
34+
`enqueued` alongside `delivered` and `notificationId`.
35+
36+
The run still reports `success`: the flow did everything it can do
37+
synchronously, and failing it would let a channel registered a moment later
38+
retroactively break the flow. Notify does not block a flow on a downstream
39+
channel, so "delivered" is not a claim it is ever in a position to make — what
40+
changes is that it no longer makes it. Inline (P0) fan-out is untouched: it has
41+
the channel's answer by the time `emit()` returns, so `acted` stays a real
42+
measurement there, including the measured zero for an unregistered channel.

packages/services/service-automation/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
"@objectstack/objectql": "workspace:*",
2929
"@objectstack/plugin-security": "workspace:*",
3030
"@objectstack/service-job": "workspace:*",
31+
"@objectstack/service-messaging": "workspace:*",
3132
"@types/node": "^26.1.2",
3233
"typescript": "^6.0.3",
3334
"vitest": "^4.1.10"
Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect } from 'vitest';
4+
import {
5+
MessagingService,
6+
MemoryNotificationOutbox,
7+
NotificationDispatcher,
8+
} from '@objectstack/service-messaging';
9+
import type { MessagingChannel } from '@objectstack/service-messaging';
10+
import { AutomationEngine } from '../engine.js';
11+
import { registerNotifyNode } from './notify-node.js';
12+
13+
/**
14+
* #7747 — the run summary an operator reads must not claim a delivery that
15+
* `sys_notification_delivery` records as dead.
16+
*
17+
* The QA repro verbatim: boot WITHOUT the `push` channel registered, fire a
18+
* flow whose notify node targets `['push']`, then read the run summary and the
19+
* delivery record. This wires the REAL `MessagingService` (outbox-backed, P1)
20+
* and the REAL `NotificationDispatcher` behind the notify node rather than a
21+
* fake, because the defect lives in the seam BETWEEN them: `emit()` returns
22+
* once the row is enqueued and the dispatcher decides the outcome afterwards,
23+
* so a fake that answers `emit()` in one shot cannot express the disagreement
24+
* at all.
25+
*
26+
* The assertions are deliberately on the two DURABLE operator-facing records —
27+
* the folded run summary and the outbox row — not on how many times anything
28+
* was called: the finding is precisely that those two records contradict each
29+
* other, so an internal call-count assertion would pass while the defect stands.
30+
*
31+
* On `origin/main` the first test fails with `acted: 1` — the notify node
32+
* counts `EmitResult.delivered`, which in outbox mode is an ENQUEUED count.
33+
*/
34+
35+
function silentLogger(): any {
36+
const l: any = { info: () => {}, warn: () => {}, error: () => {}, debug: () => {} };
37+
l.child = () => l;
38+
return l;
39+
}
40+
41+
/** A channel that records what it was handed, so a real send is distinguishable. */
42+
function recordingChannel(id: string): { channel: MessagingChannel; sent: unknown[] } {
43+
const sent: unknown[] = [];
44+
return {
45+
sent,
46+
channel: {
47+
id,
48+
async send(_ctx, delivery) {
49+
sent.push(delivery);
50+
return { ok: true };
51+
},
52+
},
53+
};
54+
}
55+
56+
/** Wire the notify node against a given messaging service. */
57+
function engineWith(messaging: MessagingService): AutomationEngine {
58+
const engine = new AutomationEngine(silentLogger());
59+
registerNotifyNode(engine, {
60+
logger: silentLogger(),
61+
getService: (name: string) => (name === 'messaging' ? messaging : undefined),
62+
} as any);
63+
return engine;
64+
}
65+
66+
/**
67+
* A stack booted the way the repro describes: messaging present and
68+
* outbox-backed (P1), with only the channels named here registered.
69+
*/
70+
function bootOutboxStack(registered: MessagingChannel[]) {
71+
const outbox = new MemoryNotificationOutbox(1);
72+
const messaging = new MessagingService({ logger: silentLogger(), outbox });
73+
for (const c of registered) messaging.registerChannel(c);
74+
75+
const dispatcher = new NotificationDispatcher({
76+
nodeId: 'node-test',
77+
outbox,
78+
channels: messaging,
79+
channelContext: { logger: silentLogger() },
80+
partitionCount: 1,
81+
intervalMs: 10_000, // ticks are driven manually
82+
});
83+
84+
return { outbox, messaging, dispatcher, engine: engineWith(messaging) };
85+
}
86+
87+
function notifyFlow(channels: string[]) {
88+
return {
89+
name: 'nudge',
90+
label: 'Nudge',
91+
type: 'autolaunched' as const,
92+
nodes: [
93+
{ id: 'start', type: 'start' as const, label: 'Start' },
94+
{
95+
id: 'notify',
96+
type: 'notify' as const,
97+
label: 'Notify',
98+
config: { recipients: ['user_1'], title: 'Renewal due', message: 'Ping', channels },
99+
},
100+
{ id: 'end', type: 'end' as const, label: 'End' },
101+
],
102+
edges: [
103+
{ id: 'e1', source: 'start', target: 'notify' },
104+
{ id: 'e2', source: 'notify', target: 'end' },
105+
],
106+
};
107+
}
108+
109+
describe('notify run summary vs. the durable delivery record (#7747)', () => {
110+
it('does not report a countable act for a delivery that dead-letters on an unregistered channel', async () => {
111+
// 1) Boot without the `push` channel registered.
112+
const { outbox, dispatcher, engine } = bootOutboxStack([recordingChannel('inbox').channel]);
113+
114+
// 2) Fire a flow whose notify node targets ['push'].
115+
engine.registerFlow('nudge', notifyFlow(['push']));
116+
const run = await engine.execute('nudge');
117+
118+
// 3a) The durable record: the dispatcher dead-letters the row, because
119+
// no transport for `push` exists.
120+
await dispatcher.tick();
121+
const rows = await outbox.list();
122+
expect(rows).toHaveLength(1);
123+
expect(rows[0].channel).toBe('push');
124+
expect(rows[0].status).toBe('dead');
125+
expect(rows[0].error).toContain("channel 'push' not registered");
126+
127+
// 3b) The record an operator reads. The run still SUCCEEDS — the flow
128+
// did everything it can do synchronously, and failing it would make
129+
// a channel that registers a moment later retroactively break the
130+
// flow. What must not survive is the claim that it DELIVERED:
131+
// `acted` is the count the broken-sweep alert trusts, and the honest
132+
// answer at the moment the run settles is "an effect I cannot count
133+
// yet" — which the platform already spells `unmeasured`, and which
134+
// is not the same as `acted: 0` alone (that would claim the run did
135+
// nothing, and trip the alert on every healthy notify).
136+
expect(run.success).toBe(true);
137+
expect(run.summary).toMatchObject({ acted: 0, unmeasured: 1 });
138+
139+
// The finding itself, as one assertion: the summary must not out-count
140+
// what the durable record shows was actually delivered (here: nothing).
141+
const notDead = rows.filter((r) => r.status !== 'dead').length;
142+
expect(run.summary!.acted).toBeLessThanOrEqual(notDead);
143+
});
144+
145+
it('reports the same uncountable effect for a channel that IS registered — the outcome is simply not known yet', async () => {
146+
// The counterpart that stops the fix from degenerating into "unregistered
147+
// channels are special": at the moment the run settles, a healthy
148+
// outbox-backed delivery is equally unsent. What separates the two cases
149+
// is the outbox row — which is exactly where `unmeasured` points.
150+
const inbox = recordingChannel('inbox');
151+
const { outbox, dispatcher, engine } = bootOutboxStack([inbox.channel]);
152+
153+
engine.registerFlow('nudge', notifyFlow(['inbox']));
154+
const run = await engine.execute('nudge');
155+
156+
expect(run.success).toBe(true);
157+
expect(run.summary).toMatchObject({ acted: 0, unmeasured: 1 });
158+
// Nothing had been sent when the run settled…
159+
expect(inbox.sent).toHaveLength(0);
160+
// …and the delivery lands afterwards, on the record that owns the truth.
161+
await dispatcher.tick();
162+
expect(inbox.sent).toHaveLength(1);
163+
expect((await outbox.list())[0].status).toBe('success');
164+
});
165+
166+
it('still reports a countable act when the messaging stack delivers inline (no outbox)', async () => {
167+
// The inline (P0) path really does know the outcome by the time `emit()`
168+
// returns, so `acted` stays a measurement there — the fix narrows what
169+
// `acted` may claim, it does not blanket every notify as unmeasurable.
170+
const inbox = recordingChannel('inbox');
171+
const messaging = new MessagingService({ logger: silentLogger() });
172+
messaging.registerChannel(inbox.channel);
173+
const engine = engineWith(messaging);
174+
175+
engine.registerFlow('nudge', notifyFlow(['inbox']));
176+
const run = await engine.execute('nudge');
177+
178+
expect(inbox.sent).toHaveLength(1);
179+
expect(run.summary).toMatchObject({ acted: 1, unmeasured: 0 });
180+
});
181+
182+
it('an inline send to an unregistered channel is a measured zero, not an unmeasured shrug', async () => {
183+
// Inline fan-out DOES observe "channel not registered" synchronously, so
184+
// that run is correctly eligible for the broken-sweep alert.
185+
const messaging = new MessagingService({ logger: silentLogger() });
186+
messaging.registerChannel(recordingChannel('inbox').channel);
187+
const engine = engineWith(messaging);
188+
189+
engine.registerFlow('nudge', notifyFlow(['push']));
190+
const run = await engine.execute('nudge');
191+
192+
expect(run.summary).toMatchObject({ acted: 0, unmeasured: 0 });
193+
});
194+
});

packages/services/service-automation/src/builtin/notify-node.ts

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,23 @@ export interface MessagingServiceSurface {
2626
source?: { object: string; id: string };
2727
actorId?: string;
2828
channels?: string[];
29-
}): Promise<{ notificationId: string; delivered: number; failed: number }>;
29+
}): Promise<{
30+
notificationId: string;
31+
/** Deliveries a channel ACCEPTED — a terminal, observed outcome. */
32+
delivered: number;
33+
failed: number;
34+
/**
35+
* Deliveries durably accepted into the messaging outbox but NOT yet
36+
* attempted; their real outcome lands on `sys_notification_delivery`
37+
* afterwards (#7747).
38+
*
39+
* Optional because this is a STRUCTURAL mirror of a service resolved at
40+
* runtime: an older or third-party messaging implementation may not
41+
* report it, and absent reads as "nothing is in flight" — which is the
42+
* only answer such a stack could honestly give.
43+
*/
44+
enqueued?: number;
45+
}>;
3046
}
3147

3248
/**
@@ -279,18 +295,46 @@ export function registerNotifyNode(engine: AutomationEngine, ctx: PluginContext)
279295
actorId,
280296
channels: channels.length ? channels : undefined,
281297
});
298+
const delivered = Number(result.delivered) || 0;
299+
const enqueued = Number(result.enqueued) || 0;
282300
return {
283301
success: true,
284302
output: {
285303
notificationId: result.notificationId,
286-
delivered: result.delivered,
304+
delivered,
305+
// Surfaced so a flow author templating the outcome can
306+
// tell "sent" from "handed to the outbox", and so the
307+
// notification id above has a stated reason to be
308+
// followed into `sys_notification_delivery`.
309+
enqueued,
287310
failed: result.failed,
288311
},
289312
// A notification IS the action for a nudge/alert sweep, so it
290313
// counts toward `acted` (#4354) — otherwise the flow whose
291314
// whole job is to notify would report acting on nothing, and
292315
// the broken-sweep detector would fire on every healthy run.
293-
metrics: { acted: Number(result.delivered) || 0 },
316+
//
317+
// But only a delivery a channel ACCEPTED is countable. With
318+
// the outbox in play (ADR-0030 P1) `emit()` returns once the
319+
// rows are durable and the dispatcher decides the outcome
320+
// afterwards — including dead-lettering an unregistered
321+
// channel — so counting the enqueue as `acted` made the run
322+
// summary assert a delivery that `sys_notification_delivery`
323+
// recorded as `dead` (#7747). The honest answer at the moment
324+
// the run settles is "an effect I cannot count yet", which is
325+
// exactly `unmeasuredEffect` — the same qualifier a
326+
// `connector_action` uses, and pointedly NOT a bare
327+
// `acted: 0`, which would claim the run did nothing and trip
328+
// the broken-sweep alert on every healthy notify. The alert
329+
// is `selected > 0 AND acted = 0 AND unmeasured = 0`, so a
330+
// pending delivery correctly suppresses it while refusing to
331+
// claim success.
332+
//
333+
// Waiting for the real outcome is not on the table: a notify
334+
// node must not block a flow on a downstream channel.
335+
metrics: enqueued > 0
336+
? { ...(delivered > 0 ? { acted: delivered } : {}), unmeasuredEffect: true }
337+
: { acted: delivered },
294338
};
295339
} catch (err) {
296340
return { success: false, error: `notify failed: ${(err as Error).message}` };

packages/services/service-messaging/src/messaging-service.test.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,10 @@ describe('MessagingService', () => {
101101
expect(inbox.seen[0].notification.title).toBe('Deal closed');
102102
expect(result.delivered).toBe(2);
103103
expect(result.failed).toBe(0);
104+
// Inline fan-out leaves nothing in flight — the counterpart to the
105+
// outbox pin below, and what keeps `delivered` a terminal count on
106+
// BOTH paths rather than a name two things share (#7747).
107+
expect(result.enqueued).toBe(0);
104108
expect(result.notificationId).toMatch(/^evt_/); // synthesized w/o data layer
105109
expect(result.deliveries[0]).toMatchObject({ channel: 'inbox', recipient: 'user_1', ok: true, externalId: 'row_1' });
106110
});
@@ -308,7 +312,14 @@ describe('MessagingService', () => {
308312

309313
// Nothing sent inline — the dispatcher owns the send.
310314
expect(inbox.seen).toHaveLength(0);
311-
expect(result.delivered).toBe(2); // 2 enqueued (accepted)
315+
// …so nothing is DELIVERED yet, and the result says so (#7747). This
316+
// pin used to read `delivered: 2` with the comment "2 enqueued
317+
// (accepted)" — the conflation itself, written down: callers were
318+
// handed an enqueue count under the name `delivered`, and it stayed
319+
// put when the dispatcher later dead-lettered the row.
320+
expect(result.enqueued).toBe(2);
321+
expect(result.delivered).toBe(0);
322+
expect(result.failed).toBe(0);
312323
const rows = await outbox.list();
313324
expect(rows).toHaveLength(2);
314325
expect(rows.every((r) => r.status === 'pending')).toBe(true);

0 commit comments

Comments
 (0)