Fix: test race condition corrupts repos.json with test data#222
Merged
Fix: test race condition corrupts repos.json with test data#222
Conversation
…t race
Root cause: RepoManager used non-atomic ??= for static path caching (StateFile,
ReposDir, WorktreesDir). Tests that call SetBaseDirForTesting used Volatile.Write
to clear the cache, but parallel test execution could interleave between the
override write and the cache resolution, causing Save() to write to the real
~/.polypilot/repos.json instead of the test temp directory.
This corrupted the user's repos.json with test data ('repo-1', 'MyRepo') and
wiped all real repository registrations.
Fix:
- Add _pathLock around all static path resolution (getters and setter)
- RepoManagerTests uses SetBaseDirForTesting instead of raw field reflection
- Add [Collection('BaseDir')] to RepoManagerTests for serialization
- Add guard tests verifying RepoManager paths point to test directory
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…le.Read cleanup Finding 1: CopilotService had the same static-path race condition as RepoManager. Added _pathLock around all static path getters and SetBaseDirForTesting. Finding 2: Save() and Load() in RepoManager accessed StateFile twice without holding the lock across both calls. Captured to local variable once. Finding 3: GetBaseDir() used Volatile.Read paired with a lock-protected write. Replaced with plain read since it's only called within _pathLock. All 1400 tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
added a commit
that referenced
this pull request
Feb 27, 2026
PR #222 fixed the test race that corrupts repos.json, but the damage was already done — real repos were replaced with test data (repo-1). The bare clones still exist on disk but aren't tracked in repos.json. Load() now calls HealMissingRepos() which scans the repos/ directory for bare clones that exist on disk but aren't in the state file, reads their remote URL from git config, and re-adds them. Also reconstructs missing worktree entries from the worktrees/ directory. Added 4 tests for the self-healing logic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Running
dotnet testcould corrupt~/.polypilot/repos.json- replacing all real repositories with test data (repo-1,MyRepo). This caused all repositories and worktrees to vanish from the UI.Root Cause
RepoManageruses static fields for path resolution with lazy??=initialization. The test setup callsSetBaseDirForTesting()to redirect writes to a temp directory, but??=is not atomic and the tests could race on the static fields, causingSave()to write to the real path.Fix
_pathLockSetBaseDirForTesting()instead of raw field reflection[Collection("BaseDir")]to RepoManagerTestsTests
1400/1400 passing