perf(pipeline): use streaming io.Copy for content passthrough file copying#606
Conversation
…pying Replace os.ReadFile/os.WriteFile with fileutil.CopyFile for content-colocated passthrough files. Avoids holding large binary assets entirely in memory during build. (issue #586) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR reduces memory usage during builds by switching content-colocated passthrough file copying (non-content files found under content/) from whole-file read/write to a streaming copy helper already used elsewhere in the pipeline.
Changes:
- Replace
os.ReadFile/os.WriteFilepassthrough copying withfileutil.CopyFilein the build pipeline. - Add
internal/fileutilimport to support the new streaming copy path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Review — 5 reviewers, 0 blocking findingsVerdict: Approve Correctly replaces inline Advisory findings (no merge blockers)
All findings are advisory. #1-#3 are pre-existing on Clean passes
Reviewed by: correctness, maintainability, performance, testing, project-standards |
zeroedin
left a comment
There was a problem hiding this comment.
Review #1 — 6 reviewers dispatched, 6 returned
Verdict: Ready to merge
Clean -8 net line reduction. Replaces a manual 10-line os.ReadFile/os.WriteFile pattern with the project's existing fileutil.CopyFile streaming utility. Eliminates heap allocation for large binary passthrough files. 0 actionable findings after synthesis.
Scope Note
3 of 6 reviewers explored beyond the PR diff (reading files on the branch not changed by this PR). 10 out-of-scope findings discarded. GitHub confirms 1 file changed (+2/-10).
Findings
0 actionable findings.
1 advisory (P2): Branch uses spec/ prefix (spec/issue-586-streaming-passthrough) but this is a perf implementation PR, not a specification PR.
Residual Risks
- Permission semantics change — Output passthrough files now inherit source file permissions instead of hardcoded
0644. This aligns with howstatic.CopyPassthroughWithValidationandassets.CopyAssetsalready work via the sameCopyFile— making behavior more consistent across copy paths. PR description doesn't mention this change.
Testing Gaps (advisory)
- Permission preservation untested — No test distinguishes
0644from source-preserving behavior. Existing passthrough test (build_test.go:1069) creates source files with0644, making the change invisible.
Pre-Existing Issues (not introduced by this PR)
fileutil.CopyFilehas no unit tests (5 error branches, used by 3 copy paths)- Sequential copy loop (static path already uses
copyDirConcurrentwithruntime.NumCPU()workers) - No
fsyncbefore close inCopyFile - Truncated destination on error (no atomic write pattern)
Correctness Verification
CopyFilealready battle-tested by static and assets copy paths on main- Handles MkdirAll, streaming
io.Copy, source permission preservation, deferred close with error propagation - Error wrapping correct —
relPathnow included in all error messages (improvement over old MkdirAll message) - Loop structure, path computation, and
reportEndStage()on error all preserved
6 reviewers (correctness, testing, maintainability, project-standards, performance, reliability) · 10 findings discarded (out-of-scope) · 4 separated (pre-existing) · 2 demoted (mode-aware) · 0 suppressed (confidence gate)
Summary
os.ReadFile/os.WriteFilewithfileutil.CopyFilefor content-colocated passthrough filesfileutil.CopyFilealready handlesMkdirAll, streamingio.Copywith 32KB buffer, source permission preservation, and deferred close with error propagationVerification
Existing test at
build_test.go:1069("build succeeds with passthrough mappings") exercises this path with real file I/O viaBuildWithContent. All 144 passing tests remain green; the 1 pre-existing failure is issue #581 (unrelated).Closes #586
🤖 Generated with Claude Code