From fde3ae7efd2dc51d16f70c072ce7413f8038b1fd Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Fri, 11 Sep 2026 14:29:48 +0700 Subject: [PATCH] refactor: share one atomic write path for generated files README.md and site/data.json were written with os.Create/os.WriteFile, so a write that failed midway left the repo front page truncated, while history.jsonl already used a temp-file-plus-rename. Extract that pattern into atomicWriteFile and route all three writers through it. Also sort snapshots by date when reading history.jsonl: delta windows pick the newest snapshot inside the window by scan order, which silently produces wrong deltas if a hand edit or a merge of two concurrent runs interleaves lines. Cover the README renderer, which had no test beyond sanitizeCell, and name the generated paths as constants instead of repeating literals. Coverage 65.1% -> 75.7%. --- fsutil.go | 38 ++++++++++++++++++++++ fsutil_test.go | 67 +++++++++++++++++++++++++++++++++++++++ history.go | 52 +++++++++++++----------------- history_test.go | 40 +++++++++++++++++++++++ main.go | 21 +++++++++---- readme.go | 24 +++++--------- readme_test.go | 84 +++++++++++++++++++++++++++++++++++++++++++++++++ site.go | 16 +++++----- 8 files changed, 280 insertions(+), 62 deletions(-) create mode 100644 fsutil.go create mode 100644 fsutil_test.go diff --git a/fsutil.go b/fsutil.go new file mode 100644 index 0000000..e2e7bad --- /dev/null +++ b/fsutil.go @@ -0,0 +1,38 @@ +package main + +import ( + "io" + "os" +) + +// atomicWriteFile writes through a temp file in the same directory and renames +// it over path, so a crash or a failed write never leaves a generated file +// truncated or half-written. The callback receives the temp file as an +// io.Writer, which suits streaming producers (templates, JSON encoders) +// without buffering the whole payload in memory. +// +// Errors from Sync and Close are reported too: on a write path they are the +// difference between "data reached the disk" and silent loss. +func atomicWriteFile(path string, write func(io.Writer) error) error { + tmp := path + ".tmp" + f, err := os.Create(tmp) + if err != nil { + return err + } + + writeErr := write(f) + + if syncErr := f.Sync(); syncErr != nil && writeErr == nil { + writeErr = syncErr + } + if closeErr := f.Close(); closeErr != nil && writeErr == nil { + writeErr = closeErr + } + + if writeErr != nil { + _ = os.Remove(tmp) // best-effort cleanup; the write error is what matters + return writeErr + } + + return os.Rename(tmp, path) +} diff --git a/fsutil_test.go b/fsutil_test.go new file mode 100644 index 0000000..9b194b1 --- /dev/null +++ b/fsutil_test.go @@ -0,0 +1,67 @@ +package main + +import ( + "errors" + "io" + "os" + "testing" +) + +func TestAtomicWriteFile_WritesAndLeavesNoTemp(t *testing.T) { + path := t.TempDir() + "/out.txt" + + if err := atomicWriteFile(path, func(w io.Writer) error { + _, err := io.WriteString(w, "hello") + return err + }); err != nil { + t.Fatalf("atomicWriteFile: %v", err) + } + + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(got) != "hello" { + t.Errorf("content = %q, want %q", got, "hello") + } + if _, err := os.Stat(path + ".tmp"); !os.IsNotExist(err) { + t.Errorf("temp file still present after success") + } +} + +func TestAtomicWriteFile_FailedWriteKeepsPreviousContent(t *testing.T) { + // The point of the temp-file dance: a generated file (README.md, + // history.jsonl) must never be truncated by a write that fails midway. + path := t.TempDir() + "/out.txt" + if err := os.WriteFile(path, []byte("previous"), 0o644); err != nil { + t.Fatalf("seed file: %v", err) + } + + boom := errors.New("boom") + err := atomicWriteFile(path, func(w io.Writer) error { + _, _ = io.WriteString(w, "partial") + return boom + }) + if !errors.Is(err, boom) { + t.Fatalf("err = %v, want %v", err, boom) + } + + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(got) != "previous" { + t.Errorf("content = %q, want the file left untouched", got) + } + if _, err := os.Stat(path + ".tmp"); !os.IsNotExist(err) { + t.Errorf("temp file left behind after a failed write") + } +} + +func TestAtomicWriteFile_UncreatableTempReportsError(t *testing.T) { + if err := atomicWriteFile(t.TempDir()+"/missing-dir/out.txt", func(w io.Writer) error { + return nil + }); err == nil { + t.Fatal("expected an error when the temp file cannot be created") + } +} diff --git a/history.go b/history.go index daaa7db..cb99382 100644 --- a/history.go +++ b/history.go @@ -2,8 +2,10 @@ package main import ( "bufio" + "cmp" "encoding/json" "fmt" + "io" "os" "slices" "time" @@ -98,7 +100,16 @@ func readSnapshots(path string) ([]Snapshot, error) { s.Stars = applyMigrations(s.Stars) out = append(out, s) } - return out, scanner.Err() + if err := scanner.Err(); err != nil { + return nil, err + } + + // Delta windows pick the newest snapshot inside the window by scan order, + // so the slice must be date-ascending. Sort rather than trust the file: + // a hand edit or a merge of two concurrent runs can interleave lines. + slices.SortStableFunc(out, func(a, b Snapshot) int { return cmp.Compare(a.Date, b.Date) }) + + return out, nil } // applyMigrations rewrites any deprecated history keys to their current @@ -156,38 +167,17 @@ func resolveCanonicalKey(key string) string { return key } -// writeSnapshots writes to a temp file then renames atomically so a crash -// mid-write never leaves history.jsonl truncated or partially written. +// writeSnapshots persists every snapshot as one JSON object per line. func writeSnapshots(path string, snapshots []Snapshot) error { - tmp := path + ".tmp" - f, err := os.Create(tmp) - if err != nil { - return err - } - - enc := json.NewEncoder(f) - writeErr := error(nil) - for _, s := range snapshots { - if err := enc.Encode(s); err != nil { - writeErr = err - break + return atomicWriteFile(path, func(w io.Writer) error { + enc := json.NewEncoder(w) + for _, s := range snapshots { + if err := enc.Encode(s); err != nil { + return err + } } - } - - if syncErr := f.Sync(); syncErr != nil && writeErr == nil { - writeErr = syncErr - } - // A failed close on a write path can hide lost data โ€” surface it. - if closeErr := f.Close(); closeErr != nil && writeErr == nil { - writeErr = closeErr - } - - if writeErr != nil { - _ = os.Remove(tmp) // best-effort cleanup; the write error is what matters - return writeErr - } - - return os.Rename(tmp, path) + return nil + }) } // computeDeltas returns stars-now minus stars-at-or-before-cutoff for each diff --git a/history_test.go b/history_test.go index 11eb96a..7c35956 100644 --- a/history_test.go +++ b/history_test.go @@ -393,3 +393,43 @@ func TestWriteSnapshots_AtomicWrite(t *testing.T) { t.Errorf("temp file should not exist after successful write") } } + +func TestReadSnapshots_SortsOutOfOrderDates(t *testing.T) { + // A hand edit or a merge of two concurrent updater runs can leave lines + // out of date order; delta windows depend on ascending order. + path := t.TempDir() + "/history.jsonl" + lines := `{"date":"2026-09-03","stars":{"org/repo":300}} +{"date":"2026-08-27","stars":{"org/repo":100}} +{"date":"2026-09-10","stars":{"org/repo":400}} +{"date":"2026-09-01","stars":{"org/repo":200}} +` + if err := os.WriteFile(path, []byte(lines), 0o644); err != nil { + t.Fatalf("seed file: %v", err) + } + + got, err := readSnapshots(path) + if err != nil { + t.Fatalf("readSnapshots: %v", err) + } + want := []string{"2026-08-27", "2026-09-01", "2026-09-03", "2026-09-10"} + if len(got) != len(want) { + t.Fatalf("got %d snapshots, want %d", len(got), len(want)) + } + for i, d := range want { + if got[i].Date != d { + t.Errorf("snapshot %d date = %s, want %s", i, got[i].Date, d) + } + } + + // With the dates ordered, the 7-day base is 2026-09-03 (not the + // last line in the file), so the delta is 400-300. + fixed := time.Date(2026, 9, 10, 0, 0, 0, 0, time.UTC) + orig := timeNow + timeNow = func() time.Time { return fixed } + defer func() { timeNow = orig }() + + deltas := computeDeltas(got, Snapshot{Date: "2026-09-10", Stars: map[string]int{"org/repo": 400}}) + if deltas["org/repo"] != 100 { + t.Errorf("delta = %d, want 100", deltas["org/repo"]) + } +} diff --git a/main.go b/main.go index 2f28b1b..b756b24 100644 --- a/main.go +++ b/main.go @@ -7,12 +7,21 @@ import ( "os" ) +// Paths the updater reads and writes, relative to the repository root. +const ( + agentsPath = "data/agents.yml" + historyPath = "data/history.jsonl" + readmeTmpl = "templates/readme.tmpl" + readmePath = "README.md" + siteDataPath = "site/data.json" +) + func main() { check := flag.Bool("check", false, "validate data/agents.yml offline (no network, no token) and exit") flag.Parse() if *check { - if err := runCheck("data/agents.yml"); err != nil { + if err := runCheck(agentsPath); err != nil { log.Printf("check failed: %v", err) os.Exit(1) } @@ -25,12 +34,12 @@ func main() { } func run() error { - agents, err := loadAgents("data/agents.yml") + agents, err := loadAgents(agentsPath) if err != nil { return err } if len(agents) == 0 { - return fmt.Errorf("no agents in data/agents.yml") + return fmt.Errorf("no agents in %s", agentsPath) } token := os.Getenv("GITHUB_TOKEN") @@ -43,16 +52,16 @@ func run() error { return err } - snapshots, deltas7, deltas30, err := appendHistory("data/history.jsonl", stats) + snapshots, deltas7, deltas30, err := appendHistory(historyPath, stats) if err != nil { return err } - if err := renderReadme("templates/readme.tmpl", "README.md", stats, deltas7); err != nil { + if err := renderReadme(readmeTmpl, readmePath, stats, deltas7); err != nil { return err } - if err := writeSiteData("site/data.json", stats, deltas7, deltas30, snapshots); err != nil { + if err := writeSiteData(siteDataPath, stats, deltas7, deltas30, snapshots); err != nil { return err } diff --git a/readme.go b/readme.go index 964ebc8..b8d7036 100644 --- a/readme.go +++ b/readme.go @@ -2,7 +2,7 @@ package main import ( "fmt" - "os" + "io" "path/filepath" "strings" "text/template" @@ -84,22 +84,14 @@ func renderReadme(tmplPath, outPath string, stats []Stat, deltas map[string]int) return err } - f, err := os.Create(outPath) - if err != nil { - return err - } - - execErr := tmpl.ExecuteTemplate(f, filepath.Base(tmplPath), map[string]any{ - "Rows": rows, - "UpdatedAt": timeNow().UTC().Format("2006-01-02 15:04 UTC"), - "Total": len(rows), - "TopMover": topMover, + return atomicWriteFile(outPath, func(w io.Writer) error { + return tmpl.ExecuteTemplate(w, filepath.Base(tmplPath), map[string]any{ + "Rows": rows, + "UpdatedAt": timeNow().UTC().Format("2006-01-02 15:04 UTC"), + "Total": len(rows), + "TopMover": topMover, + }) }) - // A failed close on a write path can hide lost data โ€” surface it. - if closeErr := f.Close(); closeErr != nil && execErr == nil { - execErr = closeErr - } - return execErr } // sanitizeCell makes a third-party repo description safe to embed in a diff --git a/readme_test.go b/readme_test.go index fb6f62a..9ed9fb5 100644 --- a/readme_test.go +++ b/readme_test.go @@ -1,7 +1,10 @@ package main import ( + "os" + "strings" "testing" + "time" ) func TestSanitizeCell(t *testing.T) { @@ -118,3 +121,84 @@ func TestSanitizeCell(t *testing.T) { }) } } + +func TestRenderReadme_TableRowsAndCallout(t *testing.T) { + fixed := time.Date(2026, 9, 11, 3, 39, 0, 0, time.UTC) + orig := timeNow + timeNow = func() time.Time { return fixed } + defer func() { timeNow = orig }() + + stats := []Stat{ + {CanonicalKey: "org/big", NameWithOwner: "org/big", URL: "https://github.com/org/big", + Stars: 1_500_000, Language: "Go", PushedAt: fixed, Description: "huge | agent", Category: "cli"}, + {CanonicalKey: "org/mid", NameWithOwner: "org/mid", URL: "https://github.com/org/mid", + Stars: 1234, Language: "Rust", PushedAt: fixed, Description: "mid agent", Category: "cli"}, + {CanonicalKey: "org/small", NameWithOwner: "org/small", URL: "https://github.com/org/small", + Stars: 999, Language: "", PushedAt: fixed, Description: "small agent", Category: "cli"}, + } + // org/small has no delta: its row must show the em dash, and it must not + // win the top-mover callout. + deltas := map[string]int{"org/big": 12, "org/mid": -3} + + out := t.TempDir() + "/README.md" + if err := renderReadme("templates/readme.tmpl", out, stats, deltas); err != nil { + t.Fatalf("renderReadme: %v", err) + } + raw, err := os.ReadFile(out) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + got := string(raw) + + want := []string{ + "**Last updated:** 2026-09-11 03:39 UTC ยท **Tracked:** 3 repos", + "**Top 7-day mover:** [org/big](https://github.com/org/big) (+12 stars)", + "| 1 | [org/big](https://github.com/org/big) | 1.5M | +12 | Go | 2026-09-11 | huge \\| agent |", + "| 2 | [org/mid](https://github.com/org/mid) | 1.2k | -3 | Rust | 2026-09-11 | mid agent |", + "| 3 | [org/small](https://github.com/org/small) | 999 | โ€” | | 2026-09-11 | small agent |", + } + for _, w := range want { + if !strings.Contains(got, w) { + t.Errorf("README missing line:\n%s\n--- got ---\n%s", w, got) + } + } +} + +func TestRenderReadme_NoDeltasOmitsTopMover(t *testing.T) { + fixed := time.Date(2026, 9, 11, 3, 39, 0, 0, time.UTC) + orig := timeNow + timeNow = func() time.Time { return fixed } + defer func() { timeNow = orig }() + + stats := []Stat{{CanonicalKey: "org/repo", NameWithOwner: "org/repo", + URL: "https://github.com/org/repo", Stars: 10, PushedAt: fixed, Category: "cli"}} + + out := t.TempDir() + "/README.md" + if err := renderReadme("templates/readme.tmpl", out, stats, map[string]int{}); err != nil { + t.Fatalf("renderReadme: %v", err) + } + raw, err := os.ReadFile(out) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if strings.Contains(string(raw), "Top 7-day mover") { + t.Error("top-mover callout rendered with no deltas available") + } +} + +func TestRenderReadme_MissingTemplateDoesNotTouchOutput(t *testing.T) { + out := t.TempDir() + "/README.md" + if err := os.WriteFile(out, []byte("previous"), 0o644); err != nil { + t.Fatalf("seed file: %v", err) + } + if err := renderReadme("templates/does-not-exist.tmpl", out, nil, nil); err == nil { + t.Fatal("expected an error for a missing template") + } + raw, err := os.ReadFile(out) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(raw) != "previous" { + t.Errorf("output = %q, want the existing README left intact", raw) + } +} diff --git a/site.go b/site.go index aaefdff..ccac7b9 100644 --- a/site.go +++ b/site.go @@ -2,7 +2,7 @@ package main import ( "encoding/json" - "os" + "io" ) // siteRow is one ranked repo in site/data.json, consumed by site/index.html. @@ -55,13 +55,11 @@ func writeSiteData(path string, stats []Stat, deltas7, deltas30 map[string]int, } } - out, err := json.Marshal(siteData{ - UpdatedAt: timeNow().UTC().Format("2006-01-02 15:04 UTC"), - Rows: rows, - History: history, + return atomicWriteFile(path, func(w io.Writer) error { + return json.NewEncoder(w).Encode(siteData{ + UpdatedAt: timeNow().UTC().Format("2006-01-02 15:04 UTC"), + Rows: rows, + History: history, + }) }) - if err != nil { - return err - } - return os.WriteFile(path, out, 0o644) }