feat(spec)!: retire waitEventConfig.timeoutMs / .onTimeout — wait never had a timeout (#4158)

.changeset/wait-timeout-keys-retired.md

@@ -0,0 +1,42 @@
+---
+'@objectstack/spec': major
+'@objectstack/service-automation': patch
+---
+
+feat(spec)!: retire `waitEventConfig.timeoutMs` / `.onTimeout` — `wait` never had a timeout (#4158)
+
+Both keys described a timeout and neither delivered one, so protocol 18 removes the pair
+rather than leaving a promise the runtime does not keep (PD #10).
+
+- **`onTimeout`** had **zero** readers. No path ever inspected it, so neither `'fail'` nor
+  `'continue'` ever happened — and its `.default('fail')` stamped a decision nothing made
+  onto every wait node. The showcase set `onTimeout: 'continue'`, which did nothing.
+- **`timeoutMs`** said *"maximum wait time before timeout"* while its only reader used it
+  as the timer **duration** when `timerDuration` was absent. It did something, just not
+  what it claimed.
+
+Together they declared a timeout `wait` does not have: a run resumes when its timer
+elapses or its signal arrives, never on a deadline. Real timeout semantics are left
+unimplemented deliberately — they should be built to a requirement, not retrofitted to
+fit two keys that happened to be declared.
+
+`timeoutMs` **converts to `timerDuration`** rather than being dropped, because that is
+what it did. It is stringified on the way: the target is `z.string()` while `timeoutMs`
+was `z.number()`, and `parseIsoDuration` reads a bare numeric string as milliseconds — so
+`timeoutMs: 60000` and `timerDuration: '60000'` are the same wait. Moving the number
+unstringified would have produced a block that no longer parses, which a test pins. With
+`timerDuration` already set it is dropped instead: the executor's `??` never looked past
+the duration, so it was already dead metadata.
+
+Both leave the **load path** (`retiredFromLoadPath`), which is the registry's existing
+split: a key retired for being *renamed* keeps a load window, because punishing an author
+for a spelling nobody warned them about is pointless; a key that **misdescribed itself**
+does not, because silently absorbing it lets the author keep believing they configured a
+timeout. That is why `api.requireAuth`, the tool/app/flow inert keys and RLS `priority`
+all left it too. The migration chain converts stored sources mechanically; the schema
+tombstones name the replacement.
+
+One fixture interaction worth recording: the #4045 lift fixture used
+`waitEventConfig.timeoutMs` to demonstrate its fourth ledger entry, and the fixture
+harness replays the whole table — so its `after` described an end state protocol 18 makes
+unreachable. It now lifts `eventType` instead. The harness caught this itself.

content/docs/references/automation/flow.mdx

@@ -104,7 +104,7 @@ const result = Flow.parse(data);
 | **timeoutMs** | `integer` | optional | Maximum execution time for this node in milliseconds |
 | **inputSchema** | `Record<string, { type: Enum<'string' \| 'number' \| 'boolean' \| 'object' \| 'array'>; required: boolean; description?: string }>` | optional | Input parameter schema for this node |
 | **outputSchema** | `any` | optional | [REMOVED] `flow.nodes[].outputSchema` was removed in @objectstack/spec 17.0.0 (#3896 audit close-out) — it was never validated: the engine does not check node outputs against it, so it documented a contract nothing enforced. Delete the key. Downstream nodes read prior outputs via expressions (`{{nodeId.field}`}) regardless of any declaration. |
-| **waitEventConfig** | `{ eventType: Enum<'timer' \| 'signal' \| 'webhook' \| 'manual' \| 'condition'>; timerDuration?: string; signalName?: string; timeoutMs?: integer; … }` | optional | Configuration for wait node event resumption |
+| **waitEventConfig** | `{ eventType: Enum<'timer' \| 'signal' \| 'webhook' \| 'manual' \| 'condition'>; timerDuration?: string; signalName?: string; timeoutMs?: any; … }` | optional | Configuration for wait node event resumption |
 | **boundaryConfig** | `{ attachedToNodeId: string; eventType: Enum<'error' \| 'timer' \| 'signal' \| 'cancel'>; interrupting: boolean; errorCode?: string; … }` | optional | Configuration for boundary events attached to host nodes |
 
 

examples/app-showcase/src/automation/flows/index.ts

@@ -554,7 +554,9 @@ export const TaskFollowUpFlow = defineFlow({
       label: 'Wait 1 min',
       // Timer wait: suspends the run, then a one-shot job resumes it after the
       // duration. ISO-8601 duration; production reminders would use e.g. 'P3D'.
-      waitEventConfig: { eventType: 'timer', timerDuration: 'PT1M', onTimeout: 'continue' },
+      // No `onTimeout`: it was retired in #4158 because nothing ever read it —
+      // `wait` has no timeout, and this run resumes when the timer elapses.
+      waitEventConfig: { eventType: 'timer', timerDuration: 'PT1M' },
     },
     {
       id: 'remind',

packages/services/service-automation/src/builtin/wait-node.test.ts

@@ -279,7 +279,9 @@ describe('rearmSuspendedWaitTimers (cold-boot timer re-arm)', () => {
 
   it('resumes an overdue timer immediately (deadline elapsed while down)', async () => {
     const store = new InMemorySuspendedRunStore();
-    const config = { eventType: 'timer', timeoutMs: 1 };
+    // `timerDuration: '1'`, not the retired `timeoutMs: 1` (#4158) — a bare
+    // numeric string is milliseconds, so this is the same 1ms deadline.
+    const config = { eventType: 'timer', timerDuration: '1' };
 
     const a = bootEngine(store, ctxNoJob(), config); // degraded: no job service
     const paused = await a.engine.execute('wait_flow');

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

@@ -64,14 +64,11 @@ export function registerWaitNode(engine: AutomationEngine, ctx: PluginContext):
       const runId = variables.get('$runId');
 
       if (eventType === 'timer') {
-        // `timeoutMs` doubling as a duration is pre-existing behaviour, kept
-        // deliberately — the declared meaning is a timeout guard, and `wait` has
-        // no timeout implementation at all (`onTimeout` has zero readers). That
-        // gap is #4158; changing it here would be a behaviour change riding on a
-        // contract cleanup.
-        const durationMs =
-          parseIsoDuration(wec.timerDuration) ??
-          (typeof wec.timeoutMs === 'number' ? wec.timeoutMs : undefined);
+        // One source for the wait length. `timeoutMs` used to double as this
+        // when `timerDuration` was absent — it was retired in protocol 18 (#4158)
+        // precisely because that is not what it said it did, and the conversion
+        // rewrites it to `timerDuration`, so there is one spelling left.
+        const durationMs = parseIsoDuration(wec.timerDuration);
 
         // Persist the wake deadline as node output: the engine writes output
         // to variables (`<nodeId>.waitUntil`) *before* snapshotting the

packages/spec/src/automation/flow.test.ts

@@ -958,8 +958,6 @@ describe('BPMN — Wait Event Configuration', () => {
       waitEventConfig: {
         eventType: 'timer',
         timerDuration: 'PT1H',
-        timeoutMs: 7200000,
-        onTimeout: 'fail',
       },
     });
     expect(result.success).toBe(true);
@@ -977,15 +975,12 @@ describe('BPMN — Wait Event Configuration', () => {
       waitEventConfig: {
         eventType: 'webhook',
         signalName: 'payment_received',
-        timeoutMs: 86400000,
-        onTimeout: 'continue',
       },
     });
     expect(result.success).toBe(true);
     if (result.success) {
       expect(result.data.waitEventConfig?.eventType).toBe('webhook');
       expect(result.data.waitEventConfig?.signalName).toBe('payment_received');
-      expect(result.data.waitEventConfig?.onTimeout).toBe('continue');
     }
   });
 
@@ -1000,8 +995,28 @@ describe('BPMN — Wait Event Configuration', () => {
       },
     });
     expect(result.success).toBe(true);
-    if (result.success) {
-      expect(result.data.waitEventConfig?.onTimeout).toBe('fail'); // default
+  });
+
+  /**
+   * These three tests used to assert the OPPOSITE — that `timeoutMs` and
+   * `onTimeout` were accepted, and that `onTimeout` defaulted to `'fail'`. They
+   * were encoding a contract the runtime never honoured: nothing read `onTimeout`,
+   * and `timeoutMs` was consumed as the timer duration rather than as a timeout
+   * (#4158). Retiring the pair is what flipped them, which is the point — the
+   * schema now says what `wait` actually does.
+   */
+  it('rejects the retired timeout keys instead of stripping them (#4158)', () => {
+    for (const retired of [{ timeoutMs: 7_200_000 }, { onTimeout: 'fail' }]) {
+      const result = FlowNodeSchema.safeParse({
+        id: 'wait_timer',
+        type: 'wait',
+        label: 'Wait 1 Hour',
+        waitEventConfig: { eventType: 'timer', timerDuration: 'PT1H', ...retired },
+      });
+      const key = Object.keys(retired)[0];
+      expect(result.success, `${key} must be rejected, not silently dropped`).toBe(false);
+      // The prescription names the issue and the replacement (or its absence).
+      expect(JSON.stringify(result.error?.issues), `${key} guidance`).toMatch(/4158/);
     }
   });
 

packages/spec/src/automation/flow.zod.ts

@@ -212,10 +212,40 @@ export const FlowNodeSchema = lazySchema(() => z.object({
     timerDuration: z.string().optional().describe('ISO 8601 duration (e.g., "PT1H") or wait time for timer events'),
     /** Signal name to listen for — for signal/webhook events */
     signalName: z.string().optional().describe('Named signal or webhook event to wait for'),
-    /** Timeout before auto-failing or continuing — optional guard */
-    timeoutMs: z.number().int().min(0).optional().describe('Maximum wait time before timeout (ms)'),
-    /** Action to take on timeout */
-    onTimeout: z.enum(['fail', 'continue']).default('fail').describe('Behavior when the wait times out'),
+
+    /**
+     * `wait` never had a timeout. Both keys below described one and neither
+     * delivered it (#4158) — the pair is retired in 18 rather than left standing
+     * as a promise the runtime does not keep (PD #10).
+     *
+     * `timeoutMs` said "maximum wait time" and its ONLY reader used it as the
+     * timer *duration* when `timerDuration` was absent — so it did something, just
+     * not what it said. `timerDuration` already expresses that (`parseIsoDuration`
+     * accepts a bare number as milliseconds), which is why the conversion can move
+     * it losslessly instead of dropping it.
+     *
+     * `onTimeout` had ZERO readers anywhere. Setting it changed nothing, and the
+     * showcase set it — a declared default (`'fail'`) stamped on every wait node
+     * that no code ever consulted.
+     *
+     * Real timeout semantics — resume the run at a deadline and either fail the
+     * node or continue past it — remain unimplemented. If they are wanted, they
+     * should be built to a requirement, not retrofitted to fit two keys that
+     * happened to be declared.
+     */
+    timeoutMs: retiredKey(
+      '`waitEventConfig.timeoutMs` was removed in @objectstack/spec 18 (#4158). It documented a '
+      + 'timeout guard that never existed: nothing ever failed or resumed a wait on a deadline. Its '
+      + 'only reader treated it as the timer DURATION when `timerDuration` was absent, so use '
+      + '`timerDuration` — it accepts a bare number as milliseconds, making `timeoutMs: 60000` and '
+      + "`timerDuration: 60000` the same wait. Stored flows are converted automatically.",
+    ),
+    onTimeout: retiredKey(
+      '`waitEventConfig.onTimeout` was removed in @objectstack/spec 18 (#4158). It had no readers at '
+      + 'all — no code path ever inspected it, so neither `fail` nor `continue` ever happened. Delete '
+      + 'the key. There is no replacement: `wait` has no timeout, and a wait node resumes only when '
+      + 'its timer elapses or its signal arrives.',
+    ),
   }).optional().describe('Configuration for wait node event resumption'),
 
   /**

packages/spec/src/conversions/conversions.test.ts

@@ -232,6 +232,63 @@ describe('conversion layer (ADR-0087 D2)', () => {
     });
   });
 
+  describe('flow-node-wait-timeout-keys-removed (#4158)', () => {
+    const wecFlow = (waitEventConfig: Record<string, unknown>) => ({
+      flows: [
+        {
+          name: 'settle',
+          label: 'Settle',
+          type: 'autolaunched',
+          edges: [],
+          nodes: [
+            { id: 'n1', type: 'start', label: 'Start' },
+            { id: 'w', type: 'wait', label: 'Wait', waitEventConfig },
+          ],
+        },
+      ],
+    });
+    const wecOf = (stack: Record<string, unknown>) => (stack.flows as any[])[0].nodes[1].waitEventConfig;
+    // Retired from the load path, so the default `applyConversions` skips it —
+    // exactly like `stack-api-require-auth-removed`.
+    const convert = (stack: Record<string, unknown>) => collectConversionNotices(stack, { includeRetired: true });
+
+    it('moves `timeoutMs` to `timerDuration` as a STRING the schema accepts', () => {
+      const { stack, notices } = convert(wecFlow({ eventType: 'timer', timeoutMs: 60_000 }));
+      expect(wecOf(stack)).toEqual({ eventType: 'timer', timerDuration: '60000' });
+      expect(notices).toHaveLength(1);
+      // The move is only lossless if the result still parses — `timerDuration` is
+      // `z.string()`, so carrying the number across would have broken the block.
+      expect(() => FlowSchema.parse((stack.flows as any[])[0])).not.toThrow();
+    });
+
+    it('drops `timeoutMs` instead of moving it when `timerDuration` already won', () => {
+      const { stack, notices } = convert(
+        wecFlow({ eventType: 'timer', timerDuration: 'PT5M', timeoutMs: 999 }),
+      );
+      expect(wecOf(stack)).toEqual({ eventType: 'timer', timerDuration: 'PT5M' });
+      expect(notices).toHaveLength(1);
+    });
+
+    it('drops `onTimeout` — it never had a reader, so there is nothing to preserve', () => {
+      const { stack, notices } = convert(wecFlow({ eventType: 'timer', timerDuration: 'PT1M', onTimeout: 'continue' }));
+      expect(wecOf(stack)).toEqual({ eventType: 'timer', timerDuration: 'PT1M' });
+      expect(notices).toHaveLength(1);
+    });
+
+    it('leaves a block carrying neither key untouched', () => {
+      const { stack, notices } = convert(wecFlow({ eventType: 'signal', signalName: 'paid' }));
+      expect(wecOf(stack)).toEqual({ eventType: 'signal', signalName: 'paid' });
+      expect(notices).toHaveLength(0);
+    });
+
+    it('tombstones both keys so a source that skipped conversion is rejected, not stripped', () => {
+      for (const bad of [{ timeoutMs: 60_000 }, { onTimeout: 'continue' }]) {
+        const flow = (wecFlow({ eventType: 'timer', timerDuration: 'PT1M', ...bad }).flows as any[])[0];
+        expect(() => FlowSchema.parse(flow), `${Object.keys(bad)[0]} must be rejected`).toThrow(/4158/);
+      }
+    });
+  });
+
   describe('flow-node-wait-event-config-lift (PD #12 retirement, #4045)', () => {
     /**
      * One `wait` node in a flow that `FlowSchema` can actually parse — `label` is