Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 2 additions & 0 deletions lib/analyze-action.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions lib/init-action.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions src/api-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ export interface ActionsCacheItem {
created_at?: string;
id?: number;
key?: string;
last_accessed_at?: string;
size_in_bytes?: number;
}

Expand Down
34 changes: 34 additions & 0 deletions src/overlay/caching.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -417,3 +417,37 @@ test.serial(
t.deepEqual(result, ["2.25.0", "2.24.0"]);
},
);

test.serial(
"getCodeQlVersionsForOverlayBaseDatabases ignores cache entries close to eviction",
async (t) => {
const logger = getRunnerLogger(true);

const now = Date.now();
const isoDaysAgo = (days: number) =>
new Date(now - days * 24 * 60 * 60 * 1000).toISOString();

sinon.stub(apiClient, "getAutomationID").resolves("test-automation-id/");
sinon.stub(apiClient, "listActionsCaches").resolves([
{
key: "codeql-overlay-base-database-1-c5666c509a2d9895-python-2.25.0-abc123-1-1",
last_accessed_at: isoDaysAgo(1),
},
{
// Older than the 6-day threshold; close to the 7-day eviction window.
key: "codeql-overlay-base-database-1-c5666c509a2d9895-python-2.26.0-def456-2-1",
last_accessed_at: isoDaysAgo(6.5),
},
{
key: "codeql-overlay-base-database-1-c5666c509a2d9895-python-2.24.0-ghi789-3-1",
last_accessed_at: isoDaysAgo(3),
},
]);

const result = await getCodeQlVersionsForOverlayBaseDatabases(
["python"],
logger,
);
t.deepEqual(result, ["2.25.0", "2.24.0"]);
},
);
64 changes: 52 additions & 12 deletions src/overlay/caching.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ import {
getWorkflowRunAttempt,
getWorkflowRunID,
} from "../actions-util";
import { getAutomationID, listActionsCaches } from "../api-client";
import {
type ActionsCacheItem,
getAutomationID,
listActionsCaches,
} from "../api-client";
import { createCacheKeyHash } from "../caching-utils";
import { type CodeQL } from "../codeql";
import { type Config } from "../config-utils";
Expand Down Expand Up @@ -48,6 +52,12 @@ const OVERLAY_BASE_DATABASE_MAX_UPLOAD_SIZE_BYTES =
const CACHE_VERSION = 1;
const CACHE_PREFIX = "codeql-overlay-base-database";

// The Actions cache evicts entries that have not been accessed in the past 7
// days. We conservatively set a limit of 6 days to avoid using a cached base DB
// that may be evicted before we can download it.
const CACHE_ENTRY_MAX_AGE_DAYS = 6;
const CACHE_ENTRY_MAX_AGE_MS = CACHE_ENTRY_MAX_AGE_DAYS * 24 * 60 * 60 * 1000;

// The purpose of this ten-minute limit is to guard against the possibility
// that the cache service is unresponsive, which would otherwise cause the
// entire action to hang. Normally we expect cache operations to complete
Expand Down Expand Up @@ -435,6 +445,39 @@ async function getCacheKeyPrefixBase(
return `${CACHE_PREFIX}-${CACHE_VERSION}-${componentsHash}-${languagesComponent}-`;
}

/**
* Lists overlay-base database cache entries with the given key prefix, ignoring entries that are
* old enough that they may be evicted by the Actions cache before we attempt to download them.
*/
async function listRecentOverlayBaseDatabaseCaches(
cacheKeyPrefix: string,
logger: Logger,
): Promise<ActionsCacheItem[]> {
Comment on lines +448 to +455
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry to be very critical here, but I don't think this is architecturally a good mitigation for the problem, and adds extra complexity for (IMHO) little gain.

Cache eviction is based on several criteria, and age is only one of them. This mitigation guards against the case where:

  • We are analysing a low-traffic repo
  • Around 7 days after the cache was last accessed

That's a valid problem and will happen once in a while, but isn't very common.

I am probably more concerned about high-traffic repos where workflows are uploading caches all the time, and may cause a cache we have identified to be evicted at essentially any time for space reasons. Checking the age of the cache we want doesn't guard against that. (Unless we also check all other caches to see how likely it is that ours would be evicted first, but I don't think that's worth the effort.)

I think realistically, a better solution might just be to stick this logic in a retry loop: find a suitable cache, try to download it, and if it fails attempt the process another time or two.

const allCaches = await listActionsCaches(cacheKeyPrefix);

if (allCaches.length === 0) {
logger.info("No overlay-base databases found in Actions cache.");
return [];
}

const cutoffMs = Date.now() - CACHE_ENTRY_MAX_AGE_MS;
const recentCaches = allCaches.filter((cache) => {
if (!cache.last_accessed_at) return true;
const lastAccessedMs = Date.parse(cache.last_accessed_at);
return Number.isNaN(lastAccessedMs) || lastAccessedMs >= cutoffMs;
});
const numTooOldDatabases = allCaches.length - recentCaches.length;
const tooOldSuffix =
numTooOldDatabases > 0
? ` (ignoring ${numTooOldDatabases} that may be evicted soon)`
: "";
logger.info(
`Found ${allCaches.length} overlay-base ${allCaches.length === 1 ? "database" : "databases"} in the Actions cache${tooOldSuffix}.`,
);

return recentCaches;
}

/**
* Searches the GitHub Actions cache for overlay-base databases matching the given languages, and
* returns all stable CodeQL versions found across matching cache entries.
Expand All @@ -448,7 +491,7 @@ export async function getCodeQlVersionsForOverlayBaseDatabases(
): Promise<string[] | undefined> {
const languages = rawLanguages.map(parseBuiltInLanguage);
if (languages.includes(undefined)) {
logger.warning(
logger.info(
"One or more provided languages are not recognized as built-in languages. " +
"Skipping searching for overlay-base databases in cache.",
);
Expand All @@ -463,22 +506,19 @@ export async function getCodeQlVersionsForOverlayBaseDatabases(
`prefix ${cacheKeyPrefix}`,
);

const caches = await listActionsCaches(cacheKeyPrefix);
const caches = await listRecentOverlayBaseDatabaseCaches(
cacheKeyPrefix,
logger,
);

if (caches.length === 0) {
logger.info("No overlay-base databases found in Actions cache.");
return [];
}

logger.info(
`Found ${caches.length} overlay-base ` +
`${caches.length === 1 ? "database" : "databases"} in the Actions cache.`,
);

// Parse CodeQL versions from cache keys, matching only stable releases.
//
// After the prefix, the remaining key format starts with `${codeQlVersion}-`. Nightlies will have
// a suffix like `+202604201548` that will break the match.
// After the prefix, the remaining key format starts with `${codeQlVersion}-`. Nightlies have a
// suffix like `+202604201548` that will prevent a match.
//
// Caveat: this relies on the fact that we haven't released any CodeQL bundles with the
// `x.y.z-<pre-release>` semver format which does not interact well with the current overlay base
Expand Down Expand Up @@ -506,7 +546,7 @@ export async function getCodeQlVersionsForOverlayBaseDatabases(
const versions = [...versionSet].sort(semver.rcompare);

logger.info(
`Found overlay databases for the following CodeQL versions in the Actions cache: ${versions.join(", ")}`,
`Found overlay-base databases for the following CodeQL versions in the Actions cache: ${versions.join(", ")}`,
);
Comment thread
henrymercer marked this conversation as resolved.

return versions;
Expand Down
Loading