diff --git a/__tests__/tar.test.ts b/__tests__/tar.test.ts index 4145d9a..ed4ab42 100644 --- a/__tests__/tar.test.ts +++ b/__tests__/tar.test.ts @@ -88,6 +88,8 @@ test('zstd extract tar with windows BSDtar', async () => { if (IS_WINDOWS) { const mkdirMock = jest.spyOn(io, 'mkdirP') const execMock = jest.spyOn(exec, 'exec') + const tempDirectory = getTempDir() + jest.spyOn(utils, 'createTempDirectory').mockResolvedValue(tempDirectory) jest .spyOn(utils, 'getGnuTarPathOnWindows') .mockReturnValue(Promise.resolve('')) @@ -106,11 +108,11 @@ test('zstd extract tar with windows BSDtar', async () => { [ 'zstd -d --long=30 --force -o', TarFilename.replace(new RegExp(`\\${path.sep}`, 'g'), '/'), - archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/') + `"${archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/')}"` ].join(' '), undefined, { - cwd: undefined, + cwd: tempDirectory, env: expect.objectContaining(defaultEnv) } ) @@ -123,11 +125,11 @@ test('zstd extract tar with windows BSDtar', async () => { TarFilename.replace(new RegExp(`\\${path.sep}`, 'g'), '/'), '-P', '-C', - workspace?.replace(/\\/g, '/') + `"${workspace?.replace(/\\/g, '/')}"` ].join(' '), undefined, { - cwd: undefined, + cwd: tempDirectory, env: expect.objectContaining(defaultEnv) } ) @@ -381,6 +383,8 @@ test('zstd list tar', async () => { test('zstd list tar with windows BSDtar', async () => { if (IS_WINDOWS) { const execMock = jest.spyOn(exec, 'exec') + const tempDirectory = getTempDir() + jest.spyOn(utils, 'createTempDirectory').mockResolvedValue(tempDirectory) jest .spyOn(utils, 'getGnuTarPathOnWindows') .mockReturnValue(Promise.resolve('')) @@ -396,11 +400,11 @@ test('zstd list tar with windows BSDtar', async () => { [ 'zstd -d --long=30 --force -o', TarFilename.replace(new RegExp(`\\${path.sep}`, 'g'), '/'), - archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/') + `"${archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/')}"` ].join(' '), undefined, { - cwd: undefined, + cwd: tempDirectory, env: expect.objectContaining(defaultEnv) } ) @@ -415,7 +419,7 @@ test('zstd list tar with windows BSDtar', async () => { ].join(' '), undefined, { - cwd: undefined, + cwd: tempDirectory, env: expect.objectContaining(defaultEnv) } ) diff --git a/__tests__/tarConcurrency.test.ts b/__tests__/tarConcurrency.test.ts new file mode 100644 index 0000000..b9ebde5 --- /dev/null +++ b/__tests__/tarConcurrency.test.ts @@ -0,0 +1,226 @@ +import * as core from '@actions/core' +import * as exec from '@actions/exec' +import * as io from '@actions/io' +import * as fs from 'fs' +import * as path from 'path' +import * as tar from '../src/internal/tar' +import * as utils from '../src/internal/cacheUtils' +import {CompressionMethod} from '../src/internal/constants' + +jest.mock('@actions/exec') + +const describeWindows = process.platform === 'win32' ? describe : describe.skip +const readers = {extract: tar.extractTar, list: tar.listTar} +type Operation = keyof typeof readers + +interface Fixture { + file: string + content: string +} + +describeWindows.each([ + CompressionMethod.Zstd, + CompressionMethod.ZstdWithoutLong +])('Windows BSD tar isolation (%s)', compression => { + const fixtures: Fixture[] = [ + {file: 'first.txt', content: 'first archive'}, + {file: 'second.txt', content: 'second archive'} + ] + const originalEnv = {...process.env} + let root: string + let workspace: string + let archives: string[] + let listedFiles: string[] + let failStage: 'decompress' | 'tar' | undefined + let afterDecompression: () => Promise + + beforeEach(async () => { + jest.restoreAllMocks() + const tempRoot = path.join(__dirname, '_temp') + await fs.promises.mkdir(tempRoot, {recursive: true}) + root = await fs.promises.mkdtemp(path.join(tempRoot, 'tar-concurrency-')) + workspace = path.join(root, 'workspace') + const archiveDirectory = path.join(root, 'archives') + await fs.promises.mkdir(workspace) + await fs.promises.mkdir(archiveDirectory) + archives = fixtures.map((fixture, index) => { + const archive = path.join(archiveDirectory, `${index}.tzst`) + fs.writeFileSync(archive, JSON.stringify(fixture)) + return archive + }) + fs.writeFileSync(path.join(root, 'cache.tar'), 'unrelated existing file') + listedFiles = [] + failStage = undefined + afterDecompression = async () => {} + process.env = { + ...originalEnv, + GITHUB_WORKSPACE: workspace, + RUNNER_TEMP: root + } + jest.spyOn(utils, 'getGnuTarPathOnWindows').mockResolvedValue('') + + // Model the filesystem effects of zstd and tar without requiring either + // program to be installed. The fixture content stands in for archive data. + jest + .spyOn(exec, 'exec') + .mockImplementation(async (command, args, options) => { + const argv = (command.match(/"[^"]*"|\S+/g) || []).map(argument => + argument.replace(/^"|"$/g, '') + ) + argv.push(...(args || [])) + // Redirect the default command cwd to avoid writing into the checkout. + const cwd = options?.cwd || root + if (argv[0] === 'zstd') { + const source = path.resolve(cwd, argv[argv.length - 1]) + const output = path.resolve(cwd, argv[argv.indexOf('-o') + 1]) + fs.copyFileSync(source, output) + if (failStage === 'decompress') { + throw new Error('simulated decompression failure') + } + await afterDecompression() + return 0 + } + if (failStage === 'tar') { + throw new Error('simulated tar failure') + } + const extracting = argv.includes('-xf') + const source = path.resolve( + cwd, + argv[argv.indexOf(extracting ? '-xf' : '-tf') + 1] + ) + const fixture: Fixture = JSON.parse(fs.readFileSync(source, 'utf8')) + if (extracting) { + const destination = path.resolve(cwd, argv[argv.indexOf('-C') + 1]) + await fs.promises.mkdir(destination, {recursive: true}) + fs.writeFileSync( + path.join(destination, fixture.file), + fixture.content + ) + } else { + listedFiles.push(fixture.file) + } + return 0 + }) + }) + + afterEach(async () => { + jest.restoreAllMocks() + process.env = {...originalEnv} + await fs.promises.rm(root, {recursive: true, force: true}) + }) + + function expectCleanScratch(): void { + expect(fs.readdirSync(root).sort()).toEqual([ + 'archives', + 'cache.tar', + 'workspace' + ]) + expect(fs.readFileSync(path.join(root, 'cache.tar'), 'utf8')).toBe( + 'unrelated existing file' + ) + for (const [index, archive] of archives.entries()) { + expect(JSON.parse(fs.readFileSync(archive, 'utf8'))).toEqual( + fixtures[index] + ) + } + } + + test.each<[Operation, Operation]>([ + ['extract', 'extract'], + ['list', 'list'], + ['extract', 'list'] + ])('keeps concurrent %s and %s results separate', async (first, second) => { + let arrivals = 0 + let release: () => void = () => {} + const bothDecompressed = new Promise(resolve => { + release = resolve + }) + afterDecompression = async () => { + if (++arrivals === 2) { + release() + } + await bothDecompressed + } + + // Both inputs share a parent directory. Force their decompressions to + // finish before either tar reads, reproducing the old shared-file race. + const operations = [first, second] + await Promise.all( + operations.map(async (operation, index) => { + await readers[operation](archives[index], compression) + }) + ) + + expect(fs.readdirSync(workspace).sort()).toEqual( + fixtures + .filter((_, index) => operations[index] === 'extract') + .map(f => f.file) + ) + expect(listedFiles.sort()).toEqual( + fixtures + .filter((_, index) => operations[index] === 'list') + .map(f => f.file) + ) + expectCleanScratch() + }) + + test('preserves relative archive and workspace paths containing spaces', async () => { + const archive = path.join(root, 'archives', 'archive with spaces.tar') + fs.renameSync(archives[0], archive) + archives[0] = archive + const relativeArchive = path.relative(process.cwd(), archive) + const destination = path.join(workspace, 'directory with spaces') + process.env['GITHUB_WORKSPACE'] = path.relative(process.cwd(), destination) + + await tar.extractTar(relativeArchive, compression) + + expect( + fs.readFileSync(path.join(destination, fixtures[0].file), 'utf8') + ).toBe(fixtures[0].content) + expectCleanScratch() + }) + + test.each<[Operation, boolean]>([ + ['extract', false], + ['extract', true], + ['list', false], + ['list', true] + ])( + 'preserves the %s result when cleanup fails (command fails=%s)', + async (operation, commandFails) => { + const debugMock = jest.spyOn(core, 'debug').mockImplementation(() => {}) + jest + .spyOn(io, 'rmRF') + .mockRejectedValue(new Error('simulated cleanup failure')) + failStage = commandFails ? 'tar' : undefined + + const result = readers[operation](archives[0], compression) + if (commandFails) { + await expect(result).rejects.toThrow('simulated tar failure') + } else { + await expect(result).resolves.toBeUndefined() + } + expect(debugMock).toHaveBeenCalledWith( + expect.stringContaining('simulated cleanup failure') + ) + } + ) + + test.each<[Operation, 'decompress' | 'tar']>([ + ['extract', 'decompress'], + ['extract', 'tar'], + ['list', 'decompress'], + ['list', 'tar'] + ])( + 'cleans temporary files when %s fails during %s', + async (operation, stage) => { + failStage = stage + + await expect( + readers[operation](archives[0], compression) + ).rejects.toThrow('simulated') + + expectCleanScratch() + } + ) +}) diff --git a/src/internal/tar.ts b/src/internal/tar.ts index 73c126c..cad3ac6 100644 --- a/src/internal/tar.ts +++ b/src/internal/tar.ts @@ -1,3 +1,4 @@ +import * as core from '@actions/core' import {exec} from '@actions/exec' import * as io from '@actions/io' import {existsSync, writeFileSync} from 'fs' @@ -14,6 +15,11 @@ import { const IS_WINDOWS = process.platform === 'win32' +interface TarCommands { + commands: string[] + requiresTempDirectory: boolean +} + // Returns tar path and type: BSD or GNU async function getTarPath(): Promise { switch (process.platform) { @@ -95,7 +101,9 @@ async function getTarArgs( : archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/'), '-P', '-C', - workingDirectory.replace(new RegExp(`\\${path.sep}`, 'g'), '/') + BSD_TAR_ZSTD + ? quoteAbsolutePath(workingDirectory) + : workingDirectory.replace(new RegExp(`\\${path.sep}`, 'g'), '/') ) break case 'list': @@ -129,7 +137,7 @@ async function getCommands( compressionMethod: CompressionMethod, type: string, archivePath = '' -): Promise { +): Promise { let args const tarPath = await getTarPath() @@ -154,17 +162,20 @@ async function getCommands( args = [[...tarArgs].join(' '), [...compressionArgs].join(' ')] } - if (BSD_TAR_ZSTD) { - return args + return { + commands: BSD_TAR_ZSTD ? args : [args.join(' ')], + requiresTempDirectory: BSD_TAR_ZSTD && type !== 'create' } - - return [args.join(' ')] } function getWorkingDirectory(): string { return process.env['GITHUB_WORKSPACE'] ?? process.cwd() } +function quoteAbsolutePath(filePath: string): string { + return `"${path.resolve(filePath).replace(new RegExp(`\\${path.sep}`, 'g'), '/')}"` +} + // Common function for extractTar and listTar to get the compression method async function getDecompressionProgram( tarPath: ArchiveTool, @@ -185,7 +196,7 @@ async function getDecompressionProgram( ? [ 'zstd -d --long=30 --force -o', TarFilename, - archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/') + quoteAbsolutePath(archivePath) ] : [ '--use-compress-program', @@ -193,11 +204,7 @@ async function getDecompressionProgram( ] case CompressionMethod.ZstdWithoutLong: return BSD_TAR_ZSTD - ? [ - 'zstd -d --force -o', - TarFilename, - archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/') - ] + ? ['zstd -d --force -o', TarFilename, quoteAbsolutePath(archivePath)] : ['--use-compress-program', IS_WINDOWS ? '"zstd -d"' : 'unzstd'] default: return ['-z'] @@ -260,13 +267,41 @@ async function execCommands(commands: string[], cwd?: string): Promise { } } +async function execReadCommands( + archivePath: string, + compressionMethod: CompressionMethod, + type: 'list' | 'extract' +): Promise { + const {commands, requiresTempDirectory} = await getCommands( + compressionMethod, + type, + archivePath + ) + // The Windows BSD-tar fallback decompresses into cache.tar before reading + // it. Isolate that intermediate for each list/extract operation, including + // debug listings during saves. + const tempDirectory = requiresTempDirectory + ? await utils.createTempDirectory() + : undefined + try { + await execCommands(commands, tempDirectory) + } finally { + if (tempDirectory) { + try { + await io.rmRF(tempDirectory) + } catch (error) { + core.debug(`Failed to delete temporary tar directory: ${error}`) + } + } + } +} + // List the contents of a tar export async function listTar( archivePath: string, compressionMethod: CompressionMethod ): Promise { - const commands = await getCommands(compressionMethod, 'list', archivePath) - await execCommands(commands) + await execReadCommands(archivePath, compressionMethod, 'list') } // Extract a tar @@ -277,8 +312,7 @@ export async function extractTar( // Create directory to extract tar into const workingDirectory = getWorkingDirectory() await io.mkdirP(workingDirectory) - const commands = await getCommands(compressionMethod, 'extract', archivePath) - await execCommands(commands) + await execReadCommands(archivePath, compressionMethod, 'extract') } // Create a tar @@ -292,6 +326,6 @@ export async function createTar( path.join(archiveFolder, ManifestFilename), sourceDirectories.join('\n') ) - const commands = await getCommands(compressionMethod, 'create') + const {commands} = await getCommands(compressionMethod, 'create') await execCommands(commands, archiveFolder) }