fix: layer publish support code is http url,model remove#144
Merged
rsonghuster merged 1 commit intomasterfrom Mar 9, 2026
Merged
fix: layer publish support code is http url,model remove#144rsonghuster merged 1 commit intomasterfrom
rsonghuster merged 1 commit intomasterfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR centralizes URL download functionality by extracting it into a shared utility function, integrates the utility into layer and function deployment workflows, adds test mocks for the downloads module, and implements cleanup logic for temporary downloaded files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/subCommands/model/model.ts (1)
120-126:⚠️ Potential issue | 🟡 MinorInconsistent null guard for
nasMountPoints.Line 120 adds a guard
nasMountPoints &&before accessingnasMountPoints[0], but line 122 still accessesnasMountPoints[0]directly. IfnasMountPointsisundefinedornull, line 122 will throw aTypeError.Consider applying the same guard consistently:
🛡️ Proposed fix
- if (storage === 'nas' && nasMountPoints[0] && nasPath?.trim() === '/') { + if (storage === 'nas' && nasMountPoints && nasMountPoints[0] && nasPath?.trim() === '/') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/model/model.ts` around lines 120 - 126, The null-guard for nasMountPoints is inconsistent: you compute nasPath using nasMountPoints && nasMountPoints[0] but then access nasMountPoints[0] again in the if condition which can throw when nasMountPoints is null/undefined; update the check so both uses are guarded (e.g. use nasMountPoints?.[0] or a local const like const firstNas = nasMountPoints?.[0] and then check storage === 'nas' && firstNas && nasPath?.trim() === '/') to avoid a TypeError when nasMountPoints is missing.
🧹 Nitpick comments (1)
src/subCommands/layer/index.ts (1)
58-65: Consider extracting duplicated cleanup logic.The cleanup code for
downloadedTempFileis duplicated at lines 58-65 and 90-97. Consider extracting it to a small helper function for maintainability.♻️ Optional refactor to reduce duplication
+function cleanupTempFile(filePath: string): void { + if (!filePath) return; + try { + logger.debug(`Removing temp download file: ${filePath}`); + fs.rmSync(filePath, { recursive: true, force: true }); + } catch (ex) { + logger.debug(`Unable to remove temp download file: ${filePath}`); + } +} + export default class Layer { static async safe_publish_layer( // ... if (latestLayer.codeChecksum === crc64Value) { // ... - if (downloadedTempFile) { - try { - logger.debug(`Removing temp download dir: ${downloadedTempFile}`); - fs.rmSync(downloadedTempFile, { recursive: true, force: true }); - } catch (ex) { - logger.debug(`Unable to remove temp download dir: ${downloadedTempFile}`); - } - } + cleanupTempFile(downloadedTempFile); return latestLayer; } } // ... later ... - if (downloadedTempFile) { - try { - logger.debug(`Removing temp download file: ${downloadedTempFile}`); - fs.rmSync(downloadedTempFile, { recursive: true, force: true }); - } catch (ex) { - logger.debug(`Unable to remove temp download file: ${downloadedTempFile}`); - } - } + cleanupTempFile(downloadedTempFile);Also applies to: 89-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/layer/index.ts` around lines 58 - 65, Extract the duplicated cleanup block that removes downloadedTempFile into a small helper function (e.g., removeTempDownload or cleanupTempDir) and call it from both locations; the helper should accept the path (downloadedTempFile), call fs.rmSync(path, { recursive: true, force: true }) inside a try/catch, and use logger.debug to log both the start ("Removing temp download dir: ...") and the failure ("Unable to remove temp download dir: ...") messages so the two duplicated blocks around downloadedTempFile are replaced by a single reusable function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/index.ts`:
- Around line 329-344: The returned downloadPath points to a different filename
than the one passed to downloads(): you compute both filename
(path.basename(urlPath)) and parsedPathName (path.parse(urlPath).name) but use
filename for downloadPath and parsedPathName for downloads(); fix by choosing a
single canonical name and using it in both places — e.g., compute a single
variable (use either basename or parsedName+path.extname(urlPath)) and replace
both uses of filename and parsedPathName so downloadPath and the downloads(...)
filename option reference the exact same string (ensure tempDir and URL parsing
logic remain unchanged).
- Around line 345-357: The cleanup currently removes the entire shared tempDir
(tempDir) which can disrupt concurrent operations; instead, in the catch block
only remove the specific downloaded file/temporary artifact (e.g., the variable
that holds the downloaded file path such as tempFilePath or
downloadedArchivePath) by checking fs.existsSync(tempFilePath) and calling
fs.unlinkSync(tempFilePath) (or async equivalent), leaving tempDir intact;
update the logger.debug messages to reference the specific file and preserve the
existing cleanupError logging behavior (logger.debug with cleanupError.message)
and then rethrow the original error as before.
---
Outside diff comments:
In `@src/subCommands/model/model.ts`:
- Around line 120-126: The null-guard for nasMountPoints is inconsistent: you
compute nasPath using nasMountPoints && nasMountPoints[0] but then access
nasMountPoints[0] again in the if condition which can throw when nasMountPoints
is null/undefined; update the check so both uses are guarded (e.g. use
nasMountPoints?.[0] or a local const like const firstNas = nasMountPoints?.[0]
and then check storage === 'nas' && firstNas && nasPath?.trim() === '/') to
avoid a TypeError when nasMountPoints is missing.
---
Nitpick comments:
In `@src/subCommands/layer/index.ts`:
- Around line 58-65: Extract the duplicated cleanup block that removes
downloadedTempFile into a small helper function (e.g., removeTempDownload or
cleanupTempDir) and call it from both locations; the helper should accept the
path (downloadedTempFile), call fs.rmSync(path, { recursive: true, force: true
}) inside a try/catch, and use logger.debug to log both the start ("Removing
temp download dir: ...") and the failure ("Unable to remove temp download dir:
...") messages so the two duplicated blocks around downloadedTempFile are
replaced by a single reusable function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7df18c9-6a61-45b6-94eb-f2a0a0d25200
📒 Files selected for processing (7)
__tests__/ut/commands/invoke_test.ts__tests__/ut/utils/utils_test.tspackage.jsonsrc/subCommands/deploy/impl/function.tssrc/subCommands/layer/index.tssrc/subCommands/model/model.tssrc/utils/index.ts
Comment on lines
+329
to
+344
| // 从URL获取文件名 | ||
| const urlPath = new URL(url).pathname; | ||
| const parsedPathName = path.parse(urlPath).name; | ||
| const filename = path.basename(urlPath) || `downloaded_code_${Date.now()}`; | ||
| downloadPath = path.join(tempDir, filename); | ||
|
|
||
| await downloads(url, { | ||
| dest: tempDir, | ||
| filename: parsedPathName, | ||
| extract: false, | ||
| }); | ||
|
|
||
| logger.debug(`Downloaded file to: ${downloadPath}`); | ||
|
|
||
| // 返回下载文件路径，由主流程决定是否需要压缩 | ||
| return downloadPath; |
There was a problem hiding this comment.
Critical: Mismatched filename causes returned path to reference a non-existent file.
The function computes two different filenames:
filename = path.basename(urlPath)→ e.g.,"code.zip"parsedPathName = path.parse(urlPath).name→ e.g.,"code"(without extension)
The downloadPath is computed using filename, but downloads() saves the file using parsedPathName. This causes the returned path to point to a file that doesn't exist.
For example, with URL https://example.com/code.zip:
- Returns:
/tmp/fc_code_download/code.zip - Actual file:
/tmp/fc_code_download/code
🐛 Proposed fix
// 从URL获取文件名
const urlPath = new URL(url).pathname;
- const parsedPathName = path.parse(urlPath).name;
const filename = path.basename(urlPath) || `downloaded_code_${Date.now()}`;
downloadPath = path.join(tempDir, filename);
await downloads(url, {
dest: tempDir,
- filename: parsedPathName,
+ filename: filename,
extract: false,
});📝 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.
Suggested change
| // 从URL获取文件名 | |
| const urlPath = new URL(url).pathname; | |
| const parsedPathName = path.parse(urlPath).name; | |
| const filename = path.basename(urlPath) || `downloaded_code_${Date.now()}`; | |
| downloadPath = path.join(tempDir, filename); | |
| await downloads(url, { | |
| dest: tempDir, | |
| filename: parsedPathName, | |
| extract: false, | |
| }); | |
| logger.debug(`Downloaded file to: ${downloadPath}`); | |
| // 返回下载文件路径，由主流程决定是否需要压缩 | |
| return downloadPath; | |
| // 从URL获取文件名 | |
| const urlPath = new URL(url).pathname; | |
| const filename = path.basename(urlPath) || `downloaded_code_${Date.now()}`; | |
| downloadPath = path.join(tempDir, filename); | |
| await downloads(url, { | |
| dest: tempDir, | |
| filename: filename, | |
| extract: false, | |
| }); | |
| logger.debug(`Downloaded file to: ${downloadPath}`); | |
| // 返回下载文件路径，由主流程决定是否需要压缩 | |
| return downloadPath; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/index.ts` around lines 329 - 344, The returned downloadPath points
to a different filename than the one passed to downloads(): you compute both
filename (path.basename(urlPath)) and parsedPathName (path.parse(urlPath).name)
but use filename for downloadPath and parsedPathName for downloads(); fix by
choosing a single canonical name and using it in both places — e.g., compute a
single variable (use either basename or parsedName+path.extname(urlPath)) and
replace both uses of filename and parsedPathName so downloadPath and the
downloads(...) filename option reference the exact same string (ensure tempDir
and URL parsing logic remain unchanged).
Comment on lines
+345
to
+357
| } catch (error) { | ||
| // 如果下载失败，清理临时目录 | ||
| try { | ||
| if (fs.existsSync(tempDir)) { | ||
| fs.rmSync(tempDir, { recursive: true, force: true }); | ||
| logger.debug(`Cleaned up temporary directory after error: ${tempDir}`); | ||
| } | ||
| } catch (cleanupError) { | ||
| logger.debug(`Failed to clean up temporary directory: ${cleanupError.message}`); | ||
| } | ||
|
|
||
| throw new Error(`Failed to download code from URL: ${error.message}`); | ||
| } |
There was a problem hiding this comment.
Error cleanup removes entire temp directory, potentially affecting concurrent operations.
On error, the function removes the entire fc_code_download directory. Since this is a shared temp directory path (os.tmpdir()/fc_code_download), concurrent downloads from different processes or calls could have their files deleted unexpectedly.
Consider cleaning up only the specific downloaded file rather than the entire directory:
🛡️ Proposed fix
} catch (error) {
- // 如果下载失败，清理临时目录
+ // 如果下载失败，清理下载的文件
try {
- if (fs.existsSync(tempDir)) {
- fs.rmSync(tempDir, { recursive: true, force: true });
- logger.debug(`Cleaned up temporary directory after error: ${tempDir}`);
+ if (downloadPath && fs.existsSync(downloadPath)) {
+ fs.rmSync(downloadPath, { force: true });
+ logger.debug(`Cleaned up downloaded file after error: ${downloadPath}`);
}
} catch (cleanupError) {
- logger.debug(`Failed to clean up temporary directory: ${cleanupError.message}`);
+ logger.debug(`Failed to clean up downloaded file: ${cleanupError.message}`);
}
throw new Error(`Failed to download code from URL: ${error.message}`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/index.ts` around lines 345 - 357, The cleanup currently removes the
entire shared tempDir (tempDir) which can disrupt concurrent operations;
instead, in the catch block only remove the specific downloaded file/temporary
artifact (e.g., the variable that holds the downloaded file path such as
tempFilePath or downloadedArchivePath) by checking fs.existsSync(tempFilePath)
and calling fs.unlinkSync(tempFilePath) (or async equivalent), leaving tempDir
intact; update the logger.debug messages to reference the specific file and
preserve the existing cleanupError logging behavior (logger.debug with
cleanupError.message) and then rethrow the original error as before.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/subCommands/layer/index.ts (1)
58-66: Log message inconsistency: says "dir" but should say "file".Lines 90-97 correctly use "file" in the log messages, but this block uses "dir". Align both cleanup blocks for consistency.
📝 Suggested fix
if (downloadedTempFile) { try { - logger.debug(`Removing temp download dir: ${downloadedTempFile}`); + logger.debug(`Removing temp download file: ${downloadedTempFile}`); fs.rmSync(downloadedTempFile, { recursive: true, force: true }); } catch (ex) { - logger.debug(`Unable to remove temp download dir: ${downloadedTempFile}`); + logger.debug(`Unable to remove temp download file: ${downloadedTempFile}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/layer/index.ts` around lines 58 - 66, The cleanup logs for the temporary download use "dir" incorrectly; update the logger.debug messages in the block that checks downloadedTempFile (around the fs.rmSync call) to say "file" instead of "dir" so they match the other cleanup block and the actual variable downloadedTempFile; adjust both the success message (`Removing temp download ...`) and the catch message (`Unable to remove temp download ...`) in src/subCommands/layer/index.ts where downloadedTempFile, fs.rmSync and latestLayer are used.src/subCommands/deploy/impl/function.ts (2)
328-335: Log message says "dir" but cleanup targets a file.The variable
downloadedTempFileholds a file path (per the_downloadFromUrlutility), but log messages reference "temp download dir". This is misleading during debugging.📝 Suggested fix
if (downloadedTempFile) { try { - logger.debug(`Removing temp download dir: ${downloadedTempFile}`); + logger.debug(`Removing temp download file: ${downloadedTempFile}`); fs.rmSync(downloadedTempFile, { recursive: true, force: true }); } catch (ex) { - logger.debug(`Unable to remove temp download dir: ${downloadedTempFile}`); + logger.debug(`Unable to remove temp download file: ${downloadedTempFile}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` around lines 328 - 335, The cleanup log incorrectly calls the path in downloadedTempFile a "dir" even though _downloadFromUrl returns a file path; update the messages in the cleanup block that references downloadedTempFile to say "file" (e.g., "Removing temp download file" and "Unable to remove temp download file") so logs accurately reflect what's being removed; locate the cleanup code that checks downloadedTempFile in function.ts and change both logger.debug strings accordingly.
353-360: Same log message inconsistency as the early return path.Apply the same fix here: change "dir" to "file" in the log messages for consistency.
📝 Suggested fix
if (downloadedTempFile) { try { - logger.debug(`Removing temp download dir: ${downloadedTempFile}`); + logger.debug(`Removing temp download file: ${downloadedTempFile}`); fs.rmSync(downloadedTempFile, { recursive: true, force: true }); } catch (ex) { - logger.debug(`Unable to remove temp download dir: ${downloadedTempFile}`); + logger.debug(`Unable to remove temp download file: ${downloadedTempFile}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` around lines 353 - 360, The cleanup block that removes downloadedTempFile uses logger.debug messages that say "dir" but should say "file" for consistency; update the two logger.debug calls in the block that references downloadedTempFile (the try/catch around fs.rmSync) to use "file" instead of "dir" so both the success and error logs read "Removing temp download file: ${downloadedTempFile}" and "Unable to remove temp download file: ${downloadedTempFile}" respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/subCommands/deploy/impl/function.ts`:
- Around line 328-335: The cleanup log incorrectly calls the path in
downloadedTempFile a "dir" even though _downloadFromUrl returns a file path;
update the messages in the cleanup block that references downloadedTempFile to
say "file" (e.g., "Removing temp download file" and "Unable to remove temp
download file") so logs accurately reflect what's being removed; locate the
cleanup code that checks downloadedTempFile in function.ts and change both
logger.debug strings accordingly.
- Around line 353-360: The cleanup block that removes downloadedTempFile uses
logger.debug messages that say "dir" but should say "file" for consistency;
update the two logger.debug calls in the block that references
downloadedTempFile (the try/catch around fs.rmSync) to use "file" instead of
"dir" so both the success and error logs read "Removing temp download file:
${downloadedTempFile}" and "Unable to remove temp download file:
${downloadedTempFile}" respectively.
In `@src/subCommands/layer/index.ts`:
- Around line 58-66: The cleanup logs for the temporary download use "dir"
incorrectly; update the logger.debug messages in the block that checks
downloadedTempFile (around the fs.rmSync call) to say "file" instead of "dir" so
they match the other cleanup block and the actual variable downloadedTempFile;
adjust both the success message (`Removing temp download ...`) and the catch
message (`Unable to remove temp download ...`) in src/subCommands/layer/index.ts
where downloadedTempFile, fs.rmSync and latestLayer are used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0877b626-2df7-4de8-b1dd-f58a3e5aa83c
📒 Files selected for processing (5)
__tests__/ut/commands/invoke_test.ts__tests__/ut/utils/utils_test.tssrc/subCommands/deploy/impl/function.tssrc/subCommands/layer/index.tssrc/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/utils/index.ts
- tests/ut/utils/utils_test.ts
- tests/ut/commands/invoke_test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Tests