fix(services): notify's run summary stops claiming a delivery that dead-lettered (#7747) - #7875
Conversation
…ad-lettered (#7747) 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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGwDLmaML1LtLmQ4F4Aq7z
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 2 package(s): 4 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 2 release-owned page(s) also reference the affected code. These are read-only:
|
Fixes #7747
The finding, confirmed
Boot without the
pushchannel registered, fire a flow whose notify node targets['push'], and the two records an operator can read contradict each other:sys_notification_deliverystatus: 'dead',error: "channel 'push' not registered"status: 'success',acted: 1Nothing was delivered, and the surface built to answer "did this sweep actually do anything" (#4354) said it had.
Root cause — located, and sharper than the dispatch hypothesis
The dispatch guessed
notify-node.ts:293(metrics: { acted: Number(result.delivered) || 0 }) was the seam. That line is where the number is consumed; the defect is where it is minted.MessagingService.emit()has two paths, and they disagreed about whatEmitResult.deliveredmeans:fanOut()has each channel's answer beforeemit()returns. An unregistered channel is alreadyok: false, sodeliveredwas a real terminal count. This path never had the bug.enqueueDeliveries()returns once the(recipient × channel)rows are durable; the dispatcher sends and decides the outcome afterwards.deliveredcounted those enqueued rows anyway. Its own docstring said so — "hereokmeans 'accepted for delivery' (enqueued), not yet delivered" — and the pin atmessaging-service.test.ts:311wrote the conflation down verbatim:expect(result.delivered).toBe(2); // 2 enqueued (accepted).So
actedwas minted before any send attempt, from a count labelleddeliveredthat meantenqueued, and nothing ever revisits it — not stale by a moment, never revised at all. The dispatcher then dead-letters thepushrow (dispatcher.ts:210/:251), and the summary keeps sayingacted: 1.The fix
1.
EmitResultseparates the two counts (messaging-service.ts)deliverednow means a channel accepted the delivery — terminal and observed. Only the inline path can report it non-zero.enqueuedcarries the outbox path's accepted rows: durable, unsent, outcome pending onsys_notification_delivery.2. The notify node counts only what was delivered toward
acted(notify-node.ts)When deliveries are merely enqueued it reports
unmeasuredEffectinstead — the qualifier aconnector_actionalready uses for an effect the platform cannot count.Why that and not the alternatives the dispatch offered:
acted: 0(option a). The codebase rules this out explicitly — "neveracted: 0, which would claim it did nothing" (connector.zod.ts:613). It would trip the broken-sweep alert on every healthy outbox-backed notify, which is exactly what the#4354comment atnotify-node.ts:290was defending against.acted: 1, surface the outcome alongside" (option b).acted_countis a column and ahighlightFieldonsys_automation_run, and the documented alert reads it directly. Leaving the overstatement in the column and annotating it elsewhere leaves the lie where the operator actually looks.unmeasuredis the platform's existing word for this exact shape. The alert isselected > 0 AND acted = 0 AND unmeasured = 0, so a pending delivery suppresses the alert without asserting success — andformatRunSummaryLinealready printsunmeasured=with the comment "acted=0on a line that also saysunmeasured=3means 'cannot tell', not 'did nothing'". No fourth counter invented.Node output gains
enqueuednext todeliveredandnotificationId, so the id has a stated reason to be followed into the delivery record.Status stays
success. The flow did everything it can do synchronously, and failing it would let a channel registered a moment later retroactively break the flow (messaging-service-plugin.ts:212documents that late registration is supported). No synchronous wait on async delivery was added — notify must not block a flow on a downstream channel, so "delivered" is not a claim it is ever positioned to make. It simply stops making it.Acceptance criterion
notify-delivery-outcome.integration.test.tswires the realMessagingService+MemoryNotificationOutbox+NotificationDispatcherbehind the notify node and asserts on the two durable records — the folded run summary and the outbox row — never on internal call counts. A fake that answersemit()in one shot cannot express the disagreement at all, because the defect lives in the seam between enqueue and dispatch.Four cases, covering both directions so the fix can't degenerate into "unregistered channels are special":
pushunregisteredacted: 0, unmeasured: 1dead,"channel 'push' not registered"inboxregisteredacted: 0, unmeasured: 1pending→successafter tickacted: 1, unmeasured: 0pushunregisteredacted: 0, unmeasured: 0(a measured zero — correctly alert-eligible)Reverse-verified on
origin/main: the durable-record assertions pass (the dead-letter is real) while the summary assertion fails withacted: 1, unmeasured: 0— the finding exactly. 2 of 4 red before, 4/4 green after.Pins
messaging-service.test.ts:311— updated deliberately:delivered: 2 // 2 enqueued (accepted)→enqueued: 2, delivered: 0. That comment was the conflation; the pin now states the distinction.messaging-service.test.ts:102(inline) — extended withenqueued: 0, keepingdeliveredterminal on both paths.connector-nodes.test.ts:292— unaffected and green. It pins connector accounting (acted: 1, unmeasured: 0), not notify's.EmitResult/.deliveredfinds only these tests and the notify node. The other twomessaging.emit()callers (plugin-approvals,plugin-audit)awaitwithout reading the result. No docs referencenotify.deliveredorEmitResult.Gates
--filter "@objectstack/service-automation...")service-automationsuiteservice-messagingsuitepnpm check:docs-audit-scopepackages/spec/src/**is not touched, sogen:schema/gen:docsdo not apply. Engine-double gate is N/A — the new test adds no data-engine fake with update/delete verbs (it drives the real messaging stack; its only fake is a send-only channel).Note for the backlog (not fixed here)
http-nodes.ts:139reports{ output: { deliveryId, enqueued: true }, metrics: { acted: 1 } }for an outbox-enqueued HTTP delivery — the same class of overstatement againstsys_http_delivery, which has its owndeadstatus. Left alone as out of scope for #7747; worth its own card.Generated by Claude Code