From 79721099c8174dfaa946b9a07718fabc8edbaf63 Mon Sep 17 00:00:00 2001 From: Andrew Walsh Date: Thu, 5 Nov 2026 16:26:07 -0800 Subject: [PATCH] fix: restore cached packages in apt install order (#228) 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(); + }); +});