fix(plugin-reports): a non-member schedule timezone no longer discards the cron expression

`ReportService.scheduleReport`'s eager guard used the callback-less
`new Cron(expr, { timezone })`, which validates the EXPRESSION and lets any
string through as the zone: croner defers that judgement to `nextRun()`.
`nextRunAt` then caught the deferred `CronDate` TypeError and returned
`from + interval_minutes`, so a schedule authored as "every weekday 09:00
Asia/Shanghai" fired every 1440 minutes forever, re-derived on every sweep
through `advanceSchedule` — the verbatim outcome the guard's own comment says
it exists to prevent — and the single warning it emitted named the cron
expression, which was fine, rather than the timezone, which was not.

- `scheduleReport` asks `isValueDomainMember('iana_time_zone', …)` from
  `@objectstack/spec/shared` — the same predicate `sys_report_schedule.timezone`
  enforces on write — so the service door and the storage door give one answer,
  and the refusal names the input that is actually wrong. The row now stores the
  same string the scheduler evaluates.
- On a sweep, a schedule whose stored zone is not a member and which carries a
  cron expression is not run and its `next_run_at` is not advanced;
  `last_status` / `last_error` (both pre-existing columns) carry the reason.
  `active` and the past `next_run_at` are left alone deliberately, so correcting
  the zone resumes the schedule on the next sweep with no second action.
  Interval-only schedules are untouched — interval arithmetic never reads the
  zone.
- Both fall-back warnings now name the expression AND the zone, and the second
  no longer asserts the expression is the broken half.

Fixes #16291

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37

Author: claude

.changeset/report-schedule-timezone-guard.md

@@ -0,0 +1,12 @@
+---
+"@objectstack/plugin-reports": patch
+---
+
+`ReportService` no longer discards a report schedule's cron expression when its `timezone` is not a real IANA zone — and when it does fall back to interval, it says which of the two inputs failed.
+
+croner (10.0.1) answers a non-member zone in three different ways, and only the middle one was ever reached here: `new Cron(expr, { timezone })` **without a callback** validates the expression and lets any zone through, `nextRun()` on that instance then throws a `CronDate` conversion `TypeError`, and the callback form throws at construction. `scheduleReport`'s eager guard used the callback-less form, so the timezone half of its own input passed straight under a guard whose stated purpose was "a clear error at schedule time instead of a schedule that silently falls back to interval on sweep" — and `nextRunAt` caught that deferred throw and returned `from + interval_minutes`. A schedule authored as "every weekday 09:00 Asia/Shanghai" became "every 1440 minutes, forever", re-derived on every sweep, logged only as a complaint about a cron expression that was perfectly good.
+
+- **The create-time guard now asks the right question.** `scheduleReport` consults `isValueDomainMember('iana_time_zone', …)` from `@objectstack/spec/shared` — the same predicate `sys_report_schedule.timezone`'s `valueDomain` declaration enforces on write — and refuses a non-member with `VALIDATION_FAILED: invalid timezone '<zone>': not a member of the 'iana_time_zone' value domain`. One answer at both doors, so this one cannot accept what the storage gate refuses; it only says so earlier and names the input that is actually wrong. It applies whether or not a `cron_expression` is set, because the storage gate does too.
+- **The row now stores the string the scheduler evaluates.** An empty `timezone` was stored verbatim while every `new Cron` call site read it as `UTC`; it is normalised to `UTC` on the way in.
+- **A schedule already holding an unusable zone is stopped, not rescheduled.** `valueDomain` is written-values-only, so rows stored before that declaration are never re-validated and no refusal reaches them. On a sweep, a schedule with a `cron_expression` whose zone is not a member is now not run and its `next_run_at` is not advanced; `last_status` becomes `failed` and `last_error` names the zone, the expression and what to do. `active` stays set and `next_run_at` stays in the past deliberately — the same posture this loop already takes for a schedule whose report has vanished — so correcting the zone resumes the schedule on the next sweep with no second action. Interval-only schedules are untouched: interval arithmetic never consults the zone, so a legacy bad value there still delivers on the cadence its author asked for.
+- **Both fall-back warnings name both inputs.** The "no next occurrence" and the former "invalid cron" lines each mentioned only the expression, so either of them on a timezone fault sent an investigator to audit the half that was fine. They now carry the expression *and* the zone, and the second no longer asserts the expression is the broken one.

