diff --git a/main.go b/main.go index e14ae1b..27f8d0c 100644 --- a/main.go +++ b/main.go @@ -1153,22 +1153,31 @@ func publishWatchReadiness(path string, readinessErr error) error { func waitWatchReadiness(path string, timeout time.Duration) error { deadline := time.Now().Add(timeout) + // A readiness file that does not parse is a file still being written, not + // a daemon that failed: publishWatchReadiness renames its payload into + // place atomically, but nothing guarantees every writer does, and treating + // the first unparseable read as fatal reported a startup failure for a + // daemon that was starting fine. Keep the last parse error so a file that + // never becomes valid says why, rather than only that it timed out. + var lastParseErr error for { data, err := os.ReadFile(path) if err == nil { var status watchReadiness - if err := json.Unmarshal(data, &status); err != nil { - return fmt.Errorf("reading daemon readiness: %w", err) - } - if status.Error != "" { + if parseErr := json.Unmarshal(data, &status); parseErr != nil { + lastParseErr = parseErr + } else if status.Error != "" { return errors.New(status.Error) + } else { + return nil } - return nil - } - if !errors.Is(err, os.ErrNotExist) { + } else if !errors.Is(err, os.ErrNotExist) { return fmt.Errorf("reading daemon readiness: %w", err) } if time.Now().After(deadline) { + if lastParseErr != nil { + return fmt.Errorf("reading daemon readiness: %w", lastParseErr) + } return fmt.Errorf("daemon readiness timed out after %s", timeout) } time.Sleep(10 * time.Millisecond) diff --git a/watch_readiness_test.go b/watch_readiness_test.go new file mode 100644 index 0000000..e2a10cb --- /dev/null +++ b/watch_readiness_test.go @@ -0,0 +1,66 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// A readiness file caught mid-write reads as zero or partial bytes. Treating +// that as fatal reported a startup failure for a daemon that was starting +// fine, and made TestRunWatchStartWaitsForChildReadinessFailure fail whenever +// the machine was loaded enough to land in the window. +func TestWaitWatchReadinessWaitsThroughPartialWrite(t *testing.T) { + path := filepath.Join(t.TempDir(), "ready.json") + if err := os.WriteFile(path, nil, 0o644); err != nil { + t.Fatal(err) + } + go func() { + time.Sleep(50 * time.Millisecond) + _ = os.WriteFile(path, []byte(`{"error":"claim rejected"}`), 0o644) + }() + + err := waitWatchReadiness(path, 5*time.Second) + if err == nil || !strings.Contains(err.Error(), "claim rejected") { + t.Fatalf("waitWatchReadiness() = %v, want the daemon's own error once the file is complete", err) + } +} + +func TestWaitWatchReadinessSucceedsAfterPartialWrite(t *testing.T) { + path := filepath.Join(t.TempDir(), "ready.json") + if err := os.WriteFile(path, []byte(`{"err`), 0o644); err != nil { + t.Fatal(err) + } + go func() { + time.Sleep(50 * time.Millisecond) + _ = os.WriteFile(path, []byte(`{}`), 0o644) + }() + + if err := waitWatchReadiness(path, 5*time.Second); err != nil { + t.Fatalf("waitWatchReadiness() = %v, want success once the file is complete", err) + } +} + +// A file that never becomes valid still has to say why, rather than reporting +// only that it timed out. +func TestWaitWatchReadinessReportsPersistentGarbage(t *testing.T) { + path := filepath.Join(t.TempDir(), "ready.json") + if err := os.WriteFile(path, []byte("not json at all"), 0o644); err != nil { + t.Fatal(err) + } + err := waitWatchReadiness(path, 200*time.Millisecond) + if err == nil || !strings.Contains(err.Error(), "reading daemon readiness") { + t.Fatalf("waitWatchReadiness() = %v, want the parse failure surfaced", err) + } +} + +// A missing file must still time out rather than hang. +func TestWaitWatchReadinessTimesOutWhenAbsent(t *testing.T) { + path := filepath.Join(t.TempDir(), "never-written.json") + err := waitWatchReadiness(path, 100*time.Millisecond) + if err == nil || !strings.Contains(err.Error(), "timed out") { + t.Fatalf("waitWatchReadiness() = %v, want a timeout", err) + } +}