From 0b7767f2ab458fe86200905f19f7aa36ca0d99e5 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 8 Jul 2026 13:27:56 -0600 Subject: [PATCH 1/3] Fix deploy silently truncating tarball on dangling symlink A dangling symlink in the project tree made harper deploy ship a truncated component: tar-fs packages with dereference:true, and when the walker stats a broken symlink target it treats the resulting ENOENT as end-of-stream, finalizing the archive early with no error. Every entry queued after the link in the walk order was silently dropped, while deploy_component reported success. Skip dangling symlinks during packaging so the walk continues past them, and warn the CLI user which links were skipped so the omission is visible instead of silent. Co-Authored-By: Claude Sonnet 5 --- bin/cliOperations.ts | 16 ++++- components/packageComponent.ts | 67 ++++++++++++++++++- unitTests/components/packageComponent.test.js | 60 ++++++++++++++++- 3 files changed, 139 insertions(+), 4 deletions(-) diff --git a/bin/cliOperations.ts b/bin/cliOperations.ts index 7e25ba0943..df01e3acac 100644 --- a/bin/cliOperations.ts +++ b/bin/cliOperations.ts @@ -9,7 +9,12 @@ import { httpRequest } from '../utility/common_utils.ts'; import * as path from 'path'; import * as fs from 'fs-extra'; import * as YAML from 'yaml'; -import { streamPackagedDirectory, getPackagedDirectorySize, packageDirectory } from '../components/packageComponent.ts'; +import { + streamPackagedDirectory, + getPackagedDirectorySize, + packageDirectory, + findDanglingSymlinks, +} from '../components/packageComponent.ts'; import { encode as encodeCbor } from 'cbor-x'; import { buildMultipartBody } from './multipartBuilder.ts'; import { parseSSE } from './sseConsumer.ts'; @@ -116,6 +121,15 @@ const PREPARE_OPERATION: any = { // Pre-walk the directory for an uncompressed-size estimate. Both the progress counter // and this total are in uncompressed units so the bar tracks to 100% naturally. req._uploadSizeEstimate = await getPackagedDirectorySize(projectPath, packageOptions); + // A dangling symlink would otherwise silently truncate the tarball (tar-fs finalizes + // early on the broken target). Packaging now skips them; warn so the omission is visible. + const danglingSymlinks = await findDanglingSymlinks(projectPath, packageOptions); + if (danglingSymlinks.length) { + process.stderr.write( + `warning: skipping ${danglingSymlinks.length} broken symlink(s) — their linked content will NOT be deployed:\n` + + danglingSymlinks.map((p) => ` ${p}\n`).join('') + ); + } req._multipart = true; }, }; diff --git a/components/packageComponent.ts b/components/packageComponent.ts index 957cd80f56..a735cb04ae 100644 --- a/components/packageComponent.ts +++ b/components/packageComponent.ts @@ -1,5 +1,6 @@ import { join, relative, sep } from 'node:path'; import { stat, readdir } from 'node:fs/promises'; +import { lstatSync, statSync } from 'node:fs'; import { Readable } from 'node:stream'; import tar from 'tar-fs'; import { createGzip } from 'node:zlib'; @@ -26,6 +27,27 @@ function isExcluded(directory: string, fullPath: string, options: PackageOptions return rel.split(sep).includes('node_modules') || rel.includes(WEBPACK_CACHE_SEGMENT); } +/** + * A dangling symlink is one whose target does not exist. Under tar-fs's `dereference: true` + * mode (our default), such a link makes the walker's `fs.stat` throw ENOENT, which tar-fs + * treats as end-of-stream: it *finalizes the archive early*, silently dropping every entry + * queued after the link — with no error emitted. Detecting and skipping these links keeps the + * package complete. Returns `false` for non-symlinks and for symlinks whose target resolves. + */ +function isDanglingSymlink(fullPath: string): boolean { + try { + if (!lstatSync(fullPath).isSymbolicLink()) return false; + } catch { + return false; // unreadable — leave it for the packer's own stat handling + } + try { + statSync(fullPath); // follows the link; throws if the target is missing + return false; + } catch { + return true; + } +} + /** * Package a directory into a tar+gzip stream. The returned Readable can be * piped directly into an HTTP request body, avoiding the Node.js 2GB Buffer @@ -40,9 +62,16 @@ export function streamPackagedDirectory( options: PackageOptions = DEFAULT_OPTIONS, onBytes?: (n: number) => void ): Readable { + const dereference = !options.skip_symlinks; const packStream = tar.pack(directory, { - dereference: !options.skip_symlinks, - ignore: (name: string) => isExcluded(directory, name, options), + dereference, + ignore: (name: string) => { + if (isExcluded(directory, name, options)) return true; + // Under dereference a dangling symlink silently truncates the archive (tar-fs + // finalizes early on the target's ENOENT), so skip it. When not dereferencing, + // tar-fs packs the link literally and never stats the target — nothing to guard. + return dereference && isDanglingSymlink(name); + }, map: (header) => { if (header.type === 'directory') { header.mode = 0o755; @@ -102,6 +131,40 @@ export async function getPackagedDirectorySize( return total; } +/** + * Walk `directory` and return the relative paths of dangling symlinks that + * `streamPackagedDirectory` would skip. The CLI uses this to warn the user before deploy, + * since a dangling link means its intended content is absent from the package (and, before + * the skip guard, would have silently truncated the whole tarball). Returns `[]` when + * `skip_symlinks` is set, because links are then packed literally with no dereference. + */ +export async function findDanglingSymlinks( + directory: string, + options: PackageOptions = DEFAULT_OPTIONS +): Promise { + if (options.skip_symlinks) return []; + const found: string[] = []; + const walk = async (dir: string): Promise => { + let entries; + try { + entries = await readdir(dir, { withFileTypes: true }); + } catch { + return; // unreadable directory — skip + } + for (const entry of entries) { + const fullPath = join(dir, entry.name); + if (isExcluded(directory, fullPath, options)) continue; + if (entry.isSymbolicLink()) { + if (isDanglingSymlink(fullPath)) found.push(relative(directory, fullPath)); + } else if (entry.isDirectory()) { + await walk(fullPath); + } + } + }; + await walk(directory); + return found; +} + /** * Package a directory into a tar+gzip buffer. Retained for callers that need * an in-memory payload (small deploys, tests). For large directories prefer diff --git a/unitTests/components/packageComponent.test.js b/unitTests/components/packageComponent.test.js index 43dd6ce342..6d5dedc1e5 100644 --- a/unitTests/components/packageComponent.test.js +++ b/unitTests/components/packageComponent.test.js @@ -10,7 +10,7 @@ const { Readable } = require('node:stream'); const testUtils = require('../testUtils.js'); testUtils.preTestPrep(); -const { streamPackagedDirectory, packageDirectory } = require('#src/components/packageComponent'); +const { streamPackagedDirectory, packageDirectory, findDanglingSymlinks } = require('#src/components/packageComponent'); const { buildMultipartBody } = require('#src/bin/multipartBuilder'); const { parseMultipartRequest } = require('#src/server/serverHelpers/multipartParser'); const gunzip = require('gunzip-maybe'); @@ -133,6 +133,64 @@ describe('streamPackagedDirectory round-trip', () => { await fs.rm(extractDir, { recursive: true, force: true }); } }); + + it('does not truncate the archive when a dangling symlink is present (regression)', async function () { + this.timeout(15000); + // A dangling symlink used to silently truncate the tarball: under dereference tar-fs + // stat()s the missing target, gets ENOENT, and finalizes the archive early — dropping + // every entry queued after the link, with no error. The packager must now skip the + // broken link and ship the full tree. + const sourceFiles = { + 'package.json': '{"name":"demo","version":"1.0.0"}\n', + 'src/resources/index.js': 'exports.x = 1;\n', + 'tests/a.test.js': 'test();\n', + '.github/workflows/ci.yml': 'name: ci\n', + }; + const sourceDir = await makeFixture(sourceFiles); + // Broken link + a valid link (to real content) that must still be dereferenced. + await fs.mkdir(path.join(sourceDir, '.claude'), { recursive: true }); + await fs.symlink('../../.agents/skills/nope', path.join(sourceDir, '.claude', 'broken')); + await fs.symlink(path.join(sourceDir, 'src'), path.join(sourceDir, 'linked-src')); + const extractDir = await fs.mkdtemp(path.join(os.tmpdir(), 'pkg-dangling-out-')); + try { + await pipeline( + streamPackagedDirectory(sourceDir, { skip_node_modules: true }), + gunzip(), + tar.extract(extractDir) + ); + const extracted = await readDirTree(extractDir); + // Every real file survives (nothing dropped after the broken link)... + for (const [rel, content] of Object.entries(sourceFiles)) { + assert.strictEqual(extracted[rel], content, `missing/altered ${rel}`); + } + // ...the valid symlink's target content is dereferenced in... + assert.strictEqual(extracted['linked-src/resources/index.js'], sourceFiles['src/resources/index.js']); + // ...and the dangling link itself is not packed as a broken entry. + assert.ok(!Object.keys(extracted).some((k) => k.includes('broken')), 'dangling link should be skipped'); + } finally { + await fs.rm(sourceDir, { recursive: true, force: true }); + await fs.rm(extractDir, { recursive: true, force: true }); + } + }); + + it('findDanglingSymlinks reports broken links and ignores valid ones and node_modules', async function () { + this.timeout(15000); + const sourceDir = await makeFixture({ 'index.js': 'x\n', 'src/a.js': 'a\n' }); + await fs.symlink('./does-not-exist', path.join(sourceDir, 'broken-root')); + await fs.symlink('../nope', path.join(sourceDir, 'src', 'broken-nested')); + await fs.symlink(path.join(sourceDir, 'src'), path.join(sourceDir, 'good')); + // A dangling link inside node_modules must be ignored when skipping node_modules. + await fs.mkdir(path.join(sourceDir, 'node_modules'), { recursive: true }); + await fs.symlink('./gone', path.join(sourceDir, 'node_modules', 'broken-dep')); + try { + const dangling = (await findDanglingSymlinks(sourceDir, { skip_node_modules: true })).sort(); + assert.deepStrictEqual(dangling, ['broken-root', path.join('src', 'broken-nested')]); + // With skip_symlinks the archive packs links literally (no dereference), so nothing to warn. + assert.deepStrictEqual(await findDanglingSymlinks(sourceDir, { skip_symlinks: true }), []); + } finally { + await fs.rm(sourceDir, { recursive: true, force: true }); + } + }); }); // keep eslint happy in case Readable isn't directly used in some branch From 8c60730b119488215623c5ba495311f7bfc7ea97 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 8 Jul 2026 13:46:30 -0600 Subject: [PATCH 2/3] Address cross-model review: async dangling-symlink scan, not sync per-entry stat Gemini flagged that tar-fs's ignore callback is synchronous (no Promise support), so the previous fix's per-entry lstatSync/statSync would have added blocking I/O to a path that also runs inline on the Harper server's event loop (package_component). Consolidate size + dangling detection into one scanPackageDirectory() async walker; the pack's ignore callback now does a cheap Set lookup against a pre-resolved set. Also fixes a correctness gap the sync version had: the walker now recurses into valid symlinked directories (matching tar-fs's own dereferenced traversal), so a dangling symlink nested inside one is still caught instead of still truncating the archive. The CLI's prepare step now does a single combined scan instead of two separate walks, and threads the result into streamPackagedDirectory to avoid a third. Add a DESIGN.md entry capturing the tar-fs early-finalize mechanism. Co-Authored-By: Claude Sonnet 5 --- DESIGN.md | 23 ++ bin/cliOperations.ts | 30 ++- components/packageComponent.ts | 220 ++++++++++-------- unitTests/bin/cliOperations.test.js | 11 +- unitTests/components/packageComponent.test.js | 40 +++- 5 files changed, 200 insertions(+), 124 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index d75c668f5a..1e0c5c42bf 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -264,3 +264,26 @@ replicability metadata is deferred to the cluster-level-config work (CORE-3018), schema. Per-peer failures never reject: they come back as `{status: 'failed', reason, node}` entries in `response.replicated[]`, and `message` still reads as success (same contract as drop_schema), so operators must inspect the array for per-node outcomes. + +## A dangling symlink silently truncates the deploy tarball (`components/packageComponent.ts`) + +Packaging uses `tar-fs.pack(dir, { dereference: true })` by default (`skip_symlinks` off). +tar-fs's own walker calls `fs.stat` (not `lstat`) on every discovered entry when dereferencing; a +dangling symlink's target throws `ENOENT`, and tar-fs's `statAll` loop treats _any_ `ENOENT` from +a walk-discovered (not explicitly-requested) entry as end-of-stream — it calls `pack.finalize()` +immediately, silently dropping every entry still queued (BFS order) after the link. No error is +ever emitted, so `packStream.on('error', ...)` never fires and `deploy_component` reports success +on a truncated archive. `scanPackageDirectory()` now pre-walks the tree once (async) to build a +skip-set of dangling symlinks, which `streamPackagedDirectory`'s `tar.pack({ ignore })` consults via +a synchronous `Set.has()` — **`ignore` is called synchronously by tar-fs with no Promise support**, +so any fix here has to resolve the dangling set _before_ constructing `tar.pack`, not from inside +the callback (an earlier draft used `lstatSync`/`statSync` per entry there, which would have added +blocking I/O to a path that also runs inline on the Harper server's event loop via the +`package_component` operation). The scan recurses into _valid_ symlinked directories the same way +tar-fs's dereferenced walk does (readdir through the link), since a dangling symlink nested inside +one is just as capable of tripping the same early-finalize — skipping recursion into symlinked +dirs there would silently reintroduce the bug for that case. Circular directory symlinks are not +guarded against (in the scan or in tar-fs's own pack walk); that's a pre-existing tar-fs limitation +this fix doesn't attempt to solve. `deploy_component`/`package_component` still never validate that +declared entry points (`jsResource`/`graphqlSchema`) survived extraction — a truncation from some +other future cause would still report success silently; that's a deferred, separate fix. diff --git a/bin/cliOperations.ts b/bin/cliOperations.ts index df01e3acac..9ff41eef52 100644 --- a/bin/cliOperations.ts +++ b/bin/cliOperations.ts @@ -9,12 +9,7 @@ import { httpRequest } from '../utility/common_utils.ts'; import * as path from 'path'; import * as fs from 'fs-extra'; import * as YAML from 'yaml'; -import { - streamPackagedDirectory, - getPackagedDirectorySize, - packageDirectory, - findDanglingSymlinks, -} from '../components/packageComponent.ts'; +import { streamPackagedDirectory, packageDirectory, scanPackageDirectory } from '../components/packageComponent.ts'; import { encode as encodeCbor } from 'cbor-x'; import { buildMultipartBody } from './multipartBuilder.ts'; import { parseSSE } from './sseConsumer.ts'; @@ -118,16 +113,18 @@ const PREPARE_OPERATION: any = { // so the pre-gzip onBytes callback can be wired directly to renderer.countUploadBytes. req._projectPath = projectPath; req._packageOptions = packageOptions; - // Pre-walk the directory for an uncompressed-size estimate. Both the progress counter - // and this total are in uncompressed units so the bar tracks to 100% naturally. - req._uploadSizeEstimate = await getPackagedDirectorySize(projectPath, packageOptions); - // A dangling symlink would otherwise silently truncate the tarball (tar-fs finalizes - // early on the broken target). Packaging now skips them; warn so the omission is visible. - const danglingSymlinks = await findDanglingSymlinks(projectPath, packageOptions); - if (danglingSymlinks.length) { + // Pre-walk the directory once for both the uncompressed-size estimate (progress bar + // total) and the dangling-symlink list — a dangling symlink would otherwise silently + // truncate the tarball (tar-fs finalizes early on the broken target). Packaging skips + // them; the list is reused below (no second walk) and warns the user which links were + // skipped so the omission is visible. + const scan = await scanPackageDirectory(projectPath, packageOptions); + req._uploadSizeEstimate = scan.totalSize; + req._danglingSymlinks = scan.danglingSymlinks; + if (scan.danglingSymlinks.length) { process.stderr.write( - `warning: skipping ${danglingSymlinks.length} broken symlink(s) — their linked content will NOT be deployed:\n` + - danglingSymlinks.map((p) => ` ${p}\n`).join('') + `warning: skipping ${scan.danglingSymlinks.length} broken symlink(s) — their linked content will NOT be deployed:\n` + + scan.danglingSymlinks.map((p) => ` ${p}\n`).join('') ); } req._multipart = true; @@ -320,7 +317,8 @@ async function cliOperations(req: any, skipResponseLog = false) { const packageStream = streamPackagedDirectory( req._projectPath, req._packageOptions, - renderer ? (n) => renderer.countUploadBytes(n) : undefined + renderer ? (n) => renderer.countUploadBytes(n) : undefined, + req._danglingSymlinks ); const fields = operationFields(req); const multipart = buildMultipartBody(fields, { diff --git a/components/packageComponent.ts b/components/packageComponent.ts index a735cb04ae..61e2de6420 100644 --- a/components/packageComponent.ts +++ b/components/packageComponent.ts @@ -1,6 +1,5 @@ import { join, relative, sep } from 'node:path'; import { stat, readdir } from 'node:fs/promises'; -import { lstatSync, statSync } from 'node:fs'; import { Readable } from 'node:stream'; import tar from 'tar-fs'; import { createGzip } from 'node:zlib'; @@ -19,7 +18,7 @@ const WEBPACK_CACHE_SEGMENT = join('cache', 'webpack'); * `skip_node_modules` is set. The path is first made relative to `directory`, so packaging a component * that itself lives under a `node_modules/` path — i.e. any npm-installed component — does not match * every entry. tar-fs invokes `ignore` with the absolute path, so a substring test on it wrongly - * excluded the whole tree. Shared by the stream packer and the size walk so the two cannot diverge. + * excluded the whole tree. Shared by the stream packer and the directory walk so they cannot diverge. */ function isExcluded(directory: string, fullPath: string, options: PackageOptions): boolean { if (!options.skip_node_modules) return false; @@ -27,81 +26,39 @@ function isExcluded(directory: string, fullPath: string, options: PackageOptions return rel.split(sep).includes('node_modules') || rel.includes(WEBPACK_CACHE_SEGMENT); } -/** - * A dangling symlink is one whose target does not exist. Under tar-fs's `dereference: true` - * mode (our default), such a link makes the walker's `fs.stat` throw ENOENT, which tar-fs - * treats as end-of-stream: it *finalizes the archive early*, silently dropping every entry - * queued after the link — with no error emitted. Detecting and skipping these links keeps the - * package complete. Returns `false` for non-symlinks and for symlinks whose target resolves. - */ -function isDanglingSymlink(fullPath: string): boolean { - try { - if (!lstatSync(fullPath).isSymbolicLink()) return false; - } catch { - return false; // unreadable — leave it for the packer's own stat handling - } - try { - statSync(fullPath); // follows the link; throws if the target is missing - return false; - } catch { - return true; - } +export interface PackageDirectoryScan { + /** Total uncompressed byte size of the files `streamPackagedDirectory` would pack. */ + totalSize: number; + /** + * Relative paths of symlinks whose target does not exist. Under tar-fs's `dereference: true` + * (our default), such a link makes the walker's stat() throw ENOENT, which tar-fs treats as + * end-of-stream: it *finalizes the archive early*, silently dropping every entry queued after + * the link. `streamPackagedDirectory` skips these paths so the walk continues past them. + */ + danglingSymlinks: string[]; } /** - * Package a directory into a tar+gzip stream. The returned Readable can be - * piped directly into an HTTP request body, avoiding the Node.js 2GB Buffer - * cap that the buffered variant runs into for large components. + * Walk `directory` once, computing both the packable size total and the dangling-symlink list — + * the two pieces of metadata the CLI's pre-deploy checks and the packer's ignore-set both need. + * A single shared walker keeps size accounting and dangling-link detection from silently + * diverging (e.g. one recursing into symlinked directories and the other not). * - * @param onBytes - Optional callback invoked with the byte length of each raw - * tar chunk *before* gzip compression. Useful for tracking upload progress - * against an uncompressed-size total (e.g. from `getPackagedDirectorySize`). + * Recurses into a *valid* symlinked directory the same way tar-fs's own dereferenced walk does + * (readdir through the link), so a dangling symlink nested inside one is still found — otherwise + * tar-fs would hit that nested ENOENT and truncate the archive regardless of this scan's result. + * This mirrors tar-fs's own lack of symlink-cycle protection under `dereference`; a circular + * symlink can already hang tar-fs's real pack walk today, so this scan doesn't guard against it + * either — fixing that is a separate, pre-existing limitation outside this change's scope. */ -export function streamPackagedDirectory( +export async function scanPackageDirectory( directory: string, - options: PackageOptions = DEFAULT_OPTIONS, - onBytes?: (n: number) => void -): Readable { + options: PackageOptions = DEFAULT_OPTIONS +): Promise { + let totalSize = 0; + const danglingSymlinks: string[] = []; const dereference = !options.skip_symlinks; - const packStream = tar.pack(directory, { - dereference, - ignore: (name: string) => { - if (isExcluded(directory, name, options)) return true; - // Under dereference a dangling symlink silently truncates the archive (tar-fs - // finalizes early on the target's ENOENT), so skip it. When not dereferencing, - // tar-fs packs the link literally and never stats the target — nothing to guard. - return dereference && isDanglingSymlink(name); - }, - map: (header) => { - if (header.type === 'directory') { - header.mode = 0o755; - } - return header; - }, - }); - const gzip = createGzip(); - // Propagate pack errors onto the gzip stream so a single consumer can listen - packStream.on('error', (err) => gzip.destroy(err)); - if (onBytes) { - // Attaching a 'data' listener after pipe() is safe — the stream is already - // in flowing mode and Node's EventEmitter supports multiple listeners. - packStream.on('data', (chunk: Buffer) => onBytes(chunk.length)); - } - return packStream.pipe(gzip); -} -/** - * Walk `directory` and return the total uncompressed size of all files that - * `streamPackagedDirectory` would include with the same options. Used by the - * CLI to give the upload progress bar a realistic total. The uncompressed size - * won't equal the gzipped wire size, but it gives the bar a steady trajectory: - * the bar moves as bytes are sent and snaps to 100% when the upload finishes. - */ -export async function getPackagedDirectorySize( - directory: string, - options: PackageOptions = DEFAULT_OPTIONS -): Promise { - let total = 0; const walk = async (dir: string): Promise => { let entries; try { @@ -111,16 +68,28 @@ export async function getPackagedDirectorySize( } for (const entry of entries) { const fullPath = join(dir, entry.name); - if (isExcluded(directory, fullPath, options)) { - continue; - } - if (entry.isDirectory()) { + if (isExcluded(directory, fullPath, options)) continue; + if (entry.isSymbolicLink()) { + // Without dereference, tar-fs packs the link literally and never stats the + // target, so an unresolved link is neither a size contributor nor a hazard. + if (!dereference) continue; + let target; + try { + target = await stat(fullPath); // follows the link; throws if missing + } catch { + danglingSymlinks.push(relative(directory, fullPath)); + continue; + } + if (target.isDirectory()) { + await walk(fullPath); + } else { + totalSize += target.size; + } + } else if (entry.isDirectory()) { await walk(fullPath); } else { - if (options.skip_symlinks && entry.isSymbolicLink()) continue; try { - const s = await stat(fullPath); // follows symlinks, matching tar dereference - total += s.size; + totalSize += (await stat(fullPath)).size; } catch { // inaccessible file — skip } @@ -128,41 +97,86 @@ export async function getPackagedDirectorySize( } }; await walk(directory); - return total; + return { totalSize, danglingSymlinks }; } /** - * Walk `directory` and return the relative paths of dangling symlinks that - * `streamPackagedDirectory` would skip. The CLI uses this to warn the user before deploy, - * since a dangling link means its intended content is absent from the package (and, before - * the skip guard, would have silently truncated the whole tarball). Returns `[]` when - * `skip_symlinks` is set, because links are then packed literally with no dereference. + * Package a directory into a tar+gzip stream. The returned Readable can be + * piped directly into an HTTP request body, avoiding the Node.js 2GB Buffer + * cap that the buffered variant runs into for large components. + * + * @param onBytes - Optional callback invoked with the byte length of each raw + * tar chunk *before* gzip compression. Useful for tracking upload progress + * against an uncompressed-size total (e.g. from `getPackagedDirectorySize`). + * @param knownDanglingSymlinks - Skip this function's own directory scan and reuse a + * dangling-symlink list the caller already computed (e.g. via `scanPackageDirectory` + * during a prior CLI step), avoiding a second full-tree walk before packing starts. + */ +export function streamPackagedDirectory( + directory: string, + options: PackageOptions = DEFAULT_OPTIONS, + onBytes?: (n: number) => void, + knownDanglingSymlinks?: string[] +): Readable { + const dereference = !options.skip_symlinks; + const gzip = createGzip(); + // Resolving the dangling-symlink set up front — rather than stat-ing candidates + // synchronously from tar-fs's `ignore` callback — keeps the pack walk free of blocking + // I/O. That matters here because this also runs server-side, inline on the shared event + // loop, via the package_component operation. + const dangling = knownDanglingSymlinks + ? Promise.resolve(knownDanglingSymlinks) + : scanPackageDirectory(directory, options).then((scan) => scan.danglingSymlinks); + dangling + .then((danglingSymlinks) => { + const danglingSet = new Set(danglingSymlinks); + const packStream = tar.pack(directory, { + dereference, + ignore: (name: string) => + isExcluded(directory, name, options) || (dereference && danglingSet.has(relative(directory, name))), + map: (header) => { + if (header.type === 'directory') { + header.mode = 0o755; + } + return header; + }, + }); + // Propagate pack errors onto the gzip stream so a single consumer can listen + packStream.on('error', (err) => gzip.destroy(err)); + if (onBytes) { + packStream.on('data', (chunk: Buffer) => onBytes(chunk.length)); + } + packStream.pipe(gzip); + }) + .catch((err) => gzip.destroy(err)); + return gzip; +} + +/** + * Total uncompressed size of the files `streamPackagedDirectory` would include with the same + * options. Used by the CLI to give the upload progress bar a realistic total. The uncompressed + * size won't equal the gzipped wire size, but it gives the bar a steady trajectory: the bar + * moves as bytes are sent and snaps to 100% when the upload finishes. + */ +export async function getPackagedDirectorySize( + directory: string, + options: PackageOptions = DEFAULT_OPTIONS +): Promise { + return (await scanPackageDirectory(directory, options)).totalSize; +} + +/** + * Relative paths of dangling symlinks that `streamPackagedDirectory` would skip. The CLI uses + * this to warn the user before deploy, since a dangling link means its intended content is + * absent from the package (and, before the skip guard, would have silently truncated the whole + * tarball). Returns `[]` when `skip_symlinks` is set, because links are then packed literally + * with no dereference. */ export async function findDanglingSymlinks( directory: string, options: PackageOptions = DEFAULT_OPTIONS ): Promise { - if (options.skip_symlinks) return []; - const found: string[] = []; - const walk = async (dir: string): Promise => { - let entries; - try { - entries = await readdir(dir, { withFileTypes: true }); - } catch { - return; // unreadable directory — skip - } - for (const entry of entries) { - const fullPath = join(dir, entry.name); - if (isExcluded(directory, fullPath, options)) continue; - if (entry.isSymbolicLink()) { - if (isDanglingSymlink(fullPath)) found.push(relative(directory, fullPath)); - } else if (entry.isDirectory()) { - await walk(fullPath); - } - } - }; - await walk(directory); - return found; + return (await scanPackageDirectory(directory, options)).danglingSymlinks; } /** diff --git a/unitTests/bin/cliOperations.test.js b/unitTests/bin/cliOperations.test.js index 1e76888ade..82c70e22da 100644 --- a/unitTests/bin/cliOperations.test.js +++ b/unitTests/bin/cliOperations.test.js @@ -102,18 +102,18 @@ describe('cliOperations', () => { describe('deploy_component cross-version compatibility', () => { const target = 'https://example.com:9925/'; let originalPackageDirectory; - let originalGetSize; + let originalScan; beforeEach(() => { saveCredentials(target, { operation_token: 'valid-token', refresh_token: 'refresh-token' }); tokenAuthModule.isJWTExpired = () => false; originalPackageDirectory = packageComponentModule.packageDirectory; - originalGetSize = packageComponentModule.getPackagedDirectorySize; + originalScan = packageComponentModule.scanPackageDirectory; }); afterEach(() => { packageComponentModule.packageDirectory = originalPackageDirectory; - packageComponentModule.getPackagedDirectorySize = originalGetSize; + packageComponentModule.scanPackageDirectory = originalScan; }); // Streams an SSE `done` event so the modern (>= 5.1) deploy path can read its result. @@ -155,7 +155,10 @@ describe('cliOperations', () => { it('downgrades a directory deploy to a CBOR binary payload when the target is < 5.1', async () => { const fakeTarball = Buffer.from('fake-tarball-bytes'); - packageComponentModule.getPackagedDirectorySize = async () => fakeTarball.length; + packageComponentModule.scanPackageDirectory = async () => ({ + totalSize: fakeTarball.length, + danglingSymlinks: [], + }); packageComponentModule.packageDirectory = async () => fakeTarball; const calls = []; diff --git a/unitTests/components/packageComponent.test.js b/unitTests/components/packageComponent.test.js index 6d5dedc1e5..615f39182c 100644 --- a/unitTests/components/packageComponent.test.js +++ b/unitTests/components/packageComponent.test.js @@ -184,13 +184,51 @@ describe('streamPackagedDirectory round-trip', () => { await fs.symlink('./gone', path.join(sourceDir, 'node_modules', 'broken-dep')); try { const dangling = (await findDanglingSymlinks(sourceDir, { skip_node_modules: true })).sort(); - assert.deepStrictEqual(dangling, ['broken-root', path.join('src', 'broken-nested')]); + // 'src/broken-nested' is reported twice — once directly, once via the 'good' alias + // symlink (which points at 'src') — since tar-fs's dereferenced walk would try to + // pack (and fail on) both as distinct archive entries. + assert.deepStrictEqual(dangling, [ + 'broken-root', + path.join('good', 'broken-nested'), + path.join('src', 'broken-nested'), + ]); // With skip_symlinks the archive packs links literally (no dereference), so nothing to warn. assert.deepStrictEqual(await findDanglingSymlinks(sourceDir, { skip_symlinks: true }), []); } finally { await fs.rm(sourceDir, { recursive: true, force: true }); } }); + + it('finds a dangling symlink nested inside a validly-linked directory, and packs the rest (regression)', async function () { + this.timeout(15000); + // tar-fs's dereferenced walk readdirs *through* a valid symlinked directory just like a + // real one, so a dangling link nested inside it is just as capable of truncating the + // archive as one at the top level. The scan must recurse into valid symlinked + // directories to catch this, or packaging will still hit the original bug. + const sourceDir = await makeFixture({ 'real/nested/deep.txt': 'deep\n', 'real/sibling.txt': 'sib\n' }); + await fs.symlink('./gone', path.join(sourceDir, 'real', 'nested', 'broken')); + await fs.symlink(path.join(sourceDir, 'real'), path.join(sourceDir, 'link-to-real')); + const extractDir = await fs.mkdtemp(path.join(os.tmpdir(), 'pkg-nested-dangling-out-')); + try { + const dangling = (await findDanglingSymlinks(sourceDir)).sort(); + // Reachable — and reported — via both the real path and the symlink alias, matching + // the two distinct archive entries tar-fs would otherwise try (and fail) to stat. + assert.deepStrictEqual(dangling, [ + path.join('link-to-real', 'nested', 'broken'), + path.join('real', 'nested', 'broken'), + ]); + + await pipeline(streamPackagedDirectory(sourceDir), gunzip(), tar.extract(extractDir)); + const extracted = await readDirTree(extractDir); + assert.strictEqual(extracted['real/nested/deep.txt'], 'deep\n'); + assert.strictEqual(extracted['real/sibling.txt'], 'sib\n'); + assert.strictEqual(extracted['link-to-real/nested/deep.txt'], 'deep\n'); + assert.strictEqual(extracted['link-to-real/sibling.txt'], 'sib\n'); + } finally { + await fs.rm(sourceDir, { recursive: true, force: true }); + await fs.rm(extractDir, { recursive: true, force: true }); + } + }); }); // keep eslint happy in case Readable isn't directly used in some branch From c1d30843b753165bac97251387e1fc5b64acefc0 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 8 Jul 2026 14:03:03 -0600 Subject: [PATCH 3/3] Use pipeline() for tar->gzip so an aborted upload stops the walk Addresses Gemini PR review feedback: the manual packStream.pipe(gzip) didn't propagate destruction in either direction. If the client aborted while the dangling-symlink prescan was still resolving, we'd still kick off a pointless directory walk; if it aborted after the walk started, packStream kept running with nowhere for its output to go. pipeline() destroys both streams on either side erroring or closing early, and the destroyed-gzip check skips starting the walk at all for the first case. Co-Authored-By: Claude Sonnet 5 --- components/packageComponent.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/components/packageComponent.ts b/components/packageComponent.ts index 61e2de6420..2501d12cc5 100644 --- a/components/packageComponent.ts +++ b/components/packageComponent.ts @@ -1,6 +1,6 @@ import { join, relative, sep } from 'node:path'; import { stat, readdir } from 'node:fs/promises'; -import { Readable } from 'node:stream'; +import { Readable, pipeline } from 'node:stream'; import tar from 'tar-fs'; import { createGzip } from 'node:zlib'; @@ -129,6 +129,9 @@ export function streamPackagedDirectory( : scanPackageDirectory(directory, options).then((scan) => scan.danglingSymlinks); dangling .then((danglingSymlinks) => { + // The consumer may have already aborted (destroying gzip) while the prescan was + // still resolving — skip starting a directory walk nothing will read. + if (gzip.destroyed) return; const danglingSet = new Set(danglingSymlinks); const packStream = tar.pack(directory, { dereference, @@ -141,12 +144,14 @@ export function streamPackagedDirectory( return header; }, }); - // Propagate pack errors onto the gzip stream so a single consumer can listen - packStream.on('error', (err) => gzip.destroy(err)); if (onBytes) { packStream.on('data', (chunk: Buffer) => onBytes(chunk.length)); } - packStream.pipe(gzip); + // pipeline (rather than a bare .pipe()) destroys both streams on either side + // erroring or the consumer aborting mid-stream, so an aborted upload doesn't leave + // the directory walk running in the background. The callback is a no-op: pipeline + // already surfaces the failure by destroying `gzip` with the error itself. + pipeline(packStream, gzip, () => {}); }) .catch((err) => gzip.destroy(err)); return gzip;