From d47746ed940324aa8ff15fb22787fa9f72f549aa Mon Sep 17 00:00:00 2001 From: awalsh128 Date: Thu, 5 Nov 2026 16:22:34 -0800 Subject: [PATCH] fix: restore cached packages in apt install order Restore extracted archives sorted by filename, so packages whose install scripts depend on files from other packages could break. Restore now follows the entry order in manifest_all.json, which comes from the apt install output, and fails explicitly when an archive is missing. Refs #196 Co-Authored-By: Claude Opus 5.5 --- src/cache.ts | 45 ++++++++++------ test/cache-restore.test.ts | 106 +++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 17 deletions(-) create mode 100644 test/cache-restore.test.ts diff --git a/src/cache.ts b/src/cache.ts index 9344e9e..aa81e22 100644 --- a/src/cache.ts +++ b/src/cache.ts @@ -8,7 +8,6 @@ import * as path from "node:path"; import { ActionPackageNames, findInstallScript } from "./packages.js"; import { Manifest, ManifestEntry } from "./manifest.js"; import { - deserializePackageName, CommandRunner, PackageInfo, PackageName, @@ -116,10 +115,7 @@ export class Cache { this.logger = logger; } - private async runInstallScripts(archiveFilename: string): Promise { - const serializePackageName = archiveFilename.replace(/\.tar$/, ""); - const packageName = deserializePackageName(serializePackageName)!; - + private async runInstallScripts(packageName: PackageName): Promise { const runScript = async ( packageName: PackageName, scriptType: "preinst" | "postinst", @@ -186,22 +182,37 @@ export class Cache { await logFileError(`Manifest file not found at ${manifestPath}`); return undefined; } - const archives = await fs - .readdir(this.path) - .then((entries: string[]) => - entries.filter((entry) => entry.endsWith(".tar")), - ) - .then((entries: string[]) => entries.sort()); - - if (archives.length === 0) { + const manifestAllPath = path.join(this.path, MANIFEST_ALL_FILENAME); + const manifestAll = await Manifest.readFromFile(manifestAllPath).catch( + () => undefined, + ); + if (!manifestAll || manifestAll.entries.length === 0) { await logFileError( - `No archive files found in cache directory ${this.path}`, + `Manifest of all packages not found or empty at ${manifestAllPath}`, ); return undefined; } - for (const archiveFilename of archives) { - const archivePath = path.join(this.path, archiveFilename); + // Restore in the order apt installed the packages (i.e. the order of the + // manifest entries) and not by archive filename. Install scripts can depend + // on files from packages installed before them (see issue #196). + const archivePaths = manifestAll.entries.map((entry) => ({ + packageName: entry.packageName, + archivePath: path.join(this.path, entry.packageName.serialize() + ".tar"), + })); + for (const { archivePath } of archivePaths) { + if ( + !(await fs + .access(archivePath, fs.constants.R_OK) + .then(() => true) + .catch(() => false)) + ) { + await logFileError(`Archive file not found at ${archivePath}`); + return undefined; + } + } + + for (const { packageName, archivePath } of archivePaths) { await tar.extract({ cwd: "/", file: archivePath, @@ -209,7 +220,7 @@ export class Cache { }); if (executeInstallScripts) { - await this.runInstallScripts(archiveFilename); + await this.runInstallScripts(packageName); } } diff --git a/test/cache-restore.test.ts b/test/cache-restore.test.ts new file mode 100644 index 0000000..d98ceab --- /dev/null +++ b/test/cache-restore.test.ts @@ -0,0 +1,106 @@ +import fs from "node:fs"; +import path from "node:path"; +import * as ghcache from "@actions/cache"; +import * as tar from "tar"; +import { createPackageName } from "ts-apt"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import winston from "winston"; + +import { + Cache, + CacheKey, + CACHE_DEFAULT_DIRNAME, + MANIFEST_ALL_FILENAME, + MANIFEST_MAIN_FILENAME, +} from "../src/cache.js"; +import { Manifest, ManifestEntry } from "../src/manifest.js"; +import { ActionPackageNames } from "../src/packages.js"; + +// Cache resolves its directory from os.homedir(), so point it at a temp dir. +const { HOME_DIR } = await vi.hoisted(async () => { + const nodeFs = await import("node:fs"); + const nodeOs = await import("node:os"); + const nodePath = await import("node:path"); + return { + HOME_DIR: nodeFs.mkdtempSync( + nodePath.join(nodeOs.tmpdir(), "cache-restore-test-"), + ), + }; +}); + +vi.mock("node:os", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, homedir: () => HOME_DIR }; +}); +vi.mock("@actions/cache", () => ({ restoreCache: vi.fn() })); +vi.mock("tar", () => ({ extract: vi.fn(), create: vi.fn() })); + +const CACHE_DIR = path.join(HOME_DIR, CACHE_DEFAULT_DIRNAME); +const REQUESTED_PACKAGE = createPackageName("app", "1.0"); +// Install order from apt, deliberately not alphabetical. +const INSTALL_ORDER = [ + createPackageName("zlib1g", "1.2"), + createPackageName("libfoo", "2.0"), + REQUESTED_PACKAGE, +]; +const KEY = new CacheKey( + "v1", + "1", + "x86_64", + ActionPackageNames.fromInput(REQUESTED_PACKAGE.serialize()), +); + +const archivePath = (packageName: { serialize(): string }) => + path.join(CACHE_DIR, packageName.serialize() + ".tar"); + +async function writeCache(archivedPackages = INSTALL_ORDER): Promise { + fs.mkdirSync(CACHE_DIR, { recursive: true }); + const created = new Date(0); + await new Manifest( + created, + [new ManifestEntry(REQUESTED_PACKAGE)], + KEY, + ).writeToFile(path.join(CACHE_DIR, MANIFEST_MAIN_FILENAME)); + await new Manifest( + created, + INSTALL_ORDER.map((pkg) => new ManifestEntry(pkg)), + KEY, + ).writeToFile(path.join(CACHE_DIR, MANIFEST_ALL_FILENAME)); + archivedPackages.forEach((pkg) => fs.writeFileSync(archivePath(pkg), "")); +} + +function createCache(): Cache { + const logger = winston.createLogger({ silent: true }); + return new Cache({ run: vi.fn() } as never, logger); +} + +describe("Cache.loadAndRestore", () => { + beforeEach(() => { + vi.mocked(ghcache.restoreCache).mockResolvedValue(KEY.hash); + }); + + afterEach(() => { + fs.rmSync(CACHE_DIR, { recursive: true, force: true }); + vi.clearAllMocks(); + }); + + it("loadAndRestore cache hit extracts archives in install order", async () => { + await writeCache(); + + const manifest = await createCache().loadAndRestore(KEY, false); + + expect(manifest).toBeDefined(); + expect( + vi.mocked(tar.extract).mock.calls.map(([options]) => options?.file), + ).toEqual(INSTALL_ORDER.map(archivePath)); + }); + + it("loadAndRestore archive missing returns undefined without extracting", async () => { + await writeCache(INSTALL_ORDER.slice(1)); + + const manifest = await createCache().loadAndRestore(KEY, false); + + expect(manifest).toBeUndefined(); + expect(tar.extract).not.toHaveBeenCalled(); + }); +});