Skip to content

plugin-reports: a non-member timezone silently discards a report schedule's cron and fires it on the interval cadence forever — and the create-time guard cannot see it #16291

Description

@claude

Found while implementing #15872 (adding valueDomain: 'iana_time_zone' to sys_report_schedule.timezone). That card's first step was to measure what the reader does with a non-member zone; this is what the measurement turned up in the reader itself, and it is a defect in plugin-reports rather than in the column declaration, so it is carded here rather than widened into that PR.

The reading

Measured on origin/main at dacb73f4f, with croner 10.0.1 on Node v22.22.2 (the repo's baseline in this container).

croner does not reject an invalid IANA zone at construction — it throws from nextRun(), and only when there is no callback. Both halves matter and they are why this is easy to miss:

new Cron('0 9 * * *', { timezone: 'Mars/Olympus' })                    -> constructs FINE
  .nextRun(...)                                                        -> TypeError: CronDate: Failed to convert
                                                                          date to timezone 'Mars/Olympus' ...
new Cron('0 9 * * *', { timezone: 'Mars/Olympus' }, async () => {})    -> THROWS at construction

The callback form schedules internally, so it computes a next run and throws there.

1. The create-time guard is blind to the timezone half of its own input

packages/plugins/plugin-reports/src/report-service.ts, in scheduleReport:

// Validate eagerly so an author gets a clear error at schedule time
// instead of a schedule that silently falls back to interval on sweep.
try {
  new Cron(cron, { timezone: input.timezone || 'UTC' });
} catch (err) {
  throw new Error(`VALIDATION_FAILED: invalid cron_expression '${cron}': ...`);
}

That is the callback-less form, so it validates the EXPRESSION and nothing else. A bad timezone sails straight through the guard whose stated purpose is to prevent exactly the outcome it then produces.

2. nextRunAt swallows the throw and falls back to interval, blaming the wrong input

Same file, nextRunAt:

try {
  const next = new Cron(cron, { timezone: schedule.timezone || 'UTC' }).nextRun(from);
  ...
} catch (err) {
  this.logger?.warn?.(`ReportService: invalid cron '${cron}'; falling back to interval`, err);
}
const interval = schedule.interval_minutes ?? DEFAULT_INTERVAL_MIN;   // 1440
return new Date(from.getTime() + interval * 60_000);

nextRunAt is reached on every sweep through advanceSchedule, with schedule.timezone lifted off the stored row by rowFromSchedule. So a schedule carrying a non-member zone permanently drops its cron expression and fires on the interval cadence instead — a report authored as "every weekday 09:00 in Asia/Shanghai" becomes "every 1440 minutes, forever". The only trace is a warn that names the cron expression, which is fine, rather than the timezone, which is not.

Consequence

Neither of the two outcomes a reader would expect: it does not throw, and it does not fall back to UTC. It schedules at the wrong instants indefinitely, and the one log line it emits points an investigator at the wrong input.

What #15872 does and does not close

#15872 declares valueDomain: 'iana_time_zone' on sys_report_schedule.timezone, so a non-member is now refused on the WRITE path with the ADR-0114 field code value_domain. That closes the door for new rows. It does NOT fix either defect above:

Suggested shape (for the implementing seat to verify, not a ruling)

  • Validate the zone explicitly in scheduleReport, with its own message, rather than hoping the Cron constructor does it: the repo already exports the one membership predicate, isValueDomainMember('iana_time_zone', tz) from @objectstack/spec/shared, which is the same probe the column now uses. One answer in both places.
  • Make nextRunAt's warning name what actually failed, or at least include the timezone in the message. A warning that names the wrong input is worse than none.
  • Consider whether an unusable schedule should be marked (last_status / last_error) rather than silently re-scheduled at a cadence nobody asked for.

Re-check

sed -n '600,620p' packages/plugins/plugin-reports/src/report-service.ts
sed -n '812,830p' packages/plugins/plugin-reports/src/report-service.ts
node -e "const {Cron}=require('croner'); const c=new Cron('0 9 * * *',{timezone:'Mars/Olympus'}); console.log('constructed'); c.nextRun()"

Generated by Claude Code

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions