fix: 2 improvements across 2 files#13481
Conversation
- Quality: Inefficient synchronous zip file creation - Quality: Fragile file extension detection Signed-off-by: Tang Vu <145498528+tang-vu@users.noreply.github.com>
- Quality: Inefficient synchronous zip file creation - Quality: Fragile file extension detection Signed-off-by: Tang Vu <145498528+tang-vu@users.noreply.github.com>
|
All contributors have signed the CLA ✍️ ✅ |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughThe serverless package utilities are refactored to enhance performance and precision: file zip creation transitions from synchronous to asynchronous file-stat operations with Promise.all handling, while file type detection logic shifts from substring matching to strict suffix matching for JSON and YAML files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/serverless/lib/utils/fs/write-file-sync.js (1)
19-20: Consider clearer boolean names for extension checks.On Line 19 and Line 20, these names imply filesystem existence checks.
isYamlFile/isYmlFilewould better match what the code does.Proposed rename
- const yamlFileExists = filePath.endsWith('.yaml') - const ymlFileExists = filePath.endsWith('.yml') + const isYamlFile = filePath.endsWith('.yaml') + const isYmlFile = filePath.endsWith('.yml') - if ((yamlFileExists || ymlFileExists) && typeof contents !== 'string') { + if ((isYamlFile || isYmlFile) && typeof contents !== 'string') { contents = yaml.dump(contents) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/serverless/lib/utils/fs/write-file-sync.js` around lines 19 - 20, Rename the misleading boolean variables yamlFileExists and ymlFileExists to more accurate names (e.g., isYamlFile and isYmlFile) in write-file-sync.js and update every usage site in the same module (keep the same .endsWith('.yaml') and .endsWith('.yml') checks and any downstream logic that references these variables such as conditional branches or return values) so the intent is clear that these flags indicate file extension checks rather than filesystem existence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/serverless/lib/utils/fs/create-zip-file.js`:
- Around line 22-31: The zip entry order is non-deterministic because
archive.append() runs as each lstat resolves; change the logic in
create-zip-file.js so you first map files to an array of stat results (e.g.,
await Promise.all(files.map(async file => ({file, stats: await
fs.promises.lstat(file.input)})))), then iterate the original files array order
and for each item check the previously collected stats and call
archive.append(fs.createReadStream(file.input), { name: file.output }) only if
stats.isFile(); keep references to the existing variables (files, archive,
fs.promises.lstat, file.input, file.output) so ordering is preserved and append
happens deterministically.
- Around line 33-34: The Promise rejection path in create-zip-file.js leaves the
piped archive and output streams open when an error occurs (archive and output
from archive.pipe(output)), causing truncated artifacts and leaked file
descriptors; modify the catch block handling the Promise.all failure to close
and/or destroy both the archive and output streams (call archive.abort() or
archive.finalize()/archive.destroy() as appropriate and output.end() or
output.destroy()) before calling reject(err) to ensure the pipeline is torn down
and resources are released. Ensure you reference the same archive and output
variables used in the function so the cleanup always runs on error.
---
Nitpick comments:
In `@packages/serverless/lib/utils/fs/write-file-sync.js`:
- Around line 19-20: Rename the misleading boolean variables yamlFileExists and
ymlFileExists to more accurate names (e.g., isYamlFile and isYmlFile) in
write-file-sync.js and update every usage site in the same module (keep the same
.endsWith('.yaml') and .endsWith('.yml') checks and any downstream logic that
references these variables such as conditional branches or return values) so the
intent is clear that these flags indicate file extension checks rather than
filesystem existence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41d58c6d-4526-4a1f-b7b5-4d4d3e56f436
📒 Files selected for processing (2)
packages/serverless/lib/utils/fs/create-zip-file.jspackages/serverless/lib/utils/fs/write-file-sync.js
| await Promise.all( | ||
| files.map(async (file) => { | ||
| const stats = await fs.promises.lstat(file.input) | ||
| if (stats.isFile()) { | ||
| archive.append(fs.createReadStream(file.input), { | ||
| name: file.output, | ||
| }) | ||
| } | ||
| }) | ||
| } | ||
| }) | ||
|
|
||
| archive.finalize() | ||
| ) |
There was a problem hiding this comment.
Keep the zip entry order deterministic.
archive.append() now runs in lstat() completion order, not in files order. That can make identical inputs produce different zip entry ordering and different archive bytes across runs. Gather the async stat results first, then append in the original array order.
♻️ Proposed fix
- await Promise.all(
- files.map(async (file) => {
- const stats = await fs.promises.lstat(file.input)
- if (stats.isFile()) {
- archive.append(fs.createReadStream(file.input), {
- name: file.output,
- })
- }
- })
- )
+ const fileEntries = await Promise.all(
+ files.map(async (file) => {
+ const stats = await fs.promises.lstat(file.input)
+ return stats.isFile() ? file : null
+ })
+ )
+
+ for (const file of fileEntries) {
+ if (!file) continue
+ archive.append(fs.createReadStream(file.input), {
+ name: file.output,
+ })
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/serverless/lib/utils/fs/create-zip-file.js` around lines 22 - 31,
The zip entry order is non-deterministic because archive.append() runs as each
lstat resolves; change the logic in create-zip-file.js so you first map files to
an array of stat results (e.g., await Promise.all(files.map(async file =>
({file, stats: await fs.promises.lstat(file.input)})))), then iterate the
original files array order and for each item check the previously collected
stats and call archive.append(fs.createReadStream(file.input), { name:
file.output }) only if stats.isFile(); keep references to the existing variables
(files, archive, fs.promises.lstat, file.input, file.output) so ordering is
preserved and append happens deterministically.
| } catch (err) { | ||
| reject(err) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "create-zip-file.js" -type fRepository: serverless/serverless
Length of output: 118
🏁 Script executed:
cat -n ./packages/serverless/lib/utils/fs/create-zip-file.jsRepository: serverless/serverless
Length of output: 1474
🏁 Script executed:
rg "archive\.destroy|output\.destroy" --type js --type tsRepository: serverless/serverless
Length of output: 47
🏁 Script executed:
cd packages/serverless && npm list archiver 2>/dev/null | head -20Repository: serverless/serverless
Length of output: 118
🏁 Script executed:
rg "archiver" package.jsonRepository: serverless/serverless
Length of output: 47
🏁 Script executed:
find . -name "package.json" -type f | head -10 | xargs grep -l "archiver"Repository: serverless/serverless
Length of output: 49
🏁 Script executed:
cat ./package.json | head -50Repository: serverless/serverless
Length of output: 936
🏁 Script executed:
cat ./packages/serverless/package.json | head -80Repository: serverless/serverless
Length of output: 2566
Close the streams on the error path to prevent file corruption and resource leaks.
At this point archive.pipe(output) has already started (line 19). If any operation in the Promise.all (lines 22-31) fails, rejecting alone leaves the archive and output streams in an incomplete piped state, resulting in a truncated artifact file and leaking file descriptors.
Proposed fix
} catch (err) {
+ archive.destroy()
+ output.destroy()
reject(err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (err) { | |
| reject(err) | |
| } catch (err) { | |
| archive.destroy() | |
| output.destroy() | |
| reject(err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/serverless/lib/utils/fs/create-zip-file.js` around lines 33 - 34,
The Promise rejection path in create-zip-file.js leaves the piped archive and
output streams open when an error occurs (archive and output from
archive.pipe(output)), causing truncated artifacts and leaked file descriptors;
modify the catch block handling the Promise.all failure to close and/or destroy
both the archive and output streams (call archive.abort() or
archive.finalize()/archive.destroy() as appropriate and output.end() or
output.destroy()) before calling reject(err) to ensure the pipeline is torn down
and resources are released. Ensure you reference the same archive and output
variables used in the function so the cleanup always runs on error.
|
I have read the CLA Document and I hereby sign the CLA |
|
Closing - the create-zip-file.js change introduces non-deterministic file ordering in zip artifacts. Promise.all resolves lstat calls in arbitrary order, so archive.append() is called in whatever order I/O completes rather than the original array order. This means identical source code can produce different zip hashes across runs, which can trigger unnecessary redeployments. The actual performance bottleneck (zlib level 9 compression) isn't addressed by making lstat async. The write-file-sync.js endsWith change is valid but doesn't warrant merging alongside the other change. Thank you for the contribution. |
Summary
fix: 2 improvements across 2 files
Problem
Severity:
Medium| File:packages/serverless/lib/utils/fs/create-zip-file.js:L14The
createZipFilefunction usesfs.lstatSyncinside a loop and processes files sequentially. A TODO comment explicitly acknowledges that this implementation is 'REALLY slow'. This can significantly impact deployment times for services with many files.Solution
Refactor the loop to use asynchronous
fs.promises.lstatand consider using a concurrency-limited parallel approach to append files to the archive.Changes
packages/serverless/lib/utils/fs/create-zip-file.js(modified)packages/serverless/lib/utils/fs/write-file-sync.js(modified)Testing
Generated by ContribAI v5.7.1
Summary by CodeRabbit
Release Notes
Bug Fixes
Performance Improvements