Skip to content

Commit 21e5979

Browse files
fix(trigger-schedule): an invalid timeRelative descriptor throws from start so the automation status reads it unbound (#27556)
Fixes #27415 Clause-②: no Metadata-protocol: none ## 维护者速读 - 改了什么:时间相对触发器(time_relative)在 `timeRelative` 描述符不合法时,`start` 由「记日志后 return」改为「记同一句日志后抛错」,并先撤掉此前已挂的扫描任务。 - 为什么改:原来 return 被引擎当成绑定成功,`GET /automation/_status` 对一个根本没挂上的流报 `bound: true`、没有 `unboundCause`,与服务器自己的 "not bound" 日志相矛盾;抛错是引擎既有的「绑定失败」通道,`schedule` 触发器的组织/部署策略拒绝早已这样做。 - 风险与回滚:只改一个文件的一个分支;合法描述符行为不变。直接调用 `TimeRelativeTrigger.start` 的宿主在非法描述符上会收到异常(引擎自身已 catch)。回滚即还原本 PR 一个 commit。 - 先看:`packages/triggers/trigger-schedule/src/time-relative-trigger.ts`(`start` 里 `!parsed.success` 分支,风险在 throw 前的 `this.stop`)、`src/time-relative-bind-status-e2e.test.ts`(真引擎状态行的 pin)。 - 两分钟验证:`pnpm --filter @objectstack/trigger-schedule exec vitest run src/time-relative-bind-status-e2e.test.ts`,预期 1 passed:非法描述符的流读 `bound: false` + `unboundCause: binding_failure`,合法的流读 `bound: true`。 - 需要你做的:无,席位落地。 ## What changes `TimeRelativeTrigger.start` refused an invalid `config.timeRelative` descriptor (both `withinDays` and `offsetDays`, an unknown key, `schedule` nested inside the descriptor) by logging `[time-relative] flow 'NAME' has no valid timeRelative descriptor — not bound` and returning. `FlowTrigger.start` returns `void`, so the engine's `activateFlowTrigger` read that return as a successful arm and recorded the flow in `boundFlowTriggers`. The status row (`getFlowRuntimeStates()`, served by `GET /api/v1/automation/_status`) then read `bound: true` with no `unboundCause`. The refusal now throws the same sentence it logs. This is the path the trigger's own organization and deployment-policy refusals already take (`refuseMissingOrganization` / `refuseScheduledWorkDisabled` in `schedule-trigger.ts`; the "Why it throws, and does not merely log and return" docblock there describes the mechanism). The engine's existing catch never marks the flow bound, so the row reads `bound: false` with `unboundCause: 'binding_failure'`, and the boot binding audit lists it. Before throwing, the branch calls `this.stop(flowName)`, as the organization refusal does, so a re-bind with a broken descriptor cannot leave the sweep from an earlier valid bind armed. The `warn` line text is byte-identical to before, and the level stays `warn`: a trigger that is not armed is a functional degradation, and the status row now reports it. No engine change and no `FlowTrigger` interface change. `packages/services/service-automation/**` is untouched. One `patch` changeset for `@objectstack/trigger-schedule`. ## Tests - `src/time-relative-trigger.test.ts`: the existing "invalid descriptor" case now asserts the throw. A new block covers three invalid shapes (both modes, an unknown key, `schedule` nested in the descriptor). For each: `start` throws a message naming the flow, arms no job, and logs the refusal once. Also new: a re-bind with a broken descriptor drops the previously armed sweep, and a control where a valid descriptor does not throw and arms its sweep. - `src/time-relative-bind-status-e2e.test.ts` (new): the real `TimeRelativeTrigger` registered on the real `AutomationEngine`. An invalid-descriptor flow reads `bound: false`, `triggerType: 'time_relative'`, `unboundCause: 'binding_failure'`, with no sweep armed. The valid control flow reads `bound: true`, has no `unboundCause` key, and its sweep is armed. The engine-only twin of this pin (`engine.test.ts`, "ON: a genuine bind failure reads as one on the status row too") lives in `service-automation`, which is outside this PR's surface. This package already devDepends on that engine, so the pin lives here. <details><summary>Package test + typecheck at da85763</summary> ``` pnpm --filter @objectstack/trigger-schedule test && pnpm --filter @objectstack/trigger-schedule typecheck Test Files 11 passed (11) Tests 196 passed (196) > tsc --noEmit os-verify-lock: VERDICT command-exit 0 ``` `tsc --listFiles` counts 11 of the package's `*.test.ts` files in the program, including the new e2e file. </details> <details><summary>Ablation (one-shot): the throw replaced by the old return</summary> `node scripts/ablation-replace.mjs --file packages/triggers/trigger-schedule/src/time-relative-trigger.ts --anchor 'throw new Error(sentence);' --replacement 'return;'` landed (anchor 1 to 0, blob 520bd25 to cf719226888f). Both suites then went red: ``` Test Files 2 failed (2) Tests 6 failed | 60 passed (66) x does not bind when the timeRelative descriptor is invalid x both windowing modes / an unknown key / schedule nested: start throws ... x a re-bind with a broken descriptor drops the sweep the previous bind armed x an invalid descriptor reads bound:false with the binding-failure cause; a valid one reads bound:true ``` Restore leg: blob after restore 520bd25 equals the HEAD blob, and `git diff HEAD` is empty. The trigger is imported by relative path, so the mutation reached the run without a rebuild. The engine resolves through `dist/`, but its source is unchanged in this diff. </details> <details><summary>Local gates (derived by dispatch-gates --commands)</summary> 57 derived families were run locally, and `--ran` reconciled 57 of 57. 54 exited 0. `check:nul-bytes`, `check:cross-package-test-inputs`, `check:test-source-alias` and `check:engine-double-contract` were re-run at da85763 and all exited 0. NOT MEASURED locally, left to CI: - `check:dual-build-cjs-loads`: exit 3, PREREQUISITE NOT MET. It needs every package's `dist/`. - `check:query-options-erasure`: hit the 300 s local timeout. It is a repo-wide ESLint scan. - `check:type-check-debt`: hit the 300 s local timeout during the whole-repo tsc walk. The derivation warned that its tree was 7 commits behind origin/main. The 2 stale files are `scripts/gen-sdui-reference.mjs` and `scripts/pm/dispatch-gates.self-test.mjs`. Neither reads this diff's paths. </details> ## Acceptance notes - The `schedule` trigger's own descriptor refusal has the same log-and-return shape: `[schedule] flow 'NAME' has no recognizable schedule descriptor — not bound` in `ScheduleTrigger.start` (`schedule-trigger.ts`). Under this PR's reasoning that flow also reads `bound: true`. That file is outside this PR's surface, so the seat has the finding to route. - Both triggers' "job service unavailable — not scheduled" branches also return rather than throw. That is a composition gap, a different remedy from a bad descriptor. This is noted only: it was inferred from reading the code and not reproduced. ## Verification Run the two-minute check above. Or, as on the card: boot the showcase with `OS_AUTOMATION_SCHEDULED_WORK_ENABLED=1` and `OS_ALLOW_UNLINTED_METADATA_WRITES=1`, `PUT` a time_relative flow whose descriptor carries both `withinDays` and `offsetDays`, then read `GET /api/v1/automation/_status`. Expect `bound: false` with `unboundCause: "binding_failure"`. This live-boot path was not run in this PR, and the e2e pin above covers the same engine row. --- _Generated by [Claude Code](https://claude.ai/code/session_017xRBcScm27Q3R5mkYLPWdB)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 475d428 commit 21e5979

4 files changed

Lines changed: 199 additions & 17 deletions

File tree

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
'@objectstack/trigger-schedule': patch
3+
---
4+
5+
fix(trigger-schedule): a time-relative flow whose `timeRelative` descriptor is refused now reads `bound: false` on the automation status, not `bound: true`
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
`TimeRelativeTrigger.start` refused an invalid `config.timeRelative` descriptor (both `withinDays` and `offsetDays`, an unknown key, `schedule` nested inside the descriptor) by logging "not bound" and returning. The engine read that return as a successful bind, so `GET /api/v1/automation/_status` reported the flow as `bound: true` with no `unboundCause`, and no sweep ever ran.
11+
12+
The refusal now throws, the same way the trigger's organization and deployment-policy refusals already do. The engine records the flow as unbound: its status row reads `bound: false` with `unboundCause: 'binding_failure'`, and the boot-time binding audit lists it. The `warn` line the server logs is unchanged, and the thrown text is the same sentence. A re-bind with a broken descriptor also cancels the sweep an earlier valid bind armed.
13+
14+
Nothing changes for a valid descriptor. A host that calls `TimeRelativeTrigger.start` directly, outside the engine, should expect it to throw on a refused descriptor.
Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
// END-TO-END: the REAL TimeRelativeTrigger wired to the REAL AutomationEngine,
4+
// pinning what the engine's status row (the rows `GET /automation/_status`
5+
// serves) says about a flow the trigger refused on its descriptor.
6+
//
7+
// `FlowTrigger.start` returns `void`, so a trigger that logs "not bound" and
8+
// returns reads to the engine as a successful arm: the engine marks the flow
9+
// bound, and the status row answers `bound: true` beside the trigger's own
10+
// refusal. The trigger throws instead; the engine's catch never marks the flow
11+
// bound, and the row reads `bound: false` with the `binding_failure` cause.
12+
// A valid descriptor on the same engine is the control that still binds.
13+
14+
import { describe, it, expect } from 'vitest';
15+
import { AutomationEngine } from '@objectstack/service-automation';
16+
import type { JobSchedule, JobHandler } from '@objectstack/spec/contracts';
17+
import { TimeRelativeTrigger, type TimeRelativeDataEngine } from './time-relative-trigger.js';
18+
import { withScheduledWorkOn } from './deployment-switch.test-support.js';
19+
20+
function quietLogger(): any {
21+
const l: any = { info() {}, warn() {}, error() {}, debug() {} };
22+
l.child = () => l;
23+
return l;
24+
}
25+
26+
const silent = { info() {}, warn() {}, debug() {} };
27+
const flush = () => new Promise<void>((r) => setTimeout(r, 0));
28+
29+
function fakeJobService() {
30+
const jobs = new Map<string, { schedule: JobSchedule; handler: JobHandler }>();
31+
return {
32+
service: {
33+
async schedule(name: string, schedule: JobSchedule, handler: JobHandler) {
34+
jobs.set(name, { schedule, handler });
35+
},
36+
async cancel(name: string) {
37+
jobs.delete(name);
38+
},
39+
},
40+
has: (name: string) => jobs.has(name),
41+
};
42+
}
43+
44+
const dataEngine: TimeRelativeDataEngine = {
45+
async find() {
46+
return [];
47+
},
48+
getObject(name: string) {
49+
return name === 'contracts' ? { name } : undefined;
50+
},
51+
};
52+
53+
function timeRelativeFlow(name: string, timeRelative: Record<string, unknown>) {
54+
return {
55+
name,
56+
label: name,
57+
type: 'autolaunched',
58+
nodes: [
59+
{ id: 'start', type: 'start', label: 'Start', config: { timeRelative } },
60+
{ id: 'end', type: 'end', label: 'End' },
61+
],
62+
edges: [{ id: 'e1', source: 'start', target: 'end' }],
63+
} as never;
64+
}
65+
66+
// The switch on: every assertion here is about a deployment that RUNS
67+
// package-authored scheduled work, so a refusal can only come from the
68+
// descriptor, never from the deployment policy.
69+
withScheduledWorkOn('single');
70+
71+
describe('time-relative trigger -> engine status: a descriptor refusal reads unbound', () => {
72+
it('an invalid descriptor reads bound:false with the binding-failure cause; a valid one reads bound:true', async () => {
73+
const engine = new AutomationEngine(quietLogger());
74+
engine.registerFlow(
75+
'tr_both',
76+
timeRelativeFlow('tr_both', { object: 'contracts', dateField: 'end_date', withinDays: 30, offsetDays: [7] }),
77+
);
78+
engine.registerFlow(
79+
'tr_ok',
80+
timeRelativeFlow('tr_ok', { object: 'contracts', dateField: 'end_date', withinDays: 30 }),
81+
);
82+
const job = fakeJobService();
83+
engine.registerTrigger(new TimeRelativeTrigger(() => job.service, () => dataEngine, silent));
84+
await flush();
85+
86+
const states = engine.getFlowRuntimeStates();
87+
const refused = states.find((s) => s.name === 'tr_both');
88+
const armed = states.find((s) => s.name === 'tr_ok');
89+
90+
expect(refused, 'the refused flow is listed').toBeTruthy();
91+
expect(refused).toMatchObject({ bound: false, triggerType: 'time_relative' });
92+
expect(refused?.unboundCause).toBe('binding_failure');
93+
expect(job.has('flow-time-relative:tr_both'), 'no sweep armed for the refused flow').toBe(false);
94+
95+
expect(armed, 'control: the valid flow is listed').toBeTruthy();
96+
expect(armed).toMatchObject({ bound: true, triggerType: 'time_relative' });
97+
expect(Object.keys(armed ?? {}), 'control: a bound row carries no unboundCause').not.toContain('unboundCause');
98+
expect(job.has('flow-time-relative:tr_ok'), 'control: the valid flow armed its sweep').toBe(true);
99+
});
100+
});

‎packages/triggers/trigger-schedule/src/time-relative-trigger.test.ts‎

Lines changed: 63 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -403,24 +403,76 @@ describe('TimeRelativeTrigger', () => {
403403
);
404404

405405
// Neither withinDays nor offsetDays → invalid.
406-
trigger.start(binding({ object: 'contracts', dateField: 'end_date' }), async () => {});
406+
expect(() => trigger.start(binding({ object: 'contracts', dateField: 'end_date' }), async () => {}))
407+
.toThrow(/flow 'renewal_alert' has no valid `timeRelative` descriptor/);
407408
await flush();
408409

409410
expect(job.jobs.size).toBe(0);
410411
expect(warn).toHaveBeenCalledWith(expect.stringContaining('no valid `timeRelative` descriptor'));
411412
});
412413

