fix: don't buffer functions and don't keep them all in memory for duration of hashing and uploading#649
fix: don't buffer functions and don't keep them all in memory for duration of hashing and uploading#649pieh wants to merge 4 commits into
Conversation
✅ Deploy Preview for open-api ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe deploy bundling flow now writes function artifacts to temporary files on disk instead of relying on in-memory buffers. Estimated code review effort: 4 (Complex) | ~40 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…of hashing and uploading
pieh
force-pushed
the
fix/dont-buffer-functions
branch
from
feac31a to
3c653ec
Compare
pieh
changed the title
![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
🧹 Nitpick comments (1)
go/porcelain/deploy.go (1)
129-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the
FileBundlecontract unambiguous.Line 129 still says
Path OR Buffercan be populated, but Lines 132-133 state uploads only readPath. Please update the comment so external callers don’t rely onBufferfor uploads.Suggested docs tweak
- // Path OR Buffer should be populated + // Path should be populated for uploads. Path string // Deprecated: uploads always stream from Path; this package no longer reads Buffer. It is retained🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go/porcelain/deploy.go` around lines 129 - 133, Update the FileBundle documentation in deploy.go so the contract is explicit that uploads use Path only; the current comment near FileBundle still suggests “Path OR Buffer” is valid, which conflicts with the deprecated Buffer note. Adjust the comment on FileBundle (and its Path/Buffer fields) to state that Buffer is retained only for backward compatibility and is not read by upload flow, so callers should populate Path for uploads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go/porcelain/deploy.go`:
- Around line 270-276: The temp directory cleanup in deploy.go is ignoring the
result of os.RemoveAll, which golangci-lint flags and can hide cleanup failures.
Update the deferred cleanup around functionsTmpDir in the deploy flow to
explicitly handle and surface the RemoveAll error instead of discarding it,
using the existing bundle/deploy context to locate the cleanup path.
- Around line 966-990: The temp ZIP file in the bundle निर्माण flow is returned
before its close result is verified, so delayed write errors can be missed. In
the function that creates the archive and returns FileBundle, make sure the
temporary file close is checked before constructing the result, and propagate
any close error alongside the existing archive/IO errors. Keep the
hash/path/name logic in place, but only return the bundle after a successful
tmp.Close().
---
Nitpick comments:
In `@go/porcelain/deploy.go`:
- Around line 129-133: Update the FileBundle documentation in deploy.go so the
contract is explicit that uploads use Path only; the current comment near
FileBundle still suggests “Path OR Buffer” is valid, which conflicts with the
deprecated Buffer note. Adjust the comment on FileBundle (and its Path/Buffer
fields) to state that Buffer is retained only for backward compatibility and is
not read by upload flow, so callers should populate Path for uploads.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd3bfee8-0e21-4f4e-82e9-87c37138a146
📒 Files selected for processing (2)
go/porcelain/deploy.gogo/porcelain/deploy_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
netlify/blueprints(manual)
pieh
force-pushed
the
fix/dont-buffer-functions
branch
from
b263961 to
4acfab0
Compare
pieh
changed the title
… no longer used to maintain backward compat
1 participant
Possible fix for https://linear.app/netlify/issue/FRB-2285/upwell-or-builds-hanging-during-function-uploads