[fix] Serialize gitignore updates#290
Conversation
ibobgunardi
changed the title
ibobgunardi
force-pushed
the
bobi/gitignore-atomic-update
branch
2 times, most recently
from
559d807 to
726d1e7
Compare
ibobgunardi
force-pushed
the
bobi/gitignore-atomic-update
branch
from
726d1e7 to
fe30e4d
Compare
| return fn() | ||
| } | ||
|
|
||
| func acquireGitignoreLock(lockPath string) (func() error, error) { |
There was a problem hiding this comment.
High — Stale lock on process crash
acquireGitignoreLock creates a directory-based lock but never provides crash recovery. If the rimba process is killed (OOM, SIGKILL, panic before the defer in withGitignoreLock runs), .gitignore.lock remains on disk permanently. Every subsequent rimba call on that repo then spins for 2 s and fails with "timed out waiting for .gitignore lock". Users have no recovery path without manually running rm -rf .gitignore.lock.
This is a strict reliability regression: the original bug produced a cosmetic duplicate line; this introduces a failure mode where all gitignore operations hard-fail after a crash.
Suggested fix: Write the current PID into a file inside the lock dir at creation time. On each failed os.Mkdir retry, read that PID and check if the process is still alive (os.FindProcess + signal 0). If dead, forcibly remove the stale lock and retry immediately. Alternatively, document the recovery step and consider a rimba init --unlock escape hatch.
| } | ||
|
|
||
| func withGitignoreLock(repoRoot string, fn func() (bool, error)) (retAdded bool, retErr error) { | ||
| lockPath := filepath.Join(repoRoot, ".gitignore.lock") |
There was a problem hiding this comment.
High — .gitignore.lock is not excluded from git tracking
The lock directory is created at $repoRoot/.gitignore.lock. If a process crashes mid-operation and leaves the directory behind, git status will show ?? .gitignore.lock as an untracked entry — which is exactly the cosmetic dirtiness that issue #286 set out to fix.
Suggested fix: Move the lock path inside .rimba/ (e.g. filepath.Join(repoRoot, config.DirName, ".gitignore.lock")). The existing .rimba/ gitignore entry already covers it, so a crash-leftover lock directory would be automatically ignored by git.
| func TestEnsureGitignoreConcurrent(t *testing.T) { | ||
| dir := t.TempDir() | ||
|
|
||
| const workers = 8 |
There was a problem hiding this comment.
Medium — Package-level var mutated without synchronisation in concurrent test
gitignoreLockTimeout is a plain var in the production package. TestEnsureGitignoreConcurrent writes it on the test goroutine while other tests in the same package (e.g. TestEnsureGitignoreLockTimeout) also mutate it. When run with go test -race or -parallel, the Go race detector will flag this as a data race.
Suggested fix: Replace var gitignoreLockTimeout = 2 * time.Second with a sync/atomic.Int64 storing nanoseconds. Both production (time.Duration(gitignoreLockTimeout.Load())) and test code get race-free access without test-package tricks.
|
|
||
| // HasGitignoreEntry reports whether entry is present as a trimmed line in the | ||
| // .gitignore file at repoRoot. Returns false (not error) when the file is absent. | ||
| func HasGitignoreEntry(repoRoot, entry string) (bool, error) { |
There was a problem hiding this comment.
Medium — HasGitignoreEntry is exported but deliberately unguarded: hidden check-then-act trap
The public HasGitignoreEntry delegates straight to hasGitignoreEntry without acquiring the lock. Any caller that uses HasGitignoreEntry and then conditionally calls EnsureGitignore re-introduces the original TOCTOU. The contract is implicit: this function is safe only for read-only status checks, never in check-then-act patterns.
Suggested fix: Either make HasGitignoreEntry package-private (no external callers are visible in this diff), or add a doc comment: // HasGitignoreEntry does not acquire the .gitignore lock. Never use it in a check-then-act pattern; use EnsureGitignore or EnsureLocalGlobIgnored for atomic read-modify-write.
| } | ||
| } | ||
|
|
||
| func gitignorePattern(dir, file string) string { |
There was a problem hiding this comment.
Low — gitignorePattern helper doesn't pull its weight
func gitignorePattern(dir, file string) string {
return dir + "/" + file
}This function is called in exactly two places and its body is a single string concatenation — the same expression already appears inline everywhere else in this file. It adds a layer of naming indirection without capturing any non-obvious invariant.
Suggested fix: Inline dir + "/" + file at both call sites and delete the function.
| func removeGitignoreEntryVariantsLocked(repoRoot, dir, file string) { | ||
| entry := gitignorePattern(dir, file) | ||
| _, _ = removeGitignoreEntryLocked(repoRoot, entry) | ||
| if legacyEntry := dir + "\\" + file; legacyEntry != entry { |
There was a problem hiding this comment.
Low — Legacy backslash guard is vacuously true
if legacyEntry := dir + "\\\\"; legacyEntry != entry {On every platform dir + "/" + file != dir + "\\" + file, so this guard is always true and the double-cleanup always runs. The intent (cross-platform migration, not a runtime OS branch) is correct but the condition is misleading.
Suggested fix: Remove the guard and call removeGitignoreEntryLocked for both forms unconditionally, or add a comment: // backslash form is always different on any OS — intentional cross-platform migration.
2 participants
Fixes #286.
EnsureGitignore,RemoveGitignoreEntry, and the local-glob migration now share a small lock around the.gitignoreread/modify/write path. That keeps parallel rimba processes from both appending the same entry.While touching that path, I also made the local glob use gitignore-style
/separators consistently, and kept migration tolerant of the older Windows\form.Checks:
go test ./internal/fileutil -run Gitignore|LocalGlob -count=1 -vgo test ./internal/fileutil -run TestEnsureGitignoreConcurrent -count=3 -timeout=180sgo test ./cmd -run InitFreshGitignoreAlreadyPresent|InitReInitMigratesPerFileEntries|InitMigrationFromLegacy|InitFresh|InitExistingDirConfig|InitPersonal -count=1 -timeout=180sgo test ./internal/trust -run RecordGitignore -count=1 -timeout=180s