From 0485dd3161364e61da70cd276b18e45ede4e364c Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 4 Dec 2023 13:37:58 -0400 Subject: [PATCH] sync: Fix locking problems during merge when annex.pidlock is set Presumably git merge sometimes needs to verifiy if a worktree file is modified, and so will then run git-annex filter-process which would try to take the pid lock. And for whatever reason, git-annex sync already had the pidlock held. I have not replicated that, but it does make enough sense to deploy the workaround. Like I said back in commit 7bdb0cdc0d6f35d3835e57c3a34740cc98f89719, Arguably, it would be better to have a way to make any process git-annex runs have the env var set. But then it would need to take the pid lock when running any and all processes, and that would be a problem when git-annex runs two processes concurrently. So, I'm left doing it ad-hoc in places where git-annex really does run a child process, directly or indirectly via a particular git command. Sponsored-by: KDM on Patreon --- CHANGELOG | 1 + Command/Sync.hs | 20 +++++++------- ...dition_or_double-locking_with_pidlock.mdwn | 2 ++ ..._bd92615c640ff017cd8a2ebfcc117a37._comment | 27 +++++++++++++++++++ 4 files changed, 41 insertions(+), 9 deletions(-) create mode 100644 doc/bugs/Race_condition_or_double-locking_with_pidlock/comment_1_bd92615c640ff017cd8a2ebfcc117a37._comment diff --git a/CHANGELOG b/CHANGELOG index de7bca3074..bf4b7fe36d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -3,6 +3,7 @@ git-annex (10.20231130) UNRELEASED; urgency=medium * Make git-annex get/copy/move --from foo override configuration of remote.foo.annex-ignore, as documented. * Support git-annex copy/move --from-anywhere --to remote. + * sync: Fix locking problems during merge when annex.pidlock is set. -- Joey Hess Thu, 30 Nov 2023 14:48:12 -0400 diff --git a/Command/Sync.hs b/Command/Sync.hs index 851776f95f..3a3d039a89 100644 --- a/Command/Sync.hs +++ b/Command/Sync.hs @@ -76,6 +76,7 @@ import Annex.TaggedPush import Annex.CurrentBranch import Annex.Import import Annex.CheckIgnore +import Annex.PidLock import Types.FileMatcher import Types.GitConfig import Types.Availability @@ -352,15 +353,16 @@ mergeConfig mergeunrelated = do ] merge :: CurrBranch -> [Git.Merge.MergeConfig] -> SyncOptions -> Git.Branch.CommitMode -> [Git.Branch] -> Annex Bool -merge currbranch mergeconfig o commitmode tomergel = do - canresolvemerge <- if resolveMergeOverride o - then getGitConfigVal annexResolveMerge - else return False - and <$> case currbranch of - (Just b, Just adj) -> forM tomergel $ \tomerge -> - mergeToAdjustedBranch tomerge (b, adj) mergeconfig canresolvemerge commitmode - (b, _) -> forM tomergel $ \tomerge -> - autoMergeFrom tomerge b mergeconfig commitmode canresolvemerge +merge currbranch mergeconfig o commitmode tomergel = + runsGitAnnexChildProcessViaGit $ do + canresolvemerge <- if resolveMergeOverride o + then getGitConfigVal annexResolveMerge + else return False + and <$> case currbranch of + (Just b, Just adj) -> forM tomergel $ \tomerge -> + mergeToAdjustedBranch tomerge (b, adj) mergeconfig canresolvemerge commitmode + (b, _) -> forM tomergel $ \tomerge -> + autoMergeFrom tomerge b mergeconfig commitmode canresolvemerge syncBranch :: Git.Branch -> Git.Branch syncBranch = Git.Ref.underBase "refs/heads/synced" . origBranch diff --git a/doc/bugs/Race_condition_or_double-locking_with_pidlock.mdwn b/doc/bugs/Race_condition_or_double-locking_with_pidlock.mdwn index db93536ffb..4347fc2e06 100644 --- a/doc/bugs/Race_condition_or_double-locking_with_pidlock.mdwn +++ b/doc/bugs/Race_condition_or_double-locking_with_pidlock.mdwn @@ -55,3 +55,5 @@ Could this be caused by e.g. git annex running git merge which runs git annex fi ### Have you had any luck using git-annex before? (Sometimes we get tired of reading bug reports all day and a lil' positive end note does wonders) Lots! This problem popped up during our regular use of git-annex in plant genomic research, where we use git annex to manage and move our analyses between the many clusters we must use for computation. Git annex is indispensable for this use case!! + +> [[fixed|done]] (for the merge case at least) --[[Joey]] diff --git a/doc/bugs/Race_condition_or_double-locking_with_pidlock/comment_1_bd92615c640ff017cd8a2ebfcc117a37._comment b/doc/bugs/Race_condition_or_double-locking_with_pidlock/comment_1_bd92615c640ff017cd8a2ebfcc117a37._comment new file mode 100644 index 0000000000..daa07df33f --- /dev/null +++ b/doc/bugs/Race_condition_or_double-locking_with_pidlock/comment_1_bd92615c640ff017cd8a2ebfcc117a37._comment @@ -0,0 +1,27 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 1""" + date="2023-12-04T17:06:11Z" + content=""" +> Could this be caused by e.g. git annex running git merge which runs git annex +> filterprocess (directly or via git status), and git-annex-filterprocess tries +> to take the pidlock that git-annex-sync already has? + +If so you should be able to see the two git-annex's running in ps when this hang occurs +with the git process in between them. +That would be very helpful to verify since it would narrow down the git command +that runs this git-annex child process. + +git-annex does deal with exactly this kind of scenario when running eg +`git update-index`, by setting an environment variable to tell the child git-annex +process not to re-lock the pid lock. But not in eg the case of a git merge. + +So, if you can narrow it down specifically to `git merge` running when this +happens, or whatever other command, it should be easy to fix it. + +Hmm, I suppose that it makes sense `git merge` will sometimes need to check if +a worktree file is modified, and so run git-annex. I'm pretty convinced by your +transcript that it is the git merge that's hanging. So I've gone ahead and put +in the workaround for that. There's of course still the possibility there's +some other git command I've not anticipated that still needs the workaround. +"""]] -- 2.30.2