Repository navigation
fix(basics): a helper timeout too long for a deadline is refused - #3141
Merged
Merged
Conversation
A helper call's `timeout`, or `[helpers] timeout_secs`, so large that now plus it overflowed the clock made no deadline at all: the helper could run forever. Such a value is now refused naming it, a call's `timeout` when the call is made and `timeout_secs` when settings are read, as 0 already is.
Checking a timeout against the clock when it is read let a value just under the limit pass and overflow for a helper started later, and the limit moved with uptime. A timeout is now refused past u32::MAX seconds, about 136 years, which every platform's clock adds to now. A deadline that still does not fit refuses the helper instead of meaning none.
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.
Description
A helper's time limit is the call's
timeout, else[helpers] timeout_secs. Only 0 was refused. The runner computes each child's deadline asInstant::now().checked_add(timeout), andProcess::runreads aNonedeadline as "no deadline", so a value too large to add to now (such astimeout: 18446744073709551615) gave the helper no time limit at all.Now a timeout over a fixed ceiling is refused, naming the value and the ceiling, where 0 is refused: a call's
timeoutwhen the call is made (intimeout()), andtimeout_secswhen settings are read (adeserialize_withon the field, so the user learns at load time).The ceiling is
u32::MAXseconds (4294967295 s, about 136 years). It is fixed, not read from the clock:Instantholds at least ani64oru64count of seconds since an epoch near boot on every platform std supports (Linux, macOS, Windows), so adding 136 years to now always fits, whatever the uptime, and a value that passed at load also fits for a helper started later in a queue or chain. Any timeout a person means is far below it. The runner no longer reads a deadline that doesn't fit as "no deadline": shouldchecked_addever fail, the helper is refused before it starts.Review round 1 changed this from a check against the clock (
Instant::now().checked_add) at read time, which let a value just under the limit pass and then overflow for a helper started later, and moved with uptime.What users see change
timeouttoo large to make a deadline (for example18446744073709551615) is refused witha timeout of 18446744073709551615 s is over 4294967295 s, instead of running with no time limit. The ceiling is 4294967295 s (about 136 years); that value itself is accepted.[helpers] timeout_secsover the ceiling (for example4294967296) is refused when settings are read, with an error naming the value, astimeout_secs = 0already is.Tests
timeout, ortimeout_secs, means no deadlinehelpers::tests::a_timeout_too_long_for_a_deadline_is_refuseda call's timeout too long for a deadline: Ok(18446744073709551615s)(the tests-only commit; round 1 extended the test to check the ceiling itself is accepted and 4294967296 is refused, for the call and fortimeout_secs)One existing test's doc changed on purpose:
a_helpers_deadline_is_the_calls_timeout_or_the_settingsaid "however long", which is no longer true.Changelog
timeout, or[helpers] timeout_secs, over 4294967295 s (about 136 years) is refused naming the value instead of possibly meaning no time limit.Migration
None
Type of change
Testing
cargo nextest run -p rig-basics)cargo xtask verify --affected --base origin/main(box2)Checklist:
CHANGELOG.mdorMIGRATING.md(they are generated at release)Notes
Line deltas: production +24 / -4 (
plugins/rig-basics/src/helpers.rs), tests +23 / -2.Not verified: not driven in the real
rigbinary; macOS and Windows (CI runs Linux; the ceiling's fit on them is from std'sInstantrepresentation, not a run).Follow-ups
None.