Repository navigation
fix(config): correct BlockTime cron evaluation and stop handling - #7082
Open
halibobo1205 wants to merge 7 commits into
Open
halibobo1205 wants to merge 7 commits into
halibobo1205 wants to merge 7 commits into
Conversation
For blocks at least 60 s old, Manager.applyBlock dereferenced the result of getNextValidTimeAfter(blockTime - maxFlushCount * 3 s) without a null check. Once the query reaches the final match of a finite cron expression, it returns null and block processing fails with a NullPointerException. With the default maxFlushCount = 1, this can occur on the first historical block after the final match. On the sync path, the exception disconnects the peer with BAD_BLOCK and can stall synchronization before the planned stop. Skip the flush adjustment when no fire time is left. The stop condition is unchanged. The new stop tests check the stop signal and persisted block height, not a real process exit.
MAX_YEAR came from Calendar.getInstance() when the class was first initialized, so it depended on the default locale at that moment. Under ja_JP_JP it is the era year plus 100, so every expression with an omitted year field or a wildcard year fails to parse. Under th_TH the limit is 643 years ahead instead of 100. Use GregorianCalendar, as getTimeAfter already does. Run each locale case in a fresh JVM so the class is first initialized under that default locale. Local patch: Quartz 2.5.2 and main still use Calendar.getInstance(), and no upstream issue was found as of 2026-11-04. Remove it when an upstream release uses the Gregorian year for MAX_YEAR regardless of the default locale's calendar.
Backport selected CronExpression fixes from Quartz 2.5.1 and replace the nearest-weekday (W) lookup with a local one. This is not a plain sync: the vendored parser/evaluator is based on Quartz main 7b3b88bad8, whose CronExpression implementation matches v2.5.0, and the W code of 2.5.2 is deliberately not taken. Taken from Quartz 2.5.1: - quartz-scheduler/quartz#1284: "L" and "L-n" resolve to the real last day of short months, fixing the quartz-scheduler/quartz#1175 regression. - quartz-scheduler/quartz#1400: reject 'L' in the seconds, minutes and hours fields. - quartz-scheduler/quartz#1346, partly: ">= 60" and ">= 24" in the seconds, minutes and hours increment messages. The other fields keep "> 31", "> 12" and "> 7", because upstream's ">=" is wrong for them. Not taken: - quartz-scheduler/quartz#1424 (2.5.2): it can still skip a weekday moved forward, keeps the start day's time of day on a later W day, and leaves next and isSatisfiedBy disagreeing. - quartz-scheduler/quartz#1350: getTimeBefore is on an API the vendored class removed. - 464ca5cea4 (quartz-scheduler/quartz#1517), not included in Quartz 2.5.1 or 2.5.2: getTimeAfter can fail to terminate for "0 15 2 * * ?" in Australia/Lord_Howe after 2027-10-01T15:45Z. Local patch, findNearestWeekday and getNearestWeekday: look configured days up from two days before the current day, because a weekday moves at most two days, and take the first whose weekday is on or after the current day. This fixes the covered UTC month-skipping and W-list cases and avoids the Quartz 2.5.2 time-of-day regression. Each returned time in those regression tests also satisfies isSatisfiedBy. For example, "0 0 0 1W 5 ? 2027" passes the startup check and matches 2027-05-03. Behaviour change outside UTC, not a fix: on a DST fall-back a W day now follows the plain day-of-month rule, as reported for plain days in quartz-scheduler/quartz#1138. "0 40 23 30W 9 ? 1978" in Asia/Amman after 1978-09-29T20:40Z returned 21:40Z and now returns null. Upstream tracking: quartz-scheduler/quartz#1420, quartz-scheduler/quartz#1444 and the quartz-scheduler/quartz#1424 time-of-day regression, for which no issue was found as of 2026-11-04. Remove the local lookup when an upstream release passes the W tests here with isSatisfiedBy asserted. Take upstream's increment text once it says ">" for those fields.
findSmallestDay compared a plain day with the slot index of an "L-n" entry, which equals the real day only in 31-day months. In February a later plain day won: "0 0 12 L-3,26 2 ? 2022" skipped 2022-02-25 and fired first on the 26th. Convert the entry to its calendar day before comparing. This adds no syntax. Local patch: Quartz 2.5.2 and main compare the same way and no upstream issue was found as of 2026-11-04. Remove it when an upstream release selects the earliest actual calendar day in mixed plain-day and L-n lists.
node.shutdown.BlockTime was interpreted in the JVM default time zone although it is matched against block timestamps, so the same value matched 8 hours apart on a UTC node and an Asia/Shanghai node. Pin the expression to UTC in Args, and document in reference.conf and config.conf how the stop matches a block. The exact block-timestamp match and persistence requirements are unchanged. Compatibility: operators using a non-UTC JVM should review the value and adjust it if they need to preserve the previous firing times. Interpreting it in UTC can leave a finite expression with no match after the head block time; the existing startup check then rejects it with AUTO_STOP_PARAMS.
Log up to three upcoming cron matches for node.shutdown.BlockTime at startup, in UTC with the weekday, counted from the head block time. They are candidates only: the node stops when a block timestamp exactly matches a cron time, which is not guaranteed. This helps an operator notice a misread expression but does not block it: "1O" is still read as 1. Treat NoSuchElementException from initAutoStop as AUTO_STOP_PARAMS. The accepted syntax is unchanged.
halibobo1205
requested review from
317787106,
bladehan1 and
waynercheung
and removed request for
317787106
November 4, 2026 10:51
processBlock checked the stop height before taking blockLock, so on the Netty path a block could pass the check and then wait for the lock. If the block ahead of it persisted the target height, the waiting block moved the persisted height past the target: the exact-match check then never held, and the node did not stop. A waiting block could also advance the persisted height after the stop signal. Run the check under blockLock, right before the block is pushed. The exact match is unchanged. The new tests force this overlap on a real Manager; they do not measure how often it happens on mainnet.
waynercheung
approved these changes
Nov 4, 2026
waynercheung
left a comment
waynercheung
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Remaining items are non-blocking:
[NIT] Please track the UTC behavior change for the v4.8.3 release notes and carry over the migration guidance from "Extra details": on a non-UTC JVM an existing expression can fire at a different time, and a finite expression can lose all future matches and be rejected at startup.
Other points are inline.
| return smallestDay == Integer.MAX_VALUE ? Optional.empty() : Optional.of(smallestDay); | ||
| } | ||
|
|
||
| private int findNearestWeekday(int day, int mon, int year) { |
Collaborator
There was a problem hiding this comment.
[SHOULD] Please document the two invariants behind this lookup: a W adjustment moves at most two days, and the adjusted day is non-decreasing in the nominal day within a month. They justify the two-day look-back and returning the first result >= day, and should stay visible in future Quartz syncs.
| addToSet(ALL_SPEC_INT, -1, incr, type); | ||
| return i; | ||
| } else if (c == 'L') { | ||
| if (type < DAY_OF_MONTH) { |
Collaborator
There was a problem hiding this comment.
[QUESTION] Will the strict-parsing follow-up also cover L in the month and year fields? Both parse today: month L (0 0 0 1 L ?) throws NoSuchElementException on evaluation, while year L (0 0 0 1 * ? L) returns null and is rejected as having no future match. Non-blocking.
| cronExpression.setTimeZone(TimeZone.getTimeZone("UTC")); | ||
| Date next = cronExpression.getNextValidTimeAfter( | ||
| Date.from(Instant.parse("2026-06-01T00:00:00Z"))); | ||
| System.out.print((CronExpression.MAX_YEAR - new GregorianCalendar().get(Calendar.YEAR)) |
Collaborator
There was a problem hiding this comment.
[NIT] Please add a short comment that stdout carries the child-JVM result read by the parent test, so it is not mistaken for leftover debug output.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes the optional
node.shutdown.BlockTimeauto-stop:NullPointerException.L-nlist and nearest-weekday (W) calculations, and makes the year limit independent of the default locale's calendar.BlockTimein UTC and documents its block timestamp matching behavior.NoSuchElementExceptionfailures asAUTO_STOP_PARAMS.blockLock, right before a block is pushed, so overlapping blocks on the P2P path cannot skip the exact target height. This also applies toBlockHeight.Selected Quartz 2.5.1 fixes are backported. Upstream provenance and removal criteria for the local
CronExpressionpatches are documented in the commit messages. No additional local strict-validation checks are introduced.Why are these changes required?
Valid schedules can currently be rejected or skipped. For example,
0 0 0 L 4 ? 2027returns no next fire time despite April 30 being valid.Wcalculations can skip a weekday moved forward to Monday or a later entry in a list. Quartz 2.5.2 still fails theseWcases, so this PR uses a local lookup.An exhausted finite schedule can also cause historical block processing to dereference a null next fire time. On the sync path, this can fail block application and disconnect the peer with
BAD_BLOCK. This can stall synchronization before the stop condition is reached. The regression tests reproduce the local processing failure; a real sync was not run.processBlockchecked the stop height before takingblockLock. A block that passed the check could then wait for the lock and move the persisted height past the target, so the exact-match check never held and the node did not stop. The regression tests force this overlap on a realManager; they do not measure how often it happens on mainnet.The JVM's default time zone and calendar previously affected scheduling. Explicit UTC evaluation and Gregorian year limits make the configured schedule predictable.
This PR has been tested by:
Unit Tests
Manual Testing
Follow up
W,L-nand increment-message fixes upstream.1OandMONDAY-FRIDAYremain accepted; previews help operators notice them but do not reject them.Extra details
BlockTimeis commented out by default. Operators who use it should review these behavior changes for release notes:AUTO_STOP_PARAMS.Wschedules can trigger on dates previously skipped.Lin seconds, minutes or hours is now rejected during configuration parsing, following Quartz 2.5.1. The year limit now uses the Gregorian current year + 100.logs/tron.logunder the default logging configuration. Candidates are counted from the head block time and may be earlier than wall-clock time on a lagging node.Stopping still requires an actual matching block timestamp and persistence of the target height. Missed slots are not caught up, so a cron candidate does not guarantee a stop.