diff --git a/.abcd/work/issues/open/iss-79-storeoriginal-inline-atomic-write.md b/.abcd/work/issues/open/iss-79-storeoriginal-inline-atomic-write.md new file mode 100644 index 00000000..372fb32e --- /dev/null +++ b/.abcd/work/issues/open/iss-79-storeoriginal-inline-atomic-write.md @@ -0,0 +1,11 @@ +--- +schema_version: 1 +id: "iss-79" +slug: "storeoriginal-inline-atomic-write" +severity: "minor" +category: "tech-debt" +source: "agent-finding" +found_during: "2026-07-12 /abcd:run iss-32 review" +--- + +fifth divergent atomic write: internal/core/memory/ingest.go storeOriginal (~:806) is an inline temp(.memtmp,O_EXCL)+fsync+rename durable write that iss-32's consolidation left untouched — it is inline, not a named func, so TestNoNonCanonicalAtomicWritePrimitives (name-based) cannot catch it, and it was outside iss-32's named 4-copy corpus. It differs from canonical fsutil.WriteFileAtomic: no parent-dir fsync, no explicit chmod. Consolidate it onto fsutil.WriteFileAtomic(target, material.rawBytes, 0644) (keeping the pre-existing sources-dir symlink guard), OR extend the canonical-primitive detector to catch inline temp+rename sequences. Safe today (has its own symlink guard); this is a completeness/consistency follow-up. \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-32-atomic-write-consolidation.md b/.abcd/work/issues/resolved/iss-32-atomic-write-consolidation.md similarity index 61% rename from .abcd/work/issues/open/iss-32-atomic-write-consolidation.md rename to .abcd/work/issues/resolved/iss-32-atomic-write-consolidation.md index 886b9aa6..361e9422 100644 --- a/.abcd/work/issues/open/iss-32-atomic-write-consolidation.md +++ b/.abcd/work/issues/resolved/iss-32-atomic-write-consolidation.md @@ -7,6 +7,7 @@ category: "tech-debt" source: "agent-finding" found_during: "2026-07-08 multi-agent review" found_at: "internal/fsutil/fsutil.go" +resolution: "Consolidated the four named atomic-write/real-dir copies onto internal/fsutil (one-canonical-primitive), armed behind TestNoNonCanonicalAtomicWritePrimitives + a new fsutil crash-safety suite (fsutil had zero tests). Deleted ahoy writeFileAtomic+isRealDir, capture writeFileAtomic; memory durableWrite -> thin writeStringAtomic adapter over fsutil.WriteFileAtomic. Added fsutil.WriteFileAtomicPreserveMode. ruthless PROMOTE, security PASS (symlink-refusal preserved, O_EXCL->CreateTemp stronger, no mode downgrade). A 5th inline copy (memory/ingest.go storeOriginal), outside this issue's named corpus, is filed as iss-79." --- atomic-write consolidation: four independent temp-file+rename implementations with divergent durability — internal/fsutil.WriteFileAtomic (file fsync + parent-dir fsync), ahoy marker.go writeFileAtomic (no parent fsync, mode-preserving), capture roots.go writeFileAtomic (no parent fsync despite a doc comment claiming durability), memory writer.go durableWrite (parent fsync, O_EXCL naming); isRealDir is likewise duplicated (ahoy store.go vs fsutil). internal/fsutil itself has zero tests. Detector (per one-canonical-primitive): a lint flagging private redefinitions of the canonical names (writeFileAtomic, isRealDir) outside internal/fsutil, plus a fsutil crash-safety test suite; consolidation then drains behind the armed lint. Acceptance corpus: the three non-canonical copies and the duplicate isRealDir. \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 0dd499f1..0e30fff2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -129,6 +129,15 @@ called out in a **Breaking** section. named intent EXISTS and points back at this spec (bidirectional agreement). - The issue ledger moved from `.abcd/development/activity/issues` to `.abcd/work/issues` (the committed shared-working tier). +- The atomic-write and real-directory primitives are consolidated onto + `internal/fsutil` (iss-32): the ahoy, capture, and memory store writers no + longer keep their own divergent temp-file+rename copies. Two observable + effects of routing through the canonical primitive: the ahoy and capture + writers now fsync the parent directory after the rename (a crash-durability + strengthening they previously lacked), and memory pages are written at a + fixed `0644` (an explicit chmod, where the old writer left the mode subject to + the process umask). A `TestNoNonCanonicalAtomicWritePrimitives` guard keeps a + fifth copy from reappearing. ### Removed diff --git a/internal/core/ahoy/apply.go b/internal/core/ahoy/apply.go index 21a92a2d..79521a97 100644 --- a/internal/core/ahoy/apply.go +++ b/internal/core/ahoy/apply.go @@ -4,6 +4,8 @@ import ( "fmt" "os" "path/filepath" + + "github.com/REPPL/abcd-cli/internal/fsutil" "sort" "strings" "time" @@ -281,7 +283,7 @@ func (a *applyCtx) stepHistory() { } repoDir := filepath.Join(root, sha) transcripts := filepath.Join(repoDir, "transcripts") - if a.approved[SafeAutocreate] && !isRealDir(transcripts) { + if a.approved[SafeAutocreate] && !fsutil.IsRealDir(transcripts) { if err := os.MkdirAll(transcripts, 0o755); err == nil { a.note(transcripts) } diff --git a/internal/core/ahoy/detect.go b/internal/core/ahoy/detect.go index 7afd92ca..c9f698dc 100644 --- a/internal/core/ahoy/detect.go +++ b/internal/core/ahoy/detect.go @@ -4,6 +4,8 @@ import ( "os" "os/exec" "path/filepath" + + "github.com/REPPL/abcd-cli/internal/fsutil" "sort" "github.com/REPPL/abcd-cli/internal/core/identity" @@ -83,7 +85,7 @@ func classify(cwd string, id RepoIdentity, idx *historyIndex) (FolderKind, map[s registered := indexHasRoot(idx, id.RootSHA) signals["index_registered"] = registered - abcdDir := isRealDir(filepath.Join(cwd, ".abcd")) + abcdDir := fsutil.IsRealDir(filepath.Join(cwd, ".abcd")) signals["abcd_dir"] = abcdDir markerFired := false @@ -259,7 +261,7 @@ func detectHistoryStore(rootSHA string) []Gap { return gaps } repoDir := filepath.Join(root, rootSHA) - if !isRealDir(filepath.Join(repoDir, "transcripts")) { + if !fsutil.IsRealDir(filepath.Join(repoDir, "transcripts")) { gaps = append(gaps, Gap{ ID: "history.transcripts_missing", Category: SafeAutocreate, Scope: "repo", Title: "history transcripts/ dir missing", diff --git a/internal/core/ahoy/gitignore.go b/internal/core/ahoy/gitignore.go index b5dc8752..c2ed9477 100644 --- a/internal/core/ahoy/gitignore.go +++ b/internal/core/ahoy/gitignore.go @@ -4,6 +4,8 @@ import ( "bytes" "os" "path/filepath" + + "github.com/REPPL/abcd-cli/internal/fsutil" "strings" ) @@ -170,7 +172,7 @@ func applyVisibilityBlock(cwd, visibility string) (bool, error) { if absent { eol := "\n" body := strings.Join(canonicalGitignoreBlock(visibility), eol) + eol - if err := writeFileAtomic(path, []byte(body)); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(path, []byte(body)); err != nil { return false, err } return true, nil @@ -195,7 +197,7 @@ func applyVisibilityBlock(cwd, visibility string) (bool, error) { if bytes.Equal(newBytes, raw) { return false, nil // byte-identical — preserve mtime } - if err := writeFileAtomic(path, newBytes); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(path, newBytes); err != nil { return false, err } return true, nil diff --git a/internal/core/ahoy/marker.go b/internal/core/ahoy/marker.go index d4d4b6fb..198e112e 100644 --- a/internal/core/ahoy/marker.go +++ b/internal/core/ahoy/marker.go @@ -4,7 +4,8 @@ import ( "bytes" _ "embed" "os" - "path/filepath" + + "github.com/REPPL/abcd-cli/internal/fsutil" "regexp" ) @@ -108,7 +109,7 @@ func installMarkerFile(targetPath string) (wrote bool, ok bool) { if absent { body := append(append([]byte{}, synth...), eol...) - if err := writeFileAtomic(targetPath, body); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(targetPath, body); err != nil { return false, false } return true, true @@ -117,7 +118,7 @@ func installMarkerFile(targetPath string) (wrote bool, ok bool) { matches := markerBlockRe.FindAllIndex(existing, -1) if len(matches) == 0 { body := composeMarkerInsertion(existing, synth, eol) - if err := writeFileAtomic(targetPath, body); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(targetPath, body); err != nil { return false, false } return true, true @@ -127,7 +128,7 @@ func installMarkerFile(targetPath string) (wrote bool, ok bool) { return false, true // current — no write, mtime preserved } body := composeMarkerReplacement(existing, matches, synth) - if err := writeFileAtomic(targetPath, body); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(targetPath, body); err != nil { return false, false } return true, true @@ -201,7 +202,7 @@ func removeMarkerFile(targetPath string) (wrote bool, ok bool) { return false, true // no block — untouched } body := composeMarkerRemoval(existing, matches) - if err := writeFileAtomic(targetPath, body); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(targetPath, body); err != nil { return false, false } return true, true @@ -296,44 +297,3 @@ func concat(parts ...[]byte) []byte { } return out } - -// writeFileAtomic writes data to path via a temp file + rename, preserving the -// existing file's mode when present. Parent dirs are created as needed. -func writeFileAtomic(path string, data []byte) error { - dir := filepath.Dir(path) - if err := os.MkdirAll(dir, 0o755); err != nil { - return err - } - mode := os.FileMode(0o644) - if fi, err := os.Stat(path); err == nil { - mode = fi.Mode().Perm() - } - tmp, err := os.CreateTemp(dir, ".abcd-tmp-*") - if err != nil { - return err - } - tmpName := tmp.Name() - if _, err := tmp.Write(data); err != nil { - tmp.Close() - os.Remove(tmpName) - return err - } - if err := tmp.Sync(); err != nil { - tmp.Close() - os.Remove(tmpName) - return err - } - if err := tmp.Close(); err != nil { - os.Remove(tmpName) - return err - } - if err := os.Chmod(tmpName, mode); err != nil { - os.Remove(tmpName) - return err - } - if err := os.Rename(tmpName, path); err != nil { - os.Remove(tmpName) - return err - } - return nil -} diff --git a/internal/core/ahoy/store.go b/internal/core/ahoy/store.go index 531d8156..c7a1258b 100644 --- a/internal/core/ahoy/store.go +++ b/internal/core/ahoy/store.go @@ -5,6 +5,8 @@ import ( "os" "os/exec" "path/filepath" + + "github.com/REPPL/abcd-cli/internal/fsutil" "strings" "github.com/REPPL/abcd-cli/internal/core" @@ -116,11 +118,6 @@ func isDir(p string) bool { return err == nil && fi.IsDir() } -func isRealDir(p string) bool { - fi, err := os.Lstat(p) - return err == nil && fi.IsDir() && fi.Mode()&os.ModeSymlink == 0 -} - // --------------------------------------------------------------------------- // ~/.abcd/history store // --------------------------------------------------------------------------- @@ -293,7 +290,7 @@ func writeJSON(path string, v any) error { return err } data = append(data, '\n') - return writeFileAtomic(path, data) + return fsutil.WriteFileAtomicPreserveMode(path, data) } // writeConfig persists a config map deterministically. diff --git a/internal/core/capture/roots.go b/internal/core/capture/roots.go index 7787cc07..e4e4235b 100644 --- a/internal/core/capture/roots.go +++ b/internal/core/capture/roots.go @@ -113,41 +113,3 @@ func readWithChecksum(path string) (string, string, error) { } return string(data), sha256Hex(data), nil } - -// writeFileAtomic writes data to path via a temp file + fsync + rename, -// preserving the existing file mode (default 0644). -func writeFileAtomic(path string, data []byte) error { - dir := filepath.Dir(path) - mode := os.FileMode(0o644) - if fi, err := os.Stat(path); err == nil { - mode = fi.Mode().Perm() - } - tmp, err := os.CreateTemp(dir, ".abcd-iss-tmp-*") - if err != nil { - return err - } - tmpName := tmp.Name() - if _, err := tmp.Write(data); err != nil { - tmp.Close() - os.Remove(tmpName) - return err - } - if err := tmp.Sync(); err != nil { - tmp.Close() - os.Remove(tmpName) - return err - } - if err := tmp.Close(); err != nil { - os.Remove(tmpName) - return err - } - if err := os.Chmod(tmpName, mode); err != nil { - os.Remove(tmpName) - return err - } - if err := os.Rename(tmpName, path); err != nil { - os.Remove(tmpName) - return err - } - return nil -} diff --git a/internal/core/capture/workflow.go b/internal/core/capture/workflow.go index 068ae45c..b5dfe8f7 100644 --- a/internal/core/capture/workflow.go +++ b/internal/core/capture/workflow.go @@ -4,6 +4,8 @@ import ( "fmt" "os" "path/filepath" + + "github.com/REPPL/abcd-cli/internal/fsutil" "sort" "strconv" "strings" @@ -110,7 +112,7 @@ func commitCapture(req CaptureRequest, issID, slug, placeholder string) (Capture if checksum != emptyChecksum { return CaptureResult{}, fmt.Errorf("%w: placeholder %s changed since reservation", ErrChecksumMismatch, placeholder) } - if err := writeFileAtomic(placeholder, []byte(content)); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(placeholder, []byte(content)); err != nil { return CaptureResult{}, err } return CaptureResult{ID: issID, Slug: slug, Path: placeholder, Status: StateOpen}, nil @@ -204,7 +206,7 @@ func commitTransition(src, dst, newContent, expected string) error { if current != expected { return fmt.Errorf("%w: %s changed since it was read", ErrChecksumMismatch, src) } - if err := writeFileAtomic(dst, []byte(newContent)); err != nil { + if err := fsutil.WriteFileAtomicPreserveMode(dst, []byte(newContent)); err != nil { return err } if err := os.Remove(src); err != nil && !os.IsNotExist(err) { diff --git a/internal/core/memory/coverage.go b/internal/core/memory/coverage.go index e4b03634..2eb3f552 100644 --- a/internal/core/memory/coverage.go +++ b/internal/core/memory/coverage.go @@ -561,7 +561,7 @@ func writeCoverageIndex(path string, result coverageResult, budget quotationBudg if err := os.MkdirAll(dirOf(path), 0o755); err != nil { return nil, err } - if err := durableWrite(path, marshalIndentNoEscape(payload)); err != nil { + if err := writeStringAtomic(path, marshalIndentNoEscape(payload)); err != nil { return nil, err } return payload, nil diff --git a/internal/core/memory/lint.go b/internal/core/memory/lint.go index dbc5245e..3d017b10 100644 --- a/internal/core/memory/lint.go +++ b/internal/core/memory/lint.go @@ -464,10 +464,10 @@ func Lint(req LintRequest) (LintResult, error) { "generated_at": generatedAt, "store_path": mem, } - if err := durableWrite(filepath.Join(reportDir, "report.json"), marshalIndentNoEscape(reportFields)); err != nil { + if err := writeStringAtomic(filepath.Join(reportDir, "report.json"), marshalIndentNoEscape(reportFields)); err != nil { return LintResult{}, err } - if err := durableWrite(filepath.Join(reportDir, "report.md"), renderLintReportMD(reportFields)); err != nil { + if err := writeStringAtomic(filepath.Join(reportDir, "report.md"), renderLintReportMD(reportFields)); err != nil { return LintResult{}, err } diff --git a/internal/core/memory/writer.go b/internal/core/memory/writer.go index 13c35456..42877fc2 100644 --- a/internal/core/memory/writer.go +++ b/internal/core/memory/writer.go @@ -5,9 +5,10 @@ import ( "path/filepath" "sort" "strings" - "sync/atomic" "syscall" "time" + + "github.com/REPPL/abcd-cli/internal/fsutil" ) // writer.go — the atomic single-writer for .abcd/memory/ (ADR-13 §1–3). EVERY @@ -33,8 +34,6 @@ type WriteReport struct { WrotePages []string `json:"wrote_pages"` } -var tmpCounter uint64 - type renderedWrite struct { write PageWrite text string @@ -171,7 +170,7 @@ func writePagesLocked(repoRoot string, rendered []renderedWrite, merge RegistryM var wrote []string for _, r := range rendered { - if err := durableWrite(filepath.Join(mem, r.write.Filename), r.text); err != nil { + if err := writeStringAtomic(filepath.Join(mem, r.write.Filename), r.text); err != nil { return WriteReport{}, err } wrote = append(wrote, r.write.Filename) @@ -189,7 +188,7 @@ func writePagesLocked(repoRoot string, rendered []renderedWrite, merge RegistryM if err != nil { return WriteReport{}, err } - if err := durableWrite(SourcesIndexPath(repoRoot), SerializeRegistry(registry)); err != nil { + if err := writeStringAtomic(SourcesIndexPath(repoRoot), SerializeRegistry(registry)); err != nil { return WriteReport{}, err } } @@ -290,35 +289,13 @@ func logEvent(w PageWrite, stamp string) string { // Six-step durable write + tri-state read // --------------------------------------------------------------------------- -func durableWrite(path, content string) error { - tmp := path + "." + itoa(os.Getpid()) + "." + itoa(int(atomic.AddUint64(&tmpCounter, 1)-1)) + ".memtmp" - f, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) - if err != nil { - return err - } - writeErr := func() error { - if _, err := f.WriteString(content); err != nil { - return err - } - if err := f.Sync(); err != nil { - return err - } - return f.Close() - }() - if writeErr != nil { - f.Close() - os.Remove(tmp) - return writeErr - } - if err := os.Rename(tmp, path); err != nil { - os.Remove(tmp) - return err - } - if dir, err := os.Open(filepath.Dir(path)); err == nil { - dir.Sync() - dir.Close() - } - return nil +// writeStringAtomic durably writes string content through the canonical +// fsutil primitive at the store's fixed 0644 mode — a thin string adapter, not +// a second implementation (iss-32: one-canonical-primitive). fsutil handles the +// temp-file, fsync, rename, and parent-dir fsync, matching the durability this +// store previously carried in its own copy. +func writeStringAtomic(path, content string) error { + return fsutil.WriteFileAtomic(path, []byte(content), 0o644) } // triStateRead: (content, present, error). Absent -> ("", false, nil); a @@ -375,7 +352,7 @@ func ensureSkeleton(mem string) ([]string, error) { return nil, err } if !present { - if err := durableWrite(path, s.render()); err != nil { + if err := writeStringAtomic(path, s.render()); err != nil { return nil, err } created = append(created, s.name) @@ -412,12 +389,12 @@ func backfillLegacy(mem string) ([]string, error) { continue } newRegion := region + "source:\n class: " + backfillSourceClass + "\n" - if err := durableWrite(path, joinFileFrontmatter(newRegion, body)); err != nil { + if err := writeStringAtomic(path, joinFileFrontmatter(newRegion, body)); err != nil { return nil, err } } else { newRegion := "source:\n class: " + backfillSourceClass + "\n" - if err := durableWrite(path, joinFileFrontmatter(newRegion, "\n"+text)); err != nil { + if err := writeStringAtomic(path, joinFileFrontmatter(newRegion, "\n"+text)); err != nil { return nil, err } } @@ -528,7 +505,7 @@ func reconcile(mem string) ([]string, error) { return nil, err } if !present || sha256Hex(current) != sha256Hex(d.content) { - if err := durableWrite(path, d.content); err != nil { + if err := writeStringAtomic(path, d.content); err != nil { return nil, err } rewritten = append(rewritten, d.name) @@ -547,7 +524,7 @@ func appendLog(mem string, events []string) error { if present { base = current } - return durableWrite(path, base+strings.Join(events, "")) + return writeStringAtomic(path, base+strings.Join(events, "")) } // --------------------------------------------------------------------------- diff --git a/internal/fsutil/canonical_test.go b/internal/fsutil/canonical_test.go new file mode 100644 index 00000000..a0d0394e --- /dev/null +++ b/internal/fsutil/canonical_test.go @@ -0,0 +1,57 @@ +package fsutil + +import ( + "os" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// nonCanonicalPrimitiveRe matches a private redefinition of a durable-write or +// real-dir primitive — the exact names iss-32 consolidates. The canonical home +// is internal/fsutil (exported WriteFileAtomic / IsRealDir); any lowercase +// redefinition elsewhere is a divergent copy. +var nonCanonicalPrimitiveRe = regexp.MustCompile(`func\s+(writeFileAtomic|durableWrite|isRealDir)\b`) + +// TestNoNonCanonicalAtomicWritePrimitives is the one-canonical-primitive +// detector: no package under internal/ (other than fsutil) may declare its own +// named atomic-write or real-dir primitive. It matches top-level function +// declarations by name (not inline temp+rename sequences), which is what the +// consolidation removes. It walks the internal/ tree from this package's +// directory; before the consolidation it flags four copies (ahoy +// marker.go/store.go, capture roots.go, memory writer.go). +func TestNoNonCanonicalAtomicWritePrimitives(t *testing.T) { + internalRoot := filepath.Join("..") // internal/fsutil -> internal/ + var offenders []string + err := filepath.WalkDir(internalRoot, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + // The canonical home is exempt; it defines the real thing. + if filepath.Base(path) == "fsutil" { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(d.Name(), ".go") || strings.HasSuffix(d.Name(), "_test.go") { + return nil + } + data, err := os.ReadFile(path) + if err != nil { + return err + } + for _, m := range nonCanonicalPrimitiveRe.FindAllStringSubmatch(string(data), -1) { + offenders = append(offenders, path+": func "+m[1]) + } + return nil + }) + if err != nil { + t.Fatalf("walk internal/: %v", err) + } + if len(offenders) > 0 { + t.Fatalf("non-canonical atomic-write/real-dir primitives (route through internal/fsutil):\n %s", + strings.Join(offenders, "\n ")) + } +} diff --git a/internal/fsutil/fsutil.go b/internal/fsutil/fsutil.go index 6d0db09a..c29df995 100644 --- a/internal/fsutil/fsutil.go +++ b/internal/fsutil/fsutil.go @@ -1,13 +1,13 @@ // Package fsutil holds the durable-write and path-safety primitives shared by -// the ~/.abcd store writers. It is transport-agnostic: no stdout, no os.Exit, -// no CLI knowledge. +// the ~/.abcd and repo .abcd store writers. It is transport-agnostic: no +// stdout, no os.Exit, no CLI knowledge. // -// It exists so the six-step atomic write and the "is this a real directory, -// not a symlink" check live in ONE place. ahoy's marker.go/store.go carry -// their own unexported copies (writeFileAtomic, isRealDir) predating this -// package; those are the flagged consolidation target — a follow-up -// behaviour-preserving refactor should route them through here rather than -// keep a divergent copy. +// It is the single home for the atomic temp-file+fsync+rename write and the +// "is this a real directory, not a symlink" check: the ahoy, capture, and +// memory store writers all route through WriteFileAtomic / +// WriteFileAtomicPreserveMode / IsRealDir rather than keep divergent copies +// (the one-canonical-primitive invariant, guarded by +// TestNoNonCanonicalAtomicWritePrimitives). package fsutil import ( @@ -60,6 +60,18 @@ func WriteFileAtomic(path string, data []byte, perm os.FileMode) error { return nil } +// WriteFileAtomicPreserveMode is WriteFileAtomic that keeps the target's +// existing permission bits when it already exists, defaulting to 0644 for a new +// file. It is the canonical form for the store writers that rewrite a file in +// place and must not silently reset its mode. +func WriteFileAtomicPreserveMode(path string, data []byte) error { + perm := os.FileMode(0o644) + if fi, err := os.Stat(path); err == nil { + perm = fi.Mode().Perm() + } + return WriteFileAtomic(path, data, perm) +} + // syncParent fsyncs the directory so a crash right after the rename cannot lose // it. Some filesystems refuse a directory fsync; that is tolerated. func syncParent(dir string) { diff --git a/internal/fsutil/fsutil_test.go b/internal/fsutil/fsutil_test.go new file mode 100644 index 00000000..127f329d --- /dev/null +++ b/internal/fsutil/fsutil_test.go @@ -0,0 +1,133 @@ +package fsutil + +import ( + "os" + "path/filepath" + "testing" +) + +func TestWriteFileAtomicCreatesWithPerm(t *testing.T) { + dir := t.TempDir() + p := filepath.Join(dir, "sub", "f.txt") // parent dir does not exist yet + if err := WriteFileAtomic(p, []byte("hello"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + got, err := os.ReadFile(p) + if err != nil { + t.Fatalf("read: %v", err) + } + if string(got) != "hello" { + t.Fatalf("content = %q, want hello", got) + } + fi, err := os.Stat(p) + if err != nil { + t.Fatal(err) + } + if fi.Mode().Perm() != 0o600 { + t.Fatalf("perm = %o, want 600", fi.Mode().Perm()) + } +} + +func TestWriteFileAtomicOverwrites(t *testing.T) { + dir := t.TempDir() + p := filepath.Join(dir, "f.txt") + if err := WriteFileAtomic(p, []byte("first"), 0o644); err != nil { + t.Fatal(err) + } + if err := WriteFileAtomic(p, []byte("second"), 0o644); err != nil { + t.Fatal(err) + } + got, _ := os.ReadFile(p) + if string(got) != "second" { + t.Fatalf("content = %q, want second", got) + } + // No temp files linger after a successful write. + entries, _ := os.ReadDir(dir) + if len(entries) != 1 { + t.Fatalf("expected exactly one file, got %d: %v", len(entries), entries) + } +} + +// TestWriteFileAtomicReplacesSymlink proves the leaf symlink is REPLACED, not +// written through: a pre-planted symlink at path must not clobber its target. +func TestWriteFileAtomicReplacesSymlink(t *testing.T) { + dir := t.TempDir() + victim := filepath.Join(dir, "victim.txt") + if err := os.WriteFile(victim, []byte("do-not-touch"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(dir, "link.txt") + if err := os.Symlink(victim, link); err != nil { + t.Fatal(err) + } + if err := WriteFileAtomic(link, []byte("new"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + // The symlink is now a real file with the new content... + fi, err := os.Lstat(link) + if err != nil { + t.Fatal(err) + } + if fi.Mode()&os.ModeSymlink != 0 { + t.Fatalf("path is still a symlink; the write followed it") + } + // ...and the victim was not written through. + got, _ := os.ReadFile(victim) + if string(got) != "do-not-touch" { + t.Fatalf("symlink target was clobbered: %q", got) + } +} + +func TestWriteFileAtomicPreserveMode(t *testing.T) { + dir := t.TempDir() + + // New file defaults to 0644. + fresh := filepath.Join(dir, "fresh.txt") + if err := WriteFileAtomicPreserveMode(fresh, []byte("x")); err != nil { + t.Fatal(err) + } + if fi, _ := os.Stat(fresh); fi.Mode().Perm() != 0o644 { + t.Fatalf("new file perm = %o, want 644", fi.Mode().Perm()) + } + + // Existing file keeps its mode across a rewrite. + kept := filepath.Join(dir, "kept.txt") + if err := WriteFileAtomic(kept, []byte("a"), 0o600); err != nil { + t.Fatal(err) + } + if err := WriteFileAtomicPreserveMode(kept, []byte("b")); err != nil { + t.Fatal(err) + } + if fi, _ := os.Stat(kept); fi.Mode().Perm() != 0o600 { + t.Fatalf("rewritten file perm = %o, want 600 (preserved)", fi.Mode().Perm()) + } +} + +func TestIsRealDir(t *testing.T) { + dir := t.TempDir() + realDir := filepath.Join(dir, "d") + if err := os.Mkdir(realDir, 0o755); err != nil { + t.Fatal(err) + } + file := filepath.Join(dir, "f") + if err := os.WriteFile(file, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + linkToDir := filepath.Join(dir, "ld") + if err := os.Symlink(realDir, linkToDir); err != nil { + t.Fatal(err) + } + + if !IsRealDir(realDir) { + t.Errorf("real dir reported as not-real") + } + if IsRealDir(file) { + t.Errorf("file reported as real dir") + } + if IsRealDir(linkToDir) { + t.Errorf("symlink-to-dir reported as real dir (must lstat, not follow)") + } + if IsRealDir(filepath.Join(dir, "missing")) { + t.Errorf("missing path reported as real dir") + } +}