`fileutil.CopyFile` calls `out.Close()` twice on the error path and may swallow Close errors on success

[github-actions]

Summary

pkg/fileutil/fileutil.go:120-147 has two related lifecycle issues:

1. Double-close on the error path. Line 134 installs defer func() { _ = out.Close() }(), and lines 137-139 also call out.Close() explicitly inside the io.Copy error branch. The second close always fails (os.ErrClosed) when the deferred close runs afterward — currently the _ = out.Close() discards it, but the explicit close at line 137 logs the error, producing misleading log output ("Failed to close destination file during cleanup: file already closed") even on a clean error path.

2. Silent loss of out.Close() errors on the happy path. The function ends with return out.Sync() (line 146). The deferred _ = out.Close() runs after the return value is captured, meaning a Close() failure (e.g. delayed write error reported only at close time on networked filesystems) is silently swallowed. For a file-copy primitive used elsewhere in the CLI, this is the wrong default — successful Sync() is not the same as successful Close().

Severity

Medium — Issue (1) produces noise in failure logs and is purely cosmetic. Issue (2) is a correctness gap: rare but real, and exactly the kind of bug that hides until the CLI is run against an NFS or sshfs mount in CI.

Evidence

Current implementation at pkg/fileutil/fileutil.go:120-147:

func CopyFile(src, dst string) error {
    in, err := os.Open(src)
    if err != nil { /* ... */ return err }
    defer in.Close()

    out, err := os.Create(dst)
    if err != nil { /* ... */ return err }
    defer func() { _ = out.Close() }()

    if _, err = io.Copy(out, in); err != nil {
        if closeErr := out.Close(); closeErr != nil {
            log.Printf("Failed to close destination file during cleanup: %s", closeErr)
        }
        if removeErr := os.Remove(dst); removeErr != nil {
            log.Printf("Failed to remove partial destination file during cleanup: %s", removeErr)
        }
        return err
    }
    log.Printf("File copied successfully: src=%s, dst=%s", src, dst)
    return out.Sync()  // Close error is discarded by the deferred Close
}

Recommendation

Adopt the canonical Go pattern: capture the close error, prefer the earliest non-nil error, and avoid double-closing.

func CopyFile(src, dst string) (err error) {
    in, openErr := os.Open(src)
    if openErr != nil { /* ... */ return openErr }
    defer in.Close()

    out, createErr := os.Create(dst)
    if createErr != nil { /* ... */ return createErr }
    defer func() {
        if closeErr := out.Close(); closeErr != nil && err == nil {
            err = closeErr
        }
    }()

    if _, copyErr := io.Copy(out, in); copyErr != nil {
        if removeErr := os.Remove(dst); removeErr != nil {
            log.Printf("Failed to remove partial destination file during cleanup: %s", removeErr)
        }
        return copyErr
    }
    return out.Sync()
}

This change:

• Removes the double-close (and its misleading log line)
• Surfaces a Close error to the caller when it would otherwise hide a partial write
• Preserves the existing partial-file removal behaviour on io.Copy failure

Validation

• go test ./pkg/fileutil/... — existing tests cover /dev/full ENOSPC and basic copy cases
• Add a test that injects a Close error via a fake file (or simulate via an NFS mount in CI) — currently the success-path Close error is untested
• Verify the misleading log message no longer appears in error-path tests

Estimated Effort

Small — One function, ~15 line edit, signature change limited to a named return.

Sergo Run Context

• Strategy: concurrency-safety-and-resource-lifecycle
• Run ID: §25592031127

Generated by Sergo - Serena Go Expert · ● 16.9M · ◷

• expires on May 16, 2026, 4:55 AM UTC