cmd/cue: do not ignore Encoder.Close errors on export

Typically, this is not a big deal: "cue export" writes to stdout,
so the output is streamed to stdout directly, which should not fail.

However, when using "cue export -o", we buffer the entire output
and Encoder.Close actually writes it to the filename via os.WriteFile.
Ignoring the error from os.WriteFile means we are ignoring any error
related to creating the file, writing to it, or flushing/closing it.

Stop ignoring the error by doing what other commands like def do.
Note that they don't defer closing the encoder, which seems okay;
all that Close does today is the equivalent of os.WriteFile,
so there aren't any resources like a file descriptor to release.

This fixes the first test case in the previous commit,
inverting the expected outcome to match our expectations.

While here, I also noticed a related issue with the writer func,
which is what constructs the only non-nil Encoder.Close function value.
The output "-o" flag comes with a "-f" flag to force overwriting
existing files, which was implemented via an upfront call to os.Stat.

This was racy: if I make a call to "cue export -o out.json",
and evaluating and exporting my CUE takes around one second,
it could be that a different process has created the file
after the initial os.Stat call but before the final os.WriteFile call.
"cue export -o" thus overwrites the file even without the "-f" flag.

Instead, implement the logic to allow or disallow overwriting a file
by using different os.FileModes and calling os.OpenFile directly,
rather than using the simpler and higher level os.WriteFile.
Without "-f", we allow creating a file with the mode bit os.O_EXCL,
so we get the underlying error fs.ErrExist if the file already exists.
With "-f", we use os.O_TRUNC instead, so we allow replacing an existing
file and truncate it to zero bytes before writing - like os.WriteFile.

This fixes the second test case in the previous commit,
uncommenting the relevant lines since the behavior is no longer racy.

Signed-off-by: Daniel Martí <[email protected]>
Change-Id: Ia78ebf5569d5bbf1dac4d660bcb2d520d97be680
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1169709
TryBot-Result: CUEcueckoo <[email protected]>
Unity-Result: CUE porcuepine <[email protected]>
Reviewed-by: Marcel van Lohuizen <[email protected]>

Author: mvdan

cmd/cue/cmd/export.go

@@ -115,7 +115,6 @@ func runExport(cmd *Command, args []string) error {
 
 	enc, err := encoding.NewEncoder(b.outFile, b.encConfig)
 	exitOnErr(cmd, err, true)
-	defer enc.Close()
 
 	iter := b.instances()
 	defer iter.close()
@@ -125,5 +124,9 @@ func runExport(cmd *Command, args []string) error {
 		exitOnErr(cmd, err, true)
 	}
 	exitOnErr(cmd, iter.err(), true)
+
+	err = enc.Close()
+	exitOnErr(cmd, err, true)
+
 	return nil
 }

cmd/cue/cmd/testdata/script/export_force.txtar

@@ -13,22 +13,20 @@ exec cue export --force -o test.yml file.cue
 cmp test.yml test.yml.golden
 
 # With or without --force, we should fail to output to a file inside a missing directory.
-# TODO: these should not succeed; the following commit fixes the bug.
-exec cue export -o /definitely/does/not/exist/test.yml file.cue
-exec cue export --force -o /definitely/does/not/exist/test.yml file.cue
+! exec cue export -o /definitely/does/not/exist/test.yml file.cue
+! exec cue export --force -o /definitely/does/not/exist/test.yml file.cue
 
 # Two concurrent exports to the same new file without --force;
 # only one should succeed. We use a relatively slow bit of CUE
 # to make it likely that both export operations begin before either
 # has finished and created the resulting output file.
 # Since it's a coin toss which command wins the race,
 # we allow both to fail but expect the joint stderr to contain exactly one error.
-# TODO: this is currently racy; the following commit fixes the bug.
-# exec cue_exitzero export -o conflict.yml slow.cue &
-# exec cue_exitzero export -o conflict.yml slow.cue &
-# wait
-# stderr -count=1 'error writing "conflict.yml": file already exists'
-# exists conflict.yml
+exec cue_exitzero export -o conflict.yml slow.cue &
+exec cue_exitzero export -o conflict.yml slow.cue &
+wait
+stderr -count=1 'error writing "conflict.yml": file already exists'
+exists conflict.yml
 
 # Now with --force; the two commands should always succeed.
 exec cue export --force -o conflict_force.yml slow.cue &

internal/encoding/encoder.go

@@ -19,6 +19,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"path/filepath"
 
@@ -300,17 +301,27 @@ func writer(f *build.File, cfg *Config) (_ io.Writer, close func() error, err er
 		}
 		return cfg.Stdout, nil, nil
 	}
-	if !cfg.Force {
-		if _, err := os.Stat(path); err == nil {
-			return nil, nil, errors.Wrapf(os.ErrExist, token.NoPos,
-				"error writing %q", path)
-		}
-	}
-	// Delay opening the file until we can write it to completion. This will
-	// prevent clobbering the file in case of a crash.
+	// Delay opening the file until we can write it to completion.
+	// This prevents clobbering the file in case of a crash.
 	b := &bytes.Buffer{}
 	fn := func() error {
-		return os.WriteFile(path, b.Bytes(), 0644)
+		mode := os.O_WRONLY | os.O_CREATE | os.O_EXCL
+		if cfg.Force {
+			// Swap O_EXCL for O_TRUNC to allow replacing an entire existing file.
+			mode = os.O_WRONLY | os.O_CREATE | os.O_TRUNC
+		}
+		f, err := os.OpenFile(path, mode, 0o644)
+		if err != nil {
+			if errors.Is(err, fs.ErrExist) {
+				return errors.Wrapf(fs.ErrExist, token.NoPos, "error writing %q", path)
+			}
+			return err
+		}
+		_, err = f.Write(b.Bytes())
+		if err1 := f.Close(); err1 != nil && err == nil {
+			err = err1
+		}
+		return err
 	}
 	return b, fn, nil
 }