fix(teststate): stop after reporting a fatal error

Every t.Fatalf in this package fell through to the code after it. With
*testing.T that is invisible, since FailNow calls runtime.Goexit, but
TestingT exists so other harnesses can be plugged in and an implementation
whose FailNow returns kept going.

The harmful case was save: a marshal failure was reported and then
os.WriteFile ran anyway with a nil byte slice, leaving a zero byte file for
a later stage to load. IsPresent and IsEmptyJSON were quieter but wrong in
the same way, reporting absent or empty when the real answer was an error
they had just announced.

Add an explicit return after each Fatalf, with a package comment explaining
why they are not dead code.

Author: james00012

modules/core/teststate/teststate.go

@@ -7,6 +7,11 @@
 //
 // This package lives in core rather than teststructure so that modules such as aws, k8s, packer, and ssh can provide
 // their own helpers without teststructure having to import every one of them.
+// Every t.Fatalf in this package is followed by an explicit return. testing.TestingT documents FailNow as stopping
+// execution via runtime.Goexit, and *testing.T honours that, so those returns are unreachable in ordinary use. They
+// are not decorative: TestingT exists so other harnesses can be plugged in, and an implementation whose FailNow
+// returns would otherwise carry on past the failure. In save that meant writing a zero byte file after a marshal
+// error, and in IsPresent and IsEmptyJSON it meant masking the real error behind a plausible looking answer.
 package teststate
 
 import (
@@ -73,6 +78,8 @@ func save(t testing.TestingT, path string, overwrite bool, value any, loggedVal
 	bytes, err := json.Marshal(value)
 	if err != nil {
 		t.Fatalf("Failed to convert value %s to JSON: %v", path, err)
+
+		return
 	}
 
 	if loggedVal {
@@ -83,6 +90,8 @@ func save(t testing.TestingT, path string, overwrite bool, value any, loggedVal
 
 	if err := os.MkdirAll(parentDir, 0o755); err != nil {
 		t.Fatalf("Failed to create folder %s: %v", parentDir, err)
+
+		return
 	}
 
 	// 0o600: this file can hold secrets, such as the private key in an aws.Ec2Keypair or an ssh.KeyPair.
@@ -100,6 +109,8 @@ func Load(t testing.TestingT, path string, value any) {
 	bytes, err := os.ReadFile(path)
 	if err != nil {
 		t.Fatalf("Failed to load value from %s: %v", path, err)
+
+		return
 	}
 
 	if err := json.Unmarshal(bytes, value); err != nil {
@@ -112,6 +123,8 @@ func IsPresent(t testing.TestingT, path string) bool {
 	exists, err := files.FileExistsE(path)
 	if err != nil {
 		t.Fatalf("Failed to load test data from %s due to unexpected error: %v", path, err)
+
+		return false
 	}
 
 	if !exists {
@@ -121,6 +134,8 @@ func IsPresent(t testing.TestingT, path string) bool {
 	bytes, err := os.ReadFile(path)
 	if err != nil {
 		t.Fatalf("Failed to load test data from %s due to unexpected error: %v", path, err)
+
+		return false
 	}
 
 	if IsEmptyJSON(t, bytes) {
@@ -141,6 +156,8 @@ func IsEmptyJSON(t testing.TestingT, bytes []byte) bool {
 
 	if err := json.Unmarshal(bytes, &value); err != nil {
 		t.Fatalf("Failed to parse JSON while testing whether it is empty: %v", err)
+
+		return false
 	}
 
 	if value == nil {

modules/core/teststate/teststate_test.go

@@ -204,3 +204,74 @@ func TestSaveWritesOwnerOnlyPermissions(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, os.FileMode(0o600), info.Mode().Perm(), "saved test data must be owner read/write only")
 }
+
+// nonStoppingT is a testing.TestingT whose FailNow returns instead of calling runtime.Goexit. The interface
+// documents Goexit semantics, but TestingT exists so other harnesses can be plugged in, and this pins that a
+// harness which does not stop cannot make this package do damage after it has reported a failure.
+type nonStoppingT struct {
+	failed bool
+	msgs   []string
+}
+
+func (r *nonStoppingT) Fail()                 { r.failed = true }
+func (r *nonStoppingT) FailNow()              { r.failed = true }
+func (r *nonStoppingT) Error(args ...any)     { r.failed = true }
+func (r *nonStoppingT) Errorf(string, ...any) { r.failed = true }
+func (r *nonStoppingT) Fatal(args ...any)     { r.msgs = append(r.msgs, fmt.Sprint(args...)); r.FailNow() }
+func (r *nonStoppingT) Name() string          { return "nonStoppingT" }
+func (r *nonStoppingT) Helper()               {}
+func (r *nonStoppingT) Fatalf(f string, a ...any) {
+	r.msgs = append(r.msgs, fmt.Sprintf(f, a...))
+	r.FailNow()
+}
+
+// unmarshalable has a func field, which encoding/json always rejects, so Save fails at the marshal step.
+type unmarshalable struct {
+	Fn func()
+}
+
+// TestSaveWritesNothingAfterAMarshalFailure is the regression test. Before the explicit return, save reported the
+// marshal failure and then carried on to os.WriteFile with a nil byte slice, leaving a zero byte file that a later
+// stage would try to load.
+func TestSaveWritesNothingAfterAMarshalFailure(t *testing.T) {
+	t.Parallel()
+
+	folder := t.TempDir()
+	path := teststate.FormatPath(folder, "Broken.json")
+
+	recorder := &nonStoppingT{}
+	teststate.Save(recorder, path, true, unmarshalable{})
+
+	assert.True(t, recorder.failed, "the marshal failure must be reported")
+	assert.NoFileExists(t, path, "no file may be written after a marshal failure")
+}
+
+// TestIsPresentDoesNotMaskAnUnreadableFile pins that a read failure reports the failure rather than quietly
+// answering "absent", which would invite a caller to overwrite state it could not read.
+func TestIsPresentDoesNotMaskAnUnreadableFile(t *testing.T) {
+	t.Parallel()
+
+	// A directory where a file is expected: FileExistsE succeeds, os.ReadFile then fails with EISDIR.
+	folder := t.TempDir()
+	path := teststate.FormatPath(folder, "IsADirectory.json")
+	require.NoError(t, os.MkdirAll(path, 0o755))
+
+	recorder := &nonStoppingT{}
+	present := teststate.IsPresent(recorder, path)
+
+	assert.True(t, recorder.failed, "the read failure must be reported")
+	assert.False(t, present)
+	require.NotEmpty(t, recorder.msgs)
+	assert.Contains(t, recorder.msgs[0], "unexpected error")
+}
+
+// TestIsEmptyJSONReportsAParseFailure pins that invalid JSON is reported rather than being called empty.
+func TestIsEmptyJSONReportsAParseFailure(t *testing.T) {
+	t.Parallel()
+
+	recorder := &nonStoppingT{}
+	empty := teststate.IsEmptyJSON(recorder, []byte("{not json"))
+
+	assert.True(t, recorder.failed, "the parse failure must be reported")
+	assert.False(t, empty, "invalid JSON is not the same as empty JSON")
+}