diff --git a/.changeset/http-node-enqueued-not-acted.md b/.changeset/http-node-enqueued-not-acted.md new file mode 100644 index 0000000000..486c263d32 --- /dev/null +++ b/.changeset/http-node-enqueued-not-acted.md @@ -0,0 +1,40 @@ +--- +"@objectstack/service-automation": patch +--- + +fix(service-automation): a durable `http` callout is `unmeasured`, not `acted` (#7882) + +The `http` node's durable path (`config.durable: true`) enqueues onto the +messaging HTTP outbox and returned `metrics: { acted: 1 }` for the enqueue. But +`enqueueHttp()` hands back the id of a **`pending`** `sys_http_delivery` row — +the `HttpDispatcher` decides the real outcome afterwards, and that outcome +includes dead-lettering the callout on a non-retriable response or an exhausted +retry budget. So an operator reading the run summary could see `acted: 1` for a +callout the durable record shows as `dead`: the summary asserted an effect that +never happened. + +The durable path now reports `unmeasuredEffect` instead. That is the platform's +existing word for "an effect happened but its outcome is not yet knowable" — the +same qualifier `connector_action` uses — and pointedly **not** a bare `acted: 0`, +which `connector.zod.ts` forbids because it would claim the run did nothing and +would trip the documented broken-sweep alert +(`selected > 0 AND acted = 0 AND unmeasured = 0`) on every healthy durable +callout. A pending delivery now suppresses that alert without asserting success, +and `unmeasured` is already surfaced by `formatRunSummaryLine` and by the +`unmeasured_count` column on `sys_automation_run`. + +Same overstatement class as #7747 at the sibling `notify` node, but a smaller +fix. That one needed `EmitResult` split into `delivered` vs `enqueued` inside +`service-messaging`, because `MessagingService.emit()` hides two outcomes behind +one call — inline (P0) fan-out, which really does know the result, and the P1 +outbox, which does not. `enqueueHttp()` has no such ambiguity: it returns a row +id and the row is unconditionally `pending`, and the two-path structure already +sits in the node itself. Nothing in `service-messaging` changed. + +**Unchanged:** the step still succeeds — the flow did everything it can do +synchronously, and not blocking on the callout is the entire point of durable +mode — and its output is still `{ deliveryId, enqueued: true }`. The inline +request/response path keeps its measured counts: a mutating call the upstream +accepted is still `acted: 1`, a `GET` is still a real `acted: 0`, and a rejected +or timed-out mutating call was already `unmeasured`. This narrows what `acted` +may claim; it does not blanket every HTTP callout as unmeasurable. diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index 6f8f4835f5..906418c752 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -734,7 +734,7 @@ instead: | `http`, `GET`/`HEAD`/`OPTIONS` | `acted: 0` — a read can never write | | `http`, mutating method, response OK | `acted: 1` | | `http`, mutating method, rejected / timed out | `unmeasured` — a 500 can arrive after the write landed | -| `http`, `durable: true` | `acted: 1` — the outbox row is a real, durable effect | +| `http`, `durable: true` | `unmeasured` — the outbox row is a real, durable effect, but a *pending* one: the dispatcher decides the outcome (including `dead`) after the run has settled, so counting it would let the summary claim a callout that `sys_http_delivery` records as dead-lettered | | `connector_action` | `unmeasured` | | `script`, function declared pure (the default) | nothing — a registered function is **contractually pure**: data I/O stays on the flow graph, so every write it causes is a downstream node that counts itself | | `script`, function declared `effect: 'writes'` | `unmeasured` — the function said it writes where the platform cannot see, so the run says the count is incomplete | diff --git a/packages/services/service-automation/src/builtin/http-delivery-outcome.integration.test.ts b/packages/services/service-automation/src/builtin/http-delivery-outcome.integration.test.ts new file mode 100644 index 0000000000..7554f92535 --- /dev/null +++ b/packages/services/service-automation/src/builtin/http-delivery-outcome.integration.test.ts @@ -0,0 +1,180 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { MessagingService, MemoryHttpOutbox, HttpDispatcher } from '@objectstack/service-messaging'; +import type { FetchImpl } from '@objectstack/service-messaging'; +import { AutomationEngine } from '../engine.js'; +import { registerHttpNodes } from './http-nodes.js'; + +/** + * #7882 — the run summary an operator reads must not claim an HTTP callout that + * `sys_http_delivery` records as dead. + * + * Same defect class as #7747/#7875 at the sibling `notify` node, but the seam + * sits somewhere else. `MessagingService.emit()` had TWO outcomes behind one + * call (inline P0 fan-out, which really does know the result, and the P1 outbox, + * which does not), so that fix had to split `EmitResult.delivered` from + * `enqueued` inside the messaging service before the node could tell them apart. + * `enqueueHttp()` has no such ambiguity: it returns a row id and nothing else, + * and the row it writes is unconditionally `pending`. The two-path structure is + * in the NODE — `durable: true` enqueues, everything else calls `fetch()` inline + * — and the inline half already reports honestly. So the whole disagreement is + * one branch of one node. + * + * The assertions are on the two DURABLE operator-facing records — the folded run + * summary and the `sys_http_delivery` row — never on how many times anything was + * called: the finding IS that those two records contradict each other, so a + * call-count assertion would pass while the defect stands. The real + * `MessagingService` + `MemoryHttpOutbox` + `HttpDispatcher` are wired for the + * same reason a fake would not do: the outcome is decided AFTER the run settles, + * which is a fact only a real enqueue/dispatch pair can express. + * + * On `origin/main` the two durable-mode tests below fail with `acted: 1`. + */ + +function silentLogger(): any { + const l: any = { info: () => {}, warn: () => {}, error: () => {}, debug: () => {} }; + l.child = () => l; + return l; +} + +/** Wire the `http` node against a given messaging service. */ +function engineWith(messaging?: MessagingService): AutomationEngine { + const engine = new AutomationEngine(silentLogger()); + registerHttpNodes(engine, { + logger: silentLogger(), + getService: (name: string) => (name === 'messaging' ? messaging : undefined), + } as any); + return engine; +} + +/** + * A stack booted the way a durable flow callout runs in production: messaging + * present with the HTTP outbox wired (ADR-0018 M3), and a dispatcher that is + * ticked manually so "before dispatch" and "after dispatch" are distinguishable. + */ +function bootOutboxStack(fetchImpl: FetchImpl) { + const outbox = new MemoryHttpOutbox(); + const messaging = new MessagingService({ logger: silentLogger() }); + messaging.setHttpOutbox(outbox); + + const dispatcher = new HttpDispatcher({ + nodeId: 'node-test', + outbox, + partitionCount: 1, + intervalMs: 10_000, // ticks are driven manually + fetchImpl, + logger: silentLogger(), + }); + + return { outbox, messaging, dispatcher, engine: engineWith(messaging) }; +} + +/** A fetch double for the DISPATCHER (not the node) returning one fixed status. */ +function respondWith(status: number): FetchImpl { + return (async () => ({ ok: status >= 200 && status < 300, status, async text() { return ''; } })) as FetchImpl; +} + +function httpFlow(config: Record) { + return { + name: 'callout', + label: 'Callout', + type: 'autolaunched' as const, + nodes: [ + { id: 'start', type: 'start' as const, label: 'Start' }, + { id: 'http', type: 'http' as const, label: 'HTTP', config }, + { id: 'end', type: 'end' as const, label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'http' }, + { id: 'e2', source: 'http', target: 'end' }, + ], + }; +} + +describe('http run summary vs. the durable sys_http_delivery record (#7882)', () => { + afterEach(() => vi.unstubAllGlobals()); + + it('does not report a countable act for a durable delivery that dead-letters', async () => { + // A 404 is non-retriable, so the dispatcher dead-letters on the first + // attempt — the terminal disagreement the finding describes. + const { outbox, dispatcher, engine } = bootOutboxStack(respondWith(404)); + engine.registerFlow('callout', httpFlow({ url: 'https://example.test/hook', durable: true, body: { a: 1 } })); + + const run = await engine.execute('callout'); + + // 1) The durable record: the row dead-letters once the dispatcher runs. + await dispatcher.tick(); + const rows = await outbox.list(); + expect(rows).toHaveLength(1); + expect(rows[0].source).toBe('flow'); + expect(rows[0].status).toBe('dead'); + expect(rows[0].responseCode).toBe(404); + + // 2) The record an operator reads. The run still SUCCEEDS — the flow did + // everything it can do synchronously, and blocking a flow on a + // downstream callout is exactly what the durable mode exists to + // avoid. What must not survive is the claim that the callout LANDED: + // `acted` is the count the broken-sweep alert trusts, and the honest + // answer when the run settles is "an effect I cannot count yet" — + // which the platform already spells `unmeasured`, and which is not + // the same as a bare `acted: 0` (that would claim the run did + // nothing, and trip the alert on every healthy durable callout). + expect(run.success).toBe(true); + expect(run.summary).toMatchObject({ acted: 0, unmeasured: 1 }); + + // The finding itself, as one assertion: the summary must not out-count + // what the durable record shows actually landed (here: nothing). + const notDead = rows.filter((r) => r.status !== 'dead').length; + expect(run.summary!.acted).toBeLessThanOrEqual(notDead); + }); + + it('reports the same uncountable effect for a delivery that later succeeds — the outcome is simply not known yet', async () => { + // The counterpart that stops the fix from degenerating into + // "dead-lettered deliveries are special": at the moment the run settles, + // a perfectly healthy durable callout is equally unsent. What separates + // the two cases is the outbox row — which is where `unmeasured` points. + const { outbox, dispatcher, engine } = bootOutboxStack(respondWith(200)); + engine.registerFlow('callout', httpFlow({ url: 'https://example.test/hook', durable: true, body: { a: 1 } })); + + const run = await engine.execute('callout'); + + expect(run.success).toBe(true); + expect(run.summary).toMatchObject({ acted: 0, unmeasured: 1 }); + // Nothing had been attempted when the run settled… + expect((await outbox.list())[0].status).toBe('pending'); + expect((await outbox.list())[0].attempts).toBe(0); + + // …and the delivery lands afterwards, on the record that owns the truth. + await dispatcher.tick(); + expect((await outbox.list())[0].status).toBe('success'); + }); + + it('still reports a countable act for the inline mode, whose outcome IS terminal at return time', async () => { + // The node's other path really does know the answer before it returns, + // so `acted` stays a measurement there — this narrows what `acted` may + // claim, it does not blanket every HTTP callout as unmeasurable. + vi.stubGlobal('fetch', vi.fn(async () => ({ + ok: true, status: 201, async json() { return { created: true }; }, async text() { return ''; }, + }))); + const engine = engineWith(); + engine.registerFlow('callout', httpFlow({ url: 'https://api.test/items', method: 'POST', body: { n: 1 } })); + + const run = await engine.execute('callout'); + + expect(run.success).toBe(true); + expect(run.summary).toMatchObject({ acted: 1, unmeasured: 0 }); + }); + + it('an inline GET is a measured zero — it cannot write, so the run is eligible for the broken-sweep alert', async () => { + vi.stubGlobal('fetch', vi.fn(async () => ({ + ok: true, status: 200, async json() { return { items: [] }; }, async text() { return ''; }, + }))); + const engine = engineWith(); + engine.registerFlow('callout', httpFlow({ url: 'https://api.test/items', method: 'GET' })); + + const run = await engine.execute('callout'); + + expect(run.summary).toMatchObject({ acted: 0, unmeasured: 0 }); + }); +}); diff --git a/packages/services/service-automation/src/builtin/http-nodes.ts b/packages/services/service-automation/src/builtin/http-nodes.ts index 13e9ef48e3..a37d7dbc9f 100644 --- a/packages/services/service-automation/src/builtin/http-nodes.ts +++ b/packages/services/service-automation/src/builtin/http-nodes.ts @@ -135,8 +135,34 @@ export function registerHttpNodes(engine: AutomationEngine, ctx: PluginContext): payload: body ?? {}, }); // #4354 — the outbox row IS a durable effect this run - // caused, even though the upstream call happens later. - return { success: true, output: { deliveryId, enqueued: true }, metrics: { acted: 1 } }; + // caused, but it is NOT a countable one (#7882). What + // `enqueueHttp` returns is the id of a `pending` row; + // the dispatcher decides the real outcome afterwards, + // and that outcome includes dead-lettering the callout + // entirely. Counting the enqueue as `acted` made the run + // summary assert a delivery that `sys_http_delivery` + // recorded as `dead` — the same overstatement #7747 + // fixed at the `notify` node. + // + // The honest answer when the run settles is "an effect I + // cannot count yet", which is exactly `unmeasuredEffect` + // — and pointedly NOT a bare `acted: 0`, which would + // claim the run did nothing (`connector.zod.ts:613`) and + // trip the broken-sweep alert on every healthy durable + // callout. That alert is `selected > 0 AND acted = 0 AND + // unmeasured = 0`, so a pending delivery suppresses it + // without asserting success. + // + // Waiting for the real outcome is not on the table: the + // whole point of `durable: true` is that the flow does + // NOT block on the callout. The inline path below keeps + // its measured `acted` — its outcome IS terminal by the + // time it returns. + return { + success: true, + output: { deliveryId, enqueued: true }, + metrics: { unmeasuredEffect: true }, + }; } catch (err) { return { success: false, error: `http (durable) failed to enqueue: ${(err as Error).message}` }; }