fix: don't buffer functions and don't keep them all in memory for duration of hashing and uploading - #649
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:
📝 WalkthroughWalkthroughThe 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 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
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)
b263961 to
4acfab0
Compare
pieh
changed the title
… no longer used to maintain backward compat
davbree
left a comment
davbree
left a comment
There was a problem hiding this comment.
Claude flagged the following:
go/porcelain/deploy.go:509-516 (uploadFiles) — cancelling a deploy mid-upload orphans goroutines that race against temp-dir cleanup. The ctx.Done() branch returns immediately without calling wg.Wait(), so in-flight uploadFile goroutines keep running after uploadFiles returns. That error propagates through DoDeploy, which then runs defer functionsTmpDir.remove() (os.RemoveAll on the whole temp dir) while orphaned goroutines may still be calling os.Open(f.Path) against files inside it. Confirmed real and new (pre-PR, orphaned goroutines only touched in-memory buffers or the user's own source tree, never a directory buildbot was actively deleting). Severity is moderate — bounded by the 2-minute backoff ceiling, no data corruption, but a cancelled/timed-out deploy with many in-flight function uploads will now surface confusing "no such file or directory" errors for up to 2 minutes instead of failing cleanly.
… finish before returning
🤖 I have created a release *beep* *boop* --- ## [2.56.1](v2.56.0...v2.56.1) (2026-07-06) ### Bug Fixes * don't buffer functions and don't keep them all in memory for duration of hashing and uploading ([#649](#649)) ([969c878](969c878)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: token-generator-app[bot] <82042599+token-generator-app[bot]@users.noreply.github.com> Co-authored-by: token-generator-app[bot] <token-generator-app[bot]@users.noreply.github.com>
2 participants
Possible fix for https://linear.app/netlify/issue/FRB-2285/upwell-or-builds-hanging-during-function-uploads