From 40af76dd9055ca781fe820e2dd70dfedbd85c5b5 Mon Sep 17 00:00:00 2001 From: "dkoosis@gmail.com" Date: Tue, 11 Aug 2026 23:48:36 -0400 Subject: [PATCH] =?UTF-8?q?fix(atomicfile):=20ccp-sbp.5=20=E2=80=94=20adop?= =?UTF-8?q?t=20atomicfile=20for=20durable-state=20writes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Converts three raw os.WriteFile durable-state sites to github.com/dkoosis/atomicfile.WriteFile (write-temp + fsync + rename), and retires the hand-rolled tmp+rename helper in internal/counts. Converted: - internal/strandmd/strandmd.go: readOrInit — shipped default STRAND.md, written once on first init (parent dir already MkdirAll'd). - internal/registry/registry.go: Registry.saveLocked — repos.json, single-writer under r's mutex (parent dir already MkdirAll'd). - internal/counts/refresh.go: writeRowsAtomic + writeState — counts.json and the per-repo state file. Both are multi-writer (launchd --all vs a manual `strand counts` can race); atomicfile removes the torn-write risk but NOT the read-modify-write race, so each now carries a NOTE comment flagging the RMW gap for a follow-up (a lock, or merge-on-write like writeState already does for its own field). Retired: internal/counts/refresh.go's tmpPath() + manual os.Rename dance in both writeRowsAtomic and writeState — atomicfile.WriteFile does the temp+fsync+rename itself, including the parent-dir fsync the old helper never had. Skipped (test-only os.WriteFile calls, not durable-state — left as-is): strandmd/northstar_test.go, strandmd/strandmd_test.go, bdcounts/bdcounts_test.go, suggest/prompts_test.go, jtbd/jtbd_test.go, bd/store_test.go, bd/write_test.go, strand/strand_test.go, registry/registry_test.go, server/northstar_test.go, server/pulse_source_test.go, counts/refresh_test.go, server/server_test.go. go.mod: adds github.com/dkoosis/atomicfile + its renameio/v2 + x/sys transitive deps via go mod tidy. go directive (1.26.4) already met atomicfile's floor — no bump needed. Gate: make check green (vet, lint, race test suite; pack-drift skipped, upstream unreachable, pre-existing network condition). --- go.mod | 3 +++ go.sum | 6 ++++++ internal/counts/refresh.go | 34 ++++++++++++++++------------------ internal/registry/registry.go | 4 +++- internal/strandmd/strandmd.go | 4 +++- 5 files changed, 31 insertions(+), 20 deletions(-) diff --git a/go.mod b/go.mod index 8656d3f..614530d 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ toolchain go1.26.5 require ( github.com/anthropics/anthropic-sdk-go v1.52.0 + github.com/dkoosis/atomicfile v0.0.0-20260811102456-9091c28d4820 github.com/quasilyte/go-ruleguard/dsl v0.3.23 gonum.org/v1/gonum v0.17.0 ) @@ -13,6 +14,7 @@ require ( require ( github.com/bahlo/generic-list-go v0.2.0 // indirect github.com/buger/jsonparser v1.1.2 // indirect + github.com/google/renameio/v2 v2.0.2 // indirect github.com/invopop/jsonschema v0.14.0 // indirect github.com/pb33f/ordered-map/v2 v2.3.1 // indirect github.com/standard-webhooks/standard-webhooks/libraries v0.0.1 // indirect @@ -22,4 +24,5 @@ require ( github.com/tidwall/sjson v1.2.5 // indirect go.yaml.in/yaml/v4 v4.0.0-rc.2 // indirect golang.org/x/sync v0.16.0 // indirect + golang.org/x/sys v0.47.0 // indirect ) diff --git a/go.sum b/go.sum index 2a5f93a..fe8d432 100644 --- a/go.sum +++ b/go.sum @@ -6,8 +6,12 @@ github.com/buger/jsonparser v1.1.2 h1:frqHqw7otoVbk5M8LlE/L7HTnIq2v9RX6EJ48i9AxJ github.com/buger/jsonparser v1.1.2/go.mod h1:6RYKKt7H4d4+iWqouImQ9R2FZql3VbhNgx27UK13J/0= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/dkoosis/atomicfile v0.0.0-20260811102456-9091c28d4820 h1:x7UDk0isvTYxv8obfKfrancH99VtIJRZ3StZ9izJlvU= +github.com/dkoosis/atomicfile v0.0.0-20260811102456-9091c28d4820/go.mod h1:6wCWesghtzLYJ0IjlbbddyTCU8ZowoiIUzcy4Qi9fDc= github.com/dnaeon/go-vcr v1.2.0 h1:zHCHvJYTMh1N7xnV7zf1m1GPBF9Ad0Jk/whtQ1663qI= github.com/dnaeon/go-vcr v1.2.0/go.mod h1:R4UdLID7HZT3taECzJs4YgbbH6PIGXB6W/sc5OLb6RQ= +github.com/google/renameio/v2 v2.0.2 h1:qKZs+tfn+arruZZhQ7TKC/ergJunuJicWS6gLDt/dGw= +github.com/google/renameio/v2 v2.0.2/go.mod h1:OX+G6WHHpHq3NVj7cAOleLOwJfcQ1s3uUJQCrr78SWo= github.com/invopop/jsonschema v0.14.0 h1:MHQqLhvpNUZfw+hM3AZDYK7jxO8FZoQeQM77g8iyZjg= github.com/invopop/jsonschema v0.14.0/go.mod h1:ygm6C2EaVNMBDPpaPlnOA2pFAxBnxGjFlMZABxm9n2I= github.com/pb33f/ordered-map/v2 v2.3.1 h1:5319HDO0aw4DA4gzi+zv4FXU9UlSs3xGZ40wcP1nBjY= @@ -34,6 +38,8 @@ go.yaml.in/yaml/v4 v4.0.0-rc.2 h1:/FrI8D64VSr4HtGIlUtlFMGsm7H7pWTbj6vOLVZcA6s= go.yaml.in/yaml/v4 v4.0.0-rc.2/go.mod h1:aZqd9kCMsGL7AuUv/m/PvWLdg5sjJsZ4oHDEnfPPfY0= golang.org/x/sync v0.16.0 h1:ycBJEhp9p4vXvUZNszeOq0kGTPghopOL8q0fq3vstxw= golang.org/x/sync v0.16.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= +golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= +golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= gonum.org/v1/gonum v0.17.0 h1:VbpOemQlsSMrYmn7T2OUvQ4dqxQXU+ouZFQsZOx50z4= gonum.org/v1/gonum v0.17.0/go.mod h1:El3tOrEuMpv2UdMrbNlKEh9vd86bmQ6vqIcDwxEOc1E= gopkg.in/yaml.v2 v2.2.8 h1:obN1ZagJSUGI0Ek/LBmuj4SNLPfIny3KsKFopxRdj10= diff --git a/internal/counts/refresh.go b/internal/counts/refresh.go index d56e3bc..928fa49 100644 --- a/internal/counts/refresh.go +++ b/internal/counts/refresh.go @@ -13,6 +13,7 @@ import ( "strings" "time" + "github.com/dkoosis/atomicfile" "github.com/dkoosis/strand/internal/bd" "github.com/dkoosis/strand/internal/bdcounts" ) @@ -208,6 +209,13 @@ func readRows(path string) map[string]Row { // file + rename, so a concurrent reader never sees a half-written file. The meta rides // under bdcounts.MetaKey — a reserved key no repo path collides with — so keyed readers // (bdcounts.Reader.Lookup, the status line's `.[$repo]`) are untouched by its presence. +// +// NOTE (RMW race, not fixed by atomic write): two concurrent refreshes (the +// launchd --all run and a manual `strand counts`) each read-compute-write the +// whole rows set independently. atomicfile.WriteFile stops either write from +// being torn, but it does not serialize the two writers — whichever finishes +// last wins wholesale, silently dropping the other's rows. Follow-up: a lock +// around the refresh, or a merge-on-write like writeState's. func writeRowsAtomic(path string, rows map[string]Row, meta bdcounts.Meta) error { out := make(map[string]any, len(rows)+1) for k, v := range rows { @@ -218,24 +226,12 @@ func writeRowsAtomic(path string, rows map[string]Row, meta bdcounts.Meta) error if err != nil { return fmt.Errorf("counts: marshal: %w", err) } - tmp := tmpPath(path) - if err := os.WriteFile(tmp, append(data, '\n'), 0o600); err != nil { + if err := atomicfile.WriteFile(path, append(data, '\n'), 0o600); err != nil { return fmt.Errorf("counts: write: %w", err) } - if err := os.Rename(tmp, path); err != nil { - return fmt.Errorf("counts: rename: %w", err) - } return nil } -// tmpPath is a per-process temp name for the atomic write. The pid suffix keeps two -// concurrent refreshes (the launchd --all and a manual `strand counts`) off one -// shared tmp, where they would clobber each other's write and race the rename to a -// spurious ENOENT — the same guard the shell got from `$OUT.$$`. -func tmpPath(path string) string { - return fmt.Sprintf("%s.%d.tmp", path, os.Getpid()) -} - // repoState is one repo's carry-forward bookkeeping between refresh runs: the // last-observed change key (changeKey — the Dolt store mtime, or last-touched when // no store), plus the st-3p8 pending bit (a change-triggered derive schedules exactly @@ -283,6 +279,12 @@ func readState(path string) map[string]repoState { // changed-mode run would then find no prior mtime for those repos and cold-recompute // them all (st-dd9). Reading the prior state and overlaying keeps the untouched repos' // gate entries intact. +// +// NOTE (RMW race, not fixed by atomic write): the read-merge-write above is not +// serialized against a concurrent writer — two refreshes racing this function can +// each read the same prior state, merge their own entries on top, and whichever +// atomicfile.WriteFile finishes last wins wholesale, silently dropping the other's +// merged entries. Same follow-up as writeRowsAtomic. func writeState(path string, state map[string]repoState) error { merged := readState(path) maps.Copy(merged, state) @@ -295,13 +297,9 @@ func writeState(path string, state map[string]repoState) error { } fmt.Fprintf(&b, "%s\t%d\t%s\n", root, st.mtime, pendingField) } - tmp := tmpPath(path) - if err := os.WriteFile(tmp, []byte(b.String()), 0o600); err != nil { + if err := atomicfile.WriteFile(path, []byte(b.String()), 0o600); err != nil { return fmt.Errorf("counts: write state: %w", err) } - if err := os.Rename(tmp, path); err != nil { - return fmt.Errorf("counts: rename state: %w", err) - } return nil } diff --git a/internal/registry/registry.go b/internal/registry/registry.go index 4ab9b44..6af36e5 100644 --- a/internal/registry/registry.go +++ b/internal/registry/registry.go @@ -15,6 +15,8 @@ import ( "strings" "sync" "time" + + "github.com/dkoosis/atomicfile" ) // ErrUnknownRepo means a switch targeted a path the registry doesn't hold. @@ -300,7 +302,7 @@ func (r *Registry) saveLocked() error { if err != nil { return fmt.Errorf("marshal registry: %w", err) } - if err := os.WriteFile(r.file, data, 0o600); err != nil { + if err := atomicfile.WriteFile(r.file, data, 0o600); err != nil { return fmt.Errorf("write registry: %w", err) } return nil diff --git a/internal/strandmd/strandmd.go b/internal/strandmd/strandmd.go index 8b07924..4675eb7 100644 --- a/internal/strandmd/strandmd.go +++ b/internal/strandmd/strandmd.go @@ -17,6 +17,8 @@ import ( "path/filepath" "strconv" "strings" + + "github.com/dkoosis/atomicfile" ) // errHomeDirRequired is returned by Load when homeDir is empty, so a default is @@ -126,7 +128,7 @@ func readOrInit(path, def string) (string, error) { if mkErr := os.MkdirAll(filepath.Dir(path), 0o755); mkErr != nil { return "", fmt.Errorf("strandmd: mkdir .strand: %w", mkErr) } - if wErr := os.WriteFile(path, []byte(def), 0o600); wErr != nil { + if wErr := atomicfile.WriteFile(path, []byte(def), 0o600); wErr != nil { return "", fmt.Errorf("strandmd: write default STRAND.md: %w", wErr) } return def, nil