Repository navigation
fix(jobs): stale_timeout=0 means no grace period - #1623
Draft
dimitri-yatsenko wants to merge 1 commit into
Draft
dimitri-yatsenko wants to merge 1 commit into
dimitri-yatsenko wants to merge 1 commit into
Conversation
dimitri-yatsenko
requested review from
MilagrosMarin,
lum-agilyti and
ttngu207
October 17, 2026 23:45
refresh() removes a job whose key has left key_source once the job is older than stale_timeout, a grace period for a key_source that is still being filled. stale_timeout=0 skipped that check instead, so there was no way to ask for the cleanup now: refresh(stale_timeout=0) left every such job in place, and so did populate() with config.jobs.stale_timeout = 0. 0 now means no grace period: on that refresh, every job whose key is no longer in key_source is removed, whatever its age, unless it is marked ignore. That includes reserved jobs, and success records of rows that remain after their key left key_source, which a nonzero stale_timeout keeps for that many seconds. stale_timeout=None still means the configured value (config.jobs.stale_timeout, 3600 seconds by default), so a refresh that does not pass it keeps the grace period. The setting's description, "Seconds before pending job is checked for staleness", now describes the grace period and what 0 does. Skipping the check is no longer expressible as 0; a long stale_timeout keeps such jobs for as long as it says. The tests delete upstream rows and check, on MySQL and PostgreSQL, that a default refresh keeps the pending jobs of the deleted keys, that refresh(stale_timeout=0) removes them, and that populate() with config.jobs.stale_timeout = 0 removes them instead of trying to compute them. Each fails without the change. They replace the test that held 0 to its old meaning. Closes #1577.
dimitri-yatsenko
force-pushed
the
fix/stale-timeout-zero
branch
from
October 20, 2026 23:53
56c20d0 to
c64f100
Compare
This was referenced Oct 20, 2026
MilagrosMarin
approved these changes
Oct 21, 2026
MilagrosMarin
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Making 0 mean "no grace period" is the more natural reading, and the change is minimal: the age filter applies only when stale_timeout > 0, and the delete always runs. Both tests would fail under the old behavior: the first shows the default keeps vanished jobs and 0 removes them, and the second shows populate() with config.jobs.stale_timeout = 0 removes job 1 instead of leaving it pending.
Two non-blocking notes:
- This changes behavior for anyone who set
config.jobs.stale_timeout = 0to keep jobs whose key has leftkey_source: after upgrading, the nextpopulate()removes them, reserved ones included. Worth calling out in the release notes, with "set a largestale_timeout" as the way to get the old behavior back. - With
0, a job reserved by a worker that is still inmake()is removed too, so itscomplete()/error()lands after its row is gone. The key has leftkey_source, so the work is likely moot, but a sentence in thestale_timeoutdocstring would make that explicit.
(The "Stacked on #1595" line in the description is out of date: #1595 has merged and the base is master.)
dimitri-yatsenko
marked this pull request as draft
October 21, 2026 16:46
This branch has not been deployed
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.
refresh() removes a job whose key has left key_source once the job is older than stale_timeout, a grace period for a key_source that is still being filled. stale_timeout=0 skipped that check instead, so there was no way to ask for the cleanup now: refresh(stale_timeout=0) left every such job in place, and so did populate() with config.jobs.stale_timeout = 0.
0 now means no grace period: on that refresh, every job whose key is no longer in key_source is removed, whatever its age, unless it is marked ignore. That includes reserved jobs, and success records of rows that remain after their key left key_source, which a nonzero stale_timeout keeps for that many seconds. stale_timeout=None still means the configured value (config.jobs.stale_timeout, 3600 seconds by default), so a refresh that does not pass it keeps the grace period. The setting's description, "Seconds before pending job is checked for staleness", now describes the grace period and what 0 does.
Skipping the check is no longer expressible as 0; a long stale_timeout keeps such jobs for as long as it says.
The tests delete upstream rows and check, on MySQL and PostgreSQL, that a default refresh keeps the pending jobs of the deleted keys, that refresh(stale_timeout=0) removes them, and that populate() with config.jobs.stale_timeout = 0 removes them instead of trying to compute them. Each fails without the change. They replace the test that held 0 to its old meaning.
Closes #1577.
Stacked on #1595: its base is
fix/jobs-refresh-restrictionuntil that merges.