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

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

@@ -0,0 +1,26 @@
+---
+"@objectstack/plugin-reports": minor
+---
+
+fix(plugin-reports)!: a non-member schedule `timezone` no longer discards the cron expression, and a schedule already holding one stops instead of firing on a cadence nobody asked for (#16291)
+
+**BREAKING** for a deployment that already stores a report schedule with a cron expression and a `timezone` that is not an IANA member. Such a schedule is delivering today, on the wrong cadence; after this change it does not deliver at all until a human corrects the zone. It ships as `minor` under the lockstep launch-window convention (`scripts/check-changeset-no-major.mjs` refuses `major`); the version number is not the signal here, this entry is.
+
+<!-- adr-0087: not-required (no-migration-prescription) No metadata moves. No key is retired, no def is unpublished, no schema shape changes, and `sys_report_schedule`'s declaration is untouched apart from a comment — so `objectstack migrate meta`, `spec-changes.json` and the upgrade guide have nothing to rewrite and no ledger entry would have anything to say. The obligation this change creates is DATA-SIDE and operational, not a code rewrite: an operator checks `sys_report_schedule.last_status` for `failed` and corrects the row's `timezone`, and the schedule resumes on the next sweep by itself. That channel is the row the operator is already looking at, which is strictly more precise than a migration-chain entry about a metadata surface that did not change. -->
+
+## What an upgrading operator has to do, and how to find out
+
+If `sys_report_schedule` holds a row whose `timezone` is not a real IANA zone **and** whose `cron_expression` is set, the sweep now marks it `last_status: 'failed'` with a `last_error` naming the zone, and stops running it. Correct the `timezone` on that row; the schedule resumes on the next sweep with no re-enable and no second action, because `active` and the past `next_run_at` are deliberately left alone.
+
+Only rows written **before** `valueDomain: 'iana_time_zone'` landed on that column can be in this state, and the set cannot grow: measured on a real kernel with a real SQLite driver, `insert` into `sys_report_schedule` with `timezone: 'Mars/Olympus'` is already refused today — `VALIDATION_FAILED · Timezone must be a valid IANA time zone identifier, e.g. Europe/Zurich (got "Mars/Olympus")`. A set that cannot grow is still not an empty one, which is why this carries a banner rather than a shrug.
+
+## The defect
+
+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.
+
+## What changed
+
+- **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 door refuses; it 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. **This is not what makes the change breaking:** the storage door already refuses the same value today, so no reachable accept set narrows — what moves is which door answers and how clearly.
+- **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.** It is not run and its `next_run_at` is not advanced; `last_status` / `last_error` carry the reason. Repairing the value automatically was rejected: the intended zone is not recoverable from a typo, and rewriting it to `UTC` would deliver at yet another set of wrong instants while the row looked healthy. 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: [] };