Repository navigation
fix(repo): repo run survives a crash mid-grade and a parallel run on one cache - #724
vlad-perevezentsev wants to merge 7 commits into
Conversation
_set_git_config_aside decided `first` from whether agent-git/ existed, and created it before the moves. A run killed after the mkdir and before the last move made the next grade of the cell take itself for a regrade and delete the agent's .git/config, hooks and pointers without archiving them. The moves now go to .agent-git.partial/, renamed to agent-git/ after the last one; a grade that finds the partial directory finishes the moves from what is still in the workspace. A link left in place of the partial directory is replaced, not moved into. The .git file case honours `first` too: a regrade no longer moves a grader's .git file over the archived agent-git/dot-git. Refs intel#687 Signed-off-by: Vladislav Perevezentsev <vladislav.perevezentsev@intel.com>
ensure_clone cloned into a fixed <git_dir>.partial: a second process cloning the same URL deleted the first's partial clone, or cloned into it after the first's rename and failed renaming over the finished clone. source_git_dir had the same race on the full clone task bases are checked out from. Both go through clone_once: each call clones into its own <git_dir>.<pid>-<random>.partial, and a call whose rename fails uses the clone already there when it has a HEAD. A partial directory older than the git timeout belongs to no live clone and is removed. Refs intel#687 Signed-off-by: Vladislav Perevezentsev <vladislav.perevezentsev@intel.com>
shutil.move across file systems copies, then deletes the source; a crash while deleting left a full copy in .agent-git.partial/ and a subset in the workspace, and the redo replaced the copy with the subset. workspace/ is made in the run dir, so each move is a rename on one file system: atomic, and loud if that ever stops holding. The clear before each move and the test of a copy cut short go with it. Refs intel#687 Signed-off-by: Vladislav Perevezentsev <vladislav.perevezentsev@intel.com>
clone_once removed a partial directory older than source.py's git timeout, which held for the full clone of source_git_dir only because workspace.py's timeout has the same value. Each caller now passes the timeout its clone runs under. Refs intel#687 Signed-off-by: Vladislav Perevezentsev <vladislav.perevezentsev@intel.com>
Every call of ensure_rev for a branch or tag fetches all refs into the shared cache. Two runs doing it at once after the origin moved fought over the ref locks, and one of them failed with "cannot lock ref ... is at X but expected Y" (20 of 40 fetches in 20 rounds of two at once). The fetch is now run again, up to three times, when it failed on a ref lock: by then the other fetch has updated the refs. Any other fetch failure is raised as before. Refs intel#687 Signed-off-by: Vladislav Perevezentsev <vladislav.perevezentsev@intel.com>
The fetch is run again when its error says "cannot lock ref", but git ran in the user's locale: a translated git printed another text, and a run that lost a ref to another run's fetch failed as before. The ref fetch now runs with LC_ALL=C, as schema_compat's git calls do for the messages they match. Refs intel#687 Signed-off-by: Vladislav Perevezentsev <vladislav.perevezentsev@intel.com>
|
🔍 OpenCodeReview found 1 issue(s) in this PR. CI at By severity: 1 medium
Review details
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #724 +/- ##
==========================================
+ Coverage 95.71% 95.73% +0.02%
==========================================
Files 397 408 +11
Lines 53247 54676 +1429
Branches 9161 9411 +250
==========================================
+ Hits 50963 52346 +1383
- Misses 1410 1442 +32
- Partials 874 888 +14
🚀 New features to boost your workflow:
|
napetrov
left a comment
napetrov
left a comment
There was a problem hiding this comment.
Change class: not computed (no receipt profile for this repo, or receipt.py failed)
Comment, one question. CI: 0 failing jobs on head. The rename-based set-aside and the per-call clone partials look right on the paths I could run. One question: a run dir the previous code left after a crash right after its mkdir still loses the agent's git config on the next grade, because the new check treats the empty agent-git/ as archived. The PR fixes new crashes only; I reproduced the old state on head.
Coverage: whole-PR and edge pass on both trees; the set-aside, clone and fetch paths; the new tests on head; a concurrent-fetch stress probe. Not run: the full suite (scipy missing here), Windows.
| if dot_git.is_symlink() or dot_git.is_file(): # the agent's own repository elsewhere: the grader gets none | ||
| shutil.move(str(dot_git), str(aside / "dot-git")) | ||
| return | ||
| if not aside.exists(): |
There was a problem hiding this comment.
question: a run dir left by the previous code after a crash right after its mkdir still loses the agent's git config on the next grade
if not aside.exists(): takes an empty agent-git/ as already archived, so the grade deletes the agent's .git/config and hooks and writes the plain config, with nothing archived. The new layout only prevents this for new crashes. Run dirs already on disk are not migrated.
How to check: an empty pre-existing agent-git/ and an agent [core] fsmonitor in .git/config, then one _set_git_config_aside call → archived: False, config replaced (probes/scripts/legacy_empty_aside.py, head; base has the same check).
Expected: a grade that finds an empty agent-git/ should finish the archive as a partial, or the PR should say runs from the previous version are out of scope.
A partial clone whose objects cannot be deleted was left in the cache without an error. The removal makes entries writable and retries, and raises when the partial is still there. Tests pin the removal, the raise, the raise on a failed rename onto a directory with no HEAD, and the SourceError after the last fetch try. Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com>
|
|
Pushed 1 commit to
Questions for @vlad-perevezentsev1. Should a grade that finds an empty
|
| for old in git_dir.parent.glob(f"{git_dir.name}.*partial"): | ||
| try: | ||
| if time.time() - old.stat().st_mtime > timeout_s: | ||
| _remove_tree(old) |
There was a problem hiding this comment.
Must-fix — Impact: abandoned partial clones remain indefinitely, potentially retaining a full repository's worth of cache storage. Trigger: one concurrent clone publishes git_dir, while another process is killed before its finally removes its partial. Evidence: subsequent clone_once calls return at the preceding HEAD check, so this cleanup loop is never reached, even after timeout_s; there is no other repository-cache cleanup path. This affects both ensure_clone and source_git_dir. Move expired-partial cleanup before the cached-repository fast path and add a regression with a valid cached clone plus an expired partial. Workaround: manually delete confirmed-inactive partial directories. Severity: medium, because the storage leak is persistent but bounded to abandoned cache clones.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 20c2efc. The set-aside rename logic holds up: a crash after any of the moves, or between the last move and the final rename, loses nothing. Two things are worth fixing before merge, both in repos/source.py: the ref-fetch retry does not fire on a current git, and an expired partial clone is never removed once a valid clone exists.
What I ran, on a Linux review host at 20c2efc:
- The two new test files: 15 passed, 3 failed. The three failures are in
test_repo_source_clone_race.pyand are the retry tests (see the inline comment onsource.py:545). - Reproductions for the code findings, on local bare origins, no network.
- Mutations of the grading change: dropping the first-grade check, the regrade cleanup, the plain config,
commondirorconfig.worktreehandling each fail a test.
Still waiting for the author: @napetrov's question on run dirs left by the previous code with an empty agent-git/ (grading.py:268) is unanswered. The choice is the author's, and the merge waits for it.
Checked, no issues
- Crash windows in the set-aside: after 0, 1 and 3 moves, and after the last move before
partial.rename(aside), the next grade finishes the archive and leaves a plain.git/config. - Glob in
clone_once:<name>.*partialcannot match another cache entry. The.full.gitclone has a different name stem. - Imports and ratchet:
shutilis still used ingrading.pyandworkspace.py.test_growth_ratchet.pypasses. - Tests are offline: they use
tmp_pathand local origins, and leave the tree clean.
| "+refs/heads/*:refs/heads/*", "+refs/tags/*:refs/tags/*", env=_FETCH_ENV) | ||
| return | ||
| except SourceError as exc: | ||
| if "cannot lock ref" not in str(exc) or attempt == _FETCH_TRIES - 1: |
There was a problem hiding this comment.
The fetch retry never fires on git 2.53, so the race the PR fixes still fails one fetch in two. The loop retries only when the error contains cannot lock ref. Git 2.53.0 prints a different text for the same collision.
- Two
ensure_rev(git_dir, "main")calls at once, 20 rounds, a bare origin with 30 branches advanced each round: 40 calls, 20 failed, all withSourceError: git fetch failed: error: fetching ref refs/heads/b9 failed: incorrect old value provided. That is the same 20 of 40 as before the retry.cannot lock refwas in none of them. - With the lock held by file, as the new tests do, the text is
error: fetching ref refs/heads/topic failed: reference already exists. On this gittest_a_branch_fetch_that_lost_a_ref_to_another_run_is_run_again[None],[de_DE.UTF-8]andtest_a_branch_fetch_that_keeps_losing_its_ref_raises_after_the_last_tryfail with it. The PR's own evidence (14 passed) was measured on an older git. process.git_failurealso keeps only the lasterror:line, so matching on one line of a multi-ref failure is fragile.
The tests are not worthless but they depend on the git version, because they make git produce the text. Mutation check: with _FETCH_TRIES = 1, the env= argument dropped, or LC_ALL removed, the same three tests fail, so on this git nothing separates a working retry from none.
Suggestion: treat any non-zero git fetch of the refspecs as retryable when the refs have moved since (for example, retry up to _FETCH_TRIES times on SourceError and re-raise the last), or match cannot lock ref, incorrect old value provided and reference already exists. Pin the loop's logic with a stubbed _git that raises a fixed message, and keep one real two-process test that does not depend on the message text.
| if (git_dir / "HEAD").is_file(): | ||
| return git_dir | ||
| cache_dir.mkdir(parents=True, exist_ok=True) | ||
| tmp = git_dir.with_name(git_dir.name + ".partial") | ||
| if tmp.exists(): | ||
| shutil.rmtree(tmp) | ||
| _git(None, "clone", "--bare", "--filter=blob:none", "--", url, str(tmp)) | ||
| tmp.rename(git_dir) | ||
| git_dir.parent.mkdir(parents=True, exist_ok=True) | ||
| for old in git_dir.parent.glob(f"{git_dir.name}.*partial"): |
There was a problem hiding this comment.
I agree with exai-bot here; the expired-partial cleanup sits after the HEAD early return, so it is skipped for good once a valid clone exists. Repro on a local bare origin: a valid clone x.git (with HEAD), plus x.git.1-abc.partial with mtime 4000 s old and a timeout of 1800 s. clone_once(x.git, ..., 1800) returns x.git and the partial is still there afterwards (old partial still exists: True). The same happens through ensure_clone and source_git_dir. This is the case the PR text describes: a run killed mid-clone while another run wins the rename. The PR text says the next clone of that URL removes it, but there is never a next clone.
Suggestion: run the loop at lines 478-484 before the HEAD check, or move it into a helper called from both paths, and add a test with a valid cached clone plus an aged partial.
Summary
Two Medium items of #687:
repo run's on-disk state broke on a crash in the middle of setting the git config aside, and when two runs used one clone cache at once.repos/grading.py)._set_git_config_asidedecided whether this was the first grade from whether{run_dir}/agent-git/existed, and created that directory before the moves. A run killed after themkdirand before the last move made the next grade of the cell take itself for a regrade: it deleted the agent's.git/config, hooks,config.worktreeandcommondirwithout archiving them. The moves now go to{run_dir}/.agent-git.partial/, which is renamed toagent-git/after the last move. A grade that finds the partial directory finishes the moves from what is still in the workspace.os.rename:workspace/is made inside the run dir, so the move is atomic on one file system. Withshutil.moveacross file systems, a crash halfway left a copy that the redo could not trust..agent-git.partial(the grader's code has the run dir read-write) is replaced, not moved into. The leading dot means noimage.envscratch entry can have that name..gitis a file, the move ignored the first-grade check. A regrade moved whatever.gitfile a grader left over the archivedagent-git/dot-git. It is now removed, like the other config sources on a regrade.repos/source.py).ensure_clonecloned into a fixed<git_dir>.partial. A second process deleted the first one's partial clone, or cloned into it after the first's rename and then failed renaming over the finished clone. The newclone_oncegives each call its own<git_dir>.<pid>-<random>.partial. A call whose rename fails uses the clone already there when it has aHEAD. A partial directory older than its clone's git timeout belongs to no live clone and is removed, as the fixed.partialwas before.source_git_dir(repos/workspace.py, the full clone that task bases are checked out from) had the same race in the same cache. It goes throughclone_oncetoo, with its own timeout.cannot lock ref 'refs/heads/b1': is at … but expected …. The fetch is now run again, up to three times, when it failed on a ref lock. By then the other fetch has updated the refs. The fetch runs withLC_ALL=C, asschema_compat's git calls do, so a translated git still prints the text that is matched. Any other fetch failure is raised as before, after one attempt.No lock file and no waiting: a run that loses either race uses the winner's result, and this works the same on Windows.
Refs #687: closes the
repos/grading.py:267box and therepos/source.py:461sub-item of "Concurrent writers collide". The other two sub-items there (caching.py,catalog_probe.py) are in #697.Changed output
None for a run that does not crash or race. On a crash before grading, the run dir can hold
.agent-git.partial/until the cell's next grade. A killed clone leaves<name>.git.<pid>-<random>.partialin the repo cache ($DOKIMI_REPO_CACHE) instead of<name>.git.partial, and the next clone of that URL removes it once it is older than the git timeout (30 min). A failed ref fetch reports git's error in English.Evidence
tests/test_repo_grading_aside.py: a crash after 0, 1 or 3 of the 4 moves, then a normal grade, archives every source (on main: the hooks and the rest are gone); a regrade keeps the first.gitfile (on main:'gitdir: /grader/repo' == 'gitdir: /agent/repo'). These pass on main: a regrade keeps the first config and drops the grader's; a link in place of the partial archive is not moved into (that directory is new).tests/test_repo_source_clone_race.py, on a local bare origin, no network: two threads past their checks before either clones (on main:Directory not emptyon the rename); a secondensure_clonestarted while the first is between its clone and its rename (on main:FileNotFoundError, its partial deleted); the same forsource_git_dir; a partial older than the timeout is removed and a young one kept; a fetch whoserefs/heads/topic.lockis held by "another run" is run again, also underLC_ALL=de_DE.UTF-8with git's text translated (on main:cannot lock ref). These pass on main: a fetch failing for another reason runs once; a full-clone partial is aged byworkspace's own timeout.ensure_rev(<branch>)at once, 20 rounds. Before the retry: 20 of 40 fetches failed. After: 0 of 40.agent-git/made before the moves; the first-grade check dropped; nothing cleared on a regrade;.agent-git.partialreused when it is a link; the rename failure raised even whenHEADexists; the per-process name made fixed again; any partial taken as stale; the partial not removed after a lost rename;source._GIT_TIMEOUT_Sused for the full clone; no fetch retry; a retry on any fetch failure. One mutant survived and is equivalent:mkdir(exist_ok=True)onagent-git/after the rename.ensure_clone('file:///nonexistent/repo.git', …):SourceError: git clone failed: fatal: Could not read from remote repository., and nothing is left in the cache.shutil.movereplaced with a rename; each clone's own timeout for the stale rule;LC_ALL=Cfor the matched fetch error.make ci-local --offline: 0 failed, 21 passed, 19 skipped (19056 tests passed).check_growth.py: clean. Not run: the Windows leg (these tests are not inwindows-smoke), and git with real translations installed (this machine has none; the test emulates one).Not in this PR
_exportinrepos/workspace.pyalso uses a fixed<dest>.partial. Its destinations are under the batch's_eval/, whichbatch_lockalready holds for one invocation, so it is not this race.fetch origin -- <sha>) inensure_revandsource_git_dirwrites no refs, onlyFETCH_HEAD; it is not retried.Checklist
pytestpasses: two new test files, 14 tests; the full suite inmake ci-localruff,mypy, and schema checks pass:make ci-local --offlinestatic and typecheck rows pass; no schema changesdocs/repo-evaluation-contract.mdsays the config is moved to{run_dir}/agent-git/, which still holdsAGENTS.md: mutations, other sites of the bug class (source_git_dirfixed,_exportlisted), a bad URL run.github/.licenserc.yaml): both test files have it; the changelog fragment is exempt