diff --git a/internal/atomicfile/atomicfile.go b/internal/atomicfile/atomicfile.go new file mode 100644 index 000000000..3bb0b9fae --- /dev/null +++ b/internal/atomicfile/atomicfile.go @@ -0,0 +1,118 @@ +// Package atomicfile writes files atomically: into a UNIQUE temp file in the +// destination's own directory, then rename over the destination. +// +// # Why this package exists (#6018) +// +// The idiom this replaces was written by hand across ~48 sites: +// +// tmp := path + ".tmp" +// os.WriteFile(tmp, b, 0o644) +// os.Rename(tmp, path) +// +// The temp name there is DETERMINISTIC PER DESTINATION, so it is shared by +// every writer aiming at that destination. Two concurrent writers both O_TRUNC +// the same temp inode and interleave their bytes into it; the destination can +// then be left holding a torn mix of two payloads, and whoever renames second +// fails with ENOENT because the first already moved the file away. The damaging +// half is not the lost write — it is the garbled one, which the next reader +// treats as truth. Where the rename error is discarded (several best-effort +// caches do that deliberately) the collision is entirely invisible at runtime. +// +// That bug was independently diagnosed and point-fixed three times before this +// package existed: internal/statusfile (review #5734), +// internal/daemon/watchreg, and internal/links (#5978). This is the shared +// remedy so there is no fourth. +// +// # Mode semantics — deliberately NOT umask-masked +// +// os.WriteFile passes perm through open(2), so the process umask narrows the +// resulting mode. os.CreateTemp always creates the file 0600 and the Chmod +// below then sets EXACTLY the requested perm, so the umask does not apply. +// Under a restrictive umask (077) a converted destination therefore widens +// from 0600 to 0644. +// +// This is a deliberate, one-time decision (#5978, generalised in #6018) and is +// not to be re-litigated per call site. The alternative — reading the umask to +// mask perm by hand — requires syscall.Umask, which is process-global, has no +// read-only form, and is racy against any concurrent file creation in the +// process. The destinations here are per-user state under the grafel home, and +// a mode that does not depend on which process (daemon, CLI, or child) happened +// to write the file is easier to reason about than one that does. Callers that +// need a genuinely private file must pass 0o600 explicitly rather than relying +// on a umask to narrow 0o644 for them. +// +// The Chmod is load-bearing and easy to delete without breaking anything +// visible — deleting it left the whole of #5978's package green. It is pinned +// by TestWriteFile_AppliesPerm; keep that test honest. +// +// # Other ways this differs from os.WriteFile +// +// All of these follow from rename-over-destination, so the hand-written +// temp+rename code this replaced behaved the same way. They are listed because +// they differ from the os.WriteFile the call sites LOOK like they still use. +// +// - The destination directory must already exist. WriteFile does not +// MkdirAll; os.CreateTemp fails on a missing directory exactly as +// os.WriteFile would. Call sites keep their own MkdirAll. +// - A SYMLINK at path is REPLACED by a regular file, not written through to +// its target. os.WriteFile follows the link. +// - A READ-ONLY (0444) destination is REPLACED. os.WriteFile fails with +// EACCES. What governs is write permission on the DIRECTORY, not on the +// existing file. +// +// # Orphaned temp files after a crash +// +// A deterministic path+".tmp" was self-healing across crashes: the next write +// simply truncated the orphan. Unique names give that up — a process killed +// between CreateTemp and Rename leaves a distinct `..tmp-` +// behind, and nothing sweeps `*.tmp-*` in any of the destination directories. +// +// Accepted deliberately: the leak is bounded by crash frequency, the files are +// small, and the alternative is the torn-write bug. #5978 made the same trade +// silently; it is written down here so the next reader does not have to +// rediscover it. A sweeper belongs with whatever GCs these directories, not in +// the write path. +package atomicfile + +import ( + "os" + "path/filepath" +) + +// WriteFile writes b to path atomically and sets path's mode to perm. +// +// The temp file is created by os.CreateTemp in path's OWN directory, so the +// rename stays within one filesystem and is therefore atomic. It is removed on +// every error path. +// +// WriteFile does NOT create the destination directory: os.CreateTemp fails on a +// missing directory exactly as os.WriteFile would have. Callers that need the +// directory keep their existing os.MkdirAll. +// +// perm is applied verbatim and is NOT masked by the process umask — see the +// package doc. +func WriteFile(path string, b []byte, perm os.FileMode) (err error) { + f, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp-*") + if err != nil { + return err + } + tmp := f.Name() + defer func() { + if err != nil { + _ = os.Remove(tmp) + } + }() + if _, err = f.Write(b); err != nil { + _ = f.Close() + return err + } + if err = f.Close(); err != nil { + return err + } + // os.CreateTemp always uses 0600; set the caller's intended mode. + if err = os.Chmod(tmp, perm); err != nil { + return err + } + err = os.Rename(tmp, path) + return err +} diff --git a/internal/atomicfile/atomicfile_test.go b/internal/atomicfile/atomicfile_test.go new file mode 100644 index 000000000..540e08092 --- /dev/null +++ b/internal/atomicfile/atomicfile_test.go @@ -0,0 +1,352 @@ +package atomicfile_test + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "sync" + "testing" + + "github.com/cajasmota/grafel/internal/atomicfile" +) + +func TestWriteFile_WritesContent(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "out.json") + want := []byte(`{"hello":"world"}`) + if err := atomicfile.WriteFile(path, want, 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if !bytes.Equal(got, want) { + t.Fatalf("content = %q, want %q", got, want) + } +} + +// TestWriteFile_AppliesPerm pins the Chmod. os.CreateTemp always creates the +// temp file 0600, so WITHOUT an explicit Chmod every destination lands at 0600 +// regardless of the requested perm. Deleting the Chmod from the helper must +// fail this test — that is the whole point of it (#5978 lost exactly this). +func TestWriteFile_AppliesPerm(t *testing.T) { + for _, perm := range []os.FileMode{0o600, 0o644, 0o755} { + t.Run(fmt.Sprintf("%04o", perm), func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "out.bin") + if err := atomicfile.WriteFile(path, []byte("x"), perm); err != nil { + t.Fatalf("WriteFile: %v", err) + } + fi, err := os.Stat(path) + if err != nil { + t.Fatalf("Stat: %v", err) + } + if got := fi.Mode().Perm(); got != perm { + t.Fatalf("mode = %04o, want %04o", got, perm) + } + }) + } +} + +// TestWriteFile_PermNotUmaskMasked pins the documented behaviour change: the +// mode is applied verbatim by Chmod and is NOT narrowed by the process umask, +// unlike the os.WriteFile form this replaces. +func TestWriteFile_PermNotUmaskMasked(t *testing.T) { + old := setUmask(t, 0o077) + defer setUmask(t, old) + + dir := t.TempDir() + path := filepath.Join(dir, "wide.json") + if err := atomicfile.WriteFile(path, []byte("x"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + fi, err := os.Stat(path) + if err != nil { + t.Fatalf("Stat: %v", err) + } + if got := fi.Mode().Perm(); got != 0o644 { + t.Fatalf("mode under umask 077 = %04o, want 0644 (perm must not be umask-masked)", got) + } +} + +// TestWriteFile_OverwritesExisting checks the rename replaces an existing +// destination and resets its mode to the requested one. +func TestWriteFile_OverwritesExisting(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "out.json") + if err := os.WriteFile(path, []byte("old-and-longer-content"), 0o600); err != nil { + t.Fatal(err) + } + if err := atomicfile.WriteFile(path, []byte("new"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(got) != "new" { + t.Fatalf("content = %q, want %q", got, "new") + } + fi, _ := os.Stat(path) + if fi.Mode().Perm() != 0o644 { + t.Fatalf("mode = %04o, want 0644", fi.Mode().Perm()) + } +} + +// TestWriteFile_NoTempLeftBehind asserts the success path leaves the +// destination directory holding exactly the destination. +func TestWriteFile_NoTempLeftBehind(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "out.json") + if err := atomicfile.WriteFile(path, []byte("payload"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + ents, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + if len(ents) != 1 || ents[0].Name() != "out.json" { + var names []string + for _, e := range ents { + names = append(names, e.Name()) + } + t.Fatalf("directory = %v, want only out.json", names) + } +} + +// TestWriteFile_RemovesTempOnError drives the rename failure path: the +// destination path is an existing DIRECTORY, so os.Rename fails and the temp +// file must not survive. +func TestWriteFile_RemovesTempOnError(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "dest") + if err := os.Mkdir(path, 0o755); err != nil { + t.Fatal(err) + } + if err := atomicfile.WriteFile(path, []byte("payload"), 0o644); err == nil { + t.Fatal("WriteFile over a directory: want error, got nil") + } + ents, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + if len(ents) != 1 || ents[0].Name() != "dest" { + var names []string + for _, e := range ents { + names = append(names, e.Name()) + } + t.Fatalf("directory = %v, want only the dest dir (temp must be removed)", names) + } +} + +// TestWriteFile_MissingDirErrors documents that WriteFile does NOT create the +// destination directory: os.CreateTemp fails there exactly as os.WriteFile +// would have. Callers keep their own MkdirAll. +func TestWriteFile_MissingDirErrors(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "nope", "out.json") + if err := atomicfile.WriteFile(path, []byte("x"), 0o644); err == nil { + t.Fatal("WriteFile into a missing directory: want error, got nil") + } +} + +// TestWriteFile_ReplacesSymlinkDestination pins that a symlink at path is +// REPLACED by a regular file rather than written through to its target. This +// differs from os.WriteFile and is inherent to rename-over-destination (the +// code this replaced renamed too, so it is not a regression). +func TestWriteFile_ReplacesSymlinkDestination(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target.json") + if err := os.WriteFile(target, []byte("original"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(dir, "link.json") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + if err := atomicfile.WriteFile(link, []byte("written"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + fi, err := os.Lstat(link) + if err != nil { + t.Fatal(err) + } + if fi.Mode()&os.ModeSymlink != 0 { + t.Fatal("destination is still a symlink; expected it to be replaced") + } + got, err := os.ReadFile(target) + if err != nil { + t.Fatal(err) + } + if string(got) != "original" { + t.Fatalf("symlink target = %q, want it untouched (%q)", got, "original") + } +} + +// TestWriteFile_OverwritesReadOnlyDestination pins that a 0444 destination is +// replaced, where os.WriteFile would fail with EACCES. Also inherent to +// rename: the permission that matters is the DIRECTORY's, not the file's. +func TestWriteFile_OverwritesReadOnlyDestination(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "ro.json") + if err := os.WriteFile(path, []byte("old"), 0o444); err != nil { + t.Fatal(err) + } + // Confirm the contrast rather than asserting it from memory. + if err := os.WriteFile(path, []byte("nope"), 0o444); err == nil { + t.Skip("os.WriteFile can write a 0444 file here (running as root?); contrast is meaningless") + } + if err := atomicfile.WriteFile(path, []byte("new"), 0o644); err != nil { + t.Fatalf("WriteFile over a 0444 destination: %v", err) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(got) != "new" { + t.Fatalf("content = %q, want %q", got, "new") + } +} + +// TestWriteFile_ConcurrentWritersSameDestination is the #6018 regression test. +// +// Eight goroutines write DIFFERENT payloads to one destination at once. Every +// write must succeed, and the surviving file must be exactly one COMPLETE +// payload — never a torn mix and never a short prefix. +// +// Against the deterministic `path + ".tmp"` shape this fails: the writers all +// O_TRUNC the same temp inode and interleave into it, and whoever renames +// second gets ENOENT because the first already moved it away. That failure is +// reproduced directly by TestDeterministicTempName_FailsConcurrently below, so +// the two tests together show this test actually binds the behaviour. +func TestWriteFile_ConcurrentWritersSameDestination(t *testing.T) { + const writers = 8 + const iterations = 40 + + payloads := makePayloads(writers) + valid := map[string]bool{} + for _, p := range payloads { + valid[string(p)] = true + } + + dir := t.TempDir() + path := filepath.Join(dir, "shared.json") + + for it := 0; it < iterations; it++ { + var wg sync.WaitGroup + errs := make([]error, writers) + start := make(chan struct{}) + for i := 0; i < writers; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + <-start + errs[i] = atomicfile.WriteFile(path, payloads[i], 0o644) + }(i) + } + close(start) + wg.Wait() + + for i, err := range errs { + if err != nil { + t.Fatalf("iteration %d writer %d: %v", it, i, err) + } + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("iteration %d: ReadFile: %v", it, err) + } + if !valid[string(got)] { + t.Fatalf("iteration %d: destination holds a torn write (%d bytes, prefix %q)", + it, len(got), truncate(got, 32)) + } + fi, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if fi.Mode().Perm() != 0o644 { + t.Fatalf("iteration %d: mode = %04o, want 0644", it, fi.Mode().Perm()) + } + } +} + +// TestDeterministicTempName_FailsConcurrently is the negative control for the +// test above: it runs the SAME concurrency harness against the old +// `path + ".tmp"` idiom and asserts it does in fact break (torn destination or +// a rename error). If this ever stops failing, the concurrency test above has +// stopped proving anything and must be strengthened. +func TestDeterministicTempName_FailsConcurrently(t *testing.T) { + const writers = 8 + const iterations = 200 + + payloads := makePayloads(writers) + valid := map[string]bool{} + for _, p := range payloads { + valid[string(p)] = true + } + + dir := t.TempDir() + path := filepath.Join(dir, "shared.json") + + // The unsafe idiom, verbatim as it appeared across the tree. + writeDeterministic := func(b []byte) error { + tmp := path + ".tmp" + if err := os.WriteFile(tmp, b, 0o644); err != nil { + return err + } + return os.Rename(tmp, path) + } + + for it := 0; it < iterations; it++ { + var wg sync.WaitGroup + errs := make([]error, writers) + start := make(chan struct{}) + for i := 0; i < writers; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + <-start + errs[i] = writeDeterministic(payloads[i]) + }(i) + } + close(start) + wg.Wait() + + for _, err := range errs { + if err != nil { + return // rename lost the race — the class, reproduced. + } + } + got, err := os.ReadFile(path) + if err != nil { + return + } + if !valid[string(got)] { + return // torn destination — the class, reproduced. + } + } + t.Fatalf("deterministic %q temp names survived %d rounds of %d concurrent writers; "+ + "the harness no longer reproduces #6018 and cannot vouch for the helper", + ".tmp", iterations, writers) +} + +func makePayloads(n int) [][]byte { + out := make([][]byte, n) + for i := range out { + // Distinct lengths and distinct bytes, and big enough that a + // partial write is visible rather than swallowed by one page. + out[i] = bytes.Repeat([]byte{byte('A' + i)}, 4096*(i+1)+i) + } + return out +} + +func truncate(b []byte, n int) string { + if len(b) <= n { + return string(b) + } + return string(b[:n]) + "..." +} diff --git a/internal/atomicfile/guard_6018_test.go b/internal/atomicfile/guard_6018_test.go new file mode 100644 index 000000000..59c269005 --- /dev/null +++ b/internal/atomicfile/guard_6018_test.go @@ -0,0 +1,458 @@ +package atomicfile_test + +// guard_6018_test.go — the guard that stops the #6018 class re-entering. +// +// It scans every non-test .go file in the repository for a temp path derived +// from its destination. Such a path is shared by every writer aiming at that +// destination, which is the whole bug (see the package doc). The shape was +// diagnosed and point-fixed three separate times before anyone generalised it; +// without a guard the fourth arrives with the next writer someone adds. +// +// The scan is AST-based, not grep, so a ".tmp" in a comment, a map key +// (internal/daemon/watch/skip.go), or a strings.HasSuffix argument does not +// trip it. +// +// # What it catches +// +// + ".tmp" // concatenated string literal +// + tmpSuffix // concatenated CONSTANT whose value ends .tmp +// fmt.Sprintf("%s.tmp", ) // format string ending .tmp +// +// # What it does NOT catch — read this before trusting it +// +// This guard ends ONE FAMILY of spellings, not the class. A scan of the tree at +// the time of writing found zero live instances of any of the following, so +// none of them is exploited today, but all of them are the same bug: +// +// - A DIFFERENT SUFFIX: `path + ".partial"`, `path + ".new"`, `path + ".swp"`. +// Identical hazard; the guard keys on ".tmp" only. Widening the suffix set +// is easy but produces false positives on non-temp uses, so it is left out +// deliberately rather than by oversight. +// - A FIXED NAME IN A SHARED DIRECTORY: +// `filepath.Join(filepath.Dir(path), "state.tmp")`. No concatenation with +// the destination at all, yet every writer into that directory collides. +// This is the most likely way the class re-enters, and the AST cannot +// distinguish it from a legitimate fixed filename without knowing the +// writer's intent. +// - Any temp path assembled across statements, through a helper, or from a +// non-constant variable. +// +// New code should use atomicfile.WriteFile. A green run of this test means no +// NEW instance of the three caught spellings, not that the tree is free of +// deterministic temp names. + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "runtime" + "sort" + "strconv" + "strings" + "testing" +) + +// notYetConverted lists the files that STILL build a deterministic temp name, +// as of #6018's first conversion pass. +// +// This list is a debt ledger, not a policy. It must only ever SHRINK: every +// follow-up that converts a file deletes its entry, and when the last entry +// goes the map should be deleted along with the allowlist logic. Nothing may be +// added to it — a new offender means new code took the unsafe shape, which is +// exactly what this test exists to stop. +// +// Why these were not converted in the first pass. The ordering rule is +// EXPOSURE, not convenience: anything written by more than one process, or read +// cross-process to locate something else, goes first regardless of how awkward +// the conversion is. +// +// - internal/graph/graph.go — only WriteAtomic remains. It encodes the WHOLE +// graph Document straight into the open file with json.NewEncoder. +// Converting it means json.Marshal into a []byte first, which doubles peak +// memory on the single largest object in the process. That is a memory +// argument, not a "it streams" one, and it is why this needs a +// writer-callback variant of the helper rather than a drop-in. +// (writeJSONAtomic in the same file WAS converted — it encoded one small +// in-memory sidecar value, so the streaming framing there was incidental.) +// - internal/graph/fbwriter/* genuinely stream: they build the flatbuffer +// incrementally against an open handle and never hold the payload as one +// []byte. Same follow-up as WriteAtomic. +// - internal/install/*, internal/cli/register.go, internal/agentpatterns/* are +// install-time / interactive single-writer paths. +// - internal/enrichment/*, internal/dashboard/*, internal/embed, +// internal/indexer/diff, internal/agents and internal/graph/manifest are +// plausible follow-ups but were left out to keep the first commit +// reviewable. None is written by more than one process today. +var notYetConverted = map[string]bool{ + "internal/agentpatterns/config.go": true, + "internal/agentpatterns/patterns.go": true, + "internal/agentpatterns/sync.go": true, + "internal/agents/inject_map.go": true, + "internal/cli/pendingtools.go": true, + "internal/cli/register.go": true, + "internal/dashboard/handlers_enrichment_writeback.go": true, + "internal/dashboard/handlers_mcp_setup.go": true, + "internal/dashboard/handlers_settings.go": true, + "internal/embed/store.go": true, + "internal/enrichment/candidates.go": true, + "internal/enrichment/docgen_repair.go": true, + "internal/graph/fbwriter/segmented.go": true, + "internal/graph/fbwriter/streaming.go": true, + "internal/graph/fbwriter/writer.go": true, + "internal/graph/graph.go": true, + "internal/graph/manifest.go": true, + "internal/indexer/diff/diff.go": true, + "internal/install/agenthooks/claudecode.go": true, + "internal/install/mcpreg/mcpreg.go": true, + "internal/install/mcpreg/toml.go": true, + "internal/install/mcptools/mcptools.go": true, + "internal/install/rulesfiles/rulesfiles.go": true, + "internal/install/state.go": true, +} + +func TestNoDeterministicTempNames(t *testing.T) { + root := repoRoot(t) + offenders := scanDeterministicTempNames(t, root) + + // 1. Nothing outside the ledger may use the shape. + var unexpected []string + seen := map[string]bool{} + for _, o := range offenders { + seen[o.file] = true + if !notYetConverted[o.file] { + unexpected = append(unexpected, o.String()) + } + } + sort.Strings(unexpected) + if len(unexpected) > 0 { + t.Errorf("deterministic temp names (#6018) in non-allowlisted files:\n %s\n\n"+ + "A temp path built as +\".tmp\" is shared by every writer aiming at that\n"+ + "destination: concurrent writers O_TRUNC and interleave into one temp inode and\n"+ + "each renames a torn file into place. Use atomicfile.WriteFile instead.\n"+ + "Do NOT add the file to notYetConverted — that list only shrinks.", + strings.Join(unexpected, "\n ")) + } + + // 2. The ledger must not rot: an entry that no longer offends is a + // conversion someone forgot to record, and leaving it in would keep the + // guard permanently blind to that file. + var stale []string + for f := range notYetConverted { + if !seen[f] { + stale = append(stale, f) + } + } + sort.Strings(stale) + if len(stale) > 0 { + t.Errorf("notYetConverted lists files that no longer build a deterministic temp name:\n %s\n\n"+ + "Delete these entries — the guard is blind to any file left on the list.", + strings.Join(stale, "\n ")) + } +} + +// TestGuardDetectsTheShape is the guard's own guard: it runs the SAME detector +// used above over a synthetic source file containing the offending shape, and +// over one that uses atomicfile. If the detector ever stops binding — a +// refactor that silently matches nothing is the failure mode this codebase +// keeps hitting — this fails even though the scan above would be vacuously +// green. +func TestGuardDetectsTheShape(t *testing.T) { + cases := []struct { + name string + src string + want int + }{ + { + name: "direct concat", + src: `package p +import "os" +func f(path string, b []byte) error { + tmp := path + ".tmp" + if err := os.WriteFile(tmp, b, 0o644); err != nil { return err } + return os.Rename(tmp, path) +}`, + want: 1, + }, + { + name: "concat inside a call", + src: `package p +import ("os"; "path/filepath") +func f(dir, name string) { _ = os.Remove(filepath.Join(dir, name+".tmp")) }`, + want: 1, + }, + { + name: "two offenders in one file", + src: `package p +func f(a, b string) (string, string) { return a + ".tmp", b + ".tmp" }`, + want: 2, + }, + { + name: "map key is not a concat", + src: `package p +var skip = map[string]struct{}{".tmp": {}}`, + want: 0, + }, + { + name: "comment mentioning the shape", + src: `package p +// NOT a fixed path+".tmp": see #6018. +func f() {}`, + want: 0, + }, + { + name: "suffix comparison is not a concat", + src: `package p +import "strings" +func f(n string) bool { return strings.HasSuffix(n, ".tmp") }`, + want: 0, + }, + { + name: "atomicfile call is clean", + src: `package p +import "github.com/cajasmota/grafel/internal/atomicfile" +func f(path string, b []byte) error { return atomicfile.WriteFile(path, b, 0o644) }`, + want: 0, + }, + { + name: "unique CreateTemp pattern is clean", + src: `package p +import ("os"; "path/filepath") +func f(path string) { _, _ = os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp-*") }`, + want: 0, + }, + { + name: "fmt.Sprintf format ending .tmp", + src: `package p +import "fmt" +func f(path string) string { return fmt.Sprintf("%s.tmp", path) }`, + want: 1, + }, + { + name: "Sprintf without a trailing .tmp is clean", + src: `package p +import "fmt" +func f(path string) string { return fmt.Sprintf("%s.tmp-%d", path, 7) }`, + want: 0, + }, + { + name: "Sprintf on another package is not fmt", + src: `package p +import "log" +func f(path string) { log.Sprintf("%s.tmp", path) }`, + want: 0, + }, + { + name: "file-local const suffix", + src: `package p +const tmpSuffix = ".tmp" +func f(path string) string { return path + tmpSuffix }`, + want: 1, + }, + { + name: "const not ending in .tmp is clean", + src: `package p +const suffix = ".json" +func f(path string) string { return path + suffix }`, + want: 0, + }, + + // --- KNOWN MISSES ------------------------------------------------- + // These are the same bug and the detector does NOT catch them. They are + // asserted at want:0 so the file states its real reach rather than + // implying total coverage. If a future change starts catching one of + // these, flip its want to 1 and delete the corresponding paragraph from + // the file doc — do not delete the row. + { + name: "MISS: a different suffix", + src: `package p +func f(path string) string { return path + ".partial" }`, + want: 0, + }, + { + name: "MISS: fixed name in a shared directory", + src: `package p +import "path/filepath" +func f(path string) string { return filepath.Join(filepath.Dir(path), "state.tmp") }`, + want: 0, + }, + { + name: "MISS: suffix const declared in another file of the package", + src: `package p +func f(path string) string { return path + suffixFromElsewhere }`, + want: 0, + }, + { + name: "MISS: suffix reaching the concat through a variable", + src: `package p +func f(path string) string { s := ".tmp"; return path + s }`, + want: 0, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "x.go", tc.src, parser.SkipObjectResolution) + if err != nil { + t.Fatalf("parse: %v", err) + } + got := len(findDeterministicTempNames(fset, f, "x.go")) + if got != tc.want { + t.Fatalf("detector found %d offenders, want %d", got, tc.want) + } + }) + } +} + +type tempNameOffence struct { + file string // repo-relative + line int +} + +func (o tempNameOffence) String() string { return o.file + ":" + strconv.Itoa(o.line) } + +// findDeterministicTempNames reports the three caught spellings in f — see the +// file doc for the full catch/miss list. +// +// Matching AST shapes rather than text is what keeps comments, map keys and +// strings.HasSuffix arguments out of the results. +// +// Constant resolution is FILE-LOCAL: a `const tmpSuffix = ".tmp"` declared in +// another file of the same package is not resolved, because the scan parses one +// file at a time and does not run go/types. That is a known hole in a spelling +// that is itself already unusual. +func findDeterministicTempNames(fset *token.FileSet, f *ast.File, rel string) []tempNameOffence { + consts := stringConsts(f) + var out []tempNameOffence + report := func(n ast.Node) { + out = append(out, tempNameOffence{file: rel, line: fset.Position(n.Pos()).Line}) + } + ast.Inspect(f, func(n ast.Node) bool { + switch e := n.(type) { + case *ast.BinaryExpr: + // + ".tmp" and + tmpSuffix + if e.Op != token.ADD { + return true + } + if s, ok := constStringValue(e.Y, consts); ok && strings.HasSuffix(s, ".tmp") { + report(e) + } + case *ast.CallExpr: + // fmt.Sprintf("%s.tmp", ) + sel, ok := e.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Sprintf" { + return true + } + pkg, ok := sel.X.(*ast.Ident) + if !ok || pkg.Name != "fmt" || len(e.Args) < 2 { + return true + } + if s, ok := constStringValue(e.Args[0], consts); ok && strings.HasSuffix(s, ".tmp") { + report(e) + } + } + return true + }) + return out +} + +// stringConsts collects file-local `const name = "literal"` bindings. +func stringConsts(f *ast.File) map[string]string { + out := map[string]string{} + for _, d := range f.Decls { + gd, ok := d.(*ast.GenDecl) + if !ok || gd.Tok != token.CONST { + continue + } + for _, spec := range gd.Specs { + vs, ok := spec.(*ast.ValueSpec) + if !ok { + continue + } + for i, name := range vs.Names { + if i >= len(vs.Values) { + continue + } + lit, ok := vs.Values[i].(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + continue + } + if s, err := strconv.Unquote(lit.Value); err == nil { + out[name.Name] = s + } + } + } + } + return out +} + +// constStringValue resolves e to a string when it is a string literal or an +// identifier bound to a file-local string constant. +func constStringValue(e ast.Expr, consts map[string]string) (string, bool) { + switch v := e.(type) { + case *ast.BasicLit: + if v.Kind != token.STRING { + return "", false + } + s, err := strconv.Unquote(v.Value) + return s, err == nil + case *ast.Ident: + s, ok := consts[v.Name] + return s, ok + } + return "", false +} + +func scanDeterministicTempNames(t *testing.T, root string) []tempNameOffence { + t.Helper() + var out []tempNameOffence + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + switch d.Name() { + case ".git", "node_modules", "vendor", "testdata", "dist", "build": + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + rel, rerr := filepath.Rel(root, path) + if rerr != nil { + return rerr + } + rel = filepath.ToSlash(rel) + fset := token.NewFileSet() + f, perr := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + if perr != nil { + // Unparseable non-test source would fail the build anyway; do not + // let it silently shrink the scan. + t.Fatalf("parse %s: %v", rel, perr) + } + out = append(out, findDeterministicTempNames(fset, f, rel)...) + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", root, err) + } + // A scan that finds nothing at all means the walk stopped binding the tree. + if len(out) == 0 && len(notYetConverted) > 0 { + t.Fatalf("scan of %s found no Go source using the shape at all — the walk is not "+ + "binding the repository (expected at least the notYetConverted entries)", root) + } + return out +} + +func repoRoot(t *testing.T) string { + t.Helper() + _, here, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("runtime.Caller failed") + } + // here = /internal/atomicfile/guard_6018_test.go + return filepath.Clean(filepath.Join(filepath.Dir(here), "..", "..")) +} diff --git a/internal/atomicfile/umask_unix_test.go b/internal/atomicfile/umask_unix_test.go new file mode 100644 index 000000000..e083bfdbf --- /dev/null +++ b/internal/atomicfile/umask_unix_test.go @@ -0,0 +1,18 @@ +//go:build !windows + +package atomicfile_test + +import ( + "syscall" + "testing" +) + +// setUmask sets the process umask and returns the previous value. +// +// This is process-global and racy by nature, which is exactly why the helper +// itself never reads it (#5978). It is safe here only because the tests in +// this package do not run in parallel with each other. +func setUmask(t *testing.T, mask int) int { + t.Helper() + return syscall.Umask(mask) +} diff --git a/internal/atomicfile/umask_windows_test.go b/internal/atomicfile/umask_windows_test.go new file mode 100644 index 000000000..d1cb70f71 --- /dev/null +++ b/internal/atomicfile/umask_windows_test.go @@ -0,0 +1,12 @@ +//go:build windows + +package atomicfile_test + +import "testing" + +// setUmask has no meaning on Windows; the umask test is skipped there. +func setUmask(t *testing.T, mask int) int { + t.Helper() + t.Skip("umask is not a Windows concept") + return 0 +} diff --git a/internal/daemon/algo/cache.go b/internal/daemon/algo/cache.go index f702b56cf..6cf34005a 100644 --- a/internal/daemon/algo/cache.go +++ b/internal/daemon/algo/cache.go @@ -24,6 +24,7 @@ import ( "sync" "time" + "github.com/cajasmota/grafel/internal/atomicfile" "github.com/cajasmota/grafel/internal/graph" ) @@ -222,13 +223,10 @@ func writeToDisk(stateDir string, r *Results) error { } cachePath := filepath.Join(stateDir, cacheFileName) - tmp := cachePath + ".tmp" - if err := os.WriteFile(tmp, data, 0o644); err != nil { - return fmt.Errorf("write tmp: %w", err) - } - if err := os.Rename(tmp, cachePath); err != nil { - _ = os.Remove(tmp) - return fmt.Errorf("rename: %w", err) + // #6018: unique temp name. A torn cache here is worse than a missing one — + // it is read back as a valid envelope on the next load. + if err := atomicfile.WriteFile(cachePath, data, 0o644); err != nil { + return fmt.Errorf("write %s: %w", cachePath, err) } return nil } diff --git a/internal/daemon/dead_letters.go b/internal/daemon/dead_letters.go index 36a35ee01..33ff0eb2c 100644 --- a/internal/daemon/dead_letters.go +++ b/internal/daemon/dead_letters.go @@ -8,6 +8,7 @@ import ( "sort" "time" + "github.com/cajasmota/grafel/internal/atomicfile" "github.com/cajasmota/grafel/internal/daemon/requests" ) @@ -68,11 +69,9 @@ func recordDeadLetter(root string, dl DeadLetter) error { return err } final := filepath.Join(dir, dl.ID+".json") - tmp := final + ".tmp" - if err := os.WriteFile(tmp, data, 0o644); err != nil { - return err - } - return os.Rename(tmp, final) + // #6018: unique temp name — dead letters are recorded from several + // concurrent daemon workers. + return atomicfile.WriteFile(final, data, 0o644) } // ReadDeadLetters returns every dead-letter record under root, newest first. A diff --git a/internal/daemon/mode/mode.go b/internal/daemon/mode/mode.go index afd1f5a69..46c3dc758 100644 --- a/internal/daemon/mode/mode.go +++ b/internal/daemon/mode/mode.go @@ -17,6 +17,8 @@ import ( "fmt" "os" "path/filepath" + + "github.com/cajasmota/grafel/internal/atomicfile" ) // Mode is one of the three operational presets. @@ -134,15 +136,13 @@ func SaveConfig(path string, cfg Config) error { if err != nil { return fmt.Errorf("marshal daemon config: %w", err) } - tmp := path + ".tmp" if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { return fmt.Errorf("mkdir %s: %w", filepath.Dir(path), err) } - if err := os.WriteFile(tmp, data, 0o600); err != nil { - return fmt.Errorf("write daemon config tmp: %w", err) - } - if err := os.Rename(tmp, path); err != nil { - return fmt.Errorf("rename daemon config: %w", err) + // #6018: unique temp name — the daemon config is written by CLI processes + // and by the daemon itself, with no cross-process lock. + if err := atomicfile.WriteFile(path, data, 0o600); err != nil { + return fmt.Errorf("write daemon config %s: %w", path, err) } return nil } diff --git a/internal/daemon/pins.go b/internal/daemon/pins.go index 83b5ac52b..3dfaf8eb3 100644 --- a/internal/daemon/pins.go +++ b/internal/daemon/pins.go @@ -21,6 +21,8 @@ import ( "path/filepath" "sync" "time" + + "github.com/cajasmota/grafel/internal/atomicfile" ) // PinRecord is a single user-managed pin entry. @@ -88,11 +90,8 @@ func (s *PinStore) save() error { if err != nil { return err } - tmp := s.path + ".tmp" - if err := os.WriteFile(tmp, data, 0o600); err != nil { - return err - } - return os.Rename(tmp, s.path) + // #6018: unique temp name (s.mu only serialises writers inside ONE process). + return atomicfile.WriteFile(s.path, data, 0o600) } // Pin adds a pin for (group, repo, ref). Idempotent — re-pinning an already diff --git a/internal/daemon/root_manifest.go b/internal/daemon/root_manifest.go index 10a42e84f..d4941c217 100644 --- a/internal/daemon/root_manifest.go +++ b/internal/daemon/root_manifest.go @@ -35,6 +35,8 @@ import ( "encoding/json" "os" "path/filepath" + + "github.com/cajasmota/grafel/internal/atomicfile" ) // RootManifestName is the basename of the per-root source-path manifest written @@ -97,15 +99,9 @@ func WriteRootManifest(repoPath string) error { return err } dst := filepath.Join(root, RootManifestName) - tmp := dst + ".tmp" - if err := os.WriteFile(tmp, data, 0o644); err != nil { - return err - } - if err := os.Rename(tmp, dst); err != nil { - _ = os.Remove(tmp) - return err - } - return nil + // #6018: unique temp name — several repos under one store root can be + // (re)registered concurrently. + return atomicfile.WriteFile(dst, data, 0o644) } // ReadRootManifest reads `/root.json` for the given top-level store-root diff --git a/internal/daemon/sched/rss.go b/internal/daemon/sched/rss.go index 865422bbc..d98c2de3d 100644 --- a/internal/daemon/sched/rss.go +++ b/internal/daemon/sched/rss.go @@ -9,6 +9,7 @@ import ( "sync" "time" + "github.com/cajasmota/grafel/internal/atomicfile" "github.com/cajasmota/grafel/internal/daemon/walk" ) @@ -120,13 +121,13 @@ func (h *RSSHistory) Record(repoPath string, peakMB int64) { } prev.LastIndex = time.Now().UTC() h.data[repoPath] = prev - tmp := h.path + ".tmp" b, _ := json.MarshalIndent(h.data, "", " ") h.mu.Unlock() if h.path == "" { return } - if err := os.WriteFile(tmp, b, 0o600); err == nil { - _ = os.Rename(tmp, h.path) - } + // #6018: unique temp name. Errors stay deliberately discarded — this is a + // best-effort budget calibration and a miss only costs a re-measure — but a + // TORN history file would be read back as truth on the next start. + _ = atomicfile.WriteFile(h.path, b, 0o600) } diff --git a/internal/daemon/watch/quarantine.go b/internal/daemon/watch/quarantine.go index 29e845894..d6b701aea 100644 --- a/internal/daemon/watch/quarantine.go +++ b/internal/daemon/watch/quarantine.go @@ -53,6 +53,8 @@ import ( "strconv" "sync" "time" + + "github.com/cajasmota/grafel/internal/atomicfile" ) // Quarantine tuning. All values are env-overridable (see quarantineConfig). @@ -654,11 +656,9 @@ func writeQuarantineFile(repo string, dirs []QuarantineReason) error { if err != nil { return err } - tmp := quarantinePath(repo) + ".tmp" - if err := os.WriteFile(tmp, data, 0o644); err != nil { - return err - } - return os.Rename(tmp, quarantinePath(repo)) + // #6018: unique temp name — the tracker (persistLocked) and this free + // function both target the same destination from different goroutines. + return atomicfile.WriteFile(quarantinePath(repo), data, 0o644) } // UnquarantineFile removes rel from a repo's persisted quarantine set. Returns @@ -769,13 +769,12 @@ func (q *QuarantineTracker) persistLocked(repo string) { if err != nil { return } - // Atomic-ish: write temp then rename. - tmp := quarantinePath(repo) + ".tmp" - if err := os.WriteFile(tmp, data, 0o644); err != nil { + // Atomic: unique temp then rename (#6018 — the temp name used to be + // quarantinePath(repo)+".tmp", shared with writeQuarantineFile above). + if err := atomicfile.WriteFile(quarantinePath(repo), data, 0o644); err != nil { if q.audit != nil { q.audit("persist-error", repo, "", err.Error()) } return } - _ = os.Rename(tmp, quarantinePath(repo)) } diff --git a/internal/daemon/worktree/persist_retry_test.go b/internal/daemon/worktree/persist_retry_test.go index 68615d0d7..b3abd6e73 100644 --- a/internal/daemon/worktree/persist_retry_test.go +++ b/internal/daemon/worktree/persist_retry_test.go @@ -1,6 +1,7 @@ package worktree import ( + "errors" "os" "path/filepath" "testing" @@ -8,10 +9,14 @@ import ( ) // TestStoreSave_RetriesTransientTmpFailure verifies the persist path recovers -// from a transient `.tmp` write failure by retrying once (#5675), rather than -// dropping the reconcile. We simulate the transient failure by pre-creating the -// `.tmp` path as a directory: the first os.WriteFile fails ("is a directory"), -// the retry removes the stale entry and succeeds. +// from a transient atomic-write failure by retrying once (#5675), rather than +// dropping the reconcile. +// +// The transient failure is injected through the writeStoreFile seam: the first +// call fails, the second delegates to the real atomicfile.WriteFile. Before +// #6018 the test seeded `path+".tmp"` as a directory to make the first +// os.WriteFile fail — that only worked because the staging name was +// deterministic, which is the bug #6018 removed. // // It also asserts the path is NEVER fatal — save() only ever returns an error; // nothing here exits the process. @@ -30,16 +35,23 @@ func TestStoreSave_RetriesTransientTmpFailure(t *testing.T) { Status: StatusActive, }} - // Inject a transient failure: the staging path exists as a directory, so - // the first WriteFile fails; the retry's os.Remove clears it. - tmp := path + ".tmp" - if err := os.Mkdir(tmp, 0o755); err != nil { - t.Fatalf("seed tmp dir: %v", err) + real := writeStoreFile + t.Cleanup(func() { writeStoreFile = real }) + calls := 0 + writeStoreFile = func(p string, b []byte, perm os.FileMode) error { + calls++ + if calls == 1 { + return errors.New("transient I/O blip") + } + return real(p, b, perm) } if err := s.save(); err != nil { t.Fatalf("save() should recover via retry, got error: %v", err) } + if calls != 2 { + t.Fatalf("writeStoreFile called %d times, want 2 (one failure + one retry)", calls) + } // The real store file must now exist with valid JSON content. data, err := os.ReadFile(path) @@ -49,9 +61,40 @@ func TestStoreSave_RetriesTransientTmpFailure(t *testing.T) { if len(data) == 0 { t.Fatal("store file is empty after save") } - // The staging file must have been renamed away. - if _, err := os.Stat(tmp); !os.IsNotExist(err) { - t.Errorf("tmp staging path still present after save: %v", err) + // No staging file may survive in the destination directory. + ents, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + for _, e := range ents { + if e.Name() != filepath.Base(path) { + t.Errorf("leftover staging entry after save: %s", e.Name()) + } + } +} + +// TestStoreSave_ReturnsErrorWhenBothAttemptsFail pins that the retry is only +// ONE retry: a persistent failure still surfaces as an error rather than +// looping or being swallowed. +func TestStoreSave_ReturnsErrorWhenBothAttemptsFail(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "worktrees.json") + s := NewStore(path) + s.children = []*WorktreeChild{{ParentSlug: "r", GroupName: "g", Path: "/w", Status: StatusActive}} + + real := writeStoreFile + t.Cleanup(func() { writeStoreFile = real }) + calls := 0 + writeStoreFile = func(string, []byte, os.FileMode) error { + calls++ + return errors.New("persistent failure") + } + + if err := s.save(); err == nil { + t.Fatal("save() with a persistent write failure: want error, got nil") + } + if calls != 2 { + t.Fatalf("writeStoreFile called %d times, want exactly 2", calls) } } diff --git a/internal/daemon/worktree/worktree.go b/internal/daemon/worktree/worktree.go index e47b852d0..e3d1b1fb2 100644 --- a/internal/daemon/worktree/worktree.go +++ b/internal/daemon/worktree/worktree.go @@ -60,6 +60,7 @@ import ( "sync" "time" + "github.com/cajasmota/grafel/internal/atomicfile" "github.com/cajasmota/grafel/internal/executil" "github.com/fsnotify/fsnotify" ) @@ -177,13 +178,23 @@ func (s *Store) Load() error { return nil } +// writeStoreFile is the store's atomic-write seam. It is a var solely so the +// #5675 retry-once can be tested with an injected transient failure: since +// #6018 the staging file has a unique name chosen inside atomicfile, so a test +// can no longer seed a known temp path to make the first attempt fail. +// +// It is a bare var with no mutex, and Store.save() reads it from the daemon's +// reconcile goroutines: NOT SAFE TO SWAP CONCURRENTLY. Tests that swap it must +// not be parallel with each other or with anything that persists a Store. +var writeStoreFile = atomicfile.WriteFile + // save writes the store atomically (caller must hold s.mu). // -// It is resilient to a transient failure writing the `.tmp` staging file -// (#5675): the daemon persists on every reconciliation tick, so a momentary -// fd-exhaustion or I/O blip must not drop the whole reconcile. The `.tmp` -// write is retried ONCE before giving up. The caller keeps this NON-FATAL — -// nothing here ever calls os.Exit/log.Fatal. +// It is resilient to a transient failure writing the staging file (#5675): the +// daemon persists on every reconciliation tick, so a momentary fd-exhaustion or +// I/O blip must not drop the whole reconcile. The atomic write is retried ONCE +// before giving up. The caller keeps this NON-FATAL — nothing here ever calls +// os.Exit/log.Fatal. func (s *Store) save() error { if err := os.MkdirAll(filepath.Dir(s.path), 0o755); err != nil { return err @@ -192,17 +203,21 @@ func (s *Store) save() error { if err != nil { return err } - tmp := s.path + ".tmp" - if err := os.WriteFile(tmp, data, 0o644); err != nil { - // Retry once: a transient fd/I-O blip during a storm should not drop - // the reconcile silently. A best-effort remove of a possibly - // partially-written tmp precedes the retry. - _ = os.Remove(tmp) - if err2 := os.WriteFile(tmp, data, 0o644); err2 != nil { - return err2 - } + // #6018: unique temp name via atomicfile (the staging file used to be + // s.path+".tmp", shared by every writer aiming at this store). + // + // The retry-once from #5675 is kept: a transient fd/I-O blip during a storm + // must not drop the whole reconcile. atomicfile removes its own temp file on + // every error path, so the retry needs no cleanup of its own — that cleanup + // is covered by atomicfile's TestWriteFile_RemovesTempOnError, NOT by the + // retry test here, which injects its failure through the writeStoreFile seam + // before any temp file exists. The retry now covers the rename as well as + // the staging write, which the split form did not — both are the same + // transient class. + if err := writeStoreFile(s.path, data, 0o644); err != nil { + return writeStoreFile(s.path, data, 0o644) } - return os.Rename(tmp, s.path) + return nil } // All returns a snapshot of all children (active and expired). diff --git a/internal/docgen/llm_cache.go b/internal/docgen/llm_cache.go index 08017d6bb..bb19d18fe 100644 --- a/internal/docgen/llm_cache.go +++ b/internal/docgen/llm_cache.go @@ -21,6 +21,8 @@ import ( "os" "path/filepath" "time" + + "github.com/cajasmota/grafel/internal/atomicfile" ) // CacheEntry is the on-disk value stored for one section result. @@ -80,13 +82,10 @@ func WriteCache(cacheDir string, entry CacheEntry) error { return fmt.Errorf("marshal cache entry: %w", err) } target := cacheFilePath(cacheDir, entry.PromptHash) - tmp := target + ".tmp" - if err := os.WriteFile(tmp, data, 0o644); err != nil { - return fmt.Errorf("write tmp cache file %q: %w", tmp, err) - } - if err := os.Rename(tmp, target); err != nil { - _ = os.Remove(tmp) // best-effort cleanup - return fmt.Errorf("rename cache file %q→%q: %w", tmp, target, err) + // #6018: unique temp name — docgen passes write this cache from concurrent + // workers, and two passes computing the same prompt hash aim at one path. + if err := atomicfile.WriteFile(target, data, 0o644); err != nil { + return fmt.Errorf("write cache file %q: %w", target, err) } return nil } diff --git a/internal/graph/genpath.go b/internal/graph/genpath.go index a3565f521..e6015d7ff 100644 --- a/internal/graph/genpath.go +++ b/internal/graph/genpath.go @@ -35,6 +35,8 @@ import ( "strconv" "strings" "time" + + "github.com/cajasmota/grafel/internal/atomicfile" ) // currentPointerName is the tiny pointer file naming the active generation. @@ -380,57 +382,65 @@ var ( ) // WriteCurrentPointer atomically points /current at genName (a bare -// graph..fb filename) via a sibling .tmp + rename. The pointer file is -// tiny and NEVER memory-mapped, so this rename-over is safe on every platform -// (it is not the ERROR_USER_MAPPED_FILE hazard the gen layout removes); a -// bounded retry absorbs the transient Windows reader-open race. +// graph..fb filename) via a unique sibling temp + rename. The pointer file +// is tiny and NEVER memory-mapped, so this rename-over is safe on every +// platform (it is not the ERROR_USER_MAPPED_FILE hazard the gen layout +// removes); a bounded retry absorbs the transient Windows reader-open race. func WriteCurrentPointer(dir, genName string) error { if _, ok := parseGen(genName); !ok { return fmt.Errorf("graph.WriteCurrentPointer: invalid gen name %q", genName) } - tmp := filepath.Join(dir, currentPointerName+".tmp") - if err := os.WriteFile(tmp, []byte(genName+"\n"), 0o644); err != nil { - return fmt.Errorf("graph.WriteCurrentPointer: write tmp: %w", err) + if err := flipCurrentPointer(dir, genName); err != nil { + return fmt.Errorf("graph.WriteCurrentPointer: %w", err) } + return nil +} + +// flipCurrentPointer writes name into /current atomically, retrying the +// whole write+rename up to pointerFlipRetries times. +// +// #6018: the staging file used to be a FIXED filepath.Join(dir, "current.tmp") +// in the shared gen directory, so the indexer publishing a graph while anything +// else flipped the same pointer tore one temp inode and could rename a garbled +// gen name into `current` — after which the daemon and MCP, which read this +// pointer cross-process to locate the current graph, resolve to a nonexistent +// or wrong generation. atomicfile gives each writer its own temp. +// +// The retry now covers the staging write as well as the rename, where the +// split form retried only the rename: atomicfile does both in one call and does +// not report which half failed. A hard failure (missing or unwritable dir) +// therefore costs pointerFlipRetries*pointerFlipRetryDelay (~200ms) before it +// surfaces, instead of failing immediately. That is irrelevant next to a +// publish that cannot complete at all. +func flipCurrentPointer(dir, name string) error { dst := filepath.Join(dir, currentPointerName) var err error for i := 0; i < pointerFlipRetries; i++ { - if err = os.Rename(tmp, dst); err == nil { + if err = atomicfile.WriteFile(dst, []byte(name+"\n"), 0o644); err == nil { return nil } time.Sleep(pointerFlipRetryDelay) } - os.Remove(tmp) - return fmt.Errorf("graph.WriteCurrentPointer: rename: %w", err) + return fmt.Errorf("flip pointer: %w", err) } // WriteCurrentPointerRaw atomically points /current at name, where name // may be EITHER a single-file gen ("graph..fb") OR a segment-set gen dir // ("graph.") or its manifest ("graph./manifest.json"). It is the // segment-aware superset of WriteCurrentPointer (which only accepts the -// single-file shape); the two share the same atomic tmp+rename + bounded retry. -// This is a format primitive for the FUTURE streaming writer (#5902) and the -// test fixtures — no existing producer calls it. +// single-file shape); the two share the same atomic write + bounded retry via +// flipCurrentPointer. This is a format primitive for the FUTURE streaming +// writer (#5902) and the test fixtures — no existing producer calls it. func WriteCurrentPointerRaw(dir, name string) error { _, single := parseGen(name) _, seg := parseSegPointer(name) if !single && !seg { return fmt.Errorf("graph.WriteCurrentPointerRaw: invalid pointer target %q", name) } - tmp := filepath.Join(dir, currentPointerName+".tmp") - if err := os.WriteFile(tmp, []byte(name+"\n"), 0o644); err != nil { - return fmt.Errorf("graph.WriteCurrentPointerRaw: write tmp: %w", err) + if err := flipCurrentPointer(dir, name); err != nil { + return fmt.Errorf("graph.WriteCurrentPointerRaw: %w", err) } - dst := filepath.Join(dir, currentPointerName) - var err error - for i := 0; i < pointerFlipRetries; i++ { - if err = os.Rename(tmp, dst); err == nil { - return nil - } - time.Sleep(pointerFlipRetryDelay) - } - os.Remove(tmp) - return fmt.Errorf("graph.WriteCurrentPointerRaw: rename: %w", err) + return nil } // GCStaleGens best-effort unlinks generation files older than the @@ -524,13 +534,11 @@ func WriteGenGraph(dir string, buf []byte) (genPath string, err error) { gen := NextGen(dir) name := GenFileName(gen) genPath = filepath.Join(dir, name) - tmp := genPath + ".tmp" - if err := os.WriteFile(tmp, buf, 0o644); err != nil { - return "", fmt.Errorf("graph.WriteGenGraph: write tmp: %w", err) - } - if err := os.Rename(tmp, genPath); err != nil { - os.Remove(tmp) - return "", fmt.Errorf("graph.WriteGenGraph: rename gen: %w", err) + // #6018: unique temp name. buf is already fully in memory, so this is a + // drop-in; two indexers landing on the same gen number would otherwise share + // one staging inode and publish a torn graph file. + if err := atomicfile.WriteFile(genPath, buf, 0o644); err != nil { + return "", fmt.Errorf("graph.WriteGenGraph: write gen: %w", err) } if err := WriteCurrentPointer(dir, name); err != nil { return "", fmt.Errorf("graph.WriteGenGraph: flip pointer: %w", err) diff --git a/internal/graph/graph.go b/internal/graph/graph.go index d798563d1..361795e75 100644 --- a/internal/graph/graph.go +++ b/internal/graph/graph.go @@ -13,6 +13,7 @@ import ( "sort" "time" + "github.com/cajasmota/grafel/internal/atomicfile" "github.com/cajasmota/grafel/internal/types" ) @@ -576,27 +577,31 @@ func WriteSidecar(outPath string, side *GraphStatsSidecar, pretty bool) error { return writeJSONAtomic(filepath.Join(dir, "graph-stats.json"), side, pretty) } -// writeJSONAtomic encodes v to target via a sibling .tmp + rename. +// writeJSONAtomic encodes v to target via a unique sibling temp + rename +// (#6018 — the temp used to be a fixed target+".tmp"). +// +// It used json.NewEncoder against the open temp file, but v is a single +// in-memory value, so this is a marshal-then-write, not a stream. The trailing +// newline that json.Encoder.Encode appends and json.Marshal does not is added +// back explicitly so the output stays byte-identical. func writeJSONAtomic(target string, v any, pretty bool) error { - tmp := target + ".tmp" - f, err := os.Create(tmp) - if err != nil { - return fmt.Errorf("graph: create sidecar tmp: %w", err) - } - enc := json.NewEncoder(f) + var ( + b []byte + err error + ) if pretty { - enc.SetIndent("", " ") + b, err = json.MarshalIndent(v, "", " ") + } else { + b, err = json.Marshal(v) } - if err := enc.Encode(v); err != nil { - f.Close() - os.Remove(tmp) + if err != nil { return fmt.Errorf("graph: encode sidecar: %w", err) } - if err := f.Close(); err != nil { - os.Remove(tmp) - return err + b = append(b, '\n') + if err := atomicfile.WriteFile(target, b, 0o644); err != nil { + return fmt.Errorf("graph: write sidecar %s: %w", target, err) } - return os.Rename(tmp, target) + return nil } // SidecarPath returns the graph-stats.json path inside stateDir. diff --git a/internal/graph/sidecar_bytes_6018_test.go b/internal/graph/sidecar_bytes_6018_test.go new file mode 100644 index 000000000..8449d861c --- /dev/null +++ b/internal/graph/sidecar_bytes_6018_test.go @@ -0,0 +1,123 @@ +package graph + +// sidecar_bytes_6018_test.go — pins the exact bytes WriteSidecar emits. +// +// #6018 converted writeJSONAtomic from json.NewEncoder(f) to +// json.Marshal + atomicfile.WriteFile. json.Encoder.Encode appends a trailing +// newline and json.Marshal does not, so the conversion had to add one back by +// hand. That is precisely the kind of one-byte difference no existing test +// noticed, hence this one. + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + "time" +) + +func sidecarFixture() *GraphStatsSidecar { + return &GraphStatsSidecar{ + Version: 1, + ComputedAt: time.Date(2026, 7, 28, 12, 0, 0, 0, time.UTC), + TotalEntities: 3, + TotalRelationships: 2, + Communities: 1, + Modularity: 0.5, + } +} + +// TestWriteSidecar_TrailingNewline pins the trailing newline json.Encoder used +// to supply. Deleting the explicit `append(b, '\n')` in writeJSONAtomic fails +// this in both modes. +func TestWriteSidecar_TrailingNewline(t *testing.T) { + for _, pretty := range []bool{false, true} { + name := "minified" + if pretty { + name = "pretty" + } + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + out := filepath.Join(dir, "graph.fb") + if err := WriteSidecar(out, sidecarFixture(), pretty); err != nil { + t.Fatalf("WriteSidecar: %v", err) + } + b, err := os.ReadFile(filepath.Join(dir, "graph-stats.json")) + if err != nil { + t.Fatalf("read sidecar: %v", err) + } + if len(b) == 0 || b[len(b)-1] != '\n' { + t.Fatalf("sidecar does not end in a newline (last 8 bytes: %q)", + b[max(0, len(b)-8):]) + } + if n := len(b); n >= 2 && b[n-2] == '\n' { + t.Fatalf("sidecar ends in TWO newlines; encoder emitted exactly one") + } + }) + } +} + +// TestWriteSidecar_ByteIdenticalToEncoder compares the file against what +// json.Encoder would have produced, which is what shipped before #6018. +func TestWriteSidecar_ByteIdenticalToEncoder(t *testing.T) { + for _, pretty := range []bool{false, true} { + name := "minified" + if pretty { + name = "pretty" + } + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + out := filepath.Join(dir, "graph.fb") + side := sidecarFixture() + if err := WriteSidecar(out, side, pretty); err != nil { + t.Fatalf("WriteSidecar: %v", err) + } + got, err := os.ReadFile(filepath.Join(dir, "graph-stats.json")) + if err != nil { + t.Fatal(err) + } + + // The pre-#6018 form, reproduced verbatim against a scratch file. + ref := filepath.Join(dir, "ref.json") + f, err := os.Create(ref) + if err != nil { + t.Fatal(err) + } + enc := json.NewEncoder(f) + if pretty { + enc.SetIndent("", " ") + } + if err := enc.Encode(side); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + want, err := os.ReadFile(ref) + if err != nil { + t.Fatal(err) + } + + if string(got) != string(want) { + t.Fatalf("sidecar bytes changed.\n got: %q\nwant: %q", got, want) + } + }) + } +} + +// TestWriteSidecar_Perm pins the mode. os.Create used 0666&^umask; the helper +// now applies 0644 verbatim (see the atomicfile package doc on umask). +func TestWriteSidecar_Perm(t *testing.T) { + dir := t.TempDir() + out := filepath.Join(dir, "graph.fb") + if err := WriteSidecar(out, sidecarFixture(), false); err != nil { + t.Fatalf("WriteSidecar: %v", err) + } + fi, err := os.Stat(filepath.Join(dir, "graph-stats.json")) + if err != nil { + t.Fatal(err) + } + if got := fi.Mode().Perm(); got != 0o644 { + t.Fatalf("mode = %04o, want 0644", got) + } +} diff --git a/internal/mcp/docstate.go b/internal/mcp/docstate.go index 429d8d6b8..5ea5b45b0 100644 --- a/internal/mcp/docstate.go +++ b/internal/mcp/docstate.go @@ -20,6 +20,7 @@ import ( "sync" "time" + "github.com/cajasmota/grafel/internal/atomicfile" "github.com/cajasmota/grafel/internal/graph" ) @@ -171,13 +172,10 @@ func SaveDocgenState(group string, st DocgenState) error { return fmt.Errorf("docstate: marshal: %w", err) } path := filepath.Join(dir, "docgen-state.json") - tmp := path + ".tmp" - if err := os.WriteFile(tmp, data, 0o644); err != nil { - return fmt.Errorf("docstate: write tmp: %w", err) - } - if err := os.Rename(tmp, path); err != nil { - _ = os.Remove(tmp) - return fmt.Errorf("docstate: rename: %w", err) + // #6018: unique temp name. Parallel agent sessions write this concurrently, + // and a deterministic path+".tmp" is shared by all of them. + if err := atomicfile.WriteFile(path, data, 0o644); err != nil { + return fmt.Errorf("docstate: write %s: %w", path, err) } return nil } diff --git a/internal/registry/registry.go b/internal/registry/registry.go index 06f91f438..ef5820977 100644 --- a/internal/registry/registry.go +++ b/internal/registry/registry.go @@ -18,6 +18,8 @@ import ( "strings" "sync" "time" + + "github.com/cajasmota/grafel/internal/atomicfile" ) // StackList is a JSON-polymorphic list of language tags for a repo. @@ -297,11 +299,10 @@ func saveTo(path string, r *Registry) error { if err != nil { return err } - tmp := path + ".tmp" - if err := os.WriteFile(tmp, b, 0o644); err != nil { - return err - } - return os.Rename(tmp, path) + // #6018: unique temp name. The registry is written by BOTH the CLI and the + // daemon with no cross-process lock, so a deterministic path+".tmp" is + // shared by concurrent writers and a torn registry is a broken install. + return atomicfile.WriteFile(path, b, 0o644) } // AddGroup adds a group to the registry and persists. Idempotent: if the @@ -381,11 +382,9 @@ func SaveGroupConfig(path string, cfg *GroupConfig) error { if err != nil { return err } - tmp := path + ".tmp" - if err := os.WriteFile(tmp, b, 0o644); err != nil { - return err - } - return os.Rename(tmp, path) + // #6018: unique temp name — same cross-process CLI/daemon exposure as the + // registry itself. + return atomicfile.WriteFile(path, b, 0o644) } // LoadManifest reads a committed teammate manifest from