Skip to content

finding: a notify flow-run summary reports success/acted:1 while the delivery dead-lettered — the dead-letter is absent from the run summary #7747

Closed

Description

@huangyiirene

Symptom

A ['push'] notification probe's flow-run summary reports status:'success', acted:1 while nothing was delivered. The dead-letter is visible in sys_notification_delivery (the push channel is unregistered — status:'dead', error: "channel 'push' not registered") but not in the flow-run summary — so an operator reading the run summary sees a successful action that in fact delivered nothing.

Root cause

Not located by the run. The flow-run summary's acted count (the selected/acted/skipped surfacing added by #4354) treats the notify node as acted on enqueue and does not reconcile against the delivery outcome recorded in sys_notification_delivery, so a dead-lettered delivery is still counted as a successful act. Split from the run's "Notes worth keeping" #6 as a real observability defect (the summary an operator reads disagrees with the durable delivery record). Fix spans the notify-node acted accounting and the delivery-outcome reconciliation (packages/services automation/messaging).

Stale-premise check: present on origin/main (the run summary does not reflect delivery dead-letters).

Reproduction

  1. Boot without the push channel registered.
  2. Fire a flow whose notify node targets ['push'].
  3. Read the flow-run summary, then sys_notification_delivery.

Expected: the run summary reflects the failed/dead-lettered delivery (not acted:1 success). Actual: run summary status:'success', acted:1; sys_notification_delivery shows the delivery dead-lettered.

Source

Extracted from the QA run #7690 (framework 92f26f7, console 09987b6).

