fix(atomicfile): ccp-sbp.5 — adopt atomicfile for durable-state writes - #108
Conversation
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).
📝 WalkthroughWalkthroughThe change adds atomic file persistence for counts, state, registry, and default ChangesAtomic persistence
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/counts/refresh.go`:
- Around line 229-231: Update the first-write paths to pass
atomicfile.WithMkdirAll(0o755) to atomicfile.WriteFile: apply this to the
counts.json and state writes in internal/counts/refresh.go (229-231), the
repos.json write in internal/registry/registry.go (305), and the default
STRAND.md write in internal/strandmd/strandmd.go (131-132). Remove the separate
os.MkdirAll calls replaced by this option.
- Around line 212-218: Serialize the entire read-modify-write sequence in
refresh, including reading existing counts state, computing updates, and writing
counts.json/counts-mtimes. Add a cross-process lock around refresh (or
equivalent re-read-and-merge retry logic) so concurrent counts.Run processes
cannot overwrite each other’s rows; retain atomicfile.WriteFile for tear-free
writes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ae059a8-249a-4e1a-bc26-b46329d46c1f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.modinternal/counts/refresh.gointernal/registry/registry.gointernal/strandmd/strandmd.go
| // | ||
| // 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. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'readRows|writeRowsAtomic|readState|writeState|refresh|flock|Lock' internal/counts
rg -n -C 3 'launchd|strand counts|counts\.Run|refresh\(' --glob '*.go' .Repository: dkoosis/strand
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- refresh sequence ---'
sed -n '92,138p' internal/counts/refresh.go
printf '%s\n' '--- row writer ---'
sed -n '195,234p' internal/counts/refresh.go
printf '%s\n' '--- state writer ---'
sed -n '274,315p' internal/counts/refresh.go
printf '%s\n' '--- production call sites and background scheduling ---'
sed -n '120,145p' internal/server/server.go
sed -n '240,265p' internal/server/server.go
rg -n -C 4 'counts\.Run|defaultRefreshCounts|refreshCounts|goBackground' --glob '*.go' .
printf '%s\n' '--- atomicfile references and module metadata ---'
rg -n -C 3 'github.com/dkoosis/atomicfile|WithMkdirAll|func WriteFile|WriteFile' --glob '*.go' --glob 'go.mod' --glob 'go.sum' .Repository: dkoosis/strand
Length of output: 43225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- external refresh trigger ---'
rg -n -C 4 'counts( --all)?|StartInterval|bd-counts|launchd' \
--glob '!internal/counts/refresh.go' --glob '!internal/counts/refresh_test.go' .
printf '%s\n' '--- pinned atomicfile implementation ---'
curl -fsSL https://raw.githubusercontent.com/dkoosis/atomicfile/9091c28d4820/atomicfile.go |
sed -n '1,240p'
printf '%s\n' '--- standalone lost-update model ---'
python3 - <<'PY'
import json
base_rows = {"repo-a": "old-a", "repo-b": "old-b"}
a_rows = dict(base_rows)
b_rows = dict(base_rows)
a_rows["repo-a"] = "new-a"
b_rows["repo-b"] = "new-b"
# Both writers use atomic replacement. B completes last, so the file is valid
# JSON but contains neither writer's complete result.
final_rows = b_rows
assert json.loads(json.dumps(final_rows)) == {
"repo-a": "old-a",
"repo-b": "new-b",
}
assert final_rows["repo-a"] != a_rows["repo-a"]
print("last-writer-wins:", json.dumps(final_rows, sort_keys=True))
print("repo-a update lost while the resulting file remains valid JSON")
PYRepository: dkoosis/strand
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- external refresh trigger ---'
rg -n -C 4 'counts( --all)?|StartInterval|bd-counts|launchd' \
--glob '!internal/counts/refresh.go' --glob '!internal/counts/refresh_test.go' .
printf '%s\n' '--- pinned atomicfile implementation ---'
curl -fsSL https://raw.githubusercontent.com/dkoosis/atomicfile/9091c28d4820/atomicfile.go |
sed -n '1,240p'
printf '%s\n' '--- standalone lost-update model ---'
python3 - <<'PY'
import json
base_rows = {"repo-a": "old-a", "repo-b": "old-b"}
a_rows = dict(base_rows)
b_rows = dict(base_rows)
a_rows["repo-a"] = "new-a"
b_rows["repo-b"] = "new-b"
final_rows = b_rows
assert json.loads(json.dumps(final_rows)) == {
"repo-a": "old-a",
"repo-b": "new-b",
}
assert final_rows["repo-a"] != a_rows["repo-a"]
print("last-writer-wins:", json.dumps(final_rows, sort_keys=True))
print("repo-a update lost while the resulting file remains valid JSON")
PYRepository: dkoosis/strand
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for log in \
/tmp/coderabbit-shell-logs/shell-output-Lk08ky \
/tmp/coderabbit-shell-logs/shell-output-wYN1HR
do
echo "--- $log: atomicfile and model results ---"
rg -n -A 80 -B 5 'pinned atomicfile|func WriteFile|atomic replacement|last-writer-wins|repo-a update lost|standalone lost-update' "$log" || true
doneRepository: dkoosis/strand
Length of output: 13000
Serialize the complete refresh read-modify-write sequence.
Concurrent counts.Run processes can read the same base, then atomically replace counts.json or counts-mtimes and silently discard the other update. atomicfile.WriteFile prevents torn writes, not lost updates. Add a cross-process lock around the entire refresh, or implement re-read and merge retry logic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/counts/refresh.go` around lines 212 - 218, Serialize the entire
read-modify-write sequence in refresh, including reading existing counts state,
computing updates, and writing counts.json/counts-mtimes. Add a cross-process
lock around refresh (or equivalent re-read-and-merge retry logic) so concurrent
counts.Run processes cannot overwrite each other’s rows; retain
atomicfile.WriteFile for tear-free writes.
Source: MCP tools
| if err := atomicfile.WriteFile(path, append(data, '\n'), 0o600); err != nil { | ||
| return fmt.Errorf("counts: write: %w", err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
go doc github.com/dkoosis/atomicfile.WithMkdirAll
for file in \
internal/counts/refresh.go \
internal/registry/registry.go \
internal/strandmd/strandmd.go
do
rg -n -C 3 'os\.MkdirAll|atomicfile\.WriteFile' "$file"
doneRepository: dkoosis/strand
Length of output: 4004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- module declaration ---'
cat go.mod | sed -n '1,120p'
printf '%s\n' '--- relevant implementations and callers ---'
for file in internal/counts/refresh.go internal/registry/registry.go internal/strandmd/strandmd.go; do
printf '\n--- %s ---\n' "$file"
sed -n '100,140p' "$file" 2>/dev/null || true
sed -n '280,315p' "$file" 2>/dev/null || true
done
printf '%s\n' '--- atomicfile source at the referenced revision ---'
curl -fsSL https://raw.githubusercontent.com/dkoosis/atomicfile/9091c28d4820/atomicfile.go | sed -n '1,260p'Repository: dkoosis/strand
Length of output: 14038
Use atomicfile.WithMkdirAll(0o755) for all first writes.
os.MkdirAll does not durably persist newly created directories. Apply the option to the counts.json and state writes, repos.json, and default STRAND.md writes. Remove the separate os.MkdirAll calls where the option replaces them.
📍 Affects 3 files
internal/counts/refresh.go#L229-L231(this comment)internal/registry/registry.go#L305-L305internal/strandmd/strandmd.go#L131-L132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/counts/refresh.go` around lines 229 - 231, Update the first-write
paths to pass atomicfile.WithMkdirAll(0o755) to atomicfile.WriteFile: apply this
to the counts.json and state writes in internal/counts/refresh.go (229-231), the
repos.json write in internal/registry/registry.go (305), and the default
STRAND.md write in internal/strandmd/strandmd.go (131-132). Remove the separate
os.MkdirAll calls replaced by this option.
Source: MCP tools
|
Assessed CodeRabbit's 2 findings: 1 accepted as follow-up bead, 1 rejected. Rejected — Accepted, out of scope — serialize the refresh read-modify-write. Real: atomicfile stops torn writes, not lost updates, and two concurrent refreshes can still discard each other's rows. That's a pre-existing race this PR documents rather than introduces (see the two Gate green on the branch: |
1 participant
Converts every non-test raw
os.WriteFiledurable-state site togithub.com/dkoosis/atomicfile.WriteFile, and retires the hand-rolled tmp+rename helper.Converted (3 sites)
internal/strandmd/strandmd.goreadOrInit— shipped default STRAND.md, written once on first init.internal/registry/registry.goRegistry.saveLocked—repos.json, single-writer under the registry's mutex.internal/counts/refresh.gowriteRowsAtomic+writeState—counts.jsonand the per-repo state file. Both are multi-writer (launchd--allvs a manualstrand countscan race the same path); atomicfile removes the torn-write risk but not the read-modify-write race, so each carries aNOTEcomment flagging the RMW gap as a follow-up (a lock, or merge-on-write likewriteStatealready does for its own field).Retired
internal/counts/refresh.go'stmpPath()+ manualos.Renamedance (used by bothwriteRowsAtomicandwriteState) —atomicfile.WriteFiledoes temp+fsync+rename itself, including the parent-dir fsync the old helper never had.Skipped (test-only, 13 sites)
All remaining
os.WriteFilehits are test fixture setup (writing input files for a test to read), not durable app state — left as-is:strandmd/northstar_test.go,strandmd/strandmd_test.go,bdcounts/bdcounts_test.go,suggest/prompts_test.go(×3),jtbd/jtbd_test.go,bd/store_test.go,bd/write_test.go(×2),strand/strand_test.go,registry/registry_test.go,server/northstar_test.go,server/pulse_source_test.go(×3),counts/refresh_test.go(×2),server/server_test.go.go.mod
Adds
github.com/dkoosis/atomicfile+ itsrenameio/v2andx/systransitive deps viago mod tidy. Thegodirective (1.26.4) already meets atomicfile's floor — no bump needed.Gate
make checkgreen:go vet,golangci-lint(0 issues),go test -race(all packages pass).pack-driftstep skipped — upstream unreachable, pre-existing network condition unrelated to this change.bead: ccp-sbp.5
Summary by CodeRabbit