Test the cache arms that only a real filesystem reaches - #179
Merged
Merged
Conversation
Three seams, all of them recovery paths, none reachable from a unit test because what decides the answer is the state of a real directory or the exit status of a real `git clone`. A mocked subprocess picks the outcome before the code does. **repo_manager, mid-disaster** (new integration file). A bare clone on disk with no metadata record is adopted, not cloned over -- proved by a ref the remote does not have, which a re-clone would lose. A directory with no HEAD is cleared and replaced. A failed clone leaves no record and, crucially, does not take the repo lock file with it: widening that cleanup to the parent directory unlinks a lock this call is still holding, which hands the next arrival a lock nobody is excluded by. Worth stating because the tests say so rather than implying it: git removes the destination it created for every failure a test can arrange, so the `rmtree` in the except is a backstop those tests do not pin. What they pin is the state the cache is left in, whoever did the removing. **migration's three OSError arms**, driven by real permissions and a real inode type rather than a patched `os.rename`: a refused rename costs one directory and not the run, an unscannable corner of the cache is reported and survived, and a listing that cannot be written degrades to an instruction the user can follow instead of a path that is not there. migration.py goes 90% -> 100%. **The clone race itself**, in two real interpreters. test_locks.py covers the lock; nothing covered the thing being excluded, which needs one real `git clone --bare` racing another. Both tests fail on every run with the lock removed. The staged one reproduces the cross-process adoption case on purpose: the loser loads its metadata before the winner writes any, so it wakes to a `.bare` its own records have never heard of. Every arm here was checked by mutation.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideAdds integration and filesystem-driven tests to cover repository cache recovery, worktree migration OSError paths, and cross-process clone races, improving confidence in failure handling and lock behavior without changing production logic. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
==========================================
+ Coverage 93.95% 94.63% +0.67%
==========================================
Files 21 21
Lines 2565 2570 +5
==========================================
+ Hits 2410 2432 +22
+ Misses 155 138 -17
🚀 New features to boost your workflow:
|
… hid them
Ten findings from the review of this PR. Two were production defects the new
tests walked past, and one of my tests was actively concealing its own.
**`_clone_dirs` abandoned the rest of the cache on the first refusal.** The
whole three-level walk sat under one `try`, and owners are walked in sorted
order, so an unreadable `acme/` ended the scan for every owner after it: no
notice, no listing file, and the version header still advanced, so those
unmigrated clones -- which may hold uncommitted work -- were abandoned
permanently. Caught at each level now. My test could not see it because I named
the unreadable directory `someone-else`, which sorts *after* `blooop`; it is
`aaa-corp` now and the name is load-bearing enough to say so in the comment.
**`_get_default_branch` truncated slashed branch names.** `.split("/")[-1]`
on `refs/heads/release/1.0` records `1.0`, a ref the repository does not have,
as the branch every later operation targets. The prefix is stripped instead. The
class covering this tested only `master`, which has no slash and passes either
way; it is parametrized over `master`, `release/1.0` and `feature/auth`.
**The simultaneous-start race test greened 27% of the time with no lock at all.**
Measured over 30 runs with the flock removed: 8 passed, because when the two
processes happen to serialize by luck every assertion still holds -- and
serialization gets *more* likely on the loaded single-core runner where the lock
matters most. A guard that greens a quarter of the time under the defect it
names is worse than none. It is gone, replaced by a second staged test that
pins the contention *notice*. Both remaining tests now fail on 12 runs out of 12
with the lock removed.
And four smaller ones:
- the unwritable-cache test died in `hold_lock`'s mkdir and never reached
`clone_repo`, so both its assertions were true before the call as well as
after; it blocks only the clone now, and its `pytest.raises` names the error
instead of accepting any `OSError`.
- the adoption test asserted `default_branch == "main"`, which is also what
`_get_default_branch` returns having read nothing. Its remote is master-headed.
- `finally: proc.kill()` neither reaped the driver nor drained it, orphaning the
`git clone` it had started into a tmp_path pytest was deleting. `stop()` kills
the process group and drains.
- `await_flags` now watches the drivers, so one that dies at import fails in a
second with its traceback instead of after a 60s timeout without it.
The chmod-based refusals no longer guess from `geteuid`: `refuses_writes` and
`refuses_reads` apply the mode and then *attempt the forbidden operation*,
skipping where the filesystem does not enforce it -- Docker Desktop and Colima
bind mounts store the mode and ignore it, which a uid check does not notice.
One finding is pinned rather than fixed: a refused rename still advances the
schema header, so those records keep their pre-#64 workspace ids and no later
run revisits them, which means `dl ... rm` can never find them again. The fix
is a choice between two imperfect options and belongs to whoever makes it
deliberately. Filed as #180, with the test that should go red when it lands.
Every fix re-verified by mutation.
Merged
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.
Three recovery seams, none reachable from a unit test: what decides the answer in each is the state of a real directory or the exit status of a real
git clone, so a mockedsubprocesspicks the outcome before the code does.repo_manager, mid-disaster —test/integration/test_repo_manager_recovery.pyHEADis cleared and replaced.rmtreeto the parent unlinks a lock the call is still holding, which is the classic self-defeating move: the holder keeps an inode nobody can see and the next arrival locks a fresh file and walks past it.master-headed remote —_get_default_branchfalls back tomainthrough threeexcepts, so a repo that really ismasterand one the function could not read otherwise give the same answer.Stated in the file rather than implied: for every failure a test can arrange,
gitremoves the destination it created, so thermtreein theexceptis a backstop those tests do not pin. Two mutations of it stayed green and are documented as such instead of being papered over.migration's three
OSErrorarms —test/test_worktree_migration.pyDriven by real permissions and a real inode type, not a patched
os.rename, because what is being tested is that the arm catches what the OS actually raises.migration.pygoes 90% → 100%.The clone race itself —
test/integration/test_clone_race.pytest_locks.pycovers the lock. Nothing covered the thing being excluded, which needs one realgit clone --bareracing another in another interpreter — threads share one file description and one GIL, so a threaded version tests a lock that is not the one shipping..bareits own records have never heard of — the cross-process adoption case, reproduced deterministically rather than by deleting a record. A marker written inside the clone under the lock proves the loser did not destroy it.Both fail on every run with
ensure_repo's lock removed (checked 5×), and pass on every run with it (checked 8×).Verification
Every arm was checked by mutation — 13 mutations applied to
repo_manager.py,migration.pyand the lock, each confirmed to turn exactly the intended test red. Two that stayed green are documented in the test file as unreachable rather than left as a silent gap.Coverage:
repo_manager.py80% → 85%,migration.py90% → 100%, total 94% → 95%.locks.pystays at 76% for the known reason —coveragedoes not follow subprocesses, and its tests are subprocesses.pylint 10.00/10 across all five environments (default, py310–py313); ruff and format clean.
Summary by Sourcery
Add integration and filesystem-driven tests that exercise repository cache recovery, migration failure handling, and clone race behaviour under real git and OS conditions.
Tests: