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

Closed

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

  1. added theissue type on Sep 8, 2026
  2. os-zhuang commented on Sep 8, 2026

    @os-zhuang
    Contributor

    分诊:domain:services / Bug / priority:p2 / pm:queue

    域 —— packages/plugins/plugin-reports/src/report-service.ts ⇒ 按车道表 plugin-reports ⇒ domain:services。

    当刻复核(origin/main)—— 两处站点成立,并且告警有两条不是一条

    packages/plugins/plugin-reports/src/report-service.ts:606
      // Validate eagerly so an author gets a clear error at schedule time
    :609        new Cron(cron, { timezone: input.timezone || 'UTC' });          ← 无回调形式
    
    :819        const next = new Cron(cron, { timezone: schedule.timezone || 'UTC' }).nextRun(from);
    :821        this.logger?.warn?.(`ReportService: cron '${cron}' has no next occurrence; falling back to interval`);
    :823        this.logger?.warn?.(`ReportService: invalid cron '${cron}'; falling back to interval`, err);
    

    ⇒ 卡面所述成立。⭐ 且比卡面多一条:两条 warn 路径(:821 与 :823)都只点名 cron 表达式,两条都会在时区问题上指错方向。⇒ 修告警文案时两条都要改,⛔ 不要只改卡面引的那一条。

    ⭐ 这张卡的核心不是「少了一次校验」,是守卫在防的正是它自己制造的后果

    :606 的注释自陈其目的:

    Validate eagerly so an author gets a clear error at schedule time instead of a schedule that silently falls back to interval on sweep.

    而它用的是无回调的 new Cron(...)。按卡面实测的 croner 行为(10.0.1 / Node v22.22.2):

    new Cron('0 9 * * *', { timezone: 'Mars/Olympus' })                  -> 构造成功
      .nextRun(...)                                                      -> TypeError: CronDate: Failed to convert …
    new Cron('0 9 * * *', { timezone: 'Mars/Olympus' }, async () => {})  -> 构造即抛
    

    ⇒ ⭐ 无回调形式只校验了表达式,时区从这道守卫底下直接穿过去 —— 而它穿过去之后造成的,逐字就是这条注释说要防止的那件事(sweep 时静默回落到 interval)。

    后果 —— 卡面这一句是定级依据

    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.

    一份被作者写成「每工作日 09:00 Asia/Shanghai」的报表,变成「每 1440 分钟一次,永远」。而 nextRunAt 每次 sweep 都经 advanceSchedule 走到,schedule.timezone 由 rowFromSchedule 从存储行取出 ⇒ 不是一次性失败,是每次 sweep 都重复的持久错误行为。

    等级 p2

    • 活的、持续的错误行为(不是缺一次校验):报表在错误的时刻发送,无限期。
    • 静默:不抛、不回落到 UTC、唯一的痕迹是一条指向错误输入的 warn。⭐ 卡面对这一点的判断本席完全同意并加权:「A warning that names the wrong input is worse than none.」 —— 它会把调查者引向 cron 表达式,而那个表达式是好的。
    • 不到 p1:无数据损坏、无越权、无租户面;受影响的是发送时刻,且需要作者先写下一个非成员时区。

    ⚠️ #15872 关掉了什么、没关掉什么 —— 认领席必读

    卡面把这一段写得很干净,本席复核其结构并原样保留为范围说明:

    #15872 给 sys_report_schedule.timezone 加了 valueDomain: 'iana_time_zone' ⇒ 新行的写入路径会以 ADR-0114 的 value_domain 字段码拒绝非成员。它不修上面任何一个缺陷:

    1. 存量行永不被重读(transition-gate 那一类不回头看),所以它们继续回落到 interval,并继续带着那条指错方向的告警;
    2. scheduleReport 的守卫仍然是瞎的 —— sys_job.timezone and sys_report_schedule.timezone disagree with each other and neither is validated — the platform's own IANA columns predate valueDomain #15872 之后拒绝来自引擎 insert 的通用字段错误,而不是守卫自己那条刻意更清楚的 VALIDATION_FAILED 消息;
    3. 告警文案与时区无关地错着 —— 对任何 croner 在 nextRun() 而非构造时拒绝的输入都错。

    ⇒ ⛔ 不要因为 #15872 已落地就降级或关闭本卡。

    交给认领席

    • ⭐ 用同一个谓词,别指望构造函数:isValueDomainMember('iana_time_zone', tz) 由 @objectstack/spec/shared 导出,正是那个列现在用的同一个探针。⇒ 一个答案用在两处(列的写入门 + scheduleReport 的守卫)。⛔ 不要在 plugin-reports 里手写第二套时区判定。
    • 两条 warn 都要点名真正失败的东西(至少把 timezone 带进消息)。
    • ⭐ 认真考虑卡面的第三条建议:一个不可用的排程该被标记(last_status / last_error),而不是被静默地按一个没人要求过的节奏重新排。⚠️ 这一条会新增存储行上的状态写入 ⇒ 若它牵动 schema,拆出去另立卡;若只是写既有的 last_* 列,留在本卡。认领时先确认这两列是否已存在。
    • ⚠️ 存量行怎么办要有个明确答案:sys_job.timezone and sys_report_schedule.timezone disagree with each other and neither is validated — the platform's own IANA columns predate valueDomain #15872 之后写不进新的坏行,但已经在库里的坏行不会自愈。是让 nextRunAt 在遇到非成员时区时标记并停止重排,还是提供一次性的修复路径 —— 请在 PR 正文里写明选了什么,⛔ 不要默认「以后不会再有了」。

    溯源

    ⚠️ 卡面测于 origin/main dacb73f4f,croner 10.0.1 / Node v22.22.2。本席独立确认的是上面引的四行源码,⛔ 未复跑 croner 的那两行行为对照。卡面给了现成的重跑命令,认领时照做 —— 那个「构造成功 / nextRun() 才抛 / 带回调则构造即抛」的三态是本卡的全部机制依据:

    node -e "const {Cron}=require('croner'); const c=new Cron('0 9 * * *',{timezone:'Mars/Olympus'}); console.log('constructed'); c.nextRun()"
    

    相邻

    #15872(列的写入门,已落地,⛔ 不关闭本卡)· #16292(CronScheduleSchema.timezone 是裸 z.string() —— 同一个概念的第三扇门,domain:spec / Task / p3)。⇒ 三扇门(授权 schema、存储列、reports 的排程守卫)应当用同一个谓词;本卡与 #16292 同窗口排期为宜,⛔ 不合并(不同包、不同验收)。


    分诊席声明:本席只分类/定级/路由,⛔ 不认领、⛔ 不派工、⛔ 不写码、⛔ 不合并、⛔ 不裁决决策箱卡。


    Generated by Claude Code

  3. github-actions commented on Sep 8, 2026

    @github-actions
    Contributor

    os-closed-card-sweep — machine-findable marker for this generated comment.

    Removed the pm-loop state label(s) this closed card no longer claims: pm:dispatched.

    A state label claims work is in flight. This card is closed on a merged delivery, so the claim
    is stale; every other label is left exactly as it was found. Nothing here is a judgement about
    the card, and no verdict-bearing label is ever touched by this sweep.

    posted by half-state-patrol run 34270387866 · trigger schedule

    Generated by Claude Code

  4. added a commit that references this issue on Sep 9, 2026
    afa3a26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions