fix(github-runner): key per-run job cache by (repo, runID) - #7952
fix(github-runner): key per-run job cache by (repo, runID)#7952hontarenko wants to merge 2 commits into
Conversation
previousJobs was keyed by repository name only, but getWorkflowRunJobs is called once per individual workflow run. When a repository has multiple simultaneous queued/in_progress runs (e.g. a matrix-heavy release workflow plus a concurrent scheduled sync run), a 304 for one run's jobs URL could return whichever run's job list was cached last for that repo, silently double-counting one run's jobs in place of another's and inflating the computed GetWorkflowQueueLength() target. Key previousJobs by (repoName, workflowRunID) instead, update pruneCaches/evictExcess accordingly, and add a regression test covering two concurrent runs in the same repo both hitting 304. Fixes kedacore#7949 Signed-off-by: Serhii Hontarenko <serhii.hontarenko@amo.tech>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Thank you for your contribution! 🙏 Please understand that we will do our best to review your PR and give you feedback as soon as possible, but please bear with us if it takes a little longer as expected. While you are waiting, make sure to:
Once the initial tests are successful, a KEDA member will ensure that the e2e tests are run. Once the e2e tests have been successfully completed, the PR may be merged at a later date. Please be patient. Learn more about our contribution guide. |
|
Great, thanks for the fix. I've one suggestion on the data structure that I think simplifies things a bit. Instead of nesting The main benefit is that the existing generic |
Per review feedback: use a flat map[jobCacheKey][]Job (a small
{repo, runID} struct key) instead of nesting map[string]map[int64][]Job.
This lets the existing generic evictExcess helper bound previousJobs
directly, same as etags and previousWfrs, so the separate
evictExcessJobs helper is no longer needed.
Signed-off-by: Serhii Hontarenko <serhii.hontarenko@amo.tech>
|
Great, this is exactly what I was looking for. I will start the e2e right away. |
|
/run-e2e github* passed tests: 1failed tests: 0 |
rickbrouwer
added
the
Awaiting/2nd-approval
dttung2905
left a comment
dttung2905
left a comment
There was a problem hiding this comment.
Thanks @hontarenko for the help! Approved from me. Just two nits from me, both are non-blocking but more like improvement and hygiene
- the PR body stills mentioned nested maps and
eviecExcessJobscan you help to update the PR description to reflect the latest changes? - How about completed run IDs lingering after they leave queued/in_progress ?
3 participants
Summary
previousJobswas keyed by repository name only, butgetWorkflowRunJobsis called once per individual workflow run. When a repository has multiple simultaneous queued/in_progress runs (e.g. a matrix-heavy release workflow plus a concurrent scheduled sync run), a304for one run's jobs URL could return whichever run's job list was cached last for that repo — silently double-counting one run's jobs in place of another's and inflating the computedGetWorkflowQueueLength()target.This keys
previousJobsby(repoName, workflowRunID)instead, updatespruneCaches/evictExcessaccordingly (added anevictExcessJobshelper bounding total(repo, runID)entries), and adds a regression test covering two concurrent runs in the same repo both hitting304.Note:
previousJobsis intentionally not pruned by thecurrentReposlist inpruneCaches(unlikepreviousWfrs) — it's keyed by each run's ownRepository.Name, which isn't guaranteed to be an element ofcurrentRepos. It's still bounded byevictExcessJobs.Fixes #7949
Checklist