413-
it('does not bind when both windowing modes are set (mutually exclusive)', async () => {
414-
const job = fakeJobService();
415-
const { engine } = fakeDataEngine([]);
416-
const trigger = new TimeRelativeTrigger(() => job.service, () => engine, silentLogger(), NOW);
414+
// A descriptor refusal THROWS, so the engine's catch records the flow as
415+
// unbound instead of marking it bound after a "not bound" log line — the
416+
// `_status` row then reads `bound: false`, never `bound: true` beside a
417+
// refusal. Each invalid shape is refused the same way; the valid case is
418+
// the control that `start` still returns on a descriptor it accepts.
419+
describe('an invalid descriptor is a binding failure the host can see', () => {
420+
const INVALID: Array<[string, Record<string, unknown>]> = [
421+
['both windowing modes (mutually exclusive)', { object: 'contracts', dateField: 'end_date', withinDays: 30, offsetDays: [7] }],
422+
['an unknown key', { object: 'contracts', dateField: 'end_date', withinDays: 30, bogus_key: 1 }],
423+
['`schedule` nested inside the descriptor', { object: 'contracts', dateField: 'end_date', withinDays: 30, schedule: { type: 'cron', expression: '0 8 * * *' } }],
424+
];
417425

418-
trigger.start(
419-
binding({ object: 'contracts', dateField: 'end_date', withinDays: 30, offsetDays: [7] }),
420-
async () => {},
421-
);
422-
await flush();
423-
expect(job.jobs.size).toBe(0);
426+
for (const [label, desc] of INVALID) {
427+
it(`${label}: start throws, names the flow, arms nothing`, async () => {
428+
const job = fakeJobService();
429+
const { engine } = fakeDataEngine([]);
430+
const warns: string[] = [];
431+
const trigger = new TimeRelativeTrigger(
432+
() => job.service,
433+
() => engine,
434+
{ info: () => {}, debug: () => {}, warn: (m: string) => void warns.push(String(m)) },
435+
NOW,
436+
);
437+
438+
expect(() => trigger.start(binding(desc), async () => {}))
439+
.toThrow(/flow 'renewal_alert' has no valid `timeRelative` descriptor — not bound/);
440+
await flush();
441+
expect(job.jobs.size, 'no sweep armed').toBe(0);
442+
expect(warns.filter((w) => w.includes('no valid `timeRelative` descriptor')), 'said once').toHaveLength(1);
443+
});
444+
}
445+
446+
it('a re-bind with a broken descriptor drops the sweep the previous bind armed', async () => {
447+
const job = fakeJobService();
448+
const { engine } = fakeDataEngine([]);
449+
const trigger = new TimeRelativeTrigger(() => job.service, () => engine, silentLogger(), NOW);
450+
451+
trigger.start(binding({ object: 'contracts', dateField: 'end_date', withinDays: 30 }), async () => {});
452+
await flush();
453+
expect(job.jobs.size, 'control: the valid bind armed a sweep').toBe(1);
454+
455+
expect(() =>
456+
trigger.start(
457+
binding({ object: 'contracts', dateField: 'end_date', withinDays: 30, offsetDays: [7] }),
458+
async () => {},
459+
),
460+
).toThrow(/no valid `timeRelative` descriptor/);
461+
await flush();
462+
expect(job.jobs.size, 'the refused re-bind left no sweep armed').toBe(0);
463+
});
464+
465+
it('control: a valid descriptor returns without throwing and arms its sweep', async () => {
466+
const job = fakeJobService();
467+
const { engine } = fakeDataEngine([]);
468+
const trigger = new TimeRelativeTrigger(() => job.service, () => engine, silentLogger(), NOW);
469+
470+
expect(() =>
471+
trigger.start(binding({ object: 'contracts', dateField: 'end_date', withinDays: 30 }), async () => {}),
472+
).not.toThrow();
473+
await flush();
474+
expect(job.jobs.size).toBe(1);
475+
});
424476
});
425477

426478
it('warns (but still binds) when the swept object is unknown', async () => {

‎packages/triggers/trigger-schedule/src/time-relative-trigger.ts‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -334,12 +334,28 @@ export class TimeRelativeTrigger implements FlowTrigger {
334334
const raw = (binding.config as Record<string, unknown> | undefined)?.timeRelative;
335335
const parsed = TimeRelativeTriggerSchema.safeParse(raw);
336336
if (!parsed.success) {
337-
this.logger.warn(
338-
`[time-relative] flow '${binding.flowName}' has no valid \`timeRelative\` descriptor — not bound. ` +
339-
`Provide { object, dateField, and exactly one of withinDays | offsetDays }. ` +
340-
`(${parsed.error.issues.map((i) => `${i.path.join('.') || '(root)'}: ${i.message}`).join('; ')})`,
341-
);
342-
return;
337+
// A descriptor refusal is a binding failure, so it THROWS rather
338+
// than logging and returning. `FlowTrigger.start` returns `void`,
339+
// and a return here reads to the engine as a successful arm: it
340+
// marks the flow bound, and `GET /automation/_status` answers
341+
// `bound: true` one line after this said "not bound". Throwing is
342+
// the engine's designed path (see `refuseMissingOrganization`'s
343+
// header): its catch never marks the flow bound, so the status row
344+
// reads `bound: false` with the `binding_failure` cause and the
345+
// binding audit lists the flow. Any prior sweep is dropped FIRST,
346+
// for the reason the organization refusal below drops it: a hot
347+
// re-publish that breaks the descriptor must not leave the previous
348+
// job armed behind a line saying it was refused. `warn`, not
349+
// `error`: a trigger that is not armed is a functional degradation,
350+
// and the status row now says so. One sentence for the log line and
351+
// the thrown text, so the two channels cannot drift.
352+
const sentence =
353+
`flow '${binding.flowName}' has no valid \`timeRelative\` descriptor — not bound. ` +
354+
`Provide { object, dateField, and exactly one of withinDays | offsetDays }. ` +
355+
`(${parsed.error.issues.map((i) => `${i.path.join('.') || '(root)'}: ${i.message}`).join('; ')})`;
356+
this.stop(binding.flowName);
357+
this.logger.warn(`[time-relative] ${sentence}`);
358+
throw new Error(sentence);
343359
}
344360
const desc = parsed.data;
345361

0 commit comments

Comments
 (0)