Skip to content

Comments

fix: pass explicit ContentLength for S3 uploads to handle symlinks#13317

Merged
czubocha merged 2 commits intomainfrom
sc-3776
Feb 4, 2026
Merged

fix: pass explicit ContentLength for S3 uploads to handle symlinks#13317
czubocha merged 2 commits intomainfrom
sc-3776

Conversation

@czubocha
Copy link
Contributor

@czubocha czubocha commented Feb 4, 2026 •
edited by cursor bot
Loading

Summary

  • Fix S3 multipart upload failure when uploading symlinked files with AWS SDK v3
  • Add explicit ContentLength parameter using fs.statSync() which follows symlinks
  • Resolves "Expected N part(s) but uploaded M part(s)" error caused by lstatSync returning symlink size instead of target file size

Root cause

AWS SDK v3's @aws-sdk/lib-storage uses lstatSync to determine file size for multipart uploads. When the serverless-python-requirements plugin creates a symlink to cached pythonRequirements.zip, lstatSync returns the symlink size (~126 bytes) instead of the actual file size (~40MB), causing a mismatch between expected and actual upload parts.

Test plan

  • Verify statSync and lstatSync return same size for regular files (no regression)
  • Verify symlink upload works with explicit ContentLength
  • Run simple-nodejs integration test
  • Manual test with reproduction (pythonRequirements layer with static cache)

Related

Summary by CodeRabbit

  • Bug Fixes
    • Ensure accurate file-size calculation for uploads and explicitly include Content-Length when sending files to S3.
    • Resolve mismatches caused by symlinked files so package uploads during serverless deployments are consistent and reliable.

Note

Medium Risk
Moderate risk because it changes S3 upload request parameters in the deploy path; incorrect size calculation could cause upload failures or unexpected multipart behavior, especially for large artifacts and symlinks.

Overview
Ensures deployment zip uploads to S3 explicitly include ContentLength, computed from the bytes read via fs.readFileSync, to avoid AWS SDK v3 multipart upload part-count mismatches when the artifact path is a symlink.

Adds inline documentation explaining the symlink/lstat behavior and why ContentLength must be provided.

Written by Cursor Bugbot for commit 8037211. This will update automatically on new commits. Configure here.

@Mmarzex
Copy link
Contributor

Mmarzex commented Feb 4, 2026 •
edited
Loading

✅ Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
✅ Open Source Security 0 0 0 0 0 issues
✅ Licenses 0 0 0 0 0 issues
✅ Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 4, 2026 •
edited
Loading

📝 Walkthrough

Walkthrough

This change adds file size calculation using fs.statSync() to the S3 upload-zip-file plugin, then passes it as the ContentLength parameter. The use of statSync follows symlinks, addressing discrepancies with AWS SDK v3 lib-storage that relied on lstatSync.

Changes

Cohort / File(s) Summary
S3 Upload Parameters
packages/serverless/lib/plugins/aws/lib/upload-zip-file.js		 Added file size calculation via fs.statSync() and included ContentLength parameter in S3 upload configuration to ensure proper file size reporting.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested reviewers

  • eahefnawy

Poem

🐰 A zippy fix hops into place,
File sizes now measured with grace,
Symlinks followed, no more confusion,
ContentLength ends the illusion,
S3 uploads, statSync's embrace! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: passing explicit ContentLength for S3 uploads to handle symlinks, which directly matches the core fix in the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch sc-3776

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@czubocha
Copy link
Contributor Author

czubocha commented Feb 4, 2026

@cursor review

cursor[bot]
cursor bot reviewed Feb 4, 2026
View reviewed changes
cursor[bot]
cursor bot reviewed Feb 4, 2026
View reviewed changes
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

@czubocha czubocha requested a review from eahefnawy February 4, 2026 10:46
@czubocha czubocha merged commit d08834e into main Feb 4, 2026
13 checks passed
@czubocha czubocha deleted the sc-3776 branch February 4, 2026 19:56
@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@cursor cursor[bot] cursor[bot] left review comments

@eahefnawy eahefnawy eahefnawy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@czubocha @Mmarzex @eahefnawy