fix: add automatic queue self-healing and retry unblocking - #97
Conversation
📝 WalkthroughWalkthroughThis PR extends the build queue recovery workflow by detecting stale jobs that lack build records, checking DockerHub for images from "started" builds, and reserving scheduling capacity for retries to prevent fresh job starvation while cleanup operations run. ChangesBuild queue recovery and retry capacity management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
functions/src/cron/index.tsOops! Something went wrong! :( ESLint: 10.3.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by functions/src/logic/buildQueue/cleaner.tsOops! Something went wrong! :( ESLint: 10.3.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by functions/src/logic/buildQueue/ingeminator.tsOops! Something went wrong! :( ESLint: 10.3.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
frostebite
changed the title
f299468 to
0db4af7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
functions/src/logic/buildQueue/scheduler.ts (1)
167-195:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftFix double-counting of capacity in
determineOpenSpotsForFreshJobs().The retried jobs are counted twice when determining available slots for fresh jobs. Here's why:
- Line 173 snapshots
openSpots = maxConcurrentJobs − getNumberOfScheduledJobs()before retries are dispatched.ingeminator.rescheduleFailedJobs()callsCiJobs.markJobAsScheduled(jobId)for each retried job (ingeminator.ts:110), transitioning them from status'failed'to status'scheduled'.- Line 183 sets
reservedRetrySlots = Math.min(scheduledRetries, openSpots)using the pre-retry snapshot.- Line 193 calls
determineOpenSpotsForFreshJobs(), which re-invokesdetermineOpenSpots()(scheduler.ts:262). SincegetNumberOfScheduledJobs()counts jobs wherestatus IN ['scheduled', 'inProgress'](ciJobs.ts:122), the retried jobs are now included in the fresh count.- Then it subtracts
reservedRetrySlots(scheduler.ts:263), double-debiting the same capacity.Example: if 5 jobs are retried and
openSpotswas 10:
reservedRetrySlots = 5- Line 193:
openSpots_fresh = maxConcurrent − (previous_scheduled + 5)← retried jobs counted here- Line 263:
availableForFresh = openSpots_fresh − 5← same 5 subtracted again- Result: fresh jobs lose 10 slots instead of 5.
Solution: Either (a) use the pre-retry
openSpotssnapshot indetermineOpenSpotsForFreshJobs()instead of re-querying, or (b) remove thereservedRetrySlotssubtraction since the retried jobs are already accounted for in the fresh count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@functions/src/logic/buildQueue/scheduler.ts` around lines 167 - 195, The code double-counts retried jobs because ensureThereAreNoFailedJobs() computes openSpots before calling Ingeminator.rescheduleFailedJobs(), then later calls determineOpenSpotsForFreshJobs() which re-reads scheduled jobs (including the newly-marked retried ones) and also subtracts reservedRetrySlots; fix by passing the pre-retry snapshot into determineOpenSpotsForFreshJobs() (or adding an optional parameter) and use that value instead of re-calling determineOpenSpots(), i.e., preserve the openSpots captured in ensureThereAreNoFailedJobs() and compute availableForFresh = preRetryOpenSpots - reservedRetrySlots (or simply return preRetryOpenSpots when reservedRetrySlots is zero) so retried jobs are only debited once; update references to determineOpenSpotsForFreshJobs(), determineOpenSpots(), reservedRetrySlots, and ensureThereAreNoFailedJobs() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@functions/src/logic/buildQueue/cleaner.ts`:
- Around line 52-57: The check-then-reset is racy: between
CiBuilds.hasAnyBuildsForJob(jobId) and CiJobs.resetJobToCreated(jobId) a build
can be inserted; change to an atomic operation by moving the existence check
into the same DB transaction or by adding a conditional reset method (e.g.,
CiJobs.resetJobToCreatedIfNoBuilds(jobId) or use a DB UPDATE ... WHERE job_id =
? AND NOT EXISTS(SELECT 1 FROM builds WHERE job_id = ?) ), ensure this method
returns whether the reset actually happened and only then increment
this.buildsProcessed and call Discord.sendAlert(jobId, ...); remove the separate
pre-check to avoid the race.
In `@functions/src/logic/buildQueue/scheduler.ts`:
- Around line 193-194: The relaxed healthy check now allows fresh jobs when
openSpotsAfterRetries > 0 even if failingJobs.length > maxToleratedFailures;
update the logic in the method containing determineOpenSpotsForFreshJobs so the
behavior is explicit: either revert to the original strict blocker by returning
failingJobs.length <= maxToleratedFailures (use && with any other checks) or
introduce a named configuration/flag (e.g., allowFreshWhenFailuresHigh) and use
it to gate the OR condition; also add a short comment referencing
scheduleBuildsFromTheQueue to document that this decision controls whether new
jobs are scheduled when failures exceed maxToleratedFailures.
In `@functions/src/model/ciJobs.ts`:
- Around line 129-134: The getActiveJobs method currently caps results with
.limit(settings.maxConcurrentJobs), which can hide active jobs beyond the
scheduler concurrency and prevent recovery; remove the .limit(...) call (or
replace it with an unbounded/paginated fetch) in static getActiveJobs so the
query on CiJobs.collection for status in ['scheduled','inProgress'] returns all
active jobs (or use cursor-based pagination if necessary) to ensure all stale
jobs can be discovered and requeued.
---
Outside diff comments:
In `@functions/src/logic/buildQueue/scheduler.ts`:
- Around line 167-195: The code double-counts retried jobs because
ensureThereAreNoFailedJobs() computes openSpots before calling
Ingeminator.rescheduleFailedJobs(), then later calls
determineOpenSpotsForFreshJobs() which re-reads scheduled jobs (including the
newly-marked retried ones) and also subtracts reservedRetrySlots; fix by passing
the pre-retry snapshot into determineOpenSpotsForFreshJobs() (or adding an
optional parameter) and use that value instead of re-calling
determineOpenSpots(), i.e., preserve the openSpots captured in
ensureThereAreNoFailedJobs() and compute availableForFresh = preRetryOpenSpots -
reservedRetrySlots (or simply return preRetryOpenSpots when reservedRetrySlots
is zero) so retried jobs are only debited once; update references to
determineOpenSpotsForFreshJobs(), determineOpenSpots(), reservedRetrySlots, and
ensureThereAreNoFailedJobs() accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b9b1778-48b7-49d8-86b9-0dc36f404cbb
📒 Files selected for processing (7)
functions/src/cron/index.tsfunctions/src/logic/buildQueue/cleanUpBuilds.tsfunctions/src/logic/buildQueue/cleaner.tsfunctions/src/logic/buildQueue/ingeminator.tsfunctions/src/logic/buildQueue/scheduler.tsfunctions/src/model/ciBuilds.tsfunctions/src/model/ciJobs.ts
| const hasBuilds = await CiBuilds.hasAnyBuildsForJob(jobId); | ||
| if (hasBuilds) continue; | ||
|
|
||
| this.buildsProcessed += 1; | ||
| await CiJobs.resetJobToCreated(jobId); | ||
| await Discord.sendAlert( |
There was a problem hiding this comment.
Make stale-job requeue atomic.
Line 52 and Line 56 are separate operations. A late build record can land between them, and the job can be reset to created even though it now has builds.
Suggested direction
- const hasBuilds = await CiBuilds.hasAnyBuildsForJob(jobId);
- if (hasBuilds) continue;
-
- this.buildsProcessed += 1;
- await CiJobs.resetJobToCreated(jobId);
+ const reset = await CiJobs.resetJobToCreatedIfStillBuildless(jobId);
+ if (!reset) continue;
+ this.buildsProcessed += 1;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@functions/src/logic/buildQueue/cleaner.ts` around lines 52 - 57, The
check-then-reset is racy: between CiBuilds.hasAnyBuildsForJob(jobId) and
CiJobs.resetJobToCreated(jobId) a build can be inserted; change to an atomic
operation by moving the existence check into the same DB transaction or by
adding a conditional reset method (e.g.,
CiJobs.resetJobToCreatedIfNoBuilds(jobId) or use a DB UPDATE ... WHERE job_id =
? AND NOT EXISTS(SELECT 1 FROM builds WHERE job_id = ?) ), ensure this method
returns whether the reset actually happened and only then increment
this.buildsProcessed and call Discord.sendAlert(jobId, ...); remove the separate
pre-check to avoid the race.
| const openSpotsAfterRetries = await this.determineOpenSpotsForFreshJobs(); | ||
| return failingJobs.length <= maxToleratedFailures || openSpotsAfterRetries > 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find callers of ensureThereAreNoFailedJobs to see how the boolean is consumed.
rg -nP --type=ts -C3 '\bensureThereAreNoFailedJobs\s*\('Repository: game-ci/versioning-backend
Length of output: 1389
🏁 Script executed:
# Search for all references to ensureThereAreNoFailedJobs more broadly
rg -n 'ensureThereAreNoFailedJobs' --type=ts -B2 -A2Repository: game-ci/versioning-backend
Length of output: 1058
🏁 Script executed:
# Look at the full context of the scheduler function to understand openSpotsAfterRetries
sed -n '160,200p' functions/src/logic/buildQueue/scheduler.tsRepository: game-ci/versioning-backend
Length of output: 1811
🏁 Script executed:
# Check if there are other files that might reference this function
fd --type=ts -x grep -l 'ensureThereAreNoFailedJobs' {} \;Repository: game-ci/versioning-backend
Length of output: 240
🏁 Script executed:
# Search for cron-related code
rg -n 'cron\|orchestrat' --type=ts -iRepository: game-ci/versioning-backend
Length of output: 52
🏁 Script executed:
# Search for scheduler instantiation and other method calls to understand broader context
rg -n 'scheduler\.' --type=ts -C1 | head -100Repository: game-ci/versioning-backend
Length of output: 2064
🏁 Script executed:
# Search for where scheduleBuildsFromTheQueue is called
rg -n 'scheduleBuildsFromTheQueue' --type=ts -B2 -A2Repository: game-ci/versioning-backend
Length of output: 1593
🏁 Script executed:
# Look for cloud functions or entry points
rg -n 'export.*function\|exports\.' --type=ts | grep -E '(schedule|build|queue)' | head -20Repository: game-ci/versioning-backend
Length of output: 52
🏁 Script executed:
# Look at the comment/documentation around ensureThereAreNoFailedJobs more closely
sed -n '160,167p' functions/src/logic/buildQueue/scheduler.tsRepository: game-ci/versioning-backend
Length of output: 353
🏁 Script executed:
# Check the determineOpenSpotsForFreshJobs method to understand what it counts
rg -n 'determineOpenSpotsForFreshJobs' --type=ts -A10Repository: game-ci/versioning-backend
Length of output: 1997
Clarify the intent of the relaxed "healthy" condition.
The return condition has been relaxed from strictly failingJobs.length <= maxToleratedFailures to now also allow returning true when openSpotsAfterRetries > 0. This means the function permits fresh job scheduling even when failures significantly exceed maxToleratedFailures, as long as capacity remains after reserving slots for retries.
While the retry logic still executes before this check, the downstream gate in scheduleBuildsFromTheQueue (the sole caller) will now proceed to schedule new jobs in this scenario, whereas the prior strict condition would have blocked scheduling entirely. Confirm this behavior aligns with the intended queue balancing strategy—particularly whether allowing fresh jobs when failures are high is acceptable, or if the failure tolerance threshold should be a hard blocker.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@functions/src/logic/buildQueue/scheduler.ts` around lines 193 - 194, The
relaxed healthy check now allows fresh jobs when openSpotsAfterRetries > 0 even
if failingJobs.length > maxToleratedFailures; update the logic in the method
containing determineOpenSpotsForFreshJobs so the behavior is explicit: either
revert to the original strict blocker by returning failingJobs.length <=
maxToleratedFailures (use && with any other checks) or introduce a named
configuration/flag (e.g., allowFreshWhenFailuresHigh) and use it to gate the OR
condition; also add a short comment referencing scheduleBuildsFromTheQueue to
document that this decision controls whether new jobs are scheduled when
failures exceed maxToleratedFailures.
| static getActiveJobs = async (): Promise<CiJobQueue> => { | ||
| const snapshot = await db | ||
| .collection(CiJobs.collection) | ||
| .where('status', 'in', ['scheduled', 'inProgress']) | ||
| .limit(settings.maxConcurrentJobs) | ||
| .get(); |
There was a problem hiding this comment.
Don’t cap active-job discovery to scheduler concurrency.
Line 133 limits recovery visibility to settings.maxConcurrentJobs. If active jobs exceed that, stale jobs outside this window may never be requeued, which can block self-healing backlog drain.
Suggested fix
static getActiveJobs = async (): Promise<CiJobQueue> => {
const snapshot = await db
.collection(CiJobs.collection)
.where('status', 'in', ['scheduled', 'inProgress'])
- .limit(settings.maxConcurrentJobs)
.get();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@functions/src/model/ciJobs.ts` around lines 129 - 134, The getActiveJobs
method currently caps results with .limit(settings.maxConcurrentJobs), which can
hide active jobs beyond the scheduler concurrency and prevent recovery; remove
the .limit(...) call (or replace it with an unbounded/paginated fetch) in static
getActiveJobs so the query on CiJobs.collection for status in
['scheduled','inProgress'] returns all active jobs (or use cursor-based
pagination if necessary) to ensure all stale jobs can be discovered and
requeued.
1 participant
Summary
Testing
Summary by CodeRabbit
Bug Fixes
Performance Improvements