Activity

  1. added
    pm:queueReady for the PM dispatch loop
    domain:servicesRetired 2026-10-22, alias of domain:server (services surface); in-flight cards keep it until landed
    findingRecorded observation — held, not dispatchable until the findings triage round grades it
    on Aug 11, 2026
  2. removed
    findingRecorded observation — held, not dispatchable until the findings triage round grades it
    on Aug 11, 2026
  3. claude commented on Aug 11, 2026

    @claude
    Contributor

    Findings triage (grade): promoted — finding removed, pm:queue stays (resolves the filed dual pm-state).

    Why queue: an operator-facing record that contradicts the durable record is a real observability defect, and both sides are named: the flow-run summary's acted accounting (the #4354 surfacing — e.g. service-automation/src/builtin/connector-nodes.test.ts:292 pins summary.acted) counts the notify node on enqueue, while the delivery outcome dead-letters in sys_notification_delivery (status:'dead', unregistered channel) with no reconciliation between them. Root cause is not yet pinned to a line, but the repro and the two disagreeing records make this dispatchable — locating the accounting seam is the first step of the fix, not a blocker to queueing it.

    本评论来自分诊座位 Routine(#5474 试点),不构成认领。


    Generated by Claude Code

  4. added
    pm:dispatchedDispatched to a dev agent by /pm-dispatch
    and removed
    pm:queueReady for the PM dispatch loop
    on Aug 12, 2026
  5. self-assigned this
    on Aug 12, 2026
  6. huangyiirene commented on Aug 12, 2026

    @huangyiirene
    CollaboratorAuthor

    Claim: PM loop round 3
    Session: session_01JVfWSQN9RDDGwdVSmMc29x
    Branch: claude/issue-7747-notify-acted-dead-letter
    Worktree: cloud session's own clone (mode:cloud)
    Domain: domain:services
    File surface: packages/services/service-automation/src/builtin/notify-node.ts (+ its test), and whatever the delivery-outcome read requires within packages/services.
    Container & model: M (observability semantics — what acted should mean against an async delivery outcome), mode:cloud, model: opus
    Serial constraints cleared: ✅ same-package predecessor #7742 (PR #7847) MERGED — serialization released; that diff touched plugin.ts's connector reconcile, not the notify node, so no interaction.

    PM landing-site read (30-second grep, card said "not located"): notify-node.ts:264 sets metrics: { acted: 0 } and :293 sets metrics: { acted: Number(result.delivered) || 0 }, with the #4354 comment at :290 explaining the intent. So acted counts what the notify call reported as delivered at enqueue, and a channel that dead-letters afterwards (sys_notification_delivery.status: 'dead', e.g. unregistered push) never revises it. That is the seam — the dispatch names it as a PM hypothesis to verify, not as fact.


    Generated by Claude Code

  7. huangyiirene commented on Aug 12, 2026

    @huangyiirene
    CollaboratorAuthor
    {
      "issue": 7747,
      "premise_still_valid": true,
      "status": "draft_pr_open",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/7875",
      "branch": "claude/issue-7747-notify-acted-dead-letter",
      "commit": "42d917d",
      "pm_hypothesis": {
        "verdict": "confirmed_and_sharpened",
        "pm_said": "notify-node.ts:293 `metrics: { acted: Number(result.delivered) || 0 }` records what the notify call reported delivered AT ENQUEUE, never revised after a dead-letter",
        "measured": "Correct in outcome, one level off in location. :293 CONSUMES the number; it is MINTED in MessagingService.emit(). `EmitResult.delivered` means two different things on the two emit paths: inline (P0) `fanOut()` has each channel's answer before emit() returns (an unregistered channel is already ok:false, delivered is terminal — this path never had the bug); outbox (P1, ADR-0030) `enqueueDeliveries()` counts ENQUEUED rows as `delivered` and the dispatcher decides the outcome afterwards. So `acted` is minted before any send attempt from a count labelled `delivered` that means `enqueued`, and nothing ever revisits it.",
        "corroborating_evidence_in_tree": [
          "messaging-service.ts:658 docstring: \"here `ok` means 'accepted for delivery' (enqueued), not yet delivered\"",
          "messaging-service.test.ts:311 pinned the conflation verbatim: `expect(result.delivered).toBe(2); // 2 enqueued (accepted)`",
          "dispatcher.ts:210 / :251 write the dead-letter (`dead: true`, \"channel '<c>' not registered\")"
        ]
      },
      "design_decision": {
        "chosen": "(c) — count only accepted deliveries toward `acted`; report a merely-enqueued delivery as `unmeasuredEffect`, the platform's existing qualifier for an effect it cannot count",
        "rationale": "The acceptance criterion is that the summary must not read as 'delivered' when sys_notification_delivery says 'dead'. `unmeasured` is already the platform's word for exactly this uncertainty and needs no new counter: the documented broken-sweep alert is `selected > 0 AND acted = 0 AND unmeasured = 0`, so a pending delivery suppresses the alert WITHOUT asserting success, and formatRunSummaryLine already prints `unmeasured=` with the comment that `acted=0` alongside it means 'cannot tell', not 'did nothing'.",
        "rejected_a_acted_zero": "Ruled out by the codebase itself — connector.zod.ts:613 'never `acted: 0`, which would claim it did nothing'. A blanket zero trips the broken-sweep alert on every healthy outbox-backed notify, which is precisely what the #4354 comment at notify-node.ts:290 was defending against.",
        "rejected_b_keep_acted_surface_outcome": "`acted_count` is a COLUMN and a highlightField on sys_automation_run and the documented alert reads it directly. Keeping the overstatement in the column and annotating it elsewhere leaves the lie where the operator actually looks.",
        "status_left_as_success": "Deliberate. The flow did everything it can do synchronously; failing it would let a channel registered a moment later retroactively break the flow (messaging-service-plugin.ts:212 documents late registration as supported). No synchronous wait on async delivery was added.",
        "blast_radius_note": "In outbox-backed stacks a notify run now reads `acted: 0, unmeasured: 1` where it read `acted: 1, unmeasured: 0`. That is the intended correction, and `unmeasured` keeps such runs out of the broken-sweep alert."
      },
      "changes": [
        "packages/services/service-messaging/src/messaging-service.ts — EmitResult gains `enqueued`; `delivered` narrowed to 'a channel ACCEPTED it' (terminal). Outbox path returns delivered:0 + enqueued:N; inline path returns enqueued:0. All 6 return sites updated.",
        "packages/services/service-automation/src/builtin/notify-node.ts — MessagingServiceSurface mirrors `enqueued?` (optional: structural mirror of a runtime-resolved service; absent reads as 'nothing in flight'). metrics: acted counts only delivered; unmeasuredEffect when enqueued > 0. Output gains `enqueued`.",
        "packages/services/service-automation/src/builtin/notify-delivery-outcome.integration.test.ts — NEW: the QA repro end-to-end.",
        "packages/services/service-automation/package.json + pnpm-lock.yaml — @objectstack/service-messaging as a TEST-ONLY devDependency (no cycle: messaging does not depend on automation; the notify node keeps its no-runtime-dep structural surface).",
        ".changeset/notify-acted-dead-letter.md — patch × 2."
      ],
      "test_evidence": {
        "asserts_on": "the two DURABLE operator-facing records — the folded run summary and the sys_notification_delivery/outbox row — never internal call counts",
        "why_real_stack": "wires the REAL MessagingService + MemoryNotificationOutbox + NotificationDispatcher behind the notify node; the defect lives in the seam BETWEEN enqueue and dispatch, so a fake answering emit() in one shot cannot express the disagreement at all",
        "cases": [
          "outbox + push unregistered → summary acted:0 unmeasured:1; row status 'dead' with \"channel 'push' not registered\"",
          "outbox + inbox registered → summary acted:0 unmeasured:1; row pending → 'success' after a dispatcher tick (stops the fix degenerating into 'unregistered channels are special')",
          "inline delivered → acted:1 unmeasured:0 (acted stays a measurement where the outcome IS known)",
          "inline + push unregistered → acted:0 unmeasured:0, a MEASURED zero, correctly alert-eligible"
        ],
        "reverse_verification": "On origin/main the durable-record assertions PASS (the dead-letter is real) while the summary assertion FAILS with `acted: 1, unmeasured: 0` — the finding exactly. 2 of 4 red before the fix, 4/4 green after."
      },
      "pins_reviewed": {
        "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": "extended with `enqueued: 0` so `delivered` is pinned terminal on both paths.",
        "connector-nodes.test.ts:292": "UNAFFECTED and green — it pins connector accounting (acted:1, unmeasured:0), not notify's.",
        "other_consumers": "none — repo-wide grep for EmitResult / .delivered finds only these tests and the notify node. plugin-approvals and plugin-audit await emit() without reading the result. No docs reference notify.delivered or EmitResult."
      },
      "gates": {
        "build_closure": "pass",
        "service_automation_suite": "pass — 77 files / 932 tests",
        "service_messaging_suite": "pass — 18 files / 206 tests",
        "check_docs_audit_scope": "pass",
        "changeset": "present (patch × 2, user-visible observability change)",
        "spec_regen": "n/a — packages/spec/src/** not touched",
        "engine_double_gate": "n/a — the new test adds no data-engine fake with update/delete verbs; its only fake is a send-only channel"
      },
      "open_questions": [
        {
          "q": "Worktree isolation: the designated branch was already checked out in this cloud container's primary clone, and `git checkout --` is blocked by the permission classifier, so the primary could not be moved off it.",
          "decided": "Created a proper dedicated worktree (/home/user/objectstack-7747) off origin/main under the local name `wt/issue-7747` and pushed it to the designated remote branch with an explicit refspec (`git push -u origin wt/issue-7747:claude/issue-7747-notify-acted-dead-letter`). The remote branch name and content are exactly as contracted; only the local branch label differs. The repo had no node_modules, so the worktree cost one pnpm install either way."
        },
        {
          "q": "Should `enqueued` be required on the notify node's MessagingServiceSurface?",
          "decided": "Required on EmitResult (the in-repo producer must be explicit), optional on the structural surface (a runtime-resolved, possibly third-party service; absent reads as 'nothing in flight', the only answer such a stack could honestly give)."
        }
      ],
      "followup_found_not_fixed": {
        "site": "packages/services/service-automation/src/builtin/http-nodes.ts:139",
        "detail": "`{ output: { deliveryId, enqueued: true }, metrics: { acted: 1 } }` for an outbox-enqueued HTTP delivery — the same class of overstatement against sys_http_delivery, which has its own `dead` status. Out of scope for #7747; worth its own card."
      }
    }

    Generated by Claude Code

  8. huangyiirene commented on Aug 12, 2026

    @huangyiirene
    CollaboratorAuthor

    ACCEPT — PR #7875 (services seat review, session session_01JVfWSQN9RDDGwdVSmMc29x).

    My dispatch hypothesis pointed at notify-node.ts:293; the dev confirmed the outcome and corrected the location one level down, which is the part that makes the fix right: :293 merely consumes the number — it is minted in MessagingService.emit(), where EmitResult.delivered means two different things on the two paths. Inline (P0) fanOut() has each channel's terminal answer before returning (that path never had the bug); outbox (P1, ADR-0030) enqueueDeliveries() counts enqueued rows as delivered and the dispatcher decides the outcome afterwards. So acted was minted before any send attempt, from a count labelled delivered that meant enqueued. The conflation was even pinned in-tree (messaging-service.test.ts:311, // 2 enqueued (accepted)) — that comment WAS the bug, and the PR turns the pin into a statement of the distinction.

    What shipped: EmitResult gains enqueued, delivered narrowed to "a channel accepted it" (terminal, both paths); notify counts only delivered toward acted and reports a merely-enqueued delivery as unmeasuredEffect.

    Review notes:

    • The design choice is the strongest part. unmeasured is the platform's existing word for this uncertainty, so the fix needs no new counter and — critically — keeps healthy outbox notifies out of the documented broken-sweep alert (selected > 0 AND acted = 0 AND unmeasured = 0) without asserting success. Option (a) blanket-zero was ruled out by the codebase's own rule (connector.zod.ts:613, "never acted: 0, which would claim it did nothing") — a rejection argued from the repo rather than from taste. Option (b) was rejected because acted_count is a column and a highlightField the alert reads directly: annotating elsewhere leaves the lie where the operator looks. Endorsed.
    • Leaving status: success is right and reasoned: a channel registered a moment later (a supported pattern per messaging-service-plugin.ts:212) must not retroactively fail a flow. No synchronous wait on async delivery — the one hard constraint in the dispatch, honored.
    • Test shape is exactly what this defect required: real MessagingService + MemoryNotificationOutbox + NotificationDispatcher, because the disagreement lives in the seam between enqueue and dispatch — a fake answering emit() in one shot cannot express it. Four cases cover both paths in both directions (including "inline + unregistered ⇒ a MEASURED zero, correctly alert-eligible", which stops the fix degenerating into "unregistered channels are special"). Reverse verification is precise: on main the durable-record assertions pass while the summary assertion fails with acted: 1, unmeasured: 0 — the finding verbatim.
    • Pin review done properly: the one updated pin is the one that encoded the conflation, connector-nodes.test.ts:292 correctly unaffected, repo-wide grep confirms no other EmitResult/.delivered consumers.
    • Blast radius declared: outbox-backed notify runs now read acted: 0, unmeasured: 1. That is the intended correction, and the alert semantics hold.
    • Worktree note (branch already checked out in the container's primary clone; dedicated worktree pushed with an explicit refspec so the remote branch name and content match the contract) — compliant, no guard bypassed.
    • All 26 checks green; changeset ×2; test-only devDependency introduces no cycle.

    Out-of-scope finding to file: http-nodes.ts:139 returns metrics: { acted: 1 } for an outbox-enqueued HTTP delivery — same overstatement class against sys_http_delivery, which has its own dead status. I'll file it unassigned rather than ride it here.

    Marking ready and arming auto-merge now.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

domain:servicesRetired 2026-10-22, alias of domain:server (services surface); in-flight cards keep it until landedpm:dispatchedDispatched to a dev agent by /pm-dispatch

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions