Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions functions/src/cron/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ if (MINUTES < 10) {
throw new Error('Is the result really worth the machine time? Remove me.');
}

// Timeout of 60 seconds will keep our routine process tight.
// Allow enough time for DockerHub-backed healing work during queue recovery.
export const trigger = onSchedule(
{
schedule: `every ${MINUTES} minutes`,
memory: '512MiB',
timeoutSeconds: 60,
timeoutSeconds: 300,
secrets: [discordToken, githubPrivateKeyConfigSecret, githubClientSecretConfigSecret],
},
async () => {
Expand Down
4 changes: 3 additions & 1 deletion functions/src/logic/buildQueue/cleanUpBuilds.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { Cleaner } from './cleaner';
import { RepoVersionInfo } from '../../model/repoVersionInfo';

export const cleanUpBuilds = async () => {
await Cleaner.cleanUp();
const latestRepoVersion = await RepoVersionInfo.getLatest();
await Cleaner.cleanUp(latestRepoVersion.version);
};
90 changes: 86 additions & 4 deletions functions/src/logic/buildQueue/cleaner.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,23 @@
import { CiBuilds } from '../../model/ciBuilds';
import { CiJobs } from '../../model/ciJobs';
import { Discord } from '../../service/discord';
import { Dockerhub } from '../../service/dockerhub';

export class Cleaner {
// Cronjob intentionally has a limited runtime
static readonly maxBuildsProcessedPerRun: number = 5;
// Cronjob intentionally has a limited runtime, but it still needs enough
// throughput to drain queue backlogs without requiring manual intervention.
static readonly maxBuildsProcessedPerRun: number = 25;
static readonly activeJobWithoutBuildsAfterMinutes: number = 30;
static readonly startedBuildPublishProbeAfterMinutes: number = 45;
static readonly startedBuildFailureAfterHours: number = 6;

static buildsProcessed: number;

public static async cleanUp() {
public static async cleanUp(latestRepoVersion: string) {
this.buildsProcessed = 0;
await this.requeueActiveJobsWithoutBuilds();
await this.recoverMaxedOutFailedBuilds(latestRepoVersion);
await this.reconcileStartedBuildsThatMayHavePublished();
await this.cleanUpBuildsThatDidntReportBack();
await this.healFailedBuildsAlreadyOnDockerHub();
}
Expand All @@ -22,6 +30,36 @@ export class Cleaner {
*/
static readonly maxRecoveryAttempts: number = 2;

/**
* Jobs can get stuck in "scheduled" or "inProgress" without ever creating a
* build record when the dispatch path flakes before reportNewBuild. Reset
* those jobs back to created so the scheduler can dispatch them again.
*/
private static async requeueActiveJobsWithoutBuilds() {
const activeJobs = await CiJobs.getActiveJobs();
const staleThresholdMs = this.activeJobWithoutBuildsAfterMinutes * 60 * 1000;

for (const activeJob of activeJobs) {
if (this.buildsProcessed >= this.maxBuildsProcessedPerRun) return;

const { id: jobId, data: job } = activeJob;
const lastTouchedSeconds = job.modifiedDate?.seconds || job.addedDate?.seconds;
if (!lastTouchedSeconds) continue;

const ageMs = Date.now() - lastTouchedSeconds * 1000;
if (ageMs < staleThresholdMs) continue;

const hasBuilds = await CiBuilds.hasAnyBuildsForJob(jobId);
if (hasBuilds) continue;

this.buildsProcessed += 1;
await CiJobs.resetJobToCreated(jobId);
await Discord.sendAlert(
Comment on lines +52 to +57

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

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.

`[Cleaner] Reset stale ${job.status} job "${jobId}" back to created because it had no build records after ${this.activeJobWithoutBuildsAfterMinutes} minutes.`,
);
}
}

/**
* Automatically recover maxed-out failed builds for the latest repo version.
* If the image is already on DockerHub, mark it as published.
Expand Down Expand Up @@ -183,6 +221,49 @@ export class Cleaner {
}
}

/**
* If a started build has been running for a while and the image already
* exists on DockerHub, we can mark it as published without waiting for the
* full GitHub Actions timeout window. This frees queue capacity earlier.
*/
private static async reconcileStartedBuildsThatMayHavePublished() {
const startedBuilds = await CiBuilds.getStartedBuilds();
const probeThresholdMs = this.startedBuildPublishProbeAfterMinutes * 60 * 1000;

for (const startedBuild of startedBuilds) {
if (this.buildsProcessed >= this.maxBuildsProcessedPerRun) return;

const { buildId, meta, relatedJobId: jobId, imageType, buildInfo } = startedBuild;
const { lastBuildStart, publishedDate } = meta;
const { baseOs, repoVersion } = buildInfo;

if (!lastBuildStart) continue;

const buildStartMs = new Date(lastBuildStart.seconds * 1000).getTime();
const nowMs = Date.now();
if (nowMs - buildStartMs < probeThresholdMs) continue;

const tag = buildId.replace(new RegExp(`^${imageType}-`), '');
this.buildsProcessed += 1;

const response = await Dockerhub.fetchImageData(imageType, tag);
if (!response) continue;

const digest = response.digest || '';
const message = publishedDate
? `[Cleaner] Build "${tag}" has published metadata and exists on DockerHub. Reconciling status back to published.`
: `[Cleaner] Build "${tag}" is still "started" but already exists on DockerHub. Marking as published early.`;
await Discord.sendDebug(message);
await CiBuilds.markBuildAsPublished(buildId, jobId, {
digest,
specificTag: `${baseOs}-${repoVersion}`,
friendlyTag: repoVersion.replace(/\.\d+$/, ''),
imageName: Dockerhub.getImageName(imageType),
imageRepo: Dockerhub.getRepositoryBaseName(),
});
}
}

private static async cleanUpBuildsThatDidntReportBack() {
const startedBuilds = await CiBuilds.getStartedBuilds();

Expand Down Expand Up @@ -218,7 +299,8 @@ export class Cleaner {
// If a job reaches this limit, the job is terminated and fails to complete.
// @see https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration
const ONE_HOUR = 1000 * 60 * 60;
const sixHoursAgo = new Date().getTime() - 6 * ONE_HOUR;
const failureThresholdMs = this.startedBuildFailureAfterHours * ONE_HOUR;
const sixHoursAgo = new Date().getTime() - failureThresholdMs;
const buildStart = new Date(lastBuildStart.seconds * 1000).getTime();

if (buildStart < sixHoursAgo) {
Expand Down
6 changes: 5 additions & 1 deletion functions/src/logic/buildQueue/ingeminator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,14 @@ import { logger } from 'firebase-functions/v2';
export class Ingeminator {
numberToSchedule: number;
gitHubClient: Octokit;
private scheduledBuilds = 0;

constructor(numberToSchedule: number, gitHubClient: Octokit) {
this.numberToSchedule = numberToSchedule;
this.gitHubClient = gitHubClient;
}

async rescheduleFailedJobs(jobs: CiJobQueue) {
async rescheduleFailedJobs(jobs: CiJobQueue): Promise<number> {
if (jobs.length <= 0) {
throw new Error(
'[Ingeminator] Expected ingeminator to be called with jobs to retry, none were given.',
Expand All @@ -36,6 +37,8 @@ export class Ingeminator {

await this.rescheduleFailedBuildsForJob(job);
}

return this.scheduledBuilds;
}

private async rescheduleFailedBuildsForJob(job: CiJobQueueItem) {
Expand Down Expand Up @@ -101,6 +104,7 @@ export class Ingeminator {
if (!(await this.rescheduleBuild(jobId, jobData, buildId, BuildData))) {
return;
}
this.scheduledBuilds += 1;
}

await CiJobs.markJobAsScheduled(jobId);
Expand Down
23 changes: 20 additions & 3 deletions functions/src/logic/buildQueue/scheduler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export class Scheduler {
private _gitHub: Octokit | undefined;
private maxConcurrentJobs: number;
private repoVersionInfo: RepoVersionInfo;
private reservedRetrySlots = 0;

private get gitHub(): Octokit {
// @ts-ignore
Expand Down Expand Up @@ -166,6 +167,7 @@ export class Scheduler {
async ensureThereAreNoFailedJobs(): Promise<boolean> {
const { maxToleratedFailures, maxExtraJobsForRescheduling } = settings;
const failingJobs = await CiJobs.getFailingJobsQueue();
this.reservedRetrySlots = 0;

if (failingJobs.length >= 1) {
const openSpots = await this.determineOpenSpots();
Expand All @@ -177,14 +179,23 @@ export class Scheduler {
}

const ingeminator = new Ingeminator(numberToReschedule, this.gitHub);
await ingeminator.rescheduleFailedJobs(failingJobs);
const scheduledRetries = await ingeminator.rescheduleFailedJobs(failingJobs);
this.reservedRetrySlots = Math.min(scheduledRetries, openSpots);

const remainingFreshSlots = openSpots - this.reservedRetrySlots;
if (remainingFreshSlots > 0) {
await Discord.sendDebug(
`[Scheduler] Reserved ${this.reservedRetrySlots} slot(s) for retries and kept ${remainingFreshSlots} slot(s) available for fresh jobs.`,
);
}
}

return failingJobs.length <= maxToleratedFailures;
const openSpotsAfterRetries = await this.determineOpenSpotsForFreshJobs();
return failingJobs.length <= maxToleratedFailures || openSpotsAfterRetries > 0;
Comment on lines +193 to +194

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 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 -A2

Repository: 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.ts

Repository: 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 -i

Repository: 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 -100

Repository: game-ci/versioning-backend

Length of output: 2064


🏁 Script executed:

# Search for where scheduleBuildsFromTheQueue is called
rg -n 'scheduleBuildsFromTheQueue' --type=ts -B2 -A2

Repository: 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 -20

Repository: 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.ts

Repository: game-ci/versioning-backend

Length of output: 353


🏁 Script executed:

# Check the determineOpenSpotsForFreshJobs method to understand what it counts
rg -n 'determineOpenSpotsForFreshJobs' --type=ts -A10

Repository: 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.

}

async buildLatestEditorImages(): Promise<boolean> {
const openSpots = await this.determineOpenSpots();
const openSpots = await this.determineOpenSpotsForFreshJobs();
if (openSpots <= 0) {
await Discord.sendDebug('[Scheduler] Not scheduling any new jobs, as the queue is full');
return false;
Expand Down Expand Up @@ -246,4 +257,10 @@ export class Scheduler {
const openSpots = this.maxConcurrentJobs - currentlyRunningJobs;
return openSpots <= 0 ? 0 : openSpots;
}

private async determineOpenSpotsForFreshJobs(): Promise<number> {
const openSpots = await this.determineOpenSpots();
const availableSpots = openSpots - this.reservedRetrySlots;
return availableSpots <= 0 ? 0 : availableSpots;
}
}
10 changes: 10 additions & 0 deletions functions/src/model/ciBuilds.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,16 @@ export class CiBuilds {
}));
};

public static hasAnyBuildsForJob = async (jobId: string): Promise<boolean> => {
const snapshot = await db
.collection(CiBuilds.collection)
.where('relatedJobId', '==', jobId)
.limit(1)
.get();

return snapshot.docs.length > 0;
};

/**
* Registers a new build or handles duplicate dispatches gracefully.
* Returns the existing status if the build is already in progress or published,
Expand Down
22 changes: 22 additions & 0 deletions functions/src/model/ciJobs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,19 @@ export class CiJobs {
return snapshot.docs.length;
};

static getActiveJobs = async (): Promise<CiJobQueue> => {
const snapshot = await db
.collection(CiJobs.collection)
.where('status', 'in', ['scheduled', 'inProgress'])
.limit(settings.maxConcurrentJobs)
.get();
Comment on lines +129 to +134

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.


return snapshot.docs.map((doc) => ({
id: doc.id,
data: doc.data() as CiJob,
}));
};

static create = async (
jobId: string,
imageType: ImageType,
Expand Down Expand Up @@ -236,6 +249,15 @@ export class CiJobs {
});
};

static resetJobToCreated = async (jobId: string) => {
const job = await db.collection(CiJobs.collection).doc(jobId);

await job.update({
status: 'created',
modifiedDate: Timestamp.now(),
});
};

static async removeDryRunJob(jobId: string) {
if (!jobId.startsWith('dryRun')) {
throw new Error('Expect only dryRun jobs to be deleted.');
Expand Down
Loading