fix(plugin-reports)!: a non-member schedule timezone no longer discards the cron expression - #16878
Conversation
…s 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
📓 Docs Drift CheckThis PR changes 2 package(s): 4 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 4 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin e45c10add784248f5ae15b4b0696960029e79082 && git checkout e45c10add784248f5ae15b4b0696960029e79082
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 25b07897fc6ab371637b7b5f3d3086a4c6a5165d deaa7b717998e46f95cff31a90ee1f49aebed7f3 && git checkout -B drift-repro 25b07897fc6ab371637b7b5f3d3086a4c6a5165d && git merge --no-ff deaa7b717998e46f95cff31a90ee1f49aebed7f3
node scripts/docs-audit/affected-docs.mjs --json 25b07897fc6ab371637b7b5f3d3086a4c6a5165d
|
…s ADR-0087 disposition The level was patch. Re-derived on two axes, and the second is what moves it. AXIS 1 -- accept-set narrowing -- DOES NOT HOLD, measured. `scheduleReport` refusing a non-member timezone looked like a narrowing of a published API, but the storage door already refuses the same value: on a real ObjectKernel with a real SqlDriver, `insert` into `sys_report_schedule` with `timezone: 'Mars/Olympus'` answers `VALIDATION_FAILED · Timezone must be a valid IANA time zone identifier` on origin/main today. No reachable accept set narrows; the refusal only moves to an earlier door with a clearer message, which is what the card asked for. AXIS 2 -- withdrawal of running behaviour -- DOES hold, and decides it. A row stored before `valueDomain: 'iana_time_zone'` landed, carrying a cron and a non-member zone, is delivering today on the interval cadence and delivers nothing after this change until a human corrects the row. The upgrade requires operator action on stored data to restore a delivery that is happening now, and nothing restores it automatically. During the launch window the level carries no breaking-ness at all -- the BREAKING banner and the ADR-0087 disposition are the only channels -- so a patch with neither would tell that operator there is nothing to look at. Level minor rather than major: `check-changeset-no-major` refuses major during the window. 226 minor changesets are already pending, so grading this honestly moves no release. Disposition is `not-required (no-migration-prescription)`: no metadata key, def or shape moves, so the migration chain has nothing to rewrite. The obligation is data-side -- `last_status` on the row the operator is already looking at. No behaviour was changed to fit the level. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
…eclaration `breakingDeclaration` reads three independent signals and the union is what CI records; the banner alone already declared it. The bang is added so the summary line carries the declaration too, matching the other declared-breaking changesets in stock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
Fixes #16291
The three-state probe, re-run on this container
Triage asked for the croner behaviour to be re-measured rather than inherited. croner
10.0.1, Nodev22.22.2:Confirmed exactly as the card states. That three-state answer is the whole mechanical basis of this change: the eager guard used the callback-less form, so it validated the expression and was blind to the zone, and
nextRunAtcaught the deferred throw and returnedfrom + interval_minutes. A report authored as "every weekday 09:00 Asia/Shanghai" fired every 1440 minutes, forever, re-derived on every sweep viaadvanceSchedule/rowFromSchedule— verbatim the outcome the guard's own comment says it exists to prevent.What changed
1. The create-time guard asks the right question, with the repo's one predicate.
scheduleReportnow consultsisValueDomainMember('iana_time_zone', …)from@objectstack/spec/shared— the same probesys_report_schedule.timezone'svalueDomainwrite gate uses since #15872. No second time-zone judgement was written inplugin-reports; the predicate is imported. The refusal isVALIDATION_FAILED: invalid timezone 'ZONE': not a member of the 'iana_time_zone' value domain, which names the input that is actually wrong.It applies whether or not a
cron_expressionis set, because the storage gate does too. A guard that accepted a value here for interval schedules would hand the engine a row it is about to refuse, and the divergence would surface as a generic field error — the exact regression #16291 records against #15872's door.The row now also stores the string the scheduler evaluates:
timezone: ''used to be stored verbatim while everynew Croncall site read it asUTCvia|| 'UTC', so the stored value was one the storage gate refuses and the scheduler silently treated as something else. It is normalised toUTCon the way in.2. Both warn paths, not one.
:821and:823each named only the cron expression. Both now name the expression and the zone, and the second no longer asserts the expression is the broken half:cron 'EXPR' has no next occurrence; falling back to intervalcron 'EXPR' (timezone 'TZ') has no next occurrence; falling back to intervalinvalid cron 'EXPR'; falling back to intervalcron 'EXPR' (timezone 'TZ') could not be evaluated; falling back to intervalA deliberate note on shape: an explicit
if (!isUsableTimezone(tz))branch insidenextRunAtwas written first and then removed. With the sweep quarantine below in place, nothing can reachnextRunAtwith a non-member zone — that branch was unreachable, i.e. a phantom check no test could ever drive red. The precise time-zone diagnosis lives where it is reachable (dispatchDue);nextRunAtnames both inputs and accuses neither, which is what makes both of its paths honest on either cause.3. Existing rows — the explicit answer triage asked for: MARK AND STOP, not repair.
#15872 refuses new bad rows at the column, and this PR refuses them at the service door, but neither heals a row already stored:
valueDomainis in the written-values-only transition-gate class, so pre-existing rows are never re-validated. On a sweep, a schedule that has acron_expressionand whose stored zone is not aniana_time_zonemember is now not run, itsnext_run_atis not advanced, andlast_status: 'failed'/last_errorcarry the zone, the expression and what to do about it.Why not the one-time repair path: the intended zone is not recoverable from a typo, and rewriting it to
UTCwould deliver at yet another set of wrong instants while the row looks healthy — trading a visible fault for an invisible one.Three sub-decisions inside that, each deliberate:
activeis left set andnext_run_atis left in the past. That is the same posture this loop already takes for a schedule whose report has vanished, and it means correcting the zone resumes the schedule on the very next sweep — no re-enable, no second admin action. (Covered by a test.)cron_expression. Interval arithmetic never reads the zone, so an interval-only schedule carrying a legacy bad value still delivers on the cadence its author asked for and is left completely alone. (Covered by a test.)failed++/last_status: 'failed', matching this loop's existing fail-closed arm for an unresolvable owner context, rather than a new status value — see the scope note below.4.
nextRunAtstays pure. It computes an instant; it does not decide policy and cannot write a row. The mark-and-stop decision lives indispatchDue, where the row and the engine are in hand.Changeset level — re-derived, with the measurement that overturned my first answer
The changeset was graded
patch. It is nowminor, declared breaking, with an ADR-0087 disposition. Two candidate axes were put to it; they did not both survive.Axis 1 — "it refuses input it used to accept" — DOES NOT HOLD. Measured.
This looked like the obvious one, and it is wrong.
scheduleReportrefusing a non-member zone is only a real narrowing if a non-member zone could reach storage today. It cannot. Booted on a realObjectKernelwith the realObjectQLPluginand a realSqlDriverover SQLite, inserting the row directly:So on
origin/maintoday,scheduleReportwith a bad zone already throws, and already throwsVALIDATION_FAILED. This PR does not narrow a reachable accept set: it moves the refusal to an earlier door and gives it a message that names the timezone instead of a generic field error — which is what the card asked for, and is not a breaking act. The one honest residual is error shape, not accept set: the engine's error carriescode: 'VALIDATION_FAILED'while the guard throws a bareErrorwhose message beginsVALIDATION_FAILED:— the same spelling the other three guards inscheduleReportalready use, so it is this door's existing convention rather than a new divergence.Axis 2 — "schedules that run today stop running" — DOES hold, and it decides.
A row stored before
valueDomainlanded, carrying a cron expression and a non-member zone, is delivering right now on the interval cadence. After this change it delivers nothing until a human corrects the row. The upgrade requires operator action on stored data to restore a delivery that is happening today, and nothing restores it automatically.That is the axis I am calling decisive, and I want to be explicit that it is not the "was the old behaviour ever promised?" axis. It was not promised — the object's own doc says a cron expression "is evaluated in
timezone", so the interval fall-back was already a contract violation, and a pure correctness reading lands onpatch. I did not take that reading, for one reason: the remedy is not automatic. Compare an RLS fix that stops returning rows a caller should never have seen — correct behaviour resumes by itself. Here it does not; a human must edit a row, and until they do a report an operator relies on is silently absent. During the launch window the level carries no breaking-ness at all (check-changeset-no-majorrefusesmajor, and the repo's own text says the BREAKING banner plus the ADR-0087 disposition are "the only signal there is"). Apatchwith neither would tell that operator there is nothing to look at.What would falsify this call: if the quarantine were self-healing without human action (a fall back to UTC, or a repair migration), or if no stored row could be in this state. Neither holds — #15872's own comment states the column is written-values-only and that "no migration is owed", so pre-existing non-members survive by design. The set cannot grow (axis 1's probe proves the write door is shut), but a set that cannot grow is not an empty one.
The gates, and what they did and did not say
Because that last line is a non-reading rather than a pass, the level axis was then driven offline with
--event, both ways, so the grade does not depend on which way clause ② is read:patchgrade:Both were green on
patch. The ADR-0087 gate is driven off the author's own declaration by design — its header says silence is the single thing it forbids, and a changeset declaring nothing has nothing demanded of it. And the level axis could not see this package at all:PUBLISHED_SOURCE_PATH = /^packages\/([^/]+)\/src\//matches one segment, sopackages/plugins/plugin-reports/src/**is invisible to it (measured here: 47 of 70 publishable workspace packages are nested deeper than one segment). That blind spot is already filed as #16713 and is not re-filed here; this PR is a corroborating data point for it, nothing more.So the correction rests on the argument above, not on a red gate — which is the right way round, and is why it is written out rather than asserted.
last_status/last_error— checked first, as directedBoth columns already exist on
sys_report_schedule(packages/platform-objects/src/audit/sys-report-schedule.object.ts, groupState), andReportSchedule/rowFromSchedulealready carry them.markSchedulealready writes them on two other arms. So marking stays in this card and moves no schema.What was deliberately kept out to avoid moving schema:
last_statusisField.select(['ok', 'failed', 'skipped']). A more precise value such as'misconfigured'would be a new enum member — that is a schema move, and per triage's instruction it is not taken here.'failed'is used instead, with the precision carried inlast_error.Also out of scope, and not filed: this arm re-marks the row on every sweep, because the row stays due. That write amplification is pre-existing and identical in kind to the missing-report arm three lines below it, so making only the new arm idempotent would leave the function inconsistent. Noted for a maintainer, not carded.
Adjacent cards — left alone, as directed
#15872 (landed, the column's write door) and #16292 (
CronScheduleSchema.timezoneis a barez.string(),domain:spec) are not closed or folded in here. Three doors, one predicate, three cards.One sibling-package edit, and why
packages/platform-objects/src/audit/sys-report-schedule.object.tscarries a present-tense paragraph stating thatscheduleReport's guard "does not catch it … and so is blind to exactly this half of its own input". This PR is what makes that false, so the sentence is moved to past tense and a[#16291]note records how each half is now closed. Comment-only; that package's full suite (545 tests) and typecheck were run.Verification
The gate union,
pnpm lintand both suites were re-run in full ondeaa7b717, the final commit. The ablation legs were run ondf9d4c9b9;report-service.tsis byte-identical at both (blobd11eff8969637d2e5737feea308adfdec8411d74), the only difference between the two heads being the changeset file.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands; the union is the same 59 families before and after the changeset edit; all 59 run; reconciled with--ran:✓ 59 derived famil(ies) accounted for — 59 run, 0 NOT-MEASURED.Every one exits 0.PREREQUISITE NOT MET(exit 3) and were NOT reported as passes.check:dual-build-cjs-loadsandcheck:i18nboth named a missing build; a fullpnpm build(73/73 tasks) was run and both were re-run to a real verdict —104 published require entry point(s) across 67 package(s) loadand9 package(s) — all bundles in sync.check:type-check-debtOOM'd at 4 GB (also exit 3, also not a pass); re-run at 8 GB it reports5 ledger entr(ies) re-measured, none above its recorded number.check:route-enveloperun explicitly (it lands in the deriver's silent bucket, dispatch-gates: a whole-tree-walk gate whose workflownames:lists only its CURRENT members is placed Silent, so it is never derived for the card that adds a new member — measured on check:route-envelope / PR #16730 #16828): exit 0. This diff adds no route module —grep -rn 'c\.json\|res\.json' packages/plugins/plugin-reports/srcis empty — so it was never in scope, but it was run rather than assumed.check:error-code-casingrun explicitly for the same reason (its roster sits underpackages/): exit 0.pnpm lint— the repo-wide run, not a narrowed one:eslint . --no-inline-config, exit 0.@objectstack/plugin-reports88 passed (5 files, 9 new),@objectstack/platform-objects545 passed (37 files); both packages'typecheckgreen including their test layers viacheck:test-typecheck.check:test-source-aliascaught a real defect in this PR's first draft and it was fixed, not routed around. The new@objectstack/spec/sharedimport is a value import (the type-only imports beside it are erased), so unaliased it resolved tospec'sdist/— the timezone tests would have been a verdict about build state, and adistmerely behind would have passed them against the old membership answer silently.vitest.config.tsgains the anchored array-form rule for spec's uniform namespace map, the same shapepackages/metadataandpackages/runtimecarry. Suite re-run against source afterwards: still 88/88.Ablation — four enforcement points, each mutated alone
Each leg: mutate → prove it reached disk (
grep -con both the removed anchor and the injected text, plusgit hash-objectdiffering from the HEAD blob) → run → restore withgit checkout HEAD -- ABSPATHundertrap ... EXIT INT TERM→ prove restoration by an emptygit diff HEADand blob equality. HEAD blob ofreport-service.ts:d11eff8969637d2e5737feea308adfdec8411d74.if (!isUsableTimezone(timezone))→if (false)31eae4c9…refuses a non-member timezone instead of storing it;… with no cron_expression tooif (false && …)6d51a175…a stored non-member timezone stops the schedule instead of rescheduling it;correcting the stored zone resumes the schedule …invalid cron 'EXPR'textee03b80c…the un-evaluatable warning names the timezone and stops calling the cron invalidda16a4ef…the no-occurrence warning names the timezone as well as the cronAll four restored byte-identically (
restored blob = d11eff89… = head,git diff HEADempty); the final whole-treegit diff HEAD --statprinted nothing.The subject of every leg is
plugin-reports' own source, imported by the suite relatively (./report-service.js) and with@objectstack/spec/sharednow aliased to source — so no leg's verdict is a function of anydist/, which is the conditionscripts/ablation-dist-preflight.mjsguards.Clause ② — re-derived mechanically from the delivered diff
The claim comment declares
Clause-②: no. Re-derived on the final diff, not inherited:Consistent with the reading by hand: no newly exported symbol (
effectiveTimezone/isUsableTimezoneare module-private), no new error code (VALIDATION_FAILEDis this file's existing prefix), no new key on any stored or returned shape (last_status/last_error/failedall pre-exist), and no newlast_statusenum member. The direction is narrowing throughout. The declaration was not edited by this seat, and the level correction above does not depend on it — the level axis was driven both ways and passes either.验收备注
last_errorsays what to correct and why the changeset carries a banner.check:route-envelope's absence from the derived union is dispatch-gates: a whole-tree-walk gate whose workflownames:lists only its CURRENT members is placed Silent, so it is never derived for the card that adds a new member — measured on check:route-envelope / PR #16730 #16828, already tracked. Corroborated, not re-filed: the level axis's nested-package blind spot is [finding] The changeset LEVEL axis is blind to every NESTED package:packages/*/src/**matches one segment, so 51 of 74 workspace packages (all drivers/services/adapters) can pairClause-②: yeswithpatchand stay green #16713.🤖 Generated with Claude Code
https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37