Merge pull request #3296 from github/mbg/dependency-caching/skip-uploads-for-exact-matches

Skip uploading dependency caches if we know they exist
2025-12-06 07:48:17 +08:00 · 2025-11-18 15:44:06 +00:00
parent 70434f6dd2 1ed85b4501
commit 249458aab2
8 changed files with 329 additions and 56 deletions
--- a/src/config-utils.test.ts
+++ b/src/config-utils.test.ts
@@ -200,12 +200,9 @@ test("load code quality config", async (t) => {
    );

    // And the config we expect it to result in
-    const expectedConfig: configUtils.Config = {
-      version: actionsUtil.getActionVersion(),
+    const expectedConfig = createTestConfig({
      analysisKinds: [AnalysisKind.CodeQuality],
      languages: [KnownLanguage.actions],
-      buildMode: undefined,
-      originalUserInput: {},
      // This gets set because we only have `AnalysisKind.CodeQuality`
      computedConfig: {
        "disable-default-queries": true,
@@ -219,14 +216,7 @@ test("load code quality config", async (t) => {
      debugMode: false,
      debugArtifactName: "",
      debugDatabaseName: "",
-      trapCaches: {},
-      trapCacheDownloadTime: 0,
-      dependencyCachingEnabled: CachingKind.None,
-      extraQueryExclusions: [],
-      overlayDatabaseMode: OverlayDatabaseMode.None,
-      useOverlayDatabaseCaching: false,
-      repositoryProperties: {},
-    };
+    });

    t.deepEqual(config, expectedConfig);
  });
@@ -507,9 +497,7 @@ test("load non-empty input", async (t) => {
    };

    // And the config we expect it to parse to
-    const expectedConfig: configUtils.Config = {
-      version: actionsUtil.getActionVersion(),
-      analysisKinds: [AnalysisKind.CodeScanning],
+    const expectedConfig = createTestConfig({
      languages: [KnownLanguage.javascript],
      buildMode: BuildMode.None,
      originalUserInput: userConfig,
@@ -521,14 +509,7 @@ test("load non-empty input", async (t) => {
      debugMode: false,
      debugArtifactName: "my-artifact",
      debugDatabaseName: "my-db",
-      trapCaches: {},
-      trapCacheDownloadTime: 0,
-      dependencyCachingEnabled: CachingKind.None,
-      extraQueryExclusions: [],
-      overlayDatabaseMode: OverlayDatabaseMode.None,
-      useOverlayDatabaseCaching: false,
-      repositoryProperties: {},
-    };
+    });

    const languagesInput = "javascript";
    const configFilePath = createConfigFile(inputFileContents, tempDir);
--- a/src/config-utils.ts
+++ b/src/config-utils.ts
@@ -148,6 +148,9 @@ export interface Config {
  /** A value indicating how dependency caching should be used. */
  dependencyCachingEnabled: CachingKind;

+  /** The keys of caches that we restored, if any. */
+  dependencyCachingRestoredKeys: string[];
+
  /**
   * Extra query exclusions to append to the config.
   */
@@ -496,6 +499,7 @@ export async function initActionState(
    trapCaches,
    trapCacheDownloadTime,
    dependencyCachingEnabled: getCachingKind(dependencyCachingEnabled),
+    dependencyCachingRestoredKeys: [],
    extraQueryExclusions: [],
    overlayDatabaseMode: OverlayDatabaseMode.None,
    useOverlayDatabaseCaching: false,
--- a/src/dependency-caching.test.ts
+++ b/src/dependency-caching.test.ts
@@ -7,6 +7,7 @@ import test from "ava";
 import * as sinon from "sinon";

 import { cacheKeyHashLength } from "./caching-utils";
+import * as cachingUtils from "./caching-utils";
 import { createStubCodeQL } from "./codeql";
 import {
  CacheConfig,
@@ -20,6 +21,8 @@ import {
  downloadDependencyCaches,
  CacheHitKind,
  cacheKey,
+  uploadDependencyCaches,
+  CacheStoreResult,
 } from "./dependency-caching";
 import { Feature } from "./feature-flags";
 import { KnownLanguage } from "./languages";
@@ -29,6 +32,7 @@ import {
  getRecordingLogger,
  checkExpectedLogMessages,
  LoggedMessage,
+  createTestConfig,
 } from "./testing-utils";
 import { withTmpDir } from "./util";

@@ -237,15 +241,17 @@ test("downloadDependencyCaches - does not restore caches with feature keys if no
    .resolves(CSHARP_BASE_PATTERNS);
  makePatternCheckStub.withArgs(CSHARP_EXTRA_PATTERNS).resolves(undefined);

-  const results = await downloadDependencyCaches(
+  const result = await downloadDependencyCaches(
    codeql,
    createFeatures([]),
    [KnownLanguage.csharp],
    logger,
  );
-  t.is(results.length, 1);
-  t.is(results[0].language, KnownLanguage.csharp);
-  t.is(results[0].hit_kind, CacheHitKind.Miss);
+  const statusReport = result.statusReport;
+  t.is(statusReport.length, 1);
+  t.is(statusReport[0].language, KnownLanguage.csharp);
+  t.is(statusReport[0].hit_kind, CacheHitKind.Miss);
+  t.deepEqual(result.restoredKeys, []);
  t.assert(restoreCacheStub.calledOnce);
 });

@@ -257,7 +263,8 @@ test("downloadDependencyCaches - restores caches with feature keys if features a
  const logger = getRecordingLogger(messages);
  const features = createFeatures([Feature.CsharpNewCacheKey]);

-  sinon.stub(glob, "hashFiles").resolves("abcdef");
+  const mockHash = "abcdef";
+  sinon.stub(glob, "hashFiles").resolves(mockHash);

  const keyWithFeature = await cacheKey(
    codeql,
@@ -277,15 +284,28 @@ test("downloadDependencyCaches - restores caches with feature keys if features a
    .resolves(CSHARP_BASE_PATTERNS);
  makePatternCheckStub.withArgs(CSHARP_EXTRA_PATTERNS).resolves(undefined);

-  const results = await downloadDependencyCaches(
+  const result = await downloadDependencyCaches(
    codeql,
    features,
    [KnownLanguage.csharp],
    logger,
  );
-  t.is(results.length, 1);
-  t.is(results[0].language, KnownLanguage.csharp);
-  t.is(results[0].hit_kind, CacheHitKind.Exact);
+
+  // Check that the status report for telemetry indicates that one cache was restored with an exact match.
+  const statusReport = result.statusReport;
+  t.is(statusReport.length, 1);
+  t.is(statusReport[0].language, KnownLanguage.csharp);
+  t.is(statusReport[0].hit_kind, CacheHitKind.Exact);
+
+  // Check that the restored key has been returned.
+  const restoredKeys = result.restoredKeys;
+  t.is(restoredKeys.length, 1);
+  t.assert(
+    restoredKeys[0].endsWith(mockHash),
+    "Expected restored key to end with hash returned by `hashFiles`",
+  );
+
+  // `restoreCache` should have been called exactly once.
  t.assert(restoreCacheStub.calledOnce);
 });

@@ -297,8 +317,14 @@ test("downloadDependencyCaches - restores caches with feature keys if features a
  const logger = getRecordingLogger(messages);
  const features = createFeatures([Feature.CsharpNewCacheKey]);

+  // We expect two calls to `hashFiles`: the first by the call to `cacheKey` below,
+  // and the second by `downloadDependencyCaches`. We use the result of the first
+  // call as part of the cache key that identifies a mock, existing cache. The result
+  // of the second call is for the primary restore key, which we don't want to match
+  // the first key so that we can test the restore keys logic.
+  const restoredHash = "abcdef";
  const hashFilesStub = sinon.stub(glob, "hashFiles");
-  hashFilesStub.onFirstCall().resolves("abcdef");
+  hashFilesStub.onFirstCall().resolves(restoredHash);
  hashFilesStub.onSecondCall().resolves("123456");

  const keyWithFeature = await cacheKey(
@@ -319,18 +345,230 @@ test("downloadDependencyCaches - restores caches with feature keys if features a
    .resolves(CSHARP_BASE_PATTERNS);
  makePatternCheckStub.withArgs(CSHARP_EXTRA_PATTERNS).resolves(undefined);

-  const results = await downloadDependencyCaches(
+  const result = await downloadDependencyCaches(
    codeql,
    features,
    [KnownLanguage.csharp],
    logger,
  );
-  t.is(results.length, 1);
-  t.is(results[0].language, KnownLanguage.csharp);
-  t.is(results[0].hit_kind, CacheHitKind.Partial);
+
+  // Check that the status report for telemetry indicates that one cache was restored with a partial match.
+  const statusReport = result.statusReport;
+  t.is(statusReport.length, 1);
+  t.is(statusReport[0].language, KnownLanguage.csharp);
+  t.is(statusReport[0].hit_kind, CacheHitKind.Partial);
+
+  // Check that the restored key has been returned.
+  const restoredKeys = result.restoredKeys;
+  t.is(restoredKeys.length, 1);
+  t.assert(
+    restoredKeys[0].endsWith(restoredHash),
+    "Expected restored key to end with hash returned by `hashFiles`",
+  );
+
  t.assert(restoreCacheStub.calledOnce);
 });

+test("uploadDependencyCaches - skips upload for a language with no cache config", async (t) => {
+  const codeql = createStubCodeQL({});
+  const messages: LoggedMessage[] = [];
+  const logger = getRecordingLogger(messages);
+  const features = createFeatures([]);
+  const config = createTestConfig({
+    languages: [KnownLanguage.actions],
+  });
+
+  const result = await uploadDependencyCaches(codeql, features, config, logger);
+  t.is(result.length, 0);
+  checkExpectedLogMessages(t, messages, [
+    "Skipping upload of dependency cache for actions",
+  ]);
+});
+
+test("uploadDependencyCaches - skips upload if no files for the hash exist", async (t) => {
+  const codeql = createStubCodeQL({});
+  const messages: LoggedMessage[] = [];
+  const logger = getRecordingLogger(messages);
+  const features = createFeatures([]);
+  const config = createTestConfig({
+    languages: [KnownLanguage.go],
+  });
+
+  const makePatternCheckStub = sinon.stub(internal, "makePatternCheck");
+  makePatternCheckStub.resolves(undefined);
+
+  const result = await uploadDependencyCaches(codeql, features, config, logger);
+  t.is(result.length, 1);
+  t.is(result[0].language, KnownLanguage.go);
+  t.is(result[0].result, CacheStoreResult.NoHash);
+});
+
+test("uploadDependencyCaches - skips upload if we know the cache already exists", async (t) => {
+  process.env["RUNNER_OS"] = "Linux";
+
+  const codeql = createStubCodeQL({});
+  const messages: LoggedMessage[] = [];
+  const logger = getRecordingLogger(messages);
+  const features = createFeatures([]);
+
+  const mockHash = "abcdef";
+  sinon.stub(glob, "hashFiles").resolves(mockHash);
+
+  const makePatternCheckStub = sinon.stub(internal, "makePatternCheck");
+  makePatternCheckStub
+    .withArgs(CSHARP_BASE_PATTERNS)
+    .resolves(CSHARP_BASE_PATTERNS);
+
+  const primaryCacheKey = await cacheKey(
+    codeql,
+    features,
+    KnownLanguage.csharp,
+    CSHARP_BASE_PATTERNS,
+  );
+
+  const config = createTestConfig({
+    languages: [KnownLanguage.csharp],
+    dependencyCachingRestoredKeys: [primaryCacheKey],
+  });
+
+  const result = await uploadDependencyCaches(codeql, features, config, logger);
+  t.is(result.length, 1);
+  t.is(result[0].language, KnownLanguage.csharp);
+  t.is(result[0].result, CacheStoreResult.Duplicate);
+});
+
+test("uploadDependencyCaches - skips upload if cache size is 0", async (t) => {
+  process.env["RUNNER_OS"] = "Linux";
+
+  const codeql = createStubCodeQL({});
+  const messages: LoggedMessage[] = [];
+  const logger = getRecordingLogger(messages);
+  const features = createFeatures([]);
+
+  const mockHash = "abcdef";
+  sinon.stub(glob, "hashFiles").resolves(mockHash);
+
+  const makePatternCheckStub = sinon.stub(internal, "makePatternCheck");
+  makePatternCheckStub
+    .withArgs(CSHARP_BASE_PATTERNS)
+    .resolves(CSHARP_BASE_PATTERNS);
+
+  sinon.stub(cachingUtils, "getTotalCacheSize").resolves(0);
+
+  const config = createTestConfig({
+    languages: [KnownLanguage.csharp],
+  });
+
+  const result = await uploadDependencyCaches(codeql, features, config, logger);
+  t.is(result.length, 1);
+  t.is(result[0].language, KnownLanguage.csharp);
+  t.is(result[0].result, CacheStoreResult.Empty);
+
+  checkExpectedLogMessages(t, messages, [
+    "Skipping upload of dependency cache",
+  ]);
+});
+
+test("uploadDependencyCaches - uploads caches when all requirements are met", async (t) => {
+  process.env["RUNNER_OS"] = "Linux";
+
+  const codeql = createStubCodeQL({});
+  const messages: LoggedMessage[] = [];
+  const logger = getRecordingLogger(messages);
+  const features = createFeatures([]);
+
+  const mockHash = "abcdef";
+  sinon.stub(glob, "hashFiles").resolves(mockHash);
+
+  const makePatternCheckStub = sinon.stub(internal, "makePatternCheck");
+  makePatternCheckStub
+    .withArgs(CSHARP_BASE_PATTERNS)
+    .resolves(CSHARP_BASE_PATTERNS);
+
+  sinon.stub(cachingUtils, "getTotalCacheSize").resolves(1024);
+  sinon.stub(actionsCache, "saveCache").resolves();
+
+  const config = createTestConfig({
+    languages: [KnownLanguage.csharp],
+  });
+
+  const result = await uploadDependencyCaches(codeql, features, config, logger);
+  t.is(result.length, 1);
+  t.is(result[0].language, KnownLanguage.csharp);
+  t.is(result[0].result, CacheStoreResult.Stored);
+  t.is(result[0].upload_size_bytes, 1024);
+
+  checkExpectedLogMessages(t, messages, ["Uploading cache of size"]);
+});
+
+test("uploadDependencyCaches - catches `ReserveCacheError` exceptions", async (t) => {
+  process.env["RUNNER_OS"] = "Linux";
+
+  const codeql = createStubCodeQL({});
+  const messages: LoggedMessage[] = [];
+  const logger = getRecordingLogger(messages);
+  const features = createFeatures([]);
+
+  const mockHash = "abcdef";
+  sinon.stub(glob, "hashFiles").resolves(mockHash);
+
+  const makePatternCheckStub = sinon.stub(internal, "makePatternCheck");
+  makePatternCheckStub
+    .withArgs(CSHARP_BASE_PATTERNS)
+    .resolves(CSHARP_BASE_PATTERNS);
+
+  sinon.stub(cachingUtils, "getTotalCacheSize").resolves(1024);
+  sinon
+    .stub(actionsCache, "saveCache")
+    .throws(new actionsCache.ReserveCacheError("Already in use"));
+
+  const config = createTestConfig({
+    languages: [KnownLanguage.csharp],
+  });
+
+  await t.notThrowsAsync(async () => {
+    const result = await uploadDependencyCaches(
+      codeql,
+      features,
+      config,
+      logger,
+    );
+    t.is(result.length, 1);
+    t.is(result[0].language, KnownLanguage.csharp);
+    t.is(result[0].result, CacheStoreResult.Duplicate);
+
+    checkExpectedLogMessages(t, messages, ["Not uploading cache for"]);
+  });
+});
+
+test("uploadDependencyCaches - throws other exceptions", async (t) => {
+  process.env["RUNNER_OS"] = "Linux";
+
+  const codeql = createStubCodeQL({});
+  const messages: LoggedMessage[] = [];
+  const logger = getRecordingLogger(messages);
+  const features = createFeatures([]);
+
+  const mockHash = "abcdef";
+  sinon.stub(glob, "hashFiles").resolves(mockHash);
+
+  const makePatternCheckStub = sinon.stub(internal, "makePatternCheck");
+  makePatternCheckStub
+    .withArgs(CSHARP_BASE_PATTERNS)
+    .resolves(CSHARP_BASE_PATTERNS);
+
+  sinon.stub(cachingUtils, "getTotalCacheSize").resolves(1024);
+  sinon.stub(actionsCache, "saveCache").throws();
+
+  const config = createTestConfig({
+    languages: [KnownLanguage.csharp],
+  });
+
+  await t.throwsAsync(async () => {
+    await uploadDependencyCaches(codeql, features, config, logger);
+  });
+});
+
 test("getFeaturePrefix - returns empty string if no features are enabled", async (t) => {
  const codeql = createStubCodeQL({});
  const features = createFeatures([]);
--- a/src/dependency-caching.ts
+++ b/src/dependency-caching.ts
@@ -193,6 +193,14 @@ export interface DependencyCacheRestoreStatus {
 /** An array of `DependencyCacheRestoreStatus` objects for each analysed language with a caching configuration. */
 export type DependencyCacheRestoreStatusReport = DependencyCacheRestoreStatus[];

+/** Represents the results of `downloadDependencyCaches`. */
+export interface DownloadDependencyCachesResult {
+  /** The status report for telemetry */
+  statusReport: DependencyCacheRestoreStatusReport;
+  /** An array of cache keys that we have restored and therefore know to exist. */
+  restoredKeys: string[];
+}
+
 /**
 * A wrapper around `cacheConfig.getHashPatterns` which logs when there are no files to calculate
 * a hash for the cache key from.
@@ -239,8 +247,9 @@ export async function downloadDependencyCaches(
  features: FeatureEnablement,
  languages: Language[],
  logger: Logger,
-): Promise<DependencyCacheRestoreStatusReport> {
+): Promise<DownloadDependencyCachesResult> {
  const status: DependencyCacheRestoreStatusReport = [];
+  const restoredKeys: string[] = [];

  for (const language of languages) {
    const cacheConfig = defaultCacheConfigs[language];
@@ -288,16 +297,27 @@ export async function downloadDependencyCaches(

    if (hitKey !== undefined) {
      logger.info(`Cache hit on key ${hitKey} for ${language}.`);
-      const hit_kind =
-        hitKey === primaryKey ? CacheHitKind.Exact : CacheHitKind.Partial;
-      status.push({ language, hit_kind, download_duration_ms });
+
+      // We have a partial cache hit, unless the key of the restored cache matches the
+      // primary restore key.
+      let hit_kind = CacheHitKind.Partial;
+      if (hitKey === primaryKey) {
+        hit_kind = CacheHitKind.Exact;
+      }
+
+      status.push({
+        language,
+        hit_kind,
+        download_duration_ms,
+      });
+      restoredKeys.push(hitKey);
    } else {
      status.push({ language, hit_kind: CacheHitKind.Miss });
      logger.info(`No suitable cache found for ${language}.`);
    }
  }

-  return status;
+  return { statusReport: status, restoredKeys };
 }

 /** Enumerates possible outcomes for storing caches. */
@@ -365,6 +385,18 @@ export async function uploadDependencyCaches(
      continue;
    }

+    // Now that we have verified that there are suitable files, compute the hash for the cache key.
+    const key = await cacheKey(codeql, features, language, patterns);
+
+    // Check that we haven't previously restored this exact key. If a cache with this key
+    // already exists in the Actions Cache, performing the next steps is pointless as the cache
+    // will not get overwritten. We can therefore skip the expensive work of measuring the size
+    // of the cache contents and attempting to upload it if we know that the cache already exists.
+    if (config.dependencyCachingRestoredKeys.includes(key)) {
+      status.push({ language, result: CacheStoreResult.Duplicate });
+      continue;
+    }
+
    // Calculate the size of the files that we would store in the cache. We use this to determine whether the
    // cache should be saved or not. For example, if there are no files to store, then we skip creating the
    // cache. In the future, we could also:
@@ -390,8 +422,6 @@ export async function uploadDependencyCaches(
      continue;
    }

-    const key = await cacheKey(codeql, features, language, patterns);
-
    logger.info(
      `Uploading cache of size ${size} for ${language} with key ${key}...`,
    );
--- a/src/init-action.ts
+++ b/src/init-action.ts
@@ -371,7 +371,7 @@ async function run() {
  }

  let overlayBaseDatabaseStats: OverlayBaseDatabaseDownloadStats | undefined;
-  let dependencyCachingResults: DependencyCacheRestoreStatusReport | undefined;
+  let dependencyCachingStatus: DependencyCacheRestoreStatusReport | undefined;
  try {
    if (
      config.overlayDatabaseMode === OverlayDatabaseMode.Overlay &&
@@ -579,12 +579,15 @@ async function run() {

    // Restore dependency cache(s), if they exist.
    if (shouldRestoreCache(config.dependencyCachingEnabled)) {
-      dependencyCachingResults = await downloadDependencyCaches(
+      const dependencyCachingResult = await downloadDependencyCaches(
        codeql,
        features,
        config.languages,
        logger,
      );
+      dependencyCachingStatus = dependencyCachingResult.statusReport;
+      config.dependencyCachingRestoredKeys =
+        dependencyCachingResult.restoredKeys;
    }

    // Suppress warnings about disabled Python library extraction.
@@ -732,7 +735,7 @@ async function run() {
      toolsSource,
      toolsVersion,
      overlayBaseDatabaseStats,
-      dependencyCachingResults,
+      dependencyCachingStatus,
      logger,
      error,
    );
@@ -755,7 +758,7 @@ async function run() {
    toolsSource,
    toolsVersion,
    overlayBaseDatabaseStats,
-    dependencyCachingResults,
+    dependencyCachingStatus,
    logger,
  );
 }
--- a/src/testing-utils.ts
+++ b/src/testing-utils.ts
@@ -392,6 +392,7 @@ export function createTestConfig(overrides: Partial<Config>): Config {
      trapCaches: {},
      trapCacheDownloadTime: 0,
      dependencyCachingEnabled: CachingKind.None,
+      dependencyCachingRestoredKeys: [],
      extraQueryExclusions: [],
      overlayDatabaseMode: OverlayDatabaseMode.None,
      useOverlayDatabaseCaching: false,