From 764929f435c098d042d3ca6418debbf81d2e66b7 Mon Sep 17 00:00:00 2001 From: Maciej Krajowski-Kukiel Date: Mon, 27 Jul 2026 17:31:58 +0200 Subject: [PATCH] fix pos-cli module update --- CHANGELOG.md | 8 + CLAUDE.md | 4 + bin/pos-cli-data-export.js | 10 +- lib/downloadFile.js | 59 ++++-- lib/modules/downloadModule.js | 172 ++++++++++++---- lib/modules/orchestrator.js | 16 +- lib/modules/staging.js | 47 +++++ test/unit/downloadFile.test.js | 119 +++++++++++ test/unit/downloadModule.test.js | 165 ++++++++++++--- test/unit/generators.test.js | 90 ++++---- test/unit/modulesStaging.test.js | 70 +++++++ test/unit/modulesUpdateIntegrity.test.js | 252 +++++++++++++++++++++++ 12 files changed, 884 insertions(+), 128 deletions(-) create mode 100644 lib/modules/staging.js create mode 100644 test/unit/downloadFile.test.js create mode 100644 test/unit/modulesStaging.test.js create mode 100644 test/unit/modulesUpdateIntegrity.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a6268b9..154d15d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ ## Unreleased +### Fixes + +* `pos-cli modules install/update` no longer leaves a module directory whose manifest claims a version its files don't match — the state behind "the version was bumped but the code is stale and a file is missing". Archives are now unpacked into a throwaway staging directory under `tmp/` and swapped into `modules/` only once complete, so an interrupted install leaves either the old module or no module, and both are correctly re-downloaded next run. The swap moves the old directory aside rather than unpacking over it, so files a new version no longer ships are actually removed. Previously the module directory was deleted and the archive unpacked over it in place: an interruption left a partial tree, and because staleness is detected by reading the version out of the module's own `pos-module.json`, a partial tree that had already written that file looked up-to-date to every later `install` and `update` — the stale code then shipped on the next deploy. +* `pos-cli modules install/update` now rejects an archive that does not contain the expected `/` directory. Such an archive used to delete `modules/`, unpack its differently-named root beside it, and still report success. +* `pos-cli modules install/update` waits for every module download to settle before reporting a failure. A failing module used to abort the command while its siblings were still being replaced on disk, so a Ctrl-C at the error prompt could interrupt an install that appeared to be over. All download failures are now reported together instead of only the first. +* `pos-module.lock.json` is written only after every module has downloaded successfully. Writing it up front recorded versions that were never installed, and the next run — seeing the lock and the modules that *did* download agree — had no way to tell the install was incomplete. +* File downloads (module archives, data exports, `pos-cli pull`) now fail on a non-2xx response instead of saving the error body as the downloaded file, and follow redirects. An expired presigned URL answers `403` with an XML body, which used to be written out as a `.zip` and only surfaced later as a corrupt-archive error. Signed query strings are stripped from error messages. + ## 6.3.0 (2026-07-27) ### New Features diff --git a/CLAUDE.md b/CLAUDE.md index cab6642a..d33e3371 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -283,6 +283,10 @@ project/ Run all commands from project root (one level above `app/` or `modules/`). +**Module staging** (`lib/modules/staging.js`). `pos-cli modules install/update` never downloads or extracts into an installed module directory. Each archive is fetched into its own throwaway staging directory under `tmp/pos-cli-module-staging/` and unpacked there; only after it is verified to contain the expected `/` root is it published with two renames — the old `modules/` moves into the staging directory, then the staged tree is renamed onto `modules/`. Moving the old tree aside wholesale (rather than unpacking over it) is what makes files deleted between two versions actually disappear; the second rename being atomic is what guarantees `modules/` is never observed half-written. A half-written module whose `pos-module.json` already reports the target version is indistinguishable from an up-to-date one (see `modulesNotOnDisk`), so it would never be repaired. + +Staging lives under the project's `tmp/` (already pos-cli's scratch area — deploy writes `tmp/release.zip` there) for two reasons: publishing ends in a `rename()` onto `modules/`, which fails with `EXDEV` across filesystems, so `os.tmpdir()` is not safe to use; and nothing enumerates project-root `tmp/`, since every glob over modules runs with `cwd` set to `modules/` and sync only watches `dir.toWatch()`. A staging directory therefore cannot be deployed, packed, or synced by construction, with no per-enumerator exclusions to maintain. + #### API Architecture Main endpoints (`${INSTANCE_URL}/api/app_builder/`): - `/marketplace_releases` (POST) - Deploy archive diff --git a/bin/pos-cli-data-export.js b/bin/pos-cli-data-export.js index c5840407..0011e733 100755 --- a/bin/pos-cli-data-export.js +++ b/bin/pos-cli-data-export.js @@ -64,7 +64,15 @@ program }; const handleZipFileExport = (exportTask, filename) => { - downloadFile(exportTask.zip_file_url, filename).then(exportFinished); + // Not chained into the caller's promise, so it needs its own handler: downloadFile + // rejects on a non-2xx response (e.g. an expired presigned URL). + downloadFile(exportTask.zip_file_url, filename) + .then(exportFinished) + .catch((e) => { + spinner.fail('Export failed'); + logger.Error(e.message); + report('[ERR] Data: Export - Failed'); + }); }; const handleJsonFileExport = (exportTask, filename) => { diff --git a/lib/downloadFile.js b/lib/downloadFile.js index ed7c199d..313720f9 100644 --- a/lib/downloadFile.js +++ b/lib/downloadFile.js @@ -1,20 +1,51 @@ import fs from 'fs'; -import https from 'https'; -import http from 'http'; +import { Readable } from 'stream'; +import { pipeline } from 'stream/promises'; -const downloadFile = (url, fileName) => { - return new Promise((resolve, reject) => { - let file = fs.createWriteStream(fileName).on('close', () => resolve()); - const request = url.startsWith('https') ? https : http; - request.get(url, response => { - response.pipe(file); - file - .on('finish', () => { - file.close(resolve); - }) - .on('error', reject); +/** + * Download URLs are frequently presigned S3 links whose query string carries the + * signature. Strip it before putting a URL in an error message. + */ +const safeUrl = (url) => { + try { + const { origin, pathname } = new URL(url); + return `${origin}${pathname}`; + } catch { + return url; + } +}; + +/** + * Downloads `url` to `fileName`. + * + * Rejects on any non-2xx response instead of writing the error body to disk: an + * expired presigned URL answers 403 with an XML body, and silently saving that as + * a .zip turned an auth failure into a corrupt-archive error further down. + * + * Rejections carry `statusCode` so callers can distinguish 404 from other failures. + */ +const downloadFile = async (url, fileName) => { + let response; + + try { + response = await fetch(url); // follows redirects, and caps the chain itself + } catch (error) { + // Wrapped rather than rethrown: fetch's own message can echo the whole signed URL, + // and `cause` keeps the network error code reachable (see ServerError). + throw new Error(`Download failed for ${safeUrl(url)}: ${error.cause?.message ?? error.message}`, { + cause: error }); - }); + } + + if (!response.ok) { + const error = new Error(`Download failed with HTTP ${response.status}: ${safeUrl(url)}`); + error.statusCode = response.status; + throw error; + } + + // pipeline (unlike pipe) forwards source errors, so a connection dropped mid-download + // rejects instead of leaving this promise pending forever. + await pipeline(Readable.fromWeb(response.body), fs.createWriteStream(fileName)); }; export default downloadFile; diff --git a/lib/modules/downloadModule.js b/lib/modules/downloadModule.js index 03930eec..c1fc43d5 100644 --- a/lib/modules/downloadModule.js +++ b/lib/modules/downloadModule.js @@ -1,73 +1,153 @@ -import { randomUUID } from 'crypto'; import logger from '../logger.js'; import downloadFile from '../downloadFile.js'; import { unzip } from '../unzip.js'; import Portal from '../portal.js'; import fs from 'fs'; import path from 'path'; -import os from 'os'; -import { getModulesDir, getModulePath, POS_MODULE_FILE, TEMPLATE_VALUES_FILE } from './paths.js'; +import { getModulePath, POS_MODULE_FILE, TEMPLATE_VALUES_FILE } from './paths.js'; +import { createStagingDir, clearStagingBase } from './staging.js'; import { safeReadFile } from './postInstall.js'; +/** Name the downloaded archive gets inside the module's own staging directory. */ +const ARCHIVE_FILE = 'archive.zip'; + +/** + * Checks that a freshly extracted staging directory contains the expected + * `/` root, and fails loudly when it does not. + * + * Without this check a mismatched archive silently wipes `modules/` + * (it is replaced by whatever directory the archive did contain) while the command + * still reports success. + */ +const verifyStagedModule = (moduleName, version, stagingDir) => { + const extracted = fs.readdirSync(stagingDir).filter(entry => entry !== ARCHIVE_FILE); + + if (!extracted.includes(moduleName)) { + const detail = extracted.length > 0 ? `archive contains: ${extracted.join(', ')}` : 'the archive is empty'; + throw new Error(`archive does not contain a "${moduleName}/" directory (${detail})`); + } + + // A module whose own manifest disagrees with the version it was published under + // is installed anyway (the registry version is authoritative), but it is worth + // surfacing: readInstalledVersion will keep flagging it as stale on every run. + const stagedVersion = readVersionFromDir(path.join(stagingDir, moduleName)); + if (stagedVersion && stagedVersion !== version) { + logger.Warn( + `${moduleName}: published as ${version} but its manifest declares ${stagedVersion}. ` + + `The module will be re-downloaded on every install until the two agree.` + ); + } +}; + /** - * Downloads and extracts a single module archive. + * Publishes a verified, fully extracted module to `modules/` with two renames: + * the old tree moves aside into the staging directory, then the staged tree takes its place. * - * @param {string} moduleName Module name (e.g. "core"). - * @param {string} version Exact version to download. - * @param {string} [registryUrl] Registry URL for the download request. - * @param {Function} [fetchVersions] Optional: replaces Portal.moduleVersionsSearch for testing. - * Signature: (moduleWithVersion, registryUrl) => Promise<{ public_archive: string }> + * The old tree has to leave wholesale rather than be written over, because a file deleted + * between the two versions would otherwise survive the update. And only the second rename + * publishes anything, so an interrupted swap leaves either the old module or none at all — + * never a half-written tree whose manifest already claims the new version, which + * modulesNotOnDisk would then treat as up-to-date forever. + * + * The displaced copy stays inside `stagingDir` so the caller's cleanup removes it along + * with everything else, rather than needing its own cleanup path. */ -const downloadModule = async (moduleName, version, registryUrl, fetchVersions = null) => { - const fetcher = fetchVersions ?? Portal.moduleVersionsSearch.bind(Portal); +const swapIntoPlace = async (moduleName, stagingDir) => { + const stagedPath = path.join(stagingDir, moduleName); + const modulePath = getModulePath(moduleName); + const backupPath = path.join(stagingDir, `old-${moduleName}`); + + await fs.promises.mkdir(path.dirname(modulePath), { recursive: true }); + const hadPrevious = fs.existsSync(modulePath); + if (hadPrevious) await fs.promises.rename(modulePath, backupPath); + + try { + await fs.promises.rename(stagedPath, modulePath); + } catch (error) { + // Best effort: a failed rollback must not mask why the swap failed. Worst case the + // module is left absent, which the next run re-downloads. + if (hadPrevious) await fs.promises.rename(backupPath, modulePath).catch(() => {}); + throw error; + } +}; + +/** + * Downloads a single module archive and installs it atomically. + * + * @param {string} moduleName Module name (e.g. "core"). + * @param {string} version Exact version to download. + * @param {string} [registryUrl] Registry URL for the download request. + */ +const downloadModule = async (moduleName, version, registryUrl) => { const moduleWithVersion = `${moduleName}@${version}`; - // randomUUID() avoids temp-file collisions under concurrent installs of the same module. - const tmpFile = path.join(os.tmpdir(), `pos-module-${moduleName}-${randomUUID()}.zip`); + let stagingDir; + try { logger.Info(`Downloading ${moduleWithVersion}...`); - const moduleVersion = await fetcher(moduleWithVersion, registryUrl); - await downloadFile(moduleVersion['public_archive'], tmpFile); - // Remove old dir only after download succeeds — keeps the module directory - // intact if the network/registry call fails mid-stream. - await fs.promises.rm(getModulePath(moduleName), { recursive: true, force: true }); - await unzip(tmpFile, getModulesDir()); + const moduleVersion = await Portal.moduleVersionsSearch(moduleWithVersion, registryUrl); + // Archive and extraction both live in the staging directory, so `modules/` is + // only touched once a complete, verified copy is ready to swap in — and one cleanup + // covers both. + stagingDir = await createStagingDir(moduleName); + const archive = path.join(stagingDir, ARCHIVE_FILE); + await downloadFile(moduleVersion['public_archive'], archive); + await unzip(archive, stagingDir); + verifyStagedModule(moduleName, version, stagingDir); + await swapIntoPlace(moduleName, stagingDir); } catch (error) { throw new Error(`${moduleWithVersion}: ${error.statusCode === 404 ? '404 not found' : error.message}`); } finally { - await fs.promises.rm(tmpFile, { force: true }); + if (stagingDir) await fs.promises.rm(stagingDir, { recursive: true, force: true }); } }; /** + * Downloads every module in `modules` concurrently. + * + * Uses allSettled rather than Promise.all so a single failure never leaves sibling + * downloads running unsupervised — reporting failure while other modules are still being + * replaced on disk means a Ctrl-C at that prompt can leave one half-installed. All + * failures are collected and reported together. + * * @param {Object} modules { name: version } map of modules to download. * @param {Function} getRegistryUrl (name) => registryUrl — called per module so each * can be fetched from its own registry. - * @param {Function} [fetchVersions] Optional: injected fetcher forwarded to downloadModule. - * Useful for testing without a global Portal mock. */ -const downloadAllModules = async (modules, getRegistryUrl, fetchVersions = null) => { - await Promise.all( +const downloadAllModules = async (modules, getRegistryUrl) => { + const settled = await Promise.allSettled( Object.entries(modules).map(([moduleName, version]) => - downloadModule(moduleName, version, getRegistryUrl(moduleName), fetchVersions) + downloadModule(moduleName, version, getRegistryUrl(moduleName)) ) ); + + // Best effort: also clears staging leftovers from a previously killed run. Safe here + // because every download of this run has settled. + await clearStagingBase(); + + const failures = settled.filter(r => r.status === 'rejected'); + if (failures.length > 0) { + const messages = failures.map(f => f.reason?.message ?? String(f.reason)); + throw new Error( + failures.length === 1 + ? messages[0] + : `Failed to download ${failures.length} modules:\n ${messages.join('\n ')}` + ); + } }; const readJsonVersion = (filePath) => safeReadFile(filePath, (raw) => JSON.parse(raw).version ?? null); /** - * Reads the `version` field recorded in an installed module's own manifest: - * modules//pos-module.json, falling back to modules//template-values.json - * for modules published before the pos-module.json convention existed (many - * currently-published registry modules still ship this way). Returns null when - * neither file exists, is readable, or carries a `version` field — treated the - * same as "not installed" by callers. Uses the same safe-read primitive as - * postInstall.js's module-manifest lookups (postInstall.js's safeReadFile). + * Reads the `version` recorded in a module directory's own manifest: pos-module.json, + * falling back to template-values.json for modules published before the pos-module.json + * convention existed (many currently-published registry modules still ship this way). + * Returns null when neither file exists, is readable, or carries a `version` field. */ -const readInstalledVersion = (name) => { - const dir = getModulePath(name); - return readJsonVersion(path.join(dir, POS_MODULE_FILE)) ?? readJsonVersion(path.join(dir, TEMPLATE_VALUES_FILE)); -}; +const readVersionFromDir = (dir) => + readJsonVersion(path.join(dir, POS_MODULE_FILE)) ?? readJsonVersion(path.join(dir, TEMPLATE_VALUES_FILE)); + +/** The same, for an installed module: `modules/`. null means "not installed". */ +const readInstalledVersion = (name) => readVersionFromDir(getModulePath(name)); /** * Returns the subset of modules whose installed disk version does not match @@ -76,10 +156,14 @@ const readInstalledVersion = (name) => { * source of truth and there is no "previous lock" to compare versions against. * * Checking installed disk version rather than mere directory presence catches - * modules whose directory exists but whose contents are stale or corrupted — - * e.g. deleted manually then recreated empty, a failed/partial extraction, or - * simply never updated after the lock file itself was bumped (by a teammate, - * a merge, etc.) without the module directory being refreshed locally. + * modules whose directory exists but whose contents are stale — e.g. deleted + * manually then recreated empty, or simply never updated after the lock file + * itself was bumped (by a teammate, a merge, etc.) without the module directory + * being refreshed locally. + * + * Note this can only detect staleness a module's own manifest admits to. Keeping + * installs atomic (see swapIntoPlace) is what guarantees a directory's contents + * actually match the version its manifest reports. */ const modulesNotOnDisk = (modules) => Object.fromEntries( @@ -98,4 +182,10 @@ const modulesToDownload = (modulesLocked, previousLock) => ({ ...modulesNotOnDisk(modulesLocked), }); -export { downloadModule, downloadAllModules, modulesToDownload, modulesNotOnDisk, readInstalledVersion }; +export { + downloadModule, + downloadAllModules, + modulesToDownload, + modulesNotOnDisk, + readInstalledVersion, +}; diff --git a/lib/modules/orchestrator.js b/lib/modules/orchestrator.js index 4eca7195..2233a496 100644 --- a/lib/modules/orchestrator.js +++ b/lib/modules/orchestrator.js @@ -88,13 +88,9 @@ const resolveAndDownload = async (spinner, prodModules, devModules = {}, registr (!includeDev || isLockUnchanged(resolvedDev, prevDev)) && isLockUnchanged(mergedRegistries, previousLock.registries); - if (lockUnchanged) { - spinner.succeed('Module dependencies up-to-date'); - } else { - writePosModulesLock(resolvedProd, lockDevToWrite, mergedRegistries); - spinner.succeed(`Modules lock file updated: ${POS_MODULE_LOCK_FILE}`); - } + spinner.succeed(lockUnchanged ? 'Module dependencies up-to-date' : 'Module dependencies resolved'); + // Computed before downloading: modulesToDownload inspects what is currently on disk. const toDownload = { ...modulesToDownload(resolvedProd, prevProd), ...(includeDev ? modulesToDownload(resolvedDev, prevDev) : {}), @@ -106,6 +102,14 @@ const resolveAndDownload = async (spinner, prodModules, devModules = {}, registr await downloadAllModules(toDownload, getRegistryUrl); spinner.succeed(`Modules downloaded successfully${skipNote}`); + // Written only after every download succeeded. Writing it earlier recorded versions + // that were never installed, and the next run — seeing lock and disk agree on the + // modules that did download — had no way to tell the install was incomplete. + if (!lockUnchanged) { + writePosModulesLock(resolvedProd, lockDevToWrite, mergedRegistries); + spinner.succeed(`Modules lock file updated: ${POS_MODULE_LOCK_FILE}`); + } + const allPrevious = includeDev ? { ...prevProd, ...prevDev } : prevProd; printDiff(allPrevious, allResolved); diff --git a/lib/modules/staging.js b/lib/modules/staging.js new file mode 100644 index 00000000..2988af5e --- /dev/null +++ b/lib/modules/staging.js @@ -0,0 +1,47 @@ +/** + * Staging area used to unpack module archives before they are swapped into + * `modules/`. + * + * A module is never extracted over its installed directory: the extraction lands in + * a throwaway directory first, and only a complete, verified copy is moved into place + * (see swapIntoPlace in downloadModule.js). + */ + +import fs from 'fs'; +import path from 'path'; + +/** + * Staging lives under the project's `tmp/` — pos-cli's existing scratch directory, where + * deploy already writes `tmp/release.zip`. + * + * Two properties make that the right home. Publishing a module ends in a `rename()` onto + * `modules/`, and `rename()` fails outright with EXDEV across filesystems, so the + * staging area has to sit under the project root rather than in `os.tmpdir()` — a tmpfs + * `/tmp` or a project on another drive would break it. And nothing enumerates project-root + * `tmp/`: every glob over modules runs with `cwd` set to `modules/` (lib/archive.js, + * lib/assets/packAssets.js, lib/files.js) and sync only watches `dir.toWatch()`, so a + * half-extracted tree can never be deployed, packed, or synced. + */ +const getStagingBase = () => path.join(process.cwd(), 'tmp', 'pos-cli-module-staging'); + +/** + * Creates an empty directory to extract one module archive into. + * + * mkdtemp supplies the random suffix and creates the directory atomically, so two + * concurrent runs — or two installs of the same module — can never land in the same place. + */ +const createStagingDir = async (moduleName) => { + const base = getStagingBase(); + await fs.promises.mkdir(base, { recursive: true }); + + return fs.promises.mkdtemp(path.join(base, `pos-cli-unpack-${moduleName}-`)); +}; + +/** + * Removes the staging base and anything left inside it — including leftovers from an + * earlier killed run. Only safe once every download of the current run has settled. + */ +const clearStagingBase = () => + fs.promises.rm(getStagingBase(), { recursive: true, force: true }).catch(() => {}); + +export { createStagingDir, clearStagingBase, getStagingBase }; diff --git a/test/unit/downloadFile.test.js b/test/unit/downloadFile.test.js new file mode 100644 index 00000000..7d9fe526 --- /dev/null +++ b/test/unit/downloadFile.test.js @@ -0,0 +1,119 @@ +/** + * downloadFile is what fetches module archives and data exports. It used to pipe any + * response body straight to disk, so an expired presigned URL (403 + XML body) was + * saved as a .zip and only surfaced later as a corrupt-archive error. + */ +import { describe, test, expect, beforeEach, afterEach } from 'vitest'; +import fs from 'fs'; +import path from 'path'; +import http from 'http'; +import downloadFile from '#lib/downloadFile.js'; +import { withTmpDir } from '#test/utils/withTmpDir.js'; + +let server; +let baseUrl; +let handler; + +const startServer = () => + new Promise(resolve => { + server = http.createServer((req, res) => handler(req, res)); + server.listen(0, '127.0.0.1', () => { + baseUrl = `http://127.0.0.1:${server.address().port}`; + resolve(); + }); + }); + +describe('downloadFile', () => { + withTmpDir('pos-cli-downloadfile-'); + + beforeEach(async () => { + await startServer(); + }); + + afterEach(async () => { + await new Promise(resolve => server.close(resolve)); + }); + + const dest = () => path.join(process.cwd(), 'out.bin'); + + test('writes the response body to the destination file', async () => { + handler = (req, res) => res.end('archive contents'); + + await downloadFile(`${baseUrl}/file.zip`, dest()); + + expect(fs.readFileSync(dest(), 'utf8')).toBe('archive contents'); + }); + + test('rejects on a 403 instead of saving the error body as the file', async () => { + handler = (req, res) => { + res.writeHead(403, { 'Content-Type': 'application/xml' }); + res.end('AccessDenied'); + }; + + await expect(downloadFile(`${baseUrl}/file.zip`, dest())).rejects.toThrow(/HTTP 403/); + expect(fs.existsSync(dest())).toBe(false); + }); + + test('surfaces the status code so callers can special-case 404', async () => { + handler = (req, res) => { + res.writeHead(404); + res.end('nope'); + }; + + await expect(downloadFile(`${baseUrl}/file.zip`, dest())).rejects.toMatchObject({ statusCode: 404 }); + }); + + test('does not put the signed query string in the error message', async () => { + handler = (req, res) => { + res.writeHead(403); + res.end('denied'); + }; + + await expect( + downloadFile(`${baseUrl}/file.zip?X-Amz-Signature=deadbeef`, dest()) + ).rejects.toThrow(/^(?!.*X-Amz-Signature).*HTTP 403/s); + }); + + test('follows redirects', async () => { + handler = (req, res) => { + if (req.url === '/redirect') { + res.writeHead(302, { Location: '/actual.zip' }); + return res.end(); + } + res.end('redirected contents'); + }; + + await downloadFile(`${baseUrl}/redirect`, dest()); + + expect(fs.readFileSync(dest(), 'utf8')).toBe('redirected contents'); + }); + + test('rejects on a redirect loop rather than hanging', async () => { + handler = (req, res) => { + res.writeHead(302, { Location: '/loop' }); + res.end(); + }; + + await expect(downloadFile(`${baseUrl}/loop`, dest())).rejects.toThrow(/Download failed/); + }); + + test('does not put the signed query string in a transport-level error message', async () => { + const port = server.address().port; + await new Promise(resolve => server.close(resolve)); + server = http.createServer(() => {}); // keeps afterEach happy + server.listen(0, '127.0.0.1'); + + await expect( + downloadFile(`http://127.0.0.1:${port}/file.zip?X-Amz-Signature=deadbeef`, dest()) + ).rejects.toThrow(/^(?!.*X-Amz-Signature).*Download failed/s); + }); + + test('rejects when the connection fails', async () => { + const port = server.address().port; + await new Promise(resolve => server.close(resolve)); + server = http.createServer(() => {}); // keeps afterEach happy + server.listen(0, '127.0.0.1'); + + await expect(downloadFile(`http://127.0.0.1:${port}/file.zip`, dest())).rejects.toThrow(); + }); +}); diff --git a/test/unit/downloadModule.test.js b/test/unit/downloadModule.test.js index 5e5c90b9..153d92d3 100644 --- a/test/unit/downloadModule.test.js +++ b/test/unit/downloadModule.test.js @@ -8,6 +8,7 @@ import { downloadModule, downloadAllModules, } from '#lib/modules/downloadModule.js'; +import { getStagingBase } from '#lib/modules/staging.js'; import { withTmpDir } from '#test/utils/withTmpDir.js'; vi.mock('#lib/portal.js', () => ({ @@ -242,6 +243,27 @@ describe('modulesNotOnDisk', () => { }); }); +// Staging directories are named `pos-cli-unpack--`; recover the +// module name so one unzip stub can serve a whole batch of concurrent downloads. +const moduleNameFromStagingDir = (dest) => + path.basename(dest).replace(/^pos-cli-unpack-/, '').replace(/-\w+$/, ''); + +/** + * Stubs unzip the way the real one behaves: writing //… + * downloadModule verifies that root exists before it touches modules/, + * so a no-op stub would (correctly) be rejected as a malformed archive. + */ +const extractsModule = (files = {}) => async (_zipPath, dest) => { + const name = moduleNameFromStagingDir(dest); + const root = path.join(dest, name); + fs.mkdirSync(root, { recursive: true }); + fs.writeFileSync(path.join(root, 'pos-module.json'), JSON.stringify({ machine_name: name })); + for (const [rel, content] of Object.entries(files)) { + fs.mkdirSync(path.dirname(path.join(root, rel)), { recursive: true }); + fs.writeFileSync(path.join(root, rel), content); + } +}; + // downloadModule downloads a single module archive and extracts it. // Uses mocked Portal, downloadFile, and unzip to avoid real network/filesystem ops. describe('downloadModule', () => { @@ -256,7 +278,7 @@ describe('downloadModule', () => { Portal.moduleVersionsSearch.mockResolvedValue({ public_archive: 'https://example.com/core-2.0.6.zip' }); downloadFile.mockResolvedValue(undefined); - unzip.mockResolvedValue(undefined); + unzip.mockImplementation(extractsModule()); }); test('calls Portal.moduleVersionsSearch with name@version and registryUrl', async () => { @@ -268,22 +290,43 @@ describe('downloadModule', () => { ); }); - test('calls downloadFile with public_archive URL', async () => { + test('downloads the archive into the staging directory it is unzipped in', async () => { await downloadModule('core', '2.0.6'); + const [, dest] = unzip.mock.calls[0]; expect(downloadFile).toHaveBeenCalledWith( 'https://example.com/core-2.0.6.zip', - expect.stringContaining('pos-module-core-') + path.join(dest, 'archive.zip') ); }); - test('calls unzip to extract to modules/ directory', async () => { + test('extracts to a staging directory outside modules/, never over the installed module', async () => { await downloadModule('core', '2.0.6'); - expect(unzip).toHaveBeenCalledWith( - expect.any(String), - path.join(process.cwd(), 'modules') - ); + const [, dest] = unzip.mock.calls[0]; + expect(path.basename(dest)).toMatch(/^pos-cli-unpack-core-\w+$/); + expect(path.dirname(dest)).toBe(getStagingBase()); + expect(getStagingBase().startsWith(path.join(process.cwd(), 'modules'))).toBe(false); + }); + + test('removes its staging directory on success', async () => { + await downloadModule('core', '2.0.6'); + const [, dest] = unzip.mock.calls[0]; + + expect(fs.existsSync(dest)).toBe(false); + expect(fs.existsSync(path.join(process.cwd(), 'modules', 'core'))).toBe(true); + expect(fs.readdirSync(path.join(process.cwd(), 'modules'))).toEqual(['core']); + }); + + test('removes its staging directory when the swap fails', async () => { + unzip.mockImplementation(async (_zip, dest) => { + fs.mkdirSync(path.join(dest, 'wrong-root'), { recursive: true }); + }); + + await expect(downloadModule('core', '2.0.6')).rejects.toThrow(); + + const [, dest] = unzip.mock.calls[0]; + expect(fs.existsSync(dest)).toBe(false); }); test('throws formatted error message on 404', async () => { @@ -300,31 +343,62 @@ describe('downloadModule', () => { await expect(downloadModule('core', '2.0.6')).rejects.toThrow('core@2.0.6: Service Unavailable'); }); - test('cleans up temp file in finally block even when an error is thrown', async () => { - Portal.moduleVersionsSearch.mockRejectedValue(new Error('Service Unavailable')); - const rmSpy = vi.spyOn(fs.promises, 'rm'); - - await expect(downloadModule('core', '2.0.6')).rejects.toThrow(); + test('removes the downloaded archive with the staging directory when the download fails', async () => { + downloadFile.mockImplementation(async (_url, dest) => { + fs.writeFileSync(dest, 'partial'); + throw new Error('Network error'); + }); - // The finally block must call fs.promises.rm on the temp file path (force: true). - const cleanupCall = rmSpy.mock.calls.find(([p, opts]) => - typeof p === 'string' && p.includes('pos-module-core-') && opts?.force === true - ); - expect(cleanupCall).toBeDefined(); + await expect(downloadModule('core', '2.0.6')).rejects.toThrow('Network error'); - rmSpy.mockRestore(); + const [, archive] = downloadFile.mock.calls[0]; + expect(fs.existsSync(path.dirname(archive))).toBe(false); }); - test('removes old module directory before downloading', async () => { + test('replaces the old module directory wholesale — no leftovers from the old version', async () => { fs.mkdirSync(path.join(process.cwd(), 'modules', 'core'), { recursive: true }); fs.writeFileSync(path.join(process.cwd(), 'modules', 'core', 'old-file.txt'), 'old'); + unzip.mockImplementation(extractsModule({ 'new-file.txt': 'new' })); await downloadModule('core', '2.0.6'); - // unzip was called, meaning the old directory was removed and download proceeded - expect(unzip).toHaveBeenCalled(); - // The old directory should be gone (removed before download, not re-created by mock) expect(fs.existsSync(path.join(process.cwd(), 'modules', 'core', 'old-file.txt'))).toBe(false); + expect(fs.existsSync(path.join(process.cwd(), 'modules', 'core', 'new-file.txt'))).toBe(true); + }); + + test('rejects an archive whose root directory is not / and keeps the installed module', async () => { + fs.mkdirSync(path.join(process.cwd(), 'modules', 'core'), { recursive: true }); + fs.writeFileSync(path.join(process.cwd(), 'modules', 'core', 'existing-file.txt'), 'keep me'); + unzip.mockImplementation(async (_zip, dest) => { + fs.mkdirSync(path.join(dest, 'pos-module-core'), { recursive: true }); + }); + + await expect(downloadModule('core', '2.0.6')).rejects.toThrow( + /archive does not contain a "core\/" directory \(archive contains: pos-module-core\)/ + ); + + // The previously installed module must survive a malformed archive untouched. + expect(fs.existsSync(path.join(process.cwd(), 'modules', 'core', 'existing-file.txt'))).toBe(true); + expect(fs.existsSync(path.join(process.cwd(), 'modules', 'pos-module-core'))).toBe(false); + }); + + test('does NOT delete the module directory when extraction fails midway', async () => { + fs.mkdirSync(path.join(process.cwd(), 'modules', 'core'), { recursive: true }); + fs.writeFileSync(path.join(process.cwd(), 'modules', 'core', 'existing-file.txt'), 'keep me'); + unzip.mockImplementation(async (_zip, dest) => { + // Partial extraction: the manifest lands, then the process dies. + const root = path.join(dest, 'core'); + fs.mkdirSync(root, { recursive: true }); + fs.writeFileSync(path.join(root, 'pos-module.json'), JSON.stringify({ version: '2.0.6' })); + throw new Error('Unexpected end of archive'); + }); + + await expect(downloadModule('core', '2.0.6')).rejects.toThrow('Unexpected end of archive'); + + // The half-extracted tree must never reach modules/core — that is the state that + // used to look up-to-date forever while missing files. + expect(fs.existsSync(path.join(process.cwd(), 'modules', 'core', 'existing-file.txt'))).toBe(true); + expect(readInstalledVersion('core')).toBeNull(); }); test('does NOT delete module directory when downloadFile fails', async () => { @@ -353,6 +427,8 @@ describe('downloadModule', () => { // downloadAllModules iterates all modules and calls downloadModule for each. describe('downloadAllModules', () => { + withTmpDir(); + let Portal, downloadFile, unzip; beforeEach(async () => { @@ -363,7 +439,7 @@ describe('downloadAllModules', () => { vi.clearAllMocks(); Portal.moduleVersionsSearch.mockResolvedValue({ public_archive: 'https://example.com/module.zip' }); downloadFile.mockResolvedValue(undefined); - unzip.mockResolvedValue(undefined); + unzip.mockImplementation(extractsModule()); }); const REGISTRY = 'https://custom.registry.example.com'; @@ -386,10 +462,49 @@ describe('downloadAllModules', () => { downloadAllModules({ core: '2.0.6', user: '5.1.2' }, getRegistryUrl) ).rejects.toThrow(/404 not found/); - // Promise.all starts all downloads concurrently, so both modules are queried + // downloads start concurrently, so both modules are queried expect(Portal.moduleVersionsSearch).toHaveBeenCalledTimes(2); }); + test('waits for every download to settle before rejecting — no work outlives the failure', async () => { + // Bug guard: with Promise.all the command reported failure while sibling modules + // were still being replaced on disk, so a Ctrl-C at the error prompt could leave a + // module half-installed. + let slowFinished = false; + Portal.moduleVersionsSearch.mockImplementation(async (nameWithVersion) => { + if (nameWithVersion.startsWith('broken')) throw new Error('Not Found'); + return { public_archive: 'https://example.com/module.zip' }; + }); + unzip.mockImplementation(async (zipPath, dest) => { + await new Promise(resolve => setTimeout(resolve, 50)); + await extractsModule()(zipPath, dest); + slowFinished = true; + }); + + await expect( + downloadAllModules({ core: '2.0.6', broken: '1.0.0' }, getRegistryUrl) + ).rejects.toThrow('Not Found'); + + expect(slowFinished).toBe(true); + expect(fs.existsSync(path.join(process.cwd(), 'modules', 'core'))).toBe(true); + }); + + test('reports every failure, not just the first', async () => { + Portal.moduleVersionsSearch.mockImplementation(async (nameWithVersion) => { + throw new Error(`boom for ${nameWithVersion}`); + }); + + await expect( + downloadAllModules({ core: '2.0.6', user: '5.1.2' }, getRegistryUrl) + ).rejects.toThrow(/Failed to download 2 modules[\s\S]*core@2\.0\.6[\s\S]*user@5\.1\.2/); + }); + + test('removes the staging directory once all downloads settle', async () => { + await downloadAllModules({ core: '2.0.6', user: '5.1.2' }, getRegistryUrl); + + expect(fs.readdirSync(path.join(process.cwd(), 'modules')).sort()).toEqual(['core', 'user']); + }); + test('passes registryUrl to every download call', async () => { await downloadAllModules( { core: '2.0.6', user: '5.1.2', tests: '1.0.0' }, diff --git a/test/unit/generators.test.js b/test/unit/generators.test.js index f0f4301d..98defcaf 100644 --- a/test/unit/generators.test.js +++ b/test/unit/generators.test.js @@ -1,4 +1,4 @@ -import { describe, test, expect, beforeEach, afterEach } from 'vitest'; +import { describe, test, expect, beforeEach, afterEach, vi } from 'vitest'; import { exec } from 'child_process'; import path from 'path'; import fs from 'fs/promises'; @@ -9,13 +9,28 @@ const __dirname = path.dirname(__filename); const cliPath = path.join(process.cwd(), 'bin', 'pos-cli.js'); +// Every generator run spawns a real node process. In isolation each finishes in +// well under a second, but the suite runs test files in parallel, so a run can +// take several seconds under load. One generous shared ceiling keeps these tests +// from failing on scheduling noise while still catching a hung generator. +const CLI_TIMEOUT = 20000; + +// Must stay above CLI_TIMEOUT, otherwise vitest aborts the test before +// execCommand gets a chance to report what the CLI actually printed. +vi.setConfig({ testTimeout: CLI_TIMEOUT + 10000 }); + const execCommand = (cmd, opts = {}) => { return new Promise((resolve) => { - const child = exec(cmd, { ...opts, stdio: ['pipe', 'pipe', 'pipe'] }, (err, stdout, stderr) => { + // exec's own `timeout` option kills the child for us and still passes the + // output collected so far to the callback, which a manual timer would lose. + const child = exec(cmd, opts, (err, stdout, stderr) => { // Extract exit code from error or default to 0 // Different environments might use err.code, err.exitCode, or err.signal let code = 0; if (err) { + if (err.killed) { + return resolve({ stdout, stderr: `${stderr}\nTimed out after ${opts.timeout}ms`, code: null }); + } code = err.code || err.exitCode || (err.signal ? 1 : 0); } return resolve({ stdout, stderr, code }); @@ -26,13 +41,6 @@ const execCommand = (cmd, opts = {}) => { if (child.stdin) { child.stdin.end(); } - - if (opts.timeout) { - setTimeout(() => { - child.kill(); - resolve({ stdout: '', stderr: 'Test timed out', code: null }); - }, opts.timeout); - } }); }; @@ -107,7 +115,7 @@ describe('pos-cli generate command', () => { test('requires modelName argument', async () => { const { stderr } = await run( 'test/fixtures/yeoman/modules/core/generators/crud', - { cwd: testDir, timeout: 5000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); // Yeoman shows error but exits with code 0 @@ -117,7 +125,7 @@ describe('pos-cli generate command', () => { test('generates CRUD files for model with attributes', async () => { const { stdout, stderr, code } = await run( 'test/fixtures/yeoman/modules/core/generators/crud product name:string price:integer description:text active:boolean', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); if (code !== 0) { @@ -177,7 +185,7 @@ describe('pos-cli generate command', () => { test('generates view files when --includeViews option is used', async () => { const { stdout, stderr, code } = await run( 'test/fixtures/yeoman/modules/core/generators/crud article title:text --includeViews', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); if (code !== 0) { @@ -225,7 +233,7 @@ describe('pos-cli generate command', () => { test('does not generate view files without --includeViews option', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/crud book title:string', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -239,7 +247,7 @@ describe('pos-cli generate command', () => { test('pluralizes model name correctly', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/crud person name:string', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -257,7 +265,7 @@ describe('pos-cli generate command', () => { test('generates files with correct content using template variables', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/crud user name:string email:string age:integer', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -275,7 +283,7 @@ describe('pos-cli generate command', () => { test('handles model with multiple word names', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/crud blog_post title:string content:text', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -287,7 +295,7 @@ describe('pos-cli generate command', () => { test('supports array type attributes', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/crud tag name:string categories:array', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -301,7 +309,7 @@ describe('pos-cli generate command', () => { test('supports float and date types', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/crud product price:float published_at:date', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -316,7 +324,7 @@ describe('pos-cli generate command', () => { test('generates config.yml file', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/crud custom_model name:string', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -343,7 +351,7 @@ describe('pos-cli generate command', () => { test('requires commandName argument', async () => { const { stderr } = await run( 'test/fixtures/yeoman/modules/core/generators/command', - { cwd: testDir, timeout: 5000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); // Yeoman shows error but exits with code 0 @@ -353,7 +361,7 @@ describe('pos-cli generate command', () => { test('generates command files with model/action format', async () => { const { stdout, stderr, code } = await run( 'test/fixtures/yeoman/modules/core/generators/command users/create', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); if (code !== 0) { @@ -383,7 +391,7 @@ describe('pos-cli generate command', () => { test('generates command directory with build and check phases', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/command products/update', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -399,7 +407,7 @@ describe('pos-cli generate command', () => { test('parses modelName and actionName from command path', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/command orders/process_payment', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -414,7 +422,7 @@ describe('pos-cli generate command', () => { test('handles simple command name without model', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/command simple_task', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -430,7 +438,7 @@ describe('pos-cli generate command', () => { test('generates GraphQL mutation file', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/command notifications/send', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -446,7 +454,7 @@ describe('pos-cli generate command', () => { test('handles multiple directory levels', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/command admin/users/create', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -461,7 +469,7 @@ describe('pos-cli generate command', () => { test('substitutes actionName template variable correctly', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/command comments/delete', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -475,7 +483,7 @@ describe('pos-cli generate command', () => { test('substitutes modelName template variable correctly', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/command sessions/destroy', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -489,7 +497,7 @@ describe('pos-cli generate command', () => { test('generates build phase with correct structure', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/command reports/generate', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -514,7 +522,7 @@ describe('pos-cli generate command', () => { test('generates check phase with correct structure', async () => { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/command documents/approve', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -550,7 +558,7 @@ describe('pos-cli generate command', () => { test('shows help for crud generator', async () => { const { stdout, code } = await run( 'test/fixtures/yeoman/modules/core/generators/crud --generator-help', - { cwd: testDir, timeout: 5000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -563,7 +571,7 @@ describe('pos-cli generate command', () => { test('shows help for command generator', async () => { const { stdout, code } = await run( 'test/fixtures/yeoman/modules/core/generators/command --generator-help', - { cwd: testDir, timeout: 5000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -590,7 +598,7 @@ describe('pos-cli generate command', () => { test('runs custom generator from non-module path', async () => { const { stdout, stderr, code } = await run( 'test/fixtures/yeoman/custom/generators/simple myitem --auto-confirm', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); if (code !== 0) { @@ -614,7 +622,7 @@ describe('pos-cli generate command', () => { test('shows help for custom generator', async () => { const { stdout, code } = await run( 'test/fixtures/yeoman/custom/generators/simple --generator-help --auto-confirm', - { cwd: testDir, timeout: 5000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -636,7 +644,7 @@ describe('pos-cli generate command', () => { const { code } = await run( 'test/fixtures/yeoman/custom/generators/simple testitem --auto-confirm', - { cwd: testDir, timeout: 15000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); // Generator should still work after installing dependencies @@ -645,7 +653,7 @@ describe('pos-cli generate command', () => { // Verify dependencies were installed const installed = await fileExists(nodeModulesPath); expect(installed).toBe(true); - }, 20000); + }); }); describe('error handling and validation', () => { @@ -664,7 +672,7 @@ describe('pos-cli generate command', () => { test('fails with clear error for non-existent generator path', async () => { const { stderr, code } = await run( 'test/fixtures/nonexistent/generator', - { cwd: testDir, timeout: 5000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).not.toBe(0); @@ -678,7 +686,7 @@ describe('pos-cli generate command', () => { const { stderr, code } = await run( badGenPath, - { cwd: testDir, timeout: 5000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).not.toBe(0); @@ -713,7 +721,7 @@ export default class extends Generator { // The important thing is it doesn't crash with a different error const { stderr, code } = await run( genPath, - { cwd: testDir, timeout: 5000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); // It's okay if this fails due to missing dependencies @@ -726,7 +734,7 @@ export default class extends Generator { test('handles deeply nested generator paths', async () => { const { stdout, code } = await run( 'test/fixtures/yeoman/modules/core/generators/crud product name:string', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); @@ -759,7 +767,7 @@ export default class extends Generator { const { code } = await run( 'test/fixtures/yeoman/modules/core/generators/crud item name:string', - { cwd: testDir, timeout: 10000 } + { cwd: testDir, timeout: CLI_TIMEOUT } ); expect(code).toBe(0); diff --git a/test/unit/modulesStaging.test.js b/test/unit/modulesStaging.test.js new file mode 100644 index 00000000..00575cc8 --- /dev/null +++ b/test/unit/modulesStaging.test.js @@ -0,0 +1,70 @@ +import { describe, test, expect, afterEach } from 'vitest'; +import fs from 'fs'; +import path from 'path'; +import { getStagingBase, createStagingDir, clearStagingBase } from '#lib/modules/staging.js'; +import { withTmpDir } from '#test/utils/withTmpDir.js'; + +withTmpDir(); + +describe('getStagingBase', () => { + // Not os.tmpdir(): publishing a module ends in a rename() onto modules/, which + // fails with EXDEV across filesystems, so staging has to live under the project root. + test('is a directory under the project, inside pos-cli\'s existing tmp/ scratch area', () => { + expect(getStagingBase()).toBe(path.join(process.cwd(), 'tmp', 'pos-cli-module-staging')); + }); +}); + +describe('createStagingDir', () => { + const created = []; + const create = async (name) => { + const dir = await createStagingDir(name); + created.push(dir); + return dir; + }; + + afterEach(() => { + while (created.length) fs.rmSync(created.pop(), { recursive: true, force: true }); + }); + + test('creates an empty directory named for the module, inside the staging base', async () => { + const dir = await create('core'); + + expect(fs.readdirSync(dir)).toEqual([]); + expect(path.dirname(dir)).toBe(getStagingBase()); + expect(path.basename(dir)).toMatch(/^pos-cli-unpack-core-\w+$/); + }); + + test('creates the staging base on demand', async () => { + expect(fs.existsSync(getStagingBase())).toBe(false); + + await create('core'); + + expect(fs.existsSync(getStagingBase())).toBe(true); + }); + + test('never collides, so concurrent installs of the same module cannot share a directory', async () => { + const dirs = await Promise.all(['core', 'core', 'core'].map(() => create('core'))); + + expect(new Set(dirs).size).toBe(3); + }); + + test('stages outside modules/, so a half-extracted tree is never enumerated as a module', async () => { + await create('core'); + + expect(fs.existsSync(path.join(process.cwd(), 'modules'))).toBe(false); + }); +}); + +describe('clearStagingBase', () => { + test('removes the base along with leftovers from an earlier killed run', async () => { + fs.mkdirSync(path.join(getStagingBase(), 'pos-cli-unpack-core-leftover'), { recursive: true }); + + await clearStagingBase(); + + expect(fs.existsSync(getStagingBase())).toBe(false); + }); + + test('is a no-op when the base was never created', async () => { + await expect(clearStagingBase()).resolves.toBeUndefined(); + }); +}); diff --git a/test/unit/modulesUpdateIntegrity.test.js b/test/unit/modulesUpdateIntegrity.test.js new file mode 100644 index 00000000..9fbbde1e --- /dev/null +++ b/test/unit/modulesUpdateIntegrity.test.js @@ -0,0 +1,252 @@ +/** + * End-to-end integrity tests for `pos-cli modules install/update`. + * + * Unlike the other module unit tests these drive real zip archives through the real + * unzip and the real filesystem — only the registry HTTP calls are faked. They exist + * because the failure they guard against is invisible to mocked-extraction tests: a + * module directory whose manifest reports the target version while its contents are + * incomplete is treated as up-to-date forever, so every later install and update + * silently skips it and the app deploys stale code. + */ +import { describe, test, expect, vi } from 'vitest'; +import fs from 'fs'; +import path from 'path'; +import os from 'os'; +import glob from 'fast-glob'; +import prepareArchive from '#lib/prepareArchive.js'; +import { withTmpDir } from '#test/utils/withTmpDir.js'; +import { makeFileHelpers } from '#test/utils/fileHelpers.js'; +import { makeSpinner } from '#test/utils/spinnerMock.js'; +import { mod, makeRegistry } from '#test/utils/moduleRegistry.js'; + +// Fake registry archives: "name@version" -> zip path on disk. +const archives = new Map(); + +vi.mock('#lib/portal.js', () => ({ + default: { + moduleVersionsSearch: vi.fn(async (nameWithVersion) => { + const archive = archives.get(nameWithVersion); + if (!archive) { + const error = new Error('Not Found'); + error.statusCode = 404; + throw error; + } + return { public_archive: `file://${archive}` }; + }), + moduleVersions: vi.fn(), + }, +})); + +// Serves the fake archives from disk instead of over HTTP. +vi.mock('#lib/downloadFile.js', () => ({ + default: vi.fn(async (url, dest) => { + await fs.promises.copyFile(url.replace('file://', ''), dest); + }), +})); + +const { updateModules } = await import('#lib/modules/update.js'); +const { installModules } = await import('#lib/modules/install.js'); +const { writePosModulesLock, readPosModulesLock } = await import('#lib/modules/configFiles.js'); +const registry = await import('#lib/modules/registry.js'); + +const REGISTRY = 'https://partners.platformos.com'; +const spinner = makeSpinner(); + +/** + * Publishes a zip to the fake registry, laid out the way `pos-cli modules push` builds it. + * `root` overrides the archive's root directory, which is normally the module name. + */ +const publish = async (name, version, files, root = name) => { + const zipPath = path.join(os.tmpdir(), `pos-cli-test-${name}-${version}-${process.pid}.zip`); + const archive = prepareArchive(zipPath); + archive.addBuffer(Buffer.from(JSON.stringify({ machine_name: name, version })), `${root}/pos-module.json`); + for (const [rel, content] of Object.entries(files)) { + archive.addBuffer(Buffer.from(content), `${root}/${rel}`); + } + archive.finalize(); + await archive.done; + archives.set(`${name}@${version}`, zipPath); +}; + +const useRegistry = (...modules) => + vi.spyOn(registry, 'createGetVersions').mockReturnValue(makeRegistry(...modules)); + +const readManifest = () => JSON.parse(fs.readFileSync('pos-module.json', 'utf8')); + +/** Writes a module to disk as a completed install would leave it. */ +const installed = (name, version, files = {}) => { + const dir = path.join(process.cwd(), 'modules', name); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'pos-module.json'), JSON.stringify({ machine_name: name, version })); + for (const [rel, content] of Object.entries(files)) { + fs.mkdirSync(path.dirname(path.join(dir, rel)), { recursive: true }); + fs.writeFileSync(path.join(dir, rel), content); + } +}; + +/** Sorted list of files inside modules/, or null when it does not exist. */ +const filesOf = (name) => { + const dir = path.join(process.cwd(), 'modules', name); + if (!fs.existsSync(dir)) return null; + return glob.sync('**/*', { cwd: dir, onlyFiles: true, dot: true }).sort(); +}; + +const fileHelpers = makeFileHelpers(withTmpDir('pos-cli-update-integrity-')); +const writeManifest = (dependencies) => fileHelpers.writeManifest({ dependencies }); + +describe('modules update — resolution and download', () => { + test('bumps a range dependency and installs the files the new version adds', async () => { + await publish('core', '1.0.0', { 'public/views/old.liquid': 'old' }); + await publish('core', '1.1.0', { 'public/views/old.liquid': 'old', 'public/views/new.liquid': 'new' }); + useRegistry(mod('core', { '1.0.0': {}, '1.1.0': {} })); + + writeManifest({ core: '^1.0.0' }); + writePosModulesLock({ core: '1.0.0' }, {}, { core: REGISTRY }); + installed('core', '1.0.0', { 'public/views/old.liquid': 'old' }); + + await updateModules(spinner, 'core'); + + expect(readPosModulesLock().dependencies).toEqual({ core: '1.1.0' }); + expect(filesOf('core')).toContain('public/views/new.liquid'); + }); + + // The new version is moved into place, not merged over the old one: a file the new + // version dropped would otherwise linger and keep being deployed. + test('drops files the new version no longer ships', async () => { + await publish('core', '1.0.0', { 'public/views/gone.liquid': 'gone', 'public/views/kept.liquid': 'kept' }); + await publish('core', '1.1.0', { 'public/views/kept.liquid': 'kept' }); + useRegistry(mod('core', { '1.0.0': {}, '1.1.0': {} })); + + writeManifest({ core: '^1.0.0' }); + writePosModulesLock({ core: '1.0.0' }, {}, { core: REGISTRY }); + installed('core', '1.0.0', { 'public/views/gone.liquid': 'gone', 'public/views/kept.liquid': 'kept' }); + + await updateModules(spinner, 'core'); + + expect(filesOf('core')).toEqual(['pos-module.json', 'public/views/kept.liquid']); + }); + + test('bumps an exact pin in the manifest and downloads the new version', async () => { + await publish('core', '1.0.0', {}); + await publish('core', '1.1.0', { 'public/views/new.liquid': 'new' }); + useRegistry(mod('core', { '1.0.0': {}, '1.1.0': {} })); + + writeManifest({ core: '1.0.0' }); + writePosModulesLock({ core: '1.0.0' }, {}, { core: REGISTRY }); + installed('core', '1.0.0'); + + await updateModules(spinner, 'core'); + + expect(readManifest().dependencies).toEqual({ core: '1.1.0' }); + expect(filesOf('core')).toContain('public/views/new.liquid'); + }); + + test('downloads a transitive dependency bumped behind an unchanged root range', async () => { + await publish('core', '1.0.0', {}); + await publish('common', '1.0.0', {}); + await publish('common', '1.2.0', { 'public/views/new.liquid': 'new' }); + useRegistry( + mod('core', { '1.0.0': { common: '^1.0.0' } }), + mod('common', { '1.0.0': {}, '1.2.0': {} }) + ); + + writeManifest({ core: '^1.0.0' }); + writePosModulesLock({ core: '1.0.0', common: '1.0.0' }, {}, { core: REGISTRY, common: REGISTRY }); + installed('core', '1.0.0'); + installed('common', '1.0.0'); + + await updateModules(spinner, undefined, {}); + + expect(readPosModulesLock().dependencies).toEqual({ core: '1.0.0', common: '1.2.0' }); + expect(filesOf('common')).toContain('public/views/new.liquid'); + }); + + test('re-downloads a module the lock moved ahead of (e.g. after a git pull)', async () => { + await publish('core', '1.1.0', { 'public/views/new.liquid': 'new' }); + useRegistry(mod('core', { '1.0.0': {}, '1.1.0': {} })); + + writeManifest({ core: '^1.0.0' }); + writePosModulesLock({ core: '1.1.0' }, {}, { core: REGISTRY }); + installed('core', '1.0.0', { 'public/views/old.liquid': 'old' }); + + await installModules(spinner, undefined, {}); + + expect(filesOf('core')).toEqual(['pos-module.json', 'public/views/new.liquid']); + }); +}); + +describe('modules update — install integrity', () => { + test('an interrupted download never leaves a partially written module directory', async () => { + // The archive is fine; extraction dies partway through, as it would on Ctrl-C, + // a full disk, or a locked file on Windows. + await publish('core', '1.1.0', { 'public/views/new.liquid': 'new' }); + useRegistry(mod('core', { '1.0.0': {}, '1.1.0': {} })); + + writeManifest({ core: '^1.0.0' }); + writePosModulesLock({ core: '1.0.0' }, {}, { core: REGISTRY }); + installed('core', '1.0.0', { 'public/views/old.liquid': 'old' }); + + const unzipModule = await import('#lib/unzip.js'); + const realUnzip = unzipModule.unzip; + const spy = vi.spyOn(unzipModule, 'unzip').mockImplementation(async (zipPath, dest) => { + await realUnzip(zipPath, dest); + throw new Error('Unexpected end of archive'); + }); + + try { + await expect(updateModules(spinner, 'core')).rejects.toThrow('Unexpected end of archive'); + } finally { + spy.mockRestore(); + } + + // The installed module is untouched and still honestly reports 1.0.0, so the + // next run knows it has work to do. + expect(filesOf('core')).toEqual(['pos-module.json', 'public/views/old.liquid']); + // The failed version was never recorded as installed. + expect(readPosModulesLock().dependencies).toEqual({ core: '1.0.0' }); + + // Re-running repairs it. + await updateModules(spinner, 'core'); + expect(filesOf('core')).toEqual(['pos-module.json', 'public/views/new.liquid']); + expect(readPosModulesLock().dependencies).toEqual({ core: '1.1.0' }); + }); + + test('a failed download for one module does not record any module as installed', async () => { + await publish('core', '1.1.0', { 'public/views/new.liquid': 'new' }); + // "broken" resolves in the registry but has no archive published. + useRegistry( + mod('core', { '1.0.0': {}, '1.1.0': {} }), + mod('broken', { '1.0.0': {}, '1.1.0': {} }) + ); + + writeManifest({ core: '^1.0.0', broken: '^1.0.0' }); + writePosModulesLock({ core: '1.0.0', broken: '1.0.0' }, {}, { core: REGISTRY, broken: REGISTRY }); + installed('core', '1.0.0', { 'public/views/old.liquid': 'old' }); + installed('broken', '1.0.0', { 'public/views/old.liquid': 'old' }); + + await expect(updateModules(spinner, undefined, {})).rejects.toThrow(/broken@1\.1\.0: 404 not found/); + + // The lock still describes what is actually installed: core downloaded fine, but + // recording broken@1.1.0 would make the next run believe the install completed. + expect(readPosModulesLock().dependencies).toEqual({ core: '1.0.0', broken: '1.0.0' }); + expect(filesOf('broken')).toEqual(['pos-module.json', 'public/views/old.liquid']); + }); + + test('rejects an archive whose root directory does not match the module name', async () => { + // Guards a silent wipe: the module directory used to be deleted and the archive's + // differently-named root extracted alongside it, while the command reported success. + await publish('core', '1.1.0', { 'public/views/new.liquid': 'new' }, 'pos-module-core'); + + useRegistry(mod('core', { '1.0.0': {}, '1.1.0': {} })); + writeManifest({ core: '^1.0.0' }); + writePosModulesLock({ core: '1.0.0' }, {}, { core: REGISTRY }); + installed('core', '1.0.0', { 'public/views/old.liquid': 'old' }); + + await expect(updateModules(spinner, 'core')).rejects.toThrow( + /archive does not contain a "core\/" directory/ + ); + + expect(filesOf('core')).toEqual(['pos-module.json', 'public/views/old.liquid']); + expect(fs.readdirSync(path.join(process.cwd(), 'modules'))).toEqual(['core']); + }); +});