packages/platform-objects/src/audit/sys-report-schedule.object.ts

@@ -98,9 +98,18 @@ export const SysReportSchedule = ObjectSchema.create({
     // `invalid cron '<expr>'` — a warning that names the wrong input, since the
     // expression was fine. Neither a throw nor a fall back to UTC: the wrong
     // instant, permanently, which is the outcome this card was told to escalate
-    // on. `scheduleReport`'s eager create-time guard does not catch it either;
-    // it constructs a callback-less `Cron` and so is blind to exactly this half
-    // of its own input. Refusing the write is what closes it.
+    // on. `scheduleReport`'s eager create-time guard did not catch it either;
+    // it constructed a callback-less `Cron` and so was blind to exactly this
+    // half of its own input. Refusing the write is what closes it HERE.
+    //
+    // [#16291] The reader's two halves are closed separately, and this line does
+    // not stand in for either: `scheduleReport` now consults
+    // `isValueDomainMember('iana_time_zone', …)` itself — this declaration's own
+    // predicate, so neither door can accept what the other refuses — and the
+    // sweep quarantines a row that was STORED before this line existed (it does
+    // not run it and does not advance `next_run_at`, and says so in
+    // `last_status` / `last_error`) rather than re-deriving a cadence from
+    // `interval_minutes` that nobody asked for.
     //
     // `maxLength: 64` and `defaultValue: 'UTC'` are BOTH unchanged. The bound is
     // already the value #14238 justified (twice the domain's real ceiling: the

packages/plugins/plugin-reports/src/report-service.test.ts

@@ -465,6 +465,197 @@ describe('ReportService', () => {
     expect(engine._tables['sys_report_schedule'][0].last_status).toBe('ok');
   });
 
+  // ─── Schedule timezone (#16291) ─────────────────────────────────
+  //
+  // croner 10.0.1 has a THREE-state answer to a non-member IANA zone, and only
+  // the middle one was ever reached here (measured on Node v22.22.2):
+  //
+  //   new Cron('0 9 * * *', { timezone: 'Mars/Olympus' })                 -> constructs FINE
+  //     .nextRun(from)                                                    -> TypeError: CronDate …
+  //   new Cron('0 9 * * *', { timezone: 'Mars/Olympus' }, async () => {}) -> throws at construction
+  //
+  // So the create-time guard, which used the callback-less form, validated the
+  // expression and was blind to the zone; and `nextRunAt` caught the deferred
+  // throw and fell back to `interval_minutes` — turning "weekdays 09:00
+  // Asia/Shanghai" into "every 1440 minutes, forever", logged as a complaint
+  // about a cron expression that was perfectly good.
+  describe('schedule timezone', () => {
+    const BAD_TZ = 'Mars/Olympus';
+
+    /** Store a schedule row directly — the shape a pre-#15872 row has. */
+    function seedScheduleRow(reportId: string, patch: Record<string, unknown>) {
+      const row = {
+        id: 'rsch_legacy',
+        report_id: reportId,
+        name: null,
+        interval_minutes: 1440,
+        cron_expression: null,
+        timezone: 'UTC',
+        active: true,
+        recipients: 'ops@t',
+        format: 'html_table',
+        subject_template: null,
+        owner_id: 'u1',
+        next_run_at: new Date(now.getTime() - 1000).toISOString(),
+        created_at: now.toISOString(),
+        updated_at: now.toISOString(),
+        ...patch,
+      };
+      (engine._tables['sys_report_schedule'] ??= []).push(row);
+      return row;
+    }
+
+    // ── The create-time door ──
+
+    it('scheduleReport: refuses a non-member timezone instead of storing it', async () => {
+      const r = await svc.saveReport({ name: 'A', object: 'lead', query: {} }, CTX);
+      await expect(svc.scheduleReport({
+        reportId: r.id, recipients: ['x@t'], cronExpression: '0 9 * * *', timezone: BAD_TZ,
+      }, CTX)).rejects.toThrow(/VALIDATION_FAILED/);
+      // Names the input that is actually wrong — not the cron expression, which
+      // is valid, and which the old guard was the only thing to mention.
+      await expect(svc.scheduleReport({
+        reportId: r.id, recipients: ['x@t'], cronExpression: '0 9 * * *', timezone: BAD_TZ,
+      }, CTX)).rejects.toThrow(new RegExp(`timezone '${BAD_TZ}'`));
+      expect(engine._tables['sys_report_schedule'] ?? []).toHaveLength(0);
+    });
+
+    it('scheduleReport: refuses a non-member timezone with no cron_expression too', async () => {
+      // One answer at both doors. `sys_report_schedule.timezone` carries
+      // `valueDomain: 'iana_time_zone'` (#15872), which refuses the value on
+      // WRITE whether or not a cron is set; a guard that accepted it here for
+      // interval schedules would hand the engine a row it is about to reject and
+      // report the divergence as a generic field error.
+      const r = await svc.saveReport({ name: 'A', object: 'lead', query: {} }, CTX);
+      await expect(svc.scheduleReport({
+        reportId: r.id, recipients: ['x@t'], intervalMinutes: 60, timezone: BAD_TZ,
+      }, CTX)).rejects.toThrow(new RegExp(`VALIDATION_FAILED.*timezone '${BAD_TZ}'`));
+    });
+
+    it('scheduleReport: the guard uses the shared predicate, so real zones still pass', async () => {
+      const r = await svc.saveReport({ name: 'A', object: 'lead', query: {} }, CTX);
+      for (const tz of ['UTC', 'Asia/Shanghai', 'America/New_York', 'Etc/GMT+8']) {
+        const s = await svc.scheduleReport({
+          reportId: r.id, recipients: ['x@t'], cronExpression: '0 9 * * *', timezone: tz,
+        }, CTX);
+        expect(s.timezone).toBe(tz);
+      }
+    });
+
+    it('scheduleReport: stores the same zone string the scheduler evaluates', async () => {
+      // `''` is not an `iana_time_zone` member, but every `new Cron` call site
+      // reads it as UTC via `|| 'UTC'`. The row must not keep a value the storage
+      // gate refuses while the scheduler quietly treats it as something else.
+      const r = await svc.saveReport({ name: 'A', object: 'lead', query: {} }, CTX);
+      const s = await svc.scheduleReport({
+        reportId: r.id, recipients: ['x@t'], cronExpression: '0 9 * * *', timezone: '',
+      }, CTX);
+      expect(s.timezone).toBe('UTC');
+      expect(engine._tables['sys_report_schedule'][0].timezone).toBe('UTC');
+      expect(s.next_run_at).toBe('2026-01-16T09:00:00.000Z');
+    });
+
+    // ── The stored-row door: rows written before #15872 ──
+
+    it('dispatchDue: a stored non-member timezone stops the schedule instead of rescheduling it', async () => {
+      const r = await svc.saveReport({ name: 'A', object: 'lead', query: {} }, CTX);
+      const seeded = seedScheduleRow(r.id, {
+        cron_expression: '0 9 * * 1-5', timezone: BAD_TZ, format: 'csv',
+      });
+
+      const result = await svc.dispatchDue();
+
+      expect(result).toEqual({ fired: 0, failed: 1, skipped: 0 });
+      expect(email._sent).toHaveLength(0);
+      const stored = engine._tables['sys_report_schedule'][0];
+      expect(stored.last_status).toBe('failed');
+      expect(stored.last_error).toContain(BAD_TZ);
+      expect(stored.last_error).toContain('0 9 * * 1-5');
+      // NOT advanced to `now + interval_minutes` — the whole defect was that it
+      // was, on every sweep, forever.
+      expect(stored.next_run_at).toBe(seeded.next_run_at);
+      expect(stored.next_run_at).not.toBe(new Date(now.getTime() + 1440 * 60_000).toISOString());
+    });
+
+    it('dispatchDue: an interval-only schedule with a stored bad zone is left alone', async () => {
+      // The zone is load-bearing only for cron evaluation; interval arithmetic
+      // never consults it. Quarantining these would stop deliveries that are
+      // landing exactly when their author asked for them.
+      const r = await svc.saveReport({ name: 'A', object: 'lead', query: {} }, CTX);
+      seedScheduleRow(r.id, { cron_expression: null, interval_minutes: 60, timezone: BAD_TZ });
+
+      const result = await svc.dispatchDue();
+
+      expect(result.fired).toBe(1);
+      expect(email._sent).toHaveLength(1);
+      const stored = engine._tables['sys_report_schedule'][0];
+      expect(stored.last_status).toBe('ok');
+      expect(stored.next_run_at).toBe(new Date(now.getTime() + 60 * 60_000).toISOString());
+    });
+
+    it('dispatchDue: correcting the stored zone resumes the schedule with no other action', async () => {
+      // Why the quarantine leaves `active` set and `next_run_at` in the past:
+      // the row stays due, so the sweep picks it up again by itself.
+      const r = await svc.saveReport({ name: 'A', object: 'lead', query: {} }, CTX);
+      seedScheduleRow(r.id, { cron_expression: '0 9 * * *', timezone: BAD_TZ, format: 'csv' });
+
+      expect((await svc.dispatchDue()).failed).toBe(1);
+      engine._tables['sys_report_schedule'][0].timezone = 'Asia/Shanghai';
+
+      const result = await svc.dispatchDue();
+      expect(result.fired).toBe(1);
+      expect(email._sent).toHaveLength(1);
+      const stored = engine._tables['sys_report_schedule'][0];
+      expect(stored.last_status).toBe('ok');
+      // 09:00 Asia/Shanghai (UTC+8) on the 16th = 01:00Z — the instant its author
+      // actually asked for, not `now + 1440m`.
+      expect(stored.next_run_at).toBe('2026-01-16T01:00:00.000Z');
+    });
+
+    // ── The warning text: both paths, neither pointing at the wrong input ──
+
+    it('nextRunAt: the no-occurrence warning names the timezone as well as the cron', async () => {
+      const warn = vi.fn();
+      const logged = new ReportService({
+        engine: engine as any, email, clock: { now: () => now }, logger: { warn },
+        resolveOwnerContext: async (id: string) => ({ userId: id, positions: [], permissions: [] }),
+      });
+      const r = await logged.saveReport({ name: 'A', object: 'lead', query: {} }, CTX);
+      // 30 February never occurs; croner returns null rather than throwing.
+      await logged.scheduleReport({
+        reportId: r.id, recipients: ['x@t'], cronExpression: '0 0 30 2 *', timezone: 'Asia/Shanghai',
+      }, CTX);
+
+      const line = warn.mock.calls.map(c => String(c[0])).find(m => m.includes('no next occurrence'));
+      expect(line).toBeDefined();
+      expect(line).toContain("timezone 'Asia/Shanghai'");
+      expect(line).toContain("cron '0 0 30 2 *'");
+    });
+
+    it('nextRunAt: the un-evaluatable warning names the timezone and stops calling the cron invalid', async () => {
+      const warn = vi.fn();
+      const logged = new ReportService({
+        engine: engine as any, email, clock: { now: () => now }, logger: { warn },
+        resolveOwnerContext: async (id: string) => ({ userId: id, positions: [], permissions: [] }),
+      });
+      const r = await logged.saveReport({ name: 'A', object: 'lead', query: {} }, CTX);
+      // A row whose cron the create-time guard would have refused — the shape
+      // that reaches `nextRunAt` through `advanceSchedule` on a sweep.
+      seedScheduleRow(r.id, { cron_expression: 'not a cron', timezone: 'Asia/Shanghai' });
+
+      await logged.dispatchDue();
+
+      const line = warn.mock.calls.map(c => String(c[0])).find(m => m.includes('could not be evaluated'));
+      expect(line).toBeDefined();
+      expect(line).toContain("timezone 'Asia/Shanghai'");
+      expect(line).toContain("cron 'not a cron'");
+      // The old text asserted the expression was the broken half. On a timezone
+      // fault that accusation was simply false, and it is the reason this card
+      // treats the warning as part of the defect rather than as cosmetics.
+      expect(warn.mock.calls.map(c => String(c[0])).join('\n')).not.toContain('invalid cron');
+    });
+  });
+
   // ─── Authorization (#2980) ──────────────────────────────────────
   describe('access control', () => {
     const OTHER = { userId: 'u2', tenantId: 't1', positions: [], permissions: [] };